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

[Ang, Chee Hong]

> On 2/21/20 8:06 PM, Ang, Chee Hong wrote:
> >> On 2/21/20 7:01 PM, Ang, Chee Hong wrote:
> >>>> 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).
> >>
> >> Maybe those registers should only be configured by the secure OS, so
> >> maybe those drivers should be fixed ?
> >>
> >>>> 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.
> >>
> >> So socfpga_secure_reg_read32 does not just read a register ? Then the
> >> naming is misleading . And documentation is missing.
> >>
> >>> 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).
> >>
> >> Why don't you configure those secure registers in the secure mode then ?
> >> It seems like that's the purpose of those registers being secure only.
> >>
> >>> 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.
> >>
> >> See above.
> > OK. Then these secure access register should be set up in SPL (EL3).
> > U-Boot drivers shouldn't access them at all because the driver may be
> > running in SPL(EL3) and in U-Boot proper (EL2) too.
> > I can take a look at those drivers accessing secure registers and try
> > to move/decouple those secure access from U-Boot drivers to SPL (EL3)
> > then we no longer need those secure register access functions.
> 
> I think that would be great, no ?
Since the SDMMC/DWMAC drivers read the device tree to configure the behaviour
of the hardware via the secure registers. I think it should still be part of the
driver instead of configuring the hardware in different places. I have proposed
using ATF's high-level APIs to achieve this when the driver is running in EL2.
I have already proposed this in other email threads.
Are you OK with this approach ?
.
> 
> > I am not sure doing this will impact the functionality of the U-Boot
> > driver running in EL2 or not.
> > I can refactor those drivers and try it out.
> 
> Thanks!