mtd/nand: fsmc driver updates

Signed-off-by Vipin Kumar <[email protected]>

---
 drivers/mtd/nand/fsmc_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mtd/nand/fsmc_nand.c +0 -0

index 38d2624..e96d7d1 100644

Signed-off-by Vipin Kumar <[email protected]>

Signed-off-by Vipin Kumar <[email protected]>

Signed-off-by Vipin Kumar <[email protected]>

---
 drivers/mtd/nand/fsmc_nand.c | 51 ++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

drivers/mtd/nand/fsmc_nand.c +24 -19

index e96d7d1..bd89580 100644
@@ -278,52 +278,61 @@ static struct fsmc_eccplace fsmc_ecc4_sp_place = {
  * struct fsmc_nand_data - structure for FSMC NAND device state
  *
  * @pid:		Part ID on the AMBA PrimeCell format
- * @mtd:		MTD info for a NAND flash.
- * @nand:		Chip related info for a NAND flash.
- * @partitions:		Partition info for a NAND Flash.
- * @nr_partitions:	Total number of partition of a NAND flash.
- *
- * @ecc_place:		ECC placing locations in oobfree type format.
- * @bank:		Bank number for probed device.
- * @clk:		Clock structure for FSMC.
+ * @mtd:		MTD info for a NAND flash
+ * @nand:		Chip related info for a NAND flash
+ * @dev:		Device structure pointer
+ * @clk:		Clock structure for FSMC
+ * @ecc_place:		ECC placing locations in oobfree type format
+ * @bank:		Bank number for probed device
  *
  * @read_dma_chan:	DMA channel for read access
  * @write_dma_chan:	DMA channel for write access to NAND
  * @dma_access_complete: Completion structure
  *
- * @data_pa:		NAND Physical port for Data.
- * @data_va:		NAND port for Data.
- * @cmd_va:		NAND port for Command.
- * @addr_va:		NAND port for Address.
- * @regs_va:		FSMC regs base address.
+ * @dev_timings:	Timings to be programmed in controller
+ * @partitions:		Partition info for a NAND Flash
+ * @nr_partitions:	Total number of partition of a NAND flash
+ * @mode:		Defines the NAND device access mode
+ *			Can be one of:
+ *			- DMA access
+ *			- Word access (CPU)
+ *			- None (Use driver default ie bus width specific
+ *			  CPU access)
+ * @select_chip:	Select a particular bank
+ *
+ * @data_pa:		NAND Physical port for Data
+ * @data_va:		NAND port for Data
+ * @cmd_va:		NAND port for Command
+ * @addr_va:		NAND port for Address
+ * @regs_va:		FSMC regs base address
  */
 struct fsmc_nand_data {
 	u32			pid;
 	struct mtd_info		mtd;
 	struct nand_chip	nand;
-	struct mtd_partition	*partitions;
-	unsigned int		nr_partitions;
-
-	struct fsmc_eccplace	*ecc_place;
-	unsigned int		bank;
 	struct device		*dev;
-	enum access_mode	mode;
 	struct clk		*clk;
+	struct fsmc_eccplace	*ecc_place;
+	unsigned int		bank;

drivers/mtd/nand/fsmc_nand.c +3 -2

+	enum access_mode	mode;
+	void			(*select_chip)(uint32_t bank, uint32_t busw);
 
+	/* Virtual/Physical addresses for CPU/DMA access */
 	dma_addr_t		data_pa;
 	void __iomem		*data_va;
 	void __iomem		*cmd_va;
 	void __iomem		*addr_va;
 	void __iomem		*regs_va;
-
-	void			(*select_chip)(uint32_t bank, uint32_t busw);
 };
 
 /* Assert CS signal based on chipnr */

Signature · 1 line

1.7.11.4

Quoted 74 lines · Vipin Kumar

Signed-off-by Vipin Kumar<[email protected]>

---
  drivers/mtd/nand/fsmc_nand.c | 51 ++++++++++++++++++++++++++------------------
  1 file changed, 30 insertions(+), 21 deletions(-)

drivers/mtd/nand/fsmc_nand.c +24 -19

index e96d7d1..bd89580 100644
@@ -278,52 +278,61 @@ static struct fsmc_eccplace fsmc_ecc4_sp_place = {
   * struct fsmc_nand_data - structure for FSMC NAND device state
   *
   * @pid:		Part ID on the AMBA PrimeCell format
- * @mtd:		MTD info for a NAND flash.
- * @nand:		Chip related info for a NAND flash.
- * @partitions:		Partition info for a NAND Flash.
- * @nr_partitions:	Total number of partition of a NAND flash.
- *
- * @ecc_place:		ECC placing locations in oobfree type format.
- * @bank:		Bank number for probed device.
- * @clk:		Clock structure for FSMC.
+ * @mtd:		MTD info for a NAND flash
+ * @nand:		Chip related info for a NAND flash
+ * @dev:		Device structure pointer
+ * @clk:		Clock structure for FSMC
+ * @ecc_place:		ECC placing locations in oobfree type format
+ * @bank:		Bank number for probed device
   *
   * @read_dma_chan:	DMA channel for read access
   * @write_dma_chan:	DMA channel for write access to NAND
   * @dma_access_complete: Completion structure
   *
- * @data_pa:		NAND Physical port for Data.
- * @data_va:		NAND port for Data.
- * @cmd_va:		NAND port for Command.
- * @addr_va:		NAND port for Address.
- * @regs_va:		FSMC regs base address.
+ * @dev_timings:	Timings to be programmed in controller
+ * @partitions:		Partition info for a NAND Flash
+ * @nr_partitions:	Total number of partition of a NAND flash
+ * @mode:		Defines the NAND device access mode
+ *			Can be one of:
+ *			- DMA access
+ *			- Word access (CPU)
+ *			- None (Use driver default ie bus width specific
+ *			  CPU access)
+ * @select_chip:	Select a particular bank
+ *
+ * @data_pa:		NAND Physical port for Data
+ * @data_va:		NAND port for Data
+ * @cmd_va:		NAND port for Command
+ * @addr_va:		NAND port for Address
+ * @regs_va:		FSMC regs base address
   */
  struct fsmc_nand_data {
  	u32			pid;
  	struct mtd_info		mtd;
  	struct nand_chip	nand;
-	struct mtd_partition	*partitions;
-	unsigned int		nr_partitions;
-
-	struct fsmc_eccplace	*ecc_place;
-	unsigned int		bank;
  	struct device		*dev;
-	enum access_mode	mode;
  	struct clk		*clk;
+	struct fsmc_eccplace	*ecc_place;
+	unsigned int		bank;
 
  	/* DMA related objects */
  	struct dma_chan		*read_dma_chan;
  	struct dma_chan		*write_dma_chan;
  	struct completion	dma_access_complete;
 

drivers/mtd/nand/fsmc_nand.c static struct fsmc_eccplace fsmc_ecc4_sp_place = { · line 326

Quoted 4 lines

drivers/mtd/nand/fsmc_nand.c +3 -0

+	/* Recieved from plat data */
  	struct fsmc_nand_timings *dev_timings;
+	struct mtd_partition	*partitions;
+	unsigned int		nr_partitions;

where do you use those execpt at probe time

Signed-off-by Vipin Kumar <[email protected]>

---
 drivers/mtd/nand/fsmc_nand.c | 51 ++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

Support up to max_banks number of banks in fsmc driver.

Signed-off-by Vipin Kumar <[email protected]>

---
 .../devicetree/bindings/mtd/fsmc-nand.txt          |  2 ++
 arch/arm/boot/dts/spear300.dtsi                    |  1 +
 arch/arm/boot/dts/spear310.dtsi                    |  1 +
 arch/arm/boot/dts/spear320.dtsi                    |  1 +
 arch/arm/boot/dts/spear600.dtsi                    |  1 +
 arch/arm/mach-u300/core.c                          |  1 +
 drivers/mtd/nand/fsmc_nand.c                       | 26 +++++++++++++++-------
 include/linux/mtd/fsmc.h                           |  2 +-
 8 files changed, 26 insertions(+), 9 deletions(-)

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +0 -0

index e2c663b..29d1a2f 100644

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +1 -0

 Optional properties:
 - bank-width : Width (in bytes) of the device.  If not present, the width

@@ -23,6 +24,7 @@ Example:
 		reg-names = "fsmc_regs", "nand_data";
 		st,ale-off = <0x20000>;
 		st,cle-off = <0x10000>;
+		maxbanks = <1>;
 
 		bank-width = <1>;
 		nand-skip-bbtscan;

arch/arm/boot/dts/spear300.dtsi +1 -0

index ed3627c..19e2328 100644
@@ -42,6 +42,7 @@
 			reg-names = "fsmc_regs", "nand_data";
 			st,ale-off = <0x20000>;
 			st,cle-off = <0x10000>;
+			maxbanks = <1>;
 			status = "disabled";
 		};
 

arch/arm/boot/dts/spear310.dtsi +1 -0

index 62fc4fb..0272afb3 100644
@@ -36,6 +36,7 @@
 			reg-names = "fsmc_regs", "nand_data";
 			st,ale-off = <0x10000>;
 			st,cle-off = <0x20000>;
+			maxbanks = <1>;
 			status = "disabled";
 		};
 

arch/arm/boot/dts/spear320.dtsi +1 -0

index 1f49d69..69fe50d 100644
@@ -42,6 +42,7 @@
 			reg-names = "fsmc_regs", "nand_data";
 			st,ale-off = <0x20000>;
 			st,cle-off = <0x10000>;
+			maxbanks = <1>;
 			status = "disabled";
 		};
 

arch/arm/boot/dts/spear600.dtsi +1 -0

index a3c36e4..6ed57c8 100644
@@ -71,6 +71,7 @@
 			reg-names = "fsmc_regs", "nand_data";
 			st,ale-off = <0x20000>;
 			st,cle-off = <0x10000>;
+			maxbanks = <1>;
 			status = "disabled";
 		};
 

