[ImageBuilder 0/2] Use lopper to generate partial dts

[ImageBuilder 0/2] Use lopper to generate partial dts

[Michal Orzel]

This patch series introduces support to generate automatically passthrough
device trees using lopper. This feature should be used with care as the
corresponding lopper changes are still in an early support state. Current
integration has been tested with several devices from ZynqMP ZCU102 board
e.g. serial, spi, ahci, mmc.

When using this feature, make sure to use the latest lopper's master branch
status [1].

[1] https://github.com/devicetree-org/lopper

Michal Orzel (2):
  Refactor sanity_check_partial_dts
  Add support for lopper to generate partial dts

 README.md                | 22 ++++++++++--
 scripts/common           | 73 +++++++++++++++++++++++++++++-----------
 scripts/uboot-script-gen | 17 ++++++++--
 3 files changed, 88 insertions(+), 24 deletions(-)

-- 
2.25.1

[ImageBuilder 1/2] Refactor sanity_check_partial_dts

[Michal Orzel]

Currently function sanity_check_partial_dts from scripts/common takes
three arguments where the last two (repo, dir) are used always in
conjuction to form a path to a directory storing partial dts. Modify the
function to take only two arguments where the second one is to be a path
to a directory storing partial dts. This will help reusing this function
in the future to perform sanity checks on partial dts that do not
originate from a repository.

