mtd: nand: gpio: make nCE GPIO optional

Submitted by Christophe Leroy on Feb. 10, 2017, 2:01 p.m.

Details

Message ID 20170210140110.87903682E7@localhost.localdomain
State Accepted
Delegated to: Boris Brezillon
Headers show

Commit Message

Christophe Leroy Feb. 10, 2017, 2:01 p.m.
On some hardware, the nCE signal is wired to the ChipSelect associated
to bus address of the NAND, so it is automatically driven during the
memory access and it is not managed by a GPIO.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/mtd/nand/gpio.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Marek Vasut Feb. 10, 2017, 3:12 p.m.
On 02/10/2017 03:01 PM, Christophe Leroy wrote:
> On some hardware,

Can you be more specific or is that confidential ? Anyway, that's just
my curiosity, what hardware is done like that.

> the nCE signal is wired to the ChipSelect associated
> to bus address of the NAND, so it is automatically driven during the
> memory access and it is not managed by a GPIO.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>

Reviewed-by: Marek Vasut <marek.vasut@gmail.com>

but please wait for the NAND experts ...

> ---
>  drivers/mtd/nand/gpio.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
> index 0d24857..85294f1 100644
> --- a/drivers/mtd/nand/gpio.c
> +++ b/drivers/mtd/nand/gpio.c
> @@ -78,7 +78,9 @@ static void gpio_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  	gpio_nand_dosync(gpiomtd);
>  
>  	if (ctrl & NAND_CTRL_CHANGE) {
> -		gpio_set_value(gpiomtd->plat.gpio_nce, !(ctrl & NAND_NCE));
> +		if (gpio_is_valid(gpiomtd->plat.gpio_nce))
> +			gpio_set_value(gpiomtd->plat.gpio_nce,
> +				       !(ctrl & NAND_NCE));
>  		gpio_set_value(gpiomtd->plat.gpio_cle, !!(ctrl & NAND_CLE));
>  		gpio_set_value(gpiomtd->plat.gpio_ale, !!(ctrl & NAND_ALE));
>  		gpio_nand_dosync(gpiomtd);
> @@ -201,7 +203,8 @@ static int gpio_nand_remove(struct platform_device *pdev)
>  
>  	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
>  		gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
> -	gpio_set_value(gpiomtd->plat.gpio_nce, 1);
> +	if (gpio_is_valid(gpiomtd->plat.gpio_nce))
> +		gpio_set_value(gpiomtd->plat.gpio_nce, 1);
>  
>  	return 0;
>  }
> @@ -239,10 +242,13 @@ static int gpio_nand_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE");
> -	if (ret)
> -		return ret;
> -	gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
> +	if (gpio_is_valid(gpiomtd->plat.gpio_nce)) {
> +		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce,
> +					"NAND NCE");
> +		if (ret)
> +			return ret;
> +		gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
> +	}
>  
>  	if (gpio_is_valid(gpiomtd->plat.gpio_nwp)) {
>  		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nwp,
>
Boris Brezillon Feb. 13, 2017, 10:30 a.m.
On Fri, 10 Feb 2017 15:01:10 +0100 (CET)
Christophe Leroy <christophe.leroy@c-s.fr> wrote:

> On some hardware, the nCE signal is wired to the ChipSelect associated
> to bus address of the NAND, so it is automatically driven during the
> memory access and it is not managed by a GPIO.

Hm, I'm not sure how this can work, because, AFAIR, the nCE line needs
to stay low for the whole CMD+ADDR[+CMD] cycle, and with your patch
it's not guaranteed.

Can you tell us more about your NAND controller?

> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  drivers/mtd/nand/gpio.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
> index 0d24857..85294f1 100644
> --- a/drivers/mtd/nand/gpio.c
> +++ b/drivers/mtd/nand/gpio.c
> @@ -78,7 +78,9 @@ static void gpio_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  	gpio_nand_dosync(gpiomtd);
>  
>  	if (ctrl & NAND_CTRL_CHANGE) {
> -		gpio_set_value(gpiomtd->plat.gpio_nce, !(ctrl & NAND_NCE));
> +		if (gpio_is_valid(gpiomtd->plat.gpio_nce))
> +			gpio_set_value(gpiomtd->plat.gpio_nce,
> +				       !(ctrl & NAND_NCE));
>  		gpio_set_value(gpiomtd->plat.gpio_cle, !!(ctrl & NAND_CLE));
>  		gpio_set_value(gpiomtd->plat.gpio_ale, !!(ctrl & NAND_ALE));
>  		gpio_nand_dosync(gpiomtd);
> @@ -201,7 +203,8 @@ static int gpio_nand_remove(struct platform_device *pdev)
>  
>  	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
>  		gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
> -	gpio_set_value(gpiomtd->plat.gpio_nce, 1);
> +	if (gpio_is_valid(gpiomtd->plat.gpio_nce))
> +		gpio_set_value(gpiomtd->plat.gpio_nce, 1);
>  
>  	return 0;
>  }
> @@ -239,10 +242,13 @@ static int gpio_nand_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE");
> -	if (ret)
> -		return ret;
> -	gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
> +	if (gpio_is_valid(gpiomtd->plat.gpio_nce)) {
> +		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce,
> +					"NAND NCE");
> +		if (ret)
> +			return ret;
> +		gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
> +	}
>  
>  	if (gpio_is_valid(gpiomtd->plat.gpio_nwp)) {
>  		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nwp,
Christophe Leroy Feb. 13, 2017, 12:58 p.m.
Le 13/02/2017 à 11:30, Boris Brezillon a écrit :
> On Fri, 10 Feb 2017 15:01:10 +0100 (CET)
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>> On some hardware, the nCE signal is wired to the ChipSelect associated
>> to bus address of the NAND, so it is automatically driven during the
>> memory access and it is not managed by a GPIO.
>
> Hm, I'm not sure how this can work, because, AFAIR, the nCE line needs
> to stay low for the whole CMD+ADDR[+CMD] cycle, and with your patch
> it's not guaranteed.

Anyway, the patch just makes it possible to register the device 
allthough nCE GPIO is not defined.
For people defining the nCE GPIO properly, the patch introduces no 
change at all.

>
> Can you tell us more about your NAND controller?

[    0.498490] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
[    0.504906] nand: Micron MT29F2G08ABAEAWP
[    0.508918] nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, 
OOB size: 64

Diagrams on datasheet shows that CE\ can optionnaly go up between each 
access. See attached exemple.



>
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>  drivers/mtd/nand/gpio.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
>> index 0d24857..85294f1 100644
>> --- a/drivers/mtd/nand/gpio.c
>> +++ b/drivers/mtd/nand/gpio.c
>> @@ -78,7 +78,9 @@ static void gpio_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>>  	gpio_nand_dosync(gpiomtd);
>>
>>  	if (ctrl & NAND_CTRL_CHANGE) {
>> -		gpio_set_value(gpiomtd->plat.gpio_nce, !(ctrl & NAND_NCE));
>> +		if (gpio_is_valid(gpiomtd->plat.gpio_nce))
>> +			gpio_set_value(gpiomtd->plat.gpio_nce,
>> +				       !(ctrl & NAND_NCE));
>>  		gpio_set_value(gpiomtd->plat.gpio_cle, !!(ctrl & NAND_CLE));
>>  		gpio_set_value(gpiomtd->plat.gpio_ale, !!(ctrl & NAND_ALE));
>>  		gpio_nand_dosync(gpiomtd);
>> @@ -201,7 +203,8 @@ static int gpio_nand_remove(struct platform_device *pdev)
>>
>>  	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
>>  		gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
>> -	gpio_set_value(gpiomtd->plat.gpio_nce, 1);
>> +	if (gpio_is_valid(gpiomtd->plat.gpio_nce))
>> +		gpio_set_value(gpiomtd->plat.gpio_nce, 1);
>>
>>  	return 0;
>>  }
>> @@ -239,10 +242,13 @@ static int gpio_nand_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>
>> -	ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE");
>> -	if (ret)
>> -		return ret;
>> -	gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
>> +	if (gpio_is_valid(gpiomtd->plat.gpio_nce)) {
>> +		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce,
>> +					"NAND NCE");
>> +		if (ret)
>> +			return ret;
>> +		gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
>> +	}
>>
>>  	if (gpio_is_valid(gpiomtd->plat.gpio_nwp)) {
>>  		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nwp,
Boris Brezillon Feb. 13, 2017, 1:22 p.m.
On Mon, 13 Feb 2017 13:58:24 +0100
Christophe LEROY <christophe.leroy@c-s.fr> wrote:

> Le 13/02/2017 à 11:30, Boris Brezillon a écrit :
> > On Fri, 10 Feb 2017 15:01:10 +0100 (CET)
> > Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >  
> >> On some hardware, the nCE signal is wired to the ChipSelect associated
> >> to bus address of the NAND, so it is automatically driven during the
> >> memory access and it is not managed by a GPIO.  
> >
> > Hm, I'm not sure how this can work, because, AFAIR, the nCE line needs
> > to stay low for the whole CMD+ADDR[+CMD] cycle, and with your patch
> > it's not guaranteed.  
> 
> Anyway, the patch just makes it possible to register the device 
> allthough nCE GPIO is not defined.
> For people defining the nCE GPIO properly, the patch introduces no 
> change at all.

Yes, I know that, just wanted to make sure that not explicitly
controlling the CE line was permitted. Modern controllers are indeed
controlling the CE line, but they usually ask you to specify the whole
CMD+ADDR[+CMD][+DATA] sequence.

> 
> >
> > Can you tell us more about your NAND controller?  
> 
> [    0.498490] nand: device found, Manufacturer ID: 0x2c, Chip ID: 0xda
> [    0.504906] nand: Micron MT29F2G08ABAEAWP
> [    0.508918] nand: 256 MiB, SLC, erase size: 128 KiB, page size: 2048, 
> OOB size: 64
> 
> Diagrams on datasheet shows that CE\ can optionnaly go up between each 
> access. See attached exemple.

I had a closer look at several datasheets (and the ONFI spec), and it
seems to be allowed as long as you follow the tCS and tCH requirements,
which I guess your controller is ensuring.

> 
> 
> 
> >  
> >>
> >> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> >> ---
> >>  drivers/mtd/nand/gpio.c | 18 ++++++++++++------
> >>  1 file changed, 12 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
> >> index 0d24857..85294f1 100644
> >> --- a/drivers/mtd/nand/gpio.c
> >> +++ b/drivers/mtd/nand/gpio.c
> >> @@ -78,7 +78,9 @@ static void gpio_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
> >>  	gpio_nand_dosync(gpiomtd);
> >>
> >>  	if (ctrl & NAND_CTRL_CHANGE) {
> >> -		gpio_set_value(gpiomtd->plat.gpio_nce, !(ctrl & NAND_NCE));
> >> +		if (gpio_is_valid(gpiomtd->plat.gpio_nce))
> >> +			gpio_set_value(gpiomtd->plat.gpio_nce,
> >> +				       !(ctrl & NAND_NCE));
> >>  		gpio_set_value(gpiomtd->plat.gpio_cle, !!(ctrl & NAND_CLE));
> >>  		gpio_set_value(gpiomtd->plat.gpio_ale, !!(ctrl & NAND_ALE));
> >>  		gpio_nand_dosync(gpiomtd);
> >> @@ -201,7 +203,8 @@ static int gpio_nand_remove(struct platform_device *pdev)
> >>
> >>  	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
> >>  		gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
> >> -	gpio_set_value(gpiomtd->plat.gpio_nce, 1);
> >> +	if (gpio_is_valid(gpiomtd->plat.gpio_nce))
> >> +		gpio_set_value(gpiomtd->plat.gpio_nce, 1);
> >>
> >>  	return 0;
> >>  }
> >> @@ -239,10 +242,13 @@ static int gpio_nand_probe(struct platform_device *pdev)
> >>  	if (ret)
> >>  		return ret;
> >>
> >> -	ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE");
> >> -	if (ret)
> >> -		return ret;
> >> -	gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
> >> +	if (gpio_is_valid(gpiomtd->plat.gpio_nce)) {
> >> +		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce,
> >> +					"NAND NCE");
> >> +		if (ret)
> >> +			return ret;
> >> +		gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
> >> +	}
> >>
> >>  	if (gpio_is_valid(gpiomtd->plat.gpio_nwp)) {
> >>  		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nwp,
Boris Brezillon March 16, 2017, 9:35 a.m.
On Fri, 10 Feb 2017 15:01:10 +0100 (CET)
Christophe Leroy <christophe.leroy@c-s.fr> wrote:

> On some hardware, the nCE signal is wired to the ChipSelect associated
> to bus address of the NAND, so it is automatically driven during the
> memory access and it is not managed by a GPIO.

Applied.

Thanks,

Boris

> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  drivers/mtd/nand/gpio.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
> index 0d24857..85294f1 100644
> --- a/drivers/mtd/nand/gpio.c
> +++ b/drivers/mtd/nand/gpio.c
> @@ -78,7 +78,9 @@ static void gpio_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  	gpio_nand_dosync(gpiomtd);
>  
>  	if (ctrl & NAND_CTRL_CHANGE) {
> -		gpio_set_value(gpiomtd->plat.gpio_nce, !(ctrl & NAND_NCE));
> +		if (gpio_is_valid(gpiomtd->plat.gpio_nce))
> +			gpio_set_value(gpiomtd->plat.gpio_nce,
> +				       !(ctrl & NAND_NCE));
>  		gpio_set_value(gpiomtd->plat.gpio_cle, !!(ctrl & NAND_CLE));
>  		gpio_set_value(gpiomtd->plat.gpio_ale, !!(ctrl & NAND_ALE));
>  		gpio_nand_dosync(gpiomtd);
> @@ -201,7 +203,8 @@ static int gpio_nand_remove(struct platform_device *pdev)
>  
>  	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
>  		gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
> -	gpio_set_value(gpiomtd->plat.gpio_nce, 1);
> +	if (gpio_is_valid(gpiomtd->plat.gpio_nce))
> +		gpio_set_value(gpiomtd->plat.gpio_nce, 1);
>  
>  	return 0;
>  }
> @@ -239,10 +242,13 @@ static int gpio_nand_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE");
> -	if (ret)
> -		return ret;
> -	gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
> +	if (gpio_is_valid(gpiomtd->plat.gpio_nce)) {
> +		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce,
> +					"NAND NCE");
> +		if (ret)
> +			return ret;
> +		gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
> +	}
>  
>  	if (gpio_is_valid(gpiomtd->plat.gpio_nwp)) {
>  		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nwp,

Patch hide | download patch | download mbox

diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c
index 0d24857..85294f1 100644
--- a/drivers/mtd/nand/gpio.c
+++ b/drivers/mtd/nand/gpio.c
@@ -78,7 +78,9 @@  static void gpio_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 	gpio_nand_dosync(gpiomtd);
 
 	if (ctrl & NAND_CTRL_CHANGE) {
-		gpio_set_value(gpiomtd->plat.gpio_nce, !(ctrl & NAND_NCE));
+		if (gpio_is_valid(gpiomtd->plat.gpio_nce))
+			gpio_set_value(gpiomtd->plat.gpio_nce,
+				       !(ctrl & NAND_NCE));
 		gpio_set_value(gpiomtd->plat.gpio_cle, !!(ctrl & NAND_CLE));
 		gpio_set_value(gpiomtd->plat.gpio_ale, !!(ctrl & NAND_ALE));
 		gpio_nand_dosync(gpiomtd);
@@ -201,7 +203,8 @@  static int gpio_nand_remove(struct platform_device *pdev)
 
 	if (gpio_is_valid(gpiomtd->plat.gpio_nwp))
 		gpio_set_value(gpiomtd->plat.gpio_nwp, 0);
-	gpio_set_value(gpiomtd->plat.gpio_nce, 1);
+	if (gpio_is_valid(gpiomtd->plat.gpio_nce))
+		gpio_set_value(gpiomtd->plat.gpio_nce, 1);
 
 	return 0;
 }
@@ -239,10 +242,13 @@  static int gpio_nand_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE");
-	if (ret)
-		return ret;
-	gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
+	if (gpio_is_valid(gpiomtd->plat.gpio_nce)) {
+		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce,
+					"NAND NCE");
+		if (ret)
+			return ret;
+		gpio_direction_output(gpiomtd->plat.gpio_nce, 1);
+	}
 
 	if (gpio_is_valid(gpiomtd->plat.gpio_nwp)) {
 		ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nwp,