Re: [PATCH v2 2/2] irqchip: Add Qualcomm MPM controller driver

[Marc Zyngier <maz@kernel.org>]

On Tue, 30 Nov 2021 09:17:08 +0000,
Shawn Guo <shawn.guo@linaro.org> wrote:
> 
> On Tue, Nov 30, 2021 at 08:42:53AM +0000, Marc Zyngier wrote:
> > On Tue, 30 Nov 2021 02:31:52 +0000,
> > Shawn Guo <shawn.guo@linaro.org> wrote:
> > > 
> > > + Maulik
> > > 
> > > On Mon, Nov 29, 2021 at 03:24:39PM +0000, Marc Zyngier wrote:
> > > [...]
> > > > > > > @@ -430,6 +430,14 @@ config QCOM_PDC
> > > > > > >  	  Power Domain Controller driver to manage and configure wakeup
> > > > > > >  	  IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
> > > > > > >  
> > > > > > > +config QCOM_MPM
> > > > > > > +	bool "QCOM MPM"
> > > > > > 
> > > > > > Can't be built as a module?
> > > > > 
> > > > > The driver is implemented as a builtin_platform_driver().
> > > > 
> > > > This, on its own, shouldn't preclude the driver from being built as a
> > > > module. However, the config option only allows it to be built in. Why?
> > > 
> > > I just tried to build it as a module, and it seems that "irq_to_desc" is
> > > only available for built-in build.
> > 
> > Yet another thing that you should not be using. The irqdomain code
> > gives you everything you need without having to resort to the
> > internals of the core IRQ infrastructure.
> 
> I see.  I should use irq_get_irq_data() rather than &desc->irq_data.

Even better:

	desc = irq_resolve_mapping(domain, hwirq);

Job done. No extra tracking, no dubious hack in the unmask callback,
works with modules.

> 
> > 
> > > > Furthermore, why would you look up anywhere other than the wake-up
> > > > domain? My impression was that only these interrupts would require
> > > > being re-triggered.
> > > 
> > > Both domains have MPM pins that could wake up system.
> > 
> > Then why do you need two domains?
> 
> This is basically the same situation as qcom-pdc, and I have some
> description about that in the commit log:
> 
> - For given SoC, a fixed number of MPM pins are supported, e.g. 96 pins
>   on QCM2290.  Each of these MPM pins can be either a MPM_GIC pin or
>   a MPM_GPIO pin. The mapping between MPM_GIC pin and GIC interrupt
>   is defined by SoC, as well as the mapping between MPM_GPIO pin and
>   GPIO number.  The former mapping can be found as the SoC data in this
>   MPM driver, while the latter can be found as the msm_gpio_wakeirq_map[]
>   in TLMM driver.
> 
> - Two irq domains are created for a single irq_chip to handle MPM_GIC
>   and MPM_GPIO pins respectively, i.e. MPM_GIC domain and MPM_GPIO domain.
>   The former is a child domain of GIC irq domain, while the latter is
>   a parent domain of TLMM/GPIO irq domain.

That doesn't answer my question.

It doesn't matter what the pins are used for as long as you can
identify which ones are routed to the GIC and which are not. You are
obviously are able to do so, since you are able to disconnect part of
the hierarchy (why is qcom_mpm_gic_alloc() named as such, since most
of the interrupts it deals with are *never* routed to the GIC).

All the interrupts have the same irqchip callbacks and act on the same
'priv' data, so they it is obvious they don't overlap in the hwirq
space.

Ergo: you can implement the whole thing with a single domain. All you
need to make sure is that you identify the pins that are routed to the
GIC, and you already have that information.

	M.

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