Messages in this thread | | | Date | Mon, 11 Mar 2019 17:52:07 -0300 | From | Paul Cercueil <> | Subject | Re: [PATCH v10 04/27] clocksource: Add a new timer-ingenic driver |
| |
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
| |