[PATCH] clk: mvebu: ap806: fix legacy binding failing to probe

[PATCH] clk: mvebu: ap806: fix legacy binding failing to probe

[Bruno Banelli]

Commit be69e55df9af ("clk: mvebu: ap806: Prepare the introduction of
AP807 clock support") factored the probe path into
ap806_syscon_common_probe() and added a test that picks the Sample at
Reset decoding table from the device node's compatible string:

	if (of_device_is_compatible(pdev->dev.of_node,
				    "marvell,ap806-clock")) {
		ret = ap806_get_sar_clocks(freq_mode, &cpuclk_freq, &dclk_freq);
	} else if (of_device_is_compatible(pdev->dev.of_node,
					   "marvell,ap807-clock")) {
		ret = ap807_get_sar_clocks(freq_mode, &cpuclk_freq, &dclk_freq);
	} else {
		dev_err(dev, "compatible not supported\n");
		return -EINVAL;
	}

That is correct for the current binding, where the clock node carries
"marvell,ap806-clock" or "marvell,ap807-clock".  It is however also
reached from ap806_syscon_legacy_probe(), whose device node carries
"marvell,ap806-system-controller" and "syscon" - neither of the two
compatibles being tested for.  The legacy path therefore prints its
three deprecation warnings and then fails unconditionally:

  marvell-ap806-system-controller f06f4000.system-controller:
    [Firmware Warn]: Using legacy device tree binding
    [Firmware Warn]: Update your device tree:
    [Firmware Warn]: This binding won't be supported in future kernel
    compatible not supported
    probe with driver marvell-ap806-system-controller failed, error -22

The AP806 system controller is the provider of the AP clocks, so no
clock is registered at all and every consumer defers indefinitely.  On
an Armada 8040 that includes the console UART:

  platform f0512000.serial: deferred probe pending: platform:
    supplier f06f4000.system-controller not ready

which turns the failure into an apparent silent hang once userspace
starts, rather than a visible probe error.

Before v5.4, ap806_syscon_common_probe() had no compatible test and went
straight from reading Sample at Reset to decoding freq_mode, so the
legacy binding did work.  The commit introducing the test described
itself as "No functional changes", ap806_syscon_legacy_of_match[] still
advertises the legacy compatible, and the warnings above state only that
the binding is deprecated.  The breakage therefore looks unintentional.

Accept the legacy compatible in the AP806 branch so that the legacy
binding works again as it did before v5.4, keeping the deprecation
warnings intact.

Fixes: be69e55df9af ("clk: mvebu: ap806: Prepare the introduction of AP807 clock support")
Signed-off-by: Bruno Banelli <bbanelli@gmail.com>
---
 drivers/clk/mvebu/ap806-system-controller.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Found while booting mainline Linux with U-Boot's control FDT on a
SolidRun MACCHIATObin (Armada 8040). U-Boot's arch/arm/dts/armada-ap80x.dtsi
still uses the legacy binding, so any board booting Linux via U-Boot's EFI
handover with the firmware-supplied FDT hits this.

diff --git a/drivers/clk/mvebu/ap806-system-controller.c b/drivers/clk/mvebu/ap806-system-controller.c
index 948bd1e71aea..332e7c21ea07 100644
--- a/drivers/clk/mvebu/ap806-system-controller.c
+++ b/drivers/clk/mvebu/ap806-system-controller.c
@@ -152,7 +152,9 @@ static int ap806_syscon_common_probe(struct platform_device *pdev,
 	freq_mode = reg & AP806_SAR_CLKFREQ_MODE_MASK;
 
 	if (of_device_is_compatible(pdev->dev.of_node,
-				    "marvell,ap806-clock")) {
+				    "marvell,ap806-clock") ||
+	    of_device_is_compatible(pdev->dev.of_node,
+				    "marvell,ap806-system-controller")) {
 		ret = ap806_get_sar_clocks(freq_mode, &cpuclk_freq, &dclk_freq);
 	} else if (of_device_is_compatible(pdev->dev.of_node,
 					   "marvell,ap807-clock")) {
-- 
2.43.0

Re: [PATCH] clk: mvebu: ap806: fix legacy binding failing to probe

[Miquel Raynal]

Hi Bruno,

[...]

> Before v5.4, ap806_syscon_common_probe() had no compatible test and went
> straight from reading Sample at Reset to decoding freq_mode, so the
> legacy binding did work.  The commit introducing the test described
> itself as "No functional changes", ap806_syscon_legacy_of_match[] still
> advertises the legacy compatible, and the warnings above state only that
> the binding is deprecated.  The breakage therefore looks unintentional.
>
> Accept the legacy compatible in the AP806 branch so that the legacy
> binding works again as it did before v5.4, keeping the deprecation
> warnings intact.
>
> Fixes: be69e55df9af ("clk: mvebu: ap806: Prepare the introduction of AP807 clock support")
> Signed-off-by: Bruno Banelli <bbanelli@gmail.com>
> ---
>  drivers/clk/mvebu/ap806-system-controller.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> Found while booting mainline Linux with U-Boot's control FDT on a
> SolidRun MACCHIATObin (Armada 8040). U-Boot's arch/arm/dts/armada-ap80x.dtsi
> still uses the legacy binding, so any board booting Linux via U-Boot's EFI
> handover with the firmware-supplied FDT hits this.

While I agree with the report, I kind of dislike the fix which
re-instates a clearly unsupported compatible, legacy since 2019. Maybe
we can start by discussing the need: wouldn't it be more sensible to
update your DT? The switch has been operated in 2017, almost 10 years
ago, with a big fat deprecation notice (which you hit).

If we *really* want to fix this situation, I would prefer to avoid using
the long time deprecated compatible. Could we instead have the callers
(ap806_syscon_common_probe() and ap806_clock_probe()) give either an enum
or a *get_sar function pointer? The former would only provide the AP806
one, whereas the latter would still go through the same DT check.

Finally, if we go for a fix, it should be backported, so stable should
probably be Cc'ed.

Thanks,
Miquèl