> On Mon, Dec 2, 2019 at 4:18 PM Ang, Chee Hong <chee.hong....@intel.com> > wrote: > > > > > On Mon, Dec 2, 2019 at 3:08 PM Ang, Chee Hong > > > <chee.hong....@intel.com> > > > wrote: > > > > > > > > > On Mon, Dec 2, 2019 at 2:38 PM Ang, Chee Hong > > > > > <chee.hong....@intel.com> > > > > > wrote: > > > > > > > > > > > > > On Mon, Dec 2, 2019 at 11:25 AM <chee.hong....@intel.com> > wrote: > > > > > > > > > > > > > > > > From: "Ang, Chee Hong" <chee.hong....@intel.com> > > > > > > > > > > > > > > > > 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@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. > > 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 <dm.h> > > #include <errno.h> > > #include <sysreset.h> > > -#include <asm/arch/mailbox_s10.h> > > > > 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(); > > So this is not an socfgpa_s10 specific driver any more, right? > > > 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 ? > 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 > > 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 > > > > > > > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
