diff mbox series

[v9,05/16] PCI: imx6: Using "linux,pci-domain" as slot ID

Message ID 20240119171122.3057511-6-Frank.Li@nxp.com
State New
Headers show
Series PCI: imx6: Clean up and add imx95 pci support | expand

Commit Message

Frank Li Jan. 19, 2024, 5:11 p.m. UTC 
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;
	...

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---

Notes:
    Change from v7 to v8
    - fixed comments
    - Added Manivannan Sadhasivam's review tag
    Change from v5 to v7
    - none
    Change from v3 to v4
    - remove compare basic address logic
    Change from v2 to v3
    - none
    Change from v1 to v2
    - fix of_get_pci_domain_nr return value check logic

 drivers/pci/controller/dwc/pci-imx6.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Comments

Lorenzo Pieralisi Feb. 2, 2024, 6:53 p.m. UTC | #1 
On Fri, Feb 02, 2024 at 08:25:58AM -0500, Frank Li wrote:
> On Fri, Feb 02, 2024 at 01:12:41PM +0100, Lorenzo Pieralisi wrote:
> > "PCI: imx6: Use "linux,pci-domain" as slot ID"
> > 
> > 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;
> > 
> > No it is not unless you magically update all firmware out
> > there in the field.
> 
> commit c0b70f05c87f3b09b391027c6f056d0facf331ef
> Author:     Peng Fan <peng.fan@nxp.com>
> AuthorDate: Fri Jan 15 11:26:57 2021 +0800
> Commit:     Shawn Guo <shawnguo@kernel.org>
> CommitDate: Fri Jan 29 14:46:28 2021 +0800
> 
> I am not sure if it is neccesary to compatible with 2 years ago's dts.
> I think it may not boot at all with using 2 year agao dtb file directly.
> 
> Do you have other comments? I can remove it from this big patch series,

I will have a look at the full series first we can decide whether
to drop it later.

I am travelling so I am not sure I can review it before February
12th, FYI.

Lorenzo
Bjorn Helgaas Feb. 2, 2024, 9:54 p.m. UTC | #2 
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.

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?

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

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

Bjorn
Frank Li Feb. 2, 2024, 10:22 p.m. UTC | #3 
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
Bjorn Helgaas Feb. 2, 2024, 10:51 p.m. UTC | #4 
[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 :)

> 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
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index eda6bc6ef80ee..773411d20329f 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -33,6 +33,7 @@ 
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 
+#include "../../pci.h"
 #include "pcie-designware.h"
 
 #define IMX8MQ_GPR_PCIE_REF_USE_PAD		BIT(9)
@@ -40,7 +41,6 @@ 
 #define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE	BIT(11)
 #define IMX8MQ_GPR_PCIE_VREG_BYPASS		BIT(12)
 #define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE	GENMASK(11, 8)
-#define IMX8MQ_PCIE2_BASE_ADDR			0x33c00000
 
 #define to_imx6_pcie(x)	dev_get_drvdata((x)->dev)
 
@@ -1279,13 +1279,17 @@  static int imx6_pcie_probe(struct platform_device *pdev)
 					     "Failed to get PCIEPHY reset control\n");
 	}
 
-	switch (imx6_pcie->drvdata->variant) {
-	case IMX7D:
-		if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
-			imx6_pcie->controller_id = 1;
-	default:
-		break;
-	}
+	/* 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");
 
 	/* Grab turnoff reset */
 	imx6_pcie->turnoff_reset = devm_reset_control_get_optional_exclusive(dev, "turnoff");