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: > > 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/ https://lore.kernel.org/imx/20231217175158.GF6748@thinkpad/ > > I expect the "compatible" string to tell the driver what the > programming model of the device is. > > > + /* 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
