Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: mkshah=codeaurora.org@mg.codeaurora.org
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org A3070C433CA
Subject: Re: [PATCH v2] soc: qcom: rpmh: Remove serialization of TCS commands
To:     Doug Anderson <dianders@chromium.org>
Cc:     Bjorn Andersson <bjorn.andersson@linaro.org>,
        Andy Gross <agross@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Rajendra Nayak <rnayak@codeaurora.org>,
        Lina Iyer <ilina@codeaurora.org>,
        Srinivas Rao L <lsrao@codeaurora.org>,
        Stephen Boyd <swboyd@chromium.org>
References: <1606203086-31218-1-git-send-email-mkshah@codeaurora.org>
 <CAD=FV=V60zVYkFGQvAu6qfpumBL6+fm_9ziRCSiN7Um7+ra6zw@mail.gmail.com>
From:   Maulik Shah <mkshah@codeaurora.org>
Message-ID: <e74ed5ba-8b5f-638a-62f5-19eb2501ce1a@codeaurora.org>
Date:   Thu, 7 Jan 2021 14:07:52 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <CAD=FV=V60zVYkFGQvAu6qfpumBL6+fm_9ziRCSiN7Um7+ra6zw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-GB
Precedence: bulk

Hi Doug,

On 12/4/2020 3:14 AM, Doug Anderson wrote:
> Hi,
>
> On Mon, Nov 23, 2020 at 11:32 PM Maulik Shah <mkshah@codeaurora.org> wrote:
>> @@ -423,8 +422,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
>>                          cmd = &req->cmds[j];
>>                          sts = read_tcs_cmd(drv, RSC_DRV_CMD_STATUS, i, j);
>>                          if (!(sts & CMD_STATUS_ISSUED) ||
>> -                          ((req->wait_for_compl || cmd->wait) &&
>> -                          !(sts & CMD_STATUS_COMPL))) {
>> +                          (cmd->wait && !(sts & CMD_STATUS_COMPL))) {
>>                                  pr_err("Incomplete request: %s: addr=%#x data=%#x",
>>                                         drv->name, cmd->addr, cmd->data);
>>                                  err = -EIO;
> In my review of v1 all those months ago, the way we left things was
> that I disagreed with this part of the patch, and I still do.  I think
> you should leave things the way they were in tcs_tx_done().  Copying
> my un-responded-to comments from v1 here for you:
>
> In your patch in __tcs_buffer_write(), if "wait_for_compl" is set then
> "CMD_MSGID_RESP_REQ" will be added for every message in the request,
> right?  That's because you have this bit of code:
>
> /* Convert all commands to RR when the request has wait_for_compl set */
> cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0;
>
> That means that if _either_ "cmd->wait" or "req->wait_for_compl" is
> set then you expect the "sts" to have "CMD_STATUS_COMPL", right?
> That's exactly the code that used to be there.
>
> Said another way, if "req->wait_for_compl" was set then it's an error
> if any of our commands are missing the "CMD_STATUS_COMPL" bit, right?
>
>
>> @@ -30,7 +30,7 @@ enum rpmh_state {
>>    *
>>    * @addr: the address of the resource slv_id:18:16 | offset:0:15
>>    * @data: the resource state request
>> - * @wait: wait for this request to be complete before sending the next
>> + * @wait: ensure that this command is complete before returning
> In my response to v1 I suggested that a comment would be nice here.
> Something akin to:
>
> Setting "wait" here only makes sense in the batch case for active-only
> transfers.
>
> This is because:
> * rpmh_write_async() - There's no callback and rpmh_write_async()
> doesn't set the "completion" to anything so there's nobody that cares
> at all
>
> * DEFINE_RPMH_MSG_ONSTACK - always sets wait_for_compl.
>
> -Doug

Addressed both comments and sent v3.

Thanks,
Maulik

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation