mbox series

[v3,0/4] drivers: clk: renesas: ignore all clocks which is assinged to non Linux system

Message ID 87v89k0yyj.wl-kuninori.morimoto.gx@renesas.com
Headers show
Series drivers: clk: renesas: ignore all clocks which is assinged to non Linux system | expand

Message

Kuninori Morimoto Nov. 30, 2023, 2:14 a.m. UTC
Hi Rob, Geert
Cc Aymeric, Goda-san

This is v3 of ignoring non Linux system assinged device.

Some board might use Linux and another OS in the same time. In such
case, current Linux will stop necessary module clock when booting
which is not used on Linux side, but is used on another OS side.

To avoid such situation, this patch-set try to find status = "reserved"
devices, and add CLK_IGNORE_UNUSED flag to its clock.

Table 2.4: Values for status property
https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf

"reserved"
	Indicates that the device is operational, but should not be
	used. Typically this is used for devices that are controlled
	by another software component, such as platform firmware.

[1/4] - [3/4] : expand existing function for "reserved"
[4/4]         : adjust to "reserved" device on Renesas CPG

v2 -> v3
	- "__of_get_next_status_child()" -> "of_get_next_status_child()"
	- add Reviewed-by from Rob

v1 -> v2
	- remove "default_ret" from __of_device_is_status()
	- add new parameter explanation on cpg_mssr_priv

Kuninori Morimoto (4):
  of: add __of_device_is_status() and makes more generic status check
  of: add of_get_next_status_child() and makes more generic of_get_next
  of: add for_each_reserved_child_of_node()
  drivers: clk: renesas: ignore all clocks which is assinged to non Linux system

 drivers/clk/renesas/renesas-cpg-mssr.c | 118 +++++++++++++++++++++++--
 drivers/of/base.c                      | 111 ++++++++++++++++-------
 include/linux/of.h                     |  11 +++
 3 files changed, 201 insertions(+), 39 deletions(-)

Comments

Geert Uytterhoeven Dec. 5, 2023, 10:55 a.m. UTC | #1
Hi Morimoto-san,

Thanks for the update!

s/is assigned/are assigned/
s/non Linux/non-Linux/

On Thu, Nov 30, 2023 at 3:15 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> Some board might use Linux and another OS in the same time. In such

boards ... at the same

> case, current Linux will stop necessary module clock when booting
> which is not used on Linux side, but is used on another OS side.

... currently, during booting, Linux will stop necessary module clocks
which are not used on the Linux side, but are used by another OS.

> To avoid such situation, renesas-cpg-mssr try to find

tries

> status = "reserved" devices (A), and add CLK_IGNORE_UNUSED flag to its

adds

> <&cgp CPG_MOD xxx> clock (B).
>
> Table 2.4: Values for status property
> https://github.com/devicetree-org/devicetree-specification/releases/download/v0.4/devicetree-specification-v0.4.pdf
>
> "reserved"
>         Indicates that the device is operational, but should not be
>         used. Typically this is used for devices that are controlled
>         by another software component, such as platform firmware.
>
> ex)
>         scif5: serial@e6f30000 {
>                 ...
> (B)             clocks = <&cpg CPG_MOD 202>,
>                          <&cpg CPG_CORE R8A7795_CLK_S3D1>,
>                          <&scif_clk>;
>                 ...
> (A)             status = "reserved";
>         };
>
> Cc: Aymeric Aillet <aymeric.aillet@iot.bzh>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Tested-by: Yusuke Goda <yusuke.goda.sx@renesas.com>

> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -453,6 +458,19 @@ static void __init
cpg_mssr_register_mod_clk(const struct mssr_mod_clk *mod,
>                         break;
>                 }
>
> +       /*
> +        * Ignore reserved device.
> +        * see
> +        *      cpg_mssr_reserved_init()
> +        */
> +       for (i = 0; i < priv->num_reserved_ids; i++) {
> +               if (id == priv->reserved_ids[i]) {
> +                       dev_info(dev, "Ignore Linux non-assigned mod (%s)\n", mod->name);
> +                       init.flags |= CLK_IGNORE_UNUSED;
> +                       break;
> +               }
> +       }
> +
>         clk = clk_register(NULL, &clock->hw);
>         if (IS_ERR(clk))
>                 goto fail;
> @@ -949,6 +967,75 @@ static const struct dev_pm_ops cpg_mssr_pm = {
>  #define DEV_PM_OPS     NULL
>  #endif /* CONFIG_PM_SLEEP && CONFIG_ARM_PSCI_FW */
>
> +static void __init cpg_mssr_reserved_exit(struct cpg_mssr_priv *priv)
> +{
> +       kfree(priv->reserved_ids);
> +}
> +
> +static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv,
> +                                        const struct cpg_mssr_info *info)
> +{
> +       struct device_node *root = of_find_node_by_path("/soc");

s/root/soc/

> +       struct device_node *node = NULL;

No need to initialize node.

> +       struct of_phandle_args clkspec;
> +       unsigned int *ids = NULL;
> +       unsigned int num = 0;
> +
> +       /*
> +        * Because cpg_mssr_info has .num_hw_mod_clks which indicates number of all Module Clocks,
> +        * and clk_disable_unused() will disable all unused clocks, the device which is assigned to

"Because clk_disable_unused() will disable all unused clocks, ..."?

> +        * non-Linux system will be disabled when Linux was booted.

a non-Linux
s/was/is/

> +        *
> +        * To avoid such situation, renesas-cpg-mssr assumes the device which has
> +        * status = "reserved" is assigned to non-Linux system, and add CLK_IGNORE_UNUSED flag

a non-Linux ... adds

> +        * to its clocks if it was CPG_MOD.

to its CPG_MOD clocks

> +        * see also
> +        *      cpg_mssr_register_mod_clk()
> +        *
> +        *      scif5: serial@e6f30000 {
> +        *              ...
> +        * =>           clocks = <&cpg CPG_MOD 202>,
> +        *                       <&cpg CPG_CORE R8A7795_CLK_S3D1>,
> +        *                       <&scif_clk>;
> +        *                       ...
> +        *               status = "reserved";
> +        *      };
> +        */
> +       for_each_reserved_child_of_node(root, node) {
> +               unsigned int i = 0;
> +
> +               while (!of_parse_phandle_with_args(node, "clocks", "#clock-cells", i++, &clkspec)) {

Looks like you missed Rob's comment on v2 to use of_for_each_phandle() instead?

> +

Please drop this blank line.

> +                       of_node_put(clkspec.np);
> +
> +                       if (clkspec.np == priv->dev->of_node &&
> +                           clkspec.args[0] == CPG_MOD) {
> +

Please drop this blank line.

> +                               ids = krealloc_array(ids, (num + 1), sizeof(*ids), GFP_KERNEL);
> +                               if (!ids)
> +                                       return -ENOMEM;
> +
> +                               ids[num] = info->num_total_core_clks +
> +                                               MOD_CLK_PACK(clkspec.args[1]);

This should use MOD_CLK_PACK_10() in case priv->reg_layout ==
CLK_REG_LAYOUT_RZ_A.

> +
> +                               num++;
> +                       }
> +               }
> +       }
> +
> +       priv->num_reserved_ids  = num;
> +       priv->reserved_ids      = ids;
> +
> +       return 0;
> +}

> @@ -1029,6 +1114,10 @@ void __init cpg_mssr_early_init(struct device_node *np,
>         if (error)
>                 return;
>
> +       error = cpg_mssr_reserved_init(cpg_mssr_priv, info);
> +       if (error)
> +               goto err;

Please move this into cpg_mssr_common_init(), which is called above.
After that, there is no longer a need to factor out
cpg_mssr_common_exit().

> +
>         for (i = 0; i < info->num_early_core_clks; i++)
>                 cpg_mssr_register_core_clk(&info->early_core_clks[i], info,
>                                            cpg_mssr_priv);
> @@ -1037,6 +1126,12 @@ void __init cpg_mssr_early_init(struct device_node *np,
>                 cpg_mssr_register_mod_clk(&info->early_mod_clks[i], info,
>                                           cpg_mssr_priv);
>
> +       cpg_mssr_reserved_exit(cpg_mssr_priv);

Please drop this, as you still need this data when cpg_mssr_probe()
is called later (yes, when a driver uses cpg_mssr_early_init(),
cpg_mssr_probe() is still called later to register the non-early
clocks).

> +
> +       return;
> +
> +err:
> +       cpg_mssr_common_exit(cpg_mssr_priv);
>  }
>
>  static int __init cpg_mssr_probe(struct platform_device *pdev)
> @@ -1060,6 +1155,10 @@ static int __init cpg_mssr_probe(struct platform_device *pdev)
>         priv->dev = dev;
>         dev_set_drvdata(dev, priv);
>
> +       error = cpg_mssr_reserved_init(priv, info);
> +       if (error)
> +               return error;

When a driver uses cpg_mssr_early_init(), this causes the reserved
clock list to be created a second time.  Moving this handling into
cpg_mssr_common_init() would fix that.

> +
>         for (i = 0; i < info->num_core_clks; i++)
>                 cpg_mssr_register_core_clk(&info->core_clks[i], info, priv);
>

Gr{oetje,eeting}s,

                        Geert