mtd: nand: marvell: Fix clock resource by adding a register clock

PATCH New Gregory CLEMENT 3 participants 7 messages Feb 28, 2018 – Mar 7, 2018

Subsystems (3)

Device tree bindings Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Gregory CLEMENT

author

2018-02-28 14:35 UTC #0 lore raw

PATCH 1/1 mtd: nand: marvell: Fix clock resource by adding a register clock

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 <[email protected]>

---
 Documentation/devicetree/bindings/mtd/marvell-nand.txt |  6 +++++-
 drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/mtd/marvell-nand.txt +5 -1

index c08fb477b3c6..4ee9813bf88f 100644
@@ -14,7 +14,11 @@ Required properties:
 - #address-cells: shall be set to 1. Encode the NAND CS.
 - #size-cells: shall be set to 0.
 - interrupts: shall define the NAND controller interrupt.
-- clocks: shall reference the NAND controller clock.
+- clocks: shall reference the NAND controller clocks, the second one is
+  optional but needed for the Armada 7K/8K SoCs
+- 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
 - marvell,system-controller: Set to retrieve the syscon node that handles
   NAND controller related registers (only required with the
   "marvell,armada-8k-nand[-controller]" compatibles).

drivers/mtd/nand/marvell_nand.c +14 -0

index 2196f2a233d6..be874c636b5f 100644
@@ -321,6 +321,7 @@ struct marvell_nfc {
 	struct device *dev;
 	void __iomem *regs;
 	struct clk *ecc_clk;
+	struct clk *reg_clk;
 	struct completion complete;
 	unsigned long assigned_cs;
 	struct list_head chips;

@@ -2747,6 +2748,17 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");
+	if (IS_ERR(nfc->reg_clk) && PTR_ERR(nfc->reg_clk) == -EPROBE_DEFER) {
+		clk_disable_unprepare(nfc->ecc_clk);
+		return -EPROBE_DEFER;
+	}
+	if (!IS_ERR(nfc->reg_clk)) {
+		ret = clk_prepare_enable(nfc->reg_clk);
+		if (ret)
+			goto unprepare_clk;
+	}
+
 	marvell_nfc_disable_int(nfc, NDCR_ALL_INT);
 	marvell_nfc_clear_int(nfc, NDCR_ALL_INT);
 	ret = devm_request_irq(dev, irq, marvell_nfc_isr,

@@ -2780,6 +2792,7 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	return 0;
 
 unprepare_clk:
+	clk_disable_unprepare(nfc->reg_clk);
 	clk_disable_unprepare(nfc->ecc_clk);
 
 	return ret;

@@ -2797,6 +2810,7 @@ static int marvell_nfc_remove(struct platform_device *pdev)
 	}
 
 	clk_disable_unprepare(nfc->ecc_clk);
+	clk_disable_unprepare(nfc->reg_clk);
 
 	return 0;
 }

Signature · 1 line

2.16.1

Boris Brezillon

2018-03-01 17:35 UTC #1 lore raw

PATCH 1/1 mtd: nand: marvell: Fix clock resource by adding a register clock

Hi Greg,

Quoted 17 lines · On Wed, 28 Feb 2018 15:35:53 +0100 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 <[email protected]>
---
 Documentation/devicetree/bindings/mtd/marvell-nand.txt |  6 +++++-
 drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/mtd/marvell-nand.txt +0 -0
index c08fb477b3c6..4ee9813bf88f 100644

Documentation/devicetree/bindings/mtd/marvell-nand.txt Required properties: · line 21

Quoted 9 lines

Documentation/devicetree/bindings/mtd/marvell-nand.txt +5 -1

@@ -14,7 +14,11 @@ Required properties:
 - #address-cells: shall be set to 1. Encode the NAND CS.
 - #size-cells: shall be set to 0.
 - interrupts: shall define the NAND controller interrupt.
-- clocks: shall reference the NAND controller clock.
+- clocks: shall reference the NAND controller clocks, the second one is
+  optional but needed for the Armada 7K/8K SoCs
+- 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

Hm, not sure this is a good idea to impose a specific order. I know you do that to avoid changing the code requesting the core clk, but I'd prefer to have a solution where we first search for a clock named "core" (devm_clk_get(&pdev->dev, "core")), and if it's missing, fall back to devm_clk_get(&pdev->dev, NULL).

Quoted 16 lines

Documentation/devicetree/bindings/mtd/marvell-nand.txt +0 -0

 - marvell,system-controller: Set to retrieve the syscon node that handles
   NAND controller related registers (only required with the
   "marvell,armada-8k-nand[-controller]" compatibles).

drivers/mtd/nand/marvell_nand.c +1 -0

index 2196f2a233d6..be874c636b5f 100644
@@ -321,6 +321,7 @@ struct marvell_nfc {
 	struct device *dev;
 	void __iomem *regs;
 	struct clk *ecc_clk;
+	struct clk *reg_clk;
 	struct completion complete;
 	unsigned long assigned_cs;
 	struct list_head chips;

drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2751

Quoted 4 lines

drivers/mtd/nand/marvell_nand.c +1 -0

@@ -2747,6 +2748,17 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");

Can we move that before the "core" clock (which for some unknown reason is called ecc_clk in the driver) is prepared, so that you don't have to call clk_disable_unprepare() here.

drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2755

Quoted 4 lines

drivers/mtd/nand/marvell_nand.c +4 -0

+	if (IS_ERR(nfc->reg_clk) && PTR_ERR(nfc->reg_clk) == -EPROBE_DEFER) {
+		clk_disable_unprepare(nfc->ecc_clk);
+		return -EPROBE_DEFER;
+	}

Why not:

	if (IS_ERR(nfc->reg_clk) && PTR_ERR(nfc->reg_clk) != -ENOENT)
		return PTR_ERR(nfc->reg_clk);

AFAIR, if the clk is not defined, ENOENT is returned, and you want to propagate all error codes, not only EPROBE_DEFER otherwise.

Another solution would be to retrieve the reg clk only on platforms that need it (based on the compatible). This way you won't have to test for -ENOENT and could simply propagate the error to the upper layer.

drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2759

Quoted 4 lines

drivers/mtd/nand/marvell_nand.c +4 -0

+	if (!IS_ERR(nfc->reg_clk)) {
+		ret = clk_prepare_enable(nfc->reg_clk);
+		if (ret)
+			goto unprepare_clk;

This is wrong: you've put a clk_disable_unprepare(nfc->reg_clk) call in the unprepare_clk path, which means you'll disable/unprepare a clk that has not been successfully prepared/enabled => unbalanced refcounting.

Please define a new label and rename the old one.

Quoted 14 lines

drivers/mtd/nand/marvell_nand.c +3 -0

+	}
+
 	marvell_nfc_disable_int(nfc, NDCR_ALL_INT);
 	marvell_nfc_clear_int(nfc, NDCR_ALL_INT);
 	ret = devm_request_irq(dev, irq, marvell_nfc_isr,

@@ -2780,6 +2792,7 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	return 0;
 
 unprepare_clk:
+	clk_disable_unprepare(nfc->reg_clk);
 	clk_disable_unprepare(nfc->ecc_clk);
 
 	return ret;

drivers/mtd/nand/marvell_nand.c static int marvell_nfc_remove(struct platform_device *pdev) · line 2816

Quoted 7 lines

drivers/mtd/nand/marvell_nand.c +1 -0

@@ -2797,6 +2810,7 @@ static int marvell_nfc_remove(struct platform_device *pdev)
 	}
 
 	clk_disable_unprepare(nfc->ecc_clk);
+	clk_disable_unprepare(nfc->reg_clk);
 
 	return 0;
 }

Regards,

Boris

Signature · 3 lines

Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Gregory CLEMENT

author

2018-03-06 11:04 UTC #2 lore raw

PATCH 1/1 mtd: nand: marvell: Fix clock resource by adding a register clock

Hi Boris,

Quoted 17 lines · Boris Brezillon

Hi Greg,
Quoted 12 lines · On Wed, 28 Feb 2018 15:35:53 +0100 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 <[email protected]>
---
 Documentation/devicetree/bindings/mtd/marvell-nand.txt |  6 +++++-
 drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

Quoted 16 lines

Quoted 5 lines
Documentation/devicetree/bindings/mtd/marvell-nand.txt +0 -0
index c08fb477b3c6..4ee9813bf88f 100644
Documentation/devicetree/bindings/mtd/marvell-nand.txt Required properties: · line 21
Quoted 9 lines
Documentation/devicetree/bindings/mtd/marvell-nand.txt +5 -1
@@ -14,7 +14,11 @@ Required properties:
 - #address-cells: shall be set to 1. Encode the NAND CS.
 - #size-cells: shall be set to 0.
 - interrupts: shall define the NAND controller interrupt.
-- clocks: shall reference the NAND controller clock.
+- clocks: shall reference the NAND controller clocks, the second one is
+  optional but needed for the Armada 7K/8K SoCs
+- 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
Hm, not sure this is a good idea to impose a specific order. I know you

It would be a problem if the use of this clock would be at dts level for each board. But here we only setup this property in the dtsi at SoC level. So enforcing the order is not a problem as the dtsi are all well reviewed and remains pretty rare.

Quoted 4 lines

do that to avoid changing the code requesting the core clk, but I'd prefer to have a solution where we first search for a clock named "core" (devm_clk_get(&pdev->dev, "core")), and if it's missing, fall back to devm_clk_get(&pdev->dev, NULL).

I really wanted to avoid adding more code only for legacy reason.

Quoted 33 lines

Quoted 17 lines
Documentation/devicetree/bindings/mtd/marvell-nand.txt +0 -0
 - marvell,system-controller: Set to retrieve the syscon node that handles
   NAND controller related registers (only required with the
   "marvell,armada-8k-nand[-controller]" compatibles).
drivers/mtd/nand/marvell_nand.c +1 -0
index 2196f2a233d6..be874c636b5f 100644
@@ -321,6 +321,7 @@ struct marvell_nfc {
 	struct device *dev;
 	void __iomem *regs;
 	struct clk *ecc_clk;
+	struct clk *reg_clk;
 	struct completion complete;
 	unsigned long assigned_cs;
 	struct list_head chips;
drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2751
Quoted 4 lines
drivers/mtd/nand/marvell_nand.c +1 -0
@@ -2747,6 +2748,17 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");
Can we move that before the "core" clock (which for some unknown reason is called ecc_clk in the driver) is prepared, so that you don't have to call clk_disable_unprepare() here.
drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2755
Quoted 4 lines
drivers/mtd/nand/marvell_nand.c +4 -0
+	if (IS_ERR(nfc->reg_clk) && PTR_ERR(nfc->reg_clk) == -EPROBE_DEFER) {
+		clk_disable_unprepare(nfc->ecc_clk);
+		return -EPROBE_DEFER;
+	}
Why not:

Quoted 7 lines

	if (IS_ERR(nfc->reg_clk) && PTR_ERR(nfc->reg_clk) != -ENOENT)
		return PTR_ERR(nfc->reg_clk);
?
AFAIR, if the clk is not defined, ENOENT is returned, and you want to propagate all error codes, not only EPROBE_DEFER otherwise.

Indeed it makes sens.

Quoted 4 lines

Another solution would be to retrieve the reg clk only on platforms that need it (based on the compatible). This way you won't have to test for -ENOENT and could simply propagate the error to the upper layer.

We can't do this because we need to be bacwkard compatible.

Quoted 11 lines

drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2759
Quoted 5 lines
drivers/mtd/nand/marvell_nand.c +4 -0
+	if (!IS_ERR(nfc->reg_clk)) {
+		ret = clk_prepare_enable(nfc->reg_clk);
+		if (ret)
+			goto unprepare_clk;
This is wrong: you've put a clk_disable_unprepare(nfc->reg_clk) call in the unprepare_clk path, which means you'll disable/unprepare a clk that has not been successfully prepared/enabled => unbalanced refcounting.
Please define a new label and rename the old one.

Thanks,

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

Boris Brezillon

2018-03-06 12:43 UTC #3 lore raw

PATCH 1/1 mtd: nand: marvell: Fix clock resource by adding a register clock

Hi Greg,

Quoted 38 lines · On Tue, 06 Mar 2018 12:04:41 +0100 Gregory CLEMENT…

Hi Boris,
Quoted 17 lines · Boris Brezillon
Hi Greg,
Quoted 12 lines · On Wed, 28 Feb 2018 15:35:53 +0100 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 <[email protected]>
---
 Documentation/devicetree/bindings/mtd/marvell-nand.txt |  6 +++++-
 drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)
Quoted 16 lines
Quoted 5 lines
Documentation/devicetree/bindings/mtd/marvell-nand.txt +0 -0
index c08fb477b3c6..4ee9813bf88f 100644
Documentation/devicetree/bindings/mtd/marvell-nand.txt Required properties: · line 21
Quoted 9 lines
Documentation/devicetree/bindings/mtd/marvell-nand.txt +5 -1
@@ -14,7 +14,11 @@ Required properties:
 - #address-cells: shall be set to 1. Encode the NAND CS.
 - #size-cells: shall be set to 0.
 - interrupts: shall define the NAND controller interrupt.
-- clocks: shall reference the NAND controller clock.
+- clocks: shall reference the NAND controller clocks, the second one is
+  optional but needed for the Armada 7K/8K SoCs
+- 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  
Hm, not sure this is a good idea to impose a specific order. I know you

Quoted 4 lines

It would be a problem if the use of this clock would be at dts level for each board. But here we only setup this property in the dtsi at SoC level. So enforcing the order is not a problem as the dtsi are all well reviewed and remains pretty rare.

I'm still not convinced this is a good idea, but okay.

Quoted 7 lines

Quoted 5 lines
do that to avoid changing the code requesting the core clk, but I'd prefer to have a solution where we first search for a clock named "core" (devm_clk_get(&pdev->dev, "core")), and if it's missing, fall back to devm_clk_get(&pdev->dev, NULL).
I really wanted to avoid adding more code only for legacy reason.

It's adding only 2 lines:

	reg_clk = devm_clk_get(&pdev->dev, "core");
	if (IS_ERR(reg_clk) && PTR_ERR(reg_clk) == -ENOENT)
		reg_clk = devm_clk_get(&pdev->dev, NULL);

Quoted 7 lines

Quoted 5 lines
Another solution would be to retrieve the reg clk only on platforms that need it (based on the compatible). This way you won't have to test for -ENOENT and could simply propagate the error to the upper layer.
We can't do this because we need to be bacwkard compatible.

I don't get it. If the clock is missing in the DT, the NAND is already not working on these platforms, right? How could we break something that is already broken?

Regards,

Boris

Signature · 3 lines

Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Russell King - ARM Linux

2018-03-06 13:08 UTC #4 lore raw

PATCH 1/1 mtd: nand: marvell: Fix clock resource by adding a register clock

Quoted 56 lines · Boris Brezillon

Hi Greg,
Quoted 38 lines · On Tue, 06 Mar 2018 12:04:41 +0100 Gregory CLEMENT…
Hi Boris,
Quoted 17 lines · Boris Brezillon
Hi Greg,
Quoted 12 lines · On Wed, 28 Feb 2018 15:35:53 +0100 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 <[email protected]>
---
 Documentation/devicetree/bindings/mtd/marvell-nand.txt |  6 +++++-
 drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)
Quoted 16 lines
Quoted 5 lines
Documentation/devicetree/bindings/mtd/marvell-nand.txt +0 -0
index c08fb477b3c6..4ee9813bf88f 100644
Documentation/devicetree/bindings/mtd/marvell-nand.txt Required properties: · line 21
Quoted 9 lines
Documentation/devicetree/bindings/mtd/marvell-nand.txt +5 -1
@@ -14,7 +14,11 @@ Required properties:
 - #address-cells: shall be set to 1. Encode the NAND CS.
 - #size-cells: shall be set to 0.
 - interrupts: shall define the NAND controller interrupt.
-- clocks: shall reference the NAND controller clock.
+- clocks: shall reference the NAND controller clocks, the second one is
+  optional but needed for the Armada 7K/8K SoCs
+- 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  
Hm, not sure this is a good idea to impose a specific order. I know you
Quoted 4 lines
It would be a problem if the use of this clock would be at dts level for each board. But here we only setup this property in the dtsi at SoC level. So enforcing the order is not a problem as the dtsi are all well reviewed and remains pretty rare.
I'm still not convinced this is a good idea, but okay.
Quoted 6 lines
Quoted 5 lines
do that to avoid changing the code requesting the core clk, but I'd prefer to have a solution where we first search for a clock named "core" (devm_clk_get(&pdev->dev, "core")), and if it's missing, fall back to devm_clk_get(&pdev->dev, NULL).

Quoted 7 lines

Quoted 1 line
I really wanted to avoid adding more code only for legacy reason.
It's adding only 2 lines:
	reg_clk = devm_clk_get(&pdev->dev, "core");
	if (IS_ERR(reg_clk) && PTR_ERR(reg_clk) == -ENOENT)
		reg_clk = devm_clk_get(&pdev->dev, NULL);

	reg_clk = devm_clk_get(&pdev->dev, "core");
	if (reg_clk == ERR_PTR(-ENOENT))
		reg_clk = devm_clk_get(&pdev->dev, NULL);

is simpler and easier to read?

Quoted 26 lines

