From patchwork Wed Apr 24 13:52:38 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Wolsieffer X-Patchwork-Id: 13641929 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 98177C4345F for ; Wed, 24 Apr 2024 13:58:49 +0000 (UTC) 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:MIME-Version:Message-ID:Date:Subject:Cc :To:From:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:References: List-Owner; bh=rA2vsF1PY2VqZC8Cj/w/QHb17oyISPz75ktfqqh8Mwc=; b=C3Nxqhn4GBc9Db IYrX7Kyn9TcSDrMeAsH7RcE21kJXjZirHWOj82DdDYdHgki2jqeHJa8KeEB8IcqgZ8Ue/yU+TNXmM tUUyQi7xpG+A6YJpK6liaM5eMj2RG1yvB/c/oeWFhqu4a+gDbezLbRmQt0EP79meZYnZhfdh5NjKw pQ2UHFXkXTCBNf1Eho+xTtmFFCFA67FKB7VY7Kebf4KTDeiwY6GllMg3oKqEGagieW0stNyYmHh6f 7607PLXkDRuik8o75aopib+REUlG5BnhOLFOT++033LqeUaxp7nWUn/VYNDVisMJhi5oOBULrH2lx F0ESi99e9fctWKJ1FCpw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rzd8w-00000004NOn-0ImA; Wed, 24 Apr 2024 13:58:38 +0000 Received: from mail-oo1-xc2c.google.com ([2607:f8b0:4864:20::c2c]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rzd8s-00000004NM6-25UP for linux-arm-kernel@lists.infradead.org; Wed, 24 Apr 2024 13:58:36 +0000 Received: by mail-oo1-xc2c.google.com with SMTP id 006d021491bc7-5a9ec68784cso3797288eaf.2 for ; Wed, 24 Apr 2024 06:58:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=hefring-com.20230601.gappssmtp.com; s=20230601; t=1713967110; x=1714571910; darn=lists.infradead.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=47bZdX0wm5QrsJ8T2X7IRvwqReXemUOe2I5iAISiMQM=; b=Snq+2Tp7hMslopmgmlgcGa9jfgC8Qr3dQbnnE4uOyOsrFWK2A+4loJnkiZER85LoDZ vEPFPWBAQDq7qB42IalYgLAgM1dzKF425xaCr4faazLjmam7/sh3ESBmyR+oFSxzrCzO +KE1C71uQbo/aV4h+z9kMay6JNB+LwuuK4pxL7G527XSGr6NjzDRlMJMT56FBM7ePppt A7oRNkzK41ntIYesL7b63KJwqCo4y2Eg0vjZPCOsmPl89h49sRagJDeIROgteHI7Yl/W 6Z1DgYbQs6+W6EtR1lXzFwuHsE/oGwMDkcXA9mdf4GmLh+QJv/cmoeekjSIvG9iuPrqJ UuxQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713967110; x=1714571910; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=47bZdX0wm5QrsJ8T2X7IRvwqReXemUOe2I5iAISiMQM=; b=cKi4MLb+b/+jUTL4o1gsqktteJDRplze6q4dIp4jxnEmmeJdma2yvpgJCuVmTBBWhb e5tJGs4ooC2b6LjZZvmqsdJdc5XME27CMtAGkXzhp2lkOGxE5vy96IQW0VmD9AFoj8qP ifUwBCR2W7eQlxU/6gviKiwZLHKutbikIOJ3FUPrMm3K8IbTxUy65D1pFxmE63u1X4rv fdjEFrcrpTRKXYFBsVnCP80ho8FSMA07VaMrVTRp6hMoPkUx+IDih/9t7S/x+MLy1bxQ kN/HBPHXteIvTKfiWT9VoARkaqcrRres0fXMBLKzSqbtsXQPt8TkCTXkRicRP7mlS4gf KsIQ== X-Forwarded-Encrypted: i=1; AJvYcCVbreDUClECpucdvNqBcsJYDgBkNsOqdyDrK4dBrYvfzfYqL0vx1YZnq9z26UzV7LSj5Mcd7p2hRhlvYELB6eiS8kkSDBaaEIwYAJa38V7EAuP176g= X-Gm-Message-State: AOJu0YydK1s81GsmM9dcTm5xXwI1AprmnoMmgiExpBrkpmgbhNGJ3bj+ KafMRNPKDWcCCrrejtDekPUzU8A+Zyk0RSf1LkmlUE23EbCa/T/tNahyzHIraro= X-Google-Smtp-Source: AGHT+IFyaaQ1OHVKcO48XtLcq1OfECVxsFjh6fMY/zT6Sx7rwkI0+KafbZXkwOwQXI0UPi0u+CFVAQ== X-Received: by 2002:a05:6358:5d8d:b0:17e:b867:cb99 with SMTP id s13-20020a0563585d8d00b0017eb867cb99mr3311133rwm.1.1713967110512; Wed, 24 Apr 2024 06:58:30 -0700 (PDT) Received: from localhost.localdomain ([50.212.55.90]) by smtp.gmail.com with ESMTPSA id h2-20020ac85682000000b004365ab2894asm6120931qta.51.2024.04.24.06.58.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Apr 2024 06:58:29 -0700 (PDT) From: Ben Wolsieffer To: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-stm32@st-md-mailman.stormreply.com, linux-spi@vger.kernel.org Cc: Alexandre Torgue , Maxime Coquelin , Mark Brown , Alain Volmat , Ben Wolsieffer Subject: [PATCH v2] spi: stm32: enable controller before asserting CS Date: Wed, 24 Apr 2024 09:52:38 -0400 Message-ID: <20240424135237.1329001-2-ben.wolsieffer@hefring.com> X-Mailer: git-send-email 2.44.0 MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240424_065834_841149_66D5ED92 X-CRM114-Status: GOOD ( 22.20 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On the STM32F4/7, the MOSI and CLK pins float while the controller is disabled. CS is a regular GPIO, and therefore always driven. Currently, the controller is enabled in the transfer_one() callback, which runs after CS is asserted. Therefore, there is a period where the SPI pins are floating while CS is asserted, making it possible for stray signals to disrupt communications. An analogous problem occurs at the end of the transfer when the controller is disabled before CS is released. This problem can be reliably observed by enabling the pull-up (if CPOL=0) or pull-down (if CPOL=1) on the clock pin. This will cause two extra unintended clock edges per transfer, when the controller is enabled and disabled. Note that this bug is likely not present on the STM32H7, because this driver sets the AFCNTR bit (not supported on F4/F7), which keeps the SPI pins driven even while the controller is disabled. Enabling/disabling the controller as part of runtime PM was suggested as an alternative approach, but this breaks the driver on the STM32MP1 (see [1]). The following quote from the manual may explain this: > To restart the internal state machine properly, SPI is strongly > suggested to be disabled and re-enabled before next transaction starts > despite its setting is not changed. This patch has been tested on an STM32F746 with a MAX14830 UART expander. [1] https://lore.kernel.org/lkml/ZXzRi_h2AMqEhMVw@dell-precision-5540/T/ Signed-off-by: Ben Wolsieffer --- v2: * Improve explanation of problem * Discuss why not to use runtime PM instead drivers/spi/spi-stm32.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/drivers/spi/spi-stm32.c b/drivers/spi/spi-stm32.c index e4e7ddb7524a..4a68abcdcc35 100644 --- a/drivers/spi/spi-stm32.c +++ b/drivers/spi/spi-stm32.c @@ -1016,10 +1016,8 @@ static irqreturn_t stm32fx_spi_irq_event(int irq, void *dev_id) static irqreturn_t stm32fx_spi_irq_thread(int irq, void *dev_id) { struct spi_controller *ctrl = dev_id; - struct stm32_spi *spi = spi_controller_get_devdata(ctrl); spi_finalize_current_transfer(ctrl); - stm32fx_spi_disable(spi); return IRQ_HANDLED; } @@ -1187,6 +1185,8 @@ static int stm32_spi_prepare_msg(struct spi_controller *ctrl, ~clrb) | setb, spi->base + spi->cfg->regs->cpol.reg); + stm32_spi_enable(spi); + spin_unlock_irqrestore(&spi->lock, flags); return 0; @@ -1204,7 +1204,6 @@ static void stm32fx_spi_dma_tx_cb(void *data) if (spi->cur_comm == SPI_SIMPLEX_TX || spi->cur_comm == SPI_3WIRE_TX) { spi_finalize_current_transfer(spi->ctrl); - stm32fx_spi_disable(spi); } } @@ -1219,7 +1218,6 @@ static void stm32_spi_dma_rx_cb(void *data) struct stm32_spi *spi = data; spi_finalize_current_transfer(spi->ctrl); - spi->cfg->disable(spi); } /** @@ -1307,8 +1305,6 @@ static int stm32fx_spi_transfer_one_irq(struct stm32_spi *spi) stm32_spi_set_bits(spi, STM32FX_SPI_CR2, cr2); - stm32_spi_enable(spi); - /* starting data transfer when buffer is loaded */ if (spi->tx_buf) spi->cfg->write_tx(spi); @@ -1345,8 +1341,6 @@ static int stm32h7_spi_transfer_one_irq(struct stm32_spi *spi) spin_lock_irqsave(&spi->lock, flags); - stm32_spi_enable(spi); - /* Be sure to have data in fifo before starting data transfer */ if (spi->tx_buf) stm32h7_spi_write_txfifo(spi); @@ -1378,8 +1372,6 @@ static void stm32fx_spi_transfer_one_dma_start(struct stm32_spi *spi) */ stm32_spi_set_bits(spi, STM32FX_SPI_CR2, STM32FX_SPI_CR2_ERRIE); } - - stm32_spi_enable(spi); } /** @@ -1413,8 +1405,6 @@ static void stm32h7_spi_transfer_one_dma_start(struct stm32_spi *spi) stm32_spi_set_bits(spi, STM32H7_SPI_IER, ier); - stm32_spi_enable(spi); - if (STM32_SPI_HOST_MODE(spi)) stm32_spi_set_bits(spi, STM32H7_SPI_CR1, STM32H7_SPI_CR1_CSTART); }