* Re: [PATCH] mtd: nand: gpio: make nCE GPIO optional
2017-02-10 14:01 [PATCH] mtd: nand: gpio: make nCE GPIO optional Christophe Leroy
@ 2017-02-10 15:12 ` Marek Vasut
2017-02-13 10:30 ` Boris Brezillon
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2017-02-10 15:12 UTC (permalink / raw)
To: Christophe Leroy, Boris Brezillon, Richard Weinberger,
David Woodhouse, Brian Norris, Cyrille Pitchen
Cc: linux-mtd, linux-kernel
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,
>
--
Best regards,
Marek Vasut
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mtd: nand: gpio: make nCE GPIO optional
2017-02-10 14:01 [PATCH] mtd: nand: gpio: make nCE GPIO optional Christophe Leroy
2017-02-10 15:12 ` Marek Vasut
@ 2017-02-13 10:30 ` Boris Brezillon
2017-02-13 12:58 ` Christophe LEROY
2017-03-16 9:35 ` Boris Brezillon
2017-05-01 21:46 ` Brian Norris
3 siblings, 1 reply; 10+ messages in thread
From: Boris Brezillon @ 2017-02-13 10:30 UTC (permalink / raw)
To: Christophe Leroy
Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut,
Cyrille Pitchen, linux-mtd, linux-kernel
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,
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mtd: nand: gpio: make nCE GPIO optional
2017-02-13 10:30 ` Boris Brezillon
@ 2017-02-13 12:58 ` Christophe LEROY
2017-02-13 13:22 ` Boris Brezillon
0 siblings, 1 reply; 10+ messages in thread
From: Christophe LEROY @ 2017-02-13 12:58 UTC (permalink / raw)
To: Boris Brezillon
Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut,
Cyrille Pitchen, linux-mtd, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2930 bytes --]
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,
[-- Attachment #2: read_status_cycle.png --]
[-- Type: image/png, Size: 21577 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mtd: nand: gpio: make nCE GPIO optional
2017-02-13 12:58 ` Christophe LEROY
@ 2017-02-13 13:22 ` Boris Brezillon
0 siblings, 0 replies; 10+ messages in thread
From: Boris Brezillon @ 2017-02-13 13:22 UTC (permalink / raw)
To: Christophe LEROY
Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut,
Cyrille Pitchen, linux-mtd, linux-kernel
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,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mtd: nand: gpio: make nCE GPIO optional
2017-02-10 14:01 [PATCH] mtd: nand: gpio: make nCE GPIO optional Christophe Leroy
2017-02-10 15:12 ` Marek Vasut
2017-02-13 10:30 ` Boris Brezillon
@ 2017-03-16 9:35 ` Boris Brezillon
2017-05-01 21:46 ` Brian Norris
3 siblings, 0 replies; 10+ messages in thread
From: Boris Brezillon @ 2017-03-16 9:35 UTC (permalink / raw)
To: Christophe Leroy
Cc: Richard Weinberger, David Woodhouse, Brian Norris, Marek Vasut,
Cyrille Pitchen, linux-mtd, linux-kernel
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,
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mtd: nand: gpio: make nCE GPIO optional
2017-02-10 14:01 [PATCH] mtd: nand: gpio: make nCE GPIO optional Christophe Leroy
` (2 preceding siblings ...)
2017-03-16 9:35 ` Boris Brezillon
@ 2017-05-01 21:46 ` Brian Norris
2017-05-02 5:47 ` Christophe LEROY
3 siblings, 1 reply; 10+ messages in thread
From: Brian Norris @ 2017-05-01 21:46 UTC (permalink / raw)
To: Christophe Leroy
Cc: Boris Brezillon, Richard Weinberger, David Woodhouse, Marek Vasut,
Cyrille Pitchen, linux-mtd, linux-kernel
On Fri, Feb 10, 2017 at 03:01:10PM +0100, Christophe Leroy 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.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Not really a problem with this patch exactly, but FYI you're only making
this optional for the non-DT case. For device tree, this is kinda hard
to do, since the current binding suggests we retrieve the GPIOs based on
index position, not by name. So if you leave one off...I guess we well
just be off-by-1 on the indeces until we hit a non-optional one...which
I guess is "CLE".
If we wanted this to work for DT, we'd need to extend this driver (and
binding doc) to support requesting GPIOs by name.
Brian
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mtd: nand: gpio: make nCE GPIO optional
2017-05-01 21:46 ` Brian Norris
@ 2017-05-02 5:47 ` Christophe LEROY
2017-05-02 9:03 ` Boris Brezillon
0 siblings, 1 reply; 10+ messages in thread
From: Christophe LEROY @ 2017-05-02 5:47 UTC (permalink / raw)
To: Brian Norris
Cc: Boris Brezillon, Richard Weinberger, David Woodhouse, Marek Vasut,
Cyrille Pitchen, linux-mtd, linux-kernel
Le 01/05/2017 à 23:46, Brian Norris a écrit :
> On Fri, Feb 10, 2017 at 03:01:10PM +0100, Christophe Leroy 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.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>
> Not really a problem with this patch exactly, but FYI you're only making
> this optional for the non-DT case. For device tree, this is kinda hard
> to do, since the current binding suggests we retrieve the GPIOs based on
> index position, not by name. So if you leave one off...I guess we well
> just be off-by-1 on the indeces until we hit a non-optional one...which
> I guess is "CLE".
>
> If we wanted this to work for DT, we'd need to extend this driver (and
> binding doc) to support requesting GPIOs by name.
>
It works for me with devicetree.
I have the following definition in my DT:
nand@1,0 {
compatible = "gpio-control-nand";
reg = <1 0x0 0x01>;
#address-cells = <1>;
#size-cells = <1>;
gpios = <&qe_pio_c 24 1 // RDY
0 // nCE
&qe_pio_c 26 1 // ALE
&qe_pio_c 25 1 // CLE
0>; // nwp
};
Christophe
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mtd: nand: gpio: make nCE GPIO optional
2017-05-02 5:47 ` Christophe LEROY
@ 2017-05-02 9:03 ` Boris Brezillon
2017-05-02 16:15 ` Brian Norris
0 siblings, 1 reply; 10+ messages in thread
From: Boris Brezillon @ 2017-05-02 9:03 UTC (permalink / raw)
To: Christophe LEROY
Cc: Brian Norris, Richard Weinberger, David Woodhouse, Marek Vasut,
Cyrille Pitchen, linux-mtd, linux-kernel
On Tue, 2 May 2017 07:47:40 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> Le 01/05/2017 à 23:46, Brian Norris a écrit :
> > On Fri, Feb 10, 2017 at 03:01:10PM +0100, Christophe Leroy 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.
> >>
> >> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> >
> > Not really a problem with this patch exactly, but FYI you're only making
> > this optional for the non-DT case. For device tree, this is kinda hard
> > to do, since the current binding suggests we retrieve the GPIOs based on
> > index position, not by name. So if you leave one off...I guess we well
> > just be off-by-1 on the indeces until we hit a non-optional one...which
> > I guess is "CLE".
> >
> > If we wanted this to work for DT, we'd need to extend this driver (and
> > binding doc) to support requesting GPIOs by name.
> >
>
> It works for me with devicetree.
>
> I have the following definition in my DT:
>
> nand@1,0 {
> compatible = "gpio-control-nand";
> reg = <1 0x0 0x01>;
> #address-cells = <1>;
> #size-cells = <1>;
> gpios = <&qe_pio_c 24 1 // RDY
> 0 // nCE
> &qe_pio_c 26 1 // ALE
> &qe_pio_c 25 1 // CLE
> 0>; // nwp
> };
>
Yep, it's perfectly fine to have 'empty' gpio entries (entries with
phandle set to 0/NULL), we're using this trick in the atmel_nand
driver as well.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] mtd: nand: gpio: make nCE GPIO optional
2017-05-02 9:03 ` Boris Brezillon
@ 2017-05-02 16:15 ` Brian Norris
0 siblings, 0 replies; 10+ messages in thread
From: Brian Norris @ 2017-05-02 16:15 UTC (permalink / raw)
To: Boris Brezillon
Cc: Christophe LEROY, Richard Weinberger, David Woodhouse,
Marek Vasut, Cyrille Pitchen, linux-mtd, linux-kernel
Hi Christophe, Boris,
On Tue, May 02, 2017 at 11:03:34AM +0200, Boris Brezillon wrote:
> On Tue, 2 May 2017 07:47:40 +0200
> Christophe LEROY <christophe.leroy@c-s.fr> wrote:
>
> > Le 01/05/2017 à 23:46, Brian Norris a écrit :
> > > On Fri, Feb 10, 2017 at 03:01:10PM +0100, Christophe Leroy 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.
> > >>
> > >> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > >
> > > Not really a problem with this patch exactly, but FYI you're only making
> > > this optional for the non-DT case. For device tree, this is kinda hard
> > > to do, since the current binding suggests we retrieve the GPIOs based on
> > > index position, not by name. So if you leave one off...I guess we well
> > > just be off-by-1 on the indeces until we hit a non-optional one...which
> > > I guess is "CLE".
> > >
> > > If we wanted this to work for DT, we'd need to extend this driver (and
> > > binding doc) to support requesting GPIOs by name.
> > >
> >
> > It works for me with devicetree.
> >
> > I have the following definition in my DT:
> >
> > nand@1,0 {
> > compatible = "gpio-control-nand";
> > reg = <1 0x0 0x01>;
> > #address-cells = <1>;
> > #size-cells = <1>;
> > gpios = <&qe_pio_c 24 1 // RDY
> > 0 // nCE
> > &qe_pio_c 26 1 // ALE
> > &qe_pio_c 25 1 // CLE
> > 0>; // nwp
> > };
> >
>
> Yep, it's perfectly fine to have 'empty' gpio entries (entries with
> phandle set to 0/NULL), we're using this trick in the atmel_nand
> driver as well.
I wasn't aware. In that case, you need to change the binding doc to
note that nCE is optional now.
Brian
^ permalink raw reply [flat|nested] 10+ messages in thread
![[-- Attachment #2: read_status_cycle.png --] [-- Type: image/png, Size: 21577 bytes --]](../../9cadc95b-dc67-9756-c244-9ce452ebeaf7@c-s.fr/2-read_status_cycle.png){kind=link}