Re: [PATCH v2] phy: dphy: Correct clk_pre parameter

[Neil Armstrong]

Hi,

On 19/01/2022 11:01, Liu Ying wrote:
> Hi Neil,
> 
> On Wed, 2022-01-19 at 10:11 +0100, Neil Armstrong wrote:
>> 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@xxxxxxxxx>
>>>> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>>>> Cc: Robert Foss <robert.foss@xxxxxxxxxx>
>>>> Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
>>>> Cc: Jonas Karlman <jonas@xxxxxxxxx>
>>>> Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
>>>> Cc: David Airlie <airlied@xxxxxxxx>
>>>> Cc: Daniel Vetter <daniel@xxxxxxxx>
>>>> Cc: Kishon Vijay Abraham I <kishon@xxxxxx>
>>>> Cc: Vinod Koul <vkoul@xxxxxxxxxx>
>>>> Cc: Kevin Hilman <khilman@xxxxxxxxxxxx>
>>>> Cc: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
>>>> Cc: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
>>>> Cc: Heiko Stuebner <heiko@xxxxxxxxx>
>>>> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
>>>> Cc: Guido Günther <agx@xxxxxxxxxxx>
>>>> Tested-by: Liu Ying <victor.liu@xxxxxxx> # RM67191 DSI panel on i.MX8mq EVK
>>>> Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
>>>> ---
>>>> 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)
> 
> This looks like clk_pre time is 10 * ui, which matches the comment
> '>8*ui' - longer time 8 * ui.
> 
>>
>> t_ui = lcd_timing.bit_rate
>>
>> t_ui = (1000000 * 100) / (dsi_ui / 1000); /*100*ns */
>> temp = t_ui * 8; /* lane_byte cycle time */
> 
> If I read correctly, this temp in vendor code is TxByteClkHS period
> time in picoseconds. So...
> 
>>
>> dphy->clk_pre = ((DPHY_TIME_CLK_PRE(t_ui) + temp - 1) / temp) & 0xff;
> 
> IIUC, 'dphy->clk_pre' essentially means dphy->clk_pre = DIV_ROUND_UP(10
> * ui, temp), that is, the time for dphy->clk_pre is no less than 
> 10 * ui.  The D-PHY spec (v1.2)'s saying is that the minimum time for
> dphy->clk_pre is 8 * ui.  So, it looks like meson is stricter than the
> spec.
> 
> However, the vendor code doesn't seem to match the current meson driver
> implementation.  The temp in driver code is also TxByteClkHS period
> time in picoseconds. And, without this patch, I'm assuming that
> priv->config.clk_pre is 8000.  So, it looks like MIPI_DSI_CLK_TIM1 is
> set to DIV_ROUND_UP(8000, temp).  This '8000' does _not_ reflect
> '10 * ui'.
> 
> So, 3 choices for meson, up to you:
> 1) If meson really requires the time for clk_pre no less than
> 10 * ui, then sth like DIV_ROUND_UP(10, BITS_PER_BYTE) can be used.
> 2) If meson follows the spec's minimum value, then the patch does
> things right for meson.
> 3) Force to use 8000 picoseconds for clk_pre, that is, use sth like
> DIV_ROUND_UP(8000, temp).
> 
> With this patch applied, do you see any display artifacts by visual
> check? Or even checked signals through DSI analyzer?

Thanks for the feedback, I'm currently trying to put the setup back to
run a test with this change.

Neil

> 
> Regards,
> Liu Ying
> 
>>
>> PHY Registers only says:
>> MIPI_DSI_CLK_TIM1	[31:0]
>> 7:0 	R/W	0	 tCLK_PRE
>>
>>> Neil
>>>
>>
>> [...]
>