[4/5] dt-binding: mtd: onenand/samsung: Add device tree support
diff mbox series

Message ID 20190426164224.11327-5-pawel.mikolaj.chmiel@gmail.com
State Changes Requested
Delegated to: Miquel Raynal
Headers show
Series
  • mtd: onenand/samsung: Add device tree support
Related show

Commit Message

Paweł Chmiel April 26, 2019, 4:42 p.m. UTC
From: Tomasz Figa <tomasz.figa@gmail.com>

This patch adds dt-bindings for Samsung OneNAND driver.

Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 .../bindings/mtd/samsung-onenand.txt          | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/samsung-onenand.txt

Comments

Rob Herring May 2, 2019, 1:54 a.m. UTC | #1
On Fri, Apr 26, 2019 at 06:42:23PM +0200, Paweł Chmiel wrote:
> From: Tomasz Figa <tomasz.figa@gmail.com>
> 
> This patch adds dt-bindings for Samsung OneNAND driver.
> 
> Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
>  .../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.

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

> +
> +For partition table parsing (optional) please refer to:
> + [1] Documentation/devicetree/bindings/mtd/partition.txt
> +
> +Example for an s5pv210 board:
> +
> +	onenand@b0600000 {
> +		compatible = "samsung,s5pv210-onenand";
> +		reg = <0xb0600000 0x2000>, <0xb0000000 0x20000>;
> +		interrupt-parent = <&vic1>;
> +		interrupts = <31>;
> +		clock-names = "bus", "onenand";
> +		clocks = <&clocks NANDXL>, <&clocks DOUT_FLASH>;
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +	};
> -- 
> 2.20.1
>
Tomasz Figa May 2, 2019, 6:23 a.m. UTC | #2
2019年5月2日(木) 10:54 Rob Herring <robh@kernel.org>:
>
> On Fri, Apr 26, 2019 at 06:42:23PM +0200, Paweł Chmiel wrote:
> > From: Tomasz Figa <tomasz.figa@gmail.com>
> >
> > This patch adds dt-bindings for Samsung OneNAND driver.
> >
> > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > ---
> >  .../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.

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

Best regards,
Tomasz
Boris Brezillon May 2, 2019, 6:36 a.m. UTC | #3
Hi Tomasz,

On Thu, 2 May 2019 15:23:33 +0900
Tomasz Figa <tomasz.figa@gmail.com> wrote:

> 2019年5月2日(木) 10:54 Rob Herring <robh@kernel.org>:
> >
> > On Fri, Apr 26, 2019 at 06:42:23PM +0200, Paweł Chmiel wrote:  
> > > From: Tomasz Figa <tomasz.figa@gmail.com>
> > >
> > > This patch adds dt-bindings for Samsung OneNAND driver.
> > >
> > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> > > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > ---
> > >  .../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.

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

Regards,

Boris
Tomasz Figa May 2, 2019, 6:42 a.m. UTC | #4
2019年5月2日(木) 15:36 Boris Brezillon <boris.brezillon@collabora.com>:
>
> Hi Tomasz,
>
> On Thu, 2 May 2019 15:23:33 +0900
> Tomasz Figa <tomasz.figa@gmail.com> wrote:
>
> > 2019年5月2日(木) 10:54 Rob Herring <robh@kernel.org>:
> > >
> > > On Fri, Apr 26, 2019 at 06:42:23PM +0200, Paweł Chmiel wrote:
> > > > From: Tomasz Figa <tomasz.figa@gmail.com>
> > > >
> > > > This patch adds dt-bindings for Samsung OneNAND driver.
> > > >
> > > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> > > > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > > ---
> > > >  .../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
Boris Brezillon May 2, 2019, 6:55 a.m. UTC | #5
On Thu, 2 May 2019 15:42:59 +0900
Tomasz Figa <tomasz.figa@gmail.com> wrote:

> 2019年5月2日(木) 15:36 Boris Brezillon <boris.brezillon@collabora.com>:
> >
> > Hi Tomasz,
> >
> > On Thu, 2 May 2019 15:23:33 +0900
> > Tomasz Figa <tomasz.figa@gmail.com> wrote:
> >  
> > > 2019年5月2日(木) 10:54 Rob Herring <robh@kernel.org>:  
> > > >
> > > > On Fri, Apr 26, 2019 at 06:42:23PM +0200, Paweł Chmiel wrote:  
> > > > > From: Tomasz Figa <tomasz.figa@gmail.com>
> > > > >
> > > > > This patch adds dt-bindings for Samsung OneNAND driver.
> > > > >
> > > > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> > > > > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > > > ---
> > > > >  .../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.

Yes, I know that.

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

I don't see how having a subnode for the NAND chip would change
anything on how the controller interacts with the NAND device. My point
is, if we ever need to add props that apply to the device rather than
the controller itself, having a single node to represent both elements
is not that great.
Tomasz Figa May 2, 2019, 6:58 a.m. UTC | #6
2019年5月2日(木) 15:55 Boris Brezillon <boris.brezillon@collabora.com>:
>
> On Thu, 2 May 2019 15:42:59 +0900
> Tomasz Figa <tomasz.figa@gmail.com> wrote:
>
> > 2019年5月2日(木) 15:36 Boris Brezillon <boris.brezillon@collabora.com>:
> > >
> > > Hi Tomasz,
> > >
> > > On Thu, 2 May 2019 15:23:33 +0900
> > > Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > >
> > > > 2019年5月2日(木) 10:54 Rob Herring <robh@kernel.org>:
> > > > >
> > > > > On Fri, Apr 26, 2019 at 06:42:23PM +0200, Paweł Chmiel wrote:
> > > > > > From: Tomasz Figa <tomasz.figa@gmail.com>
> > > > > >
> > > > > > This patch adds dt-bindings for Samsung OneNAND driver.
> > > > > >
> > > > > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> > > > > > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > > > > ---
> > > > > >  .../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.
>
> Yes, I know that.
>
> >
> > 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.
>
> I don't see how having a subnode for the NAND chip would change
> anything on how the controller interacts with the NAND device. My point
> is, if we ever need to add props that apply to the device rather than
> the controller itself, having a single node to represent both elements
> is not that great.

You mean, just having a very generic onenand@0 node that doesn't
really include any information, except maybe the partition table? I
guess that wouldn't have any negative side effects indeed.

My point was that we don't want to put things like chip vendor, size,
etc. in DT, since that's enumerable.

Best regards,
Tomasz
Boris Brezillon May 2, 2019, 7:21 a.m. UTC | #7
On Thu, 2 May 2019 15:58:24 +0900
Tomasz Figa <tomasz.figa@gmail.com> wrote:

> 2019年5月2日(木) 15:55 Boris Brezillon <boris.brezillon@collabora.com>:
> >
> > On Thu, 2 May 2019 15:42:59 +0900
> > Tomasz Figa <tomasz.figa@gmail.com> wrote:
> >  
> > > 2019年5月2日(木) 15:36 Boris Brezillon <boris.brezillon@collabora.com>:  
> > > >
> > > > Hi Tomasz,
> > > >
> > > > On Thu, 2 May 2019 15:23:33 +0900
> > > > Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > > >  
> > > > > 2019年5月2日(木) 10:54 Rob Herring <robh@kernel.org>:  
> > > > > >
> > > > > > On Fri, Apr 26, 2019 at 06:42:23PM +0200, Paweł Chmiel wrote:  
> > > > > > > From: Tomasz Figa <tomasz.figa@gmail.com>
> > > > > > >
> > > > > > > This patch adds dt-bindings for Samsung OneNAND driver.
> > > > > > >
> > > > > > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> > > > > > > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > > > > > ---
> > > > > > >  .../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.  
> >
> > Yes, I know that.
> >  
> > >
> > > 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.  
> >
> > I don't see how having a subnode for the NAND chip would change
> > anything on how the controller interacts with the NAND device. My point
> > is, if we ever need to add props that apply to the device rather than
> > the controller itself, having a single node to represent both elements
> > is not that great.  
> 
> You mean, just having a very generic onenand@0 node that doesn't
> really include any information, except maybe the partition table?

Yes.

> I
> guess that wouldn't have any negative side effects indeed.
> 
> My point was that we don't want to put things like chip vendor, size,
> etc. in DT, since that's enumerable.

Oh, definitely not, and that's exactly how we do it for NAND devices.
Everything that's discoverable is not described in the DT, but some
things can't be discovered this way (like when you want to override the
ECC strength and use SW-based implem instead of the HW-based one). I
know none of this applies to OneNAND yet, I'm just over-cautious about
that since DT bindings changes are hard to make once the bindings are
in use.
Tomasz Figa May 2, 2019, 8:41 a.m. UTC | #8
2019年5月2日(木) 16:21 Boris Brezillon <boris.brezillon@collabora.com>:
>
> On Thu, 2 May 2019 15:58:24 +0900
> Tomasz Figa <tomasz.figa@gmail.com> wrote:
>
> > 2019年5月2日(木) 15:55 Boris Brezillon <boris.brezillon@collabora.com>:
> > >
> > > On Thu, 2 May 2019 15:42:59 +0900
> > > Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > >
> > > > 2019年5月2日(木) 15:36 Boris Brezillon <boris.brezillon@collabora.com>:
> > > > >
> > > > > Hi Tomasz,
> > > > >
> > > > > On Thu, 2 May 2019 15:23:33 +0900
> > > > > Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > > > >
> > > > > > 2019年5月2日(木) 10:54 Rob Herring <robh@kernel.org>:
> > > > > > >
> > > > > > > On Fri, Apr 26, 2019 at 06:42:23PM +0200, Paweł Chmiel wrote:
> > > > > > > > From: Tomasz Figa <tomasz.figa@gmail.com>
> > > > > > > >
> > > > > > > > This patch adds dt-bindings for Samsung OneNAND driver.
> > > > > > > >
> > > > > > > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> > > > > > > > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> > > > > > > > ---
> > > > > > > >  .../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.
> > >
> > > Yes, I know that.
> > >
> > > >
> > > > 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.
> > >
> > > I don't see how having a subnode for the NAND chip would change
> > > anything on how the controller interacts with the NAND device. My point
> > > is, if we ever need to add props that apply to the device rather than
> > > the controller itself, having a single node to represent both elements
> > > is not that great.
> >
> > You mean, just having a very generic onenand@0 node that doesn't
> > really include any information, except maybe the partition table?
>
> Yes.
>
> > I
> > guess that wouldn't have any negative side effects indeed.
> >
> > My point was that we don't want to put things like chip vendor, size,
> > etc. in DT, since that's enumerable.
>
> Oh, definitely not, and that's exactly how we do it for NAND devices.
> Everything that's discoverable is not described in the DT, but some
> things can't be discovered this way (like when you want to override the
> ECC strength and use SW-based implem instead of the HW-based one). I
> know none of this applies to OneNAND yet, I'm just over-cautious about
> that since DT bindings changes are hard to make once the bindings are
> in use.

Makes sense. Thanks for clarifying!

Patch
diff mbox series

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,
+ - 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.
+ - 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.
+
+For partition table parsing (optional) please refer to:
+ [1] Documentation/devicetree/bindings/mtd/partition.txt
+
+Example for an s5pv210 board:
+
+	onenand@b0600000 {
+		compatible = "samsung,s5pv210-onenand";
+		reg = <0xb0600000 0x2000>, <0xb0000000 0x20000>;
+		interrupt-parent = <&vic1>;
+		interrupts = <31>;
+		clock-names = "bus", "onenand";
+		clocks = <&clocks NANDXL>, <&clocks DOUT_FLASH>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+	};