[PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[B. Niedermayr]

From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>

Add a new dt-binding for the wait-pin-polarity property

Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
---
 .../bindings/memory-controllers/ti,gpmc-child.yaml         | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
index 6e3995bb1630..7c721206f10b 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
@@ -230,6 +230,13 @@ properties:
       Wait-pin used by client. Must be less than "gpmc,num-waitpins".
     $ref: /schemas/types.yaml#/definitions/uint32
 
+  gpmc,wait-pin-polarity:
+    description: |
+      Wait-pin polarity used by the clien. It relates to the pin defined
+      with "gpmc,wait-pin".
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0
+
   gpmc,wait-on-read:
     description: Enables wait monitoring on reads.
     type: boolean
-- 
2.25.1

[PATCH v2 0/3] omap-gpmc wait pin additions

[B. Niedermayr]

From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>

There is currently no possibility for the gpmc to set either the
waitp-pin polarity or use the same wait-pin for different cs-regions.

While the current implementation may fullfill most usecases, it may not
be sufficient for more complex setups (e.g. FPGA/ASIC interfaces), where
more complex interfacing options where possible.

For example interfacing an ASIC which offers multiple cs-regions but
only one waitpin the current driver and dt-bindings are not sufficient.

While using the same waitpin for different cs-regions worked for older
kernels (4.14) the omap-gpmc.c driver refused to probe (-EBUSY) with
newer kernels (>5.10).

Changes since v1: 
  * Rebase against recent 6.0.0-rc3 kernel, but the maintainers list
    stays the same!
  * Updated eMail recipients list


Benedikt Niedermayr (3):
  memory: omap-gpmc: allow shared wait pins
  memory: omap-gpmc: add support for wait pin polarity
  dt-bindings: memory-controllers: gpmc-child: Add binding for
    wait-pin-polarity

 .../memory-controllers/ti,gpmc-child.yaml     |  7 ++++
 drivers/memory/omap-gpmc.c                    | 38 +++++++++++++++----
 include/linux/platform_data/gpmc-omap.h       |  1 +
 3 files changed, 39 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] memory: omap-gpmc: allow shared wait pins

[B. Niedermayr]

From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>

Newer kernels refuse to probe when using the same wait pin for
different chipselect regions.

But this may be a usecase when connecting for example FPGA or ASIC
modules to the gpmc, which only got one wait pin installed.

Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
---
 drivers/memory/omap-gpmc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 2351f2708da2..579903457415 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -2215,9 +2215,13 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 							 GPIO_ACTIVE_HIGH,
 							 GPIOD_IN);
 		if (IS_ERR(waitpin_desc)) {
-			dev_err(&pdev->dev, "invalid wait-pin: %d\n", wait_pin);
 			ret = PTR_ERR(waitpin_desc);
-			goto err;
+			if (ret == -EBUSY) {
+				dev_info(&pdev->dev, "shared wait-pin: %d\n", wait_pin);
+			} else {
+				dev_err(&pdev->dev, "invalid wait-pin: %d\n", wait_pin);
+				goto err;
+			}
 		}
 	}
 
-- 
2.25.1

[PATCH v2 2/3] memory: omap-gpmc: add support for wait pin polarity

[B. Niedermayr]

From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>

Setting the wait pin polarity from the device tree is currently not
possible. The device tree property "gpmc,wait-pin-polarity" can be used
for that. If this property is missing the previous default value
is used instead, which preserves backwards compatibility.

The wait pin polarity is then set via the gpiochip
direction_input callback function.

Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
---
 drivers/memory/omap-gpmc.c              | 30 ++++++++++++++++++++-----
 include/linux/platform_data/gpmc-omap.h |  1 +
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 579903457415..be3c35ae9619 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -35,6 +35,8 @@
 
 #include <linux/platform_data/mtd-nand-omap2.h>
 
+#include "../gpio/gpiolib.h"
+
 #define	DEVICE_NAME		"omap-gpmc"
 
 /* GPMC register offsets */
@@ -1980,6 +1982,11 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
 							"gpmc,wait-on-read");
 		p->wait_on_write = of_property_read_bool(np,
 							 "gpmc,wait-on-write");
+		p->wait_pin_polarity = of_property_read_u32(np,
+								 "gpmc,wait-pin-polarity",
+								 &p->wait_pin_polarity);
+		if (p->wait_pin_polarity < 0)
+			p->wait_pin_polarity = GPIO_ACTIVE_HIGH;
 		if (!p->wait_on_read && !p->wait_on_write)
 			pr_debug("%s: rd/wr wait monitoring not enabled!\n",
 				 __func__);
