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

[Simon Goldschmidt]

Ang, Chee Hong <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


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.
>