Date:	Fri, 31 Jul 2015 10:21:25 +0100
From:	Sudeep Holla <sudeep.holla@....com>
To:	Jassi Brar <jassisinghbrar@...il.com>
CC:	Sudeep Holla <sudeep.holla@....com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Liviu Dudau <Liviu.Dudau@....com>,
	Punit Agrawal <Punit.Agrawal@....com>,
	"Jon Medhurst (Tixy)" <tixy@...aro.org>,
	Lorenzo Pieralisi <Lorenzo.Pieralisi@....com>,
	Arnd Bergmann <arnd@...db.de>, Olof Johansson <olof@...om.net>,
	Kevin Hilman <khilman@...nel.org>
Subject: Re: [PATCH v5 2/8] firmware: add support for ARM System Control and
 Power Interface(SCPI) protocol



On 30/07/15 18:56, Jassi Brar wrote:
> On Wed, Jul 29, 2015 at 6:20 PM, Sudeep Holla <sudeep.holla@....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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists