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

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

[Boris Brezillon]

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

On Wed,  7 Mar 2018 17:13:16 +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>
> ---
> 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(-)
> 
> 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

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

>  - 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..072e23635375 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;

There's a kernel-doc header describing the marvell_nfc fields, could
you add an entry for 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;

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.

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


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

Please rename the label (unprepare_clk -> unprepare_ecc_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;



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

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

[Gregory CLEMENT]

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

>>    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..072e23635375 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;
>
> There's a kernel-doc header describing the marvell_nfc fields, could
> you add an entry for reg_clk?

OK I will

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

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.


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

>
> 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;
>
>
>> +
>>  	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:
>
> Please rename the label (unprepare_clk -> unprepare_ecc_clk).

OK

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

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

[Boris Brezillon]

On Mon, 12 Mar 2018 17:55:26 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

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

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

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.


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

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

[Gregory CLEMENT]

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

> On Mon, 12 Mar 2018 17:55:26 +0100
> Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
>
>> >  
>> >>  	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.
>> >  
>> 
>> 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

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

> 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

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



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