Received: by 2002:a05:6a11:4021:0:0:0:0 with SMTP id ky33csp485493pxb; Tue, 14 Sep 2021 02:01:29 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyXL0ghJbWJvhypZd/dI09epTDLblZuRwm3FdFXnYj7w5zKul40SLY3j0xMeKankI3000rh X-Received: by 2002:a6b:5819:: with SMTP id m25mr12795338iob.105.1631610089653; Tue, 14 Sep 2021 02:01:29 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1631610089; cv=none; d=google.com; s=arc-20160816; b=1DF1wamZRwGHoBmiFZF0AbiLqJi0PTGvCrpvb02NjvyUdLCSPNNI004N36gwpa2cQN YjWnW48asm2K156lflBWM+fVgVnEBytl2h8T2oP604pNusefIvVynQ4PUMczcN40IFBH CYzdaMxmP/EpCXKdj3JoftqUR677roEduAFMm/jwYSDLfA6O3Nl8WuGiViYh/AMGfh48 ZhxdoVOCtkSbVS5yTcc3rrgJEkcEM2dIfL6fUPA2qJHujb23iz2n9zdZey8BYeejNFV0 mtVE9JmOLZ1zrcnGyTTpiYyEV7BN5j1/frs166AXpOqSBvNnJbFmSUMlSFlelVAuGUc/ Tg2Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:mail-followup-to:message-id:subject:cc:to:from:date :dkim-signature; bh=Zd0MJdocTMCWW+PfDZkPju4Xt6xemPssuaaC/orb0cg=; b=l1OktdGy10FXFDC0Eg5FPt2dAXB8h1u3zHep+MwEr98YWEmOW1PLvA2BiWPq8/fEE9 dfy20JIod1YEdEBWO/Y/ziGj8FvghzupCxGOBkuoaJxIBjYxLerJ96PJIadB+au29/sQ I6Ge8JNLqxCeXcYqbKpJtbDb+0wfI6Exwa/V3VdT9SHvyC9lXtQtiAbBA9EjrihKDDHO OiAPRPIigXMAu82o+PKpaN1ZB31yCdrSIrkRrTHfM5ssGyahR3L+/r4apQhs34wof3Qy O9GBSHp8Y7wfD1totd2GNPoEq951jAYOc6XWr+ypHWP2YK9fVE1n/tVrI+wVbcHlIX/+ 78xw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@megous.com header.s=mail header.b=GGvk1MCa; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=megous.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id j8si2067898jat.1.2021.09.14.02.01.16; Tue, 14 Sep 2021 02:01:29 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@megous.com header.s=mail header.b=GGvk1MCa; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=megous.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229912AbhINJAm (ORCPT + 99 others); Tue, 14 Sep 2021 05:00:42 -0400 Received: from vps.xff.cz ([195.181.215.36]:38368 "EHLO vps.xff.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229526AbhINJAl (ORCPT ); Tue, 14 Sep 2021 05:00:41 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=megous.com; s=mail; t=1631609962; bh=FMNEyanpnGkCzKioEYG1k/II7rKs6qQbFTwCXVGg4oc=; h=Date:From:To:Cc:Subject:X-My-GPG-KeyId:References:From; b=GGvk1MCa4lFwc1uX1oYJ6MPX92vPtub7KCi60FulGvIvB4YakA/B6yWwCeCJbkvaF sdE2vneLJROD6ev1jyF1TxBuoaEp5kBzeFUBkFa89/dNGlnyb3UzeQKatkzk55w5fu Xq4JkBithVZ/VaOEzMCCktvrEqFFBVw58taz9MnQ= Date: Tue, 14 Sep 2021 10:59:22 +0200 From: =?utf-8?Q?Ond=C5=99ej?= Jirman To: Jernej Skrabec Cc: mripard@kernel.org, wens@csie.org, airlied@linux.ie, daniel@ffwll.ch, saravanak@google.com, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/sun4i: dw-hdmi: Fix HDMI PHY clock setup Message-ID: <20210914085922.qxhmr6puvy5d2ceo@core> Mail-Followup-To: =?utf-8?Q?Ond=C5=99ej?= Jirman , Jernej Skrabec , mripard@kernel.org, wens@csie.org, airlied@linux.ie, daniel@ffwll.ch, saravanak@google.com, dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org X-My-GPG-KeyId: EBFBDDE11FB918D44D1F56C1F9F0A873BE9777ED References: <20210913172154.2686-1-jernej.skrabec@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210913172154.2686-1-jernej.skrabec@gmail.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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; > +} > + > > [......]