Date: Mon, 24 Jun 2024 11:06:01 -0400
From: Frank Li <Frank.li@....com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Richard Zhu <hongxing.zhu@....com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	NXP Linux Team <linux-imx@....com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-pci@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, bpf@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v5 08/12] PCI: imx6: Config look up table(LUT) to support
 MSI ITS and IOMMU for i.MX95

On Sat, Jun 22, 2024 at 09:41:15AM +0530, Manivannan Sadhasivam wrote:
> On Tue, May 28, 2024 at 03:39:21PM -0400, Frank Li wrote:
> > For the i.MX95, configuration of a LUT is necessary to convert Bus Device
> > Function (BDF) to stream IDs, which are utilized by both IOMMU and ITS.
> > This involves examining the msi-map and smmu-map to ensure consistent
> > mapping of PCI BDF to the same stream IDs. Subsequently, LUT-related
> > registers are configured. In the absence of an msi-map, the built-in MSI
> > controller is utilized as a fallback.
> > 
> > Additionally, register a PCI bus notifier to trigger imx_pcie_add_device()
> > upon the appearance of a new PCI device and when the bus is an iMX6 PCI
> > controller. This function configures the correct LUT based on Device Tree
> > Settings (DTS).
> > 
> 
> Sorry for jumping the ship very late... But why can't you configure the LUT
> during probe() itself? Anyway, you are going to use the 'iommu-map' and
> 'msi-map' which are static info provided in DT. I don't see a necessity to do it
> during device addition time.
> 
> Qcom RC driver also uses a similar configuration in
> qcom_pcie_config_sid_1_9_0().

It is similar with my v3 version.
https://lore.kernel.org/imx/20240402-pci2_upstream-v3-8-803414bdb430@nxp.com/

Rob don't like parse these property by individial driver.
https://lore.kernel.org/imx/20240429150842.GC1709920-robh@kernel.org/#t

But if use standarnd of_map_xxx() API to get sid, it needs RID information,
which only get when new PCI device added.

SID problem may take more long time to discuss. Can you help review v6
version:

https://lore.kernel.org/imx/20240617-pci2_upstream-v6-0-e0821238f997@nxp.com/T/#t

I want make bug fixes and 8qxp support to be merged firstly.

Frank

