On Fri, Feb 02, 2024 at 04:51:16PM -0600, Bjorn Helgaas wrote: > [Rob to to: line] > > On Fri, Feb 02, 2024 at 05:22:07PM -0500, Frank Li wrote: > > 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/ > > My response here was too low level, just about trivial syntactic and > style issues. I should have seen the larger issue at the time; sorry > about that. > > > https://lore.kernel.org/imx/20231217175158.GF6748@thinkpad/ > > That's a good response from Mani, but again not relevant to my point. > > My point here is that "compatible" should tell the driver how to > operate the device, i.e., the driver knows what registers are present > and how they work. > > If you have two variant devices that both implement a register that > can be used to distinguish them, a single "compatible" string might be > enough because the driver can use that register to tell the > difference. > > If the driver can't tell the difference by looking at the hardware > itself, I think you need a separate "compatible" string for it. Of > course I'm far from a DT expert, so please correct this if necessary, > Rob, et al. > > > > 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. > > Nope. The driver must protect itself from garbage in the DT. > > > > That doesn't seem related to "linux,pci-domain" at all. > > > > I added comments about > > /* Using linux,pci-domain as PCI slot id */ > > That doesn't make it related :) Okay, linux,pci-domain is not good method for this. Anyways, previous implement is wrong. Let me skip it and think a better method to fix this problem later. Frank > > > 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
