[PATCH v2] mtd/spi-nor: Add SPI memory controllers for Aspeed SoCs

[Joel Stanley]

Hello Marek,

Thank you for the review. I have answered a few of your questions;
I'll leave the rest to Cedric.

On Mon, Nov 21, 2016 at 8:13 AM, Marek Vasut <marek.vasut at gmail.com> wrote:
>> diff --git a/drivers/mtd/spi-nor/Kconfig b/drivers/mtd/spi-nor/Kconfig
>> index 4a682ee0f632..96148600fdab 100644
>> --- a/drivers/mtd/spi-nor/Kconfig
>> +++ b/drivers/mtd/spi-nor/Kconfig
>> @@ -76,4 +76,16 @@ config SPI_NXP_SPIFI
>>         Flash. Enable this option if you have a device with a SPIFI
>>         controller and want to access the Flash as a mtd device.
>>
>> +config ASPEED_FLASH_SPI
>
> Should be SPI_ASPEED , see the other controllers and keep the list sorted.

Perhaps SPI_NOR_ASPEED so it's clear it's not a driver for a generic SPI bus?

>
>> +     tristate "Aspeed flash controllers in SPI mode"
>> +     depends on HAS_IOMEM && OF
>> +     depends on ARCH_ASPEED || COMPILE_TEST
>> +     # IO_SPACE_LIMIT must be equivalent to (~0UL)
>> +     depends on !NEED_MACH_IO_H
>
> Why?
>
>> +     help
>> +       This enables support for the New Static Memory Controller
>> +       (FMC) in the Aspeed SoCs (AST2400 and AST2500) when attached
>> +       to SPI nor chips, and support for the SPI Memory controller
>> +       (SPI) for the BIOS.
>
> I think there is a naming chaos between FMC, SMC (as in Static MC) and
> SMC (as in SPI MC).

Yes, you're spot on. This naming chaos comes form the vendor's documentation.

I think we could re-work this sentence to make it clearer.

>> +static int aspeed_smc_read_from_ahb(void *buf, const void __iomem *src,
>> +                                 size_t len)
>> +{
>
> What if start of buf is unaligned ?
>
>> +     if ((((unsigned long)src | (unsigned long)buf | len) & 3) == 0) {
>
> Uh, should use boolean OR, not bitwise or. Also, if you're testing
> pointer for NULL, do if (!ptr) .
>
> if (!src || !buf || !len)
>    return;

That's a different test. We're testing here that the buffers are
aligned to see if we can do a word-at-a-time copy.

If not, it falls through to do a byte-at-a-time copy. I think this
covers your first question about buf being unaligned.

Cedric, perhaps you could create a macro called IS_ALLIGNED to make it
clear what this is doing?

>
> while (...)
>
>> +             while (len > 3) {
>> +                     *(u32 *)buf = readl(src);
>> +                     buf += 4;
>> +                     src += 4;
>> +                     len -= 4;
>> +             }
>> +     }
>> +
>> +     while (len--) {
>> +             *(u8 *)buf = readb(src);
>> +             buf += 1;
>> +             src += 1;
>> +     }
>> +     return 0;
>> +}
>> +/*
>> + * SPI Flash Configuration Register (AST2400 SPI)
>> + */
>> +#define CONFIG_REG                   0x0
>> +#define    CONFIG_ENABLE_CE_INACTIVE     BIT(1)
>> +#define    CONFIG_WRITE                          BIT(0)
>
> #define[space]FOO[tab]BIT(bar)

These are bits within the CONFIG_REG. It follows the same style as
other spi-nor drivers, eg. nxp-spifi.

I think it's somewhat clearer, but if you have a strong preference
against then fair enough.

>
>> +/*
>> + * SPI Flash Configuration Register (AST2500 SPI)
>> + * Type setting Register (AST2500 FMC and AST2400 FMC)
>> + */