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

[Ang, Chee Hong]

> On 2/24/20 3:21 AM, Ang, Chee Hong wrote:
> [...]
> 
> >>>>> 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 think something more high level might be a good idea here.
What do you mean by 'more high level' ?
We handle this in SPL (EL3) ?

Since you are the author of this 'drivers/net/dwmac_socfpga.c':
https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/net/dwmac_socfpga.c#L101
https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/dts/socfpga_stratix10.dtsi#L98

Your driver selects the PHY interface (RGMII/RMII and etc) using the following register (part of System Manager):
https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#topic/jng1505406892594.html 

I personally think this PHY interface select for EMACx shouldn't be part of System Manager.
I don't see the security benefits here by making this PHY interface select as 'secure zone' register.

Same applies to DW MMC driver as well:
https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mmc/socfpga_dw_mmc.c#L60

It sets the following register in System Manager (secure zone) to configure the SDMMC clocks:
https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#topic/gil1505406886282.html

Don't you think these things should be part of driver itself as what we are doing now instead
of removing these from drivers and place them in SPL (EL3)?