Re: [PATCH 1/2] remoteproc: qcom_q6v5_mss: Populate additional devices from DT

[Stephan Gerhold <stephan@gerhold.net>]

On Tue, Nov 30, 2021 at 09:50:06PM -0600, Bjorn Andersson wrote:
> On Mon 29 Nov 07:29 CST 2021, Stephan Gerhold wrote:
> 
> > Some devices without own memory resources could be placed anywhere in the
> > device tree but they logically belong to the modem remote processor. Make
> > it possible to probe them when defined under the mpss device tree node
> > by calling of_platform_populate().
> > 
> 
> This seems reasonable, but other "child devices" of the remoteproc
> follows the state of the remoteproc instance. So I'm worried that this
> will create an inconsistency in that assumption.
> 

On Linux there are two devices managed by qcom_q6v5_mss, the platform
device used by the driver and the actual remoteproc instance that has
the state. The devices that follow the state are children of the
remoteproc instance. I'm creating BAM-DMUX below the platform device.

This is just not represented in the device tree very well since both
platform device and remoteproc instance share the same device tree node.
However, all the state managed nodes are manually managed and do not
have a compatible so it should not cause problems.

> > This can be used for BAM-DMUX for example, which provides the WWAN network
> > interfaces on some older Qualcomm SoCs such as MSM8916 or MSM8974.
> > 
> 
> Is there a technical reason for placing the BAM-DMUX within the modem
> remoteproc node? Can we simply move it to / ?
> 

I had it at / originally and at the moment it would still work there.
I moved it below the remoteproc when the whole "WWAN subsystem"
discussion came up. The goal is to have a common parent device for all
the ports of a WWAN modem.

The way it works is that you give the WWAN subsystem a (parent) device
that represents the modem itself, and then it groups all WWAN ports
below that. The control ports (rpmsg_wwan_ctrl driver) are currently
already created below the modem platform device.

The bam-dmux driver does not integrate with the WWAN subsystem yet
because of some unrelated open problems, but in the future I will likely
need a reference to the modem platform device there as well.

There are other approaches of course (e.g. a phandle to the modem
remoteproc node), but I still think it logically fits best somewhere
below the modem node. If the modem remoteproc is status = "disabled",
it does not make sense to load bam-dmux either etc.

Thanks,
Stephan

> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >  drivers/remoteproc/qcom_q6v5_mss.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > index 43ea8455546c..69f3d1ebf1f1 100644
> > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > @@ -1989,8 +1989,14 @@ static int q6v5_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto remove_sysmon_subdev;
> >  
> > +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> > +	if (ret)
> > +		goto remove_rproc;
> > +
> >  	return 0;
> >  
> > +remove_rproc:
> > +	rproc_del(rproc);
> >  remove_sysmon_subdev:
> >  	qcom_remove_sysmon_subdev(qproc->sysmon);
> >  remove_subdevs:
> > @@ -2010,6 +2016,7 @@ static int q6v5_remove(struct platform_device *pdev)
> >  	struct q6v5 *qproc = platform_get_drvdata(pdev);
> >  	struct rproc *rproc = qproc->rproc;
> >  
> > +	of_platform_depopulate(&pdev->dev);
> >  	rproc_del(rproc);
> >  
> >  	qcom_q6v5_deinit(&qproc->q6v5);
> > -- 
> > 2.34.1
> >