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

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

On 30/07/15 18:56, Jassi Brar wrote:
> On Wed, Jul 29, 2015 at 6:20 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> On 29/07/15 12:19, Jassi Brar wrote:
>>
>>> 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.
>>
> No. Polling still happens. If anything, mbox_client_txdone() should
> only speed up things.
>

Yes I understand and that's good.

>>> 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)
>>
> mbox_client_txdone() will only be called for commands that has some
> response. Commands that don't have a response would be completed by
> polling.
>

OK, got it

>>> 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.
>>
> I don't see how it could break compliance.
>

While I agree it shouldn't, the firmware guys won't support if we
deviate from the spec. I won't get support for firmware bug fixes in
that case.

Having said that, I don't rule out the usage of TX_BY_ACK, I will need
more time for testing(usually we stress test firmware using Linux for
few of days continuously as we have hit issues after that :)) and
getting things fixed if anything breaks.

>>> 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.
>>
> I am sure you would have copy of that discarded code. Care to share? I
> can't imagine how we handle completions locally could affect the
> remote. The mbox_client_txdone() is untested so I don't rule out bugs,
> otherwise it should only make things better.
>

I tested it with very old firmware almost 4-5 months back. I don't have
the patch on top of this series handy, but will dig it out and give it a
try with latest firmware. I will let you know the results.

For now, I would keep this just polling and unblock others who are
waiting on this series.

Regards,
Sudeep