| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20190625220931.2F69B2086D@mail.kernel.org>
Date: Tue, 25 Jun 2019 15:09:30 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Daniel Lezcano <daniel.lezcano@...aro.org>,
James Hogan <jhogan@...nel.org>,
Jason Cooper <jason@...edaemon.net>,
Jonathan Corbet <corbet@....net>,
Lee Jones <lee.jones@...aro.org>,
Marc Zyngier <marc.zyngier@....com>,
Michael Turquette <mturquette@...libre.com>,
Paul Burton <paul.burton@...s.com>,
Paul Cercueil <paul@...pouillou.net>,
Ralf Baechle <ralf@...ux-mips.org>,
Thomas Gleixner <tglx@...utronix.de>
Cc: Mathieu Malaterre <malat@...ian.org>, od@...c.me,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mips@...r.kernel.org, linux-doc@...r.kernel.org,
linux-clk@...r.kernel.org, Paul Cercueil <paul@...pouillou.net>,
Artur Rojek <contact@...ur-rojek.eu>
Subject: Re: [PATCH v13 05/13] clk: ingenic: Add driver for the TCU clocks
Quoting Paul Cercueil (2019-06-24 15:57:51)
> diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
> new file mode 100644
> index 000000000000..6d667c4a2bd5
> --- /dev/null
> +++ b/drivers/clk/ingenic/tcu.c
> @@ -0,0 +1,473 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ47xx SoCs TCU clocks driver
> + * Copyright (C) 2019 Paul Cercueil <paul@...pouillou.net>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clockchips.h>
> +#include <linux/mfd/ingenic-tcu.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/syscore_ops.h>
> +
> +#include <dt-bindings/clock/ingenic,tcu.h>
> +
[...]
> +
> +static const struct ingenic_soc_info jz4740_soc_info = {
> + .num_channels = 8,
> + .has_ost = false,
> + .has_tcu_clk = true,
> +};
> +
> +static const struct ingenic_soc_info jz4725b_soc_info = {
> + .num_channels = 6,
> + .has_ost = true,
> + .has_tcu_clk = true,
> +};
> +
> +static const struct ingenic_soc_info jz4770_soc_info = {
> + .num_channels = 8,
> + .has_ost = true,
> + .has_tcu_clk = false,
> +};
> +
> +static const struct of_device_id ingenic_tcu_of_match[] __initconst = {
> + { .compatible = "ingenic,jz4740-tcu", .data = &jz4740_soc_info, },
> + { .compatible = "ingenic,jz4725b-tcu", .data = &jz4725b_soc_info, },
> + { .compatible = "ingenic,jz4770-tcu", .data = &jz4770_soc_info, },
> + { }
> +};
> +
> +static int __init ingenic_tcu_probe(struct device_node *np)
> +{
> + const struct of_device_id *id = of_match_node(ingenic_tcu_of_match, np);
> + struct ingenic_tcu *tcu;
> + struct regmap *map;
> + unsigned int i;
> + int ret;
> +
> + map = ingenic_tcu_get_regmap(np);
> + if (IS_ERR(map))
> + return PTR_ERR(map);
> +
> + tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
> + if (!tcu)
> + return -ENOMEM;
> +
> + tcu->map = map;
> + tcu->soc_info = id->data;
> +
> + if (tcu->soc_info->has_tcu_clk) {
> + tcu->clk = of_clk_get_by_name(np, "tcu");
Do you need to get the clk by name? Or can that clk be "known" to the
TCU somehow so we can already have a direct clk pointer?
> + if (IS_ERR(tcu->clk)) {
> + ret = PTR_ERR(tcu->clk);
> + pr_crit("Cannot get TCU clock\n");
> + goto err_free_tcu;
> + }
> +
> + ret = clk_prepare_enable(tcu->clk);
> + if (ret) {
> + pr_crit("Unable to enable TCU clock\n");
> + goto err_put_clk;
> + }
> + }
> +
> + tcu->clocks = kzalloc(sizeof(*tcu->clocks) +
> + sizeof(*tcu->clocks->hws) * TCU_CLK_COUNT,
> + GFP_KERNEL);
> + if (!tcu->clocks) {
> + ret = -ENOMEM;
> + goto err_clk_disable;
> + }
> +
> + tcu->clocks->num = TCU_CLK_COUNT;
> +
> + for (i = 0; i < tcu->soc_info->num_channels; i++) {
> + ret = ingenic_tcu_register_clock(tcu, i, TCU_PARENT_EXT,
> + &ingenic_tcu_clk_info[i],
> + tcu->clocks);
> + if (ret) {
> + pr_crit("cannot register clock %i\n", i);
> + goto err_unregister_timer_clocks;
> + }
> + }
> +
> + /*
> + * We set EXT as the default parent clock for all the TCU clocks
> + * except for the watchdog one, where we set the RTC clock as the
> + * parent. Since the EXT and PCLK are much faster than the RTC clock,
> + * the watchdog would kick after a maximum time of 5s, and we might
> + * want a slower kicking time.
> + */
> + ret = ingenic_tcu_register_clock(tcu, TCU_CLK_WDT, TCU_PARENT_RTC,
> + &ingenic_tcu_watchdog_clk_info,
> + tcu->clocks);
> + if (ret) {
> + pr_crit("cannot register watchdog clock\n");
> + goto err_unregister_timer_clocks;
> + }
> +
> + if (tcu->soc_info->has_ost) {
> + ret = ingenic_tcu_register_clock(tcu, TCU_CLK_OST,
> + TCU_PARENT_EXT,
> + &ingenic_tcu_ost_clk_info,
> + tcu->clocks);
> + if (ret) {
> + pr_crit("cannot register ost clock\n");
> + goto err_unregister_watchdog_clock;
> + }
> + }
> +
> + ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, tcu->clocks);
> + if (ret) {
> + pr_crit("cannot add OF clock provider\n");
> + goto err_unregister_ost_clock;
> + }
> +
> + ingenic_tcu = tcu;
> +
> + return 0;
> +
> +err_unregister_ost_clock:
> + if (tcu->soc_info->has_ost)
> + clk_hw_unregister(tcu->clocks->hws[i + 1]);
> +err_unregister_watchdog_clock:
> + clk_hw_unregister(tcu->clocks->hws[i]);
> +err_unregister_timer_clocks:
> + for (i = 0; i < tcu->clocks->num; i++)
> + if (tcu->clocks->hws[i])
> + clk_hw_unregister(tcu->clocks->hws[i]);
> + kfree(tcu->clocks);
> +err_clk_disable:
> + if (tcu->soc_info->has_tcu_clk)
> + clk_disable_unprepare(tcu->clk);
> +err_put_clk:
> + if (tcu->soc_info->has_tcu_clk)
> + clk_put(tcu->clk);
> +err_free_tcu:
> + kfree(tcu);
> + return ret;
Too bad this isn't a device with devm!
> +}
> +
> +static int __maybe_unused tcu_pm_suspend(void)
> +{
> + struct ingenic_tcu *tcu = ingenic_tcu;
> +
> + if (tcu->clk)
> + clk_disable(tcu->clk);
Do you need to unprepare? Or it just isn't possible because this is
called from syscore and thus we can't sleep?
> +
> + return 0;
> +}
> +
> +static void __maybe_unused tcu_pm_resume(void)
> +{
> + struct ingenic_tcu *tcu = ingenic_tcu;
> +
> + if (tcu->clk)
> + clk_enable(tcu->clk);
> +}
> +
> +static struct syscore_ops __maybe_unused tcu_pm_ops = {
> + .suspend = tcu_pm_suspend,
> + .resume = tcu_pm_resume,
> +};
> +
> +static void __init ingenic_tcu_init(struct device_node *np)
> +{
> + int ret = ingenic_tcu_probe(np);
> +
> + if (ret)
> + pr_crit("Failed to initialize TCU clocks: %i\n", ret);
Should be %d instead of %i? Applies to all this file.
> +
> + if (IS_ENABLED(CONFIG_PM_SLEEP))
> + register_syscore_ops(&tcu_pm_ops);
> +}
Powered by blists - more mailing lists