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

[Boris Brezillon]

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 at socionext.com> wrote:

> 2016-03-25 23:45 GMT+09:00 Boris Brezillon <boris.brezillon at free-electrons.com>:
> > On Thu, 24 Mar 2016 21:24:37 +0900
> > Masahiro Yamada <yamada.masahiro at socionext.com> 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 at socionext.com>
> >> ---
> >>
> >>  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

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com