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

[Simon Glass]

Hi Scott,

On Fri, Apr 13, 2012 at 12:17 PM, Scott Wood <scottwood at freescale.com> wrote:
> On 04/13/2012 01:52 PM, Simon Glass wrote:
>> 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?
>
> 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?

I have not seen that problem, but ARCH_DMA_MINALIGN is a system option
- it generally comes from CONFIG_SYS_CACHELINE_SIZE which can be
adjusted on a per-board basis if needed.

I am rather hoping that we could avoid bounce buffers. If you think
about it, the implications of every driver in every subsystem having
to add this logic are not good. While it might feel a bit like a
fiddle, it is more efficient on code size and complexity to do this
stuff higher up if possible.

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

Yes that's probably it.

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

Yes.

>
> -Scott
>

Regards,
Simon