Re: DeviceTree and children devices

[Felipe Balbi]

On Mon, Oct 24, 2011 at 09:41:24AM +0200, Grant Likely wrote:
> On Mon, Oct 24, 2011 at 09:42:28AM +0300, Felipe Balbi wrote:
> > Hi Grant,
> > 
> > I have a question about how DeviceTree should be written in case a
> > device has a child device.
> > 
> > The way things are integrated on OMAP is that we will always have a
> > parent device which is a wrapper around an IP core in order to
> > integrate with the OMAP context (clocks, power management, etc).
> > 
> > That wrapper has its own address space and its own IRQ number
> > (generally). On my dwc3 driver I have modeled the OMAP wrapper as a
> > parent device which allocates a child device for the core IP driver.
> > This makes it a lot easier to re-use the core IP driver on other SoCs or
> > PCI (there's a glue layer for PCI too).
> > 
> > So I wonder if we should describe that on DeviceTree and not have the
> > OMAP glue layer allocate the core IP driver. Just to illustrate, here's
> > what we have:
> > 
> > static int dwc3_omap_probe(struct platform_device *pdev)
> > {
> > 	struct platform_device	*dwc3;
> > 	struct resource		res[2];
> > 
> > 	dwc3 = platform_device_alloc("dwc3", -1);
> > 	/* check*/
> > 
> > 	dwc3->dev.parent = &pdev->dev;
> > 
> > 	/* copy DMA fields from parent too */
> > 
> > 	res[0].start = start_address;
> > 	res[0].end = end_address;
> > 	res[0].flags = IORESOURCE_MEM;
> > 
> > 	res[1].start = irq_number;
> > 	res[1].flags = IORESOURCE_IRQ;
> > 
> > 	ret = platform_add_resources(dwc3, res, ARRAY_SIZE(res));
> > 	/* check */
> > 
> > 	return platform_add_device(dwc3);
> > }
> > 
> > and I wonder if I should have a DeviceTree like so:
> > 
> > usb@xxxxx {
> > 	compatible = "ti,dwc3-omap";		// This is TI OMAP
> > 						// wrapper
> > 	range = <....>;
> > 
> > 	...
> > 
> > 	usb@yyyy {
> > 		compatible = "synopsys,dwc3",	// This is core IP
> > 						// inside wrapper
> > 
> > 		...
> > 	};
> > };
> > 
> > then I can drop the dwc3 platform_device allocation and all of that
> > resource copying, etc.
> > 
> > What do you think ?
> 
> Looks reasonable to me.  of_platform_populate() should be able to
> handle the device generation for you here.

Ok cool I looking into that and it handles everything I need. There are
only three issues which I see:

a) it hardcoded DMA mask to 32-bit. Right ?
b) it's not using dma_set_coherent_mask()
c) in case parent is a valid pointer, shouldn't it copy DMA mask from
	parent ?

I mean (doesn't solve (a) above):

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index ed5a6d3..172d4a9 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -204,7 +204,12 @@ struct platform_device *of_platform_device_create_pdata(
 #if defined(CONFIG_MICROBLAZE)
 	dev->archdata.dma_mask = 0xffffffffUL;
 #endif
-	dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+
+	if (parent)
+		dma_set_coherent_mask(&dev->dev, parent->coherent_dma_mask);
+	else
+		dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));
+
 	dev->dev.bus = &platform_bus_type;
 	dev->dev.platform_data = platform_data;
 

-- 
balbi

Attachment: signature.asc
Description: Digital signature