On Sat, Feb 8, 2014 at 6:22 AM, Liviu Dudau <liviu.du...@arm.com> wrote:
> On Sat, Feb 08, 2014 at 12:21:56AM +0000, Tanmay Inamdar wrote:
>> On Thu, Feb 6, 2014 at 2:18 AM, Liviu Dudau <liviu.du...@arm.com> wrote:
>> > On Wed, Feb 05, 2014 at 10:26:27PM +0000, Tanmay Inamdar wrote:
>> >> Hello Liviu,
>> >>
>> >> I did not get the first email of this particular patch on any of
>> >> subscribed mailing lists (don't know why), hence replying here.
>> >
>> > Strange, it shows in the MARC and GMANE archive for linux-pci, probably
>> > a hickup on your receiving side?
>> >
>> >>
>> >> +struct pci_host_bridge *
>> >> +pci_host_bridge_of_init(struct device *parent, int busno, struct pci_ops
>> >> *ops,
>> >> + void *host_data, struct list_head *resources)
>> >> +{
>> >> + struct pci_bus *root_bus;
>> >> + struct pci_host_bridge *bridge;
>> >> +
>> >> + /* first parse the host bridge bus ranges */
>> >> + if (pci_host_bridge_of_get_ranges(parent->of_node, resources))
>> >> + return NULL;
>> >> +
>> >> + /* then create the root bus */
>> >> + root_bus = pci_create_root_bus(parent, busno, ops, host_data,
>> >> resources);
>> >> + if (!root_bus)
>> >> + return NULL;
>> >> +
>> >> + bridge = to_pci_host_bridge(root_bus->bridge);
>> >> +
>> >> + return bridge;
>> >> +}
>> >>
>> >> You are keeping the domain_nr inside pci_host_bridge structure. In
>> >> above API, domain_nr is required in 'pci_find_bus' function called
>> >> from 'pci_create_root_bus'. Since the bridge is allocated after
>> >> creating root bus, 'pci_find_bus' always gets domain_nr as 0. This
>> >> will cause problem for scanning multiple domains.
>> >
>> > Good catch. I was switching between creating a pci_controller in
>> > arch/arm64 and
>> > adding the needed bits in pci_host_bridge. After internal review I've
>> > decided to
>> > add the domain_nr to pci_host_bridge, but forgot to update the code
>> > everywhere.
>> >
>> > Thanks for reviewing this, will fix in v2.
>> >
>> > Do you find porting to the new API straight forward?
>>
>> It is quite straight forward for MEM regions but for IO regions it is
>> not. You always assume IO resource starting at 0x0. IMO, this will
>> cause problem for systems with multiple ports / IO windows. You can
>> take a look at 'drivers/pci/host/pcie-designware.c'.
>>
>> Also the manipulations of addresses for IO_RESOURCES can be dangerous.
>> It can make some value negative. For example if my PCI IO range starts
>> in 32 bit address range say 0x8000_0000 with CPU address
>> 0xb0_8000_0000, window->offset after manipulation will become
>> negative.
>>
>> Personally I would like to do get all the PCI and CPU addresses from
>> my device tree as is and do the 'pci_add_resource_offset'. This will
>> help me setup my regions correctly without any further arithmetic.
>> Please let me know what you think.
>
> Yes, that parsing code of ranges is incorrect in light of the current
> discussion. I
> will try to propose a fix in the v2 series.
Okay. Thanks.
>
> Regarding your PCI IO range: if your bus address starts at 0x8000_0000, would
> that not cause problems with devices that use less than 32bits for address
> decoding? It is a rather unusual setup, I believe the x86 world (even PCI
> spec?)
> mandates IO bus ranges in the first 16MB of address range?
>
Yes. It will cause problem with few devices. Anyways it was just an example.
> Best regards,
> Liviu
>
>>
>> >
>> > Best regards,
>> > Liviu
>> >
>> >>
>> >>
>> >> On Mon, Feb 3, 2014 at 10:46 AM, Arnd Bergmann <a...@arndb.de> wrote:
>> >> > On Monday 03 February 2014 18:33:48 Liviu Dudau wrote:
>> >> >> +/**
>> >> >> + * pci_host_bridge_of_get_ranges - Parse PCI host bridge resources
>> >> >> from DT
>> >> >> + * @dev: device node of the host bridge having the range property
>> >> >> + * @resources: list where the range of resources will be added after
>> >> >> DT parsing
>> >> >> + *
>> >> >> + * This function will parse the "ranges" property of a PCI host
>> >> >> bridge device
>> >> >> + * node and setup the resource mapping based on its content. It is
>> >> >> expected
>> >> >> + * that the property conforms with the Power ePAPR document.
>> >> >> + *
>> >> >> + * Each architecture will then apply their filtering based on the
>> >> >> limitations
>> >> >> + * of each platform. One general restriction seems to be the number
>> >> >> of IO space
>> >> >> + * ranges, the PCI framework makes intensive use of struct resource
>> >> >> management,
>> >> >> + * and for IORESOURCE_IO types they can only be requested if they are
>> >> >> contained
>> >> >> + * within the global ioport_resource, so that should be limited to
>> >> >> one IO space
>> >> >> + * range.
>> >> >
>> >> > Actually we have quite a different set of restrictions around I/O space
>> >> > on ARM32
>> >> > at the moment: Each host bridge can have its own 64KB range in an
>> >> > arbitrary
>> >> > location on MMIO space, and the total must not exceed 2MB of I/O space.
>> >> >
>> >> >> + */
>> >> >> +static int pci_host_bridge_of_get_ranges(struct device_node *dev,
>> >> >> + struct list_head *resources)
>> >> >> +{
>> >> >> + struct resource *res;
>> >> >> + struct of_pci_range range;
>> >> >> + struct of_pci_range_parser parser;
>> >> >> + int err;
>> >> >> +
>> >> >> + pr_info("PCI host bridge %s ranges:\n", dev->full_name);
>> >> >> +
>> >> >> + /* Check for ranges property */
>> >> >> + err = of_pci_range_parser_init(&parser, dev);
>> >> >> + if (err)
>> >> >> + return err;
>> >> >> +
>> >> >> + pr_debug("Parsing ranges property...\n");
>> >> >> + for_each_of_pci_range(&parser, &range) {
>> >> >> + /* Read next ranges element */
>> >> >> + pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
>> >> >> + range.pci_space, range.pci_addr);
>> >> >> + pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
>> >> >> + range.cpu_addr, range.size);
>> >> >> +
>> >> >> + /* If we failed translation or got a zero-sized region
>> >> >> + * (some FW try to feed us with non sensical zero sized
>> >> >> regions
>> >> >> + * such as power3 which look like some kind of attempt
>> >> >> + * at exposing the VGA memory hole) then skip this range
>> >> >> + */
>> >> >> + if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
>> >> >> + continue;
>> >> >> +
>> >> >> + res = kzalloc(sizeof(struct resource), GFP_KERNEL);
>> >> >> + if (!res) {
>> >> >> + err = -ENOMEM;
>> >> >> + goto bridge_ranges_nomem;
>> >> >> + }
>> >> >> +
>> >> >> + of_pci_range_to_resource(&range, dev, res);
>> >> >> +
>> >> >> + pci_add_resource_offset(resources, res,
>> >> >> + range.cpu_addr - range.pci_addr);
>> >> >> + }
>> >> >
>> >> > I believe of_pci_range_to_resource() will return the MMIO aperture for
>> >> > the
>> >> > I/O space window here, which is not what you are supposed to pass into
>> >> > pci_add_resource_offset.
>> >> >
>> >> >> +EXPORT_SYMBOL(pci_host_bridge_of_init);
>> >> >
>> >> > EXPORT_SYMBOL_GPL
>> >> >
>> >> >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> >> >> index 6e34498..16febae 100644
>> >> >> --- a/drivers/pci/probe.c
>> >> >> +++ b/drivers/pci/probe.c
>> >> >> @@ -1787,6 +1787,17 @@ struct pci_bus *pci_create_root_bus(struct
>> >> >> device *parent, int bus,
>> >> >> list_for_each_entry_safe(window, n, resources, list) {
>> >> >> list_move_tail(&window->list, &bridge->windows);
>> >> >> res = window->res;
>> >> >> + /*
>> >> >> + * IO resources are stored in the kernel with a CPU start
>> >> >> + * address of zero. Adjust the data accordingly and
>> >> >> remember
>> >> >> + * the offset
>> >> >> + */
>> >> >> + if (resource_type(res) == IORESOURCE_IO) {
>> >> >> + bridge->io_offset = res->start;
>> >> >> + res->end -= res->start;
>> >> >> + window->offset -= res->start;
>> >> >> + res->start = 0;
>> >> >> + }
>> >> >> offset = window->offset;
>> >> >> if (res->flags & IORESOURCE_BUS)
>> >> >
>> >> > Won't this break all existing host bridges?
>> >> >
>> >> > Arnd
>> >> >
>> >> > _______________________________________________
>> >> > linux-arm-kernel mailing list
>> >> > linux-arm-ker...@lists.infradead.org
>> >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> >>
>> >
>> > --
>> > ====================
>> > | I would like to |
>> > | fix the world, |
>> > | but they're not |
>> > | giving me the |
>> > \ source code! /
>> > ---------------
>> > ¯\_(ツ)_/¯
>> >
>> > -- IMPORTANT NOTICE: The contents of this email and any attachments are
>> > confidential and may also be privileged. If you are not the intended
>> > recipient, please notify the sender immediately and do not disclose the
>> > contents to any other person, use it for any purpose, or store or copy the
>> > information in any medium. Thank you.
>> >
>> > ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ,
>> > Registered in England & Wales, Company No: 2557590
>> > ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ,
>> > Registered in England & Wales, Company No: 2548782
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
> --
> ====================
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ¯\_(ツ)_/¯
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
>
> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ,
> Registered in England & Wales, Company No: 2557590
> ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ,
> Registered in England & Wales, Company No: 2548782
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
