[PATCH 1/2] console: provide best-effort clk_get_for_console helper

[PATCH 1/2] console: provide best-effort clk_get_for_console helper

[Ahmad Fatoum]

From: Ahmad Fatoum <ahmad@a3f.at>

clk_get will return -EPROBE_DEFER if clock provider hasn't yet been
probed. In a system with deep probe enabled, dependencies are probed
on demand, so a -EPROBE_DEFER return is final and the console probe
will never succeed.

CONFIG_DEBUG_LL is often used to debug this, but because the low-level
console is not interactive, it's a bit cumbersome. Improve upon this a
bit, by providing a clk_get_for_console helper that returns NULL if and
only if we are sure that a clock provider will not be probed.

In that case, the driver can skip code paths initialization code and
baud rate setting dependent on having access to the clock and still
register a console that reuses what was set up by CONFIG_DEBUG_LL.

Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
 include/console.h   | 26 ++++++++++++++++++++++++++
 include/linux/clk.h | 21 +++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/include/console.h b/include/console.h
index 586b68f73301..b8c901801e9f 100644
--- a/include/console.h
+++ b/include/console.h
@@ -8,6 +8,7 @@
 #define _CONSOLE_H_
 
 #include <param.h>
+#include <linux/clk.h>
 #include <linux/list.h>
 #include <driver.h>
 #include <serdev.h>
@@ -208,4 +209,29 @@ static inline void console_ctrlc_allow(void) { }
 static inline void console_ctrlc_forbid(void) { }
 #endif
 
+/**
+ * clk_get_for_console - get clock, ignoring known unavailable clock controller
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Return: a struct clk corresponding to the clock producer, a
+ * valid IS_ERR() condition containing errno or NULL if it could
+ * be determined that the clock producer will never be probed in
+ * absence of modules. The NULL return allows serial drivers to
+ * skip clock handling and rely on CONFIG_DEBUG_LL.
+ */
+static inline struct clk *clk_get_for_console(struct device *dev, const char *id)
+{
+	struct clk *clk;
+
+	if (!IS_ENABLED(CONFIG_DEBUG_LL))
+		return clk_get(dev, id);
+
+	clk = clk_get_if_available(dev, id);
+	if (clk == NULL)
+		dev_warn(dev, "couldn't get clock (ignoring)\n");
+
+	return clk;
+}
+
 #endif
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 61b4ff3127cf..f505f18a75c0 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -14,6 +14,7 @@
 #include <linux/spinlock.h>
 #include <linux/stringify.h>
 #include <linux/container_of.h>
+#include <deep-probe.h>
 #include <xfuncs.h>
 
 struct device;
@@ -982,4 +983,24 @@ static inline struct clk *clk_get_enabled(struct device *dev, const char *id)
 	return clk;
 }
 
+/**
+ * clk_get_if_available - get clock, ignoring known unavailable clock controller
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Return: a struct clk corresponding to the clock producer, a
+ * valid IS_ERR() condition containing errno or NULL if it could
+ * be determined that the clock producer will never be probed in
+ * absence of modules.
+ */
+static inline struct clk *clk_get_if_available(struct device *dev, const char *id)
+{
+	struct clk *clk = clk_get(dev, id);
+
+	if (clk == ERR_PTR(-EPROBE_DEFER) && deep_probe_is_supported())
+		return NULL;
+
+	return clk;
+}
+
 #endif
-- 
2.39.2

[PATCH 2/2] serial: stm32: support probing with missing clock provider

[Ahmad Fatoum]

The STM32MP135F-DK support in barebox has bitrotted, because of changes
to the upstream device tree: The root of the clock tree is now provided
by OP-TEE over SCMI for which no support exists in barebox, so drivers
requesting clocks will defer indefinitely, including the console device.

A series adding OP-TEE SCMI support will follow, but for now, let's
ensure that console registration isn't deferred in such a case.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/serial/serial_stm32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/serial/serial_stm32.c b/drivers/serial/serial_stm32.c
index 4a67408f44f6..3e18a2c152af 100644
--- a/drivers/serial/serial_stm32.c
+++ b/drivers/serial/serial_stm32.c
@@ -165,7 +165,7 @@ static int stm32_serial_probe(struct device *dev)
 	stm32->stm32f4 = info->stm32f4;
 	stm32->uart_enable_bit = info->uart_enable_bit;
 
-	stm32->clk = clk_get(dev, NULL);
+	stm32->clk = clk_get_for_console(dev, NULL);
 	if (IS_ERR(stm32->clk)) {
 		ret = PTR_ERR(stm32->clk);
 		dev_err(dev, "Failed to get UART clock %d\n", ret);
@@ -183,7 +183,7 @@ static int stm32_serial_probe(struct device *dev)
 	cdev->putc   = stm32_serial_putc;
 	cdev->getc   = stm32_serial_getc;
 	cdev->flush  = stm32_serial_flush;
-	cdev->setbrg = stm32_serial_setbaudrate;
+	cdev->setbrg = stm32->clk ? stm32_serial_setbaudrate : NULL;
 	cdev->linux_console_name = "ttySTM";
 
 	if (dev->of_node) {
-- 
2.39.2

Re: [PATCH 1/2] console: provide best-effort clk_get_for_console helper

[Sascha Hauer]

On Thu, Nov 09, 2023 at 12:47:06PM +0100, Ahmad Fatoum wrote:
> From: Ahmad Fatoum <ahmad@a3f.at>
> 
> clk_get will return -EPROBE_DEFER if clock provider hasn't yet been
> probed. In a system with deep probe enabled, dependencies are probed
> on demand, so a -EPROBE_DEFER return is final and the console probe
> will never succeed.
> 
> CONFIG_DEBUG_LL is often used to debug this, but because the low-level
> console is not interactive, it's a bit cumbersome. Improve upon this a
> bit, by providing a clk_get_for_console helper that returns NULL if and
> only if we are sure that a clock provider will not be probed.
> 
> In that case, the driver can skip code paths initialization code and
> baud rate setting dependent on having access to the clock and still
> register a console that reuses what was set up by CONFIG_DEBUG_LL.
> 
> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> ---
>  include/console.h   | 26 ++++++++++++++++++++++++++
>  include/linux/clk.h | 21 +++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/include/console.h b/include/console.h
> index 586b68f73301..b8c901801e9f 100644
> --- a/include/console.h
> +++ b/include/console.h
> @@ -8,6 +8,7 @@
>  #define _CONSOLE_H_
>  
>  #include <param.h>
> +#include <linux/clk.h>
>  #include <linux/list.h>
>  #include <driver.h>
>  #include <serdev.h>
> @@ -208,4 +209,29 @@ static inline void console_ctrlc_allow(void) { }
>  static inline void console_ctrlc_forbid(void) { }
>  #endif
>  
> +/**
> + * clk_get_for_console - get clock, ignoring known unavailable clock controller
> + * @dev: device for clock "consumer"
> + * @id: clock consumer ID
> + *
> + * Return: a struct clk corresponding to the clock producer, a
> + * valid IS_ERR() condition containing errno or NULL if it could
> + * be determined that the clock producer will never be probed in
> + * absence of modules. The NULL return allows serial drivers to
> + * skip clock handling and rely on CONFIG_DEBUG_LL.
> + */
> +static inline struct clk *clk_get_for_console(struct device *dev, const char *id)
> +{
> +	struct clk *clk;
> +
> +	if (!IS_ENABLED(CONFIG_DEBUG_LL))
> +		return clk_get(dev, id);

There are several SoCs out there where the UART is enabled by the ROM
already, on these we don't need to have CONFIG_DEBUG_LL enabled for a
working UART.

Maybe testing for the UART enable bit in probe() would be a better
indication that the UART is already in a working state?

Sascha

> +
> +	clk = clk_get_if_available(dev, id);
> +	if (clk == NULL)
> +		dev_warn(dev, "couldn't get clock (ignoring)\n");
> +
> +	return clk;
> +}
> +
>  #endif
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 61b4ff3127cf..f505f18a75c0 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -14,6 +14,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/stringify.h>
>  #include <linux/container_of.h>
> +#include <deep-probe.h>
>  #include <xfuncs.h>
>  
>  struct device;
> @@ -982,4 +983,24 @@ static inline struct clk *clk_get_enabled(struct device *dev, const char *id)
>  	return clk;
>  }
>  
> +/**
> + * clk_get_if_available - get clock, ignoring known unavailable clock controller
> + * @dev: device for clock "consumer"
> + * @id: clock consumer ID
> + *
> + * Return: a struct clk corresponding to the clock producer, a
> + * valid IS_ERR() condition containing errno or NULL if it could
> + * be determined that the clock producer will never be probed in
> + * absence of modules.
> + */
> +static inline struct clk *clk_get_if_available(struct device *dev, const char *id)
> +{
> +	struct clk *clk = clk_get(dev, id);
> +
> +	if (clk == ERR_PTR(-EPROBE_DEFER) && deep_probe_is_supported())
> +		return NULL;
> +
> +	return clk;
> +}
> +
>  #endif
> -- 
> 2.39.2
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] console: provide best-effort clk_get_for_console helper

