by Mathieu Poirier

[permalink] [raw]

Subject: Re: [PATCH v9 05/11] remoteproc: mediatek: Extract remoteproc initialization flow

On Wed, Apr 19, 2023 at 03:38:14AM +0000, TingHan Shen (沈廷翰) wrote:
> Hi Mathieu,
>
> On Fri, 2023-03-31 at 11:44 -0600, Mathieu Poirier wrote:
> > External email : Please do not click links or open attachments until you have verified the sender or the content.
> >
> >
> > On Tue, Mar 28, 2023 at 10:27:27AM +0800, Tinghan Shen wrote:
> > > This is the preparation for probing multi-core SCP. The remoteproc
> > > initialization flow is similar on cores and is reused to avoid
> > > redundant code.
> > >
> > > The registers of config and l1tcm are shared for multi-core
> > > SCP. Reuse the mapped addresses for all cores.
> > >
> > > Signed-off-by: Tinghan Shen <[email protected]>
> > > Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
> > > ---
> > > drivers/remoteproc/mtk_scp.c | 64 +++++++++++++++++++++++++-----------
> > > 1 file changed, 45 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> > > index a3b9bc158cd9..32ecd1450c6f 100644
> > > --- a/drivers/remoteproc/mtk_scp.c
> > > +++ b/drivers/remoteproc/mtk_scp.c
> > > @@ -23,6 +23,13 @@
> > > #define MAX_CODE_SIZE 0x500000
> > > #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
> > >
> > > +struct mtk_scp_of_regs {
> > > + void __iomem *reg_base;
> > > + void __iomem *l1tcm_base;
> > > + size_t l1tcm_size;
> > > + phys_addr_t l1tcm_phys;
> > > +};
> > > +
> >
> > This should represent the cluster with a list of mtk_scp instead of @cluster_cores as
> > introduced in the next patch.
>
> If I'm understanding you correctly, you're suggesting that @cluster_cores should be included
> as a member of this structure. Is that correct?

Correct. Than this structure is allocated in probe() and added as driver data
for the platform device. Its name should also be something like
mtk_scp_cluster or something like that. I suggest you look at what has been
done in ti_k3_r5_remoteproc.c, your end design should be quite similar to that.
In fact you are close but a few things need to be addressed.

>
> Best regards,
> TingHan
>
> >
> > > /**
> > > * scp_get() - get a reference to SCP.
> > > *
> > > @@ -855,7 +862,8 @@ static void scp_remove_rpmsg_subdev(struct mtk_scp *scp)
> > > }
> > > }
> > >
> > > -static int scp_probe(struct platform_device *pdev)
> > > +static int scp_rproc_init(struct platform_device *pdev,
> > > + struct mtk_scp_of_regs *of_regs)
> > > {
> > > struct device *dev = &pdev->dev;
> > > struct device_node *np = dev->of_node;
> > > @@ -879,6 +887,11 @@ static int scp_probe(struct platform_device *pdev)
> > > scp->data = of_device_get_match_data(dev);
> > > platform_set_drvdata(pdev, scp);
> > >
> > > + scp->reg_base = of_regs->reg_base;
> > > + scp->l1tcm_base = of_regs->l1tcm_base;
> > > + scp->l1tcm_size = of_regs->l1tcm_size;
> > > + scp->l1tcm_phys = of_regs->l1tcm_phys;
> > > +
> > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
> > > scp->sram_base = devm_ioremap_resource(dev, res);
> > > if (IS_ERR(scp->sram_base))
> > > @@ -888,24 +901,6 @@ static int scp_probe(struct platform_device *pdev)
> > > scp->sram_size = resource_size(res);
> > > scp->sram_phys = res->start;
> > >
> > > - /* l1tcm is an optional memory region */
> > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm");
> > > - scp->l1tcm_base = devm_ioremap_resource(dev, res);
> > > - if (IS_ERR(scp->l1tcm_base)) {
> > > - ret = PTR_ERR(scp->l1tcm_base);
> > > - if (ret != -EINVAL) {
> > > - return dev_err_probe(dev, ret, "Failed to map l1tcm memory\n");
> > > - }
> > > - } else {
> >
> > scp->l1tcm_base = NULL;
> >
> > > - scp->l1tcm_size = resource_size(res);
> > > - scp->l1tcm_phys = res->start;
> > > - }
> > > -
> > > - scp->reg_base = devm_platform_ioremap_resource_byname(pdev, "cfg");
> > > - if (IS_ERR(scp->reg_base))
> > > - return dev_err_probe(dev, PTR_ERR(scp->reg_base),
> > > - "Failed to parse and map cfg memory\n");
> > > -
> > > ret = scp->data->scp_clk_get(scp);
> > > if (ret)
> > > return ret;
> > > @@ -957,6 +952,37 @@ static int scp_probe(struct platform_device *pdev)
> > > return ret;
> > > }
> > >
> > > +static int scp_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + struct mtk_scp_of_regs scp_regs;
> > > + struct resource *res;
> > > + int ret;
> > > +
> > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cfg");
> > > + scp_regs.reg_base = devm_ioremap_resource(dev, res);
> > > + if (IS_ERR(scp_regs.reg_base))
> > > + return dev_err_probe(dev, PTR_ERR(scp_regs.reg_base),
> > > + "Failed to parse and map cfg memory\n");
> > > +
> > > + /* l1tcm is an optional memory region */
> > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "l1tcm");
> > > + scp_regs.l1tcm_base = devm_ioremap_resource(dev, res);
> > > + if (IS_ERR(scp_regs.l1tcm_base)) {
> > > + ret = PTR_ERR(scp_regs.l1tcm_base);
> > > + if (ret != -EINVAL)
> > > + return dev_err_probe(dev, ret, "Failed to map l1tcm memory\n");
> > > +
> > > + scp_regs.l1tcm_size = 0;
> > > + scp_regs.l1tcm_phys = 0;
> > > + } else {
> > > + scp_regs.l1tcm_size = resource_size(res);
> > > + scp_regs.l1tcm_phys = res->start;
> > > + }
> > > +
> > > + return scp_rproc_init(pdev, &scp_regs);
> > > +}
> > > +
> > > static int scp_remove(struct platform_device *pdev)
> > > {
> > > struct mtk_scp *scp = platform_get_drvdata(pdev);
> > > --
> > > 2.18.0
> > >
>
> --
> Best regards,
> TingHan