Re: [PATCH v2 27/35] irqchip: Andestech Internal Vector Interrupt Controller driver

[Marc Zyngier]

On Wed, Nov 29 2017 at 11:23:34 pm GMT, Greentime Hu <green.hu@xxxxxxxxx> wrote:

Hi Greentime,

>>> +}
>>> +
>>> +static void ativic32_mask_ack_irq(struct irq_data *data)
>>> +{
>>> +     unsigned long int_mask2 = __nds32__mfsr(NDS32_SR_INT_MASK2);
>>> + __nds32__mtsr_dsb(int_mask2 & (~(1 << data->hwirq)),
>>> NDS32_SR_INT_MASK2);
>>> +     __nds32__mtsr_dsb((1 << data->hwirq), NDS32_SR_INT_PEND2);
>>
>> This is effectively MASK+ACK, so you're better off just writing it as
>> such. And since there is no advantage in your implementation in having
>> MASK_ACK over MASK+ACK, I suggest you remove this function completely,
>> and rely on the core code which will call them in sequence.
>
> I think mask_ack is still better than mask + ack because we don't need
> to do two function call.
> We can save a prologue and a epilogue. It will benefit interrupt latency.

Can you actually measure this? Your CPU would have to be extremely slow
if you could see the impact of an extra function call on interrupt
latency. From a maintenance perspective, this isn't very nice (it is
effectively code duplication).

I suggest you start with the simplest version of the code, and provide
an optimisation once you can measurably demonstrate that mask_ack is
better.

[...]

>>> + irq_set_chip_and_handler(virq, &ativic32_chip, handle_edge_irq);
>>> +     else
>>> + irq_set_chip_and_handler(virq, &ativic32_chip, handle_level_irq);
>>
>> Since you do not express the trigger in DT, you need to tell the core
>> about it by calling irqd_set_trigger_type() with the right setting.
>>
>
> Since the comments say so, I will add ativic32_set_type() for irq_set_type()
> in the next version patch.
>
> /*
>  * Must only be called inside irq_chip.irq_set_type() functions.
>  */
> static inline void irqd_set_trigger_type(struct irq_data *d, u32 type)
> {
>         __irqd_to_state(d) &= ~IRQD_TRIGGER_MASK;
>         __irqd_to_state(d) |= type & IRQD_TRIGGER_MASK;
> }
>
> It will be like this.
> static int ativic32_set_type(struct irq_data *data, unsigned int flow_type)
> {
>         irqd_set_trigger_type(data, flow_type);
>         return IRQ_SET_MASK_OK;
> }

This feels wrong. Your interrupt controller doesn't seem to support the
trigger being changed, so irq_set_type would be a bit pointless. A
driver trying to set a level trigger on an edge interrupt would succeed,
and that is as bad as it gets. All you need is to expose the capability
of the HW when you register the flow handler instead of adding a rather
misleading callback.

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.