Hi, On 14/02/2024 17:06, John Ernberg wrote: > > > When using Linux for dom0 there are a bunch of drivers that need to do SMC > SIP calls into the firmware to enable certain hardware bits like the > watchdog. > > Provide a basic platform glue that implements the needed SMC forwarding. > > The format of these calls are as follows: > - reg 0: service ID > - reg 1: function ID > remaining regs: args > > For now we only allow Dom0 to make these calls as they are all managing > hardware. There is no specification for these SIP calls, the IDs and names > have been extracted from the upstream linux kernel and the vendor kernel. > > Most of the SIP calls are only available for the iMX8M series of SoCs, so > they are easy to reject and they need to be revisited when iMX8M series > support is added. Given that you named your driver *qm and you search for dt compatible *qm, *qxp, does it really make sense to add SIP calls for iMX8M?
> > From the other calls we can reject CPUFREQ because Dom0 cannot make an > informed decision regarding CPU frequency scaling, WAKEUP_SRC is to wake > up from suspend, which Xen doesn't support at this time. > > This leaves the TIME SIP and the OTP_WRITE SIP, which for now are allowed > to Dom0. > > NOTE: This code is based on code found in NXP Xen tree located here: > https://github.com/nxp-imx/imx-xen/blob/lf-5.10.y_4.13/xen/arch/arm/platforms/imx8qm.c > > Signed-off-by: Peng Fan <peng....@nxp.com> > [jernberg: Add SIP call filtering] > Signed-off-by: John Ernberg <john.ernb...@actia.se> > > --- > > v2: > - Reword the commit message to be a bit clearer > - Include the link previously added as a context note to the commit message > (Julien Grall) > - Add Pengs signed off (Julien Grall, Peng Fan) > - Add basic SIP call filter (Julien Grall) > - Expand the commit message a whole bunch because of the changes to the code > --- > xen/arch/arm/platforms/Makefile | 1 + > xen/arch/arm/platforms/imx8qm.c | 143 ++++++++++++++++++++++++++++++++ > 2 files changed, 144 insertions(+) > create mode 100644 xen/arch/arm/platforms/imx8qm.c > > diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile > index 8632f4115f..bec6e55d1f 100644 > --- a/xen/arch/arm/platforms/Makefile > +++ b/xen/arch/arm/platforms/Makefile > @@ -9,5 +9,6 @@ obj-$(CONFIG_ALL_PLAT) += sunxi.o > obj-$(CONFIG_ALL64_PLAT) += thunderx.o > obj-$(CONFIG_ALL64_PLAT) += xgene-storm.o > obj-$(CONFIG_ALL64_PLAT) += brcm-raspberry-pi.o > +obj-$(CONFIG_ALL64_PLAT) += imx8qm.o > obj-$(CONFIG_MPSOC_PLATFORM) += xilinx-zynqmp.o > obj-$(CONFIG_MPSOC_PLATFORM) += xilinx-zynqmp-eemi.o > diff --git a/xen/arch/arm/platforms/imx8qm.c b/xen/arch/arm/platforms/imx8qm.c > new file mode 100644 > index 0000000000..4515c75935 > --- /dev/null > +++ b/xen/arch/arm/platforms/imx8qm.c > @@ -0,0 +1,143 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * xen/arch/arm/platforms/imx8qm.c > + * > + * i.MX 8QM setup > + * > + * Copyright (c) 2016 Freescale Inc. > + * Copyright 2018-2019 NXP > + * > + * > + * Peng Fan <peng....@nxp.com> > + */ > + > +#include <xen/sched.h> > +#include <asm/platform.h> > +#include <asm/smccc.h> > + > +static const char * const imx8qm_dt_compat[] __initconst = > +{ > + "fsl,imx8qm", > + "fsl,imx8qxp", > + NULL > +}; > + > +#define IMX_SIP_GPC 0xC2000000 NIT: A matter of personal opinion but all the Arm64 SIP services starts with 0xc2000000, so you could just define a function id i.e. 1, 2, ... and use a macro similar to EEMI_FID(fid). This is just a suggestion. > +#define IMX_SIP_CPUFREQ 0xC2000001 > +#define IMX_SIP_TIME 0xC2000002 > +#define IMX_SIP_DDR_DVFS 0xC2000004 > +#define IMX_SIP_SRC 0xC2000005 > +#define IMX_SIP_GET_SOC_INFO 0xC2000006 > +#define IMX_SIP_NOC 0xC2000008 > +#define IMX_SIP_WAKEUP_SRC 0xC2000009 > +#define IMX_SIP_OTP_WRITE 0xC200000B Looking at ATF, for QM,QXP there are also: BUILDINFO 0xC2000003 OTP_READ 0xC200000A SET_TEMP 0xC200000C > + > +#define IMX_SIP_TIME_F_RTC_SET_TIME 0x00 > +#define IMX_SIP_TIME_F_WDOG_START 0x01 > +#define IMX_SIP_TIME_F_WDOG_STOP 0x02 > +#define IMX_SIP_TIME_F_WDOG_SET_ACT 0x03 > +#define IMX_SIP_TIME_F_WDOG_PING 0x04 > +#define IMX_SIP_TIME_F_WDOG_SET_TIMEOUT 0x05 > +#define IMX_SIP_TIME_F_WDOG_GET_STAT 0x06 > +#define IMX_SIP_TIME_F_WDOG_SET_PRETIME 0x07 > + > +static bool imx8qm_is_sip_time_call_ok(uint32_t function_id) > +{ > + switch ( function_id ) > + { > + case IMX_SIP_TIME_F_RTC_SET_TIME: > + return true; > + case IMX_SIP_TIME_F_WDOG_START: > + case IMX_SIP_TIME_F_WDOG_STOP: > + case IMX_SIP_TIME_F_WDOG_SET_ACT: > + case IMX_SIP_TIME_F_WDOG_PING: > + case IMX_SIP_TIME_F_WDOG_SET_TIMEOUT: > + case IMX_SIP_TIME_F_WDOG_GET_STAT: > + case IMX_SIP_TIME_F_WDOG_SET_PRETIME: > + return true; > + default: > + printk(XENLOG_WARNING "imx8qm: smc: time: Unknown function id %x\n", > function_id); 80 chars limit, move argument list to the next line > + return false; > + } > +} > + > +static bool imx8qm_smc(struct cpu_user_regs *regs) > +{ > + uint32_t service_id = get_user_reg(regs, 0); In SMCCC naming convention, W0 is called function identifier. Instead you call X1 function_id which is a bit misleading. > + uint32_t function_id = get_user_reg(regs, 1); > + struct arm_smccc_res res; > + > + if ( !cpus_have_const_cap(ARM_SMCCC_1_1) ) > + { > + printk_once(XENLOG_WARNING "no SMCCC 1.1 support. Disabling firmware > calls\n"); 80 chars limit, move string to the next line > + > + return false; > + } > + > + /* Only hardware domain may use the SIP calls */ > + if ( !is_hardware_domain(current->domain) ) > + { > + gprintk(XENLOG_WARNING, "imx8qm: smc: No access\n"); > + return false; > + } > + > + switch ( service_id ) > + { > + case IMX_SIP_GPC: /* Only available on imx8m series */ If we decide to keep iMX8M FIDs, I think adding comments on top of a case would result in a more readable code. > + return false; > + case IMX_SIP_CPUFREQ: /* Dom0 can't take any informed descision here */ > + return false; > + case IMX_SIP_TIME: > + if ( imx8qm_is_sip_time_call_ok(function_id) ) > + goto allow_call; > + return false; > + case IMX_SIP_DDR_DVFS: /* Only available on imx8m series */ > + return false; > + case IMX_SIP_SRC: /* Only available on imx8m series */ > + return false; > + case IMX_SIP_GET_SOC_INFO: /* Only available on imx8m series */ > + return false; > + case IMX_SIP_NOC: /* Only available on imx8m series */ > + return false; > + case IMX_SIP_WAKEUP_SRC: /* Xen doesn't have suspend support */ > + return false; > + case IMX_SIP_OTP_WRITE: > + /* function_id is the fuse number, no sensible check possible */ > + goto allow_call; > + default: > + printk(XENLOG_WARNING "imx8qm: smc: Unknown service id %x\n", > service_id); 80 chars limit, move argument list to the next line > + return false; > + } > + > +allow_call: labels need to be indented with one space > + arm_smccc_1_1_smc(service_id, > + function_id, > + get_user_reg(regs, 2), > + get_user_reg(regs, 3), > + get_user_reg(regs, 4), > + get_user_reg(regs, 5), > + get_user_reg(regs, 6), > + get_user_reg(regs, 7), > + &res); > + > + set_user_reg(regs, 0, res.a0); > + set_user_reg(regs, 1, res.a1); > + set_user_reg(regs, 2, res.a2); > + set_user_reg(regs, 3, res.a3); > + > + return true; > +} > + > +PLATFORM_START(imx8qm, "i.MX 8") Shouldn't it be i.MX 8Q{M,XP} ? ~Michal
