From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E6637C432BE for ; Fri, 27 Aug 2021 16:25:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id BB9B460EE4 for ; Fri, 27 Aug 2021 16:25:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231944AbhH0QZv (ORCPT ); Fri, 27 Aug 2021 12:25:51 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47132 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231318AbhH0QZu (ORCPT ); Fri, 27 Aug 2021 12:25:50 -0400 Received: from mail-pl1-x629.google.com (mail-pl1-x629.google.com [IPv6:2607:f8b0:4864:20::629]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 71E5DC0613D9 for ; Fri, 27 Aug 2021 09:25:01 -0700 (PDT) Received: by mail-pl1-x629.google.com with SMTP id q21so4260957plq.3 for ; Fri, 27 Aug 2021 09:25:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=olUZYI52FUW5gVkrVAMR5duictYeHKMMzQr6nXakedQ=; b=aNaHRKsiD+SM+Lu9zvZtUAsPhN72aJZEO5ggEUbd+9i2E2zx+tYCvMVsH57GRkwHdf TEaqxgEcVqpk2OQ9H7VyX1+9BLBU1szPxLHX49NuTsQbgdwMV28fkCyIXIQW5Xd+QD3R WQ8/mhACDBZ1RuZyFTGtQQEZWkuERfp+C3Hd1pWoNSnZkvtdCCCdX43x97hoWme9ZdEO 45oFzDbvZXtq0iWx7LSdO55FY2oYiiyByKOWYg3IoUUbV30sBk3aaucbioyl2Cd8PO6k bo+KZEykhRVe2BTh/XS6P2KQyoHNyAxEkUHVXaf1/Vmq1vtUC/5rzUkbwFKRZBjsF7bZ 26nA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=olUZYI52FUW5gVkrVAMR5duictYeHKMMzQr6nXakedQ=; b=YptALpCptRDu08co8UBqdTAD4vr6WQFcp2VrMHpzIdAhN+s7SgQD1CpKIqWt3EW8RI gCm9mTe8u+jn0zBMIh2pzov/h63zbignQR5fwwhTyfyNoTDVM1+oULwJtpL46vWA2/by pqqxGE4Q7O3WRar2PEI5If50lJV7YxmH/FRrPCChsCqMDqZdgYh55YWUjHxW+GSGbIyP P8ogn3DdykUbSNAb2VeB+fE4/HW5ERhyiNnwVzEcqT4na46oWBnxHe2S+VrpkhvJcN8o 5Q5bBwIUAyS9B2i1e83UPZnKsW9rUEMn2MpydBA832eXLAQJdRvLB/j9ENN7gq6gXfNZ 90DA== X-Gm-Message-State: AOAM532R8Oc/A99d+XfBLGQ22MhwQM7bqFEnsJxqX06ggPyHqVGUJnOM +X0vVfQ8snxD2rHI585eQWs7kmSJv8cIl/AQcpT+ug== X-Google-Smtp-Source: ABdhPJy9Fys2MKe+4NKONrWKg51ffmdsBaOmd1A6Uev8PSTHXgB2mLpKO7Fg2ktC2vZ1DnSMGf/U9PYBzR/IUsq76cs= X-Received: by 2002:a17:90a:708c:: with SMTP id g12mr24462608pjk.13.1630081500678; Fri, 27 Aug 2021 09:25:00 -0700 (PDT) MIME-Version: 1.0 References: <20210803113134.2262882-1-iwona.winiarska@intel.com> <20210803113134.2262882-8-iwona.winiarska@intel.com> In-Reply-To: From: Dan Williams Date: Fri, 27 Aug 2021 09:24:49 -0700 Message-ID: Subject: Re: [PATCH v2 07/15] peci: Add peci-aspeed controller driver To: "Winiarska, Iwona" Cc: "corbet@lwn.net" , "jae.hyun.yoo@linux.intel.com" , "d.mueller@elsoft.ch" , "linux-hwmon@vger.kernel.org" , "andrew@aj.id.au" , "Luck, Tony" , "Lutomirski, Andy" , "andriy.shevchenko@linux.intel.com" , "mchehab@kernel.org" , "jdelvare@suse.com" , "linux-kernel@vger.kernel.org" , "olof@lixom.net" , "mingo@redhat.com" , "rdunlap@infradead.org" , "devicetree@vger.kernel.org" , "tglx@linutronix.de" , "linux-aspeed@lists.ozlabs.org" , "arnd@arndb.de" , "linux-doc@vger.kernel.org" , "linux@roeck-us.net" , "zweiss@equinix.com" , "robh+dt@kernel.org" , "openbmc@lists.ozlabs.org" , "gregkh@linuxfoundation.org" , "joel@jms.id.au" , "yazen.ghannam@amd.com" , "linux-arm-kernel@lists.infradead.org" , "pierre-louis.bossart@linux.intel.com" , "x86@kernel.org" , "bp@alien8.de" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-hwmon@vger.kernel.org On Thu, Aug 26, 2021 at 4:55 PM Winiarska, Iwona wrote: > > On Wed, 2021-08-25 at 18:35 -0700, Dan Williams wrote: > > On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska > > wrote: > > > > > > From: Jae Hyun Yoo > > > > > > ASPEED AST24xx/AST25xx/AST26xx SoCs supports the PECI electrical > > > interface (a.k.a PECI wire). > > > > Maybe a one sentence blurb here and in the Kconfig reminding people > > why they should care if they have a PECI driver or not? > > Ok, I'll expand it a bit. [..] > > > +static int aspeed_peci_xfer(struct peci_controller *controller, > > > + u8 addr, struct peci_request *req) > > > +{ > > > + struct aspeed_peci *priv = dev_get_drvdata(controller->dev.parent); > > > + unsigned long flags, timeout = msecs_to_jiffies(priv- > > > >cmd_timeout_ms); > > > + u32 peci_head; > > > + int ret; > > > + > > > + if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX || > > > + req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX) > > > + return -EINVAL; > > > + > > > + /* Check command sts and bus idle state */ > > > + ret = aspeed_peci_check_idle(priv); > > > + if (ret) > > > + return ret; /* -ETIMEDOUT */ > > > + > > > + spin_lock_irqsave(&priv->lock, flags); > > > + reinit_completion(&priv->xfer_complete); > > > + > > > + peci_head = FIELD_PREP(ASPEED_PECI_TARGET_ADDR_MASK, addr) | > > > + FIELD_PREP(ASPEED_PECI_WR_LEN_MASK, req->tx.len) | > > > + FIELD_PREP(ASPEED_PECI_RD_LEN_MASK, req->rx.len); > > > + > > > + writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH); > > > + > > > + memcpy_toio(priv->base + ASPEED_PECI_WR_DATA0, req->tx.buf, > > > min_t(u8, req->tx.len, 16)); > > > + if (req->tx.len > 16) > > > + memcpy_toio(priv->base + ASPEED_PECI_WR_DATA4, req->tx.buf + > > > 16, > > > + req->tx.len - 16); > > > + > > > + dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head); > > > + print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req- > > > >tx.len); > > > > On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of > > reading through this buffer, but skip emitting it. Are you sure you > > want to pay that overhead for every transaction? > > I can remove it or I can add something like: > > #if IS_ENABLED(CONFIG_PECI_DEBUG) > #define peci_debug(fmt, ...) pr_debug(fmt, ##__VA_ARGS__) > #else > #define peci_debug(...) do { } while (0) > #endif It's the hex dump I'm worried about, not the debug statements as much. I think the choices are remove the print_hex_dump_bytes(), put it behind an IS_ENABLED(CONFIG_DYNAMIC_DEBUG) to ensure the overhead is skipped in the CONFIG_DYNAMIC_DEBUG=n case, or live with the overhead if this is not a fast path / infrequently used. > > (and similar peci_trace with trace_printk for usage in IRQ handlers and such). > > What do you think? In general, no, don't wrap the base level print routines with driver-specific ones. Also, trace_printk() is only for debug builds. Note that trace points are built to be even less overhead than dev_dbg(), so there's no overhead concern with disabled tracepoints, they literally translate to nops when disabled. > > > > > > + > > > + priv->status = 0; > > > + writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD); > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + > > > + ret = wait_for_completion_interruptible_timeout(&priv- > > > >xfer_complete, timeout); > > > > spin_lock_irqsave() says "I don't know if interrupts are disabled > > already, so I'll save the state, whatever it is, and restore later" > > > > wait_for_completion_interruptible_timeout() says "I know I am in a > > sleepable context where interrupts are enabled" > > > > So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()? > > You're right - I'll fix it. > > > > > > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (ret == 0) { > > > + dev_dbg(priv->dev, "Timeout waiting for a response!\n"); > > > + return -ETIMEDOUT; > > > + } > > > + > > > + spin_lock_irqsave(&priv->lock, flags); > > > + > > > + writel(0, priv->base + ASPEED_PECI_CMD); > > > + > > > + if (priv->status != ASPEED_PECI_INT_CMD_DONE) { > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + dev_dbg(priv->dev, "No valid response!\n"); > > > + return -EIO; > > > + } > > > + > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + > > > + memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_RD_DATA0, > > > min_t(u8, req->rx.len, 16)); > > > + if (req->rx.len > 16) > > > + memcpy_fromio(req->rx.buf + 16, priv->base + > > > ASPEED_PECI_RD_DATA4, > > > + req->rx.len - 16); > > > + > > > + print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req- > > > >rx.len); > > > + > > > + return 0; > > > +} > > > + > > > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg) > > > +{ > > > + struct aspeed_peci *priv = arg; > > > + u32 status; > > > + > > > + spin_lock(&priv->lock); > > > + status = readl(priv->base + ASPEED_PECI_INT_STS); > > > + writel(status, priv->base + ASPEED_PECI_INT_STS); > > > + priv->status |= (status & ASPEED_PECI_INT_MASK); > > > + > > > + /* > > > + * In most cases, interrupt bits will be set one by one but also > > > note > > > + * that multiple interrupt bits could be set at the same time. > > > + */ > > > + if (status & ASPEED_PECI_INT_BUS_TIMEOUT) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_BUS_TIMEOUT\n"); > > > + > > > + if (status & ASPEED_PECI_INT_BUS_CONTENTION) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_BUS_CONTENTION\n"); > > > + > > > + if (status & ASPEED_PECI_INT_WR_FCS_BAD) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_WR_FCS_BAD\n"); > > > + > > > + if (status & ASPEED_PECI_INT_WR_FCS_ABORT) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_WR_FCS_ABORT\n"); > > > > Are you sure these would not be better as tracepoints? If you're > > debugging an interrupt related failure, the ratelimiting might get in > > your way when you really need to know when one of these error > > interrupts fire relative to another event. > > Tracepoints are ABI(ish), and using a full blown tracepoint just for IRQ status > would probably be too much. Tracepoints become ABI once someone ships tooling that depends on them being there. These don't look attractive for a tool, and they don't look difficult to maintain if the interrupt handler needs to be reworked. I.e. it would be trivial to keep a dead tracepoint around if worse came to worse to keep a tool from failing to load. > I was thinking about something like trace_printk hidden under a > "CONFIG_PECI_DEBUG" (see above), but perhaps that's something for the future > improvement? Again trace_printk() is only for private builds. > > > > > > + > > > + /* > > > + * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE > > > bit > > > + * set even in an error case. > > > + */ > > > + if (status & ASPEED_PECI_INT_CMD_DONE) > > > + complete(&priv->xfer_complete); > > > > Hmm, no need to check if there was a sequencing error, like a command > > was never submitted? > > It's handled by checking if HW is idle in xfer before a command is sent, where > we just expect a single interrupt per command. I'm asking how do you determine if this status was spurious, or there was a sequencing error in the driver? From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9C1E0C432BE for ; Fri, 27 Aug 2021 16:27:48 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 6454E60E99 for ; Fri, 27 Aug 2021 16:27:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 6454E60E99 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=nHq1HyIWHlySu70cYRFi8k12vL/nLnwBdtBRMRBXhR0=; b=38XcW/kwMp7t4f jN36RGdsROGv+o5va0BRxXuRAdInHP7YS0y8TI0OFzMB+6k8feI0KN+NhIVVtFWy6E02I1ojFA6KB VmMH5MFXvhA4q+pQzLu5yNriFbX7KRA1vmNBqed/+6NGuWp4svOlN+RKdcl/6lppd/LvfVpKYnjAw imggK0/nPzdr5ALp7te2WPOROO21hKFyIO9FZeZkhRgEn/7o9mnPzIEXxOL2kP4lNoQkNYEhYVAEs 7XuUNl6M2j35NPUq51s/VgaENJBTs+oZ+G0C0fBAaK/YGsM3TAMF+3MbCcn5GEOKOjoZCq3JW8SAc Mut5xIht528aQGiUZBpQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mJefC-00Ck6U-NF; Fri, 27 Aug 2021 16:25:06 +0000 Received: from mail-pl1-x62f.google.com ([2607:f8b0:4864:20::62f]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mJef8-00Ck4W-DO for linux-arm-kernel@lists.infradead.org; Fri, 27 Aug 2021 16:25:04 +0000 Received: by mail-pl1-x62f.google.com with SMTP id m4so4287694pll.0 for ; Fri, 27 Aug 2021 09:25:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=olUZYI52FUW5gVkrVAMR5duictYeHKMMzQr6nXakedQ=; b=aNaHRKsiD+SM+Lu9zvZtUAsPhN72aJZEO5ggEUbd+9i2E2zx+tYCvMVsH57GRkwHdf TEaqxgEcVqpk2OQ9H7VyX1+9BLBU1szPxLHX49NuTsQbgdwMV28fkCyIXIQW5Xd+QD3R WQ8/mhACDBZ1RuZyFTGtQQEZWkuERfp+C3Hd1pWoNSnZkvtdCCCdX43x97hoWme9ZdEO 45oFzDbvZXtq0iWx7LSdO55FY2oYiiyByKOWYg3IoUUbV30sBk3aaucbioyl2Cd8PO6k bo+KZEykhRVe2BTh/XS6P2KQyoHNyAxEkUHVXaf1/Vmq1vtUC/5rzUkbwFKRZBjsF7bZ 26nA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=olUZYI52FUW5gVkrVAMR5duictYeHKMMzQr6nXakedQ=; b=K1o6Lb1++wN134YBeJYWnLViqPGOf2/BFZrYGubST7T5ouvUFPW3mbcDoahANFhUZt NEgQX0H/fTfiTpFjd1XjVe6ms4zBDMappBHsWuYEL0sP3YQIQc4SEmhh6wcLv1C1jeJV KA2m08gYm/aHUN7XHi3eWxhD7tZyHJe7bGObCJZDAhkSJtqgonPgOUSPeQJoRRxUZPV2 vs/2H4ef3FRIlSyphGBNGR8pza5e+kOgFOMyJIw+LfhL7PWoZ7fiUWgiFtBLEufnfaHU aoPAj7JAQUPIfqRHtnbiUM80wU+swvVom4KQue76SX0vw1idB1cgc5yN6191C408/LbW +Swg== X-Gm-Message-State: AOAM530FU9b9FTkV0jL0UfnbjyxbHMKRGmpC6PkTzgCwpc+ola3VGRrN gd61nW2q9lvDQi5R/LUBAjTbvP/lIuil6tb3tZoFeg== X-Google-Smtp-Source: ABdhPJy9Fys2MKe+4NKONrWKg51ffmdsBaOmd1A6Uev8PSTHXgB2mLpKO7Fg2ktC2vZ1DnSMGf/U9PYBzR/IUsq76cs= X-Received: by 2002:a17:90a:708c:: with SMTP id g12mr24462608pjk.13.1630081500678; Fri, 27 Aug 2021 09:25:00 -0700 (PDT) MIME-Version: 1.0 References: <20210803113134.2262882-1-iwona.winiarska@intel.com> <20210803113134.2262882-8-iwona.winiarska@intel.com> In-Reply-To: From: Dan Williams Date: Fri, 27 Aug 2021 09:24:49 -0700 Message-ID: Subject: Re: [PATCH v2 07/15] peci: Add peci-aspeed controller driver To: "Winiarska, Iwona" Cc: "corbet@lwn.net" , "jae.hyun.yoo@linux.intel.com" , "d.mueller@elsoft.ch" , "linux-hwmon@vger.kernel.org" , "andrew@aj.id.au" , "Luck, Tony" , "Lutomirski, Andy" , "andriy.shevchenko@linux.intel.com" , "mchehab@kernel.org" , "jdelvare@suse.com" , "linux-kernel@vger.kernel.org" , "olof@lixom.net" , "mingo@redhat.com" , "rdunlap@infradead.org" , "devicetree@vger.kernel.org" , "tglx@linutronix.de" , "linux-aspeed@lists.ozlabs.org" , "arnd@arndb.de" , "linux-doc@vger.kernel.org" , "linux@roeck-us.net" , "zweiss@equinix.com" , "robh+dt@kernel.org" , "openbmc@lists.ozlabs.org" , "gregkh@linuxfoundation.org" , "joel@jms.id.au" , "yazen.ghannam@amd.com" , "linux-arm-kernel@lists.infradead.org" , "pierre-louis.bossart@linux.intel.com" , "x86@kernel.org" , "bp@alien8.de" X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210827_092502_610646_AA51A37A X-CRM114-Status: GOOD ( 45.42 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Aug 26, 2021 at 4:55 PM Winiarska, Iwona wrote: > > On Wed, 2021-08-25 at 18:35 -0700, Dan Williams wrote: > > On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska > > wrote: > > > > > > From: Jae Hyun Yoo > > > > > > ASPEED AST24xx/AST25xx/AST26xx SoCs supports the PECI electrical > > > interface (a.k.a PECI wire). > > > > Maybe a one sentence blurb here and in the Kconfig reminding people > > why they should care if they have a PECI driver or not? > > Ok, I'll expand it a bit. [..] > > > +static int aspeed_peci_xfer(struct peci_controller *controller, > > > + u8 addr, struct peci_request *req) > > > +{ > > > + struct aspeed_peci *priv = dev_get_drvdata(controller->dev.parent); > > > + unsigned long flags, timeout = msecs_to_jiffies(priv- > > > >cmd_timeout_ms); > > > + u32 peci_head; > > > + int ret; > > > + > > > + if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX || > > > + req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX) > > > + return -EINVAL; > > > + > > > + /* Check command sts and bus idle state */ > > > + ret = aspeed_peci_check_idle(priv); > > > + if (ret) > > > + return ret; /* -ETIMEDOUT */ > > > + > > > + spin_lock_irqsave(&priv->lock, flags); > > > + reinit_completion(&priv->xfer_complete); > > > + > > > + peci_head = FIELD_PREP(ASPEED_PECI_TARGET_ADDR_MASK, addr) | > > > + FIELD_PREP(ASPEED_PECI_WR_LEN_MASK, req->tx.len) | > > > + FIELD_PREP(ASPEED_PECI_RD_LEN_MASK, req->rx.len); > > > + > > > + writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH); > > > + > > > + memcpy_toio(priv->base + ASPEED_PECI_WR_DATA0, req->tx.buf, > > > min_t(u8, req->tx.len, 16)); > > > + if (req->tx.len > 16) > > > + memcpy_toio(priv->base + ASPEED_PECI_WR_DATA4, req->tx.buf + > > > 16, > > > + req->tx.len - 16); > > > + > > > + dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head); > > > + print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req- > > > >tx.len); > > > > On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of > > reading through this buffer, but skip emitting it. Are you sure you > > want to pay that overhead for every transaction? > > I can remove it or I can add something like: > > #if IS_ENABLED(CONFIG_PECI_DEBUG) > #define peci_debug(fmt, ...) pr_debug(fmt, ##__VA_ARGS__) > #else > #define peci_debug(...) do { } while (0) > #endif It's the hex dump I'm worried about, not the debug statements as much. I think the choices are remove the print_hex_dump_bytes(), put it behind an IS_ENABLED(CONFIG_DYNAMIC_DEBUG) to ensure the overhead is skipped in the CONFIG_DYNAMIC_DEBUG=n case, or live with the overhead if this is not a fast path / infrequently used. > > (and similar peci_trace with trace_printk for usage in IRQ handlers and such). > > What do you think? In general, no, don't wrap the base level print routines with driver-specific ones. Also, trace_printk() is only for debug builds. Note that trace points are built to be even less overhead than dev_dbg(), so there's no overhead concern with disabled tracepoints, they literally translate to nops when disabled. > > > > > > + > > > + priv->status = 0; > > > + writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD); > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + > > > + ret = wait_for_completion_interruptible_timeout(&priv- > > > >xfer_complete, timeout); > > > > spin_lock_irqsave() says "I don't know if interrupts are disabled > > already, so I'll save the state, whatever it is, and restore later" > > > > wait_for_completion_interruptible_timeout() says "I know I am in a > > sleepable context where interrupts are enabled" > > > > So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()? > > You're right - I'll fix it. > > > > > > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (ret == 0) { > > > + dev_dbg(priv->dev, "Timeout waiting for a response!\n"); > > > + return -ETIMEDOUT; > > > + } > > > + > > > + spin_lock_irqsave(&priv->lock, flags); > > > + > > > + writel(0, priv->base + ASPEED_PECI_CMD); > > > + > > > + if (priv->status != ASPEED_PECI_INT_CMD_DONE) { > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + dev_dbg(priv->dev, "No valid response!\n"); > > > + return -EIO; > > > + } > > > + > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + > > > + memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_RD_DATA0, > > > min_t(u8, req->rx.len, 16)); > > > + if (req->rx.len > 16) > > > + memcpy_fromio(req->rx.buf + 16, priv->base + > > > ASPEED_PECI_RD_DATA4, > > > + req->rx.len - 16); > > > + > > > + print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req- > > > >rx.len); > > > + > > > + return 0; > > > +} > > > + > > > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg) > > > +{ > > > + struct aspeed_peci *priv = arg; > > > + u32 status; > > > + > > > + spin_lock(&priv->lock); > > > + status = readl(priv->base + ASPEED_PECI_INT_STS); > > > + writel(status, priv->base + ASPEED_PECI_INT_STS); > > > + priv->status |= (status & ASPEED_PECI_INT_MASK); > > > + > > > + /* > > > + * In most cases, interrupt bits will be set one by one but also > > > note > > > + * that multiple interrupt bits could be set at the same time. > > > + */ > > > + if (status & ASPEED_PECI_INT_BUS_TIMEOUT) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_BUS_TIMEOUT\n"); > > > + > > > + if (status & ASPEED_PECI_INT_BUS_CONTENTION) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_BUS_CONTENTION\n"); > > > + > > > + if (status & ASPEED_PECI_INT_WR_FCS_BAD) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_WR_FCS_BAD\n"); > > > + > > > + if (status & ASPEED_PECI_INT_WR_FCS_ABORT) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_WR_FCS_ABORT\n"); > > > > Are you sure these would not be better as tracepoints? If you're > > debugging an interrupt related failure, the ratelimiting might get in > > your way when you really need to know when one of these error > > interrupts fire relative to another event. > > Tracepoints are ABI(ish), and using a full blown tracepoint just for IRQ status > would probably be too much. Tracepoints become ABI once someone ships tooling that depends on them being there. These don't look attractive for a tool, and they don't look difficult to maintain if the interrupt handler needs to be reworked. I.e. it would be trivial to keep a dead tracepoint around if worse came to worse to keep a tool from failing to load. > I was thinking about something like trace_printk hidden under a > "CONFIG_PECI_DEBUG" (see above), but perhaps that's something for the future > improvement? Again trace_printk() is only for private builds. > > > > > > + > > > + /* > > > + * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE > > > bit > > > + * set even in an error case. > > > + */ > > > + if (status & ASPEED_PECI_INT_CMD_DONE) > > > + complete(&priv->xfer_complete); > > > > Hmm, no need to check if there was a sequencing error, like a command > > was never submitted? > > It's handled by checking if HW is idle in xfer before a command is sent, where > we just expect a single interrupt per command. I'm asking how do you determine if this status was spurious, or there was a sequencing error in the driver? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 63D1FC432BE for ; Mon, 30 Aug 2021 01:28:31 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A065D60F5E for ; Mon, 30 Aug 2021 01:28:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org A065D60F5E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.ozlabs.org Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4GyXlJ4hplz2yLN for ; Mon, 30 Aug 2021 11:28:28 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=intel-com.20150623.gappssmtp.com header.i=@intel-com.20150623.gappssmtp.com header.a=rsa-sha256 header.s=20150623 header.b=aNaHRKsi; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=intel.com (client-ip=2607:f8b0:4864:20::62f; helo=mail-pl1-x62f.google.com; envelope-from=dan.j.williams@intel.com; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=intel-com.20150623.gappssmtp.com header.i=@intel-com.20150623.gappssmtp.com header.a=rsa-sha256 header.s=20150623 header.b=aNaHRKsi; dkim-atps=neutral Received: from mail-pl1-x62f.google.com (mail-pl1-x62f.google.com [IPv6:2607:f8b0:4864:20::62f]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4Gx4nJ6nnfz2ypP for ; Sat, 28 Aug 2021 02:25:04 +1000 (AEST) Received: by mail-pl1-x62f.google.com with SMTP id m17so4253054plc.6 for ; Fri, 27 Aug 2021 09:25:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=olUZYI52FUW5gVkrVAMR5duictYeHKMMzQr6nXakedQ=; b=aNaHRKsiD+SM+Lu9zvZtUAsPhN72aJZEO5ggEUbd+9i2E2zx+tYCvMVsH57GRkwHdf TEaqxgEcVqpk2OQ9H7VyX1+9BLBU1szPxLHX49NuTsQbgdwMV28fkCyIXIQW5Xd+QD3R WQ8/mhACDBZ1RuZyFTGtQQEZWkuERfp+C3Hd1pWoNSnZkvtdCCCdX43x97hoWme9ZdEO 45oFzDbvZXtq0iWx7LSdO55FY2oYiiyByKOWYg3IoUUbV30sBk3aaucbioyl2Cd8PO6k bo+KZEykhRVe2BTh/XS6P2KQyoHNyAxEkUHVXaf1/Vmq1vtUC/5rzUkbwFKRZBjsF7bZ 26nA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=olUZYI52FUW5gVkrVAMR5duictYeHKMMzQr6nXakedQ=; b=Llv78+wSXER52xDPoZmNKKgSMdtTHmBjhSM/hv3V4EjPN1CQQuut55oj9HViszACi0 Oo4VJnQn//59qmUEU8VsjuOB6XTPrQ90bK69hYFBSG+aOX8NpU+v4kz1K7yIhXlPCpkG Kn7GBZ7YBGehgiv1Cxd1OsO2qsdPwxkFtCWAUrsfnlmJThmW7A2piF/N+ASVYsZuzXE7 gXSPG11yfPQRhaUF+UE7Goyir/U5CcHD2o1VwriYpwhvYQbUwBH5ZFBh6yR4glEupSVS iB5YMG+3aSRMnNJCAqgf968rGEO5JyAZOYmEtUmMY99cO/7Z0QW0I7YWS8cA4uowD3e7 jOFg== X-Gm-Message-State: AOAM532Lojawaek+iXVgeCmpCscG8+AcVkOYz8o7hvn1UASj+W0SJlBA qomHwnOPtfW6wwr3mfBil4YyjMXS+vtdKuTN6FYPUQ== X-Google-Smtp-Source: ABdhPJy9Fys2MKe+4NKONrWKg51ffmdsBaOmd1A6Uev8PSTHXgB2mLpKO7Fg2ktC2vZ1DnSMGf/U9PYBzR/IUsq76cs= X-Received: by 2002:a17:90a:708c:: with SMTP id g12mr24462608pjk.13.1630081500678; Fri, 27 Aug 2021 09:25:00 -0700 (PDT) MIME-Version: 1.0 References: <20210803113134.2262882-1-iwona.winiarska@intel.com> <20210803113134.2262882-8-iwona.winiarska@intel.com> In-Reply-To: From: Dan Williams Date: Fri, 27 Aug 2021 09:24:49 -0700 Message-ID: Subject: Re: [PATCH v2 07/15] peci: Add peci-aspeed controller driver To: "Winiarska, Iwona" Content-Type: text/plain; charset="UTF-8" X-Mailman-Approved-At: Mon, 30 Aug 2021 11:27:51 +1000 X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "linux-aspeed@lists.ozlabs.org" , "linux-doc@vger.kernel.org" , "zweiss@equinix.com" , "jae.hyun.yoo@linux.intel.com" , "andriy.shevchenko@linux.intel.com" , "corbet@lwn.net" , "openbmc@lists.ozlabs.org" , "x86@kernel.org" , "pierre-louis.bossart@linux.intel.com" , "mingo@redhat.com" , "linux@roeck-us.net" , "devicetree@vger.kernel.org" , "jdelvare@suse.com" , "arnd@arndb.de" , "robh+dt@kernel.org" , "bp@alien8.de" , "Lutomirski, Andy" , "tglx@linutronix.de" , "mchehab@kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-hwmon@vger.kernel.org" , "Luck, Tony" , "andrew@aj.id.au" , "gregkh@linuxfoundation.org" , "rdunlap@infradead.org" , "linux-kernel@vger.kernel.org" , "yazen.ghannam@amd.com" , "olof@lixom.net" Errors-To: openbmc-bounces+openbmc=archiver.kernel.org@lists.ozlabs.org Sender: "openbmc" On Thu, Aug 26, 2021 at 4:55 PM Winiarska, Iwona wrote: > > On Wed, 2021-08-25 at 18:35 -0700, Dan Williams wrote: > > On Tue, Aug 3, 2021 at 4:35 AM Iwona Winiarska > > wrote: > > > > > > From: Jae Hyun Yoo > > > > > > ASPEED AST24xx/AST25xx/AST26xx SoCs supports the PECI electrical > > > interface (a.k.a PECI wire). > > > > Maybe a one sentence blurb here and in the Kconfig reminding people > > why they should care if they have a PECI driver or not? > > Ok, I'll expand it a bit. [..] > > > +static int aspeed_peci_xfer(struct peci_controller *controller, > > > + u8 addr, struct peci_request *req) > > > +{ > > > + struct aspeed_peci *priv = dev_get_drvdata(controller->dev.parent); > > > + unsigned long flags, timeout = msecs_to_jiffies(priv- > > > >cmd_timeout_ms); > > > + u32 peci_head; > > > + int ret; > > > + > > > + if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX || > > > + req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX) > > > + return -EINVAL; > > > + > > > + /* Check command sts and bus idle state */ > > > + ret = aspeed_peci_check_idle(priv); > > > + if (ret) > > > + return ret; /* -ETIMEDOUT */ > > > + > > > + spin_lock_irqsave(&priv->lock, flags); > > > + reinit_completion(&priv->xfer_complete); > > > + > > > + peci_head = FIELD_PREP(ASPEED_PECI_TARGET_ADDR_MASK, addr) | > > > + FIELD_PREP(ASPEED_PECI_WR_LEN_MASK, req->tx.len) | > > > + FIELD_PREP(ASPEED_PECI_RD_LEN_MASK, req->rx.len); > > > + > > > + writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH); > > > + > > > + memcpy_toio(priv->base + ASPEED_PECI_WR_DATA0, req->tx.buf, > > > min_t(u8, req->tx.len, 16)); > > > + if (req->tx.len > 16) > > > + memcpy_toio(priv->base + ASPEED_PECI_WR_DATA4, req->tx.buf + > > > 16, > > > + req->tx.len - 16); > > > + > > > + dev_dbg(priv->dev, "HEAD : 0x%08x\n", peci_head); > > > + print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req- > > > >tx.len); > > > > On CONFIG_DYNAMIC_DEBUG=n builds the kernel will do all the work of > > reading through this buffer, but skip emitting it. Are you sure you > > want to pay that overhead for every transaction? > > I can remove it or I can add something like: > > #if IS_ENABLED(CONFIG_PECI_DEBUG) > #define peci_debug(fmt, ...) pr_debug(fmt, ##__VA_ARGS__) > #else > #define peci_debug(...) do { } while (0) > #endif It's the hex dump I'm worried about, not the debug statements as much. I think the choices are remove the print_hex_dump_bytes(), put it behind an IS_ENABLED(CONFIG_DYNAMIC_DEBUG) to ensure the overhead is skipped in the CONFIG_DYNAMIC_DEBUG=n case, or live with the overhead if this is not a fast path / infrequently used. > > (and similar peci_trace with trace_printk for usage in IRQ handlers and such). > > What do you think? In general, no, don't wrap the base level print routines with driver-specific ones. Also, trace_printk() is only for debug builds. Note that trace points are built to be even less overhead than dev_dbg(), so there's no overhead concern with disabled tracepoints, they literally translate to nops when disabled. > > > > > > + > > > + priv->status = 0; > > > + writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD); > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + > > > + ret = wait_for_completion_interruptible_timeout(&priv- > > > >xfer_complete, timeout); > > > > spin_lock_irqsave() says "I don't know if interrupts are disabled > > already, so I'll save the state, whatever it is, and restore later" > > > > wait_for_completion_interruptible_timeout() says "I know I am in a > > sleepable context where interrupts are enabled" > > > > So, one of those is wrong, i.e. should it be spin_{lock,unlock}_irq()? > > You're right - I'll fix it. > > > > > > > > + if (ret < 0) > > > + return ret; > > > + > > > + if (ret == 0) { > > > + dev_dbg(priv->dev, "Timeout waiting for a response!\n"); > > > + return -ETIMEDOUT; > > > + } > > > + > > > + spin_lock_irqsave(&priv->lock, flags); > > > + > > > + writel(0, priv->base + ASPEED_PECI_CMD); > > > + > > > + if (priv->status != ASPEED_PECI_INT_CMD_DONE) { > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + dev_dbg(priv->dev, "No valid response!\n"); > > > + return -EIO; > > > + } > > > + > > > + spin_unlock_irqrestore(&priv->lock, flags); > > > + > > > + memcpy_fromio(req->rx.buf, priv->base + ASPEED_PECI_RD_DATA0, > > > min_t(u8, req->rx.len, 16)); > > > + if (req->rx.len > 16) > > > + memcpy_fromio(req->rx.buf + 16, priv->base + > > > ASPEED_PECI_RD_DATA4, > > > + req->rx.len - 16); > > > + > > > + print_hex_dump_bytes("RX : ", DUMP_PREFIX_NONE, req->rx.buf, req- > > > >rx.len); > > > + > > > + return 0; > > > +} > > > + > > > +static irqreturn_t aspeed_peci_irq_handler(int irq, void *arg) > > > +{ > > > + struct aspeed_peci *priv = arg; > > > + u32 status; > > > + > > > + spin_lock(&priv->lock); > > > + status = readl(priv->base + ASPEED_PECI_INT_STS); > > > + writel(status, priv->base + ASPEED_PECI_INT_STS); > > > + priv->status |= (status & ASPEED_PECI_INT_MASK); > > > + > > > + /* > > > + * In most cases, interrupt bits will be set one by one but also > > > note > > > + * that multiple interrupt bits could be set at the same time. > > > + */ > > > + if (status & ASPEED_PECI_INT_BUS_TIMEOUT) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_BUS_TIMEOUT\n"); > > > + > > > + if (status & ASPEED_PECI_INT_BUS_CONTENTION) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_BUS_CONTENTION\n"); > > > + > > > + if (status & ASPEED_PECI_INT_WR_FCS_BAD) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_WR_FCS_BAD\n"); > > > + > > > + if (status & ASPEED_PECI_INT_WR_FCS_ABORT) > > > + dev_dbg_ratelimited(priv->dev, > > > "ASPEED_PECI_INT_WR_FCS_ABORT\n"); > > > > Are you sure these would not be better as tracepoints? If you're > > debugging an interrupt related failure, the ratelimiting might get in > > your way when you really need to know when one of these error > > interrupts fire relative to another event. > > Tracepoints are ABI(ish), and using a full blown tracepoint just for IRQ status > would probably be too much. Tracepoints become ABI once someone ships tooling that depends on them being there. These don't look attractive for a tool, and they don't look difficult to maintain if the interrupt handler needs to be reworked. I.e. it would be trivial to keep a dead tracepoint around if worse came to worse to keep a tool from failing to load. > I was thinking about something like trace_printk hidden under a > "CONFIG_PECI_DEBUG" (see above), but perhaps that's something for the future > improvement? Again trace_printk() is only for private builds. > > > > > > + > > > + /* > > > + * All commands should be ended up with a ASPEED_PECI_INT_CMD_DONE > > > bit > > > + * set even in an error case. > > > + */ > > > + if (status & ASPEED_PECI_INT_CMD_DONE) > > > + complete(&priv->xfer_complete); > > > > Hmm, no need to check if there was a sequencing error, like a command > > was never submitted? > > It's handled by checking if HW is idle in xfer before a command is sent, where > we just expect a single interrupt per command. I'm asking how do you determine if this status was spurious, or there was a sequencing error in the driver?