[U-Boot] [RFC PATCH] dm: device: Do not probe parents which are probed already
From the first look there is no reason to probe parent nodes if they are
active already.
Signed-off-by: Michal Simek michal.simek@xilinx.com ---
I have created this just for showing status of parent device. Maybe there is any strong reason to do this but I just wanted to check this because it looks like just wasting of time.
Just revert this condition when you want to see outputs. if (dev->parent && !(dev->parent->flags & DM_FLAG_ACTIVATED)) {
Without this line
99 amba @ 7df04d20 100 amba @ 7df04d20 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) MMC: 99 * amba @ 7df04d20, seq 0, (req -1) 100 * amba @ 7df04d20, seq 0, (req -1)
ZynqMP> i2c dev 0 Setting bus to 0 99 * amba @ 7df04d20, seq 0, (req -1) 100 * amba @ 7df04d20, seq 0, (req -1)
with this line added
99 amba @ 7df04d20 100 amba @ 7df04d20 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) MMC: 99 * amba @ 7df04d20, seq 0, (req -1)
ZynqMP> i2c dev 0 Setting bus to 0 99 * amba @ 7df04d20, seq 0, (req -1)
--- drivers/core/device.c | 7 ++++++- drivers/core/dump.c | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 0d15e5062b66..114888a8f7cf 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -341,8 +341,13 @@ int device_probe(struct udevice *dev) } }
+ if (dev->parent) + dm_display_line(dev->parent, 99); + /* Ensure all parents are probed */ - if (dev->parent) { + if (dev->parent && !(dev->parent->flags & DM_FLAG_ACTIVATED)) { + dm_display_line(dev->parent, 100); + size = dev->parent->driver->per_child_auto_alloc_size; if (!size) { size = dev->parent->uclass->uc_drv-> diff --git a/drivers/core/dump.c b/drivers/core/dump.c index 8fbfd93fb5e4..95ba7dcb9193 100644 --- a/drivers/core/dump.c +++ b/drivers/core/dump.c @@ -62,7 +62,7 @@ void dm_dump_all(void) * * @dev: Device to display */ -static void dm_display_line(struct udevice *dev, int index) +void dm_display_line(struct udevice *dev, int index) { printf("%i %c %s @ %08lx", index, dev->flags & DM_FLAG_ACTIVATED ? '*' : ' ',
Hi Michal,
On Fri, 18 Jan 2019 at 02:41, Michal Simek michal.simek@xilinx.com wrote:
From the first look there is no reason to probe parent nodes if they are active already.
Signed-off-by: Michal Simek michal.simek@xilinx.com
I have created this just for showing status of parent device. Maybe there is any strong reason to do this but I just wanted to check this because it looks like just wasting of time.
Just revert this condition when you want to see outputs. if (dev->parent && !(dev->parent->flags & DM_FLAG_ACTIVATED)) {
Without this line
99 amba @ 7df04d20 100 amba @ 7df04d20 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) MMC: 99 * amba @ 7df04d20, seq 0, (req -1) 100 * amba @ 7df04d20, seq 0, (req -1)
ZynqMP> i2c dev 0 Setting bus to 0 99 * amba @ 7df04d20, seq 0, (req -1) 100 * amba @ 7df04d20, seq 0, (req -1)
with this line added
99 amba @ 7df04d20 100 amba @ 7df04d20 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) MMC: 99 * amba @ 7df04d20, seq 0, (req -1)
ZynqMP> i2c dev 0 Setting bus to 0 99 * amba @ 7df04d20, seq 0, (req -1)
drivers/core/device.c | 7 ++++++- drivers/core/dump.c | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 0d15e5062b66..114888a8f7cf 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -341,8 +341,13 @@ int device_probe(struct udevice *dev) } }
if (dev->parent)dm_display_line(dev->parent, 99);/* Ensure all parents are probed */
if (dev->parent) {
if (dev->parent && !(dev->parent->flags & DM_FLAG_ACTIVATED)) {dm_display_line(dev->parent, 100);
Yes this looks like a good change in principle.
But we still need to execute the code below even if the parent is probed, so that we allocate the child's parent data:
size = dev->parent->driver->per_child_auto_alloc_size; if (!size) { size = dev->parent->uclass->uc_drv->
...
So can you please rework this to allow for that?
Overall I think your change saves a function call. As you can see the flag is checked right at the top of device_probe().
diff --git a/drivers/core/dump.c b/drivers/core/dump.c index 8fbfd93fb5e4..95ba7dcb9193 100644 --- a/drivers/core/dump.c +++ b/drivers/core/dump.c @@ -62,7 +62,7 @@ void dm_dump_all(void)
- @dev: Device to display
*/ -static void dm_display_line(struct udevice *dev, int index) +void dm_display_line(struct udevice *dev, int index) { printf("%i %c %s @ %08lx", index, dev->flags & DM_FLAG_ACTIVATED ? '*' : ' ', -- 1.9.1
Regards, Simon
On 31. 01. 19 11:04, Simon Glass wrote:
Hi Michal,
On Fri, 18 Jan 2019 at 02:41, Michal Simek michal.simek@xilinx.com wrote:
From the first look there is no reason to probe parent nodes if they are active already.
Signed-off-by: Michal Simek michal.simek@xilinx.com
I have created this just for showing status of parent device. Maybe there is any strong reason to do this but I just wanted to check this because it looks like just wasting of time.
Just revert this condition when you want to see outputs. if (dev->parent && !(dev->parent->flags & DM_FLAG_ACTIVATED)) {
Without this line
99 amba @ 7df04d20 100 amba @ 7df04d20 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) MMC: 99 * amba @ 7df04d20, seq 0, (req -1) 100 * amba @ 7df04d20, seq 0, (req -1)
ZynqMP> i2c dev 0 Setting bus to 0 99 * amba @ 7df04d20, seq 0, (req -1) 100 * amba @ 7df04d20, seq 0, (req -1)
with this line added
99 amba @ 7df04d20 100 amba @ 7df04d20 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) MMC: 99 * amba @ 7df04d20, seq 0, (req -1)
ZynqMP> i2c dev 0 Setting bus to 0 99 * amba @ 7df04d20, seq 0, (req -1)
drivers/core/device.c | 7 ++++++- drivers/core/dump.c | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 0d15e5062b66..114888a8f7cf 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -341,8 +341,13 @@ int device_probe(struct udevice *dev) } }
if (dev->parent)dm_display_line(dev->parent, 99);/* Ensure all parents are probed */
if (dev->parent) {
if (dev->parent && !(dev->parent->flags & DM_FLAG_ACTIVATED)) {dm_display_line(dev->parent, 100);Yes this looks like a good change in principle.
But we still need to execute the code below even if the parent is probed, so that we allocate the child's parent data:
ok.
size = dev->parent->driver->per_child_auto_alloc_size; if (!size) { size = dev->parent->uclass->uc_drv->...
So can you please rework this to allow for that?
Overall I think your change saves a function call. As you can see the flag is checked right at the top of device_probe().
I am not quite sure how that rework should look like.
If just this. if (!(dev->parent->flags & DM_FLAG_ACTIVATED)) ret = device_probe(dev->parent); if (ret) goto fail;
Then improvement will be very minimal.
Thanks, Michal
Hi Michal,
On Thu, 31 Jan 2019 at 03:28, Michal Simek michal.simek@xilinx.com wrote:
On 31. 01. 19 11:04, Simon Glass wrote:
Hi Michal,
On Fri, 18 Jan 2019 at 02:41, Michal Simek michal.simek@xilinx.com
wrote:
From the first look there is no reason to probe parent nodes if they
are
active already.
Signed-off-by: Michal Simek michal.simek@xilinx.com
I have created this just for showing status of parent device. Maybe there is any strong reason to do this but I just wanted to check this because it looks like just wasting of time.
Just revert this condition when you want to see outputs. if (dev->parent && !(dev->parent->flags & DM_FLAG_ACTIVATED)) {
Without this line
99 amba @ 7df04d20 100 amba @ 7df04d20 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) MMC: 99 * amba @ 7df04d20, seq 0, (req -1) 100 * amba @ 7df04d20, seq 0, (req -1)
ZynqMP> i2c dev 0 Setting bus to 0 99 * amba @ 7df04d20, seq 0, (req -1) 100 * amba @ 7df04d20, seq 0, (req -1)
with this line added
99 amba @ 7df04d20 100 amba @ 7df04d20 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) MMC: 99 * amba @ 7df04d20, seq 0, (req -1)
ZynqMP> i2c dev 0 Setting bus to 0 99 * amba @ 7df04d20, seq 0, (req -1)
drivers/core/device.c | 7 ++++++- drivers/core/dump.c | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 0d15e5062b66..114888a8f7cf 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -341,8 +341,13 @@ int device_probe(struct udevice *dev) } }
if (dev->parent)dm_display_line(dev->parent, 99);/* Ensure all parents are probed */
if (dev->parent) {
if (dev->parent && !(dev->parent->flags & DM_FLAG_ACTIVATED)) {dm_display_line(dev->parent, 100);Yes this looks like a good change in principle.
But we still need to execute the code below even if the parent is probed, so that we allocate the child's parent data:
ok.
size = dev->parent->driver->per_child_auto_alloc_size; if (!size) { size = dev->parent->uclass->uc_drv->...
So can you please rework this to allow for that?
Overall I think your change saves a function call. As you can see the flag is checked right at the top of device_probe().
I am not quite sure how that rework should look like.
If just this. if (!(dev->parent->flags & DM_FLAG_ACTIVATED)) ret = device_probe(dev->parent); if (ret) goto fail;
Then improvement will be very minimal.
Yes indeed, it is just saving a function call.
Regards, Simon
On 02. 02. 19 7:05, Simon Glass wrote:
Hi Michal,
On Thu, 31 Jan 2019 at 03:28, Michal Simek michal.simek@xilinx.com wrote:
On 31. 01. 19 11:04, Simon Glass wrote:
Hi Michal,
On Fri, 18 Jan 2019 at 02:41, Michal Simek michal.simek@xilinx.com
wrote:
From the first look there is no reason to probe parent nodes if they
are
active already.
Signed-off-by: Michal Simek michal.simek@xilinx.com
I have created this just for showing status of parent device. Maybe there is any strong reason to do this but I just wanted to check this because it looks like just wasting of time.
Just revert this condition when you want to see outputs. if (dev->parent && !(dev->parent->flags & DM_FLAG_ACTIVATED)) {
Without this line
99 amba @ 7df04d20 100 amba @ 7df04d20 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 100 * root_driver @ 7df04960, seq 0, (req -1) MMC: 99 * amba @ 7df04d20, seq 0, (req -1) 100 * amba @ 7df04d20, seq 0, (req -1)
ZynqMP> i2c dev 0 Setting bus to 0 99 * amba @ 7df04d20, seq 0, (req -1) 100 * amba @ 7df04d20, seq 0, (req -1)
with this line added
99 amba @ 7df04d20 100 amba @ 7df04d20 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) 99 * root_driver @ 7df04960, seq 0, (req -1) MMC: 99 * amba @ 7df04d20, seq 0, (req -1)
ZynqMP> i2c dev 0 Setting bus to 0 99 * amba @ 7df04d20, seq 0, (req -1)
drivers/core/device.c | 7 ++++++- drivers/core/dump.c | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 0d15e5062b66..114888a8f7cf 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -341,8 +341,13 @@ int device_probe(struct udevice *dev) } }
if (dev->parent)dm_display_line(dev->parent, 99);/* Ensure all parents are probed */
if (dev->parent) {
if (dev->parent && !(dev->parent->flags & DM_FLAG_ACTIVATED)) {dm_display_line(dev->parent, 100);Yes this looks like a good change in principle.
But we still need to execute the code below even if the parent is probed, so that we allocate the child's parent data:
ok.
size = dev->parent->driver->per_child_auto_alloc_size; if (!size) { size = dev->parent->uclass->uc_drv->...
So can you please rework this to allow for that?
Overall I think your change saves a function call. As you can see the flag is checked right at the top of device_probe().
I am not quite sure how that rework should look like.
If just this. if (!(dev->parent->flags & DM_FLAG_ACTIVATED)) ret = device_probe(dev->parent); if (ret) goto fail;
Then improvement will be very minimal.
Yes indeed, it is just saving a function call.
thanks for confirmation. It means let's close this RFC that this is no needed and it is an issue.
Thanks, Michal
participants (2)
-
Michal Simek -
Simon Glass