[PATCH v6 0/2] gpmc wait pin additions

[PATCH v6 0/2] 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
Changes since v5:
  * Tracking of wait-pin allocations with polarity change detection
    * Introduced a new struct gpmc_waitpin
  * Add GPMC_* to global header definitions
  * Don't allow GPMC_WAITPINPOLARITY_DEFAULT when parsing dt-properties
  * Squashed wait-pin-polarity and shared-wait-pin patches, since they
    should not be separated

Benedikt Niedermayr (2):
  memory: omap-gpmc: wait pin additions
  dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity

 .../memory-controllers/ti,gpmc-child.yaml     |   7 ++
 drivers/memory/omap-gpmc.c                    | 115 ++++++++++++++++--
 include/linux/platform_data/gpmc-omap.h       |   8 ++
 3 files changed, 118 insertions(+), 12 deletions(-)

--
2.25.1

[PATCH v6 1/2] memory: omap-gpmc: wait pin additions

[B. Niedermayr]

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

This patch introduces support for setting the wait-pin polarity as well
as using the same wait-pin for different CS regions.

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.

The wait-pin can also be shared between different CS regions for special
usecases. Therefore gpmc must keep track of wait-pin allocations, so it
knows that either gpmc itself or another driver has the ownership.

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

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index d9bf1c2ac319..24510d19a564 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)
@@ -227,11 +228,18 @@ struct omap3_gpmc_regs {
 	struct gpmc_cs_config cs_context[GPMC_CS_NUM];
 };
 
+struct gpmc_waitpin {
+	u32 pin;
+	u32 polarity;
+	struct gpio_desc *desc;
+};
+
 struct gpmc_device {
 	struct device *dev;
 	int irq;
 	struct irq_chip irq_chip;
 	struct gpio_chip gpio_chip;
+	struct gpmc_waitpin *waitpins;
 	int nirqs;
 	struct resource *data;
 };
@@ -1030,6 +1038,59 @@ void gpmc_cs_free(int cs)
 }
 EXPORT_SYMBOL(gpmc_cs_free);
 
+static bool gpmc_is_valid_waitpin(u32 waitpin)
+{
+	return waitpin >= 0 && waitpin < gpmc_nr_waitpins;
+}
+
+static int gpmc_alloc_waitpin(struct gpmc_device *gpmc,
+			      struct gpmc_settings *p)
+{
+	int ret;
+	struct gpmc_waitpin *waitpin;
+	struct gpio_desc *waitpin_desc;
+
+	if (!gpmc_is_valid_waitpin(p->wait_pin))
+		return -EINVAL;
+
+	waitpin = &gpmc->waitpins[p->wait_pin];
+
+	waitpin_desc = gpiochip_request_own_desc(&gpmc->gpio_chip,
+						 p->wait_pin, "WAITPIN",
+						 GPIO_ACTIVE_HIGH,
+						 GPIOD_IN);
+
+	ret = PTR_ERR(waitpin_desc);
+	if (IS_ERR(waitpin_desc) && ret != -EBUSY)
+		return ret;
+
+	/* Shared wait pin */
+	if (waitpin && waitpin->desc) {
+		/* Must use the same properties than previous allocation */
+		if (p->wait_pin_polarity != waitpin->polarity ||
+		    p->wait_pin != waitpin->pin) {
+			dev_err(gpmc->dev,
+				"shared-wait-pin: invalid configuration\n");
+			return -EINVAL;
+		}
+		return 0;
+	}
+
+	waitpin->desc = waitpin_desc;
+	waitpin->pin = p->wait_pin;
+	waitpin->polarity = p->wait_pin_polarity;
+	dev_info(gpmc->dev, "shared wait-pin: %d\n", waitpin->pin);
+
+	return 0;
+}
+
+static void gpmc_free_waitpin(struct gpmc_device *gpmc,
+			      int wait_pin)
+{
+	if (gpmc_is_valid_waitpin(wait_pin))
+		gpiochip_free_own_desc(gpmc->waitpins[wait_pin].desc);
+}
+
 /**
  * gpmc_configure - write request to configure gpmc
  * @cmd: command type
@@ -1881,6 +1942,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 != GPMC_WAITPINPOLARITY_DEFAULT) {
+		config1 = gpmc_read_reg(GPMC_CONFIG);
+
+		if (p->wait_pin_polarity == GPMC_WAITPINPOLARITY_ACTIVE_LOW)
+			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
+		else if (p->wait_pin_polarity == GPMC_WAITPINPOLARITY_ACTIVE_HIGH)
+			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
+
+		gpmc_write_reg(GPMC_CONFIG, config1);
+	}
+
 	return 0;
 }
 
@@ -1980,7 +2052,25 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
 				__func__);
 	}
 
+	p->wait_pin = GPMC_WAITPIN_DEFAULT;
+	p->wait_pin_polarity = GPMC_WAITPINPOLARITY_DEFAULT;
+
 	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
+		if (!gpmc_is_valid_waitpin(p->wait_pin)) {
+			pr_err("%s: Invalid wait-pin (%d)\n", __func__, p->wait_pin);
+			p->wait_pin = GPMC_WAITPIN_DEFAULT;
+		}
+
+		if (!of_property_read_u32(np, "gpmc,wait-pin-polarity",
+					  &p->wait_pin_polarity)) {
+			if (p->wait_pin_polarity != GPMC_WAITPINPOLARITY_ACTIVE_HIGH &&
+			    p->wait_pin_polarity != GPMC_WAITPINPOLARITY_ACTIVE_LOW) {
+				pr_err("%s: Invalid wait-pin-polarity (%d)\n",
+				       __func__, p->wait_pin_polarity);
+				p->wait_pin_polarity = GPMC_WAITPINPOLARITY_DEFAULT;
+				}
+		}
+
 		p->wait_on_read = of_property_read_bool(np,
 							"gpmc,wait-on-read");
 		p->wait_on_write = of_property_read_bool(np,
@@ -2085,7 +2175,6 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 	const char *name;
 	int ret, cs;
 	u32 val;
-	struct gpio_desc *waitpin_desc = NULL;
 	struct gpmc_device *gpmc = platform_get_drvdata(pdev);
 
 	if (of_property_read_u32(child, "reg", &cs) < 0) {
@@ -2214,17 +2303,9 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 
 	/* Reserve wait pin if it is required and valid */
 	if (gpmc_s.wait_on_read || gpmc_s.wait_on_write) {
-		unsigned int wait_pin = gpmc_s.wait_pin;
-
-		waitpin_desc = gpiochip_request_own_desc(&gpmc->gpio_chip,
-							 wait_pin, "WAITPIN",
-							 GPIO_ACTIVE_HIGH,
-							 GPIOD_IN);
-		if (IS_ERR(waitpin_desc)) {
-			dev_err(&pdev->dev, "invalid wait-pin: %d\n", wait_pin);
-			ret = PTR_ERR(waitpin_desc);
+		ret = gpmc_alloc_waitpin(gpmc, &gpmc_s);
+		if (ret < 0)
 			goto err;
-		}
 	}
 
 	gpmc_cs_show_timings(cs, "before gpmc_cs_program_settings");
@@ -2268,7 +2349,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 	ret = -ENODEV;
 
 err_cs:
-	gpiochip_free_own_desc(waitpin_desc);
+	gpmc_free_waitpin(gpmc, gpmc_s.wait_pin);
 err:
 	gpmc_cs_free(cs);
 
@@ -2278,6 +2359,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 static int gpmc_probe_dt(struct platform_device *pdev)
 {
 	int ret;
+	struct gpmc_device *gpmc =  platform_get_drvdata(pdev);
 	const struct of_device_id *of_id =
 		of_match_device(gpmc_dt_ids, &pdev->dev);
 
@@ -2305,6 +2387,12 @@ static int gpmc_probe_dt(struct platform_device *pdev)
 		return ret;
 	}
 
+	gpmc->waitpins = devm_kzalloc(&pdev->dev,
+				      gpmc_nr_waitpins * sizeof(struct gpmc_waitpin),
+				      GFP_KERNEL);
+	if (!gpmc->waitpins)
+		return -ENOMEM;
+
 	return 0;
 }
 
@@ -2505,8 +2593,11 @@ static int gpmc_probe(struct platform_device *pdev)
 
 static int gpmc_remove(struct platform_device *pdev)
 {
+	int i;
 	struct gpmc_device *gpmc = platform_get_drvdata(pdev);
 
+	for (i = 0; i < gpmc_nr_waitpins; i++)
+		gpmc_free_waitpin(gpmc, i);
 	gpmc_free_irq(gpmc);
 	gpmc_mem_exit();
 	pm_runtime_put_sync(&pdev->dev);
diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
index c9cc4e32435d..40481dbf793d 100644
--- a/include/linux/platform_data/gpmc-omap.h
+++ b/include/linux/platform_data/gpmc-omap.h
@@ -136,6 +136,13 @@ 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 GPMC_WAITPINPOLARITY_DEFAULT -1
+#define GPMC_WAITPINPOLARITY_ACTIVE_LOW 1
+#define GPMC_WAITPINPOLARITY_ACTIVE_HIGH 0
+
+#define GPMC_WAITPIN_DEFAULT -1
+
 struct gpmc_settings {
 	bool burst_wrap;	/* enables wrap bursting */
 	bool burst_read;	/* enables read page/burst mode */
@@ -149,6 +156,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;
 };
 
 /* Data for each chip select */
-- 
2.25.1

[PATCH v6 2/2] 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..477189973334 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.
+      1 for active low, 0 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.25.1

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

[Rob Herring]

On Thu, Sep 29, 2022 at 02:56:39PM +0200, 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..477189973334 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' is not a vendor. Don't continue this bad pattern, use 'ti'.

> +    description: |
> +      Set the desired polarity for the selected wait pin.
> +      1 for active low, 0 for active high.

Well that looks backwards. I assume from the commit msg above, it's the 
register value, but that's not what the description says. Please go with 
the logical state here and do the inversion in the driver.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1]
> +
>    gpmc,wait-on-read:
>      description: Enables wait monitoring on reads.
>      type: boolean
> -- 
> 2.25.1
> 
> 

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

[Krzysztof Kozlowski]

On 30/09/2022 21:42, Rob Herring wrote:
> On Thu, Sep 29, 2022 at 02:56:39PM +0200, 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..477189973334 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' is not a vendor. Don't continue this bad pattern, use 'ti'.
> 
>> +    description: |
>> +      Set the desired polarity for the selected wait pin.
>> +      1 for active low, 0 for active high.
> 
> Well that looks backwards. I assume from the commit msg above, it's the 
> register value, but that's not what the description says. Please go with 
> the logical state here and do the inversion in the driver.

This was actually my suggestion to keep the same value as
ACTIVE_HIGH/LOW in standard GPIO flags. The DTS could reuse the defines.

Best regards,
Krzysztof

Re: [PATCH v6 1/2] memory: omap-gpmc: wait pin additions

[Roger Quadros]

Hi Benedikt,

On 29/09/2022 15:56, B. Niedermayr wrote:
> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> 
> This patch introduces support for setting the wait-pin polarity as well
> as using the same wait-pin for different CS regions.
> 
> 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.
> 
> The wait-pin can also be shared between different CS regions for special
> usecases. Therefore gpmc must keep track of wait-pin allocations, so it

s/gpmc/GPMC

> knows that either gpmc itself or another driver has the ownership.

s/gpmc/GPMC

