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

[Hoan Tran]

Hi Rafael,

On Mon, Jun 27, 2016 at 2:32 PM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> On Monday, June 27, 2016 11:27:42 AM Hoan Tran wrote:
>> Hi Jassi and Rafael,
>>
>> On Wed, Jun 15, 2016 at 9:19 AM, Prakash, Prashanth
>> <pprakash@xxxxxxxxxxxxxx> wrote:
>> >
>> >
>> > On 6/9/2016 4:43 PM, Hoan Tran wrote:
>> >> Hi Prashanth,
>> >>
>> >> On Thu, Jun 9, 2016 at 3:25 PM, Prakash, Prashanth
>> >> <pprakash@xxxxxxxxxxxxxx> 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@xxxxxxx> wrote:
>> >>>>> Hi Prashanth,
>> >>>>>
>> >>>>>
>> >>>>> On Wed, Jun 8, 2016 at 5:32 PM, Prakash, Prashanth
>> >>>>> <pprakash@xxxxxxxxxxxxxx> 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@xxxxxxxxxx> wrote:
>> >>>>>>>> + Prashanth (Can you please have a look as well?)
>> >>>>>>>>
>> >>>>>>>> On 31 May 2016 at 15:35, Hoan Tran <hotran@xxxxxxx> 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
>> > This patch look good to me, as it doesn't deal with platform notification.
>> > We can try to get some clarification from spec side before handling the platform
>> > notification pieces.
>> >
>> > Reviewed-by: Prashanth Prakash <pprakash@xxxxxxxxxxxxxx>
>>
>> Do you have plan to apply this patch ?
>
> Yes.

Thanks and hope it'll be in 4.8.

Thanks
Hoan

>
> Thanks,
> Rafael
>