linux-kernel - Re: [PATCH v7 2/3] spi: spacemit: introduce SpacemiT K1 SPI controller driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <963990a3-d39c-4e72-8add-02d98f59770a@riscstar.com>
Date: Thu, 20 Nov 2025 09:51:00 -0600
From: Alex Elder <elder@...cstar.com>
To: Mark Brown <broonie@...nel.org>
Cc: dlan@...too.org, p.zabel@...gutronix.de, linux-spi@...r.kernel.org,
 spacemit@...ts.linux.dev, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/3] spi: spacemit: introduce SpacemiT K1 SPI
 controller driver

On 11/16/25 12:19 PM, Mark Brown wrote:
> On Fri, Nov 14, 2025 at 12:57:43PM -0600, 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.

Sorry for the delay responding to this.  You provided some really
great feedback and I appreciate it.

> This looks mostly good but there's a bit of open coding that looks like
> the driver could make more use of the core.
> 
>> @@ -0,0 +1,966 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * SpacemiT K1 SPI controller driver
>> + *
>> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
>> + * Copyright (c) 2023, spacemit Corporation.
>> + */
> 
> Please make the entire comment a C++ one so things look more
> intentional.

I do see in drivers/spi/ files that start just like above,
and others that start with all "//" comments (more of the
former than the latter).  I really don't prefer using "//"
for anything but the SPDX ID, but... since you requested
it I will make that change.

>> +static bool k1_spi_map_dma_buffer(struct k1_spi_io *io, size_t len, void *dummy)
>> +{
>> +	struct device *dmadev = io->chan->device->dev;
>> +	unsigned int nents = DIV_ROUND_UP(len, SZ_2K);
>> +	struct sg_table *sgt = &io->sgt;
>> +	void *bufp = io->buf ? : dummy;
>> +	struct scatterlist *sg;
>> +	unsigned int i;
> 
> The SPI core can do DMA mapping for you, the only thing this is doing
> that's unusual is that it's imposing a fixed 2K limit on block sizes.

OK.  I will use the DMA support provided by the core.

> If this limit comes from the DMA controller (which looks to be the case
> since we feed the entire table into the DMA controller at once?) the
> core will already DTRT here, assuming the DMA controller correctly
> advertises this restriction.

I will verify this.

>> +static bool k1_spi_map_dma_buffers(struct k1_spi_driver_data *drv_data)
>> +{
> 
> ...
> 
>> +	/* Don't bother with DMA if we can't do even a single burst */
>> +	if (drv_data->len < dma_burst_size)
>> +		return false;
>> +
>> +	/* We won't use DMA if the transfer is too big, either */
>> +	if (drv_data->len > K1_SPI_MAX_DMA_LEN)
>> +		return false;
> 
> The core has a can_dma() callback for this.
> 
>> +static int k1_spi_transfer_one_message(struct spi_controller *host,
>> +					   struct spi_message *message)
>> +{
> 
> ...
> 
>> +	/* Hold frame low to avoid losing transferred data */
>> +	val = readl(drv_data->base + SSP_TOP_CTRL);
>> +	val |= TOP_HOLD_FRAME_LOW;
>> +	writel(val, drv_data->base + SSP_TOP_CTRL);
> 
> This looks like it should be a set_cs() operation?

I'll implement that, along with using spi_transfer_one_message().

>> +	list_for_each_entry(transfer, &message->transfers, transfer_list) {
>> +		reinit_completion(completion);
>> +
>> +		/* Issue the next transfer */
>> +		if (!k1_spi_transfer_start(drv_data, transfer)) {
>> +			message->status = -EIO;
>> +			break;
>> +		}
>> +
>> +		k1_spi_transfer_wait(drv_data);
>> +
>> +		k1_spi_transfer_end(drv_data, transfer);
> 
> Why not just implement the transfer_one() callback?  This just looks
> like it's duplicating code.

I'm working on this now, and it's the reason for the delay.
Methodically switching things over to the generic interface
has been taking some time, but I think I'm close now.

>> +static irqreturn_t k1_spi_ssp_isr(int irq, void *dev_id)
>> +{
> 
>> +	/* Get status and clear pending interrupts */
>> +	val = readl(drv_data->base + SSP_STATUS);
>> +	writel(val, drv_data->base + SSP_STATUS);
> 
> This unconditionally acknowledges all interrupts even if we didn't
> handle anything...This is a good observation.

There are only 6 interrupt conditions that get cleared.  Three
are errors, and the other three are read/write FIFO "ready"
interrupts.  The code that follows handles all of those, so
doing this right away was a sort of shorthand.

That said, there was a chance for an early return (if the
message pointer was null), and that should be checked before
we clear the status register.

In any case, my work the last day or so has included a lot
of tweaks to the handler.  I'll try to make it clear what's
done makes sense.

Thank you very much for the review.  I wish I had looked
harder at just using spi_transfer_one_message() before.
But I *love* suggestions that will make the code become
smaller and simpler.

					-Alex