| Message ID | 20190930103840.18970-8-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: misc improvements | expand |
Hi Tomi, Thank you for the patch. On Mon, Sep 30, 2019 at 01:38:40PM +0300, Tomi Valkeinen wrote: > If use_mclk is false, mclk_mode is written to a register without > initialization. This doesn't cause any ill effects as the written value > is not used when use_mclk is false. > > To fix this, write use_mclk only when use_mclk is true. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/omapdrm/dss/hdmi4_core.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c > index 5d5d5588ebc1..c4ffe96e28bc 100644 > --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c > +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c > @@ -542,8 +542,9 @@ static void hdmi_core_audio_config(struct hdmi_core_data *core, > } > > /* Set ACR clock divisor */ > - REG_FLD_MOD(av_base, > - HDMI_CORE_AV_FREQ_SVAL, cfg->mclk_mode, 2, 0); > + if (cfg->use_mclk) > + REG_FLD_MOD(av_base, HDMI_CORE_AV_FREQ_SVAL, > + cfg->mclk_mode, 2, 0); > > r = hdmi_read_reg(av_base, HDMI_CORE_AV_ACR_CTRL); > /*
* Tomi Valkeinen <tomi.valkeinen@ti.com> [190930 10:38]: > If use_mclk is false, mclk_mode is written to a register without > initialization. This doesn't cause any ill effects as the written value > is not used when use_mclk is false. > > To fix this, write use_mclk only when use_mclk is true. Hey nice catch. Based on a quick test looks like this fixes an issue where power consumption stays higher after using HDMI. Would be nice to have merged in the v5.4-rc series: Tested-by: Tony Lindgren <tony@atomide.com>
On 08/10/2019 17:13, Tony Lindgren wrote: > * Tomi Valkeinen <tomi.valkeinen@ti.com> [190930 10:38]: >> If use_mclk is false, mclk_mode is written to a register without >> initialization. This doesn't cause any ill effects as the written value >> is not used when use_mclk is false. >> >> To fix this, write use_mclk only when use_mclk is true. > > Hey nice catch. Based on a quick test looks like this fixes an > issue where power consumption stays higher after using HDMI. > > Would be nice to have merged in the v5.4-rc series: > > Tested-by: Tony Lindgren <tony@atomide.com> Really? Ok, well, then it was a good random find =). I did already push this to drm-misc-next, as I thought it does not have any real effect. I'll check if it's ok to push to drm-misc-fixes too, with Cc stable. Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [191008 14:17]: > On 08/10/2019 17:13, Tony Lindgren wrote: > > * Tomi Valkeinen <tomi.valkeinen@ti.com> [190930 10:38]: > > > If use_mclk is false, mclk_mode is written to a register without > > > initialization. This doesn't cause any ill effects as the written value > > > is not used when use_mclk is false. > > > > > > To fix this, write use_mclk only when use_mclk is true. > > > > Hey nice catch. Based on a quick test looks like this fixes an > > issue where power consumption stays higher after using HDMI. > > > > Would be nice to have merged in the v5.4-rc series: > > > > Tested-by: Tony Lindgren <tony@atomide.com> > > Really? Ok, well, then it was a good random find =). Yeah so it seems :) Earlier I thought there's still some clkctrl setting wrong after using HDMI, but did not see anything diffing the clkctrl registers before and after and gave up. > I did already push this to drm-misc-next, as I thought it does not have any > real effect. I'll check if it's ok to push to drm-misc-fixes too, with Cc > stable. OK great thanks. Tony
On 08/10/2019 17:21, Tony Lindgren wrote: > * Tomi Valkeinen <tomi.valkeinen@ti.com> [191008 14:17]: >> On 08/10/2019 17:13, Tony Lindgren wrote: >>> * Tomi Valkeinen <tomi.valkeinen@ti.com> [190930 10:38]: >>>> If use_mclk is false, mclk_mode is written to a register without >>>> initialization. This doesn't cause any ill effects as the written value >>>> is not used when use_mclk is false. >>>> >>>> To fix this, write use_mclk only when use_mclk is true. >>> >>> Hey nice catch. Based on a quick test looks like this fixes an >>> issue where power consumption stays higher after using HDMI. >>> >>> Would be nice to have merged in the v5.4-rc series: >>> >>> Tested-by: Tony Lindgren <tony@atomide.com> >> >> Really? Ok, well, then it was a good random find =). > > Yeah so it seems :) Earlier I thought there's still some > clkctrl setting wrong after using HDMI, but did not see > anything diffing the clkctrl registers before and after > and gave up. > >> I did already push this to drm-misc-next, as I thought it does not have any >> real effect. I'll check if it's ok to push to drm-misc-fixes too, with Cc >> stable. > > OK great thanks. Pushing this to fixes too would cause conflicts, so we shouldn't push without good reason. How much power saving you see? I think this can still be sent to stable later, after it has been merged to mainline. Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [191010 06:48]: > On 08/10/2019 17:21, Tony Lindgren wrote: > > * Tomi Valkeinen <tomi.valkeinen@ti.com> [191008 14:17]: > > > On 08/10/2019 17:13, Tony Lindgren wrote: > > > > * Tomi Valkeinen <tomi.valkeinen@ti.com> [190930 10:38]: > > > > > If use_mclk is false, mclk_mode is written to a register without > > > > > initialization. This doesn't cause any ill effects as the written value > > > > > is not used when use_mclk is false. > > > > > > > > > > To fix this, write use_mclk only when use_mclk is true. > > > > > > > > Hey nice catch. Based on a quick test looks like this fixes an > > > > issue where power consumption stays higher after using HDMI. > > > > > > > > Would be nice to have merged in the v5.4-rc series: > > > > > > > > Tested-by: Tony Lindgren <tony@atomide.com> > > > > > > Really? Ok, well, then it was a good random find =). > > > > Yeah so it seems :) Earlier I thought there's still some > > clkctrl setting wrong after using HDMI, but did not see > > anything diffing the clkctrl registers before and after > > and gave up. > > > > > I did already push this to drm-misc-next, as I thought it does not have any > > > real effect. I'll check if it's ok to push to drm-misc-fixes too, with Cc > > > stable. > > > > OK great thanks. > > Pushing this to fixes too would cause conflicts, so we shouldn't push > without good reason. How much power saving you see? Sure no rush with this one. I should also test again that it really fixes the issue I'm seeing. Hmm so what register does this clock actually change? I'm seeing an increase of few tens of extra mW, which means at least one day of standby time less for me :) It does not happen always, maybe half of the time. > I think this can still be sent to stable later, after it has been merged to > mainline. Yes sure. Thanks, Tony
On 10/10/2019 16:24, Tony Lindgren wrote: > Hmm so what register does this clock actually change? > > I'm seeing an increase of few tens of extra mW, which means at > least one day of standby time less for me :) It does not happen > always, maybe half of the time. I have no idea why this would affect power consumption. As far as I can understand, the bits written here are a clk divider MCLK. I don't see why that would affect. Maybe Jyri or Peter has an idea, I have never looked at the HDMI audio side. Tomi
* Tomi Valkeinen <tomi.valkeinen@ti.com> [191011 10:25]: > On 10/10/2019 16:24, Tony Lindgren wrote: > > > Hmm so what register does this clock actually change? > > > > I'm seeing an increase of few tens of extra mW, which means at > > least one day of standby time less for me :) It does not happen > > always, maybe half of the time. > > I have no idea why this would affect power consumption. As far as I can > understand, the bits written here are a clk divider MCLK. I don't see why > that would affect. Yeah you're right, and I just got lucky initially. I have seen the power consumption stay higher already with the patch applied. The clocks seem just fine. > Maybe Jyri or Peter has an idea, I have never looked at the HDMI audio side. I'll try dumping out the hdmi registers before and after when I get a chance. Regards, Tony
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c b/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c index 5d5d5588ebc1..c4ffe96e28bc 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4_core.c @@ -542,8 +542,9 @@ static void hdmi_core_audio_config(struct hdmi_core_data *core, } /* Set ACR clock divisor */ - REG_FLD_MOD(av_base, - HDMI_CORE_AV_FREQ_SVAL, cfg->mclk_mode, 2, 0); + if (cfg->use_mclk) + REG_FLD_MOD(av_base, HDMI_CORE_AV_FREQ_SVAL, + cfg->mclk_mode, 2, 0); r = hdmi_read_reg(av_base, HDMI_CORE_AV_ACR_CTRL); /*
If use_mclk is false, mclk_mode is written to a register without initialization. This doesn't cause any ill effects as the written value is not used when use_mclk is false. To fix this, write use_mclk only when use_mclk is true. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/dss/hdmi4_core.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)