Re: [PATCH 4/5] dt-binding: mtd: onenand/samsung: Add device tree support

[Tomasz Figa]

2019å5æ2æ(æ) 15:36 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>:
>
> Hi Tomasz,
>
> On Thu, 2 May 2019 15:23:33 +0900
> Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote:
>
> > 2019å5æ2æ(æ) 10:54 Rob Herring <robh@xxxxxxxxxx>:
> > >
> > > On Fri, Apr 26, 2019 at 06:42:23PM +0200, PaweÅ Chmiel wrote:
> > > > From: Tomasz Figa <tomasz.figa@xxxxxxxxx>
> > > >
> > > > This patch adds dt-bindings for Samsung OneNAND driver.
> > > >
> > > > Signed-off-by: Tomasz Figa <tomasz.figa@xxxxxxxxx>
> > > > Signed-off-by: PaweÅ Chmiel <pawel.mikolaj.chmiel@xxxxxxxxx>
> > > > ---
> > > >  .../bindings/mtd/samsung-onenand.txt          | 46 +++++++++++++++++++
> > > >  1 file changed, 46 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mtd/samsung-onenand.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mtd/samsung-onenand.txt b/Documentation/devicetree/bindings/mtd/samsung-onenand.txt
> > > > new file mode 100644
> > > > index 000000000000..341d97cc1513
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mtd/samsung-onenand.txt
> > > > @@ -0,0 +1,46 @@
> > > > +Device tree bindings for Samsung SoC OneNAND controller
> > > > +
> > > > +Required properties:
> > > > + - compatible : value should be either of the following.
> > > > +   (a) "samsung,s3c6400-onenand" - for onenand controller compatible with
> > > > +       S3C6400 SoC,
> > > > +   (b) "samsung,s3c6410-onenand" - for onenand controller compatible with
> > > > +       S3C6410 SoC,
> > > > +   (c) "samsung,s5pc100-onenand" - for onenand controller compatible with
> > > > +       S5PC100 SoC,
> > > > +   (d) "samsung,s5pv210-onenand" - for onenand controller compatible with
> > > > +       S5PC110/S5PV210 SoCs.
> > > > +
> > > > + - reg : two memory mapped register regions:
> > > > +   - first entry: control registers.
> > > > +   - second and next entries: memory windows of particular OneNAND chips;
> > > > +     for variants a), b) and c) only one is allowed, in case of d) up to
> > > > +     two chips can be supported.
> > > > +
> > > > + - interrupt-parent : phandle of interrupt controller to which the OneNAND
> > > > +   controller is wired,
> > >
> > > This is implied and can be removed.
> > >
> > > > + - interrupts : specifier of interrupt signal to which the OneNAND controller
> > > > +   is wired; should contain just one entry.
> > > > + - clock-names : should contain two entries:
> > > > +   - "bus" - bus clock of the controller,
> > > > +   - "onenand" - clock supplied to OneNAND memory.
> > >
> > > If the clock just goes to the OneNAND device, then it should be in the
> > > nand device node rather than the controller node.
> > >
> >
> > (Trying hard to recall the details about this hardware.)
> > AFAIR the clock goes to the controller and the controller then feeds
> > it to the memory chips.
> >
> > Also I don't think we should have any nand device nodes here, since
> > the memory itself is only exposed via the controller, which offers
> > various queries to probe the memory at runtime, so there is no need to
> > describe it in DT.
>
> It's probably true, though not providing this controller/device
> separation for NAND controller/devices has proven to be a mistake for
> raw NAND controllers (some props apply to the controllers and others to
> the NAND device, not to mention that some controllers support
> interacting with several chips), so, if that's a new binding, I'd
> recommend having this separation even if it's not strictly required.
>

Note that OneNAND is a totally different thing than the typical NAND
memory with NAND interface. OneNAND chips have a NOR-like interface,
with internal controller and buffers inside, so technically they can
be even used without any special controller on the SoC, via a generic
parallel host interface and possibly some regular DMA engine for CPU
offload.

The controller design of the SoCs in question further abstracts the
OneNAND's programming interface into a number of high level operations
and attempts to hide the details of the underlying memory, so I don't
see the point of describing the memory in DT here, it would actually
defeat the purpose of this controller.

> >
> > > > + - clock: should contain list of phandles and specifiers for all clocks listed
> > > > +   in clock-names property.
> > > > + - #address-cells : must be 1,
> > > > + - #size-cells : must be 1.
> > >
> > > This implies some child nodes. What are the child nodes?
> > >
> >
> > I can't recall the reason for this unfortunately.
>
> Defining partitions I guess, but we ask people to use the new
> representation with a 'partitions' sub-node now, so this should
> probably be dropped.

Ah, that could be it indeed. Thanks!

Best regards,
Tomasz