| Message ID | 20230223041938.22732-6-semen.protsenko@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | clk: samsung: Add PM support for ARM64 Exynos chips | expand |
On 23/02/2023 05:19, Sam Protsenko wrote: > Extract parent clock enabling from exynos_arm64_register_cmu() to > dedicated function. > > Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > Changes in v3: > - Rebased on top of latest soc/for-next tree > - Added Marek's Acked-by tag > > Changes in v2: > - Rebased on top of latest soc/for-next tree > - Improved English in kernel doc comment > - Added clk_prepare_enable() return value check > - Added exynos_arm64_enable_bus_clk() check in > exynos_arm64_register_cmu() > - Changed the commit message to reflect code changes > > drivers/clk/samsung/clk-exynos-arm64.c | 51 ++++++++++++++++++-------- > 1 file changed, 35 insertions(+), 16 deletions(-) > > diff --git a/drivers/clk/samsung/clk-exynos-arm64.c b/drivers/clk/samsung/clk-exynos-arm64.c > index b921b9a1134a..2aa3f0a5644e 100644 > --- a/drivers/clk/samsung/clk-exynos-arm64.c > +++ b/drivers/clk/samsung/clk-exynos-arm64.c > @@ -56,6 +56,37 @@ static void __init exynos_arm64_init_clocks(struct device_node *np, > iounmap(reg_base); > } > > +/** > + * exynos_arm64_enable_bus_clk - Enable parent clock of specified CMU > + * > + * @dev: Device object; may be NULL if this function is not being > + * called from platform driver probe function > + * @np: CMU device tree node > + * @cmu: CMU data > + * > + * Keep CMU parent clock running (needed for CMU registers access). > + * > + * Return: 0 on success or a negative error code on failure. > + */ > +static int __init exynos_arm64_enable_bus_clk(struct device *dev, > + struct device_node *np, const struct samsung_cmu_info *cmu) > +{ > + struct clk *parent_clk; > + > + if (!cmu->clk_name) > + return 0; > + > + if (dev) > + parent_clk = clk_get(dev, cmu->clk_name); > + else > + parent_clk = of_clk_get_by_name(np, cmu->clk_name); > + > + if (IS_ERR(parent_clk)) > + return PTR_ERR(parent_clk); > + > + return clk_prepare_enable(parent_clk); > +} > + > /** > * exynos_arm64_register_cmu - Register specified Exynos CMU domain > * @dev: Device object; may be NULL if this function is not being > @@ -72,23 +103,11 @@ static void __init exynos_arm64_init_clocks(struct device_node *np, > void __init exynos_arm64_register_cmu(struct device *dev, > struct device_node *np, const struct samsung_cmu_info *cmu) > { > - /* Keep CMU parent clock running (needed for CMU registers access) */ > - if (cmu->clk_name) { > - struct clk *parent_clk; > - > - if (dev) > - parent_clk = clk_get(dev, cmu->clk_name); > - else > - parent_clk = of_clk_get_by_name(np, cmu->clk_name); > - > - if (IS_ERR(parent_clk)) { > - pr_err("%s: could not find bus clock %s; err = %ld\n", > - __func__, cmu->clk_name, PTR_ERR(parent_clk)); > - } else { > - clk_prepare_enable(parent_clk); > - } > - } > + int err; > > + err = exynos_arm64_enable_bus_clk(dev, np, cmu); > + if (err) > + panic("%s: could not enable bus clock\n", __func__); The error handling is changed and not equivalent. I would say that we could still try to boot even if this failed, so kernel should not panic. Maybe the parent clock is enabled by bootloader. Best regards, Krzysztof
On Mon, 6 Mar 2023 at 08:35, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 23/02/2023 05:19, Sam Protsenko wrote: > > Extract parent clock enabling from exynos_arm64_register_cmu() to > > dedicated function. > > > > Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > Changes in v3: > > - Rebased on top of latest soc/for-next tree > > - Added Marek's Acked-by tag > > > > Changes in v2: > > - Rebased on top of latest soc/for-next tree > > - Improved English in kernel doc comment > > - Added clk_prepare_enable() return value check > > - Added exynos_arm64_enable_bus_clk() check in > > exynos_arm64_register_cmu() > > - Changed the commit message to reflect code changes > > > > drivers/clk/samsung/clk-exynos-arm64.c | 51 ++++++++++++++++++-------- > > 1 file changed, 35 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/clk/samsung/clk-exynos-arm64.c b/drivers/clk/samsung/clk-exynos-arm64.c > > index b921b9a1134a..2aa3f0a5644e 100644 > > --- a/drivers/clk/samsung/clk-exynos-arm64.c > > +++ b/drivers/clk/samsung/clk-exynos-arm64.c > > @@ -56,6 +56,37 @@ static void __init exynos_arm64_init_clocks(struct device_node *np, > > iounmap(reg_base); > > } > > > > +/** > > + * exynos_arm64_enable_bus_clk - Enable parent clock of specified CMU > > + * > > + * @dev: Device object; may be NULL if this function is not being > > + * called from platform driver probe function > > + * @np: CMU device tree node > > + * @cmu: CMU data > > + * > > + * Keep CMU parent clock running (needed for CMU registers access). > > + * > > + * Return: 0 on success or a negative error code on failure. > > + */ > > +static int __init exynos_arm64_enable_bus_clk(struct device *dev, > > + struct device_node *np, const struct samsung_cmu_info *cmu) > > +{ > > + struct clk *parent_clk; > > + > > + if (!cmu->clk_name) > > + return 0; > > + > > + if (dev) > > + parent_clk = clk_get(dev, cmu->clk_name); > > + else > > + parent_clk = of_clk_get_by_name(np, cmu->clk_name); > > + > > + if (IS_ERR(parent_clk)) > > + return PTR_ERR(parent_clk); > > + > > + return clk_prepare_enable(parent_clk); > > +} > > + > > /** > > * exynos_arm64_register_cmu - Register specified Exynos CMU domain > > * @dev: Device object; may be NULL if this function is not being > > @@ -72,23 +103,11 @@ static void __init exynos_arm64_init_clocks(struct device_node *np, > > void __init exynos_arm64_register_cmu(struct device *dev, > > struct device_node *np, const struct samsung_cmu_info *cmu) > > { > > - /* Keep CMU parent clock running (needed for CMU registers access) */ > > - if (cmu->clk_name) { > > - struct clk *parent_clk; > > - > > - if (dev) > > - parent_clk = clk_get(dev, cmu->clk_name); > > - else > > - parent_clk = of_clk_get_by_name(np, cmu->clk_name); > > - > > - if (IS_ERR(parent_clk)) { > > - pr_err("%s: could not find bus clock %s; err = %ld\n", > > - __func__, cmu->clk_name, PTR_ERR(parent_clk)); > > - } else { > > - clk_prepare_enable(parent_clk); > > - } > > - } > > + int err; > > > > + err = exynos_arm64_enable_bus_clk(dev, np, cmu); > > + if (err) > > + panic("%s: could not enable bus clock\n", __func__); > > The error handling is changed and not equivalent. I would say that we > could still try to boot even if this failed, so kernel should not panic. > Maybe the parent clock is enabled by bootloader. > Agreed, I've probably overlooked that one when making all the refactoring. The same stands for the patch #6. Will rework and send out those two separately soon, as the rest of patches you already applied. Thanks! > Best regards, > Krzysztof >
diff --git a/drivers/clk/samsung/clk-exynos-arm64.c b/drivers/clk/samsung/clk-exynos-arm64.c index b921b9a1134a..2aa3f0a5644e 100644 --- a/drivers/clk/samsung/clk-exynos-arm64.c +++ b/drivers/clk/samsung/clk-exynos-arm64.c @@ -56,6 +56,37 @@ static void __init exynos_arm64_init_clocks(struct device_node *np, iounmap(reg_base); } +/** + * exynos_arm64_enable_bus_clk - Enable parent clock of specified CMU + * + * @dev: Device object; may be NULL if this function is not being + * called from platform driver probe function + * @np: CMU device tree node + * @cmu: CMU data + * + * Keep CMU parent clock running (needed for CMU registers access). + * + * Return: 0 on success or a negative error code on failure. + */ +static int __init exynos_arm64_enable_bus_clk(struct device *dev, + struct device_node *np, const struct samsung_cmu_info *cmu) +{ + struct clk *parent_clk; + + if (!cmu->clk_name) + return 0; + + if (dev) + parent_clk = clk_get(dev, cmu->clk_name); + else + parent_clk = of_clk_get_by_name(np, cmu->clk_name); + + if (IS_ERR(parent_clk)) + return PTR_ERR(parent_clk); + + return clk_prepare_enable(parent_clk); +} + /** * exynos_arm64_register_cmu - Register specified Exynos CMU domain * @dev: Device object; may be NULL if this function is not being @@ -72,23 +103,11 @@ static void __init exynos_arm64_init_clocks(struct device_node *np, void __init exynos_arm64_register_cmu(struct device *dev, struct device_node *np, const struct samsung_cmu_info *cmu) { - /* Keep CMU parent clock running (needed for CMU registers access) */ - if (cmu->clk_name) { - struct clk *parent_clk; - - if (dev) - parent_clk = clk_get(dev, cmu->clk_name); - else - parent_clk = of_clk_get_by_name(np, cmu->clk_name); - - if (IS_ERR(parent_clk)) { - pr_err("%s: could not find bus clock %s; err = %ld\n", - __func__, cmu->clk_name, PTR_ERR(parent_clk)); - } else { - clk_prepare_enable(parent_clk); - } - } + int err; + err = exynos_arm64_enable_bus_clk(dev, np, cmu); + if (err) + panic("%s: could not enable bus clock\n", __func__); exynos_arm64_init_clocks(np, cmu->clk_regs, cmu->nr_clk_regs); samsung_cmu_register_one(np, cmu); }