| Message ID | 87wmtlo2zs.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| Headers | show |
| Series | drivers: clk: renesas: ignore all clocks which are assinged to non-Linux system | expand |
Hi Morimoto-san, Thanks for the update! s/assinged/assigned/ On Mon, Dec 11, 2023 at 4:03 AM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > Some boards might use Linux and another OS at the same time. In such > case, 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 tries to find > status = "reserved" devices (A), and adds CLK_IGNORE_UNUSED flag to its > <&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> > @@ -949,6 +967,72 @@ 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); > +} This function is called only once, so you might want to inline it manually. > + > +static int __init cpg_mssr_reserved_init(struct cpg_mssr_priv *priv, > + const struct cpg_mssr_info *info) > +{ > + struct device_node *soc = of_find_node_by_path("/soc"); > + struct device_node *node; > + uint32_t args[MAX_PHANDLE_ARGS]; > + unsigned int *ids = NULL; > + unsigned int num = 0; > + > + /* > + * Because clk_disable_unused() will disable all unused clocks, the device which is assigned > + * to a non-Linux system will be disabled when Linux is booted. > + * > + * To avoid such situation, renesas-cpg-mssr assumes the device which has > + * status = "reserved" is assigned to a non-Linux system, and adds CLK_IGNORE_UNUSED flag > + * 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(soc, node) { > + struct of_phandle_iterator it; > + int rc; > + > + of_for_each_phandle(&it, rc, node, "clocks", "#clock-cells", -1) { > + int idx; > + > + of_phandle_iterator_args(&it, args, MAX_PHANDLE_ARGS); > + > + if (!(it.node == priv->np && args[0] == CPG_MOD)) I think "(it.node != priv->np || args[0] != CPG_MOD)" is easier to read ;-) However, I think it would make sense to split this in two separate checks, to avoid calling of_phandle_iterator_args() when it.node != priv->np, and to validate the number of arguments: if (it.node != priv->np) continue; if (of_phandle_iterator_args(&it, args, MAX_PHANDLE_ARGS) != 2) continue; if (args[0] != CPG_MOD) continue; > + continue; > + > + ids = krealloc_array(ids, (num + 1), sizeof(*ids), GFP_KERNEL); > + if (!ids) > + return -ENOMEM; Missing of_node_put(it.node) in the error path. > + > + if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A) > + idx = MOD_CLK_PACK_10(args[1]); /* for DEF_MOD_STB() */ > + else > + idx = MOD_CLK_PACK(args[1]); /* for DEF_MOD() */ > + > + ids[num] = info->num_total_core_clks + idx; > + > + num++; > + } > + } > + > + priv->num_reserved_ids = num; > + priv->reserved_ids = ids; > + > + return 0; > +} > + > static int __init cpg_mssr_common_init(struct device *dev, > struct device_node *np, > const struct cpg_mssr_info *info) > @@ -1007,6 +1091,10 @@ static int __init cpg_mssr_common_init(struct device *dev, > if (error) > goto out_err; > > + error = cpg_mssr_reserved_init(priv, info); > + if (error) > + goto out_err; Missing of_clk_del_provider() in the error path. You may want to move the call to cpg_mssr_reserved_init() up, as reverting that just needs an unconditional call to kfree() (kfree works fine on NULL), while calling of_clk_del_provider() requires a new label to jump to. > + > cpg_mssr_priv = priv; > > return 0; > @@ -1070,22 +1158,23 @@ static int __init cpg_mssr_probe(struct platform_device *pdev) > cpg_mssr_del_clk_provider, > np); > if (error) > - return error; > + goto reserve_err; > > error = cpg_mssr_add_clk_domain(dev, info->core_pm_clks, > info->num_core_pm_clks); > if (error) > - return error; > + goto reserve_err; > > /* Reset Controller not supported for Standby Control SoCs */ > if (priv->reg_layout == CLK_REG_LAYOUT_RZ_A) > - return 0; > + goto reserve_err; > > error = cpg_mssr_reset_controller_register(priv); > - if (error) > - return error; > > - return 0; > +reserve_err: Perhaps rename the label to "reserve_exit", as this is called on success, too? > + cpg_mssr_reserved_exit(priv); > + > + return error; > } > > static struct platform_driver cpg_mssr_driver = { Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert Thank you for your reveiw > > +static void __init cpg_mssr_reserved_exit(struct cpg_mssr_priv *priv) > > +{ > > + kfree(priv->reserved_ids); > > +} > > This function is called only once, so you might want to inline it manually. I want to keep init()/exit() pair because it is easy to understand. I will consider other comment and post v5 patch next week Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto