| Message ID | cb49cac0a394625152bfe16e101a56021e8a91a4.1629371983.git.michal.simek@xilinx.com |
|---|---|
| State | Superseded |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | xilinx: Add support for DTB reselection | expand |
On 8/19/21 12:19 PM, Michal Simek wrote: Hi, > Enabling MULTI_DTB_FIT and DTB_RESELECT can end up with multi DTBs in FIT > image placed and aligned only by 32bits (4bytes). For 64bit systems there > is 64bit (8bytes) alignment required. That's why make sure that > fit-dtb.blob and u-boot.itb as our primary target images for Xilinx ZynqMP > are all 64bit aligned. The patch is using CONFIG_PHYS_64BIT macro to > identify 64bit systems (including 32bit systems with PAE). > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > Makefile | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/Makefile b/Makefile > index 269e353a28ad..1bbe95595efe 100644 > --- a/Makefile > +++ b/Makefile > @@ -1169,6 +1169,10 @@ MKIMAGEFLAGS_fit-dtb.blob = -f auto -A $(ARCH) -T firmware -C none -O u-boot \ > -a 0 -e 0 -E \ > $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST))) -d /dev/null > > +ifeq ($(CONFIG_PHYS_64BIT),y) Why is this restricted to 64-bit "systems"? The DT spec[1] clearly states that some DT parts (/memreserved/ block, for instance), must be 64-bit aligned, which means the whole blobs needs to be 64-bit aligned. Granted this probably does not cause real issues on 32-bit systems, but is violating the spec anyway. So I'd say we add the alignment requirement unconditionally. Cheers, Andre [1] https://github.com/devicetree-org/devicetree-specification/releases/download/v0.3/devicetree-specification-v0.3.pdf > +MKIMAGEFLAGS_fit-dtb.blob += -B 0x8 > +endif > + > ifneq ($(EXT_DTB),) > u-boot-fit-dtb.bin: u-boot-nodtb.bin $(EXT_DTB) > $(call if_changed,cat) > @@ -1431,6 +1435,9 @@ MKIMAGEFLAGS_u-boot.itb = > else > MKIMAGEFLAGS_u-boot.itb = -E > endif > +ifeq ($(CONFIG_PHYS_64BIT),y) > +MKIMAGEFLAGS_u-boot.itb += -B 0x8 > +endif > > ifdef U_BOOT_ITS > u-boot.itb: u-boot-nodtb.bin \ >
Hi Andre, On 8/19/21 5:56 PM, Andre Przywara wrote: > On 8/19/21 12:19 PM, Michal Simek wrote: > > Hi, > >> Enabling MULTI_DTB_FIT and DTB_RESELECT can end up with multi DTBs in FIT >> image placed and aligned only by 32bits (4bytes). For 64bit systems there >> is 64bit (8bytes) alignment required. That's why make sure that >> fit-dtb.blob and u-boot.itb as our primary target images for Xilinx >> ZynqMP >> are all 64bit aligned. The patch is using CONFIG_PHYS_64BIT macro to >> identify 64bit systems (including 32bit systems with PAE). >> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> Makefile | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/Makefile b/Makefile >> index 269e353a28ad..1bbe95595efe 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -1169,6 +1169,10 @@ MKIMAGEFLAGS_fit-dtb.blob = -f auto -A $(ARCH) >> -T firmware -C none -O u-boot \ >> -a 0 -e 0 -E \ >> $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst >> ",,$(CONFIG_OF_LIST))) -d /dev/null >> +ifeq ($(CONFIG_PHYS_64BIT),y) > > Why is this restricted to 64-bit "systems"? The DT spec[1] clearly > states that some DT parts (/memreserved/ block, for instance), must be > 64-bit aligned, which means the whole blobs needs to be 64-bit aligned. > Granted this probably does not cause real issues on 32-bit systems, but > is violating the spec anyway. > So I'd say we add the alignment requirement unconditionally. That's even better for me and we need to make sure that dtbs itself are aligned and also dtbs inside FIT image are aligned too. Cheers, Michal
On Thu, Aug 19, 2021 at 06:01:39PM +0200, Michal Simek wrote: > Hi Andre, > > On 8/19/21 5:56 PM, Andre Przywara wrote: > > On 8/19/21 12:19 PM, Michal Simek wrote: > > > > Hi, > > > >> Enabling MULTI_DTB_FIT and DTB_RESELECT can end up with multi DTBs in FIT > >> image placed and aligned only by 32bits (4bytes). For 64bit systems there > >> is 64bit (8bytes) alignment required. That's why make sure that > >> fit-dtb.blob and u-boot.itb as our primary target images for Xilinx > >> ZynqMP > >> are all 64bit aligned. The patch is using CONFIG_PHYS_64BIT macro to > >> identify 64bit systems (including 32bit systems with PAE). > >> > >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> > >> --- > >> > >> Makefile | 7 +++++++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/Makefile b/Makefile > >> index 269e353a28ad..1bbe95595efe 100644 > >> --- a/Makefile > >> +++ b/Makefile > >> @@ -1169,6 +1169,10 @@ MKIMAGEFLAGS_fit-dtb.blob = -f auto -A $(ARCH) > >> -T firmware -C none -O u-boot \ > >> -a 0 -e 0 -E \ > >> $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst > >> ",,$(CONFIG_OF_LIST))) -d /dev/null > >> +ifeq ($(CONFIG_PHYS_64BIT),y) > > > > Why is this restricted to 64-bit "systems"? The DT spec[1] clearly > > states that some DT parts (/memreserved/ block, for instance), must be > > 64-bit aligned, which means the whole blobs needs to be 64-bit aligned. > > Granted this probably does not cause real issues on 32-bit systems, but > > is violating the spec anyway. > > So I'd say we add the alignment requirement unconditionally. > > That's even better for me and we need to make sure that dtbs itself are > aligned and also dtbs inside FIT image are aligned too. Right, all dtbs need to be 8 byte aligned to start with. Enforcing this is what will unblock moving to a newer libfdt where that's checked for up-front now.
On 8/19/21 6:18 PM, Tom Rini wrote: > On Thu, Aug 19, 2021 at 06:01:39PM +0200, Michal Simek wrote: >> Hi Andre, >> >> On 8/19/21 5:56 PM, Andre Przywara wrote: >>> On 8/19/21 12:19 PM, Michal Simek wrote: >>> >>> Hi, >>> >>>> Enabling MULTI_DTB_FIT and DTB_RESELECT can end up with multi DTBs in FIT >>>> image placed and aligned only by 32bits (4bytes). For 64bit systems there >>>> is 64bit (8bytes) alignment required. That's why make sure that >>>> fit-dtb.blob and u-boot.itb as our primary target images for Xilinx >>>> ZynqMP >>>> are all 64bit aligned. The patch is using CONFIG_PHYS_64BIT macro to >>>> identify 64bit systems (including 32bit systems with PAE). >>>> >>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >>>> --- >>>> >>>> Makefile | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/Makefile b/Makefile >>>> index 269e353a28ad..1bbe95595efe 100644 >>>> --- a/Makefile >>>> +++ b/Makefile >>>> @@ -1169,6 +1169,10 @@ MKIMAGEFLAGS_fit-dtb.blob = -f auto -A $(ARCH) >>>> -T firmware -C none -O u-boot \ >>>> -a 0 -e 0 -E \ >>>> $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst >>>> ",,$(CONFIG_OF_LIST))) -d /dev/null >>>> +ifeq ($(CONFIG_PHYS_64BIT),y) >>> >>> Why is this restricted to 64-bit "systems"? The DT spec[1] clearly >>> states that some DT parts (/memreserved/ block, for instance), must be >>> 64-bit aligned, which means the whole blobs needs to be 64-bit aligned. >>> Granted this probably does not cause real issues on 32-bit systems, but >>> is violating the spec anyway. >>> So I'd say we add the alignment requirement unconditionally. >> >> That's even better for me and we need to make sure that dtbs itself are >> aligned and also dtbs inside FIT image are aligned too. > > Right, all dtbs need to be 8 byte aligned to start with. Enforcing this > is what will unblock moving to a newer libfdt where that's checked for > up-front now. > As is in the second thread. Does it make sense to also align the end? I did that in 8/10 patch. The problem with these alignments is that you also need to align the start of FIT image. Maybe would the best to copy fdt to different aligned location. Thanks, Michal
On Thu, Aug 19, 2021 at 06:31:25PM +0200, Michal Simek wrote: > > > On 8/19/21 6:18 PM, Tom Rini wrote: > > On Thu, Aug 19, 2021 at 06:01:39PM +0200, Michal Simek wrote: > >> Hi Andre, > >> > >> On 8/19/21 5:56 PM, Andre Przywara wrote: > >>> On 8/19/21 12:19 PM, Michal Simek wrote: > >>> > >>> Hi, > >>> > >>>> Enabling MULTI_DTB_FIT and DTB_RESELECT can end up with multi DTBs in FIT > >>>> image placed and aligned only by 32bits (4bytes). For 64bit systems there > >>>> is 64bit (8bytes) alignment required. That's why make sure that > >>>> fit-dtb.blob and u-boot.itb as our primary target images for Xilinx > >>>> ZynqMP > >>>> are all 64bit aligned. The patch is using CONFIG_PHYS_64BIT macro to > >>>> identify 64bit systems (including 32bit systems with PAE). > >>>> > >>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com> > >>>> --- > >>>> > >>>> Makefile | 7 +++++++ > >>>> 1 file changed, 7 insertions(+) > >>>> > >>>> diff --git a/Makefile b/Makefile > >>>> index 269e353a28ad..1bbe95595efe 100644 > >>>> --- a/Makefile > >>>> +++ b/Makefile > >>>> @@ -1169,6 +1169,10 @@ MKIMAGEFLAGS_fit-dtb.blob = -f auto -A $(ARCH) > >>>> -T firmware -C none -O u-boot \ > >>>> -a 0 -e 0 -E \ > >>>> $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst > >>>> ",,$(CONFIG_OF_LIST))) -d /dev/null > >>>> +ifeq ($(CONFIG_PHYS_64BIT),y) > >>> > >>> Why is this restricted to 64-bit "systems"? The DT spec[1] clearly > >>> states that some DT parts (/memreserved/ block, for instance), must be > >>> 64-bit aligned, which means the whole blobs needs to be 64-bit aligned. > >>> Granted this probably does not cause real issues on 32-bit systems, but > >>> is violating the spec anyway. > >>> So I'd say we add the alignment requirement unconditionally. > >> > >> That's even better for me and we need to make sure that dtbs itself are > >> aligned and also dtbs inside FIT image are aligned too. > > > > Right, all dtbs need to be 8 byte aligned to start with. Enforcing this > > is what will unblock moving to a newer libfdt where that's checked for > > up-front now. > > As is in the second thread. Does it make sense to also align the end? > I did that in 8/10 patch. > The problem with these alignments is that you also need to align the > start of FIT image. Maybe would the best to copy fdt to different > aligned location. Right, so a good question. I have suggested before that we stop assuming that we (U-Boot SPL, etc) can use the device tree in-place and instead move it somewhere aligned. I'm not sure off-hand what ends up being best, someone would need to investigate a bit.
diff --git a/Makefile b/Makefile index 269e353a28ad..1bbe95595efe 100644 --- a/Makefile +++ b/Makefile @@ -1169,6 +1169,10 @@ MKIMAGEFLAGS_fit-dtb.blob = -f auto -A $(ARCH) -T firmware -C none -O u-boot \ -a 0 -e 0 -E \ $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST))) -d /dev/null +ifeq ($(CONFIG_PHYS_64BIT),y) +MKIMAGEFLAGS_fit-dtb.blob += -B 0x8 +endif + ifneq ($(EXT_DTB),) u-boot-fit-dtb.bin: u-boot-nodtb.bin $(EXT_DTB) $(call if_changed,cat) @@ -1431,6 +1435,9 @@ MKIMAGEFLAGS_u-boot.itb = else MKIMAGEFLAGS_u-boot.itb = -E endif +ifeq ($(CONFIG_PHYS_64BIT),y) +MKIMAGEFLAGS_u-boot.itb += -B 0x8 +endif ifdef U_BOOT_ITS u-boot.itb: u-boot-nodtb.bin \
Enabling MULTI_DTB_FIT and DTB_RESELECT can end up with multi DTBs in FIT image placed and aligned only by 32bits (4bytes). For 64bit systems there is 64bit (8bytes) alignment required. That's why make sure that fit-dtb.blob and u-boot.itb as our primary target images for Xilinx ZynqMP are all 64bit aligned. The patch is using CONFIG_PHYS_64BIT macro to identify 64bit systems (including 32bit systems with PAE). Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- Makefile | 7 +++++++ 1 file changed, 7 insertions(+)