[PATCH v2 0/2] iio: adc: meson: fix core clock enable/disable moment

[PATCH v2 0/2] 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

Changelog:

v1->v2:
patch 'iio: adc: meson: improve error logging at probe stage'
	- add fixes tag, previous version [1]
	- move to devm_clk_get_enabled
	- return enable/disable core clock calls to suspend \ resume callbacks
	but did it at the last step and the first step respectively

patch 'iio: adc: meson: improve error logging at probe stage'
	patch was added to address Jonathan's comment

[1] https://www.spinics.net/lists/linux-iio/msg80369.html

George Stark (2):
  iio: adc: meson: fix core clock enable/disable moment
  iio: adc: meson: improve error logging at probe stage

 drivers/iio/adc/meson_saradc.c | 39 +++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 17 deletions(-)

-- 
2.38.4


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

[PATCH v2 1/2] 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.

Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs")
Signed-off-by: George Stark <gnstark@sberdevices.ru>
---
 drivers/iio/adc/meson_saradc.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 2ee12f3ad312..8397a5347f32 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)
@@ -1379,7 +1369,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->clkin))
 		return dev_err_probe(dev, PTR_ERR(priv->clkin), "failed to get clkin\n");
 
-	priv->core_clk = devm_clk_get(dev, "core");
+	priv->core_clk = devm_clk_get_enabled(dev, "core");
 	if (IS_ERR(priv->core_clk))
 		return dev_err_probe(dev, PTR_ERR(priv->core_clk), "failed to get core clk\n");
 
@@ -1462,15 +1452,26 @@ static int meson_sar_adc_remove(struct platform_device *pdev)
 static int meson_sar_adc_suspend(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
 
 	meson_sar_adc_hw_disable(indio_dev);
 
+	clk_disable_unprepare(priv->core_clk);
+
 	return 0;
 }
 
 static int meson_sar_adc_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
+	int ret;
+
+	ret = clk_prepare_enable(priv->core_clk);
+	if (ret) {
+		dev_err(dev, "failed to enable core clk\n");
+		return ret;
+	}
 
 	return meson_sar_adc_hw_enable(indio_dev);
 }
-- 
2.38.4


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

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

[Jonathan Cameron]

On Fri, 21 Jul 2023 13:23:08 +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.
> 
> Fixes: 3adbf3427330 ("iio: adc: add a driver for the SAR ADC found in Amlogic Meson SoCs")
> Signed-off-by: George Stark <gnstark@sberdevices.ru>
Applied to the fixes-togreg branch of iio.git and marked for stable.

The 2nd patch will have to wait as it's not a fix.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/meson_saradc.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index 2ee12f3ad312..8397a5347f32 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)
> @@ -1379,7 +1369,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->clkin))
>  		return dev_err_probe(dev, PTR_ERR(priv->clkin), "failed to get clkin\n");
>  
> -	priv->core_clk = devm_clk_get(dev, "core");
> +	priv->core_clk = devm_clk_get_enabled(dev, "core");
>  	if (IS_ERR(priv->core_clk))
>  		return dev_err_probe(dev, PTR_ERR(priv->core_clk), "failed to get core clk\n");
>  
> @@ -1462,15 +1452,26 @@ static int meson_sar_adc_remove(struct platform_device *pdev)
>  static int meson_sar_adc_suspend(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>  
>  	meson_sar_adc_hw_disable(indio_dev);
>  
> +	clk_disable_unprepare(priv->core_clk);
> +
>  	return 0;
>  }
>  
>  static int meson_sar_adc_resume(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->core_clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable core clk\n");
> +		return ret;
> +	}
>  
>  	return meson_sar_adc_hw_enable(indio_dev);
>  }


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

[PATCH v2 2/2] iio: adc: meson: improve error logging at probe stage

[George Stark]

Add log messages for errors that may occur at probe stage

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

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 8397a5347f32..11b10459a028 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -1046,8 +1046,10 @@ static int meson_sar_adc_hw_enable(struct iio_dev *indio_dev)
 	u32 regval;
 
 	ret = meson_sar_adc_lock(indio_dev);
-	if (ret)
+	if (ret) {
+		dev_err(dev, "failed to lock adc\n");
 		goto err_lock;
+	}
 
 	ret = regulator_enable(priv->vref);
 	if (ret < 0) {
@@ -1355,15 +1357,15 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 
 	priv->regmap = devm_regmap_init_mmio(dev, base, priv->param->regmap_config);
 	if (IS_ERR(priv->regmap))
-		return PTR_ERR(priv->regmap);
+		return dev_err_probe(dev, PTR_ERR(priv->regmap), "failed to init regmap\n");
 
 	irq = irq_of_parse_and_map(dev->of_node, 0);
 	if (!irq)
-		return -EINVAL;
+		return dev_err_probe(dev, -EINVAL, "failed to get irq\n");
 
 	ret = devm_request_irq(dev, irq, meson_sar_adc_irq, IRQF_SHARED, dev_name(dev), indio_dev);
 	if (ret)
-		return ret;
+		return dev_err_probe(dev, ret, "failed to request irq\n");
 
 	priv->clkin = devm_clk_get(dev, "clkin");
 	if (IS_ERR(priv->clkin))
@@ -1385,7 +1387,7 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 	if (!priv->adc_clk) {
 		ret = meson_sar_adc_clk_init(indio_dev, base);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret, "failed to init internal clk\n");
 	}
 
 	priv->vref = devm_regulator_get(dev, "vref");
@@ -1427,8 +1429,10 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, indio_dev);
 
 	ret = iio_device_register(indio_dev);
-	if (ret)
+	if (ret) {
+		dev_err_probe(dev, ret, "failed to register iio device\n");
 		goto err_hw;
+	}
 
 	return 0;
 
-- 
2.38.4


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

Re: [PATCH v2 2/2] iio: adc: meson: improve error logging at probe stage

[Andy Shevchenko]

On Fri, Jul 21, 2023 at 01:23:09PM +0300, George Stark wrote:
> Add log messages for errors that may occur at probe stage

the probe

stage.

-- 
With Best Regards,
Andy Shevchenko



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

Re: [PATCH v2 2/2] iio: adc: meson: improve error logging at probe stage

[Jonathan Cameron]

On Fri, 21 Jul 2023 13:43:51 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Jul 21, 2023 at 01:23:09PM +0300, George Stark wrote:
> > Add log messages for errors that may occur at probe stage  
> 
> the probe

British English - excellent ! :) (I argue this one far too often in
specification committees :)


> 
> stage.
> 

I can tidy this up, but the patch will be a while anyway given the fix
needs to work it's way into the upstream for my togreg branch (and I need
to have send a pull request for togreg as well).

Jonathan


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

Re: [PATCH v2 2/2] iio: adc: meson: improve error logging at probe stage

[George Stark]

Hello Jonathan

It seems like you haven't had time for this patch yet. Should I fix it 
myself or just be more patient?

On 7/22/23 20:15, Jonathan Cameron wrote:
> On Fri, 21 Jul 2023 13:43:51 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
>> On Fri, Jul 21, 2023 at 01:23:09PM +0300, George Stark wrote:
>>> Add log messages for errors that may occur at probe stage
>>
>> the probe
> 
> British English - excellent ! :) (I argue this one far too often in
> specification committees :)
> 
> 
>>
>> stage.
>>
> 
> I can tidy this up, but the patch will be a while anyway given the fix
> needs to work it's way into the upstream for my togreg branch (and I need
> to have send a pull request for togreg as well).
> 
> Jonathan
> 

-- 
Best regards
George

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

Re: [PATCH v2 2/2] iio: adc: meson: improve error logging at probe stage

[Jonathan Cameron]

On Tue, 3 Oct 2023 15:47:23 +0300
George Stark <gnstark@sberdevices.ru> wrote:

> Hello Jonathan
> 
> It seems like you haven't had time for this patch yet. Should I fix it 
> myself or just be more patient?
I've been exceptionally slow on this one.  Was still sitting in patchwork
but I'd failed to notice the dependency was now upstream.

Applied to the togreg branch of iio.git and I'll push that out initially
as testing later today (I have to do a rebase to fix up an issue
0-day caught on previous push before I do).

Thanks for the poke and sorry it was necessary!

Jonathan

> 
> On 7/22/23 20:15, Jonathan Cameron wrote:
> > On Fri, 21 Jul 2023 13:43:51 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >   
> >> On Fri, Jul 21, 2023 at 01:23:09PM +0300, George Stark wrote:  
> >>> Add log messages for errors that may occur at probe stage  
> >>
> >> the probe  
> > 
> > British English - excellent ! :) (I argue this one far too often in
> > specification committees :)
> > 
> >   
> >>
> >> stage.
> >>  
> > 
> > I can tidy this up, but the patch will be a while anyway given the fix
> > needs to work it's way into the upstream for my togreg branch (and I need
> > to have send a pull request for togreg as well).
> > 
> > Jonathan
> >   
> 


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