| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <542e3e819e584d6e433d2c4276c3b379@www.loen.fr>
Date: Fri, 06 Dec 2019 13:13:50 +0000
From: Marc Zyngier <maz@...nel.org>
To: Qianggui Song <qianggui.song@...ogic.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
Jason Cooper <jason@...edaemon.net>,
Kevin Hilman <khilman@...libre.com>,
Neil Armstrong <narmstrong@...libre.com>,
Jerome Brunet <jbrunet@...libre.com>,
Jianxin Pan <jianxin.pan@...ogic.com>,
Xingyu Chen <xingyu.chen@...ogic.com>,
Hanjie Lin <hanjie.lin@...ogic.com>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-amlogic@...ts.infradead.org>
Subject: Re: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs
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@...ogic.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...
Powered by blists - more mailing lists