[PATCH 0/5] mtd: onenand/samsung: Add device tree support

[PATCH 0/5] mtd: onenand/samsung: Add device tree support

[Paweł Chmiel]

This patchset adds device tree support to Samsung OneNAND driver.
It was tested on Samsung Galaxy S and Samsung Galaxy S Fascinate 4G,
an Samsung S5PV210 based mobile phones.

Tomasz Figa (5):
  mtd: onenand/samsung: Unify resource order for controller variants
  mtd: onenand/samsung: Make sure that bus clock is enabled
  mtd: onenand/samsung: Add device tree support
  dt-binding: mtd: onenand/samsung: Add device tree support
  mtd: onenand/samsung: Set name field of mtd_info struct

 .../bindings/mtd/samsung-onenand.txt          | 46 +++++++++
 drivers/mtd/nand/onenand/samsung.c            | 94 +++++++++++++------
 2 files changed, 113 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/samsung-onenand.txt

-- 
2.20.1

[PATCH 1/5] mtd: onenand/samsung: Unify resource order for controller variants

[Paweł Chmiel]

From: Tomasz Figa <tomasz.figa@gmail.com>

Before this patch, the order of memory resources requested by the driver
was controller base as first and OneNAND chip base as second for
S3C64xx/S5PC100 variant and the opposite for S5PC110/S5PV210 variant.

To make this more consistent, this patch swaps the order of resources
for the latter and updates platform code accordingly. As a nice side
effect there is a slight reduction in line count of probe function.

Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 drivers/mtd/nand/onenand/samsung.c | 48 ++++++++++++++----------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/mtd/nand/onenand/samsung.c b/drivers/mtd/nand/onenand/samsung.c
index e64d0fdf7eb5..a425f19a3876 100644
--- a/drivers/mtd/nand/onenand/samsung.c
+++ b/drivers/mtd/nand/onenand/samsung.c
@@ -126,14 +126,13 @@ struct s3c_onenand {
 	struct mtd_info	*mtd;
 	struct platform_device	*pdev;
 	enum soc_type	type;
-	void __iomem	*base;
-	void __iomem	*ahb_addr;
+	void __iomem	*ctrl_base;
+	void __iomem	*chip_base;
 	int		bootram_command;
 	void		*page_buf;
 	void		*oob_buf;
 	unsigned int	(*mem_addr)(int fba, int fpa, int fsa);
 	unsigned int	(*cmd_map)(unsigned int type, unsigned int val);
-	void __iomem	*dma_addr;
 	unsigned long	phys_base;
 	struct completion	complete;
 };
@@ -147,22 +146,22 @@ static struct s3c_onenand *onenand;
 
 static inline int s3c_read_reg(int offset)
 {
-	return readl(onenand->base + offset);
+	return readl(onenand->ctrl_base + offset);
 }
 
 static inline void s3c_write_reg(int value, int offset)
 {
-	writel(value, onenand->base + offset);
+	writel(value, onenand->ctrl_base + offset);
 }
 
 static inline int s3c_read_cmd(unsigned int cmd)
 {
-	return readl(onenand->ahb_addr + cmd);
+	return readl(onenand->chip_base + cmd);
 }
 
 static inline void s3c_write_cmd(int value, unsigned int cmd)
 {
-	writel(value, onenand->ahb_addr + cmd);
+	writel(value, onenand->chip_base + cmd);
 }
 
 #ifdef SAMSUNG_DEBUG
@@ -519,7 +518,7 @@ static int (*s5pc110_dma_ops)(dma_addr_t dst, dma_addr_t src, size_t count, int
 
 static int s5pc110_dma_poll(dma_addr_t dst, dma_addr_t src, size_t count, int direction)
 {
-	void __iomem *base = onenand->dma_addr;
+	void __iomem *base = onenand->ctrl_base;
 	int status;
 	unsigned long timeout;
 
@@ -563,7 +562,7 @@ static int s5pc110_dma_poll(dma_addr_t dst, dma_addr_t src, size_t count, int di
 
 static irqreturn_t s5pc110_onenand_irq(int irq, void *data)
 {
-	void __iomem *base = onenand->dma_addr;
+	void __iomem *base = onenand->ctrl_base;
 	int status, cmd = 0;
 
 	status = readl(base + S5PC110_INTC_DMA_STATUS);
@@ -585,7 +584,7 @@ static irqreturn_t s5pc110_onenand_irq(int irq, void *data)
 
 static int s5pc110_dma_irq(dma_addr_t dst, dma_addr_t src, size_t count, int direction)
 {
-	void __iomem *base = onenand->dma_addr;
+	void __iomem *base = onenand->ctrl_base;
 	int status;
 
 	status = readl(base + S5PC110_INTC_DMA_MASK);
@@ -634,7 +633,7 @@ static int s5pc110_read_bufferram(struct mtd_info *mtd, int area,
 	}
 
 	if (offset & 3 || (size_t) buf & 3 ||
-		!onenand->dma_addr || count != mtd->writesize)
+		!onenand->ctrl_base || count != mtd->writesize)
 		goto normal;
 
 	/* Handle vmalloc address */
@@ -864,23 +863,22 @@ static int s3c_onenand_probe(struct platform_device *pdev)
 	s3c_onenand_setup(mtd);
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	onenand->base = devm_ioremap_resource(&pdev->dev, r);
-	if (IS_ERR(onenand->base))
-		return PTR_ERR(onenand->base);
-
+	onenand->ctrl_base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(onenand->ctrl_base))
+		return PTR_ERR(onenand->ctrl_base);
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	onenand->chip_base = devm_ioremap_resource(&pdev->dev, r);
+	if (IS_ERR(onenand->chip_base))
+		return PTR_ERR(onenand->chip_base);
 	onenand->phys_base = r->start;
 
-	/* Set onenand_chip also */
-	this->base = onenand->base;
-
 	/* Use runtime badblock check */
 	this->options |= ONENAND_SKIP_UNLOCK_CHECK;
 
 	if (onenand->type != TYPE_S5PC110) {
-		r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-		onenand->ahb_addr = devm_ioremap_resource(&pdev->dev, r);
-		if (IS_ERR(onenand->ahb_addr))
-			return PTR_ERR(onenand->ahb_addr);
+		/* Set onenand_chip also */
+		this->base = onenand->ctrl_base;
 
 		/* Allocate 4KiB BufferRAM */
 		onenand->page_buf = devm_kzalloc(&pdev->dev, SZ_4K,
@@ -898,10 +896,8 @@ static int s3c_onenand_probe(struct platform_device *pdev)
 		this->subpagesize = mtd->writesize;
 
 	} else { /* S5PC110 */
-		r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-		onenand->dma_addr = devm_ioremap_resource(&pdev->dev, r);
-		if (IS_ERR(onenand->dma_addr))
-			return PTR_ERR(onenand->dma_addr);
+		/* Set onenand_chip also */
+		this->base = onenand->chip_base;
 
 		s5pc110_dma_ops = s5pc110_dma_poll;
 		/* Interrupt support */
-- 
2.20.1

Re: [PATCH 1/5] mtd: onenand/samsung: Unify resource order for controller variants

[Miquel Raynal]

Hi Paweł,

Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com> wrote on Fri, 26 Apr 2019
18:42:20 +0200:

> From: Tomasz Figa <tomasz.figa@gmail.com>
> 
> Before this patch, the order of memory resources requested by the driver
> was controller base as first and OneNAND chip base as second for
> S3C64xx/S5PC100 variant and the opposite for S5PC110/S5PV210 variant.
> 
> To make this more consistent, this patch swaps the order of resources
> for the latter and updates platform code accordingly. As a nice side
> effect there is a slight reduction in line count of probe function.
> 
> Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>

Thanks for the patch but it looks like you also are renaming fields in
the main onenand structure. Maybe it is worth doing it in a preliminary
patch.

Thanks,
Miquèl

[PATCH 2/5] mtd: onenand/samsung: Make sure that bus clock is enabled

[Paweł Chmiel]

From: Tomasz Figa <tomasz.figa@gmail.com>

This patch adds basic handling of controller bus clock to make sure that
in device probe it is enabled and device can operate correctly. The
clock is optional and driver behavior is identical as before this patch
if not provided.

Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 drivers/mtd/nand/onenand/samsung.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/nand/onenand/samsung.c b/drivers/mtd/nand/onenand/samsung.c
index a425f19a3876..9628bf5bc397 100644
--- a/drivers/mtd/nand/onenand/samsung.c
+++ b/drivers/mtd/nand/onenand/samsung.c
@@ -14,6 +14,7 @@
  *	S5PC110: use DMA
  */
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/sched.h>
@@ -125,6 +126,7 @@ enum soc_type {
 struct s3c_onenand {
 	struct mtd_info	*mtd;
 	struct platform_device	*pdev;
+	struct clk	*clk_bus;
 	enum soc_type	type;
 	void __iomem	*ctrl_base;
 	void __iomem	*chip_base;
@@ -916,6 +918,10 @@ static int s3c_onenand_probe(struct platform_device *pdev)
 		}
 	}
 
+	onenand->clk_bus = devm_clk_get(&pdev->dev, "bus");
+	if (!IS_ERR(onenand->clk_bus))
+		clk_prepare_enable(onenand->clk_bus);
+
 	err = onenand_scan(mtd, 1);
 	if (err)
 		return err;
@@ -947,6 +953,8 @@ static int s3c_onenand_remove(struct platform_device *pdev)
 	struct mtd_info *mtd = platform_get_drvdata(pdev);
 
 	onenand_release(mtd);
+	if (!IS_ERR(onenand->clk_bus))
+		clk_disable_unprepare(onenand->clk_bus);
 
 	return 0;
 }
-- 
2.20.1

Re: [PATCH 2/5] mtd: onenand/samsung: Make sure that bus clock is enabled

[Miquel Raynal]

Hi Paweł,

Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com> wrote on Fri, 26 Apr 2019
18:42:21 +0200:

> From: Tomasz Figa <tomasz.figa@gmail.com>
> 
> This patch adds basic handling of controller bus clock to make sure that
> in device probe it is enabled and device can operate correctly. The
> clock is optional and driver behavior is identical as before this patch
> if not provided.
> 
> Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

[PATCH 3/5] mtd: onenand/samsung: Add device tree support

[Paweł Chmiel]

From: Tomasz Figa <tomasz.figa@gmail.com>

This patch adds support for instantation using Device Tree.

Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 drivers/mtd/nand/onenand/samsung.c | 37 +++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/onenand/samsung.c b/drivers/mtd/nand/onenand/samsung.c
index 9628bf5bc397..0f450604412f 100644
--- a/drivers/mtd/nand/onenand/samsung.c
+++ b/drivers/mtd/nand/onenand/samsung.c
@@ -25,6 +25,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/of.h>
 
 #include "samsung.h"
 
@@ -835,8 +836,36 @@ static void s3c_onenand_setup(struct mtd_info *mtd)
 	this->write_bufferram = onenand_write_bufferram;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id s3c_onenand_of_match[] = {
+	{ .compatible = "samsung,s3c6400-onenand",
+		.data = (void *)TYPE_S3C6400 },
+	{ .compatible = "samsung,s3c6410-onenand",
+		.data = (void *)TYPE_S3C6410 },
+	{ .compatible = "samsung,s5pv210-onenand",
+		.data = (void *)TYPE_S5PC110 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, onenand_s3c_dt_match);
+#endif
+
+static enum soc_type s3c_onenand_get_device_id(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+
+	if (IS_ENABLED(CONFIG_OF) && np) {
+		const struct of_device_id *match;
+
+		match = of_match_node(s3c_onenand_of_match, np);
+		return (enum soc_type)match->data;
+	}
+
+	return platform_get_device_id(pdev)->driver_data;
+}
+
 static int s3c_onenand_probe(struct platform_device *pdev)
 {
+	struct device_node *np = pdev->dev.of_node;
 	struct onenand_platform_data *pdata;
 	struct onenand_chip *this;
 	struct mtd_info *mtd;
@@ -858,9 +887,10 @@ static int s3c_onenand_probe(struct platform_device *pdev)
 
 	this = (struct onenand_chip *) &mtd[1];
 	mtd->priv = this;
+	mtd->dev.of_node = np;
 	mtd->dev.parent = &pdev->dev;
 	onenand->pdev = pdev;
-	onenand->type = platform_get_device_id(pdev)->driver_data;
+	onenand->type = s3c_onenand_get_device_id(pdev);
 
 	s3c_onenand_setup(mtd);
 
@@ -919,6 +949,10 @@ static int s3c_onenand_probe(struct platform_device *pdev)
 	}
 
 	onenand->clk_bus = devm_clk_get(&pdev->dev, "bus");
+	if (np && IS_ERR(onenand->clk_bus)) {
+		dev_err(&pdev->dev, "failed to get bus clock\n");
+		return PTR_ERR(onenand->clk_bus);
+	}
 	if (!IS_ERR(onenand->clk_bus))
 		clk_prepare_enable(onenand->clk_bus);
 
@@ -1000,6 +1034,7 @@ static struct platform_driver s3c_onenand_driver = {
 	.driver         = {
 		.name	= "samsung-onenand",
 		.pm	= &s3c_pm_ops,
+		.of_match_table = of_match_ptr(s3c_onenand_of_match),
 	},
 	.id_table	= s3c_onenand_driver_ids,
 	.probe          = s3c_onenand_probe,
-- 
2.20.1

Re: [PATCH 3/5] mtd: onenand/samsung: Add device tree support

[Miquel Raynal]

Hi Paweł,

Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com> wrote on Fri, 26 Apr 2019
18:42:22 +0200:

> From: Tomasz Figa <tomasz.figa@gmail.com>
> 
> This patch adds support for instantation using Device Tree.

"Add support for..." is enough.

Otherwise:

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

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

[Paweł Chmiel]

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,
+ - 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>;
+	};
-- 
2.20.1

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

[Rob Herring]

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
> 

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

[Tomasz Figa]

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

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

[Boris Brezillon]

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

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

[Tomasz Figa]

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

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

[Boris Brezillon]

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.

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

[Tomasz Figa]

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

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

[Boris Brezillon]

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.

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

[Tomasz Figa]

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 5/5] mtd: onenand/samsung: Set name field of mtd_info struct

[Paweł Chmiel]

From: Tomasz Figa <tomasz.figa@gmail.com>

This patch adds initialization of .name field of mtd_info struct to
avoid printing "(null)" in kernel log messages, such as:

[    1.942519] 1 ofpart partitions found on MTD device (null)
[    1.949708] Creating 1 MTD partitions on "(null)":

Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
---
 drivers/mtd/nand/onenand/samsung.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/nand/onenand/samsung.c b/drivers/mtd/nand/onenand/samsung.c
index 0f450604412f..1fda1f324cc6 100644
--- a/drivers/mtd/nand/onenand/samsung.c
+++ b/drivers/mtd/nand/onenand/samsung.c
@@ -886,6 +886,7 @@ static int s3c_onenand_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	this = (struct onenand_chip *) &mtd[1];
+	mtd->name = dev_name(&pdev->dev);
 	mtd->priv = this;
 	mtd->dev.of_node = np;
 	mtd->dev.parent = &pdev->dev;
-- 
2.20.1

Re: [PATCH 5/5] mtd: onenand/samsung: Set name field of mtd_info struct

[Miquel Raynal]

Hi Paweł,

Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com> wrote on Fri, 26 Apr 2019
18:42:24 +0200:

> From: Tomasz Figa <tomasz.figa@gmail.com>
> 
> This patch adds initialization of .name field of mtd_info struct to
> avoid printing "(null)" in kernel log messages, such as:
> 
> [    1.942519] 1 ofpart partitions found on MTD device (null)
> [    1.949708] Creating 1 MTD partitions on "(null)":
> 
> Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
>  drivers/mtd/nand/onenand/samsung.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/mtd/nand/onenand/samsung.c b/drivers/mtd/nand/onenand/samsung.c
> index 0f450604412f..1fda1f324cc6 100644
> --- a/drivers/mtd/nand/onenand/samsung.c
> +++ b/drivers/mtd/nand/onenand/samsung.c
> @@ -886,6 +886,7 @@ static int s3c_onenand_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	this = (struct onenand_chip *) &mtd[1];
> +	mtd->name = dev_name(&pdev->dev);
>  	mtd->priv = this;
>  	mtd->dev.of_node = np;
>  	mtd->dev.parent = &pdev->dev;


Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>


Thanks,
Miquèl

Re: [PATCH 0/5] mtd: onenand/samsung: Add device tree support

[Miquel Raynal]

Hi Paweł,

Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com> wrote on Fri, 26 Apr 2019
18:42:19 +0200:

> This patchset adds device tree support to Samsung OneNAND driver.
> It was tested on Samsung Galaxy S and Samsung Galaxy S Fascinate 4G,
> an Samsung S5PV210 based mobile phones.
> 
> Tomasz Figa (5):
>   mtd: onenand/samsung: Unify resource order for controller variants
>   mtd: onenand/samsung: Make sure that bus clock is enabled
>   mtd: onenand/samsung: Add device tree support
>   dt-binding: mtd: onenand/samsung: Add device tree support
>   mtd: onenand/samsung: Set name field of mtd_info struct
> 
>  .../bindings/mtd/samsung-onenand.txt          | 46 +++++++++
>  drivers/mtd/nand/onenand/samsung.c            | 94 +++++++++++++------
>  2 files changed, 113 insertions(+), 27 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/mtd/samsung-onenand.txt
> 

I think you should use "mtd: onenand: samsung:" as prefix.

Thanks,
Miquèl

Re: [PATCH 0/5] mtd: onenand/samsung: Add device tree support

[Paweł Chmiel]

On poniedziałek, 29 kwietnia 2019 10:19:28 CEST Miquel Raynal wrote:
> Hi Paweł,
> 
> Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com> wrote on Fri, 26 Apr 2019
> 18:42:19 +0200:
> 
> > This patchset adds device tree support to Samsung OneNAND driver.
> > It was tested on Samsung Galaxy S and Samsung Galaxy S Fascinate 4G,
> > an Samsung S5PV210 based mobile phones.
> > 
> > Tomasz Figa (5):
> >   mtd: onenand/samsung: Unify resource order for controller variants
> >   mtd: onenand/samsung: Make sure that bus clock is enabled
> >   mtd: onenand/samsung: Add device tree support
> >   dt-binding: mtd: onenand/samsung: Add device tree support
> >   mtd: onenand/samsung: Set name field of mtd_info struct
> > 
> >  .../bindings/mtd/samsung-onenand.txt          | 46 +++++++++
> >  drivers/mtd/nand/onenand/samsung.c            | 94 +++++++++++++------
> >  2 files changed, 113 insertions(+), 27 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/mtd/samsung-onenand.txt
> > 
> 
> I think you should use "mtd: onenand: samsung:" as prefix.
> 
> Thanks,
> Miquèl
> 
Hi Miquèl

I'll fix all issues and send new version of patchset.
Thanks for review and comments