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

[Simon Glass]

Hi Cedric,

On 28 September 2018 at 04:42, Cédric Le Goater <clg at kaod.org> wrote:
> Hello Simon,
>
>
> The Aspeed AST2500 FMC controller can handle SPI flash and NOR flash memory,
> and the Aspeed AST2500 SPI Flash Controllers only SPI. If there is some
> misunderstanding on this driver, it might come from the fact it is closer
> to a SPI-NOR driver like we have in Linux, than a generic SPI driver.
> The stm32 SPI driver is somewhat similar.
>
> Should we move it under drivers/mtd/spi/ maybe ?
>
> Nevertheless, I think I can improve the dependency on the spi_flash driver
> and probably remove the aspeed_spi_flash struct.

OK, I am not sure of the best thing to do.

Jagan (on cc) has been working on SPI NOR, but I'm not sure of the status.

If you use UCLASS_SPI_FLASH then you can put in drivers/mtd/spi, but
if UCLASS_SPI then drivers/spi
[..]

>>> +/* 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.

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.

[...]

>
>>> +/* DMA Control/Status Register */
>>> +#define DMA_CTRL_DELAY_SHIFT           8
>>> +#define DMA_CTRL_DELAY_MASK            0xf
>>> +#define DMA_CTRL_FREQ_SHIFT            4
>>> +#define DMA_CTRL_FREQ_MASK             0xf
>>> +#define TIMING_MASK(div, delay)                                           \
>>> +       (((delay & DMA_CTRL_DELAY_MASK) << DMA_CTRL_DELAY_SHIFT) | \
>>> +        ((div & DMA_CTRL_FREQ_MASK) << DMA_CTRL_FREQ_SHIFT))
>>
>> Just move this code down below?
>
> 'below' as 'closer' to aspeed_spi_fmc_checksum() ?

I mean remove the #define if it is only used once
[...]

>>> +
>>> +struct aspeed_spi_flash {
>>
>> struct comment - what is this for?
>>
>>> +       u8              cs;
>>> +       bool            init;           /* Initialized when the SPI bus is
>>> +                                        * first claimed
>>> +                                        */
>>
>> Please avoid this type of comment - either put it before the line, or
>> add a struct comment with each member listed.
>
> yes. This will be better.
>
>> Also, can you drop this variable and do the init in the probe() method instead?
>
> I haven't found a good way to do this.
>
> The problem is that the SPI slave flash devices are not available yet when
> the controller is probed. So I am using the claim_bus() method to initialize
> the settings related to each SPI device.
>
> This is what the struct aspeed_spi_flash is about.
>
>>> +       void __iomem    *ahb_base;      /* AHB Window for this device */
>>
>> Should that be a struct *?
>
> no. This is really just the memory address where the flash contents is mapped.
> Depending on the configured controller's mode, accesses will be interpreted
> as direct to the flash contents or as a command/control way to do SPI transfers.
>
> But the controller has no registers behind this address.
>
>>> +       u32             ahb_size;       /* AHB Window segment size */
>>> +       u32             ce_ctrl_user;   /* CE Control Register for USER mode */
>>> +       u32             ce_ctrl_fread;  /* CE Control Register for FREAD mode */
>>> +       u32             iomode;
>>> +
>>> +       struct spi_flash *spi;          /* Associated SPI Flash device */
>>
>> You should not need this struct here with driver model.
>
> OK. I think I can simplify as I only need the size and the dummy_bytes from
> the spi_flash struct. It felt convenient at the time.

But you should be able to access it from the struct udevice *. Thie
struct spi_flash is available using dev_get_uclass_priv(dev) where dev
is the SPI flash device.


>
>>
>>> +};
>>> +
>>> +struct aspeed_spi_priv {
>>> +       struct aspeed_spi_regs  *regs;
>>> +       void __iomem    *ahb_base;      /* AHB Window for all flash devices */
>>> +       u32             ahb_size;       /* AHB Window segments size */
>>> +
>>> +       ulong           hclk_rate;      /* AHB clock rate */
>>> +       u32             max_hz;
>>> +       u8              num_cs;
>>> +       bool            is_fmc;
>>> +
>>> +       struct aspeed_spi_flash flashes[ASPEED_SPI_MAX_CS];
>>
>> SPI flash should be modelled as UCLASS_SPI_FLASH devices. Much of the
>> code in here looks like it should move to a separate driver.
>
> This struct aspeed_spi_flash holds all the parameters related to a SPI slave
> flash device. The confusion surely comes from the fact the driver looks like
> the SPI-NOR driver we have in Linux.

Yes, I think I have to defer to Jagan on this one.

[..]

>>> +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.

How come the flash is not available when the driver is probed? Could
you probe whatever else is needed first, so that this IS available?

[..]
>>
>> 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.

Regards,
Simon