[PATCH 0/4] clk: Rate range improvements

[PATCH 0/4] clk: Rate range improvements

[Maxime Ripard]

Hi,

Here's a bunch of patches for issues I came across while debugging the bug
reported by Angelo.

The most important one is the first one. Even though it looks innoculous, it
fixes the bug in question for some reason.

Let me know what you think,
Maxime

To: Michael Turquette <mturquette@baylibre.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---
Maxime Ripard (4):
      clk: Remove WARN_ON NULL parent in clk_core_init_rate_req()
      clk: Initialize the clk_rate_request even if clk_core is NULL
      clk: Initialize max_rate in struct clk_rate_request
      clk: Warn if we register a mux without determine_rate

 drivers/clk/clk.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
---
base-commit: 56e8142dda103af35e1a47e560517dce355ac001
change-id: 20221018-clk-range-checks-fixes-2039f3523240

Best regards,
-- 
Maxime Ripard <maxime@cerno.tech>

[PATCH 1/4] clk: Remove WARN_ON NULL parent in clk_core_init_rate_req()

[Maxime Ripard]

If a clock has CLK_SET_RATE_PARENT, but core->parent is NULL (most
likely because it's orphan), callers of clk_core_init_rate_req() will
blindly call this function leading to a very verbose warning.

Since it's a fairly common situation, let's just remove the WARN_ON but
keep the check that prevents us from dereferencing the pointer.

Interestingly, it fixes a regression on the Mediatek MT8195 where the
GPU would stall during a clk_set_rate for its main clock. We couldn't
come up with a proper explanation since the condition is essentially the
same.

It was then assumed that it could be timing related since printing the
warning stacktrace takes a while, but we couldn't replicate the failure
by using fairly large (10ms) mdelays.

Fixes: 262ca38f4b6e ("clk: Stop forwarding clk_rate_requests to the parent")
Reported-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c3c3f8c07258..37d623c7b73b 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1459,7 +1459,7 @@ static void clk_core_init_rate_req(struct clk_core * const core,
 {
 	struct clk_core *parent;
 
-	if (WARN_ON(!core || !req))
+	if (!core || WARN_ON(!req))
 		return;
 
 	memset(req, 0, sizeof(*req));

-- 
b4 0.11.0-dev-7da52

[PATCH 2/4] clk: Initialize the clk_rate_request even if clk_core is NULL

[Maxime Ripard]

Since commit c35e84b09776 ("clk: Introduce clk_hw_init_rate_request()"),
users that used to initialize their clk_rate_request by initializing
their local structure now rely on clk_hw_init_rate_request().

This function is backed by clk_core_init_rate_req(), which will skip the
initialization if either the pointer to struct clk_core or to struct
clk_rate_request are NULL.

However, the core->parent pointer might be NULL because the clock is
orphan, and we will thus end up with our local struct clk_rate_request
left untouched.

And since clk_hw_init_rate_request() doesn't return an error, we will
then call a determine_rate variant with that unitialized structure.

In order to avoid this, let's clear our clk_rate_request if the pointer
to it is valid but the pointer to struct clk_core isn't.

Fixes: c35e84b09776 ("clk: Introduce clk_hw_init_rate_request()")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 37d623c7b73b..eb2f9be9b9aa 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1459,11 +1459,14 @@ static void clk_core_init_rate_req(struct clk_core * const core,
 {
 	struct clk_core *parent;
 
-	if (!core || WARN_ON(!req))
+	if (WARN_ON(!req))
 		return;
 
 	memset(req, 0, sizeof(*req));
 
+	if (!core)
+		return;
+
 	req->rate = rate;
 	clk_core_get_boundaries(core, &req->min_rate, &req->max_rate);
 

-- 
b4 0.11.0-dev-7da52

[PATCH 3/4] clk: Initialize max_rate in struct clk_rate_request

[Maxime Ripard]

Since commit b46fd8dbe8ad ("clk: Zero the clk_rate_request structure"),
the clk_core_init_rate_req() function clears the struct clk_rate_request
passed as argument.

However, the default value for max_rate isn't 0 but ULONG_MAX, and we
end up creating a clk_rate_request instance where the maximum rate is 0.

Let's initialize max_rate to ULONG_MAX properly.

Fixes: b46fd8dbe8ad ("clk: Zero the clk_rate_request structure")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index eb2f9be9b9aa..57b83665e5c3 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1463,6 +1463,7 @@ static void clk_core_init_rate_req(struct clk_core * const core,
 		return;
 
 	memset(req, 0, sizeof(*req));
+	req->max_rate = ULONG_MAX;
 
 	if (!core)
 		return;

-- 
b4 0.11.0-dev-7da52

[PATCH 4/4] clk: Warn if we register a mux without determine_rate

[Maxime Ripard]

The determine_rate hook allows to select the proper parent and its rate
for a given clock configuration. On another hand, set_parent is there to
change the parent of a mux.

Some clocks provide a set_parent hook but don't implement
determine_rate. In such a case, set_parent is pretty much useless since
the clock framework will always assume the current parent is to be used,
and we will thus never change it.

This situation can be solved in two ways:
  - either we don't need to change the parent, and we thus shouldn't
    implement set_parent;
  - or we don't want to change the parent, in this case we should set
    CLK_SET_RATE_NO_REPARENT;
  - or we're missing a determine_rate implementation.

The latter is probably just an oversight from the driver's author, and
we should thus raise their awareness about the fact that the current
state of the driver is confusing.

It's not clear at this point how many drivers are affected though, so
let's make it a warning instead of an error for now.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 57b83665e5c3..11c41d987ff4 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3700,6 +3700,11 @@ static int __clk_core_init(struct clk_core *core)
 		goto out;
 	}
 
+	/* TODO: Promote to an error */
+	if (core->ops->set_parent && !core->ops->determine_rate)
+		pr_warn("%s: %s must implement .set_parent & .determine_rate\n",
+			__func__, core->name);
+
 	if (core->num_parents > 1 && !core->ops->get_parent) {
 		pr_err("%s: %s must implement .get_parent as it has multi parents\n",
 		       __func__, core->name);

-- 
b4 0.11.0-dev-7da52

Re: [PATCH 4/4] clk: Warn if we register a mux without determine_rate

[AngeloGioacchino Del Regno]

Il 18/10/22 15:52, Maxime Ripard ha scritto:
> The determine_rate hook allows to select the proper parent and its rate
> for a given clock configuration. On another hand, set_parent is there to
> change the parent of a mux.
> 
> Some clocks provide a set_parent hook but don't implement
> determine_rate. In such a case, set_parent is pretty much useless since
> the clock framework will always assume the current parent is to be used,
> and we will thus never change it.
> 
> This situation can be solved in two ways:
>    - either we don't need to change the parent, and we thus shouldn't
>      implement set_parent;
>    - or we don't want to change the parent, in this case we should set
>      CLK_SET_RATE_NO_REPARENT;
>    - or we're missing a determine_rate implementation.
> 
> The latter is probably just an oversight from the driver's author, and
> we should thus raise their awareness about the fact that the current
> state of the driver is confusing.
> 
> It's not clear at this point how many drivers are affected though, so
> let's make it a warning instead of an error for now.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

That solves my concerns :-) :-)

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 2/4] clk: Initialize the clk_rate_request even if clk_core is NULL

[AngeloGioacchino Del Regno]

Il 18/10/22 15:52, Maxime Ripard ha scritto:
> Since commit c35e84b09776 ("clk: Introduce clk_hw_init_rate_request()"),
> users that used to initialize their clk_rate_request by initializing
> their local structure now rely on clk_hw_init_rate_request().
> 
> This function is backed by clk_core_init_rate_req(), which will skip the
> initialization if either the pointer to struct clk_core or to struct
> clk_rate_request are NULL.
> 
> However, the core->parent pointer might be NULL because the clock is
> orphan, and we will thus end up with our local struct clk_rate_request
> left untouched.
> 
> And since clk_hw_init_rate_request() doesn't return an error, we will
> then call a determine_rate variant with that unitialized structure.
> 
> In order to avoid this, let's clear our clk_rate_request if the pointer
> to it is valid but the pointer to struct clk_core isn't.
> 
> Fixes: c35e84b09776 ("clk: Introduce clk_hw_init_rate_request()")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 1/4] clk: Remove WARN_ON NULL parent in clk_core_init_rate_req()

[AngeloGioacchino Del Regno]

Il 18/10/22 15:52, Maxime Ripard ha scritto:
> If a clock has CLK_SET_RATE_PARENT, but core->parent is NULL (most
> likely because it's orphan), callers of clk_core_init_rate_req() will
> blindly call this function leading to a very verbose warning.
> 
> Since it's a fairly common situation, let's just remove the WARN_ON but
> keep the check that prevents us from dereferencing the pointer.
> 
> Interestingly, it fixes a regression on the Mediatek MT8195 where the
> GPU would stall during a clk_set_rate for its main clock. We couldn't
> come up with a proper explanation since the condition is essentially the
> same.
> 
> It was then assumed that it could be timing related since printing the
> warning stacktrace takes a while, but we couldn't replicate the failure
> by using fairly large (10ms) mdelays.
> 
> Fixes: 262ca38f4b6e ("clk: Stop forwarding clk_rate_requests to the parent")
> Reported-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 3/4] clk: Initialize max_rate in struct clk_rate_request

[AngeloGioacchino Del Regno]

Il 18/10/22 15:52, Maxime Ripard ha scritto:
> Since commit b46fd8dbe8ad ("clk: Zero the clk_rate_request structure"),
> the clk_core_init_rate_req() function clears the struct clk_rate_request
> passed as argument.
> 
> However, the default value for max_rate isn't 0 but ULONG_MAX, and we
> end up creating a clk_rate_request instance where the maximum rate is 0.
> 
> Let's initialize max_rate to ULONG_MAX properly.
> 
> Fixes: b46fd8dbe8ad ("clk: Zero the clk_rate_request structure")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH 4/4] clk: Warn if we register a mux without determine_rate

[Stephen Boyd]

Quoting Maxime Ripard (2022-10-18 06:52:59)
> The determine_rate hook allows to select the proper parent and its rate
> for a given clock configuration. On another hand, set_parent is there to
> change the parent of a mux.
> 
> Some clocks provide a set_parent hook but don't implement
> determine_rate. In such a case, set_parent is pretty much useless since
> the clock framework will always assume the current parent is to be used,
> and we will thus never change it.
> 
> This situation can be solved in two ways:
>   - either we don't need to change the parent, and we thus shouldn't
>     implement set_parent;
>   - or we don't want to change the parent, in this case we should set
>     CLK_SET_RATE_NO_REPARENT;
>   - or we're missing a determine_rate implementation.
> 
> The latter is probably just an oversight from the driver's author, and
> we should thus raise their awareness about the fact that the current
> state of the driver is confusing.

There is another case which is a leaf clk that is a mux where you only
expect clk_set_parent() to be used, and not clk_set_rate(). This use
case is odd though, so I'm not sure how much we care.

> 
> It's not clear at this point how many drivers are affected though, so
> let's make it a warning instead of an error for now.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/clk.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 57b83665e5c3..11c41d987ff4 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3700,6 +3700,11 @@ static int __clk_core_init(struct clk_core *core)
>                 goto out;
>         }
>  
> +       /* TODO: Promote to an error */

