| Message ID | f65f6ebaedf65e31a811e079a417001f50ae9d89.1367188423.git.julien.grall@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Mon, 2013-04-29 at 00:02 +0100, Julien Grall wrote: > Signed-off-by: Julien Grall <julien.grall@linaro.org> Can you arrange to share a header with the non-early version and avoid the opencoded register and bit definitions? > --- > config/arm32.mk | 1 + > xen/arch/arm/Rules.mk | 4 ++ > xen/arch/arm/arm32/Makefile | 1 + > xen/arch/arm/arm32/debug-exynos5.S | 81 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 87 insertions(+) > create mode 100644 xen/arch/arm/arm32/debug-exynos5.S > > diff --git a/config/arm32.mk b/config/arm32.mk > index 593a1d1..01c1490 100644 > --- a/config/arm32.mk > +++ b/config/arm32.mk > @@ -17,6 +17,7 @@ CFLAGS += -marm > # Possible value: > # - none: no early printk > # - pl011: printk with PL011 UART > +# - exynos5: printk with the second exynos 5 UART From the subject I infer that this UART is an Exynos 4210? If so I think that is the correct name to use here, consistent with the pl011 name above. (the alternative is that the pl011 is wrong...). I suppose in principal a single driver might apply to multiple platforms but with different base addresses, but that at build time you need to choose a specific platform, which implies a particular driver. The current infrastructure doesn't really express that notion. What about if we support a mutually exclusive (enforced only by the clash of symbols at link time) set of: CONFIG_EARLY_UART_PL011 CONFIG_EARLY_UART_EXYNOS4210 which if defined must be set to the physical address of the port? This would mean users need to know the address of the UART on their platform rather than just being able to name the platform though. Perhaps that's a feature not a bug (i.e. they can choose which UART of multiple UARTs if they want). I suppose we could also support CONFIG_EARY_UART_PLATFORM=foo and translate into the above. Or are there other hardcoded parameters (e.g. crystal rate) which stop this? > CONFIG_EARLY_PRINTK := none > HAS_PL011 := y > HAS_EXYNOS5 := y > diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk > index 2053b1e..7dcc0b7 100644 > --- a/xen/arch/arm/Rules.mk > +++ b/xen/arch/arm/Rules.mk > @@ -43,5 +43,9 @@ ifeq ($(CONFIG_EARLY_PRINTK), pl011) > EARLY_PRINTK := y > CONFIG_EARLY_PL011 := y > endif > +ifeq ($(CONFIG_EARLY_PRINTK), exynos5) > +EARLY_PRINTK := y > +CONFIG_EARLY_EXYNOS5 := y > +endif > > CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK > diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile > index 6af8ca3..90e4eab 100644 > --- a/xen/arch/arm/arm32/Makefile > +++ b/xen/arch/arm/arm32/Makefile > @@ -9,3 +9,4 @@ obj-y += domain.o > > obj-$(EARLY_PRINTK) += debug.o > obj-$(CONFIG_EARLY_PL011) += debug-pl011.o > +obj-$(CONFIG_EARLY_EXYNOS5) += debug-exynos5.o > diff --git a/xen/arch/arm/arm32/debug-exynos5.S b/xen/arch/arm/arm32/debug-exynos5.S > new file mode 100644 > index 0000000..cbe1705 > --- /dev/null > +++ b/xen/arch/arm/arm32/debug-exynos5.S > @@ -0,0 +1,81 @@ > +/* > + * xen/arch/arm/arm32/debug-exynos5.S > + * > + * Exynos 5 specific debug code > + * > + * Copyright (c) 2013 Citrix Systems. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <asm/asm_defns.h> > + > +#define EXYNOS5_UART_BASE_ADDRESS 0x12c20000 > + > +.globl early_uart_paddr > +early_uart_paddr: .word EXYNOS5_UART_BASE_ADDRESS > + > +/* Exynos 5 UART initialization > + * r11: UART base address > + * Clobber r0-r1 */ > +.globl early_uart_init > +early_uart_init: > + /* init clock */ > + ldr r1, =0x10020000 > + /* select MPLL (800MHz) source clock */ > + ldr r0, [r1, #0x250] > + and r0, r0, #(~(0xf<<8)) > + orr r0, r0, #(0x6<<8) > + str r0, [r1, #0x250] > + /* ration 800/(7+1) */ What does "ration" mean here? It doesn't seem to correspond to the French for denominator, numerator, remainder or ratio (at least not according to Google Translate ;-)) > + ldr r0, [r1, #0x558] > + and r0, r0, #(~(0xf<<8)) > + orr r0, r0, #(0x7<<8) > + str r0, [r1, #0x558] > + > + mov r1, #4 > + str r1, [r11, #0x2c] /* -> UARTIBRD (Baud divisor fraction) */ > + mov r1, #53 > + str r1, [r11, #0x28] /* -> UARTIBRD (Baud divisor integer) */ > + mov r1, #3 /* 8n1 */ > + str r1, [r11, #0x0] /* -> (Line control) */ > + ldr r1, =(1<<2) /* TX IRQMODE */ > + str r1, [r11, #0x4] /* -> (Control Register) */ > + mov r1, #0x0 > + str r1, [r11, #0x8] /* disable FIFO */ > + mov r1, #0x0 > + str r1, [r11, #0x0C] /* no auto flow control */ > + mov pc, lr > + > +/* Exynos 5 UART wait UART to be ready to transmit > + * r11: UART base address > + * Clobber r2 r11 */ > +.globl early_uart_ready > +early_uart_ready: > + ldr r2, [r11, #0x10] /* <- UTRSTAT (Flag register) */ > + tst r2, #(1<<1) /* Check BUSY bit */ > + beq early_uart_ready /* Wait for the UART to be ready */ > + mov pc, lr > + > +/* Exynos 5 UART transmit character > + * r2: character to transmit > + * r11: UART base address */ > +.globl early_uart_transmit > +early_uart_transmit: > + str r2, [r11, #0x20] /* -> UTXH (Data Register) */ > + mov pc, lr > + > +/* > + * Local variables: > + * mode: ASM > + * indent-tabs-mode: nil > + * End: > + */
On 30/04/13 10:53, Ian Campbell wrote: > On Mon, 2013-04-29 at 00:02 +0100, Julien Grall wrote: >> --- >> config/arm32.mk | 1 + >> xen/arch/arm/Rules.mk | 4 ++ >> xen/arch/arm/arm32/Makefile | 1 + >> xen/arch/arm/arm32/debug-exynos5.S | 81 ++++++++++++++++++++++++++++++++++++ >> 4 files changed, 87 insertions(+) >> create mode 100644 xen/arch/arm/arm32/debug-exynos5.S >> >> diff --git a/config/arm32.mk b/config/arm32.mk >> index 593a1d1..01c1490 100644 >> --- a/config/arm32.mk >> +++ b/config/arm32.mk >> @@ -17,6 +17,7 @@ CFLAGS += -marm >> # Possible value: >> # - none: no early printk >> # - pl011: printk with PL011 UART >> +# - exynos5: printk with the second exynos 5 UART > > From the subject I infer that this UART is an Exynos 4210? If so I think > that is the correct name to use here, consistent with the pl011 name > above. (the alternative is that the pl011 is wrong...). > > I suppose in principal a single driver might apply to multiple platforms > but with different base addresses, but that at build time you need to > choose a specific platform, which implies a particular driver. The > current infrastructure doesn't really express that notion. > > What about if we support a mutually exclusive (enforced only by the > clash of symbols at link time) set of: > CONFIG_EARLY_UART_PL011 > CONFIG_EARLY_UART_EXYNOS4210 > which if defined must be set to the physical address of the port? > > This would mean users need to know the address of the UART on their > platform rather than just being able to name the platform though. > Perhaps that's a feature not a bug (i.e. they can choose which UART of > multiple UARTs if they want). I suppose we could also support > CONFIG_EARY_UART_PLATFORM=foo and translate into the above. > > Or are there other hardcoded parameters (e.g. crystal rate) which stop > this? > This drivers have been written with the Exynos 5 platform in mind, so I'm not sure it can be called exynos4210. There would be few differences that I can extract from Linux source code, the physical address is different and the few bit to use FIFO are also different but it's not use here in the early printk. And I'm almost sure the clock stuff would be different on an exynos4. So, in my humble opinion, we should use exynos5 or exynos5250. >> +/* Exynos 5 UART initialization >> + * r11: UART base address >> + * Clobber r0-r1 */ >> +.globl early_uart_init >> +early_uart_init: >> + /* init clock */ >> + ldr r1, =0x10020000 >> + /* select MPLL (800MHz) source clock */ >> + ldr r0, [r1, #0x250] >> + and r0, r0, #(~(0xf<<8)) >> + orr r0, r0, #(0x6<<8) >> + str r0, [r1, #0x250] >> + /* ration 800/(7+1) */ > > What does "ration" mean here? It doesn't seem to correspond to the > French for denominator, numerator, remainder or ratio (at least not > according to Google Translate ;-)) It mean there is a typo ;), someone or something add a N. I suspect it's something (the keyboard) :).
On 29/04/13 00:02, Julien Grall wrote: > diff --git a/xen/arch/arm/arm32/debug-exynos5.S b/xen/arch/arm/arm32/debug-exynos5.S > new file mode 100644 > index 0000000..cbe1705 > --- /dev/null > +++ b/xen/arch/arm/arm32/debug-exynos5.S > @@ -0,0 +1,81 @@ > +/* > + * xen/arch/arm/arm32/debug-exynos5.S > + * > + * Exynos 5 specific debug code > + * > + * Copyright (c) 2013 Citrix Systems. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <asm/asm_defns.h> > + > +#define EXYNOS5_UART_BASE_ADDRESS 0x12c20000 > + > +.globl early_uart_paddr > +early_uart_paddr: .word EXYNOS5_UART_BASE_ADDRESS > + > +/* Exynos 5 UART initialization > + * r11: UART base address > + * Clobber r0-r1 */ > +.globl early_uart_init > +early_uart_init: > + /* init clock */ > + ldr r1, =0x10020000 > + /* select MPLL (800MHz) source clock */ > + ldr r0, [r1, #0x250] > + and r0, r0, #(~(0xf<<8)) > + orr r0, r0, #(0x6<<8) > + str r0, [r1, #0x250] > + /* ration 800/(7+1) */ > + ldr r0, [r1, #0x558] > + and r0, r0, #(~(0xf<<8)) > + orr r0, r0, #(0x7<<8) > + str r0, [r1, #0x558] > + > + mov r1, #4 > + str r1, [r11, #0x2c] /* -> UARTIBRD (Baud divisor fraction) */ Could you replace UARTIBRD by UFRACVAL? The former is the name for the PL011, and the latter is the name given by the Exynos 5 manual. > + mov r1, #53 > + str r1, [r11, #0x28] /* -> UARTIBRD (Baud divisor integer) */ Same here with UBRDIV.
On Wed, 2013-05-01 at 18:17 +0100, Anthony PERARD wrote: > On 30/04/13 10:53, Ian Campbell wrote: > > On Mon, 2013-04-29 at 00:02 +0100, Julien Grall wrote: > >> --- > >> config/arm32.mk | 1 + > >> xen/arch/arm/Rules.mk | 4 ++ > >> xen/arch/arm/arm32/Makefile | 1 + > >> xen/arch/arm/arm32/debug-exynos5.S | 81 ++++++++++++++++++++++++++++++++++++ > >> 4 files changed, 87 insertions(+) > >> create mode 100644 xen/arch/arm/arm32/debug-exynos5.S > >> > >> diff --git a/config/arm32.mk b/config/arm32.mk > >> index 593a1d1..01c1490 100644 > >> --- a/config/arm32.mk > >> +++ b/config/arm32.mk > >> @@ -17,6 +17,7 @@ CFLAGS += -marm > >> # Possible value: > >> # - none: no early printk > >> # - pl011: printk with PL011 UART > >> +# - exynos5: printk with the second exynos 5 UART > > > > From the subject I infer that this UART is an Exynos 4210? If so I think > > that is the correct name to use here, consistent with the pl011 name > > above. (the alternative is that the pl011 is wrong...). > > > > I suppose in principal a single driver might apply to multiple platforms > > but with different base addresses, but that at build time you need to > > choose a specific platform, which implies a particular driver. The > > current infrastructure doesn't really express that notion. > > > > What about if we support a mutually exclusive (enforced only by the > > clash of symbols at link time) set of: > > CONFIG_EARLY_UART_PL011 > > CONFIG_EARLY_UART_EXYNOS4210 > > which if defined must be set to the physical address of the port? > > > > This would mean users need to know the address of the UART on their > > platform rather than just being able to name the platform though. > > Perhaps that's a feature not a bug (i.e. they can choose which UART of > > multiple UARTs if they want). I suppose we could also support > > CONFIG_EARY_UART_PLATFORM=foo and translate into the above. > > > > Or are there other hardcoded parameters (e.g. crystal rate) which stop > > this? > > > > This drivers have been written with the Exynos 5 platform in mind, so > I'm not sure it can be called exynos4210. Maybe I'm confused, I had assumed that exynos4210 was a UART logic block which happened to have been dropped down onto the exynos5 SoC in much the same way that the Cortex-A15 contains several pl011 UART logic blocks. Is this not the case? In exynos5250.dtsi in Linux the UART is described as compatible = "samsung,exynos4210-uart", is 4210 actually a previous revision of the Exynos processor which happens to have a compatible UART on it? What I want to avoid is the case where we have dozens of UART drivers which are all identical apart from the fact that they happen to be integrated into a different SoC (all the other Exynos5xxx variants for example), so I'd like to determine the most generic name for this driver possible. > There would be few differences > that I can extract from Linux source code, the physical address is > different and the few bit to use FIFO are also different but it's not > use here in the early printk. And I'm almost sure the clock stuff would > be different on an exynos4. > > So, in my humble opinion, we should use exynos5 or exynos5250. > > > >> +/* Exynos 5 UART initialization > >> + * r11: UART base address > >> + * Clobber r0-r1 */ > >> +.globl early_uart_init > >> +early_uart_init: > >> + /* init clock */ > >> + ldr r1, =0x10020000 > >> + /* select MPLL (800MHz) source clock */ > >> + ldr r0, [r1, #0x250] > >> + and r0, r0, #(~(0xf<<8)) > >> + orr r0, r0, #(0x6<<8) > >> + str r0, [r1, #0x250] > >> + /* ration 800/(7+1) */ > > > > What does "ration" mean here? It doesn't seem to correspond to the > > French for denominator, numerator, remainder or ratio (at least not > > according to Google Translate ;-)) > > It mean there is a typo ;), someone or something add a N. I suspect it's > something (the keyboard) :). I should have thought of that! ;-) Ian.
On 02/05/13 08:58, Ian Campbell wrote: > On Wed, 2013-05-01 at 18:17 +0100, Anthony PERARD wrote: >> > On 30/04/13 10:53, Ian Campbell wrote: >>> > > On Mon, 2013-04-29 at 00:02 +0100, Julien Grall wrote: >>>> > >> --- >>>> > >> config/arm32.mk | 1 + >>>> > >> xen/arch/arm/Rules.mk | 4 ++ >>>> > >> xen/arch/arm/arm32/Makefile | 1 + >>>> > >> xen/arch/arm/arm32/debug-exynos5.S | 81 ++++++++++++++++++++++++++++++++++++ >>>> > >> 4 files changed, 87 insertions(+) >>>> > >> create mode 100644 xen/arch/arm/arm32/debug-exynos5.S >>>> > >> >>>> > >> diff --git a/config/arm32.mk b/config/arm32.mk >>>> > >> index 593a1d1..01c1490 100644 >>>> > >> --- a/config/arm32.mk >>>> > >> +++ b/config/arm32.mk >>>> > >> @@ -17,6 +17,7 @@ CFLAGS += -marm >>>> > >> # Possible value: >>>> > >> # - none: no early printk >>>> > >> # - pl011: printk with PL011 UART >>>> > >> +# - exynos5: printk with the second exynos 5 UART >>> > > >>> > > From the subject I infer that this UART is an Exynos 4210? If so I think >>> > > that is the correct name to use here, consistent with the pl011 name >>> > > above. (the alternative is that the pl011 is wrong...). >>> > > >>> > > I suppose in principal a single driver might apply to multiple platforms >>> > > but with different base addresses, but that at build time you need to >>> > > choose a specific platform, which implies a particular driver. The >>> > > current infrastructure doesn't really express that notion. >>> > > >>> > > What about if we support a mutually exclusive (enforced only by the >>> > > clash of symbols at link time) set of: >>> > > CONFIG_EARLY_UART_PL011 >>> > > CONFIG_EARLY_UART_EXYNOS4210 >>> > > which if defined must be set to the physical address of the port? >>> > > >>> > > This would mean users need to know the address of the UART on their >>> > > platform rather than just being able to name the platform though. >>> > > Perhaps that's a feature not a bug (i.e. they can choose which UART of >>> > > multiple UARTs if they want). I suppose we could also support >>> > > CONFIG_EARY_UART_PLATFORM=foo and translate into the above. >>> > > >>> > > Or are there other hardcoded parameters (e.g. crystal rate) which stop >>> > > this? >>> > > >> > >> > This drivers have been written with the Exynos 5 platform in mind, so >> > I'm not sure it can be called exynos4210. > Maybe I'm confused, I had assumed that exynos4210 was a UART logic block > which happened to have been dropped down onto the exynos5 SoC in much > the same way that the Cortex-A15 contains several pl011 UART logic > blocks. Is this not the case? Sorry, I've been confused, I did not take close look enough to how the Linux drivers was written, as the manual does not say anything about the history. So yes, I feel better in calling the driver exynos4210, with maybe a different name for the paddr which is exynos5250 specific I suppose. > In exynos5250.dtsi in Linux the UART is described as compatible = > "samsung,exynos4210-uart", is 4210 actually a previous revision of the > Exynos processor which happens to have a compatible UART on it? Yes, it seams to be the case, sorry for the confusion. > What I want to avoid is the case where we have dozens of UART drivers > which are all identical apart from the fact that they happen to be > integrated into a different SoC (all the other Exynos5xxx variants for > example), so I'd like to determine the most generic name for this driver > possible. >> > There would be few differences >> > that I can extract from Linux source code, the physical address is >> > different and the few bit to use FIFO are also different but it's not >> > use here in the early printk. And I'm almost sure the clock stuff would >> > be different on an exynos4. >> > >> > So, in my humble opinion, we should use exynos5 or exynos5250.
diff --git a/config/arm32.mk b/config/arm32.mk index 593a1d1..01c1490 100644 --- a/config/arm32.mk +++ b/config/arm32.mk @@ -17,6 +17,7 @@ CFLAGS += -marm # Possible value: # - none: no early printk # - pl011: printk with PL011 UART +# - exynos5: printk with the second exynos 5 UART CONFIG_EARLY_PRINTK := none HAS_PL011 := y HAS_EXYNOS5 := y diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk index 2053b1e..7dcc0b7 100644 --- a/xen/arch/arm/Rules.mk +++ b/xen/arch/arm/Rules.mk @@ -43,5 +43,9 @@ ifeq ($(CONFIG_EARLY_PRINTK), pl011) EARLY_PRINTK := y CONFIG_EARLY_PL011 := y endif +ifeq ($(CONFIG_EARLY_PRINTK), exynos5) +EARLY_PRINTK := y +CONFIG_EARLY_EXYNOS5 := y +endif CFLAGS-$(EARLY_PRINTK) += -DEARLY_PRINTK diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile index 6af8ca3..90e4eab 100644 --- a/xen/arch/arm/arm32/Makefile +++ b/xen/arch/arm/arm32/Makefile @@ -9,3 +9,4 @@ obj-y += domain.o obj-$(EARLY_PRINTK) += debug.o obj-$(CONFIG_EARLY_PL011) += debug-pl011.o +obj-$(CONFIG_EARLY_EXYNOS5) += debug-exynos5.o diff --git a/xen/arch/arm/arm32/debug-exynos5.S b/xen/arch/arm/arm32/debug-exynos5.S new file mode 100644 index 0000000..cbe1705 --- /dev/null +++ b/xen/arch/arm/arm32/debug-exynos5.S @@ -0,0 +1,81 @@ +/* + * xen/arch/arm/arm32/debug-exynos5.S + * + * Exynos 5 specific debug code + * + * Copyright (c) 2013 Citrix Systems. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <asm/asm_defns.h> + +#define EXYNOS5_UART_BASE_ADDRESS 0x12c20000 + +.globl early_uart_paddr +early_uart_paddr: .word EXYNOS5_UART_BASE_ADDRESS + +/* Exynos 5 UART initialization + * r11: UART base address + * Clobber r0-r1 */ +.globl early_uart_init +early_uart_init: + /* init clock */ + ldr r1, =0x10020000 + /* select MPLL (800MHz) source clock */ + ldr r0, [r1, #0x250] + and r0, r0, #(~(0xf<<8)) + orr r0, r0, #(0x6<<8) + str r0, [r1, #0x250] + /* ration 800/(7+1) */ + ldr r0, [r1, #0x558] + and r0, r0, #(~(0xf<<8)) + orr r0, r0, #(0x7<<8) + str r0, [r1, #0x558] + + mov r1, #4 + str r1, [r11, #0x2c] /* -> UARTIBRD (Baud divisor fraction) */ + mov r1, #53 + str r1, [r11, #0x28] /* -> UARTIBRD (Baud divisor integer) */ + mov r1, #3 /* 8n1 */ + str r1, [r11, #0x0] /* -> (Line control) */ + ldr r1, =(1<<2) /* TX IRQMODE */ + str r1, [r11, #0x4] /* -> (Control Register) */ + mov r1, #0x0 + str r1, [r11, #0x8] /* disable FIFO */ + mov r1, #0x0 + str r1, [r11, #0x0C] /* no auto flow control */ + mov pc, lr + +/* Exynos 5 UART wait UART to be ready to transmit + * r11: UART base address + * Clobber r2 r11 */ +.globl early_uart_ready +early_uart_ready: + ldr r2, [r11, #0x10] /* <- UTRSTAT (Flag register) */ + tst r2, #(1<<1) /* Check BUSY bit */ + beq early_uart_ready /* Wait for the UART to be ready */ + mov pc, lr + +/* Exynos 5 UART transmit character + * r2: character to transmit + * r11: UART base address */ +.globl early_uart_transmit +early_uart_transmit: + str r2, [r11, #0x20] /* -> UTXH (Data Register) */ + mov pc, lr + +/* + * Local variables: + * mode: ASM + * indent-tabs-mode: nil + * End: + */
Signed-off-by: Julien Grall <julien.grall@linaro.org> --- config/arm32.mk | 1 + xen/arch/arm/Rules.mk | 4 ++ xen/arch/arm/arm32/Makefile | 1 + xen/arch/arm/arm32/debug-exynos5.S | 81 ++++++++++++++++++++++++++++++++++++ 4 files changed, 87 insertions(+) create mode 100644 xen/arch/arm/arm32/debug-exynos5.S