Re: [PATCH v2 2/2] drm: rcar-du: Improve non-DPLL clock selection

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

Hi Jacopo,

Thank you for the patch.

On Monday, 20 August 2018 18:26:17 EEST Jacopo Mondi wrote:
> From: Jacopo Mondi <jacopo@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@60Hz).

Maybe "(such as 1920x1080@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@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