Hi Jason,
On Mon, Oct 23, 2023 at 12:39 PM Jason-JH.Lin <[email protected]> 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 <[email protected]>
> ---
> 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
>
>
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 <
> [email protected]> 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 <[email protected]>
> > ---
> > 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
> >
> >
>