[PATCH v3 0/4] NAND: Multi-chip support for FSL-UPM for TQM8548 modules

[PATCH v3 0/4] NAND: Multi-chip support for FSL-UPM for TQM8548 modules

[Wolfgang Grandegger]

This is my third version of the patch series adding generic support
for multi-chip NAND devices to the FSL-UPM driver and support for
the Micron MT29F8G08FAB NAND flash memory on the TQM8548 modules.
It addresses the issues reported on the mailing list, e.g. the new
bindings are now documented as well.

[PATCH v3 1/4] NAND: FSL-UPM: add multi chip support
[PATCH v3 2/4] NAND: FSL-UPM: Add wait flags to support board/chip specific delays
[PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings
[PATCH v3 4/4] powerpc/85xx: TQM8548: Update DTS file for multi-chip support

Please consider these patches for inclusion into kernel version 2.6.30. 
Patch 3 and 4 should go through the powerpc maintainer(s).

Thanks.

Wolfgang.

[PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Wolfgang Grandegger]

This patch adds support for multi-chip NAND devices to the FSL-UPM
driver. This requires support for multiple GPIOs for the RNB pins.
The NAND chips are selected through address lines defined by the
FDT property "chip-offset".

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 arch/powerpc/sysdev/fsl_lbc.c |    2 +-
 drivers/mtd/nand/fsl_upm.c    |   95 +++++++++++++++++++++++++++++++---------
 2 files changed, 74 insertions(+), 23 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_lbc.c b/arch/powerpc/sysdev/fsl_lbc.c
index 0494ee5..dceb8d1 100644
--- a/arch/powerpc/sysdev/fsl_lbc.c
+++ b/arch/powerpc/sysdev/fsl_lbc.c
@@ -150,7 +150,7 @@ int fsl_upm_run_pattern(struct fsl_upm *upm, void __iomem *io_base, u32 mar)
 
 	spin_lock_irqsave(&fsl_lbc_lock, flags);
 
-	out_be32(&fsl_lbc_regs->mar, mar << (32 - upm->width));
+	out_be32(&fsl_lbc_regs->mar, mar);
 
 	switch (upm->width) {
 	case 8:
diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
index 7815a40..9b314ce 100644
--- a/drivers/mtd/nand/fsl_upm.c
+++ b/drivers/mtd/nand/fsl_upm.c
@@ -36,8 +36,11 @@ struct fsl_upm_nand {
 	uint8_t upm_addr_offset;
 	uint8_t upm_cmd_offset;
 	void __iomem *io_base;
-	int rnb_gpio;
+	int rnb_gpio[NAND_MAX_CHIPS];
 	int chip_delay;
+	uint32_t num_chips;
+	uint32_t chip_number;
+	uint32_t chip_offset;
 };
 
 #define to_fsl_upm_nand(mtd) container_of(mtd, struct fsl_upm_nand, mtd)
@@ -46,7 +49,7 @@ static int fun_chip_ready(struct mtd_info *mtd)
 {
 	struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
 
-	if (gpio_get_value(fun->rnb_gpio))
+	if (gpio_get_value(fun->rnb_gpio[fun->chip_number]))
 		return 1;
 
 	dev_vdbg(fun->dev, "busy\n");
@@ -55,9 +58,9 @@ static int fun_chip_ready(struct mtd_info *mtd)
 
 static void fun_wait_rnb(struct fsl_upm_nand *fun)
 {
-	int cnt = 1000000;
 
-	if (fun->rnb_gpio >= 0) {
+	if (fun->rnb_gpio[fun->chip_number] >= 0) {
+		int cnt = 1000000;
 		while (--cnt && !fun_chip_ready(&fun->mtd))
 			cpu_relax();
 		if (!cnt)
@@ -69,7 +72,9 @@ static void fun_wait_rnb(struct fsl_upm_nand *fun)
 
 static void fun_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 {
+	struct nand_chip *chip = mtd->priv;
 	struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
+	u32 mar;
 
 	if (!(ctrl & fun->last_ctrl)) {
 		fsl_upm_end_pattern(&fun->upm);
@@ -87,11 +92,30 @@ static void fun_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 			fsl_upm_start_pattern(&fun->upm, fun->upm_cmd_offset);
 	}
 
-	fsl_upm_run_pattern(&fun->upm, fun->io_base, cmd);
+	mar = cmd << (32 - fun->upm.width);
+	if (fun->chip_offset && fun->chip_number > 0)
+		mar |= fun->chip_number * fun->chip_offset;
+	fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
 
 	fun_wait_rnb(fun);
 }
 
+static void fun_select_chip(struct mtd_info *mtd, int chip_nr)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
+
+	if (chip_nr == -1) {
+		chip->cmd_ctrl(mtd, NAND_CMD_NONE, 0 | NAND_CTRL_CHANGE);
+	} else if (chip_nr >= 0) {
+		fun->chip_number = chip_nr;
+		chip->IO_ADDR_R = chip->IO_ADDR_W =
+			fun->io_base + chip_nr * fun->chip_offset;
+	} else {
+		BUG();
+	}
+}
+
 static uint8_t fun_read_byte(struct mtd_info *mtd)
 {
 	struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
@@ -137,8 +161,10 @@ static int __devinit fun_chip_init(struct fsl_upm_nand *fun,
 	fun->chip.read_buf = fun_read_buf;
 	fun->chip.write_buf = fun_write_buf;
 	fun->chip.ecc.mode = NAND_ECC_SOFT;
+	if (fun->num_chips > 1)
+		fun->chip.select_chip = fun_select_chip;
 
-	if (fun->rnb_gpio >= 0)
+	if (fun->rnb_gpio[0] >= 0)
 		fun->chip.dev_ready = fun_chip_ready;
 
 	fun->mtd.priv = &fun->chip;
@@ -155,7 +181,7 @@ static int __devinit fun_chip_init(struct fsl_upm_nand *fun,
 		goto err;
 	}
 
-	ret = nand_scan(&fun->mtd, 1);
+	ret = nand_scan(&fun->mtd, fun->num_chips);
 	if (ret)
 		goto err;
 
@@ -187,6 +213,7 @@ static int __devinit fun_probe(struct of_device *ofdev,
 	const uint32_t *prop;
 	int ret;
 	int size;
+	int i;
 
 	fun = kzalloc(sizeof(*fun), GFP_KERNEL);
 	if (!fun)
@@ -208,7 +235,7 @@ static int __devinit fun_probe(struct of_device *ofdev,
 	if (!prop || size != sizeof(uint32_t)) {
 		dev_err(&ofdev->dev, "can't get UPM address offset\n");
 		ret = -EINVAL;
-		goto err2;
+		goto err1;
 	}
 	fun->upm_addr_offset = *prop;
 
@@ -216,21 +243,36 @@ static int __devinit fun_probe(struct of_device *ofdev,
 	if (!prop || size != sizeof(uint32_t)) {
 		dev_err(&ofdev->dev, "can't get UPM command offset\n");
 		ret = -EINVAL;
-		goto err2;
+		goto err1;
 	}
 	fun->upm_cmd_offset = *prop;
 
-	fun->rnb_gpio = of_get_gpio(ofdev->node, 0);
-	if (fun->rnb_gpio >= 0) {
-		ret = gpio_request(fun->rnb_gpio, dev_name(&ofdev->dev));
-		if (ret) {
-			dev_err(&ofdev->dev, "can't request RNB gpio\n");
+	prop = of_get_property(ofdev->node, "num-chips", &size);
+	if (prop && size == sizeof(uint32_t)) {
+		fun->num_chips = *prop;
+		if (fun->num_chips >= NAND_MAX_CHIPS) {
+			dev_err(&ofdev->dev, "too much chips");
+			ret = -EINVAL;
+			goto err1;
+		}
+	} else {
+		fun->num_chips = 1;
+	}
+
+	for (i = 0; i < fun->num_chips; i++) {
+		fun->rnb_gpio[i] = of_get_gpio(ofdev->node, i);
+		if (fun->rnb_gpio[i] >= 0) {
+			ret = gpio_request(fun->rnb_gpio[i], 
+					   dev_name(&ofdev->dev));
+			if (ret) {
+				dev_err(&ofdev->dev, "can't request RNB gpio\n");
+				goto err2;
+			}
+			gpio_direction_input(fun->rnb_gpio[i]);
+		} else if (fun->rnb_gpio[i]  == -EINVAL) {
+			dev_err(&ofdev->dev, "specified RNB gpio is invalid\n");
 			goto err2;
 		}
-		gpio_direction_input(fun->rnb_gpio);
-	} else if (fun->rnb_gpio == -EINVAL) {
-		dev_err(&ofdev->dev, "specified RNB gpio is invalid\n");
-		goto err2;
 	}
 
 	prop = of_get_property(ofdev->node, "chip-delay", NULL);
@@ -239,6 +281,10 @@ static int __devinit fun_probe(struct of_device *ofdev,
 	else
 		fun->chip_delay = 50;
 
+	prop = of_get_property(ofdev->node, "chip-offset", &size);
+	if (prop && size == sizeof(uint32_t))
+		fun->chip_offset = *prop;
+
 	fun->io_base = devm_ioremap_nocache(&ofdev->dev, io_res.start,
 					  io_res.end - io_res.start + 1);
 	if (!fun->io_base) {
@@ -257,8 +303,10 @@ static int __devinit fun_probe(struct of_device *ofdev,
 
 	return 0;
 err2:
-	if (fun->rnb_gpio >= 0)
-		gpio_free(fun->rnb_gpio);
+	for (i = 0; i < fun->num_chips; i++) {
+		if (fun->rnb_gpio[i] >= 0)
+			gpio_free(fun->rnb_gpio[i]);
+	}
 err1:
 	kfree(fun);
 
@@ -268,12 +316,15 @@ err1:
 static int __devexit fun_remove(struct of_device *ofdev)
 {
 	struct fsl_upm_nand *fun = dev_get_drvdata(&ofdev->dev);
+	int i;
 
 	nand_release(&fun->mtd);
 	kfree(fun->mtd.name);
 
-	if (fun->rnb_gpio >= 0)
-		gpio_free(fun->rnb_gpio);
+        for (i = 0; i < fun->num_chips; i++) {
+                if (fun->rnb_gpio[i] >= 0)
+                        gpio_free(fun->rnb_gpio[i]);
+        }
 
 	kfree(fun);
 
-- 
1.6.0.6

[PATCH v3 2/4] NAND: FSL-UPM: Add wait flags to support board/chip specific delays

[Wolfgang Grandegger]

The NAND flash on the TQM8548_BE modules requires a short delay after
running the UPM pattern. The TQM8548_BE requires a further short delay
after writing out a buffer. Normally the R/B pin should be checked, but
it's not connected on the TQM8548_BE. The existing driver uses similar
fixed delay points. To manage these extra delays in a more general way,
I introduced the "wait-flags" property allowing the board-specific driver
to specify various types of extra delay.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/mtd/nand/fsl_upm.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
index 9b314ce..3ecfc1e 100644
--- a/drivers/mtd/nand/fsl_upm.c
+++ b/drivers/mtd/nand/fsl_upm.c
@@ -23,6 +23,10 @@
 #include <linux/io.h>
 #include <asm/fsl_lbc.h>
 
+#define FSL_UPM_WAIT_RUN_PATTERN  0x1
+#define FSL_UPM_WAIT_WRITE_BYTE   0x2
+#define FSL_UPM_WAIT_WRITE_BUFFER 0x4
+
 struct fsl_upm_nand {
 	struct device *dev;
 	struct mtd_info mtd;
@@ -41,6 +45,7 @@ struct fsl_upm_nand {
 	uint32_t num_chips;
 	uint32_t chip_number;
 	uint32_t chip_offset;
+	uint32_t wait_flags;
 };
 
 #define to_fsl_upm_nand(mtd) container_of(mtd, struct fsl_upm_nand, mtd)
@@ -97,7 +102,8 @@ static void fun_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 		mar |= fun->chip_number * fun->chip_offset;
 	fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
 
-	fun_wait_rnb(fun);
+	if (fun->wait_flags & FSL_UPM_WAIT_RUN_PATTERN)
+		fun_wait_rnb(fun);
 }
 
 static void fun_select_chip(struct mtd_info *mtd, int chip_nr)
@@ -139,8 +145,11 @@ static void fun_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
 
 	for (i = 0; i < len; i++) {
 		out_8(fun->chip.IO_ADDR_W, buf[i]);
-		fun_wait_rnb(fun);
+		if (fun->wait_flags & FSL_UPM_WAIT_WRITE_BYTE)
+			fun_wait_rnb(fun);
 	}
+	if (fun->wait_flags & FSL_UPM_WAIT_WRITE_BUFFER)
+		fun_wait_rnb(fun);
 }
 
 static int __devinit fun_chip_init(struct fsl_upm_nand *fun,
@@ -285,6 +294,13 @@ static int __devinit fun_probe(struct of_device *ofdev,
 	if (prop && size == sizeof(uint32_t))
 		fun->chip_offset = *prop;
 
+	prop = of_get_property(ofdev->node, "fsl,upm-wait-flags", &size);
+	if (prop && size == sizeof(uint32_t))
+		fun->wait_flags = *prop;
+	else
+		fun->wait_flags =
+			FSL_UPM_WAIT_RUN_PATTERN | FSL_UPM_WAIT_WRITE_BYTE;
+
 	fun->io_base = devm_ioremap_nocache(&ofdev->dev, io_res.start,
 					  io_res.end - io_res.start + 1);
 	if (!fun->io_base) {
-- 
1.6.0.6

[PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

This patch adds documentation for the new NAND FSL UPM bindings for:

 NAND: FSL-UPM: add multi chip support
 NAND: FSL-UPM: Add wait flags to support board/chip specific delays

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 .../powerpc/dts-bindings/fsl/upm-nand.txt          |   39 +++++++++++++++++++-
 1 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
index 84a04d5..0272e70 100644
--- a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
+++ b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
@@ -5,9 +5,22 @@ Required properties:
 - reg : should specify localbus chip select and size used for the chip.
 - fsl,upm-addr-offset : UPM pattern offset for the address latch.
 - fsl,upm-cmd-offset : UPM pattern offset for the command latch.
-- gpios : may specify optional GPIO connected to the Ready-Not-Busy pin.
 
-Example:
+Optional properties:
+- fsl,upm-wait-flags : add chip-dependent short delays after running the
+  		       UPM pattern (0x1), after writing a data byte (0x2)
+		       or after writing out a buffer (0x4).
+- gpios : may specify optional GPIOs connected to the Ready-Not-Busy pins
+	  (R/B#). For multi-chip devices, "num-chips" GPIO definitions are
+	  required.
+- chip-delay : chip dependent delay for transfering data from array to
+	       read registers (tR). Required if property "gpios" is not
+	       used (R/B# pins not connected).
+- num-chips : number of chips per device for multi-chip support.
+- chip-offset : address offset between chips for multi-chip support. The
+ 		corresponding address lines are used to select the chip.
+
+Examples:
 
 upm@1,0 {
 	compatible = "fsl,upm-nand";
@@ -26,3 +39,25 @@ upm@1,0 {
 		};
 	};
 };
+
+upm@3,0 {
+	compatible = "fsl,upm-nand";
+	reg = <3 0x0 0x800>;
+	fsl,upm-addr-offset = <0x10>;
+	fsl,upm-cmd-offset = <0x08>;
+	fsl,upm-wait-flags = <0x5>;
+	/* Multi-chip device */
+	num-chips = <2>;
+	chip-offset = <0x200>;
+	chip-delay = <25>; // in micro-seconds
+
+	nand@0 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		partition@0 {
+			    label = "fs";
+			    reg = <0x00000000 0x10000000>;
+		};
+	};
+};
-- 
1.6.0.6

[PATCH v3 4/4] powerpc/85xx: TQM8548: Update DTS file for multi-chip support

[Wolfgang Grandegger]

This patch adds multi-chip support for the Micron MT29F8G08FAB NAND
flash memory on the TQM8548 modules.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 arch/powerpc/boot/dts/tqm8548-bigflash.dts |    6 +++++-
 arch/powerpc/boot/dts/tqm8548.dts          |    6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/boot/dts/tqm8548-bigflash.dts b/arch/powerpc/boot/dts/tqm8548-bigflash.dts
index 29a2b6f..bf0b4e9 100644
--- a/arch/powerpc/boot/dts/tqm8548-bigflash.dts
+++ b/arch/powerpc/boot/dts/tqm8548-bigflash.dts
@@ -389,6 +389,10 @@
 			reg = <3 0x0 0x800>;
 			fsl,upm-addr-offset = <0x10>;
 			fsl,upm-cmd-offset = <0x08>;
+			fsl,upm-wait-flags = <0x5>;
+			/* Multi-chip device */
+			num-chips = <2>;
+			chip-offset = <0x200>;
 			chip-delay = <25>; // in micro-seconds
 
 			nand@0 {
@@ -397,7 +401,7 @@
 
 				partition@0 {
 					    label = "fs";
-					    reg = <0x00000000 0x01000000>;
+					    reg = <0x00000000 0x10000000>;
 				};
 			};
 		};
diff --git a/arch/powerpc/boot/dts/tqm8548.dts b/arch/powerpc/boot/dts/tqm8548.dts
index 81d3fbb..93a53ae 100644
--- a/arch/powerpc/boot/dts/tqm8548.dts
+++ b/arch/powerpc/boot/dts/tqm8548.dts
@@ -389,6 +389,10 @@
 			reg = <3 0x0 0x800>;
 			fsl,upm-addr-offset = <0x10>;
 			fsl,upm-cmd-offset = <0x08>;
+			fsl,upm-wait-flags = <0x5>;
+			/* Multi-chip device */
+			num-chips = <2>;
+			chip-offset = <0x200>;
 			chip-delay = <25>; // in micro-seconds
 
 			nand@0 {
@@ -397,7 +401,7 @@
 
 				partition@0 {
 					    label = "fs";
-					    reg = <0x00000000 0x01000000>;
+					    reg = <0x00000000 0x10000000>;
 				};
 			};
 		};
-- 
1.6.0.6

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Anton Vorontsov]

On Wed, Mar 25, 2009 at 11:08:20AM +0100, Wolfgang Grandegger wrote:
> This patch adds documentation for the new NAND FSL UPM bindings for:
> 
>  NAND: FSL-UPM: add multi chip support
>  NAND: FSL-UPM: Add wait flags to support board/chip specific delays
> 
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---

To me it looks good.

Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com>

>  .../powerpc/dts-bindings/fsl/upm-nand.txt          |   39 +++++++++++++++++++-
>  1 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> index 84a04d5..0272e70 100644
> --- a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> @@ -5,9 +5,22 @@ Required properties:
>  - reg : should specify localbus chip select and size used for the chip.
>  - fsl,upm-addr-offset : UPM pattern offset for the address latch.
>  - fsl,upm-cmd-offset : UPM pattern offset for the command latch.
> -- gpios : may specify optional GPIO connected to the Ready-Not-Busy pin.
>  
> -Example:
> +Optional properties:
> +- fsl,upm-wait-flags : add chip-dependent short delays after running the
> +  		       UPM pattern (0x1), after writing a data byte (0x2)
> +		       or after writing out a buffer (0x4).
> +- gpios : may specify optional GPIOs connected to the Ready-Not-Busy pins
> +	  (R/B#). For multi-chip devices, "num-chips" GPIO definitions are
> +	  required.
> +- chip-delay : chip dependent delay for transfering data from array to
> +	       read registers (tR). Required if property "gpios" is not
> +	       used (R/B# pins not connected).
> +- num-chips : number of chips per device for multi-chip support.
> +- chip-offset : address offset between chips for multi-chip support. The
> + 		corresponding address lines are used to select the chip.
> +
> +Examples:
>  
>  upm@1,0 {
>  	compatible = "fsl,upm-nand";
> @@ -26,3 +39,25 @@ upm@1,0 {
>  		};
>  	};
>  };
> +
> +upm@3,0 {
> +	compatible = "fsl,upm-nand";
> +	reg = <3 0x0 0x800>;
> +	fsl,upm-addr-offset = <0x10>;
> +	fsl,upm-cmd-offset = <0x08>;
> +	fsl,upm-wait-flags = <0x5>;
> +	/* Multi-chip device */
> +	num-chips = <2>;
> +	chip-offset = <0x200>;
> +	chip-delay = <25>; // in micro-seconds
> +
> +	nand@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		partition@0 {
> +			    label = "fs";
> +			    reg = <0x00000000 0x10000000>;
> +		};
> +	};
> +};
> -- 
> 1.6.0.6

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

(cc'ing devicetree-discuss)

On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> This patch adds documentation for the new NAND FSL UPM bindings for:
>
> =A0NAND: FSL-UPM: add multi chip support
> =A0NAND: FSL-UPM: Add wait flags to support board/chip specific delays
>
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>

Mostly looks good to me; but some comments below.

> ---
> =A0.../powerpc/dts-bindings/fsl/upm-nand.txt =A0 =A0 =A0 =A0 =A0| =A0 39 =
+++++++++++++++++++-
> =A01 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt b/Docume=
ntation/powerpc/dts-bindings/fsl/upm-nand.txt
> index 84a04d5..0272e70 100644
> --- a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> @@ -5,9 +5,22 @@ Required properties:
> =A0- reg : should specify localbus chip select and size used for the chip=
.
> =A0- fsl,upm-addr-offset : UPM pattern offset for the address latch.
> =A0- fsl,upm-cmd-offset : UPM pattern offset for the command latch.
> -- gpios : may specify optional GPIO connected to the Ready-Not-Busy pin.
>
> -Example:
> +Optional properties:
> +- fsl,upm-wait-flags : add chip-dependent short delays after running the
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0UPM pattern (0x1), after wri=
ting a data byte (0x2)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0or after writing out a buffe=
r (0x4).
> +- gpios : may specify optional GPIOs connected to the Ready-Not-Busy pin=
s
> + =A0 =A0 =A0 =A0 (R/B#). For multi-chip devices, "num-chips" GPIO defini=
tions are
> + =A0 =A0 =A0 =A0 required.
> +- chip-delay : chip dependent delay for transfering data from array to
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0read registers (tR). Required if property "g=
pios" is not
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0used (R/B# pins not connected).
> +- num-chips : number of chips per device for multi-chip support.
> +- chip-offset : address offset between chips for multi-chip support. The
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 corresponding address lines are used to sel=
ect the chip.

Since these properties (chip-delay, num-chips and chip-offset) are
currently controller specific, it would probably be a good idea to
prefix 'fsl,' onto them.  That way when common NAND controller
properties start getting defined then there won't be any concern about
conflicting with existing meanings.  If you do see these as properties
that other NAND controllers will use, then maybe a 'nand-' prefix is
appropriate (like the SPI binding in booting-without-of).

For the chip offset, it's not clear what the meaning is.  First, does
the UPM controller support access of multiple chips simultaneously?
If so, then can you elaborate in the description on how board design
translates to a chip-offset value.  If it cannot, then it might be
better to have multiple tuples in the 'reg' property for each discrete
chip.  Multiple reg tuples would also remove the need for the
num-chips property.

Cheers,
g.

> +
> +Examples:
>
> =A0upm@1,0 {
> =A0 =A0 =A0 =A0compatible =3D "fsl,upm-nand";
> @@ -26,3 +39,25 @@ upm@1,0 {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0};
> =A0 =A0 =A0 =A0};
> =A0};
> +
> +upm@3,0 {
> + =A0 =A0 =A0 compatible =3D "fsl,upm-nand";
> + =A0 =A0 =A0 reg =3D <3 0x0 0x800>;
> + =A0 =A0 =A0 fsl,upm-addr-offset =3D <0x10>;
> + =A0 =A0 =A0 fsl,upm-cmd-offset =3D <0x08>;
> + =A0 =A0 =A0 fsl,upm-wait-flags =3D <0x5>;
> + =A0 =A0 =A0 /* Multi-chip device */
> + =A0 =A0 =A0 num-chips =3D <2>;
> + =A0 =A0 =A0 chip-offset =3D <0x200>;
> + =A0 =A0 =A0 chip-delay =3D <25>; // in micro-seconds
> +
> + =A0 =A0 =A0 nand@0 {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 #address-cells =3D <1>;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 #size-cells =3D <1>;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 partition@0 {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 label =3D "fs";
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 reg =3D <0x00000000=
 0x10000000>;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 };
> + =A0 =A0 =A0 };
> +};
> --
> 1.6.0.6
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Grant Likely wrote:
> (cc'ing devicetree-discuss)
> 
> On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> This patch adds documentation for the new NAND FSL UPM bindings for:
>>
>>  NAND: FSL-UPM: add multi chip support
>>  NAND: FSL-UPM: Add wait flags to support board/chip specific delays
>>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> 
> Mostly looks good to me; but some comments below.
> 
>> ---
>>  .../powerpc/dts-bindings/fsl/upm-nand.txt          |   39 +++++++++++++++++++-
>>  1 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
>> index 84a04d5..0272e70 100644
>> --- a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
>> +++ b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
>> @@ -5,9 +5,22 @@ Required properties:
>>  - reg : should specify localbus chip select and size used for the chip.
>>  - fsl,upm-addr-offset : UPM pattern offset for the address latch.
>>  - fsl,upm-cmd-offset : UPM pattern offset for the command latch.
>> -- gpios : may specify optional GPIO connected to the Ready-Not-Busy pin.
>>
>> -Example:
>> +Optional properties:
>> +- fsl,upm-wait-flags : add chip-dependent short delays after running the
>> +                      UPM pattern (0x1), after writing a data byte (0x2)
>> +                      or after writing out a buffer (0x4).
>> +- gpios : may specify optional GPIOs connected to the Ready-Not-Busy pins
>> +         (R/B#). For multi-chip devices, "num-chips" GPIO definitions are
>> +         required.
>> +- chip-delay : chip dependent delay for transfering data from array to
>> +              read registers (tR). Required if property "gpios" is not
>> +              used (R/B# pins not connected).
>> +- num-chips : number of chips per device for multi-chip support.
>> +- chip-offset : address offset between chips for multi-chip support. The
>> +               corresponding address lines are used to select the chip.
> 
> Since these properties (chip-delay, num-chips and chip-offset) are
> currently controller specific, it would probably be a good idea to
> prefix 'fsl,' onto them.  That way when common NAND controller
> properties start getting defined then there won't be any concern about
> conflicting with existing meanings.  If you do see these as properties
> that other NAND controllers will use, then maybe a 'nand-' prefix is
> appropriate (like the SPI binding in booting-without-of).

The chip-delay is NAND device specific. The proper value can be find in
the data sheet. num-chip is also quite generic. A prefix 'nand-' would
be fine for me. But fsl,upm-chip-offset would be more appropriate than
just chip-offset, indeed.

> For the chip offset, it's not clear what the meaning is.  First, does
> the UPM controller support access of multiple chips simultaneously?

The offset drives the corresponding address lines, which are used to
select the chip. That's how it's done on the TQM8548 board. In
principle, the chips could also be selected through dedicated GPIO pins.
Well, I'm not a hardware guy.

> If so, then can you elaborate in the description on how board design
> translates to a chip-offset value.  If it cannot, then it might be
> better to have multiple tuples in the 'reg' property for each discrete
> chip.  Multiple reg tuples would also remove the need for the
> num-chips property.

The node still describes one device mapping all relevant control
registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
would be more generic and makes num-chips obsolete as well. And the
property would be reserved for that way of implementing the chip select
in hardware.

Wolfgang.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> Grant Likely wrote:
>> For the chip offset, it's not clear what the meaning is. =A0First, does
>> the UPM controller support access of multiple chips simultaneously?
>
> The offset drives the corresponding address lines, which are used to
> select the chip. That's how it's done on the TQM8548 board. In
> principle, the chips could also be selected through dedicated GPIO pins.
> Well, I'm not a hardware guy.

Heh.  I mean elaborate in the binding documentation.  :-)

>> If so, then can you elaborate in the description on how board design
>> translates to a chip-offset value. =A0If it cannot, then it might be
>> better to have multiple tuples in the 'reg' property for each discrete
>> chip. =A0Multiple reg tuples would also remove the need for the
>> num-chips property.
>
> The node still describes one device mapping all relevant control
> registers. How about using fsl,upm-chip-offsets =3D <0x200 0x400>. It
> would be more generic and makes num-chips obsolete as well. And the
> property would be reserved for that way of implementing the chip select
> in hardware.

It really sounds like this binding is describing multiple NAND chips
mapped to different base addresses (and looking at the fsm_upm.c
driver appears to confirm it).  So, does this work?  reg =3D <3 0x200 4
 3 0x400 4>;

It is true that other methods could be used for implementing the chip
select, but that is *not* what the proposed binding describes.  This
proposed binding describes NAND chips selected by address lines
(particular addresses), and in this case I think using reg is the
natural description.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Grant Likely wrote:
> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>> For the chip offset, it's not clear what the meaning is.  First, does
>>> the UPM controller support access of multiple chips simultaneously?
>> The offset drives the corresponding address lines, which are used to
>> select the chip. That's how it's done on the TQM8548 board. In
>> principle, the chips could also be selected through dedicated GPIO pins.
>> Well, I'm not a hardware guy.
> 
> Heh.  I mean elaborate in the binding documentation.  :-)
> 
>>> If so, then can you elaborate in the description on how board design
>>> translates to a chip-offset value.  If it cannot, then it might be
>>> better to have multiple tuples in the 'reg' property for each discrete
>>> chip.  Multiple reg tuples would also remove the need for the
>>> num-chips property.
>> The node still describes one device mapping all relevant control
>> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
>> would be more generic and makes num-chips obsolete as well. And the
>> property would be reserved for that way of implementing the chip select
>> in hardware.
> 
> It really sounds like this binding is describing multiple NAND chips
> mapped to different base addresses (and looking at the fsm_upm.c
> driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
>  3 0x400 4>;

The chip-offset, and not the address, needs to be added to the MAR
register as well before running the pattern:

        mar = cmd << (32 - fun->upm.width);

        if (fun->chip_offset && fun->chip_number > 0)

                mar |= fun->chip_number * fun->chip_offset;

        fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);


> It is true that other methods could be used for implementing the chip
> select, but that is *not* what the proposed binding describes.  This
> proposed binding describes NAND chips selected by address lines
> (particular addresses), and in this case I think using reg is the
> natural description.

OK, the chips are selected by accessing a defined address range. Will
prepare a patch using the reg property.

Wolfgang.




> g.
> 

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

On Thu, Mar 26, 2009 at 1:42 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> Grant Likely wrote:
>> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com>=
 wrote:
>>> Grant Likely wrote:
>>>> For the chip offset, it's not clear what the meaning is. =A0First, doe=
s
>>>> the UPM controller support access of multiple chips simultaneously?
>>> The offset drives the corresponding address lines, which are used to
>>> select the chip. That's how it's done on the TQM8548 board. In
>>> principle, the chips could also be selected through dedicated GPIO pins=
.
>>> Well, I'm not a hardware guy.
>>
>> Heh. =A0I mean elaborate in the binding documentation. =A0:-)
>>
>>>> If so, then can you elaborate in the description on how board design
>>>> translates to a chip-offset value. =A0If it cannot, then it might be
>>>> better to have multiple tuples in the 'reg' property for each discrete
>>>> chip. =A0Multiple reg tuples would also remove the need for the
>>>> num-chips property.
>>> The node still describes one device mapping all relevant control
>>> registers. How about using fsl,upm-chip-offsets =3D <0x200 0x400>. It
>>> would be more generic and makes num-chips obsolete as well. And the
>>> property would be reserved for that way of implementing the chip select
>>> in hardware.
>>
>> It really sounds like this binding is describing multiple NAND chips
>> mapped to different base addresses (and looking at the fsm_upm.c
>> driver appears to confirm it). =A0So, does this work? =A0reg =3D <3 0x20=
0 4
>> =A03 0x400 4>;
>
> The chip-offset, and not the address, needs to be added to the MAR
> register as well before running the pattern:
>
> =A0 =A0 =A0 =A0mar =3D cmd << (32 - fun->upm.width);
>
> =A0 =A0 =A0 =A0if (fun->chip_offset && fun->chip_number > 0)
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0mar |=3D fun->chip_number * fun->chip_offs=
et;
>
> =A0 =A0 =A0 =A0fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>
>
>> It is true that other methods could be used for implementing the chip
>> select, but that is *not* what the proposed binding describes. =A0This
>> proposed binding describes NAND chips selected by address lines
>> (particular addresses), and in this case I think using reg is the
>> natural description.
>
> OK, the chips are selected by accessing a defined address range. Will
> prepare a patch using the reg property.

Hold on a sec.  I'm debating from my experience with device tree
bindings, but I'm fairly ignorant about the implementation of NAND on
UPM.  It *looks* to me like reg is sufficient, but if I'm wrong then
tell me so and why.  Your comment above about fsl_upm_run_pattern()
makes me doubt my position.

Does using the reg property give the driver enough information to
reliably program the MAR for NAND connections that use the address
line chip select scheme?  Related to that, should the binding include
a property that explicitly states that an address line chip select
scheme is being used?

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Grant Likely wrote:
> On Thu, Mar 26, 2009 at 1:42 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>>> Grant Likely wrote:
>>>>> For the chip offset, it's not clear what the meaning is.  First, does
>>>>> the UPM controller support access of multiple chips simultaneously?
>>>> The offset drives the corresponding address lines, which are used to
>>>> select the chip. That's how it's done on the TQM8548 board. In
>>>> principle, the chips could also be selected through dedicated GPIO pins.
>>>> Well, I'm not a hardware guy.
>>> Heh.  I mean elaborate in the binding documentation.  :-)
>>>
>>>>> If so, then can you elaborate in the description on how board design
>>>>> translates to a chip-offset value.  If it cannot, then it might be
>>>>> better to have multiple tuples in the 'reg' property for each discrete
>>>>> chip.  Multiple reg tuples would also remove the need for the
>>>>> num-chips property.
>>>> The node still describes one device mapping all relevant control
>>>> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
>>>> would be more generic and makes num-chips obsolete as well. And the
>>>> property would be reserved for that way of implementing the chip select
>>>> in hardware.
>>> It really sounds like this binding is describing multiple NAND chips
>>> mapped to different base addresses (and looking at the fsm_upm.c
>>> driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
>>>  3 0x400 4>;
>> The chip-offset, and not the address, needs to be added to the MAR
>> register as well before running the pattern:
>>
>>        mar = cmd << (32 - fun->upm.width);
>>
>>        if (fun->chip_offset && fun->chip_number > 0)
>>
>>                mar |= fun->chip_number * fun->chip_offset;
>>
>>        fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>>
>>
>>> It is true that other methods could be used for implementing the chip
>>> select, but that is *not* what the proposed binding describes.  This
>>> proposed binding describes NAND chips selected by address lines
>>> (particular addresses), and in this case I think using reg is the
>>> natural description.
>> OK, the chips are selected by accessing a defined address range. Will
>> prepare a patch using the reg property.
> 
> Hold on a sec.  I'm debating from my experience with device tree

I already started ;-).

> bindings, but I'm fairly ignorant about the implementation of NAND on
> UPM.  It *looks* to me like reg is sufficient, but if I'm wrong then
> tell me so and why.  Your comment above about fsl_upm_run_pattern()
> makes me doubt my position.

It's not sufficient to just map the related space and access it, at least.

> Does using the reg property give the driver enough information to
> reliably program the MAR for NAND connections that use the address
> line chip select scheme?  Related to that, should the binding include

In principle yes:

  if (i > 0)
      offset[i] = resource[i].start - resource[0].start;

> a property that explicitly states that an address line chip select
> scheme is being used?

That's why I'm still in favor of:

  fsl,upm-multi-chip-offsets = <0x200 0x400>

That would state that the address line chip select scheme is used with
the specified offsets. It also allows for a more elegant solution
(code-wise).

Wolfgang.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

On Thu, Mar 26, 2009 at 9:33 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> Grant Likely wrote:
>> Does using the reg property give the driver enough information to
>> reliably program the MAR for NAND connections that use the address
>> line chip select scheme? =A0Related to that, should the binding include
>
> In principle yes:
>
> =A0if (i > 0)
> =A0 =A0 =A0offset[i] =3D resource[i].start - resource[0].start;

Ewww.  That's ugly.

>> a property that explicitly states that an address line chip select
>> scheme is being used?
>
> That's why I'm still in favor of:
>
> =A0fsl,upm-multi-chip-offsets =3D <0x200 0x400>
>
> That would state that the address line chip select scheme is used with
> the specified offsets. It also allows for a more elegant solution
> (code-wise).

Alright.  Then at the very least the property name should reflect that
address lines CS is used to reduce the chance of confusion with
another multi-chip scheme.  Something like
fsl,upm-addr-line-cs-offsets maybe?

Here is another thought.  The binding is describing that address lines
are used to activate CS lines.  Offset for chip access purposes is
derived from the address line, but it doesn't directly describe the
hardware.  The following may be a better description of the hardware.

fsl,upm-addr-line-cs =3D <9 10>;

g.



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Grant Likely wrote:
> On Thu, Mar 26, 2009 at 9:33 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>> Does using the reg property give the driver enough information to
>>> reliably program the MAR for NAND connections that use the address
>>> line chip select scheme?  Related to that, should the binding include
>> In principle yes:
>>
>>  if (i > 0)
>>      offset[i] = resource[i].start - resource[0].start;
> 
> Ewww.  That's ugly.

Yep.

>>> a property that explicitly states that an address line chip select
>>> scheme is being used?
>> That's why I'm still in favor of:
>>
>>  fsl,upm-multi-chip-offsets = <0x200 0x400>
>>
>> That would state that the address line chip select scheme is used with
>> the specified offsets. It also allows for a more elegant solution
>> (code-wise).
> 
> Alright.  Then at the very least the property name should reflect that
> address lines CS is used to reduce the chance of confusion with
> another multi-chip scheme.  Something like
> fsl,upm-addr-line-cs-offsets maybe?


> 
> Here is another thought.  The binding is describing that address lines
> are used to activate CS lines.  Offset for chip access purposes is
> derived from the address line, but it doesn't directly describe the
> hardware.  The following may be a better description of the hardware.
> 
> fsl,upm-addr-line-cs = <9 10>;

The TQM8548 hardware has some logic connected to the two address lines
allowing to select up to 4 chips with two address lines:

 fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>

That's the more general solution.

Wolfgang.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

On Thu, Mar 26, 2009 at 10:35 AM, Wolfgang Grandegger <wg@grandegger.com> w=
rote:
> Grant Likely wrote:
>> On Thu, Mar 26, 2009 at 9:33 AM, Wolfgang Grandegger <wg@grandegger.com>=
 wrote:
>>> Grant Likely wrote:
>>>> Does using the reg property give the driver enough information to
>>>> reliably program the MAR for NAND connections that use the address
>>>> line chip select scheme? =A0Related to that, should the binding includ=
e
>>> In principle yes:
>>>
>>> =A0if (i > 0)
>>> =A0 =A0 =A0offset[i] =3D resource[i].start - resource[0].start;
>>
>> Ewww. =A0That's ugly.
>
> Yep.
>
>>>> a property that explicitly states that an address line chip select
>>>> scheme is being used?
>>> That's why I'm still in favor of:
>>>
>>> =A0fsl,upm-multi-chip-offsets =3D <0x200 0x400>
>>>
>>> That would state that the address line chip select scheme is used with
>>> the specified offsets. It also allows for a more elegant solution
>>> (code-wise).
>>
>> Alright. =A0Then at the very least the property name should reflect that
>> address lines CS is used to reduce the chance of confusion with
>> another multi-chip scheme. =A0Something like
>> fsl,upm-addr-line-cs-offsets maybe?
>
>
>>
>> Here is another thought. =A0The binding is describing that address lines
>> are used to activate CS lines. =A0Offset for chip access purposes is
>> derived from the address line, but it doesn't directly describe the
>> hardware. =A0The following may be a better description of the hardware.
>>
>> fsl,upm-addr-line-cs =3D <9 10>;
>
> The TQM8548 hardware has some logic connected to the two address lines
> allowing to select up to 4 chips with two address lines:
>
> =A0fsl,upm-addr-line-cs-offsets =3D <0x0 0x200 0x400 0x600>

Ah.  I see.  This is board specific then.  I think it is premature to
try and define a generic solution here because it depends on custom
board hardware and different boards could use very different logic.
The next board could end up doing something completely different.  I'd
rather start to see trends in multiple boards implementing the same
scheme before trying to craft a generic scheme.

In other words, this device is not register-level compatible with the
fsl,upm-nand device.  Give the node a new compatible value
(tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
for the new device.  Use the .data element in the match table to
supply an alternate fun_cmd_ctrl() function for this board (instead of
using a property value do decide which fun_cmd_ctrl() behaviour to
use).  New boards that *do* use the same addressing scheme can claim
compatibility with tqc,tqm8548-upm-nand.

You can still use the property names already discussed, but only
document them as valid for the tqc,tqm8548-upm-nand variant of the
device.

Very little will need to change in your patch to handle it this way.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Anton Vorontsov]

On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
[]
> >> Here is another thought.  The binding is describing that address lines
> >> are used to activate CS lines.  Offset for chip access purposes is
> >> derived from the address line, but it doesn't directly describe the
> >> hardware.  The following may be a better description of the hardware.
> >>
> >> fsl,upm-addr-line-cs = <9 10>;
> >
> > The TQM8548 hardware has some logic connected to the two address lines
> > allowing to select up to 4 chips with two address lines:
> >
> >  fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>
> 
> Ah.  I see.  This is board specific then.  I think it is premature to
> try and define a generic solution here because it depends on custom
> board hardware and different boards could use very different logic.
> The next board could end up doing something completely different.  I'd
> rather start to see trends in multiple boards implementing the same
> scheme before trying to craft a generic scheme.
> 
> In other words, this device is not register-level compatible with the
> fsl,upm-nand device.  Give the node a new compatible value
> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
> for the new device.  Use the .data element in the match table to
> supply an alternate fun_cmd_ctrl() function for this board (instead of
> using a property value do decide which fun_cmd_ctrl() behaviour to
> use).  New boards that *do* use the same addressing scheme can claim
> compatibility with tqc,tqm8548-upm-nand.

I don't like this. :-/

UPM is an universal thing, so there are thousands of ways we can
connect NAND to the UPM. Of which only ~10 would be sane (others are
insane, and nobody would do this. If they do, _then_ we'll fall back
to <board>-upm-nand scheme for a particular board).

I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
describe any scheme in "addr lines are cs" connection, it's a common
setup for multi-chip memory, we shouldn't treat it is as something
extraordinary.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Anton Vorontsov wrote:
> On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
> []
>>>> Here is another thought.  The binding is describing that address lines
>>>> are used to activate CS lines.  Offset for chip access purposes is
>>>> derived from the address line, but it doesn't directly describe the
>>>> hardware.  The following may be a better description of the hardware.
>>>>
>>>> fsl,upm-addr-line-cs = <9 10>;
>>> The TQM8548 hardware has some logic connected to the two address lines
>>> allowing to select up to 4 chips with two address lines:
>>>
>>>  fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>
>> Ah.  I see.  This is board specific then.  I think it is premature to
>> try and define a generic solution here because it depends on custom
>> board hardware and different boards could use very different logic.
>> The next board could end up doing something completely different.  I'd
>> rather start to see trends in multiple boards implementing the same
>> scheme before trying to craft a generic scheme.
>>
>> In other words, this device is not register-level compatible with the
>> fsl,upm-nand device.  Give the node a new compatible value
>> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
>> for the new device.  Use the .data element in the match table to
>> supply an alternate fun_cmd_ctrl() function for this board (instead of
>> using a property value do decide which fun_cmd_ctrl() behaviour to
>> use).  New boards that *do* use the same addressing scheme can claim
>> compatibility with tqc,tqm8548-upm-nand.
> 
> I don't like this. :-/
> 
> UPM is an universal thing, so there are thousands of ways we can
> connect NAND to the UPM. Of which only ~10 would be sane (others are
> insane, and nobody would do this. If they do, _then_ we'll fall back
> to <board>-upm-nand scheme for a particular board).

Yep.

> I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
> describe any scheme in "addr lines are cs" connection, it's a common
> setup for multi-chip memory, we shouldn't treat it is as something
> extraordinary.

I fully agree. I'm going to provide a patch on monday.

Wolfgang.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Grant Likely]

On Thu, Mar 26, 2009 at 4:14 PM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> Anton Vorontsov wrote:
>> On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
>>> In other words, this device is not register-level compatible with the
>>> fsl,upm-nand device. =A0Give the node a new compatible value
>>> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
>>> for the new device. =A0Use the .data element in the match table to
>>> supply an alternate fun_cmd_ctrl() function for this board (instead of
>>> using a property value do decide which fun_cmd_ctrl() behaviour to
>>> use). =A0New boards that *do* use the same addressing scheme can claim
>>> compatibility with tqc,tqm8548-upm-nand.
>>
>> I don't like this. :-/
>>
>> UPM is an universal thing, so there are thousands of ways we can
>> connect NAND to the UPM. Of which only ~10 would be sane (others are
>> insane, and nobody would do this. If they do, _then_ we'll fall back
>> to <board>-upm-nand scheme for a particular board).
>
> Yep.
>
>> I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
>> describe any scheme in "addr lines are cs" connection, it's a common
>> setup for multi-chip memory, we shouldn't treat it is as something
>> extraordinary.
>
> I fully agree. I'm going to provide a patch on monday.

Well, I still don't think it is the wisest choice.  My position is
that it is better to be conservative and pedantic now because it is
easy to relax the rules from that point.  If it turns out after some
experience with "fsl,upm-addr-line-cs-offset" that the scheme has a
serious flaw, then the impact is contained.  On the other side, if it
is confirmed and useful and correct, it is a trivial change to make it
available to everything that claims compatibility with fsl,upm-nand.

That said, I won't oppose it if you go this route.  However at the
very least, please change the nand node's compatible list to be:

compatible =3D "tqc,tqm8548-upm-nand", "fsl,upm-nand";

The custom glue logic makes it something unique, so "tqc,..." should
be at the start of the list to describe it as such, even if the driver
only ever uses "fsl,upm-nand".

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Anton Vorontsov]

On Thu, Mar 26, 2009 at 05:22:36PM -0600, Grant Likely wrote:
[...]
> That said, I won't oppose it if you go this route.  However at the
> very least, please change the nand node's compatible list to be:
> 
> compatible = "tqc,tqm8548-upm-nand", "fsl,upm-nand";

Yeah, that's definitely a good idea.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH v3 3/4] powerpc: NAND: FSL UPM: document new bindings

[Wolfgang Grandegger]

Grant Likely wrote:
> On Thu, Mar 26, 2009 at 4:14 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Anton Vorontsov wrote:
>>> On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
>>>> In other words, this device is not register-level compatible with the
>>>> fsl,upm-nand device.  Give the node a new compatible value
>>>> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
>>>> for the new device.  Use the .data element in the match table to
>>>> supply an alternate fun_cmd_ctrl() function for this board (instead of
>>>> using a property value do decide which fun_cmd_ctrl() behaviour to
>>>> use).  New boards that *do* use the same addressing scheme can claim
>>>> compatibility with tqc,tqm8548-upm-nand.
>>> I don't like this. :-/
>>>
>>> UPM is an universal thing, so there are thousands of ways we can
>>> connect NAND to the UPM. Of which only ~10 would be sane (others are
>>> insane, and nobody would do this. If they do, _then_ we'll fall back
>>> to <board>-upm-nand scheme for a particular board).
>> Yep.
>>
>>> I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
>>> describe any scheme in "addr lines are cs" connection, it's a common
>>> setup for multi-chip memory, we shouldn't treat it is as something
>>> extraordinary.
>> I fully agree. I'm going to provide a patch on monday.
> 
> Well, I still don't think it is the wisest choice.  My position is
> that it is better to be conservative and pedantic now because it is
> easy to relax the rules from that point.  If it turns out after some
> experience with "fsl,upm-addr-line-cs-offset" that the scheme has a
> serious flaw, then the impact is contained.  On the other side, if it
> is confirmed and useful and correct, it is a trivial change to make it
> available to everything that claims compatibility with fsl,upm-nand.
> 
> That said, I won't oppose it if you go this route.  However at the
> very least, please change the nand node's compatible list to be:
> 
> compatible = "tqc,tqm8548-upm-nand", "fsl,upm-nand";
> 
> The custom glue logic makes it something unique, so "tqc,..." should
> be at the start of the list to describe it as such, even if the driver
> only ever uses "fsl,upm-nand".

That's a good idea in case we need it lateron. For the time being, I
prefer to make the driver as generic as possible. Currently there are
only two boards using the driver, the MPC8360RTDK and the TQM8548. and
it's unlikely that there will be much more in the future.

Wolfgang.

Re: [PATCH v3 2/4] NAND: FSL-UPM: Add wait flags to support board/chip specific delays

[Anton Vorontsov]

On Wed, Mar 25, 2009 at 11:08:19AM +0100, Wolfgang Grandegger wrote:
> The NAND flash on the TQM8548_BE modules requires a short delay after
> running the UPM pattern. The TQM8548_BE requires a further short delay
> after writing out a buffer. Normally the R/B pin should be checked, but
> it's not connected on the TQM8548_BE. The existing driver uses similar
> fixed delay points. To manage these extra delays in a more general way,
> I introduced the "wait-flags" property allowing the board-specific driver
> to specify various types of extra delay.
> 
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>

Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com>

[...]
> +	prop = of_get_property(ofdev->node, "fsl,upm-wait-flags", &size);
> +	if (prop && size == sizeof(uint32_t))
> +		fun->wait_flags = *prop;
> +	else
> +		fun->wait_flags =
> +			FSL_UPM_WAIT_RUN_PATTERN | FSL_UPM_WAIT_WRITE_BYTE;

I'd write it as
		fun->wait_flags = FSL_UPM_WAIT_RUN_PATTERN |
				  FSL_UPM_WAIT_WRITE_BYTE;

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

RE: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Singh, Vimal]

> +static void fun_select_chip(struct mtd_info *mtd, int chip_nr)
> +{
> +       struct nand_chip *chip =3D mtd->priv;
> +       struct fsl_upm_nand *fun =3D to_fsl_upm_nand(mtd);
> +
> +       if (chip_nr =3D=3D -1) {
> +               chip->cmd_ctrl(mtd, NAND_CMD_NONE, 0 |=20
> NAND_CTRL_CHANGE);
> +       } else if (chip_nr >=3D 0) {
> +               fun->chip_number =3D chip_nr;
> +               chip->IO_ADDR_R =3D chip->IO_ADDR_W =3D
> +                       fun->io_base + chip_nr * fun->chip_offset;
> +       } else {
> +               BUG();
> +       }
braces are not required here...


> +       prop =3D of_get_property(ofdev->node, "num-chips", &size);
> +       if (prop && size =3D=3D sizeof(uint32_t)) {
> +               fun->num_chips =3D *prop;
> +               if (fun->num_chips >=3D NAND_MAX_CHIPS) {
> +                       dev_err(&ofdev->dev, "too much chips");
> +                       ret =3D -EINVAL;
> +                       goto err1;
> +               }
> +       } else {
> +               fun->num_chips =3D 1;
> +       }
ditto...=

Re: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Wolfgang Grandegger]

Singh, Vimal wrote:
>> +static void fun_select_chip(struct mtd_info *mtd, int chip_nr)
>> +{
>> +       struct nand_chip *chip = mtd->priv;
>> +       struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
>> +
>> +       if (chip_nr == -1) {
>> +               chip->cmd_ctrl(mtd, NAND_CMD_NONE, 0 | 
>> NAND_CTRL_CHANGE);
>> +       } else if (chip_nr >= 0) {
>> +               fun->chip_number = chip_nr;
>> +               chip->IO_ADDR_R = chip->IO_ADDR_W =
>> +                       fun->io_base + chip_nr * fun->chip_offset;
>> +       } else {
>> +               BUG();
>> +       }
> braces are not required here...

Really? In the coding style I read:

http://lxr.linux.no/linux+v2.6.29/Documentation/CodingStyle#L171

> 
> 
>> +       prop = of_get_property(ofdev->node, "num-chips", &size);
>> +       if (prop && size == sizeof(uint32_t)) {
>> +               fun->num_chips = *prop;
>> +               if (fun->num_chips >= NAND_MAX_CHIPS) {
>> +                       dev_err(&ofdev->dev, "too much chips");
>> +                       ret = -EINVAL;
>> +                       goto err1;
>> +               }
>> +       } else {
>> +               fun->num_chips = 1;
>> +       }
> ditto...

See above.

Wolfgang.

Re: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Grant Likely]

On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> This patch adds support for multi-chip NAND devices to the FSL-UPM
> driver. This requires support for multiple GPIOs for the RNB pins.
> The NAND chips are selected through address lines defined by the
> FDT property "chip-offset".
>
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>

Hi Wolfgang,

Can you please send a sample device tree snippit for this and add
documentation updates to your patch for the extended binding?

Thanks,
g.

> ---
> =A0arch/powerpc/sysdev/fsl_lbc.c | =A0 =A02 +-
> =A0drivers/mtd/nand/fsl_upm.c =A0 =A0| =A0 95 +++++++++++++++++++++++++++=
++++---------
> =A02 files changed, 74 insertions(+), 23 deletions(-)
>
> diff --git a/arch/powerpc/sysdev/fsl_lbc.c b/arch/powerpc/sysdev/fsl_lbc.=
c
> index 0494ee5..dceb8d1 100644
> --- a/arch/powerpc/sysdev/fsl_lbc.c
> +++ b/arch/powerpc/sysdev/fsl_lbc.c
> @@ -150,7 +150,7 @@ int fsl_upm_run_pattern(struct fsl_upm *upm, void __i=
omem *io_base, u32 mar)
>
> =A0 =A0 =A0 =A0spin_lock_irqsave(&fsl_lbc_lock, flags);
>
> - =A0 =A0 =A0 out_be32(&fsl_lbc_regs->mar, mar << (32 - upm->width));
> + =A0 =A0 =A0 out_be32(&fsl_lbc_regs->mar, mar);
>
> =A0 =A0 =A0 =A0switch (upm->width) {
> =A0 =A0 =A0 =A0case 8:
> diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
> index 7815a40..9b314ce 100644
> --- a/drivers/mtd/nand/fsl_upm.c
> +++ b/drivers/mtd/nand/fsl_upm.c
> @@ -36,8 +36,11 @@ struct fsl_upm_nand {
> =A0 =A0 =A0 =A0uint8_t upm_addr_offset;
> =A0 =A0 =A0 =A0uint8_t upm_cmd_offset;
> =A0 =A0 =A0 =A0void __iomem *io_base;
> - =A0 =A0 =A0 int rnb_gpio;
> + =A0 =A0 =A0 int rnb_gpio[NAND_MAX_CHIPS];
> =A0 =A0 =A0 =A0int chip_delay;
> + =A0 =A0 =A0 uint32_t num_chips;
> + =A0 =A0 =A0 uint32_t chip_number;
> + =A0 =A0 =A0 uint32_t chip_offset;
> =A0};
>
> =A0#define to_fsl_upm_nand(mtd) container_of(mtd, struct fsl_upm_nand, mt=
d)
> @@ -46,7 +49,7 @@ static int fun_chip_ready(struct mtd_info *mtd)
> =A0{
> =A0 =A0 =A0 =A0struct fsl_upm_nand *fun =3D to_fsl_upm_nand(mtd);
>
> - =A0 =A0 =A0 if (gpio_get_value(fun->rnb_gpio))
> + =A0 =A0 =A0 if (gpio_get_value(fun->rnb_gpio[fun->chip_number]))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 1;
>
> =A0 =A0 =A0 =A0dev_vdbg(fun->dev, "busy\n");
> @@ -55,9 +58,9 @@ static int fun_chip_ready(struct mtd_info *mtd)
>
> =A0static void fun_wait_rnb(struct fsl_upm_nand *fun)
> =A0{
> - =A0 =A0 =A0 int cnt =3D 1000000;
>
> - =A0 =A0 =A0 if (fun->rnb_gpio >=3D 0) {
> + =A0 =A0 =A0 if (fun->rnb_gpio[fun->chip_number] >=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 int cnt =3D 1000000;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0while (--cnt && !fun_chip_ready(&fun->mtd)=
)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cpu_relax();
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (!cnt)
> @@ -69,7 +72,9 @@ static void fun_wait_rnb(struct fsl_upm_nand *fun)
>
> =A0static void fun_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int c=
trl)
> =A0{
> + =A0 =A0 =A0 struct nand_chip *chip =3D mtd->priv;
> =A0 =A0 =A0 =A0struct fsl_upm_nand *fun =3D to_fsl_upm_nand(mtd);
> + =A0 =A0 =A0 u32 mar;
>
> =A0 =A0 =A0 =A0if (!(ctrl & fun->last_ctrl)) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl_upm_end_pattern(&fun->upm);
> @@ -87,11 +92,30 @@ static void fun_cmd_ctrl(struct mtd_info *mtd, int cm=
d, unsigned int ctrl)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl_upm_start_pattern(&fun=
->upm, fun->upm_cmd_offset);
> =A0 =A0 =A0 =A0}
>
> - =A0 =A0 =A0 fsl_upm_run_pattern(&fun->upm, fun->io_base, cmd);
> + =A0 =A0 =A0 mar =3D cmd << (32 - fun->upm.width);
> + =A0 =A0 =A0 if (fun->chip_offset && fun->chip_number > 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mar |=3D fun->chip_number * fun->chip_offse=
t;
> + =A0 =A0 =A0 fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>
> =A0 =A0 =A0 =A0fun_wait_rnb(fun);
> =A0}
>
> +static void fun_select_chip(struct mtd_info *mtd, int chip_nr)
> +{
> + =A0 =A0 =A0 struct nand_chip *chip =3D mtd->priv;
> + =A0 =A0 =A0 struct fsl_upm_nand *fun =3D to_fsl_upm_nand(mtd);
> +
> + =A0 =A0 =A0 if (chip_nr =3D=3D -1) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 chip->cmd_ctrl(mtd, NAND_CMD_NONE, 0 | NAND=
_CTRL_CHANGE);
> + =A0 =A0 =A0 } else if (chip_nr >=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->chip_number =3D chip_nr;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 chip->IO_ADDR_R =3D chip->IO_ADDR_W =3D
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->io_base + chip_nr * fu=
n->chip_offset;
> + =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BUG();
> + =A0 =A0 =A0 }
> +}
> +
> =A0static uint8_t fun_read_byte(struct mtd_info *mtd)
> =A0{
> =A0 =A0 =A0 =A0struct fsl_upm_nand *fun =3D to_fsl_upm_nand(mtd);
> @@ -137,8 +161,10 @@ static int __devinit fun_chip_init(struct fsl_upm_na=
nd *fun,
> =A0 =A0 =A0 =A0fun->chip.read_buf =3D fun_read_buf;
> =A0 =A0 =A0 =A0fun->chip.write_buf =3D fun_write_buf;
> =A0 =A0 =A0 =A0fun->chip.ecc.mode =3D NAND_ECC_SOFT;
> + =A0 =A0 =A0 if (fun->num_chips > 1)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->chip.select_chip =3D fun_select_chip;
>
> - =A0 =A0 =A0 if (fun->rnb_gpio >=3D 0)
> + =A0 =A0 =A0 if (fun->rnb_gpio[0] >=3D 0)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fun->chip.dev_ready =3D fun_chip_ready;
>
> =A0 =A0 =A0 =A0fun->mtd.priv =3D &fun->chip;
> @@ -155,7 +181,7 @@ static int __devinit fun_chip_init(struct fsl_upm_nan=
d *fun,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err;
> =A0 =A0 =A0 =A0}
>
> - =A0 =A0 =A0 ret =3D nand_scan(&fun->mtd, 1);
> + =A0 =A0 =A0 ret =3D nand_scan(&fun->mtd, fun->num_chips);
> =A0 =A0 =A0 =A0if (ret)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err;
>
> @@ -187,6 +213,7 @@ static int __devinit fun_probe(struct of_device *ofde=
v,
> =A0 =A0 =A0 =A0const uint32_t *prop;
> =A0 =A0 =A0 =A0int ret;
> =A0 =A0 =A0 =A0int size;
> + =A0 =A0 =A0 int i;
>
> =A0 =A0 =A0 =A0fun =3D kzalloc(sizeof(*fun), GFP_KERNEL);
> =A0 =A0 =A0 =A0if (!fun)
> @@ -208,7 +235,7 @@ static int __devinit fun_probe(struct of_device *ofde=
v,
> =A0 =A0 =A0 =A0if (!prop || size !=3D sizeof(uint32_t)) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_err(&ofdev->dev, "can't get UPM addres=
s offset\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D -EINVAL;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err2;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err1;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0fun->upm_addr_offset =3D *prop;
>
> @@ -216,21 +243,36 @@ static int __devinit fun_probe(struct of_device *of=
dev,
> =A0 =A0 =A0 =A0if (!prop || size !=3D sizeof(uint32_t)) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_err(&ofdev->dev, "can't get UPM comman=
d offset\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D -EINVAL;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err2;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err1;
> =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0fun->upm_cmd_offset =3D *prop;
>
> - =A0 =A0 =A0 fun->rnb_gpio =3D of_get_gpio(ofdev->node, 0);
> - =A0 =A0 =A0 if (fun->rnb_gpio >=3D 0) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D gpio_request(fun->rnb_gpio, dev_nam=
e(&ofdev->dev));
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&ofdev->dev, "can't=
 request RNB gpio\n");
> + =A0 =A0 =A0 prop =3D of_get_property(ofdev->node, "num-chips", &size);
> + =A0 =A0 =A0 if (prop && size =3D=3D sizeof(uint32_t)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->num_chips =3D *prop;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (fun->num_chips >=3D NAND_MAX_CHIPS) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&ofdev->dev, "too m=
uch chips");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -EINVAL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err1;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->num_chips =3D 1;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 for (i =3D 0; i < fun->num_chips; i++) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->rnb_gpio[i] =3D of_get_gpio(ofdev->nod=
e, i);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (fun->rnb_gpio[i] >=3D 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D gpio_request(fun->r=
nb_gpio[i],
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0dev_name(&ofdev->dev));
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&of=
dev->dev, "can't request RNB gpio\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err2;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 gpio_direction_input(fun->r=
nb_gpio[i]);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else if (fun->rnb_gpio[i] =A0=3D=3D -EINV=
AL) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&ofdev->dev, "speci=
fied RNB gpio is invalid\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err2;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 gpio_direction_input(fun->rnb_gpio);
> - =A0 =A0 =A0 } else if (fun->rnb_gpio =3D=3D -EINVAL) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&ofdev->dev, "specified RNB gpio is=
 invalid\n");
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err2;
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0prop =3D of_get_property(ofdev->node, "chip-delay", NULL);
> @@ -239,6 +281,10 @@ static int __devinit fun_probe(struct of_device *ofd=
ev,
> =A0 =A0 =A0 =A0else
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fun->chip_delay =3D 50;
>
> + =A0 =A0 =A0 prop =3D of_get_property(ofdev->node, "chip-offset", &size)=
;
> + =A0 =A0 =A0 if (prop && size =3D=3D sizeof(uint32_t))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->chip_offset =3D *prop;
> +
> =A0 =A0 =A0 =A0fun->io_base =3D devm_ioremap_nocache(&ofdev->dev, io_res.=
start,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0io_res.end - io_res.start + 1);
> =A0 =A0 =A0 =A0if (!fun->io_base) {
> @@ -257,8 +303,10 @@ static int __devinit fun_probe(struct of_device *ofd=
ev,
>
> =A0 =A0 =A0 =A0return 0;
> =A0err2:
> - =A0 =A0 =A0 if (fun->rnb_gpio >=3D 0)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 gpio_free(fun->rnb_gpio);
> + =A0 =A0 =A0 for (i =3D 0; i < fun->num_chips; i++) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (fun->rnb_gpio[i] >=3D 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 gpio_free(fun->rnb_gpio[i])=
;
> + =A0 =A0 =A0 }
> =A0err1:
> =A0 =A0 =A0 =A0kfree(fun);
>
> @@ -268,12 +316,15 @@ err1:
> =A0static int __devexit fun_remove(struct of_device *ofdev)
> =A0{
> =A0 =A0 =A0 =A0struct fsl_upm_nand *fun =3D dev_get_drvdata(&ofdev->dev);
> + =A0 =A0 =A0 int i;
>
> =A0 =A0 =A0 =A0nand_release(&fun->mtd);
> =A0 =A0 =A0 =A0kfree(fun->mtd.name);
>
> - =A0 =A0 =A0 if (fun->rnb_gpio >=3D 0)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 gpio_free(fun->rnb_gpio);
> + =A0 =A0 =A0 =A0for (i =3D 0; i < fun->num_chips; i++) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (fun->rnb_gpio[i] >=3D 0)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpio_free(fun->rnb_gpio[=
i]);
> + =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0kfree(fun);
>
> --
> 1.6.0.6
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Grant Likely]

On Wed, Mar 25, 2009 at 7:31 AM, Grant Likely <grant.likely@secretlab.ca> w=
rote:
> On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg@grandegger.com> =
wrote:
>> This patch adds support for multi-chip NAND devices to the FSL-UPM
>> driver. This requires support for multiple GPIOs for the RNB pins.
>> The NAND chips are selected through address lines defined by the
>> FDT property "chip-offset".
>>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>
> Hi Wolfgang,
>
> Can you please send a sample device tree snippit for this and add
> documentation updates to your patch for the extended binding?

Oh, and cc: devicetree-discuss@ozlabs.org in your next posting.

g.

>> ---
>> =A0arch/powerpc/sysdev/fsl_lbc.c | =A0 =A02 +-
>> =A0drivers/mtd/nand/fsl_upm.c =A0 =A0| =A0 95 ++++++++++++++++++++++++++=
+++++---------
>> =A02 files changed, 74 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/powerpc/sysdev/fsl_lbc.c b/arch/powerpc/sysdev/fsl_lbc=
.c
>> index 0494ee5..dceb8d1 100644
>> --- a/arch/powerpc/sysdev/fsl_lbc.c
>> +++ b/arch/powerpc/sysdev/fsl_lbc.c
>> @@ -150,7 +150,7 @@ int fsl_upm_run_pattern(struct fsl_upm *upm, void __=
iomem *io_base, u32 mar)
>>
>> =A0 =A0 =A0 =A0spin_lock_irqsave(&fsl_lbc_lock, flags);
>>
>> - =A0 =A0 =A0 out_be32(&fsl_lbc_regs->mar, mar << (32 - upm->width));
>> + =A0 =A0 =A0 out_be32(&fsl_lbc_regs->mar, mar);
>>
>> =A0 =A0 =A0 =A0switch (upm->width) {
>> =A0 =A0 =A0 =A0case 8:
>> diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
>> index 7815a40..9b314ce 100644
>> --- a/drivers/mtd/nand/fsl_upm.c
>> +++ b/drivers/mtd/nand/fsl_upm.c
>> @@ -36,8 +36,11 @@ struct fsl_upm_nand {
>> =A0 =A0 =A0 =A0uint8_t upm_addr_offset;
>> =A0 =A0 =A0 =A0uint8_t upm_cmd_offset;
>> =A0 =A0 =A0 =A0void __iomem *io_base;
>> - =A0 =A0 =A0 int rnb_gpio;
>> + =A0 =A0 =A0 int rnb_gpio[NAND_MAX_CHIPS];
>> =A0 =A0 =A0 =A0int chip_delay;
>> + =A0 =A0 =A0 uint32_t num_chips;
>> + =A0 =A0 =A0 uint32_t chip_number;
>> + =A0 =A0 =A0 uint32_t chip_offset;
>> =A0};
>>
>> =A0#define to_fsl_upm_nand(mtd) container_of(mtd, struct fsl_upm_nand, m=
td)
>> @@ -46,7 +49,7 @@ static int fun_chip_ready(struct mtd_info *mtd)
>> =A0{
>> =A0 =A0 =A0 =A0struct fsl_upm_nand *fun =3D to_fsl_upm_nand(mtd);
>>
>> - =A0 =A0 =A0 if (gpio_get_value(fun->rnb_gpio))
>> + =A0 =A0 =A0 if (gpio_get_value(fun->rnb_gpio[fun->chip_number]))
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 1;
>>
>> =A0 =A0 =A0 =A0dev_vdbg(fun->dev, "busy\n");
>> @@ -55,9 +58,9 @@ static int fun_chip_ready(struct mtd_info *mtd)
>>
>> =A0static void fun_wait_rnb(struct fsl_upm_nand *fun)
>> =A0{
>> - =A0 =A0 =A0 int cnt =3D 1000000;
>>
>> - =A0 =A0 =A0 if (fun->rnb_gpio >=3D 0) {
>> + =A0 =A0 =A0 if (fun->rnb_gpio[fun->chip_number] >=3D 0) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 int cnt =3D 1000000;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0while (--cnt && !fun_chip_ready(&fun->mtd=
))
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cpu_relax();
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (!cnt)
>> @@ -69,7 +72,9 @@ static void fun_wait_rnb(struct fsl_upm_nand *fun)
>>
>> =A0static void fun_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int =
ctrl)
>> =A0{
>> + =A0 =A0 =A0 struct nand_chip *chip =3D mtd->priv;
>> =A0 =A0 =A0 =A0struct fsl_upm_nand *fun =3D to_fsl_upm_nand(mtd);
>> + =A0 =A0 =A0 u32 mar;
>>
>> =A0 =A0 =A0 =A0if (!(ctrl & fun->last_ctrl)) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl_upm_end_pattern(&fun->upm);
>> @@ -87,11 +92,30 @@ static void fun_cmd_ctrl(struct mtd_info *mtd, int c=
md, unsigned int ctrl)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fsl_upm_start_pattern(&fu=
n->upm, fun->upm_cmd_offset);
>> =A0 =A0 =A0 =A0}
>>
>> - =A0 =A0 =A0 fsl_upm_run_pattern(&fun->upm, fun->io_base, cmd);
>> + =A0 =A0 =A0 mar =3D cmd << (32 - fun->upm.width);
>> + =A0 =A0 =A0 if (fun->chip_offset && fun->chip_number > 0)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 mar |=3D fun->chip_number * fun->chip_offs=
et;
>> + =A0 =A0 =A0 fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>>
>> =A0 =A0 =A0 =A0fun_wait_rnb(fun);
>> =A0}
>>
>> +static void fun_select_chip(struct mtd_info *mtd, int chip_nr)
>> +{
>> + =A0 =A0 =A0 struct nand_chip *chip =3D mtd->priv;
>> + =A0 =A0 =A0 struct fsl_upm_nand *fun =3D to_fsl_upm_nand(mtd);
>> +
>> + =A0 =A0 =A0 if (chip_nr =3D=3D -1) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 chip->cmd_ctrl(mtd, NAND_CMD_NONE, 0 | NAN=
D_CTRL_CHANGE);
>> + =A0 =A0 =A0 } else if (chip_nr >=3D 0) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->chip_number =3D chip_nr;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 chip->IO_ADDR_R =3D chip->IO_ADDR_W =3D
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->io_base + chip_nr * f=
un->chip_offset;
>> + =A0 =A0 =A0 } else {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 BUG();
>> + =A0 =A0 =A0 }
>> +}
>> +
>> =A0static uint8_t fun_read_byte(struct mtd_info *mtd)
>> =A0{
>> =A0 =A0 =A0 =A0struct fsl_upm_nand *fun =3D to_fsl_upm_nand(mtd);
>> @@ -137,8 +161,10 @@ static int __devinit fun_chip_init(struct fsl_upm_n=
and *fun,
>> =A0 =A0 =A0 =A0fun->chip.read_buf =3D fun_read_buf;
>> =A0 =A0 =A0 =A0fun->chip.write_buf =3D fun_write_buf;
>> =A0 =A0 =A0 =A0fun->chip.ecc.mode =3D NAND_ECC_SOFT;
>> + =A0 =A0 =A0 if (fun->num_chips > 1)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->chip.select_chip =3D fun_select_chip;
>>
>> - =A0 =A0 =A0 if (fun->rnb_gpio >=3D 0)
>> + =A0 =A0 =A0 if (fun->rnb_gpio[0] >=3D 0)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fun->chip.dev_ready =3D fun_chip_ready;
>>
>> =A0 =A0 =A0 =A0fun->mtd.priv =3D &fun->chip;
>> @@ -155,7 +181,7 @@ static int __devinit fun_chip_init(struct fsl_upm_na=
nd *fun,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err;
>> =A0 =A0 =A0 =A0}
>>
>> - =A0 =A0 =A0 ret =3D nand_scan(&fun->mtd, 1);
>> + =A0 =A0 =A0 ret =3D nand_scan(&fun->mtd, fun->num_chips);
>> =A0 =A0 =A0 =A0if (ret)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err;
>>
>> @@ -187,6 +213,7 @@ static int __devinit fun_probe(struct of_device *ofd=
ev,
>> =A0 =A0 =A0 =A0const uint32_t *prop;
>> =A0 =A0 =A0 =A0int ret;
>> =A0 =A0 =A0 =A0int size;
>> + =A0 =A0 =A0 int i;
>>
>> =A0 =A0 =A0 =A0fun =3D kzalloc(sizeof(*fun), GFP_KERNEL);
>> =A0 =A0 =A0 =A0if (!fun)
>> @@ -208,7 +235,7 @@ static int __devinit fun_probe(struct of_device *ofd=
ev,
>> =A0 =A0 =A0 =A0if (!prop || size !=3D sizeof(uint32_t)) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_err(&ofdev->dev, "can't get UPM addre=
ss offset\n");
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D -EINVAL;
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err2;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err1;
>> =A0 =A0 =A0 =A0}
>> =A0 =A0 =A0 =A0fun->upm_addr_offset =3D *prop;
>>
>> @@ -216,21 +243,36 @@ static int __devinit fun_probe(struct of_device *o=
fdev,
>> =A0 =A0 =A0 =A0if (!prop || size !=3D sizeof(uint32_t)) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_err(&ofdev->dev, "can't get UPM comma=
nd offset\n");
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D -EINVAL;
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err2;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err1;
>> =A0 =A0 =A0 =A0}
>> =A0 =A0 =A0 =A0fun->upm_cmd_offset =3D *prop;
>>
>> - =A0 =A0 =A0 fun->rnb_gpio =3D of_get_gpio(ofdev->node, 0);
>> - =A0 =A0 =A0 if (fun->rnb_gpio >=3D 0) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D gpio_request(fun->rnb_gpio, dev_na=
me(&ofdev->dev));
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&ofdev->dev, "can'=
t request RNB gpio\n");
>> + =A0 =A0 =A0 prop =3D of_get_property(ofdev->node, "num-chips", &size);
>> + =A0 =A0 =A0 if (prop && size =3D=3D sizeof(uint32_t)) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->num_chips =3D *prop;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (fun->num_chips >=3D NAND_MAX_CHIPS) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&ofdev->dev, "too =
much chips");
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -EINVAL;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err1;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> + =A0 =A0 =A0 } else {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->num_chips =3D 1;
>> + =A0 =A0 =A0 }
>> +
>> + =A0 =A0 =A0 for (i =3D 0; i < fun->num_chips; i++) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->rnb_gpio[i] =3D of_get_gpio(ofdev->no=
de, i);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (fun->rnb_gpio[i] >=3D 0) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D gpio_request(fun->=
rnb_gpio[i],
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0 =A0dev_name(&ofdev->dev));
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&o=
fdev->dev, "can't request RNB gpio\n");
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err2;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 gpio_direction_input(fun->=
rnb_gpio[i]);
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else if (fun->rnb_gpio[i] =A0=3D=3D -EIN=
VAL) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&ofdev->dev, "spec=
ified RNB gpio is invalid\n");
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err2;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 gpio_direction_input(fun->rnb_gpio);
>> - =A0 =A0 =A0 } else if (fun->rnb_gpio =3D=3D -EINVAL) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&ofdev->dev, "specified RNB gpio i=
s invalid\n");
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err2;
>> =A0 =A0 =A0 =A0}
>>
>> =A0 =A0 =A0 =A0prop =3D of_get_property(ofdev->node, "chip-delay", NULL)=
;
>> @@ -239,6 +281,10 @@ static int __devinit fun_probe(struct of_device *of=
dev,
>> =A0 =A0 =A0 =A0else
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fun->chip_delay =3D 50;
>>
>> + =A0 =A0 =A0 prop =3D of_get_property(ofdev->node, "chip-offset", &size=
);
>> + =A0 =A0 =A0 if (prop && size =3D=3D sizeof(uint32_t))
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 fun->chip_offset =3D *prop;
>> +
>> =A0 =A0 =A0 =A0fun->io_base =3D devm_ioremap_nocache(&ofdev->dev, io_res=
.start,
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 =A0 =A0io_res.end - io_res.start + 1);
>> =A0 =A0 =A0 =A0if (!fun->io_base) {
>> @@ -257,8 +303,10 @@ static int __devinit fun_probe(struct of_device *of=
dev,
>>
>> =A0 =A0 =A0 =A0return 0;
>> =A0err2:
>> - =A0 =A0 =A0 if (fun->rnb_gpio >=3D 0)
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 gpio_free(fun->rnb_gpio);
>> + =A0 =A0 =A0 for (i =3D 0; i < fun->num_chips; i++) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (fun->rnb_gpio[i] >=3D 0)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 gpio_free(fun->rnb_gpio[i]=
);
>> + =A0 =A0 =A0 }
>> =A0err1:
>> =A0 =A0 =A0 =A0kfree(fun);
>>
>> @@ -268,12 +316,15 @@ err1:
>> =A0static int __devexit fun_remove(struct of_device *ofdev)
>> =A0{
>> =A0 =A0 =A0 =A0struct fsl_upm_nand *fun =3D dev_get_drvdata(&ofdev->dev)=
;
>> + =A0 =A0 =A0 int i;
>>
>> =A0 =A0 =A0 =A0nand_release(&fun->mtd);
>> =A0 =A0 =A0 =A0kfree(fun->mtd.name);
>>
>> - =A0 =A0 =A0 if (fun->rnb_gpio >=3D 0)
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 gpio_free(fun->rnb_gpio);
>> + =A0 =A0 =A0 =A0for (i =3D 0; i < fun->num_chips; i++) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (fun->rnb_gpio[i] >=3D 0)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0gpio_free(fun->rnb_gpio=
[i]);
>> + =A0 =A0 =A0 =A0}
>>
>> =A0 =A0 =A0 =A0kfree(fun);
>>
>> --
>> 1.6.0.6
>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@ozlabs.org
>> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>>
>
>
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Wolfgang Grandegger]

Grant Likely wrote:
> On Wed, Mar 25, 2009 at 7:31 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>> This patch adds support for multi-chip NAND devices to the FSL-UPM
>>> driver. This requires support for multiple GPIOs for the RNB pins.
>>> The NAND chips are selected through address lines defined by the
>>> FDT property "chip-offset".
>>>
>>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> Hi Wolfgang,
>>
>> Can you please send a sample device tree snippit for this and add
>> documentation updates to your patch for the extended binding?
> 
> Oh, and cc: devicetree-discuss@ozlabs.org in your next posting.

OK, does patch 3/4 not already contain what you are looking for? See:

http://ozlabs.org/pipermail/linuxppc-dev/2009-March/069787.html

I separated it from the NAND patches because they go through the MTD
maintainer(s).

BTW: did you have a chance to look into the following RFC on I2C bus
speed setting?

 http://ozlabs.org/pipermail/linuxppc-dev/2009-March/069489.html

Thanks,

Wolfgang.

Re: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Grant Likely]

On Wed, Mar 25, 2009 at 7:43 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> Grant Likely wrote:
>> On Wed, Mar 25, 2009 at 7:31 AM, Grant Likely <grant.likely@secretlab.ca=
> wrote:
>>> On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg@grandegger.com=
> wrote:
>>>> This patch adds support for multi-chip NAND devices to the FSL-UPM
>>>> driver. This requires support for multiple GPIOs for the RNB pins.
>>>> The NAND chips are selected through address lines defined by the
>>>> FDT property "chip-offset".
>>>>
>>>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>>> Hi Wolfgang,
>>>
>>> Can you please send a sample device tree snippit for this and add
>>> documentation updates to your patch for the extended binding?
>>
>> Oh, and cc: devicetree-discuss@ozlabs.org in your next posting.
>
> OK, does patch 3/4 not already contain what you are looking for? See:
>
> http://ozlabs.org/pipermail/linuxppc-dev/2009-March/069787.html

Oops, sorry.  Missed that.

> I separated it from the NAND patches because they go through the MTD
> maintainer(s).
>
> BTW: did you have a chance to look into the following RFC on I2C bus
> speed setting?
>
> =A0http://ozlabs.org/pipermail/linuxppc-dev/2009-March/069489.html

No.  Looking at it now.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Anton Vorontsov]

Hi Wolfgang,

On Wed, Mar 25, 2009 at 11:08:18AM +0100, Wolfgang Grandegger wrote:
> This patch adds support for multi-chip NAND devices to the FSL-UPM
> driver. This requires support for multiple GPIOs for the RNB pins.
> The NAND chips are selected through address lines defined by the
> FDT property "chip-offset".
> 
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---

Some cosmetic issues are still there...

>  arch/powerpc/sysdev/fsl_lbc.c |    2 +-
>  drivers/mtd/nand/fsl_upm.c    |   95 +++++++++++++++++++++++++++++++---------
>  2 files changed, 74 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/fsl_lbc.c b/arch/powerpc/sysdev/fsl_lbc.c
> index 0494ee5..dceb8d1 100644
> --- a/arch/powerpc/sysdev/fsl_lbc.c
> +++ b/arch/powerpc/sysdev/fsl_lbc.c
> @@ -150,7 +150,7 @@ int fsl_upm_run_pattern(struct fsl_upm *upm, void __iomem *io_base, u32 mar)
>  
>  	spin_lock_irqsave(&fsl_lbc_lock, flags);
>  
> -	out_be32(&fsl_lbc_regs->mar, mar << (32 - upm->width));
> +	out_be32(&fsl_lbc_regs->mar, mar);
>  
>  	switch (upm->width) {
>  	case 8:
> diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
> index 7815a40..9b314ce 100644
> --- a/drivers/mtd/nand/fsl_upm.c
> +++ b/drivers/mtd/nand/fsl_upm.c
> @@ -36,8 +36,11 @@ struct fsl_upm_nand {
>  	uint8_t upm_addr_offset;
>  	uint8_t upm_cmd_offset;
>  	void __iomem *io_base;
> -	int rnb_gpio;
> +	int rnb_gpio[NAND_MAX_CHIPS];
>  	int chip_delay;
> +	uint32_t num_chips;
> +	uint32_t chip_number;
> +	uint32_t chip_offset;
>  };
>  
>  #define to_fsl_upm_nand(mtd) container_of(mtd, struct fsl_upm_nand, mtd)
> @@ -46,7 +49,7 @@ static int fun_chip_ready(struct mtd_info *mtd)
>  {
>  	struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
>  
> -	if (gpio_get_value(fun->rnb_gpio))
> +	if (gpio_get_value(fun->rnb_gpio[fun->chip_number]))
>  		return 1;
>  
>  	dev_vdbg(fun->dev, "busy\n");
> @@ -55,9 +58,9 @@ static int fun_chip_ready(struct mtd_info *mtd)
>  
>  static void fun_wait_rnb(struct fsl_upm_nand *fun)
>  {
> -	int cnt = 1000000;
>  

This empty line can be removed.

> -	if (fun->rnb_gpio >= 0) {
> +	if (fun->rnb_gpio[fun->chip_number] >= 0) {
> +		int cnt = 1000000;

Add an empty line here.

>  		while (--cnt && !fun_chip_ready(&fun->mtd))
>  			cpu_relax();
>  		if (!cnt)
> @@ -69,7 +72,9 @@ static void fun_wait_rnb(struct fsl_upm_nand *fun)
>  
>  static void fun_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  {
> +	struct nand_chip *chip = mtd->priv;
>  	struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
> +	u32 mar;
>  
>  	if (!(ctrl & fun->last_ctrl)) {
>  		fsl_upm_end_pattern(&fun->upm);
> @@ -87,11 +92,30 @@ static void fun_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  			fsl_upm_start_pattern(&fun->upm, fun->upm_cmd_offset);
>  	}
>  
> -	fsl_upm_run_pattern(&fun->upm, fun->io_base, cmd);
> +	mar = cmd << (32 - fun->upm.width);
> +	if (fun->chip_offset && fun->chip_number > 0)
> +		mar |= fun->chip_number * fun->chip_offset;
> +	fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>  
>  	fun_wait_rnb(fun);
>  }
>  
> +static void fun_select_chip(struct mtd_info *mtd, int chip_nr)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
> +
> +	if (chip_nr == -1) {
> +		chip->cmd_ctrl(mtd, NAND_CMD_NONE, 0 | NAND_CTRL_CHANGE);
> +	} else if (chip_nr >= 0) {
> +		fun->chip_number = chip_nr;
> +		chip->IO_ADDR_R = chip->IO_ADDR_W =
> +			fun->io_base + chip_nr * fun->chip_offset;

Please avoid = = constructions.

chip->IO_ADDR_R = fun->io_base + chip_nr * fun->chip_offset;
chip->IO_ADDR_W = chip->IO_ADDR_R;

^^ looks much better.

> +			
> +	} else {
> +		BUG();
> +	}
> +}
> +
>  static uint8_t fun_read_byte(struct mtd_info *mtd)
>  {
>  	struct fsl_upm_nand *fun = to_fsl_upm_nand(mtd);
> @@ -137,8 +161,10 @@ static int __devinit fun_chip_init(struct fsl_upm_nand *fun,
>  	fun->chip.read_buf = fun_read_buf;
>  	fun->chip.write_buf = fun_write_buf;
>  	fun->chip.ecc.mode = NAND_ECC_SOFT;
> +	if (fun->num_chips > 1)
> +		fun->chip.select_chip = fun_select_chip;
>  
> -	if (fun->rnb_gpio >= 0)
> +	if (fun->rnb_gpio[0] >= 0)
>  		fun->chip.dev_ready = fun_chip_ready;
>  
>  	fun->mtd.priv = &fun->chip;
> @@ -155,7 +181,7 @@ static int __devinit fun_chip_init(struct fsl_upm_nand *fun,
>  		goto err;
>  	}
>  
> -	ret = nand_scan(&fun->mtd, 1);
> +	ret = nand_scan(&fun->mtd, fun->num_chips);
>  	if (ret)
>  		goto err;
>  
> @@ -187,6 +213,7 @@ static int __devinit fun_probe(struct of_device *ofdev,
>  	const uint32_t *prop;
>  	int ret;
>  	int size;
> +	int i;
>  
>  	fun = kzalloc(sizeof(*fun), GFP_KERNEL);
>  	if (!fun)
> @@ -208,7 +235,7 @@ static int __devinit fun_probe(struct of_device *ofdev,
>  	if (!prop || size != sizeof(uint32_t)) {
>  		dev_err(&ofdev->dev, "can't get UPM address offset\n");
>  		ret = -EINVAL;
> -		goto err2;
> +		goto err1;
>  	}
>  	fun->upm_addr_offset = *prop;
>  
> @@ -216,21 +243,36 @@ static int __devinit fun_probe(struct of_device *ofdev,
>  	if (!prop || size != sizeof(uint32_t)) {
>  		dev_err(&ofdev->dev, "can't get UPM command offset\n");
>  		ret = -EINVAL;
> -		goto err2;
> +		goto err1;
>  	}
>  	fun->upm_cmd_offset = *prop;
>  
> -	fun->rnb_gpio = of_get_gpio(ofdev->node, 0);
> -	if (fun->rnb_gpio >= 0) {
> -		ret = gpio_request(fun->rnb_gpio, dev_name(&ofdev->dev));
> -		if (ret) {
> -			dev_err(&ofdev->dev, "can't request RNB gpio\n");
> +	prop = of_get_property(ofdev->node, "num-chips", &size);
> +	if (prop && size == sizeof(uint32_t)) {
> +		fun->num_chips = *prop;
> +		if (fun->num_chips >= NAND_MAX_CHIPS) {
> +			dev_err(&ofdev->dev, "too much chips");

\n is missing in dev_err().

> +			ret = -EINVAL;
> +			goto err1;
> +		}
> +	} else {
> +		fun->num_chips = 1;
> +	}
> +
> +	for (i = 0; i < fun->num_chips; i++) {
> +		fun->rnb_gpio[i] = of_get_gpio(ofdev->node, i);
> +		if (fun->rnb_gpio[i] >= 0) {
> +			ret = gpio_request(fun->rnb_gpio[i], 

trailing whitespace on that line.

> +					   dev_name(&ofdev->dev));
> +			if (ret) {
> +				dev_err(&ofdev->dev, "can't request RNB gpio\n");

line is over 80 chars.

> +				goto err2;
> +			}
> +			gpio_direction_input(fun->rnb_gpio[i]);
> +		} else if (fun->rnb_gpio[i]  == -EINVAL) {

stray whitespace in "  ==".

> +			dev_err(&ofdev->dev, "specified RNB gpio is invalid\n");
>  			goto err2;
>  		}
> -		gpio_direction_input(fun->rnb_gpio);
> -	} else if (fun->rnb_gpio == -EINVAL) {
> -		dev_err(&ofdev->dev, "specified RNB gpio is invalid\n");
> -		goto err2;
>  	}
>  
>  	prop = of_get_property(ofdev->node, "chip-delay", NULL);
> @@ -239,6 +281,10 @@ static int __devinit fun_probe(struct of_device *ofdev,
>  	else
>  		fun->chip_delay = 50;
>  
> +	prop = of_get_property(ofdev->node, "chip-offset", &size);
> +	if (prop && size == sizeof(uint32_t))
> +		fun->chip_offset = *prop;
> +
>  	fun->io_base = devm_ioremap_nocache(&ofdev->dev, io_res.start,
>  					  io_res.end - io_res.start + 1);
>  	if (!fun->io_base) {
> @@ -257,8 +303,10 @@ static int __devinit fun_probe(struct of_device *ofdev,
>  
>  	return 0;
>  err2:
> -	if (fun->rnb_gpio >= 0)
> -		gpio_free(fun->rnb_gpio);
> +	for (i = 0; i < fun->num_chips; i++) {
> +		if (fun->rnb_gpio[i] >= 0)
> +			gpio_free(fun->rnb_gpio[i]);
> +	}
>  err1:
>  	kfree(fun);
>  
> @@ -268,12 +316,15 @@ err1:
>  static int __devexit fun_remove(struct of_device *ofdev)
>  {
>  	struct fsl_upm_nand *fun = dev_get_drvdata(&ofdev->dev);
> +	int i;
>  
>  	nand_release(&fun->mtd);
>  	kfree(fun->mtd.name);
>  
> -	if (fun->rnb_gpio >= 0)
> -		gpio_free(fun->rnb_gpio);
> +        for (i = 0; i < fun->num_chips; i++) {
> +                if (fun->rnb_gpio[i] >= 0)
> +                        gpio_free(fun->rnb_gpio[i]);
> +        }

code indent should use tabs where possible (not white spaces).


When the cosmetic issues are fixed, I'll readily ack this patch.

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH v3 1/4] NAND: FSL-UPM: add multi chip support

[Wolfgang Grandegger]

Hi Anton,

Anton Vorontsov wrote:
> Hi Wolfgang,
> 
> On Wed, Mar 25, 2009 at 11:08:18AM +0100, Wolfgang Grandegger wrote:
>> This patch adds support for multi-chip NAND devices to the FSL-UPM
>> driver. This requires support for multiple GPIOs for the RNB pins.
>> The NAND chips are selected through address lines defined by the
>> FDT property "chip-offset".
>>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> ---
> 
> Some cosmetic issues are still there...

Normally I use checkpatch.pl to validate my patches, but ... grrr,
sorry. It finds all the obvious issues you reported.

[snip]

> When the cosmetic issues are fixed, I'll readily ack this patch.

Thanks, will resend a.s.a.p.

Wolfgang.