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

[Marek Vasut]

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 ?

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 ?