Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put()

[Wolfram Sang]

Hey guys,

I stumbled over this one again and want to make sure we have a
conclusion here.

> The way I understand it, the problem this intends to fix is not the
> state the device ends up in, but that it needs to be powered while
> registers are read or written.

I agree.

> It seems to me that that the current "resume" code should work in that
> respect, because it changes the PM state to "on" in uart_resume_port()
> before any access to hardware registers takes place, so there is
> nothing that needs to be fixed.

Ack.

> That may be different for the "suspend" part, though, because it
> assumes that the PM state is "on", and I think that is what the patch
> asserts to not be a valid assumption anymore. It's hard to tell if
> that is true, though, because I cannot reproduce the issue here; it
> just works either way...

So, do we agree then that *if* there needs to be a fix for that, it
should be in uart_suspend_port() by adding some 'uart_change_pm' shortly
before accessing the ops-callbacks? I am not super-fit with the uart
layer, but can't we assume because of the "Nothing to do if the console
is not suspending"-if-block that (for SCIF at least) it means the port
is on because it _is_ the console?

(I wonder, though, if the OMAPs won't need such a fix because they seem
to use runtime_autosuspend, so their state might be off actually?)

Regards,

   Wolfram



signature.asc
Description: PGP signature

Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put()

[Ulrich Hecht]

Sorry for the delay...

> On April 13, 2018 at 7:27 PM Geert Uytterhoeven  wrote:
> 
> On Fri, Apr 13, 2018 at 7:00 PM, Ulrich Hecht
>  wrote:
> > These patches make sure that the device is up while the suspend/resume code
> > is executed. Up-port from the BSP, tested not to break stuff.
> >
> > Hien Dang (2):
> >   serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend
> >   serial: sh-sci: Use pm_runtime_get_sync()/put() on resume
> 
> I don't think it makes much sense to split this in two parts...
> 
> Furthermore, shouldn't this be handled by the serial core, calling
> uart_change_pm()?
> 
> It looks like uart_resume_port() already changes the port's state to
> enabled, while  uart_suspend_port() assumes the port is already enabled,
> and disables it.
> 
> Perhaps handling is not correct for some code paths?

The way I understand it, the problem this intends to fix is not the state the 
device ends up in, but that it needs to be powered while registers are read or 
written.

It seems to me that that the current "resume" code should work in that respect, 
because it changes the PM state to "on" in uart_resume_port() before any access 
to hardware registers takes place, so there is nothing that needs to be fixed.

That may be different for the "suspend" part, though, because it assumes that 
the PM state is "on", and I think that is what the patch asserts to not be a 
valid assumption anymore. It's hard to tell if that is true, though, because I 
cannot reproduce the issue here; it just works either way...

CU
Uli

Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put()

[Wolfram Sang]

On Fri, Apr 13, 2018 at 07:27:59PM +0200, Geert Uytterhoeven wrote:
> Hi Ulrich,
> 
> On Fri, Apr 13, 2018 at 7:00 PM, Ulrich Hecht
>  wrote:
> > These patches make sure that the device is up while the suspend/resume code
> > is executed. Up-port from the BSP, tested not to break stuff.
> >
> > Hien Dang (2):
> >   serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend
> >   serial: sh-sci: Use pm_runtime_get_sync()/put() on resume
> 
> I don't think it makes much sense to split this in two parts...
> 
> Furthermore, shouldn't this be handled by the serial core, calling
> uart_change_pm()?
> 
> It looks like uart_resume_port() already changes the port's state to
> enabled, while  uart_suspend_port() assumes the port is already enabled,
> and disables it.
> 
> Perhaps handling is not correct for some code paths?

Ulrich, what is your take on this?



signature.asc
Description: PGP signature

Re: [PATCH 0/2] serial: sh-sci: Use pm_runtime_get_sync()/put()

[Geert Uytterhoeven]

Hi Ulrich,

On Fri, Apr 13, 2018 at 7:00 PM, Ulrich Hecht
 wrote:
> These patches make sure that the device is up while the suspend/resume code
> is executed. Up-port from the BSP, tested not to break stuff.
>
> Hien Dang (2):
>   serial: sh-sci: Use pm_runtime_get_sync()/put() on suspend
>   serial: sh-sci: Use pm_runtime_get_sync()/put() on resume

I don't think it makes much sense to split this in two parts...

Furthermore, shouldn't this be handled by the serial core, calling
uart_change_pm()?

It looks like uart_resume_port() already changes the port's state to
enabled, while  uart_suspend_port() assumes the port is already enabled,
and disables it.

Perhaps handling is not correct for some code paths?

Gr{oetje,eeting}s,

Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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