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.

[...]
> > >  +	// 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