> 
> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
> ---
>  drivers/memory/omap-gpmc.c              | 115 +++++++++++++++++++++---
>  include/linux/platform_data/gpmc-omap.h |   8 ++
>  2 files changed, 111 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index d9bf1c2ac319..24510d19a564 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)
> @@ -227,11 +228,18 @@ struct omap3_gpmc_regs {
>  	struct gpmc_cs_config cs_context[GPMC_CS_NUM];
>  };
>  
> +struct gpmc_waitpin {
> +	u32 pin;
> +	u32 polarity;
> +	struct gpio_desc *desc;
> +};
> +
>  struct gpmc_device {
>  	struct device *dev;
>  	int irq;
>  	struct irq_chip irq_chip;
>  	struct gpio_chip gpio_chip;
> +	struct gpmc_waitpin *waitpins;
>  	int nirqs;
>  	struct resource *data;
>  };
> @@ -1030,6 +1038,59 @@ void gpmc_cs_free(int cs)
>  }
>  EXPORT_SYMBOL(gpmc_cs_free);
>  
> +static bool gpmc_is_valid_waitpin(u32 waitpin)
> +{
> +	return waitpin >= 0 && waitpin < gpmc_nr_waitpins> +}
> +
> +static int gpmc_alloc_waitpin(struct gpmc_device *gpmc,
> +			      struct gpmc_settings *p)
> +{
> +	int ret;
> +	struct gpmc_waitpin *waitpin;
> +	struct gpio_desc *waitpin_desc;
> +
> +	if (!gpmc_is_valid_waitpin(p->wait_pin))
> +		return -EINVAL;
> +
> +	waitpin = &gpmc->waitpins[p->wait_pin];
> +

Please add the following comment here to help future readers

/* Reserve the GPIO for wait pin usage.
 * GPIO polarity doesn't matter here. Wait pin polarity
 * is set in GPMC_CONFIG register.
 */


> +	waitpin_desc = gpiochip_request_own_desc(&gpmc->gpio_chip,
> +						 p->wait_pin, "WAITPIN",
> +						 GPIO_ACTIVE_HIGH,
> +						 GPIOD_IN);
> +
> +	ret = PTR_ERR(waitpin_desc);
> +	if (IS_ERR(waitpin_desc) && ret != -EBUSY)
> +		return ret;
> +
> +	/* Shared wait pin */
> +	if (waitpin && waitpin->desc) {
> +		/* Must use the same properties than previous allocation */
> +		if (p->wait_pin_polarity != waitpin->polarity ||
> +		    p->wait_pin != waitpin->pin) {
> +			dev_err(gpmc->dev,
> +				"shared-wait-pin: invalid configuration\n");
> +			return -EINVAL;
> +		}
> +		return 0;
> +	}

How about this instead?
	if (!waitpin->desc) {
		/* new wait pin */
		waitpin_desc = gpiochip_request_own_desc(...
		ret = PTR_ERR(waitpin_desc);
		if (ret)
			return ret;
	} else {
		/* shared wait pin */
	}

> +
> +	waitpin->desc = waitpin_desc;
> +	waitpin->pin = p->wait_pin;
> +	waitpin->polarity = p->wait_pin_polarity;
> +	dev_info(gpmc->dev, "shared wait-pin: %d\n", waitpin->pin);

This will print for all cases. Please move it inside the else {}
that I suggested above.

> +
> +	return 0;
> +}
> +
> +static void gpmc_free_waitpin(struct gpmc_device *gpmc,
> +			      int wait_pin)
> +{
> +	if (gpmc_is_valid_waitpin(wait_pin))
> +		gpiochip_free_own_desc(gpmc->waitpins[wait_pin].desc);
> +}
> +
>  /**
>   * gpmc_configure - write request to configure gpmc
>   * @cmd: command type
> @@ -1881,6 +1942,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 != GPMC_WAITPINPOLARITY_DEFAULT) {
> +		config1 = gpmc_read_reg(GPMC_CONFIG);
> +
> +		if (p->wait_pin_polarity == GPMC_WAITPINPOLARITY_ACTIVE_LOW)
> +			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> +		else if (p->wait_pin_polarity == GPMC_WAITPINPOLARITY_ACTIVE_HIGH)
> +			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
> +
> +		gpmc_write_reg(GPMC_CONFIG, config1);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1980,7 +2052,25 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
>  				__func__);
>  	}
>  
> +	p->wait_pin = GPMC_WAITPIN_DEFAULT;
> +	p->wait_pin_polarity = GPMC_WAITPINPOLARITY_DEFAULT;
> +
>  	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
> +		if (!gpmc_is_valid_waitpin(p->wait_pin)) {
> +			pr_err("%s: Invalid wait-pin (%d)\n", __func__, p->wait_pin);
> +			p->wait_pin = GPMC_WAITPIN_DEFAULT;
> +		}
> +
> +		if (!of_property_read_u32(np, "gpmc,wait-pin-polarity",
> +					  &p->wait_pin_polarity)) {
> +			if (p->wait_pin_polarity != GPMC_WAITPINPOLARITY_ACTIVE_HIGH &&
> +			    p->wait_pin_polarity != GPMC_WAITPINPOLARITY_ACTIVE_LOW) {
> +				pr_err("%s: Invalid wait-pin-polarity (%d)\n",
> +				       __func__, p->wait_pin_polarity);
> +				p->wait_pin_polarity = GPMC_WAITPINPOLARITY_DEFAULT;
> +				}
> +		}
> +
>  		p->wait_on_read = of_property_read_bool(np,
>  							"gpmc,wait-on-read");
>  		p->wait_on_write = of_property_read_bool(np,
> @@ -2085,7 +2175,6 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  	const char *name;
>  	int ret, cs;
>  	u32 val;
> -	struct gpio_desc *waitpin_desc = NULL;
>  	struct gpmc_device *gpmc = platform_get_drvdata(pdev);
>  
>  	if (of_property_read_u32(child, "reg", &cs) < 0) {
> @@ -2214,17 +2303,9 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  
>  	/* Reserve wait pin if it is required and valid */
>  	if (gpmc_s.wait_on_read || gpmc_s.wait_on_write) {
> -		unsigned int wait_pin = gpmc_s.wait_pin;
> -
> -		waitpin_desc = gpiochip_request_own_desc(&gpmc->gpio_chip,
> -							 wait_pin, "WAITPIN",
> -							 GPIO_ACTIVE_HIGH,
> -							 GPIOD_IN);
> -		if (IS_ERR(waitpin_desc)) {
> -			dev_err(&pdev->dev, "invalid wait-pin: %d\n", wait_pin);
> -			ret = PTR_ERR(waitpin_desc);
> +		ret = gpmc_alloc_waitpin(gpmc, &gpmc_s);
> +		if (ret < 0)
>  			goto err;
> -		}
>  	}
>  
>  	gpmc_cs_show_timings(cs, "before gpmc_cs_program_settings");
> @@ -2268,7 +2349,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  	ret = -ENODEV;
>  
>  err_cs:
> -	gpiochip_free_own_desc(waitpin_desc);
> +	gpmc_free_waitpin(gpmc, gpmc_s.wait_pin);
>  err:
>  	gpmc_cs_free(cs);
>  
> @@ -2278,6 +2359,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  static int gpmc_probe_dt(struct platform_device *pdev)
>  {
>  	int ret;
> +	struct gpmc_device *gpmc =  platform_get_drvdata(pdev);
>  	const struct of_device_id *of_id =
>  		of_match_device(gpmc_dt_ids, &pdev->dev);
>  
> @@ -2305,6 +2387,12 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	gpmc->waitpins = devm_kzalloc(&pdev->dev,
> +				      gpmc_nr_waitpins * sizeof(struct gpmc_waitpin),
> +				      GFP_KERNEL);
> +	if (!gpmc->waitpins)
> +		return -ENOMEM;
> +

This allocation should happen in gpmc_probe() just after gpmc_nr_waitpins is initialized.
I see there are 2 places where gpmc_nr_waitpins is set there. gpmc->waitpins allocation
should happen after.

Also, all waitpins[i].pin are set to 0 which is a valid wait pin. This will
break your gpmc_free_waitpin() logic as you will try to free the gpio descriptor
for all waitpins.

One solution could be to set all waitpins[i].pin to -1 here.

>  	return 0;
>  }
>  
> @@ -2505,8 +2593,11 @@ static int gpmc_probe(struct platform_device *pdev)
>  
>  static int gpmc_remove(struct platform_device *pdev)
>  {
> +	int i;
>  	struct gpmc_device *gpmc = platform_get_drvdata(pdev);
>  
> +	for (i = 0; i < gpmc_nr_waitpins; i++)
> +		gpmc_free_waitpin(gpmc, i);
>  	gpmc_free_irq(gpmc);
>  	gpmc_mem_exit();
>  	pm_runtime_put_sync(&pdev->dev);
> diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
> index c9cc4e32435d..40481dbf793d 100644
> --- a/include/linux/platform_data/gpmc-omap.h
> +++ b/include/linux/platform_data/gpmc-omap.h
> @@ -136,6 +136,13 @@ 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 GPMC_WAITPINPOLARITY_DEFAULT -1

Can we please say INVALID instead of DEFAULT?
DEFAULT usually means valid but we want to indicate invalid here.

> +#define GPMC_WAITPINPOLARITY_ACTIVE_LOW 1
> +#define GPMC_WAITPINPOLARITY_ACTIVE_HIGH 0
> +
> +#define GPMC_WAITPIN_DEFAULT -1
> +
>  struct gpmc_settings {
>  	bool burst_wrap;	/* enables wrap bursting */
>  	bool burst_read;	/* enables read page/burst mode */
> @@ -149,6 +156,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;
>  };
>  
>  /* Data for each chip select */

cheers,
-roger

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

[Niedermayr, BENEDIKT]

On Fri, 2022-09-30 at 22:01 +0200, Krzysztof Kozlowski wrote:
> On 30/09/2022 21:42, Rob Herring wrote:
> > On Thu, Sep 29, 2022 at 02:56:39PM +0200, 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..477189973334 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' is not a vendor. Don't continue this bad pattern, use 'ti'.
> > 
> > > +    description: |
> > > +      Set the desired polarity for the selected wait pin.
> > > +      1 for active low, 0 for active high.
> > 
> > Well that looks backwards. I assume from the commit msg above, it's the 
> > register value, but that's not what the description says. Please go with 
> > the logical state here and do the inversion in the driver.
> 
> This was actually my suggestion to keep the same value as
> ACTIVE_HIGH/LOW in standard GPIO flags. The DTS could reuse the defines.
> 
Ok, but how to proceed know? IMHO it makes more sense to use the value which actually lands in the register since most 
people will use the value found in the Datasheet. 

We already had a discussion with Roger about the GPIO bindings vs. wait-pin binding. The point was that we do not use GPIO bindings
in this case, or? 

> Best regards,
> Krzysztof
> 

Cheers,
Benedikt

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

[Niedermayr, BENEDIKT]

On Fri, 2022-09-30 at 14:42 -0500, Rob Herring wrote:
> On Thu, Sep 29, 2022 at 02:56:39PM +0200, 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..477189973334 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' is not a vendor. Don't continue this bad pattern, use 'ti'.

You are right. But nevertheless I can't agree with that in this patch series.
I don't want to break consistency, since all bindings currently use 'gpmc'. At least this applies
to the "ti,gpmc-child.yaml".

I think it makes more sense to create a complete new patch series for that specific change? This change
wouldn't fit thematically the current patch series. 


> 
> > +    description: |
> > +      Set the desired polarity for the selected wait pin.
> > +      1 for active low, 0 for active high.
> 
> Well that looks backwards. I assume from the commit msg above, it's the 
> register value, but that's not what the description says. Please go with 
> the logical state here and do the inversion in the driver.
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum: [0, 1]
> > +
> >    gpmc,wait-on-read:
> >      description: Enables wait monitoring on reads.
> >      type: boolean
> > -- 
> > 2.25.1
> > 
> > 
Cheers,
Benedikt

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

[Krzysztof Kozlowski]

On 05/10/2022 12:13, Niedermayr, BENEDIKT wrote:
>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>> child.yaml
>>> index 6e3995bb1630..477189973334 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' is not a vendor. Don't continue this bad pattern, use 'ti'.
> 
> You are right. But nevertheless I can't agree with that in this patch series.
> I don't want to break consistency, since all bindings currently use 'gpmc'. At least this applies
> to the "ti,gpmc-child.yaml".
> 
> I think it makes more sense to create a complete new patch series for that specific change? This change
> wouldn't fit thematically the current patch series. 
> 

So you want to add new property incorrectly named and immediately new
patch which fixes the name? No, please squash this new patch into this.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 05/10/2022 12:05, Niedermayr, BENEDIKT wrote:
> On Fri, 2022-09-30 at 22:01 +0200, Krzysztof Kozlowski wrote:
>> On 30/09/2022 21:42, Rob Herring wrote:
>>> On Thu, Sep 29, 2022 at 02:56:39PM +0200, 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..477189973334 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' is not a vendor. Don't continue this bad pattern, use 'ti'.
>>>
>>>> +    description: |
>>>> +      Set the desired polarity for the selected wait pin.
>>>> +      1 for active low, 0 for active high.
>>>
>>> Well that looks backwards. I assume from the commit msg above, it's the 
>>> register value, but that's not what the description says. Please go with 
>>> the logical state here and do the inversion in the driver.
>>
>> This was actually my suggestion to keep the same value as
>> ACTIVE_HIGH/LOW in standard GPIO flags. The DTS could reuse the defines.
>>
> Ok, but how to proceed know? IMHO it makes more sense to use the value which actually lands in the register since most 
> people will use the value found in the Datasheet. 
> 
> We already had a discussion with Roger about the GPIO bindings vs. wait-pin binding. The point was that we do not use GPIO bindings
> in this case, or? 

Go with Rob's suggestion, so back to previous version.

Best regards,
Krzysztof

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

[Niedermayr, BENEDIKT]

On Wed, 2022-10-05 at 13:00 +0200, Krzysztof Kozlowski wrote:
> On 05/10/2022 12:13, Niedermayr, BENEDIKT wrote:
> > > > diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
> > > > child.yaml
> > > > index 6e3995bb1630..477189973334 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' is not a vendor. Don't continue this bad pattern, use 'ti'.
> > 
> > You are right. But nevertheless I can't agree with that in this patch series.
> > I don't want to break consistency, since all bindings currently use 'gpmc'. At least this applies
> > to the "ti,gpmc-child.yaml".
> > 
> > I think it makes more sense to create a complete new patch series for that specific change? This change
> > wouldn't fit thematically the current patch series. 
> > 
> 
> So you want to add new property incorrectly named and immediately new
> patch which fixes the name? No, please squash this new patch into this.
> 
No that's not what I meant. Currently all bindings in "ti,gpmc-child.yaml" start with "gpmc," and introducing 
a single binding in this file with "ti," feels like breaking consistency.

The "new" patch series should address **all** bindings in this file and all device trees currently using "gpmc,"
bindings. So finally we have the current patch series introducing the wait pin handling in a consisten way and then another 
patch series which changes all "gpmc," to "ti,". 

If it makes more sense to directly introduce the "ti,wait-pin-polarity" instead of "gpmc,wait-pin-polarity" then I will do. Just
give me a short feedback.
  

> Best regards,
> Krzysztof
> 

Cheers,
Benedikt

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

[Krzysztof Kozlowski]

On 05/10/2022 13:15, Niedermayr, BENEDIKT wrote:
> On Wed, 2022-10-05 at 13:00 +0200, Krzysztof Kozlowski wrote:
>> On 05/10/2022 12:13, Niedermayr, BENEDIKT wrote:
>>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-
>>>>> child.yaml
>>>>> index 6e3995bb1630..477189973334 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' is not a vendor. Don't continue this bad pattern, use 'ti'.
>>>
>>> You are right. But nevertheless I can't agree with that in this patch series.
>>> I don't want to break consistency, since all bindings currently use 'gpmc'. At least this applies
>>> to the "ti,gpmc-child.yaml".
>>>
>>> I think it makes more sense to create a complete new patch series for that specific change? This change
>>> wouldn't fit thematically the current patch series. 
>>>
>>
>> So you want to add new property incorrectly named and immediately new
>> patch which fixes the name? No, please squash this new patch into this.
>>
> No that's not what I meant. Currently all bindings in "ti,gpmc-child.yaml" start with "gpmc," and introducing 
> a single binding in this file with "ti," feels like breaking consistency.
> 
> The "new" patch series should address **all** bindings in this file and all device trees currently using "gpmc,"
> bindings. So finally we have the current patch series introducing the wait pin handling in a consisten way and then another 
> patch series which changes all "gpmc," to "ti,". 

Isn't this exactly what I said? First add gpmc (so incorrectly named
property) and then fix it to proper name (TI)? So squash that part of
second patch which relates to this one, into this patch.

Please do not commit incorrect properties, just because they are consistent.

> If it makes more sense to directly introduce the "ti,wait-pin-polarity" instead of "gpmc,wait-pin-polarity" then I will do. Just
> give me a short feedback.
>   

You got it already:

  'gpmc' is not a vendor. Don't continue this bad pattern, use 'ti'.

Best regards,
Krzysztof