[PATCH v5 0/3] gpmc wait-pin additions

[PATCH v5 0/3] 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
Changes since v2:
  * 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
Change since v3:
  * Use a uint32 dt-property instean a boolean one
  * If the dt-property is not set, then don't touch the
    GPMC_CONFIG register
  * Changed git commit descriptions
Changes since v4:
  * Use checkpatch with "--strict" option
  * Moved wait-pin sanity checks to gpmc_read_settings_dt()
  * Always assign WAITPINPOLARITY_DEFAULT on error cases
  * Track waitpin allocation within gpmc for determine
    allocation origin


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     |  7 ++++
 drivers/memory/omap-gpmc.c                    | 39 ++++++++++++++++++-
 include/linux/platform_data/gpmc-omap.h       |  6 +++
 3 files changed, 50 insertions(+), 2 deletions(-)

--
2.34.1

[PATCH v5 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.

The wait-pin allocation is now tracked by the gpmc driver in order
to be sure that the wait pin has been indeed requested by gpmc.
Therefore the "wait_pin_alloc_mask" has been introduced.

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

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index d9bf1c2ac319..ea495e93766b 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -232,6 +232,7 @@ struct gpmc_device {
 	int irq;
 	struct irq_chip irq_chip;
 	struct gpio_chip gpio_chip;
+	unsigned long wait_pin_alloc_mask;
 	int nirqs;
 	struct resource *data;
 };
@@ -2221,9 +2222,16 @@ 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 &&
+			    test_bit(wait_pin, &gpmc->wait_pin_alloc_mask)) {
+				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;
+			}
+		} else {
+			set_bit(wait_pin, &gpmc->wait_pin_alloc_mask);
 		}
 	}
 
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 16/09/2022 14:07, B. Niedermayr wrote:
> 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.
> 
> The wait-pin allocation is now tracked by the gpmc driver in order
> to be sure that the wait pin has been indeed requested by gpmc.
> Therefore the "wait_pin_alloc_mask" has been introduced.
> 
> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> ---
>  drivers/memory/omap-gpmc.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index d9bf1c2ac319..ea495e93766b 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -232,6 +232,7 @@ struct gpmc_device {
>  	int irq;
>  	struct irq_chip irq_chip;
>  	struct gpio_chip gpio_chip;
> +	unsigned long wait_pin_alloc_mask;
>  	int nirqs;
>  	struct resource *data;
>  };
> @@ -2221,9 +2222,16 @@ 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 &&
> +			    test_bit(wait_pin, &gpmc->wait_pin_alloc_mask)) {
> +				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;
> +			}
> +		} else {
> +			set_bit(wait_pin, &gpmc->wait_pin_alloc_mask);
>  		}

And how do you handle shared pin when the original owner unbinds?


Best regards,
Krzysztof

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

[Niedermayr, BENEDIKT]

Hi Krzysztof,

On Mon, 2022-09-19 at 11:34 +0200, Krzysztof Kozlowski wrote:
> On 16/09/2022 14:07, B. Niedermayr wrote:
> > 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.
> > 
> > The wait-pin allocation is now tracked by the gpmc driver in order
> > to be sure that the wait pin has been indeed requested by gpmc.
> > Therefore the "wait_pin_alloc_mask" has been introduced.
> > 
> > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> > ---
> >  drivers/memory/omap-gpmc.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> > index d9bf1c2ac319..ea495e93766b 100644
> > --- a/drivers/memory/omap-gpmc.c
> > +++ b/drivers/memory/omap-gpmc.c
> > @@ -232,6 +232,7 @@ struct gpmc_device {
> >  	int irq;
> >  	struct irq_chip irq_chip;
> >  	struct gpio_chip gpio_chip;
> > +	unsigned long wait_pin_alloc_mask;
> >  	int nirqs;
> >  	struct resource *data;
> >  };
> > @@ -2221,9 +2222,16 @@ 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 &&
> > +			    test_bit(wait_pin, &gpmc->wait_pin_alloc_mask)) {
> > +				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;
> > +			}
> > +		} else {
> > +			set_bit(wait_pin, &gpmc->wait_pin_alloc_mask);
> >  		}
> 
> And how do you handle shared pin when the original owner unbinds?
So first of all this code only keeps track of the wait_pin allocation from within the gpmc. If any other driver/code
allocated this pin than the evaluation gpiochip_request_own_desc() would fail since the pin hasn't been requested by the gpmc itself.
The testbit() only checks if this pin has been allocated by the gpmc itself. If yes, then the waitpin can be treated as shared wait pin. If no,
then another driver allocated the pin before and we return an error.

The gpmc must be able to release the wait_pin in the wait_pin_alloc_mask, that's true. The only section where the waitpin_desc is released 
can be found further down in this function:

err_cs:
	gpiochip_free_own_desc(waitpin_desc);

You're right. I must add the relase logic here as well.



 
> 
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 19/09/2022 14:37, Niedermayr, BENEDIKT wrote:
>>
>> And how do you handle shared pin when the original owner unbinds?
> So first of all this code only keeps track of the wait_pin allocation from within the gpmc. If any other driver/code
> allocated this pin than the evaluation gpiochip_request_own_desc() would fail since the pin hasn't been requested by the gpmc itself.
> The testbit() only checks if this pin has been allocated by the gpmc itself. If yes, then the waitpin can be treated as shared wait pin. If no,
> then another driver allocated the pin before and we return an error.
> 
> The gpmc must be able to release the wait_pin in the wait_pin_alloc_mask, that's true. The only section where the waitpin_desc is released 
> can be found further down in this function:
> 
> err_cs:
> 	gpiochip_free_own_desc(waitpin_desc);
> 
> You're right. I must add the relase logic here as well.

You wrote quite a lot but that was not explanation of how do you handle
unbind... Last sentence was enough.

Best regards,
Krzysztof

[PATCH v5 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-polarity" dt-property.

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

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index ea495e93766b..2853fc28bccc 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)
@@ -1882,6 +1883,17 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p)
 
 	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1);
 
+	if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
+		config1 = gpmc_read_reg(GPMC_CONFIG);
+
+		if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_LOW)
+			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
+		else if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_HIGH)
+			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
+
+		gpmc_write_reg(GPMC_CONFIG, config1);
+	}
+
 	return 0;
 }
 
@@ -1981,7 +1993,22 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
 				__func__);
 	}
 
+	p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
+
 	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
+		if (!of_property_read_u32(np, "gpmc,wait-pin-polarity",
+					  &p->wait_pin_polarity)) {
+			if (p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH &&
+			    p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW &&
+			    p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
+				pr_err("%s: Invalid wait-pin-polarity (pin: %d, pol: %d)\n",
+				       __func__, p->wait_pin, p->wait_pin_polarity);
+				p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
+			}
+		} else {
+			pr_err("%s: Failed to read gpmc,wait-pin-polarity\n", __func__);
+		}
+
 		p->wait_on_read = of_property_read_bool(np,
 							"gpmc,wait-on-read");
 		p->wait_on_write = of_property_read_bool(np,
diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
index c9cc4e32435d..c46c28069c31 100644
--- a/include/linux/platform_data/gpmc-omap.h
+++ b/include/linux/platform_data/gpmc-omap.h
@@ -136,6 +136,11 @@ struct gpmc_device_timings {
 #define GPMC_MUX_AAD			1	/* Addr-Addr-Data multiplex */
 #define GPMC_MUX_AD			2	/* Addr-Data multiplex */
 
+/* Wait pin polarity values */
+#define WAITPINPOLARITY_DEFAULT -1
+#define WAITPINPOLARITY_ACTIVE_LOW 0
+#define WAITPINPOLARITY_ACTIVE_HIGH 1
+
 struct gpmc_settings {
 	bool burst_wrap;	/* enables wrap bursting */
 	bool burst_read;	/* enables read page/burst mode */
@@ -149,6 +154,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.34.1

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

[Krzysztof Kozlowski]

On 16/09/2022 14:07, B. Niedermayr wrote:
> 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-polarity" dt-property.
> 
> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> ---
>  drivers/memory/omap-gpmc.c              | 27 +++++++++++++++++++++++++
>  include/linux/platform_data/gpmc-omap.h |  6 ++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index ea495e93766b..2853fc28bccc 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)
> @@ -1882,6 +1883,17 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p)
>  
>  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1);
>  
> +	if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
> +		config1 = gpmc_read_reg(GPMC_CONFIG);
> +
> +		if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_LOW)
> +			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> +		else if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_HIGH)
> +			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> +
> +		gpmc_write_reg(GPMC_CONFIG, config1);

What happens if wait pin is shared and you have different polarities in
both of devices?

> +	}
> +
>  	return 0;
>  }
>  
> @@ -1981,7 +1993,22 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
>  				__func__);
>  	}
>  
> +	p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
> +
>  	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
> +		if (!of_property_read_u32(np, "gpmc,wait-pin-polarity",
> +					  &p->wait_pin_polarity)) {
> +			if (p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH &&
> +			    p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW &&
> +			    p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {

WAITPINPOLARITY_DEFAULT is not allowed in DT, so you can skip it.

> +				pr_err("%s: Invalid wait-pin-polarity (pin: %d, pol: %d)\n",

dev_err, not pr_err

> +				       __func__, p->wait_pin, p->wait_pin_polarity);

Skip __func__

> +				p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
> +			}
> +		} else {
> +			pr_err("%s: Failed to read gpmc,wait-pin-polarity\n", __func__);

Ditto.

> +		}
> +
>  		p->wait_on_read = of_property_read_bool(np,
>  							"gpmc,wait-on-read");
>  		p->wait_on_write = of_property_read_bool(np,
> diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
> index c9cc4e32435d..c46c28069c31 100644
> --- a/include/linux/platform_data/gpmc-omap.h
> +++ b/include/linux/platform_data/gpmc-omap.h
> @@ -136,6 +136,11 @@ struct gpmc_device_timings {
>  #define GPMC_MUX_AAD			1	/* Addr-Addr-Data multiplex */
>  #define GPMC_MUX_AD			2	/* Addr-Data multiplex */
>  
> +/* Wait pin polarity values */
> +#define WAITPINPOLARITY_DEFAULT -1

Missing prefix. This is a global header.

> +#define WAITPINPOLARITY_ACTIVE_LOW 0
> +#define WAITPINPOLARITY_ACTIVE_HIGH 1
> +
>  struct gpmc_settings {
>  	bool burst_wrap;	/* enables wrap bursting */
>  	bool burst_read;	/* enables read page/burst mode */
> @@ -149,6 +154,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 */

Skip the comment. You just copied the name of variable. Such comments
are useless.

We do not have KPIs in kernel to achieve some comment-ratio...

Best regards,
Krzysztof

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

[Niedermayr, BENEDIKT]

Hi Krzysztof,

On Mon, 2022-09-19 at 11:38 +0200, Krzysztof Kozlowski wrote:
> On 16/09/2022 14:07, B. Niedermayr wrote:
> > 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-polarity" dt-property.
> > 
> > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> > ---
> >  drivers/memory/omap-gpmc.c              | 27 +++++++++++++++++++++++++
> >  include/linux/platform_data/gpmc-omap.h |  6 ++++++
> >  2 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> > index ea495e93766b..2853fc28bccc 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)
> > @@ -1882,6 +1883,17 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p)
> >  
> >  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1);
> >  
> > +	if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
> > +		config1 = gpmc_read_reg(GPMC_CONFIG);
> > +
> > +		if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_LOW)
> > +			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> > +		else if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_HIGH)
> > +			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> > +
> > +		gpmc_write_reg(GPMC_CONFIG, config1);
> 
> What happens if wait pin is shared and you have different polarities in
> both of devices?
In this case the second one wins and will overwrite the polarity of the first one.
But that would be the result of a misconfiguration in the DT. 

I'm not sure how to proceed here? Does it make sense to add a check for different 
waitpin polarities?


> 
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -1981,7 +1993,22 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
> >  				__func__);
> >  	}
> >  
> > +	p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
> > +
> >  	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
> > +		if (!of_property_read_u32(np, "gpmc,wait-pin-polarity",
> > +					  &p->wait_pin_polarity)) {
> > +			if (p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH &&
> > +			    p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW &&
> > +			    p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
> 
> WAITPINPOLARITY_DEFAULT is not allowed in DT, so you can skip it.
This value is not assigned from the DT. It is only assigned within the GPMC and serves as a init
value (right before the if clause). This helps in case no configuration from DT is done where the 
GPMC registers should stay untouched.

> 
> > +				pr_err("%s: Invalid wait-pin-polarity (pin: %d, pol: %d)\n",
> 
> dev_err, not pr_err

Ok. I didn't want to introduce dev_* functions. Currently pr_* functions where used all over the place.

> 
> > +				       __func__, p->wait_pin, p->wait_pin_polarity);
> 
> Skip __func__
> 
Ok.

> > +				p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
> > +			}
> > +		} else {
> > +			pr_err("%s: Failed to read gpmc,wait-pin-polarity\n", __func__);
> 
> Ditto.
Ok.

> 
> > +		}
> > +
> >  		p->wait_on_read = of_property_read_bool(np,
> >  							"gpmc,wait-on-read");
> >  		p->wait_on_write = of_property_read_bool(np,
> > diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
> > index c9cc4e32435d..c46c28069c31 100644
> > --- a/include/linux/platform_data/gpmc-omap.h
> > +++ b/include/linux/platform_data/gpmc-omap.h
> > @@ -136,6 +136,11 @@ struct gpmc_device_timings {
> >  #define GPMC_MUX_AAD			1	/* Addr-Addr-Data multiplex */
> >  #define GPMC_MUX_AD			2	/* Addr-Data multiplex */
> >  
> > +/* Wait pin polarity values */
> > +#define WAITPINPOLARITY_DEFAULT -1
> 
> Missing prefix. This is a global header.
Ok. Makes sense.
> 
> > +#define WAITPINPOLARITY_ACTIVE_LOW 0
> > +#define WAITPINPOLARITY_ACTIVE_HIGH 1
> > +
> >  struct gpmc_settings {
> >  	bool burst_wrap;	/* enables wrap bursting */
> >  	bool burst_read;	/* enables read page/burst mode */
> > @@ -149,6 +154,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 */
> 
> Skip the comment. You just copied the name of variable. Such comments
> are useless.
> 
> We do not have KPIs in kernel to achieve some comment-ratio...
> 
Ok, makes sense.

> Best regards,
> Krzysztof

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

[Roger Quadros]

On 19/09/2022 16:25, Niedermayr, BENEDIKT wrote:
> Hi Krzysztof,
> 
> On Mon, 2022-09-19 at 11:38 +0200, Krzysztof Kozlowski wrote:
>> On 16/09/2022 14:07, B. Niedermayr wrote:
>>> 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-polarity" dt-property.
>>>
>>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>> ---
>>>  drivers/memory/omap-gpmc.c              | 27 +++++++++++++++++++++++++
>>>  include/linux/platform_data/gpmc-omap.h |  6 ++++++
>>>  2 files changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
>>> index ea495e93766b..2853fc28bccc 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)
>>> @@ -1882,6 +1883,17 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p)
>>>  
>>>  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1);
>>>  
>>> +	if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
>>> +		config1 = gpmc_read_reg(GPMC_CONFIG);
>>> +
>>> +		if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_LOW)
>>> +			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
>>> +		else if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_HIGH)
>>> +			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
>>> +
>>> +		gpmc_write_reg(GPMC_CONFIG, config1);
>>
>> What happens if wait pin is shared and you have different polarities in
>> both of devices?
> In this case the second one wins and will overwrite the polarity of the first one.
> But that would be the result of a misconfiguration in the DT. 
> 
> I'm not sure how to proceed here? Does it make sense to add a check for different 
> waitpin polarities?
> 

Yes I think it is better to check and fail if different polarity is requested than the first time.
Else it will be a potential debugging nightmare. ;)

> 
>>
>>> +	}
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> @@ -1981,7 +1993,22 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
>>>  				__func__);
>>>  	}
>>>  
>>> +	p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
>>> +
>>>  	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
>>> +		if (!of_property_read_u32(np, "gpmc,wait-pin-polarity",
>>> +					  &p->wait_pin_polarity)) {
>>> +			if (p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH &&
>>> +			    p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW &&
>>> +			    p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
>>
>> WAITPINPOLARITY_DEFAULT is not allowed in DT, so you can skip it.
> This value is not assigned from the DT. It is only assigned within the GPMC and serves as a init
> value (right before the if clause). This helps in case no configuration from DT is done where the 
> GPMC registers should stay untouched.
> 
>>
>>> +				pr_err("%s: Invalid wait-pin-polarity (pin: %d, pol: %d)\n",
>>
>> dev_err, not pr_err
> 
> Ok. I didn't want to introduce dev_* functions. Currently pr_* functions where used all over the place.
> 
>>
>>> +				       __func__, p->wait_pin, p->wait_pin_polarity);
>>
>> Skip __func__
>>
> Ok.
> 
>>> +				p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
>>> +			}
>>> +		} else {
>>> +			pr_err("%s: Failed to read gpmc,wait-pin-polarity\n", __func__);
>>
>> Ditto.
> Ok.
> 
>>
>>> +		}
>>> +
>>>  		p->wait_on_read = of_property_read_bool(np,
>>>  							"gpmc,wait-on-read");
>>>  		p->wait_on_write = of_property_read_bool(np,
>>> diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
>>> index c9cc4e32435d..c46c28069c31 100644
>>> --- a/include/linux/platform_data/gpmc-omap.h
>>> +++ b/include/linux/platform_data/gpmc-omap.h
>>> @@ -136,6 +136,11 @@ struct gpmc_device_timings {
>>>  #define GPMC_MUX_AAD			1	/* Addr-Addr-Data multiplex */
>>>  #define GPMC_MUX_AD			2	/* Addr-Data multiplex */
>>>  
>>> +/* Wait pin polarity values */
>>> +#define WAITPINPOLARITY_DEFAULT -1
>>
>> Missing prefix. This is a global header.
> Ok. Makes sense.
>>
>>> +#define WAITPINPOLARITY_ACTIVE_LOW 0
>>> +#define WAITPINPOLARITY_ACTIVE_HIGH 1
>>> +
>>>  struct gpmc_settings {
>>>  	bool burst_wrap;	/* enables wrap bursting */
>>>  	bool burst_read;	/* enables read page/burst mode */
>>> @@ -149,6 +154,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 */
>>
>> Skip the comment. You just copied the name of variable. Such comments
>> are useless.
>>
>> We do not have KPIs in kernel to achieve some comment-ratio...
>>
> Ok, makes sense.
> 
>> Best regards,
>> Krzysztof
> 

cheers,
-roger

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

[Krzysztof Kozlowski]

On 19/09/2022 15:25, Niedermayr, BENEDIKT wrote:
> Hi Krzysztof,
> 
> On Mon, 2022-09-19 at 11:38 +0200, Krzysztof Kozlowski wrote:
>> On 16/09/2022 14:07, B. Niedermayr wrote:
>>> 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-polarity" dt-property.
>>>
>>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>> ---
>>>  drivers/memory/omap-gpmc.c              | 27 +++++++++++++++++++++++++
>>>  include/linux/platform_data/gpmc-omap.h |  6 ++++++
>>>  2 files changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
>>> index ea495e93766b..2853fc28bccc 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)
>>> @@ -1882,6 +1883,17 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p)
>>>  
>>>  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1);
>>>  
>>> +	if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
>>> +		config1 = gpmc_read_reg(GPMC_CONFIG);
>>> +
>>> +		if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_LOW)
>>> +			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
>>> +		else if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_HIGH)
>>> +			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
>>> +
>>> +		gpmc_write_reg(GPMC_CONFIG, config1);
>>
>> What happens if wait pin is shared and you have different polarities in
>> both of devices?
> In this case the second one wins and will overwrite the polarity of the first one.
> But that would be the result of a misconfiguration in the DT.

In many cases drivers do not accept blindly a DT, but perform some basic
sanity on it, especially if mistake is easy to make (e.g. with
overlays). Such design of DT is just fragile. Schema cannot validate it,
driver does not care, mistake is quite possible.

> 
> I'm not sure how to proceed here? Does it make sense to add a check for different 
> waitpin polarities?

I don't know. I would just disallow such sharing entirely or disallow
sharing if DT is misconfigured.


