Re: [PATCH v2 1/2] xen/arm: Add imx8q{m,x} platform glue

[Michal Orzel <michal.orzel@xxxxxxx>]

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

> +#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