All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
@ 2016-12-02 21:14 Laurent Pinchart
  2016-12-02 22:41   ` Michael Turquette
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Laurent Pinchart @ 2016-12-02 21:14 UTC (permalink / raw)
  To: linux-omap
  Cc: linux-clk, Paul Walmsley, Tero Kristo, Richard Watts,
	Tony Lindgren, Alexander Kinzer

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 }
+	};
+
+	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;
+}
+
+/**
+ * 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-02 21:14 [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1 Laurent Pinchart
@ 2016-12-02 22:41   ` Michael Turquette
  2016-12-05  8:22 ` Ladislav Michl
  2016-12-08 21:16 ` Stephen Boyd
  2 siblings, 0 replies; 21+ messages in thread
From: Michael Turquette @ 2016-12-02 22:41 UTC (permalink / raw)
  To: Laurent Pinchart, linux-omap
  Cc: linux-clk, Paul Walmsley, Tero Kristo, Richard Watts,
	Tony Lindgren, Alexander Kinzer

Quoting Laurent Pinchart (2016-12-02 13:14:38)
> 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 settin=
gs
> +        * 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 freq=
uency
> +        * and use the errata settings.
> +        */
>         dpll5_clk =3D 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 =3D 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, unsig=
ned 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 rat=
e,
>                                     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 =3D {
>         .round_rate     =3D &omap2_dpll_round_rate,
>  };
>  =

> +static const struct clk_ops omap3_dpll5_ck_ops =3D {
> +       .enable         =3D &omap3_noncore_dpll_enable,
> +       .disable        =3D &omap3_noncore_dpll_disable,
> +       .get_parent     =3D &omap2_init_dpll_parent,
> +       .recalc_rate    =3D &omap3_dpll_recalc,
> +       .set_rate       =3D &omap3_dpll5_set_rate,
> +       .set_parent     =3D &omap3_noncore_dpll_set_parent,
> +       .set_rate_and_parent    =3D &omap3_noncore_dpll_set_rate_and_pare=
nt,
> +       .determine_rate =3D &omap3_noncore_dpll_determine_rate,
> +       .round_rate     =3D &omap2_dpll_round_rate,
> +};
> +
>  static const struct clk_ops omap3_dpll_per_ck_ops =3D {
>         .enable         =3D &omap3_noncore_dpll_enable,
>         .disable        =3D &omap3_noncore_dpll_disable,
> @@ -474,7 +486,12 @@ static void __init of_ti_omap3_dpll_setup(struct dev=
ice_node *node)
>                 .modes =3D (1 << DPLL_LOW_POWER_BYPASS) | (1 << DPLL_LOCK=
ED),
>         };
>  =

> -       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 *h=
w, unsigned long rate,
>         return omap3_noncore_dpll_set_rate_and_parent(hw, rate, parent_ra=
te,
>                                                       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[] =3D {
> +               /*
> +                * From DM3730 errata advisory 2.1, table 35 and 36.
> +                * The N value is increased by 1 compared to the tables a=
s 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 }
> +       };
> +
> +       const struct omap3_dpll5_settings *d;
> +       struct clk_hw_omap *clk =3D to_clk_hw_omap(hw);
> +       struct dpll_data *dd;
> +       unsigned int i;
> +
> +       for (i =3D 0; i < ARRAY_SIZE(precomputed); ++i) {
> +               if (parent_rate =3D=3D precomputed[i].rate)
> +                       break;
> +       }
> +
> +       if (i =3D=3D ARRAY_SIZE(precomputed))
> +               return false;
> +
> +       d =3D &precomputed[i];
> +
> +       /* Update the M, N and rounded rate values and program the DPLL. =
*/
> +       dd =3D clk->dpll_data;
> +       dd->last_rounded_m =3D d->m;
> +       dd->last_rounded_n =3D d->n;
> +       dd->last_rounded_rate =3D div_u64((u64)parent_rate * d->m, d->n);
> +       omap3_noncore_dpll_program(clk, 0);
> +
> +       return true;
> +}
> +
> +/**
> + * 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 OMAP3=
6xx 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 =3D=3D 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);

Looks OK to me. This part is a bit sketchy, relying on the requested
rate to determine whether or not to deal with the drift, but errata are
often a sketchy affair.

Regards,
Mike

> +}
> -- =

> Regards,
> =

> Laurent Pinchart
> =

> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
@ 2016-12-02 22:41   ` Michael Turquette
  0 siblings, 0 replies; 21+ messages in thread
From: Michael Turquette @ 2016-12-02 22:41 UTC (permalink / raw)
  To: Laurent Pinchart, linux-omap
  Cc: linux-clk, Paul Walmsley, Tero Kristo, Richard Watts,
	Tony Lindgren, Alexander Kinzer

Quoting Laurent Pinchart (2016-12-02 13:14:38)
> 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 }
> +       };
> +
> +       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;
> +}
> +
> +/**
> + * 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);

Looks OK to me. This part is a bit sketchy, relying on the requested
rate to determine whether or not to deal with the drift, but errata are
often a sketchy affair.

Regards,
Mike

> +}
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-02 21:14 [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1 Laurent Pinchart
  2016-12-02 22:41   ` Michael Turquette
@ 2016-12-05  8:22 ` Ladislav Michl
  2016-12-05  8:46   ` Laurent Pinchart
  2016-12-08 21:16 ` Stephen Boyd
  2 siblings, 1 reply; 21+ messages in thread
From: Ladislav Michl @ 2016-12-05  8:22 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-omap, linux-clk, Paul Walmsley, Tero Kristo, Richard Watts,
	Tony Lindgren, Alexander Kinzer

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

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-05  8:22 ` Ladislav Michl
@ 2016-12-05  8:46   ` Laurent Pinchart
  2016-12-05  9:36     ` Ladislav Michl
  0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2016-12-05  8:46 UTC (permalink / raw)
  To: Ladislav Michl
  Cc: linux-omap, linux-clk, Paul Walmsley, Tero Kristo, Richard Watts,
	Tony Lindgren, Alexander Kinzer

Hi Ladislav,

On Monday 05 Dec 2016 09:22:10 Ladislav Michl wrote:
> 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(-)

[snip]

> > 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?

I'd like to, but at the moment I don't see how. Proposals are welcome :-) I 
don't think addressing that issue should be a blocker to get this patch merged 
though.

> > +	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;
> +}

I had tried that, but I find the code less readable :-S

(I wish C had a for (...) { ... } else { ... } construct like Python does.)

> > +/**
> > + * 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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-05  8:46   ` Laurent Pinchart
@ 2016-12-05  9:36     ` Ladislav Michl
  2016-12-05 11:08       ` Laurent Pinchart
  2016-12-05 23:59       ` Laurent Pinchart
  0 siblings, 2 replies; 21+ messages in thread
From: Ladislav Michl @ 2016-12-05  9:36 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-omap, linux-clk, Paul Walmsley, Tero Kristo, Richard Watts,
	Tony Lindgren, Alexander Kinzer

Hi Laurent,

On Mon, Dec 05, 2016 at 10:46:43AM +0200, Laurent Pinchart wrote:
> Hi Ladislav,
> 
> On Monday 05 Dec 2016 09:22:10 Ladislav Michl wrote:
[snip]
> > 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?
> 
> I'd like to, but at the moment I don't see how. Proposals are welcome :-) I 

One of proposals raised earlier was DT property, but that idea was scratched
later.

> don't think addressing that issue should be a blocker to get this patch merged 
> though.

Of course not. I'd like to even see it in stable ;-)

[snip]
> I had tried that, but I find the code less readable :-S

Oh... Please reconsider (I really do not like that extra test and extra
assignment to local variables (also I had 'precomputed' as mixed definition,
but Tero did not quite like that)) :-) Also, checked if the same values
are written to clk as with my patch, so here's my:
Tested-by: Ladislav Michl <ladis@linux-mips.org>

Best regards,
	ladis

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-05  9:36     ` Ladislav Michl
@ 2016-12-05 11:08       ` Laurent Pinchart
  2016-12-05 12:24           ` Tero Kristo
  2016-12-05 23:59       ` Laurent Pinchart
  1 sibling, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2016-12-05 11:08 UTC (permalink / raw)
  To: Ladislav Michl
  Cc: linux-omap, linux-clk, Paul Walmsley, Tero Kristo, Richard Watts,
	Tony Lindgren, Alexander Kinzer

Hi Ladislav,

On Monday 05 Dec 2016 10:36:49 Ladislav Michl wrote:
> On Mon, Dec 05, 2016 at 10:46:43AM +0200, Laurent Pinchart wrote:
> > On Monday 05 Dec 2016 09:22:10 Ladislav Michl wrote:
>
> [snip]
> 
> >> 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?
> > 
> > I'd like to, but at the moment I don't see how. Proposals are welcome :-)
> > I
> 
> One of proposals raised earlier was DT property, but that idea was scratched
> later.

It might not be such a bad idea, given that the decision should be made based 
on the characterization of the whole system. One could argue that such 
platform information could have its place in DT.

> > don't think addressing that issue should be a blocker to get this patch
> > merged though.
> 
> Of course not. I'd like to even see it in stable ;-)
> 
> [snip]
> 
> > I had tried that, but I find the code less readable :-S
> 
> Oh... Please reconsider (I really do not like that extra test and extra
> assignment to local variables (also I had 'precomputed' as mixed definition,
> but Tero did not quite like that)) :-)

I still like to favour code readability when possible (especially when the 
compiler should optimize both versions the same way). I'm not the maintainer 
of this driver though, so I'll let Tero decides what he prefers.

> Also, checked if the same values are written to clk as with my patch, so
> here's my:
> Tested-by: Ladislav Michl <ladis@linux-mips.org>

Thank you.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-05 11:08       ` Laurent Pinchart
@ 2016-12-05 12:24           ` Tero Kristo
  0 siblings, 0 replies; 21+ messages in thread
From: Tero Kristo @ 2016-12-05 12:24 UTC (permalink / raw)
  To: Laurent Pinchart, Ladislav Michl
  Cc: linux-omap, linux-clk, Paul Walmsley, Richard Watts,
	Tony Lindgren, Alexander Kinzer, Stephen Boyd, Michael Turquette

On 05/12/16 13:08, Laurent Pinchart wrote:
> Hi Ladislav,
>
> On Monday 05 Dec 2016 10:36:49 Ladislav Michl wrote:
>> On Mon, Dec 05, 2016 at 10:46:43AM +0200, Laurent Pinchart wrote:
>>> On Monday 05 Dec 2016 09:22:10 Ladislav Michl wrote:
>>
>> [snip]
>>
>>>> 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?
>>>
>>> I'd like to, but at the moment I don't see how. Proposals are welcome :-)
>>> I
>>
>> One of proposals raised earlier was DT property, but that idea was scratched
>> later.
>
> It might not be such a bad idea, given that the decision should be made based
> on the characterization of the whole system. One could argue that such
> platform information could have its place in DT.
>
>>> don't think addressing that issue should be a blocker to get this patch
>>> merged though.
>>
>> Of course not. I'd like to even see it in stable ;-)
>>
>> [snip]
>>
>>> I had tried that, but I find the code less readable :-S
>>
>> Oh... Please reconsider (I really do not like that extra test and extra
>> assignment to local variables (also I had 'precomputed' as mixed definition,
>> but Tero did not quite like that)) :-)
>
> I still like to favour code readability when possible (especially when the
> compiler should optimize both versions the same way). I'm not the maintainer
> of this driver though, so I'll let Tero decides what he prefers.

The compiler should ideally generate same size code for these both. 
Personally, I don't mind which version goes in; I'd say both are as 
readable.

Stephen, Mike, is one of you going to pick this up? I don't think I have 
anything else to pull due to the ongoing discussion with the other 
pending stuff.

-Tero

>
>> Also, checked if the same values are written to clk as with my patch, so
>> here's my:
>> Tested-by: Ladislav Michl <ladis@linux-mips.org>
>
> Thank you.
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
@ 2016-12-05 12:24           ` Tero Kristo
  0 siblings, 0 replies; 21+ messages in thread
From: Tero Kristo @ 2016-12-05 12:24 UTC (permalink / raw)
  To: Laurent Pinchart, Ladislav Michl
  Cc: linux-omap, linux-clk, Paul Walmsley, Richard Watts,
	Tony Lindgren, Alexander Kinzer, Stephen Boyd, Michael Turquette

On 05/12/16 13:08, Laurent Pinchart wrote:
> Hi Ladislav,
>
> On Monday 05 Dec 2016 10:36:49 Ladislav Michl wrote:
>> On Mon, Dec 05, 2016 at 10:46:43AM +0200, Laurent Pinchart wrote:
>>> On Monday 05 Dec 2016 09:22:10 Ladislav Michl wrote:
>>
>> [snip]
>>
>>>> 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?
>>>
>>> I'd like to, but at the moment I don't see how. Proposals are welcome :-)
>>> I
>>
>> One of proposals raised earlier was DT property, but that idea was scratched
>> later.
>
> It might not be such a bad idea, given that the decision should be made based
> on the characterization of the whole system. One could argue that such
> platform information could have its place in DT.
>
>>> don't think addressing that issue should be a blocker to get this patch
>>> merged though.
>>
>> Of course not. I'd like to even see it in stable ;-)
>>
>> [snip]
>>
>>> I had tried that, but I find the code less readable :-S
>>
>> Oh... Please reconsider (I really do not like that extra test and extra
>> assignment to local variables (also I had 'precomputed' as mixed definition,
>> but Tero did not quite like that)) :-)
>
> I still like to favour code readability when possible (especially when the
> compiler should optimize both versions the same way). I'm not the maintainer
> of this driver though, so I'll let Tero decides what he prefers.

The compiler should ideally generate same size code for these both. 
Personally, I don't mind which version goes in; I'd say both are as 
readable.

Stephen, Mike, is one of you going to pick this up? I don't think I have 
anything else to pull due to the ongoing discussion with the other 
pending stuff.

-Tero

>
>> Also, checked if the same values are written to clk as with my patch, so
>> here's my:
>> Tested-by: Ladislav Michl <ladis@linux-mips.org>
>
> Thank you.
>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-05  9:36     ` Ladislav Michl
  2016-12-05 11:08       ` Laurent Pinchart
@ 2016-12-05 23:59       ` Laurent Pinchart
  2016-12-07 16:34         ` Ladislav Michl
  1 sibling, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2016-12-05 23:59 UTC (permalink / raw)
  To: Ladislav Michl
  Cc: linux-omap, linux-clk, Paul Walmsley, Tero Kristo, Richard Watts,
	Tony Lindgren, Alexander Kinzer

Hi Ladislav,

On Monday 05 Dec 2016 10:36:49 Ladislav Michl wrote:
> On Mon, Dec 05, 2016 at 10:46:43AM +0200, Laurent Pinchart wrote:
> > On Monday 05 Dec 2016 09:22:10 Ladislav Michl wrote:
> [snip]
> 
> >> 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?
> > 
> > I'd like to, but at the moment I don't see how. Proposals are welcome :-)
> > I
> 
> One of proposals raised earlier was DT property, but that idea was scratched
> later.
> 
> > don't think addressing that issue should be a blocker to get this patch
> > merged though.
> 
> Of course not. I'd like to even see it in stable ;-)
> 
> [snip]
> 
> > I had tried that, but I find the code less readable :-S
> 
> Oh... Please reconsider (I really do not like that extra test and extra
> assignment to local variables (also I had 'precomputed' as mixed definition,
> but Tero did not quite like that)) :-)

I've tested both versions with gcc 4.7.3 [1] and 4.8.5 [2]. With 4.7.3 my 
version is 4 bytes longer, and with 4.8.5 it's 4 bytes shorter. Interestingly 
enough the "break + test after loop" pattern doesn't make a difference, it's 
only the intermediate variable that results in changes to the generated code.

[1] arm-linux-gnueabihf-gcc (crosstool-NG 
linaro-1.13.1-4.7-2013.02-01-20130221 - Linaro GCC 2013.02) 4.7.3 20130205 
(prerelease)

[2] arm-buildroot-linux-uclibcgnueabihf-gcc.br_real (Buildroot 2016.08-dirty) 
4.8.5

> Also, checked if the same values are
> written to clk as with my patch, so here's my:
> Tested-by: Ladislav Michl <ladis@linux-mips.org>

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-05 23:59       ` Laurent Pinchart
@ 2016-12-07 16:34         ` Ladislav Michl
  0 siblings, 0 replies; 21+ messages in thread
From: Ladislav Michl @ 2016-12-07 16:34 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-omap, linux-clk, Paul Walmsley, Tero Kristo, Richard Watts,
	Tony Lindgren, Alexander Kinzer

Hi Laurent,

On Tue, Dec 06, 2016 at 01:59:56AM +0200, Laurent Pinchart wrote:
> Hi Ladislav,
[snip]
> I've tested both versions with gcc 4.7.3 [1] and 4.8.5 [2]. With 4.7.3 my 
> version is 4 bytes longer, and with 4.8.5 it's 4 bytes shorter. Interestingly 
> enough the "break + test after loop" pattern doesn't make a difference, it's 
> only the intermediate variable that results in changes to the generated code.

Interesting exercise... I hereby present 92 bytes shorter version using
gcc version 5.4.0 (OSELAS.Toolchain-2016.06.0)
I hope it's still easily readable.

> [1] arm-linux-gnueabihf-gcc (crosstool-NG 
> linaro-1.13.1-4.7-2013.02-01-20130221 - Linaro GCC 2013.02) 4.7.3 20130205 
> (prerelease)
> 
> [2] arm-buildroot-linux-uclibcgnueabihf-gcc.br_real (Buildroot 2016.08-dirty) 
> 4.8.5

Best regards,
	ladis

diff --git a/drivers/clk/ti/dpll3xxx.c b/drivers/clk/ti/dpll3xxx.c
index 88f2ce8..f7772fc 100644
--- a/drivers/clk/ti/dpll3xxx.c
+++ b/drivers/clk/ti/dpll3xxx.c
@@ -838,3 +838,69 @@ 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;
+		unsigned short m, n;
+	};
+
+	int i;
+	struct dpll_data *dd;
+	struct clk_hw_omap *clk;
+	const struct omap3_dpll5_settings *p;
+	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 }
+	};
+
+	for (i = 0; i< ARRAY_SIZE(precomputed); i++) {
+		p = precomputed + i;
+		if (parent_rate == p->rate) {
+			clk = to_clk_hw_omap(hw);
+			dd = clk->dpll_data;
+			/* Update the M, N and rounded rate values */
+			dd->last_rounded_m = p->m;
+			dd->last_rounded_n = p->n;
+			dd->last_rounded_rate =
+				div_u64((u64)parent_rate * p->m, p->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);
+}
-- 
2.1.4


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-05 12:24           ` Tero Kristo
  (?)
@ 2016-12-08  0:16           ` Stephen Boyd
  2016-12-08  7:11             ` Ladislav Michl
  -1 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2016-12-08  0:16 UTC (permalink / raw)
  To: Tero Kristo
  Cc: Laurent Pinchart, Ladislav Michl, linux-omap, linux-clk,
	Paul Walmsley, Richard Watts, Tony Lindgren, Alexander Kinzer,
	Michael Turquette

On 12/05, Tero Kristo wrote:
> On 05/12/16 13:08, Laurent Pinchart wrote:
> >Hi Ladislav,
> >
> >On Monday 05 Dec 2016 10:36:49 Ladislav Michl wrote:
> >>On Mon, Dec 05, 2016 at 10:46:43AM +0200, Laurent Pinchart wrote:
> >>>On Monday 05 Dec 2016 09:22:10 Ladislav Michl wrote:
> >>
> >>[snip]
> >>
> >>>>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?
> >>>
> >>>I'd like to, but at the moment I don't see how. Proposals are welcome :-)
> >>>I
> >>
> >>One of proposals raised earlier was DT property, but that idea was scratched
> >>later.
> >
> >It might not be such a bad idea, given that the decision should be made based
> >on the characterization of the whole system. One could argue that such
> >platform information could have its place in DT.
> >
> >>>don't think addressing that issue should be a blocker to get this patch
> >>>merged though.
> >>
> >>Of course not. I'd like to even see it in stable ;-)
> >>
> >>[snip]
> >>
> >>>I had tried that, but I find the code less readable :-S
> >>
> >>Oh... Please reconsider (I really do not like that extra test and extra
> >>assignment to local variables (also I had 'precomputed' as mixed definition,
> >>but Tero did not quite like that)) :-)
> >
> >I still like to favour code readability when possible (especially when the
> >compiler should optimize both versions the same way). I'm not the maintainer
> >of this driver though, so I'll let Tero decides what he prefers.
> 
> The compiler should ideally generate same size code for these both.
> Personally, I don't mind which version goes in; I'd say both are as
> readable.
> 
> Stephen, Mike, is one of you going to pick this up? I don't think I
> have anything else to pull due to the ongoing discussion with the
> other pending stuff.
> 

I have no problem picking up either version. Please send it with
the appropriate tags added and I can merge it.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-08  0:16           ` Stephen Boyd
@ 2016-12-08  7:11             ` Ladislav Michl
  2016-12-08 11:40               ` Laurent Pinchart
  0 siblings, 1 reply; 21+ messages in thread
From: Ladislav Michl @ 2016-12-08  7:11 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Tero Kristo, Laurent Pinchart, linux-omap, linux-clk,
	Paul Walmsley, Richard Watts, Tony Lindgren, Alexander Kinzer,
	Michael Turquette

On Wed, Dec 07, 2016 at 04:16:54PM -0800, Stephen Boyd wrote:
> On 12/05, Tero Kristo wrote:
> > The compiler should ideally generate same size code for these both.
> > Personally, I don't mind which version goes in; I'd say both are as
> > readable.
> > 
> > Stephen, Mike, is one of you going to pick this up? I don't think I
> > have anything else to pull due to the ongoing discussion with the
> > other pending stuff.
> 
> I have no problem picking up either version. Please send it with
> the appropriate tags added and I can merge it.

Here it is for your convenience. Of course I'd like to see smaller code
to go in as exactly those sneaky tiny incremental code size increases
made Linux kernel unusable on most of my hardware over time ;-)
However, if Laurent is unhappy with changes I made, I will not object
merging his version any more.

Best regards,
	ladis

-- >8 --
From: Richard Watts <rrw@kynesim.co.uk>
Subject: [PATCH v4] clk: ti: omap36xx: Work around sprz319 advisory 2.1

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>
Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
Changes since v2:
 - Added spaces around '+'

Changes since v3:
 - Small omap3_dpll5_apply_errata rewrite to save few bytes (92 with
   gcc-5.4.0).

 drivers/clk/ti/clk-3xxx.c | 20 +++++++-------
 drivers/clk/ti/clock.h    |  9 +++++++
 drivers/clk/ti/dpll.c     | 19 +++++++++++++-
 drivers/clk/ti/dpll3xxx.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 103 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/ti/clk-3xxx.c b/drivers/clk/ti/clk-3xxx.c
index 8831e1a..11d8aa3 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 90f3f47..13c37f4 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 9fc8754..4b9a419 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 88f2ce8..f7772fc 100644
--- a/drivers/clk/ti/dpll3xxx.c
+++ b/drivers/clk/ti/dpll3xxx.c
@@ -838,3 +838,69 @@ 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;
+		unsigned short m, n;
+	};
+
+	int i;
+	struct dpll_data *dd;
+	struct clk_hw_omap *clk;
+	const struct omap3_dpll5_settings *p;
+	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 }
+	};
+
+	for (i = 0; i< ARRAY_SIZE(precomputed); i++) {
+		p = precomputed + i;
+		if (parent_rate == p->rate) {
+			clk = to_clk_hw_omap(hw);
+			dd = clk->dpll_data;
+			/* Update the M, N and rounded rate values */
+			dd->last_rounded_m = p->m;
+			dd->last_rounded_n = p->n;
+			dd->last_rounded_rate =
+				div_u64((u64)parent_rate * p->m, p->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);
+}
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-08  7:11             ` Ladislav Michl
@ 2016-12-08 11:40               ` Laurent Pinchart
  2016-12-08 21:14                 ` Stephen Boyd
  0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2016-12-08 11:40 UTC (permalink / raw)
  To: Ladislav Michl
  Cc: Stephen Boyd, Tero Kristo, linux-omap, linux-clk, Paul Walmsley,
	Richard Watts, Tony Lindgren, Alexander Kinzer,
	Michael Turquette

Hi Ladislav,

On Thursday 08 Dec 2016 08:11:55 Ladislav Michl wrote:
> On Wed, Dec 07, 2016 at 04:16:54PM -0800, Stephen Boyd wrote:
> > On 12/05, Tero Kristo wrote:
> > > The compiler should ideally generate same size code for these both.
> > > Personally, I don't mind which version goes in; I'd say both are as
> > > readable.
> > > 
> > > Stephen, Mike, is one of you going to pick this up? I don't think I
> > > have anything else to pull due to the ongoing discussion with the
> > > other pending stuff.
> > 
> > I have no problem picking up either version. Please send it with
> > the appropriate tags added and I can merge it.
> 
> Here it is for your convenience. Of course I'd like to see smaller code
> to go in as exactly those sneaky tiny incremental code size increases
> made Linux kernel unusable on most of my hardware over time ;-)
> However, if Laurent is unhappy with changes I made, I will not object
> merging his version any more.

I'd rather keep my version, thank you. Let's not spend time chasing bytes.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-08 11:40               ` Laurent Pinchart
@ 2016-12-08 21:14                 ` Stephen Boyd
  0 siblings, 0 replies; 21+ messages in thread
From: Stephen Boyd @ 2016-12-08 21:14 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Ladislav Michl, Tero Kristo, linux-omap, linux-clk,
	Paul Walmsley, Richard Watts, Tony Lindgren, Alexander Kinzer,
	Michael Turquette

On 12/08, Laurent Pinchart wrote:
> Hi Ladislav,
> 
> On Thursday 08 Dec 2016 08:11:55 Ladislav Michl wrote:
> > On Wed, Dec 07, 2016 at 04:16:54PM -0800, Stephen Boyd wrote:
> > > On 12/05, Tero Kristo wrote:
> > > > The compiler should ideally generate same size code for these both.
> > > > Personally, I don't mind which version goes in; I'd say both are as
> > > > readable.
> > > > 
> > > > Stephen, Mike, is one of you going to pick this up? I don't think I
> > > > have anything else to pull due to the ongoing discussion with the
> > > > other pending stuff.
> > > 
> > > I have no problem picking up either version. Please send it with
> > > the appropriate tags added and I can merge it.
> > 
> > Here it is for your convenience. Of course I'd like to see smaller code
> > to go in as exactly those sneaky tiny incremental code size increases
> > made Linux kernel unusable on most of my hardware over time ;-)
> > However, if Laurent is unhappy with changes I made, I will not object
> > merging his version any more.
> 
> I'd rather keep my version, thank you. Let's not spend time chasing bytes.
> 

Ok so I'll go apply the original patch v3 in this thread and dig
out the tags myself.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-02 21:14 [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1 Laurent Pinchart
  2016-12-02 22:41   ` Michael Turquette
  2016-12-05  8:22 ` Ladislav Michl
@ 2016-12-08 21:16 ` Stephen Boyd
  2016-12-08 21:24   ` Laurent Pinchart
  2 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2016-12-08 21:16 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-omap, linux-clk, Paul Walmsley, Tero Kristo, Richard Watts,
	Tony Lindgren, Alexander Kinzer

On 12/02, 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>
> ---

Applied to clk-next

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-08 21:16 ` Stephen Boyd
@ 2016-12-08 21:24   ` Laurent Pinchart
  2017-01-03 18:00     ` Adam Ford
  0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2016-12-08 21:24 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: linux-omap, linux-clk, Paul Walmsley, Tero Kristo, Richard Watts,
	Tony Lindgren, Alexander Kinzer

Hi Stephen,

On Thursday 08 Dec 2016 13:16:01 Stephen Boyd wrote:
> On 12/02, 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>
> > ---
> 
> Applied to clk-next

Thank you.

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2016-12-08 21:24   ` Laurent Pinchart
@ 2017-01-03 18:00     ` Adam Ford
  2017-01-03 18:49       ` Stephen Boyd
  0 siblings, 1 reply; 21+ messages in thread
From: Adam Ford @ 2017-01-03 18:00 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Stephen Boyd, linux-omap, linux-clk, Paul Walmsley, Tero Kristo,
	Richard Watts, Tony Lindgren, Alexander Kinzer

On Thu, Dec 8, 2016 at 3:24 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Stephen,
>
> On Thursday 08 Dec 2016 13:16:01 Stephen Boyd wrote:
>> On 12/02, 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>
>> > ---
>>
>> Applied to clk-next
>

Since this fixes an errata issue, is there any way we can get this
patch applied to the older LTS kernels?  (4.4 seems to apply cleanly,
but I didn't try any older than that)

adam
> Thank you.
>
> --
> Regards,
>
> Laurent Pinchart
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2017-01-03 18:00     ` Adam Ford
@ 2017-01-03 18:49       ` Stephen Boyd
  2017-01-03 22:16         ` Laurent Pinchart
  0 siblings, 1 reply; 21+ messages in thread
From: Stephen Boyd @ 2017-01-03 18:49 UTC (permalink / raw)
  To: Adam Ford
  Cc: Laurent Pinchart, linux-omap, linux-clk, Paul Walmsley,
	Tero Kristo, Richard Watts, Tony Lindgren, Alexander Kinzer

On 01/03, Adam Ford wrote:
> On Thu, Dec 8, 2016 at 3:24 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Stephen,
> >
> > On Thursday 08 Dec 2016 13:16:01 Stephen Boyd wrote:
> >> On 12/02, 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>
> >> > ---
> >>
> >> Applied to clk-next
> >
> 
> Since this fixes an errata issue, is there any way we can get this
> patch applied to the older LTS kernels?  (4.4 seems to apply cleanly,
> but I didn't try any older than that)
> 

Sure. Just follow the stable kernel rules. See
Documentation/process/stable-kernel-rules.rst, specifically
option #2.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2017-01-03 18:49       ` Stephen Boyd
@ 2017-01-03 22:16         ` Laurent Pinchart
  2017-01-04 12:59           ` Adam Ford
  0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2017-01-03 22:16 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Adam Ford, linux-omap, linux-clk, Paul Walmsley, Tero Kristo,
	Richard Watts, Tony Lindgren, Alexander Kinzer

On Tuesday 03 Jan 2017 10:49:56 Stephen Boyd wrote:
> On 01/03, Adam Ford wrote:
> > On Thu, Dec 8, 2016 at 3:24 PM, Laurent Pinchart wrote:
> >> On Thursday 08 Dec 2016 13:16:01 Stephen Boyd wrote:
> >>> On 12/02, 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>
> >>>> ---
> >>> 
> >>> Applied to clk-next
> > 
> > Since this fixes an errata issue, is there any way we can get this
> > patch applied to the older LTS kernels?  (4.4 seems to apply cleanly,
> > but I didn't try any older than that)
> 
> Sure. Just follow the stable kernel rules. See
> Documentation/process/stable-kernel-rules.rst, specifically
> option #2.

And needless to say, please test the backported patch before requesting it to 
be included in stable kernels :-)

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1
  2017-01-03 22:16         ` Laurent Pinchart
@ 2017-01-04 12:59           ` Adam Ford
  0 siblings, 0 replies; 21+ messages in thread
From: Adam Ford @ 2017-01-04 12:59 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Stephen Boyd, linux-omap, linux-clk, Paul Walmsley, Tero Kristo,
	Richard Watts, Tony Lindgren, Alexander Kinzer

On Tue, Jan 3, 2017 at 4:16 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tuesday 03 Jan 2017 10:49:56 Stephen Boyd wrote:
>> On 01/03, Adam Ford wrote:
>> > On Thu, Dec 8, 2016 at 3:24 PM, Laurent Pinchart wrote:
>> >> On Thursday 08 Dec 2016 13:16:01 Stephen Boyd wrote:
>> >>> On 12/02, 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>
>> >>>> ---
>> >>>
>> >>> Applied to clk-next
>> >
>> > Since this fixes an errata issue, is there any way we can get this
>> > patch applied to the older LTS kernels?  (4.4 seems to apply cleanly,
>> > but I didn't try any older than that)
>>
>> Sure. Just follow the stable kernel rules. See
>> Documentation/process/stable-kernel-rules.rst, specifically
>> option #2.
>
> And needless to say, please test the backported patch before requesting it to
> be included in stable kernels :-)
>

Tested against linux-stable 4.4.y branch, linux-stable-4.8.y branch, and 4.9.0

:-)


> --
> Regards,
>
> Laurent Pinchart
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2017-01-04 12:59 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-02 21:14 [PATCH v3] clk: ti: omap36xx: Work around sprz319 advisory 2.1 Laurent Pinchart
2016-12-02 22:41 ` Michael Turquette
2016-12-02 22:41   ` Michael Turquette
2016-12-05  8:22 ` Ladislav Michl
2016-12-05  8:46   ` Laurent Pinchart
2016-12-05  9:36     ` Ladislav Michl
2016-12-05 11:08       ` Laurent Pinchart
2016-12-05 12:24         ` Tero Kristo
2016-12-05 12:24           ` Tero Kristo
2016-12-08  0:16           ` Stephen Boyd
2016-12-08  7:11             ` Ladislav Michl
2016-12-08 11:40               ` Laurent Pinchart
2016-12-08 21:14                 ` Stephen Boyd
2016-12-05 23:59       ` Laurent Pinchart
2016-12-07 16:34         ` Ladislav Michl
2016-12-08 21:16 ` Stephen Boyd
2016-12-08 21:24   ` Laurent Pinchart
2017-01-03 18:00     ` Adam Ford
2017-01-03 18:49       ` Stephen Boyd
2017-01-03 22:16         ` Laurent Pinchart
2017-01-04 12:59           ` Adam Ford

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.