lkml.org 
[lkml]   [2019]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2019-03-11 21:53    [W:0.081 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site