Re: [PATCH v3 16/24] drm/rockchip: dw-mipi-dsi: properly configure PHY timing

[John Keeping]

On Mon, 30 Jan 2017 16:57:36 -0500, Sean Paul wrote:

> On Sun, Jan 29, 2017 at 01:24:36PM +0000, John Keeping wrote:
> > These values are specified as constant time periods but the PHY
> > configuration is in terms of the current lane byte clock so using
> > constant values guarantees that the timings will be outside the
> > specification with some display configurations.
> > 
> > Derive the necessary configuration from the byte clock in order to
> > ensure that the PHY configuration is correct.
> > 
> > Signed-off-by: John Keeping <john@xxxxxxxxxxxx>
> > ---
> > v3:
> > - Wrap some long lines
> > Unchanged in v2
> > 
> >  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 39 ++++++++++++++++++++++++++++++----
> >  1 file changed, 35 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > index cfe7e4ba305c..85edf6dd2bac 100644
> > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> > @@ -383,6 +383,26 @@ static void dw_mipi_dsi_phy_write(struct dw_mipi_dsi *dsi, u8 test_code,
> >  	dsi_write(dsi, DSI_PHY_TST_CTRL0, PHY_TESTCLK | PHY_UNTESTCLR);
> >  }
> >  
> > +/**
> > + * ns2bc - Nanoseconds to byte clock cycles
> > + */
> > +static inline unsigned int ns2bc(struct dw_mipi_dsi *dsi, int ns)
> > +{
> > +	unsigned long byte_clk_khz = dsi->lane_mbps * MSEC_PER_SEC / 8;  
> 
> Why multiply by 1000 (MSEC_PER_SEC) only to immediately divide by 1000?
> 
> > +
> > +	return (ns * (byte_clk_khz / 1000) + 999) / 1000;  
> 
> Can you replace this whole function with:
> 
> return DIV_ROUND_UP(ns * dsi->lane_mbps / 8, 1000);

Yes, I'll make this simplification.

> > +}
> > +
> > +/**
> > + * ns2ui - Nanoseconds to UI time periods
> > + */
> > +static inline unsigned int ns2ui(struct dw_mipi_dsi *dsi, int ns)
> > +{
> > +	unsigned long byte_clk_khz = dsi->lane_mbps * MSEC_PER_SEC;
> > +
> > +	return (ns * (byte_clk_khz / 1000) + 999) / 1000;  
> 
> Same remarks here.
> 
> Sean
> 
> 
> > +}
> > +
> >  static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
> >  {
> >  	int ret, testdin, vco, val;
> > @@ -434,10 +454,21 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
> >  					 SETRD_MAX | POWER_MANAGE |
> >  					 TER_RESISTORS_ON);
> >  
> > -
> > -	dw_mipi_dsi_phy_write(dsi, 0x70, TLP_PROGRAM_EN | 0xf);
> > -	dw_mipi_dsi_phy_write(dsi, 0x71, THS_PRE_PROGRAM_EN | 0x55);
> > -	dw_mipi_dsi_phy_write(dsi, 0x72, THS_ZERO_PROGRAM_EN | 0xa);
> > +	dw_mipi_dsi_phy_write(dsi, 0x60, TLP_PROGRAM_EN | ns2bc(dsi, 500));
> > +	dw_mipi_dsi_phy_write(dsi, 0x61, THS_PRE_PROGRAM_EN | ns2ui(dsi, 40));
> > +	dw_mipi_dsi_phy_write(dsi, 0x62, THS_ZERO_PROGRAM_EN | ns2bc(dsi, 300));
> > +	dw_mipi_dsi_phy_write(dsi, 0x63, THS_PRE_PROGRAM_EN | ns2ui(dsi, 100));
> > +	dw_mipi_dsi_phy_write(dsi, 0x64, BIT(5) | ns2bc(dsi, 100));
> > +	dw_mipi_dsi_phy_write(dsi, 0x65, BIT(5) | (ns2bc(dsi, 60) + 7));
> > +
> > +	dw_mipi_dsi_phy_write(dsi, 0x70, TLP_PROGRAM_EN | ns2bc(dsi, 500));
> > +	dw_mipi_dsi_phy_write(dsi, 0x71,
> > +			      THS_PRE_PROGRAM_EN | (ns2ui(dsi, 50) + 5));
> > +	dw_mipi_dsi_phy_write(dsi, 0x72,
> > +			      THS_ZERO_PROGRAM_EN | (ns2bc(dsi, 140) + 2));
> > +	dw_mipi_dsi_phy_write(dsi, 0x73,
> > +			      THS_PRE_PROGRAM_EN | (ns2ui(dsi, 60) + 8));
> > +	dw_mipi_dsi_phy_write(dsi, 0x74, BIT(5) | ns2bc(dsi, 100));
> >  
> >  	dsi_write(dsi, DSI_PHY_RSTZ, PHY_ENFORCEPLL | PHY_ENABLECLK |
> >  				     PHY_UNRSTZ | PHY_UNSHUTDOWNZ);
> > -- 
> > 2.11.0.197.gb556de5.dirty
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel  
>