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 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@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