Quoted 8 lines
Quoted 6 lines
Another solution would be to retrieve the reg clk only on platforms that need it (based on the compatible). This way you won't have to test for -ENOENT and could simply propagate the error to the upper layer.
We can't do this because we need to be bacwkard compatible.
I don't get it. If the clock is missing in the DT, the NAND is already not working on these platforms, right? How could we break something that is already broken?
Regards,
Boris
Signature · 8 lines
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 3 lines

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

Boris Brezillon

2018-03-06 13:10 UTC #5 lore raw

PATCH 1/1 mtd: nand: marvell: Fix clock resource by adding a register clock

Quoted 65 lines · On Tue, 6 Mar 2018 13:08:33 +0000 Russell King - ARM Linux…

Quoted 56 lines · Boris Brezillon
Hi Greg,
Quoted 38 lines · On Tue, 06 Mar 2018 12:04:41 +0100 Gregory CLEMENT…
Hi Boris,
Quoted 17 lines · Boris Brezillon
Hi Greg,
Quoted 12 lines · On Wed, 28 Feb 2018 15:35:53 +0100 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 <[email protected]>
---
 Documentation/devicetree/bindings/mtd/marvell-nand.txt |  6 +++++-
 drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)
Quoted 16 lines
Quoted 5 lines
Documentation/devicetree/bindings/mtd/marvell-nand.txt +0 -0
index c08fb477b3c6..4ee9813bf88f 100644
Documentation/devicetree/bindings/mtd/marvell-nand.txt Required properties: · line 21
Quoted 9 lines
Documentation/devicetree/bindings/mtd/marvell-nand.txt +5 -1
@@ -14,7 +14,11 @@ Required properties:
 - #address-cells: shall be set to 1. Encode the NAND CS.
 - #size-cells: shall be set to 0.
 - interrupts: shall define the NAND controller interrupt.
-- clocks: shall reference the NAND controller clock.
+- clocks: shall reference the NAND controller clocks, the second one is
+  optional but needed for the Armada 7K/8K SoCs
+- 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    
Hm, not sure this is a good idea to impose a specific order. I know you
Quoted 4 lines
It would be a problem if the use of this clock would be at dts level for each board. But here we only setup this property in the dtsi at SoC level. So enforcing the order is not a problem as the dtsi are all well reviewed and remains pretty rare.
I'm still not convinced this is a good idea, but okay.
Quoted 6 lines
Quoted 5 lines
do that to avoid changing the code requesting the core clk, but I'd prefer to have a solution where we first search for a clock named "core" (devm_clk_get(&pdev->dev, "core")), and if it's missing, fall back to devm_clk_get(&pdev->dev, NULL).
Quoted 7 lines
Quoted 1 line
I really wanted to avoid adding more code only for legacy reason.
It's adding only 2 lines:
	reg_clk = devm_clk_get(&pdev->dev, "core");
	if (IS_ERR(reg_clk) && PTR_ERR(reg_clk) == -ENOENT)
		reg_clk = devm_clk_get(&pdev->dev, NULL);  

Quoted 5 lines

	reg_clk = devm_clk_get(&pdev->dev, "core");
	if (reg_clk == ERR_PTR(-ENOENT))
		reg_clk = devm_clk_get(&pdev->dev, NULL);

is simpler and easier to read?

Indeed, no need to check IS_ERR(reg_clk) first.

Signature · 3 lines

Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Gregory CLEMENT

author

2018-03-07 14:23 UTC #6 lore raw

PATCH 1/1 mtd: nand: marvell: Fix clock resource by adding a register clock

In reply to Boris Brezillon

Hi Boris,

Quoted 10 lines · Boris Brezillon

Quoted 6 lines
Quoted 4 lines
Another solution would be to retrieve the reg clk only on platforms that need it (based on the compatible). This way you won't have to test for -ENOENT and could simply propagate the error to the upper layer.
We can't do this because we need to be bacwkard compatible.
I don't get it. If the clock is missing in the DT, the NAND is already not working on these platforms, right? How could we break something that is already broken?

As you probably know this patch have been tested on an Armada 7K based board, so it more or less works.

But it works because the second clock used by the nand controller is also used by other IP and already setup by the drivers of these IP. So today it is only if you boot a kernel without crypto, i2c, spi and uart support that the nand won't work.

Then the current representation is wrong, but it is still usable in most of the case. So that's why we still need to support the case where we have only one clock in the device tree.

Gregory

Quoted 9 lines

Regards,

Boris

Signature · 3 lines

Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Signature · 3 lines

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