[PATCH] ARM: OMAP2+: gpmc: enable wait-pin monitoring for NAND devices via DT

[PATCH] ARM: OMAP2+: gpmc: enable wait-pin monitoring for NAND devices via DT

[Pekon Gupta]

This patch enables 'wait-pin' monitoring in NAND driver if following properties
are present under NAND DT node
	gpmc,wait-pin = <wait-pin number>
	gpmc,wait-on-read;
	gpmc,wait-on-write;
As NAND generic framework uses common path nand_chip->dev_ready() for monitoring
completion of Read and Write status, so wait-pin monitoring is enabled only when
both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' are specified.

Signed-off-by: Pekon Gupta <pekon@ti.com>
---
 arch/arm/mach-omap2/gpmc-nand.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index 4349e82..7dc742d 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -123,11 +123,13 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
 		}
 	}
 
-	if (gpmc_nand_data->of_node)
+	if (gpmc_nand_data->of_node) {
 		gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
-	else
+		if (s.wait_on_read && s.wait_on_write)
+			gpmc_nand_data->dev_ready = "true";
+	} else {
 		gpmc_set_legacy(gpmc_nand_data, &s);
-
+	}
 	s.device_nand = true;
 
 	err = gpmc_cs_program_settings(gpmc_nand_data->cs, &s);
-- 
1.8.5.1.163.gd7aced9

Re: [PATCH] ARM: OMAP2+: gpmc: enable wait-pin monitoring for NAND devices via DT

[Javier Martinez Canillas]

Hello Pekon,

On Fri, May 16, 2014 at 9:07 PM, Pekon Gupta <pekon@ti.com> wrote:
> This patch enables 'wait-pin' monitoring in NAND driver if following properties
> are present under NAND DT node
>         gpmc,wait-pin = <wait-pin number>
>         gpmc,wait-on-read;
>         gpmc,wait-on-write;
> As NAND generic framework uses common path nand_chip->dev_ready() for monitoring
> completion of Read and Write status, so wait-pin monitoring is enabled only when
> both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' are specified.
>

I see that the GPMC driver checks if "gpmc,wait-pin" property is
defined and only in that case tries to read both "gpmc,wait-on-read"
and "gpmc,wait-on-write" and prints a "read/write wait monitoring not
enabled!" warning if one of those is not defined.

So my question is, it's a requirement and these 3 properties need to
always be defined together?  If that is the case then I wonder why
there are 3 different properties and why not just defining
"gpmc,wait-pin" implies "gpmc,wait-on-{read,write}" ?

Or if is valid to define "gpmc-wait-pin" without
"gpmc-wait-on-{read,write}" or only one those then why that scary
warning is printed?

Whatever is the case I think that the GPMC DT binding documentation
(Documentation/devicetree/bindings/bus/ti-gpmc.txt) should be more
clear on this regard.

> Signed-off-by: Pekon Gupta <pekon@ti.com>
> ---
>  arch/arm/mach-omap2/gpmc-nand.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
> index 4349e82..7dc742d 100644
> --- a/arch/arm/mach-omap2/gpmc-nand.c
> +++ b/arch/arm/mach-omap2/gpmc-nand.c
> @@ -123,11 +123,13 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
>                 }
>         }
>
> -       if (gpmc_nand_data->of_node)
> +       if (gpmc_nand_data->of_node) {
>                 gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
> -       else
> +               if (s.wait_on_read && s.wait_on_write)

You said that wait-on-pin monitoring is only enabled if both
"gpmc,wait-on-read" and "gpmc,wait-on-write" are specified. But in
that case I think we should clear the wait_pin on
gpmc_read_settings_dt() if either "gpmc,wait-on-read" or
"gpmc,wait-on-write" were not specified and check s.wait_pin instead.

Or is this semantic only for NAND and other devices connected to the
GPMC behave differently wrt "gpmc,wait-pin"?

> +                       gpmc_nand_data->dev_ready = "true";

You should really use gpmc_nand_data->dev_ready = true here. The C99
standard allows you to assign a string literal to a _Bool type and
will be evaluated to 1 but that is confusing and I haven't seen that
used in other part of the kernel.

> +       } else {
>                 gpmc_set_legacy(gpmc_nand_data, &s);
> -
> +       }
>         s.device_nand = true;
>
>         err = gpmc_cs_program_settings(gpmc_nand_data->cs, &s);
> --
> 1.8.5.1.163.gd7aced9
>

