* [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART
@ 2021-07-26 12:58 Pali Rohár
2021-07-26 12:58 ` [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init() Pali Rohár
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Pali Rohár @ 2021-07-26 12:58 UTC (permalink / raw)
To: Stefan Roese; +Cc: Konstantin Porotchkin, Marek Behún, u-boot
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);
/*
--
2.20.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
2021-07-26 12:58 [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART Pali Rohár
@ 2021-07-26 12:58 ` Pali Rohár
2021-07-26 14:55 ` Marek Behun
` (2 more replies)
2021-07-26 15:21 ` [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART Marek Behun
` (2 subsequent siblings)
3 siblings, 3 replies; 14+ messages in thread
From: Pali Rohár @ 2021-07-26 12:58 UTC (permalink / raw)
To: Stefan Roese; +Cc: Konstantin Porotchkin, Marek Behún, u-boot
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);
--
2.20.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
2021-07-26 12:58 ` [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init() Pali Rohár
@ 2021-07-26 14:55 ` Marek Behun
2021-07-26 14:58 ` Pali Rohár
2021-07-31 7:47 ` Stefan Roese
2021-07-31 10:01 ` Stefan Roese
2 siblings, 1 reply; 14+ messages in thread
From: Marek Behun @ 2021-07-26 14:55 UTC (permalink / raw)
To: Pali Rohár; +Cc: Stefan Roese, Konstantin Porotchkin, u-boot
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?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
2021-07-26 14:55 ` Marek Behun
@ 2021-07-26 14:58 ` Pali Rohár
2021-07-26 15:24 ` Marek Behun
0 siblings, 1 reply; 14+ messages in thread
From: Pali Rohár @ 2021-07-26 14:58 UTC (permalink / raw)
To: Marek Behun; +Cc: Stefan Roese, Konstantin Porotchkin, u-boot
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART
2021-07-26 12:58 [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART Pali Rohár
2021-07-26 12:58 ` [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init() Pali Rohár
@ 2021-07-26 15:21 ` Marek Behun
2021-07-31 7:45 ` Stefan Roese
2021-07-31 10:01 ` Stefan Roese
3 siblings, 0 replies; 14+ messages in thread
From: Marek Behun @ 2021-07-26 15:21 UTC (permalink / raw)
To: Pali Rohár; +Cc: Stefan Roese, Konstantin Porotchkin, u-boot
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
2021-07-26 14:58 ` Pali Rohár
@ 2021-07-26 15:24 ` Marek Behun
2021-08-11 18:57 ` Pali Rohár
0 siblings, 1 reply; 14+ messages in thread
From: Marek Behun @ 2021-07-26 15:24 UTC (permalink / raw)
To: Pali Rohár; +Cc: Stefan Roese, Konstantin Porotchkin, u-boot
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART
2021-07-26 12:58 [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART Pali Rohár
2021-07-26 12:58 ` [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init() Pali Rohár
2021-07-26 15:21 ` [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART Marek Behun
@ 2021-07-31 7:45 ` Stefan Roese
2021-07-31 10:01 ` Stefan Roese
3 siblings, 0 replies; 14+ messages in thread
From: Stefan Roese @ 2021-07-31 7:45 UTC (permalink / raw)
To: Pali Rohár; +Cc: Konstantin Porotchkin, Marek Behún, u-boot
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
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
2021-07-26 12:58 ` [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init() Pali Rohár
2021-07-26 14:55 ` Marek Behun
@ 2021-07-31 7:47 ` Stefan Roese
2021-07-31 10:01 ` Stefan Roese
2 siblings, 0 replies; 14+ messages in thread
From: Stefan Roese @ 2021-07-31 7:47 UTC (permalink / raw)
To: Pali Rohár; +Cc: Konstantin Porotchkin, Marek Behún, u-boot
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;
> divider = DIV_ROUND_CLOSEST(parent_rate, CONFIG_BAUDRATE * 16);
> writel(divider, base + UART_BAUD_REG);
>
>
Viele Grüße,
Stefan
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART
2021-07-26 12:58 [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART Pali Rohár
` (2 preceding siblings ...)
2021-07-31 7:45 ` Stefan Roese
@ 2021-07-31 10:01 ` Stefan Roese
3 siblings, 0 replies; 14+ messages in thread
From: Stefan Roese @ 2021-07-31 10:01 UTC (permalink / raw)
To: Pali Rohár; +Cc: Konstantin Porotchkin, Marek Behún, u-boot
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
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
2021-07-26 12:58 ` [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init() Pali Rohár
2021-07-26 14:55 ` Marek Behun
2021-07-31 7:47 ` Stefan Roese
@ 2021-07-31 10:01 ` Stefan Roese
2 siblings, 0 replies; 14+ messages in thread
From: Stefan Roese @ 2021-07-31 10:01 UTC (permalink / raw)
To: Pali Rohár; +Cc: Konstantin Porotchkin, Marek Behún, u-boot
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;
> divider = DIV_ROUND_CLOSEST(parent_rate, CONFIG_BAUDRATE * 16);
> writel(divider, base + UART_BAUD_REG);
>
>
Viele Grüße,
Stefan
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
2021-07-26 15:24 ` Marek Behun
@ 2021-08-11 18:57 ` Pali Rohár
2021-08-16 8:04 ` Stefan Roese
0 siblings, 1 reply; 14+ messages in thread
From: Pali Rohár @ 2021-08-11 18:57 UTC (permalink / raw)
To: Marek Behun; +Cc: Stefan Roese, Konstantin Porotchkin, u-boot
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@kernel.org/
> Thanks.
>
> Marek
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
2021-08-11 18:57 ` Pali Rohár
@ 2021-08-16 8:04 ` Stefan Roese
2021-08-16 8:33 ` Pali Rohár
0 siblings, 1 reply; 14+ messages in thread
From: Stefan Roese @ 2021-08-16 8:04 UTC (permalink / raw)
To: Pali Rohár, Marek Behun; +Cc: Konstantin Porotchkin, u-boot
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@kernel.org/
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
2021-08-16 8:04 ` Stefan Roese
@ 2021-08-16 8:33 ` Pali Rohár
2021-08-16 9:07 ` Stefan Roese
0 siblings, 1 reply; 14+ messages in thread
From: Pali Rohár @ 2021-08-16 8:33 UTC (permalink / raw)
To: Stefan Roese; +Cc: Marek Behun, Konstantin Porotchkin, u-boot
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@kernel.org/
>
> 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init()
2021-08-16 8:33 ` Pali Rohár
@ 2021-08-16 9:07 ` Stefan Roese
0 siblings, 0 replies; 14+ messages in thread
From: Stefan Roese @ 2021-08-16 9:07 UTC (permalink / raw)
To: Pali Rohár; +Cc: Marek Behun, Konstantin Porotchkin, u-boot
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@kernel.org/
>>
>> 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)
>
IMHO, this version is a bit better, as the names (hopefully) reflect
the register description in the datasheet.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-08-16 9:08 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-26 12:58 [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART Pali Rohár
2021-07-26 12:58 ` [PATCH 2/2] serial: a37xx: Do not call get_ref_clk() in _debug_uart_init() Pali Rohár
2021-07-26 14:55 ` Marek Behun
2021-07-26 14:58 ` Pali Rohár
2021-07-26 15:24 ` Marek Behun
2021-08-11 18:57 ` Pali Rohár
2021-08-16 8:04 ` Stefan Roese
2021-08-16 8:33 ` Pali Rohár
2021-08-16 9:07 ` Stefan Roese
2021-07-31 7:47 ` Stefan Roese
2021-07-31 10:01 ` Stefan Roese
2021-07-26 15:21 ` [PATCH 1/2] serial: a37xx: Use CONFIG_BAUDRATE for initializing early debug UART Marek Behun
2021-07-31 7:45 ` Stefan Roese
2021-07-31 10:01 ` Stefan Roese
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.