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

[John Ernberg <john.ernberg@xxxxxxxx>]

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.fan@xxxxxxx>
>> [jernberg: Add SIP call filtering]
>> Signed-off-by: John Ernberg <john.ernberg@xxxxxxxx>
>>
>> ---
>>
>> 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.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_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