Re: [PATCH RFC] pmdomain: ti-sci: Set PD on/off state according to the HW state

[Tomi Valkeinen]

Hi,

On 31/10/2024 12:25, Ulf Hansson wrote:
> On Thu, 31 Oct 2024 at 08:39, Tomi Valkeinen
> <tomi.valkeinen@xxxxxxxxxxxxxxxx> wrote:
> > Hi,
> >
> > On 30/10/2024 22:04, Kevin Hilman wrote:
> > > Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> writes:
> > >
> > > > At the moment the driver sets the power state of all the PDs it creates
> > > > to off, regardless of the actual HW state. This has two drawbacks:
> > > >
> > > > 1) The kernel cannot disable unused PDs automatically for power saving,
> > > >      as it thinks they are off already
> > > >
> > > > 2) A more specific case (but perhaps applicable to other scenarios
> > > >      also): bootloader enabled splash-screen cannot be kept on the screen.
> > > >
> > > > The issue in 2) is that the driver framework automatically enables the
> > > > device's PD before calling probe() and disables it after the probe().
> > > > This means that when the display subsystem (DSS) driver probes, but e.g.
> > > > fails due to deferred probing, the DSS PD gets turned off and the driver
> > > > cannot do anything to affect that.
> > > >
> > > > Solving the 2) requires more changes to actually keep the PD on during
> > > > the boot, but a prerequisite for it is to have the correct power state
> > > > for the PD.
> > > >
> > > > The downside with this patch is that it takes time to call the 'is_on'
> > > > op, and we need to call it for each PD. In my tests with AM62 SK, using
> > > > defconfig, I see an increase from ~3.5ms to ~7ms. However, the added
> > > > feature is valuable, so in my opinion it's worth it.
> > > >
> > > > The performance could probably be improved with a new firmware API which
> > > > returns the power states of all the PDs.
> > >
> > > Agreed.  I think we have to pay this performance price for correctness,
> > > and we can optimizie it later with improvements to the SCI firmware and
> > > a new API.
> > >
> > > > There's also a related HW issue at play here: if the DSS IP is enabled
> > > > and active, and its PD is turned off without first disabling the DSS
> > > > display outputs, the DSS IP will hang and causes the kernel to halt if
> > > > and when the DSS driver accesses the DSS registers the next time.
> > >
> > > Ouch.
> > >
> > > > With the current upstream kernel, with this patch applied, this means
> > > > that if the bootloader enables the display, and the DSS driver is
> > > > compiled as a module, the kernel will at some point disable unused PDs,
> > > > including the DSS PD. When the DSS module is later loaded, it will hang
> > > > the kernel.
> > > >
> > > > The same issue is already there, even without this patch, as the DSS
> > > > driver may hit deferred probing, which causes the PD to be turned off,
> > > > and leading to kernel halt when the DSS driver is probed again. This
> > > > issue has been made quite rare with some arrangements in the DSS
> > > > driver's probe, but it's still there.
> > > >
> > > > So, because of the DSS hang issues, I think this patch is still an RFC.
> > >
> > > Like you said, I think that DSS hang is an issue independently of this
> > > patch, so it shouldn't hold this up IMO.
> >
> > In current upstream, if the bootloader has enabled the display, we most
> > likely won't hit the DSS hang issue as the PD will stay on until the DSS
> > driver has had a chance to probe, and the driver takes actions to avoid
> > the hang issue.
> >
> > With this patch applied, the PD may be turned off before the DSS driver
> > has had a chance to probe, causing the board to hang when the DSS driver
> > probes the first time.
> >
> > That's why I'm a bit hesitant to apply this. It could mean that for some
> > people their board stops booting.
> >
> > I'm not even sure what would be the perfect fix for this hang problem...
> >
> > We could have some built-in early boot code which checks if the DSS is
> > enabled, and disables it, so that the hang issue won't happen. But
> > that's not good if we try to keep the boot splash on the screen until
> > the userspace takes over.
> >
> > Alternatively we could, somehow, mark the DSS powerdomain to be handled
> > in a special way: if the PD is enabled at boot time, it will be kept
> > enabled until the DSS driver (somehow) changes the PD back to normal
> > operation (and if DSS driver is never loaded, PD will stay on).
>
> This option is kind of what I am working on. Although, the goal is to
> keep the code generic, so ideally we should not need any changes in
> the DSS driver to make this work. Let's see.

If you need someone to do some testing, you know who to ask =).

> That said, it sounds like we should defer $subject patch until we have
> a solution for the above, right?

Yes, I would say so.

I'll collect the tags, and will resend this patch (as a non-RFC) when I 
think it's safe.

Thanks for the feedback, Kevin and Ulf.

 Tomi