Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Goud, Srinivas" <srinivas.goud@amd.com>
To:     Charles Keepax <ckeepax@opensource.cirrus.com>,
        "broonie@kernel.org" <broonie@kernel.org>
CC:     "linux-spi@vger.kernel.org" <linux-spi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "patches@opensource.cirrus.com" <patches@opensource.cirrus.com>
Subject: RE: [PATCH 1/2] spi: spi-cadence: Avoid read of RX FIFO before its
 ready
Thread-Topic: [PATCH 1/2] spi: spi-cadence: Avoid read of RX FIFO before its
 ready
Thread-Index: AQHZgpUsoHY1RY/m3UymshDymAXTqq9bRGXA
Date:   Mon, 15 May 2023 12:04:38 +0000
Message-ID: <PH8PR12MB6675F6CC30254C5EED898FD5E1789@PH8PR12MB6675.namprd12.prod.outlook.com>
References: <20230509164153.3907694-1-ckeepax@opensource.cirrus.com>
In-Reply-To: <20230509164153.3907694-1-ckeepax@opensource.cirrus.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?BtVS6iGk/Yh8CYTd62VgQRjvC/XXbI+lIcZPddeIzx3nZ5NbCQadUE+GDwlV?=
 =?us-ascii?Q?Gx4WgeMOa/GHFYmxyJWnNoQ1was3R4AhjsmLGAK8+MYX7B+2mLLuFDU+fg4d?=
 =?us-ascii?Q?EkfL2TjEDsruz26FboEt8K464j9T7F4JoEvPPYCwfZ3hhsleyHWrYMLn8NvD?=
 =?us-ascii?Q?BXlYlnPI/CeNrndMQElmYsK1Iax68RcUKPwePGEh9a+TH/SqSuF0W1grPShI?=
 =?us-ascii?Q?ng1Qq3Psws3DvnNPgEsgldLxEpkTd/11Gjix2BZRmv0G4Sxr23oUj6IX9GsH?=
 =?us-ascii?Q?yRtHpfFoYxcJljVLlzQPed3rM7xtE6mgtItkvxskAXKqGVjoCKUxRkJ7B1o6?=
 =?us-ascii?Q?cKmK1WYpH6okeKuyZ2e0OP0bXNiu1A+HfnAIQtcoO1+0/zkw0UTrQeCv5srP?=
 =?us-ascii?Q?mjd6RGZX32AaEx+pnjiVBrtFZoHfIfLkCQcP5Y5ab56S2OVHSspZ2S5EalBa?=
 =?us-ascii?Q?LMVnJhZdu+zKOSG27xh0i2vVVr1FVYxLZw2QO7EwwEFJtSrbOgFpGtiKvd1V?=
 =?us-ascii?Q?PcDoIgRH2J5NqsMcxfrC6i3jeeQTYPXvDQwofYYlHjtJF9D6WdevGbSOUzir?=
 =?us-ascii?Q?Ytk7C69fVdF8yYrRIHK2BW2wmXOU1FzTYh3gkMAxQ71aXDqX1UzVeH+vzFGQ?=
 =?us-ascii?Q?DwG+QYZF+PkW62MT2gm0Q1YiYjg399PQXBBoagTIQBDqHQbgWshkoonbnCOa?=
 =?us-ascii?Q?B8nPq8w51zFU5h5Hk47wquIY8Xejmec4hgvg6PSCSvRoBJNToyZBUQWxqztW?=
 =?us-ascii?Q?9B237d5i239kPaZC8CsdaADqNZI1kQJBXYeRym0FhvdD0BOQJXiySRcbFiRs?=
 =?us-ascii?Q?c6qRfNh9DO56N9daHkkOBTLCGg7OyCzvF/mZGJyzYX6EiDoiGnUettOD/wFs?=
 =?us-ascii?Q?hipOzWtCnAowlG9xQiwaly5fy7V/VN3xgfCXlGV5S8q7i++cVw4HZs01GUTG?=
 =?us-ascii?Q?Hk/KzLemlS3355wXzJ9Yjoa/jQUJa6QGgReuUCzkkQXi/rX/WYUo0qF5xeKb?=
 =?us-ascii?Q?W8Ep/9EYAyLl1Z8Ar5nMNi44Po5GcLnU5pIVI+94E/+jl8loW+nh2uGr6Tyo?=
 =?us-ascii?Q?WGfIhDUQ7DauBRmV7ZgvmVedvAjwghwCzYQRrM1ipljhpULebruzkQEdy+zG?=
 =?us-ascii?Q?lcVhuEIPSWuBtZT3WbL7PZwJSxN42h4oO7FbGmonL+Df34VMYdVUOKn4z0s5?=
 =?us-ascii?Q?fKcJkiGajz3QAUP7oQLOKZrB5Q3foX8IvgEBMJekozaxsJez0rey+FlBq3FX?=
 =?us-ascii?Q?aDof7SkJRkqWz6rgLaKUA5M8wapvzCmcsVHZ7DTakB2uQBkYEkY7VzjwWY5w?=
 =?us-ascii?Q?va2ls5MjnmLk9sBxIJJT9E2JmIk4Q/KGDXg6JW2yNR5yK+wA2emcTmltG6V9?=
 =?us-ascii?Q?iuhE4gWS+okKBn3mYxhqop5S87I3NfsFRSGHjGaaycq1Vlj5Mh5y1GiBN8JD?=
 =?us-ascii?Q?eNj3YZ34kXlt0uYAIkQSIf1XHtnl27mtEq30Bqp0lK7bH1wgx2//alC71voy?=
 =?us-ascii?Q?+E6udeTYEu9QFctfZ4kYI+2HgEVDC3iCQHfiKoVgJwaP+2Yxyhu+yE6Q7aYV?=
 =?us-ascii?Q?TC/JvWfHEnhDTSvayUU=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH8PR12MB6675.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b5a25430-d4be-4fed-ede1-08db553c8ed4
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 May 2023 12:04:38.5419
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: jq5Lse04CfFYbu/xQqZ/oHp2ne1v+uKzUi0+HN88gdbBzu2ndQoRrAmd6iOBMDmM
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB7369
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

>-----Original Message-----
>From: Charles Keepax <ckeepax@opensource.cirrus.com>
>Sent: Tuesday, May 9, 2023 10:12 PM
>To: broonie@kernel.org
>Cc: Goud, Srinivas <srinivas.goud@amd.com>; linux-spi@vger.kernel.org;
>linux-kernel@vger.kernel.org; patches@opensource.cirrus.com
>Subject: [PATCH 1/2] spi: spi-cadence: Avoid read of RX FIFO before its re=
ady
>
>Recent changes to cdns_spi_irq introduced some issues.
>
>Firstly, when writing the end of a longer transaction, the code in cdns_sp=
i_irq
>will write data into the TX FIFO, then immediately fall into the if (!xspi=
-
>>tx_bytes) path and attempt to read data from the RX FIFO. However this
>required waiting for the TX FIFO to empty before the RX data was ready.
>
>Secondly, the variable trans_cnt is now rather inaccurately named since in
>cases, where the watermark is set to 1, trans_cnt will be
>1 but the count of bytes transferred would be much longer.
>
>Finally, when setting up the transaction we set the watermark to 50% of th=
e
>FIFO if the transaction is great than 50% of the FIFO. However, there is n=
o need
>to split a tranaction that is smaller than the whole FIFO, so anything up =
to the
>FIFO size can be done in a single transaction.
>
>Tidy up the code a little, to avoid repeatedly calling cdns_spi_read_rx_fi=
fo with
>a count of 1, and correct the three issues noted above.
>
>Fixes: b1b90514eaa3 ("spi: spi-cadence: Add support for Slave mode")
>Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
>---
> drivers/spi/spi-cadence.c | 42 ++++++++++++++-------------------------
> 1 file changed, 15 insertions(+), 27 deletions(-)
>
>diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c index
>ac85d55622127..b0ccb138e3566 100644
>--- a/drivers/spi/spi-cadence.c
>+++ b/drivers/spi/spi-cadence.c
>@@ -304,13 +304,11 @@ static int cdns_spi_setup_transfer(struct spi_device
>*spi,
>  * cdns_spi_fill_tx_fifo - Fills the TX FIFO with as many bytes as possib=
le
>  * @xspi:	Pointer to the cdns_spi structure
>  */
>-static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi)
>+static void cdns_spi_fill_tx_fifo(struct cdns_spi *xspi, unsigned int
>+avail)
> {
> 	unsigned long trans_cnt =3D 0;
>
>-	while ((trans_cnt < xspi->tx_fifo_depth) &&
>-	       (xspi->tx_bytes > 0)) {
>-
>+	while ((trans_cnt < avail) && (xspi->tx_bytes > 0)) {
> 		/* When xspi in busy condition, bytes may send failed,
> 		 * then spi control did't work thoroughly, add one byte delay
> 		 */
>@@ -381,33 +379,23 @@ static irqreturn_t cdns_spi_irq(int irq, void *dev_i=
d)
> 		spi_finalize_current_transfer(ctlr);
> 		status =3D IRQ_HANDLED;
> 	} else if (intr_status & CDNS_SPI_IXR_TXOW) {
>-		int trans_cnt =3D cdns_spi_read(xspi, CDNS_SPI_THLD);
>+		int threshold =3D cdns_spi_read(xspi, CDNS_SPI_THLD);
>+		int trans_cnt =3D xspi->rx_bytes - xspi->tx_bytes;
>+
>+		if (threshold > 1)
>+			trans_cnt -=3D threshold;
>+
> 		/* Set threshold to one if number of pending are
> 		 * less than half fifo
> 		 */
> 		if (xspi->tx_bytes < xspi->tx_fifo_depth >> 1)
> 			cdns_spi_write(xspi, CDNS_SPI_THLD, 1);
>
>-		while (trans_cnt) {
>-			cdns_spi_read_rx_fifo(xspi, 1);
>-
>-			if (xspi->tx_bytes) {
>-				if (xspi->txbuf)
>-					cdns_spi_write(xspi, CDNS_SPI_TXD,
>-						       *xspi->txbuf++);
>-				else
>-					cdns_spi_write(xspi, CDNS_SPI_TXD,
>0);
>-				xspi->tx_bytes--;
>-			}
>-			trans_cnt--;
>-		}
>-		if (!xspi->tx_bytes) {
>-			/* Fixed delay due to controller limitation with
>-			 * RX_NEMPTY incorrect status
>-			 * Xilinx AR:65885 contains more details
>-			 */
>-			udelay(10);
>-			cdns_spi_read_rx_fifo(xspi, xspi->rx_bytes);
>+		cdns_spi_read_rx_fifo(xspi, trans_cnt);
Cadence SPI configured in Slave mode,  when threshold is half of FIFO depth=
 cdns_spi_read_rx_fifo() function continuously in read mode,=20
due to this we see incorrect data received on the Master side as Slave is f=
ailed to update the TX FIFO on time.

>+
>+		if (xspi->tx_bytes) {
>+			cdns_spi_fill_tx_fifo(xspi, trans_cnt);
>+		} else {
> 			cdns_spi_write(xspi, CDNS_SPI_IDR,
> 				       CDNS_SPI_IXR_DEFAULT);
> 			spi_finalize_current_transfer(ctlr);
>@@ -456,10 +444,10 @@ static int cdns_transfer_one(struct spi_controller
>*ctlr,
> 	/* Set TX empty threshold to half of FIFO depth
> 	 * only if TX bytes are more than half FIFO depth.
> 	 */
>-	if (xspi->tx_bytes > (xspi->tx_fifo_depth >> 1))
>+	if (xspi->tx_bytes > xspi->tx_fifo_depth)
> 		cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >>
>1);
>
>-	cdns_spi_fill_tx_fifo(xspi);
>+	cdns_spi_fill_tx_fifo(xspi, xspi->tx_fifo_depth);
> 	spi_transfer_delay_exec(transfer);
>
> 	cdns_spi_write(xspi, CDNS_SPI_IER, CDNS_SPI_IXR_DEFAULT);
>--
>2.30.2

Regards
Srinivas=20