Re: [PATCH v2 05/16] drm: rcar-du: lvds: D3/E3 support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Ulrich,

On Monday, 17 September 2018 13:53:58 EEST Ulrich Hecht wrote:
> > On September 14, 2018 at 11:10 AM Laurent Pinchart wrote:
> > 
> > The LVDS encoders in the D3 and E3 SoCs differ significantly from those
> > in the other R-Car Gen3 family members:
> > 
> > - The LVDS PLL architecture is more complex and requires computing PLL
> >   parameters manually.
> > 
> > - The PLL uses external clocks as inputs, which need to be retrieved
> >   from DT.
> > 
> > - In addition to the different PLL setup, the startup sequence has
> >   changed *again* (seems someone had trouble making his/her mind).
> > 
> > Supporting all this requires DT bindings extensions for external clocks,
> > brand new PLL setup code, and a few quirks to handle the differences in
> > the startup sequence.
> > 
> > The implementation doesn't support all hardware features yet, namely
> > 
> > - Using the LV[01] clocks generated by the CPG as PLL input.
> > - Providing the LVDS PLL clock to the DU for use with the RGB output.
> > 
> > Those features can be added later when the need will arise.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > Tested-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > Changes since v1:
> > 
> > - Don't compile-out debug code based on DEBUG and CONFIG_DYNAMIC_DEBUG
> > - Test all three input clocks (DOTCLKIN[01] and EXTAL) and pick the best
> >   one
> > 
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c      | 355 ++++++++++++++++++++++----
> >  drivers/gpu/drm/rcar-du/rcar_lvds_regs.h |  43 +++-
> >  2 files changed, 351 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > b/drivers/gpu/drm/rcar-du/rcar_lvds.c index ce0eb68c3416..23e7743144c8
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c

[snip]

