Re: [PATCH v2 7/9] mailbox: mediatek: Add secure CMDQ driver support for CMDQ driver

[Fei Shao]

Hi Jason,

On Mon, Oct 23, 2023 at 12:39 PM Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx> wrote:
>
> CMDQ driver will probe a secure CMDQ driver when has_sec flag
> in platform data is true and its device node in dts has defined a
> event id of CMDQ_SYNC_TOKEN_SEC_EOF.
>
> Secure CMDQ driver support on mt8188 and mt8195 currently.
> So add a has_sec flag to their driver data to probe it.
>
> Signed-off-by: Jason-JH.Lin <jason-jh.lin@xxxxxxxxxxxx>
> ---
>  drivers/mailbox/mtk-cmdq-mailbox.c       | 42 ++++++++++++++++++++++--
>  include/linux/mailbox/mtk-cmdq-mailbox.h | 11 +++++++
>  2 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c
> index 3bdfb9a60614..4db5eb76f353 100644
> --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> @@ -87,6 +87,7 @@ struct gce_plat {
>         u8 shift;
>         bool control_by_sw;
>         bool sw_ddr_en;
> +       bool has_sec;
>         u32 gce_num;
>  };
>
> @@ -560,14 +561,23 @@ static int cmdq_probe(struct platform_device *pdev)
>         int alias_id = 0;
>         static const char * const clk_name = "gce";
>         static const char * const clk_names[] = { "gce0", "gce1" };
> +       struct resource *res;
> +       struct platform_device *mtk_cmdq_sec;
> +       u32 hwid = 0;
>
>         cmdq = devm_kzalloc(dev, sizeof(*cmdq), GFP_KERNEL);
>         if (!cmdq)
>                 return -ENOMEM;
>
> -       cmdq->base = devm_platform_ioremap_resource(pdev, 0);
> -       if (IS_ERR(cmdq->base))
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
The devm_platform_ioremap_resource() helper was added on purpose [1].
Please stick to it unless you have a strong reason to re-split it.

[1]: a04f30356e75 ("mailbox: mtk-cmdq: Make use of the helper function
devm_platform_ioremap_resource()")

> +       if (!res)
> +               return -EINVAL;
> +
> +       cmdq->base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(cmdq->base)) {
> +               dev_err(dev, "failed to ioremap cmdq\n");
>                 return PTR_ERR(cmdq->base);
> +       }
>
>         cmdq->irq = platform_get_irq(pdev, 0);
>         if (cmdq->irq < 0)
> @@ -585,6 +595,8 @@ static int cmdq_probe(struct platform_device *pdev)
>                 dev, cmdq->base, cmdq->irq);
>
>         if (cmdq->pdata->gce_num > 1) {
> +               hwid = of_alias_get_id(dev->of_node, clk_name);
Why get hwid here while it's only used in the has_sec branch?

> +
>                 for_each_child_of_node(phandle->parent, node) {
>                         alias_id = of_alias_get_id(node, clk_name);
>                         if (alias_id >= 0 && alias_id < cmdq->pdata->gce_num) {
> @@ -653,6 +665,30 @@ static int cmdq_probe(struct platform_device *pdev)
>                 return err;
>         }
>
> +       if (cmdq->pdata->has_sec) {
> +               struct cmdq_sec_plat gce_sec_plat;
> +
> +               if (of_property_read_u32_index(dev->of_node, "mediatek,gce-events", 0,
> +                                              &gce_sec_plat.cmdq_event) == 0) {
> +                       gce_sec_plat.gce_dev = dev;
> +                       gce_sec_plat.base = cmdq->base;
> +                       gce_sec_plat.base_pa = res->start;
> +                       gce_sec_plat.hwid = hwid;
> +                       gce_sec_plat.gce_num = cmdq->pdata->gce_num;
> +                       gce_sec_plat.clocks = cmdq->clocks;
> +                       gce_sec_plat.thread_nr = cmdq->pdata->thread_nr;
> +
> +                       mtk_cmdq_sec = platform_device_register_data(dev, "mtk_cmdq_sec",
> +                                                                    PLATFORM_DEVID_AUTO,
> +                                                                    &gce_sec_plat,
> +                                                                    sizeof(gce_sec_plat));
> +                       if (IS_ERR(mtk_cmdq_sec)) {
> +                               dev_err(dev, "failed to register platform_device mtk_cmdq_sec\n");
> +                               return PTR_ERR(mtk_cmdq_sec);
> +                       }
> +               }
> +       }
> +
>         return 0;
>  }
>
> @@ -693,6 +729,7 @@ static const struct gce_plat gce_plat_v6 = {
>         .thread_nr = 24,
>         .shift = 3,
>         .control_by_sw = true,
> +       .has_sec = true,
>         .gce_num = 2
>  };
>
> @@ -708,6 +745,7 @@ static const struct gce_plat gce_plat_v8 = {
>         .thread_nr = 32,
>         .shift = 3,
>         .control_by_sw = true,
> +       .has_sec = true,
>         .gce_num = 2
>  };
>
> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> index f78a08e7c6ed..fdda995a69ce 100644
> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> @@ -79,6 +79,17 @@ struct cmdq_pkt {
>         bool                    loop;
>  };
>
> +struct cmdq_sec_plat {
> +       struct device *gce_dev;
> +       void __iomem *base;
> +       dma_addr_t base_pa;
> +       u32 hwid;
> +       u32 gce_num;
> +       struct clk_bulk_data *clocks;
> +       u32 thread_nr;
> +       u32 cmdq_event;
> +};
I feel this should be in mtk-cmdq-sec-mailbox.h.

Regards,
Fei



> +
>  u8 cmdq_get_shift_pa(struct mbox_chan *chan);
>
>  #endif /* __MTK_CMDQ_MAILBOX_H__ */
> --
> 2.18.0
>
>