Hi Michel, Apologies for the slow reply.
On 4/2/24 9:24 AM, Michal Orzel wrote: > Hi John, > > On 28/03/2024 17:34, 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: function ID >> - reg 1: subfunction ID (when there's a subfunction) >> 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. > I just realized that you pinged me in v2 for clarification, sorry I missed > that. > I still believe we shouldn't list FIDs that are meant for IMX8M, given that > the driver is written for IMX8QM/QXP. Ack. Will take them all out and only add the known required set. > >> >> 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, OTP SIPs, BUILDINFO SIP and TEMP ALARM 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> >> >> --- >> >> v3: >> - Adhere to style guidelines for line length and label indentation (Michal >> Orzel) >> - Use smccc macros to build the SIP function identifier (Michal Orzel) >> - Adjust platform name to be specific to QM and QXP variants (Michal Orzel) >> - Pick up additional SIPs which may be used by other Linux versions - for >> review purposes >> - Changes to the commit message due to above >> >> 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 | 168 ++++++++++++++++++++++++++++++++ >> 2 files changed, 169 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..0992475474 >> --- /dev/null >> +++ b/xen/arch/arm/platforms/imx8qm.c >> @@ -0,0 +1,168 @@ >> +/* 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_FID(fid) \ >> + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \ >> + ARM_SMCCC_CONV_64, \ >> + ARM_SMCCC_OWNER_SIP, \ >> + fid) > macro parameter should be enclosed within parenthesis Ack. > >> + >> +#define IMX_SIP_F_GPC 0x0000 >> +#define IMX_SIP_F_CPUFREQ 0x0001 >> +#define IMX_SIP_F_TIME 0x0002 >> +#define IMX_SIP_F_BUILDINFO 0x0003 >> +#define IMX_SIP_F_DDR_DVFS 0x0004 >> +#define IMX_SIP_F_SRC 0x0005 >> +#define IMX_SIP_F_GET_SOC_INFO 0x0006 >> +#define IMX_SIP_F_NOC 0x0008 >> +#define IMX_SIP_F_WAKEUP_SRC 0x0009 >> +#define IMX_SIP_F_OTP_READ 0x000A >> +#define IMX_SIP_F_OTP_WRITE 0x000B >> +#define IMX_SIP_F_SET_TEMP_ALARM 0x000C > Is there specific reason for leading zeros? Nope. I'll drop them. > >> + >> +#define IMX_SIP_TIME_SF_RTC_SET_TIME 0x00 >> +#define IMX_SIP_TIME_SF_WDOG_START 0x01 >> +#define IMX_SIP_TIME_SF_WDOG_STOP 0x02 >> +#define IMX_SIP_TIME_SF_WDOG_SET_ACT 0x03 >> +#define IMX_SIP_TIME_SF_WDOG_PING 0x04 >> +#define IMX_SIP_TIME_SF_WDOG_SET_TIMEOUT 0x05 >> +#define IMX_SIP_TIME_SF_WDOG_GET_STAT 0x06 >> +#define IMX_SIP_TIME_SF_WDOG_SET_PRETIME 0x07 >> + >> +static bool imx8qm_is_sip_time_call_ok(uint32_t subfunction_id) >> +{ >> + switch ( subfunction_id ) >> + { >> + case IMX_SIP_TIME_SF_RTC_SET_TIME: >> + return true; >> + case IMX_SIP_TIME_SF_WDOG_START: >> + case IMX_SIP_TIME_SF_WDOG_STOP: >> + case IMX_SIP_TIME_SF_WDOG_SET_ACT: >> + case IMX_SIP_TIME_SF_WDOG_PING: >> + case IMX_SIP_TIME_SF_WDOG_SET_TIMEOUT: >> + case IMX_SIP_TIME_SF_WDOG_GET_STAT: >> + case IMX_SIP_TIME_SF_WDOG_SET_PRETIME: >> + return true; >> + default: >> + printk(XENLOG_WARNING "imx8qm: smc: time: Unknown subfunction id >> %x\n", > gprintk Ack. > >> + subfunction_id); >> + return false; >> + } >> +} >> + >> +static bool imx8qm_smc(struct cpu_user_regs *regs) >> +{ >> + uint32_t function_id = get_user_reg(regs, 0); >> + uint32_t subfunction_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"); > NIT: you can move string within quotes to the next line to avoid exceeding 80 > chars. > Also, all the other messages are prepended with "imx8qm: smc:" so better be > consistent. Ack. > >> + >> + return false; >> + } >> + >> + /* Only hardware domain may use the SIP calls */ >> + if ( !is_hardware_domain(current->domain) ) >> + { >> + gprintk(XENLOG_WARNING, "imx8qm: smc: No access\n"); > Here you use gprintk, but ... > >> + return false; >> + } >> + >> + switch ( function_id ) >> + { >> + /* Only available on imx8m series */ >> + case IMX_SIP_FID(IMX_SIP_F_GPC): >> + return false; >> + case IMX_SIP_FID(IMX_SIP_F_CPUFREQ): >> + /* Hardware domain can't take any informed decision here */ >> + return false; >> + case IMX_SIP_FID(IMX_SIP_F_BUILDINFO): >> + goto allow_call; >> + case IMX_SIP_FID(IMX_SIP_F_TIME): >> + if ( imx8qm_is_sip_time_call_ok(subfunction_id) ) >> + goto allow_call; >> + return false; >> + /* Only available on imx8m series */ >> + case IMX_SIP_FID(IMX_SIP_F_DDR_DVFS): >> + return false; >> + /* Only available on imx8m series */ >> + case IMX_SIP_FID(IMX_SIP_F_SRC): >> + return false; >> + /* Only available on imx8m series */ >> + case IMX_SIP_FID(IMX_SIP_F_GET_SOC_INFO): >> + return false; >> + /* Only available on imx8m series */ >> + case IMX_SIP_FID(IMX_SIP_F_NOC): >> + return false; >> + /* Xen doesn't have suspend support */ >> + case IMX_SIP_FID(IMX_SIP_F_WAKEUP_SRC): >> + return false; >> + case IMX_SIP_FID(IMX_SIP_F_OTP_READ): >> + /* subfunction_id is the fuse number, no sensible check possible */ >> + goto allow_call; >> + case IMX_SIP_FID(IMX_SIP_F_OTP_WRITE): >> + /* subfunction_id is the fuse number, no sensible check possible */ >> + goto allow_call; >> + case IMX_SIP_FID(IMX_SIP_F_SET_TEMP_ALARM): >> + goto allow_call; >> + default: >> + printk(XENLOG_WARNING "imx8qm: smc: Unknown function id %x\n", > ... here you don't. Still getting used gprintk. > >> + function_id); >> + return false; >> + } >> + >> + allow_call: >> + arm_smccc_1_1_smc(function_id, >> + subfunction_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 8Q{M,XP}") >> + .compatible = imx8qm_dt_compat, >> + .smc = imx8qm_smc, >> +PLATFORM_END >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * indent-tabs-mode: nil >> + * End: >> + */ >> -- >> 2.44.0 > > > ~Michal Thanks! // John Ernberg
