Re: [PATCH 2/2] spi: qcom-geni: Add property to force GSI mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Forcing GSI mode via `qcom,force-gsi-mode` fails to update the hardware configuration registers (`SE_FIFO_IF_DISABLE` and `SE_DMA_IF_EN`), leaving the hardware and software in inconsistent states.
- [Medium] The new `qcom,force-gsi-mode` property is functionally redundant and bypasses the existing `qcom,enable-gsi-dma` property, muddying the device tree bindings.
--

commit 7902ba1d5ce1661e61400f2f7a40e6f1185b3264
Author: Pengyu Luo <mitltlatltl@gmail.com>

spi: qcom-geni: Add property to force GSI mode

This commit introduces a "qcom,force-gsi-mode" property to force the
controller into GSI mode during initialization for devices that do not
disable FIFO mode by default.

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 26e723cfea61e..eece7312f7805 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
[ ... ]
> @@ -655,6 +656,9 @@ static int spi_geni_init(struct spi_geni_master *mas)
>  		mas->oversampling = 1;
>  
>  	fifo_disable = readl(se->base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
> +	if (mas->force_gsi_mode)
> +		fifo_disable = 1;
> +

[Severity: High]
Does overriding this local variable actually configure the hardware for GSI DMA
mode?

Looking at the hardware initialization, geni_load_se_fw() is the only place
where SE_FIFO_IF_DISABLE and SE_DMA_IF_EN are written. By just overriding
fifo_disable here, the driver will attempt to use GENI_GPI_DMA mode while
the hardware's core FIFO interface might still be enabled and the DMA
interface disabled.

Could this lead to DMA failures, timeouts, or lockups since the software and
hardware states are inconsistent?

>  	switch (fifo_disable) {
>  	case 1:
>  		ret = spi_geni_grab_gpi_chan(mas);
[ ... ]
> @@ -1133,6 +1137,9 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	if (device_property_read_bool(&pdev->dev, "qcom,force-gsi-mode"))
> +		mas->force_gsi_mode = true;
> +

[Severity: Medium]
Is this new property functionally redundant?

The subsystem already supports the qcom,enable-gsi-dma device tree property
to configure GSI DMA mode, which is parsed in geni_load_se_firmware().

Would introducing qcom,force-gsi-mode create overlapping bindings for the
same hardware configuration and pollute the device tree ABI?

It seems this new property might be introduced to work around an internal
software parsing order issue where geni_load_se_firmware() is skipped if the
firmware is already loaded.

>  	ret = spi_geni_init(mas);
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614083424.464132-1-mitltlatltl@gmail.com?part=2