| Message ID | 87wmulrynq.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers: clk: renesas: enable all clocks which is assinged to non Linux system | expand |
Quoting Geert Uytterhoeven (2023-11-16 13:08:46) > On Thu, Nov 16, 2023 at 8:23 PM Rob Herring <robh@kernel.org> wrote: > > On Tue, Nov 14, 2023 at 12:01:14AM +0000, Kuninori Morimoto wrote: > > > 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, renesas-cpg-mssr try to find > > > status = "reserved" devices (A), and add CLK_IGNORE_UNUSED flag to its > > > <&cgp CPG_MOD xxx> clock (B). > > > > See Stephen's presentation from Plumbers this week. The default behavior > > for unused clocks may be changing soon. > > Thank you! > > ou mean "Make sync_state()/handoff work for the common clk > framework"[1]? IIUIC, that presentation didn't cover the problem we are > facing, except for the big "Kconfig for clk_ignore_unused=true" hammer. :) > > > > 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"; > > > }; > > > > I have some reservations about whether a reserved node should be touched > > at all by Linux. I suppose since it is platform specific, it's okay. I > > don't think we could apply such behavior globally. > > That's an interesting comment, as the issue is that currently Linux > does touch (resources belonging to) reserved nodes, and this patch > would prevent doing that for module clock resources;-) I think I get it. > > The core issue is that Linux distinguishes only between two cases: > 1. "device is used by Linux" (if a driver is available), > as indicated by 'status = "okay"' in DT, or > 2. "device is unused by Linux". > On a heterogenous system, the latter actually comprises two cases: > 2a. "device is unused", or > 2b. "device is used by another OS running on another CPU core". > > Looking for 'status = "reserved"' allows us to distinguish between 2a > and 2b, and can prevent disabling clocks that are used by another OS. > Probably we need a similar solution for power domains. > > Do you have a better or alternative suggestion? Does the protected-clocks property work? That basically says "don't use these clks in the OS". The driver implementation would not register those clks and then the framework would be unaware of their existence, leading to them never being disabled during late init. This approach also looks OK to me, basically programmatically creating the protected-clocks list by parsing DT for reserved consumer nodes and then figuring out that no consumer exists so we can skip registering the clk entirely, or add the flag. I'm not sure we want to implement that policy globally, because maybe someone really wants to disable the clk still to clean up bootloader state and then let a remoteproc use the clk later. Do you want to keep those clks registered with the framework? Is there any benefit to keeping clks around if linux can't do anything with them?
Hi Stephen Thank you for your feedback > Does the protected-clocks property work? That basically says "don't use > these clks in the OS". The driver implementation would not register > those clks and then the framework would be unaware of their existence, > leading to them never being disabled during late init. > > This approach also looks OK to me, basically programmatically creating > the protected-clocks list by parsing DT for reserved consumer nodes and > then figuring out that no consumer exists so we can skip registering the > clk entirely, or add the flag. I'm not sure we want to implement that > policy globally, because maybe someone really wants to disable the clk > still to clean up bootloader state and then let a remoteproc use the clk > later. > > Do you want to keep those clks registered with the framework? Is there > any benefit to keeping clks around if linux can't do anything with them? Actually, this idea (= using "protected-clocks" property style) was our 1st idea, but I noticed that it is not enough. Because clock driver is possible to know which module was not used on Linux, but other driver can't, in this idea. For example, power-domain control driver might want to know about it. In case of using "protected-clocks" property, we need to have same/similar settings on each driver, but in case of using "status = reserved", all driver is possible to know it. It has consistent, and no contradict like some module was listed as "protected-clocks" on clock / power driver, but has "status = ok" on its driver, etc. It seems we have different opinions around here ? Our other idea was having "unassigned" node instead of "status = reserved", like below. clock driver checks "unassigned" node's "devices" property, and ignore/disable listed device's "clocks". unassigned { devices = <&scif1>, <&hsusb>; }; scif1: serial@xxxx { status = "disabled"; clocks = <&cpg CPG_MOD 206>, <&cpg CPG_CORE R8A7795_CLK_S3D1>, <&scif_clk>; ... }; hsusb: usb@xxxx { status = "disabled"; clocks = <&cpg CPG_MOD 704>, <&cpg CPG_MOD 703>; ... }; Thank you for your help !! Best regards --- Kuninori Morimoto
Quoting Kuninori Morimoto (2023-11-27 23:48:04) > > Hi Stephen > > Thank you for your feedback > > > Does the protected-clocks property work? That basically says "don't use > > these clks in the OS". The driver implementation would not register > > those clks and then the framework would be unaware of their existence, > > leading to them never being disabled during late init. > > > > This approach also looks OK to me, basically programmatically creating > > the protected-clocks list by parsing DT for reserved consumer nodes and > > then figuring out that no consumer exists so we can skip registering the > > clk entirely, or add the flag. I'm not sure we want to implement that > > policy globally, because maybe someone really wants to disable the clk > > still to clean up bootloader state and then let a remoteproc use the clk > > later. > > > > Do you want to keep those clks registered with the framework? Is there > > any benefit to keeping clks around if linux can't do anything with them? > > Actually, this idea (= using "protected-clocks" property style) was our 1st > idea, but I noticed that it is not enough. Because clock driver is possible > to know which module was not used on Linux, but other driver can't, in this > idea. For example, power-domain control driver might want to know about it. > > In case of using "protected-clocks" property, we need to have same/similar > settings on each driver, but in case of using "status = reserved", all > driver is possible to know it. It has consistent, and no contradict like > some module was listed as "protected-clocks" on clock / power driver, > but has "status = ok" on its driver, etc. > > It seems we have different opinions around here ? I don't really have any opinion here. > > Our other idea was having "unassigned" node instead of > "status = reserved", like below. > clock driver checks "unassigned" node's "devices" property, and > ignore/disable listed device's "clocks". > > unassigned { > devices = <&scif1>, <&hsusb>; > }; This approach looks like the chosen node. I'd say what you have done in this patch series is better. The protected-clocks property is really about clks that shouldn't be used by the OS on some board. In your case it sounds like you want to still be able to read the clk hardware? Does anything go wrong if you let some consumer get the clk and change settings? Do you want to prevent that from happening? I'm mostly thinking it may be useful to have this logic be common in the clk framework by having the framework search through DT and figure out that the only consumers are reserved and thus we should treat those clks as read-only in the framework.
Hi Stephen Thank you for your feedback > In your case > it sounds like you want to still be able to read the clk hardware? Does > anything go wrong if you let some consumer get the clk and change > settings? Do you want to prevent that from happening? We want to do by this patch-set is to ignoring specific device/clk from Linux. If Linux side change the specific clk settings, other OS side will get damage. > I'm mostly > thinking it may be useful to have this logic be common in the clk > framework by having the framework search through DT and figure out that > the only consumers are reserved and thus we should treat those clks as > read-only in the framework. "this logic" = "this patch-set idea" = "check status=reserved" ? If so, now I quick checked the code. I think it is difficult or makes the code complex if we try implement it on framework or common code. Because the idea itself is very easy (= just adding the CLK_IGNORE_UNUSED), but how to judge the clks is very vender/driver specific. In our case, we need to think about total clock number, conver Linux number to HW number, etc, etc. # I will goto OSS-Japan conference after tomorrow, thus no response # from me until next week. Thank you for your help !! Best regards --- Renesas Electronics Ph.D. Kuninori Morimoto
diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c index cb80d1bf6c7c..3781861bdfa0 100644 --- a/drivers/clk/renesas/renesas-cpg-mssr.c +++ b/drivers/clk/renesas/renesas-cpg-mssr.c @@ -168,6 +168,9 @@ struct cpg_mssr_priv { u32 val; } smstpcr_saved[ARRAY_SIZE(mstpsr_for_gen4)]; + unsigned int *reserved_ids; + unsigned int num_reserved_ids; + struct clk *clks[]; }; @@ -453,6 +456,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 +965,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"); + struct device_node *node = NULL; + 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 + * non-Linux system will be disabled when Linux was booted. + * + * 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 + * to its clocks if it was CPG_MOD. + * 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)) { + + of_node_put(clkspec.np); + + if (clkspec.np == priv->dev->of_node && + clkspec.args[0] == CPG_MOD) { + + 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]); + + num++; + } + } + } + + priv->num_reserved_ids = num; + priv->reserved_ids = ids; + + return 0; +} + +static void __init cpg_mssr_common_exit(struct cpg_mssr_priv *priv) +{ + if (priv->base) + iounmap(priv->base); + kfree(priv); +} + static int __init cpg_mssr_common_init(struct device *dev, struct device_node *np, const struct cpg_mssr_info *info) @@ -1012,9 +1097,7 @@ static int __init cpg_mssr_common_init(struct device *dev, return 0; out_err: - if (priv->base) - iounmap(priv->base); - kfree(priv); + cpg_mssr_common_exit(priv); return error; } @@ -1029,6 +1112,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; + 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 +1124,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); + + return; + +err: + cpg_mssr_common_exit(cpg_mssr_priv); } static int __init cpg_mssr_probe(struct platform_device *pdev) @@ -1060,6 +1153,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; + for (i = 0; i < info->num_core_clks; i++) cpg_mssr_register_core_clk(&info->core_clks[i], info, priv); @@ -1070,22 +1167,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: + cpg_mssr_reserved_exit(priv); + + return error; } static struct platform_driver cpg_mssr_driver = {
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, renesas-cpg-mssr try to find status = "reserved" devices (A), and add 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> --- drivers/clk/renesas/renesas-cpg-mssr.c | 116 +++++++++++++++++++++++-- 1 file changed, 107 insertions(+), 9 deletions(-)