Fix preempt-rt on AT91

Fix preempt-rt on AT91

[Alexandre Belloni]

Hi Sebastian, Thomas,

Preemp-rt on at91 has multiple issues:
 1/ After apply the preempt-rt patch, the kernel doesn't build anymore,
 arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch is not correct
 anymore. Can you squash the following patch to solve this build issue?

 2/ This approach actually has more issues, in particular, request_irq() must
 not be calls from interrupt disabled context. That only works because
 might_sleep() is not active during early boot.
 For more information, see the following discussion:
 http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357607.html

 However, we can't remove those patches now because else we would suffer from
 another issue:
 The timer interrupt is shared with other devices, in particular the debug tty,
 the rtc dans the pmc. When using preempt-rt, those interrupts become threaded
 interrupts. But, the pit interrupt is requested with IRQF_TIMER and so has
 IRQF_NO_THREAD. If the pit interrupt is not freed at early boot, all the other
 devices will fail requesting their interrupt, for example:

genirq: Flags mismatch irq 16. 00042080 (ttyS0) vs. 00015280 (at91_tick)
atmel_usart ffffee00.serial: atmel_startup - Can't get irq

 I'd say that the proper solution would still be to implement the virtual
 irqchip because this would still hit people not wanting to use the TCB as
 their clock source.

 3/ Finally, the kernel will crash when initializing the PMC driver. This is
 solved by this series that will hopefully land in the mainline:
   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/390954.html


Alexandre Belloni (1):
  fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

 drivers/clocksource/timer-atmel-pit.c | 68 +++++++++++++++++------------------
 drivers/clocksource/timer-atmel-st.c  |  8 ++---
 2 files changed, 38 insertions(+), 38 deletions(-)

-- 
2.5.0

Fix preempt-rt on AT91

[Alexandre Belloni]

Hi Sebastian, Thomas,

Preemp-rt on at91 has multiple issues:
 1/ After apply the preempt-rt patch, the kernel doesn't build anymore,
 arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch is not correct
 anymore. Can you squash the following patch to solve this build issue?

 2/ This approach actually has more issues, in particular, request_irq() must
 not be calls from interrupt disabled context. That only works because
 might_sleep() is not active during early boot.
 For more information, see the following discussion:
 http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357607.html

 However, we can't remove those patches now because else we would suffer from
 another issue:
 The timer interrupt is shared with other devices, in particular the debug tty,
 the rtc dans the pmc. When using preempt-rt, those interrupts become threaded
 interrupts. But, the pit interrupt is requested with IRQF_TIMER and so has
 IRQF_NO_THREAD. If the pit interrupt is not freed at early boot, all the other
 devices will fail requesting their interrupt, for example:

genirq: Flags mismatch irq 16. 00042080 (ttyS0) vs. 00015280 (at91_tick)
atmel_usart ffffee00.serial: atmel_startup - Can't get irq

 I'd say that the proper solution would still be to implement the virtual
 irqchip because this would still hit people not wanting to use the TCB as
 their clock source.

 3/ Finally, the kernel will crash when initializing the PMC driver. This is
 solved by this series that will hopefully land in the mainline:
   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/390954.html


Alexandre Belloni (1):
  fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

 drivers/clocksource/timer-atmel-pit.c | 68 +++++++++++++++++------------------
 drivers/clocksource/timer-atmel-st.c  |  8 ++---
 2 files changed, 38 insertions(+), 38 deletions(-)

-- 
2.5.0

[PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Alexandre Belloni]

arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch breaks the
build, fix that.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/clocksource/timer-atmel-pit.c | 68 +++++++++++++++++------------------
 drivers/clocksource/timer-atmel-st.c  |  8 ++---
 2 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/drivers/clocksource/timer-atmel-pit.c b/drivers/clocksource/timer-atmel-pit.c
index 80d74c4adcbe..43b50634d640 100644
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
 
 	/* disable irq, leaving the clocksource active */
 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
-	free_irq(atmel_pit_irq, data);
+	if (!clockevent_state_detached(dev))
+		free_irq(data->irq, data);
 	return 0;
 }
 
 /*
+ * IRQ handler for the timer.
+ */
+static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
+{
+	struct pit_data *data = dev_id;
+
+	/*
+	 * irqs should be disabled here, but as the irq is shared they are only
+	 * guaranteed to be off if the timer irq is registered first.
+	 */
+	WARN_ON_ONCE(!irqs_disabled());
+
+	/* The PIT interrupt may be disabled, and is shared */
+	if (clockevent_state_periodic(&data->clkevt) &&
+	    (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
+		unsigned nr_ticks;
+
+		/* Get number of ticks performed before irq, and ack it */
+		nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
+		do {
+			data->cnt += data->cycle;
+			data->clkevt.event_handler(&data->clkevt);
+			nr_ticks--;
+		} while (nr_ticks);
+
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+/*
  * Clockevent device:  interrupts every 1/HZ (== pit_cycles * MCK/16)
  */
 static int pit_clkevt_set_periodic(struct clock_event_device *dev)
@@ -151,45 +184,12 @@ static void at91sam926x_pit_resume(struct clock_event_device *cedev)
 }
 
 /*
- * IRQ handler for the timer.
- */
-static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
-{
-	struct pit_data *data = dev_id;
-
-	/*
-	 * irqs should be disabled here, but as the irq is shared they are only
-	 * guaranteed to be off if the timer irq is registered first.
-	 */
-	WARN_ON_ONCE(!irqs_disabled());
-
-	/* The PIT interrupt may be disabled, and is shared */
-	if (clockevent_state_periodic(&data->clkevt) &&
-	    (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
-		unsigned nr_ticks;
-
-		/* Get number of ticks performed before irq, and ack it */
-		nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
-		do {
-			data->cnt += data->cycle;
-			data->clkevt.event_handler(&data->clkevt);
-			nr_ticks--;
-		} while (nr_ticks);
-
-		return IRQ_HANDLED;
-	}
-
-	return IRQ_NONE;
-}
-
-/*
  * Set up both clocksource and clockevent support.
  */
 static void __init at91sam926x_pit_common_init(struct pit_data *data)
 {
 	unsigned long	pit_rate;
 	unsigned	bits;
-	int		ret;
 
 	/*
 	 * Use our actual MCK to figure out how many MCK/16 ticks per
diff --git a/drivers/clocksource/timer-atmel-st.c b/drivers/clocksource/timer-atmel-st.c
index ea37afc26e1b..11ce404d0791 100644
--- a/drivers/clocksource/timer-atmel-st.c
+++ b/drivers/clocksource/timer-atmel-st.c
@@ -150,7 +150,7 @@ static int clkevt32k_set_oneshot(struct clock_event_device *dev)
 
 static int clkevt32k_set_periodic(struct clock_event_device *dev)
 {
-	int irq;
+	int ret;
 
 	clkdev32k_disable_and_flush_irq();
 
@@ -229,15 +229,15 @@ static void __init atmel_st_timer_init(struct device_node *node)
 	regmap_read(regmap_st, AT91_ST_SR, &val);
 
 	/* Get the interrupts property */
-	irq  = irq_of_parse_and_map(node, 0);
-	if (!irq)
+	atmel_st_irq  = irq_of_parse_and_map(node, 0);
+	if (!atmel_st_irq)
 		panic(pr_fmt("Unable to get IRQ from DT\n"));
 
 	sclk = of_clk_get(node, 0);
 	if (IS_ERR(sclk))
 		panic(pr_fmt("Unable to get slow clock\n"));
 
-	clk_prepare_enable(sclk);
+	ret = clk_prepare_enable(sclk);
 	if (ret)
 		panic(pr_fmt("Could not enable slow clock\n"));
 
-- 
2.5.0

[PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Alexandre Belloni]

arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch breaks the
build, fix that.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/clocksource/timer-atmel-pit.c | 68 +++++++++++++++++------------------
 drivers/clocksource/timer-atmel-st.c  |  8 ++---
 2 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/drivers/clocksource/timer-atmel-pit.c b/drivers/clocksource/timer-atmel-pit.c
index 80d74c4adcbe..43b50634d640 100644
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
 
 	/* disable irq, leaving the clocksource active */
 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
-	free_irq(atmel_pit_irq, data);
+	if (!clockevent_state_detached(dev))
+		free_irq(data->irq, data);
 	return 0;
 }
 
 /*
+ * IRQ handler for the timer.
+ */
+static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
+{
+	struct pit_data *data = dev_id;
+
+	/*
+	 * irqs should be disabled here, but as the irq is shared they are only
+	 * guaranteed to be off if the timer irq is registered first.
+	 */
+	WARN_ON_ONCE(!irqs_disabled());
+
+	/* The PIT interrupt may be disabled, and is shared */
+	if (clockevent_state_periodic(&data->clkevt) &&
+	    (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
+		unsigned nr_ticks;
+
+		/* Get number of ticks performed before irq, and ack it */
+		nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
+		do {
+			data->cnt += data->cycle;
+			data->clkevt.event_handler(&data->clkevt);
+			nr_ticks--;
+		} while (nr_ticks);
+
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+/*
  * Clockevent device:  interrupts every 1/HZ (== pit_cycles * MCK/16)
  */
 static int pit_clkevt_set_periodic(struct clock_event_device *dev)
@@ -151,45 +184,12 @@ static void at91sam926x_pit_resume(struct clock_event_device *cedev)
 }
 
 /*
- * IRQ handler for the timer.
- */
-static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
-{
-	struct pit_data *data = dev_id;
-
-	/*
-	 * irqs should be disabled here, but as the irq is shared they are only
-	 * guaranteed to be off if the timer irq is registered first.
-	 */
-	WARN_ON_ONCE(!irqs_disabled());
-
-	/* The PIT interrupt may be disabled, and is shared */
-	if (clockevent_state_periodic(&data->clkevt) &&
-	    (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
-		unsigned nr_ticks;
-
-		/* Get number of ticks performed before irq, and ack it */
-		nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
-		do {
-			data->cnt += data->cycle;
-			data->clkevt.event_handler(&data->clkevt);
-			nr_ticks--;
-		} while (nr_ticks);
-
-		return IRQ_HANDLED;
-	}
-
-	return IRQ_NONE;
-}
-
-/*
  * Set up both clocksource and clockevent support.
  */
 static void __init at91sam926x_pit_common_init(struct pit_data *data)
 {
 	unsigned long	pit_rate;
 	unsigned	bits;
-	int		ret;
 
 	/*
 	 * Use our actual MCK to figure out how many MCK/16 ticks per
diff --git a/drivers/clocksource/timer-atmel-st.c b/drivers/clocksource/timer-atmel-st.c
index ea37afc26e1b..11ce404d0791 100644
--- a/drivers/clocksource/timer-atmel-st.c
+++ b/drivers/clocksource/timer-atmel-st.c
@@ -150,7 +150,7 @@ static int clkevt32k_set_oneshot(struct clock_event_device *dev)
 
 static int clkevt32k_set_periodic(struct clock_event_device *dev)
 {
-	int irq;
+	int ret;
 
 	clkdev32k_disable_and_flush_irq();
 
@@ -229,15 +229,15 @@ static void __init atmel_st_timer_init(struct device_node *node)
 	regmap_read(regmap_st, AT91_ST_SR, &val);
 
 	/* Get the interrupts property */
-	irq  = irq_of_parse_and_map(node, 0);
-	if (!irq)
+	atmel_st_irq  = irq_of_parse_and_map(node, 0);
+	if (!atmel_st_irq)
 		panic(pr_fmt("Unable to get IRQ from DT\n"));
 
 	sclk = of_clk_get(node, 0);
 	if (IS_ERR(sclk))
 		panic(pr_fmt("Unable to get slow clock\n"));
 
-	clk_prepare_enable(sclk);
+	ret = clk_prepare_enable(sclk);
 	if (ret)
 		panic(pr_fmt("Could not enable slow clock\n"));
 
-- 
2.5.0

Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Sebastian Andrzej Siewior]

* Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:

>index 80d74c4adcbe..43b50634d640 100644
>--- a/drivers/clocksource/timer-atmel-pit.c
>+++ b/drivers/clocksource/timer-atmel-pit.c
>@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
> 
> 	/* disable irq, leaving the clocksource active */
> 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
>-	free_irq(atmel_pit_irq, data);
>+	if (!clockevent_state_detached(dev))
>+		free_irq(data->irq, data);

I did it in the meantime without clockevent_state_detached(). From what
it looks, it first sets the state and then invokes
pit_clkevt_shutdown(). Any particular reason for this?

> 	return 0;
> }
> 
> /*
>+ * IRQ handler for the timer.
>+ */
>+static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)

this is just here to avoid to forward declaration.
…
>diff --git a/drivers/clocksource/timer-atmel-st.c b/drivers/clocksource/timer-atmel-st.c
>index ea37afc26e1b..11ce404d0791 100644
>--- a/drivers/clocksource/timer-atmel-st.c
>+++ b/drivers/clocksource/timer-atmel-st.c
>@@ -229,15 +229,15 @@ static void __init atmel_st_timer_init(struct device_node *node)
> 	regmap_read(regmap_st, AT91_ST_SR, &val);
> 
> 	/* Get the interrupts property */
>-	irq  = irq_of_parse_and_map(node, 0);
>-	if (!irq)
>+	atmel_st_irq  = irq_of_parse_and_map(node, 0);
>+	if (!atmel_st_irq)
> 		panic(pr_fmt("Unable to get IRQ from DT\n"));
> 
> 	sclk = of_clk_get(node, 0);
> 	if (IS_ERR(sclk))
> 		panic(pr_fmt("Unable to get slow clock\n"));
> 
>-	clk_prepare_enable(sclk);
>+	ret = clk_prepare_enable(sclk);
this piece applies to upstream v4.4.

> 	if (ret)
> 		panic(pr_fmt("Could not enable slow clock\n"));
> 

Sebastian

Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Sebastian Andrzej Siewior]

* Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:

>index 80d74c4adcbe..43b50634d640 100644
>--- a/drivers/clocksource/timer-atmel-pit.c
>+++ b/drivers/clocksource/timer-atmel-pit.c
>@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
> 
> 	/* disable irq, leaving the clocksource active */
> 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
>-	free_irq(atmel_pit_irq, data);
>+	if (!clockevent_state_detached(dev))
>+		free_irq(data->irq, data);

I did it in the meantime without clockevent_state_detached(). From what
it looks, it first sets the state and then invokes
pit_clkevt_shutdown(). Any particular reason for this?

> 	return 0;
> }
> 
> /*
>+ * IRQ handler for the timer.
>+ */
>+static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)

this is just here to avoid to forward declaration.
…
>diff --git a/drivers/clocksource/timer-atmel-st.c b/drivers/clocksource/timer-atmel-st.c
>index ea37afc26e1b..11ce404d0791 100644
>--- a/drivers/clocksource/timer-atmel-st.c
>+++ b/drivers/clocksource/timer-atmel-st.c
>@@ -229,15 +229,15 @@ static void __init atmel_st_timer_init(struct device_node *node)
> 	regmap_read(regmap_st, AT91_ST_SR, &val);
> 
> 	/* Get the interrupts property */
>-	irq  = irq_of_parse_and_map(node, 0);
>-	if (!irq)
>+	atmel_st_irq  = irq_of_parse_and_map(node, 0);
>+	if (!atmel_st_irq)
> 		panic(pr_fmt("Unable to get IRQ from DT\n"));
> 
> 	sclk = of_clk_get(node, 0);
> 	if (IS_ERR(sclk))
> 		panic(pr_fmt("Unable to get slow clock\n"));
> 
>-	clk_prepare_enable(sclk);
>+	ret = clk_prepare_enable(sclk);
this piece applies to upstream v4.4.

> 	if (ret)
> 		panic(pr_fmt("Could not enable slow clock\n"));
> 

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Sebastian Andrzej Siewior]

* Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:

>index 80d74c4adcbe..43b50634d640 100644
>--- a/drivers/clocksource/timer-atmel-pit.c
>+++ b/drivers/clocksource/timer-atmel-pit.c
>@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
> 
> 	/* disable irq, leaving the clocksource active */
> 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
>-	free_irq(atmel_pit_irq, data);
>+	if (!clockevent_state_detached(dev))
>+		free_irq(data->irq, data);

I did it in the meantime without clockevent_state_detached(). From what
it looks, it first sets the state and then invokes
pit_clkevt_shutdown(). Any particular reason for this?

> 	return 0;
> }
> 
> /*
>+ * IRQ handler for the timer.
>+ */
>+static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)

this is just here to avoid to forward declaration.
?
>diff --git a/drivers/clocksource/timer-atmel-st.c b/drivers/clocksource/timer-atmel-st.c
>index ea37afc26e1b..11ce404d0791 100644
>--- a/drivers/clocksource/timer-atmel-st.c
>+++ b/drivers/clocksource/timer-atmel-st.c
>@@ -229,15 +229,15 @@ static void __init atmel_st_timer_init(struct device_node *node)
> 	regmap_read(regmap_st, AT91_ST_SR, &val);
> 
> 	/* Get the interrupts property */
>-	irq  = irq_of_parse_and_map(node, 0);
>-	if (!irq)
>+	atmel_st_irq  = irq_of_parse_and_map(node, 0);
>+	if (!atmel_st_irq)
> 		panic(pr_fmt("Unable to get IRQ from DT\n"));
> 
> 	sclk = of_clk_get(node, 0);
> 	if (IS_ERR(sclk))
> 		panic(pr_fmt("Unable to get slow clock\n"));
> 
>-	clk_prepare_enable(sclk);
>+	ret = clk_prepare_enable(sclk);
this piece applies to upstream v4.4.

> 	if (ret)
> 		panic(pr_fmt("Could not enable slow clock\n"));
> 

Sebastian

Re: Fix preempt-rt on AT91

[Sebastian Andrzej Siewior]

* Alexandre Belloni | 2016-01-17 03:23:13 [+0100]:

>Hi Sebastian, Thomas,
>
Hi Alexandre,

> 1/ After apply the preempt-rt patch, the kernel doesn't build anymore,
> arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch is not correct
> anymore. Can you squash the following patch to solve this build issue?

I folded most of the changes and should be part of -rt3.

> 2/ This approach actually has more issues, in particular, request_irq() must
> not be calls from interrupt disabled context. That only works because
> might_sleep() is not active during early boot.
> For more information, see the following discussion:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357607.html
>
> However, we can't remove those patches now because else we would suffer from
> another issue:
> The timer interrupt is shared with other devices, in particular the debug tty,
> the rtc dans the pmc. When using preempt-rt, those interrupts become threaded
> interrupts. But, the pit interrupt is requested with IRQF_TIMER and so has
> IRQF_NO_THREAD. If the pit interrupt is not freed at early boot, all the other
> devices will fail requesting their interrupt, for example:
>
>genirq: Flags mismatch irq 16. 00042080 (ttyS0) vs. 00015280 (at91_tick)
>atmel_usart ffffee00.serial: atmel_startup - Can't get irq

I assumed the pit is just temporary here for the bootstap. The pit irq is
removed and the system switches to the TCB clocksource which runs at a
higher rate.

> I'd say that the proper solution would still be to implement the virtual
> irqchip because this would still hit people not wanting to use the TCB as
> their clock source.

why wouldn't people not want that?
For a virtual irqchip you would need a mask/unmask register in order to
individual disable/enable the irq and you need something to figure out
which one of the three is active. You don't have all those things, do
you?

> 3/ Finally, the kernel will crash when initializing the PMC driver. This is
> solved by this series that will hopefully land in the mainline:
>   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/390954.html

This is new, isn't it? So the series in currently in v3 and people seem
happy with it. Please poke me once with the commit ids once it is merged
on its way upstream and I will try cherry-pick them from next or so.

All in all, care to forwarded the working pieces from -RT patch set
upstream? I problem I have here is mostly that I can't the patches on
actual hardware. Disabling the PIT and running on the other clocksource
isn't that -RT specific after all :)

Sebastian

Fix preempt-rt on AT91

[Sebastian Andrzej Siewior]

* Alexandre Belloni | 2016-01-17 03:23:13 [+0100]:

>Hi Sebastian, Thomas,
>
Hi Alexandre,

> 1/ After apply the preempt-rt patch, the kernel doesn't build anymore,
> arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch is not correct
> anymore. Can you squash the following patch to solve this build issue?

I folded most of the changes and should be part of -rt3.

> 2/ This approach actually has more issues, in particular, request_irq() must
> not be calls from interrupt disabled context. That only works because
> might_sleep() is not active during early boot.
> For more information, see the following discussion:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357607.html
>
> However, we can't remove those patches now because else we would suffer from
> another issue:
> The timer interrupt is shared with other devices, in particular the debug tty,
> the rtc dans the pmc. When using preempt-rt, those interrupts become threaded
> interrupts. But, the pit interrupt is requested with IRQF_TIMER and so has
> IRQF_NO_THREAD. If the pit interrupt is not freed at early boot, all the other
> devices will fail requesting their interrupt, for example:
>
>genirq: Flags mismatch irq 16. 00042080 (ttyS0) vs. 00015280 (at91_tick)
>atmel_usart ffffee00.serial: atmel_startup - Can't get irq

I assumed the pit is just temporary here for the bootstap. The pit irq is
removed and the system switches to the TCB clocksource which runs at a
higher rate.

> I'd say that the proper solution would still be to implement the virtual
> irqchip because this would still hit people not wanting to use the TCB as
> their clock source.

