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@xxxxxxxxxxx>
> ---
> 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
--
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
