Hi,
the patchset contains several patches which adding new functionality to
the driver (irq wakeup, suspend/resume) and fixing issue with DATA_RO
reg. The rest of changes are fixing kernel-doc and checkpatch warnings.
Thanks,
Michal
Borsodi Petr (1):
gpio: zynq: Wakeup gpio controller when it is used as IRQ controller
Michal Simek (2):
gpio: zynq: Fix empty lines in driver
gpio: zynq: Fix driver function parameters alignment
Nava kishore Manne (3):
gpio: zynq: Shift zynq_gpio_init() to subsys_initcall level
gpio: zynq: Fix kernel doc warnings
gpio: zynq: Fix warnings in the driver
Shubhrajyoti Datta (1):
gpio: zynq: Add support for suspend resume
Swapna Manupati (1):
gpio: zynq: Provided workaround for GPIO
drivers/gpio/gpio-zynq.c | 201 +++++++++++++++++++++++++++++++++++++++++------
1 file changed, 176 insertions(+), 25 deletions(-)
--
1.9.1
From: Nava kishore Manne <[email protected]>
This patch fixes the below warning
-->Block comments should align the * on each line.
-->suspect code indent for conditional statements.
-->Prefer 'unsigned int' to bare use of 'unsigned'
Signed-off-by: Nava kishore Manne <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
---
drivers/gpio/gpio-zynq.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 2d9f27e34d31..26653bf89e58 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -198,10 +198,10 @@ static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
for (bank = 0; bank < gpio->p_data->max_bank; bank++) {
if ((pin_num >= gpio->p_data->bank_min[bank]) &&
(pin_num <= gpio->p_data->bank_max[bank])) {
- *bank_num = bank;
- *bank_pin_num = pin_num -
- gpio->p_data->bank_min[bank];
- return;
+ *bank_num = bank;
+ *bank_pin_num = pin_num -
+ gpio->p_data->bank_min[bank];
+ return;
}
}
@@ -751,7 +751,7 @@ static int __maybe_unused zynq_gpio_runtime_resume(struct device *dev)
return clk_prepare_enable(gpio->clk);
}
-static int zynq_gpio_request(struct gpio_chip *chip, unsigned offset)
+static int zynq_gpio_request(struct gpio_chip *chip, unsigned int offset)
{
int ret;
@@ -764,7 +764,7 @@ static int zynq_gpio_request(struct gpio_chip *chip, unsigned offset)
return ret < 0 ? ret : 0;
}
-static void zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
+static void zynq_gpio_free(struct gpio_chip *chip, unsigned int offset)
{
pm_runtime_put(chip->parent);
}
--
1.9.1
From: Borsodi Petr <[email protected]>
There is a problem with GPIO driver when used as IRQ controller.
It is not working because the module is sleeping (clock is disabled).
The patch enables clocks when IP is used as IRQ controller.
Signed-off-by: Borsodi Petr <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
---
drivers/gpio/gpio-zynq.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 064033803449..5198fa6e016a 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -11,6 +11,7 @@
#include <linux/bitops.h>
#include <linux/clk.h>
+#include <linux/gpio.h>
#include <linux/gpio/driver.h>
#include <linux/init.h>
#include <linux/interrupt.h>
@@ -20,6 +21,8 @@
#include <linux/pm_runtime.h>
#include <linux/of.h>
+#include "gpiolib.h"
+
#define DRIVER_NAME "zynq-gpio"
/* Maximum banks */
@@ -498,6 +501,38 @@ static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
return 0;
}
+static int zynq_gpio_irq_request_resources(struct irq_data *d)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ int ret;
+
+ if (!try_module_get(chip->gpiodev->owner))
+ return -ENODEV;
+
+ ret = pm_runtime_get_sync(chip->parent);
+ if (ret < 0) {
+ module_put(chip->gpiodev->owner);
+ return ret;
+ }
+
+ if (gpiochip_lock_as_irq(chip, d->hwirq)) {
+ chip_err(chip, "unable to lock HW IRQ %lu for IRQ\n", d->hwirq);
+ pm_runtime_put(chip->parent);
+ module_put(chip->gpiodev->owner);
+ return -EINVAL;
+ }
+ return 0;
+}
+
+static void zynq_gpio_irq_release_resources(struct irq_data *d)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+ gpiochip_unlock_as_irq(chip, d->hwirq);
+ pm_runtime_put(chip->parent);
+ module_put(chip->gpiodev->owner);
+}
+
/* irq chip descriptor */
static struct irq_chip zynq_gpio_level_irqchip = {
.name = DRIVER_NAME,
@@ -507,6 +542,8 @@ static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
.irq_unmask = zynq_gpio_irq_unmask,
.irq_set_type = zynq_gpio_set_irq_type,
.irq_set_wake = zynq_gpio_set_wake,
+ .irq_request_resources = zynq_gpio_irq_request_resources,
+ .irq_release_resources = zynq_gpio_irq_release_resources,
.flags = IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED |
IRQCHIP_MASK_ON_SUSPEND,
};
@@ -519,6 +556,8 @@ static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
.irq_unmask = zynq_gpio_irq_unmask,
.irq_set_type = zynq_gpio_set_irq_type,
.irq_set_wake = zynq_gpio_set_wake,
+ .irq_request_resources = zynq_gpio_irq_request_resources,
+ .irq_release_resources = zynq_gpio_irq_release_resources,
.flags = IRQCHIP_MASK_ON_SUSPEND,
};
--
1.9.1
Fix function parameters alignment reported by checkpatch.
Signed-off-by: Michal Simek <[email protected]>
---
drivers/gpio/gpio-zynq.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 26653bf89e58..97b53e557c15 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -197,7 +197,7 @@ static inline void zynq_gpio_get_bank_pin(unsigned int pin_num,
for (bank = 0; bank < gpio->p_data->max_bank; bank++) {
if ((pin_num >= gpio->p_data->bank_min[bank]) &&
- (pin_num <= gpio->p_data->bank_max[bank])) {
+ (pin_num <= gpio->p_data->bank_max[bank])) {
*bank_num = bank;
*bank_pin_num = pin_num -
gpio->p_data->bank_min[bank];
@@ -313,7 +313,7 @@ static int zynq_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
* as inputs.
*/
if (zynq_gpio_is_zynq(gpio) && bank_num == 0 &&
- (bank_pin_num == 7 || bank_pin_num == 8))
+ (bank_pin_num == 7 || bank_pin_num == 8))
return -EINVAL;
/* clear the bit in direction mode reg to set the pin as input */
@@ -514,13 +514,14 @@ static int zynq_gpio_set_irq_type(struct irq_data *irq_data, unsigned int type)
writel_relaxed(int_any,
gpio->base_addr + ZYNQ_GPIO_INTANY_OFFSET(bank_num));
- if (type & IRQ_TYPE_LEVEL_MASK) {
+ if (type & IRQ_TYPE_LEVEL_MASK)
irq_set_chip_handler_name_locked(irq_data,
- &zynq_gpio_level_irqchip, handle_fasteoi_irq, NULL);
- } else {
+ &zynq_gpio_level_irqchip,
+ handle_fasteoi_irq, NULL);
+ else
irq_set_chip_handler_name_locked(irq_data,
- &zynq_gpio_edge_irqchip, handle_level_irq, NULL);
- }
+ &zynq_gpio_edge_irqchip,
+ handle_level_irq, NULL);
return 0;
}
@@ -772,7 +773,7 @@ static void zynq_gpio_free(struct gpio_chip *chip, unsigned int offset)
static const struct dev_pm_ops zynq_gpio_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(zynq_gpio_suspend, zynq_gpio_resume)
SET_RUNTIME_PM_OPS(zynq_gpio_runtime_suspend,
- zynq_gpio_runtime_resume, NULL)
+ zynq_gpio_runtime_resume, NULL)
};
static const struct zynq_platform_data zynqmp_gpio_def = {
--
1.9.1
From: Nava kishore Manne <[email protected]>
In general situation on-SoC GPIO controller drivers should be probed
after pinctrl/pinmux controller driver, because on-SoC GPIOs utilize a
pin/pad as a resource provided and controlled by pinctrl subsystem.
GPIO must come after pinctrl as gpios may need to mux pins....etc
Looking at Xilinx SoC series pinctrl drivers, zynq*_pinctrl_init()
functions are called at arch_initcall init levels,
so the change of initcall level for gpio-zynq driver from
postcore_initcall to subsys_initcall level is sufficient. Also note
that the most of GPIO controller drivers settled at subsys_initcall
level.
If pinctrl subsystem manages pads with GPIO functions, the change is
needed to avoid unwanted driver probe deferrals during kernel boot.
Signed-off-by: Nava kishore Manne <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
---
drivers/gpio/gpio-zynq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 5198fa6e016a..bcf11f0ef5c3 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -929,7 +929,7 @@ static int __init zynq_gpio_init(void)
{
return platform_driver_register(&zynq_gpio_driver);
}
-postcore_initcall(zynq_gpio_init);
+subsys_initcall(zynq_gpio_init);
static void __exit zynq_gpio_exit(void)
{
--
1.9.1
Remove one additional line and add two new. All are reported by checkpatch.
Signed-off-by: Michal Simek <[email protected]>
empty
---
drivers/gpio/gpio-zynq.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 573bd8303d5d..2d9f27e34d31 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -63,7 +63,6 @@
#define ZYNQ_GPIO_BANK5_PIN_MAX(str) (ZYNQ_GPIO_BANK5_PIN_MIN(str) + \
ZYNQ##str##_GPIO_BANK5_NGPIO - 1)
-
/* Register offsets for the GPIO device */
/* LSW Mask & Data -WO */
#define ZYNQ_GPIO_DATA_LSW_OFFSET(BANK) (0x000 + (8 * BANK))
@@ -115,6 +114,7 @@ struct gpio_regs {
u32 int_polarity[ZYNQMP_GPIO_MAX_BANK];
u32 int_any[ZYNQMP_GPIO_MAX_BANK];
};
+
/**
* struct zynq_gpio - gpio device private data structure
* @chip: instance of the gpio_chip
@@ -700,6 +700,7 @@ static void zynq_gpio_restore_context(struct zynq_gpio *gpio)
ZYNQ_GPIO_INTANY_OFFSET(bank_num));
}
}
+
static int __maybe_unused zynq_gpio_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
--
1.9.1
From: Nava kishore Manne <[email protected]>
This patch fixes the kernel doc warnings in the driver.
Signed-off-by: Nava kishore Manne <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
---
drivers/gpio/gpio-zynq.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 1ab0f8c991b6..573bd8303d5d 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -136,11 +136,12 @@ struct zynq_gpio {
/**
* struct zynq_platform_data - zynq gpio platform data structure
* @label: string to store in gpio->label
+ * @quirks: Flags is used to identify the platform
* @ngpio: max number of gpio pins
* @max_bank: maximum number of gpio banks
* @bank_min: this array represents bank's min pin
* @bank_max: this array represents bank's max pin
-*/
+ */
struct zynq_platform_data {
const char *label;
u32 quirks;
@@ -183,6 +184,7 @@ static int gpio_data_ro_bug(struct zynq_gpio *gpio)
* pin
* @bank_pin_num: an output parameter used to return pin number within a bank
* for the given gpio pin
+ * @gpio: gpio device data structure
*
* Returns the bank number and pin offset within the bank.
*/
@@ -614,7 +616,6 @@ static void zynq_gpio_handle_bank_irq(struct zynq_gpio *gpio,
/**
* zynq_gpio_irqhandler - IRQ handler for the gpio banks of a gpio device
- * @irq: irq number of the gpio bank where interrupt has occurred
* @desc: irq descriptor instance of the 'irq'
*
* This function reads the Interrupt Status Register of each bank to get the
--
1.9.1
From: Swapna Manupati <[email protected]>
This patch provides workaround in the gpio driver
for Zynq and ZynqMP Platforms by reading pin value
of EMIO banks through DATA register as it was unable
to read the value of it from DATA_RO register.
Signed-off-by: Swapna Manupati <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
---
drivers/gpio/gpio-zynq.c | 41 +++++++++++++++++++++++++++++++++++++----
1 file changed, 37 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index bcf11f0ef5c3..1ab0f8c991b6 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -70,6 +70,7 @@
/* MSW Mask & Data -WO */
#define ZYNQ_GPIO_DATA_MSW_OFFSET(BANK) (0x004 + (8 * BANK))
/* Data Register-RW */
+#define ZYNQ_GPIO_DATA_OFFSET(BANK) (0x040 + (4 * BANK))
#define ZYNQ_GPIO_DATA_RO_OFFSET(BANK) (0x060 + (4 * BANK))
/* Direction mode reg-RW */
#define ZYNQ_GPIO_DIRM_OFFSET(BANK) (0x204 + (0x40 * BANK))
@@ -101,6 +102,7 @@
/* set to differentiate zynq from zynqmp, 0=zynqmp, 1=zynq */
#define ZYNQ_GPIO_QUIRK_IS_ZYNQ BIT(0)
+#define GPIO_QUIRK_DATA_RO_BUG BIT(1)
struct gpio_regs {
u32 datamsw[ZYNQMP_GPIO_MAX_BANK];
@@ -163,6 +165,17 @@ static int zynq_gpio_is_zynq(struct zynq_gpio *gpio)
}
/**
+ * gpio_data_ro_bug - test if HW bug exists or not
+ * @gpio: Pointer to driver data struct
+ *
+ * Return: 0 if bug doesnot exist, 1 if bug exists.
+ */
+static int gpio_data_ro_bug(struct zynq_gpio *gpio)
+{
+ return !!(gpio->p_data->quirks & GPIO_QUIRK_DATA_RO_BUG);
+}
+
+/**
* zynq_gpio_get_bank_pin - Get the bank number and pin number within that bank
* for a given pin in the GPIO device
* @pin_num: gpio pin number within the device
@@ -213,9 +226,28 @@ static int zynq_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
zynq_gpio_get_bank_pin(pin, &bank_num, &bank_pin_num, gpio);
- data = readl_relaxed(gpio->base_addr +
- ZYNQ_GPIO_DATA_RO_OFFSET(bank_num));
-
+ if (gpio_data_ro_bug(gpio)) {
+ if (zynq_gpio_is_zynq(gpio)) {
+ if (bank_num <= 1) {
+ data = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_DATA_RO_OFFSET(bank_num));
+ } else {
+ data = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_DATA_OFFSET(bank_num));
+ }
+ } else {
+ if (bank_num <= 2) {
+ data = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_DATA_RO_OFFSET(bank_num));
+ } else {
+ data = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_DATA_OFFSET(bank_num));
+ }
+ }
+ } else {
+ data = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_DATA_RO_OFFSET(bank_num));
+ }
return (data >> bank_pin_num) & 1;
}
@@ -743,6 +775,7 @@ static void zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
static const struct zynq_platform_data zynqmp_gpio_def = {
.label = "zynqmp_gpio",
+ .quirks = GPIO_QUIRK_DATA_RO_BUG,
.ngpio = ZYNQMP_GPIO_NR_GPIOS,
.max_bank = ZYNQMP_GPIO_MAX_BANK,
.bank_min[0] = ZYNQ_GPIO_BANK0_PIN_MIN(MP),
@@ -761,7 +794,7 @@ static void zynq_gpio_free(struct gpio_chip *chip, unsigned offset)
static const struct zynq_platform_data zynq_gpio_def = {
.label = "zynq_gpio",
- .quirks = ZYNQ_GPIO_QUIRK_IS_ZYNQ,
+ .quirks = ZYNQ_GPIO_QUIRK_IS_ZYNQ | GPIO_QUIRK_DATA_RO_BUG,
.ngpio = ZYNQ_GPIO_NR_GPIOS,
.max_bank = ZYNQ_GPIO_MAX_BANK,
.bank_min[0] = ZYNQ_GPIO_BANK0_PIN_MIN(),
--
1.9.1
From: Shubhrajyoti Datta <[email protected]>
Add support for suspend resume. Now that we can lose context across
a suspend/ resume cycle. Add support for the context restore.
Signed-off-by: Shubhrajyoti Datta <[email protected]>
Signed-off-by: Michal Simek <[email protected]>
---
drivers/gpio/gpio-zynq.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 79 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index df0851464006..064033803449 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -99,6 +99,17 @@
/* set to differentiate zynq from zynqmp, 0=zynqmp, 1=zynq */
#define ZYNQ_GPIO_QUIRK_IS_ZYNQ BIT(0)
+struct gpio_regs {
+ u32 datamsw[ZYNQMP_GPIO_MAX_BANK];
+ u32 datalsw[ZYNQMP_GPIO_MAX_BANK];
+ u32 dirm[ZYNQMP_GPIO_MAX_BANK];
+ u32 outen[ZYNQMP_GPIO_MAX_BANK];
+ u32 int_en[ZYNQMP_GPIO_MAX_BANK];
+ u32 int_dis[ZYNQMP_GPIO_MAX_BANK];
+ u32 int_type[ZYNQMP_GPIO_MAX_BANK];
+ u32 int_polarity[ZYNQMP_GPIO_MAX_BANK];
+ u32 int_any[ZYNQMP_GPIO_MAX_BANK];
+};
/**
* struct zynq_gpio - gpio device private data structure
* @chip: instance of the gpio_chip
@@ -106,6 +117,7 @@
* @clk: clock resource for this controller
* @irq: interrupt for the GPIO device
* @p_data: pointer to platform data
+ * @context: context registers
*/
struct zynq_gpio {
struct gpio_chip chip;
@@ -113,6 +125,7 @@ struct zynq_gpio {
struct clk *clk;
int irq;
const struct zynq_platform_data *p_data;
+ struct gpio_regs context;
};
/**
@@ -560,14 +573,72 @@ static void zynq_gpio_irqhandler(struct irq_desc *desc)
chained_irq_exit(irqchip, desc);
}
+static void zynq_gpio_save_context(struct zynq_gpio *gpio)
+{
+ unsigned int bank_num;
+
+ for (bank_num = 0; bank_num < gpio->p_data->max_bank; bank_num++) {
+ gpio->context.datalsw[bank_num] =
+ readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_DATA_LSW_OFFSET(bank_num));
+ gpio->context.datamsw[bank_num] =
+ readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_DATA_MSW_OFFSET(bank_num));
+ gpio->context.dirm[bank_num] = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_DIRM_OFFSET(bank_num));
+ gpio->context.int_en[bank_num] = readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_INTMASK_OFFSET(bank_num));
+ gpio->context.int_type[bank_num] =
+ readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_INTTYPE_OFFSET(bank_num));
+ gpio->context.int_polarity[bank_num] =
+ readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_INTPOL_OFFSET(bank_num));
+ gpio->context.int_any[bank_num] =
+ readl_relaxed(gpio->base_addr +
+ ZYNQ_GPIO_INTANY_OFFSET(bank_num));
+ }
+}
+
+static void zynq_gpio_restore_context(struct zynq_gpio *gpio)
+{
+ unsigned int bank_num;
+
+ for (bank_num = 0; bank_num < gpio->p_data->max_bank; bank_num++) {
+ writel_relaxed(gpio->context.datalsw[bank_num],
+ gpio->base_addr +
+ ZYNQ_GPIO_DATA_LSW_OFFSET(bank_num));
+ writel_relaxed(gpio->context.datamsw[bank_num],
+ gpio->base_addr +
+ ZYNQ_GPIO_DATA_MSW_OFFSET(bank_num));
+ writel_relaxed(gpio->context.dirm[bank_num],
+ gpio->base_addr +
+ ZYNQ_GPIO_DIRM_OFFSET(bank_num));
+ writel_relaxed(gpio->context.int_en[bank_num],
+ gpio->base_addr +
+ ZYNQ_GPIO_INTEN_OFFSET(bank_num));
+ writel_relaxed(gpio->context.int_type[bank_num],
+ gpio->base_addr +
+ ZYNQ_GPIO_INTTYPE_OFFSET(bank_num));
+ writel_relaxed(gpio->context.int_polarity[bank_num],
+ gpio->base_addr +
+ ZYNQ_GPIO_INTPOL_OFFSET(bank_num));
+ writel_relaxed(gpio->context.int_any[bank_num],
+ gpio->base_addr +
+ ZYNQ_GPIO_INTANY_OFFSET(bank_num));
+ }
+}
static int __maybe_unused zynq_gpio_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
int irq = platform_get_irq(pdev, 0);
struct irq_data *data = irq_get_irq_data(irq);
+ struct zynq_gpio *gpio = platform_get_drvdata(pdev);
- if (!irqd_is_wakeup_set(data))
+ if (!irqd_is_wakeup_set(data)) {
+ zynq_gpio_save_context(gpio);
return pm_runtime_force_suspend(dev);
+ }
return 0;
}
@@ -577,9 +648,14 @@ static int __maybe_unused zynq_gpio_resume(struct device *dev)
struct platform_device *pdev = to_platform_device(dev);
int irq = platform_get_irq(pdev, 0);
struct irq_data *data = irq_get_irq_data(irq);
+ struct zynq_gpio *gpio = platform_get_drvdata(pdev);
+ int ret;
- if (!irqd_is_wakeup_set(data))
- return pm_runtime_force_resume(dev);
+ if (!irqd_is_wakeup_set(data)) {
+ ret = pm_runtime_force_resume(dev);
+ zynq_gpio_restore_context(gpio);
+ return ret;
+ }
return 0;
}
--
1.9.1
On Mon, Aug 7, 2017 at 1:01 PM, Michal Simek <[email protected]> wrote:
> From: Shubhrajyoti Datta <[email protected]>
>
> Add support for suspend resume. Now that we can lose context across
> a suspend/ resume cycle. Add support for the context restore.
>
> Signed-off-by: Shubhrajyoti Datta <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, Aug 7, 2017 at 1:01 PM, Michal Simek <[email protected]> wrote:
> From: Borsodi Petr <[email protected]>
>
> There is a problem with GPIO driver when used as IRQ controller.
> It is not working because the module is sleeping (clock is disabled).
> The patch enables clocks when IP is used as IRQ controller.
>
> Signed-off-by: Borsodi Petr <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>
I'm a bit worried about this patch.
> +static int zynq_gpio_irq_request_resources(struct irq_data *d)
> +{
> + struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> + int ret;
> +
> + if (!try_module_get(chip->gpiodev->owner))
> + return -ENODEV;
You are poking around in gpiolib internals, I don't really like that.
I prefer that you use accessors and try to make the core deal with
this instead of fixing it up with a local hack in the driver.
> + ret = pm_runtime_get_sync(chip->parent);
> + if (ret < 0) {
> + module_put(chip->gpiodev->owner);
> + return ret;
> + }
What you essentially do is disable runtime PM while IRQs are in
use, the patch commit log should say this.
> + if (gpiochip_lock_as_irq(chip, d->hwirq)) {
> + chip_err(chip, "unable to lock HW IRQ %lu for IRQ\n", d->hwirq);
> + pm_runtime_put(chip->parent);
> + module_put(chip->gpiodev->owner);
> + return -EINVAL;
> + }
This is essentially a separate patch that should be done orthogonally.
(I don't care super-much about that though.)
> +static void zynq_gpio_irq_release_resources(struct irq_data *d)
> +{
> + struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +
> + gpiochip_unlock_as_irq(chip, d->hwirq);
> + pm_runtime_put(chip->parent);
> + module_put(chip->gpiodev->owner);
> +}
(...)
> + .irq_request_resources = zynq_gpio_irq_request_resources,
> + .irq_release_resources = zynq_gpio_irq_release_resources,
Look at this from gpiolib.c:
static int gpiochip_irq_reqres(struct irq_data *d)
{
struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
if (!try_module_get(chip->gpiodev->owner))
return -ENODEV;
if (gpiochip_lock_as_irq(chip, d->hwirq)) {
chip_err(chip,
"unable to lock HW IRQ %lu for IRQ\n",
d->hwirq);
module_put(chip->gpiodev->owner);
return -EINVAL;
}
return 0;
}
static void gpiochip_irq_relres(struct irq_data *d)
{
struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
gpiochip_unlock_as_irq(chip, d->hwirq);
module_put(chip->gpiodev->owner);
}
If you add pm_runtime_get_sync()/put to this and export
the functions you have the same thing and you can just reuse this
code instead of copying it.
Arguably the above should indeed have that runtime PM code
(unless we know a better way to deal with IRQs).
So can we fix this in the core and reuse it from there?
Yours,
Linus Walleij
On Mon, Aug 7, 2017 at 1:01 PM, Michal Simek <[email protected]> wrote:
> From: Nava kishore Manne <[email protected]>
>
> In general situation on-SoC GPIO controller drivers should be probed
> after pinctrl/pinmux controller driver, because on-SoC GPIOs utilize a
> pin/pad as a resource provided and controlled by pinctrl subsystem.
>
> GPIO must come after pinctrl as gpios may need to mux pins....etc
>
> Looking at Xilinx SoC series pinctrl drivers, zynq*_pinctrl_init()
> functions are called at arch_initcall init levels,
> so the change of initcall level for gpio-zynq driver from
> postcore_initcall to subsys_initcall level is sufficient. Also note
> that the most of GPIO controller drivers settled at subsys_initcall
> level.
>
> If pinctrl subsystem manages pads with GPIO functions, the change is
> needed to avoid unwanted driver probe deferrals during kernel boot.
>
> Signed-off-by: Nava kishore Manne <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>
Can't you just move it all the way to device_initcall and
simply use the standard module init macros?
builtin_platform_driver(), module_platform_driver()?
Yours,
Linus Walleij
On Mon, Aug 7, 2017 at 1:01 PM, Michal Simek <[email protected]> wrote:
> From: Swapna Manupati <[email protected]>
>
> This patch provides workaround in the gpio driver
> for Zynq and ZynqMP Platforms by reading pin value
> of EMIO banks through DATA register as it was unable
> to read the value of it from DATA_RO register.
>
> Signed-off-by: Swapna Manupati <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>
Patch applied.
(I hope my random application of patches does not screw things
up, then I guess I need to back some out.)
Yours,
Linus Walleij
On Mon, Aug 7, 2017 at 1:01 PM, Michal Simek <[email protected]> wrote:
> From: Nava kishore Manne <[email protected]>
>
> This patch fixes the kernel doc warnings in the driver.
>
> Signed-off-by: Nava kishore Manne <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, Aug 7, 2017 at 1:01 PM, Michal Simek <[email protected]> wrote:
> Remove one additional line and add two new. All are reported by checkpatch.
>
> Signed-off-by: Michal Simek <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, Aug 7, 2017 at 1:02 PM, Michal Simek <[email protected]> wrote:
> From: Nava kishore Manne <[email protected]>
>
> This patch fixes the below warning
> -->Block comments should align the * on each line.
> -->suspect code indent for conditional statements.
> -->Prefer 'unsigned int' to bare use of 'unsigned'
>
> Signed-off-by: Nava kishore Manne <[email protected]>
> Signed-off-by: Michal Simek <[email protected]>
Patch applied.
Yours,
Linus Walleij
On 14.8.2017 15:57, Linus Walleij wrote:
> On Mon, Aug 7, 2017 at 1:01 PM, Michal Simek <[email protected]> wrote:
>
>> From: Swapna Manupati <[email protected]>
>>
>> This patch provides workaround in the gpio driver
>> for Zynq and ZynqMP Platforms by reading pin value
>> of EMIO banks through DATA register as it was unable
>> to read the value of it from DATA_RO register.
>>
>> Signed-off-by: Swapna Manupati <[email protected]>
>> Signed-off-by: Michal Simek <[email protected]>
>
> Patch applied.
>
> (I hope my random application of patches does not screw things
> up, then I guess I need to back some out.)
Should be fine.
Thanks,
Michal
On Mon, Aug 7, 2017 at 1:02 PM, Michal Simek <[email protected]> wrote:
> Fix function parameters alignment reported by checkpatch.
>
> Signed-off-by: Michal Simek <[email protected]>
Patch applied.
Now you just need to rebase and figure out the two patches
I had comments on.
Yours,
Linus Walleij
On 14.8.2017 16:01, Linus Walleij wrote:
> On Mon, Aug 7, 2017 at 1:02 PM, Michal Simek <[email protected]> wrote:
>
>> Fix function parameters alignment reported by checkpatch.
>>
>> Signed-off-by: Michal Simek <[email protected]>
>
> Patch applied.
>
> Now you just need to rebase and figure out the two patches
> I had comments on.
Looking at that. That level one first and then the irq one.
This came from 3rd party that's why it will require some time to look at it.
Thanks,
Michal
On 14.8.2017 15:55, Linus Walleij wrote:
> On Mon, Aug 7, 2017 at 1:01 PM, Michal Simek <[email protected]> wrote:
>
>> From: Nava kishore Manne <[email protected]>
>>
>> In general situation on-SoC GPIO controller drivers should be probed
>> after pinctrl/pinmux controller driver, because on-SoC GPIOs utilize a
>> pin/pad as a resource provided and controlled by pinctrl subsystem.
>>
>> GPIO must come after pinctrl as gpios may need to mux pins....etc
>>
>> Looking at Xilinx SoC series pinctrl drivers, zynq*_pinctrl_init()
>> functions are called at arch_initcall init levels,
>> so the change of initcall level for gpio-zynq driver from
>> postcore_initcall to subsys_initcall level is sufficient. Also note
>> that the most of GPIO controller drivers settled at subsys_initcall
>> level.
>>
>> If pinctrl subsystem manages pads with GPIO functions, the change is
>> needed to avoid unwanted driver probe deferrals during kernel boot.
>>
>> Signed-off-by: Nava kishore Manne <[email protected]>
>> Signed-off-by: Michal Simek <[email protected]>
>
> Can't you just move it all the way to device_initcall and
> simply use the standard module init macros?
> builtin_platform_driver(), module_platform_driver()?
When I grep the kernel I see this
[linux](master)$ git grep "^core_initcall" drivers/gpio/ | wc -l
1
[linux](master)$ git grep "^postcore_initcall" drivers/gpio/ | wc -l
12
[linux](master)$ git grep "^arch_initcall" drivers/gpio/ | wc -l
2
[linux](master)$ git grep "^subsys_initcall" drivers/gpio/ | wc -l
33
[linux](master)$ git grep "^device_initcall" drivers/gpio/ | wc -l
4
[linux](master)$ git grep "^core_initcall" drivers/pinctrl/ | wc -l
6
[linux](master)$ git grep "^postcore_initcall" drivers/pinctrl/ | wc -l
7
[linux](master)$ git grep "^arch_initcall" drivers/pinctrl/ | wc -l
62
[linux](master)$ git grep "^subsys_initcall" drivers/pinctrl/ | wc -l
12
[linux](master)$ git grep "^device_initcall" drivers/pinctrl/ | wc -l
0
Majority of gpio drivers are in subsys_initcall and pinctrl in
arch_initcall. It doesn't mean that I have strong opinion about doing
this change. I have also read internal tracking system and it is not
fully clear if this is fixing any issue rather than removing on
deferring probe message.
Nava: Do you have any comment?
Thanks,
Michal
On 14.8.2017 15:53, Linus Walleij wrote:
> On Mon, Aug 7, 2017 at 1:01 PM, Michal Simek <[email protected]> wrote:
>
>> From: Borsodi Petr <[email protected]>
>>
>> There is a problem with GPIO driver when used as IRQ controller.
>> It is not working because the module is sleeping (clock is disabled).
>> The patch enables clocks when IP is used as IRQ controller.
>>
>> Signed-off-by: Borsodi Petr <[email protected]>
>> Signed-off-by: Michal Simek <[email protected]>
>
> I'm a bit worried about this patch.
>
>> +static int zynq_gpio_irq_request_resources(struct irq_data *d)
>> +{
>> + struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> + int ret;
>> +
>> + if (!try_module_get(chip->gpiodev->owner))
>> + return -ENODEV;
>
> You are poking around in gpiolib internals, I don't really like that.
> I prefer that you use accessors and try to make the core deal with
> this instead of fixing it up with a local hack in the driver.
>
>> + ret = pm_runtime_get_sync(chip->parent);
>> + if (ret < 0) {
>> + module_put(chip->gpiodev->owner);
>> + return ret;
>> + }
>
> What you essentially do is disable runtime PM while IRQs are in
> use, the patch commit log should say this.
>
>> + if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>> + chip_err(chip, "unable to lock HW IRQ %lu for IRQ\n", d->hwirq);
>> + pm_runtime_put(chip->parent);
>> + module_put(chip->gpiodev->owner);
>> + return -EINVAL;
>> + }
>
> This is essentially a separate patch that should be done orthogonally.
> (I don't care super-much about that though.)
>
>> +static void zynq_gpio_irq_release_resources(struct irq_data *d)
>> +{
>> + struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +
>> + gpiochip_unlock_as_irq(chip, d->hwirq);
>> + pm_runtime_put(chip->parent);
>> + module_put(chip->gpiodev->owner);
>> +}
> (...)
>> + .irq_request_resources = zynq_gpio_irq_request_resources,
>> + .irq_release_resources = zynq_gpio_irq_release_resources,
>
> Look at this from gpiolib.c:
>
> static int gpiochip_irq_reqres(struct irq_data *d)
> {
> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>
> if (!try_module_get(chip->gpiodev->owner))
> return -ENODEV;
>
> if (gpiochip_lock_as_irq(chip, d->hwirq)) {
> chip_err(chip,
> "unable to lock HW IRQ %lu for IRQ\n",
> d->hwirq);
> module_put(chip->gpiodev->owner);
> return -EINVAL;
> }
> return 0;
> }
>
> static void gpiochip_irq_relres(struct irq_data *d)
> {
> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>
> gpiochip_unlock_as_irq(chip, d->hwirq);
> module_put(chip->gpiodev->owner);
> }
>
> If you add pm_runtime_get_sync()/put to this and export
> the functions you have the same thing and you can just reuse this
> code instead of copying it.
>
> Arguably the above should indeed have that runtime PM code
> (unless we know a better way to deal with IRQs).
>
> So can we fix this in the core and reuse it from there?
I have checked 4.13-rc1 and none is doing anything with clock in these
irq routines.
It means it is a question if they have the same issue when device is
sleeping or we do something wrong.
It is not a problem to move these calls to core (patch is quite simple)
but validate that if this is correct on others SoC.
Do you know if we can validate this on different SoC?
Nava: Can you please validate this again on ZynqMP?
Thanks,
Michal
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9568708a550b..a08a044fa4aa 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1647,14 +1647,22 @@ static void gpiochip_irq_unmap(struct irq_domain
*d, unsigned int irq)
static int gpiochip_irq_reqres(struct irq_data *d)
{
struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ int ret;
if (!try_module_get(chip->gpiodev->owner))
return -ENODEV;
+ ret = pm_runtime_get_sync(chip->parent);
+ if (ret < 0) {
+ module_put(chip->gpiodev->owner);
+ return ret;
+ }
+
if (gpiochip_lock_as_irq(chip, d->hwirq)) {
chip_err(chip,
"unable to lock HW IRQ %lu for IRQ\n",
d->hwirq);
+ pm_runtime_put(chip->parent);
module_put(chip->gpiodev->owner);
return -EINVAL;
}
@@ -1666,6 +1674,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
gpiochip_unlock_as_irq(chip, d->hwirq);
+ pm_runtime_put(chip->parent);
module_put(chip->gpiodev->owner);
}
On Mon, Aug 14, 2017 at 4:33 PM, Michal Simek <[email protected]> wrote:
> I have checked 4.13-rc1 and none is doing anything with clock in these
> irq routines.
> It means it is a question if they have the same issue when device is
> sleeping or we do something wrong.
No but they may get in the future and new drivers may have
the issue.
> It is not a problem to move these calls to core (patch is quite simple)
> but validate that if this is correct on others SoC.
> Do you know if we can validate this on different SoC?
pm_runtime_get() etc are only utilized if the driver
explicitly enable runtime PM, and if they do, they should
have their semantics right for this or their code would be
broken severely.
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 9568708a550b..a08a044fa4aa 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1647,14 +1647,22 @@ static void gpiochip_irq_unmap(struct irq_domain
> *d, unsigned int irq)
> static int gpiochip_irq_reqres(struct irq_data *d)
> {
> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> + int ret;
>
> if (!try_module_get(chip->gpiodev->owner))
> return -ENODEV;
>
> + ret = pm_runtime_get_sync(chip->parent);
> + if (ret < 0) {
> + module_put(chip->gpiodev->owner);
> + return ret;
> + }
> +
> if (gpiochip_lock_as_irq(chip, d->hwirq)) {
> chip_err(chip,
> "unable to lock HW IRQ %lu for IRQ\n",
> d->hwirq);
> + pm_runtime_put(chip->parent);
> module_put(chip->gpiodev->owner);
> return -EINVAL;
> }
> @@ -1666,6 +1674,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>
> gpiochip_unlock_as_irq(chip, d->hwirq);
> + pm_runtime_put(chip->parent);
> module_put(chip->gpiodev->owner);
This looks fine, I'm happy to apply that early for v4.15 after the merge
window (now it is a bit late for radical changes).
Yours,
Linus Walleij
On Mon, Aug 14, 2017 at 4:15 PM, Michal Simek <[email protected]> wrote:
>> Can't you just move it all the way to device_initcall and
>> simply use the standard module init macros?
>> builtin_platform_driver(), module_platform_driver()?
>
> When I grep the kernel I see this
>
> [linux](master)$ git grep "^core_initcall" drivers/gpio/ | wc -l
> 1
> [linux](master)$ git grep "^postcore_initcall" drivers/gpio/ | wc -l
> 12
> [linux](master)$ git grep "^arch_initcall" drivers/gpio/ | wc -l
> 2
> [linux](master)$ git grep "^subsys_initcall" drivers/gpio/ | wc -l
> 33
> [linux](master)$ git grep "^device_initcall" drivers/gpio/ | wc -l
> 4
>
>
> [linux](master)$ git grep "^core_initcall" drivers/pinctrl/ | wc -l
> 6
> [linux](master)$ git grep "^postcore_initcall" drivers/pinctrl/ | wc -l
> 7
> [linux](master)$ git grep "^arch_initcall" drivers/pinctrl/ | wc -l
> 62
> [linux](master)$ git grep "^subsys_initcall" drivers/pinctrl/ | wc -l
> 12
> [linux](master)$ git grep "^device_initcall" drivers/pinctrl/ | wc -l
> 0
>
> Majority of gpio drivers are in subsys_initcall and pinctrl in
> arch_initcall.
The majority is likely wrong, we don't vote about what is the
best code quality luckily :D
You do not see a lot of device_initicall because in the majority
of cases these come implicitly from module_platform_driver(),
builtin_platform_driver_probe() or builtin_platform_driver()
see include/linux/platform_device.h
> It doesn't mean that I have strong opinion about doing
> this change. I have also read internal tracking system and it is not
> fully clear if this is fixing any issue rather than removing on
> deferring probe message.
I think you can make it into module_platform_driver() please
try that approach.
Yours,
Linus Walleij
Hello,
I am reviving this old thread, because the proposed patch (almost)
solves the problem I recently reported with the bad interaction of
runtime PM with the Zynq GPIO driver (see
https://www.spinics.net/lists/linux-gpio/msg35437.html).
On Mon, 14 Aug 2017 16:33:09 +0200, Michal Simek wrote:
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 9568708a550b..a08a044fa4aa 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1647,14 +1647,22 @@ static void gpiochip_irq_unmap(struct irq_domain
> *d, unsigned int irq)
> static int gpiochip_irq_reqres(struct irq_data *d)
> {
> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> + int ret;
>
> if (!try_module_get(chip->gpiodev->owner))
> return -ENODEV;
>
> + ret = pm_runtime_get_sync(chip->parent);
> + if (ret < 0) {
> + module_put(chip->gpiodev->owner);
> + return ret;
> + }
> +
> if (gpiochip_lock_as_irq(chip, d->hwirq)) {
> chip_err(chip,
> "unable to lock HW IRQ %lu for IRQ\n",
> d->hwirq);
> + pm_runtime_put(chip->parent);
> module_put(chip->gpiodev->owner);
> return -EINVAL;
> }
> @@ -1666,6 +1674,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>
> gpiochip_unlock_as_irq(chip, d->hwirq);
> + pm_runtime_put(chip->parent);
> module_put(chip->gpiodev->owner);
> }
This patch almost solves the problem. It doesn't work as-is, because it
assumes that runtime PM is used by all GPIO controllers, which is not
the case. When runtime PM is not enabled, pm_runtime_get_sync() fails
with -EACCES, and the whole gpiochip_irq_reqres() function aborts.
The following patch works fine in my case (a MMC card detect signal is
connected to a pin of a PCA GPIO expander over I2C, whose INT# pin is
itself connected to a GPIO pin of the Zynq SoC).
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 20887c62fbb3..bd9a81fc8d56 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -27,6 +27,7 @@
#include <linux/kfifo.h>
#include <linux/poll.h>
#include <linux/timekeeping.h>
+#include <linux/pm_runtime.h>
#include <uapi/linux/gpio.h>
#include "gpiolib.h"
@@ -3540,12 +3541,23 @@ int gpiochip_reqres_irq(struct gpio_chip *chip, unsigned int offset)
if (!try_module_get(chip->gpiodev->owner))
return -ENODEV;
+ if (pm_runtime_enabled(chip->parent)) {
+ ret = pm_runtime_get_sync(chip->parent);
+ if (ret < 0) {
+ module_put(chip->gpiodev->owner);
+ return ret;
+ }
+ }
+
ret = gpiochip_lock_as_irq(chip, offset);
if (ret) {
chip_err(chip, "unable to lock HW IRQ %u for IRQ\n", offset);
+ if (pm_runtime_enabled(chip->parent))
+ pm_runtime_put(chip->parent);
module_put(chip->gpiodev->owner);
return ret;
}
+
return 0;
}
EXPORT_SYMBOL_GPL(gpiochip_reqres_irq);
@@ -3553,6 +3565,8 @@ EXPORT_SYMBOL_GPL(gpiochip_reqres_irq);
void gpiochip_relres_irq(struct gpio_chip *chip, unsigned int offset)
{
gpiochip_unlock_as_irq(chip, offset);
+ if (pm_runtime_enabled(chip->parent))
+ pm_runtime_put(chip->parent);
module_put(chip->gpiodev->owner);
}
EXPORT_SYMBOL_GPL(gpiochip_relres_irq);
However, I must say that from a design perspective, I am not a big fan
of this solution. Indeed for the normal GPIO ->request() and ->free()
hooks, it is currently the GPIO driver itself that is responsible for
runtime PM get/put, so it would be weird to have the runtime PM get/put
for the IRQ request/free be done by the GPIO core.
I believe that either the GPIO core should be in charge of the entire
runtime PM interaction, or it should entirely be the responsibility of
each GPIO controller driver. Having a mixed solution seems very
confusing.
Let me know which direction should be taken so that I can submit a
proper patch to hopefully resolve this issue.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 07. 01. 19 16:42, Thomas Petazzoni wrote:
> Hello,
>
> I am reviving this old thread, because the proposed patch (almost)
> solves the problem I recently reported with the bad interaction of
> runtime PM with the Zynq GPIO driver (see
> https://www.spinics.net/lists/linux-gpio/msg35437.html).
>
> On Mon, 14 Aug 2017 16:33:09 +0200, Michal Simek wrote:
>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 9568708a550b..a08a044fa4aa 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -1647,14 +1647,22 @@ static void gpiochip_irq_unmap(struct irq_domain
>> *d, unsigned int irq)
>> static int gpiochip_irq_reqres(struct irq_data *d)
>> {
>> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> + int ret;
>>
>> if (!try_module_get(chip->gpiodev->owner))
>> return -ENODEV;
>>
>> + ret = pm_runtime_get_sync(chip->parent);
>> + if (ret < 0) {
>> + module_put(chip->gpiodev->owner);
>> + return ret;
>> + }
>> +
>> if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>> chip_err(chip,
>> "unable to lock HW IRQ %lu for IRQ\n",
>> d->hwirq);
>> + pm_runtime_put(chip->parent);
>> module_put(chip->gpiodev->owner);
>> return -EINVAL;
>> }
>> @@ -1666,6 +1674,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
>> struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>>
>> gpiochip_unlock_as_irq(chip, d->hwirq);
>> + pm_runtime_put(chip->parent);
>> module_put(chip->gpiodev->owner);
>> }
>
> This patch almost solves the problem. It doesn't work as-is, because it
> assumes that runtime PM is used by all GPIO controllers, which is not
> the case. When runtime PM is not enabled, pm_runtime_get_sync() fails
> with -EACCES, and the whole gpiochip_irq_reqres() function aborts.
>
> The following patch works fine in my case (a MMC card detect signal is
> connected to a pin of a PCA GPIO expander over I2C, whose INT# pin is
> itself connected to a GPIO pin of the Zynq SoC).
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 20887c62fbb3..bd9a81fc8d56 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -27,6 +27,7 @@
> #include <linux/kfifo.h>
> #include <linux/poll.h>
> #include <linux/timekeeping.h>
> +#include <linux/pm_runtime.h>
> #include <uapi/linux/gpio.h>
>
> #include "gpiolib.h"
> @@ -3540,12 +3541,23 @@ int gpiochip_reqres_irq(struct gpio_chip *chip, unsigned int offset)
> if (!try_module_get(chip->gpiodev->owner))
> return -ENODEV;
>
> + if (pm_runtime_enabled(chip->parent)) {
> + ret = pm_runtime_get_sync(chip->parent);
> + if (ret < 0) {
> + module_put(chip->gpiodev->owner);
> + return ret;
> + }
> + }
> +
> ret = gpiochip_lock_as_irq(chip, offset);
> if (ret) {
> chip_err(chip, "unable to lock HW IRQ %u for IRQ\n", offset);
> + if (pm_runtime_enabled(chip->parent))
> + pm_runtime_put(chip->parent);
> module_put(chip->gpiodev->owner);
> return ret;
> }
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(gpiochip_reqres_irq);
> @@ -3553,6 +3565,8 @@ EXPORT_SYMBOL_GPL(gpiochip_reqres_irq);
> void gpiochip_relres_irq(struct gpio_chip *chip, unsigned int offset)
> {
> gpiochip_unlock_as_irq(chip, offset);
> + if (pm_runtime_enabled(chip->parent))
> + pm_runtime_put(chip->parent);
> module_put(chip->gpiodev->owner);
> }
> EXPORT_SYMBOL_GPL(gpiochip_relres_irq);
>
> However, I must say that from a design perspective, I am not a big fan
> of this solution. Indeed for the normal GPIO ->request() and ->free()
> hooks, it is currently the GPIO driver itself that is responsible for
> runtime PM get/put, so it would be weird to have the runtime PM get/put
> for the IRQ request/free be done by the GPIO core.
>
> I believe that either the GPIO core should be in charge of the entire
> runtime PM interaction, or it should entirely be the responsibility of
> each GPIO controller driver. Having a mixed solution seems very
> confusing.
>
> Let me know which direction should be taken so that I can submit a
> proper patch to hopefully resolve this issue.
I think it is up to Linus to say which way he wants to go. We found that
way which omap is using.
In connection to this old patch. I think I have tested it later and
wasn't able to replicate it that's why I stop keep track on this.
Thanks,
Michal
On Mon, Jan 7, 2019 at 4:42 PM Thomas Petazzoni
<[email protected]> wrote:
> This patch almost solves the problem. It doesn't work as-is, because it
> assumes that runtime PM is used by all GPIO controllers, which is not
> the case. When runtime PM is not enabled, pm_runtime_get_sync() fails
> with -EACCES, and the whole gpiochip_irq_reqres() function aborts.
(...)
> However, I must say that from a design perspective, I am not a big fan
> of this solution. Indeed for the normal GPIO ->request() and ->free()
> hooks, it is currently the GPIO driver itself that is responsible for
> runtime PM get/put, so it would be weird to have the runtime PM get/put
> for the IRQ request/free be done by the GPIO core.
>
> I believe that either the GPIO core should be in charge of the entire
> runtime PM interaction, or it should entirely be the responsibility of
> each GPIO controller driver. Having a mixed solution seems very
> confusing.
My stance is that the driver is responsible of enabling and managing
runtime PM for its hardware block(s).
Runtime PM in the core should only be added if the core needs to
be aware about it, such as is the case when e.g. a block device
needs to drain its write buffer before going to runtime sleep.
I fail so see why the GPIO core need to be aware about this.
Yours,
Linus Walleij
Hello Linus,
On Fri, 11 Jan 2019 10:54:20 +0100, Linus Walleij wrote:
> My stance is that the driver is responsible of enabling and managing
> runtime PM for its hardware block(s).
>
> Runtime PM in the core should only be added if the core needs to
> be aware about it, such as is the case when e.g. a block device
> needs to drain its write buffer before going to runtime sleep.
>
> I fail so see why the GPIO core need to be aware about this.
In this very same thread at
https://www.spinics.net/lists/arm-kernel/msg600515.html, you kind of
proposed to handle this in the core in fact :-) Though indeed you said
that the core could provide helpers.
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Fri, Jan 11, 2019 at 1:54 PM Thomas Petazzoni
<[email protected]> wrote:
> On Fri, 11 Jan 2019 10:54:20 +0100, Linus Walleij wrote:
>
> > My stance is that the driver is responsible of enabling and managing
> > runtime PM for its hardware block(s).
> >
> > Runtime PM in the core should only be added if the core needs to
> > be aware about it, such as is the case when e.g. a block device
> > needs to drain its write buffer before going to runtime sleep.
> >
> > I fail so see why the GPIO core need to be aware about this.
>
> In this very same thread at
> https://www.spinics.net/lists/arm-kernel/msg600515.html, you kind of
> proposed to handle this in the core in fact :-) Though indeed you said
> that the core could provide helpers.
Yeah allright, I have never been good with consistency but what I guess
I would mean to say (today) is that the driver needs to be in the driver
seat (heh) and opting in to any runtime PM support.
This is in contrast with "midlayer" where all drivers are forced to behave
"as if" they had runtime PM (i.e. calls are done to the runtime PM helpers
even if the device doesn't really activate runtime PM).
Yours,
Linus Walleij
On Fri, Jan 11, 2019 at 8:26 PM Linus Walleij <[email protected]> wrote:
>
> On Fri, Jan 11, 2019 at 1:54 PM Thomas Petazzoni
> <[email protected]> wrote:
> > On Fri, 11 Jan 2019 10:54:20 +0100, Linus Walleij wrote:
> >
> > > My stance is that the driver is responsible of enabling and managing
> > > runtime PM for its hardware block(s).
> > >
> > > Runtime PM in the core should only be added if the core needs to
> > > be aware about it, such as is the case when e.g. a block device
> > > needs to drain its write buffer before going to runtime sleep.
> > >
> > > I fail so see why the GPIO core need to be aware about this.
> >
> > In this very same thread at
> > https://www.spinics.net/lists/arm-kernel/msg600515.html, you kind of
I was not abloe to open the link could you please let me know if I am
missing something?