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]

 




> -----Original Message-----
> From: Jingoo Han [mailto:jingoohan1@xxxxxxxxx]
> Sent: Tuesday, August 04, 2015 5:20 AM
> To: Gabriele Paoloni
> Cc: Rob Herring; Kishon Vijay Abraham I; 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);
> Pratyush Anand; Arnd Bergmann; jingoohan1@xxxxxxxxx
> 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@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?

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@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
ÿ淸º{.nÇ+돴윯돪†+%듚ÿ깁負¥Šwÿº{.nÇ+돴¥Š{깸—"þ)í끾èw*jgП¨¶‰šŽ듶¢jÿ¾?G«앶ÿ◀◁¦j:+v돣ŠwèjØm¶Ÿÿ?®w?듺þf"·hš뤴얎ÿ녪¥




[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