Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT property

[Masahiro Yamada]

Hi Boris,


2016-03-29 16:53 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>:
> Hi,
>
> I'm answering to this one, but I already saw your v2.
>
> On Sat, 26 Mar 2016 13:27:50 +0900
> Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:
>
>> 2016-03-25 23:45 GMT+09:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>:
>> > On Thu, 24 Mar 2016 21:24:37 +0900
>> > Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:
>> >
>> >> Commit 271707b1d817 ("mtd: nand: denali: max_banks calculation
>> >> changed in revision 5.1") supported the new encoding of the "n_banks"
>> >> bits of the "features" register, but there is an unfortunate case
>> >> that is not covered by that commit.
>> >>
>> >> Panasonic (its System LSI division is now Socionext) bought several
>> >> versions of this IP.  The IP released for Panasonic around Feb. 2012
>> >> is revision 5 and uses the old encoding for n_banks (2 << n_banks).
>> >> While the one released around Nov. 2012 is also revision 5, but it
>> >> uses the new encoding (1 << n_banks).
>> >>
>> >> The revision register cannot distinguish these two incompatible
>> >> hardware.  I guess this IP series is not well-organized.  I could not
>> >> find any alternative but giving max_banks from DT property.
>> >
>> > Hm, shouldn't that be addressed with a new compatible instead of adding
>> > a extra property?
>
> You didn't answer to this suggestion. I see several advantages in this
> approach:
>
> 1/ You'll only break the DT once (to add your new compatible) even if
> you got your logic wrong. All you'll have to do in this case is patch
> your driver to change the compatible <-> capabilities association
>
> 2/ This may not be the only difference between the 2 revisions, and
> in this case, putting the compatible <-> capabilities association
> directly in your driver will allow you to easily tweak capabilities
> per-revision
>
>> >
>> >>
>> >> This commit works around the problem by allowing DT to set the
>> >> max_banks forcibly.  Of course, this DT property can be optional if
>> >> the auto detection based on the hardware registers works well.
>> >>
>> >> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
>> >> ---
>> >>
>> >>  Documentation/devicetree/bindings/mtd/denali-nand.txt | 4 ++++
>> >>  drivers/mtd/nand/denali.c                             | 3 ++-
>> >>  drivers/mtd/nand/denali_dt.c                          | 3 +++
>> >>  3 files changed, 9 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
>> >> index 785b825..78c250d 100644
>> >> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
>> >> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
>> >> @@ -7,6 +7,10 @@ Required properties:
>> >>    - interrupts : The interrupt number.
>> >>    - dma-mask : DMA bit mask
>> >>
>> >> +Optional properties:
>> >> +  - max-banks : Maximum number of banks supported by hardware.  If not
>> >> +    specified, it is determined based on the "features" register of hardware.
>> >> +
>> >
>> > You might want to prefix that with "denali,".
>> >
>> >>  The device tree may optionally contain sub-nodes describing partitions of the
>> >>  address space. See partition.txt for more detail.
>>
>>
>> In which case, do we have to add a vendor prefix to DT properties?
>>
>> I do not know a clear rule about this.
>
> Usually you add a vendor prefix when the property only applies to a
> specific controller/IP. In the NAND specific case, most NAND
> controllers have a fixed number of banks which can be deduced from the
> IP revision/version. I'd like to keep it like that and avoid seeing
> other drivers use this max-banks property to deduce the number of
> available banks, hence my suggestion to, at least, prefix your property
> with "denali,". But I'd still prefer to see the max-banks value be
> associated to a new compatible.
>
> Thanks,
>
> Boris

I want to use this driver for ARM-based UniPhier SoCs.
Their DT files are located as follows:

arch/arm/boot/dts/uniphier-*
arch/arm64/boot/dts/socionext/uniphier-*


If we decided to add new DT compatible strings rather than DT property,
which string would you suggest?
Should it include "denali" or just SoC name "uniphier-*"?
How about the vendor prefix?  "denali," or "socionext," ?


like this?  I am not sure...


nand: nand@68000000 {
        compatible = "socionext,uniphier-pro5-nand", "denali,denali-nand-dt";
        reg-names = "nand_data", "denali_reg";
        reg = <0x68000000 0x20>, <0x68100000 0x1000>;
        interrupts = <0 65 4>;
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_nand>;
        clocks = <&nandclk>;
};


At least, I need two DT compatible strings,
for old/new "n_banks" encoding.

The following part is also a problem for me because
platform-specific ECC bit is hard-coded in the driver.

The comment claims that Intel MRST supports 8bit and 15bit for ecc.strenth,
while the Denali IP on UniPhier SoCs supports 8bit, 16bit, and 24bit ECC.

I need to do something with it to proceed, but the code is crappy.


        /*
         * Denali Controller only support 15bit and 8bit ECC in MRST,
         * so just let controller do 15bit ECC for MLC and 8bit ECC for
         * SLC if possible.
         * */
        if (!nand_is_slc(&denali->nand) &&
                        (mtd->oobsize > (denali->bbtskipbytes +
                        ECC_15BITS * (mtd->writesize /
                        ECC_SECTOR_SIZE)))) {
                /* if MLC OOB size is large enough, use 15bit ECC*/
                denali->nand.ecc.strength = 15;
                denali->nand.ecc.layout = &nand_15bit_oob;
                denali->nand.ecc.bytes = ECC_15BITS;
                iowrite32(15, denali->flash_reg + ECC_CORRECTION);
        } else if (mtd->oobsize < (denali->bbtskipbytes +
                        ECC_8BITS * (mtd->writesize /
                        ECC_SECTOR_SIZE))) {
                pr_err("Your NAND chip OOB is not large enough to
contain 8bit ECC correction codes");
                goto failed_req_irq;
        } else {
                denali->nand.ecc.strength = 8;
                denali->nand.ecc.layout = &nand_8bit_oob;
                denali->nand.ecc.bytes = ECC_8BITS;
                iowrite32(8, denali->flash_reg + ECC_CORRECTION);
        }








-- 
Best Regards
Masahiro Yamada