why wouldn't people not want that?
For a virtual irqchip you would need a mask/unmask register in order to
individual disable/enable the irq and you need something to figure out
which one of the three is active. You don't have all those things, do
you?

> 3/ Finally, the kernel will crash when initializing the PMC driver. This is
> solved by this series that will hopefully land in the mainline:
>   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/390954.html

This is new, isn't it? So the series in currently in v3 and people seem
happy with it. Please poke me once with the commit ids once it is merged
on its way upstream and I will try cherry-pick them from next or so.

All in all, care to forwarded the working pieces from -RT patch set
upstream? I problem I have here is mostly that I can't the patches on
actual hardware. Disabling the PIT and running on the other clocksource
isn't that -RT specific after all :)

Sebastian

Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Alexandre Belloni]

On 18/01/2016 at 18:25:22 +0100, Sebastian Andrzej Siewior wrote :
> * Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:
> 
> >index 80d74c4adcbe..43b50634d640 100644
> >--- a/drivers/clocksource/timer-atmel-pit.c
> >+++ b/drivers/clocksource/timer-atmel-pit.c
> >@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
> > 
> > 	/* disable irq, leaving the clocksource active */
> > 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
> >-	free_irq(atmel_pit_irq, data);
> >+	if (!clockevent_state_detached(dev))
> >+		free_irq(data->irq, data);
> 
> I did it in the meantime without clockevent_state_detached(). From what
> it looks, it first sets the state and then invokes
> pit_clkevt_shutdown(). Any particular reason for this?
> 

Yeah, I forgot to mention that. Freeing the irq unconditionally
results in:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at kernel/irq/manage.c:1541
__free_irq+0xb4/0x2c8()
Trying to free already-free IRQ 16
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.4.0-rt2+ #31
Hardware name: Atmel SAMA5
[<c0016344>] (unwind_backtrace) from [<c0012d7c>] (show_stack+0x10/0x14)
[<c0012d7c>] (show_stack) from [<c021639c>] (dump_stack+0x80/0x94)
[<c021639c>] (dump_stack) from [<c001f528>] (warn_slowpath_common+0x80/0xb0)
[<c001f528>] (warn_slowpath_common) from [<c001f588>] (warn_slowpath_fmt+0x30/0x40)
[<c001f588>] (warn_slowpath_fmt) from [<c00615e0>] (__free_irq+0xb4/0x2c8)
[<c00615e0>] (__free_irq) from [<c0061878>] (free_irq+0x3c/0x70)
[<c0061878>] (free_irq) from [<c0391ba8>] (pit_clkevt_shutdown+0x24/0x2c)
[<c0391ba8>] (pit_clkevt_shutdown) from [<c007d9a0>] (clockevents_switch_state+0x60/0x130)
[<c007d9a0>] (clockevents_switch_state) from [<c007dda4>] (clockevents_exchange_device+0x78/0x8c)
[<c007dda4>] (clockevents_exchange_device) from [<c007e628>] (tick_check_new_device+0x90/0xd0)
[<c007e628>] (tick_check_new_device) from [<c007d488>] (clockevents_register_device+0x54/0x10c)
[<c007d488>] (clockevents_register_device) from [<c07073bc>] (clocksource_probe+0x4c/0x90)
[<c07073bc>] (clocksource_probe) from [<c06eab58>] (start_kernel+0x278/0x3a4)
[<c06eab58>] (start_kernel) from [<2000807c>] (0x2000807c)
---[ end trace 0000000000000001 ]---


My understanding is that clockevents_exchange_device() changes the state
from detached to shutdown and so at that point the IRQ has never been
requested.


> > 	return 0;
> > }
> > 
> > /*
> >+ * IRQ handler for the timer.
> >+ */
> >+static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
> 
> this is just here to avoid to forward declaration.
> …

Indeed.

> >diff --git a/drivers/clocksource/timer-atmel-st.c b/drivers/clocksource/timer-atmel-st.c
> >index ea37afc26e1b..11ce404d0791 100644
> >--- a/drivers/clocksource/timer-atmel-st.c
> >+++ b/drivers/clocksource/timer-atmel-st.c
> >@@ -229,15 +229,15 @@ static void __init atmel_st_timer_init(struct device_node *node)
> > 	regmap_read(regmap_st, AT91_ST_SR, &val);
> > 
> > 	/* Get the interrupts property */
> >-	irq  = irq_of_parse_and_map(node, 0);
> >-	if (!irq)
> >+	atmel_st_irq  = irq_of_parse_and_map(node, 0);
> >+	if (!atmel_st_irq)
> > 		panic(pr_fmt("Unable to get IRQ from DT\n"));
> > 
> > 	sclk = of_clk_get(node, 0);
> > 	if (IS_ERR(sclk))
> > 		panic(pr_fmt("Unable to get slow clock\n"));
> > 
> >-	clk_prepare_enable(sclk);
> >+	ret = clk_prepare_enable(sclk);
> this piece applies to upstream v4.4.
> 

Yeah, I'll submit it.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Alexandre Belloni]

On 18/01/2016 at 18:25:22 +0100, Sebastian Andrzej Siewior wrote :
> * Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:
> 
> >index 80d74c4adcbe..43b50634d640 100644
> >--- a/drivers/clocksource/timer-atmel-pit.c
> >+++ b/drivers/clocksource/timer-atmel-pit.c
> >@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
> > 
> > 	/* disable irq, leaving the clocksource active */
> > 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
> >-	free_irq(atmel_pit_irq, data);
> >+	if (!clockevent_state_detached(dev))
> >+		free_irq(data->irq, data);
> 
> I did it in the meantime without clockevent_state_detached(). From what
> it looks, it first sets the state and then invokes
> pit_clkevt_shutdown(). Any particular reason for this?
> 

Yeah, I forgot to mention that. Freeing the irq unconditionally
results in:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at kernel/irq/manage.c:1541
__free_irq+0xb4/0x2c8()
Trying to free already-free IRQ 16
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.4.0-rt2+ #31
Hardware name: Atmel SAMA5
[<c0016344>] (unwind_backtrace) from [<c0012d7c>] (show_stack+0x10/0x14)
[<c0012d7c>] (show_stack) from [<c021639c>] (dump_stack+0x80/0x94)
[<c021639c>] (dump_stack) from [<c001f528>] (warn_slowpath_common+0x80/0xb0)
[<c001f528>] (warn_slowpath_common) from [<c001f588>] (warn_slowpath_fmt+0x30/0x40)
[<c001f588>] (warn_slowpath_fmt) from [<c00615e0>] (__free_irq+0xb4/0x2c8)
[<c00615e0>] (__free_irq) from [<c0061878>] (free_irq+0x3c/0x70)
[<c0061878>] (free_irq) from [<c0391ba8>] (pit_clkevt_shutdown+0x24/0x2c)
[<c0391ba8>] (pit_clkevt_shutdown) from [<c007d9a0>] (clockevents_switch_state+0x60/0x130)
[<c007d9a0>] (clockevents_switch_state) from [<c007dda4>] (clockevents_exchange_device+0x78/0x8c)
[<c007dda4>] (clockevents_exchange_device) from [<c007e628>] (tick_check_new_device+0x90/0xd0)
[<c007e628>] (tick_check_new_device) from [<c007d488>] (clockevents_register_device+0x54/0x10c)
[<c007d488>] (clockevents_register_device) from [<c07073bc>] (clocksource_probe+0x4c/0x90)
[<c07073bc>] (clocksource_probe) from [<c06eab58>] (start_kernel+0x278/0x3a4)
[<c06eab58>] (start_kernel) from [<2000807c>] (0x2000807c)
---[ end trace 0000000000000001 ]---


My understanding is that clockevents_exchange_device() changes the state
from detached to shutdown and so at that point the IRQ has never been
requested.


> > 	return 0;
> > }
> > 
> > /*
> >+ * IRQ handler for the timer.
> >+ */
> >+static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
> 
> this is just here to avoid to forward declaration.
> ?

Indeed.

> >diff --git a/drivers/clocksource/timer-atmel-st.c b/drivers/clocksource/timer-atmel-st.c
> >index ea37afc26e1b..11ce404d0791 100644
> >--- a/drivers/clocksource/timer-atmel-st.c
> >+++ b/drivers/clocksource/timer-atmel-st.c
> >@@ -229,15 +229,15 @@ static void __init atmel_st_timer_init(struct device_node *node)
> > 	regmap_read(regmap_st, AT91_ST_SR, &val);
> > 
> > 	/* Get the interrupts property */
> >-	irq  = irq_of_parse_and_map(node, 0);
> >-	if (!irq)
> >+	atmel_st_irq  = irq_of_parse_and_map(node, 0);
> >+	if (!atmel_st_irq)
> > 		panic(pr_fmt("Unable to get IRQ from DT\n"));
> > 
> > 	sclk = of_clk_get(node, 0);
> > 	if (IS_ERR(sclk))
> > 		panic(pr_fmt("Unable to get slow clock\n"));
> > 
> >-	clk_prepare_enable(sclk);
> >+	ret = clk_prepare_enable(sclk);
> this piece applies to upstream v4.4.
> 

Yeah, I'll submit it.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: Fix preempt-rt on AT91

[Alexandre Belloni]

Hi,

On 18/01/2016 at 18:42:59 +0100, Sebastian Andrzej Siewior wrote :
> > 1/ After apply the preempt-rt patch, the kernel doesn't build anymore,
> > arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch is not correct
> > anymore. Can you squash the following patch to solve this build issue?
> 
> I folded most of the changes and should be part of -rt3.
> 

Thanks!

> > 2/ This approach actually has more issues, in particular, request_irq() must
> > not be calls from interrupt disabled context. That only works because
> > might_sleep() is not active during early boot.
> > For more information, see the following discussion:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357607.html
> >
> > However, we can't remove those patches now because else we would suffer from
> > another issue:
> > The timer interrupt is shared with other devices, in particular the debug tty,
> > the rtc dans the pmc. When using preempt-rt, those interrupts become threaded
> > interrupts. But, the pit interrupt is requested with IRQF_TIMER and so has
> > IRQF_NO_THREAD. If the pit interrupt is not freed at early boot, all the other
> > devices will fail requesting their interrupt, for example:
> >
> >genirq: Flags mismatch irq 16. 00042080 (ttyS0) vs. 00015280 (at91_tick)
> >atmel_usart ffffee00.serial: atmel_startup - Can't get irq
> 
> I assumed the pit is just temporary here for the bootstap. The pit irq is
> removed and the system switches to the TCB clocksource which runs at a
> higher rate.
> 
> > I'd say that the proper solution would still be to implement the virtual
> > irqchip because this would still hit people not wanting to use the TCB as
> > their clock source.
> 
> why wouldn't people not want that?

Because they may be using the TCBs for something else: PWM, frequency
measure, quadrature decoder...

> For a virtual irqchip you would need a mask/unmask register in order to
> individual disable/enable the irq and you need something to figure out
> which one of the three is active. You don't have all those things, do
> you?
> 

The proposed solution was software only. It mainly consisted in a simple
irq demuxer.

> > 3/ Finally, the kernel will crash when initializing the PMC driver. This is
> > solved by this series that will hopefully land in the mainline:
> >   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/390954.html
> 
> This is new, isn't it? So the series in currently in v3 and people seem
> happy with it. Please poke me once with the commit ids once it is merged
> on its way upstream and I will try cherry-pick them from next or so.
> 

I added that part mainly to inform people were to look to get preemt-rt
working on at91. No action on your side for that one yet.

> All in all, care to forwarded the working pieces from -RT patch set
> upstream? I problem I have here is mostly that I can't the patches on
> actual hardware. Disabling the PIT and running on the other clocksource
> isn't that -RT specific after all :)

I'd say that the only remaining part is the IRQ freeing/requesting but
as I said, this can't land in mainline as is. I still plan to work on
that later.
I'd say that most people running linux on at91 are already using the tcb
as the clocksource, this is already available in the mainline and is the
default unless the TCBs are used for something else.

If you are willing to test, I'm pretty sure we can convince Nicolas to
send you a board. Unfortunately, the main issues appear on old products,
not on sama5.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fix preempt-rt on AT91

[Alexandre Belloni]

Hi,

On 18/01/2016 at 18:42:59 +0100, Sebastian Andrzej Siewior wrote :
> > 1/ After apply the preempt-rt patch, the kernel doesn't build anymore,
> > arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch is not correct
> > anymore. Can you squash the following patch to solve this build issue?
> 
> I folded most of the changes and should be part of -rt3.
> 

Thanks!

> > 2/ This approach actually has more issues, in particular, request_irq() must
> > not be calls from interrupt disabled context. That only works because
> > might_sleep() is not active during early boot.
> > For more information, see the following discussion:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/357607.html
> >
> > However, we can't remove those patches now because else we would suffer from
> > another issue:
> > The timer interrupt is shared with other devices, in particular the debug tty,
> > the rtc dans the pmc. When using preempt-rt, those interrupts become threaded
> > interrupts. But, the pit interrupt is requested with IRQF_TIMER and so has
> > IRQF_NO_THREAD. If the pit interrupt is not freed at early boot, all the other
> > devices will fail requesting their interrupt, for example:
> >
> >genirq: Flags mismatch irq 16. 00042080 (ttyS0) vs. 00015280 (at91_tick)
> >atmel_usart ffffee00.serial: atmel_startup - Can't get irq
> 
> I assumed the pit is just temporary here for the bootstap. The pit irq is
> removed and the system switches to the TCB clocksource which runs at a
> higher rate.
> 
> > I'd say that the proper solution would still be to implement the virtual
> > irqchip because this would still hit people not wanting to use the TCB as
> > their clock source.
> 
> why wouldn't people not want that?

Because they may be using the TCBs for something else: PWM, frequency
measure, quadrature decoder...

> For a virtual irqchip you would need a mask/unmask register in order to
> individual disable/enable the irq and you need something to figure out
> which one of the three is active. You don't have all those things, do
> you?
> 

The proposed solution was software only. It mainly consisted in a simple
irq demuxer.

> > 3/ Finally, the kernel will crash when initializing the PMC driver. This is
> > solved by this series that will hopefully land in the mainline:
> >   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/390954.html
> 
> This is new, isn't it? So the series in currently in v3 and people seem
> happy with it. Please poke me once with the commit ids once it is merged
> on its way upstream and I will try cherry-pick them from next or so.
> 

I added that part mainly to inform people were to look to get preemt-rt
working on at91. No action on your side for that one yet.

> All in all, care to forwarded the working pieces from -RT patch set
> upstream? I problem I have here is mostly that I can't the patches on
> actual hardware. Disabling the PIT and running on the other clocksource
> isn't that -RT specific after all :)

I'd say that the only remaining part is the IRQ freeing/requesting but
as I said, this can't land in mainline as is. I still plan to work on
that later.
I'd say that most people running linux on at91 are already using the tcb
as the clocksource, this is already available in the mainline and is the
default unless the TCBs are used for something else.

If you are willing to test, I'm pretty sure we can convince Nicolas to
send you a board. Unfortunately, the main issues appear on old products,
not on sama5.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Sebastian Andrzej Siewior]

On 01/18/2016 07:42 PM, Alexandre Belloni wrote:
> On 18/01/2016 at 18:25:22 +0100, Sebastian Andrzej Siewior wrote :
>> * Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:
>>
>>> index 80d74c4adcbe..43b50634d640 100644
>>> --- a/drivers/clocksource/timer-atmel-pit.c
>>> +++ b/drivers/clocksource/timer-atmel-pit.c
>>> @@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
>>>
>>> 	/* disable irq, leaving the clocksource active */
>>> 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
>>> -	free_irq(atmel_pit_irq, data);
>>> +	if (!clockevent_state_detached(dev))
>>> +		free_irq(data->irq, data);
>>
>> I did it in the meantime without clockevent_state_detached(). From what
>> it looks, it first sets the state and then invokes
>> pit_clkevt_shutdown(). Any particular reason for this?
>>
> 
> Yeah, I forgot to mention that. Freeing the irq unconditionally
> results in:
…

> 
> 
> My understanding is that clockevents_exchange_device() changes the state
> from detached to shutdown and so at that point the IRQ has never been
> requested.

I see. So we get shutdown called twice while set_periodic was only
called once. In that case I would suggest to have internal bookkeeping
instead of relying on current core's behavior when it is time free the
irq.

> 

Sebastian

[PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Sebastian Andrzej Siewior]

On 01/18/2016 07:42 PM, Alexandre Belloni wrote:
> On 18/01/2016 at 18:25:22 +0100, Sebastian Andrzej Siewior wrote :
>> * Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:
>>
>>> index 80d74c4adcbe..43b50634d640 100644
>>> --- a/drivers/clocksource/timer-atmel-pit.c
>>> +++ b/drivers/clocksource/timer-atmel-pit.c
>>> @@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
>>>
>>> 	/* disable irq, leaving the clocksource active */
>>> 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
>>> -	free_irq(atmel_pit_irq, data);
>>> +	if (!clockevent_state_detached(dev))
>>> +		free_irq(data->irq, data);
>>
>> I did it in the meantime without clockevent_state_detached(). From what
>> it looks, it first sets the state and then invokes
>> pit_clkevt_shutdown(). Any particular reason for this?
>>
> 
> Yeah, I forgot to mention that. Freeing the irq unconditionally
> results in:
?

> 
> 
> My understanding is that clockevents_exchange_device() changes the state
> from detached to shutdown and so at that point the IRQ has never been
> requested.

I see. So we get shutdown called twice while set_periodic was only
called once. In that case I would suggest to have internal bookkeeping
instead of relying on current core's behavior when it is time free the
irq.

> 

Sebastian

Re: Fix preempt-rt on AT91

[Sebastian Andrzej Siewior]

On 01/18/2016 08:23 PM, Alexandre Belloni wrote:
> Hi,
Hi,

>>> I'd say that the proper solution would still be to implement the virtual
>>> irqchip because this would still hit people not wanting to use the TCB as
>>> their clock source.
>>
>> why wouldn't people not want that?
> 
> Because they may be using the TCBs for something else: PWM, frequency
> measure, quadrature decoder...

Oh okay.

>> For a virtual irqchip you would need a mask/unmask register in order to
>> individual disable/enable the irq and you need something to figure out
>> which one of the three is active. You don't have all those things, do
>> you?
>>
> 
> The proposed solution was software only. It mainly consisted in a simple
> irq demuxer.

Well, if it works properly and does not lead to any new bugs or
anything else then nobody will mind I guess.

>> All in all, care to forwarded the working pieces from -RT patch set
>> upstream? I problem I have here is mostly that I can't the patches on
>> actual hardware. Disabling the PIT and running on the other clocksource
>> isn't that -RT specific after all :)
> 
> I'd say that the only remaining part is the IRQ freeing/requesting but
> as I said, this can't land in mainline as is. I still plan to work on
> that later.
> I'd say that most people running linux on at91 are already using the tcb
> as the clocksource, this is already available in the mainline and is the
> default unless the TCBs are used for something else.

Wasn't there one of the patches to increase the frequency of the TCB
clocksource from the default to something higher?

Sebastian

Fix preempt-rt on AT91

[Sebastian Andrzej Siewior]

On 01/18/2016 08:23 PM, Alexandre Belloni wrote:
> Hi,
Hi,

>>> I'd say that the proper solution would still be to implement the virtual
>>> irqchip because this would still hit people not wanting to use the TCB as
>>> their clock source.
>>
>> why wouldn't people not want that?
> 
> Because they may be using the TCBs for something else: PWM, frequency
> measure, quadrature decoder...

Oh okay.

>> For a virtual irqchip you would need a mask/unmask register in order to
>> individual disable/enable the irq and you need something to figure out
>> which one of the three is active. You don't have all those things, do
>> you?
>>
> 
> The proposed solution was software only. It mainly consisted in a simple
> irq demuxer.

Well, if it works properly and does not lead to any new bugs or
anything else then nobody will mind I guess.

>> All in all, care to forwarded the working pieces from -RT patch set
>> upstream? I problem I have here is mostly that I can't the patches on
>> actual hardware. Disabling the PIT and running on the other clocksource
>> isn't that -RT specific after all :)
> 
> I'd say that the only remaining part is the IRQ freeing/requesting but
> as I said, this can't land in mainline as is. I still plan to work on
> that later.
> I'd say that most people running linux on at91 are already using the tcb
> as the clocksource, this is already available in the mainline and is the
> default unless the TCBs are used for something else.

Wasn't there one of the patches to increase the frequency of the TCB
clocksource from the default to something higher?

Sebastian

Re: Fix preempt-rt on AT91

[Alexandre Belloni]

On 18/01/2016 at 21:30:13 +0100, Sebastian Andrzej Siewior wrote :
> Well, if it works properly and does not lead to any new bugs or
> anything else then nobody will mind I guess.
> 

Yeah, the DT guys were not happy about the patch set, I'll try to work
around using DT ;)

> >> All in all, care to forwarded the working pieces from -RT patch set
> >> upstream? I problem I have here is mostly that I can't the patches on
> >> actual hardware. Disabling the PIT and running on the other clocksource
> >> isn't that -RT specific after all :)
> > 
> > I'd say that the only remaining part is the IRQ freeing/requesting but
> > as I said, this can't land in mainline as is. I still plan to work on
> > that later.
> > I'd say that most people running linux on at91 are already using the tcb
> > as the clocksource, this is already available in the mainline and is the
> > default unless the TCBs are used for something else.
> 
> Wasn't there one of the patches to increase the frequency of the TCB
> clocksource from the default to something higher?
> 

Indeed, it may be worth mainlining that one.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fix preempt-rt on AT91

[Alexandre Belloni]

On 18/01/2016 at 21:30:13 +0100, Sebastian Andrzej Siewior wrote :
> Well, if it works properly and does not lead to any new bugs or
> anything else then nobody will mind I guess.
> 

Yeah, the DT guys were not happy about the patch set, I'll try to work
around using DT ;)

> >> All in all, care to forwarded the working pieces from -RT patch set
> >> upstream? I problem I have here is mostly that I can't the patches on
> >> actual hardware. Disabling the PIT and running on the other clocksource
> >> isn't that -RT specific after all :)
> > 
> > I'd say that the only remaining part is the IRQ freeing/requesting but
> > as I said, this can't land in mainline as is. I still plan to work on
> > that later.
> > I'd say that most people running linux on at91 are already using the tcb
> > as the clocksource, this is already available in the mainline and is the
> > default unless the TCBs are used for something else.
> 
> Wasn't there one of the patches to increase the frequency of the TCB
> clocksource from the default to something higher?
> 

Indeed, it may be worth mainlining that one.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Alexandre Belloni]

On 18/01/2016 at 21:24:28 +0100, Sebastian Andrzej Siewior wrote :
> > 
> > My understanding is that clockevents_exchange_device() changes the state
> > from detached to shutdown and so at that point the IRQ has never been
> > requested.
> 
> I see. So we get shutdown called twice while set_periodic was only
> called once. In that case I would suggest to have internal bookkeeping
> instead of relying on current core's behavior when it is time free the
> irq.
> 

Ok, I can do that. What should I base my patch on?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Alexandre Belloni]

On 18/01/2016 at 21:24:28 +0100, Sebastian Andrzej Siewior wrote :
> > 
> > My understanding is that clockevents_exchange_device() changes the state
> > from detached to shutdown and so at that point the IRQ has never been
> > requested.
> 
> I see. So we get shutdown called twice while set_periodic was only
> called once. In that case I would suggest to have internal bookkeeping
> instead of relying on current core's behavior when it is time free the
> irq.
> 

Ok, I can do that. What should I base my patch on?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Thomas Gleixner]

On Mon, 18 Jan 2016, Alexandre Belloni wrote:
> On 18/01/2016 at 18:25:22 +0100, Sebastian Andrzej Siewior wrote :
> > * Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:
> > 
> > >index 80d74c4adcbe..43b50634d640 100644
> > >--- a/drivers/clocksource/timer-atmel-pit.c
> > >+++ b/drivers/clocksource/timer-atmel-pit.c
> > >@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
> > > 
> > > 	/* disable irq, leaving the clocksource active */
> > > 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
> > >-	free_irq(atmel_pit_irq, data);
> > >+	if (!clockevent_state_detached(dev))
> > >+		free_irq(data->irq, data);
> > 
> > I did it in the meantime without clockevent_state_detached(). From what
> > it looks, it first sets the state and then invokes
> > pit_clkevt_shutdown(). Any particular reason for this?
> > 
> 
> Yeah, I forgot to mention that. Freeing the irq unconditionally
> results in:

Well freeing the irq from that context in RT only works because its called
before SYSTEM_STATE=RUNNING. So no, this was wrong forever.

The issue we are dealing with is that the timer interrupt is shared with the
uart. So the timer has IRQ_NO_THREAD set and the uart interrupt gets force
threaded. So that results in a failure to request the interrupt for the
UART. That's not RT specific, that already happens in mainline if you add
'threadirqs' to the command line.

So until the DT folks come to senses and we get that dummy demux chip done, I
came up with the following - completely untested - solution.

The downside of this is, that the timer will be delayed until the uart thread
returns, but with the replacement clockevent in place on RT that's a non
issue. For mainline it's obviously better than what we have now.

Thanks,

	tglx

8<---------------

--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -61,6 +61,10 @@
  *                interrupt handler after suspending interrupts. For system
  *                wakeup devices users need to implement wakeup detection in
  *                their interrupt handlers.
+ * IRQF_COND_ONESHOT - If the IRQ is shared between a NO_THREAD user and a
+ *		regular interrupt, force the ONESHOT flag on the NO_THREAD user
+ *		when threaded irqs are enforced. Workaround for silly ATMEL
+ *		SoCs which share the timer and the UART interrupt
  * IRQF_NO_SOFTIRQ_CALL - Do not process softirqs in the irq thread context (RT)
  */
 #define IRQF_SHARED		0x00000080
@@ -75,7 +79,8 @@
 #define IRQF_NO_THREAD		0x00010000
 #define IRQF_EARLY_RESUME	0x00020000
 #define IRQF_COND_SUSPEND	0x00040000
-#define IRQF_NO_SOFTIRQ_CALL	0x00080000
+#define IRQF_COND_ONESHOT	0x00080000
+#define IRQF_NO_SOFTIRQ_CALL	0x00100000
 
 #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1208,6 +1208,14 @@ static int
 	new->irq = irq;
 
 	/*
+	 * Workaround for silly ATMEL SoCs with shared timer and uart
+	 * interrupt.
+	 */
+	if (force_irqthreads && (new->flags & IRQF_COND_ONESHOT) &&
+	    (new->flags & IRQF_NO_THREAD))
+		new->flags |= IRQF_ONESHOT;
+
+	/*
 	 * Check whether the interrupt nests into another interrupt
 	 * thread.
 	 */
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -208,7 +208,7 @@ static void __init at91sam926x_pit_commo
 
 	/* Set up irq handler */
 	ret = request_irq(data->irq, at91sam926x_pit_interrupt,
-			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
+			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL| IRQF_COND_ONESHOT,
 			  "at91_tick", data);
 	if (ret)
 		panic(pr_fmt("Unable to setup IRQ\n"));
--- a/drivers/clocksource/timer-atmel-st.c
+++ b/drivers/clocksource/timer-atmel-st.c
@@ -216,7 +216,7 @@ static void __init atmel_st_timer_init(s
 
 	/* Make IRQs happen for the system timer */
 	ret = request_irq(irq, at91rm9200_timer_interrupt,
-			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
+			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL | IRQF_COND_ONESHOT,
 			  "at91_tick", regmap_st);
 	if (ret)
 		panic(pr_fmt("Unable to setup IRQ\n"));

[PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Thomas Gleixner]

On Mon, 18 Jan 2016, Alexandre Belloni wrote:
> On 18/01/2016 at 18:25:22 +0100, Sebastian Andrzej Siewior wrote :
> > * Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:
> > 
> > >index 80d74c4adcbe..43b50634d640 100644
> > >--- a/drivers/clocksource/timer-atmel-pit.c
> > >+++ b/drivers/clocksource/timer-atmel-pit.c
> > >@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
> > > 
> > > 	/* disable irq, leaving the clocksource active */
> > > 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
> > >-	free_irq(atmel_pit_irq, data);
> > >+	if (!clockevent_state_detached(dev))
> > >+		free_irq(data->irq, data);
> > 
> > I did it in the meantime without clockevent_state_detached(). From what
> > it looks, it first sets the state and then invokes
> > pit_clkevt_shutdown(). Any particular reason for this?
> > 
> 
> Yeah, I forgot to mention that. Freeing the irq unconditionally
> results in:

Well freeing the irq from that context in RT only works because its called
before SYSTEM_STATE=RUNNING. So no, this was wrong forever.

The issue we are dealing with is that the timer interrupt is shared with the
uart. So the timer has IRQ_NO_THREAD set and the uart interrupt gets force
threaded. So that results in a failure to request the interrupt for the
UART. That's not RT specific, that already happens in mainline if you add
'threadirqs' to the command line.

So until the DT folks come to senses and we get that dummy demux chip done, I
came up with the following - completely untested - solution.

The downside of this is, that the timer will be delayed until the uart thread
returns, but with the replacement clockevent in place on RT that's a non
issue. For mainline it's obviously better than what we have now.

Thanks,

	tglx

8<---------------

--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -61,6 +61,10 @@
  *                interrupt handler after suspending interrupts. For system
  *                wakeup devices users need to implement wakeup detection in
  *                their interrupt handlers.
+ * IRQF_COND_ONESHOT - If the IRQ is shared between a NO_THREAD user and a
+ *		regular interrupt, force the ONESHOT flag on the NO_THREAD user
+ *		when threaded irqs are enforced. Workaround for silly ATMEL
+ *		SoCs which share the timer and the UART interrupt
  * IRQF_NO_SOFTIRQ_CALL - Do not process softirqs in the irq thread context (RT)
  */
 #define IRQF_SHARED		0x00000080
@@ -75,7 +79,8 @@
 #define IRQF_NO_THREAD		0x00010000
 #define IRQF_EARLY_RESUME	0x00020000
 #define IRQF_COND_SUSPEND	0x00040000
-#define IRQF_NO_SOFTIRQ_CALL	0x00080000
+#define IRQF_COND_ONESHOT	0x00080000
+#define IRQF_NO_SOFTIRQ_CALL	0x00100000
 
 #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1208,6 +1208,14 @@ static int
 	new->irq = irq;
 
 	/*
+	 * Workaround for silly ATMEL SoCs with shared timer and uart
+	 * interrupt.
+	 */
+	if (force_irqthreads && (new->flags & IRQF_COND_ONESHOT) &&
+	    (new->flags & IRQF_NO_THREAD))
+		new->flags |= IRQF_ONESHOT;
+
+	/*
 	 * Check whether the interrupt nests into another interrupt
 	 * thread.
 	 */
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -208,7 +208,7 @@ static void __init at91sam926x_pit_commo
 
 	/* Set up irq handler */
 	ret = request_irq(data->irq, at91sam926x_pit_interrupt,
-			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
+			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL| IRQF_COND_ONESHOT,
 			  "at91_tick", data);
 	if (ret)
 		panic(pr_fmt("Unable to setup IRQ\n"));
--- a/drivers/clocksource/timer-atmel-st.c
+++ b/drivers/clocksource/timer-atmel-st.c
@@ -216,7 +216,7 @@ static void __init atmel_st_timer_init(s
 
 	/* Make IRQs happen for the system timer */
 	ret = request_irq(irq, at91rm9200_timer_interrupt,
-			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
+			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL | IRQF_COND_ONESHOT,
 			  "at91_tick", regmap_st);
 	if (ret)
 		panic(pr_fmt("Unable to setup IRQ\n"));

Re: Fix preempt-rt on AT91

[Jean-Denis Girard]

Hi Alexandre,

Le 16/01/2016 16:23, Alexandre Belloni a écrit :
> Preemp-rt on at91 has multiple issues:

I'd like to use Linux RT on a Acme Arietta (AT91SAM9G25). I tried
4.4.1-rt6, but it does not boot, I get absolutely no message on the
debug port interface. I'm a complete newbie on Linux RT, so I may well
have done something wrong, but the same kernel config without
CONFIG_PREEMPT_RT runs fine.

I'm a bit confused about what is needed for running 4.4.1-rt6 on this
board: it seems some of your patches got merged, could you please
clarify the situation? Are there some peripherals not supported (I
mainly need ADC and PWM)?


Thanks,
-- 
Jean-Denis Girard

SysNux                Systèmes   Linux   en   Polynésie   française
http://www.sysnux.pf/ Tél: +689 40.50.10.40 / GSM: +689 87.79.75.27

Re: Fix preempt-rt on AT91

[Alexandre Belloni]

Hi Sebastian,

On 18/01/2016 at 18:42:59 +0100, Sebastian Andrzej Siewior wrote :
> > 3/ Finally, the kernel will crash when initializing the PMC driver. This is
> > solved by this series that will hopefully land in the mainline:
> >   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/390954.html
> 
> This is new, isn't it? So the series in currently in v3 and people seem
> happy with it. Please poke me once with the commit ids once it is merged
> on its way upstream and I will try cherry-pick them from next or so.
> 

The series landed in arm-soc and is making its way to v4.6. I guess you
can pull the original pull request. It is based on v4.5-rc1 but should
apply cleanly on v4.4:

The following changes since commit
92e963f50fc74041b5e9e744c330dca48e04f08d:

  Linux 4.5-rc1 (2016-01-24 13:06:47 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git
tags/at91-ab-4.6-drivers

for you to fetch changes up to 0002ca168f16e5b6ac67415a4e0198cc39af2b7f:

  clk: at91: remove useless includes (2016-02-17 17:53:04 +0100)




You may also stop at 99a81706526fb167029a940ef1f7bfbe882abd3e which
should solve the crash but it has not been tested as thoroughly.

Thanks!

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fix preempt-rt on AT91

[Alexandre Belloni]

Hi Sebastian,

On 18/01/2016 at 18:42:59 +0100, Sebastian Andrzej Siewior wrote :
> > 3/ Finally, the kernel will crash when initializing the PMC driver. This is
> > solved by this series that will hopefully land in the mainline:
> >   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/390954.html
> 
> This is new, isn't it? So the series in currently in v3 and people seem
> happy with it. Please poke me once with the commit ids once it is merged
> on its way upstream and I will try cherry-pick them from next or so.
> 

The series landed in arm-soc and is making its way to v4.6. I guess you
can pull the original pull request. It is based on v4.5-rc1 but should
apply cleanly on v4.4:

The following changes since commit
92e963f50fc74041b5e9e744c330dca48e04f08d:

  Linux 4.5-rc1 (2016-01-24 13:06:47 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git
tags/at91-ab-4.6-drivers

for you to fetch changes up to 0002ca168f16e5b6ac67415a4e0198cc39af2b7f:

  clk: at91: remove useless includes (2016-02-17 17:53:04 +0100)




You may also stop at 99a81706526fb167029a940ef1f7bfbe882abd3e which
should solve the crash but it has not been tested as thoroughly.

Thanks!

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: Fix preempt-rt on AT91

[Sebastian Andrzej Siewior]

* Alexandre Belloni | 2016-03-05 12:35:53 [+0100]:

>Hi Sebastian,
Hi Alexandre,

>The series landed in arm-soc and is making its way to v4.6. I guess you
>can pull the original pull request. It is based on v4.5-rc1 but should
>apply cleanly on v4.4:

I sucked this in. It seems to work. What remains that free_irq() thingy:
|WARNING: CPU: 0 PID: 0 at kernel/irq/manage.c:1541 __free_irq+0xb4/0x2e0()
|Trying to free already-free IRQ 16
|CPU: 0 PID: 0 Comm: swapper Not tainted 4.4.4-rt10+ #219
|Hardware name: Atmel SAMA5
|[<c0015bd0>] (unwind_backtrace) from [<c0013604>] (show_stack+0x10/0x14)
|[<c0013604>] (show_stack) from [<c002b850>] (warn_slowpath_common+0x80/0xac)
|[<c002b850>] (warn_slowpath_common) from [<c002b8ac>] (warn_slowpath_fmt+0x30/0x40)
|[<c002b8ac>] (warn_slowpath_fmt) from [<c005adfc>] (__free_irq+0xb4/0x2e0)
|[<c005adfc>] (__free_irq) from [<c005b0a0>] (free_irq+0x30/0x4c)
|[<c005b0a0>] (free_irq) from [<c0429034>] (pit_clkevt_shutdown+0x24/0x2c)
|[<c0429034>] (pit_clkevt_shutdown) from [<c0076430>] (clockevents_switch_state+0x60/0x130)
|---[ end trace 0000000000000001 ]---

which is a different problem and was there. The new thing is this:

|WARNING: CPU: 0 PID: 1 at kernel/locking/rtmutex.c:2062 rt_mutex_trylock+0x30/0x108)
|Modules linked in:
|CPU: 0 PID: 1 Comm: swapper Tainted: G        W       4.4.4-rt10+ #219
|Hardware name: Atmel SAMA5
|[<c0015bd0>] (unwind_backtrace) from [<c0013604>] (show_stack+0x10/0x14)
|[<c0013604>] (show_stack) from [<c002b850>] (warn_slowpath_common+0x80/0xac)
|[<c002b850>] (warn_slowpath_common) from [<c002b918>] (warn_slowpath_null+0x1c/0x24)
|[<c002b918>] (warn_slowpath_null) from [<c054a54c>] (rt_mutex_trylock+0x30/0x108)
|[<c054a54c>] (rt_mutex_trylock) from [<c054bd0c>] (rt_spin_trylock_irqsave+0x10/0x1c)
|[<c054bd0c>] (rt_spin_trylock_irqsave) from [<c043e030>] (clk_enable_lock+0x18/0x114)
|[<c043e030>] (clk_enable_lock) from [<c043f600>] (clk_disable+0x1c/0x34)
|[<c043f600>] (clk_disable) from [<c0429188>] (tc_shutdown+0x34/0x3c)
|[<c0429188>] (tc_shutdown) from [<c04291b0>] (tc_set_oneshot+0x20/0x50)
|[<c04291b0>] (tc_set_oneshot) from [<c00764a4>] (clockevents_switch_state+0xd4/0x130)
|[<c00764a4>] (clockevents_switch_state) from [<c00773e8>] (tick_switch_to_oneshot+0x48/0xb8)
|[<c00773e8>] (tick_switch_to_oneshot) from [<c006a480>] (hrtimer_run_queues+0x48/0x108)
|[<c006a480>] (hrtimer_run_queues) from [<c0068dd8>] (update_process_times+0x2c/0x64)
|[<c0068dd8>] (update_process_times) from [<c0076ad0>] (tick_handle_periodic+0x1c/0x90)
|[<c0076ad0>] (tick_handle_periodic) from [<c0429274>] (ch2_irq+0x20/0x28)
|[<c0429274>] (ch2_irq) from [<c005a39c>] (handle_irq_event_percpu+0x74/0x16c)

Is it possible to drop this disable/enable clock on the switch from pit
to one shot mode?

>You may also stop at 99a81706526fb167029a940ef1f7bfbe882abd3e which
>should solve the crash but it has not been tested as thoroughly.

Did you confuse the sha1 with something? This patch is called ("clk:
at91: remove IRQ handling and use polling") and is part of the series
you gave (patch #3).

>Thanks!

Sebastian

Fix preempt-rt on AT91

[Sebastian Andrzej Siewior]

* Alexandre Belloni | 2016-03-05 12:35:53 [+0100]:

>Hi Sebastian,
Hi Alexandre,

>The series landed in arm-soc and is making its way to v4.6. I guess you
>can pull the original pull request. It is based on v4.5-rc1 but should
>apply cleanly on v4.4:

I sucked this in. It seems to work. What remains that free_irq() thingy:
|WARNING: CPU: 0 PID: 0 at kernel/irq/manage.c:1541 __free_irq+0xb4/0x2e0()
|Trying to free already-free IRQ 16
|CPU: 0 PID: 0 Comm: swapper Not tainted 4.4.4-rt10+ #219
|Hardware name: Atmel SAMA5
|[<c0015bd0>] (unwind_backtrace) from [<c0013604>] (show_stack+0x10/0x14)
|[<c0013604>] (show_stack) from [<c002b850>] (warn_slowpath_common+0x80/0xac)
|[<c002b850>] (warn_slowpath_common) from [<c002b8ac>] (warn_slowpath_fmt+0x30/0x40)
|[<c002b8ac>] (warn_slowpath_fmt) from [<c005adfc>] (__free_irq+0xb4/0x2e0)
|[<c005adfc>] (__free_irq) from [<c005b0a0>] (free_irq+0x30/0x4c)
|[<c005b0a0>] (free_irq) from [<c0429034>] (pit_clkevt_shutdown+0x24/0x2c)
|[<c0429034>] (pit_clkevt_shutdown) from [<c0076430>] (clockevents_switch_state+0x60/0x130)
|---[ end trace 0000000000000001 ]---

which is a different problem and was there. The new thing is this:

|WARNING: CPU: 0 PID: 1 at kernel/locking/rtmutex.c:2062 rt_mutex_trylock+0x30/0x108)
|Modules linked in:
|CPU: 0 PID: 1 Comm: swapper Tainted: G        W       4.4.4-rt10+ #219
|Hardware name: Atmel SAMA5
|[<c0015bd0>] (unwind_backtrace) from [<c0013604>] (show_stack+0x10/0x14)
|[<c0013604>] (show_stack) from [<c002b850>] (warn_slowpath_common+0x80/0xac)
|[<c002b850>] (warn_slowpath_common) from [<c002b918>] (warn_slowpath_null+0x1c/0x24)
|[<c002b918>] (warn_slowpath_null) from [<c054a54c>] (rt_mutex_trylock+0x30/0x108)
|[<c054a54c>] (rt_mutex_trylock) from [<c054bd0c>] (rt_spin_trylock_irqsave+0x10/0x1c)
|[<c054bd0c>] (rt_spin_trylock_irqsave) from [<c043e030>] (clk_enable_lock+0x18/0x114)
|[<c043e030>] (clk_enable_lock) from [<c043f600>] (clk_disable+0x1c/0x34)
|[<c043f600>] (clk_disable) from [<c0429188>] (tc_shutdown+0x34/0x3c)
|[<c0429188>] (tc_shutdown) from [<c04291b0>] (tc_set_oneshot+0x20/0x50)
|[<c04291b0>] (tc_set_oneshot) from [<c00764a4>] (clockevents_switch_state+0xd4/0x130)
|[<c00764a4>] (clockevents_switch_state) from [<c00773e8>] (tick_switch_to_oneshot+0x48/0xb8)
|[<c00773e8>] (tick_switch_to_oneshot) from [<c006a480>] (hrtimer_run_queues+0x48/0x108)
|[<c006a480>] (hrtimer_run_queues) from [<c0068dd8>] (update_process_times+0x2c/0x64)
|[<c0068dd8>] (update_process_times) from [<c0076ad0>] (tick_handle_periodic+0x1c/0x90)
|[<c0076ad0>] (tick_handle_periodic) from [<c0429274>] (ch2_irq+0x20/0x28)
|[<c0429274>] (ch2_irq) from [<c005a39c>] (handle_irq_event_percpu+0x74/0x16c)

Is it possible to drop this disable/enable clock on the switch from pit
to one shot mode?

>You may also stop at 99a81706526fb167029a940ef1f7bfbe882abd3e which
>should solve the crash but it has not been tested as thoroughly.

Did you confuse the sha1 with something? This patch is called ("clk:
at91: remove IRQ handling and use polling") and is part of the series
you gave (patch #3).

>Thanks!

Sebastian

Re: Fix preempt-rt on AT91

[Thomas Gleixner]

On Tue, 8 Mar 2016, Sebastian Andrzej Siewior wrote:
> I sucked this in. It seems to work. What remains that free_irq() thingy:
> |WARNING: CPU: 0 PID: 0 at kernel/irq/manage.c:1541 __free_irq+0xb4/0x2e0()
> |Trying to free already-free IRQ 16
> |CPU: 0 PID: 0 Comm: swapper Not tainted 4.4.4-rt10+ #219
> |Hardware name: Atmel SAMA5
> |[<c0015bd0>] (unwind_backtrace) from [<c0013604>] (show_stack+0x10/0x14)
> |[<c0013604>] (show_stack) from [<c002b850>] (warn_slowpath_common+0x80/0xac)
> |[<c002b850>] (warn_slowpath_common) from [<c002b8ac>] (warn_slowpath_fmt+0x30/0x40)
> |[<c002b8ac>] (warn_slowpath_fmt) from [<c005adfc>] (__free_irq+0xb4/0x2e0)
> |[<c005adfc>] (__free_irq) from [<c005b0a0>] (free_irq+0x30/0x4c)
> |[<c005b0a0>] (free_irq) from [<c0429034>] (pit_clkevt_shutdown+0x24/0x2c)
> |[<c0429034>] (pit_clkevt_shutdown) from [<c0076430>] (clockevents_switch_state+0x60/0x130)
> |---[ end trace 0000000000000001 ]---
> 
> which is a different problem and was there. The new thing is this:

I think we really need to have this irq_en/disable_action() thingy sooner than
later.

> Is it possible to drop this disable/enable clock on the switch from pit
> to one shot mode?

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.

Thanks,

	tglx

Fix preempt-rt on AT91

[Thomas Gleixner]

On Tue, 8 Mar 2016, Sebastian Andrzej Siewior wrote:
> I sucked this in. It seems to work. What remains that free_irq() thingy:
> |WARNING: CPU: 0 PID: 0 at kernel/irq/manage.c:1541 __free_irq+0xb4/0x2e0()
> |Trying to free already-free IRQ 16
> |CPU: 0 PID: 0 Comm: swapper Not tainted 4.4.4-rt10+ #219
> |Hardware name: Atmel SAMA5
> |[<c0015bd0>] (unwind_backtrace) from [<c0013604>] (show_stack+0x10/0x14)
> |[<c0013604>] (show_stack) from [<c002b850>] (warn_slowpath_common+0x80/0xac)
> |[<c002b850>] (warn_slowpath_common) from [<c002b8ac>] (warn_slowpath_fmt+0x30/0x40)
> |[<c002b8ac>] (warn_slowpath_fmt) from [<c005adfc>] (__free_irq+0xb4/0x2e0)
> |[<c005adfc>] (__free_irq) from [<c005b0a0>] (free_irq+0x30/0x4c)
> |[<c005b0a0>] (free_irq) from [<c0429034>] (pit_clkevt_shutdown+0x24/0x2c)
> |[<c0429034>] (pit_clkevt_shutdown) from [<c0076430>] (clockevents_switch_state+0x60/0x130)
> |---[ end trace 0000000000000001 ]---
> 
> which is a different problem and was there. The new thing is this:

I think we really need to have this irq_en/disable_action() thingy sooner than
later.

> Is it possible to drop this disable/enable clock on the switch from pit
> to one shot mode?

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.

Thanks,

	tglx

Re: Fix preempt-rt on AT91

[Alexandre Belloni]

Hi,

On 08/03/2016 at 12:06:39 +0100, Sebastian Andrzej Siewior wrote :
> I sucked this in. It seems to work. What remains that free_irq() thingy:
> |WARNING: CPU: 0 PID: 0 at kernel/irq/manage.c:1541 __free_irq+0xb4/0x2e0()
> |Trying to free already-free IRQ 16
> |CPU: 0 PID: 0 Comm: swapper Not tainted 4.4.4-rt10+ #219
> |Hardware name: Atmel SAMA5
> |[<c0015bd0>] (unwind_backtrace) from [<c0013604>] (show_stack+0x10/0x14)
> |[<c0013604>] (show_stack) from [<c002b850>] (warn_slowpath_common+0x80/0xac)
> |[<c002b850>] (warn_slowpath_common) from [<c002b8ac>] (warn_slowpath_fmt+0x30/0x40)
> |[<c002b8ac>] (warn_slowpath_fmt) from [<c005adfc>] (__free_irq+0xb4/0x2e0)
> |[<c005adfc>] (__free_irq) from [<c005b0a0>] (free_irq+0x30/0x4c)
> |[<c005b0a0>] (free_irq) from [<c0429034>] (pit_clkevt_shutdown+0x24/0x2c)
> |[<c0429034>] (pit_clkevt_shutdown) from [<c0076430>] (clockevents_switch_state+0x60/0x130)
> |---[ end trace 0000000000000001 ]---
> 
> which is a different problem and was there. The new thing is this:
> 
> |WARNING: CPU: 0 PID: 1 at kernel/locking/rtmutex.c:2062 rt_mutex_trylock+0x30/0x108)
> |Modules linked in:
> |CPU: 0 PID: 1 Comm: swapper Tainted: G        W       4.4.4-rt10+ #219
> |Hardware name: Atmel SAMA5
> |[<c0015bd0>] (unwind_backtrace) from [<c0013604>] (show_stack+0x10/0x14)
> |[<c0013604>] (show_stack) from [<c002b850>] (warn_slowpath_common+0x80/0xac)
> |[<c002b850>] (warn_slowpath_common) from [<c002b918>] (warn_slowpath_null+0x1c/0x24)
> |[<c002b918>] (warn_slowpath_null) from [<c054a54c>] (rt_mutex_trylock+0x30/0x108)
> |[<c054a54c>] (rt_mutex_trylock) from [<c054bd0c>] (rt_spin_trylock_irqsave+0x10/0x1c)
> |[<c054bd0c>] (rt_spin_trylock_irqsave) from [<c043e030>] (clk_enable_lock+0x18/0x114)
> |[<c043e030>] (clk_enable_lock) from [<c043f600>] (clk_disable+0x1c/0x34)
> |[<c043f600>] (clk_disable) from [<c0429188>] (tc_shutdown+0x34/0x3c)
> |[<c0429188>] (tc_shutdown) from [<c04291b0>] (tc_set_oneshot+0x20/0x50)
> |[<c04291b0>] (tc_set_oneshot) from [<c00764a4>] (clockevents_switch_state+0xd4/0x130)
> |[<c00764a4>] (clockevents_switch_state) from [<c00773e8>] (tick_switch_to_oneshot+0x48/0xb8)
> |[<c00773e8>] (tick_switch_to_oneshot) from [<c006a480>] (hrtimer_run_queues+0x48/0x108)
> |[<c006a480>] (hrtimer_run_queues) from [<c0068dd8>] (update_process_times+0x2c/0x64)
> |[<c0068dd8>] (update_process_times) from [<c0076ad0>] (tick_handle_periodic+0x1c/0x90)
> |[<c0076ad0>] (tick_handle_periodic) from [<c0429274>] (ch2_irq+0x20/0x28)
> |[<c0429274>] (ch2_irq) from [<c005a39c>] (handle_irq_event_percpu+0x74/0x16c)
> 
> Is it possible to drop this disable/enable clock on the switch from pit
> to one shot mode?
> 

Both are things to work on. In the mean time, I'm using the following
patch:
https://github.com/alexandrebelloni/linux/commit/3a2eae463fce18ae815b887a5c9ca1a657b180ac

I understood from
http://lkml.iu.edu/hypermail/linux/kernel/1601.2/00941.html that this
was not your preferred course of action.

Even if that is something that is only seen when using preempt-rt, that
is probably something I should push mainline.

> >You may also stop at 99a81706526fb167029a940ef1f7bfbe882abd3e which
> >should solve the crash but it has not been tested as thoroughly.
> 
> Did you confuse the sha1 with something? This patch is called ("clk:
> at91: remove IRQ handling and use polling") and is part of the series
> you gave (patch #3).
> 

Yeah, what I meant is that you could stop merging after that patch as
this is the one solving the crash.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Fix preempt-rt on AT91

[Alexandre Belloni]

Hi,

On 08/03/2016 at 12:06:39 +0100, Sebastian Andrzej Siewior wrote :
> I sucked this in. It seems to work. What remains that free_irq() thingy:
> |WARNING: CPU: 0 PID: 0 at kernel/irq/manage.c:1541 __free_irq+0xb4/0x2e0()
> |Trying to free already-free IRQ 16
> |CPU: 0 PID: 0 Comm: swapper Not tainted 4.4.4-rt10+ #219
> |Hardware name: Atmel SAMA5
> |[<c0015bd0>] (unwind_backtrace) from [<c0013604>] (show_stack+0x10/0x14)
> |[<c0013604>] (show_stack) from [<c002b850>] (warn_slowpath_common+0x80/0xac)
> |[<c002b850>] (warn_slowpath_common) from [<c002b8ac>] (warn_slowpath_fmt+0x30/0x40)
> |[<c002b8ac>] (warn_slowpath_fmt) from [<c005adfc>] (__free_irq+0xb4/0x2e0)
> |[<c005adfc>] (__free_irq) from [<c005b0a0>] (free_irq+0x30/0x4c)
> |[<c005b0a0>] (free_irq) from [<c0429034>] (pit_clkevt_shutdown+0x24/0x2c)
> |[<c0429034>] (pit_clkevt_shutdown) from [<c0076430>] (clockevents_switch_state+0x60/0x130)
> |---[ end trace 0000000000000001 ]---
> 
> which is a different problem and was there. The new thing is this:
> 
> |WARNING: CPU: 0 PID: 1 at kernel/locking/rtmutex.c:2062 rt_mutex_trylock+0x30/0x108)
> |Modules linked in:
> |CPU: 0 PID: 1 Comm: swapper Tainted: G        W       4.4.4-rt10+ #219
> |Hardware name: Atmel SAMA5
> |[<c0015bd0>] (unwind_backtrace) from [<c0013604>] (show_stack+0x10/0x14)
> |[<c0013604>] (show_stack) from [<c002b850>] (warn_slowpath_common+0x80/0xac)
> |[<c002b850>] (warn_slowpath_common) from [<c002b918>] (warn_slowpath_null+0x1c/0x24)
> |[<c002b918>] (warn_slowpath_null) from [<c054a54c>] (rt_mutex_trylock+0x30/0x108)
> |[<c054a54c>] (rt_mutex_trylock) from [<c054bd0c>] (rt_spin_trylock_irqsave+0x10/0x1c)
> |[<c054bd0c>] (rt_spin_trylock_irqsave) from [<c043e030>] (clk_enable_lock+0x18/0x114)
> |[<c043e030>] (clk_enable_lock) from [<c043f600>] (clk_disable+0x1c/0x34)
> |[<c043f600>] (clk_disable) from [<c0429188>] (tc_shutdown+0x34/0x3c)
> |[<c0429188>] (tc_shutdown) from [<c04291b0>] (tc_set_oneshot+0x20/0x50)
> |[<c04291b0>] (tc_set_oneshot) from [<c00764a4>] (clockevents_switch_state+0xd4/0x130)
> |[<c00764a4>] (clockevents_switch_state) from [<c00773e8>] (tick_switch_to_oneshot+0x48/0xb8)
> |[<c00773e8>] (tick_switch_to_oneshot) from [<c006a480>] (hrtimer_run_queues+0x48/0x108)
> |[<c006a480>] (hrtimer_run_queues) from [<c0068dd8>] (update_process_times+0x2c/0x64)
> |[<c0068dd8>] (update_process_times) from [<c0076ad0>] (tick_handle_periodic+0x1c/0x90)
> |[<c0076ad0>] (tick_handle_periodic) from [<c0429274>] (ch2_irq+0x20/0x28)
> |[<c0429274>] (ch2_irq) from [<c005a39c>] (handle_irq_event_percpu+0x74/0x16c)
> 
> Is it possible to drop this disable/enable clock on the switch from pit
> to one shot mode?
> 

Both are things to work on. In the mean time, I'm using the following
patch:
https://github.com/alexandrebelloni/linux/commit/3a2eae463fce18ae815b887a5c9ca1a657b180ac

I understood from
http://lkml.iu.edu/hypermail/linux/kernel/1601.2/00941.html that this
was not your preferred course of action.

Even if that is something that is only seen when using preempt-rt, that
is probably something I should push mainline.

> >You may also stop at 99a81706526fb167029a940ef1f7bfbe882abd3e which
> >should solve the crash but it has not been tested as thoroughly.
> 
> Did you confuse the sha1 with something? This patch is called ("clk:
> at91: remove IRQ handling and use polling") and is part of the series
> you gave (patch #3).
> 

Yeah, what I meant is that you could stop merging after that patch as
this is the one solving the crash.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

arm: at91: do not disable/enable clocks in a row

[Sebastian Andrzej Siewior]

* Alexandre Belloni | 2016-03-08 19:39:10 [+0100]:

>Hi,
Hi,

>Both are things to work on. In the mean time, I'm using the following
>patch:
>https://github.com/alexandrebelloni/linux/commit/3a2eae463fce18ae815b887a5c9ca1a657b180ac
>
>I understood from
>http://lkml.iu.edu/hypermail/linux/kernel/1601.2/00941.html that this
>was not your preferred course of action.

That was one thing.  What about this:

---

Currently the driver will disable the clock and enable it one line later
if it is switching from periodic mode into one shot.
This can be avoided and causes a needless warning on -RT.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/clocksource/tcb_clksrc.c |   33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

--- a/drivers/clocksource/tcb_clksrc.c
+++ b/drivers/clocksource/tcb_clksrc.c
@@ -74,6 +74,7 @@ static struct clocksource clksrc = {
 struct tc_clkevt_device {
 	struct clock_event_device	clkevt;
 	struct clk			*clk;
+	bool				clk_enabled;
 	void __iomem			*regs;
 };
 
@@ -91,6 +92,24 @@ static struct tc_clkevt_device *to_tc_cl
  */
 static u32 timer_clock;
 
+static void tc_clk_disable(struct clock_event_device *d)
+{
+	struct tc_clkevt_device *tcd = to_tc_clkevt(d);
+
+	clk_disable(tcd->clk);
+	tcd->clk_enabled = false;
+}
+
+static void tc_clk_enable(struct clock_event_device *d)
+{
+	struct tc_clkevt_device *tcd = to_tc_clkevt(d);
+
+	if (tcd->clk_enabled)
+		return;
+	clk_enable(tcd->clk);
+	tcd->clk_enabled = true;
+}
+
 static int tc_shutdown(struct clock_event_device *d)
 {
 	struct tc_clkevt_device *tcd = to_tc_clkevt(d);
@@ -98,8 +117,14 @@ static int tc_shutdown(struct clock_even
 
 	__raw_writel(0xff, regs + ATMEL_TC_REG(2, IDR));
 	__raw_writel(ATMEL_TC_CLKDIS, regs + ATMEL_TC_REG(2, CCR));
+	return 0;
+}
+
+static int tc_shutdown_clk_off(struct clock_event_device *d)
+{
+	tc_shutdown(d);
 	if (!clockevent_state_detached(d))
-		clk_disable(tcd->clk);
+		tc_clk_disable(d);
 
 	return 0;
 }
@@ -112,7 +137,7 @@ static int tc_set_oneshot(struct clock_e
 	if (clockevent_state_oneshot(d) || clockevent_state_periodic(d))
 		tc_shutdown(d);
 
-	clk_enable(tcd->clk);
+	tc_clk_enable(d);
 
 	/* slow clock, count up to RC, then irq and stop */
 	__raw_writel(timer_clock | ATMEL_TC_CPCSTOP | ATMEL_TC_WAVE |
@@ -134,7 +159,7 @@ static int tc_set_periodic(struct clock_
 	/* By not making the gentime core emulate periodic mode on top
 	 * of oneshot, we get lower overhead and improved accuracy.
 	 */
-	clk_enable(tcd->clk);
+	tc_clk_enable(d);
 
 	/* slow clock, count up to RC, then irq and restart */
 	__raw_writel(timer_clock | ATMEL_TC_WAVE | ATMEL_TC_WAVESEL_UP_AUTO,
@@ -168,7 +193,7 @@ static struct tc_clkevt_device clkevt =
 		/* Should be lower than at91rm9200's system timer */
 		.rating			= 125,
 		.set_next_event		= tc_next_event,
-		.set_state_shutdown	= tc_shutdown,
+		.set_state_shutdown	= tc_shutdown_clk_off,
 		.set_state_periodic	= tc_set_periodic,
 		.set_state_oneshot	= tc_set_oneshot,
 	},

Sebastian

Re: arm: at91: do not disable/enable clocks in a row

[Alexandre Belloni]

On 09/03/2016 at 10:58:24 +0100, Sebastian Andrzej Siewior wrote :
> * Alexandre Belloni | 2016-03-08 19:39:10 [+0100]:
> 
> >Hi,
> Hi,
> 
> >Both are things to work on. In the mean time, I'm using the following
> >patch:
> >https://github.com/alexandrebelloni/linux/commit/3a2eae463fce18ae815b887a5c9ca1a657b180ac
> >
> >I understood from
> >http://lkml.iu.edu/hypermail/linux/kernel/1601.2/00941.html that this
> >was not your preferred course of action.
> 
> That was one thing.  What about this:
> 
> ---
> 
> Currently the driver will disable the clock and enable it one line later
> if it is switching from periodic mode into one shot.
> This can be avoided and causes a needless warning on -RT.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

I'm fine with that if you don't want to rely on the clock event state
machine.

Can you submit it to the mainline with:
Tested-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

arm: at91: do not disable/enable clocks in a row

[Alexandre Belloni]

On 09/03/2016 at 10:58:24 +0100, Sebastian Andrzej Siewior wrote :
> * Alexandre Belloni | 2016-03-08 19:39:10 [+0100]:
> 
> >Hi,
> Hi,
> 
> >Both are things to work on. In the mean time, I'm using the following
> >patch:
> >https://github.com/alexandrebelloni/linux/commit/3a2eae463fce18ae815b887a5c9ca1a657b180ac
> >
> >I understood from
> >http://lkml.iu.edu/hypermail/linux/kernel/1601.2/00941.html that this
> >was not your preferred course of action.
> 
> That was one thing.  What about this:
> 
> ---
> 
> Currently the driver will disable the clock and enable it one line later
> if it is switching from periodic mode into one shot.
> This can be avoided and causes a needless warning on -RT.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

I'm fine with that if you don't want to rely on the clock event state
machine.

Can you submit it to the mainline with:
Tested-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Alexandre Belloni]

Hi Thomas,

On 20/01/2016 at 12:07:30 +0100, Thomas Gleixner wrote :
> Well freeing the irq from that context in RT only works because its called
> before SYSTEM_STATE=RUNNING. So no, this was wrong forever.
> 
> The issue we are dealing with is that the timer interrupt is shared with the
> uart. So the timer has IRQ_NO_THREAD set and the uart interrupt gets force
> threaded. So that results in a failure to request the interrupt for the
> UART. That's not RT specific, that already happens in mainline if you add
> 'threadirqs' to the command line.
> 
> So until the DT folks come to senses and we get that dummy demux chip done, I
> came up with the following - completely untested - solution.
> 
> The downside of this is, that the timer will be delayed until the uart thread
> returns, but with the replacement clockevent in place on RT that's a non
> issue. For mainline it's obviously better than what we have now.
> 

I've tested it and it seems to work properly on the few platform where I
can reproduce the issue. What is your plan regarding upstreaming? I
guess you can split and take the resulting patches through your tree.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

[Alexandre Belloni]

Hi Thomas,

On 20/01/2016 at 12:07:30 +0100, Thomas Gleixner wrote :
> Well freeing the irq from that context in RT only works because its called
> before SYSTEM_STATE=RUNNING. So no, this was wrong forever.
> 
> The issue we are dealing with is that the timer interrupt is shared with the
> uart. So the timer has IRQ_NO_THREAD set and the uart interrupt gets force
> threaded. So that results in a failure to request the interrupt for the
> UART. That's not RT specific, that already happens in mainline if you add
> 'threadirqs' to the command line.
> 
> So until the DT folks come to senses and we get that dummy demux chip done, I
> came up with the following - completely untested - solution.
> 
> The downside of this is, that the timer will be delayed until the uart thread
> returns, but with the replacement clockevent in place on RT that's a non
> issue. For mainline it's obviously better than what we have now.
> 

I've tested it and it seems to work properly on the few platform where I
can reproduce the issue. What is your plan regarding upstreaming? I
guess you can split and take the resulting patches through your tree.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com