[PATCH v4 0/3] PCI: qcom: Add support to msm8996 pcie controller.

[PATCH v4 0/3] PCI: qcom: Add support to msm8996 pcie controller.

[Srinivas Kandagatla]

This patchset adds support to msm8996 pcie controller. I tested this patch on
v4.9-rc2 along with phy driver patch [1] and
"PCI: designware: check for iATU unroll support after initializing host"
fix [2] on DB820c APQ8096 board on port B and port C using sata and
ethernet controller.

Changes since v3:
	- remove unnesessary variable initialization spotted by vivek.
	- moved pipe clk disable before other clocks suggested by vivek.
	- fixed dt example suggested by Rob.
Changes since v2:
	- Removed regulators that belong to phy, spotted by Stephen
	- Removed clocks in to simple pm bus driver, spotted by Stephen
	- renamed msm8996 ops to v2 ops as suggested by Stephen.
	- cleanups as suggested by Stephen.
	- Add runtime pm support to driver.
	- Added pm clk support to simple pm bus driver.

Changes since v1:
	- Fixed dt example as suggested by Rob
	- added smmu bus clk dependency as smmu sits in between
	  system NOC and PCIe.
	- Removed smmu configuration from bindings and driver as
	  the smmu Level2 translation on this SOC is controlled by
	  the secure world, and level 1 translation is disabled,
	  so there is one-to-one mapping of the address space.

Thanks,
srini

[1] https://patchwork.kernel.org/patch/9384711/
[2] https://patchwork.kernel.org/patch/9377557/


Srinivas Kandagatla (3):
  bus: simple-pm: add support to pm clocks
  PCI: qcom: add support to msm8996 PCIE controller
  PCI: qcom: add runtime pm support to pcie_port

 .../devicetree/bindings/pci/qcom,pcie.txt          |  67 +++++++-
 drivers/bus/simple-pm-bus.c                        |  13 +-
 drivers/pci/host/pcie-qcom.c                       | 181 ++++++++++++++++++++-
 3 files changed, 254 insertions(+), 7 deletions(-)

-- 
2.10.1

[PATCH v4 1/3] bus: simple-pm: add support to pm clocks

[Srinivas Kandagatla]

This patch adds support to pm clocks via device tree, so that the clocks
can be turned on and off during runtime pm. This patch is required for
Qualcomm msm8996 pcie controller which sits on a bus with its own
power-domain and clocks.

Without this patch the clock associated with the bus are never turned on.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/bus/simple-pm-bus.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
index c5eb46c..63b7e8c 100644
--- a/drivers/bus/simple-pm-bus.c
+++ b/drivers/bus/simple-pm-bus.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/pm_clock.h>
 #include <linux/pm_runtime.h>
 
 
@@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(&pdev->dev);
 
-	if (np)
+	if (np) {
+		of_pm_clk_add_clks(&pdev->dev);
 		of_platform_populate(np, NULL, NULL, &pdev->dev);
+	}
 
 	return 0;
 }
 
+static const struct dev_pm_ops simple_pm_bus_pm_ops = {
+	SET_RUNTIME_PM_OPS(pm_clk_suspend,
+			   pm_clk_resume, NULL)
+};
+
 static int simple_pm_bus_remove(struct platform_device *pdev)
 {
 	dev_dbg(&pdev->dev, "%s\n", __func__);
 
 	pm_runtime_disable(&pdev->dev);
+	pm_clk_destroy(&pdev->dev);
+
 	return 0;
 }
 
@@ -48,6 +58,7 @@ static struct platform_driver simple_pm_bus_driver = {
 	.driver = {
 		.name = "simple-pm-bus",
 		.of_match_table = simple_pm_bus_of_match,
+		.pm = &simple_pm_bus_pm_ops,
 	},
 };
 
-- 
2.10.1

Re: [PATCH v4 1/3] bus: simple-pm: add support to pm clocks

[Bjorn Helgaas]

[+cc Geert, Kevin, Simon]

On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote:
> This patch adds support to pm clocks via device tree, so that the clocks
> can be turned on and off during runtime pm. This patch is required for
> Qualcomm msm8996 pcie controller which sits on a bus with its own
> power-domain and clocks.
> 
> Without this patch the clock associated with the bus are never turned on.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd
like an ack or at least a review from Geert or Simon.

> ---
>  drivers/bus/simple-pm-bus.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> index c5eb46c..63b7e8c 100644
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -11,6 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_clock.h>
>  #include <linux/pm_runtime.h>
>  
>  
> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
>  
>  	pm_runtime_enable(&pdev->dev);
>  
> -	if (np)
> +	if (np) {
> +		of_pm_clk_add_clks(&pdev->dev);
>  		of_platform_populate(np, NULL, NULL, &pdev->dev);
> +	}
>  
>  	return 0;
>  }
>  
> +static const struct dev_pm_ops simple_pm_bus_pm_ops = {
> +	SET_RUNTIME_PM_OPS(pm_clk_suspend,
> +			   pm_clk_resume, NULL)
> +};
> +
>  static int simple_pm_bus_remove(struct platform_device *pdev)
>  {
>  	dev_dbg(&pdev->dev, "%s\n", __func__);
>  
>  	pm_runtime_disable(&pdev->dev);
> +	pm_clk_destroy(&pdev->dev);
> +
>  	return 0;
>  }
>  
> @@ -48,6 +58,7 @@ static struct platform_driver simple_pm_bus_driver = {
>  	.driver = {
>  		.name = "simple-pm-bus",
>  		.of_match_table = simple_pm_bus_of_match,
> +		.pm = &simple_pm_bus_pm_ops,
>  	},
>  };
>  
> -- 
> 2.10.1
> 
> --
> 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

Re: [PATCH v4 1/3] bus: simple-pm: add support to pm clocks

[Geert Uytterhoeven]

+cc linux-pm

On Mon, Nov 14, 2016 at 11:14 PM, Bjorn Helgaas <helgaas-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> [+cc Geert, Kevin, Simon]
>
> On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote:
>> This patch adds support to pm clocks via device tree, so that the clocks
>> can be turned on and off during runtime pm. This patch is required for
>> Qualcomm msm8996 pcie controller which sits on a bus with its own
>> power-domain and clocks.
>>
>> Without this patch the clock associated with the bus are never turned on.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd
> like an ack or at least a review from Geert or Simon.

Thanks for letting me know!

>> ---
>>  drivers/bus/simple-pm-bus.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
>> index c5eb46c..63b7e8c 100644
>> --- a/drivers/bus/simple-pm-bus.c
>> +++ b/drivers/bus/simple-pm-bus.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/module.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_clock.h>
>>  #include <linux/pm_runtime.h>
>>
>>
>> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
>>
>>       pm_runtime_enable(&pdev->dev);
>>
>> -     if (np)
>> +     if (np) {
>> +             of_pm_clk_add_clks(&pdev->dev);

This should work out-of-the-box (that's the actual purpose of this driver),
if the platform code that registers your PM Domain would take care
of registering the clocks needed for PM management of the bus.

Adding of_pm_clk_add_clks() here will start managing all clocks of the bus,
which may not be wanted on all platforms.

>>               of_platform_populate(np, NULL, NULL, &pdev->dev);
>> +     }
>>
>>       return 0;
>>  }
>>
>> +static const struct dev_pm_ops simple_pm_bus_pm_ops = {
>> +     SET_RUNTIME_PM_OPS(pm_clk_suspend,
>> +                        pm_clk_resume, NULL)
>> +};
>> +
>>  static int simple_pm_bus_remove(struct platform_device *pdev)
>>  {
>>       dev_dbg(&pdev->dev, "%s\n", __func__);
>>
>>       pm_runtime_disable(&pdev->dev);
>> +     pm_clk_destroy(&pdev->dev);
>> +
>>       return 0;
>>  }
>>
>> @@ -48,6 +58,7 @@ static struct platform_driver simple_pm_bus_driver = {
>>       .driver = {
>>               .name = "simple-pm-bus",
>>               .of_match_table = simple_pm_bus_of_match,
>> +             .pm = &simple_pm_bus_pm_ops,
>>       },
>>  };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
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 1/3] bus: simple-pm: add support to pm clocks

[Geert Uytterhoeven]

+cc linux-pm

On Mon, Nov 14, 2016 at 11:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Geert, Kevin, Simon]
>
> On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote:
>> This patch adds support to pm clocks via device tree, so that the clocks
>> can be turned on and off during runtime pm. This patch is required for
>> Qualcomm msm8996 pcie controller which sits on a bus with its own
>> power-domain and clocks.
>>
>> Without this patch the clock associated with the bus are never turned on.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd
> like an ack or at least a review from Geert or Simon.

Thanks for letting me know!

>> ---
>>  drivers/bus/simple-pm-bus.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
>> index c5eb46c..63b7e8c 100644
>> --- a/drivers/bus/simple-pm-bus.c
>> +++ b/drivers/bus/simple-pm-bus.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/module.h>
>>  #include <linux/of_platform.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_clock.h>
>>  #include <linux/pm_runtime.h>
>>
>>
>> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
>>
>>       pm_runtime_enable(&pdev->dev);
>>
>> -     if (np)
>> +     if (np) {
>> +             of_pm_clk_add_clks(&pdev->dev);

This should work out-of-the-box (that's the actual purpose of this driver),
if the platform code that registers your PM Domain would take care
of registering the clocks needed for PM management of the bus.

Adding of_pm_clk_add_clks() here will start managing all clocks of the bus,
which may not be wanted on all platforms.

>>               of_platform_populate(np, NULL, NULL, &pdev->dev);
>> +     }
>>
>>       return 0;
>>  }
>>
>> +static const struct dev_pm_ops simple_pm_bus_pm_ops = {
>> +     SET_RUNTIME_PM_OPS(pm_clk_suspend,
>> +                        pm_clk_resume, NULL)
>> +};
>> +
>>  static int simple_pm_bus_remove(struct platform_device *pdev)
>>  {
>>       dev_dbg(&pdev->dev, "%s\n", __func__);
>>
>>       pm_runtime_disable(&pdev->dev);
>> +     pm_clk_destroy(&pdev->dev);
>> +
>>       return 0;
>>  }
>>
>> @@ -48,6 +58,7 @@ static struct platform_driver simple_pm_bus_driver = {
>>       .driver = {
>>               .name = "simple-pm-bus",
>>               .of_match_table = simple_pm_bus_of_match,
>> +             .pm = &simple_pm_bus_pm_ops,
>>       },
>>  };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 1/3] bus: simple-pm: add support to pm clocks

[Srinivas Kandagatla]

+ Rajendra (qcom,gdsc author)

On 15/11/16 08:23, Geert Uytterhoeven wrote:
> +cc linux-pm
>
> On Mon, Nov 14, 2016 at 11:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> [+cc Geert, Kevin, Simon]
>>
>> On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote:
>>> This patch adds support to pm clocks via device tree, so that the clocks
>>> can be turned on and off during runtime pm. This patch is required for
>>> Qualcomm msm8996 pcie controller which sits on a bus with its own
>>> power-domain and clocks.
>>>
>>> Without this patch the clock associated with the bus are never turned on.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd
>> like an ack or at least a review from Geert or Simon.
>
> Thanks for letting me know!
>
>>> ---
>>>  drivers/bus/simple-pm-bus.c | 13 ++++++++++++-
>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
>>> index c5eb46c..63b7e8c 100644
>>> --- a/drivers/bus/simple-pm-bus.c
>>> +++ b/drivers/bus/simple-pm-bus.c
>>> @@ -11,6 +11,7 @@
>>>  #include <linux/module.h>
>>>  #include <linux/of_platform.h>
>>>  #include <linux/platform_device.h>
>>> +#include <linux/pm_clock.h>
>>>  #include <linux/pm_runtime.h>
>>>
>>>
>>> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
>>>
>>>       pm_runtime_enable(&pdev->dev);
>>>
>>> -     if (np)
>>> +     if (np) {
>>> +             of_pm_clk_add_clks(&pdev->dev);
>
> This should work out-of-the-box (that's the actual purpose of this driver),
> if the platform code that registers your PM Domain would take care
> of registering the clocks needed for PM management of the bus.

Yep, if the pm domain provider takes care of the bus clks, then it would 
work.

Am guessing that the clocks property in the DT node would be read by the 
PM domain provider and enable/disable during attach/detach callbacks.
If that is true, then any device tree nodes which are not children of 
"simple-pm-bus" and consumers of power-domain provider would enable all 
(including non-bus clks) clks twice. Once in the power-domain provider 
and once in the actual driver. Is this expected behavior from 
power-domains in general?

>
> Adding of_pm_clk_add_clks() here will start managing all clocks of the bus,
> which may not be wanted on all platforms.
>
That was the purpose.


Rajendra,
Looks like qcom gdsc pm domain provider driver does not handle bus clks 
along with power-domain, Is this something we should do? Or the bus 
driver take care of it?


Thanks,
srini
>>>               of_platform_populate(np, NULL, NULL, &pdev->dev);
>>> +     }
>>>
>>>       return 0;
>>>  }
>>>
>>> +static const struct dev_pm_ops simple_pm_bus_pm_ops = {
>>> +     SET_RUNTIME_PM_OPS(pm_clk_suspend,
>>> +                        pm_clk_resume, NULL)
>>> +};
>>> +
>>>  static int simple_pm_bus_remove(struct platform_device *pdev)
>>>  {
>>>       dev_dbg(&pdev->dev, "%s\n", __func__);
>>>
>>>       pm_runtime_disable(&pdev->dev);
>>> +     pm_clk_destroy(&pdev->dev);
>>> +
>>>       return 0;
>>>  }
>>>
>>> @@ -48,6 +58,7 @@ static struct platform_driver simple_pm_bus_driver = {
>>>       .driver = {
>>>               .name = "simple-pm-bus",
>>>               .of_match_table = simple_pm_bus_of_match,
>>> +             .pm = &simple_pm_bus_pm_ops,
>>>       },
>>>  };
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

Re: [PATCH v4 1/3] bus: simple-pm: add support to pm clocks

[Nayak, Rajendra]

Hey Srini,

On 11/15/2016 4:55 PM, Srinivas Kandagatla wrote:
> + Rajendra (qcom,gdsc author)
[]..

>>
>>>> ---
>>>>  drivers/bus/simple-pm-bus.c | 13 ++++++++++++-
>>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
>>>> index c5eb46c..63b7e8c 100644
>>>> --- a/drivers/bus/simple-pm-bus.c
>>>> +++ b/drivers/bus/simple-pm-bus.c
>>>> @@ -11,6 +11,7 @@
>>>>  #include <linux/module.h>
>>>>  #include <linux/of_platform.h>
>>>>  #include <linux/platform_device.h>
>>>> +#include <linux/pm_clock.h>
>>>>  #include <linux/pm_runtime.h>
>>>>
>>>>
>>>> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct
>>>> platform_device *pdev)
>>>>
>>>>       pm_runtime_enable(&pdev->dev);
>>>>
>>>> -     if (np)
>>>> +     if (np) {
>>>> +             of_pm_clk_add_clks(&pdev->dev);
>>
>> This should work out-of-the-box (that's the actual purpose of this
>> driver),
>> if the platform code that registers your PM Domain would take care
>> of registering the clocks needed for PM management of the bus.
>
> Yep, if the pm domain provider takes care of the bus clks, then it would
> work.
>
> Am guessing that the clocks property in the DT node would be read by the
> PM domain provider and enable/disable during attach/detach callbacks.
> If that is true, then any device tree nodes which are not children of
> "simple-pm-bus" and consumers of power-domain provider would enable all
> (including non-bus clks) clks twice. Once in the power-domain provider
> and once in the actual driver. Is this expected behavior from
> power-domains in general?
>
>>
>> Adding of_pm_clk_add_clks() here will start managing all clocks of the
>> bus,
>> which may not be wanted on all platforms.
>>
> That was the purpose.
>
>
> Rajendra,
> Looks like qcom gdsc pm domain provider driver does not handle bus clks
> along with power-domain, Is this something we should do? Or the bus
> driver take care of it?

I did post some patches to support handling of clocks associated with
gdscs [1], but it got dropped at that point since there wasn't a
real user, besides there were some open issues wrt the handling of
!CONFIG_PM cases etc.
I will revive and repost those patches again now based on the
discussions last time around.

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/362492.html

> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] bus: simple-pm: add support to pm clocks

[Srinivas Kandagatla]

Thanks Rajendra for pointing to the patch

On 16/11/16 15:50, Nayak, Rajendra wrote:
>>
>>
>> Rajendra,
>> Looks like qcom gdsc pm domain provider driver does not handle bus clks
>> along with power-domain, Is this something we should do? Or the bus
>> driver take care of it?
>
> I did post some patches to support handling of clocks associated with
> gdscs [1], but it got dropped at that point since there wasn't a
> real user, besides there were some open issues wrt the handling of
> !CONFIG_PM cases etc.
> I will revive and repost those patches again now based on the
> discussions last time around.
>
> [1]
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/362492.html

This patch looks exactly like the one useful in pcie case, i would be 
interesting to see the final patch on how we handle clocks which are 
both related and not related to power domain.

thanks,
srini
>

Re: [PATCH v4 1/3] bus: simple-pm: add support to pm clocks

[Bjorn Andersson]

On Tue 15 Nov 00:23 PST 2016, Geert Uytterhoeven wrote:

