[PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART
CONFIG_BAUDRATE should be used for setting the baudrate for the early debug UART. This replaces current hardcoded 115200 value.
Signed-off-by: Pali Rohár pali@kernel.org --- drivers/serial/serial_mvebu_a3700.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/serial/serial_mvebu_a3700.c b/drivers/serial/serial_mvebu_a3700.c index c7e66fef8768..52dc3fdad7b4 100644 --- a/drivers/serial/serial_mvebu_a3700.c +++ b/drivers/serial/serial_mvebu_a3700.c @@ -309,7 +309,7 @@ U_BOOT_DRIVER(serial_mvebu) = { static inline void _debug_uart_init(void) { void __iomem *base = (void __iomem *)CONFIG_DEBUG_UART_BASE; - u32 baudrate, parent_rate, divider; + u32 parent_rate, divider;
/* reset FIFOs */ writel(UART_CTRL_RXFIFO_RESET | UART_CTRL_TXFIFO_RESET, @@ -322,9 +322,8 @@ static inline void _debug_uart_init(void) * Calculate divider * baudrate = clock / 16 / divider */ - baudrate = 115200; parent_rate = get_ref_clk() * 1000000; - divider = DIV_ROUND_CLOSEST(parent_rate, baudrate * 16); + divider = DIV_ROUND_CLOSEST(parent_rate, CONFIG_BAUDRATE * 16); writel(divider, base + UART_BAUD_REG);
/*
Static inline function _debug_uart_init() should avoid calling external (non-inline) functions. Therefore do not call get_ref_clk() in _debug_uart_init() and reimplement its functionality without external function calls.
Signed-off-by: Pali Rohár pali@kernel.org --- drivers/serial/serial_mvebu_a3700.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/serial/serial_mvebu_a3700.c b/drivers/serial/serial_mvebu_a3700.c index 52dc3fdad7b4..6bca8e4b7e2d 100644 --- a/drivers/serial/serial_mvebu_a3700.c +++ b/drivers/serial/serial_mvebu_a3700.c @@ -305,6 +305,7 @@ U_BOOT_DRIVER(serial_mvebu) = { #ifdef CONFIG_DEBUG_MVEBU_A3700_UART
#include <debug_uart.h> +#include <mach/soc.h>
static inline void _debug_uart_init(void) { @@ -322,7 +323,8 @@ static inline void _debug_uart_init(void) * Calculate divider * baudrate = clock / 16 / divider */ - parent_rate = get_ref_clk() * 1000000; + parent_rate = (readl(MVEBU_REGISTER(0x13808)) & BIT(9)) ? + 40000000 : 25000000; divider = DIV_ROUND_CLOSEST(parent_rate, CONFIG_BAUDRATE * 16); writel(divider, base + UART_BAUD_REG);
On Mon, 26 Jul 2021 14:58:59 +0200 Pali Rohár pali@kernel.org wrote:
Static inline function _debug_uart_init() should avoid calling external (non-inline) functions.
Why?
On Monday 26 July 2021 16:55:22 Marek Behun wrote:
On Mon, 26 Jul 2021 14:58:59 +0200 Pali Rohár pali@kernel.org wrote:
Static inline function _debug_uart_init() should avoid calling external (non-inline) functions.
Why?
Function is called in stage when stack is not fully initialized and documentation suggest to avoid stack usage and other functions.
On Mon, 26 Jul 2021 16:58:04 +0200 Pali Rohár pali@kernel.org wrote:
On Monday 26 July 2021 16:55:22 Marek Behun wrote:
On Mon, 26 Jul 2021 14:58:59 +0200 Pali Rohár pali@kernel.org wrote:
Static inline function _debug_uart_init() should avoid calling external (non-inline) functions.
Why?
Function is called in stage when stack is not fully initialized and documentation suggest to avoid stack usage and other functions.
OK, but maybe we should use the macro names for register constants.
Could you move (in a separate patch) the corresponding macros from arch/arm/mach-mvebu/armada3700/cpu.c to arch/arm/mach-mvebu/include/mach/soc.h and then in this patch include <asm/arch/soc.h> in the serial driver and use the constant names?
Thanks.
Marek
On Monday 26 July 2021 17:24:26 Marek Behun wrote:
On Mon, 26 Jul 2021 16:58:04 +0200 Pali Rohár pali@kernel.org wrote:
On Monday 26 July 2021 16:55:22 Marek Behun wrote:
On Mon, 26 Jul 2021 14:58:59 +0200 Pali Rohár pali@kernel.org wrote:
Static inline function _debug_uart_init() should avoid calling external (non-inline) functions.
Why?
Function is called in stage when stack is not fully initialized and documentation suggest to avoid stack usage and other functions.
OK, but maybe we should use the macro names for register constants.
Could you move (in a separate patch) the corresponding macros from arch/arm/mach-mvebu/armada3700/cpu.c to arch/arm/mach-mvebu/include/mach/soc.h and then in this patch include <asm/arch/soc.h> in the serial driver and use the constant names?
Implemented in separate patch as Stefan wanted: http://patchwork.ozlabs.org/project/uboot/patch/20210811185330.15414-2-pali@...
Thanks.
Marek
Hi Pali,
On 11.08.21 20:57, Pali Rohár wrote:
On Monday 26 July 2021 17:24:26 Marek Behun wrote:
On Mon, 26 Jul 2021 16:58:04 +0200 Pali Rohár pali@kernel.org wrote:
On Monday 26 July 2021 16:55:22 Marek Behun wrote:
On Mon, 26 Jul 2021 14:58:59 +0200 Pali Rohár pali@kernel.org wrote:
Static inline function _debug_uart_init() should avoid calling external (non-inline) functions.
Why?
Function is called in stage when stack is not fully initialized and documentation suggest to avoid stack usage and other functions.
OK, but maybe we should use the macro names for register constants.
Could you move (in a separate patch) the corresponding macros from arch/arm/mach-mvebu/armada3700/cpu.c to arch/arm/mach-mvebu/include/mach/soc.h and then in this patch include <asm/arch/soc.h> in the serial driver and use the constant names?
Implemented in separate patch as Stefan wanted: http://patchwork.ozlabs.org/project/uboot/patch/20210811185330.15414-2-pali@...
Did I really want this? You're removing the macro names with this patch, and I would prefer using the defines/names instead of register numbers like here:
+#elif defined(CONFIG_ARMADA_3700) +/* SAR values for Armada 3700 */ +#define CONFIG_SYS_REF_CLK ((readl(MVEBU_REGISTER(0x13808)) & BIT(9)) ? \ + 40000000 : 25000000) #endif
Or did I miss something?
Thanks, Stefan
On Monday 16 August 2021 10:04:39 Stefan Roese wrote:
Hi Pali,
On 11.08.21 20:57, Pali Rohár wrote:
On Monday 26 July 2021 17:24:26 Marek Behun wrote:
On Mon, 26 Jul 2021 16:58:04 +0200 Pali Rohár pali@kernel.org wrote:
On Monday 26 July 2021 16:55:22 Marek Behun wrote:
On Mon, 26 Jul 2021 14:58:59 +0200 Pali Rohár pali@kernel.org wrote:
Static inline function _debug_uart_init() should avoid calling external (non-inline) functions.
Why?
Function is called in stage when stack is not fully initialized and documentation suggest to avoid stack usage and other functions.
OK, but maybe we should use the macro names for register constants.
Could you move (in a separate patch) the corresponding macros from arch/arm/mach-mvebu/armada3700/cpu.c to arch/arm/mach-mvebu/include/mach/soc.h and then in this patch include <asm/arch/soc.h> in the serial driver and use the constant names?
Implemented in separate patch as Stefan wanted: http://patchwork.ozlabs.org/project/uboot/patch/20210811185330.15414-2-pali@...
Did I really want this? You're removing the macro names with this patch, and I would prefer using the defines/names instead of register numbers like here:
+#elif defined(CONFIG_ARMADA_3700) +/* SAR values for Armada 3700 */ +#define CONFIG_SYS_REF_CLK ((readl(MVEBU_REGISTER(0x13808)) & BIT(9)) ? \
40000000 : 25000000)#endif
Or did I miss something?
Thanks, Stefan
It is global include header file, so I though it would be better to not export lot of other macros which are relevant only for CONFIG_SYS_REF_CLK
But if you want to see names, I can change it e.g. to:
+#elif defined(CONFIG_ARMADA_3700) +/* SAR values for Armada 3700 */ +#define MVEBU_TEST_PIN_LATCH_N MVEBU_REGISTER(0x13808) +#define MVEBU_XTAL_MODE_MASK BIT(9) +#define CONFIG_SYS_REF_CLK ((readl(MVEBU_TEST_PIN_LATCH_N) & MVEBU_XTAL_MODE_MASK) ? \ + 40000000 : 25000000)
On 16.08.21 10:33, Pali Rohár wrote:
On Monday 16 August 2021 10:04:39 Stefan Roese wrote:
Hi Pali,
On 11.08.21 20:57, Pali Rohár wrote:
On Monday 26 July 2021 17:24:26 Marek Behun wrote:
On Mon, 26 Jul 2021 16:58:04 +0200 Pali Rohár pali@kernel.org wrote:
On Monday 26 July 2021 16:55:22 Marek Behun wrote:
On Mon, 26 Jul 2021 14:58:59 +0200 Pali Rohár pali@kernel.org wrote: > Static inline function _debug_uart_init() should avoid calling external > (non-inline) functions.
Why?
Function is called in stage when stack is not fully initialized and documentation suggest to avoid stack usage and other functions.
OK, but maybe we should use the macro names for register constants.
Could you move (in a separate patch) the corresponding macros from arch/arm/mach-mvebu/armada3700/cpu.c to arch/arm/mach-mvebu/include/mach/soc.h and then in this patch include <asm/arch/soc.h> in the serial driver and use the constant names?
Implemented in separate patch as Stefan wanted: http://patchwork.ozlabs.org/project/uboot/patch/20210811185330.15414-2-pali@...
Did I really want this? You're removing the macro names with this patch, and I would prefer using the defines/names instead of register numbers like here:
+#elif defined(CONFIG_ARMADA_3700) +/* SAR values for Armada 3700 */ +#define CONFIG_SYS_REF_CLK ((readl(MVEBU_REGISTER(0x13808)) & BIT(9)) ? \
40000000 : 25000000)Or did I miss something?
Thanks, Stefan
It is global include header file, so I though it would be better to not export lot of other macros which are relevant only for CONFIG_SYS_REF_CLK
But if you want to see names, I can change it e.g. to:
+#elif defined(CONFIG_ARMADA_3700) +/* SAR values for Armada 3700 */ +#define MVEBU_TEST_PIN_LATCH_N MVEBU_REGISTER(0x13808) +#define MVEBU_XTAL_MODE_MASK BIT(9) +#define CONFIG_SYS_REF_CLK ((readl(MVEBU_TEST_PIN_LATCH_N) & MVEBU_XTAL_MODE_MASK) ? \
40000000 : 25000000)
IMHO, this version is a bit better, as the names (hopefully) reflect the register description in the datasheet.
Thanks, Stefan
On 26.07.21 14:58, Pali Rohár wrote:
Static inline function _debug_uart_init() should avoid calling external (non-inline) functions. Therefore do not call get_ref_clk() in _debug_uart_init() and reimplement its functionality without external function calls.
Signed-off-by: Pali Rohár pali@kernel.org
Apart from the comments from Marek (please in a separate patch):
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
drivers/serial/serial_mvebu_a3700.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/serial/serial_mvebu_a3700.c b/drivers/serial/serial_mvebu_a3700.c index 52dc3fdad7b4..6bca8e4b7e2d 100644 --- a/drivers/serial/serial_mvebu_a3700.c +++ b/drivers/serial/serial_mvebu_a3700.c @@ -305,6 +305,7 @@ U_BOOT_DRIVER(serial_mvebu) = { #ifdef CONFIG_DEBUG_MVEBU_A3700_UART
#include <debug_uart.h> +#include <mach/soc.h>
static inline void _debug_uart_init(void) { @@ -322,7 +323,8 @@ static inline void _debug_uart_init(void) * Calculate divider * baudrate = clock / 16 / divider */
- parent_rate = get_ref_clk() * 1000000;
- parent_rate = (readl(MVEBU_REGISTER(0x13808)) & BIT(9)) ?
40000000 : 25000000;
Viele Grüße, Stefan
On 26.07.21 14:58, Pali Rohár wrote:
Static inline function _debug_uart_init() should avoid calling external (non-inline) functions. Therefore do not call get_ref_clk() in _debug_uart_init() and reimplement its functionality without external function calls.
Signed-off-by: Pali Rohár pali@kernel.org
Applied to u-boot-marvell/master
Thanks, Stefan
drivers/serial/serial_mvebu_a3700.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/serial/serial_mvebu_a3700.c b/drivers/serial/serial_mvebu_a3700.c index 52dc3fdad7b4..6bca8e4b7e2d 100644 --- a/drivers/serial/serial_mvebu_a3700.c +++ b/drivers/serial/serial_mvebu_a3700.c @@ -305,6 +305,7 @@ U_BOOT_DRIVER(serial_mvebu) = { #ifdef CONFIG_DEBUG_MVEBU_A3700_UART
#include <debug_uart.h> +#include <mach/soc.h>
static inline void _debug_uart_init(void) { @@ -322,7 +323,8 @@ static inline void _debug_uart_init(void) * Calculate divider * baudrate = clock / 16 / divider */
- parent_rate = get_ref_clk() * 1000000;
- parent_rate = (readl(MVEBU_REGISTER(0x13808)) & BIT(9)) ?
40000000 : 25000000;
Viele Grüße, Stefan
On Mon, 26 Jul 2021 14:58:58 +0200 Pali Rohár pali@kernel.org wrote:
CONFIG_BAUDRATE should be used for setting the baudrate for the early debug UART. This replaces current hardcoded 115200 value.
Signed-off-by: Pali Rohár pali@kernel.org
Reviewed-by: Marek Behun marek.behun@nic.cz
On 26.07.21 14:58, Pali Rohár wrote:
CONFIG_BAUDRATE should be used for setting the baudrate for the early debug UART. This replaces current hardcoded 115200 value.
Signed-off-by: Pali Rohár pali@kernel.org
Reviewed-by: Stefan Roese sr@denx.de
Thanks, Stefan
drivers/serial/serial_mvebu_a3700.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/serial/serial_mvebu_a3700.c b/drivers/serial/serial_mvebu_a3700.c index c7e66fef8768..52dc3fdad7b4 100644 --- a/drivers/serial/serial_mvebu_a3700.c +++ b/drivers/serial/serial_mvebu_a3700.c @@ -309,7 +309,7 @@ U_BOOT_DRIVER(serial_mvebu) = { static inline void _debug_uart_init(void) { void __iomem *base = (void __iomem *)CONFIG_DEBUG_UART_BASE;
- u32 baudrate, parent_rate, divider;
u32 parent_rate, divider;
/* reset FIFOs */ writel(UART_CTRL_RXFIFO_RESET | UART_CTRL_TXFIFO_RESET,
@@ -322,9 +322,8 @@ static inline void _debug_uart_init(void) * Calculate divider * baudrate = clock / 16 / divider */
- baudrate = 115200; parent_rate = get_ref_clk() * 1000000;
- divider = DIV_ROUND_CLOSEST(parent_rate, baudrate * 16);
divider = DIV_ROUND_CLOSEST(parent_rate, CONFIG_BAUDRATE * 16); writel(divider, base + UART_BAUD_REG);
/*
Viele Grüße, Stefan
On 26.07.21 14:58, Pali Rohár wrote:
CONFIG_BAUDRATE should be used for setting the baudrate for the early debug UART. This replaces current hardcoded 115200 value.
Signed-off-by: Pali Rohár pali@kernel.org
Applied to u-boot-marvell/master
Thanks, Stefan
drivers/serial/serial_mvebu_a3700.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/serial/serial_mvebu_a3700.c b/drivers/serial/serial_mvebu_a3700.c index c7e66fef8768..52dc3fdad7b4 100644 --- a/drivers/serial/serial_mvebu_a3700.c +++ b/drivers/serial/serial_mvebu_a3700.c @@ -309,7 +309,7 @@ U_BOOT_DRIVER(serial_mvebu) = { static inline void _debug_uart_init(void) { void __iomem *base = (void __iomem *)CONFIG_DEBUG_UART_BASE;
- u32 baudrate, parent_rate, divider;
u32 parent_rate, divider;
/* reset FIFOs */ writel(UART_CTRL_RXFIFO_RESET | UART_CTRL_TXFIFO_RESET,
@@ -322,9 +322,8 @@ static inline void _debug_uart_init(void) * Calculate divider * baudrate = clock / 16 / divider */
- baudrate = 115200; parent_rate = get_ref_clk() * 1000000;
- divider = DIV_ROUND_CLOSEST(parent_rate, baudrate * 16);
divider = DIV_ROUND_CLOSEST(parent_rate, CONFIG_BAUDRATE * 16); writel(divider, base + UART_BAUD_REG);
/*
Viele Grüße, Stefan
participants (3)
-
Marek Behun -
Pali Rohár -
Stefan Roese