[PATCH 0/2] clk: enforce CLK_SET_RATE_GATE along the tree

[PATCH 0/2] clk: enforce CLK_SET_RATE_GATE along the tree

[Jerome Brunet]

This is a resend of the last unmerged patch of the clock protect v5 [0]
patchset. It makes use of the clock protection mechanism to fix and
enforce CLK_SET_RATE_GATE flag.

With this flag set, any operation resulting in a rate change or glitch
should be prevented. This might be useful when, for example, a PLL can't
safely relock while in use.

ATM, this is enforced only on the leaf clock of the operation (the clock
on which clk_set_rate() is called). It is ignored otherwise. It means
that, if:
 * clock A has CLK_SET_RATE_PARENT and is a child on clock B
 * clock B has CLK_SET_RATE_GATE set
it is possible to:
 * call clk_prepare_enable on clock A (clock B also gets enabled)
 * call clk_set_rate() on clock A,
  => with rate propagation clock B (or its parents) might change while
     enabled

Patch 2 of this series fixes this problem.

Some platform may have been relying on the broken implementation of
CLK_SET_RATE_GATE but, since the platform actually bypassed
CLK_SET_RATE_GATE, we can drop the flag without changing anything.

This is the case for qcom SDC clock and mmci driver, which does the
following sequence:
 * clk_get()
 * clk_prepare_enable()
 * clk_get_rate()
 * clk_set_rate()
This is obviously not possible when CLK_SET_RATE_GATE is set.
Patch 1 drops the CLK_SET_RATE_GATE from qcom sdc clocks. Other qcom's
clocks are using the same scheme, so maybe there something worth
checking here.

This case was detected using kernelCI but some other platforms might be
doing the same. Maintainers of the platforms using this flag have been
warned ... but please test!

[0]: https://lkml.kernel.org/r/20171201215200.23523-1-jbrunet@baylibre.com

Jerome Brunet (2):
  clk: qcom: drop CLK_SET_RATE_GATE from sdc clocks
  clk: fix CLK_SET_RATE_GATE with clock rate protection

 drivers/clk/clk.c              | 16 +++++++++++++---
 drivers/clk/qcom/gcc-ipq806x.c |  3 ---
 drivers/clk/qcom/gcc-mdm9615.c |  2 --
 drivers/clk/qcom/gcc-msm8660.c |  5 -----
 drivers/clk/qcom/gcc-msm8960.c |  5 -----
 5 files changed, 13 insertions(+), 18 deletions(-)

-- 
2.14.3

[PATCH 1/2] clk: qcom: drop CLK_SET_RATE_GATE from sdc clocks

[Jerome Brunet]

the mmci driver (drivers/mmc/host/mmci.c) does the following sequence:
* clk_prepare_enable()
* clk_set_rate()

on SDCx_clk which is a children of SDCx_src. SDCx_src has
CLK_SET_RATE_GATE so this sequence should not be allowed but this was not
enforced. IOW, the flag is ignored. Dropping the flag won't change
anything to the current behaviour of the platform.

CLK_SET_RATE_GATE is being fixed and enforced now. If the flag was kept,
the mmci driver would receive -EBUSY when calling clk_set_rate()

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/qcom/gcc-ipq806x.c | 3 ---
 drivers/clk/qcom/gcc-mdm9615.c | 2 --
 drivers/clk/qcom/gcc-msm8660.c | 5 -----
 drivers/clk/qcom/gcc-msm8960.c | 5 -----
 4 files changed, 15 deletions(-)

diff --git a/drivers/clk/qcom/gcc-ipq806x.c b/drivers/clk/qcom/gcc-ipq806x.c
index 28eb200d0f1e..5f61225657ab 100644
--- a/drivers/clk/qcom/gcc-ipq806x.c
+++ b/drivers/clk/qcom/gcc-ipq806x.c
@@ -1220,7 +1220,6 @@ static struct clk_rcg sdc1_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
@@ -1269,7 +1268,6 @@ static struct clk_rcg sdc3_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
@@ -1353,7 +1351,6 @@ static struct clk_rcg tsif_ref_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
diff --git a/drivers/clk/qcom/gcc-mdm9615.c b/drivers/clk/qcom/gcc-mdm9615.c
index b99dd406e907..849046fbed6d 100644
--- a/drivers/clk/qcom/gcc-mdm9615.c
+++ b/drivers/clk/qcom/gcc-mdm9615.c
@@ -947,7 +947,6 @@ static struct clk_rcg sdc1_src = {
 			.parent_names = gcc_cxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
@@ -996,7 +995,6 @@ static struct clk_rcg sdc2_src = {
 			.parent_names = gcc_cxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
diff --git a/drivers/clk/qcom/gcc-msm8660.c b/drivers/clk/qcom/gcc-msm8660.c
index c347a0d44bc8..7e930e25c79f 100644
--- a/drivers/clk/qcom/gcc-msm8660.c
+++ b/drivers/clk/qcom/gcc-msm8660.c
@@ -1558,7 +1558,6 @@ static struct clk_rcg sdc1_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
@@ -1607,7 +1606,6 @@ static struct clk_rcg sdc2_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
@@ -1656,7 +1654,6 @@ static struct clk_rcg sdc3_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
@@ -1705,7 +1702,6 @@ static struct clk_rcg sdc4_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
@@ -1754,7 +1750,6 @@ static struct clk_rcg sdc5_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c
index eb551c75fba6..fd495e0471bb 100644
--- a/drivers/clk/qcom/gcc-msm8960.c
+++ b/drivers/clk/qcom/gcc-msm8960.c
@@ -1628,7 +1628,6 @@ static struct clk_rcg sdc1_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
@@ -1677,7 +1676,6 @@ static struct clk_rcg sdc2_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
@@ -1726,7 +1724,6 @@ static struct clk_rcg sdc3_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
@@ -1775,7 +1772,6 @@ static struct clk_rcg sdc4_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
@@ -1824,7 +1820,6 @@ static struct clk_rcg sdc5_src = {
 			.parent_names = gcc_pxo_pll8,
 			.num_parents = 2,
 			.ops = &clk_rcg_ops,
-			.flags = CLK_SET_RATE_GATE,
 		},
 	}
 };
-- 
2.14.3

Re: [PATCH 1/2] clk: qcom: drop CLK_SET_RATE_GATE from sdc clocks

[Michael Turquette]

Quoting Jerome Brunet (2018-06-19 06:40:50)
> the mmci driver (drivers/mmc/host/mmci.c) does the following sequence:
> * clk_prepare_enable()
> * clk_set_rate()
> =

> on SDCx_clk which is a children of SDCx_src. SDCx_src has
> CLK_SET_RATE_GATE so this sequence should not be allowed but this was not
> enforced. IOW, the flag is ignored. Dropping the flag won't change
> anything to the current behaviour of the platform.
> =

> CLK_SET_RATE_GATE is being fixed and enforced now. If the flag was kept,
> the mmci driver would receive -EBUSY when calling clk_set_rate()
> =

> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

Applied to clk-qcom-set-rate-gate.

Thanks,
Mike

> ---
>  drivers/clk/qcom/gcc-ipq806x.c | 3 ---
>  drivers/clk/qcom/gcc-mdm9615.c | 2 --
>  drivers/clk/qcom/gcc-msm8660.c | 5 -----
>  drivers/clk/qcom/gcc-msm8960.c | 5 -----
>  4 files changed, 15 deletions(-)
> =

> diff --git a/drivers/clk/qcom/gcc-ipq806x.c b/drivers/clk/qcom/gcc-ipq806=
x.c
> index 28eb200d0f1e..5f61225657ab 100644
> --- a/drivers/clk/qcom/gcc-ipq806x.c
> +++ b/drivers/clk/qcom/gcc-ipq806x.c
> @@ -1220,7 +1220,6 @@ static struct clk_rcg sdc1_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> @@ -1269,7 +1268,6 @@ static struct clk_rcg sdc3_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> @@ -1353,7 +1351,6 @@ static struct clk_rcg tsif_ref_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> diff --git a/drivers/clk/qcom/gcc-mdm9615.c b/drivers/clk/qcom/gcc-mdm961=
5.c
> index b99dd406e907..849046fbed6d 100644
> --- a/drivers/clk/qcom/gcc-mdm9615.c
> +++ b/drivers/clk/qcom/gcc-mdm9615.c
> @@ -947,7 +947,6 @@ static struct clk_rcg sdc1_src =3D {
>                         .parent_names =3D gcc_cxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> @@ -996,7 +995,6 @@ static struct clk_rcg sdc2_src =3D {
>                         .parent_names =3D gcc_cxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> diff --git a/drivers/clk/qcom/gcc-msm8660.c b/drivers/clk/qcom/gcc-msm866=
0.c
> index c347a0d44bc8..7e930e25c79f 100644
> --- a/drivers/clk/qcom/gcc-msm8660.c
> +++ b/drivers/clk/qcom/gcc-msm8660.c
> @@ -1558,7 +1558,6 @@ static struct clk_rcg sdc1_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> @@ -1607,7 +1606,6 @@ static struct clk_rcg sdc2_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> @@ -1656,7 +1654,6 @@ static struct clk_rcg sdc3_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> @@ -1705,7 +1702,6 @@ static struct clk_rcg sdc4_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> @@ -1754,7 +1750,6 @@ static struct clk_rcg sdc5_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm896=
0.c
> index eb551c75fba6..fd495e0471bb 100644
> --- a/drivers/clk/qcom/gcc-msm8960.c
> +++ b/drivers/clk/qcom/gcc-msm8960.c
> @@ -1628,7 +1628,6 @@ static struct clk_rcg sdc1_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> @@ -1677,7 +1676,6 @@ static struct clk_rcg sdc2_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> @@ -1726,7 +1724,6 @@ static struct clk_rcg sdc3_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> @@ -1775,7 +1772,6 @@ static struct clk_rcg sdc4_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> @@ -1824,7 +1820,6 @@ static struct clk_rcg sdc5_src =3D {
>                         .parent_names =3D gcc_pxo_pll8,
>                         .num_parents =3D 2,
>                         .ops =3D &clk_rcg_ops,
> -                       .flags =3D CLK_SET_RATE_GATE,
>                 },
>         }
>  };
> -- =

> 2.14.3
>=20

[PATCH 2/2] clk: fix CLK_SET_RATE_GATE with clock rate protection

[Jerome Brunet]

CLK_SET_RATE_GATE should prevent any operation which may result in a rate
change or glitch while the clock is prepared/enabled.

IOW, the following sequence is not allowed anymore with CLK_SET_RATE_GATE:
* clk_get()
* clk_prepare_enable()
* clk_get_rate()
* clk_set_rate()

At the moment this is enforced on the leaf clock of the operation, not
along the tree. This problematic because, if a PLL has the CLK_RATE_GATE,
it won't be enforced if the clk_set_rate() is called on its child clocks.

Using clock rate protection, we can now enforce CLK_SET_RATE_GATE along the
clock tree

Acked-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/clk.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 9760b526ca31..97c09243fb21 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -691,6 +691,9 @@ static void clk_core_unprepare(struct clk_core *core)
 	    "Unpreparing critical %s\n", core->name))
 		return;
 
+	if (core->flags & CLK_SET_RATE_GATE)
+		clk_core_rate_unprotect(core);
+
 	if (--core->prepare_count > 0)
 		return;
 
@@ -765,6 +768,16 @@ static int clk_core_prepare(struct clk_core *core)
 
 	core->prepare_count++;
 
+	/*
+	 * CLK_SET_RATE_GATE is a special case of clock protection
+	 * Instead of a consumer claiming exclusive rate control, it is
+	 * actually the provider which prevents any consumer from making any
+	 * operation which could result in a rate change or rate glitch while
+	 * the clock is prepared.
+	 */
+	if (core->flags & CLK_SET_RATE_GATE)
+		clk_core_rate_protect(core);
+
 	return 0;
 unprepare:
 	clk_core_unprepare(core->parent);
@@ -1888,9 +1901,6 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	if (clk_core_rate_is_protected(core))
 		return -EBUSY;
 
-	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
-		return -EBUSY;
-
 	/* calculate new rates and get the topmost changed clock */
 	top = clk_calc_new_rates(core, req_rate);
 	if (!top)
-- 
2.14.3

Re: [PATCH 2/2] clk: fix CLK_SET_RATE_GATE with clock rate protection

[Michael Turquette]

Quoting Jerome Brunet (2018-06-19 06:40:51)
> CLK_SET_RATE_GATE should prevent any operation which may result in a rate
> change or glitch while the clock is prepared/enabled.
> =

> IOW, the following sequence is not allowed anymore with CLK_SET_RATE_GATE:
> * clk_get()
> * clk_prepare_enable()
> * clk_get_rate()
> * clk_set_rate()
> =

> At the moment this is enforced on the leaf clock of the operation, not
> along the tree. This problematic because, if a PLL has the CLK_RATE_GATE,
> it won't be enforced if the clk_set_rate() is called on its child clocks.
> =

> Using clock rate protection, we can now enforce CLK_SET_RATE_GATE along t=
he
> clock tree
> =

> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Tested-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> Tested-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

Applied to clk-core-set-rate-gate.

Thanks,
Mike

> ---
>  drivers/clk/clk.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> =

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 9760b526ca31..97c09243fb21 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -691,6 +691,9 @@ static void clk_core_unprepare(struct clk_core *core)
>             "Unpreparing critical %s\n", core->name))
>                 return;
>  =

> +       if (core->flags & CLK_SET_RATE_GATE)
> +               clk_core_rate_unprotect(core);
> +
>         if (--core->prepare_count > 0)
>                 return;
>  =

> @@ -765,6 +768,16 @@ static int clk_core_prepare(struct clk_core *core)
>  =

>         core->prepare_count++;
>  =

> +       /*
> +        * CLK_SET_RATE_GATE is a special case of clock protection
> +        * Instead of a consumer claiming exclusive rate control, it is
> +        * actually the provider which prevents any consumer from making =
any
> +        * operation which could result in a rate change or rate glitch w=
hile
> +        * the clock is prepared.
> +        */
> +       if (core->flags & CLK_SET_RATE_GATE)
> +               clk_core_rate_protect(core);
> +
>         return 0;
>  unprepare:
>         clk_core_unprepare(core->parent);
> @@ -1888,9 +1901,6 @@ static int clk_core_set_rate_nolock(struct clk_core=
 *core,
>         if (clk_core_rate_is_protected(core))
>                 return -EBUSY;
>  =

> -       if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
> -               return -EBUSY;
> -
>         /* calculate new rates and get the topmost changed clock */
>         top =3D clk_calc_new_rates(core, req_rate);
>         if (!top)
> -- =

> 2.14.3
>=20