[RFC] serial: mxc: get the clock frequency from the used clock for the device
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)) { + struct clk clk; + if(!clk_get_by_name(dev, "ipg", &clk)) + rate = clk_get_rate(&clk); + } + + /* as fallback we try to get the clk rate that way */ + if (rate == 0) + rate = imx_get_uartclk();
- _mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte); + _mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
return 0; }
Hi Heiko,
On 2022-03-17 05:41, 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)) {
struct clk clk;if(!clk_get_by_name(dev, "ipg", &clk))rate = clk_get_rate(&clk);
Is the "ipg" clock the correct name for all of the imx DM boards ?
- }
- /* 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 ?
With DM clocks enabled I don't even think it makes sense to call those older functions.
Angus
- _mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
_mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
return 0;
}
Hi Angus,
Am Do., 17. März 2022 um 14:19 Uhr schrieb Angus Ainslie angus@akkea.ca:
Hi Heiko,
On 2022-03-17 05:41, 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)) {struct clk clk;if(!clk_get_by_name(dev, "ipg", &clk))rate = clk_get_rate(&clk);Is the "ipg" clock the correct name for all of the imx DM boards ?
I checked the dtsi files for all the boards that use the compatible strings from the serial_mxc and see that there are always 2 clocks: 'ipg' and 'per'.
The imx8 dtsi files describe ipg and per always the same clock. The imx7 dtsi files descibe ipg and per always the same clock. The imx6 dtsi files describe ipg and per are 2 different clocks The imx51 dtsi files describeipg and per are 2 different clocks
So I'm not sure if the ipg clock is the right one for the boards that has different clock for ipg and per.
}/* 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.
With DM clocks enabled I don't even think it makes sense to call those older functions.
You mean when DM clocks are available the "new" method should be used and no fallback to the old mechanism?
Angus
_mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
_mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte); return 0;}
On 2022-03-18 12:06, Heiko Thiery wrote:
Hi Angus,
Am Do., 17. März 2022 um 14:19 Uhr schrieb Angus Ainslie angus@akkea.ca:
Hi Heiko,
On 2022-03-17 05:41, 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)) {struct clk clk;if(!clk_get_by_name(dev, "ipg", &clk))rate = clk_get_rate(&clk);Is the "ipg" clock the correct name for all of the imx DM boards ?
I checked the dtsi files for all the boards that use the compatible strings from the serial_mxc and see that there are always 2 clocks: 'ipg' and 'per'.
The imx8 dtsi files describe ipg and per always the same clock. The imx7 dtsi files descibe ipg and per always the same clock. The imx6 dtsi files describe ipg and per are 2 different clocks The imx51 dtsi files describeipg and per are 2 different clocks
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.
Should the clock be looked up by id instead of by name and then have a different code path for each imx board type ?
}/* 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.
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.
With DM clocks enabled I don't even think it makes sense to call those older functions.
You mean when DM clocks are available the "new" method should be used and no fallback to the old mechanism?
For older boards it makes sense to fallback to the single clock. On newer boards if it returned an error instead it would have been easier for you to figure out where the serial console failed.
Angus
Angus
_mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
_mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte); return 0;}
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?
}/* 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?
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?
With DM clocks enabled I don't even think it makes sense to call those older functions.
You mean when DM clocks are available the "new" method should be used and no fallback to the old mechanism?
For older boards it makes sense to fallback to the single clock. On newer boards if it returned an error instead it would have been easier for you to figure out where the serial console failed.
-- Heiko
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
On Tue, Mar 22, 2022 at 7:48 AM Angus Ainslie angus@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
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]
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
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...
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?
mx6ull at least does not have CONFIG_SPL_CLK enabled.
struct clk clk;if(!clk_get_by_name(dev, "ipg", &clk))rate = clk_get_rate(&clk);- }
- /* as fallback we try to get the clk rate that way */
- if (rate == 0)
!rate || IS_ERR_VALUE(rate)
rate = imx_get_uartclk();
- _mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
_mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte);
return 0; }
--Sean
struct clk clk;if(!clk_get_by_name(dev, "ipg", &clk))rate = clk_get_rate(&clk);- }
- /* as fallback we try to get the clk rate that way */
- if (rate == 0)
!rate || IS_ERR_VALUE(rate)
This looked so weird I had to actually look at clk_get_rate() in u-boot.
/** * clk_get_rate() - Get current clock rate. * @clk: A clock struct that was previously successfully requested by * clk_request/get_by_*(). * * Return: clock rate in Hz on success, 0 for invalid clock, or -ve error code * for other errors. */ ulong clk_get_rate(struct clk *clk);
How can an ulong return a negative error value? What if the clock speed happens to be the same as -errno?
-michael
On 3/17/22 10:47 AM, Michael Walle wrote:
+ struct clk clk; + if(!clk_get_by_name(dev, "ipg", &clk)) + rate = clk_get_rate(&clk); + }
+ /* as fallback we try to get the clk rate that way */ + if (rate == 0)
!rate || IS_ERR_VALUE(rate)
This looked so weird I had to actually look at clk_get_rate() in u-boot.
/** * clk_get_rate() - Get current clock rate. * @clk: A clock struct that was previously successfully requested by * clk_request/get_by_*(). * * Return: clock rate in Hz on success, 0 for invalid clock, or -ve error code * for other errors. */ ulong clk_get_rate(struct clk *clk);
How can an ulong return a negative error value?
It can't. Which is why we need IS_ERR_VALUE and not < 0.
What if the clock speed happens to be the same as -errno?
Due to physical constraints, this isn't an issue on 64 bit systems. And you could run into trouble if you have a 32-bit system with a 4GHz processor.
I don't think it's the best interface. In e.g. Linux clk_get_rate returns 0 on failure, which I think is reasonable. If you are interested, please send a patch series :)
--Sean
Hi Sean,
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?
mx6ull at least does not have CONFIG_SPL_CLK enabled.
The problem with serial is that not all boards support DM clock in SPL. I'm wondering if this patch has passed the CI tests for all boards.
struct clk clk;if(!clk_get_by_name(dev, "ipg", &clk))rate = clk_get_rate(&clk);- }
- /* as fallback we try to get the clk rate that way */
- if (rate == 0)
!rate || IS_ERR_VALUE(rate)
rate = imx_get_uartclk();
- _mxc_serial_setbrg(plat->reg, clk, baudrate,
plat->use_dte);
- _mxc_serial_setbrg(plat->reg, rate, baudrate,
plat->use_dte); return 0; }
--Sean
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
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?
mx6ull at least does not have CONFIG_SPL_CLK enabled.
I expect that in that case it will fallback to the old behavior ... not?
struct clk clk;if(!clk_get_by_name(dev, "ipg", &clk))rate = clk_get_rate(&clk);}/* as fallback we try to get the clk rate that way */if (rate == 0)!rate || IS_ERR_VALUE(rate)
rate = imx_get_uartclk();
_mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
_mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte); return 0;}
--Sean
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.
}/* as fallback we try to get the clk rate that way */if (rate == 0)!rate || IS_ERR_VALUE(rate)
rate = imx_get_uartclk();
_mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
_mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte); return 0;}
--Sean
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()?
}/* as fallback we try to get the clk rate that way */if (rate == 0)!rate || IS_ERR_VALUE(rate)
rate = imx_get_uartclk();
_mxc_serial_setbrg(plat->reg, clk, baudrate, plat->use_dte);
_mxc_serial_setbrg(plat->reg, rate, baudrate, plat->use_dte); return 0;}
--Sean
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
participants (6)
-
Adam Ford -
Angus Ainslie -
Heiko Thiery -
Lukasz Majewski -
Michael Walle -
Sean Anderson