[PATCH 1/2] mfd: madera: Allow more time for hardware reset

[PATCH 1/2] mfd: madera: Allow more time for hardware reset

[Charles Keepax]

Both manual and power on resets have a brief period where the chip will
not be accessible immediately afterwards. Extend the time allowed for
this from a minimum of 1mS to 2mS based on newer evaluation of the
hardware and ensure this reset happens in all reset conditions. Whilst
making the change also remove the redundant NULL checks in the reset
functions as the GPIO functions already check for this.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/mfd/madera-core.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
index a8cfadc1fc01e..f41ce408259fb 100644
--- a/drivers/mfd/madera-core.c
+++ b/drivers/mfd/madera-core.c
@@ -238,6 +238,11 @@ static int madera_wait_for_boot(struct madera *madera)
 	return ret;
 }
 
+static inline void madera_reset_delay(void)
+{
+	usleep_range(2000, 3000);
+}
+
 static int madera_soft_reset(struct madera *madera)
 {
 	int ret;
@@ -249,16 +254,13 @@ static int madera_soft_reset(struct madera *madera)
 	}
 
 	/* Allow time for internal clocks to startup after reset */
-	usleep_range(1000, 2000);
+	madera_reset_delay();
 
 	return 0;
 }
 
 static void madera_enable_hard_reset(struct madera *madera)
 {
-	if (!madera->pdata.reset)
-		return;
-
 	/*
 	 * There are many existing out-of-tree users of these codecs that we
 	 * can't break so preserve the expected behaviour of setting the line
@@ -269,11 +271,9 @@ static void madera_enable_hard_reset(struct madera *madera)
 
 static void madera_disable_hard_reset(struct madera *madera)
 {
-	if (!madera->pdata.reset)
-		return;
-
 	gpiod_set_raw_value_cansleep(madera->pdata.reset, 1);
-	usleep_range(1000, 2000);
+
+	madera_reset_delay();
 }
 
 static int __maybe_unused madera_runtime_resume(struct device *dev)
@@ -292,6 +292,8 @@ static int __maybe_unused madera_runtime_resume(struct device *dev)
 	regcache_cache_only(madera->regmap, false);
 	regcache_cache_only(madera->regmap_32bit, false);
 
+	madera_reset_delay();
+
 	ret = madera_wait_for_boot(madera);
 	if (ret)
 		goto err;
-- 
2.11.0

[PATCH 2/2] mfd: madera: Wait for boot done before accessing any other registers

[Charles Keepax]

It is advised to wait for the boot done bit to be set before reading
any other register, update the driver to respect this.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/mfd/madera-core.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
index f41ce408259fb..edf3d22dd82f9 100644
--- a/drivers/mfd/madera-core.c
+++ b/drivers/mfd/madera-core.c
@@ -199,7 +199,7 @@ EXPORT_SYMBOL_GPL(madera_name_from_type);
 #define MADERA_BOOT_POLL_INTERVAL_USEC		5000
 #define MADERA_BOOT_POLL_TIMEOUT_USEC		25000
 
-static int madera_wait_for_boot(struct madera *madera)
+static int madera_wait_for_boot_noack(struct madera *madera)
 {
 	ktime_t timeout;
 	unsigned int val = 0;
@@ -226,6 +226,13 @@ static int madera_wait_for_boot(struct madera *madera)
 		ret = -ETIMEDOUT;
 	}
 
+	return ret;
+}
+
+static int madera_wait_for_boot(struct madera *madera)
+{
+	int ret = madera_wait_for_boot_noack(madera);
+
 	/*
 	 * BOOT_DONE defaults to unmasked on boot so we must ack it.
 	 * Do this even after a timeout to avoid interrupt storms.
@@ -547,6 +554,12 @@ int madera_dev_init(struct madera *madera)
 	regcache_cache_only(madera->regmap, false);
 	regcache_cache_only(madera->regmap_32bit, false);
 
+	ret = madera_wait_for_boot_noack(madera);
+	if (ret) {
+		dev_err(madera->dev, "Device failed initial boot: %d\n", ret);
+		goto err_reset;
+	}
+
 	/*
 	 * Now we can power up and verify that this is a chip we know about
 	 * before we start doing any writes to its registers.
@@ -652,7 +665,7 @@ int madera_dev_init(struct madera *madera)
 
 	ret = madera_wait_for_boot(madera);
 	if (ret) {
-		dev_err(madera->dev, "Device failed initial boot: %d\n", ret);
+		dev_err(madera->dev, "Failed to clear boot done: %d\n", ret);
 		goto err_reset;
 	}
 
-- 
2.11.0

Re: [PATCH 1/2] mfd: madera: Allow more time for hardware reset

[Lee Jones]

On Mon, 06 Jan 2020, Charles Keepax wrote:

> Both manual and power on resets have a brief period where the chip will
> not be accessible immediately afterwards. Extend the time allowed for
> this from a minimum of 1mS to 2mS based on newer evaluation of the
> hardware and ensure this reset happens in all reset conditions. Whilst
> making the change also remove the redundant NULL checks in the reset
> functions as the GPIO functions already check for this.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  drivers/mfd/madera-core.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
> index a8cfadc1fc01e..f41ce408259fb 100644
> --- a/drivers/mfd/madera-core.c
> +++ b/drivers/mfd/madera-core.c
> @@ -238,6 +238,11 @@ static int madera_wait_for_boot(struct madera *madera)
>  	return ret;
>  }
>  
> +static inline void madera_reset_delay(void)
> +{
> +	usleep_range(2000, 3000);
> +}

Hmm ... We usually shy away from abstraction for the sake of
abstraction.  What's preventing you from using the preferred method of
simply calling the abstracted function from each of the call-sites?

I could understand (a little) if you needed to frequently change these
values, since changing them in once place is obviously simpler than
changing them in 3, but even then it's marginal.

>  static int madera_soft_reset(struct madera *madera)
>  {
>  	int ret;
> @@ -249,16 +254,13 @@ static int madera_soft_reset(struct madera *madera)
>  	}
>  
>  	/* Allow time for internal clocks to startup after reset */
> -	usleep_range(1000, 2000);
> +	madera_reset_delay();
>  
>  	return 0;
>  }
>  
>  static void madera_enable_hard_reset(struct madera *madera)
>  {
> -	if (!madera->pdata.reset)
> -		return;
> -
>  	/*
>  	 * There are many existing out-of-tree users of these codecs that we
>  	 * can't break so preserve the expected behaviour of setting the line
> @@ -269,11 +271,9 @@ static void madera_enable_hard_reset(struct madera *madera)
>  
>  static void madera_disable_hard_reset(struct madera *madera)
>  {
> -	if (!madera->pdata.reset)
> -		return;
> -
>  	gpiod_set_raw_value_cansleep(madera->pdata.reset, 1);
> -	usleep_range(1000, 2000);
> +
> +	madera_reset_delay();
>  }
>  
>  static int __maybe_unused madera_runtime_resume(struct device *dev)
> @@ -292,6 +292,8 @@ static int __maybe_unused madera_runtime_resume(struct device *dev)
>  	regcache_cache_only(madera->regmap, false);
>  	regcache_cache_only(madera->regmap_32bit, false);
>  
> +	madera_reset_delay();
> +
>  	ret = madera_wait_for_boot(madera);
>  	if (ret)
>  		goto err;

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: madera: Wait for boot done before accessing any other registers

[Lee Jones]

On Mon, 06 Jan 2020, Charles Keepax wrote:

> It is advised to wait for the boot done bit to be set before reading
> any other register, update the driver to respect this.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  drivers/mfd/madera-core.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)

I'm assuming this patch is orthogonal to the last?

Can I take it on its own?

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: madera: Wait for boot done before accessing any other registers

[Charles Keepax]

On Tue, Jan 07, 2020 at 02:29:42PM +0000, Lee Jones wrote:
> On Mon, 06 Jan 2020, Charles Keepax wrote:
> 
> > It is advised to wait for the boot done bit to be set before reading
> > any other register, update the driver to respect this.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> >  drivers/mfd/madera-core.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> I'm assuming this patch is orthogonal to the last?
> 
> Can I take it on its own?
> 

Yeah these can be taken separately to the other series we are
waiting on Mark to review.

Thanks,
Charles

> For my own reference:
>   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> 
> -- 
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] mfd: madera: Allow more time for hardware reset

[Charles Keepax]

On Tue, Jan 07, 2020 at 02:27:42PM +0000, Lee Jones wrote:
> On Mon, 06 Jan 2020, Charles Keepax wrote:
> 
> > Both manual and power on resets have a brief period where the chip will
> > not be accessible immediately afterwards. Extend the time allowed for
> > this from a minimum of 1mS to 2mS based on newer evaluation of the
> > hardware and ensure this reset happens in all reset conditions. Whilst
> > making the change also remove the redundant NULL checks in the reset
> > functions as the GPIO functions already check for this.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> >  drivers/mfd/madera-core.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
> > index a8cfadc1fc01e..f41ce408259fb 100644
> > --- a/drivers/mfd/madera-core.c
> > +++ b/drivers/mfd/madera-core.c
> > @@ -238,6 +238,11 @@ static int madera_wait_for_boot(struct madera *madera)
> >  	return ret;
> >  }
> >  
> > +static inline void madera_reset_delay(void)
> > +{
> > +	usleep_range(2000, 3000);
> > +}
> 
> Hmm ... We usually shy away from abstraction for the sake of
> abstraction.  What's preventing you from using the preferred method of
> simply calling the abstracted function from each of the call-sites?
> 
> I could understand (a little) if you needed to frequently change these
> values, since changing them in once place is obviously simpler than
> changing them in 3, but even then it's marginal.
> 

I don't mind manually inline it, we don't plan on changing the
values very often certainly. It really was just to avoid future
bugs if someone adds a new place that needs the delay or does
indeed change the delay. Would you mind if I used a define for
the time instead, if I am manually inlining? That keeps the same
single place to update, but without the extra function.

Thanks,
Charles

Re: [PATCH 1/2] mfd: madera: Allow more time for hardware reset

[Lee Jones]

On Wed, 08 Jan 2020, Charles Keepax wrote:

> On Tue, Jan 07, 2020 at 02:27:42PM +0000, Lee Jones wrote:
> > On Mon, 06 Jan 2020, Charles Keepax wrote:
> > 
> > > Both manual and power on resets have a brief period where the chip will
> > > not be accessible immediately afterwards. Extend the time allowed for
> > > this from a minimum of 1mS to 2mS based on newer evaluation of the
> > > hardware and ensure this reset happens in all reset conditions. Whilst
> > > making the change also remove the redundant NULL checks in the reset
> > > functions as the GPIO functions already check for this.
> > > 
> > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > > ---
> > >  drivers/mfd/madera-core.c | 18 ++++++++++--------
> > >  1 file changed, 10 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/madera-core.c b/drivers/mfd/madera-core.c
> > > index a8cfadc1fc01e..f41ce408259fb 100644
> > > --- a/drivers/mfd/madera-core.c
> > > +++ b/drivers/mfd/madera-core.c
> > > @@ -238,6 +238,11 @@ static int madera_wait_for_boot(struct madera *madera)
> > >  	return ret;
> > >  }
> > >  
> > > +static inline void madera_reset_delay(void)
> > > +{
> > > +	usleep_range(2000, 3000);
> > > +}
> > 
> > Hmm ... We usually shy away from abstraction for the sake of
> > abstraction.  What's preventing you from using the preferred method of
> > simply calling the abstracted function from each of the call-sites?
> > 
> > I could understand (a little) if you needed to frequently change these
> > values, since changing them in once place is obviously simpler than
> > changing them in 3, but even then it's marginal.
> > 
> 
> I don't mind manually inline it, we don't plan on changing the
> values very often certainly. It really was just to avoid future
> bugs if someone adds a new place that needs the delay or does
> indeed change the delay. Would you mind if I used a define for
> the time instead, if I am manually inlining? That keeps the same
> single place to update, but without the extra function.

That would be my preference, yes.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: madera: Wait for boot done before accessing any other registers

[Lee Jones]

On Wed, 08 Jan 2020, Charles Keepax wrote:

> On Tue, Jan 07, 2020 at 02:29:42PM +0000, Lee Jones wrote:
> > On Mon, 06 Jan 2020, Charles Keepax wrote:
> > 
> > > It is advised to wait for the boot done bit to be set before reading
> > > any other register, update the driver to respect this.
> > > 
> > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > > ---
> > >  drivers/mfd/madera-core.c | 17 +++++++++++++++--
> > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > I'm assuming this patch is orthogonal to the last?
> > 
> > Can I take it on its own?
> > 
> 
> Yeah these can be taken separately to the other series we are
> waiting on Mark to review.

I mean the patch.  Can I take 2/2 without 1/2?

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: madera: Wait for boot done before accessing any other registers

[Charles Keepax]

On Mon, Jan 13, 2020 at 10:44:25AM +0000, Lee Jones wrote:
> On Wed, 08 Jan 2020, Charles Keepax wrote:
> 
> > On Tue, Jan 07, 2020 at 02:29:42PM +0000, Lee Jones wrote:
> > > On Mon, 06 Jan 2020, Charles Keepax wrote:
> > > 
> > > > It is advised to wait for the boot done bit to be set before reading
> > > > any other register, update the driver to respect this.
> > > > 
> > > > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > > > ---
> > > >  drivers/mfd/madera-core.c | 17 +++++++++++++++--
> > > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > 
> > > I'm assuming this patch is orthogonal to the last?
> > > 
> > > Can I take it on its own?
> > > 
> > 
> > Yeah these can be taken separately to the other series we are
> > waiting on Mark to review.
> 
> I mean the patch.  Can I take 2/2 without 1/2?
> 

Sorry, yes that should work too. Just tested it on my system
seems fine.

Thanks,
Charles

Re: [PATCH 2/2] mfd: madera: Wait for boot done before accessing any other registers

[Lee Jones]

On Mon, 06 Jan 2020, Charles Keepax wrote:

> It is advised to wait for the boot done bit to be set before reading
> any other register, update the driver to respect this.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  drivers/mfd/madera-core.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog