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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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 |