[PATCH v8 0/2] PCI: add enabe(disable)_device() hook for bridge

[PATCH v8 0/2] PCI: add enabe(disable)_device() hook for bridge

[Frank Li]

Some system's IOMMU stream(master) ID bits(such as 6bits) less than
pci_device_id (16bit). It needs add hardware configuration to enable
pci_device_id to stream ID convert.

https://lore.kernel.org/imx/20240622173849.GA1432357@bhelgaas/
This ways use pcie bus notifier (like apple pci controller), when new PCIe
device added, bus notifier will call register specific callback to handle
look up table (LUT) configuration.

https://lore.kernel.org/imx/20240429150842.GC1709920-robh@kernel.org/
which parse dt's 'msi-map' and 'iommu-map' property to static config LUT
table (qcom use this way). This way is rejected by DT maintainer Rob.

Above ways can resolve LUT take or stream id out of usage the problem. If
there are not enough stream id resource, not error return, EP hardware
still issue DMA to do transfer, which may transfer to wrong possition.

Add enable(disable)_device() hook for bridge can return error when not
enough resource, and PCI device can't enabled.

Basicallly this version can match Bjorn's requirement:
1: simple, because it is rare that there are no LUT resource.
2: EP driver probe failure when no LUT, but lspci can see such device.

[    2.164415] nvme nvme0: pci function 0000:01:00.0
[    2.169142] pci 0000:00:00.0: Error enabling bridge (-1), continuing
[    2.175654] nvme 0000:01:00.0: probe with driver nvme failed with error -12

> lspci
0000:00:00.0 PCI bridge: Philips Semiconductors Device 0000
0000:01:00.0 Non-Volatile memory controller: Micron Technology Inc 2100AI NVMe SSD [Nitro] (rev 03)

To: Bjorn Helgaas <bhelgaas@google.com>
To: Richard Zhu <hongxing.zhu@nxp.com>
To: Lucas Stach <l.stach@pengutronix.de>
To: Lorenzo Pieralisi <lpieralisi@kernel.org>
To: Krzysztof Wilczyński <kw@linux.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Rob Herring <robh@kernel.org>
To: Shawn Guo <shawnguo@kernel.org>
To: Sascha Hauer <s.hauer@pengutronix.de>
To: Pengutronix Kernel Team <kernel@pengutronix.de>
To: Fabio Estevam <festevam@gmail.com>
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: imx@lists.linux.dev
Cc: Frank.li@nxp.com \
Cc: alyssa@rosenzweig.io \
Cc: bpf@vger.kernel.org \
Cc: broonie@kernel.org \
Cc: jgg@ziepe.ca \
Cc: joro@8bytes.org \
Cc: l.stach@pengutronix.de \
Cc: lgirdwood@gmail.com \
Cc: maz@kernel.org \
Cc: p.zabel@pengutronix.de \
Cc: robin.murphy@arm.com \
Cc: will@kernel.org \
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Marc Zyngier <maz@kernel.org>

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v8:
- update comment message according to Lorenzo Pieralisi's suggestion.
- rework err target table
- improve err==0 && target ==NULL description, use 1:1 map RID to
stream ID.
- invalidate case -> unexisted case, never happen
- sid_i will not do mask, add comments said only MSI glue layer add
controller id.
- rework iommu map and msi map return value check logic according to
Lorenzo Pieralisi's suggestion
- Link to v7: https://lore.kernel.org/r/20241203-imx95_lut-v7-0-d0cd6293225e@nxp.com

Changes in v7:
- Rebase v6.13-rc1
- Update patch 2 according to mani's feedback
- Link to v6: https://lore.kernel.org/r/20241118-imx95_lut-v6-0-a2951ba13347@nxp.com

Changes in v6:
- Bjorn give review tags at v4, but v5 have big change, drop Bjorn's review
tag.
- Add back Marc Zyngier't review and test tags
- Add mani's ack at first patch
- Mini change for patch 2 according to mani's feedback
- Link to v5: https://lore.kernel.org/r/20241104-imx95_lut-v5-0-feb972f3f13b@nxp.com

Changes in v5:
- Add help function of pci_bridge_enable(disable)_device
- Because big change, removed Bjorn's review tags and have not
added
Marc Zyngier't review and test tags
- Fix pci-imx6.c according to Mani's feedback
- Link to v4: https://lore.kernel.org/r/20241101-imx95_lut-v4-0-0fdf9a2fe754@nxp.com

Changes in v4:
- Add Bjorn Helgaas review tag for patch1
- check 'target' value for patch2
- detail see each patches
- Link to v3: https://lore.kernel.org/r/20241024-imx95_lut-v3-0-7509c9bbab86@nxp.com

Changes in v3:
- disable_device when error happen
- use target for of_map_id
- Check if rid already in lut table when enable deviced
- Link to v2: https://lore.kernel.org/r/20240930-imx95_lut-v2-0-3b6467ba539a@nxp.com

Changes in v2:
- see each patch
- Link to v1: https://lore.kernel.org/r/20240926-imx95_lut-v1-0-d0c62087dbab@nxp.com

---
Frank Li (2):
      PCI: Add enable_device() and disable_device() callbacks for bridges
      PCI: imx6: Add IOMMU and ITS MSI support for i.MX95

 drivers/pci/controller/dwc/pci-imx6.c | 186 +++++++++++++++++++++++++++++++++-
 drivers/pci/pci.c                     |  36 ++++++-
 include/linux/pci.h                   |   2 +
 3 files changed, 222 insertions(+), 2 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20240926-imx95_lut-1c68222e0944

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH v8 1/2] PCI: Add enable_device() and disable_device() callbacks for bridges

[Frank Li]

Some PCIe host bridges require special handling when enabling or disabling
PCIe Endpoints. For example, the i.MX95 platform has a lookup table to map
Requester IDs to StreamIDs, which are used by the SMMU and MSI controller
to identify the source of DMA accesses.

Without this mapping, DMA accesses may target unintended memory, which
would corrupt memory or read the wrong data.

Add a host bridge .enable_device() hook the imx6 driver can use to
configure the Requester ID to StreamID mapping. The hardware table isn't
big enough to map all possible Requester IDs, so this hook may fail if no
table space is available. In that case, return failure from
pci_enable_device().

It might make more sense to make pci_set_master() decline to enable bus
mastering and return failure, but it currently doesn't have a way to return
failure.

Reviewed-by: Marc Zyngier <maz@kernel.org>
Tested-by: Marc Zyngier <maz@kernel.org>
Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v6 to v8
- none

Change from v5 to v6
- Add Marc testedby and Reviewed-by tag
- Add Mani's acked tag

Change from v4 to v5
- Add two static help functions
int pci_host_bridge_enable_device(dev);
void pci_host_bridge_disable_device(dev);
- remove tags because big change
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Marc Zyngier <maz@kernel.org>
Tested-by: Marc Zyngier <maz@kernel.org>

Change from v3 to v4
- Add Bjorn's ack tag

Change from v2 to v3
- use Bjorn suggest's commit message.
- call disable_device() when error happen.

Change from v1 to v2
- move enable(disable)device ops to pci_host_bridge
---
 drivers/pci/pci.c   | 36 +++++++++++++++++++++++++++++++++++-
 include/linux/pci.h |  2 ++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0b29ec6e8e5e2..773ca3cbd3221 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2059,6 +2059,28 @@ int __weak pcibios_enable_device(struct pci_dev *dev, int bars)
 	return pci_enable_resources(dev, bars);
 }
 
