[PATCH] mtd: nand: marvell: Fix clock resource by adding a register clock

[PATCH] mtd: nand: marvell: 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/mtd/marvell-nand.txt |  6 +++++-
 drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
index c08fb477b3c6..4ee9813bf88f 100644
--- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
@@ -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).
diff --git a/drivers/mtd/nand/marvell_nand.c b/drivers/mtd/nand/marvell_nand.c
index 2196f2a233d6..be874c636b5f 100644
--- a/drivers/mtd/nand/marvell_nand.c
+++ b/drivers/mtd/nand/marvell_nand.c
@@ -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;
 }
-- 
2.16.1

[PATCH] mtd: nand: marvell: 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/mtd/marvell-nand.txt |  6 +++++-
 drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
index c08fb477b3c6..4ee9813bf88f 100644
--- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
@@ -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).
diff --git a/drivers/mtd/nand/marvell_nand.c b/drivers/mtd/nand/marvell_nand.c
index 2196f2a233d6..be874c636b5f 100644
--- a/drivers/mtd/nand/marvell_nand.c
+++ b/drivers/mtd/nand/marvell_nand.c
@@ -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;
 }
-- 
2.16.1

Re: [PATCH] mtd: nand: marvell: Fix clock resource by adding a register clock

[Boris Brezillon]

Hi Greg,

On Wed, 28 Feb 2018 15:35:53 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> 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/mtd/marvell-nand.txt |  6 +++++-
>  drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> index c08fb477b3c6..4ee9813bf88f 100644
> --- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> @@ -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).

>  - 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).
> diff --git a/drivers/mtd/nand/marvell_nand.c b/drivers/mtd/nand/marvell_nand.c
> index 2196f2a233d6..be874c636b5f 100644
> --- a/drivers/mtd/nand/marvell_nand.c
> +++ b/drivers/mtd/nand/marvell_nand.c
> @@ -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");

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.

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

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

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

Regards,

Boris

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

[PATCH] mtd: nand: marvell: Fix clock resource by adding a register clock

[Boris Brezillon]

Hi Greg,

On Wed, 28 Feb 2018 15:35:53 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> 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/mtd/marvell-nand.txt |  6 +++++-
>  drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> index c08fb477b3c6..4ee9813bf88f 100644
> --- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> @@ -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).

>  - 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).
> diff --git a/drivers/mtd/nand/marvell_nand.c b/drivers/mtd/nand/marvell_nand.c
> index 2196f2a233d6..be874c636b5f 100644
> --- a/drivers/mtd/nand/marvell_nand.c
> +++ b/drivers/mtd/nand/marvell_nand.c
> @@ -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");

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.

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

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

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

Regards,

Boris

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

Re: [PATCH] mtd: nand: marvell: Fix clock resource by adding a register clock

[Gregory CLEMENT]

Hi Boris,
 
 On jeu., mars 01 2018, Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> Hi Greg,
>
> On Wed, 28 Feb 2018 15:35:53 +0100
> Gregory CLEMENT <gregory.clement@bootlin.com> 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/mtd/marvell-nand.txt |  6 +++++-
>>  drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
>>  2 files changed, 19 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
>> index c08fb477b3c6..4ee9813bf88f 100644
>> --- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
>> @@ -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.

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

>
>>  - 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).
>> diff --git a/drivers/mtd/nand/marvell_nand.c b/drivers/mtd/nand/marvell_nand.c
>> index 2196f2a233d6..be874c636b5f 100644
>> --- a/drivers/mtd/nand/marvell_nand.c
>> +++ b/drivers/mtd/nand/marvell_nand.c
>> @@ -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");
>
> 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.
>
>> +	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.


Indeed it makes sens.

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

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


OK

Thanks,

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

[PATCH] mtd: nand: marvell: Fix clock resource by adding a register clock

[Gregory CLEMENT]

Hi Boris,
 
 On jeu., mars 01 2018, Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> Hi Greg,
>
> On Wed, 28 Feb 2018 15:35:53 +0100
> Gregory CLEMENT <gregory.clement@bootlin.com> 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/mtd/marvell-nand.txt |  6 +++++-
>>  drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
>>  2 files changed, 19 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
>> index c08fb477b3c6..4ee9813bf88f 100644
>> --- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
>> @@ -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.

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

>
>>  - 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).
>> diff --git a/drivers/mtd/nand/marvell_nand.c b/drivers/mtd/nand/marvell_nand.c
>> index 2196f2a233d6..be874c636b5f 100644
>> --- a/drivers/mtd/nand/marvell_nand.c
>> +++ b/drivers/mtd/nand/marvell_nand.c
>> @@ -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");
>
> 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.
>
>> +	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.


Indeed it makes sens.

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

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


OK

Thanks,

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

Re: [PATCH] mtd: nand: marvell: Fix clock resource by adding a register clock

[Boris Brezillon]

Hi Greg,

On Tue, 06 Mar 2018 12:04:41 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> Hi Boris,
>  
>  On jeu., mars 01 2018, Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> 
> > Hi Greg,
> >
> > On Wed, 28 Feb 2018 15:35:53 +0100
> > Gregory CLEMENT <gregory.clement@bootlin.com> 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/mtd/marvell-nand.txt |  6 +++++-
> >>  drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
> >>  2 files changed, 19 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> >> index c08fb477b3c6..4ee9813bf88f 100644
> >> --- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> >> +++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> >> @@ -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.

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

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

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

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

[PATCH] mtd: nand: marvell: Fix clock resource by adding a register clock

[Boris Brezillon]

Hi Greg,

