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

PATCH Superseded Gregory CLEMENT 2 participants 5 messages Mar 7, 2018 – Mar 13, 2018

Subsystems (3)

Device tree bindings Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Gregory CLEMENT

author

2018-03-07 16:13 UTC #0 lore raw

PATCH v2 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]>

---
Changelog:
v1-> v2

- Removed the unnecessary IS_ERR() call
- Skip the reg clock only if it is not present by checking "-ENOENT"
- Add a label for uninitializing the reg clock.

 .../devicetree/bindings/mtd/marvell-nand.txt       |  6 +++++-
 drivers/mtd/nand/marvell_nand.c                    | 24 ++++++++++++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)

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 +20 -4

index 2196f2a233d6..072e23635375 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,12 +2748,24 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");
+	if (PTR_ERR(nfc->reg_clk) != -ENOENT) {
+		if (!IS_ERR(nfc->reg_clk)) {
+			ret = clk_prepare_enable(nfc->reg_clk);
+			if (ret)
+				goto unprepare_clk;
+		} else {
+			ret = PTR_ERR(nfc->reg_clk);
+			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,
 			       0, "marvell-nfc", nfc);
 	if (ret)
-		goto unprepare_clk;
+		goto unprepare_clk_reg;
 
 	/* Get NAND controller capabilities */
 	if (pdev->id_entry)

@@ -2763,22 +2776,24 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	if (!nfc->caps) {
 		dev_err(dev, "Could not retrieve NFC caps\n");
 		ret = -EINVAL;
-		goto unprepare_clk;
+		goto unprepare_clk_reg;
 	}
 
 	/* Init the controller and then probe the chips */
 	ret = marvell_nfc_init(nfc);
 	if (ret)
-		goto unprepare_clk;
+		goto unprepare_clk_reg;
 
 	platform_set_drvdata(pdev, nfc);
 
 	ret = marvell_nand_chips_init(dev, nfc);
 	if (ret)
-		goto unprepare_clk;
+		goto unprepare_clk_reg;
 
 	return 0;
 
+unprepare_clk_reg:
+	clk_disable_unprepare(nfc->reg_clk);
 unprepare_clk:
 	clk_disable_unprepare(nfc->ecc_clk);

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

Signature · 1 line

2.16.1

Boris Brezillon

2018-03-07 19:44 UTC #1 lore raw

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

+Rob and the DT ML for the DT bindings changes.

Quoted 24 lines · On Wed, 7 Mar 2018 17:13:16 +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]>
---
Changelog:
v1-> v2
- Removed the unnecessary IS_ERR() call
- Skip the reg clock only if it is not present by checking "-ENOENT"
- Add a label for uninitializing the reg clock.
 .../devicetree/bindings/mtd/marvell-nand.txt       |  6 +++++-
 drivers/mtd/nand/marvell_nand.c                    | 24 ++++++++++++++++++----
 2 files changed, 25 insertions(+), 5 deletions(-)
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

Still think we should not enforce the order, and I proposed a simple solution for that in my previous review:

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

	if (IS_ERR(nfc->ecc_clk))
		return PTR_ERR(nfc->ecc_clk);

drivers/mtd/nand/marvell_nand.c struct marvell_nfc { · line 324

Quoted 12 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..072e23635375 100644
@@ -321,6 +321,7 @@ struct marvell_nfc {
 	struct device *dev;
 	void __iomem *regs;
 	struct clk *ecc_clk;
+	struct clk *reg_clk;

There's a kernel-doc header describing the marvell_nfc fields, could you add an entry for reg_clk?

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

Quoted 13 lines

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

 	struct completion complete;
 	unsigned long assigned_cs;
 	struct list_head chips;

@@ -2747,12 +2748,24 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");
+	if (PTR_ERR(nfc->reg_clk) != -ENOENT) {
+		if (!IS_ERR(nfc->reg_clk)) {
+			ret = clk_prepare_enable(nfc->reg_clk);
+			if (ret)
+				goto unprepare_clk;

I already suggested to move the devm_clk_get(&pdev->dev, "reg") before the clk_prepare_enable(nfc->ecc_clk) one to simplify the error path.

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

Quoted 5 lines

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

+		} else {
+			ret = PTR_ERR(nfc->reg_clk);
+			goto unprepare_clk;
+		}
+	}

So nfc->reg_clk stays assigned to -ENOENT if the clk is not present, and clk_disable_unprepare() will manipulate an invalid pointer when called from the error or ->remove() path.

Could be addressed/simplified with something like that:

	/*
	 * The register clk is only required on armada 8k. By assigning
	 * ->reg_clk to NULL when -ENOENT is returned, we make sure all
	 * clk_prepare_enable()/clk_disable_unprepare() calls work
	 * correctly even if the clk is missing.
	 */
	nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");
	if (PTR_ERR(nfc->reg_clk) == -ENOENT)
		nfc->reg_clk = NULL;

	if (IS_ERR(nfc->reg_clk))
		return PTR_ERR(nfc->reg_clk);

	...

	ret = clk_prepare_enable(nfc->reg_clk);
	if (ret)
		goto unprepare_ecc_clk;

Quoted 27 lines

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

+
 	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,
 			       0, "marvell-nfc", nfc);
 	if (ret)
-		goto unprepare_clk;
+		goto unprepare_clk_reg;
 
 	/* Get NAND controller capabilities */
 	if (pdev->id_entry)

@@ -2763,22 +2776,24 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	if (!nfc->caps) {
 		dev_err(dev, "Could not retrieve NFC caps\n");
 		ret = -EINVAL;
-		goto unprepare_clk;
+		goto unprepare_clk_reg;
 	}
 
 	/* Init the controller and then probe the chips */
 	ret = marvell_nfc_init(nfc);
 	if (ret)
-		goto unprepare_clk;
+		goto unprepare_clk_reg;
 
 	platform_set_drvdata(pdev, nfc);

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

Quoted 10 lines

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

 	ret = marvell_nand_chips_init(dev, nfc);
 	if (ret)
-		goto unprepare_clk;
+		goto unprepare_clk_reg;
 
 	return 0;
 
+unprepare_clk_reg:
+	clk_disable_unprepare(nfc->reg_clk);
 unprepare_clk:

Please rename the label (unprepare_clk -> unprepare_ecc_clk).

Quoted 10 lines

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

 	clk_disable_unprepare(nfc->ecc_clk);

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

Signature · 3 lines

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

Gregory CLEMENT

author

2018-03-12 16:55 UTC #2 lore raw

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

Hi Boris,

Quoted 6 lines · Boris Brezillon

Quoted 6 lines
NAND controller related registers (only required with the "marvell,armada-8k-nand[-controller]" compatibles).
drivers/mtd/nand/marvell_nand.c +0 -0
index 2196f2a233d6..072e23635375 100644

Quoted 8 lines

drivers/mtd/nand/marvell_nand.c struct marvell_nfc { · line 324
Quoted 5 lines
drivers/mtd/nand/marvell_nand.c +1 -0
@@ -321,6 +321,7 @@ struct marvell_nfc {
 	struct device *dev;
 	void __iomem *regs;
 	struct clk *ecc_clk;
+	struct clk *reg_clk;
There's a kernel-doc header describing the marvell_nfc fields, could you add an entry for reg_clk?

OK I will

Quoted 8 lines

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

 	struct completion complete;
 	unsigned long assigned_cs;
 	struct list_head chips;

@@ -2747,12 +2748,24 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;

Quoted 10 lines

drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2756
Quoted 6 lines
drivers/mtd/nand/marvell_nand.c +6 -0
+	nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");
+	if (PTR_ERR(nfc->reg_clk) != -ENOENT) {
+		if (!IS_ERR(nfc->reg_clk)) {
+			ret = clk_prepare_enable(nfc->reg_clk);
+			if (ret)
+				goto unprepare_clk;
I already suggested to move the devm_clk_get(&pdev->dev, "reg") before the clk_prepare_enable(nfc->ecc_clk) one to simplify the error path.

Actually I started to implement your suggestion but unlike what you though it made the code less simpler. Indeed by having the mandatory clock first than in case of failure we can directly exit the function.

If the reg clock was initialized first, then if the core/ecc clock fail in soem case we woudl need to daisbel the reg clock and in other case we could directly exit.

Quoted 9 lines

drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2761
Quoted 5 lines
drivers/mtd/nand/marvell_nand.c +5 -0
+		} else {
+			ret = PTR_ERR(nfc->reg_clk);
+			goto unprepare_clk;
+		}
+	}
So nfc->reg_clk stays assigned to -ENOENT if the clk is not present, and clk_disable_unprepare() will manipulate an invalid pointer when called from the error or ->remove() path.

I think you missed the fact that the clk_disable_unprepare() function managed the invalid pointer, have a look on the functions and you will see that IS_ERR() is used, so there is no point to set the pointer to NULL.

Quoted 55 lines

Could be addressed/simplified with something like that:

	/*
	 * The register clk is only required on armada 8k. By assigning
	 * ->reg_clk to NULL when -ENOENT is returned, we make sure all
	 * clk_prepare_enable()/clk_disable_unprepare() calls work
	 * correctly even if the clk is missing.
	 */
	nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");
	if (PTR_ERR(nfc->reg_clk) == -ENOENT)
		nfc->reg_clk = NULL;

	if (IS_ERR(nfc->reg_clk))
		return PTR_ERR(nfc->reg_clk);

	...

	ret = clk_prepare_enable(nfc->reg_clk);
	if (ret)
		goto unprepare_ecc_clk;

Quoted 32 lines

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

+
 	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,
 			       0, "marvell-nfc", nfc);
 	if (ret)
-		goto unprepare_clk;
+		goto unprepare_clk_reg;
 
 	/* Get NAND controller capabilities */
 	if (pdev->id_entry)

@@ -2763,22 +2776,24 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	if (!nfc->caps) {
 		dev_err(dev, "Could not retrieve NFC caps\n");
 		ret = -EINVAL;
-		goto unprepare_clk;
+		goto unprepare_clk_reg;
 	}
 
 	/* Init the controller and then probe the chips */
 	ret = marvell_nfc_init(nfc);
 	if (ret)
-		goto unprepare_clk;
+		goto unprepare_clk_reg;
 
 	platform_set_drvdata(pdev, nfc);
 
 	ret = marvell_nand_chips_init(dev, nfc);
 	if (ret)
-		goto unprepare_clk;
+		goto unprepare_clk_reg;

Quoted 7 lines

drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2797
Quoted 5 lines
drivers/mtd/nand/marvell_nand.c +2 -0
 	return 0;
 
+unprepare_clk_reg:
+	clk_disable_unprepare(nfc->reg_clk);
 unprepare_clk:
Please rename the label (unprepare_clk -> unprepare_ecc_clk).

Quoted 18 lines

Quoted 11 lines

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

 	clk_disable_unprepare(nfc->ecc_clk);

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

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

Boris Brezillon

2018-03-12 19:35 UTC #3 lore raw

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

Quoted 19 lines · On Mon, 12 Mar 2018 17:55:26 +0100 Gregory CLEMENT…

Quoted 8 lines
Quoted 8 lines
drivers/mtd/nand/marvell_nand.c +0 -0
 	struct completion complete;
 	unsigned long assigned_cs;
 	struct list_head chips;
@@ -2747,12 +2748,24 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
Quoted 10 lines
drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2756
Quoted 6 lines
drivers/mtd/nand/marvell_nand.c +6 -0
+	nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");
+	if (PTR_ERR(nfc->reg_clk) != -ENOENT) {
+		if (!IS_ERR(nfc->reg_clk)) {
+			ret = clk_prepare_enable(nfc->reg_clk);
+			if (ret)
+				goto unprepare_clk;  
I already suggested to move the devm_clk_get(&pdev->dev, "reg") before the clk_prepare_enable(nfc->ecc_clk) one to simplify the error path.

Quoted 7 lines

Actually I started to implement your suggestion but unlike what you though it made the code less simpler. Indeed by having the mandatory clock first than in case of failure we can directly exit the function.
If the reg clock was initialized first, then if the core/ecc clock fail in soem case we woudl need to daisbel the reg clock and in other case we could directly exit.

Well, it's pretty much the same problem if you do it in the order you propose here: if the core clk enable fails, you'll have to disable the reg clk. Plus, I'm not a big fan of if/else block imbrications when we can avoid them.

Quoted 12 lines

Quoted 11 lines
drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2761
Quoted 7 lines
drivers/mtd/nand/marvell_nand.c +5 -0
+		} else {
+			ret = PTR_ERR(nfc->reg_clk);
+			goto unprepare_clk;
+		}
+	}  
So nfc->reg_clk stays assigned to -ENOENT if the clk is not present, and clk_disable_unprepare() will manipulate an invalid pointer when called from the error or ->remove() path.

Quoted 3 lines

I think you missed the fact that the clk_disable_unprepare() function managed the invalid pointer, have a look on the functions and you will see that IS_ERR() is used, so there is no point to set the pointer to NULL.

Right. I just checked the clk_prepare() implementation which is checking for NULL value only and I thought clk_disable() and clk_unprepare() were doing the same, which apparently is not the case.

Signature · 3 lines

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

Gregory CLEMENT

author

2018-03-13 10:29 UTC #4 lore raw

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

Hi Boris,

Quoted 26 lines · Boris Brezillon

Quoted 23 lines · On Mon, 12 Mar 2018 17:55:26 +0100 Gregory CLEMENT…
Quoted 8 lines
Quoted 8 lines
drivers/mtd/nand/marvell_nand.c +0 -0
 	struct completion complete;
 	unsigned long assigned_cs;
 	struct list_head chips;
@@ -2747,12 +2748,24 @@ static int marvell_nfc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
Quoted 10 lines
drivers/mtd/nand/marvell_nand.c static int marvell_nfc_probe(struct platform_device *pdev) · line 2756
Quoted 6 lines
drivers/mtd/nand/marvell_nand.c +6 -0
+	nfc->reg_clk = devm_clk_get(&pdev->dev, "reg");
+	if (PTR_ERR(nfc->reg_clk) != -ENOENT) {
+		if (!IS_ERR(nfc->reg_clk)) {
+			ret = clk_prepare_enable(nfc->reg_clk);
+			if (ret)
+				goto unprepare_clk;  
I already suggested to move the devm_clk_get(&pdev->dev, "reg") before the clk_prepare_enable(nfc->ecc_clk) one to simplify the error path.
Actually I started to implement your suggestion but unlike what you though it made the code less simpler. Indeed by having the mandatory clock first than in case of failure we can directly exit the function.

Quoted 6 lines

Quoted 3 lines
If the reg clock was initialized first, then if the core/ecc clock fail in soem case we woudl need to daisbel the reg clock and in other case we could directly exit.
Well, it's pretty much the same problem if you do it in the order you propose here: if the core clk enable fails, you'll have to disable the

So if it is the same no need to change! :)

Quoted 2 lines

reg clk. Plus, I'm not a big fan of if/else block imbrications when we can avoid them.

Your solution to avoid if/else block is to add extra code and extra test which do not bring anything except removing an if/else bloc.

For the record it was

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

--> here you set to NULL whereas it is useless

	if (IS_ERR(nfc->reg_clk))
		return PTR_ERR(nfc->reg_clk);

--> here you test again the return value even if it was previously set to -ENOENT

	...

	ret = clk_prepare_enable(nfc->reg_clk);

--> here if reg_clk was NULL due to the beginning of the block you do a useless call to clk_prepare_enable

	if (ret)
		goto unprepare_ecc_clk;

Gregory

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/mtd/nand/marvell_nand.c +5 -0
+		} else {
+			ret = PTR_ERR(nfc->reg_clk);
+			goto unprepare_clk;
+		}
+	}  

Signature · 3 lines

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