> +cc linux-pm
> 
> On Mon, Nov 14, 2016 at 11:14 PM, Bjorn Helgaas <helgaas-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > [+cc Geert, Kevin, Simon]
> >
> > On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote:
> >> This patch adds support to pm clocks via device tree, so that the clocks
> >> can be turned on and off during runtime pm. This patch is required for
> >> Qualcomm msm8996 pcie controller which sits on a bus with its own
> >> power-domain and clocks.
> >>
> >> Without this patch the clock associated with the bus are never turned on.
> >>
> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >
> > I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd
> > like an ack or at least a review from Geert or Simon.
> 
> Thanks for letting me know!
> 
> >> ---
> >>  drivers/bus/simple-pm-bus.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> >> index c5eb46c..63b7e8c 100644
> >> --- a/drivers/bus/simple-pm-bus.c
> >> +++ b/drivers/bus/simple-pm-bus.c
> >> @@ -11,6 +11,7 @@
> >>  #include <linux/module.h>
> >>  #include <linux/of_platform.h>
> >>  #include <linux/platform_device.h>
> >> +#include <linux/pm_clock.h>
> >>  #include <linux/pm_runtime.h>
> >>
> >>
> >> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
> >>
> >>       pm_runtime_enable(&pdev->dev);
> >>
> >> -     if (np)
> >> +     if (np) {
> >> +             of_pm_clk_add_clks(&pdev->dev);
> 
> This should work out-of-the-box (that's the actual purpose of this driver),
> if the platform code that registers your PM Domain would take care
> of registering the clocks needed for PM management of the bus.
> 

Hi Geert,

I'm having problems finding any code that would make this work
"out-of-the-box".  The DT binding documents a clocks property but I
can't find any code referencing this in the kernel.

I see that Srinivas interpreted your response as that we should fold the
clocks in behind the power-domain, rather than referencing them from the
bus - but this seems awkward and would indicate the DT binding being
wrong. Perhaps I'm just misunderstanding the design here?

Which "platform code" do you refer to, can you help me by pointing me to
the code that handles the zb_clk in the Renesas case?

> Adding of_pm_clk_add_clks() here will start managing all clocks of the bus,
> which may not be wanted on all platforms.
> 

It would not be strange to do so in the "simple" implementation for the
bus, allowing custom behavior to be implemented in a more specific
driver for a platform with custom needs.

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 1/3] bus: simple-pm: add support to pm clocks

[Bjorn Andersson]

On Tue 15 Nov 00:23 PST 2016, Geert Uytterhoeven wrote:

> +cc linux-pm
> 
> On Mon, Nov 14, 2016 at 11:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > [+cc Geert, Kevin, Simon]
> >
> > On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote:
> >> This patch adds support to pm clocks via device tree, so that the clocks
> >> can be turned on and off during runtime pm. This patch is required for
> >> Qualcomm msm8996 pcie controller which sits on a bus with its own
> >> power-domain and clocks.
> >>
> >> Without this patch the clock associated with the bus are never turned on.
> >>
> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> >
> > I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd
> > like an ack or at least a review from Geert or Simon.
> 
> Thanks for letting me know!
> 
> >> ---
> >>  drivers/bus/simple-pm-bus.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> >> index c5eb46c..63b7e8c 100644
> >> --- a/drivers/bus/simple-pm-bus.c
> >> +++ b/drivers/bus/simple-pm-bus.c
> >> @@ -11,6 +11,7 @@
> >>  #include <linux/module.h>
> >>  #include <linux/of_platform.h>
> >>  #include <linux/platform_device.h>
> >> +#include <linux/pm_clock.h>
> >>  #include <linux/pm_runtime.h>
> >>
> >>
> >> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
> >>
> >>       pm_runtime_enable(&pdev->dev);
> >>
> >> -     if (np)
> >> +     if (np) {
> >> +             of_pm_clk_add_clks(&pdev->dev);
> 
> This should work out-of-the-box (that's the actual purpose of this driver),
> if the platform code that registers your PM Domain would take care
> of registering the clocks needed for PM management of the bus.
> 

Hi Geert,

I'm having problems finding any code that would make this work
"out-of-the-box".  The DT binding documents a clocks property but I
can't find any code referencing this in the kernel.

I see that Srinivas interpreted your response as that we should fold the
clocks in behind the power-domain, rather than referencing them from the
bus - but this seems awkward and would indicate the DT binding being
wrong. Perhaps I'm just misunderstanding the design here?

Which "platform code" do you refer to, can you help me by pointing me to
the code that handles the zb_clk in the Renesas case?

> Adding of_pm_clk_add_clks() here will start managing all clocks of the bus,
> which may not be wanted on all platforms.
> 

It would not be strange to do so in the "simple" implementation for the
bus, allowing custom behavior to be implemented in a more specific
driver for a platform with custom needs.

Regards,
Bjorn

Re: [PATCH v4 1/3] bus: simple-pm: add support to pm clocks

[Geert Uytterhoeven]

Hi Bjorn,

On Thu, Dec 15, 2016 at 11:26 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Tue 15 Nov 00:23 PST 2016, Geert Uytterhoeven wrote:
>> On Mon, Nov 14, 2016 at 11:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Mon, Nov 14, 2016 at 11:15:53AM +0000, Srinivas Kandagatla wrote:
>> >> This patch adds support to pm clocks via device tree, so that the clocks
>> >> can be turned on and off during runtime pm. This patch is required for
>> >> Qualcomm msm8996 pcie controller which sits on a bus with its own
>> >> power-domain and clocks.
>> >>
>> >> Without this patch the clock associated with the bus are never turned on.
>> >>
>> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> >
>> > I don't see a formal maintainer for drivers/bus/simple-pm-bus.c, but I'd
>> > like an ack or at least a review from Geert or Simon.
>>
>> Thanks for letting me know!
>>
>> >> ---
>> >>  drivers/bus/simple-pm-bus.c | 13 ++++++++++++-
>> >>  1 file changed, 12 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
>> >> index c5eb46c..63b7e8c 100644
>> >> --- a/drivers/bus/simple-pm-bus.c
>> >> +++ b/drivers/bus/simple-pm-bus.c

>> >> @@ -22,17 +23,26 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
>> >>
>> >>       pm_runtime_enable(&pdev->dev);
>> >>
>> >> -     if (np)
>> >> +     if (np) {
>> >> +             of_pm_clk_add_clks(&pdev->dev);
>>
>> This should work out-of-the-box (that's the actual purpose of this driver),
>> if the platform code that registers your PM Domain would take care
>> of registering the clocks needed for PM management of the bus.

> I'm having problems finding any code that would make this work
> "out-of-the-box".  The DT binding documents a clocks property but I
> can't find any code referencing this in the kernel.
>
> I see that Srinivas interpreted your response as that we should fold the
> clocks in behind the power-domain, rather than referencing them from the
> bus - but this seems awkward and would indicate the DT binding being
> wrong. Perhaps I'm just misunderstanding the design here?

Platform-wide PM depends heavily on the platform. Instead of adding code to
handle all relevant platforms to all drivers, the generic PM Domain code is
used. The "power-domains" and corresponding PM "clocks" properties may be
added to any device name, depending on the platform.

> Which "platform code" do you refer to, can you help me by pointing me to
> the code that handles the zb_clk in the Renesas case?

See drivers/clk/renesas/clk-mstp.c:cpg_mstp_attach_dev(), which registers
the clocks that are used for PM.
If a PM Domain sets the GENPD_FLAG_PM_CLK flag, these clocks are enabled
resp. disabled by the genpd code when the device is resumed resp. suspend.

>> Adding of_pm_clk_add_clks() here will start managing all clocks of the bus,
>> which may not be wanted on all platforms.
>
> It would not be strange to do so in the "simple" implementation for the
> bus, allowing custom behavior to be implemented in a more specific
> driver for a platform with custom needs.

Doing that means every platform that doesn't want all clocks to be used for
PM need to add custom drivers, while we already handle this in genpd.
Moreover, the same functionality (some clocks are used for PM and/or the
device may be part of a power domain) is needed for non-bus devices, and
thus handled by genpd.

BTW, actually bus drivers are the case that's currently handled special: I'd
rather seen the pm_runtime_*() handling being added to plain simple-bus.
For non-bus drivers, we just add the calls to any driver that may be used on
platforms with clock and/or power domains, but for bus drivers, people wanted
a separate driver (with its own DT binding). Sigh...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v4 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Srinivas Kandagatla]

This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
legacy interrupts and it conforms to PCI Express Base 2.1 specification.

This patch adds post_init callback to qcom_pcie_ops, as this is pcie
pipe clocks are only setup after the phy is powered on.
It also adds ltssm_enable callback as it is very much different to other
supported SOCs in the driver.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 .../devicetree/bindings/pci/qcom,pcie.txt          |  67 +++++++-
 drivers/pci/host/pcie-qcom.c                       | 177 ++++++++++++++++++++-
 2 files changed, 238 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 4059a6f..141d8c3 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -7,6 +7,7 @@
 			- "qcom,pcie-ipq8064" for ipq8064
 			- "qcom,pcie-apq8064" for apq8064
 			- "qcom,pcie-apq8084" for apq8084
+			- "qcom,pcie-msm8996" for msm8996 or apq8096
 
 - reg:
 	Usage: required
@@ -92,6 +93,17 @@
 			- "aux"		Auxiliary (AUX) clock
 			- "bus_master"	Master AXI clock
 			- "bus_slave"	Slave AXI clock
+
+- clock-names:
+	Usage: required for msm8996/apq8096
+	Value type: <stringlist>
+	Definition: Should contain the following entries
+			- "pipe"	Pipe Clock driving internal logic.
+			- "aux"		Auxiliary (AUX) clock.
+			- "cfg"		Configuration clk.
+			- "bus_master"	Master AXI clock.
+			- "bus_slave"	Slave AXI clock.
+
 - resets:
 	Usage: required
 	Value type: <prop-encoded-array>
@@ -115,7 +127,7 @@
 			- "core" Core reset
 
 - power-domains:
-	Usage: required for apq8084
+	Usage: required for apq8084 and msm8996/apq8096
 	Value type: <prop-encoded-array>
 	Definition: A phandle and power domain specifier pair to the
 		    power domain which is responsible for collapsing
@@ -231,3 +243,56 @@
 		pinctrl-0 = <&pcie0_pins_default>;
 		pinctrl-names = "default";
 	};
+
+* Example for apq8096:
+
+	pcie@608000{
+		compatible = "qcom,pcie-msm8996", "snps,dw-pcie";
+		power-domains = <&gcc PCIE1_GDSC>;
+		bus-range = <0x00 0xff>;
+		num-lanes = <1>;
+
+		reg = <0x00608000 0x2000>,
+		      <0x0d000000 0xf1d>,
+		      <0x0d000f20 0xa8>,
+		      <0x0d100000 0x100000>;
+
+		reg-names = "parf", "dbi", "elbi", "config";
+
+		phys = <&pcie_phy 1>;
+		phy-names = "pciephy";
+
+		#address-cells = <3>;
+		#size-cells = <2>;
+		ranges = <0x01000000 0x0 0x0d200000 0x0d200000 0x0 0x100000>,
+			<0x02000000 0x0 0x0d300000 0x0d300000 0x0 0xd00000>;
+
+		interrupts = <GIC_SPI 413 IRQ_TYPE_NONE>;
+		interrupt-names = "msi";
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0x7>;
+		interrupt-map = <0 0 0 1 &intc 0 272 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+				<0 0 0 2 &intc 0 273 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+				<0 0 0 3 &intc 0 274 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+				<0 0 0 4 &intc 0 275 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+
+		pinctrl-names = "default", "sleep";
+		pinctrl-0 = <&pcie1_clkreq_default &pcie1_perst_default &pcie1_wake_default>;
+		pinctrl-1 = <&pcie1_clkreq_sleep &pcie1_perst_default &pcie1_wake_sleep>;
+
+		vdda-1p8-supply = <&pm8994_l12>;
+		vdda-supply = <&pm8994_l28>;
+		linux,pci-domain = <1>;
+
+		clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
+			<&gcc GCC_PCIE_1_AUX_CLK>,
+			<&gcc GCC_PCIE_1_CFG_AHB_CLK>,
+			<&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
+			<&gcc GCC_PCIE_1_SLV_AXI_CLK>;
+
+		clock-names =  "pipe",
+				"aux",
+				"cfg",
+				"bus_master",
+				"bus_slave";
+	};
diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
index 3593640..03ba6b1 100644
--- a/drivers/pci/host/pcie-qcom.c
+++ b/drivers/pci/host/pcie-qcom.c
@@ -36,11 +36,19 @@
 
 #include "pcie-designware.h"
 
+#define PCIE20_PARF_DBI_BASE_ADDR	0x168
+
+#define PCIE20_PARF_SYS_CTRL			0x00
 #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_MHI_CLOCK_RESET_CTRL	0x174
 #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
+#define MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT   0x1A8
+#define PCIE20_PARF_LTSSM			0x1B0
+#define PCIE20_PARF_SID_OFFSET			0x234
+#define PCIE20_PARF_BDF_TRANSLATE_CFG		0x24C
 
 #define PCIE20_ELBI_SYS_CTRL			0x04
 #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE		BIT(0)
@@ -72,9 +80,18 @@ struct qcom_pcie_resources_v1 {
 	struct regulator *vdda;
 };
 
+struct qcom_pcie_resources_v2 {
+	struct clk *aux_clk;
+	struct clk *master_clk;
+	struct clk *slave_clk;
+	struct clk *cfg_clk;
+	struct clk *pipe_clk;
+};
+
 union qcom_pcie_resources {
 	struct qcom_pcie_resources_v0 v0;
 	struct qcom_pcie_resources_v1 v1;
+	struct qcom_pcie_resources_v2 v2;
 };
 
 struct qcom_pcie;
@@ -82,7 +99,9 @@ struct qcom_pcie;
 struct qcom_pcie_ops {
 	int (*get_resources)(struct qcom_pcie *pcie);
 	int (*init)(struct qcom_pcie *pcie);
+	int (*post_init)(struct qcom_pcie *pcie);
 	void (*deinit)(struct qcom_pcie *pcie);
+	void (*ltssm_enable)(struct qcom_pcie *pcie);
 };
 
 struct qcom_pcie {
@@ -116,17 +135,33 @@ static irqreturn_t qcom_pcie_msi_irq_handler(int irq, void *arg)
 	return dw_handle_msi_irq(pp);
 }
 
-static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
+static void qcom_pcie_v0_v1_ltssm_enable(struct qcom_pcie *pcie)
 {
 	u32 val;
-
-	if (dw_pcie_link_up(&pcie->pp))
-		return 0;
-
 	/* 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);
+}
+
+static void qcom_pcie_v2_ltssm_enable(struct qcom_pcie *pcie)
+{
+	u32 val;
+	/* enable link training */
+	val = readl(pcie->parf + PCIE20_PARF_LTSSM);
+	val |= BIT(8);
+	writel(val, pcie->parf + PCIE20_PARF_LTSSM);
+}
+
+static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
+{
+
+	if (dw_pcie_link_up(&pcie->pp))
+		return 0;
+
+	/* Enable Link Training state machine */
+	if (pcie->ops->ltssm_enable)
+		pcie->ops->ltssm_enable(pcie);
 
 	return dw_pcie_wait_for_link(&pcie->pp);
 }
@@ -421,6 +456,113 @@ static int qcom_pcie_init_v1(struct qcom_pcie *pcie)
 	return ret;
 }
 
+static int qcom_pcie_get_resources_v2(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+	struct device *dev = pcie->pp.dev;
+
+	res->aux_clk = devm_clk_get(dev, "aux");
+	if (IS_ERR(res->aux_clk))
+		return PTR_ERR(res->aux_clk);
+
+	res->cfg_clk = devm_clk_get(dev, "cfg");
+	if (IS_ERR(res->cfg_clk))
+		return PTR_ERR(res->cfg_clk);
+
+	res->master_clk = devm_clk_get(dev, "bus_master");
+	if (IS_ERR(res->master_clk))
+		return PTR_ERR(res->master_clk);
+
+	res->slave_clk = devm_clk_get(dev, "bus_slave");
+	if (IS_ERR(res->slave_clk))
+		return PTR_ERR(res->slave_clk);
+
+	res->pipe_clk = devm_clk_get(dev, "pipe");
+	if (IS_ERR(res->pipe_clk))
+		return PTR_ERR(res->pipe_clk);
+
+	return 0;
+}
+
+static int qcom_pcie_init_v2(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+	struct device *dev = pcie->pp.dev;
+	u32 val;
+	int ret;
+
+	ret = clk_prepare_enable(res->aux_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable aux clock\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(res->cfg_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable cfg clock\n");
+		goto err_cfg_clk;
+	}
+
+	ret = clk_prepare_enable(res->master_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable master clock\n");
+		goto err_master_clk;
+	}
+
+	ret = clk_prepare_enable(res->slave_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable slave clock\n");
+		goto err_slave_clk;
+	}
+
+	/* enable PCIe clocks and resets */
+	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
+	val &= ~BIT(0);
+	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
+
+	/* change DBI base address */
+	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
+
+	/* MAC PHY_POWERDOWN MUX DISABLE  */
+	val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
+	val &= ~BIT(29);
+	writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
+
+	val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+	val |= BIT(4);
+	writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
+
+	val = readl(pcie->parf + MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+	val |= BIT(31);
+	writel(val, pcie->parf + MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
+
+	return 0;
+
+err_slave_clk:
+	clk_disable_unprepare(res->master_clk);
+err_master_clk:
+	clk_disable_unprepare(res->cfg_clk);
+err_cfg_clk:
+	clk_disable_unprepare(res->aux_clk);
+
+	return ret;
+}
+
+static int qcom_pcie_post_init_v2(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+	struct device *dev = pcie->pp.dev;
+	int ret;
+
+	ret = clk_prepare_enable(res->pipe_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable pipe clock\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int qcom_pcie_link_up(struct pcie_port *pp)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pp);
@@ -429,6 +571,17 @@ static int qcom_pcie_link_up(struct pcie_port *pp)
 	return !!(val & PCI_EXP_LNKSTA_DLLLA);
 }
 
+static void qcom_pcie_deinit_v2(struct qcom_pcie *pcie)
+{
+	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
+
+	clk_disable_unprepare(res->pipe_clk);
+	clk_disable_unprepare(res->slave_clk);
+	clk_disable_unprepare(res->master_clk);
+	clk_disable_unprepare(res->cfg_clk);
+	clk_disable_unprepare(res->aux_clk);
+}
+
 static void qcom_pcie_host_init(struct pcie_port *pp)
 {
 	struct qcom_pcie *pcie = to_qcom_pcie(pp);
@@ -444,6 +597,9 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
 	if (ret)
 		goto err_deinit;
 
+	if (pcie->ops->post_init)
+		pcie->ops->post_init(pcie);
+
 	dw_pcie_setup_rc(pp);
 
 	if (IS_ENABLED(CONFIG_PCI_MSI))
@@ -487,12 +643,22 @@ 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,
+	.ltssm_enable = qcom_pcie_v0_v1_ltssm_enable,
 };
 
 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,
+	.ltssm_enable = qcom_pcie_v0_v1_ltssm_enable,
+};
+
+static const struct qcom_pcie_ops ops_v2 = {
+	.get_resources = qcom_pcie_get_resources_v2,
+	.init = qcom_pcie_init_v2,
+	.post_init = qcom_pcie_post_init_v2,
+	.deinit = qcom_pcie_deinit_v2,
+	.ltssm_enable = qcom_pcie_v2_ltssm_enable,
 };
 
 static int qcom_pcie_probe(struct platform_device *pdev)
@@ -572,6 +738,7 @@ 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 },
+	{ .compatible = "qcom,pcie-msm8996", .data = &ops_v2 },
 	{ }
 };
 
-- 
2.10.1

Re: [PATCH v4 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Vivek Gautam]

On Mon, Nov 14, 2016 at 4:45 PM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
>
> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
> pipe clocks are only setup after the phy is powered on.
> It also adds ltssm_enable callback as it is very much different to other
> supported SOCs in the driver.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---

Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>


Thanks

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Bjorn Helgaas]

On Mon, Nov 14, 2016 at 11:15:54AM +0000, Srinivas Kandagatla wrote:
> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
> 
> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
> pipe clocks are only setup after the phy is powered on.
> It also adds ltssm_enable callback as it is very much different to other
> supported SOCs in the driver.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Will need ack from Stanimir before I can apply it.

> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt          |  67 +++++++-
>  drivers/pci/host/pcie-qcom.c                       | 177 ++++++++++++++++++++-
>  2 files changed, 238 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 4059a6f..141d8c3 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -7,6 +7,7 @@
>  			- "qcom,pcie-ipq8064" for ipq8064
>  			- "qcom,pcie-apq8064" for apq8064
>  			- "qcom,pcie-apq8084" for apq8084
> +			- "qcom,pcie-msm8996" for msm8996 or apq8096
>  
>  - reg:
>  	Usage: required
> @@ -92,6 +93,17 @@
>  			- "aux"		Auxiliary (AUX) clock
>  			- "bus_master"	Master AXI clock
>  			- "bus_slave"	Slave AXI clock
> +
> +- clock-names:
> +	Usage: required for msm8996/apq8096
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "pipe"	Pipe Clock driving internal logic.
> +			- "aux"		Auxiliary (AUX) clock.
> +			- "cfg"		Configuration clk.
> +			- "bus_master"	Master AXI clock.
> +			- "bus_slave"	Slave AXI clock.
> +
>  - resets:
>  	Usage: required
>  	Value type: <prop-encoded-array>
> @@ -115,7 +127,7 @@
>  			- "core" Core reset
>  
>  - power-domains:
> -	Usage: required for apq8084
> +	Usage: required for apq8084 and msm8996/apq8096
>  	Value type: <prop-encoded-array>
>  	Definition: A phandle and power domain specifier pair to the
>  		    power domain which is responsible for collapsing
> @@ -231,3 +243,56 @@
>  		pinctrl-0 = <&pcie0_pins_default>;
>  		pinctrl-names = "default";
>  	};
> +
> +* Example for apq8096:
> +
> +	pcie@608000{
> +		compatible = "qcom,pcie-msm8996", "snps,dw-pcie";
> +		power-domains = <&gcc PCIE1_GDSC>;
> +		bus-range = <0x00 0xff>;
> +		num-lanes = <1>;
> +
> +		reg = <0x00608000 0x2000>,
> +		      <0x0d000000 0xf1d>,
> +		      <0x0d000f20 0xa8>,
> +		      <0x0d100000 0x100000>;
> +
> +		reg-names = "parf", "dbi", "elbi", "config";
> +
> +		phys = <&pcie_phy 1>;
> +		phy-names = "pciephy";
> +
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		ranges = <0x01000000 0x0 0x0d200000 0x0d200000 0x0 0x100000>,
> +			<0x02000000 0x0 0x0d300000 0x0d300000 0x0 0xd00000>;
> +
> +		interrupts = <GIC_SPI 413 IRQ_TYPE_NONE>;
> +		interrupt-names = "msi";
> +		#interrupt-cells = <1>;
> +		interrupt-map-mask = <0 0 0 0x7>;
> +		interrupt-map = <0 0 0 1 &intc 0 272 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> +				<0 0 0 2 &intc 0 273 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +				<0 0 0 3 &intc 0 274 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +				<0 0 0 4 &intc 0 275 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> +		pinctrl-names = "default", "sleep";
> +		pinctrl-0 = <&pcie1_clkreq_default &pcie1_perst_default &pcie1_wake_default>;
> +		pinctrl-1 = <&pcie1_clkreq_sleep &pcie1_perst_default &pcie1_wake_sleep>;
> +
> +		vdda-1p8-supply = <&pm8994_l12>;
> +		vdda-supply = <&pm8994_l28>;
> +		linux,pci-domain = <1>;
> +
> +		clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> +			<&gcc GCC_PCIE_1_AUX_CLK>,
> +			<&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> +			<&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
> +			<&gcc GCC_PCIE_1_SLV_AXI_CLK>;
> +
> +		clock-names =  "pipe",
> +				"aux",
> +				"cfg",
> +				"bus_master",
> +				"bus_slave";
> +	};
> diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
> index 3593640..03ba6b1 100644
> --- a/drivers/pci/host/pcie-qcom.c
> +++ b/drivers/pci/host/pcie-qcom.c
> @@ -36,11 +36,19 @@
>  
>  #include "pcie-designware.h"
>  
> +#define PCIE20_PARF_DBI_BASE_ADDR	0x168
> +
> +#define PCIE20_PARF_SYS_CTRL			0x00
>  #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_MHI_CLOCK_RESET_CTRL	0x174
>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
> +#define MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT   0x1A8
> +#define PCIE20_PARF_LTSSM			0x1B0
> +#define PCIE20_PARF_SID_OFFSET			0x234
> +#define PCIE20_PARF_BDF_TRANSLATE_CFG		0x24C
>  
>  #define PCIE20_ELBI_SYS_CTRL			0x04
>  #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE		BIT(0)
> @@ -72,9 +80,18 @@ struct qcom_pcie_resources_v1 {
>  	struct regulator *vdda;
>  };
>  
> +struct qcom_pcie_resources_v2 {
> +	struct clk *aux_clk;
> +	struct clk *master_clk;
> +	struct clk *slave_clk;
> +	struct clk *cfg_clk;
> +	struct clk *pipe_clk;
> +};
> +
>  union qcom_pcie_resources {
>  	struct qcom_pcie_resources_v0 v0;
>  	struct qcom_pcie_resources_v1 v1;
> +	struct qcom_pcie_resources_v2 v2;
>  };
>  
>  struct qcom_pcie;
> @@ -82,7 +99,9 @@ struct qcom_pcie;
>  struct qcom_pcie_ops {
>  	int (*get_resources)(struct qcom_pcie *pcie);
>  	int (*init)(struct qcom_pcie *pcie);
> +	int (*post_init)(struct qcom_pcie *pcie);
>  	void (*deinit)(struct qcom_pcie *pcie);
> +	void (*ltssm_enable)(struct qcom_pcie *pcie);
>  };
>  
>  struct qcom_pcie {
> @@ -116,17 +135,33 @@ static irqreturn_t qcom_pcie_msi_irq_handler(int irq, void *arg)
>  	return dw_handle_msi_irq(pp);
>  }
>  
> -static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
> +static void qcom_pcie_v0_v1_ltssm_enable(struct qcom_pcie *pcie)
>  {
>  	u32 val;
> -
> -	if (dw_pcie_link_up(&pcie->pp))
> -		return 0;
> -
>  	/* 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);
> +}
> +
> +static void qcom_pcie_v2_ltssm_enable(struct qcom_pcie *pcie)
> +{
> +	u32 val;
> +	/* enable link training */
> +	val = readl(pcie->parf + PCIE20_PARF_LTSSM);
> +	val |= BIT(8);
> +	writel(val, pcie->parf + PCIE20_PARF_LTSSM);
> +}
> +
> +static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
> +{
> +
> +	if (dw_pcie_link_up(&pcie->pp))
> +		return 0;
> +
> +	/* Enable Link Training state machine */
> +	if (pcie->ops->ltssm_enable)
> +		pcie->ops->ltssm_enable(pcie);
>  
>  	return dw_pcie_wait_for_link(&pcie->pp);
>  }
> @@ -421,6 +456,113 @@ static int qcom_pcie_init_v1(struct qcom_pcie *pcie)
>  	return ret;
>  }
>  
> +static int qcom_pcie_get_resources_v2(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +	struct device *dev = pcie->pp.dev;
> +
> +	res->aux_clk = devm_clk_get(dev, "aux");
> +	if (IS_ERR(res->aux_clk))
> +		return PTR_ERR(res->aux_clk);
> +
> +	res->cfg_clk = devm_clk_get(dev, "cfg");
> +	if (IS_ERR(res->cfg_clk))
> +		return PTR_ERR(res->cfg_clk);
> +
> +	res->master_clk = devm_clk_get(dev, "bus_master");
> +	if (IS_ERR(res->master_clk))
> +		return PTR_ERR(res->master_clk);
> +
> +	res->slave_clk = devm_clk_get(dev, "bus_slave");
> +	if (IS_ERR(res->slave_clk))
> +		return PTR_ERR(res->slave_clk);
> +
> +	res->pipe_clk = devm_clk_get(dev, "pipe");
> +	if (IS_ERR(res->pipe_clk))
> +		return PTR_ERR(res->pipe_clk);
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_init_v2(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +	struct device *dev = pcie->pp.dev;
> +	u32 val;
> +	int ret;
> +
> +	ret = clk_prepare_enable(res->aux_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable aux clock\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(res->cfg_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable cfg clock\n");
> +		goto err_cfg_clk;
> +	}
> +
> +	ret = clk_prepare_enable(res->master_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable master clock\n");
> +		goto err_master_clk;
> +	}
> +
> +	ret = clk_prepare_enable(res->slave_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable slave clock\n");
> +		goto err_slave_clk;
> +	}
> +
> +	/* enable PCIe clocks and resets */
> +	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> +	val &= ~BIT(0);
> +	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> +
> +	/* change DBI base address */
> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> +
> +	/* MAC PHY_POWERDOWN MUX DISABLE  */
> +	val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> +	val &= ~BIT(29);
> +	writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> +
> +	val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> +	val |= BIT(4);
> +	writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> +
> +	val = readl(pcie->parf + MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> +	val |= BIT(31);
> +	writel(val, pcie->parf + MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> +
> +	return 0;
> +
> +err_slave_clk:
> +	clk_disable_unprepare(res->master_clk);
> +err_master_clk:
> +	clk_disable_unprepare(res->cfg_clk);
> +err_cfg_clk:
> +	clk_disable_unprepare(res->aux_clk);
> +
> +	return ret;
> +}
> +
> +static int qcom_pcie_post_init_v2(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +	struct device *dev = pcie->pp.dev;
> +	int ret;
> +
> +	ret = clk_prepare_enable(res->pipe_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable pipe clock\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int qcom_pcie_link_up(struct pcie_port *pp)
>  {
>  	struct qcom_pcie *pcie = to_qcom_pcie(pp);
> @@ -429,6 +571,17 @@ static int qcom_pcie_link_up(struct pcie_port *pp)
>  	return !!(val & PCI_EXP_LNKSTA_DLLLA);
>  }
>  
> +static void qcom_pcie_deinit_v2(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +
> +	clk_disable_unprepare(res->pipe_clk);
> +	clk_disable_unprepare(res->slave_clk);
> +	clk_disable_unprepare(res->master_clk);
> +	clk_disable_unprepare(res->cfg_clk);
> +	clk_disable_unprepare(res->aux_clk);
> +}
> +
>  static void qcom_pcie_host_init(struct pcie_port *pp)
>  {
>  	struct qcom_pcie *pcie = to_qcom_pcie(pp);
> @@ -444,6 +597,9 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
>  	if (ret)
>  		goto err_deinit;
>  
> +	if (pcie->ops->post_init)
> +		pcie->ops->post_init(pcie);
> +
>  	dw_pcie_setup_rc(pp);
>  
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
> @@ -487,12 +643,22 @@ 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,
> +	.ltssm_enable = qcom_pcie_v0_v1_ltssm_enable,
>  };
>  
>  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,
> +	.ltssm_enable = qcom_pcie_v0_v1_ltssm_enable,
> +};
> +
> +static const struct qcom_pcie_ops ops_v2 = {
> +	.get_resources = qcom_pcie_get_resources_v2,
> +	.init = qcom_pcie_init_v2,
> +	.post_init = qcom_pcie_post_init_v2,
> +	.deinit = qcom_pcie_deinit_v2,
> +	.ltssm_enable = qcom_pcie_v2_ltssm_enable,
>  };
>  
>  static int qcom_pcie_probe(struct platform_device *pdev)
> @@ -572,6 +738,7 @@ 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 },
> +	{ .compatible = "qcom,pcie-msm8996", .data = &ops_v2 },
>  	{ }
>  };
>  
> -- 
> 2.10.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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/3] PCI: qcom: add support to msm8996 PCIE controller

[Bjorn Helgaas]

On Mon, Nov 14, 2016 at 11:15:54AM +0000, Srinivas Kandagatla wrote:
> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
> 
> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
> pipe clocks are only setup after the phy is powered on.
> It also adds ltssm_enable callback as it is very much different to other
> supported SOCs in the driver.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Will need ack from Stanimir before I can apply it.

> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt          |  67 +++++++-
>  drivers/pci/host/pcie-qcom.c                       | 177 ++++++++++++++++++++-
>  2 files changed, 238 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 4059a6f..141d8c3 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -7,6 +7,7 @@
>  			- "qcom,pcie-ipq8064" for ipq8064
>  			- "qcom,pcie-apq8064" for apq8064
>  			- "qcom,pcie-apq8084" for apq8084
> +			- "qcom,pcie-msm8996" for msm8996 or apq8096
>  
>  - reg:
>  	Usage: required
> @@ -92,6 +93,17 @@
>  			- "aux"		Auxiliary (AUX) clock
>  			- "bus_master"	Master AXI clock
>  			- "bus_slave"	Slave AXI clock
> +
> +- clock-names:
> +	Usage: required for msm8996/apq8096
> +	Value type: <stringlist>
> +	Definition: Should contain the following entries
> +			- "pipe"	Pipe Clock driving internal logic.
> +			- "aux"		Auxiliary (AUX) clock.
> +			- "cfg"		Configuration clk.
> +			- "bus_master"	Master AXI clock.
> +			- "bus_slave"	Slave AXI clock.
> +
>  - resets:
>  	Usage: required
>  	Value type: <prop-encoded-array>
> @@ -115,7 +127,7 @@
>  			- "core" Core reset
>  
>  - power-domains:
> -	Usage: required for apq8084
> +	Usage: required for apq8084 and msm8996/apq8096
>  	Value type: <prop-encoded-array>
>  	Definition: A phandle and power domain specifier pair to the
>  		    power domain which is responsible for collapsing
> @@ -231,3 +243,56 @@
>  		pinctrl-0 = <&pcie0_pins_default>;
>  		pinctrl-names = "default";
>  	};
> +
> +* Example for apq8096:
> +
> +	pcie@608000{
> +		compatible = "qcom,pcie-msm8996", "snps,dw-pcie";
> +		power-domains = <&gcc PCIE1_GDSC>;
> +		bus-range = <0x00 0xff>;
> +		num-lanes = <1>;
> +
> +		reg = <0x00608000 0x2000>,
> +		      <0x0d000000 0xf1d>,
> +		      <0x0d000f20 0xa8>,
> +		      <0x0d100000 0x100000>;
> +
> +		reg-names = "parf", "dbi", "elbi", "config";
> +
> +		phys = <&pcie_phy 1>;
> +		phy-names = "pciephy";
> +
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		ranges = <0x01000000 0x0 0x0d200000 0x0d200000 0x0 0x100000>,
> +			<0x02000000 0x0 0x0d300000 0x0d300000 0x0 0xd00000>;
> +
> +		interrupts = <GIC_SPI 413 IRQ_TYPE_NONE>;
> +		interrupt-names = "msi";
> +		#interrupt-cells = <1>;
> +		interrupt-map-mask = <0 0 0 0x7>;
> +		interrupt-map = <0 0 0 1 &intc 0 272 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
> +				<0 0 0 2 &intc 0 273 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
> +				<0 0 0 3 &intc 0 274 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
> +				<0 0 0 4 &intc 0 275 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
> +
> +		pinctrl-names = "default", "sleep";
> +		pinctrl-0 = <&pcie1_clkreq_default &pcie1_perst_default &pcie1_wake_default>;
> +		pinctrl-1 = <&pcie1_clkreq_sleep &pcie1_perst_default &pcie1_wake_sleep>;
> +
> +		vdda-1p8-supply = <&pm8994_l12>;
> +		vdda-supply = <&pm8994_l28>;
> +		linux,pci-domain = <1>;
> +
> +		clocks = <&gcc GCC_PCIE_1_PIPE_CLK>,
> +			<&gcc GCC_PCIE_1_AUX_CLK>,
> +			<&gcc GCC_PCIE_1_CFG_AHB_CLK>,
> +			<&gcc GCC_PCIE_1_MSTR_AXI_CLK>,
> +			<&gcc GCC_PCIE_1_SLV_AXI_CLK>;
> +
> +		clock-names =  "pipe",
> +				"aux",
> +				"cfg",
> +				"bus_master",
> +				"bus_slave";
> +	};
> diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
> index 3593640..03ba6b1 100644
> --- a/drivers/pci/host/pcie-qcom.c
> +++ b/drivers/pci/host/pcie-qcom.c
> @@ -36,11 +36,19 @@
>  
>  #include "pcie-designware.h"
>  
> +#define PCIE20_PARF_DBI_BASE_ADDR	0x168
> +
> +#define PCIE20_PARF_SYS_CTRL			0x00
>  #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_MHI_CLOCK_RESET_CTRL	0x174
>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
> +#define MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT   0x1A8
> +#define PCIE20_PARF_LTSSM			0x1B0
> +#define PCIE20_PARF_SID_OFFSET			0x234
> +#define PCIE20_PARF_BDF_TRANSLATE_CFG		0x24C
>  
>  #define PCIE20_ELBI_SYS_CTRL			0x04
>  #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE		BIT(0)
> @@ -72,9 +80,18 @@ struct qcom_pcie_resources_v1 {
>  	struct regulator *vdda;
>  };
>  
> +struct qcom_pcie_resources_v2 {
> +	struct clk *aux_clk;
> +	struct clk *master_clk;
> +	struct clk *slave_clk;
> +	struct clk *cfg_clk;
> +	struct clk *pipe_clk;
> +};
> +
>  union qcom_pcie_resources {
>  	struct qcom_pcie_resources_v0 v0;
>  	struct qcom_pcie_resources_v1 v1;
> +	struct qcom_pcie_resources_v2 v2;
>  };
>  
>  struct qcom_pcie;
> @@ -82,7 +99,9 @@ struct qcom_pcie;
>  struct qcom_pcie_ops {
>  	int (*get_resources)(struct qcom_pcie *pcie);
>  	int (*init)(struct qcom_pcie *pcie);
> +	int (*post_init)(struct qcom_pcie *pcie);
>  	void (*deinit)(struct qcom_pcie *pcie);
> +	void (*ltssm_enable)(struct qcom_pcie *pcie);
>  };
>  
>  struct qcom_pcie {
> @@ -116,17 +135,33 @@ static irqreturn_t qcom_pcie_msi_irq_handler(int irq, void *arg)
>  	return dw_handle_msi_irq(pp);
>  }
>  
> -static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
> +static void qcom_pcie_v0_v1_ltssm_enable(struct qcom_pcie *pcie)
>  {
>  	u32 val;
> -
> -	if (dw_pcie_link_up(&pcie->pp))
> -		return 0;
> -
>  	/* 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);
> +}
> +
> +static void qcom_pcie_v2_ltssm_enable(struct qcom_pcie *pcie)
> +{
> +	u32 val;
> +	/* enable link training */
> +	val = readl(pcie->parf + PCIE20_PARF_LTSSM);
> +	val |= BIT(8);
> +	writel(val, pcie->parf + PCIE20_PARF_LTSSM);
> +}
> +
> +static int qcom_pcie_establish_link(struct qcom_pcie *pcie)
> +{
> +
> +	if (dw_pcie_link_up(&pcie->pp))
> +		return 0;
> +
> +	/* Enable Link Training state machine */
> +	if (pcie->ops->ltssm_enable)
> +		pcie->ops->ltssm_enable(pcie);
>  
>  	return dw_pcie_wait_for_link(&pcie->pp);
>  }
> @@ -421,6 +456,113 @@ static int qcom_pcie_init_v1(struct qcom_pcie *pcie)
>  	return ret;
>  }
>  
> +static int qcom_pcie_get_resources_v2(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +	struct device *dev = pcie->pp.dev;
> +
> +	res->aux_clk = devm_clk_get(dev, "aux");
> +	if (IS_ERR(res->aux_clk))
> +		return PTR_ERR(res->aux_clk);
> +
> +	res->cfg_clk = devm_clk_get(dev, "cfg");
> +	if (IS_ERR(res->cfg_clk))
> +		return PTR_ERR(res->cfg_clk);
> +
> +	res->master_clk = devm_clk_get(dev, "bus_master");
> +	if (IS_ERR(res->master_clk))
> +		return PTR_ERR(res->master_clk);
> +
> +	res->slave_clk = devm_clk_get(dev, "bus_slave");
> +	if (IS_ERR(res->slave_clk))
> +		return PTR_ERR(res->slave_clk);
> +
> +	res->pipe_clk = devm_clk_get(dev, "pipe");
> +	if (IS_ERR(res->pipe_clk))
> +		return PTR_ERR(res->pipe_clk);
> +
> +	return 0;
> +}
> +
> +static int qcom_pcie_init_v2(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +	struct device *dev = pcie->pp.dev;
> +	u32 val;
> +	int ret;
> +
> +	ret = clk_prepare_enable(res->aux_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable aux clock\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(res->cfg_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable cfg clock\n");
> +		goto err_cfg_clk;
> +	}
> +
> +	ret = clk_prepare_enable(res->master_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable master clock\n");
> +		goto err_master_clk;
> +	}
> +
> +	ret = clk_prepare_enable(res->slave_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable slave clock\n");
> +		goto err_slave_clk;
> +	}
> +
> +	/* enable PCIe clocks and resets */
> +	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> +	val &= ~BIT(0);
> +	writel(val, pcie->parf + PCIE20_PARF_PHY_CTRL);
> +
> +	/* change DBI base address */
> +	writel(0, pcie->parf + PCIE20_PARF_DBI_BASE_ADDR);
> +
> +	/* MAC PHY_POWERDOWN MUX DISABLE  */
> +	val = readl(pcie->parf + PCIE20_PARF_SYS_CTRL);
> +	val &= ~BIT(29);
> +	writel(val, pcie->parf + PCIE20_PARF_SYS_CTRL);
> +
> +	val = readl(pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> +	val |= BIT(4);
> +	writel(val, pcie->parf + PCIE20_PARF_MHI_CLOCK_RESET_CTRL);
> +
> +	val = readl(pcie->parf + MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> +	val |= BIT(31);
> +	writel(val, pcie->parf + MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT);
> +
> +	return 0;
> +
> +err_slave_clk:
> +	clk_disable_unprepare(res->master_clk);
> +err_master_clk:
> +	clk_disable_unprepare(res->cfg_clk);
> +err_cfg_clk:
> +	clk_disable_unprepare(res->aux_clk);
> +
> +	return ret;
> +}
> +
> +static int qcom_pcie_post_init_v2(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +	struct device *dev = pcie->pp.dev;
> +	int ret;
> +
> +	ret = clk_prepare_enable(res->pipe_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable pipe clock\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int qcom_pcie_link_up(struct pcie_port *pp)
>  {
>  	struct qcom_pcie *pcie = to_qcom_pcie(pp);
> @@ -429,6 +571,17 @@ static int qcom_pcie_link_up(struct pcie_port *pp)
>  	return !!(val & PCI_EXP_LNKSTA_DLLLA);
>  }
>  
> +static void qcom_pcie_deinit_v2(struct qcom_pcie *pcie)
> +{
> +	struct qcom_pcie_resources_v2 *res = &pcie->res.v2;
> +
> +	clk_disable_unprepare(res->pipe_clk);
> +	clk_disable_unprepare(res->slave_clk);
> +	clk_disable_unprepare(res->master_clk);
> +	clk_disable_unprepare(res->cfg_clk);
> +	clk_disable_unprepare(res->aux_clk);
> +}
> +
>  static void qcom_pcie_host_init(struct pcie_port *pp)
>  {
>  	struct qcom_pcie *pcie = to_qcom_pcie(pp);
> @@ -444,6 +597,9 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
>  	if (ret)
>  		goto err_deinit;
>  
> +	if (pcie->ops->post_init)
> +		pcie->ops->post_init(pcie);
> +
>  	dw_pcie_setup_rc(pp);
>  
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
> @@ -487,12 +643,22 @@ 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,
> +	.ltssm_enable = qcom_pcie_v0_v1_ltssm_enable,
>  };
>  
>  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,
> +	.ltssm_enable = qcom_pcie_v0_v1_ltssm_enable,
> +};
> +
> +static const struct qcom_pcie_ops ops_v2 = {
> +	.get_resources = qcom_pcie_get_resources_v2,
> +	.init = qcom_pcie_init_v2,
> +	.post_init = qcom_pcie_post_init_v2,
> +	.deinit = qcom_pcie_deinit_v2,
> +	.ltssm_enable = qcom_pcie_v2_ltssm_enable,
>  };
>  
>  static int qcom_pcie_probe(struct platform_device *pdev)
> @@ -572,6 +738,7 @@ 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 },
> +	{ .compatible = "qcom,pcie-msm8996", .data = &ops_v2 },
>  	{ }
>  };
>  
> -- 
> 2.10.1
> 
> --
> 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

Re: [PATCH v4 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Stanimir Varbanov]

Hi Srini,

On 11/14/2016 01:15 PM, Srinivas Kandagatla wrote:
> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
> 
> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
> pipe clocks are only setup after the phy is powered on.
> It also adds ltssm_enable callback as it is very much different to other
> supported SOCs in the driver.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

With below comments addressed:

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

> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt          |  67 +++++++-
>  drivers/pci/host/pcie-qcom.c                       | 177 ++++++++++++++++++++-
>  2 files changed, 238 insertions(+), 6 deletions(-)
> 

<snip>

> diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
> index 3593640..03ba6b1 100644
> --- a/drivers/pci/host/pcie-qcom.c
> +++ b/drivers/pci/host/pcie-qcom.c
> @@ -36,11 +36,19 @@
>  
>  #include "pcie-designware.h"
>  
> +#define PCIE20_PARF_DBI_BASE_ADDR	0x168

This is already defined few rows below, please drop it.

> +
> +#define PCIE20_PARF_SYS_CTRL			0x00
>  #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_MHI_CLOCK_RESET_CTRL	0x174
>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
> +#define MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT   0x1A8

I don't like MSM8996_ prefix. Could you invent a macro which depending
on controller selects proper offset?

> +#define PCIE20_PARF_LTSSM			0x1B0
> +#define PCIE20_PARF_SID_OFFSET			0x234
> +#define PCIE20_PARF_BDF_TRANSLATE_CFG		0x24C
>  
>  #define PCIE20_ELBI_SYS_CTRL			0x04
>  #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE		BIT(0)
> @@ -72,9 +80,18 @@ struct qcom_pcie_resources_v1 {
>  	struct regulator *vdda;
>  };
>  
> +struct qcom_pcie_resources_v2 {
> +	struct clk *aux_clk;
> +	struct clk *master_clk;
> +	struct clk *slave_clk;
> +	struct clk *cfg_clk;
> +	struct clk *pipe_clk;
> +};

<snip>

regards,
Stan

Re: [PATCH v4 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Srinivas Kandagatla]

On 15/11/16 12:24, Stanimir Varbanov wrote:
> Hi Srini,
>
> On 11/14/2016 01:15 PM, Srinivas Kandagatla wrote:
>> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
>> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
>> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
>>
>> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
>> pipe clocks are only setup after the phy is powered on.
>> It also adds ltssm_enable callback as it is very much different to other
>> supported SOCs in the driver.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> With below comments addressed:
>
> Acked-by: Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
Thanks for the ack.
>
>> ---
>>  .../devicetree/bindings/pci/qcom,pcie.txt          |  67 +++++++-
>>  drivers/pci/host/pcie-qcom.c                       | 177 ++++++++++++++++++++-
>>  2 files changed, 238 insertions(+), 6 deletions(-)
>>
>
> <snip>
>
>> diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
>> index 3593640..03ba6b1 100644
>> --- a/drivers/pci/host/pcie-qcom.c
>> +++ b/drivers/pci/host/pcie-qcom.c
>> @@ -36,11 +36,19 @@
>>
>>  #include "pcie-designware.h"
>>
>> +#define PCIE20_PARF_DBI_BASE_ADDR	0x168
>
> This is already defined few rows below, please drop it.
>
Yep, will remove this.
>> +
>> +#define PCIE20_PARF_SYS_CTRL			0x00
>>  #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_MHI_CLOCK_RESET_CTRL	0x174
>>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
>> +#define MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT   0x1A8
>
> I don't like MSM8996_ prefix. Could you invent a macro which depending
> on controller selects proper offset?

maybe some like this ??

#define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2    0x1A8


--srini
>
>> +#define PCIE20_PARF_LTSSM			0x1B0
>> +#define PCIE20_PARF_SID_OFFSET			0x234
>> +#define PCIE20_PARF_BDF_TRANSLATE_CFG		0x24C
>>
>>  #define PCIE20_ELBI_SYS_CTRL			0x04
>>  #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE		BIT(0)
>> @@ -72,9 +80,18 @@ struct qcom_pcie_resources_v1 {
>>  	struct regulator *vdda;
>>  };
>>
>> +struct qcom_pcie_resources_v2 {
>> +	struct clk *aux_clk;
>> +	struct clk *master_clk;
>> +	struct clk *slave_clk;
>> +	struct clk *cfg_clk;
>> +	struct clk *pipe_clk;
>> +};
>
> <snip>
>
> regards,
> Stan
>
--
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/3] PCI: qcom: add support to msm8996 PCIE controller

[Srinivas Kandagatla]

On 15/11/16 12:24, Stanimir Varbanov wrote:
> Hi Srini,
>
> On 11/14/2016 01:15 PM, Srinivas Kandagatla wrote:
>> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
>> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
>> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
>>
>> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
>> pipe clocks are only setup after the phy is powered on.
>> It also adds ltssm_enable callback as it is very much different to other
>> supported SOCs in the driver.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> With below comments addressed:
>
> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
Thanks for the ack.
>
>> ---
>>  .../devicetree/bindings/pci/qcom,pcie.txt          |  67 +++++++-
>>  drivers/pci/host/pcie-qcom.c                       | 177 ++++++++++++++++++++-
>>  2 files changed, 238 insertions(+), 6 deletions(-)
>>
>
> <snip>
>
>> diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
>> index 3593640..03ba6b1 100644
>> --- a/drivers/pci/host/pcie-qcom.c
>> +++ b/drivers/pci/host/pcie-qcom.c
>> @@ -36,11 +36,19 @@
>>
>>  #include "pcie-designware.h"
>>
>> +#define PCIE20_PARF_DBI_BASE_ADDR	0x168
>
> This is already defined few rows below, please drop it.
>
Yep, will remove this.
>> +
>> +#define PCIE20_PARF_SYS_CTRL			0x00
>>  #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_MHI_CLOCK_RESET_CTRL	0x174
>>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT	0x178
>> +#define MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT   0x1A8
>
> I don't like MSM8996_ prefix. Could you invent a macro which depending
> on controller selects proper offset?

maybe some like this ??

#define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2    0x1A8


--srini
>
>> +#define PCIE20_PARF_LTSSM			0x1B0
>> +#define PCIE20_PARF_SID_OFFSET			0x234
>> +#define PCIE20_PARF_BDF_TRANSLATE_CFG		0x24C
>>
>>  #define PCIE20_ELBI_SYS_CTRL			0x04
>>  #define PCIE20_ELBI_SYS_CTRL_LT_ENABLE		BIT(0)
>> @@ -72,9 +80,18 @@ struct qcom_pcie_resources_v1 {
>>  	struct regulator *vdda;
>>  };
>>
>> +struct qcom_pcie_resources_v2 {
>> +	struct clk *aux_clk;
>> +	struct clk *master_clk;
>> +	struct clk *slave_clk;
>> +	struct clk *cfg_clk;
>> +	struct clk *pipe_clk;
>> +};
>
> <snip>
>
> regards,
> Stan
>

Re: [PATCH v4 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Stanimir Varbanov]

Srini,

On 11/15/2016 03:22 PM, Srinivas Kandagatla wrote:
> 
> 
> On 15/11/16 12:24, Stanimir Varbanov wrote:
>> Hi Srini,
>>
>> On 11/14/2016 01:15 PM, Srinivas Kandagatla wrote:
>>> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
>>> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
>>> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
>>>
>>> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
>>> pipe clocks are only setup after the phy is powered on.
>>> It also adds ltssm_enable callback as it is very much different to other
>>> supported SOCs in the driver.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>
>> With below comments addressed:
>>
>> Acked-by: Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
> Thanks for the ack.
>>
>>> ---
>>>  .../devicetree/bindings/pci/qcom,pcie.txt          |  67 +++++++-
>>>  drivers/pci/host/pcie-qcom.c                       | 177
>>> ++++++++++++++++++++-
>>>  2 files changed, 238 insertions(+), 6 deletions(-)
>>>
>>
>> <snip>
>>
>>> diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
>>> index 3593640..03ba6b1 100644
>>> --- a/drivers/pci/host/pcie-qcom.c
>>> +++ b/drivers/pci/host/pcie-qcom.c
>>> @@ -36,11 +36,19 @@
>>>
>>>  #include "pcie-designware.h"
>>>
>>> +#define PCIE20_PARF_DBI_BASE_ADDR    0x168
>>
>> This is already defined few rows below, please drop it.
>>
> Yep, will remove this.
>>> +
>>> +#define PCIE20_PARF_SYS_CTRL            0x00
>>>  #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_MHI_CLOCK_RESET_CTRL    0x174
>>>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT    0x178
>>> +#define MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT   0x1A8
>>
>> I don't like MSM8996_ prefix. Could you invent a macro which depending
>> on controller selects proper offset?
> 
> maybe some like this ??
> 
> #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2    0x1A8

No, I wanted to preserve the name of the register offset. By that way in
the next pcie controller version we do not need to have
PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V3.

I was thinking for something like

PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT(ver)	\
		((ver) == VERSION_1 ? 0x178 : 0x1A8)

But you will need to extend qcom_pcie_ops with new member to store the
version.

It's up to you ... or we can fix it when new version of the controller
appear.

regards,
Stan
--
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/3] PCI: qcom: add support to msm8996 PCIE controller

[Stanimir Varbanov]

Srini,

On 11/15/2016 03:22 PM, Srinivas Kandagatla wrote:
> 
> 
> On 15/11/16 12:24, Stanimir Varbanov wrote:
>> Hi Srini,
>>
>> On 11/14/2016 01:15 PM, Srinivas Kandagatla wrote:
>>> This patch adds support to msm8996/apq8096 pcie, MSM8996 supports
>>> Gen 1/2, One lane, 3 pcie root-complex with support to MSI and
>>> legacy interrupts and it conforms to PCI Express Base 2.1 specification.
>>>
>>> This patch adds post_init callback to qcom_pcie_ops, as this is pcie
>>> pipe clocks are only setup after the phy is powered on.
>>> It also adds ltssm_enable callback as it is very much different to other
>>> supported SOCs in the driver.
>>>
>>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> With below comments addressed:
>>
>> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Thanks for the ack.
>>
>>> ---
>>>  .../devicetree/bindings/pci/qcom,pcie.txt          |  67 +++++++-
>>>  drivers/pci/host/pcie-qcom.c                       | 177
>>> ++++++++++++++++++++-
>>>  2 files changed, 238 insertions(+), 6 deletions(-)
>>>
>>
>> <snip>
>>
>>> diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
>>> index 3593640..03ba6b1 100644
>>> --- a/drivers/pci/host/pcie-qcom.c
>>> +++ b/drivers/pci/host/pcie-qcom.c
>>> @@ -36,11 +36,19 @@
>>>
>>>  #include "pcie-designware.h"
>>>
>>> +#define PCIE20_PARF_DBI_BASE_ADDR    0x168
>>
>> This is already defined few rows below, please drop it.
>>
> Yep, will remove this.
>>> +
>>> +#define PCIE20_PARF_SYS_CTRL            0x00
>>>  #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_MHI_CLOCK_RESET_CTRL    0x174
>>>  #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT    0x178
>>> +#define MSM8996_PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT   0x1A8
>>
>> I don't like MSM8996_ prefix. Could you invent a macro which depending
>> on controller selects proper offset?
> 
> maybe some like this ??
> 
> #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2    0x1A8

No, I wanted to preserve the name of the register offset. By that way in
the next pcie controller version we do not need to have
PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V3.

I was thinking for something like

PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT(ver)	\
		((ver) == VERSION_1 ? 0x178 : 0x1A8)

But you will need to extend qcom_pcie_ops with new member to store the
version.

It's up to you ... or we can fix it when new version of the controller
appear.

regards,
Stan

Re: [PATCH v4 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Srinivas Kandagatla]

On 15/11/16 15:08, Stanimir Varbanov wrote:
>>> I don't like MSM8996_ prefix. Could you invent a macro which depending
>>> >> on controller selects proper offset?
>> >
>> > maybe some like this ??
>> >
>> > #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2    0x1A8
> No, I wanted to preserve the name of the register offset. By that way in
> the next pcie controller version we do not need to have
> PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V3.
>
> I was thinking for something like
>
> PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT(ver)	\
> 		((ver) == VERSION_1 ? 0x178 : 0x1A8)
>
> But you will need to extend qcom_pcie_ops with new member to store the
> version.
>
> It's up to you ... or we can fix it when new version of the controller
> appear.
TBH, I don't want to add this just for this one case, looks bit over do.
So I skipped to using V2 Suffix.
We can fix later if required.

--srini

--
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/3] PCI: qcom: add support to msm8996 PCIE controller

[Srinivas Kandagatla]

On 15/11/16 15:08, Stanimir Varbanov wrote:
>>> I don't like MSM8996_ prefix. Could you invent a macro which depending
>>> >> on controller selects proper offset?
>> >
>> > maybe some like this ??
>> >
>> > #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2    0x1A8
> No, I wanted to preserve the name of the register offset. By that way in
> the next pcie controller version we do not need to have
> PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V3.
>
> I was thinking for something like
>
> PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT(ver)	\
> 		((ver) == VERSION_1 ? 0x178 : 0x1A8)
>
> But you will need to extend qcom_pcie_ops with new member to store the
> version.
>
> It's up to you ... or we can fix it when new version of the controller
> appear.
TBH, I don't want to add this just for this one case, looks bit over do.
So I skipped to using V2 Suffix.
We can fix later if required.

--srini

Re: [PATCH v4 2/3] PCI: qcom: add support to msm8996 PCIE controller

[Stanimir Varbanov]

On 11/15/2016 06:10 PM, Srinivas Kandagatla wrote:
> 
> 
> On 15/11/16 15:08, Stanimir Varbanov wrote:
>>>> I don't like MSM8996_ prefix. Could you invent a macro which depending
>>>> >> on controller selects proper offset?
>>> >
>>> > maybe some like this ??
>>> >
>>> > #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2    0x1A8
>> No, I wanted to preserve the name of the register offset. By that way in
>> the next pcie controller version we do not need to have
>> PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V3.
>>
>> I was thinking for something like
>>
>> PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT(ver)    \
>>         ((ver) == VERSION_1 ? 0x178 : 0x1A8)
>>
>> But you will need to extend qcom_pcie_ops with new member to store the
>> version.
>>
>> It's up to you ... or we can fix it when new version of the controller
>> appear.
> TBH, I don't want to add this just for this one case, looks bit over do.
> So I skipped to using V2 Suffix.
> We can fix later if required.

OK, sounds good.

regards,
Stan
--
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/3] PCI: qcom: add support to msm8996 PCIE controller

[Stanimir Varbanov]

On 11/15/2016 06:10 PM, Srinivas Kandagatla wrote:
> 
> 
> On 15/11/16 15:08, Stanimir Varbanov wrote:
>>>> I don't like MSM8996_ prefix. Could you invent a macro which depending
>>>> >> on controller selects proper offset?
>>> >
>>> > maybe some like this ??
>>> >
>>> > #define PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V2    0x1A8
>> No, I wanted to preserve the name of the register offset. By that way in
>> the next pcie controller version we do not need to have
>> PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT_V3.
>>
>> I was thinking for something like
>>
>> PCIE20_PARF_AXI_MSTR_WR_ADDR_HALT(ver)    \
>>         ((ver) == VERSION_1 ? 0x178 : 0x1A8)
>>
>> But you will need to extend qcom_pcie_ops with new member to store the
>> version.
>>
>> It's up to you ... or we can fix it when new version of the controller
>> appear.
> TBH, I don't want to add this just for this one case, looks bit over do.
> So I skipped to using V2 Suffix.
> We can fix later if required.

OK, sounds good.

regards,
Stan

[PATCH v4 3/3] PCI: qcom: add runtime pm support to pcie_port

[Srinivas Kandagatla]

This patch is required when the pcie controller sits on a bus with
its own power domain and clocks which are controlled via a bus driver
like simple pm bus. As these bus driver have runtime pm enabled, it makes
sense to update the usage counter so that the runtime pm does not suspend
the clks or power domain associated with the bus driver.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/pci/host/pcie-qcom.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
index 03ba6b1..c2ca848 100644
--- a/drivers/pci/host/pcie-qcom.c
+++ b/drivers/pci/host/pcie-qcom.c
@@ -587,6 +587,8 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
 	struct qcom_pcie *pcie = to_qcom_pcie(pp);
 	int ret;
 
+	pm_runtime_get_sync(pp->dev);
+
 	qcom_ep_reset_assert(pcie);
 
 	ret = pcie->ops->init(pcie);
@@ -617,6 +619,7 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
 	phy_power_off(pcie->phy);
 err_deinit:
 	pcie->ops->deinit(pcie);
+	pm_runtime_put_sync(pp->dev);
 }
 
 static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
@@ -673,6 +676,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	if (!pcie)
 		return -ENOMEM;
 
+	pm_runtime_enable(dev);
 	pp = &pcie->pp;
 	pcie->ops = (struct qcom_pcie_ops *)of_device_get_match_data(dev);
 
-- 
2.10.1

--
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 3/3] PCI: qcom: add runtime pm support to pcie_port

[Srinivas Kandagatla]

This patch is required when the pcie controller sits on a bus with
its own power domain and clocks which are controlled via a bus driver
like simple pm bus. As these bus driver have runtime pm enabled, it makes
sense to update the usage counter so that the runtime pm does not suspend
the clks or power domain associated with the bus driver.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/pci/host/pcie-qcom.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/host/pcie-qcom.c b/drivers/pci/host/pcie-qcom.c
index 03ba6b1..c2ca848 100644
--- a/drivers/pci/host/pcie-qcom.c
+++ b/drivers/pci/host/pcie-qcom.c
@@ -587,6 +587,8 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
 	struct qcom_pcie *pcie = to_qcom_pcie(pp);
 	int ret;
 
+	pm_runtime_get_sync(pp->dev);
+
 	qcom_ep_reset_assert(pcie);
 
 	ret = pcie->ops->init(pcie);
@@ -617,6 +619,7 @@ static void qcom_pcie_host_init(struct pcie_port *pp)
 	phy_power_off(pcie->phy);
 err_deinit:
 	pcie->ops->deinit(pcie);
+	pm_runtime_put_sync(pp->dev);
 }
 
 static int qcom_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
@@ -673,6 +676,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 	if (!pcie)
 		return -ENOMEM;
 
+	pm_runtime_enable(dev);
 	pp = &pcie->pp;
 	pcie->ops = (struct qcom_pcie_ops *)of_device_get_match_data(dev);
 
-- 
2.10.1