Re: [PATCH v3 2/2] i2c: Add Renesas RZ/V2M controller

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Phil,

On Thu, Jul 7, 2022 at 6:37 PM Phil Edworthy <phil.edworthy@renesas.com> 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@linux.intel.com> 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.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds