From patchwork Tue Jun 16 10:40:46 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Anderson X-Patchwork-Id: 11607145 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id E2C9214DD for ; Tue, 16 Jun 2020 10:41:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C67D020776 for ; Tue, 16 Jun 2020 10:41:43 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="DfEHxSBo" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728532AbgFPKlm (ORCPT ); Tue, 16 Jun 2020 06:41:42 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55294 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725901AbgFPKlT (ORCPT ); Tue, 16 Jun 2020 06:41:19 -0400 Received: from mail-pl1-x642.google.com (mail-pl1-x642.google.com [IPv6:2607:f8b0:4864:20::642]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CCD09C08C5C5 for ; Tue, 16 Jun 2020 03:41:17 -0700 (PDT) Received: by mail-pl1-x642.google.com with SMTP id n9so8219267plk.1 for ; Tue, 16 Jun 2020 03:41:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=lePPQwo+mnHTXVB83gJI9+SsNpNRSndF9oxmatv8k7U=; b=DfEHxSBojphs0ObnKRo4J+et+0qiewmZYv/0OgsaZYBd2sfXX7jWAGuPWt3UtIO9pN /sA9X4OCpTwWyBWmV+/grX8R3FzDd73aQI8mCXdLFvnnfqRbbhhd7SYFpPx6MLR0pK8c lwyaiuQ6ap4hqnHiZlcUxUex35ERKeV0q5hy0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=lePPQwo+mnHTXVB83gJI9+SsNpNRSndF9oxmatv8k7U=; b=TwAB/bdhZj2Gqvo2hOdVGwsnAoSHy30Tu2hB27IxPY3OQomKkJaTVpGZGXS2tO1PrA i1cOheuZ1pbnIOriVIcJxCrpxwX6/o8VLwpN90qIP9JdMtIVl5n4snK/koZ/L1FtEIch c0iEitofwQeJks2Ib2DRUwDZVur3jxsWp6FTvxzlT/Oa3x59sSGJ5MoeQSZgrY8v0yoL OnGKelynKnAf6K8XvVzSFR4EMB7Q91+enAYlSKsR4AwTnv3MAbDAV8BdZ153oPRgid2C zbCvqP+hHMMnfRM3xA/6BB5z8LwImjng1zK72Xz4iTGOabTgDkLjTVfgkx+WfApvhFz7 NaVw== X-Gm-Message-State: AOAM5310knAy2jICYKjmusKQ7rp1FAHWn6rEeMPSPmYYIWQAB5My41co nK3JCbnW7jB/yX/5p3NjoJ2oVH1K8WU= X-Google-Smtp-Source: ABdhPJy3TA7Zm0d3Igecwi3K7vC2UTfhsaUsH3wa9qhNxxQdvtzae9mfrad6ZSTgTAbD042xH+gvNw== X-Received: by 2002:a17:902:9009:: with SMTP id a9mr1561226plp.21.1592304077364; Tue, 16 Jun 2020 03:41:17 -0700 (PDT) Received: from tictac2.mtv.corp.google.com ([2620:15c:202:1:24fa:e766:52c9:e3b2]) by smtp.gmail.com with ESMTPSA id 140sm16947400pfz.154.2020.06.16.03.41.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jun 2020 03:41:16 -0700 (PDT) From: Douglas Anderson To: Mark Brown Cc: Alok Chauhan , skakit@codeaurora.org, swboyd@chromium.org, Douglas Anderson , Andy Gross , Bjorn Andersson , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org Subject: [PATCH v3 1/5] spi: spi-geni-qcom: No need for irqsave variant of spinlock calls Date: Tue, 16 Jun 2020 03:40:46 -0700 Message-Id: <20200616034044.v3.1.Ic50cccdf27d42420a63485082f8b5bf86ed1a2b6@changeid> X-Mailer: git-send-email 2.27.0.290.gba653c62da-goog In-Reply-To: <20200616104050.84764-1-dianders@chromium.org> References: <20200616104050.84764-1-dianders@chromium.org> MIME-Version: 1.0 Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org The driver locks its locks in two places. In the first usage of the lock the function doing the locking already has a sleeping call and thus we know we can't be called from interrupt context. That means we can use the "spin_lock_irq" variant of the function. In the second usage of the lock the function is the interrupt handler and we know interrupt handlers are called with interrupts disabled. That means we can use the "spin_lock" variant of the function. This patch is expected to be a no-op and is just a cleanup / slight optimization. Signed-off-by: Douglas Anderson Reviewed-by: Stephen Boyd --- Changes in v3: - ("spi: spi-geni-qcom: No need for irqsave variant...") new for v3 Changes in v2: None drivers/spi/spi-geni-qcom.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index c3972424af71..c7d2c7e45b3f 100644 --- a/drivers/spi/spi-geni-qcom.c +++ b/drivers/spi/spi-geni-qcom.c @@ -122,23 +122,23 @@ static void handle_fifo_timeout(struct spi_master *spi, struct spi_message *msg) { struct spi_geni_master *mas = spi_master_get_devdata(spi); - unsigned long time_left, flags; + unsigned long time_left; struct geni_se *se = &mas->se; - spin_lock_irqsave(&mas->lock, flags); + spin_lock_irq(&mas->lock); reinit_completion(&mas->xfer_done); mas->cur_mcmd = CMD_CANCEL; geni_se_cancel_m_cmd(se); writel(0, se->base + SE_GENI_TX_WATERMARK_REG); - spin_unlock_irqrestore(&mas->lock, flags); + spin_unlock_irq(&mas->lock); time_left = wait_for_completion_timeout(&mas->xfer_done, HZ); if (time_left) return; - spin_lock_irqsave(&mas->lock, flags); + spin_lock_irq(&mas->lock); reinit_completion(&mas->xfer_done); geni_se_abort_m_cmd(se); - spin_unlock_irqrestore(&mas->lock, flags); + spin_unlock_irq(&mas->lock); time_left = wait_for_completion_timeout(&mas->xfer_done, HZ); if (!time_left) dev_err(mas->dev, "Failed to cancel/abort m_cmd\n"); @@ -477,12 +477,11 @@ static irqreturn_t geni_spi_isr(int irq, void *data) struct spi_geni_master *mas = spi_master_get_devdata(spi); struct geni_se *se = &mas->se; u32 m_irq; - unsigned long flags; if (mas->cur_mcmd == CMD_NONE) return IRQ_NONE; - spin_lock_irqsave(&mas->lock, flags); + spin_lock(&mas->lock); m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS); if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN)) @@ -524,7 +523,7 @@ static irqreturn_t geni_spi_isr(int irq, void *data) } writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR); - spin_unlock_irqrestore(&mas->lock, flags); + spin_unlock(&mas->lock); return IRQ_HANDLED; } From patchwork Tue Jun 16 10:40:47 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Anderson X-Patchwork-Id: 11607131 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id B22411392 for ; Tue, 16 Jun 2020 10:41:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 95C6520786 for ; Tue, 16 Jun 2020 10:41:30 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="ONPCf/tn" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728489AbgFPKlZ (ORCPT ); Tue, 16 Jun 2020 06:41:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55308 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728450AbgFPKlU (ORCPT ); Tue, 16 Jun 2020 06:41:20 -0400 Received: from mail-pl1-x632.google.com (mail-pl1-x632.google.com [IPv6:2607:f8b0:4864:20::632]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3F5DFC08C5C4 for ; Tue, 16 Jun 2020 03:41:19 -0700 (PDT) Received: by mail-pl1-x632.google.com with SMTP id m7so8205487plt.5 for ; Tue, 16 Jun 2020 03:41:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=TPfD16LMcnCPHfyxWwsic4qGGN1GTg8YG3jhDYqUndk=; b=ONPCf/tn87lXfOWlBr49mcpBon8SJ0/o14H2GFRj2pOkUHr3UjTzZY9ux3YCzU5zJV JZiMBHBK44NFc6lhQ4g8zJnOcsg6ICk+PrbiK2cxWvf8ZS1EPmPtxjmGcvr5JPj3BQUc nXaoRSh18sFwkUCvA00dFDTuLka6bP8VpqrZY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=TPfD16LMcnCPHfyxWwsic4qGGN1GTg8YG3jhDYqUndk=; b=oBvoUm1KtmI15fvfGYczAf91D0IQzHaGOC40cDFrcWwPhQQ2RDDAh/RU+X65c8C5Wf 5OJ8z7qHcMuz70VkbWHonuMFVP2tB3jcxayla5kFTUiLYeQ4NRTUyi9/hBzSaSYh0tiD 3GHFQXR4XPQx/VUfEcvEbdZOzcVqsIu3i9YR4Xm6dlUZaUnOgYDj+98cfFf2AomJTvMw uGpnsmMP22dCSpBHie7NEayvn0pjssGQLzcVGjhaKuhgrpoLBTQALNxejZKqsHP0KDrv fKYeDvCfQs72jFvazNiiNXp9ewqEYCcBkKdMMrsOdFXHN8bDzHN0YuK9wYMrM4p28+sH ANwQ== X-Gm-Message-State: AOAM531xmsAFKxFGLpR7/5Oy7gxayuQSKII9awGe8r+EIASBBitPoIXe w5iVqqQGl0e+ELKL7m+G4WuZBMKVvpo= X-Google-Smtp-Source: ABdhPJzzTOX0a5TxOiFL9gkTxc2JhbpANn1TcI4Nl1+7bGQ1sKBMNrrK8TvVznKEQRAx2l0f7EJe8w== X-Received: by 2002:a17:902:d712:: with SMTP id w18mr1520710ply.63.1592304078627; Tue, 16 Jun 2020 03:41:18 -0700 (PDT) Received: from tictac2.mtv.corp.google.com ([2620:15c:202:1:24fa:e766:52c9:e3b2]) by smtp.gmail.com with ESMTPSA id 140sm16947400pfz.154.2020.06.16.03.41.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jun 2020 03:41:18 -0700 (PDT) From: Douglas Anderson To: Mark Brown Cc: Alok Chauhan , skakit@codeaurora.org, swboyd@chromium.org, Douglas Anderson , Andy Gross , Bjorn Andersson , Dilip Kota , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org Subject: [PATCH v3 2/5] spi: spi-geni-qcom: Mo' betta locking Date: Tue, 16 Jun 2020 03:40:47 -0700 Message-Id: <20200616034044.v3.2.I752ebdcfd5e8bf0de06d66e767b8974932b3620e@changeid> X-Mailer: git-send-email 2.27.0.290.gba653c62da-goog In-Reply-To: <20200616104050.84764-1-dianders@chromium.org> References: <20200616104050.84764-1-dianders@chromium.org> MIME-Version: 1.0 Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org If you added a bit of a delay (like a trace_printk) into the ISR for the spi-geni-qcom driver, you would suddenly start seeing some errors spit out. The problem was that, though the ISR itself held a lock, other parts of the driver didn't always grab the lock. One example race was this: a) Driver queues off a command to set a Chip Select (CS). b) ISR fires indicating the CS is done. c) Done bit is set, so we complete(). d) Second CPU gallops off and starts a transfer. e) Second CPU starts messing with hardware / software state (not under spinlock). f) ISR now does things like set "mas->cur_mcmd" to CMD_NONE, prints errors if "tx_rem_bytes" / "rx_rem_bytes" have been set, and also Acks all interrupts it handled. Let's fix this. Before we start messing with hardware, we'll grab the lock to make sure that the IRQ handler from some previous command has really finished. We don't need to hold the lock unless we're in a state where more interrupts can come in, but we at least need to make sure the previous IRQ is done. This lock is used exclusively to prevent the IRQ handler and non-IRQ from stomping on each other. The SPI core handles all other mutual exclusion. As part of this, we change the way that the IRQ handler detects spurious interrupts. Previously we checked for our state variable being set to IRQ_NONE, but that was done outside the spinlock. We could move it into the spinlock, but instead let's just change it to look for the lack of any IRQ status bits being set. This can be done outside the lock--the hardware certainly isn't grabbing or looking at the spinlock when it updates its status register. It's possible that this will fix real (but very rare) errors seen in the field that look like: irq ...: nobody cared (try booting with the "irqpoll" option) NOTE: an alternate strategy considered here was to always make the complete() / spi_finalize_current_transfer() the very last thing in our IRQ handler. With such a change you could consider that we could be "lockless". In that case, though, we'd have to be very careful w/ memory barriers so we made sure we didn't have any bugs with weakly ordered memory. Using spinlocks makes the driver much easier to understand. Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP") Signed-off-by: Douglas Anderson --- Changes in v3: - Split out some lock cleanup to previous patch. - Don't need to read IRQ status register inside spinlock. - Don't check for state CMD_NONE; later patch is removing state var. - Don't hold the lock for all of setup_fifo_xfer(). - Comment about why it's safe to Ack interrupts at the end. - Subject/desc changed since race is definitely there. Changes in v2: - Detect true spurious interrupt. - Still return IRQ_NONE for state machine mismatch, but print warn. drivers/spi/spi-geni-qcom.c | 45 ++++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index c7d2c7e45b3f..e0f0e5c241f3 100644 --- a/drivers/spi/spi-geni-qcom.c +++ b/drivers/spi/spi-geni-qcom.c @@ -151,16 +151,18 @@ static void spi_geni_set_cs(struct spi_device *slv, bool set_flag) struct geni_se *se = &mas->se; unsigned long time_left; - reinit_completion(&mas->xfer_done); pm_runtime_get_sync(mas->dev); if (!(slv->mode & SPI_CS_HIGH)) set_flag = !set_flag; + spin_lock_irq(&mas->lock); + reinit_completion(&mas->xfer_done); mas->cur_mcmd = CMD_CS; if (set_flag) geni_se_setup_m_cmd(se, SPI_CS_ASSERT, 0); else geni_se_setup_m_cmd(se, SPI_CS_DEASSERT, 0); + spin_unlock_irq(&mas->lock); time_left = wait_for_completion_timeout(&mas->xfer_done, HZ); if (!time_left) @@ -307,6 +309,21 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, u32 spi_tx_cfg, len; struct geni_se *se = &mas->se; + /* + * Ensure that our interrupt handler isn't still running from some + * prior command before we start messing with the hardware behind + * its back. We don't need to _keep_ the lock here since we're only + * worried about racing with out interrupt handler. The SPI core + * already handles making sure that we're not trying to do two + * transfers at once or setting a chip select and doing a transfer + * concurrently. + * + * NOTE: we actually _can't_ hold the lock here because possibly we + * might call clk_set_rate() which needs to be able to sleep. + */ + spin_lock_irq(&mas->lock); + spin_unlock_irq(&mas->lock); + spi_tx_cfg = readl(se->base + SE_SPI_TRANS_CFG); if (xfer->bits_per_word != mas->cur_bits_per_word) { spi_setup_word_len(mas, mode, xfer->bits_per_word); @@ -367,6 +384,12 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, } writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG); mas->cur_mcmd = CMD_XFER; + + /* + * Lock around right before we start the transfer since our + * interrupt controller could come in at any time now. + */ + spin_lock_irq(&mas->lock); geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION); /* @@ -376,6 +399,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, */ if (m_cmd & SPI_TX_ONLY) writel(mas->tx_wm, se->base + SE_GENI_TX_WATERMARK_REG); + spin_unlock_irq(&mas->lock); } static int spi_geni_transfer_one(struct spi_master *spi, @@ -478,11 +502,11 @@ static irqreturn_t geni_spi_isr(int irq, void *data) struct geni_se *se = &mas->se; u32 m_irq; - if (mas->cur_mcmd == CMD_NONE) + m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS); + if (!m_irq) return IRQ_NONE; spin_lock(&mas->lock); - m_irq = readl(se->base + SE_GENI_M_IRQ_STATUS); if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN)) geni_spi_handle_rx(mas); @@ -522,8 +546,23 @@ static irqreturn_t geni_spi_isr(int irq, void *data) complete(&mas->xfer_done); } + /* + * It's safe or a good idea to Ack all of our our interrupts at the + * end of the function. Specifically: + * - M_CMD_DONE_EN / M_RX_FIFO_LAST_EN: Edge triggered interrupts and + * clearing Acks. Clearing at the end relies on nobody else having + * started a new transfer yet or else we could be clearing _their_ + * done bit, but everyone grabs the spinlock before starting a new + * transfer. + * - M_RX_FIFO_WATERMARK_EN / M_TX_FIFO_WATERMARK_EN: These appear + * to be "latched level" interrupts so it's important to clear them + * _after_ you've handled the condition and always safe to do so + * since they'll re-assert if they're still happening. + */ writel(m_irq, se->base + SE_GENI_M_IRQ_CLEAR); + spin_unlock(&mas->lock); + return IRQ_HANDLED; } From patchwork Tue Jun 16 10:40:48 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Anderson X-Patchwork-Id: 11607141 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id AD5E114DD for ; Tue, 16 Jun 2020 10:41:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9532520776 for ; Tue, 16 Jun 2020 10:41:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="SBUAQMaC" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728460AbgFPKlk (ORCPT ); Tue, 16 Jun 2020 06:41:40 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55294 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728454AbgFPKlU (ORCPT ); Tue, 16 Jun 2020 06:41:20 -0400 Received: from mail-pj1-x1041.google.com (mail-pj1-x1041.google.com [IPv6:2607:f8b0:4864:20::1041]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 417AAC08C5C8 for ; Tue, 16 Jun 2020 03:41:20 -0700 (PDT) Received: by mail-pj1-x1041.google.com with SMTP id jz3so1259054pjb.0 for ; Tue, 16 Jun 2020 03:41:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=e4m6P3VqQCLvUhFBDKb6BzhpBcCZvCCTqjg+tD53gAY=; b=SBUAQMaCh6XaARCtdGI3FoxefEJ7byLWZI/XJCo4ieCiBLghkY+Y7V23Z0RXkfYm3D VR88nWZw16xFyasefWh9WQruIAP7V9tX6o+hkWYu581CG23OGPBlIKwCE0TkKqIi1i+I BtEFwt3dguzFXCdl1m7x2Yof9DwFlJpnWNlmI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=e4m6P3VqQCLvUhFBDKb6BzhpBcCZvCCTqjg+tD53gAY=; b=S7HoRc0RoY0q/fl0RLye0wyMQjJSmQnjyLrtG6GpX+OyvnYoZRx/DC6nqZ4JDQsXIi h6gmYwOi+WOnEiyrTlueEc2DP2D1rMEme0OEuTc8LNhpL414HewI/Co8GzK5U90dttOH /QSP8uvxU4mooXd7CpmIeuC4qUFF9tBQTKavNpt0k43TZ32n36+ogesYy/OkkvXFD33t zzL3TN+FoyCNKutY72FdvoqC4wj0ARj8cOV6dPEGYvfGpjl6XEayn/1VFbcsBFmy27yD kq6ntHXgBjD/LBWqE3351JkFytR2rA0kLq2vAGpqHUCd7VcGKewp/+h4+NTNJPymNaZk YHiA== X-Gm-Message-State: AOAM530fnUsxDUbaIAy9cF9bKSwwIoYY6IEXuN2EIhbhiiNdLdjlfE4Q XapWDPQ686aeBZyzojvs4nqf4w== X-Google-Smtp-Source: ABdhPJzI5J1yKUbOrc2+yB3c5xtwjsI2MMS87NAhcz5LEgsif7C24AcptmwDLNOtc1vhUZcllgVfwg== X-Received: by 2002:a17:902:9044:: with SMTP id w4mr1574397plz.83.1592304079792; Tue, 16 Jun 2020 03:41:19 -0700 (PDT) Received: from tictac2.mtv.corp.google.com ([2620:15c:202:1:24fa:e766:52c9:e3b2]) by smtp.gmail.com with ESMTPSA id 140sm16947400pfz.154.2020.06.16.03.41.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jun 2020 03:41:19 -0700 (PDT) From: Douglas Anderson To: Mark Brown Cc: Alok Chauhan , skakit@codeaurora.org, swboyd@chromium.org, Douglas Anderson , Andy Gross , Bjorn Andersson , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org Subject: [PATCH v3 3/5] spi: spi-geni-qcom: Check for error IRQs Date: Tue, 16 Jun 2020 03:40:48 -0700 Message-Id: <20200616034044.v3.3.Id8bebdbdb4d2ed9468634343a7e6207d6cffff8a@changeid> X-Mailer: git-send-email 2.27.0.290.gba653c62da-goog In-Reply-To: <20200616104050.84764-1-dianders@chromium.org> References: <20200616104050.84764-1-dianders@chromium.org> MIME-Version: 1.0 Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org From reading the #defines it seems like we should shout if we ever see one of these error bits. Let's do so. This doesn't do anything functional except print a yell in the log if the error bits are seen. Signed-off-by: Douglas Anderson Reviewed-by: Stephen Boyd --- Changes in v3: - ("spi: spi-geni-qcom: Check for error IRQs") new in v3. Changes in v2: None drivers/spi/spi-geni-qcom.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index e0f0e5c241f3..56628d45421e 100644 --- a/drivers/spi/spi-geni-qcom.c +++ b/drivers/spi/spi-geni-qcom.c @@ -506,6 +506,11 @@ static irqreturn_t geni_spi_isr(int irq, void *data) if (!m_irq) return IRQ_NONE; + if (m_irq & (M_CMD_OVERRUN_EN | M_ILLEGAL_CMD_EN | M_CMD_FAILURE_EN | + M_RX_FIFO_RD_ERR_EN | M_RX_FIFO_WR_ERR_EN | + M_TX_FIFO_RD_ERR_EN | M_TX_FIFO_WR_ERR_EN)) + dev_warn(mas->dev, "Unexpected IRQ err status %#010x\n", m_irq); + spin_lock(&mas->lock); if ((m_irq & M_RX_FIFO_WATERMARK_EN) || (m_irq & M_RX_FIFO_LAST_EN)) From patchwork Tue Jun 16 10:40:49 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Anderson X-Patchwork-Id: 11607139 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 4E3C21392 for ; Tue, 16 Jun 2020 10:41:41 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 35F2520776 for ; Tue, 16 Jun 2020 10:41:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="QpMn2xbY" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727969AbgFPKlk (ORCPT ); Tue, 16 Jun 2020 06:41:40 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55314 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728460AbgFPKlV (ORCPT ); Tue, 16 Jun 2020 06:41:21 -0400 Received: from mail-pj1-x1041.google.com (mail-pj1-x1041.google.com [IPv6:2607:f8b0:4864:20::1041]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 57880C08C5C5 for ; Tue, 16 Jun 2020 03:41:21 -0700 (PDT) Received: by mail-pj1-x1041.google.com with SMTP id b7so1066241pju.0 for ; Tue, 16 Jun 2020 03:41:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=QnbTanlK6vmWDYRWZepA4l+8SpUkNjPIBDmpYRotz+c=; b=QpMn2xbYSqizB3W5EgihyPoGHa7zsnzjCRgfXCMifc7/VUP+4ThmvoemTFLohLL71y 5JZ0Q0dgMtGOZDUGdg1J3B/nyTn0U4uyXNLHnMmhE5az2Q+lu98ih1EAI7g4wZRv/T55 kuvRNyqbCpQOn4KyyBHAEw92sdEYnuxzHmxIo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=QnbTanlK6vmWDYRWZepA4l+8SpUkNjPIBDmpYRotz+c=; b=WZOwkzVS8K6aZR3wtzAu1RfMHo1jesfkjVcNFOpQkgncKx0cUBPA+IN+/8mglHUtEZ F1jZ9ERhLtz0LGECv2KGV7UqVats90LPQczp7uJxbHu0DxEqIiGVBViHgGC5JoQw20yp UI2o4jWNKcWkJcThNID7WIne3n2YgPfzhC9xNwb9JURGX2oFvim3YiqAIZrSsduA+6/R QzA1BEuqbz0JLnZf8h9gJNWV/VCMDksn6+M3vF8I4keQuGYdPtpCq0BCWMvIKzSPwDPk fMPcK1Em8Dn00JTGb5cYW9vBrs2g8stuUKtxllLmyFfds13Fwn2yXswXJMxFKZSFvp7Z 49HA== X-Gm-Message-State: AOAM530kIEDArbSTHwJs6qFU4tAqd1pbRIyEiy7zwqpjtzsKeciuA7vp uGbg+VkYZTOFb9Rdvzkn5KvdVA== X-Google-Smtp-Source: ABdhPJxcOZtKi9rHF3B2KWjnaAn4f3KL9+GoKE61t5pAbz14hlSOGhr1kijdk3Ef/LVhBV48UWkvxg== X-Received: by 2002:a17:902:ff06:: with SMTP id f6mr1523980plj.22.1592304080864; Tue, 16 Jun 2020 03:41:20 -0700 (PDT) Received: from tictac2.mtv.corp.google.com ([2620:15c:202:1:24fa:e766:52c9:e3b2]) by smtp.gmail.com with ESMTPSA id 140sm16947400pfz.154.2020.06.16.03.41.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jun 2020 03:41:20 -0700 (PDT) From: Douglas Anderson To: Mark Brown Cc: Alok Chauhan , skakit@codeaurora.org, swboyd@chromium.org, Douglas Anderson , Andy Gross , Bjorn Andersson , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org Subject: [PATCH v3 4/5] spi: spi-geni-qcom: Actually use our FIFO Date: Tue, 16 Jun 2020 03:40:49 -0700 Message-Id: <20200616034044.v3.4.I988281f7c6ee0ed00325559bfce7539f403da69e@changeid> X-Mailer: git-send-email 2.27.0.290.gba653c62da-goog In-Reply-To: <20200616104050.84764-1-dianders@chromium.org> References: <20200616104050.84764-1-dianders@chromium.org> MIME-Version: 1.0 Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org The geni hardware has a FIFO that can hold up to 64 bytes (it has 16 entries that can hold 4 bytes each), at least on the two SoCs I tested (sdm845 and sc7180). We configured our RX Watermark to 0, which basically meant we got an interrupt as soon as the first 4 bytes showed up in the FIFO. Tracing the IRQ handler showed that we often only read 4 or 8 bytes per IRQ handler. I tried setting the RX Watermark to "fifo size - 2" but that just got me a bunch of overrun errors reported. Setting it to "fifo size - 3" seemed to work great, though. This made me worried that we'd start getting overruns if we had long interrupt latency, but that doesn't appear to be the case and delays inserted in the IRQ handler while using "fifo size - 3" didn't cause any errors. Presumably there is some interaction with the poorly-documented RFR (ready for receive) level means that "fifo size - 3" is the max. We are the SPI master, so it makes sense that there would be no problems with overruns, the master should just stop clocking. Despite "fifo size - 3" working, I chose "fifo size / 2" (8 entries = 32 bytes) which gives us a little extra time to get to the interrupt handler and should reduce dead time on the SPI wires. With this setting, I often saw the IRQ handler handle 40 bytes but sometimes up to 56 if we had bad interrupt latency. Testing by running "flashrom -p ec -r" on a Chromebook saw interrupts from the SPI driver cut roughly in half. Time was roughly the same. Signed-off-by: Douglas Anderson Reviewed-by: Stephen Boyd --- Changes in v3: - ("spi: spi-geni-qcom: Actually use our FIFO") new in v3. Changes in v2: None drivers/spi/spi-geni-qcom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index 56628d45421e..63a62548b078 100644 --- a/drivers/spi/spi-geni-qcom.c +++ b/drivers/spi/spi-geni-qcom.c @@ -285,7 +285,7 @@ static int spi_geni_init(struct spi_geni_master *mas) * Hardware programming guide suggests to configure * RX FIFO RFR level to fifo_depth-2. */ - geni_se_init(se, 0x0, mas->tx_fifo_depth - 2); + geni_se_init(se, mas->tx_fifo_depth / 2, mas->tx_fifo_depth - 2); /* Transmit an entire FIFO worth of data per IRQ */ mas->tx_wm = 1; ver = geni_se_get_qup_hw_version(se); From patchwork Tue Jun 16 10:40:50 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Anderson X-Patchwork-Id: 11607137 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 55BE21392 for ; Tue, 16 Jun 2020 10:41:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3989720776 for ; Tue, 16 Jun 2020 10:41:40 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="fi9cinkk" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728496AbgFPKla (ORCPT ); Tue, 16 Jun 2020 06:41:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55320 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728469AbgFPKlW (ORCPT ); Tue, 16 Jun 2020 06:41:22 -0400 Received: from mail-pg1-x543.google.com (mail-pg1-x543.google.com [IPv6:2607:f8b0:4864:20::543]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id BB009C08C5C3 for ; Tue, 16 Jun 2020 03:41:22 -0700 (PDT) Received: by mail-pg1-x543.google.com with SMTP id u128so2146208pgu.13 for ; Tue, 16 Jun 2020 03:41:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=wPzcsZu19gLGHlfTpg/OVz60qYIP10yYC7Od6iya2fQ=; b=fi9cinkktR+lFKaArv2L5i3SkW2V2zi8UtNp33Wy74Aua59Fm+PF5Yqfb+WQ4Hcr1w JJrhwpirE5At/gcnA3IzafQ3pmIaLZayOLOVeKcOCU3MAbgJ8G/358NcBH6HG/WT8QiE xv7a6CcWTp0oBUx8bNfOw1IMeIFKYI7M8IJ10= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=wPzcsZu19gLGHlfTpg/OVz60qYIP10yYC7Od6iya2fQ=; b=eqrdr87qsJk7wBiHOJ2IXyY028MYLKmVGgW9x9CC2PXO35B7RXTwfTJnZOVL3LRKGe LSKa4AIiHXVuvwPyWeLWfCogikghhe1QmnoXixdYEz917ZzsKwW5XpP7338QIYZSHNIB nRoavqc2EMmSF1CuPkS/3ajBUsNzYqYQvorcAtf3YtEHtsz7BES8nh/FTtEOI4NCLNlv xvUlqbhPdBKTeMRxZmVB3266ZiFls7Upp+yjvY1c0TgfkBLKSrWfx5QxySKkAXCnXyjW gkb1R2WPvq3vQN43U0RpYTe1EYIxuva/pAJrWW7KWMtOQomauRre9ff6oJOoL9X2CGVJ whMA== X-Gm-Message-State: AOAM532rhYWY9RVtPTkV2/tVLR0ZMebjM1XMI39aaX7gmpua4cHsNere Ff8z6HIjybX1UC8p+GnXx7tM/w== X-Google-Smtp-Source: ABdhPJwNARi0Kqm3Jnvas3QIe0jp32CNeyZFGnkYa+K2v8cg9KXzvug6VY8RJKi2tgtrvOeQU4mf7A== X-Received: by 2002:a63:e04a:: with SMTP id n10mr1552555pgj.157.1592304082218; Tue, 16 Jun 2020 03:41:22 -0700 (PDT) Received: from tictac2.mtv.corp.google.com ([2620:15c:202:1:24fa:e766:52c9:e3b2]) by smtp.gmail.com with ESMTPSA id 140sm16947400pfz.154.2020.06.16.03.41.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 16 Jun 2020 03:41:21 -0700 (PDT) From: Douglas Anderson To: Mark Brown Cc: Alok Chauhan , skakit@codeaurora.org, swboyd@chromium.org, Douglas Anderson , Andy Gross , Bjorn Andersson , linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org Subject: [PATCH v3 5/5] spi: spi-geni-qcom: Don't keep a local state variable Date: Tue, 16 Jun 2020 03:40:50 -0700 Message-Id: <20200616034044.v3.5.Ib1e6855405fc9c99916ab7c7dee84d73a8bf3d68@changeid> X-Mailer: git-send-email 2.27.0.290.gba653c62da-goog In-Reply-To: <20200616104050.84764-1-dianders@chromium.org> References: <20200616104050.84764-1-dianders@chromium.org> MIME-Version: 1.0 Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org The variable "cur_mcmd" kept track of our current state (idle, xfer, cs, cancel). We don't really need it, so get rid of it. Instead: * Use separate condition variables for "chip select done", "cancel done", and "abort done". This is important so that if a "done" comes through (perhaps some previous interrupt finally came through) it can't confuse the cancel/abort function. * Use the "done" interrupt only for when a chip select or transfer is done and we can tell the difference by looking at whether "cur_xfer" is NULL. This is mostly a no-op change. However, it is possible it could fix an issue where a super delayed interrupt for a cancel command could have confused our waiting for an abort command. Signed-off-by: Douglas Anderson --- Changes in v3: - ("spi: spi-geni-qcom: Don't keep a local state variable") new in v3. Changes in v2: None drivers/spi/spi-geni-qcom.c | 55 ++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index 63a62548b078..6feea88d63ac 100644 --- a/drivers/spi/spi-geni-qcom.c +++ b/drivers/spi/spi-geni-qcom.c @@ -63,13 +63,6 @@ #define TIMESTAMP_AFTER BIT(3) #define POST_CMD_DELAY BIT(4) -enum spi_m_cmd_opcode { - CMD_NONE, - CMD_XFER, - CMD_CS, - CMD_CANCEL, -}; - struct spi_geni_master { struct geni_se se; struct device *dev; @@ -81,10 +74,11 @@ struct spi_geni_master { unsigned int tx_rem_bytes; unsigned int rx_rem_bytes; const struct spi_transfer *cur_xfer; - struct completion xfer_done; + struct completion cs_done; + struct completion cancel_done; + struct completion abort_done; unsigned int oversampling; spinlock_t lock; - enum spi_m_cmd_opcode cur_mcmd; int irq; }; @@ -126,20 +120,23 @@ static void handle_fifo_timeout(struct spi_master *spi, struct geni_se *se = &mas->se; spin_lock_irq(&mas->lock); - reinit_completion(&mas->xfer_done); - mas->cur_mcmd = CMD_CANCEL; - geni_se_cancel_m_cmd(se); + reinit_completion(&mas->cancel_done); writel(0, se->base + SE_GENI_TX_WATERMARK_REG); + mas->cur_xfer = NULL; + mas->tx_rem_bytes = mas->rx_rem_bytes = 0; + geni_se_cancel_m_cmd(se); spin_unlock_irq(&mas->lock); - time_left = wait_for_completion_timeout(&mas->xfer_done, HZ); + + time_left = wait_for_completion_timeout(&mas->cancel_done, HZ); if (time_left) return; spin_lock_irq(&mas->lock); - reinit_completion(&mas->xfer_done); + reinit_completion(&mas->abort_done); geni_se_abort_m_cmd(se); spin_unlock_irq(&mas->lock); - time_left = wait_for_completion_timeout(&mas->xfer_done, HZ); + + time_left = wait_for_completion_timeout(&mas->abort_done, HZ); if (!time_left) dev_err(mas->dev, "Failed to cancel/abort m_cmd\n"); } @@ -156,15 +153,14 @@ static void spi_geni_set_cs(struct spi_device *slv, bool set_flag) set_flag = !set_flag; spin_lock_irq(&mas->lock); - reinit_completion(&mas->xfer_done); - mas->cur_mcmd = CMD_CS; + reinit_completion(&mas->cs_done); if (set_flag) geni_se_setup_m_cmd(se, SPI_CS_ASSERT, 0); else geni_se_setup_m_cmd(se, SPI_CS_DEASSERT, 0); spin_unlock_irq(&mas->lock); - time_left = wait_for_completion_timeout(&mas->xfer_done, HZ); + time_left = wait_for_completion_timeout(&mas->cs_done, HZ); if (!time_left) handle_fifo_timeout(spi, NULL); @@ -383,7 +379,6 @@ static void setup_fifo_xfer(struct spi_transfer *xfer, mas->rx_rem_bytes = xfer->len; } writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG); - mas->cur_mcmd = CMD_XFER; /* * Lock around right before we start the transfer since our @@ -520,11 +515,13 @@ static irqreturn_t geni_spi_isr(int irq, void *data) geni_spi_handle_tx(mas); if (m_irq & M_CMD_DONE_EN) { - if (mas->cur_mcmd == CMD_XFER) + if (mas->cur_xfer) { spi_finalize_current_transfer(spi); - else if (mas->cur_mcmd == CMD_CS) - complete(&mas->xfer_done); - mas->cur_mcmd = CMD_NONE; + mas->cur_xfer = NULL; + } else { + complete(&mas->cs_done); + } + /* * If this happens, then a CMD_DONE came before all the Tx * buffer bytes were sent out. This is unusual, log this @@ -546,10 +543,10 @@ static irqreturn_t geni_spi_isr(int irq, void *data) mas->rx_rem_bytes, mas->cur_bits_per_word); } - if ((m_irq & M_CMD_CANCEL_EN) || (m_irq & M_CMD_ABORT_EN)) { - mas->cur_mcmd = CMD_NONE; - complete(&mas->xfer_done); - } + if (m_irq & M_CMD_CANCEL_EN) + complete(&mas->cancel_done); + if (m_irq & M_CMD_ABORT_EN) + complete(&mas->abort_done); /* * It's safe or a good idea to Ack all of our our interrupts at the @@ -617,7 +614,9 @@ static int spi_geni_probe(struct platform_device *pdev) spi->handle_err = handle_fifo_timeout; spi->set_cs = spi_geni_set_cs; - init_completion(&mas->xfer_done); + init_completion(&mas->cs_done); + init_completion(&mas->cancel_done); + init_completion(&mas->abort_done); spin_lock_init(&mas->lock); pm_runtime_enable(dev);