[PATCH v7 0/2] gpmc wait pin additions

[PATCH v7 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
  * 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
Changes since v6:
  * Move wait-pin allocation into gpmc_probe()
  * Fix s/gpmc/GPMC/ in commit description
  * use ti,wait-pin-polarity instead of gpmc,wait-pin-polarity
  * Refactored if clause in gpmc_alloc_waitpin()
  * Revert values for GPMC_WAITPINPOLARITY_ACTIVE_LOW and
    GPMC_WAITPINPOLARITY_ACTIVE_HIGH.
    Use the exact same values which are written into the register.

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                    | 123 ++++++++++++++++--
 include/linux/platform_data/gpmc-omap.h       |   8 ++
 3 files changed, 125 insertions(+), 13 deletions(-)

--
2.25.1

[PATCH v7 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 "ti,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              | 122 +++++++++++++++++++++---
 include/linux/platform_data/gpmc-omap.h |   8 ++
 2 files changed, 117 insertions(+), 13 deletions(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index d9bf1c2ac319..95c4e61753c0 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,62 @@ 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];
+
+	/* 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;
+
+	if (!waitpin->desc) {
+		/* New wait pin */
+		waitpin->desc = waitpin_desc;
+		waitpin->pin = p->wait_pin;
+		waitpin->polarity = p->wait_pin_polarity;
+	} else {
+		/* Shared wait pin */
+		if (p->wait_pin_polarity != waitpin->polarity ||
+		    p->wait_pin != waitpin->pin) {
+			dev_err(gpmc->dev,
+				"shared-wait-pin: invalid configuration\n");
+			return -EINVAL;
+		}
+		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 +1945,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_INVALID) {
+		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 +2055,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_INVALID;
+
 	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_INVALID;
+				}
+		}
+
 		p->wait_on_read = of_property_read_bool(np,
 							"gpmc,wait-on-read");
 		p->wait_on_write = of_property_read_bool(np,
@@ -2085,7 +2178,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 +2306,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 +2352,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);
 
@@ -2395,7 +2479,7 @@ static int gpmc_gpio_init(struct gpmc_device *gpmc)
 
 static int gpmc_probe(struct platform_device *pdev)
 {
-	int rc;
+	int rc, i;
 	u32 l;
 	struct resource *res;
 	struct gpmc_device *gpmc;
@@ -2455,6 +2539,15 @@ static int gpmc_probe(struct platform_device *pdev)
 		gpmc_nr_waitpins = GPMC_NR_WAITPINS;
 	}
 
+	gpmc->waitpins = devm_kzalloc(&pdev->dev,
+				      gpmc_nr_waitpins * sizeof(struct gpmc_waitpin),
+				      GFP_KERNEL);
+	if (!gpmc->waitpins)
+		return -ENOMEM;
+
+	for (i = 0; i < gpmc_nr_waitpins; i++)
+		gpmc->waitpins[i].pin = GPMC_WAITPIN_DEFAULT;
+
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
@@ -2505,8 +2598,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..9aa47dd9b6c6 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_INVALID -1
+#define GPMC_WAITPINPOLARITY_ACTIVE_LOW 0
+#define GPMC_WAITPINPOLARITY_ACTIVE_HIGH 1
+
+#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 v7 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 "ti,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..4a257fac577e 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
 
+  ti,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.25.1

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

[Rob Herring]

On Wed, 05 Oct 2022 16:22:24 +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 "ti,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(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Roger Quadros]

Hello Benedikt,

Thanks for doing the changes but still a few comments. ;)

On 05/10/2022 17:22, 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 "ti,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              | 122 +++++++++++++++++++++---
>  include/linux/platform_data/gpmc-omap.h |   8 ++
>  2 files changed, 117 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index d9bf1c2ac319..95c4e61753c0 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,62 @@ 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];
> +
> +	/* 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;

We don't want to request a new GPIO descriptor if waiptin->desc is already present
which means it was requested before.

So let's move the above gpiochip_request_own_desc() call and error check to inside
the below if {}.

> +
> +	if (!waitpin->desc) {
> +		/* New wait pin */

here --->

> +		waitpin->desc = waitpin_desc;
> +		waitpin->pin = p->wait_pin;
> +		waitpin->polarity = p->wait_pin_polarity;
> +	} else {
> +		/* Shared wait pin */
> +		if (p->wait_pin_polarity != waitpin->polarity ||
> +		    p->wait_pin != waitpin->pin) {
> +			dev_err(gpmc->dev,
> +				"shared-wait-pin: invalid configuration\n");
> +			return -EINVAL;
> +		}
> +		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 +1945,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_INVALID) {
> +		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 +2055,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_INVALID;
> +
>  	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",

"ti,wait-pin-polarity" to match what you put in the DT bindings document?

> +					  &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_INVALID;
> +				}
> +		}
> +
>  		p->wait_on_read = of_property_read_bool(np,
>  							"gpmc,wait-on-read");
>  		p->wait_on_write = of_property_read_bool(np,
> @@ -2085,7 +2178,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 +2306,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 +2352,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);
>  
> @@ -2395,7 +2479,7 @@ static int gpmc_gpio_init(struct gpmc_device *gpmc)
>  
>  static int gpmc_probe(struct platform_device *pdev)
>  {
> -	int rc;
> +	int rc, i;
>  	u32 l;
>  	struct resource *res;
>  	struct gpmc_device *gpmc;
> @@ -2455,6 +2539,15 @@ static int gpmc_probe(struct platform_device *pdev)
>  		gpmc_nr_waitpins = GPMC_NR_WAITPINS;
>  	}
>  
> +	gpmc->waitpins = devm_kzalloc(&pdev->dev,
> +				      gpmc_nr_waitpins * sizeof(struct gpmc_waitpin),
> +				      GFP_KERNEL);
> +	if (!gpmc->waitpins)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < gpmc_nr_waitpins; i++)
> +		gpmc->waitpins[i].pin = GPMC_WAITPIN_DEFAULT;
> +
>  	pm_runtime_enable(&pdev->dev);
>  	pm_runtime_get_sync(&pdev->dev);
>  
> @@ -2505,8 +2598,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..9aa47dd9b6c6 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_INVALID -1
> +#define GPMC_WAITPINPOLARITY_ACTIVE_LOW 0
> +#define GPMC_WAITPINPOLARITY_ACTIVE_HIGH 1
> +
> +#define GPMC_WAITPIN_DEFAULT -1

