| Message ID | 1366818789-20276-1-git-send-email-sjg@chromium.org |
|---|---|
| State | RFC |
| Headers | show |
Hello On Wednesday 24 of April 2013 08:53:09 Simon Glass wrote: > From: Pavel Herrmann <morpheus.ibis@gmail.com> > > ** Please note that this is very early code. I am sending out an RFC to > get comments. This is not a commit message. This will only build on sandbox > and all you can do it try the 'demo' command. I am most interested in > comments about how to optimise the implementation to minimise impact on > existing drivers, minimise code bloat and make use of existing knowledge > users may have another other driver model implementations (e.g. Linux). > > This patch adds a very simple drive model implementation. It is taken from > the driver model code developed by: > > Marek Vasut <marex@denx.de> > Pavel Herrmann <morpheus.ibis@gmail.com> > Viktor Křivák <viktor.krivak@gmail.com> > Tomas Hlavacek <tmshlvck@gmail.com> > > and possible others? > > Please see doc/driver-model/README.txt for details of how to run this and > what to look for. > > You can find a test version of the code used here in branch dm2 at: > > http://git.denx.de/u-boot-x86.git > > (Branch dm contains the original implementation) > > This patch requires sandbox generic board support and other enhancements. > > Signed-off-by: Simon Glass <sjg@chromium.org> Thanks for building on our design! What actually happened (for those who were wondering) is that we sucesfully defended the university project part of this (which required us to have a working build, that did not necessarily adhere to what would be applicable to U-Boot upstream, so we dropped of the map for some time), and then went into "rethink and upgrade" mode of operations. I can see that you have already implemented some ot he changes I had in mind (keeping size of private data as a part of the driver, so relocation can be done semi-automatically). I actually had a little more automation in mind (specifically the calls to core_replace, or uclass_replace in your naming scheme, and core_remove), which would mean that for a driver with a flat private data structure (which would be most of them), no relocation hook would be needed. Of course, we could arguably remove relocation as a whole, and use DM only after the relocation has taken place. I agree with the naming change, we started wth the instances+cores, and then stuck with it (and added some more). as for (some of) your changes, i would have a few comments: the driver_instance wrapper around instance was created so that the driver itself doesn't have access to its status flags (currently only the activated flag) and siblings, as it should need them, and definitely should ever modify them. There was no other level of indirection, because the struct instance was embedded in struct driver_instance (eg. not a pointer). I dont see where/if you actually do anything with the private_data_size (didnt read the whole yet), but i certianly didnt find it used for smarter relocations (see above). There is one more problem with the DM structures management, which is not entirely obvious - there is currently no way to tell whether platform_data is in static memory or not (and therefore if it should be relocated/freed. this would of course require to remember the size somewhere). this will arise when you start handling dynamically generated devices (disks, USB) As for removal of platform data, this is not possible. You can load them from FDT for "static" devices, but it will still be required in dynamic devices (again, port number for a disk, USB descriptor number for an USB device). Also relocation/deallocation of platform data is not currently handled in DM code, which causes a slight memory leak. driver_activate() should not be called by the user (or bootup routine) at any point, instead it should be called by the classes when/before proxying the call to the driver specified implementation. We had it in our bootup code only to show that it is possible to do selectively. having a list_head for uclass membership in the struct device is not really a good idea, for two reasons. the biggest one is that some of the classes will need to keep some information about the devices, that is not usable outside the class (id/name for a disk, mapping to global linear numbering for GPIO...). This kind of information is used in queries "give me a device with specific properties", instead of the generic "give me Nth device of this class", The other problem is that it disallows the device to support multiple classes (this could probably be worked around by having virtual child devices for this case) - an exapmle that would use this would be a RAM disk, which you would want to still behave as a normal disk, but be tracked by a separate class (so you can resize/delete/whatever it), I like how you cache the driver structure for each device, instead of looking it up every time. I cannot recall why we didnt use this approach, but i would point out that the name is present/used so we can have runtime loadable drivers sometime in the future (so the symbol would not resolve at link time). I see that you are not building on the latest version from DM branch. there were a few changes in the DM mechanism there (the one I see now is the USB driver match function, which is useless as defined before we actually implemented USB). One more thing that is not possible in current DM - sometimes, we would like to call an operation on ourselves as a part of .probe(), which would create an infinite loop in with driver_activate(). an example of this is when probing a bus controller, you probably want to scan the bus. Thats it for now, I will spend some time over your code in near future, to provide more comments Pavel Herrmann
Hi Peter, On Wed, Apr 24, 2013 at 9:51 AM, Pavel Herrmann <morpheus.ibis@gmail.com> wrote: > Hello > > On Wednesday 24 of April 2013 08:53:09 Simon Glass wrote: >> From: Pavel Herrmann <morpheus.ibis@gmail.com> >> >> ** Please note that this is very early code. I am sending out an RFC to >> get comments. This is not a commit message. This will only build on sandbox >> and all you can do it try the 'demo' command. I am most interested in >> comments about how to optimise the implementation to minimise impact on >> existing drivers, minimise code bloat and make use of existing knowledge >> users may have another other driver model implementations (e.g. Linux). >> >> This patch adds a very simple drive model implementation. It is taken from >> the driver model code developed by: >> >> Marek Vasut <marex@denx.de> >> Pavel Herrmann <morpheus.ibis@gmail.com> >> Viktor Křivák <viktor.krivak@gmail.com> >> Tomas Hlavacek <tmshlvck@gmail.com> >> >> and possible others? >> >> Please see doc/driver-model/README.txt for details of how to run this and >> what to look for. >> >> You can find a test version of the code used here in branch dm2 at: >> >> http://git.denx.de/u-boot-x86.git >> >> (Branch dm contains the original implementation) >> >> This patch requires sandbox generic board support and other enhancements. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> > > > Thanks for building on our design! Thanks for the comments - it has taken me a bit of time to digest... > > What actually happened (for those who were wondering) is that we sucesfully > defended the university project part of this (which required us to have a > working build, that did not necessarily adhere to what would be applicable to > U-Boot upstream, so we dropped of the map for some time), and then went into > "rethink and upgrade" mode of operations. I'm pleased it all worked out well. > > I can see that you have already implemented some ot he changes I had in mind > (keeping size of private data as a part of the driver, so relocation can be > done semi-automatically). I actually had a little more automation in mind > (specifically the calls to core_replace, or uclass_replace in your naming > scheme, and core_remove), which would mean that for a driver with a flat > private data structure (which would be most of them), no relocation hook would > be needed. Of course, we could arguably remove relocation as a whole, and use > DM only after the relocation has taken place. Yes I am beginning to think that we should drop relocation for now, and add it back when we find a problem that it solves. > > I agree with the naming change, we started wth the instances+cores, and then > stuck with it (and added some more). OK good. > > as for (some of) your changes, i would have a few comments: > > the driver_instance wrapper around instance was created so that the driver > itself doesn't have access to its status flags (currently only the activated > flag) and siblings, as it should need them, and definitely should ever modify > them. There was no other level of indirection, because the struct instance was > embedded in struct driver_instance (eg. not a pointer). OK I see. I don't think there is any problem with storing flags or the sibling list node info in the driver data. In fact the whole struct device can be considered to be private to the driver model, except the private data. I wonder if we should allow drivers to embed struct device in their own structure as Linux does? Then it might avoid private data altogether in the common case. Perhaps driver model could be given a size value, which is the size of the entire structure including struct device, and it could automatically allocate this for you? > > I dont see where/if you actually do anything with the private_data_size (didnt > read the whole yet), but i certianly didnt find it used for smarter relocations > (see above). It is allocated for the driver automatically before probe() is callled, and freed after remove() returns. > There is one more problem with the DM structures management, which is not > entirely obvious - there is currently no way to tell whether platform_data is > in static memory or not (and therefore if it should be relocated/freed. this > would of course require to remember the size somewhere). this will arise when > you start handling dynamically generated devices (disks, USB) I suppose we can check the address range? Or perhaps add a flag to indicate whether the device was created pre-relocation. > > As for removal of platform data, this is not possible. You can load them from > FDT for "static" devices, but it will still be required in dynamic devices > (again, port number for a disk, USB descriptor number for an USB device). Also > relocation/deallocation of platform data is not currently handled in DM code, > which causes a slight memory leak. Here I was referring to the need to declare platform_data static data in your board file. With FDT it should not be needed, since the driver can convert the FDT node into platform_data for you, and then use that. For dynamic devices, they probably don't have platform_data anyway, but if they do, then I think the FDT can still be used. It may be true that eliminating platform_data is a step too far in some cases, but I think it is possible. > > driver_activate() should not be called by the user (or bootup routine) at any > point, instead it should be called by the classes when/before proxying the > call to the driver specified implementation. We had it in our bootup code only > to show that it is possible to do selectively. Yes I need to tidy that up. > > having a list_head for uclass membership in the struct device is not really a > good idea, for two reasons. the biggest one is that some of the classes will > need to keep some information about the devices, that is not usable outside > the class (id/name for a disk, mapping to global linear numbering for > GPIO...). This kind of information is used in queries "give me a device with > specific properties", instead of the generic "give me Nth device of this > class", The other problem is that it disallows the device to support multiple > classes (this could probably be worked around by having virtual child devices > for this case) - an exapmle that would use this would be a RAM disk, which you > would want to still behave as a normal disk, but be tracked by a separate > class (so you can resize/delete/whatever it), To take your second point first, I think that devices which implement multiple uclasses should be considered multi-function-devices (MFD). These then to sit in a uclass of their own. Then, they can allocate separate individual devices for each uclass. This seems cleaner than dealing with a device in multiple uclasses. It also mirrors how this is done in Linux. Re the uclass information, that is true. We need a way for the uclass code to store information about each device in a uclass. I will have a think about how to do that, perhaps another private data pointer? > > I like how you cache the driver structure for each device, instead of looking > it up every time. I cannot recall why we didnt use this approach, but i would > point out that the name is present/used so we can have runtime loadable > drivers sometime in the future (so the symbol would not resolve at link time). > OK, there are a few things like that which could do with work. > I see that you are not building on the latest version from DM branch. there > were a few changes in the DM mechanism there (the one I see now is the USB > driver match function, which is useless as defined before we actually > implemented USB). OK - I did compare the code against the final commit in that branch. There certainly were changes, but not many to the core code, so I decided it did not matter for now. > > One more thing that is not possible in current DM - sometimes, we would like > to call an operation on ourselves as a part of .probe(), which would create an > infinite loop in with driver_activate(). an example of this is when probing a > bus controller, you probably want to scan the bus. What sort of operation do you mean here? I would probably hold that when a driver is being probed, it can't use driver model calls to itself from within its probe function. If it needs to, it can call its own functions directly from probe. For example, if it wants to read from the device, ti can call its local read() function instead of trying to call the driver model 'read' handler for itself. The effect should be the same but it will be much less confusing. But perhaps I have missed something here. > > Thats it for now, I will spend some time over your code in near future, to > provide more comments Thanks again for your comments and insight. I am hoping to find the time to respin this again soon. Regards, Simon > > Pavel Herrmann
Hi On Friday 26 of April 2013 06:30:17 Simon Glass wrote: > Hi Peter, > > On Wed, Apr 24, 2013 at 9:51 AM, Pavel Herrmann <morpheus.ibis@gmail.com> wrote: > > Hello > > > > On Wednesday 24 of April 2013 08:53:09 Simon Glass wrote: > > > snip > > > > Thanks for building on our design! > > Thanks for the comments - it has taken me a bit of time to digest... > > > What actually happened (for those who were wondering) is that we > > sucesfully > > defended the university project part of this (which required us to have a > > working build, that did not necessarily adhere to what would be applicable > > to U-Boot upstream, so we dropped of the map for some time), and then > > went into "rethink and upgrade" mode of operations. > > I'm pleased it all worked out well. > > > I can see that you have already implemented some ot he changes I had in > > mind (keeping size of private data as a part of the driver, so relocation > > can be done semi-automatically). I actually had a little more automation > > in mind (specifically the calls to core_replace, or uclass_replace in > > your naming scheme, and core_remove), which would mean that for a driver > > with a flat private data structure (which would be most of them), no > > relocation hook would be needed. Of course, we could arguably remove > > relocation as a whole, and use DM only after the relocation has taken > > place. > > Yes I am beginning to think that we should drop relocation for now, > and add it back when we find a problem that it solves. That would certainly make things simpler, but then one must be very cautious not to make reintroduction of it impossible/significantly harder. The basic idea behind the whole running in early phase was to get serial port (as a debug tool) up and running as soon as possible, without any special handling for it. Later it went as far as doing the whole boot without relocation (if we have enough early memory, or we can initialize RAM but not copy the U-Boot binary there, just the kernel), and then went to being mostly just early access to serial port. > > I agree with the naming change, we started wth the instances+cores, and > > then stuck with it (and added some more). > > OK good. > > > as for (some of) your changes, i would have a few comments: > > > > the driver_instance wrapper around instance was created so that the driver > > itself doesn't have access to its status flags (currently only the > > activated flag) and siblings, as it should need them, and definitely > > should ever modify them. There was no other level of indirection, because > > the struct instance was embedded in struct driver_instance (eg. not a > > pointer). > > OK I see. I don't think there is any problem with storing flags or the > sibling list node info in the driver data. In fact the whole struct > device can be considered to be private to the driver model, except the > private data. parent and platform_data is also of use to the driver, for devices not direcly mapped in memory, but behind some non-transparent bus. > I wonder if we should allow drivers to embed struct device in their > own structure as Linux does? Then it might avoid private data > altogether in the common case. Perhaps driver model could be given a > size value, which is the size of the entire structure including struct > device, and it could automatically allocate this for you? This would make any automation of relocation very hard (because you would need to keep this size information in the tree, instead of just using it once). otherwise I don't see much of a problem with that (the arguments would probably be the size of your whole data structure, and offset of struct device in it, or you would rather have that as a field in the u_boot_driver structure, solving the relocation issue as well). One minor issue could arise if you didn't know the exact size of your private data at compile time, and would only get it after querying the parent in some way during .probe() > > I dont see where/if you actually do anything with the private_data_size > > (didnt read the whole yet), but i certianly didnt find it used for > > smarter relocations (see above). > > It is allocated for the driver automatically before probe() is > callled, and freed after remove() returns. > > > There is one more problem with the DM structures management, which is not > > entirely obvious - there is currently no way to tell whether platform_data > > is in static memory or not (and therefore if it should be > > relocated/freed. this would of course require to remember the size > > somewhere). this will arise when you start handling dynamically generated > > devices (disks, USB) > > I suppose we can check the address range? Or perhaps add a flag to > indicate whether the device was created pre-relocation. Address checking was one of the ideas we had in mind, but it was dismissed as "too hackish". Another idea was to just always make a copy on .reloc() regardless (no free, as reloc() assumes the old memory area will be discarded as a whole afterwards), and depend on free doing the right thing on .remove(). flags are probably a good idea, since you already have a field that is almost empty. You would either need an extra parameter to driver_bind() (since C doesnt have default argument values, it would need to be present in every call), or have a separate driver_dynamic_bind() function that would set the flag to indicate that platform_data is in dynamic memory. This also applies to driver_info structure, but i think it would be safe to only allow driver_info + driver name + platform_data statically, or platform_data and driver_info on heap, driver name statically as valid combinations, so a single flag would be sufficient. > > As for removal of platform data, this is not possible. You can load them > > from FDT for "static" devices, but it will still be required in dynamic > > devices (again, port number for a disk, USB descriptor number for an USB > > device). Also relocation/deallocation of platform data is not currently > > handled in DM code, which causes a slight memory leak. > > Here I was referring to the need to declare platform_data static data > in your board file. With FDT it should not be needed, since the driver > can convert the FDT node into platform_data for you, and then use > that. > > For dynamic devices, they probably don't have platform_data anyway, > but if they do, then I think the FDT can still be used. > > It may be true that eliminating platform_data is a step too far in > some cases, but I think it is possible. Not sure we agree on what is "dynamic devices" here (or maybe on what you can do with FDT). I use "dynamic devices" to refer to devices that can be connected on runtime (or before, of course), and therefore not known before booting and actually scanning available busses. These devices are basically anything that sits on a Plug-and-Play bus, like USB, PCI or SATA We had a rather long discussion (more than a year ago, apologies if i forgot/mixed something up) about the intended semantics of the parent/bus calls, but the one we chose actually needs platform_data for dynamically created devices. One option for calling bus drivers had a call(str device bus, str device child, ...) template, where the bus would be responsible for mapping children to actual bus addresses, using some sort of structure in its private data. The option we went for is the one with call(str device bus, whatever child_addr, ...) template. the child_addr type of course depends on the actual bus in question, and the actual value was stored in the platform_data of the child when it was found/bound. The reason why we chose the second approach is because the first one doesn't handle situations where you have devices in the tree between the bus controller and the real child, but on the bus protocol those devices operate transparently (USB hubs for instance). In that case, the controller would receive a call from either a child it doesn't know, or from the hub, but then it would resolve a wrong bus address. > > driver_activate() should not be called by the user (or bootup routine) at > > any point, instead it should be called by the classes when/before > > proxying the call to the driver specified implementation. We had it in > > our bootup code only to show that it is possible to do selectively. > > Yes I need to tidy that up. > > > having a list_head for uclass membership in the struct device is not > > really a good idea, for two reasons. the biggest one is that some of the > > classes will need to keep some information about the devices, that is not > > usable outside the class (id/name for a disk, mapping to global linear > > numbering for GPIO...). This kind of information is used in queries "give > > me a device with specific properties", instead of the generic "give me > > Nth device of this class", The other problem is that it disallows the > > device to support multiple classes (this could probably be worked around > > by having virtual child devices for this case) - an exapmle that would > > use this would be a RAM disk, which you would want to still behave as a > > normal disk, but be tracked by a separate class (so you can > > resize/delete/whatever it), > > To take your second point first, I think that devices which implement > multiple uclasses should be considered multi-function-devices (MFD). > These then to sit in a uclass of their own. Then, they can allocate > separate individual devices for each uclass. This seems cleaner than > dealing with a device in multiple uclasses. It also mirrors how this > is done in Linux. I guess that is the smarter way of doing it, since there will not be many of such devices. The issue I see would be the interface betweeen the MFD and its children - you either could have a very generic interface for all of them, or have the children play dirty with the parent pointer (getting the ops structure directly and casting it as necessary, which was not really possible with our design, where all calls had to go through the class), or something in the middle (maybe having set interfaces for specific set of functions? that could get very ugly very quick) > Re the uclass information, that is true. We need a way for the uclass > code to store information about each device in a uclass. I will have a > think about how to do that, perhaps another private data pointer? the original design for uclass structure had a strongly-typed (as in not depening on the actual class like now) list of devices in it, and had to store a separate list in the private data (which contained the ops, and possibly other data). after debating proper pairing (structures with the same index being considered paired was considered not robust enough) of those structures we abondoned this idea and merged the two lists into one, making it opque to the outside of the class code. Since you do not need the ops stored for each device, you would end up with most classes not keeping anything, and thus private data for any that do would probably be a way to go. The only thing left is figuring out if you want to store a full mapping (having both the device pointer and its additional info in private data), or just keep the additional info and pair by list indicies (which basically works, but requires some work to code properly, and is harder to debug if things break) > > I like how you cache the driver structure for each device, instead of > > looking it up every time. I cannot recall why we didnt use this approach, > > but i would point out that the name is present/used so we can have > > runtime loadable drivers sometime in the future (so the symbol would not > > resolve at link time). > OK, there are a few things like that which could do with work. > > > I see that you are not building on the latest version from DM branch. > > there > > were a few changes in the DM mechanism there (the one I see now is the USB > > driver match function, which is useless as defined before we actually > > implemented USB). > > OK - I did compare the code against the final commit in that branch. > There certainly were changes, but not many to the core code, so I > decided it did not matter for now. Yes, most of the work was porting drivers, so we could show that all of the board works. the changes in core code was only done when we found some overlooked design flaws (I think there were some minor changes in class relocation, and the USB match, can't remember anything else) > > One more thing that is not possible in current DM - sometimes, we would > > like to call an operation on ourselves as a part of .probe(), which would > > create an infinite loop in with driver_activate(). an example of this is > > when probing a bus controller, you probably want to scan the bus. > > What sort of operation do you mean here? I would probably hold that > when a driver is being probed, it can't use driver model calls to > itself from within its probe function. If it needs to, it can call its > own functions directly from probe. For example, if it wants to read > from the device, ti can call its local read() function instead of > trying to call the driver model 'read' handler for itself. The effect > should be the same but it will be much less confusing. > > But perhaps I have missed something here. The idea is that if your device exports only a very simple I/O, and more complex operations (like scanning for and initializing new child devices on a bus) are actually wrapped in the uclass code. this would mean that calling the complex code would call the proxy, resulting in a call to driver_activate() and an endless recursion. This could probably be solved by having init_started and init_finished flags, to break the recursion (check for init_started, set init_started, call .probe(), set init_finished). This should work as long as the drivers are decent (using their class-provided functions only at the very end of .probe()), and some sanity checking (like what to do when removing a device that has init_started but not init_finished). I am not sure where I encountered this issue at this point, so feel free to drop it until it comes up (or it doesn't, maybe it was caused by a bad prior design decision on my part). Come to think about it, it would also be handy to do this from .bind() hook, but that should work fine at the moment (the use for this would be in USB storage, which would like to scan for partitions when found on USB, without any special handling in the USB code) > > Thats it for now, I will spend some time over your code in near future, to > > provide more comments > > Thanks again for your comments and insight. I am hoping to find the > time to respin this again soon. Apologies if this doesnt make sense at some point, the answers were not done in linear order, and thus may contain contradictions or forward references or be just plain wrong Thanks Pavel
Hi Pavel, On Fri, Apr 26, 2013 at 8:35 AM, Pavel Herrmann <morpheus.ibis@gmail.com> wrote: > Hi > > On Friday 26 of April 2013 06:30:17 Simon Glass wrote: >> Hi Peter, >> >> On Wed, Apr 24, 2013 at 9:51 AM, Pavel Herrmann <morpheus.ibis@gmail.com> > wrote: >> > Hello >> > >> > On Wednesday 24 of April 2013 08:53:09 Simon Glass wrote: >> > > snip >> > >> > Thanks for building on our design! >> >> Thanks for the comments - it has taken me a bit of time to digest... >> >> > What actually happened (for those who were wondering) is that we >> > sucesfully >> > defended the university project part of this (which required us to have a >> > working build, that did not necessarily adhere to what would be applicable >> > to U-Boot upstream, so we dropped of the map for some time), and then >> > went into "rethink and upgrade" mode of operations. >> >> I'm pleased it all worked out well. >> >> > I can see that you have already implemented some ot he changes I had in >> > mind (keeping size of private data as a part of the driver, so relocation >> > can be done semi-automatically). I actually had a little more automation >> > in mind (specifically the calls to core_replace, or uclass_replace in >> > your naming scheme, and core_remove), which would mean that for a driver >> > with a flat private data structure (which would be most of them), no >> > relocation hook would be needed. Of course, we could arguably remove >> > relocation as a whole, and use DM only after the relocation has taken >> > place. >> >> Yes I am beginning to think that we should drop relocation for now, >> and add it back when we find a problem that it solves. > > That would certainly make things simpler, but then one must be very cautious > not to make reintroduction of it impossible/significantly harder. > > The basic idea behind the whole running in early phase was to get serial port > (as a debug tool) up and running as soon as possible, without any special > handling for it. > Later it went as far as doing the whole boot without relocation (if we have > enough early memory, or we can initialize RAM but not copy the U-Boot binary > there, just the kernel), and then went to being mostly just early access to > serial port. OK, let's make the decision to drop it for now, in favour of some sort of link back to the pre-relocation data during probe. So we call the driver's probe() twice - one before relocation, once after. If the drivers wants to check whether it has already been probed, we can provide a way of doing this. We perhaps need to add a capability flag, to indicate that the driver can be used prior to relocation. I suspect some won't work. > >> > I agree with the naming change, we started wth the instances+cores, and >> > then stuck with it (and added some more). >> >> OK good. >> >> > as for (some of) your changes, i would have a few comments: >> > >> > the driver_instance wrapper around instance was created so that the driver >> > itself doesn't have access to its status flags (currently only the >> > activated flag) and siblings, as it should need them, and definitely >> > should ever modify them. There was no other level of indirection, because >> > the struct instance was embedded in struct driver_instance (eg. not a >> > pointer). >> >> OK I see. I don't think there is any problem with storing flags or the >> sibling list node info in the driver data. In fact the whole struct >> device can be considered to be private to the driver model, except the >> private data. > > parent and platform_data is also of use to the driver, for devices not direcly > mapped in memory, but behind some non-transparent bus. Yes that's right. > >> I wonder if we should allow drivers to embed struct device in their >> own structure as Linux does? Then it might avoid private data >> altogether in the common case. Perhaps driver model could be given a >> size value, which is the size of the entire structure including struct >> device, and it could automatically allocate this for you? > > This would make any automation of relocation very hard (because you would need > to keep this size information in the tree, instead of just using it once). > otherwise I don't see much of a problem with that (the arguments would > probably be the size of your whole data structure, and offset of struct device > in it, or you would rather have that as a field in the u_boot_driver structure, > solving the relocation issue as well). > One minor issue could arise if you didn't know the exact size of your private > data at compile time, and would only get it after querying the parent in some > way during .probe() We could require the driver to provide this information in its structure if it wants to use this feature. > >> > I dont see where/if you actually do anything with the private_data_size >> > (didnt read the whole yet), but i certianly didnt find it used for >> > smarter relocations (see above). >> >> It is allocated for the driver automatically before probe() is >> callled, and freed after remove() returns. >> >> > There is one more problem with the DM structures management, which is not >> > entirely obvious - there is currently no way to tell whether platform_data >> > is in static memory or not (and therefore if it should be >> > relocated/freed. this would of course require to remember the size >> > somewhere). this will arise when you start handling dynamically generated >> > devices (disks, USB) >> >> I suppose we can check the address range? Or perhaps add a flag to >> indicate whether the device was created pre-relocation. > > Address checking was one of the ideas we had in mind, but it was dismissed as > "too hackish". Another idea was to just always make a copy on .reloc() > regardless (no free, as reloc() assumes the old memory area will be discarded > as a whole afterwards), and depend on free doing the right thing on .remove(). > flags are probably a good idea, since you already have a field that is almost > empty. You would either need an extra parameter to driver_bind() (since C > doesnt have default argument values, it would need to be present in every > call), or have a separate driver_dynamic_bind() function that would set the > flag to indicate that platform_data is in dynamic memory. > > This also applies to driver_info structure, but i think it would be safe to > only allow driver_info + driver name + platform_data statically, or > platform_data and driver_info on heap, driver name statically as valid > combinations, so a single flag would be sufficient. Well I don't think driver rmodel is responsible for platform data. It is an input to driver model. So if it is static data then it will probably be in rodata or data sections, and will be relocated by U-Boot. We can go through and update the pointers in the hackish way if we wish. If it is allocated data, then I am not sure, but that seems like a strange case. For things like USB, it will be the USB subsystem drivers which create the platform_data presumably. So they are responsible for managing it, not driver model per se. So I think/hope we can not worry about this for now... > >> > As for removal of platform data, this is not possible. You can load them >> > from FDT for "static" devices, but it will still be required in dynamic >> > devices (again, port number for a disk, USB descriptor number for an USB >> > device). Also relocation/deallocation of platform data is not currently >> > handled in DM code, which causes a slight memory leak. >> >> Here I was referring to the need to declare platform_data static data >> in your board file. With FDT it should not be needed, since the driver >> can convert the FDT node into platform_data for you, and then use >> that. >> >> For dynamic devices, they probably don't have platform_data anyway, >> but if they do, then I think the FDT can still be used. >> >> It may be true that eliminating platform_data is a step too far in >> some cases, but I think it is possible. > > Not sure we agree on what is "dynamic devices" here (or maybe on what you can > do with FDT). I use "dynamic devices" to refer to devices that can be > connected on runtime (or before, of course), and therefore not known before > booting and actually scanning available busses. These devices are basically > anything that sits on a Plug-and-Play bus, like USB, PCI or SATA > > We had a rather long discussion (more than a year ago, apologies if i > forgot/mixed something up) about the intended semantics of the parent/bus > calls, but the one we chose actually needs platform_data for dynamically > created devices. > One option for calling bus drivers had a call(str device bus, str device > child, ...) template, where the bus would be responsible for mapping children > to actual bus addresses, using some sort of structure in its private data. > > The option we went for is the one with call(str device bus, whatever > child_addr, ...) template. the child_addr type of course depends on the actual > bus in question, and the actual value was stored in the platform_data of the > child when it was found/bound. > > The reason why we chose the second approach is because the first one doesn't > handle situations where you have devices in the tree between the bus > controller and the real child, but on the bus protocol those devices operate > transparently (USB hubs for instance). In that case, the controller would > receive a call from either a child it doesn't know, or from the hub, but then > it would resolve a wrong bus address. OK, well in this case platform_data is no different for static and dynamic devices. In many cases, dynamic devices will not have platform_data, or a device tree node. But there should be nothing stopping that from being possible. So yes I think providing platform data / FDT node to dynamic devices is fine. > >> > driver_activate() should not be called by the user (or bootup routine) at >> > any point, instead it should be called by the classes when/before >> > proxying the call to the driver specified implementation. We had it in >> > our bootup code only to show that it is possible to do selectively. >> >> Yes I need to tidy that up. >> >> > having a list_head for uclass membership in the struct device is not >> > really a good idea, for two reasons. the biggest one is that some of the >> > classes will need to keep some information about the devices, that is not >> > usable outside the class (id/name for a disk, mapping to global linear >> > numbering for GPIO...). This kind of information is used in queries "give >> > me a device with specific properties", instead of the generic "give me >> > Nth device of this class", The other problem is that it disallows the >> > device to support multiple classes (this could probably be worked around >> > by having virtual child devices for this case) - an exapmle that would >> > use this would be a RAM disk, which you would want to still behave as a >> > normal disk, but be tracked by a separate class (so you can >> > resize/delete/whatever it), >> >> To take your second point first, I think that devices which implement >> multiple uclasses should be considered multi-function-devices (MFD). >> These then to sit in a uclass of their own. Then, they can allocate >> separate individual devices for each uclass. This seems cleaner than >> dealing with a device in multiple uclasses. It also mirrors how this >> is done in Linux. > > I guess that is the smarter way of doing it, since there will not be many of > such devices. The issue I see would be the interface betweeen the MFD and its > children - you either could have a very generic interface for all of them, or > have the children play dirty with the parent pointer (getting the ops > structure directly and casting it as necessary, which was not really possible > with our design, where all calls had to go through the class), or something in > the middle (maybe having set interfaces for specific set of functions? that > could get very ugly very quick) I think for now we need something device-specific. Each child will have knowledge of the parent driver, which may export some operations which the children can use. We can still use the driver model abstraction, but it doesn't need to be handled specially by driver model I think. > >> Re the uclass information, that is true. We need a way for the uclass >> code to store information about each device in a uclass. I will have a >> think about how to do that, perhaps another private data pointer? > > the original design for uclass structure had a strongly-typed (as in not > depening on the actual class like now) list of devices in it, and had to store > a separate list in the private data (which contained the ops, and possibly > other data). > after debating proper pairing (structures with the same index being considered > paired was considered not robust enough) of those structures we abondoned this > idea and merged the two lists into one, making it opque to the outside of the > class code. > Since you do not need the ops stored for each device, you would end up with > most classes not keeping anything, and thus private data for any that do would > probably be a way to go. ' OK. > The only thing left is figuring out if you want to store a full mapping (having > both the device pointer and its additional info in private data), or just keep > the additional info and pair by list indicies (which basically works, but > requires some work to code properly, and is harder to debug if things break) I am thinking putting a private data pointer into struct device, i.e. making that structure responsible for it. Of course this wastes a pointer if most uclasses don't need to store anything about each of their devices. > >> > I like how you cache the driver structure for each device, instead of >> > looking it up every time. I cannot recall why we didnt use this approach, >> > but i would point out that the name is present/used so we can have >> > runtime loadable drivers sometime in the future (so the symbol would not >> > resolve at link time). >> OK, there are a few things like that which could do with work. >> >> > I see that you are not building on the latest version from DM branch. >> > there >> > were a few changes in the DM mechanism there (the one I see now is the USB >> > driver match function, which is useless as defined before we actually >> > implemented USB). >> >> OK - I did compare the code against the final commit in that branch. >> There certainly were changes, but not many to the core code, so I >> decided it did not matter for now. > > Yes, most of the work was porting drivers, so we could show that all of the > board works. the changes in core code was only done when we found some > overlooked design flaws (I think there were some minor changes in class > relocation, and the USB match, can't remember anything else) OK great. > >> > One more thing that is not possible in current DM - sometimes, we would >> > like to call an operation on ourselves as a part of .probe(), which would >> > create an infinite loop in with driver_activate(). an example of this is >> > when probing a bus controller, you probably want to scan the bus. >> >> What sort of operation do you mean here? I would probably hold that >> when a driver is being probed, it can't use driver model calls to >> itself from within its probe function. If it needs to, it can call its >> own functions directly from probe. For example, if it wants to read >> from the device, ti can call its local read() function instead of >> trying to call the driver model 'read' handler for itself. The effect >> should be the same but it will be much less confusing. >> >> But perhaps I have missed something here. > > The idea is that if your device exports only a very simple I/O, and more > complex operations (like scanning for and initializing new child devices on a > bus) are actually wrapped in the uclass code. this would mean that calling the > complex code would call the proxy, resulting in a call to driver_activate() > and an endless recursion. > > This could probably be solved by having init_started and init_finished flags, to > break the recursion (check for init_started, set init_started, call .probe(), > set init_finished). This should work as long as the drivers are decent (using > their class-provided functions only at the very end of .probe()), and some > sanity checking (like what to do when removing a device that has init_started > but not init_finished). > > I am not sure where I encountered this issue at this point, so feel free to > drop it until it comes up (or it doesn't, maybe it was caused by a bad prior > design decision on my part). > > Come to think about it, it would also be handy to do this from .bind() hook, > but that should work fine at the moment (the use for this would be in USB > storage, which would like to scan for partitions when found on USB, without > any special handling in the USB code) OK, let's leave this for now and see if it comes up again in practice. I'm not quite sure the best way to solve it, other than what I suggested. > >> > Thats it for now, I will spend some time over your code in near future, to >> > provide more comments >> >> Thanks again for your comments and insight. I am hoping to find the >> time to respin this again soon. > > Apologies if this doesnt make sense at some point, the answers were not done > in linear order, and thus may contain contradictions or forward references or > be just plain wrong It makes sense to me, thanks for your comments. I will start looking at a new patch. Regards, Simon > > Thanks > Pavel
On Saturday 27 of April 2013 09:08:48 Simon Glass wrote: > Hi Pavel, > > On Fri, Apr 26, 2013 at 8:35 AM, Pavel Herrmann <morpheus.ibis@gmail.com> wrote: > > Hi > > > > On Friday 26 of April 2013 06:30:17 Simon Glass wrote: > >> Hi Peter, > >> > >> On Wed, Apr 24, 2013 at 9:51 AM, Pavel Herrmann <morpheus.ibis@gmail.com> > > > > wrote: > >> > Hello > >> > > >> > On Wednesday 24 of April 2013 08:53:09 Simon Glass wrote: > >> > > snip > >> > > >> > Thanks for building on our design! > >> > >> Thanks for the comments - it has taken me a bit of time to digest... > >> > >> > What actually happened (for those who were wondering) is that we > >> > sucesfully > >> > defended the university project part of this (which required us to have > >> > a > >> > working build, that did not necessarily adhere to what would be > >> > applicable > >> > to U-Boot upstream, so we dropped of the map for some time), and then > >> > went into "rethink and upgrade" mode of operations. > >> > >> I'm pleased it all worked out well. > >> > >> > I can see that you have already implemented some ot he changes I had in > >> > mind (keeping size of private data as a part of the driver, so > >> > relocation > >> > can be done semi-automatically). I actually had a little more > >> > automation > >> > in mind (specifically the calls to core_replace, or uclass_replace in > >> > your naming scheme, and core_remove), which would mean that for a > >> > driver > >> > with a flat private data structure (which would be most of them), no > >> > relocation hook would be needed. Of course, we could arguably remove > >> > relocation as a whole, and use DM only after the relocation has taken > >> > place. > >> > >> Yes I am beginning to think that we should drop relocation for now, > >> and add it back when we find a problem that it solves. > > > > That would certainly make things simpler, but then one must be very > > cautious not to make reintroduction of it impossible/significantly > > harder. > > > > The basic idea behind the whole running in early phase was to get serial > > port (as a debug tool) up and running as soon as possible, without any > > special handling for it. > > Later it went as far as doing the whole boot without relocation (if we > > have > > enough early memory, or we can initialize RAM but not copy the U-Boot > > binary there, just the kernel), and then went to being mostly just early > > access to serial port. > > OK, let's make the decision to drop it for now, in favour of some sort > of link back to the pre-relocation data during probe. So we call the > driver's probe() twice - one before relocation, once after. If the > drivers wants to check whether it has already been probed, we can > provide a way of doing this. > > We perhaps need to add a capability flag, to indicate that the driver > can be used prior to relocation. I suspect some won't work. there is just too much that can happen - either the driver can work in both phases, or it works but needs a relocation hook (which is what we do now), or it can only work in one of the stages (probably the later one, but there might be some very wierd drivers), or it could work in both, but could not be easily relocated (therefore do .remove() and .probe() instead of .reloc()). I agree the best way now is dropping this, but keeping it around as a possible advanced feature (like the runtime driver loading) > >> > I agree with the naming change, we started wth the instances+cores, and > >> > then stuck with it (and added some more). > >> > >> OK good. > >> > >> > as for (some of) your changes, i would have a few comments: > >> > > >> > the driver_instance wrapper around instance was created so that the > >> > driver > >> > itself doesn't have access to its status flags (currently only the > >> > activated flag) and siblings, as it should need them, and definitely > >> > should ever modify them. There was no other level of indirection, > >> > because > >> > the struct instance was embedded in struct driver_instance (eg. not a > >> > pointer). > >> > >> OK I see. I don't think there is any problem with storing flags or the > >> sibling list node info in the driver data. In fact the whole struct > >> device can be considered to be private to the driver model, except the > >> private data. > > > > parent and platform_data is also of use to the driver, for devices not > > direcly mapped in memory, but behind some non-transparent bus. > > Yes that's right. > > >> I wonder if we should allow drivers to embed struct device in their > >> own structure as Linux does? Then it might avoid private data > >> altogether in the common case. Perhaps driver model could be given a > >> size value, which is the size of the entire structure including struct > >> device, and it could automatically allocate this for you? > > > > This would make any automation of relocation very hard (because you would > > need to keep this size information in the tree, instead of just using it > > once). otherwise I don't see much of a problem with that (the arguments > > would probably be the size of your whole data structure, and offset of > > struct device in it, or you would rather have that as a field in the > > u_boot_driver structure, solving the relocation issue as well). > > One minor issue could arise if you didn't know the exact size of your > > private data at compile time, and would only get it after querying the > > parent in some way during .probe() > > We could require the driver to provide this information in its > structure if it wants to use this feature. This starts to bloat the driver structure a little, but as it is in rodata it should not be such an issue. Agreed. > >> > I dont see where/if you actually do anything with the private_data_size > >> > (didnt read the whole yet), but i certianly didnt find it used for > >> > smarter relocations (see above). > >> > >> It is allocated for the driver automatically before probe() is > >> callled, and freed after remove() returns. > >> > >> > There is one more problem with the DM structures management, which is > >> > not > >> > entirely obvious - there is currently no way to tell whether > >> > platform_data > >> > is in static memory or not (and therefore if it should be > >> > relocated/freed. this would of course require to remember the size > >> > somewhere). this will arise when you start handling dynamically > >> > generated > >> > devices (disks, USB) > >> > >> I suppose we can check the address range? Or perhaps add a flag to > >> indicate whether the device was created pre-relocation. > > > > Address checking was one of the ideas we had in mind, but it was dismissed > > as "too hackish". Another idea was to just always make a copy on .reloc() > > regardless (no free, as reloc() assumes the old memory area will be > > discarded as a whole afterwards), and depend on free doing the right > > thing on .remove(). flags are probably a good idea, since you already > > have a field that is almost empty. You would either need an extra > > parameter to driver_bind() (since C doesnt have default argument values, > > it would need to be present in every call), or have a separate > > driver_dynamic_bind() function that would set the flag to indicate that > > platform_data is in dynamic memory. > > > > This also applies to driver_info structure, but i think it would be safe > > to > > only allow driver_info + driver name + platform_data statically, or > > platform_data and driver_info on heap, driver name statically as valid > > combinations, so a single flag would be sufficient. > > Well I don't think driver rmodel is responsible for platform data. It > is an input to driver model. > > So if it is static data then it will probably be in rodata or data > sections, and will be relocated by U-Boot. We can go through and > update the pointers in the hackish way if we wish. > > If it is allocated data, then I am not sure, but that seems like a strange > case. > > For things like USB, it will be the USB subsystem drivers which create > the platform_data presumably. So they are responsible for managing it, > not driver model per se. > > So I think/hope we can not worry about this for now... I think the driver model has to worry about this, specifically the driver_unbind() function. In case of USB (as implemented in our DM tree), the child nodes are created by the USB class, along with platform_data based on the driver probing. However, the class is not called when the device structure is being deallocated (not even via the class_unbind(), since the children do not implement USB interface). At this point, the driver_unbind() function should somehow know whether to free the platform_data as well. This occurs on every call to "usb reset", so its not that rare. Again, assuming we drop relocations now, this would only happen on busses that are rescanned on user command (like USB), but still should be handled in a clean and generic way. > >> > As for removal of platform data, this is not possible. You can load > >> > them > >> > from FDT for "static" devices, but it will still be required in dynamic > >> > devices (again, port number for a disk, USB descriptor number for an > >> > USB > >> > device). Also relocation/deallocation of platform data is not currently > >> > handled in DM code, which causes a slight memory leak. > >> > >> Here I was referring to the need to declare platform_data static data > >> in your board file. With FDT it should not be needed, since the driver > >> can convert the FDT node into platform_data for you, and then use > >> that. > >> > >> For dynamic devices, they probably don't have platform_data anyway, > >> but if they do, then I think the FDT can still be used. > >> > >> It may be true that eliminating platform_data is a step too far in > >> some cases, but I think it is possible. > > > > Not sure we agree on what is "dynamic devices" here (or maybe on what you > > can do with FDT). I use "dynamic devices" to refer to devices that can be > > connected on runtime (or before, of course), and therefore not known > > before booting and actually scanning available busses. These devices are > > basically anything that sits on a Plug-and-Play bus, like USB, PCI or > > SATA > > > > We had a rather long discussion (more than a year ago, apologies if i > > forgot/mixed something up) about the intended semantics of the parent/bus > > calls, but the one we chose actually needs platform_data for dynamically > > created devices. > > One option for calling bus drivers had a call(str device bus, str device > > child, ...) template, where the bus would be responsible for mapping > > children to actual bus addresses, using some sort of structure in its > > private data. > > > > The option we went for is the one with call(str device bus, whatever > > child_addr, ...) template. the child_addr type of course depends on the > > actual bus in question, and the actual value was stored in the > > platform_data of the child when it was found/bound. > > > > The reason why we chose the second approach is because the first one > > doesn't handle situations where you have devices in the tree between the > > bus controller and the real child, but on the bus protocol those devices > > operate transparently (USB hubs for instance). In that case, the > > controller would receive a call from either a child it doesn't know, or > > from the hub, but then it would resolve a wrong bus address. > > OK, well in this case platform_data is no different for static and > dynamic devices. In many cases, dynamic devices will not have > platform_data, or a device tree node. But there should be nothing > stopping that from being possible. > > So yes I think providing platform data / FDT node to dynamic devices is > fine. Of course it would be possible to provide some dynamic devices with FDT nodes (like integrated devices that appear to be on PCI), but we should be able to handle the case where there is no such node, and therefore need a way to provide bus address information (which is basically what platform_data is meant for) to such a device. maybe there is some misunderstanding caused by my lack of knowledge of what you can actually do with FDT, especially if you can add nodes in runtime. this could then effectively replace platform_data (except you would need a way to differentiate between these nodes in case there are multiple same devices present on the same bus) > >> > driver_activate() should not be called by the user (or bootup routine) > >> > at > >> > any point, instead it should be called by the classes when/before > >> > proxying the call to the driver specified implementation. We had it in > >> > our bootup code only to show that it is possible to do selectively. > >> > >> Yes I need to tidy that up. > >> > >> > having a list_head for uclass membership in the struct device is not > >> > really a good idea, for two reasons. the biggest one is that some of > >> > the > >> > classes will need to keep some information about the devices, that is > >> > not > >> > usable outside the class (id/name for a disk, mapping to global linear > >> > numbering for GPIO...). This kind of information is used in queries > >> > "give > >> > me a device with specific properties", instead of the generic "give me > >> > Nth device of this class", The other problem is that it disallows the > >> > device to support multiple classes (this could probably be worked > >> > around > >> > by having virtual child devices for this case) - an exapmle that would > >> > use this would be a RAM disk, which you would want to still behave as a > >> > normal disk, but be tracked by a separate class (so you can > >> > resize/delete/whatever it), > >> > >> To take your second point first, I think that devices which implement > >> multiple uclasses should be considered multi-function-devices (MFD). > >> These then to sit in a uclass of their own. Then, they can allocate > >> separate individual devices for each uclass. This seems cleaner than > >> dealing with a device in multiple uclasses. It also mirrors how this > >> is done in Linux. > > > > I guess that is the smarter way of doing it, since there will not be many > > of such devices. The issue I see would be the interface betweeen the MFD > > and its children - you either could have a very generic interface for all > > of them, or have the children play dirty with the parent pointer (getting > > the ops structure directly and casting it as necessary, which was not > > really possible with our design, where all calls had to go through the > > class), or something in the middle (maybe having set interfaces for > > specific set of functions? that could get very ugly very quick) > > I think for now we need something device-specific. Each child will > have knowledge of the parent driver, which may export some operations > which the children can use. We can still use the driver model > abstraction, but it doesn't need to be handled specially by driver > model I think. I had a thought (possibly very stupid) on how to handle this case in a somewhat clean manner: The MFD interface would contain only a single function, lets call it "void *get_subops_for_class(struct driver *mfd, enum uclass_id)". this would return the ops structure for the specific class (or NULL in case it is not present in this device), which the child would use to call the interface implementation directly (not via the class proxy). This would mean that there would only be a single virtual MFD child driver for each class (avoiding the driver explosion cause by each MFD driver implementing its own children), and that it would be quite simple (very similar to the proxying code of the class itself). The MFD driver would then be responsible for creating the correct children, and providing them with specific interface implementations. > >> Re the uclass information, that is true. We need a way for the uclass > >> code to store information about each device in a uclass. I will have a > >> think about how to do that, perhaps another private data pointer? > > > > the original design for uclass structure had a strongly-typed (as in not > > depening on the actual class like now) list of devices in it, and had to > > store a separate list in the private data (which contained the ops, and > > possibly other data). > > after debating proper pairing (structures with the same index being > > considered paired was considered not robust enough) of those structures > > we abondoned this idea and merged the two lists into one, making it opque > > to the outside of the class code. > > Since you do not need the ops stored for each device, you would end up > > with > > most classes not keeping anything, and thus private data for any that do > > would probably be a way to go. > > ' > OK. > > > The only thing left is figuring out if you want to store a full mapping > > (having both the device pointer and its additional info in private data), > > or just keep the additional info and pair by list indicies (which > > basically works, but requires some work to code properly, and is harder > > to debug if things break) > I am thinking putting a private data pointer into struct device, i.e. > making that structure responsible for it. Of course this wastes a > pointer if most uclasses don't need to store anything about each of > their devices. I would be more concerned about the "here's a pointer, keep it, but don't look" approach. I understand that being fool-proof is not exactly a priority, but still (yes, if it were hidden somewhere you could still access it, but it would look obviously wrong in the code). Keeping an extra pointer (something like class_private) should not be such an issue, considering you would have 200% increase in size if you went for the separate linked list (300% if it were doubly linked), especially if your malloc would be doubleword (as in 2 pointer sizes) aligned, and your structure would end up the same size anyways. > >> > I like how you cache the driver structure for each device, instead of > >> > looking it up every time. I cannot recall why we didnt use this > >> > approach, > >> > but i would point out that the name is present/used so we can have > >> > runtime loadable drivers sometime in the future (so the symbol would > >> > not > >> > resolve at link time). > >> > >> OK, there are a few things like that which could do with work. > >> > >> > I see that you are not building on the latest version from DM branch. > >> > there > >> > were a few changes in the DM mechanism there (the one I see now is the > >> > USB > >> > driver match function, which is useless as defined before we actually > >> > implemented USB). > >> > >> OK - I did compare the code against the final commit in that branch. > >> There certainly were changes, but not many to the core code, so I > >> decided it did not matter for now. > > > > Yes, most of the work was porting drivers, so we could show that all of > > the > > board works. the changes in core code was only done when we found some > > overlooked design flaws (I think there were some minor changes in class > > relocation, and the USB match, can't remember anything else) > > OK great. > > >> > One more thing that is not possible in current DM - sometimes, we would > >> > like to call an operation on ourselves as a part of .probe(), which > >> > would > >> > create an infinite loop in with driver_activate(). an example of this > >> > is > >> > when probing a bus controller, you probably want to scan the bus. > >> > >> What sort of operation do you mean here? I would probably hold that > >> when a driver is being probed, it can't use driver model calls to > >> itself from within its probe function. If it needs to, it can call its > >> own functions directly from probe. For example, if it wants to read > >> from the device, ti can call its local read() function instead of > >> trying to call the driver model 'read' handler for itself. The effect > >> should be the same but it will be much less confusing. > >> > >> But perhaps I have missed something here. > > > > The idea is that if your device exports only a very simple I/O, and more > > complex operations (like scanning for and initializing new child devices > > on a bus) are actually wrapped in the uclass code. this would mean that > > calling the complex code would call the proxy, resulting in a call to > > driver_activate() and an endless recursion. > > > > This could probably be solved by having init_started and init_finished > > flags, to break the recursion (check for init_started, set init_started, > > call .probe(), set init_finished). This should work as long as the > > drivers are decent (using their class-provided functions only at the very > > end of .probe()), and some sanity checking (like what to do when removing > > a device that has init_started but not init_finished). > > > > I am not sure where I encountered this issue at this point, so feel free > > to > > drop it until it comes up (or it doesn't, maybe it was caused by a bad > > prior design decision on my part). > > > > Come to think about it, it would also be handy to do this from .bind() > > hook, but that should work fine at the moment (the use for this would be > > in USB storage, which would like to scan for partitions when found on > > USB, without any special handling in the USB code) > > OK, let's leave this for now and see if it comes up again in practice. > I'm not quite sure the best way to solve it, other than what I > suggested. > > >> > Thats it for now, I will spend some time over your code in near future, > >> > to > >> > provide more comments > >> > >> Thanks again for your comments and insight. I am hoping to find the > >> time to respin this again soon. > > > > Apologies if this doesnt make sense at some point, the answers were not > > done in linear order, and thus may contain contradictions or forward > > references or be just plain wrong > > It makes sense to me, thanks for your comments. I will start looking > at a new patch. Thanks Pavel
diff --git a/Makefile b/Makefile index 42f2b02..7f6e007 100644 --- a/Makefile +++ b/Makefile @@ -337,6 +337,9 @@ LIBS-y += api/libapi.o LIBS-y += post/libpost.o LIBS-y += test/libtest.o +LIBS-$(CONFIG_DM) += common/dm/libdm.o +LIBS-$(CONFIG_DM) += drivers/demo/libdemo.o + ifneq ($(CONFIG_AM33XX)$(CONFIG_OMAP34XX)$(CONFIG_OMAP44XX)$(CONFIG_OMAP54XX)$(CONFIG_TI814X),) LIBS-y += $(CPUDIR)/omap-common/libomap-common.o endif diff --git a/arch/sandbox/include/asm/io.h b/arch/sandbox/include/asm/io.h index 0c022f1..321bc0d 100644 --- a/arch/sandbox/include/asm/io.h +++ b/arch/sandbox/include/asm/io.h @@ -54,6 +54,6 @@ static inline void unmap_sysmem(const void *vaddr) } /* Map from a pointer to our RAM buffer */ -phys_addr_t map_to_sysmem(void *ptr); +phys_addr_t map_to_sysmem(const void *ptr); #endif diff --git a/arch/sandbox/include/asm/types.h b/arch/sandbox/include/asm/types.h index 2316c2d..07c2986 100644 --- a/arch/sandbox/include/asm/types.h +++ b/arch/sandbox/include/asm/types.h @@ -64,8 +64,8 @@ typedef unsigned long long u64; #define BITS_PER_LONG CONFIG_SANDBOX_BITS_PER_LONG typedef unsigned long dma_addr_t; -typedef unsigned long phys_addr_t; -typedef unsigned long phys_size_t; +typedef u32 phys_addr_t; +typedef u32 phys_size_t; #endif /* __KERNEL__ */ diff --git a/common/Makefile b/common/Makefile index 0e0fff1..680ad01 100644 --- a/common/Makefile +++ b/common/Makefile @@ -85,6 +85,7 @@ COBJS-$(CONFIG_CMD_CONSOLE) += cmd_console.o COBJS-$(CONFIG_CMD_CPLBINFO) += cmd_cplbinfo.o COBJS-$(CONFIG_DATAFLASH_MMC_SELECT) += cmd_dataflash_mmc_mux.o COBJS-$(CONFIG_CMD_DATE) += cmd_date.o +COBJS-$(CONFIG_CMD_DEMO) += cmd_demo.o COBJS-$(CONFIG_CMD_SOUND) += cmd_sound.o ifdef CONFIG_4xx COBJS-$(CONFIG_CMD_SETGETDCR) += cmd_dcr.o diff --git a/common/board_r.c b/common/board_r.c index f801e41..436ae83 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -66,7 +66,9 @@ #ifdef CONFIG_X86 #include <asm/init_helpers.h> #endif +#include <dm-demo.h> #include <linux/compiler.h> +#include <linux/err.h> DECLARE_GLOBAL_DATA_PTR; @@ -269,6 +271,57 @@ static int initr_malloc(void) return 0; } +static int initr_dm(void) +{ + dm_init(); + static const struct dm_demo_cdata red_square = { + .colour = "red", + .sides = 4. + }; + static const struct dm_demo_cdata green_triangle = { + .colour = "green", + .sides = 3. + }; + static const struct dm_demo_cdata yellow_hexagon = { + .colour = "yellow", + .sides = 6. + }; + static const struct driver_info info[] = { + { + .name = "demo_shape_drv", + .platform_data = &red_square, + }, + { + .name = "demo_simple_drv", + .platform_data = &red_square, + }, + { + .name = "demo_shape_drv", + .platform_data = &green_triangle, + }, + { + .name = "demo_simple_drv", + .platform_data = &yellow_hexagon, + }, + { + .name = "demo_shape_drv", + .platform_data = &yellow_hexagon, + }, + }; + struct device *root = get_root_instance(); + struct device *demo1; + + demo1 = driver_bind(root, &info[0]); + if (IS_ERR(demo1)) + printf("Demo bind failed: %ld\n", PTR_ERR(demo1)); + driver_bind(demo1, &info[1]); + driver_bind(root, &info[2]); + driver_bind(root, &info[3]); + driver_bind(root, &info[4]); + + return 0; +} + __weak int power_init_board(void) { return 0; @@ -765,6 +818,9 @@ init_fnc_t init_sequence_r[] = { #endif initr_barrier, initr_malloc, +#ifdef CONFIG_DM + initr_dm, +#endif #ifdef CONFIG_ARCH_EARLY_INIT_R arch_early_init_r, #endif diff --git a/common/cmd_demo.c b/common/cmd_demo.c new file mode 100644 index 0000000..657a541 --- /dev/null +++ b/common/cmd_demo.c @@ -0,0 +1,126 @@ +/* + * Copyright (C) 2013 Google, Inc + * + * (C) Copyright 2012 + * Pavel Herrmann <morpheus.ibis@gmail.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <dm-demo.h> +#include <asm/io.h> + +struct device *demo_dev; + +static int do_demo_hello(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int ch = '@'; + + if (argc) + ch = *argv[0]; + + return demo_hello(demo_dev, ch); +} + +static int do_demo_status(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + int status; + int ret; + + ret = demo_status(demo_dev, &status); + if (ret) + return ret; + + printf("Status: %d\n", status); + + return 0; +} + +int do_demo_list(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + struct uclass *class; + struct device *dev; + int i = 0; + + class = class_get_instance(UCLASS_DEMO); + if (!class) + return -ENOMEM; + + puts("Demo class entries:\n"); + + list_for_each_entry(dev, &class->class_node, class_node) { + printf("entry %d - instance %08x, ops %08x, platform_data %08x\n", + i++, map_to_sysmem(dev), + map_to_sysmem(dev->driver->ops), + map_to_sysmem(dev->info->platform_data)); + } + + return 0; +} + +static cmd_tbl_t demo_commands[] = { + U_BOOT_CMD_MKENT(list, 0, 1, do_demo_list, "", ""), + U_BOOT_CMD_MKENT(hello, 2, 1, do_demo_hello, "", ""), + U_BOOT_CMD_MKENT(status, 1, 1, do_demo_status, "", ""), +}; + +static int do_demo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + cmd_tbl_t *demo_cmd; + int devnum = 0; + int ret; + + if (argc < 2) + return CMD_RET_USAGE; + demo_cmd = find_cmd_tbl(argv[1], demo_commands, + ARRAY_SIZE(demo_commands)); + argc -= 2; + argv += 2; + if (!demo_cmd || argc > demo_cmd->maxargs) + return CMD_RET_USAGE; + + if (argc) { + devnum = simple_strtoul(argv[0], NULL, 10); + demo_dev = uclass_get_child(UCLASS_DEMO, devnum); + if (!demo_dev) { + puts("Device not found\n"); + return 1; + } + + ret = driver_activate(demo_dev); + if (ret) + return cmd_process_error(cmdtp, ret); + } + + argc--; + argv++; + ret = demo_cmd->cmd(demo_cmd, flag, argc, argv); + return cmd_process_error(demo_cmd, ret); +} + +U_BOOT_CMD( + demo, 4, 1, do_demo, + "Driver model (dm) demo operations", + "list List available demo devices\n" + "hello <num> Say hello\n" + "status <num> Get demo device status" +); diff --git a/common/command.c b/common/command.c index 305a236..70faa60 100644 --- a/common/command.c +++ b/common/command.c @@ -554,3 +554,13 @@ enum command_ret_t cmd_process(int flag, int argc, char * const argv[], rc = cmd_usage(cmdtp); return rc; } + +int cmd_process_error(cmd_tbl_t *cmdtp, int err) +{ + if (err) { + printf("Command '%s' failed: Error %d\n", cmdtp->name, err); + return 1; + } + + return 0; +} diff --git a/common/dm/Makefile b/common/dm/Makefile new file mode 100644 index 0000000..982a3d5 --- /dev/null +++ b/common/dm/Makefile @@ -0,0 +1,40 @@ +# See file CREDITS for list of people who contributed to this +# project. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation; either version 2 of +# the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, +# MA 02111-1307 USA +# + +include $(TOPDIR)/config.mk + +LIB := $(obj)libdm.o + +COBJS := debug.o driver.o lists.o tree.o root.o uclass.o +SRCS := $(COBJS:.o=.c) +OBJS := $(addprefix $(obj),$(COBJS)) + +all: $(LIB) + +$(LIB): $(obj).depend $(OBJS) + $(call cmd_link_o_target, $(OBJS)) + +######################################################################### + +# defines $(obj).depend target +include $(SRCTREE)/rules.mk + +sinclude $(obj).depend + +######################################################################### diff --git a/common/dm/debug.c b/common/dm/debug.c new file mode 100644 index 0000000..cb36fb4 --- /dev/null +++ b/common/dm/debug.c @@ -0,0 +1,137 @@ +/* + * (C) Copyright 2012 + * Marek Vasut <marex@denx.de> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <malloc.h> +#include <errno.h> +#include <asm/io.h> +#include <dm/debug.h> +#include <dm/manager.h> +#include <dm/structures.h> +#include <dm/uclass.h> + +static int display_succ(struct device *in, char *buf) +{ + int len; + int ip = 0; + char local[16]; + struct device *pos, *n, *prev = NULL; + + printf("%s- %s @ %08x", buf, in->info->name, map_to_sysmem(in)); + if (in->flags & DM_FLAG_ACTIVATED) + puts(" - activated"); + puts("\n"); + + if (list_empty(&in->succ)) + return 0; + + len = strlen(buf); + strncpy(local, buf, sizeof(local)); + snprintf(local + len, 2, "|"); + if (len && local[len - 1] == '`') + local[len - 1] = ' '; + + list_for_each_entry_safe(pos, n, &in->succ, sibling) { + if (ip++) + display_succ(prev, local); + prev = pos; + } + + snprintf(local + len, 2, "`"); + display_succ(prev, local); + + return 0; +} + +int dm_dump_all() +{ + struct device *root; + + root = get_root_instance(); + printf("ROOT %08x\n", map_to_sysmem(root)); + return dm_dump(root); +} + +int dm_dump(struct device *dev) +{ + if (!dev) + return -EINVAL; + return display_succ(dev, ""); +} + +static int dm_dump_classs(void) +{ + struct uclass *class; + int count, num; + int id; + + for (id = 0; id < UCLASS_COUNT; id++) { + class = class_get_instance(id); + if (!class) + continue; + + count = uclass_get_count(id); + printf("class %d: %d instances\n", id, count); + for (num = 0; num < count; num++) { + struct device *inst; + + inst = uclass_get_child(id, num); + printf(" %08x:\n", map_to_sysmem(inst)); + } + puts("\n"); + } + + return 0; +} + +static int do_dm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +{ + struct device *root; + + if (argc != 2) + return -EINVAL; + if (!strncmp(argv[1], "dump", 4)) + return dm_dump_all(); + + if (!strncmp(argv[1], "remove", 6)) { + root = get_root_instance(); + return driver_remove(root); + } + + if (!strncmp(argv[1], "unbind", 6)) { + root = get_root_instance(); + return driver_unbind(root); + } + + if (!strcmp(argv[1], "class")) + return dm_dump_classs(); + + return -EINVAL; +} + +U_BOOT_CMD( + dm, 2, 1, do_dm, + "Driver model ops", + "dump dump driver model tree\n" + "class Dump list of instances for each class" +); diff --git a/common/dm/driver.c b/common/dm/driver.c new file mode 100644 index 0000000..597feab --- /dev/null +++ b/common/dm/driver.c @@ -0,0 +1,374 @@ +/* + * (C) Copyright 2013 Google, Inc + * + * (C) Copyright 2012 + * Pavel Herrmann <morpheus.ibis@gmail.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <malloc.h> +#include <dm/manager.h> +#include <linux/err.h> +#include <linux/list.h> + +/* + * List management functions + */ + +/** + * driver_add_child() - Connect one driver under another + * @parent: Parent driver, under which will the driver be connected + * @child: Child driver, which is to be connected under the parent + */ +static void driver_add_child(struct device *parent, struct device *child) +{ + list_add_tail(&child->sibling, &parent->succ); +} + +/** + * driver_remove_child() - Unbind driver's child from the driver + * @parent: The parent device, from which the child is unbound + * @child: The child device, which is being unbound + */ +static void driver_remove_child(struct device *parent, struct device *child) +{ + list_del(&child->sibling); +} + +/** + * driver_driver_reloc() - Relocate driver's children + * @old: Instance of the old driver + * @new: Instance of the new driver + * + * Relocate the children of a driver. The supplied old device + * contains the list of a children, usually generated during the + * pre-relocation stage, whereas the new device contains no + * children nodes yet. The new device is usually created at + * the post-relocation stage. The nodes from old instance are relocated + * and connected under the new instance. + */ +static int driver_driver_reloc(struct device *old, + struct device *new) +{ + struct device *pos, *n; + struct device *inst; + + list_for_each_entry_safe(pos, n, &old->succ, sibling) { + inst = driver_relocate(pos, new); + if (IS_ERR(inst)) + return PTR_ERR(inst); + driver_add_child(new, inst); + } + + return 0; +} + +/** + * driver_chld_unbind() - Unbind all driver's children from the driver + * @dev: The driver that is to be stripped of off children + */ +static int driver_chld_unbind(struct device *dev) +{ + struct device *pos, *n; + int ret; + + if (!dev) + return -EINVAL; + + if (list_empty(&dev->succ)) + return 0; + + list_for_each_entry_safe(pos, n, &dev->succ, sibling) { + ret = driver_unbind(pos); + if (ret) + return ret; + } + + return 0; +} + +/** + * driver_chld_remove() - Stop all driver's children + * @dev: The driver whose children are to be stopped + */ +static int driver_chld_remove(struct device *dev) +{ + struct device *pos, *n; + int ret; + + if (!dev) + return -EINVAL; + + list_for_each_entry_safe(pos, n, &dev->succ, sibling) { + ret = driver_remove(pos); + if (ret) + return ret; + } + + return 0; +} + +/* + * Driver management functions + */ + +/** + * driver_bind() - Instantiate and connect a driver under a parent one + * @parent: The parent driver, under which the new driver will be connected + * @info: Information about the driver to be probed + * + * Make a new instance of a driver based on data passed by the user. This + * instance is then connected under the parent driver. The driver is not yet + * started after being bound, it's only present in the driver tree. + * + * Returns the instance of a new driver on success, NULL on error. + */ +struct device *driver_bind(struct device *parent, + const struct driver_info *info) +{ + struct device *dev; + struct u_boot_driver *drv; + int ret = 0; + + if (!info || !parent) { + ret = -EINVAL; + goto fail; + } + + dev = calloc(1, sizeof(struct device)); + if (!dev) { + ret = -ENOMEM; + goto fail; + } + + INIT_LIST_HEAD(&dev->sibling); + INIT_LIST_HEAD(&dev->succ); + INIT_LIST_HEAD(&dev->class_node); + dev->info = info; + dev->bus = parent; + drv = get_driver_by_instance(dev); + + if (!drv) { + ret = -ENOENT; + goto fail_free; + } + dev->driver = drv; + + /* put dev into parents successor list */ + driver_add_child(parent, dev); + + ret = uclass_bind(drv->id, dev); + if (ret) + goto fail_bind; + + /* if we fail to bind we remove device from successors and free it */ + if (drv->bind) { + ret = drv->bind(dev); + if (ret) + goto fail_bind; + } + + return dev; + +fail_bind: + driver_remove_child(parent, dev); +fail_free: + free(dev); +fail: + return ERR_PTR(ret); +} + +/** + * driver_activate() - Start a driver + * @i: Instance of a driver that is to be started + * + * This function runs the .probe() function, which initializes the hardware. + * The hardware and all it's predecessors are started by this function. + */ +int driver_activate(struct device *inst) +{ + struct u_boot_driver *drv; + int ret; + + if (!inst) + return -EINVAL; + + drv = get_driver_by_instance(inst); + if (!drv) + return -ENOENT; + + if (inst->flags & DM_FLAG_ACTIVATED) + return 0; + + /* Allocate private data if requested */ + if (drv->priv_data_size) { + inst->priv = calloc(1, drv->priv_data_size); + if (!inst->priv) + return -ENOMEM; + } + + if (inst->bus) { + ret = driver_activate(inst->bus); + if (ret) + goto err; + } + + if (drv->probe) { + ret = drv->probe(inst); + if (ret) + goto err; + } + + inst->flags |= DM_FLAG_ACTIVATED; + return 0; +err: + if (drv->priv_data_size) + free(inst->priv); + + return ret; +} + +/** + * driver_relocate() - Relocate an instance of a driver + * @dev: The instance of a driver to be relocated + * @bus: The instance of a new parent of this driver + * + * Relocate an instance of a driver and it's children. The new + * instance of a driver is allocated and connected under the + * already-relocated parent/bus. The children of the old driver + * are relocated afterwards as well. + */ +struct device *driver_relocate(struct device *dev, struct device *bus) +{ + struct u_boot_driver *ops; + struct device *new_dev; + int ret; + + if (!dev) + return NULL; + + ops = get_driver_by_instance(dev); + if (!ops || !ops->reloc) + return NULL; + + new_dev = calloc(1, sizeof(struct device)); + if (!new_dev) + return NULL; + + INIT_LIST_HEAD(&new_dev->sibling); + INIT_LIST_HEAD(&new_dev->succ); + new_dev->flags = dev->flags; + new_dev->bus = bus; + new_dev->info = dev->info; + /* FIXME: handle driver_info in dynamic memory */ + new_dev->priv = NULL; + + /* relocate the instance */ + ret = ops->reloc(new_dev, dev); + if (ret) + goto err; + + ret = driver_driver_reloc(dev, new_dev); + if (ret) + goto err; + + return new_dev; + +err: + free(new_dev); + return NULL; +}; + +/** + * driver_unbind() - Unbind driver from it's parent + * @dev: The driver to be unbound + * + * Disconnect a driver from it's parent. The driver must be deactived, + * otherwise this function fails and doesn't unbind the driver. + */ +int driver_unbind(struct device *dev) +{ + struct u_boot_driver *ops; + int ret; + + if (!dev) + return -EINVAL; + + if (dev->flags & DM_FLAG_ACTIVATED) + return -EINVAL; + + ops = get_driver_by_instance(dev); + if (!ops) + return -EINVAL; + + ret = driver_chld_unbind(dev); + if (ret) + return ret; + + if (ops->unbind) { + ret = ops->unbind(dev); + if (ret) + return ret; + } + + if (dev->bus) + driver_remove_child(dev->bus, dev); + + return 0; +} + +/** + * driver_remove() - Disable the driver and it's children + * @dev: The instance of a driver to be disabled + * + * Deconfigure the hardware and all it's children. + */ +int driver_remove(struct device *dev) +{ + struct u_boot_driver *ops; + int ret; + + if (!dev) + return -EINVAL; + + if (!(dev->flags & DM_FLAG_ACTIVATED)) + return 0; + + ops = get_driver_by_instance(dev); + if (!ops) + return -EINVAL; + + ret = driver_chld_remove(dev); + if (ret) + return ret; + + if (ops->remove) { + ret = ops->remove(dev); + if (ret) + return ret; + } + + if (dev->driver->priv_data_size) + free(dev->priv); + + dev->flags &= ~DM_FLAG_ACTIVATED; + + return ret; +} diff --git a/common/dm/lists.c b/common/dm/lists.c new file mode 100644 index 0000000..f914143 --- /dev/null +++ b/common/dm/lists.c @@ -0,0 +1,138 @@ +/* + * (C) Copyright 2012 + * Marek Vasut <marex@denx.de> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <dm/manager.h> + +/** + * get_driver_by_instance() - Return u_boot_driver classsponding to instance + * instance: Instance of the driver + * + * This function returns pointer to an u_boot_driver, which is base for the + * supplied instance of a driver. Returns NULL on error. + */ +struct u_boot_driver *get_driver_by_instance(struct device *i) +{ + struct u_boot_driver *drv = + ll_entry_start(struct u_boot_driver, driver); + const int n_ents = ll_entry_count(struct u_boot_driver, driver); + struct u_boot_driver *entry; + const char *name; + int len; + + if (!i || !drv || !n_ents) + return NULL; + + name = i->info->name; + len = strlen(name); + + for (entry = drv; entry != drv + n_ents; entry++) { + if (strncmp(name, entry->name, len)) + continue; + + /* Full match */ + if (len == strlen(entry->name)) + return entry; + } + + /* Not found */ + return NULL; +} + +/** + * get_usb_driver_by_ids() - Return u_boot_driver based on USB IDs + * major: USB major ID + * minor: USB minor ID + * + * This function finds an u_boot_driver inside the U-Boot USB driver list + * based on the USB's major and minor IDs. Returns pointer to the driver on + * success, NULL on error. + */ +struct u_boot_driver *get_usb_driver_by_ids(uint16_t major, uint16_t minor) +{ + struct u_boot_driver *drv = + ll_entry_start(struct u_boot_driver, driver); + const int n_ents = ll_entry_count(struct u_boot_driver, driver); + struct u_boot_driver *entry; + struct usb_ids *ids; + int i; + + if (!drv || !n_ents) + return NULL; + + /* First walk the ID tables */ + for (entry = drv; entry != drv + n_ents; entry++) { + i = 0; + ids = entry->aux_data; + for (;;) { + if (!ids->ids[i].major && !ids->ids[i].minor) + break; + + if (ids->ids[i].major != major) { + i++; + continue; + } + + if (ids->ids[i].minor != minor) { + i++; + continue; + } + + return entry; + } + } + + /* Next, try the match functions */ + for (entry = drv; entry != drv + n_ents; entry++) { + ids = entry->aux_data; + if (ids->match && ids->match()) + return entry; + } + + /* No driver found */ + return NULL; +} + +/** + * get_class_by_id() - Return U_BOOT_CLASS based on ID of the class + * id: ID of the class + * + * This function returns the pointer to U_BOOT_CLASS, which is the class's + * base structure based on the ID of the class. Returns NULL on error. + */ +struct u_boot_class *get_class_by_id(enum uclass_id id) +{ + struct u_boot_class *class = + ll_entry_start(struct u_boot_class, class); + const int n_ents = ll_entry_count(struct u_boot_class, class); + struct u_boot_class *entry; + + if ((id == UCLASS_INVALID) || !class) + return NULL; + + for (entry = class; entry != class + n_ents; entry++) { + if (entry->id == id) + return entry; + } + + return NULL; +} diff --git a/common/dm/root.c b/common/dm/root.c new file mode 100644 index 0000000..3a569ef --- /dev/null +++ b/common/dm/root.c @@ -0,0 +1,101 @@ +/* + * (C) Copyright 2012 + * Pavel Herrmann <morpheus.ibis@gmail.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <dm/structures.h> +#include <dm/manager.h> +#include <malloc.h> +#include <errno.h> +#include <linux/list.h> + +DECLARE_GLOBAL_DATA_PTR; + +static const struct driver_info root_info = { + .name = "root_driver", +}; + +/** + * create_root_instance() - Instantiate the root driver + * + * Create instance of the root driver, the root of the driver tree. + * This root driver is pointed to by the global data. Returns 0 on + * success, negative value on error. + */ +static int create_root_instance(void) +{ + struct device *di; + + if (gd->dm_root) { + puts("Virtual root driver already exists!\n"); + return -EINVAL; + } + + di = calloc(1, sizeof(struct device)); + if (di == NULL) + return -ENOMEM; + + INIT_LIST_HEAD(&di->sibling); + INIT_LIST_HEAD(&di->succ); + di->info = &root_info; + gd->dm_root = di; + + return 0; +} + +/** + * get_root_instance() - Return pointer to the top of the driver tree + * + * This function returns pointer to the root node of the driver tree, + * in case this root wasn't created yet, reports it and returns NULL. + */ +struct device *get_root_instance(void) +{ + if (!gd->dm_root) { + puts("Virtual root driver does not exist!\n"); + return NULL; + } + + return gd->dm_root; +} + +/** + * dm_init() - Initialize Driver Model structures + * + * This function will initialize roots of driver tree and class tree. + * This needs to be called before anything uses the DM + */ +int dm_init(void) +{ + int retval = 0; + + retval = create_root_instance(); + if (retval) + hang(); + + INIT_LIST_HEAD(&gd->class_root); + + return retval; +} + +U_BOOT_DRIVER(__root_driver) = { + .name = "root_driver", +}; diff --git a/common/dm/tree.c b/common/dm/tree.c new file mode 100644 index 0000000..bd08b06 --- /dev/null +++ b/common/dm/tree.c @@ -0,0 +1,71 @@ +/* + * (C) Copyright 2012 + * Viktor Krivak <viktor.krivak@gmail.com> + * Pavel Herrmann <morpheus.ibis@gmail.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <dm/manager.h> +#include <dm/structures.h> +#include <linux/list.h> +#include <malloc.h> +#include <errno.h> + +DECLARE_GLOBAL_DATA_PTR; + +struct node { + struct list_head list; + enum uclass_id key; + struct uclass class; +}; + +/** + * classs_relocate() - Relocate classs from early init memory + * + * Allocate new memory for each class, copy key and call reloc function found + * in specific class ops. Return 0 if succeeded. + */ +int classs_relocate(void) +{ + struct u_boot_class *ops; + struct node *node; + struct node *new; + struct list_head new_head; + INIT_LIST_HEAD(&new_head); + list_for_each_entry(node, &gd->class_root, list) { + ops = get_class_by_id(node->key); + if (!ops) + panic("%s: No ops\n", __func__); + new = malloc(sizeof(*new)); + if (!new) + panic("%s: Unable to allocate memory!", __func__); + new->key = node->key; + /* No reloc method. It is strage but not error */ + if (!ops->reloc) + continue; + if (!ops->reloc(&new->class, &node->class)) + list_add_tail(&new->list, &new_head); + else + panic("%s: Relocation of class fail!", __func__); + } + gd->class_root = new_head; + return 0; +} + diff --git a/common/dm/uclass.c b/common/dm/uclass.c new file mode 100644 index 0000000..97430b2 --- /dev/null +++ b/common/dm/uclass.c @@ -0,0 +1,227 @@ +/* + * (C) Copyright 2012 + * Pavel Herrmann <morpheus.ibis@gmail.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <malloc.h> +#include <dm/manager.h> + +DECLARE_GLOBAL_DATA_PTR; + +/** + * search() - Search class by key in list + * @key: Key used to search apropriet class instance + * + * Linear search in list and if found something, replace it at beginning + * of the list and return class instance inside node. + * If nothing was found return NULL. + */ +static struct uclass *class_search(enum uclass_id key) +{ + struct uclass *inst; + + list_for_each_entry(inst, &gd->class_root, succ) { + if (inst->dm_class->id == key) + return inst; + } + return NULL; +} + +/** + * class_add_instance() - Create and add new class in list + * @key: Key used to init class instance + * + * Call init_node to create class instance and its container structure. + * After that add this class to list. + */ +static struct uclass *class_add_instance(enum uclass_id key) +{ + struct u_boot_class *cls; + struct uclass *inst; + + cls = get_class_by_id(key); + if (!cls) + panic("Cannot find class for id %d\n", key); + inst = calloc(1, sizeof(*inst)); + inst->dm_class = cls; + INIT_LIST_HEAD(&inst->succ); + INIT_LIST_HEAD(&inst->class_node); + list_add(&inst->succ, &gd->class_root); + + return inst; +} + +/** + * class_get_instance() - Return or create class by key + * @key: Search class with this key or create new one + * + * Try to find class in list structure. If fail create a new class and place it + * in list. If fail to create new class, return NULL. Otherwise return + * pointer to designated class. + */ +struct uclass *class_get_instance(enum uclass_id key) +{ + struct uclass *instance; + + instance = class_search(key); + if (!instance) + instance = class_add_instance(key); + return instance; +} + +/** + * uclass_get_count() - Return number of registered children with class + * @id: ID of the class + * + * Count the number of members registered with this class and return + * the value. Negative value is returned in case of failure. + */ +int uclass_get_count(enum uclass_id id) +{ + struct uclass *class; + struct device *inst; + int count = 0; + + class = class_get_instance(id); + if (!class) + return -ENOMEM; + + list_for_each_entry(inst, &class->class_node, class_node) + count++; + + return count; +} + +/** + * uclass_get_child() - Return n-th child of class + * @id: ID of the class + * @index: Position of the child in class's list + * + * Return the instance pointer of a child at the given index or + * return NULL on error. + */ +struct device *uclass_get_child(enum uclass_id id, int index) +{ + struct uclass *class; + struct device *inst; + + class = class_get_instance(id); + if (!class) + return NULL; + + list_for_each_entry(inst, &class->class_node, class_node) { + if (!index--) + return inst; + } + + return NULL; +} + +/** + * uclass_bind() - Associate driver with a class + * @id: ID of the class + * @dev: Pointer to the driver's instance + * @ops: Structure carrying operations this driver provides to the class + * + * Connect the driver into class's list of drivers. Returns 0 on success, + * negative value on error. + */ +int uclass_bind(enum uclass_id id, struct device *dev) +{ + struct uclass *class; + struct u_boot_class *class_ops; + int ret; + + class = class_get_instance(id); + if (!class) + return -ENOMEM; + + class_ops = get_class_by_id(id); + if (!class_ops) + return -ENOENT; + if (class_ops->ops) + return -EINVAL; + + list_add_tail(&dev->class_node, &class->class_node); + + if (class_ops->bind) { + ret = class_ops->bind(class, dev); + if (ret) { + list_del(&dev->class_node); + return ret; + } + } + + return 0; +} + +/** + * uclass_unbind() - Deassociate driver with a class + * @id: ID of the class + * @dev: Pointer to the driver's instance + * + * Disconnect the driver from class's list of drivers. + * Returns 0 on success, negative value on error. + */ +int uclass_unbind(enum uclass_id id, struct device *dev) +{ + struct uclass *class; + struct u_boot_class *class_ops; + + class = class_get_instance(id); + if (!class) + return -ENOMEM; + + class_ops = get_class_by_id(id); + if (!class_ops) + return -ENOENT; + + return class_ops->unbind(class, dev); +} + +/** + * uclass_replace() - Replace instance of a driver with another in class's list + * @id: ID of the class + * @new: Pointer to the driver's new instance + * @old: Pointer to the driver's old instance + * + * Replace old instance of a driver, usually pre-relocation one, in the list + * of drivers associated with a class with a new one, usually a post-relocation + * one. All of the internal state of the class associated with the old instance + * is preserved. + * + * Returns 0 on success, negative value on error. + */ +int uclass_replace(enum uclass_id id, struct device *new, struct device *old) +{ + struct uclass *class; + struct u_boot_class *class_ops; + + class = class_get_instance(id); + if (!class) + return -ENOMEM; + + class_ops = get_class_by_id(id); + if (!class_ops) + return -ENOENT; + + return class_ops->replace(class, new, old); +} diff --git a/doc/driver-model/README.txt b/doc/driver-model/README.txt new file mode 100644 index 0000000..e077220 --- /dev/null +++ b/doc/driver-model/README.txt @@ -0,0 +1,307 @@ +Driver Model RFC +================ + +This README contains information about an early RFC implementation of +driver model. The original work was done by: + + Marek Vasut <marex@denx.de> + Pavel Herrmann <morpheus.ibis@gmail.com> + Viktor Křivák <viktor.krivak@gmail.com> + Tomas Hlavacek <tmshlvck@gmail.com> + +The code was taken from branch dm at: + + git://git.denx.de/u-boot-marex.git + +You can find a test version of the code used here in branch dm2 at: + + http://git.denx.de/u-boot-x86.git + +(Branch dm contains the original implementation) + + +The goal of this effort is to get a very basic implementation into +the U-Boot source tree quickly - something which people can build on, +rather than something that solves all of the problems immediately. +The only driver support that I am planning to add is GPIO, because +it is simple, but non-trivial, and much of the work is already done +by the people above. + +NOTE: This code is an early incoherent, inconsistent and largely +incorrect draft. Please use it for making comments on the approach. +Some knowledge of the existing driver model design is helpful. + +NOTE: Please read the above note again. + + +How to try it +------------- + +Build U-Boot sandbox and run it: + + make sandbox_config + make + ./u-boot + + (type 'reset' to exit U-Boot) + + +The only available device class (uclass) is 'demo'. This uclass handles +saying hello, and reporting its status. There are two drivers in this +uclass: + + - simple: Just prints a message for hello, doesn't implement status + - shape: Prints shapes and reports number of characters printed as status + +The demo class is pretty simple, but not trivial. The intention is that it +can be used for testing, so it will implement all driver model features and +provide 100% code coverage of them. It does have multiple drivers, it +handles parameter data and platform_data (data which tells the driver how +to operate on a particular platform) and it uses private driver data. + +To try it, see the example session below: + +=>demo hello 1 +probe from 07981110 +Hello '@' from 07981110: red 4 +=>demo status 2 +Status: 0 +=>demo hello 2 +g +r@ +e@@ +e@@@ +n@@@@ +g@@@@@ +=>demo status 2 +Status: 21 +=>demo hello 4 ^ + y^^^ + e^^^^^ +l^^^^^^^ +l^^^^^^^ + o^^^^^ + w^^^ +=>demo status 4 +Status: 36 +=> + + +What is going on? +----------------- + +Let's start at the top. The demo command is in common/cmd_demo.c. It does +the usual command procesing and then: + + struct device *demo_dev; + + demo_dev = uclass_get_child(UCLASS_DEMO, devnum); + +UCLASS_DEMO means the class of devices which implement 'demo'. Other +classes might be MMC, or GPIO, hashing or serial. The idea is that the +devices in the class all share a particular way of working. The class +presents a unified view of all these devices to U-Boot. + +This function looks up the device for the demo uclass. Given a device +number we can find the device because all devices have registered with +the UCLASS_DEMO uclass. + +Having found the device, we activate it with: + + ret = driver_activate(demo_dev); + +This is because all devices are inactive until used in U-Boot. The exact +mechanism of activating a driver will hopefully change. For example, i +may be possible to combine uclass_get_child() and driver_activate(). + +Now that we have the device we can do things like: + + return demo_hello(demo_dev, ch); + +This function is in the demo uclass. It takes care of calling the 'hello' +method of the relevant driver. Bearing in mind that there are two drivers, +this particular device may use one or other of them. + +The code for demo_hello() is in drivers/demo/demo-uclass.c: + +int demo_hello(struct device *dev, int ch) +{ + const struct demo_ops *ops = device_get_ops(dev); + + if (!ops->hello) + return -ENOSYS; + + return ops->hello(dev, ch); +} + +As you can see it just calls the relevant driver method. One of these is +in drivers/demo/demo-simple.c: + +static int simple_hello(struct device *dev, int ch) +{ + const struct dm_demo_cdata *pdata = dev->info->platform_data; + + printf("Hello '%c' from %08x: %s %d\n", ch, map_to_sysmem(dev), + pdata->colour, pdata->sides); + + return 0; +} + + +So that is a trip from top to bottom but it leaves a lot of topics to +address. + + +Declaring Drivers +----------------- + +A driver declaration looks something like this (see +drivers/demo/demo-shape.c): + +static const struct demo_ops simple_ops = { + .hello = shape_hello, + .status = shape_status, +}; + +U_BOOT_DRIVER(demo_shape_drv) = { + .name = "demo_shape_drv", + .id = UCLASS_DEMO, + .ops = &simple_ops, + .priv_data_size = sizeof(struct shape_data), +}; + + +This driver has two methods (hello and status) and requires a bit +of private data (accessible through dev->priv once the driver has +been probed). It is a member of UCLASS_DEMO so will register itself +there. + +In U_BOOT_DRIVER it is also possible to specify special methods for probe, +and bind, and these are called at appropriate times. For many drivers +it is hoped that only 'probe' and 'remove' will be needed. + +The U_BOOT_DRIVER macro creates a data structure accessible from C, +so driver model can find the drivers that are available. + + +Platform Data +------------- + +Where does the platform data come from? See common/board_r.c which +sets up a table of driver names and their associated platform data. +The data can be interpreted by the drivers however they like - it is +basically a communication scheme between the board-specific code and +the generic drivers, which are intended to work on any board. + +Drivers can acceess their data via dev->info->platform_data. Here is +the declaration for the platform data, which would normally appear +in the board file. + + static const struct dm_demo_cdata red_square = { + .colour = "red", + .sides = 4. + }; + static const struct driver_info info[] = { + { + .name = "demo_shape_drv", + .platform_data = &red_square, + }, + }; + + demo1 = driver_bind(root, &info[0]); + + +Device Tree +----------- + +This is not yet implemented but will be before this work leaves RFC status. +We will use device tree to specify the platform data and activated drivers. +In other words we replace the above code with the following device tree +fragment: + + red-square { + compatible = "demo-shape"; + colour = "red"; + sides = <4>; + }; + + +Driver model will look after creating a new device and feeding the device +node into it. The driver itself will decode the device tree node and +produce its own platform_data from it. + + +Declaring Uclasses +------------------ + +The demo uclass is declared like this: + +U_BOOT_CLASS(demo) = { + .id = UCLASS_DEMO, +}; + +It is also possible to specify special methods for probe, etc. The uclass +numbering comes from include/dm/uclass.h. + + +Data Structures +--------------- + +Driver model uses a doubly-linked list as the basic data structure. Some +nodes have several lists running through them. Creating a more efficient +data structure might be worthwhile in some rare cases, once we understand +what the bottlenecks are. + + +Changes so far +-------------- + +The documenation in this direction is out of data and until the patch +is in more solid form I have avoided updating it. This implementation +uses a very similar approach, but makes at least the following changes: + +- Tried to agressively remove boilerplate, so that for most drivers there +is little or no 'driver model' code to write. +- Moved some data from code into data structure - e.g. store a pointer to +the driver operations structure in the driver, rather than passing it +to the driver bind function. +- Rename some structures to make them more similar to Linux (struct device +instead of struct instance, struct platform_data, etc.) +- Change the name 'core' to 'uclass', meaning U-Boot class. It seems that +this concept relates to a class of drivers (or a subsystem). We shouldn't +use 'class' since it is a C++ reserved word, so U-Boot class (uclass) seems +better than 'core'. +- Remove 'struct driver_instance' and just use a single 'struct device'. +This removes a level of indirection that doesn't seem necessary. + + +Proposed additional changes +--------------------------- + +- Build in device tree support, to avoid the need for platform_data +- Possibly remove the concept of driver relocation, and just make it +possible for the new driver (created after relocation) to access the old +driver data. I feel that relocation is a very special case and will only +apply to a few drivers, many of which can/will just re-init anyway. So +the overhead of dealing with this might not be worth it. This is TBD +though. +- Implement for GPIO subsystem. An implementation is already done in the +driver model tree + + +Things to punt for later +------------------------ + +- SPL support - this will have to be present before many drivers can be +converted, but it seems like we can add it once we are happy with the +core implementation. +- Pre-relocation support - similar story + +That is not to say that no thinking has gone into these - in fact there +is quite a lot there. However, getting these right is non-trivial and +there is a high cost associated with going down the wrong path. + + +Simon Glass +sjg@chromium.org +April 2013 diff --git a/drivers/demo/Makefile b/drivers/demo/Makefile new file mode 100644 index 0000000..60a2b88 --- /dev/null +++ b/drivers/demo/Makefile @@ -0,0 +1,44 @@ +# See file CREDITS for list of people who contributed to this +# project. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation; either version 2 of +# the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, +# MA 02111-1307 USA +# + +include $(TOPDIR)/config.mk + +LIB := $(obj)libdemo.o + +COBJS-$(CONFIG_DM_DEMO) += demo-uclass.o +COBJS-$(CONFIG_DM_DEMO_SIMPLE) += demo-simple.o +COBJS-$(CONFIG_DM_DEMO_SHAPE) += demo-shape.o + +COBJS := $(COBJS-y) +SRCS := $(COBJS:.o=.c) +OBJS := $(addprefix $(obj),$(COBJS)) + +all: $(LIB) + +$(LIB): $(obj).depend $(OBJS) + $(call cmd_link_o_target, $(OBJS)) + +######################################################################### + +# defines $(obj).depend target +include $(SRCTREE)/rules.mk + +sinclude $(obj).depend + +######################################################################### diff --git a/drivers/demo/demo-shape.c b/drivers/demo/demo-shape.c new file mode 100644 index 0000000..b72fcba --- /dev/null +++ b/drivers/demo/demo-shape.c @@ -0,0 +1,106 @@ +/* + * (C) Copyright 2013 Google, Inc + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <malloc.h> +#include <dm-demo.h> +#include <asm/io.h> +#include <dm/manager.h> + +/* Shape size */ +#define WIDTH 8 +#define HEIGHT 6 + +struct shape_data { + int num_chars; /* Number of non-space characters output so far */ +}; + +/* Crazy little function to draw shapes on the console */ +static int shape_hello(struct device *dev, int ch) +{ + const struct dm_demo_cdata *pdata = dev->info->platform_data; + struct shape_data *data = dev->priv; + static const struct shape { + int start; + int end; + int dstart; + int dend; + } shapes[3] = { + { 0, 1, 0, 1 }, + { 0, WIDTH, 0, 0 }, + { HEIGHT / 2 - 1, WIDTH - HEIGHT / 2 + 1, -1, 1}, + }; + struct shape shape; + unsigned int index; + int line, pos, inside; + const char *colour = pdata->colour; + int first = 0; + + index = (pdata->sides / 2) - 1; + if (index >= ARRAY_SIZE(shapes)) + return -EIO; + shape = shapes[index]; + + for (line = 0; line < HEIGHT; line++) { + first = 1; + for (pos = 0; pos < WIDTH; pos++) { + inside = pos >= shape.start && pos < shape.end; + if (inside) { + putc(first ? *colour++ : ch); + data->num_chars++; + first = 0; + if (!*colour) + colour = pdata->colour; + } else { + putc(' '); + } + } + putc('\n'); + shape.start += shape.dstart; + shape.end += shape.dend; + if (shape.start < 0) { + shape.dstart = -shape.dstart; + shape.dend = -shape.dend; + shape.start += shape.dstart; + shape.end += shape.dend; + } + } + + return 0; +} + +static int shape_status(struct device *dev, int *status) +{ + struct shape_data *data = dev->priv; + + *status = data->num_chars; + return 0; +} + +static const struct demo_ops simple_ops = { + .hello = shape_hello, + .status = shape_status, +}; + +U_BOOT_DRIVER(demo_shape_drv) = { + .name = "demo_shape_drv", + .id = UCLASS_DEMO, + .ops = &simple_ops, + .priv_data_size = sizeof(struct shape_data), +}; diff --git a/drivers/demo/demo-simple.c b/drivers/demo/demo-simple.c new file mode 100644 index 0000000..e0c55f3 --- /dev/null +++ b/drivers/demo/demo-simple.c @@ -0,0 +1,82 @@ +/* + * (C) Copyright 2013 Google, Inc + * + * (C) Copyright 2012 + * Pavel Herrmann <morpheus.ibis@gmail.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <dm-demo.h> +#include <asm/io.h> +#include <dm/manager.h> + +static int simple_hello(struct device *dev, int ch) +{ + const struct dm_demo_cdata *pdata = dev->info->platform_data; + + printf("Hello '%c' from %08x: %s %d\n", ch, map_to_sysmem(dev), + pdata->colour, pdata->sides); + + return 0; +} + +static const struct demo_ops simple_ops = { + .hello = simple_hello, +}; + +static int simple_bind(struct device *dev) +{ + printf("bind from %08x\n", map_to_sysmem(dev)); + return 0; +} + +static int simple_probe(struct device *dev) +{ + printf("probe from %08x\n", map_to_sysmem(dev)); + return 0; +} + +static int simple_reloc(struct device *new, struct device *old) +{ + printf("reloc to %08x from %08x\n", map_to_sysmem(new), + map_to_sysmem(old)); + return 0; +} + +static int simple_remove(struct device *dev) +{ + printf("remove from %08x\n", map_to_sysmem(dev)); + return 0; +} + +static int simple_unbind(struct device *dev) +{ + printf("unbind from %08x\n", map_to_sysmem(dev)); + return 0; +} + +U_BOOT_DRIVER(demo_simple_drv) = { + .name = "demo_simple_drv", + .id = UCLASS_DEMO, + .ops = &simple_ops, + .bind = simple_bind, + .probe = simple_probe, + .reloc = simple_reloc, + .remove = simple_remove, + .unbind = simple_unbind, +}; diff --git a/drivers/demo/demo-uclass.c b/drivers/demo/demo-uclass.c new file mode 100644 index 0000000..a5a3367 --- /dev/null +++ b/drivers/demo/demo-uclass.c @@ -0,0 +1,52 @@ +/* + * (C) Copyright 2012 + * Pavel Herrmann <morpheus.ibis@gmail.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <malloc.h> +#include <dm-demo.h> +#include <asm/io.h> +#include <dm/manager.h> +#include <linux/list.h> + +U_BOOT_CLASS(demo) = { + .id = UCLASS_DEMO, +}; + +int demo_hello(struct device *dev, int ch) +{ + const struct demo_ops *ops = device_get_ops(dev); + + if (!ops->hello) + return -ENOSYS; + + return ops->hello(dev, ch); +} + +int demo_status(struct device *dev, int *status) +{ + const struct demo_ops *ops = device_get_ops(dev); + + if (!ops->status) + return -ENOSYS; + + return ops->status(dev, status); +} diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 5416f46..4425284 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -24,6 +24,9 @@ #ifndef __ASM_GENERIC_GBL_DATA_H #define __ASM_GENERIC_GBL_DATA_H + +#include <linux/list.h> + /* * The following data structure is placed in some memory which is * available very early after boot (like DPRAM on MPC8xx/MPC82xx, or @@ -80,6 +83,12 @@ typedef struct global_data { unsigned long start_addr_sp; /* start_addr_stackpointer */ unsigned long reloc_off; struct global_data *new_gd; /* relocated global data */ + +#ifdef CONFIG_DM + struct device *dm_root; /* Root instance for Driver Model */ + struct list_head class_root; /* Head of core tree */ +#endif + const void *fdt_blob; /* Our device tree, NULL if none */ void *new_fdt; /* Relocated FDT */ unsigned long fdt_size; /* Space reserved for relocated FDT */ diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index bfedbe4..ead3515 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -94,4 +94,24 @@ int gpio_get_value(unsigned gpio); * @return 0 if ok, -1 on error */ int gpio_set_value(unsigned gpio, int value); + +/* + * Driver model GPIO operations, refer to functions above for description. + * These function copy the old API. + * + * This is trying to be close to Linux GPIO API. Once the U-Boot uses the + * new DM GPIO API, this should be really easy to flip over to the Linux + * GPIO API-alike interface. + */ +struct dm_gpio_ops { + int base; + u16 ngpio; + int (*gpio_request)(unsigned gpio, const char *label); + int (*gpio_free)(unsigned gpio); + int (*gpio_direction_input)(unsigned gpio); + int (*gpio_direction_output)(unsigned gpio, int value); + int (*gpio_get_value)(unsigned gpio); + int (*gpio_set_value)(unsigned gpio, int value); +}; + #endif /* _ASM_GENERIC_GPIO_H_ */ diff --git a/include/command.h b/include/command.h index 65692fd..445c9e8 100644 --- a/include/command.h +++ b/include/command.h @@ -80,6 +80,8 @@ extern int var_complete(int argc, char * const argv[], char last_char, int maxv, extern int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp); #endif +int cmd_process_error(cmd_tbl_t *cmdtp, int err); + /* * Monitor Command * diff --git a/include/common.h b/include/common.h index 28aa4b9..bdf598f 100644 --- a/include/common.h +++ b/include/common.h @@ -911,7 +911,7 @@ static inline void unmap_sysmem(const void *vaddr) { } -static inline phys_addr_t map_to_sysmem(void *ptr) +static inline phys_addr_t map_to_sysmem(const void *ptr) { return (phys_addr_t)(uintptr_t)ptr; } diff --git a/include/config_fallbacks.h b/include/config_fallbacks.h index e59ee96..c91a383 100644 --- a/include/config_fallbacks.h +++ b/include/config_fallbacks.h @@ -53,4 +53,12 @@ #define HAVE_BLOCK_DEVICE #endif +#ifdef CONFIG_DM +/* For now, enable the driver model demo command */ +#define CONFIG_DM_DEMO +#define CONFIG_CMD_DEMO +#define CONFIG_DM_DEMO_SIMPLE +#define CONFIG_DM_DEMO_SHAPE +#endif + #endif /* __CONFIG_FALLBACKS_H */ diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h index 788207d..76c4398 100644 --- a/include/configs/sandbox.h +++ b/include/configs/sandbox.h @@ -22,6 +22,8 @@ #ifndef __CONFIG_H #define __CONFIG_H +#define CONFIG_DM + /* Number of bits in a C 'long' on this architecture */ #define CONFIG_SANDBOX_BITS_PER_LONG 64 diff --git a/include/dm-demo.h b/include/dm-demo.h new file mode 100644 index 0000000..3bfac5a --- /dev/null +++ b/include/dm-demo.h @@ -0,0 +1,47 @@ +/* + * Copyright (c) 2013 The Chromium OS Authors. + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef __DM_DEMO_H +#define __DM_DEMO_H + +#include <dm.h> + +/** + * struct dm_demo_cdata - configuration data for demo instance + * + * @colour: Color of the demo + * @sides: Numbers of sides + */ +struct dm_demo_cdata { + const char *colour; + int sides; +}; + +struct demo_ops { + int (*hello)(struct device *i, int ch); + int (*status)(struct device *i, int *status); +}; + +int demo_hello(struct device *i, int ch); +int demo_status(struct device *i, int *status); +int demo_list(void); + +#endif diff --git a/include/dm.h b/include/dm.h new file mode 100644 index 0000000..6af3900 --- /dev/null +++ b/include/dm.h @@ -0,0 +1,27 @@ +/* + * Copyright (C) 2013 Google, Inc + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef _DM_H_ +#define _DM_H + +#include <dm/manager.h> +#include <dm/structures.h> +#include <dm/uclass.h> + +#endif diff --git a/include/dm/debug.h b/include/dm/debug.h new file mode 100644 index 0000000..3f5ea7b --- /dev/null +++ b/include/dm/debug.h @@ -0,0 +1,33 @@ +/* + * (C) Copyright 2012 + * Pavel Herrmann <morpheus.ibis@gmail.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + + +#ifndef _DM_DEBUG_H_ +#define _DM_DEBUG_H_ 1 + +#include <dm/structures.h> + +int dm_dump_all(void); +int dm_dump(struct device *i); + +#endif diff --git a/include/dm/manager.h b/include/dm/manager.h new file mode 100644 index 0000000..fcc001e --- /dev/null +++ b/include/dm/manager.h @@ -0,0 +1,57 @@ +/* + * (C) Copyright 2012 + * Pavel Herrmann <morpheus.ibis@gmail.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef _DM_MANAGER_H_ +#define _DM_MANAGER_H_ + +#include <dm/uclass.h> +#include <dm/structures.h> + +/* find-and-add function for the class tree */ +struct uclass *class_get_instance(enum uclass_id id); +struct u_boot_class *get_class_by_id(enum uclass_id id); + +/* class API wrappers */ +int uclass_get_count(enum uclass_id id); +struct device *uclass_get_child(enum uclass_id id, int index); +int uclass_bind(enum uclass_id id, struct device *dev); +int uclass_unbind(enum uclass_id id, struct device *dev); +int uclass_replace(enum uclass_id id, struct device *new, struct device *old); + +/* driver manager API */ +struct device *driver_bind(struct device *parent, + const struct driver_info *info); +int driver_activate(struct device *i); +int driver_remove(struct device *i); +int driver_unbind(struct device *dev); +struct u_boot_driver *get_driver_by_instance(struct device *i); + +/* relocation stuff */ +struct device *driver_relocate(struct device *dev, struct device *bus); +int classs_relocate(void); + +/* tree creation helpers */ +int dm_init(void); +struct device *get_root_instance(void); + +#endif diff --git a/include/dm/structures.h b/include/dm/structures.h new file mode 100644 index 0000000..32ca832 --- /dev/null +++ b/include/dm/structures.h @@ -0,0 +1,97 @@ +/* + * (C) Copyright 2012 + * Pavel Herrmann <morpheus.ibis@gmail.com> + * Marek Vasut <marex@denx.de> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef _DM_STRUCTURES_H_ +#define _DM_STRUCTURES_H_ + +#include <common.h> +#include <errno.h> +#include <linker_lists.h> +#include <dm/uclass.h> +#include <linux/list.h> + +struct driver_info { + const char *name; + const void *platform_data; +}; + +#define DM_FLAG_ACTIVATED 1 + +struct device { + struct u_boot_driver *driver; + const struct driver_info *info; + struct device *bus; + void *priv; + struct list_head class_node; /* attach instances to class */ + struct list_head succ; /* list of all instances */ + struct list_head sibling; /* siblings */ + uint32_t flags; +}; + +#define device_get_ops(dev) (dev->driver->ops) + +/* structures for driver manager */ + +enum aux_data_type { + U_BOOT_DRIVER_AUX_NONE = 0, + U_BOOT_DRIVER_AUX_USB, + U_BOOT_DRIVER_AUX_PCI, +}; + +struct u_boot_driver { + char *name; + enum uclass_id id; + int (*bind)(struct device *i); + int (*probe)(struct device *i); + int (*reloc)(struct device *new, struct device *old); + int (*remove)(struct device *i); + int (*unbind)(struct device *i); + int priv_data_size; + const void *ops; /* driver-specific operations */ + enum aux_data_type aux_data_type; + void *aux_data; +}; + +struct usb_ids { + int (*match)(void); + struct { + uint16_t major; + uint16_t minor; + } ids[]; +}; + +/* + * The USB driver has special needs, hence the separate type of driver. + * Yet, the struct u_boot_driver must be located at the first place, so the + * generic searching functions can be used. + */ +struct u_boot_usb_driver { + struct u_boot_driver driver; + struct usb_ids *ids; +}; + +#define U_BOOT_DRIVER(__name) \ + ll_entry_declare(struct u_boot_driver, __name, driver) + +#endif diff --git a/include/dm/uclass.h b/include/dm/uclass.h new file mode 100644 index 0000000..ce040e7 --- /dev/null +++ b/include/dm/uclass.h @@ -0,0 +1,62 @@ +/* + * (C) Copyright 2012 + * Pavel Herrmann <morpheus.ibis@gmail.com> + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef _DM_UCLASS_H +#define _DM_UCLASS_H + +#include <linux/list.h> + +/* TODO: this could be compile-time generated */ +enum uclass_id { + UCLASS_GPIO = 0, + UCLASS_DEMO, + + UCLASS_COUNT, + UCLASS_INVALID = -1, +}; + +struct uclass { + void *priv; + struct u_boot_class *dm_class; + struct list_head class_node; /* instances for this class */ + struct list_head succ; +}; + +struct device; + +struct u_boot_class { + enum uclass_id id; + int (*bind)(struct uclass *class, struct device *dev); + int (*unbind)(struct uclass *class, struct device *dev); + int (*init)(struct uclass *class); + int (*reloc)(struct uclass *new, struct uclass *old); + int (*destroy)(struct uclass *class); + int (*replace)(struct uclass *class, struct device *new, + struct device *old); + const void *ops; /* class-specific operations */ +}; + +#define U_BOOT_CLASS(__name) \ + ll_entry_declare(struct u_boot_class, __name, class) + +#endif