+static int pci_host_bridge_enable_device(struct pci_dev *dev)
+{
+	struct pci_host_bridge *host_bridge = pci_find_host_bridge(dev->bus);
+	int err;
+
+	if (host_bridge && host_bridge->enable_device) {
+		err = host_bridge->enable_device(host_bridge, dev);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static void pci_host_bridge_disable_device(struct pci_dev *dev)
+{
+	struct pci_host_bridge *host_bridge = pci_find_host_bridge(dev->bus);
+
+	if (host_bridge && host_bridge->disable_device)
+		host_bridge->disable_device(host_bridge, dev);
+}
+
 static int do_pci_enable_device(struct pci_dev *dev, int bars)
 {
 	int err;
@@ -2074,9 +2096,13 @@ static int do_pci_enable_device(struct pci_dev *dev, int bars)
 	if (bridge)
 		pcie_aspm_powersave_config_link(bridge);
 
+	err = pci_host_bridge_enable_device(dev);
+	if (err)
+		return err;
+
 	err = pcibios_enable_device(dev, bars);
 	if (err < 0)
-		return err;
+		goto err_enable;
 	pci_fixup_device(pci_fixup_enable, dev);
 
 	if (dev->msi_enabled || dev->msix_enabled)
@@ -2091,6 +2117,12 @@ static int do_pci_enable_device(struct pci_dev *dev, int bars)
 	}
 
 	return 0;
+
+err_enable:
+	pci_host_bridge_disable_device(dev);
+
+	return err;
+
 }
 
 /**
@@ -2274,6 +2306,8 @@ void pci_disable_device(struct pci_dev *dev)
 	if (atomic_dec_return(&dev->enable_cnt) != 0)
 		return;
 
+	pci_host_bridge_disable_device(dev);
+
 	do_pci_disable_device(dev);
 
 	dev->is_busmaster = 0;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index db9b47ce3eefd..bcbef004dd561 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -595,6 +595,8 @@ struct pci_host_bridge {
 	u8 (*swizzle_irq)(struct pci_dev *, u8 *); /* Platform IRQ swizzler */
 	int (*map_irq)(const struct pci_dev *, u8, u8);
 	void (*release_fn)(struct pci_host_bridge *);
+	int (*enable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
+	void (*disable_device)(struct pci_host_bridge *bridge, struct pci_dev *dev);
 	void		*release_data;
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */

-- 
2.34.1

[PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for i.MX95

[Frank Li]

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 checking msi-map and iommu-map device tree properties 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.

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@nxp.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v7 to v8
- update comment message according to Lorenzo Pieralisi's suggestion.
- rework err target table
- improve err==0 && target ==NULL description, use 1:1 map RID to
stream ID.
- invalidate case -> unexisted case, never happen
- sid_i will not do mask, add comments said only MSI glue layer add
controller id.
- rework iommu map and msi map return value check logic according to
Lorenzo Pieralisi's suggestion

Change from v5 to v7
- change comment rid to RID
- some mini change according to mani's feedback

Change from v4 to v5
- rework commt message
- add comment for mutex
- s/reqid/rid/
- keep only one loop when enable lut
- add warning when try to add duplicate rid
- Replace hardcode 0xffff with IMX95_PE0_LUT_MASK
- Fix some error message

Change from v3 to v4
- Check target value at of_map_id().
- of_node_put() for target.
- add case for msi-map exist, but rid entry is not exist.

Change from v2 to v3
- Use the "target" argument of of_map_id()
- Check if rid already in lut table when enable device

change from v1 to v2
- set callback to pci_host_bridge instead pci->ops.
---
 drivers/pci/controller/dwc/pci-imx6.c | 186 +++++++++++++++++++++++++++++++++-
 1 file changed, 185 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index c8d5c90aa4d45..d325f4fb279c8 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -55,6 +55,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 {
@@ -87,6 +103,7 @@ enum imx_pcie_variants {
  * workaround suspend resume on some devices which are affected by this errata.
  */
 #define IMX_PCIE_FLAG_BROKEN_SUSPEND		BIT(9)
+#define IMX_PCIE_FLAG_HAS_LUT			BIT(10)
 
 #define imx_check_flag(pci, val)	(pci->drvdata->flags & val)
 
@@ -139,6 +156,9 @@ struct imx_pcie {
 	struct device		*pd_pcie_phy;
 	struct phy		*phy;
 	const struct imx_pcie_drvdata *drvdata;
+
+	/* Ensure that only one device's LUT is configured at any given time */
+	struct mutex		lock;
 };
 
 /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
@@ -930,6 +950,162 @@ static void imx_pcie_stop_link(struct dw_pcie *pci)
 	imx_pcie_ltssm_disable(dev);
 }
 
+static int imx_pcie_add_lut(struct imx_pcie *imx_pcie, u16 rid, u8 sid)
+{
+	struct dw_pcie *pci = imx_pcie->pci;
+	struct device *dev = pci->dev;
+	u32 data1, data2;
+	int free = -1;
+	int i;
+
+	if (sid >= 64) {
+		dev_err(dev, "Invalid SID for index %d\n", sid);
+		return -EINVAL;
+	}
+
+	guard(mutex)(&imx_pcie->lock);
+
+	/*
+	 * Iterate through all LUT entries to check for duplicate RID and
+	 * identify the first available entry. Configure this available entry
+	 * immediately after verification to avoid rescanning it.
+	 */
+	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)) {
+			if (free < 0)
+				free = i;
+			continue;
+		}
+
+		regmap_read(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_DATA2, &data2);
+
+		/* Do not add duplicate RID */
+		if (rid == FIELD_GET(IMX95_PE0_LUT_REQID, data2)) {
+			dev_warn(dev, "Existing LUT entry available for RID (%d)", rid);
+			return 0;
+		}
+	}
+
+	if (free < 0) {
+		dev_err(dev, "LUT entry is not available\n");
+		return -ENOSPC;
+	}
+
+	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 = IMX95_PE0_LUT_MASK; /* Match all bits of RID */
+	data2 |= FIELD_PREP(IMX95_PE0_LUT_REQID, rid);
+	regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_DATA2, data2);
+
+	regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_ACSCTRL, free);
+
+	return 0;
+}
+
+static void imx_pcie_remove_lut(struct imx_pcie *imx_pcie, u16 rid)
+{
+	u32 data2;
+	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) == rid) {
+			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);
+
+			break;
+		}
+	}
+}
+
+static int imx_pcie_enable_device(struct pci_host_bridge *bridge, struct pci_dev *pdev)
+{
+	struct imx_pcie *imx_pcie = to_imx_pcie(to_dw_pcie_from_pp(bridge->sysdata));
+	u32 sid_i, sid_m, rid = pci_dev_id(pdev);
+	struct device_node *target;
+	struct device *dev;
+	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
+		 * stream ID. Hardware can't support this because stream ID
+		 * only 5bits
+		 */
+		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,
+	 *				Current hardware can't support it,
+	 *				So return -EINVAL.
+	 *      != 0    NULL		msi-map not exist, use built-in MSI.
+	 *	0	!= NULL		Get correct streamID from RID.
+	 *	!= 0	!= NULL		Unexisted case, never happen.
+	 */
+	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
+		 * ID, so mask this 2bits to get stream ID.
+		 * But IOMMU glue layer doesn't do that.
+		 */
+		if (sid_i != (sid_m & IMX95_SID_MASK)) {
+			dev_err(dev, "iommu-map and msi-map entries mismatch!\n");
+			return -EINVAL;
+		}
+	}
+
+	sid = sid_i;
+	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

Re: [PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for i.MX95

[Lorenzo Pieralisi]

On Tue, Dec 10, 2024 at 05:48:59PM -0500, 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 checking msi-map and iommu-map device tree properties 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.

This is wrong information. What you want to say is that if an msi-map
isn't detected this means that the platform relies on DWC built-in
controller for MSIs (that does not need streamIDs handling).

That's quite different from what you are writing here.

> 
> 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@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> Change from v7 to v8
> - update comment message according to Lorenzo Pieralisi's suggestion.
> - rework err target table
> - improve err==0 && target ==NULL description, use 1:1 map RID to
> stream ID.
> - invalidate case -> unexisted case, never happen
> - sid_i will not do mask, add comments said only MSI glue layer add
> controller id.
> - rework iommu map and msi map return value check logic according to
> Lorenzo Pieralisi's suggestion
> 
> Change from v5 to v7
> - change comment rid to RID
> - some mini change according to mani's feedback
> 
> Change from v4 to v5
> - rework commt message
> - add comment for mutex
> - s/reqid/rid/
> - keep only one loop when enable lut
> - add warning when try to add duplicate rid
> - Replace hardcode 0xffff with IMX95_PE0_LUT_MASK
> - Fix some error message
> 
> Change from v3 to v4
> - Check target value at of_map_id().
> - of_node_put() for target.
> - add case for msi-map exist, but rid entry is not exist.
> 
> Change from v2 to v3
> - Use the "target" argument of of_map_id()
> - Check if rid already in lut table when enable device
> 
> change from v1 to v2
> - set callback to pci_host_bridge instead pci->ops.
> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 186 +++++++++++++++++++++++++++++++++-
>  1 file changed, 185 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index c8d5c90aa4d45..d325f4fb279c8 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -55,6 +55,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 {
> @@ -87,6 +103,7 @@ enum imx_pcie_variants {
>   * workaround suspend resume on some devices which are affected by this errata.
>   */
>  #define IMX_PCIE_FLAG_BROKEN_SUSPEND		BIT(9)
> +#define IMX_PCIE_FLAG_HAS_LUT			BIT(10)
>  
>  #define imx_check_flag(pci, val)	(pci->drvdata->flags & val)
>  
> @@ -139,6 +156,9 @@ struct imx_pcie {
>  	struct device		*pd_pcie_phy;
>  	struct phy		*phy;
>  	const struct imx_pcie_drvdata *drvdata;
> +
> +	/* Ensure that only one device's LUT is configured at any given time */
> +	struct mutex		lock;
>  };
>  
>  /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
> @@ -930,6 +950,162 @@ static void imx_pcie_stop_link(struct dw_pcie *pci)
>  	imx_pcie_ltssm_disable(dev);
>  }
>  
> +static int imx_pcie_add_lut(struct imx_pcie *imx_pcie, u16 rid, u8 sid)
> +{
> +	struct dw_pcie *pci = imx_pcie->pci;
> +	struct device *dev = pci->dev;
> +	u32 data1, data2;
> +	int free = -1;
> +	int i;
> +
> +	if (sid >= 64) {
> +		dev_err(dev, "Invalid SID for index %d\n", sid);
> +		return -EINVAL;
> +	}
> +
> +	guard(mutex)(&imx_pcie->lock);
> +
> +	/*
> +	 * Iterate through all LUT entries to check for duplicate RID and
> +	 * identify the first available entry. Configure this available entry
> +	 * immediately after verification to avoid rescanning it.
> +	 */
> +	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)) {
> +			if (free < 0)
> +				free = i;
> +			continue;
> +		}
> +
> +		regmap_read(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_DATA2, &data2);
> +
> +		/* Do not add duplicate RID */
> +		if (rid == FIELD_GET(IMX95_PE0_LUT_REQID, data2)) {
> +			dev_warn(dev, "Existing LUT entry available for RID (%d)", rid);
> +			return 0;
> +		}
> +	}
> +
> +	if (free < 0) {
> +		dev_err(dev, "LUT entry is not available\n");
> +		return -ENOSPC;
> +	}
> +
> +	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 = IMX95_PE0_LUT_MASK; /* Match all bits of RID */
> +	data2 |= FIELD_PREP(IMX95_PE0_LUT_REQID, rid);
> +	regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_DATA2, data2);
> +
> +	regmap_write(imx_pcie->iomuxc_gpr, IMX95_PE0_LUT_ACSCTRL, free);
> +
> +	return 0;
> +}
> +
> +static void imx_pcie_remove_lut(struct imx_pcie *imx_pcie, u16 rid)
> +{
> +	u32 data2;
> +	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) == rid) {
> +			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);
> +
> +			break;
> +		}
> +	}
> +}
> +
> +static int imx_pcie_enable_device(struct pci_host_bridge *bridge, struct pci_dev *pdev)
> +{
> +	struct imx_pcie *imx_pcie = to_imx_pcie(to_dw_pcie_from_pp(bridge->sysdata));
> +	u32 sid_i, sid_m, rid = pci_dev_id(pdev);
> +	struct device_node *target;
> +	struct device *dev;
> +	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 ?

Could you point me at a sample dts for this host bridge please ?

> +		 * 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.

> +		 */
> +		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" ?

> +		 * ID, so mask this 2bits to get stream ID.
> +		 * But IOMMU glue layer doesn't do that.

and "IOMMU glue layer" ?

> +		 */
> +		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)

