lkml.org 
[lkml]   [2019]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs
    From
    Date
    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.
    >

    \
     
     \ /
      Last update: 2019-12-10 03:08    [W:2.953 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site