[PATCH v2 0/2] PCI: xilinx-nwl: Add clock handling

[PATCH v2 0/2] PCI: xilinx-nwl: Add clock handling

[Michal Simek]

Hi,

this small series add support for enabling pcie reference clock by driver.

Thanks,
Michal

Changes in v2:
- new patch in this series because I found that it has never been sent
- Update commit message - reported by Krzysztof
- Check return value from clk_prepare_enable() - reported by Krzysztof

Hyun Kwon (1):
  PCI: xilinx-nwl: Enable the clock through CCF

Michal Simek (1):
  dt-bindings: pci: xilinx-nwl: Document optional clock property

 .../devicetree/bindings/pci/xilinx-nwl-pcie.txt      |  1 +
 drivers/pci/controller/pcie-xilinx-nwl.c             | 12 ++++++++++++
 2 files changed, 13 insertions(+)

-- 
2.32.0

[PATCH v2 1/2] dt-bindings: pci: xilinx-nwl: Document optional clock property

[Michal Simek]

Clock property hasn't been documented in binding document but it is used
for quite a long time where clock was specified by commit 9c8a47b484ed
("arm64: dts: xilinx: Add the clock nodes for zynqmp").

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v2:
- new patch in this series because I found that it has never been sent

Bharat: Can you please start to work on converting it to yaml?

---
 Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
index 2d677e90a7e2..f56f8c58c5d9 100644
--- a/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/xilinx-nwl-pcie.txt
@@ -35,6 +35,7 @@ Required properties:
 
 Optional properties:
 - dma-coherent: present if DMA operations are coherent
+- clocks: Input clock specifier. Refer to common clock bindings
 
 Example:
 ++++++++
-- 
2.32.0

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

[Michal Simek]

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

Enable PCIE reference clock. There is no remove function that's why
this should be enough for simple operation.
Normally this clock is enabled by default by firmware but there are
usecases where this clock should be enabled by driver itself.
It is also good that clock user is recorded in clock framework.

Fixes: ab597d35ef11 ("PCI: xilinx-nwl: Add support for Xilinx NWL PCIe Host Controller")
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>
---

Changes in v2:
- Update commit message - reported by Krzysztof
- Check return value from clk_prepare_enable() - reported by Krzysztof

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

diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index 8689311c5ef6..67639f5a5e79 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,16 @@ 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);
+
+	err = clk_prepare_enable(pcie->clk);
+	if (err) {
+		dev_err(dev, "can't enable pcie ref clock\n");
+		return err;
+	}
+
 	err = nwl_pcie_bridge_init(pcie);
 	if (err) {
 		dev_err(dev, "HW Initialization failed\n");
-- 
2.32.0

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

[Krzysztof Wilczyński]

[+cc Sasha for visibility]

Hi Michal,

Thank you for sending v2 so promptly!  And for all the extra changes and
fixes.  Much appreciated!

> Enable PCIE reference clock. There is no remove function that's why
> this should be enough for simple operation.
> Normally this clock is enabled by default by firmware but there are
> usecases where this clock should be enabled by driver itself.
> It is also good that clock user is recorded in clock framework.

Small nitpicks: it would be PCIe here in the above and in the error
message (this is as per [1]), and "use cases" also in the above. 

This can be corrected when the patch will be merged by either Bjorn or
Lorenzo, to avoid sending v3 unnecessarily, provided that they would
have a moment to do it, of course.

> Fixes: ab597d35ef11 ("PCI: xilinx-nwl: Add support for Xilinx NWL PCIe Host Controller")

Thank you!

Does it make sense for this change to be back-ported to stable and
long-term kernels?

I am asking to make sure we do the right thing here, as I can imagine
that older kernels (primarily because some folks could use, for example,
Ubuntu LTS releases for development) might often be used by people who
work with the Xilinx FPGAs and such.

[...]
> +	err = clk_prepare_enable(pcie->clk);
> +	if (err) {
> +		dev_err(dev, "can't enable pcie ref clock\n");
> +		return err;
> +	}
> +

As per the nitpick above, it would be "PCIe", but probably no need to
send v3 to correct this.

1. https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/

	Krzysztof

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

[Michal Simek]

Hi Krzysztof,

On 6/23/21 3:53 PM, Krzysztof Wilczyński wrote:
> [+cc Sasha for visibility]
> 
> Hi Michal,
> 
> Thank you for sending v2 so promptly!  And for all the extra changes and
> fixes.  Much appreciated!
> 
>> Enable PCIE reference clock. There is no remove function that's why
>> this should be enough for simple operation.
>> Normally this clock is enabled by default by firmware but there are
>> usecases where this clock should be enabled by driver itself.
>> It is also good that clock user is recorded in clock framework.
> 
> Small nitpicks: it would be PCIe here in the above and in the error
> message (this is as per [1]), and "use cases" also in the above. 
> 
> This can be corrected when the patch will be merged by either Bjorn or
> Lorenzo, to avoid sending v3 unnecessarily, provided that they would
> have a moment to do it, of course.

Ok. Will wait for them.

> 
>> Fixes: ab597d35ef11 ("PCI: xilinx-nwl: Add support for Xilinx NWL PCIe Host Controller")
> 
> Thank you!
> 
> Does it make sense for this change to be back-ported to stable and
> long-term kernels?
> 
> I am asking to make sure we do the right thing here, as I can imagine
> that older kernels (primarily because some folks could use, for example,
> Ubuntu LTS releases for development) might often be used by people who
> work with the Xilinx FPGAs and such.

I think that make sense to do so. I haven't had a time to take look at
it closely but I think on Xilinx ZynqMP zcu102 board this missing patch
is causing hang when standard debian 5.10 is used.


> 
> [...]
>> +	err = clk_prepare_enable(pcie->clk);
>> +	if (err) {
>> +		dev_err(dev, "can't enable pcie ref clock\n");
>> +		return err;
>> +	}
>> +
> 
> As per the nitpick above, it would be "PCIe", but probably no need to
> send v3 to correct this.

I will keep my eyes on it and will update it if v3 is required.

Thanks,
Michal

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

[Krzysztof Wilczyński]

Hi Michal,

[...]
> > Does it make sense for this change to be back-ported to stable and
> > long-term kernels?
> > 
> > I am asking to make sure we do the right thing here, as I can imagine
> > that older kernels (primarily because some folks could use, for example,
> > Ubuntu LTS releases for development) might often be used by people who
> > work with the Xilinx FPGAs and such.
> 
> I think that make sense to do so. I haven't had a time to take look at
> it closely but I think on Xilinx ZynqMP zcu102 board this missing patch
> is causing hang when standard debian 5.10 is used.

OK.  This definitely would be a good candidate for back-port then - it
might help quite a few folks to get their device going without this
troublesome hang you mentioned.

There are a few options as per:
  https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

You can send v3 adding the appropriate tag (see above link or the
comment below) or once this series (or mainly this patch) reaches Linus'
tree, then send a message to the stable maintainers mailing list to let
them know what any why to back-port.

At this point, I believe that adding the "Cc:" tag which includes the
"stable@vger.kernel.org" might be the best option as it would involve
the least amount of work to for Sasha et al.

What do you think?  Which option would you like to go for?

	Krzysztof

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

[Michal Simek]

Hi,

On 6/23/21 4:19 PM, Krzysztof Wilczyński wrote:
> Hi Michal,
> 
> [...]
>>> Does it make sense for this change to be back-ported to stable and
>>> long-term kernels?
>>>
>>> I am asking to make sure we do the right thing here, as I can imagine
>>> that older kernels (primarily because some folks could use, for example,
>>> Ubuntu LTS releases for development) might often be used by people who
>>> work with the Xilinx FPGAs and such.
>>
>> I think that make sense to do so. I haven't had a time to take look at
>> it closely but I think on Xilinx ZynqMP zcu102 board this missing patch
>> is causing hang when standard debian 5.10 is used.
> 
> OK.  This definitely would be a good candidate for back-port then - it
> might help quite a few folks to get their device going without this
> troublesome hang you mentioned.
> 
> There are a few options as per:
>   https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> 
> You can send v3 adding the appropriate tag (see above link or the
> comment below) or once this series (or mainly this patch) reaches Linus'
> tree, then send a message to the stable maintainers mailing list to let
> them know what any why to back-port.
> 
> At this point, I believe that adding the "Cc:" tag which includes the
> "stable@vger.kernel.org" might be the best option as it would involve
> the least amount of work to for Sasha et al.
> 
> What do you think?  Which option would you like to go for?

I have sent v3 with above changes.

Thanks,
Michal