LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Nicolas Ferre <nicolas.ferre@microchip.com>,
	Boris Brezillon <boris.brezillon@bootlin.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 0/6] clocksource: rework Atmel TCB timer driver
Date: Thu, 26 Apr 2018 20:52:33 +0200	[thread overview]
Message-ID: <20180426185233.GU4813@piout.net> (raw)
In-Reply-To: <20180426164623.tlv6kz5yopk6voxx@breakpoint.cc>

On 26/04/2018 18:46:23+0200, Sebastian Andrzej Siewior wrote:
> On 2018-04-18 12:51:37 [+0200], Alexandre Belloni wrote:
> > Hi,
> 
> Hi,
> 
> please keep on Cc if you intend to repost this.
> 

Sure, I'll do.

> > This series gets back on the TCB drivers rework. It introduces a new driver to
> > handle the clocksource and clockevent devices.
> 
> So you don't want the old thing we have in -RT. I remember you (the
> atmel folks) wanted something different the last time I posted the
> patches.
> 

I don't remember you posted anything for the TCB. Wasn't it focused on
getting rid of the PIT irq?

As said below, this is solving multiple issues, including the one for
SoCs that don't have the PIT.

> > As a reminder, this is necessary because:
> >  - the current tcb_clksrc driver is probed too late to be able to be used at
> >    boot and we now have SoCs that don't have a PIT. They currently are not able
> >    to boot a mainline kernel.
> >  - using the PIT doesn't work well with preempt-rt because its interrupt is
> >    shared (in particular with the UART and their interrupt flags are
> >    incompatible)
> 
> If you could get rid of this:
> | clocksource: Switched to clocksource timer@f0010000:0
> | BUG: sleeping function called from invalid context at kernel/locking/rtmutex.c:974
> | in_atomic(): 1, irqs_disabled(): 128, pid: 1, name: swapper
> | Preemption disabled at:
> | [<c014f7ac>] __handle_domain_irq+0x40/0xdc
> | Hardware name: Atmel SAMA5
> | [<c010e14c>] (unwind_backtrace) from [<c010bd74>] (show_stack+0x10/0x14)
> | [<c010bd74>] (show_stack) from [<c013b314>] (___might_sleep+0x16c/0x1c0)
> | [<c013b314>] (___might_sleep) from [<c06696f0>] (rt_spin_lock+0x40/0x80)
> | [<c06696f0>] (rt_spin_lock) from [<c046e030>] (clk_enable_lock+0x30/0xe4)
> | [<c046e030>] (clk_enable_lock) from [<c046ea4c>] (clk_core_disable_lock+0xc/0x1c)
> | [<c046ea4c>] (clk_core_disable_lock) from [<c059d07c>] (tc_clkevt2_shutdown+0x64/0x6c)
> | [<c059d07c>] (tc_clkevt2_shutdown) from [<c059d09c>] (tc_clkevt2_set_oneshot+0x18/0x78)
> | [<c059d09c>] (tc_clkevt2_set_oneshot) from [<c0172a00>] (clockevents_switch_state+0xb8/0x130)
> | [<c0172a00>] (clockevents_switch_state) from [<c0173a60>] (tick_switch_to_oneshot+0x48/0xb8)
> | [<c0173a60>] (tick_switch_to_oneshot) from [<c0165a0c>] (hrtimer_run_queues+0xf0/0x15c)
> | [<c0165a0c>] (hrtimer_run_queues) from [<c0164010>] (run_local_timers+0x8/0x38)
> | [<c0164010>] (run_local_timers) from [<c0164070>] (update_process_times+0x30/0x60)
> | [<c0164070>] (update_process_times) from [<c0173008>] (tick_handle_periodic+0x1c/0x90)
> | [<c0173008>] (tick_handle_periodic) from [<c059c954>] (tc_clkevt2_irq+0x38/0x48)
> | [<c059c954>] (tc_clkevt2_irq) from [<c014fe6c>] (__handle_irq_event_percpu+0x7c/0x28c)
> 
> that would be so awesome.
> 

Ok, if I understand correctly tc_clkevt2_set_oneshot is called from
atomic context so it shouldn't call tc_clkevt2_shutdown.

Back in 2016, tglx said:
"There was an earlier discussion about the clk stuff. Actually the lock
protecting disable/enable should be made raw, but there was some crap going on
in some of the clk callbacks which made that impossible. We realy should
revisit this."

Anyway, I'll try to avoid disabling the clock just to reenable it
afterwards.

(Actually, I've just checked before sending and I've found your patch,
I'll do something similar)

> >  - the current solution is wasting some TCB channels
> > 
> > The plan is to get this driver upstream, then convert the TCB PWM driver to be
> > able to get rid of the tcb_clksrc driver along with atmel_tclib.
> 
> The config options are now less than optimal (for me at least). On
> oldconfig it asks you for PIT and I say selected no because I wanted the
> new one. So I end up with nothing.
> Not sure you want do something about it…
> 

I don't think you ending up with nothing but probably you removed the
PIT and kept ATMEL_TCLIB which is the combination that is really
difficult to protect from. I don't think it is worth the added Kconfig
complexity.

> Is the resolution more or the same compared what we have in -RT? On an
> idle system this clocks goes up to 180us/ 190us while the old clock
> started at 160us and moved to around 180us after one hackbench
> invocation. This could be nothing, it could be a lower frequency of the
> clockevents device.
> 

The driver shouldn't change anything on that side.

> If you intend to stick with this driver then I would replace the current
> hack in -RT with this series.
> 

That is one of the goal, yes.

The remaining one would be "clocksource: TCLIB: Allow higher clock rates
for clock events"

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

  reply	other threads:[~2018-04-26 18:52 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-18 10:51 Alexandre Belloni
2018-04-18 10:51 ` [PATCH v4 1/6] ARM: at91: add TCB registers definitions Alexandre Belloni
2018-04-26 16:52   ` Sebastian Andrzej Siewior
2018-04-26 17:48     ` Alexandre Belloni
2018-04-18 10:51 ` [PATCH v4 2/6] clocksource/drivers: Add a new driver for the Atmel ARM TC blocks Alexandre Belloni
2018-04-18 10:51 ` [PATCH v4 3/6] clocksource/drivers: atmel-pit: make option silent Alexandre Belloni
2018-04-18 10:51 ` [PATCH v4 4/6] ARM: at91: Implement clocksource selection Alexandre Belloni
2018-04-18 10:51 ` [PATCH v4 5/6] ARM: configs: at91: use new TCB timer driver Alexandre Belloni
2018-04-18 10:51 ` [PATCH v4 6/6] ARM: configs: at91: unselect PIT Alexandre Belloni
2018-04-26 16:46 ` [PATCH v4 0/6] clocksource: rework Atmel TCB timer driver Sebastian Andrzej Siewior
2018-04-26 18:52   ` Alexandre Belloni [this message]
2018-05-02 13:34     ` Sebastian Andrzej Siewior
2018-05-02 18:10       ` Alexandre Belloni
2018-05-15 17:26         ` Sebastian Andrzej Siewior

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180426185233.GU4813@piout.net \
    --to=alexandre.belloni@bootlin.com \
    --cc=bigeasy@linutronix.de \
    --cc=boris.brezillon@bootlin.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nicolas.ferre@microchip.com \
    --cc=tglx@linutronix.de \
    --subject='Re: [PATCH v4 0/6] clocksource: rework Atmel TCB timer driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).