Date: Tue, 19 Sep 2017 11:00:25 -0700
From: Sean Paul <seanpaul@chromium.org>
To: Nickey Yang <nickey.yang@rock-chips.com>
Cc: mark.yao@rock-chips.com, robh+dt@kernel.org, heiko@sntech.de,
        mark.rutland@arm.com, airlied@linux.ie, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org,
        seanpaul@chromium.org, briannorris@chromium.org, hl@rock-chips.com,
        zyw@rock-chips.com, bivvy.bi@rock-chips.com, xbl@rock-chips.com
Subject: Re: [PATCH 1/7] drm/rockchip/dsi: correct Feedback divider setting
Message-ID: <20170919180025.apb4aq7ca3filh6c@art_vandelay>
References: <1505725539-6309-1-git-send-email-nickey.yang@rock-chips.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1505725539-6309-1-git-send-email-nickey.yang@rock-chips.com>
User-Agent: NeoMutt/20170306-97-7656f1-dirty (1.8.0)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5394
Lines: 164

On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
> This patch correct Feedback divider setting:
> 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN
> 2、Due to the use of a "by 2 pre-scaler," the range of the
> feedback multiplication Feedback divider is limited to even
> division numbers, and Feedback divider must be greater than
> 12, less than 1000.
> 3、Make the previously configured Feedback divider(LSB)
> factors effective
> 
> Signed-off-by: Nickey Yang <nickey.yang@rock-chips.com>
> ---
>  drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------
>  1 file changed, 54 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> index 9a20b9d..52698b7 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c
> @@ -228,7 +228,7 @@
>  #define LOW_PROGRAM_EN		0
>  #define HIGH_PROGRAM_EN		BIT(7)
>  #define LOOP_DIV_LOW_SEL(val)	(((val) - 1) & 0x1f)
> -#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0x1f)
> +#define LOOP_DIV_HIGH_SEL(val)	((((val) - 1) >> 5) & 0xf)
>  #define PLL_LOOP_DIV_EN		BIT(5)
>  #define PLL_INPUT_DIV_EN	BIT(4)
>  
> @@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>  	dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div));
>  	dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) |
>  					 LOW_PROGRAM_EN);
> +	dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);

You do the same write 2 lines down. Are both needed? It would be nice if the
register names were also defined, so this is easier to read.

>  	dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) |
>  					 HIGH_PROGRAM_EN);
>  	dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
> @@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi)
>  static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>  				    struct drm_display_mode *mode)
>  {
> -	unsigned int i, pre;
> -	unsigned long mpclk, pllref, tmp;
> -	unsigned int m = 1, n = 1, target_mbps = 1000;
> +	unsigned long mpclk, tmp;
> +	unsigned int target_mbps = 1000;
>  	unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps;
>  	int bpp;
> +	unsigned long best_freq = 0;
> +	unsigned long fvco_min, fvco_max, fin ,fout;
> +	unsigned int min_prediv, max_prediv;
> +	unsigned int _prediv, uninitialized_var(best_prediv);
> +	unsigned long _fbdiv, uninitialized_var(best_fbdiv);
> +	unsigned long min_delta = UINT_MAX;

Once you explicitly type these, make sure you use the appropriate _MAX value
(ie: U64_MAX)

>  
>  	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>  	if (bpp < 0) {
> @@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi,
>  			dev_err(dsi->dev, "DPHY clock frequency is out of range\n");
>  	}
>  
> -	pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
> -	tmp = pllref;
> -
> -	/*
> -	 * The limits on the PLL divisor are:
> -	 *
> -	 *	5MHz <= (pllref / n) <= 40MHz
> -	 *
> -	 * we walk over these values in descreasing order so that if we hit
> -	 * an exact match for target_mbps it is more likely that "m" will be
> -	 * even.
> -	 *
> -	 * TODO: ensure that "m" is even after this loop.
> -	 */
> -	for (i = pllref / 5; i > (pllref / 40); i--) {
> -		pre = pllref / i;
> -		if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {
> -			tmp = target_mbps % pre;
> -			n = i;
> -			m = target_mbps / pre;
> +	fin = clk_get_rate(dsi->pllref_clk);
> +	fout = target_mbps * USEC_PER_SEC;
> +
> +	/* constraint: 5Mhz < Fref / N < 40MHz */
> +	min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC);
> +	max_prediv = fin / (5 * USEC_PER_SEC);
> +
> +	/* constraint: 80MHz < Fvco < 1500Mhz */
> +	fvco_min = 80 * USEC_PER_SEC;
> +	fvco_max = 1500 * USEC_PER_SEC;
> +
> +	for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) {
> +		u32 delta;
> +		/* Fvco = Fref * M / N */
> +		tmp = fout * _prediv;
> +		do_div(tmp, fin);
> +		_fbdiv = tmp;

Why use tmp at all? Can't you just use _fbdiv directly in do_div?

> +		/*
> +		 * Due to the use of a "by 2 pre-scaler," the range of the
> +		 * feedback multiplication value M is limited to even division
> +		 * numbers, and m must be greater than 12, less than 1000.
> +		 */
> +		if (_fbdiv < 12 || _fbdiv > 1000)
> +			continue;
> +
> +		if (_fbdiv % 2)
> +			++_fbdiv;

You can reduce this down to:
_fbdiv += _fbdiv % 2;

> +
> +		tmp = (u64)_fbdiv * fin;

I'll echo Brian's concerns on type mixing here. Please be explicit with size
when you declare your variables.

> +		do_div(tmp, _prediv);
> +		if (tmp < fvco_min || tmp > fvco_max)
> +			continue;
> +
> +		delta = abs(fout - tmp);
> +		if (delta < min_delta) {
> +			best_prediv = _prediv;
> +			best_fbdiv = _fbdiv;
> +			min_delta = delta;
> +			best_freq = tmp;
>  		}
> -		if (tmp == 0)
> -			break;
>  	}
>  
> -	dsi->lane_mbps = pllref / n * m;
> -	dsi->input_div = n;
> -	dsi->feedback_div = m;
> +	if (best_freq) {

Should you return an error or log something if best_freq is not found?

> +		dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC);
> +		dsi->input_div = best_prediv;
> +		dsi->feedback_div = best_fbdiv;
> +	}
>  
>  	return 0;
>  }
> -- 
> 1.9.1
> 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS