| 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: <CAMuHMdXOaasX9Dv-Kv=VOO0dhnp8ObQC6-YqsFZT0Q-VeqPg+Q@mail.gmail.com>
Date: Mon, 26 Feb 2024 17:54:43 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Yoshinori Sato <ysato@...rs.sourceforge.jp>
Cc: linux-sh@...r.kernel.org, Damien Le Moal <dlemoal@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
Geert Uytterhoeven <geert+renesas@...der.be>, Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Thomas Gleixner <tglx@...utronix.de>, Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Bjorn Helgaas <bhelgaas@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>, Magnus Damm <magnus.damm@...il.com>,
Daniel Lezcano <daniel.lezcano@...aro.org>, Rich Felker <dalias@...c.org>,
John Paul Adrian Glaubitz <glaubitz@...sik.fu-berlin.de>, Lee Jones <lee@...nel.org>,
Helge Deller <deller@....de>, Heiko Stuebner <heiko@...ech.de>,
Jernej Skrabec <jernej.skrabec@...il.com>, Chris Morgan <macromorgan@...mail.com>,
Yang Xiwen <forbidden405@...mail.com>, Sebastian Reichel <sre@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>, Randy Dunlap <rdunlap@...radead.org>,
Arnd Bergmann <arnd@...db.de>, Vlastimil Babka <vbabka@...e.cz>, Hyeonggon Yoo <42.hyeyoo@...il.com>,
David Rientjes <rientjes@...gle.com>, Baoquan He <bhe@...hat.com>,
Andrew Morton <akpm@...ux-foundation.org>, Guenter Roeck <linux@...ck-us.net>,
Stephen Rothwell <sfr@...b.auug.org.au>, Azeem Shaikh <azeemshaikh38@...il.com>,
Javier Martinez Canillas <javierm@...hat.com>, Max Filippov <jcmvbkbc@...il.com>,
Palmer Dabbelt <palmer@...osinc.com>, Bin Meng <bmeng@...ylab.org>,
Jonathan Corbet <corbet@....net>, Jacky Huang <ychuang3@...oton.com>,
Lukas Bulwahn <lukas.bulwahn@...il.com>, Biju Das <biju.das.jz@...renesas.com>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
Sam Ravnborg <sam@...nborg.org>, Sergey Shtylyov <s.shtylyov@....ru>,
Michael Karcher <kernel@...rcher.dialup.fu-berlin.de>,
Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>, linux-ide@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, linux-clk@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-pci@...r.kernel.org,
linux-serial@...r.kernel.org, linux-fbdev@...r.kernel.org
Subject: Re: [DO NOT MERGE v6 08/37] clocksource: sh_tmu: CLOCKSOURCE support.
Hi Saton-san,
Thanks for your patch!
Please drop the period at the end of the one-line summary.
On Tue, Jan 9, 2024 at 9:23 AM Yoshinori Sato
<ysato@...rs.sourceforge.jp> wrote:
> Allows initialization as CLOCKSOURCE.
Please explain why this is needed. E.g.
Add support for early registration using TIMER_OF_DECLARE(),
so the timer can be used as a clocksource on SoCs that do not
have any other suitable timer.
>
> Signed-off-by: Yoshinori Sato <ysato@...rs.sourceforge.jp>
> --- a/drivers/clocksource/sh_tmu.c
> +++ b/drivers/clocksource/sh_tmu.c
> @@ -148,8 +151,8 @@ static int __sh_tmu_enable(struct sh_tmu_channel *ch)
> /* enable clock */
> ret = clk_enable(ch->tmu->clk);
> if (ret) {
> - dev_err(&ch->tmu->pdev->dev, "ch%u: cannot enable clock\n",
> - ch->index);
> + pr_err("%s ch%u: cannot enable clock\n",
> + ch->tmu->name, ch->index);
Please wrap the line after, not before, "ch->tmu->name,".
> return ret;
> }
>
> @@ -324,14 +332,14 @@ static int sh_tmu_register_clocksource(struct sh_tmu_channel *ch,
> cs->mask = CLOCKSOURCE_MASK(32);
> cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
>
> - dev_info(&ch->tmu->pdev->dev, "ch%u: used as clock source\n",
> - ch->index);
> + pr_info("%s ch%u: used as clock source\n",
> + ch->tmu->name, ch->index);
No need to wrap this line at all.
>
> clocksource_register_hz(cs, ch->tmu->rate);
> return 0;
> }
>
> -static struct sh_tmu_channel *ced_to_sh_tmu(struct clock_event_device *ced)
> +static inline struct sh_tmu_channel *ced_to_sh_tmu(struct clock_event_device *ced)
> {
> return container_of(ced, struct sh_tmu_channel, ced);
> }
> @@ -364,8 +372,8 @@ static int sh_tmu_clock_event_set_state(struct clock_event_device *ced,
> if (clockevent_state_oneshot(ced) || clockevent_state_periodic(ced))
> sh_tmu_disable(ch);
>
> - dev_info(&ch->tmu->pdev->dev, "ch%u: used for %s clock events\n",
> - ch->index, periodic ? "periodic" : "oneshot");
> + pr_info("%s ch%u: used for %s clock events\n",
> + ch->tmu->name, ch->index, periodic ? "periodic" : "oneshot");
Please wrap the line after, not before, "ch->tmu->name,".
> sh_tmu_clock_event_start(ch, periodic);
> return 0;
> }
> @@ -403,7 +411,8 @@ static void sh_tmu_clock_event_resume(struct clock_event_device *ced)
> }
>
> static void sh_tmu_register_clockevent(struct sh_tmu_channel *ch,
> - const char *name)
> + const char *name,
> + struct device_node *np)
"np" is unused in this function, hence this change is unneeded.
(Hey, I already said that in my review of v3)
> {
> struct clock_event_device *ced = &ch->ced;
> int ret;
> @@ -417,30 +426,32 @@ static void sh_tmu_register_clockevent(struct sh_tmu_channel *ch,
> ced->set_state_shutdown = sh_tmu_clock_event_shutdown;
> ced->set_state_periodic = sh_tmu_clock_event_set_periodic;
> ced->set_state_oneshot = sh_tmu_clock_event_set_oneshot;
> - ced->suspend = sh_tmu_clock_event_suspend;
> - ced->resume = sh_tmu_clock_event_resume;
> -
> - dev_info(&ch->tmu->pdev->dev, "ch%u: used for clock events\n",
> - ch->index);
> + if (ch->tmu->pdev) {
> + ced->suspend = sh_tmu_clock_event_suspend;
> + ced->resume = sh_tmu_clock_event_resume;
> + }
> + pr_info("%s ch%u: used for clock events\n",
> + ch->tmu->name, ch->index);
No need to wrap this line at all.
>
> clockevents_config_and_register(ced, ch->tmu->rate, 0x300, 0xffffffff);
>
> ret = request_irq(ch->irq, sh_tmu_interrupt,
> IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
> - dev_name(&ch->tmu->pdev->dev), ch);
> + ch->tmu->name, ch);
> if (ret) {
> - dev_err(&ch->tmu->pdev->dev, "ch%u: failed to request irq %d\n",
> - ch->index, ch->irq);
> + pr_err("%s ch%u: failed to request irq %d\n",
> + ch->tmu->name, ch->index, ch->irq);
Please wrap the line after, not before, "ch->tmu->name,".
> return;
> }
> }
>
> static int sh_tmu_register(struct sh_tmu_channel *ch, const char *name,
> + struct device_node *np,
np is unneeded.
> bool clockevent, bool clocksource)
> {
> if (clockevent) {
> ch->tmu->has_clockevent = true;
> - sh_tmu_register_clockevent(ch, name);
> + sh_tmu_register_clockevent(ch, name, np);
> } else if (clocksource) {
> ch->tmu->has_clocksource = true;
> sh_tmu_register_clocksource(ch, name);
> @@ -465,53 +477,59 @@ static int sh_tmu_channel_setup(struct sh_tmu_channel *ch, unsigned int index,
> else
> ch->base = tmu->mapbase + 8 + ch->index * 12;
>
> - ch->irq = platform_get_irq(tmu->pdev, index);
> + if (tmu->pdev)
> + ch->irq = platform_get_irq(tmu->pdev, index);
> + else
> + ch->irq = of_irq_get(np, index);
You can use of_irq_get() unconditionally.
> if (ch->irq < 0)
> return ch->irq;
>
> ch->cs_enabled = false;
> ch->enable_count = 0;
>
> - return sh_tmu_register(ch, dev_name(&tmu->pdev->dev),
> + return sh_tmu_register(ch, tmu->name, np,
No need to pass np.
> clockevent, clocksource);
> }
>
> -static int sh_tmu_map_memory(struct sh_tmu_device *tmu)
> +static int sh_tmu_map_memory(struct sh_tmu_device *tmu, struct device_node *np)
> {
> struct resource *res;
>
> - res = platform_get_resource(tmu->pdev, IORESOURCE_MEM, 0);
> - if (!res) {
> - dev_err(&tmu->pdev->dev, "failed to get I/O memory\n");
> - return -ENXIO;
> - }
> + if (tmu->pdev) {
> + res = platform_get_resource(tmu->pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + pr_err("sh_tmu failed to get I/O memory\n");
> + return -ENXIO;
> + }
> +
> + tmu->mapbase = ioremap(res->start, resource_size(res));
> + } else
> + tmu->mapbase = of_iomap(np, 0);
You can use of_iomap() unconditionally.
>
> - tmu->mapbase = ioremap(res->start, resource_size(res));
> if (tmu->mapbase == NULL)
> return -ENXIO;
>
> return 0;
> }
>
> -static int sh_tmu_parse_dt(struct sh_tmu_device *tmu)
> +static int sh_tmu_parse_dt(struct sh_tmu_device *tmu, struct device_node *np)
> {
> - struct device_node *np = tmu->pdev->dev.of_node;
> -
> tmu->model = SH_TMU;
> tmu->num_channels = 3;
>
> of_property_read_u32(np, "#renesas,channels", &tmu->num_channels);
>
> if (tmu->num_channels != 2 && tmu->num_channels != 3) {
> - dev_err(&tmu->pdev->dev, "invalid number of channels %u\n",
> - tmu->num_channels);
> + pr_err("%s: invalid number of channels %u\n",
> + tmu->name, tmu->num_channels);
Please wrap the line after, not before, "ch->tmu->name,".
> return -EINVAL;
> }
>
> return 0;
> }
>
> -static int sh_tmu_setup(struct sh_tmu_device *tmu, struct platform_device *pdev)
> +static int sh_tmu_setup(struct sh_tmu_device *tmu,
> + struct platform_device *pdev, struct device_node *np)
> {
> unsigned int i;
> int ret;
> @@ -531,14 +554,17 @@ static int sh_tmu_setup(struct sh_tmu_device *tmu, struct platform_device *pdev)
> tmu->model = id->driver_data;
> tmu->num_channels = hweight8(cfg->channels_mask);
> } else {
> - dev_err(&tmu->pdev->dev, "missing platform data\n");
> + pr_err("%s missing platform data\n", tmu->name);
> return -ENXIO;
> }
>
> /* Get hold of clock. */
> - tmu->clk = clk_get(&tmu->pdev->dev, "fck");
> + if (pdev)
> + tmu->clk = clk_get(&tmu->pdev->dev, "fck");
> + else
> + tmu->clk = of_clk_get(np, 0);
You can use of_clk_get() unconditionally.
> if (IS_ERR(tmu->clk)) {
> - dev_err(&tmu->pdev->dev, "cannot get clock\n");
> + pr_err("%s: cannot get clock\n", tmu->name);
> return PTR_ERR(tmu->clk);
> }
>
> @@ -665,12 +711,17 @@ static void __exit sh_tmu_exit(void)
> platform_driver_unregister(&sh_tmu_device_driver);
> }
>
> +subsys_initcall(sh_tmu_init);
> +module_exit(sh_tmu_exit);
> +#endif
> +
> #ifdef CONFIG_SUPERH
> +#ifdef CONFIG_SH_DEVICE_TREE
> +TIMER_OF_DECLARE(sh_tmu, "renesas,tmu", sh_tmu_of_register);
Probably this TIMER_OF_DECLARE() should be done unconditionally,
like is done in drivers/clocksource/renesas-ostm.c.
I gave that a try on R-Mobile A1, which also has TMU, but it didn't
seem to work (timer not firing?). So I suspect there are some missing
clk_enable() calls. In the case of the platform driver, these are
handled using pm_runtime_get_sync().
> +#else
> sh_early_platform_init("earlytimer", &sh_tmu_device_driver);
> #endif
> -
> -subsys_initcall(sh_tmu_init);
> -module_exit(sh_tmu_exit);
> +#endif
>
> MODULE_AUTHOR("Magnus Damm");
> MODULE_DESCRIPTION("SuperH TMU Timer Driver");
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Powered by blists - more mailing lists