On Tue, Mar 22, 2022 at 7:48 AM Angus Ainslie <an...@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
