Re: [RFC PATCH 04/23] hw/arm: add SVD file for NXP i.MX RT595

[Octavian Purdila]

On Fri, Aug 9, 2024 at 2:13 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Tue, Aug 06, 2024 at 01:31:51PM -0700, Octavian Purdila wrote:
> > On Tue, Aug 6, 2024 at 7:06 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> > >
> > > Octavian Purdila <tavip@google.com> writes:
> > >
> > > > Picked from:
> > > >
> > > > https://github.com/nxp-mcuxpresso/mcux-soc-svd/blob/main/MIMXRT595S/MIMXRT595S_cm33.xml
> > > >
> > > > NOTE: the file is truncated to keep the email size reasonable. Please
> > > > use the link above and download the full file if you want to try out
> > > > the patch.
>
> "the file is truncated" wins understatement of the week.
>
> The full XML file that would need to be in QEMU git for this is 8.6 MB in
> size.
>
> The overall generated headers that we get from it are ~16k lines,
> or ~0.5 MB
>
> $ wc -l build/hw/arm/svd/*.h
>    135 build/hw/arm/svd/flexcomm.h
>   1227 build/hw/arm/svd/flexcomm_i2c.h
>   1161 build/hw/arm/svd/flexcomm_spi.h
>   1231 build/hw/arm/svd/flexcomm_usart.h
>   2243 build/hw/arm/svd/flexspi.h
>   3100 build/hw/arm/svd/rt500_clkctl0.h
>   4022 build/hw/arm/svd/rt500_clkctl1.h
>     64 build/hw/arm/svd/rt500.h
>   1073 build/hw/arm/svd/rt500_rstctl0.h
>   1697 build/hw/arm/svd/rt500_rstctl1.h
>  15953 total
>
> $ wc -c build/hw/arm/svd/*.h
>   4349 build/hw/arm/svd/flexcomm.h
>  51135 build/hw/arm/svd/flexcomm_i2c.h
>  43822 build/hw/arm/svd/flexcomm_spi.h
>  46331 build/hw/arm/svd/flexcomm_usart.h
>  89224 build/hw/arm/svd/flexspi.h
> 113952 build/hw/arm/svd/rt500_clkctl0.h
> 141885 build/hw/arm/svd/rt500_clkctl1.h
>   1881 build/hw/arm/svd/rt500.h
>  38881 build/hw/arm/svd/rt500_rstctl0.h
>  61449 build/hw/arm/svd/rt500_rstctl1.h
> 592909 total
>
>
>
> > > >
> > > > Signed-off-by: Octavian Purdila <tavip@google.com>
> > > > ---
> > > >  hw/arm/svd/MIMXRT595S_cm33.xml | 224052
> > > > ++++++++++++++++++++++++++++++
> > >
> > > I guess one thing we need to decide is if the source XML should live in
> > > the repository as the preferred method of making changes or just the
> > > translations generated by the tool.
> > >
> >
> > I think we might want to store the XML in the qemu repo, even if we
> > don't use it to generate the header files at compile time. This avoids
> > issues with the original XML moving, going away, changed in
> > incompatible ways, etc.
> >
> > As for generating the headers at compile time, I don't have a strong
> > preference. I like it because there is slightly less work to do and it
> > avoids dealing with resolving changes on both the SVD and the
> > generated headers. For example, the initial headers are committed,
> > then some changes are done directly to the headers and then we want to
> > pick up a new SVD from the vendor to support a new hardware revision.
>
> IIUC the structs/enums/etc are defining guest ABI. So if we want to
> preserve guest ABI for these devices across QEMU releases, we don't
> want the generated output to be arbitrarily changing. If there are
> different revisions of a device, we might need separate structs for
> each maintained in parallel.
>

I think we can review changes to prevent ABI breakages both for XML
and generated headers - unless we use meson git-wrap submodules.

>
>
> >
> > There are disadvantages as well: pysvd dependency for building qemu,
> > hard to review if the vendor dumps a new version with lots of changes
> > and we want to update to it for a new hardware revision, slight
> > increase in build time.
> >
> > > >  1 file changed, 224052 insertions(+)
> > > >  create mode 100644 hw/arm/svd/MIMXRT595S_cm33.xml
> > > >
> > > > diff --git a/hw/arm/svd/MIMXRT595S_cm33.xml 
> > > > b/hw/arm/svd/MIMXRT595S_cm33.xml
> > > > new file mode 100644
> > > > index 0000000000..8943aa3555
> > > > --- /dev/null
> > > > +++ b/hw/arm/svd/MIMXRT595S_cm33.xml
> > > > @@ -0,0 +1,1725 @@
> > > > +<?xml version="1.0" encoding="UTF-8"?>
> > > > +<device schemaVersion="1.3" 
> > > > xmlns:xs="http://www.w3.org/2001/XMLSchema-instance"; 
> > > > xs:noNamespaceSchemaLocation="CMSIS-SVD.xsd">
> > > > +  <vendor>nxp.com</vendor>
> > > > +  <name>MIMXRT595S_cm33</name>
> > > > +  <version>1.0</version>
> > > > +  <description>MIMXRT595SFAWC,MIMXRT595SFFOC</description>
> > > > +  <licenseText>
> > > > +Copyright 2016-2023 NXP
> > > > +SPDX-License-Identifier: BSD-3-Clause
> > > > +  </licenseText>
> > >
> > > This certainly seems compatible. XML is not the medium I personally
> > > would have chosen as a register specification language but I guess there
> > > are no other alternatives?
> > >
> >
> > I agree that the choice of XML is unfortunate but I am not aware of
> > alternatives, this is what vendors will provide.
>
> Given the size of the XML I'm inclined to say that we should just be
> committing the generated header files to qemu.git
>

Fine with me. I think we can also reconsider later with minimal effort
if this gets widely used and there is lots of churn.

Peter, what do you think?

> Then add https://github.com/nxp-mcuxpresso/mcux-soc-svd as a git
> submodule, and provide some meson rules for triggering re-generation
> that are off-by-default.
>

Sounds good. I was thinking of adding mcux-soc-svd as a meson
subproject via git-wrap as suggested by Philippe.



> With regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
>