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

[Ashwin Chaugule]

Hello,

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(-)
>
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 043828d..58c9a67 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -59,6 +59,7 @@
>  #include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/init.h>
> +#include <linux/interrupt.h>
>  #include <linux/list.h>
>  #include <linux/platform_device.h>
>  #include <linux/mailbox_controller.h>
> @@ -68,27 +69,179 @@
>  #include "mailbox.h"
>
>  #define MAX_PCC_SUBSPACES      256
> +#define MBOX_IRQ_NAME          "pcc-mbox"
>
> -static struct mbox_chan *pcc_mbox_channels;
> +/**
> + * PCC mailbox channel information
> + *
> + * @chan:      Pointer to mailbox communication channel
> + * @pcc_doorbell_vaddr: PCC doorbell register address
> + * @pcc_doorbell_ack_vaddr: PCC doorbell ack register address
> + * @irq:       Interrupt number of the channel
> + */
> +struct pcc_mbox_chan {
> +       struct mbox_chan        *chan;
> +       void __iomem            *pcc_doorbell_vaddr;
> +       void __iomem            *pcc_doorbell_ack_vaddr;
> +       int                     irq;
> +};
>
> -/* Array of cached virtual address for doorbell registers */
> -static void __iomem **pcc_doorbell_vaddr;
> +/**
> + * PCC mailbox controller data
> + *
> + * @mb_ctrl:   Representation of the communication channel controller
> + * @mbox_chan: Array of PCC mailbox channels of the controller
> + * @chans:     Array of mailbox communication channels
> + */
> +struct pcc_mbox {
> +       struct mbox_controller  mbox_ctrl;
> +       struct pcc_mbox_chan    *mbox_chans;
> +       struct mbox_chan        *chans;
> +};
> +
> +static struct pcc_mbox pcc_mbox_ctx = {};

Im missing the idea behind creating this structure. Are you
registering multiple controllers somewhere?


[...]

>
> @@ -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.

>
>                 /* If doorbell is in system memory cache the virt address */
>                 pcct_ss = (struct acpi_pcct_hw_reduced *)pcct_entry;
>                 db_reg = &pcct_ss->doorbell_register;
> -               if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
> -                       pcc_doorbell_vaddr[i] = acpi_os_ioremap(db_reg->address,
> +               if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> +                       pcc_mbox_ctx.mbox_chans[i].pcc_doorbell_vaddr =
> +                                       acpi_os_ioremap(db_reg->address,
>                                                         db_reg->bit_width/8);
> +               }
> +
>                 pcct_entry = (struct acpi_subtable_header *)
>                         ((unsigned long) pcct_entry + pcct_entry->length);
>         }
>
> -       pcc_mbox_ctrl.num_chans = count;
> +       pcc_mbox_ctx.mbox_ctrl.num_chans = count;
>
> -       pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
> +       pr_info("Detected %d PCC Subspaces\n",
> +               pcc_mbox_ctx.mbox_ctrl.num_chans);
>
>         return 0;
>  }
> @@ -394,12 +591,12 @@ static int pcc_mbox_probe(struct platform_device *pdev)
>  {
>         int ret = 0;
>
> -       pcc_mbox_ctrl.chans = pcc_mbox_channels;
> -       pcc_mbox_ctrl.ops = &pcc_chan_ops;
> -       pcc_mbox_ctrl.dev = &pdev->dev;
> +       pcc_mbox_ctx.mbox_ctrl.chans = pcc_mbox_ctx.chans;
> +       pcc_mbox_ctx.mbox_ctrl.ops = &pcc_chan_ops;
> +       pcc_mbox_ctx.mbox_ctrl.dev = &pdev->dev;
>
>         pr_info("Registering PCC driver as Mailbox controller\n");
> -       ret = mbox_controller_register(&pcc_mbox_ctrl);
> +       ret = mbox_controller_register(&pcc_mbox_ctx.mbox_ctrl);

That pcc_mbox_ctx usage everywhere seems questionable to me. But its
also too early in the morning here. ;)

Regards,
Ashwin.