| 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
| ||
|
Message-ID: <a7070f3f-8857-4834-9e9e-02068637075c@iscas.ac.cn>
Date: Sat, 20 Sep 2025 11:52:49 +0800
From: Vivian Wang <wangruikang@...as.ac.cn>
To: Alex Elder <elder@...cstar.com>, 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
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
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?
> 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)
> + *(u16 *)rx->buf = val;
> + else
> + *(u32 *)rx->buf = val;
Maybe
else if (bytes == 4)
*(u32 *)rx->buf = val;
else
WARN_ON_ONCE(1);
Just to make the pattern consistent? Same for k1_spi_write_word.
> +
> + 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.
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.
> +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.
> [...]
>
> +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.
This range seems to be fixed by hardware, so, it should also be
specified in the bindings.
> +}
> +
>
> [...]
>
> +
> +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