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

[Alex Elder]

On 9/28/25 7:36 AM, Yixun Lan wrote:
> 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@xxxxxxxxxxxx>
> > > > ---
> ..
> > > > 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.

The code is earlier in that function.  I'll repeat it here.

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);

=== Line above reads the status register
=== Next line clears all status bits, acknowledging interrupts

        writel(val, drv_data->base + SSP_STATUS);

        if (!drv_data->message)
                return IRQ_NONE;

        /* Check for a TX underrun or RX underrun first */
        if (val & (SSP_STATUS_TUR | SSP_STATUS_ROR)) {
                /* Disable all interrupts on error */
                writel(0, drv_data->base + SSP_INT_EN);

=== And then the above line disables all interrupts if one of the
    error conditions was shown in the status.

I hope I'm not missing something.  I think you might have
misunderstood what I was referring to by "above" when I said
"The status is cleared immediately after reading, above."

					-Alex