Re: SPI regression seen on ARM am335x in kernel 6.12.8 and 6.6.71

[Sicelo <absicsz@gmail.com>]

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

Hi

On Fri, Jan 17, 2025 at 03:49:25PM +0100, Lars Pedersen wrote:
> On Fri, 17 Jan 2025 at 13:32, Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Jan 16, 2025 at 03:21:13PM +0100, Lars Pedersen wrote:
> > > Hi. We have discovered an SPI regression when upgrading from 6.1.99 to
> > > a newer LTS version. Same error on kernel 6.6.71 and 6.12.8.

We have a very similar regression on the Nokia N900, causing everything
on the SPI bus (WLAN, screen) to stop working or be unreliable.

> > >
> > > I think we have identified the problem down to the reference clock
> > > calculation that seems to end up to zero in the spi-omap2-mcspi
> > > driver.
> > >
> > > Also we think it relates to commit
> > > 4c6ac5446d060f0bf435ccc8bc3aa7b7b5f718ad, where OMAP2_MCSPI_MAX_FREQ
> > > is used as fallback on error. In our case it seems to hit the else
> > > case.
> > >
> > If you revert the offending commit, does that solve the issue?
> >
> > thanks,
> >
> > greg k-h
> 
> Hi Greg.
> 
> No it doesn't solve the issue by reverting the commit. The commit
> isn't the regression, but it attempts to handle it in the if/else
> statement. Everything starts to work again if we hard code it to
> "mcspi->ref_clk_hz = OMAP2_MCSPI_MAX_FREQ;", so it seems like the if
> else statement isn't 100% foolproof (or we have missed a setting in
> the device tree).

The commit actually *is* the regression. The subtle issue causing it is
that devm_clk_get_optional_enabled() may also return NULL. In the
previous code, the NULL was not considered an error condition. The
change to the IS_ERR macro causes the NULL condition to be unhandled,
hence it takes the `else` path. Using IS_ERR_OR_NULL restores the
previous behavior, since changing it was not the intention of the
commit in question.

Please check if the attached patch does not fix it for you, and I can
submit it, perhaps with your Tested-by. It does resolve the issue on
the Nokia N900, for both WLAN and the screen.

Regards
Sicelo

[-- Attachment #2: 0001-spi-omap2-mcspi-allow-NULL-from-devm_clk_get_optiona.patch --]
[-- Type: text/x-diff, Size: 1282 bytes --]

From 451b01800b7fde7be3fa64b3eb87d8ff91628eb4 Mon Sep 17 00:00:00 2001
From: "Sicelo A. Mhlongo" <absicsz@gmail.com>
Date: Thu, 23 Jan 2025 07:38:02 +0200
Subject: [PATCH] spi: omap2-mcspi: allow NULL from
 devm_clk_get_optional_enabled

In addition to an error pointer, devm_clk_get_optional_enabled can also
return NULL, which in omap2-mcspi is not to be considered an error.
Rework  4c6ac5446d06 ("spi: omap2-mcspi: Fix the IS_ERR() bug for
devm_clk_get_optional_enabled()") so the NULL is handled correctly.

Fixes: 4c6ac5446d06 ("spi: omap2-mcspi: Fix the IS_ERR() bug for devm_clk_get_optional_enabled()")
Signed-off-By: Sicelo A. Mhlongo <absicsz@gmail.com>
---
 drivers/spi/spi-omap2-mcspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index add6247d3481..cde4416f3cb2 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -1561,7 +1561,7 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
 	}
 
 	mcspi->ref_clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
-	if (IS_ERR(mcspi->ref_clk))
+	if (IS_ERR_OR_NULL(mcspi->ref_clk))
 		mcspi->ref_clk_hz = OMAP2_MCSPI_MAX_FREQ;
 	else
 		mcspi->ref_clk_hz = clk_get_rate(mcspi->ref_clk);
-- 
2.47.1