| Message ID | 1421756219-21037-10-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Marek, On 01/20/2015 01:16 PM, Marek Szyprowski wrote: > Mixed need to have hdmi clock enabled to properly perform power on/off > sequences, so add handling of this clock directly to the mixer driver. > Dependency between hdmi clock and mixer module has been observed on > Exynos4 based boards. > > Suggested-by: Andrzej Hajda <a.hajda@samsung.com> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/gpu/drm/exynos/exynos_mixer.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c > index 820b76234ef4..e5ef1fccd8fb 100644 > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c > @@ -72,6 +72,7 @@ struct mixer_resources { > spinlock_t reg_slock; > struct clk *mixer; > struct clk *vp; > + struct clk *hdmi; > struct clk *sclk_mixer; > struct clk *sclk_hdmi; > struct clk *mout_mixer; > @@ -774,6 +775,12 @@ static int mixer_resources_init(struct mixer_context *mixer_ctx) > return -ENODEV; > } > > + mixer_res->hdmi = devm_clk_get(dev, "hdmi"); You need to update the Documentation/devicetree/bindings/video/exynos_mixer.txt DT binding docs to also mention the "hdmi" clock in the list of clocks. But as I mentioned in "[PATCH v2 0/6] Enable HDMI support on Exynos platforms" thread, while this seems to be enough to prevent the issue on Exynos4 is not enough on the Exynos5420/5422/5800 boards I've tested. So I wonder if $subject is fixing the root cause or just fixing a symptom and the cause is that the exynos_hdmi DPMS handler has to be executed before the exynos_mixer DPMS handler for DRM_MODE_DPMS_ON like is the case for DPMS_OFF after commit 245f98f269714 ("drm/exynos: hdmi: fix power order issue"). Best regards, Javier
Hello, On 2015-01-20 13:52, Javier Martinez Canillas wrote: > On 01/20/2015 01:16 PM, Marek Szyprowski wrote: >> Mixed need to have hdmi clock enabled to properly perform power on/off >> sequences, so add handling of this clock directly to the mixer driver. >> Dependency between hdmi clock and mixer module has been observed on >> Exynos4 based boards. >> >> Suggested-by: Andrzej Hajda<a.hajda@samsung.com> >> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com> >> --- >> drivers/gpu/drm/exynos/exynos_mixer.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c >> index 820b76234ef4..e5ef1fccd8fb 100644 >> --- a/drivers/gpu/drm/exynos/exynos_mixer.c >> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c >> @@ -72,6 +72,7 @@ struct mixer_resources { >> spinlock_t reg_slock; >> struct clk *mixer; >> struct clk *vp; >> + struct clk *hdmi; >> struct clk *sclk_mixer; >> struct clk *sclk_hdmi; >> struct clk *mout_mixer; >> @@ -774,6 +775,12 @@ static int mixer_resources_init(struct mixer_context *mixer_ctx) >> return -ENODEV; >> } >> >> + mixer_res->hdmi = devm_clk_get(dev, "hdmi"); > You need to update the Documentation/devicetree/bindings/video/exynos_mixer.txt > DT binding docs to also mention the "hdmi" clock in the list of clocks. Right, I've send an updated version of the patch. > But as I mentioned in "[PATCH v2 0/6] Enable HDMI support on Exynos platforms" > thread, while this seems to be enough to prevent the issue on Exynos4 is not > enough on the Exynos5420/5422/5800 boards I've tested. > > So I wonder if $subject is fixing the root cause or just fixing a symptom and > the cause is that the exynos_hdmi DPMS handler has to be executed before the > exynos_mixer DPMS handler for DRM_MODE_DPMS_ON like is the case for DPMS_OFF > after commit 245f98f269714 ("drm/exynos: hdmi: fix power order issue"). I'm aware of the issues with Exynos542x, I've tested it with Odroid XU3, but I really have no idea how to fix it. The reference manual (both for power domain and mixer/hdmi modules) also doesn't provide any useful information for this case. The issue with power on/off sequence definitely IS related to clock configuration, but we didn't figure out how to solve it in a generic way. This will be handled in Exynos HDMI and mixer drivers anyway, so the DTS part (at least for Exynos4 SoC) will not change. We would really like to have HDMI support for Exynos4 merged, especially that the first version of the HDMI patches was posted in v3.16 times and now we are close to v3.20 -next merge window end... Best regards
Hello Marek, On 01/22/2015 01:41 PM, Marek Szyprowski wrote: >>> >>> + mixer_res->hdmi = devm_clk_get(dev, "hdmi"); >> You need to update the Documentation/devicetree/bindings/video/exynos_mixer.txt >> DT binding docs to also mention the "hdmi" clock in the list of clocks. > > Right, I've send an updated version of the patch. > Great thanks. >> But as I mentioned in "[PATCH v2 0/6] Enable HDMI support on Exynos platforms" >> thread, while this seems to be enough to prevent the issue on Exynos4 is not >> enough on the Exynos5420/5422/5800 boards I've tested. >> >> So I wonder if $subject is fixing the root cause or just fixing a symptom and >> the cause is that the exynos_hdmi DPMS handler has to be executed before the >> exynos_mixer DPMS handler for DRM_MODE_DPMS_ON like is the case for DPMS_OFF >> after commit 245f98f269714 ("drm/exynos: hdmi: fix power order issue"). > > I'm aware of the issues with Exynos542x, I've tested it with Odroid XU3, > but I > really have no idea how to fix it. The reference manual (both for power > domain > and mixer/hdmi modules) also doesn't provide any useful information for this > case. > Yeah, I'm in the same situation. All the documentation I had access to doesn't now explain what's happening. > The issue with power on/off sequence definitely IS related to clock > configuration, > but we didn't figure out how to solve it in a generic way. This will be > handled in > Exynos HDMI and mixer drivers anyway, so the DTS part (at least for > Exynos4 SoC) > will not change. > > We would really like to have HDMI support for Exynos4 merged, especially > that the > first version of the HDMI patches was posted in v3.16 times and now we > are close > to v3.20 -next merge window end... > Sorry, I didn't mean to imply that $subject should be blocked. I agree with you that the power on/off sequence has to be fixed in the hdmi and mixer drivers and is orthogonal to the DTS changes. That's why I also decided to finally post my "Add HDMI support for Exynos5420 platform" [0] series too. > Best regards > Best regards, Javier [0]: https://lkml.org/lkml/2015/1/20/235
On Tue, Jan 20, 2015 at 10:16 AM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > + mixer_res->hdmi = devm_clk_get(dev, "hdmi"); > + if (IS_ERR(mixer_res->hdmi)) { > + dev_err(dev, "failed to get clock 'hdmi'\n"); > + return -ENODEV; 'return PTR_ERR(mixer_res->hdmi);' would be better. > + } > + > mixer_res->sclk_hdmi = devm_clk_get(dev, "sclk_hdmi"); > if (IS_ERR(mixer_res->sclk_hdmi)) { > dev_err(dev, "failed to get clock 'sclk_hdmi'\n"); > @@ -1095,6 +1102,7 @@ static void mixer_poweron(struct exynos_drm_manager *mgr) > pm_runtime_get_sync(ctx->dev); > > clk_prepare_enable(res->mixer); > + clk_prepare_enable(res->hdmi); Better check whether clk_prepare_enable failed.
Hello Marek, On 01/22/2015 01:28 PM, Marek Szyprowski wrote: > Mixed need to have hdmi clock enabled to properly perform power on/off > sequences, so add handling of this clock directly to the mixer driver. > Dependency between hdmi clock and mixer module has been observed on > Exynos4 based boards. > > Suggested-by: Andrzej Hajda <a.hajda@samsung.com> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- The patch looks good to me and I tested that it does not regress HDMI on other platforms (Exynos5420 Peach Pit). I've just a comment below. Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> Tested-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > Documentation/devicetree/bindings/video/exynos_mixer.txt | 1 + > drivers/gpu/drm/exynos/exynos_mixer.c | 9 +++++++++ > 2 files changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/video/exynos_mixer.txt b/Documentation/devicetree/bindings/video/exynos_mixer.txt > index 08b394b..3e38128 100644 > --- a/Documentation/devicetree/bindings/video/exynos_mixer.txt > +++ b/Documentation/devicetree/bindings/video/exynos_mixer.txt > @@ -15,6 +15,7 @@ Required properties: > a) mixer: Gate of Mixer IP bus clock. > b) sclk_hdmi: HDMI Special clock, one of the two possible inputs of > mixer mux. > + c) hdmi: Gate of HDMI IP bus clock, needed together with sclk_hdmi. > You are adding as a required property which means that this breaks DT backward compatibility. I guess is not a big issue here since HDMI seems to have been broken in mainline on most Exynos platforms anyways. Best regards, Javier
Hello, On 2015-01-22 13:51, Javier Martinez Canillas wrote: > Hello Marek, > > On 01/22/2015 01:41 PM, Marek Szyprowski wrote: >>>> >>>> + mixer_res->hdmi = devm_clk_get(dev, "hdmi"); >>> You need to update the Documentation/devicetree/bindings/video/exynos_mixer.txt >>> DT binding docs to also mention the "hdmi" clock in the list of clocks. >> Right, I've send an updated version of the patch. >> > Great thanks. > >>> But as I mentioned in "[PATCH v2 0/6] Enable HDMI support on Exynos platforms" >>> thread, while this seems to be enough to prevent the issue on Exynos4 is not >>> enough on the Exynos5420/5422/5800 boards I've tested. >>> >>> So I wonder if $subject is fixing the root cause or just fixing a symptom and >>> the cause is that the exynos_hdmi DPMS handler has to be executed before the >>> exynos_mixer DPMS handler for DRM_MODE_DPMS_ON like is the case for DPMS_OFF >>> after commit 245f98f269714 ("drm/exynos: hdmi: fix power order issue"). >> I'm aware of the issues with Exynos542x, I've tested it with Odroid XU3, >> but I >> really have no idea how to fix it. The reference manual (both for power >> domain >> and mixer/hdmi modules) also doesn't provide any useful information for this >> case. >> > Yeah, I'm in the same situation. All the documentation I had access to doesn't > now explain what's happening. Just to let you know. The problem with power domain failure to turn off is something orthogonal to 'Unhandled fault: external abort on non-linefetch' issue in drm mixer driver. It looks that even if domains reports that it failed to turn off, it somehow disabled the power, because this 'external abort' issue happens on Odroid XU3 when driver tries to access mixer registers with power domain turned off. It must be something badly broken in Exynos DRM HDMI/Mixer handling of runtime pm, because such scenario can be easily triggered simply by running "libdrm/modetest -M exynos -s 16@13:1920x1080". It looks that in case of Exynos4 access to mixer registers in case of disabled power domain doesn't have such terrible results and thus the driver is somehow working well. >> The issue with power on/off sequence definitely IS related to clock >> configuration, >> but we didn't figure out how to solve it in a generic way. This will be >> handled in >> Exynos HDMI and mixer drivers anyway, so the DTS part (at least for >> Exynos4 SoC) >> will not change. >> >> We would really like to have HDMI support for Exynos4 merged, especially >> that the >> first version of the HDMI patches was posted in v3.16 times and now we >> are close >> to v3.20 -next merge window end... >> > Sorry, I didn't mean to imply that $subject should be blocked. I agree with > you that the power on/off sequence has to be fixed in the hdmi and mixer > drivers and is orthogonal to the DTS changes. That's why I also decided to > finally post my "Add HDMI support for Exynos5420 platform" [0] series too. Thanks. I hope that both series will get merged to v3.20 what will bring more attention to this problem. Best regards
Hello, On 2015-01-22 14:00, Fabio Estevam wrote: > On Tue, Jan 20, 2015 at 10:16 AM, Marek Szyprowski > <m.szyprowski@samsung.com> wrote: > >> + mixer_res->hdmi = devm_clk_get(dev, "hdmi"); >> + if (IS_ERR(mixer_res->hdmi)) { >> + dev_err(dev, "failed to get clock 'hdmi'\n"); >> + return -ENODEV; > 'return PTR_ERR(mixer_res->hdmi);' would be better. ok. > >> + } >> + >> mixer_res->sclk_hdmi = devm_clk_get(dev, "sclk_hdmi"); >> if (IS_ERR(mixer_res->sclk_hdmi)) { >> dev_err(dev, "failed to get clock 'sclk_hdmi'\n"); >> @@ -1095,6 +1102,7 @@ static void mixer_poweron(struct exynos_drm_manager *mgr) >> pm_runtime_get_sync(ctx->dev); >> >> clk_prepare_enable(res->mixer); >> + clk_prepare_enable(res->hdmi); > Better check whether clk_prepare_enable failed. This will be handled by a separate patch, because checking for error values from clk_repare_enable is missing almost all over the Exynos DRM driver. Best regards
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index 820b76234ef4..e5ef1fccd8fb 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -72,6 +72,7 @@ struct mixer_resources { spinlock_t reg_slock; struct clk *mixer; struct clk *vp; + struct clk *hdmi; struct clk *sclk_mixer; struct clk *sclk_hdmi; struct clk *mout_mixer; @@ -774,6 +775,12 @@ static int mixer_resources_init(struct mixer_context *mixer_ctx) return -ENODEV; } + mixer_res->hdmi = devm_clk_get(dev, "hdmi"); + if (IS_ERR(mixer_res->hdmi)) { + dev_err(dev, "failed to get clock 'hdmi'\n"); + return -ENODEV; + } + mixer_res->sclk_hdmi = devm_clk_get(dev, "sclk_hdmi"); if (IS_ERR(mixer_res->sclk_hdmi)) { dev_err(dev, "failed to get clock 'sclk_hdmi'\n"); @@ -1095,6 +1102,7 @@ static void mixer_poweron(struct exynos_drm_manager *mgr) pm_runtime_get_sync(ctx->dev); clk_prepare_enable(res->mixer); + clk_prepare_enable(res->hdmi); if (ctx->vp_enabled) { clk_prepare_enable(res->vp); if (ctx->has_sclk) @@ -1134,6 +1142,7 @@ static void mixer_poweroff(struct exynos_drm_manager *mgr) ctx->powered = false; mutex_unlock(&ctx->mixer_mutex); + clk_disable_unprepare(res->hdmi); clk_disable_unprepare(res->mixer); if (ctx->vp_enabled) { clk_disable_unprepare(res->vp);
Mixed need to have hdmi clock enabled to properly perform power on/off sequences, so add handling of this clock directly to the mixer driver. Dependency between hdmi clock and mixer module has been observed on Exynos4 based boards. Suggested-by: Andrzej Hajda <a.hajda@samsung.com> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/gpu/drm/exynos/exynos_mixer.c | 9 +++++++++ 1 file changed, 9 insertions(+)