Re: [PATCH v1 RESEND] SoundWire: Allow Prepare command for Simplified_CP_SM

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

On 2/9/26 08:09, Niranjan H Y wrote:
> As defined in the MIPI SoundWire specification v1-2 for
> Simplified Channel Prepare State Machine (Simplified_CP_SM):
> 
> * Figure 141 for the Simplified_CP_SM in the specification
>   shows the "Ready" state (NF=0, P=1) that can be reached via
>   "Prepare0 OR Prepare1" transitions.
> * Table 115 (Stimulus to the Channel Prepare State Machine)
>   indicates that Prepare0 and Prepare1 are read-only/"write-ignored"
>   bits for Simplified_CP_SM.
> 
> In TI device implementations, we've found that some devices with
> Simplified_CP_SM still benefit from receiving the Prepare command.

This is a bit weird really...

There is no such thing as a Prepare command, what is used is a regular write that sets or clears Prepare bits in port registers. If those bits are NOT read-only/write-ignored, then presumably they are writable. One could then argue the device does not follow the Simplified_CP_SM state machine but the generic one.
It would also be good to clarify when the device is actually prepared after the bits are set, if the implementation cannot guarantee that the prepared status is reached by the end of the frame where the write occurs, then it's definitively NOT a Simplified_CP_SM.

What happens is the simple_ch_prep_sm bit is not set for those devices, and the regular state machine is used instead? Is anything broken?

> This patch modifies the code to:
> 1. Send the Prepare command to all devices, including those with
>    Simplified_CP_SM
> 2. Ignore errors returned by devices with Simplified_CP_SM that
>    might not support this command
> 
> This approach maintains compatibility with all devices while ensuring
> proper functionality of dataport operations for devices that can
> make use of the Prepare command despite using Simplified_CP_SM.
> 
> Signed-off-by: Niranjan H Y <niranjan.hy@ti.com>
> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> Reviewed-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Tested-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Tested-by: Shuming Fan <shumingf@realtek.com>
> ---
>  drivers/soundwire/stream.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 38c9dbd35..33605dc83 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -504,14 +504,19 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
>  	sdw_do_port_prep(s_rt, prep_ch, prep ? SDW_OPS_PORT_PRE_PREP : SDW_OPS_PORT_PRE_DEPREP);
>  
>  	/* Prepare Slave port implementing CP_SM */
> -	if (!simple_ch_prep_sm) {
> -		addr = SDW_DPN_PREPARECTRL(p_rt->num);
> -
> -		if (prep)
> -			ret = sdw_write_no_pm(s_rt->slave, addr, p_rt->ch_mask);
> -		else
> -			ret = sdw_write_no_pm(s_rt->slave, addr, 0x0);
> +	/* For Simplified_CP_SM, MIPI SoundWire specification v1-2 indicates
> +	 * Prepare bits are "write-ignored" - this means devices may ignore the command.
> +	 * Some devices still benefit from receiving this command even when using
> +	 * Simplified_CP_SM, so we send it to all devices and ignore errors from those
> +	 * that don't support it.
> +	 */
> +	addr = SDW_DPN_PREPARECTRL(p_rt->num);
> +	if (prep)
> +		ret = sdw_write_no_pm(s_rt->slave, addr, p_rt->ch_mask);
> +	else
> +		ret = sdw_write_no_pm(s_rt->slave, addr, 0x0);
>  
> +	if (!simple_ch_prep_sm) {
>  		if (ret < 0) {
>  			dev_err(&s_rt->slave->dev,
>  				"Slave prep_ctrl reg write failed\n");
> @@ -530,6 +535,11 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
>  				"Chn prep failed for port %d: %d\n", prep_ch.num, ret);
>  			return ret;
>  		}
> +	} else {
> +		/* Some device return error for the prepare command,
> +		 * ignore the error for Simplified CP_SM

that comment is misleading anyways, it's not 'some device' it's ALL the conformant devices that will return Command_Ignored when trying to write read-only bits. You haven't described what the TI devices would return in this case.

> +		 */
> +		ret = 0;
>  	}
>  
>  	/* Inform slaves about ports prepared */