No support of platform device instance id?

No support of platform device instance id?

[Shawn Guo]

Hello all,

I'm looking at function of_device_alloc in drivers/of/platform.c, and
surprisingly found that the platform device instance id is being
hard-coded as -1 when calling platform_device_alloc.  Does this mean
that there is no support of pdev id in DT?  If yes, can anyone please
help me understand why we do not have something in DT node to reflect
this id and get it supported?  Or this is something on TODO list?

I'm asking this because I feel we still need pdev id support in DT when
reviewing the patch set of 'mx51 basic DT support'.

-- 
Regards,
Shawn

Re: No support of platform device instance id?

[Grant Likely]

On Sat, Mar 12, 2011 at 03:00:57PM +0800, Shawn Guo wrote:
> Hello all,
> 
> I'm looking at function of_device_alloc in drivers/of/platform.c, and
> surprisingly found that the platform device instance id is being
> hard-coded as -1 when calling platform_device_alloc.  Does this mean
> that there is no support of pdev id in DT?

Correct, when generating devices from the DT, the core code has no way
of knowing what the 'preferred' number of the device should be.  It
expects that when the driver's .probe() hook is called, the driver is
smart enough to enumerate the automatically.

> If yes, can anyone please
> help me understand why we do not have something in DT node to reflect
> this id and get it supported?  Or this is something on TODO list?

Ideally, you shouldn't be relying on .id at all.  Fix your device
drivers to enumerate correctly, and use explicit phandle references
when specifying connections between devices.

That said, I do understand the issue with needing to reference a
specific device by name; like for specifying the console device.  In
those cases, the correct way to figure out what number a device is
supposed to have is to add a property to the /aliases node (see ePAPR
for a description of /aliases).  However, even in this case the driver
should be looking at the device tree data, and not depend on the value
in pdev->id.

> I'm asking this because I feel we still need pdev id support in DT when
> reviewing the patch set of 'mx51 basic DT support'.
> 
> -- 
> Regards,
> Shawn
> 
> 
> _______________________________________________
> linaro-dev mailing list
> linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev

Re: No support of platform device instance id?

[Shawn Guo]

Hi Grant,

Thanks for the explanation.

On Sat, Mar 12, 2011 at 02:46:20AM -0700, Grant Likely wrote:
> On Sat, Mar 12, 2011 at 03:00:57PM +0800, Shawn Guo wrote:
> > Hello all,
> > 
> > I'm looking at function of_device_alloc in drivers/of/platform.c, and
> > surprisingly found that the platform device instance id is being
> > hard-coded as -1 when calling platform_device_alloc.  Does this mean
> > that there is no support of pdev id in DT?
> 
> Correct, when generating devices from the DT, the core code has no way
> of knowing what the 'preferred' number of the device should be.  It

If we have this number in DT, the core code gets the way.

> expects that when the driver's .probe() hook is called, the driver is
> smart enough to enumerate the automatically.
> 
Considering that many drivers on ARM platform are replying on
pdev->id, we will have to make every single of such drivers become
smart.

> > If yes, can anyone please
> > help me understand why we do not have something in DT node to reflect
> > this id and get it supported?  Or this is something on TODO list?
> 
> Ideally, you shouldn't be relying on .id at all.  Fix your device
> drivers to enumerate correctly, and use explicit phandle references
> when specifying connections between devices.
> 
Again, many existing ARM drivers will need to get fixed.

> That said, I do understand the issue with needing to reference a
> specific device by name; like for specifying the console device.  In
> those cases, the correct way to figure out what number a device is
> supposed to have is to add a property to the /aliases node (see ePAPR

To me, specifying the id in DT, and letting core code get it from DT
and pass it to platform_device_alloc seems more natural and
straight-forward than creating /aliases node and then parsing it in
every single driver that need to figure out the device number.

The most important thing is that the ARM drivers relying on pdev->id
will need no changes on this point, if we have pdev->id supported in
DT core code.

I can live with the approach you tell, but I doubt that ARM community
people will have the same question, when we post the driver changes
for DT support.

> for a description of /aliases).  However, even in this case the driver
> should be looking at the device tree data, and not depend on the value
> in pdev->id.
> 
If we have pdev->id specified in DT and retrieved it by core code,
the driver is actually looking at the data from DT.

-- 
Regards,
Shawn

Re: No support of platform device instance id?

[Grant Likely]

On Sat, Mar 12, 2011 at 10:55:33PM +0800, Shawn Guo wrote:
> Hi Grant,
> 
> Thanks for the explanation.
> 
> On Sat, Mar 12, 2011 at 02:46:20AM -0700, Grant Likely wrote:
> > On Sat, Mar 12, 2011 at 03:00:57PM +0800, Shawn Guo wrote:
> > > Hello all,
> > > 
> > > I'm looking at function of_device_alloc in drivers/of/platform.c, and
> > > surprisingly found that the platform device instance id is being
> > > hard-coded as -1 when calling platform_device_alloc.  Does this mean
> > > that there is no support of pdev id in DT?
> > 
> > Correct, when generating devices from the DT, the core code has no way
> > of knowing what the 'preferred' number of the device should be.  It
> 
> If we have this number in DT, the core code gets the way.
> 
> > expects that when the driver's .probe() hook is called, the driver is
> > smart enough to enumerate the automatically.
> > 
> Considering that many drivers on ARM platform are replying on
> pdev->id, we will have to make every single of such drivers become
> smart.
> 
> > > If yes, can anyone please
> > > help me understand why we do not have something in DT node to reflect
> > > this id and get it supported?  Or this is something on TODO list?
> > 
> > Ideally, you shouldn't be relying on .id at all.  Fix your device
> > drivers to enumerate correctly, and use explicit phandle references
> > when specifying connections between devices.
> > 
> Again, many existing ARM drivers will need to get fixed.

I've got another solution for this (see below)

> 
> > That said, I do understand the issue with needing to reference a
> > specific device by name; like for specifying the console device.  In
> > those cases, the correct way to figure out what number a device is
> > supposed to have is to add a property to the /aliases node (see ePAPR
> 
> To me, specifying the id in DT, and letting core code get it from DT
> and pass it to platform_device_alloc seems more natural and
> straight-forward than creating /aliases node and then parsing it in
> every single driver that need to figure out the device number.

...and you're certainly not the first person to suggest that.  That
was certainly my opinion when I started working with the device tree.
However, that approach incurs a number of both conceptual and
practical problems that are hard to solve.

On the conceptual front; the enumeration of devices is in a lot of
regards an implementation detail of the Linux kernel and is
potentially subject to change, whereas the device tree is an
OS-independent description of the hardware.  The last thing we want to
do in the device tree is to try and reconcile all the different ways
that operating systems want to enumerate devices.  Instead, references
between nodes in the tree use explicit phandle references which
always works for referencing a specific device.  Example: An Ethernet
node is attached to its phy with a phandle instead of "phy 5 on
bus 3".

That said, the argument can be made that on-chip SoC device do have a
natural enumeration which is documented in the device, which is true,
but that leads to practical problems:

Say I have three nodes; one using id 0, one using id 1, and one that
doesn't specify an id.  Now, lets also say that the third device gets
probed first.  What ID should it use?  Should it start at the bottom
and assign #0?  That would make 0 unavailable for the first device.
How would the driver know that it is not supposed to enumerate either
id 0 or id 1?  There are two problems here:

First problem is that any kind of mixed enumeration+assignment sucks.
If there is any kind of assignment, then the enumeration engine has to
jump through hoops to figure out which numbers are eligible to be used
for enumerating.  If the identifier assignments are distributed
throughout the tree, then drivers need to have special knowledge about
which nodes to look in for finding pre-assigned ids.

It's particularly troublesome when multiple drivers share the same
numberspace, like i2c and spi bus numbers.  A system could have many
spi busses, some on-chip, some on the main board, and some on add-in
boards with little to no guarantee about which will be bound to
drivers first.  I'm more interested in seeing code that simply doesn't
care what id gets assigned.  This is completely possible with the
device tree when explicit phandle references are used between device
nodes.

However, it does make sense to have logical names for devices,
particularly for things that have labels on the outside of the box.
(ie: serial0 or eth1).  That kind of thing is not a Linux
implementation detail, and certainly it belongs in the device tree.
The /aliases node was designed for exactly that purpose.  Rather than
distributing the name/id assignments throughout the tree, the aliases
node collects all system wide naming information into a single node.
Any driver in the system can use that node to determine which names
are already assigned without needing to search the entire tree.  I am
absolutely adamant on this point that if you want to assign
system-wide naming/numbering, then the /aliases node is the correct
place to do so.

> 
> The most important thing is that the ARM drivers relying on pdev->id
> will need no changes on this point, if we have pdev->id supported in
> DT core code.

You're not going to be able to get this.  There is just no way that
the core DT code can have the information needed to set the .id
correctly.  However, I think I have another solution.

Several weeks back I posted a patch for of_platform_bus_snoop() which
matches platform_device registrations to nodes in the device tree
instead of allocating and registering a new device.  I've spent some
more time on that patch in the last couple of weeks to the point that
I'm happy with the model and I'm almost ready to push it out to my
devicetree/test branch.  John Bonesio is currently refactoring and
cleaning it up for me so that it can get posted.  You can see the
current state in my devicetree/preregister branch, with tegra modified
to use it.

The model is:

1) Platform code calls of_platform_device_preregister() to tell the DT
   code about the nodes it /intends/ to register as devices.
