| Message ID | 20240814121027.3477977-1-hs@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Fabio Estevam |
| Headers | show |
| Series | [v3] driver: pwm: pwm-imx: fix probing on imx6 | expand |
On Wed, Aug 14, 2024 at 02:10:27PM +0200, Heiko Schocher wrote: > U-Boot 2024.07 drops on aristainetos2 board the following > warning: > > Failed to enable per_clk > > and bootlogo is not seen on LVDS display. > > This patch uses old behaviour for systems without clock framework > if CONFIG_CLK is not enabled. > > Fixes: bfc778cb93a3 ("driver: pwm: pwm-imx: get and enable per/ipg clock using dm") > Fixes: 4eea76574062 ("pwm: imx: Remove unused references to CONFIG_IMX6_PWM_PER_CLK") > > Signed-off-by: Heiko Schocher <hs@denx.de> I really do not like this and would like to see the clock migration also enabled / completed on these platforms. Then we can also go and rip out the non-DM_PWM code here.
Hello Tom, On 14.08.24 16:17, Tom Rini wrote: > On Wed, Aug 14, 2024 at 02:10:27PM +0200, Heiko Schocher wrote: > >> U-Boot 2024.07 drops on aristainetos2 board the following >> warning: >> >> Failed to enable per_clk >> >> and bootlogo is not seen on LVDS display. >> >> This patch uses old behaviour for systems without clock framework >> if CONFIG_CLK is not enabled. >> >> Fixes: bfc778cb93a3 ("driver: pwm: pwm-imx: get and enable per/ipg clock using dm") >> Fixes: 4eea76574062 ("pwm: imx: Remove unused references to CONFIG_IMX6_PWM_PER_CLK") >> >> Signed-off-by: Heiko Schocher <hs@denx.de> > > I really do not like this and would like to see the clock migration also > enabled / completed on these platforms. Then we can also go and rip out > the non-DM_PWM code here. Yes, valid argument, I also thought about this. Anyone working on this part? Until this work is not done, this patch is in my eyes a bugfix... Just enabled CONFIG_CLK and CONFIG_CLK_IMX6Q on the aristainetos2 board and get [1] (as I also have there CONFIG_VIDEO_IPUV3=y) Uff... yes, there are such functions in drivers/video/imx/ipu_common.c :-( and even worser "struct clk" is defined in drivers/video/imx/ipu.h#L22 and include/clk.h#L63 completly different ... so this is not a fast job... I fear... bye, Heiko [1] | arm-poky-linux-gnueabi-ld.bfd: drivers/video/imx/ipu_common.o: in function `clk_enable': | /usr/src/debug/u-boot/aristainetos2_2024.07/drivers/video/imx/ipu_common.c:93: multiple definition of `clk_enable'; drivers/clk/clk-uclass.o:/usr/src/debug/u-boot/aristainetos2_2024.07/drivers/clk/clk-uclass.c:611: first defined here | arm-poky-linux-gnueabi-ld.bfd: drivers/video/imx/ipu_common.o: in function `clk_disable': | /usr/src/debug/u-boot/aristainetos2_2024.07/drivers/video/imx/ipu_common.c:102: multiple definition of `clk_disable'; drivers/clk/clk-uclass.o:/usr/src/debug/u-boot/aristainetos2_2024.07/drivers/clk/clk-uclass.c:672: first defined here | arm-poky-linux-gnueabi-ld.bfd: drivers/video/imx/ipu_common.o: in function `clk_get_rate': | /usr/src/debug/u-boot/aristainetos2_2024.07/drivers/video/imx/ipu_common.c:120: multiple definition of `clk_get_rate'; drivers/clk/clk-uclass.o:/usr/src/debug/u-boot/aristainetos2_2024.07/include/clk.h:645: first defined here | arm-poky-linux-gnueabi-ld.bfd: drivers/video/imx/ipu_common.o: in function `clk_get_parent': | /usr/src/debug/u-boot/aristainetos2_2024.07/drivers/video/imx/ipu_common.c:128: multiple definition of `clk_get_parent'; drivers/clk/clk-uclass.o:/usr/src/debug/u-boot/aristainetos2_2024.07/drivers/clk/clk-uclass.c:478: first defined here | arm-poky-linux-gnueabi-ld.bfd: drivers/video/imx/ipu_common.o: in function `clk_set_rate': | /usr/src/debug/u-boot/aristainetos2_2024.07/drivers/video/imx/ipu_common.c:135: multiple definition of `clk_set_rate'; drivers/clk/clk-uclass.o:/usr/src/debug/u-boot/aristainetos2_2024.07/drivers/clk/clk-uclass.c:565: first defined here | arm-poky-linux-gnueabi-ld.bfd: drivers/video/imx/ipu_common.o: in function `clk_round_rate': | /usr/src/debug/u-boot/aristainetos2_2024.07/drivers/video/imx/ipu_common.c:147: multiple definition of `clk_round_rate'; drivers/clk/clk-uclass.o:/usr/src/debug/u-boot/aristainetos2_2024.07/include/clk.h:645: first defined here | arm-poky-linux-gnueabi-ld.bfd: drivers/video/imx/ipu_common.o: in function `clk_set_parent': | /usr/src/debug/u-boot/aristainetos2_2024.07/drivers/video/imx/ipu_common.c:156: multiple definition of `clk_set_parent'; drivers/clk/clk-uclass.o:/usr/src/debug/u-boot/aristainetos2_2024.07/drivers/clk/clk-uclass.c:586: first defined here
On Thu, Aug 15, 2024 at 01:17:02PM +0200, Heiko Schocher wrote: > Hello Tom, > > On 14.08.24 16:17, Tom Rini wrote: > > On Wed, Aug 14, 2024 at 02:10:27PM +0200, Heiko Schocher wrote: > > > > > U-Boot 2024.07 drops on aristainetos2 board the following > > > warning: > > > > > > Failed to enable per_clk > > > > > > and bootlogo is not seen on LVDS display. > > > > > > This patch uses old behaviour for systems without clock framework > > > if CONFIG_CLK is not enabled. > > > > > > Fixes: bfc778cb93a3 ("driver: pwm: pwm-imx: get and enable per/ipg clock using dm") > > > Fixes: 4eea76574062 ("pwm: imx: Remove unused references to CONFIG_IMX6_PWM_PER_CLK") > > > > > > Signed-off-by: Heiko Schocher <hs@denx.de> > > > > I really do not like this and would like to see the clock migration also > > enabled / completed on these platforms. Then we can also go and rip out > > the non-DM_PWM code here. > > Yes, valid argument, I also thought about this. Anyone working on > this part? > > Until this work is not done, this patch is in my eyes a bugfix... > > Just enabled CONFIG_CLK and CONFIG_CLK_IMX6Q on the aristainetos2 > board and get [1] (as I also have there CONFIG_VIDEO_IPUV3=y) > > Uff... yes, there are such functions in drivers/video/imx/ipu_common.c > > :-( > > and even worser "struct clk" is defined in > > drivers/video/imx/ipu.h#L22 > and > include/clk.h#L63 > > completly different ... so this is not a fast job... I fear... Yeah, that looks like lots of legacy code that needs to be updated. Is someone going to update it? If not, perhaps we should be removing it, and boards, instead of adding work-arounds for lack of migration.
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c index 320ea7c423..d285730694 100644 --- a/drivers/pwm/pwm-imx.c +++ b/drivers/pwm/pwm-imx.c @@ -14,6 +14,10 @@ #include <asm/io.h> #include <clk.h> +#if !defined(CFG_IMX6_PWM_PER_CLK) +#define CFG_IMX6_PWM_PER_CLK 66000000 +#endif + int pwm_config_internal(struct pwm_regs *pwm, unsigned long period_cycles, unsigned long duty_cycles, unsigned long prescale) { @@ -159,7 +163,11 @@ int pwm_dm_imx_get_parms(struct imx_pwm_priv *priv, int period_ns, { unsigned long long c; - c = clk_get_rate(&priv->per_clk); + if (CONFIG_IS_ENABLED(CLK)) + c = clk_get_rate(&priv->per_clk); + else + c = CFG_IMX6_PWM_PER_CLK; + c = c * period_ns; do_div(c, 1000000000); *period_c = c; @@ -226,43 +234,45 @@ static int imx_pwm_set_enable(struct udevice *dev, uint channel, bool enable) static int imx_pwm_of_to_plat(struct udevice *dev) { - int ret; + int __maybe_unused ret; struct imx_pwm_priv *priv = dev_get_priv(dev); priv->regs = dev_read_addr_ptr(dev); - ret = clk_get_by_name(dev, "per", &priv->per_clk); - if (ret) { - printf("Failed to get per_clk\n"); - return ret; - } - - ret = clk_get_by_name(dev, "ipg", &priv->ipg_clk); - if (ret) { - printf("Failed to get ipg_clk\n"); - return ret; + if (CONFIG_IS_ENABLED(CLK)) { + ret = clk_get_by_name(dev, "per", &priv->per_clk); + if (ret) { + printf("Failed to get per_clk\n"); + return ret; + } + + ret = clk_get_by_name(dev, "ipg", &priv->ipg_clk); + if (ret) { + printf("Failed to get ipg_clk\n"); + return ret; + } } - return 0; } static int imx_pwm_probe(struct udevice *dev) { - int ret; + int __maybe_unused ret; struct imx_pwm_priv *priv = dev_get_priv(dev); - ret = clk_enable(&priv->per_clk); - if (ret) { - printf("Failed to enable per_clk\n"); - return ret; + if (CONFIG_IS_ENABLED(CLK)) { + ret = clk_enable(&priv->per_clk); + if (ret) { + printf("Failed to enable per_clk\n"); + return ret; + } + + ret = clk_enable(&priv->ipg_clk); + if (ret) { + printf("Failed to enable ipg_clk\n"); + return ret; + } } - - ret = clk_enable(&priv->ipg_clk); - if (ret) { - printf("Failed to enable ipg_clk\n"); - return ret; - } - return 0; }
U-Boot 2024.07 drops on aristainetos2 board the following warning: Failed to enable per_clk and bootlogo is not seen on LVDS display. This patch uses old behaviour for systems without clock framework if CONFIG_CLK is not enabled. Fixes: bfc778cb93a3 ("driver: pwm: pwm-imx: get and enable per/ipg clock using dm") Fixes: 4eea76574062 ("pwm: imx: Remove unused references to CONFIG_IMX6_PWM_PER_CLK") Signed-off-by: Heiko Schocher <hs@denx.de> --- Changes in v3: add a default value for CFG_IMX6_PWM_PER_CLK as Fabio suggested add Fixes line for commit 4eea76574062 azure build: https://dev.azure.com/hs0298/hs/_build/results?buildId=123&view=results Changes in v2: use CONFIG_IS_ENABLED instead of IS_ENABLED as Anatolij suggested. drivers/pwm/pwm-imx.c | 60 +++++++++++++++++++++++++------------------ 1 file changed, 35 insertions(+), 25 deletions(-)