Re: [PATCH 3/9] irqchip/loongson-pch-pic: Add ACPI init support

[Marc Zyngier]

On Thu, 12 Aug 2021 13:23:27 +0100,
Huacai Chen <chenhuacai@xxxxxxxxx> wrote:
> 
[...]

> > > > +struct fwnode_handle *pch_pic_acpi_init(struct fwnode_handle *parent,
> > > > +                                     struct acpi_madt_bio_pic *acpi_pchpic)
> > > > +{
> > > > +     int count;
> > > > +     struct pch_pic *priv;
> > > > +     struct irq_domain *parent_domain;
> > > > +
> > > > +     priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > > > +     if (!priv)
> > > > +             return NULL;
> > > > +
> > > > +     raw_spin_lock_init(&priv->pic_lock);
> > > > +     priv->base = ioremap(acpi_pchpic->address, acpi_pchpic->size);
> > > > +     if (!priv->base)
> > > > +             goto free_priv;
> > > > +
> > > > +     priv->domain_handle = irq_domain_alloc_fwnode(priv->base);
> > > > +     if (!priv->domain_handle) {
> > > > +             pr_err("Unable to allocate domain handle\n");
> > > > +             goto iounmap_base;
> > > > +     }
> > > > +
> > > > +     priv->ht_vec_base = acpi_pchpic->gsi_base;
> > > > +     count = ((readq(priv->base) >> 48) & 0xff) + 1;
> > > > +     parent_domain = irq_find_matching_fwnode(parent, DOMAIN_BUS_ANY);
> > > > +     if (!parent_domain) {
> > > > +             pr_err("Failed to find the parent domain\n");
> > > > +             goto iounmap_base;
> > > > +     }
> > > > +
> > > > +     priv->pic_domain = irq_domain_create_hierarchy(parent_domain, 0,
> > > > +                                             count, priv->domain_handle,
> > > > +                                             &pch_pic_domain_ops, priv);
> > > > +
> > > > +     if (!priv->pic_domain) {
> > > > +             pr_err("Failed to create IRQ domain\n");
> > > > +             goto iounmap_base;
> > > > +     }
> > > > +
> > > > +     pch_pic_reset(priv);
> > > > +     pch_pic_priv[nr_pch_pics++] = priv;
> > > > +
> > > > +     register_syscore_ops(&pch_pic_syscore_ops);
> > > > +
> > > > +     return priv->domain_handle;
> > > > +
> > > > +iounmap_base:
> > > > +     iounmap(priv->base);
> > > > +free_priv:
> > > > +     kfree(priv);
> > > > +
> > > > +     return NULL;
> > > > +}
> > > > +
> > > > +#endif
> > >
> > > A lot of this code is common with its OF counterpart. How about making
> > > this logic common?
> > OK, let me think about.
> Though pch_pic_acpi_init() is similar to pch_pic_of_init(), but it is
> difficult to make a common function, because we cannot prepare
> everything before the common function. For example, ioremap() can be
> the common part, but pch_pic_acpi_init() should get the vector count
> after ioremap(). If we use an argument to distinguish the caller in
> the common function, the complexity increases and seems no benefits.

All firmware implementations allocate private data structures, irq
domains, map MMIO regions, etc. All that can be common. We even have
APIs that deal with both firmware interfaces.

As for the 'read the vector count from the HW', what does it have to
do with driving the HW using DT or ACPI? The HW doesn't *know*. If you
are conflating HW changes and firmware interfaces, then you have
already lost.

	M.

-- 
Without deviation from the norm, progress is not possible.