Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d5638ec8-3fa4-4643-9740-ef87a4ba5833@linux.intel.com>
Date:   Mon, 16 Jan 2023 08:57:13 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.4.2
Subject: Re: [PATCH 02/19] soundwire: amd: Add support for AMD Master driver
Content-Language: en-US
To:     "Mukunda,Vijendar" <vijendar.mukunda@amd.com>, broonie@kernel.org,
        vkoul@kernel.org, alsa-devel@alsa-project.org
Cc:     Basavaraj.Hiregoudar@amd.com, Sunil-kumar.Dommati@amd.com,
        Mario.Limonciello@amd.com, Mastan.Katragadda@amd.com,
        arungopal.kondaveeti@amd.com,
        Bard Liao <yung-chuan.liao@linux.intel.com>,
        Sanyog Kale <sanyog.r.kale@intel.com>,
        open list <linux-kernel@vger.kernel.org>
References: <20230111090222.2016499-1-Vijendar.Mukunda@amd.com>
 <20230111090222.2016499-3-Vijendar.Mukunda@amd.com>
 <991ff630-17a7-eef3-1436-e4a905fe0541@linux.intel.com>
 <78741dee-9257-77c2-8950-0519ccb462e6@amd.com>
 <420258d4-1f66-5288-f421-b26b2a2a35ea@linux.intel.com>
 <dbf20726-3900-9bff-7a72-14608702f636@amd.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <dbf20726-3900-9bff-7a72-14608702f636@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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.