diff mbox series

[v2] phy: dphy: Correct clk_pre parameter

Message ID 20220119023714.1498508-1-victor.liu@nxp.com (mailing list archive)
State Superseded
Headers show
Series [v2] phy: dphy: Correct clk_pre parameter | expand

Commit Message

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

Comments

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

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards

Andrzej

> ---
> 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/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> index a7389a0facfb..af07eeb47ca0 100644
> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> @@ -7,6 +7,7 @@
>    */
>   
>   #include <linux/bitfield.h>
> +#include <linux/bits.h>
>   #include <linux/clk.h>
>   #include <linux/irq.h>
>   #include <linux/math64.h>
> @@ -196,12 +197,9 @@ static u32 ps2bc(struct nwl_dsi *dsi, unsigned long long ps)
>   /*
>    * ui2bc - UI time periods to byte clock cycles
>    */
> -static u32 ui2bc(struct nwl_dsi *dsi, unsigned long long ui)
> +static u32 ui2bc(unsigned int ui)
>   {
> -	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> -
> -	return DIV64_U64_ROUND_UP(ui * dsi->lanes,
> -				  dsi->mode.clock * 1000 * bpp);
> +	return DIV_ROUND_UP(ui, BITS_PER_BYTE);
>   }
>   
>   /*
> @@ -232,12 +230,12 @@ static int nwl_dsi_config_host(struct nwl_dsi *dsi)
>   	}
>   
>   	/* values in byte clock cycles */
> -	cycles = ui2bc(dsi, cfg->clk_pre);
> +	cycles = ui2bc(cfg->clk_pre);
>   	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_pre: 0x%x\n", cycles);
>   	nwl_dsi_write(dsi, NWL_DSI_CFG_T_PRE, cycles);
>   	cycles = ps2bc(dsi, cfg->lpx + cfg->clk_prepare + cfg->clk_zero);
>   	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap (pre): 0x%x\n", cycles);
> -	cycles += ui2bc(dsi, cfg->clk_pre);
> +	cycles += ui2bc(cfg->clk_pre);
>   	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_post: 0x%x\n", cycles);
>   	nwl_dsi_write(dsi, NWL_DSI_CFG_T_POST, cycles);
>   	cycles = ps2bc(dsi, cfg->hs_exit);
> 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) |
> diff --git a/drivers/phy/phy-core-mipi-dphy.c b/drivers/phy/phy-core-mipi-dphy.c
> index 288c9c67aa74..ccb4045685cd 100644
> --- a/drivers/phy/phy-core-mipi-dphy.c
> +++ b/drivers/phy/phy-core-mipi-dphy.c
> @@ -36,7 +36,7 @@ int phy_mipi_dphy_get_default_config(unsigned long pixel_clock,
>   
>   	cfg->clk_miss = 0;
>   	cfg->clk_post = 60000 + 52 * ui;
> -	cfg->clk_pre = 8000;
> +	cfg->clk_pre = 8;
>   	cfg->clk_prepare = 38000;
>   	cfg->clk_settle = 95000;
>   	cfg->clk_term_en = 0;
> @@ -97,7 +97,7 @@ int phy_mipi_dphy_config_validate(struct phy_configure_opts_mipi_dphy *cfg)
>   	if (cfg->clk_post < (60000 + 52 * ui))
>   		return -EINVAL;
>   
> -	if (cfg->clk_pre < 8000)
> +	if (cfg->clk_pre < 8)
>   		return -EINVAL;
>   
>   	if (cfg->clk_prepare < 38000 || cfg->clk_prepare > 95000)
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> index 347dc79a18c1..630e01b5c19b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> @@ -5,6 +5,7 @@
>    * Author: Wyon Bi <bivvy.bi@rock-chips.com>
>    */
>   
> +#include <linux/bits.h>
>   #include <linux/kernel.h>
>   #include <linux/clk.h>
>   #include <linux/iopoll.h>
> @@ -364,7 +365,7 @@ static void inno_dsidphy_mipi_mode_enable(struct inno_dsidphy *inno)
>   	 * The value of counter for HS Tclk-pre
>   	 * Tclk-pre = Tpin_txbyteclkhs * value
>   	 */
> -	clk_pre = DIV_ROUND_UP(cfg->clk_pre, t_txbyteclkhs);
> +	clk_pre = DIV_ROUND_UP(cfg->clk_pre, BITS_PER_BYTE);
>   
>   	/*
>   	 * The value of counter for HS Tlpx Time
> diff --git a/include/linux/phy/phy-mipi-dphy.h b/include/linux/phy/phy-mipi-dphy.h
> index a877ffee845d..59a5e77ab409 100644
> --- a/include/linux/phy/phy-mipi-dphy.h
> +++ b/include/linux/phy/phy-mipi-dphy.h
> @@ -42,7 +42,7 @@ struct phy_configure_opts_mipi_dphy {
>   	 * the transmitter prior to any associated Data Lane beginning
>   	 * the transition from LP to HS mode.
>   	 *
> -	 * Minimum value: 8 UI
> +	 * Minimum value: 8
>   	 */
>   	unsigned int		clk_pre;
>
Neil Armstrong Jan. 19, 2022, 8:40 a.m. UTC | #2
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@intel.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Guido Günther <agx@sigxcpu.org>
> Tested-by: Liu Ying <victor.liu@nxp.com> # RM67191 DSI panel on i.MX8mq EVK
> Signed-off-by: Liu Ying <victor.liu@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/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> index a7389a0facfb..af07eeb47ca0 100644
> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/bits.h>
>  #include <linux/clk.h>
>  #include <linux/irq.h>
>  #include <linux/math64.h>
> @@ -196,12 +197,9 @@ static u32 ps2bc(struct nwl_dsi *dsi, unsigned long long ps)
>  /*
>   * ui2bc - UI time periods to byte clock cycles
>   */
> -static u32 ui2bc(struct nwl_dsi *dsi, unsigned long long ui)
> +static u32 ui2bc(unsigned int ui)
>  {
> -	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> -
> -	return DIV64_U64_ROUND_UP(ui * dsi->lanes,
> -				  dsi->mode.clock * 1000 * bpp);
> +	return DIV_ROUND_UP(ui, BITS_PER_BYTE);
>  }
>  
>  /*
> @@ -232,12 +230,12 @@ static int nwl_dsi_config_host(struct nwl_dsi *dsi)
>  	}
>  
>  	/* values in byte clock cycles */
> -	cycles = ui2bc(dsi, cfg->clk_pre);
> +	cycles = ui2bc(cfg->clk_pre);
>  	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_pre: 0x%x\n", cycles);
>  	nwl_dsi_write(dsi, NWL_DSI_CFG_T_PRE, cycles);
>  	cycles = ps2bc(dsi, cfg->lpx + cfg->clk_prepare + cfg->clk_zero);
>  	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap (pre): 0x%x\n", cycles);
> -	cycles += ui2bc(dsi, cfg->clk_pre);
> +	cycles += ui2bc(cfg->clk_pre);
>  	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_post: 0x%x\n", cycles);
>  	nwl_dsi_write(dsi, NWL_DSI_CFG_T_POST, cycles);
>  	cycles = ps2bc(dsi, cfg->hs_exit);
> 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.

Neil

> diff --git a/drivers/phy/phy-core-mipi-dphy.c b/drivers/phy/phy-core-mipi-dphy.c
> index 288c9c67aa74..ccb4045685cd 100644
> --- a/drivers/phy/phy-core-mipi-dphy.c
> +++ b/drivers/phy/phy-core-mipi-dphy.c
> @@ -36,7 +36,7 @@ int phy_mipi_dphy_get_default_config(unsigned long pixel_clock,
>  
>  	cfg->clk_miss = 0;
>  	cfg->clk_post = 60000 + 52 * ui;
> -	cfg->clk_pre = 8000;
> +	cfg->clk_pre = 8;
>  	cfg->clk_prepare = 38000;
>  	cfg->clk_settle = 95000;
>  	cfg->clk_term_en = 0;
> @@ -97,7 +97,7 @@ int phy_mipi_dphy_config_validate(struct phy_configure_opts_mipi_dphy *cfg)
>  	if (cfg->clk_post < (60000 + 52 * ui))
>  		return -EINVAL;
>  
> -	if (cfg->clk_pre < 8000)
> +	if (cfg->clk_pre < 8)
>  		return -EINVAL;
>  
>  	if (cfg->clk_prepare < 38000 || cfg->clk_prepare > 95000)
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> index 347dc79a18c1..630e01b5c19b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> @@ -5,6 +5,7 @@
>   * Author: Wyon Bi <bivvy.bi@rock-chips.com>
>   */
>  
> +#include <linux/bits.h>
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
>  #include <linux/iopoll.h>
> @@ -364,7 +365,7 @@ static void inno_dsidphy_mipi_mode_enable(struct inno_dsidphy *inno)
>  	 * The value of counter for HS Tclk-pre
>  	 * Tclk-pre = Tpin_txbyteclkhs * value
>  	 */
> -	clk_pre = DIV_ROUND_UP(cfg->clk_pre, t_txbyteclkhs);
> +	clk_pre = DIV_ROUND_UP(cfg->clk_pre, BITS_PER_BYTE);
>  
>  	/*
>  	 * The value of counter for HS Tlpx Time
> diff --git a/include/linux/phy/phy-mipi-dphy.h b/include/linux/phy/phy-mipi-dphy.h
> index a877ffee845d..59a5e77ab409 100644
> --- a/include/linux/phy/phy-mipi-dphy.h
> +++ b/include/linux/phy/phy-mipi-dphy.h
> @@ -42,7 +42,7 @@ struct phy_configure_opts_mipi_dphy {
>  	 * the transmitter prior to any associated Data Lane beginning
>  	 * the transition from LP to HS mode.
>  	 *
> -	 * Minimum value: 8 UI
> +	 * Minimum value: 8
>  	 */
>  	unsigned int		clk_pre;
>  
>
Neil Armstrong Jan. 19, 2022, 9:11 a.m. UTC | #3
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@intel.com>
>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>> Cc: Robert Foss <robert.foss@linaro.org>
>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>> Cc: Jonas Karlman <jonas@kwiboo.se>
>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>> Cc: Vinod Koul <vkoul@kernel.org>
>> Cc: Kevin Hilman <khilman@baylibre.com>
>> Cc: Jerome Brunet <jbrunet@baylibre.com>
>> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Cc: Heiko Stuebner <heiko@sntech.de>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Guido Günther <agx@sigxcpu.org>
>> Tested-by: Liu Ying <victor.liu@nxp.com> # RM67191 DSI panel on i.MX8mq EVK
>> Signed-off-by: Liu Ying <victor.liu@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)

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
> 

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

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
>>>
>>
>> [...]
>
Neil Armstrong Jan. 19, 2022, 10:47 a.m. UTC | #6
Hi,

On 19/01/2022 11:11, Neil Armstrong wrote:
> 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@intel.com>
>>>>> Cc: Neil Armstrong <narmstrong@baylibre.com>
>>>>> Cc: Robert Foss <robert.foss@linaro.org>
>>>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>>>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>>>>> Cc: David Airlie <airlied@linux.ie>
>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>>>>> Cc: Vinod Koul <vkoul@kernel.org>
>>>>> Cc: Kevin Hilman <khilman@baylibre.com>
>>>>> Cc: Jerome Brunet <jbrunet@baylibre.com>
>>>>> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>>> Cc: Heiko Stuebner <heiko@sntech.de>
>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>> Cc: Guido Günther <agx@sigxcpu.org>
>>>>> Tested-by: Liu Ying <victor.liu@nxp.com> # RM67191 DSI panel on i.MX8mq EVK
>>>>> Signed-off-by: Liu Ying <victor.liu@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?
> 
> Thanks for the feedback, I'm currently trying to put the setup back to
> run a test with this change.

I don't see any visible artifacts.

Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> # for phy-meson-axg-mipi-dphy.c
Tested-by: Neil Armstrong <narmstrong@baylibre.com> # for phy-meson-axg-mipi-dphy.c

Thanks,

> 
> Neil
> 
>>
>> Regards,
>> Liu Ying
>>
>>>
>>> PHY Registers only says:
>>> MIPI_DSI_CLK_TIM1	[31:0]
>>> 7:0 	R/W	0	 tCLK_PRE
>>>
>>>> Neil
>>>>
>>>
>>> [...]
>>
>
Guido Günther Jan. 19, 2022, 11:47 a.m. UTC | #7
Hi,
On Wed, Jan 19, 2022 at 10:37:14AM +0800, 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@intel.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Guido Günther <agx@sigxcpu.org>
> Tested-by: Liu Ying <victor.liu@nxp.com> # RM67191 DSI panel on i.MX8mq EVK
> Signed-off-by: Liu Ying <victor.liu@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/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> index a7389a0facfb..af07eeb47ca0 100644
> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/bits.h>
>  #include <linux/clk.h>
>  #include <linux/irq.h>
>  #include <linux/math64.h>
> @@ -196,12 +197,9 @@ static u32 ps2bc(struct nwl_dsi *dsi, unsigned long long ps)
>  /*
>   * ui2bc - UI time periods to byte clock cycles
>   */
> -static u32 ui2bc(struct nwl_dsi *dsi, unsigned long long ui)
> +static u32 ui2bc(unsigned int ui)
>  {
> -	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> -
> -	return DIV64_U64_ROUND_UP(ui * dsi->lanes,
> -				  dsi->mode.clock * 1000 * bpp);
> +	return DIV_ROUND_UP(ui, BITS_PER_BYTE);
>  }
>  
>  /*
> @@ -232,12 +230,12 @@ static int nwl_dsi_config_host(struct nwl_dsi *dsi)
>  	}
>  
>  	/* values in byte clock cycles */
> -	cycles = ui2bc(dsi, cfg->clk_pre);
> +	cycles = ui2bc(cfg->clk_pre);
>  	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_pre: 0x%x\n", cycles);
>  	nwl_dsi_write(dsi, NWL_DSI_CFG_T_PRE, cycles);
>  	cycles = ps2bc(dsi, cfg->lpx + cfg->clk_prepare + cfg->clk_zero);
>  	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap (pre): 0x%x\n", cycles);
> -	cycles += ui2bc(dsi, cfg->clk_pre);
> +	cycles += ui2bc(cfg->clk_pre);
>  	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_post: 0x%x\n", cycles);
>  	nwl_dsi_write(dsi, NWL_DSI_CFG_T_POST, cycles);
>  	cycles = ps2bc(dsi, cfg->hs_exit);
> 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) |
> diff --git a/drivers/phy/phy-core-mipi-dphy.c b/drivers/phy/phy-core-mipi-dphy.c
> index 288c9c67aa74..ccb4045685cd 100644
> --- a/drivers/phy/phy-core-mipi-dphy.c
> +++ b/drivers/phy/phy-core-mipi-dphy.c
> @@ -36,7 +36,7 @@ int phy_mipi_dphy_get_default_config(unsigned long pixel_clock,
>  
>  	cfg->clk_miss = 0;
>  	cfg->clk_post = 60000 + 52 * ui;
> -	cfg->clk_pre = 8000;
> +	cfg->clk_pre = 8;
>  	cfg->clk_prepare = 38000;
>  	cfg->clk_settle = 95000;
>  	cfg->clk_term_en = 0;
> @@ -97,7 +97,7 @@ int phy_mipi_dphy_config_validate(struct phy_configure_opts_mipi_dphy *cfg)
>  	if (cfg->clk_post < (60000 + 52 * ui))
>  		return -EINVAL;
>  
> -	if (cfg->clk_pre < 8000)
> +	if (cfg->clk_pre < 8)
>  		return -EINVAL;
>  
>  	if (cfg->clk_prepare < 38000 || cfg->clk_prepare > 95000)
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> index 347dc79a18c1..630e01b5c19b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> @@ -5,6 +5,7 @@
>   * Author: Wyon Bi <bivvy.bi@rock-chips.com>
>   */
>  
> +#include <linux/bits.h>
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
>  #include <linux/iopoll.h>
> @@ -364,7 +365,7 @@ static void inno_dsidphy_mipi_mode_enable(struct inno_dsidphy *inno)
>  	 * The value of counter for HS Tclk-pre
>  	 * Tclk-pre = Tpin_txbyteclkhs * value
>  	 */
> -	clk_pre = DIV_ROUND_UP(cfg->clk_pre, t_txbyteclkhs);
> +	clk_pre = DIV_ROUND_UP(cfg->clk_pre, BITS_PER_BYTE);
>  
>  	/*
>  	 * The value of counter for HS Tlpx Time
> diff --git a/include/linux/phy/phy-mipi-dphy.h b/include/linux/phy/phy-mipi-dphy.h
> index a877ffee845d..59a5e77ab409 100644
> --- a/include/linux/phy/phy-mipi-dphy.h
> +++ b/include/linux/phy/phy-mipi-dphy.h
> @@ -42,7 +42,7 @@ struct phy_configure_opts_mipi_dphy {
>  	 * the transmitter prior to any associated Data Lane beginning
>  	 * the transition from LP to HS mode.
>  	 *
> -	 * Minimum value: 8 UI
> +	 * Minimum value: 8
>  	 */
>  	unsigned int		clk_pre;


Tested on the Librem 5 (imx8mq) with it's rather picky panel:

Tested-by: Guido Günther <agx@sigxcpu.org>

Cheers
 -- Guido


>  
> -- 
> 2.25.1
>
Liu Ying Jan. 20, 2022, 3 a.m. UTC | #8
Hi Heiko, Wyon,

On Wed, 2022-01-19 at 10:37 +0800, 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@intel.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Guido Günther <agx@sigxcpu.org>
> Tested-by: Liu Ying <victor.liu@nxp.com> # RM67191 DSI panel on i.MX8mq EVK
> Signed-off-by: Liu Ying <victor.liu@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/phy-core-mipi-dphy.c b/drivers/phy/phy-core-mipi-dphy.c
> index 288c9c67aa74..ccb4045685cd 100644
> --- a/drivers/phy/phy-core-mipi-dphy.c
> +++ b/drivers/phy/phy-core-mipi-dphy.c
> @@ -36,7 +36,7 @@ int phy_mipi_dphy_get_default_config(unsigned long pixel_clock,
>  
>  	cfg->clk_miss = 0;
>  	cfg->clk_post = 60000 + 52 * ui;
> -	cfg->clk_pre = 8000;
> +	cfg->clk_pre = 8;
>  	cfg->clk_prepare = 38000;
>  	cfg->clk_settle = 95000;
>  	cfg->clk_term_en = 0;
> @@ -97,7 +97,7 @@ int phy_mipi_dphy_config_validate(struct phy_configure_opts_mipi_dphy *cfg)
>  	if (cfg->clk_post < (60000 + 52 * ui))
>  		return -EINVAL;
>  
> -	if (cfg->clk_pre < 8000)
> +	if (cfg->clk_pre < 8)
>  		return -EINVAL;
>  
>  	if (cfg->clk_prepare < 38000 || cfg->clk_prepare > 95000)
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> index 347dc79a18c1..630e01b5c19b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> @@ -5,6 +5,7 @@
>   * Author: Wyon Bi <bivvy.bi@rock-chips.com>
>   */
>  
> +#include <linux/bits.h>
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
>  #include <linux/iopoll.h>
> @@ -364,7 +365,7 @@ static void inno_dsidphy_mipi_mode_enable(struct inno_dsidphy *inno)
>  	 * The value of counter for HS Tclk-pre
>  	 * Tclk-pre = Tpin_txbyteclkhs * value
>  	 */
> -	clk_pre = DIV_ROUND_UP(cfg->clk_pre, t_txbyteclkhs);
> +	clk_pre = DIV_ROUND_UP(cfg->clk_pre, BITS_PER_BYTE);

For the Rockchip part, can you please give a test? Any comments?

We already have T-b tags on i.MX8mq and Meson platforms.

Regards,
Liu Ying

>  
>  	/*
>  	 * The value of counter for HS Tlpx Time
> diff --git a/include/linux/phy/phy-mipi-dphy.h b/include/linux/phy/phy-mipi-dphy.h
> index a877ffee845d..59a5e77ab409 100644
> --- a/include/linux/phy/phy-mipi-dphy.h
> +++ b/include/linux/phy/phy-mipi-dphy.h
> @@ -42,7 +42,7 @@ struct phy_configure_opts_mipi_dphy {
>  	 * the transmitter prior to any associated Data Lane beginning
>  	 * the transition from LP to HS mode.
>  	 *
> -	 * Minimum value: 8 UI
> +	 * Minimum value: 8
>  	 */
>  	unsigned int		clk_pre;
>
Laurent Pinchart Jan. 23, 2022, 10:15 p.m. UTC | #9
Hi Liu,

Thank you for the patch.

On Wed, Jan 19, 2022 at 10:37:14AM +0800, 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.

I'm not sure I'd change the kernel doc. Other fields that are documented
as "Time, in picoseconds" have min/max values expressed in ps, so
clk_pre, documented as "Time, in UI" doesn't appear wrong if the minimum
value is "8 UI".

Otherwise, this patch looks fine to me. With or without dropping that
documentation change,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 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@intel.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Cc: Vinod Koul <vkoul@kernel.org>
> Cc: Kevin Hilman <khilman@baylibre.com>
> Cc: Jerome Brunet <jbrunet@baylibre.com>
> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Guido Günther <agx@sigxcpu.org>
> Tested-by: Liu Ying <victor.liu@nxp.com> # RM67191 DSI panel on i.MX8mq EVK
> Signed-off-by: Liu Ying <victor.liu@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/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> index a7389a0facfb..af07eeb47ca0 100644
> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/bits.h>
>  #include <linux/clk.h>
>  #include <linux/irq.h>
>  #include <linux/math64.h>
> @@ -196,12 +197,9 @@ static u32 ps2bc(struct nwl_dsi *dsi, unsigned long long ps)
>  /*
>   * ui2bc - UI time periods to byte clock cycles
>   */
> -static u32 ui2bc(struct nwl_dsi *dsi, unsigned long long ui)
> +static u32 ui2bc(unsigned int ui)
>  {
> -	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> -
> -	return DIV64_U64_ROUND_UP(ui * dsi->lanes,
> -				  dsi->mode.clock * 1000 * bpp);
> +	return DIV_ROUND_UP(ui, BITS_PER_BYTE);
>  }
>  
>  /*
> @@ -232,12 +230,12 @@ static int nwl_dsi_config_host(struct nwl_dsi *dsi)
>  	}
>  
>  	/* values in byte clock cycles */
> -	cycles = ui2bc(dsi, cfg->clk_pre);
> +	cycles = ui2bc(cfg->clk_pre);
>  	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_pre: 0x%x\n", cycles);
>  	nwl_dsi_write(dsi, NWL_DSI_CFG_T_PRE, cycles);
>  	cycles = ps2bc(dsi, cfg->lpx + cfg->clk_prepare + cfg->clk_zero);
>  	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap (pre): 0x%x\n", cycles);
> -	cycles += ui2bc(dsi, cfg->clk_pre);
> +	cycles += ui2bc(cfg->clk_pre);
>  	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_post: 0x%x\n", cycles);
>  	nwl_dsi_write(dsi, NWL_DSI_CFG_T_POST, cycles);
>  	cycles = ps2bc(dsi, cfg->hs_exit);
> 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) |
> diff --git a/drivers/phy/phy-core-mipi-dphy.c b/drivers/phy/phy-core-mipi-dphy.c
> index 288c9c67aa74..ccb4045685cd 100644
> --- a/drivers/phy/phy-core-mipi-dphy.c
> +++ b/drivers/phy/phy-core-mipi-dphy.c
> @@ -36,7 +36,7 @@ int phy_mipi_dphy_get_default_config(unsigned long pixel_clock,
>  
>  	cfg->clk_miss = 0;
>  	cfg->clk_post = 60000 + 52 * ui;
> -	cfg->clk_pre = 8000;
> +	cfg->clk_pre = 8;
>  	cfg->clk_prepare = 38000;
>  	cfg->clk_settle = 95000;
>  	cfg->clk_term_en = 0;
> @@ -97,7 +97,7 @@ int phy_mipi_dphy_config_validate(struct phy_configure_opts_mipi_dphy *cfg)
>  	if (cfg->clk_post < (60000 + 52 * ui))
>  		return -EINVAL;
>  
> -	if (cfg->clk_pre < 8000)
> +	if (cfg->clk_pre < 8)
>  		return -EINVAL;
>  
>  	if (cfg->clk_prepare < 38000 || cfg->clk_prepare > 95000)
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> index 347dc79a18c1..630e01b5c19b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> @@ -5,6 +5,7 @@
>   * Author: Wyon Bi <bivvy.bi@rock-chips.com>
>   */
>  
> +#include <linux/bits.h>
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
>  #include <linux/iopoll.h>
> @@ -364,7 +365,7 @@ static void inno_dsidphy_mipi_mode_enable(struct inno_dsidphy *inno)
>  	 * The value of counter for HS Tclk-pre
>  	 * Tclk-pre = Tpin_txbyteclkhs * value
>  	 */
> -	clk_pre = DIV_ROUND_UP(cfg->clk_pre, t_txbyteclkhs);
> +	clk_pre = DIV_ROUND_UP(cfg->clk_pre, BITS_PER_BYTE);
>  
>  	/*
>  	 * The value of counter for HS Tlpx Time
> diff --git a/include/linux/phy/phy-mipi-dphy.h b/include/linux/phy/phy-mipi-dphy.h
> index a877ffee845d..59a5e77ab409 100644
> --- a/include/linux/phy/phy-mipi-dphy.h
> +++ b/include/linux/phy/phy-mipi-dphy.h
> @@ -42,7 +42,7 @@ struct phy_configure_opts_mipi_dphy {
>  	 * the transmitter prior to any associated Data Lane beginning
>  	 * the transition from LP to HS mode.
>  	 *
> -	 * Minimum value: 8 UI
> +	 * Minimum value: 8
>  	 */
>  	unsigned int		clk_pre;
>
Liu Ying Jan. 24, 2022, 2:31 a.m. UTC | #10
Hi Laurent,

On Mon, 2022-01-24 at 00:15 +0200, Laurent Pinchart wrote:
> Hi Liu,
> 
> Thank you for the patch.

Thank you for your review.

> 
> On Wed, Jan 19, 2022 at 10:37:14AM +0800, 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.
> 
> I'm not sure I'd change the kernel doc. Other fields that are documented
> as "Time, in picoseconds" have min/max values expressed in ps, so
> clk_pre, documented as "Time, in UI" doesn't appear wrong if the minimum
> value is "8 UI".
> 
> Otherwise, this patch looks fine to me. With or without dropping that
> documentation change,

I'll drop the documentation change in v3.

Regards,
Liu Ying

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
index a7389a0facfb..af07eeb47ca0 100644
--- a/drivers/gpu/drm/bridge/nwl-dsi.c
+++ b/drivers/gpu/drm/bridge/nwl-dsi.c
@@ -7,6 +7,7 @@ 
  */
 
 #include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/clk.h>
 #include <linux/irq.h>
 #include <linux/math64.h>
@@ -196,12 +197,9 @@  static u32 ps2bc(struct nwl_dsi *dsi, unsigned long long ps)
 /*
  * ui2bc - UI time periods to byte clock cycles
  */
-static u32 ui2bc(struct nwl_dsi *dsi, unsigned long long ui)
+static u32 ui2bc(unsigned int ui)
 {
-	u32 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
-
-	return DIV64_U64_ROUND_UP(ui * dsi->lanes,
-				  dsi->mode.clock * 1000 * bpp);
+	return DIV_ROUND_UP(ui, BITS_PER_BYTE);
 }
 
 /*
@@ -232,12 +230,12 @@  static int nwl_dsi_config_host(struct nwl_dsi *dsi)
 	}
 
 	/* values in byte clock cycles */
-	cycles = ui2bc(dsi, cfg->clk_pre);
+	cycles = ui2bc(cfg->clk_pre);
 	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_pre: 0x%x\n", cycles);
 	nwl_dsi_write(dsi, NWL_DSI_CFG_T_PRE, cycles);
 	cycles = ps2bc(dsi, cfg->lpx + cfg->clk_prepare + cfg->clk_zero);
 	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_tx_gap (pre): 0x%x\n", cycles);
-	cycles += ui2bc(dsi, cfg->clk_pre);
+	cycles += ui2bc(cfg->clk_pre);
 	DRM_DEV_DEBUG_DRIVER(dsi->dev, "cfg_t_post: 0x%x\n", cycles);
 	nwl_dsi_write(dsi, NWL_DSI_CFG_T_POST, cycles);
 	cycles = ps2bc(dsi, cfg->hs_exit);
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) |
diff --git a/drivers/phy/phy-core-mipi-dphy.c b/drivers/phy/phy-core-mipi-dphy.c
index 288c9c67aa74..ccb4045685cd 100644
--- a/drivers/phy/phy-core-mipi-dphy.c
+++ b/drivers/phy/phy-core-mipi-dphy.c
@@ -36,7 +36,7 @@  int phy_mipi_dphy_get_default_config(unsigned long pixel_clock,
 
 	cfg->clk_miss = 0;
 	cfg->clk_post = 60000 + 52 * ui;
-	cfg->clk_pre = 8000;
+	cfg->clk_pre = 8;
 	cfg->clk_prepare = 38000;
 	cfg->clk_settle = 95000;
 	cfg->clk_term_en = 0;
@@ -97,7 +97,7 @@  int phy_mipi_dphy_config_validate(struct phy_configure_opts_mipi_dphy *cfg)
 	if (cfg->clk_post < (60000 + 52 * ui))
 		return -EINVAL;
 
-	if (cfg->clk_pre < 8000)
+	if (cfg->clk_pre < 8)
 		return -EINVAL;
 
 	if (cfg->clk_prepare < 38000 || cfg->clk_prepare > 95000)
diff --git a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
index 347dc79a18c1..630e01b5c19b 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
@@ -5,6 +5,7 @@ 
  * Author: Wyon Bi <bivvy.bi@rock-chips.com>
  */
 
+#include <linux/bits.h>
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/iopoll.h>
@@ -364,7 +365,7 @@  static void inno_dsidphy_mipi_mode_enable(struct inno_dsidphy *inno)
 	 * The value of counter for HS Tclk-pre
 	 * Tclk-pre = Tpin_txbyteclkhs * value
 	 */
-	clk_pre = DIV_ROUND_UP(cfg->clk_pre, t_txbyteclkhs);
+	clk_pre = DIV_ROUND_UP(cfg->clk_pre, BITS_PER_BYTE);
 
 	/*
 	 * The value of counter for HS Tlpx Time
diff --git a/include/linux/phy/phy-mipi-dphy.h b/include/linux/phy/phy-mipi-dphy.h
index a877ffee845d..59a5e77ab409 100644
--- a/include/linux/phy/phy-mipi-dphy.h
+++ b/include/linux/phy/phy-mipi-dphy.h
@@ -42,7 +42,7 @@  struct phy_configure_opts_mipi_dphy {
 	 * the transmitter prior to any associated Data Lane beginning
 	 * the transition from LP to HS mode.
 	 *
-	 * Minimum value: 8 UI
+	 * Minimum value: 8
 	 */
 	unsigned int		clk_pre;