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

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

[B. Niedermayr]

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

Currently it is not possible to configure the WAIT0PINPOLARITY and 
WAIT1PINPOLARITY bits of the GPMC_CONFIG register directly via 
device tree properties.

It is also not possible to 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
  * Remove the gpmc register configuration out of the gpiochip
    callbacks. In this case the memory interface configuration
    is not done via gpio bindings.
  * Some minor code fixes
  * Changed git commit descriptions


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 wait-pin polarity

 .../memory-controllers/ti,gpmc-child.yaml     |  6 +++++
 drivers/memory/omap-gpmc.c                    | 23 +++++++++++++++++--
 include/linux/platform_data/gpmc-omap.h       |  1 +
 3 files changed, 28 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v3 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 d9bf1c2ac319..e3674a15b934 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -2221,9 +2221,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 v3 2/3] memory: omap-gpmc: add support for wait pin polarity

[B. Niedermayr]

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

The waitpin polarity can be configured via the WAITPIN<X>POLARITY bits
in the GPMC_CONFIG register. This is currently not supported by the
driver. This patch adds support for setting the required register bits
with the "gpmc,wait-pin-active-low" device tree property.

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

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index e3674a15b934..609df21e0ce6 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -132,6 +132,7 @@
 #define GPMC_CONFIG_DEV_SIZE	0x00000002
 #define GPMC_CONFIG_DEV_TYPE	0x00000003
 
+#define GPMC_CONFIG_WAITPINPOLARITY(pin)	(BIT(pin) << 8)
 #define GPMC_CONFIG1_WRAPBURST_SUPP     (1 << 31)
 #define GPMC_CONFIG1_READMULTIPLE_SUPP  (1 << 30)
 #define GPMC_CONFIG1_READTYPE_ASYNC     (0 << 29)
@@ -1881,6 +1882,18 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p)
 
 	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1);
 
+	if (p->wait_on_read || p->wait_on_write) {
+		config1 = gpmc_read_reg(GPMC_CONFIG);
+
+		if (p->wait_pin_active_low)
+			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
+		else
+			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
+
+		gpmc_write_reg(GPMC_CONFIG, config1);
+	}
+
+
 	return 0;
 }
 
@@ -1985,6 +1998,8 @@ 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_active_low = of_property_read_bool(np,
+								"gpmc,wait-pin-active-low");
 		if (!p->wait_on_read && !p->wait_on_write)
 			pr_debug("%s: rd/wr wait monitoring not enabled!\n",
 				 __func__);
diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
index c9cc4e32435d..7c9675165e8a 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 */
+	bool wait_pin_active_low;	/* wait-pin polarity */
 };
 
 /* Data for each chip select */
-- 
2.25.1

[PATCH 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 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Roger Quadros]

On 06/09/2022 15:47, 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:

gpmc,wait-pin-active-low ?

> +    description: |
> +      Wait-pin polarity used by the clien. It relates to the pin defined
> +      with "gpmc,wait-pin".

Please update description accordingly.

> +    $ref: /schemas/types.yaml#/definitions/uint32

boolean type

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


cheers,
-roger

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

[Niedermayr, BENEDIKT]

On Thu, 2022-09-08 at 15:03 +0300, Roger Quadros wrote:
> 
> On 06/09/2022 15:47, 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:
> 
> gpmc,wait-pin-active-low ?
> 
> > +    description: |
> > +      Wait-pin polarity used by the clien. It relates to the pin
> > defined
> > +      with "gpmc,wait-pin".
> 
> Please update description accordingly.
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> 
> boolean type
> 
> > +    default: 0
> > +
> >    gpmc,wait-on-read:
> >      description: Enables wait monitoring on reads.
> >      type: boolean
> 
> cheers,
> -roger
Sorry, 

this patch was an fragment of the old patch series, and can be skipped.


cheers,
benedikt

[PATCH v3 3/3] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity

[B. Niedermayr]

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

The GPMC controller has the ability to configure the polarity for the
wait pin. The current properties do not allow this configuration.
This binding directly configures the WAITPIN<X>POLARITY bit
in the GPMC_CONFIG register.

Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
---
 .../bindings/memory-controllers/ti,gpmc-child.yaml          | 6 ++++++
 1 file changed, 6 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..a115b544a407 100644
--- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
@@ -230,6 +230,12 @@ properties:
       Wait-pin used by client. Must be less than "gpmc,num-waitpins".
     $ref: /schemas/types.yaml#/definitions/uint32
 
+  gpmc,wait-pin-active-low:
+    description: |
+      Set the polarity for the selected wait pin to active low.
+      Defaults to active high if this is not set.
+    type: boolean
+
   gpmc,wait-on-read:
     description: Enables wait monitoring on reads.
     type: boolean
-- 
2.25.1

Re: [PATCH v3 3/3] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity

[Roger Quadros]

Benedikt,


