Re: [PATCH v3 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

From: Yixun Lan <dlan@gentoo.org>
To: Alex Elder <elder@riscstar.com>
Cc: broonie@kernel.org, robh@kernel.org, krzk+dt@kernel.org,
	conor+dt@kernel.org, ziyao@disroot.org,
	linux-spi@vger.kernel.org, devicetree@vger.kernel.org,
	paul.walmsley@sifive.com, palmer@dabbelt.com,
	aou@eecs.berkeley.edu, alex@ghiti.fr, p.zabel@pengutronix.de,
	spacemit@lists.linux.dev, linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver
Date: Sun, 28 Sep 2025 20:36:22 +0800	[thread overview]
Message-ID: <20250928123622-GYA1347020@gentoo.org> (raw)
In-Reply-To: <786f4a5e-f62e-4cd0-a017-7b61408f34aa@riscstar.com>

Hi Alex,

On 07:49 Tue 23 Sep     , Alex Elder wrote:
> On 9/22/25 6:06 PM, Yixun Lan wrote:
> > Hi Alex,
> > 
> > On 11:17 Mon 22 Sep     , Alex Elder wrote:
> >> This patch introduces the driver for the SPI controller found in the
> >> SpacemiT K1 SoC.  Currently the driver supports master mode only.
> >> The SPI hardware implements RX and TX FIFOs, 32 entries each, and
> >> supports both PIO and DMA mode transfers.
> >>
> >> Signed-off-by: Alex Elder <elder@riscstar.com>
> >> ---
..
> 
> >> diff --git a/drivers/spi/spi-spacemit-k1.c b/drivers/spi/spi-spacemit-k1.c
> >> new file mode 100644
> >> index 0000000000000..2b932d80cc510
> >> --- /dev/null
> >> +++ b/drivers/spi/spi-spacemit-k1.c
> >> @@ -0,0 +1,965 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Support for SpacemiT K1 SPI controller
> >> + *
> >> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
> >> + * Copyright (c) 2023, spacemit Corporation.
> >> + */
> 
> . . .
> 
> >> +static irqreturn_t k1_spi_ssp_isr(int irq, void *dev_id)
> >> +{
> >> +	struct k1_spi_driver_data *drv_data = dev_id;
> >> +	bool rx_done;
> >> +	bool tx_done;
> >> +	u32 val;
> >> +
> >> +	/* Get status and clear pending interrupts */
> >> +	val = readl(drv_data->base + SSP_STATUS);
> >> +	writel(val, drv_data->base + SSP_STATUS);
> >> +
> >> +	if (!drv_data->message)
> >> +		return IRQ_NONE;
> >> +
> >> +	/* Check for a TX underrun or RX underrun first */
> > s/RX underrun/RX overrun/
> 
> OK.
> 
> >> +	if (val & (SSP_STATUS_TUR | SSP_STATUS_ROR)) {
> >> +		/* Disable all interrupts on error */
> >> +		writel(0, drv_data->base + SSP_INT_EN);
> > should clear status of SSP_STATUS instead of disabling ISR, see commet below
> 
> The status is cleared immediately after reading, above.  We hold
> the status value so we can act on the current state of the FIFOs.
> 
I'm surprised by this, do you mean the status will be cleared by reading?
can you double checck and prove it? by reading twice and compare?

while according to the docs - 18.2.4.6 SSSR REGISTERS, the status bits has
two types: 
  R - Read only
  R/W1C - Read only, write 1 to clear

if you're right, then the docs should be fixed.

-- 
Yixun Lan (dlan)

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv