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

Ashwin Chaugule
2016-05-13T17:23:36Z

On 11 May 2016 at 14:15, Hoan Tran <hotran@apm.com> wrote:
> On Wed, May 11, 2016 at 4:57 AM, Ashwin Chaugule
> <ashwin.chaugule@linaro.org> wrote:
>> On 11 May 2016 at 00:21, Hoan Tran <hotran@apm.com> wrote:
>>> On Tue, May 10, 2016 at 5:00 AM, Ashwin Chaugule
>>>> On 6 May 2016 at 14:38, Hoan Tran <hotran@apm.com> wrote:
>>>>> From: hotran <hotran@apm.com>
>>>>>
>>>>> 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@apm.com>
>>>>> ---
>>>>> 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.