On Thu, 7 Oct 2021 at 01:21, Dmitry Osipenko [off-list ref] wrote:

07.10.2021 01:01, Dmitry Osipenko пишет:

quoted

07.10.2021 00:14, Dmitry Osipenko пишет:

quoted

06.10.2021 15:43, Ulf Hansson пишет:

quoted

On Wed, 6 Oct 2021 at 00:43, Dmitry Osipenko [off-list ref] wrote:

quoted

06.10.2021 01:19, Dmitry Osipenko пишет:
...

quoted

I reproduced the OFF problem by removing the clk prepare/unprepare from
the suspend/resume of the clk driver and making some extra changes to
clock tree topology and etc to trigger the problem on Nexus 7.

tegra-pmc 7000e400.pmc: failed to turn off PM domain heg: -13

It happens from genpd_suspend_noirq() -> tegra_genpd_power_off() -> clk
-> GENPD -> I2C -> runtime-pm.

-13 is EACCES, it comes from the runtime PM of I2C device. RPM is
prohibited/disabled during late (NOIRQ) suspend by the drivers core.

My bad, I double-checked and it's not I2C RPM that is failing now, but
the clock's RPM [1], which is also unavailable during NOIRQ.

Yes, that sounds reasonable.

You would then need a similar patch for the tegra clock driver as I
suggested for tegra I2C driver. That should solve the problem, I
think.

quoted

[1]
https://elixir.free-electrons.com/linux/v5.15-rc4/source/drivers/clk/clk.c#L116

Previously it was I2C RPM that was failing in a similar way, but code
changed a tad since that time.

Alright. In any case, as long as the devices gets suspended in the
correct order, I think it should be fine to cook a patch along the
lines of what I suggest for the I2C driver as well.

It should work, I think. Although, maybe you want to avoid runtime
resuming the I2C device, unless it's the device belonging to the PMIC
interface, if there is a way to distinguish that for the driver.

Ulf, thank you very much for the suggestions! I was thinking about this
all once again and concluded that the simplest variant will be to just
remove the suspend ops from the clk driver since neither of PLLs require
high voltage. We now have voltage bumped to a nominal level during
suspend by Tegra's regulator-coupler driver and it's much higher than
voltage needed by PLLs. So the problem I was trying to work around
doesn't really exist anymore.

I hurried a bit with the conclusion, keep forgetting that I need to
change the clock tree in order to test it all properly :/ It's not fixed
yet.

Please let me iterate once again. The problem we currently have is that
clock may be enabled during NOIRQ time. In order to enable clock, it
needs to be prepared. In order to prepare clock, the clock's device
needs to be runtime-resumed. The runtime PM is unavailable at the NOIRQ
time.

To solve this problem we need to prepare clock beforehand.

The clock will stay prepared during suspend, but this is not a problem
since all the clocks we care about don't require high voltage and
voltage is guaranteed to be bumped high during suspend by Tegra's
regulator-coupler driver anyways.

So everything we need to do is to keep clocks prepared. There are two
options how to do that:

[1] this patch which explicitly prepares clocks using clk API.

[2] Use runtime PM API, like this:

static const struct dev_pm_ops tegra_clock_pm = {
        SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_resume_and_get, pm_runtime_put)
};

Ulf, are you now okay with the current variant [1] of the patch or you
prefer the second [2] option more?

I prefer option [2]. The clock_prepare|unprepare() thingy in option
[1], looks more like an odd workaround to me.

Does that make sense to you as well?

Kind regards
Uffe