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

Michal Orzel posted 2 patches 1 year, 7 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
README.md                | 22 ++++++++++--
scripts/common           | 73 +++++++++++++++++++++++++++++-----------
scripts/uboot-script-gen | 17 ++++++++--
3 files changed, 88 insertions(+), 24 deletions(-)
[ImageBuilder 0/2] Use lopper to generate partial dts
Posted by Michal Orzel 1 year, 7 months ago
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
Re: [ImageBuilder 0/2] Use lopper to generate partial dts
Posted by Ayan Kumar Halder 1 year, 7 months ago
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 0/2] Use lopper to generate partial dts
Posted by Michal Orzel 1 year, 7 months ago
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 0/2] Use lopper to generate partial dts
Posted by Stefano Stabellini 1 year, 7 months ago
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
Posted by Michal Orzel 1 year, 7 months ago
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