> +	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
> 

Re: [PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for i.MX95

[Frank Li]

On Thu, Dec 12, 2024 at 05:46:09PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Dec 10, 2024 at 05:48:59PM -0500, 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 checking msi-map and iommu-map device tree properties 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.
>
> This is wrong information. What you want to say is that if an msi-map
> isn't detected this means that the platform relies on DWC built-in
> controller for MSIs (that does not need streamIDs handling).
>
> That's quite different from what you are writing here.

How about ?

"If an msi-map isn't detected, platform relies on DWC built-in controller
for MSIs that does not need streamdIDs"

>
> >
> > 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@nxp.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.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
> >

Re: [PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for i.MX95

[Lorenzo Pieralisi]

On Thu, Dec 12, 2024 at 12:29:16PM -0500, Frank Li wrote:
> On Thu, Dec 12, 2024 at 05:46:09PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Dec 10, 2024 at 05:48:59PM -0500, 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 checking msi-map and iommu-map device tree properties 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.
> >
> > This is wrong information. What you want to say is that if an msi-map
> > isn't detected this means that the platform relies on DWC built-in
> > controller for MSIs (that does not need streamIDs handling).
> >
> > That's quite different from what you are writing here.
> 
> How about ?
> 
> "If an msi-map isn't detected, platform relies on DWC built-in controller
> for MSIs that does not need streamdIDs"

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 ?

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@nxp.com>
> > > Signed-off-by: Frank Li <Frank.Li@nxp.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
> > >

Re: [PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for i.MX95

[Frank Li]

On Fri, Dec 13, 2024 at 10:32:28AM +0100, Lorenzo Pieralisi wrote:
> On Thu, Dec 12, 2024 at 12:29:16PM -0500, Frank Li wrote:
> > On Thu, Dec 12, 2024 at 05:46:09PM +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Dec 10, 2024 at 05:48:59PM -0500, 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 checking msi-map and iommu-map device tree properties 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.
> > >
> > > This is wrong information. What you want to say is that if an msi-map
> > > isn't detected this means that the platform relies on DWC built-in
> > > controller for MSIs (that does not need streamIDs handling).
> > >
> > > That's quite different from what you are writing here.
> >
> > How about ?
> >
> > "If an msi-map isn't detected, platform relies on DWC built-in controller
> > for MSIs that does not need streamdIDs"
>
> 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.

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.

MSI:
case 1:
	msi-controller {
		status = "disabled";
	}
	Whole all dwc drivers will be broken.

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@nxp.com>
> > > > Signed-off-by: Frank Li <Frank.Li@nxp.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
> > > >

Re: [PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for i.MX95

[Lorenzo Pieralisi]

On Fri, Dec 13, 2024 at 12:20:40PM -0500, Frank Li wrote:
> On Fri, Dec 13, 2024 at 10:32:28AM +0100, Lorenzo Pieralisi wrote:
> > On Thu, Dec 12, 2024 at 12:29:16PM -0500, Frank Li wrote:
> > > On Thu, Dec 12, 2024 at 05:46:09PM +0100, Lorenzo Pieralisi wrote:
> > > > On Tue, Dec 10, 2024 at 05:48:59PM -0500, 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 checking msi-map and iommu-map device tree properties 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.
> > > >
> > > > This is wrong information. What you want to say is that if an msi-map
> > > > isn't detected this means that the platform relies on DWC built-in
> > > > controller for MSIs (that does not need streamIDs handling).
> > > >
> > > > That's quite different from what you are writing here.
> > >
> > > How about ?
> > >
> > > "If an msi-map isn't detected, platform relies on DWC built-in controller
> > > for MSIs that does not need streamdIDs"
> >
> > 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 ? That does not look like a false alarm to me.

> 
> 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.

> 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.

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@nxp.com>
> > > > > Signed-off-by: Frank Li <Frank.Li@nxp.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
> > > > >

Re: [PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for i.MX95

[Frank Li]

On Tue, Dec 17, 2024 at 10:25:59AM +0100, Lorenzo Pieralisi wrote:
> On Fri, Dec 13, 2024 at 12:20:40PM -0500, Frank Li wrote:
> > On Fri, Dec 13, 2024 at 10:32:28AM +0100, Lorenzo Pieralisi wrote:
> > > On Thu, Dec 12, 2024 at 12:29:16PM -0500, Frank Li wrote:
> > > > On Thu, Dec 12, 2024 at 05:46:09PM +0100, Lorenzo Pieralisi wrote:
> > > > > On Tue, Dec 10, 2024 at 05:48:59PM -0500, 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 checking msi-map and iommu-map device tree properties 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.
> > > > >
> > > > > This is wrong information. What you want to say is that if an msi-map
> > > > > isn't detected this means that the platform relies on DWC built-in
> > > > > controller for MSIs (that does not need streamIDs handling).
> > > > >
> > > > > That's quite different from what you are writing here.
> > > >
> > > > How about ?
> > > >
> > > > "If an msi-map isn't detected, platform relies on DWC built-in controller
> > > > for MSIs that does not need streamdIDs"
> > >
> > > 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.

> 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@nxp.com>
> > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.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
> > > > > >

Re: [PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for i.MX95

[Lorenzo Pieralisi]

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.

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@nxp.com>
> > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.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
> > > > > > >

Re: [PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for i.MX95

[Frank Li]

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@nxp.com>
> > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.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
> > > > > > > >

Re: [PATCH v8 2/2] PCI: imx6: Add IOMMU and ITS MSI support for i.MX95

[Rob Herring]

On Tue, Jan 07, 2025 at 12:03:39PM -0500, Frank Li wrote:
> 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?

My objection was only parsing the property yourself rather than using 
existing functions. So it looks fine to me now. Though you might 
consider if there is something to be shared with QCom driver.

Rob