Re: [PATCH v4 2/2] spi: spi-mem: Add Realtek SPI NOR flash controller driver

[Philipp Zabel]

On Mi, 2026-09-09 at 20:57 +0800, Yu-Chun Lin wrote:
> From: Jyan Chou <jyanchou@xxxxxxxxxxx>
> 
> Add a spi-mem driver for the SPI NOR Flash Controller (SFC) found on Realtek
> DHC SoC. SFC supports Dual I/O.
> 
> Implement the exec_op API for regular control commands and the dirmap API
> for hardware-accelerated read/write operations.
> 
> The controller is described by three register ranges. The first one (ctrl)
> holds the control registers. The second one (dma) holds the DMA engine
> registers, used to transfer the payload between the flash and main memory.
> The third range is the memory aperture (dirmap) that maps the flash
> contents.
> 
> Signed-off-by: Jyan Chou <jyanchou@xxxxxxxxxxx>
> Co-developed-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> Signed-off-by: Yu-Chun Lin <eleanor.lin@xxxxxxxxxxx>
> ---
> Changes since v2:
> - Add Yu-Chun's last name.
> - Specify "NOR" in the commit message and descriptions to avoid future
> conflicts with SPI NAND or GSPI.
> - Make SPDX license and copyright notice headers in C++-style.
> - Drop driver-internal polling (rtk_spi_read_status()), leaving the status
> polling and timeout handling to framework.
> - Track the 4-byte addressing state and properly restore it in rtk_spi_init()
> to prevent state loss during runtime PM suspend/resume.
> - Add hardware cleanup in rtk_spi_dma_transfer() to explicitly trigger a
> graceful stop of the DMA engine if a timeout occurs.
> - Check bounds against the direct mapping window (dirmap_size) in both PIO and
> DMA transfer paths.
> - Add reset_control_assert() in the probe error path and .remove().
> - Replace RUNTIME_PM_OPS() + SYSTEM_SLEEP_PM_OPS() with
> DEFINE_RUNTIME_DEV_PM_OPS().
> ---
>  MAINTAINERS               |   6 +
>  drivers/spi/Kconfig       |  10 +
>  drivers/spi/Makefile      |   1 +
>  drivers/spi/spi-rtk-nor.c | 623 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 640 insertions(+)
>  create mode 100644 drivers/spi/spi-rtk-nor.c
> 
[...]
> diff --git a/drivers/spi/spi-rtk-nor.c b/drivers/spi/spi-rtk-nor.c
> new file mode 100644
> index 000000000000..5ccfa234b6b1
> --- /dev/null
> +++ b/drivers/spi/spi-rtk-nor.c
> @@ -0,0 +1,623 @@
[...]
> +static int rtk_spi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct spi_controller *ctrl;
> +	struct rtk_spi_host *host;
> +	struct resource *res;
> +	int ret;
> +
> +	ctrl = devm_spi_alloc_host(dev, sizeof(*host));
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, ctrl);
> +	host = spi_controller_get_devdata(ctrl);
> +	host->dev = dev;
> +
> +	host->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(host->clk))
> +		return PTR_ERR(host->clk);
> +
> +	ret = clk_prepare_enable(host->clk);
> +	if (ret)
> +		return ret;
> +
> +	host->rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(host->rstc)) {
> +		ret = PTR_ERR(host->rstc);
> +		goto err_disable_clk;

If you moved clk_prepare_enable() below this, you could just return
here.

> +	}
> +
> +	reset_control_assert(host->rstc);
> +	usleep_range(10, 20);
> +	reset_control_deassert(host->rstc);
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(35));
> +	if (ret) {
> +		dev_err(dev, "Failed to set dma mask\n");
> +		goto err_disable_clk;

No reset_control_assert() in this error path?

> +	}
> +
> +	host->buffer = dmam_alloc_coherent(dev, SFC_DMA_MAX_LEN,
> +					   &host->dma_buffer, GFP_KERNEL);
> +	if (!host->buffer) {
> +		ret = -ENOMEM;
> +		goto err_disable_clk;
> +	}
> +
> +	host->regbase = devm_platform_ioremap_resource_byname(pdev, "ctrl");
> +	if (IS_ERR(host->regbase)) {
> +		ret = PTR_ERR(host->regbase);
> +		goto err_disable_clk;
> +	}
> +
> +	host->mdbase = devm_platform_ioremap_resource_byname(pdev, "dma");
> +	if (IS_ERR(host->mdbase)) {
> +		ret = PTR_ERR(host->mdbase);
> +		goto err_disable_clk;
> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap");
> +	host->iobase = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(host->iobase)) {
> +		ret = PTR_ERR(host->iobase);
> +		goto err_disable_clk;
> +	}

It looks to me like all these dmam_ and devm_ initialization calls
should be moved up before the clock enable and reset assert/deassert.

> +
> +	host->dirmap_size = resource_size(res);
> +	host->flash_phys_base = res->start;
> +	host->is_4byte = false;
> +
> +	ctrl->mode_bits = SPI_RX_DUAL | SPI_TX_DUAL;
> +	ctrl->bus_num = -1;
> +	ctrl->mem_ops = &rtk_spi_mem_ops;
> +	ctrl->num_chipselect = 1;
> +	ctrl->auto_runtime_pm = true;
> +
> +	rtk_spi_init(host);

This is the first call that actually needs clocks running and reset
triggered.

> +
> +	pm_runtime_set_autosuspend_delay(dev, SFC_AUTOSUSPEND_TIMEOUT);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_noresume(dev);
> +
> +	ret = spi_register_controller(ctrl);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register controller\n");
> +		goto err_pm_disable;
> +	}
> +
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +
> +err_pm_disable:
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_dont_use_autosuspend(dev);
> +	reset_control_assert(host->rstc);
> +
> +err_disable_clk:
> +	clk_disable_unprepare(host->clk);
> +
> +	return ret;
> +}
> +
> +static void rtk_spi_remove(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctrl = platform_get_drvdata(pdev);
> +	struct rtk_spi_host *host = spi_controller_get_devdata(ctrl);
> +	struct device *dev = &pdev->dev;
> +
> +	spi_unregister_controller(ctrl);
> +
> +	pm_runtime_get_sync(dev);
> +	pm_runtime_disable(dev);
> +	reset_control_assert(host->rstc);
> +	clk_disable_unprepare(host->clk);
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_set_suspended(dev);

Why is this cleanup order different from the probe error path?


regards
Philipp