Re: [PATCH v6] PCI: Store PCIe bus address in struct of_pci_range

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2015. 8. 3., at PM 8:18, Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx> wrote:
> 
> Rob, Kishon what about the following solution?....
> 
> ---
> drivers/pci/host/pci-dra7xx.c      | 12 ++++++++++++
> drivers/pci/host/pcie-designware.c |  9 +++------

Hi Paoloni,

OK, it looks good.
I want you to revert the following patch. 

commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated address)"

Would you remove all '*_mode_base's?

Best regards,
Jingoo Han

> 2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
> index 80db09e..bb2635f 100644
> --- a/drivers/pci/host/pci-dra7xx.c
> +++ b/drivers/pci/host/pci-dra7xx.c
> @@ -61,6 +61,7 @@
> 
> #define    PCIECTRL_DRA7XX_CONF_PHY_CS            0x010C
> #define    LINK_UP                        BIT(16)
> +#define CPU_TO_BUS_ADDR             0x0FFFFFFF
> 
> struct dra7xx_pcie {
>    void __iomem        *base;
> @@ -138,6 +139,17 @@ static void dra7xx_pcie_enable_interrupts(struct pcie_port *pp)
> 
> static void dra7xx_pcie_host_init(struct pcie_port *pp)
> {
> +    if (pp->io_mod_base)
> +        pp->io_mod_base &= CPU_TO_BUS_ADDR;
> +
> +    if (pp->mem_mod_base)
> +        pp->mem_mod_base &= CPU_TO_BUS_ADDR;
> +
> +    if (pp->cfg0_mod_base) {
> +        pp->cfg0_mod_base &= CPU_TO_BUS_ADDR;
> +        pp->cfg1_mod_base &= CPU_TO_BUS_ADDR;
> +    }
> +
>    dw_pcie_setup_rc(pp);
>    dra7xx_pcie_establish_link(pp);
>    if (IS_ENABLED(CONFIG_PCI_MSI))
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 69486be..06c682b 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -416,8 +416,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
>            pp->io_base = range.cpu_addr;
> 
>            /* Find the untranslated IO space address */
> -            pp->io_mod_base = of_read_number(parser.range -
> -                             parser.np + na, ns);
> +            pp->io_mod_base = range.cpu_addr;;
>        }
>        if (restype == IORESOURCE_MEM) {
>            of_pci_range_to_resource(&range, np, &pp->mem);
> @@ -426,8 +425,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
>            pp->mem_bus_addr = range.pci_addr;
> 
>            /* Find the untranslated MEM space address */
> -            pp->mem_mod_base = of_read_number(parser.range -
> -                              parser.np + na, ns);
> +            pp->mem_mod_base = range.cpu_addr;
>        }
>        if (restype == 0) {
>            of_pci_range_to_resource(&range, np, &pp->cfg);
> @@ -437,8 +435,7 @@ int dw_pcie_host_init(struct pcie_port *pp)
>            pp->cfg1_base = pp->cfg.start + pp->cfg0_size;
> 
>            /* Find the untranslated configuration space address */
> -            pp->cfg0_mod_base = of_read_number(parser.range -
> -                               parser.np + na, ns);
> +            pp->cfg0_mod_base = range.cpu_addr;
>            pp->cfg1_mod_base = pp->cfg0_mod_base +
>                        pp->cfg0_size;
>        }
> -- 
> 1.9.1
> 
> 
>> -----Original Message-----
>> From: linux-pci-owner@xxxxxxxxxxxxxxx [mailto:linux-pci-
>> owner@xxxxxxxxxxxxxxx] On Behalf Of Rob Herring
>> Sent: Friday, July 31, 2015 5:53 PM
>> To: Kishon Vijay Abraham I
>> Cc: Gabriele Paoloni; Bjorn Helgaas; arnd@xxxxxxxx;
>> lorenzo.pieralisi@xxxxxxx; Wangzhou (B); robh+dt@xxxxxxxxxx;
>> james.morse@xxxxxxx; Liviu.Dudau@xxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
>> Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);
>> Jingoo Han; Pratyush Anand; Arnd Bergmann
>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
>> of_pci_range
>> 
>> On Fri, Jul 31, 2015 at 9:57 AM, Kishon Vijay Abraham I <kishon@xxxxxx>
>> wrote:
>>> +Arnd
>>> 
>>> Hi,
>>> 
>>>> On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:
>>>> [+cc Kishon]
>>>> 
>>>>> -----Original Message-----
>>>>> From: linux-pci-owner@xxxxxxxxxxxxxxx [mailto:linux-pci-
>>>>> owner@xxxxxxxxxxxxxxx] On Behalf Of Rob Herring
>>>>> Sent: Thursday, July 30, 2015 9:42 PM
>>>>> To: Gabriele Paoloni
>>>>> Cc: Bjorn Helgaas; arnd@xxxxxxxx; lorenzo.pieralisi@xxxxxxx;
>> Wangzhou
>>>>> (B); robh+dt@xxxxxxxxxx; james.morse@xxxxxxx; Liviu.Dudau@xxxxxxx;
>>>>> linux-pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>>>>> devicetree@xxxxxxxxxxxxxxx; Yuanzhichang; Zhudacai; zhangjukuo;
>>>>> qiuzhenfa; Liguozhu (Kenneth); Jingoo Han; Pratyush Anand
>>>>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
>>>>> of_pci_range
>>>>> 
>>>>> On Thu, Jul 30, 2015 at 12:34 PM, Gabriele Paoloni
>>>>> <gabriele.paoloni@xxxxxxxxxx> wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx]
>>>>>>> Sent: 30 July 2015 18:15
>>>>>>> On Thu, Jul 30, 2015 at 04:50:55PM +0000, Gabriele Paoloni wrote:
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: linux-pci-owner@xxxxxxxxxxxxxxx [mailto:linux-pci-
>>>>>>>>> owner@xxxxxxxxxxxxxxx] On Behalf Of Bjorn Helgaas
>>>>>>>>> Sent: Thursday, July 30, 2015 5:15 PM
>>>>>>>>> On Thu, Jul 30, 2015 at 01:52:13PM +0000, Gabriele Paoloni
>> wrote:
>>>>> 
>>>>> [...]
>>>>> 
>>>>>>>>>> I don’t think we should rely on [CPU] addresses...what if the
>>>>>>>>> intermediate
>>>>>>>>>> translation layer changes the lower significant bits of the
>>>>> "bus
>>>>>>>>> address"
>>>>>>>>>> to translate into a cpu address?
>>>>>>>>> 
>>>>>>>>> Is it really a possiblity that the lower bits could be changed?
>>>>>>>> 
>>>>>>>> I've checked all the current deignware users DTs except "pci-
>>>>>>> layerscape"
>>>>>>>> that I could not find:
>>>>>>>> spear1310.dtsi
>>>>>>>> spear1340.dtsi
>>>>>>>> dra7.dtsi
>>>>>>>> imx6qdl.dtsi
>>>>>>>> imx6sx.dtsi
>>>>>>>> keystone.dtsi
>>>>>>>> exynos5440.dtsi
>>>>>>>> 
>>>>>>>> None of them modifies the lower bits. To be more precise the
>> only
>>>>> guy
>>>>>>>> that provides another translation layer is "dra7.dtsi":
>>>>>>>> axi0
>>>>>>>> http://lxr.free-
>>>>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L207
>>>>>>>> 
>>>>>>>> axi1
>>>>>>>> http://lxr.free-
>>>>> electrons.com/source/arch/arm/boot/dts/dra7.dtsi#L241
>>>>>>>> 
>>>>>>>> For this case masking the top 4bits (bits28 to 31) should make
>> the
>>>>> job.
>>>>> 
>>>>> IMO, we should just fix this case. After further study, I don't
>> think
>>>>> this is a DW issue, but rather an SOC integration issue.
>>>>> 
>>>>> I believe you can just fixup the address in the pp->ops->host_init
>> hook.
>>>> 
>>>> Yes I guess that I could just assign pp->(*)_mod_base to the CPU
>> address
>>>> in DW and mask it out in dra7xx_pcie_host_init()...
>>>> 
>>>> Kishon, would you be ok with that?
>>> 
>>> Initially I was using *base-mask* property from dt. Me and Arnd
>> (cc'ed) had
>>> this discussion [1] before we decided the current approach. It'll be
>> good to
>>> check with Arnd too.
>>> 
>>> [1] ->  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-
>> May/253528.html
>> 
>> The problem I have here is the use of ranges does not necessarily mean
>> fewer address bits are available. It can be used just for convenience
>> of not putting the full address into every node's reg property. And
>> vice versa, there are probably plenty of cases where we have the full
>> address in the nodes, but really only some of the address bits are
>> decoded at the IP block. Whether the address bits are present is
>> rarely cared about or known by s/w folks until you hit a problem like
>> this. Given this is an isolated case ATM, I would fix it in an
>> isolated way. It does not affect the binding and could be changed in
>> the kernel later if this becomes a common need.
>> 
>> Rob
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux