| Message ID | 20180615095107.24610-3-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, 2018-06-15 at 11:51 +0200, Oleksij Rempel wrote: > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > .../bindings/mailbox/imx-mailbox.txt | 35 +++++++++++++++++++ > 1 file changed, 35 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mailbox/imx-mailbox.txt A recent patch was posted which adds a similar but different binding for the MU on 8qm/8qxp SOCs: https://patchwork.kernel.org/patch/10468885/ Looking at manuals side-by-side the hardware seems to be the same so there should be a single binding. Right? That series I pointed to uses the MU to implement a communication with a special "SCU" core which runs NXP firmware for handling details like power management. However imx8 socs also have other MUs and M4 cores for customers to use pretty exactly like they would on 7d. The hardware exposes a very generic interface and my impression is that drivers for the MU are actually highly specific to what is on the other side of the MU. For example your driver code seems to be mapping the 4 MU registers to separate "channels" but for SCU messages are written in all registers in a round-robin way. Shouldn't your MU-using driver be a separate node which references the MU by phandle? Like in this patch: https://patchwork.kernel.org/patch/10468887/ > diff --git a/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt > new file mode 100644 > index 000000000000..1577b86f1206 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt > @@ -0,0 +1,35 @@ > +i.MX Messaging Unit > +=================== > + > +The i.MX Messaging Unit (MU) contains two register sets: "A" and "B". In most > +cases they are accessible from all Processor Units. On one hand, at least for > +mailbox functionality, it makes no difference which application or processor is > +using which set of the MU. On other hand, the register sets for each of the MU > +parts are not identical. > + > +Required properties: > +- compatible : Shell be one of: > + "fsl,imx7s-mu-a" and "fsl,imx7s-mu-b" for i.MX7S or i.MX7D > +- reg : physical base address of the mailbox and length of > + memory mapped region. > +- #mbox-cells: Common mailbox binding property to identify the number > + of cells required for the mailbox specifier. Should be 1. > +- interrupts : The interrupt number > +- clocks : phandle to the input clock. > + > +Example: > + mu0a: mailbox@30aa0000 { > + compatible = "fsl,imx7s-mu-a"; > + reg = <0x30aa0000 0x28>; > + interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clks IMX7D_MU_ROOT_CLK>; > + #mbox-cells = <1>; > + }; > + > + mu0b: mailbox@30ab0000 { > + compatible = "fsl,imx7s-mu-b"; > + reg = <0x30ab0000 0x28>; > + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clks IMX7D_MU_ROOT_CLK>; > + #mbox-cells = <1>; > + };
Hi Leonard, On 18.06.2018 10:53, Leonard Crestez wrote: > On Fri, 2018-06-15 at 11:51 +0200, Oleksij Rempel wrote: >> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> >> --- >> .../bindings/mailbox/imx-mailbox.txt | 35 +++++++++++++++++++ >> 1 file changed, 35 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/mailbox/imx-mailbox.txt > > A recent patch was posted which adds a similar but different binding > for the MU on 8qm/8qxp SOCs: > > https://patchwork.kernel.org/patch/10468885/ > > Looking at manuals side-by-side the hardware seems to be the same so > there should be a single binding. Right? yes. This is why it make no sense to create imx8 specific MU driver. > That series I pointed to uses the MU to implement a communication with > a special "SCU" core which runs NXP firmware for handling details like > power management. However imx8 socs also have other MUs and M4 cores > for customers to use pretty exactly like they would on 7d. > > The hardware exposes a very generic interface and my impression is that > drivers for the MU are actually highly specific to what is on the > other side of the MU. For example your driver code seems to be mapping > the 4 MU registers to separate "channels" but for SCU messages are > written in all registers in a round-robin way. ok.. let's take some of IMX8 SCU driver code to see if there any difference: -- this part of the code is blocking write procedure for one channeler per write-- correct? +void mu_send_msg(struct mu_priv *priv, uint32_t index, uint32_t msg) +{ + uint32_t mask = MU_SR_TE0_MASK >> index; + + /* Wait TX register to be empty. */ + while (!(readl_relaxed(priv->base + MU_ASR) & mask)) + ; + writel_relaxed(msg, priv->base + MU_ATR0 + (index * 4)); +} +EXPORT_SYMBOL_GPL(mu_send_msg); +static void sc_ipc_write(struct sc_ipc *sc_ipc, void *data) +{ + sc_rpc_msg_t *msg = (sc_rpc_msg_t *) data; + uint8_t count = 0; + + /* Check size */ + if (msg->size > SC_RPC_MAX_MSG) + return; + + /* Write first word */ + mu_send_msg(sc_ipc->mu_base, 0, *((uint32_t *) msg)); + count++; --- in this loop we are writing to one channel per loop and waiting until the channel was done ---- + /* Write remaining words */ + while (count < msg->size) { + mu_send_msg(sc_ipc->mu_base, count % MU_TR_COUNT, + msg->DATA.u32[count - 1]); + count++; + } +} ... and here is a proof that sc_ipc_write will do in some cases 5 rounds (5 * 4 bytes = 20 bytes single message) with probable busy waiting for each channel, which make me think that shared memory would be a better deal. +sc_err_t sc_misc_seco_image_load(sc_ipc_t ipc, uint32_t addr_src, + uint32_t addr_dst, uint32_t len, bool fw) +{ + sc_rpc_msg_t msg; + uint8_t result; + + RPC_VER(&msg) = SC_RPC_VERSION; + RPC_SVC(&msg) = (uint8_t)SC_RPC_SVC_MISC; + RPC_FUNC(&msg) = (uint8_t)MISC_FUNC_SECO_IMAGE_LOAD; + RPC_U32(&msg, 0) = addr_src; + RPC_U32(&msg, 4) = addr_dst; + RPC_U32(&msg, 8) = len; + RPC_U8(&msg, 12) = (uint8_t)fw; + RPC_SIZE(&msg) = 5; + + sc_call_rpc(ipc, &msg, false); + + result = RPC_R8(&msg); + return (sc_err_t)result; +} + So, the same code with mailbox framework will be some thing like this: /* Write remaining words */ while (count < msg->size) { mbox_send_message(sc_ipc->mbox_chan[count % MU_TR_COUNT], msg->DATA.u32[count - 1]); count++; } to provide identical behavior we should set mbox_client->tx_block = true and implement polling mode in the mailbox driver. Please note. I don't think sc_ipc_write() implementation is good. I just don't expect it will be ever changed. > Shouldn't your MU-using driver be a separate node which references the > MU by phandle? Like in this patch: sure. But it is generic, not mailbox controller specific and make no sense to describe client binding again in the controller binding. > https://patchwork.kernel.org/patch/10468887/ > >> diff --git a/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt >> new file mode 100644 >> index 000000000000..1577b86f1206 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt >> @@ -0,0 +1,35 @@ >> +i.MX Messaging Unit >> +=================== >> + >> +The i.MX Messaging Unit (MU) contains two register sets: "A" and "B". In most >> +cases they are accessible from all Processor Units. On one hand, at least for >> +mailbox functionality, it makes no difference which application or processor is >> +using which set of the MU. On other hand, the register sets for each of the MU >> +parts are not identical. >> + >> +Required properties: >> +- compatible : Shell be one of: >> + "fsl,imx7s-mu-a" and "fsl,imx7s-mu-b" for i.MX7S or i.MX7D >> +- reg : physical base address of the mailbox and length of >> + memory mapped region. >> +- #mbox-cells: Common mailbox binding property to identify the number >> + of cells required for the mailbox specifier. Should be 1. >> +- interrupts : The interrupt number >> +- clocks : phandle to the input clock. >> + >> +Example: >> + mu0a: mailbox@30aa0000 { >> + compatible = "fsl,imx7s-mu-a"; >> + reg = <0x30aa0000 0x28>; >> + interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&clks IMX7D_MU_ROOT_CLK>; >> + #mbox-cells = <1>; >> + }; >> + >> + mu0b: mailbox@30ab0000 { >> + compatible = "fsl,imx7s-mu-b"; >> + reg = <0x30ab0000 0x28>; >> + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&clks IMX7D_MU_ROOT_CLK>; >> + #mbox-cells = <1>; >> + };
diff --git a/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt new file mode 100644 index 000000000000..1577b86f1206 --- /dev/null +++ b/Documentation/devicetree/bindings/mailbox/imx-mailbox.txt @@ -0,0 +1,35 @@ +i.MX Messaging Unit +=================== + +The i.MX Messaging Unit (MU) contains two register sets: "A" and "B". In most +cases they are accessible from all Processor Units. On one hand, at least for +mailbox functionality, it makes no difference which application or processor is +using which set of the MU. On other hand, the register sets for each of the MU +parts are not identical. + +Required properties: +- compatible : Shell be one of: + "fsl,imx7s-mu-a" and "fsl,imx7s-mu-b" for i.MX7S or i.MX7D +- reg : physical base address of the mailbox and length of + memory mapped region. +- #mbox-cells: Common mailbox binding property to identify the number + of cells required for the mailbox specifier. Should be 1. +- interrupts : The interrupt number +- clocks : phandle to the input clock. + +Example: + mu0a: mailbox@30aa0000 { + compatible = "fsl,imx7s-mu-a"; + reg = <0x30aa0000 0x28>; + interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clks IMX7D_MU_ROOT_CLK>; + #mbox-cells = <1>; + }; + + mu0b: mailbox@30ab0000 { + compatible = "fsl,imx7s-mu-b"; + reg = <0x30ab0000 0x28>; + interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clks IMX7D_MU_ROOT_CLK>; + #mbox-cells = <1>; + };
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- .../bindings/mailbox/imx-mailbox.txt | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/imx-mailbox.txt