[U-Boot] [PATCH V3 07/32] misc: add i.MX8 misc driver

[Peng Fan]

Hi Anatolij,

> -----Original Message-----
> From: Anatolij Gustschin [mailto:agust at denx.de]
> Sent: 2018年8月10日 0:19
> To: Peng Fan <peng.fan at nxp.com>; u-boot at lists.denx.de
> Cc: sbabic at denx.de; Fabio Estevam <fabio.estevam at nxp.com>; dl-linux-imx
> <linux-imx at nxp.com>
> Subject: Re: [U-Boot] [PATCH V3 07/32] misc: add i.MX8 misc driver
> 
> Hi Peng,
> 
> On Mon,  6 Aug 2018 10:50:22 +0800
> Peng Fan peng.fan at nxp.com wrote:
> 
> > Add i.MX8 MISC driver to handle the communication between
> > A35 Core and SCU.
> 
> Thanks for working on this! I think we should rename this driver to imx-mu and
> unify it, so that it can be used on i.MX7 and i.MX6SX, if needed.


Thanks for reviewing the patcset. Communicating with SCU needs
dedicated mu designed for SCU, and the scfw requires all the 4 channels
being used. It will introduce too much complexity to develop a generic mu
driver support generic mu and scu mu. I would not do that.

> 
> ...
> > diff --git a/drivers/misc/imx8/scu.c b/drivers/misc/imx8/scu.c new
> > file mode 100644 index 0000000000..3cc6b719e3
> > --- /dev/null
> > +++ b/drivers/misc/imx8/scu.c
> > @@ -0,0 +1,247 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2018 NXP
> > + *
> > + * Peng Fan <peng.fan at nxp.com>
> > + */
> > +
> > +#include <common.h>
> > +#include <asm/io.h>
> > +#include <dm.h>
> > +#include <dm/lists.h>
> > +#include <dm/root.h>
> > +#include <dm/device-internal.h>
> > +#include <asm/arch/sci/sci.h>
> > +#include <linux/iopoll.h>
> > +#include <misc.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> > +struct mu_type {
> > +	u32 tr[4];
> > +	u32 rr[4];
> > +	u32 sr;
> > +	u32 cr;
> > +};
> > +
> > +struct imx8_scu {
> > +	struct mu_type *base;
> > +	struct udevice *clk;
> > +	struct udevice *pinclk;
> > +};
> > +
> > +#define MU_CR_GIE_MASK		0xF0000000u
> > +#define MU_CR_RIE_MASK		0xF000000u
> > +#define MU_CR_GIR_MASK		0xF0000u
> > +#define MU_CR_TIE_MASK		0xF00000u
> > +#define MU_CR_F_MASK		0x7u
> > +#define MU_SR_TE0_MASK		BIT(23)
> > +#define MU_SR_RF0_MASK		BIT(27)
> > +#define MU_TR_COUNT		4
> > +#define MU_RR_COUNT		4
> > +static inline void mu_hal_init(struct mu_type *base)
> 
> Please use empty line after macro definition.
> 
> ...
> > +static int mu_hal_receivemsg(struct mu_type *base, u32 reg_index, u32
> > +*msg) {
> > +	u32 mask = MU_SR_RF0_MASK >> reg_index;
> > +	u32 val;
> > +	int ret;
> > +
> > +	assert(reg_index < MU_TR_COUNT);
> > +
> > +	/* Wait RX register to be full. */
> > +	ret = readl_poll_timeout(&base->sr, val, val & mask, 10000);
> > +	if (ret < 0) {
> > +		printf("%s timeout\n", __func__);
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	*msg = readl(&base->rr[reg_index]);
> > +
> > +	return 0;
> > +}
> > +
> > +static void sc_ipc_read(struct mu_type *base, void *data) {
> > +	struct sc_rpc_msg_s *msg = (struct sc_rpc_msg_s *)data;
> > +	u8 count = 0;
> > +
> > +	/* Check parms */
> > +	if (!base || !msg)
> 
> Can base ever be NULL? If the driver is bound, base is always initialized, so I think
> base check can be dropped.

ok. Fix in V4.

> 
> ...
> > +	/* Read first word */
> > +	mu_hal_receivemsg(base, 0, (u32 *)msg);
> 
> The return value is never checked. I don't know the internal details of the
> message protocol, but does it make sense to read more data if reading first
> word failed?

Actually it should not fail, there is already a error msg in mu_hal_receivemsg.
You point is also valid, Fix in V4.

> 
> > +	count++;
> > +
> > +	/* Check size */
> > +	if (msg->size > SC_RPC_MAX_MSG) {
> > +		*((u32 *)msg) = 0;
> > +		return;
> 
> Should we return an error in this case to propagate it to the caller?

Fix in V4.

> 
> > +	}
> > +
> > +	/* Read remaining words */
> > +	while (count < msg->size) {
> > +		mu_hal_receivemsg(base, count % MU_RR_COUNT,
> > +				  &msg->DATA.u32[count - 1]);
> 
> Return value check needed?

Fix in V4.

> 
> > +		count++;
> > +	}
> > +}
> > +
> > +static void sc_ipc_write(struct mu_type *base, void *data) {
> > +	struct sc_rpc_msg_s *msg = (struct sc_rpc_msg_s *)data;
> > +	u8 count = 0;
> > +
> > +	/* Check parms */
> > +	if (!base || !msg)
> 
> Drop base check?

Fix in V4.

> 
> > +		return;
> > +
> > +	/* Check size */
> > +	if (msg->size > SC_RPC_MAX_MSG)
> > +		return;
> > +
> > +	/* Write first word */
> > +	mu_hal_sendmsg(base, 0, *((u32 *)msg));
> 
> Return value is not checked here. Does it make sense to send remaining data if
> transmitting of the first word failed? Wouldn't there be a protocol error at the
> receiver side when the message is partially received? I think we should stop
> sending here and return an error code to the caller.

Fix in V4. 

> 
> > +	count++;
> > +
> > +	/* Write remaining words */
> > +	while (count < msg->size) {
> > +		mu_hal_sendmsg(base, count % MU_TR_COUNT,
> > +			       msg->DATA.u32[count - 1]);
> 
> Return value check?
> 
> > +		count++;
> > +	}
> > +}
> > +
> > +/*
> > + * Note the function prototype use msgid as the 2nd parameter, here
> > + * we take it as no_resp.
> > + */
> > +static int imx8_scu_call(struct udevice *dev, int no_resp, void *tx_msg,
> > +			 int tx_size, void *rx_msg, int rx_size) {
> > +	struct imx8_scu *priv = dev_get_priv(dev);
> > +	sc_err_t result;
> > +
> > +	/* Expect tx_msg, rx_msg are the same value */
> > +	if (rx_msg && tx_msg != rx_msg)
> > +		printf("tx_msg %p, rx_msg %p\n", tx_msg, rx_msg);
> > +
> > +	sc_ipc_write(priv->base, tx_msg);
> 
> Check and propagate error code here?

Fix in v4.

> 
> > +	if (!no_resp)
> > +		sc_ipc_read(priv->base, rx_msg);
> > +
> > +	result = RPC_R8((struct sc_rpc_msg_s *)tx_msg);
> > +
> > +	return sc_err_to_linux(result);
> > +}
> > +
> > +static int imx8_scu_probe(struct udevice *dev) {
> > +	struct imx8_scu *priv = dev_get_priv(dev);
> > +	fdt_addr_t addr;
> > +
> > +	debug("%s(dev=%p) (priv=%p)\n", __func__, dev, priv);
> > +
> > +	addr = devfdt_get_addr(dev);
> > +	if (addr == FDT_ADDR_T_NONE)
> > +		return -EINVAL;
> > +
> > +	priv->base = (struct mu_type *)addr;
> > +
> > +	/* U-Boot not enable interrupts, so need to enable RX interrupts */
> > +	mu_hal_init(priv->base);
> > +
> > +	gd->arch.scu_dev = dev;
> 
> Can we avoid using the dev pointer in global data?

I could not find a better solution to make other components could
easily refer the scu dev. Do you have any suggestions?

Do you have time to review the remaining patches in the patchset?

Thanks,
Peng

> 
> --
> Anatolij