| Message ID | 1438183681-30519-1-git-send-email-jchandra@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 29, 2015 at 04:28:00PM +0100, Jayachandran C wrote: > The current code in pci-host-generic.c uses pci_common_init_dev() > from ARM platform to do some of the PCI initializations, and this > prevents it from being used in ARM64. > > The initialization done by pci_common_init_dev() that is needed > by pci-host-generic.c is really limited, and can be done easily > in the same file without using hw_pci API. The ARM platform > requires a pci_sys_data as sysdata for the PCI bus, this can be > handled by setting up gen_pci to have a pci_sys_data variable as > the first element. Ok, I still do not like leaving pci_sys_data there, and to remove it there is only one snag (+ one patch to prevent resources enablement on ARM64 PROBE_ONLY systems to have a consolidated ARM/ARM64 host generic), which consists in removing the align_resource pointer from pci_sys_data. A proposed solution (maybe not ideal, we can consider it a temporary step to make progress) here: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/359273.html Comments very appreciated on the patch above, I do not have major concerns with this patch other than the pci_sys_data hack, because that's what it is. Thanks, Lorenzo > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > --- > Here's v3 of the patchset. > > v2-v3 > - rebase to 4.2-rc > - fix PCI_PROBE_ONLY check before calling pcie configure > - added a comment above sysdata > - updated the commit message > > v1->v2 > - Address comments from Arnd Bergmann and Lorenzo Pieralisi > - move contents of gen_pci_init to gen_pci_probe > - assign resources only when !probe_only > - tested on ARM32 with qemu option -M virt > > Notes: > - passing a zeroed out pci_sys_data for ARM looks ok, but I haven't > tested it on ARM. > - tested it on ARM64 fast model > - Any information on how this can be tested on arm is welcome. > - There is only one ifdef, and that can be removed when arm64 gets > a sysdata, or when arm loses its sysdata. > > drivers/pci/host/pci-host-generic.c | 55 ++++++++++++++++++++++++------------- > 1 file changed, 36 insertions(+), 19 deletions(-) > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index ba46e58..e9d9c80 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -38,7 +38,15 @@ struct gen_pci_cfg_windows { > const struct gen_pci_cfg_bus_ops *ops; > }; > > +/* > + * ARM needs platform specific pci_sys_data as the sysdata for PCI. > + * We add the sys as the first field below to handle this. sys will > + * set to 0, so that the pci functions in do the right thing. > + */ > struct gen_pci { > +#ifdef CONFIG_ARM > + struct pci_sys_data sys; > +#endif > struct pci_host_bridge host; > struct gen_pci_cfg_windows cfg; > struct list_head resources; > @@ -48,8 +56,7 @@ static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, > unsigned int devfn, > int where) > { > - struct pci_sys_data *sys = bus->sysdata; > - struct gen_pci *pci = sys->private_data; > + struct gen_pci *pci = bus->sysdata; > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > return pci->cfg.win[idx] + ((devfn << 8) | where); > @@ -64,8 +71,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus, > unsigned int devfn, > int where) > { > - struct pci_sys_data *sys = bus->sysdata; > - struct gen_pci *pci = sys->private_data; > + struct gen_pci *pci = bus->sysdata; > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > return pci->cfg.win[idx] + ((devfn << 12) | where); > @@ -198,13 +204,6 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > return 0; > } > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > -{ > - struct gen_pci *pci = sys->private_data; > - list_splice_init(&pci->resources, &sys->resources); > - return 1; > -} > - > static int gen_pci_probe(struct platform_device *pdev) > { > int err; > @@ -214,13 +213,7 @@ static int gen_pci_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > - struct hw_pci hw = { > - .nr_controllers = 1, > - .private_data = (void **)&pci, > - .setup = gen_pci_setup, > - .map_irq = of_irq_parse_and_map_pci, > - .ops = &gen_pci_ops, > - }; > + struct pci_bus *bus; > > if (!pci) > return -ENOMEM; > @@ -258,7 +251,31 @@ static int gen_pci_probe(struct platform_device *pdev) > return err; > } > > - pci_common_init_dev(dev, &hw); > + /* do not reassign resource if probe only */ > + if (!pci_has_flag(PCI_PROBE_ONLY)) > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > + > + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); > + if (!bus) { > + dev_err(dev, "Scanning rootbus failed"); > + return -ENODEV; > + } > + > + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > + > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > + pci_bus_size_bridges(bus); > + pci_bus_assign_resources(bus); > + } > + pci_bus_add_devices(bus); > + > + /* Configure PCI Express settings */ > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > + struct pci_bus *child; > + > + list_for_each_entry(child, &bus->children, node) > + pcie_bus_configure_settings(child); > + } > return 0; > } > > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Thu, Jul 30, 2015 at 10:28:08AM +0100, Lorenzo Pieralisi wrote: > On Wed, Jul 29, 2015 at 04:28:00PM +0100, Jayachandran C wrote: > > The current code in pci-host-generic.c uses pci_common_init_dev() > > from ARM platform to do some of the PCI initializations, and this > > prevents it from being used in ARM64. > > > > The initialization done by pci_common_init_dev() that is needed > > by pci-host-generic.c is really limited, and can be done easily > > in the same file without using hw_pci API. The ARM platform > > requires a pci_sys_data as sysdata for the PCI bus, this can be > > handled by setting up gen_pci to have a pci_sys_data variable as > > the first element. > > Ok, I still do not like leaving pci_sys_data there, and to remove > it there is only one snag (+ one patch to prevent > resources enablement on ARM64 PROBE_ONLY systems to have a > consolidated ARM/ARM64 host generic), which consists in removing the > align_resource pointer from pci_sys_data. A proposed solution (maybe > not ideal, we can consider it a temporary step to make progress) > here: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/359273.html > > Comments very appreciated on the patch above, I do not have major > concerns with this patch other than the pci_sys_data hack, because > that's what it is. In may opinion, we have to leave ->sysdata on ARM pointing to a valid pci_sys_data. Leaving it pointing to random stuff and hoping that it will not be used is pretty bad - so I don't see why such a patch would make it upstream. The second issue I have is adding dependencies between patches that enable basic functionality on arm64 with arm infrastructure cleanup patches. Given the number of kernel releases the cleanups take [and factoring in the amount of mess arm has :)], we will end up waiting a long time for arm64. BTW, I am not sure why there is a pushback on wrapping a structure in another, container_of() is the linux way of doing things :) JC.
On Wed, Jul 29, 2015 at 04:28:00PM +0100, Jayachandran C wrote: > The current code in pci-host-generic.c uses pci_common_init_dev() > from ARM platform to do some of the PCI initializations, and this > prevents it from being used in ARM64. > > The initialization done by pci_common_init_dev() that is needed > by pci-host-generic.c is really limited, and can be done easily > in the same file without using hw_pci API. The ARM platform > requires a pci_sys_data as sysdata for the PCI bus, this can be > handled by setting up gen_pci to have a pci_sys_data variable as > the first element. For the sake of enabling it on ARM64, I am okay with getting this merged, keeping in mind that as I already said pci_sys_data has to go, it is a hack and I do not want it to spread to ALL PCI host drivers that have to work on both ARM/ARM64. Comments and tags below. > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > --- > Here's v3 of the patchset. > > v2-v3 > - rebase to 4.2-rc > - fix PCI_PROBE_ONLY check before calling pcie configure > - added a comment above sysdata > - updated the commit message > > v1->v2 > - Address comments from Arnd Bergmann and Lorenzo Pieralisi > - move contents of gen_pci_init to gen_pci_probe > - assign resources only when !probe_only > - tested on ARM32 with qemu option -M virt > > Notes: > - passing a zeroed out pci_sys_data for ARM looks ok, but I haven't > tested it on ARM. > - tested it on ARM64 fast model > - Any information on how this can be tested on arm is welcome. > - There is only one ifdef, and that can be removed when arm64 gets > a sysdata, or when arm loses its sysdata. > > drivers/pci/host/pci-host-generic.c | 55 ++++++++++++++++++++++++------------- > 1 file changed, 36 insertions(+), 19 deletions(-) > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index ba46e58..e9d9c80 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -38,7 +38,15 @@ struct gen_pci_cfg_windows { > const struct gen_pci_cfg_bus_ops *ops; > }; > > +/* > + * ARM needs platform specific pci_sys_data as the sysdata for PCI. > + * We add the sys as the first field below to handle this. sys will > + * set to 0, so that the pci functions in do the right thing. > + */ > struct gen_pci { > +#ifdef CONFIG_ARM > + struct pci_sys_data sys; > +#endif > struct pci_host_bridge host; > struct gen_pci_cfg_windows cfg; > struct list_head resources; > @@ -48,8 +56,7 @@ static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, > unsigned int devfn, > int where) > { > - struct pci_sys_data *sys = bus->sysdata; > - struct gen_pci *pci = sys->private_data; > + struct gen_pci *pci = bus->sysdata; > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > return pci->cfg.win[idx] + ((devfn << 8) | where); > @@ -64,8 +71,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus, > unsigned int devfn, > int where) > { > - struct pci_sys_data *sys = bus->sysdata; > - struct gen_pci *pci = sys->private_data; > + struct gen_pci *pci = bus->sysdata; > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > return pci->cfg.win[idx] + ((devfn << 12) | where); > @@ -198,13 +204,6 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > return 0; > } > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > -{ > - struct gen_pci *pci = sys->private_data; > - list_splice_init(&pci->resources, &sys->resources); > - return 1; > -} > - > static int gen_pci_probe(struct platform_device *pdev) > { > int err; > @@ -214,13 +213,7 @@ static int gen_pci_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > - struct hw_pci hw = { > - .nr_controllers = 1, > - .private_data = (void **)&pci, > - .setup = gen_pci_setup, > - .map_irq = of_irq_parse_and_map_pci, > - .ops = &gen_pci_ops, > - }; > + struct pci_bus *bus; > > if (!pci) > return -ENOMEM; > @@ -258,7 +251,31 @@ static int gen_pci_probe(struct platform_device *pdev) > return err; > } > > - pci_common_init_dev(dev, &hw); > + /* do not reassign resource if probe only */ > + if (!pci_has_flag(PCI_PROBE_ONLY)) > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); You should add PCI_REASSIGN_ALL_BUS here. As I said in patch 2 comment, please enable setup-irq.c in an explicit commit, re-sequence the series and fold the Kconfig ARM64 enablement in this patch. With those changes, for the patch series: Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Tested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > + > + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); > + if (!bus) { > + dev_err(dev, "Scanning rootbus failed"); > + return -ENODEV; > + } > + > + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > + > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > + pci_bus_size_bridges(bus); > + pci_bus_assign_resources(bus); > + } > + pci_bus_add_devices(bus); > + > + /* Configure PCI Express settings */ > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > + struct pci_bus *child; > + > + list_for_each_entry(child, &bus->children, node) > + pcie_bus_configure_settings(child); > + } > return 0; > } > > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Thu, Jul 30, 2015 at 11:13:52AM +0100, Jayachandran C. wrote: > On Thu, Jul 30, 2015 at 10:28:08AM +0100, Lorenzo Pieralisi wrote: > > On Wed, Jul 29, 2015 at 04:28:00PM +0100, Jayachandran C wrote: > > > The current code in pci-host-generic.c uses pci_common_init_dev() > > > from ARM platform to do some of the PCI initializations, and this > > > prevents it from being used in ARM64. > > > > > > The initialization done by pci_common_init_dev() that is needed > > > by pci-host-generic.c is really limited, and can be done easily > > > in the same file without using hw_pci API. The ARM platform > > > requires a pci_sys_data as sysdata for the PCI bus, this can be > > > handled by setting up gen_pci to have a pci_sys_data variable as > > > the first element. > > > > Ok, I still do not like leaving pci_sys_data there, and to remove > > it there is only one snag (+ one patch to prevent > > resources enablement on ARM64 PROBE_ONLY systems to have a > > consolidated ARM/ARM64 host generic), which consists in removing the > > align_resource pointer from pci_sys_data. A proposed solution (maybe > > not ideal, we can consider it a temporary step to make progress) > > here: > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/359273.html > > > > Comments very appreciated on the patch above, I do not have major > > concerns with this patch other than the pci_sys_data hack, because > > that's what it is. > > In may opinion, we have to leave ->sysdata on ARM pointing to a valid > pci_sys_data. Leaving it pointing to random stuff and hoping that it > will not be used is pretty bad - so I don't see why such a patch would > make it upstream. I really can't follow what you mean here, if we remove pci_sys_data dependency pci_sys_data will be an internal data structure for drivers still depending on bios32 to initialize hosts, it won't extend to all ARM PCI host controllers, so I struggle to see your point. > The second issue I have is adding dependencies between patches that > enable basic functionality on arm64 with arm infrastructure cleanup > patches. Given the number of kernel releases the cleanups take [and > factoring in the amount of mess arm has :)], we will end up waiting > a long time for arm64. I agree, that's the reason why I think it is time to merge your changes, but this does not mean that the consolidation stops here, I made an exception since this patch has been hanging in the balance for a while, but the clean-up has to be completed anyway and I will take care of that. > BTW, I am not sure why there is a pushback on wrapping a structure in > another, container_of() is the linux way of doing things :) I think you are aware of my opinion by now, no point stating it again. Lorenzo
On Wed, Jul 29, 2015 at 10:28 AM, Jayachandran C <jchandra@broadcom.com> wrote: > The current code in pci-host-generic.c uses pci_common_init_dev() > from ARM platform to do some of the PCI initializations, and this > prevents it from being used in ARM64. > > The initialization done by pci_common_init_dev() that is needed > by pci-host-generic.c is really limited, and can be done easily > in the same file without using hw_pci API. The ARM platform > requires a pci_sys_data as sysdata for the PCI bus, this can be > handled by setting up gen_pci to have a pci_sys_data variable as > the first element. > > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > --- > Here's v3 of the patchset. > > v2-v3 > - rebase to 4.2-rc > - fix PCI_PROBE_ONLY check before calling pcie configure > - added a comment above sysdata > - updated the commit message > > v1->v2 > - Address comments from Arnd Bergmann and Lorenzo Pieralisi > - move contents of gen_pci_init to gen_pci_probe > - assign resources only when !probe_only > - tested on ARM32 with qemu option -M virt > > Notes: > - passing a zeroed out pci_sys_data for ARM looks ok, but I haven't > tested it on ARM. It is tested on other drivers. > - tested it on ARM64 fast model > - Any information on how this can be tested on arm is welcome. > - There is only one ifdef, and that can be removed when arm64 gets > a sysdata, or when arm loses its sysdata. > [...] > @@ -258,7 +251,31 @@ static int gen_pci_probe(struct platform_device *pdev) > return err; > } > > - pci_common_init_dev(dev, &hw); > + /* do not reassign resource if probe only */ > + if (!pci_has_flag(PCI_PROBE_ONLY)) > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > + > + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); > + if (!bus) { > + dev_err(dev, "Scanning rootbus failed"); > + return -ENODEV; > + } > + > + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > + > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > + pci_bus_size_bridges(bus); > + pci_bus_assign_resources(bus); > + } > + pci_bus_add_devices(bus); > + > + /* Configure PCI Express settings */ > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > + struct pci_bus *child; > + > + list_for_each_entry(child, &bus->children, node) > + pcie_bus_configure_settings(child); > + } Can't we come up with a common function for all this hunk? Every host doesn't nearly the same thing and I'd guess the differences are omissions rather than necessary differences. Rob
On Thu, Jul 30, 2015 at 02:35:21PM +0100, Lorenzo Pieralisi wrote: > On Wed, Jul 29, 2015 at 04:28:00PM +0100, Jayachandran C wrote: > > The current code in pci-host-generic.c uses pci_common_init_dev() > > from ARM platform to do some of the PCI initializations, and this > > prevents it from being used in ARM64. > > > > The initialization done by pci_common_init_dev() that is needed > > by pci-host-generic.c is really limited, and can be done easily > > in the same file without using hw_pci API. The ARM platform > > requires a pci_sys_data as sysdata for the PCI bus, this can be > > handled by setting up gen_pci to have a pci_sys_data variable as > > the first element. > > For the sake of enabling it on ARM64, I am okay with getting > this merged, keeping in mind that as I already said pci_sys_data > has to go, it is a hack and I do not want it to spread to > ALL PCI host drivers that have to work on both ARM/ARM64. > > Comments and tags below. > > > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > > --- > > Here's v3 of the patchset. > > > > v2-v3 > > - rebase to 4.2-rc > > - fix PCI_PROBE_ONLY check before calling pcie configure > > - added a comment above sysdata > > - updated the commit message > > > > v1->v2 > > - Address comments from Arnd Bergmann and Lorenzo Pieralisi > > - move contents of gen_pci_init to gen_pci_probe > > - assign resources only when !probe_only > > - tested on ARM32 with qemu option -M virt > > > > Notes: > > - passing a zeroed out pci_sys_data for ARM looks ok, but I haven't > > tested it on ARM. > > - tested it on ARM64 fast model > > - Any information on how this can be tested on arm is welcome. > > - There is only one ifdef, and that can be removed when arm64 gets > > a sysdata, or when arm loses its sysdata. > > > > drivers/pci/host/pci-host-generic.c | 55 ++++++++++++++++++++++++------------- > > 1 file changed, 36 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > > index ba46e58..e9d9c80 100644 > > --- a/drivers/pci/host/pci-host-generic.c > > +++ b/drivers/pci/host/pci-host-generic.c > > @@ -38,7 +38,15 @@ struct gen_pci_cfg_windows { > > const struct gen_pci_cfg_bus_ops *ops; > > }; > > > > +/* > > + * ARM needs platform specific pci_sys_data as the sysdata for PCI. > > + * We add the sys as the first field below to handle this. sys will > > + * set to 0, so that the pci functions in do the right thing. > > + */ > > struct gen_pci { > > +#ifdef CONFIG_ARM > > + struct pci_sys_data sys; > > +#endif > > struct pci_host_bridge host; > > struct gen_pci_cfg_windows cfg; > > struct list_head resources; > > @@ -48,8 +56,7 @@ static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, > > unsigned int devfn, > > int where) > > { > > - struct pci_sys_data *sys = bus->sysdata; > > - struct gen_pci *pci = sys->private_data; > > + struct gen_pci *pci = bus->sysdata; > > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > > > return pci->cfg.win[idx] + ((devfn << 8) | where); > > @@ -64,8 +71,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus, > > unsigned int devfn, > > int where) > > { > > - struct pci_sys_data *sys = bus->sysdata; > > - struct gen_pci *pci = sys->private_data; > > + struct gen_pci *pci = bus->sysdata; > > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > > > return pci->cfg.win[idx] + ((devfn << 12) | where); > > @@ -198,13 +204,6 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > > return 0; > > } > > > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > > -{ > > - struct gen_pci *pci = sys->private_data; > > - list_splice_init(&pci->resources, &sys->resources); > > - return 1; > > -} > > - > > static int gen_pci_probe(struct platform_device *pdev) > > { > > int err; > > @@ -214,13 +213,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > struct device *dev = &pdev->dev; > > struct device_node *np = dev->of_node; > > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > > - struct hw_pci hw = { > > - .nr_controllers = 1, > > - .private_data = (void **)&pci, > > - .setup = gen_pci_setup, > > - .map_irq = of_irq_parse_and_map_pci, > > - .ops = &gen_pci_ops, > > - }; > > + struct pci_bus *bus; > > > > if (!pci) > > return -ENOMEM; > > @@ -258,7 +251,31 @@ static int gen_pci_probe(struct platform_device *pdev) > > return err; > > } > > > > - pci_common_init_dev(dev, &hw); > > + /* do not reassign resource if probe only */ > > + if (!pci_has_flag(PCI_PROBE_ONLY)) > > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > > You should add PCI_REASSIGN_ALL_BUS here. As I said in patch 2 comment, The idea is to do exactly what pci_common_init_dev does on ARM. There is not need to change behavior on ARM and deal with the fallout in this patchset. > please enable setup-irq.c in an explicit commit, re-sequence the > series and fold the Kconfig ARM64 enablement in this patch. I would think that the Makefile and Kconfig changes should be togethter as the arm64 enablement patch. The previous patch is to update the gen_pci driver, and it really makes sense in that order. I am not sure why you suggest this change. JC.
On Fri, Jul 31, 2015 at 05:07:01PM +0100, Jayachandran C. wrote: > On Thu, Jul 30, 2015 at 02:35:21PM +0100, Lorenzo Pieralisi wrote: > > On Wed, Jul 29, 2015 at 04:28:00PM +0100, Jayachandran C wrote: > > > The current code in pci-host-generic.c uses pci_common_init_dev() > > > from ARM platform to do some of the PCI initializations, and this > > > prevents it from being used in ARM64. > > > > > > The initialization done by pci_common_init_dev() that is needed > > > by pci-host-generic.c is really limited, and can be done easily > > > in the same file without using hw_pci API. The ARM platform > > > requires a pci_sys_data as sysdata for the PCI bus, this can be > > > handled by setting up gen_pci to have a pci_sys_data variable as > > > the first element. > > > > For the sake of enabling it on ARM64, I am okay with getting > > this merged, keeping in mind that as I already said pci_sys_data > > has to go, it is a hack and I do not want it to spread to > > ALL PCI host drivers that have to work on both ARM/ARM64. > > > > Comments and tags below. > > > > > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > > > --- > > > Here's v3 of the patchset. > > > > > > v2-v3 > > > - rebase to 4.2-rc > > > - fix PCI_PROBE_ONLY check before calling pcie configure > > > - added a comment above sysdata > > > - updated the commit message > > > > > > v1->v2 > > > - Address comments from Arnd Bergmann and Lorenzo Pieralisi > > > - move contents of gen_pci_init to gen_pci_probe > > > - assign resources only when !probe_only > > > - tested on ARM32 with qemu option -M virt > > > > > > Notes: > > > - passing a zeroed out pci_sys_data for ARM looks ok, but I haven't > > > tested it on ARM. > > > - tested it on ARM64 fast model > > > - Any information on how this can be tested on arm is welcome. > > > - There is only one ifdef, and that can be removed when arm64 gets > > > a sysdata, or when arm loses its sysdata. > > > > > > drivers/pci/host/pci-host-generic.c | 55 ++++++++++++++++++++++++------------- > > > 1 file changed, 36 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > > > index ba46e58..e9d9c80 100644 > > > --- a/drivers/pci/host/pci-host-generic.c > > > +++ b/drivers/pci/host/pci-host-generic.c > > > @@ -38,7 +38,15 @@ struct gen_pci_cfg_windows { > > > const struct gen_pci_cfg_bus_ops *ops; > > > }; > > > > > > +/* > > > + * ARM needs platform specific pci_sys_data as the sysdata for PCI. > > > + * We add the sys as the first field below to handle this. sys will > > > + * set to 0, so that the pci functions in do the right thing. > > > + */ > > > struct gen_pci { > > > +#ifdef CONFIG_ARM > > > + struct pci_sys_data sys; > > > +#endif > > > struct pci_host_bridge host; > > > struct gen_pci_cfg_windows cfg; > > > struct list_head resources; > > > @@ -48,8 +56,7 @@ static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, > > > unsigned int devfn, > > > int where) > > > { > > > - struct pci_sys_data *sys = bus->sysdata; > > > - struct gen_pci *pci = sys->private_data; > > > + struct gen_pci *pci = bus->sysdata; > > > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > > > > > return pci->cfg.win[idx] + ((devfn << 8) | where); > > > @@ -64,8 +71,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus, > > > unsigned int devfn, > > > int where) > > > { > > > - struct pci_sys_data *sys = bus->sysdata; > > > - struct gen_pci *pci = sys->private_data; > > > + struct gen_pci *pci = bus->sysdata; > > > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > > > > > return pci->cfg.win[idx] + ((devfn << 12) | where); > > > @@ -198,13 +204,6 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > > > return 0; > > > } > > > > > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > > > -{ > > > - struct gen_pci *pci = sys->private_data; > > > - list_splice_init(&pci->resources, &sys->resources); > > > - return 1; > > > -} > > > - > > > static int gen_pci_probe(struct platform_device *pdev) > > > { > > > int err; > > > @@ -214,13 +213,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > struct device *dev = &pdev->dev; > > > struct device_node *np = dev->of_node; > > > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > > > - struct hw_pci hw = { > > > - .nr_controllers = 1, > > > - .private_data = (void **)&pci, > > > - .setup = gen_pci_setup, > > > - .map_irq = of_irq_parse_and_map_pci, > > > - .ops = &gen_pci_ops, > > > - }; > > > + struct pci_bus *bus; > > > > > > if (!pci) > > > return -ENOMEM; > > > @@ -258,7 +251,31 @@ static int gen_pci_probe(struct platform_device *pdev) > > > return err; > > > } > > > > > > - pci_common_init_dev(dev, &hw); > > > + /* do not reassign resource if probe only */ > > > + if (!pci_has_flag(PCI_PROBE_ONLY)) > > > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > > > > You should add PCI_REASSIGN_ALL_BUS here. As I said in patch 2 comment, > > The idea is to do exactly what pci_common_init_dev does on ARM. There is > not need to change behavior on ARM and deal with the fallout in this > patchset. You are enabling this driver on ARM64, and on ARM64 that flag does make a difference, it does not on ARM as far as I can tell, but you do not seem to care. Actually Rob asked you to put together a function since most of this code is replicated across ARM drivers eg: drivers/pci/host/pci-versatile.c Leave the code as it is and I will consolidate the code later. > > please enable setup-irq.c in an explicit commit, re-sequence the > > series and fold the Kconfig ARM64 enablement in this patch. > > I would think that the Makefile and Kconfig changes should be > togethter as the arm64 enablement patch. The previous patch is > to update the gen_pci driver, and it really makes sense in that > order. I am not sure why you suggest this change. Because this is not the only driver that requires compiling drivers/pci/setup-irq.c on ARM64, I thought it made more sense to do it in a separate patch to make that change standalone. If for any reason we have to revert your patch that enables PCI host generic on ARM64 (and we remove with it the compilation of setup-irq.c) we would break other ARM64 PCI hosts that require it, right ? I leave the decision to Bjorn since I am away next week, I am fine with the code as it is so please go ahead. Thanks, Lorenzo
Hi Jayachandran, Bjorn, On Fri, Jul 31, 2015 at 05:27:16PM +0100, Lorenzo Pieralisi wrote: > On Fri, Jul 31, 2015 at 05:07:01PM +0100, Jayachandran C. wrote: > > On Thu, Jul 30, 2015 at 02:35:21PM +0100, Lorenzo Pieralisi wrote: > > > On Wed, Jul 29, 2015 at 04:28:00PM +0100, Jayachandran C wrote: > > > > - pci_common_init_dev(dev, &hw); > > > > + /* do not reassign resource if probe only */ > > > > + if (!pci_has_flag(PCI_PROBE_ONLY)) > > > > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > > > > > > You should add PCI_REASSIGN_ALL_BUS here. As I said in patch 2 comment, > > > > The idea is to do exactly what pci_common_init_dev does on ARM. There is > > not need to change behavior on ARM and deal with the fallout in this > > patchset. > > You are enabling this driver on ARM64, and on ARM64 that flag does > make a difference, it does not on ARM as far as I can tell, but you do > not seem to care. > > Actually Rob asked you to put together a function since most of > this code is replicated across ARM drivers eg: > > drivers/pci/host/pci-versatile.c > > Leave the code as it is and I will consolidate the code later. > > > > please enable setup-irq.c in an explicit commit, re-sequence the > > > series and fold the Kconfig ARM64 enablement in this patch. > > > > I would think that the Makefile and Kconfig changes should be > > togethter as the arm64 enablement patch. The previous patch is > > to update the gen_pci driver, and it really makes sense in that > > order. I am not sure why you suggest this change. > > Because this is not the only driver that requires compiling > drivers/pci/setup-irq.c on ARM64, I thought it made more sense > to do it in a separate patch to make that change standalone. > > If for any reason we have to revert your patch that enables > PCI host generic on ARM64 (and we remove with it the compilation > of setup-irq.c) we would break other ARM64 PCI hosts that require it, > right ? > > I leave the decision to Bjorn since I am away next week, I am fine > with the code as it is so please go ahead. In the interest of getting something merged, can I suggest you (Jayachandran) repost the patches with Lorenzo's suggestions to reorder the series and split up the setup-irq compilation into a separate patch? You can also add his ack to this patch. It's a pity that the code isn't quite how we'd like it to end up, but this at least enables people for 4.3 and I know that Lorenzo is working on tidying up the loose ends as additional patches (but he's on holiday this week). That includes removal of the PROBE_ONLY bodge that I've got queued in arch/arm64/kernel/pci.c. Bjorn, does that sound ok to you? I'm keen to have something working, as I'm starting to get beaten up about the lack of generic PCI on arm64. Cheers, Will
On Mon, Aug 03, 2015 at 02:11:32PM +0100, Will Deacon wrote: > Hi Jayachandran, Bjorn, > > On Fri, Jul 31, 2015 at 05:27:16PM +0100, Lorenzo Pieralisi wrote: > > On Fri, Jul 31, 2015 at 05:07:01PM +0100, Jayachandran C. wrote: > > > On Thu, Jul 30, 2015 at 02:35:21PM +0100, Lorenzo Pieralisi wrote: > > > > On Wed, Jul 29, 2015 at 04:28:00PM +0100, Jayachandran C wrote: > > > > > - pci_common_init_dev(dev, &hw); > > > > > + /* do not reassign resource if probe only */ > > > > > + if (!pci_has_flag(PCI_PROBE_ONLY)) > > > > > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > > > > > > > > You should add PCI_REASSIGN_ALL_BUS here. As I said in patch 2 comment, > > > > > > The idea is to do exactly what pci_common_init_dev does on ARM. There is > > > not need to change behavior on ARM and deal with the fallout in this > > > patchset. > > > > You are enabling this driver on ARM64, and on ARM64 that flag does > > make a difference, it does not on ARM as far as I can tell, but you do > > not seem to care. > > > > Actually Rob asked you to put together a function since most of > > this code is replicated across ARM drivers eg: > > > > drivers/pci/host/pci-versatile.c > > > > Leave the code as it is and I will consolidate the code later. > > > > > > please enable setup-irq.c in an explicit commit, re-sequence the > > > > series and fold the Kconfig ARM64 enablement in this patch. > > > > > > I would think that the Makefile and Kconfig changes should be > > > togethter as the arm64 enablement patch. The previous patch is > > > to update the gen_pci driver, and it really makes sense in that > > > order. I am not sure why you suggest this change. > > > > Because this is not the only driver that requires compiling > > drivers/pci/setup-irq.c on ARM64, I thought it made more sense > > to do it in a separate patch to make that change standalone. > > > > If for any reason we have to revert your patch that enables > > PCI host generic on ARM64 (and we remove with it the compilation > > of setup-irq.c) we would break other ARM64 PCI hosts that require it, > > right ? > > > > I leave the decision to Bjorn since I am away next week, I am fine > > with the code as it is so please go ahead. > > In the interest of getting something merged, can I suggest you > (Jayachandran) repost the patches with Lorenzo's suggestions to reorder > the series and split up the setup-irq compilation into a separate patch? > > You can also add his ack to this patch. > > It's a pity that the code isn't quite how we'd like it to end up, but > this at least enables people for 4.3 and I know that Lorenzo is working > on tidying up the loose ends as additional patches (but he's on holiday > this week). That includes removal of the PROBE_ONLY bodge that I've got > queued in arch/arm64/kernel/pci.c. > > Bjorn, does that sound ok to you? I'm keen to have something working, as > I'm starting to get beaten up about the lack of generic PCI on arm64. Sounds good to me.
Hello! > Notes: > - passing a zeroed out pci_sys_data for ARM looks ok, but I haven't > tested it on ARM. > - tested it on ARM64 fast model > - Any information on how this can be tested on arm is welcome. I have tested it on SMDK5410, works OK. By the way, PCI generic is missing MSI support, i had to add it. I can share the patch if interested, it's very small. Tested-by: Pavel Fedin <p.fedin@samsung.com> Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c index ba46e58..e9d9c80 100644 --- a/drivers/pci/host/pci-host-generic.c +++ b/drivers/pci/host/pci-host-generic.c @@ -38,7 +38,15 @@ struct gen_pci_cfg_windows { const struct gen_pci_cfg_bus_ops *ops; }; +/* + * ARM needs platform specific pci_sys_data as the sysdata for PCI. + * We add the sys as the first field below to handle this. sys will + * set to 0, so that the pci functions in do the right thing. + */ struct gen_pci { +#ifdef CONFIG_ARM + struct pci_sys_data sys; +#endif struct pci_host_bridge host; struct gen_pci_cfg_windows cfg; struct list_head resources; @@ -48,8 +56,7 @@ static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, unsigned int devfn, int where) { - struct pci_sys_data *sys = bus->sysdata; - struct gen_pci *pci = sys->private_data; + struct gen_pci *pci = bus->sysdata; resource_size_t idx = bus->number - pci->cfg.bus_range->start; return pci->cfg.win[idx] + ((devfn << 8) | where); @@ -64,8 +71,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus, unsigned int devfn, int where) { - struct pci_sys_data *sys = bus->sysdata; - struct gen_pci *pci = sys->private_data; + struct gen_pci *pci = bus->sysdata; resource_size_t idx = bus->number - pci->cfg.bus_range->start; return pci->cfg.win[idx] + ((devfn << 12) | where); @@ -198,13 +204,6 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) return 0; } -static int gen_pci_setup(int nr, struct pci_sys_data *sys) -{ - struct gen_pci *pci = sys->private_data; - list_splice_init(&pci->resources, &sys->resources); - return 1; -} - static int gen_pci_probe(struct platform_device *pdev) { int err; @@ -214,13 +213,7 @@ static int gen_pci_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); - struct hw_pci hw = { - .nr_controllers = 1, - .private_data = (void **)&pci, - .setup = gen_pci_setup, - .map_irq = of_irq_parse_and_map_pci, - .ops = &gen_pci_ops, - }; + struct pci_bus *bus; if (!pci) return -ENOMEM; @@ -258,7 +251,31 @@ static int gen_pci_probe(struct platform_device *pdev) return err; } - pci_common_init_dev(dev, &hw); + /* do not reassign resource if probe only */ + if (!pci_has_flag(PCI_PROBE_ONLY)) + pci_add_flags(PCI_REASSIGN_ALL_RSRC); + + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); + if (!bus) { + dev_err(dev, "Scanning rootbus failed"); + return -ENODEV; + } + + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); + + if (!pci_has_flag(PCI_PROBE_ONLY)) { + pci_bus_size_bridges(bus); + pci_bus_assign_resources(bus); + } + pci_bus_add_devices(bus); + + /* Configure PCI Express settings */ + if (!pci_has_flag(PCI_PROBE_ONLY)) { + struct pci_bus *child; + + list_for_each_entry(child, &bus->children, node) + pcie_bus_configure_settings(child); + } return 0; }
The current code in pci-host-generic.c uses pci_common_init_dev() from ARM platform to do some of the PCI initializations, and this prevents it from being used in ARM64. The initialization done by pci_common_init_dev() that is needed by pci-host-generic.c is really limited, and can be done easily in the same file without using hw_pci API. The ARM platform requires a pci_sys_data as sysdata for the PCI bus, this can be handled by setting up gen_pci to have a pci_sys_data variable as the first element. Signed-off-by: Jayachandran C <jchandra@broadcom.com> --- Here's v3 of the patchset. v2-v3 - rebase to 4.2-rc - fix PCI_PROBE_ONLY check before calling pcie configure - added a comment above sysdata - updated the commit message v1->v2 - Address comments from Arnd Bergmann and Lorenzo Pieralisi - move contents of gen_pci_init to gen_pci_probe - assign resources only when !probe_only - tested on ARM32 with qemu option -M virt Notes: - passing a zeroed out pci_sys_data for ARM looks ok, but I haven't tested it on ARM. - tested it on ARM64 fast model - Any information on how this can be tested on arm is welcome. - There is only one ifdef, and that can be removed when arm64 gets a sysdata, or when arm loses its sysdata. drivers/pci/host/pci-host-generic.c | 55 ++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 19 deletions(-)