[v4,02/10] clk: sunxi-ng: Mark AR100 clocks as critical
diff mbox series

Message ID 20190820032311.6506-3-samuel@sholland.org
State New
Headers show
Series
  • Allwinner sunxi message box support
Related show

Commit Message

Samuel Holland Aug. 20, 2019, 3:23 a.m. UTC
On sun8i, sun9i, and sun50i SoCs, system suspend/resume support requires
firmware running on the AR100 coprocessor (the "SCP"). Such firmware can
provide additional features, such as thermal monitoring and poweron/off
support for boards without a PMIC.

Since the AR100 may be running critical firmware, even if Linux does not
know about it or directly interact with it (all requests may go through
an intermediary interface such as PSCI), Linux must not turn off its
clock.

At this time, such power management firmware only exists for the A64 and
H5 SoCs.  However, it makes sense to take care of all CCU drivers now
for consistency, and to ease the transition in the future once firmware
is ported to the other SoCs.

Leaving the clock running is safe even if no firmware is present, since
the AR100 stays in reset by default. In most cases, the AR100 clock is
kept enabled by Linux anyway, since it is the parent of all APB0 bus
peripherals. This change only prevents Linux from turning off the AR100
clock in the rare case that no peripherals are in use.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---
 drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c | 2 +-
 drivers/clk/sunxi-ng/ccu-sun8i-r.c     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Maxime Ripard Aug. 20, 2019, 7:11 a.m. UTC | #1
Hi,

On Mon, Aug 19, 2019 at 10:23:03PM -0500, Samuel Holland wrote:
> On sun8i, sun9i, and sun50i SoCs, system suspend/resume support requires
> firmware running on the AR100 coprocessor (the "SCP"). Such firmware can
> provide additional features, such as thermal monitoring and poweron/off
> support for boards without a PMIC.
>
> Since the AR100 may be running critical firmware, even if Linux does not
> know about it or directly interact with it (all requests may go through
> an intermediary interface such as PSCI), Linux must not turn off its
> clock.
>
> At this time, such power management firmware only exists for the A64 and
> H5 SoCs.  However, it makes sense to take care of all CCU drivers now
> for consistency, and to ease the transition in the future once firmware
> is ported to the other SoCs.
>
> Leaving the clock running is safe even if no firmware is present, since
> the AR100 stays in reset by default. In most cases, the AR100 clock is
> kept enabled by Linux anyway, since it is the parent of all APB0 bus
> peripherals. This change only prevents Linux from turning off the AR100
> clock in the rare case that no peripherals are in use.
>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

So I'm not really sure where you want to go with this.

That clock is only useful where you're having a firmware running on
the AR100, and that firmware would have a device tree node of its own,
where we could list the clocks needed for the firmware to keep
running, if it ever runs. If the driver has not been compiled in /
loaded, then we don't care either.

But more fundamentally, if we're going to use SCPI, then those clocks
will not be handled by that driver anyway, but by the firmware, right?

So I'm not really sure that we should do it statically this way, and
that we should do it at all.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Samuel Holland Aug. 20, 2019, 1:02 p.m. UTC | #2
On 8/20/19 2:11 AM, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Aug 19, 2019 at 10:23:03PM -0500, Samuel Holland wrote:
>> On sun8i, sun9i, and sun50i SoCs, system suspend/resume support requires
>> firmware running on the AR100 coprocessor (the "SCP"). Such firmware can
>> provide additional features, such as thermal monitoring and poweron/off
>> support for boards without a PMIC.
>>
>> Since the AR100 may be running critical firmware, even if Linux does not
>> know about it or directly interact with it (all requests may go through
>> an intermediary interface such as PSCI), Linux must not turn off its
>> clock.

This paragraph here is the key. The firmware won't necessarily have a device
tree node, and in the current design it will not, since Linux never communicates
with it directly. All communication goes through ATF via PSCI.

>> At this time, such power management firmware only exists for the A64 and
>> H5 SoCs.  However, it makes sense to take care of all CCU drivers now
>> for consistency, and to ease the transition in the future once firmware
>> is ported to the other SoCs.
>>
>> Leaving the clock running is safe even if no firmware is present, since
>> the AR100 stays in reset by default. In most cases, the AR100 clock is
>> kept enabled by Linux anyway, since it is the parent of all APB0 bus
>> peripherals. This change only prevents Linux from turning off the AR100
>> clock in the rare case that no peripherals are in use.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
> 
> So I'm not really sure where you want to go with this.
> 
> That clock is only useful where you're having a firmware running on
> the AR100, and that firmware would have a device tree node of its own,
> where we could list the clocks needed for the firmware to keep
> running, if it ever runs. If the driver has not been compiled in /
> loaded, then we don't care either.

See above. I don't expect that the firmware would have a device tree node,
because the firmware doesn't need any Linux drivers.

> But more fundamentally, if we're going to use SCPI, then those clocks
> will not be handled by that driver anyway, but by the firmware, right?

In the future, we might use SCPI clocks/sensors/regulators/etc. from Linux, but
that's not the plan at the moment. Given that it's already been two years since
I started this project, I'm trying to limit its scope so I can get at least some
part merged. The first step is to integrate a firmware that provides
suspend/resume functionality only. That firmware does implement SCPI, and if the
top-level Linux SCPI driver worked with multiple mailbox channels, it could
query the firmware's version and fetures. But all of the SCPI commands used for
suspend/resume must go through ATF via PSCI.

> So I'm not really sure that we should do it statically this way, and
> that we should do it at all.

Do you have a better way to model "firmware uses this clock behind the scenes,
so Linux please don't touch it"? It's unfortunate that we have Linux and
firmware fighting over the R_CCU, but since we didn't have firmware (e.g. SCPI
clocks) in the beginning, it's where we are today.

The AR100 clock doesn't actually have a gate, and it generally has dependencies
like R_INTC in use. So as I mentioned in the commit message, the clock will
normally be on anyway. The goal was to model the fact that there are users of
this clock that Linux doesn't/can't know about.

> Maxime

Thanks,
Samuel
Maxime Ripard Aug. 21, 2019, 12:24 p.m. UTC | #3
On Tue, Aug 20, 2019 at 08:02:55AM -0500, Samuel Holland wrote:
> On 8/20/19 2:11 AM, Maxime Ripard wrote:
> > On Mon, Aug 19, 2019 at 10:23:03PM -0500, Samuel Holland wrote:
> >> On sun8i, sun9i, and sun50i SoCs, system suspend/resume support requires
> >> firmware running on the AR100 coprocessor (the "SCP"). Such firmware can
> >> provide additional features, such as thermal monitoring and poweron/off
> >> support for boards without a PMIC.
> >>
> >> Since the AR100 may be running critical firmware, even if Linux does not
> >> know about it or directly interact with it (all requests may go through
> >> an intermediary interface such as PSCI), Linux must not turn off its
> >> clock.
>
> This paragraph here is the key. The firmware won't necessarily have a device
> tree node, and in the current design it will not, since Linux never communicates
> with it directly. All communication goes through ATF via PSCI.

Sorry, I'm a bit lost in all those ARM firmware interfaces.

I thought SCPI was supposed to be a separate interface that had
nothing to do with PSCI?

Anyway, my main concern is that I don't really want to make exceptions
to the clock handling for everyone's usecase, and this creates a
precedent (and not even a permanent one, since eventually the plan is
to have all the clock handling happening in the firmware, right?).

There's the protected-clocks property in the DT though that will
achieve the same goal. The code to deal with it is not generic at the
moment, but it could be made that way. Would patching the DT to
protect the clock you care about, when you care about protecting them,
be an option for you?

> >> At this time, such power management firmware only exists for the A64 and
> >> H5 SoCs.  However, it makes sense to take care of all CCU drivers now
> >> for consistency, and to ease the transition in the future once firmware
> >> is ported to the other SoCs.
> >>
> >> Leaving the clock running is safe even if no firmware is present, since
> >> the AR100 stays in reset by default. In most cases, the AR100 clock is
> >> kept enabled by Linux anyway, since it is the parent of all APB0 bus
> >> peripherals. This change only prevents Linux from turning off the AR100
> >> clock in the rare case that no peripherals are in use.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >
> > So I'm not really sure where you want to go with this.
> >
> > That clock is only useful where you're having a firmware running on
> > the AR100, and that firmware would have a device tree node of its own,
> > where we could list the clocks needed for the firmware to keep
> > running, if it ever runs. If the driver has not been compiled in /
> > loaded, then we don't care either.
>
> See above. I don't expect that the firmware would have a device tree node,
> because the firmware doesn't need any Linux drivers.
>
> > But more fundamentally, if we're going to use SCPI, then those clocks
> > will not be handled by that driver anyway, but by the firmware, right?
>
> In the future, we might use SCPI clocks/sensors/regulators/etc. from Linux, but
> that's not the plan at the moment. Given that it's already been two years since
> I started this project, I'm trying to limit its scope so I can get at least some
> part merged. The first step is to integrate a firmware that provides
> suspend/resume functionality only. That firmware does implement SCPI, and if the
> top-level Linux SCPI driver worked with multiple mailbox channels, it could
> query the firmware's version and fetures. But all of the SCPI commands used for
> suspend/resume must go through ATF via PSCI.

I didn't know that you could / should do that with PSCI / SCPI.

> > So I'm not really sure that we should do it statically this way, and
> > that we should do it at all.
>
> Do you have a better way to model "firmware uses this clock behind the scenes,
> so Linux please don't touch it"? It's unfortunate that we have Linux and
> firmware fighting over the R_CCU, but since we didn't have firmware (e.g. SCPI
> clocks) in the beginning, it's where we are today.
>
> The AR100 clock doesn't actually have a gate, and it generally has dependencies
> like R_INTC in use. So as I mentioned in the commit message, the clock will
> normally be on anyway. The goal was to model the fact that there are users of
> this clock that Linux doesn't/can't know about.

Like I said, if that's an option, I'd prefer to have protected-clocks
work for everyone / for sunxi.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Stephen Boyd Sept. 5, 2019, 6:56 p.m. UTC | #4
Quoting Maxime Ripard (2019-08-21 05:24:36)
> On Tue, Aug 20, 2019 at 08:02:55AM -0500, Samuel Holland wrote:
> > On 8/20/19 2:11 AM, Maxime Ripard wrote:
> > > So I'm not really sure that we should do it statically this way, and
> > > that we should do it at all.
> >
> > Do you have a better way to model "firmware uses this clock behind the scenes,
> > so Linux please don't touch it"? It's unfortunate that we have Linux and
> > firmware fighting over the R_CCU, but since we didn't have firmware (e.g. SCPI
> > clocks) in the beginning, it's where we are today.
> >
> > The AR100 clock doesn't actually have a gate, and it generally has dependencies
> > like R_INTC in use. So as I mentioned in the commit message, the clock will
> > normally be on anyway. The goal was to model the fact that there are users of
> > this clock that Linux doesn't/can't know about.
> 
> Like I said, if that's an option, I'd prefer to have protected-clocks
> work for everyone / for sunxi.
> 

Yes. Use protected-clocks to indicate what shouldn't be touched by the
kernel. It's not super easy to make it "generic" right now, but I
suppose we can work the flag into the core framework more so that we
still register the clks but otherwise make the 'clk_get()' operation
fail on them somehow and the disable unused operation skip them. I just
took the easy way out for qcom for the time being and didn't register
them from the driver.

Patch
diff mbox series

diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
index 45a1ed3fe674..adf907020951 100644
--- a/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
+++ b/drivers/clk/sunxi-ng/ccu-sun50i-h6-r.c
@@ -45,7 +45,7 @@  static struct ccu_div ar100_clk = {
 		.hw.init	= CLK_HW_INIT_PARENTS("ar100",
 						      ar100_r_apb2_parents,
 						      &ccu_div_ops,
-						      0),
+						      CLK_IS_CRITICAL),
 	},
 };
 
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r.c b/drivers/clk/sunxi-ng/ccu-sun8i-r.c
index 4646fdc61053..feef4f750943 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-r.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-r.c
@@ -45,7 +45,7 @@  static struct ccu_div ar100_clk = {
 		.hw.init	= CLK_HW_INIT_PARENTS_DATA("ar100",
 							   ar100_parents,
 							   &ccu_div_ops,
-							   0),
+							   CLK_IS_CRITICAL),
 	},
 };