[1/6] event: signal when livetree has been built

OF_LIVE offers a variety of benefits, one of them being that the live
tree can be modified without caring about the underlying FDT. This is
particularly valuable for working around U-Boot limitations like lacking
USB superspeed support on Qualcomm platforms, no runtime OTG, or
peripherals like the sdcard being broken (and displaying potentially
worrying error messages).

Add an event to signal when the live tree has been built so that we can
apply fixups to it directly before devices are bound.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 common/event.c  | 3 +++
 include/event.h | 9 +++++++++
 lib/of_live.c   | 3 +++
 3 files changed, 15 insertions(+)

On Wed, Apr 09, 2025 at 07:17:24PM +0200, Caleb Connolly wrote:
> OF_LIVE offers a variety of benefits, one of them being that the live
> tree can be modified without caring about the underlying FDT. This is
> particularly valuable for working around U-Boot limitations like lacking
> USB superspeed support on Qualcomm platforms, no runtime OTG, or
> peripherals like the sdcard being broken (and displaying potentially
> worrying error messages).
> 
> Add an event to signal when the live tree has been built so that we can
> apply fixups to it directly before devices are bound.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  common/event.c  | 3 +++
>  include/event.h | 9 +++++++++
>  lib/of_live.c   | 3 +++
>  3 files changed, 15 insertions(+)
>

Acked-by: Sumit Garg <sumit.garg@oss.qualcomm.com>

-Sumit

