Hi,
good news: mails are back bad news: phone lost a battery pin :(
Here are two small patches to help the mediatek-branch compile without calypso references. These were tested before the breakage and 'make all' still builds fine for compal.
Regards,
Wolfram
Wolfram Sang (2): comm: msgb: don't set backlight on error lib: move delay.c from calypso to lib
src/target/firmware/calypso/Makefile | 2 +- src/target/firmware/calypso/delay.c | 16 ---------------- src/target/firmware/comm/msgb.c | 1 - src/target/firmware/lib/Makefile | 2 +- src/target/firmware/lib/delay.c | 16 ++++++++++++++++ 5 files changed, 18 insertions(+), 19 deletions(-) delete mode 100644 src/target/firmware/calypso/delay.c create mode 100644 src/target/firmware/lib/delay.c
It seems just to be a debugging aid, but brings in an unwanted calypso-dependency.
Signed-off-by: Wolfram Sang wolfram@the-dreams.de Cc: Harald Welte laforge@gnumonks.org --- src/target/firmware/comm/msgb.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/src/target/firmware/comm/msgb.c b/src/target/firmware/comm/msgb.c index ff18e65..792abc6 100644 --- a/src/target/firmware/comm/msgb.c +++ b/src/target/firmware/comm/msgb.c @@ -60,7 +60,6 @@ void *_talloc_zero(void *ctx, unsigned int size, const char *name) } } cons_puts("unable to allocate msgb\n"); - bl_level(++i % 50); delay_ms(50); } panic:
Hi,
On Sat, Apr 30, 2011 at 9:53 AM, Wolfram Sang wolfram@the-dreams.de wrote:
It seems just to be a debugging aid, but brings in an unwanted calypso-dependency.
Well it is :)
I suggest you just replace it by a 'board_panic' call or something and leave it to the board specific code how to signal panic conditions.
Cheers,
Sylvain
On 30/04/11 10:12, Sylvain Munaut wrote:
Hi,
On Sat, Apr 30, 2011 at 9:53 AM, Wolfram Sangwolfram@the-dreams.de wrote:
It seems just to be a debugging aid, but brings in an unwanted calypso-dependency.
Well it is :)
I suggest you just replace it by a 'board_panic' call or something and leave it to the board specific code how to signal panic conditions.
I had another look and still have the impression that backlight usage should just go. Look at the implementation when the file was introduced:
+panic: + while (1) { + bl_level(++i % 50); + delay_ms(50); + } + return NULL;
This, I understand, it blinks. But it was changed three days later to the current state (c917fd43797c30385a1ba860fef95be97c84d51d) which broke it ('i' is constant now). So it is broken for over a year and nobody noticed :) So, instead of introducing a board_panic which nobody seems to need, I'd just suggest using plain osmo_panic as in other places. Much cleaner, too, IMO.
Regards,
Wolfram
noticed :) So, instead of introducing a board_panic which nobody seems to need, I'd just suggest using plain osmo_panic as in other places. Much cleaner, too, IMO.
Ehrm, considering the while(1)-loop in the current form, this isn't even a panic? So just print the msg as is now? Confused...
Hi Wolfgang,
On Sun, May 01, 2011 at 10:18:04AM +0200, Wolfram Sang wrote:
This, I understand, it blinks. But it was changed three days later to the current state (c917fd43797c30385a1ba860fef95be97c84d51d) which broke it ('i' is constant now). So it is broken for over a year and nobody noticed :)
Well, let's say we noticed that we get layer1 panics without any indication.
So, instead of introducing a board_panic which nobody seems to need, I'd just suggest using plain osmo_panic as in other places. Much cleaner, too, IMO.
I disagree. We desperately need board_panic, (with much more than just backlight blinking in the future), to be able to solve the various mysterious layer1/firmware crashes that we're seeing more or less all the time.
I had recently discussed this with Andreas and Sylvain at the Easterhegg, and they both agreed that we need a way to debug crashes better. We need to tap into the various exception handlers, we need to use the debug unit to get backtraces, and we need ways like backlight blinking or buzzer sounds that can be done from a few lines of stack-independent assembly code to reliably indicate what has happened to the system.
Regards, Harald
Hi,
I disagree. We desperately need board_panic, (with much more than just backlight blinking in the future), to be able to solve the various mysterious layer1/firmware crashes that we're seeing more or less all the time.
Yes, it definitely needed.
And calling osmo_panic might be the right thing, and then have the board register their osmo_panic handler in its init (the buzzer would be a good choice :)
(libosmocore already has a way to register a panic handler IIRC which is for the target a while(1) loop by default)
Cheers,
Sylvain
On 02/05/11 14:02, Sylvain Munaut wrote:
I disagree. We desperately need board_panic, (with much more than just backlight blinking in the future), to be able to solve the various mysterious layer1/firmware crashes that we're seeing more or less all the time.
Yes, it definitely needed.
And calling osmo_panic might be the right thing, and then have the board register their osmo_panic handler in its init (the buzzer would be a good choice :)
Okay, understood that it is needed. Still, the discussion also showed that I seem to not have the proper knowledge of what is desired instead by those people hacking at layer 1 (what I don't do) :) Thus, may I suggest that I just remove the (static value) backlight setting in msgb.c to remove the calypso-dependency and leave a proper board_panic for someone else with a seperate patchset? To me, Harald's post indicates that it is a seperate topic?
Regards,
Wolfram
On Mon, May 02, 2011 at 02:27:03PM +0200, Wolfram Sang wrote:
Okay, understood that it is needed. Still, the discussion also showed that I seem to not have the proper knowledge of what is desired instead by those people hacking at layer 1 (what I don't do) :) Thus, may I suggest that I just remove the (static value) backlight setting in msgb.c to remove the calypso-dependency and leave a proper board_panic for someone else with a seperate patchset? To me, Harald's post indicates that it is a seperate topic?
ACK.
Nothing calypso-related in there and needed for Mediatek, too.
Signed-off-by: Wolfram Sang wolfram@the-dreams.de --- src/target/firmware/calypso/Makefile | 2 +- src/target/firmware/calypso/delay.c | 16 ---------------- src/target/firmware/lib/Makefile | 2 +- src/target/firmware/lib/delay.c | 16 ++++++++++++++++ 4 files changed, 18 insertions(+), 18 deletions(-) delete mode 100644 src/target/firmware/calypso/delay.c create mode 100644 src/target/firmware/lib/delay.c
diff --git a/src/target/firmware/calypso/Makefile b/src/target/firmware/calypso/Makefile index c0620ed..610a82c 100644 --- a/src/target/firmware/calypso/Makefile +++ b/src/target/firmware/calypso/Makefile @@ -1,4 +1,4 @@
LIBRARIES+=calypso calypso_DIR=calypso -calypso_SRCS=arm.c buzzer.c clock.c delay.c dma.c dsp.c du.c i2c.c irq.c rtc.c sim.c spi.c tpu.c tsp.c keypad.c misc.c timer.c backlight.c uart.c uwire.c +calypso_SRCS=arm.c buzzer.c clock.c dma.c dsp.c du.c i2c.c irq.c rtc.c sim.c spi.c tpu.c tsp.c keypad.c misc.c timer.c backlight.c uart.c uwire.c diff --git a/src/target/firmware/calypso/delay.c b/src/target/firmware/calypso/delay.c deleted file mode 100644 index 443ca82..0000000 --- a/src/target/firmware/calypso/delay.c +++ /dev/null @@ -1,16 +0,0 @@ -#include <delay.h> - -/* FIXME: We need properly calibrated delay loops at some point! */ -void delay_us(unsigned int us) -{ - volatile unsigned int i; - - for (i= 0; i < us*4; i++) { i; } -} - -void delay_ms(unsigned int ms) -{ - volatile unsigned int i; - - for (i= 0; i < ms*1300; i++) { i; } -} diff --git a/src/target/firmware/lib/Makefile b/src/target/firmware/lib/Makefile index 987857c..83f9966 100644 --- a/src/target/firmware/lib/Makefile +++ b/src/target/firmware/lib/Makefile @@ -2,6 +2,6 @@ LIBRARIES+=mini mini_DIR=lib mini_SRCS=vsprintf.c string.c ctype.c printf.c console.c ctors.c \ - changebit.S clearbit.S div64.S lib1funcs.S memcpy.S memset.S setbit.S testchangebit.S testclearbit.S testsetbit.S + changebit.S clearbit.S delay.c div64.S lib1funcs.S memcpy.S memset.S setbit.S testchangebit.S testclearbit.S testsetbit.S
diff --git a/src/target/firmware/lib/delay.c b/src/target/firmware/lib/delay.c new file mode 100644 index 0000000..443ca82 --- /dev/null +++ b/src/target/firmware/lib/delay.c @@ -0,0 +1,16 @@ +#include <delay.h> + +/* FIXME: We need properly calibrated delay loops at some point! */ +void delay_us(unsigned int us) +{ + volatile unsigned int i; + + for (i= 0; i < us*4; i++) { i; } +} + +void delay_ms(unsigned int ms) +{ + volatile unsigned int i; + + for (i= 0; i < ms*1300; i++) { i; } +}
baseband-devel@lists.osmocom.org
-
Harald Welte -
Sylvain Munaut -
Wolfram Sang