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

[Michal Orzel <michal.orzel@xxxxxxx>]

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@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.
>>> +
>>>  - 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