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 8:47 PM Phil Edworthy <phil.edworthy@renesas.com> wrote:
> 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@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.
> 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?

That's one option.
There's also i2c_mark_adapter_suspended().

And if you care about sending messages in atomic/noirq context,
you want to implement i2c_algorithm.master_xfer_atomic().

You may want to have a look at drivers/i2c/busses/i2c-sh_mobile.c,
which is used to control the PMIC on R-Car SoCs.

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