diff mbox

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

Message ID EE11001F9E5DDD47B7634E2F8A612F2E01D712E7@lhreml503-mbs (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Gabriele Paoloni Aug. 3, 2015, 11:18 a.m. UTC
Rob, Kishon what about the following solution?....

---
 drivers/pci/host/pci-dra7xx.c      | 12 ++++++++++++
 drivers/pci/host/pcie-designware.c |  9 +++------
 2 files changed, 15 insertions(+), 6 deletions(-)

-- 
1.9.1


> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> owner@vger.kernel.org] On Behalf Of Rob Herring
> Sent: Friday, July 31, 2015 5:53 PM
> To: Kishon Vijay Abraham I
> Cc: Gabriele Paoloni; Bjorn Helgaas; arnd@arndb.de;
> lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;
> james.morse@arm.com; Liviu.Dudau@arm.com; linux-pci@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> 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@ti.com>
> wrote:
> > +Arnd
> >
> > Hi,
> >
> > On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:
> >> [+cc Kishon]
> >>
> >>> -----Original Message-----
> >>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> >>> owner@vger.kernel.org] On Behalf Of Rob Herring
> >>> Sent: Thursday, July 30, 2015 9:42 PM
> >>> To: Gabriele Paoloni
> >>> Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com;
> Wangzhou
> >>> (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com;
> >>> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >>> devicetree@vger.kernel.org; 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@huawei.com> wrote:
> >>>>> -----Original Message-----
> >>>>> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> >>>>> 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@vger.kernel.org [mailto:linux-pci-
> >>>>>>> owner@vger.kernel.org] 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Han Jingoo Aug. 4, 2015, 4:19 a.m. UTC | #1
On 2015. 8. 3., at PM 8:18, Gabriele Paoloni <gabriele.paoloni@huawei.com> 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@vger.kernel.org [mailto:linux-pci-
>> owner@vger.kernel.org] On Behalf Of Rob Herring
>> Sent: Friday, July 31, 2015 5:53 PM
>> To: Kishon Vijay Abraham I
>> Cc: Gabriele Paoloni; Bjorn Helgaas; arnd@arndb.de;
>> lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;
>> james.morse@arm.com; Liviu.Dudau@arm.com; linux-pci@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
>> 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@ti.com>
>> wrote:
>>> +Arnd
>>> 
>>> Hi,
>>> 
>>>> On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:
>>>> [+cc Kishon]
>>>> 
>>>>> -----Original Message-----
>>>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
>>>>> owner@vger.kernel.org] On Behalf Of Rob Herring
>>>>> Sent: Thursday, July 30, 2015 9:42 PM
>>>>> To: Gabriele Paoloni
>>>>> Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com;
>> Wangzhou
>>>>> (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com;
>>>>> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>>> devicetree@vger.kernel.org; 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@huawei.com> wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
>>>>>>> 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@vger.kernel.org [mailto:linux-pci-
>>>>>>>>> owner@vger.kernel.org] 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@vger.kernel.org
>> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gabriele Paoloni Aug. 4, 2015, 10:12 a.m. UTC | #2
> -----Original Message-----

> From: Jingoo Han [mailto:jingoohan1@gmail.com]

> Sent: Tuesday, August 04, 2015 5:20 AM

> To: Gabriele Paoloni

> Cc: Rob Herring; Kishon Vijay Abraham I; Bjorn Helgaas; arnd@arndb.de;

> lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;

> james.morse@arm.com; Liviu.Dudau@arm.com; linux-pci@vger.kernel.org;

> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;

> Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);

> Pratyush Anand; Arnd Bergmann; jingoohan1@gmail.com

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

> of_pci_range

> 

> On 2015. 8. 3., at PM 8:18, Gabriele Paoloni

> <gabriele.paoloni@huawei.com> 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?


Hi Jingoo Han,

If I reverted the commit, in order to get designware working, dra7 
should mask directly the CPU addresses stored in pp->cfg0_base, 
pp->cfg1_base, pp->mem_base and pp->io_base.

The masking would happen at this point: 
http://lxr.free-electrons.com/source/drivers/pci/host/pcie-designware.c#L493

