Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1

[Ladislav Michl <ladis@linux-mips.org>]

Hi Laurent,

I'm happy someone is stepping into this again :-) Just a few comments bellow
(and this thread for more: http://www.spinics.net/lists/linux-omap/msg126591.html)

On Fri, Dec 02, 2016 at 11:14:38PM +0200, Laurent Pinchart wrote:
> From: Richard Watts <rrw@kynesim.co.uk>
> 
> The OMAP36xx DPLL5, driving EHCI USB, can be subject to a long-term
> frequency drift. The frequency drift magnitude depends on the VCO update
> rate, which is inversely proportional to the PLL divider. The kernel
> DPLL configuration code results in a high value for the divider, leading
> to a long term drift high enough to cause USB transmission errors. In
> the worst case the USB PHY's ULPI interface can stop responding,
> breaking USB operation completely. This manifests itself on the
> Beagleboard xM by the LAN9514 reporting 'Cannot enable port 2. Maybe the
> cable is bad?' in the kernel log.
> 
> Errata sprz319 advisory 2.1 documents PLL values that minimize the
> drift. Use them automatically when DPLL5 is used for USB operation,
> which we detect based on the requested clock rate. The clock framework
> will still compute the PLL parameters and resulting rate as usual, but
> the PLL M and N values will then be overridden. This can result in the
> effective clock rate being slightly different than the rate cached by
> the clock framework, but won't cause any adverse effect to USB
> operation.
> 
> Signed-off-by: Richard Watts <rrw@kynesim.co.uk>
> [Upported from v3.2 to v4.9]
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v2:
> 
> - Added spaces around +
> ---
>  drivers/clk/ti/clk-3xxx.c | 20 +++++++-------
>  drivers/clk/ti/clock.h    |  9 +++++++
>  drivers/clk/ti/dpll.c     | 19 +++++++++++++-
>  drivers/clk/ti/dpll3xxx.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 104 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/ti/clk-3xxx.c b/drivers/clk/ti/clk-3xxx.c
> index 8831e1a05367..11d8aa3ec186 100644
> --- a/drivers/clk/ti/clk-3xxx.c
> +++ b/drivers/clk/ti/clk-3xxx.c
> @@ -22,13 +22,6 @@
>  
>  #include "clock.h"
>  
> -/*
> - * DPLL5_FREQ_FOR_USBHOST: USBHOST and USBTLL are the only clocks
> - * that are sourced by DPLL5, and both of these require this clock
> - * to be at 120 MHz for proper operation.
> - */
> -#define DPLL5_FREQ_FOR_USBHOST		120000000
> -
>  #define OMAP3430ES2_ST_DSS_IDLE_SHIFT			1
>  #define OMAP3430ES2_ST_HSOTGUSB_IDLE_SHIFT		5
>  #define OMAP3430ES2_ST_SSI_IDLE_SHIFT			8
> @@ -546,14 +539,21 @@ void __init omap3_clk_lock_dpll5(void)
>  	struct clk *dpll5_clk;
>  	struct clk *dpll5_m2_clk;
>  
> +	/*
> +	 * Errata sprz319f advisory 2.1 documents a USB host clock drift issue
> +	 * that can be worked around using specially crafted dpll5 settings
> +	 * with a dpll5_m2 divider set to 8. Set the dpll5 rate to 8x the USB
> +	 * host clock rate, its .set_rate handler() will detect that frequency
> +	 * and use the errata settings.
> +	 */
>  	dpll5_clk = clk_get(NULL, "dpll5_ck");
> -	clk_set_rate(dpll5_clk, DPLL5_FREQ_FOR_USBHOST);
> +	clk_set_rate(dpll5_clk, OMAP3_DPLL5_FREQ_FOR_USBHOST * 8);
>  	clk_prepare_enable(dpll5_clk);
>  
> -	/* Program dpll5_m2_clk divider for no division */
> +	/* Program dpll5_m2_clk divider */
>  	dpll5_m2_clk = clk_get(NULL, "dpll5_m2_ck");
>  	clk_prepare_enable(dpll5_m2_clk);
> -	clk_set_rate(dpll5_m2_clk, DPLL5_FREQ_FOR_USBHOST);
> +	clk_set_rate(dpll5_m2_clk, OMAP3_DPLL5_FREQ_FOR_USBHOST);
>  
>  	clk_disable_unprepare(dpll5_m2_clk);
>  	clk_disable_unprepare(dpll5_clk);
> diff --git a/drivers/clk/ti/clock.h b/drivers/clk/ti/clock.h
> index 90f3f472ae1c..13c37f48d9d6 100644
> --- a/drivers/clk/ti/clock.h
> +++ b/drivers/clk/ti/clock.h
> @@ -257,11 +257,20 @@ long omap2_dpll_round_rate(struct clk_hw *hw, unsigned long target_rate,
>  unsigned long omap3_clkoutx2_recalc(struct clk_hw *hw,
>  				    unsigned long parent_rate);
>  
> +/*
> + * OMAP3_DPLL5_FREQ_FOR_USBHOST: USBHOST and USBTLL are the only clocks
> + * that are sourced by DPLL5, and both of these require this clock
> + * to be at 120 MHz for proper operation.
> + */
> +#define OMAP3_DPLL5_FREQ_FOR_USBHOST	120000000
> +
>  unsigned long omap3_dpll_recalc(struct clk_hw *hw, unsigned long parent_rate);
>  int omap3_dpll4_set_rate(struct clk_hw *clk, unsigned long rate,
>  			 unsigned long parent_rate);
>  int omap3_dpll4_set_rate_and_parent(struct clk_hw *hw, unsigned long rate,
>  				    unsigned long parent_rate, u8 index);
> +int omap3_dpll5_set_rate(struct clk_hw *hw, unsigned long rate,
> +			 unsigned long parent_rate);
>  void omap3_clk_lock_dpll5(void);
>  
>  unsigned long omap4_dpll_regm4xen_recalc(struct clk_hw *hw,
> diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
> index 9fc8754a6e61..4b9a419d8e14 100644
> --- a/drivers/clk/ti/dpll.c
> +++ b/drivers/clk/ti/dpll.c
> @@ -114,6 +114,18 @@ static const struct clk_ops omap3_dpll_ck_ops = {
>  	.round_rate	= &omap2_dpll_round_rate,
>  };
>  
> +static const struct clk_ops omap3_dpll5_ck_ops = {
> +	.enable		= &omap3_noncore_dpll_enable,
> +	.disable	= &omap3_noncore_dpll_disable,
> +	.get_parent	= &omap2_init_dpll_parent,
> +	.recalc_rate	= &omap3_dpll_recalc,
> +	.set_rate	= &omap3_dpll5_set_rate,
> +	.set_parent	= &omap3_noncore_dpll_set_parent,
> +	.set_rate_and_parent	= &omap3_noncore_dpll_set_rate_and_parent,
> +	.determine_rate	= &omap3_noncore_dpll_determine_rate,
> +	.round_rate	= &omap2_dpll_round_rate,
> +};
> +
>  static const struct clk_ops omap3_dpll_per_ck_ops = {
>  	.enable		= &omap3_noncore_dpll_enable,
>  	.disable	= &omap3_noncore_dpll_disable,
> @@ -474,7 +486,12 @@ static void __init of_ti_omap3_dpll_setup(struct device_node *node)
>  		.modes = (1 << DPLL_LOW_POWER_BYPASS) | (1 << DPLL_LOCKED),
>  	};
>  
> -	of_ti_dpll_setup(node, &omap3_dpll_ck_ops, &dd);
> +	if ((of_machine_is_compatible("ti,omap3630") ||
> +	     of_machine_is_compatible("ti,omap36xx")) &&
> +	    !strcmp(node->name, "dpll5_ck"))
> +		of_ti_dpll_setup(node, &omap3_dpll5_ck_ops, &dd);
> +	else
> +		of_ti_dpll_setup(node, &omap3_dpll_ck_ops, &dd);
>  }
>  CLK_OF_DECLARE(ti_omap3_dpll_clock, "ti,omap3-dpll-clock",
>  	       of_ti_omap3_dpll_setup);
> diff --git a/drivers/clk/ti/dpll3xxx.c b/drivers/clk/ti/dpll3xxx.c
> index 88f2ce81ba55..4cdd28a25584 100644
> --- a/drivers/clk/ti/dpll3xxx.c
> +++ b/drivers/clk/ti/dpll3xxx.c
> @@ -838,3 +838,70 @@ int omap3_dpll4_set_rate_and_parent(struct clk_hw *hw, unsigned long rate,
>  	return omap3_noncore_dpll_set_rate_and_parent(hw, rate, parent_rate,
>  						      index);
>  }
> +
> +/* Apply DM3730 errata sprz319 advisory 2.1. */
> +static bool omap3_dpll5_apply_errata(struct clk_hw *hw,
> +				     unsigned long parent_rate)
> +{
> +	struct omap3_dpll5_settings {
> +		unsigned int rate, m, n;
> +	};
> +
> +	static const struct omap3_dpll5_settings precomputed[] = {
> +		/*
> +		 * From DM3730 errata advisory 2.1, table 35 and 36.
> +		 * The N value is increased by 1 compared to the tables as the
> +		 * errata lists register values while last_rounded_field is the
> +		 * real divider value.
> +		 */
> +		{ 12000000,  80,  0 + 1 },
> +		{ 13000000, 443,  5 + 1 },
> +		{ 19200000,  50,  0 + 1 },
> +		{ 26000000, 443, 11 + 1 },
> +		{ 38400000,  25,  0 + 1 }
> +	};

Table 36 list two options with 26MHz clocks: m=443, n=11 and m=480, n=12
with a statement: "The choice between these two options with a 26 MHz input
should be based on characterization on the end system."

Shall we care about that?

> +	const struct omap3_dpll5_settings *d;
> +	struct clk_hw_omap *clk = to_clk_hw_omap(hw);
> +	struct dpll_data *dd;
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(precomputed); ++i) {
> +		if (parent_rate == precomputed[i].rate)
> +			break;
> +	}
> +
> +	if (i == ARRAY_SIZE(precomputed))
> +		return false;
> +
> +	d = &precomputed[i];
> +
> +	/* Update the M, N and rounded rate values and program the DPLL. */
> +	dd = clk->dpll_data;
> +	dd->last_rounded_m = d->m;
> +	dd->last_rounded_n = d->n;
> +	dd->last_rounded_rate = div_u64((u64)parent_rate * d->m, d->n);
> +	omap3_noncore_dpll_program(clk, 0);
> +
> +	return true;
> +}

What about small optimization? Gives a few tens of bytes smaller code...

diff --git a/drivers/clk/ti/dpll3xxx.c b/drivers/clk/ti/dpll3xxx.c
index 88f2ce8..cd22bcc 100644
--- a/drivers/clk/ti/dpll3xxx.c
+++ b/drivers/clk/ti/dpll3xxx.c
@@ -838,3 +838,67 @@ int omap3_dpll4_set_rate_and_parent(struct clk_hw *hw, unsigned long rate,
 	return omap3_noncore_dpll_set_rate_and_parent(hw, rate, parent_rate,
 						      index);
 }
+
+/* Apply DM3730 errata sprz319 advisory 2.1. */
+static bool omap3_dpll5_apply_errata(struct clk_hw *hw,
+				     unsigned long parent_rate)
+{
+	struct omap3_dpll5_settings {
+		unsigned int rate, m, n;
+	};
+
+	static const struct omap3_dpll5_settings precomputed[] = {
+		/*
+		 * From DM3730 errata advisory 2.1, table 35 and 36.
+		 * The N value is increased by 1 compared to the tables as the
+		 * errata lists register values while last_rounded_field is the
+		 * real divider value.
+		 */
+		{ 12000000,  80,  0 + 1 },
+		{ 13000000, 443,  5 + 1 },
+		{ 19200000,  50,  0 + 1 },
+		{ 26000000, 443, 11 + 1 },
+		{ 38400000,  25,  0 + 1 }
+	};
+
+	struct clk_hw_omap *clk;
+	struct dpll_data *dd;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(precomputed); i++)
+		if (parent_rate == precomputed[i].rate) {
+			clk = to_clk_hw_omap(hw);
+			/* Update the M, N and rounded rate values */
+			dd = clk->dpll_data;
+			dd->last_rounded_m = precomputed[i].m;
+			dd->last_rounded_n = precomputed[i].n;
+			dd->last_rounded_rate =
+				div_u64((u64)parent_rate * dd->last_rounded_m,
+					dd->last_rounded_n);
+			omap3_noncore_dpll_program(clk, 0);
+
+			return true;
+		}
+
+	return false;
+}

> +/**
> + * omap3_dpll5_set_rate - set rate for omap3 dpll5
> + * @hw: clock to change
> + * @rate: target rate for clock
> + * @parent_rate: rate of the parent clock
> + *
> + * Set rate for the DPLL5 clock. Apply the sprz319 advisory 2.1 on OMAP36xx if
> + * the DPLL is used for USB host (detected through the requested rate).
> + */
> +int omap3_dpll5_set_rate(struct clk_hw *hw, unsigned long rate,
> +			 unsigned long parent_rate)
> +{
> +	if (rate == OMAP3_DPLL5_FREQ_FOR_USBHOST * 8) {
> +		if (omap3_dpll5_apply_errata(hw, parent_rate))
> +			return 0;
> +	}
> +
> +	return omap3_noncore_dpll_set_rate(hw, rate, parent_rate);
> +}
> -- 
> Regards,
> 
> Laurent Pinchart

Best regards,
	ladis