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, MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD), 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 <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Robert Foss <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Jonas Karlman <[email protected]>
Cc: Jernej Skrabec <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Kishon Vijay Abraham I <[email protected]>
Cc: Vinod Koul <[email protected]>
Cc: Kevin Hilman <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Martin Blumenstingl <[email protected]>
Cc: Heiko Stuebner <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Guido Günther <[email protected]>
Tested-by: Liu Ying <[email protected]> # RM67191 DSI panel on i.MX8mq EVK
Signed-off-by: Liu Ying <[email protected]>
---
drivers/gpu/drm/bridge/nwl-dsi.c | 7 ++-----
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 | 4 +++-
5 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
index a7389a0facfb..f1fdcdf763ee 100644
--- a/drivers/gpu/drm/bridge/nwl-dsi.c
+++ b/drivers/gpu/drm/bridge/nwl-dsi.c
@@ -196,12 +196,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(struct nwl_dsi *dsi, 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, MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD);
}
/*
diff --git a/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c b/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
index cd2332bf0e31..8a818cdb7606 100644
--- a/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
+++ b/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
@@ -250,7 +250,8 @@ 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,
+ MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD));
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..67b0a17be7e3 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
@@ -364,7 +364,8 @@ 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,
+ MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD);
/*
* 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..ab1f736fbcd3 100644
--- a/include/linux/phy/phy-mipi-dphy.h
+++ b/include/linux/phy/phy-mipi-dphy.h
@@ -6,6 +6,8 @@
#ifndef __PHY_MIPI_DPHY_H_
#define __PHY_MIPI_DPHY_H_
+#define MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD 8
+
/**
* struct phy_configure_opts_mipi_dphy - MIPI D-PHY configuration set
*
@@ -42,7 +44,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;
--
2.25.1
Hi,
On 18.01.2022 03:59, 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, MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD), 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 <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> Cc: Robert Foss <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: Jonas Karlman <[email protected]>
> Cc: Jernej Skrabec <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Kishon Vijay Abraham I <[email protected]>
> Cc: Vinod Koul <[email protected]>
> Cc: Kevin Hilman <[email protected]>
> Cc: Jerome Brunet <[email protected]>
> Cc: Martin Blumenstingl <[email protected]>
> Cc: Heiko Stuebner <[email protected]>
> Cc: Maxime Ripard <[email protected]>
> Cc: Guido Günther <[email protected]>
> Tested-by: Liu Ying <[email protected]> # RM67191 DSI panel on i.MX8mq EVK
> Signed-off-by: Liu Ying <[email protected]>
> ---
> drivers/gpu/drm/bridge/nwl-dsi.c | 7 ++-----
> 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 | 4 +++-
> 5 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> index a7389a0facfb..f1fdcdf763ee 100644
> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> @@ -196,12 +196,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(struct nwl_dsi *dsi, 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, MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD);
I have some doubts here. According to specs:
UI - duration of any HS state on the Clock Lane,
TxByteClkHS - exactly 1/8 the High-Speed(HS) bit rate
I'd like to emphasize "BIT RATE" above (not clock rate).
In such case I would expect that ui2bc would take into account number of
lanes:
byte clock cycles = div_round_up(ui * dsi->lanes, 8)
So in case of one lane we need 8 ticks to get byte, and in case of 4
lanes 2 ticks are enough.
Am I correct, or I've missed sth?
> }
>
> /*
> diff --git a/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c b/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
> index cd2332bf0e31..8a818cdb7606 100644
> --- a/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
> +++ b/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
> @@ -250,7 +250,8 @@ 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,
> + MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD));
>
> 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..67b0a17be7e3 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> @@ -364,7 +364,8 @@ 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,
> + MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD);
>
> /*
> * 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..ab1f736fbcd3 100644
> --- a/include/linux/phy/phy-mipi-dphy.h
> +++ b/include/linux/phy/phy-mipi-dphy.h
> @@ -6,6 +6,8 @@
> #ifndef __PHY_MIPI_DPHY_H_
> #define __PHY_MIPI_DPHY_H_
>
> +#define MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD 8
Do we need to define it? I guess it is just BITS_PER_BYTE.
Regards
Andrzej
> +
> /**
> * struct phy_configure_opts_mipi_dphy - MIPI D-PHY configuration set
> *
> @@ -42,7 +44,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;
>
Hi Andrzej,
Thanks for your review.
On Tue, 2022-01-18 at 10:24 +0100, Andrzej Hajda wrote:
> Hi,
>
> On 18.01.2022 03:59, 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, MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD), 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 <[email protected]>
> > Cc: Neil Armstrong <[email protected]>
> > Cc: Robert Foss <[email protected]>
> > Cc: Laurent Pinchart <[email protected]>
> > Cc: Jonas Karlman <[email protected]>
> > Cc: Jernej Skrabec <[email protected]>
> > Cc: David Airlie <[email protected]>
> > Cc: Daniel Vetter <[email protected]>
> > Cc: Kishon Vijay Abraham I <[email protected]>
> > Cc: Vinod Koul <[email protected]>
> > Cc: Kevin Hilman <[email protected]>
> > Cc: Jerome Brunet <[email protected]>
> > Cc: Martin Blumenstingl <[email protected]>
> > Cc: Heiko Stuebner <[email protected]>
> > Cc: Maxime Ripard <[email protected]>
> > Cc: Guido Günther <[email protected]>
> > Tested-by: Liu Ying <[email protected]> # RM67191 DSI panel on i.MX8mq EVK
> > Signed-off-by: Liu Ying <[email protected]>
> > ---
> > drivers/gpu/drm/bridge/nwl-dsi.c | 7 ++-----
> > 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 | 4 +++-
> > 5 files changed, 11 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
> > index a7389a0facfb..f1fdcdf763ee 100644
> > --- a/drivers/gpu/drm/bridge/nwl-dsi.c
> > +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
> > @@ -196,12 +196,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(struct nwl_dsi *dsi, 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, MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD);
>
> I have some doubts here. According to specs:
>
> UI - duration of any HS state on the Clock Lane,
>
> TxByteClkHS - exactly 1/8 the High-Speed(HS) bit rate
>
> I'd like to emphasize "BIT RATE" above (not clock rate).
>
> In such case I would expect that ui2bc would take into account number of
> lanes:
>
> byte clock cycles = div_round_up(ui * dsi->lanes, 8)
>
> So in case of one lane we need 8 ticks to get byte, and in case of 4
> lanes 2 ticks are enough.
>
> Am I correct, or I've missed sth?
Sorry, I think you are wrong.
The spec also mentions that it is recommended that all transmitting
Data Lane Modules share one TxByteClkHS signal. So, usually,
TxByteClkHS has nothing to do with data lane number, but only UI - one
bit period is HS state. I think the NWL DSI follows the recommended
implementation.
'Table 20 HS Transmitter AC Specifications' in the spec notes
that Applicable when supporting maximum HS bit rates ≤ 1 Gbps (UI ≥ 1
ns). This hints that HS bit rate is 1/UI.
>
>
> > }
> >
> > /*
> > diff --git a/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c b/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
> > index cd2332bf0e31..8a818cdb7606 100644
> > --- a/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
> > +++ b/drivers/phy/amlogic/phy-meson-axg-mipi-dphy.c
> > @@ -250,7 +250,8 @@ 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,
> > + MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD));
> >
> > 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..67b0a17be7e3 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-inno-dsidphy.c
> > @@ -364,7 +364,8 @@ 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,
> > + MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD);
> >
> > /*
> > * 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..ab1f736fbcd3 100644
> > --- a/include/linux/phy/phy-mipi-dphy.h
> > +++ b/include/linux/phy/phy-mipi-dphy.h
> > @@ -6,6 +6,8 @@
> > #ifndef __PHY_MIPI_DPHY_H_
> > #define __PHY_MIPI_DPHY_H_
> >
> > +#define MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD 8
>
> Do we need to define it? I guess it is just BITS_PER_BYTE.
Either way would do I think. I'll wait for a while to see if others
have comment here. If no, I may switch to use BITS_PER_BYTE in v2.
Regards,
Liu Ying
>
>
> Regards
>
> Andrzej
>
>
> > +
> > /**
> > * struct phy_configure_opts_mipi_dphy - MIPI D-PHY configuration set
> > *
> > @@ -42,7 +44,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;
> >
On 18.01.2022 11:14, Liu Ying wrote:
> Hi Andrzej,
>
> Thanks for your review.
>
> On Tue, 2022-01-18 at 10:24 +0100, Andrzej Hajda wrote:
>> Hi,
>>
>> On 18.01.2022 03:59, 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, MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD), 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 <[email protected]>
>>> Cc: Neil Armstrong <[email protected]>
>>> Cc: Robert Foss <[email protected]>
>>> Cc: Laurent Pinchart <[email protected]>
>>> Cc: Jonas Karlman <[email protected]>
>>> Cc: Jernej Skrabec <[email protected]>
>>> Cc: David Airlie <[email protected]>
>>> Cc: Daniel Vetter <[email protected]>
>>> Cc: Kishon Vijay Abraham I <[email protected]>
>>> Cc: Vinod Koul <[email protected]>
>>> Cc: Kevin Hilman <[email protected]>
>>> Cc: Jerome Brunet <[email protected]>
>>> Cc: Martin Blumenstingl <[email protected]>
>>> Cc: Heiko Stuebner <[email protected]>
>>> Cc: Maxime Ripard <[email protected]>
>>> Cc: Guido Günther <[email protected]>
>>> Tested-by: Liu Ying <[email protected]> # RM67191 DSI panel on i.MX8mq EVK
>>> Signed-off-by: Liu Ying <[email protected]>
>>> ---
>>> drivers/gpu/drm/bridge/nwl-dsi.c | 7 ++-----
>>> 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 | 4 +++-
>>> 5 files changed, 11 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
>>> index a7389a0facfb..f1fdcdf763ee 100644
>>> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
>>> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
>>> @@ -196,12 +196,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(struct nwl_dsi *dsi, 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, MIPI_DPHY_UI_PER_TXBYTECLKHS_PERIOD);
>> I have some doubts here. According to specs:
>>
>> UI - duration of any HS state on the Clock Lane,
>>
>> TxByteClkHS - exactly 1/8 the High-Speed(HS) bit rate
>>
>> I'd like to emphasize "BIT RATE" above (not clock rate).
>>
>> In such case I would expect that ui2bc would take into account number of
>> lanes:
>>
>> byte clock cycles = div_round_up(ui * dsi->lanes, 8)
>>
>> So in case of one lane we need 8 ticks to get byte, and in case of 4
>> lanes 2 ticks are enough.
>>
>> Am I correct, or I've missed sth?
> Sorry, I think you are wrong.
>
> The spec also mentions that it is recommended that all transmitting
> Data Lane Modules share one TxByteClkHS signal. So, usually,
> TxByteClkHS has nothing to do with data lane number, but only UI - one
> bit period is HS state. I think the NWL DSI follows the recommended
> implementation.
>
> 'Table 20 HS Transmitter AC Specifications' in the spec notes
> that Applicable when supporting maximum HS bit rates ≤ 1 Gbps (UI ≥ 1
> ns). This hints that HS bit rate is 1/UI.
OK, apparently "bit rate" in case of PHYs usually means "bit rate per lane".
Regards
Andrzej