Re: [PATCH v2 2/2] clk: tegra: support BPMP-FW ABI deny flags

[Thierry Reding <treding@nvidia.com>]

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

On Tue, Oct 25, 2022 at 12:35:36PM +0300, Peter De Schrijver wrote:
> Support BPMP_CLK_STATE_CHANGE_DENIED by not populating state changing
> operations when the flag is set.
> 
> Support BPMP_CLK_RATE_PARENT_CHANGE_DENIED by not populating rate or
> parent  changing operations when the flag is set.
> 
> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
> ---
>  drivers/clk/tegra/clk-bpmp.c | 37 +++++++++++++++++++++++++++++++++---
>  1 file changed, 34 insertions(+), 3 deletions(-)

This is missing a changelog. From a quick look it seems the only change
is the fix for the dev_warn() issue pointed out by the kernel test
robot.

> 
> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
> index d82a71f10c2c..c912c5f0d1eb 100644
> --- a/drivers/clk/tegra/clk-bpmp.c
> +++ b/drivers/clk/tegra/clk-bpmp.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Copyright (C) 2016-2020 NVIDIA Corporation
> + * Copyright (C) 2016-2022 NVIDIA Corporation
>   */
>  
>  #include <linux/clk-provider.h>
> @@ -310,6 +310,23 @@ static const struct clk_ops tegra_bpmp_clk_mux_rate_ops = {
>  	.set_rate = tegra_bpmp_clk_set_rate,
>  };
>  
> +static const struct clk_ops tegra_bpmp_clk_mux_read_only_ops = {
> +	.get_parent = tegra_bpmp_clk_get_parent,
> +	.recalc_rate = tegra_bpmp_clk_recalc_rate,
> +};
> +
> +static const struct clk_ops tegra_bpmp_clk_read_only_ops = {
> +	.recalc_rate = tegra_bpmp_clk_recalc_rate,
> +};
> +
> +static const struct clk_ops tegra_bpmp_clk_gate_mux_read_only_ops = {
> +	.prepare = tegra_bpmp_clk_prepare,
> +	.unprepare = tegra_bpmp_clk_unprepare,
> +	.is_prepared = tegra_bpmp_clk_is_prepared,
> +	.recalc_rate = tegra_bpmp_clk_recalc_rate,
> +	.get_parent = tegra_bpmp_clk_get_parent,
> +};
> +
>  static int tegra_bpmp_clk_get_max_id(struct tegra_bpmp *bpmp)
>  {
>  	struct cmd_clk_get_max_clk_id_response response;
> @@ -510,8 +527,22 @@ tegra_bpmp_clk_register(struct tegra_bpmp *bpmp,
>  	memset(&init, 0, sizeof(init));
>  	init.name = info->name;
>  	clk->hw.init = &init;
> -
> -	if (info->flags & TEGRA_BPMP_CLK_HAS_MUX) {
> +	if (info->flags & BPMP_CLK_STATE_CHANGE_DENIED) {
> +		if ((info->flags & BPMP_CLK_RATE_PARENT_CHANGE_DENIED) == 0) {
> +			dev_warn(bpmp->dev,
> +				"clock %s does not allow state change but does allow rate/parent change",
> +				 init.name);

It's not clear to me from the warning what exactly this means. Does it
mean the clock won't work? Does it mean that parent changes are not
allowed either even though the flag says otherwise.

Looking at the code, the latter is the case, but for users not looking
at the code this may be confusing. Originally this was in the format of
a WARN, though I think you had used dev_warn() which then caused the
robot to flag this. I think you could use dev_WARN() instead which may
be what you had intended. That would make it clearer that this is
something for developers to look at, rather than some sort of issue that
users would need to deal with.

My understanding is that the WARN splat would only occur if the BPMP
firmware was somehow faulty (because STATE_CHANGE_DENIED and
!RATE_PARENT_CHANGE_DENIED is not a valid combination), so that seems
more appropriate than dev_warn().

Thierry

> +		}
> +		if (info->flags & TEGRA_BPMP_CLK_HAS_MUX)
> +			init.ops = &tegra_bpmp_clk_mux_read_only_ops;
> +		else
> +			init.ops = &tegra_bpmp_clk_read_only_ops;
> +	} else if (info->flags & BPMP_CLK_RATE_PARENT_CHANGE_DENIED) {
> +		if (info->flags & TEGRA_BPMP_CLK_HAS_MUX)
> +			init.ops = &tegra_bpmp_clk_gate_mux_read_only_ops;
> +		else
> +			init.ops = &tegra_bpmp_clk_gate_ops;
> +	} else if (info->flags & TEGRA_BPMP_CLK_HAS_MUX) {
>  		if (info->flags & TEGRA_BPMP_CLK_HAS_SET_RATE)
>  			init.ops = &tegra_bpmp_clk_mux_rate_ops;
>  		else
> -- 
> 2.34.1
> 

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