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

[Adam Ford]

On Tue, Mar 22, 2022 at 7:48 AM Angus Ainslie <angus at akkea.ca> wrote:
>
> 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.

I was thinking that we could expand the struct mxc_serial_plat to
include both per and igp clocks to cover devices have clocks that are
not the same.  The configuring of the platdata can happen using
mxc_serial_of_to_plat to 'get' both igp and per clocks.  The probe
would enable both per and igp to ensure they are operating, then the
mxc_serial_setbrg could use clk_get_rate(plat->clk_igp) to determine
the clock rate.

I am guessing the clock composite would have to be expanded to include
the UART clocks because from what I can see, they're not included.
However, this could potentially eliminate the need to use some of the
functions in arch/arm/mach-imx/imx8m/clock_imx8mm.

The down-side is that a bunch of SoC's might need to be updated to
support more and more clocks, so we could potentially use
!CONFIG_IS_ENABLED to use the existing functions as a fall-back.

adam
>
> Angus