[PATCH] bsp/imxrt: Enforce alignment for devicetree

Wed Feb 16 16:01:58 UTC 2022

On Wed, Feb 16, 2022 at 9:24 AM Christian MAUDERER
<christian.mauderer at embedded-brains.de> wrote:
>
> Hello Joel,
>
> Am 16.02.22 um 16:04 schrieb Joel Sherrill:
> > On Wed, Feb 16, 2022 at 3:22 AM Christian Mauderer
> > <christian.mauderer at embedded-brains.de> wrote:
> >>
> >> A device tree binary has to be 8 byte aligned in memory. This is checked
> >> since RTEMS commit 34052ef78cf8724dee73e9279b2c6bff8cfed234 "libfdt: Add
> >> FDT alignment check to fdt_check_header()".
> >> ---
> >>   bsps/arm/imxrt/dts/imxrt1050-evkb.c   | 2 +-
> >>   bsps/arm/imxrt/dts/imxrt1050-evkb.dts | 2 +-
> >>   bsps/arm/imxrt/include/bsp.h          | 2 +-
> >>   3 files changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/bsps/arm/imxrt/dts/imxrt1050-evkb.c b/bsps/arm/imxrt/dts/imxrt1050-evkb.c
> >> index 301e790c6b..ecae160a8e 100644
> >> --- a/bsps/arm/imxrt/dts/imxrt1050-evkb.c
> >> +++ b/bsps/arm/imxrt/dts/imxrt1050-evkb.c
> >> @@ -6,7 +6,7 @@
> >>
> >>   #include <sys/types.h>
> >>
> >> -const unsigned char imxrt_dtb[] = {
> >> +const unsigned char imxrt_dtb[] __attribute__(( __aligned__(8) )) = {
> >>     0xd0, 0x0d, 0xfe, 0xed, 0x00, 0x00, 0x3f, 0xf6, 0x00, 0x00, 0x00, 0x38,
> >>     0x00, 0x00, 0x3b, 0xf8, 0x00, 0x00, 0x00, 0x28, 0x00, 0x00, 0x00, 0x11,
> >>     0x00, 0x00, 0x00, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0xbe,
> >> diff --git a/bsps/arm/imxrt/dts/imxrt1050-evkb.dts b/bsps/arm/imxrt/dts/imxrt1050-evkb.dts
> >> index 9310371428..dae7e8939f 100644
> >> --- a/bsps/arm/imxrt/dts/imxrt1050-evkb.dts
> >> +++ b/bsps/arm/imxrt/dts/imxrt1050-evkb.dts
> >> @@ -31,7 +31,7 @@
> >>    *     export BSP_DIR="${RTEMS_SRC_DIR}/bsps/arm/imxrt/"
> >>    *     arm-rtems6-cpp -P -x assembler-with-cpp -I "${BSP_DIR}/include/" -include "${BSP_DIR}/dts/imxrt1050-evkb.dts" /dev/null | \
> >>    *         dtc -O dtb -o "${BSP_DIR}/dts/imxrt1050-evkb.dtb" -b 0 -p 64
> >> - *     rtems-bin2c -C -N imxrt_dtb "${BSP_DIR}/dts/imxrt1050-evkb.dtb" "${BSP_DIR}/dts/imxrt1050-evkb.c"
> >> + *     rtems-bin2c -A 8 -C -N imxrt_dtb "${BSP_DIR}/dts/imxrt1050-evkb.dtb" "${BSP_DIR}/dts/imxrt1050-evkb.c"
> >>    */
> >>
> >>   /dts-v1/;
> >> diff --git a/bsps/arm/imxrt/include/bsp.h b/bsps/arm/imxrt/include/bsp.h
> >> index d7e37628e5..9f3b913a17 100644
> >> --- a/bsps/arm/imxrt/include/bsp.h
> >> +++ b/bsps/arm/imxrt/include/bsp.h
> >> @@ -57,7 +57,7 @@ extern "C" {
> >>   #define BSP_FEATURE_IRQ_EXTENSION
> >>
> >>   #define BSP_FDT_IS_SUPPORTED
> >> -extern const unsigned char imxrt_dtb[];
> >> +extern const unsigned char imxrt_dtb[] __attribute__(( __aligned__(8) ));
> >
> > Why not use RTEMS_ALIGNED?
>
> Because I generated the imxrt1050-evkb.c using the new option in the
> rtems-bin2c and I wanted to use the same name for the prototypes in the
> header. If I would have used RTEMS_ALIGNED in the code generated by
> rtems-bin2c, it would have been necessary to add the header for
> RTEMS_ALIGNED in the generated code. I wanted to avoid that so
> rtems-bin2c generated code doesn't get new dependencies.

yep. rtems-bin2c code should compile without rtems so it is useful
on any host for any application. I was wondering about a naked
attribute inside RTEMS. Not a huge deal. I guess consistency wins.

>
> >
> > And is it assumed that if the user wants to provide a different
> > device tree, they will follow the same naming? Otherwise, why
> > are symbols in the public facing bsp.h.
> >
>
> The i.MXRT doesn't have a bootloader that could provide the device tree.
> Therefore at the moment I just linked the device tree into the binary.
> Using a fixed name was the simplest method for allowing a user to
> replace the device tree blob. The bsp_fdt_get just retuns the pointer to
> that blob:
>
> https://git.rtems.org/rtems/tree/bsps/arm/imxrt/start/bspstart.c?id=d24da94f620af3b8446a8305056bf690339bbeac#n74

OK. Then they are considered part of the public facing interface for
the BSP. Just checking.

> The commands that I documented generate exactly that name:
>
> https://docs.rtems.org/branches/master/user/bsps/bsps-arm.html#fdt
>
> By the way: I will have to update the docuentation too as soon as this
> patch is pushed. Thanks for the hint.

:) Just double checking the public view of bsp.h to make sure you
were not accidentally leaking something. It's a continuous battle.

> The alternative would have been to move the bsp_fdt_get into a separate
> C file and skip the symbols in the bsp.h header. In that case, a user
> would have to add his own device tree blob and overwrite the bsp_fdt_get
> so that it returns his own blob. That seemed like an unnecessary extra step.
>
> Best regards
>
> Christian
>
> >>   extern const size_t imxrt_dtb_size;
> >>
> >>   void *imx_get_reg_of_node(const void *fdt, int node);
> >> --
> >> 2.31.1
> >>
> >> _______________________________________________
> >> devel mailing list
> >> devel at rtems.org
> >> http://lists.rtems.org/mailman/listinfo/devel
>
> --
> --------------------------------------------
> embedded brains GmbH
> Herr Christian MAUDERER
> Dornierstr. 4
> 82178 Puchheim
> Germany
> email: christian.mauderer at embedded-brains.de
> phone: +49-89-18 94 741 - 18
> fax:   +49-89-18 94 741 - 08
>
> Registergericht: Amtsgericht München
> Registernummer: HRB 157899
> Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
> Unsere Datenschutzerklärung finden Sie hier:
> https://embedded-brains.de/datenschutzerklaerung/