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

[PATCH 0/3] 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>
---
Sunny Luo (1):
      spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

Xianwei Zhao (2):
      spi: dt-bindings: amlogic: spisg: Document A9-specific properties
      spi: amlogic: spisg: Add support for A9 controller features

 .../devicetree/bindings/spi/amlogic,a4-spisg.yaml  | 36 ++++++++--
 drivers/spi/spi-amlogic-spisg.c                    | 78 ++++++++++++++++++++--
 2 files changed, 103 insertions(+), 11 deletions(-)
---
base-commit: d0b5dd16226814e5c73ba8daf88d3d91c990a38f
change-id: 20260715-a9-spisg-f19ce0403a50

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



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Xianwei Zhao]

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>
---
Sunny Luo (1):
      spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

Xianwei Zhao (2):
      spi: dt-bindings: amlogic: spisg: Document A9-specific properties
      spi: amlogic: spisg: Add support for A9 controller features

 .../devicetree/bindings/spi/amlogic,a4-spisg.yaml  | 36 ++++++++--
 drivers/spi/spi-amlogic-spisg.c                    | 78 ++++++++++++++++++++--
 2 files changed, 103 insertions(+), 11 deletions(-)
---
base-commit: d0b5dd16226814e5c73ba8daf88d3d91c990a38f
change-id: 20260715-a9-spisg-f19ce0403a50

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

[PATCH 0/3] 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>
---
Sunny Luo (1):
      spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

Xianwei Zhao (2):
      spi: dt-bindings: amlogic: spisg: Document A9-specific properties
      spi: amlogic: spisg: Add support for A9 controller features

 .../devicetree/bindings/spi/amlogic,a4-spisg.yaml  | 36 ++++++++--
 drivers/spi/spi-amlogic-spisg.c                    | 78 ++++++++++++++++++++--
 2 files changed, 103 insertions(+), 11 deletions(-)
---
base-commit: d0b5dd16226814e5c73ba8daf88d3d91c990a38f
change-id: 20260715-a9-spisg-f19ce0403a50

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

[PATCH 1/3] 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  | 36 +++++++++++++++++++---
 1 file changed, 32 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..7efdc748d566 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
@@ -42,6 +41,35 @@ required:
   - clocks
   - clock-names
 
+allOf:
+  - $ref: spi-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - amlogic,a9-spisg
+    then:
+      properties:
+        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
+
 unevaluatedProperties: false
 
 examples:

-- 
2.52.0



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Xianwei Zhao]

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  | 36 +++++++++++++++++++---
 1 file changed, 32 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..7efdc748d566 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
@@ -42,6 +41,35 @@ required:
   - clocks
   - clock-names
 
+allOf:
+  - $ref: spi-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - amlogic,a9-spisg
+    then:
+      properties:
+        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
+
 unevaluatedProperties: false
 
 examples:

-- 
2.52.0

[PATCH 1/3] 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  | 36 +++++++++++++++++++---
 1 file changed, 32 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..7efdc748d566 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
@@ -42,6 +41,35 @@ required:
   - clocks
   - clock-names
 
+allOf:
+  - $ref: spi-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - amlogic,a9-spisg
+    then:
+      properties:
+        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
+
 unevaluatedProperties: false
 
 examples:

-- 
2.52.0

[PATCH 2/3] 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 afc8af04638d..0f026d3e43e0 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



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Xianwei Zhao]

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 afc8af04638d..0f026d3e43e0 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

[PATCH 2/3] 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 afc8af04638d..0f026d3e43e0 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 2/3] 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:
- [Critical] Missing hardware abort on SPI transfer timeout leads to DMA Use-After-Free and memory corruption.
- [High] 32-bit integer overflow in `spi_delay_to_sclk` leads to severely truncated SPI delays.
- [High] `SPISG_BLOCK_MAX` exceeds the hardware mask size, causing maximum-length transfers to be processed as zero-length.
- [High] Hardware limits for `cs_setup` and `cs_hold` delays are not clamped before being packed into register fields, causing long delays to silently wrap around to short values.
--

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

spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

This patch fixes an issue where the CFG_KEEP_SS flag is unconditionally
cleared on the last SPI descriptor. It records the cs_change value of
the last transfer and properly programs CFG_KEEP_SS on the final descriptor.
When a null descriptor is used for cs-hold, the flag is kept on the
preceding descriptor and the recorded value is applied at the end.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index afc8af04638d3..0f026d3e43e0b 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
> @@ -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));

[Severity: High]
This isn't a bug introduced by this patch, but can the 32-bit multiplication
in spi_delay_to_sclk() overflow here?

If a requested delay in nanoseconds and the SPI clock frequency in Hz are
multiplied together, it evaluates as a 32-bit unsigned operation before being
cast to 64-bit for the division:

drivers/spi/spi-amlogic-spisg.c:spi_delay_to_sclk() {
    ...
	return DIV_ROUND_UP_ULL(slck_speed_hz * ns, NSEC_PER_SEC);
}

For example, at a 100MHz clock, a delay greater than 42.9us will wrap around,
causing hardware setup or hold delays to become extremely short or zero.

[Severity: High]
This is also a pre-existing issue, but are the hardware limits for cs_setup
and cs_hold delays clamped before being packed into the register fields?

CFG_CS_SETUP is a 4-bit field (max 15). If spi_delay_to_sclk() returns
a value greater than 15, FIELD_PREP() will silently truncate the upper
bits, resulting in an incorrect, drastically shorter delay.

>  		/* 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;
> +		}

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but does the error handling path for timeouts in
aml_spisg_transfer_one_message() lack a hardware abort?

If a hardware transfer times out, the driver unmaps DMA buffers and frees the
transfer descriptors without stopping the SPI controller hardware:

drivers/spi/spi-amlogic-spisg.c:aml_spisg_transfer_one_message() {
    ...
	if (wait_for_completion_timeout(&spisg->completion, ...))
		ret = spisg->status ? -EIO : 0;
	else
		ret = -ETIMEDOUT;

	dma_unmap_single(dev, descs_paddr, descs_len, DMA_TO_DEVICE);
end:
    ...
}

Could this lead to the hardware continuing to write to or read from freed memory
pages (DMA Use-After-Free) since the hardware isn't stopped?

[Severity: High]
This is a pre-existing issue, but can the maximum blocks count exceed the
hardware mask size in aml_spisg_setup_transfer()?

The blocks count is capped at SPISG_BLOCK_MAX (0x100000):

drivers/spi/spi-amlogic-spisg.c:aml_spisg_setup_transfer() {
    ...
		blocks = min_t(int, blocks, SPISG_BLOCK_MAX);
		desc->cfg_start |= FIELD_PREP(CFG_BLOCK_SIZE, block_size & 0x7) |
				   FIELD_PREP(CFG_BLOCK_NUM, blocks);
    ...
}

Since CFG_BLOCK_NUM uses a 20-bit mask (GENMASK(19, 0)), a transfer bounded
exactly at SPISG_BLOCK_MAX (0x100000) will be silently zeroed out when masked,
causing it to be processed as a zero-length transfer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-a9-spisg-v1-0-2eeea77be19f@amlogic.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH 2/3] 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:
- [Critical] Missing hardware abort on SPI transfer timeout leads to DMA Use-After-Free and memory corruption.
- [High] 32-bit integer overflow in `spi_delay_to_sclk` leads to severely truncated SPI delays.
- [High] `SPISG_BLOCK_MAX` exceeds the hardware mask size, causing maximum-length transfers to be processed as zero-length.
- [High] Hardware limits for `cs_setup` and `cs_hold` delays are not clamped before being packed into register fields, causing long delays to silently wrap around to short values.
--

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

spi: amlogic: spisg: Fix the incorrect keep_ss of the last descriptor

This patch fixes an issue where the CFG_KEEP_SS flag is unconditionally
cleared on the last SPI descriptor. It records the cs_change value of
the last transfer and properly programs CFG_KEEP_SS on the final descriptor.
When a null descriptor is used for cs-hold, the flag is kept on the
preceding descriptor and the recorded value is applied at the end.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index afc8af04638d3..0f026d3e43e0b 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
> @@ -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));

[Severity: High]
This isn't a bug introduced by this patch, but can the 32-bit multiplication
in spi_delay_to_sclk() overflow here?

If a requested delay in nanoseconds and the SPI clock frequency in Hz are
multiplied together, it evaluates as a 32-bit unsigned operation before being
cast to 64-bit for the division:

drivers/spi/spi-amlogic-spisg.c:spi_delay_to_sclk() {
    ...
	return DIV_ROUND_UP_ULL(slck_speed_hz * ns, NSEC_PER_SEC);
}

For example, at a 100MHz clock, a delay greater than 42.9us will wrap around,
causing hardware setup or hold delays to become extremely short or zero.

[Severity: High]
This is also a pre-existing issue, but are the hardware limits for cs_setup
and cs_hold delays clamped before being packed into the register fields?

CFG_CS_SETUP is a 4-bit field (max 15). If spi_delay_to_sclk() returns
a value greater than 15, FIELD_PREP() will silently truncate the upper
bits, resulting in an incorrect, drastically shorter delay.

>  		/* 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;
> +		}

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but does the error handling path for timeouts in
aml_spisg_transfer_one_message() lack a hardware abort?

If a hardware transfer times out, the driver unmaps DMA buffers and frees the
transfer descriptors without stopping the SPI controller hardware:

drivers/spi/spi-amlogic-spisg.c:aml_spisg_transfer_one_message() {
    ...
	if (wait_for_completion_timeout(&spisg->completion, ...))
		ret = spisg->status ? -EIO : 0;
	else
		ret = -ETIMEDOUT;

	dma_unmap_single(dev, descs_paddr, descs_len, DMA_TO_DEVICE);
end:
    ...
}

Could this lead to the hardware continuing to write to or read from freed memory
pages (DMA Use-After-Free) since the hardware isn't stopped?

[Severity: High]
This is a pre-existing issue, but can the maximum blocks count exceed the
hardware mask size in aml_spisg_setup_transfer()?

The blocks count is capped at SPISG_BLOCK_MAX (0x100000):

drivers/spi/spi-amlogic-spisg.c:aml_spisg_setup_transfer() {
    ...
		blocks = min_t(int, blocks, SPISG_BLOCK_MAX);
		desc->cfg_start |= FIELD_PREP(CFG_BLOCK_SIZE, block_size & 0x7) |
				   FIELD_PREP(CFG_BLOCK_NUM, blocks);
    ...
}

Since CFG_BLOCK_NUM uses a 20-bit mask (GENMASK(19, 0)), a transfer bounded
exactly at SPISG_BLOCK_MAX (0x100000) will be silently zeroed out when masked,
causing it to be processed as a zero-length transfer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-a9-spisg-v1-0-2eeea77be19f@amlogic.com?part=2

[PATCH 3/3] 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 | 65 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
index 0f026d3e43e0..845eb81d483a 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,12 @@ 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);
+		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--;
@@ -722,6 +751,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 = {
@@ -740,6 +770,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);
@@ -770,6 +801,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);
@@ -856,6 +902,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,
@@ -865,6 +918,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



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Xianwei Zhao]

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 | 65 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
index 0f026d3e43e0..845eb81d483a 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,12 @@ 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);
+		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--;
@@ -722,6 +751,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 = {
@@ -740,6 +770,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);
@@ -770,6 +801,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);
@@ -856,6 +902,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,
@@ -865,6 +918,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

[PATCH 3/3] 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 | 65 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
index 0f026d3e43e0..845eb81d483a 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,12 @@ 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);
+		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--;
@@ -722,6 +751,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 = {
@@ -740,6 +770,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);
@@ -770,6 +801,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);
@@ -856,6 +902,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,
@@ -865,6 +918,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 3/3] spi: amlogic: spisg: Add support for A9 controller features

[sashiko-bot]

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

New issues:
- [High] The calculated CFG_CS_HOLD delay value lacks bounds checking before being written via FIELD_MODIFY, leading to silent bitwise truncation and incorrectly shortened hold times.

Pre-existing issues:
- [High] The driver fails to abort the hardware operation when a transfer times out, leading to DMA use-after-free and cascading synchronization failures.
- [High] `spi_delay_to_sclk()` suffers from a 32-bit integer overflow when calculating delay cycles, resulting in drastically shorter setup and hold times than requested.
--

--- Patch [3]: [PATCH 3/3] spi: amlogic: spisg: Add support for A9 controller features ---
commit 850f7cb5955f8bd514dfd78b459ee02b15378db4
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 supported by the hardware, while keeping compatibility
with existing A4 controllers.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index 0f026d3e43e0b..845eb81d483a3 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
[ ... ]
> @@ -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);

[Severity: High]
This is a pre-existing issue, but can spi_delay_to_sclk() suffer from a
32-bit integer overflow when calculating delay cycles?

Looking at spi_delay_to_sclk():

drivers/spi/spi-amlogic-spisg.c:spi_delay_to_sclk() {
    ...
    return DIV_ROUND_UP_ULL(slck_speed_hz * ns, NSEC_PER_SEC);
}

If a client requests a large delay, the multiplication of the two 32-bit
integers (slck_speed_hz and ns) can wrap around before being cast to a
64-bit integer by DIV_ROUND_UP_ULL.

Could this result in drastically shorter setup and hold times than
requested?

> +			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,12 @@ 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);
> +		FIELD_MODIFY(CFG_CS_HOLD, &spisg->cfg_spi, val);

[Severity: High]
Does the calculated CFG_CS_HOLD delay value lack bounds checking before
being written via FIELD_MODIFY?

Unlike CFG_CS_SETUP which safely uses min_t(), cs_hold is left unbounded.
Since CFG_CS_HOLD is a 15-bit field, if an SPI client requests a large CS
hold time, the FIELD_MODIFY macro will silently apply a bitwise mask.

Will this cause the delay to wrap around (e.g. 32768 becomes 0) rather
than clamping at the maximum supported hardware limit, leading to hardware
timing violations?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the
timeout error path to free DMA-mapped memory without halting the active
hardware?

Looking further down in aml_spisg_transfer_one_message():

drivers/spi/spi-amlogic-spisg.c:aml_spisg_transfer_one_message() {
    ...
    if (wait_for_completion_timeout(&spisg->completion, ...))
        ret = spisg->status ? -EIO : 0;
    else
        ret = -ETIMEDOUT;

    dma_unmap_single(dev, descs_paddr, descs_len, DMA_TO_DEVICE);
end:
    ...
    kfree(descs);
    ...
}

If an SPI transfer times out and wait_for_completion_timeout() expires,
the code proceeds directly to unmap the DMA memory and free the descriptor
array. However, the hardware is never halted or reset (e.g. by writing 0
to the descriptor list register).

Might the hardware still be actively performing DMA, resulting in memory
corruption from a use-after-free? Furthermore, if the hardware eventually
completes, could the delayed interrupt falsely wake up subsequent transfers?

> +		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--;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-a9-spisg-v1-0-2eeea77be19f@amlogic.com?part=3

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[sashiko-bot]

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

New issues:
- [High] The calculated CFG_CS_HOLD delay value lacks bounds checking before being written via FIELD_MODIFY, leading to silent bitwise truncation and incorrectly shortened hold times.

Pre-existing issues:
- [High] The driver fails to abort the hardware operation when a transfer times out, leading to DMA use-after-free and cascading synchronization failures.
- [High] `spi_delay_to_sclk()` suffers from a 32-bit integer overflow when calculating delay cycles, resulting in drastically shorter setup and hold times than requested.
--

--- Patch [3]: [PATCH 3/3] spi: amlogic: spisg: Add support for A9 controller features ---
commit 850f7cb5955f8bd514dfd78b459ee02b15378db4
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 supported by the hardware, while keeping compatibility
with existing A4 controllers.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index 0f026d3e43e0b..845eb81d483a3 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
[ ... ]
> @@ -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);

[Severity: High]
This is a pre-existing issue, but can spi_delay_to_sclk() suffer from a
32-bit integer overflow when calculating delay cycles?

Looking at spi_delay_to_sclk():

drivers/spi/spi-amlogic-spisg.c:spi_delay_to_sclk() {
    ...
    return DIV_ROUND_UP_ULL(slck_speed_hz * ns, NSEC_PER_SEC);
}

If a client requests a large delay, the multiplication of the two 32-bit
integers (slck_speed_hz and ns) can wrap around before being cast to a
64-bit integer by DIV_ROUND_UP_ULL.

Could this result in drastically shorter setup and hold times than
requested?

> +			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,12 @@ 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);
> +		FIELD_MODIFY(CFG_CS_HOLD, &spisg->cfg_spi, val);

[Severity: High]
Does the calculated CFG_CS_HOLD delay value lack bounds checking before
being written via FIELD_MODIFY?

Unlike CFG_CS_SETUP which safely uses min_t(), cs_hold is left unbounded.
Since CFG_CS_HOLD is a 15-bit field, if an SPI client requests a large CS
hold time, the FIELD_MODIFY macro will silently apply a bitwise mask.

Will this cause the delay to wrap around (e.g. 32768 becomes 0) rather
than clamping at the maximum supported hardware limit, leading to hardware
timing violations?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the
timeout error path to free DMA-mapped memory without halting the active
hardware?

Looking further down in aml_spisg_transfer_one_message():

drivers/spi/spi-amlogic-spisg.c:aml_spisg_transfer_one_message() {
    ...
    if (wait_for_completion_timeout(&spisg->completion, ...))
        ret = spisg->status ? -EIO : 0;
    else
        ret = -ETIMEDOUT;

    dma_unmap_single(dev, descs_paddr, descs_len, DMA_TO_DEVICE);
end:
    ...
    kfree(descs);
    ...
}

If an SPI transfer times out and wait_for_completion_timeout() expires,
the code proceeds directly to unmap the DMA memory and free the descriptor
array. However, the hardware is never halted or reset (e.g. by writing 0
to the descriptor list register).

Might the hardware still be actively performing DMA, resulting in memory
corruption from a use-after-free? Furthermore, if the hardware eventually
completes, could the delayed interrupt falsely wake up subsequent transfers?

> +		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--;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-a9-spisg-v1-0-2eeea77be19f@amlogic.com?part=3