Re: [PATCH] i3c: master: dw-i3c-master: Add HDR-DDR support to Designware Controller.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] i3c: bitwise OR without clear corrupts COMMAND_PORT_SPEED in HDR-DDR mode
- [High] i3c: ENTHDR0 CCC redundantly applied to all transfers in a batch
- [High] i3c: speed field corruption and ENTHDR repetition in HDR mode
- [Medium] i3c: missing HDR-DDR capability advertisement in dw_i3c_master_bus_init()
--

commit 9eeaabb296fe13353bcd0cb7883076567e265760
Author: Dineshkumar Parameswaran <Dineshkumar.Parameswaran@synopsys.com>

i3c: master: dw-i3c-master: Add HDR-DDR support to Designware Controller.
    
Implement HDR-DDR transfer support for the DesignWare Controller.
Only support for private transfers and does not support sending CCC
commands in HDR mode.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index d1119f203d1b6..d1013204ab7b7 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
[ ... ]
> @@ -1017,6 +1019,11 @@ static int dw_i3c_master_i3c_xfers(struct i3c_dev_desc *dev,
>  			       COMMAND_PORT_DEV_INDEX(data->index) |
>  			       COMMAND_PORT_ROC;
>  
> +		if (mode == I3C_HDR_DDR)
> +			cmd->cmd_lo |= COMMAND_PORT_CP |
> +				COMMAND_PORT_CMD(I3C_CCC_ENTHDR(0)) |
> +				COMMAND_PORT_SPEED(DW_I3C_HDR_DDR_SPEED);

[Severity: High]
Since cmd->cmd_lo was initialized earlier in this loop with
COMMAND_PORT_SPEED(dev->info.max_read_ds) or max_write_ds, won't this bitwise
OR operation merge the two speed values together without clearing the previous
bits? This could result in corrupted speed configuration bits being sent to the
hardware controller.

[Severity: High]
This block is inside the for loop iterating over i3c_nxfers. Will this
cause the COMMAND_PORT_CP and I3C_CCC_ENTHDR(0) command bits to be repeatedly
applied to every segment of a multi-message transfer, instead of just the first
transfer? Sending the ENTHDR0 command repeatedly while already in HDR mode may
cause protocol violations on the bus.

[Severity: Medium]
Since this patch adds HDR-DDR transfer support, should the master controller
also advertise this new capability during bus initialization?

In dw_i3c_master_bus_init(), info.hdr_cap is not updated to include
the I3C_HDR_DDR capability:

	memset(&info, 0, sizeof(info));
	info.dyn_addr = ret;

	ret = i3c_master_set_info(&master->base, &info);

Without setting info.hdr_cap to include I3C_CCC_HDR_MODE(I3C_HDR_DDR), won't
the I3C core reject HDR-DDR transfers with -EOPNOTSUPP in
i3c_dev_do_xfers_locked() before they can ever reach the newly added code in
dw_i3c_master_i3c_xfers()?

> +
>  		if (i == (i3c_nxfers - 1))
>  			cmd->cmd_lo |= COMMAND_PORT_TOC;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013073126.5948-1-parame@synopsys.com?part=1

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