Linux-MIPS Archive on lore.kernel.org
 help / color / Atom feed
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>,
	Mark Rutland <mark.rutland@arm.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Paul Burton <paul.burton@mips.com>,
	Ralf Baechle <ralf@linux-mips.org>,
	Rob Herring <robh+dt@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Mathieu Malaterre <malat@debian.org>,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	linux-mips@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-clk@vger.kernel.org, od@zcrc.me
Subject: Re: [PATCH v12 05/13] clk: ingenic: Add driver for the TCU clocks
Date: Fri, 07 Jun 2019 23:59:54 +0200
Message-ID: <1559944794.11351.0@crapouillou.net> (raw)
In-Reply-To: <20190607212819.A5FAE208C3@mail.kernel.org>

Hi Stephen, thanks for the review.

Le ven. 7 juin 2019 à 23:28, Stephen Boyd <sboyd@kernel.org> a écrit :
> Quoting Paul Cercueil (2019-05-21 07:51:33)
>>  diff --git a/drivers/clk/ingenic/Kconfig 
>> b/drivers/clk/ingenic/Kconfig
>>  index 34dc0da79c39..434893133eb4 100644
>>  --- a/drivers/clk/ingenic/Kconfig
>>  +++ b/drivers/clk/ingenic/Kconfig
>>  @@ -1,4 +1,4 @@
>>  -menu "Ingenic JZ47xx CGU drivers"
>>  +menu "Ingenic JZ47xx drivers"
>>          depends on MIPS
>> 
>>   config INGENIC_CGU_COMMON
>>  @@ -44,4 +44,13 @@ config INGENIC_CGU_JZ4780
>> 
>>            If building for a JZ4780 SoC, you want to say Y here.
>> 
>>  +config INGENIC_TCU_CLK
>>  +       bool "Ingenic JZ47xx TCU clocks driver"
>>  +       default MACH_INGENIC
>>  +       depends on COMMON_CLK
> 
> Does the INGENIC_TCU_CLK config even exist if COMMON_CLK is disabled? 
> I
> suspect it's all part of the menuconfig so this depends is not useful?

Right, it can be dropped.

>>  +       select INGENIC_TCU
>>  +       help
>>  +         Support the clocks of the Timer/Counter Unit (TCU) of the 
>> Ingenic
>>  +         JZ47xx SoCs.
>>  +
>>   endmenu
>>  diff --git a/drivers/clk/ingenic/tcu.c b/drivers/clk/ingenic/tcu.c
>>  new file mode 100644
>>  index 000000000000..7249225a6994
>>  --- /dev/null
>>  +++ b/drivers/clk/ingenic/tcu.c
>>  @@ -0,0 +1,458 @@
>>  +// 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/clkdev.h>
>>  +#include <linux/clockchips.h>
>>  +#include <linux/mfd/ingenic-tcu.h>
>>  +#include <linux/regmap.h>
>>  +
>>  +#include <dt-bindings/clock/ingenic,tcu.h>
>>  +
>>  +/* 8 channels max + watchdog + OST */
>>  +#define TCU_CLK_COUNT  10
>>  +
>>  +#define TCU_ERR(...) pr_crit("ingenic-tcu-clk: " __VA_ARGS__)
> 
> Why is it pr_crit instead of pr_err()?

If the TCU timer clocks are not provided for any reason, the system
will have no timer, and the kernel will hang very early in the init
process. That's why I chose pr_crit().

>>  +
>>  +enum tcu_clk_parent {
>>  +       TCU_PARENT_PCLK,
>>  +       TCU_PARENT_RTC,
>>  +       TCU_PARENT_EXT,
>>  +};
>>  +
> [...]
>>  +
>>  +static int __init ingenic_tcu_register_clock(struct ingenic_tcu 
>> *tcu,
>>  +                       unsigned int idx, enum tcu_clk_parent 
>> parent,
>>  +                       const struct ingenic_tcu_clk_info *info,
>>  +                       struct clk_hw_onecell_data *clocks)
>>  +{
>>  +       struct ingenic_tcu_clk *tcu_clk;
>>  +       int err;
>>  +
>>  +       tcu_clk = kzalloc(sizeof(*tcu_clk), GFP_KERNEL);
>>  +       if (!tcu_clk)
>>  +               return -ENOMEM;
>>  +
>>  +       tcu_clk->hw.init = &info->init_data;
>>  +       tcu_clk->idx = idx;
>>  +       tcu_clk->info = info;
>>  +       tcu_clk->tcu = tcu;
>>  +
>>  +       /* Reset channel and clock divider, set default parent */
>>  +       ingenic_tcu_enable_regs(&tcu_clk->hw);
>>  +       regmap_update_bits(tcu->map, info->tcsr_reg, 0xffff, 
>> BIT(parent));
>>  +       ingenic_tcu_disable_regs(&tcu_clk->hw);
>>  +
>>  +       err = clk_hw_register(NULL, &tcu_clk->hw);
>>  +       if (err)
>>  +               goto err_free_tcu_clk;
>>  +
>>  +       err = clk_hw_register_clkdev(&tcu_clk->hw, 
>> info->init_data.name, NULL);
> 
> Do you have a use for clkdev? If DT lookups work just as well it would
> be better to skip clkdev registration.

OK.

>>  +       if (err)
>>  +               goto err_clk_unregister;
>>  +
>>  +       clocks->hws[idx] = &tcu_clk->hw;
>>  +
>>  +       return 0;
>>  +
>>  +err_clk_unregister:
>>  +       clk_hw_unregister(&tcu_clk->hw);
>>  +err_free_tcu_clk:
>>  +       kfree(tcu_clk);
>>  +       return err;
>>  +}
>>  +
>>  +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");
> 
> Is this clk necessary to read/write registers in this clk driver? And
> this clk isn't the parent of the clks? Why is it managed by Linux at
> all? Will there be a time when it's turned off?

For the SoCs which have the "tcu" clock, it has to be enabled for the
registers to be accessible, yes. And as you noticed, it is not the
parent of the timer clocks.

The "tcu" clock can be turned off during suspend, for instance.

> I'm asking because it looks like we're calling clk APIs from within 
> clk
> provider implementation. That works right now because of our locking
> scheme, but this will put up another roadblock to making the prepare 
> and
> enable locks not recursive. I've seen some drivers take an approach of
> enabling the clk when the provider is PM runtime active, and disable 
> the
> clk when the provider is runtime PM inactive. This gets it out of the
> provider path and into the runtime PM path. If you take that approach
> then when we move the runtime PM code in clk core outside of the 
> prepare
> lock we should be able to avoid any recursive locking scenarios.

Most of the code here works without a struct device, it wouldn't be 
easy to
get it to work with runtime PM.

I can enable the "tcu" clock in the probe and just gate/ungate it in the
suspend/resume callbacks, that would work just fine. We don't need 
anything
fancy here.

>>  +               if (IS_ERR(tcu->clk)) {
>>  +                       ret = PTR_ERR(tcu->clk);
>>  +                       TCU_ERR("Cannot get TCU clock\n");
>>  +                       goto err_free_tcu;
>>  +               }
>>  +



  reply index

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-21 14:51 Ingenic Timer/Counter Unit (TCU) patchset v12 Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 01/13] dt-bindings: ingenic: Add DT bindings for TCU clocks Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 02/13] doc: Add doc for the Ingenic TCU hardware Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 03/13] dt-bindings: Add doc for the Ingenic TCU drivers Paul Cercueil
2019-05-24 20:21   ` Rob Herring
2019-05-25 19:13     ` Paul Cercueil
2019-06-11 14:57       ` Rob Herring
2019-05-21 14:51 ` [PATCH v12 04/13] mfd: Add Ingenic TCU driver Paul Cercueil
2019-06-22 12:21   ` Paul Cercueil
2019-06-26 13:18   ` Lee Jones
2019-06-26 13:55     ` Paul Cercueil
2019-06-27  6:58       ` Lee Jones
2019-06-27  8:49         ` Paul Cercueil
2019-06-27  9:01           ` Lee Jones
2019-06-27  9:19             ` Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 05/13] clk: ingenic: Add driver for the TCU clocks Paul Cercueil
2019-06-07 21:28   ` Stephen Boyd
2019-06-07 21:59     ` Paul Cercueil [this message]
2019-06-07 22:50       ` Stephen Boyd
2019-05-21 14:51 ` [PATCH v12 06/13] irqchip: Add irq-ingenic-tcu driver Paul Cercueil
2019-06-22 12:22   ` Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 07/13] clocksource: Add a new timer-ingenic driver Paul Cercueil
2019-06-22 12:23   ` Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 08/13] clk: jz4740: Add TCU clock Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 09/13] MIPS: jz4740: Add DTS nodes for the TCU drivers Paul Cercueil
2019-05-22  9:21   ` Mathieu Malaterre
2019-05-22 11:15     ` Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 10/13] MIPS: qi_lb60: Reduce system timer and clocksource to 750 kHz Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 11/13] MIPS: CI20: Reduce system timer and clocksource to 3 MHz Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 12/13] MIPS: GCW0: Reduce system timer and clocksource to 750 kHz Paul Cercueil
2019-05-21 14:51 ` [PATCH v12 13/13] MIPS: jz4740: Drop obsolete code Paul Cercueil
2019-05-27 11:39 ` Ingenic Timer/Counter Unit (TCU) patchset v12 Mathieu Malaterre

Reply instructions:

You may reply publically 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=1559944794.11351.0@crapouillou.net \
    --to=paul@crapouillou.net \
    --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=mark.rutland@arm.com \
    --cc=mturquette@baylibre.com \
    --cc=od@zcrc.me \
    --cc=paul.burton@mips.com \
    --cc=ralf@linux-mips.org \
    --cc=robh+dt@kernel.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

Linux-MIPS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mips/0 linux-mips/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mips linux-mips/ https://lore.kernel.org/linux-mips \
		linux-mips@vger.kernel.org
	public-inbox-index linux-mips

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mips


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git