| Message ID | 1407541685-20021-1-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 9, 2014 at 1:48 AM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > From: Tomasz Figa <t.figa@samsung.com> > > Currently after configuring a GPIO pin as an interrupt related pinmux > registers are changed, but there is no protection from calling > gpio_direction_*() in a badly written driver, which would cause the same > pinmux register to be reconfigured for regular input/output and this > disabling interrupt capability of the pin. > > This patch addresses this issue by moving pinmux reconfiguration to > .irq_{request,release}_resources() callback of irq_chip and calling > gpio_lock_as_irq() helper to prevent reconfiguration of pin direction. > > Setting up a GPIO interrupt on Samsung SoCs is a two-step operation - > in addition to trigger configuration in a dedicated register, the pinmux > must be also reconfigured to GPIO interrupt, which is a different function > than normal GPIO input, although I/O-wise they both behave in the same way > and gpio_get_value() can be used on a pin configured as IRQ as well. > > Such design implies subtleties such as gpio_direction_input() not having > to fail if a pin is already configured as an interrupt nor change the > configuration to normal input. But the FLAG_USED_AS_IRQ set in gpiolib by > gpio_lock_as_irq() is only used to check that gpio_direction_output() is > not called, it's not used to prevent gpio_direction_input() to be called. > So this is not a complete solution for Samsung SoCs but it's definitely a > move in the right direction. > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > [javier: use request resources instead of startup and expand commit message] > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > --- > > This patch solves the issue explained in https://lkml.org/lkml/2014/8/8/461 Hm, this looks much better atleast, it is not possible to use the irqchip helpers from gpiolib then, because that grabs the request/release resource callbacks. If I get some ACKs on this we can go for this solution. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 8, 2014 at 6:48 PM, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > From: Tomasz Figa <t.figa@samsung.com> > > Currently after configuring a GPIO pin as an interrupt related pinmux > registers are changed, but there is no protection from calling > gpio_direction_*() in a badly written driver, which would cause the same > pinmux register to be reconfigured for regular input/output and this > disabling interrupt capability of the pin. > > This patch addresses this issue by moving pinmux reconfiguration to > .irq_{request,release}_resources() callback of irq_chip and calling > gpio_lock_as_irq() helper to prevent reconfiguration of pin direction. > > Setting up a GPIO interrupt on Samsung SoCs is a two-step operation - > in addition to trigger configuration in a dedicated register, the pinmux > must be also reconfigured to GPIO interrupt, which is a different function > than normal GPIO input, although I/O-wise they both behave in the same way > and gpio_get_value() can be used on a pin configured as IRQ as well. > > Such design implies subtleties such as gpio_direction_input() not having > to fail if a pin is already configured as an interrupt nor change the > configuration to normal input. But the FLAG_USED_AS_IRQ set in gpiolib by > gpio_lock_as_irq() is only used to check that gpio_direction_output() is > not called, it's not used to prevent gpio_direction_input() to be called. > So this is not a complete solution for Samsung SoCs but it's definitely a > move in the right direction. > > Signed-off-by: Tomasz Figa <t.figa@samsung.com> > [javier: use request resources instead of startup and expand commit message] > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> OK patch applied for fixes, sorry for missing to follow up on this. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c index 003bfd8..d7154ed 100644 --- a/drivers/pinctrl/samsung/pinctrl-exynos.c +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c @@ -127,14 +127,10 @@ static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type) struct irq_chip *chip = irq_data_get_irq_chip(irqd); struct exynos_irq_chip *our_chip = to_exynos_irq_chip(chip); struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); - struct samsung_pin_bank_type *bank_type = bank->type; struct samsung_pinctrl_drv_data *d = bank->drvdata; - unsigned int pin = irqd->hwirq; - unsigned int shift = EXYNOS_EINT_CON_LEN * pin; + unsigned int shift = EXYNOS_EINT_CON_LEN * irqd->hwirq; unsigned int con, trig_type; unsigned long reg_con = our_chip->eint_con + bank->eint_offset; - unsigned long flags; - unsigned int mask; switch (type) { case IRQ_TYPE_EDGE_RISING: @@ -167,8 +163,32 @@ static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type) con |= trig_type << shift; writel(con, d->virt_base + reg_con); + return 0; +} + +static int exynos_irq_request_resources(struct irq_data *irqd) +{ + struct irq_chip *chip = irq_data_get_irq_chip(irqd); + struct exynos_irq_chip *our_chip = to_exynos_irq_chip(chip); + struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); + struct samsung_pin_bank_type *bank_type = bank->type; + struct samsung_pinctrl_drv_data *d = bank->drvdata; + unsigned int shift = EXYNOS_EINT_CON_LEN * irqd->hwirq; + unsigned long reg_con = our_chip->eint_con + bank->eint_offset; + unsigned long flags; + unsigned int mask; + unsigned int con; + int ret; + + ret = gpio_lock_as_irq(&bank->gpio_chip, irqd->hwirq); + if (ret) { + dev_err(bank->gpio_chip.dev, "unable to lock pin %s-%lu IRQ\n", + bank->name, irqd->hwirq); + return ret; + } + reg_con = bank->pctl_offset + bank_type->reg_offset[PINCFG_TYPE_FUNC]; - shift = pin * bank_type->fld_width[PINCFG_TYPE_FUNC]; + shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC]; mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1; spin_lock_irqsave(&bank->slock, flags); @@ -180,9 +200,42 @@ static int exynos_irq_set_type(struct irq_data *irqd, unsigned int type) spin_unlock_irqrestore(&bank->slock, flags); + exynos_irq_unmask(irqd); + return 0; } +static void exynos_irq_release_resources(struct irq_data *irqd) +{ + struct irq_chip *chip = irq_data_get_irq_chip(irqd); + struct exynos_irq_chip *our_chip = to_exynos_irq_chip(chip); + struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); + struct samsung_pin_bank_type *bank_type = bank->type; + struct samsung_pinctrl_drv_data *d = bank->drvdata; + unsigned int shift = EXYNOS_EINT_CON_LEN * irqd->hwirq; + unsigned long reg_con = our_chip->eint_con + bank->eint_offset; + unsigned long flags; + unsigned int mask; + unsigned int con; + + reg_con = bank->pctl_offset + bank_type->reg_offset[PINCFG_TYPE_FUNC]; + shift = irqd->hwirq * bank_type->fld_width[PINCFG_TYPE_FUNC]; + mask = (1 << bank_type->fld_width[PINCFG_TYPE_FUNC]) - 1; + + exynos_irq_mask(irqd); + + spin_lock_irqsave(&bank->slock, flags); + + con = readl(d->virt_base + reg_con); + con &= ~(mask << shift); + con |= FUNC_INPUT << shift; + writel(con, d->virt_base + reg_con); + + spin_unlock_irqrestore(&bank->slock, flags); + + gpio_unlock_as_irq(&bank->gpio_chip, irqd->hwirq); +} + /* * irq_chip for gpio interrupts. */ @@ -193,6 +246,8 @@ static struct exynos_irq_chip exynos_gpio_irq_chip = { .irq_mask = exynos_irq_mask, .irq_ack = exynos_irq_ack, .irq_set_type = exynos_irq_set_type, + .irq_request_resources = exynos_irq_request_resources, + .irq_release_resources = exynos_irq_release_resources, }, .eint_con = EXYNOS_GPIO_ECON_OFFSET, .eint_mask = EXYNOS_GPIO_EMASK_OFFSET, @@ -336,6 +391,8 @@ static struct exynos_irq_chip exynos_wkup_irq_chip = { .irq_ack = exynos_irq_ack, .irq_set_type = exynos_irq_set_type, .irq_set_wake = exynos_wkup_irq_set_wake, + .irq_request_resources = exynos_irq_request_resources, + .irq_release_resources = exynos_irq_release_resources, }, .eint_con = EXYNOS_WKUP_ECON_OFFSET, .eint_mask = EXYNOS_WKUP_EMASK_OFFSET, diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h index 2b88232..5cedc9d 100644 --- a/drivers/pinctrl/samsung/pinctrl-samsung.h +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h @@ -26,6 +26,7 @@ #include <linux/gpio.h> /* pinmux function number for pin as gpio output line */ +#define FUNC_INPUT 0x0 #define FUNC_OUTPUT 0x1 /**