* 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@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
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@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
On 07/05/11 07:21, Sylvain Munaut wrote:
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@the-dreams.de> ---
This looks very weird to me ...
Sorry, then my descriptive text was not good enough :(
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
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
On 07/05/11 10:04, Sylvain Munaut wrote:
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.
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.
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
... 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
I start to wonder if it makes sense to do such kind of work before the RTOS comes along?
Indeed, good question ...
Having thought about this issue every now and then, I currently feel more tempted to port the g2-loader to NuttX, just to have it tried and see how that ends up. Working on the current bare metal-infrastructure for multi-platform feels like a dead end somehow. Wolfram PS: I'll be in Berlin for LinuxTag from Wed to Sat. So, if somebody is interested to meet somewhen/somewhere and chat about RTOS/infrastructure (or just chat ;)), I am all for it!
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@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
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@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
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@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
participants (5)
-
246tnt＠gmail.com -
Harald Welte -
Leif Jakob -
Sylvain Munaut
-
Wolfram Sang