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

[Michal Orzel <michal.orzel@xxxxxxx>]

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@xxxxxxx>
>> ---
>>   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