Re: [PATCH v13 05/13] clk: ingenic: Add driver for the TCU clocks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





Le mer. 26 juin 2019 à 0:09, Stephen Boyd <sboyd@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
 + */
 +
 +#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);
 +}






[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux