From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Wed, 13 Apr 2016 12:51:29 -0600 Subject: [U-Boot] [PATCH] jetson-tk1: Set fdtfile environment variable In-Reply-To: <570E8D3B.40601@suse.de> References: <1460551699-30796-1-git-send-email-afaerber@suse.de> <570E41C0.1010904@suse.de> <570E665F.405@wwwdotorg.org> <570E6B19.1090402@suse.de> <570E7B17.9030701@wwwdotorg.org> <570E84F3.5080806@suse.de> <570E88BD.1050006@wwwdotorg.org> <570E8D3B.40601@suse.de> Message-ID: <570E9531.10108@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 04/13/2016 12:17 PM, Andreas F?rber wrote: > Am 13.04.2016 um 19:58 schrieb Stephen Warren: >> On 04/13/2016 11:42 AM, Andreas F?rber wrote: >>> Am 13.04.2016 um 19:00 schrieb Stephen Warren: >>>> Anyway, nothing in your benefits-of-EFI statement implies that relying >>>> on $fdtfile being set is correct. That's a new requirement that didn't >>>> exist before. Either the requirement needs to be removed (e.g. using a >>>> default FDT filename such as "${soc}-${board}${boardver}.dts") or only >>>> enabling this functionality on boards that do set $fdtfile, since it >>>> relies on that. >>> >>> $fdtfile needs to be the Linux filename. It does not always follow the >>> same pattern as the U-Boot variables you suggest here. >>> CONFIG_DEFAULT_DEVICE_TREE ".dtb" might work better, and that was my >>> question to you. >> >> That pattern is a good default that at least historically applied to all >> the systems where the distro bootcmds were enabled. Perhaps the set of >> systems using the distro bootcmds has increased now so the default isn't >> always applicable. Boards can set $fdtfile /if/ needed because of that, >> but I don't think should be forced to in all cases where the default >> makes sense. > > I really don't care whether we set fdtfile and use $fdtfile or whether > we insert the filename string directly into the appropriate command > variable... My point is U-Boot via its jetson-tk1_defconfig / .config > knows this (or should know) better than any user. Yes, U-Boot is a good place to put this information. I disagree that it /has/ to come from the defconfig/.config file. > And it seemed to me > that variables were not exactly used sparingly in the distro mechanism > so far, so I don't see why not to populate that variable _if_ we know > what its value needs to be. Do you have any real reason for being > against populating fdtfile at whatever level turns out to be suitable? Yes, $fdtfile} can be auto-populated /at whatver level turns out to be suitable/. I think the only question is what "level" /is/ suitable. > I believe there is no argument that this patch will not be applied. There's a double negative there so I may not be interpreting you correctly. I believe this patch should not be applied. My objections are: a) This only solves the problem for a single board, yet the issue it solves occurs on many/all boards. We need a solution that solves them all. b) Boards should not need to define this value given that it can be calculated automatically. > However I am strongly rejecting your attitude that everything is there > already with variables and that nothing new is needed. Something needs > to be done somewhere - and we need to figure out what exactly and where > for minimum impact to the release. Everything is there. $soc/$board are set by include/env_default.h, with the option for per-board configuration files to override those values if the default CONFIG_SYS_* are incorrect for some reason. Differences between CONFIG_SYS_* and DTB filenames should not be an argument against the default I proposed. So, those values are always available (well, the board must enable CONFIG_ENV_VARS_UBOOT_CONFIG to get them; part of using distro_bootcmd is either enabling that config option or setting $fdtfile some other way). I put effort into ensuring that "${soc}-${board}${boardver}.dtb" was the correct filename for Tegra boards, and I believe this holds true for a variety of non-Tegra boards as well. The proposed default $fdtfile value I mentioned above should be the default, since everything required to construct it is already there. Boards that use distro_bootcmd should ensure that they set those variables correctly to allow that default to work; that's the whole point of those variables. Anyone enabling distro_bootcmd on other platforms hopefully ensured those variables were set correctly. If not, we can surely go back and fix those platforms. Do you have a handle on exactly how many boards don't set those variables in a way that default won't work for them? Can't we fix their $soc/$board values so that the default does work? I believe the calculation of the default/fallback value for $fdtfile does need to happen at runtime. ${boardver} is by definition a runtime variable since it reflects the HW version. As such, we can't simply put the following into e.g. tegra-common.h: #define fdtfile "${soc}-${board}${boardver}.dtb" (or any equivalent of that is actually valid syntax, i.e. using C pre-processor macros) (Right now, I believe we don't actually set $boardver at runtime since for the board where it matters we only support one HW revision at all. However, I don't want to break that runtime capability. If we do, we should just override $board for that board, and delete all references to $boardver. I vaguely recall that some non-Tegra boards might use $boardver though?)