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

Add i.MX8 MISC driver to handle the communication between
A35 Core and SCU.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/misc/Makefile      |   1 +
 drivers/misc/imx8/Makefile |   3 +
 drivers/misc/imx8/scu.c    | 247 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 251 insertions(+)
 create mode 100644 drivers/misc/imx8/Makefile
 create mode 100644 drivers/misc/imx8/scu.c

Hi Peng,

On Mon,  6 Aug 2018 10:50:22 +0800
Peng Fan peng.fan@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

Hi Anatolij,

> -----Original Message-----
> From: Anatolij Gustschin [mailto:agust@denx.de]
> Sent: 2018年8月10日 0:19
> To: Peng Fan <peng.fan@nxp.com>; u-boot@lists.denx.de
> Cc: sbabic@denx.de; Fabio Estevam <fabio.estevam@nxp.com>; dl-linux-imx
> <linux-imx@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@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@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

Hi Peng,

On Fri, 10 Aug 2018 02:30:09 +0000
Peng Fan peng.fan@nxp.com wrote:
...
> > 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.

OK.

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

No. I think we will have to stick with the scu_dev in global data, now
I see that it is needed just after relocation and before initr_dm(),
so we can't get it just after relocation via dm functions.

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

I'm working on ENET and I2C/USB/LVDS support for i.MX8QXP MEK based
on this patch series and will try to review remaining patches over
the weekend.

Thanks,
Anatolij