Received: by 10.223.176.5 with SMTP id f5csp55850wra; Mon, 5 Feb 2018 16:34:26 -0800 (PST) X-Google-Smtp-Source: AH8x2247zuKHFf4KSZjn7QC3kTKpB5ip0lrS8pZdh+1NQC5R+Yb9Ga5MK6Qt3YI/LTf44mj5EHRU X-Received: by 10.98.8.206 with SMTP id 75mr583104pfi.172.1517877266146; Mon, 05 Feb 2018 16:34:26 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1517877266; cv=none; d=google.com; s=arc-20160816; b=yzX5TTOxp3n9jntcGkEFeybgRaTxgv4mH1LbkC31NV/WodwpE3S7pRmfimfwFPcir0 MnXi8yiKFbG+6y4u3EKnXj1dvgkhmLe/9vuckYTXVPZ2MZyOPzfAFNxXSRUDXMTwNiNz b/VwSCR1hULWcqIcPi0JcBo9J2ZVfyp/qoW/+VJOgwpVTS3nwZb6RqgNGJTibBg1OK7F RKFJtgfPm+EAZXnwwT5+VAacWXOSUXrgW2nrIXm5Wc8Q19UPC/+uyTklCCNqVyYOF8S3 UevAf5ppKQ7dnL1fon+EOYu0ApvJaUDEkeAaNa7+P833nFSAOqMPVLTGjOGs2sf7PZXP 41QQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:dkim-signature:dkim-signature :arc-authentication-results; bh=byJ5fg3d/9eunhHAQADzpyPgszFM6q0bi/WjA4suXBA=; b=j9ertHUSJh4tZVjiIoDbcNHnvQyKNEI9Wj7FiInwqLinHYsA441d+TmKbzo3YJhv0K jY+SQAe5m7iHeBPgvLF3aj5rEnDblLB1uiEjiGmKgV5YgIShKuF39aczVUwxAAUE96WM Qw1aNiLuzQI4cTvEBUXx3wqaVc5j0SMEDnOMk700s8mZ8HC6T4TZquZSOh08wl0NWxjx 344Bl7qGM56RXu7cj9Y9/ocGiTaMPibXNQlxt9EpQlXm+iL3cXbYKsPv1TgVGVZTlJux EG+QZpHyHKBuPDd0HFsvVcXNCl/NAu/nq2Z/qNFrmoBoWQRYn0BfZ9l9baYqvjoC7SsM KaDA== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@google.com header.s=20161025 header.b=JYtGvO+y; dkim=fail header.i=@chromium.org header.s=google header.b=H4As0pdn; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=chromium.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id m2si739186pfh.158.2018.02.05.16.34.12; Mon, 05 Feb 2018 16:34:26 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=fail header.i=@google.com header.s=20161025 header.b=JYtGvO+y; dkim=fail header.i=@chromium.org header.s=google header.b=H4As0pdn; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752207AbeBFAdn (ORCPT + 99 others); Mon, 5 Feb 2018 19:33:43 -0500 Received: from mail-wr0-f193.google.com ([209.85.128.193]:40446 "EHLO mail-wr0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751878AbeBFAdh (ORCPT ); Mon, 5 Feb 2018 19:33:37 -0500 Received: by mail-wr0-f193.google.com with SMTP id i56so160681wra.7 for ; Mon, 05 Feb 2018 16:33:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=byJ5fg3d/9eunhHAQADzpyPgszFM6q0bi/WjA4suXBA=; b=JYtGvO+yb3m5Yx/s+zY7a0VB6S+BioZH8r/MGmVqvtcUbaApfRFBiXhFGTd3EdNDmZ JNef+yGGgVkv6rqptpC+WDh5bKQRLyq05fo8UIO5a+kjzTAc7KA4OJ5vfyh2bT3fCDx6 UbJohVUYxcSLPeYJD97rpn6tbpnHAt9OWcQ+ayaV3J1AcQYIigTpmpmPLkz8EZsCiLKh AD0e/YjD9KisyP9JR1jQZg3vltvlNcvR2oebvi3P5UNd8YdTC3VQ1evTAeTIpnib7BWp OdswZHBba/uj1kc3Qq41FmrcZG2ICIC9+BA+whIeJH+lhXj48fyUxkTmhTn3MybWkT6d 3J9w== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=byJ5fg3d/9eunhHAQADzpyPgszFM6q0bi/WjA4suXBA=; b=H4As0pdnTMnZZacBekZZc1r19wIWCsPxvBR6W7khm2dS3FbCUjjR4eTGeZab5vb/ro qRVhVxM1QrpGW3HouY+RQ01iWcuTSDjZ6x9YAtuPH2GnpSB59PMGKzLX0duGrcg7G4hp r999TgZid77OuWN2KGCCWKNp/LyMQ97rXSjt8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=byJ5fg3d/9eunhHAQADzpyPgszFM6q0bi/WjA4suXBA=; b=P0I0jXHYeR/v/JVMksh+A3xbdhf9cMO9nkn8EMQ/Gf1IDIpEHFBhWSuiB+wXNFQu5n 6ZNaQGL0Svxm+ND9DRO5k1mAszQjbKYpVg71SA4rQf2TmGO2PHzhdDwl+9bYIAA2MBEs YHQjVuQ4iusHvTvuv05EVDQgXVcSv98DPPTT+V0PTbeV/onNm1xSKa5/cb01yQ2sdRFe vqzxjaMvuoLvqEBjrp/KvCSzXLXj3WsVoApocIgepa5bqSz5n7rTbwU53H3MRJCAofP3 y5IfCV463S9kHcUN4rJ3/khQRSv28rVzdMCOr99s6oj1hvDsoNHMJGkccubY/dxvH2iS uvIw== X-Gm-Message-State: APf1xPAIeqxP1od4g14LevE68GN2+mNA1jK1mdFAN37O6JwChPFXh45w T10G4u1IeSgWeK7sBsHFA/EVWVAli9h9AJ167eyClg== X-Received: by 10.223.157.135 with SMTP id p7mr520291wre.34.1517877216146; Mon, 05 Feb 2018 16:33:36 -0800 (PST) MIME-Version: 1.0 Received: by 10.28.124.6 with HTTP; Mon, 5 Feb 2018 16:33:35 -0800 (PST) In-Reply-To: References: <20180203012450.18378-1-dbasehore@chromium.org> <20180203012450.18378-3-dbasehore@chromium.org> From: "dbasehore ." Date: Mon, 5 Feb 2018 16:33:35 -0800 X-Google-Sender-Auth: OqmkqG0tnKVC29CPo5vnDDd4vQ4 Message-ID: Subject: Re: [PATCH v4 2/5] irqchip/gic-v3-its: add ability to save/restore ITS state To: Marc Zyngier Cc: linux-kernel , Soby Mathew , Sudeep Holla , devicetree@vger.kernel.org, robh+dt@kernel.org, Mark Rutland , Linux-pm mailing list , "Wysocki, Rafael J" , Thomas Gleixner , Brian Norris Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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_dev); >> + 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 *its) >> gic_enable_quirks(iidr, its_quirks, its); >> } >> >> +static int its_save_disable(void) >> +{ >> + struct its_node *its; >> + int err = 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 = &its->its_ctx; >> + base = its->base; >> + ctx->ctlr = readl_relaxed(base + GITS_CTLR); >> + err = its_force_quiescent(base); >> + if (err) { >> + pr_err("ITS failed to quiesce\n"); >> + writel_relaxed(ctx->ctlr, base + GITS_CTLR); >> + goto err; >> + } >> + >> + ctx->cbaser = gits_read_cbaser(base + GITS_CBASER); >> + } >> + >> +err: >> + if (err) { >> + list_for_each_entry_continue_reverse(its, &its_nodes, entry) { >> + if (its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE) { >> + struct its_ctx *ctx = &its->its_ctx; >> + void __iomem *base = its->base; >> + >> + writel_relaxed(ctx->ctlr, base + GITS_CTLR); >> + } >> + } >> + } >> + >> + 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 = &its->its_ctx; >> + void __iomem *base = its->base; >> + /* >> + * Only the lower 32 bits matter here since the upper 32 >> + * don't include any of the offset. >> + */ >> + u32 creader = 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 = &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 = 0; i < GITS_BASER_NR_REGS; i++) { >> + struct its_baser *baser = &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. >> + } >> + 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 = { >> + .suspend = its_save_disable, >> + .resume = its_restore_enable, >> +}; >> + >> static int its_init_domain(struct fwnode_handle *handle, struct its_node *its) >> { >> struct irq_domain *inner_domain; >> @@ -3261,6 +3357,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; >> + >> err = 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...