2) Platform code can register as many or as few platform_devices as it
   likes.  If any of these devices match one of the nodes passed by
   of_platform_device_preregister(), then the DT code will set the
   of_node pointer before it gets bound to a device.
3) Platform code calls of_platform_device_probe() which will
   register platform_devices for any nodes which *did not* get
   assigned to a device in step 2.

I implemented this as a way to allow dt and non-dt use-cases to share
the same SoC setup code so that anything on-chip would get registered
in the same way, but would also get the benefit of being linked to its
device tree node.  For example, to obtain the list of i2c devices or
gpio connections from the tree.  It also helps solve the problem of
matching nodes to clks which are currently matched by name.  I think
it would also solve your use case, at least in the short term.

I should have patches out later this week.

g.

Re: No support of platform device instance id?

[Shawn Guo]

On Mon, Mar 14, 2011 at 03:10:19PM -0600, Grant Likely wrote:
> 
[...]
> Several weeks back I posted a patch for of_platform_bus_snoop() which
> matches platform_device registrations to nodes in the device tree
> instead of allocating and registering a new device.  I've spent some
> more time on that patch in the last couple of weeks to the point that
> I'm happy with the model and I'm almost ready to push it out to my
> devicetree/test branch.  John Bonesio is currently refactoring and

I have seen it on devicetree/test branch.

> cleaning it up for me so that it can get posted.  You can see the
> current state in my devicetree/preregister branch, with tegra modified
> to use it.
> 
> The model is:
> 
> 1) Platform code calls of_platform_device_preregister() to tell the DT
>    code about the nodes it /intends/ to register as devices.
> 2) Platform code can register as many or as few platform_devices as it
>    likes.  If any of these devices match one of the nodes passed by
>    of_platform_device_preregister(), then the DT code will set the
>    of_node pointer before it gets bound to a device.
> 3) Platform code calls of_platform_device_probe() which will
>    register platform_devices for any nodes which *did not* get
>    assigned to a device in step 2.
> 
> I implemented this as a way to allow dt and non-dt use-cases to share
> the same SoC setup code so that anything on-chip would get registered
> in the same way, but would also get the benefit of being linked to its
> device tree node.  For example, to obtain the list of i2c devices or
> gpio connections from the tree.  It also helps solve the problem of
> matching nodes to clks which are currently matched by name.  I think
> it would also solve your use case, at least in the short term.
> 
I'm seeing this approach benefits the smooth moving of dt on ARM, but
will this be the ultimate shape of dt support on ARM?

-- 
Regards,
Shawn

Re: No support of platform device instance id?

[Grant Likely]

[widening cc: list to solicit feedback on the new model]

