[U-Boot] [RFC PATCH] dm: device: Do not probe parents which are probed already

[Michal Simek]

On 02. 02. 19 7:05, Simon Glass wrote:
> Hi Michal,
> 
> On Thu, 31 Jan 2019 at 03:28, Michal Simek <michal.simek at 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 at 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 at 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