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.