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

["Jason-JH Lin (林睿祥)" <Jason-JH.Lin@mediatek.com>]

Hi Fei,

Thanks for the reviews.

On Mon, 2023-10-23 at 18:47 +0800, Fei Shao wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  Hi Jason,
> 
> On Mon, Oct 23, 2023 at 12:39 PM Jason-JH.Lin <
> jason-jh.lin@mediatek.com> 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@mediatek.com>
> > ---
> >  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()")
> 
OK, because want to get the pa of GCE by resource like this:
gce_sec_plat.base_pa = res->start;

I'll find another way to do that.

> > +       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?

Because I want to pass the hwid to secure world and let it know what
GCE pa should use in TEE rather than passing GCE pa.

> 
> > +
> >                 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_eve
> nt) == 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.
> 
That means we have to include <mtk-cmdq-sec-mailbox.h> in this file,
but actually mtk-cmdq-sec-mailbox.h have to include <mtk-cmdq-
mailbox.h>. So I add this here finally.

But I agree with you, I'll try to move this struct into mtk-cmdq-sec-
mailbox.h and include <mtk-cmdq-sec-mailbox.h> in mtk-cmdq-mailbox.c.

Regards,
Jason-JH.Lin

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