Message-ID: <Z31eaxD1h3Om3bHS@lizhi-Precision-Tower-5810>
Date: Tue, 7 Jan 2025 12:03:39 -0500
From: Frank Li <Frank.li@....com>
To: Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Rob Herring <robh@...nel.org>, robin.murphy@....com
Cc: Rob Herring <robh@...nel.org>, robin.murphy@....com,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Richard Zhu <hongxing.zhu@....com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	imx@...ts.linux.dev, alyssa@...enzweig.io, bpf@...r.kernel.org,
	broonie@...nel.org, jgg@...pe.ca, joro@...tes.org,
	lgirdwood@...il.com, maz@...nel.org, p.zabel@...gutronix.de,
	will@...nel.org
Subject: Re: [PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for
 i.MX95

On Fri, Dec 27, 2024 at 10:58:15AM +0100, Lorenzo Pieralisi wrote:
> On Tue, Dec 17, 2024 at 10:50:22AM -0500, Frank Li wrote:
>
> [...]
>
> > > > > Right. Question: what happens if DT shows that there are SMMU and/or
> > > > > ITS bindings/mappings but the SMMU driver and ITS driver are either not
> > > > > enabled or have not probed ?
> > > >
> > > > It is little bit complex.
> > > > iommu:
> > > > Case 1:
> > > > 	iommu{
> > > > 		status = "disabled"
> > > > 	};
> > > >
> > > > 	PCI driver normal probed. if RID is in range of iommu-map, not
> > > > any functional impact and harmless.
> > > > 	If RID is out of range of iommu-map, "false alarm" will return.
> > > > enable PCI EP device failure, but actually it can work without IOMMU.
> > >
> > > What does "false alarm" mean in practice ? PCI device enable fails
> > > but actually it should not ?
> >
> > Yes, you are right. It should work without iommu. but return failure for
> > this case.
>
> Rob, Robin, are you OK with this patch DT bindings usage (and the
> related dependencies described in Frank's reply) ?
>
> I am referring to "iommu-map" and "msi-map" usage, everything else
> is platform specific code.
>
> It looks like things can break in multiple ways but I don't want
> to hold up this series forever.

Rob and Robin:

	Let me simple summary situation. PCIe controler driver need config
"stream id" for each PCI endpoint devices for IOMMU and MSI.

	So add callback for host bridge enable/disable an endpoint devices.
In callback function, call of_map_id("iommu-map" | "msi-map") to get
devices's "stream id" from pci's rid.  Then config hardware.

	The limiation is, if smmu/its controller's "status" is disabled
and rid is out of the range of "iommu-map" and "msi-map". Enable device
will be fail although it should be success because "stream id" will not be
used at all at this case. The out of range of "iommu-map" and "msi-map" is
rare.

	dwc common pci driver simple check "msi-map", which should be
another limition and not related this patch.

	In many dwc platform (like qcom) need config "stream id" also. But
that direct parse "iommu-map" and "msi-map" by their drivers, which is not
prefered by Rob now when I try to upstream at v3
https://lore.kernel.org/imx/20240429150842.GC1709920-robh@kernel.org/

	Rob: can you help check if this is correct direction?

Frank Li

>
> Thanks,
> Lorenzo
>
> > > That does not look like a false alarm to me.
> >
> > My means:  return failure but it should work without iommu. Ideally
> > of_map_id() should return failure when iommu is disabled. It needs more
> > work for that. I think we can improve it later.
> >
> > >
> > > >
> > > > Case 2:
> > > > 	iommu {
> > > > 		status = "Okay"
> > > > 	}
> > > > 	but iommu driver have not probed yet.  PCI Host bridge driver
> > > > should defer till iommu probed.
> > > >
> > > > Worst case is "false alarm". But this happen is very rare if DTS is
> > > > correct.
> > >
> > > Explain what this means.
> >
> > It return failure, but it should return success if "iommu disabled" and
> > "RID is out of iommu-map range".
> >
> > >
> > > > MSI:
> > > > case 1:
> > > > 	msi-controller {
> > > > 		status = "disabled";
> > > > 	}
> > > > 	Whole all dwc drivers will be broken.
> > >
> > > What MSI controller. Please make an effort to be precise and explain.
> >
> > For example: ARM its, I use general term here because some other platform
> > such as RISC V have not use ARM ITS.
> >
> > pcie {
> > 	...
> > 	msi-map= <...>
> > 	...
> > }
> >
> > DWC common driver will check property "msi-map". if it exist, built-in
> > MSI controller will skip init by history reason. That is also the another
> > reason why Rob don't want us to check these standard property.
> >
> > Without MSI, system will failure back to INTx mode, same to no-msi=yes.
> > But some EP devices require MSI support.
> >
> > Frank
> >
> > >
> > > Thanks,
> > > Lorenzo
> > >
> > > > case 2:
> > > > 	msi-controller {
> > > > 		status = "Okay"
> > > > 	}
> > > > 	if msi driver have not probed yet, PCI Host bridge driver will
> > > > defer.
> > > >
> > > > Frank
> > > >
> > > > >
> > > > > I assume the LUT programming makes no difference (it is useless yes but
> > > > > should be harmless too) in this case but wanted to check with you`.
> > > > >
> > > > > Thanks,
> > > > > Lorenzo
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Register a PCI bus callback function to handle enable_device() and
> > > > > > > > disable_device() operations, setting up the LUT whenever a new PCI device
> > > > > > > > is enabled.
> > > > > > > >
> > > > > > > > Acked-by: Richard Zhu <hongxing.zhu@....com>
> > > > > > > > Signed-off-by: Frank Li <Frank.Li@....com>
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > +	int err_i, err_m;
> > > > > > > > +	u32 sid;
> > > > > > > > +
> > > > > > > > +	dev = imx_pcie->pci->dev;
> > > > > > > > +
> > > > > > > > +	target = NULL;
> > > > > > > > +	err_i = of_map_id(dev->of_node, rid, "iommu-map", "iommu-map-mask", &target, &sid_i);
> > > > > > > > +	if (target) {
> > > > > > > > +		of_node_put(target);
> > > > > > > > +	} else {
> > > > > > > > +		/*
> > > > > > > > +		 * "target == NULL && err_i == 0" means use 1:1 map RID to
> > > > > > >
> > > > > > > Is it what it means ? Or does it mean that the iommu-map property was found
> > > > > > > and RID is out of range ?
> > > > > >
> > > > > > yes, if this happen, sid_i will be equal to RID.
> > > > > >
> > > > > > >
> > > > > > > Could you point me at a sample dts for this host bridge please ?
> > > > > >
> > > > > > https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx95.dtsi
> > > > > >
> > > > > > /* 0x10~0x17 stream id for pci0 */
> > > > > >    iommu-map = <0x000 &smmu 0x10 0x1>,
> > > > > >                <0x100 &smmu 0x11 0x7>;
> > > > > >
> > > > > > /* msi part */
> > > > > >    msi-map = <0x000 &its 0x10 0x1>,
> > > > > >              <0x100 &its 0x11 0x7>;
> > > > > >
> > > > > > >
> > > > > > > > +		 * stream ID. Hardware can't support this because stream ID
> > > > > > > > +		 * only 5bits
> > > > > > >
> > > > > > > It is 5 or 6 bits ? From GENMASK(5, 0) above it should be 6.
> > > > > >
> > > > > > Sorry for typo. it is 6bits.
> > > > > >
> > > > > > >
> > > > > > > > +		 */
> > > > > > > > +		err_i = -EINVAL;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	target = NULL;
> > > > > > > > +	err_m = of_map_id(dev->of_node, rid, "msi-map", "msi-map-mask", &target, &sid_m);
> > > > > > > > +
> > > > > > > > +	/*
> > > > > > > > +	 *   err_m      target
> > > > > > > > +	 *	0	NULL		Use 1:1 map RID to stream ID,
> > > > > > >
> > > > > > > Again, is that what it really means ?
> > > > > > >
> > > > > > > > +	 *				Current hardware can't support it,
> > > > > > > > +	 *				So return -EINVAL.
> > > > > > > > +	 *      != 0    NULL		msi-map not exist, use built-in MSI.
> > > > > > >
> > > > > > > does not exist.
> > > > > > >
> > > > > > > > +	 *	0	!= NULL		Get correct streamID from RID.
> > > > > > > > +	 *	!= 0	!= NULL		Unexisted case, never happen.
> > > > > > >
> > > > > > > "Invalid combination"
> > > > > > >
> > > > > > > > +	 */
> > > > > > > > +	if (!err_m && !target)
> > > > > > > > +		return -EINVAL;
> > > > > > > > +	else if (target)
> > > > > > > > +		of_node_put(target); /* Find stream ID map entry for RID in msi-map */
> > > > > > > > +
> > > > > > > > +	/*
> > > > > > > > +	 * msi-map        iommu-map
> > > > > > > > +	 *   N                N            DWC MSI Ctrl
> > > > > > > > +	 *   Y                Y            ITS + SMMU, require the same sid
> > > > > > > > +	 *   Y                N            ITS
> > > > > > > > +	 *   N                Y            DWC MSI Ctrl + SMMU
> > > > > > > > +	 */
> > > > > > > > +	if (err_i && err_m)
> > > > > > > > +		return 0;
> > > > > > > > +
> > > > > > > > +	if (!err_i && !err_m) {
> > > > > > > > +		/*
> > > > > > > > +		 * MSI glue layer auto add 2 bits controller ID ahead of stream
> > > > > > >
> > > > > > > What's "MSI glue layer" ?
> > > > > >
> > > > > > It is common term for IC desgin, which connect IP's signal to platform with
> > > > > > some simple logic. Inside chip, when connect LUT output 6bit streamIDs
> > > > > > to MSI controller, there are 2bits hardcode controller ID information
> > > > > > append to 6 bits streamID.
> > > > > >
> > > > > >            Glue Layer
> > > > > >           <==========>
> > > > > > ┌─────┐                  ┌──────────┐
> > > > > > │ LUT │ 6bit stream ID   │          │
> > > > > > │     ┼─────────────────►│  MSI     │
> > > > > > └─────┘    2bit ctrl ID  │          │
> > > > > >             ┌───────────►│          │
> > > > > >             │            │          │
> > > > > >  00 PCIe0   │            │          │
> > > > > >  01 ENETC   │            │          │
> > > > > >  10 PCIe1   │            │          │
> > > > > >             │            └──────────┘
> > > > > >
> > > > > > >
> > > > > > > > +		 * ID, so mask this 2bits to get stream ID.
> > > > > > > > +		 * But IOMMU glue layer doesn't do that.
> > > > > > >
> > > > > > > and "IOMMU glue layer" ?
> > > > > >
> > > > > > See above.
> > > > > >
> > > > > > Frank
> > > > > >
> > > > > > >
> > > > > > > > +		 */
> > > > > > > > +		if (sid_i != (sid_m & IMX95_SID_MASK)) {
> > > > > > > > +			dev_err(dev, "iommu-map and msi-map entries mismatch!\n");
> > > > > > > > +			return -EINVAL;
> > > > > > > > +		}
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	sid = sid_i;
> > > > > > >
> > > > > > > err_i could be != 0 here, I understand that the end result is
> > > > > > > fine given how the code is written but it is misleading.
> > > > > > >
> > > > > > > 	if (!err_i)
> > > > > > > 	else if (!err_m)
> > > > > >
> > > > > > Okay
> > > > > >
> > > > > > >
> > > > > > > > +	if (!err_m)
> > > > > > > > +		sid = sid_m & IMX95_SID_MASK;
> > > > > > > > +
> > > > > > > > +	return imx_pcie_add_lut(imx_pcie, rid, sid);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void imx_pcie_disable_device(struct pci_host_bridge *bridge, struct pci_dev *pdev)
> > > > > > > > +{
> > > > > > > > +	struct imx_pcie *imx_pcie;
> > > > > > > > +
> > > > > > > > +	imx_pcie = to_imx_pcie(to_dw_pcie_from_pp(bridge->sysdata));
> > > > > > > > +	imx_pcie_remove_lut(imx_pcie, pci_dev_id(pdev));
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static int imx_pcie_host_init(struct dw_pcie_rp *pp)
> > > > > > > >  {
> > > > > > > >  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > > > > @@ -946,6 +1122,11 @@ static int imx_pcie_host_init(struct dw_pcie_rp *pp)
> > > > > > > >  		}
> > > > > > > >  	}
> > > > > > > >
> > > > > > > > +	if (pp->bridge && imx_check_flag(imx_pcie, IMX_PCIE_FLAG_HAS_LUT)) {
> > > > > > > > +		pp->bridge->enable_device = imx_pcie_enable_device;
> > > > > > > > +		pp->bridge->disable_device = imx_pcie_disable_device;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > >  	imx_pcie_assert_core_reset(imx_pcie);
> > > > > > > >
> > > > > > > >  	if (imx_pcie->drvdata->init_phy)
> > > > > > > > @@ -1330,6 +1511,8 @@ static int imx_pcie_probe(struct platform_device *pdev)
> > > > > > > >  	imx_pcie->pci = pci;
> > > > > > > >  	imx_pcie->drvdata = of_device_get_match_data(dev);
> > > > > > > >
> > > > > > > > +	mutex_init(&imx_pcie->lock);
> > > > > > > > +
> > > > > > > >  	/* Find the PHY if one is defined, only imx7d uses it */
> > > > > > > >  	np = of_parse_phandle(node, "fsl,imx7d-pcie-phy", 0);
> > > > > > > >  	if (np) {
> > > > > > > > @@ -1627,7 +1810,8 @@ static const struct imx_pcie_drvdata drvdata[] = {
> > > > > > > >  	},
> > > > > > > >  	[IMX95] = {
> > > > > > > >  		.variant = IMX95,
> > > > > > > > -		.flags = IMX_PCIE_FLAG_HAS_SERDES,
> > > > > > > > +		.flags = IMX_PCIE_FLAG_HAS_SERDES |
> > > > > > > > +			 IMX_PCIE_FLAG_HAS_LUT,
> > > > > > > >  		.clk_names = imx8mq_clks,
> > > > > > > >  		.clks_cnt = ARRAY_SIZE(imx8mq_clks),
> > > > > > > >  		.ltssm_off = IMX95_PE0_GEN_CTRL_3,
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.34.1
> > > > > > > >

Powered by blists - more mailing lists