| Message ID | 20240730125023.710237-6-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/meson: dw-hdmi: clean-up | expand |
Hi Jerome, On Tue, Jul 30, 2024 at 2:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote: > > This prepares the migration to regmap usage. > > To properly setup regmap, the APB needs to be in working order. > This is easier handled if the resets are not mixed with hw init. > > More checks are required to determine if the resets are needed > on resume or not. Add a note for this. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/gpu/drm/meson/meson_dw_hdmi.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c > index 5cd3264ab874..47aa3e184e98 100644 > --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c > +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c > @@ -581,11 +581,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi) > /* Bring HDMITX MEM output of power down */ > regmap_update_bits(priv->hhi, HHI_MEM_PD_REG0, 0xff << 8, 0); > > - /* Reset HDMITX APB & TX & PHY */ > - reset_control_reset(meson_dw_hdmi->hdmitx_apb); > - reset_control_reset(meson_dw_hdmi->hdmitx_ctrl); > - reset_control_reset(meson_dw_hdmi->hdmitx_phy); > - > /* Enable APB3 fail on error */ > if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) { > writel_bits_relaxed(BIT(15), BIT(15), > @@ -675,6 +670,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master, > return PTR_ERR(meson_dw_hdmi->hdmitx_phy); > } > > + reset_control_reset(meson_dw_hdmi->hdmitx_apb); > + reset_control_reset(meson_dw_hdmi->hdmitx_ctrl); > + reset_control_reset(meson_dw_hdmi->hdmitx_phy); The old out of tree vendor driver [0] enables the "isfr" and "iahb" (in P_HHI_HDMI_CLK_CNTL and P_HHI_GCLK_MPEG2) clocks before triggering the resets. Previously meson_dw_hdmi's behavior was identical as it enabled the clocks in meson_dw_hdmi_bind() and only later triggered the resets. I'm totally fine with moving the resets to meson_dw_hdmi_bind() but I think it should happen after devm_clk_bulk_get_all_enable() has been called (to keep the order and thus avoid side-effects that we don't know about yet). Also out of curiosity: are you planning to convert the driver to use devm_reset_control_bulk_get_exclusive()? Best regards, Martin [0] https://github.com/endlessm/linux-s905x/blob/master/drivers/amlogic/hdmi/hdmi_tx_20/hw/hdmi_tx_hw.c#L470
On Tue 06 Aug 2024 at 22:49, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > Hi Jerome, > > On Tue, Jul 30, 2024 at 2:50 PM Jerome Brunet <jbrunet@baylibre.com> wrote: >> >> This prepares the migration to regmap usage. >> >> To properly setup regmap, the APB needs to be in working order. >> This is easier handled if the resets are not mixed with hw init. >> >> More checks are required to determine if the resets are needed >> on resume or not. Add a note for this. >> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> drivers/gpu/drm/meson/meson_dw_hdmi.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c >> index 5cd3264ab874..47aa3e184e98 100644 >> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c >> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c >> @@ -581,11 +581,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi) >> /* Bring HDMITX MEM output of power down */ >> regmap_update_bits(priv->hhi, HHI_MEM_PD_REG0, 0xff << 8, 0); >> >> - /* Reset HDMITX APB & TX & PHY */ >> - reset_control_reset(meson_dw_hdmi->hdmitx_apb); >> - reset_control_reset(meson_dw_hdmi->hdmitx_ctrl); >> - reset_control_reset(meson_dw_hdmi->hdmitx_phy); >> - >> /* Enable APB3 fail on error */ >> if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) { >> writel_bits_relaxed(BIT(15), BIT(15), >> @@ -675,6 +670,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master, >> return PTR_ERR(meson_dw_hdmi->hdmitx_phy); >> } >> >> + reset_control_reset(meson_dw_hdmi->hdmitx_apb); >> + reset_control_reset(meson_dw_hdmi->hdmitx_ctrl); >> + reset_control_reset(meson_dw_hdmi->hdmitx_phy); > The old out of tree vendor driver [0] enables the "isfr" and "iahb" > (in P_HHI_HDMI_CLK_CNTL and P_HHI_GCLK_MPEG2) clocks before triggering > the resets. > Previously meson_dw_hdmi's behavior was identical as it enabled the > clocks in meson_dw_hdmi_bind() and only later triggered the resets. > > I'm totally fine with moving the resets to meson_dw_hdmi_bind() but I > think it should happen after devm_clk_bulk_get_all_enable() has been > called (to keep the order and thus avoid side-effects that we don't > know about yet). Good point. I was also thinking about squashing this with the regmap patch. I've split it apart for v1 to make things a bit more clear but it only really makes sense with the regmap conversion. > > Also out of curiosity: are you planning to convert the driver to use > devm_reset_control_bulk_get_exclusive()? > It's been a while this I've done that. I remember I thought about it. I think it was a bit more difficult to use that clocks. I was looking at making the driver a bit more clean and simple. It was not really helping to move it in that direction IIRC. > > Best regards, > Martin > > > [0] https://github.com/endlessm/linux-s905x/blob/master/drivers/amlogic/hdmi/hdmi_tx_20/hw/hdmi_tx_hw.c#L470
On 30/07/2024 14:50, Jerome Brunet wrote: > This prepares the migration to regmap usage. > > To properly setup regmap, the APB needs to be in working order. > This is easier handled if the resets are not mixed with hw init. > > More checks are required to determine if the resets are needed > on resume or not. Add a note for this. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/gpu/drm/meson/meson_dw_hdmi.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c > index 5cd3264ab874..47aa3e184e98 100644 > --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c > +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c > @@ -581,11 +581,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi) > /* Bring HDMITX MEM output of power down */ > regmap_update_bits(priv->hhi, HHI_MEM_PD_REG0, 0xff << 8, 0); > > - /* Reset HDMITX APB & TX & PHY */ > - reset_control_reset(meson_dw_hdmi->hdmitx_apb); > - reset_control_reset(meson_dw_hdmi->hdmitx_ctrl); > - reset_control_reset(meson_dw_hdmi->hdmitx_phy); > - > /* Enable APB3 fail on error */ > if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) { > writel_bits_relaxed(BIT(15), BIT(15), > @@ -675,6 +670,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master, > return PTR_ERR(meson_dw_hdmi->hdmitx_phy); > } > > + reset_control_reset(meson_dw_hdmi->hdmitx_apb); > + reset_control_reset(meson_dw_hdmi->hdmitx_ctrl); > + reset_control_reset(meson_dw_hdmi->hdmitx_phy); > + > meson_dw_hdmi->hdmitx = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(meson_dw_hdmi->hdmitx)) > return PTR_ERR(meson_dw_hdmi->hdmitx); > @@ -765,6 +764,11 @@ static int __maybe_unused meson_dw_hdmi_pm_resume(struct device *dev) > if (!meson_dw_hdmi) > return 0; > > + /* TODO: Is this really necessary/desirable on resume ? */ Yes to reset the HDMI controller to it's default state, not sure if the note is important here. > + reset_control_reset(meson_dw_hdmi->hdmitx_apb); > + reset_control_reset(meson_dw_hdmi->hdmitx_ctrl); > + reset_control_reset(meson_dw_hdmi->hdmitx_phy); > + > meson_dw_hdmi_init(meson_dw_hdmi); > > dw_hdmi_resume(meson_dw_hdmi->hdmi);
diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c b/drivers/gpu/drm/meson/meson_dw_hdmi.c index 5cd3264ab874..47aa3e184e98 100644 --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c @@ -581,11 +581,6 @@ static void meson_dw_hdmi_init(struct meson_dw_hdmi *meson_dw_hdmi) /* Bring HDMITX MEM output of power down */ regmap_update_bits(priv->hhi, HHI_MEM_PD_REG0, 0xff << 8, 0); - /* Reset HDMITX APB & TX & PHY */ - reset_control_reset(meson_dw_hdmi->hdmitx_apb); - reset_control_reset(meson_dw_hdmi->hdmitx_ctrl); - reset_control_reset(meson_dw_hdmi->hdmitx_phy); - /* Enable APB3 fail on error */ if (!meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A)) { writel_bits_relaxed(BIT(15), BIT(15), @@ -675,6 +670,10 @@ static int meson_dw_hdmi_bind(struct device *dev, struct device *master, return PTR_ERR(meson_dw_hdmi->hdmitx_phy); } + reset_control_reset(meson_dw_hdmi->hdmitx_apb); + reset_control_reset(meson_dw_hdmi->hdmitx_ctrl); + reset_control_reset(meson_dw_hdmi->hdmitx_phy); + meson_dw_hdmi->hdmitx = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(meson_dw_hdmi->hdmitx)) return PTR_ERR(meson_dw_hdmi->hdmitx); @@ -765,6 +764,11 @@ static int __maybe_unused meson_dw_hdmi_pm_resume(struct device *dev) if (!meson_dw_hdmi) return 0; + /* TODO: Is this really necessary/desirable on resume ? */ + reset_control_reset(meson_dw_hdmi->hdmitx_apb); + reset_control_reset(meson_dw_hdmi->hdmitx_ctrl); + reset_control_reset(meson_dw_hdmi->hdmitx_phy); + meson_dw_hdmi_init(meson_dw_hdmi); dw_hdmi_resume(meson_dw_hdmi->hdmi);
This prepares the migration to regmap usage. To properly setup regmap, the APB needs to be in working order. This is easier handled if the resets are not mixed with hw init. More checks are required to determine if the resets are needed on resume or not. Add a note for this. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/gpu/drm/meson/meson_dw_hdmi.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)