Re: [RFC] serial: mxc: get the clock frequency from the used clock for the device

[Sean Anderson <seanga2@gmail.com>]

On 3/18/22 4:05 AM, Heiko Thiery wrote:
> Hi Sean,
> 
> Am Fr., 18. März 2022 um 03:19 Uhr schrieb Sean Anderson <seanga2@gmail.com>:
>>
>> On 3/17/22 3:14 PM, Heiko Thiery wrote:
>>> Hi Sean,
>>>
>>> Am Do., 17. März 2022 um 15:38 Uhr schrieb Sean Anderson <seanga2@gmail.com>:
>>>>
>>>> Hi Heiko,
>>>>
>>>> On 3/17/22 8:41 AM, Heiko Thiery wrote:
>>>>> With the clock driver enabled for the imx8mq, it was noticed that the
>>>>> frequency used to calculate the baud rate is always taken from the root
>>>>> clock of UART1. This can cause problems if UART1 is not used as console
>>>>> and the settings are different from UART1. The result is that the console
>>>>> output is garbage. To do this correctly the UART frequency is taken from
>>>>> the used device. For the implementations that don't have the igp clock
>>>>> frequency written or can't return it the old way is tried.
>>>>>
>>>>> Signed-off-by: Heiko Thiery <heiko.thiery@gmail.com>
>>>>> ---
>>>>>     drivers/serial/serial_mxc.c | 15 +++++++++++++--
>>>>>     1 file changed, 13 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
>>>>> index e4970a169b..6fdb2b2397 100644
>>>>> --- a/drivers/serial/serial_mxc.c
>>>>> +++ b/drivers/serial/serial_mxc.c
>>>>> @@ -3,6 +3,7 @@
>>>>>      * (c) 2007 Sascha Hauer <s.hauer@pengutronix.de>
>>>>>      */
>>>>>
>>>>> +#include <clk.h>
>>>>>     #include <common.h>
>>>>>     #include <dm.h>
>>>>>     #include <errno.h>
>>>>> @@ -266,9 +267,19 @@ __weak struct serial_device *default_serial_console(void)
>>>>>     int mxc_serial_setbrg(struct udevice *dev, int baudrate)
>>>>>     {
>>>>>         struct mxc_serial_plat *plat = dev_get_plat(dev);
>>>>> -     u32 clk = imx_get_uartclk();
>>>>> +     u32 rate = 0;
>>>>> +
>>>>> +     if (IS_ENABLED(CONFIG_CLK)) {
>>>>
>>>> CONFIG_IS_ENABLED?
>>>
>>> This should be correct. The CONFIG_IS_ENABLED is a preprocessor macro
>>> and the IS_ENABLED can be used in c code. Or do you mean something
>>> else?
>>
>> I mean you should be using CONFIG_IS_ENABLED(CLK) so that this code is
>> correct for both SPL and U-Boot proper. But it is also fine to "try and
>> fail" (making this check unnecessary).
>>
>>>>
>>>> mx6ull at least does not have CONFIG_SPL_CLK enabled.
>>>
>>> I expect that in that case it will fallback to the old behavior ... not?
>>
>> Yes, if you handle -ENOSYS correctly.
>>
>>>>
>>>>> +             struct clk clk;
>>>>> +             if(!clk_get_by_name(dev, "ipg", &clk))
>>>>> +                     rate = clk_get_rate(&clk);
>>
>> You may also need to enable this clock.
> 
> What would be the right place to enable the clock? in mxc_serial_probe()?

Yes.

--Sean