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 ? > 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!
