Re: [PATCH] phy: dphy: Correct clk_pre parameter

[Andrzej Hajda]

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@xxxxxxxxx>
> > > Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> > > Cc: Robert Foss <robert.foss@xxxxxxxxxx>
> > > Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > Cc: Jonas Karlman <jonas@xxxxxxxxx>
> > > Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
> > > Cc: David Airlie <airlied@xxxxxxxx>
> > > Cc: Daniel Vetter <daniel@xxxxxxxx>
> > > Cc: Kishon Vijay Abraham I <kishon@xxxxxx>
> > > Cc: Vinod Koul <vkoul@xxxxxxxxxx>
> > > Cc: Kevin Hilman <khilman@xxxxxxxxxxxx>
> > > Cc: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> > > Cc: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> > > Cc: Heiko Stuebner <heiko@xxxxxxxxx>
> > > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > > Cc: Guido Günther <agx@xxxxxxxxxxx>
> > > Tested-by: Liu Ying <victor.liu@xxxxxxx> # RM67191 DSI panel on i.MX8mq EVK
> > > Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> > > ---
> > >    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