Re: [PATCH v10 04/27] clocksource: Add a new timer-ingenic driver

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, 11 Mar 2019 17:52:07 -0300	[thread overview]
Message-ID: <1552337527.24876.0@crapouillou.net> (raw)
In-Reply-To: <20190308102225.GC22655@ulmo>

Hi Thierry,

Le ven. 8 mars 2019 à 7:22, Thierry Reding <thierry.reding@gmail.com> 
a écrit :
> On Mon, Mar 04, 2019 at 07:13:05PM +0100, Paul Cercueil wrote:
>>  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
> [...]
>>  > >  +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.
> 
> Is the path through the regmap really significantly slower than the
> direct register access? Anyway, if you need the global anyway, might 
> as
> well use it.

About 10% slower.

> [...]
>>  > >  +	// 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.
> 
> Yeah, I suppose that would be possible. I'll defer to Daniel and 
> Thomas
> on this. They may not consider this important enough.
> 
>>  > >  +
>>  > >  +	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.
> 
> I'm not sure if the driver core is ready at this point. It's likely it
> isn't, otherwise there'd be no need for TIMER_OF_DECLARE(), really. I
> also vaguely recall looking into this a few years ago because of some
> similar work I was but I eventually gave up because I couldn't find a
> way that would allow both the code to execute early enough and still
> use the regular driver model.
> 
> Platform device become available at arch_initcall_sync (3s), while the
> TIMER_OF_DECLARE code runs way earlier than any of the initcalls, so I
> don't think there's a way to do it. Unless perhaps if the timers can
> be initialized later. I'm not sure if that's possible, and might not 
> be
> worth it just for the sake of being pedantic.
> 
> Thierry

I think for the time being I can't really go around using the global 
variables.
Hopefully that's something that can be fixed in the future.

-Paul