Now:
- I see that pp->cfg<0/1>_base are used in devm_ioremap before that 
  point and nowhere else, so they should be ok.
- pp->mem_base is used in dw_pcie_setup_rc(); dw_pcie_setup_rc() is called
  in dra7xx_pcie_host_init()...so here I should move the masking after
  dw_pcie_setup() retuned, but I think it should be ok
- pp->io_base is used in pci_ioremap_io() in dw_pcie_setup(). Now currently
  in designware this is called by pci_bios_init_hw(); this is after the 
  masking....so here we would have a the wrong value passed

Said that if you look at "[PATCH v5 2/5] PCI: designware: Add ARM64 support",
that is the patchset where this patch is needed, you can see that 
dw_pcie_setup() is removed and pp->io_base is used in pci_remap_iospace() before 
the masking would happen in dra7xx_pcie_host_init()...so for this patchset we 
should be good to revert the commit.

I propose to add a new patch in the patchset "[PATCH v5 0/5] PCI: hisi: Add PCIe 
host support for HiSilicon SoC Hip05" as the one I just posted in this 
thread (this would not revert the commit but just moves the masking to dra7xx)

I would revert the commit in "[PATCH v5 2/5] PCI: designware: Add ARM64 support"
together with the rest of designware rework. 

So we would have always a working version of designware...

Are you ok with this?

> 

> 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@vger.kernel.org [mailto:linux-pci-

> >> owner@vger.kernel.org] On Behalf Of Rob Herring

> >> Sent: Friday, July 31, 2015 5:53 PM

> >> To: Kishon Vijay Abraham I

> >> Cc: Gabriele Paoloni; Bjorn Helgaas; arnd@arndb.de;

> >> lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;

> >> james.morse@arm.com; Liviu.Dudau@arm.com; linux-pci@vger.kernel.org;

> >> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;

> >> 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@ti.com>

> >> wrote:

> >>> +Arnd

> >>>

> >>> Hi,

> >>>

> >>>> On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:

> >>>> [+cc Kishon]

> >>>>

> >>>>> -----Original Message-----

> >>>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-

> >>>>> owner@vger.kernel.org] On Behalf Of Rob Herring

> >>>>> Sent: Thursday, July 30, 2015 9:42 PM

> >>>>> To: Gabriele Paoloni

> >>>>> Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com;

> >> Wangzhou

> >>>>> (B); robh+dt@kernel.org; james.morse@arm.com; Liviu.Dudau@arm.com;

> >>>>> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> >>>>> devicetree@vger.kernel.org; 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@huawei.com> wrote:

> >>>>>>> -----Original Message-----

> >>>>>>> From: Bjorn Helgaas [mailto:bhelgaas@google.com]

> >>>>>>> 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@vger.kernel.org [mailto:linux-pci-

> >>>>>>>>> owner@vger.kernel.org] 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@vger.kernel.org

> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Gabriele Paoloni Aug. 6, 2015, 1:52 p.m. UTC | #3
Hi all

This patch has now been moved in "[PATCH v6 1/6] PCI: designware: move calculation of bus addresses to DRA7xx"

commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated address)" has been reverted in
"[PATCH v6 3/6] PCI: designware: Add ARM64 support"

Gab

> -----Original Message-----

> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-

> owner@vger.kernel.org] On Behalf Of Gabriele Paoloni

> Sent: Tuesday, August 04, 2015 11:12 AM

> To: Jingoo Han

> Cc: Rob Herring; Kishon Vijay Abraham I; Bjorn Helgaas; arnd@arndb.de;

> lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;

> james.morse@arm.com; Liviu.Dudau@arm.com; linux-pci@vger.kernel.org;

> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;

> Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);

> Pratyush Anand; Arnd Bergmann

> Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct

> of_pci_range

> 

> 

> 

> > -----Original Message-----

> > From: Jingoo Han [mailto:jingoohan1@gmail.com]

> > Sent: Tuesday, August 04, 2015 5:20 AM

> > To: Gabriele Paoloni

> > Cc: Rob Herring; Kishon Vijay Abraham I; Bjorn Helgaas; arnd@arndb.de;

> > lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;

> > james.morse@arm.com; Liviu.Dudau@arm.com; linux-pci@vger.kernel.org;

> > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;

> > Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);

> > Pratyush Anand; Arnd Bergmann; jingoohan1@gmail.com

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

> > of_pci_range

> >

> > On 2015. 8. 3., at PM 8:18, Gabriele Paoloni

> > <gabriele.paoloni@huawei.com> 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?

> 

> Hi Jingoo Han,

> 

> If I reverted the commit, in order to get designware working, dra7

> should mask directly the CPU addresses stored in pp->cfg0_base,

> pp->cfg1_base, pp->mem_base and pp->io_base.

> 

> The masking would happen at this point:

> http://lxr.free-electrons.com/source/drivers/pci/host/pcie-

> designware.c#L493

> 

> Now:

> - I see that pp->cfg<0/1>_base are used in devm_ioremap before that

>   point and nowhere else, so they should be ok.

> - pp->mem_base is used in dw_pcie_setup_rc(); dw_pcie_setup_rc() is

> called

>   in dra7xx_pcie_host_init()...so here I should move the masking after

>   dw_pcie_setup() retuned, but I think it should be ok

> - pp->io_base is used in pci_ioremap_io() in dw_pcie_setup(). Now

> currently

>   in designware this is called by pci_bios_init_hw(); this is after the

>   masking....so here we would have a the wrong value passed

> 

> Said that if you look at "[PATCH v5 2/5] PCI: designware: Add ARM64

> support",

> that is the patchset where this patch is needed, you can see that

> dw_pcie_setup() is removed and pp->io_base is used in

> pci_remap_iospace() before

> the masking would happen in dra7xx_pcie_host_init()...so for this

> patchset we

> should be good to revert the commit.

> 

> I propose to add a new patch in the patchset "[PATCH v5 0/5] PCI: hisi:

> Add PCIe

> host support for HiSilicon SoC Hip05" as the one I just posted in this

> thread (this would not revert the commit but just moves the masking to

> dra7xx)

> 

> I would revert the commit in "[PATCH v5 2/5] PCI: designware: Add ARM64

> support"

> together with the rest of designware rework.

> 

> So we would have always a working version of designware...

> 

> Are you ok with this?

> 

> >

> > 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@vger.kernel.org [mailto:linux-pci-

> > >> owner@vger.kernel.org] On Behalf Of Rob Herring

> > >> Sent: Friday, July 31, 2015 5:53 PM

> > >> To: Kishon Vijay Abraham I

> > >> Cc: Gabriele Paoloni; Bjorn Helgaas; arnd@arndb.de;

> > >> lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;

> > >> james.morse@arm.com; Liviu.Dudau@arm.com; linux-

> pci@vger.kernel.org;

> > >> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;

> > >> 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@ti.com>

> > >> wrote:

> > >>> +Arnd

> > >>>

> > >>> Hi,

> > >>>

> > >>>> On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:

> > >>>> [+cc Kishon]

> > >>>>

> > >>>>> -----Original Message-----

> > >>>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-

> > >>>>> owner@vger.kernel.org] On Behalf Of Rob Herring

> > >>>>> Sent: Thursday, July 30, 2015 9:42 PM

> > >>>>> To: Gabriele Paoloni

> > >>>>> Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com;

> > >> Wangzhou

> > >>>>> (B); robh+dt@kernel.org; james.morse@arm.com;

> Liviu.Dudau@arm.com;

> > >>>>> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;

> > >>>>> devicetree@vger.kernel.org; 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@huawei.com> wrote:

> > >>>>>>> -----Original Message-----

> > >>>>>>> From: Bjorn Helgaas [mailto:bhelgaas@google.com]

> > >>>>>>> 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@vger.kernel.org [mailto:linux-pci-

> > >>>>>>>>> owner@vger.kernel.org] 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@vger.kernel.org

> > >> More majordomo info at  http://vger.kernel.org/majordomo-info.html

> ????????+-???w??????ir(???}????j:+v????zZ+??zf?

> ?????i??z?????????f
Han Jingoo Aug. 6, 2015, 3:06 p.m. UTC | #4
On Thursday, August 06, 2015 10:53 PM, Gabriele Paoloni wrote:
> 
> Hi all
> 
> This patch has now been moved in "[PATCH v6 1/6] PCI: designware: move calculation of bus addresses to
> DRA7xx"

To Zhou Wang,

Please send your patches to 'jingoohan1@gmail.com', instead of 'jg1.han@samsung.com'.
Even though, I have done mainline works for Samsung SoCs, Samsung does not support me
as a maintainer. So, please don't send your patches to my Samsung e-mail.

Best regards,
Jingoo Han

> 
> commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated address)" has been reverted in
> "[PATCH v6 3/6] PCI: designware: Add ARM64 support"
> 
> Gab
> 
> > -----Original Message-----
> > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> > owner@vger.kernel.org] On Behalf Of Gabriele Paoloni
> > Sent: Tuesday, August 04, 2015 11:12 AM
> > To: Jingoo Han
> > Cc: Rob Herring; Kishon Vijay Abraham I; Bjorn Helgaas; arnd@arndb.de;
> > lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;
> > james.morse@arm.com; Liviu.Dudau@arm.com; linux-pci@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> > Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);
> > Pratyush Anand; Arnd Bergmann
> > Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct
> > of_pci_range
> >
> >
> >
> > > -----Original Message-----
> > > From: Jingoo Han [mailto:jingoohan1@gmail.com]
> > > Sent: Tuesday, August 04, 2015 5:20 AM
> > > To: Gabriele Paoloni
> > > Cc: Rob Herring; Kishon Vijay Abraham I; Bjorn Helgaas; arnd@arndb.de;
> > > lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;
> > > james.morse@arm.com; Liviu.Dudau@arm.com; linux-pci@vger.kernel.org;
> > > linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> > > Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);
> > > Pratyush Anand; Arnd Bergmann; jingoohan1@gmail.com
> > > Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
> > > of_pci_range
> > >
> > > On 2015. 8. 3., at PM 8:18, Gabriele Paoloni
> > > <gabriele.paoloni@huawei.com> 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?
> >
> > Hi Jingoo Han,
> >
> > If I reverted the commit, in order to get designware working, dra7
> > should mask directly the CPU addresses stored in pp->cfg0_base,
> > pp->cfg1_base, pp->mem_base and pp->io_base.
> >
> > The masking would happen at this point:
> > http://lxr.free-electrons.com/source/drivers/pci/host/pcie-
> > designware.c#L493
> >
> > Now:
> > - I see that pp->cfg<0/1>_base are used in devm_ioremap before that
> >   point and nowhere else, so they should be ok.
> > - pp->mem_base is used in dw_pcie_setup_rc(); dw_pcie_setup_rc() is
> > called
> >   in dra7xx_pcie_host_init()...so here I should move the masking after
> >   dw_pcie_setup() retuned, but I think it should be ok
> > - pp->io_base is used in pci_ioremap_io() in dw_pcie_setup(). Now
> > currently
> >   in designware this is called by pci_bios_init_hw(); this is after the
> >   masking....so here we would have a the wrong value passed
> >
> > Said that if you look at "[PATCH v5 2/5] PCI: designware: Add ARM64
> > support",
> > that is the patchset where this patch is needed, you can see that
> > dw_pcie_setup() is removed and pp->io_base is used in
> > pci_remap_iospace() before
> > the masking would happen in dra7xx_pcie_host_init()...so for this
> > patchset we
> > should be good to revert the commit.
> >
> > I propose to add a new patch in the patchset "[PATCH v5 0/5] PCI: hisi:
> > Add PCIe
> > host support for HiSilicon SoC Hip05" as the one I just posted in this
> > thread (this would not revert the commit but just moves the masking to
> > dra7xx)
> >
> > I would revert the commit in "[PATCH v5 2/5] PCI: designware: Add ARM64
> > support"
> > together with the rest of designware rework.
> >
> > So we would have always a working version of designware...
> >
> > Are you ok with this?
> >
> > >
> > > 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@vger.kernel.org [mailto:linux-pci-
> > > >> owner@vger.kernel.org] On Behalf Of Rob Herring
> > > >> Sent: Friday, July 31, 2015 5:53 PM
> > > >> To: Kishon Vijay Abraham I
> > > >> Cc: Gabriele Paoloni; Bjorn Helgaas; arnd@arndb.de;
> > > >> lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;
> > > >> james.morse@arm.com; Liviu.Dudau@arm.com; linux-
> > pci@vger.kernel.org;
> > > >> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
> > > >> 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@ti.com>
> > > >> wrote:
> > > >>> +Arnd
> > > >>>
> > > >>> Hi,
> > > >>>
> > > >>>> On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:
> > > >>>> [+cc Kishon]
> > > >>>>
> > > >>>>> -----Original Message-----
> > > >>>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> > > >>>>> owner@vger.kernel.org] On Behalf Of Rob Herring
> > > >>>>> Sent: Thursday, July 30, 2015 9:42 PM
> > > >>>>> To: Gabriele Paoloni
> > > >>>>> Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com;
> > > >> Wangzhou
> > > >>>>> (B); robh+dt@kernel.org; james.morse@arm.com;
> > Liviu.Dudau@arm.com;
> > > >>>>> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > >>>>> devicetree@vger.kernel.org; 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@huawei.com> wrote:
> > > >>>>>>> -----Original Message-----
> > > >>>>>>> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
> > > >>>>>>> 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@vger.kernel.org [mailto:linux-pci-
> > > >>>>>>>>> owner@vger.kernel.org] 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@vger.kernel.org
> > > >> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhou Wang Aug. 7, 2015, 5:46 a.m. UTC | #5
On 2015/8/6 23:06, Jingoo Han wrote:
> On Thursday, August 06, 2015 10:53 PM, Gabriele Paoloni wrote:
>>
>> Hi all
>>
>> This patch has now been moved in "[PATCH v6 1/6] PCI: designware: move calculation of bus addresses to
>> DRA7xx"
> 
> To Zhou Wang,
> 
> Please send your patches to 'jingoohan1@gmail.com', instead of 'jg1.han@samsung.com'.
> Even though, I have done mainline works for Samsung SoCs, Samsung does not support me
> as a maintainer. So, please don't send your patches to my Samsung e-mail.
> 
> Best regards,
> Jingoo Han
>

