Messages in this thread | | | Subject | Re: [PATCH v3 04/25] clocksource: Add Owl timer | From | Andreas Färber <> | Date | Tue, 28 Feb 2017 19:01:23 +0100 |
| |
Am 28.02.2017 um 18:39 schrieb Daniel Lezcano: > On Tue, Feb 28, 2017 at 06:08:06PM +0100, Andreas Färber wrote: >>> Instead of computing again and again the base, why not just precompute: >>> >>> owl_clksrc_base = owl_timer_base + owl_timer_info->timer_offset[OWL_TIMER0] >>> owl_clkevt_base = owl_timer_base + owl_timer_info->timer_offset[OWL_TIMER1] >>> >>> at init time. >>> >>> And use these variables directly in the functions. >> >> Either that, or revert to previous simpler behavior... > > Not sure to get what the 'previous simpler behavior' is,
v2. :)
> but until it does not > recompute the offset each time, I'm fine with that.
>>>> +} >>>> + >>>> +static inline void owl_timer_reset(unsigned index) >>>> +{ >>>> + void __iomem *base; >>>> + >>>> + base = owl_timer_get_base(index); >>>> + if (!base) >>>> + return; >>> >>> Same here, this test is pointless. >> >> Seems like you didn't look at the following patch yet. It sets two S500 >> offsets as -1, i.e. non-existant, which then results in NULL here. > > May be I missed something, but so far, the base addresses must be setup before > reset is called, no?
They are known in advance, yes. Where/how we set them up is the culprit.
>>> static inline int owl_timer_set_state_disable(struct clock_event_device *evt) >>> { >>> return writel(0, owl_clkevt_base + OWL_Tx_CTL); >>> } >> >> That I don't like. Disabling is just setting a bit. We save a readl by >> just writing where we know it's safe. An API like this is not safe. > > I don't get the point. Writing this simple function has the benefit to give the > reader the information about the disabling register. Even if it does make sense > for you, for me it has its purpose when I try to factor out different drivers > code.
I mean a proper _disable() function would need to do:
val = readl() val &= ~bit; writel(val)
Not just writel(0), overwriting any other bits. Therefore an inline write would be faster - your concern elsewhere. I'll happily implement the proper API if you prefer.
>>>> +static int owl_timer_set_state_shutdown(struct clock_event_device *evt) >>>> +{ >>>> + writel(0, owl_timer_get_base(0) + OWL_Tx_CTL); >>> >>> return owl_timer_set_state_disable(evt); >>> >>>> + >>>> + return 0; >>>> +}
>>>> +static int owl_timer_set_next_event(unsigned long evt, >>>> + struct clock_event_device *ev) >>>> +{ >>>> + void __iomem *base = owl_timer_get_base(1); >>>> + >>>> + writel(0, base + OWL_Tx_CTL); >>>> + >>>> + writel(0, base + OWL_Tx_VAL); >>> >> >> Are you suggesting a while line here? The point was disable first, then >> initialize (2x), then activate. Maybe add comments instead? > > I meant, base + OWL_Tx_CTL and base + OWL_Tx_VAL are set to zero since the > beginning. If their values do not change, it is not necessary to set their > values to zero again.
This is a callback, which I thought is re-entrant. VAL changes when the timer is running, and CTL changes every time we enable the timer. We could call _reset() here, but then we would be initializing CMP twice, which again would be less performant then just setting the registers to their final values directly.
>>>> + writel(evt, base + OWL_Tx_CMP); >>>> + >>>> + writel(OWL_Tx_CTL_EN | OWL_Tx_CTL_INTEN, base + OWL_Tx_CTL); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static struct clock_event_device owl_clockevent = { >>>> + .name = "owl_tick", >>>> + .rating = 200, >>>> + .features = CLOCK_EVT_FEAT_ONESHOT | >>>> + CLOCK_EVT_FEAT_DYNIRQ, >>>> + .set_state_shutdown = owl_timer_set_state_shutdown, >>>> + .set_state_oneshot = owl_timer_set_state_oneshot, >>>> + .tick_resume = owl_timer_tick_resume, >>>> + .set_next_event = owl_timer_set_next_event, >>>> +}; >>>> + >>>> +static irqreturn_t owl_timer1_interrupt(int irq, void *dev_id) >>>> +{ >>>> + struct clock_event_device *evt = (struct clock_event_device *)dev_id; >>>> + >>>> + writel(OWL_Tx_CTL_PD, owl_timer_get_base(1) + OWL_Tx_CTL); >>>> + >>>> + evt->event_handler(evt); >> >> Is there any guideline as to whether to clear such flag before or after? > > Mmh, good question. I'm not sure it makes a different. > >>>> + >>>> + return IRQ_HANDLED; >>>> +}
Thanks, Andreas
-- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
| |