[PATCH v2 0/4] spi: add support for Amlogic A9

[PATCH v2 0/4] spi: add support for Amlogic A9

[Xianwei Zhao via B4 Relay]

Add bindings for A9 with some features, and driver for A9 base on A4.

Fix the incorrect keep_ss of the last descriptor.

Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
Changes in v2:
- Move the added properties defined to top-level and then reestrict them  in bindings.
- Fix some pre-exiting bug reported by Sashiko AI review that as a separate submission.
- Limit the value that will be writed to CFG_CS_HOLD.
- Link to v1: https://lore.kernel.org/r/20260717-a9-spisg-v1-0-2eeea77be19f@amlogic.com

---
Sunny Luo (1):
      spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

Xianwei Zhao (3):
      spi: dt-bindings: amlogic: spisg: Document A9-specific properties
      spi: amlogic: spisg: Fix transfer size limit and timeout handling
      spi: amlogic: spisg: Add support for A9 controller features

 .../devicetree/bindings/spi/amlogic,a4-spisg.yaml  | 38 ++++++++-
 drivers/spi/spi-amlogic-spisg.c                    | 90 +++++++++++++++++++---
 2 files changed, 113 insertions(+), 15 deletions(-)
---
base-commit: d0b5dd16226814e5c73ba8daf88d3d91c990a38f
change-id: 20260715-a9-spisg-f19ce0403a50

Best regards,
-- 
Xianwei Zhao <xianwei.zhao@amlogic.com>

[PATCH v2 1/4] spi: dt-bindings: amlogic: spisg: Document A9-specific properties

[Xianwei Zhao via B4 Relay]

From: Xianwei Zhao <xianwei.zhao@amlogic.com>

Document the Amlogic A9 SPISG controller and its additional
Device Tree properties.

Add the "amlogic,a9-spisg" compatible string and define the
A9-specific "amlogic,mo-idle-output" and "amlogic,word-gap"
properties, which configure the MOSI idle output level and
the inter-word gap, respectively.

Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 .../devicetree/bindings/spi/amlogic,a4-spisg.yaml  | 38 +++++++++++++++++++---
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml b/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml
index 9bfb8089f7ea..ddef92f3f3b6 100644
--- a/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml
+++ b/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml
@@ -11,12 +11,11 @@ maintainers:
   - Xianwei Zhao <xianwei.zhao@amlogic.com>
   - Sunny Luo <sunny.luo@amlogic.com>
 
-allOf:
-  - $ref: spi-controller.yaml#
-
 properties:
   compatible:
-    const: amlogic,a4-spisg
+    enum:
+      - amlogic,a4-spisg
+      - amlogic,a9-spisg
 
   reg:
     maxItems: 1
@@ -35,6 +34,23 @@ properties:
   resets:
     maxItems: 1
 
+  amlogic,mo-idle-output:
+    description: |
+      Controls the MOSI output level when the controller is idle.
+      0 - Drive MOSI low
+      1 - Drive MOSI high
+      2 - Follow the last transmitted bit
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2]
+    default: 0
+
+  amlogic,word-gap:
+    description: |
+      Number of idle clock cycles inserted between consecutive SPI words.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3]
+    default: 1
+
 required:
   - compatible
   - reg
@@ -42,6 +58,20 @@ required:
   - clocks
   - clock-names
 
+allOf:
+  - $ref: spi-controller.yaml#
+  - if:
+      not:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - amlogic,a9-spisg
+    then:
+      properties:
+        amlogic,mo-idle-output: false
+        amlogic,word-gap: false
+
 unevaluatedProperties: false
 
 examples:

-- 
2.52.0

Re: [PATCH v2 1/4] spi: dt-bindings: amlogic: spisg: Document A9-specific properties

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Setting `default` values on properties that are conditionally forbidden (`false`) causes device tree schema validation to fail on valid older nodes.
--

--- Patch [1]: [PATCH v2 1/4] spi: dt-bindings: amlogic: spisg: Document A9-specific properties ---
commit 5cdfe189c3095f9606a5c72f8f2856e0d4d26388
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>

spi: dt-bindings: amlogic: spisg: Document A9-specific properties

This commit documents the Amlogic A9 SPISG controller and adds the
"amlogic,a9-spisg" compatible string. It also defines the A9-specific
"amlogic,mo-idle-output" and "amlogic,word-gap" properties.

> diff --git a/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml b/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml
> index 9bfb8089f7ea2..ddef92f3f3b66 100644
> --- a/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml
> +++ b/Documentation/devicetree/bindings/spi/amlogic,a4-spisg.yaml
[ ... ]
> @@ -35,6 +34,23 @@ properties:
>    resets:
>      maxItems: 1
>  
> +  amlogic,mo-idle-output:
[ ... ]
> +    enum: [0, 1, 2]
> +    default: 0
> +
> +  amlogic,word-gap:
[ ... ]
> +    enum: [0, 1, 2, 3]
> +    default: 1
> +
[ ... ]
> +allOf:
> +  - $ref: spi-controller.yaml#
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              enum:
> +                - amlogic,a9-spisg
> +    then:
> +      properties:
> +        amlogic,mo-idle-output: false
> +        amlogic,word-gap: false

[Severity: Medium]
Does setting a default value on properties that are conditionally forbidden
cause device tree schema validation to fail on valid older nodes?

The dtschema validation tool injects top-level default properties into a
parsed node before evaluating allOf blocks. For the amlogic,a4-spisg
compatible, it will inject the defaults (0 and 1) for the missing
properties. 

Then the allOf block will enforce the false schema constraint. Since the
properties were injected, the schema evaluates to false, causing validation
errors for any existing or example a4-spisg node.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-a9-spisg-v2-0-9fc39106b7e2@amlogic.com?part=1

[PATCH v2 2/4] spi: amlogic: spisg: Fix transfer size limit and timeout handling

[Xianwei Zhao via B4 Relay]

From: Xianwei Zhao <xianwei.zhao@amlogic.com>

The CFG_BLOCK_NUM field can encode at most 20 bits, so reduce
SPISG_BLOCK_MAX to 0xfffff to avoid programming a zero-length transfer.

Perform the delay calculation in 64-bit arithmetic to avoid overflow
when converting nanoseconds to SPI clock cycles.

Stop the controller on transfer timeout by clearing the descriptor list
register before returning an error.

Fixes: cef9991e04ae ("spi: Add Amlogic SPISG driver")
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 drivers/spi/spi-amlogic-spisg.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
index afc8af04638d..a6123975a37c 100644
--- a/drivers/spi/spi-amlogic-spisg.c
+++ b/drivers/spi/spi-amlogic-spisg.c
@@ -95,7 +95,7 @@
 
 #define SPISG_MAX_REG			0x40
 
-#define SPISG_BLOCK_MAX			0x100000
+#define SPISG_BLOCK_MAX			0xFFFFF
 
 #define SPISG_OP_MODE_WRITE_CMD		0
 #define SPISG_OP_MODE_READ_STS		1
@@ -175,7 +175,7 @@ static int spi_delay_to_sclk(u32 slck_speed_hz, struct spi_delay *delay)
 	if (ns < 0)
 		return 0;
 
-	return DIV_ROUND_UP_ULL(slck_speed_hz * ns, NSEC_PER_SEC);
+	return DIV_ROUND_UP_ULL((u64)slck_speed_hz * ns, NSEC_PER_SEC);
 }
 
 static inline u32 aml_spisg_sem_down_read(struct spisg_device *spisg)
@@ -565,10 +565,13 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
 	aml_spisg_pending(spisg, descs_paddr, false, true);
 	if (wait_for_completion_timeout(&spisg->completion,
 					spi_controller_is_target(spisg->controller) ?
-					MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(ms)))
+					MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(ms))) {
 		ret = spisg->status ? -EIO : 0;
-	else
+	} else {
+		/* stop transfer */
+		regmap_write(spisg->map, SPISG_REG_DESC_LIST_H, 0);
 		ret = -ETIMEDOUT;
+	}
 
 	dma_unmap_single(dev, descs_paddr, descs_len, DMA_TO_DEVICE);
 end:

-- 
2.52.0

Re: [PATCH v2 2/4] spi: amlogic: spisg: Fix transfer size limit and timeout handling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing max_transfer_size callback leads to silent truncation of large transfers.
- [High] Incomplete timeout handling leaves interrupts unmasked, allowing a delayed interrupt to prematurely complete a subsequent transfer.
- [High] Immediate DMA buffer unmapping on transfer timeout causes memory corruption if the hardware DMA engine is still active.
--

--- Patch [2]: [PATCH v2 2/4] spi: amlogic: spisg: Fix transfer size limit and timeout handling ---
commit d149d3bcf81987c3bc95c5cc0053e984ee79e15e
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>
Subject: spi: amlogic: spisg: Fix transfer size limit and timeout handling

This commit reduces SPISG_BLOCK_MAX to 0xfffff to avoid programming zero-length
transfers because the CFG_BLOCK_NUM field can only encode 20 bits. It also
fixes a delay calculation overflow and attempts to stop the controller on
transfer timeout by clearing the descriptor list register.

Fixes: cef9991e04ae ("spi: Add Amlogic SPISG driver")

> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
> @@ -95,7 +95,7 @@
>  
>  #define SPISG_MAX_REG			0x40
>  
> -#define SPISG_BLOCK_MAX			0x100000
> +#define SPISG_BLOCK_MAX			0xFFFFF

[Severity: High]
This isn't a bug introduced by this patch, but modifying SPISG_BLOCK_MAX
brings up a question about large transfers.

Are large I/O requests being silently truncated?

In aml_spisg_setup_transfer(), the hardware block count is dynamically clamped:

    blocks = min_t(int, blocks, SPISG_BLOCK_MAX);

However, the driver does not provide a ctlr->max_transfer_size callback.
The SPI core assumes SIZE_MAX is supported and won't split large transfers.
The hardware will stop after transferring SPISG_BLOCK_MAX blocks.

Later in aml_spisg_transfer_one_message(), the driver unconditionally reports
success for the full length:

    msg->actual_length = msg->frame_length;

Could this cause silent data corruption for transfers larger than
SPISG_BLOCK_MAX * (bits_per_word / 8) bytes, since the caller is unaware the
transfer was truncated?

[ ... ]

> @@ -565,10 +565,13 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
>  	aml_spisg_pending(spisg, descs_paddr, false, true);
>  	if (wait_for_completion_timeout(&spisg->completion,
>  					spi_controller_is_target(spisg->controller) ?
> -					MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(ms)))
> +					MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(ms))) {
>  		ret = spisg->status ? -EIO : 0;
> -	else
> +	} else {
> +		/* stop transfer */
> +		regmap_write(spisg->map, SPISG_REG_DESC_LIST_H, 0);
>  		ret = -ETIMEDOUT;
> +	}

[Severity: High]
This is a pre-existing issue, but does the timeout path safely halt the DMA
engine before unmapping memory?

After writing 0 to SPISG_REG_DESC_LIST_H to abort the transfer, the driver
immediately calls dma_unmap_single() and then aml_spisg_cleanup_transfer()
to unmap the data buffers.

If the hardware DMA engine requires time to flush outstanding AXI transactions
or if the register write is delayed, could the hardware access unmapped pages
or memory reallocated to other components, resulting in memory corruption or
IOMMU faults? Is it necessary to poll the hardware to confirm it has halted
before proceeding?

[Severity: High]
This is also a pre-existing issue, but is there a race condition with
delayed interrupts on a timeout?

If a transfer times out, the driver issues the stop command but does not
mask the controller's interrupt (SPISG_REG_IRQ_ENABLE) or use synchronize_irq()
to ensure the handler is not running.

The interrupt handler aml_spisg_irq() unconditionally calls:

    complete(&spisg->completion);

If a delayed interrupt fires concurrently with a subsequent transfer that has
just called reinit_completion(&spisg->completion), might the new transfer's
wait_for_completion_timeout() return immediately as a false success? This could
cause the driver to unmap DMA buffers while the hardware is still actively
processing the new transfer.

>  
>  	dma_unmap_single(dev, descs_paddr, descs_len, DMA_TO_DEVICE);
>  end:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-a9-spisg-v2-0-9fc39106b7e2@amlogic.com?part=2

[PATCH v2 3/4] spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

[Xianwei Zhao via B4 Relay]

From: Sunny Luo <sunny.luo@amlogic.com>

The driver currently unconditionally clears CFG_KEEP_SS on the last
descriptor, causing the last transfer's cs_change setting to be ignored.

Record the cs_change value of the last SPI transfer and use it to
program CFG_KEEP_SS on the final descriptor. When a null descriptor is
inserted to implement the cs-hold delay, keep CFG_KEEP_SS set on the
preceding transfer descriptor and apply the recorded value to the final
descriptor instead.

This ensures the controller handles chip select correctly for the last
transfer regardless of whether a cs-hold delay is required.

Fixes: cef9991e04ae ("spi: Add Amlogic SPISG driver")
Signed-off-by: Sunny Luo <sunny.luo@amlogic.com>
Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 drivers/spi/spi-amlogic-spisg.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
index a6123975a37c..2d66f00dccd4 100644
--- a/drivers/spi/spi-amlogic-spisg.c
+++ b/drivers/spi/spi-amlogic-spisg.c
@@ -489,6 +489,7 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
 	struct spisg_descriptor_extra *exdescs, *exdesc;
 	dma_addr_t descs_paddr;
 	int desc_num = 1, descs_len;
+	bool last_xfer_keep_ss = false;
 	u32 cs_hold_in_sclk = 0;
 	int ret = -EIO;
 
@@ -529,9 +530,11 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
 				spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_setup));
 
 		/* calculate cs-hold delay with the last xfer speed */
-		if (list_is_last(&xfer->transfer_list, &msg->transfers))
+		if (list_is_last(&xfer->transfer_list, &msg->transfers)) {
 			cs_hold_in_sclk =
 				spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_hold);
+			last_xfer_keep_ss = xfer->cs_change;
+		}
 
 		desc++;
 		exdesc++;
@@ -539,13 +542,17 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
 				       xfer->effective_speed_hz);
 	}
 
-	if (cs_hold_in_sclk)
+	if (cs_hold_in_sclk) {
 		/* additional null-descriptor to achieve the cs-hold delay */
 		aml_spisg_setup_null_desc(spisg, desc, cs_hold_in_sclk);
-	else
 		desc--;
+		desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, 1);
+		desc++;
+	} else {
+		desc--;
+	}
 
-	desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, 0);
+	FIELD_MODIFY(CFG_KEEP_SS, &desc->cfg_bus, last_xfer_keep_ss);
 	desc->cfg_start |= FIELD_PREP(CFG_EOC, 1);
 
 	/* some tolerances */

-- 
2.52.0

Re: [PATCH v2 3/4] spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing pm_runtime_disable() and pm_runtime_put() in probe error path and remove().
- [High] Double-disable of clocks managed by devm_clk_get_enabled().
- [High] Race condition causing spurious -EBUSY transfer failures.
- [High] Target abort incorrectly signals successful transfer completion.
--

--- Patch [3]: [PATCH v2 3/4] spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor ---
commit c9f43474b4fa19eea599df7045d868e70af19bd8
Author: Sunny Luo <sunny.luo@amlogic.com>
spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

This commit fixes the incorrect handling of the keep_ss state for the last
descriptor in the Amlogic SPISG driver. It ensures the CFG_KEEP_SS bit is
properly set based on the final transfer's cs_change value, ensuring the
chip select behaves correctly even when a cs-hold delay is present.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index a6123975a37cf..2d66f00dccd45 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c

[ ... ]

> @@ -539,13 +542,17 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
>  				       xfer->effective_speed_hz);
>  	}
>  
> -	if (cs_hold_in_sclk)
> +	if (cs_hold_in_sclk) {
>  		/* additional null-descriptor to achieve the cs-hold delay */
>  		aml_spisg_setup_null_desc(spisg, desc, cs_hold_in_sclk);
> -	else
>  		desc--;
> +		desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, 1);
> +		desc++;
> +	} else {
> +		desc--;
> +	}
>  
> -	desc->cfg_bus |= FIELD_PREP(CFG_KEEP_SS, 0);
> +	FIELD_MODIFY(CFG_KEEP_SS, &desc->cfg_bus, last_xfer_keep_ss);
>  	desc->cfg_start |= FIELD_PREP(CFG_EOC, 1);
>  
>  	/* some tolerances */

[Severity: High]
This is a pre-existing issue, but does calling spi_finalize_current_message()
before releasing the hardware lock open a race window for the next transfer?

In aml_spisg_transfer_one_message(), we have:

	msg->status = ret;
	spi_finalize_current_message(ctlr);
	aml_spisg_sem_up_write(spisg);

Since spi_finalize_current_message() signals the SPI core that the controller
is ready, could the SPI core immediately dispatch the next transfer on another
CPU, which then fails aml_spisg_sem_down_read() and aborts with -EBUSY?

[Severity: High]
This isn't a bug introduced by this patch, but does the target abort handler
incorrectly signal a successful transfer?

In aml_spisg_target_abort():

	spisg->status = 0;
	regmap_write(spisg->map, SPISG_REG_DESC_LIST_H, 0);
	complete(&spisg->completion);

And in aml_spisg_transfer_one_message():

	if (wait_for_completion_timeout(&spisg->completion,
					spi_controller_is_target(spisg->controller) ?
					MAX_SCHEDULE_TIMEOUT : msecs_to_jiffies(ms))) {
		ret = spisg->status ? -EIO : 0;

If a transfer is aborted, status is set to 0 and the wait completes. Does
this cause the SPI client to incorrectly interpret an aborted transfer as
fully successful?

[Severity: High]
This is a pre-existing issue, but do the probe error path and remove
functions leak PM runtime references?

In aml_spisg_probe(), the out_clk error path exits without disabling
PM runtime:

out_clk:
	if (spisg->core)
		clk_disable_unprepare(spisg->core);
	clk_disable_unprepare(spisg->pclk);

	return ret;

Additionally, aml_spisg_remove() does not call pm_runtime_disable():

static void aml_spisg_remove(struct platform_device *pdev)
{
...
	if (!pm_runtime_suspended(&pdev->dev)) {

Does this prevent the device's power domain from suspending and cause
a kernel warning when rebinding?

[Severity: High]
This isn't a bug introduced by this patch, but are the devm-managed clocks
being double-disabled?

In aml_spisg_clk_init(), pclk is explicitly disabled:

	clk_set_min_rate(spisg->pclk, SPISG_PCLK_RATE_MIN);

	clk_disable_unprepare(spisg->pclk);

In the aml_spisg_probe() error path, both clocks are manually disabled:

out_clk:
	if (spisg->core)
		clk_disable_unprepare(spisg->core);
	clk_disable_unprepare(spisg->pclk);

And in aml_spisg_remove(), they are disabled again:

		clk_disable_unprepare(spisg->core);
		clk_disable_unprepare(spisg->pclk);

Since these clocks are acquired using devm_clk_get_enabled(), which
automatically registers a cleanup action, will manually disabling them
underflow the clock enable count and trigger warnings from the clock
framework?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-a9-spisg-v2-0-9fc39106b7e2@amlogic.com?part=3

[PATCH v2 4/4] spi: amlogic: spisg: Add support for A9 controller features

[Xianwei Zhao via B4 Relay]

From: Xianwei Zhao <xianwei.zhao@amlogic.com>

The Amlogic A9 SPISG controller extends the A4 controller with additional
configuration options, including:

- Extended CS setup timing
- Hardware-controlled CS hold timing
- MOSI idle output configuration
- Configurable word gap

Add SoC-specific capability data and configure these features when they
are supported by the underlying hardware while keeping compatibility with
existing A4 controllers.

Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 drivers/spi/spi-amlogic-spisg.c | 66 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 62 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
index 2d66f00dccd4..bc9564aef245 100644
--- a/drivers/spi/spi-amlogic-spisg.c
+++ b/drivers/spi/spi-amlogic-spisg.c
@@ -37,6 +37,12 @@
 #define CFG_HW_POS			BIT(6)
 /* start on vsync falling */
 #define CFG_HW_NEG			BIT(7)
+#define CFG_WORD_GAP			GENMASK(9, 8)
+#define CFG_MO_IDLE_OUTPUT		GENMASK(11, 10)
+/* cs hold time in pclk */
+#define CFG_CS_HOLD			GENMASK(26, 12)
+/* high 4 bits of cs setup time in sclk */
+#define CFG_CS_SETUP_EXTEND		GENMASK(30, 27)
 
 #define SPISG_REG_CFG_START		0x08
 #define CFG_BLOCK_NUM			GENMASK(19, 0)
@@ -143,6 +149,13 @@ struct spisg_descriptor_extra {
 	int				rx_ccsg_len;
 };
 
+struct aml_spisg_data {
+	bool				mo_idle_output_ctrl;
+	bool				word_gap_ctrl;
+	bool				cs_hold_ctrl;
+	bool				cs_setup_extend_ctrl;
+};
+
 struct spisg_device {
 	struct spi_controller		*controller;
 	struct platform_device		*pdev;
@@ -152,6 +165,7 @@ struct spisg_device {
 	struct clk			*sclk;
 	struct clk_div_table		*tbl;
 	struct completion		completion;
+	const struct aml_spisg_data	*data;
 	u32				status;
 	u32				speed_hz;
 	u32				effective_speed_hz;
@@ -483,6 +497,7 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
 {
 	struct spisg_device *spisg = spi_controller_get_devdata(ctlr);
 	struct device *dev = &spisg->pdev->dev;
+	const struct aml_spisg_data *data = spisg->data;
 	unsigned long long ms = 0;
 	struct spi_transfer *xfer;
 	struct spisg_descriptor *descs, *desc;
@@ -491,6 +506,7 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
 	int desc_num = 1, descs_len;
 	bool last_xfer_keep_ss = false;
 	u32 cs_hold_in_sclk = 0;
+	u32 val;
 	int ret = -EIO;
 
 	if (!aml_spisg_sem_down_read(spisg)) {
@@ -525,9 +541,17 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
 		}
 
 		/* calculate cs-setup delay with the first xfer speed */
-		if (list_is_first(&xfer->transfer_list, &msg->transfers))
-			desc->cfg_bus |= FIELD_PREP(CFG_CS_SETUP,
-				spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_setup));
+		if (list_is_first(&xfer->transfer_list, &msg->transfers)) {
+			val = spi_delay_to_sclk(xfer->effective_speed_hz, &msg->spi->cs_setup);
+			if (data && data->cs_setup_extend_ctrl) {
+				val = min_t(u32, 0xFF, val);
+				desc->cfg_bus |= FIELD_PREP(CFG_CS_SETUP, val & 0xF);
+				FIELD_MODIFY(CFG_CS_SETUP_EXTEND, &spisg->cfg_spi, val >> 4);
+			} else {
+				val = min_t(u32, 0xF, val);
+				desc->cfg_bus |= FIELD_PREP(CFG_CS_SETUP, val);
+			}
+		}
 
 		/* calculate cs-hold delay with the last xfer speed */
 		if (list_is_last(&xfer->transfer_list, &msg->transfers)) {
@@ -542,7 +566,13 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
 				       xfer->effective_speed_hz);
 	}
 
-	if (cs_hold_in_sclk) {
+	if (data && data->cs_hold_ctrl) {
+		cs_hold_in_sclk = cs_hold_in_sclk ? : 1;
+		val = cs_hold_in_sclk * (FIELD_GET(CFG_CLK_DIV, spisg->cfg_bus) + 1);
+		val = min_t(u32, 0x7FFF, val);
+		FIELD_MODIFY(CFG_CS_HOLD, &spisg->cfg_spi, val);
+		desc--;
+	} else if (cs_hold_in_sclk) {
 		/* additional null-descriptor to achieve the cs-hold delay */
 		aml_spisg_setup_null_desc(spisg, desc, cs_hold_in_sclk);
 		desc--;
@@ -725,6 +755,7 @@ static int aml_spisg_probe(struct platform_device *pdev)
 	struct spisg_device *spisg;
 	struct device *dev = &pdev->dev;
 	void __iomem *base;
+	u32 val = 0;
 	int ret, irq;
 
 	const struct regmap_config aml_regmap_config = {
@@ -743,6 +774,7 @@ static int aml_spisg_probe(struct platform_device *pdev)
 
 	spisg = spi_controller_get_devdata(ctlr);
 	spisg->controller = ctlr;
+	spisg->data = (struct aml_spisg_data *)of_device_get_match_data(dev);
 
 	spisg->pdev = pdev;
 	platform_set_drvdata(pdev, spisg);
@@ -773,6 +805,21 @@ static int aml_spisg_probe(struct platform_device *pdev)
 
 	spisg->cfg_spi = FIELD_PREP(CFG_SFLASH_WP, 1) |
 			 FIELD_PREP(CFG_SFLASH_HD, 1);
+
+	if (spisg->data && spisg->data->mo_idle_output_ctrl) {
+		if (!of_property_read_u32(dev->of_node, "amlogic,mo-idle-output", &val))
+			spisg->cfg_spi |=  FIELD_PREP(CFG_MO_IDLE_OUTPUT, val);
+		else
+			spisg->cfg_spi |=  FIELD_PREP(CFG_MO_IDLE_OUTPUT, 0);
+	}
+
+	if (spisg->data && spisg->data->word_gap_ctrl) {
+		if (!of_property_read_u32(dev->of_node, "amlogic,word-gap", &val))
+			spisg->cfg_spi |=  FIELD_PREP(CFG_WORD_GAP, val);
+		else
+			spisg->cfg_spi |=  FIELD_PREP(CFG_WORD_GAP, 1);
+	}
+
 	if (spi_controller_is_target(ctlr)) {
 		spisg->cfg_spi |= FIELD_PREP(CFG_SLAVE_EN, 1);
 		spisg->cfg_bus = FIELD_PREP(CFG_TX_TUNING, 0xf);
@@ -859,6 +906,13 @@ static int spisg_resume_runtime(struct device *dev)
 	return 0;
 }
 
+static const struct aml_spisg_data a9_spisg_data = {
+	.mo_idle_output_ctrl = true,
+	.word_gap_ctrl = true,
+	.cs_hold_ctrl = true,
+	.cs_setup_extend_ctrl = true,
+};
+
 static const struct dev_pm_ops amlogic_spisg_pm_ops = {
 	.runtime_suspend	= spisg_suspend_runtime,
 	.runtime_resume		= spisg_resume_runtime,
@@ -868,6 +922,10 @@ static const struct of_device_id amlogic_spisg_of_match[] = {
 	{
 		.compatible = "amlogic,a4-spisg",
 	},
+	{
+		.compatible = "amlogic,a9-spisg",
+		.data = &a9_spisg_data,
+	},
 
 	{ /* sentinel */ }
 };

-- 
2.52.0

Re: [PATCH v2 4/4] spi: amlogic: spisg: Add support for A9 controller features

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Timeout completion race leading to cross-message corruption and potential DMA UAF.
- [High] PM runtime usage counter leak and missing disable on probe error and module remove.
- [High] Driver erroneously assumes DMA address 0 is invalid for mapping tracking.
- [High] The runtime PM resume callback ignores clock enablement errors.
--

--- Patch [4]: [PATCH v2 4/4] spi: amlogic: spisg: Add support for A9 controller features ---
commit 733f8e986a88161245fdf261af60ee5f16e4648c
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>

spi: amlogic: spisg: Add support for A9 controller features
    
The Amlogic A9 SPISG controller extends the A4 controller with additional
configuration options. Add SoC-specific capability data and configure
these features when they are supported by the underlying hardware.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index 2d66f00dccd45..bc9564aef2452 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c

[ ... ]

> @@ -542,7 +566,13 @@ static int aml_spisg_transfer_one_message(struct spi_controller *ctlr,
>  				       xfer->effective_speed_hz);
>  	}
>  
> -	if (cs_hold_in_sclk) {
> +	if (data && data->cs_hold_ctrl) {
> +		cs_hold_in_sclk = cs_hold_in_sclk ? : 1;
> +		val = cs_hold_in_sclk * (FIELD_GET(CFG_CLK_DIV, spisg->cfg_bus) + 1);
> +		val = min_t(u32, 0x7FFF, val);
> +		FIELD_MODIFY(CFG_CS_HOLD, &spisg->cfg_spi, val);
> +		desc--;
> +	} else if (cs_hold_in_sclk) {
>  		/* additional null-descriptor to achieve the cs-hold delay */
>  		aml_spisg_setup_null_desc(spisg, desc, cs_hold_in_sclk);
>  		desc--;

[Severity: High]
This is a pre-existing issue, but does aml_spisg_transfer_one_message() have a
timeout completion race that could lead to DMA use-after-free?

If wait_for_completion_timeout() expires, the driver attempts to stop the
transfer by writing 0 to SPISG_REG_DESC_LIST_H:

	if (wait_for_completion_timeout(&spisg->completion, ...)) {
		ret = spisg->status ? -EIO : 0;
	} else {
		/* stop transfer */
		regmap_write(spisg->map, SPISG_REG_DESC_LIST_H, 0);
		ret = -ETIMEDOUT;
	}

Since it fails to disable the hardware interrupt or synchronize with the IRQ
handler via synchronize_irq(), could a delayed hardware interrupt call
complete(&spisg->completion) slightly late?

If this happens during or just after the next SPI message calls
reinit_completion(), it would immediately satisfy the new message's
completion wait, unmapping the DMA buffers while the hardware is actively
using them.

[Severity: High]
This isn't a bug introduced by this patch, but does this driver erroneously
assume DMA address 0 is invalid for mapping tracking?

In aml_spisg_setup_transfer() and aml_spisg_cleanup_transfer(), the code checks
if the mapped DMA address is zero to determine if a mapping exists:

	if (desc->tx_paddr) {
		if (FIELD_GET(CFG_TXD_MODE, desc->cfg_start) == SPISG_DATA_MODE_SG) {
			dma_unmap_single(dev, (dma_addr_t)desc->tx_paddr,
					 exdesc->tx_ccsg_len, DMA_TO_DEVICE);
			kfree(exdesc->tx_ccsg);

Since 0 is a valid DMA address on some platforms, if dma_map_single()
successfully maps a buffer to address 0, would this skip the unmap and kfree
calls, leaking memory and the DMA mapping? Should this use a separate boolean
flag like tx_mapped instead of testing the DMA address value?

[ ... ]

> @@ -773,6 +805,21 @@ static int aml_spisg_probe(struct platform_device *pdev)
>  
>  	spisg->cfg_spi = FIELD_PREP(CFG_SFLASH_WP, 1) |
>  			 FIELD_PREP(CFG_SFLASH_HD, 1);
> +
> +	if (spisg->data && spisg->data->mo_idle_output_ctrl) {
> +		if (!of_property_read_u32(dev->of_node, "amlogic,mo-idle-output", &val))
> +			spisg->cfg_spi |=  FIELD_PREP(CFG_MO_IDLE_OUTPUT, val);
> +		else
> +			spisg->cfg_spi |=  FIELD_PREP(CFG_MO_IDLE_OUTPUT, 0);
> +	}
> +
> +	if (spisg->data && spisg->data->word_gap_ctrl) {
> +		if (!of_property_read_u32(dev->of_node, "amlogic,word-gap", &val))
> +			spisg->cfg_spi |=  FIELD_PREP(CFG_WORD_GAP, val);
> +		else
> +			spisg->cfg_spi |=  FIELD_PREP(CFG_WORD_GAP, 1);
> +	}
> +
>  	if (spi_controller_is_target(ctlr)) {
>  		spisg->cfg_spi |= FIELD_PREP(CFG_SLAVE_EN, 1);
>  		spisg->cfg_bus = FIELD_PREP(CFG_TX_TUNING, 0xf);

[Severity: High]
This isn't a bug introduced by this patch, but does aml_spisg_probe() leak
the PM runtime usage counter on error paths?

The driver enables PM runtime and increments the usage counter:

	pm_runtime_set_active(&spisg->pdev->dev);
	pm_runtime_enable(&spisg->pdev->dev);
	pm_runtime_resume_and_get(&spisg->pdev->dev);

If a subsequent operation like devm_request_irq() fails, the code branches to
out_clk and returns without decrementing the usage counter or disabling PM
runtime. 

Additionally, aml_spisg_remove() does not appear to call pm_runtime_disable(),
which could leave the device dangling in the PM runtime core after module
unload.

[ ... ]

> @@ -859,6 +906,13 @@ static int spisg_resume_runtime(struct device *dev)
>  	return 0;
>  }
>  
> +static const struct aml_spisg_data a9_spisg_data = {
> +	.mo_idle_output_ctrl = true,
> +	.word_gap_ctrl = true,
> +	.cs_hold_ctrl = true,
> +	.cs_setup_extend_ctrl = true,
> +};
> +

[Severity: High]
This is a pre-existing issue, but does spisg_resume_runtime() ignore clock
enablement errors?

In spisg_resume_runtime(), the driver calls:

	clk_prepare_enable(spisg->core);
	clk_prepare_enable(spisg->sclk);

If clock enablement fails, the function still returns 0 (success). Could this
trick the PM runtime core into believing the device is active, causing
subsequent MMIO register accesses to hang the bus or crash the system since
the clock domain is off?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723-a9-spisg-v2-0-9fc39106b7e2@amlogic.com?part=4