[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Ang, Chee Hong]

> On 2/20/20 6:54 PM, Ang, Chee Hong wrote:
> >> On 2/20/20 3:02 AM, Ang, Chee Hong wrote:
> >> [...]
> >>>>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF)
> >>>>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void
> >>>>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void
> >>>>> +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32
> >>>>> +val); #else
> >>>>> +#define socfpga_secure_reg_read32	readl
> >>>>> +#define socfpga_secure_reg_write32	writel
> >>>>> +#define socfpga_secure_reg_update32	clrsetbits_le32
> >>>>> +#endif
> >>>>
> >>>> I think I don't understand how this is supposed to work. Would
> >>>> every place in U- Boot have to be patched to call these functions now ?
> >>>
> >>> Not every register access need this. Only those accessing registers
> >>> in secure zone such as 'System Manager' registers need to call this.
> >>> It's basically determine whether the driver should issue SMC/PSCI
> >>> call if it's running in EL2 (non-secure) or access the registers
> >>> directly by simply using
> >> readl/writel and etc if it's running in EL3 (secure).
> >>> Accessing those registers in secure zone in non-secure mode (EL2)
> >>> will cause
> >> SError exception.
> >>> So we can determine this behaviour in compile time:
> >>> SPL always running in EL3. So it just simply fallback to use
> >> readl/writel/clrsetbits_le32.
> >>>
> >>> For U-Boot proper (SSBL), there are 2 scenarios:
> >>> 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It
> >>> implies that U-Boot proper will be running in EL2 (non-secure), then
> >>> it will use
> >> SMC/PSCI calls to access the secure registers.
> >>>
> >>> 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will
> >>> be running in EL3 which will fall back to simply using the direct
> >>> access functions
> >> (readl/writel and etc).
> >>
> >> I would expect the standard IO accessors would or should handle this stuff ?
> > Standard IO accessors are just general memory read/write functions
> > designed to be compatible with general hardware platforms. Not all
> > platforms have secure/non-secure hardware zones. I don't think they should
> handle this.
> >
> > If it's running in EL3 (secure mode) the standard I/O accessors will
> > work just fine because
> > EL3 can access to all secure/non-secure zones. In the header file, you
> > can see the secure I/O accessors will be replaced by the standard I/O
> > accessors if it's built for SPL and U-Boot proper without ATF. Because both are
> running in EL3 (secure).
> >
> > If ATF is enabled, SPL will be still running in EL3 but U-Boot proper
> > will be running in
> > EL2 (non-secure). If any code accessing those secure zones without
> > going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError'
> exceptions and crash the U-Boot.
> 
> Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever access
> secure zones in the first place ?
SPL and U-Boot reuse the same drivers code. It runs without issue in SPL (EL3) but
it crashes if running the same driver code in EL2 which access some secure zone registers.
The System Manager (secure zone) contains some register which control the behaviours of
EMAC/SDMMC and etc. Clock manager driver rely on those registers in System Manager as well
for retrieving clock information. These clock/EMAC/SDMMC drivers access the System Manager
(secure zone).
> 
> And if that's really necessary, should the ATF really provide secure-mode register
> access primitives or should it provide some more high-level interface instead ?
I see your point. I should have mentioned to you that these secure-mode register access provided by
ATF actually do more stuffs than just primitive accessors.
ATF only allow certain secure registers required by the non-secure driver to be accessed.
It will check the secure register address and block access if the register address is not allowed
to be accessed by non-secure world (EL2).
So these secure register access provided by ATF is not just simple accessor/delegate which
simply allow access to any secure zone from non-secure world without any restrictions.
I would say the secure register access provided by ATF is a 'middle-level' interface not just
some primitive accessors.

Currently, we have like 20+ secure registers allowed access by drivers running in
non-secure mode (U-Boot proper / Linux).
I don't think we want to define and maintain those high level interfaces for each of those secure
register accesses in ATF and U-Boot.