[PATCH] PCI: xilinx-nwl: Enable the clock through CCF

[PATCH] PCI: xilinx-nwl: Enable the clock through CCF

[Michal Simek]

From: Hyun Kwon <hyun.kwon@xilinx.com>

Simply enable clocks. There is no remove function that's why
this should be enough for simple operation.

Signed-off-by: Hyun Kwon <hyun.kwon@xilinx.com>
Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 drivers/pci/controller/pcie-xilinx-nwl.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index 8689311c5ef6..3afd4f89ba77 100644
--- a/drivers/pci/controller/pcie-xilinx-nwl.c
+++ b/drivers/pci/controller/pcie-xilinx-nwl.c
@@ -6,6 +6,7 @@
  * (C) Copyright 2014 - 2015, Xilinx, Inc.
  */
 
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -169,6 +170,7 @@ struct nwl_pcie {
 	u8 last_busno;
 	struct nwl_msi msi;
 	struct irq_domain *legacy_irq_domain;
+	struct clk *clk;
 	raw_spinlock_t leg_mask_lock;
 };
 
@@ -823,6 +825,11 @@ static int nwl_pcie_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	pcie->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pcie->clk))
+		return PTR_ERR(pcie->clk);
+	clk_prepare_enable(pcie->clk);
+
 	err = nwl_pcie_bridge_init(pcie);
 	if (err) {
 		dev_err(dev, "HW Initialization failed\n");
-- 
2.32.0

Re: [PATCH] PCI: xilinx-nwl: Enable the clock through CCF

[Krzysztof Wilczyński]

Hi Michal,

Thank you for sending the patch over!

> Simply enable clocks. There is no remove function that's why
> this should be enough for simple operation.

What clock is this?  Would it be worth mentioning what it is for
a reference (and for posterity) the commit message?

Also why it would need to be enabled and wasn't before?  Would this be
a fix for some problem?  Would this warrant a "Fixes:" tag?  And would
it need to be back-ported to stable kernels?

[...]
> @@ -823,6 +825,11 @@ static int nwl_pcie_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> +	pcie->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pcie->clk))
> +		return PTR_ERR(pcie->clk);
> +	clk_prepare_enable(pcie->clk);
> +
[...]

Almost every other user of clk_prepare_enable() would check for
potential failure, print an appropriate message, and then do the
necessary clean-up before bailing out and returning an error.

Would adding an error check for clk_prepare_enable() and printing an
error message using dev_err() be too much in this case?  If not, then
I would rather follow the pattern that other users established and
handle errors as needed.  What do you think?

	Krzysztof

Re: [PATCH] PCI: xilinx-nwl: Enable the clock through CCF

[Michal Simek]

Hi Krzysztof,

On 6/23/21 2:20 PM, Krzysztof Wilczyński wrote:
> Hi Michal,
> 
> Thank you for sending the patch over!

Thanks for review.

> 
>> Simply enable clocks. There is no remove function that's why
>> this should be enough for simple operation.
> 
> What clock is this?  Would it be worth mentioning what it is for
> a reference (and for posterity) the commit message?

It is reference clock coming to the IP. I will update commit message.


> 
> Also why it would need to be enabled and wasn't before?  Would this be
> a fix for some problem?  Would this warrant a "Fixes:" tag?  And would
> it need to be back-ported to stable kernels?

I will update commit message. Normally reference clock is enabled by
firmware but on some configurations this doesn't need to be truth that's
why it is necessary to enable it. It also records refcount for this
reference clock is good.

I will add Fixes tag to v2.

> 
> [...]
>> @@ -823,6 +825,11 @@ static int nwl_pcie_probe(struct platform_device *pdev)
>>  		return err;
>>  	}
>>  
>> +	pcie->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(pcie->clk))
>> +		return PTR_ERR(pcie->clk);
>> +	clk_prepare_enable(pcie->clk);
>> +
> [...]
> 
> Almost every other user of clk_prepare_enable() would check for
> potential failure, print an appropriate message, and then do the
> necessary clean-up before bailing out and returning an error.
> 
> Would adding an error check for clk_prepare_enable() and printing an
> error message using dev_err() be too much in this case?  If not, then
> I would rather follow the pattern that other users established and
> handle errors as needed.  What do you think?

Agree. I have added it. It is called very early and devm_ functions are
used that's why cleanup shouldn't be necessary.

I have also found that clock wasn't documented in dt binding for this IP
but we are setting it up for quite a long time.
9c8a47b484ed ("arm64: dts: xilinx: Add the clock nodes for zynqmp")

Thanks,
Michal