[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 1/2] xen/arm: Add imx8q{m,x} platform glue
Hi Michal, On 2/15/24 10:02, Michal Orzel wrote: > 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? The idea was to be explicit about why it makes no sense to support those SIPs on a qm/qxp implementation. I can take it out if that's not argument enough to have them. > >> >> 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.fan@xxxxxxx> >> [jernberg: Add SIP call filtering] >> Signed-off-by: John Ernberg <john.ernberg@xxxxxxxx> >> >> --- >> >> 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.fan@xxxxxxx> >> + */ >> + >> +#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. Ack. > >> +#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 I can't find them in the current Linux code, perhaps deprecated. I can add them and deny them if it makes sense. > >> + >> +#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 Ack. I forgot to check the coding standard on line length. > >> + 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. Ack. Will use function_id and subfunction_id instead. > >> + 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 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"); >> + 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 Ack > >> + return false; >> + } >> + >> +allow_call: > labels need to be indented with one space Ack. Missed that clause in the indentation chapter. > >> + 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} ? Ack. > > ~Michal Thanks! // John Ernberg
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |