| Message ID | 1450749222-15966-2-git-send-email-ddaney.cavm@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 21, 2015 at 05:53:41PM -0800, David Daney wrote: > From: David Daney <david.daney@cavium.com> > > No change in functionality. > > Move structure definitions into a separate header file. Split probe > function in to two parts: > > - a small driver specific probe function (gen_pci_probe) > > - a common probe that can be used by other drivers > (gen_pci_common_probe) > > Signed-off-by: David Daney <david.daney@cavium.com> > --- > drivers/pci/host/pci-host-generic.c | 53 ++++++++++++----------------------- > drivers/pci/host/pci-host-generic.h | 56 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 74 insertions(+), 35 deletions(-) > create mode 100644 drivers/pci/host/pci-host-generic.h > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index 5434c90..e83cec7 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -25,33 +25,7 @@ > #include <linux/of_pci.h> > #include <linux/platform_device.h> > > -struct gen_pci_cfg_bus_ops { > - u32 bus_shift; > - struct pci_ops ops; > -}; > - > -struct gen_pci_cfg_windows { > - struct resource res; > - struct resource *bus_range; > - void __iomem **win; > - > - struct gen_pci_cfg_bus_ops *ops; > -}; > - > -/* > - * ARM pcibios functions expect the ARM struct pci_sys_data as the PCI > - * sysdata. Add pci_sys_data as the first element in struct gen_pci so > - * that when we use a gen_pci pointer as sysdata, it is also a pointer to > - * a struct pci_sys_data. > - */ > -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; > -}; > +#include "pci-host-generic.h" > > static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, > unsigned int devfn, > @@ -208,19 +182,15 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > return 0; > } > > -static int gen_pci_probe(struct platform_device *pdev) > +int gen_pci_common_probe(struct platform_device *pdev, > + struct gen_pci *pci) Whilst I'm fine with this patch, I don't know how Bjorn will feel about exposing this function outside of the generic host driver. We could avoid it by turning things upside-down and having the generic driver probe the other drivers by matching a compatible string with a probe function pointer, but I'd be interested to see what others think. Will
On 12/22/2015 02:07 AM, Will Deacon wrote: > On Mon, Dec 21, 2015 at 05:53:41PM -0800, David Daney wrote: >> From: David Daney <david.daney@cavium.com> >> >> No change in functionality. >> >> Move structure definitions into a separate header file. Split probe >> function in to two parts: >> >> - a small driver specific probe function (gen_pci_probe) >> >> - a common probe that can be used by other drivers >> (gen_pci_common_probe) >> >> Signed-off-by: David Daney <david.daney@cavium.com> >> --- >> drivers/pci/host/pci-host-generic.c | 53 ++++++++++++----------------------- >> drivers/pci/host/pci-host-generic.h | 56 +++++++++++++++++++++++++++++++++++++ >> 2 files changed, 74 insertions(+), 35 deletions(-) >> create mode 100644 drivers/pci/host/pci-host-generic.h >> >> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c >> index 5434c90..e83cec7 100644 >> --- a/drivers/pci/host/pci-host-generic.c >> +++ b/drivers/pci/host/pci-host-generic.c [...] >> -static int gen_pci_probe(struct platform_device *pdev) >> +int gen_pci_common_probe(struct platform_device *pdev, >> + struct gen_pci *pci) > > Whilst I'm fine with this patch, I don't know how Bjorn will feel about > exposing this function outside of the generic host driver. We could avoid > it by turning things upside-down and having the generic driver probe > the other drivers by matching a compatible string with a probe function > pointer, but I'd be interested to see what others think. > Note: I know that pci-host-generic is not built as a loadable module, but... struct of_device_id, MODULE_DEVICE_TABLE, struct platform_driver and the registering of platform drivers is fairly well standardized in the kernel, and module loading userpace tools. The struct of_device_id, MODULE_DEVICE_TABLE must really reside in the same module as the driver for the device. We are creating a separate driver precisely because we don't want to mix all this ThunderX specific code into the pci-host-generic driver when it is used by arm-32bit and others. This means that, at a minimum, we would have to export the pci-host-generic probe function so that it could be referenced by struct platform_driver in other modules. This brings up the next problem. How to attach driver specific data to the generic driver structures? At first I tried augmenting struct gen_pci_cfg_bus_ops with a callback .init() function to be called by the generic driver, but this would also require adding an an element to struct gen_pci to point to a driver specific data object. It felt a little convoluted and complex. This led me to the current design where struct gen_pci is embedded in the driver specific structure, and the allocation of this is done in the driver specific probe function. No more callbacks, no additions to the pci-host-generic structures. I think it is a little cleaner this way. If there are suggestions as to how it can be made cleaner yet, I would be happy to implement and test them. David Daney > Will >
On Tuesday 22 December 2015, David Daney wrote: > On 12/22/2015 02:07 AM, Will Deacon wrote: > > On Mon, Dec 21, 2015 at 05:53:41PM -0800, David Daney wrote: > >> From: David Daney <david.daney@cavium.com> > >> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > >> index 5434c90..e83cec7 100644 > >> --- a/drivers/pci/host/pci-host-generic.c > >> +++ b/drivers/pci/host/pci-host-generic.c > [...] > >> -static int gen_pci_probe(struct platform_device *pdev) > >> +int gen_pci_common_probe(struct platform_device *pdev, > >> + struct gen_pci *pci) > > > > Whilst I'm fine with this patch, I don't know how Bjorn will feel about > > exposing this function outside of the generic host driver. We could avoid > > it by turning things upside-down and having the generic driver probe > > the other drivers by matching a compatible string with a probe function > > pointer, but I'd be interested to see what others think. > > > > Note: I know that pci-host-generic is not built as a loadable module, but... > > struct of_device_id, MODULE_DEVICE_TABLE, struct platform_driver and the > registering of platform drivers is fairly well standardized in the > kernel, and module loading userpace tools. Agreed, this is the correct way to do the abstraction if we want one, the way that Will describes is generally not a good idea, and we've converted a number of drivers that did it like that to the way you do it here. > The struct of_device_id, MODULE_DEVICE_TABLE must really reside in the > same module as the driver for the device. We are creating a separate > driver precisely because we don't want to mix all this ThunderX specific > code into the pci-host-generic driver when it is used by arm-32bit and > others. This means that, at a minimum, we would have to export the > pci-host-generic probe function so that it could be referenced by struct > platform_driver in other modules. Right. > This brings up the next problem. How to attach driver specific data to > the generic driver structures? At first I tried augmenting struct > gen_pci_cfg_bus_ops with a callback .init() function to be called by the > generic driver, but this would also require adding an an element to > struct gen_pci to point to a driver specific data object. It felt a > little convoluted and complex. > > This led me to the current design where struct gen_pci is embedded in > the driver specific structure, and the allocation of this is done in the > driver specific probe function. No more callbacks, no additions to the > pci-host-generic structures. I think it is a little cleaner this way. > > If there are suggestions as to how it can be made cleaner yet, I would > be happy to implement and test them. My idea of the long-term direction for the pci-host-generic driver would be to move more parts into the PCI core code as library functions that can be used by other drivers as well. This would also address my other concern that I'd like to see the generic host driver remain the simplest example that we have, and only require any additional code in other drivers to add functionality or workarounds. Adding an abstraction layer within the driver to some degree goes in the opposite direction of that. One approach that might work would be to split the existing driver into three files: one for CAM, one for ECAM and one for the common parts, with an interface similar to what you have here. Then you can add your driver as a third front-end, and we can keep working on integrating the common parts further into the PCI core. Arnd
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c index 5434c90..e83cec7 100644 --- a/drivers/pci/host/pci-host-generic.c +++ b/drivers/pci/host/pci-host-generic.c @@ -25,33 +25,7 @@ #include <linux/of_pci.h> #include <linux/platform_device.h> -struct gen_pci_cfg_bus_ops { - u32 bus_shift; - struct pci_ops ops; -}; - -struct gen_pci_cfg_windows { - struct resource res; - struct resource *bus_range; - void __iomem **win; - - struct gen_pci_cfg_bus_ops *ops; -}; - -/* - * ARM pcibios functions expect the ARM struct pci_sys_data as the PCI - * sysdata. Add pci_sys_data as the first element in struct gen_pci so - * that when we use a gen_pci pointer as sysdata, it is also a pointer to - * a struct pci_sys_data. - */ -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; -}; +#include "pci-host-generic.h" static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, unsigned int devfn, @@ -208,19 +182,15 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) return 0; } -static int gen_pci_probe(struct platform_device *pdev) +int gen_pci_common_probe(struct platform_device *pdev, + struct gen_pci *pci) { int err; const char *type; - const struct of_device_id *of_id; struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; - struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); struct pci_bus *bus, *child; - if (!pci) - return -ENOMEM; - type = of_get_property(np, "device_type", NULL); if (!type || strcmp(type, "pci")) { dev_err(dev, "invalid \"device_type\" %s\n", type); @@ -229,8 +199,6 @@ static int gen_pci_probe(struct platform_device *pdev) of_pci_check_probe_only(); - of_id = of_match_node(gen_pci_of_match, np); - pci->cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data; pci->host.dev.parent = dev; INIT_LIST_HEAD(&pci->host.windows); INIT_LIST_HEAD(&pci->resources); @@ -273,6 +241,21 @@ static int gen_pci_probe(struct platform_device *pdev) return 0; } +static int gen_pci_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + const struct of_device_id *of_id; + struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); + + if (!pci) + return -ENOMEM; + + of_id = of_match_node(gen_pci_of_match, dev->of_node); + pci->cfg.ops = (struct gen_pci_cfg_bus_ops *)of_id->data; + + return gen_pci_common_probe(pdev, pci); +} + static struct platform_driver gen_pci_driver = { .driver = { .name = "pci-host-generic", diff --git a/drivers/pci/host/pci-host-generic.h b/drivers/pci/host/pci-host-generic.h new file mode 100644 index 0000000..089fecb --- /dev/null +++ b/drivers/pci/host/pci-host-generic.h @@ -0,0 +1,56 @@ +/* + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + * + * Copyright (C) 2014 ARM Limited + * + * Author: Will Deacon <will.deacon@arm.com> + */ + +#ifndef _PCI_HOST_GENERIC_H +#define _PCI_HOST_GENERIC_H + +#include <linux/kernel.h> +#include <linux/platform_device.h> + +struct gen_pci_cfg_bus_ops { + u32 bus_shift; + struct pci_ops ops; +}; + +struct gen_pci_cfg_windows { + struct resource res; + struct resource *bus_range; + void __iomem **win; + + struct gen_pci_cfg_bus_ops *ops; +}; + +/* + * ARM pcibios functions expect the ARM struct pci_sys_data as the PCI + * sysdata. Add pci_sys_data as the first element in struct gen_pci so + * that when we use a gen_pci pointer as sysdata, it is also a pointer to + * a struct pci_sys_data. + */ +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; +}; + +int gen_pci_common_probe(struct platform_device *pdev, + struct gen_pci *pci); + +#endif /* _PCI_HOST_GENERIC_H */