[U-Boot] [PATCH 5/6] tegra: nand: Add Tegra NAND driver

[Scott Wood <scottwood@freescale.com>]

On 04/13/2012 01:25 PM, Simon Glass wrote:
> Hi Scott,
> 
> On Fri, Apr 13, 2012 at 11:09 AM, Scott Wood <scottwood@freescale.com> wrote:
>> On 04/13/2012 12:42 PM, Simon Glass wrote:
>>> I am not keen on adding cache alignment into every driver - IMO this
>>> should happen at the level above as with MMC, USB, etc. A fairly
>>> trivial fix to nand_base caches most of the problems. I will include a
>>> patch in my next series. Anyway for now, Tegra has cache off because
>>> of all these warnings.
>>
>> Why would nand_base be in a position to know that you have special
>> alignment needs?  Most NAND controllers don't do DMA, and many systems
>> don't require cacheline alignment for DMA.  Linux hasn't needed this,
>> why does U-Boot?
> 
> There is now an ARCH_DMA_MINALIGN define in U-Boot, and a
> ALLOC_CACHE_ALIGN_BUFFER macro just for this purpose. There has been
> some effort in making things like ext2, mmc and USB work properly with
> DMA alignment. I don't see any need to make it hard to the driver - if
> we can avoid bounce buffers we should.

I understand the desire to avoid bounce buffers, but I also don't want
to introduce unnecessary differences between Linux and U-Boot in
nand_base.c.  How does your Linux driver handle this?

>>>>> +int fdt_decode_nand(const void *blob, int node, struct fdt_nand *config)
>>>>> +{
>>>>> +     int err;
>>>>> +
>>>>> +     config->reg = (struct nand_ctlr *)fdtdec_get_addr(blob, node, "reg");
>>>>> +     config->enabled = fdtdec_get_is_enabled(blob, node);
>>>>> +     config->width = fdtdec_get_int(blob, node, "width", 8);
>>>>> +     err = fdtdec_decode_gpio(blob, node, "wp-gpios", &config->wp_gpio);
>>>>> +     if (err)
>>>>> +             return err;
>>>>> +     err = fdtdec_get_int_array(blob, node, "nvidia,timing",
>>>>> +                     config->timing, FDT_NAND_TIMING_COUNT);
>>>>> +     if (err < 0)
>>>>> +             return err;
>>>>> +
>>>>> +     /* Now look up the controller and decode that */
>>>>> +     node = fdt_next_node(blob, node, NULL);
>>>>> +     if (node < 0)
>>>>> +             return node;
>>>>> +
>>>>> +     config->page_data_bytes = fdtdec_get_int(blob, node,
>>>>> +                                             "page-data-bytes", -1);
>>>>> +     config->tag_ecc_bytes = fdtdec_get_int(blob, node,
>>>>> +                                             "tag-ecc-bytes", -1);
>>>>> +     config->tag_bytes = fdtdec_get_int(blob, node, "tag-bytes", -1);
>>>>> +     config->data_ecc_bytes = fdtdec_get_int(blob, node,
>>>>> +                                             "data-ecc-bytes", -1);
>>>>> +     config->skipped_spare_bytes = fdtdec_get_int(blob, node,
>>>>> +                                             "skipped-spare-bytes", -1);
>>>>> +     config->page_spare_bytes = fdtdec_get_int(blob, node,
>>>>> +                                             "page-spare-bytes", -1);
>>>>
>>>> Has this binding been accepted into Linux's documentation or another
>>>> canonical source?
>>>
>>> No
>>
>> It would be good to get that settled first.  I assume you'll want to use
>> this binding with Linux eventually.
> 
> I am not sure - from what I hear Linux does ID lookup instead - but
> really this binding is only useful without ONFI support I suspect.

I'm not sure what difference you're seeing between U-Boot and Linux.
Both U-Boot and Linux uses the ID table in the absence of ONFI.

>> This code also needs better namespacing -- this isn't applicable for all
>> NAND controllers/bindings.
> 
> Do you mean the "nvidia," prefix?

No, I mean the fact that this is a global function called
"fdt_decode_nand", taking a "struct fdt_nand" argument, that is specific
to one controller type.

-Scott