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

[Scott Wood <scottwood@freescale.com>]

On 04/13/2012 01:52 PM, Simon Glass wrote:
> Hi Scott,
> 
> On Fri, Apr 13, 2012 at 11:37 AM, Scott Wood <scottwood@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@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?

Most NAND drivers do not do DMA, and ARCH_DMA_MINALIGN seems a little
oversimple -- what if I have one device that needs more alignment than
another on the same system?  I guess it's better to just apply the max
alignment of any device if the cost is low, but what if some driver
starts using ARCH_DMA_MINALIGN to determine whether it needs bounce buffers?

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

Maybe databuf[] used to be split into two arrays?

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

They have been useful to the most drivers so far, but I guess this is
OK... we'd have to make one change or another anyway (either add
alignment or convert to pointers).

-Scott