Message-ID: <3c9aaa62-f685-47f7-a21c-00f51550f185@riscstar.com>
Date: Sat, 20 Sep 2025 10:59:58 -0500
From: Alex Elder <elder@...cstar.com>
To: Vivian Wang <wangruikang@...as.ac.cn>, broonie@...nel.org,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Cc: dlan@...too.org, ziyao@...root.org, linux-spi@...r.kernel.org,
 devicetree@...r.kernel.org, paul.walmsley@...ive.com, palmer@...belt.com,
 aou@...s.berkeley.edu, alex@...ti.fr, p.zabel@...gutronix.de,
 spacemit@...ts.linux.dev, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] spi: spacemit: introduce SpacemiT K1 SPI
 controller driver

On 9/19/25 10:52 PM, Vivian Wang wrote:
> Hi Alex,
> 
> On 9/19/25 23:59, Alex Elder wrote:
>> [...]
>>
>> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
>> index 82fa5eb3b8684..4f6c446c6bc16 100644
>> --- a/drivers/spi/Kconfig
>> +++ b/drivers/spi/Kconfig
>> @@ -1071,6 +1071,14 @@ config SPI_SG2044_NOR
>>   	  also supporting 3Byte address devices and 4Byte address
>>   	  devices.
>>   
>> +config SPI_SPACEMIT_K1
>> +	tristate "K1 SPI Controller"
>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	depends on OF
>> +	default ARCH_SPACEMIT
>> +	help
>> +	  Enable support for the SpacemiT K1 SPI controller.
>> +
> 
> We could still add:
> 
> 	imply MMP_PDMA if ARCH_SPACEMIT

I have never used "imply", I guess it's new (as of 2016)...

This sounds like a good suggestion.  This would mean MMP_PDMA
would by default take the same value as SPI_SPACEMIT_K1 (if
the former's dependencies were met), while allowing it to be
deselected for the configuration.
  > To add a "recommended" dependency. This way, enabling SPI_SPACEMIT_K1
> automatically enables MMP_PDMA, but if the user is willing to fiddle
> around, they can explicitly disable it. What do you think?

I like it.

>>   config SPI_SPRD
>>   	tristate "Spreadtrum SPI controller"
>>   	depends on ARCH_SPRD || COMPILE_TEST
>>
>> [...]
>>
>> diff --git a/drivers/spi/spi-spacemit-k1.c b/drivers/spi/spi-spacemit-k1.c
>> new file mode 100644
>> index 0000000000000..8d564fe6c4303
>> --- /dev/null
>> +++ b/drivers/spi/spi-spacemit-k1.c
>> @@ -0,0 +1,968 @@
>>
>> [...]
>>
>> +static void k1_spi_read_word(struct k1_spi_driver_data *drv_data)
>> +{
>> +	struct k1_spi_io *rx = &drv_data->rx;
>> +	u32 bytes = drv_data->bytes;
>> +	u32 val;
>> +
>> +	val = readl(drv_data->base + SSP_DATAR);
>> +	rx->resid -= bytes;
>> +
>> +	if (!rx->buf)
>> +		return;	/* Null reader: discard the data */
>> +
>> +	if (bytes == 1)
>> +		*(u8 *)rx->buf = val;
>> +	else if (bytes == 1)
> 
> Typo? else if (bytes == 2)

Wow.  Yes that is an error that I'll correct.

>> +		*(u16 *)rx->buf = val;
>> +	else
>> +		*(u32 *)rx->buf = val;
> 
> Maybe
> 
> 	else if (bytes == 4)
> 		*(u32 *)rx->buf = val;
> 	else
> 		WARN_ON_ONCE(1);

The value of bytes will be 1, 2, or 4, which we can tell
by inspection.  At one time I had a switch statement with
a default, but I decided to leave out the default, which
won't happen.

> Just to make the pattern consistent? Same for k1_spi_write_word.

Consistent with what?

>> +	rx->buf += bytes;
>> +}
>>
>> [...]
>>
>> +static int k1_spi_dma_setup(struct k1_spi_driver_data *drv_data)
>> +{
>> +	struct device *dev = drv_data->dev;
>> +	int rx_ret;
>> +	int tx_ret;
>> +
>> +	/* We must get both DMA channels, or neither of them */
>> +	rx_ret = k1_spi_dma_setup_io(drv_data, true);
>> +	if (rx_ret == -EPROBE_DEFER)
>> +		return -EPROBE_DEFER;
>> +
>> +	tx_ret = k1_spi_dma_setup_io(drv_data, false);
>> +
>> +	/* If neither is specified, we don't use DMA */
>> +	if (rx_ret == -ENODEV && tx_ret == -ENODEV)
>> +		return 0;
>> +
>> +	if (rx_ret || tx_ret)
>> +		goto err_cleanup;
> 
> This seems a bit convoluted. I'm wondering if all this explicit handling
> really is necessary - if we get an error at probe time, can we just
> return that error and let devres handle the rest? With the special case
> that if we get both -ENODEV then disable DMA.

I agree it seems it should be less complex.

I'm trying to ensure that both channels are set up, or
that neither channel is set up, or that we try again if
we get -EPROBE_DEFER.  And if there's something wrong
with the configuration (only one of TX and RX is set up
successfully), an error occurs.

RX		TX		Result
--		--		------
0		0		0	(DMA)
-ENODEV		-ENODEV		0	(PIO)
-EPROBE_DEFER	(anything)	-EPROBE_DEFER (try again)
(anything)	-EPROBE_DEFER	-EPROBE_DEFER (try again)
0		-ENODEV		-ENODEV	(error, abort probe)
-ENODEV		0		-ENODEV	(error, abort probe)
error		(anything)	error	(error, abort probe)
(anything)	error		error	(error, abort probe)

Finally, if the buffer allocation fails:
0		0		0	(PIO; clean up TX and RX)

Let me think about this.  I'll see if I can find a simpler way
to achieve the above result, relying on devres to clean things
up.  I'd have to change k1_spi_dma_cleanup(), but you might be
right that it could be done more simply.

> k1_spi_dma_cleanup_io seems to handle unmapping and termination of DMA,
> which we... don't need, right?
> 
>> +
>> +	drv_data->dummy = devm_kzalloc(dev, SZ_2K, GFP_KERNEL);
>> +	if (drv_data->dummy)
>> +		return 0;		/* Success! */
>> +
>> +	dev_warn(dev, "error allocating DMA dummy buffer; DMA disabled\n");
> 
> Just return -ENOMEM. If we can't even allocate 2K of buffer, we're
> doomed anyway.

You're generally right, but I don't want my code to assume that.

>> +err_cleanup:
>> +	if (tx_ret) {
>> +		if (tx_ret != -EPROBE_DEFER)
>> +			dev_err(dev, "error requesting DMA TX DMA channel\n");
>> +	} else {
>> +		k1_spi_dma_cleanup_io(drv_data, false);
>> +	}
>> +
>> +	if (rx_ret)
>> +		dev_err(dev, "error requesting DMA RX DMA channel\n");
>> +	else
>> +		k1_spi_dma_cleanup_io(drv_data, true);
>> +
>> +	if (tx_ret == -EPROBE_DEFER)
>> +		return -EPROBE_DEFER;
>> +
>> +	/* Return success if we don't get the dummy buffer; PIO will be used */
>> +
>> +	return rx_ret ? : tx_ret ? : 0;
>> +}
>>
>> [...]
>>
>> +static int devm_k1_spi_dma_setup(struct k1_spi_driver_data *drv_data)
>> +{
>> +	struct k1_spi_driver_data **ptr;
>> +	int ret;
>> +
>> +	if (!IS_ENABLED(CONFIG_MMP_PDMA)) {
>> +		dev_warn(drv_data->dev, "DMA not available; using PIO\n");
>> +		return 0;
>> +	}
>> +
> 
> Shouldn't be necessary if we do the "imply" thing in Kconfig.

The messages I provide are based on an assumption that using
DMA is desirable and it will normally be used by this driver.
So if it won't be used, I'd like to provide that information.

On the other hand, I don't issue a warning if neither of
the channels is configured in the DTB.

I'm not going to commit either way on keeping/removing this.
If someone else weighs in I'm open to changing it.

>> [...]
>>
>> +static void k1_spi_host_init(struct k1_spi_driver_data *drv_data)
>> +{
>>
>> [...]
>>
>> +
>> +	ret = of_property_read_u32(np, "spi-max-frequency", &max_speed_hz);
>> +	if (!ret) {
>> +		host->max_speed_hz = clamp(max_speed_hz, K1_SPI_MIN_SPEED_HZ,
>> +					   K1_SPI_MAX_SPEED_HZ);
>> +		if (host->max_speed_hz != max_speed_hz)
>> +			dev_warn(dev, "spi-max-frequency %u out of range, using %u\n",
>> +				max_speed_hz, host->max_speed_hz);
>> +	} else {
>> +		if (ret != -EINVAL)
>> +			dev_warn(dev, "bad spi-max-frequency, using %u\n",
>> +				 K1_SPI_DEFAULT_MAX_SPEED_HZ);
>> +		host->max_speed_hz = K1_SPI_DEFAULT_MAX_SPEED_HZ;
>> +	}
> 
> I think it makes sense to have spi-max-frequency default to
> K1_SPI_DEFAULT_MAX_SPEED_HZ, but if the value is out of range just print
> a message and return an error, to get whoever wrote the bad value to fix it.

These errors just shouldn't happen.  But the way I handle this,
it allows the SPI controller to still be used, even if the
administrator can't really update the DTB.

> This range seems to be fixed by hardware, so, it should also be
> specified in the bindings.

I define the hardware limits here, and enforce
them, in case the bindings specify an out-of-range
value.  Again, this is an error that just shouldn't
occur in practice, but the code is defensive.

Most of your comments really made me think about how
errors are handled.  I appreciate it.

					-Alex

> 
>> +}
>> +
>>
>> [...]
>>
>> +
>> +static int k1_spi_probe(struct platform_device *pdev)
>> +{
>> +	struct k1_spi_driver_data *drv_data;
>> +	struct device *dev = &pdev->dev;
>> +	struct reset_control *reset;
>> +	struct spi_controller *host;
>> +	struct resource *iores;
>> +	struct clk *clk_bus;
>> +	int ret;
>> +
>> +	host = devm_spi_alloc_host(dev, sizeof(*drv_data));
>> +	if (!host)
>> +		return -ENOMEM;
>> +	drv_data = spi_controller_get_devdata(host);
>> +	drv_data->controller = host;
>> +	platform_set_drvdata(pdev, drv_data);
>> +	drv_data->dev = dev;
>> +	init_completion(&drv_data->completion);
>> +
>> +	drv_data->base = devm_platform_get_and_ioremap_resource(pdev, 0,
>> +								&iores);
> 
> Maybe
> 
>      devm_platform_ioremap_resource(pdev, 0);
> 
>> [...]
>>
>> +
>> +MODULE_DESCRIPTION("SpacemiT K1 SPI controller driver");
>> +MODULE_LICENSE("GPL");
> 
> Maybe MODULE_AUTHOR()?
> 
> Vivian "dramforever" Wang
> 

Powered by blists - more mailing lists