From mboxrd@z Thu Jan 1 00:00:00 1970 From: "dbasehore ." Subject: Re: [PATCH v4 2/5] irqchip/gic-v3-its: add ability to save/restore ITS state Date: Mon, 5 Feb 2018 17:01:15 -0800 Message-ID: References: <20180203012450.18378-1-dbasehore@chromium.org> <20180203012450.18378-3-dbasehore@chromium.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Marc Zyngier Cc: linux-kernel , Soby Mathew , Sudeep Holla , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, Mark Rutland , Linux-pm mailing list , "Wysocki, Rafael J" , Thomas Gleixner , Brian Norris List-Id: devicetree@vger.kernel.org On Mon, Feb 5, 2018 at 4:33 PM, dbasehore . wrote: > On Mon, Feb 5, 2018 at 7:56 AM, Marc Zyngier wrote= : >> On 03/02/18 01:24, Derek Basehore wrote: >>> 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 >>> --- >>> drivers/irqchip/irq-gic-v3-its.c | 101 +++++++++++++++++++++++++++++++= ++++++++ >>> 1 file changed, 101 insertions(+) >>> >>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic= -v3-its.c >>> index 06f025fd5726..e13515cdb68f 100644 >>> --- a/drivers/irqchip/irq-gic-v3-its.c >>> +++ b/drivers/irqchip/irq-gic-v3-its.c >>> @@ -33,6 +33,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include >>> #include >>> @@ -46,6 +47,7 @@ >>> #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING (1ULL << 0) >>> #define ITS_FLAGS_WORKAROUND_CAVIUM_22375 (1ULL << 1) >>> #define ITS_FLAGS_WORKAROUND_CAVIUM_23144 (1ULL << 2) >>> +#define ITS_FLAGS_SAVE_SUSPEND_STATE (1ULL << 3) >>> >>> #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0) >>> >>> @@ -83,6 +85,15 @@ struct its_baser { >>> u32 psz; >>> }; >>> >>> +/* >>> + * Saved ITS state - this is where saved state for the ITS is stored >>> + * when it's disabled during system suspend. >>> + */ >>> +struct its_ctx { >>> + u64 cbaser; >>> + u32 ctlr; >>> +}; >> >> Nit: This is pretty small for the ITS context. Given that its_node is >> the context, you can safely expand this in the its_node structure. >> >>> + >>> struct its_device; >>> >>> /* >>> @@ -101,6 +112,7 @@ struct its_node { >>> struct its_collection *collections; >>> struct fwnode_handle *fwnode_handle; >>> u64 (*get_msi_base)(struct its_device *its_de= v); >>> + struct its_ctx its_ctx; >>> struct list_head its_device_list; >>> u64 flags; >>> unsigned long list_nr; >>> @@ -3042,6 +3054,90 @@ static void its_enable_quirks(struct its_node *i= ts) >>> gic_enable_quirks(iidr, its_quirks, its); >>> } >>> >>> +static int its_save_disable(void) >>> +{ >>> + struct its_node *its; >>> + int err =3D 0; >>> + >>> + spin_lock(&its_lock); >>> + list_for_each_entry(its, &its_nodes, entry) { >>> + struct its_ctx *ctx; >>> + void __iomem *base; >>> + >>> + if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE)) >>> + continue; >>> + >>> + ctx =3D &its->its_ctx; >>> + base =3D its->base; >>> + ctx->ctlr =3D readl_relaxed(base + GITS_CTLR); >>> + err =3D its_force_quiescent(base); >>> + if (err) { >>> + pr_err("ITS failed to quiesce\n"); >>> + writel_relaxed(ctx->ctlr, base + GITS_CTLR); >>> + goto err; >>> + } >>> + >>> + ctx->cbaser =3D gits_read_cbaser(base + GITS_CBASER); >>> + } >>> + >>> +err: >>> + if (err) { >>> + list_for_each_entry_continue_reverse(its, &its_nodes, ent= ry) { >>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) { >>> + struct its_ctx *ctx =3D &its->its_ctx; >>> + void __iomem *base =3D its->base; >>> + >>> + writel_relaxed(ctx->ctlr, base + GITS_CTL= R); >>> + } >>> + } >>> + } >>> + >>> + spin_unlock(&its_lock); >>> + >>> + return err; >>> +} >>> + >>> +static void its_restore_enable(void) >>> +{ >>> + struct its_node *its; >>> + >>> + spin_lock(&its_lock); >>> + list_for_each_entry(its, &its_nodes, entry) { >>> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) { >>> + struct its_ctx *ctx =3D &its->its_ctx; >>> + void __iomem *base =3D its->base; >>> + /* >>> + * Only the lower 32 bits matter here since the u= pper 32 >>> + * don't include any of the offset. >>> + */ >>> + u32 creader =3D readl_relaxed(base + GITS_CREADR)= ; >> >> Accessor matching gits_write_cwriter and co? >> >>> + int i; >>> + >>> + /* >>> + * Reset the write location to where the ITS is >>> + * currently at. >>> + */ >>> + gits_write_cbaser(ctx->cbaser, base + GITS_CBASER= ); >>> + gits_write_cwriter(creader, base + GITS_CWRITER); >>> + its->cmd_write =3D &its->cmd_base[ >>> + creader / sizeof(struct its_cmd_block)]; >> >> Nit: please do not split lines like this, this is unreadable. We both >> have screens that are wide enough for this to fit on a single line. >> >> More importantly: Why isn't it sufficient to reset both CREADR and >> CWRITER to zero? Is there any case where you can suspend whilst having >> anything in flight? >> >>> + /* Restore GITS_BASER from the value cache. */ >>> + for (i =3D 0; i < GITS_BASER_NR_REGS; i++) { >>> + struct its_baser *baser =3D &its->tables[= i]; >>> + >>> + its_write_baser(its, baser, baser->val); >> >> You may want to first test that this BASER register is actually >> requiring something before writing to it. Yes, this is normally safe. >> But HW is also normally broken. >> > > Anything specific to test? I see that I can check if the BASER > register has the valid bit set and also that the type is not none. > Okay, I found this snippet in the GICv3 Specification: No memory is allocated for the translation table. The ITS discards any writes to the interrupt translation page when either: =E2=80=A2 GITS_BASER.Type specifies any valid table entry type other tha= n interrupt collections, that is, any value other than 100. =E2=80=A2 GITS_BASER.Type specifies an interrupt collection and GITS_TYPER.HCC =3D=3D 0. I'll turn this into a test in code and only write the BASER if that passes. >>> + } >>> + writel_relaxed(ctx->ctlr, base + GITS_CTLR); >> >> Before restoring all of this, shouldn't you first test that the ITS is >> actually in a disabled state? >> >>> + } >>> + } >>> + spin_unlock(&its_lock); >>> +} >>> + >>> +static struct syscore_ops its_syscore_ops =3D { >>> + .suspend =3D its_save_disable, >>> + .resume =3D its_restore_enable, >>> +}; >>> + >>> static int its_init_domain(struct fwnode_handle *handle, struct its_no= de *its) >>> { >>> struct irq_domain *inner_domain; >>> @@ -3261,6 +3357,9 @@ static int __init its_probe_one(struct resource *= res, >>> ctlr |=3D GITS_CTLR_ImDe; >>> writel_relaxed(ctlr, its->base + GITS_CTLR); >>> >>> + if (fwnode_property_present(handle, "reset-on-suspend")) >>> + its->flags |=3D ITS_FLAGS_SAVE_SUSPEND_STATE; >>> + >>> err =3D its_init_domain(handle, its); >>> if (err) >>> goto out_free_tables; >>> @@ -3515,5 +3614,7 @@ int __init its_init(struct fwnode_handle *handle,= struct rdists *rdists, >>> } >>> } >>> >>> + register_syscore_ops(&its_syscore_ops); >>> + >>> return 0; >>> } >>> >> >> Thanks, >> >> M. >> -- >> Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html