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?
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.
> 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).
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...
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
