| Message ID | 20210514014011.2832707-13-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2,01/16] sandbox: net: Ensure host name is always a valid string | expand |
On Thu, May 13, 2021 at 07:39:31PM -0600, Simon Glass wrote: > When the default clocks cannot be set, the clock is silently probed and > the error is ignored. This is incorrect, since having the clocks at the > correct speed may be important for operation of the system. > > Fix it by checking the return code. > > Signed-off-by: Simon Glass <sjg@chromium.org> Applied to u-boot/master, thanks!
On Thu, 2021-05-13 at 19:39 -0600, Simon Glass wrote: > When the default clocks cannot be set, the clock is silently probed and > the error is ignored. This is incorrect, since having the clocks at the > correct speed may be important for operation of the system. > > Fix it by checking the return code. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > (no changes since v1) > > drivers/clk/clk-uclass.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > index 4ab3c402ed8..2a2e1cfbd61 100644 > --- a/drivers/clk/clk-uclass.c > +++ b/drivers/clk/clk-uclass.c > @@ -796,13 +796,17 @@ void devm_clk_put(struct udevice *dev, struct clk > *clk) > > int clk_uclass_post_probe(struct udevice *dev) > { > + int ret; > + > /* > * when a clock provider is probed. Call clk_set_defaults() > * also after the device is probed. This takes care of cases > * where the DT is used to setup default parents and rates > * using assigned-clocks > */ > - clk_set_defaults(dev, 1); > + ret = clk_set_defaults(dev, 1); > + if (ret) > + return log_ret(ret); > > return 0; > } Note that this patch broke booting my imx8mn based board on U-Boot v2021.10-rc2. The failure is due to the clock-controller@30380000 configuration in the imx8mn.dtsi file. I had to remove the following clocks from the device tree to get my device to boot again (all from the assigned-clocks of clock-controller@30380000): <&clk IMX8MN_CLK_A53_CORE>, <&clk IMX8MN_CLK_NOC>, <&clk IMX8MN_CLK_AUDIO_AHB>, <&clk IMX8MN_CLK_IPG_AUDIO_ROOT>, <&clk IMX8MN_SYS_PLL3>, <&clk IMX8MN_AUDIO_PLL1>, <&clk IMX8MN_AUDIO_PLL2>; I looked into the clk-imx8mn.c code and I see that we indeed miss clocks there. Unfortunately I could not port code from the Linux kernel: we are missing the imx_clk_hw_mux2 function for the IMX8MN_CLK_A53_CORE clock. I did not look into the other clocks. -- Harm
Hi Harm, On Wed, 18 Aug 2021 at 08:09, Harm Berntsen <harm.berntsen@nedap.com> wrote: > > On Thu, 2021-05-13 at 19:39 -0600, Simon Glass wrote: > > When the default clocks cannot be set, the clock is silently probed and > > the error is ignored. This is incorrect, since having the clocks at the > > correct speed may be important for operation of the system. > > > > Fix it by checking the return code. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > (no changes since v1) > > > > drivers/clk/clk-uclass.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > > index 4ab3c402ed8..2a2e1cfbd61 100644 > > --- a/drivers/clk/clk-uclass.c > > +++ b/drivers/clk/clk-uclass.c > > @@ -796,13 +796,17 @@ void devm_clk_put(struct udevice *dev, struct clk > > *clk) > > > > int clk_uclass_post_probe(struct udevice *dev) > > { > > + int ret; > > + > > /* > > * when a clock provider is probed. Call clk_set_defaults() > > * also after the device is probed. This takes care of cases > > * where the DT is used to setup default parents and rates > > * using assigned-clocks > > */ > > - clk_set_defaults(dev, 1); > > + ret = clk_set_defaults(dev, 1); > > + if (ret) > > + return log_ret(ret); > > > > return 0; > > } > > Note that this patch broke booting my imx8mn based board on U-Boot > v2021.10-rc2. The failure is due to the clock-controller@30380000 > configuration in the imx8mn.dtsi file. I had to remove the following > clocks from the device tree to get my device to boot again (all from > the assigned-clocks of clock-controller@30380000): > > <&clk IMX8MN_CLK_A53_CORE>, > <&clk IMX8MN_CLK_NOC>, > <&clk IMX8MN_CLK_AUDIO_AHB>, > <&clk IMX8MN_CLK_IPG_AUDIO_ROOT>, > <&clk IMX8MN_SYS_PLL3>, > <&clk IMX8MN_AUDIO_PLL1>, > <&clk IMX8MN_AUDIO_PLL2>; > > I looked into the clk-imx8mn.c code and I see that we indeed miss > clocks there. Unfortunately I could not port code from the Linux > kernel: we are missing the imx_clk_hw_mux2 function for the > IMX8MN_CLK_A53_CORE clock. I did not look into the other clocks. Perhaps the iMX maintainer could help with this? It does sound like a bug. Regards, SImon > > > -- Harm
Hi Stefano and Peng, There is an issue that prevents the imx8mn to boot in 2021.10-rc2. See the conversation below. Could you help with this? -- Harm -------- Forwarded Message -------- From: Simon Glass <sjg@chromium.org> To: Harm Berntsen <harm.berntsen@nedap.com> Cc: u-boot@lists.denx.de <u-boot@lists.denx.de>, trini@konsulko.com <trini@konsulko.com> Subject: Re: [PATCH v2 15/16] clk: Detect failure to set defaults Date: Fri, 20 Aug 2021 12:18:07 -0600 > Hi Harm, > > On Wed, 18 Aug 2021 at 08:09, Harm Berntsen <harm.berntsen@nedap.com> > wrote: > > > > On Thu, 2021-05-13 at 19:39 -0600, Simon Glass wrote: > > > When the default clocks cannot be set, the clock is silently > > > probed and > > > the error is ignored. This is incorrect, since having the clocks > > > at the > > > correct speed may be important for operation of the system. > > > > > > Fix it by checking the return code. > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > --- > > > > > > (no changes since v1) > > > > > > drivers/clk/clk-uclass.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c > > > index 4ab3c402ed8..2a2e1cfbd61 100644 > > > --- a/drivers/clk/clk-uclass.c > > > +++ b/drivers/clk/clk-uclass.c > > > @@ -796,13 +796,17 @@ void devm_clk_put(struct udevice *dev, > > > struct clk > > > *clk) > > > > > > int clk_uclass_post_probe(struct udevice *dev) > > > { > > > + int ret; > > > + > > > /* > > > * when a clock provider is probed. Call > > > clk_set_defaults() > > > * also after the device is probed. This takes care of > > > cases > > > * where the DT is used to setup default parents and > > > rates > > > * using assigned-clocks > > > */ > > > - clk_set_defaults(dev, 1); > > > + ret = clk_set_defaults(dev, 1); > > > + if (ret) > > > + return log_ret(ret); > > > > > > return 0; > > > } > > > > Note that this patch broke booting my imx8mn based board on U-Boot > > v2021.10-rc2. The failure is due to the clock-controller@30380000 > > configuration in the imx8mn.dtsi file. I had to remove the > > following > > clocks from the device tree to get my device to boot again (all > > from > > the assigned-clocks of clock-controller@30380000): > > > > <&clk IMX8MN_CLK_A53_CORE>, > > <&clk IMX8MN_CLK_NOC>, > > <&clk IMX8MN_CLK_AUDIO_AHB>, > > <&clk IMX8MN_CLK_IPG_AUDIO_ROOT>, > > <&clk IMX8MN_SYS_PLL3>, > > <&clk IMX8MN_AUDIO_PLL1>, > > <&clk IMX8MN_AUDIO_PLL2>; > > > > I looked into the clk-imx8mn.c code and I see that we indeed miss > > clocks there. Unfortunately I could not port code from the Linux > > kernel: we are missing the imx_clk_hw_mux2 function for the > > IMX8MN_CLK_A53_CORE clock. I did not look into the other clocks. > > > Perhaps the iMX maintainer could help with this? It does sound like a > bug. > > Regards, > SImon > > > > > > > -- Harm
On 20/08/2021 20.18, Simon Glass wrote: > Hi Harm, > > On Wed, 18 Aug 2021 at 08:09, Harm Berntsen <harm.berntsen@nedap.com> wrote: >> >> On Thu, 2021-05-13 at 19:39 -0600, Simon Glass wrote: >>> int clk_uclass_post_probe(struct udevice *dev) >>> { >>> + int ret; >>> + >>> /* >>> * when a clock provider is probed. Call clk_set_defaults() >>> * also after the device is probed. This takes care of cases >>> * where the DT is used to setup default parents and rates >>> * using assigned-clocks >>> */ >>> - clk_set_defaults(dev, 1); >>> + ret = clk_set_defaults(dev, 1); >>> + if (ret) >>> + return log_ret(ret); >>> >>> return 0; >>> } >> >> Note that this patch broke booting my imx8mn based board on U-Boot >> v2021.10-rc2. I just ran into the same issue with v2021.10 being broken for imx8mp_evk, and git bisect pointing at this commit, symptoms being U-Boot 2021.10 (Oct 20 2021 - 08:45:51 +0200) CPU: Freescale i.MX8MP[8] rev1.1 at 1200 MHz ... MMC: mmc@30b50000 - probe failed: -2 mmc@30b60000 - probe failed: -2 Reverting 92f1e9a4b on top of v2021.10 yields U-Boot 2021.10-00001-gac2520a138 (Oct 20 2021 - 09:05:48 +0200) CPU: Freescale i.MX8MP[8] rev1.1 at 1200 MHz ... MMC: FSL_SDHC: 1, FSL_SDHC: 2 cc += imx maintainers, this should not still be broken 2 months (and a release) after it was reported. Rasmus
Hi, On Wed, 20 Oct 2021 at 01:17, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > > On 20/08/2021 20.18, Simon Glass wrote: > > Hi Harm, > > > > On Wed, 18 Aug 2021 at 08:09, Harm Berntsen <harm.berntsen@nedap.com> wrote: > >> > >> On Thu, 2021-05-13 at 19:39 -0600, Simon Glass wrote: > >>> int clk_uclass_post_probe(struct udevice *dev) > >>> { > >>> + int ret; > >>> + > >>> /* > >>> * when a clock provider is probed. Call clk_set_defaults() > >>> * also after the device is probed. This takes care of cases > >>> * where the DT is used to setup default parents and rates > >>> * using assigned-clocks > >>> */ > >>> - clk_set_defaults(dev, 1); > >>> + ret = clk_set_defaults(dev, 1); > >>> + if (ret) > >>> + return log_ret(ret); > >>> > >>> return 0; > >>> } > >> > >> Note that this patch broke booting my imx8mn based board on U-Boot > >> v2021.10-rc2. > > I just ran into the same issue with v2021.10 being broken for > imx8mp_evk, and git bisect pointing at this commit, symptoms being > > U-Boot 2021.10 (Oct 20 2021 - 08:45:51 +0200) > > CPU: Freescale i.MX8MP[8] rev1.1 at 1200 MHz > ... > MMC: mmc@30b50000 - probe failed: -2 > mmc@30b60000 - probe failed: -2 > > Reverting 92f1e9a4b on top of v2021.10 yields > > U-Boot 2021.10-00001-gac2520a138 (Oct 20 2021 - 09:05:48 +0200) > > CPU: Freescale i.MX8MP[8] rev1.1 at 1200 MHz > ... > MMC: FSL_SDHC: 1, FSL_SDHC: 2 > > cc += imx maintainers, this should not still be broken 2 months (and a > release) after it was reported. I see a patch to explicitly make this optional, using the devicetree. Regards, Simon
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index 4ab3c402ed8..2a2e1cfbd61 100644 --- a/drivers/clk/clk-uclass.c +++ b/drivers/clk/clk-uclass.c @@ -796,13 +796,17 @@ void devm_clk_put(struct udevice *dev, struct clk *clk) int clk_uclass_post_probe(struct udevice *dev) { + int ret; + /* * when a clock provider is probed. Call clk_set_defaults() * also after the device is probed. This takes care of cases * where the DT is used to setup default parents and rates * using assigned-clocks */ - clk_set_defaults(dev, 1); + ret = clk_set_defaults(dev, 1); + if (ret) + return log_ret(ret); return 0; }
When the default clocks cannot be set, the clock is silently probed and the error is ignored. This is incorrect, since having the clocks at the correct speed may be important for operation of the system. Fix it by checking the return code. Signed-off-by: Simon Glass <sjg@chromium.org> --- (no changes since v1) drivers/clk/clk-uclass.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)