On Thu, Mar 24, 2022 at 4:58 AM Heiko Thiery heiko.thiery@gmail.com wrote:
Hi Adam,
[SNIP]
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.
With your comment I have made the following, does this fit with your suggestion?
diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c index e4970a169b..7f9f7e8383 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,16 @@ __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 defined(CONFIG_CLK)
rate = clk_get_rate(&plat->per_clk);+#else
/* as fallback we try to get the clk rate that way */rate = imx_get_uartclk();+#endif
_mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
_mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte); return 0;} @@ -277,6 +285,11 @@ static int mxc_serial_probe(struct udevice *dev) { struct mxc_serial_plat *plat = dev_get_plat(dev);
+#if defined(CONFIG_CLK)
clk_enable(&plat->ipg_clk);clk_enable(&plat->per_clk);+#endif
_mxc_serial_init(plat->reg, plat->use_dte); return 0;@@ -339,6 +352,10 @@ static int mxc_serial_of_to_plat(struct udevice *dev)
plat->use_dte = fdtdec_get_bool(gd->fdt_blob, dev_of_offset(dev), "fsl,dte-mode");+#if defined(CONFIG_CLK)
clk_get_by_name(dev, "ipg", &plat->ipg_clk);clk_get_by_name(dev, "per", &plat->per_clk);+#endif return 0; }
diff --git a/include/dm/platform_data/serial_mxc.h b/include/dm/platform_data/serial_mxc.h index cc59eeb1dd..330476f816 100644 --- a/include/dm/platform_data/serial_mxc.h +++ b/include/dm/platform_data/serial_mxc.h @@ -10,6 +10,10 @@ struct mxc_serial_plat { struct mxc_uart *reg; /* address of registers in physical memory */ bool use_dte; +#if defined(CONFIG_CLK)
struct clk ipg_clk;struct clk per_clk;+#endif };
#endif
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.
For the imx8mq I added this and asked Angus to integrate it into his patchset. [1]
That's basically what I was thinking. It probably wouldn't hurt to add some error handling. What's is not clear to me is the impact of every other IMX platform out there. I am guessing the clk_get_by_name calls will fail if the UART clocks haven't been configured or added to the SoC's respective clock driver. We also might want to check for the presence of plat->ipg_clk and plat->per_clk before requesting their clock rate. If either the UART clocks don't exist or ipg or per don't exist, you'll likely need to fallback on the older functions as well to avoid crashing.
adam
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
-- Heiko
[1] https://lore.kernel.org/u-boot/CAEyMn7bC-p6o6VCu7YWQyXMm+51EcJ5OVXv_625cjDia...