Hi Jingoo,

Sorry for that, I will add jingoohan1@gmail.com to v6 thread.

Thanks for reminding,
Zhou

>>
>> commit "f4c55c5a3f7f (PCI: designware: Program ATU with untranslated address)" has been reverted in
>> "[PATCH v6 3/6] PCI: designware: Add ARM64 support"
>>
>> Gab
>>
>>> -----Original Message-----
>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
>>> owner@vger.kernel.org] On Behalf Of Gabriele Paoloni
>>> Sent: Tuesday, August 04, 2015 11:12 AM
>>> To: Jingoo Han
>>> Cc: Rob Herring; Kishon Vijay Abraham I; Bjorn Helgaas; arnd@arndb.de;
>>> lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;
>>> james.morse@arm.com; Liviu.Dudau@arm.com; linux-pci@vger.kernel.org;
>>> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
>>> Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);
>>> Pratyush Anand; Arnd Bergmann
>>> Subject: RE: [PATCH v6] PCI: Store PCIe bus address in struct
>>> of_pci_range
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Jingoo Han [mailto:jingoohan1@gmail.com]
>>>> Sent: Tuesday, August 04, 2015 5:20 AM
>>>> To: Gabriele Paoloni
>>>> Cc: Rob Herring; Kishon Vijay Abraham I; Bjorn Helgaas; arnd@arndb.de;
>>>> lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;
>>>> james.morse@arm.com; Liviu.Dudau@arm.com; linux-pci@vger.kernel.org;
>>>> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
>>>> Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth);
>>>> Pratyush Anand; Arnd Bergmann; jingoohan1@gmail.com
>>>> Subject: Re: [PATCH v6] PCI: Store PCIe bus address in struct
>>>> of_pci_range
>>>>
>>>> On 2015. 8. 3., at PM 8:18, Gabriele Paoloni
>>>> <gabriele.paoloni@huawei.com> 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?
>>>
>>> Hi Jingoo Han,
>>>
>>> If I reverted the commit, in order to get designware working, dra7
>>> should mask directly the CPU addresses stored in pp->cfg0_base,
>>> pp->cfg1_base, pp->mem_base and pp->io_base.
>>>
>>> The masking would happen at this point:
>>> http://lxr.free-electrons.com/source/drivers/pci/host/pcie-
>>> designware.c#L493
>>>
>>> Now:
>>> - I see that pp->cfg<0/1>_base are used in devm_ioremap before that
>>>   point and nowhere else, so they should be ok.
>>> - pp->mem_base is used in dw_pcie_setup_rc(); dw_pcie_setup_rc() is
>>> called
>>>   in dra7xx_pcie_host_init()...so here I should move the masking after
>>>   dw_pcie_setup() retuned, but I think it should be ok
>>> - pp->io_base is used in pci_ioremap_io() in dw_pcie_setup(). Now
>>> currently
>>>   in designware this is called by pci_bios_init_hw(); this is after the
>>>   masking....so here we would have a the wrong value passed
>>>
>>> Said that if you look at "[PATCH v5 2/5] PCI: designware: Add ARM64
>>> support",
>>> that is the patchset where this patch is needed, you can see that
>>> dw_pcie_setup() is removed and pp->io_base is used in
>>> pci_remap_iospace() before
>>> the masking would happen in dra7xx_pcie_host_init()...so for this
>>> patchset we
>>> should be good to revert the commit.
>>>
>>> I propose to add a new patch in the patchset "[PATCH v5 0/5] PCI: hisi:
>>> Add PCIe
>>> host support for HiSilicon SoC Hip05" as the one I just posted in this
>>> thread (this would not revert the commit but just moves the masking to
>>> dra7xx)
>>>
>>> I would revert the commit in "[PATCH v5 2/5] PCI: designware: Add ARM64
>>> support"
>>> together with the rest of designware rework.
>>>
>>> So we would have always a working version of designware...
>>>
>>> Are you ok with this?
>>>
>>>>
>>>> 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@vger.kernel.org [mailto:linux-pci-
>>>>>> owner@vger.kernel.org] On Behalf Of Rob Herring
>>>>>> Sent: Friday, July 31, 2015 5:53 PM
>>>>>> To: Kishon Vijay Abraham I
>>>>>> Cc: Gabriele Paoloni; Bjorn Helgaas; arnd@arndb.de;
>>>>>> lorenzo.pieralisi@arm.com; Wangzhou (B); robh+dt@kernel.org;
>>>>>> james.morse@arm.com; Liviu.Dudau@arm.com; linux-
>>> pci@vger.kernel.org;
>>>>>> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org;
>>>>>> 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@ti.com>
>>>>>> wrote:
>>>>>>> +Arnd
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>>> On Friday 31 July 2015 07:55 PM, Gabriele Paoloni wrote:
>>>>>>>> [+cc Kishon]
>>>>>>>>
>>>>>>>>> -----Original Message-----
>>>>>>>>> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
>>>>>>>>> owner@vger.kernel.org] On Behalf Of Rob Herring
>>>>>>>>> Sent: Thursday, July 30, 2015 9:42 PM
>>>>>>>>> To: Gabriele Paoloni
>>>>>>>>> Cc: Bjorn Helgaas; arnd@arndb.de; lorenzo.pieralisi@arm.com;
>>>>>> Wangzhou
>>>>>>>>> (B); robh+dt@kernel.org; james.morse@arm.com;
>>> Liviu.Dudau@arm.com;
>>>>>>>>> linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>>>>>>>>> devicetree@vger.kernel.org; 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@huawei.com> wrote:
>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>> From: Bjorn Helgaas [mailto:bhelgaas@google.com]
>>>>>>>>>>> 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@vger.kernel.org [mailto:linux-pci-
>>>>>>>>>>>>> owner@vger.kernel.org] 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@vger.kernel.org
>>>>>> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 		}