diff mbox series

[v9,1/2] memory: omap-gpmc: wait pin additions

Message ID 20221102133047.1654449-2-benedikt.niedermayr@siemens.com
State New
Headers show
Series gpmc wait pin additions | expand

Commit Message

B. Niedermayr Nov. 2, 2022, 1:30 p.m. UTC
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(-)

Comments

Tony Lindgren Dec. 7, 2022, 1:51 p.m. UTC | #1
Hi,

* B. Niedermayr <benedikt.niedermayr@siemens.com> [221102 13:21]:
> 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.

Looks like Linux next commit 89aed3cd5cb9 ("memory: omap-gpmc: wait pin
additions") breaks the old smsc911x using devices somehow for nfsroot.

Reverting this commit makes things work again. Any ideas?

Regards,

Tony
B. Niedermayr Dec. 7, 2022, 2:52 p.m. UTC | #2
Hi Tony,

On Wed, 2022-12-07 at 15:51 +0200, Tony Lindgren wrote:
> Hi,
> 
> * B. Niedermayr <benedikt.niedermayr@siemens.com> [221102 13:21]:
> > 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.
> 
> Looks like Linux next commit 89aed3cd5cb9 ("memory: omap-gpmc: wait pin
> additions") breaks the old smsc911x using devices somehow for nfsroot.
> 
Can you explain how this breaking change looks like, in bit more detail?
I'm a bit confused since the changes on omap-gpmc have nothing in common with
smsc911x. 
 
> Reverting this commit makes things work again. Any ideas?
> 
> Regards,
> 
> Tony

cheers,
Benedikt
Tony Lindgren Dec. 7, 2022, 3:06 p.m. UTC | #3
* Niedermayr, BENEDIKT <benedikt.niedermayr@siemens.com> [221207 14:52]:
> Hi Tony,
> 
> On Wed, 2022-12-07 at 15:51 +0200, Tony Lindgren wrote:
> > Hi,
> > 
> > * B. Niedermayr <benedikt.niedermayr@siemens.com> [221102 13:21]:
> > > 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.
> > 
> > Looks like Linux next commit 89aed3cd5cb9 ("memory: omap-gpmc: wait pin
> > additions") breaks the old smsc911x using devices somehow for nfsroot.
> > 
> Can you explain how this breaking change looks like, in bit more detail?
> I'm a bit confused since the changes on omap-gpmc have nothing in common with
> smsc911x. 

The smsc911x device is on gpmc. It's not found with this change.
See arch/arm/boot/dts/omap-gpmc-smsc911x.dtsi for the configuration.

Regards,

Tony
B. Niedermayr Dec. 7, 2022, 5:28 p.m. UTC | #4
Hi Tony,
On Wed, 2022-12-07 at 17:06 +0200, Tony Lindgren wrote:
> * Niedermayr, BENEDIKT <benedikt.niedermayr@siemens.com> [221207 14:52]:
> > Hi Tony,
> > 
> > On Wed, 2022-12-07 at 15:51 +0200, Tony Lindgren wrote:
> > > Hi,
> > > 
> > > * B. Niedermayr <benedikt.niedermayr@siemens.com> [221102 13:21]:
> > > > 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.
> > > 
> > > Looks like Linux next commit 89aed3cd5cb9 ("memory: omap-gpmc: wait pin
> > > additions") breaks the old smsc911x using devices somehow for nfsroot.
> > > 
> > Can you explain how this breaking change looks like, in bit more detail?
> > I'm a bit confused since the changes on omap-gpmc have nothing in common with
> > smsc911x. 
> 
> The smsc911x device is on gpmc. It's not found with this change.
> See arch/arm/boot/dts/omap-gpmc-smsc911x.dtsi for the configuration.
Thanks for the configuration example. 

I found the cause of this bug.
At least when "gpmc_cs_program_settings: invalid wait-pin (-1)" is printed in the kernel log.

Now I'm not sure where to send the bugfix patch (linux-next, linux-omap, both?). 

> 
> Regards,
> 
> Tony

cheers,
benedikt
Tony Lindgren Dec. 8, 2022, 5:49 a.m. UTC | #5
* Niedermayr, BENEDIKT <benedikt.niedermayr@siemens.com> [221207 17:29]:
> I found the cause of this bug.
> At least when "gpmc_cs_program_settings: invalid wait-pin (-1)" is printed in the kernel log.

OK

> Now I'm not sure where to send the bugfix patch (linux-next, linux-omap, both?). 

Please send a fix as against current Linux next as a separate patch as
your earlier patches have been already applied for the merge window.

If dts changes are also needed, let's first try to fix the driver to handle
the invalid wait-pin case. Then we can patch the dts files separately as
needed.

Regards,

Tony
B. Niedermayr Dec. 8, 2022, 3:55 p.m. UTC | #6
Hi Tony,

On Thu, 2022-12-08 at 07:49 +0200, Tony Lindgren wrote:
> * Niedermayr, BENEDIKT <benedikt.niedermayr@siemens.com> [221207 17:29]:
> > I found the cause of this bug.
> > At least when "gpmc_cs_program_settings: invalid wait-pin (-1)" is
> > printed in the kernel log.
> 
> OK
> 
> > Now I'm not sure where to send the bugfix patch (linux-next, linux-omap, 
> > both?). 
> 
> Please send a fix as against current Linux next as a separate patch as
> your earlier patches have been already applied for the merge window.

OK. Thanks!

> If dts changes are also needed, let's first try to fix the driver to
> handle
> the invalid wait-pin case. Then we can patch the dts files separately as
> needed.
No need for dts changes. One concern for the wait-pin implementation was to
not break existing dts's where the wait-pin is not used.   

> 
> Regards,
> 
> Tony

cheers,
Benedikt
Tony Lindgren Dec. 12, 2022, 7:16 a.m. UTC | #7
* Niedermayr, BENEDIKT <benedikt.niedermayr@siemens.com> [221208 15:55]:
> No need for dts changes. One concern for the wait-pin implementation was to
> not break existing dts's where the wait-pin is not used.   

OK thanks.

Tony
diff mbox series

Patch

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 2351f2708da2..e427572712e2 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -134,6 +134,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)
@@ -229,6 +230,12 @@  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;
@@ -236,6 +243,7 @@  struct gpmc_device {
 	struct gpio_chip gpio_chip;
 	struct notifier_block nb;
 	struct omap3_gpmc_regs context;
+	struct gpmc_waitpin *waitpins;
 	int nirqs;
 	unsigned int is_suspended:1;
 	struct resource *data;
@@ -1035,6 +1043,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];
+
+	if (!waitpin->desc) {
+		/* 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;
+
+		/* 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
@@ -1886,6 +1950,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;
 }
 
@@ -1975,7 +2050,25 @@  void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
 				__func__);
 	}
 
+	p->wait_pin = GPMC_WAITPIN_INVALID;
+	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_INVALID;
+		}
+
+		if (!of_property_read_u32(np, "ti,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,
@@ -2080,7 +2173,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) {
@@ -2208,17 +2300,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");
@@ -2260,7 +2344,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);
 
@@ -2489,7 +2573,7 @@  static int omap_gpmc_context_notifier(struct notifier_block *nb,
 
 static int gpmc_probe(struct platform_device *pdev)
 {
-	int rc;
+	int rc, i;
 	u32 l;
 	struct resource *res;
 	struct gpmc_device *gpmc;
@@ -2545,6 +2629,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_INVALID;
+
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_sync(&pdev->dev);
 
@@ -2598,9 +2691,12 @@  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);
 
 	cpu_pm_unregister_notifier(&gpmc->nb);
+	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..296b080c5c67 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_INVALID -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 */