On 15/06/17 20:03, Heiner Kallweit wrote: > Am 15.06.2017 um 18:58 schrieb Marc Zyngier: >> On 15/06/17 17:37, Heiner Kallweit wrote: >>> Am 15.06.2017 um 18:04 schrieb Marc Zyngier: >>>> On 15/06/17 16:24, Heiner Kallweit wrote: >>>>> Am 15.06.2017 um 15:27 schrieb Marc Zyngier: >>>>>> On 15/06/17 14:10, Heiner Kallweit wrote: >>>>>>> Am 13.06.2017 um 10:31 schrieb Marc Zyngier: >>>>>>>> On 10/06/17 22:57, Heiner Kallweit wrote: >>>>>>>>> Add a driver supporting the GPIO interrupt controller on certain >>>>>>>>> Amlogic meson SoC's. >>>>>>>>> >>>>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx> >>>>>>>>> --- >>>>>>>>> v5: >>>>>>>>> - changed Kconfig entry based on Neil's suggestion >>>>>>>>> - added authors >>>>>>>>> - extended explanation why 2 * n hwirqs are used >>>>>>>>> v6: >>>>>>>>> - change DT property parent-interrupts to interrupts >>>>>>>>> v7: >>>>>>>>> - no changes >>>>>>>>> --- >>>>>>>>> drivers/irqchip/Kconfig | 5 + >>>>>>>>> drivers/irqchip/Makefile | 1 + >>>>>>>>> drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++ >>>>>>>>> 3 files changed, 301 insertions(+) >>>>>>>>> create mode 100644 drivers/irqchip/irq-meson-gpio.c >>>>>>>>> >>>>>>>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig >>>>>>>>> index 478f8ace..bdc86e14 100644 >>>>>>>>> --- a/drivers/irqchip/Kconfig >>>>>>>>> +++ b/drivers/irqchip/Kconfig >>>>>>>>> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER >>>>>>>>> help >>>>>>>>> Say yes here to add support for the IRQ combiner devices embedded >>>>>>>>> in Qualcomm Technologies chips. >>>>>>>>> + >>>>>>>>> +config MESON_GPIO_INTC >>>>>>>>> + bool >>>>>>>>> + depends on ARCH_MESON >>>>>>>>> + default y >>>>>>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile >>>>>>>>> index b64c59b8..1be482bd 100644 >>>>>>>>> --- a/drivers/irqchip/Makefile >>>>>>>>> +++ b/drivers/irqchip/Makefile >>>>>>>>> @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o >>>>>>>>> obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o >>>>>>>>> obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o >>>>>>>>> obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o >>>>>>>>> +obj-$(CONFIG_MESON_GPIO_INTC) += irq-meson-gpio.o >>>>>>>>> diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c >>>>>>>>> new file mode 100644 >>>>>>>>> index 00000000..925d00c2 >>>>>>>>> --- /dev/null >>>>>>>>> +++ b/drivers/irqchip/irq-meson-gpio.c >>>>>>>>> @@ -0,0 +1,295 @@ >>>>>>>>> +/* >>>>>>>>> + * Amlogic Meson GPIO IRQ chip driver >>>>>>>>> + * >>>>>>>>> + * Copyright (c) 2015 Endless Mobile, Inc. >>>>>>>>> + * Author: Carlo Caione <carlo@xxxxxxxxxxxx> >>>>>>>>> + * Copyright (c) 2016 BayLibre, SAS. >>>>>>>>> + * Author: Jerome Brunet <jbrunet@xxxxxxxxxxxx> >>>>>>>>> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1@xxxxxxxxx> >>>>>>>>> + * >>>>>>>>> + * This program is free software; you can redistribute it and/or >>>>>>>>> + * modify it under the terms of the GNU General Public License as >>>>>>>>> + * published by the Free Software Foundation, version 2. >>>>>>>>> + */ >>>>>>>>> + >>>>>>>>> +#include <linux/device.h> >>>>>>>>> +#include <linux/init.h> >>>>>>>>> +#include <linux/interrupt.h> >>>>>>>>> +#include <linux/irqchip.h> >>>>>>>>> +#include <linux/of.h> >>>>>>>>> +#include <linux/of_irq.h> >>>>>>>>> +#include <linux/of_address.h> >>>>>>>>> +#include <linux/regmap.h> >>>>>>>>> + >>>>>>>>> +#define REG_EDGE_POL 0x00 >>>>>>>>> +#define REG_PIN_03_SEL 0x04 >>>>>>>>> +#define REG_PIN_47_SEL 0x08 >>>>>>>>> +#define REG_FILTER_SEL 0x0c >>>>>>>>> + >>>>>>>>> +#define REG_EDGE_POL_MASK(x) (BIT(x) | BIT(16 + (x))) >>>>>>>>> +#define REG_EDGE_POL_EDGE(x) BIT(x) >>>>>>>>> +#define REG_EDGE_POL_LOW(x) BIT(16 + (x)) >>>>>>>>> + >>>>>>>>> +#define MAX_PARENT_IRQ_NUM 8 >>>>>>>>> + >>>>>>>>> +/* maximum number of GPIO IRQs on supported platforms */ >>>>>>>>> +#define MAX_NUM_GPIO_IRQ 133 >>>>>>>> >>>>>>>> Why aren't these values coming from DT? I bet that a future revision of >>>>>>>> the same HW will double these. Or at least, you could match it on the >>>>>>>> compatible string. >>>>>>>> >>>>>>> Alternatively this value can be set to 255. The GPIO source is an 8 bit >>>>>>> value with 255 being reserved for "no interrupt source assigned". >>>>>> >>>>>> Who is reserving it? The HW? Or is that your own defined convention? >>>>>> >>>>>>> This way we cover all chips based on the same IP. >>>>>> >>>>>> Why? Where is that 8bit limit coming from? >>>>>> >>>>> The 8 bit limit is in the HW. >>>>> >>>>>>> I think what we could gain by introducing an additional DT property >>>>>>> (saving a few bytes in the irqdomain mapping table) isn't worth the effort. >>>>>> >>>>>> It is not about saving or wasting memory. It is about making the driver >>>>>> and its binding able to span multiple generation of the HW without too >>>>>> much churn. Which is why I'm suggesting that you either define these >>>>>> properties in DT *or* match the compatible string to obtain these values. >>>>>> >>>>>>>>> + >>>>>>>>> +/* >>>>>>>>> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each >>>>>>>>> + * edge. That's due to HW constraints. >>>>>>>>> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one >>>>>>>>> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq >>>>>>>>> + * one bit to the right. >>>>>>>> >>>>>>>> Please expand on how you expect this to work, specially when a random >>>>>>>> driver expects a single interrupt. >>>>>>>> >>>>>>> The gpio interrupt controller in this chip doesn't have native support for >>>>>>> IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio >>>>>>> to two parent interrupts, one for each edge. >>>>>> >>>>>> No, that's horrible, racy, and impractical. It has been proposed in the >>>>>> past (for the same HW), and we're not going there again. >>>>>> >>>>> IIRC what has been proposed before is to re-program the polarity of edge >>>>> detection withing the ISR. This would match your concern that it is racy. >>>>> >>>>> Here it's about using two parent irq's, one programmed to react on the >>>>> rising edge whilst the other is triggered in case of falling edge. >>>>> Would you consider this to be racy too? >>>> >>>> How do you reconcile two interrupts to make look like a single one for a >>>> random, pre-existing driver? >>>> >>>> [...] >>>> >>>>>>>> So you reject EDGE_BOTH? So what's the deal with the beginning of the patch? >>>>>>>> >>>>>>> We reject it in the initial version of the patch set as there's no consensus >>>>>>> yet on some details of the workaround needed for EDGE_BOTH support. >>>>>> >>>>>> There is a consensus: The HW doesn't support this feature. >>>>>> >>>>> Means what? There is no acceptable way to support EDGE_BOTH on this HW? >>>>> In this case I could stop here as for me this feature is important. >>>> >>>> Answer my question above, which I asked in my initial review: How do you >>>> make two interrupts appear as one for a driver that wants to get >>>> signaled on each edge, using the existing API. >>>> >>> Please see the pinctrl/gpio part of the patch set. >>> >>> The GPIO controller has an own IRQ domain. When requesting an interrupt >>> in request_resources if needed two parent irq's are allocated, (was removed >>> in the current initial version of the patch set) both calling the same ISR >>> via a chained interrupt. >>> >>> Works perfectly fine here. >> >> Are you referring to the horror that performs interrupt allocations from >> within the irq_set_type callback? No way. That's beyond disgusting. And >> potentially broken, as the locks that are being taken were never >> designed to nest that way. >> > No, this horror was the first attempt. In v7 of the patch set this is > done from the request_resources callback. Which makes a lot more sense, thanks. It remains that none of that code supports EDGE_BOTH, so please remove all traces of it in your next submission. With the right level of abstraction, you'll be able to add it as a subsequent series, and it will be easier to review once the basics are out of the way. Thanks, M. -- Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