> diff --git a/common/event.c b/common/event.c
> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
> --- a/common/event.c
> +++ b/common/event.c
> @@ -47,8 +47,11 @@ const char *const type_name[] = {
>  	"ft_fixup",
>  
>  	/* main loop events */
>  	"main_loop",
> +
> +	/* livetree has been built */
> +	"of_live_init",
>  };
>  
>  _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
>  #endif
> diff --git a/include/event.h b/include/event.h
> index 75141a192a48b0931667632f41be8ff4d6139f7c..3fc673ba635ed45467aae8587705d37bef1c2a3f 100644
> --- a/include/event.h
> +++ b/include/event.h
> @@ -152,8 +152,17 @@ enum event_t {
>  	 * A non-zero return value causes the boot to fail.
>  	 */
>  	EVT_MAIN_LOOP,
>  
> +	/**
> +	 * @EVT_OF_LIVE_INIT:
> +	 * This event is triggered immediately after the live device tree has been
> +	 * built. This allows for machine specific fixups to be done to the live tree
> +	 * (like disabling known-unsupported devices) before DM init happens. This
> +	 * event is only available if OF_LIVE is enabled and is only used after relocation.
> +	 */
> +	EVT_OF_LIVE_INIT,
> +
>  	/**
>  	 * @EVT_COUNT:
>  	 * This constants holds the maximum event number + 1 and is used when
>  	 * looping over all event classes.
> diff --git a/lib/of_live.c b/lib/of_live.c
> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a 100644
> --- a/lib/of_live.c
> +++ b/lib/of_live.c
> @@ -10,8 +10,9 @@
>  
>  #define LOG_CATEGORY	LOGC_DT
>  
>  #include <abuf.h>
> +#include <event.h>
>  #include <log.h>
>  #include <linux/libfdt.h>
>  #include <of_live.h>
>  #include <malloc.h>
> @@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
>  		return ret;
>  	}
>  	debug("%s: stop\n", __func__);
>  
> +	event_notify_null(EVT_OF_LIVE_INIT);
> +
>  	return ret;
>  }
>  
>  void of_live_free(struct device_node *root)
> 
> -- 
> 2.49.0
>

On 09/04/2025 19:17, Caleb Connolly wrote:
> OF_LIVE offers a variety of benefits, one of them being that the live
> tree can be modified without caring about the underlying FDT. This is
> particularly valuable for working around U-Boot limitations like lacking
> USB superspeed support on Qualcomm platforms, no runtime OTG, or
> peripherals like the sdcard being broken (and displaying potentially
> worrying error messages).
> 
> Add an event to signal when the live tree has been built so that we can
> apply fixups to it directly before devices are bound.
> 
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>   common/event.c  | 3 +++
>   include/event.h | 9 +++++++++
>   lib/of_live.c   | 3 +++
>   3 files changed, 15 insertions(+)
> 
> diff --git a/common/event.c b/common/event.c
> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
> --- a/common/event.c
> +++ b/common/event.c
> @@ -47,8 +47,11 @@ const char *const type_name[] = {
>   	"ft_fixup",
>   
>   	/* main loop events */
>   	"main_loop",
> +
> +	/* livetree has been built */
> +	"of_live_init",
>   };
>   
>   _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
>   #endif
> diff --git a/include/event.h b/include/event.h
> index 75141a192a48b0931667632f41be8ff4d6139f7c..3fc673ba635ed45467aae8587705d37bef1c2a3f 100644
> --- a/include/event.h
> +++ b/include/event.h
> @@ -152,8 +152,17 @@ enum event_t {
>   	 * A non-zero return value causes the boot to fail.
>   	 */
>   	EVT_MAIN_LOOP,
>   
> +	/**
> +	 * @EVT_OF_LIVE_INIT:
> +	 * This event is triggered immediately after the live device tree has been
> +	 * built. This allows for machine specific fixups to be done to the live tree
> +	 * (like disabling known-unsupported devices) before DM init happens. This
> +	 * event is only available if OF_LIVE is enabled and is only used after relocation.
> +	 */
> +	EVT_OF_LIVE_INIT,
> +
>   	/**
>   	 * @EVT_COUNT:
>   	 * This constants holds the maximum event number + 1 and is used when
>   	 * looping over all event classes.
> diff --git a/lib/of_live.c b/lib/of_live.c
> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a 100644
> --- a/lib/of_live.c
> +++ b/lib/of_live.c
> @@ -10,8 +10,9 @@
>   
>   #define LOG_CATEGORY	LOGC_DT
>   
>   #include <abuf.h>
> +#include <event.h>
>   #include <log.h>
>   #include <linux/libfdt.h>
>   #include <of_live.h>
>   #include <malloc.h>
> @@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
>   		return ret;
>   	}
>   	debug("%s: stop\n", __func__);
>   
> +	event_notify_null(EVT_OF_LIVE_INIT);
> +
>   	return ret;
>   }
>   
>   void of_live_free(struct device_node *root)
> 

Looks good :-)

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Hi Caleb,

On Wed, 9 Apr 2025 at 11:17, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
> OF_LIVE offers a variety of benefits, one of them being that the live
> tree can be modified without caring about the underlying FDT. This is
> particularly valuable for working around U-Boot limitations like lacking
> USB superspeed support on Qualcomm platforms, no runtime OTG, or
> peripherals like the sdcard being broken (and displaying potentially
> worrying error messages).
>
> Add an event to signal when the live tree has been built so that we can
> apply fixups to it directly before devices are bound.
>
> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  common/event.c  | 3 +++
>  include/event.h | 9 +++++++++
>  lib/of_live.c   | 3 +++
>  3 files changed, 15 insertions(+)
>
> diff --git a/common/event.c b/common/event.c
> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
> --- a/common/event.c
> +++ b/common/event.c
> @@ -47,8 +47,11 @@ const char *const type_name[] = {
>         "ft_fixup",
>
>         /* main loop events */
>         "main_loop",
> +
> +       /* livetree has been built */
> +       "of_live_init",
>  };
>
>  _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
>  #endif
> diff --git a/include/event.h b/include/event.h
> index 75141a192a48b0931667632f41be8ff4d6139f7c..3fc673ba635ed45467aae8587705d37bef1c2a3f 100644
> --- a/include/event.h
> +++ b/include/event.h
> @@ -152,8 +152,17 @@ enum event_t {
>          * A non-zero return value causes the boot to fail.
>          */
>         EVT_MAIN_LOOP,
>
> +       /**
> +        * @EVT_OF_LIVE_INIT:
> +        * This event is triggered immediately after the live device tree has been
> +        * built. This allows for machine specific fixups to be done to the live tree
> +        * (like disabling known-unsupported devices) before DM init happens. This
> +        * event is only available if OF_LIVE is enabled and is only used after relocation.
> +        */
> +       EVT_OF_LIVE_INIT,
> +
>         /**
>          * @EVT_COUNT:
>          * This constants holds the maximum event number + 1 and is used when
>          * looping over all event classes.
> diff --git a/lib/of_live.c b/lib/of_live.c
> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a 100644
> --- a/lib/of_live.c
> +++ b/lib/of_live.c
> @@ -10,8 +10,9 @@
>
>  #define LOG_CATEGORY   LOGC_DT
>
>  #include <abuf.h>
> +#include <event.h>
>  #include <log.h>
>  #include <linux/libfdt.h>
>  #include <of_live.h>
>  #include <malloc.h>
> @@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
>                 return ret;
>         }
>         debug("%s: stop\n", __func__);
>
> +       event_notify_null(EVT_OF_LIVE_INIT);
> +

This should go in initr_of_live() since the function you are dealing
with here is supposed to just do the live build.

Same for the EFI_STUB thing which I just noticed, actually

Also please check for error

Otherwise this seems OK to me. I do wonder why we can't use
EVT_FT_FIXUP though. Could you add mention of that to your comment in
event.h?

>         return ret;
>  }
>
>  void of_live_free(struct device_node *root)
>
> --
> 2.49.0
>

Regards,
Simon

Hi Simon,

On 4/10/25 13:27, Simon Glass wrote:
> Hi Caleb,
> 
> On Wed, 9 Apr 2025 at 11:17, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>
>> OF_LIVE offers a variety of benefits, one of them being that the live
>> tree can be modified without caring about the underlying FDT. This is
>> particularly valuable for working around U-Boot limitations like lacking
>> USB superspeed support on Qualcomm platforms, no runtime OTG, or
>> peripherals like the sdcard being broken (and displaying potentially
>> worrying error messages).
>>
>> Add an event to signal when the live tree has been built so that we can
>> apply fixups to it directly before devices are bound.
>>
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>>   common/event.c  | 3 +++
>>   include/event.h | 9 +++++++++
>>   lib/of_live.c   | 3 +++
>>   3 files changed, 15 insertions(+)
>>
>> diff --git a/common/event.c b/common/event.c
>> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
>> --- a/common/event.c
>> +++ b/common/event.c
>> @@ -47,8 +47,11 @@ const char *const type_name[] = {
>>          "ft_fixup",
>>
>>          /* main loop events */
>>          "main_loop",
>> +
>> +       /* livetree has been built */
>> +       "of_live_init",
>>   };
>>
>>   _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
>>   #endif
>> diff --git a/include/event.h b/include/event.h
>> index 75141a192a48b0931667632f41be8ff4d6139f7c..3fc673ba635ed45467aae8587705d37bef1c2a3f 100644
>> --- a/include/event.h
>> +++ b/include/event.h
>> @@ -152,8 +152,17 @@ enum event_t {
>>           * A non-zero return value causes the boot to fail.
>>           */
>>          EVT_MAIN_LOOP,
>>
>> +       /**
>> +        * @EVT_OF_LIVE_INIT:
>> +        * This event is triggered immediately after the live device tree has been
>> +        * built. This allows for machine specific fixups to be done to the live tree
>> +        * (like disabling known-unsupported devices) before DM init happens. This
>> +        * event is only available if OF_LIVE is enabled and is only used after relocation.
>> +        */
>> +       EVT_OF_LIVE_INIT,
>> +
>>          /**
>>           * @EVT_COUNT:
>>           * This constants holds the maximum event number + 1 and is used when
>>           * looping over all event classes.
>> diff --git a/lib/of_live.c b/lib/of_live.c
>> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a 100644
>> --- a/lib/of_live.c
>> +++ b/lib/of_live.c
>> @@ -10,8 +10,9 @@
>>
>>   #define LOG_CATEGORY   LOGC_DT
>>
>>   #include <abuf.h>
>> +#include <event.h>
>>   #include <log.h>
>>   #include <linux/libfdt.h>
>>   #include <of_live.h>
>>   #include <malloc.h>
>> @@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
>>                  return ret;
>>          }
>>          debug("%s: stop\n", __func__);
>>
>> +       event_notify_null(EVT_OF_LIVE_INIT);
>> +
> 
> This should go in initr_of_live() since the function you are dealing
> with here is supposed to just do the live build.

Well, we only every call this function from one place right now, but if 
it was called multiple times for some reason then I would want to be 
able to re-apply fixups to the new live tree.... I guess it should 
probably pass in *rootp to the event handler, let me rework that.>
> Same for the EFI_STUB thing which I just noticed, actually

what EFI_STUB thing?>
> Also please check for error
> 
> Otherwise this seems OK to me. I do wonder why we can't use
> EVT_FT_FIXUP though. Could you add mention of that to your comment in
> event.h?

Because FT_FIXUP is for fixing up the flat tree before starting the OS? 
these are obviously different things imo, im not sure how i could 
clarify this.>
>>          return ret;
>>   }
>>
>>   void of_live_free(struct device_node *root)
>>
>> --
>> 2.49.0
>>
> 
> Regards,
> Simon

Hi Caleb,

On Thu, 10 Apr 2025 at 07:00, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
> Hi Simon,
>
> On 4/10/25 13:27, Simon Glass wrote:
> > Hi Caleb,
> >
> > On Wed, 9 Apr 2025 at 11:17, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>
> >> OF_LIVE offers a variety of benefits, one of them being that the live
> >> tree can be modified without caring about the underlying FDT. This is
> >> particularly valuable for working around U-Boot limitations like lacking
> >> USB superspeed support on Qualcomm platforms, no runtime OTG, or
> >> peripherals like the sdcard being broken (and displaying potentially
> >> worrying error messages).
> >>
> >> Add an event to signal when the live tree has been built so that we can
> >> apply fixups to it directly before devices are bound.
> >>
> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> >> ---
> >>   common/event.c  | 3 +++
> >>   include/event.h | 9 +++++++++
> >>   lib/of_live.c   | 3 +++
> >>   3 files changed, 15 insertions(+)
> >>
> >> diff --git a/common/event.c b/common/event.c
> >> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
> >> --- a/common/event.c
> >> +++ b/common/event.c
> >> @@ -47,8 +47,11 @@ const char *const type_name[] = {
> >>          "ft_fixup",
> >>
> >>          /* main loop events */
> >>          "main_loop",
> >> +
> >> +       /* livetree has been built */
> >> +       "of_live_init",
> >>   };
> >>
> >>   _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
> >>   #endif
> >> diff --git a/include/event.h b/include/event.h
> >> index 75141a192a48b0931667632f41be8ff4d6139f7c..3fc673ba635ed45467aae8587705d37bef1c2a3f 100644
> >> --- a/include/event.h
> >> +++ b/include/event.h
> >> @@ -152,8 +152,17 @@ enum event_t {
> >>           * A non-zero return value causes the boot to fail.
> >>           */
> >>          EVT_MAIN_LOOP,
> >>
> >> +       /**
> >> +        * @EVT_OF_LIVE_INIT:
> >> +        * This event is triggered immediately after the live device tree has been
> >> +        * built. This allows for machine specific fixups to be done to the live tree
> >> +        * (like disabling known-unsupported devices) before DM init happens. This
> >> +        * event is only available if OF_LIVE is enabled and is only used after relocation.
> >> +        */
> >> +       EVT_OF_LIVE_INIT,
> >> +
> >>          /**
> >>           * @EVT_COUNT:
> >>           * This constants holds the maximum event number + 1 and is used when
> >>           * looping over all event classes.
> >> diff --git a/lib/of_live.c b/lib/of_live.c
> >> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a 100644
> >> --- a/lib/of_live.c
> >> +++ b/lib/of_live.c
> >> @@ -10,8 +10,9 @@
> >>
> >>   #define LOG_CATEGORY   LOGC_DT
> >>
> >>   #include <abuf.h>
> >> +#include <event.h>
> >>   #include <log.h>
> >>   #include <linux/libfdt.h>
> >>   #include <of_live.h>
> >>   #include <malloc.h>
> >> @@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
> >>                  return ret;
> >>          }
> >>          debug("%s: stop\n", __func__);
> >>
> >> +       event_notify_null(EVT_OF_LIVE_INIT);
> >> +
> >
> > This should go in initr_of_live() since the function you are dealing
> > with here is supposed to just do the live build.
>
> Well, we only every call this function from one place right now, but if
> it was called multiple times for some reason then I would want to be
> able to re-apply fixups to the new live tree.... I guess it should
> probably pass in *rootp to the event handler, let me rework that.>

There's no need to change the root, so what you have is find here.

> > Same for the EFI_STUB thing which I just noticed, actually
>
> what EFI_STUB thing?>

Oh, it's your EFI stub patch which isn't in Tom's tree yet. Just for
when you get to it, then.

> > Also please check for error
> >
> > Otherwise this seems OK to me. I do wonder why we can't use
> > EVT_FT_FIXUP though. Could you add mention of that to your comment in
> > event.h?
>
> Because FT_FIXUP is for fixing up the flat tree before starting the OS?
> these are obviously different things imo, im not sure how i could
> clarify this.>

Well, what is the purpose of your code? Are you saying that it is used
within U-Boot, but not passed to the OS?

> >>          return ret;
> >>   }
> >>
> >>   void of_live_free(struct device_node *root)
> >>
> >> --
> >> 2.49.0
> >>

Regards,
Simon

On 4/10/25 15:07, Simon Glass wrote:
> Hi Caleb,
> 
> On Thu, 10 Apr 2025 at 07:00, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>
>> Hi Simon,
>>
>> On 4/10/25 13:27, Simon Glass wrote:
>>> Hi Caleb,
>>>
>>> On Wed, 9 Apr 2025 at 11:17, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>>>
>>>> OF_LIVE offers a variety of benefits, one of them being that the live
>>>> tree can be modified without caring about the underlying FDT. This is
>>>> particularly valuable for working around U-Boot limitations like lacking
>>>> USB superspeed support on Qualcomm platforms, no runtime OTG, or
>>>> peripherals like the sdcard being broken (and displaying potentially
>>>> worrying error messages).
>>>>
>>>> Add an event to signal when the live tree has been built so that we can
>>>> apply fixups to it directly before devices are bound.
>>>>
>>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>>>> ---
>>>>    common/event.c  | 3 +++
>>>>    include/event.h | 9 +++++++++
>>>>    lib/of_live.c   | 3 +++
>>>>    3 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/common/event.c b/common/event.c
>>>> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
>>>> --- a/common/event.c
>>>> +++ b/common/event.c
>>>> @@ -47,8 +47,11 @@ const char *const type_name[] = {
>>>>           "ft_fixup",
>>>>
>>>>           /* main loop events */
>>>>           "main_loop",
>>>> +
>>>> +       /* livetree has been built */
>>>> +       "of_live_init",
>>>>    };
>>>>
>>>>    _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
>>>>    #endif
>>>> diff --git a/include/event.h b/include/event.h
>>>> index 75141a192a48b0931667632f41be8ff4d6139f7c..3fc673ba635ed45467aae8587705d37bef1c2a3f 100644
>>>> --- a/include/event.h
>>>> +++ b/include/event.h
>>>> @@ -152,8 +152,17 @@ enum event_t {
>>>>            * A non-zero return value causes the boot to fail.
>>>>            */
>>>>           EVT_MAIN_LOOP,
>>>>
>>>> +       /**
>>>> +        * @EVT_OF_LIVE_INIT:
>>>> +        * This event is triggered immediately after the live device tree has been
>>>> +        * built. This allows for machine specific fixups to be done to the live tree
>>>> +        * (like disabling known-unsupported devices) before DM init happens. This
>>>> +        * event is only available if OF_LIVE is enabled and is only used after relocation.
>>>> +        */
>>>> +       EVT_OF_LIVE_INIT,
>>>> +
>>>>           /**
>>>>            * @EVT_COUNT:
>>>>            * This constants holds the maximum event number + 1 and is used when
>>>>            * looping over all event classes.
>>>> diff --git a/lib/of_live.c b/lib/of_live.c
>>>> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a 100644
>>>> --- a/lib/of_live.c
>>>> +++ b/lib/of_live.c
>>>> @@ -10,8 +10,9 @@
>>>>
>>>>    #define LOG_CATEGORY   LOGC_DT
>>>>
>>>>    #include <abuf.h>
>>>> +#include <event.h>
>>>>    #include <log.h>
>>>>    #include <linux/libfdt.h>
>>>>    #include <of_live.h>
>>>>    #include <malloc.h>
>>>> @@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
>>>>                   return ret;
>>>>           }
>>>>           debug("%s: stop\n", __func__);
>>>>
>>>> +       event_notify_null(EVT_OF_LIVE_INIT);
>>>> +
>>>
>>> This should go in initr_of_live() since the function you are dealing
>>> with here is supposed to just do the live build.
>>
>> Well, we only every call this function from one place right now, but if
>> it was called multiple times for some reason then I would want to be
>> able to re-apply fixups to the new live tree.... I guess it should
>> probably pass in *rootp to the event handler, let me rework that.>
> 
> There's no need to change the root, so what you have is find here.
> 
>>> Same for the EFI_STUB thing which I just noticed, actually
>>
>> what EFI_STUB thing?>
> 
> Oh, it's your EFI stub patch which isn't in Tom's tree yet. Just for
> when you get to it, then.
> 
>>> Also please check for error
>>>
>>> Otherwise this seems OK to me. I do wonder why we can't use
>>> EVT_FT_FIXUP though. Could you add mention of that to your comment in
>>> event.h?
>>
>> Because FT_FIXUP is for fixing up the flat tree before starting the OS?
>> these are obviously different things imo, im not sure how i could
>> clarify this.>
> 
> Well, what is the purpose of your code? Are you saying that it is used
> within U-Boot, but not passed to the OS?

Yes, please read the cover letter and commit messages. FT_FIXUP allows 
for the FDT that is about to be passed to the OS to be fixed up, 
OF_LIVE_INIT signifies that U-Boot has finished building it's livetree. 
The livetree is obviously not used outside of U-Boot, being a totally 
custom in-memory representation of the DT.

> 
>>>>           return ret;
>>>>    }
>>>>
>>>>    void of_live_free(struct device_node *root)
>>>>
>>>> --
>>>> 2.49.0
>>>>
> 
> Regards,
> Simon

Hi Caleb,

On Thu, 10 Apr 2025 at 08:04, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
>
>
> On 4/10/25 15:07, Simon Glass wrote:
> > Hi Caleb,
> >
> > On Thu, 10 Apr 2025 at 07:00, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 4/10/25 13:27, Simon Glass wrote:
> >>> Hi Caleb,
> >>>
> >>> On Wed, 9 Apr 2025 at 11:17, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>>>
> >>>> OF_LIVE offers a variety of benefits, one of them being that the live
> >>>> tree can be modified without caring about the underlying FDT. This is
> >>>> particularly valuable for working around U-Boot limitations like lacking
> >>>> USB superspeed support on Qualcomm platforms, no runtime OTG, or
> >>>> peripherals like the sdcard being broken (and displaying potentially
> >>>> worrying error messages).
> >>>>
> >>>> Add an event to signal when the live tree has been built so that we can
> >>>> apply fixups to it directly before devices are bound.
> >>>>
> >>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> >>>> ---
> >>>>    common/event.c  | 3 +++
> >>>>    include/event.h | 9 +++++++++
> >>>>    lib/of_live.c   | 3 +++
> >>>>    3 files changed, 15 insertions(+)
> >>>>
> >>>> diff --git a/common/event.c b/common/event.c
> >>>> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
> >>>> --- a/common/event.c
> >>>> +++ b/common/event.c
> >>>> @@ -47,8 +47,11 @@ const char *const type_name[] = {
> >>>>           "ft_fixup",
> >>>>
> >>>>           /* main loop events */
> >>>>           "main_loop",
> >>>> +
> >>>> +       /* livetree has been built */
> >>>> +       "of_live_init",
> >>>>    };
> >>>>
> >>>>    _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
> >>>>    #endif
> >>>> diff --git a/include/event.h b/include/event.h
> >>>> index 75141a192a48b0931667632f41be8ff4d6139f7c..3fc673ba635ed45467aae8587705d37bef1c2a3f 100644
> >>>> --- a/include/event.h
> >>>> +++ b/include/event.h
> >>>> @@ -152,8 +152,17 @@ enum event_t {
> >>>>            * A non-zero return value causes the boot to fail.
> >>>>            */
> >>>>           EVT_MAIN_LOOP,
> >>>>
> >>>> +       /**
> >>>> +        * @EVT_OF_LIVE_INIT:
> >>>> +        * This event is triggered immediately after the live device tree has been
> >>>> +        * built. This allows for machine specific fixups to be done to the live tree
> >>>> +        * (like disabling known-unsupported devices) before DM init happens. This
> >>>> +        * event is only available if OF_LIVE is enabled and is only used after relocation.
> >>>> +        */
> >>>> +       EVT_OF_LIVE_INIT,
> >>>> +
> >>>>           /**
> >>>>            * @EVT_COUNT:
> >>>>            * This constants holds the maximum event number + 1 and is used when
> >>>>            * looping over all event classes.
> >>>> diff --git a/lib/of_live.c b/lib/of_live.c
> >>>> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a 100644
> >>>> --- a/lib/of_live.c
> >>>> +++ b/lib/of_live.c
> >>>> @@ -10,8 +10,9 @@
> >>>>
> >>>>    #define LOG_CATEGORY   LOGC_DT
> >>>>
> >>>>    #include <abuf.h>
> >>>> +#include <event.h>
> >>>>    #include <log.h>
> >>>>    #include <linux/libfdt.h>
> >>>>    #include <of_live.h>
> >>>>    #include <malloc.h>
> >>>> @@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
> >>>>                   return ret;
> >>>>           }
> >>>>           debug("%s: stop\n", __func__);
> >>>>
> >>>> +       event_notify_null(EVT_OF_LIVE_INIT);
> >>>> +
> >>>
> >>> This should go in initr_of_live() since the function you are dealing
> >>> with here is supposed to just do the live build.
> >>
> >> Well, we only every call this function from one place right now, but if
> >> it was called multiple times for some reason then I would want to be
> >> able to re-apply fixups to the new live tree.... I guess it should
> >> probably pass in *rootp to the event handler, let me rework that.>
> >
> > There's no need to change the root, so what you have is find here.
> >
> >>> Same for the EFI_STUB thing which I just noticed, actually
> >>
> >> what EFI_STUB thing?>
> >
> > Oh, it's your EFI stub patch which isn't in Tom's tree yet. Just for
> > when you get to it, then.
> >
> >>> Also please check for error
> >>>
> >>> Otherwise this seems OK to me. I do wonder why we can't use
> >>> EVT_FT_FIXUP though. Could you add mention of that to your comment in
> >>> event.h?
> >>
> >> Because FT_FIXUP is for fixing up the flat tree before starting the OS?
> >> these are obviously different things imo, im not sure how i could
> >> clarify this.>
> >
> > Well, what is the purpose of your code? Are you saying that it is used
> > within U-Boot, but not passed to the OS?
>
> Yes, please read the cover letter and commit messages. FT_FIXUP allows
> for the FDT that is about to be passed to the OS to be fixed up,
> OF_LIVE_INIT signifies that U-Boot has finished building it's livetree.
> The livetree is obviously not used outside of U-Boot, being a totally
> custom in-memory representation of the DT.

Oh, I wondered what '(which is not passed on to further boot stages)' meant.

Sorry, no, we can't do that. We are moving towards using livetree in
U-Boot - there is already the fixup thing as mentioned. We just don't
yet have the flattening at the end before boot. If you have time you
could look at that.

Your event to modify the livetree is fine, but any modifications
should be passed on to the OS. If you don't want that, then your fixup
would need to remove them, I suppose. Does the OS actually care,
though?

Regards,
Simon

Hi Simon,

On 4/10/25 16:15, Simon Glass wrote:
> Hi Caleb,
> 
> On Thu, 10 Apr 2025 at 08:04, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>
>>
>>
>> On 4/10/25 15:07, Simon Glass wrote:
>>> Hi Caleb,
>>>
>>> On Thu, 10 Apr 2025 at 07:00, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>>>
>>>> Hi Simon,
>>>>
>>>> On 4/10/25 13:27, Simon Glass wrote:
>>>>> Hi Caleb,
>>>>>
>>>>> On Wed, 9 Apr 2025 at 11:17, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>>>>>
>>>>>> OF_LIVE offers a variety of benefits, one of them being that the live
>>>>>> tree can be modified without caring about the underlying FDT. This is
>>>>>> particularly valuable for working around U-Boot limitations like lacking
>>>>>> USB superspeed support on Qualcomm platforms, no runtime OTG, or
>>>>>> peripherals like the sdcard being broken (and displaying potentially
>>>>>> worrying error messages).
>>>>>>
>>>>>> Add an event to signal when the live tree has been built so that we can
>>>>>> apply fixups to it directly before devices are bound.
>>>>>>
>>>>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>>>>>> ---
>>>>>>     common/event.c  | 3 +++
>>>>>>     include/event.h | 9 +++++++++
>>>>>>     lib/of_live.c   | 3 +++
>>>>>>     3 files changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/common/event.c b/common/event.c
>>>>>> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
>>>>>> --- a/common/event.c
>>>>>> +++ b/common/event.c
>>>>>> @@ -47,8 +47,11 @@ const char *const type_name[] = {
>>>>>>            "ft_fixup",
>>>>>>
>>>>>>            /* main loop events */
>>>>>>            "main_loop",
>>>>>> +
>>>>>> +       /* livetree has been built */
>>>>>> +       "of_live_init",
>>>>>>     };
>>>>>>
>>>>>>     _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
>>>>>>     #endif
>>>>>> diff --git a/include/event.h b/include/event.h
>>>>>> index 75141a192a48b0931667632f41be8ff4d6139f7c..3fc673ba635ed45467aae8587705d37bef1c2a3f 100644
>>>>>> --- a/include/event.h
>>>>>> +++ b/include/event.h
>>>>>> @@ -152,8 +152,17 @@ enum event_t {
>>>>>>             * A non-zero return value causes the boot to fail.
>>>>>>             */
>>>>>>            EVT_MAIN_LOOP,
>>>>>>
>>>>>> +       /**
>>>>>> +        * @EVT_OF_LIVE_INIT:
>>>>>> +        * This event is triggered immediately after the live device tree has been
>>>>>> +        * built. This allows for machine specific fixups to be done to the live tree
>>>>>> +        * (like disabling known-unsupported devices) before DM init happens. This
>>>>>> +        * event is only available if OF_LIVE is enabled and is only used after relocation.
>>>>>> +        */
>>>>>> +       EVT_OF_LIVE_INIT,
>>>>>> +
>>>>>>            /**
>>>>>>             * @EVT_COUNT:
>>>>>>             * This constants holds the maximum event number + 1 and is used when
>>>>>>             * looping over all event classes.
>>>>>> diff --git a/lib/of_live.c b/lib/of_live.c
>>>>>> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a 100644
>>>>>> --- a/lib/of_live.c
>>>>>> +++ b/lib/of_live.c
>>>>>> @@ -10,8 +10,9 @@
>>>>>>
>>>>>>     #define LOG_CATEGORY   LOGC_DT
>>>>>>
>>>>>>     #include <abuf.h>
>>>>>> +#include <event.h>
>>>>>>     #include <log.h>
>>>>>>     #include <linux/libfdt.h>
>>>>>>     #include <of_live.h>
>>>>>>     #include <malloc.h>
>>>>>> @@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
>>>>>>                    return ret;
>>>>>>            }
>>>>>>            debug("%s: stop\n", __func__);
>>>>>>
>>>>>> +       event_notify_null(EVT_OF_LIVE_INIT);
>>>>>> +
>>>>>
>>>>> This should go in initr_of_live() since the function you are dealing
>>>>> with here is supposed to just do the live build.
>>>>
>>>> Well, we only every call this function from one place right now, but if
>>>> it was called multiple times for some reason then I would want to be
>>>> able to re-apply fixups to the new live tree.... I guess it should
>>>> probably pass in *rootp to the event handler, let me rework that.>
>>>
>>> There's no need to change the root, so what you have is find here.
>>>
>>>>> Same for the EFI_STUB thing which I just noticed, actually
>>>>
>>>> what EFI_STUB thing?>
>>>
>>> Oh, it's your EFI stub patch which isn't in Tom's tree yet. Just for
>>> when you get to it, then.
>>>
>>>>> Also please check for error
>>>>>
>>>>> Otherwise this seems OK to me. I do wonder why we can't use
>>>>> EVT_FT_FIXUP though. Could you add mention of that to your comment in
>>>>> event.h?
>>>>
>>>> Because FT_FIXUP is for fixing up the flat tree before starting the OS?
>>>> these are obviously different things imo, im not sure how i could
>>>> clarify this.>
>>>
>>> Well, what is the purpose of your code? Are you saying that it is used
>>> within U-Boot, but not passed to the OS?
>>
>> Yes, please read the cover letter and commit messages. FT_FIXUP allows
>> for the FDT that is about to be passed to the OS to be fixed up,
>> OF_LIVE_INIT signifies that U-Boot has finished building it's livetree.
>> The livetree is obviously not used outside of U-Boot, being a totally
>> custom in-memory representation of the DT.
> 
> Oh, I wondered what '(which is not passed on to further boot stages)' meant.

If you don't understand what my patch series does, it would save us both 
  a lot of time if you asked me to clarify some specific point than to 
reply with your stream-of-consciousness ponderings about what perhaps I 
might be doing.

I've tried -- again -- to explain what we're doing here, comments below.

> 
> Sorry, no, we can't do that. 
We have been for months already

> We are moving towards using livetree in U-Boot

indeed, mach-snapdragon has been using it for 372 days now

commit 1534186f2953d99dcbc757a19b43e4fac644e8a9
Author: Caleb Connolly <caleb.connolly@linaro.org>
Date:   Wed Apr 3 14:07:49 2024 +0200

     qcom_defconfig: enable livetree

> - there is already the fixup thing as mentioned.

The FT_FIXUP event which has absolutely nothing to do with livetree or 
this patch series and which is triggered at a different point in the 
boot process for an entirely different purpose (preparing for the OS 
rather than preparing for U-Boot itself)?

> We just don't yet have the flattening at the end before boot.

Which is exactly what we leverage in mach-snapdragon to bridge the gap 
between U-Boot (a bootloader that intentionally has a simplified model 
of the hardware) and upstream DT which contains a lot of additional 
context which is (as far as U-Boot is concerned) noise that we have to 
filter out.

 > If you have time you could look at that.

this is something literally nobody wants that wouldn't even be used in 
most cases (since EFI DT FIXUP protocol passes you an FDT anyway).

> 
> Your event to modify the livetree is fine, but any modifications
> should be passed on to the OS.

As stated clearly in the commit message and the cover letter, making 
modifications that don't get passed on to the OS is the entire point.

Assuming you didn't take the time to look at mach-snapdragon/of_fixup.c, 
it is literally rewriting the livetree to patch out the USB superspeed 
phy and only initialise USB in high-speed mode. This is quite simply 
because U-Boot doesn't have PHY drivers for superspeed USB on Qualcomm 
platforms yet.

We absolutely don't want this to be passed on to the OS because it is 
entirely an implementation detail in U-Boot. It's functionally All this 
additional complexiequivalent to adding a bunch of "#ifdef 
ARCH_SNAPDRAGON" ... "skip the superspeed phy" in the dwc3 driver.

This is really basic abstraction, and hopefully you'll tell me that I 
didn't need to explain all this because you did in fact take the time to 
understand how mach-snapdragon is using the livetree before replying 
(like by reading the big comment at the top of of_fixup.c).

> If you don't want that, then your fixup would need to remove them, I suppose.

why why why???

 > Does the OS actually care, though?

The OS would almost certainly be unhappy at the tree U-Boot is using 
internally yes, because we don't bother fixing up stuff like the 
usb-connector nodes which are unused in U-Boot.

I know, I seem very frustrated, and I am. I have no problem with you 
misunderstanding a patch or asking for clarifications, but you keep 
repeatedly misunderstanding fundamental technical realities of an issue 
or patch series and then seemingly doubling down and just saying 
whatever comes to mind without exerting any effort to understand the 
other side.

----

You didn't ask, but I'm going to explain how I review patches to 
hopefully convey some of my confusion here because I'm worried this is a 
pattern that is repeating itself and it is starting to wear on me. I've 
never been one to beat around the bush I guess.

My first thought when I don't understand some change a patch is making 
is "huh I wonder why they did that instead of this", but you will never 
see me say that verbatim on the lists because I immediately go and read 
the code to remind myself what it is doing and validate any assumptions 
I might be making, as well as to try to understand the thought process 
of the author.

What you WILL see me say on the lists is either

1) A (usually backed up by references) explanation of why I think the 
change is wrong, or

2) If I'm really confused I'll usually try to explain what I think 
they're trying to do (again backed up by code references where relevant) 
and ask for clarification.

This way in (1) the author can say "oh yeah my bad, thanks for noticing 
that" or point out what I missed that makes their change make sense. 
This is easy, painless, everyone can go about their day (unless there is 
a technical disagreement, but really that's besides the point here)

In the second case, they can confirm that my understanding is correct 
and respond to my (typically pointed) questions to clarify both how they 
understand the code and whatever context I'm missing. Assuming I'm any 
good at asking the right questions, this results in the same outcomes as 
(1).

This works because I'm trying to bridge the gap between my understanding 
and the patch authors, I have my own mental model of how things work but 
it could be wrong, so I want to convey what my model is so that it can 
be corrected in that case, and so that if the authors understanding is 
wrong then they can correct it.

Your reviews don't do this, they leave me guessing wildly at what you 
might possibly think my code is doing.

There is no polite way for me to say "this comment you made is so out of 
left field that I think you fundamentally misunderstand this patch", but 
if you tell me what you think I'm trying to do (if you're at all 
unsure), so I know what foundation you're working on, then I can easily 
and politely say "ah your understanding here is wrong", you can re-do 
your review with a correct mental model and I don't have to try and 
parse things the way I have done above.

If I were to go as far as to define a rule for patch review, it would be 
"whenever the words 'i wonder' go through your head, try to find out 
before sending your reply, and include your findings if you don't find a 
satisfactory answer"

Kind regards,

> 
> Regards,
> Simon

Hi Caleb,

On Thu, 10 Apr 2025 at 09:41, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
> Hi Simon,
>
> On 4/10/25 16:15, Simon Glass wrote:
> > Hi Caleb,
> >
> > On Thu, 10 Apr 2025 at 08:04, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>
> >>
> >>
> >> On 4/10/25 15:07, Simon Glass wrote:
> >>> Hi Caleb,
> >>>
> >>> On Thu, 10 Apr 2025 at 07:00, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On 4/10/25 13:27, Simon Glass wrote:
> >>>>> Hi Caleb,
> >>>>>
> >>>>> On Wed, 9 Apr 2025 at 11:17, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>>>>>
> >>>>>> OF_LIVE offers a variety of benefits, one of them being that the live
> >>>>>> tree can be modified without caring about the underlying FDT. This is
> >>>>>> particularly valuable for working around U-Boot limitations like lacking
> >>>>>> USB superspeed support on Qualcomm platforms, no runtime OTG, or
> >>>>>> peripherals like the sdcard being broken (and displaying potentially
> >>>>>> worrying error messages).
> >>>>>>
> >>>>>> Add an event to signal when the live tree has been built so that we can
> >>>>>> apply fixups to it directly before devices are bound.
> >>>>>>
> >>>>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> >>>>>> ---
> >>>>>>     common/event.c  | 3 +++
> >>>>>>     include/event.h | 9 +++++++++
> >>>>>>     lib/of_live.c   | 3 +++
> >>>>>>     3 files changed, 15 insertions(+)
> >>>>>>
> >>>>>> diff --git a/common/event.c b/common/event.c
> >>>>>> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
> >>>>>> --- a/common/event.c
> >>>>>> +++ b/common/event.c
> >>>>>> @@ -47,8 +47,11 @@ const char *const type_name[] = {
> >>>>>>            "ft_fixup",
> >>>>>>
> >>>>>>            /* main loop events */
> >>>>>>            "main_loop",
> >>>>>> +
> >>>>>> +       /* livetree has been built */
> >>>>>> +       "of_live_init",
> >>>>>>     };
> >>>>>>
> >>>>>>     _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
> >>>>>>     #endif
> >>>>>> diff --git a/include/event.h b/include/event.h
> >>>>>> index 75141a192a48b0931667632f41be8ff4d6139f7c..3fc673ba635ed45467aae8587705d37bef1c2a3f 100644
> >>>>>> --- a/include/event.h
> >>>>>> +++ b/include/event.h
> >>>>>> @@ -152,8 +152,17 @@ enum event_t {
> >>>>>>             * A non-zero return value causes the boot to fail.
> >>>>>>             */
> >>>>>>            EVT_MAIN_LOOP,
> >>>>>>
> >>>>>> +       /**
> >>>>>> +        * @EVT_OF_LIVE_INIT:
> >>>>>> +        * This event is triggered immediately after the live device tree has been
> >>>>>> +        * built. This allows for machine specific fixups to be done to the live tree
> >>>>>> +        * (like disabling known-unsupported devices) before DM init happens. This
> >>>>>> +        * event is only available if OF_LIVE is enabled and is only used after relocation.
> >>>>>> +        */
> >>>>>> +       EVT_OF_LIVE_INIT,
> >>>>>> +
> >>>>>>            /**
> >>>>>>             * @EVT_COUNT:
> >>>>>>             * This constants holds the maximum event number + 1 and is used when
> >>>>>>             * looping over all event classes.
> >>>>>> diff --git a/lib/of_live.c b/lib/of_live.c
> >>>>>> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a 100644
> >>>>>> --- a/lib/of_live.c
> >>>>>> +++ b/lib/of_live.c
> >>>>>> @@ -10,8 +10,9 @@
> >>>>>>
> >>>>>>     #define LOG_CATEGORY   LOGC_DT
> >>>>>>
> >>>>>>     #include <abuf.h>
> >>>>>> +#include <event.h>
> >>>>>>     #include <log.h>
> >>>>>>     #include <linux/libfdt.h>
> >>>>>>     #include <of_live.h>
> >>>>>>     #include <malloc.h>
> >>>>>> @@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
> >>>>>>                    return ret;
> >>>>>>            }
> >>>>>>            debug("%s: stop\n", __func__);
> >>>>>>
> >>>>>> +       event_notify_null(EVT_OF_LIVE_INIT);
> >>>>>> +
> >>>>>
> >>>>> This should go in initr_of_live() since the function you are dealing
> >>>>> with here is supposed to just do the live build.
> >>>>
> >>>> Well, we only every call this function from one place right now, but if
> >>>> it was called multiple times for some reason then I would want to be
> >>>> able to re-apply fixups to the new live tree.... I guess it should
> >>>> probably pass in *rootp to the event handler, let me rework that.>
> >>>
> >>> There's no need to change the root, so what you have is find here.
> >>>
> >>>>> Same for the EFI_STUB thing which I just noticed, actually
> >>>>
> >>>> what EFI_STUB thing?>
> >>>
> >>> Oh, it's your EFI stub patch which isn't in Tom's tree yet. Just for
> >>> when you get to it, then.
> >>>
> >>>>> Also please check for error
> >>>>>
> >>>>> Otherwise this seems OK to me. I do wonder why we can't use
> >>>>> EVT_FT_FIXUP though. Could you add mention of that to your comment in
> >>>>> event.h?
> >>>>
> >>>> Because FT_FIXUP is for fixing up the flat tree before starting the OS?
> >>>> these are obviously different things imo, im not sure how i could
> >>>> clarify this.>
> >>>
> >>> Well, what is the purpose of your code? Are you saying that it is used
> >>> within U-Boot, but not passed to the OS?
> >>
> >> Yes, please read the cover letter and commit messages. FT_FIXUP allows
> >> for the FDT that is about to be passed to the OS to be fixed up,
> >> OF_LIVE_INIT signifies that U-Boot has finished building it's livetree.
> >> The livetree is obviously not used outside of U-Boot, being a totally
> >> custom in-memory representation of the DT.
> >
> > Oh, I wondered what '(which is not passed on to further boot stages)' meant.
>
> If you don't understand what my patch series does, it would save us both
>   a lot of time if you asked me to clarify some specific point than to
> reply with your stream-of-consciousness ponderings about what perhaps I
> might be doing.
>
> I've tried -- again -- to explain what we're doing here, comments below.
>
> >
> > Sorry, no, we can't do that.
> We have been for months already
>
> > We are moving towards using livetree in U-Boot
>
> indeed, mach-snapdragon has been using it for 372 days now
>
> commit 1534186f2953d99dcbc757a19b43e4fac644e8a9
> Author: Caleb Connolly <caleb.connolly@linaro.org>
> Date:   Wed Apr 3 14:07:49 2024 +0200
>
>      qcom_defconfig: enable livetree
>
> > - there is already the fixup thing as mentioned.
>
> The FT_FIXUP event which has absolutely nothing to do with livetree or
> this patch series and which is triggered at a different point in the
> boot process for an entirely different purpose (preparing for the OS
> rather than preparing for U-Boot itself)?
>
> > We just don't yet have the flattening at the end before boot.
>
> Which is exactly what we leverage in mach-snapdragon to bridge the gap
> between U-Boot (a bootloader that intentionally has a simplified model
> of the hardware) and upstream DT which contains a lot of additional
> context which is (as far as U-Boot is concerned) noise that we have to
> filter out.
>
>  > If you have time you could look at that.
>
> this is something literally nobody wants that wouldn't even be used in
> most cases (since EFI DT FIXUP protocol passes you an FDT anyway).
>
> >
> > Your event to modify the livetree is fine, but any modifications
> > should be passed on to the OS.
>
> As stated clearly in the commit message and the cover letter, making
> modifications that don't get passed on to the OS is the entire point.
>
> Assuming you didn't take the time to look at mach-snapdragon/of_fixup.c,
> it is literally rewriting the livetree to patch out the USB superspeed
> phy and only initialise USB in high-speed mode. This is quite simply
> because U-Boot doesn't have PHY drivers for superspeed USB on Qualcomm
> platforms yet.
>
> We absolutely don't want this to be passed on to the OS because it is
> entirely an implementation detail in U-Boot. It's functionally All this
> additional complexiequivalent to adding a bunch of "#ifdef
> ARCH_SNAPDRAGON" ... "skip the superspeed phy" in the dwc3 driver.
>
> This is really basic abstraction, and hopefully you'll tell me that I
> didn't need to explain all this because you did in fact take the time to
> understand how mach-snapdragon is using the livetree before replying
> (like by reading the big comment at the top of of_fixup.c).
>
> > If you don't want that, then your fixup would need to remove them, I suppose.
>
> why why why???
>
>  > Does the OS actually care, though?
>
> The OS would almost certainly be unhappy at the tree U-Boot is using
> internally yes, because we don't bother fixing up stuff like the
> usb-connector nodes which are unused in U-Boot.
>
> I know, I seem very frustrated, and I am. I have no problem with you
> misunderstanding a patch or asking for clarifications, but you keep
> repeatedly misunderstanding fundamental technical realities of an issue
> or patch series and then seemingly doubling down and just saying
> whatever comes to mind without exerting any effort to understand the
> other side.
>
> ----
>
> You didn't ask, but I'm going to explain how I review patches to
> hopefully convey some of my confusion here because I'm worried this is a
> pattern that is repeating itself and it is starting to wear on me. I've
> never been one to beat around the bush I guess.
>
> My first thought when I don't understand some change a patch is making
> is "huh I wonder why they did that instead of this", but you will never
> see me say that verbatim on the lists because I immediately go and read
> the code to remind myself what it is doing and validate any assumptions
> I might be making, as well as to try to understand the thought process
> of the author.
>
> What you WILL see me say on the lists is either
>
> 1) A (usually backed up by references) explanation of why I think the
> change is wrong, or
>
> 2) If I'm really confused I'll usually try to explain what I think
> they're trying to do (again backed up by code references where relevant)
> and ask for clarification.
>
> This way in (1) the author can say "oh yeah my bad, thanks for noticing
> that" or point out what I missed that makes their change make sense.
> This is easy, painless, everyone can go about their day (unless there is
> a technical disagreement, but really that's besides the point here)
>
> In the second case, they can confirm that my understanding is correct
> and respond to my (typically pointed) questions to clarify both how they
> understand the code and whatever context I'm missing. Assuming I'm any
> good at asking the right questions, this results in the same outcomes as
> (1).
>
> This works because I'm trying to bridge the gap between my understanding
> and the patch authors, I have my own mental model of how things work but
> it could be wrong, so I want to convey what my model is so that it can
> be corrected in that case, and so that if the authors understanding is
> wrong then they can correct it.
>
> Your reviews don't do this, they leave me guessing wildly at what you
> might possibly think my code is doing.
>
> There is no polite way for me to say "this comment you made is so out of
> left field that I think you fundamentally misunderstand this patch", but
> if you tell me what you think I'm trying to do (if you're at all
> unsure), so I know what foundation you're working on, then I can easily
> and politely say "ah your understanding here is wrong", you can re-do
> your review with a correct mental model and I don't have to try and
> parse things the way I have done above.
>
> If I were to go as far as to define a rule for patch review, it would be
> "whenever the words 'i wonder' go through your head, try to find out
> before sending your reply, and include your findings if you don't find a
> satisfactory answer"

I'm sorry for upsetting you and I really hope I can improve the
situation here. I also really appreciate you taking the time to write
all these details.

It is true that I struggle to find time for reviews at the moment, all
in my spare time. Even then, I just checked and I am number two in the
review tags for the most recent release. I am definitely pulling my
weight here. Also I have knowledge of many of the subsystems. Yes I
wish I could dig in and understand things more and I will try to do
better. But my intentions are good.

Here is how U-Boot should (eventually) work:

Flat DT
    |
unflattened
    |
used in U-Boot               some-other-DT-packaged-with-OS
    |                                       /
    |                                   /
this is where the fixups happen
    |
flattened
    |
    |
passed to Linux

The branch is due to the fact that the OS may have a better
devicetree, but ideally, in the fullness of time, the devicetree
provided by firmware would be good enough.

I don't believe it is a good idea for U-Boot to use its own
devicetree, separate from the Linux one. U-Boot should use the same
one. That is the idea behind the dts-upstream feature in U-Boot, to
make that easier.

For the EVT_FT_FIXUP event, the intent is to do fixups on the livetree
as it should be more efficient. There's a lot of existing code that
uses the flattree and I haven't figured out a clever way to migrate
it.

But overall, the way to think of the livetree and flattree is that
they are the same tree. We can unflatten, modify and flatten, but not
unflatten, modify and throw away. We certainly don't maintain two
separate trees within U-Boot.

I suspect the main problem is that this part of U-Boot is in slow
transition. Perhaps some better documentation of the eventual state
would help?

If you are still unhappy, how about we organise a call to discuss
this? I really don't think we are on the same page at all right now.
It might help to reduce frustration.

Regards,
Simon

On 4/10/25 23:25, Simon Glass wrote:
> Hi Caleb,
> 
> On Thu, 10 Apr 2025 at 09:41, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>
>> Hi Simon,
>>
>> On 4/10/25 16:15, Simon Glass wrote:
>>> Hi Caleb,
>>>
>>> On Thu, 10 Apr 2025 at 08:04, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 4/10/25 15:07, Simon Glass wrote:
>>>>> Hi Caleb,
>>>>>
>>>>> On Thu, 10 Apr 2025 at 07:00, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>>>>>
>>>>>> Hi Simon,
>>>>>>
>>>>>> On 4/10/25 13:27, Simon Glass wrote:
>>>>>>> Hi Caleb,
>>>>>>>
>>>>>>> On Wed, 9 Apr 2025 at 11:17, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>>>>>>>>
>>>>>>>> OF_LIVE offers a variety of benefits, one of them being that the live
>>>>>>>> tree can be modified without caring about the underlying FDT. This is
>>>>>>>> particularly valuable for working around U-Boot limitations like lacking
>>>>>>>> USB superspeed support on Qualcomm platforms, no runtime OTG, or
>>>>>>>> peripherals like the sdcard being broken (and displaying potentially
>>>>>>>> worrying error messages).
>>>>>>>>
>>>>>>>> Add an event to signal when the live tree has been built so that we can
>>>>>>>> apply fixups to it directly before devices are bound.
>>>>>>>>
>>>>>>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>>>>>>>> ---
>>>>>>>>      common/event.c  | 3 +++
>>>>>>>>      include/event.h | 9 +++++++++
>>>>>>>>      lib/of_live.c   | 3 +++
>>>>>>>>      3 files changed, 15 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/common/event.c b/common/event.c
>>>>>>>> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
>>>>>>>> --- a/common/event.c
>>>>>>>> +++ b/common/event.c
>>>>>>>> @@ -47,8 +47,11 @@ const char *const type_name[] = {
>>>>>>>>             "ft_fixup",
>>>>>>>>
>>>>>>>>             /* main loop events */
>>>>>>>>             "main_loop",
>>>>>>>> +
>>>>>>>> +       /* livetree has been built */
>>>>>>>> +       "of_live_init",
>>>>>>>>      };
>>>>>>>>
>>>>>>>>      _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
>>>>>>>>      #endif
>>>>>>>> diff --git a/include/event.h b/include/event.h
>>>>>>>> index 75141a192a48b0931667632f41be8ff4d6139f7c..3fc673ba635ed45467aae8587705d37bef1c2a3f 100644
>>>>>>>> --- a/include/event.h
>>>>>>>> +++ b/include/event.h
>>>>>>>> @@ -152,8 +152,17 @@ enum event_t {
>>>>>>>>              * A non-zero return value causes the boot to fail.
>>>>>>>>              */
>>>>>>>>             EVT_MAIN_LOOP,
>>>>>>>>
>>>>>>>> +       /**
>>>>>>>> +        * @EVT_OF_LIVE_INIT:
>>>>>>>> +        * This event is triggered immediately after the live device tree has been
>>>>>>>> +        * built. This allows for machine specific fixups to be done to the live tree
>>>>>>>> +        * (like disabling known-unsupported devices) before DM init happens. This
>>>>>>>> +        * event is only available if OF_LIVE is enabled and is only used after relocation.
>>>>>>>> +        */
>>>>>>>> +       EVT_OF_LIVE_INIT,
>>>>>>>> +
>>>>>>>>             /**
>>>>>>>>              * @EVT_COUNT:
>>>>>>>>              * This constants holds the maximum event number + 1 and is used when
>>>>>>>>              * looping over all event classes.
>>>>>>>> diff --git a/lib/of_live.c b/lib/of_live.c
>>>>>>>> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a 100644
>>>>>>>> --- a/lib/of_live.c
>>>>>>>> +++ b/lib/of_live.c
>>>>>>>> @@ -10,8 +10,9 @@
>>>>>>>>
>>>>>>>>      #define LOG_CATEGORY   LOGC_DT
>>>>>>>>
>>>>>>>>      #include <abuf.h>
>>>>>>>> +#include <event.h>
>>>>>>>>      #include <log.h>
>>>>>>>>      #include <linux/libfdt.h>
>>>>>>>>      #include <of_live.h>
>>>>>>>>      #include <malloc.h>
>>>>>>>> @@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
>>>>>>>>                     return ret;
>>>>>>>>             }
>>>>>>>>             debug("%s: stop\n", __func__);
>>>>>>>>
>>>>>>>> +       event_notify_null(EVT_OF_LIVE_INIT);
>>>>>>>> +
>>>>>>>
>>>>>>> This should go in initr_of_live() since the function you are dealing
>>>>>>> with here is supposed to just do the live build.
>>>>>>
>>>>>> Well, we only every call this function from one place right now, but if
>>>>>> it was called multiple times for some reason then I would want to be
>>>>>> able to re-apply fixups to the new live tree.... I guess it should
>>>>>> probably pass in *rootp to the event handler, let me rework that.>
>>>>>
>>>>> There's no need to change the root, so what you have is find here.
>>>>>
>>>>>>> Same for the EFI_STUB thing which I just noticed, actually
>>>>>>
>>>>>> what EFI_STUB thing?>
>>>>>
>>>>> Oh, it's your EFI stub patch which isn't in Tom's tree yet. Just for
>>>>> when you get to it, then.
>>>>>
>>>>>>> Also please check for error
>>>>>>>
>>>>>>> Otherwise this seems OK to me. I do wonder why we can't use
>>>>>>> EVT_FT_FIXUP though. Could you add mention of that to your comment in
>>>>>>> event.h?
>>>>>>
>>>>>> Because FT_FIXUP is for fixing up the flat tree before starting the OS?
>>>>>> these are obviously different things imo, im not sure how i could
>>>>>> clarify this.>
>>>>>
>>>>> Well, what is the purpose of your code? Are you saying that it is used
>>>>> within U-Boot, but not passed to the OS?
>>>>
>>>> Yes, please read the cover letter and commit messages. FT_FIXUP allows
>>>> for the FDT that is about to be passed to the OS to be fixed up,
>>>> OF_LIVE_INIT signifies that U-Boot has finished building it's livetree.
>>>> The livetree is obviously not used outside of U-Boot, being a totally
>>>> custom in-memory representation of the DT.
>>>
>>> Oh, I wondered what '(which is not passed on to further boot stages)' meant.
>>
>> If you don't understand what my patch series does, it would save us both
>>    a lot of time if you asked me to clarify some specific point than to
>> reply with your stream-of-consciousness ponderings about what perhaps I
>> might be doing.
>>
>> I've tried -- again -- to explain what we're doing here, comments below.
>>
>>>
>>> Sorry, no, we can't do that.
>> We have been for months already
>>
>>> We are moving towards using livetree in U-Boot
>>
>> indeed, mach-snapdragon has been using it for 372 days now
>>
>> commit 1534186f2953d99dcbc757a19b43e4fac644e8a9
>> Author: Caleb Connolly <caleb.connolly@linaro.org>
>> Date:   Wed Apr 3 14:07:49 2024 +0200
>>
>>       qcom_defconfig: enable livetree
>>
>>> - there is already the fixup thing as mentioned.
>>
>> The FT_FIXUP event which has absolutely nothing to do with livetree or
>> this patch series and which is triggered at a different point in the
>> boot process for an entirely different purpose (preparing for the OS
>> rather than preparing for U-Boot itself)?
>>
>>> We just don't yet have the flattening at the end before boot.
>>
>> Which is exactly what we leverage in mach-snapdragon to bridge the gap
>> between U-Boot (a bootloader that intentionally has a simplified model
>> of the hardware) and upstream DT which contains a lot of additional
>> context which is (as far as U-Boot is concerned) noise that we have to
>> filter out.
>>
>>   > If you have time you could look at that.
>>
>> this is something literally nobody wants that wouldn't even be used in
>> most cases (since EFI DT FIXUP protocol passes you an FDT anyway).
>>
>>>
>>> Your event to modify the livetree is fine, but any modifications
>>> should be passed on to the OS.
>>
>> As stated clearly in the commit message and the cover letter, making
>> modifications that don't get passed on to the OS is the entire point.
>>
>> Assuming you didn't take the time to look at mach-snapdragon/of_fixup.c,
>> it is literally rewriting the livetree to patch out the USB superspeed
>> phy and only initialise USB in high-speed mode. This is quite simply
>> because U-Boot doesn't have PHY drivers for superspeed USB on Qualcomm
>> platforms yet.
>>
>> We absolutely don't want this to be passed on to the OS because it is
>> entirely an implementation detail in U-Boot. It's functionally All this
>> additional complexiequivalent to adding a bunch of "#ifdef
>> ARCH_SNAPDRAGON" ... "skip the superspeed phy" in the dwc3 driver.
>>
>> This is really basic abstraction, and hopefully you'll tell me that I
>> didn't need to explain all this because you did in fact take the time to
>> understand how mach-snapdragon is using the livetree before replying
>> (like by reading the big comment at the top of of_fixup.c).
>>
>>> If you don't want that, then your fixup would need to remove them, I suppose.
>>
>> why why why???
>>
>>   > Does the OS actually care, though?
>>
>> The OS would almost certainly be unhappy at the tree U-Boot is using
>> internally yes, because we don't bother fixing up stuff like the
>> usb-connector nodes which are unused in U-Boot.
>>
>> I know, I seem very frustrated, and I am. I have no problem with you
>> misunderstanding a patch or asking for clarifications, but you keep
>> repeatedly misunderstanding fundamental technical realities of an issue
>> or patch series and then seemingly doubling down and just saying
>> whatever comes to mind without exerting any effort to understand the
>> other side.
>>
>> ----
>>
>> You didn't ask, but I'm going to explain how I review patches to
>> hopefully convey some of my confusion here because I'm worried this is a
>> pattern that is repeating itself and it is starting to wear on me. I've
>> never been one to beat around the bush I guess.
>>
>> My first thought when I don't understand some change a patch is making
>> is "huh I wonder why they did that instead of this", but you will never
>> see me say that verbatim on the lists because I immediately go and read
>> the code to remind myself what it is doing and validate any assumptions
>> I might be making, as well as to try to understand the thought process
>> of the author.
>>
>> What you WILL see me say on the lists is either
>>
>> 1) A (usually backed up by references) explanation of why I think the
>> change is wrong, or
>>
>> 2) If I'm really confused I'll usually try to explain what I think
>> they're trying to do (again backed up by code references where relevant)
>> and ask for clarification.
>>
>> This way in (1) the author can say "oh yeah my bad, thanks for noticing
>> that" or point out what I missed that makes their change make sense.
>> This is easy, painless, everyone can go about their day (unless there is
>> a technical disagreement, but really that's besides the point here)
>>
>> In the second case, they can confirm that my understanding is correct
>> and respond to my (typically pointed) questions to clarify both how they
>> understand the code and whatever context I'm missing. Assuming I'm any
>> good at asking the right questions, this results in the same outcomes as
>> (1).
>>
>> This works because I'm trying to bridge the gap between my understanding
>> and the patch authors, I have my own mental model of how things work but
>> it could be wrong, so I want to convey what my model is so that it can
>> be corrected in that case, and so that if the authors understanding is
>> wrong then they can correct it.
>>
>> Your reviews don't do this, they leave me guessing wildly at what you
>> might possibly think my code is doing.
>>
>> There is no polite way for me to say "this comment you made is so out of
>> left field that I think you fundamentally misunderstand this patch", but
>> if you tell me what you think I'm trying to do (if you're at all
>> unsure), so I know what foundation you're working on, then I can easily
>> and politely say "ah your understanding here is wrong", you can re-do
>> your review with a correct mental model and I don't have to try and
>> parse things the way I have done above.
>>
>> If I were to go as far as to define a rule for patch review, it would be
>> "whenever the words 'i wonder' go through your head, try to find out
>> before sending your reply, and include your findings if you don't find a
>> satisfactory answer"
> 
> I'm sorry for upsetting you and I really hope I can improve the
> situation here. I also really appreciate you taking the time to write
> all these details.

Thank you>
> It is true that I struggle to find time for reviews at the moment, all
> in my spare time. Even then, I just checked and I am number two in the
> review tags for the most recent release. I am definitely pulling my
> weight here. Also I have knowledge of many of the subsystems. Yes I
> wish I could dig in and understand things more and I will try to do
> better. But my intentions are good.

I wouldn't have taken the time to reply if I thought otherwise.

> 
> Here is how U-Boot should (eventually) work:
> 
> Flat DT
>      |
> unflattened
>      |
> used in U-Boot               some-other-DT-packaged-with-OS
>      |                                       /
>      |                                   /
> this is where the fixups happen
>      |
> flattened
>      |
>      |
> passed to Linux
> 
> The branch is due to the fact that the OS may have a better
> devicetree, but ideally, in the fullness of time, the devicetree
> provided by firmware would be good enough.

we're on the same page here, though you're glossing over the complexity 
of potentially un-flattening an OS-provided DT, doing fixups, and then 
flattening again, maybe the cost is small enough that this is acceptable 
for the relative simplicity, but only if ALL boards can migrate to this 
approach.>
> I don't believe it is a good idea for U-Boot to use its own
> devicetree, separate from the Linux one. U-Boot should use the same
> one. That is the idea behind the dts-upstream feature in U-Boot, to
> make that easier.

Right, and this has been a huge success for Snapdragon. But there are 
cases like I described above where the Qualcomm drivers in U-Boot are 
lacking, and we need to "simplify" some interfaces, like removing 
superspeed from USB.

We can do this with really gross driver code, or we can have some 
relatively clean code that rewrites the live-tree. I picked the latter 
approach and leveraged the *convenience* that we can do this rewriting 
and not have to undo it because the livetree is not flattened again.

This code is already in mach-snapdragon and in use, you're now telling 
me that in the future we want to flatten the live-tree and pass it to 
the OS -- ok, sure, whatever, but we cannot do this on mach-snapdragon 
because the livetree that U-Boot is using has been patched in ways that 
would break the OS.

I am certain that we will find ways around this, like a generic way to 
do reversable hot-patches on the livetree (dynamic overlays I guess), or 
just something less elegant like having two copies of the livetree 
internally.

But what I've got from you is "no, you can't do what you're doing 
because it doesn't align with my vision" and I just don't think that's 
the right way to go about this.

> 
> For the EVT_FT_FIXUP event, the intent is to do fixups on the livetree
> as it should be more efficient. There's a lot of existing code that
> uses the flattree and I haven't figured out a clever way to migrate
> it.
> 
> But overall, the way to think of the livetree and flattree is that
> they are the same tree. We can unflatten, modify and flatten, but not
> unflatten, modify and throw away. We certainly don't maintain two
> separate trees within U-Boot.

In an ideal world this would be great, and I can appreciate why this 
mental model is appealing. But it isn't how U-Boot works today, and it's 
something we'll have to move towards. Maybe the dynamic overlays idea I 
mentioned above would be a good compromise?

I believe I understand how you see things and want them to work, I see 
the reality (and potential compromises) as a superset of your view. I 
think there are some assumptions I glossed over that could have made 
that clearer.

> 
> I suspect the main problem is that this part of U-Boot is in slow
> transition. Perhaps some better documentation of the eventual state
> would help?I think it would be better to propose your ideas in a way that ties in 
to what boards are doing today, and offers tangible benefits. The 
livetree could simplify a lot of FDT wrangling in U-Boot, dynamic 
overlays could be an appealing way for vendors to handle different SKUs, 
something like that?

So far the primary motivation I see from you is that this is what you 
think makes sense, but then you totally ignore how much worse it 
actually makes it for mach-snapdragon.

For a while (as you can see in this series) we had DTS overrides in 
U-Boot for some boards that we manually undid in ft_fixup, literally 
just for one property, and it was a total PITA.

The way I see it, the livetree is U-Boots internal representation of the 
DT and NOT something to pass on to the OS. It is literally "live". The 
desire to have U-Boot function with upstream DT without modifications is 
self-evident, obviously we want this, Qualcomm was the first to adopt 
OF_UPSTREAM (ahead of time, even).

However, once the FDT gets handed over to U-Boot, what U-Boot does 
internally should be treated like a black box (as we do for other 
consumers of DT). If U-Boot wants to only read the board compatible and 
then hardcode literally everything else per-board, so be it. U-Boots 
internal abstractions (like the livetree and the ability for board 
specific code to mess with it before drivers parse it) should have 
absolutely no bearing on whatever code comes after it.

To me this is obvious, and is the assumption I've been working from but 
I really should have made that explicit earlier.

Through that lens, I do really disagree with your idea that we should 
treat the DT as something pure to merely be looked at (and maybe 
tweaked) before being passed to the next stage. I think it's absurd to 
justify clobbering a perfectly good abstraction that U-Boot has today 
(whether intentionally or not).

> 
> If you are still unhappy, how about we organise a call to discuss
> this? I really don't think we are on the same page at all right now.
> It might help to reduce frustration.

Sure, I'd be happy to have a call next week to flesh out whatever our 
differences are and clarify what I've said above. I don't doubt that 
we'll be able to find a solution that doesn't involve doing away with 
our ability to abstract away board-specific requirements from generic 
drivers.

Today we do this livetree fixup already, but in between 
dm_bind()/of_to_platdata() and dm_probe() which is actually super duper 
brittle, because some properties have already been parsed. Introducing 
this event will let us make it much less brittle (and is required to 
change some of the properties I want to change with this series), can we 
please get this in?

I'll re-send this series (picked it a little prematurely, sorry for 
that) to fix the missing error check and add some data to the event 
(namely the root node pointer).

Whenever we get around to implementing your proposal to do fixups for 
the OS via livetree, perhaps a good migration path would be to have the 
OF_LIVE_INIT event also signal the intended use of the tree (for U-Boot 
or for the OS), then board using the FT_FIXUP event today can switch to 
using OF_LIVE_INIT and checking that property.

But for now I'd like to leave that out of scope if that's ok.

Kind regards,

> 
> Regards,
> Simon

Hi Caleb,

On Fri, 11 Apr 2025 at 05:52, Caleb Connolly <caleb.connolly@linaro.org> wrote:
>
>
>
> On 4/10/25 23:25, Simon Glass wrote:
> > Hi Caleb,
> >
> > On Thu, 10 Apr 2025 at 09:41, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>
> >> Hi Simon,
> >>
> >> On 4/10/25 16:15, Simon Glass wrote:
> >>> Hi Caleb,
> >>>
> >>> On Thu, 10 Apr 2025 at 08:04, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 4/10/25 15:07, Simon Glass wrote:
> >>>>> Hi Caleb,
> >>>>>
> >>>>> On Thu, 10 Apr 2025 at 07:00, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>>>>>
> >>>>>> Hi Simon,
> >>>>>>
> >>>>>> On 4/10/25 13:27, Simon Glass wrote:
> >>>>>>> Hi Caleb,
> >>>>>>>
> >>>>>>> On Wed, 9 Apr 2025 at 11:17, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> >>>>>>>>
> >>>>>>>> OF_LIVE offers a variety of benefits, one of them being that the live
> >>>>>>>> tree can be modified without caring about the underlying FDT. This is
> >>>>>>>> particularly valuable for working around U-Boot limitations like lacking
> >>>>>>>> USB superspeed support on Qualcomm platforms, no runtime OTG, or
> >>>>>>>> peripherals like the sdcard being broken (and displaying potentially
> >>>>>>>> worrying error messages).
> >>>>>>>>
> >>>>>>>> Add an event to signal when the live tree has been built so that we can
> >>>>>>>> apply fixups to it directly before devices are bound.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> >>>>>>>> ---
> >>>>>>>>      common/event.c  | 3 +++
> >>>>>>>>      include/event.h | 9 +++++++++
> >>>>>>>>      lib/of_live.c   | 3 +++
> >>>>>>>>      3 files changed, 15 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/common/event.c b/common/event.c
> >>>>>>>> index dda569d447851f559a83f98fb7b1f3543156eab5..8d7513eb10b61919e1e784481dfdcc076be14986 100644
> >>>>>>>> --- a/common/event.c
> >>>>>>>> +++ b/common/event.c
> >>>>>>>> @@ -47,8 +47,11 @@ const char *const type_name[] = {
> >>>>>>>>             "ft_fixup",
> >>>>>>>>
> >>>>>>>>             /* main loop events */
> >>>>>>>>             "main_loop",
> >>>>>>>> +
> >>>>>>>> +       /* livetree has been built */
> >>>>>>>> +       "of_live_init",
> >>>>>>>>      };
> >>>>>>>>
> >>>>>>>>      _Static_assert(ARRAY_SIZE(type_name) == EVT_COUNT, "event type_name size");
> >>>>>>>>      #endif
> >>>>>>>> diff --git a/include/event.h b/include/event.h
> >>>>>>>> index 75141a192a48b0931667632f41be8ff4d6139f7c..3fc673ba635ed45467aae8587705d37bef1c2a3f 100644
> >>>>>>>> --- a/include/event.h
> >>>>>>>> +++ b/include/event.h
> >>>>>>>> @@ -152,8 +152,17 @@ enum event_t {
> >>>>>>>>              * A non-zero return value causes the boot to fail.
> >>>>>>>>              */
> >>>>>>>>             EVT_MAIN_LOOP,
> >>>>>>>>
> >>>>>>>> +       /**
> >>>>>>>> +        * @EVT_OF_LIVE_INIT:
> >>>>>>>> +        * This event is triggered immediately after the live device tree has been
> >>>>>>>> +        * built. This allows for machine specific fixups to be done to the live tree
> >>>>>>>> +        * (like disabling known-unsupported devices) before DM init happens. This
> >>>>>>>> +        * event is only available if OF_LIVE is enabled and is only used after relocation.
> >>>>>>>> +        */
> >>>>>>>> +       EVT_OF_LIVE_INIT,
> >>>>>>>> +
> >>>>>>>>             /**
> >>>>>>>>              * @EVT_COUNT:
> >>>>>>>>              * This constants holds the maximum event number + 1 and is used when
> >>>>>>>>              * looping over all event classes.
> >>>>>>>> diff --git a/lib/of_live.c b/lib/of_live.c
> >>>>>>>> index 90b9459ede313e492e28c8556c730f3bd8aaa9df..e1962b8f1fb9d8c2c87d04ca4e238a1e4d00376a 100644
> >>>>>>>> --- a/lib/of_live.c
> >>>>>>>> +++ b/lib/of_live.c
> >>>>>>>> @@ -10,8 +10,9 @@
> >>>>>>>>
> >>>>>>>>      #define LOG_CATEGORY   LOGC_DT
> >>>>>>>>
> >>>>>>>>      #include <abuf.h>
> >>>>>>>> +#include <event.h>
> >>>>>>>>      #include <log.h>
> >>>>>>>>      #include <linux/libfdt.h>
> >>>>>>>>      #include <of_live.h>
> >>>>>>>>      #include <malloc.h>
> >>>>>>>> @@ -334,8 +335,10 @@ int of_live_build(const void *fdt_blob, struct device_node **rootp)
> >>>>>>>>                     return ret;
> >>>>>>>>             }
> >>>>>>>>             debug("%s: stop\n", __func__);
> >>>>>>>>
> >>>>>>>> +       event_notify_null(EVT_OF_LIVE_INIT);
> >>>>>>>> +
> >>>>>>>
> >>>>>>> This should go in initr_of_live() since the function you are dealing
> >>>>>>> with here is supposed to just do the live build.
> >>>>>>
> >>>>>> Well, we only every call this function from one place right now, but if
> >>>>>> it was called multiple times for some reason then I would want to be
> >>>>>> able to re-apply fixups to the new live tree.... I guess it should
> >>>>>> probably pass in *rootp to the event handler, let me rework that.>
> >>>>>
> >>>>> There's no need to change the root, so what you have is find here.
> >>>>>
> >>>>>>> Same for the EFI_STUB thing which I just noticed, actually
> >>>>>>
> >>>>>> what EFI_STUB thing?>
> >>>>>
> >>>>> Oh, it's your EFI stub patch which isn't in Tom's tree yet. Just for
> >>>>> when you get to it, then.
> >>>>>
> >>>>>>> Also please check for error
> >>>>>>>
> >>>>>>> Otherwise this seems OK to me. I do wonder why we can't use
> >>>>>>> EVT_FT_FIXUP though. Could you add mention of that to your comment in
> >>>>>>> event.h?
> >>>>>>
> >>>>>> Because FT_FIXUP is for fixing up the flat tree before starting the OS?
> >>>>>> these are obviously different things imo, im not sure how i could
> >>>>>> clarify this.>
> >>>>>
> >>>>> Well, what is the purpose of your code? Are you saying that it is used
> >>>>> within U-Boot, but not passed to the OS?
> >>>>
> >>>> Yes, please read the cover letter and commit messages. FT_FIXUP allows
> >>>> for the FDT that is about to be passed to the OS to be fixed up,
> >>>> OF_LIVE_INIT signifies that U-Boot has finished building it's livetree.
> >>>> The livetree is obviously not used outside of U-Boot, being a totally
> >>>> custom in-memory representation of the DT.
> >>>
> >>> Oh, I wondered what '(which is not passed on to further boot stages)' meant.
> >>
> >> If you don't understand what my patch series does, it would save us both
> >>    a lot of time if you asked me to clarify some specific point than to
> >> reply with your stream-of-consciousness ponderings about what perhaps I
> >> might be doing.
> >>
> >> I've tried -- again -- to explain what we're doing here, comments below.
> >>
> >>>
> >>> Sorry, no, we can't do that.
> >> We have been for months already
> >>
> >>> We are moving towards using livetree in U-Boot
> >>
> >> indeed, mach-snapdragon has been using it for 372 days now
> >>
> >> commit 1534186f2953d99dcbc757a19b43e4fac644e8a9
> >> Author: Caleb Connolly <caleb.connolly@linaro.org>
> >> Date:   Wed Apr 3 14:07:49 2024 +0200
> >>
> >>       qcom_defconfig: enable livetree
> >>
> >>> - there is already the fixup thing as mentioned.
> >>
> >> The FT_FIXUP event which has absolutely nothing to do with livetree or
> >> this patch series and which is triggered at a different point in the
> >> boot process for an entirely different purpose (preparing for the OS
> >> rather than preparing for U-Boot itself)?
> >>
> >>> We just don't yet have the flattening at the end before boot.
> >>
> >> Which is exactly what we leverage in mach-snapdragon to bridge the gap
> >> between U-Boot (a bootloader that intentionally has a simplified model
> >> of the hardware) and upstream DT which contains a lot of additional
> >> context which is (as far as U-Boot is concerned) noise that we have to
> >> filter out.
> >>
> >>   > If you have time you could look at that.
> >>
> >> this is something literally nobody wants that wouldn't even be used in
> >> most cases (since EFI DT FIXUP protocol passes you an FDT anyway).
> >>
> >>>
> >>> Your event to modify the livetree is fine, but any modifications
> >>> should be passed on to the OS.
> >>
> >> As stated clearly in the commit message and the cover letter, making
> >> modifications that don't get passed on to the OS is the entire point.
> >>
> >> Assuming you didn't take the time to look at mach-snapdragon/of_fixup.c,
> >> it is literally rewriting the livetree to patch out the USB superspeed
> >> phy and only initialise USB in high-speed mode. This is quite simply
> >> because U-Boot doesn't have PHY drivers for superspeed USB on Qualcomm
> >> platforms yet.
> >>
> >> We absolutely don't want this to be passed on to the OS because it is
> >> entirely an implementation detail in U-Boot. It's functionally All this
> >> additional complexiequivalent to adding a bunch of "#ifdef
> >> ARCH_SNAPDRAGON" ... "skip the superspeed phy" in the dwc3 driver.
> >>
> >> This is really basic abstraction, and hopefully you'll tell me that I
> >> didn't need to explain all this because you did in fact take the time to
> >> understand how mach-snapdragon is using the livetree before replying
> >> (like by reading the big comment at the top of of_fixup.c).
> >>
> >>> If you don't want that, then your fixup would need to remove them, I suppose.
> >>
> >> why why why???
> >>
> >>   > Does the OS actually care, though?
> >>
> >> The OS would almost certainly be unhappy at the tree U-Boot is using
> >> internally yes, because we don't bother fixing up stuff like the
> >> usb-connector nodes which are unused in U-Boot.
> >>
> >> I know, I seem very frustrated, and I am. I have no problem with you
> >> misunderstanding a patch or asking for clarifications, but you keep
> >> repeatedly misunderstanding fundamental technical realities of an issue
> >> or patch series and then seemingly doubling down and just saying
> >> whatever comes to mind without exerting any effort to understand the
> >> other side.
> >>
> >> ----
> >>
> >> You didn't ask, but I'm going to explain how I review patches to
> >> hopefully convey some of my confusion here because I'm worried this is a
> >> pattern that is repeating itself and it is starting to wear on me. I've
> >> never been one to beat around the bush I guess.
> >>
> >> My first thought when I don't understand some change a patch is making
> >> is "huh I wonder why they did that instead of this", but you will never
> >> see me say that verbatim on the lists because I immediately go and read
> >> the code to remind myself what it is doing and validate any assumptions
> >> I might be making, as well as to try to understand the thought process
> >> of the author.
> >>
> >> What you WILL see me say on the lists is either
> >>
> >> 1) A (usually backed up by references) explanation of why I think the
> >> change is wrong, or
> >>
> >> 2) If I'm really confused I'll usually try to explain what I think
> >> they're trying to do (again backed up by code references where relevant)
> >> and ask for clarification.
> >>
> >> This way in (1) the author can say "oh yeah my bad, thanks for noticing
> >> that" or point out what I missed that makes their change make sense.
> >> This is easy, painless, everyone can go about their day (unless there is
> >> a technical disagreement, but really that's besides the point here)
> >>
> >> In the second case, they can confirm that my understanding is correct
> >> and respond to my (typically pointed) questions to clarify both how they
> >> understand the code and whatever context I'm missing. Assuming I'm any
> >> good at asking the right questions, this results in the same outcomes as
> >> (1).
> >>
> >> This works because I'm trying to bridge the gap between my understanding
> >> and the patch authors, I have my own mental model of how things work but
> >> it could be wrong, so I want to convey what my model is so that it can
> >> be corrected in that case, and so that if the authors understanding is
> >> wrong then they can correct it.
> >>
> >> Your reviews don't do this, they leave me guessing wildly at what you
> >> might possibly think my code is doing.
> >>
> >> There is no polite way for me to say "this comment you made is so out of
> >> left field that I think you fundamentally misunderstand this patch", but
> >> if you tell me what you think I'm trying to do (if you're at all
> >> unsure), so I know what foundation you're working on, then I can easily
> >> and politely say "ah your understanding here is wrong", you can re-do
> >> your review with a correct mental model and I don't have to try and
> >> parse things the way I have done above.
> >>
> >> If I were to go as far as to define a rule for patch review, it would be
> >> "whenever the words 'i wonder' go through your head, try to find out
> >> before sending your reply, and include your findings if you don't find a
> >> satisfactory answer"
> >
> > I'm sorry for upsetting you and I really hope I can improve the
> > situation here. I also really appreciate you taking the time to write
> > all these details.
>
> Thank you>
> > It is true that I struggle to find time for reviews at the moment, all
> > in my spare time. Even then, I just checked and I am number two in the
> > review tags for the most recent release. I am definitely pulling my
> > weight here. Also I have knowledge of many of the subsystems. Yes I
> > wish I could dig in and understand things more and I will try to do
> > better. But my intentions are good.
>
> I wouldn't have taken the time to reply if I thought otherwise.
>
> >
> > Here is how U-Boot should (eventually) work:
> >
> > Flat DT
> >      |
> > unflattened
> >      |
> > used in U-Boot               some-other-DT-packaged-with-OS
> >      |                                       /
> >      |                                   /
> > this is where the fixups happen
> >      |
> > flattened
> >      |
> >      |
> > passed to Linux
> >
> > The branch is due to the fact that the OS may have a better
> > devicetree, but ideally, in the fullness of time, the devicetree
> > provided by firmware would be good enough.
>
> we're on the same page here, though you're glossing over the complexity
> of potentially un-flattening an OS-provided DT, doing fixups, and then
> flattening again, maybe the cost is small enough that this is acceptable
> for the relative simplicity, but only if ALL boards can migrate to this
> approach.>
> > I don't believe it is a good idea for U-Boot to use its own
> > devicetree, separate from the Linux one. U-Boot should use the same
> > one. That is the idea behind the dts-upstream feature in U-Boot, to
> > make that easier.
>
> Right, and this has been a huge success for Snapdragon. But there are
> cases like I described above where the Qualcomm drivers in U-Boot are
> lacking, and we need to "simplify" some interfaces, like removing
> superspeed from USB.
>
> We can do this with really gross driver code, or we can have some
> relatively clean code that rewrites the live-tree. I picked the latter
> approach and leveraged the *convenience* that we can do this rewriting
> and not have to undo it because the livetree is not flattened again.
>
> This code is already in mach-snapdragon and in use, you're now telling
> me that in the future we want to flatten the live-tree and pass it to
> the OS -- ok, sure, whatever, but we cannot do this on mach-snapdragon
> because the livetree that U-Boot is using has been patched in ways that
> would break the OS.
>
> I am certain that we will find ways around this, like a generic way to
> do reversable hot-patches on the livetree (dynamic overlays I guess), or
> just something less elegant like having two copies of the livetree
> internally.
>
> But what I've got from you is "no, you can't do what you're doing
> because it doesn't align with my vision" and I just don't think that's
> the right way to go about this.

Well there are two things here:
- what you want to do for your particular board
- how you want to change the semantics of livetree/flatree in U-Boot

A simple (alternative) way to solve this would be to add a flag for
USB3 to struct usb_plat, set it in your board code (after devices are
bound) and then update the USB driver to respect the flag.

>
> >
> > For the EVT_FT_FIXUP event, the intent is to do fixups on the livetree
> > as it should be more efficient. There's a lot of existing code that
> > uses the flattree and I haven't figured out a clever way to migrate
> > it.
> >
> > But overall, the way to think of the livetree and flattree is that
> > they are the same tree. We can unflatten, modify and flatten, but not
> > unflatten, modify and throw away. We certainly don't maintain two
> > separate trees within U-Boot.
>
> In an ideal world this would be great, and I can appreciate why this
> mental model is appealing. But it isn't how U-Boot works today, and it's
> something we'll have to move towards. Maybe the dynamic overlays idea I
> mentioned above would be a good compromise?
>
> I believe I understand how you see things and want them to work, I see
> the reality (and potential compromises) as a superset of your view. I
> think there are some assumptions I glossed over that could have made
> that clearer.
>
> >
> > I suspect the main problem is that this part of U-Boot is in slow
> > transition. Perhaps some better documentation of the eventual state
> > would help?I think it would be better to propose your ideas in a way that ties in
> to what boards are doing today, and offers tangible benefits. The
> livetree could simplify a lot of FDT wrangling in U-Boot, dynamic
> overlays could be an appealing way for vendors to handle different SKUs,
> something like that?
>
> So far the primary motivation I see from you is that this is what you
> think makes sense, but then you totally ignore how much worse it
> actually makes it for mach-snapdragon.
>
> For a while (as you can see in this series) we had DTS overrides in
> U-Boot for some boards that we manually undid in ft_fixup, literally
> just for one property, and it was a total PITA.
>
> The way I see it, the livetree is U-Boots internal representation of the
> DT and NOT something to pass on to the OS. It is literally "live". The
> desire to have U-Boot function with upstream DT without modifications is
> self-evident, obviously we want this, Qualcomm was the first to adopt
> OF_UPSTREAM (ahead of time, even).

That's incorrect, though. The livetree is the 'only' tree - the
flattree should have gone away. You are actually using a back door
here. When it goes away, what will you do?

>
> However, once the FDT gets handed over to U-Boot, what U-Boot does
> internally should be treated like a black box (as we do for other
> consumers of DT). If U-Boot wants to only read the board compatible and
> then hardcode literally everything else per-board, so be it. U-Boots
> internal abstractions (like the livetree and the ability for board
> specific code to mess with it before drivers parse it) should have
> absolutely no bearing on whatever code comes after it.
>
> To me this is obvious, and is the assumption I've been working from but
> I really should have made that explicit earlier.
>
> Through that lens, I do really disagree with your idea that we should
> treat the DT as something pure to merely be looked at (and maybe
> tweaked) before being passed to the next stage. I think it's absurd to
> justify clobbering a perfectly good abstraction that U-Boot has today
> (whether intentionally or not).

I have no issue with changing the devicetree. It's the idea that it is
a temporary thing that is the problem. Once we are doing fixups in the
livetree, what do we do with your changes? They need to be reverted in
the DT fixups, I assume.

>
> >
> > If you are still unhappy, how about we organise a call to discuss
> > this? I really don't think we are on the same page at all right now.
> > It might help to reduce frustration.
>
> Sure, I'd be happy to have a call next week to flesh out whatever our
> differences are and clarify what I've said above. I don't doubt that
> we'll be able to find a solution that doesn't involve doing away with
> our ability to abstract away board-specific requirements from generic
> drivers.
>
> Today we do this livetree fixup already, but in between
> dm_bind()/of_to_platdata() and dm_probe() which is actually super duper
> brittle, because some properties have already been parsed. Introducing
> this event will let us make it much less brittle (and is required to
> change some of the properties I want to change with this series), can we
> please get this in?

Right, I don't like that either.

I suppose this issue is caused by dts-upstream, since in U-Boot we
could normally just change the dts temporarily. How come you can't use
a u-boot.dtsi file to make the changes?

>
> I'll re-send this series (picked it a little prematurely, sorry for
> that) to fix the missing error check and add some data to the event
> (namely the root node pointer).
>
> Whenever we get around to implementing your proposal to do fixups for
> the OS via livetree, perhaps a good migration path would be to have the
> OF_LIVE_INIT event also signal the intended use of the tree (for U-Boot
> or for the OS), then board using the FT_FIXUP event today can switch to
> using OF_LIVE_INIT and checking that property.
>
> But for now I'd like to leave that out of scope if that's ok.

That's an interesting idea, having a tree that has been hacked for
U-Boot only, while keeping a second tree around that we will use for
fixups. I can definitely see situations where it could be useful.

It might be confusing / inefficient to have multiple trees, though.
Wouldn't we just want to change a few nodes and then revert them
later? But that is probably brittle, or annoying, as you say. So I
think multiple trees for this case makes at least some sense to me.
It's the sort of thing that could be useful in the early days of a
board's bringup.

Anyway, if you were willing to do that as a follow-up then I would
normally be OK with your series and I'm quite happy to have it in my
tree, since I can change it later if needed. But for Tom's tree I am
concerned that:

- I won't be able to change things later (e.g. by making fixup from
livetree work, which you are not offering to do and fair enough),
since any patch that Linaro objects to doesn't go in to Tom's tree
(EFI, bloblist, devicetree, etc.)
- various of my series have been blocked by work that I have offered
to do in future but Tom has required that I do first (including
Linaro's lwip)

So yes, I suggest we have a call at some point to talk through the
options, e.g. I might be able to contribute a few patches to help
here, once I have your ideas from some of my questions above. I'm free
from Tuesday next week.

Regards,
Simon