* [PATCH 1/8] dt-bindings: i3c: Document the AST2700 I3C controller
2026-09-01 11:35 [PATCH 0/8] i3c: mipi-i3c-hci: Add Aspeed AST2700 support Billy Tsai
@ 2026-09-01 11:35 ` Billy Tsai
2026-09-01 20:41 ` Frank Li
2026-09-01 11:35 ` [PATCH 2/8] i3c: mipi-i3c-hci: Support address-indexed DAT slots Billy Tsai
` (6 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Billy Tsai @ 2026-09-01 11:35 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
Cc: linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, Billy Tsai
The AST2700 I3C controller implements the MIPI I3C HCI programming
model, extended with an ASPEED in-house control block and a PHY
programming window advertised through an HCI vendor extended
capability. Follow the existing aspeed,ast2600-i3c precedent of
describing SoC integrations of a common I3C IP in their own schema.
The controller takes a core clock for PHY timing and two reset-names
lines: its own reset and a DMA engine reset shared by all I3C
controllers on the SoC.
The PHY timings derived from the core clock and bus rate assume
nominal bus loading. Boards with long traces, weak pull-ups, or
capacitive loading need different ratios and setup periods, so
document properties to override the push-pull and open-drain SCL
periods, the SDA TX hold time, the tCAS/tCBP periods, and the SCL low
period used for a repeated START or STOP.
Boards without external I3C pull-up resistors can use the PHY's
built-in pull-ups instead. The achievable resistance depends on the
voltage domain of the pins (high-voltage vs low-voltage), which can be
determined from the pin group assigned to the bus. Reuse bias-pull-up
(pincfg-node.yaml) for the strength in Ohms; it is declared on this
node rather than a pin-config node since the pull-up switch is part of
the I3C PHY, not the broader SoC pin control IPs.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Assisted-by: Claude:claude-fable-5
---
We considered formally expressing bias-pull-up's dependency on a
pinctrl state in the schema, but couldn't find a clean way to say
"depends on whichever pinctrl-N is active" without hard-coding
pinctrl-0 specifically, which would be wrong in general (a node's
active pin state doesn't have to sit at index 0). We've added
pinctrl-0 to the example to illustrate the typical usage instead,
without a formal schema-level dependency.
---
.../bindings/i3c/aspeed,ast2700-i3c-hci.yaml | 133 +++++++++++++++++++++
1 file changed, 133 insertions(+)
diff --git a/Documentation/devicetree/bindings/i3c/aspeed,ast2700-i3c-hci.yaml b/Documentation/devicetree/bindings/i3c/aspeed,ast2700-i3c-hci.yaml
new file mode 100644
index 000000000000..19ecb5b15038
--- /dev/null
+++ b/Documentation/devicetree/bindings/i3c/aspeed,ast2700-i3c-hci.yaml
@@ -0,0 +1,133 @@
+# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i3c/aspeed,ast2700-i3c-hci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED AST2700 I3C controller
+
+maintainers:
+ - Billy Tsai <billy_tsai@aspeedtech.com>
+
+description:
+ The AST2700 I3C controller implements the MIPI I3C HCI programming model,
+ extended with an ASPEED in-house control block and a PHY programming window
+ that are advertised through an HCI vendor extended capability.
+
+properties:
+ compatible:
+ const: aspeed,ast2700-i3c-hci
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ resets:
+ items:
+ - description: Core controller reset.
+ - description: DMA engine reset shared by all I3C controllers.
+
+ reset-names:
+ items:
+ - const: core
+ - const: dma
+
+ bias-pull-up:
+ $ref: /schemas/pinctrl/pincfg-node.yaml#/properties/bias-pull-up
+ enum: [177, 231, 261, 317, 400, 470, 550, 600, 750]
+ description:
+ Enable the PHY-internal pull-ups on SCL and SDA and select their
+ strength, in Ohms. High-voltage pins support 400, 470 or 600 Ohms,
+ and low-voltage pins support 177, 231, 261, 317, 400, 550 or 750
+ Ohms. When absent the internal pull-ups stay disabled and the bus
+ relies on external pull-ups.
+
+ aspeed,pp-scl-high-ns:
+ description:
+ Push-pull SCL high period. Overrides the value derived from the
+ i3c-scl-hz bus rate.
+
+ aspeed,pp-scl-low-ns:
+ description:
+ Push-pull SCL low period. Overrides the value derived from the
+ i3c-scl-hz bus rate.
+
+ aspeed,od-scl-high-ns:
+ default: 380
+ description:
+ Open-drain SCL high period.
+
+ aspeed,od-scl-low-ns:
+ default: 620
+ description:
+ Open-drain SCL low period.
+
+ aspeed,sda-tx-hold-ns:
+ default: 10
+ description:
+ SDA output hold time relative to the SCL edge.
+
+ aspeed,cas-ns:
+ minimum: 39
+ description:
+ Clock-after-start (tCAS) period driven after a START condition.
+ When absent it is derived from the SCL low period of the
+ applicable bus context, clamped to the protocol minimum.
+
+ aspeed,cbp-ns:
+ minimum: 20
+ description:
+ Clock-before-stop (tCBP) period driven before a STOP condition.
+ When absent it is derived from the SCL low period of the
+ applicable bus context, clamped to the protocol minimum.
+
+ aspeed,sr-p-scl-low-ns:
+ default: 40
+ description:
+ SCL low period used while preparing a repeated START or STOP
+ condition, kept separate from the push-pull SCL low period to
+ satisfy the MIPI I3C clock stalling requirements.
+
+dependencies:
+ aspeed,pp-scl-high-ns:
+ - aspeed,pp-scl-low-ns
+ aspeed,pp-scl-low-ns:
+ - aspeed,pp-scl-high-ns
+ aspeed,od-scl-high-ns:
+ - aspeed,od-scl-low-ns
+ aspeed,od-scl-low-ns:
+ - aspeed,od-scl-high-ns
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - resets
+ - reset-names
+
+allOf:
+ - $ref: i3c.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ i3c@14c24000 {
+ compatible = "aspeed,ast2700-i3c-hci";
+ reg = <0x14c24000 0x1000>;
+ interrupts = <100>;
+ clocks = <&syscon 20>;
+ resets = <&syscon 37>, <&syscon 34>;
+ reset-names = "core", "dma";
+ bias-pull-up = <400>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i3c4_default>;
+ #address-cells = <3>;
+ #size-cells = <0>;
+ };
--
2.34.1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 1/8] dt-bindings: i3c: Document the AST2700 I3C controller
2026-09-01 11:35 ` [PATCH 1/8] dt-bindings: i3c: Document the AST2700 I3C controller Billy Tsai
@ 2026-09-01 20:41 ` Frank Li
0 siblings, 0 replies; 19+ messages in thread
From: Frank Li @ 2026-09-01 20:41 UTC (permalink / raw)
To: Billy Tsai
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel,
linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel
On Tue, Sep 01, 2026 at 07:35:28PM +0800, Billy Tsai wrote:
> The AST2700 I3C controller implements the MIPI I3C HCI programming
> model, extended with an ASPEED in-house control block and a PHY
> programming window advertised through an HCI vendor extended
> capability. Follow the existing aspeed,ast2600-i3c precedent of
> describing SoC integrations of a common I3C IP in their own schema.
>
> The controller takes a core clock for PHY timing and two reset-names
> lines: its own reset and a DMA engine reset shared by all I3C
> controllers on the SoC.
>
> The PHY timings derived from the core clock and bus rate assume
> nominal bus loading. Boards with long traces, weak pull-ups, or
> capacitive loading need different ratios and setup periods, so
> document properties to override the push-pull and open-drain SCL
> periods, the SDA TX hold time, the tCAS/tCBP periods, and the SCL low
> period used for a repeated START or STOP.
>
> Boards without external I3C pull-up resistors can use the PHY's
> built-in pull-ups instead. The achievable resistance depends on the
> voltage domain of the pins (high-voltage vs low-voltage), which can be
> determined from the pin group assigned to the bus. Reuse bias-pull-up
> (pincfg-node.yaml) for the strength in Ohms; it is declared on this
> node rather than a pin-config node since the pull-up switch is part of
> the I3C PHY, not the broader SoC pin control IPs.
look likes AI generated message, can you try keep short and avoid
duplicate information, which already in binding doc.
Add AST2700 I3C controller, which compatible with MIPI I3C HCI. Extend with
an ASPEED in-house control block ....
introduce some hardware feature here ...
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Assisted-by: Claude:claude-fable-5
>
> ---
> We considered formally expressing bias-pull-up's dependency on a
> pinctrl state in the schema, but couldn't find a clean way to say
> "depends on whichever pinctrl-N is active" without hard-coding
> pinctrl-0 specifically, which would be wrong in general (a node's
> active pin state doesn't have to sit at index 0). We've added
> pinctrl-0 to the example to illustrate the typical usage instead,
> without a formal schema-level dependency.
> ---
> .../bindings/i3c/aspeed,ast2700-i3c-hci.yaml | 133 +++++++++++++++++++++
> 1 file changed, 133 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i3c/aspeed,ast2700-i3c-hci.yaml b/Documentation/devicetree/bindings/i3c/aspeed,ast2700-i3c-hci.yaml
> new file mode 100644
> index 000000000000..19ecb5b15038
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/aspeed,ast2700-i3c-hci.yaml
> @@ -0,0 +1,133 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i3c/aspeed,ast2700-i3c-hci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ASPEED AST2700 I3C controller
> +
> +maintainers:
> + - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description:
> + The AST2700 I3C controller implements the MIPI I3C HCI programming model,
> + extended with an ASPEED in-house control block and a PHY programming window
> + that are advertised through an HCI vendor extended capability.
> +
> +properties:
> + compatible:
> + const: aspeed,ast2700-i3c-hci
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> +
> + resets:
> + items:
> + - description: Core controller reset.
> + - description: DMA engine reset shared by all I3C controllers.
> +
> + reset-names:
> + items:
> + - const: core
> + - const: dma
> +
> + bias-pull-up:
> + $ref: /schemas/pinctrl/pincfg-node.yaml#/properties/bias-pull-up
> + enum: [177, 231, 261, 317, 400, 470, 550, 600, 750]
> + description:
> + Enable the PHY-internal pull-ups on SCL and SDA and select their
> + strength, in Ohms. High-voltage pins support 400, 470 or 600 Ohms,
> + and low-voltage pins support 177, 231, 261, 317, 400, 550 or 750
> + Ohms. When absent the internal pull-ups stay disabled and the bus
> + relies on external pull-ups.
this one should go through pinctrl.
> +
> + aspeed,pp-scl-high-ns:
> + description:
> + Push-pull SCL high period. Overrides the value derived from the
> + i3c-scl-hz bus rate.
> +
> + aspeed,pp-scl-low-ns:
> + description:
> + Push-pull SCL low period. Overrides the value derived from the
> + i3c-scl-hz bus rate.
> +
> + aspeed,od-scl-high-ns:
> + default: 380
> + description:
> + Open-drain SCL high period.
> +
> + aspeed,od-scl-low-ns:
> + default: 620
> + description:
> + Open-drain SCL low period.
> +
> + aspeed,sda-tx-hold-ns:
> + default: 10
> + description:
> + SDA output hold time relative to the SCL edge.
> +
> + aspeed,cas-ns:
> + minimum: 39
> + description:
> + Clock-after-start (tCAS) period driven after a START condition.
> + When absent it is derived from the SCL low period of the
> + applicable bus context, clamped to the protocol minimum.
> +
> + aspeed,cbp-ns:
> + minimum: 20
> + description:
> + Clock-before-stop (tCBP) period driven before a STOP condition.
> + When absent it is derived from the SCL low period of the
> + applicable bus context, clamped to the protocol minimum.
> +
> + aspeed,sr-p-scl-low-ns:
> + default: 40
> + description:
> + SCL low period used while preparing a repeated START or STOP
> + condition, kept separate from the push-pull SCL low period to
> + satisfy the MIPI I3C clock stalling requirements.
these value is i3c stardard defined value, why need put into dt-binding.
Frank
> +
> +dependencies:
> + aspeed,pp-scl-high-ns:
> + - aspeed,pp-scl-low-ns
> + aspeed,pp-scl-low-ns:
> + - aspeed,pp-scl-high-ns
> + aspeed,od-scl-high-ns:
> + - aspeed,od-scl-low-ns
> + aspeed,od-scl-low-ns:
> + - aspeed,od-scl-high-ns
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - resets
> + - reset-names
> +
> +allOf:
> + - $ref: i3c.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + i3c@14c24000 {
> + compatible = "aspeed,ast2700-i3c-hci";
> + reg = <0x14c24000 0x1000>;
> + interrupts = <100>;
> + clocks = <&syscon 20>;
> + resets = <&syscon 37>, <&syscon 34>;
> + reset-names = "core", "dma";
> + bias-pull-up = <400>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pinctrl_i3c4_default>;
> + #address-cells = <3>;
> + #size-cells = <0>;
> + };
>
> --
> 2.34.1
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/8] i3c: mipi-i3c-hci: Support address-indexed DAT slots
2026-09-01 11:35 [PATCH 0/8] i3c: mipi-i3c-hci: Add Aspeed AST2700 support Billy Tsai
2026-09-01 11:35 ` [PATCH 1/8] dt-bindings: i3c: Document the AST2700 I3C controller Billy Tsai
@ 2026-09-01 11:35 ` Billy Tsai
2026-09-01 11:49 ` sashiko-bot
2026-09-01 20:47 ` Frank Li
2026-09-01 11:35 ` [PATCH 3/8] i3c: mipi-i3c-hci: Add a quirk for 64-bit DMA addressing Billy Tsai
` (5 subsequent siblings)
7 siblings, 2 replies; 19+ messages in thread
From: Billy Tsai @ 2026-09-01 11:35 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
Cc: linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, Billy Tsai
HCI v1's DEV_INDEX allocation is sequential: the driver picks any free
DAT slot, with no relationship between the slot number and the
device's I3C address. ASPEED's I3C controllers have long needed to
support more devices than the standard 5-bit DEV_INDEX field
(addresses 0-31) allows, so they use a layout where the DAT slot
number is itself the device's address, requiring DEV_INDEX to widen
to 7 bits (addresses 0-127).
Add HCI_QUIRK_DAT_INDEX_IS_ADDR for controllers with this layout:
allocate address-indexed DAT slots and emit the wider DEV_INDEX field.
Moving an entry on a dynamic address change can fail if its slot is
already taken, so set_dynamic_addr() now returns an error that the
reattach and DAA paths propagate. Controllers without the quirk keep
the existing behavior.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Assisted-by: Claude:claude-fable-5
---
drivers/i3c/master/mipi-i3c-hci/cmd_v1.c | 35 ++++++++++++------
drivers/i3c/master/mipi-i3c-hci/core.c | 19 +++++++---
drivers/i3c/master/mipi-i3c-hci/dat.h | 5 ++-
drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 63 +++++++++++++++++++++++++++++---
drivers/i3c/master/mipi-i3c-hci/hci.h | 1 +
5 files changed, 98 insertions(+), 25 deletions(-)
diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
index 3b9345718d273..9a11affb14bf6 100644
--- a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
+++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
@@ -141,6 +141,14 @@ static enum hci_cmd_mode get_i2c_mode(struct i3c_hci *hci)
return MODE_I2C_Fm;
}
+static u32 cmd_v1_dev_index(struct i3c_hci *hci, unsigned int dat_idx)
+{
+ u32 mask = hci->quirks & HCI_QUIRK_DAT_INDEX_IS_ADDR ?
+ W0_MASK(22, 16) : W0_MASK(20, 16);
+
+ return field_prep(mask, dat_idx);
+}
+
static void fill_data_bytes(struct hci_xfer *xfer, u8 *data,
unsigned int data_len)
{
@@ -195,7 +203,7 @@ static int hci_cmd_v1_prep_ccc(struct i3c_hci *hci,
CMD_0_ATTR_I |
CMD_I0_TID(xfer->cmd_tid) |
CMD_I0_CMD(ccc_cmd) | CMD_I0_CP |
- CMD_I0_DEV_INDEX(dat_idx) |
+ cmd_v1_dev_index(hci, dat_idx) |
CMD_I0_DTT(data_len) |
CMD_I0_MODE(mode);
fill_data_bytes(xfer, data, data_len);
@@ -205,7 +213,7 @@ static int hci_cmd_v1_prep_ccc(struct i3c_hci *hci,
CMD_0_ATTR_R |
CMD_R0_TID(xfer->cmd_tid) |
CMD_R0_CMD(ccc_cmd) | CMD_R0_CP |
- CMD_R0_DEV_INDEX(dat_idx) |
+ cmd_v1_dev_index(hci, dat_idx) |
CMD_R0_MODE(mode) |
(rnw ? CMD_R0_RNW : 0);
xfer->cmd_desc[1] =
@@ -233,7 +241,7 @@ static void hci_cmd_v1_prep_i3c_xfer(struct i3c_hci *hci,
xfer->cmd_desc[0] =
CMD_0_ATTR_I |
CMD_I0_TID(xfer->cmd_tid) |
- CMD_I0_DEV_INDEX(dat_idx) |
+ cmd_v1_dev_index(hci, dat_idx) |
CMD_I0_DTT(data_len) |
CMD_I0_MODE(mode);
fill_data_bytes(xfer, data, data_len);
@@ -242,7 +250,7 @@ static void hci_cmd_v1_prep_i3c_xfer(struct i3c_hci *hci,
xfer->cmd_desc[0] =
CMD_0_ATTR_R |
CMD_R0_TID(xfer->cmd_tid) |
- CMD_R0_DEV_INDEX(dat_idx) |
+ cmd_v1_dev_index(hci, dat_idx) |
CMD_R0_MODE(mode) |
(rnw ? CMD_R0_RNW : 0);
xfer->cmd_desc[1] =
@@ -268,7 +276,7 @@ static void hci_cmd_v1_prep_i2c_xfer(struct i3c_hci *hci,
xfer->cmd_desc[0] =
CMD_0_ATTR_I |
CMD_I0_TID(xfer->cmd_tid) |
- CMD_I0_DEV_INDEX(dat_idx) |
+ cmd_v1_dev_index(hci, dat_idx) |
CMD_I0_DTT(data_len) |
CMD_I0_MODE(mode);
fill_data_bytes(xfer, data, data_len);
@@ -277,7 +285,7 @@ static void hci_cmd_v1_prep_i2c_xfer(struct i3c_hci *hci,
xfer->cmd_desc[0] =
CMD_0_ATTR_R |
CMD_R0_TID(xfer->cmd_tid) |
- CMD_R0_DEV_INDEX(dat_idx) |
+ cmd_v1_dev_index(hci, dat_idx) |
CMD_R0_MODE(mode) |
(rnw ? CMD_R0_RNW : 0);
xfer->cmd_desc[1] =
@@ -306,19 +314,22 @@ static int hci_cmd_v1_daa(struct i3c_hci *hci)
* Yes, there is room for improvements.
*/
for (;;) {
- ret = mipi_i3c_hci_dat_v1.alloc_entry(hci);
- if (ret < 0)
- break;
- dat_idx = ret;
ret = i3c_master_get_free_addr(&hci->master, next_addr);
if (ret < 0)
break;
next_addr = ret;
+ ret = mipi_i3c_hci_dat_v1.alloc_entry(hci, next_addr);
+ if (ret < 0)
+ break;
+ dat_idx = ret;
dev_dbg(&hci->master.dev,
"next_addr = 0x%02x, DAA using DAT %d",
next_addr, dat_idx);
- mipi_i3c_hci_dat_v1.set_dynamic_addr(hci, dat_idx, next_addr);
+ ret = mipi_i3c_hci_dat_v1.set_dynamic_addr(hci, dat_idx,
+ next_addr);
+ if (ret < 0)
+ break;
mipi_i3c_hci_dct_index_reset(hci);
xfer->cmd_tid = hci_get_tid();
@@ -326,7 +337,7 @@ static int hci_cmd_v1_daa(struct i3c_hci *hci)
CMD_0_ATTR_A |
CMD_A0_TID(xfer->cmd_tid) |
CMD_A0_CMD(I3C_CCC_ENTDAA) |
- CMD_A0_DEV_INDEX(dat_idx) |
+ cmd_v1_dev_index(hci, dat_idx) |
CMD_A0_DEV_COUNT(1) |
CMD_A0_ROC | CMD_A0_TOC;
xfer->cmd_desc[1] = 0;
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index dadf049bd4b54..a624e3c40484e 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -574,7 +574,8 @@ static int i3c_hci_attach_i3c_dev(struct i3c_dev_desc *dev)
if (!dev_data)
return -ENOMEM;
if (hci->cmd == &mipi_i3c_hci_cmd_v1) {
- ret = mipi_i3c_hci_dat_v1.alloc_entry(hci);
+ ret = mipi_i3c_hci_dat_v1.alloc_entry(hci,
+ dev->info.dyn_addr ?: dev->info.static_addr);
if (ret < 0) {
kfree(dev_data);
return ret;
@@ -593,9 +594,17 @@ static int i3c_hci_reattach_i3c_dev(struct i3c_dev_desc *dev, u8 old_dyn_addr)
struct i3c_hci *hci = to_i3c_hci(m);
struct i3c_hci_dev_data *dev_data = i3c_dev_get_master_data(dev);
- if (hci->cmd == &mipi_i3c_hci_cmd_v1)
- mipi_i3c_hci_dat_v1.set_dynamic_addr(hci, dev_data->dat_idx,
- dev->info.dyn_addr);
+ if (hci->cmd == &mipi_i3c_hci_cmd_v1) {
+ int ret;
+
+ ret = mipi_i3c_hci_dat_v1.set_dynamic_addr(hci,
+ dev_data->dat_idx,
+ dev->info.dyn_addr);
+ if (ret)
+ return ret;
+ if (hci->quirks & HCI_QUIRK_DAT_INDEX_IS_ADDR)
+ dev_data->dat_idx = dev->info.dyn_addr;
+ }
return 0;
}
@@ -623,7 +632,7 @@ static int i3c_hci_attach_i2c_dev(struct i2c_dev_desc *dev)
dev_data = kzalloc_obj(*dev_data);
if (!dev_data)
return -ENOMEM;
- ret = mipi_i3c_hci_dat_v1.alloc_entry(hci);
+ ret = mipi_i3c_hci_dat_v1.alloc_entry(hci, dev->addr);
if (ret < 0) {
kfree(dev_data);
return ret;
diff --git a/drivers/i3c/master/mipi-i3c-hci/dat.h b/drivers/i3c/master/mipi-i3c-hci/dat.h
index 6881f19da77f2..d4c7b03724d37 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dat.h
+++ b/drivers/i3c/master/mipi-i3c-hci/dat.h
@@ -14,12 +14,13 @@
#define DAT_0_I2C_DEVICE W0_BIT_(31)
#define DAT_0_SIR_REJECT W0_BIT_(13)
#define DAT_0_IBI_PAYLOAD W0_BIT_(12)
+#define HCI_DAT_ANY_ADDRESS ((unsigned int)-1)
struct hci_dat_ops {
int (*init)(struct i3c_hci *hci);
- int (*alloc_entry)(struct i3c_hci *hci);
+ int (*alloc_entry)(struct i3c_hci *hci, unsigned int address);
void (*free_entry)(struct i3c_hci *hci, unsigned int dat_idx);
- void (*set_dynamic_addr)(struct i3c_hci *hci, unsigned int dat_idx, u8 addr);
+ int (*set_dynamic_addr)(struct i3c_hci *hci, unsigned int dat_idx, u8 addr);
void (*set_static_addr)(struct i3c_hci *hci, unsigned int dat_idx, u8 addr);
void (*set_flags)(struct i3c_hci *hci, unsigned int dat_idx, u32 w0, u32 w1);
void (*clear_flags)(struct i3c_hci *hci, unsigned int dat_idx, u32 w0, u32 w1);
diff --git a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
index 852966aa20d98..19b0f72e4ac82 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
@@ -89,7 +89,7 @@ static int hci_dat_v1_init(struct i3c_hci *hci)
return 0;
}
-static int hci_dat_v1_alloc_entry(struct i3c_hci *hci)
+static int hci_dat_v1_alloc_entry(struct i3c_hci *hci, unsigned int address)
{
unsigned int dat_idx;
int ret;
@@ -99,9 +99,17 @@ static int hci_dat_v1_alloc_entry(struct i3c_hci *hci)
if (ret)
return ret;
}
- dat_idx = find_first_zero_bit(hci->DAT_data, hci->DAT_entries);
- if (dat_idx >= hci->DAT_entries)
- return -ENOENT;
+ if (hci->quirks & HCI_QUIRK_DAT_INDEX_IS_ADDR) {
+ if (address >= hci->DAT_entries)
+ return -EINVAL;
+ if (test_bit_acquire(address, hci->DAT_data))
+ return -ENOENT;
+ dat_idx = address;
+ } else {
+ dat_idx = find_first_zero_bit(hci->DAT_data, hci->DAT_entries);
+ if (dat_idx >= hci->DAT_entries)
+ return -ENOENT;
+ }
__set_bit(dat_idx, hci->DAT_data);
/* default flags */
@@ -118,9 +126,43 @@ static void hci_dat_v1_free_entry(struct i3c_hci *hci, unsigned int dat_idx)
__clear_bit(dat_idx, hci->DAT_data);
}
-static void hci_dat_v1_set_dynamic_addr(struct i3c_hci *hci,
- unsigned int dat_idx, u8 address)
+static int hci_dat_v1_set_dynamic_addr(struct i3c_hci *hci,
+ unsigned int dat_idx, u8 address)
{
+ if (hci->quirks & HCI_QUIRK_DAT_INDEX_IS_ADDR) {
+ u32 dat_w0, dat_w1;
+
+ /*
+ * The controller derives the device address from the DAT slot
+ * index, so DAT_0_DYNAMIC_ADDRESS is never written: an address
+ * change moves the whole entry to the slot named by the new
+ * address instead.
+ */
+ if (dat_idx == address)
+ return 0;
+ if (address >= hci->DAT_entries) {
+ dev_err(&hci->master.dev, "invalid DAT slot %u\n", address);
+ return -EINVAL;
+ }
+ if (test_bit(address, hci->DAT_data)) {
+ dev_err(&hci->master.dev, "DAT slot %u already in use\n",
+ address);
+ return -EBUSY;
+ }
+
+ dat_w0 = dat_w0_read(dat_idx);
+ dat_w1 = dat_w1_read(dat_idx);
+
+ __set_bit(address, hci->DAT_data);
+ dat_w0_write(address, dat_w0);
+ dat_w1_write(address, dat_w1);
+
+ dat_w0_write(dat_idx, 0);
+ dat_w1_write(dat_idx, 0);
+ __clear_bit(dat_idx, hci->DAT_data);
+ return 0;
+ }
+
u32 dat_w0;
dat_w0 = dat_w0_read(dat_idx);
@@ -128,6 +170,8 @@ static void hci_dat_v1_set_dynamic_addr(struct i3c_hci *hci,
dat_w0 |= FIELD_PREP(DAT_0_DYNAMIC_ADDRESS, address) |
(parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);
dat_w0_write(dat_idx, dat_w0);
+
+ return 0;
}
static void hci_dat_v1_set_static_addr(struct i3c_hci *hci,
@@ -169,6 +213,13 @@ static void hci_dat_v1_clear_flags(struct i3c_hci *hci, unsigned int dat_idx,
static int hci_dat_v1_get_index(struct i3c_hci *hci, u8 dev_addr)
{
+ if (hci->quirks & HCI_QUIRK_DAT_INDEX_IS_ADDR) {
+ if (dev_addr < hci->DAT_entries &&
+ test_bit(dev_addr, hci->DAT_data))
+ return dev_addr;
+ return -ENODEV;
+ }
+
unsigned int dat_idx;
u32 dat_w0;
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index ee73f6e6756a1..f50fc1e22a855 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -177,6 +177,7 @@ struct i3c_hci_dev_data {
#define HCI_QUIRK_RPM_PARENT_MANAGED BIT(7) /* Runtime PM managed by parent device */
#define HCI_QUIRK_DMA_ABORT_REQUIRES_PIO_RESET BIT(8) /* Do PIO queue SW resets after DMA abort */
#define HCI_QUIRK_DMA_REQUIRES_HC_ABORT BIT(9) /* Use HC_CONTROL ABORT to abort DMA */
+#define HCI_QUIRK_DAT_INDEX_IS_ADDR BIT(10) /* DAT entries are indexed by device address */
/* global functions */
void mipi_i3c_hci_resume(struct i3c_hci *hci);
--
2.34.1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/8] i3c: mipi-i3c-hci: Support address-indexed DAT slots
2026-09-01 11:35 ` [PATCH 2/8] i3c: mipi-i3c-hci: Support address-indexed DAT slots Billy Tsai
@ 2026-09-01 11:49 ` sashiko-bot
2026-09-01 20:47 ` Frank Li
1 sibling, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-09-01 11:49 UTC (permalink / raw)
To: Billy Tsai
Cc: conor+dt, devicetree, robh, linux-i3c, Alexandre Belloni,
Frank.Li
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The macro `HCI_DAT_ANY_ADDRESS` is defined but never used.
--
commit 8ebe233a1f220dcff7a28a779f787f8b63ea1787
Author: Billy Tsai <billy_tsai@aspeedtech.com>
i3c: mipi-i3c-hci: Support address-indexed DAT slots
This commit introduces the HCI_QUIRK_DAT_INDEX_IS_ADDR quirk to support
ASPEED I3C controllers. Controllers with this quirk allocate DAT slots
based directly on the device's 7-bit address instead of using sequential
DAT slot assignments.
[ ... ]
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dat.h b/drivers/i3c/master/mipi-i3c-hci/dat.h
> index 6881f19da77f2..d4c7b03724d37 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dat.h
> +++ b/drivers/i3c/master/mipi-i3c-hci/dat.h
> @@ -14,12 +14,13 @@
> #define DAT_0_I2C_DEVICE W0_BIT_(31)
> #define DAT_0_SIR_REJECT W0_BIT_(13)
> #define DAT_0_IBI_PAYLOAD W0_BIT_(12)
> +#define HCI_DAT_ANY_ADDRESS ((unsigned int)-1)
[Severity: Low]
Is HCI_DAT_ANY_ADDRESS intended to be used in this patch series? This macro
is defined here in drivers/i3c/master/mipi-i3c-hci/dat.h but never referenced
anywhere in the codebase or in the remainder of the patch. If it is unused,
can it be removed to prevent dead code?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-b4-i3c-hci-ast2700-v1-0-19909e7cbd7e@aspeedtech.com?part=2
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/8] i3c: mipi-i3c-hci: Support address-indexed DAT slots
2026-09-01 11:35 ` [PATCH 2/8] i3c: mipi-i3c-hci: Support address-indexed DAT slots Billy Tsai
2026-09-01 11:49 ` sashiko-bot
@ 2026-09-01 20:47 ` Frank Li
1 sibling, 0 replies; 19+ messages in thread
From: Frank Li @ 2026-09-01 20:47 UTC (permalink / raw)
To: Billy Tsai
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel,
linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel
On Tue, Sep 01, 2026 at 07:35:29PM +0800, Billy Tsai wrote:
> HCI v1's DEV_INDEX allocation is sequential: the driver picks any free
> DAT slot, with no relationship between the slot number and the
> device's I3C address. ASPEED's I3C controllers have long needed to
> support more devices than the standard 5-bit DEV_INDEX field
> (addresses 0-31) allows, so they use a layout where the DAT slot
> number is itself the device's address, requiring DEV_INDEX to widen
> to 7 bits (addresses 0-127).
Does ASPEED work if use 5bit DEV_INDEX?
Frank
>
> Add HCI_QUIRK_DAT_INDEX_IS_ADDR for controllers with this layout:
> allocate address-indexed DAT slots and emit the wider DEV_INDEX field.
> Moving an entry on a dynamic address change can fail if its slot is
> already taken, so set_dynamic_addr() now returns an error that the
> reattach and DAA paths propagate. Controllers without the quirk keep
> the existing behavior.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Assisted-by: Claude:claude-fable-5
> ---
> drivers/i3c/master/mipi-i3c-hci/cmd_v1.c | 35 ++++++++++++------
> drivers/i3c/master/mipi-i3c-hci/core.c | 19 +++++++---
> drivers/i3c/master/mipi-i3c-hci/dat.h | 5 ++-
> drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 63 +++++++++++++++++++++++++++++---
> drivers/i3c/master/mipi-i3c-hci/hci.h | 1 +
> 5 files changed, 98 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> index 3b9345718d273..9a11affb14bf6 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> @@ -141,6 +141,14 @@ static enum hci_cmd_mode get_i2c_mode(struct i3c_hci *hci)
> return MODE_I2C_Fm;
> }
>
> +static u32 cmd_v1_dev_index(struct i3c_hci *hci, unsigned int dat_idx)
> +{
> + u32 mask = hci->quirks & HCI_QUIRK_DAT_INDEX_IS_ADDR ?
> + W0_MASK(22, 16) : W0_MASK(20, 16);
> +
> + return field_prep(mask, dat_idx);
> +}
> +
> static void fill_data_bytes(struct hci_xfer *xfer, u8 *data,
> unsigned int data_len)
> {
> @@ -195,7 +203,7 @@ static int hci_cmd_v1_prep_ccc(struct i3c_hci *hci,
> CMD_0_ATTR_I |
> CMD_I0_TID(xfer->cmd_tid) |
> CMD_I0_CMD(ccc_cmd) | CMD_I0_CP |
> - CMD_I0_DEV_INDEX(dat_idx) |
> + cmd_v1_dev_index(hci, dat_idx) |
> CMD_I0_DTT(data_len) |
> CMD_I0_MODE(mode);
> fill_data_bytes(xfer, data, data_len);
> @@ -205,7 +213,7 @@ static int hci_cmd_v1_prep_ccc(struct i3c_hci *hci,
> CMD_0_ATTR_R |
> CMD_R0_TID(xfer->cmd_tid) |
> CMD_R0_CMD(ccc_cmd) | CMD_R0_CP |
> - CMD_R0_DEV_INDEX(dat_idx) |
> + cmd_v1_dev_index(hci, dat_idx) |
> CMD_R0_MODE(mode) |
> (rnw ? CMD_R0_RNW : 0);
> xfer->cmd_desc[1] =
> @@ -233,7 +241,7 @@ static void hci_cmd_v1_prep_i3c_xfer(struct i3c_hci *hci,
> xfer->cmd_desc[0] =
> CMD_0_ATTR_I |
> CMD_I0_TID(xfer->cmd_tid) |
> - CMD_I0_DEV_INDEX(dat_idx) |
> + cmd_v1_dev_index(hci, dat_idx) |
> CMD_I0_DTT(data_len) |
> CMD_I0_MODE(mode);
> fill_data_bytes(xfer, data, data_len);
> @@ -242,7 +250,7 @@ static void hci_cmd_v1_prep_i3c_xfer(struct i3c_hci *hci,
> xfer->cmd_desc[0] =
> CMD_0_ATTR_R |
> CMD_R0_TID(xfer->cmd_tid) |
> - CMD_R0_DEV_INDEX(dat_idx) |
> + cmd_v1_dev_index(hci, dat_idx) |
> CMD_R0_MODE(mode) |
> (rnw ? CMD_R0_RNW : 0);
> xfer->cmd_desc[1] =
> @@ -268,7 +276,7 @@ static void hci_cmd_v1_prep_i2c_xfer(struct i3c_hci *hci,
> xfer->cmd_desc[0] =
> CMD_0_ATTR_I |
> CMD_I0_TID(xfer->cmd_tid) |
> - CMD_I0_DEV_INDEX(dat_idx) |
> + cmd_v1_dev_index(hci, dat_idx) |
> CMD_I0_DTT(data_len) |
> CMD_I0_MODE(mode);
> fill_data_bytes(xfer, data, data_len);
> @@ -277,7 +285,7 @@ static void hci_cmd_v1_prep_i2c_xfer(struct i3c_hci *hci,
> xfer->cmd_desc[0] =
> CMD_0_ATTR_R |
> CMD_R0_TID(xfer->cmd_tid) |
> - CMD_R0_DEV_INDEX(dat_idx) |
> + cmd_v1_dev_index(hci, dat_idx) |
> CMD_R0_MODE(mode) |
> (rnw ? CMD_R0_RNW : 0);
> xfer->cmd_desc[1] =
> @@ -306,19 +314,22 @@ static int hci_cmd_v1_daa(struct i3c_hci *hci)
> * Yes, there is room for improvements.
> */
> for (;;) {
> - ret = mipi_i3c_hci_dat_v1.alloc_entry(hci);
> - if (ret < 0)
> - break;
> - dat_idx = ret;
> ret = i3c_master_get_free_addr(&hci->master, next_addr);
> if (ret < 0)
> break;
> next_addr = ret;
> + ret = mipi_i3c_hci_dat_v1.alloc_entry(hci, next_addr);
> + if (ret < 0)
> + break;
> + dat_idx = ret;
>
> dev_dbg(&hci->master.dev,
> "next_addr = 0x%02x, DAA using DAT %d",
> next_addr, dat_idx);
> - mipi_i3c_hci_dat_v1.set_dynamic_addr(hci, dat_idx, next_addr);
> + ret = mipi_i3c_hci_dat_v1.set_dynamic_addr(hci, dat_idx,
> + next_addr);
> + if (ret < 0)
> + break;
> mipi_i3c_hci_dct_index_reset(hci);
>
> xfer->cmd_tid = hci_get_tid();
> @@ -326,7 +337,7 @@ static int hci_cmd_v1_daa(struct i3c_hci *hci)
> CMD_0_ATTR_A |
> CMD_A0_TID(xfer->cmd_tid) |
> CMD_A0_CMD(I3C_CCC_ENTDAA) |
> - CMD_A0_DEV_INDEX(dat_idx) |
> + cmd_v1_dev_index(hci, dat_idx) |
> CMD_A0_DEV_COUNT(1) |
> CMD_A0_ROC | CMD_A0_TOC;
> xfer->cmd_desc[1] = 0;
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index dadf049bd4b54..a624e3c40484e 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -574,7 +574,8 @@ static int i3c_hci_attach_i3c_dev(struct i3c_dev_desc *dev)
> if (!dev_data)
> return -ENOMEM;
> if (hci->cmd == &mipi_i3c_hci_cmd_v1) {
> - ret = mipi_i3c_hci_dat_v1.alloc_entry(hci);
> + ret = mipi_i3c_hci_dat_v1.alloc_entry(hci,
> + dev->info.dyn_addr ?: dev->info.static_addr);
> if (ret < 0) {
> kfree(dev_data);
> return ret;
> @@ -593,9 +594,17 @@ static int i3c_hci_reattach_i3c_dev(struct i3c_dev_desc *dev, u8 old_dyn_addr)
> struct i3c_hci *hci = to_i3c_hci(m);
> struct i3c_hci_dev_data *dev_data = i3c_dev_get_master_data(dev);
>
> - if (hci->cmd == &mipi_i3c_hci_cmd_v1)
> - mipi_i3c_hci_dat_v1.set_dynamic_addr(hci, dev_data->dat_idx,
> - dev->info.dyn_addr);
> + if (hci->cmd == &mipi_i3c_hci_cmd_v1) {
> + int ret;
> +
> + ret = mipi_i3c_hci_dat_v1.set_dynamic_addr(hci,
> + dev_data->dat_idx,
> + dev->info.dyn_addr);
> + if (ret)
> + return ret;
> + if (hci->quirks & HCI_QUIRK_DAT_INDEX_IS_ADDR)
> + dev_data->dat_idx = dev->info.dyn_addr;
> + }
> return 0;
> }
>
> @@ -623,7 +632,7 @@ static int i3c_hci_attach_i2c_dev(struct i2c_dev_desc *dev)
> dev_data = kzalloc_obj(*dev_data);
> if (!dev_data)
> return -ENOMEM;
> - ret = mipi_i3c_hci_dat_v1.alloc_entry(hci);
> + ret = mipi_i3c_hci_dat_v1.alloc_entry(hci, dev->addr);
> if (ret < 0) {
> kfree(dev_data);
> return ret;
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dat.h b/drivers/i3c/master/mipi-i3c-hci/dat.h
> index 6881f19da77f2..d4c7b03724d37 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dat.h
> +++ b/drivers/i3c/master/mipi-i3c-hci/dat.h
> @@ -14,12 +14,13 @@
> #define DAT_0_I2C_DEVICE W0_BIT_(31)
> #define DAT_0_SIR_REJECT W0_BIT_(13)
> #define DAT_0_IBI_PAYLOAD W0_BIT_(12)
> +#define HCI_DAT_ANY_ADDRESS ((unsigned int)-1)
>
> struct hci_dat_ops {
> int (*init)(struct i3c_hci *hci);
> - int (*alloc_entry)(struct i3c_hci *hci);
> + int (*alloc_entry)(struct i3c_hci *hci, unsigned int address);
> void (*free_entry)(struct i3c_hci *hci, unsigned int dat_idx);
> - void (*set_dynamic_addr)(struct i3c_hci *hci, unsigned int dat_idx, u8 addr);
> + int (*set_dynamic_addr)(struct i3c_hci *hci, unsigned int dat_idx, u8 addr);
> void (*set_static_addr)(struct i3c_hci *hci, unsigned int dat_idx, u8 addr);
> void (*set_flags)(struct i3c_hci *hci, unsigned int dat_idx, u32 w0, u32 w1);
> void (*clear_flags)(struct i3c_hci *hci, unsigned int dat_idx, u32 w0, u32 w1);
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> index 852966aa20d98..19b0f72e4ac82 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dat_v1.c
> @@ -89,7 +89,7 @@ static int hci_dat_v1_init(struct i3c_hci *hci)
> return 0;
> }
>
> -static int hci_dat_v1_alloc_entry(struct i3c_hci *hci)
> +static int hci_dat_v1_alloc_entry(struct i3c_hci *hci, unsigned int address)
> {
> unsigned int dat_idx;
> int ret;
> @@ -99,9 +99,17 @@ static int hci_dat_v1_alloc_entry(struct i3c_hci *hci)
> if (ret)
> return ret;
> }
> - dat_idx = find_first_zero_bit(hci->DAT_data, hci->DAT_entries);
> - if (dat_idx >= hci->DAT_entries)
> - return -ENOENT;
> + if (hci->quirks & HCI_QUIRK_DAT_INDEX_IS_ADDR) {
> + if (address >= hci->DAT_entries)
> + return -EINVAL;
> + if (test_bit_acquire(address, hci->DAT_data))
> + return -ENOENT;
> + dat_idx = address;
> + } else {
> + dat_idx = find_first_zero_bit(hci->DAT_data, hci->DAT_entries);
> + if (dat_idx >= hci->DAT_entries)
> + return -ENOENT;
> + }
> __set_bit(dat_idx, hci->DAT_data);
>
> /* default flags */
> @@ -118,9 +126,43 @@ static void hci_dat_v1_free_entry(struct i3c_hci *hci, unsigned int dat_idx)
> __clear_bit(dat_idx, hci->DAT_data);
> }
>
> -static void hci_dat_v1_set_dynamic_addr(struct i3c_hci *hci,
> - unsigned int dat_idx, u8 address)
> +static int hci_dat_v1_set_dynamic_addr(struct i3c_hci *hci,
> + unsigned int dat_idx, u8 address)
> {
> + if (hci->quirks & HCI_QUIRK_DAT_INDEX_IS_ADDR) {
> + u32 dat_w0, dat_w1;
> +
> + /*
> + * The controller derives the device address from the DAT slot
> + * index, so DAT_0_DYNAMIC_ADDRESS is never written: an address
> + * change moves the whole entry to the slot named by the new
> + * address instead.
> + */
> + if (dat_idx == address)
> + return 0;
> + if (address >= hci->DAT_entries) {
> + dev_err(&hci->master.dev, "invalid DAT slot %u\n", address);
> + return -EINVAL;
> + }
> + if (test_bit(address, hci->DAT_data)) {
> + dev_err(&hci->master.dev, "DAT slot %u already in use\n",
> + address);
> + return -EBUSY;
> + }
> +
> + dat_w0 = dat_w0_read(dat_idx);
> + dat_w1 = dat_w1_read(dat_idx);
> +
> + __set_bit(address, hci->DAT_data);
> + dat_w0_write(address, dat_w0);
> + dat_w1_write(address, dat_w1);
> +
> + dat_w0_write(dat_idx, 0);
> + dat_w1_write(dat_idx, 0);
> + __clear_bit(dat_idx, hci->DAT_data);
> + return 0;
> + }
> +
> u32 dat_w0;
>
> dat_w0 = dat_w0_read(dat_idx);
> @@ -128,6 +170,8 @@ static void hci_dat_v1_set_dynamic_addr(struct i3c_hci *hci,
> dat_w0 |= FIELD_PREP(DAT_0_DYNAMIC_ADDRESS, address) |
> (parity8(address) ? 0 : DAT_0_DYNADDR_PARITY);
> dat_w0_write(dat_idx, dat_w0);
> +
> + return 0;
> }
>
> static void hci_dat_v1_set_static_addr(struct i3c_hci *hci,
> @@ -169,6 +213,13 @@ static void hci_dat_v1_clear_flags(struct i3c_hci *hci, unsigned int dat_idx,
>
> static int hci_dat_v1_get_index(struct i3c_hci *hci, u8 dev_addr)
> {
> + if (hci->quirks & HCI_QUIRK_DAT_INDEX_IS_ADDR) {
> + if (dev_addr < hci->DAT_entries &&
> + test_bit(dev_addr, hci->DAT_data))
> + return dev_addr;
> + return -ENODEV;
> + }
> +
> unsigned int dat_idx;
> u32 dat_w0;
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
> index ee73f6e6756a1..f50fc1e22a855 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/hci.h
> +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
> @@ -177,6 +177,7 @@ struct i3c_hci_dev_data {
> #define HCI_QUIRK_RPM_PARENT_MANAGED BIT(7) /* Runtime PM managed by parent device */
> #define HCI_QUIRK_DMA_ABORT_REQUIRES_PIO_RESET BIT(8) /* Do PIO queue SW resets after DMA abort */
> #define HCI_QUIRK_DMA_REQUIRES_HC_ABORT BIT(9) /* Use HC_CONTROL ABORT to abort DMA */
> +#define HCI_QUIRK_DAT_INDEX_IS_ADDR BIT(10) /* DAT entries are indexed by device address */
>
> /* global functions */
> void mipi_i3c_hci_resume(struct i3c_hci *hci);
>
> --
> 2.34.1
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/8] i3c: mipi-i3c-hci: Add a quirk for 64-bit DMA addressing
2026-09-01 11:35 [PATCH 0/8] i3c: mipi-i3c-hci: Add Aspeed AST2700 support Billy Tsai
2026-09-01 11:35 ` [PATCH 1/8] dt-bindings: i3c: Document the AST2700 I3C controller Billy Tsai
2026-09-01 11:35 ` [PATCH 2/8] i3c: mipi-i3c-hci: Support address-indexed DAT slots Billy Tsai
@ 2026-09-01 11:35 ` Billy Tsai
2026-09-01 11:55 ` sashiko-bot
2026-09-01 20:51 ` Frank Li
2026-09-01 11:35 ` [PATCH 4/8] i3c: mipi-i3c-hci: Add a quirk to clear the TX start threshold Billy Tsai
` (4 subsequent siblings)
7 siblings, 2 replies; 19+ messages in thread
From: Billy Tsai @ 2026-09-01 11:35 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
Cc: linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, Billy Tsai
The HCI ring base registers are 64 bits wide by specification, but not
every implementation wires the upper half, so the driver leaves the
platform device at the default 32-bit DMA mask unless told otherwise.
ASPEED platforms place all of DRAM above the 32-bit boundary
(0x4_00000000), so under the default mask every dma_alloc_coherent()
call for the DMA rings fails.
Add HCI_QUIRK_DMA_64BIT for controllers whose DMA engine drives the
full address width, and declare a 64-bit streaming and coherent DMA
mask for them before the rings are allocated.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Assisted-by: Claude:claude-fable-5
---
drivers/i3c/master/mipi-i3c-hci/core.c | 7 +++++++
drivers/i3c/master/mipi-i3c-hci/hci.h | 1 +
2 files changed, 8 insertions(+)
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index a624e3c40484..c03c3a9cbe4f 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -10,6 +10,7 @@
#include <linux/bitfield.h>
#include <linux/clk.h>
#include <linux/device.h>
+#include <linux/dma-mapping.h>
#include <linux/errno.h>
#include <linux/i3c/master.h>
#include <linux/interrupt.h>
@@ -1167,6 +1168,12 @@ static int i3c_hci_probe(struct platform_device *pdev)
hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);
if (!hci->quirks && platform_get_device_id(pdev))
hci->quirks = platform_get_device_id(pdev)->driver_data;
+ if (hci->quirks & HCI_QUIRK_DMA_64BIT) {
+ ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "cannot set DMA mask\n");
+ }
ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &clks);
if (ret < 0)
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index f50fc1e22a85..2110f806a53c 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -178,6 +178,7 @@ struct i3c_hci_dev_data {
#define HCI_QUIRK_DMA_ABORT_REQUIRES_PIO_RESET BIT(8) /* Do PIO queue SW resets after DMA abort */
#define HCI_QUIRK_DMA_REQUIRES_HC_ABORT BIT(9) /* Use HC_CONTROL ABORT to abort DMA */
#define HCI_QUIRK_DAT_INDEX_IS_ADDR BIT(10) /* DAT entries are indexed by device address */
+#define HCI_QUIRK_DMA_64BIT BIT(11) /* Controller DMA supports 64-bit addressing */
/* global functions */
void mipi_i3c_hci_resume(struct i3c_hci *hci);
--
2.34.1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/8] i3c: mipi-i3c-hci: Add a quirk for 64-bit DMA addressing
2026-09-01 11:35 ` [PATCH 3/8] i3c: mipi-i3c-hci: Add a quirk for 64-bit DMA addressing Billy Tsai
@ 2026-09-01 11:55 ` sashiko-bot
2026-09-01 20:51 ` Frank Li
1 sibling, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-09-01 11:55 UTC (permalink / raw)
To: Billy Tsai
Cc: linux-i3c, Alexandre Belloni, Frank.Li, devicetree, conor+dt,
robh
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] A malicious or non-compliant hardware controller can trigger a division-by-zero kernel panic during probe by reporting a non-zero DAT_ENTRY_SIZE or DCT_ENTRY_SIZE.
- [High] If an I3C transfer is queued but never started by the hardware, the waiting thread will enter an infinite loop in the kernel, leading to a permanent hang.
- [High] A malicious I3C hardware controller can trigger a heap out-of-bounds read by reporting an arbitrarily large number of chunks in IBI status descriptors.
--
commit a8b22e15760d201630758778160cab05d823bd74
Author: Billy Tsai <billy_tsai@aspeedtech.com>
i3c: mipi-i3c-hci: Add a quirk for 64-bit DMA addressing
This commit adds a HCI_QUIRK_DMA_64BIT quirk to declare a 64-bit
streaming and coherent DMA mask for I3C controllers that support it.
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index a624e3c40484e..c03c3a9cbe4f9 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
[ ... ]
> @@ -1167,6 +1168,12 @@ static int i3c_hci_probe(struct platform_device *pdev)
> hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);
> if (!hci->quirks && platform_get_device_id(pdev))
> hci->quirks = platform_get_device_id(pdev)->driver_data;
> + if (hci->quirks & HCI_QUIRK_DMA_64BIT) {
> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "cannot set DMA mask\n");
> + }
[Severity: High]
This isn't a bug introduced by this patch, but could a malicious or buggy
hardware controller trigger a kernel panic due to division by zero?
In i3c_hci_init(), if the hardware is version 1.0 (where size_in_dwords is
true) and reports a non-zero DAT_ENTRY_SIZE, the driver sets DAT_entry_size
to 0 and then immediately divides by it:
drivers/i3c/master/mipi-i3c-hci/core.c:i3c_hci_init() {
...
hci->DAT_entry_size = FIELD_GET(DAT_ENTRY_SIZE, regval) ? 0 : 8;
if (size_in_dwords)
hci->DAT_entries = 4 * hci->DAT_entries / hci->DAT_entry_size;
...
}
[Severity: High]
This is a pre-existing issue, but could an unresponsive hardware controller
cause a permanent kernel thread hang?
In i3c_hci_process_xfer(), if the hardware stalls or the DMA ring is blocked,
the transfer might never be marked as started. The loop checks for a timeout,
but remaining_timeout is not decremented, causing an infinite loop:
drivers/i3c/master/mipi-i3c-hci/core.c:i3c_hci_process_xfer() {
...
/* Keep waiting if xfer has not started */
if (!started)
continue;
...
}
[Severity: High]
This isn't a bug introduced by this patch, but can an out-of-bounds heap
read happen if a hardware controller reports an arbitrarily large number of
chunks?
In hci_dma_process_ibi(), ibi_size is calculated from chunk counts reported
in IBI status descriptors. If the calculated ibi_size exceeds the physical
ring buffer size (rh->ibi_chunks_total * rh->ibi_chunk_sz), this could
overflow the rh->ibi_data buffer during memcpy:
drivers/i3c/master/mipi-i3c-hci/dma.c:hci_dma_process_ibi() {
...
memcpy(slot->data + first_part, ring_ibi_data,
ibi_size - first_part);
...
}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-b4-i3c-hci-ast2700-v1-0-19909e7cbd7e@aspeedtech.com?part=3
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/8] i3c: mipi-i3c-hci: Add a quirk for 64-bit DMA addressing
2026-09-01 11:35 ` [PATCH 3/8] i3c: mipi-i3c-hci: Add a quirk for 64-bit DMA addressing Billy Tsai
2026-09-01 11:55 ` sashiko-bot
@ 2026-09-01 20:51 ` Frank Li
1 sibling, 0 replies; 19+ messages in thread
From: Frank Li @ 2026-09-01 20:51 UTC (permalink / raw)
To: Billy Tsai
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel,
linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel
On Tue, Sep 01, 2026 at 07:35:30PM +0800, Billy Tsai wrote:
> The HCI ring base registers are 64 bits wide by specification, but not
> every implementation wires the upper half, so the driver leaves the
> platform device at the default 32-bit DMA mask unless told otherwise.
> ASPEED platforms place all of DRAM above the 32-bit boundary
> (0x4_00000000), so under the default mask every dma_alloc_coherent()
> call for the DMA rings fails.
>
> Add HCI_QUIRK_DMA_64BIT for controllers whose DMA engine drives the
> full address width, and declare a 64-bit streaming and coherent DMA
> mask for them before the rings are allocated.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Assisted-by: Claude:claude-fable-5
> ---
> drivers/i3c/master/mipi-i3c-hci/core.c | 7 +++++++
> drivers/i3c/master/mipi-i3c-hci/hci.h | 1 +
> 2 files changed, 8 insertions(+)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index a624e3c40484..c03c3a9cbe4f 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -10,6 +10,7 @@
> #include <linux/bitfield.h>
> #include <linux/clk.h>
> #include <linux/device.h>
> +#include <linux/dma-mapping.h>
> #include <linux/errno.h>
> #include <linux/i3c/master.h>
> #include <linux/interrupt.h>
> @@ -1167,6 +1168,12 @@ static int i3c_hci_probe(struct platform_device *pdev)
> hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);
> if (!hci->quirks && platform_get_device_id(pdev))
> hci->quirks = platform_get_device_id(pdev)->driver_data;
> + if (hci->quirks & HCI_QUIRK_DMA_64BIT) {
> + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
dma_set_mask_and_coherent() never return fail when bit mask >= 32
if (hci->quirks & HCI_QUIRK_DMA_64BIT)
dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
Frank
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "cannot set DMA mask\n");
> + }
>
> ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &clks);
> if (ret < 0)
> diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
> index f50fc1e22a85..2110f806a53c 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/hci.h
> +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
> @@ -178,6 +178,7 @@ struct i3c_hci_dev_data {
> #define HCI_QUIRK_DMA_ABORT_REQUIRES_PIO_RESET BIT(8) /* Do PIO queue SW resets after DMA abort */
> #define HCI_QUIRK_DMA_REQUIRES_HC_ABORT BIT(9) /* Use HC_CONTROL ABORT to abort DMA */
> #define HCI_QUIRK_DAT_INDEX_IS_ADDR BIT(10) /* DAT entries are indexed by device address */
> +#define HCI_QUIRK_DMA_64BIT BIT(11) /* Controller DMA supports 64-bit addressing */
>
> /* global functions */
> void mipi_i3c_hci_resume(struct i3c_hci *hci);
>
> --
> 2.34.1
>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/8] i3c: mipi-i3c-hci: Add a quirk to clear the TX start threshold
2026-09-01 11:35 [PATCH 0/8] i3c: mipi-i3c-hci: Add Aspeed AST2700 support Billy Tsai
` (2 preceding siblings ...)
2026-09-01 11:35 ` [PATCH 3/8] i3c: mipi-i3c-hci: Add a quirk for 64-bit DMA addressing Billy Tsai
@ 2026-09-01 11:35 ` Billy Tsai
2026-09-01 11:49 ` sashiko-bot
2026-09-01 20:58 ` Frank Li
2026-09-01 11:35 ` [PATCH 5/8] i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller Billy Tsai
` (3 subsequent siblings)
7 siblings, 2 replies; 19+ messages in thread
From: Billy Tsai @ 2026-09-01 11:35 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
Cc: linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, Billy Tsai
The DATA_TX_START_THLD field of the PIO data buffer threshold register
holds off the start of a TX transfer until the FIFO holds a set amount
of data, which in PIO mode cuts down on the number of software writes.
The field resets to 0x1, requiring at least (2 ^ 2) DWORDs (16 bytes)
in the FIFO before transmission starts.
HCI controllers that support both PIO and DMA can expose the two as
separate register blocks; nothing in the specification says selecting
DMA mode disables the PIO block's own gating logic. On ASPEED
platforms it doesn't: DATA_TX_START_THLD still holds up transfer
start regardless of which mode feeds the FIFO, and in DMA mode that
threshold is never satisfied for some transfer sizes:
- 1-4 bytes: uses the Immediate Data Transfer Command.
- 13+ bytes: since the hardware fetches data in 4-byte chunks, a
13-byte transfer fetches 16 bytes into the FIFO and reaches the
threshold.
- 5-12 bytes: the threshold is never reached and the transfer stalls.
Add HCI_QUIRK_TX_START_THLD to clear the field whenever DMA mode is
selected. The clear lives in i3c_hci_set_io_mode() rather than in
probe so the reset-and-restore recovery path also reapplies it after
a controller soft reset. Move the PIO Access Area register and
bitfield definitions from pio.c to a new pio.h so this quirk can reuse
PIO_DATA_BUFFER_THLD_CTRL and DATA_TX_START_THLD instead of
redefining them.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Assisted-by: Claude:claude-fable-5
---
drivers/i3c/master/mipi-i3c-hci/core.c | 14 +++++
drivers/i3c/master/mipi-i3c-hci/hci.h | 1 +
drivers/i3c/master/mipi-i3c-hci/pio.c | 97 +----------------------------
drivers/i3c/master/mipi-i3c-hci/pio.h | 109 +++++++++++++++++++++++++++++++++
4 files changed, 125 insertions(+), 96 deletions(-)
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index c03c3a9cbe4f..2290a889701c 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -26,6 +26,7 @@
#include "cmd.h"
#include "dat.h"
#include "ibi.h"
+#include "pio.h"
/*
* Host Controller Capabilities and Operation Registers
@@ -823,6 +824,19 @@ static int i3c_hci_set_io_mode(struct i3c_hci *hci, bool dma)
else
reg_set(HC_CONTROL, HC_CONTROL_PIO_MODE);
+ /*
+ * On the ASPEED AST2700 the TX start threshold gates transfer start
+ * even in DMA mode. Clear it so DMA transfers are not held back
+ * waiting for a PIO FIFO level that will never be reached.
+ */
+ if (dma && (hci->quirks & HCI_QUIRK_TX_START_THLD) && hci->PIO_regs) {
+ void __iomem *thld_reg = hci->PIO_regs + PIO_DATA_BUFFER_THLD_CTRL;
+ u32 thld_val = readl(thld_reg);
+
+ thld_val &= ~DATA_TX_START_THLD;
+ writel(thld_val, thld_reg);
+ }
+
if (!is_version_1_1_or_newer(hci))
return 0;
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index 2110f806a53c..d9c9e609a879 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -179,6 +179,7 @@ struct i3c_hci_dev_data {
#define HCI_QUIRK_DMA_REQUIRES_HC_ABORT BIT(9) /* Use HC_CONTROL ABORT to abort DMA */
#define HCI_QUIRK_DAT_INDEX_IS_ADDR BIT(10) /* DAT entries are indexed by device address */
#define HCI_QUIRK_DMA_64BIT BIT(11) /* Controller DMA supports 64-bit addressing */
+#define HCI_QUIRK_TX_START_THLD BIT(12) /* Clear TX start threshold in DMA mode */
/* global functions */
void mipi_i3c_hci_resume(struct i3c_hci *hci);
diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
index 439578a6eb54..6bd5e43364a1 100644
--- a/drivers/i3c/master/mipi-i3c-hci/pio.c
+++ b/drivers/i3c/master/mipi-i3c-hci/pio.c
@@ -14,6 +14,7 @@
#include "hci.h"
#include "cmd.h"
#include "ibi.h"
+#include "pio.h"
/*
* PIO Access Area
@@ -22,102 +23,6 @@
#define pio_reg_read(r) readl(hci->PIO_regs + (PIO_##r))
#define pio_reg_write(r, v) writel(v, hci->PIO_regs + (PIO_##r))
-#define PIO_COMMAND_QUEUE_PORT 0x00
-#define PIO_RESPONSE_QUEUE_PORT 0x04
-#define PIO_XFER_DATA_PORT 0x08
-#define PIO_IBI_PORT 0x0c
-
-#define PIO_QUEUE_THLD_CTRL 0x10
-#define QUEUE_IBI_STATUS_THLD GENMASK(31, 24)
-#define QUEUE_IBI_DATA_THLD GENMASK(23, 16)
-#define QUEUE_RESP_BUF_THLD GENMASK(15, 8)
-#define QUEUE_CMD_EMPTY_BUF_THLD GENMASK(7, 0)
-
-#define PIO_DATA_BUFFER_THLD_CTRL 0x14
-#define DATA_RX_START_THLD GENMASK(26, 24)
-#define DATA_TX_START_THLD GENMASK(18, 16)
-#define DATA_RX_BUF_THLD GENMASK(10, 8)
-#define DATA_TX_BUF_THLD GENMASK(2, 0)
-
-#define PIO_QUEUE_SIZE 0x18
-#define TX_DATA_BUFFER_SIZE GENMASK(31, 24)
-#define RX_DATA_BUFFER_SIZE GENMASK(23, 16)
-#define IBI_STATUS_SIZE GENMASK(15, 8)
-#define CR_QUEUE_SIZE GENMASK(7, 0)
-
-#define PIO_ALT_QUEUE_SIZE 0x1C
-#define EXT_IBI_QUEUE_EN BIT(28)
-#define ALT_RESP_QUEUE_EN BIT(24)
-#define ALT_RESP_QUEUE_SIZE GENMASK(7, 0)
-
-#define PIO_INTR_STATUS 0x20
-#define PIO_INTR_STATUS_ENABLE 0x24
-#define PIO_INTR_SIGNAL_ENABLE 0x28
-#define PIO_INTR_FORCE 0x2c
-#define STAT_TRANSFER_BLOCKED BIT(25)
-#define STAT_PERR_RESP_UFLOW BIT(24)
-#define STAT_PERR_CMD_OFLOW BIT(23)
-#define STAT_PERR_IBI_UFLOW BIT(22)
-#define STAT_PERR_RX_UFLOW BIT(21)
-#define STAT_PERR_TX_OFLOW BIT(20)
-#define STAT_ERR_RESP_QUEUE_FULL BIT(19)
-#define STAT_WARN_RESP_QUEUE_FULL BIT(18)
-#define STAT_ERR_IBI_QUEUE_FULL BIT(17)
-#define STAT_WARN_IBI_QUEUE_FULL BIT(16)
-#define STAT_ERR_RX_DATA_FULL BIT(15)
-#define STAT_WARN_RX_DATA_FULL BIT(14)
-#define STAT_ERR_TX_DATA_EMPTY BIT(13)
-#define STAT_WARN_TX_DATA_EMPTY BIT(12)
-#define STAT_TRANSFER_ERR BIT(9)
-#define STAT_WARN_INS_STOP_MODE BIT(7)
-#define STAT_TRANSFER_ABORT BIT(5)
-#define STAT_RESP_READY BIT(4)
-#define STAT_CMD_QUEUE_READY BIT(3)
-#define STAT_IBI_STATUS_THLD BIT(2)
-#define STAT_RX_THLD BIT(1)
-#define STAT_TX_THLD BIT(0)
-
-#define PIO_CONTROL 0x30
-#define PIO_CONTROL_ABORT BIT(2)
-#define PIO_CONTROL_RS BIT(1)
-#define PIO_CONTROL_ENABLE BIT(0)
-
-#define PIO_QUEUE_CUR_STATUS 0x38
-#define CUR_IBI_Q_LEVEL GENMASK(28, 20)
-#define CUR_RESP_Q_LEVEL GENMASK(18, 10)
-#define CUR_CMD_Q_EMPTY_LEVEL GENMASK(8, 0)
-
-#define PIO_DATA_BUFFER_CUR_STATUS 0x3c
-#define CUR_RX_BUF_LVL GENMASK(26, 16)
-#define CUR_TX_BUF_LVL GENMASK(10, 0)
-
-/*
- * Handy status bit combinations
- */
-
-#define STAT_LATENCY_WARNINGS (STAT_WARN_RESP_QUEUE_FULL | \
- STAT_WARN_IBI_QUEUE_FULL | \
- STAT_WARN_RX_DATA_FULL | \
- STAT_WARN_TX_DATA_EMPTY | \
- STAT_WARN_INS_STOP_MODE)
-
-#define STAT_LATENCY_ERRORS (STAT_ERR_RESP_QUEUE_FULL | \
- STAT_ERR_IBI_QUEUE_FULL | \
- STAT_ERR_RX_DATA_FULL | \
- STAT_ERR_TX_DATA_EMPTY)
-
-#define STAT_PROG_ERRORS (STAT_TRANSFER_BLOCKED | \
- STAT_PERR_RESP_UFLOW | \
- STAT_PERR_CMD_OFLOW | \
- STAT_PERR_IBI_UFLOW | \
- STAT_PERR_RX_UFLOW | \
- STAT_PERR_TX_OFLOW)
-
-#define STAT_ALL_ERRORS (STAT_TRANSFER_ABORT | \
- STAT_TRANSFER_ERR | \
- STAT_LATENCY_ERRORS | \
- STAT_PROG_ERRORS)
-
struct hci_pio_dev_ibi_data {
struct i3c_generic_ibi_pool *pool;
unsigned int max_len;
diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.h b/drivers/i3c/master/mipi-i3c-hci/pio.h
new file mode 100644
index 000000000000..72318961e991
--- /dev/null
+++ b/drivers/i3c/master/mipi-i3c-hci/pio.h
@@ -0,0 +1,109 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Copyright (c) 2020, MIPI Alliance, Inc.
+ *
+ * Author: Nicolas Pitre <npitre@baylibre.com>
+ *
+ * PIO Access Area register and bitfield definitions.
+ */
+
+#ifndef PIO_H
+#define PIO_H
+
+#define PIO_COMMAND_QUEUE_PORT 0x00
+#define PIO_RESPONSE_QUEUE_PORT 0x04
+#define PIO_XFER_DATA_PORT 0x08
+#define PIO_IBI_PORT 0x0c
+
+#define PIO_QUEUE_THLD_CTRL 0x10
+#define QUEUE_IBI_STATUS_THLD GENMASK(31, 24)
+#define QUEUE_IBI_DATA_THLD GENMASK(23, 16)
+#define QUEUE_RESP_BUF_THLD GENMASK(15, 8)
+#define QUEUE_CMD_EMPTY_BUF_THLD GENMASK(7, 0)
+
+#define PIO_DATA_BUFFER_THLD_CTRL 0x14
+#define DATA_TX_START_THLD GENMASK(18, 16)
+#define DATA_RX_START_THLD GENMASK(26, 24)
+#define DATA_RX_BUF_THLD GENMASK(10, 8)
+#define DATA_TX_BUF_THLD GENMASK(2, 0)
+
+#define PIO_QUEUE_SIZE 0x18
+#define TX_DATA_BUFFER_SIZE GENMASK(31, 24)
+#define RX_DATA_BUFFER_SIZE GENMASK(23, 16)
+#define IBI_STATUS_SIZE GENMASK(15, 8)
+#define CR_QUEUE_SIZE GENMASK(7, 0)
+
+#define PIO_ALT_QUEUE_SIZE 0x1C
+#define EXT_IBI_QUEUE_EN BIT(28)
+#define ALT_RESP_QUEUE_EN BIT(24)
+#define ALT_RESP_QUEUE_SIZE GENMASK(7, 0)
+
+#define PIO_INTR_STATUS 0x20
+#define PIO_INTR_STATUS_ENABLE 0x24
+#define PIO_INTR_SIGNAL_ENABLE 0x28
+#define PIO_INTR_FORCE 0x2c
+#define STAT_TRANSFER_BLOCKED BIT(25)
+#define STAT_PERR_RESP_UFLOW BIT(24)
+#define STAT_PERR_CMD_OFLOW BIT(23)
+#define STAT_PERR_IBI_UFLOW BIT(22)
+#define STAT_PERR_RX_UFLOW BIT(21)
+#define STAT_PERR_TX_OFLOW BIT(20)
+#define STAT_ERR_RESP_QUEUE_FULL BIT(19)
+#define STAT_WARN_RESP_QUEUE_FULL BIT(18)
+#define STAT_ERR_IBI_QUEUE_FULL BIT(17)
+#define STAT_WARN_IBI_QUEUE_FULL BIT(16)
+#define STAT_ERR_RX_DATA_FULL BIT(15)
+#define STAT_WARN_RX_DATA_FULL BIT(14)
+#define STAT_ERR_TX_DATA_EMPTY BIT(13)
+#define STAT_WARN_TX_DATA_EMPTY BIT(12)
+#define STAT_TRANSFER_ERR BIT(9)
+#define STAT_WARN_INS_STOP_MODE BIT(7)
+#define STAT_TRANSFER_ABORT BIT(5)
+#define STAT_RESP_READY BIT(4)
+#define STAT_CMD_QUEUE_READY BIT(3)
+#define STAT_IBI_STATUS_THLD BIT(2)
+#define STAT_RX_THLD BIT(1)
+#define STAT_TX_THLD BIT(0)
+
+#define PIO_CONTROL 0x30
+#define PIO_CONTROL_ABORT BIT(2)
+#define PIO_CONTROL_RS BIT(1)
+#define PIO_CONTROL_ENABLE BIT(0)
+
+#define PIO_QUEUE_CUR_STATUS 0x38
+#define CUR_IBI_Q_LEVEL GENMASK(28, 20)
+#define CUR_RESP_Q_LEVEL GENMASK(18, 10)
+#define CUR_CMD_Q_EMPTY_LEVEL GENMASK(8, 0)
+
+#define PIO_DATA_BUFFER_CUR_STATUS 0x3c
+#define CUR_RX_BUF_LVL GENMASK(26, 16)
+#define CUR_TX_BUF_LVL GENMASK(10, 0)
+
+/*
+ * Handy status bit combinations
+ */
+
+#define STAT_LATENCY_WARNINGS (STAT_WARN_RESP_QUEUE_FULL | \
+ STAT_WARN_IBI_QUEUE_FULL | \
+ STAT_WARN_RX_DATA_FULL | \
+ STAT_WARN_TX_DATA_EMPTY | \
+ STAT_WARN_INS_STOP_MODE)
+
+#define STAT_LATENCY_ERRORS (STAT_ERR_RESP_QUEUE_FULL | \
+ STAT_ERR_IBI_QUEUE_FULL | \
+ STAT_ERR_RX_DATA_FULL | \
+ STAT_ERR_TX_DATA_EMPTY)
+
+#define STAT_PROG_ERRORS (STAT_TRANSFER_BLOCKED | \
+ STAT_PERR_RESP_UFLOW | \
+ STAT_PERR_CMD_OFLOW | \
+ STAT_PERR_IBI_UFLOW | \
+ STAT_PERR_RX_UFLOW | \
+ STAT_PERR_TX_OFLOW)
+
+#define STAT_ALL_ERRORS (STAT_TRANSFER_ABORT | \
+ STAT_TRANSFER_ERR | \
+ STAT_LATENCY_ERRORS | \
+ STAT_PROG_ERRORS)
+
+#endif
--
2.34.1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 4/8] i3c: mipi-i3c-hci: Add a quirk to clear the TX start threshold
2026-09-01 11:35 ` [PATCH 4/8] i3c: mipi-i3c-hci: Add a quirk to clear the TX start threshold Billy Tsai
@ 2026-09-01 11:49 ` sashiko-bot
2026-09-01 20:58 ` Frank Li
1 sibling, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-09-01 11:49 UTC (permalink / raw)
To: Billy Tsai
Cc: Frank.Li, linux-i3c, Alexandre Belloni, robh, conor+dt,
devicetree
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Endianness Portability Bug: Register accessors used for stream FIFOs
- [High] Out-of-bounds Read when handling trailing bytes in TX FIFO
- [High] Unaligned Memory Access when casting caller-provided buffer
--
commit 88ae252ec612f81d6b6f55d8dd0ef78352a438bf
Author: Billy Tsai <billy_tsai@aspeedtech.com>
i3c: mipi-i3c-hci: Add a quirk to clear the TX start threshold
This commit introduces a new quirk, HCI_QUIRK_TX_START_THLD, to clear the
TX start threshold in DMA mode on ASPEED platforms to prevent transfer
stalls. It also moves the PIO access area definitions from pio.c to a new
pio.h header file so they can be shared.
> diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
> index 439578a6eb54c..6bd5e43364a1d 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/pio.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/pio.c
[ ... ]
While reviewing this file, I noticed a few pre-existing issues that are not
introduced by this patch, but remain uncorrected in this series.
In drivers/i3c/master/mipi-i3c-hci/pio.c:hci_pio_do_rx():
while (xfer->data_left >= 4) {
/* bail out if FIFO hasn't reached the threshold value yet */
if (!(pio_reg_read(INTR_STATUS) & STAT_RX_THLD))
return false;
nr_words = min(xfer->data_left / 4, pio->rx_thresh_size);
/* extract data from FIFO */
xfer->data_left -= nr_words * 4;
dev_dbg(&hci->master.dev, "now %d left %d",
nr_words * 4, xfer->data_left);
while (nr_words--)
*p++ = pio_reg_read(XFER_DATA_PORT);
}
[Severity: High]
This isn't a bug introduced by this patch, but does reading from the stream
FIFO (XFER_DATA_PORT) using pio_reg_read() cause unintended byteswapping on
big-endian architectures?
According to the I/O Accessors Subsystem Guidelines, stream FIFOs must
exclusively use stream accessors (readsl() / writesl()) to avoid corrupting
the I3C byte stream payload on big-endian systems.
Additionally, if the caller provides an arbitrary buffer in xfer->data that
is not aligned to a 4-byte boundary, doesn't casting it directly to a u32
pointer and dereferencing it via *p++ cause an alignment fault (kernel
panic) on architectures that do not support hardware unaligned accesses?
The same patterns appear in hci_pio_do_tx() for writes:
drivers/i3c/master/mipi-i3c-hci/pio.c:hci_pio_do_tx():
while (xfer->data_left >= 4) {
/* bail out if FIFO free space is below set threshold */
if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD))
return false;
/* we can fill up to that TX threshold */
nr_words = min(xfer->data_left / 4, pio->tx_thresh_size);
/* push data into the FIFO */
xfer->data_left -= nr_words * 4;
dev_dbg(&hci->master.dev, "now %d left %d",
nr_words * 4, xfer->data_left);
while (nr_words--)
pio_reg_write(XFER_DATA_PORT, *p++);
}
[Severity: High]
This is a pre-existing issue, but does writing to the TX stream FIFO using
pio_reg_write() also byteswap the data incorrectly on big-endian
architectures, and does it similarly suffer from unaligned memory access
panics?
Further down in hci_pio_do_tx(), trailing bytes are handled:
if (xfer->data_left) {
/*
* There are trailing bytes to send. We can simply load
* them from memory as a word which will keep those bytes
* in their proper place even on a BE system. This will
* also get some bytes past the actual buffer but no one
* should care as they won't be sent out.
*/
if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD))
return false;
dev_dbg(&hci->master.dev, "trailing %d", xfer->data_left);
pio_reg_write(XFER_DATA_PORT, *p);
xfer->data_left = 0;
}
[Severity: High]
This isn't a bug introduced by this patch, but does the 32-bit dereference
of *p for the trailing bytes read past the end of the allocated buffer?
If an I3C transfer occurs where the provided payload buffer length leaves
1 to 3 trailing bytes and unprivileged userspace passes an unpadded buffer
via an ioctl, reading past the allocation could trigger KASAN splats or
cause a fatal page fault if the buffer ends exactly at a page boundary or
a SLUB debugging redzone.
Finally, in hci_pio_get_ibi_segment():
drivers/i3c/master/mipi-i3c-hci/pio.c:hci_pio_get_ibi_segment():
/* extract the data from the IBI port */
nr_words = thresh_val;
ibi->seg_cnt -= nr_words * 4;
dev_dbg(&hci->master.dev, "now %d left %d",
nr_words * 4, ibi->seg_cnt);
while (nr_words--)
*p++ = pio_reg_read(IBI_PORT);
[Severity: High]
This is a pre-existing issue, but does reading from the IBI FIFO stream
using pio_reg_read() incorrectly byteswap the data payload on big-endian
platforms?
Does this need to be updated to use readsl() or similar stream accessors?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-b4-i3c-hci-ast2700-v1-0-19909e7cbd7e@aspeedtech.com?part=4
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 4/8] i3c: mipi-i3c-hci: Add a quirk to clear the TX start threshold
2026-09-01 11:35 ` [PATCH 4/8] i3c: mipi-i3c-hci: Add a quirk to clear the TX start threshold Billy Tsai
2026-09-01 11:49 ` sashiko-bot
@ 2026-09-01 20:58 ` Frank Li
1 sibling, 0 replies; 19+ messages in thread
From: Frank Li @ 2026-09-01 20:58 UTC (permalink / raw)
To: Billy Tsai
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel,
linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel
On Tue, Sep 01, 2026 at 07:35:31PM +0800, Billy Tsai wrote:
> The DATA_TX_START_THLD field of the PIO data buffer threshold register
> holds off the start of a TX transfer until the FIFO holds a set amount
> of data, which in PIO mode cuts down on the number of software writes.
> The field resets to 0x1, requiring at least (2 ^ 2) DWORDs (16 bytes)
> in the FIFO before transmission starts.
>
> HCI controllers that support both PIO and DMA can expose the two as
> separate register blocks; nothing in the specification says selecting
> DMA mode disables the PIO block's own gating logic. On ASPEED
> platforms it doesn't: DATA_TX_START_THLD still holds up transfer
> start regardless of which mode feeds the FIFO, and in DMA mode that
> threshold is never satisfied for some transfer sizes:
>
> - 1-4 bytes: uses the Immediate Data Transfer Command.
> - 13+ bytes: since the hardware fetches data in 4-byte chunks, a
> 13-byte transfer fetches 16 bytes into the FIFO and reaches the
> threshold.
> - 5-12 bytes: the threshold is never reached and the transfer stalls.
>
> Add HCI_QUIRK_TX_START_THLD to clear the field whenever DMA mode is
> selected. The clear lives in i3c_hci_set_io_mode() rather than in
> probe so the reset-and-restore recovery path also reapplies it after
> a controller soft reset. Move the PIO Access Area register and
> bitfield definitions from pio.c to a new pio.h so this quirk can reuse
> PIO_DATA_BUFFER_THLD_CTRL and DATA_TX_START_THLD instead of
> redefining them.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Assisted-by: Claude:claude-fable-5
> ---
> drivers/i3c/master/mipi-i3c-hci/core.c | 14 +++++
> drivers/i3c/master/mipi-i3c-hci/hci.h | 1 +
> drivers/i3c/master/mipi-i3c-hci/pio.c | 97 +----------------------------
> drivers/i3c/master/mipi-i3c-hci/pio.h | 109 +++++++++++++++++++++++++++++++++
> 4 files changed, 125 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index c03c3a9cbe4f..2290a889701c 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -26,6 +26,7 @@
> #include "cmd.h"
> #include "dat.h"
> #include "ibi.h"
> +#include "pio.h"
>
> /*
> * Host Controller Capabilities and Operation Registers
> @@ -823,6 +824,19 @@ static int i3c_hci_set_io_mode(struct i3c_hci *hci, bool dma)
> else
> reg_set(HC_CONTROL, HC_CONTROL_PIO_MODE);
>
> + /*
> + * On the ASPEED AST2700 the TX start threshold gates transfer start
> + * even in DMA mode. Clear it so DMA transfers are not held back
> + * waiting for a PIO FIFO level that will never be reached.
> + */
> + if (dma && (hci->quirks & HCI_QUIRK_TX_START_THLD) && hci->PIO_regs) {
> + void __iomem *thld_reg = hci->PIO_regs + PIO_DATA_BUFFER_THLD_CTRL;
> + u32 thld_val = readl(thld_reg);
> +
> + thld_val &= ~DATA_TX_START_THLD;
> + writel(thld_val, thld_reg);
don't move these defination, original isolate is quite good. Add API or
callback to implement clean DATA_TX_START_THLD.
Frank
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/8] i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller
2026-09-01 11:35 [PATCH 0/8] i3c: mipi-i3c-hci: Add Aspeed AST2700 support Billy Tsai
` (3 preceding siblings ...)
2026-09-01 11:35 ` [PATCH 4/8] i3c: mipi-i3c-hci: Add a quirk to clear the TX start threshold Billy Tsai
@ 2026-09-01 11:35 ` Billy Tsai
2026-09-01 11:52 ` sashiko-bot
2026-09-01 21:18 ` Frank Li
2026-09-01 11:35 ` [PATCH 6/8] i3c: mipi-i3c-hci: Program AST2700 IBI termination threshold Billy Tsai
` (2 subsequent siblings)
7 siblings, 2 replies; 19+ messages in thread
From: Billy Tsai @ 2026-09-01 11:35 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
Cc: linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, Billy Tsai
The AST2700 I3C controller carries an ASPEED vendor extended
capability describing an in-house control block and a PHY programming
window. Recognize the ASPEED MIPI vendor ID in the extended capability
parser and cache both register bases via the generic vendor_data
pointer, which other vendors (e.g. NXP) also populate; add is_aspeed()
to identify ASPEED specifically.
Bringing the controller up for transfers needs more than the generic
HCI reset sequence: the vendor block has to be switched to master
mode, PHY timing registers programmed from the selected bus rates
(with aspeed,* device tree properties to override values derived under
nominal bus loading), and all interrupts funneled through a vendor
summary register whose handler dispatches to the same core and IO
handlers as the generic path. Master clock stall is enabled alongside
master-mode init so an underrun pauses and resumes the transfer
instead of aborting it. Hook this initialization into bus setup and
resume behind is_aspeed(), and acquire the core clock and reset-names
resources the binding describes for it.
During normal operation, the vendor DAA index registers must be told
which DAT slot is being assigned during ENTDAA, and the PIO/IBI FIFOs
need resetting after a DMA error or abort — which the core already
implements behind HCI_QUIRK_DMA_ABORT_REQUIRES_PIO_RESET, so set that
quirk rather than open-coding a separate recovery path.
With the required support in place, make the "aspeed,ast2700-i3c-hci"
compatible matchable with the DAT_INDEX_IS_ADDR, DMA_64BIT,
DMA_ABORT_REQUIRES_PIO_RESET and TX_START_THLD quirks, and set
is_aspeed() from the same compatible.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Assisted-by: Claude:claude-fable-5
---
drivers/i3c/master/mipi-i3c-hci/Makefile | 2 +-
drivers/i3c/master/mipi-i3c-hci/cmd_v1.c | 21 +++
drivers/i3c/master/mipi-i3c-hci/core.c | 175 ++++++++++++++++--
drivers/i3c/master/mipi-i3c-hci/ext_caps.c | 15 ++
drivers/i3c/master/mipi-i3c-hci/ext_caps.h | 1 +
drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c | 234 ++++++++++++++++++++++++
drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h | 174 ++++++++++++++++++
7 files changed, 606 insertions(+), 16 deletions(-)
diff --git a/drivers/i3c/master/mipi-i3c-hci/Makefile b/drivers/i3c/master/mipi-i3c-hci/Makefile
index e3d3ef757035f..f4f048786c95b 100644
--- a/drivers/i3c/master/mipi-i3c-hci/Makefile
+++ b/drivers/i3c/master/mipi-i3c-hci/Makefile
@@ -4,5 +4,5 @@ obj-$(CONFIG_MIPI_I3C_HCI) += mipi-i3c-hci.o
mipi-i3c-hci-y := core.o ext_caps.o pio.o dma.o \
cmd_v1.o cmd_v2.o \
dat_v1.o dct_v1.o \
- hci_quirks.o
+ hci_quirks.o vendor_aspeed.o
obj-$(CONFIG_MIPI_I3C_HCI_PCI) += mipi-i3c-hci-pci.o
diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
index 9a11affb14bf6..bf097d55f18cf 100644
--- a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
+++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
@@ -14,6 +14,7 @@
#include "cmd.h"
#include "dat.h"
#include "dct.h"
+#include "vendor_aspeed.h"
/*
* Address Assignment Command
@@ -293,6 +294,24 @@ static void hci_cmd_v1_prep_i2c_xfer(struct i3c_hci *hci,
}
}
+/*
+ * The DAA index registers name the DAT slot being assigned during ENTDAA;
+ * they live in the vendor capability block discovered at probe time.
+ */
+static void aspeed_i3c_set_daa_index(struct i3c_hci *hci, unsigned int dat_idx)
+{
+ void __iomem *inhouse_regs = to_aspeed_vendor_data(hci)->inhouse_regs;
+
+ if (dat_idx < 32)
+ writel(BIT(dat_idx), inhouse_regs + ASPEED_I3C_DAA_INDEX0);
+ else if (dat_idx < 64)
+ writel(BIT(dat_idx - 32), inhouse_regs + ASPEED_I3C_DAA_INDEX1);
+ else if (dat_idx < 96)
+ writel(BIT(dat_idx - 64), inhouse_regs + ASPEED_I3C_DAA_INDEX2);
+ else
+ writel(BIT(dat_idx - 96), inhouse_regs + ASPEED_I3C_DAA_INDEX3);
+}
+
static int hci_cmd_v1_daa(struct i3c_hci *hci)
{
struct hci_xfer *xfer;
@@ -322,6 +341,8 @@ static int hci_cmd_v1_daa(struct i3c_hci *hci)
if (ret < 0)
break;
dat_idx = ret;
+ if (is_aspeed(hci))
+ aspeed_i3c_set_daa_index(hci, dat_idx);
dev_dbg(&hci->master.dev,
"next_addr = 0x%02x, DAA using DAT %d",
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 2290a889701cd..e0ee8148aa2f8 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -20,6 +20,7 @@
#include <linux/platform_data/mipi-i3c-hci.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
+#include <linux/reset.h>
#include "hci.h"
#include "ext_caps.h"
@@ -27,6 +28,7 @@
#include "dat.h"
#include "ibi.h"
#include "pio.h"
+#include "vendor_aspeed.h"
/*
* Host Controller Capabilities and Operation Registers
@@ -136,6 +138,25 @@ struct device *i3c_hci_sysdev(struct device *dev)
return dev->parent && dev_is_pci(dev->parent) ? dev->parent : dev;
}
+static int aspeed_i3c_bus_setup(struct i3c_hci *hci)
+{
+ if (!to_aspeed_vendor_data(hci)->phy_regs)
+ return -ENODEV;
+
+ /*
+ * Enable master clock stall: when the controller cannot keep the
+ * data pipeline fed it holds SCL low instead of underrunning, so a
+ * transfer pauses and resumes cleanly rather than aborting.
+ */
+ aspeed_i3c_write(hci, ASPEED_I3C_CTRL,
+ ASPEED_I3C_CTRL_CLOCK_STALL_EN |
+ ASPEED_I3C_CTRL_INIT |
+ FIELD_PREP(ASPEED_I3C_CTRL_INIT_MODE,
+ ASPEED_I3C_INIT_MST_MODE));
+
+ return aspeed_i3c_phy_init(hci);
+}
+
static void i3c_hci_set_master_dyn_addr(struct i3c_hci *hci)
{
reg_write(MASTER_DEVICE_ADDR,
@@ -149,6 +170,12 @@ static int i3c_hci_bus_init(struct i3c_master_controller *m)
struct i3c_device_info info;
int ret;
+ if (is_aspeed(hci)) {
+ ret = aspeed_i3c_bus_setup(hci);
+ if (ret)
+ return ret;
+ }
+
if (hci->cmd == &mipi_i3c_hci_cmd_v1) {
ret = mipi_i3c_hci_dat_v1.init(hci);
if (ret)
@@ -766,22 +793,12 @@ static const struct i3c_master_controller_ops i3c_hci_ops = {
.disable_hotjoin = i3c_hci_disable_hotjoin,
};
-static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
+static irqreturn_t i3c_hci_core_irq(struct i3c_hci *hci)
+__must_hold(&hci->lock)
{
- struct i3c_hci *hci = dev_id;
irqreturn_t result = IRQ_NONE;
u32 val;
- guard(spinlock)(&hci->lock);
-
- /*
- * The IRQ can be shared, so the handler may be called when the IRQ is
- * due to a different device. That could happen when runtime suspended,
- * so exit immediately if IRQs are not expected for this device.
- */
- if (hci->irq_inactive)
- return IRQ_NONE;
-
val = reg_read(INTR_STATUS);
reg_write(INTR_STATUS, val);
dev_dbg(&hci->master.dev, "INTR_STATUS %#x", val);
@@ -804,12 +821,73 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
dev_warn_once(&hci->master.dev,
"unexpected INTR_STATUS %#x\n", val);
+ return result;
+}
+
+static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
+{
+ irqreturn_t result = IRQ_NONE;
+ struct i3c_hci *hci = dev_id;
+
+ guard(spinlock)(&hci->lock);
+
+ /*
+ * The IRQ can be shared, so the handler may be called when the IRQ is
+ * due to a different device. That could happen when runtime suspended,
+ * so exit immediately if IRQs are not expected for this device.
+ */
+ if (hci->irq_inactive)
+ return IRQ_NONE;
+
+ result = i3c_hci_core_irq(hci);
+
if (hci->io->irq_handler(hci))
result = IRQ_HANDLED;
return result;
}
+static irqreturn_t i3c_aspeed_irq_handler(int irq, void *dev_id)
+{
+ struct i3c_hci *hci = dev_id;
+ u32 status, inhouse_status;
+
+ guard(spinlock)(&hci->lock);
+
+ if (hci->irq_inactive)
+ return IRQ_NONE;
+
+ /*
+ * The vendor summary register ORs together the core HCI status
+ * (CAP), the PIO/DMA ring status (PIO/RHS) and the in-house block's
+ * own status (INHOUSE) into single bits, so each source is
+ * dispatched to the handler that knows how to service it.
+ */
+ status = aspeed_i3c_read(hci, ASPEED_I3C_INTR_SUM_STATUS);
+ if (!status)
+ return IRQ_NONE;
+
+ if (status & ASPEED_INTR_SUM_CAP)
+ i3c_hci_core_irq(hci);
+ if (status & (ASPEED_INTR_SUM_PIO | ASPEED_INTR_SUM_RHS))
+ hci->io->irq_handler(hci);
+ if (status & ASPEED_INTR_SUM_INHOUSE) {
+ /*
+ * The in-house block has no dedicated handler; just
+ * acknowledge its status bits so the summary IRQ clears.
+ */
+ inhouse_status = aspeed_i3c_read(hci, ASPEED_I3C_INTR_STATUS);
+ aspeed_i3c_write(hci, ASPEED_I3C_INTR_STATUS, inhouse_status);
+ }
+
+ /*
+ * Unlike i3c_hci_irq_handler(), this IRQ is not shared
+ * (IRQF_SHARED is not passed at request_irq() time), so a nonzero
+ * summary status is always ours to handle.
+ */
+ return IRQ_HANDLED;
+}
+
static inline bool is_version_1_1_or_newer(struct i3c_hci *hci)
{
return hci->version_major > 1 || (hci->version_major == 1 && hci->version_minor > 0);
@@ -866,6 +944,11 @@ static int i3c_hci_reset_and_init(struct i3c_hci *hci)
* Bit 0:5 are defined in IP version < 0.8 but not handled by PIO code
*/
reg_write(INTR_STATUS_ENABLE, GENMASK(31, 10));
+ if (is_aspeed(hci)) {
+ aspeed_i3c_write(hci, ASPEED_I3C_INTR_SIGNAL_ENABLE, 0);
+ aspeed_i3c_write(hci, ASPEED_I3C_INTR_STATUS_ENABLE,
+ GENMASK(31, 0));
+ }
/* Make sure our data ordering fits the host's */
regval = reg_read(HC_CONTROL);
@@ -949,6 +1032,12 @@ static int i3c_hci_do_reset_and_restore(struct i3c_hci *hci)
if (ret)
return -EIO;
+ if (is_aspeed(hci)) {
+ ret = aspeed_i3c_bus_setup(hci);
+ if (ret)
+ return ret;
+ }
+
i3c_hci_set_master_dyn_addr(hci);
mipi_i3c_hci_dat_v1.restore(hci);
@@ -1146,10 +1235,17 @@ static int i3c_hci_init(struct i3c_hci *hci)
return i3c_hci_reset_and_init(hci);
}
+static void i3c_hci_dma_rst_assert(void *data)
+{
+ reset_control_assert(data);
+}
+
static int i3c_hci_probe(struct platform_device *pdev)
{
const struct mipi_i3c_hci_platform_data *pdata = pdev->dev.platform_data;
struct clk_bulk_data *clks;
+ irq_handler_t irq_handler;
+ unsigned long irq_flags;
struct i3c_hci *hci;
int irq, ret;
@@ -1176,8 +1272,12 @@ static int i3c_hci_probe(struct platform_device *pdev)
}
platform_set_drvdata(pdev, hci);
- /* temporary for dev_printk's, to be replaced in i3c_master_register */
+ /*
+ * Temporary for dev_printk's and is_aspeed(), both replaced with
+ * the real values by i3c_master_register().
+ */
hci->master.dev.init_name = dev_name(&pdev->dev);
+ hci->master.dev.of_node = pdev->dev.of_node;
hci->quirks = (unsigned long)device_get_match_data(&pdev->dev);
if (!hci->quirks && platform_get_device_id(pdev))
@@ -1189,6 +1289,40 @@ static int i3c_hci_probe(struct platform_device *pdev)
"cannot set DMA mask\n");
}
+ if (is_aspeed(hci)) {
+ struct aspeed_i3c_vendor_data *vd;
+
+ vd = devm_kzalloc(&pdev->dev, sizeof(*vd), GFP_KERNEL);
+ if (!vd)
+ return -ENOMEM;
+ hci->vendor_data = vd;
+
+ vd->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, "core");
+ if (IS_ERR(vd->rst))
+ return PTR_ERR(vd->rst);
+ ret = reset_control_deassert(vd->rst);
+ if (ret)
+ return ret;
+
+ vd->dma_rst = devm_reset_control_get_optional_shared(&pdev->dev, "dma");
+ if (IS_ERR(vd->dma_rst))
+ return PTR_ERR(vd->dma_rst);
+ if (vd->dma_rst) {
+ ret = reset_control_deassert(vd->dma_rst);
+ if (ret)
+ return ret;
+ ret = devm_add_action_or_reset(&pdev->dev,
+ i3c_hci_dma_rst_assert,
+ vd->dma_rst);
+ if (ret)
+ return ret;
+ }
+
+ vd->clk = devm_clk_get_optional(&pdev->dev, NULL);
+ if (IS_ERR(vd->clk))
+ return PTR_ERR(vd->clk);
+ }
+
ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &clks);
if (ret < 0)
return dev_err_probe(&pdev->dev, ret,
@@ -1200,9 +1334,15 @@ static int i3c_hci_probe(struct platform_device *pdev)
hci->irq_inactive = true;
+ irq_handler = i3c_hci_irq_handler;
+ irq_flags = IRQF_SHARED;
+ if (is_aspeed(hci)) {
+ irq_handler = i3c_aspeed_irq_handler;
+ irq_flags = 0;
+ }
+
irq = platform_get_irq(pdev, 0);
- ret = devm_request_irq(&pdev->dev, irq, i3c_hci_irq_handler,
- IRQF_SHARED, NULL, hci);
+ ret = devm_request_irq(&pdev->dev, irq, irq_handler, irq_flags, NULL, hci);
if (ret)
return ret;
@@ -1230,6 +1370,11 @@ static const __maybe_unused struct of_device_id i3c_hci_of_match[] = {
{ .compatible = "microchip,sama7d65-i3c-hci",
.data = (void *)(ulong)(HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING |
HCI_QUIRK_RESP_BUF_THLD) },
+ { .compatible = "aspeed,ast2700-i3c-hci",
+ .data = (void *)(ulong)(HCI_QUIRK_DAT_INDEX_IS_ADDR |
+ HCI_QUIRK_DMA_64BIT |
+ HCI_QUIRK_DMA_ABORT_REQUIRES_PIO_RESET |
+ HCI_QUIRK_TX_START_THLD) },
{},
};
MODULE_DEVICE_TABLE(of, i3c_hci_of_match);
diff --git a/drivers/i3c/master/mipi-i3c-hci/ext_caps.c b/drivers/i3c/master/mipi-i3c-hci/ext_caps.c
index 77840fd4aa51f..b97306d498715 100644
--- a/drivers/i3c/master/mipi-i3c-hci/ext_caps.c
+++ b/drivers/i3c/master/mipi-i3c-hci/ext_caps.c
@@ -14,6 +14,7 @@
#include "hci.h"
#include "ext_caps.h"
+#include "vendor_aspeed.h"
#include "xfer_mode_rate.h"
/* Extended Capability Header */
@@ -207,6 +208,19 @@ static int hci_extcap_vendor_NXP(struct i3c_hci *hci, void __iomem *base)
return 0;
}
+static int hci_extcap_vendor_ASPEED(struct i3c_hci *hci, void __iomem *base)
+{
+ struct aspeed_i3c_vendor_data *vd = to_aspeed_vendor_data(hci);
+
+ if (!vd)
+ return 0;
+
+ vd->inhouse_regs = hci->base_regs + readl(base + 0x04);
+ vd->phy_regs = hci->base_regs + readl(base + 0x08);
+
+ return 0;
+}
+
struct hci_ext_cap_vendor_specific {
u32 vendor;
u8 cap;
@@ -221,6 +235,7 @@ struct hci_ext_cap_vendor_specific {
static const struct hci_ext_cap_vendor_specific vendor_ext_caps[] = {
EXT_CAP_VENDOR(NXP, 0xc0, 0x20),
+ EXT_CAP_VENDOR(ASPEED, 0xc0, 0x08),
};
static int hci_extcap_vendor_specific(struct i3c_hci *hci, void __iomem *base,
diff --git a/drivers/i3c/master/mipi-i3c-hci/ext_caps.h b/drivers/i3c/master/mipi-i3c-hci/ext_caps.h
index b15e629951f03..46dd83c0d4c54 100644
--- a/drivers/i3c/master/mipi-i3c-hci/ext_caps.h
+++ b/drivers/i3c/master/mipi-i3c-hci/ext_caps.h
@@ -12,6 +12,7 @@
/* MIPI vendor IDs */
#define MIPI_VENDOR_NXP 0x11b
+#define MIPI_VENDOR_ASPEED 0x3f6
int i3c_hci_parse_ext_caps(struct i3c_hci *hci);
diff --git a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
new file mode 100644
index 0000000000000..b521fce4c084a
--- /dev/null
+++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
@@ -0,0 +1,234 @@
+// SPDX-License-Identifier: BSD-3-Clause
+/*
+ * Copyright (c) 2026 ASPEED Technology Inc.
+ *
+ * AST2700 vendor register access and PHY timing initialization.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/i3c/master.h>
+#include <linux/io.h>
+#include <linux/of.h>
+
+#include "hci.h"
+#include "vendor_aspeed.h"
+
+u32 aspeed_i3c_read(struct i3c_hci *hci, u32 reg)
+{
+ return readl(to_aspeed_vendor_data(hci)->inhouse_regs + reg);
+}
+
+void aspeed_i3c_write(struct i3c_hci *hci, u32 reg, u32 val)
+{
+ writel(val, to_aspeed_vendor_data(hci)->inhouse_regs + reg);
+}
+
+static void aspeed_i3c_phy_write(struct i3c_hci *hci, u32 reg, u32 val)
+{
+ writel(val, to_aspeed_vendor_data(hci)->phy_regs + reg);
+}
+
+static u16 aspeed_i3c_ns_to_cnt(unsigned int ns, unsigned long period_ns)
+{
+ unsigned long cycles = DIV_ROUND_CLOSEST(ns, period_ns);
+
+ /* All PHY counter fields are 11 bits wide */
+ return min_t(unsigned long, max_t(unsigned long, cycles, 1) - 1,
+ ASPEED_I3C_PHY_CNT_MAX);
+}
+
+static u32 aspeed_i3c_get_sdr_ctrl0_reg(struct i3c_hci *hci)
+{
+ struct i3c_bus *bus = i3c_master_get_bus(&hci->master);
+
+ if (bus->scl_rate.i3c > 8000000)
+ return ASPEED_I3C_PHY_I3C_SDR0_CTRL0;
+ if (bus->scl_rate.i3c > 6000000)
+ return ASPEED_I3C_PHY_I3C_SDR1_CTRL0;
+ if (bus->scl_rate.i3c > 4000000)
+ return ASPEED_I3C_PHY_I3C_SDR2_CTRL0;
+ if (bus->scl_rate.i3c > 2000000)
+ return ASPEED_I3C_PHY_I3C_SDR3_CTRL0;
+
+ return ASPEED_I3C_PHY_I3C_SDR4_CTRL0;
+}
+
+int aspeed_i3c_phy_init(struct i3c_hci *hci)
+{
+ u16 hcnt, lcnt, total_cnt, min_tbit_cnt, cas_lcnt, cas_cnt, cbp_cnt;
+ u32 sda_tx_hold = 0, cas_ns = 0, cbp_ns = 0, sr_p_low = 0;
+ struct i3c_bus *bus = i3c_master_get_bus(&hci->master);
+ u32 pp_high = 0, pp_low = 0, od_high = 0, od_low = 0;
+ struct device_node *np = hci->master.dev.of_node;
+ u32 sdr_ctrl0_reg, ctrl0, ctrl1, ctrl2;
+ unsigned long core_rate, period_ns;
+
+ core_rate = clk_get_rate(to_aspeed_vendor_data(hci)->clk);
+ if (!core_rate) {
+ dev_err(&hci->master.dev, "invalid core clock rate\n");
+ return -EINVAL;
+ }
+
+ period_ns = DIV_ROUND_UP(NSEC_PER_SEC, core_rate);
+
+ hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FM_CAS_NS, period_ns);
+ lcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FM_SU_STO_NS, period_ns);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FM_CTRL0,
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL0_CAS, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL0_SU_STO, lcnt));
+
+ hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FM_SCL_H_NS, period_ns);
+ lcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FM_SCL_L_NS, period_ns);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FM_CTRL1,
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL1_SCL_H, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL1_SCL_L, lcnt));
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FM_CTRL2,
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL2_ACK_H, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL2_ACK_L, hcnt));
+
+ hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FM_HD_DAT_NS, period_ns);
+ lcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FM_AHD_DAT_NS, period_ns);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FM_CTRL3,
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL3_HD_DAT, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FM_CTRL3_AHD_DAT, lcnt));
+
+ hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FMP_CAS_NS, period_ns);
+ lcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FMP_SU_STO_NS, period_ns);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FMP_CTRL0,
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL0_CAS, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL0_SU_STO, lcnt));
+
+ hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FMP_SCL_H_NS, period_ns);
+ lcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FMP_SCL_L_NS, period_ns);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FMP_CTRL1,
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL1_SCL_H, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL1_SCL_L, lcnt));
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FMP_CTRL2,
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL2_ACK_H, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL2_ACK_L, hcnt));
+
+ hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FMP_HD_DAT_NS, period_ns);
+ lcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FMP_AHD_DAT_NS, period_ns);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I2C_FMP_CTRL3,
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL3_HD_DAT, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I2C_FMP_CTRL3_AHD_DAT, lcnt));
+
+ of_property_read_u32(np, "aspeed,pp-scl-high-ns", &pp_high);
+ of_property_read_u32(np, "aspeed,pp-scl-low-ns", &pp_low);
+ if (pp_high && pp_low) {
+ hcnt = aspeed_i3c_ns_to_cnt(pp_high, period_ns);
+ lcnt = aspeed_i3c_ns_to_cnt(pp_low, period_ns);
+ } else {
+ /*
+ * Each of hcnt/lcnt is programmed as N-1 (the PHY counts N
+ * cycles for a stored value of N-1), so pre-subtract 2 here
+ * to cancel the two implicit +1s and match the requested SCL
+ * period exactly. Clamp the pre-subtraction value so
+ * total_cnt cannot underflow when an unrealistically high
+ * scl_rate leaves less than 3 cycles per period.
+ */
+ unsigned long cycles_per_scl = DIV_ROUND_UP(core_rate, bus->scl_rate.i3c);
+
+ total_cnt = max_t(unsigned long, cycles_per_scl, 3) - 2;
+ if (hci->master.bus.mode == I3C_BUS_MODE_PURE) {
+ hcnt = DIV_ROUND_DOWN_ULL(total_cnt * 2, 5);
+ lcnt = total_cnt - hcnt;
+ } else {
+ hcnt = DIV_ROUND_UP(I3C_BUS_THIGH_MIXED_MAX_NS,
+ period_ns) - 1;
+ lcnt = total_cnt - hcnt;
+ }
+ }
+
+ ctrl0 = FIELD_PREP(ASPEED_I3C_PHY_I3C_CTRL0_SCL_H, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_CTRL0_SCL_L, lcnt);
+ sdr_ctrl0_reg = aspeed_i3c_get_sdr_ctrl0_reg(hci);
+ aspeed_i3c_phy_write(hci, sdr_ctrl0_reg, ctrl0);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I3C_SDR0_CTRL0, ctrl0);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I3C_DDR_CTRL0, ctrl0);
+
+ min_tbit_cnt = DIV_ROUND_UP(60, period_ns) - 1;
+ ctrl1 = FIELD_PREP(ASPEED_I3C_PHY_I3C_CTRL1_TBIT_H,
+ max_t(u16, hcnt, min_tbit_cnt)) |
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_CTRL1_TBIT_L,
+ max_t(u16, lcnt, min_tbit_cnt));
+ aspeed_i3c_phy_write(hci, sdr_ctrl0_reg + ASPEED_I3C_PHY_CTRL1_OFFSET,
+ ctrl1);
+ aspeed_i3c_phy_write(hci,
+ ASPEED_I3C_PHY_I3C_SDR0_CTRL0 +
+ ASPEED_I3C_PHY_CTRL1_OFFSET, ctrl1);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I3C_DDR_CTRL1, ctrl1);
+
+ /*
+ * tCAS and tCBP default to a value derived from the SCL low period
+ * of the applicable bus context, clamped to the MIPI I3C minima
+ * (tCAS >= 38.4 ns, tCBP >= 19.2 ns). Explicit device tree values
+ * are used as-is; the binding carries the minima as constraints.
+ */
+ if (hci->master.bus.mode == I3C_BUS_MODE_PURE)
+ cas_lcnt = lcnt;
+ else
+ cas_lcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_I2C_FM_SCL_L_NS,
+ period_ns);
+ of_property_read_u32(np, "aspeed,cas-ns", &cas_ns);
+ of_property_read_u32(np, "aspeed,cbp-ns", &cbp_ns);
+ cas_cnt = cas_ns ? aspeed_i3c_ns_to_cnt(cas_ns, period_ns)
+ : max_t(u16, cas_lcnt,
+ DIV_ROUND_UP(ASPEED_I3C_PHY_OD_MIN_CAS_NS_X10,
+ period_ns * 10) - 1);
+ cbp_cnt = cbp_ns ? aspeed_i3c_ns_to_cnt(cbp_ns, period_ns)
+ : max_t(u16, cas_lcnt,
+ DIV_ROUND_UP(ASPEED_I3C_PHY_OD_MIN_CBP_NS_X10,
+ period_ns * 10) - 1);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I3C_OD_CTRL0,
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_OD_CTRL0_CAS, cas_cnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_OD_CTRL0_CBP, cbp_cnt));
+
+ of_property_read_u32(np, "aspeed,od-scl-high-ns", &od_high);
+ of_property_read_u32(np, "aspeed,od-scl-low-ns", &od_low);
+ hcnt = aspeed_i3c_ns_to_cnt(od_high ?: ASPEED_I3C_PHY_OD_SCL_H_NS,
+ period_ns);
+ lcnt = aspeed_i3c_ns_to_cnt(od_low ?: ASPEED_I3C_PHY_OD_SCL_L_NS,
+ period_ns);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I3C_OD_CTRL1,
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_OD_CTRL1_SCL_H, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_OD_CTRL1_SCL_L, lcnt));
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I3C_OD_CTRL2,
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_OD_CTRL2_ACK_H, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_OD_CTRL2_ACK_L, lcnt));
+
+ of_property_read_u32(np, "aspeed,sda-tx-hold-ns", &sda_tx_hold);
+ hcnt = aspeed_i3c_ns_to_cnt(sda_tx_hold ?: ASPEED_I3C_PHY_OD_HD_DAT_NS,
+ period_ns);
+ lcnt = aspeed_i3c_ns_to_cnt(sda_tx_hold ?: ASPEED_I3C_PHY_OD_AHD_DAT_NS,
+ period_ns);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I3C_OD_CTRL3,
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_OD_CTRL3_HD_DAT, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_OD_CTRL3_AHD_DAT, lcnt));
+ ctrl2 = FIELD_PREP(ASPEED_I3C_PHY_I3C_CTRL2_HD_PP, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_CTRL2_TBIT_HD_PP, lcnt);
+ aspeed_i3c_phy_write(hci, sdr_ctrl0_reg + ASPEED_I3C_PHY_CTRL2_OFFSET,
+ ctrl2);
+ aspeed_i3c_phy_write(hci,
+ ASPEED_I3C_PHY_I3C_SDR0_CTRL0 +
+ ASPEED_I3C_PHY_CTRL2_OFFSET, ctrl2);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I3C_DDR_CTRL2, ctrl2);
+
+ of_property_read_u32(np, "aspeed,sr-p-scl-low-ns", &sr_p_low);
+ hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_SR_P_HD_NS, period_ns);
+ lcnt = aspeed_i3c_ns_to_cnt(sr_p_low ?: ASPEED_I3C_PHY_SR_P_SCL_L_NS,
+ period_ns);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_SR_P_PREPARE_CTRL,
+ FIELD_PREP(ASPEED_I3C_PHY_SR_P_PREPARE_HD, hcnt) |
+ FIELD_PREP(ASPEED_I3C_PHY_SR_P_PREPARE_SCL_L, lcnt));
+
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_PULLUP_EN, 0);
+
+ hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_OD_DAP_NS, period_ns);
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I3C_OD_CTRL4,
+ FIELD_PREP(ASPEED_I3C_PHY_I3C_OD_CTRL4_DAP, hcnt));
+
+ return 0;
+}
diff --git a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
new file mode 100644
index 0000000000000..8760384d1c6f5
--- /dev/null
+++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
@@ -0,0 +1,174 @@
+/* SPDX-License-Identifier: BSD-3-Clause */
+/*
+ * Copyright (c) 2026 ASPEED Technology Inc.
+ *
+ * AST2700 specific MIPI I3C HCI definitions
+ */
+
+#ifndef VENDOR_ASPEED_H
+#define VENDOR_ASPEED_H
+
+#include <linux/bitfield.h>
+#include <linux/of.h>
+
+#include "ext_caps.h"
+
+struct clk;
+struct reset_control;
+
+/*
+ * The AST2700 vendor extended capability points to an in-house control
+ * block and a PHY programming window inside the controller's register
+ * space. The core clock and reset lines are only specified in the
+ * AST2700 binding as well. All of it is ASPEED-specific, so it is kept
+ * out of the generic struct i3c_hci and reached instead through its
+ * vendor_data pointer.
+ */
+struct aspeed_i3c_vendor_data {
+ void __iomem *inhouse_regs;
+ void __iomem *phy_regs;
+ struct reset_control *rst;
+ struct reset_control *dma_rst;
+ struct clk *clk;
+};
+
+/*
+ * hci->master.dev.of_node is only valid once i3c_master_register() has
+ * run device_set_node() on it; probe() pre-populates it before that
+ * point (see i3c_hci_probe()) so this works from early init onward too.
+ */
+static inline bool is_aspeed(struct i3c_hci *hci)
+{
+ return of_device_is_compatible(hci->master.dev.of_node,
+ "aspeed,ast2700-i3c-hci");
+}
+
+static inline struct aspeed_i3c_vendor_data *to_aspeed_vendor_data(struct i3c_hci *hci)
+{
+ return hci->vendor_data;
+}
+
+u32 aspeed_i3c_read(struct i3c_hci *hci, u32 reg);
+void aspeed_i3c_write(struct i3c_hci *hci, u32 reg, u32 val);
+int aspeed_i3c_phy_init(struct i3c_hci *hci);
+
+#define ASPEED_I3C_CTRL 0x00
+#define ASPEED_I3C_CTRL_CLOCK_STALL_EN BIT(14)
+#define ASPEED_I3C_CTRL_INIT BIT(4)
+#define ASPEED_I3C_CTRL_INIT_MODE GENMASK(1, 0)
+#define ASPEED_I3C_INIT_MST_MODE 0
+
+#define ASPEED_I3C_DAA_INDEX0 0x10
+#define ASPEED_I3C_DAA_INDEX1 0x14
+#define ASPEED_I3C_DAA_INDEX2 0x18
+#define ASPEED_I3C_DAA_INDEX3 0x1c
+
+#define ASPEED_I3C_INTR_STATUS 0xe0
+#define ASPEED_I3C_INTR_STATUS_ENABLE 0xe4
+#define ASPEED_I3C_INTR_SIGNAL_ENABLE 0xe8
+#define ASPEED_I3C_INTR_SUM_STATUS 0xf0
+#define ASPEED_INTR_SUM_INHOUSE BIT(3)
+#define ASPEED_INTR_SUM_RHS BIT(2)
+#define ASPEED_INTR_SUM_PIO BIT(1)
+#define ASPEED_INTR_SUM_CAP BIT(0)
+
+#define ASPEED_I3C_PHY_I2C_FM_CTRL0 0x08
+#define ASPEED_I3C_PHY_I2C_FM_CTRL0_CAS GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I2C_FM_CTRL0_SU_STO GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I2C_FM_CTRL1 0x0c
+#define ASPEED_I3C_PHY_I2C_FM_CTRL1_SCL_H GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I2C_FM_CTRL1_SCL_L GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I2C_FM_CTRL2 0x10
+#define ASPEED_I3C_PHY_I2C_FM_CTRL2_ACK_H GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I2C_FM_CTRL2_ACK_L GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I2C_FM_CTRL3 0x14
+#define ASPEED_I3C_PHY_I2C_FM_CTRL3_HD_DAT GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I2C_FM_CTRL3_AHD_DAT GENMASK(10, 0)
+
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL0 0x18
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL0_CAS GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL0_SU_STO GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL1 0x1c
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL1_SCL_H GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL1_SCL_L GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL2 0x20
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL2_ACK_H GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL2_ACK_L GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL3 0x24
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL3_HD_DAT GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I2C_FMP_CTRL3_AHD_DAT GENMASK(10, 0)
+
+#define ASPEED_I3C_PHY_I3C_OD_CTRL0 0x28
+#define ASPEED_I3C_PHY_I3C_OD_CTRL0_CAS GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I3C_OD_CTRL0_CBP GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I3C_OD_CTRL1 0x2c
+#define ASPEED_I3C_PHY_I3C_OD_CTRL1_SCL_H GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I3C_OD_CTRL1_SCL_L GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I3C_OD_CTRL2 0x30
+#define ASPEED_I3C_PHY_I3C_OD_CTRL2_ACK_H GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I3C_OD_CTRL2_ACK_L GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I3C_OD_CTRL3 0x34
+#define ASPEED_I3C_PHY_I3C_OD_CTRL3_HD_DAT GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I3C_OD_CTRL3_AHD_DAT GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I3C_OD_CTRL4 0xd8
+#define ASPEED_I3C_PHY_I3C_OD_CTRL4_DAP GENMASK(26, 16)
+
+#define ASPEED_I3C_PHY_I3C_SDR0_CTRL0 0x38
+#define ASPEED_I3C_PHY_I3C_SDR1_CTRL0 0x44
+#define ASPEED_I3C_PHY_I3C_SDR2_CTRL0 0x50
+#define ASPEED_I3C_PHY_I3C_SDR3_CTRL0 0x5c
+#define ASPEED_I3C_PHY_I3C_SDR4_CTRL0 0x68
+#define ASPEED_I3C_PHY_I3C_CTRL0_SCL_H GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I3C_CTRL0_SCL_L GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I3C_CTRL1_TBIT_H GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I3C_CTRL1_TBIT_L GENMASK(10, 0)
+#define ASPEED_I3C_PHY_I3C_CTRL2_HD_PP GENMASK(26, 16)
+#define ASPEED_I3C_PHY_I3C_CTRL2_TBIT_HD_PP GENMASK(10, 0)
+
+#define ASPEED_I3C_PHY_I3C_DDR_CTRL0 0x74
+#define ASPEED_I3C_PHY_I3C_DDR_CTRL1 0x78
+#define ASPEED_I3C_PHY_I3C_DDR_CTRL2 0x7c
+
+#define ASPEED_I3C_PHY_SR_P_PREPARE_CTRL 0x80
+#define ASPEED_I3C_PHY_SR_P_PREPARE_HD GENMASK(26, 16)
+#define ASPEED_I3C_PHY_SR_P_PREPARE_SCL_L GENMASK(10, 0)
+
+#define ASPEED_I3C_PHY_PULLUP_EN 0x98
+
+#define ASPEED_I3C_PHY_CTRL1_OFFSET 0x04
+#define ASPEED_I3C_PHY_CTRL2_OFFSET 0x08
+
+#define ASPEED_I3C_PHY_CNT_MAX GENMASK(10, 0)
+
+#define ASPEED_I3C_PHY_I2C_FM_CAS_NS 1130
+#define ASPEED_I3C_PHY_I2C_FM_SU_STO_NS 1370
+#define ASPEED_I3C_PHY_I2C_FM_SCL_H_NS 1130
+#define ASPEED_I3C_PHY_I2C_FM_SCL_L_NS 1370
+#define ASPEED_I3C_PHY_I2C_FM_HD_DAT_NS 10
+#define ASPEED_I3C_PHY_I2C_FM_AHD_DAT_NS 10
+
+#define ASPEED_I3C_PHY_I2C_FMP_CAS_NS 380
+#define ASPEED_I3C_PHY_I2C_FMP_SU_STO_NS 620
+#define ASPEED_I3C_PHY_I2C_FMP_SCL_H_NS 380
+#define ASPEED_I3C_PHY_I2C_FMP_SCL_L_NS 620
+#define ASPEED_I3C_PHY_I2C_FMP_HD_DAT_NS 10
+#define ASPEED_I3C_PHY_I2C_FMP_AHD_DAT_NS 10
+
+#define ASPEED_I3C_PHY_OD_SCL_H_NS 380
+#define ASPEED_I3C_PHY_OD_SCL_L_NS 620
+#define ASPEED_I3C_PHY_OD_HD_DAT_NS 10
+#define ASPEED_I3C_PHY_OD_AHD_DAT_NS 10
+#define ASPEED_I3C_PHY_OD_DAP_NS 12
+
+#define ASPEED_I3C_PHY_SR_P_HD_NS 16
+#define ASPEED_I3C_PHY_SR_P_SCL_L_NS 40
+
+/*
+ * MIPI I3C minimum timing for the open-drain start/stop, in tenths of a
+ * nanosecond to keep the fractional spec values (tCAS >= 38.4 ns,
+ * tCBP >= 19.2 ns) in integer arithmetic.
+ */
+#define ASPEED_I3C_PHY_OD_MIN_CAS_NS_X10 384
+#define ASPEED_I3C_PHY_OD_MIN_CBP_NS_X10 192
+
+#endif
--
2.34.1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 5/8] i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller
2026-09-01 11:35 ` [PATCH 5/8] i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller Billy Tsai
@ 2026-09-01 11:52 ` sashiko-bot
2026-09-01 21:18 ` Frank Li
1 sibling, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-09-01 11:52 UTC (permalink / raw)
To: Billy Tsai
Cc: robh, devicetree, conor+dt, linux-i3c, Alexandre Belloni,
Frank.Li
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Kernel crash due to unchecked vendor register access if the hardware lacks the ASPEED extended capability.
- [High] Incomplete clearing of previous DAA index across registers leaves stale bits set.
- [Low] Contradictory use of optional clock API leads to misleading probe failures.
--
commit 3d4e54b2ee6f05bbf18ce4fc75b881c1d3330487
Author: Billy Tsai <billy_tsai@aspeedtech.com>
i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller
This commit adds support for the AST2700 I3C controller by caching vendor
data through a MIPI extended capability parser. It implements ASPEED-specific
initialization hooks, hardware configurations like DAA indexing, and custom
IRQ dispatching.
> diff --git a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> --- a/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c
[ ... ]
> @@ -293,6 +294,24 @@ static void hci_cmd_v1_prep_i2c_xfer(struct i3c_hci *hci,
> }
> }
>
> +/*
> + * The DAA index registers name the DAT slot being assigned during ENTDAA;
> + * they live in the vendor capability block discovered at probe time.
> + */
> +static void aspeed_i3c_set_daa_index(struct i3c_hci *hci, unsigned int dat_idx)
> +{
> + void __iomem *inhouse_regs = to_aspeed_vendor_data(hci)->inhouse_regs;
> +
> + if (dat_idx < 32)
> + writel(BIT(dat_idx), inhouse_regs + ASPEED_I3C_DAA_INDEX0);
> + else if (dat_idx < 64)
> + writel(BIT(dat_idx - 32), inhouse_regs + ASPEED_I3C_DAA_INDEX1);
> + else if (dat_idx < 96)
> + writel(BIT(dat_idx - 64), inhouse_regs + ASPEED_I3C_DAA_INDEX2);
> + else
> + writel(BIT(dat_idx - 96), inhouse_regs + ASPEED_I3C_DAA_INDEX3);
[Severity: High]
Does this leave stale bits set in the other register banks?
Writing to one register bank leaves the other three register banks untouched.
As devices are sequentially assigned to DAT slots during ENTDAA, if the index
crosses a 32-bit boundary (e.g., from index 31 to 32), the old bit remains set
globally.
The hardware might interpret multiple set bits as instructions to assign
multiple DAT slots simultaneously, corrupting the HCI's internal routing.
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
[ ... ]
> @@ -866,6 +944,11 @@ static int i3c_hci_reset_and_init(struct i3c_hci *hci)
> * Bit 0:5 are defined in IP version < 0.8 but not handled by PIO code
> */
> reg_write(INTR_STATUS_ENABLE, GENMASK(31, 10));
> + if (is_aspeed(hci)) {
> + aspeed_i3c_write(hci, ASPEED_I3C_INTR_SIGNAL_ENABLE, 0);
[Severity: High]
Can this cause a kernel panic due to a NULL pointer dereference or type
confusion?
The unconditional write relies solely on the DT compatible string through
is_aspeed() without verifying if the hardware capability was actually found and
parsed. If the DT indicates an ASPEED compatible device but the hardware does
not expose the ASPEED MIPI vendor extended capability (e.g. due to a mismatched
DT binding), i3c_hci_probe() allocates vd with a NULL inhouse_regs pointer.
Additionally, if the hardware is NXP, the NXP parser incorrectly overwrites
hci->vendor_data. Later, i3c_hci_reset_and_init() unconditionally executes
ASPEED-specific writes using to_aspeed_vendor_data(hci)->inhouse_regs,
causing a crash.
[ ... ]
> @@ -1189,6 +1289,40 @@ static int i3c_hci_probe(struct platform_device *pdev)
> "cannot set DMA mask\n");
> }
>
> + if (is_aspeed(hci)) {
> + struct aspeed_i3c_vendor_data *vd;
> +
> + vd = devm_kzalloc(&pdev->dev, sizeof(*vd), GFP_KERNEL);
[ ... ]
> + vd->dma_rst = devm_reset_control_get_optional_shared(&pdev->dev, "dma");
> + if (IS_ERR(vd->dma_rst))
> + return PTR_ERR(vd->dma_rst);
> + if (vd->dma_rst) {
> + ret = reset_control_deassert(vd->dma_rst);
> + if (ret)
> + return ret;
> + ret = devm_add_action_or_reset(&pdev->dev,
> + i3c_hci_dma_rst_assert,
> + vd->dma_rst);
> + if (ret)
> + return ret;
> + }
> +
> + vd->clk = devm_clk_get_optional(&pdev->dev, NULL);
[Severity: Low]
Is the core clock actually optional?
The clock is retrieved as optional here, which will return NULL if it is not
specified in the device tree (masking the -ENOENT failure). However, in
aspeed_i3c_phy_init(), it appears to be treated as a mandatory resource.
> diff --git a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
> --- a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
[ ... ]
> +void aspeed_i3c_write(struct i3c_hci *hci, u32 reg, u32 val)
> +{
> + writel(val, to_aspeed_vendor_data(hci)->inhouse_regs + reg);
> +}
[Severity: High]
Does this result in an invalid memory access or NULL pointer dereference?
If the ASPEED extended capability wasn't present in hardware (or if an NXP
device incorrectly overwrote hci->vendor_data), to_aspeed_vendor_data(hci)
might be NULL or point to invalid memory here, triggering a crash when called
unconditionally from i3c_hci_reset_and_init().
[ ... ]
> +int aspeed_i3c_phy_init(struct i3c_hci *hci)
> +{
> + u16 hcnt, lcnt, total_cnt, min_tbit_cnt, cas_lcnt, cas_cnt, cbp_cnt;
> + u32 sda_tx_hold = 0, cas_ns = 0, cbp_ns = 0, sr_p_low = 0;
> + struct i3c_bus *bus = i3c_master_get_bus(&hci->master);
> + u32 pp_high = 0, pp_low = 0, od_high = 0, od_low = 0;
> + struct device_node *np = hci->master.dev.of_node;
> + u32 sdr_ctrl0_reg, ctrl0, ctrl1, ctrl2;
> + unsigned long core_rate, period_ns;
> +
> + core_rate = clk_get_rate(to_aspeed_vendor_data(hci)->clk);
> + if (!core_rate) {
> + dev_err(&hci->master.dev, "invalid core clock rate\n");
> + return -EINVAL;
> + }
[Severity: Low]
Does this contradict the optional clock retrieval in i3c_hci_probe()?
If devm_clk_get_optional() returns NULL during probe, clk_get_rate() will
return 0 here.
The code hard-fails if the rate is 0, completely negating the optional
retrieval pattern. This hides the actual missing resource error, returning
a confusing failure message instead of properly propagating a missing DT
requirement.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-b4-i3c-hci-ast2700-v1-0-19909e7cbd7e@aspeedtech.com?part=5
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 5/8] i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller
2026-09-01 11:35 ` [PATCH 5/8] i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller Billy Tsai
2026-09-01 11:52 ` sashiko-bot
@ 2026-09-01 21:18 ` Frank Li
1 sibling, 0 replies; 19+ messages in thread
From: Frank Li @ 2026-09-01 21:18 UTC (permalink / raw)
To: Billy Tsai
Cc: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel,
linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel
On Tue, Sep 01, 2026 at 07:35:32PM +0800, Billy Tsai wrote:
> The AST2700 I3C controller carries an ASPEED vendor extended
> capability describing an in-house control block and a PHY programming
> window. Recognize the ASPEED MIPI vendor ID in the extended capability
> parser and cache both register bases via the generic vendor_data
> pointer, which other vendors (e.g. NXP) also populate; add is_aspeed()
> to identify ASPEED specifically.
>
> Bringing the controller up for transfers needs more than the generic
> HCI reset sequence: the vendor block has to be switched to master
> mode, PHY timing registers programmed from the selected bus rates
> (with aspeed,* device tree properties to override values derived under
> nominal bus loading), and all interrupts funneled through a vendor
> summary register whose handler dispatches to the same core and IO
> handlers as the generic path. Master clock stall is enabled alongside
> master-mode init so an underrun pauses and resumes the transfer
> instead of aborting it. Hook this initialization into bus setup and
> resume behind is_aspeed(), and acquire the core clock and reset-names
> resources the binding describes for it.
>
> During normal operation, the vendor DAA index registers must be told
> which DAT slot is being assigned during ENTDAA, and the PIO/IBI FIFOs
> need resetting after a DMA error or abort — which the core already
> implements behind HCI_QUIRK_DMA_ABORT_REQUIRES_PIO_RESET, so set that
> quirk rather than open-coding a separate recovery path.
>
> With the required support in place, make the "aspeed,ast2700-i3c-hci"
> compatible matchable with the DAT_INDEX_IS_ADDR, DMA_64BIT,
> DMA_ABORT_REQUIRES_PIO_RESET and TX_START_THLD quirks, and set
> is_aspeed() from the same compatible.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> Assisted-by: Claude:claude-fable-5
> ---
> drivers/i3c/master/mipi-i3c-hci/Makefile | 2 +-
> drivers/i3c/master/mipi-i3c-hci/cmd_v1.c | 21 +++
> drivers/i3c/master/mipi-i3c-hci/core.c | 175 ++++++++++++++++--
> drivers/i3c/master/mipi-i3c-hci/ext_caps.c | 15 ++
> drivers/i3c/master/mipi-i3c-hci/ext_caps.h | 1 +
> drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c | 234 ++++++++++++++++++++++++
> drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h | 174 ++++++++++++++++++
needn't "vendor"
> 7 files changed, 606 insertions(+), 16 deletions(-)
>
...
> +++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
> @@ -0,0 +1,174 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Copyright (c) 2026 ASPEED Technology Inc.
> + *
> + * AST2700 specific MIPI I3C HCI definitions
> + */
> +
> +#ifndef VENDOR_ASPEED_H
> +#define VENDOR_ASPEED_H
> +
> +#include <linux/bitfield.h>
> +#include <linux/of.h>
> +
> +#include "ext_caps.h"
> +
> +struct clk;
> +struct reset_control;
> +
> +/*
> + * The AST2700 vendor extended capability points to an in-house control
> + * block and a PHY programming window inside the controller's register
> + * space. The core clock and reset lines are only specified in the
> + * AST2700 binding as well. All of it is ASPEED-specific, so it is kept
> + * out of the generic struct i3c_hci and reached instead through its
> + * vendor_data pointer.
> + */
> +struct aspeed_i3c_vendor_data {
> + void __iomem *inhouse_regs;
> + void __iomem *phy_regs;
> + struct reset_control *rst;
> + struct reset_control *dma_rst;
> + struct clk *clk;
> +};
should be in aspeed.c
> +
> +/*
> + * hci->master.dev.of_node is only valid once i3c_master_register() has
> + * run device_set_node() on it; probe() pre-populates it before that
> + * point (see i3c_hci_probe()) so this works from early init onward too.
> + */
> +static inline bool is_aspeed(struct i3c_hci *hci)
> +{
> + return of_device_is_compatible(hci->master.dev.of_node,
> + "aspeed,ast2700-i3c-hci");
> +}
Don't suggest is_aspeed(), you split each feature and use drvdata like
previous QUIRK.
It will become complex if new chip appear, such aspeed,ast2800-i3c-hci,
Frank
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 6/8] i3c: mipi-i3c-hci: Program AST2700 IBI termination threshold
2026-09-01 11:35 [PATCH 0/8] i3c: mipi-i3c-hci: Add Aspeed AST2700 support Billy Tsai
` (4 preceding siblings ...)
2026-09-01 11:35 ` [PATCH 5/8] i3c: mipi-i3c-hci: Add support for the AST2700 I3C controller Billy Tsai
@ 2026-09-01 11:35 ` Billy Tsai
2026-09-01 11:35 ` [PATCH 7/8] i3c: mipi-i3c-hci: Improve AST2700 PIO TX queue utilization Billy Tsai
2026-09-01 11:35 ` [PATCH 8/8] i3c: mipi-i3c-hci: Support the AST2700 internal pull-ups Billy Tsai
7 siblings, 0 replies; 19+ messages in thread
From: Billy Tsai @ 2026-09-01 11:35 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
Cc: linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, Billy Tsai
For an IBI with payload, it's normally the device that decides when to
stop sending payload bytes. A misbehaving device that never terminates
can leave the controller stuck waiting on the transfer. AST2700 has an
in-house IBI payload length threshold that, once enabled, forces
termination once the received payload reaches it.
When payload IBIs are requested, grow the AST2700 termination
threshold to at least the requested payload length and enable
termination on that value. The register is controller-wide while IBI
requests for different devices can run in parallel, so the
read-modify-write is serialized under the controller lock. Controllers
without the vendor capability block keep the existing behavior.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Assisted-by: Claude:claude-fable-5
---
drivers/i3c/master/mipi-i3c-hci/core.c | 14 ++++++++++----
drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c | 20 ++++++++++++++++++++
drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h | 5 +++++
3 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index e0ee8148aa2f..3d52d8f277ad 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -690,15 +690,21 @@ static void i3c_hci_detach_i2c_dev(struct i2c_dev_desc *dev)
static int i3c_hci_request_ibi(struct i3c_dev_desc *dev,
const struct i3c_ibi_setup *req)
{
- struct i3c_master_controller *m = i3c_dev_get_master(dev);
- struct i3c_hci *hci = to_i3c_hci(m);
struct i3c_hci_dev_data *dev_data = i3c_dev_get_master_data(dev);
+ struct i3c_master_controller *m = i3c_dev_get_master(dev);
+ unsigned int ibi_max_len = req->max_payload_len;
unsigned int dat_idx = dev_data->dat_idx;
+ struct i3c_hci *hci = to_i3c_hci(m);
- if (req->max_payload_len != 0)
+ if (req->max_payload_len != 0) {
mipi_i3c_hci_dat_v1.set_flags(hci, dat_idx, DAT_0_IBI_PAYLOAD, 0);
- else
+
+ if (is_aspeed(hci))
+ aspeed_i3c_ibi_grow_threshold(hci, ibi_max_len);
+ } else {
mipi_i3c_hci_dat_v1.clear_flags(hci, dat_idx, DAT_0_IBI_PAYLOAD, 0);
+ }
+
return hci->io->request_ibi(hci, dev, req);
}
diff --git a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
index b521fce4c084..e32dea0d5fc3 100644
--- a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
+++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
@@ -25,6 +25,26 @@ void aspeed_i3c_write(struct i3c_hci *hci, u32 reg, u32 val)
writel(val, to_aspeed_vendor_data(hci)->inhouse_regs + reg);
}
+void aspeed_i3c_ibi_grow_threshold(struct i3c_hci *hci, unsigned int ibi_max_len)
+{
+ u32 mrl, cur_len;
+
+ /*
+ * The termination threshold is controller-wide while IBI requests
+ * for different devices can run in parallel, so serialize the
+ * read-modify-write.
+ */
+ guard(spinlock_irqsave)(&hci->lock);
+
+ mrl = aspeed_i3c_read(hci, ASPEED_I3C_MST_MRL);
+ cur_len = FIELD_GET(ASPEED_I3C_IBI_TERMINATE_LEN, mrl);
+
+ ibi_max_len = max(ibi_max_len, cur_len);
+ aspeed_i3c_write(hci, ASPEED_I3C_MST_MRL,
+ ASPEED_I3C_IBI_TERMINATE_EN |
+ FIELD_PREP(ASPEED_I3C_IBI_TERMINATE_LEN, ibi_max_len));
+}
+
static void aspeed_i3c_phy_write(struct i3c_hci *hci, u32 reg, u32 val)
{
writel(val, to_aspeed_vendor_data(hci)->phy_regs + reg);
diff --git a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
index 8760384d1c6f..84afb2bec956 100644
--- a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
+++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
@@ -51,6 +51,7 @@ static inline struct aspeed_i3c_vendor_data *to_aspeed_vendor_data(struct i3c_hc
u32 aspeed_i3c_read(struct i3c_hci *hci, u32 reg);
void aspeed_i3c_write(struct i3c_hci *hci, u32 reg, u32 val);
int aspeed_i3c_phy_init(struct i3c_hci *hci);
+void aspeed_i3c_ibi_grow_threshold(struct i3c_hci *hci, unsigned int ibi_max_len);
#define ASPEED_I3C_CTRL 0x00
#define ASPEED_I3C_CTRL_CLOCK_STALL_EN BIT(14)
@@ -58,6 +59,10 @@ int aspeed_i3c_phy_init(struct i3c_hci *hci);
#define ASPEED_I3C_CTRL_INIT_MODE GENMASK(1, 0)
#define ASPEED_I3C_INIT_MST_MODE 0
+#define ASPEED_I3C_MST_MRL 0x08
+#define ASPEED_I3C_IBI_TERMINATE_EN BIT(16)
+#define ASPEED_I3C_IBI_TERMINATE_LEN GENMASK(15, 0)
+
#define ASPEED_I3C_DAA_INDEX0 0x10
#define ASPEED_I3C_DAA_INDEX1 0x14
#define ASPEED_I3C_DAA_INDEX2 0x18
--
2.34.1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH 7/8] i3c: mipi-i3c-hci: Improve AST2700 PIO TX queue utilization
2026-09-01 11:35 [PATCH 0/8] i3c: mipi-i3c-hci: Add Aspeed AST2700 support Billy Tsai
` (5 preceding siblings ...)
2026-09-01 11:35 ` [PATCH 6/8] i3c: mipi-i3c-hci: Program AST2700 IBI termination threshold Billy Tsai
@ 2026-09-01 11:35 ` Billy Tsai
2026-09-01 11:51 ` sashiko-bot
2026-09-01 11:35 ` [PATCH 8/8] i3c: mipi-i3c-hci: Support the AST2700 internal pull-ups Billy Tsai
7 siblings, 1 reply; 19+ messages in thread
From: Billy Tsai @ 2026-09-01 11:35 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
Cc: linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, Billy Tsai
The PIO TX path only pushes data once STAT_TX_THLD reports that
tx_thresh_size words are free, with the threshold set to half the TX
FIFO. The FIFO is therefore never filled beyond half from a single
wait, which under-utilizes the queue, and sending buffers larger than
the threshold makes software race against the hardware draining the
FIFO, resulting in an unstable flow.
The AST2700 vendor block exposes the TX FIFO read and write pointers,
which give the exact number of free entries. When is_aspeed() is true,
size each burst from those pointers, using CIRC_SPACE() to derive the
free-entry count, so the FIFO can be used up to its full depth. Other
controllers keep the existing threshold-based flow.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Assisted-by: Claude:claude-fable-5
---
drivers/i3c/master/mipi-i3c-hci/pio.c | 25 +++++++++++++++++++++----
drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c | 11 +++++++++++
drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h | 6 ++++++
3 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
index 6bd5e43364a1..3d70257dcc1d 100644
--- a/drivers/i3c/master/mipi-i3c-hci/pio.c
+++ b/drivers/i3c/master/mipi-i3c-hci/pio.c
@@ -15,6 +15,7 @@
#include "cmd.h"
#include "ibi.h"
#include "pio.h"
+#include "vendor_aspeed.h"
/*
* PIO Access Area
@@ -295,11 +296,23 @@ static bool hci_pio_do_tx(struct i3c_hci *hci, struct hci_pio_data *pio)
p += (xfer->data_len - xfer->data_left) / 4;
while (xfer->data_left >= 4) {
- /* bail out if FIFO free space is below set threshold */
- if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD))
+ unsigned int avail = pio->tx_thresh_size;
+
+ if (is_aspeed(hci)) {
+ /*
+ * The vendor FIFO pointers give the exact free space,
+ * allowing better TX FIFO utilization than the
+ * threshold status.
+ */
+ avail = aspeed_i3c_avail_tx_entries(hci);
+ if (!avail)
+ return false;
+ } else if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD)) {
+ /* bail out if FIFO free space is below set threshold */
return false;
+ }
/* we can fill up to that TX threshold */
- nr_words = min(xfer->data_left / 4, pio->tx_thresh_size);
+ nr_words = min(xfer->data_left / 4, avail);
/* push data into the FIFO */
xfer->data_left -= nr_words * 4;
dev_dbg(&hci->master.dev, "now %d left %d",
@@ -316,8 +329,12 @@ static bool hci_pio_do_tx(struct i3c_hci *hci, struct hci_pio_data *pio)
* also get some bytes past the actual buffer but no one
* should care as they won't be sent out.
*/
- if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD))
+ if (is_aspeed(hci)) {
+ if (!aspeed_i3c_avail_tx_entries(hci))
+ return false;
+ } else if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD)) {
return false;
+ }
dev_dbg(&hci->master.dev, "trailing %d", xfer->data_left);
pio_reg_write(XFER_DATA_PORT, *p);
xfer->data_left = 0;
diff --git a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
index e32dea0d5fc3..182111b857c9 100644
--- a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
+++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
@@ -6,6 +6,7 @@
*/
#include <linux/bitfield.h>
+#include <linux/circ_buf.h>
#include <linux/clk.h>
#include <linux/device.h>
#include <linux/i3c/master.h>
@@ -45,6 +46,16 @@ void aspeed_i3c_ibi_grow_threshold(struct i3c_hci *hci, unsigned int ibi_max_len
FIELD_PREP(ASPEED_I3C_IBI_TERMINATE_LEN, ibi_max_len));
}
+/* Free TX FIFO entries derived from the vendor FIFO read/write pointers */
+unsigned int aspeed_i3c_avail_tx_entries(struct i3c_hci *hci)
+{
+ u32 ptr = aspeed_i3c_read(hci, ASPEED_I3C_QUEUE_PTR0);
+ unsigned int w = FIELD_GET(ASPEED_I3C_QUEUE_PTR0_TX_W, ptr);
+ unsigned int r = FIELD_GET(ASPEED_I3C_QUEUE_PTR0_TX_R, ptr);
+
+ return CIRC_SPACE(w, r, ASPEED_I3C_TX_QUEUE_ENTRIES);
+}
+
static void aspeed_i3c_phy_write(struct i3c_hci *hci, u32 reg, u32 val)
{
writel(val, to_aspeed_vendor_data(hci)->phy_regs + reg);
diff --git a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
index 84afb2bec956..7147abea830d 100644
--- a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
+++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
@@ -52,6 +52,7 @@ u32 aspeed_i3c_read(struct i3c_hci *hci, u32 reg);
void aspeed_i3c_write(struct i3c_hci *hci, u32 reg, u32 val);
int aspeed_i3c_phy_init(struct i3c_hci *hci);
void aspeed_i3c_ibi_grow_threshold(struct i3c_hci *hci, unsigned int ibi_max_len);
+unsigned int aspeed_i3c_avail_tx_entries(struct i3c_hci *hci);
#define ASPEED_I3C_CTRL 0x00
#define ASPEED_I3C_CTRL_CLOCK_STALL_EN BIT(14)
@@ -68,6 +69,11 @@ void aspeed_i3c_ibi_grow_threshold(struct i3c_hci *hci, unsigned int ibi_max_len
#define ASPEED_I3C_DAA_INDEX2 0x18
#define ASPEED_I3C_DAA_INDEX3 0x1c
+#define ASPEED_I3C_QUEUE_PTR0 0xd8
+#define ASPEED_I3C_QUEUE_PTR0_TX_R GENMASK(24, 20)
+#define ASPEED_I3C_QUEUE_PTR0_TX_W GENMASK(16, 12)
+#define ASPEED_I3C_TX_QUEUE_ENTRIES 32
+
#define ASPEED_I3C_INTR_STATUS 0xe0
#define ASPEED_I3C_INTR_STATUS_ENABLE 0xe4
#define ASPEED_I3C_INTR_SIGNAL_ENABLE 0xe8
--
2.34.1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 7/8] i3c: mipi-i3c-hci: Improve AST2700 PIO TX queue utilization
2026-09-01 11:35 ` [PATCH 7/8] i3c: mipi-i3c-hci: Improve AST2700 PIO TX queue utilization Billy Tsai
@ 2026-09-01 11:51 ` sashiko-bot
0 siblings, 0 replies; 19+ messages in thread
From: sashiko-bot @ 2026-09-01 11:51 UTC (permalink / raw)
To: Billy Tsai
Cc: Alexandre Belloni, devicetree, robh, conor+dt, Frank.Li,
linux-i3c
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling `of_device_is_compatible()` via the `is_aspeed()` helper inside the high-speed PIO TX data transfer loop causes severe CPU overhead and latency.
- [High] Hardware TOCTOU / Stale write pointer read leading to FIFO overflow.
--
commit 3be95193756af176407d7f4071b09a591d9a4eb3
Author: Billy Tsai <billy_tsai@aspeedtech.com>
i3c: mipi-i3c-hci: Improve AST2700 PIO TX queue utilization
This commit updates the MIPI I3C HCI driver to improve TX FIFO utilization
for the AST2700 controller. It introduces a vendor-specific check to poll
the exact number of free FIFO entries using hardware read/write pointers
instead of relying on a half-empty threshold.
> diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
> index 6bd5e43364a1d..3d70257dcc1dc 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/pio.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/pio.c
[ ... ]
> @@ -295,11 +296,23 @@ static bool hci_pio_do_tx(struct i3c_hci *hci, struct hci_pio_data *pio)
> p += (xfer->data_len - xfer->data_left) / 4;
>
> while (xfer->data_left >= 4) {
> - /* bail out if FIFO free space is below set threshold */
> - if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD))
> + unsigned int avail = pio->tx_thresh_size;
> +
> + if (is_aspeed(hci)) {
[Severity: High]
Does calling of_device_is_compatible() inside this fastpath byte-transfer
loop cause significant CPU overhead and latency?
Looking at is_aspeed() in vendor_aspeed.h, it parses the Device Tree and
performs string matching:
static inline bool is_aspeed(struct i3c_hci *hci)
{
return of_device_is_compatible(hci->master.dev.of_node,
"aspeed,ast2700-i3c-hci");
}
Since this executes repeatedly for every 4 bytes transferred, it might ruin
throughput, especially for non-Aspeed controllers where it gets evaluated
constantly. Would it be better to check compatibility exactly once during
probe and cache the result as a quirk flag?
> + /*
> + * The vendor FIFO pointers give the exact free space,
> + * allowing better TX FIFO utilization than the
> + * threshold status.
> + */
> + avail = aspeed_i3c_avail_tx_entries(hci);
[Severity: High]
Could this read of the hardware write pointer immediately after posting a
write lead to a FIFO overflow?
Memory-mapped I/O writes over buses like AHB/AXI are posted, and the
hardware controller might need a few clock cycles to commit the data and
update its pointers.
If aspeed_i3c_avail_tx_entries() reads QUEUE_PTR0 before the hardware
increments the write pointer, won't avail remain artificially high?
If it does, the loop will immediately push another chunk of data,
potentially overflowing the 32-entry hardware FIFO.
Should the driver locally track and deduct the number of words written from
the available space during the active transfer instead of re-polling the
hardware register?
> + if (!avail)
> + return false;
> + } else if (!(pio_reg_read(INTR_STATUS) & STAT_TX_THLD)) {
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-b4-i3c-hci-ast2700-v1-0-19909e7cbd7e@aspeedtech.com?part=7
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 8/8] i3c: mipi-i3c-hci: Support the AST2700 internal pull-ups
2026-09-01 11:35 [PATCH 0/8] i3c: mipi-i3c-hci: Add Aspeed AST2700 support Billy Tsai
` (6 preceding siblings ...)
2026-09-01 11:35 ` [PATCH 7/8] i3c: mipi-i3c-hci: Improve AST2700 PIO TX queue utilization Billy Tsai
@ 2026-09-01 11:35 ` Billy Tsai
7 siblings, 0 replies; 19+ messages in thread
From: Billy Tsai @ 2026-09-01 11:35 UTC (permalink / raw)
To: Alexandre Belloni, Frank Li, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Joel Stanley, Andrew Jeffery, Philipp Zabel
Cc: linux-i3c, devicetree, linux-arm-kernel, linux-aspeed,
linux-kernel, Billy Tsai
Boards that do not populate external I3C pull-up resistors can use the
pull-ups built into the AST2700 PHY.
When bias-pull-up is present, determine the pin voltage domain from
the pinctrl-0 group already selected for the bus and enable the
internal pull-ups on both SCL and SDA at the requested strength.
Currently only pinctrl-0 is inspected; a node selecting its group
through a different pinctrl state is not yet supported. The controller
reset clears the SW force control register, so the internal pull-ups
start disabled unless bias-pull-up requests otherwise. A resistance
with no entry in the domain's table, or an unrecognized pinctrl-0
group, is rejected with an error.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Assisted-by: Claude:claude-fable-5
---
drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c | 118 ++++++++++++++++++++++++
drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h | 6 ++
2 files changed, 124 insertions(+)
diff --git a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
index 182111b857c9..d28d5680d326 100644
--- a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
+++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.c
@@ -12,6 +12,7 @@
#include <linux/i3c/master.h>
#include <linux/io.h>
#include <linux/of.h>
+#include <linux/string.h>
#include "hci.h"
#include "vendor_aspeed.h"
@@ -86,6 +87,100 @@ static u32 aspeed_i3c_get_sdr_ctrl0_reg(struct i3c_hci *hci)
return ASPEED_I3C_PHY_I3C_SDR4_CTRL0;
}
+struct aspeed_i3c_pullup_ohms {
+ u32 ohms;
+ u8 code;
+};
+
+/* SW_FORCE_CTRL strength selector to resistance, by pin voltage domain */
+static const struct aspeed_i3c_pullup_ohms aspeed_i3c_pullup_hv[] = {
+ { 600, 1 },
+ { 470, 2 },
+ { 400, 3 },
+};
+
+static const struct aspeed_i3c_pullup_ohms aspeed_i3c_pullup_lv[] = {
+ { 750, 1 },
+ { 550, 2 },
+ { 317, 3 },
+ { 400, 4 },
+ { 261, 5 },
+ { 231, 6 },
+ { 177, 7 },
+};
+
+/* pinctrl-0 group names for each pin voltage domain */
+static const char * const aspeed_i3c_hv_groups[] = {
+ "HVI3C0", "HVI3C1", "HVI3C2", "HVI3C3",
+ "HVI3C12", "HVI3C13", "HVI3C14", "HVI3C15",
+};
+
+static const char * const aspeed_i3c_lv_groups[] = {
+ "I3C4", "I3C5", "I3C6", "I3C7",
+ "I3C8", "I3C9", "I3C10", "I3C11",
+};
+
+static int aspeed_i3c_pin_is_hv(struct i3c_hci *hci, bool *is_hv)
+{
+ struct device_node *np = hci->master.dev.of_node;
+ struct device_node *state;
+ const char *group;
+ int ret, i;
+
+ state = of_parse_phandle(np, "pinctrl-0", 0);
+ if (!state)
+ return -ENODEV;
+
+ ret = of_property_read_string(state, "groups", &group);
+ if (ret)
+ goto put_state;
+
+ for (i = 0; i < ARRAY_SIZE(aspeed_i3c_hv_groups); i++) {
+ if (!strcmp(group, aspeed_i3c_hv_groups[i])) {
+ *is_hv = true;
+ goto put_state;
+ }
+ }
+
+ for (i = 0; i < ARRAY_SIZE(aspeed_i3c_lv_groups); i++) {
+ if (!strcmp(group, aspeed_i3c_lv_groups[i])) {
+ *is_hv = false;
+ goto put_state;
+ }
+ }
+
+ dev_err(&hci->master.dev, "unrecognized pinctrl-0 group \"%s\"\n",
+ group);
+ ret = -EINVAL;
+
+put_state:
+ of_node_put(state);
+ return ret;
+}
+
+static int aspeed_i3c_pullup_code(struct i3c_hci *hci, bool is_hv, u32 ohms)
+{
+ const struct aspeed_i3c_pullup_ohms *map;
+ size_t i, n;
+
+ if (is_hv) {
+ map = aspeed_i3c_pullup_hv;
+ n = ARRAY_SIZE(aspeed_i3c_pullup_hv);
+ } else {
+ map = aspeed_i3c_pullup_lv;
+ n = ARRAY_SIZE(aspeed_i3c_pullup_lv);
+ }
+
+ for (i = 0; i < n; i++)
+ if (map[i].ohms == ohms)
+ return map[i].code;
+
+ dev_err(&hci->master.dev,
+ "unsupported internal pull-up %u ohms for %s-voltage domain\n",
+ ohms, is_hv ? "high" : "low");
+ return -EINVAL;
+}
+
int aspeed_i3c_phy_init(struct i3c_hci *hci)
{
u16 hcnt, lcnt, total_cnt, min_tbit_cnt, cas_lcnt, cas_cnt, cbp_cnt;
@@ -95,6 +190,7 @@ int aspeed_i3c_phy_init(struct i3c_hci *hci)
struct device_node *np = hci->master.dev.of_node;
u32 sdr_ctrl0_reg, ctrl0, ctrl1, ctrl2;
unsigned long core_rate, period_ns;
+ u32 pullup_ohms = 0;
core_rate = clk_get_rate(to_aspeed_vendor_data(hci)->clk);
if (!core_rate) {
@@ -257,6 +353,28 @@ int aspeed_i3c_phy_init(struct i3c_hci *hci)
aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_PULLUP_EN, 0);
+ of_property_read_u32(np, "bias-pull-up", &pullup_ohms);
+ if (pullup_ohms) {
+ int ret, strength;
+ bool is_hv;
+
+ ret = aspeed_i3c_pin_is_hv(hci, &is_hv);
+ if (ret)
+ return ret;
+
+ strength = aspeed_i3c_pullup_code(hci, is_hv, pullup_ohms);
+ if (strength < 0)
+ return strength;
+
+ aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_SW_FORCE_CTRL,
+ ASPEED_I3C_PHY_SW_FORCE_SCL_PU_EN |
+ ASPEED_I3C_PHY_SW_FORCE_SDA_PU_EN |
+ FIELD_PREP(ASPEED_I3C_PHY_SW_FORCE_SCL_PU_VAL,
+ strength) |
+ FIELD_PREP(ASPEED_I3C_PHY_SW_FORCE_SDA_PU_VAL,
+ strength));
+ }
+
hcnt = aspeed_i3c_ns_to_cnt(ASPEED_I3C_PHY_OD_DAP_NS, period_ns);
aspeed_i3c_phy_write(hci, ASPEED_I3C_PHY_I3C_OD_CTRL4,
FIELD_PREP(ASPEED_I3C_PHY_I3C_OD_CTRL4_DAP, hcnt));
diff --git a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
index 7147abea830d..80938c580800 100644
--- a/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
+++ b/drivers/i3c/master/mipi-i3c-hci/vendor_aspeed.h
@@ -83,6 +83,12 @@ unsigned int aspeed_i3c_avail_tx_entries(struct i3c_hci *hci);
#define ASPEED_INTR_SUM_PIO BIT(1)
#define ASPEED_INTR_SUM_CAP BIT(0)
+#define ASPEED_I3C_PHY_SW_FORCE_CTRL 0x04
+#define ASPEED_I3C_PHY_SW_FORCE_SCL_PU_EN BIT(28)
+#define ASPEED_I3C_PHY_SW_FORCE_SDA_PU_EN BIT(24)
+#define ASPEED_I3C_PHY_SW_FORCE_SCL_PU_VAL GENMASK(10, 8)
+#define ASPEED_I3C_PHY_SW_FORCE_SDA_PU_VAL GENMASK(2, 0)
+
#define ASPEED_I3C_PHY_I2C_FM_CTRL0 0x08
#define ASPEED_I3C_PHY_I2C_FM_CTRL0_CAS GENMASK(26, 16)
#define ASPEED_I3C_PHY_I2C_FM_CTRL0_SU_STO GENMASK(10, 0)
--
2.34.1
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 19+ messages in thread