Hi Geert, On 07 July 2022 18:46 Geert Uytterhoeven wrote: > On Thu, Jul 7, 2022 at 6:37 PM Phil Edworthy wrote: > > On 07 July 2022 08:21 Phil Edworthy wrote: > > > On 03 July 2022 16:17 Andy Shevchenko wrote: > > > > On Sun, Jul 03, 2022 at 10:41:45AM +0200, Geert Uytterhoeven wrote: > > > > > On Sat, Jul 2, 2022 at 1:51 PM Andy Shevchenko > > > > > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > > > > > > On Fri, Jul 01, 2022 at 05:39:16PM +0100, Phil Edworthy wrote: > > > > > > > Yet another i2c controller from Renesas that is found on the > > > RZ/V2M > > > > > > > (r9a09g011) SoC. It can support only 100kHz and 400KHz > operation. > > > > > > > > ... > > > All other suggested changes are ok. > > > > > > > > > > > > > + pm_runtime_get_sync(dev); > > > > > > > > > > pm_runtime_resume_and_get() ;-) > > > > > > > > This makes sense only if we test for error. Otherwise the put might > > > > imbalance > > > > counter. > > > I added code to check the return value and to my surprise it returned > > > -EACCES. > > > Some digging later, this only happens when I have an i2c controller > > > enabled that doesn't have any children. > > > > > > rpm_resume() returns -EACCES [1] because runtime_status and > last_status > > > are set to RPM_SUSPENDED. > > > > > > The i2c controller that does have a child has runtime_status = > RPM_ACTIVE > > > as there is a call to pm_runtime_resume_and_get() on it due to the i2c > > > controller performing an i2c transfer for the slave device. > > > > > > I am currently struggling to work out why this is happening... > > > > First pm_suspend() works it's way down to __pm_runtime_disable(): > > __pm_runtime_disable+0x134/0x1e0 > > __device_suspend_late+0x28/0x1c4 > > dpm_suspend_late+0x158/0x230 > > suspend_devices_and_enter+0x1c8/0x4b4 > > pm_suspend+0x210/0x28c > > At the end of which, runtime_status and last_status are both > RPM_SUSPENDED, > > and disable_depth = 1 [1] > > > > After that rzv2m_i2c_suspend() is called triggering the EACCES error > > condition [2]: > > rpm_resume+0x338/0x630 > > __pm_runtime_resume+0x4c/0x80 > > rzv2m_i2c_suspend+0x24/0xb0 > > pm_generic_suspend_noirq+0x30/0x50 > > genpd_finish_suspend+0xb0/0x130 > > genpd_suspend_noirq+0x14/0x20 > > __device_suspend_noirq+0x68/0x1d0 > > dpm_noirq_suspend_devices+0x110/0x1dc > > dpm_suspend_noirq+0x24/0xa0 > > suspend_devices_and_enter+0x2f0/0x4b4 > > pm_suspend+0x210/0x28c > > > > I think using runtime PM from within driver suspend/resume is simply not > > supported. However I had some difficulty following the runtime PM code, > > so I could be wrong. > > Oh, it's a NOIRQ system sleep op. You indeed cannot use runtime resume > from such a callback, as the latter may sleep. Thanks for confirming this. I believe i2c controller driver should use NOIRQ system sleep ops as i2c children may need to send I2C messages during suspend, and the noirq sleep ops are called after the late sleep ops (used by some i2c children drivers). So should I just use clk_prepare_enable() and clk_prepare_enable() within the i2c controller's suspend and resume? BR Phil
