[PATCH] i3c: master: dw: Report CCC M0/M2 errors and retry on failure
NG, TZE YEE
tze.yee.ng at altera.com
Tue Jun 9 03:01:32 PDT 2026
On 3/6/2026 5:27 pm, Adrian Hunter wrote:
> [You don't often get email from adrian.hunter at intel.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> On 03/06/2026 10:25, tze.yee.ng at altera.com wrote:
>> From: Adrian Ng Ho Yin <adrian.ho.yin.ng at altera.com>
>>
>> Improve CCC reliability on the DesignWare master by reporting
>> standard I3C error codes to the core and retrying the command once
>> when the failure looks transient.
>>
>> Map hardware status to ccc->err as follows:
>> - I3C_ERROR_M2: IBA NACK or address NACK
>
> That sounds like it duplicates dw_i3c_master_set_dev_nack_retry().
>
These are different layers and do not duplicate each other.
set_dev_nack_retry() programs the DesignWare DAT DEV_NACK_RETRY field so
the hardware may automatically retry private transfers when a target
NACKs. That is a controller-specific mechanism below the I3C core.
Reporting I3C_ERROR_M2 in ccc->err is separate: it tells the core that a
CCC failed with an address-header NACK (e.g. IBI/CRR arbitration per
spec §5.1.2.2.3). The driver only maps DW response-queue status to
standard I3C error codes; it does not retry CCCs itself anymore.
>> - I3C_ERROR_M0: frame error, or an incomplete GET/SET payload when
>> the controller reports a successful transfer (short read vs
>> requested length; GETMRL uses GET_MRL_MIN_LEN if the buffer is
>> larger than struct i3c_ccc_mrl)
>
> Need to consider what would be better handled by I3C core.
> Anything related to the protocol, that every controller driver
> needs, should be handled there.
>
Agree. I will move the protocol-level handling to master.c.>>
>> On a successful GET CCC, set dests[0].payload.len to the number of
>> bytes actually received.
>
> Is that a bug fix? Maybe it needs a separate patch?
>
Yes, I will create a separate patch for this standalone bug fix in this
series.>>
>> Reset ccc->err before each attempt. Retry the CCC get or set once
>> if the transfer fails with I3C_ERROR_M0 or I3C_ERROR_M2.
>
> Again, please consider what would be better handled in
> drivers/i3c/master.c
>
I will rework the series based on your feedback. Summary of changes:
Patch split (3 commits)
Patch 1/3 — DW: report actual GET payload.len on success
Driver-only bug fix. On successful GET CCC, set dests[0].payload.len
from RESPONSE_PORT_DATA_LEN. Core helpers such as
i3c_master_getmrl_locked() depend on the actual RX count.
Patch 2/3 — DW: map CCC hardware errors to I3C M0/M2
Driver-only mapping from DW RESPONSE_ERROR_* constants to ccc->err. No
payload validation and no retry loop in the driver. ccc->err is reset
before each transfer. RESPONSE_ERROR_ADDRESS_NACK now returns -EIO (not
-EINVAL) alongside I3C_ERROR_M2.
Patch 3/3 — core: validate GET payload length and retry M0/M2 once
- Validate GET CCC payload length after a successful transfer; short/
invalid reads become I3C_ERROR_M0 / -EIO.
- GETMRL: exactly 2 or 3 bytes. GETMXDS: 2 or 5 bytes. Other GET CCCs
must match the requested length.
- Retry once on I3C_ERROR_M0 or I3C_ERROR_M2 in
i3c_master_send_ccc_cmd_locked().
- Retries are GET-only (cmd->rnw) so side-effecting SET CCCs are not
repeated.
- Restore dests[].payload.len to the originally requested length before
each attempt and again before returning an error.
>>
>> Signed-off-by: Adrian Ng Ho Yin <adrian.ho.yin.ng at altera.com>
>> Signed-off-by: Tze Yee Ng <tze.yee.ng at altera.com>
>> ---
>> drivers/i3c/master/dw-i3c-master.c | 79 ++++++++++++++++++++++++++----
>> 1 file changed, 69 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
>> index 655693a2187e..85ea0af57717 100644
>> --- a/drivers/i3c/master/dw-i3c-master.c
>> +++ b/drivers/i3c/master/dw-i3c-master.c
>> @@ -243,6 +243,12 @@
>> #define AMD_I3C_OD_PP_TIMING BIT(1)
>> #define DW_I3C_DISABLE_RUNTIME_PM_QUIRK BIT(2)
>>
>> +/* Minimum GETMRL payload (read_len only; no IBI byte). */
>> +#define GET_MRL_MIN_LEN 2
>> +
>> +/* Maximum number of retries for CCC commands */
>> +#define DW_I3C_CCC_MAX_RETRIES 2
>> +
>> struct dw_i3c_cmd {
>> u32 cmd_lo;
>> u32 cmd_hi;
>> @@ -708,12 +714,47 @@ static void dw_i3c_master_bus_cleanup(struct i3c_master_controller *m)
>> dw_i3c_master_disable(master);
>> }
>>
>> +static bool dw_i3c_ccc_get_len_mismatch(u8 ccc_id, u16 req_len, u16 rx_len)
>> +{
>> + if (ccc_id != I3C_CCC_GETMRL)
>> + return rx_len < req_len;
>> +
>> + /*
>> + * GETMRL returns 2 or 3 bytes; core sets req_len accordingly.
>> + * If the buffer is larger, only enforce the minimum valid size.
>> + */
>> + if (req_len <= sizeof(struct i3c_ccc_mrl))
>> + return rx_len < req_len;
>> +
>> + return rx_len < GET_MRL_MIN_LEN;
>> +}
>> +
>> +static void dw_i3c_ccc_map_err(struct i3c_ccc_cmd *ccc,
>> + struct dw_i3c_cmd *cmd,
>> + bool data_len_mismatch, int *ret)
>> +{
>> + u8 err = cmd->error;
>> +
>> + if (err == RESPONSE_ERROR_IBA_NACK ||
>> + err == RESPONSE_ERROR_ADDRESS_NACK) {
>> + ccc->err = I3C_ERROR_M2;
>> + } else if (err == RESPONSE_ERROR_FRAME) {
>> + ccc->err = I3C_ERROR_M0;
>> + } else if (data_len_mismatch && err == RESPONSE_NO_ERROR && !*ret) {
>> + ccc->err = I3C_ERROR_M0;
>> + *ret = -EIO;
>> + }
>> +}
>> +
>> static int dw_i3c_ccc_set(struct dw_i3c_master *master,
>> struct i3c_ccc_cmd *ccc)
>> {
>> struct dw_i3c_cmd *cmd;
>> + bool data_len_mismatch;
>> int ret, pos = 0;
>>
>> + ccc->err = I3C_ERROR_UNKNOWN;
>> +
>> if (ccc->id & I3C_CCC_DIRECT) {
>> pos = dw_i3c_master_get_addr_pos(master, ccc->dests[0].addr);
>> if (pos < 0)
>> @@ -742,8 +783,14 @@ static int dw_i3c_ccc_set(struct dw_i3c_master *master,
>> dw_i3c_master_dequeue_xfer(master, xfer);
>>
>> ret = xfer->ret;
>> - if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
>> - ccc->err = I3C_ERROR_M2;
>> + cmd = &xfer->cmds[0];
>> + /*
>> + * RESPONSE_PORT_DATA_LEN reports bytes transferred; on SET CCCs
>> + * this reflects the write count (stored in cmd->rx_len).
>> + */
>> + data_len_mismatch = ccc->dests[0].payload.len &&
>> + cmd->rx_len < ccc->dests[0].payload.len;
>> + dw_i3c_ccc_map_err(ccc, cmd, data_len_mismatch, &ret);
>>
>> return ret;
>> }
>> @@ -751,21 +798,27 @@ static int dw_i3c_ccc_set(struct dw_i3c_master *master,
>> static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
>> {
>> struct dw_i3c_cmd *cmd;
>> + u16 req_len;
>> + bool rx_len_mismatch;
>> int ret, pos;
>>
>> + ccc->err = I3C_ERROR_UNKNOWN;
>> +
>> pos = dw_i3c_master_get_addr_pos(master, ccc->dests[0].addr);
>> if (pos < 0)
>> return pos;
>>
>> + req_len = ccc->dests[0].payload.len;
>> +
>> struct dw_i3c_xfer *xfer __free(kfree) = dw_i3c_master_alloc_xfer(master, 1);
>> if (!xfer)
>> return -ENOMEM;
>>
>> cmd = xfer->cmds;
>> cmd->rx_buf = ccc->dests[0].payload.data;
>> - cmd->rx_len = ccc->dests[0].payload.len;
>> + cmd->rx_len = req_len;
>>
>> - cmd->cmd_hi = COMMAND_PORT_ARG_DATA_LEN(ccc->dests[0].payload.len) |
>> + cmd->cmd_hi = COMMAND_PORT_ARG_DATA_LEN(req_len) |
>> COMMAND_PORT_TRANSFER_ARG;
>>
>> cmd->cmd_lo = COMMAND_PORT_READ_TRANSFER |
>> @@ -780,8 +833,12 @@ static int dw_i3c_ccc_get(struct dw_i3c_master *master, struct i3c_ccc_cmd *ccc)
>> dw_i3c_master_dequeue_xfer(master, xfer);
>>
>> ret = xfer->ret;
>> - if (xfer->cmds[0].error == RESPONSE_ERROR_IBA_NACK)
>> - ccc->err = I3C_ERROR_M2;
>> + cmd = &xfer->cmds[0];
>> + rx_len_mismatch = dw_i3c_ccc_get_len_mismatch(ccc->id, req_len,
>> + cmd->rx_len);
>> + dw_i3c_ccc_map_err(ccc, cmd, rx_len_mismatch, &ret);
>> + if (!ret)
>> + ccc->dests[0].payload.len = cmd->rx_len;
>>
>> return ret;
>> }
>> @@ -796,6 +853,7 @@ static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
>> struct i3c_ccc_cmd *ccc)
>> {
>> struct dw_i3c_master *master = to_dw_i3c_master(m);
>> + int retries = DW_I3C_CCC_MAX_RETRIES;
>> int ret = 0;
>>
>> if (ccc->id == I3C_CCC_ENTDAA)
>> @@ -816,10 +874,11 @@ static int dw_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
>> return ret;
>> }
>>
>> - if (ccc->rnw)
>> - ret = dw_i3c_ccc_get(master, ccc);
>> - else
>> - ret = dw_i3c_ccc_set(master, ccc);
>> + do {
>> + ret = ccc->rnw ? dw_i3c_ccc_get(master, ccc)
>> + : dw_i3c_ccc_set(master, ccc);
>> + } while (--retries &&
>> + ret && (ccc->err == I3C_ERROR_M0 || ccc->err == I3C_ERROR_M2));
>>
>> pm_runtime_put_autosuspend(master->dev);
>> return ret;
>
More information about the linux-i3c
mailing list