ep93xx irq-vic regression

ep93xx irq-vic regression

[H Hartley Sweeten]

 Hello all,

Yesterday I tried upgrading my ep93xx system from 3.6.6 to 3.8.3.
The kernel compiles fine but the machine would not boot. It just
hangs when the bootloader (RedBoot) tries to exec the kernel.

I then tried 3.7.0 and it works fine.

I did a bisect and got down to this commit:

$ git bisect bad
07c9249f1fa90cc8189bed44c0bcece664596a72 is the first bad commit
commit 07c9249f1fa90cc8189bed44c0bcece664596a72
Author: Linus Walleij <linus.walleij@linaro.org>
Date:   Tue Oct 16 18:50:00 2012 +0100

    ARM: 7554/1: VIC: use irq_domain_add_simple()

Any ideas on what might be the problem?

Regards,
Hartley

ep93xx irq-vic regression

[Ryan Mallon]

On 16/03/13 08:46, H Hartley Sweeten wrote:

>  Hello all,
> 
> Yesterday I tried upgrading my ep93xx system from 3.6.6 to 3.8.3.
> The kernel compiles fine but the machine would not boot. It just
> hangs when the bootloader (RedBoot) tries to exec the kernel.
> 
> I then tried 3.7.0 and it works fine.
> 
> I did a bisect and got down to this commit:
> 
> $ git bisect bad
> 07c9249f1fa90cc8189bed44c0bcece664596a72 is the first bad commit
> commit 07c9249f1fa90cc8189bed44c0bcece664596a72
> Author: Linus Walleij <linus.walleij@linaro.org>
> Date:   Tue Oct 16 18:50:00 2012 +0100
> 
>     ARM: 7554/1: VIC: use irq_domain_add_simple()
> 
> Any ideas on what might be the problem?


I don't know the code very well, but looking at the change we go
from being having the first vic bank being registered with
irq_domain_add_legacy to irq_domain_add_linear (since first_irq
is zero). It looks like the linear domain needs to have it's
revmap initialised with a call to either irq_domain_associate_many
or irq_create_strict_mappings, which I can't find a call to. Are
we left with the first vic bank having no revmap? That's from a
ten minute, uninformed glance at the code though, so I might be
off base :-).

~Ryan

ep93xx irq-vic regression

[H Hartley Sweeten]

On Friday, March 15, 2013 4:12 PM, Ryan Mallon wrote:
> On 16/03/13 08:46, H Hartley Sweeten wrote:
>
>>  Hello all,
>> 
>> Yesterday I tried upgrading my ep93xx system from 3.6.6 to 3.8.3.
>> The kernel compiles fine but the machine would not boot. It just
>> hangs when the bootloader (RedBoot) tries to exec the kernel.
>> 
>> I then tried 3.7.0 and it works fine.
>> 
>> I did a bisect and got down to this commit:
>> 
>> $ git bisect bad
>> 07c9249f1fa90cc8189bed44c0bcece664596a72 is the first bad commit
>> commit 07c9249f1fa90cc8189bed44c0bcece664596a72
>> Author: Linus Walleij <linus.walleij@linaro.org>
>> Date:   Tue Oct 16 18:50:00 2012 +0100
>> 
>>     ARM: 7554/1: VIC: use irq_domain_add_simple()
>> 
>> Any ideas on what might be the problem?
>
>
> I don't know the code very well, but looking at the change we go
> from being having the first vic bank being registered with
> irq_domain_add_legacy to irq_domain_add_linear (since first_irq
> is zero). It looks like the linear domain needs to have it's
> revmap initialised with a call to either irq_domain_associate_many
> or irq_create_strict_mappings, which I can't find a call to. Are
> we left with the first vic bank having no revmap? That's from a
> ten minute, uninformed glance at the code though, so I might be
> off base :-).

Grrr... chasing my tail...

It looks like the commit above _did_ have a couple bugs but they were
fixed:

commit 07c9249f1fa90cc8189bed44c0bcece664596a72
Author: Linus Walleij <linus.walleij@linaro.org>
Date:   Tue Oct 16 18:50:00 2012 +0100

    ARM: 7554/1: VIC: use irq_domain_add_simple()

commit 946c59a08a2497303750c0fee4367ca32009155c
Author: Russell King <rmk+kernel@arm.linux.org.uk>
Date:   Thu Nov 8 10:48:30 2012 +0000

    ARM: vic: fix build warning caused by previous commit

commit 5ced33bc06e7e76cb8a236f15bff49eb6155b618
Author: Linus Walleij <linus.walleij@linaro.org>
Date:   Wed Dec 26 01:39:16 2012 +0100

    ARM: 7611/1: VIC: fix bug in VIC irqdomain code

It appears the reason my system was hanging is due to this
commit:

commit 210dce5faf89c9677ac1a6273bc53f130843539f
Author: Florian Fainelli <florian@openwrt.org>
Date:   Mon Dec 10 22:21:19 2012 +0100

    ARM: ep93xx: properly wait for UART FIFO to be empty

Reverting that one allows my system to boot with 3.9.0-rc2.

Instead of removing the wait loop completely I propose this
patch instead. I can repost this patch in a new thread if
necessary.

Regards,
Hartley

---

From: H Hartley Sweeten <hsweeten@visionengravers.com>
Subject: [PATCH] arm: ep93xx: fix wait for UART FIFO to be empty

commit 210dce5f "ARM: ep93xx: properly wait for UART FIFO to be empty"

Removed the timeout loop while waiting for the uart transmit fifo to
empty. Some bootloaders leave the uart in a state where there might
be bytes in the uart that are not transmitted when execution is handed
over to the kernel. This results in a deadlocked system while waiting
for the fifo to empty.

Add back the timeout wait to prevent the deadlock.

Increase the wait time to hopefully prevent the decompressor corruption
that lead to commit 210dce5f. This corruption was probably due to a
slow uart baudrate. The 10* increase in the wait time should be enough
for all cases.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: Ryan Mallon <rmallon@gmail.com>
---
 arch/arm/mach-ep93xx/include/mach/uncompress.h | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-ep93xx/include/mach/uncompress.h b/arch/arm/mach-ep93xx/include/mach/uncompress.h
index d2afb4d..b5cc77d 100644
--- a/arch/arm/mach-ep93xx/include/mach/uncompress.h
+++ b/arch/arm/mach-ep93xx/include/mach/uncompress.h
@@ -47,9 +47,13 @@ static void __raw_writel(unsigned int value, unsigned int ptr)
 
 static inline void putc(int c)
 {
-	/* Transmit fifo not full?  */
-	while (__raw_readb(PHYS_UART_FLAG) & UART_FLAG_TXFF)
-		;
+	int i;
+
+	for (i = 0; i < 10000; i++) {
+		/* Transmit fifo not full? */
+		if (!(__raw_readb(PHYS_UART_FLAG) & UART_FLAG_TXFF))
+			break;
+	}
 
 	__raw_writeb(c, PHYS_UART_DATA);
 }
-- 
1.8.1.4

ep93xx irq-vic regression

[Florian Fainelli]

Hello,

Le samedi 16 mars 2013 01:16:52, H Hartley Sweeten a ?crit :
[snip]

> 
>     ARM: ep93xx: properly wait for UART FIFO to be empty
> 
> Reverting that one allows my system to boot with 3.9.0-rc2.
> 
> Instead of removing the wait loop completely I propose this
> patch instead. I can repost this patch in a new thread if
> necessary.
> 
> Regards,
> Hartley
> 
> ---
> 
> From: H Hartley Sweeten <hsweeten@visionengravers.com>
> Subject: [PATCH] arm: ep93xx: fix wait for UART FIFO to be empty
> 
> commit 210dce5f "ARM: ep93xx: properly wait for UART FIFO to be empty"
> 
> Removed the timeout loop while waiting for the uart transmit fifo to
> empty. Some bootloaders leave the uart in a state where there might
> be bytes in the uart that are not transmitted when execution is handed
> over to the kernel. This results in a deadlocked system while waiting
> for the fifo to empty.
> 
> Add back the timeout wait to prevent the deadlock.
> 
> Increase the wait time to hopefully prevent the decompressor corruption
> that lead to commit 210dce5f. This corruption was probably due to a
> slow uart baudrate. The 10* increase in the wait time should be enough
> for all cases.

Ok, your solution seems like it would work, when I come accross this bug I 
initially ended up doing the same thing and incrementing the number of 
iterations in the loop. I was not quite happy with that as it would still be 
highly depending on the clocking. Anyway, sorry for breaking your system with 
this commit.
-- 
Florian

ep93xx irq-vic regression

[Ryan Mallon]

On 17/03/13 00:49, Florian Fainelli wrote:

> Hello,
> 
> Le samedi 16 mars 2013 01:16:52, H Hartley Sweeten a ?crit :
> [snip]
> 
>>
>>     ARM: ep93xx: properly wait for UART FIFO to be empty
>>
>> Reverting that one allows my system to boot with 3.9.0-rc2.
>>
>> Instead of removing the wait loop completely I propose this
>> patch instead. I can repost this patch in a new thread if
>> necessary.
>>
>> Regards,
>> Hartley
>>
>> ---
>>
>> From: H Hartley Sweeten <hsweeten@visionengravers.com>
>> Subject: [PATCH] arm: ep93xx: fix wait for UART FIFO to be empty
>>
>> commit 210dce5f "ARM: ep93xx: properly wait for UART FIFO to be empty"
>>
>> Removed the timeout loop while waiting for the uart transmit fifo to
>> empty. Some bootloaders leave the uart in a state where there might
>> be bytes in the uart that are not transmitted when execution is handed
>> over to the kernel. This results in a deadlocked system while waiting
>> for the fifo to empty.
>>
>> Add back the timeout wait to prevent the deadlock.
>>
>> Increase the wait time to hopefully prevent the decompressor corruption
>> that lead to commit 210dce5f. This corruption was probably due to a
>> slow uart baudrate. The 10* increase in the wait time should be enough
>> for all cases.
> 
> Ok, your solution seems like it would work, when I come accross this bug I 
> initially ended up doing the same thing and incrementing the number of 
> iterations in the loop. I was not quite happy with that as it would still be 
> highly depending on the clocking. Anyway, sorry for breaking your system with 
> this commit.


Do you want to add an Acked/Reviewed-by Florian? I'll queue this
up in my tree if you are happy with it.

~Ryan

ep93xx irq-vic regression

[Florian Fainelli]

On Sunday 17 March 2013 11:06:29 Ryan Mallon wrote:
[snip]
> >> From: H Hartley Sweeten <hsweeten@visionengravers.com>
> >> Subject: [PATCH] arm: ep93xx: fix wait for UART FIFO to be empty
> >>
> >> commit 210dce5f "ARM: ep93xx: properly wait for UART FIFO to be empty"
> >>
> >> Removed the timeout loop while waiting for the uart transmit fifo to
> >> empty. Some bootloaders leave the uart in a state where there might
> >> be bytes in the uart that are not transmitted when execution is handed
> >> over to the kernel. This results in a deadlocked system while waiting
> >> for the fifo to empty.
> >>
> >> Add back the timeout wait to prevent the deadlock.
> >>
> >> Increase the wait time to hopefully prevent the decompressor corruption
> >> that lead to commit 210dce5f. This corruption was probably due to a
> >> slow uart baudrate. The 10* increase in the wait time should be enough
> >> for all cases.
> > 
> > Ok, your solution seems like it would work, when I come accross this bug I 
> > initially ended up doing the same thing and incrementing the number of 
> > iterations in the loop. I was not quite happy with that as it would still 
be 
> > highly depending on the clocking. Anyway, sorry for breaking your system 
with 
> > this commit.
> 
> 
> Do you want to add an Acked/Reviewed-by Florian? I'll queue this
> up in my tree if you are happy with it.

Yes, I just re-tested with 10000 as a loop value, and it fixes the corruption I 
was seeing, you can add my Acked-by: Florian Fainelli <florian@openwrt.org>

Thanks to both of you!
-- 
Florian

ep93xx irq-vic regression

[Linus Walleij]

On Sat, Mar 16, 2013 at 1:16 AM, H Hartley Sweeten
<hartleys@visionengravers.com> wrote:

> It looks like the commit above _did_ have a couple bugs but they were
> fixed:

Oh thanks, I already was scared I had broken something again.
The irqdomain stuff is a bit fragile, everytime I refactor something
it blows up in my face. I hope there are no remaining problems.

Yours,
Linus Walleij

ep93xx irq-vic regression

[H Hartley Sweeten]

On Monday, March 18, 2013 1:22 PM, Linus Walleij wrote:
> On Sat, Mar 16, 2013 at 1:16 AM, H Hartley Sweeten <hartleys@visionengravers.com> wrote:
>
>> It looks like the commit above _did_ have a couple bugs but they were
>> fixed:
>
> Oh thanks, I already was scared I had broken something again.
> The irqdomain stuff is a bit fragile, everytime I refactor something
> it blows up in my face. I hope there are no remaining problems.

Linus,

Sorry about not putting you in the loop with this.

The vic driver works fine on the ep93xx. As I stated, there _was_ a
couple bugs in the driver but they have been fixed.

It appears I was chasing a red herring with the vic driver. It ends up that
my system was not booting due to a patch to the ep93xx uncompress.h
header that was causing a deadlock in the early printk support.

This has been fixed and Ryan Mallon has merged the patch into his
ep93xx-fixes branch.

Regards,
Hartley