[U-Boot] [PATCH v2 1/7] nand: Try to align the default buffers

[Simon Glass]

Hi Scott,

On Fri, Apr 13, 2012 at 11:37 AM, Scott Wood <scottwood at freescale.com> wrote:
> On 04/13/2012 01:29 PM, Simon Glass wrote:
>> The NAND layer needs to use cache-aligned buffers by default. Towards this
>> goal. align the default buffers and their members according to the minimum
>> DMA alignment defined for the architecture.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>> Changes in v2:
>> - Add new patch to align default buffers in nand_base
>>
>>  drivers/mtd/nand/nand_base.c |    3 ++-
>>  include/linux/mtd/nand.h     |    7 ++++---
>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index 44f7b91..7bfc29e 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -2935,7 +2935,8 @@ int nand_scan_tail(struct mtd_info *mtd)
>>       struct nand_chip *chip = mtd->priv;
>>
>>       if (!(chip->options & NAND_OWN_BUFFERS))
>> -             chip->buffers = kmalloc(sizeof(*chip->buffers), GFP_KERNEL);
>> +             chip->buffers = memalign(ARCH_DMA_MINALIGN,
>> +                                      sizeof(*chip->buffers));
>
> This sort of requirement seems to be what NAND_OWN_BUFFERS was made for.

That's a bit of a cop-out I think. Arguably the current NAND code is
deliberately ignoring DMA alignment and requiring bounce buffers in
the drivers to deal with its ignorance. Other subsystems are changing
over, so what not NAND?

>
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index da6fa18..ae0bdf6 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -391,9 +391,10 @@ struct nand_ecc_ctrl {
>>   * consecutive order.
>>   */
>>  struct nand_buffers {
>> -     uint8_t ecccalc[NAND_MAX_OOBSIZE];
>> -     uint8_t ecccode[NAND_MAX_OOBSIZE];
>> -     uint8_t databuf[NAND_MAX_PAGESIZE + NAND_MAX_OOBSIZE];
>> +     uint8_t ecccalc[ALIGN(NAND_MAX_OOBSIZE, ARCH_DMA_MINALIGN)];
>> +     uint8_t ecccode[ALIGN(NAND_MAX_OOBSIZE, ARCH_DMA_MINALIGN)];
>> +     uint8_t databuf[ALIGN(NAND_MAX_PAGESIZE + NAND_MAX_OOBSIZE,
>> +                           ARCH_DMA_MINALIGN)];
>>  };
>
> I remember a while back someone wanting to change this to be pointers
> intsead of arrays, so that the driver can manage alignment -- I don't
> recall what happened to that.

I was concerned about the comment "Do not change the order of buffers.
databuf and oobrbuf must be in consecutive order." but then I couldn't
find oobrbuf so perhaps it is not true.

Anyway, alignment seems like a small price to pay - if the NAND layer
is going to allocate buffers they may as well be generally useful.

>
> -Scott
>

Regards,
Simon