| Message ID | 1468297308-32102-5-git-send-email-afaerber@suse.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
On 12.07.16 06:21, Andreas Färber wrote: > On arm64 Linux device trees are organized by SoC vendor. Therefore we > need to search the vendor subdirectory as well. > > Since the SoC vendor may be different from ${vendor}, introduce a new > ${soc_vendor}. If this is not set, the behavior remains unchanged. > > Cc: Alexander Graf <agraf@suse.de> > Signed-off-by: Andreas Färber <afaerber@suse.de> Stephen had pretty strong opinions on the naming, mostly because "pxe boot" uses the same naming scheme. So we should either change it there as well to stay consistent or just make the implicit ruling always work :). I guess the best case would be to fix up the path names of boards so that $vendor is always $soc_vendor. Do you have an example where that wouldn't work? Alex > --- > include/config_distro_bootcmd.h | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h > index eadec2e..8f14457 100644 > --- a/include/config_distro_bootcmd.h > +++ b/include/config_distro_bootcmd.h > @@ -113,6 +113,15 @@ > #define BOOTENV_EFI_SET_FDTFILE_FALLBACK > #endif > > +#if defined(CONFIG_ARM64) > +#define BOOTENV_EFI_SET_FDTFILE_VENDOR \ > + "if test -n \"${soc_vendor}\"; then " \ > + "setenv efi_dtb_vendor_prefix ${soc_vendor}/; " \ > + "fi; " > +#else > +#define BOOTENV_EFI_SET_FDTFILE_VENDOR > +#endif > + > #define BOOTENV_SHARED_EFI \ > "boot_efi_binary=" \ > "load ${devtype} ${devnum}:${distro_bootpart} " \ > @@ -125,18 +134,25 @@ > \ > "load_efi_dtb=" \ > "load ${devtype} ${devnum}:${efi_bootpart} ${fdt_addr_r} "\ > - "${prefix}${dtb_prefix}${efi_fdtfile}\0" \ > + "${prefix}${dtb_prefix}${dtb_vendor_prefix}" \ > + "${efi_fdtfile}\0" \ > \ > "efi_dtb_prefixes=\"\" dtb/ dtb/current/\0" \ > "scan_dev_for_efi_fdt=" \ > "for prefix in ${boot_prefixes}; do " \ > "for dtb_prefix in ${efi_dtb_prefixes}; do " \ > + BOOTENV_EFI_SET_FDTFILE_VENDOR \ > + "for dtb_vendor_prefix in \"\" " \ > + "${efi_dtb_vendor_prefix}; do " \ > "if test -e ${devtype} " \ > "${devnum}:${efi_bootpart} " \ > "${prefix}${dtb_prefix}" \ > + "${dtb_vendor_prefix}" \ > "${efi_fdtfile}; then " \ > "run load_efi_dtb; " \ > "fi; " \ > + "done; " \ > + "setenv efi_dtb_vendor_prefix; " \ > "done; " \ > "done\0" \ > "scan_dev_for_efi=" \ >
Am 12.07.2016 um 09:29 schrieb Alexander Graf: > On 12.07.16 06:21, Andreas Färber wrote: >> On arm64 Linux device trees are organized by SoC vendor. Therefore we >> need to search the vendor subdirectory as well. >> >> Since the SoC vendor may be different from ${vendor}, introduce a new >> ${soc_vendor}. If this is not set, the behavior remains unchanged. >> >> Cc: Alexander Graf <agraf@suse.de> >> Signed-off-by: Andreas Färber <afaerber@suse.de> > > Stephen had pretty strong opinions on the naming, mostly because "pxe > boot" uses the same naming scheme. So we should either change it there > as well to stay consistent or just make the implicit ruling always work :). > > I guess the best case would be to fix up the path names of boards so > that $vendor is always $soc_vendor. Do you have an example where that > wouldn't work? AFAIU for U-Boot $vendor is often the board vendor and matches the board/ subdirectory, so there may be plenty vendors. I didn't see any code setting this value, so I assume this comes from .config options. My $soc_vendor is the SoC vendor instead and only for the environment. And because there were opinions on how to form the 32-bit efi_fdtfile, both Tom and Stephen were CC'ed on the cover letter. :) On the dragonboard410c: CONFIG_SYS_SOC="snapdragon" CONFIG_SYS_VENDOR="qualcomm" CONFIG_SYS_BOARD="dragonboard410c" CONFIG_SYS_CONFIG_NAME="dragonboard410c" soc=snapdragon vendor=qualcomm board=dragonboard410c board_name=dragonboard410c => Therefore my previous patch setting fdtfile to apq8016-sbc.dtb, because it is absolutely different. This patch allows searching qcom/ for our dtb-qcom aarch64 openSUSE package. On the odroid-c2: CONFIG_SYS_SOC="meson" CONFIG_SYS_VENDOR="amlogic" CONFIG_SYS_BOARD="odroid-c2" CONFIG_SYS_CONFIG_NAME="odroid-c2" board=odroid-c2 board_name=odroid-c2 soc=meson vendor=amlogic => Here $vendor would match Linux' dts subdirectory, but $soc and $board are wrong, I need fdtfile=meson-gxbb-odroidc2.dtb. But maybe I'm just not understanding what all those configs are good for and how deeply they're tied to board/ subdirectories and mach-foo? If we can clean them up to match Linux then all the better. If that were intended, I wonder why no one flagged that during review. Anyway, maybe add an else clause to this patch and reuse $vendor if $soc_vendor is unavailable? Regards, Andreas
Am 12.07.2016 um 14:38 schrieb Andreas Färber: > Am 12.07.2016 um 09:29 schrieb Alexander Graf: >> On 12.07.16 06:21, Andreas Färber wrote: >>> On arm64 Linux device trees are organized by SoC vendor. Therefore we >>> need to search the vendor subdirectory as well. >>> >>> Since the SoC vendor may be different from ${vendor}, introduce a new >>> ${soc_vendor}. If this is not set, the behavior remains unchanged. >>> >>> Cc: Alexander Graf <agraf@suse.de> >>> Signed-off-by: Andreas Färber <afaerber@suse.de> >> >> Stephen had pretty strong opinions on the naming, mostly because "pxe >> boot" uses the same naming scheme. So we should either change it there >> as well to stay consistent or just make the implicit ruling always work :). >> >> I guess the best case would be to fix up the path names of boards so >> that $vendor is always $soc_vendor. Do you have an example where that >> wouldn't work? > > AFAIU for U-Boot $vendor is often the board vendor and matches the > board/ subdirectory, so there may be plenty vendors. I didn't see any > code setting this value, so I assume this comes from .config options. > > My $soc_vendor is the SoC vendor instead and only for the environment. > > And because there were opinions on how to form the 32-bit efi_fdtfile, > both Tom and Stephen were CC'ed on the cover letter. :) > > On the dragonboard410c: > CONFIG_SYS_SOC="snapdragon" > CONFIG_SYS_VENDOR="qualcomm" > CONFIG_SYS_BOARD="dragonboard410c" > CONFIG_SYS_CONFIG_NAME="dragonboard410c" > soc=snapdragon > vendor=qualcomm > board=dragonboard410c > board_name=dragonboard410c > => Therefore my previous patch setting fdtfile to apq8016-sbc.dtb, > because it is absolutely different. This patch allows searching qcom/ > for our dtb-qcom aarch64 openSUSE package. > > On the odroid-c2: > CONFIG_SYS_SOC="meson" > CONFIG_SYS_VENDOR="amlogic" > CONFIG_SYS_BOARD="odroid-c2" > CONFIG_SYS_CONFIG_NAME="odroid-c2" > board=odroid-c2 > board_name=odroid-c2 > soc=meson > vendor=amlogic > => Here $vendor would match Linux' dts subdirectory, but $soc and $board > are wrong, I need fdtfile=meson-gxbb-odroidc2.dtb. Adding a 32-bit example: On the firefly-rk3288: CONFIG_SYS_SOC="rockchip" CONFIG_SYS_VENDOR="firefly" CONFIG_SYS_BOARD="firefly-rk3288" CONFIG_SYS_CONFIG_NAME="firefly-rk3288" board=firefly-rk3288 board_name=firefly-rk3288 soc=rockchip vendor=firefly => $vendor is not the SoC vendor, it's the board vendor. My point. => $soc is not rk3288, as needed for the ${soc}-${board}${boardrev}.dtb formula. We either need to fix $soc or define $fdtfile. Further, I have an early board that needs rk3288-firefly-beta.dtb instead of rk3288-firefly.dtb. No hits for `git grep firefly-beta`, so there doesn't seem to be any auto-detection... Note that ~2016.07 seemed to be completely busted on this board, see the multiple discussions about disabling EFI_LOADER and other random options as workaround. Not even bootz succeeded for me any more. Having a default way to load some uboot.env or uEnv.txt file might help the user set $fdtfile and similar options while still using the default boot mechanism. > > But maybe I'm just not understanding what all those configs are good for > and how deeply they're tied to board/ subdirectories and mach-foo? If we > can clean them up to match Linux then all the better. If that were > intended, I wonder why no one flagged that during review. > > Anyway, maybe add an else clause to this patch and reuse $vendor if > $soc_vendor is unavailable? > > Regards, > Andreas
On Tue, Jul 12, 2016 at 06:21:45AM +0200, Andreas Färber wrote: > On arm64 Linux device trees are organized by SoC vendor. Therefore we > need to search the vendor subdirectory as well. > > Since the SoC vendor may be different from ${vendor}, introduce a new > ${soc_vendor}. If this is not set, the behavior remains unchanged. > > Cc: Alexander Graf <agraf@suse.de> > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > include/config_distro_bootcmd.h | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h > index eadec2e..8f14457 100644 > --- a/include/config_distro_bootcmd.h > +++ b/include/config_distro_bootcmd.h > @@ -113,6 +113,15 @@ > #define BOOTENV_EFI_SET_FDTFILE_FALLBACK > #endif > > +#if defined(CONFIG_ARM64) > +#define BOOTENV_EFI_SET_FDTFILE_VENDOR \ > + "if test -n \"${soc_vendor}\"; then " \ > + "setenv efi_dtb_vendor_prefix ${soc_vendor}/; " \ > + "fi; " > +#else OK. Looking at the current Linux kernel, it's a given that for arm64 a DTB will be in a subdirectory, always. Perhaps we should fix this in Kconfig and have... CONFIG_FDTFILE_VENDOR_DIRECTORY and set this correctly per vendor (and yes, eventually there will be some "fun" as NXP boards will sometimes be in freescale/ and sometimes nxp/ so maybe try and futureproof ourselves so that we loop over this variable) ?
Am 12.07.2016 um 16:50 schrieb Tom Rini: > On Tue, Jul 12, 2016 at 06:21:45AM +0200, Andreas Färber wrote: > >> On arm64 Linux device trees are organized by SoC vendor. Therefore we >> need to search the vendor subdirectory as well. >> >> Since the SoC vendor may be different from ${vendor}, introduce a new >> ${soc_vendor}. If this is not set, the behavior remains unchanged. >> >> Cc: Alexander Graf <agraf@suse.de> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> --- >> include/config_distro_bootcmd.h | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h >> index eadec2e..8f14457 100644 >> --- a/include/config_distro_bootcmd.h >> +++ b/include/config_distro_bootcmd.h >> @@ -113,6 +113,15 @@ >> #define BOOTENV_EFI_SET_FDTFILE_FALLBACK >> #endif >> >> +#if defined(CONFIG_ARM64) >> +#define BOOTENV_EFI_SET_FDTFILE_VENDOR \ >> + "if test -n \"${soc_vendor}\"; then " \ >> + "setenv efi_dtb_vendor_prefix ${soc_vendor}/; " \ >> + "fi; " >> +#else > > OK. Looking at the current Linux kernel, it's a given that for arm64 a > DTB will be in a subdirectory, always. Perhaps we should fix this in > Kconfig and have... CONFIG_FDTFILE_VENDOR_DIRECTORY and set this > correctly per vendor (and yes, eventually there will be some "fun" as > NXP boards will sometimes be in freescale/ and sometimes nxp/ so maybe > try and futureproof ourselves so that we loop over this variable) ? Yes, it's a given that in the kernel the .dts is always in a vendor subdirectory - as listed in an earlier thread also for mips etc. And yes, it's also true that due to our scripted openSUSE packaging we therefore install .dtb to /boot/dtb-foo/vendor/ - but that's not guaranteed for other distros and users, is it? Therefore I include "" in the loop further down. Anyway, adding a config option sounds good. Will you look into that yourself or should I do that as part of this series? I had considered the ambiguous vendor case and am iterating over $efi_dtb_vendor_prefix in a list of vendors already below, only the addition of the trailing slash above is limiting us to one currently. Do you have a concrete suggestion of how to make that more flexible? Cheers, Andreas
On 07/12/2016 01:29 AM, Alexander Graf wrote: > > > On 12.07.16 06:21, Andreas Färber wrote: >> On arm64 Linux device trees are organized by SoC vendor. Therefore we >> need to search the vendor subdirectory as well. >> >> Since the SoC vendor may be different from ${vendor}, introduce a new >> ${soc_vendor}. If this is not set, the behavior remains unchanged. >> >> Cc: Alexander Graf <agraf@suse.de> >> Signed-off-by: Andreas Färber <afaerber@suse.de> > > Stephen had pretty strong opinions on the naming, mostly because "pxe > boot" uses the same naming scheme. So we should either change it there > as well to stay consistent or just make the implicit ruling always work :). > > I guess the best case would be to fix up the path names of boards so > that $vendor is always $soc_vendor. Do you have an example where that > wouldn't work? All I'll say is that if the "auto calculation" algorithm of ${soc}-${board}.dtb doesn't work, U-Boot should set ${fdtfile} with the correct full name. Introducing other more complex "auto calculation" is just going to lead to more complexity and yet still not solve 100% of the cases, which will be confusing. I'm unlikely to review any other aspects of the series, since I'm still quite disappointed that distros wouldn't engage in the discussions boot methods and agree on one solution, and that config_distro*.h is being abused as a dumping ground for tons of different boot methods; the whole point of it was to unify on *one* method that distros could rely upon, which is the opposite of where it's now going:-(
On Tue, Jul 12, 2016 at 05:59:58PM +0200, Andreas Färber wrote: > Am 12.07.2016 um 16:50 schrieb Tom Rini: > > On Tue, Jul 12, 2016 at 06:21:45AM +0200, Andreas Färber wrote: > > > >> On arm64 Linux device trees are organized by SoC vendor. Therefore we > >> need to search the vendor subdirectory as well. > >> > >> Since the SoC vendor may be different from ${vendor}, introduce a new > >> ${soc_vendor}. If this is not set, the behavior remains unchanged. > >> > >> Cc: Alexander Graf <agraf@suse.de> > >> Signed-off-by: Andreas Färber <afaerber@suse.de> > >> --- > >> include/config_distro_bootcmd.h | 18 +++++++++++++++++- > >> 1 file changed, 17 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h > >> index eadec2e..8f14457 100644 > >> --- a/include/config_distro_bootcmd.h > >> +++ b/include/config_distro_bootcmd.h > >> @@ -113,6 +113,15 @@ > >> #define BOOTENV_EFI_SET_FDTFILE_FALLBACK > >> #endif > >> > >> +#if defined(CONFIG_ARM64) > >> +#define BOOTENV_EFI_SET_FDTFILE_VENDOR \ > >> + "if test -n \"${soc_vendor}\"; then " \ > >> + "setenv efi_dtb_vendor_prefix ${soc_vendor}/; " \ > >> + "fi; " > >> +#else > > > > OK. Looking at the current Linux kernel, it's a given that for arm64 a > > DTB will be in a subdirectory, always. Perhaps we should fix this in > > Kconfig and have... CONFIG_FDTFILE_VENDOR_DIRECTORY and set this > > correctly per vendor (and yes, eventually there will be some "fun" as > > NXP boards will sometimes be in freescale/ and sometimes nxp/ so maybe > > try and futureproof ourselves so that we loop over this variable) ? > > Yes, it's a given that in the kernel the .dts is always in a vendor > subdirectory - as listed in an earlier thread also for mips etc. Right, OK. And it will be extra fun later on when arm and powerpc (probably) switch over. > And yes, it's also true that due to our scripted openSUSE packaging we > therefore install .dtb to /boot/dtb-foo/vendor/ - but that's not > guaranteed for other distros and users, is it? Therefore I include "" in > the loop further down. It feels unlikely that other distributions would unroll the directories that exist already upstream. > Anyway, adding a config option sounds good. Will you look into that > yourself or should I do that as part of this series? Please re-work the series around this idea. > I had considered the ambiguous vendor case and am iterating over > $efi_dtb_vendor_prefix in a list of vendors already below, only the > addition of the trailing slash above is limiting us to one currently. Do > you have a concrete suggestion of how to make that more flexible? The more I think about it, the less worried I am about it, actually. Lets just assume that we will have the right vendor set and if it turns out that a 'git mv' is done for say freescale later on down the line, we'll worry about it then. Unless someone knows about how it might go and wants to speak up now? York?
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index eadec2e..8f14457 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -113,6 +113,15 @@ #define BOOTENV_EFI_SET_FDTFILE_FALLBACK #endif +#if defined(CONFIG_ARM64) +#define BOOTENV_EFI_SET_FDTFILE_VENDOR \ + "if test -n \"${soc_vendor}\"; then " \ + "setenv efi_dtb_vendor_prefix ${soc_vendor}/; " \ + "fi; " +#else +#define BOOTENV_EFI_SET_FDTFILE_VENDOR +#endif + #define BOOTENV_SHARED_EFI \ "boot_efi_binary=" \ "load ${devtype} ${devnum}:${distro_bootpart} " \ @@ -125,18 +134,25 @@ \ "load_efi_dtb=" \ "load ${devtype} ${devnum}:${efi_bootpart} ${fdt_addr_r} "\ - "${prefix}${dtb_prefix}${efi_fdtfile}\0" \ + "${prefix}${dtb_prefix}${dtb_vendor_prefix}" \ + "${efi_fdtfile}\0" \ \ "efi_dtb_prefixes=\"\" dtb/ dtb/current/\0" \ "scan_dev_for_efi_fdt=" \ "for prefix in ${boot_prefixes}; do " \ "for dtb_prefix in ${efi_dtb_prefixes}; do " \ + BOOTENV_EFI_SET_FDTFILE_VENDOR \ + "for dtb_vendor_prefix in \"\" " \ + "${efi_dtb_vendor_prefix}; do " \ "if test -e ${devtype} " \ "${devnum}:${efi_bootpart} " \ "${prefix}${dtb_prefix}" \ + "${dtb_vendor_prefix}" \ "${efi_fdtfile}; then " \ "run load_efi_dtb; " \ "fi; " \ + "done; " \ + "setenv efi_dtb_vendor_prefix; " \ "done; " \ "done\0" \ "scan_dev_for_efi=" \
On arm64 Linux device trees are organized by SoC vendor. Therefore we need to search the vendor subdirectory as well. Since the SoC vendor may be different from ${vendor}, introduce a new ${soc_vendor}. If this is not set, the behavior remains unchanged. Cc: Alexander Graf <agraf@suse.de> Signed-off-by: Andreas Färber <afaerber@suse.de> --- include/config_distro_bootcmd.h | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)