Re: [PATCH] pci: generic host: make it more generic

From: Lorenzo Pieralisi
Date: Tue Nov 11 2014 - 07:29:14 EST


On Tue, Nov 11, 2014 at 10:33:59AM +0000, Ming Lei wrote:
> This patch converts the generic host controller driver
> into a more generic one, and basically don't need
> platform's pcibios support, and use DT based generic
> APIs to parse resource and remap IO port.
>
> This patch has been tested on both ARMv7 and ARMv8
> VM, and in theroy it should support other ARCHs too.

In theory, yes.

> With this patch, virtio PCI block, network and scsi devices
> can work well on ARMv7 and ARMv8 VM.
>
> QEMU needs below patches for runing the test:
>
> - Rob Herring's "Add generic PCI host device" patches
> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03482.html
> http://lists.gnu.org/archive/html/qemu-devel/2014-06/msg03483.html
> - Alvise Rigo's "Add Generic PCI host device update"
> http://marc.info/?l=qemu-devel&m=140506329920172&w=2
>
> For ARMv8, cpu model of "host" and "cortex-a57" is missed
> in Alvise Rigo's patchset, otherwise ARM64 VM can't boot.
>
> All these QEMU patches can be found in below tree:
>
> http://kernel.ubuntu.com/git?p=ming/qemu.git;a=shortlog;h=refs/heads/arm64-pci-test
>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxxxxx>
> ---
> drivers/pci/host/Kconfig | 2 +-
> drivers/pci/host/pci-host-generic.c | 196 +++++++++++------------------------
> 2 files changed, 63 insertions(+), 135 deletions(-)
>
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 3dc25fa..885034d 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -50,7 +50,7 @@ config PCI_RCAR_GEN2_PCIE
>
> config PCI_HOST_GENERIC
> bool "Generic PCI host controller"
> - depends on ARM && OF
> + depends on OF
> help
> Say Y here if you want to support a simple generic PCI host
> controller, such as the one emulated by kvmtool.
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 3d2076f..00d0ca3 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -44,12 +44,14 @@ struct gen_pci {
> struct list_head resources;
> };
>
> +/* fake sysdata for cheating ARCH's pcibios code */
> +static char gen_sysdata[256];

I take this as a joke and it does not make me laugh.

> 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 = dev_get_drvdata(bus->dev.parent->parent);
> resource_size_t idx = bus->number - pci->cfg.bus_range.start;
>
> return pci->cfg.win[idx] + ((devfn << 8) | where);
> @@ -64,8 +66,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 = dev_get_drvdata(bus->dev.parent->parent);
> resource_size_t idx = bus->number - pci->cfg.bus_range.start;
>
> return pci->cfg.win[idx] + ((devfn << 12) | where);
> @@ -80,8 +81,7 @@ static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn,
> int where, int size, u32 *val)
> {
> void __iomem *addr;
> - struct pci_sys_data *sys = bus->sysdata;
> - struct gen_pci *pci = sys->private_data;
> + struct gen_pci *pci = dev_get_drvdata(bus->dev.parent->parent);
>
> addr = pci->cfg.ops->map_bus(bus, devfn, where);
>
> @@ -103,8 +103,7 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
> int where, int size, u32 val)
> {
> void __iomem *addr;
> - struct pci_sys_data *sys = bus->sysdata;
> - struct gen_pci *pci = sys->private_data;
> + struct gen_pci *pci = dev_get_drvdata(bus->dev.parent->parent);
>
> addr = pci->cfg.ops->map_bus(bus, devfn, where);
>
> @@ -138,45 +137,6 @@ static const struct of_device_id gen_pci_of_match[] = {
> };
> MODULE_DEVICE_TABLE(of, gen_pci_of_match);
>
> -static int gen_pci_calc_io_offset(struct device *dev,
> - struct of_pci_range *range,
> - struct resource *res,
> - resource_size_t *offset)
> -{
> - static atomic_t wins = ATOMIC_INIT(0);
> - int err, idx, max_win;
> - unsigned int window;
> -
> - if (!PAGE_ALIGNED(range->cpu_addr))
> - return -EINVAL;
> -
> - max_win = (IO_SPACE_LIMIT + 1) / SZ_64K;
> - idx = atomic_inc_return(&wins);
> - if (idx > max_win)
> - return -ENOSPC;
> -
> - window = (idx - 1) * SZ_64K;
> - err = pci_ioremap_io(window, range->cpu_addr);
> - if (err)
> - return err;
> -
> - of_pci_range_to_resource(range, dev->of_node, res);
> - res->start = window;
> - res->end = res->start + range->size - 1;
> - *offset = window - range->pci_addr;
> - return 0;
> -}
> -
> -static int gen_pci_calc_mem_offset(struct device *dev,
> - struct of_pci_range *range,
> - struct resource *res,
> - resource_size_t *offset)
> -{
> - of_pci_range_to_resource(range, dev->of_node, res);
> - *offset = range->cpu_addr - range->pci_addr;
> - return 0;
> -}
> -
> static void gen_pci_release_of_pci_ranges(struct gen_pci *pci)
> {
> struct pci_host_bridge_window *win;
> @@ -187,72 +147,6 @@ static void gen_pci_release_of_pci_ranges(struct gen_pci *pci)
> pci_free_resource_list(&pci->resources);
> }
>
> -static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci)
> -{
> - struct of_pci_range range;
> - struct of_pci_range_parser parser;
> - int err, res_valid = 0;
> - struct device *dev = pci->host.dev.parent;
> - struct device_node *np = dev->of_node;
> -
> - if (of_pci_range_parser_init(&parser, np)) {
> - dev_err(dev, "missing \"ranges\" property\n");
> - return -EINVAL;
> - }
> -
> - for_each_of_pci_range(&parser, &range) {
> - struct resource *parent, *res;
> - resource_size_t offset;
> - u32 restype = range.flags & IORESOURCE_TYPE_BITS;
> -
> - res = devm_kmalloc(dev, sizeof(*res), GFP_KERNEL);
> - if (!res) {
> - err = -ENOMEM;
> - goto out_release_res;
> - }
> -
> - switch (restype) {
> - case IORESOURCE_IO:
> - parent = &ioport_resource;
> - err = gen_pci_calc_io_offset(dev, &range, res, &offset);
> - break;
> - case IORESOURCE_MEM:
> - parent = &iomem_resource;
> - err = gen_pci_calc_mem_offset(dev, &range, res, &offset);
> - res_valid |= !(res->flags & IORESOURCE_PREFETCH || err);
> - break;
> - default:
> - err = -EINVAL;
> - continue;
> - }
> -
> - if (err) {
> - dev_warn(dev,
> - "error %d: failed to add resource [type 0x%x, %lld bytes]\n",
> - err, restype, range.size);
> - continue;
> - }
> -
> - err = request_resource(parent, res);
> - if (err)
> - goto out_release_res;
> -
> - pci_add_resource_offset(&pci->resources, res, offset);
> - }
> -
> - if (!res_valid) {
> - dev_err(dev, "non-prefetchable memory resource required\n");
> - err = -EINVAL;
> - goto out_release_res;
> - }
> -
> - return 0;
> -
> -out_release_res:
> - gen_pci_release_of_pci_ranges(pci);
> - return err;
> -}
> -
> static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
> {
> int err;
> @@ -305,16 +199,33 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
> return -ENOMEM;
> }
>
> - /* Register bus resource */
> - pci_add_resource(&pci->resources, bus_range);
> return 0;
> }
>
> -static int gen_pci_setup(int nr, struct pci_sys_data *sys)
> +static int gen_pci_map_ranges(struct list_head *res,
> + resource_size_t io_base)
> {
> - struct gen_pci *pci = sys->private_data;
> - list_splice_init(&pci->resources, &sys->resources);
> - return 1;
> + struct pci_host_bridge_window *window;
> + int ret;
> +
> + list_for_each_entry(window, res, list) {
> + struct resource *res = window->res;
> + u64 restype = resource_type(res);
> +
> + switch (restype) {
> + case IORESOURCE_IO:
> + ret = pci_remap_iospace(res, io_base);
> + if (ret < 0)
> + return ret;
> + break;
> + case IORESOURCE_MEM:
> + case IORESOURCE_BUS:
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> + return 0;
> }

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/296535.html

Patch above is already queued and applies most of the changes you have posted
above.

> static int gen_pci_probe(struct platform_device *pdev)
> @@ -325,14 +236,11 @@ static int gen_pci_probe(struct platform_device *pdev)
> const int *prop;
> struct device *dev = &pdev->dev;
> struct device_node *np = dev->of_node;
> + resource_size_t iobase = 0;
> 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;
> + struct pci_dev *pci_dev = NULL;
> + bool probe_only = false;
>
> if (!pci)
> return -ENOMEM;
> @@ -346,9 +254,9 @@ static int gen_pci_probe(struct platform_device *pdev)
> prop = of_get_property(of_chosen, "linux,pci-probe-only", NULL);
> if (prop) {
> if (*prop)
> - pci_add_flags(PCI_PROBE_ONLY);
> + probe_only = true;
> else
> - pci_clear_flags(PCI_PROBE_ONLY);
> + probe_only = false;
> }

How can this possibly work ? If firmware assigns resources, they
should not be enabled, and this is done in pcibios code by checking
the PCI_PROBE_ONLY flag (pcibios_enable_device()).
Now you are removing the flag, how can pcibios_enable_device() work on
platforms that are PROBE_ONLY ? I do not see any additional patch, so I
am confused.

>
> of_id = of_match_node(gen_pci_of_match, np);
> @@ -357,20 +265,40 @@ static int gen_pci_probe(struct platform_device *pdev)
> INIT_LIST_HEAD(&pci->host.windows);
> INIT_LIST_HEAD(&pci->resources);
>
> - /* Parse our PCI ranges and request their resources */
> - err = gen_pci_parse_request_of_pci_ranges(pci);
> + err = of_pci_get_host_bridge_resources(np, 0, 0xff,
> + &pci->resources, &iobase);
> if (err)
> return err;
>
> /* Parse and map our Configuration Space windows */
> err = gen_pci_parse_map_cfg_windows(pci);
> - if (err) {
> - gen_pci_release_of_pci_ranges(pci);
> - return err;
> + if (err)
> + goto fail;
> +
> + err = gen_pci_map_ranges(&pci->resources, iobase);
> + if (err)
> + goto fail;
> +
> + err = -ENOMEM;
> + platform_set_drvdata(pdev, pci);
> + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, gen_sysdata,
> + &pci->resources);
> + if (!bus)
> + goto fail;
> +
> + for_each_pci_dev(pci_dev)
> + pci_dev->irq = of_irq_parse_and_map_pci(pci_dev, 0, 0);
> +
> + if (!probe_only) {
> + pci_bus_size_bridges(bus);
> + pci_bus_assign_resources(bus);
> + pci_bus_add_devices(bus);
> }

Do you realize that pci_scan_root_bus() already calls pci_bus_add_devices() ?
On top of that:

+ if (!probe_only) {
+ pci_bus_size_bridges(bus);
+ pci_bus_assign_resources(bus);
+ pci_bus_add_devices(bus);

Should be called in pci_scan_root_bus() before pci_bus_add_devices(), if
things are working for you at the moment it is out of sheer chance (and
unfortunately that's the case for other host controllers IMO).

I am working on adding the code above to pci_scan_root_bus() implementation
this is a generic issue that we need to solve, there are already patches
on the list to remove the dependency on pci_sys_data (I mean properly)
and move the code to generic pci_scan_root_bus() implementation.

Overall, we are already working to achieve what this patch is trying to
achieve, I will keep you posted.

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/