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

["Mukunda,Vijendar" <vijendar.mukunda@amd.com>]

On 16/01/23 20:27, Pierre-Louis Bossart wrote:
>
> 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.
>
> will implement a helper function to analyze the response.