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@xxxxxxxxxxxxxxxx>
> > ---
> >
> > 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@xxxxxxxxxxxxxxxxxx>
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
