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