The documentation should be updated in this patch (see the table of
hardware characteristics in Documentation/driver-api/clk.rst).

> +       if (core->ops->set_parent && !core->ops->determine_rate)
> +               pr_warn("%s: %s must implement .set_parent & .determine_rate\n",

You can grep for it:

 $ git grep -W 'struct clk_ops .*='

but I'm fairly certain a coccinelle script can detect most of these
because clk_ops are usually statically defined (although not always).

Either way I already see that 'owl_comp_div_ops' will trigger this
warning. And 'at91sam9x5_smd_ops' looks even more likely. Given that I'm
not super keen on applying this patch.

Re: [PATCH 4/4] clk: Warn if we register a mux without determine_rate

[maxime]

[-- Attachment #1: Type: text/plain, Size: 3003 bytes --]

Hi,

On Tue, Oct 25, 2022 at 07:07:58PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-10-18 06:52:59)
> > The determine_rate hook allows to select the proper parent and its rate
> > for a given clock configuration. On another hand, set_parent is there to
> > change the parent of a mux.
> > 
> > Some clocks provide a set_parent hook but don't implement
> > determine_rate. In such a case, set_parent is pretty much useless since
> > the clock framework will always assume the current parent is to be used,
> > and we will thus never change it.
> > 
> > This situation can be solved in two ways:
> >   - either we don't need to change the parent, and we thus shouldn't
> >     implement set_parent;
> >   - or we don't want to change the parent, in this case we should set
> >     CLK_SET_RATE_NO_REPARENT;
> >   - or we're missing a determine_rate implementation.
> > 
> > The latter is probably just an oversight from the driver's author, and
> > we should thus raise their awareness about the fact that the current
> > state of the driver is confusing.
> 
> There is another case which is a leaf clk that is a mux where you only
> expect clk_set_parent() to be used, and not clk_set_rate(). This use
> case is odd though, so I'm not sure how much we care.
> 
> > 
> > It's not clear at this point how many drivers are affected though, so
> > let's make it a warning instead of an error for now.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/clk/clk.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 57b83665e5c3..11c41d987ff4 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3700,6 +3700,11 @@ static int __clk_core_init(struct clk_core *core)
> >                 goto out;
> >         }
> >  
> > +       /* TODO: Promote to an error */
> 
> The documentation should be updated in this patch (see the table of
> hardware characteristics in Documentation/driver-api/clk.rst).
> 
> > +       if (core->ops->set_parent && !core->ops->determine_rate)
> > +               pr_warn("%s: %s must implement .set_parent & .determine_rate\n",
> 
> You can grep for it:
> 
>  $ git grep -W 'struct clk_ops .*='

TIL about -W. It's awesome, thanks

> but I'm fairly certain a coccinelle script can detect most of these
> because clk_ops are usually statically defined (although not always).
> 
> Either way I already see that 'owl_comp_div_ops' will trigger this
> warning. And 'at91sam9x5_smd_ops' looks even more likely. Given that I'm
> not super keen on applying this patch.

It's the reason why I didn't return an error at first, I wanted to
report that it's invalid and let to drivers the chance to be fixed
still.

Should I take your above comment as you'd rather have the affected
drivers fixed in this patch and we then return an error, or is it that
you don't want that patch at all?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 4/4] clk: Warn if we register a mux without determine_rate

[Stephen Boyd]

Quoting maxime@cerno.tech (2022-10-26 06:52:15)
> On Tue, Oct 25, 2022 at 07:07:58PM -0700, Stephen Boyd wrote:
> > 
> > You can grep for it:
> > 
> >  $ git grep -W 'struct clk_ops .*='
> 
> TIL about -W. It's awesome, thanks

:)

> 
> > but I'm fairly certain a coccinelle script can detect most of these
> > because clk_ops are usually statically defined (although not always).
> > 
> > Either way I already see that 'owl_comp_div_ops' will trigger this
> > warning. And 'at91sam9x5_smd_ops' looks even more likely. Given that I'm
> > not super keen on applying this patch.
> 
> It's the reason why I didn't return an error at first, I wanted to
> report that it's invalid and let to drivers the chance to be fixed
> still.
> 
> Should I take your above comment as you'd rather have the affected
> drivers fixed in this patch and we then return an error, or is it that
> you don't want that patch at all?

You can try fixing all the drivers that are failing to meet this
requirement (found with grep) and if they are fixed we can start
printing the warning. That seems to be the practical approach to getting
this patch accepted. The TODO irks me to be honest. I'd rather we fix
everything and make it an error and be done with it.

Re: [PATCH 1/4] clk: Remove WARN_ON NULL parent in clk_core_init_rate_req()

[Stephen Boyd]

Quoting Maxime Ripard (2022-10-18 06:52:56)
> If a clock has CLK_SET_RATE_PARENT, but core->parent is NULL (most
> likely because it's orphan), callers of clk_core_init_rate_req() will
> blindly call this function leading to a very verbose warning.
> 
> Since it's a fairly common situation, let's just remove the WARN_ON but
> keep the check that prevents us from dereferencing the pointer.
> 
> Interestingly, it fixes a regression on the Mediatek MT8195 where the
> GPU would stall during a clk_set_rate for its main clock. We couldn't
> come up with a proper explanation since the condition is essentially the
> same.
> 
> It was then assumed that it could be timing related since printing the
> warning stacktrace takes a while, but we couldn't replicate the failure
> by using fairly large (10ms) mdelays.
> 
> Fixes: 262ca38f4b6e ("clk: Stop forwarding clk_rate_requests to the parent")
> Reported-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Applied to clk-fixes

Re: [PATCH 2/4] clk: Initialize the clk_rate_request even if clk_core is NULL

[Stephen Boyd]

Quoting Maxime Ripard (2022-10-18 06:52:57)
> Since commit c35e84b09776 ("clk: Introduce clk_hw_init_rate_request()"),
> users that used to initialize their clk_rate_request by initializing
> their local structure now rely on clk_hw_init_rate_request().
> 
> This function is backed by clk_core_init_rate_req(), which will skip the
> initialization if either the pointer to struct clk_core or to struct
> clk_rate_request are NULL.
> 
> However, the core->parent pointer might be NULL because the clock is
> orphan, and we will thus end up with our local struct clk_rate_request
> left untouched.
> 
> And since clk_hw_init_rate_request() doesn't return an error, we will
> then call a determine_rate variant with that unitialized structure.
> 
> In order to avoid this, let's clear our clk_rate_request if the pointer
> to it is valid but the pointer to struct clk_core isn't.
> 
> Fixes: c35e84b09776 ("clk: Introduce clk_hw_init_rate_request()")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Applied to clk-fixes

Re: [PATCH 3/4] clk: Initialize max_rate in struct clk_rate_request

[Stephen Boyd]

Quoting Maxime Ripard (2022-10-18 06:52:58)
> Since commit b46fd8dbe8ad ("clk: Zero the clk_rate_request structure"),
> the clk_core_init_rate_req() function clears the struct clk_rate_request
> passed as argument.
> 
> However, the default value for max_rate isn't 0 but ULONG_MAX, and we
> end up creating a clk_rate_request instance where the maximum rate is 0.
> 
> Let's initialize max_rate to ULONG_MAX properly.
> 
> Fixes: b46fd8dbe8ad ("clk: Zero the clk_rate_request structure")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Applied to clk-fixes

Re: [PATCH 4/4] clk: Warn if we register a mux without determine_rate

[Maxime Ripard]

Going back to this mail.

On Tue, Oct 25, 2022 at 07:07:58PM -0700, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-10-18 06:52:59)
> > The determine_rate hook allows to select the proper parent and its rate
> > for a given clock configuration. On another hand, set_parent is there to
> > change the parent of a mux.
> > 
> > Some clocks provide a set_parent hook but don't implement
> > determine_rate. In such a case, set_parent is pretty much useless since
> > the clock framework will always assume the current parent is to be used,
> > and we will thus never change it.
> > 
> > This situation can be solved in two ways:
> >   - either we don't need to change the parent, and we thus shouldn't
> >     implement set_parent;
> >   - or we don't want to change the parent, in this case we should set
> >     CLK_SET_RATE_NO_REPARENT;
> >   - or we're missing a determine_rate implementation.
> > 
> > The latter is probably just an oversight from the driver's author, and
> > we should thus raise their awareness about the fact that the current
> > state of the driver is confusing.
> 
> There is another case which is a leaf clk that is a mux where you only
> expect clk_set_parent() to be used, and not clk_set_rate(). This use
> case is odd though, so I'm not sure how much we care.

