On Wed, Mar 14, 2018 at 3:22 AM, Marc Zyngier [off-list ref] wrote:

On 02/03/18 02:08, dbasehore . wrote:

quoted

On Thu, Mar 1, 2018 at 4:29 AM, Marc Zyngier [off-list ref] wrote:

quoted

Hi Mark,

On 01/03/18 11:41, Mark Rutland wrote:

quoted

On Wed, Feb 28, 2018 at 09:48:18PM -0800, Derek Basehore wrote:

quoted

Some platforms power off GIC logic in suspend, so we need to
save/restore state. The distributor and redistributor registers need
to be handled in platform code due to access permissions on those
registers, but the ITS registers can be restored in the kernel.

Signed-off-by: Derek Basehore <redacted>

How much state do we have to save/restore?

Given we can apparently read all this state, couldn't we *always* save
the state, then upon resume detect if the state has been lost, restoring
it if so?

That way, we don't need a property in FW tables for DT or ACPI.

That's a good point. I guess that we could just compare the saved
GITS_CTLR register and restore the full state only if the ITS comes back
as disabled.

I'm just a bit worried that it makes it an implicit convention between
kernel an FW, which could change in funny ways. Importantly, the PSCI
spec says states FW should restore *the whole state*. Obviously, it
cannot to that on HW that doesn't allow you to read out the state, hence
the DT flag that outlines the departure from the expected behaviour.

I'm happy to go either way, but then I have the feeling that we should
go back to quirking it on the actual implementation (GIC500 in this
case) if we're to from the property.

quoted

[...]

quoted

@@ -3261,6 +3363,9 @@ static int __init its_probe_one(struct resource *res,
             ctlr |= GITS_CTLR_ImDe;
     writel_relaxed(ctlr, its->base + GITS_CTLR);

+    if (fwnode_property_present(handle, "reset-on-suspend"))
+            its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;

Does this allow this property on an ACPI system?

If we need this on ACPI, we need a spec update to handle this properly,
and shouldn't use device properties like this.

Well spotted. I guess that dropping the property would fix that
altogether, assuming we feel that the above is safe enough.

Thoughts?

I'm fine changing it to get rid of the devicetree property.

What's the reason for quirking the behavior though? It's not that much
code + data and nothing else relies on the state of the ITS getting
disabled across suspend/resume. Even if something did, we'd have to
resolve it with this feature anyways.

The reason we do this is to cope with GIC500 having the collection state
in registers instead of memory. If we didn't have this extraordinary
misfeature, FW could do a full save/restore of the ITS, and we wouldn't
have to do anything (which is what the driver currently expects).

A middle ground approach is to limit the feature to systems where
GITS_TYPER.HCC is non-zero instead of limiting it to GIC500. Pretty easy
to fix. This should have the same effect, as GIC500 is the only
implementation I'm aware of with HCC!=0.

Given that we're already at -rc5 and that I'd like to queue things for
4.17, I've made this change myself and queued patches 1 and 3 here[1].

Can you please have a look at let me know if that works for you?

Assuming that your fine with only having the GIC500 implementations
that have HCC as non-zero getting ITS registers restored in the
kernel. As far as I can tell, this can happen in firmware for all
implementations. It's only the code to resend that MAPC on resume that
needs to be in the kernel.

Thanks,

        M.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
irq/irqchip-next
--
Jazz is not dead. It just smells funny...