Re: [PATCH 2/4] irqchip/meson-gpio: rework meson irqchip driver to support meson-A1 SoCs

[Qianggui Song]

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@xxxxxxxxxxx>
>> ---
>>  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.
>