| Message ID | 20190502223808.185180-1-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm: bridge: dw-hdmi: Add hooks for suspend/resume | expand |
Hi From: Douglas Anderson <dianders@chromium.org> Date: Thu, May 2, 2019 at 3:38 PM To: Heiko Stuebner, Sandy Huang, Andrzej Hajda, Laurent Pinchart Cc: <linux-rockchip@lists.infradead.org>, Neil Armstrong, <mka@chromium.org>, Sean Paul, Douglas Anderson, Zheng Yang, Sam Ravnborg, <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>, Ville Syrjälä, David Airlie, Jernej Skrabec, Daniel Vetter > On Rockchip rk3288-based Chromebooks when you do a suspend/resume > cycle: > > 1. You lose the ability to detect an HDMI device being plugged in. > > 2. If you're using the i2c bus built in to dw_hdmi then it stops > working. > > Let's add a hook to the core dw-hdmi driver so that we can call it in > dw_hdmi-rockchip in the next commit. > > NOTE: the exact set of steps I've done here in resume come from > looking at the normal dw_hdmi init sequence in upstream Linux plus the > sequence that we did in downstream Chrome OS 3.14. Testing show that > it seems to work, but if an extra step is needed or something here is > not needed we could improve it. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++ > include/drm/bridge/dw_hdmi.h | 3 +++ > 2 files changed, 24 insertions(+) Did anyone have any thoughts on this patch series? Thanks! :-) -Doug
On Thu, May 02, 2019 at 03:38:07PM -0700, Douglas Anderson wrote: > On Rockchip rk3288-based Chromebooks when you do a suspend/resume > cycle: > > 1. You lose the ability to detect an HDMI device being plugged in. > > 2. If you're using the i2c bus built in to dw_hdmi then it stops > working. > > Let's add a hook to the core dw-hdmi driver so that we can call it in > dw_hdmi-rockchip in the next commit. > > NOTE: the exact set of steps I've done here in resume come from > looking at the normal dw_hdmi init sequence in upstream Linux plus the > sequence that we did in downstream Chrome OS 3.14. Testing show that > it seems to work, but if an extra step is needed or something here is > not needed we could improve it. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++ > include/drm/bridge/dw_hdmi.h | 3 +++ > 2 files changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index db761329a1e3..4b38bfd43e59 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -2780,6 +2780,27 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi) > } > EXPORT_SYMBOL_GPL(dw_hdmi_unbind); > > +int dw_hdmi_suspend(struct dw_hdmi *hdmi) > +{ > + return 0; > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_suspend); > + > +int dw_hdmi_resume(struct dw_hdmi *hdmi) > +{ > + initialize_hdmi_ih_mutes(hdmi); > + > + dw_hdmi_setup_i2c(hdmi); > + if (hdmi->i2c) > + dw_hdmi_i2c_init(hdmi); > + > + if (hdmi->phy.ops->setup_hpd) > + hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_resume); Both patches look good to me, I'd probably prefer to just smash them together, but meh. If no one more authoritative chimes in, I'll apply them to -misc in a few days. Sean > + > MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>"); > MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>"); > MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>"); > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index 66e70770cce5..c4132e9a5ae3 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -154,6 +154,9 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev, > struct drm_encoder *encoder, > const struct dw_hdmi_plat_data *plat_data); > > +int dw_hdmi_suspend(struct dw_hdmi *hdmi); > +int dw_hdmi_resume(struct dw_hdmi *hdmi); > + > void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense); > > void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate); > -- > 2.21.0.1020.gf2820cf01a-goog >
Hi, On Wed, May 15, 2019 at 10:58 AM Sean Paul <sean@poorly.run> wrote: > On Thu, May 02, 2019 at 03:38:07PM -0700, Douglas Anderson wrote: > > On Rockchip rk3288-based Chromebooks when you do a suspend/resume > > cycle: > > > > 1. You lose the ability to detect an HDMI device being plugged in. > > > > 2. If you're using the i2c bus built in to dw_hdmi then it stops > > working. > > > > Let's add a hook to the core dw-hdmi driver so that we can call it in > > dw_hdmi-rockchip in the next commit. > > > > NOTE: the exact set of steps I've done here in resume come from > > looking at the normal dw_hdmi init sequence in upstream Linux plus the > > sequence that we did in downstream Chrome OS 3.14. Testing show that > > it seems to work, but if an extra step is needed or something here is > > not needed we could improve it. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++ > > include/drm/bridge/dw_hdmi.h | 3 +++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > index db761329a1e3..4b38bfd43e59 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > @@ -2780,6 +2780,27 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi) > > } > > EXPORT_SYMBOL_GPL(dw_hdmi_unbind); > > > > +int dw_hdmi_suspend(struct dw_hdmi *hdmi) > > +{ > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(dw_hdmi_suspend); > > + > > +int dw_hdmi_resume(struct dw_hdmi *hdmi) > > +{ > > + initialize_hdmi_ih_mutes(hdmi); > > + > > + dw_hdmi_setup_i2c(hdmi); > > + if (hdmi->i2c) > > + dw_hdmi_i2c_init(hdmi); > > + > > + if (hdmi->phy.ops->setup_hpd) > > + hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(dw_hdmi_resume); > > Both patches look good to me, I'd probably prefer to just smash them together, > but meh. > > If no one more authoritative chimes in, I'll apply them to -misc in a few days. Sure. I can smash them and re-post or you can smash them for me or we can keep them as-is. I originally separated because I wasn't sure if they'd eventually go through different trees. Just let me know! :-) -Doug
On Wed, May 15, 2019 at 11:01:26AM -0700, Doug Anderson wrote: > Hi, > > On Wed, May 15, 2019 at 10:58 AM Sean Paul <sean@poorly.run> wrote: > > > On Thu, May 02, 2019 at 03:38:07PM -0700, Douglas Anderson wrote: > > > On Rockchip rk3288-based Chromebooks when you do a suspend/resume > > > cycle: > > > > > > 1. You lose the ability to detect an HDMI device being plugged in. > > > > > > 2. If you're using the i2c bus built in to dw_hdmi then it stops > > > working. > > > > > > Let's add a hook to the core dw-hdmi driver so that we can call it in > > > dw_hdmi-rockchip in the next commit. > > > > > > NOTE: the exact set of steps I've done here in resume come from > > > looking at the normal dw_hdmi init sequence in upstream Linux plus the > > > sequence that we did in downstream Chrome OS 3.14. Testing show that > > > it seems to work, but if an extra step is needed or something here is > > > not needed we could improve it. > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > --- > > > > > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++ > > > include/drm/bridge/dw_hdmi.h | 3 +++ > > > 2 files changed, 24 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > > index db761329a1e3..4b38bfd43e59 100644 > > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > > @@ -2780,6 +2780,27 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi) > > > } > > > EXPORT_SYMBOL_GPL(dw_hdmi_unbind); > > > > > > +int dw_hdmi_suspend(struct dw_hdmi *hdmi) > > > +{ > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(dw_hdmi_suspend); > > > + > > > +int dw_hdmi_resume(struct dw_hdmi *hdmi) > > > +{ > > > + initialize_hdmi_ih_mutes(hdmi); > > > + > > > + dw_hdmi_setup_i2c(hdmi); > > > + if (hdmi->i2c) > > > + dw_hdmi_i2c_init(hdmi); > > > + > > > + if (hdmi->phy.ops->setup_hpd) > > > + hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(dw_hdmi_resume); > > > > Both patches look good to me, I'd probably prefer to just smash them together, > > but meh. > > > > If no one more authoritative chimes in, I'll apply them to -misc in a few days. > > Sure. I can smash them and re-post or you can smash them for me or we > can keep them as-is. I originally separated because I wasn't sure if > they'd eventually go through different trees. Just let me know! :-) Definitely no need to repost. It's entirely possible Andrzej or Heiko prefer to have the dw-hdmi stuff broken out anyways. My opinion is of little value here :) Sean > > -Doug
Am Mittwoch, 15. Mai 2019, 20:05:03 CEST schrieb Sean Paul: > On Wed, May 15, 2019 at 11:01:26AM -0700, Doug Anderson wrote: > > Hi, > > > > On Wed, May 15, 2019 at 10:58 AM Sean Paul <sean@poorly.run> wrote: > > > > > On Thu, May 02, 2019 at 03:38:07PM -0700, Douglas Anderson wrote: > > > > On Rockchip rk3288-based Chromebooks when you do a suspend/resume > > > > cycle: > > > > > > > > 1. You lose the ability to detect an HDMI device being plugged in. > > > > > > > > 2. If you're using the i2c bus built in to dw_hdmi then it stops > > > > working. > > > > > > > > Let's add a hook to the core dw-hdmi driver so that we can call it in > > > > dw_hdmi-rockchip in the next commit. > > > > > > > > NOTE: the exact set of steps I've done here in resume come from > > > > looking at the normal dw_hdmi init sequence in upstream Linux plus the > > > > sequence that we did in downstream Chrome OS 3.14. Testing show that > > > > it seems to work, but if an extra step is needed or something here is > > > > not needed we could improve it. > > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > --- > > > > > > > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++ > > > > include/drm/bridge/dw_hdmi.h | 3 +++ > > > > 2 files changed, 24 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > > > index db761329a1e3..4b38bfd43e59 100644 > > > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > > > @@ -2780,6 +2780,27 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi) > > > > } > > > > EXPORT_SYMBOL_GPL(dw_hdmi_unbind); > > > > > > > > +int dw_hdmi_suspend(struct dw_hdmi *hdmi) > > > > +{ > > > > + return 0; > > > > +} > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_suspend); > > > > + > > > > +int dw_hdmi_resume(struct dw_hdmi *hdmi) > > > > +{ > > > > + initialize_hdmi_ih_mutes(hdmi); > > > > + > > > > + dw_hdmi_setup_i2c(hdmi); > > > > + if (hdmi->i2c) > > > > + dw_hdmi_i2c_init(hdmi); > > > > + > > > > + if (hdmi->phy.ops->setup_hpd) > > > > + hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); > > > > + > > > > + return 0; > > > > +} > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_resume); > > > > > > Both patches look good to me, I'd probably prefer to just smash them together, > > > but meh. > > > > > > If no one more authoritative chimes in, I'll apply them to -misc in a few days. > > > > Sure. I can smash them and re-post or you can smash them for me or we > > can keep them as-is. I originally separated because I wasn't sure if > > they'd eventually go through different trees. Just let me know! :-) > > Definitely no need to repost. It's entirely possible Andrzej or Heiko prefer to > have the dw-hdmi stuff broken out anyways. My opinion is of little value here :) I guess my own preference is to keep them as they are now - so separate. It makes it easier to see what gets done and also keeps the boundary on where to split pretty clear. Heiko
Hi Douglas, Thank you for the patch. On Thu, May 02, 2019 at 03:38:07PM -0700, Douglas Anderson wrote: > On Rockchip rk3288-based Chromebooks when you do a suspend/resume > cycle: > > 1. You lose the ability to detect an HDMI device being plugged in. > > 2. If you're using the i2c bus built in to dw_hdmi then it stops > working. > > Let's add a hook to the core dw-hdmi driver so that we can call it in > dw_hdmi-rockchip in the next commit. > > NOTE: the exact set of steps I've done here in resume come from > looking at the normal dw_hdmi init sequence in upstream Linux plus the > sequence that we did in downstream Chrome OS 3.14. Testing show that > it seems to work, but if an extra step is needed or something here is > not needed we could improve it. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++ > include/drm/bridge/dw_hdmi.h | 3 +++ > 2 files changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index db761329a1e3..4b38bfd43e59 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -2780,6 +2780,27 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi) > } > EXPORT_SYMBOL_GPL(dw_hdmi_unbind); > > +int dw_hdmi_suspend(struct dw_hdmi *hdmi) > +{ > + return 0; > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_suspend); > + As this is empty, should we leave it out ? It adds a bit of bloat to the kernel for no real reason, and we can add it later if required. > +int dw_hdmi_resume(struct dw_hdmi *hdmi) > +{ > + initialize_hdmi_ih_mutes(hdmi); > + > + dw_hdmi_setup_i2c(hdmi); > + if (hdmi->i2c) > + dw_hdmi_i2c_init(hdmi); > + > + if (hdmi->phy.ops->setup_hpd) > + hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); > + > + return 0; How about refactoring the probe function to extract hardware initialisation to a separate function, and calling it from here ? > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_resume); > + > MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>"); > MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>"); > MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>"); > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index 66e70770cce5..c4132e9a5ae3 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -154,6 +154,9 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev, > struct drm_encoder *encoder, > const struct dw_hdmi_plat_data *plat_data); > > +int dw_hdmi_suspend(struct dw_hdmi *hdmi); > +int dw_hdmi_resume(struct dw_hdmi *hdmi); > + > void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense); > > void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index db761329a1e3..4b38bfd43e59 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2780,6 +2780,27 @@ void dw_hdmi_unbind(struct dw_hdmi *hdmi) } EXPORT_SYMBOL_GPL(dw_hdmi_unbind); +int dw_hdmi_suspend(struct dw_hdmi *hdmi) +{ + return 0; +} +EXPORT_SYMBOL_GPL(dw_hdmi_suspend); + +int dw_hdmi_resume(struct dw_hdmi *hdmi) +{ + initialize_hdmi_ih_mutes(hdmi); + + dw_hdmi_setup_i2c(hdmi); + if (hdmi->i2c) + dw_hdmi_i2c_init(hdmi); + + if (hdmi->phy.ops->setup_hpd) + hdmi->phy.ops->setup_hpd(hdmi, hdmi->phy.data); + + return 0; +} +EXPORT_SYMBOL_GPL(dw_hdmi_resume); + MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>"); MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>"); MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>"); diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h index 66e70770cce5..c4132e9a5ae3 100644 --- a/include/drm/bridge/dw_hdmi.h +++ b/include/drm/bridge/dw_hdmi.h @@ -154,6 +154,9 @@ struct dw_hdmi *dw_hdmi_bind(struct platform_device *pdev, struct drm_encoder *encoder, const struct dw_hdmi_plat_data *plat_data); +int dw_hdmi_suspend(struct dw_hdmi *hdmi); +int dw_hdmi_resume(struct dw_hdmi *hdmi); + void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense); void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
On Rockchip rk3288-based Chromebooks when you do a suspend/resume cycle: 1. You lose the ability to detect an HDMI device being plugged in. 2. If you're using the i2c bus built in to dw_hdmi then it stops working. Let's add a hook to the core dw-hdmi driver so that we can call it in dw_hdmi-rockchip in the next commit. NOTE: the exact set of steps I've done here in resume come from looking at the normal dw_hdmi init sequence in upstream Linux plus the sequence that we did in downstream Chrome OS 3.14. Testing show that it seems to work, but if an extra step is needed or something here is not needed we could improve it. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 21 +++++++++++++++++++++ include/drm/bridge/dw_hdmi.h | 3 +++ 2 files changed, 24 insertions(+)