Re: [PATCH v5 2/8] firmware: add support for ARM System Control and Power Interface(SCPI) protocol

[Sudeep Holla <sudeep.holla@arm.com>]

On 29/07/15 12:19, Jassi Brar wrote:
> On Wed, Jul 29, 2015 at 2:08 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 29/07/15 09:05, Jassi Brar wrote:
>>
>>>
>>>> +static int scpi_probe(struct platform_device *pdev)
>>>> +{
>>>> +       int count, idx, ret;
>>>> +       struct resource res;
>>>> +       struct scpi_chan *scpi_chan;
>>>> +       struct device *dev = &pdev->dev;
>>>> +       struct device_node *np = dev->of_node;
>>>> +
>>>> +       scpi_info = devm_kzalloc(dev, sizeof(*scpi_info), GFP_KERNEL);
>>>> +       if (!scpi_info)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       count = of_count_phandle_with_args(np, "mboxes", "#mbox-cells");
>>>> +       if (count < 0) {
>>>> +               dev_err(dev, "no mboxes property in '%s'\n",
>>>> np->full_name);
>>>> +               return -ENODEV;
>>>> +       }
>>>> +
>>>> +       scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan),
>>>> GFP_KERNEL);
>>>> +       if (!scpi_chan)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       for (idx = 0; idx < count; idx++) {
>>>> +               resource_size_t size;
>>>> +               struct scpi_chan *pchan = scpi_chan + idx;
>>>> +               struct mbox_client *cl = &pchan->cl;
>>>> +               struct device_node *shmem = of_parse_phandle(np, "shmem",
>>>> idx);
>>>> +
>>>> +               if (of_address_to_resource(shmem, 0, &res)) {
>>>> +                       dev_err(dev, "failed to get SCPI payload mem
>>>> resource\n");
>>>> +                       ret = -EINVAL;
>>>> +                       goto err;
>>>> +               }
>>>> +
>>>> +               size = resource_size(&res);
>>>> +               pchan->rx_payload = devm_ioremap(dev, res.start, size);
>>>> +               if (!pchan->rx_payload) {
>>>> +                       dev_err(dev, "failed to ioremap SCPI payload\n");
>>>> +                       ret = -EADDRNOTAVAIL;
>>>> +                       goto err;
>>>> +               }
>>>> +               pchan->tx_payload = pchan->rx_payload + (size >> 1);
>>>> +
>>>> +               cl->dev = dev;
>>>> +               cl->rx_callback = scpi_handle_remote_msg;
>>>> +               cl->tx_prepare = scpi_tx_prepare;
>>>> +               cl->tx_block = true;
>>>> +               cl->tx_tout = 50;
>>>> +               cl->knows_txdone = false; /* controller can ack */
>>>>
>>>    This is the cause of your problems that you think should be solved by
>>> using hrtimer.
>>>
>>
>> Ah sorry, it's stupid mistake on my part while writing the comment. It
>> should have been controller can't ack, fixed locally now thanks for
>> pointing it out.
>>
> No, I am talking about code, not the comment.
>
>>>    Controller may or may not (like MHU) set txdone_irq. However every
>>> scpi command (struct scpi_ops members) is replied to as a response
>>> packet reporting success or failure.
>>
>>
>> No that's not true, I have already mentioned that couple of times in the
>> other thread. It's just wrong comment here which went unnoticed from
>> day#1, sorry for that.
>>
>>> So the client should set 'knows_txdone' to be true unless it is told
>>> the controller on that platform supports txdone_irq (what you call
>>> 'ack').
>>>
>> I got the concept but SCP can't ack via protocol, protocol has no such
>> provision and it sets flags in MHU status register.
>>
> You either don't get the concept of TXDONE_BY_ACK  or deliberately
> overlook my point.
>

No I do understand the concept and didn't overlook the points you made.

> Assuming the former, let me explain. When a client receives a
> response, it can be sure that the request has already been read by the
> remote.

Waiting for the response would be too late for few expensive commands
(e.g setting up external regulators). The remote firmware acknowledges
Tx by setting status flags and will be ready to accept new commands.

> If the protocol specifies every request has some response, the

Not always true there can be few commands without response. The protocol
specifies that we need check the status flag before sending the new
command as it's bidirectional, hence polling is recommended (Section 2.2
Communication flow in the SCPI specification)

> client should assert 'knows_txdone' and call mbox_client_txdone() upon
> receiving a reply packet.

Since this is not always true and not recommended in the specification,
I am hesitant to use this option as the firmware can always change their
internal mechanics without breaking the protocol. We need to ensure we 
are compliant to the spec.

> So I said,  cl->knows_txdone = false;   is the root of problems you

It could be and won't rule that out. I would prefer using knows_txdone
and use mbox_client_txdone if feasible, but I can't as the without
violating the specification.

FYI, I had tried it and ended up with issues in the firmware. The
argument from the firmware is that we aren't specification compliant,
so I had to use polling.

> report. If you fix that, the performance should be even better than
> using hrtimer.
>

That would have been ideal and much better but I can't use that for
above mentioned reason.

Though you had valid concerns here and I hope I clarified them all
sucessfully, none were related to hrtimers.

Can I assume you are fine with hrtimers ? If so, can you review this patch ?

Regards,
Sudeep