diff mbox series

[v2,01/10] PCIe: qcom: add missing ipq806x clocks in PCIe driver

Message ID 20200402121148.1767-2-ansuelsmth@gmail.com
State New
Headers show
Series Multiple fixes in PCIe qcom driver | expand

Commit Message

Ansuel Smith April 2, 2020, 12:11 p.m. UTC
Aux and Ref clk are missing in pcie qcom driver.
Add support in the driver to fix pcie inizialization in ipq806x.

Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver
Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 38 ++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 5 deletions(-)

Comments

Stanimir Varbanov April 8, 2020, 8:50 a.m. UTC | #1
Ansuel,

On 4/2/20 3:11 PM, Ansuel Smith wrote:
> Aux and Ref clk are missing in pcie qcom driver.
> Add support in the driver to fix pcie inizialization in ipq806x.
> 
> Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver

this should be:

Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver

and add:

Cc: stable@vger.kernel.org # v4.5+

But, I wonder, as apq8064 shares the same ops_2_1_0 how it worked until
now. Something more I cannot find such clocks for apq8064, which means
that this patch will break it.

One option is to use those new clocks only for ipq806x.

> Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 38 ++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 5ea527a6bd9f..f958c535de6e 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -88,6 +88,8 @@ struct qcom_pcie_resources_2_1_0 {
>  	struct clk *iface_clk;
>  	struct clk *core_clk;
>  	struct clk *phy_clk;
> +	struct clk *aux_clk;
> +	struct clk *ref_clk;
>  	struct reset_control *pci_reset;
>  	struct reset_control *axi_reset;
>  	struct reset_control *ahb_reset;
> @@ -246,6 +248,14 @@ static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
>  	if (IS_ERR(res->phy_clk))
>  		return PTR_ERR(res->phy_clk);
>  
> +	res->aux_clk = devm_clk_get(dev, "aux");
> +	if (IS_ERR(res->aux_clk))
> +		return PTR_ERR(res->aux_clk);
> +
> +	res->ref_clk = devm_clk_get(dev, "ref");
> +	if (IS_ERR(res->ref_clk))
> +		return PTR_ERR(res->ref_clk);
> +
>  	res->pci_reset = devm_reset_control_get_exclusive(dev, "pci");
>  	if (IS_ERR(res->pci_reset))
>  		return PTR_ERR(res->pci_reset);
> @@ -278,6 +288,8 @@ static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
>  	clk_disable_unprepare(res->iface_clk);
>  	clk_disable_unprepare(res->core_clk);
>  	clk_disable_unprepare(res->phy_clk);
> +	clk_disable_unprepare(res->aux_clk);
> +	clk_disable_unprepare(res->ref_clk);
>  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>  }
>  
> @@ -307,16 +319,28 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
>  		goto err_assert_ahb;
>  	}
>  
> +	ret = clk_prepare_enable(res->core_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable core clock\n");
> +		goto err_clk_core;
> +	}
> +
>  	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);
> +	ret = clk_prepare_enable(res->aux_clk);
>  	if (ret) {
> -		dev_err(dev, "cannot prepare/enable core clock\n");
> -		goto err_clk_core;
> +		dev_err(dev, "cannot prepare/enable aux clock\n");
> +		goto err_clk_aux;
> +	}
> +
> +	ret = clk_prepare_enable(res->ref_clk);
> +	if (ret) {
> +		dev_err(dev, "cannot prepare/enable ref clock\n");
> +		goto err_clk_ref;
>  	}
>  
>  	ret = reset_control_deassert(res->ahb_reset);
> @@ -372,10 +396,14 @@ static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
>  	return 0;
>  
>  err_deassert_ahb:
> -	clk_disable_unprepare(res->core_clk);
> -err_clk_core:
> +	clk_disable_unprepare(res->ref_clk);
> +err_clk_ref:
> +	clk_disable_unprepare(res->aux_clk);
> +err_clk_aux:
>  	clk_disable_unprepare(res->phy_clk);
>  err_clk_phy:
> +	clk_disable_unprepare(res->core_clk);
> +err_clk_core:
>  	clk_disable_unprepare(res->iface_clk);
>  err_assert_ahb:
>  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>
Ansuel Smith April 8, 2020, 12:36 p.m. UTC | #2
> PCIe driver
> 
> Ansuel,
> 
> On 4/2/20 3:11 PM, Ansuel Smith wrote:
> > Aux and Ref clk are missing in pcie qcom driver.
> > Add support in the driver to fix pcie inizialization in ipq806x.
> >
> > Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver
> 
> this should be:
> 
> Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
> 
> and add:
> 
> Cc: stable@vger.kernel.org # v4.5+
> 
> But, I wonder, as apq8064 shares the same ops_2_1_0 how it worked until
> now. Something more I cannot find such clocks for apq8064, which means
> that this patch will break it.
> 
> One option is to use those new clocks only for ipq806x.
> 

How to add this new clocks only for ipq806x? Check the compatible and add
them accordingly? 

> > Signed-off-by: Sham Muthayyan <smuthayy@codeaurora.org>
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 38
> ++++++++++++++++++++++----
> >  1 file changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c
> b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 5ea527a6bd9f..f958c535de6e 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -88,6 +88,8 @@ struct qcom_pcie_resources_2_1_0 {
> >  	struct clk *iface_clk;
> >  	struct clk *core_clk;
> >  	struct clk *phy_clk;
> > +	struct clk *aux_clk;
> > +	struct clk *ref_clk;
> >  	struct reset_control *pci_reset;
> >  	struct reset_control *axi_reset;
> >  	struct reset_control *ahb_reset;
> > @@ -246,6 +248,14 @@ static int
> qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
> >  	if (IS_ERR(res->phy_clk))
> >  		return PTR_ERR(res->phy_clk);
> >
> > +	res->aux_clk = devm_clk_get(dev, "aux");
> > +	if (IS_ERR(res->aux_clk))
> > +		return PTR_ERR(res->aux_clk);
> > +
> > +	res->ref_clk = devm_clk_get(dev, "ref");
> > +	if (IS_ERR(res->ref_clk))
> > +		return PTR_ERR(res->ref_clk);
> > +
> >  	res->pci_reset = devm_reset_control_get_exclusive(dev, "pci");
> >  	if (IS_ERR(res->pci_reset))
> >  		return PTR_ERR(res->pci_reset);
> > @@ -278,6 +288,8 @@ static void qcom_pcie_deinit_2_1_0(struct
> qcom_pcie *pcie)
> >  	clk_disable_unprepare(res->iface_clk);
> >  	clk_disable_unprepare(res->core_clk);
> >  	clk_disable_unprepare(res->phy_clk);
> > +	clk_disable_unprepare(res->aux_clk);
> > +	clk_disable_unprepare(res->ref_clk);
> >  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> >  }
> >
> > @@ -307,16 +319,28 @@ static int qcom_pcie_init_2_1_0(struct
> qcom_pcie *pcie)
> >  		goto err_assert_ahb;
> >  	}
> >
> > +	ret = clk_prepare_enable(res->core_clk);
> > +	if (ret) {
> > +		dev_err(dev, "cannot prepare/enable core clock\n");
> > +		goto err_clk_core;
> > +	}
> > +
> >  	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);
> > +	ret = clk_prepare_enable(res->aux_clk);
> >  	if (ret) {
> > -		dev_err(dev, "cannot prepare/enable core clock\n");
> > -		goto err_clk_core;
> > +		dev_err(dev, "cannot prepare/enable aux clock\n");
> > +		goto err_clk_aux;
> > +	}
> > +
> > +	ret = clk_prepare_enable(res->ref_clk);
> > +	if (ret) {
> > +		dev_err(dev, "cannot prepare/enable ref clock\n");
> > +		goto err_clk_ref;
> >  	}
> >
> >  	ret = reset_control_deassert(res->ahb_reset);
> > @@ -372,10 +396,14 @@ static int qcom_pcie_init_2_1_0(struct
> qcom_pcie *pcie)
> >  	return 0;
> >
> >  err_deassert_ahb:
> > -	clk_disable_unprepare(res->core_clk);
> > -err_clk_core:
> > +	clk_disable_unprepare(res->ref_clk);
> > +err_clk_ref:
> > +	clk_disable_unprepare(res->aux_clk);
> > +err_clk_aux:
> >  	clk_disable_unprepare(res->phy_clk);
> >  err_clk_phy:
> > +	clk_disable_unprepare(res->core_clk);
> > +err_clk_core:
> >  	clk_disable_unprepare(res->iface_clk);
> >  err_assert_ahb:
> >  	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> >
> 
> --
> regards,
> Stan
Stanimir Varbanov April 8, 2020, 12:48 p.m. UTC | #3
Hi Ansuel,

