| Message ID | 71412095f94231421e227d0eb73d7170b9d3e5bd.1547804471.git.michal.simek@xilinx.com |
|---|---|
| State | Rejected |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [U-Boot,RFC] dm: device: Do not probe parents which are probed already | expand |
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
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 ? '*' : ' ',
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(-)