Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2

[Hoan Tran <hotran@apm.com>]

Hi Prashanth,

On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
<pprakash@codeaurora.org> wrote:
>
>
> On 6/9/2016 2:47 PM, Hoan Tran wrote:
>> Hi Ashwin and Prashanth,
>>
>> On Wed, Jun 8, 2016 at 5:41 PM, Hoan Tran <hotran@apm.com> wrote:
>>> Hi Prashanth,
>>>
>>>
>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
>>> <pprakash@codeaurora.org> wrote:
>>>>
>>>> On 6/8/2016 10:24 AM, Hoan Tran wrote:
>>>>> Hi Ashwin,
>>>>>
>>>>> On Wed, Jun 8, 2016 at 5:18 AM, Ashwin Chaugule
>>>>> <ashwin.chaugule@linaro.org> wrote:
>>>>>> + Prashanth (Can you please have a look as well?)
>>>>>>
>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@apm.com> wrote:
>>>>>>> Hi Ashwin,
>>>>>> Hi,
>>>>>>
>>>>>> Sorry about the delay. I'm in the middle of switching jobs and
>>>>>> locations, so its been a bit crazy lately.
>>>>> It's ok and hope you're doing well.
>>>>>
>>>>>> I dont have any major
>>>>>> concerns with this code, although there could be subtle issues with
>>>>>> this IRQ thing. In this patchset, your intent is to add support for
>>>>>> PCC subspace type 2. But you're also adding support for tx command
>>>>>> completion which is not specific to Type 2. We could support that even
>>>>>> in Type 1. Hence I wanted to separate the two, not just for review,
>>>>>> but also the async IRQ completion has subtle issues esp. in the case
>>>>>> of async platform notification, where you could have a PCC client in
>>>>>> the OS writing to the cmd bit and the platform sending an async
>>>>>> notification by writing to some bits in the same 8byte address as the
>>>>>> cmd bit. So we need some mutual exclusivity there, otherwise the OS
>>>>>> and platform could step on each other. Perhaps Prashanth has better
>>>>>> insight into this.
>>>>> I think, this mutual exclusivity could be in another patch.
>>>> Ashwin,
>>>> Sorry, I am not sure how we can prevent platform and OSPM from stepping on
>>>> each other.  There is a line is spec that says "all operations on status field
>>>> must be made using interlocked operations", but not sure what these
>>>> interlocked operation translates to.
>>> Yes, I had the same question about how to prevent it.
>> For platform notification, if the hardware doesn't support interlocked
>> operations. I think we can use a workaround that, platform triggers
>> interrupt to OSPM without touching status field. The OSPM PCC client
>> will decide what to do with this interrupt. For example, OSPM sends a
>> consumer command to check it.
> How do we decide which platform can support this interlocked operation?
> and how do we decide between a completion notification and platform
> notification?

Truly, we should follow the specification. But I don't know if there's
any hardware support this interlocked operation.
For the decide between a completion notification and platform notification
 - Completion notification: Bit "Command Complete" is set.
 - Platform notification: Bit "Command Complete" is not set.

>
> I think the ACPI spec on platform notification is quite ambiguous and it is
> best to get the necessary clarification and/or correction before implementing
> anything related to platform notification.

Agreed, a clarification inside ACPI Specification is needed

Thanks
Hoan

>
> With respect to to this patch, since we are not doing anything specific to
> platform notification and the interrupt can be used only for notification
> of  completion, I suppose we should be okay.
>
> Thanks,
> Prashanth
>> Thanks
>> Hoan
>>
>>>> Hoan,
>>>> Even if we are not using platform notification, we still need to clear the doorbell
>>>> interrupt bit in the PCC interrupt handler (Section14.2.2 and 14.4).  I didn't see
>>>> clearing the doorbell interrupt bit in this patch (and platform is supposed to set
>>>> it again when it is sending the interrupt again). Did I miss it? or is it intentionally
>>>> left out to avoid the race that Ashwin mentioned above?
>>>>
>>> The PCC client driver is supposed to do that. Which mean, the
>>> mbox_chan_received_data() function should clear it.
>>>
>>> Thanks
>>> Hoan
>>>
>>>> Thanks,
>>>> Prashanth
>>>>
>>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>