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 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
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 rateI'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