[U-Boot] [PATCH 2/3] spi: Add support for the Aspeed ast2500 SPI controllers

[Cédric Le Goater]

Hello Simon,

[...]

>>>> +/* CEx Control Register */
>>>> +#define CE_CTRL_IO_MODE_MASK           GENMASK(30, 28)
>>>> +#define CE_CTRL_IO_DUAL_DATA           BIT(29)
>>>> +#define CE_CTRL_IO_DUAL_ADDR_DATA      (BIT(29) | BIT(28))
>>>> +#define CE_CTRL_CMD_SHIFT              16
>>>> +#define CE_CTRL_CMD_MASK               0xff
>>>> +#define CE_CTRL_CMD(cmd)                                       \
>>>> +       (((cmd) & CE_CTRL_CMD_MASK) << CE_CTRL_CMD_SHIFT)
>>>> +#define CE_CTRL_DUMMY_HIGH_SHIFT       14
>>>> +#define CE_CTRL_CLOCK_FREQ_SHIFT       8
>>>> +#define CE_CTRL_CLOCK_FREQ_MASK                0xf
>>>> +#define CE_CTRL_CLOCK_FREQ(div)                                                \
>>>> +       (((div) & CE_CTRL_CLOCK_FREQ_MASK) << CE_CTRL_CLOCK_FREQ_SHIFT)
>>>
>>> Do you need this, and other macros like it? I suppose you do use them
>>> twice in the code, at least.
>>
>> CE_CTRL_CLOCK_FREQ() is used twice in aspeed_spi_flash_init().
>>
>> Are you suggesting that we should not use such macros ? I agree this one is
>> not very useful.
> 
> Yes I think it is better to just spell it out in the code. Defining
> shifts and masks is good but creating macros with them can make the
> code more confusing I think and it is less common to do that in
> U-Boot.

ok. 

> BTW the mask is normally a shifted mask:
> 
>  +#define CE_CTRL_CLOCK_FREQ_SHIFT       8
>  +#define CE_CTRL_CLOCK_FREQ_MASK                (0xf <<
> CE_CTRL_CLOCK_FREQ_SHIFT)
> 
> and you can put it in an anonymous enum if you prefer.

Let's cleanup the defines then.

[...]

>>>> +static int aspeed_spi_flash_init(struct aspeed_spi_priv *priv,
>>>> +                                struct aspeed_spi_flash *flash,
>>>> +                                struct udevice *dev)
>>>> +{
>>>> +       struct spi_flash *spi_flash = dev_get_uclass_priv(dev);
>>>> +       struct spi_slave *slave = dev_get_parent_priv(dev);
>>>> +       u32 user_hclk;
>>>> +       u32 read_hclk;
>>>> +
>>>> +       /*
>>>> +        * The SPI flash device slave should not change, so initialize
>>>> +        * it only once.
>>>> +        */
>>>> +       if (flash->init)
>>>> +               return 0;
>>>
>>> Why does the init happen here?
>>
>> I would rather do it under the probe routine but the flash device is probed
>> later in the sequence. I do agree it's a bit awkard and looks like a work
>> around.
>>
>> We could also do the setting each time the device claims the bus, like in
>> the stm32_qspi driver ?
> 
> I think the init option is OK, basd on what you said. But please add a
> comment to the var explaining why it is needed.

ok. will do.

> How come the flash is not available when the driver is probed? 

The flash device OF nodes are available, this is how we get the count 
of CS, but when controller is probed, the flash device properties are 
unknown which makes sense as we need a working SPI controller to probe 
the SPI slave flash devices. 

> Could you probe whatever else is needed first, so that this IS available?

I would love to but I don't have much control on device_probe() and 
spi_get_bus_and_cs() ... :/

 
> [..]
>>>
>>> This is a SPI driver so it should implement the SPI interface. It
>>> should not be messing with SPI flash things.
>>
>> I would agree but the Aspeed SoC FMC controller and the two SPI controllers
>> are designed for SPI flash memory devices (and also NOR flash memory for the
>> FMC)
>>
>>> I have a hard time understanding why this driver knows about SPI
>>> flash devices?
>>
>> because I made look like a SPI-NOR driver I suppose. Should it be in its
>> own uclass category ?
>>
> 
> As above, let's check with Jagan on SPI nor.
> 
> With the x86 driver (ich.c) we got around it by trying to make the SPI
> controller look like a normal SPI controller.

Ah. I will take a look.

Thanks,

C.