| Message ID | be603822953d0a815034a952b9c71bac642f22ae.1624454607.git.michal.simek@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: xilinx-nwl: Add clock handling | expand |
[+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
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
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
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
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");