From: Paul Cercueil <paul@crapouillou.net>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>,
James Hogan <jhogan@kernel.org>,
Jason Cooper <jason@lakedaemon.net>,
Jonathan Corbet <corbet@lwn.net>,
Lee Jones <lee.jones@linaro.org>,
Marc Zyngier <marc.zyngier@arm.com>,
Michael Turquette <mturquette@baylibre.com>,
Paul Burton <paul.burton@mips.com>,
Ralf Baechle <ralf@linux-mips.org>,
Thomas Gleixner <tglx@linutronix.de>,
Mathieu Malaterre <malat@debian.org>,
od@zcrc.me, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org,
linux-doc@vger.kernel.org, linux-clk@vger.kernel.org,
Artur Rojek <contact@artur-rojek.eu>
Subject: Re: [PATCH v13 05/13] clk: ingenic: Add driver for the TCU clocks
Date: Wed, 26 Jun 2019 00:37:07 +0200 [thread overview]
Message-ID: <1561502227.10069.1@crapouillou.net> (raw)
In-Reply-To: <20190625220931.2F69B2086D@mail.kernel.org>
Le mer. 26 juin 2019 à 0:09, Stephen Boyd <sboyd@kernel.org> a écrit :
> 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@crapouillou.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?
This clock is provided by a separate driver, so I have to obtain the
clock pointer from devicetree.
>
>> + 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?
I thought that clk_disable() was enough. We don't actually need to
unprepare, do we? And yes, as you pointed out, this call cannot 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.
OK.
Thanks,
-Paul
>> +
>> + if (IS_ENABLED(CONFIG_PM_SLEEP))
>> + register_syscore_ops(&tcu_pm_ops);
>> +}
next prev parent reply other threads:[~2019-06-25 22:37 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-24 22:57 [PATCH v13 00/13] Ingenic Timer/Counter Unit patchset v13 Paul Cercueil
2019-06-24 22:57 ` [PATCH v13 01/13] dt-bindings: ingenic: Add DT bindings for TCU clocks Paul Cercueil
2019-06-24 22:57 ` [PATCH v13 02/13] doc: Add doc for the Ingenic TCU hardware Paul Cercueil
2019-06-25 6:44 ` Thomas Gleixner
2019-06-24 22:57 ` [PATCH v13 03/13] dt-bindings: Add doc for the Ingenic TCU drivers Paul Cercueil
2019-06-24 22:57 ` [PATCH v13 04/13] mfd: Add Ingenic TCU driver Paul Cercueil
2019-06-25 17:30 ` Paul Burton
2019-06-25 17:47 ` Paul Cercueil
2019-06-24 22:57 ` [PATCH v13 05/13] clk: ingenic: Add driver for the TCU clocks Paul Cercueil
2019-06-25 22:09 ` Stephen Boyd
2019-06-25 22:37 ` Paul Cercueil [this message]
2019-06-25 23:28 ` Stephen Boyd
2019-06-24 22:57 ` [PATCH v13 06/13] irqchip: Add irq-ingenic-tcu driver Paul Cercueil
2019-06-25 6:51 ` Thomas Gleixner
2019-06-24 22:57 ` [PATCH v13 07/13] clocksource: Add a new timer-ingenic driver Paul Cercueil
2019-06-25 6:53 ` Thomas Gleixner
2019-06-24 22:57 ` [PATCH v13 08/13] clk: jz4740: Add TCU clock Paul Cercueil
2019-06-24 22:57 ` [PATCH v13 09/13] MIPS: jz4740: Add DTS nodes for the TCU drivers Paul Cercueil
2019-06-24 22:57 ` [PATCH v13 10/13] MIPS: qi_lb60: Reduce system timer and clocksource to 750 kHz Paul Cercueil
2019-06-24 22:57 ` [PATCH v13 11/13] MIPS: CI20: Reduce system timer and clocksource to 3 MHz Paul Cercueil
2019-06-24 22:57 ` [PATCH v13 12/13] MIPS: GCW0: Reduce system timer and clocksource to 750 kHz Paul Cercueil
2019-06-24 22:57 ` [PATCH v13 13/13] MIPS: jz4740: Drop obsolete code Paul Cercueil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1561502227.10069.1@crapouillou.net \
--to=paul@crapouillou.net \
--cc=contact@artur-rojek.eu \
--cc=corbet@lwn.net \
--cc=daniel.lezcano@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=jason@lakedaemon.net \
--cc=jhogan@kernel.org \
--cc=lee.jones@linaro.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@vger.kernel.org \
--cc=malat@debian.org \
--cc=marc.zyngier@arm.com \
--cc=mturquette@baylibre.com \
--cc=od@zcrc.me \
--cc=paul.burton@mips.com \
--cc=ralf@linux-mips.org \
--cc=sboyd@kernel.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).