[v9,0/4] add mailbox support for i.MX7D

20180802 changes v9:
- DT: remove notes about shmem
- imx-mailbox: reword changelog 
- imx-mailbox: use tasklet instead of workqueue
- imx-mailbox: remove imx_mu_last_tx_done
- imx-mailbox: move irq_desc to probe
- imx-mailbox: change (const == var) to (var == const)

20180731 changes v8:
- imx-mailbox & DT: rework driver from 4 channel bidirectional mode to
  16 channel unidirectional mode. 8 extra channels are implemented on
  top of General Purpose Interrupt bits provided by this MU.

20180726 changes v7:
- DT: add i.MX6SX and i.MX7S to the documentation.
- imx-mailbox: don't use devm_ functions for startup and shutdown.
- imx-mailbox: rename imx_mu_rmw to imx_mu_xcr_rmw and add locks
- imx-mailbox: pass of_property_read_bool directly to side_b

20180722 changes v6:
- include one more patch provided by Aisheng
- DT: add fall back compatible fsl,imx6sx-mu
- imx-mailbox: for now, use only fsl,imx6sx-mu

20180721 changes v5:
- DT: revert most of the changes from previous version
- imx-mailbox: remove struct imx_mu_cfg
- imx-mailbox: remove !! from imx_mu_last_tx_done()

20180718 changes v4:
- DT: change fsl,mu-side-a to fsl,mu-side-b
- DT: split the patches.
- DT: add all currently known SoCs.
- imx-mailbox: free allocated irq name on channel shutdown
- imx-mailbox: rename *_imx7 functions to *_generic

20180715 changes v3:
- DT: remove prosaic part of documentation. It describes software
  or firmware specific usage and not relevant for HW description.
- DT: use <soc>-mu instead of <soc>-mu-<mu side> and add fsl,mu-side-a
  parameter.
- DT: add most of know i.MX variants with MU
- imx-mailbox: use macros instead of precalculated bit index.
- imx-mailbox: remove warning message for clk.
- imx-mailbox: use imx_mu_chan[%idx] for devm_request_irq.
- imx-mailbox: depend on ARCH_MXC instead of SOX_IMX7

20180615 changes v2:
- DT: use mailbox@ instead of mu@
- DT: change interrupts description
- clk: use imx_clk_gate4 instead of imx_clk_gate2
- imx-mailbox: remove last_tx_done support
- imx-mailbox: fix module description 

This patches are providing support for mailbox (Messaging Unit)
for i.MX7D.
Functionality was tested on PHYTEC phyBOARD-Zeta i.MX7D with
Linux running on all cores: ARM Cortex-A7 and ARM Cortex-M4.

Both parts of i.MX messaging Unit are visible for all CPUs available
on i.MX7D. Communication worked independent of MU side in combination
with CPU. For example MU-A used on ARM Cortex-A7 and MU-B used on ARM Cortex-M4
or other ways around.

Dong Aisheng (1):
  dt-bindings: arm: fsl: add mu binding doc

Oleksij Rempel (3):
  dt-bindings: mailbox: imx-mu: add generic MU channel support
  ARM: dts: imx7s: add i.MX7 messaging unit support
  mailbox: Add support for i.MX messaging unit

 .../devicetree/bindings/mailbox/fsl,mu.txt    |  54 +++
 arch/arm/boot/dts/imx7s.dtsi                  |  19 +
 drivers/mailbox/Kconfig                       |   6 +
 drivers/mailbox/Makefile                      |   2 +
 drivers/mailbox/imx-mailbox.c                 | 357 ++++++++++++++++++
 5 files changed, 438 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/fsl,mu.txt
 create mode 100644 drivers/mailbox/imx-mailbox.c

Hi Oleksij,

two more nitpickings fro my side.

On 08/02/2018 10:23 AM, Oleksij Rempel wrote:
> The i.MX Messaging Unit is a two side block which allows applications
> implement communication over this sides.
> 
> The MU includes the following features:
> - Messaging control by interrupts or by polling
> - Four general-purpose interrupt requests reflected to the other side
> - Three general-purpose flags reflected to the other side
> - Four receive registers with maskable interrupt
> - Four transmit registers with maskable interrupt
> 
> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

[snip]

> +static irqreturn_t imx_mu_isr(int irq, void *p)
> +{
> +	struct mbox_chan *chan = p;
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +	u32 val, ctrl, dat;
> +
> +	ctrl = imx_mu_read(priv, IMX_MU_xCR);
> +	val = imx_mu_read(priv, IMX_MU_xSR);
> +
> +	switch (cp->type) {
> +	case IMX_MU_TYPE_TX:
> +		val &= IMX_MU_xSR_TEn(cp->idx) &
> +			(ctrl & IMX_MU_xCR_TIEn(cp->idx));
> +		break;
> +	case IMX_MU_TYPE_RX:
> +		val &= IMX_MU_xSR_RFn(cp->idx) &
> +			(ctrl & IMX_MU_xCR_RIEn(cp->idx));
> +		break;
> +	case IMX_MU_TYPE_RXDB:
> +		val &= IMX_MU_xSR_GIPn(cp->idx) &
> +			(ctrl & IMX_MU_xCR_GIEn(cp->idx));
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	if (!val) {
> +		return IRQ_NONE;
> +	} else if (val == IMX_MU_xSR_TEn(cp->idx)) {

Please drop the 'else' branch above.

Here you can either just start from a new 'if', or all together drop 'if (!val)'
and return IRQ_NONE at the end of the if-else construction.

> +		imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx));
> +		mbox_chan_txdone(chan, 0);
> +	} else if (val == IMX_MU_xSR_RFn(cp->idx)) {
> +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> +		mbox_chan_received_data(chan, (void *)&dat);
> +	} else if (val == IMX_MU_xSR_GIPn(cp->idx)) {
> +		imx_mu_write(priv, IMX_MU_xSR_GIPn(cp->idx), IMX_MU_xSR);
> +		mbox_chan_received_data(chan, NULL);
> +	} else {
> +		dev_warn_ratelimited(priv->dev, "Not handled interrupt\n");
> +		return IRQ_NONE;
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[snip]

> +	for (i = 0; i < IMX_MU_CHANS; i++) {
> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
> +
> +		cp->idx = i % 4;
> +		cp->type = (i - cp->idx) >> 2;

cp->type = i >> 2;

> +		cp->chan = &priv->mbox_chans[i];
> +		priv->mbox_chans[i].con_priv = cp;
> +		snprintf(cp->irq_desc, sizeof(cp->irq_desc),
> +			 "imx_mu_chan[%i-%i]", cp->type, cp->idx);
> +	}
> +
> +	priv->side_b = of_property_read_bool(np, "fsl,mu-side-b");
> +
> +	spin_lock_init(&priv->xcr_lock);
> +
> +	priv->mbox.dev = dev;
> +	priv->mbox.ops = &imx_mu_ops;
> +	priv->mbox.chans = priv->mbox_chans;
> +	priv->mbox.num_chans = IMX_MU_CHANS;
> +	priv->mbox.of_xlate = imx_mu_xlate;
> +	priv->mbox.txdone_irq = true;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	imx_mu_init_generic(priv);
> +
> +	return mbox_controller_register(&priv->mbox);
> +}

--
Best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Vladimir,

thank you for review.

Jassi, Dong, any other comments? If no, i'll send tomorrow updated version.

On 02.08.2018 09:54, Vladimir Zapolskiy wrote:
> Hi Oleksij,
> 
> two more nitpickings fro my side.
> 
> On 08/02/2018 10:23 AM, Oleksij Rempel wrote:
>> The i.MX Messaging Unit is a two side block which allows applications
>> implement communication over this sides.
>>
>> The MU includes the following features:
>> - Messaging control by interrupts or by polling
>> - Four general-purpose interrupt requests reflected to the other side
>> - Three general-purpose flags reflected to the other side
>> - Four receive registers with maskable interrupt
>> - Four transmit registers with maskable interrupt
>>
>> Reviewed-by: Vladimir Zapolskiy <vz@mleia.com>
>> Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> [snip]
> 
>> +static irqreturn_t imx_mu_isr(int irq, void *p)
>> +{
>> +	struct mbox_chan *chan = p;
>> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>> +	struct imx_mu_con_priv *cp = chan->con_priv;
>> +	u32 val, ctrl, dat;
>> +
>> +	ctrl = imx_mu_read(priv, IMX_MU_xCR);
>> +	val = imx_mu_read(priv, IMX_MU_xSR);
>> +
>> +	switch (cp->type) {
>> +	case IMX_MU_TYPE_TX:
>> +		val &= IMX_MU_xSR_TEn(cp->idx) &
>> +			(ctrl & IMX_MU_xCR_TIEn(cp->idx));
>> +		break;
>> +	case IMX_MU_TYPE_RX:
>> +		val &= IMX_MU_xSR_RFn(cp->idx) &
>> +			(ctrl & IMX_MU_xCR_RIEn(cp->idx));
>> +		break;
>> +	case IMX_MU_TYPE_RXDB:
>> +		val &= IMX_MU_xSR_GIPn(cp->idx) &
>> +			(ctrl & IMX_MU_xCR_GIEn(cp->idx));
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	if (!val) {
>> +		return IRQ_NONE;
>> +	} else if (val == IMX_MU_xSR_TEn(cp->idx)) {
> 
> Please drop the 'else' branch above.
> 
> Here you can either just start from a new 'if', or all together drop 'if (!val)'
> and return IRQ_NONE at the end of the if-else construction.
> 
>> +		imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx));
>> +		mbox_chan_txdone(chan, 0);
>> +	} else if (val == IMX_MU_xSR_RFn(cp->idx)) {
>> +		dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
>> +		mbox_chan_received_data(chan, (void *)&dat);
>> +	} else if (val == IMX_MU_xSR_GIPn(cp->idx)) {
>> +		imx_mu_write(priv, IMX_MU_xSR_GIPn(cp->idx), IMX_MU_xSR);
>> +		mbox_chan_received_data(chan, NULL);
>> +	} else {
>> +		dev_warn_ratelimited(priv->dev, "Not handled interrupt\n");
>> +		return IRQ_NONE;
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> [snip]
> 
>> +	for (i = 0; i < IMX_MU_CHANS; i++) {
>> +		struct imx_mu_con_priv *cp = &priv->con_priv[i];
>> +
>> +		cp->idx = i % 4;
>> +		cp->type = (i - cp->idx) >> 2;
> 
> cp->type = i >> 2;
> 
>> +		cp->chan = &priv->mbox_chans[i];
>> +		priv->mbox_chans[i].con_priv = cp;
>> +		snprintf(cp->irq_desc, sizeof(cp->irq_desc),
>> +			 "imx_mu_chan[%i-%i]", cp->type, cp->idx);
>> +	}
>> +
>> +	priv->side_b = of_property_read_bool(np, "fsl,mu-side-b");
>> +
>> +	spin_lock_init(&priv->xcr_lock);
>> +
>> +	priv->mbox.dev = dev;
>> +	priv->mbox.ops = &imx_mu_ops;
>> +	priv->mbox.chans = priv->mbox_chans;
>> +	priv->mbox.num_chans = IMX_MU_CHANS;
>> +	priv->mbox.of_xlate = imx_mu_xlate;
>> +	priv->mbox.txdone_irq = true;
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	imx_mu_init_generic(priv);
>> +
>> +	return mbox_controller_register(&priv->mbox);
>> +}
> 
> --
> Best wishes,
> Vladimir
>

On Thu, Aug 2, 2018 at 3:17 PM, Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> Hi Vladimir,
>
> thank you for review.
>
> Jassi, Dong, any other comments? If no, i'll send tomorrow updated version.
>
Looks fine to me.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html