[PATCH v6 03/15] irq: gic: support hip04 gic

[haojian.zhuang@linaro.org (Haojian Zhuang)]

On 20 May 2014 17:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On Tue, May 20 2014 at  4:24:08 am BST, Haojian Zhuang <haojian.zhuang@linaro.org> wrote:
>> On 15 May 2014 17:34, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 11/05/14 09:05, Haojian Zhuang wrote:
>>>>
>>>> +static inline bool gic_is_standard(struct gic_chip_data *gic)
>>>
>>> Please loose all of the inlines. The compiler can do this by itself.
>>>
>> Since others also agree on inline, I'll keep to use it.
>>
>>>> -static u8 gic_get_cpumask(struct gic_chip_data *gic)
>>>> +static u16 gic_get_cpumask(struct gic_chip_data *gic)
>>>>  {
>>>>         void __iomem *base = gic_data_dist_base(gic);
>>>>         u32 mask, i;
>>>>
>>>> -       for (i = mask = 0; i < 32; i += 4) {
>>>> -               mask = readl_relaxed(base + GIC_DIST_TARGET + i);
>>>> -               mask |= mask >> 16;
>>>> -               mask |= mask >> 8;
>>>> +       /*
>>>> +        * ARM GIC uses 8 registers for interrupt 0-31,
>>>> +        * HiP04 GIC uses 16 registers for interrupt 0-31.
>>>> +        */
>>>> +       for (i = mask = 0; i < 32; i += gic_irqs_per_target_reg(gic)) {
>>>> +               if (gic_is_standard(gic)) {
>>>> +                       mask = readl_relaxed(base + GIC_DIST_TARGET + i);
>>>> +                       mask |= mask >> 16;
>>>> +                       mask |= mask >> 8;
>>>> +               } else {                        /* HiP04 GIC */
>>>> +                       mask = readl_relaxed(base + GIC_DIST_TARGET + i * 2);
>>>> +                       mask |= mask >> 16;
>>>> +               }
>>>
>>> You have irq_to_target_reg now, and you can rewrite most of this without
>>> duplication (see my previous review comment).
>>>
>>
>> At here, the offset from GIC_DIST_TARGET is got directly.
>> In irq_to_target_reg(), the parameter is struct irq_data. These two
>> cases are different.
>>
>> How could I reuse the irq_to_target_reg() at here?
>
> By using your imagination, and redefining irq_to_target_reg to this:
>
> static inline u32 irq_to_target_reg(struct gic_chip_data *gic, int irq)
> {
>        if (!gic_is_standard(gic))
>                i *= 2;
>        irq &= ~3U;
>        return (i + GIC_DIST_TARGET);
> }
>
> You could then try modifying the only existing caller, and then rewrite
> the above hunk as such:
>
>        for (i = mask = 0; i < 32; i += gic_irqs_per_target_reg(gic)) {
>                mask = readl_relaxed(base + irq_to_target_reg(gic, i));
>                mask |= mask >> 16;
>                if (gic_is_standard(gic))
>                        mask |= mask >> 8;
>        }
>

But why should I name it as irq_to_target_reg()? There's nothing related
to irq at here.

So I'll append a new function to handle it.

Regards
Haojian