arch/arm/mach-u300/core.c +1 -0

index 03acf18..5ea9f71 100644
@@ -1546,6 +1546,7 @@ static struct fsmc_nand_platform_data nand_platform_data = {
 	.width = FSMC_NAND_BW8,
 	.ale_off = PLAT_NAND_ALE,
 	.cle_off = PLAT_NAND_CLE,
+	.max_banks = 1,
 };
 
 static struct platform_device nand_device = {

drivers/mtd/nand/fsmc_nand.c +18 -8

index bd89580..fc6a044 100644
@@ -298,6 +298,7 @@ static struct fsmc_eccplace fsmc_ecc4_sp_place = {
  *			- Word access (CPU)
  *			- None (Use driver default ie bus width specific
  *			  CPU access)
+ * @max_banks:		Maximum number of banks supported
  * @select_chip:	Select a particular bank
  *
  * @data_pa:		NAND Physical port for Data

@@ -325,6 +326,7 @@ struct fsmc_nand_data {
 	struct mtd_partition	*partitions;
 	unsigned int		nr_partitions;
 	enum access_mode	mode;
+	uint32_t		max_banks;
 	void			(*select_chip)(uint32_t bank, uint32_t busw);
 
 	/* Virtual/Physical addresses for CPU/DMA access */

@@ -343,6 +345,7 @@ static void fsmc_select_chip(struct mtd_info *mtd, int chipnr)
 
 	host = container_of(mtd, struct fsmc_nand_data, mtd);
 
+	host->bank = chipnr;
 	switch (chipnr) {
 	case -1:
 		chip->cmd_ctrl(mtd, NAND_CMD_NONE, 0 | NAND_CTRL_CHANGE);

@@ -889,6 +892,7 @@ static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
 	of_property_read_u32(np, "st,cle-off", &pdata->cle_off);
 	if (of_get_property(np, "nand-skip-bbtscan", NULL))
 		pdata->options = NAND_SKIP_BBTSCAN;
+	of_property_read_u32(np, "maxbanks", &pdata->max_banks);
 
 	return 0;
 }

@@ -915,7 +919,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 	struct resource *res;
 	dma_cap_mask_t mask;
 	int ret = 0;
-	u32 pid;
+	u32 pid, bank;
 	int i;
 
 	if (np) {

@@ -1023,13 +1027,13 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 		 AMBA_PART_BITS(pid), AMBA_MANF_BITS(pid),
 		 AMBA_REV_BITS(pid), AMBA_CONFIG_BITS(pid));
 
-	host->bank = pdata->bank;
 	host->select_chip = pdata->select_bank;
 	host->partitions = pdata->partitions;
 	host->nr_partitions = pdata->nr_partitions;
 	host->dev = &pdev->dev;
 	host->dev_timings = pdata->nand_timings;
 	host->mode = pdata->mode;
+	host->max_banks = pdata->max_banks;
 
 	if (host->mode == USE_DMA_ACCESS)
 		init_completion(&host->dma_access_complete);

@@ -1083,9 +1087,10 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 		break;
 	}
 
-	fsmc_nand_setup(host->regs_va, host->bank,
-			nand->options & NAND_BUSWIDTH_16,
-			host->dev_timings);
+	for (bank = 0; bank < host->max_banks; bank++)
+		fsmc_nand_setup(host->regs_va, bank,
+				nand->options & NAND_BUSWIDTH_16,
+				host->dev_timings);
 
 	if (AMBA_REV_BITS(host->pid) >= 8) {
 		nand->ecc.read_page = fsmc_read_page_hwecc;

@@ -1230,11 +1235,16 @@ static int fsmc_nand_suspend(struct device *dev)
 static int fsmc_nand_resume(struct device *dev)
 {
 	struct fsmc_nand_data *host = dev_get_drvdata(dev);
+	uint32_t bank;
+
 	if (host) {
 		clk_prepare_enable(host->clk);
-		fsmc_nand_setup(host->regs_va, host->bank,
-				host->nand.options & NAND_BUSWIDTH_16,
-				host->dev_timings);
+
+		for (bank = 0; bank < host->max_banks; bank++)
+			fsmc_nand_setup(host->regs_va, bank,
+					host->nand.options & NAND_BUSWIDTH_16,
+					host->dev_timings);
+
 	}
 	return 0;
 }

include/linux/mtd/fsmc.h +1 -1

index b200292..f0ab734 100644
@@ -153,7 +153,7 @@ struct fsmc_nand_platform_data {
 	unsigned int		nr_partitions;
 	unsigned int		options;
 	unsigned int		width;
-	unsigned int		bank;
+	unsigned int		max_banks;
 
 	/* CLE, ALE offsets */
 	unsigned int		cle_off;

Signature · 1 line

1.7.11.4

Quoted 3 lines · Vipin Kumar

Support up to max_banks number of banks in fsmc driver.

Signed-off-by Vipin Kumar<[email protected]>

please rebase this over my patch that drop -off to resource

Quoted 11 lines

---
  .../devicetree/bindings/mtd/fsmc-nand.txt          |  2 ++
  arch/arm/boot/dts/spear300.dtsi                    |  1 +
  arch/arm/boot/dts/spear310.dtsi                    |  1 +
  arch/arm/boot/dts/spear320.dtsi                    |  1 +
  arch/arm/boot/dts/spear600.dtsi                    |  1 +
  arch/arm/mach-u300/core.c                          |  1 +
  drivers/mtd/nand/fsmc_nand.c                       | 26 +++++++++++++++-------
  include/linux/mtd/fsmc.h                           |  2 +-
  8 files changed, 26 insertions(+), 9 deletions(-)

Documentation/devicetree/bindings/mtd/fsmc-nand.txt Required properties: · line 9

Quoted 9 lines

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +1 -0

index e2c663b..29d1a2f 100644
@@ -6,6 +6,7 @@ Required properties:
  - reg-names: Should contain the reg names "fsmc_regs" and "nand_data"
  - st,ale-off : Chip specific offset to ALE
  - st,cle-off : Chip specific offset to CLE
+- maxbanks: Number of banks supported by SoC

make it optionnal will simplify the binding with 1 by default and will allow to avoid issue

Quoted 179 lines

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +1 -0

  Optional properties:
  - bank-width : Width (in bytes) of the device.  If not present, the width

@@ -23,6 +24,7 @@ Example:
  		reg-names = "fsmc_regs", "nand_data";
  		st,ale-off =<0x20000>;
  		st,cle-off =<0x10000>;
+		maxbanks =<1>;
 
  		bank-width =<1>;
  		nand-skip-bbtscan;

arch/arm/boot/dts/spear300.dtsi +1 -0

index ed3627c..19e2328 100644
@@ -42,6 +42,7 @@
  			reg-names = "fsmc_regs", "nand_data";
  			st,ale-off =<0x20000>;
  			st,cle-off =<0x10000>;
+			maxbanks =<1>;
  			status = "disabled";
  		};
 

arch/arm/boot/dts/spear310.dtsi +1 -0

index 62fc4fb..0272afb3 100644
@@ -36,6 +36,7 @@
  			reg-names = "fsmc_regs", "nand_data";
  			st,ale-off =<0x10000>;
  			st,cle-off =<0x20000>;
+			maxbanks =<1>;
  			status = "disabled";
  		};
 

arch/arm/boot/dts/spear320.dtsi +1 -0

index 1f49d69..69fe50d 100644
@@ -42,6 +42,7 @@
  			reg-names = "fsmc_regs", "nand_data";
  			st,ale-off =<0x20000>;
  			st,cle-off =<0x10000>;
+			maxbanks =<1>;
  			status = "disabled";
  		};
 

arch/arm/boot/dts/spear600.dtsi +1 -0

index a3c36e4..6ed57c8 100644
@@ -71,6 +71,7 @@
  			reg-names = "fsmc_regs", "nand_data";
  			st,ale-off =<0x20000>;
  			st,cle-off =<0x10000>;
+			maxbanks =<1>;
  			status = "disabled";
  		};
 

arch/arm/mach-u300/core.c +1 -0