Sorry I missed this one last time. I think it should be called GPMC_WAITPIN_INVALID.

> +
>  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 v7 1/2] memory: omap-gpmc: wait pin additions

[Niedermayr, BENEDIKT]

Hello Roger,

On Fri, 2022-10-07 at 14:03 +0300, Roger Quadros wrote:
> Hello Benedikt,
> 
> Thanks for doing the changes but still a few comments. ;)
> 
> On 05/10/2022 17:22, 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 "ti,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              | 122 +++++++++++++++++++++---
> >  include/linux/platform_data/gpmc-omap.h |   8 ++
> >  2 files changed, 117 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> > index d9bf1c2ac319..95c4e61753c0 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,62 @@ 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];
> > +
> > +	/* 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;
> 
> We don't want to request a new GPIO descriptor if waiptin->desc is already present
> which means it was requested before.
> 
> So let's move the above gpiochip_request_own_desc() call and error check to inside
> the below if {}.
> 
> > +
> > +	if (!waitpin->desc) {
> > +		/* New wait pin */
> 
> here --->
> 
Ok that makes sense. I will change that.

> > +		waitpin->desc = waitpin_desc;
> > +		waitpin->pin = p->wait_pin;
> > +		waitpin->polarity = p->wait_pin_polarity;
> > +	} else {
> > +		/* Shared wait pin */
> > +		if (p->wait_pin_polarity != waitpin->polarity ||
> > +		    p->wait_pin != waitpin->pin) {
> > +			dev_err(gpmc->dev,
> > +				"shared-wait-pin: invalid configuration\n");
> > +			return -EINVAL;
> > +		}
> > +		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 +1945,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_INVALID) {
> > +		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 +2055,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_INVALID;
> > +
> >  	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",
> 
> "ti,wait-pin-polarity" to match what you put in the DT bindings document?
> 
Upps. That's a dump mistake. I thought I tested it. But I only looked at the dmesg output without 
actually checking the CONFIG register afterwards.
We made the property optional which doesn't lead to errors in case we're not using it....

I will change that.


> > +					  &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_INVALID;
> > +				}
> > +		}
> > +
> >  		p->wait_on_read = of_property_read_bool(np,
> >  							"gpmc,wait-on-read");
> >  		p->wait_on_write = of_property_read_bool(np,
> > @@ -2085,7 +2178,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 +2306,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 +2352,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);
> >  
> > @@ -2395,7 +2479,7 @@ static int gpmc_gpio_init(struct gpmc_device *gpmc)
> >  
> >  static int gpmc_probe(struct platform_device *pdev)
> >  {
> > -	int rc;
> > +	int rc, i;
> >  	u32 l;
> >  	struct resource *res;
> >  	struct gpmc_device *gpmc;
> > @@ -2455,6 +2539,15 @@ static int gpmc_probe(struct platform_device *pdev)
> >  		gpmc_nr_waitpins = GPMC_NR_WAITPINS;
> >  	}
> >  
> > +	gpmc->waitpins = devm_kzalloc(&pdev->dev,
> > +				      gpmc_nr_waitpins * sizeof(struct gpmc_waitpin),
> > +				      GFP_KERNEL);
> > +	if (!gpmc->waitpins)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < gpmc_nr_waitpins; i++)
> > +		gpmc->waitpins[i].pin = GPMC_WAITPIN_DEFAULT;
> > +
> >  	pm_runtime_enable(&pdev->dev);
> >  	pm_runtime_get_sync(&pdev->dev);
> >  
> > @@ -2505,8 +2598,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..9aa47dd9b6c6 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_INVALID -1
> > +#define GPMC_WAITPINPOLARITY_ACTIVE_LOW 0
> > +#define GPMC_WAITPINPOLARITY_ACTIVE_HIGH 1
> > +
> > +#define GPMC_WAITPIN_DEFAULT -1
> 
> Sorry I missed this one last time. I think it should be called GPMC_WAITPIN_INVALID.
> 
So I will change GPMC_WAITPIN_DEFAULT -> GPMC_WAITPIN_INVALID as well.

> > +
> >  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
cheers,
benedikt