[PATCH 00/11] mtd/nand: fsmc driver updates

[PATCH 00/11] mtd/nand: fsmc driver updates

[Vipin Kumar]

This patch-set contains several fixes and a few enhancements in fsmc driver.
Modifications include
- General enhancements
  * Add ready/busy support through a GPIO pin
  * Add software bch support
  * Increase ecc loactions to 640
  * Support multiple banks
  * Use relaxed io accessors for increased throughput
- Enhance DT support
  * Add timing parameters
- Several fixes
  * Fix the type for regs
  * Replace change_bit routine to handle unaligned addresses
  * Provide contiguous buffers for DMA operation

Vipin Kumar (11):
  fsmc/nand:FIX: Change the type for regs to void __iomem *
  fsmc/nand: Rearrange the fsmc_nand_data structure and update comments
  fsmc/nand: Support multiple banks connected to controller
  fsmc/nand: Accept nand timing parameters via DT
  fsmc: Implement ready/busy through gpio pin
  fsmc/nand: Modify the wait to uninterruptible
  fsmc/nand: Provide contiguous buffers to dma
  fsmc/nand: Use relaxed variants of io accessors
  fsmc/nand:FIX: replace change_bit routine
  fsmc/nand: Add sw bch support for ecc calculation/correction
  nand: Increase the ecc placement locations to 640

 .../devicetree/bindings/mtd/fsmc-nand.txt          |  29 ++
 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                       | 419 ++++++++++++++-------
 include/linux/mtd/fsmc.h                           |  18 +-
 include/linux/mtd/mtd.h                            |   2 +-
 9 files changed, 332 insertions(+), 141 deletions(-)

-- 
1.7.11.4

[PATCH 01/11] fsmc/nand:FIX: Change the type for regs to void __iomem *

[Vipin Kumar]

Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
---
 drivers/mtd/nand/fsmc_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index 38d2624..e96d7d1 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -361,7 +361,7 @@ static void fsmc_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 	struct nand_chip *this = mtd->priv;
 	struct fsmc_nand_data *host = container_of(mtd,
 					struct fsmc_nand_data, mtd);
-	void *__iomem *regs = host->regs_va;
+	void __iomem *regs = host->regs_va;
 	unsigned int bank = host->bank;
 
 	if (ctrl & NAND_CTRL_CHANGE) {
-- 
1.7.11.4

[PATCH 01/11] fsmc/nand:FIX: Change the type for regs to void __iomem *

[viresh kumar]

On Tue, Oct 9, 2012 at 4:14 PM, Vipin Kumar <vipin.kumar@st.com> wrote:
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
> ---
>  drivers/mtd/nand/fsmc_nand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
> index 38d2624..e96d7d1 100644
> --- a/drivers/mtd/nand/fsmc_nand.c
> +++ b/drivers/mtd/nand/fsmc_nand.c
> @@ -361,7 +361,7 @@ static void fsmc_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>         struct nand_chip *this = mtd->priv;
>         struct fsmc_nand_data *host = container_of(mtd,
>                                         struct fsmc_nand_data, mtd);
> -       void *__iomem *regs = host->regs_va;
> +       void __iomem *regs = host->regs_va;

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

[PATCH 01/11] fsmc/nand:FIX: Change the type for regs to void __iomem *

[Linus Walleij]

On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar <vipin.kumar@st.com> wrote:

> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 01/11] fsmc/nand:FIX: Change the type for regs to void __iomem *

[Artem Bityutskiy]

On Tue, 2012-10-09 at 16:14 +0530, Vipin Kumar wrote:
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>

Pushed this one to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121017/4621e9f2/attachment.sig>

[PATCH 02/11] fsmc/nand: Rearrange the fsmc_nand_data structure and update comments

[Vipin Kumar]

Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
---
 drivers/mtd/nand/fsmc_nand.c | 51 ++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index e96d7d1..bd89580 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -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;
 
+	/* Recieved from plat data */
 	struct fsmc_nand_timings *dev_timings;
+	struct mtd_partition	*partitions;
+	unsigned int		nr_partitions;
+	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 */
-- 
1.7.11.4

[PATCH 02/11] fsmc/nand: Rearrange the fsmc_nand_data structure and update comments

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:14 Tue 09 Oct     , Vipin Kumar wrote:
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
> ---
>  drivers/mtd/nand/fsmc_nand.c | 51 ++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
> index e96d7d1..bd89580 100644
> --- a/drivers/mtd/nand/fsmc_nand.c
> +++ b/drivers/mtd/nand/fsmc_nand.c
> @@ -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;
>  
> +	/* 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
> +	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 */
> -- 
> 1.7.11.4
> 

[PATCH 02/11] fsmc/nand: Rearrange the fsmc_nand_data structure and update comments

[Vipin Kumar]

On 10/9/2012 5:22 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:14 Tue 09 Oct     , Vipin Kumar wrote:
>> Signed-off-by: Vipin Kumar<vipin.kumar@st.com>
>> ---
>>   drivers/mtd/nand/fsmc_nand.c | 51 ++++++++++++++++++++++++++------------------
>>   1 file changed, 30 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
>> index e96d7d1..bd89580 100644
>> --- a/drivers/mtd/nand/fsmc_nand.c
>> +++ b/drivers/mtd/nand/fsmc_nand.c
>> @@ -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;
>>
>> +	/* 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

Yes, I can remove it completely. Thanks for that

[PATCH 02/11] fsmc/nand: Rearrange the fsmc_nand_data structure and update comments

[viresh kumar]

On Tue, Oct 9, 2012 at 4:14 PM, Vipin Kumar <vipin.kumar@st.com> wrote:
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
> ---
>  drivers/mtd/nand/fsmc_nand.c | 51 ++++++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

[PATCH 03/11] fsmc/nand: Support multiple banks connected to controller

[Vipin Kumar]

Support up to max_banks number of banks in fsmc driver.

Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
---
 .../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(-)

diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
index e2c663b..29d1a2f 100644
--- a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
@@ -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
 
 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;
diff --git a/arch/arm/boot/dts/spear300.dtsi b/arch/arm/boot/dts/spear300.dtsi
index ed3627c..19e2328 100644
--- a/arch/arm/boot/dts/spear300.dtsi
+++ b/arch/arm/boot/dts/spear300.dtsi
@@ -42,6 +42,7 @@
 			reg-names = "fsmc_regs", "nand_data";
 			st,ale-off = <0x20000>;
 			st,cle-off = <0x10000>;
+			maxbanks = <1>;
 			status = "disabled";
 		};
 
diff --git a/arch/arm/boot/dts/spear310.dtsi b/arch/arm/boot/dts/spear310.dtsi
index 62fc4fb..0272afb3 100644
--- a/arch/arm/boot/dts/spear310.dtsi
+++ b/arch/arm/boot/dts/spear310.dtsi
@@ -36,6 +36,7 @@
 			reg-names = "fsmc_regs", "nand_data";
 			st,ale-off = <0x10000>;
 			st,cle-off = <0x20000>;
+			maxbanks = <1>;
 			status = "disabled";
 		};
 
diff --git a/arch/arm/boot/dts/spear320.dtsi b/arch/arm/boot/dts/spear320.dtsi
index 1f49d69..69fe50d 100644
--- a/arch/arm/boot/dts/spear320.dtsi
+++ b/arch/arm/boot/dts/spear320.dtsi
@@ -42,6 +42,7 @@
 			reg-names = "fsmc_regs", "nand_data";
 			st,ale-off = <0x20000>;
 			st,cle-off = <0x10000>;
+			maxbanks = <1>;
 			status = "disabled";
 		};
 
diff --git a/arch/arm/boot/dts/spear600.dtsi b/arch/arm/boot/dts/spear600.dtsi
index a3c36e4..6ed57c8 100644
--- a/arch/arm/boot/dts/spear600.dtsi
+++ b/arch/arm/boot/dts/spear600.dtsi
@@ -71,6 +71,7 @@
 			reg-names = "fsmc_regs", "nand_data";
 			st,ale-off = <0x20000>;
 			st,cle-off = <0x10000>;
+			maxbanks = <1>;
 			status = "disabled";
 		};
 
diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
index 03acf18..5ea9f71 100644
--- a/arch/arm/mach-u300/core.c
+++ b/arch/arm/mach-u300/core.c
@@ -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 = {
diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index bd89580..fc6a044 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -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;
 }
diff --git a/include/linux/mtd/fsmc.h b/include/linux/mtd/fsmc.h
index b200292..f0ab734 100644
--- a/include/linux/mtd/fsmc.h
+++ b/include/linux/mtd/fsmc.h
@@ -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;
-- 
1.7.11.4

[PATCH 03/11] fsmc/nand: Support multiple banks connected to controller

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:14 Tue 09 Oct     , Vipin Kumar wrote:
> Support up to max_banks number of banks in fsmc driver.
> 
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
please rebase this over my patch that drop -off to resource
> ---
>  .../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(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> index e2c663b..29d1a2f 100644
> --- a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> @@ -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

>  
>  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;
> diff --git a/arch/arm/boot/dts/spear300.dtsi b/arch/arm/boot/dts/spear300.dtsi
> index ed3627c..19e2328 100644
> --- a/arch/arm/boot/dts/spear300.dtsi
> +++ b/arch/arm/boot/dts/spear300.dtsi
> @@ -42,6 +42,7 @@
>  			reg-names = "fsmc_regs", "nand_data";
>  			st,ale-off = <0x20000>;
>  			st,cle-off = <0x10000>;
> +			maxbanks = <1>;
>  			status = "disabled";
>  		};
>  
> diff --git a/arch/arm/boot/dts/spear310.dtsi b/arch/arm/boot/dts/spear310.dtsi
> index 62fc4fb..0272afb3 100644
> --- a/arch/arm/boot/dts/spear310.dtsi
> +++ b/arch/arm/boot/dts/spear310.dtsi
> @@ -36,6 +36,7 @@
>  			reg-names = "fsmc_regs", "nand_data";
>  			st,ale-off = <0x10000>;
>  			st,cle-off = <0x20000>;
> +			maxbanks = <1>;
>  			status = "disabled";
>  		};
>  
> diff --git a/arch/arm/boot/dts/spear320.dtsi b/arch/arm/boot/dts/spear320.dtsi
> index 1f49d69..69fe50d 100644
> --- a/arch/arm/boot/dts/spear320.dtsi
> +++ b/arch/arm/boot/dts/spear320.dtsi
> @@ -42,6 +42,7 @@
>  			reg-names = "fsmc_regs", "nand_data";
>  			st,ale-off = <0x20000>;
>  			st,cle-off = <0x10000>;
> +			maxbanks = <1>;
>  			status = "disabled";
>  		};
>  
> diff --git a/arch/arm/boot/dts/spear600.dtsi b/arch/arm/boot/dts/spear600.dtsi
> index a3c36e4..6ed57c8 100644
> --- a/arch/arm/boot/dts/spear600.dtsi
> +++ b/arch/arm/boot/dts/spear600.dtsi
> @@ -71,6 +71,7 @@
>  			reg-names = "fsmc_regs", "nand_data";
>  			st,ale-off = <0x20000>;
>  			st,cle-off = <0x10000>;
> +			maxbanks = <1>;
>  			status = "disabled";
>  		};
>  
> diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
> index 03acf18..5ea9f71 100644
> --- a/arch/arm/mach-u300/core.c
> +++ b/arch/arm/mach-u300/core.c
> @@ -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 = {
> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
> index bd89580..fc6a044 100644
> --- a/drivers/mtd/nand/fsmc_nand.c
> +++ b/drivers/mtd/nand/fsmc_nand.c
> @@ -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;
>  }
> diff --git a/include/linux/mtd/fsmc.h b/include/linux/mtd/fsmc.h
> index b200292..f0ab734 100644
> --- a/include/linux/mtd/fsmc.h
> +++ b/include/linux/mtd/fsmc.h
> @@ -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;
> -- 
> 1.7.11.4
> 

[PATCH 03/11] fsmc/nand: Support multiple banks connected to controller

[Vipin Kumar]

On 10/9/2012 5:25 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:14 Tue 09 Oct     , Vipin Kumar wrote:
>> Support up to max_banks number of banks in fsmc driver.
>>
>> Signed-off-by: Vipin Kumar<vipin.kumar@st.com>
> please rebase this over my patch that drop -off to resource

Sure, I will do that. I actually planned to do it but forgot while 
sending it

>> ---
>>   .../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(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
>> index e2c663b..29d1a2f 100644
>> --- a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
>> @@ -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
>

hmmm....OK
I think you are right. May be optional with a default of 1 is a better 
option

>>
>>   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;
>> diff --git a/arch/arm/boot/dts/spear300.dtsi b/arch/arm/boot/dts/spear300.dtsi
>> index ed3627c..19e2328 100644
>> --- a/arch/arm/boot/dts/spear300.dtsi
>> +++ b/arch/arm/boot/dts/spear300.dtsi
>> @@ -42,6 +42,7 @@
>>   			reg-names = "fsmc_regs", "nand_data";
>>   			st,ale-off =<0x20000>;
>>   			st,cle-off =<0x10000>;
>> +			maxbanks =<1>;
>>   			status = "disabled";
>>   		};
>>
>> diff --git a/arch/arm/boot/dts/spear310.dtsi b/arch/arm/boot/dts/spear310.dtsi
>> index 62fc4fb..0272afb3 100644
>> --- a/arch/arm/boot/dts/spear310.dtsi
>> +++ b/arch/arm/boot/dts/spear310.dtsi
>> @@ -36,6 +36,7 @@
>>   			reg-names = "fsmc_regs", "nand_data";
>>   			st,ale-off =<0x10000>;
>>   			st,cle-off =<0x20000>;
>> +			maxbanks =<1>;
>>   			status = "disabled";
>>   		};
>>
>> diff --git a/arch/arm/boot/dts/spear320.dtsi b/arch/arm/boot/dts/spear320.dtsi
>> index 1f49d69..69fe50d 100644
>> --- a/arch/arm/boot/dts/spear320.dtsi
>> +++ b/arch/arm/boot/dts/spear320.dtsi
>> @@ -42,6 +42,7 @@
>>   			reg-names = "fsmc_regs", "nand_data";
>>   			st,ale-off =<0x20000>;
>>   			st,cle-off =<0x10000>;
>> +			maxbanks =<1>;
>>   			status = "disabled";
>>   		};
>>
>> diff --git a/arch/arm/boot/dts/spear600.dtsi b/arch/arm/boot/dts/spear600.dtsi
>> index a3c36e4..6ed57c8 100644
>> --- a/arch/arm/boot/dts/spear600.dtsi
>> +++ b/arch/arm/boot/dts/spear600.dtsi
>> @@ -71,6 +71,7 @@
>>   			reg-names = "fsmc_regs", "nand_data";
>>   			st,ale-off =<0x20000>;
>>   			st,cle-off =<0x10000>;
>> +			maxbanks =<1>;
>>   			status = "disabled";
>>   		};
>>
>> diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
>> index 03acf18..5ea9f71 100644
>> --- a/arch/arm/mach-u300/core.c
>> +++ b/arch/arm/mach-u300/core.c
>> @@ -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 = {
>> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
>> index bd89580..fc6a044 100644
>> --- a/drivers/mtd/nand/fsmc_nand.c
>> +++ b/drivers/mtd/nand/fsmc_nand.c
>> @@ -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;
>>   }
>> diff --git a/include/linux/mtd/fsmc.h b/include/linux/mtd/fsmc.h
>> index b200292..f0ab734 100644
>> --- a/include/linux/mtd/fsmc.h
>> +++ b/include/linux/mtd/fsmc.h
>> @@ -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;
>> --
>> 1.7.11.4
>>
> .
>

[PATCH 03/11] fsmc/nand: Support multiple banks connected to controller

[Artem Bityutskiy]

On Tue, 2012-10-09 at 16:14 +0530, Vipin Kumar wrote:
> Support up to max_banks number of banks in fsmc driver.
> 
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>

I tried to apply this, but it does not apply cleanly to l2-mtd.git,
which is based on 3.7-rc1.

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121017/8034920f/attachment.sig>

[PATCH 04/11] fsmc/nand: Accept nand timing parameters via DT

[Vipin Kumar]

Add support to accept nand timing parameters via device tree

Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
---
 .../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(-)

diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
index 29d1a2f..6a7fc43 100644
--- a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
@@ -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;
 
diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index fc6a044..f3d69b3 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -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);
 
+	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;
+
+		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;
 
diff --git a/include/linux/mtd/fsmc.h b/include/linux/mtd/fsmc.h
index f0ab734..4fbdce4 100644
--- a/include/linux/mtd/fsmc.h
+++ b/include/linux/mtd/fsmc.h
@@ -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;
-- 
1.7.11.4

[PATCH 04/11] fsmc/nand: Accept nand timing parameters via DT

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:14 Tue 09 Oct     , Vipin Kumar wrote:
> Add support to accept nand timing parameters via device tree
> 
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
> ---
>  .../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(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> index 29d1a2f..6a7fc43 100644
> --- a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> @@ -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;
>  
> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
> index fc6a044..f3d69b3 100644
> --- a/drivers/mtd/nand/fsmc_nand.c
> +++ b/drivers/mtd/nand/fsmc_nand.c
> @@ -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);
>  
> +	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
> +
> +		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;
>  
> diff --git a/include/linux/mtd/fsmc.h b/include/linux/mtd/fsmc.h
> index f0ab734..4fbdce4 100644
> --- a/include/linux/mtd/fsmc.h
> +++ b/include/linux/mtd/fsmc.h
> @@ -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;
> -- 
> 1.7.11.4
> 

[PATCH 04/11] fsmc/nand: Accept nand timing parameters via DT

[Vipin Kumar]

On 10/9/2012 5:27 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:14 Tue 09 Oct     , Vipin Kumar wrote:
>> Add support to accept nand timing parameters via device tree
>>
>> Signed-off-by: Vipin Kumar<vipin.kumar@st.com>
>> ---
>>   .../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(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
>> index 29d1a2f..6a7fc43 100644
>> --- a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
>> @@ -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;
>>
>> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
>> index fc6a044..f3d69b3 100644
>> --- a/drivers/mtd/nand/fsmc_nand.c
>> +++ b/drivers/mtd/nand/fsmc_nand.c
>> @@ -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);
>>
>> +	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

OK, will add a mask in v2

>> +
>> +		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;
>>
>> diff --git a/include/linux/mtd/fsmc.h b/include/linux/mtd/fsmc.h
>> index f0ab734..4fbdce4 100644
>> --- a/include/linux/mtd/fsmc.h
>> +++ b/include/linux/mtd/fsmc.h
>> @@ -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;
>> --
>> 1.7.11.4
>>
> .
>

[PATCH 04/11] fsmc/nand: Accept nand timing parameters via DT

[viresh kumar]

On Tue, Oct 9, 2012 at 4:14 PM, Vipin Kumar <vipin.kumar@st.com> wrote:
> diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt

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

Are they specific to fsmc controller or nand memory type? If to fsmc then
please add my

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

else
I would suggest to add these to mtd bindings instead, so that same fields
can be used by multiple drivers....

Similar approach is used for mmc controllers too.

[PATCH 05/11] fsmc: Implement ready/busy through gpio pin

[Vipin Kumar]

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 <vipin.kumar@st.com>
---
 .../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(-)

diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
index 6a7fc43..598bca2 100644
--- a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
@@ -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 {
diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index f3d69b3..eaffd3b 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -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;
 }
diff --git a/include/linux/mtd/fsmc.h b/include/linux/mtd/fsmc.h
index 4fbdce4..eed22a1 100644
--- a/include/linux/mtd/fsmc.h
+++ b/include/linux/mtd/fsmc.h
@@ -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,
@@ -149,6 +159,7 @@ enum access_mode {
  */
 struct fsmc_nand_platform_data {
 	struct fsmc_nand_timings nand_timings;
+	struct fsmc_rbpin	rbpin;
 	struct mtd_partition	*partitions;
 	unsigned int		nr_partitions;
 	unsigned int		options;
-- 
1.7.11.4

[PATCH 05/11] fsmc: Implement ready/busy through gpio pin

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:14 Tue 09 Oct     , Vipin Kumar wrote:
> 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 <vipin.kumar@st.com>
> ---
>  .../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(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> index 6a7fc43..598bca2 100644
> --- a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> @@ -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 {
> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
> index f3d69b3..eaffd3b 100644
> --- a/drivers/mtd/nand/fsmc_nand.c
> +++ b/drivers/mtd/nand/fsmc_nand.c
> @@ -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;
>  }
> diff --git a/include/linux/mtd/fsmc.h b/include/linux/mtd/fsmc.h
> index 4fbdce4..eed22a1 100644
> --- a/include/linux/mtd/fsmc.h
> +++ b/include/linux/mtd/fsmc.h
> @@ -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,
> @@ -149,6 +159,7 @@ enum access_mode {
>   */
>  struct fsmc_nand_platform_data {
>  	struct fsmc_nand_timings nand_timings;
> +	struct fsmc_rbpin	rbpin;
do more simple
	int 			rbpin;

	and set it as -EINVAL to use RB_WAIT

so in the binding if you do not specifcy a gpio or a valid one we switch to
wait

Best Regards,
J.

[PATCH 05/11] fsmc: Implement ready/busy through gpio pin

[viresh kumar]

On Tue, Oct 9, 2012 at 4:14 PM, Vipin Kumar <vipin.kumar@st.com> wrote:
> diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> +- 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

Why st,*** maybe fsmc,***?

Would be better if you update the example too in this file.

> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c

> + * @rb_pin:            Ready/Busy data

> +       struct fsmc_rbpin       *rbpin;

comment variable mismatch :(

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

Can function prototype come in two lines instead of three?

> @@ -1049,6 +1075,8 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)

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

Don't you want to set its direction to input?

[PATCH 06/11] fsmc/nand: Modify the wait to uninterruptible

[Vipin Kumar]

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 <vipin.kumar@st.com>
---
 drivers/mtd/nand/fsmc_nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index eaffd3b..4b29a64 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -607,7 +607,7 @@ static int dma_xfer(struct fsmc_nand_data *host, void *buffer, int len,
 	dma_async_issue_pending(chan);
 
 	ret =
-	wait_for_completion_interruptible_timeout(&host->dma_access_complete,
+	wait_for_completion_timeout(&host->dma_access_complete,
 				msecs_to_jiffies(3000));
 	if (ret <= 0) {
 		chan->device->device_control(chan, DMA_TERMINATE_ALL, 0);
-- 
1.7.11.4

[PATCH 06/11] fsmc/nand: Modify the wait to uninterruptible

[Linus Walleij]

On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar <vipin.kumar@st.com> wrote:

> 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 <vipin.kumar@st.com>

Yes this is wise.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 06/11] fsmc/nand: Modify the wait to uninterruptible

[Artem Bityutskiy]

On Tue, 2012-10-09 at 16:14 +0530, Vipin Kumar wrote:
> 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 <vipin.kumar@st.com>

Pushed this one to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121017/59249ec7/attachment-0001.sig>

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[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
xfers during read and write accesses.

Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
---
 drivers/mtd/nand/fsmc_nand.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index 4b29a64..8de6dcf 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -323,6 +323,7 @@ struct fsmc_nand_data {
 	struct dma_chan		*read_dma_chan;
 	struct dma_chan		*write_dma_chan;
 	struct completion	dma_access_complete;
+	void			*dma_buf;
 
 	/* Recieved from plat data */
 	struct fsmc_rbpin	*rbpin;
@@ -675,7 +676,8 @@ static void fsmc_read_buf_dma(struct mtd_info *mtd, uint8_t *buf, int len)
 	struct fsmc_nand_data *host;
 
 	host = container_of(mtd, struct fsmc_nand_data, mtd);
-	dma_xfer(host, buf, len, DMA_FROM_DEVICE);
+	dma_xfer(host, host->dma_buf, len, DMA_FROM_DEVICE);
+	memcpy(buf, (const void *)host->dma_buf, len);
 }
 
 /*
@@ -690,7 +692,8 @@ static void fsmc_write_buf_dma(struct mtd_info *mtd, const uint8_t *buf,
 	struct fsmc_nand_data *host;
 
 	host = container_of(mtd, struct fsmc_nand_data, mtd);
-	dma_xfer(host, (void *)buf, len, DMA_TO_DEVICE);
+	memcpy(host->dma_buf, buf, len);
+	dma_xfer(host, host->dma_buf, len, DMA_TO_DEVICE);
 }
 
 /*
@@ -1133,6 +1136,13 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "Unable to get write dma channel\n");
 			goto err_req_write_chnl;
 		}
+
+		host->dma_buf = kmalloc(NAND_MAX_PAGESIZE + NAND_MAX_OOBSIZE,
+				GFP_KERNEL);
+		if (!host->dma_buf) {
+			dev_err(&pdev->dev, "failed to allocate dma buffer\n");
+			goto err_req_dma_buf;
+		}
 		nand->read_buf = fsmc_read_buf_dma;
 		nand->write_buf = fsmc_write_buf_dma;
 		break;
@@ -1246,6 +1256,9 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
 err_probe:
 err_scan_ident:
 	if (host->mode == USE_DMA_ACCESS)
+		kfree(host->dma_buf);
+err_req_dma_buf:
+	if (host->mode == USE_DMA_ACCESS)
 		dma_release_channel(host->write_dma_chan);
 err_req_write_chnl:
 	if (host->mode == USE_DMA_ACCESS)
-- 
1.7.11.4

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[viresh kumar]

On Tue, Oct 9, 2012 at 4:14 PM, Vipin Kumar <vipin.kumar@st.com> wrote:
> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
> @@ -675,7 +676,8 @@ static void fsmc_read_buf_dma(struct mtd_info *mtd, uint8_t *buf, int len)
>         struct fsmc_nand_data *host;
>
>         host = container_of(mtd, struct fsmc_nand_data, mtd);
> -       dma_xfer(host, buf, len, DMA_FROM_DEVICE);
> +       dma_xfer(host, host->dma_buf, len, DMA_FROM_DEVICE);
> +       memcpy(buf, (const void *)host->dma_buf, len);

Ahh.. Too much overhead. Can't you do something better here?
- DMA can be done to user buffers too, Pratyush has done some work in
past on this.
- There must be some other way of sharing kernel buffer to user space.

--
viresh

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Linus Walleij]

On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar <vipin.kumar@st.com> wrote:

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

> xfers during read and write accesses.
>
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>

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

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[viresh kumar]

On Wed, Oct 10, 2012 at 10:37 PM, Linus Walleij
<linus.walleij@linaro.org> 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

Both dw_dmac and pl080 for different SPEAr SoC's.

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

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

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Vipin Kumar]

On 10/11/2012 8:46 AM, viresh kumar wrote:
> On Wed, Oct 10, 2012 at 10:37 PM, Linus Walleij
> <linus.walleij@linaro.org>  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
>
> Both dw_dmac and pl080 for different SPEAr SoC's.
>
>> 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.
>
> But what's the problem you saw vipin, for which you generated this patch?
> .
>

The nand tests were failing and that was because of a user pointer being 
passed to them but it was a long time back and this patch just carried 
on in the local repo. It is only now that I am sending it

It seems may be the dma drivers also got updated so it needs another 
test cycle it. I will do the needful and re-reply back on this mail

Vipin

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Vipin Kumar]

On 10/10/2012 10:37 PM, Linus Walleij wrote:
> On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar<vipin.kumar@st.com>  wrote:
>
>> 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?
>

Yes Sorry for that :)

>> xfers during read and write accesses.
>>
>> Signed-off-by: Vipin Kumar<vipin.kumar@st.com>
>
> 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.
>

I would debug again and reply to this mail soon

> Yours,
> Linus Walleij
> .
>

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[viresh kumar]

On Wed, Oct 10, 2012 at 10:37 PM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar <vipin.kumar@st.com> 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.

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.

dwc_prep_dma_memcpy() doesn't expect a virtual address, look at type
of src & dest bufs: dma_addr_t. It is responsibility of user drivers to pass
physically contiguous address to it.

--
viresh

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Linus Walleij]

On Thu, Oct 11, 2012 at 6:15 AM, viresh kumar <viresh.kumar@linaro.org> wrote:
> On Wed, Oct 10, 2012 at 10:37 PM, Linus Walleij
> <linus.walleij@linaro.org> wrote:

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

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 :-/

Artem will know I hope!

Yours,
Linus Walleij

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[viresh kumar]

On Thu, Oct 11, 2012 at 9:36 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Oct 11, 2012 at 6:15 AM, viresh kumar <viresh.kumar@linaro.org> wrote:
>> I pray that i am wrong here, otherwise i would be thrown out from

s/am/am not/ :(

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

Glad to hear that. :)
It happens very few times in one's lifetime, that a beginner like me is correct
and an expert like you is not. Just kidding :)

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

--
viresh

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Linus Walleij]

On Thu, Oct 11, 2012 at 7:07 PM, viresh kumar <viresh.kumar@linaro.org> wrote:
> On Thu, Oct 11, 2012 at 9:36 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> 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.

Not the block layer, the block layer of the subsystem.

It has to pick out the chunks that get transferred down to the drivers.

For example drivers/mmc/card/block.c will convert transfers into
sglists and pass down to the MMC drivers. mmc_queue_map_sg()
calls blk_rq_map_sg() to do the job. It's even documented in
Documentation/block/biodoc.txt

I don't know how that works in MTD...

Yours,
Linus Walleij

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Vipin Kumar]

On 10/11/2012 9:36 PM, Linus Walleij wrote:
> On Thu, Oct 11, 2012 at 6:15 AM, viresh kumar<viresh.kumar@linaro.org>  wrote:
>> On Wed, Oct 10, 2012 at 10:37 PM, Linus Walleij
>> <linus.walleij@linaro.org>  wrote:
>
>>> 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.
>
> 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 *'.

Regards
Vipin

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Artem Bityutskiy]

On Fri, 2012-10-12 at 09:25 +0530, Vipin Kumar wrote:
> 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

I am not sure what does "user" buffers mean, but they are vmalloced()
buffer, not kmalloc()'ed, so they are not physically contiguous.

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

They are not "_user", they are really kernel buffers. Or what do you
mean, which exactly type do you suggest?

This stuff is something many people are bringing up for many years
already. Drivers that do DMA do not cope with vmalloc()ed memory well,
and we grew a number of hacks in several drives. I mean, hacks like the
one you are introducing to your driver.

I'd solve the problem by changing the in-kernel mtd users to use
physically-contiguous memory instead. The following are the users I can
think of:

UBI, UBIFS, JFFS2, mtdtests and probably mtdswap.

They use vmalloc() when they need to read/write entire eraseblock, which
is usually 128KiB or 256KiB, and kmalloc() that much may fail if the
memory is fragmented. 

In many cases, it is easy to read/write in smaller chunk, but several
times. E.g., mtdtests could be changed.

In some cases, it is not easy, though.

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.


Then we can add a guard check to all mtd function which accept a buffer
and WARN() if it not physically contiguous.

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121015/738aa0ca/attachment-0001.sig>

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Brian Norris]

On Mon, Oct 15, 2012 at 6:18 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> 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.

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

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

Brian

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Linus Walleij]

On Mon, Oct 15, 2012 at 6:27 PM, Brian Norris
<computersforpeace@gmail.com> wrote:

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

Yours,
Linus Walleij

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Artem Bityutskiy]

On Mon, 2012-10-15 at 21:51 +0200, Linus Walleij wrote:
> On Mon, Oct 15, 2012 at 6:27 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> 
> > 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?

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121016/e67d78eb/attachment-0001.sig>

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Linus Walleij]

On Tue, Oct 16, 2012 at 9:14 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Mon, 2012-10-15 at 21:51 +0200, Linus Walleij wrote:
>> On Mon, Oct 15, 2012 at 6:27 PM, Brian Norris
>> <computersforpeace@gmail.com> wrote:
>>
>> > 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?

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

Yours,
Linus Walleij

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Brian Norris]

On Tue, Oct 16, 2012 at 3:05 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Oct 16, 2012 at 9:14 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> On Mon, 2012-10-15 at 21:51 +0200, Linus Walleij wrote:
>>> On Mon, Oct 15, 2012 at 6:27 PM, Brian Norris
>>> <computersforpeace@gmail.com> wrote:
>>>
>>> > 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.

Did you mean vmalloc()'s?

>> What are the SC functions names you refer?
>
> I'm mainly thinking of blk_rq_map_sg()
> from block/blk-merge.c

I am also interested in the potential for the S-G approach. I'll see
what kind of work it takes to implement soon, hopefully.

As a side note, my hardware can, in fact, perform scatter-gather DMA.
If we were to support both S-G hardware like mine in addition to
standard, page-at-time access (nand_chip->write_page, for example),
could we introduce a DMA interface to nand_chip? Then this interface
would just be set to NULL for those that can't handle it.

Brian

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Artem Bityutskiy]

On Sun, 2012-10-21 at 00:38 -0700, Brian Norris wrote:
> >> 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?

Yes.

> 
> >> What are the SC functions names you refer?
> >
> > 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...

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121021/243509aa/attachment.sig>

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Artem Bityutskiy]

On Sun, 2012-10-21 at 14:00 +0300, Artem Bityutskiy wrote:
> > > 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...

Sorry, it would be correct to say that re-working MTD API _users_ to
incorporate the SG API would be a lot more work.

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121021/28e3b5cc/attachment.sig>

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Artem Bityutskiy]

On Mon, 2012-10-15 at 09:27 -0700, Brian Norris wrote:
> > 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

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.

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

Me too. But this does not happen. People bring this up for years. Of
course it is easier to hack drivers, and very understandable. So what
I would like to do is somehow force people to fix this issue.

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

This function would probably need a cookie which it returns and which
the mtd_free() function would use to call either kfree() or
dma_release_from_contiguous()...

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?

-- 
Best Regards,
Artem Bityutskiy


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121016/04d3c8f5/attachment.sig>

[PATCH 07/11] fsmc/nand: Provide contiguous buffers to dma

[Brian Norris]

On Tue, Oct 16, 2012 at 12:11 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Mon, 2012-10-15 at 09:27 -0700, Brian Norris wrote:
>> 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.

Yeah, I know it was a workaround. It's another issue of balancing time
and the "best" solution. Of course, the solutions actually don't seem
that difficult now, so I probably should have just fixed the ioctl()
in the first place... But I was simply pointing this out as evidence
of a real memory-fragmentation issue.

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

That's one way, but it would require specifying some arbitrary
iteration length, right? Like some multiple of writesize?

Instead, how about modelling it more closely after mtdchar_write(),
using mtd_kmalloc_up_to(), then iteratively call copy_from_user()?

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

It sounds OK, but that depends on the success rate of
dma_alloc_from_contiguous(). I'm not familiar with it.

Brian

[PATCH 08/11] fsmc/nand: Use relaxed variants of io accessors

[Vipin Kumar]

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 <vipin.kumar@st.com>
---
 drivers/mtd/nand/fsmc_nand.c | 54 ++++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index 8de6dcf..f48ee60 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -400,13 +400,13 @@ static void fsmc_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 			pc |= FSMC_ENABLE;
 		else
 			pc &= ~FSMC_ENABLE;
-		writel(pc, FSMC_NAND_REG(regs, bank, PC));
+		writel_relaxed(pc, FSMC_NAND_REG(regs, bank, PC));
 	}
 
 	mb();
 
 	if (cmd != NAND_CMD_NONE)
-		writeb(cmd, this->IO_ADDR_W);
+		writeb_relaxed(cmd, this->IO_ADDR_W);
 }
 
 /*
@@ -433,14 +433,18 @@ static void fsmc_nand_setup(void __iomem *regs, uint32_t bank, uint32_t busw,
 	tset = (timings->tset & FSMC_TSET_MASK) << FSMC_TSET_SHIFT;
 
 	if (busw)
-		writel(value | FSMC_DEVWID_16, FSMC_NAND_REG(regs, bank, PC));
+		writel_relaxed(value | FSMC_DEVWID_16,
+				FSMC_NAND_REG(regs, bank, PC));
 	else
-		writel(value | FSMC_DEVWID_8, FSMC_NAND_REG(regs, bank, PC));
+		writel_relaxed(value | FSMC_DEVWID_8,
+				FSMC_NAND_REG(regs, bank, PC));
 
-	writel(readl(FSMC_NAND_REG(regs, bank, PC)) | tclr | tar,
+	writel_relaxed(readl(FSMC_NAND_REG(regs, bank, PC)) | tclr | tar,
 			FSMC_NAND_REG(regs, bank, PC));
-	writel(thiz | thold | twait | tset, FSMC_NAND_REG(regs, bank, COMM));
-	writel(thiz | thold | twait | tset, FSMC_NAND_REG(regs, bank, ATTRIB));
+	writel_relaxed(thiz | thold | twait | tset,
+			FSMC_NAND_REG(regs, bank, COMM));
+	writel_relaxed(thiz | thold | twait | tset,
+			FSMC_NAND_REG(regs, bank, ATTRIB));
 }
 
 /*
@@ -453,11 +457,11 @@ static void fsmc_enable_hwecc(struct mtd_info *mtd, int mode)
 	void __iomem *regs = host->regs_va;
 	uint32_t bank = host->bank;
 
-	writel(readl(FSMC_NAND_REG(regs, bank, PC)) & ~FSMC_ECCPLEN_256,
+	writel_relaxed(readl(FSMC_NAND_REG(regs, bank, PC)) & ~FSMC_ECCPLEN_256,
 			FSMC_NAND_REG(regs, bank, PC));
-	writel(readl(FSMC_NAND_REG(regs, bank, PC)) & ~FSMC_ECCEN,
+	writel_relaxed(readl(FSMC_NAND_REG(regs, bank, PC)) & ~FSMC_ECCEN,
 			FSMC_NAND_REG(regs, bank, PC));
-	writel(readl(FSMC_NAND_REG(regs, bank, PC)) | FSMC_ECCEN,
+	writel_relaxed(readl(FSMC_NAND_REG(regs, bank, PC)) | FSMC_ECCEN,
 			FSMC_NAND_REG(regs, bank, PC));
 }
 
@@ -477,7 +481,7 @@ static int fsmc_read_hwecc_ecc4(struct mtd_info *mtd, const uint8_t *data,
 	unsigned long deadline = jiffies + FSMC_BUSY_WAIT_TIMEOUT;
 
 	do {
-		if (readl(FSMC_NAND_REG(regs, bank, STS)) & FSMC_CODE_RDY)
+		if (readl_relaxed(FSMC_NAND_REG(regs, bank, STS)) & FSMC_CODE_RDY)
 			break;
 		else
 			cond_resched();
@@ -488,25 +492,25 @@ static int fsmc_read_hwecc_ecc4(struct mtd_info *mtd, const uint8_t *data,
 		return -ETIMEDOUT;
 	}
 
-	ecc_tmp = readl(FSMC_NAND_REG(regs, bank, ECC1));
+	ecc_tmp = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC1));
 	ecc[0] = (uint8_t) (ecc_tmp >> 0);
 	ecc[1] = (uint8_t) (ecc_tmp >> 8);
 	ecc[2] = (uint8_t) (ecc_tmp >> 16);
 	ecc[3] = (uint8_t) (ecc_tmp >> 24);
 
-	ecc_tmp = readl(FSMC_NAND_REG(regs, bank, ECC2));
+	ecc_tmp = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC2));
 	ecc[4] = (uint8_t) (ecc_tmp >> 0);
 	ecc[5] = (uint8_t) (ecc_tmp >> 8);
 	ecc[6] = (uint8_t) (ecc_tmp >> 16);
 	ecc[7] = (uint8_t) (ecc_tmp >> 24);
 
-	ecc_tmp = readl(FSMC_NAND_REG(regs, bank, ECC3));
+	ecc_tmp = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC3));
 	ecc[8] = (uint8_t) (ecc_tmp >> 0);
 	ecc[9] = (uint8_t) (ecc_tmp >> 8);
 	ecc[10] = (uint8_t) (ecc_tmp >> 16);
 	ecc[11] = (uint8_t) (ecc_tmp >> 24);
 
-	ecc_tmp = readl(FSMC_NAND_REG(regs, bank, STS));
+	ecc_tmp = readl_relaxed(FSMC_NAND_REG(regs, bank, STS));
 	ecc[12] = (uint8_t) (ecc_tmp >> 16);
 
 	return 0;
@@ -526,7 +530,7 @@ static int fsmc_read_hwecc_ecc1(struct mtd_info *mtd, const uint8_t *data,
 	uint32_t bank = host->bank;
 	uint32_t ecc_tmp;
 
-	ecc_tmp = readl(FSMC_NAND_REG(regs, bank, ECC1));
+	ecc_tmp = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC1));
 	ecc[0] = (uint8_t) (ecc_tmp >> 0);
 	ecc[1] = (uint8_t) (ecc_tmp >> 8);
 	ecc[2] = (uint8_t) (ecc_tmp >> 16);
@@ -635,10 +639,10 @@ static void fsmc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
 		uint32_t *p = (uint32_t *)buf;
 		len = len >> 2;
 		for (i = 0; i < len; i++)
-			writel(p[i], chip->IO_ADDR_W);
+			writel_relaxed(p[i], chip->IO_ADDR_W);
 	} else {
 		for (i = 0; i < len; i++)
-			writeb(buf[i], chip->IO_ADDR_W);
+			writeb_relaxed(buf[i], chip->IO_ADDR_W);
 	}
 }
 
@@ -658,10 +662,10 @@ static void fsmc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
 		uint32_t *p = (uint32_t *)buf;
 		len = len >> 2;
 		for (i = 0; i < len; i++)
-			p[i] = readl(chip->IO_ADDR_R);
+			p[i] = readl_relaxed(chip->IO_ADDR_R);
 	} else {
 		for (i = 0; i < len; i++)
-			buf[i] = readb(chip->IO_ADDR_R);
+			buf[i] = readb_relaxed(chip->IO_ADDR_R);
 	}
 }
 
@@ -792,7 +796,7 @@ static int fsmc_bch8_correct_data(struct mtd_info *mtd, uint8_t *dat,
 	uint32_t num_err, i;
 	uint32_t ecc1, ecc2, ecc3, ecc4;
 
-	num_err = (readl(FSMC_NAND_REG(regs, bank, STS)) >> 10) & 0xF;
+	num_err = (readl_relaxed(FSMC_NAND_REG(regs, bank, STS)) >> 10) & 0xF;
 
 	/* no bit flipping */
 	if (likely(num_err == 0))
@@ -835,10 +839,10 @@ static int fsmc_bch8_correct_data(struct mtd_info *mtd, uint8_t *dat,
 	 * uint64_t array and error offset indexes are populated in err_idx
 	 * array
 	 */
-	ecc1 = readl(FSMC_NAND_REG(regs, bank, ECC1));
-	ecc2 = readl(FSMC_NAND_REG(regs, bank, ECC2));
-	ecc3 = readl(FSMC_NAND_REG(regs, bank, ECC3));
-	ecc4 = readl(FSMC_NAND_REG(regs, bank, STS));
+	ecc1 = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC1));
+	ecc2 = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC2));
+	ecc3 = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC3));
+	ecc4 = readl_relaxed(FSMC_NAND_REG(regs, bank, STS));
 
 	err_idx[0] = (ecc1 >> 0) & 0x1FFF;
 	err_idx[1] = (ecc1 >> 13) & 0x1FFF;
-- 
1.7.11.4

[PATCH 08/11] fsmc/nand: Use relaxed variants of io accessors

[viresh kumar]

On Tue, Oct 9, 2012 at 4:14 PM, Vipin Kumar <vipin.kumar@st.com> wrote:
> 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.

Good change.

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

[PATCH 08/11] fsmc/nand: Use relaxed variants of io accessors

[Linus Walleij]

On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar <vipin.kumar@st.com> wrote:

> 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 <vipin.kumar@st.com>

Good idea for any performance-critical code! Thanks!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 09/11] fsmc/nand:FIX: replace change_bit routine

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

Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
---
 drivers/mtd/nand/fsmc_nand.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index f48ee60..762cf83 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -859,7 +859,9 @@ static int fsmc_bch8_correct_data(struct mtd_info *mtd, uint8_t *dat,
 		change_bit(1, (unsigned long *)&err_idx[i]);
 
 		if (err_idx[i] < chip->ecc.size * 8) {
-			change_bit(err_idx[i], (unsigned long *)dat);
+			uint8_t *p = dat + err_idx[i] / 8;
+			*p = *p ^ (1 << (err_idx[i] % 8));
+
 			i++;
 		}
 	}
-- 
1.7.11.4

[PATCH 09/11] fsmc/nand:FIX: replace change_bit routine

[Linus Walleij]

On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar <vipin.kumar@st.com> wrote:

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

>                 if (err_idx[i] < chip->ecc.size * 8) {
> -                       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...

Yours,
Linus Walleij

[PATCH 09/11] fsmc/nand:FIX: replace change_bit routine

[Nicolas Pitre]

On Wed, 10 Oct 2012, Linus Walleij wrote:

> On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar <vipin.kumar@st.com> wrote:
> 
> > 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?

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.

> >                 if (err_idx[i] < chip->ecc.size * 8) {
> > -                       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

[PATCH 09/11] fsmc/nand:FIX: replace change_bit routine

[Vipin Kumar]

On 10/11/2012 1:51 AM, Nicolas Pitre wrote:
> On Wed, 10 Oct 2012, Linus Walleij wrote:
>
>> On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar<vipin.kumar@st.com>  wrote:
>>
>>> 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?
>

Hello Nicolas

> No.  It is this code which is totally broken.
>

Yes, I understand and accept the probelm. That's why the fix

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

No, there is no requirement for an atomic change_bit

>>>                  if (err_idx[i]<  chip->ecc.size * 8) {
>>> -                       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.
>

Yes, that's exactly why I kept /8 and %8. I would change the code as 
suggested by you

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

There is only one place which needs a change_bit. I would send a v2 with 
the suggested change

Vipin

>
> Nicolas
> .
>

[PATCH 09/11] fsmc/nand:FIX: replace change_bit routine

[Russell King - ARM Linux]

On Wed, Oct 10, 2012 at 07:22:04PM +0200, Linus Walleij wrote:
> On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar <vipin.kumar@st.com> wrote:
> 
> > 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.

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

Spot the intentional cast:

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

[PATCH 09/11] fsmc/nand:FIX: replace change_bit routine

[Vipin Kumar]

On 10/11/2012 2:15 AM, Russell King - ARM Linux wrote:
> On Wed, Oct 10, 2012 at 07:22:04PM +0200, Linus Walleij wrote:
>> On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar<vipin.kumar@st.com>  wrote:
>>
>>> 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.
>

Thanks. Got your point

>> Can the ARM change_bit() not be fixed, so that
>> long arguments are the only option?
>
> Spot the intentional cast:
>
>>> -                       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. ;)
> .

I agree with you. Although I was a bit relaxed this time, I would really 
think before adding a cast explicitly just to avoid a warning

-Vipin

[PATCH 10/11] fsmc/nand: Add sw bch support for ecc calculation/correction

[Vipin Kumar]

Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
---
 .../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(-)

diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
index 598bca2..dcf513b 100644
--- a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
@@ -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
 
+- nand-sw-ecc: boolean indicating whether s/w ecc is supported
+
 Example:
 
 	fsmc: flash at d1800000 {
diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
index 762cf83..ff84468 100644
--- a/drivers/mtd/nand/fsmc_nand.c
+++ b/drivers/mtd/nand/fsmc_nand.c
@@ -946,6 +946,9 @@ static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
 	} else
 		pdata->rbpin.use_pin = FSMC_RB_WAIT;
 
+	if (of_property_read_bool(np, "nand-sw-ecc"))
+		pdata->sw_ecc = true;
+
 	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();
+			}
 		}
 	}
 
diff --git a/include/linux/mtd/fsmc.h b/include/linux/mtd/fsmc.h
index eed22a1..07eee6e 100644
--- a/include/linux/mtd/fsmc.h
+++ b/include/linux/mtd/fsmc.h
@@ -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,
-- 
1.7.11.4

[PATCH 10/11] fsmc/nand: Add sw bch support for ecc calculation/correction

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:14 Tue 09 Oct     , Vipin Kumar wrote:
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
> ---
>  .../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(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> index 598bca2..dcf513b 100644
> --- a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
> @@ -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
>  
> +- nand-sw-ecc: boolean indicating whether s/w ecc is supported
> +
please use the generic binding
 - nand-ecc-mode = xx
>  Example:
>  
>  	fsmc: flash at d1800000 {
> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
> index 762cf83..ff84468 100644
> --- a/drivers/mtd/nand/fsmc_nand.c
> +++ b/drivers/mtd/nand/fsmc_nand.c
> @@ -946,6 +946,9 @@ static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
>  	} else
>  		pdata->rbpin.use_pin = FSMC_RB_WAIT;
>  
> +	if (of_property_read_bool(np, "nand-sw-ecc"))
> +		pdata->sw_ecc = true;
> +
of_get_nand_ecc_mode

>  	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();
> +			}
>  		}
>  	}
>  
> diff --git a/include/linux/mtd/fsmc.h b/include/linux/mtd/fsmc.h
> index eed22a1..07eee6e 100644
> --- a/include/linux/mtd/fsmc.h
> +++ b/include/linux/mtd/fsmc.h
> @@ -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,
> -- 
> 1.7.11.4
> 

[PATCH 10/11] fsmc/nand: Add sw bch support for ecc calculation/correction

[Vipin Kumar]

On 10/9/2012 5:20 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:14 Tue 09 Oct     , Vipin Kumar wrote:
>> Signed-off-by: Vipin Kumar<vipin.kumar@st.com>
>> ---
>>   .../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(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
>> index 598bca2..dcf513b 100644
>> --- a/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/fsmc-nand.txt
>> @@ -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
>>
>> +- nand-sw-ecc: boolean indicating whether s/w ecc is supported
>> +
> please use the generic binding
>   - nand-ecc-mode = xx

Thanks for pointing. I will do the needful

>>   Example:
>>
>>   	fsmc: flash at d1800000 {
>> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
>> index 762cf83..ff84468 100644
>> --- a/drivers/mtd/nand/fsmc_nand.c
>> +++ b/drivers/mtd/nand/fsmc_nand.c
>> @@ -946,6 +946,9 @@ static int __devinit fsmc_nand_probe_config_dt(struct platform_device *pdev,
>>   	} else
>>   		pdata->rbpin.use_pin = FSMC_RB_WAIT;
>>
>> +	if (of_property_read_bool(np, "nand-sw-ecc"))
>> +		pdata->sw_ecc = true;
>> +
> of_get_nand_ecc_mode
>

Sure

-Vipin

[PATCH 11/11] nand: Increase the ecc placement locations to 640

[Vipin Kumar]

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 <vipin.kumar@st.com>
---
 include/linux/mtd/mtd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index 81d61e7..f9ac289 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -98,7 +98,7 @@ struct mtd_oob_ops {
 };
 
 #define MTD_MAX_OOBFREE_ENTRIES_LARGE	32
-#define MTD_MAX_ECCPOS_ENTRIES_LARGE	448
+#define MTD_MAX_ECCPOS_ENTRIES_LARGE	640
 /*
  * Internal ECC layout control structure. For historical reasons, there is a
  * similar, smaller struct nand_ecclayout_user (in mtd-abi.h) that is retained
-- 
1.7.11.4

[PATCH 11/11] nand: Increase the ecc placement locations to 640

[Artem Bityutskiy]

On Tue, 2012-10-09 at 16:14 +0530, Vipin Kumar wrote:
> 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 <vipin.kumar@st.com>

Pushed this one to l2-mtd.git, thanks!

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121017/508f73cf/attachment.sig>

[PATCH 11/11] nand: Increase the ecc placement locations to 640

[Brian Norris]

On Tue, Oct 9, 2012 at 3:44 AM, Vipin Kumar <vipin.kumar@st.com> wrote:
> 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.

We really need to change the nand_ecclayout struct sometime. Each one
takes something like 4+4?640+4+32?8 = 2824 bytes now, and we have 4 of
them statically declared in nand_base.c. And most drivers just declare
their own anyway. (fsl_{elbc,ifc}_nand.c have 4 and 5 of them each)

I've thought about dynamically allocating and/or changing its layout
so that we can do eccpos ranges, similar to the oobfree entries. But
this gets harder, with the old ioctl(ECCGETLAYOUT) still hanging
around, a few platform uses of nand_ecclayout, and probably some other
complications.

Anyway, this comes off as basically a complaint, while in fact, I
wanted to see if anyone else agreed and/or had any suggestions. Or,
maybe somebody could convince me not to care...

Brian

[PATCH 00/11] mtd/nand: fsmc driver updates

[Artem Bityutskiy]

On Tue, 2012-10-09 at 16:14 +0530, Vipin Kumar wrote:
> 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

Thanks!

-- 
Best Regards,
Artem Bityutskiy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121017/d73a2a7b/attachment.sig>

[PATCH 00/11] mtd/nand: fsmc driver updates

[Vipin Kumar]

On 10/17/2012 6:18 PM, Artem Bityutskiy wrote:
> On Tue, 2012-10-09 at 16:14 +0530, Vipin Kumar wrote:
>> 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
>

Sure, I would do that soon. Thanks

-Vipin