[U-Boot] [PATCH v1 00/20] Enable ARM Trusted Firmware for U-Boot
Ang, Chee Hong
chee.hong.ang at intel.com
Mon Dec 2 17:10:28 CET 2019
> On Mon, Dec 2, 2019 at 4:18 PM Ang, Chee Hong <chee.hong.ang at intel.com>
> wrote:
> >
> > > On Mon, Dec 2, 2019 at 3:08 PM Ang, Chee Hong
> > > <chee.hong.ang at intel.com>
> > > wrote:
> > > >
> > > > > On Mon, Dec 2, 2019 at 2:38 PM Ang, Chee Hong
> > > > > <chee.hong.ang at intel.com>
> > > > > wrote:
> > > > > >
> > > > > > > On Mon, Dec 2, 2019 at 11:25 AM <chee.hong.ang at intel.com>
> wrote:
> > > > > > > >
> > > > > > > > From: "Ang, Chee Hong" <chee.hong.ang at 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
> > > > > > > >
More information about the U-Boot
mailing list