| Message ID | 20210913172154.2686-1-jernej.skrabec@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sun4i: dw-hdmi: Fix HDMI PHY clock setup | expand |
Hello Jernej, On Mon, Sep 13, 2021 at 07:21:54PM +0200, Jernej Skrabec wrote: > Recent rework, which made HDMI PHY driver a platform device, inadvertely > reversed clock setup order. HW is very touchy about it. Proper way is to > handle controllers resets and clocks first and HDMI PHYs second. > > Currently, without this fix, first mode set completely fails (nothing on > HDMI monitor) on H3 era PHYs. On H6, it still somehow work. > > Move HDMI PHY reset & clocks handling to sun8i_hdmi_phy_init() which > will assure that code is executed after controllers reset & clocks are > handled. Additionally, add sun8i_hdmi_phy_deinit() which will deinit > them at controllers driver unload. > > Tested on A64, H3, H6 and R40. > > Fixes: 9bf3797796f5 ("drm/sun4i: dw-hdmi: Make HDMI PHY into a platform device") > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> > --- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 7 +- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 4 +- > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 97 ++++++++++++++------------ > 3 files changed, 61 insertions(+), 47 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > index f75fb157f2ff..5fa5407ac583 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > @@ -216,11 +216,13 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master, > goto err_disable_clk_tmds; > } ^^^ This looks like... > + ret = sun8i_hdmi_phy_init(hdmi->phy); > + if (ret) > + return ret; ... you need 'goto err_disable_clk_tmds;' here, instead. > + > drm_encoder_helper_add(encoder, &sun8i_dw_hdmi_encoder_helper_funcs); > drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS); > > - sun8i_hdmi_phy_init(hdmi->phy); > - > plat_data->mode_valid = hdmi->quirks->mode_valid; > plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe; > sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data); > @@ -262,6 +264,7 @@ static void sun8i_dw_hdmi_unbind(struct device *dev, struct device *master, > struct sun8i_dw_hdmi *hdmi = dev_get_drvdata(dev); > > dw_hdmi_unbind(hdmi->hdmi); > + sun8i_hdmi_phy_deinit(hdmi->phy); > clk_disable_unprepare(hdmi->clk_tmds); > reset_control_assert(hdmi->rst_ctrl); > gpiod_set_value(hdmi->ddc_en, 0); > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > index 74f6ed0e2570..bffe1b9cd3dc 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > @@ -169,6 +169,7 @@ struct sun8i_hdmi_phy { > struct clk *clk_phy; > struct clk *clk_pll0; > struct clk *clk_pll1; > + struct device *dev; > unsigned int rcal; > struct regmap *regs; > struct reset_control *rst_phy; > @@ -205,7 +206,8 @@ encoder_to_sun8i_dw_hdmi(struct drm_encoder *encoder) > > int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node); > > -void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy); > +int sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy); > +void sun8i_hdmi_phy_deinit(struct sun8i_hdmi_phy *phy); > void sun8i_hdmi_phy_set_ops(struct sun8i_hdmi_phy *phy, > struct dw_hdmi_plat_data *plat_data); > > diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > index c9239708d398..78b152973957 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > @@ -506,9 +506,60 @@ static void sun8i_hdmi_phy_init_h3(struct sun8i_hdmi_phy *phy) > phy->rcal = (val & SUN8I_HDMI_PHY_ANA_STS_RCAL_MASK) >> 2; > } > > -void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy) > +int sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy) > { > + int ret; > + > + ret = reset_control_deassert(phy->rst_phy); > + if (ret) { > + dev_err(phy->dev, "Cannot deassert phy reset control: %d\n", ret); > + return ret; > + } > + > + ret = clk_prepare_enable(phy->clk_bus); > + if (ret) { > + dev_err(phy->dev, "Cannot enable bus clock: %d\n", ret); > + goto err_deassert_rst_phy; I know it was there before, but please: s/deassert/assert/ kind regards, o. > + } > + > + ret = clk_prepare_enable(phy->clk_mod); > + if (ret) { > + dev_err(phy->dev, "Cannot enable mod clock: %d\n", ret); > + goto err_disable_clk_bus; > + } > + > + if (phy->variant->has_phy_clk) { > + ret = sun8i_phy_clk_create(phy, phy->dev, > + phy->variant->has_second_pll); > + if (ret) { > + dev_err(phy->dev, "Couldn't create the PHY clock\n"); > + goto err_disable_clk_mod; > + } > + > + clk_prepare_enable(phy->clk_phy); > + } > + > phy->variant->phy_init(phy); > + > + return 0; > + > +err_disable_clk_mod: > + clk_disable_unprepare(phy->clk_mod); > +err_disable_clk_bus: > + clk_disable_unprepare(phy->clk_bus); > +err_deassert_rst_phy: > + reset_control_assert(phy->rst_phy); > + > + return ret; > +} > + > > [......]
Hi! Dne torek, 14. september 2021 ob 10:59:22 CEST je Ondřej Jirman napisal(a): > Hello Jernej, > > On Mon, Sep 13, 2021 at 07:21:54PM +0200, Jernej Skrabec wrote: > > Recent rework, which made HDMI PHY driver a platform device, inadvertely > > reversed clock setup order. HW is very touchy about it. Proper way is to > > handle controllers resets and clocks first and HDMI PHYs second. > > > > Currently, without this fix, first mode set completely fails (nothing on > > HDMI monitor) on H3 era PHYs. On H6, it still somehow work. > > > > Move HDMI PHY reset & clocks handling to sun8i_hdmi_phy_init() which > > will assure that code is executed after controllers reset & clocks are > > handled. Additionally, add sun8i_hdmi_phy_deinit() which will deinit > > them at controllers driver unload. > > > > Tested on A64, H3, H6 and R40. > > > > Fixes: 9bf3797796f5 ("drm/sun4i: dw-hdmi: Make HDMI PHY into a platform device") > > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> > > --- > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 7 +- > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 4 +- > > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 97 ++++++++++++++------------ > > 3 files changed, 61 insertions(+), 47 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/ sun8i_dw_hdmi.c > > index f75fb157f2ff..5fa5407ac583 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c > > @@ -216,11 +216,13 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master, > > goto err_disable_clk_tmds; > > } > > ^^^ This looks like... > > > + ret = sun8i_hdmi_phy_init(hdmi->phy); > > + if (ret) > > + return ret; > > ... you need 'goto err_disable_clk_tmds;' here, instead. Ah, right. Will fix in v2. > > > + > > drm_encoder_helper_add(encoder, &sun8i_dw_hdmi_encoder_helper_funcs); > > drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS); > > > > - sun8i_hdmi_phy_init(hdmi->phy); > > - > > plat_data->mode_valid = hdmi->quirks->mode_valid; > > plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe; > > sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data); > > @@ -262,6 +264,7 @@ static void sun8i_dw_hdmi_unbind(struct device *dev, struct device *master, > > struct sun8i_dw_hdmi *hdmi = dev_get_drvdata(dev); > > > > dw_hdmi_unbind(hdmi->hdmi); > > + sun8i_hdmi_phy_deinit(hdmi->phy); > > clk_disable_unprepare(hdmi->clk_tmds); > > reset_control_assert(hdmi->rst_ctrl); > > gpiod_set_value(hdmi->ddc_en, 0); > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/ sun8i_dw_hdmi.h > > index 74f6ed0e2570..bffe1b9cd3dc 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > > @@ -169,6 +169,7 @@ struct sun8i_hdmi_phy { > > struct clk *clk_phy; > > struct clk *clk_pll0; > > struct clk *clk_pll1; > > + struct device *dev; > > unsigned int rcal; > > struct regmap *regs; > > struct reset_control *rst_phy; > > @@ -205,7 +206,8 @@ encoder_to_sun8i_dw_hdmi(struct drm_encoder *encoder) > > > > int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node); > > > > -void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy); > > +int sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy); > > +void sun8i_hdmi_phy_deinit(struct sun8i_hdmi_phy *phy); > > void sun8i_hdmi_phy_set_ops(struct sun8i_hdmi_phy *phy, > > struct dw_hdmi_plat_data *plat_data); > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/ sun4i/sun8i_hdmi_phy.c > > index c9239708d398..78b152973957 100644 > > --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > > +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > > @@ -506,9 +506,60 @@ static void sun8i_hdmi_phy_init_h3(struct sun8i_hdmi_phy *phy) > > phy->rcal = (val & SUN8I_HDMI_PHY_ANA_STS_RCAL_MASK) >> 2; > > } > > > > -void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy) > > +int sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy) > > { > > + int ret; > > + > > + ret = reset_control_deassert(phy->rst_phy); > > + if (ret) { > > + dev_err(phy->dev, "Cannot deassert phy reset control: %d\n", ret); > > + return ret; > > + } > > + > > + ret = clk_prepare_enable(phy->clk_bus); > > + if (ret) { > > + dev_err(phy->dev, "Cannot enable bus clock: %d\n", ret); > > + goto err_deassert_rst_phy; > > I know it was there before, but please: > > s/deassert/assert/ Ok. Best regards, Jernej > > kind regards, > o. > > > + } > > + > > + ret = clk_prepare_enable(phy->clk_mod); > > + if (ret) { > > + dev_err(phy->dev, "Cannot enable mod clock: %d\n", ret); > > + goto err_disable_clk_bus; > > + } > > + > > + if (phy->variant->has_phy_clk) { > > + ret = sun8i_phy_clk_create(phy, phy->dev, > > + phy->variant- >has_second_pll); > > + if (ret) { > > + dev_err(phy->dev, "Couldn't create the PHY clock\n"); > > + goto err_disable_clk_mod; > > + } > > + > > + clk_prepare_enable(phy->clk_phy); > > + } > > + > > phy->variant->phy_init(phy); > > + > > + return 0; > > + > > +err_disable_clk_mod: > > + clk_disable_unprepare(phy->clk_mod); > > +err_disable_clk_bus: > > + clk_disable_unprepare(phy->clk_bus); > > +err_deassert_rst_phy: > > + reset_control_assert(phy->rst_phy); > > + > > + return ret; > > +} > > + > > > > [......] >
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c index f75fb157f2ff..5fa5407ac583 100644 --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c @@ -216,11 +216,13 @@ static int sun8i_dw_hdmi_bind(struct device *dev, struct device *master, goto err_disable_clk_tmds; } + ret = sun8i_hdmi_phy_init(hdmi->phy); + if (ret) + return ret; + drm_encoder_helper_add(encoder, &sun8i_dw_hdmi_encoder_helper_funcs); drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS); - sun8i_hdmi_phy_init(hdmi->phy); - plat_data->mode_valid = hdmi->quirks->mode_valid; plat_data->use_drm_infoframe = hdmi->quirks->use_drm_infoframe; sun8i_hdmi_phy_set_ops(hdmi->phy, plat_data); @@ -262,6 +264,7 @@ static void sun8i_dw_hdmi_unbind(struct device *dev, struct device *master, struct sun8i_dw_hdmi *hdmi = dev_get_drvdata(dev); dw_hdmi_unbind(hdmi->hdmi); + sun8i_hdmi_phy_deinit(hdmi->phy); clk_disable_unprepare(hdmi->clk_tmds); reset_control_assert(hdmi->rst_ctrl); gpiod_set_value(hdmi->ddc_en, 0); diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 74f6ed0e2570..bffe1b9cd3dc 100644 --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h @@ -169,6 +169,7 @@ struct sun8i_hdmi_phy { struct clk *clk_phy; struct clk *clk_pll0; struct clk *clk_pll1; + struct device *dev; unsigned int rcal; struct regmap *regs; struct reset_control *rst_phy; @@ -205,7 +206,8 @@ encoder_to_sun8i_dw_hdmi(struct drm_encoder *encoder) int sun8i_hdmi_phy_get(struct sun8i_dw_hdmi *hdmi, struct device_node *node); -void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy); +int sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy); +void sun8i_hdmi_phy_deinit(struct sun8i_hdmi_phy *phy); void sun8i_hdmi_phy_set_ops(struct sun8i_hdmi_phy *phy, struct dw_hdmi_plat_data *plat_data); diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index c9239708d398..78b152973957 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -506,9 +506,60 @@ static void sun8i_hdmi_phy_init_h3(struct sun8i_hdmi_phy *phy) phy->rcal = (val & SUN8I_HDMI_PHY_ANA_STS_RCAL_MASK) >> 2; } -void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy) +int sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy) { + int ret; + + ret = reset_control_deassert(phy->rst_phy); + if (ret) { + dev_err(phy->dev, "Cannot deassert phy reset control: %d\n", ret); + return ret; + } + + ret = clk_prepare_enable(phy->clk_bus); + if (ret) { + dev_err(phy->dev, "Cannot enable bus clock: %d\n", ret); + goto err_deassert_rst_phy; + } + + ret = clk_prepare_enable(phy->clk_mod); + if (ret) { + dev_err(phy->dev, "Cannot enable mod clock: %d\n", ret); + goto err_disable_clk_bus; + } + + if (phy->variant->has_phy_clk) { + ret = sun8i_phy_clk_create(phy, phy->dev, + phy->variant->has_second_pll); + if (ret) { + dev_err(phy->dev, "Couldn't create the PHY clock\n"); + goto err_disable_clk_mod; + } + + clk_prepare_enable(phy->clk_phy); + } + phy->variant->phy_init(phy); + + return 0; + +err_disable_clk_mod: + clk_disable_unprepare(phy->clk_mod); +err_disable_clk_bus: + clk_disable_unprepare(phy->clk_bus); +err_deassert_rst_phy: + reset_control_assert(phy->rst_phy); + + return ret; +} + +void sun8i_hdmi_phy_deinit(struct sun8i_hdmi_phy *phy) +{ + clk_disable_unprepare(phy->clk_mod); + clk_disable_unprepare(phy->clk_bus); + clk_disable_unprepare(phy->clk_phy); + + reset_control_assert(phy->rst_phy); } void sun8i_hdmi_phy_set_ops(struct sun8i_hdmi_phy *phy, @@ -638,6 +689,7 @@ static int sun8i_hdmi_phy_probe(struct platform_device *pdev) return -ENOMEM; phy->variant = (struct sun8i_hdmi_phy_variant *)match->data; + phy->dev = dev; ret = of_address_to_resource(node, 0, &res); if (ret) { @@ -696,47 +748,10 @@ static int sun8i_hdmi_phy_probe(struct platform_device *pdev) goto err_put_clk_pll1; } - ret = reset_control_deassert(phy->rst_phy); - if (ret) { - dev_err(dev, "Cannot deassert phy reset control: %d\n", ret); - goto err_put_rst_phy; - } - - ret = clk_prepare_enable(phy->clk_bus); - if (ret) { - dev_err(dev, "Cannot enable bus clock: %d\n", ret); - goto err_deassert_rst_phy; - } - - ret = clk_prepare_enable(phy->clk_mod); - if (ret) { - dev_err(dev, "Cannot enable mod clock: %d\n", ret); - goto err_disable_clk_bus; - } - - if (phy->variant->has_phy_clk) { - ret = sun8i_phy_clk_create(phy, dev, - phy->variant->has_second_pll); - if (ret) { - dev_err(dev, "Couldn't create the PHY clock\n"); - goto err_disable_clk_mod; - } - - clk_prepare_enable(phy->clk_phy); - } - platform_set_drvdata(pdev, phy); return 0; -err_disable_clk_mod: - clk_disable_unprepare(phy->clk_mod); -err_disable_clk_bus: - clk_disable_unprepare(phy->clk_bus); -err_deassert_rst_phy: - reset_control_assert(phy->rst_phy); -err_put_rst_phy: - reset_control_put(phy->rst_phy); err_put_clk_pll1: clk_put(phy->clk_pll1); err_put_clk_pll0: @@ -753,12 +768,6 @@ static int sun8i_hdmi_phy_remove(struct platform_device *pdev) { struct sun8i_hdmi_phy *phy = platform_get_drvdata(pdev); - clk_disable_unprepare(phy->clk_mod); - clk_disable_unprepare(phy->clk_bus); - clk_disable_unprepare(phy->clk_phy); - - reset_control_assert(phy->rst_phy); - reset_control_put(phy->rst_phy); clk_put(phy->clk_pll0);
Recent rework, which made HDMI PHY driver a platform device, inadvertely reversed clock setup order. HW is very touchy about it. Proper way is to handle controllers resets and clocks first and HDMI PHYs second. Currently, without this fix, first mode set completely fails (nothing on HDMI monitor) on H3 era PHYs. On H6, it still somehow work. Move HDMI PHY reset & clocks handling to sun8i_hdmi_phy_init() which will assure that code is executed after controllers reset & clocks are handled. Additionally, add sun8i_hdmi_phy_deinit() which will deinit them at controllers driver unload. Tested on A64, H3, H6 and R40. Fixes: 9bf3797796f5 ("drm/sun4i: dw-hdmi: Make HDMI PHY into a platform device") Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com> --- drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c | 7 +- drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 4 +- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 97 ++++++++++++++------------ 3 files changed, 61 insertions(+), 47 deletions(-)