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

[Simon Glass]

Hi Scott,

On Fri, Apr 13, 2012 at 11:34 AM, Scott Wood <scottwood at freescale.com> wrote:
> On 04/13/2012 01:25 PM, Simon Glass wrote:
>> Hi Scott,
>>
>> On Fri, Apr 13, 2012 at 11:09 AM, Scott Wood <scottwood at 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?

I have included a patch with the changes in the next version, it's
pretty minimal.

I don't see a linux driver in mainline Linux. We did have one in our
tree but it seems to be gone also. That one had lots of TODOs, and
used bounce buffers from what I can tell.

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

I don't think this driver supports ONFI. I haven't tried it though.

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

OK, so rename struct fdt_nand to struct fdt_tegra_nand or similar? I
will do this in the next version.

>
> -Scott
>

Regards,
Simon