Messages in this thread | | | Date | Thu, 9 Jun 2016 13:47:10 -0700 | Subject | Re: [PATCH v3] mailbox: pcc: Support HW-Reduced Communication Subspace type 2 | From | Hoan Tran <> |
| |
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.
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 >> >>
| |