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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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


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?