> 
> 
>>
>>> +	}
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> @@ -1981,7 +1993,22 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
>>>  				__func__);
>>>  	}
>>>  
>>> +	p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
>>> +
>>>  	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
>>> +		if (!of_property_read_u32(np, "gpmc,wait-pin-polarity",
>>> +					  &p->wait_pin_polarity)) {
>>> +			if (p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH &&
>>> +			    p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW &&
>>> +			    p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
>>
>> WAITPINPOLARITY_DEFAULT is not allowed in DT, so you can skip it.
> This value is not assigned from the DT. It is only assigned within the GPMC and serves as a init
> value (right before the if clause). This helps in case no configuration from DT is done where the 
> GPMC registers should stay untouched.

I don't see it. Your code is:

p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
# and DT has WAITPINPOLARITY_DEFAULT
if (....) {
  pr_err
  p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
} else {
  pr_err
}

so how this helps in case no configuration from DT is done?

Best regards,
Krzysztof

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

[Niedermayr, BENEDIKT]

Hi Krzysztof,

On Tue, 2022-09-20 at 09:39 +0200, Krzysztof Kozlowski wrote:
> On 19/09/2022 15:25, Niedermayr, BENEDIKT wrote:
> > Hi Krzysztof,
> > 
> > On Mon, 2022-09-19 at 11:38 +0200, Krzysztof Kozlowski wrote:
> > > On 16/09/2022 14:07, B. Niedermayr wrote:
> > > > 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-polarity" dt-property.
> > > > 
> > > > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> > > > ---
> > > >  drivers/memory/omap-gpmc.c              | 27 +++++++++++++++++++++++++
> > > >  include/linux/platform_data/gpmc-omap.h |  6 ++++++
> > > >  2 files changed, 33 insertions(+)
> > > > 
> > > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> > > > index ea495e93766b..2853fc28bccc 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)
> > > > @@ -1882,6 +1883,17 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p)
> > > >  
> > > >  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1);
> > > >  
> > > > +	if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
> > > > +		config1 = gpmc_read_reg(GPMC_CONFIG);
> > > > +
> > > > +		if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_LOW)
> > > > +			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> > > > +		else if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_HIGH)
> > > > +			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> > > > +
> > > > +		gpmc_write_reg(GPMC_CONFIG, config1);
> > > 
> > > What happens if wait pin is shared and you have different polarities in
> > > both of devices?
> > In this case the second one wins and will overwrite the polarity of the first one.
> > But that would be the result of a misconfiguration in the DT.
> 
> In many cases drivers do not accept blindly a DT, but perform some basic
> sanity on it, especially if mistake is easy to make (e.g. with
> overlays). Such design of DT is just fragile. Schema cannot validate it,
> driver does not care, mistake is quite possible.

