Hi Jacopo,
Thank you for the patch.
On Monday, 20 August 2018 18:26:17 EEST Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo at jmondi.org>
>
> DU channels not equipped with a DPLL use an internal (aka SoC provided) or
I'd say "SoC internal (provided by the CPG)"
> external clock source combined with an internal divider to generate the
and here "a DU internal divider" to avoid confusion with an SoC internal
divider outside of the DU.
> desired output dot clock frequency.
>
> The current clock selection procedure does not fully exploit the ability
> of external clock sources to generate the exact dot clock frequency by
> themselves, but relies instead on tuning the internal DU clock divider
> only, resulting in a less precise clock generation process.
>
> When possible, and desirable, ask the external clock source for the exact
> output dot clock frequency, and test the returned frequency against the
> internally generated one to better approximate the desired output dot clock.
To make this a big more generic, I' say "and select the clock source that
produces the frequency closest to the desired output dot clock".
> This patch specifically targets platforms (like Salvator-X[S] and ULCBs)
> where the DU's input dotclock.in is generated by the versaclock VC5 clock
> source, which is capable of generating the exact rate the DU needs as pixel
> clock output.
>
> This patch fixes higher resolution modes wich requires an high pixel clock
s/wich/which/
> output currently not working on non-HDMI DU channel (as VGA 1920x1080 at 60Hz).
Maybe "(such as 1920x1080 at 60Hz on the VGA output)" ?
>
> Fixes: 1b30dbde8 "drm: rcar-du: Add support for external pixel clock"
Please use the output of git show --pretty=fixes to generate the fixes line.
Your SHA1 needs more digits, and the subject should be enclosed with
parentheses.
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 53 +++++++++++++++++++------------
> 1 file changed, 32 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 077e681..98ae697 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -258,42 +258,53 @@ static void rcar_du_crtc_set_display_timing(struct
> rcar_du_crtc *rcrtc)
>
> escr = ESCR_DCLKSEL_DCLKIN | div;
> } else {
> - unsigned long clk;
> + unsigned long dotclkin_rate;
> + struct clk *dotclkin_clk;
> + unsigned long cpg_dist;
> u32 div;
>
> /*
> * Compute the clock divisor and select the internal or external
> * dot clock based on the requested frequency.
> */
> - clk = clk_get_rate(rcrtc->clock);
> - div = DIV_ROUND_CLOSEST(clk, mode_clock);
> - div = clamp(div, 1U, 64U) - 1;
> -
> + dotclkin_clk = rcrtc->clock;
> + dotclkin_rate = clk_get_rate(rcrtc->clock);
> + div = clamp(DIV_ROUND_CLOSEST(dotclkin_rate, mode_clock),
> + 1UL, 64UL) - 1;
> + cpg_dist = abs(dotclkin_rate / (div + 1) - mode_clock);
> escr = ESCR_DCLKSEL_CLKS | div;
>
> if (rcrtc->extclock) {
> - unsigned long extclk;
> - unsigned long extrate;
> - unsigned long rate;
> - u32 extdiv;
> -
> - extclk = clk_get_rate(rcrtc->extclock);
> - extdiv = DIV_ROUND_CLOSEST(extclk, mode_clock);
> - extdiv = clamp(extdiv, 1U, 64U) - 1;
> + unsigned long ext_rate;
> + unsigned long ext_dist;
>
> - extrate = extclk / (extdiv + 1);
> - rate = clk / (div + 1);
> + /*
> + * If an external clock source is present ask it
> + * for the exact dot clock rate, and test the
> + * returned value against the cpg provided one.
> + */
> + ext_rate = clk_round_rate(rcrtc->extclock,
> + mode_clock);
>
> - if (abs((long)extrate - (long)mode_clock) <
> - abs((long)rate - (long)mode_clock))
> - escr = ESCR_DCLKSEL_DCLKIN | extdiv;
> + div = clamp(DIV_ROUND_CLOSEST(ext_rate, mode_clock),
> + 1UL, 64UL) - 1;
> + ext_dist = abs(ext_rate / (div + 1) - mode_clock);
>
> - dev_dbg(rcrtc->group->dev->dev,
> - "mode clock %lu extrate %lu rate %lu ESCR 0x%08x\n",
> - mode_clock, extrate, rate, escr);
> + if (ext_dist < cpg_dist) {
> + dotclkin_clk = rcrtc->extclock;
> + dotclkin_rate = ext_rate;
> + escr = ESCR_DCLKSEL_DCLKIN | div;
> + }
> }
I think we can do something much simpler here by factoring some code out to a
function. I'll send a proposal in reply to this e-mail.
> + dev_dbg(rcrtc->group->dev->dev, "mode clock %lu %s rate %lu\n",
> + mode_clock, dotclkin_clk == rcrtc->clock ? "cpg" : "ext",
> + dotclkin_rate);
> + clk_set_rate(dotclkin_clk, dotclkin_rate);
> }
>
> + dev_dbg(rcrtc->group->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr);
> +
> rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? ESCR2 : ESCR,
> escr);
> rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? OTAR2 : OTAR, 0);
--
Regards,
Laurent Pinchart