[PATCH v6] PCI: Store PCIe bus address in struct of_pci_range
Zhou Wang
wangzhou1 at hisilicon.com
Thu Jul 30 00:16:42 PDT 2015
On 2015/7/30 3:44, Gabriele Paoloni wrote:
> Hi Bjorn
>
> Many Thanks for your reply
>
> I have commented back inline with resolutions from my side.
>
> If you're ok with them I'll send it out a new version in the appropriate patchset
>
> Cheers
>
> Gab
>
>> -----Original Message-----
>> From: Bjorn Helgaas [mailto:bhelgaas at google.com]
>> Sent: Wednesday, July 29, 2015 6:21 PM
>> To: Gabriele Paoloni
>> Cc: arnd at arndb.de; lorenzo.pieralisi at arm.com; Wangzhou (B);
>> robh+dt at kernel.org; james.morse at arm.com; Liviu.Dudau at arm.com; linux-
>> pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
>> devicetree at vger.kernel.org; Yuanzhichang; Zhudacai; zhangjukuo;
>> qiuzhenfa; Liguozhu (Kenneth)
>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
>> of_pci_range
>>
>> Hi Gabriele,
>>
>> As far as I can tell, this is not specific to PCIe, so please use "PCI"
>> in
>> the subject as a generic term that includes both PCI and PCIe.
>
> sure agreed
>
>>
>> On Mon, Jul 27, 2015 at 11:17:03PM +0800, Gabriele Paoloni wrote:
>>> From: gabriele paoloni <gabriele.paoloni at huawei.com>
>>>
>>> This patch is needed port PCIe designware to new DT parsing API
>>> As discussed in
>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-
>> January/317743.html
>>> in designware we have a problem as the PCI addresses in the PCIe
>> controller
>>> address space are required in order to perform correct HW
>> operation.
>>>
>>> In order to solve this problem commit f4c55c5a3 "PCI: designware:
>>> Program ATU with untranslated address" added code to read the
>> PCIe
>>
>> Conventional reference is 12-char SHA1, like this:
>>
>> f4c55c5a3f7f ("PCI: designware: Program ATU with untranslated
>> address")
>
> Agreed, will change this
>
>>
>>> controller start address directly from the DT ranges.
>>>
>>> In the new DT parsing API of_pci_get_host_bridge_resources()
>> hides the
>>> DT parser from the host controller drivers, so it is not possible
>>> for drivers to parse values directly from the DT.
>>>
>>> In http://www.spinics.net/lists/linux-pci/msg42540.html we
>> already tried
>>> to use the new DT parsing API but there is a bug (obviously) in
>> setting
>>> the <*>_mod_base addresses
>>> Applying this patch we can easily set "<*>_mod_base = win-
>>> __res.start"
>>
>> By itself, this patch adds something. It would help me understand it
>> if
>> the *user* of this new something were in the same patch series.
>
> the user is: "[PATCH v5 2/5] PCI: designware: Add ARM64 support"
> I will ask Zhou Wang to include this patch in his patchset
>
Hi Gab,
I can merge this patch in my series if this make it clearer to understand.
Thanks,
Zhou
>
>>
>>> This patch adds a new field in "struct of_pci_range" to store the
>>> pci bus start address; it fills the field in
>> of_pci_range_parser_one();
>>> in of_pci_get_host_bridge_resources() it retrieves the resource
>> entry
>>> after it is created and added to the resource list and uses
>>> entry->__res.start to store the pci controller address
>>
>> struct of_pci_range is starting to get confusing to non-OF folks like
>> me.
>> It now contains:
>>
>> u32 pci_space;
>> u64 pci_addr;
>> u64 cpu_addr;
>> u64 bus_addr;
>>
>> Can you explain what all these things mean, and maybe even add one-line
>> comments to the structure?
>
> sure I can add comments inline in the code
>
>>
>> pci_space: The only uses I see are to determine whether to print
>> "Prefetch". I don't see any real functionality that uses this.
>
> Looking at the code I agree. it's seems to be used only in powerpc
> and microblaze to print out.
> However from my understanding pci_space is the phys.hi field of the
> ranges property: it defines the properties of the address space associated
> to the PCI address. if you're curious you can find a nice and quick to read
> "guide" in http://devicetree.org/MPC5200:PCI
>
>>
>> pci_addr: I assume this is a PCI bus address, like what you would see
>> if
>> you put an analyzer on the bus/link. This address could go in a BAR.
>
> Yes, this is the PCI start address of the range: phys.mid + phys.low in the
> guide mentioned above
>
>>
>> cpu_addr: I assume this is a CPU physical address, like what you would
>> see
>> in /proc/iomem and what you would pass to ioremap().
>
> Yes correct
>
>>
>> bus_addr: ?
>>
>
> According to the guide above, this is the address into which the pci_address
> get translated to and that is passed to the root complex. Between the root
> complex and the CPU there can be intermediate translation layers: see that to
> get pci_address we call "of_translate_address"; this will apply all the
> translation layers (ranges in the DT) that it finds till it comes to the root
> node of the DT (thus retrieving the CPU address).
> Now said that, for designware we need the first translated PCI address, that we call
> here bus_addr after Rob Herring suggested the name...honestly I cannot think of a
> different name
>
>
>
>> I'm trying to imagine how this might be expressed in ACPI. A host
>> bridge
>> ACPI _CRS contains a CPU physical address and applying a _TRA
>> (translation
>> offset) to the CPU address gives you a PCI bus address. I know this
>> code
>> is OF, not ACPI, but I assume that it should be possible to describe
>> your
>> hardware via ACPI as well as by OF.
>>
>>> the patch is based on 4.2-rc1
>>
>> You can put this after the "---" line because it's not relevant in the
>> permanent changelog.
>
> Agreed
>
>>
>>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni at huawei.com>
>>> Acked-by: Liviu Dudau <Liviu.Dudau at arm.com>
>>> Acked-by: Rob Herring <robh at kernel.org>
>>
>> Please un-indent your changelog.
>
> Ok agreed
>
>>
>>> ---
>>> drivers/of/address.c | 2 ++
>>> drivers/of/of_pci.c | 4 ++++
>>> include/linux/of_address.h | 1 +
>>> 3 files changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>>> index 8bfda6a..23a5793 100644
>>> --- a/drivers/of/address.c
>>> +++ b/drivers/of/address.c
>>> @@ -253,6 +253,7 @@ struct of_pci_range
>> *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>>> struct of_pci_range *range)
>>> {
>>> const int na = 3, ns = 2;
>>> + const int p_ns = of_n_size_cells(parser->node);
>>>
>>> if (!range)
>>> return NULL;
>>> @@ -265,6 +266,7 @@ struct of_pci_range
>> *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>>> range->pci_addr = of_read_number(parser->range + 1, ns);
>>> range->cpu_addr = of_translate_address(parser->node,
>>> parser->range + na);
>>> + range->bus_addr = of_read_number(parser->range + na, p_ns);
>>> range->size = of_read_number(parser->range + parser->pna + na,
>> ns);
>>>
>>> parser->range += parser->np;
>>> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
>>> index 5751dc5..fe57030 100644
>>> --- a/drivers/of/of_pci.c
>>> +++ b/drivers/of/of_pci.c
>>> @@ -198,6 +198,7 @@ int of_pci_get_host_bridge_resources(struct
>> device_node *dev,
>>>
>>> pr_debug("Parsing ranges property...\n");
>>> for_each_of_pci_range(&parser, &range) {
>>> + struct resource_entry *entry;
>>> /* Read next ranges element */
>>> if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO)
>>> snprintf(range_type, 4, " IO");
>>> @@ -240,6 +241,9 @@ int of_pci_get_host_bridge_resources(struct
>> device_node *dev,
>>> }
>>>
>>> pci_add_resource_offset(resources, res, res->start -
>> range.pci_addr);
>>> + entry = list_last_entry(resources, struct resource_entry,
>> node);
>>> + /* we are using __res for storing the PCI controller
>> address */
>>> + entry->__res.start = range.bus_addr;
>>> }
>>>
>>> return 0;
>>> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
>>> index d88e81b..865f96e 100644
>>> --- a/include/linux/of_address.h
>>> +++ b/include/linux/of_address.h
>>> @@ -16,6 +16,7 @@ struct of_pci_range {
>>> u32 pci_space;
>>> u64 pci_addr;
>>> u64 cpu_addr;
>>> + u64 bus_addr;
>>> u64 size;
>>> u32 flags;
>>> };
>>> --
>>> 1.9.1
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-pci"
>> in
>>> the body of a message to majordomo at vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> .
>
More information about the linux-arm-kernel
mailing list