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

[Ang, Chee Hong]

Ang, Chee Hong <chee.hong.ang at intel.com<mailto:chee.hong.ang at intel.com>> schrieb am Fr., 28. Feb. 2020, 03:53:
> > 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)?

These 2 drivers that access the system manager:
drivers/mmc/socfpga_dw_mmc.c
- MMC driver access System Manager's 'SDMMC' register to set clock phase
https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/topic/gil1505406886282.html

drivers/net/dwmac_socfpga.c
- MAC driver access System Manager's 'EMACx' registers to set MAC's PHY interface:
https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#topic/jng1505406892594.html

Gen5/Arria10/Stratix10 & Agilex all using these drivers.
They do not cause any issue in Gen5/Arria10 because there is no 'secure zone' in System Manager.
For Stratix10 & Agilex, it has issue with U-Boot proper running in EL2.

I don't think is good idea to separate those System Manager access from these 2 drivers and move them to SPL as they are shared by all SOCFPGA platforms.

I think the proper way to resolve this is we add a new System Manager driver under 'drivers/misc'.
The device type should be 'UCLASS_MISC'.

>I have a pending patchset that adds such a sysmgr driver at least for gen5. I haven't published it yet because the whole >series as one makes gen5 SRAM overlow, but maybe that part can be split out...
>
>Regards,
>Simon

Thanks. I am working on the changes. I will need to test that out to make sure the new sysmgr driver works on all socfpga platforms.
Will let you guys review whether this is a better/cleaner solution to handle the system manager access.

Then we make changes to those drivers (SDMMC/MAC) to access the System Manager through the System Manager driver by using 'misc_read(dev...)' & 'misc_write(dev...)'
The System Manager driver should decide whether to access those 'secure zone' registers directly in EL3 or making SMC call to ATF to access the System Manager if it's running in EL2.

Linux has similar MAC driver running in EL1 (non-secure) accessing System Manager:
https://elixir.bootlin.com/linux/v5.5/source/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c#L302

Linux MAC driver access System Manager via this 'altr,system_manager' driver:
https://elixir.bootlin.com/linux/v5.5/source/drivers/mfd/altera-sysmgr.c#L44
System Manager driver will make SMC/PSCI call to ATF to access the System Manager's registers.