[Ahmad Fatoum]

On 13.11.23 14:03, Sascha Hauer wrote:
> On Thu, Nov 09, 2023 at 12:47:06PM +0100, Ahmad Fatoum wrote:
>> From: Ahmad Fatoum <ahmad@a3f.at>
>>
>> clk_get will return -EPROBE_DEFER if clock provider hasn't yet been
>> probed. In a system with deep probe enabled, dependencies are probed
>> on demand, so a -EPROBE_DEFER return is final and the console probe
>> will never succeed.
>>
>> CONFIG_DEBUG_LL is often used to debug this, but because the low-level
>> console is not interactive, it's a bit cumbersome. Improve upon this a
>> bit, by providing a clk_get_for_console helper that returns NULL if and
>> only if we are sure that a clock provider will not be probed.
>>
>> In that case, the driver can skip code paths initialization code and
>> baud rate setting dependent on having access to the clock and still
>> register a console that reuses what was set up by CONFIG_DEBUG_LL.
>>
>> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
>> ---
>>  include/console.h   | 26 ++++++++++++++++++++++++++
>>  include/linux/clk.h | 21 +++++++++++++++++++++
>>  2 files changed, 47 insertions(+)
>>
>> diff --git a/include/console.h b/include/console.h
>> index 586b68f73301..b8c901801e9f 100644
>> --- a/include/console.h
>> +++ b/include/console.h
>> @@ -8,6 +8,7 @@
>>  #define _CONSOLE_H_
>>  
>>  #include <param.h>
>> +#include <linux/clk.h>
>>  #include <linux/list.h>
>>  #include <driver.h>
>>  #include <serdev.h>
>> @@ -208,4 +209,29 @@ static inline void console_ctrlc_allow(void) { }
>>  static inline void console_ctrlc_forbid(void) { }
>>  #endif
>>  
>> +/**
>> + * clk_get_for_console - get clock, ignoring known unavailable clock controller
>> + * @dev: device for clock "consumer"
>> + * @id: clock consumer ID
>> + *
>> + * Return: a struct clk corresponding to the clock producer, a
>> + * valid IS_ERR() condition containing errno or NULL if it could
>> + * be determined that the clock producer will never be probed in
>> + * absence of modules. The NULL return allows serial drivers to
>> + * skip clock handling and rely on CONFIG_DEBUG_LL.
>> + */
>> +static inline struct clk *clk_get_for_console(struct device *dev, const char *id)
>> +{
>> +	struct clk *clk;
>> +
>> +	if (!IS_ENABLED(CONFIG_DEBUG_LL))
>> +		return clk_get(dev, id);
> 
> There are several SoCs out there where the UART is enabled by the ROM
> already, on these we don't need to have CONFIG_DEBUG_LL enabled for a
> working UART.

Those SoCs can still implement CONFIG_DEBUG_LL and just skip the
initialization step.

> Maybe testing for the UART enable bit in probe() would be a better
> indication that the UART is already in a working state?

If clk turns out to be not enabled, system would hang on e.g. i.MX.

This is a mere debugging measure for SoCs with complex clock controllers
backed by firmware, so I think having to enable CONFIG_DEBUG_LL to use
is acceptable.

Cheers,
Ahmad


> 
> Sascha
> 
>> +
>> +	clk = clk_get_if_available(dev, id);
>> +	if (clk == NULL)
>> +		dev_warn(dev, "couldn't get clock (ignoring)\n");
>> +
>> +	return clk;
>> +}
>> +
>>  #endif
>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>> index 61b4ff3127cf..f505f18a75c0 100644
>> --- a/include/linux/clk.h
>> +++ b/include/linux/clk.h
>> @@ -14,6 +14,7 @@
>>  #include <linux/spinlock.h>
>>  #include <linux/stringify.h>
>>  #include <linux/container_of.h>
>> +#include <deep-probe.h>
>>  #include <xfuncs.h>
>>  
>>  struct device;
>> @@ -982,4 +983,24 @@ static inline struct clk *clk_get_enabled(struct device *dev, const char *id)
>>  	return clk;
>>  }
>>  
>> +/**
>> + * clk_get_if_available - get clock, ignoring known unavailable clock controller
>> + * @dev: device for clock "consumer"
>> + * @id: clock consumer ID
>> + *
>> + * Return: a struct clk corresponding to the clock producer, a
>> + * valid IS_ERR() condition containing errno or NULL if it could
>> + * be determined that the clock producer will never be probed in
>> + * absence of modules.
>> + */
>> +static inline struct clk *clk_get_if_available(struct device *dev, const char *id)
>> +{
>> +	struct clk *clk = clk_get(dev, id);
>> +
>> +	if (clk == ERR_PTR(-EPROBE_DEFER) && deep_probe_is_supported())
>> +		return NULL;
>> +
>> +	return clk;
>> +}
>> +
>>  #endif
>> -- 
>> 2.39.2
>>
>>
>>
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] console: provide best-effort clk_get_for_console helper

[Sascha Hauer]

