From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Goldschmidt Date: Tue, 3 Dec 2019 12:35:27 +0100 Subject: [U-Boot] [PATCH v1 00/20] Enable ARM Trusted Firmware for U-Boot In-Reply-To: References: <1575282321-11597-1-git-send-email-chee.hong.ang@intel.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, Dec 3, 2019 at 2:37 AM Ang, Chee Hong wrote: > > > Am 02.12.2019 um 17:10 schrieb Ang, Chee Hong: > > >> On Mon, Dec 2, 2019 at 4:18 PM Ang, Chee Hong > > >> > > >> wrote: > > >>> > > >>>> On Mon, Dec 2, 2019 at 3:08 PM Ang, Chee Hong > > >>>> > > >>>> wrote: > > >>>>> > > >>>>>> On Mon, Dec 2, 2019 at 2:38 PM Ang, Chee Hong > > >>>>>> > > >>>>>> wrote: > > >>>>>>> > > >>>>>>>> On Mon, Dec 2, 2019 at 11:25 AM > > >> wrote: > > >>>>>>>>> > > >>>>>>>>> From: "Ang, Chee Hong" > > >>>>>>>>> > > >>>>>>>>> New U-boot flow with ARM Trusted Firmware (ATF) support: > > >>>>>>>>> SPL (EL3) -> ATF-BL31 (EL3) -> U-Boot Proper (EL2) -> Linux > > >>>>>>>>> (EL1) > > >>>>>>>> > > >>>>>>>> Adding support for ATF means that using U-Boot on Stratix10 and > > >>>>>>>> Agilex without ATF keeps working, right? > > >>>>>>> ATF is needed in order for Stratix10 and Agilex's U-Boot to work. > > >>>>>> > > >>>>>> Is there a technical requirement for that? > > >>>>> Yes. We are using ATF to provide PSCI services such as system > > >>>>> reset (COLD reset), CPU_ON/CPU_OFF (CPU hotplug in Linux) and > > >>>>> other secure services such as mailbox communications with Secure > > >>>>> Device Manager and accessing the System Manager registers which > > >>>>> are > > >> secure. > > >>>>> Without PSCI services, we are able to boot until U-Boot proper only. > > >>>>> Currently, our U-Boot in mainline doesn't boot to Linux due to > > >>>>> these missing > > >>>> PSCI services. > > >>>>> Another reason is we have another boot flow which is using ATF + UEFI. > > >>>>> So now we are re-using the PSCI services from ATF so that now > > >>>>> U-Boot and UEFI share the same ATF-BL31 image so that we don't > > >>>>> have to > > >>>> reimplement another sets of PSCI services for U-Boot again. > > >>>>> This will greatly reduce our engineering efforts. > > >>>> > > >>>> Hmm, thanks for the explanation. > > >>>> > > >>>> I don't really think I can review this, given the lack of knowledge > > >>>> about that PSCI stuff. > > >>> I believe you can review it. > > >>> Have you briefly gone through the patches ? It has nothing to do > > >>> with the PSCI > > >> stuff. > > >>> It just call the invoke_smc() function to call the ATF's PSCI > > >>> functions. Those PSCI functions in ATF will do the rest. > > >> > > >> No, not yet. But see below. > > >> > > >>>> > > >>>> I must say it seems strange to me that U-Boot would have to rely on > > >>>> ATF > > >> though. > > >>>> How do other platforms implement this? Shouldn't PSCI be generic or > > >>>> is it really platform specific? If it's generic, isn't that a > > >>>> dupliation of code if you implement e.g. a reset driver for > > >>>> Stratix10 but call > > >> into PSCI? > > >>> It's not strange at all. A lot of U-Boot users already using this > > >>> boot flow (ATF + > > >> U-Boot). > > >> > > >> Just because other boards do this doesn't mean it's not strange. > > >> Wasn't there some kind of discussion around that PSCI stuff to make it > > available from U-Boot? > > >> What's wrong with that way? > > > Our downstream U-Boot branch is already implemented the PSCI stuffs in U- > > Boot. > > > I tried to upstream my PSCI code: > > > https://lists.denx.de/pipermail/u-boot/2019-May/368822.html > > > > > > Marek pointed me to this thread: > > > https://www.mail-archive.com/u-boot at lists.denx.de/msg319458.html > > > > > > He had a point. He suggested maybe we can implement the PSCI stuffs in > > > SPL/TPL. I took a look at the U-Boot code and found out ATF is already well > > supported. Why don't we just use the PSCI code from ATF rather than re- > > implementing the PSCI code again in SPL/TPL. > > > It will save our effort to maintain two PSCI code in U-Boot and ATF while we > > already have the ATF PSCI implementation to support UEFI. > > > > It seems to me we do have working code in U-Boot, what's missing is "only" to > > turn it ino PSCI? > Existing PSCI framework in U-Boot provide a way for us to turn the code into a PSCI handler > by just adding a '__secure' keyword before the function name. See: > https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/mach-socfpga/mailbox_s10.c > > Below is one of the functions that has 2 versions. One 'live' in a normal code section and another > one will be relocated to "__secure" section (for PSCI). You can see that 2 same functions > are duplicated for normal code section and PSCI section. > > int mbox_send_cmd(u8 id, u32 cmd, u8 is_indirect, u32 len, u32 *arg, > u8 urgent, u32 *resp_buf_len, u32 *resp_buf) > { > return mbox_send_cmd_common(id, cmd, is_indirect, len, arg, urgent, > resp_buf_len, resp_buf); > } > > int __secure mbox_send_cmd_psci(u8 id, u32 cmd, u8 is_indirect, u32 len, > u32 *arg, u8 urgent, u32 *resp_buf_len, > u32 *resp_buf) > { > return mbox_send_cmd_common(id, cmd, is_indirect, len, arg, urgent, > resp_buf_len, resp_buf); > } > > Those functions that are needed by PSCI runtime need to be duplicated for "__secure" section. > U-Boot Proper will copy and relocate the PSCI code in "__secure" section to a location before booting > Linux whereby they can be called by Linux. Using the PSCI framework, U-Boot proper is not able to call > any PSCI functions because PSCI code is not available until U-Boot proper ready to boot Linux. > So that's the reason we need to have 2 sets of code in U-Boot. One for SPL/U-Boot and another one for > PSCI section which is used by Linux. > Currently we have 2 implementations for FPGA reconfiguration driver in our downstream branch. > One for SPL/U-Boot and another one for Linux (PSCI). FPGA reconfiguration driver for U-Boot is already > upstreamed but I don't think I can get the FPGA reconfiguration for the PSCI part upstreamed. > They are 2 sets of different code for the same purpose. But that is what we have done in downstream > to make sure we can support Linux. > > BTW, we are going to get rid of those duplicated code for PSCI after we switch to ATF boot flow. I think we have already discussed why that style is bad and unstable. The correct thing to do would be to compile an SPL style binary from the U-Boot sources that can replace ATF-BL31, not this messy __secure thing. I can see others (rockchip, TI, NXP?) might in part rely on ATF as well, but speaking for socfpga, if you must insist on using ATF, I would be happy if you could do it in a way that does not reduce existing functionality in U-Boot. > > > > And given U-Boot aims to support UEFI (kind of?), I'd rather argue: why do you > > need ATF at all? > > No, U-Boot does not aim to support UEFI. We have 2 boot flows that don't mix: Really? Or do you mean you don't aim to support EFI boot using U-Boot? I don't know that (U)EFI stuff too well, yet, but I was under the impression that Heinrich et. al. do want U-Boot to support UEFI? > > 1) U-Boot -> ATF-BL31 -> U-Boot Proper -> Linux > > 2) ATF-BL2 -> ATF-BL31 -> UEFI -> Other OSes or Linux > > These two boot flows now share the same code base (ATF-BL31). > > > > Indeed, having the same code in both seems like double effort for maintenance. > > > > >> > > >> In my opinion, you're reducing functionality in U-Boot by making ATF > > >> a requirement. > > >> > > >> And by saying "I can't review this", I mean this looks like a > > >> questionable way to me and I'm not the one to say if a U-Boot board should > > go this way or not. > > > I understand. Btw, who should I include to review this ? > > >> > > >>> Yes. PSCI is a generic software interface which encapsulate the > > >>> platform > > >> specific code. > > >>> Let me give you a good example in one of your sysreset driver you > > >>> have > > >> implemented for S10. > > >>> > > >>> #include > > >>> #include > > >>> #include > > >>> -#include > > >>> > > >>> static int socfpga_sysreset_request(struct udevice *dev, > > >>> enum sysreset_t type) { > > >>> - puts("Mailbox: Issuing mailbox cmd REBOOT_HPS\n"); > > >>> - mbox_reset_cold(); > > >>> + psci_system_reset(); > > > > And coming back on this, the sysreset driver won't work in SPL any more, right? > You brought a very good point. See my comment at the bottom. > > > > >> > > >> So this is not an socfgpa_s10 specific driver any more, right? > This driver code can be renamed to more generic name such as socfpga_soc64.c. > So that it can be shared by both Stratix10 and Agilex. > > >> > > >>> return -EINPROGRESS; > > >>> } > > >>> > > >>> Above is the changes in one of my patchsets, the sysreset driver for > > >>> S10 used to call mbox_reset_cold() to send mailbox message to Secure > > >>> Device Manager > > >>> (SDM) to trigger COLD reset. > > >>> Calling 'mbox_reset_cold()' means you are calling platform specific > > >>> code in the sysreset driver to perform COLD reset. What if method to > > >>> trigger > > >> COLD reset is changed in new platform ? > > >>> We have to change the sysreset driver code to support another new > > platform. > > >>> If this function call is replaced with "psci_system_reset()", we > > >>> don't have to change the code at all because all the platform > > >>> specific code for COLD reset is now reside in ATF because this > > >>> function just invoke the PSCI function provided by ATF. You just > > >>> have to replace the ATF binary with the new implementation for the > > >>> new platform. We can re-use this sysreset driver for any platform > > >>> that support ATF. In fact, it makes our U-Boot driver code more > > >>> 'generic' because PSCI interface stay the same. BTW, Linux also call > > >>> PSCI functions to perform COLD reset. By > > >> using ATF, U-Boot and Linux share the same COLD reset service provided by > > ATF. > > >> It actually reduce code duplication. > > >> > > >> What I meant was code duplication inside the U-Boot tree (having one > > >> driver for each arch but in effect all those drivers will call into the same psci > > function). > > > Can different archs share the same driver if the driver code is common to > > those platforms ? > > > > I don't know why not. However, you then need a different way to select this > > driver: you clearly cannot use DT compatibles as this DT entry does not in any > > way stand for what you make the driver binding to it execute. > > > > Instead, I would think of a way to make your PSCI-aware U-Boot proper use a > > generic PSCI-reset driver instead of the one matching the devicetree. And then > > keep in mind you still need the DT-matching driver in SPL. Thinking about it, > > having a driver in SPL you don't use in U-Boot proper is probably not done, yet, > > as well. > I don't have any problem with this approach (PSCI-reset driver) but it is very easy to support SPL and U-Boot proper > in the same driver by just checking the current exception level. Please take a look at the code below. > > #include > #include > #include > #include > > static int socfpga_sysreset_request(struct udevice *dev, > enum sysreset_t type) { > - puts("Mailbox: Issuing mailbox cmd REBOOT_HPS\n"); > + If (current_el() == 3) Hard-coding the EL here seems quite a hack? > + mbox_reset_cold(); > + else > + psci_system_reset(); > return -EINPROGRESS; > } > > We can make the sysreset driver compatible in SPL and U-Boot proper by just checking the current exception level. > If it's EL3 (secure), we knew SPL is running and otherwise U-Boot proper (EL2, non-secure) is running. So you compile all the PSCI stuff into SPL although never using it? I'd still prefer to have DT-compat matched drivers implementing the hardware access. Then you can instantiate different drivers in U-Boot proper if you want to use PSCI, not the hardware. Having DT-compat matched drivers do something completely different (issuing PSCI calls instead of accessing the hardware they matched on) seems wrong. Regards, Simon > Or we can make a small generic function like below and call it from sysreset driver code: > > void soc64_cold_reset(void) > { > If (current_el() == 3) > mbox_reset_cold(); > else > psci_system_reset(); > } > > > > > > > > >> What you're doing is to move this code from U-Boot open U-Boot > > >> sources to possibly closed source ATF sources. But we've already had > > >> that discussion, I think... > > > Our PSCI implementation in ATF is open source: > > > https://github.com/ARM-software/arm-trusted-firmware/tree/master/plat/ > > > intel/soc > > > > Well, open source... Without implying anything: it's BSD, so it's open source as > > long as Intel wants it to be open source and nothing prevents the next manager > > from keeping additions or even bugfixes closed source. > > For whatever reasons might come. > > > > > > Regards, > > Simon > > > > > > > >> > > >> Regards, > > >> Simon > > >> > > >>> > > >>> I think you are aware of we are working to move the mailbox driver > > >>> code away > > >> from arch to drivers. > > >>> You will see that a lot of those mailbox functions will be removed > > >>> from the > > >> mailbox driver. > > >>> One of them is 'mbox_reset_cold()' which you called in sysreset driver for > > S10. > > >>> > > >>>> Regards, > > >>>> Simon > > >>>> > > >>>>>> > > >>>>>> Regard, > > >>>>>> Simon > > >>>>>> > > >>>>>>>>> > > >>>>>>>>> SPL loads the u-boot.itb which consist of: > > >>>>>>>>> 1) u-boot-nodtb.bin (U-Boot Proper image) > > >>>>>>>>> 2) u-boot.dtb (U-Boot Proper DTB) > > >>>>>>>>> 3) bl31.bin (ATF-BL31 image) > > >>>>>>>>> > > >>>>>>>>> Supported Platform: Intel SoCFPGA 64bits (Stratix10 & > > >>>>>>>>> Agilex) > > >>>>>>>>> > > >>>>>>>>> Now, U-Boot Proper is running in non-secure mode (EL2), it > > >>>>>>>>> invokes SMC/PSCI calls provided by ATF to perform COLD reset, > > >>>>>>>>> System Manager register accesses and mailbox communications > > >>>>>>>>> with Secure Device Manager (SDM). > > >>>>>>>>> > > >>>>>>>>> Steps to build the U-Boot with ATF support: > > >>>>>>>>> 1) Build U-Boot > > >>>>>>>>> 2) Build ATF BL31 > > >>>>>>>>> 3) Copy ATF BL31 binary image into U-Boot's root folder > > >>>>>>>>> 4) "make u-boot.itb" to generate u-boot.itb > > >>>>>>>>> > > >>>>>>>>> These patchsets have dependency on: > > >>>>>>>>> [U-Boot,v8,00/19] Add Intel Agilex SoC support: > > >>>>>>>>> https://patchwork.ozlabs.org/cover/1201373/ > > >>>>>>>>> > > >>>>>>>>> Chee Hong Ang (19): > > >>>>>>>>> arm: socfpga: add fit source file for pack itb with ATF > > >>>>>>>>> arm: socfpga: Add function for checking description from > > >>>>>>>>> FIT > > >> image > > >>>>>>>>> arm: socfpga: Load FIT image with ATF support > > >>>>>>>>> arm: socfpga: Override 'lowlevel_init' to support ATF > > >>>>>>>>> configs: socfpga: Enable FIT image loading with ATF support > > >>>>>>>>> arm: socfpga: Disable "spin-table" method for booting Linux > > >>>>>>>>> arm: socfpga: Add SMC helper function for Intel SOCFPGA (64bits) > > >>>>>>>>> arm: socfpga: Define SMC function identifiers for PSCI SiP services > > >>>>>>>>> arm: socfpga: Add secure register access helper functions for SoC > > >>>>>>>>> 64bits > > >>>>>>>>> arm: socfpga: Secure register access for clock manager (SoC 64bits) > > >>>>>>>>> arm: socfpga: Secure register access in PHY mode setup > > >>>>>>>>> arm: socfpga: Secure register access for reading PLL frequency > > >>>>>>>>> mmc: dwmmc: socfpga: Secure register access in MMC driver > > >>>>>>>>> net: designware: socfpga: Secure register access in MAC driver > > >>>>>>>>> arm: socfpga: Secure register access in Reset Manager driver > > >>>>>>>>> arm: socfpga: stratix10: Initialize timer in SPL > > >>>>>>>>> arm: socfpga: stratix10: Refactor FPGA reconfig driver to > > >>>>>>>>> support > > >> ATF > > >>>>>>>>> arm: socfpga: Bridge reset now invokes SMC calls to query > > >>>>>>>>> FPGA > > >>>> config > > >>>>>>>>> status > > >>>>>>>>> sysreset: socfpga: Invoke PSCI call for COLD reset > > >>>>>>>>> > > >>>>>>>>> Dalon Westergreen (1): > > >>>>>>>>> configs: stratix10: Remove CONFIG_OF_EMBED > > >>>>>>>> > > >>>>>>>> This one is included in another series already: > > >>>>>>>> https://patchwork.ozlabs.org/user/todo/uboot/?series=132976 > > >>>>>>>> > > >>>>>>>> Does this mean that one will be abandonen? > > >>>>>>>> So the combined hex output part of that series is not required > > >>>>>>>> any > > >> more? > > >>>>>>>> > > >>>>>>>> Regards, > > >>>>>>>> Simon > > >>>>>>>> > > >>>>>>>>> > > >>>>>>>>> arch/arm/mach-socfpga/Kconfig | 2 - > > >>>>>>>>> arch/arm/mach-socfpga/Makefile | 4 + > > >>>>>>>>> arch/arm/mach-socfpga/board.c | 10 + > > >>>>>>>>> arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +- > > >>>>>>>>> arch/arm/mach-socfpga/clock_manager_s10.c | 5 +- > > >>>>>>>>> arch/arm/mach-socfpga/include/mach/misc.h | 3 + > > >>>>>>>>> .../mach-socfpga/include/mach/secure_reg_helper.h | 20 ++ > > >>>>>>>>> arch/arm/mach-socfpga/lowlevel_init.S | 48 +++ > > >>>>>>>>> arch/arm/mach-socfpga/misc_s10.c | 47 ++- > > >>>>>>>>> arch/arm/mach-socfpga/reset_manager_s10.c | 31 +- > > >>>>>>>>> arch/arm/mach-socfpga/secure_reg_helper.c | 67 ++++ > > >>>>>>>>> arch/arm/mach-socfpga/timer_s10.c | 3 +- > > >>>>>>>>> arch/arm/mach-socfpga/wrap_pll_config_s10.c | 9 +- > > >>>>>>>>> board/altera/soc64/its/fit_spl_atf.its | 51 +++ > > >>>>>>>>> configs/socfpga_agilex_defconfig | 8 +- > > >>>>>>>>> configs/socfpga_stratix10_defconfig | 9 +- > > >>>>>>>>> drivers/fpga/stratix10.c | 261 ++++---------- > > >>>>>>>>> drivers/mmc/socfpga_dw_mmc.c | 7 +- > > >>>>>>>>> drivers/net/dwmac_socfpga.c | 5 +- > > >>>>>>>>> drivers/sysreset/sysreset_socfpga_s10.c | 4 +- > > >>>>>>>>> include/configs/socfpga_soc64_common.h | 2 +- > > >>>>>>>>> include/linux/intel-smc.h | 374 > > >> +++++++++++++++++++++ > > >>>>>>>>> 22 files changed, 732 insertions(+), 243 deletions(-) create > > >>>>>>>>> mode > > >>>>>>>>> 100644 > > >>>>>>>>> arch/arm/mach-socfpga/include/mach/secure_reg_helper.h > > >>>>>>>>> create mode 100644 arch/arm/mach-socfpga/lowlevel_init.S > > >>>>>>>>> create mode 100644 > > >>>>>>>>> arch/arm/mach-socfpga/secure_reg_helper.c > > >>>>>>>>> create mode 100644 board/altera/soc64/its/fit_spl_atf.its > > >>>>>>>>> create mode 100644 include/linux/intel-smc.h > > >>>>>>>>> > > >>>>>>>>> -- > > >>>>>>>>> 2.7.4 > > >>>>>>>>> >