| Message ID | 20190325083501.8088-7-kishon@ti.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add support for PCIe RC and EP mode in TI's AM654 SoC | expand |
On Mon, Mar 25, 2019 at 02:04:41PM +0530, Kishon Vijay Abraham I wrote: > No functional change. Move host specific platform_get_resource to > ks_add_pcie_port and the common platform_get_resource (applicable > to both host and endpoint) to probe. This is in preparation for > adding endpoint support to pci-keystone driver. This seems to move platform_get_resource() *from* (not *to*) ks_add_pcie_port(). You seem to be making a distinction in the commit log between (1) a resource that's only used for host mode, and (2) a common resource that's used for both host and endpoint mode. But I don't see that distinction in the patch, so it's a little confusing to mention it in the commit log. It must make endpoint support easier, but I can't quite connect the dots yet. Maybe endpoint will also use ks_pcie_add_pcie_port(), but will have a separate .probe() function that doesn't look up the resource that's specific to host mode? > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/pci/controller/dwc/pci-keystone.c | 27 +++++++++++++---------- > 1 file changed, 15 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index 5eebef9b9ada..95997885a05c 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -806,11 +806,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, > struct resource *res; > int ret; > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbics"); > - pci->dbi_base = devm_pci_remap_cfg_resource(dev, res); > - if (IS_ERR(pci->dbi_base)) > - return PTR_ERR(pci->dbi_base); > - > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config"); > pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res); > if (IS_ERR(pp->va_cfg0_base)) > @@ -818,13 +813,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, > > pp->va_cfg1_base = pp->va_cfg0_base; > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); > - ks_pcie->va_app_base = devm_ioremap_resource(dev, res); > - if (IS_ERR(ks_pcie->va_app_base)) > - return PTR_ERR(ks_pcie->va_app_base); > - > - ks_pcie->app = *res; > - > pp->ops = &ks_pcie_host_ops; > ret = dw_pcie_host_init(pp); > if (ret) { > @@ -895,6 +883,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > struct dw_pcie *pci; > struct keystone_pcie *ks_pcie; > struct device_link **link; > + struct resource *res; > + void __iomem *base; > u32 num_viewport; > struct phy **phy; > u32 num_lanes; > @@ -911,6 +901,19 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > if (!pci) > return -ENOMEM; > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); > + ks_pcie->va_app_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(ks_pcie->va_app_base)) > + return PTR_ERR(ks_pcie->va_app_base); > + > + ks_pcie->app = *res; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbics"); > + base = devm_pci_remap_cfg_resource(dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + pci->dbi_base = base; > pci->dev = dev; > pci->ops = &ks_pcie_dw_pcie_ops; > > -- > 2.17.1 >
Hi Bjorn, On 13/04/19 8:00 PM, Bjorn Helgaas wrote: > On Mon, Mar 25, 2019 at 02:04:41PM +0530, Kishon Vijay Abraham I wrote: >> No functional change. Move host specific platform_get_resource to >> ks_add_pcie_port and the common platform_get_resource (applicable >> to both host and endpoint) to probe. This is in preparation for >> adding endpoint support to pci-keystone driver. > > This seems to move platform_get_resource() *from* (not *to*) > ks_add_pcie_port(). Maybe I should have mentioned "Keep host specific platform_get_resource() in ks_add_pcie_port() and move the common platform_get_resource() (applicable to both host and endpoint) to probe()" > > You seem to be making a distinction in the commit log between (1) a > resource that's only used for host mode, and (2) a common resource > that's used for both host and endpoint mode. But I don't see that > distinction in the patch, so it's a little confusing to mention it in > the commit log. > > It must make endpoint support easier, but I can't quite connect the > dots yet. Maybe endpoint will also use ks_pcie_add_pcie_port(), but > will have a separate .probe() function that doesn't look up the > resource that's specific to host mode? No ks_pcie_add_pcie_port() is specific to host mode, so "config" resource is kept there whereas "dbics" and "app" resources are common to both host mode and device mode, so they are moved to probe(). Thanks Kishon > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> drivers/pci/controller/dwc/pci-keystone.c | 27 +++++++++++++---------- >> 1 file changed, 15 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c >> index 5eebef9b9ada..95997885a05c 100644 >> --- a/drivers/pci/controller/dwc/pci-keystone.c >> +++ b/drivers/pci/controller/dwc/pci-keystone.c >> @@ -806,11 +806,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, >> struct resource *res; >> int ret; >> >> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbics"); >> - pci->dbi_base = devm_pci_remap_cfg_resource(dev, res); >> - if (IS_ERR(pci->dbi_base)) >> - return PTR_ERR(pci->dbi_base); >> - >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config"); >> pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res); >> if (IS_ERR(pp->va_cfg0_base)) >> @@ -818,13 +813,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, >> >> pp->va_cfg1_base = pp->va_cfg0_base; >> >> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); >> - ks_pcie->va_app_base = devm_ioremap_resource(dev, res); >> - if (IS_ERR(ks_pcie->va_app_base)) >> - return PTR_ERR(ks_pcie->va_app_base); >> - >> - ks_pcie->app = *res; >> - >> pp->ops = &ks_pcie_host_ops; >> ret = dw_pcie_host_init(pp); >> if (ret) { >> @@ -895,6 +883,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev) >> struct dw_pcie *pci; >> struct keystone_pcie *ks_pcie; >> struct device_link **link; >> + struct resource *res; >> + void __iomem *base; >> u32 num_viewport; >> struct phy **phy; >> u32 num_lanes; >> @@ -911,6 +901,19 @@ static int __init ks_pcie_probe(struct platform_device *pdev) >> if (!pci) >> return -ENOMEM; >> >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); >> + ks_pcie->va_app_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(ks_pcie->va_app_base)) >> + return PTR_ERR(ks_pcie->va_app_base); >> + >> + ks_pcie->app = *res; >> + >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbics"); >> + base = devm_pci_remap_cfg_resource(dev, res); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> + >> + pci->dbi_base = base; >> pci->dev = dev; >> pci->ops = &ks_pcie_dw_pcie_ops; >> >> -- >> 2.17.1 >>
On Mon, Apr 15, 2019 at 11:04:10AM +0530, Kishon Vijay Abraham I wrote: > Hi Bjorn, > > On 13/04/19 8:00 PM, Bjorn Helgaas wrote: > > On Mon, Mar 25, 2019 at 02:04:41PM +0530, Kishon Vijay Abraham I wrote: > >> No functional change. Move host specific platform_get_resource to > >> ks_add_pcie_port and the common platform_get_resource (applicable > >> to both host and endpoint) to probe. This is in preparation for > >> adding endpoint support to pci-keystone driver. > > > > This seems to move platform_get_resource() *from* (not *to*) > > ks_add_pcie_port(). > > Maybe I should have mentioned "Keep host specific platform_get_resource() in > ks_add_pcie_port() and move the common platform_get_resource() (applicable > to both host and endpoint) to probe()" Commit log updated, pushed out pci/keystone again. Thanks, Lorenzo > > You seem to be making a distinction in the commit log between (1) a > > resource that's only used for host mode, and (2) a common resource > > that's used for both host and endpoint mode. But I don't see that > > distinction in the patch, so it's a little confusing to mention it in > > the commit log. > > > > It must make endpoint support easier, but I can't quite connect the > > dots yet. Maybe endpoint will also use ks_pcie_add_pcie_port(), but > > will have a separate .probe() function that doesn't look up the > > resource that's specific to host mode? > > No ks_pcie_add_pcie_port() is specific to host mode, so "config" resource is > kept there whereas "dbics" and "app" resources are common to both host mode and > device mode, so they are moved to probe(). > > Thanks > Kishon > > > > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >> --- > >> drivers/pci/controller/dwc/pci-keystone.c | 27 +++++++++++++---------- > >> 1 file changed, 15 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > >> index 5eebef9b9ada..95997885a05c 100644 > >> --- a/drivers/pci/controller/dwc/pci-keystone.c > >> +++ b/drivers/pci/controller/dwc/pci-keystone.c > >> @@ -806,11 +806,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, > >> struct resource *res; > >> int ret; > >> > >> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbics"); > >> - pci->dbi_base = devm_pci_remap_cfg_resource(dev, res); > >> - if (IS_ERR(pci->dbi_base)) > >> - return PTR_ERR(pci->dbi_base); > >> - > >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config"); > >> pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res); > >> if (IS_ERR(pp->va_cfg0_base)) > >> @@ -818,13 +813,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, > >> > >> pp->va_cfg1_base = pp->va_cfg0_base; > >> > >> - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); > >> - ks_pcie->va_app_base = devm_ioremap_resource(dev, res); > >> - if (IS_ERR(ks_pcie->va_app_base)) > >> - return PTR_ERR(ks_pcie->va_app_base); > >> - > >> - ks_pcie->app = *res; > >> - > >> pp->ops = &ks_pcie_host_ops; > >> ret = dw_pcie_host_init(pp); > >> if (ret) { > >> @@ -895,6 +883,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > >> struct dw_pcie *pci; > >> struct keystone_pcie *ks_pcie; > >> struct device_link **link; > >> + struct resource *res; > >> + void __iomem *base; > >> u32 num_viewport; > >> struct phy **phy; > >> u32 num_lanes; > >> @@ -911,6 +901,19 @@ static int __init ks_pcie_probe(struct platform_device *pdev) > >> if (!pci) > >> return -ENOMEM; > >> > >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); > >> + ks_pcie->va_app_base = devm_ioremap_resource(dev, res); > >> + if (IS_ERR(ks_pcie->va_app_base)) > >> + return PTR_ERR(ks_pcie->va_app_base); > >> + > >> + ks_pcie->app = *res; > >> + > >> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbics"); > >> + base = devm_pci_remap_cfg_resource(dev, res); > >> + if (IS_ERR(base)) > >> + return PTR_ERR(base); > >> + > >> + pci->dbi_base = base; > >> pci->dev = dev; > >> pci->ops = &ks_pcie_dw_pcie_ops; > >> > >> -- > >> 2.17.1 > >>
diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index 5eebef9b9ada..95997885a05c 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -806,11 +806,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, struct resource *res; int ret; - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbics"); - pci->dbi_base = devm_pci_remap_cfg_resource(dev, res); - if (IS_ERR(pci->dbi_base)) - return PTR_ERR(pci->dbi_base); - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config"); pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res); if (IS_ERR(pp->va_cfg0_base)) @@ -818,13 +813,6 @@ static int __init ks_pcie_add_pcie_port(struct keystone_pcie *ks_pcie, pp->va_cfg1_base = pp->va_cfg0_base; - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); - ks_pcie->va_app_base = devm_ioremap_resource(dev, res); - if (IS_ERR(ks_pcie->va_app_base)) - return PTR_ERR(ks_pcie->va_app_base); - - ks_pcie->app = *res; - pp->ops = &ks_pcie_host_ops; ret = dw_pcie_host_init(pp); if (ret) { @@ -895,6 +883,8 @@ static int __init ks_pcie_probe(struct platform_device *pdev) struct dw_pcie *pci; struct keystone_pcie *ks_pcie; struct device_link **link; + struct resource *res; + void __iomem *base; u32 num_viewport; struct phy **phy; u32 num_lanes; @@ -911,6 +901,19 @@ static int __init ks_pcie_probe(struct platform_device *pdev) if (!pci) return -ENOMEM; + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "app"); + ks_pcie->va_app_base = devm_ioremap_resource(dev, res); + if (IS_ERR(ks_pcie->va_app_base)) + return PTR_ERR(ks_pcie->va_app_base); + + ks_pcie->app = *res; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbics"); + base = devm_pci_remap_cfg_resource(dev, res); + if (IS_ERR(base)) + return PTR_ERR(base); + + pci->dbi_base = base; pci->dev = dev; pci->ops = &ks_pcie_dw_pcie_ops;
No functional change. Move host specific platform_get_resource to ks_add_pcie_port and the common platform_get_resource (applicable to both host and endpoint) to probe. This is in preparation for adding endpoint support to pci-keystone driver. Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- drivers/pci/controller/dwc/pci-keystone.c | 27 +++++++++++++---------- 1 file changed, 15 insertions(+), 12 deletions(-)