[linux-next, 1/1] clk: imx: imx8mm-anatop: probe only on i.MX8MM platforms

[linux-next, 1/1] clk: imx: imx8mm-anatop: probe only on i.MX8MM platforms

[Dario Binacchi]

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

Re: [linux-next, 1/1] clk: imx: imx8mm-anatop: probe only on i.MX8MM platforms

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 876 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [linux-next, 1/1] clk: imx: imx8mm-anatop: probe only on i.MX8MM platforms

[Peng Fan]

> 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

Re: [linux-next, 1/1] clk: imx: imx8mm-anatop: probe only on i.MX8MM platforms

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 670 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [linux-next, 1/1] clk: imx: imx8mm-anatop: probe only on i.MX8MM platforms

[Krzysztof Kozlowski]

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

Re: [linux-next, 1/1] clk: imx: imx8mm-anatop: probe only on i.MX8MM platforms

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 827 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]