From: Paul Cercueil <paul@crapouillou.net>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: "Daniel Lezcano" <daniel.lezcano@linaro.org>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Ralf Baechle" <ralf@linux-mips.org>,
"Paul Burton" <paul.burton@mips.com>,
"James Hogan" <jhogan@kernel.org>,
"Jonathan Corbet" <corbet@lwn.net>,
"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
"Mathieu Malaterre" <malat@debian.org>,
od@zcrc.me, linux-pwm@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-watchdog@vger.kernel.org, linux-mips@vger.kernel.org,
linux-doc@vger.kernel.org, linux-clk@vger.kernel.org
Subject: Re: [PATCH v10 04/27] clocksource: Add a new timer-ingenic driver
Date: Mon, 04 Mar 2019 19:13:05 +0100 [thread overview]
Message-ID: <1551723186.4932.0@crapouillou.net> (raw)
In-Reply-To: <20190304122250.GC9040@ulmo>
Hi Thierry,
On Mon, Mar 4, 2019 at 1:22 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Sat, Mar 02, 2019 at 08:33:50PM -0300, Paul Cercueil wrote:
> [...]
>> diff --git a/drivers/clocksource/ingenic-timer.c
>> b/drivers/clocksource/ingenic-timer.c
> [...]
>> +struct ingenic_tcu {
>> + const struct ingenic_soc_info *soc_info;
>> + struct regmap *map;
>> + struct clk *clk, *timer_clk, *cs_clk;
>> +
>> + struct irq_domain *domain;
>> + unsigned int nb_parent_irqs;
>> + u32 parent_irqs[3];
>> +
>> + struct clk_hw_onecell_data *clocks;
>> +
>> + unsigned int timer_channel, cs_channel;
>> + struct clock_event_device cevt;
>> + struct clocksource cs;
>> + char name[4];
>> +
>> + unsigned long pwm_channels_mask;
>> +};
>> +
>> +static struct ingenic_tcu *ingenic_tcu;
>
> Is there really a need for this? Seems like the only two reasons for
> why
> you keep this around are as pointed out below.
>
>> +void __iomem *ingenic_tcu_base;
>> +EXPORT_SYMBOL_GPL(ingenic_tcu_base);
>
> Same here.
>
>> +static u64 notrace ingenic_tcu_timer_read(void)
>> +{
>> + unsigned int channel = ingenic_tcu->cs_channel;
>> + u16 count;
>> +
>> + count = readw(ingenic_tcu_base + TCU_REG_TCNTc(channel));
>
> Can't yo do this via the regmap?
I could, but for the sched_clock to be precise the function must return
as fast as possible. That's the rationale behind the use of readw()
here.
That's also the reason why ingenic_tcu_base is global and exported, so
that the OS Timer driver can use it as well.
>> +
>> + return count;
>> +}
>> +
>> +static u64 notrace ingenic_tcu_timer_cs_read(struct clocksource
>> *cs)
>> +{
>> + return ingenic_tcu_timer_read();
>> +}
>
> And here you've got access to the struct clocksource *, from which you
> should be able to get at struct ingenic_tcu * using container_of.
>
> [...]
>> +static int __init ingenic_tcu_init(struct device_node *np)
>> +{
>> + const struct of_device_id *id =
>> of_match_node(ingenic_tcu_of_match, np);
>> + const struct ingenic_soc_info *soc_info = id->data;
>> + struct ingenic_tcu *tcu;
>> + struct resource res;
>> + void __iomem *base;
>> + long rate;
>> + int ret;
>> +
>> + of_node_clear_flag(np, OF_POPULATED);
>> +
>> + tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
>> + if (!tcu)
>> + return -ENOMEM;
>> +
>> + /* Enable all TCU channels for PWM use by default except channel 0
>> + * and channel 1.
>> + */
>> + tcu->pwm_channels_mask = GENMASK(soc_info->num_channels - 1, 2);
>> + of_property_read_u32(np, "ingenic,pwm-channels-mask",
>> + (u32 *)&tcu->pwm_channels_mask);
>> +
>> + /* Verify that we have at least two free channels */
>> + if (hweight8(tcu->pwm_channels_mask) > soc_info->num_channels -
>> 2) {
>> + pr_crit("ingenic-tcu: Invalid PWM channel mask: 0x%02lx\n",
>> + tcu->pwm_channels_mask);
>> + return -EINVAL;
>> + }
>> +
>> + tcu->soc_info = soc_info;
>> + ingenic_tcu = tcu;
>> +
>> + tcu->timer_channel = find_first_zero_bit(&tcu->pwm_channels_mask,
>> + soc_info->num_channels);
>> + tcu->cs_channel = find_next_zero_bit(&tcu->pwm_channels_mask,
>> + soc_info->num_channels,
>> + tcu->timer_channel + 1);
>> +
>> + base = of_io_request_and_map(np, 0, NULL);
>> + if (IS_ERR(base)) {
>> + ret = PTR_ERR(base);
>> + goto err_free_ingenic_tcu;
>> + }
>> +
>> + of_address_to_resource(np, 0, &res);
>> +
>> + ingenic_tcu_base = base;
>> +
>> + tcu->map = regmap_init_mmio(NULL, base,
>> &ingenic_tcu_regmap_config);
>> + if (IS_ERR(tcu->map)) {
>> + ret = PTR_ERR(tcu->map);
>> + goto err_iounmap;
>> + }
>> +
>> + tcu->clk = of_clk_get_by_name(np, "tcu");
>> + if (IS_ERR(tcu->clk)) {
>> + ret = PTR_ERR(tcu->clk);
>> + pr_crit("ingenic-tcu: Unable to find TCU clock: %i\n", ret);
>> + goto err_free_regmap;
>> + }
>> +
>> + ret = clk_prepare_enable(tcu->clk);
>> + if (ret) {
>> + pr_crit("ingenic-tcu: Unable to enable TCU clock: %i\n", ret);
>> + goto err_clk_put;
>> + }
>> +
>> + ret = ingenic_tcu_intc_init(tcu, np);
>> + if (ret)
>> + goto err_clk_disable;
>> +
>> + ret = ingenic_tcu_clk_init(tcu, np);
>> + if (ret)
>> + goto err_tcu_intc_cleanup;
>> +
>> + ret = ingenic_tcu_clocksource_init(tcu);
>> + if (ret)
>> + goto err_tcu_clk_cleanup;
>> +
>> + ret = ingenic_tcu_timer_init(tcu);
>> + if (ret)
>> + goto err_tcu_clocksource_cleanup;
>> +
>> + // Register the sched_clock at the very end as there's no way to
>> undo it
>> + rate = clk_get_rate(tcu->cs_clk);
>> + sched_clock_register(ingenic_tcu_timer_read, 16, rate);
>
> Oh wow... so you managed to nicely encapsulate everything and now this
> seems to be the only reason why you need to rely on global variables.
>
> That's unfortunate. I suppose we could go and add a void *data
> parameter
> to sched_clock_register() and pass that to the read() function. That
> way
> you could make this completely independent of global variables, but
> there are 73 callers of sched_clock_register() and they are spread all
> over the place, so sounds a bit daunting to me.
Yes, that's the main reason behind the use of a global variables.
Is there a way we could introduce another callback, e.g. .read_value(),
that would receive a void *param? Then the current .read() callback
can be deprecated and the 73 callers can be migrated later.
>> +
>> + return 0;
>> +
>> +err_tcu_clocksource_cleanup:
>> + ingenic_tcu_clocksource_cleanup(tcu);
>> +err_tcu_clk_cleanup:
>> + ingenic_tcu_clk_cleanup(tcu, np);
>> +err_tcu_intc_cleanup:
>> + ingenic_tcu_intc_cleanup(tcu);
>> +err_clk_disable:
>> + clk_disable_unprepare(tcu->clk);
>> +err_clk_put:
>> + clk_put(tcu->clk);
>> +err_free_regmap:
>> + regmap_exit(tcu->map);
>> +err_iounmap:
>> + iounmap(base);
>> + release_mem_region(res.start, resource_size(&res));
>> +err_free_ingenic_tcu:
>> + kfree(tcu);
>> + return ret;
>> +}
>> +
>> +TIMER_OF_DECLARE(jz4740_tcu_intc, "ingenic,jz4740-tcu",
>> ingenic_tcu_init);
>> +TIMER_OF_DECLARE(jz4725b_tcu_intc, "ingenic,jz4725b-tcu",
>> ingenic_tcu_init);
>> +TIMER_OF_DECLARE(jz4770_tcu_intc, "ingenic,jz4770-tcu",
>> ingenic_tcu_init);
>> +
>> +
>> +static int __init ingenic_tcu_probe(struct platform_device *pdev)
>> +{
>> + platform_set_drvdata(pdev, ingenic_tcu);
>
> Then there's also this. Oh well... nevermind then.
The content of ingenic_tcu_platform_init() could be moved inside
ingenic_tcu_init(). But can we get a hold of the struct device before
the
probe function is called? That would allow to set the drvdata and regmap
without relying on global state.
>> +bool ingenic_tcu_pwm_can_use_chn(unsigned int channel)
>> +{
>> + return !!(ingenic_tcu->pwm_channels_mask & BIT(channel));
>> +}
>> +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
>
> I wonder if we could make this slightly nicer, though. Judging by the
> name, this one seems like it would only ever be used by the PWM. If
> the
> PWM is a child of the TCU, could it not pass in a struct device * that
> points to its parent into this? Or perhaps pass its own struct device
> *
> and have this function look up the struct ingenic_tcu from that?
> Should
> be something trivial as:
>
> bool ingenic_tcu_pwm_can_use_channel(struct device *dev, unsigned
> int channel)
> {
> struct ingenic_tcu *tcu = dev_get_drvdata(dev->parent);
>
> return !!(tcu->pwm_channels_mask & BIT(channel));
> }
>
> Thierry
Yes, that definitely looks better. Thanks for the hint.
-Paul
next prev parent reply other threads:[~2019-03-04 18:13 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-02 23:33 [5.2][PATCH v10 00/27] Ingenic TCU patchset Paul Cercueil
2019-03-02 23:33 ` [PATCH v10 01/27] dt-bindings: ingenic: Add DT bindings for TCU clocks Paul Cercueil
2019-03-02 23:33 ` [PATCH v10 02/27] doc: Add doc for the Ingenic TCU hardware Paul Cercueil
2019-03-02 23:33 ` [PATCH v10 03/27] dt-bindings: Add doc for the Ingenic TCU drivers Paul Cercueil
2019-03-02 23:33 ` [PATCH v10 04/27] clocksource: Add a new timer-ingenic driver Paul Cercueil
2019-03-04 12:22 ` Thierry Reding
2019-03-04 18:13 ` Paul Cercueil [this message]
2019-03-08 10:22 ` Thierry Reding
2019-03-11 20:52 ` Paul Cercueil
2019-03-05 3:15 ` kbuild test robot
2019-03-02 23:33 ` [PATCH v10 05/27] clocksource: Add driver for the Ingenic JZ47xx OST Paul Cercueil
2019-03-02 23:33 ` [PATCH v10 06/27] MAINTAINERS: Add myself as maintainer for Ingenic TCU drivers Paul Cercueil
2019-03-02 23:33 ` [PATCH v10 07/27] watchdog: jz4740: Use WDT clock provided by TCU driver Paul Cercueil
2019-03-02 23:33 ` [PATCH v10 08/27] watchdog: jz4740: Use regmap " Paul Cercueil
2019-03-02 23:33 ` [PATCH v10 09/27] watchdog: jz4740: Avoid starting watchdog in set_timeout Paul Cercueil
2019-03-02 23:33 ` [PATCH v10 10/27] watchdog: jz4740: Drop dependency on MACH_JZ47xx, use COMPILE_TEST Paul Cercueil
2019-03-02 23:33 ` [PATCH v10 11/27] pwm: jz4740: Apply configuration atomically Paul Cercueil
2019-03-04 11:59 ` Thierry Reding
2019-03-02 23:33 ` [PATCH v10 12/27] pwm: jz4740: Use regmap from TCU driver Paul Cercueil
2019-03-04 12:24 ` Thierry Reding
2019-03-02 23:33 ` [PATCH v10 13/27] pwm: jz4740: Use clocks " Paul Cercueil
2019-03-04 12:30 ` Thierry Reding
2019-03-04 18:18 ` Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 14/27] pwm: jz4740: Improve algorithm of clock calculation Paul Cercueil
2019-03-04 12:33 ` Thierry Reding
2019-03-02 23:34 ` [PATCH v10 15/27] pwm: jz4740: Allow selection of PWM channels 0 and 1 Paul Cercueil
2019-03-04 12:34 ` Thierry Reding
2019-03-02 23:34 ` [PATCH v10 16/27] pwm: jz4740: Drop dependency on MACH_INGENIC, use COMPILE_TEST Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 17/27] pwm: jz4740: Remove unused devicetree compatible strings Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 18/27] clk: jz4740: Add TCU clock Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 19/27] MIPS: Kconfig: Select TCU timer driver when MACH_INGENIC is set Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 20/27] MIPS: jz4740: Add DTS nodes for the TCU drivers Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 21/27] MIPS: qi_lb60: Move PWM devices to devicetree Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 22/27] MIPS: qi_lb60: Reduce system timer and clocksource to 750 kHz Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 23/27] MIPS: CI20: Reduce system timer and clocksource to 3 MHz Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 24/27] MIPS: CI20: defconfig: enable OST driver Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 25/27] MIPS: GCW0: Reduce system timer and clocksource to 750 kHz Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 26/27] MIPS: GCW0: defconfig: Enable OST, watchdog, PWM drivers Paul Cercueil
2019-03-02 23:34 ` [PATCH v10 27/27] 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=1551723186.4932.0@crapouillou.net \
--to=paul@crapouillou.net \
--cc=corbet@lwn.net \
--cc=daniel.lezcano@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=jhogan@kernel.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=linux-pwm@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=malat@debian.org \
--cc=od@zcrc.me \
--cc=paul.burton@mips.com \
--cc=ralf@linux-mips.org \
--cc=tglx@linutronix.de \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.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).