Re: [PATCH] i3c: master: dw: Report CCC M0/M2 errors and retry on failure

[NG, TZE YEE]

On 3/6/2026 5:27 pm, Adrian Hunter wrote:
> [You don't often get email from adrian.hunter@xxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 03/06/2026 10:25, tze.yee.ng@xxxxxxxxxx wrote:
>> From: Adrian Ng Ho Yin <adrian.ho.yin.ng@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>
>> ---
>>   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;
>