Re: [EXT] Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller

From: Marc Zyngier <maz@kernel.org>
To: Frank Li <frank.li@nxp.com>
Cc: "tglx@linutronix.de" <tglx@linutronix.de>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"krzysztof.kozlowski+dt@linaro.org"
	<krzysztof.kozlowski+dt@linaro.org>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
	"kw@linux.com" <kw@linux.com>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	Peng Fan <peng.fan@nxp.com>, Aisheng Dong <aisheng.dong@nxp.com>,
	"jdmason@kudzu.us" <jdmason@kudzu.us>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	"festevam@gmail.com" <festevam@gmail.com>,
	dl-linux-imx <linux-imx@nxp.com>, "kishon@ti.com" <kishon@ti.com>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"ntb@lists.linux.dev" <ntb@lists.linux.dev>,
	"lznuaa@gmail.com" <lznuaa@gmail.com>
Subject: Re: [EXT] Re: [PATCH v4 2/4] irqchip: imx mu worked as msi controller
Date: Mon, 15 Aug 2022 09:58:46 +0100	[thread overview]
Message-ID: <87czd1vq61.wl-maz@kernel.org> (raw)
In-Reply-To: <PAXPR04MB918614DF535DA7FBADC956F988699@PAXPR04MB9186.eurprd04.prod.outlook.com>

On Sun, 14 Aug 2022 04:12:01 +0100,
Frank Li <frank.li@nxp.com> wrote:

> > > new file mode 100644
> > > index 0000000000000..bb111412d598f
> > > --- /dev/null
> > > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > > @@ -0,0 +1,443 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * NXP MU worked as MSI controller
> > 
> > Freescale? Or NXP? Please make up your mind.
> 
> 
> [Frank Li] NXP and freescale is the same thing. 
> It is mux used at many place.

Pick one, and stick to it. Having two names for the same thing is
pointlessly confusing.

> > > +static struct irq_chip imx_mu_msi_irq_chip = {
> > > +     .name = "MU-MSI",
> > > +     .irq_ack = irq_chip_ack_parent,
> > 
> > Crucially, no irq_write_msi_msg callback. So we happily inherit
> > platform_msi_write_msg() and use the per descriptor write_msg()
> > callback. Who sets this? Nobody.
> 
> [Frank Li] when set flag MSI_FLAG_USE_DEF_CHIP_OPS, 
>  irq_write_msi_msg callback will be set at function platform_msi_update_chip_ops();

That wasn't my question. But never mind, I found the call to
platform_msi_domain_alloc_irqs() in patch #4.

> > > +
> > > +     /* Initialize MSI domain parent */
> > > +     msi_data->parent = irq_domain_create_linear(fwnodes,
> > > +                                                 IMX_MU_CHANS,
> > > +                                                 &imx_mu_msi_domain_ops,
> > > +                                                 msi_data);
> > 
> > Consider setting the bus_token attribute for this domain to something
> > that isn't the default, as it otherwise clashes with the following
> > creation.
> 
> [Frank Li] Any suggestion? Which bus_token is good?

DOMAIN_BUS_NEXUS is what other drivers use.

> > > +     priv->pd_a = dev_pm_domain_attach_by_name(dev, "a");
> > 
> > I'm sorry, but you'll have to come up with something slightly more
> > descriptive than "a" or "b". At least add a qualifier to it. Same
> > thing for the DT by the way.
> 
> [Frank Li] MU spec using  term "A side" and "B side".  So I think "a" and "b"
> is enough.

No, it really isn't.

> 
> Or do you think "a-side" is better?

No, I would like something fully descriptive. The DT actually has
"Processor A-facing", which seems like a reasonable description.

	M.

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