RE: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver

From: Chris Brandt <Chris.Brandt@renesas.com>
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Simon Horman <horms@verge.net.au>,
	Magnus Damm <magnus.damm@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	Thomas Gleixner <tglx@linutronix.de>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH v3 2/2] clocksource: Add renesas-ostm timer driver
Date: Tue, 24 Jan 2017 04:45:47 +0000	[thread overview]
Message-ID: <SG2PR06MB11657AE9653A4B66B1EA02718A750@SG2PR06MB1165.apcprd06.prod.outlook.com> (raw)
In-Reply-To: <20170123175210.GH2166@mai>

Hello Daniel,

Thank you for the review.
Your suggestions were very helpful to me.

I basically made all the changes, but I had some questions on
the Kconfig and multi-line commenting.

On Monday, January 23, 2017, Daniel Lezcano wrote:
> > This patch adds a OSTM driver for the Renesas architecture.
> 
> As it is a new driver, please give technical details for the log.

OK.

> Replace ioread/write by readl/writel in the code.

OK.

> > @@ -467,6 +470,15 @@ config SH_TIMER_MTU2
> >  	  Timer Pulse Unit 2 (MTU2) hardware available on SoCs from Renesas.
> >  	  This hardware comes with 16 bit-timer registers.
> >
> > +config RENESAS_OSTM
> > +	bool "Renesas OSTM timer driver" if COMPILE_TEST
> > +	depends on GENERIC_CLOCKEVENTS
> > +	select CLKSRC_MMIO
> > +	default SYS_SUPPORTS_RENESAS_OSTM
> 
> - default SYS_SUPPORTS_RENESAS_OSTM
> 
> Except I missing something, I don' the point of having this intermediate
> config option.

I was basically following what all the other clocksource drivers do.
Selecting an ARCH will then set SYS_SUPPORTS_xxTIMERxx which will then
force xxTIMERxx=y.
But, if "COMPILE_TEST" is set, you can choose what clocksource timers
you want to build in.

Is your suggestion to do away with the COMPILE_TEST ability and
just force RENESAS_OSTM=y if ARCH_R7S72100 is selected?

> > +#include <linux/module.h>
> 
> Please remove everything module related here and below. This driver is not
> supposed to be removed and it is compiled in with the Kconfig option.

Good point. I will remove.

I guess if you can't compile it as a module, you don't need things like
'MODULE_LICENSE'.

> > +struct ostm_device {
> > +	struct platform_device *pdev;
> > +	int irq;
> > +	struct clk *clk;
> > +	unsigned long rate;
> > +	void __iomem *base;
> > +	unsigned long ticks_per_jiffy;
> > +	struct clock_event_device ced;
> > +};
> 
> You can probably reduce the size of this structure by removing some fields
> which are used only at init time.

Looks like I can get rid of 'clk', 'irq' and 'rate'.
Thanks.

> > +static int __init ostm_init_clksrc(struct ostm_device *ostm) {
> > +	int ret;
> > +
> > +	/* irq not used (clock sources don't use interrupts) */
> > +
> > +	/* stop counter */
> 
> One line comment is usually in the network code. Please use multiline.
> 
> /*
>  * Bla bla
>  */

I'm confused. Do you mean:

A) use multiline formatting for every single-line comment throughout
   the file?

B) use multiline for any place where you have 2 or more single-line
   comments back to back?

> > +	iowrite8(TT, ostm->base + OSTM_TT);
> > +	while (ioread8(ostm->base + OSTM_TE) & TE)
> > +		;
> 
> Comment here for this dangerous loop. Explain why we can't be stuck here.

OK, I'll add something in as it's pretty safe.

> 
> > +
> > +	/* setup as freerun */
> > +	iowrite32(0, ostm->base + OSTM_CMP);
> > +	iowrite8(CTL_FREERUN, ostm->base + OSTM_CTL);
> > +	iowrite8(TS, ostm->base + OSTM_TS);
> > +
> > +	/* register */
> > +	ret = clocksource_mmio_init(ostm->base + OSTM_CNT,
> > +			"ostm", ostm->rate,
> > +			300, 32, clocksource_mmio_readl_up);
> > +
> > +	return ret;
> 
> return clocksource_mmio_init(...);

OK.

> > +static int __init ostm_init_sched_clock(struct ostm_device *ostm) {
> > +	unsigned long flags;
> > +
> > +	system_clock = ostm->base + OSTM_CNT;
> > +	local_irq_save(flags);
> > +	local_irq_disable();
> 
> 1. local_irq_disable() is not needed, local_irq_save() saves the irq flags
> and disables the irq.
> 
> 2. Why do you need to disable the irq here ?

OK, I see that none of the other drivers are disabling irq, so I'll
take that code out.
Thanks.

> > +static int ostm_clock_event_next(unsigned long delta,
> > +				     struct clock_event_device *ced) {
> > +	struct ostm_device *ostm = ced_to_ostm(ced);
> > +
> > +	WARN_ON(!clockevent_state_oneshot(ced));
> 
> Pointless check. It is up to the time framework to handle that and that is
> the case.

OK. I saw other drivers doing it so I thought it was my responsibility.
I'll take that lineout.

> > +static int __init ostm_init_clkevt(struct ostm_device *ostm) {
> > +	struct clock_event_device *ced = &ostm->ced;
> > +	int ret = -ENXIO;
> > +
> > +	ret = request_irq(ostm->irq, ostm_timer_interrupt,
> > +			  IRQF_TIMER | IRQF_IRQPOLL | IRQF_NOBALANCING,
> > +			  dev_name(&ostm->pdev->dev), ostm);
> 
> 	devm_request_irq
> 
> Are you sure the IRQF_NOBALANCING flag should be used ? By default the
> interrupt is pinned to cpu0 below. If this timer is used as a broadcast
> timer for a system with deep idle, you may want to add the DYNIRQ flag
> feature to improve the wakeups on the system.

OK, thank you. After some reading, I understand IRQF_NOBALANCING better.

You're point is valid: Regardless of the fact that the SoCs I'll be using
this on are all single core, like you said below the cpu mask is set to cpu0.

I will remove IRQF_NOBALANCING.

> > +static int __init ostm_probe(struct platform_device *pdev) {
> > +	struct ostm_device *ostm;
> > +	struct resource *res;
> > +	int ret = -EFAULT;
> > +
> > +	if (!is_early_platform_device(pdev)) {
> > +		pm_runtime_set_active(&pdev->dev);
> > +		pm_runtime_enable(&pdev->dev);
> > +	}
> 
> Can you clarify why the 'early' is needed here ?

Actually, it means nothing.

I was using sh_mtu2.c and sh_cmt.c as reference and they were registering
an "earlytimer" which made me think the kernel would probe this driver
early in boot....but....now I see that is just a SH4 kernel specific thing.

So, I will remove all the early platform reference stuff.

Of course I still have the question of how are you supposed to get a
clocksource up and running early in boot. (but I'll figure that out later).

> I don't see the pm_runtime_get/pm_runtime_put in the corresponding
> function.
> 
> What is the benefit of using pm_runtime in this driver ? Isn't the timer
> supposed to be in an always-on power domain ?

When you register a clocksource, you can specify a .enable and .disable
callback where you can do runtime_pm. However...

A. I'm using clocksource_mmio_init, so no enable/dispable option is
   possible.
B. I just found out today that runtime pm is not going to work well on the
   SoC this driver was intended for, so it pointless anyway.

I'll remove all the runtime pm stuff.
Thank you for point this out.

> > +	ostm = platform_get_drvdata(pdev);
> > +	if (ostm) {
> > +		dev_info(&pdev->dev, "kept as earlytimer\n");
> > +		ret = 0;
> > +		goto out;
> > +	}
> > +
> > +	ostm = kzalloc(sizeof(*ostm), GFP_KERNEL);
> 
> 	devm_kzalloc ?

OK, thank you.
Means I can also get rid of '#include <linux/slab.h>'

> > +	if (!ostm) {
> > +		dev_err(&pdev->dev, "failed to allocate memory\n");
> 
> A memory failure allocation always triggers a dumpstack (IIRC), so this
> error message is pointless.

OK, removed.

> > +		return -ENOMEM;
> > +	}
> > +
> > +	ostm->pdev = pdev;
> > +	platform_set_drvdata(ostm->pdev, ostm);
> > +
> > +	res = platform_get_resource(ostm->pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&ostm->pdev->dev, "failed to get I/O memory\n");
> > +		goto err;
> > +	}
> > +
> > +	ostm->base = ioremap_nocache(res->start, resource_size(res));
> 
> 	devm_ioremap_nocache ?

OK, thank you.
Just need to also add '#include <linux/io.h>'

(looks like devm_ioremap and devm_ioremap_nocache is not as popular as
devm_kzalloc for some reason)

> > +	/* First probed device will be used as system clocksource */
> > +	if (!system_clock) {
> > +		/* use as clocksource */
> > +		ret = ostm_init_clksrc(ostm);
> > +
> > +		/* use as system scheduling clock */
> > +		if (!ret)
> > +			ret = ostm_init_sched_clock(ostm);
> > +
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "failed to use as sched_clock\n");
> > +			system_clock = (void *)-1; /* prevent future attempts */
> > +			ret = 0;	/* still works as clocksource */
> > +		}
> 
> This error code check is unnecessary complex. ostm_init_sched_clock always
> return zero.

Good point.
And since sched_clock_register() always returns void, ostm_init_sched_clock
might was well return void as well.

> > +err:
> > +	if (ret) {
> > +		if (ostm->clk)
> > +			clk_disable_unprepare(ostm->clk);
> > +		if (ostm->base)
> > +			iounmap(ostm->base);
> > +		kfree(ostm);
> > +		platform_set_drvdata(pdev, NULL);
> 
> As iomap, kzalloc were done with the devm, then the rollback will be
> handled with the device framework.

I didn't realize that. Thank you.

> > +static int __init ostm_init(void)
> > +{
> > +	return platform_driver_register(&ostm_timer);
> > +}
> > +
> > +static void __exit ostm_exit(void)
> > +{
> > +	platform_driver_unregister(&ostm_timer);
> > +}
> > +
> > +early_platform_init("earlytimer", &ostm_timer);
> > +subsys_initcall(ostm_init); module_exit(ostm_exit);
> > +
> > +MODULE_AUTHOR("Chris Brandt");
> > +MODULE_DESCRIPTION("Renesas OSTM Timer Driver"); MODULE_LICENSE("GPL
> > +v2");
> 
> Maybe you can try with builtin_platform ?

Good idea. But, now I get a "Section mismatch" during link time so I'll
have to figure out why that is.

Thank you,
Chris