[PATCH 4/4] soc: imx: add SC firmware IPC and APIs
A.s. Dong
aisheng.dong at nxp.com
Thu May 3 05:29:40 PDT 2018
> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer at pengutronix.de]
> Sent: Thursday, May 3, 2018 7:06 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 Wed, May 02, 2018 at 06:40:03PM +0000, A.s. Dong wrote:
> > > -----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.
>
> What I'm suggesting is a single node:
>
> scu at 5d1b0000 {
> compatible = "fsl,imx8qxp-scu";
> reg = <0x0 0x5d1b0000 0x0 0x10000>;
> };
>
> Attach your code to this one, without any further separation between mu
> and scu code.
>
A bit confused. You're suggesting a single node here without mu or mailbox node
phandle in it? Then how SCU use MU?
> We discussed this internally and came to the conclusion that the SCU is not a
> generic user of a MU. The MU is designed to work together with a piece of
> SRAM to form a mailbox system. Instead of working as designed the SCU
> morses the messages through the doorbell (what the MU really is). For
> anything that uses the MU in the way it is designed I would suggest using the
> mailbox interface from drivers/mailbox/ along with the binding from
> Documentation/devicetree/bindings/mailbox/mailbox.txt.
>
> In the way I suggest there is no need for any MU ID.
>
So you're suggesting switch to use mailbox instead of private MU library
function calls?
Something like:
scu {
compatible = "nxp,imx8qxp-scu", "simple-bus";
mboxes = <&mailbox 0>;
}
Then IPC is implemented based on mailbox?
As I replied Oleksij Rempel in another mail in this thread, we've tried mailbox
but got performance regression issue and need more time to investigate
whether it's really quite suitable for i.MX SCU firmware as SCU handling
message quite fast in micro seconds. (Mailbox polling method has much
more latency than current MU sample polling we used)
and we want to avoid the SCU firmware API changes.
> >
> > > > +#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?
>
> Hm, I'm not suggesting to change the API, only the header files. In another
> mail you say that the SCU firmware is stable, so there shouldn't be any strong
> need to sync the header files from your internal repositories, right?
> Besides, you could also update your internal repositories with the
> suggestions I make ;)
>
Yes, I will try to update it if no big side effect.
Thanks for the suggestion.
Regards
Dong Aisheng
> 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%7Cfeb
> 56093d6964d31a97c08d5b0e5e28c%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C636609423746219869&sdata=l6hKOW1WakB4tpB4%2Bfcp6dLt4t
> 97Q51Feb%2FTkKHZF3g%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