Re: [PATCH v2] mailbox: pcc: Support HW-Reduced Communication Subspace Type 2

[Ashwin Chaugule]

On 11 May 2016 at 14:15, Hoan Tran <hotran@xxxxxxx> wrote:
> On Wed, May 11, 2016 at 4:57 AM, Ashwin Chaugule
> <ashwin.chaugule@xxxxxxxxxx> wrote:
>> On 11 May 2016 at 00:21, Hoan Tran <hotran@xxxxxxx> wrote:
>>> On Tue, May 10, 2016 at 5:00 AM, Ashwin Chaugule
>>>> On 6 May 2016 at 14:38, Hoan Tran <hotran@xxxxxxx> wrote:
>>>>> From: hotran <hotran@xxxxxxx>
>>>>>
>>>>> ACPI 6.1 has a PCC HW-Reduced Communication Subspace Type 2 intended for
>>>>> use on HW-Reduce ACPI Platform, which requires read-modify-write sequence
>>>>> to acknowledge doorbell interrupt. This patch provides the implementation
>>>>> for the Communication Subspace Type 2.
>>>>>
>>>>> This patch depends on patch [1] which supports PCC subspace type 2 header
>>>>> [1] https://lkml.org/lkml/2016/5/5/14
>>>>>  - [PATCH v2 03/13] ACPICA: ACPI 6.1: Support for new PCCT subtable
>>>>>
>>>>> v2
>>>>>  * Remove changes inside "actbl3.h". This file is taken care by ACPICA.
>>>>>  * Parse both subspace type 1 and subspace type 2
>>>>>  * Remove unnecessary variable initialization
>>>>>  * ISR returns IRQ_NONE in case of error
>>>>>
>>>>> v1
>>>>>  * Initial
>>>>>
>>>>> Signed-off-by: Hoan Tran <hotran@xxxxxxx>
>>>>> ---
>>>>>  drivers/mailbox/pcc.c | 395 +++++++++++++++++++++++++++++++++++++-------------
>>>>>  1 file changed, 296 insertions(+), 99 deletions(-)

[..]

>>>>> @@ -357,24 +534,44 @@ static int __init acpi_pcc_probe(void)
>>>>>         pcct_entry = (struct acpi_subtable_header *) (
>>>>>                 (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
>>>>>
>>>>> +       acpi_pcct_tbl = (struct acpi_table_pcct *) pcct_tbl;
>>>>> +       if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
>>>>> +               pcc_mbox_ctx.mbox_ctrl.txdone_irq = true;
>>>>> +
>>>>>         for (i = 0; i < count; i++) {
>>>>>                 struct acpi_generic_address *db_reg;
>>>>>                 struct acpi_pcct_hw_reduced *pcct_ss;
>>>>> -               pcc_mbox_channels[i].con_priv = pcct_entry;
>>>>> +
>>>>> +               pcc_mbox_ctx.chans[i].con_priv = pcct_entry;
>>>>> +               pcc_mbox_ctx.chans[i].mbox = &pcc_mbox_ctx.mbox_ctrl;
>>>>> +
>>>>> +               pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
>>>>> +
>>>>> +               pcc_mbox_ctx.mbox_chans[i].chan = &pcc_mbox_ctx.chans[i];
>>>>> +               if (pcc_mbox_ctx.mbox_ctrl.txdone_irq) {
>>>>> +                       rc = pcc_parse_subspace_irq(&pcc_mbox_ctx.mbox_chans[i],
>>>>> +                                                   pcct_ss);
>>>>> +                       if (rc < 0)
>>>>> +                               return rc;
>>>>> +               }
>>>>
>>>> I think we should parse the remaining entries and register them if
>>>> they are sane. Some other PCC clients can then continue to function.
>>> I think, it could be an error of PCCT table and need to be returned.
>>>>
>>
>> Well, that could be dealt with a pr_err/warn() for that specific entry
>> ?
> But what happens if the PCC client requests this failed PCC subspace.
> "pcc_parse_subspace_irq" function does "pr_err" for error cases.
>
>> IIRC not all subspaces require IRQ's. Its optional, isnt it?
> Maybe I misunderstood: if "doorbell interrupt" bit of "platform
> communications channel global flags" is set, the platform is capable
> of generating an interrupt to indicate completion of a command. And if
> this bit is set, "doorbell interrupt" and "doorbell interrupt flags"
> fields of each subspace are active
> Could you correct me, thanks
>>

You're right. My concern is addressed by your check for txdone_irq.
Are you able to test this on a system which doesn't have Type 2
support?

Thanks,
Ashwin.