On Fri, Feb 02, 2024 at 03:54:31PM -0600, Bjorn Helgaas wrote:

On Fri, Jan 19, 2024 at 12:11:11PM -0500, Frank Li wrote:

quoted

Avoid use get slot id by compared with register physical address. If there
are more than 2 slots, compared logic will become complex.

"linux,pci-domain" already exist at dts since commit:
	commit (c0b70f05c87f3b arm64: dts: imx8mq: use_dt_domains for pci node).

So it is safe to remove compare basic address code:
	...
	if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
		imx6_pcie->controller_id = 1;
	...

I have no idea what this is telling me.  I guess you don't want to use
IMX8MQ_PCIE2_BASE_ADDR to decide something?  That much sounds good:
the *address* of some MMIO space doesn't tell us anything about the
function of that space.

You are right. If there are more than two controller. The check logic
will be extremely complex.

There are some discussin at below thread about linux,pci-domain
https://lore.kernel.org/imx/20231206165953.GA717921@bhelgaas/ (local)

https://lore.kernel.org/imx/20231217175158.GF6748@thinkpad/ (local)

I expect the "compatible" string to tell the driver what the
programming model of the device is.

quoted

+	/* Using linux,pci-domain as PCI slot id */
+	imx6_pcie->controller_id = of_get_pci_domain_nr(node);
+	/*
+	 * If there are no "linux,pci-domain" property specified in DT, then assume only one
+	 * controller is available.
+	 */
+	if (imx6_pcie->controller_id == -EINVAL)
+		imx6_pcie->controller_id = 0;
+	else if (imx6_pcie->controller_id < 0)
+		return dev_err_probe(dev, imx6_pcie->controller_id,
+				     "linux,pci-domain have wrong value\n");

Maybe I'm missing something here.  It looks like this driver uses
controller_id to distinguish between hardware variants or maybe
between two Root Ports (slots?) in the same SoC?

Yes!

  imx6_pcie_grp_offset
    return imx6_pcie->controller_id == 1 ? IOMUXC_GPR16 : IOMUXC_GPR14;

  imx6_pcie_configure_type
    id = imx6_pcie->controller_id
    if (!drvdata->mode_mask[id])         # <-- looks unsafe

I can add safe check here.

      id = 0;
    regmap_update_bits(drvdata->mode_off[id], ...)

(This "mode_mask[id]" looks like it will reference garbage if the DT
supplies "linux,pci-domain = <2>".  A bogus DT shouldn't be able to
cause a driver to misbehave like that.)

Suppose I can use dt-bind doc to force to 0,1 and safe check here.

That doesn't seem related to "linux,pci-domain" at all.

I added comments about
/* Using linux,pci-domain as PCI slot id */

We may add new property about controller-id, but there already have common
one "linux,pci-domain", which value in upstreamed dts exactly match our
expection, I also found other platform use it as slot id in kernel tree.

Any way, we can continue discuss the better solution here. But I hope
it was not block whole 16 patches. we can skip this one firstly.

I still have more than 10 clean up patches my local tree.

Bjorn