On Wed, Mar 16, 2011 at 09:58:01PM +0800, Shawn Guo wrote:
> On Mon, Mar 14, 2011 at 03:10:19PM -0600, Grant Likely wrote:
> > 
> [...]
> > Several weeks back I posted a patch for of_platform_bus_snoop() which
> > matches platform_device registrations to nodes in the device tree
> > instead of allocating and registering a new device.  I've spent some
> > more time on that patch in the last couple of weeks to the point that
> > I'm happy with the model and I'm almost ready to push it out to my
> > devicetree/test branch.  John Bonesio is currently refactoring and
> 
> I have seen it on devicetree/test branch.

Good.  I don't know if you've seen it yet, but I also posted and
pushed out an updated version last night that cleans up the usage
model quite a bit.  New code uses of_platform_prepare() for flagging
nodes that will be registered later, and both of_platform_bus_probe()
and of_platform_populate() will respect the discoveries made by
of_platform_prepare().

> > cleaning it up for me so that it can get posted.  You can see the
> > current state in my devicetree/preregister branch, with tegra modified
> > to use it.
> > 
> > The model is:
> > 
> > 1) Platform code calls of_platform_device_preregister() to tell the DT
> >    code about the nodes it /intends/ to register as devices.
> > 2) Platform code can register as many or as few platform_devices as it
> >    likes.  If any of these devices match one of the nodes passed by
> >    of_platform_device_preregister(), then the DT code will set the
> >    of_node pointer before it gets bound to a device.
> > 3) Platform code calls of_platform_device_probe() which will
> >    register platform_devices for any nodes which *did not* get
> >    assigned to a device in step 2.
> > 
> > I implemented this as a way to allow dt and non-dt use-cases to share
> > the same SoC setup code so that anything on-chip would get registered
> > in the same way, but would also get the benefit of being linked to its
> > device tree node.  For example, to obtain the list of i2c devices or
> > gpio connections from the tree.  It also helps solve the problem of
> > matching nodes to clks which are currently matched by name.  I think
> > it would also solve your use case, at least in the short term.
> > 
> I'm seeing this approach benefits the smooth moving of dt on ARM, but
> will this be the ultimate shape of dt support on ARM?

I've been spending a *lot* of time thinking about this (which is part
of the reason why it's taken so long to get full dt support for ARM
hammered out; decisions made now will be with us for a long time to
come).  On powerpc the design was easy because all of the BSPs were
converted to require a device tree.  Therefore it made complete sense
to obtain all device information from the tree.

On ARM however, SoC support code must handle both DT and non-DT use
cases, which means that the internal SoC device layout is going to be
encoded in the kernel regardless of whether or not it is available in
the device tree.  If we follow the lead of PowerPC here and obtain all
device information from the DT, then it means DT and non-DT
initialization uses entirely different code paths for the same SoC.
Sure, the DT init code will a lot simpler that its non-DT counterpart,
but is that really an advantage when the non-DT init code needs to be
written regardless?  Perhaps not.

To answer your question, "will this be the ultimate shape of dt
support on ARM?"  I think that ultimately that decision needs to be
made by each sub-arch maintainer.  For existing SoCs, yes I think that
this new model will be the right thing to do, with SoC internals
registered statically, and board-level devices and clocks probed
entirely from the device tree.

I also expect that some maintainers will decide to go 100% device tree
for their SoC.  The Xilinx ARM FPGA is a likely candidate here.  In
that scenario, this new model wouldn't make much sense, and it would
be better to follow the powerpc lead by registering everything
directly from the DT.

Either choice is valid.  It is a tradeoff between sharing
initialization code and the simplicity of full DT device
registration.

I'd like to have feedback on the new code to make sure that the model
is sane.  There are some fiddly code it there which is used to match
platform_device registrations to nodes in the device tree.  I *think*
it makes sense, but I'd like to hear other opinions.

g.

Re: No support of platform device instance id?

[Shawn Guo]

On Wed, Mar 16, 2011 at 05:05:00PM -0600, Grant Likely wrote:
> 
[...]
> I'd like to have feedback on the new code to make sure that the model
> is sane.  There are some fiddly code it there which is used to match
> platform_device registrations to nodes in the device tree.  I *think*
> it makes sense, but I'd like to hear other opinions.
> 
Binding platform_device with of_node via resources matching seems a
little fiddly, but your implementation looks solid to me.  I made a
quick scan on all the platform_device registration under
arch/arm/plat-mxc/devices, nothing would be broken, only except
platform-gpio_keys which is not an on-chip device.

Looking at the code, I'm wondering how the binding of resource type
IORESOURCE_DMA looks like.  I'm seeing some platform_device is using
this flag to tell dma channel fixed for the device.  I'm not sure if
it can be fit in property 'dma-ranges'.  And I only see IORESOURCE_MEM
and IORESOURCE_IRQ being used in the matching model.

-- 
Regards,
Shawn

Re: No support of platform device instance id?

[Grant Likely]

On Thu, Mar 17, 2011 at 12:31:15PM +0800, Shawn Guo wrote:
> On Wed, Mar 16, 2011 at 05:05:00PM -0600, Grant Likely wrote:
> > 
> [...]
> > I'd like to have feedback on the new code to make sure that the model
> > is sane.  There are some fiddly code it there which is used to match
> > platform_device registrations to nodes in the device tree.  I *think*
> > it makes sense, but I'd like to hear other opinions.
> > 
> Binding platform_device with of_node via resources matching seems a
> little fiddly, but your implementation looks solid to me.  I made a
> quick scan on all the platform_device registration under
> arch/arm/plat-mxc/devices, nothing would be broken, only except
> platform-gpio_keys which is not an on-chip device.

Right; and doing it this way doesn't make sense for anything that is
board/system specific because if the board is using the DT, then it is
simpler and more reliable to avoid the matching heuristic entirely.

> Looking at the code, I'm wondering how the binding of resource type
> IORESOURCE_DMA looks like.  I'm seeing some platform_device is using
> this flag to tell dma channel fixed for the device.  I'm not sure if
> it can be fit in property 'dma-ranges'.  And I only see IORESOURCE_MEM
> and IORESOURCE_IRQ being used in the matching model.

The matching is simply a heuristic that makes a best guess about how
how device nodes line up with platform_device registrations.  Right
now the heuristic is to look at the register ranges and irqs described
in the node and make sure that each of them are listed in the
platform_device.  If the platform_device has additional resources,
then it simply ignores them.  That seems to be sufficient for accurate
matching, but I won't know for sure until people start using it.

If there are situations where the heuristic does not work, then I'd
like to know what they are so that the algorithm can be fixed, or in
the absolute worst case implement workaround logic.

One option I'm considering which would improve the accuracy of the
heuristic is to add a 'char *of_compatible' field to struct
platform_device.  Then platform_devices could optionally be tagged
with the compatible value that must be present in order to be matched
with a node.

I definitely want feedback on the matching heuristic!

g.

Re: No support of platform device instance id?

[Shawn Guo]