index 03acf18..5ea9f71 100644
@@ -1546,6 +1546,7 @@ static struct fsmc_nand_platform_data nand_platform_data = {
  	.width = FSMC_NAND_BW8,
  	.ale_off = PLAT_NAND_ALE,
  	.cle_off = PLAT_NAND_CLE,
+	.max_banks = 1,
  };
 
  static struct platform_device nand_device = {

drivers/mtd/nand/fsmc_nand.c +18 -8

index bd89580..fc6a044 100644
@@ -298,6 +298,7 @@ static struct fsmc_eccplace fsmc_ecc4_sp_place = {
   *			- Word access (CPU)
   *			- None (Use driver default ie bus width specific
   *			  CPU access)
+ * @max_banks:		Maximum number of banks supported
   * @select_chip:	Select a particular bank
   *
   * @data_pa:		NAND Physical port for Data

@@ -325,6 +326,7 @@ struct fsmc_nand_data {
  	struct mtd_partition	*partitions;
  	unsigned int		nr_partitions;
  	enum access_mode	mode;
+	uint32_t		max_banks;
  	void			(*select_chip)(uint32_t bank, uint32_t busw);
 
  	/* Virtual/Physical addresses for CPU/DMA access */

@@ -343,6 +345,7 @@ static void fsmc_select_chip(struct mtd_info *mtd, int chipnr)
 
  	host = container_of(mtd, struct fsmc_nand_data, mtd);
 
+	host->bank = chipnr;
  	switch (chipnr) {
  	case -1:
  		chip->cmd_ctrl(mtd, NAND_CMD_NONE, 0 | NAND_CTRL_CHANGE);

@@ -889,6 +892,7 @@ static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
  	of_property_read_u32(np, "st,cle-off",&pdata->cle_off);
  	if (of_get_property(np, "nand-skip-bbtscan", NULL))
  		pdata->options = NAND_SKIP_BBTSCAN;
+	of_property_read_u32(np, "maxbanks",&pdata->max_banks);
 
  	return 0;
  }

@@ -915,7 +919,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
  	struct resource *res;
  	dma_cap_mask_t mask;
  	int ret = 0;
-	u32 pid;
+	u32 pid, bank;
  	int i;
 
  	if (np) {

@@ -1023,13 +1027,13 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
  		 AMBA_PART_BITS(pid), AMBA_MANF_BITS(pid),
  		 AMBA_REV_BITS(pid), AMBA_CONFIG_BITS(pid));
 
-	host->bank = pdata->bank;
  	host->select_chip = pdata->select_bank;
  	host->partitions = pdata->partitions;
  	host->nr_partitions = pdata->nr_partitions;
  	host->dev =&pdev->dev;
  	host->dev_timings = pdata->nand_timings;
  	host->mode = pdata->mode;
+	host->max_banks = pdata->max_banks;
 
  	if (host->mode == USE_DMA_ACCESS)
  		init_completion(&host->dma_access_complete);

@@ -1083,9 +1087,10 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
  		break;
  	}
 
-	fsmc_nand_setup(host->regs_va, host->bank,
-			nand->options&  NAND_BUSWIDTH_16,
-			host->dev_timings);
+	for (bank = 0; bank<  host->max_banks; bank++)
+		fsmc_nand_setup(host->regs_va, bank,
+				nand->options&  NAND_BUSWIDTH_16,
+				host->dev_timings);
 
  	if (AMBA_REV_BITS(host->pid)>= 8) {
  		nand->ecc.read_page = fsmc_read_page_hwecc;

@@ -1230,11 +1235,16 @@ static int fsmc_nand_suspend(struct device *dev)
  static int fsmc_nand_resume(struct device *dev)
  {
  	struct fsmc_nand_data *host = dev_get_drvdata(dev);
+	uint32_t bank;
+
  	if (host) {
  		clk_prepare_enable(host->clk);
-		fsmc_nand_setup(host->regs_va, host->bank,
-				host->nand.options&  NAND_BUSWIDTH_16,
-				host->dev_timings);
+
+		for (bank = 0; bank<  host->max_banks; bank++)
+			fsmc_nand_setup(host->regs_va, bank,
+					host->nand.options&  NAND_BUSWIDTH_16,
+					host->dev_timings);
+
  	}
  	return 0;
  }

include/linux/mtd/fsmc.h +1 -2

index b200292..f0ab734 100644
@@ -153,7 +153,7 @@ struct fsmc_nand_platform_data {
  	unsigned int		nr_partitions;
  	unsigned int		options;
  	unsigned int		width;
-	unsigned int		bank;
+	unsigned int		max_banks;
 
  	/* CLE, ALE offsets */
  	unsigned int		cle_off;
--

Quoted 2 lines

1.7.11.4

.

Support up to max_banks number of banks in fsmc driver.

Signed-off-by Vipin Kumar <[email protected]>

Add support to accept nand timing parameters via device tree

Signed-off-by Vipin Kumar <[email protected]>

---
 .../devicetree/bindings/mtd/fsmc-nand.txt          | 20 ++++++++
 drivers/mtd/nand/fsmc_nand.c                       | 59 ++++++++++++++--------
 include/linux/mtd/fsmc.h                           |  2 +-
 3 files changed, 59 insertions(+), 22 deletions(-)

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +20 -0

index 29d1a2f..6a7fc43 100644
@@ -13,6 +13,18 @@ Optional properties:
   defaults to 1 byte
 - nand-skip-bbtscan: Indicates the the BBT scanning should be skipped
 
+- nand-timings-enabled: Indicates if the timing parameters are passed
+  via DT
+- nand-timings,tclr:
+- nand-timings,tar:
+- nand-timings,thiz:
+- nand-timings,thold:
+- nand-timings,twait:
+- nand-timings,tset: All these timing parameters come from the actual
+  nand device specification. Each of this represents a number of time
+  period of hclk ie a number 4 in thold with hclk = 166MHz means that
+  thold = (1000 / 166) * 4 ns = 24.09ns
+
 Example:
 
 	fsmc: flash at d1800000 {

@@ -26,6 +38,14 @@ Example:
 		st,cle-off = <0x10000>;
 		maxbanks = <1>;
 
+		nand-timings-enabled;
+		nand-timings,tclr = <1>;
+		nand-timings,tar = <1>;
+		nand-timings,thiz = <1>;
+		nand-timings,thold = <4>;
+		nand-timings,twait = <6>;
+		nand-timings,tset = <0>;
+
 		bank-width = <1>;
 		nand-skip-bbtscan;
 

drivers/mtd/nand/fsmc_nand.c +14 -20

index fc6a044..f3d69b3 100644
@@ -415,27 +415,13 @@ static void fsmc_nand_setup(void __iomem *regs, uint32_t bank,
 {
 	uint32_t value = FSMC_DEVTYPE_NAND | FSMC_ENABLE | FSMC_WAITON;
 	uint32_t tclr, tar, thiz, thold, twait, tset;
-	struct fsmc_nand_timings *tims;
-	struct fsmc_nand_timings default_timings = {
-		.tclr	= FSMC_TCLR_1,
-		.tar	= FSMC_TAR_1,
-		.thiz	= FSMC_THIZ_1,
-		.thold	= FSMC_THOLD_4,
-		.twait	= FSMC_TWAIT_6,
-		.tset	= FSMC_TSET_0,
-	};
-
-	if (timings)
-		tims = timings;
-	else
-		tims = &default_timings;
 
-	tclr = (tims->tclr & FSMC_TCLR_MASK) << FSMC_TCLR_SHIFT;
-	tar = (tims->tar & FSMC_TAR_MASK) << FSMC_TAR_SHIFT;
-	thiz = (tims->thiz & FSMC_THIZ_MASK) << FSMC_THIZ_SHIFT;
-	thold = (tims->thold & FSMC_THOLD_MASK) << FSMC_THOLD_SHIFT;
-	twait = (tims->twait & FSMC_TWAIT_MASK) << FSMC_TWAIT_SHIFT;
-	tset = (tims->tset & FSMC_TSET_MASK) << FSMC_TSET_SHIFT;
+	tclr = (timings->tclr & FSMC_TCLR_MASK) << FSMC_TCLR_SHIFT;
+	tar = (timings->tar & FSMC_TAR_MASK) << FSMC_TAR_SHIFT;
+	thiz = (timings->thiz & FSMC_THIZ_MASK) << FSMC_THIZ_SHIFT;
+	thold = (timings->thold & FSMC_THOLD_MASK) << FSMC_THOLD_SHIFT;
+	twait = (timings->twait & FSMC_TWAIT_MASK) << FSMC_TWAIT_SHIFT;
+	tset = (timings->tset & FSMC_TSET_MASK) << FSMC_TSET_SHIFT;
 
 	if (busw)
 		writel(value | FSMC_DEVWID_16, FSMC_NAND_REG(regs, bank, PC));

@@ -876,6 +862,14 @@ static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
 					       struct device_node *np)
 {
 	struct fsmc_nand_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct fsmc_nand_timings default_timings = {
+		.tclr	= FSMC_TCLR_1,
+		.tar	= FSMC_TAR_1,
+		.thiz	= FSMC_THIZ_1,
+		.thold	= FSMC_THOLD_4,
+		.twait	= FSMC_TWAIT_6,
+		.tset	= FSMC_TSET_0,
+	};
 	u32 val;
 
 	/* Set default NAND width to 8 bits */

drivers/mtd/nand/fsmc_nand.c +21 -1

+
+		of_property_read_u32(np, "nand-timings,tar", &val);
+		pdata->nand_timings.tar = (uint8_t)val;
+
+		of_property_read_u32(np, "nand-timings,thiz", &val);
+		pdata->nand_timings.thiz = (uint8_t)val;
+
+		of_property_read_u32(np, "nand-timings,thold", &val);
+		pdata->nand_timings.thold = (uint8_t)val;
+
+		of_property_read_u32(np, "nand-timings,twait", &val);
+		pdata->nand_timings.twait = (uint8_t)val;
+
+		of_property_read_u32(np, "nand-timings,tset", &val);
+		pdata->nand_timings.tset = (uint8_t)val;
+	} else {
+		memcpy(&pdata->nand_timings, &default_timings,
+				sizeof(default_timings));
+	}
+
 	return 0;
 }
 #else

@@ -1031,7 +1048,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 	host->partitions = pdata->partitions;
 	host->nr_partitions = pdata->nr_partitions;
 	host->dev = &pdev->dev;
-	host->dev_timings = pdata->nand_timings;
+	host->dev_timings = &pdata->nand_timings;
 	host->mode = pdata->mode;
 	host->max_banks = pdata->max_banks;
 

include/linux/mtd/fsmc.h +1 -1

index f0ab734..4fbdce4 100644
@@ -148,7 +148,7 @@ enum access_mode {
  * this may be set to NULL
  */
 struct fsmc_nand_platform_data {
-	struct fsmc_nand_timings *nand_timings;
+	struct fsmc_nand_timings nand_timings;
 	struct mtd_partition	*partitions;
 	unsigned int		nr_partitions;
 	unsigned int		options;

Signature · 1 line

1.7.11.4

Quoted 104 lines · Vipin Kumar

Add support to accept nand timing parameters via device tree

Signed-off-by Vipin Kumar<[email protected]>

---
  .../devicetree/bindings/mtd/fsmc-nand.txt          | 20 ++++++++
  drivers/mtd/nand/fsmc_nand.c                       | 59 ++++++++++++++--------
  include/linux/mtd/fsmc.h                           |  2 +-
  3 files changed, 59 insertions(+), 22 deletions(-)

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +20 -0

index 29d1a2f..6a7fc43 100644
@@ -13,6 +13,18 @@ Optional properties:
    defaults to 1 byte
  - nand-skip-bbtscan: Indicates the the BBT scanning should be skipped
 
+- nand-timings-enabled: Indicates if the timing parameters are passed
+  via DT
+- nand-timings,tclr:
+- nand-timings,tar:
+- nand-timings,thiz:
+- nand-timings,thold:
+- nand-timings,twait:
+- nand-timings,tset: All these timing parameters come from the actual
+  nand device specification. Each of this represents a number of time
+  period of hclk ie a number 4 in thold with hclk = 166MHz means that
+  thold = (1000 / 166) * 4 ns = 24.09ns
+
  Example:
 
  	fsmc: flash at d1800000 {

@@ -26,6 +38,14 @@ Example:
  		st,cle-off =<0x10000>;
  		maxbanks =<1>;
 
+		nand-timings-enabled;
+		nand-timings,tclr =<1>;
+		nand-timings,tar =<1>;
+		nand-timings,thiz =<1>;
+		nand-timings,thold =<4>;
+		nand-timings,twait =<6>;
+		nand-timings,tset =<0>;
+
  		bank-width =<1>;
  		nand-skip-bbtscan;
 

drivers/mtd/nand/fsmc_nand.c +14 -20

index fc6a044..f3d69b3 100644
@@ -415,27 +415,13 @@ static void fsmc_nand_setup(void __iomem *regs, uint32_t bank,
  {
  	uint32_t value = FSMC_DEVTYPE_NAND | FSMC_ENABLE | FSMC_WAITON;
  	uint32_t tclr, tar, thiz, thold, twait, tset;
-	struct fsmc_nand_timings *tims;
-	struct fsmc_nand_timings default_timings = {
-		.tclr	= FSMC_TCLR_1,
-		.tar	= FSMC_TAR_1,
-		.thiz	= FSMC_THIZ_1,
-		.thold	= FSMC_THOLD_4,
-		.twait	= FSMC_TWAIT_6,
-		.tset	= FSMC_TSET_0,
-	};
-
-	if (timings)
-		tims = timings;
-	else
-		tims =&default_timings;
 
-	tclr = (tims->tclr&  FSMC_TCLR_MASK)<<  FSMC_TCLR_SHIFT;
-	tar = (tims->tar&  FSMC_TAR_MASK)<<  FSMC_TAR_SHIFT;
-	thiz = (tims->thiz&  FSMC_THIZ_MASK)<<  FSMC_THIZ_SHIFT;
-	thold = (tims->thold&  FSMC_THOLD_MASK)<<  FSMC_THOLD_SHIFT;
-	twait = (tims->twait&  FSMC_TWAIT_MASK)<<  FSMC_TWAIT_SHIFT;
-	tset = (tims->tset&  FSMC_TSET_MASK)<<  FSMC_TSET_SHIFT;
+	tclr = (timings->tclr&  FSMC_TCLR_MASK)<<  FSMC_TCLR_SHIFT;
+	tar = (timings->tar&  FSMC_TAR_MASK)<<  FSMC_TAR_SHIFT;
+	thiz = (timings->thiz&  FSMC_THIZ_MASK)<<  FSMC_THIZ_SHIFT;
+	thold = (timings->thold&  FSMC_THOLD_MASK)<<  FSMC_THOLD_SHIFT;
+	twait = (timings->twait&  FSMC_TWAIT_MASK)<<  FSMC_TWAIT_SHIFT;
+	tset = (timings->tset&  FSMC_TSET_MASK)<<  FSMC_TSET_SHIFT;
 
  	if (busw)
  		writel(value | FSMC_DEVWID_16, FSMC_NAND_REG(regs, bank, PC));

@@ -876,6 +862,14 @@ static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
  					       struct device_node *np)
  {
  	struct fsmc_nand_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct fsmc_nand_timings default_timings = {
+		.tclr	= FSMC_TCLR_1,
+		.tar	= FSMC_TAR_1,
+		.thiz	= FSMC_THIZ_1,
+		.thold	= FSMC_THOLD_4,
+		.twait	= FSMC_TWAIT_6,
+		.tset	= FSMC_TSET_0,
+	};
  	u32 val;
 
  	/* Set default NAND width to 8 bits */

@@ -894,6 +888,29 @@ static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
  		pdata->options = NAND_SKIP_BBTSCAN;
  	of_property_read_u32(np, "maxbanks",&pdata->max_banks);
 

drivers/mtd/nand/fsmc_nand.c static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev, · line 893

Quoted 3 lines

drivers/mtd/nand/fsmc_nand.c +3 -0

+	if (of_property_read_bool(np, "nand-timings-enabled")) {
+		of_property_read_u32(np, "nand-timings,tclr",&val);
+		pdata->nand_timings.tclr = (uint8_t)val;

use a mask will be better

Quoted 46 lines

drivers/mtd/nand/fsmc_nand.c +21 -1

+
+		of_property_read_u32(np, "nand-timings,tar",&val);
+		pdata->nand_timings.tar = (uint8_t)val;
+
+		of_property_read_u32(np, "nand-timings,thiz",&val);
+		pdata->nand_timings.thiz = (uint8_t)val;
+
+		of_property_read_u32(np, "nand-timings,thold",&val);
+		pdata->nand_timings.thold = (uint8_t)val;
+
+		of_property_read_u32(np, "nand-timings,twait",&val);
+		pdata->nand_timings.twait = (uint8_t)val;
+
+		of_property_read_u32(np, "nand-timings,tset",&val);
+		pdata->nand_timings.tset = (uint8_t)val;
+	} else {
+		memcpy(&pdata->nand_timings,&default_timings,
+				sizeof(default_timings));
+	}
+
  	return 0;
  }
  #else

@@ -1031,7 +1048,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
  	host->partitions = pdata->partitions;
  	host->nr_partitions = pdata->nr_partitions;
  	host->dev =&pdev->dev;
-	host->dev_timings = pdata->nand_timings;
+	host->dev_timings =&pdata->nand_timings;
  	host->mode = pdata->mode;
  	host->max_banks = pdata->max_banks;
 

include/linux/mtd/fsmc.h +1 -2

index f0ab734..4fbdce4 100644
@@ -148,7 +148,7 @@ enum access_mode {
   * this may be set to NULL
   */
  struct fsmc_nand_platform_data {
-	struct fsmc_nand_timings *nand_timings;
+	struct fsmc_nand_timings nand_timings;
  	struct mtd_partition	*partitions;
  	unsigned int		nr_partitions;
  	unsigned int		options;
--

Quoted 2 lines

1.7.11.4

.

The NAND devices have a ready/busy (active low) output which is connected to the FSMC controller FSMC_RnB pin

The CPU can work in following two ways
* CPU keeps waiting for a ready response and it halts until it receives a ready response from NAND device. This halts the ldr instruction in between and in effect holds the CPU down for a number of cycles. During this period, CPU can't even service interrupts. This behavior is enabled via WAIT_ON bit of FSMC controller
* CPU merely issues the command and does not wait for response. The NAND device is later probed for being ready by reading the NAND_RnB signal

Until now, WAIT_ON was enabled by default. This patch implements reading the device ready state through a gpio. The gpio number is passed via DT and this patch also adds a corresponding binding for fsmc

Signed-off-by Vipin Kumar <[email protected]>

---
 .../devicetree/bindings/mtd/fsmc-nand.txt          |  5 ++
 drivers/mtd/nand/fsmc_nand.c                       | 54 +++++++++++++++++++---
 include/linux/mtd/fsmc.h                           | 11 +++++
 3 files changed, 64 insertions(+), 6 deletions(-)

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +5 -0

index 6a7fc43..598bca2 100644
@@ -25,6 +25,11 @@ Optional properties:
   period of hclk ie a number 4 in thold with hclk = 166MHz means that
   thold = (1000 / 166) * 4 ns = 24.09ns
 
+- st,ready-busy: Should contain either of "rb-gpio" or "rb-wait". When
+  this property is not defined, the default is to use rb-wait.
+- st,rb-gpios: When the st,ready-busy is defined as "rb-gpio", a gpio
+  pin number is defined in this property
+
 Example:
 
 	fsmc: flash at d1800000 {

drivers/mtd/nand/fsmc_nand.c +48 -6

index f3d69b3..eaffd3b 100644
@@ -22,6 +22,7 @@
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
+#include <linux/gpio.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/resource.h>

@@ -32,6 +33,7 @@
 #include <linux/mtd/nand_ecc.h>
 #include <linux/platform_device.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/mtd/partitions.h>
 #include <linux/io.h>
 #include <linux/slab.h>

@@ -289,6 +291,7 @@ static struct fsmc_eccplace fsmc_ecc4_sp_place = {
  * @write_dma_chan:	DMA channel for write access to NAND
  * @dma_access_complete: Completion structure
  *
+ * @rb_pin:		Ready/Busy data
  * @dev_timings:	Timings to be programmed in controller
  * @partitions:		Partition info for a NAND Flash
  * @nr_partitions:	Total number of partition of a NAND flash

@@ -322,6 +325,7 @@ struct fsmc_nand_data {
 	struct completion	dma_access_complete;
 
 	/* Recieved from plat data */
+	struct fsmc_rbpin	*rbpin;
 	struct fsmc_nand_timings *dev_timings;
 	struct mtd_partition	*partitions;
 	unsigned int		nr_partitions;

@@ -410,12 +414,16 @@ static void fsmc_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
  * This routine initializes timing parameters related to NAND memory access in
  * FSMC registers
  */
-static void fsmc_nand_setup(void __iomem *regs, uint32_t bank,
-			   uint32_t busw, struct fsmc_nand_timings *timings)
+static void fsmc_nand_setup(void __iomem *regs, uint32_t bank, uint32_t busw,
+			   struct fsmc_nand_timings *timings,
+			   struct fsmc_rbpin *rbpin)
 {
-	uint32_t value = FSMC_DEVTYPE_NAND | FSMC_ENABLE | FSMC_WAITON;
+	uint32_t value = FSMC_DEVTYPE_NAND | FSMC_ENABLE;
 	uint32_t tclr, tar, thiz, thold, twait, tset;
 
+	if (!rbpin || (rbpin->use_pin == FSMC_RB_WAIT))
+		value |= FSMC_WAITON;
+
 	tclr = (timings->tclr & FSMC_TCLR_MASK) << FSMC_TCLR_SHIFT;
 	tar = (timings->tar & FSMC_TAR_MASK) << FSMC_TAR_SHIFT;
 	thiz = (timings->thiz & FSMC_THIZ_MASK) << FSMC_THIZ_SHIFT;

@@ -857,6 +865,14 @@ static bool filter(struct dma_chan *chan, void *slave)
 	return true;
 }
 
+static int fsmc_dev_ready(struct mtd_info *mtd)
+{
+	struct fsmc_nand_data *host = container_of(mtd,
+					struct fsmc_nand_data, mtd);
+
+	return !!gpio_get_value(host->rbpin->gpio_pin);
+}
+
 #ifdef CONFIG_OF
 static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
 					       struct device_node *np)

@@ -870,7 +886,9 @@ static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
 		.twait	= FSMC_TWAIT_6,
 		.tset	= FSMC_TSET_0,
 	};
+	const char *rb;
 	u32 val;
+	enum of_gpio_flags flags;
 
 	/* Set default NAND width to 8 bits */
 	pdata->width = 8;

@@ -911,6 +929,14 @@ static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
 				sizeof(default_timings));
 	}
 
+	if (!of_property_read_string(np, "st,ready-busy", &rb) &&
+			!strcmp(rb, "rb-gpio")) {
+		pdata->rbpin.use_pin = FSMC_RB_GPIO;
+		pdata->rbpin.gpio_pin =
+			of_get_named_gpio_flags(np, "st,rb-gpios", 0, &flags);
+	} else
+		pdata->rbpin.use_pin = FSMC_RB_WAIT;
+
 	return 0;
 }
 #else

@@ -1049,6 +1075,8 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 	host->nr_partitions = pdata->nr_partitions;
 	host->dev = &pdev->dev;
 	host->dev_timings = &pdata->nand_timings;
+	host->rbpin = &pdata->rbpin;
+
 	host->mode = pdata->mode;
 	host->max_banks = pdata->max_banks;
 

@@ -1074,6 +1102,18 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 	nand->select_chip = fsmc_select_chip;
 	nand->badblockbits = 7;
 
+	if (host->rbpin->use_pin != FSMC_RB_WAIT)
+		nand->dev_ready = fsmc_dev_ready;
+
+	if (host->rbpin->use_pin == FSMC_RB_GPIO) {
+		ret = gpio_request(host->rbpin->gpio_pin, "fsmc-rb");
+		if (ret < 0) {
+			dev_err(&pdev->dev, "gpio request fail: %d\n",
+					host->rbpin->gpio_pin);
+			goto err_gpio_req;
+		}
+	}
+
 	if (pdata->width == FSMC_NAND_BW16)
 		nand->options |= NAND_BUSWIDTH_16;
 

@@ -1107,7 +1147,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 	for (bank = 0; bank < host->max_banks; bank++)
 		fsmc_nand_setup(host->regs_va, bank,
 				nand->options & NAND_BUSWIDTH_16,
-				host->dev_timings);
+				host->dev_timings, host->rbpin);
 
 	if (AMBA_REV_BITS(host->pid) >= 8) {
 		nand->ecc.read_page = fsmc_read_page_hwecc;

@@ -1211,6 +1251,9 @@ err_req_write_chnl:
 	if (host->mode == USE_DMA_ACCESS)
 		dma_release_channel(host->read_dma_chan);
 err_req_read_chnl:
+	if (host->rbpin->use_pin == FSMC_RB_GPIO)
+		gpio_free(host->rbpin->gpio_pin);
+err_gpio_req:
 	clk_disable_unprepare(host->clk);
 err_clk_prepare_enable:
 	clk_put(host->clk);

@@ -1260,8 +1303,7 @@ static int fsmc_nand_resume(struct device *dev)
 		for (bank = 0; bank < host->max_banks; bank++)
 			fsmc_nand_setup(host->regs_va, bank,
 					host->nand.options & NAND_BUSWIDTH_16,
-					host->dev_timings);
-
+					host->dev_timings, host->rbpin);
 	}
 	return 0;
 }

include/linux/mtd/fsmc.h +10 -0

index 4fbdce4..eed22a1 100644
@@ -130,6 +130,16 @@ struct fsmc_nand_timings {
 	uint8_t tset;
 };
 
+enum rbpin {
+	FSMC_RB_WAIT = 0,
+	FSMC_RB_GPIO,
+};
+
+struct fsmc_rbpin {
+	enum rbpin	use_pin;
+	uint32_t	gpio_pin;
+};
+
 enum access_mode {
 	USE_DMA_ACCESS = 1,
 	USE_WORD_ACCESS,

Interruptible wait caused trouble in fsmc hardware state machine if the application was killed abruptly. To make fsmc operation safe turn wait in to un-interruptible.

Signed-off-by Vipin Kumar <[email protected]>

Interruptible wait caused trouble in fsmc hardware state machine if the application was killed abruptly. To make fsmc operation safe turn wait in to un-interruptible.

Signed-off-by Vipin Kumar <[email protected]>

read_buf/write_buf callbacks should be able to accept a user space memory address (virtually contiguous memory) as buffer pointer.

This patch allocates a logically contiguous memory area which is use for dma

xfers during read and write accesses.

Signed-off-by Vipin Kumar <[email protected]>

The real problem is likely the DMA driver. The stuf that get fed into dma.device_prep_dma_memcpy() needs to be converted to a scatterlist and then set up in the LLI list for the controller.

IIRC SPEAr is using drivers/dma/dw_dmac.c so

check this driver's dwc_prep_dma_memcpy().
It does seem like it is checking whether src or
dest is scattered in this for() loop:

for (offset = 0; offset < len; offset += xfer_count << src_width) {}

dma_sync_single_for_device() is translating the virtual address to physical for every chunk BTW.

So instead of doing this copying, debug the problem, and see if there is a bug in that for()-loop or similar, if it needs to be rewritten or so.

Quoted 6 lines · Linus Walleij

The real problem is likely the DMA driver. The stuf that get fed into dma.device_prep_dma_memcpy() needs to be converted to a scatterlist and then set up in the LLI list for the controller.

IIRC SPEAr is using drivers/dma/dw_dmac.c so

Both dw_dmac and pl080 for different SPEAr SoC's.

Quoted 9 lines

check this driver's dwc_prep_dma_memcpy().
It does seem like it is checking whether src or
dest is scattered in this for() loop:

for (offset = 0; offset< len; offset += xfer_count<< src_width) {}

dma_sync_single_for_device() is translating the virtual address to physical for every chunk BTW.

Quoted 3 lines

So instead of doing this copying, debug the problem, and see if there is a bug in that for()-loop or similar, if it needs to be rewritten or so.

But what's the problem you saw vipin, for which you generated this patch? .

Quoted 4 lines · Vipin Kumar

read_buf/write_buf callbacks should be able to accept a user space memory address (virtually contiguous memory) as buffer pointer.

This patch allocates a logically contiguous memory area which is use for dma

You mean PHYSICALLY contigous, don't you?

Quoted 3 lines

xfers during read and write accesses.

Signed-off-by Vipin Kumar<[email protected]>

If you really want a physically contigous buffer you need to use CMA, but I don't think that is the real problem here...

We're already using userspace buffers in e.g. the MMCI driver (drivers/mmc/host/mmci.c).

The real problem is likely the DMA driver. The stuf that get fed into dma.device_prep_dma_memcpy() needs to be converted to a scatterlist and then set up in the LLI list for the controller.

IIRC SPEAr is using drivers/dma/dw_dmac.c so
check this driver's dwc_prep_dma_memcpy().
It does seem like it is checking whether src or
dest is scattered in this for() loop:

for (offset = 0; offset< len; offset += xfer_count<< src_width) {}

dma_sync_single_for_device() is translating the virtual address to physical for every chunk BTW.

So instead of doing this copying, debug the problem, and see if there is a bug in that for()-loop or similar, if it needs to be rewritten or so.

Yours,
Linus Walleij
.

On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar <[email protected]> wrote:

The real problem is likely the DMA driver. The stuf that get fed into dma.device_prep_dma_memcpy() needs to be converted to a scatterlist and then set up in the LLI list for the controller.

IIRC SPEAr is using drivers/dma/dw_dmac.c so
check this driver's dwc_prep_dma_memcpy().
It does seem like it is checking whether src or
dest is scattered in this for() loop:

for (offset = 0; offset < len; offset += xfer_count << src_width) {}

dma_sync_single_for_device() is translating the virtual address to physical for every chunk BTW.

Quoted 2 lines · Linus Walleij

dma_sync_single_for_device() is translating the virtual address to physical for every chunk BTW.

I pray that i am wrong here, otherwise i would be thrown out from the maintainers list for this driver :)

dma_sync_single_for_device() is not doing anything on the buffer, but on the LLI item. Actually it is flushing LLI struct so that DMA h/w can get the correct values.

Quoted 1 line · viresh kumar

I pray that i am wrong here, otherwise i would be thrown out from

Quoted 5 lines

the maintainers list for this driver :)

dma_sync_single_for_device() is not doing anything on the buffer, but on the LLI item. Actually it is flushing LLI struct so that DMA h/w can get the correct values.

Sorry no, I'm the one who's wrong...

And the block layer of the subsystem should take care of only handing the driver buffers that are contiguous

Quoted 3 lines · Linus Walleij

And the block layer of the subsystem should take care of only handing the driver buffers that are contiguous

But why should that be a constraint on block layer? It is working in virtual space and is concerned about that onlly.

Quoted 6 lines · viresh kumar

Quoted 2 lines · Linus Walleij

dma_sync_single_for_device() is translating the virtual address to physical for every chunk BTW.

Quoted 6 lines

I pray that i am wrong here, otherwise i would be thrown out from the maintainers list for this driver :)

dma_sync_single_for_device() is not doing anything on the buffer, but on the LLI item. Actually it is flushing LLI struct so that DMA h/w can get the correct values.

Sorry no, I'm the one who's wrong...

So the DMA engine memcpy() is not mapping virt->phys but expects physical addresses to be provided.

So dma_map_single() needs to be called on the stuff passed in to dev->device_prep_dma_memcpy().

And currently there is indeed a dma_map_single() in dma_xfer() in fsmc_nand.c which should work just fine.

dma_map_single() will only work if the buffer is physically contiguous.

And the block layer of the subsystem should take care of only handing the driver buffers that are contiguous I think? Not that I'm an expert here ... more some guesswork :-/

The buffers provided to the driver are actually user buffers. The reason I say that is because the generic nand test modules eg drivers/mtd/nand/mtd_stresstest.c calls mtd->_read with a user buffer as an argument

This same buffer directly trickles down to the driver

Artem, should we clearly cast this buffer as a user pointer instead of just a 'uint8_t *'.

First thing which comes to mind is that in modern kernels memory fragmentation is not that big issue as it used to be. So may be kmalloc() the memory is not that bad nowadays? We have page migration, memory compaction, etc?

I'd really prefer to just switch to kmalloc() everywhere instead of adding hacks like this to the drivers. Then if this is a problem for someone, he can fix it by either switching to smaller buffers (possible in many places), or by improving memory fragmentation issues on his system, or by just using CMA.

We can even have an mtd-wide funcion which will try kmalloc(), and if that fails, fall-back to CMA.

I would prefer not building a solution that hopes kmalloc() can get a large contiguous buffer (remember, eraseblock sizes come as large as 2MB these days). A real solution like CMA or scatter-gather seems like a better idea.

Quoted 4 lines · Brian Norris

I would prefer not building a solution that hopes kmalloc() can get a large contiguous buffer (remember, eraseblock sizes come as large as 2MB these days). A real solution like CMA or scatter-gather seems like a better idea.

+1 on that, and as I think I illustrated the MMC subsystem is using the block layer helpers to form scatter-gather lists for it's requests. I don't see why the MTD subsystem need to be very different?

Quoted 11 lines · Linus Walleij

Quoted 4 lines · Brian Norris

I would prefer not building a solution that hopes kmalloc() can get a large contiguous buffer (remember, eraseblock sizes come as large as 2MB these days). A real solution like CMA or scatter-gather seems like a better idea.

+1 on that, and as I think I illustrated the MMC subsystem is using the block layer helpers to form scatter-gather lists for it's requests. I don't see why the MTD subsystem need to be very different?

This is good idea, the problem is that it is a bit more work than hacking the driver, so most people prefer the latter. By removing all kmalloc()'s I basically wish to force people to go this way.

What are the SC functions names you refer?

Quoted 9 lines · Artem Bityutskiy

Quoted 8 lines · Linus Walleij

Quoted 4 lines · Brian Norris

I would prefer not building a solution that hopes kmalloc() can get a large contiguous buffer (remember, eraseblock sizes come as large as 2MB these days). A real solution like CMA or scatter-gather seems like a better idea.

Quoted 7 lines

Quoted 3 lines

+1 on that, and as I think I illustrated the MMC subsystem is using the block layer helpers to form scatter-gather lists for it's requests. I don't see why the MTD subsystem need to be very different?

This is good idea, the problem is that it is a bit more work than hacking the driver, so most people prefer the latter. By removing all kmalloc()'s I basically wish to force people to go this way.

Quoted 1 line

What are the SC functions names you refer?

I'm mainly thinking of blk_rq_map_sg()
from block/blk-merge.c

Quoted 3 lines

Quoted 3 lines

This is good idea, the problem is that it is a bit more work than hacking the driver, so most people prefer the latter. By removing all kmalloc()'s I basically wish to force people to go this way.

Did you mean vmalloc()'s?

Quoted 5 lines

Quoted 2 lines

What are the SC functions names you refer?

I'm mainly thinking of blk_rq_map_sg()
from block/blk-merge.c

Quoted 2 lines

Quoted 2 lines

I'm mainly thinking of blk_rq_map_sg()
from block/blk-merge.c

Yeah, may be re-working or extending MTD api with SG lists would be the way to go, but this is a lot more work...

Quoted 5 lines

I'd really prefer to just switch to kmalloc() everywhere instead of adding hacks like this to the drivers. Then if this is a problem for someone, he can fix it by either switching to smaller buffers (possible in many places), or by improving memory fragmentation issues on his system, or by just using CMA.

I think I can suggest that this is already a problem on real systems. In bringing up a board on v3.3 kernel, I experienced a kernel memory allocation error when trying to memdup_user() in eraseblock-size regions. I believe I would experience more of these if all eraseblock-sized buffers were kmalloc()'d.

See the commit description for the following commit in mtd-utils.git:

commit 71c76e74661492b4f68f670514866cfc85f47089
libmtd: fix mtd_write() issues for large data-only writes

I would prefer not building a solution that hopes kmalloc() can get a large contiguous buffer (remember, eraseblock sizes come as large as 2MB these days). A real solution like CMA or scatter-gather seems like a better idea.

Quoted 4 lines · Brian Norris

See the commit description for the following commit in mtd-utils.git:

commit 71c76e74661492b4f68f670514866cfc85f47089
libmtd: fix mtd_write() issues for large data-only writes

Well, this is another work-around.

The better way to fix this would be
to change kernel's 'mtdchar_write_ioctl()' to be iterative and avoid calling 'memdup_user(len)' for arbitrary 'len' passed from user-space.

So I thought about something like introducing an mtd_alloc() which would:
a. Try doing CMA allocation: dma_alloc_from_contiguous() b. If it fails, use kmalloc().

If both fail - bad luck. Go look at the code and switch to S-G lists instead of allocating contiguous regions. As I said, in many places it is easy to do, and there are few places when you would need to spend a bit more time.

How does this sound to you?

Use relaxed variants of readl/writel accessors. readl/writel io accessors use explicit dsb instruction which causes stalls in the processor core resulting several cycles of delay for each access

Use relaxed variants where ever possible. This also results in an improved read/write performance.

Use relaxed variants of readl/writel accessors. readl/writel io accessors use explicit dsb instruction which causes stalls in the processor core resulting several cycles of delay for each access

Use relaxed variants where ever possible. This also results in an improved read/write performance.

Signed-off-by Vipin Kumar <[email protected]>

change_bit routine accepts only ulong pointers as buffer, so an unaligned char pointer passed to change_bit may lead to a crash.

Fix this bug by accessing the buffer as char pointer.

Quoted 3 lines · Vipin Kumar

change_bit routine accepts only ulong pointers as buffer, so an unaligned char pointer passed to change_bit may lead to a crash.

Quoted 1 line

Fix this bug by accessing the buffer as char pointer.

Why not see if we can fix change_bit() instead? Since I suspect this is on ARM I bet Russell and Nico want to hear about this if there is a problem.

Can the ARM change_bit() not be fixed, so that long arguments are the only option?

drivers/mtd/nand/fsmc_nand.c static int fsmc_bch8_correct_data(struct mtd_info *mtd, uint8_t *dat, · line 863

Quoted 4 lines

                if (err_idx[i] < chip->ecc.size * 8) {

drivers/mtd/nand/fsmc_nand.c +2 -1

-                       change_bit(err_idx[i], (unsigned long *)dat);
+                       uint8_t *p = dat + err_idx[i] / 8;
+                       *p = *p ^ (1 << (err_idx[i] % 8));

I'm one of these people who would write >>3 and &7 rather than /8 or %8 but I guess we are all different. Atleast consider it if you stick with this...

Quoted 5 lines · Linus Walleij

Quoted 3 lines · Vipin Kumar

change_bit routine accepts only ulong pointers as buffer, so an unaligned char pointer passed to change_bit may lead to a crash.

Quoted 8 lines

Quoted 1 line

Fix this bug by accessing the buffer as char pointer.

Why not see if we can fix change_bit() instead? Since I suspect this is on ARM I bet Russell and Nico want to hear about this if there is a problem.

Can the ARM change_bit() not be fixed, so that long arguments are the only option?

No. It is this code which is totally broken.

The change_bit() is defined to operate on long values, in the _native_ endian. Now imagine what this is going to do to your data buffer if instead you are running on a big endian device.

And I doubt there is anything requiring atomic bit manipulation here either.

Quoted 8 lines

drivers/mtd/nand/fsmc_nand.c static int fsmc_bch8_correct_data(struct mtd_info *mtd, uint8_t *dat,

Quoted 4 lines

                 if (err_idx[i]<  chip->ecc.size * 8) {

drivers/mtd/nand/fsmc_nand.c +2 -1

-                       change_bit(err_idx[i], (unsigned long *)dat);
+                       uint8_t *p = dat + err_idx[i] / 8;
+                       *p = *p ^ (1<<  (err_idx[i] % 8));

I'm one of these people who would write>>3 and &7 rather than /8 or %8 but I guess we are all different. Atleast consider it if you stick with this...

Better yet:

		dat[err_idx[i] / 8] ^= (1<<  (err_idx[i] % 8));

The /8 and %8 will be changed into>>3 and&7 by the compiler anyway, while the /8 and %8 form is possibly a bit less obscure.

Of course, this needs to be done over _all_ this driver, not only a few cases.

Nicolas
.

Quoted 4 lines · Vipin Kumar

change_bit routine accepts only ulong pointers as buffer, so an unaligned char pointer passed to change_bit may lead to a crash.

Fix this bug by accessing the buffer as char pointer.

Why not see if we can fix change_bit() instead? Since I suspect this is on ARM I bet Russell and Nico want to hear about this if there is a problem.

Can the ARM change_bit() not be fixed, so that long arguments are the only option?

Quoted 1 line

- change_bit(err_idx[i], (unsigned long *)dat);

Quoted 10 lines · Linus Walleij

Quoted 4 lines · Vipin Kumar

change_bit routine accepts only ulong pointers as buffer, so an unaligned char pointer passed to change_bit may lead to a crash.

Fix this bug by accessing the buffer as char pointer.

Why not see if we can fix change_bit() instead? Since I suspect this is on ARM I bet Russell and Nico want to hear about this if there is a problem.

Not particularly. There's a reason the argument to change_bit() is typed 'unsigned long' and that's not because it can take a void, char, or a short. It's because it _expects_ the buffer to be aligned to an "unsigned long" quantity.

That's because on many architectures, misaligned loads and stores are not atomic operations - and in this case, load/store exclusive will fail when they're misalighed.

So...

- change_bit(err_idx[i], (unsigned long *)dat);

is highly invalid code.

Quoted 2 lines

Can the ARM change_bit() not be fixed, so that long arguments are the only option?

Spot the intentional cast:

Quoted 1 line

Quoted 1 line

- change_bit(err_idx[i], (unsigned long *)dat);

which tries to work around this. Remember my attitude towards casts:
if you're having to use a cast, you are _probably_ doing something wrong. In this case, it's hiding a warning which was saying that the code is doing something wrong, and then the result blows up. By adding that cast, the wrong wire was cut... you get to keep the remains. ;)
.

Signed-off-by Vipin Kumar <[email protected]>

---
 .../devicetree/bindings/mtd/fsmc-nand.txt          |   2 +
 drivers/mtd/nand/fsmc_nand.c                       | 156 +++++++++++++--------
 include/linux/mtd/fsmc.h                           |   3 +
 3 files changed, 106 insertions(+), 55 deletions(-)

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +0 -0

index 598bca2..dcf513b 100644

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +0 -0

 Example:
 
 	fsmc: flash at d1800000 {

drivers/mtd/nand/fsmc_nand.c +0 -0

index 762cf83..ff84468 100644

drivers/mtd/nand/fsmc_nand.c +98 -55

 	return 0;
 }
 #else

@@ -972,6 +975,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 	dma_cap_mask_t mask;
 	int ret = 0;
 	u32 pid, bank;
+	uint oobeccsize, m;
 	int i;
 
 	if (np) {

@@ -1104,9 +1108,24 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 	nand->cmd_ctrl = fsmc_cmd_ctrl;
 	nand->chip_delay = 30;
 
-	nand->ecc.mode = NAND_ECC_HW;
-	nand->ecc.hwctl = fsmc_enable_hwecc;
-	nand->ecc.size = 512;
+	if (pdata->sw_ecc) {
+		nand->ecc.mode = NAND_ECC_SOFT_BCH;
+		/*
+		 * The recent devices require n-bit correctibility in x bytes.
+		 * The values of n and x varies as below
+		 * n - 1 to 100
+		 * x - 512 to 1K
+		 * TODO: For now, take x = 1K for all sw bch mathematics. Think
+		 * of a better way to handle other device dependent
+		 * requirements. May be it should come from board dts files
+		 */
+		nand->ecc.size = 1024;
+	} else {
+		nand->ecc.mode = NAND_ECC_HW;
+		nand->ecc.hwctl = fsmc_enable_hwecc;
+		nand->ecc.size = 512;
+	}
+
 	nand->options = pdata->options;
 	nand->select_chip = fsmc_select_chip;
 	nand->badblockbits = 7;

@@ -1165,17 +1184,19 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 				nand->options & NAND_BUSWIDTH_16,
 				host->dev_timings, host->rbpin);
 
-	if (AMBA_REV_BITS(host->pid) >= 8) {
-		nand->ecc.read_page = fsmc_read_page_hwecc;
-		nand->ecc.calculate = fsmc_read_hwecc_ecc4;
-		nand->ecc.correct = fsmc_bch8_correct_data;
-		nand->ecc.bytes = 13;
-		nand->ecc.strength = 8;
-	} else {
-		nand->ecc.calculate = fsmc_read_hwecc_ecc1;
-		nand->ecc.correct = nand_correct_data;
-		nand->ecc.bytes = 3;
-		nand->ecc.strength = 1;
+	if (nand->ecc.mode != NAND_ECC_SOFT_BCH) {
+		if (AMBA_REV_BITS(host->pid) >= 8) {
+			nand->ecc.read_page = fsmc_read_page_hwecc;
+			nand->ecc.calculate = fsmc_read_hwecc_ecc4;
+			nand->ecc.correct = fsmc_bch8_correct_data;
+			nand->ecc.bytes = 13;
+			nand->ecc.strength = 8;
+		} else {
+			nand->ecc.calculate = fsmc_read_hwecc_ecc1;
+			nand->ecc.correct = nand_correct_data;
+			nand->ecc.bytes = 3;
+			nand->ecc.strength = 1;
+		}
 	}
 
 	/*

@@ -1187,48 +1208,73 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 		goto err_scan_ident;
 	}
 
-	if (AMBA_REV_BITS(host->pid) >= 8) {
-		switch (host->mtd.oobsize) {
-		case 16:
-			nand->ecc.layout = &fsmc_ecc4_16_layout;
-			host->ecc_place = &fsmc_ecc4_sp_place;
-			break;
-		case 64:
-			nand->ecc.layout = &fsmc_ecc4_64_layout;
-			host->ecc_place = &fsmc_ecc4_lp_place;
-			break;
-		case 128:
-			nand->ecc.layout = &fsmc_ecc4_128_layout;
-			host->ecc_place = &fsmc_ecc4_lp_place;
-			break;
-		case 224:
-			nand->ecc.layout = &fsmc_ecc4_224_layout;
-			host->ecc_place = &fsmc_ecc4_lp_place;
-			break;
-		case 256:
-			nand->ecc.layout = &fsmc_ecc4_256_layout;
-			host->ecc_place = &fsmc_ecc4_lp_place;
-			break;
-		default:
-			printk(KERN_WARNING "No oob scheme defined for "
-			       "oobsize %d\n", mtd->oobsize);
-			BUG();
-		}
+	if (nand->ecc.mode == NAND_ECC_SOFT_BCH) {
+		/*
+		 * Initialize the ecc bytes and strength dynamically based on eccsize
+		 * and writesize.
+		 *
+		 * Parameters @eccsize and @eccbytes are used to compute BCH parameters
+		 * m (Galois field order) and t (error correction capability). @eccbytes
+		 * should be equal to the number of bytes required to store m*t bits,
+		 * where m is such that 2^m-1 > @eccsize*8.
+		 *
+		 * Example: to configure 4 bit correction per 512 bytes, you should pass
+		 * @eccsize = 512  (thus, m=13 is the smallest integer such that 2^m-1 >
+		 * 512*8) @eccbytes = 7 (7 bytes are required to store m*t = 13*4 = 52
+		 * bits)
+		 *
+		 * Note: 2 bytes of oob are considered reserved for bad block marking
+		 */
+		m = fls(1 + 8 * nand->ecc.size);
+		oobeccsize = ((host->mtd.oobsize - 2) * \
+				nand->ecc.size) / host->mtd.writesize;
+		nand->ecc.bytes = (oobeccsize / m) * m;
+		nand->ecc.strength = (nand->ecc.bytes * 8) / m;
+		nand->ecc.layout = NULL;
 	} else {
-		switch (host->mtd.oobsize) {
-		case 16:
-			nand->ecc.layout = &fsmc_ecc1_16_layout;
-			break;
-		case 64:
-			nand->ecc.layout = &fsmc_ecc1_64_layout;
-			break;
-		case 128:
-			nand->ecc.layout = &fsmc_ecc1_128_layout;
-			break;
-		default:
-			printk(KERN_WARNING "No oob scheme defined for "
-			       "oobsize %d\n", mtd->oobsize);
-			BUG();
+		if (AMBA_REV_BITS(host->pid) >= 8) {
+			switch (host->mtd.oobsize) {
+			case 16:
+				nand->ecc.layout = &fsmc_ecc4_16_layout;
+				host->ecc_place = &fsmc_ecc4_sp_place;
+				break;
+			case 64:
+				nand->ecc.layout = &fsmc_ecc4_64_layout;
+				host->ecc_place = &fsmc_ecc4_lp_place;
+				break;
+			case 128:
+				nand->ecc.layout = &fsmc_ecc4_128_layout;
+				host->ecc_place = &fsmc_ecc4_lp_place;
+				break;
+			case 224:
+				nand->ecc.layout = &fsmc_ecc4_224_layout;
+				host->ecc_place = &fsmc_ecc4_lp_place;
+				break;
+			case 256:
+				nand->ecc.layout = &fsmc_ecc4_256_layout;
+				host->ecc_place = &fsmc_ecc4_lp_place;
+				break;
+			default:
+				printk(KERN_WARNING "No oob scheme defined for "
+						"oobsize %d\n", mtd->oobsize);
+				BUG();
+			}
+		} else {
+			switch (host->mtd.oobsize) {
+			case 16:
+				nand->ecc.layout = &fsmc_ecc1_16_layout;
+				break;
+			case 64:
+				nand->ecc.layout = &fsmc_ecc1_64_layout;
+				break;
+			case 128:
+				nand->ecc.layout = &fsmc_ecc1_128_layout;
+				break;
+			default:
+				printk(KERN_WARNING "No oob scheme defined for "
+						"oobsize %d\n", mtd->oobsize);
+				BUG();
+			}
 		}
 	}
 

include/linux/mtd/fsmc.h +3 -0

index eed22a1..07eee6e 100644
@@ -176,6 +176,9 @@ struct fsmc_nand_platform_data {
 	/* priv structures for dma accesses */
 	void			*read_dma_priv;
 	void			*write_dma_priv;
+
+	/* whether s/w ecc is supported */
+	bool			sw_ecc;
 };
 
 extern int __init fsmc_nor_init(struct platform_device *pdev,

Signature · 1 line

1.7.11.4

Quoted 15 lines · Vipin Kumar

Signed-off-by Vipin Kumar<[email protected]>

---
  .../devicetree/bindings/mtd/fsmc-nand.txt          |   2 +
  drivers/mtd/nand/fsmc_nand.c                       | 156 +++++++++++++--------
  include/linux/mtd/fsmc.h                           |   3 +
  3 files changed, 106 insertions(+), 55 deletions(-)

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +0 -0

index 598bca2..dcf513b 100644
@@ -30,6 +30,8 @@ Optional properties:
  - st,rb-gpios: When the st,ready-busy is defined as "rb-gpio", a gpio
    pin number is defined in this property
 

Documentation/devicetree/bindings/mtd/fsmc-nand.txt Optional properties: · line 34

Quoted 2 lines

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +2 -0

+- nand-sw-ecc: boolean indicating whether s/w ecc is supported
+

please use the generic binding
- nand-ecc-mode = xx

Quoted 11 lines

Documentation/devicetree/bindings/mtd/fsmc-nand.txt +0 -0

  Example:
 
  	fsmc: flash at d1800000 {

drivers/mtd/nand/fsmc_nand.c +0 -0

index 762cf83..ff84468 100644
@@ -946,6 +946,9 @@ static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
  	} else
  		pdata->rbpin.use_pin = FSMC_RB_WAIT;
 

drivers/mtd/nand/fsmc_nand.c static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev, · line 951

Quoted 3 lines

drivers/mtd/nand/fsmc_nand.c +3 -0

+	if (of_property_read_bool(np, "nand-sw-ecc"))
+		pdata->sw_ecc = true;
+

of_get_nand_ecc_mode

Few devices like H27UBG8T2CTR have a writesize/oobsize of 8KB/640B. This means that the maximum oobsize has gone up to 640 bytes and consequently the maximum ecc placement locations have also gone up to 640.

Signed-off-by Vipin Kumar <[email protected]>

Few devices like H27UBG8T2CTR have a writesize/oobsize of 8KB/640B. This means that the maximum oobsize has gone up to 640 bytes and consequently the maximum ecc placement locations have also gone up to 640.

This patch-set contains several fixes and a few enhancements in fsmc driver. Modifications include

Quoted 2 lines · Vipin Kumar

This patch-set contains several fixes and a few enhancements in fsmc driver. Modifications include

OK, picked some patches from your series, some do not apply. Please, send v2 against the l2-mtd.git tree:

git://git.infradead.org/users/dedekind/l2-mtd.git