| Message ID | 20231206155903.566194-5-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: imx6: Clean up and add imx95 pci support | expand |
In subject, maybe you mean "Use 'linux,pci-domain' as slot ID"? "Using" is the wrong verb form here. On Wed, Dec 06, 2023 at 10:58:58AM -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. But this doesn't say anything about "linux,pci-domain", and I don't see anything about a register physical address in the patch. Maybe this commit log was meant for a different patch? I'm confused. > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 62d77fabd82a..239ef439ba70 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) > @@ -1333,6 +1334,11 @@ static int imx6_pcie_probe(struct platform_device *pdev) > "Failed to get PCIEPHY reset control\n"); > } > > + /* Using linux,pci-domain as PCI slot id */ > + imx6_pcie->controller_id = of_get_pci_domain_nr(node); > + if (imx6_pcie->controller_id) > + imx6_pcie->controller_id = 0; I don't understand what this is doing. It looks the same as just: imx6_pcie->controller_id = 0; Maybe this is a typo? As written, it doesn't look like there's any point in calling of_get_pci_domain_nr(). > switch (imx6_pcie->drvdata->variant) { > case IMX7D: > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > -- > 2.34.1 >
On Wed, Dec 06, 2023 at 10:36:56AM -0600, Bjorn Helgaas wrote: > In subject, maybe you mean "Use 'linux,pci-domain' as slot ID"? > "Using" is the wrong verb form here. > > On Wed, Dec 06, 2023 at 10:58:58AM -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. > > But this doesn't say anything about "linux,pci-domain", and I don't > see anything about a register physical address in the patch. > > Maybe this commit log was meant for a different patch? I'm confused. > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > > index 62d77fabd82a..239ef439ba70 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) > > @@ -1333,6 +1334,11 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > "Failed to get PCIEPHY reset control\n"); > > } > > > > + /* Using linux,pci-domain as PCI slot id */ > > + imx6_pcie->controller_id = of_get_pci_domain_nr(node); > > + if (imx6_pcie->controller_id) > > + imx6_pcie->controller_id = 0; > > I don't understand what this is doing. It looks the same as just: Good capture. It should be if (imx6_pcie->controller_id < 0) imx6_pcie->controller_id = 0; for only one PCI controller case. I just tested first one slot before send patch, so not met problem. Previously, we use below logic if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) imx6_pcie->controller_id = 1; It is not good to depend on register's base address. If there are 3 controllers, check logic will becomoe ugly. Frank > > imx6_pcie->controller_id = 0; > > Maybe this is a typo? As written, it doesn't look like there's any > point in calling of_get_pci_domain_nr(). > > > switch (imx6_pcie->drvdata->variant) { > > case IMX7D: > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > > -- > > 2.34.1 > >
On Wed, Dec 06, 2023 at 11:50:16AM -0500, Frank Li wrote: > On Wed, Dec 06, 2023 at 10:36:56AM -0600, Bjorn Helgaas wrote: > > In subject, maybe you mean "Use 'linux,pci-domain' as slot ID"? > > "Using" is the wrong verb form here. > > > > On Wed, Dec 06, 2023 at 10:58:58AM -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. > > > > But this doesn't say anything about "linux,pci-domain", and I don't > > see anything about a register physical address in the patch. > > > > Maybe this commit log was meant for a different patch? I'm confused. > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > --- > > > drivers/pci/controller/dwc/pci-imx6.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > > > index 62d77fabd82a..239ef439ba70 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) > > > @@ -1333,6 +1334,11 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > > "Failed to get PCIEPHY reset control\n"); > > > } > > > > > > + /* Using linux,pci-domain as PCI slot id */ > > > + imx6_pcie->controller_id = of_get_pci_domain_nr(node); > > > + if (imx6_pcie->controller_id) > > > + imx6_pcie->controller_id = 0; > > > > I don't understand what this is doing. It looks the same as just: > > Good capture. It should be > if (imx6_pcie->controller_id < 0) > imx6_pcie->controller_id = 0; > > for only one PCI controller case. I just tested first one slot before send > patch, so not met problem. > > Previously, we use below logic > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > imx6_pcie->controller_id = 1; > > It is not good to depend on register's base address. If there are 3 > controllers, check logic will becomoe ugly. Makes sense. If the previous code depended on the base address, this patch would make more sense if it contained both the addition of the of_get_pci_domain_nr() call and the removal of the base address code. > > Maybe this is a typo? As written, it doesn't look like there's any > > point in calling of_get_pci_domain_nr(). > > > > > switch (imx6_pcie->drvdata->variant) { > > > case IMX7D: > > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > > > -- > > > 2.34.1 > > >
On Wed, Dec 06, 2023 at 10:59:53AM -0600, Bjorn Helgaas wrote: > On Wed, Dec 06, 2023 at 11:50:16AM -0500, Frank Li wrote: > > On Wed, Dec 06, 2023 at 10:36:56AM -0600, Bjorn Helgaas wrote: > > > In subject, maybe you mean "Use 'linux,pci-domain' as slot ID"? > > > "Using" is the wrong verb form here. > > > > > > On Wed, Dec 06, 2023 at 10:58:58AM -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. > > > > > > But this doesn't say anything about "linux,pci-domain", and I don't > > > see anything about a register physical address in the patch. > > > > > > Maybe this commit log was meant for a different patch? I'm confused. > > > > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > > > --- > > > > drivers/pci/controller/dwc/pci-imx6.c | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > > > > index 62d77fabd82a..239ef439ba70 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) > > > > @@ -1333,6 +1334,11 @@ static int imx6_pcie_probe(struct platform_device *pdev) > > > > "Failed to get PCIEPHY reset control\n"); > > > > } > > > > > > > > + /* Using linux,pci-domain as PCI slot id */ > > > > + imx6_pcie->controller_id = of_get_pci_domain_nr(node); > > > > + if (imx6_pcie->controller_id) > > > > + imx6_pcie->controller_id = 0; > > > > > > I don't understand what this is doing. It looks the same as just: > > > > Good capture. It should be > > if (imx6_pcie->controller_id < 0) > > imx6_pcie->controller_id = 0; > > > > for only one PCI controller case. I just tested first one slot before send > > patch, so not met problem. > > > > Previously, we use below logic > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > > imx6_pcie->controller_id = 1; > > > > It is not good to depend on register's base address. If there are 3 > > controllers, check logic will becomoe ugly. > > Makes sense. If the previous code depended on the base address, this > patch would make more sense if it contained both the addition of the > of_get_pci_domain_nr() call and the removal of the base address code. Remove base address code will block existed functions. My plan is 1. this patch upstreamed 2. related dts add linux,pci-domain. 3. removed base address compare code. Frank > > > > Maybe this is a typo? As written, it doesn't look like there's any > > > point in calling of_get_pci_domain_nr(). > > > > > > > switch (imx6_pcie->drvdata->variant) { > > > > case IMX7D: > > > > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > > > > -- > > > > 2.34.1 > > > >
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 62d77fabd82a..239ef439ba70 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) @@ -1333,6 +1334,11 @@ static int imx6_pcie_probe(struct platform_device *pdev) "Failed to get PCIEPHY reset control\n"); } + /* Using linux,pci-domain as PCI slot id */ + imx6_pcie->controller_id = of_get_pci_domain_nr(node); + if (imx6_pcie->controller_id) + imx6_pcie->controller_id = 0; + switch (imx6_pcie->drvdata->variant) { case IMX7D: if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
Avoid use get slot id by compared with register physical address. If there are more than 2 slots, compared logic will become complex. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/pci/controller/dwc/pci-imx6.c | 6 ++++++ 1 file changed, 6 insertions(+)