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

[Anatolij Gustschin <agust@denx.de>]

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.

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

...
> +	/* 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?

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

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

Return value check needed?

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

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

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

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

--
Anatolij