On Mon, Nov 13, 2023 at 04:02:56PM +0100, Ahmad Fatoum wrote:
> On 13.11.23 14:03, Sascha Hauer wrote:
> > On Thu, Nov 09, 2023 at 12:47:06PM +0100, Ahmad Fatoum wrote:
> >> From: Ahmad Fatoum <ahmad@a3f.at>
> >>
> >> clk_get will return -EPROBE_DEFER if clock provider hasn't yet been
> >> probed. In a system with deep probe enabled, dependencies are probed
> >> on demand, so a -EPROBE_DEFER return is final and the console probe
> >> will never succeed.
> >>
> >> CONFIG_DEBUG_LL is often used to debug this, but because the low-level
> >> console is not interactive, it's a bit cumbersome. Improve upon this a
> >> bit, by providing a clk_get_for_console helper that returns NULL if and
> >> only if we are sure that a clock provider will not be probed.
> >>
> >> In that case, the driver can skip code paths initialization code and
> >> baud rate setting dependent on having access to the clock and still
> >> register a console that reuses what was set up by CONFIG_DEBUG_LL.
> >>
> >> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> >> ---
> >>  include/console.h   | 26 ++++++++++++++++++++++++++
> >>  include/linux/clk.h | 21 +++++++++++++++++++++
> >>  2 files changed, 47 insertions(+)
> >>
> >> diff --git a/include/console.h b/include/console.h
> >> index 586b68f73301..b8c901801e9f 100644
> >> --- a/include/console.h
> >> +++ b/include/console.h
> >> @@ -8,6 +8,7 @@
> >>  #define _CONSOLE_H_
> >>  
> >>  #include <param.h>
> >> +#include <linux/clk.h>
> >>  #include <linux/list.h>
> >>  #include <driver.h>
> >>  #include <serdev.h>
> >> @@ -208,4 +209,29 @@ static inline void console_ctrlc_allow(void) { }
> >>  static inline void console_ctrlc_forbid(void) { }
> >>  #endif
> >>  
> >> +/**
> >> + * clk_get_for_console - get clock, ignoring known unavailable clock controller
> >> + * @dev: device for clock "consumer"
> >> + * @id: clock consumer ID
> >> + *
> >> + * Return: a struct clk corresponding to the clock producer, a
> >> + * valid IS_ERR() condition containing errno or NULL if it could
> >> + * be determined that the clock producer will never be probed in
> >> + * absence of modules. The NULL return allows serial drivers to
> >> + * skip clock handling and rely on CONFIG_DEBUG_LL.
> >> + */
> >> +static inline struct clk *clk_get_for_console(struct device *dev, const char *id)
> >> +{
> >> +	struct clk *clk;
> >> +
> >> +	if (!IS_ENABLED(CONFIG_DEBUG_LL))
> >> +		return clk_get(dev, id);
> > 
> > There are several SoCs out there where the UART is enabled by the ROM
> > already, on these we don't need to have CONFIG_DEBUG_LL enabled for a
> > working UART.
> 
> Those SoCs can still implement CONFIG_DEBUG_LL and just skip the
> initialization step.
> 
> > Maybe testing for the UART enable bit in probe() would be a better
> > indication that the UART is already in a working state?
> 
> If clk turns out to be not enabled, system would hang on e.g. i.MX.

That can happen with your patch as well as you don't limit the
usage of clk_get_for_console() to the UART putc_ll is configured
for. Initializing one of the other UARTs might hang your system
once you access a register.

> 
> This is a mere debugging measure for SoCs with complex clock controllers
> backed by firmware, so I think having to enable CONFIG_DEBUG_LL to use
> is acceptable.

Another option would be to implement and use a GETC_LL() macro. This
requires some thinking how this can be integrated into the console
code, but would in the end be more universally usable. With this we
could make barebox interactive even when the real serial driver is
not compiled in (or not even yet existing). See below for a prototype.

Sascha

----------------------------8<-----------------------------

>From a737458a64718f96c8d5267551895b0fcea23089 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Tue, 14 Nov 2023 09:22:24 +0100
Subject: [PATCH] debug_ll: implement lowlevel debug input functions

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/boards/tqmba9xxxca/lowlevel.c |  1 +
 common/console.c                       |  6 ++++++
 common/startup.c                       |  3 ++-
 include/debug_ll.h                     | 16 +++++++++++++++
 include/mach/imx/debug_ll.h            | 28 ++++++++++++++++++++++++++
 include/serial/lpuart32.h              | 12 +++++++++++
 6 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boards/tqmba9xxxca/lowlevel.c b/arch/arm/boards/tqmba9xxxca/lowlevel.c
index 8207cd9515..761ca38dc4 100644
--- a/arch/arm/boards/tqmba9xxxca/lowlevel.c
+++ b/arch/arm/boards/tqmba9xxxca/lowlevel.c
@@ -17,6 +17,7 @@ static noinline void tqma9352_mba93xxca_continue(void)
 	void *base = IOMEM(MX9_UART1_BASE_ADDR);
 	void *muxbase = IOMEM(MX9_IOMUXC_BASE_ADDR);
 
+	writel(0x0, muxbase + 0x180);
 	writel(0x0, muxbase + 0x184);
 	imx9_uart_setup(IOMEM(base));
 	pbl_set_putc(lpuart32_putc, base + 0x10);
diff --git a/common/console.c b/common/console.c
index 7e43a9b5fc..4209561db9 100644
--- a/common/console.c
+++ b/common/console.c
@@ -452,6 +452,9 @@ static int getc_raw(void)
 	struct console_device *cdev;
 	int active = 0;
 
+	if (initialized < CONSOLE_INIT_FULL)
+		return getc_ll();
+
 	while (1) {
 		for_each_console(cdev) {
 			if (!(cdev->f_active & CONSOLE_STDIN))
@@ -478,6 +481,9 @@ static int tstc_raw(void)
 {
 	struct console_device *cdev;
 
+	if (initialized < CONSOLE_INIT_FULL)
+		return tstc_ll();
+
 	for_each_console(cdev) {
 		if (!(cdev->f_active & CONSOLE_STDIN))
 			continue;
diff --git a/common/startup.c b/common/startup.c
index bbba72f892..1f15dc5b91 100644
--- a/common/startup.c
+++ b/common/startup.c
@@ -174,11 +174,12 @@ enum autoboot_state do_autoboot_countdown(void)
 	if (autoboot_state != AUTOBOOT_UNKNOWN)
 		return autoboot_state;
 
+#ifndef GETC_LL
 	if (!console_get_first_active()) {
 		printf("\nNon-interactive console, booting system\n");
 		return autoboot_state = AUTOBOOT_BOOT;
 	}
-
+#endif
 	if (global_autoboot_state != AUTOBOOT_COUNTDOWN)
 		return global_autoboot_state;
 
diff --git a/include/debug_ll.h b/include/debug_ll.h
index 0128ab524a..84933657e6 100644
--- a/include/debug_ll.h
+++ b/include/debug_ll.h
@@ -29,6 +29,22 @@ static inline void putc_ll(char value)
 	PUTC_LL(value);
 }
 
+static inline int getc_ll(void)
+{
+#ifdef GETC_LL
+	return GETC_LL();
+#endif
+	return -EAGAIN;
+}
+
+static inline int tstc_ll(void)
+{
+#ifdef TSTC_LL
+	return TSTC_LL();
+#endif
+	return 0;
+}
+
 static inline void puthexc_ll(unsigned char value)
 {
 	int i; unsigned char ch;
diff --git a/include/mach/imx/debug_ll.h b/include/mach/imx/debug_ll.h
index 618cbc784e..e5abf1a216 100644
--- a/include/mach/imx/debug_ll.h
+++ b/include/mach/imx/debug_ll.h
@@ -134,6 +134,34 @@ static inline void PUTC_LL(int c)
 		imx_uart_putc(base, c);
 }
 
+#define GETC_LL GETC_LL
+static inline int GETC_LL(void)
+{
+	void __iomem *base = IOMEM(IMX_UART_BASE(IMX_DEBUG_SOC,
+						 CONFIG_DEBUG_IMX_UART_PORT));
+
+	if (!base)
+		return -EINVAL;
+
+	if (IS_ENABLED(CONFIG_DEBUG_IMX9_UART))
+		return lpuart32_getc(base + 0x10);
+	return -ENOTSUPP;
+}
+
+#define TSTC_LL TSTC_LL
+static inline int TSTC_LL(void)
+{
+	void __iomem *base = IOMEM(IMX_UART_BASE(IMX_DEBUG_SOC,
+						 CONFIG_DEBUG_IMX_UART_PORT));
+
+	if (!base)
+		return -EINVAL;
+
+	if (IS_ENABLED(CONFIG_DEBUG_IMX9_UART))
+		return lpuart32_tstc(base + 0x10);
+	return -ENOTSUPP;
+}
+
 #else
 
 static inline void imx50_uart_setup_ll(void) {}
diff --git a/include/serial/lpuart32.h b/include/serial/lpuart32.h
index 12526ee0ae..9df5e0937f 100644
--- a/include/serial/lpuart32.h
+++ b/include/serial/lpuart32.h
@@ -155,6 +155,18 @@ static inline void lpuart32_putc(void __iomem *base, int c)
 	writel(c, base + LPUART32_UARTDATA);
 }
 
+static inline int lpuart32_tstc(void __iomem *base)
+{
+	return readl(base + LPUART32_UARTSTAT) & LPUART32_UARTSTAT_RDRF;
+}
+
+static inline int lpuart32_getc(void __iomem *base)
+{
+	while (!lpuart32_tstc(base));
+
+	return readl(base + LPUART32_UARTDATA) & 0xff;
+}
+
 static inline void imx9_uart_setup(void __iomem *uartbase)
 {
 	/*
-- 
2.39.2

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] console: provide best-effort clk_get_for_console helper

[Ahmad Fatoum]

On 14.11.23 09:23, Sascha Hauer wrote:
> On Mon, Nov 13, 2023 at 04:02:56PM +0100, Ahmad Fatoum wrote:
>> On 13.11.23 14:03, Sascha Hauer wrote:
>>> On Thu, Nov 09, 2023 at 12:47:06PM +0100, Ahmad Fatoum wrote:
>>>> From: Ahmad Fatoum <ahmad@a3f.at>
>>>>
>>>> clk_get will return -EPROBE_DEFER if clock provider hasn't yet been
>>>> probed. In a system with deep probe enabled, dependencies are probed
>>>> on demand, so a -EPROBE_DEFER return is final and the console probe
>>>> will never succeed.
>>>>
>>>> CONFIG_DEBUG_LL is often used to debug this, but because the low-level
>>>> console is not interactive, it's a bit cumbersome. Improve upon this a
>>>> bit, by providing a clk_get_for_console helper that returns NULL if and
>>>> only if we are sure that a clock provider will not be probed.
>>>>
>>>> In that case, the driver can skip code paths initialization code and
>>>> baud rate setting dependent on having access to the clock and still
>>>> register a console that reuses what was set up by CONFIG_DEBUG_LL.
>>>>
>>>> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
>>>> ---
>>>>  include/console.h   | 26 ++++++++++++++++++++++++++
>>>>  include/linux/clk.h | 21 +++++++++++++++++++++
>>>>  2 files changed, 47 insertions(+)
>>>>
>>>> diff --git a/include/console.h b/include/console.h
>>>> index 586b68f73301..b8c901801e9f 100644
>>>> --- a/include/console.h
>>>> +++ b/include/console.h
>>>> @@ -8,6 +8,7 @@
>>>>  #define _CONSOLE_H_
>>>>  
>>>>  #include <param.h>
>>>> +#include <linux/clk.h>
>>>>  #include <linux/list.h>
>>>>  #include <driver.h>
>>>>  #include <serdev.h>
>>>> @@ -208,4 +209,29 @@ static inline void console_ctrlc_allow(void) { }
>>>>  static inline void console_ctrlc_forbid(void) { }
>>>>  #endif
>>>>  
>>>> +/**
>>>> + * clk_get_for_console - get clock, ignoring known unavailable clock controller
>>>> + * @dev: device for clock "consumer"
>>>> + * @id: clock consumer ID
>>>> + *
>>>> + * Return: a struct clk corresponding to the clock producer, a
>>>> + * valid IS_ERR() condition containing errno or NULL if it could
>>>> + * be determined that the clock producer will never be probed in
>>>> + * absence of modules. The NULL return allows serial drivers to
>>>> + * skip clock handling and rely on CONFIG_DEBUG_LL.
>>>> + */
>>>> +static inline struct clk *clk_get_for_console(struct device *dev, const char *id)
>>>> +{
>>>> +	struct clk *clk;
>>>> +
>>>> +	if (!IS_ENABLED(CONFIG_DEBUG_LL))
>>>> +		return clk_get(dev, id);
>>>
>>> There are several SoCs out there where the UART is enabled by the ROM
>>> already, on these we don't need to have CONFIG_DEBUG_LL enabled for a
>>> working UART.
>>
>> Those SoCs can still implement CONFIG_DEBUG_LL and just skip the
>> initialization step.
>>
>>> Maybe testing for the UART enable bit in probe() would be a better
>>> indication that the UART is already in a working state?
>>
>> If clk turns out to be not enabled, system would hang on e.g. i.MX.
> 
> That can happen with your patch as well as you don't limit the
> usage of clk_get_for_console() to the UART putc_ll is configured
> for. Initializing one of the other UARTs might hang your system
> once you access a register.

That's why it's a debugging measure behind DEBUG_LL, so you need to
opt-in into this.

> 
>>
>> This is a mere debugging measure for SoCs with complex clock controllers
>> backed by firmware, so I think having to enable CONFIG_DEBUG_LL to use
>> is acceptable.
> 
> Another option would be to implement and use a GETC_LL() macro. This
> requires some thinking how this can be integrated into the console
> code, but would in the end be more universally usable. With this we
> could make barebox interactive even when the real serial driver is
> not compiled in (or not even yet existing). See below for a prototype.

I don't think the DEBUG_LL API should be extended. Rather we should
look into how to inherit PBL console in barebox proper. But that's a
bigger change, thus the middle ground in my patch.

Cheers,
Ahmad

> 
> Sascha
> 
> ----------------------------8<-----------------------------
> 
> From a737458a64718f96c8d5267551895b0fcea23089 Mon Sep 17 00:00:00 2001
> From: Sascha Hauer <s.hauer@pengutronix.de>
> Date: Tue, 14 Nov 2023 09:22:24 +0100
> Subject: [PATCH] debug_ll: implement lowlevel debug input functions
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/boards/tqmba9xxxca/lowlevel.c |  1 +
>  common/console.c                       |  6 ++++++
>  common/startup.c                       |  3 ++-
>  include/debug_ll.h                     | 16 +++++++++++++++
>  include/mach/imx/debug_ll.h            | 28 ++++++++++++++++++++++++++
>  include/serial/lpuart32.h              | 12 +++++++++++
>  6 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boards/tqmba9xxxca/lowlevel.c b/arch/arm/boards/tqmba9xxxca/lowlevel.c
> index 8207cd9515..761ca38dc4 100644
> --- a/arch/arm/boards/tqmba9xxxca/lowlevel.c
> +++ b/arch/arm/boards/tqmba9xxxca/lowlevel.c
> @@ -17,6 +17,7 @@ static noinline void tqma9352_mba93xxca_continue(void)
>  	void *base = IOMEM(MX9_UART1_BASE_ADDR);
>  	void *muxbase = IOMEM(MX9_IOMUXC_BASE_ADDR);
>  
> +	writel(0x0, muxbase + 0x180);
>  	writel(0x0, muxbase + 0x184);
>  	imx9_uart_setup(IOMEM(base));
>  	pbl_set_putc(lpuart32_putc, base + 0x10);
> diff --git a/common/console.c b/common/console.c
> index 7e43a9b5fc..4209561db9 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -452,6 +452,9 @@ static int getc_raw(void)
>  	struct console_device *cdev;
>  	int active = 0;
>  
> +	if (initialized < CONSOLE_INIT_FULL)
> +		return getc_ll();
> +
>  	while (1) {
>  		for_each_console(cdev) {
>  			if (!(cdev->f_active & CONSOLE_STDIN))
> @@ -478,6 +481,9 @@ static int tstc_raw(void)
>  {
>  	struct console_device *cdev;
>  
> +	if (initialized < CONSOLE_INIT_FULL)
> +		return tstc_ll();
> +
>  	for_each_console(cdev) {
>  		if (!(cdev->f_active & CONSOLE_STDIN))
>  			continue;
> diff --git a/common/startup.c b/common/startup.c
> index bbba72f892..1f15dc5b91 100644
> --- a/common/startup.c
> +++ b/common/startup.c
> @@ -174,11 +174,12 @@ enum autoboot_state do_autoboot_countdown(void)
>  	if (autoboot_state != AUTOBOOT_UNKNOWN)
>  		return autoboot_state;
>  
> +#ifndef GETC_LL
>  	if (!console_get_first_active()) {
>  		printf("\nNon-interactive console, booting system\n");
>  		return autoboot_state = AUTOBOOT_BOOT;
>  	}
> -
> +#endif
>  	if (global_autoboot_state != AUTOBOOT_COUNTDOWN)
>  		return global_autoboot_state;
>  
> diff --git a/include/debug_ll.h b/include/debug_ll.h
> index 0128ab524a..84933657e6 100644
> --- a/include/debug_ll.h
> +++ b/include/debug_ll.h
> @@ -29,6 +29,22 @@ static inline void putc_ll(char value)
>  	PUTC_LL(value);
>  }
>  
> +static inline int getc_ll(void)
> +{
> +#ifdef GETC_LL
> +	return GETC_LL();
> +#endif
> +	return -EAGAIN;
> +}
> +
> +static inline int tstc_ll(void)
> +{
> +#ifdef TSTC_LL
> +	return TSTC_LL();
> +#endif
> +	return 0;
> +}
> +
>  static inline void puthexc_ll(unsigned char value)
>  {
>  	int i; unsigned char ch;
> diff --git a/include/mach/imx/debug_ll.h b/include/mach/imx/debug_ll.h
> index 618cbc784e..e5abf1a216 100644
> --- a/include/mach/imx/debug_ll.h
> +++ b/include/mach/imx/debug_ll.h
> @@ -134,6 +134,34 @@ static inline void PUTC_LL(int c)
>  		imx_uart_putc(base, c);
>  }
>  
> +#define GETC_LL GETC_LL
> +static inline int GETC_LL(void)
> +{
> +	void __iomem *base = IOMEM(IMX_UART_BASE(IMX_DEBUG_SOC,
> +						 CONFIG_DEBUG_IMX_UART_PORT));
> +
> +	if (!base)
> +		return -EINVAL;
> +
> +	if (IS_ENABLED(CONFIG_DEBUG_IMX9_UART))
> +		return lpuart32_getc(base + 0x10);
> +	return -ENOTSUPP;
> +}
> +
> +#define TSTC_LL TSTC_LL
> +static inline int TSTC_LL(void)
> +{
> +	void __iomem *base = IOMEM(IMX_UART_BASE(IMX_DEBUG_SOC,
> +						 CONFIG_DEBUG_IMX_UART_PORT));
> +
> +	if (!base)
> +		return -EINVAL;
> +
> +	if (IS_ENABLED(CONFIG_DEBUG_IMX9_UART))
> +		return lpuart32_tstc(base + 0x10);
> +	return -ENOTSUPP;
> +}
> +
>  #else
>  
>  static inline void imx50_uart_setup_ll(void) {}
> diff --git a/include/serial/lpuart32.h b/include/serial/lpuart32.h
> index 12526ee0ae..9df5e0937f 100644
> --- a/include/serial/lpuart32.h
> +++ b/include/serial/lpuart32.h
> @@ -155,6 +155,18 @@ static inline void lpuart32_putc(void __iomem *base, int c)
>  	writel(c, base + LPUART32_UARTDATA);
>  }
>  
> +static inline int lpuart32_tstc(void __iomem *base)
> +{
> +	return readl(base + LPUART32_UARTSTAT) & LPUART32_UARTSTAT_RDRF;
> +}
> +
> +static inline int lpuart32_getc(void __iomem *base)
> +{
> +	while (!lpuart32_tstc(base));
> +
> +	return readl(base + LPUART32_UARTDATA) & 0xff;
> +}
> +
>  static inline void imx9_uart_setup(void __iomem *uartbase)
>  {
>  	/*

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] console: provide best-effort clk_get_for_console helper

[Sascha Hauer]

On Tue, Nov 14, 2023 at 09:33:47AM +0100, Ahmad Fatoum wrote:
> On 14.11.23 09:23, Sascha Hauer wrote:
> > On Mon, Nov 13, 2023 at 04:02:56PM +0100, Ahmad Fatoum wrote:
> >> On 13.11.23 14:03, Sascha Hauer wrote:
> >>> On Thu, Nov 09, 2023 at 12:47:06PM +0100, Ahmad Fatoum wrote:
> >>>> From: Ahmad Fatoum <ahmad@a3f.at>
> >>>>
> >>>> clk_get will return -EPROBE_DEFER if clock provider hasn't yet been
> >>>> probed. In a system with deep probe enabled, dependencies are probed
> >>>> on demand, so a -EPROBE_DEFER return is final and the console probe
> >>>> will never succeed.
> >>>>
> >>>> CONFIG_DEBUG_LL is often used to debug this, but because the low-level
> >>>> console is not interactive, it's a bit cumbersome. Improve upon this a
> >>>> bit, by providing a clk_get_for_console helper that returns NULL if and
> >>>> only if we are sure that a clock provider will not be probed.
> >>>>
> >>>> In that case, the driver can skip code paths initialization code and
> >>>> baud rate setting dependent on having access to the clock and still
> >>>> register a console that reuses what was set up by CONFIG_DEBUG_LL.
> >>>>
> >>>> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> >>>> ---
> >>>>  include/console.h   | 26 ++++++++++++++++++++++++++
> >>>>  include/linux/clk.h | 21 +++++++++++++++++++++
> >>>>  2 files changed, 47 insertions(+)
> >>>>
> >>>> diff --git a/include/console.h b/include/console.h
> >>>> index 586b68f73301..b8c901801e9f 100644
> >>>> --- a/include/console.h
> >>>> +++ b/include/console.h
> >>>> @@ -8,6 +8,7 @@
> >>>>  #define _CONSOLE_H_
> >>>>  
> >>>>  #include <param.h>
> >>>> +#include <linux/clk.h>
> >>>>  #include <linux/list.h>
> >>>>  #include <driver.h>
> >>>>  #include <serdev.h>
> >>>> @@ -208,4 +209,29 @@ static inline void console_ctrlc_allow(void) { }
> >>>>  static inline void console_ctrlc_forbid(void) { }
> >>>>  #endif
> >>>>  
> >>>> +/**
> >>>> + * clk_get_for_console - get clock, ignoring known unavailable clock controller
> >>>> + * @dev: device for clock "consumer"
> >>>> + * @id: clock consumer ID
> >>>> + *
> >>>> + * Return: a struct clk corresponding to the clock producer, a
> >>>> + * valid IS_ERR() condition containing errno or NULL if it could
> >>>> + * be determined that the clock producer will never be probed in
> >>>> + * absence of modules. The NULL return allows serial drivers to
> >>>> + * skip clock handling and rely on CONFIG_DEBUG_LL.
> >>>> + */
> >>>> +static inline struct clk *clk_get_for_console(struct device *dev, const char *id)
> >>>> +{
> >>>> +	struct clk *clk;
> >>>> +
> >>>> +	if (!IS_ENABLED(CONFIG_DEBUG_LL))
> >>>> +		return clk_get(dev, id);
> >>>
> >>> There are several SoCs out there where the UART is enabled by the ROM
> >>> already, on these we don't need to have CONFIG_DEBUG_LL enabled for a
> >>> working UART.
> >>
> >> Those SoCs can still implement CONFIG_DEBUG_LL and just skip the
> >> initialization step.
> >>
> >>> Maybe testing for the UART enable bit in probe() would be a better
> >>> indication that the UART is already in a working state?
> >>
> >> If clk turns out to be not enabled, system would hang on e.g. i.MX.
> > 
> > That can happen with your patch as well as you don't limit the
> > usage of clk_get_for_console() to the UART putc_ll is configured
> > for. Initializing one of the other UARTs might hang your system
> > once you access a register.
> 
> That's why it's a debugging measure behind DEBUG_LL, so you need to
> opt-in into this.

The opt-in in your case is that you decided that this quirk works with
the stm32 UART driver, but really it might only work on the SoC you
tested it with and maybe only with this specific firmware version which
uses clocks provided via SCMI.

> 
> > 
> >>
> >> This is a mere debugging measure for SoCs with complex clock controllers
> >> backed by firmware, so I think having to enable CONFIG_DEBUG_LL to use
> >> is acceptable.
> > 
> > Another option would be to implement and use a GETC_LL() macro. This
> > requires some thinking how this can be integrated into the console
> > code, but would in the end be more universally usable. With this we
> > could make barebox interactive even when the real serial driver is
> > not compiled in (or not even yet existing). See below for a prototype.
> 
> I don't think the DEBUG_LL API should be extended. Rather we should
> look into how to inherit PBL console in barebox proper. But that's a
> bigger change, thus the middle ground in my patch.

Fine with me, but then please let's don't clutter the current code with
wobbly special purpose solutions.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] console: provide best-effort clk_get_for_console helper

[Ahmad Fatoum]

On 14.11.23 09:46, Sascha Hauer wrote:
> On Tue, Nov 14, 2023 at 09:33:47AM +0100, Ahmad Fatoum wrote:
>> On 14.11.23 09:23, Sascha Hauer wrote:
>>> On Mon, Nov 13, 2023 at 04:02:56PM +0100, Ahmad Fatoum wrote:
>>>> On 13.11.23 14:03, Sascha Hauer wrote:
>>>>> On Thu, Nov 09, 2023 at 12:47:06PM +0100, Ahmad Fatoum wrote:
>>>>>> From: Ahmad Fatoum <ahmad@a3f.at>
>>>>>>
>>>>>> clk_get will return -EPROBE_DEFER if clock provider hasn't yet been
>>>>>> probed. In a system with deep probe enabled, dependencies are probed
>>>>>> on demand, so a -EPROBE_DEFER return is final and the console probe
>>>>>> will never succeed.
>>>>>>
>>>>>> CONFIG_DEBUG_LL is often used to debug this, but because the low-level
>>>>>> console is not interactive, it's a bit cumbersome. Improve upon this a
>>>>>> bit, by providing a clk_get_for_console helper that returns NULL if and
>>>>>> only if we are sure that a clock provider will not be probed.
>>>>>>
>>>>>> In that case, the driver can skip code paths initialization code and
>>>>>> baud rate setting dependent on having access to the clock and still
>>>>>> register a console that reuses what was set up by CONFIG_DEBUG_LL.
>>>>>>
>>>>>> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
>>>>>> ---
>>>>>>  include/console.h   | 26 ++++++++++++++++++++++++++
>>>>>>  include/linux/clk.h | 21 +++++++++++++++++++++
>>>>>>  2 files changed, 47 insertions(+)
>>>>>>
>>>>>> diff --git a/include/console.h b/include/console.h
>>>>>> index 586b68f73301..b8c901801e9f 100644
>>>>>> --- a/include/console.h
>>>>>> +++ b/include/console.h
>>>>>> @@ -8,6 +8,7 @@
>>>>>>  #define _CONSOLE_H_
>>>>>>  
>>>>>>  #include <param.h>
>>>>>> +#include <linux/clk.h>
>>>>>>  #include <linux/list.h>
>>>>>>  #include <driver.h>
>>>>>>  #include <serdev.h>
>>>>>> @@ -208,4 +209,29 @@ static inline void console_ctrlc_allow(void) { }
>>>>>>  static inline void console_ctrlc_forbid(void) { }
>>>>>>  #endif
>>>>>>  
>>>>>> +/**
>>>>>> + * clk_get_for_console - get clock, ignoring known unavailable clock controller
>>>>>> + * @dev: device for clock "consumer"
>>>>>> + * @id: clock consumer ID
>>>>>> + *
>>>>>> + * Return: a struct clk corresponding to the clock producer, a
>>>>>> + * valid IS_ERR() condition containing errno or NULL if it could
>>>>>> + * be determined that the clock producer will never be probed in
>>>>>> + * absence of modules. The NULL return allows serial drivers to
>>>>>> + * skip clock handling and rely on CONFIG_DEBUG_LL.
>>>>>> + */
>>>>>> +static inline struct clk *clk_get_for_console(struct device *dev, const char *id)
>>>>>> +{
>>>>>> +	struct clk *clk;
>>>>>> +
>>>>>> +	if (!IS_ENABLED(CONFIG_DEBUG_LL))
>>>>>> +		return clk_get(dev, id);
>>>>>
>>>>> There are several SoCs out there where the UART is enabled by the ROM
>>>>> already, on these we don't need to have CONFIG_DEBUG_LL enabled for a
>>>>> working UART.
>>>>
>>>> Those SoCs can still implement CONFIG_DEBUG_LL and just skip the
>>>> initialization step.
>>>>
>>>>> Maybe testing for the UART enable bit in probe() would be a better
>>>>> indication that the UART is already in a working state?
>>>>
>>>> If clk turns out to be not enabled, system would hang on e.g. i.MX.
>>>
>>> That can happen with your patch as well as you don't limit the
>>> usage of clk_get_for_console() to the UART putc_ll is configured
>>> for. Initializing one of the other UARTs might hang your system
>>> once you access a register.
>>
>> That's why it's a debugging measure behind DEBUG_LL, so you need to
>> opt-in into this.
> 
> The opt-in in your case is that you decided that this quirk works with
> the stm32 UART driver, but really it might only work on the SoC you
> tested it with and maybe only with this specific firmware version which
> uses clocks provided via SCMI.

STM32MP13 is currently broken, because upstream changed clock controller
DT. This is an easy way to have users at least get to an interactive
shell to be able to do something about this instead of staying on DEBUG_LL
shell with no interaction. FWIW, I originally wrote this patch for ZynqMP,
which also has a firmware-managed clock controller and it worked equally
well there. For bring-up, it can also be useful.


> 
>>
>>>
>>>>
>>>> This is a mere debugging measure for SoCs with complex clock controllers
>>>> backed by firmware, so I think having to enable CONFIG_DEBUG_LL to use
>>>> is acceptable.
>>>
>>> Another option would be to implement and use a GETC_LL() macro. This
>>> requires some thinking how this can be integrated into the console
>>> code, but would in the end be more universally usable. With this we
>>> could make barebox interactive even when the real serial driver is
>>> not compiled in (or not even yet existing). See below for a prototype.
>>
>> I don't think the DEBUG_LL API should be extended. Rather we should
>> look into how to inherit PBL console in barebox proper. But that's a
>> bigger change, thus the middle ground in my patch.
> 
> Fine with me, but then please let's don't clutter the current code with
> wobbly special purpose solutions.

What's wobbly about it? I consider it rather elegant. If we have DEBUG_LL
and no clock driver -> Just assume DEBUG_LL will have enabled the necessary
clocks and have the user reach a shell.

Cheers,
Ahmad

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] console: provide best-effort clk_get_for_console helper

[Sascha Hauer]

On Tue, Nov 14, 2023 at 09:54:57AM +0100, Ahmad Fatoum wrote:
> On 14.11.23 09:46, Sascha Hauer wrote:
> > On Tue, Nov 14, 2023 at 09:33:47AM +0100, Ahmad Fatoum wrote:
> >> On 14.11.23 09:23, Sascha Hauer wrote:
> >>> On Mon, Nov 13, 2023 at 04:02:56PM +0100, Ahmad Fatoum wrote:
> >>>> On 13.11.23 14:03, Sascha Hauer wrote:
> >>>>> On Thu, Nov 09, 2023 at 12:47:06PM +0100, Ahmad Fatoum wrote:
> >>>>>> From: Ahmad Fatoum <ahmad@a3f.at>
> >>>>>>
> >>>>>> clk_get will return -EPROBE_DEFER if clock provider hasn't yet been
> >>>>>> probed. In a system with deep probe enabled, dependencies are probed
> >>>>>> on demand, so a -EPROBE_DEFER return is final and the console probe
> >>>>>> will never succeed.
> >>>>>>
> >>>>>> CONFIG_DEBUG_LL is often used to debug this, but because the low-level
> >>>>>> console is not interactive, it's a bit cumbersome. Improve upon this a
> >>>>>> bit, by providing a clk_get_for_console helper that returns NULL if and
> >>>>>> only if we are sure that a clock provider will not be probed.
> >>>>>>
> >>>>>> In that case, the driver can skip code paths initialization code and
> >>>>>> baud rate setting dependent on having access to the clock and still
> >>>>>> register a console that reuses what was set up by CONFIG_DEBUG_LL.
> >>>>>>
> >>>>>> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> >>>>>> ---
> >>>>>>  include/console.h   | 26 ++++++++++++++++++++++++++
> >>>>>>  include/linux/clk.h | 21 +++++++++++++++++++++
> >>>>>>  2 files changed, 47 insertions(+)
> >>>>>>
> >>>>>> diff --git a/include/console.h b/include/console.h
> >>>>>> index 586b68f73301..b8c901801e9f 100644
> >>>>>> --- a/include/console.h
> >>>>>> +++ b/include/console.h
> >>>>>> @@ -8,6 +8,7 @@
> >>>>>>  #define _CONSOLE_H_
> >>>>>>  
> >>>>>>  #include <param.h>
> >>>>>> +#include <linux/clk.h>
> >>>>>>  #include <linux/list.h>
> >>>>>>  #include <driver.h>
> >>>>>>  #include <serdev.h>
> >>>>>> @@ -208,4 +209,29 @@ static inline void console_ctrlc_allow(void) { }
> >>>>>>  static inline void console_ctrlc_forbid(void) { }
> >>>>>>  #endif
> >>>>>>  
> >>>>>> +/**
> >>>>>> + * clk_get_for_console - get clock, ignoring known unavailable clock controller
> >>>>>> + * @dev: device for clock "consumer"
> >>>>>> + * @id: clock consumer ID
> >>>>>> + *
> >>>>>> + * Return: a struct clk corresponding to the clock producer, a
> >>>>>> + * valid IS_ERR() condition containing errno or NULL if it could
> >>>>>> + * be determined that the clock producer will never be probed in
> >>>>>> + * absence of modules. The NULL return allows serial drivers to
> >>>>>> + * skip clock handling and rely on CONFIG_DEBUG_LL.
> >>>>>> + */
> >>>>>> +static inline struct clk *clk_get_for_console(struct device *dev, const char *id)
> >>>>>> +{
> >>>>>> +	struct clk *clk;
> >>>>>> +
> >>>>>> +	if (!IS_ENABLED(CONFIG_DEBUG_LL))
> >>>>>> +		return clk_get(dev, id);
> >>>>>
> >>>>> There are several SoCs out there where the UART is enabled by the ROM
> >>>>> already, on these we don't need to have CONFIG_DEBUG_LL enabled for a
> >>>>> working UART.
> >>>>
> >>>> Those SoCs can still implement CONFIG_DEBUG_LL and just skip the
> >>>> initialization step.
> >>>>
> >>>>> Maybe testing for the UART enable bit in probe() would be a better
> >>>>> indication that the UART is already in a working state?
> >>>>
> >>>> If clk turns out to be not enabled, system would hang on e.g. i.MX.
> >>>
> >>> That can happen with your patch as well as you don't limit the
> >>> usage of clk_get_for_console() to the UART putc_ll is configured
> >>> for. Initializing one of the other UARTs might hang your system
> >>> once you access a register.
> >>
> >> That's why it's a debugging measure behind DEBUG_LL, so you need to
> >> opt-in into this.
> > 
> > The opt-in in your case is that you decided that this quirk works with
> > the stm32 UART driver, but really it might only work on the SoC you
> > tested it with and maybe only with this specific firmware version which
> > uses clocks provided via SCMI.
> 
> STM32MP13 is currently broken, because upstream changed clock controller
> DT. This is an easy way to have users at least get to an interactive
> shell to be able to do something about this instead of staying on DEBUG_LL
> shell with no interaction. FWIW, I originally wrote this patch for ZynqMP,
> which also has a firmware-managed clock controller and it worked equally
> well there. For bring-up, it can also be useful.
> 
> 
> > 
> >>
> >>>
> >>>>
> >>>> This is a mere debugging measure for SoCs with complex clock controllers
> >>>> backed by firmware, so I think having to enable CONFIG_DEBUG_LL to use
> >>>> is acceptable.
> >>>
> >>> Another option would be to implement and use a GETC_LL() macro. This
> >>> requires some thinking how this can be integrated into the console
> >>> code, but would in the end be more universally usable. With this we
> >>> could make barebox interactive even when the real serial driver is
> >>> not compiled in (or not even yet existing). See below for a prototype.
> >>
> >> I don't think the DEBUG_LL API should be extended. Rather we should
> >> look into how to inherit PBL console in barebox proper. But that's a
> >> bigger change, thus the middle ground in my patch.
> > 
> > Fine with me, but then please let's don't clutter the current code with
> > wobbly special purpose solutions.
> 
> What's wobbly about it? I consider it rather elegant. If we have DEBUG_LL
> and no clock driver -> Just assume DEBUG_LL will have enabled the necessary
> clocks and have the user reach a shell.

DEBUG_LL will at maximum enable the clocks needed for the one UART used
as debug UART, but not the clocks for the other UARTs. Maybe your
firmware enables the clocks for all UARTs by default, but you can't tell this is
the case for all firmware versions. Maybe accessing UART registers with
dsabled clocks works on the one SoC you tested it, but you can't tell
this works for other SoCs.

Maybe you carefully tested all possible SoC/firmware combinations this
UART is used on, but what if somebody comes along and sends patches for
using this pattern on other UARTs? In that case there's no way to find
out if the patch is safe or not.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] console: provide best-effort clk_get_for_console helper

[Ahmad Fatoum]

On 14.11.23 10:56, Sascha Hauer wrote:
> On Tue, Nov 14, 2023 at 09:54:57AM +0100, Ahmad Fatoum wrote:
>> On 14.11.23 09:46, Sascha Hauer wrote:
>>> On Tue, Nov 14, 2023 at 09:33:47AM +0100, Ahmad Fatoum wrote:
>>>> On 14.11.23 09:23, Sascha Hauer wrote:
>>>>> On Mon, Nov 13, 2023 at 04:02:56PM +0100, Ahmad Fatoum wrote:
>>>>>> On 13.11.23 14:03, Sascha Hauer wrote:
>>>>>>> On Thu, Nov 09, 2023 at 12:47:06PM +0100, Ahmad Fatoum wrote:
>>>>>>>> From: Ahmad Fatoum <ahmad@a3f.at>
>>>>>>>>
>>>>>>>> clk_get will return -EPROBE_DEFER if clock provider hasn't yet been
>>>>>>>> probed. In a system with deep probe enabled, dependencies are probed
>>>>>>>> on demand, so a -EPROBE_DEFER return is final and the console probe
>>>>>>>> will never succeed.
>>>>>>>>
>>>>>>>> CONFIG_DEBUG_LL is often used to debug this, but because the low-level
>>>>>>>> console is not interactive, it's a bit cumbersome. Improve upon this a
>>>>>>>> bit, by providing a clk_get_for_console helper that returns NULL if and
>>>>>>>> only if we are sure that a clock provider will not be probed.
>>>>>>>>
>>>>>>>> In that case, the driver can skip code paths initialization code and
>>>>>>>> baud rate setting dependent on having access to the clock and still
>>>>>>>> register a console that reuses what was set up by CONFIG_DEBUG_LL.
>>>>>>>>
>>>>>>>> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
>>>>>>>> ---
>>>>>>>>  include/console.h   | 26 ++++++++++++++++++++++++++
>>>>>>>>  include/linux/clk.h | 21 +++++++++++++++++++++
>>>>>>>>  2 files changed, 47 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/include/console.h b/include/console.h
>>>>>>>> index 586b68f73301..b8c901801e9f 100644
>>>>>>>> --- a/include/console.h
>>>>>>>> +++ b/include/console.h
>>>>>>>> @@ -8,6 +8,7 @@
>>>>>>>>  #define _CONSOLE_H_
>>>>>>>>  
>>>>>>>>  #include <param.h>
>>>>>>>> +#include <linux/clk.h>
>>>>>>>>  #include <linux/list.h>
>>>>>>>>  #include <driver.h>
>>>>>>>>  #include <serdev.h>
>>>>>>>> @@ -208,4 +209,29 @@ static inline void console_ctrlc_allow(void) { }
>>>>>>>>  static inline void console_ctrlc_forbid(void) { }
>>>>>>>>  #endif
>>>>>>>>  
>>>>>>>> +/**
>>>>>>>> + * clk_get_for_console - get clock, ignoring known unavailable clock controller
>>>>>>>> + * @dev: device for clock "consumer"
>>>>>>>> + * @id: clock consumer ID
>>>>>>>> + *
>>>>>>>> + * Return: a struct clk corresponding to the clock producer, a
>>>>>>>> + * valid IS_ERR() condition containing errno or NULL if it could
>>>>>>>> + * be determined that the clock producer will never be probed in
>>>>>>>> + * absence of modules. The NULL return allows serial drivers to
>>>>>>>> + * skip clock handling and rely on CONFIG_DEBUG_LL.
>>>>>>>> + */
>>>>>>>> +static inline struct clk *clk_get_for_console(struct device *dev, const char *id)
>>>>>>>> +{
>>>>>>>> +	struct clk *clk;
>>>>>>>> +
>>>>>>>> +	if (!IS_ENABLED(CONFIG_DEBUG_LL))
>>>>>>>> +		return clk_get(dev, id);
>>>>>>>
>>>>>>> There are several SoCs out there where the UART is enabled by the ROM
>>>>>>> already, on these we don't need to have CONFIG_DEBUG_LL enabled for a
>>>>>>> working UART.
>>>>>>
>>>>>> Those SoCs can still implement CONFIG_DEBUG_LL and just skip the
>>>>>> initialization step.
>>>>>>
>>>>>>> Maybe testing for the UART enable bit in probe() would be a better
>>>>>>> indication that the UART is already in a working state?
>>>>>>
>>>>>> If clk turns out to be not enabled, system would hang on e.g. i.MX.
>>>>>
>>>>> That can happen with your patch as well as you don't limit the
>>>>> usage of clk_get_for_console() to the UART putc_ll is configured
>>>>> for. Initializing one of the other UARTs might hang your system
>>>>> once you access a register.
>>>>
>>>> That's why it's a debugging measure behind DEBUG_LL, so you need to
>>>> opt-in into this.
>>>
>>> The opt-in in your case is that you decided that this quirk works with
>>> the stm32 UART driver, but really it might only work on the SoC you
>>> tested it with and maybe only with this specific firmware version which
>>> uses clocks provided via SCMI.
>>
>> STM32MP13 is currently broken, because upstream changed clock controller
>> DT. This is an easy way to have users at least get to an interactive
>> shell to be able to do something about this instead of staying on DEBUG_LL
>> shell with no interaction. FWIW, I originally wrote this patch for ZynqMP,
>> which also has a firmware-managed clock controller and it worked equally
>> well there. For bring-up, it can also be useful.
>>
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> This is a mere debugging measure for SoCs with complex clock controllers
>>>>>> backed by firmware, so I think having to enable CONFIG_DEBUG_LL to use
>>>>>> is acceptable.
>>>>>
>>>>> Another option would be to implement and use a GETC_LL() macro. This
>>>>> requires some thinking how this can be integrated into the console
>>>>> code, but would in the end be more universally usable. With this we
>>>>> could make barebox interactive even when the real serial driver is
>>>>> not compiled in (or not even yet existing). See below for a prototype.
>>>>
>>>> I don't think the DEBUG_LL API should be extended. Rather we should
>>>> look into how to inherit PBL console in barebox proper. But that's a
>>>> bigger change, thus the middle ground in my patch.
>>>
>>> Fine with me, but then please let's don't clutter the current code with
>>> wobbly special purpose solutions.
>>
>> What's wobbly about it? I consider it rather elegant. If we have DEBUG_LL
>> and no clock driver -> Just assume DEBUG_LL will have enabled the necessary
>> clocks and have the user reach a shell.
> 
> DEBUG_LL will at maximum enable the clocks needed for the one UART used
> as debug UART, but not the clocks for the other UARTs. Maybe your
> firmware enables the clocks for all UARTs by default, but you can't tell this is
> the case for all firmware versions. Maybe accessing UART registers with
> dsabled clocks works on the one SoC you tested it, but you can't tell
> this works for other SoCs.

If I restrict clk_get_for_console() to return NULL only for the stdout
console, would this alleviate your concerns?

> Maybe you carefully tested all possible SoC/firmware combinations this
> UART is used on, but what if somebody comes along and sends patches for
> using this pattern on other UARTs? In that case there's no way to find
> out if the patch is safe or not.

I can't follow your objection honestly. This is only relevant if clock
controller support is unexpectedly missing. In that case, you don't have
a booting system anyway, but let's at least have a shell.

Cheers,
Ahmad

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |