[PATCH v2] phy: dphy: Correct clk_pre parameter

Wed Jan 19 10:01:08 UTC 2022

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 at intel.com>
> > > Cc: Neil Armstrong <narmstrong at baylibre.com>
> > > Cc: Robert Foss <robert.foss at linaro.org>
> > > Cc: Laurent Pinchart <Laurent.pinchart at ideasonboard.com>
> > > Cc: Jonas Karlman <jonas at kwiboo.se>
> > > Cc: Jernej Skrabec <jernej.skrabec at gmail.com>
> > > Cc: David Airlie <airlied at linux.ie>
> > > Cc: Daniel Vetter <daniel at ffwll.ch>
> > > Cc: Kishon Vijay Abraham I <kishon at ti.com>
> > > Cc: Vinod Koul <vkoul at kernel.org>
> > > Cc: Kevin Hilman <khilman at baylibre.com>
> > > Cc: Jerome Brunet <jbrunet at baylibre.com>
> > > Cc: Martin Blumenstingl <martin.blumenstingl at googlemail.com>
> > > Cc: Heiko Stuebner <heiko at sntech.de>
> > > Cc: Maxime Ripard <mripard at kernel.org>
> > > Cc: Guido Günther <agx at sigxcpu.org>
> > > Tested-by: Liu Ying <victor.liu at nxp.com> # RM67191 DSI panel on i.MX8mq EVK
> > > Signed-off-by: Liu Ying <victor.liu at nxp.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)

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?

Regards,
Liu Ying

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