Best regards,
Javier

RE: [PATCH] ARM: OMAP2+: gpmc: enable wait-pin monitoring for NAND devices via DT

[Gupta, Pekon]

Hi Javier,

>From: Javier Martinez Canillas [mailto:javier@dowhile0.org]
>
>Hello Pekon,
>
>On Fri, May 16, 2014 at 9:07 PM, Pekon Gupta <pekon@ti.com> wrote:
>> This patch enables 'wait-pin' monitoring in NAND driver if following properties
>> are present under NAND DT node
>>         gpmc,wait-pin = <wait-pin number>
>>         gpmc,wait-on-read;
>>         gpmc,wait-on-write;
>> As NAND generic framework uses common path nand_chip->dev_ready() for monitoring
>> completion of Read and Write status, so wait-pin monitoring is enabled only when
>> both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' are specified.
>>
>
>I see that the GPMC driver checks if "gpmc,wait-pin" property is
>defined and only in that case tries to read both "gpmc,wait-on-read"
>and "gpmc,wait-on-write" and prints a "read/write wait monitoring not
>enabled!" warning if one of those is not defined.
>
>So my question is, it's a requirement and these 3 properties need to
>always be defined together?  If that is the case then I wonder why
>there are 3 different properties and why not just defining
>"gpmc,wait-pin" implies "gpmc,wait-on-{read,write}" ?
>
GPMC as a hardware engine allows selection of wait-pin independently
for both Read and Write paths, but NAND generic framework uses single
common function nand_chip->dev_ready() which is used for both
Read and Write paths to poll wait-pin. 
So, in case of NAND both 'gpmc,wait-on-read' and 'gpmc,wait-on-write'
should be simultaneously defined to enable wait-pin monitoring. 

But GPMC being generic hardware engine for NOR, OneNAND and other
parallel interfaces like Camera, Ethernet the two separate bindings allow
flexibility to use wait-pin monitoring for only one of the paths {Read | Write}.

Therefore this check is added in gpmc_nand_init(), and not in gpmc_read_settings_dt()
which is common for all type of GPMC devices (NAND, NOR, .. )


>Or if is valid to define "gpmc-wait-pin" without
>"gpmc-wait-on-{read,write}" or only one those then why that scary
>warning is printed?
>
>Whatever is the case I think that the GPMC DT binding documentation
>(Documentation/devicetree/bindings/bus/ti-gpmc.txt) should be more
>clear on this regard.
>
Agree. But I think this clarification fits better in NAND specific documentation
Documentation/devicetree/bindings/mtd/gpmc-nand.txt


>> Signed-off-by: Pekon Gupta <pekon@ti.com>
>> ---
>>  arch/arm/mach-omap2/gpmc-nand.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
>> index 4349e82..7dc742d 100644
>> --- a/arch/arm/mach-omap2/gpmc-nand.c
>> +++ b/arch/arm/mach-omap2/gpmc-nand.c
>> @@ -123,11 +123,13 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
>>                 }
>>         }
>>
>> -       if (gpmc_nand_data->of_node)
>> +       if (gpmc_nand_data->of_node) {
>>                 gpmc_read_settings_dt(gpmc_nand_data->of_node, &s);
>> -       else
>> +               if (s.wait_on_read && s.wait_on_write)
>
>You said that wait-on-pin monitoring is only enabled if both
>"gpmc,wait-on-read" and "gpmc,wait-on-write" are specified. But in
>that case I think we should clear the wait_pin on
>gpmc_read_settings_dt() if either "gpmc,wait-on-read" or
>"gpmc,wait-on-write" were not specified and check s.wait_pin instead.
>
>Or is this semantic only for NAND and other devices connected to the
>GPMC behave differently wrt "gpmc,wait-pin"?
>
True, this is only for NAND. Other device drivers may use wait-pin
either 'only for Read' or 'only for Write' access. However I doubt why
anyone would do so, unless there is some design bug | constrain.


>> +                       gpmc_nand_data->dev_ready = "true";
>
>You should really use gpmc_nand_data->dev_ready = true here. The C99
>standard allows you to assign a string literal to a _Bool type and
>will be evaluated to 1 but that is confusing and I haven't seen that
>used in other part of the kernel.
>
Sorry, my bad. I over looked it.
I'll fix it in next version along with Documentation update.


With regards, pekon

Re: [PATCH] ARM: OMAP2+: gpmc: enable wait-pin monitoring for NAND devices via DT

[Roger Quadros]

On 05/19/2014 09:26 PM, Gupta, Pekon wrote:
> Hi Javier,
> 
>> From: Javier Martinez Canillas [mailto:javier@dowhile0.org]
>>
>> Hello Pekon,
>>
>> On Fri, May 16, 2014 at 9:07 PM, Pekon Gupta <pekon@ti.com> wrote:
>>> This patch enables 'wait-pin' monitoring in NAND driver if following properties
>>> are present under NAND DT node
>>>         gpmc,wait-pin = <wait-pin number>
>>>         gpmc,wait-on-read;
>>>         gpmc,wait-on-write;
>>> As NAND generic framework uses common path nand_chip->dev_ready() for monitoring
>>> completion of Read and Write status, so wait-pin monitoring is enabled only when
>>> both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' are specified.
>>>
>>
>> I see that the GPMC driver checks if "gpmc,wait-pin" property is
>> defined and only in that case tries to read both "gpmc,wait-on-read"
>> and "gpmc,wait-on-write" and prints a "read/write wait monitoring not
>> enabled!" warning if one of those is not defined.
>>
>> So my question is, it's a requirement and these 3 properties need to
>> always be defined together?  If that is the case then I wonder why
>> there are 3 different properties and why not just defining
>> "gpmc,wait-pin" implies "gpmc,wait-on-{read,write}" ?
>>
> GPMC as a hardware engine allows selection of wait-pin independently
> for both Read and Write paths, but NAND generic framework uses single
> common function nand_chip->dev_ready() which is used for both
> Read and Write paths to poll wait-pin. 
> So, in case of NAND both 'gpmc,wait-on-read' and 'gpmc,wait-on-write'
> should be simultaneously defined to enable wait-pin monitoring. 
> 
> But GPMC being generic hardware engine for NOR, OneNAND and other
> parallel interfaces like Camera, Ethernet the two separate bindings allow
> flexibility to use wait-pin monitoring for only one of the paths {Read | Write}.
> 
> Therefore this check is added in gpmc_nand_init(), and not in gpmc_read_settings_dt()
> which is common for all type of GPMC devices (NAND, NOR, .. )

The behavior of wait pin is slightly different when it is a NAND device when compared to other NOR like devices. On NAND, the pin is not used for inserting wait states in the bus access cycle, but instead it is used for waiting for the device to be ready after a particular command. So this wait logic must be implemented in the NAND driver software. GPMC will only forward the wait pin state via a status register, or at best give you an interrupt.

For NAND use case, the GPMC hardware WAIT pin monitoring logic needs to be disabled (see NOTE in section 10.1.5.14.2 NAND Device-Ready Pin). The NAND driver only needs to know which wait pin the NAND chips device ready is connected to so that it can monitor it via the GPMC.STATUS register. The current omap_dev_ready() is so fragile that it will break if NAND device is not on CS0.

cheers,
-roger