On 06/09/2022 15:47, B. Niedermayr wrote:
> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> 
> The GPMC controller has the ability to configure the polarity for the
> wait pin. The current properties do not allow this configuration.
> This binding directly configures the WAITPIN<X>POLARITY bit
> in the GPMC_CONFIG register.
> 
> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> ---
>  .../bindings/memory-controllers/ti,gpmc-child.yaml          | 6 ++++++
>  1 file changed, 6 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..a115b544a407 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml
> @@ -230,6 +230,12 @@ properties:
>        Wait-pin used by client. Must be less than "gpmc,num-waitpins".
>      $ref: /schemas/types.yaml#/definitions/uint32
>  
> +  gpmc,wait-pin-active-low:
> +    description: |
> +      Set the polarity for the selected wait pin to active low.
> +      Defaults to active high if this is not set.
> +    type: boolean
> +

I just checked that the default behaviour is active low.
Reset value of the polarity register field is 0, which means active low.

We will need to use the property "gpmc,wait-pin-active-high" instead.

Sorry for not catching this earlier.

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

cheers,
-roger

Re: [PATCH v3 3/3] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity

[Niedermayr, BENEDIKT]

On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote:
> Benedikt,
> 
> 
> On 06/09/2022 15:47, B. Niedermayr wrote:
> > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> > 
> > The GPMC controller has the ability to configure the polarity for
> > the
> > wait pin. The current properties do not allow this configuration.
> > This binding directly configures the WAITPIN<X>POLARITY bit
> > in the GPMC_CONFIG register.
> > 
> > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com
> > >
> > ---
> >  .../bindings/memory-controllers/ti,gpmc-child.yaml          | 6
> > ++++++
> >  1 file changed, 6 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..a115b544a407 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
> > child.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
> > child.yaml
> > @@ -230,6 +230,12 @@ properties:
> >        Wait-pin used by client. Must be less than "gpmc,num-
> > waitpins".
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >  
> > +  gpmc,wait-pin-active-low:
> > +    description: |
> > +      Set the polarity for the selected wait pin to active low.
> > +      Defaults to active high if this is not set.
> > +    type: boolean
> > +
> 
> I just checked that the default behaviour is active low.
> Reset value of the polarity register field is 0, which means active
> low.
> 
> We will need to use the property "gpmc,wait-pin-active-high" instead.
> 
> Sorry for not catching this earlier.

It's ok. No worries.

Well, the Datasheets are telling me different reset values here. 
The am335x TRM (Rev. Q) defines the reset value of WAIT1PINPOLARITY as
0x0, whereas the am64x TRM (Rev. C) defines the reset value of WAIT1PIN
POLARITY as 0x1. The am64x TRM also defines different reset values for 
WAIT0PINPOLARITY and WAIT1PINPOLARITY.

The interesting thing is that I'm currently working on an am335x
platform and I dumped the GPMC_CONFIG register and got 0x00000a00
(WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM specifies.


Nevertheless, I'm setting the WAITXPINPOLARITY bits in both cases
accordingly.  
0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case
"gpmc,wait-pin-active-low" is not set. So the reset value is always
overwritten.


Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin-active-low
" is also ok for me, but it feels more like a cosmetic thing at this
point. 



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

cheers,
benedikt

Re: [PATCH v3 3/3] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity

[Roger Quadros]

Benedikt,

On 12/09/2022 10:43, Niedermayr, BENEDIKT wrote:
> On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote:
>> Benedikt,
>>
>>
>> On 06/09/2022 15:47, B. Niedermayr wrote:
>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>
>>> The GPMC controller has the ability to configure the polarity for
>>> the
>>> wait pin. The current properties do not allow this configuration.
>>> This binding directly configures the WAITPIN<X>POLARITY bit
>>> in the GPMC_CONFIG register.
>>>
>>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com
>>>>
>>> ---
>>>  .../bindings/memory-controllers/ti,gpmc-child.yaml          | 6
>>> ++++++
>>>  1 file changed, 6 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..a115b544a407 100644
>>> --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>> child.yaml
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>> child.yaml
>>> @@ -230,6 +230,12 @@ properties:
>>>        Wait-pin used by client. Must be less than "gpmc,num-
>>> waitpins".
>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>  
>>> +  gpmc,wait-pin-active-low:
>>> +    description: |
>>> +      Set the polarity for the selected wait pin to active low.
>>> +      Defaults to active high if this is not set.
>>> +    type: boolean
>>> +
>>
>> I just checked that the default behaviour is active low.
>> Reset value of the polarity register field is 0, which means active
>> low.
>>
>> We will need to use the property "gpmc,wait-pin-active-high" instead.
>>
>> Sorry for not catching this earlier.
> 
> It's ok. No worries.
> 
> Well, the Datasheets are telling me different reset values here. 
> The am335x TRM (Rev. Q) defines the reset value of WAIT1PINPOLARITY as
> 0x0, whereas the am64x TRM (Rev. C) defines the reset value of WAIT1PIN
> POLARITY as 0x1. The am64x TRM also defines different reset values for 
> WAIT0PINPOLARITY and WAIT1PINPOLARITY.
> 
> The interesting thing is that I'm currently working on an am335x
> platform and I dumped the GPMC_CONFIG register and got 0x00000a00
> (WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM specifies.

I can confirm the same behaviour on am642 EVM as well.
I get 0xa00 on reading GPMC_CONFIG.

> 
> 
> Nevertheless, I'm setting the WAITXPINPOLARITY bits in both cases
> accordingly.  
> 0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case
> "gpmc,wait-pin-active-low" is not set. So the reset value is always
> overwritten.
> 
> 
> Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin-active-low
> " is also ok for me, but it feels more like a cosmetic thing at this
> point. 

My main concern is for legacy platforms not specifying the property in DT.
Earlier we were not touching the WAITPINPOLARITY config and now we are
so we might break some legacy platforms that don't specify
the polarity and we flip it here.

Fortunately, there are only few boards using gpmc wait-pin and mostly wait-pin 0
for which there is no discrepancy as far as wait-pin reset value is concerned.

logicpd-torpedo-baseboard.dtsi:		gpmc,wait-pin = <0>;
omap3-devkit8000-common.dtsi:		gpmc,wait-pin = <0>;
Binary file omap3-devkit8000.dtb matches
Binary file omap3-devkit8000-lcd43.dtb matches
Binary file omap3-devkit8000-lcd70.dtb matches
omap3-lilly-a83x.dtsi:		gpmc,wait-pin = <0>;
Binary file omap3-lilly-dbb056.dtb matches
Binary file omap3-zoom3.dtb matches

Only 1 board is using wait-pin 1
omap-zoom-common.dtsi:		gpmc,wait-pin = <1>;

from OMP36xx TRM, here are the reset values
WAIT3PINPOLARITY 0x1
WAIT2PINPOLARITY 0x0
WAIT1PINPOLARITY 0x1
WAIT0PINPOLARITY 0x0

cheers,
-roger

Re: [PATCH v3 3/3] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity

[Niedermayr, BENEDIKT]

Roger,

On Mon, 2022-09-12 at 14:04 +0300, Roger Quadros wrote:
> Benedikt,
> 
> On 12/09/2022 10:43, Niedermayr, BENEDIKT wrote:
> > On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote:
> > > Benedikt,
> > > 
> > > 
> > > On 06/09/2022 15:47, B. Niedermayr wrote:
> > > > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> > > > 
> > > > The GPMC controller has the ability to configure the polarity
> > > > for
> > > > the
> > > > wait pin. The current properties do not allow this
> > > > configuration.
> > > > This binding directly configures the WAITPIN<X>POLARITY bit
> > > > in the GPMC_CONFIG register.
> > > > 
> > > > Signed-off-by: Benedikt Niedermayr <
> > > > benedikt.niedermayr@siemens.com
> > > > ---
> > > >  .../bindings/memory-controllers/ti,gpmc-child.yaml          |
> > > > 6
> > > > ++++++
> > > >  1 file changed, 6 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..a115b544a407 100644
> > > > --- a/Documentation/devicetree/bindings/memory-
> > > > controllers/ti,gpmc-
> > > > child.yaml
> > > > +++ b/Documentation/devicetree/bindings/memory-
> > > > controllers/ti,gpmc-
> > > > child.yaml
> > > > @@ -230,6 +230,12 @@ properties:
> > > >        Wait-pin used by client. Must be less than "gpmc,num-
> > > > waitpins".
> > > >      $ref: /schemas/types.yaml#/definitions/uint32
> > > >  
> > > > +  gpmc,wait-pin-active-low:
> > > > +    description: |
> > > > +      Set the polarity for the selected wait pin to active
> > > > low.
> > > > +      Defaults to active high if this is not set.
> > > > +    type: boolean
> > > > +
> > > 
> > > I just checked that the default behaviour is active low.
> > > Reset value of the polarity register field is 0, which means
> > > active
> > > low.
> > > 
> > > We will need to use the property "gpmc,wait-pin-active-high"
> > > instead.
> > > 
> > > Sorry for not catching this earlier.
> > 
> > It's ok. No worries.
> > 
> > Well, the Datasheets are telling me different reset values here. 
> > The am335x TRM (Rev. Q) defines the reset value of WAIT1PINPOLARITY
> > as
> > 0x0, whereas the am64x TRM (Rev. C) defines the reset value of
> > WAIT1PIN
> > POLARITY as 0x1. The am64x TRM also defines different reset values
> > for 
> > WAIT0PINPOLARITY and WAIT1PINPOLARITY.
> > 
> > The interesting thing is that I'm currently working on an am335x
> > platform and I dumped the GPMC_CONFIG register and got 0x00000a00
> > (WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM
> > specifies.
> 
> I can confirm the same behaviour on am642 EVM as well.
> I get 0xa00 on reading GPMC_CONFIG.
> 
> > 
> > Nevertheless, I'm setting the WAITXPINPOLARITY bits in both cases
> > accordingly.  
> > 0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case
> > "gpmc,wait-pin-active-low" is not set. So the reset value is always
> > overwritten.
> > 
> > 
> > Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin-
> > active-low
> > " is also ok for me, but it feels more like a cosmetic thing at
> > this
> > point. 
> 
> My main concern is for legacy platforms not specifying the property
> in DT.
> Earlier we were not touching the WAITPINPOLARITY config and now we
> are
> so we might break some legacy platforms that don't specify
> the polarity and we flip it here.
> 
> Fortunately, there are only few boards using gpmc wait-pin and mostly
> wait-pin 0
> for which there is no discrepancy as far as wait-pin reset value is
> concerned.
> 
> logicpd-torpedo-baseboard.dtsi:		gpmc,wait-pin = <0>;
> omap3-devkit8000-common.dtsi:		gpmc,wait-pin = <0>;
> Binary file omap3-devkit8000.dtb matches
> Binary file omap3-devkit8000-lcd43.dtb matches
> Binary file omap3-devkit8000-lcd70.dtb matches
> omap3-lilly-a83x.dtsi:		gpmc,wait-pin = <0>;
> Binary file omap3-lilly-dbb056.dtb matches
> Binary file omap3-zoom3.dtb matches
> 
> Only 1 board is using wait-pin 1
> omap-zoom-common.dtsi:		gpmc,wait-pin = <1>;
> 
> from OMP36xx TRM, here are the reset values
> WAIT3PINPOLARITY 0x1
> WAIT2PINPOLARITY 0x0
> WAIT1PINPOLARITY 0x1
> WAIT0PINPOLARITY 0x0

Ah ok. The picture is getting clearer.

Does it make sense then not to use a boolean property in that case?
With a boolean property we are only able to change the polarity bits
into one direction (0 -> 1 or 1 -> 0) but we have different reset
values for each bit.

This part of my patch may then break the mentioned legacy platforms
because it even overwrites the register in case the property is not
set:


+	if (p->wait_pin_active_low)
+		config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
+	else
+		config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
+
+	gpmc_write_reg(GPMC_CONFIG, config1);


So in order to preserve compatibility as well as the possibility to
change the polarity bits into the desired value I would propose to use
an uint32 value for the desired value and in case the dt-property is
not set we should not touch the register at all.


> 
> cheers,
> -roger

cheers,
benedikt

Re: [PATCH v3 3/3] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity

[Roger Quadros]

Benedikt,

On 13/09/2022 11:23, Niedermayr, BENEDIKT wrote:
> Roger,
> 
> On Mon, 2022-09-12 at 14:04 +0300, Roger Quadros wrote:
>> Benedikt,
>>
>> On 12/09/2022 10:43, Niedermayr, BENEDIKT wrote:
>>> On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote:
>>>> Benedikt,
>>>>
>>>>
>>>> On 06/09/2022 15:47, B. Niedermayr wrote:
>>>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>>>
>>>>> The GPMC controller has the ability to configure the polarity
>>>>> for
>>>>> the
>>>>> wait pin. The current properties do not allow this
>>>>> configuration.
>>>>> This binding directly configures the WAITPIN<X>POLARITY bit
>>>>> in the GPMC_CONFIG register.
>>>>>
>>>>> Signed-off-by: Benedikt Niedermayr <
>>>>> benedikt.niedermayr@siemens.com
>>>>> ---
>>>>>  .../bindings/memory-controllers/ti,gpmc-child.yaml          |
>>>>> 6
>>>>> ++++++
>>>>>  1 file changed, 6 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..a115b544a407 100644
>>>>> --- a/Documentation/devicetree/bindings/memory-
>>>>> controllers/ti,gpmc-
>>>>> child.yaml
>>>>> +++ b/Documentation/devicetree/bindings/memory-
>>>>> controllers/ti,gpmc-
>>>>> child.yaml
>>>>> @@ -230,6 +230,12 @@ properties:
>>>>>        Wait-pin used by client. Must be less than "gpmc,num-
>>>>> waitpins".
>>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>>  
>>>>> +  gpmc,wait-pin-active-low:
>>>>> +    description: |
>>>>> +      Set the polarity for the selected wait pin to active
>>>>> low.
>>>>> +      Defaults to active high if this is not set.
>>>>> +    type: boolean
>>>>> +
>>>>
>>>> I just checked that the default behaviour is active low.
>>>> Reset value of the polarity register field is 0, which means
>>>> active
>>>> low.
>>>>
>>>> We will need to use the property "gpmc,wait-pin-active-high"
>>>> instead.
>>>>
>>>> Sorry for not catching this earlier.
>>>
>>> It's ok. No worries.
>>>
>>> Well, the Datasheets are telling me different reset values here. 
>>> The am335x TRM (Rev. Q) defines the reset value of WAIT1PINPOLARITY
>>> as
>>> 0x0, whereas the am64x TRM (Rev. C) defines the reset value of
>>> WAIT1PIN
>>> POLARITY as 0x1. The am64x TRM also defines different reset values
>>> for 
>>> WAIT0PINPOLARITY and WAIT1PINPOLARITY.
>>>
>>> The interesting thing is that I'm currently working on an am335x
>>> platform and I dumped the GPMC_CONFIG register and got 0x00000a00
>>> (WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM
>>> specifies.
>>
>> I can confirm the same behaviour on am642 EVM as well.
>> I get 0xa00 on reading GPMC_CONFIG.
>>
>>>
>>> Nevertheless, I'm setting the WAITXPINPOLARITY bits in both cases
>>> accordingly.  
>>> 0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case
>>> "gpmc,wait-pin-active-low" is not set. So the reset value is always
>>> overwritten.
>>>
>>>
>>> Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin-
>>> active-low
>>> " is also ok for me, but it feels more like a cosmetic thing at
>>> this
>>> point. 
>>
>> My main concern is for legacy platforms not specifying the property
>> in DT.
>> Earlier we were not touching the WAITPINPOLARITY config and now we
>> are
>> so we might break some legacy platforms that don't specify
>> the polarity and we flip it here.
>>
>> Fortunately, there are only few boards using gpmc wait-pin and mostly
>> wait-pin 0
>> for which there is no discrepancy as far as wait-pin reset value is
>> concerned.
>>
>> logicpd-torpedo-baseboard.dtsi:		gpmc,wait-pin = <0>;
>> omap3-devkit8000-common.dtsi:		gpmc,wait-pin = <0>;
>> Binary file omap3-devkit8000.dtb matches
>> Binary file omap3-devkit8000-lcd43.dtb matches
>> Binary file omap3-devkit8000-lcd70.dtb matches
>> omap3-lilly-a83x.dtsi:		gpmc,wait-pin = <0>;
>> Binary file omap3-lilly-dbb056.dtb matches
>> Binary file omap3-zoom3.dtb matches
>>
>> Only 1 board is using wait-pin 1
>> omap-zoom-common.dtsi:		gpmc,wait-pin = <1>;
>>
>> from OMP36xx TRM, here are the reset values
>> WAIT3PINPOLARITY 0x1
>> WAIT2PINPOLARITY 0x0
>> WAIT1PINPOLARITY 0x1
>> WAIT0PINPOLARITY 0x0
> 
> Ah ok. The picture is getting clearer.
> 
> Does it make sense then not to use a boolean property in that case?
> With a boolean property we are only able to change the polarity bits
> into one direction (0 -> 1 or 1 -> 0) but we have different reset
> values for each bit.
> 
> This part of my patch may then break the mentioned legacy platforms
> because it even overwrites the register in case the property is not
> set:
> 
> 
> +	if (p->wait_pin_active_low)
> +		config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> +	else
> +		config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> +
> +	gpmc_write_reg(GPMC_CONFIG, config1);
> 
> 
> So in order to preserve compatibility as well as the possibility to
> change the polarity bits into the desired value I would propose to use
> an uint32 value for the desired value and in case the dt-property is
> not set we should not touch the register at all.

I'm sorry I didn't understand how uint32 value solves this issue.
Could you please explain with a DT example?

cheers,
-roger

Re: [PATCH v3 3/3] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity

[Niedermayr, BENEDIKT]

Roger,

On Tue, 2022-09-13 at 16:18 +0300, Roger Quadros wrote:
> Benedikt,
> 
> On 13/09/2022 11:23, Niedermayr, BENEDIKT wrote:
> > Roger,
> > 
> > On Mon, 2022-09-12 at 14:04 +0300, Roger Quadros wrote:
> > > Benedikt,
> > > 
> > > On 12/09/2022 10:43, Niedermayr, BENEDIKT wrote:
> > > > On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote:
> > > > > Benedikt,
> > > > > 
> > > > > 
> > > > > On 06/09/2022 15:47, B. Niedermayr wrote:
> > > > > > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> > > > > > 
> > > > > > The GPMC controller has the ability to configure the
> > > > > > polarity
> > > > > > for
> > > > > > the
> > > > > > wait pin. The current properties do not allow this
> > > > > > configuration.
> > > > > > This binding directly configures the WAITPIN<X>POLARITY bit
> > > > > > in the GPMC_CONFIG register.
> > > > > > 
> > > > > > Signed-off-by: Benedikt Niedermayr <
> > > > > > benedikt.niedermayr@siemens.com
> > > > > > ---
> > > > > >  .../bindings/memory-controllers/ti,gpmc-
> > > > > > child.yaml          |
> > > > > > 6
> > > > > > ++++++
> > > > > >  1 file changed, 6 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..a115b544a407 100644
> > > > > > --- a/Documentation/devicetree/bindings/memory-
> > > > > > controllers/ti,gpmc-
> > > > > > child.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/memory-
> > > > > > controllers/ti,gpmc-
> > > > > > child.yaml
> > > > > > @@ -230,6 +230,12 @@ properties:
> > > > > >        Wait-pin used by client. Must be less than
> > > > > > "gpmc,num-
> > > > > > waitpins".
> > > > > >      $ref: /schemas/types.yaml#/definitions/uint32
> > > > > >  
> > > > > > +  gpmc,wait-pin-active-low:
> > > > > > +    description: |
> > > > > > +      Set the polarity for the selected wait pin to active
> > > > > > low.
> > > > > > +      Defaults to active high if this is not set.
> > > > > > +    type: boolean
> > > > > > +
> > > > > 
> > > > > I just checked that the default behaviour is active low.
> > > > > Reset value of the polarity register field is 0, which means
> > > > > active
> > > > > low.
> > > > > 
> > > > > We will need to use the property "gpmc,wait-pin-active-high"
> > > > > instead.
> > > > > 
> > > > > Sorry for not catching this earlier.
> > > > 
> > > > It's ok. No worries.
> > > > 
> > > > Well, the Datasheets are telling me different reset values
> > > > here. 
> > > > The am335x TRM (Rev. Q) defines the reset value of
> > > > WAIT1PINPOLARITY
> > > > as
> > > > 0x0, whereas the am64x TRM (Rev. C) defines the reset value of
> > > > WAIT1PIN
> > > > POLARITY as 0x1. The am64x TRM also defines different reset
> > > > values
> > > > for 
> > > > WAIT0PINPOLARITY and WAIT1PINPOLARITY.
> > > > 
> > > > The interesting thing is that I'm currently working on an
> > > > am335x
> > > > platform and I dumped the GPMC_CONFIG register and got
> > > > 0x00000a00
> > > > (WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM
> > > > specifies.
> > > 
> > > I can confirm the same behaviour on am642 EVM as well.
> > > I get 0xa00 on reading GPMC_CONFIG.
> > > 
> > > > Nevertheless, I'm setting the WAITXPINPOLARITY bits in both
> > > > cases
> > > > accordingly.  
> > > > 0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case
> > > > "gpmc,wait-pin-active-low" is not set. So the reset value is
> > > > always
> > > > overwritten.
> > > > 
> > > > 
> > > > Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin-
> > > > active-low
> > > > " is also ok for me, but it feels more like a cosmetic thing at
> > > > this
> > > > point. 
> > > 
> > > My main concern is for legacy platforms not specifying the
> > > property
> > > in DT.
> > > Earlier we were not touching the WAITPINPOLARITY config and now
> > > we
> > > are
> > > so we might break some legacy platforms that don't specify
> > > the polarity and we flip it here.
> > > 
> > > Fortunately, there are only few boards using gpmc wait-pin and
> > > mostly
> > > wait-pin 0
> > > for which there is no discrepancy as far as wait-pin reset value
> > > is
> > > concerned.
> > > 
> > > logicpd-torpedo-baseboard.dtsi:		gpmc,wait-pin = <0>;
> > > omap3-devkit8000-common.dtsi:		gpmc,wait-pin = <0>;
> > > Binary file omap3-devkit8000.dtb matches
> > > Binary file omap3-devkit8000-lcd43.dtb matches
> > > Binary file omap3-devkit8000-lcd70.dtb matches
> > > omap3-lilly-a83x.dtsi:		gpmc,wait-pin = <0>;
> > > Binary file omap3-lilly-dbb056.dtb matches
> > > Binary file omap3-zoom3.dtb matches
> > > 
> > > Only 1 board is using wait-pin 1
> > > omap-zoom-common.dtsi:		gpmc,wait-pin = <1>;
> > > 
> > > from OMP36xx TRM, here are the reset values
> > > WAIT3PINPOLARITY 0x1
> > > WAIT2PINPOLARITY 0x0
> > > WAIT1PINPOLARITY 0x1
> > > WAIT0PINPOLARITY 0x0
> > 
> > Ah ok. The picture is getting clearer.
> > 
> > Does it make sense then not to use a boolean property in that case?
> > With a boolean property we are only able to change the polarity
> > bits
> > into one direction (0 -> 1 or 1 -> 0) but we have different reset
> > values for each bit.
> > 
> > This part of my patch may then break the mentioned legacy platforms
> > because it even overwrites the register in case the property is not
> > set:
> > 
> > 
> > +	if (p->wait_pin_active_low)
> > +		config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> > +	else
> > +		config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> > +
> > +	gpmc_write_reg(GPMC_CONFIG, config1);
> > 
> > 
> > So in order to preserve compatibility as well as the possibility to
> > change the polarity bits into the desired value I would propose to
> > use
> > an uint32 value for the desired value and in case the dt-property
> > is
> > not set we should not touch the register at all.
> 
> I'm sorry I didn't understand how uint32 value solves this issue.
> Could you please explain with a DT example?

I meant a similar implementation like in my first patchseries.

Just a example:

dts:

gpmc {

    foo0@0 {
        gpmc,wait-pin = <0>;
        gpmc,wait-pin-polarity = <0>;  /* active low */
    };
    
    bar0@1 {
        gpmc,wait-pin = <1>;
        gpmc,wait-pin-polarity = <1>; /* active high */
    };
    
    foobar0@2 {
        gpmc,wait-pin = <2>;
        /* don't touch wait pin polarity here */
    };
};

omap-gpmc:

gpmc_read_settings_dt() 
{
  p->wait-pin_polarity = -1;  /* some init value required here */
  if (!of_property_read_u32(np, "gpmc,wait-pin-polarity", &p->wait-pin_polarity) {

  ....
  } 
}

gpmc_cs_program_settings() 
{
   if (p->wait_pin_polarity == 0)
     config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
   if (p->wait_pin_polarity == 1)
     config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
}

This should met all requirements.

If "gpmc,wait-pin-polarity" is not set in the device tree, then the
registers stay untouched. 

If it is set, then the WAIT<X>PINPOLARITY bit is set accordingly.


On the OMP36xx platform for example we have want to set all wait pin
polarities to active low (0) and have following register reset values:

WAIT3PINPOLARITY 0x1
WAIT2PINPOLARITY 0x0
WAIT1PINPOLARITY 0x1
WAIT0PINPOLARITY 0x0

With an boolean "gpmc,wait-pin-active-high" property we're not able to
set WAIT3PINPOLARITY and WAIT1PINPOLARITY to 0. 
And vice versa with WAIT2PINPOLARITY and WAIT0PINPOLARITY if we want
to 
set them to active high (1) and only would have a "gpmc,wait-pin-
active-low" property.

I hope this clarifies my proposal.

> 
cheers,
-roger

cheers,
benedikt

Re: [PATCH v3 3/3] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity

[Roger Quadros]

Benedikt,

On 13/09/2022 23:56, Niedermayr, BENEDIKT wrote:
> Roger,
> 
> On Tue, 2022-09-13 at 16:18 +0300, Roger Quadros wrote:
>> Benedikt,
>>
>> On 13/09/2022 11:23, Niedermayr, BENEDIKT wrote:
>>> Roger,
>>>
>>> On Mon, 2022-09-12 at 14:04 +0300, Roger Quadros wrote:
>>>> Benedikt,
>>>>
>>>> On 12/09/2022 10:43, Niedermayr, BENEDIKT wrote:
>>>>> On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote:
>>>>>> Benedikt,
>>>>>>
>>>>>>
>>>>>> On 06/09/2022 15:47, B. Niedermayr wrote:
>>>>>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>>>>>
>>>>>>> The GPMC controller has the ability to configure the
>>>>>>> polarity
>>>>>>> for
>>>>>>> the
>>>>>>> wait pin. The current properties do not allow this
>>>>>>> configuration.
>>>>>>> This binding directly configures the WAITPIN<X>POLARITY bit
>>>>>>> in the GPMC_CONFIG register.
>>>>>>>
>>>>>>> Signed-off-by: Benedikt Niedermayr <
>>>>>>> benedikt.niedermayr@siemens.com
>>>>>>> ---
>>>>>>>  .../bindings/memory-controllers/ti,gpmc-
>>>>>>> child.yaml          |
>>>>>>> 6
>>>>>>> ++++++
>>>>>>>  1 file changed, 6 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..a115b544a407 100644
>>>>>>> --- a/Documentation/devicetree/bindings/memory-
>>>>>>> controllers/ti,gpmc-
>>>>>>> child.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/memory-
>>>>>>> controllers/ti,gpmc-
>>>>>>> child.yaml
>>>>>>> @@ -230,6 +230,12 @@ properties:
>>>>>>>        Wait-pin used by client. Must be less than
>>>>>>> "gpmc,num-
>>>>>>> waitpins".
>>>>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>>  
>>>>>>> +  gpmc,wait-pin-active-low:
>>>>>>> +    description: |
>>>>>>> +      Set the polarity for the selected wait pin to active
>>>>>>> low.
>>>>>>> +      Defaults to active high if this is not set.
>>>>>>> +    type: boolean
>>>>>>> +
>>>>>>
>>>>>> I just checked that the default behaviour is active low.
>>>>>> Reset value of the polarity register field is 0, which means
>>>>>> active
>>>>>> low.
>>>>>>
>>>>>> We will need to use the property "gpmc,wait-pin-active-high"
>>>>>> instead.
>>>>>>
>>>>>> Sorry for not catching this earlier.
>>>>>
>>>>> It's ok. No worries.
>>>>>
>>>>> Well, the Datasheets are telling me different reset values
>>>>> here. 
>>>>> The am335x TRM (Rev. Q) defines the reset value of
>>>>> WAIT1PINPOLARITY
>>>>> as
>>>>> 0x0, whereas the am64x TRM (Rev. C) defines the reset value of
>>>>> WAIT1PIN
>>>>> POLARITY as 0x1. The am64x TRM also defines different reset
>>>>> values
>>>>> for 
>>>>> WAIT0PINPOLARITY and WAIT1PINPOLARITY.
>>>>>
>>>>> The interesting thing is that I'm currently working on an
>>>>> am335x
>>>>> platform and I dumped the GPMC_CONFIG register and got
>>>>> 0x00000a00
>>>>> (WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM
>>>>> specifies.
>>>>
>>>> I can confirm the same behaviour on am642 EVM as well.
>>>> I get 0xa00 on reading GPMC_CONFIG.
>>>>
>>>>> Nevertheless, I'm setting the WAITXPINPOLARITY bits in both
>>>>> cases
>>>>> accordingly.  
>>>>> 0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case
>>>>> "gpmc,wait-pin-active-low" is not set. So the reset value is
>>>>> always
>>>>> overwritten.
>>>>>
>>>>>
>>>>> Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin-
>>>>> active-low
>>>>> " is also ok for me, but it feels more like a cosmetic thing at
>>>>> this
>>>>> point. 
>>>>
>>>> My main concern is for legacy platforms not specifying the
>>>> property
>>>> in DT.
>>>> Earlier we were not touching the WAITPINPOLARITY config and now
>>>> we
>>>> are
>>>> so we might break some legacy platforms that don't specify
>>>> the polarity and we flip it here.
>>>>
>>>> Fortunately, there are only few boards using gpmc wait-pin and
>>>> mostly
>>>> wait-pin 0
>>>> for which there is no discrepancy as far as wait-pin reset value
>>>> is
>>>> concerned.
>>>>
>>>> logicpd-torpedo-baseboard.dtsi:		gpmc,wait-pin = <0>;
>>>> omap3-devkit8000-common.dtsi:		gpmc,wait-pin = <0>;
>>>> Binary file omap3-devkit8000.dtb matches
>>>> Binary file omap3-devkit8000-lcd43.dtb matches
>>>> Binary file omap3-devkit8000-lcd70.dtb matches
>>>> omap3-lilly-a83x.dtsi:		gpmc,wait-pin = <0>;
>>>> Binary file omap3-lilly-dbb056.dtb matches
>>>> Binary file omap3-zoom3.dtb matches
>>>>
>>>> Only 1 board is using wait-pin 1
>>>> omap-zoom-common.dtsi:		gpmc,wait-pin = <1>;
>>>>
>>>> from OMP36xx TRM, here are the reset values
>>>> WAIT3PINPOLARITY 0x1
>>>> WAIT2PINPOLARITY 0x0
>>>> WAIT1PINPOLARITY 0x1
>>>> WAIT0PINPOLARITY 0x0
>>>
>>> Ah ok. The picture is getting clearer.
>>>
>>> Does it make sense then not to use a boolean property in that case?
>>> With a boolean property we are only able to change the polarity
>>> bits
>>> into one direction (0 -> 1 or 1 -> 0) but we have different reset
>>> values for each bit.
>>>
>>> This part of my patch may then break the mentioned legacy platforms
>>> because it even overwrites the register in case the property is not
>>> set:
>>>
>>>
>>> +	if (p->wait_pin_active_low)
>>> +		config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
>>> +	else
>>> +		config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
>>> +
>>> +	gpmc_write_reg(GPMC_CONFIG, config1);
>>>
>>>
>>> So in order to preserve compatibility as well as the possibility to
>>> change the polarity bits into the desired value I would propose to
>>> use
>>> an uint32 value for the desired value and in case the dt-property
>>> is
>>> not set we should not touch the register at all.
>>
>> I'm sorry I didn't understand how uint32 value solves this issue.
>> Could you please explain with a DT example?
> 
> I meant a similar implementation like in my first patchseries.
> 
> Just a example:
> 
> dts:
> 
> gpmc {
> 
>     foo0@0 {
>         gpmc,wait-pin = <0>;
>         gpmc,wait-pin-polarity = <0>;  /* active low */
>     };
>     
>     bar0@1 {
>         gpmc,wait-pin = <1>;
>         gpmc,wait-pin-polarity = <1>; /* active high */
>     };
>     
>     foobar0@2 {
>         gpmc,wait-pin = <2>;
>         /* don't touch wait pin polarity here */
>     };
> };
> 
> omap-gpmc:
> 
> gpmc_read_settings_dt() 
> {
>   p->wait-pin_polarity = -1;  /* some init value required here */
>   if (!of_property_read_u32(np, "gpmc,wait-pin-polarity", &p->wait-pin_polarity) {
> 
>   ....
>   } 
> }
> 
> gpmc_cs_program_settings() 
> {
>    if (p->wait_pin_polarity == 0)
>      config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
>    if (p->wait_pin_polarity == 1)
>      config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> }
> 
> This should met all requirements.
> 
> If "gpmc,wait-pin-polarity" is not set in the device tree, then the
> registers stay untouched. 
> 
> If it is set, then the WAIT<X>PINPOLARITY bit is set accordingly.
> 
> 
> On the OMP36xx platform for example we have want to set all wait pin
> polarities to active low (0) and have following register reset values:
> 
> WAIT3PINPOLARITY 0x1
> WAIT2PINPOLARITY 0x0
> WAIT1PINPOLARITY 0x1
> WAIT0PINPOLARITY 0x0
> 
> With an boolean "gpmc,wait-pin-active-high" property we're not able to
> set WAIT3PINPOLARITY and WAIT1PINPOLARITY to 0. 
> And vice versa with WAIT2PINPOLARITY and WAIT0PINPOLARITY if we want
> to 
> set them to active high (1) and only would have a "gpmc,wait-pin-
> active-low" property.
> 
> I hope this clarifies my proposal.
> 

Yes now it is clear. I think it is a good proposal and solves all our current issues.

cheers,
-roger

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

[B. Niedermayr]

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

Currently it is not possible to configure the WAIT0PINPOLARITY and 
WAIT1PINPOLARITY bits of the GPMC_CONFIG register directly via 
device tree properties.

It is also not possible to 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
  * Remove the gpmc register configuration out of the gpiochip
    callbacks. In this case the memory interface configuration
    is not done via gpio bindings.
  * Some minor code fixes
  * Changed git commit descriptions

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 wait-pin polarity

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

-- 
2.25.1

Re: [PATCH v3 0/3] omap-gpmc wait pin additions

[Krzysztof Kozlowski]

On 06/09/2022 14:47, B. Niedermayr wrote:
> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> 
> Currently it is not possible to configure the WAIT0PINPOLARITY and 
> WAIT1PINPOLARITY bits of the GPMC_CONFIG register directly via 
> device tree properties.
> 

Just a disclaimer:
This waits in my inbox for a week, it's not ignored. Unfortunately it
will need to wait a bit more, due to traveling / ELCE. I see some
discussion going, though.

Best regards,
Krzysztof