[PATCH v4 0/5] Qualcomm PCIe driver and designware fixes

[PATCH v4 0/5] Qualcomm PCIe driver and designware fixes

[Stanimir Varbanov]

Hi,

Here is v4, comments from Bjorn and Rob have been addressed.
The previous version can be found at [1].

regards,
Stan

[1] https://lkml.org/lkml/2015/11/23/114

Stanimir Varbanov (5):
  PCI: designware: add memory barrier after enabling region
  DT: PCI: qcom: Document PCIe devicetree bindings
  PCI: qcom: Add Qualcomm PCIe controller driver
  ARM: dts: apq8064: add pcie devicetree node
  ARM: dts: ifc6410: enable pcie dt node for this board

 .../devicetree/bindings/pci/qcom,pcie.txt          |  233 ++++++++
 MAINTAINERS                                        |    7 +
 arch/arm/boot/dts/qcom-apq8064-ifc6410.dts         |   26 +
 arch/arm/boot/dts/qcom-apq8064.dtsi                |   36 ++
 drivers/pci/host/Kconfig                           |   10 +
 drivers/pci/host/Makefile                          |    1 +
 drivers/pci/host/pcie-designware.c                 |    5 +
 drivers/pci/host/pcie-qcom.c                       |  624 ++++++++++++++++++++
 8 files changed, 942 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt
 create mode 100644 drivers/pci/host/pcie-qcom.c

-- 
1.7.9.5

[PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

[Stanimir Varbanov]

Add 'write memory' barrier after enable region in PCIE_ATU_CR2
register. The barrier is needed to ensure that the region enable
request has been reached it's destination at time when we
read/write to PCI configuration space.

Without this barrier PCI device enumeration during kernel boot
is not reliable, and reading configuration space for particular
PCI device on the bus returns zero aka no device.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 drivers/pci/host/pcie-designware.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 02a7452bdf23..ed4dc2e2553b 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -164,6 +164,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index,
 	dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET);
 	dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1);
 	dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
+	/*
+	 * ensure that the ATU enable has been happaned before accessing
+	 * pci configuration/io spaces through dw_pcie_cfg_[read|write].
+	 */
+	wmb();
 }
 
 static struct irq_chip dw_msi_irq_chip = {
-- 
1.7.9.5

Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

[Stanimir Varbanov]

On 12/03/2015 03:35 PM, Stanimir Varbanov wrote:
> Add 'write memory' barrier after enable region in PCIE_ATU_CR2
> register. The barrier is needed to ensure that the region enable
> request has been reached it's destination at time when we
> read/write to PCI configuration space.
> 
> Without this barrier PCI device enumeration during kernel boot
> is not reliable, and reading configuration space for particular
> PCI device on the bus returns zero aka no device.

Anand, Jingoo, what is your opinion?

> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/pci/host/pcie-designware.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 02a7452bdf23..ed4dc2e2553b 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -164,6 +164,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index,
>  	dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET);
>  	dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1);
>  	dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> +	/*
> +	 * ensure that the ATU enable has been happaned before accessing
> +	 * pci configuration/io spaces through dw_pcie_cfg_[read|write].
> +	 */
> +	wmb();
>  }
>  
>  static struct irq_chip dw_msi_irq_chip = {
> 


-- 
regards,
Stan

Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

[Pratyush Anand]

On Tue, Dec 8, 2015 at 2:31 PM, Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> On 12/03/2015 03:35 PM, Stanimir Varbanov wrote:
> > Add 'write memory' barrier after enable region in PCIE_ATU_CR2
> > register. The barrier is needed to ensure that the region enable
> > request has been reached it's destination at time when we
> > read/write to PCI configuration space.
> >
> > Without this barrier PCI device enumeration during kernel boot
> > is not reliable, and reading configuration space for particular
> > PCI device on the bus returns zero aka no device.
>
> Anand, Jingoo, what is your opinion?
>
> >
> > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > ---
> >  drivers/pci/host/pcie-designware.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > index 02a7452bdf23..ed4dc2e2553b 100644
> > --- a/drivers/pci/host/pcie-designware.c
> > +++ b/drivers/pci/host/pcie-designware.c
> > @@ -164,6 +164,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index,
> >       dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET);
> >       dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1);
> >       dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> > +     /*
> > +      * ensure that the ATU enable has been happaned before accessing
> > +      * pci configuration/io spaces through dw_pcie_cfg_[read|write].
> > +      */
> > +     wmb();
> >  }
> >


My understnading is that since writel() of dw_pcie_writel_rc() in
above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
will follow) goes through same device (ie PCIe host here). So, it is
guaranteed that 1st writel() will be executed before later
readl()/writel(). If that is true then we do not need any explicit
barrier here.

Arnd, Russel: whats your opinion here.

~Pratyush

Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

[Arnd Bergmann]

On Wednesday 09 December 2015 10:10:05 Pratyush Anand wrote:
> On Tue, Dec 8, 2015 at 2:31 PM, Stanimir Varbanov
> > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > > ---
> > >  drivers/pci/host/pcie-designware.c |    5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > > index 02a7452bdf23..ed4dc2e2553b 100644
> > > --- a/drivers/pci/host/pcie-designware.c
> > > +++ b/drivers/pci/host/pcie-designware.c
> > > @@ -164,6 +164,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index,
> > >       dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET);
> > >       dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1);
> > >       dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> > > +     /*
> > > +      * ensure that the ATU enable has been happaned before accessing
> > > +      * pci configuration/io spaces through dw_pcie_cfg_[read|write].
> > > +      */
> > > +     wmb();
> > >  }
> > >
> 
> 
> My understnading is that since writel() of dw_pcie_writel_rc() in
> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
> will follow) goes through same device (ie PCIe host here). So, it is
> guaranteed that 1st writel() will be executed before later
> readl()/writel(). If that is true then we do not need any explicit
> barrier here.
> 
> Arnd, Russel: whats your opinion here.

I think the ordering is only enforced if the two register accesses are
on the same device as seen from the bus, and it's possible that the
RC registers and the config space registers are not considered the
same thing here.

For config write, this is not a problem, because the config space write
has a wmb() that enforces ordering, but it's possible that the config
space read may hit the device in parallel with the PCIE_ATU_ENABLE
write.

	Arnd

Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

[Stanimir Varbanov]

On 12/09/2015 11:52 AM, Arnd Bergmann wrote:
> On Wednesday 09 December 2015 10:10:05 Pratyush Anand wrote:
>> On Tue, Dec 8, 2015 at 2:31 PM, Stanimir Varbanov
>>>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>>>> ---
>>>>  drivers/pci/host/pcie-designware.c |    5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>>>> index 02a7452bdf23..ed4dc2e2553b 100644
>>>> --- a/drivers/pci/host/pcie-designware.c
>>>> +++ b/drivers/pci/host/pcie-designware.c
>>>> @@ -164,6 +164,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index,
>>>>       dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET);
>>>>       dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1);
>>>>       dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
>>>> +     /*
>>>> +      * ensure that the ATU enable has been happaned before accessing
>>>> +      * pci configuration/io spaces through dw_pcie_cfg_[read|write].
>>>> +      */
>>>> +     wmb();
>>>>  }
>>>>
>>
>>
>> My understnading is that since writel() of dw_pcie_writel_rc() in
>> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
>> will follow) goes through same device (ie PCIe host here). So, it is
>> guaranteed that 1st writel() will be executed before later
>> readl()/writel(). If that is true then we do not need any explicit
>> barrier here.
>>
>> Arnd, Russel: whats your opinion here.
> 
> I think the ordering is only enforced if the two register accesses are
> on the same device as seen from the bus, and it's possible that the
> RC registers and the config space registers are not considered the
> same thing here.
> 
> For config write, this is not a problem, because the config space write
> has a wmb() that enforces ordering, but it's possible that the config
> space read may hit the device in parallel with the PCIE_ATU_ENABLE
> write.

Hmm, just a matter of fact - as I described in the patch description
this wmb() fixed an issue with pcie device enumeration (I came down to
pci_bus_read_dev_vendor_id() returns zero) i.e. exactly a pci
configuration space read.

-- 
regards,
Stan

Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

[Russell King - ARM Linux]

On Wed, Dec 09, 2015 at 10:10:05AM +0530, Pratyush Anand wrote:
> On Tue, Dec 8, 2015 at 2:31 PM, Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
> >
> > On 12/03/2015 03:35 PM, Stanimir Varbanov wrote:
> > > Add 'write memory' barrier after enable region in PCIE_ATU_CR2
> > > register. The barrier is needed to ensure that the region enable
> > > request has been reached it's destination at time when we
> > > read/write to PCI configuration space.
> > >
> > > Without this barrier PCI device enumeration during kernel boot
> > > is not reliable, and reading configuration space for particular
> > > PCI device on the bus returns zero aka no device.
> >
> > Anand, Jingoo, what is your opinion?
> >
> > >
> > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > > ---
> > >  drivers/pci/host/pcie-designware.c |    5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > > index 02a7452bdf23..ed4dc2e2553b 100644
> > > --- a/drivers/pci/host/pcie-designware.c
> > > +++ b/drivers/pci/host/pcie-designware.c
> > > @@ -164,6 +164,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port *pp, int index,
> > >       dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET);
> > >       dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1);
> > >       dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> > > +     /*
> > > +      * ensure that the ATU enable has been happaned before accessing
> > > +      * pci configuration/io spaces through dw_pcie_cfg_[read|write].
> > > +      */
> > > +     wmb();
> > >  }
> > >
> 
> 
> My understnading is that since writel() of dw_pcie_writel_rc() in
> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
> will follow) goes through same device (ie PCIe host here). So, it is
> guaranteed that 1st writel() will be executed before later
> readl()/writel(). If that is true then we do not need any explicit
> barrier here.
> 
> Arnd, Russel: whats your opinion here.
              ^l

writel() has a barrier _before_ the access but not after.

The fact is that there's nothing which guarantees that the write will hit
the hardware in a timely manner (forget any rules about PCI config space,
the PCI ordering rules apply to the PCI bus, not to the ARM buses.)

If you need this write to have hit the hardware before continuing, you
need to read back from the same register.

I'm just looking at this driver, trying to decipher what it's doing.  It
_looks_ to me like it's reprogramming one of the outbound windows (IO?)
so that configuration space can be accessed.  Doesn't this have the
effect of disabling access to the IO segment of the PCI bus from the
host CPU?

What protections are there against other CPUs in the system issuing a
PCI I/O read/write while this outbound window is programmed as
configuration space?

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

[Pratyush Anand]

On Wed, Dec 9, 2015 at 3:53 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

[...]

>> > >       dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
>> > > +     /*
>> > > +      * ensure that the ATU enable has been happaned before accessing
>> > > +      * pci configuration/io spaces through dw_pcie_cfg_[read|write].
>> > > +      */
>> > > +     wmb();
>> > >  }
>> > >
>>
>>
>> My understnading is that since writel() of dw_pcie_writel_rc() in
>> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
>> will follow) goes through same device (ie PCIe host here). So, it is
>> guaranteed that 1st writel() will be executed before later
>> readl()/writel(). If that is true then we do not need any explicit
>> barrier here.
>>
>> Arnd, Russel: whats your opinion here.
>               ^l

Sorry :(

>
> writel() has a barrier _before_ the access but not after.
>
> The fact is that there's nothing which guarantees that the write will hit
> the hardware in a timely manner (forget any rules about PCI config space,
> the PCI ordering rules apply to the PCI bus, not to the ARM buses.)
>
> If you need this write to have hit the hardware before continuing, you
> need to read back from the same register.

OK, so better to replace wmb() with read back of control register.

>
> I'm just looking at this driver, trying to decipher what it's doing.  It
> _looks_ to me like it's reprogramming one of the outbound windows (IO?)
> so that configuration space can be accessed.  Doesn't this have the
> effect of disabling access to the IO segment of the PCI bus from the
> host CPU?
>
> What protections are there against other CPUs in the system issuing a
> PCI I/O read/write while this outbound window is programmed as
> configuration space?


Yes, that is an issue with this driver. Most of the host controller
has 4 or more viewpoints, and it is very easy to handle for them. But
there are few which has only two viewpoints. Do not know how to solve
it, so that it works for all.

~Pratyush

Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

[Jisheng Zhang]

On Fri, 11 Dec 2015 09:35:10 +0530 Pratyush Anand wrote:

> On Wed, Dec 9, 2015 at 3:53 PM, Russell King - ARM Linux wrote:
> 
> [...]
> 
> >> > >       dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> >> > > +     /*
> >> > > +      * ensure that the ATU enable has been happaned before accessing
> >> > > +      * pci configuration/io spaces through dw_pcie_cfg_[read|write].
> >> > > +      */
> >> > > +     wmb();
> >> > >  }
> >> > >  
> >>
> >>
> >> My understnading is that since writel() of dw_pcie_writel_rc() in
> >> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
> >> will follow) goes through same device (ie PCIe host here). So, it is
> >> guaranteed that 1st writel() will be executed before later
> >> readl()/writel(). If that is true then we do not need any explicit
> >> barrier here.
> >>
> >> Arnd, Russel: whats your opinion here.  
> >               ^l  
> 
> Sorry :(
> 
> >
> > writel() has a barrier _before_ the access but not after.
> >
> > The fact is that there's nothing which guarantees that the write will hit
> > the hardware in a timely manner (forget any rules about PCI config space,
> > the PCI ordering rules apply to the PCI bus, not to the ARM buses.)
> >
> > If you need this write to have hit the hardware before continuing, you
> > need to read back from the same register.  
> 
> OK, so better to replace wmb() with read back of control register.
> 
> >
> > I'm just looking at this driver, trying to decipher what it's doing.  It
> > _looks_ to me like it's reprogramming one of the outbound windows (IO?)
> > so that configuration space can be accessed.  Doesn't this have the
> > effect of disabling access to the IO segment of the PCI bus from the
> > host CPU?
> >
> > What protections are there against other CPUs in the system issuing a
> > PCI I/O read/write while this outbound window is programmed as
> > configuration space?  
> 
> 
> Yes, that is an issue with this driver. Most of the host controller
> has 4 or more viewpoints, and it is very easy to handle for them. But
> there are few which has only two viewpoints. Do not know how to solve
> it, so that it works for all.
> 

The default outbound iATU number is two, this may be the reason why the driver
is written in current style. And two outbound iATUs may be common for pcie dw
users because ASIC people just follow the default configuration ;).

In our case, Marvell Berlin SoCs have two outbound iATUs.

Thanks,
Jisheng

Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

[Jingoo Han]

On Friday, December 11, 2015 2:49 PM, Jisheng Zhang wrote:
> 
> On Fri, 11 Dec 2015 09:35:10 +0530 Pratyush Anand wrote:
> 
> > On Wed, Dec 9, 2015 at 3:53 PM, Russell King - ARM Linux wrote:
> >
> > [...]
> >
> > >> > >       dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> > >> > > +     /*
> > >> > > +      * ensure that the ATU enable has been happaned before accessing
> > >> > > +      * pci configuration/io spaces through dw_pcie_cfg_[read|write].
> > >> > > +      */
> > >> > > +     wmb();
> > >> > >  }
> > >> > >
> > >>
> > >>
> > >> My understnading is that since writel() of dw_pcie_writel_rc() in
> > >> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
> > >> will follow) goes through same device (ie PCIe host here). So, it is
> > >> guaranteed that 1st writel() will be executed before later
> > >> readl()/writel(). If that is true then we do not need any explicit
> > >> barrier here.
> > >>
> > >> Arnd, Russel: whats your opinion here.
> > >               ^l
> >
> > Sorry :(
> >
> > >
> > > writel() has a barrier _before_ the access but not after.
> > >
> > > The fact is that there's nothing which guarantees that the write will hit
> > > the hardware in a timely manner (forget any rules about PCI config space,
> > > the PCI ordering rules apply to the PCI bus, not to the ARM buses.)
> > >
> > > If you need this write to have hit the hardware before continuing, you
> > > need to read back from the same register.
> >
> > OK, so better to replace wmb() with read back of control register.
> >
> > >
> > > I'm just looking at this driver, trying to decipher what it's doing.  It
> > > _looks_ to me like it's reprogramming one of the outbound windows (IO?)
> > > so that configuration space can be accessed.  Doesn't this have the
> > > effect of disabling access to the IO segment of the PCI bus from the
> > > host CPU?
> > >
> > > What protections are there against other CPUs in the system issuing a
> > > PCI I/O read/write while this outbound window is programmed as
> > > configuration space?
> >
> >
> > Yes, that is an issue with this driver. Most of the host controller
> > has 4 or more viewpoints, and it is very easy to handle for them. But
> > there are few which has only two viewpoints. Do not know how to solve
> > it, so that it works for all.
> >
> 
> The default outbound iATU number is two, this may be the reason why the driver
> is written in current style. And two outbound iATUs may be common for pcie dw
> users because ASIC people just follow the default configuration ;).
> 
> In our case, Marvell Berlin SoCs have two outbound iATUs.

Hmm, we need to add new DT property to handle the number of outbound iATUs.
Then, 'pcie-designware.c' should configure registers according to the number.
Anyway, we should add this agenda to ToDo list.

Best regards,
Jingoo Han

> 
> Thanks,
> Jisheng

Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

[Stanimir Varbanov]

On 12/11/2015 06:05 AM, Pratyush Anand wrote:
> On Wed, Dec 9, 2015 at 3:53 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> 
> [...]
> 
>>>>>       dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
>>>>> +     /*
>>>>> +      * ensure that the ATU enable has been happaned before accessing
>>>>> +      * pci configuration/io spaces through dw_pcie_cfg_[read|write].
>>>>> +      */
>>>>> +     wmb();
>>>>>  }
>>>>>
>>>
>>>
>>> My understnading is that since writel() of dw_pcie_writel_rc() in
>>> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
>>> will follow) goes through same device (ie PCIe host here). So, it is
>>> guaranteed that 1st writel() will be executed before later
>>> readl()/writel(). If that is true then we do not need any explicit
>>> barrier here.
>>>
>>> Arnd, Russel: whats your opinion here.
>>               ^l
> 
> Sorry :(
> 
>>
>> writel() has a barrier _before_ the access but not after.
>>
>> The fact is that there's nothing which guarantees that the write will hit
>> the hardware in a timely manner (forget any rules about PCI config space,
>> the PCI ordering rules apply to the PCI bus, not to the ARM buses.)
>>
>> If you need this write to have hit the hardware before continuing, you
>> need to read back from the same register.
> 
> OK, so better to replace wmb() with read back of control register.

Would the patch be acceptable if I replace wmb with read?

-- 
regards,
Stan

Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region

[Pratyush Anand]

On Thu, Dec 17, 2015 at 9:15 PM, Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> On 12/11/2015 06:05 AM, Pratyush Anand wrote:
> > On Wed, Dec 9, 2015 at 3:53 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> >
> > [...]
> >
> >>>>>       dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2);
> >>>>> +     /*
> >>>>> +      * ensure that the ATU enable has been happaned before accessing
> >>>>> +      * pci configuration/io spaces through dw_pcie_cfg_[read|write].
> >>>>> +      */
> >>>>> +     wmb();
> >>>>>  }
> >>>>>
> >>>
> >>>
> >>> My understnading is that since writel() of dw_pcie_writel_rc() in
> >>> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which
> >>> will follow) goes through same device (ie PCIe host here). So, it is
> >>> guaranteed that 1st writel() will be executed before later
> >>> readl()/writel(). If that is true then we do not need any explicit
> >>> barrier here.
> >>>
> >>> Arnd, Russel: whats your opinion here.
> >>               ^l
> >
> > Sorry :(
> >
> >>
> >> writel() has a barrier _before_ the access but not after.
> >>
> >> The fact is that there's nothing which guarantees that the write will hit
> >> the hardware in a timely manner (forget any rules about PCI config space,
> >> the PCI ordering rules apply to the PCI bus, not to the ARM buses.)
> >>
> >> If you need this write to have hit the hardware before continuing, you
> >> need to read back from the same register.
> >
> > OK, so better to replace wmb() with read back of control register.
>
> Would the patch be acceptable if I replace wmb with read?

For me it would be fine.

~Pratyush

[PATCH v4 3/5] PCI: qcom: Add Qualcomm PCIe controller driver

[Stanimir Varbanov]

From: Stanimir Varbanov <svarbanov@mm-sol.com>

The PCIe driver reuse the Designware common code for host
and MSI initialization, and also program the Qualcomm
application specific registers.

Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 MAINTAINERS                  |    7 +
 drivers/pci/host/Kconfig     |   10 +
 drivers/pci/host/Makefile    |    1 +
 drivers/pci/host/pcie-qcom.c |  624 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 642 insertions(+)
 create mode 100644 drivers/pci/host/pcie-qcom.c

diff --git a/MAINTAINERS b/MAINTAINERS
index cba790b42f23..c07377148399 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8247,6 +8247,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
 F:	drivers/pci/host/pcie-hisi.c
 
+PCIE DRIVER FOR QUALCOMM MSM
+M:     Stanimir Varbanov <svarbanov@mm-sol.com>
+L:     linux-pci@vger.kernel.org
+L:     linux-arm-msm@vger.kernel.org
+S:     Maintained
+F:     drivers/pci/host/*qcom*
+
 PCMCIA SUBSYSTEM
 P:	Linux PCMCIA Team
 L:	linux-pcmcia@lists.infradead.org
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index f131ba947dc6..64d33ba3e336 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -172,4 +172,14 @@ config PCI_HISI
 	help
 	  Say Y here if you want PCIe controller support on HiSilicon HIP05 SoC
 
+config PCIE_QCOM
+	bool "Qualcomm PCIe controller"
+	depends on ARCH_QCOM && OF || COMPILE_TEST
+	select PCIE_DW
+	select PCIEPORTBUS
+	help
+	  Say Y here to enable PCIe controller support on Qualcomm SoCs. The
+	  PCIe controller use Designware core plus Qualcomm specific hardware
+	  wrappers.
+
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9d4d3c6924a1..e523c171febf 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -20,3 +20,4 @@ obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
 obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
 obj-$(CONFIG_PCIE_ALTERA_MSI) += pcie-altera-msi.o
 obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
+obj-$(CONFIG_PCIE_QCOM) += pcie-qcom.o
diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
new file mode 100644
index 000000000000..7d9b7f14354b
--- /dev/null
+++ b/drivers/pci/host/pcie-qcom.c
@@ -0,0 +1,624 @@
+/*
+ * Copyright (c) 2014-2015, The Linux Foundation. All rights reserved.
+ * Copyright 2015 Linaro Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/phy/phy.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+
+#define PCIE20_PARF_PHY_CTRL			0x40
+#define PCIE20_PARF_PHY_REFCLK			0x4C
+#define PCIE20_PARF_DBI_BASE_ADDR		0x168
+#define PCIE20_PARF_SLV_ADDR_SPACE_SIZE		0x16c
+#define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
+
+#define PCIE20_ELBI_SYS_CTRL			0x04
+#define PCIE20_ELBI_SYS_CTRL_LT_ENABLE		BIT(0)
+
+#define PCIE20_ELBI_SYS_STTS			0x08
+#define XMLH_LINK_UP				BIT(10)
+
+#define PCIE20_CAP				0x70
+#define PCIE20_CAP_LINKCTRLSTATUS		(PCIE20_CAP + 0x10)
+#define PCIE20_CAP_LINKCTRLSTATUS_LINK_UP	BIT(29)
+
+#define PERST_DELAY_US				1000
+
+#define LINKUP_DELAY_US				5000
+#define LINKUP_TIMEOUT_US			100000
+
+struct qcom_pcie_resources_v0 {
+	struct clk *iface_clk;
+	struct clk *core_clk;
+	struct clk *phy_clk;
+	struct reset_control *pci_reset;
+	struct reset_control *axi_reset;
+	struct reset_control *ahb_reset;
+	struct reset_control *por_reset;
+	struct reset_control *phy_reset;
+	struct regulator *vdda;
+	struct regulator *vdda_phy;
+	struct regulator *vdda_refclk;
+};
+
+struct qcom_pcie_resources_v1 {
+	struct clk *iface;
+	struct clk *aux;
+	struct clk *master_bus;
+	struct clk *slave_bus;
+	struct reset_control *core;
+	struct regulator *vdda;
+};
+
+union qcom_pcie_resources {
+	struct qcom_pcie_resources_v0 v0;
+	struct qcom_pcie_resources_v1 v1;
+};
+
+struct qcom_pcie;
+
+struct qcom_pcie_ops {
+	int (*get_resources)(struct qcom_pcie *pcie);
+	int (*init)(struct qcom_pcie *pcie);
+	void (*deinit)(struct qcom_pcie *pcie);
+};
+
+struct qcom_pcie {
+	struct pcie_port pp;
+	struct device *dev;
+	union qcom_pcie_resources res;
+	void __iomem *parf;
+	void __iomem *dbi;
+	void __iomem *elbi;
+	struct phy *phy;
+	struct gpio_desc *reset;
+	struct qcom_pcie_ops *ops;
+};
+
+#define to_qcom_pcie(x)		container_of(x, struct qcom_pcie, pp)
+
+static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
+{
+	gpiod_set_value(pcie->reset, 1);
+	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
+}
+
+static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
+{
+	gpiod_set_value(pcie->reset, 0);
+	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
+}
+
+static irqreturn_t qcom_pcie_msi_irq_handler(int irq, void *arg)
+{
+	struct pcie_port *pp = arg;
+
+	return dw_handle_msi_irq(pp);
+}
+
+static int qcom_pcie_enable_link_training(struct qcom_pcie *pcie)
+{
+	struct device *dev = pcie->dev;
+	u32 val;
+	int ret;
+
+	/* enable link training */
+	val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL);
+	val |= PCIE20_ELBI_SYS_CTRL_LT_ENABLE;
+	writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
+
+	/* wait for up to 100ms for the link to come up */
+	ret = readl_poll_timeout(pcie->elbi + PCIE20_ELBI_SYS_STTS, val,
+				 val & XMLH_LINK_UP, LINKUP_DELAY_US,
+				 LINKUP_TIMEOUT_US);
+
+	if (ret < 0 || !dw_pcie_link_up(&pcie->pp)) {
+		dev_err(dev, "link initialization failed\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static int qcom_pcie_get_resources_v0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
+	struct device *dev = pcie->dev;
+
+	res->vdda = devm_regulator_get(dev, "vdda");
+	if (IS_ERR(res->vdda))
+		return PTR_ERR(res->vdda);
+
+	res->vdda_phy = devm_regulator_get(dev, "vdda_phy");
+	if (IS_ERR(res->vdda_phy))
+		return PTR_ERR(res->vdda_phy);
+
+	res->vdda_refclk = devm_regulator_get(dev, "vdda_refclk");
+	if (IS_ERR(res->vdda_refclk))
+		return PTR_ERR(res->vdda_refclk);
+
+	res->iface_clk = devm_clk_get(dev, "iface");
+	if (IS_ERR(res->iface_clk))
+		return PTR_ERR(res->iface_clk);
+
+	res->core_clk = devm_clk_get(dev, "core");
+	if (IS_ERR(res->core_clk))
+		return PTR_ERR(res->core_clk);
+
+	res->phy_clk = devm_clk_get(dev, "phy");
+	if (IS_ERR(res->phy_clk))
+		return PTR_ERR(res->phy_clk);
+
+	res->pci_reset = devm_reset_control_get(dev, "pci");
+	if (IS_ERR(res->pci_reset))
+		return PTR_ERR(res->pci_reset);
+
+	res->axi_reset = devm_reset_control_get(dev, "axi");
+	if (IS_ERR(res->axi_reset))
+		return PTR_ERR(res->axi_reset);
+
+	res->ahb_reset = devm_reset_control_get(dev, "ahb");
+	if (IS_ERR(res->ahb_reset))
+		return PTR_ERR(res->ahb_reset);
+
+	res->por_reset = devm_reset_control_get(dev, "por");
+	if (IS_ERR(res->por_reset))
+		return PTR_ERR(res->por_reset);
+
+	res->phy_reset = devm_reset_control_get(dev, "phy");
+	if (IS_ERR(res->phy_reset))
+		return PTR_ERR(res->phy_reset);
+
+	return 0;
+}
+
+static int qcom_pcie_get_resources_v1(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
+	struct device *dev = pcie->dev;
+
+	res->vdda = devm_regulator_get(dev, "vdda");
+	if (IS_ERR(res->vdda))
+		return PTR_ERR(res->vdda);
+
+	res->iface = devm_clk_get(dev, "iface");
+	if (IS_ERR(res->iface))
+		return PTR_ERR(res->iface);
+
+	res->aux = devm_clk_get(dev, "aux");
+	if (IS_ERR(res->aux))
+		return PTR_ERR(res->aux);
+
+	res->master_bus = devm_clk_get(dev, "master_bus");
+	if (IS_ERR(res->master_bus))
+		return PTR_ERR(res->master_bus);
+
+	res->slave_bus = devm_clk_get(dev, "slave_bus");
+	if (IS_ERR(res->slave_bus))
+		return PTR_ERR(res->slave_bus);
+
+	res->core = devm_reset_control_get(dev, "core");
+	if (IS_ERR(res->core))
+		return PTR_ERR(res->core);
+
+	return 0;
+}
+
+static void qcom_pcie_deinit_v0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
+
+	reset_control_assert(res->pci_reset);
+	reset_control_assert(res->axi_reset);
+	reset_control_assert(res->ahb_reset);
+	reset_control_assert(res->por_reset);
+	reset_control_assert(res->pci_reset);
+	clk_disable_unprepare(res->iface_clk);
+	clk_disable_unprepare(res->core_clk);
+	clk_disable_unprepare(res->phy_clk);
+	regulator_disable(res->vdda);
+	regulator_disable(res->vdda_phy);
+	regulator_disable(res->vdda_refclk);
+}
+
+static int qcom_pcie_init_v0(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v0 *res = &pcie->res.v0;
+	struct device *dev = pcie->dev;
+	u32 val;
+	int ret;
+
+	ret = regulator_enable(res->vdda);
+	if (ret) {
+		dev_err(dev, "cannot enable vdda regulator\n");
+		return ret;
+	}
+
+	ret = regulator_enable(res->vdda_refclk);
+	if (ret) {
+		dev_err(dev, "cannot enable vdda_refclk regulator\n");
+		goto err_refclk;
+	}
+
+	ret = regulator_enable(res->vdda_phy);
+	if (ret) {
+		dev_err(dev, "cannot enable vdda_phy regulator\n");
+		goto err_vdda_phy;
+	}
+
+	ret = reset_control_assert(res->ahb_reset);
+	if (ret) {
+		dev_err(dev, "cannot assert ahb reset\n");
+		goto err_assert_ahb;
+	}
+
+	ret = clk_prepare_enable(res->iface_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable iface clock\n");
+		goto err_assert_ahb;
+	}
+
+	ret = clk_prepare_enable(res->phy_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable phy clock\n");
+		goto err_clk_phy;
+	}
+
+	ret = clk_prepare_enable(res->core_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable core clock\n");
+		goto err_clk_core;
+	}
+
+	ret = reset_control_deassert(res->ahb_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert ahb reset\n");
+		goto err_deassert_ahb;
+	}
+
+	/* enable PCIe clocks and resets */
+	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
+	val &= ~BIT(0);
+	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+
+	/* enable external reference clock */
+	val = readl(pcie->parf + PCIE20_PARF_PHY_REFCLK);
+	val |= BIT(16);
+	writel(val, pcie->parf + PCIE20_PARF_PHY_REFCLK);
+
+	ret = reset_control_deassert(res->phy_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert phy reset\n");
+		return ret;
+	}
+
+	ret = reset_control_deassert(res->pci_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert pci reset\n");
+		return ret;
+	}
+
+	ret = reset_control_deassert(res->por_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert por reset\n");
+		return ret;
+	}
+
+	ret = reset_control_deassert(res->axi_reset);
+	if (ret) {
+		dev_err(dev, "cannot deassert axi reset\n");
+		return ret;
+	}
+
+	/* wait for clock acquisition */
+	usleep_range(1000, 1500);
+
+	return 0;
+
+err_deassert_ahb:
+	clk_disable_unprepare(res->core_clk);
+err_clk_core:
+	clk_disable_unprepare(res->phy_clk);
+err_clk_phy:
+	clk_disable_unprepare(res->iface_clk);
+err_assert_ahb:
+	regulator_disable(res->vdda_phy);
+err_vdda_phy:
+	regulator_disable(res->vdda_refclk);
+err_refclk:
+	regulator_disable(res->vdda);
+
+	return ret;
+}
+
+static void qcom_pcie_deinit_v1(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
+
+	reset_control_assert(res->core);
+	clk_disable_unprepare(res->slave_bus);
+	clk_disable_unprepare(res->master_bus);
+	clk_disable_unprepare(res->iface);
+	clk_disable_unprepare(res->aux);
+	regulator_disable(res->vdda);
+}
+
+static int qcom_pcie_init_v1(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v1 *res = &pcie->res.v1;
+	struct device *dev = pcie->dev;
+	int ret;
+
+	ret = reset_control_deassert(res->core);
+	if (ret) {
+		dev_err(dev, "cannot deassert core reset\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(res->aux);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable aux clock\n");
+		goto err_res;
+	}
+
+	ret = clk_prepare_enable(res->iface);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable iface clock\n");
+		goto err_aux;
+	}
+
+	ret = clk_prepare_enable(res->master_bus);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable master_bus clock\n");
+		goto err_iface;
+	}
+
+	ret = clk_prepare_enable(res->slave_bus);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable slave_bus clock\n");
+		goto err_master;
+	}
+
+	ret = regulator_enable(res->vdda);
+	if (ret) {
+		dev_err(dev, "cannot enable vdda regulator\n");
+		goto err_slave;
+	}
+
+
+	/* change DBI base address */
+	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		u32 val = readl(pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+
+		val |= BIT(31);
+		writel(val, pcie->parf + PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+	}
+
+	return 0;
+err_slave:
+	clk_disable_unprepare(res->slave_bus);
+err_master:
+	clk_disable_unprepare(res->master_bus);
+err_iface:
+	clk_disable_unprepare(res->iface);
+err_aux:
+	clk_disable_unprepare(res->aux);
+err_res:
+	reset_control_assert(res->core);
+
+	return ret;
+}
+
+static int qcom_pcie_link_up(struct pcie_port *pp)
+{
+	struct qcom_pcie *pcie = to_qcom_pcie(pp);
+	u32 val = readl(pcie->dbi + PCIE20_CAP_LINKCTRLSTATUS);
+
+	return !!(val & PCIE20_CAP_LINKCTRLSTATUS_LINK_UP);
+}
+
+static void qcom_pcie_host_init(struct pcie_port *pp)
+{
+	struct qcom_pcie *pcie = to_qcom_pcie(pp);
+	int ret;
+
+	qcom_ep_reset_assert(pcie);
+
+	ret = pcie->ops->init(pcie);
+	if (ret)
+		goto err_deinit;
+
+	ret = phy_power_on(pcie->phy);
+	if (ret)
+		goto err_deinit;
+
+	dw_pcie_setup_rc(pp);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		dw_pcie_msi_init(pp);
+
+	qcom_ep_reset_deassert(pcie);
+
+	ret = qcom_pcie_enable_link_training(pcie);
+	if (ret)
+		goto err;
+
+	return;
+err:
+	qcom_ep_reset_assert(pcie);
+	phy_power_off(pcie->phy);
+err_deinit:
+	pcie->ops->deinit(pcie);
+}
+
+static int
+qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, u32 *val)
+{
+	/* the device class is not reported correctly from the register */
+	if (where == PCI_CLASS_REVISION && size == 4) {
+		*val = readl(pp->dbi_base + PCI_CLASS_REVISION);
+		*val &= 0xff;	/* keep revision id */
+		*val |= PCI_CLASS_BRIDGE_PCI << 16;
+		return PCIBIOS_SUCCESSFUL;
+	}
+
+	return dw_pcie_cfg_read(pp->dbi_base + where, size, val);
+}
+
+static struct pcie_host_ops qcom_pcie_dw_ops = {
+	.link_up = qcom_pcie_link_up,
+	.host_init = qcom_pcie_host_init,
+	.rd_own_conf = qcom_pcie_rd_own_conf,
+};
+
+static const struct qcom_pcie_ops ops_v0 = {
+	.get_resources = qcom_pcie_get_resources_v0,
+	.init = qcom_pcie_init_v0,
+	.deinit = qcom_pcie_deinit_v0,
+};
+
+static const struct qcom_pcie_ops ops_v1 = {
+	.get_resources = qcom_pcie_get_resources_v1,
+	.init = qcom_pcie_init_v1,
+	.deinit = qcom_pcie_deinit_v1,
+};
+
+static int qcom_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct qcom_pcie *pcie;
+	struct pcie_port *pp;
+	int ret;
+
+	pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL);
+	if (!pcie)
+		return -ENOMEM;
+
+	pcie->ops = (struct qcom_pcie_ops *)of_device_get_match_data(dev);
+	pcie->dev = dev;
+
+	pcie->reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_LOW);
+	if (IS_ERR(pcie->reset))
+		return PTR_ERR(pcie->reset);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "parf");
+	pcie->parf = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pcie->parf))
+		return PTR_ERR(pcie->parf);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
+	pcie->dbi = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pcie->dbi))
+		return PTR_ERR(pcie->dbi);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "elbi");
+	pcie->elbi = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pcie->elbi))
+		return PTR_ERR(pcie->elbi);
+
+	pcie->phy = devm_phy_optional_get(dev, "pciephy");
+	if (IS_ERR(pcie->phy))
+		return PTR_ERR(pcie->phy);
+
+	ret = pcie->ops->get_resources(pcie);
+	if (ret)
+		return ret;
+
+	pp = &pcie->pp;
+	pp->dev = dev;
+	pp->dbi_base = pcie->dbi;
+	pp->root_bus_nr = -1;
+	pp->ops = &qcom_pcie_dw_ops;
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		pp->msi_irq = platform_get_irq_byname(pdev, "msi");
+		if (pp->msi_irq < 0)
+			return pp->msi_irq;
+
+		ret = devm_request_irq(dev, pp->msi_irq,
+				       qcom_pcie_msi_irq_handler,
+				       IRQF_SHARED, "qcom-pcie-msi", pp);
+		if (ret) {
+			dev_err(dev, "cannot request msi irq\n");
+			return ret;
+		}
+	}
+
+	ret = phy_init(pcie->phy);
+	if (ret)
+		return ret;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(dev, "cannot initialize host\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pcie);
+
+	return 0;
+}
+
+static int qcom_pcie_remove(struct platform_device *pdev)
+{
+	struct qcom_pcie *pcie = platform_get_drvdata(pdev);
+
+	qcom_ep_reset_assert(pcie);
+	phy_power_off(pcie->phy);
+	phy_exit(pcie->phy);
+	pcie->ops->deinit(pcie);
+
+	return 0;
+}
+
+static const struct of_device_id qcom_pcie_match[] = {
+	{ .compatible = "qcom,pcie-ipq8064", .data = &ops_v0 },
+	{ .compatible = "qcom,pcie-apq8064", .data = &ops_v0 },
+	{ .compatible = "qcom,pcie-apq8084", .data = &ops_v1 },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, qcom_pcie_match);
+
+static struct platform_driver qcom_pcie_driver = {
+	.probe = qcom_pcie_probe,
+	.remove = qcom_pcie_remove,
+	.driver = {
+		.name = "qcom-pcie",
+		.of_match_table = qcom_pcie_match,
+	},
+};
+
+module_platform_driver(qcom_pcie_driver);
+
+MODULE_AUTHOR("Stanimir Varbanov <svarbanov@mm-sol.com>");
+MODULE_DESCRIPTION("Qualcomm PCIe root complex driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v4 3/5] PCI: qcom: Add Qualcomm PCIe controller driver

[Stanimir Varbanov]

On 12/03/2015 03:35 PM, Stanimir Varbanov wrote:
> From: Stanimir Varbanov <svarbanov@mm-sol.com>
> 
> The PCIe driver reuse the Designware common code for host
> and MSI initialization, and also program the Qualcomm
> application specific registers.
> 
> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  MAINTAINERS                  |    7 +
>  drivers/pci/host/Kconfig     |   10 +
>  drivers/pci/host/Makefile    |    1 +
>  drivers/pci/host/pcie-qcom.c |  624 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 642 insertions(+)
>  create mode 100644 drivers/pci/host/pcie-qcom.c

Hi Bjorn, any chance to get this merged in 4.5 ?


-- 
regards,
Stan

Re: [PATCH v4 3/5] PCI: qcom: Add Qualcomm PCIe controller driver

[Bjorn Helgaas]

Hi Stan,

On Tue, Dec 15, 2015 at 10:24:07AM +0200, Stanimir Varbanov wrote:
> On 12/03/2015 03:35 PM, Stanimir Varbanov wrote:
> > From: Stanimir Varbanov <svarbanov@mm-sol.com>
> > 
> > The PCIe driver reuse the Designware common code for host
> > and MSI initialization, and also program the Qualcomm
> > application specific registers.
> > 
> > Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> > ---
> >  MAINTAINERS                  |    7 +
> >  drivers/pci/host/Kconfig     |   10 +
> >  drivers/pci/host/Makefile    |    1 +
> >  drivers/pci/host/pcie-qcom.c |  624 ++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 642 insertions(+)
> >  create mode 100644 drivers/pci/host/pcie-qcom.c
> 
> Hi Bjorn, any chance to get this merged in 4.5 ?

The driver looks good overall, so I think 4.5 is very possible.  It
doesn't look like there's really a consensus on the memory barrier
thing yet, and I assume the testing reports include that first patch,
so it'd be good to get that resolved first.

Bjorn

Re: [PATCH v4 3/5] PCI: qcom: Add Qualcomm PCIe controller driver

[Bjorn Helgaas]

On Thu, Dec 03, 2015 at 03:35:22PM +0200, Stanimir Varbanov wrote:
> From: Stanimir Varbanov <svarbanov@mm-sol.com>
> 
> The PCIe driver reuse the Designware common code for host
> and MSI initialization, and also program the Qualcomm
> application specific registers.
> 
> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  MAINTAINERS                  |    7 +
>  drivers/pci/host/Kconfig     |   10 +
>  drivers/pci/host/Makefile    |    1 +
>  drivers/pci/host/pcie-qcom.c |  624 ++++++++++++++++++++++++++++++++++++++++++

> +#define PCIE20_CAP				0x70
> +#define PCIE20_CAP_LINKCTRLSTATUS		(PCIE20_CAP + 0x10)
> +#define PCIE20_CAP_LINKCTRLSTATUS_LINK_UP	BIT(29)

This looks like it could be referring to a standard PCIe Capability;
could you use the existing PCI_EXP_LNKSTA and PCI_EXP_LNKSTA_DLLLA
symbols here?  And readw() instead of readl()?

> +static int qcom_pcie_enable_link_training(struct qcom_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	u32 val;
> +	int ret;
> +
> +	/* enable link training */
> +	val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL);
> +	val |= PCIE20_ELBI_SYS_CTRL_LT_ENABLE;
> +	writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
> +
> +	/* wait for up to 100ms for the link to come up */
> +	ret = readl_poll_timeout(pcie->elbi + PCIE20_ELBI_SYS_STTS, val,
> +				 val & XMLH_LINK_UP, LINKUP_DELAY_US,
> +				 LINKUP_TIMEOUT_US);
> +
> +	if (ret < 0 || !dw_pcie_link_up(&pcie->pp)) {
> +		dev_err(dev, "link initialization failed\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}

This looks a lot like the *_establish_link() functions in other
DesignWare-based drivers.  Can you make it look even more similar,
e.g., by renaming it to qcom_pcie_establish_link() and maybe moving
some of the PHY functionality here?

readl_poll_timeout() is nice and avoids the hand-coded timeout loop
the other drivers use.  But is there benefit in checking for
XMLH_LINK_UP, or could you simply poll dw_pcie_link_up() like the
others do?  If it's sufficient, I'd prefer using dw_pcie_link_up()
by itself because it's a little more generic.

Bjorn

Re: [PATCH v4 3/5] PCI: qcom: Add Qualcomm PCIe controller driver

[Stanimir Varbanov]

Bjorn, thanks for the comments!

On 12/16/2015 11:53 PM, Bjorn Helgaas wrote:
> On Thu, Dec 03, 2015 at 03:35:22PM +0200, Stanimir Varbanov wrote:
>> From: Stanimir Varbanov <svarbanov@mm-sol.com>
>>
>> The PCIe driver reuse the Designware common code for host
>> and MSI initialization, and also program the Qualcomm
>> application specific registers.
>>
>> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  MAINTAINERS                  |    7 +
>>  drivers/pci/host/Kconfig     |   10 +
>>  drivers/pci/host/Makefile    |    1 +
>>  drivers/pci/host/pcie-qcom.c |  624 ++++++++++++++++++++++++++++++++++++++++++
> 
>> +#define PCIE20_CAP				0x70
>> +#define PCIE20_CAP_LINKCTRLSTATUS		(PCIE20_CAP + 0x10)
>> +#define PCIE20_CAP_LINKCTRLSTATUS_LINK_UP	BIT(29)
> 
> This looks like it could be referring to a standard PCIe Capability;
> could you use the existing PCI_EXP_LNKSTA and PCI_EXP_LNKSTA_DLLLA
> symbols here?  And readw() instead of readl()?

Yes, that is possible but I still need to keep PCIE20_CAP capabilities
offset.

> 
>> +static int qcom_pcie_enable_link_training(struct qcom_pcie *pcie)
>> +{
>> +	struct device *dev = pcie->dev;
>> +	u32 val;
>> +	int ret;
>> +
>> +	/* enable link training */
>> +	val = readl(pcie->elbi + PCIE20_ELBI_SYS_CTRL);
>> +	val |= PCIE20_ELBI_SYS_CTRL_LT_ENABLE;
>> +	writel(val, pcie->elbi + PCIE20_ELBI_SYS_CTRL);
>> +
>> +	/* wait for up to 100ms for the link to come up */
>> +	ret = readl_poll_timeout(pcie->elbi + PCIE20_ELBI_SYS_STTS, val,
>> +				 val & XMLH_LINK_UP, LINKUP_DELAY_US,
>> +				 LINKUP_TIMEOUT_US);
>> +
>> +	if (ret < 0 || !dw_pcie_link_up(&pcie->pp)) {
>> +		dev_err(dev, "link initialization failed\n");
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> This looks a lot like the *_establish_link() functions in other
> DesignWare-based drivers.  Can you make it look even more similar,
> e.g., by renaming it to qcom_pcie_establish_link() and maybe moving
> some of the PHY functionality here?
> 
> readl_poll_timeout() is nice and avoids the hand-coded timeout loop
> the other drivers use.  But is there benefit in checking for
> XMLH_LINK_UP, or could you simply poll dw_pcie_link_up() like the
> others do?  If it's sufficient, I'd prefer using dw_pcie_link_up()
> by itself because it's a little more generic.

OK I will modify the code to use dw_pcie_link_up() and ensure that this
check is sufficient.

-- 
regards,
Stan

Re: [PATCH v4 3/5] PCI: qcom: Add Qualcomm PCIe controller driver

[Bjorn Helgaas]

On Thu, Dec 17, 2015 at 03:18:43PM +0200, Stanimir Varbanov wrote:
> Bjorn, thanks for the comments!
> 
> On 12/16/2015 11:53 PM, Bjorn Helgaas wrote:
> > On Thu, Dec 03, 2015 at 03:35:22PM +0200, Stanimir Varbanov wrote:
> >> From: Stanimir Varbanov <svarbanov@mm-sol.com>
> >>
> >> The PCIe driver reuse the Designware common code for host
> >> and MSI initialization, and also program the Qualcomm
> >> application specific registers.
> >>
> >> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >> ---
> >>  MAINTAINERS                  |    7 +
> >>  drivers/pci/host/Kconfig     |   10 +
> >>  drivers/pci/host/Makefile    |    1 +
> >>  drivers/pci/host/pcie-qcom.c |  624 ++++++++++++++++++++++++++++++++++++++++++
> > 
> >> +#define PCIE20_CAP				0x70
> >> +#define PCIE20_CAP_LINKCTRLSTATUS		(PCIE20_CAP + 0x10)
> >> +#define PCIE20_CAP_LINKCTRLSTATUS_LINK_UP	BIT(29)
> > 
> > This looks like it could be referring to a standard PCIe Capability;
> > could you use the existing PCI_EXP_LNKSTA and PCI_EXP_LNKSTA_DLLLA
> > symbols here?  And readw() instead of readl()?
> 
> Yes, that is possible but I still need to keep PCIE20_CAP capabilities
> offset.

Great, thanks!  I think that will help keep generic PCIe things from
looking like they're special implementation-specific things.

Bjorn

[PATCH v4 2/5] DT: PCI: qcom: Document PCIe devicetree bindings

[Stanimir Varbanov]

From: Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>

Document Qualcomm PCIe driver devicetree bindings.

Signed-off-by: Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Stanimir Varbanov <stanimir.varbanov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 .../devicetree/bindings/pci/qcom,pcie.txt          |  233 ++++++++++++++++++++
 1 file changed, 233 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
new file mode 100644
index 000000000000..6d71ee2e335d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -0,0 +1,233 @@
+* Qualcomm PCI express root complex
+
+- compatible:
+	Usage: required
+	Value type: <stringlist>
+	Definition: Value should contain
+			- "qcom,pcie-ipq8064" for ipq8064
+			- "qcom,pcie-apq8064" for apq8064
+			- "qcom,pcie-apq8084" for apq8084
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: Register ranges as listed in the reg-names property
+
+- reg-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: Must include the following entries
+			- "parf"   Qualcomm specific registers
+			- "dbi"	   Designware PCIe registers
+			- "elbi"   External local bus interface registers
+			- "config" PCIe configuration space
+
+- device_type:
+	Usage: required
+	Value type: <string>
+	Definition: Should be "pci". As specified in designware-pcie.txt
+
+- #address-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: Should be set to 3. As specified in designware-pcie.txt
+
+- #size-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: Should be set 2. As specified in designware-pcie.txt
+
+- ranges:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: As specified in designware-pcie.txt
+
+- interrupts:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: MSI interrupt
+
+- interrupt-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: Should contain "msi"
+
+- #interrupt-cells:
+	Usage: required
+	Value type: <u32>
+	Definition: Should be 1. As specified in designware-pcie.txt
+
+- interrupt-map-mask:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: As specified in designware-pcie.txt
+
+- interrupt-map:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: As specified in designware-pcie.txt
+
+- clocks:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: List of phandle and clock specifier pairs as listed
+		    in clock-names property
+
+- clock-names:
+	Usage: required
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "iface"	Configuration AHB clock
+
+- clock-names:
+	Usage: required for ipq/apq8064
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "core"	Clocks the pcie hw block
+			- "phy"		Clocks the pcie PHY block
+- clock-names:
+	Usage: required for apq8084
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "aux"		Auxiliary (AUX) clock
+			- "bus_master"	Master AXI clock
+			- "bus_slave"	Slave AXI clock
+- resets:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: List of phandle and reset specifier pairs as listed
+		    in reset-names property
+
+- reset-names:
+	Usage: required for ipq/apq8064
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "axi"  AXI reset
+			- "ahb"  AHB reset
+			- "por"  POR reset
+			- "pci"  PCI reset
+			- "phy"  PHY reset
+
+- reset-names:
+	Usage: required for apq8084
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "core" Core reset
+
+- power-domains:
+	Usage: required for apq8084
+	Value type: <prop-encoded-array>
+	Definition: A phandle and power domain specifier pair to the
+		    power domain which is responsible for collapsing
+		    and restoring power to the peripheral
+
+- vdda-supply:
+	Usage: required
+	Value type: <phandle>
+	Definition: A phandle to the core analog power supply
+
+- vdda_phy-supply:
+	Usage: required for ipq/apq8064
+	Value type: <phandle>
+	Definition: A phandle to the analog power supply for PHY
+
+- vdda_refclk-supply:
+	Usage: required for ipq/apq8064
+	Value type: <phandle>
+	Definition: A phandle to the analog power supply for IC which generates
+		    reference clock
+
+- phys:
+	Usage: required for apq8084
+	Value type: <phandle>
+	Definition: List of phandle(s) as listed in phy-names property
+
+- phy-names:
+	Usage: required for apq8084
+	Value type: <stringlist>
+	Definition: Should contain "pciephy"
+
+- <name>-gpios:
+	Usage: optional
+	Value type: <prop-encoded-array>
+	Definition: List of phandle and gpio specifier pairs. Should contain
+			- "perst-gpios"	PCIe endpoint reset signal line
+			- "wake-gpios"	PCIe endpoint wake signal line
+
+* Example for ipq/apq8064
+	pcie@1b500000 {
+		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
+		reg = <0x1b500000 0x1000
+		       0x1b502000 0x80
+		       0x1b600000 0x100
+		       0x0ff00000 0x100000>;
+		reg-names = "dbi", "elbi", "parf", "config";
+		device_type = "pci";
+		linux,pci-domain = <0>;
+		bus-range = <0x00 0xff>;
+		num-lanes = <1>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		ranges = <0x81000000 0 0 0x0fe00000 0 0x00100000   /* I/O */
+			  0x82000000 0 0 0x08000000 0 0x07e00000>; /* memory */
+		interrupts = <GIC_SPI 238 IRQ_TYPE_NONE>;
+		interrupt-names = "msi";
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0x7>;
+		interrupt-map = <0 0 0 1 &intc 0 36 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+				<0 0 0 2 &intc 0 37 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+				<0 0 0 3 &intc 0 38 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+				<0 0 0 4 &intc 0 39 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+		clocks = <&gcc PCIE_A_CLK>,
+			 <&gcc PCIE_H_CLK>,
+			 <&gcc PCIE_PHY_CLK>;
+		clock-names = "core", "iface", "phy";
+		resets = <&gcc PCIE_ACLK_RESET>,
+			 <&gcc PCIE_HCLK_RESET>,
+			 <&gcc PCIE_POR_RESET>,
+			 <&gcc PCIE_PCI_RESET>,
+			 <&gcc PCIE_PHY_RESET>;
+		reset-names = "axi", "ahb", "por", "pci", "phy";
+		pinctrl-0 = <&pcie_pins_default>;
+		pinctrl-names = "default";
+	};
+
+* Example for apq8084
+	pcie0@fc520000 {
+		compatible = "qcom,pcie-apq8084", "snps,dw-pcie";
+		reg = <0xfc520000 0x2000>,
+		      <0xff000000 0x1000>,
+		      <0xff001000 0x1000>,
+		      <0xff002000 0x2000>;
+		reg-names = "parf", "dbi", "elbi", "config";
+		device_type = "pci";
+		linux,pci-domain = <0>;
+		bus-range = <0x00 0xff>;
+		num-lanes = <1>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		ranges = <0x81000000 0 0          0xff200000 0 0x00100000   /* I/O */
+			  0x82000000 0 0x00300000 0xff300000 0 0x00d00000>; /* memory */
+		interrupts = <GIC_SPI 243 IRQ_TYPE_NONE>;
+		interrupt-names = "msi";
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0x7>;
+		interrupt-map = <0 0 0 1 &intc 0 244 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+				<0 0 0 2 &intc 0 245 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+				<0 0 0 3 &intc 0 247 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+				<0 0 0 4 &intc 0 248 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+		clocks = <&gcc GCC_PCIE_0_CFG_AHB_CLK>,
+			 <&gcc GCC_PCIE_0_MSTR_AXI_CLK>,
+			 <&gcc GCC_PCIE_0_SLV_AXI_CLK>,
+			 <&gcc GCC_PCIE_0_AUX_CLK>;
+		clock-names = "iface", "master_bus", "slave_bus", "aux";
+		resets = <&gcc GCC_PCIE_0_BCR>;
+		reset-names = "core";
+		power-domains = <&gcc PCIE0_GDSC>;
+		vdda-supply = <&pma8084_l3>;
+		phys = <&pciephy0>;
+		phy-names = "pciephy";
+		perst-gpio = <&tlmm 70 GPIO_ACTIVE_LOW>;
+		pinctrl-0 = <&pcie0_pins_default>;
+		pinctrl-names = "default";
+	};
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/5] DT: PCI: qcom: Document PCIe devicetree bindings

[Rob Herring]

On Thu, Dec 03, 2015 at 03:35:21PM +0200, Stanimir Varbanov wrote:
> From: Stanimir Varbanov <svarbanov@mm-sol.com>
> 
> Document Qualcomm PCIe driver devicetree bindings.
> 
> Signed-off-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt          |  233 ++++++++++++++++++++
>  1 file changed, 233 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> new file mode 100644
> index 000000000000..6d71ee2e335d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -0,0 +1,233 @@
> +* Qualcomm PCI express root complex
> +
> +- compatible:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: Value should contain
> +			- "qcom,pcie-ipq8064" for ipq8064
> +			- "qcom,pcie-apq8064" for apq8064
> +			- "qcom,pcie-apq8084" for apq8084
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: Register ranges as listed in the reg-names property
> +
> +- reg-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: Must include the following entries
> +			- "parf"   Qualcomm specific registers
> +			- "dbi"	   Designware PCIe registers
> +			- "elbi"   External local bus interface registers
> +			- "config" PCIe configuration space
> +
> +- device_type:
> +	Usage: required
> +	Value type: <string>
> +	Definition: Should be "pci". As specified in designware-pcie.txt
> +
> +- #address-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: Should be set to 3. As specified in designware-pcie.txt
> +
> +- #size-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: Should be set 2. As specified in designware-pcie.txt
> +
> +- ranges:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: As specified in designware-pcie.txt
> +
> +- interrupts:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: MSI interrupt
> +
> +- interrupt-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: Should contain "msi"
> +
> +- #interrupt-cells:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: Should be 1. As specified in designware-pcie.txt
> +
> +- interrupt-map-mask:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: As specified in designware-pcie.txt
> +
> +- interrupt-map:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: As specified in designware-pcie.txt
> +
> +- clocks:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: List of phandle and clock specifier pairs as listed
> +		    in clock-names property
> +
> +- clock-names:
> +	Usage: required
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "iface"	Configuration AHB clock
> +
> +- clock-names:
> +	Usage: required for ipq/apq8064
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "core"	Clocks the pcie hw block
> +			- "phy"		Clocks the pcie PHY block
> +- clock-names:
> +	Usage: required for apq8084
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "aux"		Auxiliary (AUX) clock
> +			- "bus_master"	Master AXI clock
> +			- "bus_slave"	Slave AXI clock
> +- resets:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: List of phandle and reset specifier pairs as listed
> +		    in reset-names property
> +
> +- reset-names:
> +	Usage: required for ipq/apq8064
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "axi"  AXI reset
> +			- "ahb"  AHB reset
> +			- "por"  POR reset
> +			- "pci"  PCI reset
> +			- "phy"  PHY reset
> +
> +- reset-names:
> +	Usage: required for apq8084
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "core" Core reset
> +
> +- power-domains:
> +	Usage: required for apq8084
> +	Value type: <prop-encoded-array>
> +	Definition: A phandle and power domain specifier pair to the
> +		    power domain which is responsible for collapsing
> +		    and restoring power to the peripheral
> +
> +- vdda-supply:
> +	Usage: required
> +	Value type: <phandle>
> +	Definition: A phandle to the core analog power supply
> +
> +- vdda_phy-supply:
> +	Usage: required for ipq/apq8064
> +	Value type: <phandle>
> +	Definition: A phandle to the analog power supply for PHY
> +
> +- vdda_refclk-supply:
> +	Usage: required for ipq/apq8064
> +	Value type: <phandle>
> +	Definition: A phandle to the analog power supply for IC which generates
> +		    reference clock
> +
> +- phys:
> +	Usage: required for apq8084
> +	Value type: <phandle>
> +	Definition: List of phandle(s) as listed in phy-names property
> +
> +- phy-names:
> +	Usage: required for apq8084
> +	Value type: <stringlist>
> +	Definition: Should contain "pciephy"
> +
> +- <name>-gpios:
> +	Usage: optional
> +	Value type: <prop-encoded-array>
> +	Definition: List of phandle and gpio specifier pairs. Should contain
> +			- "perst-gpios"	PCIe endpoint reset signal line
> +			- "wake-gpios"	PCIe endpoint wake signal line
> +
> +* Example for ipq/apq8064
> +	pcie@1b500000 {
> +		compatible = "qcom,pcie-apq8064", "qcom,pcie-ipq8064", "snps,dw-pcie";
> +		reg = <0x1b500000 0x1000
> +		       0x1b502000 0x80
> +		       0x1b600000 0x100
> +		       0x0ff00000 0x100000>;
> +		reg-names = "dbi", "elbi", "parf", "config";
> +		device_type = "pci";
> +		linux,pci-domain = <0>;
> +		bus-range = <0x00 0xff>;
> +		num-lanes = <1>;
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		ranges = <0x81000000 0 0 0x0fe00000 0 0x00100000   /* I/O */
> +			  0x82000000 0 0 0x08000000 0 0x07e00000>; /* memory */
> +		interrupts = <GIC_SPI 238 IRQ_TYPE_NONE>;
> +		interrupt-names = "msi";
> +		#interrupt-cells = <1>;
> +		interrupt-map-mask = <0 0 0 0x7>;
> +		interrupt-map = <0 0 0 1 &intc 0 36 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> +				<0 0 0 2 &intc 0 37 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +				<0 0 0 3 &intc 0 38 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +				<0 0 0 4 &intc 0 39 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +		clocks = <&gcc PCIE_A_CLK>,
> +			 <&gcc PCIE_H_CLK>,
> +			 <&gcc PCIE_PHY_CLK>;
> +		clock-names = "core", "iface", "phy";
> +		resets = <&gcc PCIE_ACLK_RESET>,
> +			 <&gcc PCIE_HCLK_RESET>,
> +			 <&gcc PCIE_POR_RESET>,
> +			 <&gcc PCIE_PCI_RESET>,
> +			 <&gcc PCIE_PHY_RESET>;
> +		reset-names = "axi", "ahb", "por", "pci", "phy";
> +		pinctrl-0 = <&pcie_pins_default>;
> +		pinctrl-names = "default";
> +	};
> +
> +* Example for apq8084
> +	pcie0@fc520000 {
> +		compatible = "qcom,pcie-apq8084", "snps,dw-pcie";
> +		reg = <0xfc520000 0x2000>,
> +		      <0xff000000 0x1000>,
> +		      <0xff001000 0x1000>,
> +		      <0xff002000 0x2000>;
> +		reg-names = "parf", "dbi", "elbi", "config";
> +		device_type = "pci";
> +		linux,pci-domain = <0>;
> +		bus-range = <0x00 0xff>;
> +		num-lanes = <1>;
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		ranges = <0x81000000 0 0          0xff200000 0 0x00100000   /* I/O */
> +			  0x82000000 0 0x00300000 0xff300000 0 0x00d00000>; /* memory */
> +		interrupts = <GIC_SPI 243 IRQ_TYPE_NONE>;
> +		interrupt-names = "msi";
> +		#interrupt-cells = <1>;
> +		interrupt-map-mask = <0 0 0 0x7>;
> +		interrupt-map = <0 0 0 1 &intc 0 244 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> +				<0 0 0 2 &intc 0 245 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +				<0 0 0 3 &intc 0 247 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +				<0 0 0 4 &intc 0 248 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +		clocks = <&gcc GCC_PCIE_0_CFG_AHB_CLK>,
> +			 <&gcc GCC_PCIE_0_MSTR_AXI_CLK>,
> +			 <&gcc GCC_PCIE_0_SLV_AXI_CLK>,
> +			 <&gcc GCC_PCIE_0_AUX_CLK>;
> +		clock-names = "iface", "master_bus", "slave_bus", "aux";
> +		resets = <&gcc GCC_PCIE_0_BCR>;
> +		reset-names = "core";
> +		power-domains = <&gcc PCIE0_GDSC>;
> +		vdda-supply = <&pma8084_l3>;
> +		phys = <&pciephy0>;
> +		phy-names = "pciephy";
> +		perst-gpio = <&tlmm 70 GPIO_ACTIVE_LOW>;
> +		pinctrl-0 = <&pcie0_pins_default>;
> +		pinctrl-names = "default";
> +	};
> -- 
> 1.7.9.5
> 

[PATCH v4 4/5] ARM: dts: apq8064: add pcie devicetree node

[Stanimir Varbanov]

Add the pcie dt node so that it can probe and used.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/dts/qcom-apq8064.dtsi |   36 +++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
index a4c1762b53ea..847150fbfdbf 100644
--- a/arch/arm/boot/dts/qcom-apq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
@@ -659,5 +659,41 @@
 			compatible = "qcom,tcsr-apq8064", "syscon";
 			reg = <0x1a400000 0x100>;
 		};
+
+		pcie: pci@1b500000 {
+			compatible = "qcom,pcie-apq8064", "snps,dw-pcie";
+			reg = <0x1b500000 0x1000
+			       0x1b502000 0x80
+			       0x1b600000 0x100
+			       0x0ff00000 0x100000>;
+			reg-names = "dbi", "elbi", "parf", "config";
+			device_type = "pci";
+			linux,pci-domain = <0>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <1>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+			ranges = <0x81000000 0 0 0x0fe00000 0 0x00100000   /* I/O */
+				  0x82000000 0 0 0x08000000 0 0x07e00000>; /* memory */
+			interrupts = <GIC_SPI 238 IRQ_TYPE_NONE>;
+			interrupt-names = "msi";
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &intc 0 36 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+					<0 0 0 2 &intc 0 37 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+					<0 0 0 3 &intc 0 38 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+					<0 0 0 4 &intc 0 39 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+			clocks = <&gcc PCIE_A_CLK>,
+				 <&gcc PCIE_H_CLK>,
+				 <&gcc PCIE_PHY_REF_CLK>;
+			clock-names = "core", "iface", "phy";
+			resets = <&gcc PCIE_ACLK_RESET>,
+				 <&gcc PCIE_HCLK_RESET>,
+				 <&gcc PCIE_POR_RESET>,
+				 <&gcc PCIE_PCI_RESET>,
+				 <&gcc PCIE_PHY_RESET>;
+			reset-names = "axi", "ahb", "por", "pci", "phy";
+			status = "disabled";
+		};
 	};
 };
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 5/5] ARM: dts: ifc6410: enable pcie dt node for this board

[Stanimir Varbanov]

Enable pcie dt node and fill pcie dt node with regulator, pinctrl
and reset gpio, to use the pcie on the ifc6410 board.

Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
---
 arch/arm/boot/dts/qcom-apq8064-ifc6410.dts |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts b/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
index 11ac608b6d50..f203b94ee460 100644
--- a/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
+++ b/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
@@ -47,6 +47,18 @@
 					bias-disable;
 				};
 			};
+
+			pcie_pins: pcie_pinmux {
+				mux {
+					pins = "gpio27";
+					function = "gpio";
+				};
+				conf {
+					pins = "gpio27";
+					drive-strength = <12>;
+					bias-disable;
+				};
+			};
 		};
 
 		rpm@108000 {
@@ -123,6 +135,10 @@
 				pm8921_lvs1: lvs1 {
 					bias-pull-down;
 				};
+
+				pm8921_lvs6: lvs6 {
+					bias-pull-down;
+				};
 			};
 		};
 
@@ -231,6 +247,16 @@
 			status = "okay";
 		};
 
+		pci@1b500000 {
+			status = "ok";
+			vdda-supply = <&pm8921_s3>;
+			vdda_phy-supply = <&pm8921_lvs6>;
+			vdda_refclk-supply = <&ext_3p3v>;
+			pinctrl-0 = <&pcie_pins>;
+			pinctrl-names = "default";
+			perst-gpio = <&tlmm_pinmux 27 GPIO_ACTIVE_LOW>;
+		};
+
 		qcom,ssbi@500000 {
 			pmic@0 {
 				gpio@150 {
-- 
1.7.9.5

Re: [PATCH v4 5/5] ARM: dts: ifc6410: enable pcie dt node for this board

[Bjorn Andersson]

On Thu 03 Dec 05:35 PST 2015, Stanimir Varbanov wrote:

> Enable pcie dt node and fill pcie dt node with regulator, pinctrl
> and reset gpio, to use the pcie on the ifc6410 board.
> 
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/boot/dts/qcom-apq8064-ifc6410.dts |   26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts b/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
> index 11ac608b6d50..f203b94ee460 100644
> --- a/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
> +++ b/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
> @@ -47,6 +47,18 @@
>  					bias-disable;
>  				};
>  			};
> +
> +			pcie_pins: pcie_pinmux {
> +				mux {
> +					pins = "gpio27";
> +					function = "gpio";
> +				};
> +				conf {
> +					pins = "gpio27";
> +					drive-strength = <12>;
> +					bias-disable;
> +				};
> +			};
>  		};
>  
>  		rpm@108000 {
> @@ -123,6 +135,10 @@
>  				pm8921_lvs1: lvs1 {
>  					bias-pull-down;
>  				};
> +
> +				pm8921_lvs6: lvs6 {
> +					bias-pull-down;
> +				};

Please don't duplicate the labels in the dts files.

>  			};
>  		};
>  

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/5] ARM: dts: ifc6410: enable pcie dt node for this board

[Stanimir Varbanov]

On 12/17/2015 11:55 PM, Bjorn Andersson wrote:
> On Thu 03 Dec 05:35 PST 2015, Stanimir Varbanov wrote:
> 
>> Enable pcie dt node and fill pcie dt node with regulator, pinctrl
>> and reset gpio, to use the pcie on the ifc6410 board.
>>
>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
>> ---
>>  arch/arm/boot/dts/qcom-apq8064-ifc6410.dts |   26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts b/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
>> index 11ac608b6d50..f203b94ee460 100644
>> --- a/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
>> +++ b/arch/arm/boot/dts/qcom-apq8064-ifc6410.dts
>> @@ -47,6 +47,18 @@
>>  					bias-disable;
>>  				};
>>  			};
>> +
>> +			pcie_pins: pcie_pinmux {
>> +				mux {
>> +					pins = "gpio27";
>> +					function = "gpio";
>> +				};
>> +				conf {
>> +					pins = "gpio27";
>> +					drive-strength = <12>;
>> +					bias-disable;
>> +				};
>> +			};
>>  		};
>>  
>>  		rpm@108000 {
>> @@ -123,6 +135,10 @@
>>  				pm8921_lvs1: lvs1 {
>>  					bias-pull-down;
>>  				};
>> +
>> +				pm8921_lvs6: lvs6 {
>> +					bias-pull-down;
>> +				};
> 
> Please don't duplicate the labels in the dts files.

Sorry, I based those dts patches before [1] has been applied in -next. I
will rebase this patch on top of [1]. Thanks!

[1] "ARM: dts: qcom: apq8064: Declare all pm8921 regulators"

-- 
regards,
Stan

Re: [PATCH v4 0/5] Qualcomm PCIe driver and designware fixes

[Srinivas Kandagatla]

On 03/12/15 13:35, Stanimir Varbanov wrote:
> Hi,
>
> Here is v4, comments from Bjorn and Rob have been addressed.
> The previous version can be found at [1].
>
> regards,
> Stan
>
> [1] https://lkml.org/lkml/2015/11/23/114
>
> Stanimir Varbanov (5):
>    PCI: designware: add memory barrier after enabling region
>    DT: PCI: qcom: Document PCIe devicetree bindings
>    PCI: qcom: Add Qualcomm PCIe controller driver
>    ARM: dts: apq8064: add pcie devicetree node
>    ARM: dts: ifc6410: enable pcie dt node for this board
>
Thanks for the patches, Tested on IFC6410.

Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>


>   .../devicetree/bindings/pci/qcom,pcie.txt          |  233 ++++++++
>   MAINTAINERS                                        |    7 +
>   arch/arm/boot/dts/qcom-apq8064-ifc6410.dts         |   26 +
>   arch/arm/boot/dts/qcom-apq8064.dtsi                |   36 ++
>   drivers/pci/host/Kconfig                           |   10 +
>   drivers/pci/host/Makefile                          |    1 +
>   drivers/pci/host/pcie-designware.c                 |    5 +
>   drivers/pci/host/pcie-qcom.c                       |  624 ++++++++++++++++++++
>   8 files changed, 942 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt
>   create mode 100644 drivers/pci/host/pcie-qcom.c
>