Re: [PATCH v2 26/34] staging: vchiq_arm: Set up dma ranges on child devices

From: Jacopo Mondi <jacopo@jmondi.org>
To: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>, phil@raspberrypi.com
Cc: "Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
	linux-media@vger.kernel.org,
	"Kieran Bingham" <kieran.bingham@ideasonboard.com>,
	"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
	"Naushir Patuck" <naush@raspberrypi.com>,
	"Dave Stevenson" <dave.stevenson@raspberrypi.com>,
	"Dave Stevenson" <dave.stevenson@raspberrypi.org>
Subject: Re: [PATCH v2 26/34] staging: vchiq_arm: Set up dma ranges on child devices
Date: Tue, 25 Aug 2020 18:57:53 +0200	[thread overview]
Message-ID: <20200825165753.rfaisv7mvxf4glg5@uno.localdomain> (raw)
In-Reply-To: <078d57964f47ea29af3b8ab0af65c244a158e9b1.camel@suse.de>

[-- Attachment #1: Type: text/plain, Size: 3011 bytes --]

Hi Nicolas, Phil,
    I'm in the process of sending a v2 of this series, trying to
reduce the patch count by only picking what's required to support
the ISP.

This patch and [30/34] puzzles me a bit, so I'll ask a few questions.

On Mon, May 04, 2020 at 06:54:32PM +0200, Nicolas Saenz Julienne wrote:
> On Mon, 2020-05-04 at 12:26 +0300, Laurent Pinchart wrote:
> > From: Dave Stevenson <dave.stevenson@raspberrypi.org>
> >
> > The VCHIQ driver now loads the audio, camera, codec, and vc-sm
> > drivers as platform drivers. However they were not being given
> > the correct DMA configuration.
> >
> > Call of_dma_configure with the parent (VCHIQ) parameters to be
> > inherited by the child.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c   | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > index c5c7af28c1c8..15ccd624aaab 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -2733,6 +2733,12 @@ vchiq_register_child(struct platform_device *pdev,
> > const char *name)
> >  		child = NULL;
> >  	}
> >
> > +	/*
> > +	 * We want the dma-ranges etc to be copied from the parent VCHIQ device
> > +	 * to be passed on to the children too.
> > +	 */
> > +	of_dma_configure(&new_dev->dev, pdev->dev.of_node, true);
>
> I think you could use struct platform_device_info's of_node_reused. See patch
> 908f6fc3a1405 ('usb: musb: sunxi: propagate devicetree node to glue pdev') for
> an example. AFAIK of_dma_configure() is only to be used in bus code, and there
> has been a huge effort in the past to make sure it says as such. With a proper
> fwnode set of_dma_configure() will be called during the device's probe.

I just tried giving to the children devices the parent's fwnode and set
of_node_reused as done in 908f6fc3a1405 to avoid usage of
of_dma_configure, and now I have the parent being probed 4 times, one per
each children :) I guess that was not the expected result...

What is in your opinions the best course of actions for this and patch
[30/34] ? Can we live with 'of_dma_configure()' ? [1]

I am assuming currently the vchiq children are manually registered as
there are no entries in the DTS for them, so I'm kind of missing the
point of having [30/34] in. What am I missing ?

Thanks
  j

[1] There are a few things that are not clear to me though, possible due
to my rather poor understanding of this core parts. In particular, as
the dma-ranges are assigned to the parent 'soc' node, and I assume
propagated to the children such as the 'vchiq' node, isnt' setting the
'parent' field of each newly created platform device enough to have
the property propagated ?

>
> Regards,
> Nicolas
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]