[PATCH v1 0/1] iio: adc: meson: fix core clock enable/disable moment

[PATCH v1 0/1] iio: adc: meson: fix core clock enable/disable moment

[George Stark]

This patch is a part of effort to support meson a1 SoC and make meson saradc driver
independent from vendor boot code initialization in common.

Core clock (passed to adc module thru dts) is supposed to be responsible for entier module
and should be on before accessing modules' regs.

I've made experiments and here are the results:

on odroid-c1 (meson8) adc regs became readonly with core clock off:
# disable clock (HHI_GCLK_MPEG0 bit 10)
devmem 0xc1104140 32 0xBFFA72FF
devmem 0xc110868C
0xE3A851FF
devmem 0xc110868C 32 0xE3A85100
devmem 0xc110868C
0xE3A851FF
# enable clock
devmem 0xc1104140 32 0xBFFA76FF
 devmem 0xc110868C
0xE3A851FF
devmem 0xc110868C 32 0xE3A85100
devmem 0xc110868C
0xE3A85100

on vim3 (a311d) adc regs became readonly with core clock off:
# disable adc core clock:
devmem 0xff80004C 32 0xFFFFFEFF
# the adc register become readonly:
devmem 0xff80902c
0x002C2002
devmem 0xff80902c 32 0x002C2000
devmem 0xff80902c
0x002C2002

on a1 adc registers are none-readable-writeable when adc core clock is off:
devmem 0xfe002c2c
0x00002003
# disable clock
devmem 0xfe00081c 32 0xFFFF9FFF
devmem 0xfe002c2c
0x00000000
# enable clock
devmem 0xfe00081c 32 0xFFFFFFFF
devmem 0xfe002c2c
0x00002003

George Stark (1):
  iio: adc: meson: fix core clock enable/disable moment

 drivers/iio/adc/meson_saradc.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

-- 
2.38.4

[PATCH v1 1/1] iio: adc: meson: fix core clock enable/disable moment

[George Stark]

Enable core clock at probe stage and disable it at remove stage.
Core clock is responsible for turning on/off the entire SoC module so
it should be on before the first module register is touched and be off
at very last moment.

Signed-off-by: George Stark <gnstark@sberdevices.ru>
---
 drivers/iio/adc/meson_saradc.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index fe066c9fab83..5a5bb5cc1320 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -1055,12 +1055,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev)
 		goto err_vref;
 	}
 
-	ret = clk_prepare_enable(priv->core_clk);
-	if (ret) {
-		dev_err(dev, "failed to enable core clk\n");
-		goto err_core_clk;
-	}
-
 	regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1);
 	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
 			   MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval);
@@ -1087,8 +1081,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev)
 	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3,
 			   MESON_SAR_ADC_REG3_ADC_EN, 0);
 	meson_sar_adc_set_bandgap(indio_dev, false);
-	clk_disable_unprepare(priv->core_clk);
-err_core_clk:
 	regulator_disable(priv->vref);
 err_vref:
 	meson_sar_adc_unlock(indio_dev);
@@ -1116,8 +1108,6 @@ static void meson_sar_adc_hw_disable(struct iio_dev *indio_dev)
 
 	meson_sar_adc_set_bandgap(indio_dev, false);
 
-	clk_disable_unprepare(priv->core_clk);
-
 	regulator_disable(priv->vref);
 
 	if (!ret)
@@ -1420,6 +1410,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 			ARRAY_SIZE(meson_sar_adc_iio_channels);
 	}
 
+	ret = clk_prepare_enable(priv->core_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable core clk\n");
+		goto err;
+	}
+
 	ret = meson_sar_adc_init(indio_dev);
 	if (ret)
 		goto err;
@@ -1445,17 +1441,21 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 err_hw:
 	meson_sar_adc_hw_disable(indio_dev);
 err:
+	clk_disable_unprepare(priv->core_clk);
 	return ret;
 }
 
 static int meson_sar_adc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
 
 	meson_sar_adc_hw_disable(indio_dev);
 
+	clk_disable_unprepare(priv->core_clk);
+
 	return 0;
 }
 
-- 
2.38.4

Re: [PATCH v1 1/1] iio: adc: meson: fix core clock enable/disable moment

[Jonathan Cameron]

On Fri, 14 Jul 2023 22:03:15 +0300
George Stark <gnstark@sberdevices.ru> wrote:

> Enable core clock at probe stage and disable it at remove stage.
> Core clock is responsible for turning on/off the entire SoC module so
> it should be on before the first module register is touched and be off
> at very last moment.
> 
> Signed-off-by: George Stark <gnstark@sberdevices.ru>

This sounds to me like this should perhaps have a fixes tag?

Given where it is in the new sequence you can also use
devm_clk_get_enabled() though that makes a potential backport of
the fix trickier...

I'd go with it anyway as it will make this change quite a bit simpler.

More comments inline

Jonathan

