* DeviceTree and children devices
@ 2011-10-24 6:42 Felipe Balbi
2011-10-24 7:41 ` Grant Likely
0 siblings, 1 reply; 6+ messages in thread
From: Felipe Balbi @ 2011-10-24 6:42 UTC (permalink / raw)
To: Felipe Balbi
Cc: Grant Likely, Linux Kernel Mailing List, Linux USB Mailing List
[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]
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 ?
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: DeviceTree and children devices
2011-10-24 6:42 DeviceTree and children devices Felipe Balbi
@ 2011-10-24 7:41 ` Grant Likely
2011-10-24 7:49 ` Felipe Balbi
0 siblings, 1 reply; 6+ messages in thread
From: Grant Likely @ 2011-10-24 7:41 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Linux Kernel Mailing List, Linux USB Mailing List
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.
g.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: DeviceTree and children devices
2011-10-24 7:41 ` Grant Likely
@ 2011-10-24 7:49 ` Felipe Balbi
2011-10-24 7:58 ` Grant Likely
0 siblings, 1 reply; 6+ messages in thread
From: Felipe Balbi @ 2011-10-24 7:49 UTC (permalink / raw)
To: Grant Likely
Cc: Felipe Balbi, Linux Kernel Mailing List, Linux USB Mailing List
[-- Attachment #1: Type: text/plain, Size: 3152 bytes --]
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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: DeviceTree and children devices
2011-10-24 7:49 ` Felipe Balbi
@ 2011-10-24 7:58 ` Grant Likely
2011-10-24 8:12 ` Felipe Balbi
0 siblings, 1 reply; 6+ messages in thread
From: Grant Likely @ 2011-10-24 7:58 UTC (permalink / raw)
To: balbi; +Cc: Linux Kernel Mailing List, Linux USB Mailing List
On Mon, Oct 24, 2011 at 9:49 AM, Felipe Balbi <balbi@ti.com> wrote:
> 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:
>> > 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));
> +
Right, this does need to be fixed. The existing code just matched
what the historical powerpc code did, but it is certainly not correct.
g.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: DeviceTree and children devices
2011-10-24 7:58 ` Grant Likely
@ 2011-10-24 8:12 ` Felipe Balbi
2011-10-24 8:23 ` Grant Likely
0 siblings, 1 reply; 6+ messages in thread
From: Felipe Balbi @ 2011-10-24 8:12 UTC (permalink / raw)
To: Grant Likely; +Cc: balbi, Linux Kernel Mailing List, Linux USB Mailing List
[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]
On Mon, Oct 24, 2011 at 09:58:39AM +0200, Grant Likely wrote:
> On Mon, Oct 24, 2011 at 9:49 AM, Felipe Balbi <balbi@ti.com> wrote:
> > 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:
> >> > 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));
> > +
>
> Right, this does need to be fixed. The existing code just matched
> what the historical powerpc code did, but it is certainly not correct.
should I send patch above correctly ? Or do you want to also solve
32-bit coherent mask altogether ? What are your plans for that ? Add a
separate property to pass coherent_mask size (32-bit, 64-bit, etc) ?
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: DeviceTree and children devices
2011-10-24 8:12 ` Felipe Balbi
@ 2011-10-24 8:23 ` Grant Likely
0 siblings, 0 replies; 6+ messages in thread
From: Grant Likely @ 2011-10-24 8:23 UTC (permalink / raw)
To: balbi; +Cc: Linux Kernel Mailing List, Linux USB Mailing List
On Mon, Oct 24, 2011 at 10:12 AM, Felipe Balbi <balbi@ti.com> wrote:
> On Mon, Oct 24, 2011 at 09:58:39AM +0200, Grant Likely wrote:
>> On Mon, Oct 24, 2011 at 9:49 AM, Felipe Balbi <balbi@ti.com> wrote:
>> > 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));
>> > +
>>
>> Right, this does need to be fixed. The existing code just matched
>> what the historical powerpc code did, but it is certainly not correct.
>
> should I send patch above correctly ? Or do you want to also solve
> 32-bit coherent mask altogether ? What are your plans for that ? Add a
> separate property to pass coherent_mask size (32-bit, 64-bit, etc) ?
i don't know. I'm not the expert on how the coherent mask should be
set. Your patch does look sane as a starting point, but it bears
looking at by someone more cluefull than me. In particular, someone
should investigate if the dma mask can be calculated from a dma-ranges
property.
g.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-10-24 8:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-24 6:42 DeviceTree and children devices Felipe Balbi
2011-10-24 7:41 ` Grant Likely
2011-10-24 7:49 ` Felipe Balbi
2011-10-24 7:58 ` Grant Likely
2011-10-24 8:12 ` Felipe Balbi
2011-10-24 8:23 ` Grant Likely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®