[PATCH 0/2] PCI: armada8k: Fix clock resource for Armada 7K/8K

[PATCH 0/2] PCI: armada8k: Fix clock resource for Armada 7K/8K

[Gregory CLEMENT]

Hi,

This short series fixes the way the clocks are used for the PCIe host
controller embedded in the Marvell Armada 7K/8K SoCs. On these SoCs a
second one is needed in order to clock the registers. It was not
noticed until now because we relied on the bootloader and also because
the clock driver was wrong.

Thanks to this fix, it would be possible to fix the clock driver
without introducing a regression.

The first patch is just a small cleanup found when I wrote the main
patch.

Gregory CLEMENT (2):
  PCI: armada8k: Remove useless test before clk_disable_unprepare
  PCI: armada8k: Fix clock resource by adding a register clock

 Documentation/devicetree/bindings/pci/pci-armada8k.txt |  6 +++++-
 drivers/pci/dwc/pcie-armada8k.c                        | 14 ++++++++++++--
 2 files changed, 17 insertions(+), 3 deletions(-)

-- 
2.16.1

[PATCH 1/2] PCI: armada8k: Remove useless test before clk_disable_unprepare

[Gregory CLEMENT]

clk_disable_unprepare() already checks that the clock pointer is valid.
No need to test it before calling it.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/pci/dwc/pcie-armada8k.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pci/dwc/pcie-armada8k.c b/drivers/pci/dwc/pcie-armada8k.c
index b587352f8b9f..f9b1aec25c5c 100644
--- a/drivers/pci/dwc/pcie-armada8k.c
+++ b/drivers/pci/dwc/pcie-armada8k.c
@@ -247,8 +247,7 @@ static int armada8k_pcie_probe(struct platform_device *pdev)
 	return 0;
 
 fail:
-	if (!IS_ERR(pcie->clk))
-		clk_disable_unprepare(pcie->clk);
+	clk_disable_unprepare(pcie->clk);
 
 	return ret;
 }
-- 
2.16.1

[PATCH 2/2] PCI: armada8k: Fix clock resource by adding a register clock

[Gregory CLEMENT]

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
+
 
 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;
-- 
2.16.1

[PATCH 2/2] PCI: armada8k: Fix clock resource by adding a register clock

[Thomas Petazzoni]

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

[PATCH 2/2] PCI: armada8k: Fix clock resource by adding a register clock

[Russell King - ARM Linux]

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.

-- 
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

[PATCH 2/2] PCI: armada8k: Fix clock resource by adding a register clock

[Gregory CLEMENT]

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

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH 2/2] PCI: armada8k: Fix clock resource by adding a register clock

[Gregory CLEMENT]

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

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com