> 
> - Mani
> 
> > Signed-off-by: Frank Li <Frank.Li@....com>
> > ---
> >  drivers/pci/controller/dwc/pci-imx6.c | 175 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 174 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> > index 29309ad0e352b..8ecc00049e20b 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -54,6 +54,22 @@
> >  #define IMX95_PE0_GEN_CTRL_3			0x1058
> >  #define IMX95_PCIE_LTSSM_EN			BIT(0)
> >  
> > +#define IMX95_PE0_LUT_ACSCTRL			0x1008
> > +#define IMX95_PEO_LUT_RWA			BIT(16)
> > +#define IMX95_PE0_LUT_ENLOC			GENMASK(4, 0)
> > +
> > +#define IMX95_PE0_LUT_DATA1			0x100c
> > +#define IMX95_PE0_LUT_VLD			BIT(31)
> > +#define IMX95_PE0_LUT_DAC_ID			GENMASK(10, 8)
> > +#define IMX95_PE0_LUT_STREAM_ID			GENMASK(5, 0)
> > +
> > +#define IMX95_PE0_LUT_DATA2			0x1010
> > +#define IMX95_PE0_LUT_REQID			GENMASK(31, 16)
> > +#define IMX95_PE0_LUT_MASK			GENMASK(15, 0)
> > +
> > +#define IMX95_SID_MASK				GENMASK(5, 0)
> > +#define IMX95_MAX_LUT				32
> > +
> >  #define to_imx_pcie(x)	dev_get_drvdata((x)->dev)
> >  
> >  enum imx_pcie_variants {
> > @@ -79,6 +95,7 @@ enum imx_pcie_variants {
> >  #define IMX_PCIE_FLAG_HAS_PHY_RESET		BIT(5)
> >  #define IMX_PCIE_FLAG_HAS_SERDES		BIT(6)
> >  #define IMX_PCIE_FLAG_SUPPORT_64BIT		BIT(7)
> > +#define IMX_PCIE_FLAG_MONITOR_DEV		BIT(8)
> >  
> >  #define imx_check_flag(pci, val)     (pci->drvdata->flags & val)
> >  
> > @@ -132,6 +149,8 @@ struct imx_pcie {
> >  	struct device		*pd_pcie_phy;
> >  	struct phy		*phy;
> >  	const struct imx_pcie_drvdata *drvdata;
> > +
> > +	struct mutex		lock;
> >  };
> >  
> >  /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
> > @@ -215,6 +234,66 @@ static int imx95_pcie_init_phy(struct imx_pcie *imx_pcie)
> >  	return 0;
> >  }
> >  
> > +static int imx_pcie_config_lut(struct imx_pcie *imx_pcie, u16 reqid, u8 sid)
> > +{
> > +	struct dw_pcie *pci = imx_pcie->pci;
> > +	struct device *dev = pci->dev;
> > +	u32 data1, data2;
> > +	int i;
> > +
> > +	if (sid >= 64) {
> > +		dev_err(dev, "Invalid SID for index %d\n", sid);
> > +		return -EINVAL;
> > +	}
> > +
> > +	guard(mutex)(&imx_pcie->lock);
> > +
> > +	for (i = 0; i < IMX95_MAX_LUT; i++) {
> > +		regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_ACSCTRL, IMX95_PEO_LUT_RWA | i);
> > +
> > +		regmap_read(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_DATA1, &data1);
> > +		if (data1 & IMX95_PE0_LUT_VLD)
> > +			continue;
> > +
> > +		data1 = FIELD_PREP(IMX95_PE0_LUT_DAC_ID, 0);
> > +		data1 |= FIELD_PREP(IMX95_PE0_LUT_STREAM_ID, sid);
> > +		data1 |= IMX95_PE0_LUT_VLD;
> > +
> > +		regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_DATA1, data1);
> > +
> > +		data2 = 0xffff;
> > +		data2 |= FIELD_PREP(IMX95_PE0_LUT_REQID, reqid);
> > +
> > +		regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_DATA2, data2);
> > +
> > +		regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_ACSCTRL, i);
> > +
> > +		return 0;
> > +	}
> > +
> > +	dev_err(dev, "All lut already used\n");
> > +	return -EINVAL;
> > +}
> > +
> > +static void imx_pcie_remove_lut(struct imx_pcie *imx_pcie, u16 reqid)
> > +{
> > +	u32 data2 = 0;
> > +	int i;
> > +
> > +	guard(mutex)(&imx_pcie->lock);
> > +
> > +	for (i = 0; i < IMX95_MAX_LUT; i++) {
> > +		regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_ACSCTRL, IMX95_PEO_LUT_RWA | i);
> > +
> > +		regmap_read(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_DATA2, &data2);
> > +		if (FIELD_GET(IMX95_PE0_LUT_REQID, data2) == reqid) {
> > +			regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_DATA1, 0);
> > +			regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_DATA2, 0);
> > +			regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_ACSCTRL, i);
> > +		}
> > +	}
> > +}
> > +
> >  static void imx_pcie_configure_type(struct imx_pcie *imx_pcie)
> >  {
> >  	const struct imx_pcie_drvdata *drvdata = imx_pcie->drvdata;
> > @@ -1232,6 +1311,85 @@ static int imx_pcie_resume_noirq(struct device *dev)
> >  	return 0;
> >  }
> >  
> > +static bool imx_pcie_match_device(struct pci_bus *bus);
> > +
> > +static int imx_pcie_add_device(struct imx_pcie *imx_pcie, struct pci_dev *pdev)
> > +{
> > +	u32 sid_i = 0, sid_m = 0, rid = pci_dev_id(pdev);
> > +	struct device *dev = imx_pcie->pci->dev;
> > +	int err;
> > +
> > +	err = of_map_id(dev->of_node, rid, "iommu-map", "iommu-map-mask", NULL, &sid_i);
> > +	if (err)
> > +		return err;
> > +
> > +	err = of_map_id(dev->of_node, rid, "msi-map", "msi-map-mask", NULL, &sid_m);
> > +	if (err)
> > +		return err;
> > +
> > +	if (sid_i != rid && sid_m != rid)
> > +		if ((sid_i & IMX95_SID_MASK) != (sid_m & IMX95_SID_MASK)) {
> > +			dev_err(dev, "its and iommu stream id miss match, please check dts file\n");
> > +			return -EINVAL;
> > +		}
> > +
> > +	/* if iommu-map is not existed then use msi-map's stream id*/
> > +	if (sid_i == rid)
> > +		sid_i = sid_m;
> > +
> > +	sid_i &= IMX95_SID_MASK;
> > +
> > +	if (sid_i != rid)
> > +		return imx_pcie_config_lut(imx_pcie, rid, sid_i);
> > +
> > +	/* Use dwc built-in MSI controller */
> > +	return 0;
> > +}
> > +
> > +static void imx_pcie_del_device(struct imx_pcie *imx_pcie, struct pci_dev *pdev)
> > +{
> > +	imx_pcie_remove_lut(imx_pcie, pci_dev_id(pdev));
> > +}
> > +
> > +
> > +static int imx_pcie_bus_notifier(struct notifier_block *nb, unsigned long action, void *data)
> > +{
> > +	struct pci_host_bridge *host;
> > +	struct imx_pcie *imx_pcie;
> > +	struct pci_dev *pdev;
> > +	int err;
> > +
> > +	pdev = to_pci_dev(data);
> > +	host = pci_find_host_bridge(pdev->bus);
> > +
> > +	if (!imx_pcie_match_device(host->bus))
> > +		return NOTIFY_OK;
> > +
> > +	imx_pcie = to_imx_pcie(to_dw_pcie_from_pp(host->sysdata));
> > +
> > +	if (!imx_check_flag(imx_pcie, IMX_PCIE_FLAG_MONITOR_DEV))
> > +		return NOTIFY_OK;
> > +
> > +	switch (action) {
> > +	case BUS_NOTIFY_ADD_DEVICE:
> > +		err = imx_pcie_add_device(imx_pcie, pdev);
> > +		if (err)
> > +			return notifier_from_errno(err);
> > +		break;
> > +	case BUS_NOTIFY_DEL_DEVICE:
> > +		imx_pcie_del_device(imx_pcie, pdev);
> > +		break;
> > +	default:
> > +		return NOTIFY_DONE;
> > +	}
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block imx_pcie_nb = {
> > +	.notifier_call = imx_pcie_bus_notifier,
> > +};
> > +
> >  static const struct dev_pm_ops imx_pcie_pm_ops = {
> >  	NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_pcie_suspend_noirq,
> >  				  imx_pcie_resume_noirq)
> > @@ -1264,6 +1422,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) {
> > @@ -1551,7 +1711,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_MONITOR_DEV,
> >  		.clk_names = imx8mq_clks,
> >  		.clks_cnt = ARRAY_SIZE(imx8mq_clks),
> >  		.ltssm_off = IMX95_PE0_GEN_CTRL_3,
> > @@ -1687,6 +1848,8 @@ DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_SYNOPSYS, 0xabcd,
> >  
> >  static int __init imx_pcie_init(void)
> >  {
> > +	int ret;
> > +
> >  #ifdef CONFIG_ARM
> >  	struct device_node *np;
> >  
> > @@ -1705,7 +1868,17 @@ static int __init imx_pcie_init(void)
> >  	hook_fault_code(8, imx6q_pcie_abort_handler, SIGBUS, 0,
> >  			"external abort on non-linefetch");
> >  #endif
> > +	ret = bus_register_notifier(&pci_bus_type, &imx_pcie_nb);
> > +	if (ret)
> > +		return ret;
> >  
> >  	return platform_driver_register(&imx_pcie_driver);
> >  }
> > +
> > +static void __exit imx_pcie_exit(void)
> > +{
> > +	bus_unregister_notifier(&pci_bus_type, &imx_pcie_nb);
> > +}
> > +
> >  device_initcall(imx_pcie_init);
> > +__exitcall(imx_pcie_exit);
> > 
> > -- 
> > 2.34.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists