| Message ID | 20250516134945.14692-1-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Fri, May 16, 2025 at 03:49:27PM +0200, Dario Binacchi wrote: > Commit 9c1e388af87c ("clk: imx: add support for i.MX8MM anatop clock > driver") breaks boot on i.MX8M{P,N} platforms. ... > In the imx8mp.dtsi device tree, the anatop compatible string is: > > compatible = "fsl,imx8mp-anatop", "fsl,imx8mm-anatop"; > > So, in configurations like arm64 defconfig, where CONFIG_CLK_IMX8MP and > CONFIG_CLK_IMX8MM as well as CONFIG_CLK_IMX8MN are enabled, the driver > for the i.MX8MM anatop is incorrectly loaded. > > The patch fixes the regression by ensuring that the i.MX8MM anatop > driver only probes on i.MX8MM platforms. This *works* so Tested-by: Mark Brown <broonie@kernel.org> and we probably need it for existing built DTs, but shouldn't there also be a patch to the DT source since the two blocks clearly don't seem to be compatible. To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
> Subject: [linux-next, 1/1] clk: imx: imx8mm-anatop: probe only on > i.MX8MM platforms DT Maintainer Krzysztof NACKed [1] because of ABI break. Are we continuing breaking the ABI? [1] https://lore.kernel.org/imx/6a28f9bb-05fa-45ff-8c0b-790c0caf3252@kernel.org/T/#u Regards, Peng. > > Commit 9c1e388af87c ("clk: imx: add support for i.MX8MM anatop > clock > driver") breaks boot on i.MX8M{P,N} platforms. > > Here's the log for a board based on the i.MX8MP platform: > > [ 1.439320] i.MX clk 1: register failed with -2 > [ 1.441014] i.MX clk 2: register failed with -2 > [ 1.445610] imx8mm-anatop 30360000.clock-controller: NXP > i.MX8MM anatop clock driver probed > [ 1.455068] Unable to handle kernel paging request at virtual address > fffffffffffffffe > > ... > > [ 1.634650] Call trace: > [ 1.637102] __clk_get_hw+0x4/0x18 (P) > [ 1.640862] imx8mp_clocks_probe+0xdc/0x2f50 > [ 1.645152] platform_probe+0x68/0xc4 > [ 1.648827] really_probe+0xbc/0x298 > [ 1.652413] __driver_probe_device+0x78/0x12c > > In the imx8mp.dtsi device tree, the anatop compatible string is: > > compatible = "fsl,imx8mp-anatop", "fsl,imx8mm-anatop"; > > So, in configurations like arm64 defconfig, where > CONFIG_CLK_IMX8MP and CONFIG_CLK_IMX8MM as well as > CONFIG_CLK_IMX8MN are enabled, the driver for the i.MX8MM > anatop is incorrectly loaded. > > The patch fixes the regression by ensuring that the i.MX8MM anatop > driver only probes on i.MX8MM platforms. > > Fixes: 9c1e388af87c ("clk: imx: add support for i.MX8MM anatop clock > driver") > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> > > --- > > drivers/clk/imx/clk-imx8mm-anatop.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/clk/imx/clk-imx8mm-anatop.c b/drivers/clk/imx/clk- > imx8mm-anatop.c > index 4ac870df6370..90ff11a93fe5 100644 > --- a/drivers/clk/imx/clk-imx8mm-anatop.c > +++ b/drivers/clk/imx/clk-imx8mm-anatop.c > @@ -37,6 +37,19 @@ static const char * const clkout_sels[] = > {"audio_pll1_out", "audio_pll2_out", " > static struct clk_hw_onecell_data *clk_hw_data; static struct clk_hw > **hws; > > +static int is_really_imx8mm(struct device_node *np) { > + const char *compat; > + struct property *p; > + > + of_property_for_each_string(np, "compatible", p, compat) { > + if (strcmp(compat, "fsl,imx8mm-anatop")) > + return -EFAULT; > + } > + > + return 0; > +} > + > static int imx8mm_anatop_clocks_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -44,6 +57,10 @@ static int imx8mm_anatop_clocks_probe(struct > platform_device *pdev) > void __iomem *base; > int ret; > > + ret = is_really_imx8mm(np); > + if (ret) > + return ret; > + > base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(base)) { > dev_err(dev, "failed to get base address\n"); > -- > 2.43.0 > > base-commit: 8566fc3b96539e3235909d6bdda198e1282beaed > branch: fix-imx8mm-probing To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
On Mon, May 19, 2025 at 12:58:48PM +0000, Peng Fan wrote: > > Subject: [linux-next, 1/1] clk: imx: imx8mm-anatop: probe only on > > i.MX8MM platforms > DT Maintainer Krzysztof NACKed [1] because of ABI break. > Are we continuing breaking the ABI? > [1] https://lore.kernel.org/imx/6a28f9bb-05fa-45ff-8c0b-790c0caf3252@kernel.org/T/#u As I've reported a couple of times now (including in the thread you link above) i.MX8MP platforms haven't booted -next for several weeks: https://lore.kernel.org/imx/6a28f9bb-05fa-45ff-8c0b-790c0caf3252@kernel.org/T/#m8f7886b2f57fe80e2d87e2242bc88389dedf2ae4a These patches need to be either fixed or dropped. To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
On 19/05/2025 14:58, Peng Fan wrote: >> Subject: [linux-next, 1/1] clk: imx: imx8mm-anatop: probe only on >> i.MX8MM platforms > > DT Maintainer Krzysztof NACKed [1] because of ABI break. > > Are we continuing breaking the ABI? > > [1] https://lore.kernel.org/imx/6a28f9bb-05fa-45ff-8c0b-790c0caf3252@kernel.org/T/#u > > Regards, > Peng. > >> >> Commit 9c1e388af87c ("clk: imx: add support for i.MX8MM anatop >> clock >> driver") breaks boot on i.MX8M{P,N} platforms. >> >> Here's the log for a board based on the i.MX8MP platform: >> >> [ 1.439320] i.MX clk 1: register failed with -2 >> [ 1.441014] i.MX clk 2: register failed with -2 >> [ 1.445610] imx8mm-anatop 30360000.clock-controller: NXP >> i.MX8MM anatop clock driver probed >> [ 1.455068] Unable to handle kernel paging request at virtual address >> fffffffffffffffe >> >> ... >> >> [ 1.634650] Call trace: >> [ 1.637102] __clk_get_hw+0x4/0x18 (P) >> [ 1.640862] imx8mp_clocks_probe+0xdc/0x2f50 >> [ 1.645152] platform_probe+0x68/0xc4 >> [ 1.648827] really_probe+0xbc/0x298 >> [ 1.652413] __driver_probe_device+0x78/0x12c >> >> In the imx8mp.dtsi device tree, the anatop compatible string is: >> >> compatible = "fsl,imx8mp-anatop", "fsl,imx8mm-anatop"; >> >> So, in configurations like arm64 defconfig, where >> CONFIG_CLK_IMX8MP and CONFIG_CLK_IMX8MM as well as >> CONFIG_CLK_IMX8MN are enabled, the driver for the i.MX8MM >> anatop is incorrectly loaded. >> >> The patch fixes the regression by ensuring that the i.MX8MM anatop >> driver only probes on i.MX8MM platforms. >> >> Fixes: 9c1e388af87c ("clk: imx: add support for i.MX8MM anatop clock >> driver") >> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> >> >> --- >> >> drivers/clk/imx/clk-imx8mm-anatop.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/drivers/clk/imx/clk-imx8mm-anatop.c b/drivers/clk/imx/clk- >> imx8mm-anatop.c >> index 4ac870df6370..90ff11a93fe5 100644 >> --- a/drivers/clk/imx/clk-imx8mm-anatop.c >> +++ b/drivers/clk/imx/clk-imx8mm-anatop.c >> @@ -37,6 +37,19 @@ static const char * const clkout_sels[] = >> {"audio_pll1_out", "audio_pll2_out", " >> static struct clk_hw_onecell_data *clk_hw_data; static struct clk_hw >> **hws; >> >> +static int is_really_imx8mm(struct device_node *np) { >> + const char *compat; >> + struct property *p; >> + >> + of_property_for_each_string(np, "compatible", p, compat) { >> + if (strcmp(compat, "fsl,imx8mm-anatop")) >> + return -EFAULT; EFAULT does not seem like proper error code for ignoring probe. I am pretty sure it leaves you with dmesg regression. Probably you wanted to fix driver matching. Otherwise your compatibles are just wrong. You claim with this: compatible = "fsl,imx8mp-anatop", "fsl,imx8mm-anatop"; That 8mp is fully compatible with 8mm, yet here you claim that 8mm is not handled. So it is both compatible and not compatible. There is some gross misunderstanding what the compatibles mean. Please look at DT spec. Shortly explaining: fallback (8mm) means new device will work somehow correctly, but maybe with limited features, with the driver when bound by the fallback. Other meanings are most likely wrong. I consider that "Support spread spectrum clocking for i.MX8M PLLs" patchset broken (to which I was pointing on early discussions) and should be dropped. Don't fix broken stuff with more broken code. Best regards, Krzysztof To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
On Mon, May 19, 2025 at 04:13:14PM +0200, Krzysztof Kozlowski wrote: > You claim with this: > compatible = "fsl,imx8mp-anatop", "fsl,imx8mm-anatop"; > That 8mp is fully compatible with 8mm, yet here you claim that 8mm is > not handled. So it is both compatible and not compatible. Note that binding for anatop and the above compatible list are in mainline so this is a preexisting issue, whatever differences there are in the versions in the two SoCs aren't triggering problems with the code that's in mainline. The series adding actual clock drivers triggers the issue but the claim that the two were compatible wasn't introduced by it. Given this if people are shipping DTBs based on the existing kernel we might need something like this patch to work around them, even if we fix the existing binding and .dtsi files. To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
diff --git a/drivers/clk/imx/clk-imx8mm-anatop.c b/drivers/clk/imx/clk-imx8mm-anatop.c index 4ac870df6370..90ff11a93fe5 100644 --- a/drivers/clk/imx/clk-imx8mm-anatop.c +++ b/drivers/clk/imx/clk-imx8mm-anatop.c @@ -37,6 +37,19 @@ static const char * const clkout_sels[] = {"audio_pll1_out", "audio_pll2_out", " static struct clk_hw_onecell_data *clk_hw_data; static struct clk_hw **hws; +static int is_really_imx8mm(struct device_node *np) +{ + const char *compat; + struct property *p; + + of_property_for_each_string(np, "compatible", p, compat) { + if (strcmp(compat, "fsl,imx8mm-anatop")) + return -EFAULT; + } + + return 0; +} + static int imx8mm_anatop_clocks_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -44,6 +57,10 @@ static int imx8mm_anatop_clocks_probe(struct platform_device *pdev) void __iomem *base; int ret; + ret = is_really_imx8mm(np); + if (ret) + return ret; + base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(base)) { dev_err(dev, "failed to get base address\n");
Commit 9c1e388af87c ("clk: imx: add support for i.MX8MM anatop clock driver") breaks boot on i.MX8M{P,N} platforms. Here's the log for a board based on the i.MX8MP platform: [ 1.439320] i.MX clk 1: register failed with -2 [ 1.441014] i.MX clk 2: register failed with -2 [ 1.445610] imx8mm-anatop 30360000.clock-controller: NXP i.MX8MM anatop clock driver probed [ 1.455068] Unable to handle kernel paging request at virtual address fffffffffffffffe ... [ 1.634650] Call trace: [ 1.637102] __clk_get_hw+0x4/0x18 (P) [ 1.640862] imx8mp_clocks_probe+0xdc/0x2f50 [ 1.645152] platform_probe+0x68/0xc4 [ 1.648827] really_probe+0xbc/0x298 [ 1.652413] __driver_probe_device+0x78/0x12c In the imx8mp.dtsi device tree, the anatop compatible string is: compatible = "fsl,imx8mp-anatop", "fsl,imx8mm-anatop"; So, in configurations like arm64 defconfig, where CONFIG_CLK_IMX8MP and CONFIG_CLK_IMX8MM as well as CONFIG_CLK_IMX8MN are enabled, the driver for the i.MX8MM anatop is incorrectly loaded. The patch fixes the regression by ensuring that the i.MX8MM anatop driver only probes on i.MX8MM platforms. Fixes: 9c1e388af87c ("clk: imx: add support for i.MX8MM anatop clock driver") Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> --- drivers/clk/imx/clk-imx8mm-anatop.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)