On 4/8/20 3:36 PM, ansuelsmth@gmail.com wrote:
>> PCIe driver
>>
>> Ansuel,
>>
>> On 4/2/20 3:11 PM, Ansuel Smith wrote:
>>> Aux and Ref clk are missing in pcie qcom driver.
>>> Add support in the driver to fix pcie inizialization in ipq806x.
>>>
>>> Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver
>>
>> this should be:
>>
>> Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
>>
>> and add:
>>
>> Cc: stable@vger.kernel.org # v4.5+
>>
>> But, I wonder, as apq8064 shares the same ops_2_1_0 how it worked until
>> now. Something more I cannot find such clocks for apq8064, which means
>> that this patch will break it.
>>
>> One option is to use those new clocks only for ipq806x.
>>
> 
> How to add this new clocks only for ipq806x? Check the compatible and add
> them accordingly? 
> 

Yes, through of_device_is_compatible(). See how we done this in
qcom_pcie_get_resources_2_4_0.

I thought about second option though - encoder what clocks we have for
any SoC but if you take into that direction you have to change the whole
driver :)

Another option is to use clk_get_optional() for the clocks which you
have on ipq806x (and don't have on apq8064). Please research this one
first.
Ansuel Smith April 8, 2020, 12:55 p.m. UTC | #4
> in PCIe driver
> 
> Hi Ansuel,
> 
> On 4/8/20 3:36 PM, ansuelsmth@gmail.com wrote:
> >> PCIe driver
> >>
> >> Ansuel,
> >>
> >> On 4/2/20 3:11 PM, Ansuel Smith wrote:
> >>> Aux and Ref clk are missing in pcie qcom driver.
> >>> Add support in the driver to fix pcie inizialization in ipq806x.
> >>>
> >>> Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver
> >>
> >> this should be:
> >>
> >> Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
> >>
> >> and add:
> >>
> >> Cc: stable@vger.kernel.org # v4.5+
> >>
> >> But, I wonder, as apq8064 shares the same ops_2_1_0 how it worked
> until
> >> now. Something more I cannot find such clocks for apq8064, which
> means
> >> that this patch will break it.
> >>
> >> One option is to use those new clocks only for ipq806x.
> >>
> >
> > How to add this new clocks only for ipq806x? Check the compatible and
> add
> > them accordingly?
> >
> 
> Yes, through of_device_is_compatible(). See how we done this in
> qcom_pcie_get_resources_2_4_0.
> 
> I thought about second option though - encoder what clocks we have for
> any SoC but if you take into that direction you have to change the whole
> driver :)
> 
> Another option is to use clk_get_optional() for the clocks which you
> have on ipq806x (and don't have on apq8064). Please research this one
> first.
> 
> --
> regards,
> Stan

Ok I will use get optional for the extra clocks. Should I add a warning if they 
are not present? Also what about the extra reset? Should I follow the same
approach? 
Thx for the suggestions.
Stanimir Varbanov April 8, 2020, 1:06 p.m. UTC | #5
On 4/8/20 3:55 PM, ansuelsmth@gmail.com wrote:
>> in PCIe driver
>>
>> Hi Ansuel,
>>
>> On 4/8/20 3:36 PM, ansuelsmth@gmail.com wrote:
>>>> PCIe driver
>>>>
>>>> Ansuel,
>>>>
>>>> On 4/2/20 3:11 PM, Ansuel Smith wrote:
>>>>> Aux and Ref clk are missing in pcie qcom driver.
>>>>> Add support in the driver to fix pcie inizialization in ipq806x.
>>>>>
>>>>> Fixes: 82a82383 PCI: qcom: Add Qualcomm PCIe controller driver
>>>>
>>>> this should be:
>>>>
>>>> Fixes: 82a823833f4e PCI: qcom: Add Qualcomm PCIe controller driver
>>>>
>>>> and add:
>>>>
>>>> Cc: stable@vger.kernel.org # v4.5+
>>>>
>>>> But, I wonder, as apq8064 shares the same ops_2_1_0 how it worked
>> until
>>>> now. Something more I cannot find such clocks for apq8064, which
>> means
>>>> that this patch will break it.
>>>>
>>>> One option is to use those new clocks only for ipq806x.
>>>>
>>>
>>> How to add this new clocks only for ipq806x? Check the compatible and
>> add
>>> them accordingly?
>>>
>>
>> Yes, through of_device_is_compatible(). See how we done this in
>> qcom_pcie_get_resources_2_4_0.
>>
>> I thought about second option though - encoder what clocks we have for
>> any SoC but if you take into that direction you have to change the whole
>> driver :)
>>
>> Another option is to use clk_get_optional() for the clocks which you
>> have on ipq806x (and don't have on apq8064). Please research this one
>> first.
>>
>> --
>> regards,
>> Stan
> 
> Ok I will use get optional for the extra clocks. Should I add a warning if they 
> are not present? Also what about the extra reset? Should I follow the same
> approach? 
> Thx for the suggestions. 
> 

No warnings please. You should follow the same rules for resets.
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 5ea527a6bd9f..f958c535de6e 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -88,6 +88,8 @@  struct qcom_pcie_resources_2_1_0 {
 	struct clk *iface_clk;
 	struct clk *core_clk;
 	struct clk *phy_clk;
+	struct clk *aux_clk;
+	struct clk *ref_clk;
 	struct reset_control *pci_reset;
 	struct reset_control *axi_reset;
 	struct reset_control *ahb_reset;
@@ -246,6 +248,14 @@  static int qcom_pcie_get_resources_2_1_0(struct qcom_pcie *pcie)
 	if (IS_ERR(res->phy_clk))
 		return PTR_ERR(res->phy_clk);
 
+	res->aux_clk = devm_clk_get(dev, "aux");
+	if (IS_ERR(res->aux_clk))
+		return PTR_ERR(res->aux_clk);
+
+	res->ref_clk = devm_clk_get(dev, "ref");
+	if (IS_ERR(res->ref_clk))
+		return PTR_ERR(res->ref_clk);
+
 	res->pci_reset = devm_reset_control_get_exclusive(dev, "pci");
 	if (IS_ERR(res->pci_reset))
 		return PTR_ERR(res->pci_reset);
@@ -278,6 +288,8 @@  static void qcom_pcie_deinit_2_1_0(struct qcom_pcie *pcie)
 	clk_disable_unprepare(res->iface_clk);
 	clk_disable_unprepare(res->core_clk);
 	clk_disable_unprepare(res->phy_clk);
+	clk_disable_unprepare(res->aux_clk);
+	clk_disable_unprepare(res->ref_clk);
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
@@ -307,16 +319,28 @@  static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 		goto err_assert_ahb;
 	}
 
+	ret = clk_prepare_enable(res->core_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable core clock\n");
+		goto err_clk_core;
+	}
+
 	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);
+	ret = clk_prepare_enable(res->aux_clk);
 	if (ret) {
-		dev_err(dev, "cannot prepare/enable core clock\n");
-		goto err_clk_core;
+		dev_err(dev, "cannot prepare/enable aux clock\n");
+		goto err_clk_aux;
+	}
+
+	ret = clk_prepare_enable(res->ref_clk);
+	if (ret) {
+		dev_err(dev, "cannot prepare/enable ref clock\n");
+		goto err_clk_ref;
 	}
 
 	ret = reset_control_deassert(res->ahb_reset);
@@ -372,10 +396,14 @@  static int qcom_pcie_init_2_1_0(struct qcom_pcie *pcie)
 	return 0;
 
 err_deassert_ahb:
-	clk_disable_unprepare(res->core_clk);
-err_clk_core:
+	clk_disable_unprepare(res->ref_clk);
+err_clk_ref:
+	clk_disable_unprepare(res->aux_clk);
+err_clk_aux:
 	clk_disable_unprepare(res->phy_clk);
 err_clk_phy:
+	clk_disable_unprepare(res->core_clk);
+err_clk_core:
 	clk_disable_unprepare(res->iface_clk);
 err_assert_ahb:
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);