Ok, that makes sense. I'm going to implement this in v6.
> 
> > I'm not sure how to proceed here? Does it make sense to add a check for different 
> > waitpin polarities?
> 
> I don't know. I would just disallow such sharing entirely or disallow
> sharing if DT is misconfigured.
> 
> 
> > 
> > > > +	}
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > > @@ -1981,7 +1993,22 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
> > > >  				__func__);
> > > >  	}
> > > >  
> > > > +	p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
> > > > +
> > > >  	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
> > > > +		if (!of_property_read_u32(np, "gpmc,wait-pin-polarity",
> > > > +					  &p->wait_pin_polarity)) {
> > > > +			if (p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH &&
> > > > +			    p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW &&
> > > > +			    p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
> > > 
> > > WAITPINPOLARITY_DEFAULT is not allowed in DT, so you can skip it.
> > This value is not assigned from the DT. It is only assigned within the GPMC and serves as a init
> > value (right before the if clause). This helps in case no configuration from DT is done where the 
> > GPMC registers should stay untouched.
> 
> I don't see it. Your code is:
> 
> p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
> # and DT has WAITPINPOLARITY_DEFAULT
> if (....) {
>   pr_err
>   p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
> } else {
>   pr_err
> }
> 
Maybe I dont't get what you mean with DT in this context.

What I meant is that the value WAITPINPOLARITY_DEFAULT is not directly extracted from the DT but is assigned in case
"gpmc,wait-pin-polarity" is not set or has an invalid value. In any case the p->wait_pin_polarity should have
at least the init value assigned so we can make proper decisions in gpmc_cs_program_settings().

Maybe I need some clarification what exatly is forbidden here.


> so how this helps in case no configuration from DT is done?
> 
> Best regards,
> Krzysztof
Cheers,
benedikt

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

[Krzysztof Kozlowski]

On 20/09/2022 11:13, Niedermayr, BENEDIKT wrote:
> Hi Krzysztof,
> 
> On Tue, 2022-09-20 at 09:39 +0200, Krzysztof Kozlowski wrote:
>> On 19/09/2022 15:25, Niedermayr, BENEDIKT wrote:
>>> Hi Krzysztof,
>>>
>>> On Mon, 2022-09-19 at 11:38 +0200, Krzysztof Kozlowski wrote:
>>>> On 16/09/2022 14:07, B. Niedermayr wrote:
>>>>> 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-polarity" dt-property.
>>>>>
>>>>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>>> ---
>>>>>  drivers/memory/omap-gpmc.c              | 27 +++++++++++++++++++++++++
>>>>>  include/linux/platform_data/gpmc-omap.h |  6 ++++++
>>>>>  2 files changed, 33 insertions(+)
>>>>>
>>>>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
>>>>> index ea495e93766b..2853fc28bccc 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)
>>>>> @@ -1882,6 +1883,17 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p)
>>>>>  
>>>>>  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1);
>>>>>  
>>>>> +	if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
>>>>> +		config1 = gpmc_read_reg(GPMC_CONFIG);
>>>>> +
>>>>> +		if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_LOW)
>>>>> +			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
>>>>> +		else if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_HIGH)
>>>>> +			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
>>>>> +
>>>>> +		gpmc_write_reg(GPMC_CONFIG, config1);
>>>>
>>>> What happens if wait pin is shared and you have different polarities in
>>>> both of devices?
>>> In this case the second one wins and will overwrite the polarity of the first one.
>>> But that would be the result of a misconfiguration in the DT.
>>
>> In many cases drivers do not accept blindly a DT, but perform some basic
>> sanity on it, especially if mistake is easy to make (e.g. with
>> overlays). Such design of DT is just fragile. Schema cannot validate it,
>> driver does not care, mistake is quite possible.
> 
> Ok, that makes sense. I'm going to implement this in v6.
>>
>>> I'm not sure how to proceed here? Does it make sense to add a check for different 
>>> waitpin polarities?
>>
>> I don't know. I would just disallow such sharing entirely or disallow
>> sharing if DT is misconfigured.
>>
>>
>>>
>>>>> +	}
>>>>> +
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> @@ -1981,7 +1993,22 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
>>>>>  				__func__);
>>>>>  	}
>>>>>  
>>>>> +	p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
>>>>> +
>>>>>  	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
>>>>> +		if (!of_property_read_u32(np, "gpmc,wait-pin-polarity",
>>>>> +					  &p->wait_pin_polarity)) {
>>>>> +			if (p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH &&
>>>>> +			    p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW &&
>>>>> +			    p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
>>>>
>>>> WAITPINPOLARITY_DEFAULT is not allowed in DT, so you can skip it.
>>> This value is not assigned from the DT. It is only assigned within the GPMC and serves as a init
>>> value (right before the if clause). This helps in case no configuration from DT is done where the 
>>> GPMC registers should stay untouched.
>>
>> I don't see it. Your code is:
>>
>> p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
>> # and DT has WAITPINPOLARITY_DEFAULT
>> if (....) {
>>   pr_err
>>   p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
>> } else {
>>   pr_err
>> }
>>
> Maybe I dont't get what you mean with DT in this context.
> 
> What I meant is that the value WAITPINPOLARITY_DEFAULT is not directly extracted from the DT but is assigned in case
> "gpmc,wait-pin-polarity" is not set or has an invalid value. In any case the p->wait_pin_polarity should have
> at least the init value assigned so we can make proper decisions in gpmc_cs_program_settings().
> 
> Maybe I need some clarification what exatly is forbidden here.

I commented exactly below the line which I question. I don't question
other lines. So let me be a bit more specific:

Why do you need
"p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT"
? Can you write a scenario where this is useful?

Best regards,
Krzysztof

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

[Niedermayr, BENEDIKT]

Hi Krzysztof,

On Tue, 2022-09-20 at 11:47 +0200, Krzysztof Kozlowski wrote:
> On 20/09/2022 11:13, Niedermayr, BENEDIKT wrote:
> > Hi Krzysztof,
> > 
> > On Tue, 2022-09-20 at 09:39 +0200, Krzysztof Kozlowski wrote:
> > > On 19/09/2022 15:25, Niedermayr, BENEDIKT wrote:
> > > > Hi Krzysztof,
> > > > 
> > > > On Mon, 2022-09-19 at 11:38 +0200, Krzysztof Kozlowski wrote:
> > > > > On 16/09/2022 14:07, B. Niedermayr wrote:
> > > > > > 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-polarity" dt-property.
> > > > > > 
> > > > > > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> > > > > > ---
> > > > > >  drivers/memory/omap-gpmc.c              | 27 +++++++++++++++++++++++++
> > > > > >  include/linux/platform_data/gpmc-omap.h |  6 ++++++
> > > > > >  2 files changed, 33 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> > > > > > index ea495e93766b..2853fc28bccc 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)
> > > > > > @@ -1882,6 +1883,17 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p)
> > > > > >  
> > > > > >  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1);
> > > > > >  
> > > > > > +	if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
> > > > > > +		config1 = gpmc_read_reg(GPMC_CONFIG);
> > > > > > +
> > > > > > +		if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_LOW)
> > > > > > +			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> > > > > > +		else if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_HIGH)
> > > > > > +			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> > > > > > +
> > > > > > +		gpmc_write_reg(GPMC_CONFIG, config1);
> > > > > 
> > > > > What happens if wait pin is shared and you have different polarities in
> > > > > both of devices?
> > > > In this case the second one wins and will overwrite the polarity of the first one.
> > > > But that would be the result of a misconfiguration in the DT.
> > > 
> > > In many cases drivers do not accept blindly a DT, but perform some basic
> > > sanity on it, especially if mistake is easy to make (e.g. with
> > > overlays). Such design of DT is just fragile. Schema cannot validate it,
> > > driver does not care, mistake is quite possible.
> > 
> > Ok, that makes sense. I'm going to implement this in v6.
> > > > I'm not sure how to proceed here? Does it make sense to add a check for different 
> > > > waitpin polarities?
> > > 
> > > I don't know. I would just disallow such sharing entirely or disallow
> > > sharing if DT is misconfigured.
> > > 
> > > 
> > > > > > +	}
> > > > > > +
> > > > > >  	return 0;
> > > > > >  }
> > > > > >  
> > > > > > @@ -1981,7 +1993,22 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
> > > > > >  				__func__);
> > > > > >  	}
> > > > > >  
> > > > > > +	p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
> > > > > > +
> > > > > >  	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
> > > > > > +		if (!of_property_read_u32(np, "gpmc,wait-pin-polarity",
> > > > > > +					  &p->wait_pin_polarity)) {
> > > > > > +			if (p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH &&
> > > > > > +			    p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW &&
> > > > > > +			    p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
> > > > > 
> > > > > WAITPINPOLARITY_DEFAULT is not allowed in DT, so you can skip it.
> > > > This value is not assigned from the DT. It is only assigned within the GPMC and serves as a init
> > > > value (right before the if clause). This helps in case no configuration from DT is done where the 
> > > > GPMC registers should stay untouched.
> > > 
> > > I don't see it. Your code is:
> > > 
> > > p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
> > > # and DT has WAITPINPOLARITY_DEFAULT
> > > if (....) {
> > >   pr_err
> > >   p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
> > > } else {
> > >   pr_err
> > > }
> > > 
> > Maybe I dont't get what you mean with DT in this context.
> > 
> > What I meant is that the value WAITPINPOLARITY_DEFAULT is not directly extracted from the DT but is assigned in case
> > "gpmc,wait-pin-polarity" is not set or has an invalid value. In any case the p->wait_pin_polarity should have
> > at least the init value assigned so we can make proper decisions in gpmc_cs_program_settings().
> > 
> > Maybe I need some clarification what exatly is forbidden here.
> 
> I commented exactly below the line which I question. I don't question
> other lines. So let me be a bit more specific:
> 
> Why do you need
> "p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT"
> ? Can you write a scenario where this is useful?
> 
Ok. I think I got you now. Sorry I'm relatively new to OSS contributions, so please be patient with me...

If I remove that part of the if clause, then an error message would be printed in case "p->wait_pin_polarity == WAITPINPOLARITY_DEFAULT".
But this is a not an error case. WAITPINPOLARITY_DEFAULT is a valid value, is assigned right before the if clause as an init value(not extracted from DT),
and leads to not touching the GPMC_CONFIG register in gpmc_cs_program_settings().
So in gpmc_cs_program_settings() if:
    p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH -> Issue a write to the GPMC_CONFIG register
    p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW  -> Issua a write to the GPMC_CONFIG register
    p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT     -> Do not touch the GPMC_CONFIG register

We want to preserve the reset value of the GPMC_CONFIG register in case the DT does not use the "gpmc,wait-pin-polarity" property. Otherwise
we might break platforms which rely on these reset values. 

> Best regards,
> Krzysztof
Cheers,
benedikt

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

[Krzysztof Kozlowski]

On 20/09/2022 12:12, Niedermayr, BENEDIKT wrote:
>> I commented exactly below the line which I question. I don't question
>> other lines. So let me be a bit more specific:
>>
>> Why do you need
>> "p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT"
>> ? Can you write a scenario where this is useful?
>>
> Ok. I think I got you now. Sorry I'm relatively new to OSS contributions, so please be patient with me...
> 
> If I remove that part of the if clause, then an error message would be printed in case "p->wait_pin_polarity == WAITPINPOLARITY_DEFAULT".

Exactly this will happen. As expected. This value cannot appear in DTS,
therefore I would expect error message.

Now you allow such value in DTS which is not the same as your bindings.


> But this is a not an error case. WAITPINPOLARITY_DEFAULT is a valid value, is assigned right before the if clause as an init value(not extracted from DT),
> and leads to not touching the GPMC_CONFIG register in gpmc_cs_program_settings().
> So in gpmc_cs_program_settings() if:
>     p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH -> Issue a write to the GPMC_CONFIG register
>     p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW  -> Issua a write to the GPMC_CONFIG register
>     p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT     -> Do not touch the GPMC_CONFIG register
> 
> We want to preserve the reset value of the GPMC_CONFIG register in case the DT does not use the "gpmc,wait-pin-polarity" property. Otherwise
> we might break platforms which rely on these reset values. 

Best regards,
Krzysztof

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

[Niedermayr, BENEDIKT]

Hi Krzysztof,

On Tue, 2022-09-20 at 13:23 +0200, Krzysztof Kozlowski wrote:
> On 20/09/2022 12:12, Niedermayr, BENEDIKT wrote:
> > > I commented exactly below the line which I question. I don't question
> > > other lines. So let me be a bit more specific:
> > > 
> > > Why do you need
> > > "p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT"
> > > ? Can you write a scenario where this is useful?
> > > 
> > Ok. I think I got you now. Sorry I'm relatively new to OSS contributions, so please be patient with me...
> > 
> > If I remove that part of the if clause, then an error message would be printed in case "p->wait_pin_polarity == WAITPINPOLARITY_DEFAULT".
> 
> Exactly this will happen. As expected. This value cannot appear in DTS,
> therefore I would expect error message.
> 
> Now you allow such value in DTS which is not the same as your bindings.
> 
And now I completely got it...
With this implementation it's even possible to set WAITPINPOLARITY_DEFAULT in the DT...

Ok, changing this will lead to an error message if the "gpmc,wait-pin-polarity" is not set in DT. Means the DT property is more orless not an optional
property anymore.
If one defines the wait-pin without defining the polarity the driver probes successfully but and error message is printed.
Is this an acceptable solution for you?


> 
> > But this is a not an error case. WAITPINPOLARITY_DEFAULT is a valid value, is assigned right before the if clause as an init value(not extracted from
> > DT),
> > and leads to not touching the GPMC_CONFIG register in gpmc_cs_program_settings().
> > So in gpmc_cs_program_settings() if:
> >     p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH -> Issue a write to the GPMC_CONFIG register
> >     p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW  -> Issua a write to the GPMC_CONFIG register
> >     p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT     -> Do not touch the GPMC_CONFIG register
> > 
> > We want to preserve the reset value of the GPMC_CONFIG register in case the DT does not use the "gpmc,wait-pin-polarity" property. Otherwise
> > we might break platforms which rely on these reset values. 
> 
> Best regards,
> Krzysztof
Cheers,
benedikt

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

[Roger Quadros]

Hi,

On 20/09/2022 15:17, Niedermayr, BENEDIKT wrote:
> Hi Krzysztof,
> 
> On Tue, 2022-09-20 at 13:23 +0200, Krzysztof Kozlowski wrote:
>> On 20/09/2022 12:12, Niedermayr, BENEDIKT wrote:
>>>> I commented exactly below the line which I question. I don't question
>>>> other lines. So let me be a bit more specific:
>>>>
>>>> Why do you need
>>>> "p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT"
>>>> ? Can you write a scenario where this is useful?
>>>>
>>> Ok. I think I got you now. Sorry I'm relatively new to OSS contributions, so please be patient with me...
>>>
>>> If I remove that part of the if clause, then an error message would be printed in case "p->wait_pin_polarity == WAITPINPOLARITY_DEFAULT".
>>
>> Exactly this will happen. As expected. This value cannot appear in DTS,
>> therefore I would expect error message.
>>
>> Now you allow such value in DTS which is not the same as your bindings.
>>
> And now I completely got it...
> With this implementation it's even possible to set WAITPINPOLARITY_DEFAULT in the DT...
> 
> Ok, changing this will lead to an error message if the "gpmc,wait-pin-polarity" is not set in DT. Means the DT property is more orless not an optional
> property anymore.
> If one defines the wait-pin without defining the polarity the driver probes successfully but and error message is printed.
> Is this an acceptable solution for you?
> 

No this is not acceptable. As current implementations don't define polarity and rely on reset defaults.

You can check return value of "of_property_read_u32(np, "gpmc,wait-pin-polarity", &p->wait_pin_polarity))"

" * Return: 0 on success, -EINVAL if the property does not exist,
 * -ENODATA if property does not have a value, and -EOVERFLOW if the
 * property data isn't large enough."

If property is present you don't need to check for WAITPINPOLARITY_DEFAULT as that is not valid value for this property.
If property is not present you force WAITPINPOLARITY_DEFAULT and don't print error message.


> 
>>
>>> But this is a not an error case. WAITPINPOLARITY_DEFAULT is a valid value, is assigned right before the if clause as an init value(not extracted from
>>> DT),
>>> and leads to not touching the GPMC_CONFIG register in gpmc_cs_program_settings().
>>> So in gpmc_cs_program_settings() if:
>>>     p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH -> Issue a write to the GPMC_CONFIG register
>>>     p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW  -> Issua a write to the GPMC_CONFIG register
>>>     p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT     -> Do not touch the GPMC_CONFIG register
>>>
>>> We want to preserve the reset value of the GPMC_CONFIG register in case the DT does not use the "gpmc,wait-pin-polarity" property. Otherwise
>>> we might break platforms which rely on these reset values. 
>>
>> Best regards,
>> Krzysztof
> Cheers,
> benedikt
> 

cheers,
-roger

[PATCH v5 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 by setting the gpmc,wait-pin-polarity
dt-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..8e541acdb1ff 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: |
+      Set the desired polarity for the selected wait pin.
+      0 for active low, 1 for active high.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1]
+
   gpmc,wait-on-read:
     description: Enables wait monitoring on reads.
     type: boolean
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 16/09/2022 14:07, 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 by setting the gpmc,wait-pin-polarity
> dt-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..8e541acdb1ff 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: |
> +      Set the desired polarity for the selected wait pin.
> +      0 for active low, 1 for active high.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]

I propose to keep the same value as GPIO flags. Not that it matters, but
maybe one day you will unify it.

Best regards,
Krzysztof

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

[Niedermayr, BENEDIKT]

Hi Krzysztof,

On Tue, 2022-09-20 at 13:21 +0200, Krzysztof Kozlowski wrote:
> On 16/09/2022 14:07, 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 by setting the gpmc,wait-pin-polarity
> > dt-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..8e541acdb1ff 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: |
> > +      Set the desired polarity for the selected wait pin.
> > +      0 for active low, 1 for active high.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1]
> 
> I propose to keep the same value as GPIO flags. Not that it matters, but
> maybe one day you will unify it.
Ah yes. Makes sense!

> Best regards,
> Krzysztof
Cheers,
benedikt