Re: [PATCH 02/19] soundwire: amd: Add support for AMD Master driver

[Pierre-Louis Bossart]

On 1/16/23 01:53, Mukunda,Vijendar wrote:
> On 14/01/23 00:11, Pierre-Louis Bossart wrote:
>>>>> +	for (index = 0; index < 2; index++) {
>>>>> +		if (response_buf[index] == -ETIMEDOUT) {
>>>>> +			dev_err(ctrl->dev, "Program SCP cmd timeout\n");
>>>>> +			timeout = 1;
>>>>> +		} else if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
>>>>> +			no_ack = 1;
>>>>> +			if (response_buf[index] & AMD_SDW_MCP_RESP_NACK) {
>>>>> +				nack = 1;
>>>>> +				dev_err(ctrl->dev, "Program SCP NACK received\n");
>>>>> +			}
>>>> this is a copy of the cadence_master.c code... With the error added that
>>>> this is not for a controller but for a master...
>>> Its manager instance only. Our immediate command and response
>>> mechanism allows sending commands over the link and get the
>>> response for every command immediately, unlike as mentioned in
>>> candence_master.c.
>> I don't get the reply. The Cadence IP also has the ability to get the
>> response immediately. There's limited scope for creativity, the commands
>> are defined in the spec and the responses as well.
> As per our understanding in Intel code, responses are processed
> after sending all commands.
> In our case, we send the command and process the response
> immediately before invoking the next command.

The Cadence IP can queue a number of commands, I think 8 off the top of
my head. But the response is provided immediately after each command.

Maybe the disconnect is that there's an ability to define a watermark on
the response buffer, so that the software can decide to process the
command responses in one shot.

>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	if (timeout) {
>>>>> +		dev_err_ratelimited(ctrl->dev,
>>>>> +				    "SCP_addrpage command timeout for Slave %d\n", msg->dev_num);
>>>>> +		return SDW_CMD_TIMEOUT;
>>>>> +	}
>>>>> +
>>>>> +	if (nack) {
>>>>> +		dev_err_ratelimited(ctrl->dev,
>>>>> +				    "SCP_addrpage NACKed for Slave %d\n", msg->dev_num);
>>>>> +		return SDW_CMD_FAIL;
>>>>> +	}
>>>>> +
>>>>> +	if (no_ack) {
>>>>> +		dev_dbg_ratelimited(ctrl->dev,
>>>>> +				    "SCP_addrpage ignored for Slave %d\n", msg->dev_num);
>>>>> +		return SDW_CMD_IGNORED;
>>>>> +	}
>>>>> +	return SDW_CMD_OK;
>>>> this should probably become a helper since the response is really the
>>>> same as in cadence_master.c
>>>>
>>>> There's really room for optimization and reuse here.
>>> not really needed. Please refer above comment as command/response
>>> mechanism differs from Intel's implementation.
>> how? there's a buffer of responses in both cases. please clarify.
> Ours implementation is not interrupt driven like Intel.
> When we send command over the link, we will wait for command's
> response in polling method and process the response immediately
> before issuing the new command.

On the Intel side we use an interrupt to avoid polling, and in case of N
commands the watermark will be set to N to reduce the overhead. That
said, most users only use 1 command at a time, it's only recently that
Cirrus Logic experimented with multiple commands to speed-up transfers.

Even if there are differences in the way the responses are processed,
whether one-at-a-time or in a batch, the point remains that each command
response can be individually analyzed and that could be using a helper -
moving code from cadence_master.c into the bus layer.