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 >>> >> >> [...] >