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

[Sascha Hauer <sha@pengutronix.de>]

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 |