* [PATCH 0/4] irqchip/meson-gpio: Add support for Meson-A1 SoC @ 2019-12-06 12:17 Qianggui Song 2019-12-06 12:17 ` [PATCH 1/4] dt-bindings: interrupt-controller: New binding for Meson-A1 SoCs Qianggui Song ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Qianggui Song @ 2019-12-06 12:17 UTC (permalink / raw) To: Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Qianggui Song, devicetree, Hanjie Lin, Jianxin Pan, Neil Armstrong, Kevin Hilman, linux-kernel, Rob Herring, linux-arm-kernel, linux-amlogic, Mark Rutland, Xingyu Chen, Jerome Brunet This patchset add support for GPIO interrupt controller of Meson-A1 SoC which use new reigster layout, two main things are done in the patchset 1. rework current driver 2. add a1 support Qianggui Song (4): dt-bindings: interrupt-controller: New binding for Meson-A1 SoCs irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs irqchip/meson-gpio: Add support for meson a1 SoCs arm64: dts: meson: a1: add gpio interrupt controller support .../amlogic,meson-gpio-intc.txt | 1 + arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 9 ++ drivers/irqchip/irq-meson-gpio.c | 126 +++++++++++++++--- 3 files changed, 117 insertions(+), 19 deletions(-) -- 2.24.0 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/4] dt-bindings: interrupt-controller: New binding for Meson-A1 SoCs 2019-12-06 12:17 [PATCH 0/4] irqchip/meson-gpio: Add support for Meson-A1 SoC Qianggui Song @ 2019-12-06 12:17 ` Qianggui Song 2019-12-06 12:17 ` [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs Qianggui Song ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Qianggui Song @ 2019-12-06 12:17 UTC (permalink / raw) To: Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Qianggui Song, devicetree, Hanjie Lin, Jianxin Pan, Neil Armstrong, Kevin Hilman, linux-kernel, Rob Herring, linux-arm-kernel, linux-amlogic, Mark Rutland, Xingyu Chen, Jerome Brunet Update dt-binding document for GPIO interrupt controller of Meson-A1 SoCs Signed-off-by: Qianggui Song <qianggui.song@amlogic.com> --- .../bindings/interrupt-controller/amlogic,meson-gpio-intc.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt index 684bb1cd75ec..23b18b92c558 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt +++ b/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt @@ -17,6 +17,7 @@ Required properties: "amlogic,meson-axg-gpio-intc" for AXG SoCs (A113D, A113X) "amlogic,meson-g12a-gpio-intc" for G12A SoCs (S905D2, S905X2, S905Y2) "amlogic,meson-sm1-gpio-intc" for SM1 SoCs (S905D3, S905X3, S905Y3) + "amlogic,meson-a1-gpio-intc" for A1 SoCs (A113L) - reg : Specifies base physical address and size of the registers. - interrupt-controller : Identifies the node as an interrupt controller. - #interrupt-cells : Specifies the number of cells needed to encode an -- 2.24.0 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs 2019-12-06 12:17 [PATCH 0/4] irqchip/meson-gpio: Add support for Meson-A1 SoC Qianggui Song 2019-12-06 12:17 ` [PATCH 1/4] dt-bindings: interrupt-controller: New binding for Meson-A1 SoCs Qianggui Song @ 2019-12-06 12:17 ` Qianggui Song 2019-12-06 13:13 ` Marc Zyngier 2019-12-06 12:17 ` [PATCH 3/4] irqchip/meson-gpio: Add support for meson a1 SoCs Qianggui Song 2019-12-06 12:17 ` [PATCH 4/4] arm64: dts: meson: a1: add gpio interrupt controller support Qianggui Song 3 siblings, 1 reply; 9+ messages in thread From: Qianggui Song @ 2019-12-06 12:17 UTC (permalink / raw) To: Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Qianggui Song, Hanjie Lin, Jianxin Pan, Neil Armstrong, Kevin Hilman, linux-kernel, linux-arm-kernel, linux-amlogic, Xingyu Chen, Jerome Brunet Since Meson-A1 Socs register layout of gpio interrupt controller have difference with previous chips, registers to decide irq line and offset of trigger method are all changed, the current driver should be modified. Signed-off-by: Qianggui Song <qianggui.song@amlogic.com> --- drivers/irqchip/irq-meson-gpio.c | 79 ++++++++++++++++++++++++-------- 1 file changed, 60 insertions(+), 19 deletions(-) diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c index 829084b568fa..1824ffc30de2 100644 --- a/drivers/irqchip/irq-meson-gpio.c +++ b/drivers/irqchip/irq-meson-gpio.c @@ -30,44 +30,74 @@ * stuck at 0. Bits 8 to 15 are responsive and have the expected * effect. */ -#define REG_EDGE_POL_EDGE(x) BIT(x) -#define REG_EDGE_POL_LOW(x) BIT(16 + (x)) -#define REG_BOTH_EDGE(x) BIT(8 + (x)) -#define REG_EDGE_POL_MASK(x) ( \ - REG_EDGE_POL_EDGE(x) | \ - REG_EDGE_POL_LOW(x) | \ - REG_BOTH_EDGE(x)) +#define REG_EDGE_POL_EDGE(params, x) BIT((params)->edge_single_offset + (x)) +#define REG_EDGE_POL_LOW(params, x) BIT((params)->pol_low_offset + (x)) +#define REG_BOTH_EDGE(params, x) BIT((params)->edge_both_offset + (x)) +#define REG_EDGE_POL_MASK(params, x) ( \ + REG_EDGE_POL_EDGE(params, x) | \ + REG_EDGE_POL_LOW(params, x) | \ + REG_BOTH_EDGE(params, x)) #define REG_PIN_SEL_SHIFT(x) (((x) % 4) * 8) #define REG_FILTER_SEL_SHIFT(x) ((x) * 4) +#define INIT_MESON8_COMMON_DATA \ + .edge_single_offset = 0, \ + .pol_low_offset = 16, \ + .pin_sel_mask = 0xff, \ + .ops = { \ + .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, \ + }, + +struct meson_gpio_irq_controller; +static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl, + unsigned int channel, unsigned long hwirq); +struct irq_ctl_ops { + void (*gpio_irq_sel_pin)(struct meson_gpio_irq_controller *ctl, + unsigned int channel, + unsigned long hwirq); + void (*gpio_irq_init)(struct meson_gpio_irq_controller *ctl); +}; + struct meson_gpio_irq_params { unsigned int nr_hwirq; bool support_edge_both; + unsigned int edge_both_offset; + unsigned int edge_single_offset; + unsigned int pol_low_offset; + unsigned int pin_sel_mask; + struct irq_ctl_ops ops; }; static const struct meson_gpio_irq_params meson8_params = { .nr_hwirq = 134, + INIT_MESON8_COMMON_DATA }; static const struct meson_gpio_irq_params meson8b_params = { .nr_hwirq = 119, + INIT_MESON8_COMMON_DATA }; static const struct meson_gpio_irq_params gxbb_params = { .nr_hwirq = 133, + INIT_MESON8_COMMON_DATA }; static const struct meson_gpio_irq_params gxl_params = { .nr_hwirq = 110, + INIT_MESON8_COMMON_DATA }; static const struct meson_gpio_irq_params axg_params = { .nr_hwirq = 100, + INIT_MESON8_COMMON_DATA }; static const struct meson_gpio_irq_params sm1_params = { .nr_hwirq = 100, .support_edge_both = true, + .edge_both_offset = 8, + INIT_MESON8_COMMON_DATA }; static const struct of_device_id meson_irq_gpio_matches[] = { @@ -100,9 +130,18 @@ static void meson_gpio_irq_update_bits(struct meson_gpio_irq_controller *ctl, writel_relaxed(tmp, ctl->base + reg); } -static unsigned int meson_gpio_irq_channel_to_reg(unsigned int channel) +static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl, + unsigned int channel, unsigned long hwirq) { - return (channel < 4) ? REG_PIN_03_SEL : REG_PIN_47_SEL; + unsigned int reg_offset; + unsigned int bit_offset; + + reg_offset = (channel < 4) ? REG_PIN_03_SEL : REG_PIN_47_SEL; + bit_offset = REG_PIN_SEL_SHIFT(channel); + + meson_gpio_irq_update_bits(ctl, reg_offset, + ctl->params->pin_sel_mask << bit_offset, + hwirq << bit_offset); } static int @@ -110,7 +149,7 @@ meson_gpio_irq_request_channel(struct meson_gpio_irq_controller *ctl, unsigned long hwirq, u32 **channel_hwirq) { - unsigned int reg, idx; + unsigned int idx; spin_lock(&ctl->lock); @@ -129,10 +168,7 @@ meson_gpio_irq_request_channel(struct meson_gpio_irq_controller *ctl, * Setup the mux of the channel to route the signal of the pad * to the appropriate input of the GIC */ - reg = meson_gpio_irq_channel_to_reg(idx); - meson_gpio_irq_update_bits(ctl, reg, - 0xff << REG_PIN_SEL_SHIFT(idx), - hwirq << REG_PIN_SEL_SHIFT(idx)); + ctl->params->ops.gpio_irq_sel_pin(ctl, idx, hwirq); /* * Get the hwirq number assigned to this channel through @@ -173,7 +209,9 @@ static int meson_gpio_irq_type_setup(struct meson_gpio_irq_controller *ctl, { u32 val = 0; unsigned int idx; + const struct meson_gpio_irq_params *params; + params = ctl->params; idx = meson_gpio_irq_get_channel_idx(ctl, channel_hwirq); /* @@ -190,22 +228,22 @@ static int meson_gpio_irq_type_setup(struct meson_gpio_irq_controller *ctl, * precedence over the other edge/polarity settings */ if (type == IRQ_TYPE_EDGE_BOTH) { - if (!ctl->params->support_edge_both) + if (!params->support_edge_both) return -EINVAL; - val |= REG_BOTH_EDGE(idx); + val |= REG_BOTH_EDGE(params, idx); } else { if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING)) - val |= REG_EDGE_POL_EDGE(idx); + val |= REG_EDGE_POL_EDGE(params, idx); if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) - val |= REG_EDGE_POL_LOW(idx); + val |= REG_EDGE_POL_LOW(params, idx); } spin_lock(&ctl->lock); meson_gpio_irq_update_bits(ctl, REG_EDGE_POL, - REG_EDGE_POL_MASK(idx), val); + REG_EDGE_POL_MASK(params, idx), val); spin_unlock(&ctl->lock); @@ -371,6 +409,9 @@ static int __init meson_gpio_irq_parse_dt(struct device_node *node, return ret; } + if (ctl->params->ops.gpio_irq_init) + ctl->params->ops.gpio_irq_init(ctl); + return 0; } -- 2.24.0 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs 2019-12-06 12:17 ` [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs Qianggui Song @ 2019-12-06 13:13 ` Marc Zyngier 2019-12-10 2:08 ` Qianggui Song 0 siblings, 1 reply; 9+ messages in thread From: Marc Zyngier @ 2019-12-06 13:13 UTC (permalink / raw) To: Qianggui Song Cc: Hanjie Lin, Jason Cooper, Jianxin Pan, Neil Armstrong, Kevin Hilman, linux-kernel, linux-arm-kernel, linux-amlogic, Thomas Gleixner, Xingyu Chen, Jerome Brunet On 2019-12-06 12:17, Qianggui Song wrote: > Since Meson-A1 Socs register layout of gpio interrupt controller have > difference with previous chips, registers to decide irq line and > offset > of trigger method are all changed, the current driver should be > modified. > > Signed-off-by: Qianggui Song <qianggui.song@amlogic.com> > --- > drivers/irqchip/irq-meson-gpio.c | 79 > ++++++++++++++++++++++++-------- > 1 file changed, 60 insertions(+), 19 deletions(-) > > diff --git a/drivers/irqchip/irq-meson-gpio.c > b/drivers/irqchip/irq-meson-gpio.c > index 829084b568fa..1824ffc30de2 100644 > --- a/drivers/irqchip/irq-meson-gpio.c > +++ b/drivers/irqchip/irq-meson-gpio.c > @@ -30,44 +30,74 @@ > * stuck at 0. Bits 8 to 15 are responsive and have the expected > * effect. > */ > -#define REG_EDGE_POL_EDGE(x) BIT(x) > -#define REG_EDGE_POL_LOW(x) BIT(16 + (x)) > -#define REG_BOTH_EDGE(x) BIT(8 + (x)) > -#define REG_EDGE_POL_MASK(x) ( \ > - REG_EDGE_POL_EDGE(x) | \ > - REG_EDGE_POL_LOW(x) | \ > - REG_BOTH_EDGE(x)) > +#define REG_EDGE_POL_EDGE(params, > x) BIT((params)->edge_single_offset + (x)) > +#define REG_EDGE_POL_LOW(params, x) BIT((params)->pol_low_offset + > (x)) > +#define REG_BOTH_EDGE(params, x) BIT((params)->edge_both_offset + > (x)) > +#define REG_EDGE_POL_MASK(params, x) ( \ > + REG_EDGE_POL_EDGE(params, x) | \ > + REG_EDGE_POL_LOW(params, x) | \ > + REG_BOTH_EDGE(params, x)) > #define REG_PIN_SEL_SHIFT(x) (((x) % 4) * 8) > #define REG_FILTER_SEL_SHIFT(x) ((x) * 4) > > +#define INIT_MESON8_COMMON_DATA \ > + .edge_single_offset = 0, \ > + .pol_low_offset = 16, \ > + .pin_sel_mask = 0xff, \ > + .ops = { \ > + .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, \ > + }, Please place the #defines that operate on the various data structures *after* the definition of the structures. It would greatly help reading the changes. > + > +struct meson_gpio_irq_controller; > +static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller > *ctl, > + unsigned int channel, unsigned long hwirq); > +struct irq_ctl_ops { > + void (*gpio_irq_sel_pin)(struct meson_gpio_irq_controller *ctl, > + unsigned int channel, > + unsigned long hwirq); > + void (*gpio_irq_init)(struct meson_gpio_irq_controller *ctl); > +}; > + > struct meson_gpio_irq_params { > unsigned int nr_hwirq; > bool support_edge_both; > + unsigned int edge_both_offset; > + unsigned int edge_single_offset; > + unsigned int pol_low_offset; > + unsigned int pin_sel_mask; > + struct irq_ctl_ops ops; > }; > > static const struct meson_gpio_irq_params meson8_params = { > .nr_hwirq = 134, > + INIT_MESON8_COMMON_DATA > }; > > static const struct meson_gpio_irq_params meson8b_params = { > .nr_hwirq = 119, > + INIT_MESON8_COMMON_DATA > }; > > static const struct meson_gpio_irq_params gxbb_params = { > .nr_hwirq = 133, > + INIT_MESON8_COMMON_DATA > }; > > static const struct meson_gpio_irq_params gxl_params = { > .nr_hwirq = 110, > + INIT_MESON8_COMMON_DATA > }; > > static const struct meson_gpio_irq_params axg_params = { > .nr_hwirq = 100, > + INIT_MESON8_COMMON_DATA > }; > > static const struct meson_gpio_irq_params sm1_params = { > .nr_hwirq = 100, > .support_edge_both = true, > + .edge_both_offset = 8, > + INIT_MESON8_COMMON_DATA > }; OK, this isn't great. The least you could do is to make your initializer parametric, so that it takes the nr_hwirq as a parameter. Then, any additional member that overrides common behaviour should come after the main initializer. Also, do you need 'support_edge_both'? Isn't a non-zero 'edge_both_offset' enough to detect the feature? > > static const struct of_device_id meson_irq_gpio_matches[] = { > @@ -100,9 +130,18 @@ static void meson_gpio_irq_update_bits(struct > meson_gpio_irq_controller *ctl, > writel_relaxed(tmp, ctl->base + reg); > } > > -static unsigned int meson_gpio_irq_channel_to_reg(unsigned int > channel) > +static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller > *ctl, > + unsigned int channel, unsigned long hwirq) > { > - return (channel < 4) ? REG_PIN_03_SEL : REG_PIN_47_SEL; > + unsigned int reg_offset; > + unsigned int bit_offset; > + > + reg_offset = (channel < 4) ? REG_PIN_03_SEL : REG_PIN_47_SEL; > + bit_offset = REG_PIN_SEL_SHIFT(channel); > + > + meson_gpio_irq_update_bits(ctl, reg_offset, > + ctl->params->pin_sel_mask << bit_offset, > + hwirq << bit_offset); > } > > static int > @@ -110,7 +149,7 @@ meson_gpio_irq_request_channel(struct > meson_gpio_irq_controller *ctl, > unsigned long hwirq, > u32 **channel_hwirq) > { > - unsigned int reg, idx; > + unsigned int idx; > > spin_lock(&ctl->lock); > > @@ -129,10 +168,7 @@ meson_gpio_irq_request_channel(struct > meson_gpio_irq_controller *ctl, > * Setup the mux of the channel to route the signal of the pad > * to the appropriate input of the GIC > */ > - reg = meson_gpio_irq_channel_to_reg(idx); > - meson_gpio_irq_update_bits(ctl, reg, > - 0xff << REG_PIN_SEL_SHIFT(idx), > - hwirq << REG_PIN_SEL_SHIFT(idx)); > + ctl->params->ops.gpio_irq_sel_pin(ctl, idx, hwirq); > > /* > * Get the hwirq number assigned to this channel through > @@ -173,7 +209,9 @@ static int meson_gpio_irq_type_setup(struct > meson_gpio_irq_controller *ctl, > { > u32 val = 0; > unsigned int idx; > + const struct meson_gpio_irq_params *params; > > + params = ctl->params; > idx = meson_gpio_irq_get_channel_idx(ctl, channel_hwirq); > > /* > @@ -190,22 +228,22 @@ static int meson_gpio_irq_type_setup(struct > meson_gpio_irq_controller *ctl, > * precedence over the other edge/polarity settings > */ > if (type == IRQ_TYPE_EDGE_BOTH) { > - if (!ctl->params->support_edge_both) > + if (!params->support_edge_both) > return -EINVAL; > > - val |= REG_BOTH_EDGE(idx); > + val |= REG_BOTH_EDGE(params, idx); > } else { > if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING)) > - val |= REG_EDGE_POL_EDGE(idx); > + val |= REG_EDGE_POL_EDGE(params, idx); > > if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) > - val |= REG_EDGE_POL_LOW(idx); > + val |= REG_EDGE_POL_LOW(params, idx); > } > > spin_lock(&ctl->lock); > > meson_gpio_irq_update_bits(ctl, REG_EDGE_POL, > - REG_EDGE_POL_MASK(idx), val); > + REG_EDGE_POL_MASK(params, idx), val); > > spin_unlock(&ctl->lock); > > @@ -371,6 +409,9 @@ static int __init meson_gpio_irq_parse_dt(struct > device_node *node, > return ret; > } > > + if (ctl->params->ops.gpio_irq_init) > + ctl->params->ops.gpio_irq_init(ctl); It would make sense to provide a dummy init() method, since you have all the infrastructure already. > + > return 0; > } Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs 2019-12-06 13:13 ` Marc Zyngier @ 2019-12-10 2:08 ` Qianggui Song 2019-12-11 17:26 ` Marc Zyngier 0 siblings, 1 reply; 9+ messages in thread From: Qianggui Song @ 2019-12-10 2:08 UTC (permalink / raw) To: Marc Zyngier Cc: Hanjie Lin, Jason Cooper, Jianxin Pan, Neil Armstrong, Kevin Hilman, linux-kernel, linux-arm-kernel, linux-amlogic, Thomas Gleixner, Xingyu Chen, Jerome Brunet Hi, Marc Thank you for your review On 2019/12/6 21:13, Marc Zyngier wrote: > On 2019-12-06 12:17, Qianggui Song wrote: >> Since Meson-A1 Socs register layout of gpio interrupt controller have >> difference with previous chips, registers to decide irq line and >> offset >> of trigger method are all changed, the current driver should be >> modified. >> >> Signed-off-by: Qianggui Song <qianggui.song@amlogic.com> >> --- >> drivers/irqchip/irq-meson-gpio.c | 79 >> ++++++++++++++++++++++++-------- >> 1 file changed, 60 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/irqchip/irq-meson-gpio.c >> b/drivers/irqchip/irq-meson-gpio.c >> index 829084b568fa..1824ffc30de2 100644 >> --- a/drivers/irqchip/irq-meson-gpio.c >> +++ b/drivers/irqchip/irq-meson-gpio.c >> @@ -30,44 +30,74 @@ >> * stuck at 0. Bits 8 to 15 are responsive and have the expected >> * effect. >> */ >> -#define REG_EDGE_POL_EDGE(x) BIT(x) >> -#define REG_EDGE_POL_LOW(x) BIT(16 + (x)) >> -#define REG_BOTH_EDGE(x) BIT(8 + (x)) >> -#define REG_EDGE_POL_MASK(x) ( \ >> - REG_EDGE_POL_EDGE(x) | \ >> - REG_EDGE_POL_LOW(x) | \ >> - REG_BOTH_EDGE(x)) >> +#define REG_EDGE_POL_EDGE(params, >> x) BIT((params)->edge_single_offset + (x)) >> +#define REG_EDGE_POL_LOW(params, x) BIT((params)->pol_low_offset + >> (x)) >> +#define REG_BOTH_EDGE(params, x) BIT((params)->edge_both_offset + >> (x)) >> +#define REG_EDGE_POL_MASK(params, x) ( \ >> + REG_EDGE_POL_EDGE(params, x) | \ >> + REG_EDGE_POL_LOW(params, x) | \ >> + REG_BOTH_EDGE(params, x)) >> #define REG_PIN_SEL_SHIFT(x) (((x) % 4) * 8) >> #define REG_FILTER_SEL_SHIFT(x) ((x) * 4) >> >> +#define INIT_MESON8_COMMON_DATA \ >> + .edge_single_offset = 0, \ >> + .pol_low_offset = 16, \ >> + .pin_sel_mask = 0xff, \ >> + .ops = { \ >> + .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, \ >> + }, > > Please place the #defines that operate on the various data structures > *after* the definition of the structures. It would greatly help > reading the changes. > OK, will place it below the definition of struct meson_gpio_irq_params in the next patch. >> + >> +struct meson_gpio_irq_controller; >> +static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller >> *ctl, >> + unsigned int channel, unsigned long hwirq); >> +struct irq_ctl_ops { >> + void (*gpio_irq_sel_pin)(struct meson_gpio_irq_controller *ctl, >> + unsigned int channel, >> + unsigned long hwirq); >> + void (*gpio_irq_init)(struct meson_gpio_irq_controller *ctl); >> +}; >> + >> struct meson_gpio_irq_params { >> unsigned int nr_hwirq; >> bool support_edge_both; >> + unsigned int edge_both_offset; >> + unsigned int edge_single_offset; >> + unsigned int pol_low_offset; >> + unsigned int pin_sel_mask; >> + struct irq_ctl_ops ops; >> }; >> >> static const struct meson_gpio_irq_params meson8_params = { >> .nr_hwirq = 134, >> + INIT_MESON8_COMMON_DATA >> }; >> >> static const struct meson_gpio_irq_params meson8b_params = { >> .nr_hwirq = 119, >> + INIT_MESON8_COMMON_DATA >> }; >> >> static const struct meson_gpio_irq_params gxbb_params = { >> .nr_hwirq = 133, >> + INIT_MESON8_COMMON_DATA >> }; >> >> static const struct meson_gpio_irq_params gxl_params = { >> .nr_hwirq = 110, >> + INIT_MESON8_COMMON_DATA >> }; >> >> static const struct meson_gpio_irq_params axg_params = { >> .nr_hwirq = 100, >> + INIT_MESON8_COMMON_DATA >> }; >> >> static const struct meson_gpio_irq_params sm1_params = { >> .nr_hwirq = 100, >> .support_edge_both = true, >> + .edge_both_offset = 8, >> + INIT_MESON8_COMMON_DATA >> }; > > OK, this isn't great. The least you could do is to make > your initializer parametric, so that it takes the nr_hwirq as > a parameter. > > Then, any additional member that overrides common behaviour > should come after the main initializer. > > Also, do you need 'support_edge_both'? Isn't a non-zero > 'edge_both_offset' enough to detect the feature? > Sorry, but I am not very clear that "make your initializer parametric, so that it takes the nr_hwirq as a parameter". Is that initializer(initial function in .ops ? ) as a parameter of struct meson_gpio_irq_params ? If nr_hwirq as a parameter of init function of .ops then will make lot of init function for each platform. How about move .ops from macro like below: #define INIT_MESON8_COMMON_DATA \ .edge_single_offset = 0, \ .pol_low_offset = 16, \ .pin_sel_mask = 0xff, static const struct meson_gpio_irq_params sm1_params = { .nr_hwirq = 100,//main initializer .ops = { .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, /*in below to assign support_edge_both * edge_both_offset * call after main initializer to additional * member */ .gpio_irq_init = meson_sm1_irq_init, }, INIT_MESON8_COMMON_DATA// m8 to sm1 are the same. }; About support_edge_both support_edge_both & edge_both_offset are used for sm1 or later chip, but the value are different with A1 which this patch set support.For SM1 is 8, but A1 is 16, so I am not one hundred percent sure that later chip design will change to edge_both_offet to zero to set both edge trigger. Let' s see edge_single_offset, it is set to 0 in A1 from 16 which is previous chip use.I think support_edge_bot should be kept. >> >> static const struct of_device_id meson_irq_gpio_matches[] = { >> @@ -100,9 +130,18 @@ static void meson_gpio_irq_update_bits(struct >> meson_gpio_irq_controller *ctl, >> writel_relaxed(tmp, ctl->base + reg); >> } >> >> -static unsigned int meson_gpio_irq_channel_to_reg(unsigned int >> channel) >> +static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller >> *ctl, >> + unsigned int channel, unsigned long hwirq) >> { >> - return (channel < 4) ? REG_PIN_03_SEL : REG_PIN_47_SEL; >> + unsigned int reg_offset; >> + unsigned int bit_offset; >> + >> + reg_offset = (channel < 4) ? REG_PIN_03_SEL : REG_PIN_47_SEL; >> + bit_offset = REG_PIN_SEL_SHIFT(channel); >> + >> + meson_gpio_irq_update_bits(ctl, reg_offset, >> + ctl->params->pin_sel_mask << bit_offset, >> + hwirq << bit_offset); >> } >> >> static int >> @@ -110,7 +149,7 @@ meson_gpio_irq_request_channel(struct >> meson_gpio_irq_controller *ctl, >> unsigned long hwirq, >> u32 **channel_hwirq) >> { >> - unsigned int reg, idx; >> + unsigned int idx; >> >> spin_lock(&ctl->lock); >> >> @@ -129,10 +168,7 @@ meson_gpio_irq_request_channel(struct >> meson_gpio_irq_controller *ctl, >> * Setup the mux of the channel to route the signal of the pad >> * to the appropriate input of the GIC >> */ >> - reg = meson_gpio_irq_channel_to_reg(idx); >> - meson_gpio_irq_update_bits(ctl, reg, >> - 0xff << REG_PIN_SEL_SHIFT(idx), >> - hwirq << REG_PIN_SEL_SHIFT(idx)); >> + ctl->params->ops.gpio_irq_sel_pin(ctl, idx, hwirq); >> >> /* >> * Get the hwirq number assigned to this channel through >> @@ -173,7 +209,9 @@ static int meson_gpio_irq_type_setup(struct >> meson_gpio_irq_controller *ctl, >> { >> u32 val = 0; >> unsigned int idx; >> + const struct meson_gpio_irq_params *params; >> >> + params = ctl->params; >> idx = meson_gpio_irq_get_channel_idx(ctl, channel_hwirq); >> >> /* >> @@ -190,22 +228,22 @@ static int meson_gpio_irq_type_setup(struct >> meson_gpio_irq_controller *ctl, >> * precedence over the other edge/polarity settings >> */ >> if (type == IRQ_TYPE_EDGE_BOTH) { >> - if (!ctl->params->support_edge_both) >> + if (!params->support_edge_both) >> return -EINVAL; >> >> - val |= REG_BOTH_EDGE(idx); >> + val |= REG_BOTH_EDGE(params, idx); >> } else { >> if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING)) >> - val |= REG_EDGE_POL_EDGE(idx); >> + val |= REG_EDGE_POL_EDGE(params, idx); >> >> if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) >> - val |= REG_EDGE_POL_LOW(idx); >> + val |= REG_EDGE_POL_LOW(params, idx); >> } >> >> spin_lock(&ctl->lock); >> >> meson_gpio_irq_update_bits(ctl, REG_EDGE_POL, >> - REG_EDGE_POL_MASK(idx), val); >> + REG_EDGE_POL_MASK(params, idx), val); >> >> spin_unlock(&ctl->lock); >> >> @@ -371,6 +409,9 @@ static int __init meson_gpio_irq_parse_dt(struct >> device_node *node, >> return ret; >> } >> >> + if (ctl->params->ops.gpio_irq_init) >> + ctl->params->ops.gpio_irq_init(ctl); > > It would make sense to provide a dummy init() method, since > you have all the infrastructure already. > Okay >> + >> return 0; >> } > > Thanks, > > M. > _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs 2019-12-10 2:08 ` Qianggui Song @ 2019-12-11 17:26 ` Marc Zyngier 2019-12-12 11:13 ` Qianggui Song 0 siblings, 1 reply; 9+ messages in thread From: Marc Zyngier @ 2019-12-11 17:26 UTC (permalink / raw) To: Qianggui Song Cc: Hanjie Lin, Jason Cooper, Jianxin Pan, Neil Armstrong, Kevin Hilman, linux-kernel, linux-arm-kernel, linux-amlogic, Thomas Gleixner, Xingyu Chen, Jerome Brunet On 2019-12-10 02:08, Qianggui Song wrote: > Hi, Marc > Thank you for your review > > On 2019/12/6 21:13, Marc Zyngier wrote: >> On 2019-12-06 12:17, Qianggui Song wrote: >>> Since Meson-A1 Socs register layout of gpio interrupt controller >>> have >>> difference with previous chips, registers to decide irq line and >>> offset >>> of trigger method are all changed, the current driver should be >>> modified. >>> >>> Signed-off-by: Qianggui Song <qianggui.song@amlogic.com> >>> --- >>> drivers/irqchip/irq-meson-gpio.c | 79 >>> ++++++++++++++++++++++++-------- >>> 1 file changed, 60 insertions(+), 19 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-meson-gpio.c >>> b/drivers/irqchip/irq-meson-gpio.c >>> index 829084b568fa..1824ffc30de2 100644 >>> --- a/drivers/irqchip/irq-meson-gpio.c >>> +++ b/drivers/irqchip/irq-meson-gpio.c >>> @@ -30,44 +30,74 @@ >>> * stuck at 0. Bits 8 to 15 are responsive and have the expected >>> * effect. >>> */ >>> -#define REG_EDGE_POL_EDGE(x) BIT(x) >>> -#define REG_EDGE_POL_LOW(x) BIT(16 + (x)) >>> -#define REG_BOTH_EDGE(x) BIT(8 + (x)) >>> -#define REG_EDGE_POL_MASK(x) ( \ >>> - REG_EDGE_POL_EDGE(x) | \ >>> - REG_EDGE_POL_LOW(x) | \ >>> - REG_BOTH_EDGE(x)) >>> +#define REG_EDGE_POL_EDGE(params, >>> x) BIT((params)->edge_single_offset + (x)) >>> +#define REG_EDGE_POL_LOW(params, x) BIT((params)->pol_low_offset + >>> (x)) >>> +#define REG_BOTH_EDGE(params, x) BIT((params)->edge_both_offset + >>> (x)) >>> +#define REG_EDGE_POL_MASK(params, x) ( \ >>> + REG_EDGE_POL_EDGE(params, x) | \ >>> + REG_EDGE_POL_LOW(params, x) | \ >>> + REG_BOTH_EDGE(params, x)) >>> #define REG_PIN_SEL_SHIFT(x) (((x) % 4) * 8) >>> #define REG_FILTER_SEL_SHIFT(x) ((x) * 4) >>> >>> +#define INIT_MESON8_COMMON_DATA \ >>> + .edge_single_offset = 0, \ >>> + .pol_low_offset = 16, \ >>> + .pin_sel_mask = 0xff, \ >>> + .ops = { \ >>> + .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, \ >>> + }, >> >> Please place the #defines that operate on the various data >> structures >> *after* the definition of the structures. It would greatly help >> reading the changes. >> > OK, will place it below the definition of struct > meson_gpio_irq_params > in the next patch. >>> + >>> +struct meson_gpio_irq_controller; >>> +static void meson8_gpio_irq_sel_pin(struct >>> meson_gpio_irq_controller >>> *ctl, >>> + unsigned int channel, unsigned long hwirq); >>> +struct irq_ctl_ops { >>> + void (*gpio_irq_sel_pin)(struct meson_gpio_irq_controller *ctl, >>> + unsigned int channel, >>> + unsigned long hwirq); >>> + void (*gpio_irq_init)(struct meson_gpio_irq_controller *ctl); >>> +}; >>> + >>> struct meson_gpio_irq_params { >>> unsigned int nr_hwirq; >>> bool support_edge_both; >>> + unsigned int edge_both_offset; >>> + unsigned int edge_single_offset; >>> + unsigned int pol_low_offset; >>> + unsigned int pin_sel_mask; >>> + struct irq_ctl_ops ops; >>> }; >>> >>> static const struct meson_gpio_irq_params meson8_params = { >>> .nr_hwirq = 134, >>> + INIT_MESON8_COMMON_DATA >>> }; >>> >>> static const struct meson_gpio_irq_params meson8b_params = { >>> .nr_hwirq = 119, >>> + INIT_MESON8_COMMON_DATA >>> }; >>> >>> static const struct meson_gpio_irq_params gxbb_params = { >>> .nr_hwirq = 133, >>> + INIT_MESON8_COMMON_DATA >>> }; >>> >>> static const struct meson_gpio_irq_params gxl_params = { >>> .nr_hwirq = 110, >>> + INIT_MESON8_COMMON_DATA >>> }; >>> >>> static const struct meson_gpio_irq_params axg_params = { >>> .nr_hwirq = 100, >>> + INIT_MESON8_COMMON_DATA >>> }; >>> >>> static const struct meson_gpio_irq_params sm1_params = { >>> .nr_hwirq = 100, >>> .support_edge_both = true, >>> + .edge_both_offset = 8, >>> + INIT_MESON8_COMMON_DATA >>> }; >> >> OK, this isn't great. The least you could do is to make >> your initializer parametric, so that it takes the nr_hwirq as >> a parameter. >> >> Then, any additional member that overrides common behaviour >> should come after the main initializer. >> >> Also, do you need 'support_edge_both'? Isn't a non-zero >> 'edge_both_offset' enough to detect the feature? >> > > Sorry, but I am not very clear that "make your initializer > parametric, > so that it takes the nr_hwirq as a parameter". Is that > initializer(initial function in .ops ? ) as a parameter of struct > meson_gpio_irq_params ? If nr_hwirq as a parameter of init function > of > .ops then will make lot of init function for each platform. > > How about move .ops from macro like below: > #define INIT_MESON8_COMMON_DATA \ > .edge_single_offset = 0, \ > .pol_low_offset = 16, \ > .pin_sel_mask = 0xff, > > static const struct meson_gpio_irq_params sm1_params = { > .nr_hwirq = 100,//main initializer > .ops = { > .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, > /*in below to assign support_edge_both > * edge_both_offset > * call after main initializer to additional > * member > */ > .gpio_irq_init = meson_sm1_irq_init, > }, > INIT_MESON8_COMMON_DATA// m8 to sm1 are the same. > }; No, what I'm suggesting is something like this: diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c index 8478100706a6..27a3207a944d 100644 --- a/drivers/irqchip/irq-meson-gpio.c +++ b/drivers/irqchip/irq-meson-gpio.c @@ -43,24 +43,27 @@ /* Below is used for Meson-A1 series like chips*/ #define REG_PIN_A1_SEL 0x04 -#define INIT_MESON8_COMMON_DATA \ - .edge_single_offset = 0, \ - .pol_low_offset = 16, \ - .pin_sel_mask = 0xff, \ - .ops = { \ - .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, \ - }, - -#define INIT_MESON_A1_COMMON_DATA \ - .support_edge_both = true, \ - .edge_both_offset = 16, \ - .edge_single_offset = 8, \ - .pol_low_offset = 0, \ - .pin_sel_mask = 0x7f, \ - .ops = { \ - .gpio_irq_sel_pin = meson_a1_gpio_irq_sel_pin, \ - .gpio_irq_init = meson_a1_gpio_irq_init, \ - }, +#define INIT_MESON_COMMON(irqs, init, sel) \ + .nr_hwirq = irqs, \ + .ops = { \ + .gpio_irq_sel_pin = sel, \ + .gpio_irq_init = init, \ + } + +#define INIT_MESON8_COMMON_DATA(irqs) \ + INIT_MESON_COMMON(irqs, NULL, \ + meson8_gpio_irq_sel_pin), \ + .pol_low_offset = 16, \ + .pin_sel_mask = 0xff, + +#define INIT_MESON_A1_COMMON_DATA(irqs) \ + INIT_MESON_COMMON(irqs, meson_a1_gpio_irq_init, \ + meson_a1_gpio_irq_sel_pin), \ + .support_edge_both = true, \ + .edge_both_offset = 16, \ + .edge_single_offset = 8, \ + .pol_low_offset = 0, \ + .pin_sel_mask = 0x7f, struct meson_gpio_irq_controller; static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl, @@ -89,40 +92,33 @@ struct meson_gpio_irq_params { }; static const struct meson_gpio_irq_params meson8_params = { - .nr_hwirq = 134, - INIT_MESON8_COMMON_DATA + INIT_MESON8_COMMON_DATA(134), }; static const struct meson_gpio_irq_params meson8b_params = { - .nr_hwirq = 119, - INIT_MESON8_COMMON_DATA + INIT_MESON8_COMMON_DATA(119), }; static const struct meson_gpio_irq_params gxbb_params = { - .nr_hwirq = 133, - INIT_MESON8_COMMON_DATA + INIT_MESON8_COMMON_DATA(133), }; static const struct meson_gpio_irq_params gxl_params = { - .nr_hwirq = 110, - INIT_MESON8_COMMON_DATA + INIT_MESON8_COMMON_DATA(110), }; static const struct meson_gpio_irq_params axg_params = { - .nr_hwirq = 100, - INIT_MESON8_COMMON_DATA + INIT_MESON8_COMMON_DATA(100), }; static const struct meson_gpio_irq_params sm1_params = { - .nr_hwirq = 100, + INIT_MESON8_COMMON_DATA(100), .support_edge_both = true, .edge_both_offset = 8, - INIT_MESON8_COMMON_DATA }; static const struct meson_gpio_irq_params a1_params = { - .nr_hwirq = 62, - INIT_MESON_A1_COMMON_DATA + INIT_MESON_A1_COMMON_DATA(62), }; static const struct of_device_id meson_irq_gpio_matches[] = { Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs 2019-12-11 17:26 ` Marc Zyngier @ 2019-12-12 11:13 ` Qianggui Song 0 siblings, 0 replies; 9+ messages in thread From: Qianggui Song @ 2019-12-12 11:13 UTC (permalink / raw) To: Marc Zyngier Cc: Hanjie Lin, Jason Cooper, Jianxin Pan, Neil Armstrong, Kevin Hilman, linux-kernel, linux-arm-kernel, linux-amlogic, Thomas Gleixner, Xingyu Chen, Jerome Brunet On 2019/12/12 1:26, Marc Zyngier wrote: > On 2019-12-10 02:08, Qianggui Song wrote: >> Hi, Marc >> Thank you for your review >> >> On 2019/12/6 21:13, Marc Zyngier wrote: >>> On 2019-12-06 12:17, Qianggui Song wrote: >>>> Since Meson-A1 Socs register layout of gpio interrupt controller >>>> have >>>> difference with previous chips, registers to decide irq line and >>>> offset >>>> of trigger method are all changed, the current driver should be >>>> modified. >>>> >>>> Signed-off-by: Qianggui Song <qianggui.song@amlogic.com> >>>> --- >>>> drivers/irqchip/irq-meson-gpio.c | 79 >>>> ++++++++++++++++++++++++-------- >>>> 1 file changed, 60 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/drivers/irqchip/irq-meson-gpio.c >>>> b/drivers/irqchip/irq-meson-gpio.c >>>> index 829084b568fa..1824ffc30de2 100644 >>>> --- a/drivers/irqchip/irq-meson-gpio.c >>>> +++ b/drivers/irqchip/irq-meson-gpio.c >>>> @@ -30,44 +30,74 @@ >>>> * stuck at 0. Bits 8 to 15 are responsive and have the expected >>>> * effect. >>>> */ >>>> -#define REG_EDGE_POL_EDGE(x) BIT(x) >>>> -#define REG_EDGE_POL_LOW(x) BIT(16 + (x)) >>>> -#define REG_BOTH_EDGE(x) BIT(8 + (x)) >>>> -#define REG_EDGE_POL_MASK(x) ( \ >>>> - REG_EDGE_POL_EDGE(x) | \ >>>> - REG_EDGE_POL_LOW(x) | \ >>>> - REG_BOTH_EDGE(x)) >>>> +#define REG_EDGE_POL_EDGE(params, >>>> x) BIT((params)->edge_single_offset + (x)) >>>> +#define REG_EDGE_POL_LOW(params, x) BIT((params)->pol_low_offset + >>>> (x)) >>>> +#define REG_BOTH_EDGE(params, x) BIT((params)->edge_both_offset + >>>> (x)) >>>> +#define REG_EDGE_POL_MASK(params, x) ( \ >>>> + REG_EDGE_POL_EDGE(params, x) | \ >>>> + REG_EDGE_POL_LOW(params, x) | \ >>>> + REG_BOTH_EDGE(params, x)) >>>> #define REG_PIN_SEL_SHIFT(x) (((x) % 4) * 8) >>>> #define REG_FILTER_SEL_SHIFT(x) ((x) * 4) >>>> >>>> +#define INIT_MESON8_COMMON_DATA \ >>>> + .edge_single_offset = 0, \ >>>> + .pol_low_offset = 16, \ >>>> + .pin_sel_mask = 0xff, \ >>>> + .ops = { \ >>>> + .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, \ >>>> + }, >>> >>> Please place the #defines that operate on the various data >>> structures >>> *after* the definition of the structures. It would greatly help >>> reading the changes. >>> >> OK, will place it below the definition of struct >> meson_gpio_irq_params >> in the next patch. >>>> + >>>> +struct meson_gpio_irq_controller; >>>> +static void meson8_gpio_irq_sel_pin(struct >>>> meson_gpio_irq_controller >>>> *ctl, >>>> + unsigned int channel, unsigned long hwirq); >>>> +struct irq_ctl_ops { >>>> + void (*gpio_irq_sel_pin)(struct meson_gpio_irq_controller *ctl, >>>> + unsigned int channel, >>>> + unsigned long hwirq); >>>> + void (*gpio_irq_init)(struct meson_gpio_irq_controller *ctl); >>>> +}; >>>> + >>>> struct meson_gpio_irq_params { >>>> unsigned int nr_hwirq; >>>> bool support_edge_both; >>>> + unsigned int edge_both_offset; >>>> + unsigned int edge_single_offset; >>>> + unsigned int pol_low_offset; >>>> + unsigned int pin_sel_mask; >>>> + struct irq_ctl_ops ops; >>>> }; >>>> >>>> static const struct meson_gpio_irq_params meson8_params = { >>>> .nr_hwirq = 134, >>>> + INIT_MESON8_COMMON_DATA >>>> }; >>>> >>>> static const struct meson_gpio_irq_params meson8b_params = { >>>> .nr_hwirq = 119, >>>> + INIT_MESON8_COMMON_DATA >>>> }; >>>> >>>> static const struct meson_gpio_irq_params gxbb_params = { >>>> .nr_hwirq = 133, >>>> + INIT_MESON8_COMMON_DATA >>>> }; >>>> >>>> static const struct meson_gpio_irq_params gxl_params = { >>>> .nr_hwirq = 110, >>>> + INIT_MESON8_COMMON_DATA >>>> }; >>>> >>>> static const struct meson_gpio_irq_params axg_params = { >>>> .nr_hwirq = 100, >>>> + INIT_MESON8_COMMON_DATA >>>> }; >>>> >>>> static const struct meson_gpio_irq_params sm1_params = { >>>> .nr_hwirq = 100, >>>> .support_edge_both = true, >>>> + .edge_both_offset = 8, >>>> + INIT_MESON8_COMMON_DATA >>>> }; >>> >>> OK, this isn't great. The least you could do is to make >>> your initializer parametric, so that it takes the nr_hwirq as >>> a parameter. >>> >>> Then, any additional member that overrides common behaviour >>> should come after the main initializer. >>> >>> Also, do you need 'support_edge_both'? Isn't a non-zero >>> 'edge_both_offset' enough to detect the feature? >>> >> >> Sorry, but I am not very clear that "make your initializer >> parametric, >> so that it takes the nr_hwirq as a parameter". Is that >> initializer(initial function in .ops ? ) as a parameter of struct >> meson_gpio_irq_params ? If nr_hwirq as a parameter of init function >> of >> .ops then will make lot of init function for each platform. >> >> How about move .ops from macro like below: >> #define INIT_MESON8_COMMON_DATA \ >> .edge_single_offset = 0, \ >> .pol_low_offset = 16, \ >> .pin_sel_mask = 0xff, >> >> static const struct meson_gpio_irq_params sm1_params = { >> .nr_hwirq = 100,//main initializer >> .ops = { >> .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, >> /*in below to assign support_edge_both >> * edge_both_offset >> * call after main initializer to additional >> * member >> */ >> .gpio_irq_init = meson_sm1_irq_init, >> }, >> INIT_MESON8_COMMON_DATA// m8 to sm1 are the same. >> }; > > No, what I'm suggesting is something like this: > > diff --git a/drivers/irqchip/irq-meson-gpio.c > b/drivers/irqchip/irq-meson-gpio.c > index 8478100706a6..27a3207a944d 100644 > --- a/drivers/irqchip/irq-meson-gpio.c > +++ b/drivers/irqchip/irq-meson-gpio.c > @@ -43,24 +43,27 @@ > /* Below is used for Meson-A1 series like chips*/ > #define REG_PIN_A1_SEL 0x04 > > -#define INIT_MESON8_COMMON_DATA \ > - .edge_single_offset = 0, \ > - .pol_low_offset = 16, \ > - .pin_sel_mask = 0xff, \ > - .ops = { \ > - .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, \ > - }, > - > -#define INIT_MESON_A1_COMMON_DATA \ > - .support_edge_both = true, \ > - .edge_both_offset = 16, \ > - .edge_single_offset = 8, \ > - .pol_low_offset = 0, \ > - .pin_sel_mask = 0x7f, \ > - .ops = { \ > - .gpio_irq_sel_pin = meson_a1_gpio_irq_sel_pin, \ > - .gpio_irq_init = meson_a1_gpio_irq_init, \ > - }, > +#define INIT_MESON_COMMON(irqs, init, sel) \ > + .nr_hwirq = irqs, \ > + .ops = { \ > + .gpio_irq_sel_pin = sel, \ > + .gpio_irq_init = init, \ > + } > + > +#define INIT_MESON8_COMMON_DATA(irqs) \ > + INIT_MESON_COMMON(irqs, NULL, \ > + meson8_gpio_irq_sel_pin), \ > + .pol_low_offset = 16, \ > + .pin_sel_mask = 0xff, > + > +#define INIT_MESON_A1_COMMON_DATA(irqs) \ > + INIT_MESON_COMMON(irqs, meson_a1_gpio_irq_init, \ > + meson_a1_gpio_irq_sel_pin), \ > + .support_edge_both = true, \ > + .edge_both_offset = 16, \ > + .edge_single_offset = 8, \ > + .pol_low_offset = 0, \ > + .pin_sel_mask = 0x7f, > > struct meson_gpio_irq_controller; > static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller > *ctl, > @@ -89,40 +92,33 @@ struct meson_gpio_irq_params { > }; > > static const struct meson_gpio_irq_params meson8_params = { > - .nr_hwirq = 134, > - INIT_MESON8_COMMON_DATA > + INIT_MESON8_COMMON_DATA(134), > }; > > static const struct meson_gpio_irq_params meson8b_params = { > - .nr_hwirq = 119, > - INIT_MESON8_COMMON_DATA > + INIT_MESON8_COMMON_DATA(119), > }; > > static const struct meson_gpio_irq_params gxbb_params = { > - .nr_hwirq = 133, > - INIT_MESON8_COMMON_DATA > + INIT_MESON8_COMMON_DATA(133), > }; > > static const struct meson_gpio_irq_params gxl_params = { > - .nr_hwirq = 110, > - INIT_MESON8_COMMON_DATA > + INIT_MESON8_COMMON_DATA(110), > }; > > static const struct meson_gpio_irq_params axg_params = { > - .nr_hwirq = 100, > - INIT_MESON8_COMMON_DATA > + INIT_MESON8_COMMON_DATA(100), > }; > > static const struct meson_gpio_irq_params sm1_params = { > - .nr_hwirq = 100, > + INIT_MESON8_COMMON_DATA(100), > .support_edge_both = true, > .edge_both_offset = 8, > - INIT_MESON8_COMMON_DATA > }; > > static const struct meson_gpio_irq_params a1_params = { > - .nr_hwirq = 62, > - INIT_MESON_A1_COMMON_DATA > + INIT_MESON_A1_COMMON_DATA(62), > }; > Thanks, will try it in later patch > static const struct of_device_id meson_irq_gpio_matches[] = { > > > Thanks, > > M. > _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/4] irqchip/meson-gpio: Add support for meson a1 SoCs 2019-12-06 12:17 [PATCH 0/4] irqchip/meson-gpio: Add support for Meson-A1 SoC Qianggui Song 2019-12-06 12:17 ` [PATCH 1/4] dt-bindings: interrupt-controller: New binding for Meson-A1 SoCs Qianggui Song 2019-12-06 12:17 ` [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs Qianggui Song @ 2019-12-06 12:17 ` Qianggui Song 2019-12-06 12:17 ` [PATCH 4/4] arm64: dts: meson: a1: add gpio interrupt controller support Qianggui Song 3 siblings, 0 replies; 9+ messages in thread From: Qianggui Song @ 2019-12-06 12:17 UTC (permalink / raw) To: Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Qianggui Song, Hanjie Lin, Jianxin Pan, Neil Armstrong, Kevin Hilman, linux-kernel, linux-arm-kernel, linux-amlogic, Xingyu Chen, Jerome Brunet The meson a1 Socs have some changes compared with previous chips. For A113L, it contains 62 pins and can be spied on: - 62:128 undefined - 61:50 12 pins on bank A - 49:37 13 pins on bank F - 36:20 17 pins on bank X - 19:13 7 pins on bank B - 12:0 13 pins on bank P There are five relative registers for gpio interrupt controller, details are as below: - PADCTRL_GPIO_IRQ_CTRL0 bit[31]: enable/disable the whole irq lines bit[16-23]: both edge trigger bit[8-15]: single edge trigger bit[0-7]: pol trigger - PADCTRL_GPIO_IRQ_CTRL[X] bit[0-6]: 7 bits to choose gpio source for irq line 2*[X] - 2 bit[16-22]: 7 bits to choose gpio source for irq line 2*[X] - 1 where X =1,2,3,4 Signed-off-by: Qianggui Song <qianggui.song@amlogic.com> --- drivers/irqchip/irq-meson-gpio.c | 47 ++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c index 1824ffc30de2..8478100706a6 100644 --- a/drivers/irqchip/irq-meson-gpio.c +++ b/drivers/irqchip/irq-meson-gpio.c @@ -40,6 +40,9 @@ #define REG_PIN_SEL_SHIFT(x) (((x) % 4) * 8) #define REG_FILTER_SEL_SHIFT(x) ((x) * 4) +/* Below is used for Meson-A1 series like chips*/ +#define REG_PIN_A1_SEL 0x04 + #define INIT_MESON8_COMMON_DATA \ .edge_single_offset = 0, \ .pol_low_offset = 16, \ @@ -48,9 +51,26 @@ .gpio_irq_sel_pin = meson8_gpio_irq_sel_pin, \ }, +#define INIT_MESON_A1_COMMON_DATA \ + .support_edge_both = true, \ + .edge_both_offset = 16, \ + .edge_single_offset = 8, \ + .pol_low_offset = 0, \ + .pin_sel_mask = 0x7f, \ + .ops = { \ + .gpio_irq_sel_pin = meson_a1_gpio_irq_sel_pin, \ + .gpio_irq_init = meson_a1_gpio_irq_init, \ + }, + struct meson_gpio_irq_controller; static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl, unsigned int channel, unsigned long hwirq); +static void meson_a1_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl, + unsigned int channel, + unsigned long hwirq); + +static void meson_a1_gpio_irq_init(struct meson_gpio_irq_controller *ctl); + struct irq_ctl_ops { void (*gpio_irq_sel_pin)(struct meson_gpio_irq_controller *ctl, unsigned int channel, @@ -100,6 +120,11 @@ static const struct meson_gpio_irq_params sm1_params = { INIT_MESON8_COMMON_DATA }; +static const struct meson_gpio_irq_params a1_params = { + .nr_hwirq = 62, + INIT_MESON_A1_COMMON_DATA +}; + static const struct of_device_id meson_irq_gpio_matches[] = { { .compatible = "amlogic,meson8-gpio-intc", .data = &meson8_params }, { .compatible = "amlogic,meson8b-gpio-intc", .data = &meson8b_params }, @@ -108,6 +133,7 @@ static const struct of_device_id meson_irq_gpio_matches[] = { { .compatible = "amlogic,meson-axg-gpio-intc", .data = &axg_params }, { .compatible = "amlogic,meson-g12a-gpio-intc", .data = &axg_params }, { .compatible = "amlogic,meson-sm1-gpio-intc", .data = &sm1_params }, + { .compatible = "amlogic,meson-a1-gpio-intc", .data = &a1_params }, { } }; @@ -144,6 +170,21 @@ static void meson8_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl, hwirq << bit_offset); } +static void meson_a1_gpio_irq_sel_pin(struct meson_gpio_irq_controller *ctl, + unsigned int channel, + unsigned long hwirq) +{ + unsigned int reg_offset; + unsigned int bit_offset; + + bit_offset = ((channel % 2) == 0) ? 0 : 16; + reg_offset = REG_PIN_A1_SEL + ((channel / 2) << 2); + + meson_gpio_irq_update_bits(ctl, reg_offset, + ctl->params->pin_sel_mask << bit_offset, + hwirq << bit_offset); +} + static int meson_gpio_irq_request_channel(struct meson_gpio_irq_controller *ctl, unsigned long hwirq, @@ -250,6 +291,12 @@ static int meson_gpio_irq_type_setup(struct meson_gpio_irq_controller *ctl, return 0; } +/* For a1 or later chips like a1 there is a switch to enable/disable irq */ +static void meson_a1_gpio_irq_init(struct meson_gpio_irq_controller *ctl) +{ + meson_gpio_irq_update_bits(ctl, REG_EDGE_POL, BIT(31), BIT(31)); +} + static unsigned int meson_gpio_irq_type_output(unsigned int type) { unsigned int sense = type & IRQ_TYPE_SENSE_MASK; -- 2.24.0 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/4] arm64: dts: meson: a1: add gpio interrupt controller support 2019-12-06 12:17 [PATCH 0/4] irqchip/meson-gpio: Add support for Meson-A1 SoC Qianggui Song ` (2 preceding siblings ...) 2019-12-06 12:17 ` [PATCH 3/4] irqchip/meson-gpio: Add support for meson a1 SoCs Qianggui Song @ 2019-12-06 12:17 ` Qianggui Song 3 siblings, 0 replies; 9+ messages in thread From: Qianggui Song @ 2019-12-06 12:17 UTC (permalink / raw) To: Thomas Gleixner, Jason Cooper, Marc Zyngier Cc: Qianggui Song, devicetree, Hanjie Lin, Jianxin Pan, Neil Armstrong, Kevin Hilman, linux-kernel, Rob Herring, linux-arm-kernel, linux-amlogic, Mark Rutland, Xingyu Chen, Jerome Brunet add gpio interrupt controller node to a1 SoC Signed-off-by: Qianggui Song <qianggui.song@amlogic.com> --- arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi index 0965259af869..6d52350a5652 100644 --- a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi @@ -92,6 +92,15 @@ gpio: bank@0400 { }; + gpio_intc: interrupt-controller@0440 { + compatible = "amlogic,meson-gpio-intc", + "amlogic,meson-a1-gpio-intc"; + reg = <0x0 0x0440 0x0 0x14>; + interrupt-controller; + #interrupt-cells = <2>; + amlogic,channel-interrupts = <49 50 51 52 53 54 55 56>; + }; + uart_AO: serial@1c00 { compatible = "amlogic,meson-gx-uart", "amlogic,meson-ao-uart"; -- 2.24.0 _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-12-12 11:13 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-06 12:17 [PATCH 0/4] irqchip/meson-gpio: Add support for Meson-A1 SoC Qianggui Song 2019-12-06 12:17 ` [PATCH 1/4] dt-bindings: interrupt-controller: New binding for Meson-A1 SoCs Qianggui Song 2019-12-06 12:17 ` [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs Qianggui Song 2019-12-06 13:13 ` Marc Zyngier 2019-12-10 2:08 ` Qianggui Song 2019-12-11 17:26 ` Marc Zyngier 2019-12-12 11:13 ` Qianggui Song 2019-12-06 12:17 ` [PATCH 3/4] irqchip/meson-gpio: Add support for meson a1 SoCs Qianggui Song 2019-12-06 12:17 ` [PATCH 4/4] arm64: dts: meson: a1: add gpio interrupt controller support Qianggui Song
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).