@@ -2210,10 +2217,11 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 	if (gpmc_s.wait_on_read || gpmc_s.wait_on_write) {
 		unsigned int wait_pin = gpmc_s.wait_pin;
 
-		waitpin_desc = gpiochip_request_own_desc(&gpmc->gpio_chip,
-							 wait_pin, "WAITPIN",
-							 GPIO_ACTIVE_HIGH,
-							 GPIOD_IN);
+		waitpin_desc =
+			gpiochip_request_own_desc(&gpmc->gpio_chip,
+				wait_pin, "WAITPIN",
+				gpmc_s.wait_pin_polarity ? GPIO_ACTIVE_HIGH : GPIO_ACTIVE_LOW,
+				GPIOD_IN);
 		if (IS_ERR(waitpin_desc)) {
 			ret = PTR_ERR(waitpin_desc);
 			if (ret == -EBUSY) {
@@ -2342,7 +2350,19 @@ static int gpmc_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 static int gpmc_gpio_direction_input(struct gpio_chip *chip,
 				     unsigned int offset)
 {
-	return 0;	/* we're input only */
+	u32 reg;
+	struct gpio_desc *desc = gpiochip_get_desc(chip, offset);
+
+	offset += 8;
+	reg = gpmc_read_reg(GPMC_CONFIG);
+
+	if (BIT(FLAG_ACTIVE_LOW) & desc->flags)
+		reg &= ~BIT(offset);
+	else
+		reg |= BIT(offset);
+
+	gpmc_write_reg(GPMC_CONFIG, reg);
+	return 0;
 }
 
 static int gpmc_gpio_direction_output(struct gpio_chip *chip,
diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
index c9cc4e32435d..bf4f2246f31d 100644
--- a/include/linux/platform_data/gpmc-omap.h
+++ b/include/linux/platform_data/gpmc-omap.h
@@ -149,6 +149,7 @@ struct gpmc_settings {
 	u32 device_width;	/* device bus width (8 or 16 bit) */
 	u32 mux_add_data;	/* multiplex address & data */
 	u32 wait_pin;		/* wait-pin to be used */
+	u32 wait_pin_polarity;	/* wait-pin polarity */
 };
 
 /* Data for each chip select */
-- 
2.25.1

[PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[B. Niedermayr]

From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>

Add a new dt-binding for the wait-pin-polarity property

Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
---
 .../bindings/memory-controllers/ti,gpmc-child.yaml         | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
index 6e3995bb1630..7c721206f10b 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
@@ -230,6 +230,13 @@ properties:
       Wait-pin used by client. Must be less than "gpmc,num-waitpins".
     $ref: /schemas/types.yaml#/definitions/uint32
 
+  gpmc,wait-pin-polarity:
+    description: |
+      Wait-pin polarity used by the clien. It relates to the pin defined
+      with "gpmc,wait-pin".
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0
+
   gpmc,wait-on-read:
     description: Enables wait monitoring on reads.
     type: boolean
-- 
2.25.1

Re: [PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Roger Quadros]

Hi Benedikt,

On 05/09/2022 10:17, B. Niedermayr wrote:
> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> 
> Add a new dt-binding for the wait-pin-polarity property
> 
> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> ---
>  .../bindings/memory-controllers/ti,gpmc-child.yaml         | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
> index 6e3995bb1630..7c721206f10b 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
> @@ -230,6 +230,13 @@ properties:
>        Wait-pin used by client. Must be less than "gpmc,num-waitpins".
>      $ref: /schemas/types.yaml#/definitions/uint32
>  
> +  gpmc,wait-pin-polarity:
> +    description: |
> +      Wait-pin polarity used by the clien. It relates to the pin defined

did you mean "client?"
Can you please specify what value is for Active Low vs Active High?

> +      with "gpmc,wait-pin".
> +    $ref: /schemas/types.yaml#/definitions/uint32

Why can't type be boolean?

> +    default: 0
> +
>    gpmc,wait-on-read:
>      description: Enables wait monitoring on reads.
>      type: boolean

cheers,
-roger

Re: [PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Niedermayr, BENEDIKT]

On Mon, 2022-09-05 at 11:56 +0300, Roger Quadros wrote:
> Hi Benedikt,
> 
> On 05/09/2022 10:17, B. Niedermayr wrote:
> > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> > 
> > Add a new dt-binding for the wait-pin-polarity property
> > 
> > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com
> > >
> > ---
> >  .../bindings/memory-controllers/ti,gpmc-child.yaml         | 7
> > +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/memory-
> > controllers/ti,gpmc-child.yaml
> > b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
> > child.yaml
> > index 6e3995bb1630..7c721206f10b 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
> > child.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
> > child.yaml
> > @@ -230,6 +230,13 @@ properties:
> >        Wait-pin used by client. Must be less than "gpmc,num-
> > waitpins".
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >  
> > +  gpmc,wait-pin-polarity:
> > +    description: |
> > +      Wait-pin polarity used by the clien. It relates to the pin
> > defined
> 
> did you mean "client?"
> Can you please specify what value is for Active Low vs Active High?

Yes, that makes sense. And yes I meant "client". My typo.....
> 
> > +      with "gpmc,wait-pin".
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> Why can't type be boolean?

Of course we can use the boolean there. In that case I should give the
property a more meaningful name e.g. wait-pin-active-high or wait-pin-
active-low. 
Since the default behavour of this pin is Active High,
a bool property "gpmc,wait-pin-active-low" would make more sense for
backwards compatibility. 
If the property is missing, than the polarity stays on Active High like
before.

> 
> > +    default: 0
> > +
> >    gpmc,wait-on-read:
> >      description: Enables wait monitoring on reads.
> >      type: boolean
> 
> cheers,
> -roger

cheers,
benedikt

Re: [PATCH v2 2/3] memory: omap-gpmc: add support for wait pin polarity

[Roger Quadros]

Hi,

On 05/09/2022 10:17, B. Niedermayr wrote:
> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> 
> Setting the wait pin polarity from the device tree is currently not
> possible. The device tree property "gpmc,wait-pin-polarity" can be used
> for that. If this property is missing the previous default value
> is used instead, which preserves backwards compatibility.
> 
> The wait pin polarity is then set via the gpiochip
> direction_input callback function.
> 
> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> ---
>  drivers/memory/omap-gpmc.c              | 30 ++++++++++++++++++++-----
>  include/linux/platform_data/gpmc-omap.h |  1 +
>  2 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 579903457415..be3c35ae9619 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -35,6 +35,8 @@
>  
>  #include <linux/platform_data/mtd-nand-omap2.h>
>  
> +#include "../gpio/gpiolib.h"
> +
>  #define	DEVICE_NAME		"omap-gpmc"
>  
>  /* GPMC register offsets */
> @@ -1980,6 +1982,11 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
>  							"gpmc,wait-on-read");
>  		p->wait_on_write = of_property_read_bool(np,
>  							 "gpmc,wait-on-write");
> +		p->wait_pin_polarity = of_property_read_u32(np,
> +								 "gpmc,wait-pin-polarity",
> +								 &p->wait_pin_polarity);

This is wrong. of_property_read_u32() returns 0 or error value. It does not return the properties value.
The properties value is already stored in the pointer passed in the 3rd argument.

> +		if (p->wait_pin_polarity < 0)
> +			p->wait_pin_polarity = GPIO_ACTIVE_HIGH;
>  		if (!p->wait_on_read && !p->wait_on_write)
>  			pr_debug("%s: rd/wr wait monitoring not enabled!\n",
>  				 __func__);
> @@ -2210,10 +2217,11 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  	if (gpmc_s.wait_on_read || gpmc_s.wait_on_write) {
>  		unsigned int wait_pin = gpmc_s.wait_pin;
>  
> -		waitpin_desc = gpiochip_request_own_desc(&gpmc->gpio_chip,
> -							 wait_pin, "WAITPIN",
> -							 GPIO_ACTIVE_HIGH,
> -							 GPIOD_IN);
> +		waitpin_desc =
> +			gpiochip_request_own_desc(&gpmc->gpio_chip,
> +				wait_pin, "WAITPIN",
> +				gpmc_s.wait_pin_polarity ? GPIO_ACTIVE_HIGH : GPIO_ACTIVE_LOW,

#define GPIO_ACTIVE_HIGH 0
#define GPIO_ACTIVE_LOW 1

Why not just retain the same logic for gpmc_s.wait_pin_polarity and the DT property?

> +				GPIOD_IN);

This change to gpiochip_request_own_desc() is not really required as we are merely reserving the GPIO so other users cannot use it. The wait_pin's polarity is actually set in GPMC_CONFIG register. GPMC wait_pin polarity logic is hard-wired and doesn't depend on GPIO subsystem for its polarity.

>  		if (IS_ERR(waitpin_desc)) {
>  			ret = PTR_ERR(waitpin_desc);
>  			if (ret == -EBUSY) {
> @@ -2342,7 +2350,19 @@ static int gpmc_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>  static int gpmc_gpio_direction_input(struct gpio_chip *chip,
>  				     unsigned int offset)
>  {
> -	return 0;	/* we're input only */
> +	u32 reg;
> +	struct gpio_desc *desc = gpiochip_get_desc(chip, offset);
> +
> +	offset += 8;
> +	reg = gpmc_read_reg(GPMC_CONFIG);
> +
> +	if (BIT(FLAG_ACTIVE_LOW) & desc->flags)
> +		reg &= ~BIT(offset);
> +	else
> +		reg |= BIT(offset);
> +
> +	gpmc_write_reg(GPMC_CONFIG, reg);
> +	return 0;

This is the wrong place to put this code.
Wait pin plarity logic has nothing to do with GPIO subsystem.

The GPMC_CONFIG wait_pin polarity setting needs to be set in gpmc_cs_program_settings().
You need to check gpmc_settings->wait_pin_polarity there and set the polarity flag in GPMC_CONFIG register accordingly.

>  }
>  
>  static int gpmc_gpio_direction_output(struct gpio_chip *chip,
> diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
> index c9cc4e32435d..bf4f2246f31d 100644
> --- a/include/linux/platform_data/gpmc-omap.h
> +++ b/include/linux/platform_data/gpmc-omap.h
> @@ -149,6 +149,7 @@ struct gpmc_settings {
>  	u32 device_width;	/* device bus width (8 or 16 bit) */
>  	u32 mux_add_data;	/* multiplex address & data */
>  	u32 wait_pin;		/* wait-pin to be used */
> +	u32 wait_pin_polarity;	/* wait-pin polarity */
>  };
>  
>  /* Data for each chip select */

cheers,
-roger

Re: [PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Roger Quadros]

On 05/09/2022 12:14, Niedermayr, BENEDIKT wrote:
> On Mon, 2022-09-05 at 11:56 +0300, Roger Quadros wrote:
>> Hi Benedikt,
>>
>> On 05/09/2022 10:17, B. Niedermayr wrote:
>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>
>>> Add a new dt-binding for the wait-pin-polarity property
>>>
>>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com
>>>>
>>> ---
>>>  .../bindings/memory-controllers/ti,gpmc-child.yaml         | 7
>>> +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/memory-
>>> controllers/ti,gpmc-child.yaml
>>> b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>> child.yaml
>>> index 6e3995bb1630..7c721206f10b 100644
>>> --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>> child.yaml
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>> child.yaml
>>> @@ -230,6 +230,13 @@ properties:
>>>        Wait-pin used by client. Must be less than "gpmc,num-
>>> waitpins".
>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>  
>>> +  gpmc,wait-pin-polarity:
>>> +    description: |
>>> +      Wait-pin polarity used by the clien. It relates to the pin
>>> defined
>>
>> did you mean "client?"
>> Can you please specify what value is for Active Low vs Active High?
> 
> Yes, that makes sense. And yes I meant "client". My typo.....
>>
>>> +      with "gpmc,wait-pin".
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>
>> Why can't type be boolean?
> 
> Of course we can use the boolean there. In that case I should give the
> property a more meaningful name e.g. wait-pin-active-high or wait-pin-
> active-low. 
> Since the default behavour of this pin is Active High,
> a bool property "gpmc,wait-pin-active-low" would make more sense for
> backwards compatibility. 
> If the property is missing, than the polarity stays on Active High like
> before.
> 

OK, in that case you don't have to clarify the polarity in description.

>>
>>> +    default: 0
>>> +
>>>    gpmc,wait-on-read:
>>>      description: Enables wait monitoring on reads.
>>>      type: boolean
>>
>> cheers,
>> -roger
> 
> cheers,
> benedikt
> 

cheers,
-roger

Re: [PATCH v2 2/3] memory: omap-gpmc: add support for wait pin polarity

[Niedermayr, BENEDIKT]

On Mon, 2022-09-05 at 12:19 +0300, Roger Quadros wrote:
> Hi,
> 
> On 05/09/2022 10:17, B. Niedermayr wrote:
> > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> > 
> > Setting the wait pin polarity from the device tree is currently not
> > possible. The device tree property "gpmc,wait-pin-polarity" can be
> > used
> > for that. If this property is missing the previous default value
> > is used instead, which preserves backwards compatibility.
> > 
> > The wait pin polarity is then set via the gpiochip
> > direction_input callback function.
> > 
> > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com
> > >
> > ---
> >  drivers/memory/omap-gpmc.c              | 30 ++++++++++++++++++++-
> > ----
> >  include/linux/platform_data/gpmc-omap.h |  1 +
> >  2 files changed, 26 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-
> > gpmc.c
> > index 579903457415..be3c35ae9619 100644
> > --- a/drivers/memory/omap-gpmc.c
> > +++ b/drivers/memory/omap-gpmc.c
> > @@ -35,6 +35,8 @@
> >  
> >  #include <linux/platform_data/mtd-nand-omap2.h>
> >  
> > +#include "../gpio/gpiolib.h"
> > +
> >  #define	DEVICE_NAME		"omap-gpmc"
> >  
> >  /* GPMC register offsets */
> > @@ -1980,6 +1982,11 @@ void gpmc_read_settings_dt(struct
> > device_node *np, struct gpmc_settings *p)
> >  							"gpmc,wait-on-
> > read");
> >  		p->wait_on_write = of_property_read_bool(np,
> >  							 "gpmc,wait-on-
> > write");
> > +		p->wait_pin_polarity = of_property_read_u32(np,
> > +								 "gpmc,
> > wait-pin-polarity",
> > +								 &p-
> > >wait_pin_polarity);
> 
> This is wrong. of_property_read_u32() returns 0 or error value. It
> does not return the properties value.
> The properties value is already stored in the pointer passed in the
> 3rd argument.
> 

Ah. Yes that's a mistake. Thanks for that hint!
A bool property will make things a bit easier at this point.


> > +		if (p->wait_pin_polarity < 0)
> > +			p->wait_pin_polarity = GPIO_ACTIVE_HIGH;
> >  		if (!p->wait_on_read && !p->wait_on_write)
> >  			pr_debug("%s: rd/wr wait monitoring not
> > enabled!\n",
> >  				 __func__);
> > @@ -2210,10 +2217,11 @@ static int gpmc_probe_generic_child(struct
> > platform_device *pdev,
> >  	if (gpmc_s.wait_on_read || gpmc_s.wait_on_write) {
> >  		unsigned int wait_pin = gpmc_s.wait_pin;
> >  
> > -		waitpin_desc = gpiochip_request_own_desc(&gpmc-
> > >gpio_chip,
> > -							 wait_pin,
> > "WAITPIN",
> > -							 GPIO_ACTIVE_HI
> > GH,
> > -							 GPIOD_IN);
> > +		waitpin_desc =
> > +			gpiochip_request_own_desc(&gpmc->gpio_chip,
> > +				wait_pin, "WAITPIN",
> > +				gpmc_s.wait_pin_polarity ?
> > GPIO_ACTIVE_HIGH : GPIO_ACTIVE_LOW,
> 
> #define GPIO_ACTIVE_HIGH 0
> #define GPIO_ACTIVE_LOW 1
> 
> Why not just retain the same logic for gpmc_s.wait_pin_polarity and
> the DT property?

Also makes sense!
> 
> > +				GPIOD_IN);
> 
> This change to gpiochip_request_own_desc() is not really required as
> we are merely reserving the GPIO so other users cannot use it. The
> wait_pin's polarity is actually set in GPMC_CONFIG register. GPMC
> wait_pin polarity logic is hard-wired and doesn't depend on GPIO
> subsystem for its polarity.
> 
> >  		if (IS_ERR(waitpin_desc)) {
> >  			ret = PTR_ERR(waitpin_desc);
> >  			if (ret == -EBUSY) {
> > @@ -2342,7 +2350,19 @@ static int gpmc_gpio_get_direction(struct
> > gpio_chip *chip, unsigned int offset)
> >  static int gpmc_gpio_direction_input(struct gpio_chip *chip,
> >  				     unsigned int offset)
> >  {
> > -	return 0;	/* we're input only */
> > +	u32 reg;
> > +	struct gpio_desc *desc = gpiochip_get_desc(chip, offset);
> > +
> > +	offset += 8;
> > +	reg = gpmc_read_reg(GPMC_CONFIG);
> > +
> > +	if (BIT(FLAG_ACTIVE_LOW) & desc->flags)
> > +		reg &= ~BIT(offset);
> > +	else
> > +		reg |= BIT(offset);
> > +
> > +	gpmc_write_reg(GPMC_CONFIG, reg);
> > +	return 0;
> 
> This is the wrong place to put this code.
> Wait pin plarity logic has nothing to do with GPIO subsystem.
> 
> The GPMC_CONFIG wait_pin polarity setting needs to be set in
> gpmc_cs_program_settings().
> You need to check gpmc_settings->wait_pin_polarity there and set the
> polarity flag in GPMC_CONFIG register accordingly.
> 
Ok than I will put this into gpmc_cs_programm_settings function.
This way, the "#include ../gpio/gpiolib.h" is also not required
anymore.

It wasn't very sure about why the waitpins where allocated via a
gpiochip. But I can image it was because of ressource locking of those
pins so they don't get allocated from somewhere else?


> >  }
> >  
> >  static int gpmc_gpio_direction_output(struct gpio_chip *chip,
> > diff --git a/include/linux/platform_data/gpmc-omap.h
> > b/include/linux/platform_data/gpmc-omap.h
> > index c9cc4e32435d..bf4f2246f31d 100644
> > --- a/include/linux/platform_data/gpmc-omap.h
> > +++ b/include/linux/platform_data/gpmc-omap.h
> > @@ -149,6 +149,7 @@ struct gpmc_settings {
> >  	u32 device_width;	/* device bus width (8 or 16 bit) */
> >  	u32 mux_add_data;	/* multiplex address & data */
> >  	u32 wait_pin;		/* wait-pin to be used */
> > +	u32 wait_pin_polarity;	/* wait-pin polarity */
> >  };
> >  
> >  /* Data for each chip select */
> 
> cheers,
> -roger

I will submit my changes during the day.
Thanks for the feedback!

cheers,
benedikt

Re: [PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Krzysztof Kozlowski]

On 05/09/2022 11:21, Roger Quadros wrote:
> 
> 
> On 05/09/2022 12:14, Niedermayr, BENEDIKT wrote:
>> On Mon, 2022-09-05 at 11:56 +0300, Roger Quadros wrote:
>>> Hi Benedikt,
>>>
>>> On 05/09/2022 10:17, B. Niedermayr wrote:
>>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>>
>>>> Add a new dt-binding for the wait-pin-polarity property
>>>>
>>>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com
>>>>>
>>>> ---
>>>>  .../bindings/memory-controllers/ti,gpmc-child.yaml         | 7
>>>> +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/memory-
>>>> controllers/ti,gpmc-child.yaml
>>>> b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>>> child.yaml
>>>> index 6e3995bb1630..7c721206f10b 100644
>>>> --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>>> child.yaml
>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>>> child.yaml
>>>> @@ -230,6 +230,13 @@ properties:
>>>>        Wait-pin used by client. Must be less than "gpmc,num-
>>>> waitpins".
>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>  
>>>> +  gpmc,wait-pin-polarity:
>>>> +    description: |
>>>> +      Wait-pin polarity used by the clien. It relates to the pin
>>>> defined
>>>
>>> did you mean "client?"
>>> Can you please specify what value is for Active Low vs Active High?
>>
>> Yes, that makes sense. And yes I meant "client". My typo.....
>>>
>>>> +      with "gpmc,wait-pin".
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>
>>> Why can't type be boolean?
>>
>> Of course we can use the boolean there. In that case I should give the
>> property a more meaningful name e.g. wait-pin-active-high or wait-pin-
>> active-low. 
>> Since the default behavour of this pin is Active High,
>> a bool property "gpmc,wait-pin-active-low" would make more sense for
>> backwards compatibility. 
>> If the property is missing, than the polarity stays on Active High like
>> before.
>>
> 
> OK, in that case you don't have to clarify the polarity in description.

I don't understand (and it is not explained in commit msg), why do you
need such property instead of using standard GPIO flags.

The driver should use standard GPIO descriptor and standard bindings. If
it cannot, this has to be explained.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Niedermayr, BENEDIKT]

On Mon, 2022-09-05 at 11:54 +0200, Krzysztof Kozlowski wrote:
> On 05/09/2022 11:21, Roger Quadros wrote:
> > 
> > On 05/09/2022 12:14, Niedermayr, BENEDIKT wrote:
> > > On Mon, 2022-09-05 at 11:56 +0300, Roger Quadros wrote:
> > > > Hi Benedikt,
> > > > 
> > > > On 05/09/2022 10:17, B. Niedermayr wrote:
> > > > > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> > > > > 
> > > > > Add a new dt-binding for the wait-pin-polarity property
> > > > > 
> > > > > Signed-off-by: Benedikt Niedermayr <
> > > > > benedikt.niedermayr@siemens.com
> > > > > ---
> > > > >  .../bindings/memory-controllers/ti,gpmc-child.yaml         | 
> > > > > 7
> > > > > +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/memory-
> > > > > controllers/ti,gpmc-child.yaml
> > > > > b/Documentation/devicetree/bindings/memory-
> > > > > controllers/ti,gpmc-
> > > > > child.yaml
> > > > > index 6e3995bb1630..7c721206f10b 100644
> > > > > --- a/Documentation/devicetree/bindings/memory-
> > > > > controllers/ti,gpmc-
> > > > > child.yaml
> > > > > +++ b/Documentation/devicetree/bindings/memory-
> > > > > controllers/ti,gpmc-
> > > > > child.yaml
> > > > > @@ -230,6 +230,13 @@ properties:
> > > > >        Wait-pin used by client. Must be less than "gpmc,num-
> > > > > waitpins".
> > > > >      $ref: /schemas/types.yaml#/definitions/uint32
> > > > >  
> > > > > +  gpmc,wait-pin-polarity:
> > > > > +    description: |
> > > > > +      Wait-pin polarity used by the clien. It relates to the
> > > > > pin
> > > > > defined
> > > > 
> > > > did you mean "client?"
> > > > Can you please specify what value is for Active Low vs Active
> > > > High?
> > > 
> > > Yes, that makes sense. And yes I meant "client". My typo.....
> > > > > +      with "gpmc,wait-pin".
> > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > 
> > > > Why can't type be boolean?
> > > 
> > > Of course we can use the boolean there. In that case I should
> > > give the
> > > property a more meaningful name e.g. wait-pin-active-high or
> > > wait-pin-
> > > active-low. 
> > > Since the default behavour of this pin is Active High,
> > > a bool property "gpmc,wait-pin-active-low" would make more sense
> > > for
> > > backwards compatibility. 
> > > If the property is missing, than the polarity stays on Active
> > > High like
> > > before.
> > > 
> > 
> > OK, in that case you don't have to clarify the polarity in
> > description.
> 
> I don't understand (and it is not explained in commit msg), why do
> you
> need such property instead of using standard GPIO flags.
> 
> The driver should use standard GPIO descriptor and standard bindings.
> If
> it cannot, this has to be explained.
> 
> Best regards,
> Krzysztof

I think this is beacause the GPMC controller itself is not respecting
the GPIO flags. Instead the GPMC is reading the Line Level directly
(high,low) and then evaluates the logic depending how
the WAIT<x>PINPOLARITY bit is set in the GPMPC_CONFIG register.

Until now gpiochip_request_own_desc() was hardcorded
to GPIO_ACTIVE_HIGH. An the GPMC_CONFIG register configuration has no
relation to the GPIO setting (in the current implementation).
My first approach was to make this part configurable via a new device
tree property (wait-pin-polarity).

IMHO (correct me if I'm wrong) the current implementation also does not
make ues of standart GPIO bindings and defines the wait pin via a
separate "gpmc,waitpin" binding.

E.g. gpmc,watipin = <0> or gpmc,waitpin=<1>

The best solution would should be when setting the binding this way for
example: gpmc,wait-pin = <&gpiox y ACTIVE_X>

But I think the current omap-gpmc.c implementation does not offer such
a usecase and as roger already mentioned: 
"GPMC wait_pin polarity logic is hard-wired and doesn't depend on GPIO
subsystem for its polarity"

cheers,
benedikt

Re: [PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Krzysztof Kozlowski]

On 05/09/2022 13:48, Niedermayr, BENEDIKT wrote:
> On Mon, 2022-09-05 at 11:54 +0200, Krzysztof Kozlowski wrote:
>> On 05/09/2022 11:21, Roger Quadros wrote:
>>>
>>> On 05/09/2022 12:14, Niedermayr, BENEDIKT wrote:
>>>> On Mon, 2022-09-05 at 11:56 +0300, Roger Quadros wrote:
>>>>> Hi Benedikt,
>>>>>
>>>>> On 05/09/2022 10:17, B. Niedermayr wrote:
>>>>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>>>>
>>>>>> Add a new dt-binding for the wait-pin-polarity property
>>>>>>
>>>>>> Signed-off-by: Benedikt Niedermayr <
>>>>>> benedikt.niedermayr@siemens.com
>>>>>> ---
>>>>>>  .../bindings/memory-controllers/ti,gpmc-child.yaml         | 
>>>>>> 7
>>>>>> +++++++
>>>>>>  1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/memory-
>>>>>> controllers/ti,gpmc-child.yaml
>>>>>> b/Documentation/devicetree/bindings/memory-
>>>>>> controllers/ti,gpmc-
>>>>>> child.yaml
>>>>>> index 6e3995bb1630..7c721206f10b 100644
>>>>>> --- a/Documentation/devicetree/bindings/memory-
>>>>>> controllers/ti,gpmc-
>>>>>> child.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/memory-
>>>>>> controllers/ti,gpmc-
>>>>>> child.yaml
>>>>>> @@ -230,6 +230,13 @@ properties:
>>>>>>        Wait-pin used by client. Must be less than "gpmc,num-
>>>>>> waitpins".
>>>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>  
>>>>>> +  gpmc,wait-pin-polarity:
>>>>>> +    description: |
>>>>>> +      Wait-pin polarity used by the clien. It relates to the
>>>>>> pin
>>>>>> defined
>>>>>
>>>>> did you mean "client?"
>>>>> Can you please specify what value is for Active Low vs Active
>>>>> High?
>>>>
>>>> Yes, that makes sense. And yes I meant "client". My typo.....
>>>>>> +      with "gpmc,wait-pin".
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>
>>>>> Why can't type be boolean?
>>>>
>>>> Of course we can use the boolean there. In that case I should
>>>> give the
>>>> property a more meaningful name e.g. wait-pin-active-high or
>>>> wait-pin-
>>>> active-low. 
>>>> Since the default behavour of this pin is Active High,
>>>> a bool property "gpmc,wait-pin-active-low" would make more sense
>>>> for
>>>> backwards compatibility. 
>>>> If the property is missing, than the polarity stays on Active
>>>> High like
>>>> before.
>>>>
>>>
>>> OK, in that case you don't have to clarify the polarity in
>>> description.
>>
>> I don't understand (and it is not explained in commit msg), why do
>> you
>> need such property instead of using standard GPIO flags.
>>
>> The driver should use standard GPIO descriptor and standard bindings.
>> If
>> it cannot, this has to be explained.
>>
>> Best regards,
>> Krzysztof
> 
> I think this is beacause the GPMC controller itself is not respecting
> the GPIO flags. Instead the GPMC is reading the Line Level directly
> (high,low) and then evaluates the logic depending how
> the WAIT<x>PINPOLARITY bit is set in the GPMPC_CONFIG register.
> 
> Until now gpiochip_request_own_desc() was hardcorded
> to GPIO_ACTIVE_HIGH. An the GPMC_CONFIG register configuration has no
> relation to the GPIO setting (in the current implementation).
> My first approach was to make this part configurable via a new device
> tree property (wait-pin-polarity).
> 
> IMHO (correct me if I'm wrong) the current implementation also does not
> make ues of standart GPIO bindings and defines the wait pin via a
> separate "gpmc,waitpin" binding.
> 
> E.g. gpmc,watipin = <0> or gpmc,waitpin=<1>
> 
> The best solution would should be when setting the binding this way for
> example: gpmc,wait-pin = <&gpiox y ACTIVE_X>

Yes and I am afraid this will grow instead of adding proper GPIO usage.
Any reason why it cannot be a standard GPIO pin desc?

> 
> But I think the current omap-gpmc.c implementation does not offer such
> a usecase and as roger already mentioned: 
> "GPMC wait_pin polarity logic is hard-wired and doesn't depend on GPIO
> subsystem for its polarity"

This part I don't get. You mean hard-wired in the driver or hard-wired
in the hardware? If the first, please un-wire it. If the latter, your
property makes no sense, right?

Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Niedermayr, BENEDIKT]

On Mon, 2022-09-05 at 14:08 +0200, Krzysztof Kozlowski wrote:
> On 05/09/2022 13:48, Niedermayr, BENEDIKT wrote:
> > On Mon, 2022-09-05 at 11:54 +0200, Krzysztof Kozlowski wrote:
> > > On 05/09/2022 11:21, Roger Quadros wrote:
> > > > On 05/09/2022 12:14, Niedermayr, BENEDIKT wrote:
> > > > > On Mon, 2022-09-05 at 11:56 +0300, Roger Quadros wrote:
> > > > > > Hi Benedikt,
> > > > > > 
> > > > > > On 05/09/2022 10:17, B. Niedermayr wrote:
> > > > > > > From: Benedikt Niedermayr <
> > > > > > > benedikt.niedermayr@siemens.com>
> > > > > > > 
> > > > > > > Add a new dt-binding for the wait-pin-polarity property
> > > > > > > 
> > > > > > > Signed-off-by: Benedikt Niedermayr <
> > > > > > > benedikt.niedermayr@siemens.com
> > > > > > > ---
> > > > > > >  .../bindings/memory-controllers/ti,gpmc-
> > > > > > > child.yaml         | 
> > > > > > > 7
> > > > > > > +++++++
> > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/memory-
> > > > > > > controllers/ti,gpmc-child.yaml
> > > > > > > b/Documentation/devicetree/bindings/memory-
> > > > > > > controllers/ti,gpmc-
> > > > > > > child.yaml
> > > > > > > index 6e3995bb1630..7c721206f10b 100644
> > > > > > > --- a/Documentation/devicetree/bindings/memory-
> > > > > > > controllers/ti,gpmc-
> > > > > > > child.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/memory-
> > > > > > > controllers/ti,gpmc-
> > > > > > > child.yaml
> > > > > > > @@ -230,6 +230,13 @@ properties:
> > > > > > >        Wait-pin used by client. Must be less than
> > > > > > > "gpmc,num-
> > > > > > > waitpins".
> > > > > > >      $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > >  
> > > > > > > +  gpmc,wait-pin-polarity:
> > > > > > > +    description: |
> > > > > > > +      Wait-pin polarity used by the clien. It relates to
> > > > > > > the
> > > > > > > pin
> > > > > > > defined
> > > > > > 
> > > > > > did you mean "client?"
> > > > > > Can you please specify what value is for Active Low vs
> > > > > > Active
> > > > > > High?
> > > > > 
> > > > > Yes, that makes sense. And yes I meant "client". My typo.....
> > > > > > > +      with "gpmc,wait-pin".
> > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > 
> > > > > > Why can't type be boolean?
> > > > > 
> > > > > Of course we can use the boolean there. In that case I should
> > > > > give the
> > > > > property a more meaningful name e.g. wait-pin-active-high or
> > > > > wait-pin-
> > > > > active-low. 
> > > > > Since the default behavour of this pin is Active High,
> > > > > a bool property "gpmc,wait-pin-active-low" would make more
> > > > > sense
> > > > > for
> > > > > backwards compatibility. 
> > > > > If the property is missing, than the polarity stays on Active
> > > > > High like
> > > > > before.
> > > > > 
> > > > 
> > > > OK, in that case you don't have to clarify the polarity in
> > > > description.
> > > 
> > > I don't understand (and it is not explained in commit msg), why
> > > do
> > > you
> > > need such property instead of using standard GPIO flags.
> > > 
> > > The driver should use standard GPIO descriptor and standard
> > > bindings.
> > > If
> > > it cannot, this has to be explained.
> > > 
> > > Best regards,
> > > Krzysztof
> > 
> > I think this is beacause the GPMC controller itself is not
> > respecting
> > the GPIO flags. Instead the GPMC is reading the Line Level directly
> > (high,low) and then evaluates the logic depending how
> > the WAIT<x>PINPOLARITY bit is set in the GPMPC_CONFIG register.
> > 
> > Until now gpiochip_request_own_desc() was hardcorded
> > to GPIO_ACTIVE_HIGH. An the GPMC_CONFIG register configuration has
> > no
> > relation to the GPIO setting (in the current implementation).
> > My first approach was to make this part configurable via a new
> > device
> > tree property (wait-pin-polarity).
> > 
> > IMHO (correct me if I'm wrong) the current implementation also does
> > not
> > make ues of standart GPIO bindings and defines the wait pin via a
> > separate "gpmc,waitpin" binding.
> > 
> > E.g. gpmc,watipin = <0> or gpmc,waitpin=<1>
> > 
> > The best solution would should be when setting the binding this way
> > for
> > example: gpmc,wait-pin = <&gpiox y ACTIVE_X>
> 
> Yes and I am afraid this will grow instead of adding proper GPIO
> usage.
> Any reason why it cannot be a standard GPIO pin desc?

This change (gpmc,wait-pin = <&gpiox y ACTIVE_X>) may break current
implementations when the GPMC is used with NAND devices. It comes to
some configuration in the GPMC_CONFIG register when IRQs are setup
in Nand Mode. 

But when using the Nand mode the register configuration in question is
properly configured, but in a complete different context:

E.g. in am335x-baltos.dtsi:

interrupts = <0 IRQ_TYPE_NONE>, /* fifoevent */
             <1
IRQ_TYPE_NONE>; /* termcount */
rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>;
/* gpmc_wait0 */

The "interrupts" property will configure the GPMC_CONFIG register bits
for the waitpins. 

But in a non-NAND case, the "interrupt" configuration wouldn't make any
sense, since interrupts are not used at all.

The "rb-gpios" is *not* handled by the omap-gpmc(maybe somewhere in the
NAND subsystem?). 

Changing the wait-pin property to "gpmc,wait-pin = <&gpmc X ACTIVE_X>"
will currently break at least 3 device trees:

arch/arm/boot/dts/omap3-devkit8000-common.dtsi
arch/arm/boot/dts/omap-zoom-common.dtsi
arch/arm/boot/dts/omap3-lilly-a83x.dtsi

 
I think it makes sense to implement a v3 as POC?

> > But I think the current omap-gpmc.c implementation does not offer
> > such
> > a usecase and as roger already mentioned: 
> > "GPMC wait_pin polarity logic is hard-wired and doesn't depend on
> > GPIO
> > subsystem for its polarity"
> 
> This part I don't get. You mean hard-wired in the driver or hard-
> wired
> in the hardware? If the first, please un-wire it. If the latter, your
> property makes no sense, right?
> 
IMHO roger meant that configuring the GPMC registers via gpiolib
callbacks would be the wrong place to implementent. I implemented 
the gpmc register configuration in the gpio_direction_input function.


> Best regards,
> Krzysztof

Re: [PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Roger Quadros]

Hi,

On 05/09/2022 16:33, Niedermayr, BENEDIKT wrote:
> On Mon, 2022-09-05 at 14:08 +0200, Krzysztof Kozlowski wrote:
>> On 05/09/2022 13:48, Niedermayr, BENEDIKT wrote:
>>> On Mon, 2022-09-05 at 11:54 +0200, Krzysztof Kozlowski wrote:
>>>> On 05/09/2022 11:21, Roger Quadros wrote:
>>>>> On 05/09/2022 12:14, Niedermayr, BENEDIKT wrote:
>>>>>> On Mon, 2022-09-05 at 11:56 +0300, Roger Quadros wrote:
>>>>>>> Hi Benedikt,
>>>>>>>
>>>>>>> On 05/09/2022 10:17, B. Niedermayr wrote:
>>>>>>>> From: Benedikt Niedermayr <
>>>>>>>> benedikt.niedermayr@siemens.com>
>>>>>>>>
>>>>>>>> Add a new dt-binding for the wait-pin-polarity property
>>>>>>>>
>>>>>>>> Signed-off-by: Benedikt Niedermayr <
>>>>>>>> benedikt.niedermayr@siemens.com
>>>>>>>> ---
>>>>>>>>  .../bindings/memory-controllers/ti,gpmc-
>>>>>>>> child.yaml         | 
>>>>>>>> 7
>>>>>>>> +++++++
>>>>>>>>  1 file changed, 7 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/memory-
>>>>>>>> controllers/ti,gpmc-child.yaml
>>>>>>>> b/Documentation/devicetree/bindings/memory-
>>>>>>>> controllers/ti,gpmc-
>>>>>>>> child.yaml
>>>>>>>> index 6e3995bb1630..7c721206f10b 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/memory-
>>>>>>>> controllers/ti,gpmc-
>>>>>>>> child.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/memory-
>>>>>>>> controllers/ti,gpmc-
>>>>>>>> child.yaml
>>>>>>>> @@ -230,6 +230,13 @@ properties:
>>>>>>>>        Wait-pin used by client. Must be less than
>>>>>>>> "gpmc,num-
>>>>>>>> waitpins".
>>>>>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>>>  
>>>>>>>> +  gpmc,wait-pin-polarity:
>>>>>>>> +    description: |
>>>>>>>> +      Wait-pin polarity used by the clien. It relates to
>>>>>>>> the
>>>>>>>> pin
>>>>>>>> defined
>>>>>>>
>>>>>>> did you mean "client?"
>>>>>>> Can you please specify what value is for Active Low vs
>>>>>>> Active
>>>>>>> High?
>>>>>>
>>>>>> Yes, that makes sense. And yes I meant "client". My typo.....
>>>>>>>> +      with "gpmc,wait-pin".
>>>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>>
>>>>>>> Why can't type be boolean?
>>>>>>
>>>>>> Of course we can use the boolean there. In that case I should
>>>>>> give the
>>>>>> property a more meaningful name e.g. wait-pin-active-high or
>>>>>> wait-pin-
>>>>>> active-low. 
>>>>>> Since the default behavour of this pin is Active High,
>>>>>> a bool property "gpmc,wait-pin-active-low" would make more
>>>>>> sense
>>>>>> for
>>>>>> backwards compatibility. 
>>>>>> If the property is missing, than the polarity stays on Active
>>>>>> High like
>>>>>> before.
>>>>>>
>>>>>
>>>>> OK, in that case you don't have to clarify the polarity in
>>>>> description.
>>>>
>>>> I don't understand (and it is not explained in commit msg), why
>>>> do
>>>> you
>>>> need such property instead of using standard GPIO flags.
>>>>
>>>> The driver should use standard GPIO descriptor and standard
>>>> bindings.
>>>> If
>>>> it cannot, this has to be explained.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> I think this is beacause the GPMC controller itself is not
>>> respecting
>>> the GPIO flags. Instead the GPMC is reading the Line Level directly
>>> (high,low) and then evaluates the logic depending how
>>> the WAIT<x>PINPOLARITY bit is set in the GPMPC_CONFIG register.
>>>
>>> Until now gpiochip_request_own_desc() was hardcorded
>>> to GPIO_ACTIVE_HIGH. An the GPMC_CONFIG register configuration has
>>> no
>>> relation to the GPIO setting (in the current implementation).
>>> My first approach was to make this part configurable via a new
>>> device
>>> tree property (wait-pin-polarity).
>>>
>>> IMHO (correct me if I'm wrong) the current implementation also does
>>> not
>>> make ues of standart GPIO bindings and defines the wait pin via a
>>> separate "gpmc,waitpin" binding.
>>>
>>> E.g. gpmc,watipin = <0> or gpmc,waitpin=<1>
>>>
>>> The best solution would should be when setting the binding this way
>>> for
>>> example: gpmc,wait-pin = <&gpiox y ACTIVE_X>
>>
>> Yes and I am afraid this will grow instead of adding proper GPIO
>> usage.
>> Any reason why it cannot be a standard GPIO pin desc?
> 
> This change (gpmc,wait-pin = <&gpiox y ACTIVE_X>) may break current
> implementations when the GPMC is used with NAND devices. It comes to
> some configuration in the GPMC_CONFIG register when IRQs are setup
> in Nand Mode. 
> 
> But when using the Nand mode the register configuration in question is
> properly configured, but in a complete different context:
> 
> E.g. in am335x-baltos.dtsi:

Let me clarify a bit.

The GPMC subsystem has one wait_pin per Chip select region. Some SoCs
may have 2 Chip Selects some may have more.

Each wait_pin can be used in 2 ways currently.
1) As a General purpose GPIO (GPI actually as output not supported)
via the Linux GPIO subsystem
2) As a wait state signalling for memory interface independently to
Linux GPIO subsystem. Via GPMC configuration.

> 
> interrupts = <0 IRQ_TYPE_NONE>, /* fifoevent */
>              <1
> IRQ_TYPE_NONE>; /* termcount */
> rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>;

The rb-gpios is an example of (1) I listed above. Here you can use
the standard GPIO polarity specifier and it should work currently.

An example of (2) you can see in omap3-devkit8000-common.dtsi:
        ethernet@6,0 {
                compatible = "davicom,dm9000";
		...
                gpmc,mux-add-data = <0>;
                gpmc,device-width = <1>;
-->             gpmc,wait-pin = <0>;
		..
        };

Here we set the GPMC hardware configuration to use wait_pin of
Chip Select 0 to add wait state to each bus access cycle to the
external Ethernet device. Linux GPIO subsystem has no role to play
here and everything is dealt with in GPMC hardware.

Now what this current patch series is trying to do is to add a
polarity specifier to the use case (2).
There is a GPMC hardware setting to decide the polarity of the wait_pin.
The code just needs to get the polarity setting from DT and set
this setting correctly.
I don't think this has got to do anything with GPIO as it is very specific
to GPMC configuration. So the vendor specific property, "gpmc,wait-pin-active-low"
is appropriate I think.

Hope this clarifies everything ;)


> /* gpmc_wait0 */
> 
> The "interrupts" property will configure the GPMC_CONFIG register bits
> for the waitpins. 
> 
> But in a non-NAND case, the "interrupt" configuration wouldn't make any
> sense, since interrupts are not used at all.
> 
> The "rb-gpios" is *not* handled by the omap-gpmc(maybe somewhere in the
> NAND subsystem?). 
> 
> Changing the wait-pin property to "gpmc,wait-pin = <&gpmc X ACTIVE_X>"
> will currently break at least 3 device trees:
> 
> arch/arm/boot/dts/omap3-devkit8000-common.dtsi
> arch/arm/boot/dts/omap-zoom-common.dtsi
> arch/arm/boot/dts/omap3-lilly-a83x.dtsi
> 
>  
> I think it makes sense to implement a v3 as POC?
> 
>>> But I think the current omap-gpmc.c implementation does not offer
>>> such
>>> a usecase and as roger already mentioned: 
>>> "GPMC wait_pin polarity logic is hard-wired and doesn't depend on
>>> GPIO
>>> subsystem for its polarity"
>>
>> This part I don't get. You mean hard-wired in the driver or hard-
>> wired
>> in the hardware? If the first, please un-wire it. If the latter, your
>> property makes no sense, right?
>>
> IMHO roger meant that configuring the GPMC registers via gpiolib
> callbacks would be the wrong place to implementent. I implemented 
> the gpmc register configuration in the gpio_direction_input function.
> 
> 
>> Best regards,
>> Krzysztof
> 

cheers,
-roger

Re: [PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Niedermayr, BENEDIKT]

On Tue, 2022-09-06 at 14:38 +0300, Roger Quadros wrote:
> Hi,
> 
> On 05/09/2022 16:33, Niedermayr, BENEDIKT wrote:
> > On Mon, 2022-09-05 at 14:08 +0200, Krzysztof Kozlowski wrote:
> > > On 05/09/2022 13:48, Niedermayr, BENEDIKT wrote:
> > > > On Mon, 2022-09-05 at 11:54 +0200, Krzysztof Kozlowski wrote:
> > > > > On 05/09/2022 11:21, Roger Quadros wrote:
> > > > > > On 05/09/2022 12:14, Niedermayr, BENEDIKT wrote:
> > > > > > > On Mon, 2022-09-05 at 11:56 +0300, Roger Quadros wrote:
> > > > > > > > Hi Benedikt,
> > > > > > > > 
> > > > > > > > On 05/09/2022 10:17, B. Niedermayr wrote:
> > > > > > > > > From: Benedikt Niedermayr <
> > > > > > > > > benedikt.niedermayr@siemens.com>
> > > > > > > > > 
> > > > > > > > > Add a new dt-binding for the wait-pin-polarity
> > > > > > > > > property
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Benedikt Niedermayr <
> > > > > > > > > benedikt.niedermayr@siemens.com
> > > > > > > > > ---
> > > > > > > > >  .../bindings/memory-controllers/ti,gpmc-
> > > > > > > > > child.yaml         | 
> > > > > > > > > 7
> > > > > > > > > +++++++
> > > > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git
> > > > > > > > > a/Documentation/devicetree/bindings/memory-
> > > > > > > > > controllers/ti,gpmc-child.yaml
> > > > > > > > > b/Documentation/devicetree/bindings/memory-
> > > > > > > > > controllers/ti,gpmc-
> > > > > > > > > child.yaml
> > > > > > > > > index 6e3995bb1630..7c721206f10b 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/memory-
> > > > > > > > > controllers/ti,gpmc-
> > > > > > > > > child.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-
> > > > > > > > > controllers/ti,gpmc-
> > > > > > > > > child.yaml
> > > > > > > > > @@ -230,6 +230,13 @@ properties:
> > > > > > > > >        Wait-pin used by client. Must be less than
> > > > > > > > > "gpmc,num-
> > > > > > > > > waitpins".
> > > > > > > > >      $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > > >  
> > > > > > > > > +  gpmc,wait-pin-polarity:
> > > > > > > > > +    description: |
> > > > > > > > > +      Wait-pin polarity used by the clien. It
> > > > > > > > > relates to
> > > > > > > > > the
> > > > > > > > > pin
> > > > > > > > > defined
> > > > > > > > 
> > > > > > > > did you mean "client?"
> > > > > > > > Can you please specify what value is for Active Low vs
> > > > > > > > Active
> > > > > > > > High?
> > > > > > > 
> > > > > > > Yes, that makes sense. And yes I meant "client". My
> > > > > > > typo.....
> > > > > > > > > +      with "gpmc,wait-pin".
> > > > > > > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > > > > > 
> > > > > > > > Why can't type be boolean?
> > > > > > > 
> > > > > > > Of course we can use the boolean there. In that case I
> > > > > > > should
> > > > > > > give the
> > > > > > > property a more meaningful name e.g. wait-pin-active-high 
> > > > > > > or
> > > > > > > wait-pin-
> > > > > > > active-low. 
> > > > > > > Since the default behavour of this pin is Active High,
> > > > > > > a bool property "gpmc,wait-pin-active-low" would make
> > > > > > > more
> > > > > > > sense
> > > > > > > for
> > > > > > > backwards compatibility. 
> > > > > > > If the property is missing, than the polarity stays on
> > > > > > > Active
> > > > > > > High like
> > > > > > > before.
> > > > > > > 
> > > > > > 
> > > > > > OK, in that case you don't have to clarify the polarity in
> > > > > > description.
> > > > > 
> > > > > I don't understand (and it is not explained in commit msg),
> > > > > why
> > > > > do
> > > > > you
> > > > > need such property instead of using standard GPIO flags.
> > > > > 
> > > > > The driver should use standard GPIO descriptor and standard
> > > > > bindings.
> > > > > If
> > > > > it cannot, this has to be explained.
> > > > > 
> > > > > Best regards,
> > > > > Krzysztof
> > > > 
> > > > I think this is beacause the GPMC controller itself is not
> > > > respecting
> > > > the GPIO flags. Instead the GPMC is reading the Line Level
> > > > directly
> > > > (high,low) and then evaluates the logic depending how
> > > > the WAIT<x>PINPOLARITY bit is set in the GPMPC_CONFIG register.
> > > > 
> > > > Until now gpiochip_request_own_desc() was hardcorded
> > > > to GPIO_ACTIVE_HIGH. An the GPMC_CONFIG register configuration
> > > > has
> > > > no
> > > > relation to the GPIO setting (in the current implementation).
> > > > My first approach was to make this part configurable via a new
> > > > device
> > > > tree property (wait-pin-polarity).
> > > > 
> > > > IMHO (correct me if I'm wrong) the current implementation also
> > > > does
> > > > not
> > > > make ues of standart GPIO bindings and defines the wait pin via
> > > > a
> > > > separate "gpmc,waitpin" binding.
> > > > 
> > > > E.g. gpmc,watipin = <0> or gpmc,waitpin=<1>
> > > > 
> > > > The best solution would should be when setting the binding this
> > > > way
> > > > for
> > > > example: gpmc,wait-pin = <&gpiox y ACTIVE_X>
> > > 
> > > Yes and I am afraid this will grow instead of adding proper GPIO
> > > usage.
> > > Any reason why it cannot be a standard GPIO pin desc?
> > 
> > This change (gpmc,wait-pin = <&gpiox y ACTIVE_X>) may break current
> > implementations when the GPMC is used with NAND devices. It comes
> > to
> > some configuration in the GPMC_CONFIG register when IRQs are setup
> > in Nand Mode. 
> > 
> > But when using the Nand mode the register configuration in question
> > is
> > properly configured, but in a complete different context:
> > 
> > E.g. in am335x-baltos.dtsi:
> 
> Let me clarify a bit.
> 
> The GPMC subsystem has one wait_pin per Chip select region. Some SoCs
> may have 2 Chip Selects some may have more.
> 
> Each wait_pin can be used in 2 ways currently.
> 1) As a General purpose GPIO (GPI actually as output not supported)
> via the Linux GPIO subsystem
> 2) As a wait state signalling for memory interface independently to
> Linux GPIO subsystem. Via GPMC configuration.
> 
> > interrupts = <0 IRQ_TYPE_NONE>, /* fifoevent */
> >              <1
> > IRQ_TYPE_NONE>; /* termcount */
> > rb-gpios = <&gpmc 0 GPIO_ACTIVE_HIGH>;
> 
> The rb-gpios is an example of (1) I listed above. Here you can use
> the standard GPIO polarity specifier and it should work currently.
> 
> An example of (2) you can see in omap3-devkit8000-common.dtsi:
>         ethernet@6,0 {
>                 compatible = "davicom,dm9000";
> 		...
>                 gpmc,mux-add-data = <0>;
>                 gpmc,device-width = <1>;
> -->             gpmc,wait-pin = <0>;
> 		..
>         };
> 
> Here we set the GPMC hardware configuration to use wait_pin of
> Chip Select 0 to add wait state to each bus access cycle to the
> external Ethernet device. Linux GPIO subsystem has no role to play
> here and everything is dealt with in GPMC hardware.
> 
> Now what this current patch series is trying to do is to add a
> polarity specifier to the use case (2).
> There is a GPMC hardware setting to decide the polarity of the
> wait_pin.
> The code just needs to get the polarity setting from DT and set
> this setting correctly.
> I don't think this has got to do anything with GPIO as it is very
> specific
> to GPMC configuration. So the vendor specific property, "gpmc,wait-
> pin-active-low"
> is appropriate I think.
> 
> Hope this clarifies everything ;)
> 
Thanks roger for clarification!

It's a bit confusing that both ways work (GPI & direct GPMC)
configuration. And yes, my patches are based on the direct
configuration of GPMC, which has nothing to do with GPIO.

The more I dig into the code myself the better I understand things and
the better I now how to explain those things.

I think it can be seen somthing similar to the SPI subsystem, where you
can on the one hand use SPI hw-chipselects which are directly
controlled by the SPI controller and not by the GPIO subsystem. and on
the other hand use GPIO chipselects which where handled by the GPIO
subsystem.

So would propose to refactor the patches and send a v3.
 
> 
> > /* gpmc_wait0 */
> > 
> > The "interrupts" property will configure the GPMC_CONFIG register
> > bits
> > for the waitpins. 
> > 
> > But in a non-NAND case, the "interrupt" configuration wouldn't make
> > any
> > sense, since interrupts are not used at all.
> > 
> > The "rb-gpios" is *not* handled by the omap-gpmc(maybe somewhere in
> > the
> > NAND subsystem?). 
> > 
> > Changing the wait-pin property to "gpmc,wait-pin = <&gpmc X
> > ACTIVE_X>"
> > will currently break at least 3 device trees:
> > 
> > arch/arm/boot/dts/omap3-devkit8000-common.dtsi
> > arch/arm/boot/dts/omap-zoom-common.dtsi
> > arch/arm/boot/dts/omap3-lilly-a83x.dtsi
> > 
> >  
> > I think it makes sense to implement a v3 as POC?
> > 
> > > > But I think the current omap-gpmc.c implementation does not
> > > > offer
> > > > such
> > > > a usecase and as roger already mentioned: 
> > > > "GPMC wait_pin polarity logic is hard-wired and doesn't depend
> > > > on
> > > > GPIO
> > > > subsystem for its polarity"
> > > 
> > > This part I don't get. You mean hard-wired in the driver or hard-
> > > wired
> > > in the hardware? If the first, please un-wire it. If the latter,
> > > your
> > > property makes no sense, right?
> > > 
> > IMHO roger meant that configuring the GPMC registers via gpiolib
> > callbacks would be the wrong place to implementent. I implemented 
> > the gpmc register configuration in the gpio_direction_input
> > function.
> > 
> > 
> > > Best regards,
> > > Krzysztof
> 
> cheers,
> -roger

cheers,
benedikt