[U-Boot] [PATCH] mtd: nand: omap_gpmc: Make ready/busy pins configurable

[U-Boot] [PATCH] mtd: nand: omap_gpmc: Make ready/busy pins configurable

[Michal Sojka]

Commit fb384c4720ca7496775d6578f184bf628db73456 introduced the use of
WAIT0 pin for determining whether the NAND is ready or not. This only
works if all NAND chips are connected to WAIT0. If some chips are
connected to the other available pin WAIT1, nand_wait() does not really
wait and prints a WARN_ON message.

This patch allows the board to provide configuration of which chip is
connected to which WAITx signal. For example, one can define in
include/configs/foo.h:

    #define CONFIG_NAND_OMAP_GPMC_WSCFG     0,0,1,1

This would mean that chips using to CS0 and 1 are connected to WAIT0 and
chips with CS2 and 3 are connected to WAIT1.

Signed-off-by: Michal Sojka <sojka@merica.cz>

Cc: Tom Rini <trini@ti.com>
Cc: Stefan Roese <sr@denx.de>
---
 drivers/mtd/nand/omap_gpmc.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
index 459904d..57fc7b9 100644
--- a/drivers/mtd/nand/omap_gpmc.c
+++ b/drivers/mtd/nand/omap_gpmc.c
@@ -36,7 +36,8 @@ static __maybe_unused struct nand_ecclayout omap_ecclayout;
 struct omap_nand_info {
 	struct bch_control *control;
 	enum omap_ecc ecc_scheme;
-	int cs;
+	uint8_t cs;
+	uint8_t ws;		/* wait status pin (0,1) */
 };
 
 /* We are wasting a bit of memory but al least we are safe */
@@ -76,7 +77,9 @@ static void omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd,
 /* Check wait pin as dev ready indicator */
 static int omap_dev_ready(struct mtd_info *mtd)
 {
-	return gpmc_cfg->status & (1 << 8);
+	register struct nand_chip *this = mtd->priv;
+	struct omap_nand_info *info = this->priv;
+	return gpmc_cfg->status & (1 << (8 + info->ws));
 }
 
 /*
@@ -853,6 +856,12 @@ int board_nand_init(struct nand_chip *nand)
 	nand->IO_ADDR_W = (void __iomem *)&gpmc_cfg->cs[cs].nand_cmd;
 	omap_nand_info[cs].control = NULL;
 	omap_nand_info[cs].cs = cs;
+#if !defined(CONFIG_NAND_OMAP_GPMC_WSCFG)
+	omap_nand_info[cs].ws = 0;
+#else
+	int8_t ws[CONFIG_SYS_MAX_NAND_DEVICE] = { CONFIG_NAND_OMAP_GPMC_WSCFG };
+	omap_nand_info[cs].ws = ws[cs];
+#endif
 	nand->priv	= &omap_nand_info[cs];
 	nand->cmd_ctrl	= omap_nand_hwcontrol;
 	nand->options	|= NAND_NO_PADDING | NAND_CACHEPRG;
-- 
2.1.4

[U-Boot] [PATCH] mtd: nand: omap_gpmc: Make ready/busy pins configurable

[Stefan Roese]

On 10.02.2015 14:21, Michal Sojka wrote:
> Commit fb384c4720ca7496775d6578f184bf628db73456 introduced the use of
> WAIT0 pin for determining whether the NAND is ready or not. This only
> works if all NAND chips are connected to WAIT0. If some chips are
> connected to the other available pin WAIT1, nand_wait() does not really
> wait and prints a WARN_ON message.
>
> This patch allows the board to provide configuration of which chip is
> connected to which WAITx signal. For example, one can define in
> include/configs/foo.h:
>
>      #define CONFIG_NAND_OMAP_GPMC_WSCFG     0,0,1,1
>
> This would mean that chips using to CS0 and 1 are connected to WAIT0 and
> chips with CS2 and 3 are connected to WAIT1.
>
> Signed-off-by: Michal Sojka <sojka@merica.cz>
>
> Cc: Tom Rini <trini@ti.com>
> Cc: Stefan Roese <sr@denx.de>
> ---
>   drivers/mtd/nand/omap_gpmc.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
> index 459904d..57fc7b9 100644
> --- a/drivers/mtd/nand/omap_gpmc.c
> +++ b/drivers/mtd/nand/omap_gpmc.c
> @@ -36,7 +36,8 @@ static __maybe_unused struct nand_ecclayout omap_ecclayout;
>   struct omap_nand_info {
>   	struct bch_control *control;
>   	enum omap_ecc ecc_scheme;
> -	int cs;
> +	uint8_t cs;
> +	uint8_t ws;		/* wait status pin (0,1) */
>   };
>
>   /* We are wasting a bit of memory but al least we are safe */
> @@ -76,7 +77,9 @@ static void omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd,
>   /* Check wait pin as dev ready indicator */
>   static int omap_dev_ready(struct mtd_info *mtd)
>   {
> -	return gpmc_cfg->status & (1 << 8);
> +	register struct nand_chip *this = mtd->priv;
> +	struct omap_nand_info *info = this->priv;
> +	return gpmc_cfg->status & (1 << (8 + info->ws));
>   }
>
>   /*
> @@ -853,6 +856,12 @@ int board_nand_init(struct nand_chip *nand)
>   	nand->IO_ADDR_W = (void __iomem *)&gpmc_cfg->cs[cs].nand_cmd;
>   	omap_nand_info[cs].control = NULL;
>   	omap_nand_info[cs].cs = cs;
> +#if !defined(CONFIG_NAND_OMAP_GPMC_WSCFG)
> +	omap_nand_info[cs].ws = 0;
> +#else
> +	int8_t ws[CONFIG_SYS_MAX_NAND_DEVICE] = { CONFIG_NAND_OMAP_GPMC_WSCFG };

Doesn't this declaration trigger a compilation warning?

> +	omap_nand_info[cs].ws = ws[cs];
> +#endif

I've attached a new version of this patch. It removed the ifdef from the 
code. Please take a look at it and let me know what you think.

Thanks,
Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-mtd-nand-omap_gpmc-Make-ready-busy-pins-configurable.patch
Type: text/x-diff
Size: 2666 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150211/a0390e77/attachment.patch>

[U-Boot] [PATCH] mtd: nand: omap_gpmc: Make ready/busy pins configurable

[Michal Sojka]

Hi Stefan,

On Wed, Feb 11 2015, Stefan Roese wrote:
> On 10.02.2015 14:21, Michal Sojka wrote:
>> Commit fb384c4720ca7496775d6578f184bf628db73456 introduced the use of
>> WAIT0 pin for determining whether the NAND is ready or not. This only
>> works if all NAND chips are connected to WAIT0. If some chips are
>> connected to the other available pin WAIT1, nand_wait() does not really
>> wait and prints a WARN_ON message.
>>
>> This patch allows the board to provide configuration of which chip is
>> connected to which WAITx signal. For example, one can define in
>> include/configs/foo.h:
>>
>>      #define CONFIG_NAND_OMAP_GPMC_WSCFG     0,0,1,1
>>
>> This would mean that chips using to CS0 and 1 are connected to WAIT0 and
>> chips with CS2 and 3 are connected to WAIT1.
>>
>> Signed-off-by: Michal Sojka <sojka@merica.cz>
>>
>> Cc: Tom Rini <trini@ti.com>
>> Cc: Stefan Roese <sr@denx.de>
>> ---
>>   drivers/mtd/nand/omap_gpmc.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
>> index 459904d..57fc7b9 100644
>> --- a/drivers/mtd/nand/omap_gpmc.c
>> +++ b/drivers/mtd/nand/omap_gpmc.c
>> @@ -36,7 +36,8 @@ static __maybe_unused struct nand_ecclayout omap_ecclayout;
>>   struct omap_nand_info {
>>   	struct bch_control *control;
>>   	enum omap_ecc ecc_scheme;
>> -	int cs;
>> +	uint8_t cs;
>> +	uint8_t ws;		/* wait status pin (0,1) */
>>   };
>>
>>   /* We are wasting a bit of memory but al least we are safe */
>> @@ -76,7 +77,9 @@ static void omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd,
>>   /* Check wait pin as dev ready indicator */
>>   static int omap_dev_ready(struct mtd_info *mtd)
>>   {
>> -	return gpmc_cfg->status & (1 << 8);
>> +	register struct nand_chip *this = mtd->priv;
>> +	struct omap_nand_info *info = this->priv;
>> +	return gpmc_cfg->status & (1 << (8 + info->ws));
>>   }
>>
>>   /*
>> @@ -853,6 +856,12 @@ int board_nand_init(struct nand_chip *nand)
>>   	nand->IO_ADDR_W = (void __iomem *)&gpmc_cfg->cs[cs].nand_cmd;
>>   	omap_nand_info[cs].control = NULL;
>>   	omap_nand_info[cs].cs = cs;
>> +#if !defined(CONFIG_NAND_OMAP_GPMC_WSCFG)
>> +	omap_nand_info[cs].ws = 0;
>> +#else
>> +	int8_t ws[CONFIG_SYS_MAX_NAND_DEVICE] = { CONFIG_NAND_OMAP_GPMC_WSCFG };
>
> Doesn't this declaration trigger a compilation warning?

Not with gcc-4.8.3 that we use.

>> +	omap_nand_info[cs].ws = ws[cs];
>> +#endif
>
> I've attached a new version of this patch. It removed the ifdef from the
> code. Please take a look at it and let me know what you think.

Yes, it's better and it works for us. Just one point - what about making
the variable const as in the patch below?

Thanks,
-Michal

-------------8<------------------
Commit fb384c4720ca7496775d6578f184bf628db73456 introduced the use of
WAIT0 pin for determining whether the NAND is ready or not. This only
works if all NAND chips are connected to WAIT0. If some chips are
connected to the other available pin WAIT1, nand_wait() does not really
wait and prints a WARN_ON message.

This patch allows the board to provide configuration of which chip is
connected to which WAITx signal. For example, one can define in
include/configs/foo.h:

    #define CONFIG_NAND_OMAP_GPMC_WSCFG     0,0,1,1

This would mean that chips using to CS0 and 1 are connected to WAIT0 and
chips with CS2 and 3 are connected to WAIT1.

Signed-off-by: Michal Sojka <sojka@merica.cz>

Cc: Tom Rini <trini@ti.com>
Cc: Stefan Roese <sr@denx.de>
---
 drivers/mtd/nand/omap_gpmc.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
index 459904d..4f9120d 100644
--- a/drivers/mtd/nand/omap_gpmc.c
+++ b/drivers/mtd/nand/omap_gpmc.c
@@ -30,13 +30,22 @@ static u8  bch8_polynomial[] = {0xef, 0x51, 0x2e, 0x09, 0xed, 0x93, 0x9a, 0xc2,
 static uint8_t cs_next;
 static __maybe_unused struct nand_ecclayout omap_ecclayout;
 
+#if defined(CONFIG_NAND_OMAP_GPMC_WSCFG)
+static const int8_t wscfg[CONFIG_SYS_MAX_NAND_DEVICE] =
+	{ CONFIG_NAND_OMAP_GPMC_WSCFG };
+#else
+/* wscfg is preset to zero since its a static variable */
+static const int8_t wscfg[CONFIG_SYS_MAX_NAND_DEVICE];
+#endif
+
 /*
  * Driver configurations
  */
 struct omap_nand_info {
 	struct bch_control *control;
 	enum omap_ecc ecc_scheme;
-	int cs;
+	uint8_t cs;
+	uint8_t ws;		/* wait status pin (0,1) */
 };
 
 /* We are wasting a bit of memory but al least we are safe */
@@ -76,7 +85,9 @@ static void omap_nand_hwcontrol(struct mtd_info *mtd, int32_t cmd,
 /* Check wait pin as dev ready indicator */
 static int omap_dev_ready(struct mtd_info *mtd)
 {
-	return gpmc_cfg->status & (1 << 8);
+	register struct nand_chip *this = mtd->priv;
+	struct omap_nand_info *info = this->priv;
+	return gpmc_cfg->status & (1 << (8 + info->ws));
 }
 
 /*
@@ -853,6 +864,7 @@ int board_nand_init(struct nand_chip *nand)
 	nand->IO_ADDR_W = (void __iomem *)&gpmc_cfg->cs[cs].nand_cmd;
 	omap_nand_info[cs].control = NULL;
 	omap_nand_info[cs].cs = cs;
+	omap_nand_info[cs].ws = wscfg[cs];
 	nand->priv	= &omap_nand_info[cs];
 	nand->cmd_ctrl	= omap_nand_hwcontrol;
 	nand->options	|= NAND_NO_PADDING | NAND_CACHEPRG;
-- 
2.1.4

[U-Boot] [PATCH] mtd: nand: omap_gpmc: Make ready/busy pins configurable

[Stefan Roese]

Hi Michal,

On 17.02.2015 14:52, Michal Sojka wrote:

<snip>

>>> +	omap_nand_info[cs].ws = ws[cs];
>>> +#endif
>>
>> I've attached a new version of this patch. It removed the ifdef from the
>> code. Please take a look at it and let me know what you think.
>
> Yes, it's better and it works for us. Just one point - what about making
> the variable const as in the patch below?

Even better. Please submit it as a regular patch (v2) and my:

Acked-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan