[PATCH 4/4] soc: imx: add SC firmware IPC and APIs
A.s. Dong
aisheng.dong at nxp.com
Wed May 2 11:40:03 PDT 2018
> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Wednesday, May 2, 2018 6:04 PM
> To: A.s. Dong <aisheng.dong at nxp.com>
> Cc: linux-arm-kernel at lists.infradead.org; dongas86 at gmail.com; dl-linux-imx
> <linux-imx at nxp.com>; kernel at pengutronix.de; Fabio Estevam
> <fabio.estevam at nxp.com>; shawnguo at kernel.org
> Subject: Re: [PATCH 4/4] soc: imx: add SC firmware IPC and APIs
>
> On Sat, Apr 28, 2018 at 02:46:16AM +0800, Dong Aisheng wrote:
> > +/* Initialization of the MU code. */
> > +int __init imx8_scu_init(void)
> > +{
> > + struct device_node *np, *mu_np;
> > + struct resource mu_res;
> > + sc_err_t sci_err;
> > + int ret;
> > +
> > + if (!of_machine_is_compatible("fsl,imx8qxp"))
> > + return 0;
> > +
> > + np = of_find_compatible_node(NULL, NULL, "nxp,imx8qxp-scu");
> > + if (!np)
> > + return -ENODEV;
> > +
> > + mu_np = of_parse_phandle(np, "fsl,mu", 0);
> > + if (WARN_ON(!mu_np))
> > + return -EINVAL;
> > +
> > + ret = of_address_to_resource(mu_np, 0, &mu_res);
> > + if (WARN_ON(ret))
> > + return -EINVAL;
> > +
> > + /* we use mu physical address as IPC communication channel ID */
> > + sci_err = sc_ipc_open(&scu_ipc_handle, (sc_ipc_id_t)(mu_res.start));
> > + if (sci_err != SC_ERR_NONE) {
> > + pr_err("Cannot open MU channel to SCU\n");
> > + return sci_err;
> > + };
>
> Introducing private error codes always has the risk of just forwarding them as
> errno codes as done here.
>
Yes, you're right.
We probably could do the same as scpi_to_linux_errno in
drivers/firmware/arm_scpi.c.
However, may can't fix the issue permanently.
> > +
> > + of_node_put(mu_np);
> > +
> > + pr_info("NXP i.MX SCU Initialized (scu_ipc %u)\n", scu_ipc_handle);
> > +
> > + return 0;
> > +}
> > +early_initcall(imx8_scu_init);
>
> This code shows that the separate 'scu' device node shouldn't exist. It is only
> used as a stepping stone to find the 'mu' node. Instead of providing a proper
> driver for the 'mu' node the scu code registers it with its physical address.
> I don't think it makes sense to separate mu and scu code in this way.
>
I agree that may not look quite well.
It mainly because we want to use the MU physical address as a MU ID.
(can't use virtual address as sc_ipc_id_t is u32 defined by SCU firmware.
If you have any better suggestion please let me know.
> > +#define RPC_VER(MSG) ((MSG)->version)
> > +#define RPC_SIZE(MSG) ((MSG)->size)
> > +#define RPC_SVC(MSG) ((MSG)->svc)
> > +#define RPC_FUNC(MSG) ((MSG)->func)
> > +#define RPC_R8(MSG) ((MSG)->func)
> > +#define RPC_I32(MSG, IDX) ((MSG)->DATA.i32[(IDX) / 4])
> > +#define RPC_I16(MSG, IDX) ((MSG)->DATA.i16[(IDX) / 2])
> > +#define RPC_I8(MSG, IDX) ((MSG)->DATA.i8[(IDX)])
> > +#define RPC_U32(MSG, IDX) ((MSG)->DATA.u32[(IDX) / 4])
> > +#define RPC_U16(MSG, IDX) ((MSG)->DATA.u16[(IDX) / 2])
> > +#define RPC_U8(MSG, IDX) ((MSG)->DATA.u8[(IDX)])
>
> These macros only make the code less readable, please drop them. Plain
> m->version is easier to read and forces the reader through less
> indirections. IMO m->DATA.u32[0] = foo; m->DATA.u8[4] = bar is still better
> readable than doing the division by size in macros.
>
I somehow agree with you.
See below...
> > +typedef enum sc_rpc_async_state_e {
> > + SC_RPC_ASYNC_STATE_RD_START = 0,
> > + SC_RPC_ASYNC_STATE_RD_ACTIVE = 1,
> > + SC_RPC_ASYNC_STATE_RD_DONE = 2,
> > + SC_RPC_ASYNC_STATE_WR_START = 3,
> > + SC_RPC_ASYNC_STATE_WR_ACTIVE = 4,
> > + SC_RPC_ASYNC_STATE_WR_DONE = 5,
> > +} sc_rpc_async_state_t;
> > +
> > +typedef struct sc_rpc_async_msg_s {
> > + sc_rpc_async_state_t state;
> > + uint8_t wordIdx;
> > + sc_rpc_msg_t msg;
> > + uint32_t timeStamp;
> > +} sc_rpc_async_msg_t;
>
> We normally do not typedef struct types. Any good reasons to do so here?
>
Yes, I did not some checkpatch warnings about this.
For this patch, I only implemented IPC function calls based on MU which is in
drivers/soc/imx/sc/main/ipc.c. All other files actually are generated by SCU firmware
script automatically for Linux OS. They're tightly couple with SCU firmware side
implementation. In order to gain better maintainability (easy upgrade and sync with
SCU firmware), we're trying to not change those APIs too much unless we have strong
reasons. Does it make sense to you?
> > +
> > +/* Functions */
> > +
> > +/*
> > + * This is an internal function to send an RPC message over an IPC
> > + * channel. It is called by client-side SCFW API function shims.
> > + *
> > + * @param[in] ipc IPC handle
> > + * @param[in,out] msg handle to a message
> > + * @param[in] no_resp response flag
> > + *
> > + * If no_resp is false then this function waits for a response
> > + * and returns the result in msg.
> > + */
> > +void sc_call_rpc(sc_ipc_t ipc, sc_rpc_msg_t *msg, bool no_resp);
> > +
> > +/*
> > + * This is an internal function to dispath an RPC call that has
> > + * arrived via IPC over an MU. It is called by server-side SCFW.
> > + *
> > + * @param[in] mu MU message arrived on
> > + * @param[in,out] msg handle to a message
> > + *
> > + * The function result is returned in msg.
> > + */
> > +void sc_rpc_dispatch(sc_rsrc_t mu, sc_rpc_msg_t *msg);
>
> Declared but never defined.
>
Good catch.
> > +
> > +/*
> > + * This function translates an RPC message and forwards on to the
> > + * normal RPC API. It is used only by hypervisors.
> > + *
> > + * @param[in] ipc IPC handle
> > + * @param[in,out] msg handle to a message
> > + *
> > + * This function decodes a message, calls macros to translate the
> > + * resources, pins, addresses, partitions, memory regions, etc. and
> > + * then forwards on to the hypervisors SCFW API.Return results are
> > + * translated back abd placed back into the message to be returned
> > + * to the original API.
> > + */
> > +void sc_rpc_xlate(sc_ipc_t ipc, sc_rpc_msg_t *msg);
>
> ditto.
>
> > +
> > +#endif /* _SC_RPC_H */
> > diff --git a/drivers/soc/imx/sc/svc/irq/rpc.h
> > b/drivers/soc/imx/sc/svc/irq/rpc.h
> > new file mode 100644
> > index 0000000..0de8718
> > --- /dev/null
> > +++ b/drivers/soc/imx/sc/svc/irq/rpc.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Copyright 2017~2018 NXP
> > + *
> > + * Header file for the IRQ RPC implementation.
> > + */
> > +
> > +#ifndef _SC_IRQ_RPC_H
> > +#define _SC_IRQ_RPC_H
> > +
> > +/* Types */
> > +
> > +/*!
> > + * This type is used to indicate RPC IRQ function calls.
> > + */
> > +typedef enum irq_func_e {
> > + IRQ_FUNC_UNKNOWN = 0, /* Unknown function */
> > + IRQ_FUNC_ENABLE = 1, /* Index for irq_enable() RPC call */
> > + IRQ_FUNC_STATUS = 2, /* Index for irq_status() RPC call */
> > +} irq_func_t;
> > +
> > +/* Functions */
> > +
> > +/*!
> > + * This function dispatches an incoming IRQ RPC request.
> > + *
> > + * @param[in] caller_pt caller partition
> > + * @param[in] msg pointer to RPC message
> > + */
> > +void irq_dispatch(sc_rm_pt_t caller_pt, sc_rpc_msg_t *msg);
>
> ditto.
>
> > +
> > +/*!
> > + * This function translates and dispatches an IRQ RPC request.
> > + *
> > + * @param[in] ipc IPC handle
> > + * @param[in] msg pointer to RPC message
> > + */
> > +void irq_xlate(sc_ipc_t ipc, sc_rpc_msg_t *msg);
>
> ditto.
>
Those APIs seems are over generated by SCU firmware.
I will clean up it.
> > +++ b/include/soc/imx/sc/svc/timer/api.h
> > @@ -0,0 +1,265 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> > + * Copyright 2017~2018 NXP
> > + *
> > + * Header file containing the public API for the System Controller
> > +(SC)
> > + * Timer function.
> > + *
> > + * TIMER_SVC (SVC) Timer Service
> > + *
> > + * Module for the Timer service. This includes support for the
> > +watchdog, RTC,
> > + * and system counter. Note every resource partition has a watchdog
> > +it can
> > + * use.
> > + */
> > +
> > +#ifndef _SC_TIMER_API_H
> > +#define _SC_TIMER_API_H
> > +
> > +/* Includes */
> > +
> > +#include <soc/imx/sc/types.h>
> > +#include <soc/imx/sc/svc/rm/api.h>
> > +
> > +/* Defines */
> > +
> > +/*
> > + * @name Defines for type widths
> > + */
> > +#define SC_TIMER_ACTION_W 3 /* Width of sc_timer_wdog_action_t
> */
> > +
> > +/*
> > + * @name Defines for sc_timer_wdog_action_t */
> > +#define SC_TIMER_WDOG_ACTION_PARTITION 0 /* Reset
> partition */
> > +#define SC_TIMER_WDOG_ACTION_WARM 1 /* Warm reset
> system */
> > +#define SC_TIMER_WDOG_ACTION_COLD 2 /* Cold reset system
> */
> > +#define SC_TIMER_WDOG_ACTION_BOARD 3 /* Reset board */
> > +#define SC_TIMER_WDOG_ACTION_IRQ 4 /* Only generate
> IRQs */
> > +
> > +/* Types */
> > +
> > +/*
> > + * This type is used to configure the watchdog action.
> > + */
> > +typedef uint8_t sc_timer_wdog_action_t;
>
> Mixing an unnecessary typedef and defines doesn't look good. This should
> be
>
> typedef enum {
> SC_TIMER_WDOG_ACTION_PARTITION,
> } sc_timer_wdog_action_t;
>
> The same pattern is used elsewhere in this patch.
>
Yes, I understand.
Do you think we need change them all?
> > +
> > +/*
> > + * This type is used to declare a watchdog time value in milliseconds.
> > + */
> > +typedef uint32_t sc_timer_wdog_time_t;
>
> Why an extra type for this?
>
> Sascha
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions |
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fww
> w.pengutronix.de%2F&data=02%7C01%7Caisheng.dong%40nxp.com%7C2d0
> 5ba17b0d24957022a08d5b0140906%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636608522458461041&sdata=WMYanmj1AZkESu0fbXrspidjPbVc
> t3sKQYNhGt5J9ME%3D&reserved=0 |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
More information about the linux-arm-kernel
mailing list