* [PATCH v7 0/2] gpmc wait pin additions @ 2022-10-05 14:22 B. Niedermayr 2022-10-05 14:22 ` [PATCH v7 1/2] memory: omap-gpmc: " B. Niedermayr 2022-10-05 14:22 ` [PATCH v7 2/2] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity B. Niedermayr 0 siblings, 2 replies; 6+ messages in thread From: B. Niedermayr @ 2022-10-05 14:22 UTC (permalink / raw) To: devicetree, linux-omap; +Cc: krzysztof.kozlowski, robh+dt, rogerq, tony 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v7 1/2] memory: omap-gpmc: wait pin additions 2022-10-05 14:22 [PATCH v7 0/2] gpmc wait pin additions B. Niedermayr @ 2022-10-05 14:22 ` B. Niedermayr 2022-10-07 11:03 ` Roger Quadros 2022-10-05 14:22 ` [PATCH v7 2/2] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity B. Niedermayr 1 sibling, 1 reply; 6+ messages in thread From: B. Niedermayr @ 2022-10-05 14:22 UTC (permalink / raw) To: devicetree, linux-omap; +Cc: krzysztof.kozlowski, robh+dt, rogerq, tony 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v7 1/2] memory: omap-gpmc: wait pin additions 2022-10-05 14:22 ` [PATCH v7 1/2] memory: omap-gpmc: " B. Niedermayr @ 2022-10-07 11:03 ` Roger Quadros 2022-10-07 11:12 ` Niedermayr, BENEDIKT 0 siblings, 1 reply; 6+ messages in thread From: Roger Quadros @ 2022-10-07 11:03 UTC (permalink / raw) To: B. Niedermayr, devicetree, linux-omap; +Cc: krzysztof.kozlowski, robh+dt, tony 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7 1/2] memory: omap-gpmc: wait pin additions 2022-10-07 11:03 ` Roger Quadros @ 2022-10-07 11:12 ` Niedermayr, BENEDIKT 0 siblings, 0 replies; 6+ messages in thread From: Niedermayr, BENEDIKT @ 2022-10-07 11:12 UTC (permalink / raw) To: rogerq, devicetree, linux-omap; +Cc: tony, krzysztof.kozlowski, robh+dt 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v7 2/2] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity 2022-10-05 14:22 [PATCH v7 0/2] gpmc wait pin additions B. Niedermayr 2022-10-05 14:22 ` [PATCH v7 1/2] memory: omap-gpmc: " B. Niedermayr @ 2022-10-05 14:22 ` B. Niedermayr 2022-10-06 18:54 ` Rob Herring 1 sibling, 1 reply; 6+ messages in thread From: B. Niedermayr @ 2022-10-05 14:22 UTC (permalink / raw) To: devicetree, linux-omap; +Cc: krzysztof.kozlowski, robh+dt, rogerq, tony 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v7 2/2] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity 2022-10-05 14:22 ` [PATCH v7 2/2] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity B. Niedermayr @ 2022-10-06 18:54 ` Rob Herring 0 siblings, 0 replies; 6+ messages in thread From: Rob Herring @ 2022-10-06 18:54 UTC (permalink / raw) To: B. Niedermayr Cc: robh+dt, tony, devicetree, rogerq, linux-omap, krzysztof.kozlowski 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-07 11:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-05 14:22 [PATCH v7 0/2] gpmc wait pin additions B. Niedermayr 2022-10-05 14:22 ` [PATCH v7 1/2] memory: omap-gpmc: " B. Niedermayr 2022-10-07 11:03 ` Roger Quadros 2022-10-07 11:12 ` Niedermayr, BENEDIKT 2022-10-05 14:22 ` [PATCH v7 2/2] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity B. Niedermayr 2022-10-06 18:54 ` Rob Herring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).