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 <ah...@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 <ah...@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.ha...@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.ha...@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 |