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@xxxxxx>
>>>>
>>>> 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@xxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> ---
> 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 |
