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

[Marek Vasut <marex@denx.de>]

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!