| 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
| ||
|
Date: Tue, 10 Dec 2019 10:08:04 +0800
From: Qianggui Song <qianggui.song@...ogic.com>
To: Marc Zyngier <maz@...nel.org>
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
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@...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.
>
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.
>
Powered by blists - more mailing lists