linux-kernel - Re: [PATCH] mtd: nand: denali: allow to override max

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160329095305.46229ad5@bbrezillon>
Date:	Tue, 29 Mar 2016 09:53:05 +0200
From:	Boris Brezillon <boris.brezillon@...e-electrons.com>
To:	Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:	linux-mtd@...ts.infradead.org,
	Graham Moore <grmoore@...nsource.altera.com>,
	Dinh Nguyen <dinguyen@...nsource.altera.com>,
	devicetree@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Richard Weinberger <richard@....at>,
	Kumar Gala <galak@...eaurora.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Brian Norris <computersforpeace@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	David Woodhouse <dwmw2@...radead.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH] mtd: nand: denali: allow to override max_banks from DT
 property

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@...ionext.com> wrote:

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