linux-kernel - Re: [PATCH v4 07/12] phy: rockchip: samsung-hdptx: Setup TMDS char rate via phy_configure_opts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ddf4c3c9-849c-4d96-a073-966bb19a6047@collabora.com>
Date: Tue, 4 Mar 2025 14:12:19 +0200
From: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I
 <kishon@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Algea Cao <algea.cao@...k-chips.com>, Sandor Yu <Sandor.yu@....com>,
 Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, kernel@...labora.com,
 linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v4 07/12] phy: rockchip: samsung-hdptx: Setup TMDS char
 rate via phy_configure_opts_hdmi

On 3/4/25 10:15 AM, Maxime Ripard wrote:
> On Tue, Mar 04, 2025 at 03:44:06AM +0200, Cristian Ciocaltea wrote:
>> The current workaround to setup the TMDS character rate relies on the
>> unconventional usage of phy_set_bus_width().
>>
>> Make use of the recently introduced HDMI PHY configuration API to
>> properly handle the setup.  The workaround will be dropped as soon as
>> the switch has been completed on both ends.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 54 ++++++++++++++++-------
>>  1 file changed, 38 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> index 2bf525514c1991a1299265d12e1e85f66333c604..7e1d1c10758249aa5bbddbdaae0108bba04f30df 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>> @@ -394,6 +394,7 @@ struct rk_hdptx_phy {
>>  
>>  	int phy_id;
>>  	struct phy *phy;
>> +	struct phy_configure_opts_hdmi hdmi_cfg;
>>  	struct clk_bulk_data *clks;
>>  	int nr_clks;
>>  	struct reset_control_bulk_data rsts[RST_MAX];
>> @@ -1409,19 +1410,25 @@ static int rk_hdptx_dp_aux_init(struct rk_hdptx_phy *hdptx)
>>  static int rk_hdptx_phy_power_on(struct phy *phy)
>>  {
>>  	struct rk_hdptx_phy *hdptx = phy_get_drvdata(phy);
>> -	int bus_width = phy_get_bus_width(hdptx->phy);
>>  	enum phy_mode mode = phy_get_mode(phy);
>> +	unsigned int rate = 0;
>>  	int ret, lane;
>>  
>> -	/*
>> -	 * FIXME: Temporary workaround to pass pixel_clk_rate
>> -	 * from the HDMI bridge driver until phy_configure_opts_hdmi
>> -	 * becomes available in the PHY API.
>> -	 */
>> -	unsigned int rate = bus_width & 0xfffffff;
>> -
>> -	dev_dbg(hdptx->dev, "%s bus_width=%x rate=%u\n",
>> -		__func__, bus_width, rate);
>> +	if (mode != PHY_MODE_DP) {
>> +		if (!hdptx->hdmi_cfg.tmds_char_rate) {
>> +			/*
>> +			 * FIXME: Temporary workaround to setup TMDS char rate
>> +			 * from the RK DW HDMI QP bridge driver.
>> +			 * Will be removed as soon the switch to the HDMI PHY
>> +			 * configuration API has been completed on both ends.
>> +			 */
>> +			rate = phy_get_bus_width(hdptx->phy) & 0xfffffff;
>> +			hdptx->hdmi_cfg.tmds_char_rate = rate * 100;
>> +		} else {
>> +			rate = hdptx->hdmi_cfg.tmds_char_rate / 100;
>> +		}
>> +		dev_dbg(hdptx->dev, "%s rate=%u\n", __func__, rate);
>> +	}
> 
> Some story here, I can't make sense of a variable in hHz. If it's
> actually needed and not a bug, this should be very explictly documented.

Not a bug - as explained earlier, phy_set_bus_width() on the other end
passes this in hHz.  I agree it should have been properly documented,
but eventually we got this cleaned up in the last patch of the series.

Thanks,
Cristian