| Message ID | dbc0ab2e109111ca814e73abb30a1dda5d333dbe.1624449519.git.michal.simek@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: xilinx-nwl: Enable the clock through CCF | expand |
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
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
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");