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

[Angus Ainslie <angus@akkea.ca>]

On 2022-03-21 06:50, Heiko Thiery wrote:
> Hi Angus,
> 
> [snip]
> 
>> > So I'm not sure if the ipg clock is the right one for the boards that
>> > has different clock for ipg and per.
>> 
>> So I only looked at imx6qdl.dtsi where the clocks are different
>> 
>> clocks = <&clks IMX6QDL_CLK_UART_IPG>,
>>           <&clks IMX6QDL_CLK_UART_SERIAL>;
>> clock-names = "ipg", "per";
>> 
>> And from that file it looks like the per clock would be the correct 
>> one.
> 
> Yes, 'per' seems to be the right one.
> 
>> Should the clock be looked up by id instead of by name and then have a
>> different code path for each imx board type ?
> 
> But how to get the right clk id? The id's for all the implementations
> are different. Or not?
> 

Yeah you're correct that won't work.

>> 
>> >
>> >> > +     }
>> >> > +
>> >> > +     /* as fallback we try to get the clk rate that way */
>> >> > +     if (rate == 0)
>> >> > +             rate = imx_get_uartclk();
>> >>
>> >> Would it be better to re-write imx_get_uartclk so that both the
>> >> getting
>> >> and setting of clocks was correct ?
>> >
>> > I do not understand what you mean with that.
>> >
>> 
>> There are other places in the code that imx_get_uartclk gets called. 
>> If
>> an index was added to imx_get_uartclk(int index) then you wouldn't 
>> need
>> the code above in the mxc_serial_setbrg function. That would also make
>> all of the places where imx_get_uartclk gets called return the correct
>> value.
> 
> By index do you mean the clk id?
> 

No I was thinking number of the device uart[0-3].

Thinking about it some more it's probably not useful as you already have 
the udevice pointer. I was just trying to think of ways to reduce the 
amount of code change for the older SOCs. Using the 'per' clock is a 
better solution.

>> 
>> It might make sense to create a new function imx7_get_uartclk that 
>> gets
>> called on newer SOCs so that the imx6 and earlier code doesn't need to
>> get changed.
> 
> At what point should this be called instead of the imx_get_uartclk() 
> function?
> 

I was thinking a CONFIG_IS_ENBLED switch between the old version and the 
new one based on SOC.

Angus