On Tue, 6 Jun 2017 16:30:15 +0300
Tomi Valkeinen <tomi.valkeinen@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
