Re: [PATCH v2 1/2] drm/bridge: Add Cadence DSI driver

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Tue, 6 Jun 2017 16:30:15 +0300
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> On 02/06/17 15:04, Boris Brezillon wrote:
> 
> > +enum cdns_dsi_output_type {
> > +	CDNS_DSI_PANEL = 0,
> > +	CDNS_DSI_BRIDGE = 1,
> > +};  
> 
> Just my opinion, but I think you should only define values for enums
> when those values actually mean something and are needed. In this case,
> it doesn't matter which values those enums have.

Actually, this will go away in the next version (see Archit's comment).

> 
> > +static int cdns_dsi_init_link(struct cdns_dsi *dsi)
> > +{
> > +	u32 val;
> > +	int i;
> > +
> > +	writel(CLK_UNIT_INTERVAL(16), dsi->regs + MCTL_DPHY_STATIC);
> > +	writel(CLK_DIV(0xb) | HSTX_TIMEOUT(0xed8afff),
> > +	       dsi->regs + MCTL_DPHY_TIMEOUT1);
> > +	writel(LPRX_TIMEOUT(0xf30fffff), dsi->regs + MCTL_DPHY_TIMEOUT2);
> > +
> > +	val = WAIT_BURST_TIME(0xf);
> > +	for (i = 1; i < dsi->output->dev->lanes; i++)
> > +		val |= DATA_LANE_EN(i);
> > +
> > +	if (!(dsi->output->dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
> > +		val |= CLK_CONTINUOUS;
> > +
> > +	writel(val, dsi->regs + MCTL_MAIN_PHY_CTL);
> > +
> > +	writel(CLK_LANE_ULPOUT_TIME(0x105) | DATA_LANE_ULPOUT_TIME(0x1d5),
> > +	       dsi->regs + MCTL_ULPOUT_TIME);
> > +
> > +	writel(LINK_EN, dsi->regs + MCTL_MAIN_DATA_CTL);
> > +
> > +	val = CLK_LANE_EN | PLL_START;
> > +	for (i = 0; i < dsi->output->dev->lanes; i++)
> > +		val |= DATA_LANE_START(i);
> > +
> > +	writel(val, dsi->regs + MCTL_MAIN_EN);
> > +
> > +	ndelay(100);
> > +
> > +	return 0;
> > +}  
> 
> There are quite a bit of magic values here (elsewhere too). Looking at
> the names of the macros, many of them probably represent spans of time,
> in clock ticks? Would it make more sense to have the times defined in,
> say, nanoseconds, and a function to convert the ns to clock ticks?
> 
> And a hardcoded CLK_DIV, does that work? Is the clock rate fixed?

Okay, it seems I have all the necessary information to dynamically
calculate ULPOUT_TIME values (these values are based on sysclk and
ULPOUT_TIME should be 1ms).

It's a bit more complicated for MCTL_DPHY_TIMEOUT1 and
MCTL_DPHY_TIMEOUT2, because counters are based on the dsi_tx_byte clock
which is derived from the DPHY PLL, and I don't have the final
spec of the DPHY block yet, which means I can't extract the dsi_tx_byte
clock rate.
A temporary solution would be to hardcore the tx_byte_clk in the driver
and do all the calculation based on this hardcoded value. Or maybe we
should expose the DPHY PLL in the DT.

I also don't know what CLK_UNIT_INTERVAL() is encoding. I'll check with
Cadence engineers.

Thanks,

Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel