Re: [PATCH v13 05/11] remoteproc: mediatek: Introduce cluster on single-core SCP

[AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>]

Il 07/06/23 10:00, TingHan Shen (沈廷翰) ha scritto:
> Hi Angelo,
>
> On Wed, 2023-06-07 at 09:43 +0200, AngeloGioacchino Del Regno wrote:
> > Il 07/06/23 09:22, Tinghan Shen ha scritto:
> > > This is the preliminary step for probing multi-core SCP.
> > > The initialization procedure for remoteproc is similar for both
> > > single-core and multi-core architectures and is reusing to avoid
> > > redundant code.
> > >
> > > Rewrite the probing flow of single-core SCP to adapt with the 'cluster'
> > > concept needed by probing the multi-core SCP. The main differences
> > > are,
> > > - the SCP core object(s) is maintained at the cluster list instead of at
> > >     the platofmr device driver data property.
> >
> > s/platofmr/platform/g
> >
> > > - save the cluster information at the platofmr device driver data property.
> > > - In order to keep the compatibility of exported SCP APIs which getting
> > >     the SCP core object by SCP node phandle, move the SCP core object
> > >     pointers to the platform device platform data property.
> > >
> > > The registers of config and l1tcm are shared for multi-core
> > > SCP. Reuse the mapped addresses for all cores.
> > >
> > > Signed-off-by: Tinghan Shen <tinghan.shen@xxxxxxxxxxxx>
> > > ---
> > >    drivers/remoteproc/mtk_common.h |   2 +
> > >    drivers/remoteproc/mtk_scp.c    | 151 +++++++++++++++++++++++---------
> > >    2 files changed, 112 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/mtk_common.h b/drivers/remoteproc/mtk_common.h
> > > index c0905aec3b4b..56395e8664cb 100644
> > > --- a/drivers/remoteproc/mtk_common.h
> > > +++ b/drivers/remoteproc/mtk_common.h
> > > @@ -128,6 +128,8 @@ struct mtk_scp {
> > >    	size_t dram_size;
> > >    
> > >    	struct rproc_subdev *rpmsg_subdev;
> > > +
> > > +	struct list_head elem;
> > >    };
> > >    
> > >    /**
> > > diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> > > index d66822dad943..c8fc6b46f82b 100644
> > > --- a/drivers/remoteproc/mtk_scp.c
> > > +++ b/drivers/remoteproc/mtk_scp.c
> > > @@ -23,6 +23,14 @@
> > >    #define MAX_CODE_SIZE 0x500000
> > >    #define SECTION_NAME_IPI_BUFFER ".ipi_buffer"
> > >    
> > > +struct mtk_scp_of_cluster {
> > > +	void __iomem *reg_base;
> > > +	void __iomem *l1tcm_base;
> > > +	size_t l1tcm_size;
> > > +	phys_addr_t l1tcm_phys;
> > > +	struct list_head mtk_scp_list;
> > > +};
> > > +
> > >    /**
> > >     * scp_get() - get a reference to SCP.
> > >     *
> > > @@ -51,7 +59,7 @@ struct mtk_scp *scp_get(struct platform_device *pdev)
> > >    		return NULL;
> > >    	}
> > >    
> > > -	return platform_get_drvdata(scp_pdev);
> > > +	return *(struct mtk_scp **)dev_get_platdata(&scp_pdev->dev);
> > >    }
> > >    EXPORT_SYMBOL_GPL(scp_get);
> > >    
> > > @@ -810,14 +818,14 @@ static void scp_unmap_memory_region(struct mtk_scp *scp)
> > >    static int scp_register_ipi(struct platform_device *pdev, u32 id,
> > >    			    ipi_handler_t handler, void *priv)
> > >    {
> > > -	struct mtk_scp *scp = platform_get_drvdata(pdev);
> > > +	struct mtk_scp *scp = *(struct mtk_scp **)dev_get_platdata(&pdev->dev);
> > >    
> > >    	return scp_ipi_register(scp, id, handler, priv);
> > >    }
> > >    
> > >    static void scp_unregister_ipi(struct platform_device *pdev, u32 id)
> > >    {
> > > -	struct mtk_scp *scp = platform_get_drvdata(pdev);
> > > +	struct mtk_scp *scp = *(struct mtk_scp **)dev_get_platdata(&pdev->dev);
> > >    
> > >    	scp_ipi_unregister(scp, id);
> > >    }
> > > @@ -825,7 +833,7 @@ static void scp_unregister_ipi(struct platform_device *pdev, u32 id)
> > >    static int scp_send_ipi(struct platform_device *pdev, u32 id, void *buf,
> > >    			unsigned int len, unsigned int wait)
> > >    {
> > > -	struct mtk_scp *scp = platform_get_drvdata(pdev);
> > > +	struct mtk_scp *scp = *(struct mtk_scp **)dev_get_platdata(&pdev->dev);
> > >    
> > >    	return scp_ipi_send(scp, id, buf, len, wait);
> > >    }
> > > @@ -855,7 +863,8 @@ static void scp_remove_rpmsg_subdev(struct mtk_scp *scp)
> > >    	}
> > >    }
> > >    
> > > -static int scp_probe(struct platform_device *pdev)
> > > +static struct mtk_scp *scp_rproc_init(struct platform_device *pdev,
> > > +				      struct mtk_scp_of_cluster *scp_cluster)
> > >    {
> > >    	struct device *dev = &pdev->dev;
> > >    	struct device_node *np = dev->of_node;
> > > @@ -867,52 +876,42 @@ static int scp_probe(struct platform_device *pdev)
> > >    
> > >    	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
> > >    	if (ret < 0 && ret != -EINVAL)
> > > -		return ret;
> > > +		return ERR_PTR(ret);
> > >    
> > >    	rproc = devm_rproc_alloc(dev, np->name, &scp_ops, fw_name, sizeof(*scp));
> > > -	if (!rproc)
> > > -		return dev_err_probe(dev, -ENOMEM, "unable to allocate remoteproc\n");
> > > +	if (!rproc) {
> > > +		dev_err(dev, "unable to allocate remoteproc\n");
> > > +		return ERR_PTR(-ENOMEM);
> >
> > Why are you converting those dev_err_probe to dev_err->return?!
> >
> > Regards,
> > Angelo
>
> It's because the return type of this function changed from "int" to "struct mtk_scp *".
> Maybe I should change them to "ERR_PTR(dev_err_probe(...))" ?

Oh, sorry - you're right. No, it's fine as you did it.

Cheers