On Thu, Mar 17, 2011 at 11:08:19AM -0600, Grant Likely wrote:
> On Thu, Mar 17, 2011 at 12:31:15PM +0800, Shawn Guo wrote:
> > On Wed, Mar 16, 2011 at 05:05:00PM -0600, Grant Likely wrote:
> > > 
> > [...]
> > > I'd like to have feedback on the new code to make sure that the model
> > > is sane.  There are some fiddly code it there which is used to match
> > > platform_device registrations to nodes in the device tree.  I *think*
> > > it makes sense, but I'd like to hear other opinions.
> > > 
> > Binding platform_device with of_node via resources matching seems a
> > little fiddly, but your implementation looks solid to me.  I made a
> > quick scan on all the platform_device registration under
> > arch/arm/plat-mxc/devices, nothing would be broken, only except
> > platform-gpio_keys which is not an on-chip device.
> 
> Right; and doing it this way doesn't make sense for anything that is
> board/system specific because if the board is using the DT, then it is
> simpler and more reliable to avoid the matching heuristic entirely.
> 
> > Looking at the code, I'm wondering how the binding of resource type
> > IORESOURCE_DMA looks like.  I'm seeing some platform_device is using
> > this flag to tell dma channel fixed for the device.  I'm not sure if
> > it can be fit in property 'dma-ranges'.  And I only see IORESOURCE_MEM
> > and IORESOURCE_IRQ being used in the matching model.
> 
> The matching is simply a heuristic that makes a best guess about how
> how device nodes line up with platform_device registrations.  Right
> now the heuristic is to look at the register ranges and irqs described
> in the node and make sure that each of them are listed in the
> platform_device.  If the platform_device has additional resources,
> then it simply ignores them.  That seems to be sufficient for accurate
> matching, but I won't know for sure until people start using it.
> 
> If there are situations where the heuristic does not work, then I'd
> like to know what they are so that the algorithm can be fixed, or in
> the absolute worst case implement workaround logic.
> 
> One option I'm considering which would improve the accuracy of the
> heuristic is to add a 'char *of_compatible' field to struct
> platform_device.  Then platform_devices could optionally be tagged
> with the compatible value that must be present in order to be matched
> with a node.
> 
> I definitely want feedback on the matching heuristic!
> 
Tested on babbage with serial, fec and esdhc drivers, and I did see
it solves problems of clkdev dev_id matching and pdev->id use.  And
it even helped to discover one problem with .end definition of fec
resource IORESOURCE_MEM.  I will send mainline a patch for that soon.

-- 
Regards,
Shawn

Re: No support of platform device instance id?

[Grant Likely]

On Sat, Mar 19, 2011 at 8:39 PM, Shawn Guo <shawn.guo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> On Thu, Mar 17, 2011 at 11:08:19AM -0600, Grant Likely wrote:
>> On Thu, Mar 17, 2011 at 12:31:15PM +0800, Shawn Guo wrote:
>> > On Wed, Mar 16, 2011 at 05:05:00PM -0600, Grant Likely wrote:
>> > >
>> > [...]
>> > > I'd like to have feedback on the new code to make sure that the model
>> > > is sane.  There are some fiddly code it there which is used to match
>> > > platform_device registrations to nodes in the device tree.  I *think*
>> > > it makes sense, but I'd like to hear other opinions.
>> > >
>> > Binding platform_device with of_node via resources matching seems a
>> > little fiddly, but your implementation looks solid to me.  I made a
>> > quick scan on all the platform_device registration under
>> > arch/arm/plat-mxc/devices, nothing would be broken, only except
>> > platform-gpio_keys which is not an on-chip device.
>>
>> Right; and doing it this way doesn't make sense for anything that is
>> board/system specific because if the board is using the DT, then it is
>> simpler and more reliable to avoid the matching heuristic entirely.
>>
>> > Looking at the code, I'm wondering how the binding of resource type
>> > IORESOURCE_DMA looks like.  I'm seeing some platform_device is using
>> > this flag to tell dma channel fixed for the device.  I'm not sure if
>> > it can be fit in property 'dma-ranges'.  And I only see IORESOURCE_MEM
>> > and IORESOURCE_IRQ being used in the matching model.
>>
>> The matching is simply a heuristic that makes a best guess about how
>> how device nodes line up with platform_device registrations.  Right
>> now the heuristic is to look at the register ranges and irqs described
>> in the node and make sure that each of them are listed in the
>> platform_device.  If the platform_device has additional resources,
>> then it simply ignores them.  That seems to be sufficient for accurate
>> matching, but I won't know for sure until people start using it.
>>
>> If there are situations where the heuristic does not work, then I'd
>> like to know what they are so that the algorithm can be fixed, or in
>> the absolute worst case implement workaround logic.
>>
>> One option I'm considering which would improve the accuracy of the
>> heuristic is to add a 'char *of_compatible' field to struct
>> platform_device.  Then platform_devices could optionally be tagged
>> with the compatible value that must be present in order to be matched
>> with a node.
>>
>> I definitely want feedback on the matching heuristic!
>>
> Tested on babbage with serial, fec and esdhc drivers, and I did see
> it solves problems of clkdev dev_id matching and pdev->id use.  And
> it even helped to discover one problem with .end definition of fec
> resource IORESOURCE_MEM.  I will send mainline a patch for that soon.

Awesome, thanks!

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.