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'.
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@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
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.
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.
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:
- Platform code calls of_platform_device_preregister() to tell the DT code about the nodes it /intends/ to register as devices.
- 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.
- 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?
[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:
- Platform code calls of_platform_device_preregister() to tell the DT code about the nodes it /intends/ to register as devices.
- 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.
- 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.
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.
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.
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.
On Sat, Mar 19, 2011 at 8:39 PM, Shawn Guo shawn.guo@freescale.com 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 -
Shawn Guo