Modify compile_merge_partial_dts to take this change into account.

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 scripts/common | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/scripts/common b/scripts/common
index 25c041270c29..ccad03d82b30 100644
--- a/scripts/common
+++ b/scripts/common
@@ -40,8 +40,7 @@ function get_next_phandle()
 function sanity_check_partial_dts()
 {
     local domU_passthrough_path="$1"
-    local repo="$2"
-    local dir="$3"
+    local partial_dts_dir="$2"
     local address_cells_val
     local size_cells_val
     local tmpdtb
@@ -51,7 +50,7 @@ function sanity_check_partial_dts()
     for devpath in $domU_passthrough_path
     do
         file=${devpath##*/}
-        file="$repo"/"$dir"/"$file".dts
+        file="$partial_dts_dir"/"$file".dts
 
         if ! test -f "$file"
         then
@@ -96,6 +95,7 @@ function compile_merge_partial_dts()
     local dtb_dir=$1
     local repo=$(echo "$2" | awk '{print $1}')
     local dir=$(echo "$2" | awk '{print $2}')
+    local partial_dts_dir
     local tmp
     local tmpdts
     local file
@@ -123,6 +123,7 @@ function compile_merge_partial_dts()
         dir="."
     fi
 
+    partial_dts_dir="$repo"/"$dir"
     i=0
     while test $i -lt $NUM_DOMUS
     do
@@ -132,7 +133,7 @@ function compile_merge_partial_dts()
             return 1
         fi
 
-        sanity_check_partial_dts "${DOMU_PASSTHROUGH_PATHS[$i]}" "$repo" "$dir"
+        sanity_check_partial_dts "${DOMU_PASSTHROUGH_PATHS[$i]}" "$partial_dts_dir"
         if test $? -ne 0
         then
             return 1
@@ -146,7 +147,7 @@ function compile_merge_partial_dts()
         for devpath in ${DOMU_PASSTHROUGH_PATHS[$i]}
         do
             file=${devpath##*/}
-            file="$repo"/"$dir"/"$file".dts
+            file="$partial_dts_dir"/"$file".dts
 
             # All the subsequent dts files should not have dts version mentioned
             if test $j -gt 1
-- 
2.25.1

[ImageBuilder 2/2] Add support for lopper to generate partial dts

[Michal Orzel]

Currently ImageBuilder can compile and merge partial dts obtained from
a repository specified using PASSTHROUGH_DTS_REPO. With the recent
changes done in the lopper, we can use it to generate partial dts
automatically (to some extent as this is still an early support).

Introduce LOPPER_PATH option to specify a path to a lopper.py script,
that if set, will invoke lopper to generate partial dts for the
passthrough devices specified in DOMU_PASSTHROUGH_PATHS.

Introduce LOPPER_CMD option to specify custom command line arguments
(if needed) for lopper's extract assist.

Example usage:
LOPPER_PATH="/home/user/lopper/lopper.py"
DOMU_PASSTHROUGH_PATHS[0]="/axi/spi@ff0f0000 /axi/serial@ff010000"

Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 README.md                | 22 ++++++++++++--
 scripts/common           | 64 ++++++++++++++++++++++++++++++----------
 scripts/uboot-script-gen | 17 +++++++++--
 3 files changed, 83 insertions(+), 20 deletions(-)

diff --git a/README.md b/README.md
index da9ba788a3bf..aaee0939b589 100644
--- a/README.md
+++ b/README.md
@@ -128,6 +128,19 @@ Where:
 - DT_OVERLAY[number] specifies the path to the hosts device tree overlays
   to be added at boot time in u-boot
 
+- LOPPER_PATH specifies the path to lopper.py script. This is optional.
+  However, if this is specified, then DOMU_PASSTHROUGH_PATHS[number] need
+  to be specified. uboot-script-gen will invoke lopper to generate the partial
+  device trees which have been specified in DOMU_PASSTHROUGH_PATHS[number].
+  This option is currently in experimental state as the corresponding lopper
+  changes are still in an early support state.
+
+- LOPPER_CMD specifies the command line arguments for lopper's extract assist.
+  This is optional and only applicable when LOPPER_PATH is specified. Only to be
+  used to specify which nodes to include (using -i <node_name>) and which
+  nodes/properties to exclude (using -x <regex>). If not set at all, the default
+  one is used applicable for ZynqMP MPSoC boards.
+
 - NUM_DOMUS specifies how many Dom0-less DomUs to load
 
 - DOMU_KERNEL[number] specifies the DomU kernel to use.
@@ -140,7 +153,7 @@ Where:
 - DOMU_PASSTHROUGH_PATHS[number] specifies the passthrough devices (
   separated by spaces). It adds "xen,passthrough" to the corresponding
   dtb nodes in xen device tree blob.
-  This option is valid in the following two cases:
+  This option is valid in the following cases:
 
   1. When PASSTHROUGH_DTS_REPO is provided.
   With this option, the partial device trees (corresponding to the
@@ -149,7 +162,12 @@ Where:
   Note it assumes that the names of the partial device trees will match
   to the names of the devices specified here.
 
-  2. When DOMU_NOBOOT[number] is provided. In this case, it will only
+  2. When LOPPER_PATH is provided.
+  With this option, the partial device trees (corresponding to the
+  passthrough devices) are generated by the lopper and then compiled and merged
+  by ImageBuilder to be used as DOMU[number] device tree blob.
+
+  3. When DOMU_NOBOOT[number] is provided. In this case, it will only
   add "xen,passthrough" as mentioned before.
 
 - DOMU_PASSTHROUGH_DTB[number] specifies the passthrough device trees
diff --git a/scripts/common b/scripts/common
index ccad03d82b30..680c5090cd07 100644
--- a/scripts/common
+++ b/scripts/common
@@ -9,6 +9,9 @@
 # - NUM_DOMUS
 # - DOMU_PASSTHROUGH_PATHS
 # - DOMU_PASSTHROUGH_DTB
+# - LOPPER_PATH
+# - LOPPER_CMD
+# - DEVICE_TREE
 
 tmp_files=()
 tmp_dirs=()
@@ -99,31 +102,41 @@ function compile_merge_partial_dts()
     local tmp
     local tmpdts
     local file
+    local node
     local i
     local j
 
-    if [[ "$repo" =~ .*@.*:.* ]]
+    if test "$repo"
     then
-        tmp=`mktemp -d`
-        tmp_dirs+=($tmp)
-
-        echo "Cloning git repo \"$git_repo\""
-        git clone "$repo" $tmp
-        if test $? -ne 0
+        # Partial dts will be obtained from PASSTHROUGH_DTS_REPO
+        if [[ "$repo" =~ .*@.*:.* ]]
         then
-            echo "Error occurred while cloning \"$git_repo\""
-            return 1
-        fi
+            tmp=`mktemp -d`
+            tmp_dirs+=($tmp)
 
-        repo=$tmp
-    fi
+            echo "Cloning git repo \"$git_repo\""
+            git clone "$repo" $tmp
+            if test $? -ne 0
+            then
+                echo "Error occurred while cloning \"$git_repo\""
+                return 1
+            fi
 
-    if test -z "$dir"
-    then
-        dir="."
+            repo=$tmp
+        fi
+
+        if test -z "$dir"
+        then
+            dir="."
+        fi
+        partial_dts_dir="$repo"/"$dir"
+    else
+        # Partial dts will be generated by the lopper
+        tmp=`mktemp -d`
+        tmp_dirs+=($tmp)
+        partial_dts_dir="$tmp"
     fi
 
-    partial_dts_dir="$repo"/"$dir"
     i=0
     while test $i -lt $NUM_DOMUS
     do
@@ -133,6 +146,25 @@ function compile_merge_partial_dts()
             return 1
         fi
 
+        if test -z "$repo"
+        then
+            # Generate partial dts using lopper
+            for devpath in ${DOMU_PASSTHROUGH_PATHS[$i]}
+            do
+                node=${devpath##*/}
+                file="$partial_dts_dir"/"$node".dts
+
+                $LOPPER_PATH --permissive -f $DEVICE_TREE \
+                -- extract -t $devpath $LOPPER_CMD \
+                -- extract-xen -t $node -o $file
+
+                if test $? -ne 0
+                then
+                    return 1
+                fi
+            done
+        fi
+
         sanity_check_partial_dts "${DOMU_PASSTHROUGH_PATHS[$i]}" "$partial_dts_dir"
         if test $? -ne 0
         then
diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
index 1f8ab5ffd193..84a68d6bd0b0 100755
--- a/scripts/uboot-script-gen
+++ b/scripts/uboot-script-gen
@@ -1138,10 +1138,23 @@ fi
 # tftp or move the files to a partition
 cd "$uboot_dir"
 
-if test "$PASSTHROUGH_DTS_REPO"
+# If both PASSTHROUGH_DTS_REPO and LOPPER_PATH options are specified,
+# the former takes precedence because the partial device trees are already
+# created (probably tested), hence the reliability is higher than using lopper.
+if test "$PASSTHROUGH_DTS_REPO" || test "$LOPPER_PATH"
 then
     output_dir=`mktemp -d "partial-dtbs-XXX"`
-    compile_merge_partial_dts $output_dir "$PASSTHROUGH_DTS_REPO"
+    if test "$PASSTHROUGH_DTS_REPO"
+    then
+        compile_merge_partial_dts $output_dir "$PASSTHROUGH_DTS_REPO"
+    else
+        if test -z "$LOPPER_CMD"
+        then
+            # Default for ZynqMP MPSoC
+            LOPPER_CMD="-i zynqmp-firmware -x interrupt-controller -x pinctrl -x power-domains -x resets -x current-speed"
+        fi
+        compile_merge_partial_dts $output_dir
+    fi
     if test $? -ne 0
     then
         # Remove the output dir holding the partial dtbs in case of any error
-- 
2.25.1

Re: [ImageBuilder 0/2] Use lopper to generate partial dts

[Ayan Kumar Halder]

Hi Michal,

On 12/09/2022 12:59, Michal Orzel wrote:
> This patch series introduces support to generate automatically passthrough
> device trees using lopper. This feature should be used with care as the
> corresponding lopper changes are still in an early support state. Current
> integration has been tested with several devices from ZynqMP ZCU102 board
> e.g. serial, spi, ahci, mmc.
>
> When using this feature, make sure to use the latest lopper's master branch
> status [1].

I am guessing that this is the first time the imagebuilder is using 
script from an external repo. There might always be a possibility that 
future changes to lopper (master branch) might not be backward 
compatible or might break something in imagebuilder.

As such, will it make things better if lopper is included as a 
gitsubmodule for imagebuilder. This way a specific revision of lopper 
will be in sync with a specific revision of imagebuilder.

Please let me know your thoughts.

- Ayan

>
> [1] https://github.com/devicetree-org/lopper
>
> Michal Orzel (2):
>    Refactor sanity_check_partial_dts
>    Add support for lopper to generate partial dts
>
>   README.md                | 22 ++++++++++--
>   scripts/common           | 73 +++++++++++++++++++++++++++++-----------
>   scripts/uboot-script-gen | 17 ++++++++--
>   3 files changed, 88 insertions(+), 24 deletions(-)
>

Re: [ImageBuilder 1/2] Refactor sanity_check_partial_dts

[Ayan Kumar Halder]

On 12/09/2022 12:59, Michal Orzel wrote:
> Currently function sanity_check_partial_dts from scripts/common takes
> three arguments where the last two (repo, dir) are used always in
> conjuction to form a path to a directory storing partial dts. Modify the
> function to take only two arguments where the second one is to be a path
> to a directory storing partial dts. This will help reusing this function
> in the future to perform sanity checks on partial dts that do not
> originate from a repository.
>
> Modify compile_merge_partial_dts to take this change into account.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Ayan Kumar Halder <ayankuma@amd.com>
> ---
>   scripts/common | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/common b/scripts/common
> index 25c041270c29..ccad03d82b30 100644
> --- a/scripts/common
> +++ b/scripts/common
> @@ -40,8 +40,7 @@ function get_next_phandle()
>   function sanity_check_partial_dts()
>   {
>       local domU_passthrough_path="$1"
> -    local repo="$2"
> -    local dir="$3"
> +    local partial_dts_dir="$2"
>       local address_cells_val
>       local size_cells_val
>       local tmpdtb
> @@ -51,7 +50,7 @@ function sanity_check_partial_dts()
>       for devpath in $domU_passthrough_path
>       do
>           file=${devpath##*/}
> -        file="$repo"/"$dir"/"$file".dts
> +        file="$partial_dts_dir"/"$file".dts
>   
>           if ! test -f "$file"
>           then
> @@ -96,6 +95,7 @@ function compile_merge_partial_dts()
>       local dtb_dir=$1
>       local repo=$(echo "$2" | awk '{print $1}')
>       local dir=$(echo "$2" | awk '{print $2}')
> +    local partial_dts_dir
>       local tmp
>       local tmpdts
>       local file
> @@ -123,6 +123,7 @@ function compile_merge_partial_dts()
>           dir="."
>       fi
>   
> +    partial_dts_dir="$repo"/"$dir"
>       i=0
>       while test $i -lt $NUM_DOMUS
>       do
> @@ -132,7 +133,7 @@ function compile_merge_partial_dts()
>               return 1
>           fi
>   
> -        sanity_check_partial_dts "${DOMU_PASSTHROUGH_PATHS[$i]}" "$repo" "$dir"
> +        sanity_check_partial_dts "${DOMU_PASSTHROUGH_PATHS[$i]}" "$partial_dts_dir"
>           if test $? -ne 0
>           then
>               return 1
> @@ -146,7 +147,7 @@ function compile_merge_partial_dts()
>           for devpath in ${DOMU_PASSTHROUGH_PATHS[$i]}
>           do
>               file=${devpath##*/}
> -            file="$repo"/"$dir"/"$file".dts
> +            file="$partial_dts_dir"/"$file".dts
>   
>               # All the subsequent dts files should not have dts version mentioned
>               if test $j -gt 1

Re: [ImageBuilder 2/2] Add support for lopper to generate partial dts

[Ayan Kumar Halder]

Hi Michal,

On 12/09/2022 12:59, Michal Orzel wrote:
> Currently ImageBuilder can compile and merge partial dts obtained from
> a repository specified using PASSTHROUGH_DTS_REPO. With the recent
> changes done in the lopper, we can use it to generate partial dts
> automatically (to some extent as this is still an early support).
>
> Introduce LOPPER_PATH option to specify a path to a lopper.py script,
> that if set, will invoke lopper to generate partial dts for the
> passthrough devices specified in DOMU_PASSTHROUGH_PATHS.
>
> Introduce LOPPER_CMD option to specify custom command line arguments
> (if needed) for lopper's extract assist.
>
> Example usage:
> LOPPER_PATH="/home/user/lopper/lopper.py"
> DOMU_PASSTHROUGH_PATHS[0]="/axi/spi@ff0f0000 /axi/serial@ff010000"
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>   README.md                | 22 ++++++++++++--
>   scripts/common           | 64 ++++++++++++++++++++++++++++++----------
>   scripts/uboot-script-gen | 17 +++++++++--
>   3 files changed, 83 insertions(+), 20 deletions(-)
>
> diff --git a/README.md b/README.md
> index da9ba788a3bf..aaee0939b589 100644
> --- a/README.md
> +++ b/README.md
> @@ -128,6 +128,19 @@ Where:
>   - DT_OVERLAY[number] specifies the path to the hosts device tree overlays
>     to be added at boot time in u-boot
>   
> +- LOPPER_PATH specifies the path to lopper.py script. This is optional.
> +  However, if this is specified, then DOMU_PASSTHROUGH_PATHS[number] need
> +  to be specified. uboot-script-gen will invoke lopper to generate the partial
> +  device trees which have been specified in DOMU_PASSTHROUGH_PATHS[number].
> +  This option is currently in experimental state as the corresponding lopper
> +  changes are still in an early support state.
> +
> +- LOPPER_CMD specifies the command line arguments for lopper's extract assist.
> +  This is optional and only applicable when LOPPER_PATH is specified. Only to be
> +  used to specify which nodes to include (using -i <node_name>) and which
> +  nodes/properties to exclude (using -x <regex>). If not set at all, the default
> +  one is used applicable for ZynqMP MPSoC boards.

You are using some more arguments (besides -x and -i) :-

--permissive -f
-- extract -t
-- extract-xen -t $node -o

It will be good to have some explaination for these. See my comments below.

> +
>   - NUM_DOMUS specifies how many Dom0-less DomUs to load
>   
>   - DOMU_KERNEL[number] specifies the DomU kernel to use.
> @@ -140,7 +153,7 @@ Where:
>   - DOMU_PASSTHROUGH_PATHS[number] specifies the passthrough devices (
>     separated by spaces). It adds "xen,passthrough" to the corresponding
>     dtb nodes in xen device tree blob.
> -  This option is valid in the following two cases:
> +  This option is valid in the following cases:
>   
>     1. When PASSTHROUGH_DTS_REPO is provided.
>     With this option, the partial device trees (corresponding to the
> @@ -149,7 +162,12 @@ Where:
>     Note it assumes that the names of the partial device trees will match
>     to the names of the devices specified here.
>   
> -  2. When DOMU_NOBOOT[number] is provided. In this case, it will only
> +  2. When LOPPER_PATH is provided.
> +  With this option, the partial device trees (corresponding to the
> +  passthrough devices) are generated by the lopper and then compiled and merged
> +  by ImageBuilder to be used as DOMU[number] device tree blob.
> +
> +  3. When DOMU_NOBOOT[number] is provided. In this case, it will only
>     add "xen,passthrough" as mentioned before.
>   
>   - DOMU_PASSTHROUGH_DTB[number] specifies the passthrough device trees
> diff --git a/scripts/common b/scripts/common
> index ccad03d82b30..680c5090cd07 100644
> --- a/scripts/common
> +++ b/scripts/common
> @@ -9,6 +9,9 @@
>   # - NUM_DOMUS
>   # - DOMU_PASSTHROUGH_PATHS
>   # - DOMU_PASSTHROUGH_DTB
> +# - LOPPER_PATH
> +# - LOPPER_CMD
> +# - DEVICE_TREE
>   
>   tmp_files=()
>   tmp_dirs=()
> @@ -99,31 +102,41 @@ function compile_merge_partial_dts()
>       local tmp
>       local tmpdts
>       local file
> +    local node
>       local i
>       local j
>   
> -    if [[ "$repo" =~ .*@.*:.* ]]
> +    if test "$repo"
>       then
> -        tmp=`mktemp -d`
> -        tmp_dirs+=($tmp)
> -
> -        echo "Cloning git repo \"$git_repo\""
> -        git clone "$repo" $tmp
> -        if test $? -ne 0
> +        # Partial dts will be obtained from PASSTHROUGH_DTS_REPO
> +        if [[ "$repo" =~ .*@.*:.* ]]
>           then
> -            echo "Error occurred while cloning \"$git_repo\""
> -            return 1
> -        fi
> +            tmp=`mktemp -d`
> +            tmp_dirs+=($tmp)
>   
> -        repo=$tmp
> -    fi
> +            echo "Cloning git repo \"$git_repo\""
> +            git clone "$repo" $tmp
> +            if test $? -ne 0
> +            then
> +                echo "Error occurred while cloning \"$git_repo\""
> +                return 1
> +            fi
>   
> -    if test -z "$dir"
> -    then
> -        dir="."
> +            repo=$tmp
> +        fi
> +
> +        if test -z "$dir"
> +        then
> +            dir="."
> +        fi
> +        partial_dts_dir="$repo"/"$dir"
> +    else
> +        # Partial dts will be generated by the lopper
> +        tmp=`mktemp -d`
> +        tmp_dirs+=($tmp)
> +        partial_dts_dir="$tmp"
>       fi
>   
> -    partial_dts_dir="$repo"/"$dir"
>       i=0
>       while test $i -lt $NUM_DOMUS
>       do
> @@ -133,6 +146,25 @@ function compile_merge_partial_dts()
>               return 1
>           fi
>   
> +        if test -z "$repo"
> +        then
> +            # Generate partial dts using lopper
> +            for devpath in ${DOMU_PASSTHROUGH_PATHS[$i]}
> +            do
> +                node=${devpath##*/}
> +                file="$partial_dts_dir"/"$node".dts
> +
> +                $LOPPER_PATH --permissive -f $DEVICE_TREE \
> +                -- extract -t $devpath $LOPPER_CMD \
> +                -- extract-xen -t $node -o $file
See below comment. Applies here as well.
> +
> +                if test $? -ne 0
> +                then
> +                    return 1
> +                fi
> +            done
> +        fi
> +
>           sanity_check_partial_dts "${DOMU_PASSTHROUGH_PATHS[$i]}" "$partial_dts_dir"
>           if test $? -ne 0
>           then
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index 1f8ab5ffd193..84a68d6bd0b0 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -1138,10 +1138,23 @@ fi
>   # tftp or move the files to a partition
>   cd "$uboot_dir"
>   
> -if test "$PASSTHROUGH_DTS_REPO"
> +# If both PASSTHROUGH_DTS_REPO and LOPPER_PATH options are specified,
> +# the former takes precedence because the partial device trees are already
> +# created (probably tested), hence the reliability is higher than using lopper.
> +if test "$PASSTHROUGH_DTS_REPO" || test "$LOPPER_PATH"
>   then
>       output_dir=`mktemp -d "partial-dtbs-XXX"`
> -    compile_merge_partial_dts $output_dir "$PASSTHROUGH_DTS_REPO"
> +    if test "$PASSTHROUGH_DTS_REPO"
> +    then
> +        compile_merge_partial_dts $output_dir "$PASSTHROUGH_DTS_REPO"
> +    else
> +        if test -z "$LOPPER_CMD"
> +        then
> +            # Default for ZynqMP MPSoC
> +            LOPPER_CMD="-i zynqmp-firmware -x interrupt-controller -x pinctrl -x power-domains -x resets -x current-speed"

It will be very useful, if you could provide the link to Lopper's README 
which explains the arguments used here, as a comment.

Even better if you can provide some explaination (as a comment) to what 
the command intends to do here.

- Ayan

> +        fi
> +        compile_merge_partial_dts $output_dir
> +    fi
>       if test $? -ne 0
>       then
>           # Remove the output dir holding the partial dtbs in case of any error

Re: [ImageBuilder 2/2] Add support for lopper to generate partial dts

[Michal Orzel]

Hi Ayan,

On 12/09/2022 18:41, Ayan Kumar Halder wrote:
> Hi Michal,
> 
> On 12/09/2022 12:59, Michal Orzel wrote:
>> Currently ImageBuilder can compile and merge partial dts obtained from
>> a repository specified using PASSTHROUGH_DTS_REPO. With the recent
>> changes done in the lopper, we can use it to generate partial dts
>> automatically (to some extent as this is still an early support).
>>
>> Introduce LOPPER_PATH option to specify a path to a lopper.py script,
>> that if set, will invoke lopper to generate partial dts for the
>> passthrough devices specified in DOMU_PASSTHROUGH_PATHS.
>>
>> Introduce LOPPER_CMD option to specify custom command line arguments
>> (if needed) for lopper's extract assist.
>>
>> Example usage:
>> LOPPER_PATH="/home/user/lopper/lopper.py"
>> DOMU_PASSTHROUGH_PATHS[0]="/axi/spi@ff0f0000 /axi/serial@ff010000"
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>>   README.md                | 22 ++++++++++++--
>>   scripts/common           | 64 ++++++++++++++++++++++++++++++----------
>>   scripts/uboot-script-gen | 17 +++++++++--
>>   3 files changed, 83 insertions(+), 20 deletions(-)
>>
>> diff --git a/README.md b/README.md
>> index da9ba788a3bf..aaee0939b589 100644
>> --- a/README.md
>> +++ b/README.md
>> @@ -128,6 +128,19 @@ Where:
>>   - DT_OVERLAY[number] specifies the path to the hosts device tree overlays
>>     to be added at boot time in u-boot
>>   
>> +- LOPPER_PATH specifies the path to lopper.py script. This is optional.
>> +  However, if this is specified, then DOMU_PASSTHROUGH_PATHS[number] need
>> +  to be specified. uboot-script-gen will invoke lopper to generate the partial
>> +  device trees which have been specified in DOMU_PASSTHROUGH_PATHS[number].
>> +  This option is currently in experimental state as the corresponding lopper
>> +  changes are still in an early support state.
>> +
>> +- LOPPER_CMD specifies the command line arguments for lopper's extract assist.
>> +  This is optional and only applicable when LOPPER_PATH is specified. Only to be
>> +  used to specify which nodes to include (using -i <node_name>) and which
>> +  nodes/properties to exclude (using -x <regex>). If not set at all, the default
>> +  one is used applicable for ZynqMP MPSoC boards.
> 
> You are using some more arguments (besides -x and -i) :-
> 
> --permissive -f
> -- extract -t
> -- extract-xen -t $node -o
These ones are fixed and do not differ depending on the type of device or board.
That is why LOPPER_CMD is used only to allow users to specify what can be required
to support a new device (usually not necessary) or a new board.

> 
> It will be good to have some explaination for these. See my comments below.
> 
We don't seem to do it in general (see all the commands used by disk_image) so I think
we should only describe what is available to the user. Otherwise we would need to be
consistent and apply this rule to all the other places.

>> +
>>   - NUM_DOMUS specifies how many Dom0-less DomUs to load
>>   
>>   - DOMU_KERNEL[number] specifies the DomU kernel to use.
>> @@ -140,7 +153,7 @@ Where:
>>   - DOMU_PASSTHROUGH_PATHS[number] specifies the passthrough devices (
>>     separated by spaces). It adds "xen,passthrough" to the corresponding
>>     dtb nodes in xen device tree blob.
>> -  This option is valid in the following two cases:
>> +  This option is valid in the following cases:
>>   
>>     1. When PASSTHROUGH_DTS_REPO is provided.
>>     With this option, the partial device trees (corresponding to the
>> @@ -149,7 +162,12 @@ Where:
>>     Note it assumes that the names of the partial device trees will match
>>     to the names of the devices specified here.
>>   
>> -  2. When DOMU_NOBOOT[number] is provided. In this case, it will only
>> +  2. When LOPPER_PATH is provided.
>> +  With this option, the partial device trees (corresponding to the
>> +  passthrough devices) are generated by the lopper and then compiled and merged
>> +  by ImageBuilder to be used as DOMU[number] device tree blob.
>> +
>> +  3. When DOMU_NOBOOT[number] is provided. In this case, it will only
>>     add "xen,passthrough" as mentioned before.
>>   
>>   - DOMU_PASSTHROUGH_DTB[number] specifies the passthrough device trees
>> diff --git a/scripts/common b/scripts/common
>> index ccad03d82b30..680c5090cd07 100644
>> --- a/scripts/common
>> +++ b/scripts/common
>> @@ -9,6 +9,9 @@
>>   # - NUM_DOMUS
>>   # - DOMU_PASSTHROUGH_PATHS
>>   # - DOMU_PASSTHROUGH_DTB
>> +# - LOPPER_PATH
>> +# - LOPPER_CMD
>> +# - DEVICE_TREE
>>   
>>   tmp_files=()
>>   tmp_dirs=()
>> @@ -99,31 +102,41 @@ function compile_merge_partial_dts()
>>       local tmp
>>       local tmpdts
>>       local file
>> +    local node
>>       local i
>>       local j
>>   
>> -    if [[ "$repo" =~ .*@.*:.* ]]
>> +    if test "$repo"
>>       then
>> -        tmp=`mktemp -d`
>> -        tmp_dirs+=($tmp)
>> -
>> -        echo "Cloning git repo \"$git_repo\""
>> -        git clone "$repo" $tmp
>> -        if test $? -ne 0
>> +        # Partial dts will be obtained from PASSTHROUGH_DTS_REPO
>> +        if [[ "$repo" =~ .*@.*:.* ]]
>>           then
>> -            echo "Error occurred while cloning \"$git_repo\""
>> -            return 1
>> -        fi
>> +            tmp=`mktemp -d`
>> +            tmp_dirs+=($tmp)
>>   
>> -        repo=$tmp
>> -    fi
>> +            echo "Cloning git repo \"$git_repo\""
>> +            git clone "$repo" $tmp
>> +            if test $? -ne 0
>> +            then
>> +                echo "Error occurred while cloning \"$git_repo\""
>> +                return 1
>> +            fi
>>   
>> -    if test -z "$dir"
>> -    then
>> -        dir="."
>> +            repo=$tmp
>> +        fi
>> +
>> +        if test -z "$dir"
>> +        then
>> +            dir="."
>> +        fi
>> +        partial_dts_dir="$repo"/"$dir"
>> +    else
>> +        # Partial dts will be generated by the lopper
>> +        tmp=`mktemp -d`
>> +        tmp_dirs+=($tmp)
>> +        partial_dts_dir="$tmp"
>>       fi
>>   
>> -    partial_dts_dir="$repo"/"$dir"
>>       i=0
>>       while test $i -lt $NUM_DOMUS
>>       do
>> @@ -133,6 +146,25 @@ function compile_merge_partial_dts()
>>               return 1
>>           fi
>>   
>> +        if test -z "$repo"
>> +        then
>> +            # Generate partial dts using lopper
>> +            for devpath in ${DOMU_PASSTHROUGH_PATHS[$i]}
>> +            do
>> +                node=${devpath##*/}
>> +                file="$partial_dts_dir"/"$node".dts
>> +
>> +                $LOPPER_PATH --permissive -f $DEVICE_TREE \
>> +                -- extract -t $devpath $LOPPER_CMD \
>> +                -- extract-xen -t $node -o $file
> See below comment. Applies here as well.
>> +
>> +                if test $? -ne 0
>> +                then
>> +                    return 1
>> +                fi
>> +            done
>> +        fi
>> +
>>           sanity_check_partial_dts "${DOMU_PASSTHROUGH_PATHS[$i]}" "$partial_dts_dir"
>>           if test $? -ne 0
>>           then
>> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
>> index 1f8ab5ffd193..84a68d6bd0b0 100755
>> --- a/scripts/uboot-script-gen
>> +++ b/scripts/uboot-script-gen
>> @@ -1138,10 +1138,23 @@ fi
>>   # tftp or move the files to a partition
>>   cd "$uboot_dir"
>>   
>> -if test "$PASSTHROUGH_DTS_REPO"
>> +# If both PASSTHROUGH_DTS_REPO and LOPPER_PATH options are specified,
>> +# the former takes precedence because the partial device trees are already
>> +# created (probably tested), hence the reliability is higher than using lopper.
>> +if test "$PASSTHROUGH_DTS_REPO" || test "$LOPPER_PATH"
>>   then
>>       output_dir=`mktemp -d "partial-dtbs-XXX"`
>> -    compile_merge_partial_dts $output_dir "$PASSTHROUGH_DTS_REPO"
>> +    if test "$PASSTHROUGH_DTS_REPO"
>> +    then
>> +        compile_merge_partial_dts $output_dir "$PASSTHROUGH_DTS_REPO"
>> +    else
>> +        if test -z "$LOPPER_CMD"
>> +        then
>> +            # Default for ZynqMP MPSoC
>> +            LOPPER_CMD="-i zynqmp-firmware -x interrupt-controller -x pinctrl -x power-domains -x resets -x current-speed"
> 
> It will be very useful, if you could provide the link to Lopper's README 
> which explains the arguments used here, as a comment.
> 
This lopper feature is still in an early state, hence there is no such information
in the README. I described everything a user can change (like -i and -x option) using the information
from the extract's help. 

> Even better if you can provide some explaination (as a comment) to what 
> the command intends to do here
> 
> - Ayan
> 
>> +        fi
>> +        compile_merge_partial_dts $output_dir
>> +    fi
>>       if test $? -ne 0
>>       then
>>           # Remove the output dir holding the partial dtbs in case of any error

~Michal

Re: [ImageBuilder 1/2] Refactor sanity_check_partial_dts

[Stefano Stabellini]

On Mon, 12 Sep 2022, Ayan Kumar Halder wrote:
> On 12/09/2022 12:59, Michal Orzel wrote:
> > Currently function sanity_check_partial_dts from scripts/common takes
> > three arguments where the last two (repo, dir) are used always in
> > conjuction to form a path to a directory storing partial dts. Modify the
> > function to take only two arguments where the second one is to be a path
> > to a directory storing partial dts. This will help reusing this function
> > in the future to perform sanity checks on partial dts that do not
> > originate from a repository.
> > 
> > Modify compile_merge_partial_dts to take this change into account.
> > 
> > Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> Reviewed-by: Ayan Kumar Halder <ayankuma@amd.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> > ---
> >   scripts/common | 11 ++++++-----
> >   1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/scripts/common b/scripts/common
> > index 25c041270c29..ccad03d82b30 100644
> > --- a/scripts/common
> > +++ b/scripts/common
> > @@ -40,8 +40,7 @@ function get_next_phandle()
> >   function sanity_check_partial_dts()
> >   {
> >       local domU_passthrough_path="$1"
> > -    local repo="$2"
> > -    local dir="$3"
> > +    local partial_dts_dir="$2"
> >       local address_cells_val
> >       local size_cells_val
> >       local tmpdtb
> > @@ -51,7 +50,7 @@ function sanity_check_partial_dts()
> >       for devpath in $domU_passthrough_path
> >       do
> >           file=${devpath##*/}
> > -        file="$repo"/"$dir"/"$file".dts
> > +        file="$partial_dts_dir"/"$file".dts
> >             if ! test -f "$file"
> >           then
> > @@ -96,6 +95,7 @@ function compile_merge_partial_dts()
> >       local dtb_dir=$1
> >       local repo=$(echo "$2" | awk '{print $1}')
> >       local dir=$(echo "$2" | awk '{print $2}')
> > +    local partial_dts_dir
> >       local tmp
> >       local tmpdts
> >       local file
> > @@ -123,6 +123,7 @@ function compile_merge_partial_dts()
> >           dir="."
> >       fi
> >   +    partial_dts_dir="$repo"/"$dir"
> >       i=0
> >       while test $i -lt $NUM_DOMUS
> >       do
> > @@ -132,7 +133,7 @@ function compile_merge_partial_dts()
> >               return 1
> >           fi
> >   -        sanity_check_partial_dts "${DOMU_PASSTHROUGH_PATHS[$i]}" "$repo"
> > "$dir"
> > +        sanity_check_partial_dts "${DOMU_PASSTHROUGH_PATHS[$i]}"
> > "$partial_dts_dir"
> >           if test $? -ne 0
> >           then
> >               return 1
> > @@ -146,7 +147,7 @@ function compile_merge_partial_dts()
> >           for devpath in ${DOMU_PASSTHROUGH_PATHS[$i]}
> >           do
> >               file=${devpath##*/}
> > -            file="$repo"/"$dir"/"$file".dts
> > +            file="$partial_dts_dir"/"$file".dts
> >                 # All the subsequent dts files should not have dts version
> > mentioned
> >               if test $j -gt 1
> 

Re: [ImageBuilder 2/2] Add support for lopper to generate partial dts

[Stefano Stabellini]

On Mon, 12 Sep 2022, Michal Orzel wrote:
> Currently ImageBuilder can compile and merge partial dts obtained from
> a repository specified using PASSTHROUGH_DTS_REPO. With the recent
> changes done in the lopper, we can use it to generate partial dts
> automatically (to some extent as this is still an early support).
> 
> Introduce LOPPER_PATH option to specify a path to a lopper.py script,
> that if set, will invoke lopper to generate partial dts for the
> passthrough devices specified in DOMU_PASSTHROUGH_PATHS.
> 
> Introduce LOPPER_CMD option to specify custom command line arguments
> (if needed) for lopper's extract assist.
> 
> Example usage:
> LOPPER_PATH="/home/user/lopper/lopper.py"
> DOMU_PASSTHROUGH_PATHS[0]="/axi/spi@ff0f0000 /axi/serial@ff010000"

Is lopper.py this file?

https://github.com/devicetree-org/lopper/blob/master/lopper.py

If so, it would be good to specify in the README that this is not just
an arbitrary lopper.py script, but the main entry point of Lopper as a
project. For instance:

---
Introduce LOPPER_PATH option to specify a path to a lopper.py script,
the main script in the Lopper repository
(https://github.com/devicetree-org/lopper). If set, ....
---


> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
>  README.md                | 22 ++++++++++++--
>  scripts/common           | 64 ++++++++++++++++++++++++++++++----------
>  scripts/uboot-script-gen | 17 +++++++++--
>  3 files changed, 83 insertions(+), 20 deletions(-)
> 
> diff --git a/README.md b/README.md
> index da9ba788a3bf..aaee0939b589 100644
> --- a/README.md
> +++ b/README.md
> @@ -128,6 +128,19 @@ Where:
>  - DT_OVERLAY[number] specifies the path to the hosts device tree overlays
>    to be added at boot time in u-boot
>  
> +- LOPPER_PATH specifies the path to lopper.py script. This is optional.
> +  However, if this is specified, then DOMU_PASSTHROUGH_PATHS[number] need
> +  to be specified. uboot-script-gen will invoke lopper to generate the partial
> +  device trees which have been specified in DOMU_PASSTHROUGH_PATHS[number].
> +  This option is currently in experimental state as the corresponding lopper
> +  changes are still in an early support state.
> +
> +- LOPPER_CMD specifies the command line arguments for lopper's extract assist.
> +  This is optional and only applicable when LOPPER_PATH is specified. Only to be
> +  used to specify which nodes to include (using -i <node_name>) and which
> +  nodes/properties to exclude (using -x <regex>). If not set at all, the default
> +  one is used applicable for ZynqMP MPSoC boards.
> +
>  - NUM_DOMUS specifies how many Dom0-less DomUs to load
>  
>  - DOMU_KERNEL[number] specifies the DomU kernel to use.
> @@ -140,7 +153,7 @@ Where:
>  - DOMU_PASSTHROUGH_PATHS[number] specifies the passthrough devices (
>    separated by spaces). It adds "xen,passthrough" to the corresponding
>    dtb nodes in xen device tree blob.
> -  This option is valid in the following two cases:
> +  This option is valid in the following cases:
>  
>    1. When PASSTHROUGH_DTS_REPO is provided.
>    With this option, the partial device trees (corresponding to the
> @@ -149,7 +162,12 @@ Where:
>    Note it assumes that the names of the partial device trees will match
>    to the names of the devices specified here.
>  
> -  2. When DOMU_NOBOOT[number] is provided. In this case, it will only
> +  2. When LOPPER_PATH is provided.
> +  With this option, the partial device trees (corresponding to the
> +  passthrough devices) are generated by the lopper and then compiled and merged
> +  by ImageBuilder to be used as DOMU[number] device tree blob.
> +
> +  3. When DOMU_NOBOOT[number] is provided. In this case, it will only
>    add "xen,passthrough" as mentioned before.
>  
>  - DOMU_PASSTHROUGH_DTB[number] specifies the passthrough device trees
> diff --git a/scripts/common b/scripts/common
> index ccad03d82b30..680c5090cd07 100644
> --- a/scripts/common
> +++ b/scripts/common
> @@ -9,6 +9,9 @@
>  # - NUM_DOMUS
>  # - DOMU_PASSTHROUGH_PATHS
>  # - DOMU_PASSTHROUGH_DTB
> +# - LOPPER_PATH
> +# - LOPPER_CMD
> +# - DEVICE_TREE
>  
>  tmp_files=()
>  tmp_dirs=()
> @@ -99,31 +102,41 @@ function compile_merge_partial_dts()
>      local tmp
>      local tmpdts
>      local file
> +    local node
>      local i
>      local j
>  
> -    if [[ "$repo" =~ .*@.*:.* ]]
> +    if test "$repo"
>      then
> -        tmp=`mktemp -d`
> -        tmp_dirs+=($tmp)
> -
> -        echo "Cloning git repo \"$git_repo\""
> -        git clone "$repo" $tmp
> -        if test $? -ne 0
> +        # Partial dts will be obtained from PASSTHROUGH_DTS_REPO
> +        if [[ "$repo" =~ .*@.*:.* ]]
>          then
> -            echo "Error occurred while cloning \"$git_repo\""
> -            return 1
> -        fi
> +            tmp=`mktemp -d`
> +            tmp_dirs+=($tmp)
>  
> -        repo=$tmp
> -    fi
> +            echo "Cloning git repo \"$git_repo\""
> +            git clone "$repo" $tmp
> +            if test $? -ne 0
> +            then
> +                echo "Error occurred while cloning \"$git_repo\""
> +                return 1
> +            fi
>  
> -    if test -z "$dir"
> -    then
> -        dir="."
> +            repo=$tmp
> +        fi
> +
> +        if test -z "$dir"
> +        then
> +            dir="."
> +        fi
> +        partial_dts_dir="$repo"/"$dir"
> +    else
> +        # Partial dts will be generated by the lopper
> +        tmp=`mktemp -d`
> +        tmp_dirs+=($tmp)

setting tmp and tmp_dirs can be moved outside of the if


> +        partial_dts_dir="$tmp"
>      fi
>  
> -    partial_dts_dir="$repo"/"$dir"
>      i=0
>      while test $i -lt $NUM_DOMUS
>      do
> @@ -133,6 +146,25 @@ function compile_merge_partial_dts()
>              return 1
>          fi
>  
> +        if test -z "$repo"
> +        then
> +            # Generate partial dts using lopper
> +            for devpath in ${DOMU_PASSTHROUGH_PATHS[$i]}
> +            do
> +                node=${devpath##*/}
> +                file="$partial_dts_dir"/"$node".dts

This is a minor NIT. As we do below: 

            file=${devpath##*/}
            file="$partial_dts_dir"/"$file".dts

Can you change the code below to use node and file as you do here for
consistency?


> +                $LOPPER_PATH --permissive -f $DEVICE_TREE \
> +                -- extract -t $devpath $LOPPER_CMD \
> +                -- extract-xen -t $node -o $file
> +
> +                if test $? -ne 0
> +                then
> +                    return 1
> +                fi
> +            done
> +        fi
> +
>          sanity_check_partial_dts "${DOMU_PASSTHROUGH_PATHS[$i]}" "$partial_dts_dir"
>          if test $? -ne 0
>          then
> diff --git a/scripts/uboot-script-gen b/scripts/uboot-script-gen
> index 1f8ab5ffd193..84a68d6bd0b0 100755
> --- a/scripts/uboot-script-gen
> +++ b/scripts/uboot-script-gen
> @@ -1138,10 +1138,23 @@ fi
>  # tftp or move the files to a partition
>  cd "$uboot_dir"
>  
> -if test "$PASSTHROUGH_DTS_REPO"
> +# If both PASSTHROUGH_DTS_REPO and LOPPER_PATH options are specified,
> +# the former takes precedence because the partial device trees are already
> +# created (probably tested), hence the reliability is higher than using lopper.
> +if test "$PASSTHROUGH_DTS_REPO" || test "$LOPPER_PATH"
>  then
>      output_dir=`mktemp -d "partial-dtbs-XXX"`
> -    compile_merge_partial_dts $output_dir "$PASSTHROUGH_DTS_REPO"
> +    if test "$PASSTHROUGH_DTS_REPO"
> +    then
> +        compile_merge_partial_dts $output_dir "$PASSTHROUGH_DTS_REPO"
> +    else
> +        if test -z "$LOPPER_CMD"
> +        then
> +            # Default for ZynqMP MPSoC
> +            LOPPER_CMD="-i zynqmp-firmware -x interrupt-controller -x pinctrl -x power-domains -x resets -x current-speed"
> +        fi
> +        compile_merge_partial_dts $output_dir
> +    fi
>      if test $? -ne 0
>      then
>          # Remove the output dir holding the partial dtbs in case of any error
> -- 
> 2.25.1
> 

Re: [ImageBuilder 2/2] Add support for lopper to generate partial dts

[Michal Orzel]

Hi Stefano,

On 13/09/2022 03:13, Stefano Stabellini wrote:
> 
> On Mon, 12 Sep 2022, Michal Orzel wrote:
>> Currently ImageBuilder can compile and merge partial dts obtained from
>> a repository specified using PASSTHROUGH_DTS_REPO. With the recent
>> changes done in the lopper, we can use it to generate partial dts
>> automatically (to some extent as this is still an early support).
>>
>> Introduce LOPPER_PATH option to specify a path to a lopper.py script,
>> that if set, will invoke lopper to generate partial dts for the
>> passthrough devices specified in DOMU_PASSTHROUGH_PATHS.
>>
>> Introduce LOPPER_CMD option to specify custom command line arguments
>> (if needed) for lopper's extract assist.
>>
>> Example usage:
>> LOPPER_PATH="/home/user/lopper/lopper.py"
>> DOMU_PASSTHROUGH_PATHS[0]="/axi/spi@ff0f0000 /axi/serial@ff010000"
> 
> Is lopper.py this file?
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdevicetree-org%2Flopper%2Fblob%2Fmaster%2Flopper.py&data=05%7C01%7Cmichal.orzel%40amd.com%7Cb756682b3b0a460f5c3608da95252c7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637986284138713501%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=KShbbVjvB1vG26vUQL6py7edWylpoZ63n5BW11dxbmo%3D&reserved=0
> 
> If so, it would be good to specify in the README that this is not just
> an arbitrary lopper.py script, but the main entry point of Lopper as a
> project. For instance:
> 
> ---
> Introduce LOPPER_PATH option to specify a path to a lopper.py script,
> the main script in the Lopper repository
> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdevicetree-org%2Flopper&data=05%7C01%7Cmichal.orzel%40amd.com%7Cb756682b3b0a460f5c3608da95252c7c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637986284138713501%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vEh2VZz84MQiZJnKyGzGejJ7QKO%2FYENwg1v4XdF8PRk%3D&reserved=0). If set, ....
> ---
> 
Sounds good. I will add this explanation.

> 
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>> ---
>>  README.md                | 22 ++++++++++++--
>>  scripts/common           | 64 ++++++++++++++++++++++++++++++----------
>>  scripts/uboot-script-gen | 17 +++++++++--
>>  3 files changed, 83 insertions(+), 20 deletions(-)
>>
>> diff --git a/README.md b/README.md
>> index da9ba788a3bf..aaee0939b589 100644
>> --- a/README.md
>> +++ b/README.md
>> @@ -128,6 +128,19 @@ Where:
>>  - DT_OVERLAY[number] specifies the path to the hosts device tree overlays
>>    to be added at boot time in u-boot
>>
>> +- LOPPER_PATH specifies the path to lopper.py script. This is optional.
>> +  However, if this is specified, then DOMU_PASSTHROUGH_PATHS[number] need
>> +  to be specified. uboot-script-gen will invoke lopper to generate the partial
>> +  device trees which have been specified in DOMU_PASSTHROUGH_PATHS[number].
>> +  This option is currently in experimental state as the corresponding lopper
>> +  changes are still in an early support state.
>> +
>> +- LOPPER_CMD specifies the command line arguments for lopper's extract assist.
>> +  This is optional and only applicable when LOPPER_PATH is specified. Only to be
>> +  used to specify which nodes to include (using -i <node_name>) and which
>> +  nodes/properties to exclude (using -x <regex>). If not set at all, the default
>> +  one is used applicable for ZynqMP MPSoC boards.
>> +
>>  - NUM_DOMUS specifies how many Dom0-less DomUs to load
>>
>>  - DOMU_KERNEL[number] specifies the DomU kernel to use.
>> @@ -140,7 +153,7 @@ Where:
>>  - DOMU_PASSTHROUGH_PATHS[number] specifies the passthrough devices (
>>    separated by spaces). It adds "xen,passthrough" to the corresponding
>>    dtb nodes in xen device tree blob.
>> -  This option is valid in the following two cases:
>> +  This option is valid in the following cases:
>>
>>    1. When PASSTHROUGH_DTS_REPO is provided.
>>    With this option, the partial device trees (corresponding to the
>> @@ -149,7 +162,12 @@ Where:
>>    Note it assumes that the names of the partial device trees will match
>>    to the names of the devices specified here.
>>
>> -  2. When DOMU_NOBOOT[number] is provided. In this case, it will only
>> +  2. When LOPPER_PATH is provided.
>> +  With this option, the partial device trees (corresponding to the
>> +  passthrough devices) are generated by the lopper and then compiled and merged
>> +  by ImageBuilder to be used as DOMU[number] device tree blob.
>> +
>> +  3. When DOMU_NOBOOT[number] is provided. In this case, it will only
>>    add "xen,passthrough" as mentioned before.
>>
>>  - DOMU_PASSTHROUGH_DTB[number] specifies the passthrough device trees
>> diff --git a/scripts/common b/scripts/common
>> index ccad03d82b30..680c5090cd07 100644
>> --- a/scripts/common
>> +++ b/scripts/common
>> @@ -9,6 +9,9 @@
>>  # - NUM_DOMUS
>>  # - DOMU_PASSTHROUGH_PATHS
>>  # - DOMU_PASSTHROUGH_DTB
>> +# - LOPPER_PATH
>> +# - LOPPER_CMD
>> +# - DEVICE_TREE
>>
>>  tmp_files=()
>>  tmp_dirs=()
>> @@ -99,31 +102,41 @@ function compile_merge_partial_dts()
>>      local tmp
>>      local tmpdts
>>      local file
>> +    local node
>>      local i
>>      local j
>>
>> -    if [[ "$repo" =~ .*@.*:.* ]]
>> +    if test "$repo"
>>      then
>> -        tmp=`mktemp -d`
>> -        tmp_dirs+=($tmp)
>> -
>> -        echo "Cloning git repo \"$git_repo\""
>> -        git clone "$repo" $tmp
>> -        if test $? -ne 0
>> +        # Partial dts will be obtained from PASSTHROUGH_DTS_REPO
>> +        if [[ "$repo" =~ .*@.*:.* ]]
>>          then
>> -            echo "Error occurred while cloning \"$git_repo\""
>> -            return 1
>> -        fi
>> +            tmp=`mktemp -d`
>> +            tmp_dirs+=($tmp)
>>
>> -        repo=$tmp
>> -    fi
>> +            echo "Cloning git repo \"$git_repo\""
>> +            git clone "$repo" $tmp
>> +            if test $? -ne 0
>> +            then
>> +                echo "Error occurred while cloning \"$git_repo\""
>> +                return 1
>> +            fi
>>
>> -    if test -z "$dir"
>> -    then
>> -        dir="."
>> +            repo=$tmp
>> +        fi
>> +
>> +        if test -z "$dir"
>> +        then
>> +            dir="."
>> +        fi
>> +        partial_dts_dir="$repo"/"$dir"
>> +    else
>> +        # Partial dts will be generated by the lopper
>> +        tmp=`mktemp -d`
>> +        tmp_dirs+=($tmp)
> 
> setting tmp and tmp_dirs can be moved outside of the if
> 
Ok.

> 
>> +        partial_dts_dir="$tmp"
>>      fi
>>
>> -    partial_dts_dir="$repo"/"$dir"
>>      i=0
>>      while test $i -lt $NUM_DOMUS
>>      do
>> @@ -133,6 +146,25 @@ function compile_merge_partial_dts()
>>              return 1
>>          fi
>>
>> +        if test -z "$repo"
>> +        then
>> +            # Generate partial dts using lopper
>> +            for devpath in ${DOMU_PASSTHROUGH_PATHS[$i]}
>> +            do
>> +                node=${devpath##*/}
>> +                file="$partial_dts_dir"/"$node".dts
> 
> This is a minor NIT. As we do below:
> 
>             file=${devpath##*/}
>             file="$partial_dts_dir"/"$file".dts
> 
> Can you change the code below to use node and file as you do here for
> consistency?
> 
Sure, I will modify them.

~Michal

Re: [ImageBuilder 2/2] Add support for lopper to generate partial dts

[Michal Orzel]

On 13/09/2022 09:54, Michal Orzel wrote:
> 
> Hi Stefano,
> 
> On 13/09/2022 03:13, Stefano Stabellini wrote:
>>
>> On Mon, 12 Sep 2022, Michal Orzel wrote:
>>> Currently ImageBuilder can compile and merge partial dts obtained from
>>> a repository specified using PASSTHROUGH_DTS_REPO. With the recent
>>> changes done in the lopper, we can use it to generate partial dts
>>> automatically (to some extent as this is still an early support).
>>>
>>> Introduce LOPPER_PATH option to specify a path to a lopper.py script,
>>> that if set, will invoke lopper to generate partial dts for the
>>> passthrough devices specified in DOMU_PASSTHROUGH_PATHS.
>>>
>>> Introduce LOPPER_CMD option to specify custom command line arguments
>>> (if needed) for lopper's extract assist.
>>>
>>> Example usage:
>>> LOPPER_PATH="/home/user/lopper/lopper.py"
>>> DOMU_PASSTHROUGH_PATHS[0]="/axi/spi@ff0f0000 /axi/serial@ff010000"
>>
>> Is lopper.py this file?
>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdevicetree-org%2Flopper%2Fblob%2Fmaster%2Flopper.py&data=05%7C01%7Cmichal.orzel%40amd.com%7Ca7d4f0cc1c07424da8ba08da955d2fcc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637986524721374780%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=oz5an1eISNbs6LoX3lE90RR%2FnYTX7ikZXw%2Fl57HlHV8%3D&reserved=0
>>
>> If so, it would be good to specify in the README that this is not just
>> an arbitrary lopper.py script, but the main entry point of Lopper as a
>> project. For instance:
>>
>> ---
>> Introduce LOPPER_PATH option to specify a path to a lopper.py script,
>> the main script in the Lopper repository
>> (https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdevicetree-org%2Flopper&data=05%7C01%7Cmichal.orzel%40amd.com%7Ca7d4f0cc1c07424da8ba08da955d2fcc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637986524721374780%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=6ScZeGSMsX4MGgbOEi6I%2FDvkGNPlbVvBeSKQwexTHGA%3D&reserved=0). If set, ....
>> ---
>>
> Sounds good. I will add this explanation.
> 
>>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>> ---
>>>  README.md                | 22 ++++++++++++--
>>>  scripts/common           | 64 ++++++++++++++++++++++++++++++----------
>>>  scripts/uboot-script-gen | 17 +++++++++--
>>>  3 files changed, 83 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/README.md b/README.md
>>> index da9ba788a3bf..aaee0939b589 100644
>>> --- a/README.md
>>> +++ b/README.md
>>> @@ -128,6 +128,19 @@ Where:
>>>  - DT_OVERLAY[number] specifies the path to the hosts device tree overlays
>>>    to be added at boot time in u-boot
>>>
>>> +- LOPPER_PATH specifies the path to lopper.py script. This is optional.
>>> +  However, if this is specified, then DOMU_PASSTHROUGH_PATHS[number] need
>>> +  to be specified. uboot-script-gen will invoke lopper to generate the partial
>>> +  device trees which have been specified in DOMU_PASSTHROUGH_PATHS[number].
>>> +  This option is currently in experimental state as the corresponding lopper
>>> +  changes are still in an early support state.
>>> +
>>> +- LOPPER_CMD specifies the command line arguments for lopper's extract assist.
>>> +  This is optional and only applicable when LOPPER_PATH is specified. Only to be
>>> +  used to specify which nodes to include (using -i <node_name>) and which
>>> +  nodes/properties to exclude (using -x <regex>). If not set at all, the default
>>> +  one is used applicable for ZynqMP MPSoC boards.
>>> +
>>>  - NUM_DOMUS specifies how many Dom0-less DomUs to load
>>>
>>>  - DOMU_KERNEL[number] specifies the DomU kernel to use.
>>> @@ -140,7 +153,7 @@ Where:
>>>  - DOMU_PASSTHROUGH_PATHS[number] specifies the passthrough devices (
>>>    separated by spaces). It adds "xen,passthrough" to the corresponding
>>>    dtb nodes in xen device tree blob.
>>> -  This option is valid in the following two cases:
>>> +  This option is valid in the following cases:
>>>
>>>    1. When PASSTHROUGH_DTS_REPO is provided.
>>>    With this option, the partial device trees (corresponding to the
>>> @@ -149,7 +162,12 @@ Where:
>>>    Note it assumes that the names of the partial device trees will match
>>>    to the names of the devices specified here.
>>>
>>> -  2. When DOMU_NOBOOT[number] is provided. In this case, it will only
>>> +  2. When LOPPER_PATH is provided.
>>> +  With this option, the partial device trees (corresponding to the
>>> +  passthrough devices) are generated by the lopper and then compiled and merged
>>> +  by ImageBuilder to be used as DOMU[number] device tree blob.
>>> +
>>> +  3. When DOMU_NOBOOT[number] is provided. In this case, it will only
>>>    add "xen,passthrough" as mentioned before.
>>>
>>>  - DOMU_PASSTHROUGH_DTB[number] specifies the passthrough device trees
>>> diff --git a/scripts/common b/scripts/common
>>> index ccad03d82b30..680c5090cd07 100644
>>> --- a/scripts/common
>>> +++ b/scripts/common
>>> @@ -9,6 +9,9 @@
>>>  # - NUM_DOMUS
>>>  # - DOMU_PASSTHROUGH_PATHS
>>>  # - DOMU_PASSTHROUGH_DTB
>>> +# - LOPPER_PATH
>>> +# - LOPPER_CMD
>>> +# - DEVICE_TREE
>>>
>>>  tmp_files=()
>>>  tmp_dirs=()
>>> @@ -99,31 +102,41 @@ function compile_merge_partial_dts()
>>>      local tmp
>>>      local tmpdts
>>>      local file
>>> +    local node
>>>      local i
>>>      local j
>>>
>>> -    if [[ "$repo" =~ .*@.*:.* ]]
>>> +    if test "$repo"
>>>      then
>>> -        tmp=`mktemp -d`
>>> -        tmp_dirs+=($tmp)
>>> -
>>> -        echo "Cloning git repo \"$git_repo\""
>>> -        git clone "$repo" $tmp
>>> -        if test $? -ne 0
>>> +        # Partial dts will be obtained from PASSTHROUGH_DTS_REPO
>>> +        if [[ "$repo" =~ .*@.*:.* ]]
>>>          then
>>> -            echo "Error occurred while cloning \"$git_repo\""
>>> -            return 1
>>> -        fi
>>> +            tmp=`mktemp -d`
>>> +            tmp_dirs+=($tmp)
>>>
>>> -        repo=$tmp
>>> -    fi
>>> +            echo "Cloning git repo \"$git_repo\""
>>> +            git clone "$repo" $tmp
>>> +            if test $? -ne 0
>>> +            then
>>> +                echo "Error occurred while cloning \"$git_repo\""
>>> +                return 1
>>> +            fi
>>>
>>> -    if test -z "$dir"
>>> -    then
>>> -        dir="."
>>> +            repo=$tmp
>>> +        fi
>>> +
>>> +        if test -z "$dir"
>>> +        then
>>> +            dir="."
>>> +        fi
>>> +        partial_dts_dir="$repo"/"$dir"
>>> +    else
>>> +        # Partial dts will be generated by the lopper
>>> +        tmp=`mktemp -d`
>>> +        tmp_dirs+=($tmp)
>>
>> setting tmp and tmp_dirs can be moved outside of the if
>>
> Ok.
> 
Actually, these cannot be moved outside of the if because we have
3 possibilities and we need to create tmp dir only in 2 of them.
1) partial dts stored in repository - tmp needed
2) partial dts stored in a local dir - tmp not needed
3) partial dts will be generated by lopper - tmp needed

Moving the tmp creation at the top of if would result in creating redundant tmp
for second case. So it should stay as it is.

~Michal

Re: [ImageBuilder 0/2] Use lopper to generate partial dts

[Michal Orzel]

Hi Ayan,

On 12/09/2022 18:27, Ayan Kumar Halder wrote:
> Hi Michal,
> 
> On 12/09/2022 12:59, Michal Orzel wrote:
>> This patch series introduces support to generate automatically passthrough
>> device trees using lopper. This feature should be used with care as the
>> corresponding lopper changes are still in an early support state. Current
>> integration has been tested with several devices from ZynqMP ZCU102 board
>> e.g. serial, spi, ahci, mmc.
>>
>> When using this feature, make sure to use the latest lopper's master branch
>> status [1].
> 
> I am guessing that this is the first time the imagebuilder is using 
> script from an external repo. There might always be a possibility that 
> future changes to lopper (master branch) might not be backward 
> compatible or might break something in imagebuilder.
> 
> As such, will it make things better if lopper is included as a 
> gitsubmodule for imagebuilder. This way a specific revision of lopper 
> will be in sync with a specific revision of imagebuilder.
> 
> Please let me know your thoughts.
> 
I think it could be beneficial in the future but not in the current state.
The reason why is that the lopper changes are in an early support state
(I try to highlight it on each occasion). This means that in the near
future we will be improving lopper extract assists to cover some corner cases.
Adding lopper as a submodule now, would result in a need of additional commits
for the ImageBuilder fetching new lopper changes each time we improve something
in lopper. I think we do not need such overhead at this stage.

Also lopper's README states that "Internal interfaces are subject to change"
so we can assume that the interface given to the user will not change.

> - Ayan
> 
>>
>> [1] https://github.com/devicetree-org/lopper
>>
>> Michal Orzel (2):
>>    Refactor sanity_check_partial_dts
>>    Add support for lopper to generate partial dts
>>
>>   README.md                | 22 ++++++++++--
>>   scripts/common           | 73 +++++++++++++++++++++++++++++-----------
>>   scripts/uboot-script-gen | 17 ++++++++--
>>   3 files changed, 88 insertions(+), 24 deletions(-)
>>

~Michal

Re: [ImageBuilder 2/2] Add support for lopper to generate partial dts

[Stefano Stabellini]

On Mon, 12 Sep 2022, Michal Orzel wrote:
> On 12/09/2022 18:41, Ayan Kumar Halder wrote:
> > Hi Michal,
> > 
> > On 12/09/2022 12:59, Michal Orzel wrote:
> >> Currently ImageBuilder can compile and merge partial dts obtained from
> >> a repository specified using PASSTHROUGH_DTS_REPO. With the recent
> >> changes done in the lopper, we can use it to generate partial dts
> >> automatically (to some extent as this is still an early support).
> >>
> >> Introduce LOPPER_PATH option to specify a path to a lopper.py script,
> >> that if set, will invoke lopper to generate partial dts for the
> >> passthrough devices specified in DOMU_PASSTHROUGH_PATHS.
> >>
> >> Introduce LOPPER_CMD option to specify custom command line arguments
> >> (if needed) for lopper's extract assist.
> >>
> >> Example usage:
> >> LOPPER_PATH="/home/user/lopper/lopper.py"
> >> DOMU_PASSTHROUGH_PATHS[0]="/axi/spi@ff0f0000 /axi/serial@ff010000"
> >>
> >> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> >> ---
> >>   README.md                | 22 ++++++++++++--
> >>   scripts/common           | 64 ++++++++++++++++++++++++++++++----------
> >>   scripts/uboot-script-gen | 17 +++++++++--
> >>   3 files changed, 83 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/README.md b/README.md
> >> index da9ba788a3bf..aaee0939b589 100644
> >> --- a/README.md
> >> +++ b/README.md
> >> @@ -128,6 +128,19 @@ Where:
> >>   - DT_OVERLAY[number] specifies the path to the hosts device tree overlays
> >>     to be added at boot time in u-boot
> >>   
> >> +- LOPPER_PATH specifies the path to lopper.py script. This is optional.
> >> +  However, if this is specified, then DOMU_PASSTHROUGH_PATHS[number] need
> >> +  to be specified. uboot-script-gen will invoke lopper to generate the partial
> >> +  device trees which have been specified in DOMU_PASSTHROUGH_PATHS[number].
> >> +  This option is currently in experimental state as the corresponding lopper
> >> +  changes are still in an early support state.
> >> +
> >> +- LOPPER_CMD specifies the command line arguments for lopper's extract assist.
> >> +  This is optional and only applicable when LOPPER_PATH is specified. Only to be
> >> +  used to specify which nodes to include (using -i <node_name>) and which
> >> +  nodes/properties to exclude (using -x <regex>). If not set at all, the default
> >> +  one is used applicable for ZynqMP MPSoC boards.
> > 
> > You are using some more arguments (besides -x and -i) :-
> > 
> > --permissive -f
> > -- extract -t
> > -- extract-xen -t $node -o
> These ones are fixed and do not differ depending on the type of device or board.
> That is why LOPPER_CMD is used only to allow users to specify what can be required
> to support a new device (usually not necessary) or a new board.
> 
> > 
> > It will be good to have some explaination for these. See my comments below.
> > 
> We don't seem to do it in general (see all the commands used by disk_image) so I think
> we should only describe what is available to the user. Otherwise we would need to be
> consistent and apply this rule to all the other places.


My thinking is that Lopper documentation is best kept under the Lopper
repository. ImageBuilder documentation should be under the ImageBuilder
repository.

In this case, I think Lopper might benefit from better docs on how to
use extract-xen. extract-xen doesn't even seem to be described in
README.md?

I think it would be good to add at least a mention there, or another doc
under lopper.git.

Here in ImageBuilder I don't know if I would add anything. We could
explain why we chose this set of Lopper command line options, but I
think that if Lopper was well documented we wouldn't need to.

So in conclusion: I am OK with no extra docs in this series but please
have a look at lopper.git to see if we are missing anything there.

Do you guys agree?

Re: [ImageBuilder 0/2] Use lopper to generate partial dts

[Stefano Stabellini]

On Tue, 13 Sep 2022, Michal Orzel wrote:
> Hi Ayan,
> 
> On 12/09/2022 18:27, Ayan Kumar Halder wrote:
> > Hi Michal,
> > 
> > On 12/09/2022 12:59, Michal Orzel wrote:
> >> This patch series introduces support to generate automatically passthrough
> >> device trees using lopper. This feature should be used with care as the
> >> corresponding lopper changes are still in an early support state. Current
> >> integration has been tested with several devices from ZynqMP ZCU102 board
> >> e.g. serial, spi, ahci, mmc.
> >>
> >> When using this feature, make sure to use the latest lopper's master branch
> >> status [1].
> > 
> > I am guessing that this is the first time the imagebuilder is using 
> > script from an external repo. There might always be a possibility that 
> > future changes to lopper (master branch) might not be backward 
> > compatible or might break something in imagebuilder.
> > 
> > As such, will it make things better if lopper is included as a 
> > gitsubmodule for imagebuilder. This way a specific revision of lopper 
> > will be in sync with a specific revision of imagebuilder.
> > 
> > Please let me know your thoughts.
>
> I think it could be beneficial in the future but not in the current state.
> The reason why is that the lopper changes are in an early support state
> (I try to highlight it on each occasion). This means that in the near
> future we will be improving lopper extract assists to cover some corner cases.
> Adding lopper as a submodule now, would result in a need of additional commits
> for the ImageBuilder fetching new lopper changes each time we improve something
> in lopper. I think we do not need such overhead at this stage.
> 
> Also lopper's README states that "Internal interfaces are subject to change"
> so we can assume that the interface given to the user will not change.

Forward and backward compatibility is something we'll need to think
about at some point.

Personally I dislike git submodules and I would try to avoid using them
unless strictly necessary. However, we could specify a commit-id or tag
to use (the same way Yocto specifies component versions.)

Given that it is still early stage for this feature, I think we could
ignore the problem for now and come back to it in the future.

Or we could change this patch series now to take as LOPPER_PATH input
something like a SRC_URI in Yocto, which could be any of the following:

git://some.host/somepath;branch=branchX,branchY;name=nameX,nameY
https://some.host/somepath;branch=branchX,branchY;name=nameX,nameY
file://local.path/to/file.txt

If we did this, it would be more future proof and we could use the
https:// URI by default with the "master" or "master-next" branch so
that we would automatically get the latest updates. In the future we
would specificy a stable branch instead (e.g. v0.2022.x).

Re: [ImageBuilder 0/2] Use lopper to generate partial dts

[Michal Orzel]

Hi Stefano,

On 13/09/2022 21:28, Stefano Stabellini wrote:
> 
> 
> On Tue, 13 Sep 2022, Michal Orzel wrote:
>> Hi Ayan,
>>
>> On 12/09/2022 18:27, Ayan Kumar Halder wrote:
>>> Hi Michal,
>>>
>>> On 12/09/2022 12:59, Michal Orzel wrote:
>>>> This patch series introduces support to generate automatically passthrough
>>>> device trees using lopper. This feature should be used with care as the
>>>> corresponding lopper changes are still in an early support state. Current
>>>> integration has been tested with several devices from ZynqMP ZCU102 board
>>>> e.g. serial, spi, ahci, mmc.
>>>>
>>>> When using this feature, make sure to use the latest lopper's master branch
>>>> status [1].
>>>
>>> I am guessing that this is the first time the imagebuilder is using
>>> script from an external repo. There might always be a possibility that
>>> future changes to lopper (master branch) might not be backward
>>> compatible or might break something in imagebuilder.
>>>
>>> As such, will it make things better if lopper is included as a
>>> gitsubmodule for imagebuilder. This way a specific revision of lopper
>>> will be in sync with a specific revision of imagebuilder.
>>>
>>> Please let me know your thoughts.
>>
>> I think it could be beneficial in the future but not in the current state.
>> The reason why is that the lopper changes are in an early support state
>> (I try to highlight it on each occasion). This means that in the near
>> future we will be improving lopper extract assists to cover some corner cases.
>> Adding lopper as a submodule now, would result in a need of additional commits
>> for the ImageBuilder fetching new lopper changes each time we improve something
>> in lopper. I think we do not need such overhead at this stage.
>>
>> Also lopper's README states that "Internal interfaces are subject to change"
>> so we can assume that the interface given to the user will not change.
> 
> Forward and backward compatibility is something we'll need to think
> about at some point.
> 
> Personally I dislike git submodules and I would try to avoid using them
> unless strictly necessary. However, we could specify a commit-id or tag
> to use (the same way Yocto specifies component versions.)
> 
> Given that it is still early stage for this feature, I think we could
> ignore the problem for now and come back to it in the future.
> 
> Or we could change this patch series now to take as LOPPER_PATH input
> something like a SRC_URI in Yocto, which could be any of the following:
> 
> git://some.host/somepath;branch=branchX,branchY;name=nameX,nameY
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsome.host%2Fsomepath%3Bbranch%3DbranchX%2CbranchY%3Bname%3DnameX%2CnameY&data=05%7C01%7Cmichal.orzel%40amd.com%7C5ae88781fff24f8e6a8c08da95be2fb6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637986941323037721%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=dHzcGYqr9mIv2t746FnEE8QTrSmxqBJ4G9esebbAnu4%3D&reserved=0
> file://local.path/to/file.txt
> 
> If we did this, it would be more future proof and we could use the
> https:// URI by default with the "master" or "master-next" branch so
> that we would automatically get the latest updates. In the future we
> would specificy a stable branch instead (e.g. v0.2022.x).

This is a good idea in general but has one big drawback IMO.
Specifying the git repository such as lopper to be cloned by ImageBuilder results in
transferring the responsibility of installing prerequisite packages required by looper from a user to ImageBuilder.
Some of the packages are available to download using apt-get install and some of them unfortunately using pip manager which can be tricky sometimes.
In the current solution, lopper is an external dependency and the user is responsible for cloning the lopper and making sure the packages are installed,
in the same way as we require e.g. mkfs.ext4 to be installed. However, cloning a project from ImageBuilder means that it is the one who needs to fulfill the requirements
of the cloned project and keep up with syncing them (different package versions required by different versions of lopper).

Once the lopper+imagebuilder integration is in the shippable state, we can just inform user in the README with regards
to which lopper's branch/commit-id should be used.

~Michal