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.