> ---
>  drivers/iio/adc/meson_saradc.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index fe066c9fab83..5a5bb5cc1320 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -1055,12 +1055,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev)
>  		goto err_vref;
>  	}
>  
> -	ret = clk_prepare_enable(priv->core_clk);
> -	if (ret) {
> -		dev_err(dev, "failed to enable core clk\n");
> -		goto err_core_clk;
> -	}
> -
>  	regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1);
>  	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
>  			   MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval);
> @@ -1087,8 +1081,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev)
>  	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3,
>  			   MESON_SAR_ADC_REG3_ADC_EN, 0);
>  	meson_sar_adc_set_bandgap(indio_dev, false);
> -	clk_disable_unprepare(priv->core_clk);
> -err_core_clk:
>  	regulator_disable(priv->vref);
>  err_vref:
>  	meson_sar_adc_unlock(indio_dev);
> @@ -1116,8 +1108,6 @@ static void meson_sar_adc_hw_disable(struct iio_dev *indio_dev)
>  
>  	meson_sar_adc_set_bandgap(indio_dev, false);
>  
> -	clk_disable_unprepare(priv->core_clk);
> -
>  	regulator_disable(priv->vref);
>  
>  	if (!ret)
> @@ -1420,6 +1410,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>  			ARRAY_SIZE(meson_sar_adc_iio_channels);
>  	}
>  
> +	ret = clk_prepare_enable(priv->core_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable core clk\n");
> +		goto err;

If clk_prepare_enable() failed, then you shoudl not call clk_disable_unprepare()

> +	}
> +
>  	ret = meson_sar_adc_init(indio_dev);
>  	if (ret)
>  		goto err;
> @@ -1445,17 +1441,21 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>  err_hw:
>  	meson_sar_adc_hw_disable(indio_dev);
>  err:
> +	clk_disable_unprepare(priv->core_clk);

Nothing to do with your patch, but this driver previously did some odd mixing
of direct returns and going to an empty label which definitely doesn't help
make this clear to follow.

>  	return ret;
>  }
>  
>  static int meson_sar_adc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
>  
>  	meson_sar_adc_hw_disable(indio_dev);
>  
> +	clk_disable_unprepare(priv->core_clk);
> +
>  	return 0;
>  }
>  

Re: [PATCH v1 1/1] iio: adc: meson: fix core clock enable/disable moment

[George Stark]

Hello Jonathan

Thanks for the feedback.

On 7/15/23 20:21, Jonathan Cameron wrote:
> On Fri, 14 Jul 2023 22:03:15 +0300
> George Stark <gnstark@sberdevices.ru> wrote:
> 
>> Enable core clock at probe stage and disable it at remove stage.
>> Core clock is responsible for turning on/off the entire SoC module so
>> it should be on before the first module register is touched and be off
>> at very last moment.
>>
>> Signed-off-by: George Stark <gnstark@sberdevices.ru>
> 
> This sounds to me like this should perhaps have a fixes tag?
ack
> 
> Given where it is in the new sequence you can also use
> devm_clk_get_enabled() though that makes a potential backport of
> the fix trickier...
> 
> I'd go with it anyway as it will make this change quite a bit simpler.
ack
> 
> More comments inline
> 
> Jonathan
> 
>> ---
>>   drivers/iio/adc/meson_saradc.c | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
>> index fe066c9fab83..5a5bb5cc1320 100644
>> --- a/drivers/iio/adc/meson_saradc.c
>> +++ b/drivers/iio/adc/meson_saradc.c
>> @@ -1055,12 +1055,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev)
>>   		goto err_vref;
>>   	}
>>   
>> -	ret = clk_prepare_enable(priv->core_clk);
>> -	if (ret) {
>> -		dev_err(dev, "failed to enable core clk\n");
>> -		goto err_core_clk;
>> -	}
>> -
>>   	regval = FIELD_PREP(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, 1);
>>   	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
>>   			   MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval);
>> @@ -1087,8 +1081,6 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev)
>>   	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG3,
>>   			   MESON_SAR_ADC_REG3_ADC_EN, 0);
>>   	meson_sar_adc_set_bandgap(indio_dev, false);
>> -	clk_disable_unprepare(priv->core_clk);
>> -err_core_clk:
>>   	regulator_disable(priv->vref);
>>   err_vref:
>>   	meson_sar_adc_unlock(indio_dev);
>> @@ -1116,8 +1108,6 @@ static void meson_sar_adc_hw_disable(struct iio_dev *indio_dev)
>>   
>>   	meson_sar_adc_set_bandgap(indio_dev, false);
>>   
>> -	clk_disable_unprepare(priv->core_clk);
>> -
>>   	regulator_disable(priv->vref);
>>   
>>   	if (!ret)
>> @@ -1420,6 +1410,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>>   			ARRAY_SIZE(meson_sar_adc_iio_channels);
>>   	}
>>   
>> +	ret = clk_prepare_enable(priv->core_clk);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable core clk\n");
>> +		goto err;
> 
> If clk_prepare_enable() failed, then you shoudl not call clk_disable_unprepare()
yes, but with devm_clk_get_enabled we dont't need this line anymore
> 
>> +	}
>> +
>>   	ret = meson_sar_adc_init(indio_dev);
>>   	if (ret)
>>   		goto err;
>> @@ -1445,17 +1441,21 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>>   err_hw:
>>   	meson_sar_adc_hw_disable(indio_dev);
>>   err:
>> +	clk_disable_unprepare(priv->core_clk);
> 
> Nothing to do with your patch, but this driver previously did some odd mixing
> of direct returns and going to an empty label which definitely doesn't help
> make this clear to follow.
I made additional patch adding log messages to all errors those can 
break probe stage
> 
>>   	return ret;
>>   }
>>   
>>   static int meson_sar_adc_remove(struct platform_device *pdev)
>>   {
>>   	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>>   
>>   	iio_device_unregister(indio_dev);
>>   
>>   	meson_sar_adc_hw_disable(indio_dev);
>>   
>> +	clk_disable_unprepare(priv->core_clk);
>> +
>>   	return 0;
>>   }
>>   
> 

-- 
Best regards
George