Re: [PATCH] pci: generic host: make it more generic
From: Ming Lei
Date: Tue Nov 11 2014 - 09:03:39 EST
On Tue, Nov 11, 2014 at 8:29 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@xxxxxxx> wrote:
> 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.
One motivation of the patch is to remove ARCH or platform dependency,
that is why the above faked sysdata is introduced, but pci_sysdata is a
generic issue.
>
>> 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.
I should have looked at list first before writing the patch, :-(
Could you make it more generic so that other ARCHs(at least ARM64)
can benefit from it too?
>
>> 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() ?
Yes, it can be removed, but basically that is what the ARM pcibios code
is doing, :-)
> 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.
Nice to know you are working on it.
>
> Overall, we are already working to achieve what this patch is trying to
> achieve, I will keep you posted.
OK, thanks.
--
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/