| Message ID | 20181126183942.2631-1-ryans.lee@maximintegrated.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: max98373: Added max98373_reset for stable amp reset | expand |
Hi Ryan! Just some questions inline - in general I like the reset function. On Mon, Nov 26, 2018 at 10:46 AM Ryan Lee <RyanS.Lee@maximintegrated.com> wrote: > > Signed-off-by: Ryan Lee <ryans.lee@maximintegrated.com> > --- > Changes : Created max98373_reset function to minimize code duplication. > Changed regmap_write to regmap_update_bits. Other bits except LSB need to be masked. > Added reset verification step to make sure software reset is completed well. Software reset is done in 10ms in normal case. > Revision ID is available when the amp is in the idle state which means software reset is completed. > Software reset will be performed maximum 3 times to avoid amp reset failure. Generally it is done in the first trial. > sleep time after software reset is increased + 30ms for every retrial. Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times). Why is the sleep time increased after each SW reset? What is the failure case that you've seen which would benefit from this? > > sound/soc/codecs/max98373.c | 41 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c > index a09d013..55af7f02 100644 > --- a/sound/soc/codecs/max98373.c > +++ b/sound/soc/codecs/max98373.c > @@ -724,14 +724,45 @@ static struct snd_soc_dai_driver max98373_dai[] = { > } > }; > > +static void max98373_reset(struct max98373_priv *max98373, struct device *dev) > +{ > + int ret, reg, count, delay; > + > + count = 0; > + while (true) { > + /* Software Reset */ > + ret = regmap_update_bits(max98373->regmap, > + MAX98373_R2000_SW_RESET, > + MAX98373_SOFT_RESET, > + MAX98373_SOFT_RESET); > + if (ret) > + dev_err(dev, "Reset command failed. (ret:%d)\n", ret); > + > + delay = 10000 + (count * 30000); > + usleep_range(delay, delay + 1000); > + > + /* Software Reset Verification */ > + ret = regmap_read(max98373->regmap, > + MAX98373_R21FF_REV_ID, ®); > + if (!ret) { > + dev_info(dev, "Reset completed (retry:%d)\n", count); > + break; Instead of break, can the code return here? "break" implies something else will happen after the while loop exits - there isn't. > + } > + > + if (++count > 3) { > + dev_err(dev, "Reset failed. (ret:%d)\n", ret); > + break; > + } > + usleep_range(10000, 11000); Why is there a second delay after reading MAX98373_R21FF_REV_ID? Is this really necessary? If the second usleep_range() isn't needed, it would be better/clearer to make code loop on "while (count < 4)". And then outside the while loop, use dev_err() to share what the failure was. > + } > +} > + > static int max98373_probe(struct snd_soc_component *component) > { > struct max98373_priv *max98373 = snd_soc_component_get_drvdata(component); > > /* Software Reset */ > - regmap_write(max98373->regmap, > - MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET); > - usleep_range(10000, 11000); > + max98373_reset(max98373, component->dev); > > /* IV default slot configuration */ > regmap_write(max98373->regmap, > @@ -818,9 +849,7 @@ static int max98373_resume(struct device *dev) > { > struct max98373_priv *max98373 = dev_get_drvdata(dev); > > - regmap_write(max98373->regmap, > - MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET); > - usleep_range(10000, 11000); > + max98373_reset(max98373, dev); > regcache_cache_only(max98373->regmap, false); > regcache_sync(max98373->regmap); > return 0; > -- > 2.7.4 > cheers, grant
I just realized I had one more question... On Mon, Nov 26, 2018 at 6:24 PM Grant Grundler <grundler@chromium.org> wrote: > > Hi Ryan! > > Just some questions inline - in general I like the reset function. > > On Mon, Nov 26, 2018 at 10:46 AM Ryan Lee <RyanS.Lee@maximintegrated.com> wrote: > > > > Signed-off-by: Ryan Lee <ryans.lee@maximintegrated.com> > > --- > > Changes : Created max98373_reset function to minimize code duplication. > > Changed regmap_write to regmap_update_bits. Other bits except LSB need to be masked. > > Added reset verification step to make sure software reset is completed well. Software reset is done in 10ms in normal case. > > Revision ID is available when the amp is in the idle state which means software reset is completed. Why not poll the RevID register a few times until it gives a value? Then structure the code to try reset twice (maybe three times). This would avoid the unusual "sleep time after reset is increased" code. cheers, grant > > Software reset will be performed maximum 3 times to avoid amp reset failure. Generally it is done in the first trial. > > sleep time after software reset is increased + 30ms for every retrial. Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times). > > Why is the sleep time increased after each SW reset? > What is the failure case that you've seen which would benefit from this? > > > > > sound/soc/codecs/max98373.c | 41 +++++++++++++++++++++++++++++++++++------ > > 1 file changed, 35 insertions(+), 6 deletions(-) > > > > diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c > > index a09d013..55af7f02 100644 > > --- a/sound/soc/codecs/max98373.c > > +++ b/sound/soc/codecs/max98373.c > > @@ -724,14 +724,45 @@ static struct snd_soc_dai_driver max98373_dai[] = { > > } > > }; > > > > +static void max98373_reset(struct max98373_priv *max98373, struct device *dev) > > +{ > > + int ret, reg, count, delay; > > + > > + count = 0; > > + while (true) { > > + /* Software Reset */ > > + ret = regmap_update_bits(max98373->regmap, > > + MAX98373_R2000_SW_RESET, > > + MAX98373_SOFT_RESET, > > + MAX98373_SOFT_RESET); > > + if (ret) > > + dev_err(dev, "Reset command failed. (ret:%d)\n", ret); > > + > > + delay = 10000 + (count * 30000); > > + usleep_range(delay, delay + 1000); > > + > > + /* Software Reset Verification */ > > + ret = regmap_read(max98373->regmap, > > + MAX98373_R21FF_REV_ID, ®); > > + if (!ret) { > > + dev_info(dev, "Reset completed (retry:%d)\n", count); > > + break; > > Instead of break, can the code return here? > "break" implies something else will happen after the while loop exits > - there isn't. > > > + } > > + > > + if (++count > 3) { > > + dev_err(dev, "Reset failed. (ret:%d)\n", ret); > > + break; > > + } > > + usleep_range(10000, 11000); > > Why is there a second delay after reading MAX98373_R21FF_REV_ID? > Is this really necessary? > > If the second usleep_range() isn't needed, it would be better/clearer > to make code loop on "while (count < 4)". And then outside the while > loop, use dev_err() to share what the failure was. > > > + } > > +} > > + > > static int max98373_probe(struct snd_soc_component *component) > > { > > struct max98373_priv *max98373 = snd_soc_component_get_drvdata(component); > > > > /* Software Reset */ > > - regmap_write(max98373->regmap, > > - MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET); > > - usleep_range(10000, 11000); > > + max98373_reset(max98373, component->dev); > > > > /* IV default slot configuration */ > > regmap_write(max98373->regmap, > > @@ -818,9 +849,7 @@ static int max98373_resume(struct device *dev) > > { > > struct max98373_priv *max98373 = dev_get_drvdata(dev); > > > > - regmap_write(max98373->regmap, > > - MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET); > > - usleep_range(10000, 11000); > > + max98373_reset(max98373, dev); > > regcache_cache_only(max98373->regmap, false); > > regcache_sync(max98373->regmap); > > return 0; > > -- > > 2.7.4 > > > > cheers, > grant
On Mon, Nov 26, 2018 at 06:46:05PM +0000, Ryan Lee wrote: > Signed-off-by: Ryan Lee <ryans.lee@maximintegrated.com> > --- This really needs a changelog to explain what is going on here, and we need some more documentation in the code. It is *extremely* unusual to have to poll for reset like this, and if the failure mode is I/O errors that's going to be pretty painful.
Grant, Thanks for your feedback. Please find my answers inline. >-----Original Message----- >From: Grant Grundler <grundler@chromium.org> >Sent: Monday, November 26, 2018 6:25 PM >To: Ryan Lee <RyanS.Lee@maximintegrated.com> >Cc: Liam Girdwood <lgirdwood@gmail.com>; broonie@kernel.org; >perex@perex.cz; tiwai@suse.com; Grant Grundler ><grundler@chromium.org>; Kuninori Morimoto ><kuninori.morimoto.gx@renesas.com>; Benson Leung ><bleung@chromium.org>; alsa-devel@alsa-project.org; LKML <linux- >kernel@vger.kernel.org> >Subject: Re: [PATCH] ASoC: max98373: Added max98373_reset for stable amp >reset > >EXTERNAL EMAIL > > > >Hi Ryan! > >Just some questions inline - in general I like the reset function. > >On Mon, Nov 26, 2018 at 10:46 AM Ryan Lee ><RyanS.Lee@maximintegrated.com> wrote: >> >> Signed-off-by: Ryan Lee <ryans.lee@maximintegrated.com> >> --- >> Changes : Created max98373_reset function to minimize code duplication. >> Changed regmap_write to regmap_update_bits. Other bits except LSB >need to be masked. >> Added reset verification step to make sure software reset is >completed well. Software reset is done in 10ms in normal case. >> Revision ID is available when the amp is in the idle state which means >software reset is completed. >> Software reset will be performed maximum 3 times to avoid amp reset >failure. Generally it is done in the first trial. >> sleep time after software reset is increased + 30ms for every retrial. >Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times). > >Why is the sleep time increased after each SW reset? >What is the failure case that you've seen which would benefit from this? Generally 10ms is enough time for amp software reset but I wanted to add more guard time for the retrial because it is already failed once. I have not seen a failure issue with 10ms delay on my test setup but I wanted to make it more conservative. Let me remove this. > >> >> sound/soc/codecs/max98373.c | 41 >> +++++++++++++++++++++++++++++++++++------ >> 1 file changed, 35 insertions(+), 6 deletions(-) >> >> diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c >> index a09d013..55af7f02 100644 >> --- a/sound/soc/codecs/max98373.c >> +++ b/sound/soc/codecs/max98373.c >> @@ -724,14 +724,45 @@ static struct snd_soc_dai_driver max98373_dai[] = >{ >> } >> }; >> >> +static void max98373_reset(struct max98373_priv *max98373, struct >> +device *dev) { >> + int ret, reg, count, delay; >> + >> + count = 0; >> + while (true) { >> + /* Software Reset */ >> + ret = regmap_update_bits(max98373->regmap, >> + MAX98373_R2000_SW_RESET, >> + MAX98373_SOFT_RESET, >> + MAX98373_SOFT_RESET); >> + if (ret) >> + dev_err(dev, "Reset command failed. >> + (ret:%d)\n", ret); >> + >> + delay = 10000 + (count * 30000); >> + usleep_range(delay, delay + 1000); >> + >> + /* Software Reset Verification */ >> + ret = regmap_read(max98373->regmap, >> + MAX98373_R21FF_REV_ID, ®); >> + if (!ret) { >> + dev_info(dev, "Reset completed (retry:%d)\n", count); >> + break; > >Instead of break, can the code return here? >"break" implies something else will happen after the while loop exits >- there isn't. Okay. I will fix it. Thanks for the comment. > >> + } >> + >> + if (++count > 3) { >> + dev_err(dev, "Reset failed. (ret:%d)\n", ret); >> + break; >> + } >> + usleep_range(10000, 11000); > >Why is there a second delay after reading MAX98373_R21FF_REV_ID? >Is this really necessary? It is not very necessary. I just wanted to make it more conservative because this delay is applied only when reset is failed. > >If the second usleep_range() isn't needed, it would be better/clearer >to make code loop on "while (count < 4)". And then outside the while >loop, use dev_err() to share what the failure was. OK. Let me fix it. > >> + } >> +} >> + >> static int max98373_probe(struct snd_soc_component *component) { >> struct max98373_priv *max98373 = >> snd_soc_component_get_drvdata(component); >> >> /* Software Reset */ >> - regmap_write(max98373->regmap, >> - MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET); >> - usleep_range(10000, 11000); >> + max98373_reset(max98373, component->dev); >> >> /* IV default slot configuration */ >> regmap_write(max98373->regmap, @@ -818,9 +849,7 @@ static int >> max98373_resume(struct device *dev) { >> struct max98373_priv *max98373 = dev_get_drvdata(dev); >> >> - regmap_write(max98373->regmap, >> - MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET); >> - usleep_range(10000, 11000); >> + max98373_reset(max98373, dev); >> regcache_cache_only(max98373->regmap, false); >> regcache_sync(max98373->regmap); >> return 0; >> -- >> 2.7.4 >> > >cheers, >grant
>-----Original Message----- >From: Grant Grundler <grundler@chromium.org> >Sent: Monday, November 26, 2018 6:28 PM >To: Grant Grundler <grundler@chromium.org> >Cc: Ryan Lee <RyanS.Lee@maximintegrated.com>; Liam Girdwood ><lgirdwood@gmail.com>; broonie@kernel.org; perex@perex.cz; >tiwai@suse.com; Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>; >Benson Leung <bleung@chromium.org>; alsa-devel@alsa-project.org; LKML ><linux-kernel@vger.kernel.org> >Subject: Re: [PATCH] ASoC: max98373: Added max98373_reset for stable amp >reset > >EXTERNAL EMAIL > > > >I just realized I had one more question... > >On Mon, Nov 26, 2018 at 6:24 PM Grant Grundler <grundler@chromium.org> >wrote: >> >> Hi Ryan! >> >> Just some questions inline - in general I like the reset function. >> >> On Mon, Nov 26, 2018 at 10:46 AM Ryan Lee ><RyanS.Lee@maximintegrated.com> wrote: >> > >> > Signed-off-by: Ryan Lee <ryans.lee@maximintegrated.com> >> > --- >> > Changes : Created max98373_reset function to minimize code duplication. >> > Changed regmap_write to regmap_update_bits. Other bits except >LSB need to be masked. >> > Added reset verification step to make sure software reset is >completed well. Software reset is done in 10ms in normal case. >> > Revision ID is available when the amp is in the idle state which means >software reset is completed. > >Why not poll the RevID register a few times until it gives a value? > >Then structure the code to try reset twice (maybe three times). >This would avoid the unusual "sleep time after reset is increased" code. Let me fix unusual things here. Thanks for the comment. > >cheers, >grant > >> > Software reset will be performed maximum 3 times to avoid amp >reset failure. Generally it is done in the first trial. >> > sleep time after software reset is increased + 30ms for every retrial. >Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times). >> >> Why is the sleep time increased after each SW reset? >> What is the failure case that you've seen which would benefit from this? >> >> > >> > sound/soc/codecs/max98373.c | 41 >> > +++++++++++++++++++++++++++++++++++------ >> > 1 file changed, 35 insertions(+), 6 deletions(-) >> > >> > diff --git a/sound/soc/codecs/max98373.c >> > b/sound/soc/codecs/max98373.c index a09d013..55af7f02 100644 >> > --- a/sound/soc/codecs/max98373.c >> > +++ b/sound/soc/codecs/max98373.c >> > @@ -724,14 +724,45 @@ static struct snd_soc_dai_driver max98373_dai[] >= { >> > } >> > }; >> > >> > +static void max98373_reset(struct max98373_priv *max98373, struct >> > +device *dev) { >> > + int ret, reg, count, delay; >> > + >> > + count = 0; >> > + while (true) { >> > + /* Software Reset */ >> > + ret = regmap_update_bits(max98373->regmap, >> > + MAX98373_R2000_SW_RESET, >> > + MAX98373_SOFT_RESET, >> > + MAX98373_SOFT_RESET); >> > + if (ret) >> > + dev_err(dev, "Reset command failed. >> > + (ret:%d)\n", ret); >> > + >> > + delay = 10000 + (count * 30000); >> > + usleep_range(delay, delay + 1000); >> > + >> > + /* Software Reset Verification */ >> > + ret = regmap_read(max98373->regmap, >> > + MAX98373_R21FF_REV_ID, ®); >> > + if (!ret) { >> > + dev_info(dev, "Reset completed (retry:%d)\n", count); >> > + break; >> >> Instead of break, can the code return here? >> "break" implies something else will happen after the while loop exits >> - there isn't. >> >> > + } >> > + >> > + if (++count > 3) { >> > + dev_err(dev, "Reset failed. (ret:%d)\n", ret); >> > + break; >> > + } >> > + usleep_range(10000, 11000); >> >> Why is there a second delay after reading MAX98373_R21FF_REV_ID? >> Is this really necessary? >> >> If the second usleep_range() isn't needed, it would be better/clearer >> to make code loop on "while (count < 4)". And then outside the while >> loop, use dev_err() to share what the failure was. >> >> > + } >> > +} >> > + >> > static int max98373_probe(struct snd_soc_component *component) { >> > struct max98373_priv *max98373 = >> > snd_soc_component_get_drvdata(component); >> > >> > /* Software Reset */ >> > - regmap_write(max98373->regmap, >> > - MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET); >> > - usleep_range(10000, 11000); >> > + max98373_reset(max98373, component->dev); >> > >> > /* IV default slot configuration */ >> > regmap_write(max98373->regmap, @@ -818,9 +849,7 @@ static >> > int max98373_resume(struct device *dev) { >> > struct max98373_priv *max98373 = dev_get_drvdata(dev); >> > >> > - regmap_write(max98373->regmap, >> > - MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET); >> > - usleep_range(10000, 11000); >> > + max98373_reset(max98373, dev); >> > regcache_cache_only(max98373->regmap, false); >> > regcache_sync(max98373->regmap); >> > return 0; >> > -- >> > 2.7.4 >> > >> >> cheers, >> grant
>-----Original Message----- >From: Mark Brown <broonie@kernel.org> >Sent: Tuesday, November 27, 2018 3:51 AM >To: Ryan Lee <RyanS.Lee@maximintegrated.com> >Cc: Liam Girdwood <lgirdwood@gmail.com>; Jaroslav Kysela ><perex@perex.cz>; Takashi Iwai <tiwai@suse.com>; Grant Grundler ><grundler@chromium.org>; Kuninori Morimoto ><kuninori.morimoto.gx@renesas.com>; Benson Leung ><bleung@chromium.org>; alsa-devel@alsa-project.org; linux- >kernel@vger.kernel.org >Subject: Re: [PATCH] ASoC: max98373: Added max98373_reset for stable amp >reset > >On Mon, Nov 26, 2018 at 06:46:05PM +0000, Ryan Lee wrote: >> Signed-off-by: Ryan Lee <ryans.lee@maximintegrated.com> >> --- > >This really needs a changelog to explain what is going on here, and we need >some more documentation in the code. It is *extremely* unusual to have to >poll for reset like this, and if the failure mode is I/O errors that's going to be >pretty painful. OK. I agree that this is very unusual. I wanted to make this code change very conservative and this caused unusual overhead. Let me fix this.
diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c index a09d013..55af7f02 100644 --- a/sound/soc/codecs/max98373.c +++ b/sound/soc/codecs/max98373.c @@ -724,14 +724,45 @@ static struct snd_soc_dai_driver max98373_dai[] = { } }; +static void max98373_reset(struct max98373_priv *max98373, struct device *dev) +{ + int ret, reg, count, delay; + + count = 0; + while (true) { + /* Software Reset */ + ret = regmap_update_bits(max98373->regmap, + MAX98373_R2000_SW_RESET, + MAX98373_SOFT_RESET, + MAX98373_SOFT_RESET); + if (ret) + dev_err(dev, "Reset command failed. (ret:%d)\n", ret); + + delay = 10000 + (count * 30000); + usleep_range(delay, delay + 1000); + + /* Software Reset Verification */ + ret = regmap_read(max98373->regmap, + MAX98373_R21FF_REV_ID, ®); + if (!ret) { + dev_info(dev, "Reset completed (retry:%d)\n", count); + break; + } + + if (++count > 3) { + dev_err(dev, "Reset failed. (ret:%d)\n", ret); + break; + } + usleep_range(10000, 11000); + } +} + static int max98373_probe(struct snd_soc_component *component) { struct max98373_priv *max98373 = snd_soc_component_get_drvdata(component); /* Software Reset */ - regmap_write(max98373->regmap, - MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET); - usleep_range(10000, 11000); + max98373_reset(max98373, component->dev); /* IV default slot configuration */ regmap_write(max98373->regmap, @@ -818,9 +849,7 @@ static int max98373_resume(struct device *dev) { struct max98373_priv *max98373 = dev_get_drvdata(dev); - regmap_write(max98373->regmap, - MAX98373_R2000_SW_RESET, MAX98373_SOFT_RESET); - usleep_range(10000, 11000); + max98373_reset(max98373, dev); regcache_cache_only(max98373->regmap, false); regcache_sync(max98373->regmap); return 0;
Signed-off-by: Ryan Lee <ryans.lee@maximintegrated.com> --- Changes : Created max98373_reset function to minimize code duplication. Changed regmap_write to regmap_update_bits. Other bits except LSB need to be masked. Added reset verification step to make sure software reset is completed well. Software reset is done in 10ms in normal case. Revision ID is available when the amp is in the idle state which means software reset is completed. Software reset will be performed maximum 3 times to avoid amp reset failure. Generally it is done in the first trial. sleep time after software reset is increased + 30ms for every retrial. Maximum possible msleep time is 100 ms (initial 10 ms + 30 ms * 3 times). sound/soc/codecs/max98373.c | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-)