Hi Simon,
2015-07-09 9:22 GMT+09:00 Simon Glass <[email protected]>: > Hi Masahiro, > > On 7 July 2015 at 22:29, Masahiro Yamada <[email protected]> > wrote: >> In U-Boot's driver model, memory is basically allocated and freed >> in the core framework. So, low level drivers generally only have >> to specify the size of needed memory with .priv_auto_alloc_size, >> .platdata_auto_alloc_size, etc. Nevertheless, some drivers still >> need to allocate memory on their own in case they cannot statically >> know how much memory is needed. Moreover, I am afraid the failure >> paths of driver model core parts are getting messier as more and >> more memory size members are supported, .per_child_auto_alloc_size, >> .per_child_platdata_auto_alloc_size... So, I believe it is >> reasonable enough to port Devres into U-boot. >> >> As you know, Devres, which originates in Linux, manages device >> resources for each device and automatically releases them on driver >> detach. With devres, device resources are guaranteed to be freed >> whether initialization fails half-way or the device gets detached. >> >> The basic idea is totally the same to that of Linux, but I tweaked >> it a bit so that it fits in U-Boot's driver model. >> >> In U-Boot, drivers are activated in two steps: binding and probing. >> Binding puts a driver and a device together. It is just data >> manipulation on the system memory, so nothing has happened on the >> hardware device at this moment. When the device is really used, it >> is probed. Probing initializes the real hardware device to make it >> really ready for use. >> >> So, the resources acquired during the probing process must be freed >> when the device is removed. Likewise, what has been allocated in >> binding should be released when the device is unbound. The struct >> devres has a member "probe" to remember when the resource was >> allocated. >> >> CONFIG_DEBUG_DEVRES is also supported for easier debugging. >> If enabled, debug messages are printed each time a resource is >> allocated/freed. >> >> Signed-off-by: Masahiro Yamada <[email protected]> > > A few points to make: > > 1. I don't think we will be adding a lot more types of memory attached > to devices. We have: > > - device private data > - uclass' private data for the device > - parent's private data for the device > > That is all set up in device_probe() and friends. Then we have > platform data for the above which is set up in device_bind(). Within > the driver model architecture it's hard to see another type of data > coming along although of course I would not rule it out. > > 2. The auto-allocation feature of driver model has actually been very > successful at removing the need for drivers to do their own memory > allocation. > > git grep -l U_BOOT_DRIVER |wc > 105 > > grep '[mc]alloc(' `git grep -l U_BOOT_DRIVER ` |wc > 20 > > and a quick check suggests that half of those 10 are bogus (could be > redone to avoid a special malloc()). > > So I don't think the devm functions will be used much. Right. > 3. How do we handle things like gpio_exynos_bind() which allocs some > data and passes it to a device it creates, as platform data? At > present we don't free it. So, currently this driver is leaking the memory, isn't it? If we use devm_kzalloc() here, the platdata for GPIOs will be released when the parent pinctrl is unbound. > 4. There is a data size overhead to this which is not insignificant. > As I read it we add 16 bytes to each memory allocation, which for most > devices will amount to 32 or 48 bytes. Currently struct udevice is 84 > bytes so increasing the overhead by 50% for no real improvement in > functionality. This does matter in SPL in some cases. > > With all that said I think overall this is a good and useful change. I > can see it saving hassle later. > > So, can we reduce the overhead / turn it off for SPL? Perhaps it could > dissolve to nothing if CONFIG_DM_DEVICE_REMOVE is not defined? I think I can do this. devres.c can be built (and makes sense) only when CONFIG_DM_DEVICE_REMOVE is enabled. > As it happens I started yesterday on a change to check driver model > pointers. I've been talking about it for a while but there are enough > drivers out there that I think it is worth doing now. I hope to have > something next week. However it doesn't look like it will interfere > with your change much. > > BTW can we please have exported functions documented in the header file? IIRC, when we discussed this before, we could not reach agreement which should be documented, headers or sources. I know you prefer comments in headers, while I prefer in sources (like Linux). I can move them to dm/device.h if you think it is important to be consistent in the DM core portion. -- Best Regards Masahiro Yamada _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
