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

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

Hi Jacopo,

On Tuesday, 11 September 2018 16:23:23 EEST jacopo mondi wrote:
> On Tue, Sep 04, 2018 at 03:10:16PM +0300, 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>
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c      | 365 ++++++++++++++++++++++----
> >  drivers/gpu/drm/rcar-du/rcar_lvds_regs.h |  43 +++-
> >  2 files changed, 361 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..aac4acbcfbfc
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c

[snip]

> > +struct pll_info {
> > +	struct clk *clk;
> > +	unsigned long diff;
> > +	unsigned int pll_m;
> > +	unsigned int pll_n;
> > +	unsigned int pll_e;
> > +	unsigned int div;
> > +};
> > +
> > +static void rcar_lvds_d3_e3_pll_calc(struct rcar_lvds *lvds, struct clk
> > *clk,
> > +				     unsigned long target, struct pll_info *pll)
> 
> Do you think it is worth mentioning d3_e3 in the function name? I know
> it's not a big deal, but in future generation this PLL circuit may be
> re-used.

How would you name it ? Other LVDS encoder instances have a different PLL, and 
they are not named in the datasheet. I propose renaming it later if needed.

> > +{
> > +	unsigned long fin;
> > +	unsigned int m_min;
> > +	unsigned int m_max;
> > +	unsigned int m;
> > +
> > +	if (!clk)
> > +		return;
> > +
> > +	/*
> > +	 * The LVDS PLL is made of a pre-divider and a multiplier (strangerly
> > +	 * 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->clk = clk;
> > +					pll->diff = diff;
> > +					pll->pll_m = m;
> > +					pll->pll_n = n;
> > +					pll->pll_e = e;
> > +					pll->div = div;
> > +
> > +					if (diff == 0)
> > +						goto done;
> > +				}
> > +			}
> > +		}
> > +	}
> 
> Very nice :)

Thanks :-)

> > +
> > +done:
> > +#if defined(CONFIG_DEBUG) || defined(CONFIG_DYNAMIC_DEBUG)
> > +	{
> > +		unsigned long output = fin * pll->pll_n / pll->pll_m
> > +				     / (1 << pll->pll_e) / 7 / pll->div;
> > +		int 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);
> > +	}
> > +#endif
> 
> I know you know about this already, but
> 
> ../drivers/gpu/drm/rcar-du/rcar_lvds.c:298:1: error: label at end of
> compound statement
> 
> I'm still not sure what actually disturbs gcc here

Neither do I, but I've fixed it anyway.

> >  }
> > 
> > -static u32 rcar_lvds_lvdpllcr_gen3(unsigned int freq)
> > +static void rcar_lvds_pll_setup_d3_e3(struct rcar_lvds *lvds, unsigned
> > int freq)> 
> >  {
> > 
> > -	if (freq < 42000)
> > -		return LVDPLLCR_PLLDIVCNT_42M;
> > -	else if (freq < 85000)
> > -		return LVDPLLCR_PLLDIVCNT_85M;
> > -	else if (freq < 128000)
> > -		return LVDPLLCR_PLLDIVCNT_128M;
> > +	struct drm_crtc *crtc = lvds->bridge.encoder->crtc;
> > +	struct pll_info pll = { .diff = (unsigned long)-1 };
> > +	u32 lvdpllcr;
> > +
> > +	if (lvds->clocks.dotclkin[0] || lvds->clocks.dotclkin[1]) {
> > +		rcar_lvds_d3_e3_pll_calc(lvds, lvds->clocks.dotclkin[0],
> > +					 freq, &pll);
> > +		rcar_lvds_d3_e3_pll_calc(lvds, lvds->clocks.dotclkin[1],
> > +					 freq, &pll);
> > +	} else if (lvds->clocks.extal) {
> > +		rcar_lvds_d3_e3_pll_calc(lvds, lvds->clocks.extal,
> > +					 freq, &pll);
> > +	}
> 
> here it's either ((dotclkin[0] or dotclock[1]) or extal). Are they
> mutually exclusive? Can't we try all of them? The probe routine
> guarantees we have at least of of them...

I think you're right, I can't remember why I did it this way. I'll update the 
code to try the three clocks.

> > +
> > +	lvdpllcr = LVDPLLCR_PLLON | LVDPLLCR_CLKOUT
> > +		 | LVDPLLCR_PLLN(pll.pll_n - 1) | LVDPLLCR_PLLM(pll.pll_m - 1);
> > +
> > +	if (pll.clk == lvds->clocks.extal)
> > +		lvdpllcr |= LVDPLLCR_CKSEL_EXTAL;
> > +	else
> > +		lvdpllcr |= LVDPLLCR_CKSEL_DU_DOTCLKIN(drm_crtc_index(crtc));
> 
> Here you select du_clkin[0] or du_clkin[1] based on the DU index (btw,
> the drm_crtc_index() function is funny, it simply "crtc->index" no
> checks, no validation, what's the benefit of using it?).

See

commit 490d3d1b91201fd3d3d01d64e11df4eac1d92bd4
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri May 27 20:05:00 2016 +0100

    drm: Store the plane's index

> Looking at the LVDS PLL block diagram for D3/E3 (Figure 37.3) I see
> that both clkin[0] and clkin[1] could be used independently from the du
> index. Shouldn't we use the one performing better? (now how to make
> sure it gets not selected twice in case of both DU0 and DU1 using the
> LVDS PLL it's another problem)

You're right again, I'll fix that.

> > +
> > +	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
> > -		return LVDPLLCR_PLLDIVCNT_148M;
> > +		rcar_lvds_write(lvds, LVDDIV, 0);
> >  }

[snip]

> > +static struct clk *rcar_lvds_get_clock(struct rcar_lvds *lvds, const char
> > *name,
> > +				       bool optional)
> > +{
> > +	struct clk *clk;
> > +
> > +	clk = devm_clk_get(lvds->dev, name);
> 
> I wish we had clk_get_optional() as we have gpiod_get_optional().
> There are probably good reasons if it's not there though...

I don't know, given that this function is pretty much a clk_get_optional(), it 
would seem useful to me. Feel free to propose it :-)

> > +	if (!IS_ERR(clk))
> > +		return clk;
> > +
> > +	if (PTR_ERR(clk) == -ENOENT && optional)
> > +		return NULL;
> > +
> > +	if (PTR_ERR(clk) != -EPROBE_DEFER)
> > +		dev_err(lvds->dev, "failed to get %s clock\n",
> > +			name ? name : "module");
> > +
> > +	return clk;
> > +}

[snip]

-- 
Regards,

Laurent Pinchart