Re: [PATCH] iio: imu: lsm6dsx: Add shutdown callback support for I3C interface

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2025-07-29 at 12:02 +0000, Guntupalli, Manikanta wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
> Hi @Jorge Marques,
> 
> > -----Original Message-----
> > From: Jorge Marques <gastmaier@gmail.com>
> > Sent: Tuesday, July 22, 2025 1:27 PM
> > To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>
> > Cc: David Lechner <dlechner@baylibre.com>; Andy Shevchenko
> > <andriy.shevchenko@intel.com>; git (AMD-Xilinx) <git@amd.com>; Simek, Michal
> > <michal.simek@amd.com>; lorenzo@kernel.org; jic23@kernel.org;
> > nuno.sa@analog.com; andy@kernel.org; linux-iio@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Pandey, Radhey Shyam
> > <radhey.shyam.pandey@amd.com>; Goud, Srinivas <srinivas.goud@amd.com>;
> > manion05gk@gmail.com
> > Subject: Re: [PATCH] iio: imu: lsm6dsx: Add shutdown callback support for
> > I3C
> > interface
> > 
> > On Tue, Jul 22, 2025 at 07:32:54AM +0000, Guntupalli, Manikanta wrote:
> > > [AMD Official Use Only - AMD Internal Distribution Only]
> > > 
> > > Hi @David Lechner,
> > > 
> > > > -----Original Message-----
> > > > From: David Lechner <dlechner@baylibre.com>
> > > > Sent: Tuesday, July 22, 2025 2:31 AM
> > > > To: Andy Shevchenko <andriy.shevchenko@intel.com>; Guntupalli,
> > > > Manikanta <manikanta.guntupalli@amd.com>
> > > > Cc: git (AMD-Xilinx) <git@amd.com>; Simek, Michal
> > > > <michal.simek@amd.com>; lorenzo@kernel.org; jic23@kernel.org;
> > > > nuno.sa@analog.com; andy@kernel.org; linux-iio@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; Pandey, Radhey Shyam
> > > > <radhey.shyam.pandey@amd.com>; Goud, Srinivas
> > > > <srinivas.goud@amd.com>; manion05gk@gmail.com
> > > > Subject: Re: [PATCH] iio: imu: lsm6dsx: Add shutdown callback
> > > > support for I3C interface
> > > > 
> > > > On 7/21/25 6:38 AM, Andy Shevchenko wrote:
> > > > > On Mon, Jul 21, 2025 at 04:37:41PM +0530, Manikanta Guntupalli wrote:
> > > > > > Add a shutdown handler for the ST LSM6DSx I3C driver to perform a
> > > > > > hardware reset during system shutdown. This ensures the sensor is
> > > > > > placed in a well-defined reset state, preventing issues during
> > > > > > subsequent reboots, such as kexec, where the device may fail to
> > > > > > respond correctly during enumeration.
> > > > > 
> > > > > Do you imply that tons of device drivers missing this? I don't
> > > > > think we have even 5% of the drivers implementing the feature.
> > > > > 
> > > > In the IIO drivers I've worked on, we always do reset in the probe()
> > > > function. The
> > > > shutdown() function might not run, e.g. if the board loses power, so
> > > > it doesn't fix 100% of the cases.
> > > 
> > > Thank you for the input.
> > > 
> > > You're absolutely right — shutdown() may not cover all cases like power
> > > loss.
> > However, in scenarios such as a warm reboot (kexec), the situation is
> > different.
> > > 
> > > Before the probe is called in the next boot, device enumeration takes
> > > place. During
> > this process, the I3C framework compares the device’s PID, BCR, and DCR
> > values
> > against the ones registered in the driver:
> > > 
> > > static const struct i3c_device_id st_lsm6dsx_i3c_ids[] = {
> > >         I3C_DEVICE(0x0104, 0x006C, (void *)ST_LSM6DSO_ID),
> > >         I3C_DEVICE(0x0104, 0x006B, (void *)ST_LSM6DSR_ID),
> > >         { }
> > > };
> > > 
> > > Only if this matching succeeds, the probe will be invoked.
> > > 
> > > Since the sensor reset logic is placed inside the probe, the device must
> > > be in a
> > responsive state during enumeration. In the case of kexec, we observed that
> > the
> > sensor does not respond correctly unless it is explicitly reset during
> > shutdown().
> > Hence, adding the reset in shutdown() addresses this specific case where the
> > probe
> > isn't reached due to failed enumeration.
> > > 
> > Hi Manikanta,
> > 
> > During i3c bus init, the CCC RSTDAA is emitted to reset all DAs of all
> > devices in the
> > bus (drivers/i3c/master.c@i3c_master_bus_init -> i3c_master_rstdaa_locked).
> > Is the
> > LSM6DSX not compliant with that?
> LSM6DSX is compliant with RSTDAA CCC.
> 
> > 
> > I get your solution but find odd to use the same method as in the probe.
> > In the probe, you would, in general, reset the device logic, but leave the
> > i3c
> > peripheral logic intact, because you don't want to undo whatever the
> > controller has
> > set-up for the current bus attached devices (ibi config, da, max devices
> > speed, all the
> > good i3c stuff).
> > For this device, the st_lsm6dsx_reset_device seems to flush a FIFO, do a
> > software
> > reset, and reload a trimming parameter; which are necessary to solve the bug
> > you
> > are observed?
> Only software reset necessary to solve the bug.
> 
> > 
> > If possible, please explain better why the device won't enumerate correctly
> > after a
> > reboot without the reset. If it is a device bug, explicitly state that and
> > that it is not
> > compliant. Also, take a look at fig.100 of the i3c spec basic 1.1.1.
> > 
> > Thank you for looking into this, this type of corner case is usually
> > overlooked.
> It appears that the sensor device is entering a deep sleep or low-power state
> and is not responding to CCC commands. However, after a software reset, the
> sensor starts responding to CCCs as expected.
> 

It seems the device has some PM ops implemented [1]. Can it be that those are
being called somehow? I don't think they should be called during shutdown but it
would at least explain the low power state.

[1]: https://elixir.bootlin.com/linux/v6.17-rc6/source/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c#L2730

- Nuno Sá

> Shall we proceed with only the software reset changes along with an improved
> description, or do you recommend any additional modifications?
> 
> Thanks,
> Manikanta.