Re: [U-Boot] [PATCH v2 06/26] dm: core: Allocate platform data when binding a device
Hi Masahiro, On 23 January 2015 at 22:04, Masahiro YAMADA wrote: > Hi Simon, > > > 2015-01-24 0:50 GMT+09:00 Simon Glass : > >> >> I tried to document the reasoning in the patches, but let me try to >> expand a bit. Hopefully this can provoke further comments / >> improvements. >> >> The main motivation for me was that buses want to set up the platform >> data for their children before they are probed. In fact some children >> may never be probed. For example a SPI bus wants to know the chip >> select for each of its children. >> >> At present we have to hunt around in the device tree to figure out >> which child is the right one, so we can probe it. Worse, the >> children's drivers (e.g. cros_ec on a SPI bus) have to be involved in >> setting themselves up. The device_probe_child() function was my >> attempt to make this fit better, and it did work, but I was not happy >> with it. You can see that from my unhappy comments in cros_ec, or SPI >> flash probe, for example. >> >> The new approach makes buses easier to deal with as I hope you can >> see. The 'bind' step is supposed to set up the entire basic framework >> of the drivers at start-up. Everything should be visible in the tree >> (the exception being buses which must be probed at run-time) but >> nothing should be probed. Now, we are following that approach for >> buses also. > > OK. > I understand that this concept makes things much simpler, so > > Reviewed-by: Masahiro Yamada > OK I will respin this series. > > Perhaps, we can have a flag like DM_FLAG_ALLOC_PLATDATA_LATE: > If this flag is set, platdata is allocated in device_probe(), not > device_bind(). > But, I think it might make things much more complicated and > probably make you unhappy. > I do not have a strong option about this. > Well I'm keen on anything that makes it more efficient, but yes if we can keep it simple we should do that for now. Let's see how things pan out. > > Let's go with your idea! > If something does not go well, then we can come back here. > > Yes we can. > > >> Re the disadvantages: >> >> - the per-child platform data for a bus is small, and we need not bind >> devices which are disabled. So, a board should avoid having a lot of >> enabled devices which are never used >> - malloc() is very fast, it is the platform data setup that takes >> time. I agree this slows things down. But currently it only affects >> bus children, and only their basic information such as chip select. >> >> The use of ofdata_to_platdata() is stil confined to when the device is >> actually probed. This helps to keep things fast in the common case >> where we don't need the platform data earlier. >> >> I think we can refine this further, but what I have now feels a lot >> cleaner to me. > > OK! > > > > I have done with a quick review of this series. > > I left some comments on some patches,but they are all minor issues. > > If you agree to fix them, please send v3. > I am OK with the whole concept, so I guess we can get it in during this MW. Will do. Thanks very much for going through this in detail. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 06/26] dm: core: Allocate platform data when binding a device
Hi Simon, 2015-01-24 0:50 GMT+09:00 Simon Glass : > > I tried to document the reasoning in the patches, but let me try to > expand a bit. Hopefully this can provoke further comments / > improvements. > > The main motivation for me was that buses want to set up the platform > data for their children before they are probed. In fact some children > may never be probed. For example a SPI bus wants to know the chip > select for each of its children. > > At present we have to hunt around in the device tree to figure out > which child is the right one, so we can probe it. Worse, the > children's drivers (e.g. cros_ec on a SPI bus) have to be involved in > setting themselves up. The device_probe_child() function was my > attempt to make this fit better, and it did work, but I was not happy > with it. You can see that from my unhappy comments in cros_ec, or SPI > flash probe, for example. > > The new approach makes buses easier to deal with as I hope you can > see. The 'bind' step is supposed to set up the entire basic framework > of the drivers at start-up. Everything should be visible in the tree > (the exception being buses which must be probed at run-time) but > nothing should be probed. Now, we are following that approach for > buses also. OK. I understand that this concept makes things much simpler, so Reviewed-by: Masahiro Yamada Perhaps, we can have a flag like DM_FLAG_ALLOC_PLATDATA_LATE: If this flag is set, platdata is allocated in device_probe(), not device_bind(). But, I think it might make things much more complicated and probably make you unhappy. I do not have a strong option about this. Let's go with your idea! If something does not go well, then we can come back here. > Re the disadvantages: > > - the per-child platform data for a bus is small, and we need not bind > devices which are disabled. So, a board should avoid having a lot of > enabled devices which are never used > - malloc() is very fast, it is the platform data setup that takes > time. I agree this slows things down. But currently it only affects > bus children, and only their basic information such as chip select. > > The use of ofdata_to_platdata() is stil confined to when the device is > actually probed. This helps to keep things fast in the common case > where we don't need the platform data earlier. > > I think we can refine this further, but what I have now feels a lot > cleaner to me. OK! I have done with a quick review of this series. I left some comments on some patches,but they are all minor issues. If you agree to fix them, please send v3. I am OK with the whole concept, so I guess we can get it in during this MW. Thanks! -- Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 06/26] dm: core: Allocate platform data when binding a device
Hi Masahiro, On 23 January 2015 at 02:20, Masahiro Yamada wrote: > Hi Simon, > > > On Mon, 19 Jan 2015 20:12:35 -0700 > Simon Glass wrote: > >> When using allocated platform data, allocate it when we bind the device. >> This makes it possible to fill in this information before the device is >> probed. >> >> This fits with the platform data model (when not using device tree), >> since platform data exists at bind-time. >> >> Signed-off-by: Simon Glass > > > Looks like you have changed your mind > to allocate platdata in device_bind() rather than device_probe(). Yes. In fact I think my attempts to avoid this were a little too heroic. > > > Could you explain why you think this should be done? > > I might have missed something, but your motivation is still unclear to me. > > I thought one reason is consistency with platform data. > > But drv->ofdata_to_platdata() is still called from device_probe(), > i.e. it is just like zero-filled memory is allocated at the binding stage. > Filling it with real device properties is delayed until the device is probed. > What is the difference from the before and what does it buy us? > > Its disadvantage are clear: > - We need more malloc memory for devices that may or may not be used. > - The boot sequence becomes slower. > > I want good reasons to compensate these disadvantages. > > I tried to document the reasoning in the patches, but let me try to expand a bit. Hopefully this can provoke further comments / improvements. The main motivation for me was that buses want to set up the platform data for their children before they are probed. In fact some children may never be probed. For example a SPI bus wants to know the chip select for each of its children. At present we have to hunt around in the device tree to figure out which child is the right one, so we can probe it. Worse, the children's drivers (e.g. cros_ec on a SPI bus) have to be involved in setting themselves up. The device_probe_child() function was my attempt to make this fit better, and it did work, but I was not happy with it. You can see that from my unhappy comments in cros_ec, or SPI flash probe, for example. The new approach makes buses easier to deal with as I hope you can see. The 'bind' step is supposed to set up the entire basic framework of the drivers at start-up. Everything should be visible in the tree (the exception being buses which must be probed at run-time) but nothing should be probed. Now, we are following that approach for buses also. Re the disadvantages: - the per-child platform data for a bus is small, and we need not bind devices which are disabled. So, a board should avoid having a lot of enabled devices which are never used - malloc() is very fast, it is the platform data setup that takes time. I agree this slows things down. But currently it only affects bus children, and only their basic information such as chip select. The use of ofdata_to_platdata() is stil confined to when the device is actually probed. This helps to keep things fast in the common case where we don't need the platform data earlier. I think we can refine this further, but what I have now feels a lot cleaner to me. > > > BTW, you missed to fix the comment in device_probe_child(): > > /* Allocate private data and platdata if requested */ OK. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 06/26] dm: core: Allocate platform data when binding a device
Hi Simon, On Mon, 19 Jan 2015 20:12:35 -0700 Simon Glass wrote: > When using allocated platform data, allocate it when we bind the device. > This makes it possible to fill in this information before the device is > probed. > > This fits with the platform data model (when not using device tree), > since platform data exists at bind-time. > > Signed-off-by: Simon Glass Looks like you have changed your mind to allocate platdata in device_bind() rather than device_probe(). Could you explain why you think this should be done? I might have missed something, but your motivation is still unclear to me. I thought one reason is consistency with platform data. But drv->ofdata_to_platdata() is still called from device_probe(), i.e. it is just like zero-filled memory is allocated at the binding stage. Filling it with real device properties is delayed until the device is probed. What is the difference from the before and what does it buy us? Its disadvantage are clear: - We need more malloc memory for devices that may or may not be used. - The boot sequence becomes slower. I want good reasons to compensate these disadvantages. BTW, you missed to fix the comment in device_probe_child(): /* Allocate private data and platdata if requested */ Best Regards Masahiro Yamada ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2 06/26] dm: core: Allocate platform data when binding a device
When using allocated platform data, allocate it when we bind the device.
This makes it possible to fill in this information before the device is
probed.
This fits with the platform data model (when not using device tree),
since platform data exists at bind-time.
Signed-off-by: Simon Glass
---
Changes in v2: None
drivers/core/device-remove.c | 8
drivers/core/device.c| 20
test/dm/test-fdt.c | 4 ++--
3 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index 8fc6b71..2c82577 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -88,6 +88,10 @@ int device_unbind(struct udevice *dev)
if (ret)
return ret;
+ if (dev->flags & DM_FLAG_ALLOC_PDATA) {
+ free(dev->platdata);
+ dev->platdata = NULL;
+ }
ret = uclass_unbind_device(dev);
if (ret)
return ret;
@@ -111,10 +115,6 @@ void device_free(struct udevice *dev)
free(dev->priv);
dev->priv = NULL;
}
- if (dev->flags & DM_FLAG_ALLOC_PDATA) {
- free(dev->platdata);
- dev->platdata = NULL;
- }
size = dev->uclass->uc_drv->per_device_auto_alloc_size;
if (size) {
free(dev->uclass_priv);
diff --git a/drivers/core/device.c b/drivers/core/device.c
index eca8eda..23ee771 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -72,8 +72,14 @@ int device_bind(struct udevice *parent, struct driver *drv,
const char *name,
#else
dev->req_seq = -1;
#endif
- if (!dev->platdata && drv->platdata_auto_alloc_size)
+ if (!dev->platdata && drv->platdata_auto_alloc_size) {
dev->flags |= DM_FLAG_ALLOC_PDATA;
+ dev->platdata = calloc(1, drv->platdata_auto_alloc_size);
+ if (!dev->platdata) {
+ ret = -ENOMEM;
+ goto fail_alloc1;
+ }
+ }
/* put dev into parent's successor list */
if (parent)
@@ -103,6 +109,11 @@ fail_bind:
fail_uclass_bind:
if (parent)
list_del(&dev->sibling_node);
+ if (dev->flags & DM_FLAG_ALLOC_PDATA) {
+ free(dev->platdata);
+ dev->platdata = NULL;
+ }
+fail_alloc1:
free(dev);
return ret;
@@ -148,13 +159,6 @@ int device_probe_child(struct udevice *dev, void
*parent_priv)
}
}
/* Allocate private data if requested */
- if (dev->flags & DM_FLAG_ALLOC_PDATA) {
- dev->platdata = calloc(1, drv->platdata_auto_alloc_size);
- if (!dev->platdata) {
- ret = -ENOMEM;
- goto fail;
- }
- }
size = dev->uclass->uc_drv->per_device_auto_alloc_size;
if (size) {
dev->uclass_priv = calloc(1, size);
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index cd2c389..dc4ebf9 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -143,12 +143,12 @@ static int dm_test_fdt(struct dm_test_state *dms)
/* These are num_devices compatible root-level device tree nodes */
ut_asserteq(num_devices, list_count_items(&uc->dev_head));
- /* Each should have no platdata / priv */
+ /* Each should have platform data but no private data */
for (i = 0; i < num_devices; i++) {
ret = uclass_find_device(UCLASS_TEST_FDT, i, &dev);
ut_assert(!ret);
ut_assert(!dev_get_priv(dev));
- ut_assert(!dev->platdata);
+ ut_assert(dev->platdata);
}
ut_assertok(dm_check_devices(dms, num_devices));
--
2.2.0.rc0.207.ga3a616c
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
