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

[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

Re: [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

[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

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

[Gregory CLEMENT]

SGkgVGhvbWFzLAogCiBPbiBtZXIuLCBmw6l2ci4gMjggMjAxOCwgVGhvbWFzIFBldGF6em9uaSA8
dGhvbWFzLnBldGF6em9uaUBib290bGluLmNvbT4gd3JvdGU6Cgo+IEhlbGxvLAo+Cj4gT24gV2Vk
LCAyOCBGZWIgMjAxOCAxNTo0NzowNCArMDEwMCwgR3JlZ29yeSBDTEVNRU5UIHdyb3RlOgo+PiBP
biBBcm1hZGEgN0svOEsgd2UgbmVlZCB0byBleHBsaWNpdGx5IGVuYWJsZSB0aGUgcmVnaXN0ZXIg
Y2xvY2suIFRoaXMKPj4gY2xvY2sgaXMgb3B0aW9uYWwgYmVjYXVzZSBub3QgYWxsIHRoZSBTb0Nz
IHVzaW5nIHRoaXMgSVAgbmVlZCBpdCBidXQgYXQKPj4gbGVhc3QgZm9yIEFybWFkYSA3Sy84SyBp
dCBpcyBhY3R1YWxseSBtYW5kYXRvcnkuCj4+IAo+PiBUaGUgYmluZGluZyBkb2N1bWVudGF0aW9u
IGlzIHVwZGF0ZWQgYWNjb3JkaW5nbHkuCj4+IAo+PiBTaWduZWQtb2ZmLWJ5OiBHcmVnb3J5IENM
RU1FTlQgPGdyZWdvcnkuY2xlbWVudEBib290bGluLmNvbT4KPj4gLS0tCj4+ICBEb2N1bWVudGF0
aW9uL2RldmljZXRyZWUvYmluZGluZ3MvcGNpL3BjaS1hcm1hZGE4ay50eHQgfCAgNiArKysrKy0K
Pj4gIGRyaXZlcnMvcGNpL2R3Yy9wY2llLWFybWFkYThrLmMgICAgICAgICAgICAgICAgICAgICAg
ICB8IDExICsrKysrKysrKysrCj4+ICAyIGZpbGVzIGNoYW5nZWQsIDE2IGluc2VydGlvbnMoKyks
IDEgZGVsZXRpb24oLSkKPj4gCj4+IGRpZmYgLS1naXQgYS9Eb2N1bWVudGF0aW9uL2RldmljZXRy
ZWUvYmluZGluZ3MvcGNpL3BjaS1hcm1hZGE4ay50eHQgYi9Eb2N1bWVudGF0aW9uL2RldmljZXRy
ZWUvYmluZGluZ3MvcGNpL3BjaS1hcm1hZGE4ay50eHQKPj4gaW5kZXggYzFlNGMzZDEwYTc0Li45
OTQ4YjFlOWE4ZTUgMTAwNjQ0Cj4+IC0tLSBhL0RvY3VtZW50YXRpb24vZGV2aWNldHJlZS9iaW5k
aW5ncy9wY2kvcGNpLWFybWFkYThrLnR4dAo+PiArKysgYi9Eb2N1bWVudGF0aW9uL2RldmljZXRy
ZWUvYmluZGluZ3MvcGNpL3BjaS1hcm1hZGE4ay50eHQKPj4gQEAgLTEyLDcgKzEyLDExIEBAIFJl
cXVpcmVkIHByb3BlcnRpZXM6Cj4+ICAgICAtICJjdHJsIiBmb3IgdGhlIGNvbnRyb2wgcmVnaXN0
ZXIgcmVnaW9uCj4+ICAgICAtICJjb25maWciIGZvciB0aGUgY29uZmlnIHNwYWNlIHJlZ2lvbgo+
PiAgLSBpbnRlcnJ1cHRzOiBJbnRlcnJ1cHQgc3BlY2lmaWVyIGZvciB0aGUgUENJZSBjb250cm9s
ZXIKPj4gLS0gY2xvY2tzOiByZWZlcmVuY2UgdG8gdGhlIFBDSWUgY29udHJvbGxlciBjbG9jawo+
PiArLSBjbG9ja3M6IHJlZmVyZW5jZSB0byB0aGUgUENJZSBjb250cm9sbGVyIGNsb2Nrcwo+PiAr
LSBjbG9jay1uYW1lczogbWFuZGF0b3J5IGlmIHRoZXJlIGlzIGEgc2Vjb25kIGNsb2NrLCBpbiB0
aGlzIGNhc2UgdGhlCj4+ICsgICBuYW1lIG11c3QgYmUgImNvcmUiIGZvciB0aGUgZmlyc3QgY2xv
Y2sgYW5kICJyZWciIGZvciB0aGUgc2Vjb25kCj4+ICsgICBvbmUKPj4gKwo+Cj4gVW5uZWVkZWQg
bmV3IGxpbmUgYWRkZWQgaGVyZS4Kd2lsbCByZW1vdmVkIGl0Cgo+Cj4+ICAKPj4gIEV4YW1wbGU6
Cj4+ICAKPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvcGNpL2R3Yy9wY2llLWFybWFkYThrLmMgYi9k
cml2ZXJzL3BjaS9kd2MvcGNpZS1hcm1hZGE4ay5jCj4+IGluZGV4IGY5YjFhZWMyNWM1Yy4uYWE0
ZTVjYzRhYjdiIDEwMDY0NAo+PiAtLS0gYS9kcml2ZXJzL3BjaS9kd2MvcGNpZS1hcm1hZGE4ay5j
Cj4+ICsrKyBiL2RyaXZlcnMvcGNpL2R3Yy9wY2llLWFybWFkYThrLmMKPj4gQEAgLTI4LDYgKzI4
LDcgQEAKPj4gIHN0cnVjdCBhcm1hZGE4a19wY2llIHsKPj4gIAlzdHJ1Y3QgZHdfcGNpZSAqcGNp
Owo+PiAgCXN0cnVjdCBjbGsgKmNsazsKPj4gKwlzdHJ1Y3QgY2xrICpjbGtfcmVnOwo+PiAgfTsK
Pj4gIAo+PiAgI2RlZmluZSBQQ0lFX1ZFTkRPUl9SRUdTX09GRlNFVAkJMHg4MDAwCj4+IEBAIC0y
MjksNiArMjMwLDE1IEBAIHN0YXRpYyBpbnQgYXJtYWRhOGtfcGNpZV9wcm9iZShzdHJ1Y3QgcGxh
dGZvcm1fZGV2aWNlICpwZGV2KQo+PiAgCWlmIChyZXQpCj4+ICAJCXJldHVybiByZXQ7Cj4+ICAK
Pj4gKwlpZiAoSVNfRVJSKHBjaWUtPmNsa19yZWcpICYmIFBUUl9FUlIocGNpZS0+Y2xrX3JlZykg
PT0gLUVQUk9CRV9ERUZFUikgewo+PiArCQljbGtfZGlzYWJsZV91bnByZXBhcmUocGNpZS0+Y2xr
KTsKPj4gKwkJcmV0dXJuIC1FUFJPQkVfREVGRVI7Cj4+ICsJfQo+PiArCWlmICghSVNfRVJSKHBj
aWUtPmNsa19yZWcpKSB7Cj4+ICsJCXJldCA9IGNsa19wcmVwYXJlX2VuYWJsZShwY2llLT5jbGtf
cmVnKTsKPj4gKwkJaWYgKHJldCkKPj4gKwkJCWdvdG8gZmFpbDsKPj4gKwl9Cj4+ICAJLyogR2V0
IHRoZSBkdy1wY2llIHVuaXQgY29uZmlndXJhdGlvbi9jb250cm9sIHJlZ2lzdGVycyBiYXNlLiAq
Lwo+Cj4gTWlzc2luZyBuZXcgbGluZSBiZXR3ZWVuIHRoZSBlbmQgb2YgdGhlIGJsb2NrIGFuZCB0
aGUgbmV4dCBjb21tZW50Lgo+Ck9LCgo+IFJlZ2FyZGluZyB0aGUgZXJyb3IgaGFuZGxpbmcsIGRv
ZXNuJ3QgaXQgbWFrZSBtb3JlIHNlbnNlIHRvIGFsc28gdXNlIGEKPiBnb3RvIGxhYmVsIHRvIGRp
c2FibGUgcGNpZS0+Y2xrIHdoZW4gZ2V0dGluZyB0aGUgc2Vjb25kIGNsb2NrIGdldHMgYQo+IC1F
UFJPQkVfREVGRVIgPwo+Cj4+ICAJYmFzZSA9IHBsYXRmb3JtX2dldF9yZXNvdXJjZV9ieW5hbWUo
cGRldiwgSU9SRVNPVVJDRV9NRU0sICJjdHJsIik7Cj4+ICAJcGNpLT5kYmlfYmFzZSA9IGRldm1f
cGNpX3JlbWFwX2NmZ19yZXNvdXJjZShkZXYsIGJhc2UpOwo+PiBAQCAtMjQ3LDYgKzI1Nyw3IEBA
IHN0YXRpYyBpbnQgYXJtYWRhOGtfcGNpZV9wcm9iZShzdHJ1Y3QgcGxhdGZvcm1fZGV2aWNlICpw
ZGV2KQo+PiAgCXJldHVybiAwOwo+PiAgCj4+ICBmYWlsOgo+PiArCWNsa19kaXNhYmxlX3VucHJl
cGFyZShwY2llLT5jbGtfcmVnKTsKPgo+IFNvIHlvdSBhcmUgZGlzYWJsaW5nL3VucHJlcGFyaW5n
IHRoZSBjbG9jaywgd2hpY2ggZmFpbGVkIHRvCj4gcHJlcGFyZS9lbmFibGUgPwoKSSB3YXMgdGhp
bmtpbmcgdG8gYSBzaW5nbGUgdXNlciwgaW4gdGhpcyBjYXNlIGlmIHRoZSBwcmVwYXJlL2VuYWJs
ZQpmYWlsZWQgdGhlbiB0aGUgZGlzYWJsaW5nL3VucHJlcGFyaW5nIGRvIG5vdGhpbmcgYmVjYXVz
ZSB0aGUgY291bnRlciBpcwphbHJlYWR5IHRvIDAuIEJ1dCBpbmRlZWQgaW4gY2FzZSBvZiBtdWx0
aXBsZSB1c2VycyBvZiB0aGUgY2xvY2ssIHRoZW4gdGhlCmNvdW50ZXIgY291bGQgYmUgd3Jvbmds
eSBkZWNyZWFzZS4gSSB3aWxsIG1vZGlmeSBpdCBieSBhZGRpbmcgYSBvdGhlcgpsYWJlbC4KClRo
YW5rcywKCkdyZWdvcnkKCj4KPj4gIAljbGtfZGlzYWJsZV91bnByZXBhcmUocGNpZS0+Y2xrKTsK
Pj4gIAo+PiAgCXJldHVybiByZXQ7Cj4KPiBUaG9tYXMKPiAtLSAKPiBUaG9tYXMgUGV0YXp6b25p
LCBDVE8sIEJvb3RsaW4gKGZvcm1lcmx5IEZyZWUgRWxlY3Ryb25zKQo+IEVtYmVkZGVkIExpbnV4
IGFuZCBLZXJuZWwgZW5naW5lZXJpbmcKPiBodHRwOi8vYm9vdGxpbi5jb20KCi0tIApHcmVnb3J5
IENsZW1lbnQsIEJvb3RsaW4gKGZvcm1lcmx5IEZyZWUgRWxlY3Ryb25zKQpFbWJlZGRlZCBMaW51
eCBhbmQgS2VybmVsIGVuZ2luZWVyaW5nCmh0dHA6Ly9ib290bGluLmNvbQoKX19fX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbGludXgtYXJtLWtlcm5lbCBtYWls
aW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnCmh0dHA6Ly9saXN0
cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtYXJtLWtlcm5lbAo=

[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

Re: [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

[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

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

[Gregory CLEMENT]

SGkgUnVzc2VsbCBLaW5nLAogCiBPbiBtZXIuLCBmw6l2ci4gMjggMjAxOCwgUnVzc2VsbCBLaW5n
IC0gQVJNIExpbnV4IDxsaW51eEBhcm1saW51eC5vcmcudWs+IHdyb3RlOgoKPiBPbiBXZWQsIEZl
YiAyOCwgMjAxOCBhdCAwMzo0NzowNFBNICswMTAwLCBHcmVnb3J5IENMRU1FTlQgd3JvdGU6Cj4+
IEBAIC0yMjksNiArMjMwLDE1IEBAIHN0YXRpYyBpbnQgYXJtYWRhOGtfcGNpZV9wcm9iZShzdHJ1
Y3QgcGxhdGZvcm1fZGV2aWNlICpwZGV2KQo+PiAgCWlmIChyZXQpCj4+ICAJCXJldHVybiByZXQ7
Cj4+ICAKPj4gKwlpZiAoSVNfRVJSKHBjaWUtPmNsa19yZWcpICYmIFBUUl9FUlIocGNpZS0+Y2xr
X3JlZykgPT0gLUVQUk9CRV9ERUZFUikgewo+Cj4gWW91IGRvIHJlYWxpc2UgdGhpcyBpcyBuZWVk
bGVzc2x5IGNvbXBsZXguCj4KPiBQb2ludGVyIGVycm9ycyBhcmUgdW5pcXVlLCBzbzoKPgo+IAlp
ZiAocGNpZS0+Y2xrX3JlZyA9PSBFUlJfUFRSKC1FUFJPQkVfREVGRVIpKSB7Cj4KPiB3aWxsIGRv
IHRoZSBzYW1lIHRoaW5nIGJ1dCB3aXRob3V0IHRoZSBjb21wbGV4aXR5LiAgVHJhbnNmb3JtaW5n
IHRoZQo+IGNvbnN0YW50IHJhdGhlciB0aGFuIHRoZSB2YXJpYWJsZSBpcyBhbHNvIGEgZ29vZCBo
YWJiaXQgdG8gZ2V0IGludG8gLQo+IHRoZSBjb21waWxlciBjYW4gb3B0aW1pc2UgdHJhbnNmb3Jt
cyB0byBjb25zdGFudHMsIGJ1dCBjYW4ndCB3aXRoCj4gdmFyaWFibGVzLCBzbyBjb21wYXJpc29u
cyBpbnZvbHZpbmcgdGhpbmdzIGxpa2UgZW5kaWFuIGNvbnZlcnNpb24KPiBzaG91bGQgYWx3YXlz
IGJlIGRvbmUgYnkgdHJhbnNmb3JtaW5nIHRoZSBjb25zdGFudCBub3QgdGhlIHZhcmlhYmxlLgoK
VGhhbmtzIGZvciB0aGUgdGlwLCBJIHdpbGwgdXNlIGl0IGluIHRoZSBuZXh0IHZlcnNpb24uCgpH
cmVnb3J5Cgo+Cj4gLS0gCj4gUk1LJ3MgUGF0Y2ggc3lzdGVtOiBodHRwOi8vd3d3LmFybWxpbnV4
Lm9yZy51ay9kZXZlbG9wZXIvcGF0Y2hlcy8KPiBGVFRDIGJyb2FkYmFuZCBmb3IgMC44bWlsZSBs
aW5lIGluIHN1YnVyYmlhOiBzeW5jIGF0IDguOE1icHMgZG93biA2MzBrYnBzIHVwCj4gQWNjb3Jk
aW5nIHRvIHNwZWVkdGVzdC5uZXQ6IDguMjFNYnBzIGRvd24gNTEwa2JwcyB1cAoKLS0gCkdyZWdv
cnkgQ2xlbWVudCwgQm9vdGxpbiAoZm9ybWVybHkgRnJlZSBFbGVjdHJvbnMpCkVtYmVkZGVkIExp
bnV4IGFuZCBLZXJuZWwgZW5naW5lZXJpbmcKaHR0cDovL2Jvb3RsaW4uY29tCgpfX19fX19fX19f
X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0ta2VybmVsIG1h
aWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcKaHR0cDovL2xp
c3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2VybmVsCg==

[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