On Tue, 06 Mar 2018 12:04:41 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> Hi Boris,
>  
>  On jeu., mars 01 2018, Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> 
> > Hi Greg,
> >
> > On Wed, 28 Feb 2018 15:35:53 +0100
> > Gregory CLEMENT <gregory.clement@bootlin.com> 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/mtd/marvell-nand.txt |  6 +++++-
> >>  drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
> >>  2 files changed, 19 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> >> index c08fb477b3c6..4ee9813bf88f 100644
> >> --- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> >> +++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> >> @@ -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.

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

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

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

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

Re: [PATCH] mtd: nand: marvell: Fix clock resource by adding a register clock

[Russell King - ARM Linux]

On Tue, Mar 06, 2018 at 01:43:11PM +0100, Boris Brezillon wrote:
> Hi Greg,
> 
> On Tue, 06 Mar 2018 12:04:41 +0100
> Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
> 
> > Hi Boris,
> >  
> >  On jeu., mars 01 2018, Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> > 
> > > Hi Greg,
> > >
> > > On Wed, 28 Feb 2018 15:35:53 +0100
> > > Gregory CLEMENT <gregory.clement@bootlin.com> 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/mtd/marvell-nand.txt |  6 +++++-
> > >>  drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
> > >>  2 files changed, 19 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > >> index c08fb477b3c6..4ee9813bf88f 100644
> > >> --- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > >> +++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > >> @@ -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.
> 
> I'm still not convinced this is a good idea, but okay.
> 
> > 
> > > 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);

	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?

> 
> > 
> > >
> > > 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
> 
> -- 
> Boris Brezillon, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
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] mtd: nand: marvell: Fix clock resource by adding a register clock

[Russell King - ARM Linux]

On Tue, Mar 06, 2018 at 01:43:11PM +0100, Boris Brezillon wrote:
> Hi Greg,
> 
> On Tue, 06 Mar 2018 12:04:41 +0100
> Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
> 
> > Hi Boris,
> >  
> >  On jeu., mars 01 2018, Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> > 
> > > Hi Greg,
> > >
> > > On Wed, 28 Feb 2018 15:35:53 +0100
> > > Gregory CLEMENT <gregory.clement@bootlin.com> 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/mtd/marvell-nand.txt |  6 +++++-
> > >>  drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
> > >>  2 files changed, 19 insertions(+), 1 deletion(-)
> > >> 
> > >> diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > >> index c08fb477b3c6..4ee9813bf88f 100644
> > >> --- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > >> +++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > >> @@ -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.
> 
> I'm still not convinced this is a good idea, but okay.
> 
> > 
> > > 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);

	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?

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

-- 
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] mtd: nand: marvell: Fix clock resource by adding a register clock

[Boris Brezillon]

On Tue, 6 Mar 2018 13:08:33 +0000
Russell King - ARM Linux <linux@armlinux.org.uk> wrote:

> On Tue, Mar 06, 2018 at 01:43:11PM +0100, Boris Brezillon wrote:
> > Hi Greg,
> > 
> > On Tue, 06 Mar 2018 12:04:41 +0100
> > Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
> >   
> > > Hi Boris,
> > >  
> > >  On jeu., mars 01 2018, Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> > >   
> > > > Hi Greg,
> > > >
> > > > On Wed, 28 Feb 2018 15:35:53 +0100
> > > > Gregory CLEMENT <gregory.clement@bootlin.com> 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/mtd/marvell-nand.txt |  6 +++++-
> > > >>  drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
> > > >>  2 files changed, 19 insertions(+), 1 deletion(-)
> > > >> 
> > > >> diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > > >> index c08fb477b3c6..4ee9813bf88f 100644
> > > >> --- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > > >> +++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > > >> @@ -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.  
> > 
> > I'm still not convinced this is a good idea, but okay.
> >   
> > >   
> > > > 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);  
> 
> 	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.

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

[PATCH] mtd: nand: marvell: Fix clock resource by adding a register clock

[Boris Brezillon]

On Tue, 6 Mar 2018 13:08:33 +0000
Russell King - ARM Linux <linux@armlinux.org.uk> wrote:

> On Tue, Mar 06, 2018 at 01:43:11PM +0100, Boris Brezillon wrote:
> > Hi Greg,
> > 
> > On Tue, 06 Mar 2018 12:04:41 +0100
> > Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
> >   
> > > Hi Boris,
> > >  
> > >  On jeu., mars 01 2018, Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> > >   
> > > > Hi Greg,
> > > >
> > > > On Wed, 28 Feb 2018 15:35:53 +0100
> > > > Gregory CLEMENT <gregory.clement@bootlin.com> 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/mtd/marvell-nand.txt |  6 +++++-
> > > >>  drivers/mtd/nand/marvell_nand.c                        | 14 ++++++++++++++
> > > >>  2 files changed, 19 insertions(+), 1 deletion(-)
> > > >> 
> > > >> diff --git a/Documentation/devicetree/bindings/mtd/marvell-nand.txt b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > > >> index c08fb477b3c6..4ee9813bf88f 100644
> > > >> --- a/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > > >> +++ b/Documentation/devicetree/bindings/mtd/marvell-nand.txt
> > > >> @@ -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.  
> > 
> > I'm still not convinced this is a good idea, but okay.
> >   
> > >   
> > > > 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);  
> 
> 	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.

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

Re: [PATCH] mtd: nand: marvell: Fix clock resource by adding a register clock

[Gregory CLEMENT]

Hi Boris,
 
 On mar., mars 06 2018, Boris Brezillon <boris.brezillon@bootlin.com> wrote:

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


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

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

[PATCH] mtd: nand: marvell: Fix clock resource by adding a register clock

[Gregory CLEMENT]

Hi Boris,
 
 On mar., mars 06 2018, Boris Brezillon <boris.brezillon@bootlin.com> wrote:

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


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

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