Message-ID: <20250318-cuddly-translucent-teal-e2ac2d@leitao>
Date: Tue, 18 Mar 2025 03:36:54 -0700
From: Breno Leitao <leitao@...ian.org>
To: Mark Brown <broonie@...ian.org>
Cc: Thierry Reding <thierry.reding@...il.com>,
	Jonathan Hunter <jonathanh@...dia.com>,
	Sowjanya Komatineni <skomatineni@...dia.com>,
	Laxman Dewangan <ldewangan@...dia.com>, linux-tegra@...r.kernel.org,
	linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
	rmikey@...a.com, kernel-team@...a.com
Subject: Re: [PATCH 1/3] spi: tegra210-quad: use device_reset_optional()
 instead of device_reset()

Hello Mark,

On Mon, Mar 17, 2025 at 05:24:24PM +0000, Mark Brown wrote:
> > > There's no specific handling for ACPI here.
>
> > Do you mean no _RST method as stated above?
>
> That's only happening in the case where the device has an ACPI handle,
> the SPI driver has no idea why the reset API failed to look up a reset
> controller.  Your change is to the SPI driver, not the reset framework.
>
> > > It's also not clear that this is a false positive, the
> > > driver did indeed fail to reset the device and especially for the error
> > > handling case that seems like relevant information.
>
> > If the driver failed to reset the device, then device_reset_optional()
> > it will return an error code, but it will not return an error code if
> > the RST method is not found, right?
>
> > Sorry, if I am mis-understading the code here.
>
> Clearly if no reset controller is available then the driver will have
> been unable to reset the hardware.  That seems like something it
> actually wanted to do, especially in the error handling case - it's a
> lot less likely that we'll recover things without the reset happening.
> During probe it's possibly not so urgent but at other times it seems
> more relevant.

Thanks for your answer! Let me backup and explain how I am understanding
this issue, and my possible wrong assumptions:

  1) The SPI controller is reseted in the driver in a few cases:
  	a) At probe time
  	b) At transmission side (when there is a timeout to the controller)

  2) On the machines I have, I understand that  the controller failed to
     reset on both cases:
  	a) At boot time with "tegra-qspi NVDA1513:00: device reset failed"
  	b) At error handling with, the message below:

  	   tegra-qspi NVDA1513:00: QSPI Transfer failed with timeout: 0
             spi_master spi0: failed to transfer one message from queue
             spi_master spi0: noqueue transfer failed
  	   WARNING: CPU: 1 PID: 1221 at drivers/spi/spi-tegra210-quad.c:1120 tegra_qspi_transfer_one_message+0x780/0x918 [spi_tegra210_quad]

  	   Full log at: https://paste.debian.net/1363773/

  	c) I don't see the "device reset failed" in this case
  	transmission side. But the device doesn't recover also.

  3) These device fail to reset at probe because there is no ACPI method
     related resetting then (aka "_RST" methods), thus, device_reset() will
     return -ENOENT;

  4) Not being able to reset the driver seems to be the root cause of
     the WARNING flood I am seeing.


My assumptions, now:

  1) This controller doesn't have _RST ACPI method by design.

  2) It is OK to not have reset methods (!?)

  3) There are two helpers to reset the driver device_reset_optional() and
     device_reset().
  	a) For device_reset(), the helper will fail if the device
  	doesn't reset, thus, for ACPI systems, the _RST method needs
  	to exist and return successful, otherwise it will return
  	a ERRNO.

  	b) device_reset_optional() only fails if the reset fail (either
  	in ACPI or not), but, doesn't fail (aka returning 0) if reset
  	methods (aka _RST in ACPI) is not available.

  	c) Given assumption #1, device_reset_optional() is more
  	appropriate given that this method does not exist anyway.

  	d) This should be a no-op for systems that have proper reset
  	methods.

Thanks for helping me with this issue,
--breno

Powered by blists - more mailing lists