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