linux-kernel - Re: [PATCH v2] phy: dphy: Correct clk

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]
Date:   Wed, 19 Jan 2022 10:11:51 +0100
From:   Neil Armstrong <narmstrong@...libre.com>
To:     Liu Ying <victor.liu@....com>, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org
Cc:     linux-imx@....com, Andrzej Hajda <andrzej.hajda@...el.com>,
        Robert Foss <robert.foss@...aro.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Kishon Vijay Abraham I <kishon@...com>,
        Vinod Koul <vkoul@...nel.org>,
        Kevin Hilman <khilman@...libre.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Maxime Ripard <mripard@...nel.org>,
        Guido Günther <agx@...xcpu.org>
Subject: Re: [PATCH v2] phy: dphy: Correct clk_pre parameter

On 19/01/2022 09:40, Neil Armstrong wrote:
> Hi,
> 
> On 19/01/2022 03:37, Liu Ying wrote:
>> The D-PHY specification (v1.2) explicitly mentions that the T-CLK-PRE
>> parameter's unit is Unit Interval(UI) and the minimum value is 8.  Also,
>> kernel doc of the 'clk_pre' member of struct phy_configure_opts_mipi_dphy
>> mentions that it should be in UI.  However, the dphy core driver wrongly
>> sets 'clk_pre' to 8000, which seems to hint that it's in picoseconds.
>> And, the kernel doc of the 'clk_pre' member wrongly says the minimum value
>> is '8 UI', instead of 8.
>>
>> So, let's fix both the dphy core driver and the kernel doc of the 'clk_pre'
>> member to correctly reflect the T-CLK-PRE parameter's unit and the minimum
>> value according to the D-PHY specification.
>>
>> I'm assuming that all impacted custom drivers shall program values in
>> TxByteClkHS cycles into hardware for the T-CLK-PRE parameter.  The D-PHY
>> specification mentions that the frequency of TxByteClkHS is exactly 1/8
>> the High-Speed(HS) bit rate(each HS bit consumes one UI).  So, relevant
>> custom driver code is changed to program those values as
>> DIV_ROUND_UP(cfg->clk_pre, BITS_PER_BYTE), then.
>>
>> Note that I've only tested the patch with RM67191 DSI panel on i.MX8mq EVK.
>> Help is needed to test with other i.MX8mq, Meson and Rockchip platforms,
>> as I don't have the hardwares.
>>
>> Fixes: 2ed869990e14 ("phy: Add MIPI D-PHY configuration options")
>> Cc: Andrzej Hajda <andrzej.hajda@...el.com>
>> Cc: Neil Armstrong <narmstrong@...libre.com>
>> Cc: Robert Foss <robert.foss@...aro.org>
>> Cc: Laurent Pinchart <Laurent.pinchart@...asonboard.com>
>> Cc: Jonas Karlman <jonas@...boo.se>
>> Cc: Jernej Skrabec <jernej.skrabec@...il.com>
>> Cc: David Airlie <airlied@...ux.ie>
>> Cc: Daniel Vetter <daniel@...ll.ch>
>> Cc: Kishon Vijay Abraham I <kishon@...com>
>> Cc: Vinod Koul <vkoul@...nel.org>
>> Cc: Kevin Hilman <khilman@...libre.com>
>> Cc: Jerome Brunet <jbrunet@...libre.com>
>> Cc: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
>> Cc: Heiko Stuebner <heiko@...ech.de>
>> Cc: Maxime Ripard <mripard@...nel.org>
>> Cc: Guido Günther <agx@...xcpu.org>
>> Tested-by: Liu Ying <victor.liu@....com> # RM67191 DSI panel on i.MX8mq EVK
>> Signed-off-by: Liu Ying <victor.liu@....com>
>> ---
>> v1->v2:
>> * Use BITS_PER_BYTE macro. (Andrzej)
>> * Drop dsi argument from ui2bc() in nwl-dsi.c.
>>
>>  drivers/gpu/drm/bridge/nwl-dsi.c                 | 12 +++++-------
>>  drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c    |  3 ++-
>>  drivers/phy/phy-core-mipi-dphy.c                 |  4 ++--
>>  drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c |  3 ++-
>>  include/linux/phy/phy-mipi-dphy.h                |  2 +-
>>  5 files changed, 12 insertions(+), 12 deletions(-)
>>
[...]

>> diff --git a/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c b/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
>> index cd2332bf0e31..fdbd64c03e12 100644
>> --- a/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
>> +++ b/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
>> @@ -9,6 +9,7 @@
>>  
>>  #include <linux/bitfield.h>
>>  #include <linux/bitops.h>
>> +#include <linux/bits.h>
>>  #include <linux/clk.h>
>>  #include <linux/delay.h>
>>  #include <linux/io.h>
>> @@ -250,7 +251,7 @@ static int phy_meson_axg_mipi_dphy_power_on(struct phy *phy)
>>  		     (DIV_ROUND_UP(priv->config.clk_zero, temp) << 16) |
>>  		     (DIV_ROUND_UP(priv->config.clk_prepare, temp) << 24));
>>  	regmap_write(priv->regmap, MIPI_DSI_CLK_TIM1,
>> -		     DIV_ROUND_UP(priv->config.clk_pre, temp));
>> +		     DIV_ROUND_UP(priv->config.clk_pre, BITS_PER_BYTE));
>>  
>>  	regmap_write(priv->regmap, MIPI_DSI_HS_TIM,
>>  		     DIV_ROUND_UP(priv->config.hs_exit, temp) |
> 
> I'll try to run a test, currently the calculation gives 2, so this would give 1.

The Amlogic vendor code does:

/* >8*ui */
#define DPHY_TIME_CLK_PRE(ui)       (10 * ui)

t_ui = lcd_timing.bit_rate

t_ui = (1000000 * 100) / (dsi_ui / 1000); /*100*ns */
temp = t_ui * 8; /* lane_byte cycle time */

dphy->clk_pre = ((DPHY_TIME_CLK_PRE(t_ui) + temp - 1) / temp) & 0xff;

PHY Registers only says:
MIPI_DSI_CLK_TIM1	[31:0]
7:0 	R/W	0	 tCLK_PRE

> 
> Neil
> 

[...]