Hi Thierry,
Le ven. 8 mars 2019 à 7:22, Thierry Reding <thierry.reding@xxxxxxxxx>
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@xxxxxxxxx>
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
