| Message ID | 20180228144704.12947-3-gregory.clement@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: armada8k: Fix clock resource for Armada 7K/8K | expand |
Hello, On Wed, 28 Feb 2018 15:47:04 +0100, Gregory CLEMENT wrote: > On Armada 7K/8K we need to explicitly enable the register clock. This > clock is optional because not all the SoCs using this IP need it but at > least for Armada 7K/8K it is actually mandatory. > > The binding documentation is updated accordingly. > > Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> > --- > Documentation/devicetree/bindings/pci/pci-armada8k.txt | 6 +++++- > drivers/pci/dwc/pcie-armada8k.c | 11 +++++++++++ > 2 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/pci/pci-armada8k.txt b/Documentation/devicetree/bindings/pci/pci-armada8k.txt > index c1e4c3d10a74..9948b1e9a8e5 100644 > --- a/Documentation/devicetree/bindings/pci/pci-armada8k.txt > +++ b/Documentation/devicetree/bindings/pci/pci-armada8k.txt > @@ -12,7 +12,11 @@ Required properties: > - "ctrl" for the control register region > - "config" for the config space region > - interrupts: Interrupt specifier for the PCIe controler > -- clocks: reference to the PCIe controller clock > +- clocks: reference to the PCIe controller clocks > +- clock-names: mandatory if there is a second clock, in this case the > + name must be "core" for the first clock and "reg" for the second > + one > + Unneeded new line added here. > > Example: > > diff --git a/drivers/pci/dwc/pcie-armada8k.c b/drivers/pci/dwc/pcie-armada8k.c > index f9b1aec25c5c..aa4e5cc4ab7b 100644 > --- a/drivers/pci/dwc/pcie-armada8k.c > +++ b/drivers/pci/dwc/pcie-armada8k.c > @@ -28,6 +28,7 @@ > struct armada8k_pcie { > struct dw_pcie *pci; > struct clk *clk; > + struct clk *clk_reg; > }; > > #define PCIE_VENDOR_REGS_OFFSET 0x8000 > @@ -229,6 +230,15 @@ static int armada8k_pcie_probe(struct platform_device *pdev) > if (ret) > return ret; > > + if (IS_ERR(pcie->clk_reg) && PTR_ERR(pcie->clk_reg) == -EPROBE_DEFER) { > + clk_disable_unprepare(pcie->clk); > + return -EPROBE_DEFER; > + } > + if (!IS_ERR(pcie->clk_reg)) { > + ret = clk_prepare_enable(pcie->clk_reg); > + if (ret) > + goto fail; > + } > /* Get the dw-pcie unit configuration/control registers base. */ Missing new line between the end of the block and the next comment. Regarding the error handling, doesn't it make more sense to also use a goto label to disable pcie->clk when getting the second clock gets a -EPROBE_DEFER ? > base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl"); > pci->dbi_base = devm_pci_remap_cfg_resource(dev, base); > @@ -247,6 +257,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev) > return 0; > > fail: > + clk_disable_unprepare(pcie->clk_reg); So you are disabling/unpreparing the clock, which failed to prepare/enable ? > clk_disable_unprepare(pcie->clk); > > return ret; Thomas
On Wed, Feb 28, 2018 at 03:47:04PM +0100, Gregory CLEMENT wrote: > @@ -229,6 +230,15 @@ static int armada8k_pcie_probe(struct platform_device *pdev) > if (ret) > return ret; > > + if (IS_ERR(pcie->clk_reg) && PTR_ERR(pcie->clk_reg) == -EPROBE_DEFER) { You do realise this is needlessly complex. Pointer errors are unique, so: if (pcie->clk_reg == ERR_PTR(-EPROBE_DEFER)) { will do the same thing but without the complexity. Transforming the constant rather than the variable is also a good habbit to get into - the compiler can optimise transforms to constants, but can't with variables, so comparisons involving things like endian conversion should always be done by transforming the constant not the variable.
Hi Russell King, On mer., févr. 28 2018, Russell King - ARM Linux <linux@armlinux.org.uk> wrote: > On Wed, Feb 28, 2018 at 03:47:04PM +0100, Gregory CLEMENT wrote: >> @@ -229,6 +230,15 @@ static int armada8k_pcie_probe(struct platform_device *pdev) >> if (ret) >> return ret; >> >> + if (IS_ERR(pcie->clk_reg) && PTR_ERR(pcie->clk_reg) == -EPROBE_DEFER) { > > You do realise this is needlessly complex. > > Pointer errors are unique, so: > > if (pcie->clk_reg == ERR_PTR(-EPROBE_DEFER)) { > > will do the same thing but without the complexity. Transforming the > constant rather than the variable is also a good habbit to get into - > the compiler can optimise transforms to constants, but can't with > variables, so comparisons involving things like endian conversion > should always be done by transforming the constant not the variable. Thanks for the tip, I will use it in the next version. Gregory > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up > According to speedtest.net: 8.21Mbps down 510kbps up
Hi Thomas, On mer., févr. 28 2018, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > Hello, > > On Wed, 28 Feb 2018 15:47:04 +0100, Gregory CLEMENT wrote: >> On Armada 7K/8K we need to explicitly enable the register clock. This >> clock is optional because not all the SoCs using this IP need it but at >> least for Armada 7K/8K it is actually mandatory. >> >> The binding documentation is updated accordingly. >> >> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> >> --- >> Documentation/devicetree/bindings/pci/pci-armada8k.txt | 6 +++++- >> drivers/pci/dwc/pcie-armada8k.c | 11 +++++++++++ >> 2 files changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/pci/pci-armada8k.txt b/Documentation/devicetree/bindings/pci/pci-armada8k.txt >> index c1e4c3d10a74..9948b1e9a8e5 100644 >> --- a/Documentation/devicetree/bindings/pci/pci-armada8k.txt >> +++ b/Documentation/devicetree/bindings/pci/pci-armada8k.txt >> @@ -12,7 +12,11 @@ Required properties: >> - "ctrl" for the control register region >> - "config" for the config space region >> - interrupts: Interrupt specifier for the PCIe controler >> -- clocks: reference to the PCIe controller clock >> +- clocks: reference to the PCIe controller clocks >> +- clock-names: mandatory if there is a second clock, in this case the >> + name must be "core" for the first clock and "reg" for the second >> + one >> + > > Unneeded new line added here. will removed it > >> >> Example: >> >> diff --git a/drivers/pci/dwc/pcie-armada8k.c b/drivers/pci/dwc/pcie-armada8k.c >> index f9b1aec25c5c..aa4e5cc4ab7b 100644 >> --- a/drivers/pci/dwc/pcie-armada8k.c >> +++ b/drivers/pci/dwc/pcie-armada8k.c >> @@ -28,6 +28,7 @@ >> struct armada8k_pcie { >> struct dw_pcie *pci; >> struct clk *clk; >> + struct clk *clk_reg; >> }; >> >> #define PCIE_VENDOR_REGS_OFFSET 0x8000 >> @@ -229,6 +230,15 @@ static int armada8k_pcie_probe(struct platform_device *pdev) >> if (ret) >> return ret; >> >> + if (IS_ERR(pcie->clk_reg) && PTR_ERR(pcie->clk_reg) == -EPROBE_DEFER) { >> + clk_disable_unprepare(pcie->clk); >> + return -EPROBE_DEFER; >> + } >> + if (!IS_ERR(pcie->clk_reg)) { >> + ret = clk_prepare_enable(pcie->clk_reg); >> + if (ret) >> + goto fail; >> + } >> /* Get the dw-pcie unit configuration/control registers base. */ > > Missing new line between the end of the block and the next comment. > OK > Regarding the error handling, doesn't it make more sense to also use a > goto label to disable pcie->clk when getting the second clock gets a > -EPROBE_DEFER ? > >> base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl"); >> pci->dbi_base = devm_pci_remap_cfg_resource(dev, base); >> @@ -247,6 +257,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev) >> return 0; >> >> fail: >> + clk_disable_unprepare(pcie->clk_reg); > > So you are disabling/unpreparing the clock, which failed to > prepare/enable ? I was thinking to a single user, in this case if the prepare/enable failed then the disabling/unpreparing do nothing because the counter is already to 0. But indeed in case of multiple users of the clock, then the counter could be wrongly decrease. I will modify it by adding a other label. Thanks, Gregory > >> clk_disable_unprepare(pcie->clk); >> >> return ret; > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) > Embedded Linux and Kernel engineering > http://bootlin.com
diff --git a/Documentation/devicetree/bindings/pci/pci-armada8k.txt b/Documentation/devicetree/bindings/pci/pci-armada8k.txt index c1e4c3d10a74..9948b1e9a8e5 100644 --- a/Documentation/devicetree/bindings/pci/pci-armada8k.txt +++ b/Documentation/devicetree/bindings/pci/pci-armada8k.txt @@ -12,7 +12,11 @@ Required properties: - "ctrl" for the control register region - "config" for the config space region - interrupts: Interrupt specifier for the PCIe controler -- clocks: reference to the PCIe controller clock +- clocks: reference to the PCIe controller clocks +- clock-names: mandatory if there is a second clock, in this case the + name must be "core" for the first clock and "reg" for the second + one + Example: diff --git a/drivers/pci/dwc/pcie-armada8k.c b/drivers/pci/dwc/pcie-armada8k.c index f9b1aec25c5c..aa4e5cc4ab7b 100644 --- a/drivers/pci/dwc/pcie-armada8k.c +++ b/drivers/pci/dwc/pcie-armada8k.c @@ -28,6 +28,7 @@ struct armada8k_pcie { struct dw_pcie *pci; struct clk *clk; + struct clk *clk_reg; }; #define PCIE_VENDOR_REGS_OFFSET 0x8000 @@ -229,6 +230,15 @@ static int armada8k_pcie_probe(struct platform_device *pdev) if (ret) return ret; + if (IS_ERR(pcie->clk_reg) && PTR_ERR(pcie->clk_reg) == -EPROBE_DEFER) { + clk_disable_unprepare(pcie->clk); + return -EPROBE_DEFER; + } + if (!IS_ERR(pcie->clk_reg)) { + ret = clk_prepare_enable(pcie->clk_reg); + if (ret) + goto fail; + } /* Get the dw-pcie unit configuration/control registers base. */ base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl"); pci->dbi_base = devm_pci_remap_cfg_resource(dev, base); @@ -247,6 +257,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev) return 0; fail: + clk_disable_unprepare(pcie->clk_reg); clk_disable_unprepare(pcie->clk); return ret;
On Armada 7K/8K we need to explicitly enable the register clock. This clock is optional because not all the SoCs using this IP need it but at least for Armada 7K/8K it is actually mandatory. The binding documentation is updated accordingly. Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> --- Documentation/devicetree/bindings/pci/pci-armada8k.txt | 6 +++++- drivers/pci/dwc/pcie-armada8k.c | 11 +++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-)