It looks like there's a good number of clocks that do indeed only
provide get_parent / set_parent. It's hard to tell if it's an oversight
or a choice.

I think we can make that decision explicit by providing a determine_rate
helper that always returns the current parent and its rate. It shouldn't
change anything from a CCF behavior point of view, and it makes it clear
what the behavior is. And if someone wants something else, then they can
change it to whatever they want.

Maxime

Re: [PATCH 4/4] clk: Warn if we register a mux without determine_rate

[Maxime Ripard]

On Thu, Oct 27, 2022 at 02:45:07PM -0700, Stephen Boyd wrote:
> Quoting maxime@cerno.tech (2022-10-26 06:52:15)
> > On Tue, Oct 25, 2022 at 07:07:58PM -0700, Stephen Boyd wrote:
> > > but I'm fairly certain a coccinelle script can detect most of these
> > > because clk_ops are usually statically defined (although not always).
> > > 
> > > Either way I already see that 'owl_comp_div_ops' will trigger this
> > > warning. And 'at91sam9x5_smd_ops' looks even more likely. Given that I'm
> > > not super keen on applying this patch.
> > 
> > It's the reason why I didn't return an error at first, I wanted to
> > report that it's invalid and let to drivers the chance to be fixed
> > still.
> > 
> > Should I take your above comment as you'd rather have the affected
> > drivers fixed in this patch and we then return an error, or is it that
> > you don't want that patch at all?
> 
> You can try fixing all the drivers that are failing to meet this
> requirement (found with grep) and if they are fixed we can start
> printing the warning. That seems to be the practical approach to
> getting this patch accepted. The TODO irks me to be honest. I'd rather
> we fix everything and make it an error and be done with it.

ACK. I had a look this morning and there's indeed a good number of
clocks in that case. I'll work on it.

Maxime

Re: [PATCH 4/4] clk: Warn if we register a mux without determine_rate

[Stephen Boyd]

Quoting Maxime Ripard (2022-11-03 05:33:28)
> On Tue, Oct 25, 2022 at 07:07:58PM -0700, Stephen Boyd wrote:
> > There is another case which is a leaf clk that is a mux where you only
> > expect clk_set_parent() to be used, and not clk_set_rate(). This use
> > case is odd though, so I'm not sure how much we care.
> 
> It looks like there's a good number of clocks that do indeed only
> provide get_parent / set_parent. It's hard to tell if it's an oversight
> or a choice.
> 
> I think we can make that decision explicit by providing a determine_rate
> helper that always returns the current parent and its rate. It shouldn't
> change anything from a CCF behavior point of view, and it makes it clear
> what the behavior is. And if someone wants something else, then they can
> change it to whatever they want.

Ok sounds like a plan.