6 May
2011
6 May
'11
11:57 p.m.
* convert *_UART_NR to enum uart_type in uart.h
* make uart-functions use uart_type consistently
(was uint8_t and int)
* map uart_type to correct uart for mtk
* (remove forgotten calypso-include for mtk while we are here)
Binaries have been successfully tested with a Sciphone G2 and a Motorola C155.
Signed-off-by: Wolfram Sang <wolfram(a)the-dreams.de>
---
And I just wanted to get rid of some build warnings ;)
src/target/firmware/board/mediatek/uart.c | 31 ++++++++++++---------------
src/target/firmware/calypso/uart.c | 26 +++++++++++-----------
src/target/firmware/include/comm/sercomm.h | 4 ---
src/target/firmware/include/console.h | 3 --
src/target/firmware/include/uart.h | 21 +++++++++++-------
5 files changed, 40 insertions(+), 45 deletions(-)
diff --git a/src/target/firmware/board/mediatek/uart.c
b/src/target/firmware/board/mediatek/uart.c
index ff82245..6a87473 100644
--- a/src/target/firmware/board/mediatek/uart.c
+++ b/src/target/firmware/board/mediatek/uart.c
@@ -36,8 +36,6 @@
#include <comm/sercomm.h>
-#include <calypso/irq.h>
-
/* MT622x */
#if 0
#define BASE_ADDR_UART1 0x80130000
@@ -48,8 +46,7 @@
/* MT 6235 */
#define BASE_ADDR_UART1 0x81030000
-//TODO make UART2 and 3 work
-#define UART_REG(n,m) (BASE_ADDR_UART1 + (m))
+#define UART_REG(n,m) (BASE_ADDR_UART1 + (!n) * 0x10000 + (m))
#define LCR7BIT 0x80
#define LCRBFBIT 0x40
@@ -112,7 +109,7 @@ enum iir_bits {
/* enable or disable the divisor latch for access to DLL, DLH */
-static void uart_set_lcr7bit(int uart, int on)
+static void uart_set_lcr7bit(enum uart_type uart, int on)
{
uint8_t reg;
@@ -125,7 +122,7 @@ static void uart_set_lcr7bit(int uart, int on)
}
static uint8_t old_lcr;
-static void uart_set_lcr_bf(int uart, int on)
+static void uart_set_lcr_bf(enum uart_type uart, int on)
{
if (on) {
old_lcr = readb(UART_REG(uart, LCR));
@@ -136,7 +133,7 @@ static void uart_set_lcr_bf(int uart, int on)
}
/* Enable or disable the TCR_TLR latch bit in MCR[6] */
-static void uart_set_mcr6bit(int uart, int on)
+static void uart_set_mcr6bit(enum uart_type uart, int on)
{
uint8_t mcr;
/* we assume EFR[4] is always set to 1 */
@@ -148,7 +145,7 @@ static void uart_set_mcr6bit(int uart, int on)
writeb(mcr, UART_REG(uart, MCR));
}
-static void uart_reg_write(int uart, enum uart_reg reg, uint8_t val)
+static void uart_reg_write(enum uart_type uart, enum uart_reg reg, uint8_t val)
{
if (reg & LCRBFBIT)
uart_set_lcr_bf(uart, 1);
@@ -168,7 +165,7 @@ static void uart_reg_write(int uart, enum uart_reg reg, uint8_t val)
}
/* read from a UART register, applying any required latch bits */
-static uint8_t uart_reg_read(int uart, enum uart_reg reg)
+static uint8_t uart_reg_read(enum uart_type uart, enum uart_reg reg)
{
uint8_t ret;
@@ -271,7 +268,7 @@ static void uart_irq_handler_sercomm(__unused int irqnr)
}
}
-void uart_init(uint8_t uart, __unused uint8_t interrupts)
+void uart_init(enum uart_type uart, __unused uint8_t interrupts)
{
/* no interrupts, only polling so far */
@@ -302,7 +299,7 @@ void uart_init(uint8_t uart, __unused uint8_t interrupts)
uart_set_lcr7bit(uart, 0);
}
-void uart_poll(uint8_t uart) {
+void uart_poll(enum uart_type uart) {
if(uart == CONS_UART_NR) {
uart_irq_handler_cons(0);
} else {
@@ -310,7 +307,7 @@ void uart_poll(uint8_t uart) {
}
}
-void uart_irq_enable(uint8_t uart, enum uart_irq irq, int on)
+void uart_irq_enable(enum uart_type uart, enum uart_irq irq, int on)
{
uint8_t ier = uart_reg_read(uart, IER);
uint8_t mask = 0;
@@ -333,7 +330,7 @@ void uart_irq_enable(uint8_t uart, enum uart_irq irq, int on)
}
-void uart_putchar_wait(uint8_t uart, int c)
+void uart_putchar_wait(enum uart_type uart, int c)
{
/* wait while TX FIFO indicates full */
while (~readb(UART_REG(uart, LSR)) & 0x20) { }
@@ -342,7 +339,7 @@ void uart_putchar_wait(uint8_t uart, int c)
writeb(c, UART_REG(uart, RBR));
}
-int uart_putchar_nb(uint8_t uart, int c)
+int uart_putchar_nb(enum uart_type uart, int c)
{
/* if TX FIFO indicates full, abort */
if (~readb(UART_REG(uart, LSR)) & 0x20)
@@ -352,7 +349,7 @@ int uart_putchar_nb(uint8_t uart, int c)
return 1;
}
-int uart_getchar_nb(uint8_t uart, uint8_t *ch)
+int uart_getchar_nb(enum uart_type uart, uint8_t *ch)
{
uint8_t lsr;
@@ -379,7 +376,7 @@ int uart_getchar_nb(uint8_t uart, uint8_t *ch)
return 1;
}
-int uart_tx_busy(uint8_t uart)
+int uart_tx_busy(enum uart_type uart)
{
/* Check THRE bit (LSR[5]) to see if FIFO is full */
if (~readb(UART_REG(uart, LSR)) & 0x20)
@@ -409,7 +406,7 @@ static const uint16_t divider[] = {
[UART_921600] = 7, /* would need UART_REG(HIGHSPEED) = 1 */
};
-int uart_baudrate(uint8_t uart, enum uart_baudrate bdrt)
+int uart_baudrate(enum uart_type uart, enum uart_baudrate bdrt)
{
uint16_t div;
diff --git a/src/target/firmware/calypso/uart.c b/src/target/firmware/calypso/uart.c
index d3ede4d..0355ee2 100644
--- a/src/target/firmware/calypso/uart.c
+++ b/src/target/firmware/calypso/uart.c
@@ -112,7 +112,7 @@ enum iir_bits {
#define UART_REG_UIR 0xffff6000
/* enable or disable the divisor latch for access to DLL, DLH */
-static void uart_set_lcr7bit(int uart, int on)
+static void uart_set_lcr7bit(enum uart_type uart, int on)
{
uint8_t reg;
@@ -125,7 +125,7 @@ static void uart_set_lcr7bit(int uart, int on)
}
static uint8_t old_lcr;
-static void uart_set_lcr_bf(int uart, int on)
+static void uart_set_lcr_bf(enum uart_type uart, int on)
{
if (on) {
old_lcr = readb(UART_REG(uart, LCR));
@@ -136,7 +136,7 @@ static void uart_set_lcr_bf(int uart, int on)
}
/* Enable or disable the TCR_TLR latch bit in MCR[6] */
-static void uart_set_mcr6bit(int uart, int on)
+static void uart_set_mcr6bit(enum uart_type uart, int on)
{
uint8_t mcr;
/* we assume EFR[4] is always set to 1 */
@@ -148,7 +148,7 @@ static void uart_set_mcr6bit(int uart, int on)
writeb(mcr, UART_REG(uart, MCR));
}
-static void uart_reg_write(int uart, enum uart_reg reg, uint8_t val)
+static void uart_reg_write(enum uart_type uart, enum uart_reg reg, uint8_t val)
{
if (reg & LCRBFBIT)
uart_set_lcr_bf(uart, 1);
@@ -168,7 +168,7 @@ static void uart_reg_write(int uart, enum uart_reg reg, uint8_t val)
}
/* read from a UART register, applying any required latch bits */
-static uint8_t uart_reg_read(int uart, enum uart_reg reg)
+static uint8_t uart_reg_read(enum uart_type uart, enum uart_reg reg)
{
uint8_t ret;
@@ -276,7 +276,7 @@ static const uint8_t uart2irq[] = {
[1] = IRQ_UART_MODEM,
};
-void uart_init(uint8_t uart, uint8_t interrupts)
+void uart_init(enum uart_type uart, uint8_t interrupts)
{
uint8_t irq = uart2irq[uart];
@@ -330,7 +330,7 @@ void uart_init(uint8_t uart, uint8_t interrupts)
uart_set_lcr7bit(uart, 0);
}
-void uart_poll(uint8_t uart) {
+void uart_poll(enum uart_type uart) {
if(uart == CONS_UART_NR) {
uart_irq_handler_cons(0);
} else {
@@ -338,7 +338,7 @@ void uart_poll(uint8_t uart) {
}
}
-void uart_irq_enable(uint8_t uart, enum uart_irq irq, int on)
+void uart_irq_enable(enum uart_type uart, enum uart_irq irq, int on)
{
uint8_t ier = uart_reg_read(uart, IER);
uint8_t mask = 0;
@@ -361,7 +361,7 @@ void uart_irq_enable(uint8_t uart, enum uart_irq irq, int on)
}
-void uart_putchar_wait(uint8_t uart, int c)
+void uart_putchar_wait(enum uart_type uart, int c)
{
/* wait while TX FIFO indicates full */
while (readb(UART_REG(uart, SSR)) & 0x01) { }
@@ -370,7 +370,7 @@ void uart_putchar_wait(uint8_t uart, int c)
writeb(c, UART_REG(uart, THR));
}
-int uart_putchar_nb(uint8_t uart, int c)
+int uart_putchar_nb(enum uart_type uart, int c)
{
/* if TX FIFO indicates full, abort */
if (readb(UART_REG(uart, SSR)) & 0x01)
@@ -380,7 +380,7 @@ int uart_putchar_nb(uint8_t uart, int c)
return 1;
}
-int uart_getchar_nb(uint8_t uart, uint8_t *ch)
+int uart_getchar_nb(enum uart_type uart, uint8_t *ch)
{
uint8_t lsr;
@@ -407,7 +407,7 @@ int uart_getchar_nb(uint8_t uart, uint8_t *ch)
return 1;
}
-int uart_tx_busy(uint8_t uart)
+int uart_tx_busy(enum uart_type uart)
{
if (readb(UART_REG(uart, SSR)) & 0x01)
return 1;
@@ -423,7 +423,7 @@ static const uint16_t divider[] = {
[UART_921600] = 1, /* 812,500! (-3% would be 893,952) */
};
-int uart_baudrate(uint8_t uart, enum uart_baudrate bdrt)
+int uart_baudrate(enum uart_type uart, enum uart_baudrate bdrt)
{
uint16_t div;
diff --git a/src/target/firmware/include/comm/sercomm.h
b/src/target/firmware/include/comm/sercomm.h
index 54256b5..4c23b31 100644
--- a/src/target/firmware/include/comm/sercomm.h
+++ b/src/target/firmware/include/comm/sercomm.h
@@ -1,12 +1,8 @@
#ifndef _SERCOMM_H
#define _SERCOMM_H
-/* SERCOMM layer on UART1 (modem UART) */
-
#include <osmocom/core/msgb.h>
-#define SERCOMM_UART_NR 1
-
#define HDLC_FLAG 0x7E
#define HDLC_ESCAPE 0x7D
diff --git a/src/target/firmware/include/console.h
b/src/target/firmware/include/console.h
index 7146e99..b5a83f5 100644
--- a/src/target/firmware/include/console.h
+++ b/src/target/firmware/include/console.h
@@ -11,9 +11,6 @@ int cons_putchar(char c);
int cons_rb_flush(void);
void cons_init(void);
-/* We want the console on UART 0 (IRDA UART) */
-#define CONS_UART_NR 0
-
/* Size of the static ring-buffer that we keep for console print messages */
#define CONS_RB_SIZE 4096
diff --git a/src/target/firmware/include/uart.h b/src/target/firmware/include/uart.h
index 81d7a15..eedb006 100644
--- a/src/target/firmware/include/uart.h
+++ b/src/target/firmware/include/uart.h
@@ -3,6 +3,11 @@
#include <stdint.h>
+enum uart_type {
+ CONS_UART_NR,
+ SERCOMM_UART_NR,
+};
+
enum uart_baudrate {
UART_38400,
UART_57600,
@@ -13,20 +18,20 @@ enum uart_baudrate {
UART_921600,
};
-void uart_init(uint8_t uart, uint8_t interrupts);
-void uart_putchar_wait(uint8_t uart, int c);
-int uart_putchar_nb(uint8_t uart, int c);
-int uart_getchar_nb(uint8_t uart, uint8_t *ch);
-int uart_tx_busy(uint8_t uart);
-int uart_baudrate(uint8_t uart, enum uart_baudrate bdrt);
+void uart_init(enum uart_type uart, uint8_t interrupts);
+void uart_putchar_wait(enum uart_type uart, int c);
+int uart_putchar_nb(enum uart_type uart, int c);
+int uart_getchar_nb(enum uart_type uart, uint8_t *ch);
+int uart_tx_busy(enum uart_type uart);
+int uart_baudrate(enum uart_type uart, enum uart_baudrate bdrt);
enum uart_irq {
UART_IRQ_TX_EMPTY,
UART_IRQ_RX_CHAR,
};
-void uart_irq_enable(uint8_t uart, enum uart_irq irq, int on);
+void uart_irq_enable(enum uart_type uart, enum uart_irq irq, int on);
-void uart_poll(uint8_t uart);
+void uart_poll(enum uart_type uart);
#endif /* _UART_H */
--
1.7.2.5
7 May
7 May
7:21 a.m.
Hi,
* convert *_UART_NR to enum uart_type in uart.h
* make uart-functions use uart_type consistently
(was uint8_t and int)
* map uart_type to correct uart for mtk
* (remove forgotten calypso-include for mtk while we are here)
Binaries have been successfully tested with a Sciphone G2 and a Motorola C155.
Signed-off-by: Wolfram Sang <wolfram(a)the-dreams.de>
---
This looks very weird to me ...
The uart paraemeter is not a 'type', it's the index of the UART. And
we use define so we can easily change if we use UART0 or UART1 for the
console / sercomm.
With your modification you have to change the order of the enum if you
want to swap them ? (like it's required on the freerunner)
There is someclean up to be made, but IMHO that's not it at all.
Cheers,
Sylvain
9:15 a.m.
On 07/05/11 07:21, Sylvain Munaut wrote:
Hi,
Sorry, then my descriptive text was not good enough :(
* convert *_UART_NR to enum uart_type in uart.h
* make uart-functions use uart_type consistently
(was uint8_t and int)
* map uart_type to correct uart for mtk
* (remove forgotten calypso-include for mtk while we are here)
Binaries have been successfully tested with a Sciphone G2 and a Motorola C155.
Signed-off-by: Wolfram Sang<wolfram(a)the-dreams.de>
---
This looks very weird to me ... The uart paraemeter is not a 'type', it's
the index of the UART. And
we use define so we can easily change if we use UART0 or UART1 for the
console / sercomm.
That was what I intentionally wanted to change, because it caused
problems for MTK.
With your modification you have to change the order of
the enum if you
want to swap them ? (like it's required on the freerunner)
The enums should never be changed; that is the meaning of it all. The
idea is to have consistent ids for the type of uart across the whole
tree and have it mapped to the correct uart only in the platform
specific uart.c just before accessing the registers. So, this is some
kind of virtual UART_NR which gets mapped to the internal uart very
late. Probaby they should be renamed to simply CONS_UART (instead of
CONS_UART_NR) then.
MTK also needs them to be swapped. This is why I can't first compile
'firmware' and then 'mtk-firmware', because lib/console.c won't get
recompiled and will still use the non-swapped *_UART_NR. And IMO it
should not be recompiled, thus the added layer of abstraction.
The freerunner should have the same problem, but I can't find the code
which swaps the uarts?
There is someclean up to be made, but IMHO that's
not it at all.
Yup, found some local uart-variables. Do you mean that?
Thanks,
Wolfram
10:04 a.m.
The enums should never be changed; that is the meaning
of it all. The idea
is to have consistent ids for the type of uart across the whole tree and
have it mapped to the correct uart only in the platform specific uart.c
But it is not "platform" specific, it is _board_ specific. So the same
driver uart.c must deal with several mapping.
IMHO drivers should get as parameter the index of the device you want
to address and not some global id and map it internally to the driver.
What if I want for debug to output something on UART3 or something,
adding something like uart_putc(3, 'b') is much easier than having to
go add a new value for the enum and its mapping ...
If something doesn't get recompiled when a header change or something,
you must fix the build system.
The current system of trying to build for all platforms at once is
unsustainable IMHO and we should just have something like
make TARGET=compal_e88 make TARGET=sciphone_g2 or something and if
the TARGET is != from the last built target, just assume everything is
dirty.
Cheers,
Sylvain
11:04 a.m.
On 07/05/11 10:04, Sylvain Munaut wrote:
Yes, I know. Ultimately, there should be something like "#include
<mach/uart.h>" which will pull in the correct settings per board. But
that seemed overkill to me for now (seeing that there might be a move to
an RTOS anyway). I thought board differences can be easier handled on a
per-platform basis until then.
The enums
should never be changed; that is the meaning of it all. The idea
is to have consistent ids for the type of uart across the whole tree and
have it mapped to the correct uart only in the platform specific uart.c
But it is not "platform" specific, it is _board_ specific. So the same
driver uart.c must deal with several mapping. IMHO drivers should get as parameter the index of the
device you want
to address and not some global id and map it internally to the driver.
What if I want for debug to output something on UART3 or something,
adding something like uart_putc(3, 'b') is much easier than having to
go add a new value for the enum and its mapping ...
That is easier, but not portable. What if some other board doesn't have
a UART3? Adding MY_DEBUG_UART does not really hurt IMHO and other boards
will notice that there is something to take care of.
If something doesn't get recompiled when a header
change or something,
you must fix the build system.
This is true, of course...
The current system of trying to build for all
platforms at once is
unsustainable IMHO and we should just have something like
make TARGET=compal_e88 make TARGET=sciphone_g2 or something and if
... right, too ...
the TARGET is != from the last built target, just
assume everything is
dirty.
... though in my book, lib/console.c should not recompile if the
underlying platform changes (arch is something different, of course ;)).
If lib/* depends on the platform (like with UART numbering), to me this
sounds like something to be fixed.
I don't feel strong about this patch, though. If it is not good enough,
we can just skip it. I start to wonder if it makes sense to do such kind
of work before the RTOS comes along?
Regards,
Wolfram
11:37 a.m.
... though in my book, lib/console.c should not
recompile if the
underlying platform changes (arch is something different, of course ;)).
If lib/* depends on the platform (like with UART numbering), to me this
sounds like something to be fixed.
IMHO board startup functions shoudl have a call like
console_init( XXX ) with XXX being the uart index to start the console on.
I don't feel strong about this patch, though. If
it is not good enough,
we can just skip it.
Yes sorry but I really don't like the drivers being put in charge of a
mapping.
I start to wonder if it makes sense to do such kind of
work before the
RTOS comes along?
Indeed, good question ...
Cheers,
Sylvain Munaut
9 May
9 May
6:56 p.m.
I start to
wonder if it makes sense to do such kind of work before
the RTOS comes along?
Indeed, good question ... 
7 May
7 May
12:10 p.m.
New subject: build system / building for all targets
On Sat, May 07, 2011 at 10:04:25AM +0200, Sylvain Munaut wrote:
The current system of trying to build for all
platforms at once is
unsustainable IMHO and we should just have something like
make TARGET=compal_e88 make TARGET=sciphone_g2 or something and if
the TARGET is != from the last built target, just assume everything is
dirty.
I intentionally did it the way it currently is: to ensure we don't have
bit-rot of non-building code in the tree. So whatever we do in the
future, especially with the small code size we deal, I would like to
keep it the way that by default we build every image for every target.
We can have a TARGET=xxx as an option. This way the user can select to
deviate from the default, i.e. an optimization.
I understand your concern is not about build time but about the fact
that different boards might #define things differently. This indeed
needs to be fixed, and I think we should then probably put our .o files
out of the source tree, and dynamically create a build-compal_e88
directory which contains all the compiler output for that target.
If anyone wants to work on something like this, I would be happy to
merge the patches.
But from my point of view the default make file target should then still
build all the .c files for all the targets.
Regards,
Harald
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
12:13 p.m.
Regarding the UART numbers. My view on this is:
1) I agree with Sylvain that the driver should simply take a uart-number
as input, and what that number maps to should be SoC-specific.
2) We should either
a) have a #define for every board (not SoC), which is something like
#define CONS_UART_NR 0" and which is then used in the actual
callers of the uart driver, or
b) have a global board-configuration structure which gets filled in by the
board_init() code, which then contains a boardcf->cons_uart_nr
member whihc can be used by the callers of the uart driver.
'2a' requires that we modify our build system to make per-board
compiles.
Regards,
Harald
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
18 May
18 May
7:11 a.m.
On Sat, May 07, 2011 at 12:13:50PM +0200 Harald Welte wrote:
Date: Sat, May 07, 2011 at 12:13:50PM +0200
From: Harald Welte <laforge(a)gnumonks.org>
To:
Subject: [PATCH] uart: make *_UART_NR platform independent
Hi,
b) have a global board-configuration structure
which gets filled in by the
board_init() code, which then contains a boardcf->cons_uart_nr
member whihc can be used by the callers of the uart driver.
attached is my local patch I tried to get merged some time ago that
implements the board structure. It needs no active initialization and
has default values (see patch).
Cheers Leif
4735
days inactive
4747
days old
baseband-devel@lists.osmocom.org
9 comments
5 participants
participants (5)
-
246tnt＠gmail.com -
Harald Welte -
Leif Jakob -
Sylvain Munaut
-
Wolfram Sang