> > +static void rcar_lvds_d3_e3_pll_calc(struct rcar_lvds *lvds, struct clk
> > *clk,
> > +				     unsigned long target, struct pll_info *pll,
> > +				     u32 clksel)
> > +{
> > +	unsigned long output;
> > +	unsigned long fin;
> > +	unsigned int m_min;
> > +	unsigned int m_max;
> > +	unsigned int m;
> > +	int error;
> > +
> > +	if (!clk)
> > +		return;
> > +
> > +	/*
> > +	 * The LVDS PLL is made of a pre-divider and a multiplier (strangerly
> 
> strangely

Will be fixed in v2.

> > +	 * enough called M and N respectively), followed by a post-divider E.
> > +	 *
> > +	 *         ,-----.         ,-----.     ,-----.         ,-----.
> > +	 * Fin --> | 1/M | -Fpdf-> | PFD | --> | VCO | -Fvco-> | 1/E | --> Fout
> > +	 *         `-----'     ,-> |     |     `-----'   |     `-----'
> > +	 *                     |   `-----'               |
> > +	 *                     |         ,-----.         |
> > +	 *                     `-------- | 1/N | <-------'
> > +	 *                               `-----'
> > +	 *
> > +	 * The clock output by the PLL is then further divided by a
> > programmable
> > +	 * divider DIV to achieve the desired target frequency. Finally, an
> > +	 * optional fixed /7 divider is used to convert the bit clock to a
> > pixel
> > +	 * clock (as LVDS transmits 7 bits per lane per clock sample).
> > +	 *
> > +	 *          ,-------.     ,-----.     |\
> > +	 * Fout --> | 1/DIV | --> | 1/7 | --> | |
> > +	 *          `-------'  |  `-----'     | | --> dot clock
> > +	 *                     `------------> | |
> > +	 *                                    |/
> > +	 *
> > +	 * The /7 divider is optional when the LVDS PLL is used to generate a
> > +	 * dot clock for the DU RGB output, without using the LVDS encoder. We
> > +	 * don't support this configuration yet.
> > +	 *
> > +	 * The PLL allowed input frequency range is 12 MHz to 192 MHz.
> > +	 */
> > +
> > +	fin = clk_get_rate(clk);
> > +	if (fin < 12000000 || fin > 192000000)
> > +		return;
> > +
> > +	/*
> > +	 * The comparison frequency range is 12 MHz to 24 MHz, which limits the
> > +	 * allowed values for the pre-divider M (normal range 1-8).
> > +	 *
> > +	 * Fpfd = Fin / M
> > +	 */
> > +	m_min = max_t(unsigned int, 1, DIV_ROUND_UP(fin, 24000000));
> > +	m_max = min_t(unsigned int, 8, fin / 12000000);
> > +
> > +	for (m = m_min; m <= m_max; ++m) {
> > +		unsigned long fpfd;
> > +		unsigned int n_min;
> > +		unsigned int n_max;
> > +		unsigned int n;
> > +
> > +		/*
> > +		 * The VCO operating range is 900 Mhz to 1800 MHz, which limits
> > +		 * the allowed values for the multiplier N (normal range
> > +		 * 60-120).
> > +		 *
> > +		 * Fvco = Fin * N / M
> > +		 */
> > +		fpfd = fin / m;
> > +		n_min = max_t(unsigned int, 60, DIV_ROUND_UP(900000000, fpfd));
> > +		n_max = min_t(unsigned int, 120, 1800000000 / fpfd);
> > +
> > +		for (n = n_min; n < n_max; ++n) {
> > +			unsigned long fvco;
> > +			unsigned int e_min;
> > +			unsigned int e;
> > +
> > +			/*
> > +			 * The output frequency is limited to 1039.5 MHz,
> > +			 * limiting again the allowed values for the
> > +			 * post-divider E (normal value 1, 2 or 4).
> > +			 *
> > +			 * Fout = Fvco / E
> > +			 */
> > +			fvco = fpfd * n;
> > +			e_min = fvco > 1039500000 ? 1 : 0;
> > +
> > +			for (e = e_min; e < 3; ++e) {
> > +				unsigned long fout;
> > +				unsigned long diff;
> > +				unsigned int div;
> > +
> > +				/*
> > +				 * Finally we have a programable divider after
> > +				 * the PLL, followed by a an optional fixed /7
> > +				 * divider.
> > +				 */
> > +				fout = fvco / (1 << e) / 7;
> > +				div = DIV_ROUND_CLOSEST(fout, target);
> > +				diff = abs(fout / div - target);
> > +
> > +				if (diff < pll->diff) {
> > +					pll->diff = diff;
> > +					pll->pll_m = m;
> > +					pll->pll_n = n;
> > +					pll->pll_e = e;
> > +					pll->div = div;
> > +					pll->clksel = clksel;
> > +
> > +					if (diff == 0)
> > +						goto done;
> > +				}
> > +			}
> > +		}
> > +	}
> > +
> > +done:
> > +	output = fin * pll->pll_n / pll->pll_m / (1 << pll->pll_e)
> > +	       / 7 / pll->div;
> > +	error = (long)(output - target) * 10000 / (long)target;
> > +
> > +	dev_dbg(lvds->dev,
> > +		"%pC %lu Hz -> Fout %lu Hz (target %lu Hz, error %d.%02u%%), PLL
> > M/N/E/DIV %u/%u/%u/%u\n", +		clk, fin, output, target, error / 100,
> > +		error < 0 ? -error % 100 : error % 100,
> > +		pll->pll_m, pll->pll_n, pll->pll_e, pll->div);
> > +}
> > +
> > +static void rcar_lvds_pll_setup_d3_e3(struct rcar_lvds *lvds, unsigned
> > int freq)
> > +{
> > +	struct pll_info pll = { .diff = (unsigned long)-1 };
> > +	u32 lvdpllcr;
> > +
> > +	rcar_lvds_d3_e3_pll_calc(lvds, lvds->clocks.dotclkin[0], freq, &pll,
> > +				 LVDPLLCR_CKSEL_DU_DOTCLKIN(0));
> > +	rcar_lvds_d3_e3_pll_calc(lvds, lvds->clocks.dotclkin[1], freq, &pll,
> > +				 LVDPLLCR_CKSEL_DU_DOTCLKIN(1));
> > +	rcar_lvds_d3_e3_pll_calc(lvds, lvds->clocks.extal, freq, &pll,
> > +				 LVDPLLCR_CKSEL_EXTAL);
> > +
> > +	lvdpllcr = LVDPLLCR_PLLON | pll.clksel | LVDPLLCR_CLKOUT
> > +		 | LVDPLLCR_PLLN(pll.pll_n - 1) | LVDPLLCR_PLLM(pll.pll_m - 1);
> > +
> > +	if (pll.pll_e > 0)
> > +		lvdpllcr |= LVDPLLCR_STP_CLKOUTE | LVDPLLCR_OUTCLKSEL
> > +			 |  LVDPLLCR_PLLE(pll.pll_e - 1);
> > +
> > +	rcar_lvds_write(lvds, LVDPLLCR, lvdpllcr);
> > +
> > +	if (pll.div > 1)
> > +		rcar_lvds_write(lvds, LVDDIV, LVDDIV_DIVSEL |
> > +				LVDDIV_DIVRESET | LVDDIV_DIV(pll.div - 1));
> > +	else
> > +		rcar_lvds_write(lvds, LVDDIV, 0);
> 
> The datasheet says DIVRESET == 0 is "reset" and DIVRESET == 1 is "clear". I
> haven't found anything clarifying what that exactly means, but my best
> guess would have been that this should be the other way round: assert
> LVDDIV_DIVRESET ("clear" the divider) if the divider is 1, and deassert it
> ("reset" the divider) otherwise.

My interpretation is opposite, 0 meaning asserting reset and 1 meaning 
clearing reset (and thus deasserting it).

> The BSP driver, however, does it the same way as above, and the PLL setting
> examples (table 37.5) in the datasheet also have DIVRESET asserted when the
> divider is on, so the above code is likely to be correct. Maybe it's the
> LVDDIV register description that is wrong?
> 
> At any rate, I think this is confusing enough to deserve a comment.

I agree, I'll add that.

> > +}

[snip]

-- 
Regards,

Laurent Pinchart



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