Re: [PATCH v2, 0/5] Revert "mailbox: mediatek: remove implementation related to atomic_exec"
On Wed, Apr 14, 2021 at 3:51 AM Matthias Brugger wrote: > > > > On 12/04/2021 17:29, Jassi Brar wrote: > > On Mon, Apr 12, 2021 at 6:18 AM Yongqiang Niu > > wrote: > >> > >> This series base linux 5.12-rc2 > >> these patches will cause home ui flick when cursor moved, > >> there is no fix solution yet, revert these patches first. > >> > >> change since v1: > >> add mtk-gce.txt and dts modification > >> > >> Yongqiang Niu (5): > >> Revert "drm/mediatek: Make sure previous message done or be aborted > >> before send" > >> Revert "mailbox: mediatek: remove implementation related to > >> atomic_exec" > >> Revert "dt-bindings: mailbox: mtk-gce: fix incorrect mbox-cells value" > >> Revert "arm64: dts: mediatek: mt8183: fix gce incorrect mbox-cells > >> value" > >> arm64: dts: mediatek: mt8183: add gce information for mmsys > >> > >> .../devicetree/bindings/mailbox/mtk-gce.txt| 2 +- > >> arch/arm64/boot/dts/mediatek/mt8183.dtsi | 5 +- > >> drivers/gpu/drm/mediatek/mtk_drm_crtc.c| 1 - > >> drivers/mailbox/mtk-cmdq-mailbox.c | 80 > >> +++--- > >> 4 files changed, 76 insertions(+), 12 deletions(-) > >> > > Please break the patchsets (this and the other 3) into mailbox only > > and platform specific ones. > > Also, it would help if there are some acked/reviewed by some mtk folks > > especially when reverting patches. > > > > I'd prefer to have DT and mailbox patches together as otherwise the burden on > me > to find out which patches in the driver are needed, and to check if these got > accepted, gets higher. > I meant the patches that need to go via mailbox tree (controller) and the rest. thanks.
Re: [PATCH v6 04/34] dt-bindings: Add bindings for Keem Bay IPC driver
On Mon, Mar 8, 2021 at 2:20 PM mark gross wrote: > > On Fri, Mar 05, 2021 at 03:01:40PM -0600, Rob Herring wrote: > > On Fri, Feb 12, 2021 at 02:22:34PM -0800, mgr...@linux.intel.com wrote: > > > From: Daniele Alessandrelli > > > > > > Add DT binding documentation for the Intel Keem Bay IPC driver, which > > > > Bindings are for h/w blocks, not drivers. From a binding perspective, I > > don't really care what the driver architecture for some OS looks like. I > > continue to not understand what this h/w looks like. A block diagram > > would help as would understanding what blocks have multiple clients > > (mailboxes and xlink in particular). > I'm working to gather this info. > Do I pick the mailbox related patches (and which ones exactly) ? thanks.
Re: [PATCH] mailbox: hisilicon: Use the correct HiSilicon copyright
On Tue, Mar 30, 2021 at 2:22 AM Leo Yan wrote: > > On Tue, Mar 30, 2021 at 02:48:40PM +0800, Hao Fang wrote: > > s/Hisilicon/HiSilicon/g. > > It should use capital S, according to > > https://www.hisilicon.com/en/terms-of-use. > > > > Signed-off-by: Hao Fang > > Though the kernel has tons of "Hisilicon", for this patch: > How about one patch to convert all occurrences? thanks.
Re: [PATCH v2, 0/5] Revert "mailbox: mediatek: remove implementation related to atomic_exec"
On Mon, Apr 12, 2021 at 6:18 AM Yongqiang Niu wrote: > > This series base linux 5.12-rc2 > these patches will cause home ui flick when cursor moved, > there is no fix solution yet, revert these patches first. > > change since v1: > add mtk-gce.txt and dts modification > > Yongqiang Niu (5): > Revert "drm/mediatek: Make sure previous message done or be aborted > before send" > Revert "mailbox: mediatek: remove implementation related to > atomic_exec" > Revert "dt-bindings: mailbox: mtk-gce: fix incorrect mbox-cells value" > Revert "arm64: dts: mediatek: mt8183: fix gce incorrect mbox-cells > value" > arm64: dts: mediatek: mt8183: add gce information for mmsys > > .../devicetree/bindings/mailbox/mtk-gce.txt| 2 +- > arch/arm64/boot/dts/mediatek/mt8183.dtsi | 5 +- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c| 1 - > drivers/mailbox/mtk-cmdq-mailbox.c | 80 > +++--- > 4 files changed, 76 insertions(+), 12 deletions(-) > Please break the patchsets (this and the other 3) into mailbox only and platform specific ones. Also, it would help if there are some acked/reviewed by some mtk folks especially when reverting patches. thanks
Re: [PATCH] dt-bindings: mailbox: Add compatible for SM8350 IPCC
On Thu, Mar 25, 2021 at 1:08 AM Vinod Koul wrote: > > On 12-03-21, 11:28, Bjorn Andersson wrote: > > On Thu 11 Mar 23:12 CST 2021, Vinod Koul wrote: > > > > Adding Jassi as recipient. Please let Vinod know if you want him to > > resend this patch to you. (I send a patch for MAINTAINERS yesterday) > > Jassi, should I resend or you can pick from lore? > I will pick. thanks
[GIT PULL] Mailbox changes for v5.12
Hi Linus, The following changes since commit 92bf22614b21a2706f4993b278017e437f7785b3: Linux 5.11-rc7 (2021-02-07 13:57:38 -0800) are available in the Git repository at: git://git.linaro.org/landing-teams/working/fujitsu/integration.git tags/mailbox-v5.12 for you to fetch changes up to 6b50df2b8c208a04d44b8df5b7baaf668ceb8fc3: mailbox: arm_mhuv2: Skip calling kfree() with invalid pointer (2021-02-22 13:34:27 -0600) - sprd: fix a macro value - omap: support for K3 AM64x - tegra: fix lockdep warnings - qcom: support for SDX55 and SC8180X - arm: fixes for sparse, kfree and void return Bjorn Andersson (2): dt-bindings: mailbox: qcom: Add SC8180X APCS compatible mailbox: qcom: Add SC8180X apcs compatible Magnum Shan (1): mailbox: sprd: correct definition of SPRD_OUTBOX_FIFO_FULL Manivannan Sadhasivam (2): dt-bindings: mailbox: Add binding for SDX55 APCS mailbox: qcom: Add support for SDX55 APCS IPC Mikko Perttunen (1): mailbox: tegra-hsp: Set lockdep class dynamically Suman Anna (2): dt-bindings: mailbox: omap: Update binding for AM64x SoCs mailbox: omap: Add support for K3 AM64x SoCs Uwe Kleine-König (1): mailbox: arm_mhuv2: make remove callback return void Viresh Kumar (2): mailbox: arm_mhuv2: Fix sparse warnings mailbox: arm_mhuv2: Skip calling kfree() with invalid pointer .../devicetree/bindings/mailbox/omap-mailbox.txt | 4 +++ .../bindings/mailbox/qcom,apcs-kpss-global.yaml| 34 ++ drivers/mailbox/arm_mhuv2.c| 30 +-- drivers/mailbox/omap-mailbox.c | 6 +++- drivers/mailbox/qcom-apcs-ipc-mailbox.c| 8 - drivers/mailbox/sprd-mailbox.c | 2 +- drivers/mailbox/tegra-hsp.c| 15 ++ 7 files changed, 81 insertions(+), 18 deletions(-)
Re: [PATCH v6 03/34] mailbox: vpu-ipc-mailbox: Add support for Intel VPU IPC mailbox
On Thu, Feb 18, 2021 at 6:02 AM Alessandrelli, Daniele wrote: > > Hi Jassi, > > Thank you very much for your feedback. > > On Sun, 2021-02-14 at 22:54 -0600, Jassi Brar wrote: > > IIUIC, maybe the solution is simpler What if we set txdone_poll. > > Always return success in send_data(). And check if we overflew the > > fifo in last_tx_done(). If we did overflow, try to rewrite the data > > and check again. Return true, if not overflew this time, otherwise > > return false so that mailbox api can ask us to try again in next > > last_tx_done(). This way we can do away with the tasklet and, more > > importantly, avoid send_data() failures and retries on clients' part. > > That's a clever solution to avoid the tasklet. The only issue for us is > the automatic TX retry from the controller. I understand that's > generally a desirable feature, but in our case we'd like the client to > have full control on re-transmission attempts. > > That's because some of our data is time-sensitive. For instance, when > we process frames from a video stream we prefer dropping a frame rather > than re-transmitting it and delaying the processing of the rest. > > Now, I understand that the client can set the 'tx_block' and 'tx_tout' > channel fields to specify how long it wishes to wait, but the problem > is that our (single) channel is shared between multiple applications > having different timing requirements. That's why we prefer to let > applications deal we re-transmissions. > > Given the above, do you think it's reasonable to leave the > implementation as it is now? > (from initial analysis, the tasklet doesn't seem to affect the > performance of our use cases significantly, so we are fine with it) > Yup. It is intel specific so, hopefully, we don't have to deal with other vendors trying to support their use cases. Are you targeting the next merge window or this one? cheers.
Re: [PATCH v2] soc: mediatek: cmdq: add address shift in jump
On Thu, Jan 7, 2021 at 7:48 PM Yongqiang Niu wrote:
>
> On Wed, 2020-12-23 at 16:34 +0800, Yongqiang Niu wrote:
> > Add address shift when compose jump instruction
> > to compatible with 35bit format.
> >
> > Fixes: 0858fde496f8 ("mailbox: cmdq: variablize address shift in platform")
> >
> > Signed-off-by: Yongqiang Niu
> > Reviewed-by: Nicolas Boichat
> > ---
> > drivers/mailbox/mtk-cmdq-mailbox.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c
> > b/drivers/mailbox/mtk-cmdq-mailbox.c
> > index 5665b6e..75378e3 100644
> > --- a/drivers/mailbox/mtk-cmdq-mailbox.c
> > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c
> > @@ -168,7 +168,8 @@ static void cmdq_task_insert_into_thread(struct
> > cmdq_task *task)
> > dma_sync_single_for_cpu(dev, prev_task->pa_base,
> > prev_task->pkt->cmd_buf_size, DMA_TO_DEVICE);
> > prev_task_base[CMDQ_NUM_CMD(prev_task->pkt) - 1] =
> > - (u64)CMDQ_JUMP_BY_PA << 32 | task->pa_base;
> > + (u64)CMDQ_JUMP_BY_PA << 32 |
> > + (task->pa_base >> task->cmdq->shift_pa);
> > dma_sync_single_for_device(dev, prev_task->pa_base,
> > prev_task->pkt->cmd_buf_size,
> > DMA_TO_DEVICE);
> >
>
> hi jassi
>
> please confirm is there any question about this patch.
> if not, please apply this into next version, tks
>
I can't locate this patch in my inbox. And I can't fwd it from lkml to
myself. Please resend.
thnks.
Re: [PATCH v6 03/34] mailbox: vpu-ipc-mailbox: Add support for Intel VPU IPC mailbox
On Fri, Feb 12, 2021 at 4:23 PM wrote: > > From: Daniele Alessandrelli > > Add mailbox controller enabling inter-processor communication (IPC) > between the CPU (aka, the Application Processor - AP) and the VPU on > Intel Movidius SoCs like Keem Bay. > > The controller uses HW FIFOs to enable such communication. Specifically, > there are two FIFOs, one for the CPU and one for VPU. Each FIFO can hold > 128 entries (messages) of 32-bit each (but only 26 bits are actually > usable, since the 6 least-significant bits are reserved for the overflow > detection mechanism, more info below). > > When the Linux kernel on the AP needs to send messages to the VPU > firmware, it writes them to the VPU FIFO; similarly, when the VPU > firmware needs to send messages to the AP, it writes them to the CPU > FIFO. > > It's important to note that the AP is not the only one that can write to > the VPU FIFO: other components within the SoC can write to it too. Each > of these components (including the AP) has a unique 6-bit processor ID > associated to it. The VPU HW FIFO expects the last 6 bits of each 32-bit > FIFO entry to contain the processor ID of the sender. > > Therefore, sending a message from the AP to the VPU works as follows: > > 1. The message must be a 32-bit value with the last 6-bit set to 0 (in >practice, the message is meant to be a 32-bit address value, aligned >to 64 bytes; but this is not really relevant for this mailbox >controller). > > 2. The AP adds its processor ID to the 32-bit message being sent: >M = m | AP_ProcID > > 3. The sender writes the message (M) to the TIM_IPC_FIFO register of the >VPU FIFO. > > 4. The HW atomically checks if the FIFO is full and if not it writes it >to the actual FIFO; if the FIFO is full, the HW reads the ProcID >from M and then sets the corresponding bit of TIM_IPC_FIFO_OF_FLAG0, >to signal that the write failed, because the FIFO was full. > > 5. The AP reads the TIM_IPC_FIFO_OF_FLAG0 register and checks if the >bit corresponding to its ProcID has been set (in order to know if the >TX succeeded or failed); if the bit is set, the AP clears it. > > Note: as briefly mentioned above, the 32-bit value is meant to be a 32- > bit physical address (64-byte aligned). This address points to a > predefined struct (i.e., the IPC packet) in shared memory. However, > since this struct is not HW dependent (it's just the struct the VPU > firmware expects and in theory it could change if a different VPU FW is > used), it's not defined here, but in the Keem Bay IPC driver, which is > the mailbox client for this controller. > > The AP is notified of pending messages in the CPU FIFO by means of the > 'FIFO-not-empty' interrupt, which is generated by the CPU FIFO while not > empty. This interrupt is cleared automatically once all messages have > been read from the FIFO (i.e., the FIFO has been emptied). > > The hardware doesn't provide an TX done IRQ (i.e., an IRQ that allows > the VPU firmware to notify the AP that the message put into the VPU FIFO > has been received); however the AP can ensure that the message has been > successfully put into the VPU FIFO (and therefore transmitted) by > checking the VPU FIFO status register (TIM_IPC_FIFO_OF_FLAG0) to ensure > that writing the message didn't cause the FIFO to overflow (as described > above). > I don't remember the last time I saw such a detailed explanation! Thanks. > Therefore, the mailbox controller is configured as capable of tx_done > IRQs and a tasklet is used to simulate the tx_done IRQ. The tasklet is > activated by send_data() right after the message has been put into the > VPU FIFO and the VPU FIFO status registers has been checked. If an > overflow is reported by the status register, the tasklet passes -EBUSY > to mbox_chan_txdone(), to notify the mailbox client of the failed TX. > > The client should therefore register a tx_done() callback to properly > handle failed transmissions. > > Note: the 'txdone_poll' mechanism cannot be used because it doesn't > provide a way to report a failed transmission. > IIUIC, maybe the solution is simpler What if we set txdone_poll. Always return success in send_data(). And check if we overflew the fifo in last_tx_done(). If we did overflow, try to rewrite the data and check again. Return true, if not overflew this time, otherwise return false so that mailbox api can ask us to try again in next last_tx_done(). This way we can do away with the tasklet and, more importantly, avoid send_data() failures and retries on clients' part. Cheers!
Re: [PATCH Resend] mailbox: arm_mhuv2: Fix sparse warnings
On Tue, Feb 9, 2021 at 5:18 AM Viresh Kumar wrote: > > This patch fixes a bunch of sparse warnings in the newly added arm_mhuv2 > driver. > > drivers/mailbox/arm_mhuv2.c:506:24: warning: incorrect type in argument 1 > (different address spaces) > drivers/mailbox/arm_mhuv2.c:506:24:expected void const volatile [noderef] > __iomem *addr > drivers/mailbox/arm_mhuv2.c:506:24:got unsigned int [usertype] * > drivers/mailbox/arm_mhuv2.c:547:42: warning: incorrect type in argument 2 > (different address spaces) > drivers/mailbox/arm_mhuv2.c:547:42:expected unsigned int [usertype] *reg > drivers/mailbox/arm_mhuv2.c:547:42:got unsigned int [noderef] __iomem * > drivers/mailbox/arm_mhuv2.c:625:42: warning: incorrect type in argument 2 > (different address spaces) > drivers/mailbox/arm_mhuv2.c:625:42:expected unsigned int [usertype] *reg > drivers/mailbox/arm_mhuv2.c:625:42:got unsigned int [noderef] __iomem * > drivers/mailbox/arm_mhuv2.c:972:24: warning: dereference of noderef expression > drivers/mailbox/arm_mhuv2.c:973:22: warning: dereference of noderef expression > drivers/mailbox/arm_mhuv2.c:993:25: warning: dereference of noderef expression > drivers/mailbox/arm_mhuv2.c:1026:24: warning: dereference of noderef > expression > drivers/mailbox/arm_mhuv2.c:1027:22: warning: dereference of noderef > expression > drivers/mailbox/arm_mhuv2.c:1048:17: warning: dereference of noderef > expression > > Reported-by: kernel test robot > Signed-off-by: Viresh Kumar > --- > Hi, > > This should have been merged to 5.11-rc since the driver got introduced > in 5.11-rc1 itself. I don't see it queued for linux-next as well. It was > posted over 6 weeks back and no response is received yet: > Yup any bug fix should be sent in rc. But this, imo, lies on the boundary of code and cosmetic issues, so I practiced discretion to keep it for the next pull request lest I won't have much to send ;) > https://lore.kernel.org/lkml/db5dd593cfd8b428ce44c1cce7484d887fa5e67c.1609303304.git.viresh.ku...@linaro.org/ > > Would be good if this can still go in 5.11. > Of course it will. thanks.
Re: [PATCH v3 0/5] Add APCS support for SDX55
On Mon, Feb 8, 2021 at 12:20 PM Manivannan Sadhasivam wrote: > > On Mon, Feb 08, 2021 at 09:46:11AM -0800, Stephen Boyd wrote: > > Quoting Manivannan Sadhasivam (2021-01-17 20:11:51) > > > Changes in v2: > > > > > > * Modified the max_register value as per the SDX55 IPC offset in mailbox > > > driver. > > > > > > Manivannan Sadhasivam (5): > > > dt-bindings: mailbox: Add binding for SDX55 APCS > > > mailbox: qcom: Add support for SDX55 APCS IPC > > > > I think I can apply the clk patches to clk tree without the mailbox > > patches, right? > > > > Yes, you can. Thanks for applying! > > Jassi: Can you please look into the mailbox patches? > They are compatible strings mostly... so assume it ok. cheers!
Re: [PATCH v3 03/34] mailbox: vpu-ipc-mailbox: Add support for Intel VPU IPC mailbox
On Fri, Jan 29, 2021 at 8:21 PM wrote:
>
> From: Daniele Alessandrelli
>
> Add mailbox controller enabling inter-processor communication (IPC)
> between the CPU (aka, the Application Processor - AP) and the VPU on
> Intel Movidius SoCs like Keem Bay.
>
> The controller uses HW FIFOs to enable such communication. Specifically,
> there are two FIFOs, one for the CPU and one for VPU. Each FIFO can hold
> 128 entries (messages) of 32-bit each (but only 26 bits are actually
> usable, since the 6 least-significant bits are reserved).
>
> When the Linux kernel on the AP needs to send messages to the VPU
> firmware, it writes them to the VPU FIFO; similarly, when the VPU
> firmware needs to send messages to the AP, it writes them to the CPU
> FIFO.
>
> The AP is notified of pending messages in the CPU FIFO by means of the
> 'FIFO-not-empty' interrupt, which is generated by the CPU FIFO while not
> empty. This interrupt is cleared automatically once all messages have
> been read from the FIFO (i.e., the FIFO has been emptied).
>
> The hardware doesn't provide an TX done IRQ (i.e., an IRQ that allows
> the VPU firmware to notify the AP that the message put into the VPU FIFO
> has been received); however the AP can ensure that the message has been
> successfully put into the VPU FIFO (and therefore transmitted) by
> checking the VPU FIFO status register to ensure that writing the message
> didn't cause the FIFO to overflow.
>
> Therefore, the mailbox controller is configured as capable of tx_done
> IRQs and a tasklet is used to simulate the tx_done IRQ. The tasklet is
> activated by send_data() right after the message has been put into the
> VPU FIFO and the VPU FIFO status registers has been checked. If an
> overflow is reported by the status register, the tasklet passes -EBUSY
> to mbox_chan_txdone(), to notify the mailbox client of the failed TX.
>
> The client should therefore register a tx_done() callback to properly
> handle failed transmissions.
>
> Note: the 'txdone_poll' mechanism cannot be used because it doesn't
> provide a way to report a failed transmission.
>
txdone means the last submitted transfer has been done with --
successfully or not.
So I think we can do without the tasklet as explained below
> +static int vpu_ipc_mailbox_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct vpu_ipc_mbox *vpu_ipc_mbox = chan->con_priv;
> + u32 entry, overflow;
> +
> + entry = *((u32 *)data);
> +
Are all messages max 32bits wide?
Usually the controller specifies a packet format (more than just a
word but of course that's not mandatory) that a client submits the
data to be transmitted in. Esp when it has deep FIFOs.
> + /* Ensure last 6-bits of entry are not used. */
> + if (unlikely(entry & IPC_FIFO_ENTRY_RSVD_MASK)) {
> + vpu_ipc_mbox->txdone_result = -EINVAL;
> + goto exit;
> + }
> +
> + /* Add processor ID to entry. */
> + entry |= IPC_FIFO_ID_CPU & IPC_FIFO_ENTRY_RSVD_MASK;
> +
> + /* Write entry to VPU FIFO. */
> + iowrite32(entry, vpu_ipc_mbox->vpu_fifo_base + IPC_FIFO);
> +
> + /* Check if we overflew the VPU FIFO. */
> + overflow = ioread32(vpu_ipc_mbox->vpu_fifo_base + IPC_FIFO_OF_FLAG0) &
> + BIT(IPC_FIFO_ID_CPU);
> + if (unlikely(overflow)) {
> + /* Reset overflow register. */
> + iowrite32(BIT(IPC_FIFO_ID_CPU),
> + vpu_ipc_mbox->vpu_fifo_base + IPC_FIFO_OF_FLAG0);
> + vpu_ipc_mbox->txdone_result = -EBUSY;
> + goto exit;
> + }
> + vpu_ipc_mbox->txdone_result = 0;
> +
> +exit:
> + /* Schedule tasklet to call mbox_chan_txdone(). */
> + tasklet_schedule(_ipc_mbox->txdone_tasklet);
> +
> + return 0;
> +}
> +
Maybe set txdone_poll and implement last_tx_done() where you can wait
for FIFO to have enough space for another message, so that the next
submitted request will never return -EBUSY.
thanks
Re: [PATCH] add chan->cl check in mbox_chan_received_data()
On Thu, Jan 7, 2021 at 5:53 AM haidong yao wrote: > > Hi Jassi Brar > > Thank you very much for your reply. > > Look at the function sprd_mbox_outbox_isr . > > Chan is !NULL. > > chan->cl is NULL when the client driver not loaded, the controller > driver don't know the client driver loaded successfully, so, I do not > use mbox_free_channel. > > Here,How do you know chan->cl is ok? > The channel is supposed to get/send data _only_ if it is being used by a client. Which you can mark in .startup() and .shutdown(). Checking for chan->cl will make your symptoms disappear but that is not the right fix for the issue. The right fix is to EITHER not cause Rx/Tx interrupt on a channel not being used, OR not send it to upper layers. thanks.
Re: [PATCH] mailbox: bcm: Replace tasklet with threaded irq
On Thu, Jan 14, 2021 at 6:21 PM Davidlohr Bueso wrote: > > Tasklets have long been deprecated as being too heavy on the system > by running in irq context - and this is not a performance critical > path. If a higher priority process wants to run, it must wait for > the tasklet to finish before doing so. > > Use a more suitable alternative such as threaded irqs and do the > async work in process context. > > Signed-off-by: Davidlohr Bueso > --- Please cc the author and other contributors to this file, esp when this is vendor specific code. thanks.
Re: [PATCH] dt-bindings: Drop redundant maxItems/items
On Mon, Dec 21, 2020 at 10:10 PM Rob Herring wrote: > > 'maxItems' equal to the 'items' list length is redundant. 'maxItems' is > preferred for a single entry while greater than 1 should have an 'items' > list. > > A meta-schema check for this is pending once these existing cases are > fixed. > > Cc: Laurent Pinchart > Cc: Vinod Koul > Cc: Mark Brown > Cc: Greg Kroah-Hartman > Cc: Jassi Brar > Cc: dri-de...@lists.freedesktop.org > Cc: dmaeng...@vger.kernel.org > Cc: alsa-de...@alsa-project.org > Cc: linux-...@vger.kernel.org > Signed-off-by: Rob Herring > --- > .../devicetree/bindings/display/xlnx/xlnx,zynqmp-dpsub.yaml| 1 - > Documentation/devicetree/bindings/dma/renesas,rcar-dmac.yaml | 1 - > Documentation/devicetree/bindings/mailbox/arm,mhu.yaml | 1 - > .../devicetree/bindings/sound/nvidia,tegra30-hda.yaml | 2 -- > Documentation/devicetree/bindings/usb/renesas,usb-xhci.yaml| 1 - > Documentation/devicetree/bindings/usb/renesas,usbhs.yaml | 3 --- > 6 files changed, 9 deletions(-) Acked-by: Jassi Brar
[GIT PULL] Mailbox changes for v5.11]
Hi Linus, The following changes since commit 509a15421674b9e1a3e1916939d0d0efd3e578da: Merge tag '5.10-rc6-smb3-fixes' of git://git.samba.org/sfrench/cifs-2.6 (2020-12-01 15:43:53 -0800) are available in the Git repository at: git://git.linaro.org/landing-teams/working/fujitsu/integration.git tags/mailbox-v5.11 for you to fetch changes up to 5a6338cce9f4133c478d3b10b300f96dd644379a: mailbox: arm_mhuv2: Add driver (2020-12-09 19:26:02 -0600) - arm: added mhu-v2 controller driver mhu_db fix kfree by using devm_ variant - stm32-ipcc: misc cleanup Martin Kaiser (3): mailbox: stm32-ipcc: add COMPILE_TEST dependency mailbox: stm32-ipcc: remove duplicate error message mailbox: stm32-ipcc: cast void pointers to unsigned long Sudeep Holla (1): mailbox: arm_mhu_db: Fix mhu_db_shutdown by replacing kfree with devm_kfree Viresh Kumar (2): dt-bindings: mailbox : arm,mhuv2: Add bindings mailbox: arm_mhuv2: Add driver .../devicetree/bindings/mailbox/arm,mhuv2.yaml | 209 MAINTAINERS|9 + drivers/mailbox/Kconfig|9 +- drivers/mailbox/Makefile |2 + drivers/mailbox/arm_mhu_db.c |2 +- drivers/mailbox/arm_mhuv2.c| 1136 drivers/mailbox/stm32-ipcc.c | 15 +- include/linux/mailbox/arm_mhuv2_message.h | 20 + 8 files changed, 1391 insertions(+), 11 deletions(-) create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhuv2.yaml create mode 100644 drivers/mailbox/arm_mhuv2.c create mode 100644 include/linux/mailbox/arm_mhuv2_message.h
Re: [PATCH] add chan->cl check in mbox_chan_received_data()
On Tue, Dec 15, 2020 at 2:48 AM Haidong Yao wrote:
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -152,7 +152,7 @@ static enum hrtimer_restart txdone_hrtimer(struct hrtimer
> *hrtimer)
> void mbox_chan_received_data(struct mbox_chan *chan, void *mssg)
> {
> /* No buffering the received data */
> - if (chan->cl->rx_callback)
> + if (chan->cl && chan->cl->rx_callback)
> chan->cl->rx_callback(chan->cl, mssg);
> }
The proper fix is in the controller driver. Which should stop tx/rx
when the channel is freed.
thnx.
Re: [PATCH 02/22] dt-bindings: Add bindings for Keem Bay IPC driver
On Tue, Dec 8, 2020 at 6:12 PM mark gross wrote: > > On Mon, Dec 07, 2020 at 02:31:37PM -0600, Jassi Brar wrote: > > On Mon, Dec 7, 2020 at 12:43 PM Daniele Alessandrelli > > wrote: > > > > > > Hi Rob, > > > > > > Thanks for the feedback. > > > > > > On Mon, 2020-12-07 at 10:01 -0600, Rob Herring wrote: > > > > On Tue, Dec 01, 2020 at 02:34:51PM -0800, mgr...@linux.intel.com wrote: > > > > > From: Daniele Alessandrelli > > > > > > > > > > Add DT binding documentation for the Intel Keem Bay IPC driver, which > > > > > enables communication between the Computing Sub-System (CSS) and the > > > > > Multimedia Sub-System (MSS) of the Intel Movidius SoC code named Keem > > > > > Bay. > > > > > > > > > > > [cut] > > > > > > > > + > > > > > +description: > > > > > + The Keem Bay IPC driver enables Inter-Processor Communication > > > > > (IPC) with the > > > > > + Visual Processor Unit (VPU) embedded in the Intel Movidius SoC > > > > > code named > > > > > + Keem Bay. > > > > > > > > Sounds like a mailbox. > > > > > > We did consider using the mailbox framework, but eventually decided > > > against it; mainly because of the following two reasons: > > > > > > 1. The channel concept in the Mailbox framework is different than the > > >channel concept in Keem Bay IPC: > > > > > >a. My understanding is that Mailbox channels are meant to be SW > > > representation of physical HW channels, while in Keem Bay IPC > > > channels are software abstractions to achieve communication > > > multiplexing over a single HW link > > > > > In mailbox api, that would be a physical channel shared between various > > clients. > > > > >b. Additionally, Keem Bay IPC has two different classes of channels > > > (high-speed channels and general-purpose channels) that need to > > > access the same HW link with different priorities. > > > > > If the priorities are hard (programmed into some register), you could > > do that via dt during channel population. > > If they are soft, that would be handled in the shared channel > > implementation. > > > > > 2. The blocking / non-blocking TX behavior of mailbox channels is > > >defined at channel creation time (by the tx_block value of the > > >mailbox client passed to mbox_request_channel(); > > > > > No, that is checked at mbox_send_message() > > > > > my understanding > > >is that the tx_block value cannot be modified after the channel is > > >created), > > > > > Again no. If you don't queue more than one message at any time you can > > change it between transfers. To be safe you can always change it > > between channel release - request calls. > > > > > while in Keem Bay IPC the same channel can be used for > > >both blocking and non-blocking TX (behavior is controlled by the > > >timeout argument passed to keembay_ipc_send()). > > > > > > Having said that, I guess that it could be possible to create a Mailbox > > > driver implementing the core communication mechanism used by the Keem > > > Bay IPC and then build our API around it (basically having two > > > drivers). But I'm not sure that would make the code simpler or easier > > > to maintain. Any thoughts on this? > > > > > I think so. Most of KeemBay specific behaviour would be implemented in > > the shared channel above the mailbox api. > > Quick question. By "I think so" > >From what I have read, it seems it should be doable as a mailbox. > do you mean that you feel the keem bay IPC > code will be simpler and easier to maintain if we make yet another driver at > the > Keem Bay IPC driver sits on top off? Or, the current implementation would be > simpler if we rework the implementation to use the mailbox api? > Most of the queue management code would be taken over my mailbox api. And you'll have separated out the pure controller driver from protocol assumptions. So there shouldn't be extra code... just this code reorganised and some reduced. cheers!
Re: [PATCH 02/22] dt-bindings: Add bindings for Keem Bay IPC driver
On Mon, Dec 7, 2020 at 12:43 PM Daniele Alessandrelli wrote: > > Hi Rob, > > Thanks for the feedback. > > On Mon, 2020-12-07 at 10:01 -0600, Rob Herring wrote: > > On Tue, Dec 01, 2020 at 02:34:51PM -0800, mgr...@linux.intel.com wrote: > > > From: Daniele Alessandrelli > > > > > > Add DT binding documentation for the Intel Keem Bay IPC driver, which > > > enables communication between the Computing Sub-System (CSS) and the > > > Multimedia Sub-System (MSS) of the Intel Movidius SoC code named Keem > > > Bay. > > > > > [cut] > > > > + > > > +description: > > > + The Keem Bay IPC driver enables Inter-Processor Communication (IPC) > > > with the > > > + Visual Processor Unit (VPU) embedded in the Intel Movidius SoC code > > > named > > > + Keem Bay. > > > > Sounds like a mailbox. > > We did consider using the mailbox framework, but eventually decided > against it; mainly because of the following two reasons: > > 1. The channel concept in the Mailbox framework is different than the >channel concept in Keem Bay IPC: > >a. My understanding is that Mailbox channels are meant to be SW > representation of physical HW channels, while in Keem Bay IPC > channels are software abstractions to achieve communication > multiplexing over a single HW link > In mailbox api, that would be a physical channel shared between various clients. >b. Additionally, Keem Bay IPC has two different classes of channels > (high-speed channels and general-purpose channels) that need to > access the same HW link with different priorities. > If the priorities are hard (programmed into some register), you could do that via dt during channel population. If they are soft, that would be handled in the shared channel implementation. > 2. The blocking / non-blocking TX behavior of mailbox channels is >defined at channel creation time (by the tx_block value of the >mailbox client passed to mbox_request_channel(); > No, that is checked at mbox_send_message() > my understanding >is that the tx_block value cannot be modified after the channel is >created), > Again no. If you don't queue more than one message at any time you can change it between transfers. To be safe you can always change it between channel release - request calls. > while in Keem Bay IPC the same channel can be used for >both blocking and non-blocking TX (behavior is controlled by the >timeout argument passed to keembay_ipc_send()). > > Having said that, I guess that it could be possible to create a Mailbox > driver implementing the core communication mechanism used by the Keem > Bay IPC and then build our API around it (basically having two > drivers). But I'm not sure that would make the code simpler or easier > to maintain. Any thoughts on this? > I think so. Most of KeemBay specific behaviour would be implemented in the shared channel above the mailbox api. cheers!
Re: [PATCH V5 2/2] mailbox: arm_mhuv2: Add driver
On Tue, Nov 17, 2020 at 4:02 AM Viresh Kumar wrote:
> +
> +/**
> + * struct mhuv2 - MHUv2 mailbox controller data
> + *
> + * @mbox: Mailbox controller belonging to the MHU frame.
> + * @send/recv: Base address of the register mapping region.
> + * @frame: Frame type: RECEIVER_FRAME or SENDER_FRAME.
> + * @irq: Interrupt.
> + * @windows: Channel windows implemented by the platform.
> + * @minor: Minor version of the controller.
> + * @length:Length of the protocols array in bytes.
> + * @protocols: Raw protocol information, derived from device tree.
> + * @doorbell_pending_lock: spinlock required for correct operation of Tx
> + * interrupt for doorbells.
> + */
> +struct mhuv2 {
> + struct mbox_controller mbox;
> + union {
> + struct mhu2_send_frame_reg __iomem *send;
> + struct mhu2_recv_frame_reg __iomem *recv;
> + };
> + enum mhuv2_frame frame;
> + unsigned int irq;
> + unsigned int windows;
> + unsigned int minor;
> + unsigned int length;
> + u32 *protocols;
> +
> + spinlock_t doorbell_pending_lock;
>
Can you please explain the need of this lock? Some usecase?
It should be unnecessary if the controller natively supports doorbell mode.
thanks.
Re: [PATCH] mailbox: arm_mhuv2: Add driver
On Wed, Nov 11, 2020 at 12:02 AM Viresh Kumar wrote: > > On 27-10-20, 17:23, Viresh Kumar wrote: > > This adds driver for the ARM MHUv2 (Message Handling Unit) mailbox > > controller. > > > > This is based on the accepted DT bindings of the controller and supports > > combination of all transport protocols, i.e. single-word, multi-word and > > doorbell. > > > > Transmitting and receiving data through the mailbox framework in > > multi-word mode is done through struct arm_mhuv2_mbox_msg. Rest of the > > implementation details can be seen in the bindings document or in the > > driver itself. > > > > Based on the initial work done by Morten Borup Petersen from ARM. > > > > Co-developed-by: Tushar Khandelwal > > Signed-off-by: Tushar Khandelwal > > Tested-by: Usama Arif > > Signed-off-by: Viresh Kumar > > --- > > Bindings are already reviewed by Rob and are present here: > > > > http://lore.kernel.org/lkml/61ca14fc441f92c1e7994e5bebae5c49811a3050.1602563406.git.viresh.ku...@linaro.org > > Jassi, Any inputs on this ? > Can we make 'single-word' as a special case of 'multi-word', i.e, effect single-word by specifying 1 sized multi-word. And the names of structs and members could be lower case? It seems very firmware style, or is it just me? cheers.
[GIT PULL] Mailbox changes for v5.10
Hi Linus, The following changes since commit 549738f15da0e5a00275977623be199fbbf7df50: Linux 5.9-rc8 (2020-10-04 16:04:34 -0700) are available in the Git repository at: git://git.linaro.org/landing-teams/working/fujitsu/integration.git tags/mailbox-v5.10 for you to fetch changes up to c7dacf5b0f32957b24ef29df1207dc2cd8307743: mailbox: avoid timer start from callback (2020-10-16 19:09:17 -0500) - arm: implementation of mhu as a doorbell driver conversion of dt-bindings to json-schema - mediatek: fix platform_get_irq error handling - bcm: convert tasklets to use new tasklet_setup api - core: fix race cause by hrtimer starting inappropriately Allen Pais (1): mailbox: bcm: convert tasklets to use new tasklet_setup() API Jassi Brar (1): mailbox: avoid timer start from callback Krzysztof Kozlowski (1): maiblox: mediatek: Fix handling of platform_get_irq() error Sudeep Holla (3): dt-bindings: mailbox: add doorbell support to ARM MHU mailbox: arm_mhu: Match only if compatible is "arm,mhu" mailbox: arm_mhu: Add ARM MHU doorbell driver Viresh Kumar (1): dt-bindings: mailbox : arm,mhu: Convert to Json-schema .../devicetree/bindings/mailbox/arm,mhu.yaml | 135 .../devicetree/bindings/mailbox/arm-mhu.txt| 43 --- drivers/mailbox/Makefile | 2 +- drivers/mailbox/arm_mhu.c | 3 + drivers/mailbox/arm_mhu_db.c | 354 + drivers/mailbox/bcm-pdc-mailbox.c | 6 +- drivers/mailbox/mailbox.c | 12 +- drivers/mailbox/mtk-cmdq-mailbox.c | 8 +- 8 files changed, 506 insertions(+), 57 deletions(-) create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhu.yaml delete mode 100644 Documentation/devicetree/bindings/mailbox/arm-mhu.txt create mode 100644 drivers/mailbox/arm_mhu_db.c
Re: [PATCH] mailbox: cancel timer before starting it
On Fri, Oct 16, 2020 at 1:54 PM Jerome Brunet wrote:
>
>
> On Fri 16 Oct 2020 at 19:33, Jassi Brar wrote:
>
> > On Fri, Oct 16, 2020 at 4:00 AM Jerome Brunet wrote:
> >>
> >>
> >> On Fri 16 Oct 2020 at 10:52, Ionela Voinescu
> >> wrote:
> >>
> >> > On Thursday 15 Oct 2020 at 13:45:54 (-0500), Jassi Brar wrote:
> >> > [..]
> >> >> > >> --- a/drivers/mailbox/mailbox.c
> >> >> > >> +++ b/drivers/mailbox/mailbox.c
> >> >> > >> @@ -82,9 +82,13 @@ static void msg_submit(struct mbox_chan *chan)
> >> >> > >> exit:
> >> >> > >> spin_unlock_irqrestore(>lock, flags);
> >> >> > >>
> >> >> > >> -if (!err && (chan->txdone_method & TXDONE_BY_POLL))
> >> >> > >> -/* kick start the timer immediately to avoid delays */
> >> >> > >> +if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
> >> >> > >> +/* Disable the timer if already active ... */
> >> >> > >> +hrtimer_cancel(>mbox->poll_hrt);
> >> >> > >> +
> >> >> > >> +/* ... and kick start it immediately to avoid delays
> >> >> > >> */
> >> >> > >> hrtimer_start(>mbox->poll_hrt, 0,
> >> >> > >> HRTIMER_MODE_REL);
> >> >> > >> +}
> >> >> > >> }
> >> >> > >>
> >> >> > >> static void tx_tick(struct mbox_chan *chan, int r)
> >> >> > >
> >> >> > > I've tracked a regression back to this commit. Details to reproduce:
> >> >> >
> >> >> > Hi Ionela,
> >> >> >
> >> >> > I don't have access to your platform and I don't get what is going on
> >> >> > from the log below.
> >> >> >
> >> >> > Could you please give us a bit more details about what is going on ?
> >> >> >
> >> >> > All this patch does is add hrtimer_cancel().
> >> >> > * It is needed if the timer had already been started, which is
> >> >> > appropriate AFAIU
> >> >> > * It is a NO-OP is the timer is not active.
> >> >> >
> >> >> Can you please try using hrtimer_try_to_cancel() instead ?
> >> >>
> >> >
> >> > Yes, using hrtimer_try_to_cancel() instead works for me. But doesn't
> >> > this limit how effective this change is? AFAIU, this will possibly only
> >> > reduce the chances for the race condition, but not solve it.
> >> >
> >>
> >> It is also my understanding, hrtimer_try_to_cancel() would remove a
> >> timer which as not already started but would return withtout doing
> >> anything if the callback is already running ... which is the original
> >> problem
> >>
> > If we are running in the callback path, hrtimer_try_to_cancel will
> > return -1, in which case we could skip hrtimer_start.
> > Anyways, I think simply checking for hrtimer_active should effect the same.
> > I have submitted a patch, of course not tested.
>
> Yes it sloves this race but ...
>
Thanks for confirmation.
> If a race is possible between a timer callback rescheduling itself (which
> is not that uncommon) and another thread trying to cancel it
>
In our case, we should not be cancelling+restarting the timer in the
first place, because txdone_hrtimer will take care of it via
hrtimer_forward_now.
>, maybe
> there is something worth fixing in hrtimer ? Also, mailbox calls
> hrtimer_cancel() in unregister ... are we confident this would work ?
>
Yes. After unregister() every channel is supposed to die and so must
its resources.
-jassi
Re: [PATCH] mailbox: avoid timer start from callback
On Fri, Oct 16, 2020 at 1:38 PM Jerome Brunet wrote:
>
>
> On Fri 16 Oct 2020 at 19:30, jassisinghb...@gmail.com wrote:
>
> > From: Jassi Brar
> >
> > If the txdone is done by polling, it is possible for msg_submit() to start
> > the timer while txdone_hrtimer() callback is running. If the timer needs
> > recheduling, it could already be enqueued by the time hrtimer_forward_now()
> > is called, leading hrtimer to loudly complain.
> >
> > WARNING: CPU: 3 PID: 74 at kernel/time/hrtimer.c:932
> > hrtimer_forward+0xc4/0x110
> > CPU: 3 PID: 74 Comm: kworker/u8:1 Not tainted
> > 5.9.0-rc2-00236-gd3520067d01c-dirty #5
> > Hardware name: Libre Computer AML-S805X-AC (DT)
> > Workqueue: events_freezable_power_ thermal_zone_device_check
> > pstate: 2085 (nzCv daIf -PAN -UAO BTYPE=--)
> > pc : hrtimer_forward+0xc4/0x110
> > lr : txdone_hrtimer+0xf8/0x118
> > [...]
> >
> > This can be fixed by not starting the timer from the callback path. Which
> > requires the timer reloading as long as any message is queued on the
> > channel, and not just when current tx is not done yet.
> >
> > Signed-off-by: Jassi Brar
> > ---
> > drivers/mailbox/mailbox.c | 12 +++-
> > 1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> > index 0b821a5b2db8..a093a6ecaa66 100644
> > --- a/drivers/mailbox/mailbox.c
> > +++ b/drivers/mailbox/mailbox.c
> > @@ -82,9 +82,12 @@ static void msg_submit(struct mbox_chan *chan)
> > exit:
> > spin_unlock_irqrestore(>lock, flags);
> >
> > - if (!err && (chan->txdone_method & TXDONE_BY_POLL))
> > - /* kick start the timer immediately to avoid delays */
> > - hrtimer_start(>mbox->poll_hrt, 0, HRTIMER_MODE_REL);
> > + /* kick start the timer immediately to avoid delays */
> > + if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
> > + /* but only if not already active */
>
> It would solve the problem I reported as well but instead of running the
> check immediately (timer with value 0), we will have to wait for the
> next of the timer, it is already started. IOW, there might be a delay
> now. I don't know if this important for the mailbox - the existing
> comments in the code suggested it was.
>
That comment is for when the first message is queued on the channel,
which remains unimpacted.
So, do I have your tested/acked by ?
thnx,
Re: [PATCH] mailbox: avoid timer start from callback
On Fri, Oct 16, 2020 at 12:50 PM Sudeep Holla wrote: > > On Fri, Oct 16, 2020 at 12:30:20PM -0500, jassisinghb...@gmail.com wrote: > > From: Jassi Brar > > > > If the txdone is done by polling, it is possible for msg_submit() to start > > the timer while txdone_hrtimer() callback is running. If the timer needs > > recheduling, it could already be enqueued by the time hrtimer_forward_now() > > is called, leading hrtimer to loudly complain. > > > > WARNING: CPU: 3 PID: 74 at kernel/time/hrtimer.c:932 > > hrtimer_forward+0xc4/0x110 > > CPU: 3 PID: 74 Comm: kworker/u8:1 Not tainted > > 5.9.0-rc2-00236-gd3520067d01c-dirty #5 > > Hardware name: Libre Computer AML-S805X-AC (DT) > > Workqueue: events_freezable_power_ thermal_zone_device_check > > pstate: 2085 (nzCv daIf -PAN -UAO BTYPE=--) > > pc : hrtimer_forward+0xc4/0x110 > > lr : txdone_hrtimer+0xf8/0x118 > > [...] > > > > This can be fixed by not starting the timer from the callback path. Which > > requires the timer reloading as long as any message is queued on the > > channel, and not just when current tx is not done yet. > > > > I came to similar conclusion and was testing something similar. You bet > me. Since we have single timer and multiple channels, each time a message > is enqueued on any channel, timer gets added which is wrong. > > Reviewed-by: Sudeep Holla > > I tested this patch too by reverting offending commit in -next, so > > Tested-by: Sudeep Holla > > You seem to have dropped the Fixes tags. Is that intentional ? If so, > any particular reasons. I think it is stable material and better to have > fixes tag so that it gets added to stable trees. > Thanks for testing. I will decorate it appropriately once I have Jerome's tested-by too. -jassi
Re: [PATCH] mailbox: cancel timer before starting it
On Fri, Oct 16, 2020 at 4:00 AM Jerome Brunet wrote:
>
>
> On Fri 16 Oct 2020 at 10:52, Ionela Voinescu wrote:
>
> > On Thursday 15 Oct 2020 at 13:45:54 (-0500), Jassi Brar wrote:
> > [..]
> >> > >> --- a/drivers/mailbox/mailbox.c
> >> > >> +++ b/drivers/mailbox/mailbox.c
> >> > >> @@ -82,9 +82,13 @@ static void msg_submit(struct mbox_chan *chan)
> >> > >> exit:
> >> > >> spin_unlock_irqrestore(>lock, flags);
> >> > >>
> >> > >> -if (!err && (chan->txdone_method & TXDONE_BY_POLL))
> >> > >> -/* kick start the timer immediately to avoid delays */
> >> > >> +if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
> >> > >> +/* Disable the timer if already active ... */
> >> > >> +hrtimer_cancel(>mbox->poll_hrt);
> >> > >> +
> >> > >> +/* ... and kick start it immediately to avoid delays */
> >> > >> hrtimer_start(>mbox->poll_hrt, 0,
> >> > >> HRTIMER_MODE_REL);
> >> > >> +}
> >> > >> }
> >> > >>
> >> > >> static void tx_tick(struct mbox_chan *chan, int r)
> >> > >
> >> > > I've tracked a regression back to this commit. Details to reproduce:
> >> >
> >> > Hi Ionela,
> >> >
> >> > I don't have access to your platform and I don't get what is going on
> >> > from the log below.
> >> >
> >> > Could you please give us a bit more details about what is going on ?
> >> >
> >> > All this patch does is add hrtimer_cancel().
> >> > * It is needed if the timer had already been started, which is
> >> > appropriate AFAIU
> >> > * It is a NO-OP is the timer is not active.
> >> >
> >> Can you please try using hrtimer_try_to_cancel() instead ?
> >>
> >
> > Yes, using hrtimer_try_to_cancel() instead works for me. But doesn't
> > this limit how effective this change is? AFAIU, this will possibly only
> > reduce the chances for the race condition, but not solve it.
> >
>
> It is also my understanding, hrtimer_try_to_cancel() would remove a
> timer which as not already started but would return withtout doing
> anything if the callback is already running ... which is the original
> problem
>
If we are running in the callback path, hrtimer_try_to_cancel will
return -1, in which case we could skip hrtimer_start.
Anyways, I think simply checking for hrtimer_active should effect the same.
I have submitted a patch, of course not tested.
Thanks
Re: [PATCH] mailbox: cancel timer before starting it
On Thu, Oct 15, 2020 at 8:58 AM Jerome Brunet wrote:
>
>
> On Thu 15 Oct 2020 at 15:46, Ionela Voinescu wrote:
>
> > Hi guys,
> >
> > On Wednesday 23 Sep 2020 at 14:39:16 (+0200), Jerome Brunet wrote:
> >> If the txdone is done by polling, it is possible for msg_submit() to start
> >> the timer while txdone_hrtimer() callback is running. If the timer needs
> >> recheduling, it could already be enqueued by the time hrtimer_forward_now()
> >> is called, leading hrtimer to loudly complain.
> >>
> >> WARNING: CPU: 3 PID: 74 at kernel/time/hrtimer.c:932
> >> hrtimer_forward+0xc4/0x110
> >> CPU: 3 PID: 74 Comm: kworker/u8:1 Not tainted
> >> 5.9.0-rc2-00236-gd3520067d01c-dirty #5
> >> Hardware name: Libre Computer AML-S805X-AC (DT)
> >> Workqueue: events_freezable_power_ thermal_zone_device_check
> >> pstate: 2085 (nzCv daIf -PAN -UAO BTYPE=--)
> >> pc : hrtimer_forward+0xc4/0x110
> >> lr : txdone_hrtimer+0xf8/0x118
> >> [...]
> >>
> >> Canceling the timer before starting it ensure that the timer callback is
> >> not running when the timer is started, solving this race condition.
> >>
> >> Fixes: 0cc67945ea59 ("mailbox: switch to hrtimer for tx_complete polling")
> >> Reported-by: Da Xue
> >> Signed-off-by: Jerome Brunet
> >> ---
> >> drivers/mailbox/mailbox.c | 8 ++--
> >> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> >> index 0b821a5b2db8..34f9ab01caef 100644
> >> --- a/drivers/mailbox/mailbox.c
> >> +++ b/drivers/mailbox/mailbox.c
> >> @@ -82,9 +82,13 @@ static void msg_submit(struct mbox_chan *chan)
> >> exit:
> >> spin_unlock_irqrestore(>lock, flags);
> >>
> >> -if (!err && (chan->txdone_method & TXDONE_BY_POLL))
> >> -/* kick start the timer immediately to avoid delays */
> >> +if (!err && (chan->txdone_method & TXDONE_BY_POLL)) {
> >> +/* Disable the timer if already active ... */
> >> +hrtimer_cancel(>mbox->poll_hrt);
> >> +
> >> +/* ... and kick start it immediately to avoid delays */
> >> hrtimer_start(>mbox->poll_hrt, 0, HRTIMER_MODE_REL);
> >> +}
> >> }
> >>
> >> static void tx_tick(struct mbox_chan *chan, int r)
> >
> > I've tracked a regression back to this commit. Details to reproduce:
>
> Hi Ionela,
>
> I don't have access to your platform and I don't get what is going on
> from the log below.
>
> Could you please give us a bit more details about what is going on ?
>
> All this patch does is add hrtimer_cancel().
> * It is needed if the timer had already been started, which is
> appropriate AFAIU
> * It is a NO-OP is the timer is not active.
>
Can you please try using hrtimer_try_to_cancel() instead ?
thanks
Re: [PATCH 4/4] mailbox: arm_mhu: Add ARM MHU doorbell driver
On Wed, Oct 7, 2020 at 6:40 AM Sudeep Holla wrote:
>
> On Fri, Oct 02, 2020 at 02:42:37PM -0500, Jassi Brar wrote:
> > On Mon, Sep 28, 2020 at 6:45 AM Sudeep Holla wrote:
> >
> > > +
> > > +static void mhu_db_shutdown(struct mbox_chan *chan)
> > > +{
> > > + struct mhu_db_channel *chan_info = chan->con_priv;
> > > + struct mbox_controller *mbox = _info->mhu->mbox;
> > > + int i;
> > > +
> > > + for (i = 0; i < mbox->num_chans; i++)
> > > + if (chan == >chans[i])
> > > + break;
> > > +
> > > + if (mbox->num_chans == i) {
> > > + dev_warn(mbox->dev, "Request to free non-existent
> > > channel\n");
> > > + return;
> > > + }
> > > +
> > > + /* Reset channel */
> > > + mhu_db_mbox_clear_irq(chan);
> > > + chan->con_priv = NULL;
> > >
> > request->free->request will fail because of this NULL assignment.
> > Maybe add a 'taken' flag in mhu_db_channel, which should also be
> > checked before calling mbox_chan_received_data because the data may
> > arrive for a now relinquished channel.
> >
>
> Good point, but the new 'taken' flag will have the same race as con_priv.
> We need a lock here and can we use chan->lock or do you prefer this
> driver maintains it own for this purpose.
>
I meant the con_priv is allocated in mhu_db_mbox_xlate and simply
assigning it NULL leaks memory, if not a crash by some other path. At
least free it before.
-j
Re: [PATCH 0/4] Mediatek DRM driver detect CMDQ execution timeout by vblank IRQ
On Sun, Sep 27, 2020 at 6:04 PM Chun-Kuang Hu wrote: > > CMDQ helper provide timer to detect execution timeout, but DRM driver > could have a better way to detect execution timeout by vblank IRQ. > For DRM, CMDQ command should execute in vblank, so if it fail to > execute in next 2 vblank, timeout happen. Even though we could > calculate time between 2 vblank and use timer to delect, this would > make things more complicated. > > This introduce a series refinement for CMDQ mailbox controller and CMDQ > helper. Remove timer handler in helper function because different > client have different way to detect timeout. Use standard mailbox > callback instead of proprietary one to get the necessary data > in callback function. Remove struct cmdq_client to access client > instance data by struct mbox_client. > > Chun-Kuang Hu (4): > soc / drm: mediatek: cmdq: Remove timeout handler in helper function > mailbox / soc / drm: mediatek: Use mailbox rx_callback instead of > cmdq_task_cb > mailbox / soc / drm: mediatek: Remove struct cmdq_client > drm/mediatek: Detect CMDQ execution timeout > > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 54 ++--- > drivers/mailbox/mtk-cmdq-mailbox.c | 24 ++-- > drivers/soc/mediatek/mtk-cmdq-helper.c | 146 ++- > include/linux/mailbox/mtk-cmdq-mailbox.h | 25 +--- > include/linux/soc/mediatek/mtk-cmdq.h| 54 + > 5 files changed, 66 insertions(+), 237 deletions(-) > Please break this into two patchsets - one for mailbox and one for its users. Also, CC original author and recent major contributors to mtk-cmdq-mailbox.c Thanks.
Re: [PATCH 4/4] mailbox: arm_mhu: Add ARM MHU doorbell driver
On Mon, Sep 28, 2020 at 6:45 AM Sudeep Holla wrote:
> +
> +static void mhu_db_shutdown(struct mbox_chan *chan)
> +{
> + struct mhu_db_channel *chan_info = chan->con_priv;
> + struct mbox_controller *mbox = _info->mhu->mbox;
> + int i;
> +
> + for (i = 0; i < mbox->num_chans; i++)
> + if (chan == >chans[i])
> + break;
> +
> + if (mbox->num_chans == i) {
> + dev_warn(mbox->dev, "Request to free non-existent channel\n");
> + return;
> + }
> +
> + /* Reset channel */
> + mhu_db_mbox_clear_irq(chan);
> + chan->con_priv = NULL;
>
request->free->request will fail because of this NULL assignment.
Maybe add a 'taken' flag in mhu_db_channel, which should also be
checked before calling mbox_chan_received_data because the data may
arrive for a now relinquished channel.
> +
> +static struct mbox_chan *mhu_db_mbox_xlate(struct mbox_controller *mbox,
> + const struct of_phandle_args *spec)
> +{
> + struct arm_mhu *mhu = dev_get_drvdata(mbox->dev);
> + struct mhu_db_channel *chan_info;
> + struct mbox_chan *chan = NULL;
> + unsigned int pchan = spec->args[0];
> + unsigned int doorbell = spec->args[1];
> + int i;
> +
> + /* Bounds checking */
> + if (pchan >= MHU_CHANS || doorbell >= MHU_NUM_DOORBELLS) {
> + dev_err(mbox->dev,
> + "Invalid channel requested pchan: %d doorbell: %d\n",
> + pchan, doorbell);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + for (i = 0; i < mbox->num_chans; i++) {
> + chan_info = mbox->chans[i].con_priv;
> +
> + /* Is requested channel free? */
> + if (chan_info &&
> + mbox->dev == chan_info->mhu->dev &&
> + pchan == chan_info->pchan &&
> + doorbell == chan_info->doorbell) {
> + dev_err(mbox->dev, "Channel in use\n");
> + return ERR_PTR(-EBUSY);
> + }
> +
You may want to reuse mhu_db_mbox_to_channel.
Re: [PATCH V3 1/2] dt-bindings: mailbox : arm,mhu: Convert to Json-schema
On Tue, Sep 15, 2020 at 2:35 PM Rob Herring wrote: > > On Thu, Sep 10, 2020 at 03:25:18PM +0530, Viresh Kumar wrote: > > Convert the DT binding over to Json-schema. > > > > Signed-off-by: Viresh Kumar > > --- > > V3: New patch. > > > > .../devicetree/bindings/mailbox/arm,mhu.yaml | 86 +++ > > .../devicetree/bindings/mailbox/arm-mhu.txt | 43 -- > > 2 files changed, 86 insertions(+), 43 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/mailbox/arm,mhu.yaml > > delete mode 100644 Documentation/devicetree/bindings/mailbox/arm-mhu.txt > > > > diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml > > b/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml > > new file mode 100644 > > index ..4e840cedb2e4 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mailbox/arm,mhu.yaml > > @@ -0,0 +1,86 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mailbox/arm,mhu.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ARM MHU Mailbox Controller > > + > > +maintainers: > > + - Jassi Brar > > Ideally, this should be someone familiar with the h/w, not the subsystem > maintainer. Sudeep or you in this case? > If you are ok with the content of this file, maybe I am not as unfamiliar with MHU as you think :D I wrote the original doc/binding that this yaml is translated from. Maybe Viresh/Sudeep should be the maintainer of the mhu doorbell bindings ?
Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
On Tue, Sep 8, 2020 at 4:15 AM Arnd Bergmann wrote: > > Picking up the old thread again after and getting pinged by multiple > colleagues about it (thanks!) reading through the history. > > On Fri, Jun 12, 2020 at 7:29 AM Viresh Kumar wrote: > > > > On 11-06-20, 19:34, Jassi Brar wrote: > > > In the first post in this thread, Viresh lamented that mailbox > > > introduces "a few ms" delay in the scheduler path. > > > Your own tests show that is certainly not the case -- average is the > > > same as proposed virtual channels 50-100us, the best case is 3us vs > > > 53us for virtual channels. > > > > Hmmm, I am not sure where is the confusion here Jassi. There are two > > things which are very very different from each other. > > > > - Time taken by the mailbox framework (and remote for acknowledging > > it) for completion of a single request, this can be 3us to 100s of > > us. This is clear for everyone. THIS IS NOT THE PROBLEM. > > > > - Delay introduced by few of such requests on the last one, i.e. 5 > > normal requests followed by an important one (like DVFS), the last > > one needs to wait for the first 5 to finish first. THIS IS THE > > PROBLEM. > > Earlier, Jassi also commented "Linux does not provide real-time > guarantees", which to me is what actually causes the issue here: > > Linux having timeouts when communicating to the firmware means > that it relies on the hardware and firmware having real-time behavior > even when not providing real-time guarantees to its processes. > The timeout used in SCMI is simply based on how long the Juno (?) platform takes to reply in most cases. Talking proper code-design, the timeout (if at all) shouldn't even be a hardcoded value, but instead taken from the platform. > When comparing the two usage models, it's clear that the minimum > latency for a message delivery is always at least the time time > to process an interrupt, plus at least one expensive MMIO read > and one less expensive posted MMIO write for an Ack. If we > have a doorbell plus out-of-band message, we need an extra > DMA barrier and a read from coherent memory, both of which can > be noticeable. As soon as messages are queued in the current > model, the maximum latency increases by a potentially unbounded > number of round-trips, while in the doorbell model that problem > does not exist, so I agree that we need to handle both modes > in the kernel deal with all existing hardware as well as firmware > that requires low-latency communication. > >From the test case Sudeep last shared, the scmi usage on mhu doesn't not even hit any bottleneck ... the test "failed" because of the too small hardcoded timeout value. Otherwise the current code actually shows better numbers. We need some synthetic tests to bring the limitation to the surface. I agree that there may be such a test case, however fictitious. For that reason, I am ok with the doorbell mode. > The only questions that I see in need of being answered are: > > 1. Should the binding use just different "#mbox-cells" values or >also different "compatible" strings to tell that difference? > 2. Should one driver try to handle both modes or should there >be two drivers? > > It sounds like Jassi strongly prefers separate drivers, which > would make separate compatible strings the more practical > approach. While the argument can be made that a single > piece of hardware should only have one DT description, > the counter-argument would be that the behavior described > by the DT here is made up by both the hardware and the > firmware behind it, and they are in fact different. > I totally agree with one compat-string for one hardware. However, as you said, unlike other device classes, the mailbox driver runs the sumtotal of hardware and the remote firmware behaviour. Also the implementations wouldn't share much, so I think a separate file+dt will be better. But I wanna get rid of this toothache that flares up every season, so whatever. Cheers!
[GIT PULL] Mailbox changes for v5.9
Hi Linus, The following changes since commit 92ed301919932f13b9172e525674157e983d: Linux 5.8-rc7 (2020-07-26 14:14:06 -0700) are available in the Git repository at: git://git.linaro.org/landing-teams/working/fujitsu/integration.git tags/mailbox-v5.9 for you to fetch changes up to 884996986347dbe3b735cfa9bc041dd98a533796: mailbox: mediatek: cmdq: clear task in channel before shutdown (2020-08-03 23:56:38 -0500) - mediatek : add support for mt6779 gce shutdown cleanup and address shift support - qcom : add msm8994 apcs and sdm660 hmss compatibility - imx : mark PM funcs __maybe - pcc : put acpi table before bailout - misc: replace http with https links Alexander A. Klimov (1): mailbox: Replace HTTP links with HTTPS ones Dennis YC Hsieh (4): dt-binding: gce: add gce header file for mt6779 mailbox: cmdq: variablize address shift in platform mailbox: cmdq: support mt6779 gce platform definition mailbox: mediatek: cmdq: clear task in channel before shutdown Hanjun Guo (1): mailbox: pcc: Put the PCCT table for error path Konrad Dybcio (2): mailbox: qcom: Add sdm660 hmss compatible mailbox: qcom: Add msm8994 apcs compatible Nathan Chancellor (1): mailbox: imx: Mark PM functions as __maybe_unused .../devicetree/bindings/mailbox/mtk-gce.txt| 8 +- .../bindings/mailbox/qcom,apcs-kpss-global.yaml| 2 + drivers/mailbox/imx-mailbox.c | 8 +- drivers/mailbox/mtk-cmdq-mailbox.c | 97 +++-- drivers/mailbox/omap-mailbox.c | 2 +- drivers/mailbox/pcc.c | 9 +- drivers/mailbox/qcom-apcs-ipc-mailbox.c| 10 + drivers/mailbox/ti-msgmgr.c| 2 +- include/dt-bindings/gce/mt6779-gce.h | 222 + include/linux/mailbox/mtk-cmdq-mailbox.h | 2 + 10 files changed, 338 insertions(+), 24 deletions(-) create mode 100644 include/dt-bindings/gce/mt6779-gce.h
Re: [PATCH 0/7] Add initial Keem Bay SoC / Board support
On Tue, Jul 7, 2020 at 4:18 PM Daniele Alessandrelli wrote: > Just one question: should I remove the mailbox and scmi nodes from the > soc DT or can I keep them there even if the mailbox driver is not > available yet? > A device node can not be merged before its dt-bindings are.
Re: [PATCH 23/32] usb: gadget: udc: max3420_udc: Remove set, but never checked variable 'addr'
On Mon, 6 Jul 2020 at 08:34, Lee Jones wrote:
> diff --git a/drivers/usb/gadget/udc/max3420_udc.c
> b/drivers/usb/gadget/udc/max3420_udc.c
> index 23f33946d80c4..52884bae4af11 100644
> --- a/drivers/usb/gadget/udc/max3420_udc.c
> +++ b/drivers/usb/gadget/udc/max3420_udc.c
> @@ -623,7 +623,6 @@ static void max3420_set_clear_feature(struct max3420_udc
> *udc)
> static void max3420_handle_setup(struct max3420_udc *udc)
> {
> struct usb_ctrlrequest setup;
> - u8 addr;
>
> spi_rd_buf(udc, MAX3420_REG_SUDFIFO, (void *), 8);
>
> @@ -647,7 +646,7 @@ static void max3420_handle_setup(struct max3420_udc *udc)
> USB_TYPE_STANDARD | USB_RECIP_DEVICE)) {
> break;
> }
> - addr = spi_rd8_ack(udc, MAX3420_REG_FNADDR, 1);
> + spi_rd8_ack(udc, MAX3420_REG_FNADDR, 1);
> dev_dbg(udc->dev, "Assigned Address=%d\n", udc->setup.wValue);
> return;
> case USB_REQ_CLEAR_FEATURE:
Acked-by: Jassi Brar
Re: [PATCH 0/7] Add initial Keem Bay SoC / Board support
On Tue, Jun 16, 2020 at 10:56 AM Daniele Alessandrelli wrote: > > Hi, > > This patch-set adds initial support for a new Intel Movidius SoC code-named > Keem Bay. The SoC couples an ARM Cortex A53 CPU with an Intel Movidius VPU. > > This initial patch-set enables only the minimal set of components required to > make the Keem Bay EVM board boot into initramfs. > > Brief summary of the patch-set: > * Patches 1-2 add the Keem Bay SCMI Mailbox driver (needed to enable SCMI in > Keem Bay) > * Patch 3 adds the ARCH_KEEMBAY config option > * Patches 4-7 add minimal device tree for Keem Bay SoC and Keem Bay EVM > (together with information about the SoC maintainers) > Please break this into two patchsets - first enabling platform support and second adding mailbox support. thanks.
Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
On Thu, Jun 11, 2020 at 5:00 AM Sudeep Holla wrote: > > > > > Interesting logs ! The time taken to complete _successful_ requests > > > are arguably better in bad_trace ... there are many <10usec responses > > > in bad_trace, while the fastest response in good_trace is 53usec. > > > > Indeed this is interesting. It may be worth looking (separately) into > > why don't we see those 3 us long requests anymore, or maybe they were > > just not there in the logs. > > > > As I mentioned in another thread that non-dvfs requests may be prioritised > lower when there are parallel request to the remote. The so called bad > trace doesn't have such scenario with single channel and all requests > from OS being serialised. The good trace has 2 channels and requests to > remote happen in parallel and hence it is fair to see slightly higher > latencies for lower priority requests. > In the first post in this thread, Viresh lamented that mailbox introduces "a few ms" delay in the scheduler path. Your own tests show that is certainly not the case -- average is the same as proposed virtual channels 50-100us, the best case is 3us vs 53us for virtual channels. -#define SCMI_MAX_POLL_TO_NS (100 * NSEC_PER_USEC) +#define SCMI_MAX_POLL_TO_NS (30 * 1000 * NSEC_PER_USEC) The above simple fix (not a hack or workaround) avoids the need of virtual channels' implementation, as per tests you conducted. It might have been silly to not implement virtual channels originally, but it would be just as silly now to implement if we can reuse the code. So I welcome new tests. thanks.
Re: [GIT PULL] Mailbox changes for v5.8
On Thu, Jun 11, 2020 at 11:56 AM Rob Herring wrote: > > On Wed, Jun 10, 2020 at 11:10:56PM -0500, Jassi Brar wrote: > > > > Sivaprakash Murugesan (3): > > dt-bindings: mailbox: Add YAML schemas for QCOM APCS global block > > mailbox: qcom: Add clock driver name in apcs mailbox driver data > > mailbox: qcom: Add ipq6018 apcs compatible > > You've dropped the binding change that breaks 'make dt_binding_check' > from this PR, but do you intend to drop it from linux-next because it's > still in today's next? > Yes. I had dropped the offending commit at the last moment. > And really the above commit should not be applied until the binding > change is, but fine. > I made an exception here because I wanted to make as little change to my PR as necessary. And it wouldn't cause any regression - the new compat string has already been acked by you. thanks.
Re: [PATCH] firmware: arm_scmi: fix timeout value for send_message
On Thu, Jun 11, 2020 at 3:40 AM Sudeep Holla wrote:
> >
> > > > > > if (xfer->hdr.poll_completion) {
> > > > > > - ktime_t stop = ktime_add_ns(ktime_get(),
> > > > > > SCMI_MAX_POLL_TO_NS);
> > > > > > + ktime_t stop = ktime_add_ns(ktime_get(), 500 * 1000 *
> > > > > > NSEC_PER_USEC);
> > > > > >
> > > > >
> > > > > This is unacceptable delay for schedutil fast_switch. So no for this
> > > > > one.
> > > > >
> > > > Increasing timeout does not increase latency.
> > >
> > > Agreed, but worst case you may be stuck here for 500ms which is not
> > > acceptable.
> > >
> > Not acceptable to who, you or the kernel? :)Now that you said you
> > are fixing the scmi's fast_switch implementation.
> >
> Sorry, I meant to disable it for single channel implementation.
>
The single-channel platform may have only cpufreq as the user, or only
users that respond quickly ~3us. So maybe we leave the decision, to
enable fast_switch, totally to the platform. But of course, this
discussion is for another thread.
> > Even though I don't think 500ms would ruin our lives, but ok, I will
> > make it 30ms - same as you did in the 'else' block. And drop the other
> > change.
>
> I am fine if cpufreq maintainers allow that in the fast switch path that
> happens in the fast path.
>
Thanks, let me respin the patch and include some cpufreq folks.
Cheers!
[GIT PULL] Mailbox changes for v5.8
Hi Linus, The following changes since commit ffeb595d84811dde16a28b33d8a7cf26d51d51b3: Merge tag 'powerpc-5.7-6' of git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux (2020-05-30 12:28:44 -0700) are available in the Git repository at: git://git.linaro.org/landing-teams/working/fujitsu/integration.git tags/mailbox-v5.8 for you to fetch changes up to e9f901dc05c09c4f89183cadcb2d93177f3100cb: mailbox: qcom: Add ipq6018 apcs compatible (2020-06-10 22:43:57 -0500) - qcom : new controller driver for IPCC reorg the of_device data add support for ipq6018 platform - spreadtrum: new sprd controller driver - imx: implement suspend/resume PM support - Misc : make pcc driver struct as static fix return value in imx_mu_scu disable clock before bailout in imx probe remove duplicate error mssg in zynqmp probe fix header size in imx.scu check for null instead of is-err in zynqmp Anson Huang (3): mailbox: imx: Support runtime PM mailbox: imx: Add runtime PM callback to handle MU clocks mailbox: imx: ONLY IPC MU needs IRQF_NO_SUSPEND flag Baolin Wang (2): dt-bindings: mailbox: Add the Spreadtrum mailbox documentation mailbox: sprd: Add Spreadtrum mailbox driver Dan Carpenter (1): mailbox: imx: Fix return in imx_mu_scu_xlate() Dong Aisheng (1): mailbox: imx: Add context save/restore for suspend/resume Fabio Estevam (1): mailbox: imx: Disable the clock on devm_mbox_controller_register() failure Jason Yan (1): mailbox: pcc: make pcc_mbox_driver static Manivannan Sadhasivam (3): dt-bindings: mailbox: Add devicetree binding for Qcom IPCC mailbox: Add support for Qualcomm IPCC MAINTAINERS: Add entry for Qualcomm IPCC driver Markus Elfring (1): mailbox: ZynqMP IPI: Delete an error message in zynqmp_ipi_probe() Peng Fan (1): mailbox: imx-mailbox: fix scu msg header size check Sivaprakash Murugesan (3): dt-bindings: mailbox: Add YAML schemas for QCOM APCS global block mailbox: qcom: Add clock driver name in apcs mailbox driver data mailbox: qcom: Add ipq6018 apcs compatible Wei Yongjun (1): mailbox: zynqmp-ipi: Fix NULL vs IS_ERR() check in zynqmp_ipi_mbox_probe() .../bindings/mailbox/qcom,apcs-kpss-global.txt | 88 - .../bindings/mailbox/qcom,apcs-kpss-global.yaml| 86 + .../devicetree/bindings/mailbox/qcom-ipcc.yaml | 80 + .../devicetree/bindings/mailbox/sprd-mailbox.yaml | 60 MAINTAINERS| 8 + drivers/mailbox/Kconfig| 18 + drivers/mailbox/Makefile | 4 + drivers/mailbox/imx-mailbox.c | 117 ++- drivers/mailbox/pcc.c | 2 +- drivers/mailbox/qcom-apcs-ipc-mailbox.c| 61 +++- drivers/mailbox/qcom-ipcc.c| 286 drivers/mailbox/sprd-mailbox.c | 361 + drivers/mailbox/zynqmp-ipi-mailbox.c | 25 +- include/dt-bindings/mailbox/qcom-ipcc.h| 33 ++ 14 files changed, 1097 insertions(+), 132 deletions(-) delete mode 100644 Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt create mode 100644 Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml create mode 100644 Documentation/devicetree/bindings/mailbox/qcom-ipcc.yaml create mode 100644 Documentation/devicetree/bindings/mailbox/sprd-mailbox.yaml create mode 100644 drivers/mailbox/qcom-ipcc.c create mode 100644 drivers/mailbox/sprd-mailbox.c create mode 100644 include/dt-bindings/mailbox/qcom-ipcc.h
Re: [PATCH] firmware: arm_scmi: fix timeout value for send_message
On Wed, Jun 10, 2020 at 10:56 AM Sudeep Holla wrote:
[I admit you can write bigger posts than me, so I am not going to
write a passionate response to each of your paragraphs.
Let's keep it to the point.]
> > > > if (xfer->hdr.poll_completion) {
> > > > - ktime_t stop = ktime_add_ns(ktime_get(),
> > > > SCMI_MAX_POLL_TO_NS);
> > > > + ktime_t stop = ktime_add_ns(ktime_get(), 500 * 1000 *
> > > > NSEC_PER_USEC);
> > > >
> > >
> > > This is unacceptable delay for schedutil fast_switch. So no for this one.
> > >
> > Increasing timeout does not increase latency.
>
> Agreed, but worst case you may be stuck here for 500ms which is not
> acceptable.
>
Not acceptable to who, you or the kernel? :)Now that you said you
are fixing the scmi's fast_switch implementation.
Even though I don't think 500ms would ruin our lives, but ok, I will
make it 30ms - same as you did in the 'else' block. And drop the other
change.
Thanks.
Re: [PATCH] firmware: arm_scmi: fix timeout value for send_message
On Wed, Jun 10, 2020 at 3:23 AM Sudeep Holla wrote:
>
> On Sun, Jun 07, 2020 at 02:30:23PM -0500, jassisinghb...@gmail.com wrote:
> > From: Jassi Brar
> >
> > Currently scmi_do_xfer() submits a message to mailbox api and waits
> > for an apparently very short time. This works if there are not many
> > messages in the queue already. However, if many clients share a
> > channel and/or each client submits many messages in a row, the
>
> The recommendation in such scenarios is to use multiple channel.
>
If SCMI is to be accepted as a standard (which I hope), it has to
support most kinds of controllers, but currently the implementation is
too myopic. It is only a matter of time, when someone sees value in
reusing firmware implementation (scmi) but does not have a MHU like
controller.
> > timeout value becomes too short and returns error even if the mailbox
> > is working fine according to the load. The timeout occurs when the
> > message is still in the api/queue awaiting its turn to ride the bus.
> >
> > Fix this by increasing the timeout value enough (500ms?) so that it
> > fails only if there is an actual problem in the transmission (like a
> > lockup or crash).
> >
> > [If we want to capture a situation when the remote didn't
> > respond within expected latency, then the timeout should not
> > start here, but from tx_prepare callback ... just before the
> > message physically gets on the channel]
> >
>
> The bottle neck may not be in the remote. It may be mailbox serialising
> the requests even when it can parallelise.
>
Your logs show (in your test case), using 1 physical channel shows
better transfer (those that complete) rates than virtual channels.
The transfers that fail are purely because of this short timeout.
> >
> > if (xfer->hdr.poll_completion) {
> > - ktime_t stop = ktime_add_ns(ktime_get(), SCMI_MAX_POLL_TO_NS);
> > + ktime_t stop = ktime_add_ns(ktime_get(), 500 * 1000 *
> > NSEC_PER_USEC);
> >
>
> This is unacceptable delay for schedutil fast_switch. So no for this one.
>
Increasing timeout does not increase latency.
Also scmi_xfer() can not know if it was reached from the fast_switch path.
If a platform has many users over a channel such that it can not
guarantee low enough latency, then it must not set the
fast_switch_possible flag, which is optional for this reason.
> > @@ -313,7 +313,7 @@ int scmi_do_xfer(const struct scmi_handle *handle,
> > struct scmi_xfer *xfer)
> > ret = -ETIMEDOUT;
> > } else {
> > /* And we wait for the response. */
> > - timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
> > + timeout = msecs_to_jiffies(500);
>
> In general, this hides issues in the remote.
>
If you want to uncover remote issues, start the timeout in
tx_prepare() because that is when the message is physically sent to
the remote.
> We are trying to move towards
> tops 1ms for a request and with MBOX_QUEUE at 20, I see 20ms is more that
> big enough. We have it set to 30ms now. 500ms is way too large and not
> required IMO.
>
Again, increasing timeout does not slow the system down. It is to
support more variety of platform setups.
Cheers!
Re: [PATCH V2 3/3] mailbox: qcom: Add ipq6018 apcs compatible
On Sat, Jun 6, 2020 at 5:59 AM Sivaprakash Murugesan wrote: > > The Qualcomm ipq6018 has apcs block, add compatible for the same. > Also, the apcs provides a clock controller functionality similar > to msm8916 but the clock driver is different. > > Create a child platform device based on the apcs compatible for the > clock controller functionality. > > Signed-off-by: Sivaprakash Murugesan > --- > [V2] > * created a new structur for driver data. > * re-arranged compatible strings in sorted order > Please break this into two patches, first reorganise and then add new ipq6018 of_match. thanks.
Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
On Fri, Jun 5, 2020 at 3:58 AM Sudeep Holla wrote: > > > > > >> bash-1526 [000] 1149.472553: scmi_xfer_begin: > > > > >> transfer_id=1538 msg_id=6 protocol_id=21 seq=0 poll=0 > > > > >> -0 [001] 1149.472733: scmi_xfer_begin: > > > > >> transfer_id=1539 msg_id=7 protocol_id=19 seq=1 poll=1 > > > > > > > > > Here another request is started before the first is finished. > > > > > > Ah, the prints are when the client requested. It is not when the mailbox > > > started it. So this just indicates the beginning of the transfer from the > > > client. > > > > > There maybe condition on a sensor read to finish within 1ms, but there > > is no condition for the read to _start_ at this very moment (usually > > there are sleeps in the path to sensor requests). > > > > Again I wasn't clear. The trace logs are at the point just before calling > mbox_send_messages. So any delay in sensor drivers won't get include. It > is after the point sensor driver request to read the value and before we > send the request via mailbox. > No, you were clear, I wasn't. Let me try again. Since origin upto scmi_xfer, there can be many forms of sleep like schedule/mutexlock etc think of some userspace triggering sensor or dvfs operation. Linux does not provide real-time guarantees. Even if remote (scmi) firmware guarantee RT response, it makes sense to timeout a response only after the _request is on the bus_ and not when you submit a request to the api (unless you serialise it). IOW, start the timeout from mbox_client.tx_prepare() when the message actually gets on the bus. > > You have shared only 'bad' log without serialising access. Please > > share log after serialising access to the channel and the 'good' log > > with virtual channels. That should put the topic to rest. > > > > I didn't realise that, sorry for missing that earlier. Attached both > now, thanks for asking. > Interesting logs ! The time taken to complete _successful_ requests are arguably better in bad_trace ... there are many <10usec responses in bad_trace, while the fastest response in good_trace is 53usec. And the requests that 'fail/timeout' are purely the result of not serialising them or checkout for timeout at wrong place as explained above. thanks.
Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
On Thu, Jun 4, 2020 at 11:56 PM Sudeep Holla wrote: > > > >> bash-1526 [000] 1149.472553: scmi_xfer_begin: > > >> transfer_id=1538 msg_id=6 protocol_id=21 seq=0 poll=0 > > >> -0 [001] 1149.472733: scmi_xfer_begin: > > >> transfer_id=1539 msg_id=7 protocol_id=19 seq=1 poll=1 > > > > > Here another request is started before the first is finished. > > Ah, the prints are when the client requested. It is not when the mailbox > started it. So this just indicates the beginning of the transfer from the > client. > There maybe condition on a sensor read to finish within 1ms, but there is no condition for the read to _start_ at this very moment (usually there are sleeps in the path to sensor requests). > > If you want this to work you have to serialise access to the single > > physical channel and/or run the remote firmware in asynchronous mode - > > that is, the firmware ack the bit as soon as it sees and starts > > working in the background, so that we return in ~3usec always, while > > the data returns whenever it is ready. > > Yes it does that for few requests like DVFS while it uses synchronous > mode for few others. While ideally I agree everything in asynchronous > most is better, I don't know there may be reasons for such design. > The reason is that, if the firmware works asynchronously, every call would return in ~3usec and you won't think you need virtual channels. You have shared only 'bad' log without serialising access. Please share log after serialising access to the channel and the 'good' log with virtual channels. That should put the topic to rest. thanks.
Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
On Thu, Jun 4, 2020 at 4:20 AM Sudeep Holla wrote: > > On Wed, Jun 03, 2020 at 01:32:42PM -0500, Jassi Brar wrote: > > On Wed, Jun 3, 2020 at 1:04 PM Sudeep Holla wrote: > > > > > > On Fri, May 29, 2020 at 09:37:58AM +0530, Viresh Kumar wrote: > > > > On 28-05-20, 13:20, Rob Herring wrote: > > > > > Whether Linux > > > > > requires serializing mailbox accesses is a separate issue. On that > > > > > side, > > > > > it seems silly to not allow driving the h/w in the most efficient way > > > > > possible. > > > > > > > > That's exactly what we are trying to say. The hardware allows us to > > > > write all 32 bits in parallel, without any hardware issues, why > > > > shouldn't we do that ? The delay (which Sudeep will find out, he is > > > > facing issues with hardware access because of lockdown right now) > > > > > > OK, I was able to access the setup today. I couldn't reach a point > > > where I can do measurements as the system just became unusable with > > > one physical channel instead of 2 virtual channels as in my patches. > > > > > > My test was simple. Switch to schedutil and read sensors periodically > > > via sysfs. > > > > > > arm-scmi firmware:scmi: message for 1 is not expected! > > > > > This sounds like you are not serialising requests on a shared channel. > > Can you please also share the patch? > > OK, I did try with a small patch initially and then realised we must hit > issue with mainline as is. Tried and the behaviour is exact same. All > I did is removed my patches and use bit[0] as the signal. It doesn't > matter as writes to the register are now serialised. Oh, the above > message comes when OS times out in advance while firmware continues to > process the old request and respond. > > The trace I sent gives much better view of what's going on. > BTW, you didn't even share 'good' vs 'bad' log for me to actually see if the api lacks. Let us look closely ... >>bash-1019 [005] 1149.452340: scmi_xfer_begin: transfer_id=1537 msg_id=7 protocol_id=19 seq=0 poll=1 >>bash-1019 [005] 1149.452407: scmi_xfer_end: transfer_id=1537 msg_id=7 protocol_id=19 seq=0 status=0 > This round trip took 67usecs. (log shows some at even 3usecs) That includes mailbox api overhead, memcpy and the time taken by remote to respond. So the api is definitely capable of much faster transfers than you require. >> bash-1526 [000] 1149.472553: scmi_xfer_begin: transfer_id=1538 >> msg_id=6 protocol_id=21 seq=0 poll=0 >> -0 [001] 1149.472733: scmi_xfer_begin: transfer_id=1539 >> msg_id=7 protocol_id=19 seq=1 poll=1 > Here another request is started before the first is finished. If you want this to work you have to serialise access to the single physical channel and/or run the remote firmware in asynchronous mode - that is, the firmware ack the bit as soon as it sees and starts working in the background, so that we return in ~3usec always, while the data returns whenever it is ready. Again, I don't have the code or the 'good' run log to compare. PS: I feel it is probably less effort for me to simply let the patch through, than use my reiki powers to imagine how your test code and firmware looks like.
Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
On Wed, Jun 3, 2020 at 1:31 PM Sudeep Holla wrote: > > > > > > H/W is actually fine :) Its just that the driver is written to > > _also_ support a platform (my original) that doesn't have shmem and > > need to pass data via 32bit registers. > > Frankly, I am not against the doorbell mode, I am against implementing > > two modes in a driver. If it really helped (note the past tense) the > > SCMI, we could implement the driver only in doorbell mode but > > unfortunately SCMI would still be _broken_ for non-doorbell > > controllers. > > Should be fine as the specification is designed to work with only shmem > for any data transfer and mailbox is just a signal mechanism. I won't > be too worried about that. > Please clarify how it will work on, say again, rockchip platform with shmem. thanks.
Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
On Wed, Jun 3, 2020 at 1:04 PM Sudeep Holla wrote: > > On Fri, May 29, 2020 at 09:37:58AM +0530, Viresh Kumar wrote: > > On 28-05-20, 13:20, Rob Herring wrote: > > > Whether Linux > > > requires serializing mailbox accesses is a separate issue. On that side, > > > it seems silly to not allow driving the h/w in the most efficient way > > > possible. > > > > That's exactly what we are trying to say. The hardware allows us to > > write all 32 bits in parallel, without any hardware issues, why > > shouldn't we do that ? The delay (which Sudeep will find out, he is > > facing issues with hardware access because of lockdown right now) > > OK, I was able to access the setup today. I couldn't reach a point > where I can do measurements as the system just became unusable with > one physical channel instead of 2 virtual channels as in my patches. > > My test was simple. Switch to schedutil and read sensors periodically > via sysfs. > > arm-scmi firmware:scmi: message for 1 is not expected! > This sounds like you are not serialising requests on a shared channel. Can you please also share the patch? Thanks.
Re: [PATCH] mailbox: imx: Add context save/restore for suspend/resume
On Wed, May 27, 2020 at 8:55 PM Anson Huang wrote:
>
> Gentle ping...
>
>
> > Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> > suspend/resume
> >
> >
> >
> > > Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> > > suspend/resume
> > >
> > > > From: Anson Huang
> > > > Sent: Friday, April 24, 2020 10:33 AM
> > > >
> > > > > Subject: RE: [PATCH] mailbox: imx: Add context save/restore for
> > > > > suspend/resume
> > > > >
> > > > > > From: Anson Huang
> > > > > > Sent: Friday, April 24, 2020 7:01 AM
> > > > > >
> > > > > > For "mem" mode suspend on i.MX8 SoCs, MU settings could be lost
> > > > > > because its power is off, so save/restore is needed for MU
> > > > > > settings during
> > > > > suspend/resume.
> > > > > > However, the restore can ONLY be done when MU settings are
> > > > > > actually lost, for the scenario of settings NOT lost in "freeze"
> > > > > > mode suspend, since there could be still IPC going on multiple
> > > > > > CPUs, restoring the MU settings could overwrite the TIE by
> > > > > > mistake and cause system freeze, so need to make sure ONLY
> > > > > > restore the MU settings when it is
> > > > > powered off.
> > > > > >
> > > > > > Signed-off-by: Anson Huang
> > > > >
> > > > > As mentioned before, we'd better keep the original author.
> > > > >
> > > > > > ---
> > > > > > drivers/mailbox/imx-mailbox.c | 35
> > > > > > +++
> > > > > > 1 file changed, 35 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/mailbox/imx-mailbox.c
> > > > > > b/drivers/mailbox/imx-mailbox.c index 97bf0ac..b53cf63 100644
> > > > > > --- a/drivers/mailbox/imx-mailbox.c
> > > > > > +++ b/drivers/mailbox/imx-mailbox.c
> > > > > > @@ -67,6 +67,8 @@ struct imx_mu_priv {
> > > > > > struct clk *clk;
> > > > > > int irq;
> > > > > >
> > > > > > + u32 xcr;
> > > > > > +
> > > > > > boolside_b;
> > > > > > };
> > > > > >
> > > > > > @@ -583,12 +585,45 @@ static const struct of_device_id
> > > > > > imx_mu_dt_ids[] = { }; MODULE_DEVICE_TABLE(of, imx_mu_dt_ids);
> > > > > >
> > > > > > +static int imx_mu_suspend_noirq(struct device *dev) {
> > > > > > + struct imx_mu_priv *priv = dev_get_drvdata(dev);
> > > > > > +
> > > > > > + priv->xcr = imx_mu_read(priv, priv->dcfg->xCR);
> > > > > > +
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int imx_mu_resume_noirq(struct device *dev) {
> > > > > > + struct imx_mu_priv *priv = dev_get_drvdata(dev);
> > > > > > +
> > > > > > + /*
> > > > > > + * ONLY restore MU when context lost, the TIE could
> > > > > > + * be set during noirq resume as there is MU data
> > > > > > + * communication going on, and restore the saved
> > > > > > + * value will overwrite the TIE and cause MU data
> > > > > > + * send failed, may lead to system freeze. This issue
> > > > > > + * is observed by testing freeze mode suspend.
> > > > > > + */
> > > > > > + if (!imx_mu_read(priv, priv->dcfg->xCR))
> > > > > > + imx_mu_write(priv, priv->xcr, priv->dcfg->xCR);
> > > > >
> > > > > This could be separate patch if it aims to fix a specific corner case.
> > > >
> > > > This is NOT corner case, it can be reproduced with our imx_5.4.y
> > > > very easily, and this issue cause me many days to debug...Also cause
> > > > Clark's effort to help test it a lot for many days...
> > > >
> > >
> > > Is this issue only happen for non-state lost case (eg. Freeze mode)?
> > > If yes, it's a specific case and worth a separate patch to highlight it
> > > IMHO.
> > >
> > > BTW, it seems most drivers have this issue in current kernel because
> > > they don't know whether the state are really lost, it seems like
> > > kernel still doesn't support this well.
> > >
> > > > I do NOT think it makes sense to first send out your patch with bug
> > > > for review, And then add another patch to fix it. 1 patch is enough
> > > > for this
> > > feature.
> > > >
> > >
> > > Anyway, if you really want to go with one patch, for this case, we
> > > usually could keep original author and add a small fix note in commit
> > message.
> > > (You could see many community guys do like this if you search kernel
> > > commit
> > > log)
> > >
> > > Basically we try our best to keep origin author in order to respect
> > > others' work for community work.
> >
> > I am fine with whoever is the author, my focus is the issue fix and easy
> > rebase.
> > If maintainer agrees that introduce a patch with bug and add another patch
> > to
> > fix is OK, then I can rework the patch into 2 patches.
> >
Not two patches, just add to the original patch and add a fix note in
commit as Anson suggested ... though I don't know what the original
patch was. But I am definitely in support of giving credit to the
original author.
thanks.
Re: [PATCH v6] support gce on mt6779 platform
On Thu, May 28, 2020 at 12:05 PM Dennis YC Hsieh wrote: > > This patch support gce on mt6779 platform. > > Change since v5: > - spearate address shift code in client helper and mailbox controller > - separate write_s/write_s_mask and write_s_value/write_s_mask_value so that > client can decide use mask or not > - fix typo in header > > Change since v4: > - do not clear disp event again in drm driver > - symbolize value 1 to jump relative > > [... snip ...] > > > > Dennis YC Hsieh (16): > dt-binding: gce: add gce header file for mt6779 > mailbox: cmdq: variablize address shift in platform > mailbox: cmdq: support mt6779 gce platform definition > mailbox: mediatek: cmdq: clear task in channel before shutdown > soc: mediatek: cmdq: return send msg error code > soc: mediatek: cmdq: add address shift in jump > soc: mediatek: cmdq: add assign function > soc: mediatek: cmdq: add write_s function > soc: mediatek: cmdq: add write_s_mask function > soc: mediatek: cmdq: add read_s function > soc: mediatek: cmdq: add write_s value function > soc: mediatek: cmdq: add write_s_mask value function > soc: mediatek: cmdq: export finalize function > soc: mediatek: cmdq: add jump function > soc: mediatek: cmdq: add clear option in cmdq_pkt_wfe api > soc: mediatek: cmdq: add set event function > > .../devicetree/bindings/mailbox/mtk-gce.txt | 8 +- > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 3 +- > drivers/mailbox/mtk-cmdq-mailbox.c| 101 ++-- > drivers/soc/mediatek/mtk-cmdq-helper.c| 163 - > include/dt-bindings/gce/mt6779-gce.h | 222 ++ > include/linux/mailbox/mtk-cmdq-mailbox.h | 10 +- > include/linux/soc/mediatek/mtk-cmdq.h | 125 +- > Please break the patchset into two. The lower mailbox related changes with soc changes on top. thanks
Re: [PATCH] mailbox: no error log in mbox_client_txdone() for tx done by irq
On Mon, May 11, 2020 at 12:52 AM wrote:
>
> From: Joe Zhu
>
> client does not know and not care about how controller implement tx done.
> mbox_client_txdone() may be called when controller uses irq method.
>
> Signed-off-by: Joe Zhu
> ---
> drivers/mailbox/mailbox.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 0b821a5b2db8..116124adf188 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -189,7 +189,9 @@ EXPORT_SYMBOL_GPL(mbox_chan_txdone);
> void mbox_client_txdone(struct mbox_chan *chan, int r)
> {
> if (unlikely(!(chan->txdone_method & TXDONE_BY_ACK))) {
> - dev_err(chan->mbox->dev, "Client can't run the TX ticker\n");
> + if (unlikely(!(chan->txdone_method & TXDONE_BY_IRQ)))
> + dev_err(chan->mbox->dev,
> + "Client can't run the TX ticker\n");
> return;
> }
If it is not by ACK, client should not call txdone() so we shout
immediately. Otherwise something is wrong.
thanks.
Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
On Thu, May 28, 2020 at 2:20 PM Rob Herring wrote: > > On Fri, May 15, 2020 at 10:47:38AM +0530, Viresh Kumar wrote: > > From: Sudeep Holla > > > > Hi Rob, Arnd and Jassi, > > > > This stuff has been doing rounds on the mailing list since several years > > now with no agreed conclusion by all the parties. And here is another > > attempt to get some feedback from everyone involved to close this once > > and for ever. Your comments will very much be appreciated. > > > > The ARM MHU is defined here in the TRM [1] for your reference, which > > states following: > > > > "The MHU drives the signal using a 32-bit register, with all 32 > > bits logically ORed together. The MHU provides a set of > > registers to enable software to set, clear, and check the status > > of each of the bits of this register independently. The use of > > 32 bits for each interrupt line enables software to provide more > > information about the source of the interrupt. For example, each > > bit of the register can be associated with a type of event that > > can contribute to raising the interrupt." > > > > On few other platforms, like qcom, similar doorbell mechanism is present > > with separate interrupt for each of the bits (that's how I understood > > it), while in case of ARM MHU, there is a single interrupt line for all > > the 32 bits. Also in case of ARM MHU, these registers and interrupts > > have 3 copies for different priority levels, i.e. low priority > > non-secure, high priority non-secure and secure channels. > > > > For ARM MHU, both the dt bindings and the Linux driver support 3 > > channels for the different priorities right now and support sending a 32 > > bit data on every transfer in a locked fashion, i.e. only one transfer > > can be done at once and the other have to wait for it to finish first. > > > > Here are the point of view of the parties involved on this subject: > > > > Jassi's viewpoint: > > > > - Virtualization of channels should be discouraged in software based on > > specific usecases of the application. This may invite other mailbox > > driver authors to ask for doing virtualization in their drivers. > > > > - In mailbox's terminology, every channel is equivalent to a signal, > > since there is only one signal generated here by the MHU, there should > > be only one channel per priority level. > > > > - The clients should send data (of just setting 1 bit or many in the 32 > > bit word) using the existing mechanism as the delays due to > > serialization shouldn't be significant anyway. > > > > - The driver supports 90% of the users with the current implementation > > and it shouldn't be extended to support doorbell and implement two > > different modes by changing value of #mbox-cells field in bindings. > > > > Sudeep (ARM) and myself as well to some extent: > > > > - The hardware gives us the capability to write the register in > > parallel, i.e. we can write 0x800 and 0x400 together without any > > software locks, and so these 32 bits should be considered as separate > > channel even if only one interrupt is issued by the hardware finally. > > This shouldn't be called as virtualization of the channels, as the > > hardware supports this (as clearly mentioned in the TRM) and it takes > > care of handling the signal properly. > > > > - With serialization, if we use only one channel as today at every > > priority, if there are 5 requests to send signal to the receiver and > > the dvfs request is the last one in queue (which may be called from > > scheduler's hot path with fast switching), it unnecessarily needs to > > wait for the first four transfers to finish due to the software > > locking imposed by the mailbox framework. This adds additional delay, > > maybe of few ms only, which isn't required by the hardware but just by > > the software and few ms can be important in scheduler's hotpath. > > > > - With the current approach it isn't possible to assign different bits > > (or doorbell numbers) to clients from DT and the only way of doing > > that without adding new bindings is by extending #mbox-cells to accept > > a value of 2 as done in this patch. > > > > Jassi and Sudeep, I hope I was able to represent both the view points > > properly here. Please correct me if I have made a mistake here. > > > > This is it. It would be nice to get the views of everyone now on this > > and how should this be handled. > > I am perfectly fine with adding another cell which seems appropriate > here. You can have 5 cells for all I care if that makes sense for > the h/w. That has nothing to do with the Linux design. Whether Linux > requires serializing mailbox accesses is a separate issue. On that side, > it seems silly to not allow driving the h/w in the most efficient way > possible. > The fact that all these bits are backed by one physical signal makes the firmware implement its own mux-demux'ing scheme. So the choice
Re: [PATCH v4 2/2] mailbox: sprd: Add Spreadtrum mailbox driver
On Thu, May 21, 2020 at 7:24 AM Baolin Wang wrote: > > Hi Jassi, > > On Wed, May 13, 2020 at 2:32 PM Baolin Wang wrote: > > > > On Wed, May 13, 2020 at 2:05 PM Jassi Brar wrote: > > > > > > On Tue, May 12, 2020 at 11:14 PM Baolin Wang > > > wrote: > > > > > > > > Hi Jassi, > > > > > > > > On Thu, May 7, 2020 at 11:23 AM Baolin Wang > > > > wrote: > > > > > > > > > > Hi Jassi, > > > > > > > > > > On Thu, May 7, 2020 at 7:25 AM Jassi Brar > > > > > wrote: > > > > > > > > > > > > On Wed, May 6, 2020 at 8:29 AM Baolin Wang > > > > > > wrote: > > > > > > > > > > > > > > Hi Jassi, > > > > > > > > > > > > > > On Tue, Apr 28, 2020 at 11:10 AM Baolin Wang > > > > > > > wrote: > > > > > > > > > > > > > > > > From: Baolin Wang > > > > > > > > > > > > > > > > The Spreadtrum mailbox controller supports 8 channels to > > > > > > > > communicate > > > > > > > > with MCUs, and it contains 2 different parts: inbox and outbox, > > > > > > > > which > > > > > > > > are used to send and receive messages by IRQ mode. > > > > > > > > > > > > > > > > Signed-off-by: Baolin Wang > > > > > > > > Signed-off-by: Baolin Wang > > > > > > > > --- > > > > > > > > Changes from v3: > > > > > > > > - Save the id in mbox_chan.con_priv and remove the > > > > > > > > 'sprd_mbox_chan' > > > > > > > > > > > > > > > > Changes from v2: > > > > > > > > - None. > > > > > > > > > > > > > > > > Changes from v1: > > > > > > > > - None > > > > > > > > > > > > > > Gentle ping, do you have any other comments? Thanks. > > > > > > > > > > > > > Yea, I am still not sure about the error returned in send_data(). > > > > > > It > > > > > > will either never hit or there will be no easy recovery from it. The > > > > > > api expects the driver to tell it the last-tx was done only when it > > > > > > can send the next message. (There may be case like sending depend on > > > > > > remote, which can't be ensured before hand). > > > > > > > > > > Actually this is an unusual case, suppose the remote target did not > > > > > fetch the message as soon as possile, which will cause the FIFO > > > > > overflow, so in this case we can not send messages to the remote > > > > > target any more, otherwise messages will be lost. Thus we can return > > > > > errors to users to indicate that something wrong with the remote > > > > > target need to be checked. > > > > > > > > > > So this validation in send_data() is mostly for debugging for this > > > > > abnormal case and we will not trigger this issue if the remote target > > > > > works well. So I think it is useful to keep this validation in > > > > > send_data(). Thanks. > > > > > > > > Any comments? Thanks. > > > > > > > Same as my last post. > > > > I think I've explained the reason why we need add this validation in > > my previous email, I am not sure how do you think? You still want to > > remove this validation? > > Gentle ping. > > As I explained in previous email, this validation is for an unusual > case, suppose the remote target did not fetch the message as soon as > possile, which will cause the FIFO overflow, so in this case we can > not send messages to the remote > target any more, otherwise messages will be lost. Thus we can return > errors to users to indicate that something wrong with the remote > target need to be checked. > > So this validation in send_data() is mostly for debugging for this > abnormal case and we will not trigger this issue if the remote target > works well. So I think it is useful to keep this validation in > send_data(). What do you think? Thanks. > I still think the same as before. You should do this check before you call mbox_chan_txdone() and wait if busy ... which is exactly the purpose of txdone(). It seems harmless to be paranoid and place a block of code in practically "if 0", but that sets bad precedence for other drivers. So please move the check before txdone(). thanks.
Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
On Mon, May 18, 2020 at 10:40 PM Viresh Kumar wrote: > > On 18-05-20, 18:29, Bjorn Andersson wrote: > > On Thu 14 May 22:17 PDT 2020, Viresh Kumar wrote: > > > This stuff has been doing rounds on the mailing list since several years > > > now with no agreed conclusion by all the parties. And here is another > > > attempt to get some feedback from everyone involved to close this once > > > and for ever. Your comments will very much be appreciated. > > > > > > The ARM MHU is defined here in the TRM [1] for your reference, which > > > states following: > > > > > > "The MHU drives the signal using a 32-bit register, with all 32 > > > bits logically ORed together. The MHU provides a set of > > > registers to enable software to set, clear, and check the status > > > of each of the bits of this register independently. The use of > > > 32 bits for each interrupt line enables software to provide more > > > information about the source of the interrupt. For example, each > > > bit of the register can be associated with a type of event that > > > can contribute to raising the interrupt." > > > > > > > Does this mean that there are 32 different signals and they are all ORed > > into the same interrupt line to trigger software action when something > > happens? > > > > Or does it mean that this register is used to pass multi-bit information > > and when any such information is passed an interrupt will be triggered? > > If so, what does that information mean? How is it tied into other Linux > > drivers/subsystems? > > I have started to believe the hardware is written badly at this point > :) > H/W is actually fine :) Its just that the driver is written to _also_ support a platform (my original) that doesn't have shmem and need to pass data via 32bit registers. Frankly, I am not against the doorbell mode, I am against implementing two modes in a driver. If it really helped (note the past tense) the SCMI, we could implement the driver only in doorbell mode but unfortunately SCMI would still be _broken_ for non-doorbell controllers.
Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
On Mon, May 18, 2020 at 2:42 AM Viresh Kumar wrote: > > On 15-05-20, 11:46, Jassi Brar wrote: > > As I asked you yesterday over the call, it may help if you could share > > some numbers to back up the doomsday scenario. > > Yes, I have already asked Sudeep to get some numbers for this. He will > get back to us. > Thanks, current bottleneck numbers and the patch/changes to improve that, would help. > > > - With the current approach it isn't possible to assign different bits > > > (or doorbell numbers) to clients from DT and the only way of doing > > > that without adding new bindings is by extending #mbox-cells to accept > > > a value of 2 as done in this patch. > > > > > I am afraid you are confused. You can use bit/doorbell-6 by passing > > 0x40 to mhu as the data to send. > > That's how the code will do it, right I agree. What I was asking was > the way this information is passed from DT. > That is a client/protocol property and has nothing to do with the controller dt node. cheers!
Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU
On Fri, May 15, 2020 at 12:17 AM Viresh Kumar wrote: > > - The hardware gives us the capability to write the register in > parallel, i.e. we can write 0x800 and 0x400 together without any > software locks, and so these 32 bits should be considered as separate > channel even if only one interrupt is issued by the hardware finally. > This shouldn't be called as virtualization of the channels, as the > hardware supports this (as clearly mentioned in the TRM) and it takes > care of handling the signal properly. > I'll leave this one open to bikeshed arguments. > - With serialization, if we use only one channel as today at every > priority, if there are 5 requests to send signal to the receiver and > the dvfs request is the last one in queue (which may be called from > scheduler's hot path with fast switching), it unnecessarily needs to > wait for the first four transfers to finish due to the software > locking imposed by the mailbox framework. This adds additional delay, > maybe of few ms only, which isn't required by the hardware but just by > the software and few ms can be important in scheduler's hotpath. > As I asked you yesterday over the call, it may help if you could share some numbers to back up the doomsday scenario. I don't believe mailbox will be a bottleneck, unless you send commands in a while(1) ... but even then you have to compare against the virtual-channel implementation. (Not to forget one usually doesn't need/want the dvfs, power, clock, hotplug all happening at the _same_ time) Please note, SCMI... lets not pretend it is not about making scmi work with mhu :) ... itself uses shared-memory transfers and wait_for_completion_timeout in scmi_do_xfer(). If some platform _really-really_ faced speed bottlenecks, it would come to want to exchange 32-bit encoded command/response over the mhu register, asynchronously and totally bypassing shmem... which is possible only now. > - With the current approach it isn't possible to assign different bits > (or doorbell numbers) to clients from DT and the only way of doing > that without adding new bindings is by extending #mbox-cells to accept > a value of 2 as done in this patch. > I am afraid you are confused. You can use bit/doorbell-6 by passing 0x40 to mhu as the data to send. Cheers!
Re: [PATCH v4 2/2] mailbox: sprd: Add Spreadtrum mailbox driver
On Tue, May 12, 2020 at 11:14 PM Baolin Wang wrote: > > Hi Jassi, > > On Thu, May 7, 2020 at 11:23 AM Baolin Wang wrote: > > > > Hi Jassi, > > > > On Thu, May 7, 2020 at 7:25 AM Jassi Brar wrote: > > > > > > On Wed, May 6, 2020 at 8:29 AM Baolin Wang wrote: > > > > > > > > Hi Jassi, > > > > > > > > On Tue, Apr 28, 2020 at 11:10 AM Baolin Wang > > > > wrote: > > > > > > > > > > From: Baolin Wang > > > > > > > > > > The Spreadtrum mailbox controller supports 8 channels to communicate > > > > > with MCUs, and it contains 2 different parts: inbox and outbox, which > > > > > are used to send and receive messages by IRQ mode. > > > > > > > > > > Signed-off-by: Baolin Wang > > > > > Signed-off-by: Baolin Wang > > > > > --- > > > > > Changes from v3: > > > > > - Save the id in mbox_chan.con_priv and remove the 'sprd_mbox_chan' > > > > > > > > > > Changes from v2: > > > > > - None. > > > > > > > > > > Changes from v1: > > > > > - None > > > > > > > > Gentle ping, do you have any other comments? Thanks. > > > > > > > Yea, I am still not sure about the error returned in send_data(). It > > > will either never hit or there will be no easy recovery from it. The > > > api expects the driver to tell it the last-tx was done only when it > > > can send the next message. (There may be case like sending depend on > > > remote, which can't be ensured before hand). > > > > Actually this is an unusual case, suppose the remote target did not > > fetch the message as soon as possile, which will cause the FIFO > > overflow, so in this case we can not send messages to the remote > > target any more, otherwise messages will be lost. Thus we can return > > errors to users to indicate that something wrong with the remote > > target need to be checked. > > > > So this validation in send_data() is mostly for debugging for this > > abnormal case and we will not trigger this issue if the remote target > > works well. So I think it is useful to keep this validation in > > send_data(). Thanks. > > Any comments? Thanks. > Same as my last post. thnx
Re: [PATCH v4 2/2] mailbox: sprd: Add Spreadtrum mailbox driver
On Wed, May 6, 2020 at 8:29 AM Baolin Wang wrote: > > Hi Jassi, > > On Tue, Apr 28, 2020 at 11:10 AM Baolin Wang wrote: > > > > From: Baolin Wang > > > > The Spreadtrum mailbox controller supports 8 channels to communicate > > with MCUs, and it contains 2 different parts: inbox and outbox, which > > are used to send and receive messages by IRQ mode. > > > > Signed-off-by: Baolin Wang > > Signed-off-by: Baolin Wang > > --- > > Changes from v3: > > - Save the id in mbox_chan.con_priv and remove the 'sprd_mbox_chan' > > > > Changes from v2: > > - None. > > > > Changes from v1: > > - None > > Gentle ping, do you have any other comments? Thanks. > Yea, I am still not sure about the error returned in send_data(). It will either never hit or there will be no easy recovery from it. The api expects the driver to tell it the last-tx was done only when it can send the next message. (There may be case like sending depend on remote, which can't be ensured before hand). thnx
Re: [BUGFIX PATCH] selftests: Use real temporary working directory for archiving
On Fri, 4 Oct 2019 at 00:13, Masami Hiramatsu wrote:
>
> Use real temporary working directory for generating kselftest
> archive.
>
> tools/testing/selftests/kselftest directory has been used for
> the temporary working directory for making a tar archive from
> gen_kselftest_tar.sh, and it removes the directory for cleanup.
>
> However, since the kselftest directory became a part of the
> repository, it must not be used as a working dir.
>
> Introduce mktemp to prepare a temporary working directory
> for archiving kselftests.
>
> Signed-off-by: Masami Hiramatsu
> ---
> tools/testing/selftests/gen_kselftest_tar.sh |8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/gen_kselftest_tar.sh
> b/tools/testing/selftests/gen_kselftest_tar.sh
> index a27e2eec3586..eba1e9987ffc 100755
> --- a/tools/testing/selftests/gen_kselftest_tar.sh
> +++ b/tools/testing/selftests/gen_kselftest_tar.sh
> @@ -38,16 +38,16 @@ main()
> esac
> fi
>
> - install_dir=./kselftest
> + tmpdir=`mktemp -d ./install-XX` || exit 1
>
> # Run install using INSTALL_KSFT_PATH override to generate install
> # directory
> -./kselftest_install.sh
> -tar $copts kselftest${ext} $install_dir
> +./kselftest_install.sh $tmpdir
> +tar $copts kselftest${ext} -C $tmpdir kselftest
> echo "Kselftest archive kselftest${ext} created!"
>
> # clean up install directory
> -rm -rf kselftest
> +rm -rf $tmpdir
> }
>
> main "$@"
>
FWIW, Acked-by: Jassi Brar
--
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106 -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
[GIT PULL] Mailbox changes for v5.4
Hi Linus, The following changes since commit f74c2bb98776e2de508f4d607cd519873065118e: Linux 5.3-rc8 (2019-09-08 13:33:15 -0700) are available in the Git repository at: git://git.linaro.org/landing-teams/working/fujitsu/integration.git tags/mailbox-v5.4 for you to fetch changes up to 556a0964e28c4441dcdd50fb07596fd042246bd5: mailbox: qcom-apcs: fix max_register value (2019-09-17 00:54:29 -0500) - qcom : enable support for ipq8074, sm1850 and sm7180. add child device node for qcs404. misc fixes. - mediatek : enable support for mt8183. misc rejig of cmdq driver. new client-reg dt property. - armada: use device-managed registration api Bibby Hsieh (6): dt-binding: gce: remove thread-num property dt-binding: gce: add gce header file for mt8183 dt-binding: gce: add binding for gce client reg property mailbox: mediatek: cmdq: move the CMDQ_IRQ_MASK into cmdq driver data mailbox: mediatek: cmdq: support mt8183 gce function mailbox: mediatek: cmdq: clear the event in cmdq initial flow Chuhong Yuan (1): mailbox: armada-37xx-rwtm: Use device-managed registration API Gokul Sriram Palanisamy (2): dt-bindings: mailbox: qom: Add ipq8074 APPS compatible mailbox: qcom: Add support for IPQ8074 APCS Jorge Ramirez-Ortiz (3): mbox: qcom: add APCS child device for QCS404 mbox: qcom: replace integer with valid macro mailbox: qcom-apcs: fix max_register value Sibi Sankar (2): dt-bindings: mailbox: Add APSS shared for SM8150 and SC7180 SoCs mailbox: qcom: Add support for Qualcomm SM8150 and SC7180 SoCs .../devicetree/bindings/mailbox/mtk-gce.txt| 23 ++- .../bindings/mailbox/qcom,apcs-kpss-global.txt | 3 + drivers/mailbox/armada-37xx-rwtm-mailbox.c | 14 +- drivers/mailbox/mtk-cmdq-mailbox.c | 18 ++- drivers/mailbox/qcom-apcs-ipc-mailbox.c| 16 +- include/dt-bindings/gce/mt8183-gce.h | 175 + include/linux/mailbox/mtk-cmdq-mailbox.h | 3 + include/linux/soc/mediatek/mtk-cmdq.h | 3 - 8 files changed, 222 insertions(+), 33 deletions(-) create mode 100644 include/dt-bindings/gce/mt8183-gce.h
Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Wed, Sep 18, 2019 at 9:46 AM Andre Przywara wrote: > > On Wed, 18 Sep 2019 09:19:46 -0500 > Jassi Brar wrote: > > Hi, > > > On Wed, Sep 18, 2019 at 4:44 AM Andre Przywara > > wrote: > > > > > > > > > which needs 9 arguments to work. The fact that the fist argument is > > > > always going to be same on a platform is just the way we use this > > > > instruction. > > > > > > > > > We should be as strict as possible to avoid any security issues. > > > > > > > > > Any example of such a security issue? > > > > > > Someone finds a way to trick some mailbox client to send a crafted > > > message to the mailbox. > > > > > What if someone finds a way to trick the block layer to erase 'sda' ? > > Yes, the Linux block driver control the whole block device, it can do > whatever it wants. > Sorry, it doesn't make any sense. > > That is called "bug in the code". > > It does happen in every subsystem but we don't stop implementing new > > features we write flexible code and then fix any bug. > > > > > > > Do you have any example of a use case where the mailbox client needs to > > > provide the function ID? > > > > > FSL_SIP_SCMI_1/2 ? > > Huh? Where does the SCPI or SCMI driver provide this? Those clients don't > even provide any arguments. Adding some would defeat the whole point of > having this mailbox in the first place, which was to provide a drop-in > replacement for a hardware mailbox device used on other platforms. > SCPI/SCMI implementation is broken. I did NAK it. With the 'smc' mailbox you may get away without have to program the channel before transmit, but not every controller is natively so. > > But that is not the main point, which is to be consistent (not > > ignoring first argument because someone may find a bug to exploit) and > > flexible. > > Please read the SMCCC[1]: The first argument is in r1/w1/x1. r0/w0 is the > function ID, and this is a specific value (fixed by the firmware > implementation, see Peng's ATF patch) and not up to be guessed by a client. > The first argument of smc call is the function-id arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4, arg5, 0, ); > > That's why I think the function ID (which is part of the SMCCC protocol, not > of the mailbox service!) should just be set in the controller DT node and > nowhere else. > Actually that is the very reason func-id should be a client thing and passed via client's DT node :) It is general understanding that protocol specific bits should not be a part of controller driver, but the client(protocol) driver. Page-7 Function-ID specifies :- 1) The service to be invoked. 2) The function to be invoked. 3) The calling convention (32-bit or 64-bit) that is in use. 4) The call type (fast or yielding) that is in use. Even if we turn blind to 2,3 & 4, but (1) shouts like a runtime property. Thanks.
Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox
On Wed, Sep 18, 2019 at 8:58 AM Andre Przywara wrote: > > > > Also there is mbox_chan_txdone() with which a controller driver can > > > signal TX completion explicitly. > > > > > No. Controller can use that only if it has specified txdone_irq, which > > is not the case here. > > I see. So does the framework handle the case where both txdone_poll and > txdone_irq are false? > Of course. If there is no IRQ or POLL mechanism for controller to detect tx-done, the only way left is for client driver to know by some 'ack' response (if any). The client should call mbox_client_txdone() Thanks
Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Wed, Sep 18, 2019 at 4:44 AM Andre Przywara wrote: > > > which needs 9 arguments to work. The fact that the fist argument is > > always going to be same on a platform is just the way we use this > > instruction. > > > > > We should be as strict as possible to avoid any security issues. > > > > > Any example of such a security issue? > > Someone finds a way to trick some mailbox client to send a crafted message to > the mailbox. > What if someone finds a way to trick the block layer to erase 'sda' ? That is called "bug in the code". It does happen in every subsystem but we don't stop implementing new features we write flexible code and then fix any bug. > Do you have any example of a use case where the mailbox client needs to > provide the function ID? > FSL_SIP_SCMI_1/2 ? But that is not the main point, which is to be consistent (not ignoring first argument because someone may find a bug to exploit) and flexible. > > > The firmware certainly knows the function ID it implements. The firmware > > > controls the DT. So it is straight-forward to put the ID into the DT. The > > > firmware could even do this at boot time, dynamically, before passing on > > > the DT to the non-secure world (bootloader or kernel). > > > > > > What would be the use case of this functionality? > > > > > At least for flexibility and consistency. > > I appreciate the flexibility idea, but when creating an interface, especially > a generic one to any kind of firmware, you should be as strict as possible, > to avoid clashes in the future. > I really don't see how there can be clashes with more complete and flexible implementation. Thanks.
Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Wed, Sep 18, 2019 at 3:53 AM Peng Fan wrote: > > > > > > > + > > > > + "#mbox-cells": > > > > +const: 1 > > > > > > Why is this "1"? What is this number used for? It used to be the channel > > > ID, > > but since you are describing a single channel controller only, this should > > be 0 > > now. > > > > > Yes. I overlooked it and actually queued the patch for pull request. > > In Documentation/devicetree/bindings/mailbox/mailbox.txt > #mbox-cells: Must be at least 1. > > So I use 1 here, 0 not work. Because of_mbox_index_xlate expect at least 1 > here. > So I need modify Documentation/devicetree/bindings/mailbox/mailbox.txt > and add xlate for smc mailbox? > No, you just can not use the generic xlate() provided by the api. Please implement your own xlate() that requires no argument. Cheers!
Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox
On Wed, Sep 18, 2019 at 5:00 AM Andre Przywara wrote:
> >
> > >
> > > > + };
> > > > +};
> > >
> > > If this is the data structure that this mailbox controller uses, I would
> > > expect
> > > this to be documented somewhere, or even exported.
> >
> > Export this structure in include/linux/mailbox/smc-mailbox.h?
>
> For instance, even though I am not sure this is really desired or helpful,
> since we expect a generic mailbox client (the SCPI or SCMI driver) to deal
> with that mailbox.
>
> But at least the expected format should be documented, which could just be in
> writing, not necessarily in code.
>
Yes, the packet format is specified by the controller and defined in
some header for clients to include. Other platforms do that already.
> > > > +
> > > > +typedef unsigned long (smc_mbox_fn)(unsigned int, unsigned long,
> > > > + unsigned long, unsigned long,
> > > > + unsigned long, unsigned long,
> > > > + unsigned long);
> > > > +static smc_mbox_fn *invoke_smc_mbox_fn;
> > > > +
> > > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > > + struct arm_smc_chan_data *chan_data = link->con_priv;
> > > > + struct arm_smccc_mbox_cmd *cmd = data;
> > > > + unsigned long ret;
> > > > + u32 function_id;
> > > > +
> > > > + function_id = chan_data->function_id;
> > > > + if (!function_id)
> > > > + function_id = cmd->function_id;
> > > > +
> > > > + if (function_id & BIT(30)) {
> > >
> > > if (ARM_SMCCC_IS_64(function_id)) {
> >
> > ok
> >
> > >
> > > > + ret = invoke_smc_mbox_fn(function_id, cmd->args_smccc64[0],
> > > > + cmd->args_smccc64[1],
> > > > + cmd->args_smccc64[2],
> > > > + cmd->args_smccc64[3],
> > > > + cmd->args_smccc64[4],
> > > > + cmd->args_smccc64[5]);
> > > > + } else {
> > > > + ret = invoke_smc_mbox_fn(function_id, cmd->args_smccc32[0],
> > > > + cmd->args_smccc32[1],
> > > > + cmd->args_smccc32[2],
> > > > + cmd->args_smccc32[3],
> > > > + cmd->args_smccc32[4],
> > > > + cmd->args_smccc32[5]);
> > > > + }
> > > > +
> > > > + mbox_chan_received_data(link, (void *)ret);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static unsigned long __invoke_fn_hvc(unsigned int function_id,
> > > > + unsigned long arg0, unsigned long arg1,
> > > > + unsigned long arg2, unsigned long arg3,
> > > > + unsigned long arg4, unsigned long arg5) {
> > > > + struct arm_smccc_res res;
> > > > +
> > > > + arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4,
> > > > + arg5, 0, );
> > > > + return res.a0;
> > > > +}
> > > > +
> > > > +static unsigned long __invoke_fn_smc(unsigned int function_id,
> > > > + unsigned long arg0, unsigned long arg1,
> > > > + unsigned long arg2, unsigned long arg3,
> > > > + unsigned long arg4, unsigned long arg5) {
> > > > + struct arm_smccc_res res;
> > > > +
> > > > + arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4,
> > > > + arg5, 0, );
> > > > + return res.a0;
> > > > +}
> > > > +
> > > > +static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
> > > > + .send_data = arm_smc_send_data,
> > > > +};
> > > > +
> > > > +static int arm_smc_mbox_probe(struct platform_device *pdev) {
> > > > + struct device *dev = >dev;
> > > > + struct mbox_controller *mbox;
> > > > + struct arm_smc_chan_data *chan_data;
> > > > + int ret;
> > > > + u32 function_id = 0;
> > > > +
> > > > + if (of_device_is_compatible(dev->of_node, "arm,smc-mbox"))
> > > > + invoke_smc_mbox_fn = __invoke_fn_smc;
> > > > + else
> > > > + invoke_smc_mbox_fn = __invoke_fn_hvc;
> > > > +
> > > > + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> > > > + if (!mbox)
> > > > + return -ENOMEM;
> > > > +
> > > > + mbox->num_chans = 1;
> > > > + mbox->chans = devm_kzalloc(dev, sizeof(*mbox->chans), GFP_KERNEL);
> > > > + if (!mbox->chans)
> > > > + return -ENOMEM;
> > > > +
> > > > + chan_data = devm_kzalloc(dev, sizeof(*chan_data), GFP_KERNEL);
> > > > + if (!chan_data)
> > > > + return -ENOMEM;
> > > > +
> > > > + of_property_read_u32(dev->of_node, "arm,func-id", _id);
> > > > + chan_data->function_id = function_id;
> > > > +
> > > > + mbox->chans->con_priv = chan_data;
> > > > +
> > > > + mbox->txdone_poll = false;
> > > > + mbox->txdone_irq = false;
> > >
> > > Don't we need to provide something to confirm reception to the client? In
> > > our
> > > case we can set this as soon as
Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Tue, Sep 17, 2019 at 12:31 PM Andre Przywara wrote: > > On Mon, 16 Sep 2019 09:44:37 + > Peng Fan wrote: > > Hi, > > > From: Peng Fan > > > > The ARM SMC/HVC mailbox binding describes a firmware interface to trigger > > actions in software layers running in the EL2 or EL3 exception levels. > > The term "ARM" here relates to the SMC instruction as part of the ARM > > instruction set, not as a standard endorsed by ARM Ltd. > > > > Signed-off-by: Peng Fan > > --- > > .../devicetree/bindings/mailbox/arm-smc.yaml | 96 > > ++ > > 1 file changed, 96 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml > > > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml > > b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml > > new file mode 100644 > > index ..bf01bec035fc > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml > > @@ -0,0 +1,96 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ARM SMC Mailbox Interface > > + > > +maintainers: > > + - Peng Fan > > + > > +description: | > > + This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor > > I think "or" instead of "and" is less confusing. > > > + call) instruction to trigger a mailbox-connected activity in firmware, > > + executing on the very same core as the caller. The value of r0/w0/x0 > > + the firmware returns after the smc call is delivered as a received > > + message to the mailbox framework, so synchronous communication can be > > + established. The exact meaning of the action the mailbox triggers as > > + well as the return value is defined by their users and is not subject > > + to this binding. > > + > > + One use case of this mailbox is the SCMI interface, which uses shared > > One example use case of this mailbox ... > (to make it more obvious that it's not restricted to this) > > > + memory to transfer commands and parameters, and a mailbox to trigger a > > + function call. This allows SoCs without a separate management processor > > + (or when such a processor is not available or used) to use this > > + standardized interface anyway. > > + > > + This binding describes no hardware, but establishes a firmware interface. > > + Upon receiving an SMC using one of the described SMC function > > identifiers, > > ... the described SMC function identifier, > > > + the firmware is expected to trigger some mailbox connected functionality. > > + The communication follows the ARM SMC calling convention. > > + Firmware expects an SMC function identifier in r0 or w0. The supported > > + identifiers are passed from consumers, > > identifier > > "passed from consumers": How? Where? > But I want to repeat: We should not allow this. > This is a binding for a mailbox controller driver, not a generic firmware > backdoor. > Exactly. The mailbox controller here is the SMC/HVC instruction, which needs 9 arguments to work. The fact that the fist argument is always going to be same on a platform is just the way we use this instruction. > We should be as strict as possible to avoid any security issues. > Any example of such a security issue? > The firmware certainly knows the function ID it implements. The firmware > controls the DT. So it is straight-forward to put the ID into the DT. The > firmware could even do this at boot time, dynamically, before passing on the > DT to the non-secure world (bootloader or kernel). > > What would be the use case of this functionality? > At least for flexibility and consistency. > > or listed in the the arm,func-ids > >arm,func-id > > > + properties as described below. The firmware can return one value in > > property > > > + the first SMC result register, it is expected to be an error value, > > + which shall be propagated to the mailbox client. > > + > > + Any core which supports the SMC or HVC instruction can be used, as long > > + as a firmware component running in EL3 or EL2 is handling these calls. > > + > > +properties: > > + compatible: > > +oneOf: > > + - description: > > + For implementations using ARM SMC instruction. > > +const: arm,smc-mbox > > + > > + - description: > > + For implementations using ARM HVC instruction. > > +const: arm,hvc-mbox > > I am not particularly happy with this, but well ... > > > + > > + "#mbox-cells": > > +const: 1 > > Why is this "1"? What is this number used for? It used to be the channel ID, > but since you are describing a single channel controller only, this should be > 0 now. > Yes. I overlooked it and actually queued the patch for pull request. But I think the bindings should not carry a 'fix' patch later.
Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Wed, Sep 11, 2019 at 10:03 AM Andre Przywara wrote:
>
> On Tue, 10 Sep 2019 21:44:11 -0500
> Jassi Brar wrote:
>
> Hi,
>
> > On Mon, Sep 9, 2019 at 10:42 AM Andre Przywara
> > wrote:
> > >
> > > On Wed, 28 Aug 2019 03:02:58 +
> > > Peng Fan wrote:
> > >
> [ ... ]
> > >
> > > > +
> > > > + arm,func-ids:
> > > > +description: |
> > > > + An array of 32-bit values specifying the function IDs used by
> > > > each
> > > > + mailbox channel. Those function IDs follow the ARM SMC calling
> > > > + convention standard [1].
> > > > +
> > > > + There is one identifier per channel and the number of supported
> > > > + channels is determined by the length of this array.
> > >
> > > I think this makes it obvious that arm,num-chans is not needed.
> > >
> > > Also this somewhat contradicts the driver implementation, which allows
> > > the array to be shorter, marking this as UINT_MAX and later on using the
> > > first data item as a function identifier. This is somewhat surprising and
> > > not documented (unless I missed something).
> > >
> > > So I would suggest:
> > > - We drop the transports property, and always put the client provided
> > > data in the registers, according to the SMCCC. Document this here.
> > > A client not needing those could always puts zeros (or garbage) in
> > > there, the respective firmware would just ignore the registers.
> > > - We drop "arm,num-chans", as this is just redundant with the length of
> > > the func-ids array.
> > > - We don't impose an arbitrary limit on the number of channels. From the
> > > firmware point of view this is just different function IDs, from Linux'
> > > point of view just the size of the memory used. Both don't need to be
> > > limited artificially IMHO.
> > >
> > Sounds like we are in sync.
> >
> > > - We mark arm,func-ids as required, as this needs to be fixed, allocated
> > > number.
> > >
> > I still think func-id can be done without. A client can always pass
> > the value as it knows what it expects.
>
> I don't think it's the right abstraction. The mailbox *controller* uses a
> specific func-id, this has to match the one the firmware expects. So this is
> a property of the mailbox transport channel (the SMC call), and the *client*
> should *not* care about it. It just sees the logical channel ID (if we have
> one), which the controller translates into the func-ID.
>
arg0 is special only to the client/protocol, otherwise it is simply
the first argument for the arm_smccc_smc *instruction* controller.
arg[1,7] are already provided by the client, so it is only neater if
arg0 is also taken from the client.
But as I said, I am still ok if func-id is passed from dt and arg0
from client is ignored because we have one channel per controller
design and we don't have to worry about number of channels there can
be dedicated to specific functions.
> So it should really look like this (assuming only single channel controllers):
> mailbox: smc-mailbox {
> #mbox-cells = <0>;
> compatible = "arm,smc-mbox";
> method = "smc";
>
Do we want to do away with 'method' property and use different
'compatible' properties instead?
compatible = "arm,smc-mbox"; orcompatible = "arm,hvc-mbox";
> arm,func-id = <0x82fe>;
> };
> scmi {
> compatible = "arm,scmi";
> mboxes = <_mbox>;
> mbox-names = "tx"; /* rx is optional */
> shmem = <_scp_hpri>;
> };
>
> If you allow the client to provide the function ID (and I am not saying this
> is a good idea): where would this func ID come from? It would need to be a
> property of the client DT node, then. So one way would be to use the func ID
> as the Linux mailbox channel ID:
> mailbox: smc-mailbox {
> #mbox-cells = <1>;
> compatible = "arm,smc-mbox";
> method = "smc";
> };
> scmi {
> compatible = "arm,scmi";
> mboxes = <_mbox 0x82fe>;
> mbox-names = "tx"; /* rx is optional */
> shmem = <_scp_hpri>;
> };
>
> But this doesn't look desirable.
>
> And as I mentioned this before: allowing some mailbox clients to provide the
> function IDs sound scary, as they could use anything they want, triggering
> random firmware actions (think PSCI_CPU_OFF).
>
That paranoia is unwarranted. We have to keep faith in kernel-space
code doing the right thing.
Either the illegitimate function request should be rejected by the
firmware or client driver be called buggy just as we would call a
block device driver buggy if it messed up the sector numbers in a
write request.
thnx.
Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Mon, Sep 9, 2019 at 10:42 AM Andre Przywara wrote: > > On Wed, 28 Aug 2019 03:02:58 + > Peng Fan wrote: > > Hi, > > sorry for the late reply, eventually managed to have a closer look on this. > > > From: Peng Fan > > > > The ARM SMC/HVC mailbox binding describes a firmware interface to trigger > > actions in software layers running in the EL2 or EL3 exception levels. > > The term "ARM" here relates to the SMC instruction as part of the ARM > > instruction set, not as a standard endorsed by ARM Ltd. > > > > Signed-off-by: Peng Fan > > --- > > .../devicetree/bindings/mailbox/arm-smc.yaml | 125 > > + > > 1 file changed, 125 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml > > > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml > > b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml > > new file mode 100644 > > index ..f8eb28d5e307 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml > > @@ -0,0 +1,125 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ARM SMC Mailbox Interface > > + > > +maintainers: > > + - Peng Fan > > + > > +description: | > > + This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor > > + call) instruction to trigger a mailbox-connected activity in firmware, > > + executing on the very same core as the caller. By nature this operation > > + is synchronous and this mailbox provides no way for asynchronous messages > > + to be delivered the other way round, from firmware to the OS, but > > + asynchronous notification could also be supported. However the value of > > + r0/w0/x0 the firmware returns after the smc call is delivered as a > > received > > + message to the mailbox framework, so a synchronous communication can be > > + established, for a asynchronous notification, no value will be returned. > > + The exact meaning of both the action the mailbox triggers as well as the > > + return value is defined by their users and is not subject to this > > binding. > > + > > + One use case of this mailbox is the SCMI interface, which uses shared > > memory > > + to transfer commands and parameters, and a mailbox to trigger a function > > + call. This allows SoCs without a separate management processor (or when > > + such a processor is not available or used) to use this standardized > > + interface anyway. > > + > > + This binding describes no hardware, but establishes a firmware interface. > > + Upon receiving an SMC using one of the described SMC function > > identifiers, > > + the firmware is expected to trigger some mailbox connected functionality. > > + The communication follows the ARM SMC calling convention. > > + Firmware expects an SMC function identifier in r0 or w0. The supported > > + identifiers are passed from consumers, or listed in the the arm,func-ids > > + properties as described below. The firmware can return one value in > > + the first SMC result register, it is expected to be an error value, > > + which shall be propagated to the mailbox client. > > + > > + Any core which supports the SMC or HVC instruction can be used, as long > > as > > + a firmware component running in EL3 or EL2 is handling these calls. > > + > > +properties: > > + compatible: > > +const: arm,smc-mbox > > + > > + "#mbox-cells": > > +const: 1 > > + > > + arm,num-chans: > > +description: The number of channels supported. > > +items: > > + minimum: 1 > > + maximum: 4096 # Should be enough? > > This maximum sounds rather arbitrary. Why do we need one? In the driver this > just allocates more memory, so why not just impose no artificial limit at all? > This will be gone, once the driver is converted to 1channel per controller. > Actually, do we need this property at all? Can't we just rely on the size of > arm,func-ids to determine this (using of_property_count_elems_of_size() in > the driver)? Having both sounds redundant and brings up the question what to > do if they don't match. > > > + > > + method: > > +- enum: > > +- smc > > +- hvc > > + > > + transports: > > +- enum: > > +- mem > > +- reg > > Shouldn't there be a description on what both mean, exactly? > For instance I would expect a list of registers to be shown for the "reg" > case, and be it by referring to the ARM SMCCC. > > Also looking at the driver this brings up more questions: > - Which memory does mem refer to? If this is really the means of transport, > it should be referenced in this *controller* node and populated by the > driver. Looking at the example below and the driver code, it actually isn't > used that way, instead the memory is used and controlled by the mailbox > *client*. > -
Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Mon, Sep 9, 2019 at 8:32 AM Andre Przywara wrote:
>
> On Fri, 30 Aug 2019 03:12:29 -0500
> Jassi Brar wrote:
>
> Hi,
>
> > On Fri, Aug 30, 2019 at 3:07 AM Peng Fan wrote:
> > >
> > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for
> > > > the ARM
> > > > SMC/HVC mailbox
> > > >
> > > > On Fri, Aug 30, 2019 at 2:37 AM Peng Fan wrote:
> > > > >
> > > > > Hi Jassi,
> > > > >
> > > > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc
> > > > > > for the ARM SMC/HVC mailbox
> > > > > >
> > > > > > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan wrote:
> > > > > >
> > > > > > > > > +examples:
> > > > > > > > > + - |
> > > > > > > > > +sram@91 {
> > > > > > > > > + compatible = "mmio-sram";
> > > > > > > > > + reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > > > > > + #address-cells = <1>;
> > > > > > > > > + #size-cells = <1>;
> > > > > > > > > + ranges = <0 0x0 0x93f000 0x1000>;
> > > > > > > > > +
> > > > > > > > > + cpu_scp_lpri: scp-shmem@0 {
> > > > > > > > > +compatible = "arm,scmi-shmem";
> > > > > > > > > +reg = <0x0 0x200>;
> > > > > > > > > + };
> > > > > > > > > +
> > > > > > > > > + cpu_scp_hpri: scp-shmem@200 {
> > > > > > > > > +compatible = "arm,scmi-shmem";
> > > > > > > > > +reg = <0x200 0x200>;
> > > > > > > > > + };
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > +firmware {
> > > > > > > > > + smc_mbox: mailbox {
> > > > > > > > > +#mbox-cells = <1>;
> > > > > > > > > +compatible = "arm,smc-mbox";
> > > > > > > > > +method = "smc";
> > > > > > > > > +arm,num-chans = <0x2>;
> > > > > > > > > +transports = "mem";
> > > > > > > > > +/* Optional */
> > > > > > > > > +arm,func-ids = <0xc2fe>, <0xc2ff>;
> > > > > > > > >
> > > > > > > > SMC/HVC is synchronously(block) running in "secure mode", i.e,
> > > > > > > > there can only be one instance running platform wide. Right?
> > > > > > >
> > > > > > > I think there could be channel for TEE, and channel for Linux.
> > > > > > > For virtualization case, there could be dedicated channel for
> > > > > > > each VM.
> > > > > > >
> > > > > > I am talking from Linux pov. Functions 0xfe and 0xff above, can't
> > > > > > both be active at the same time, right?
> > > > >
> > > > > If I get your point correctly,
> > > > > On UP, both could not be active. On SMP, tx/rx could be both active,
> > > > > anyway this depends on secure firmware and Linux firmware design.
> > > > >
> > > > > Do you have any suggestions about arm,func-ids here?
> > > > >
> > > > I was thinking if this is just an instruction, why can't each channel be
> > > > represented as a controller, i.e, have exactly one func-id per
> > > > controller node.
> > > > Define as many controllers as you need channels ?
> > >
> > > I am ok, this could make driver code simpler. Something as below?
> > >
> > > smc_tx_mbox: tx_mbox {
> > > #mbox-cells = <0>;
> > > compatible = "arm,smc-mbox";
> > > method = "smc";
> > > transports = "mem";
> > > arm,func-id = <0xc2fe>;
> > > };
> > >
> > > smc_rx_mbox: rx_mbox {
> > > #mbox-cells = <0>;
> > > compatible = "arm,smc-mbox";
> > > method = "smc";
> > > transports = "mem";
> > > arm,func-id = <0xc2ff>;
> > > };
> > >
> > > firmware {
> > > scmi {
> > > compatible = "arm,scmi";
> > > mboxes = <_tx_mbox>, <_rx_mbox 1>;
> > > mbox-names = "tx", "rx";
> > > shmem = <_scp_lpri>, <_scp_hpri>;
> > > };
> > > };
> > >
> > Yes, the channel part is good.
> > But I am not convinced by the need to have SCMI specific "transport" mode.
>
> Why would this be SCMI specific and what is the problem with having this
> property?
> By the very nature of the SMC/HVC call you would expect to also pass
> parameters in registers.
> However this limits the amount of data you can push, so the option of
> reverting to a
> memory based payload sounds very reasonable.
>
Of course, it is very legit to pass data via mem and many platforms do
that. But as you note in your next post, the 'transport' doesn't seem
necessary doing what it does in the driver.
Cheers!
Re: [PATCH v2 1/2] dt-bindings: milbeaut-m10v-hdmac: Add Socionext Milbeaut HDMAC bindings
On Wed, Sep 4, 2019 at 12:51 AM Vinod Koul wrote:
>
> On 18-08-19, 00:17, jassisinghb...@gmail.com wrote:
> > From: Jassi Brar
> >
> > Document the devicetree bindings for Socionext Milbeaut HDMAC
> > controller. Controller has upto 8 floating channels, that need
> > a predefined slave-id to work from a set of slaves.
> >
> > Signed-off-by: Jassi Brar
> > ---
> > .../bindings/dma/milbeaut-m10v-hdmac.txt | 32 +++
> > 1 file changed, 32 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/dma/milbeaut-m10v-hdmac.txt
> >
> > diff --git a/Documentation/devicetree/bindings/dma/milbeaut-m10v-hdmac.txt
> > b/Documentation/devicetree/bindings/dma/milbeaut-m10v-hdmac.txt
> > new file mode 100644
> > index ..f0960724f1c7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma/milbeaut-m10v-hdmac.txt
> > @@ -0,0 +1,32 @@
> > +* Milbeaut AHB DMA Controller
> > +
> > +Milbeaut AHB DMA controller has transfer capability bellow.
>
> s/bellow/below:
>
> > + - device to memory transfer
> > + - memory to device transfer
> > +
> > +Required property:
> > +- compatible: Should be "socionext,milbeaut-m10v-hdmac"
> > +- reg: Should contain DMA registers location and length.
> > +- interrupts: Should contain all of the per-channel DMA interrupts.
> > + Number of channels is configurable - 2, 4 or 8, so
> > + the number of interrupts specfied should be {2,4,8}.
>
> s/specfied/specified
>
Hi Vinod,
Do you want me to spin yet another revision for the two types in text?
thnx
Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Fri, Aug 30, 2019 at 4:32 AM Sudeep Holla wrote: > > On Fri, Aug 30, 2019 at 02:52:40AM -0500, Jassi Brar wrote: > > On Fri, Aug 30, 2019 at 2:37 AM Peng Fan wrote: > > [...] > > > > > > > If I get your point correctly, > > > On UP, both could not be active. On SMP, tx/rx could be both active, > > > anyway > > > this depends on secure firmware and Linux firmware design. > > > > > > Do you have any suggestions about arm,func-ids here? > > > > > I was thinking if this is just an instruction, why can't each channel > > be represented as a controller, i.e, have exactly one func-id per > > controller node. Define as many controllers as you need channels ? > > > > I might have missed to follow this, but what's the advantage of doing so ? > Which can't single controller instance deal with all the channels ? > There are many advantages ... 1) Design reflects the reality - two smc/hvc instructions have nothing tying them together. 2) Driver code becomes simpler - don't have to pre-populate channels, deducting from the size of func-ids array. 3) Driver becomes more flexible - We can have channels that pass func-id runtime and channels that pass via DT (if we must have the option of DT property). -jassi
Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Fri, Aug 30, 2019 at 3:07 AM Peng Fan wrote:
>
> > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the
> > ARM
> > SMC/HVC mailbox
> >
> > On Fri, Aug 30, 2019 at 2:37 AM Peng Fan wrote:
> > >
> > > Hi Jassi,
> > >
> > > > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc
> > > > for the ARM SMC/HVC mailbox
> > > >
> > > > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan wrote:
> > > >
> > > > > > > +examples:
> > > > > > > + - |
> > > > > > > +sram@91 {
> > > > > > > + compatible = "mmio-sram";
> > > > > > > + reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > > > + #address-cells = <1>;
> > > > > > > + #size-cells = <1>;
> > > > > > > + ranges = <0 0x0 0x93f000 0x1000>;
> > > > > > > +
> > > > > > > + cpu_scp_lpri: scp-shmem@0 {
> > > > > > > +compatible = "arm,scmi-shmem";
> > > > > > > +reg = <0x0 0x200>;
> > > > > > > + };
> > > > > > > +
> > > > > > > + cpu_scp_hpri: scp-shmem@200 {
> > > > > > > +compatible = "arm,scmi-shmem";
> > > > > > > +reg = <0x200 0x200>;
> > > > > > > + };
> > > > > > > +};
> > > > > > > +
> > > > > > > +firmware {
> > > > > > > + smc_mbox: mailbox {
> > > > > > > +#mbox-cells = <1>;
> > > > > > > +compatible = "arm,smc-mbox";
> > > > > > > +method = "smc";
> > > > > > > +arm,num-chans = <0x2>;
> > > > > > > +transports = "mem";
> > > > > > > +/* Optional */
> > > > > > > +arm,func-ids = <0xc2fe>, <0xc2ff>;
> > > > > > >
> > > > > > SMC/HVC is synchronously(block) running in "secure mode", i.e,
> > > > > > there can only be one instance running platform wide. Right?
> > > > >
> > > > > I think there could be channel for TEE, and channel for Linux.
> > > > > For virtualization case, there could be dedicated channel for each VM.
> > > > >
> > > > I am talking from Linux pov. Functions 0xfe and 0xff above, can't
> > > > both be active at the same time, right?
> > >
> > > If I get your point correctly,
> > > On UP, both could not be active. On SMP, tx/rx could be both active,
> > > anyway this depends on secure firmware and Linux firmware design.
> > >
> > > Do you have any suggestions about arm,func-ids here?
> > >
> > I was thinking if this is just an instruction, why can't each channel be
> > represented as a controller, i.e, have exactly one func-id per controller
> > node.
> > Define as many controllers as you need channels ?
>
> I am ok, this could make driver code simpler. Something as below?
>
> smc_tx_mbox: tx_mbox {
> #mbox-cells = <0>;
> compatible = "arm,smc-mbox";
> method = "smc";
> transports = "mem";
> arm,func-id = <0xc2fe>;
> };
>
> smc_rx_mbox: rx_mbox {
> #mbox-cells = <0>;
> compatible = "arm,smc-mbox";
> method = "smc";
> transports = "mem";
> arm,func-id = <0xc2ff>;
> };
>
> firmware {
> scmi {
> compatible = "arm,scmi";
> mboxes = <_tx_mbox>, <_rx_mbox 1>;
> mbox-names = "tx", "rx";
> shmem = <_scp_lpri>, <_scp_hpri>;
> };
> };
>
Yes, the channel part is good.
But I am not convinced by the need to have SCMI specific "transport" mode.
thanks
Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Fri, Aug 30, 2019 at 2:37 AM Peng Fan wrote:
>
> Hi Jassi,
>
> > Subject: Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the
> > ARM
> > SMC/HVC mailbox
> >
> > On Fri, Aug 30, 2019 at 1:28 AM Peng Fan wrote:
> >
> > > > > +examples:
> > > > > + - |
> > > > > +sram@91 {
> > > > > + compatible = "mmio-sram";
> > > > > + reg = <0x0 0x93f000 0x0 0x1000>;
> > > > > + #address-cells = <1>;
> > > > > + #size-cells = <1>;
> > > > > + ranges = <0 0x0 0x93f000 0x1000>;
> > > > > +
> > > > > + cpu_scp_lpri: scp-shmem@0 {
> > > > > +compatible = "arm,scmi-shmem";
> > > > > +reg = <0x0 0x200>;
> > > > > + };
> > > > > +
> > > > > + cpu_scp_hpri: scp-shmem@200 {
> > > > > +compatible = "arm,scmi-shmem";
> > > > > +reg = <0x200 0x200>;
> > > > > + };
> > > > > +};
> > > > > +
> > > > > +firmware {
> > > > > + smc_mbox: mailbox {
> > > > > +#mbox-cells = <1>;
> > > > > +compatible = "arm,smc-mbox";
> > > > > +method = "smc";
> > > > > +arm,num-chans = <0x2>;
> > > > > +transports = "mem";
> > > > > +/* Optional */
> > > > > +arm,func-ids = <0xc2fe>, <0xc2ff>;
> > > > >
> > > > SMC/HVC is synchronously(block) running in "secure mode", i.e, there
> > > > can only be one instance running platform wide. Right?
> > >
> > > I think there could be channel for TEE, and channel for Linux.
> > > For virtualization case, there could be dedicated channel for each VM.
> > >
> > I am talking from Linux pov. Functions 0xfe and 0xff above, can't both be
> > active at the same time, right?
>
> If I get your point correctly,
> On UP, both could not be active. On SMP, tx/rx could be both active, anyway
> this depends on secure firmware and Linux firmware design.
>
> Do you have any suggestions about arm,func-ids here?
>
I was thinking if this is just an instruction, why can't each channel
be represented as a controller, i.e, have exactly one func-id per
controller node. Define as many controllers as you need channels ?
-j
Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Fri, Aug 30, 2019 at 1:28 AM Peng Fan wrote:
> > > +examples:
> > > + - |
> > > +sram@91 {
> > > + compatible = "mmio-sram";
> > > + reg = <0x0 0x93f000 0x0 0x1000>;
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> > > + ranges = <0 0x0 0x93f000 0x1000>;
> > > +
> > > + cpu_scp_lpri: scp-shmem@0 {
> > > +compatible = "arm,scmi-shmem";
> > > +reg = <0x0 0x200>;
> > > + };
> > > +
> > > + cpu_scp_hpri: scp-shmem@200 {
> > > +compatible = "arm,scmi-shmem";
> > > +reg = <0x200 0x200>;
> > > + };
> > > +};
> > > +
> > > +firmware {
> > > + smc_mbox: mailbox {
> > > +#mbox-cells = <1>;
> > > +compatible = "arm,smc-mbox";
> > > +method = "smc";
> > > +arm,num-chans = <0x2>;
> > > +transports = "mem";
> > > +/* Optional */
> > > +arm,func-ids = <0xc2fe>, <0xc2ff>;
> > >
> > SMC/HVC is synchronously(block) running in "secure mode", i.e, there can
> > only be one instance running platform wide. Right?
>
> I think there could be channel for TEE, and channel for Linux.
> For virtualization case, there could be dedicated channel for each VM.
>
I am talking from Linux pov. Functions 0xfe and 0xff above, can't both
be active at the same time, right?
Re: [PATCH v5 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
On Tue, Aug 27, 2019 at 10:02 PM Peng Fan wrote:
>
> From: Peng Fan
>
> The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
> actions in software layers running in the EL2 or EL3 exception levels.
> The term "ARM" here relates to the SMC instruction as part of the ARM
> instruction set, not as a standard endorsed by ARM Ltd.
>
> Signed-off-by: Peng Fan
> ---
> .../devicetree/bindings/mailbox/arm-smc.yaml | 125
> +
> 1 file changed, 125 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
>
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> new file mode 100644
> index ..f8eb28d5e307
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM SMC Mailbox Interface
> +
> +maintainers:
> + - Peng Fan
> +
> +description: |
> + This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
> + call) instruction to trigger a mailbox-connected activity in firmware,
> + executing on the very same core as the caller. By nature this operation
> + is synchronous and this mailbox provides no way for asynchronous messages
> + to be delivered the other way round, from firmware to the OS, but
> + asynchronous notification could also be supported. However the value of
> + r0/w0/x0 the firmware returns after the smc call is delivered as a received
> + message to the mailbox framework, so a synchronous communication can be
> + established, for a asynchronous notification, no value will be returned.
> + The exact meaning of both the action the mailbox triggers as well as the
> + return value is defined by their users and is not subject to this binding.
> +
> + One use case of this mailbox is the SCMI interface, which uses shared
> memory
> + to transfer commands and parameters, and a mailbox to trigger a function
> + call. This allows SoCs without a separate management processor (or when
> + such a processor is not available or used) to use this standardized
> + interface anyway.
> +
> + This binding describes no hardware, but establishes a firmware interface.
> + Upon receiving an SMC using one of the described SMC function identifiers,
> + the firmware is expected to trigger some mailbox connected functionality.
> + The communication follows the ARM SMC calling convention.
> + Firmware expects an SMC function identifier in r0 or w0. The supported
> + identifiers are passed from consumers, or listed in the the arm,func-ids
> + properties as described below. The firmware can return one value in
> + the first SMC result register, it is expected to be an error value,
> + which shall be propagated to the mailbox client.
> +
> + Any core which supports the SMC or HVC instruction can be used, as long as
> + a firmware component running in EL3 or EL2 is handling these calls.
> +
> +properties:
> + compatible:
> +const: arm,smc-mbox
> +
> + "#mbox-cells":
> +const: 1
> +
> + arm,num-chans:
> +description: The number of channels supported.
> +items:
> + minimum: 1
> + maximum: 4096 # Should be enough?
> +
> + method:
> +- enum:
> +- smc
> +- hvc
> +
> + transports:
> +- enum:
> +- mem
> +- reg
> +
> + arm,func-ids:
> +description: |
> + An array of 32-bit values specifying the function IDs used by each
> + mailbox channel. Those function IDs follow the ARM SMC calling
> + convention standard [1].
> +
> + There is one identifier per channel and the number of supported
> + channels is determined by the length of this array.
> +$ref: /schemas/types.yaml#/definitions/uint32-array
> +minItems: 0
> +maxItems: 4096 # Should be enough?
> +
> +required:
> + - compatible
> + - "#mbox-cells"
> + - arm,num-chans
> + - transports
> + - method
> +
> +examples:
> + - |
> +sram@91 {
> + compatible = "mmio-sram";
> + reg = <0x0 0x93f000 0x0 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x0 0x93f000 0x1000>;
> +
> + cpu_scp_lpri: scp-shmem@0 {
> +compatible = "arm,scmi-shmem";
> +reg = <0x0 0x200>;
> + };
> +
> + cpu_scp_hpri: scp-shmem@200 {
> +compatible = "arm,scmi-shmem";
> +reg = <0x200 0x200>;
> + };
> +};
> +
> +firmware {
> + smc_mbox: mailbox {
> +#mbox-cells = <1>;
> +compatible = "arm,smc-mbox";
> +method = "smc";
> +arm,num-chans = <0x2>;
> +transports = "mem";
> +/* Optional */
> +arm,func-ids = <0xc2fe>, <0xc2ff>;
>
SMC/HVC is synchronously(block) running in
Re: [PATCH 2/2] dmaengine: milbeaut-hdmac: Add HDMAC driver for Milbeaut platforms
On Mon, Jun 24, 2019 at 1:47 AM Vinod Koul wrote:
>
> On 12-06-19, 19:52, jassisinghb...@gmail.com wrote:
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> Do we need both, IIRC of_dma.h does include of.h!
>
OK
> > +/* mc->vc.lock must be held by caller */
> > +static void milbeaut_chan_start(struct milbeaut_hdmac_chan *mc,
> > + struct milbeaut_hdmac_desc *md)
> > +{
> > + struct scatterlist *sg;
> > + u32 cb, ca, src_addr, dest_addr, len;
>^^
> double space
>
OK
> > +static irqreturn_t milbeaut_hdmac_interrupt(int irq, void *dev_id)
> > +{
> > + struct milbeaut_hdmac_chan *mc = dev_id;
> > + struct milbeaut_hdmac_desc *md;
> > + irqreturn_t ret = IRQ_HANDLED;
> > + u32 val;
> > +
> > + spin_lock(>vc.lock);
> > +
> > + /* Ack and Disable irqs */
> > + val = readl_relaxed(mc->reg_ch_base + MLB_HDMAC_DMACB);
> > + val &= ~(FIELD_PREP(MLB_HDMAC_SS, 0x7));
>
> Magic ..?
>
OK, will define a macro for 7
> > +static int milbeaut_hdmac_chan_pause(struct dma_chan *chan)
> > +{
> > + struct virt_dma_chan *vc = to_virt_chan(chan);
> > + struct milbeaut_hdmac_chan *mc = to_milbeaut_hdmac_chan(vc);
> > + u32 val;
> > +
> > + spin_lock(>vc.lock);
> > + val = readl_relaxed(mc->reg_ch_base + MLB_HDMAC_DMACA);
> > + val |= MLB_HDMAC_PB;
> > + writel_relaxed(val, mc->reg_ch_base + MLB_HDMAC_DMACA);
>
> We really should have an updatel() and friends in kernel, feel free to
> add in your driver though!
>
I'll pass on that for now.
> > +static int milbeaut_hdmac_chan_init(struct platform_device *pdev,
> > + struct milbeaut_hdmac_device *mdev,
> > + int chan_id)
> > +{
> > + struct device *dev = >dev;
> > + struct milbeaut_hdmac_chan *mc = >channels[chan_id];
> > + char *irq_name;
> > + int irq, ret;
> > +
> > + irq = platform_get_irq(pdev, chan_id);
> > + if (irq < 0) {
> > + dev_err(>dev, "failed to get IRQ number for ch%d\n",
> > + chan_id);
> > + return irq;
> > + }
> > +
> > + irq_name = devm_kasprintf(dev, GFP_KERNEL, "milbeaut-hdmac-%d",
> > + chan_id);
> > + if (!irq_name)
> > + return -ENOMEM;
> > +
> > + ret = devm_request_irq(dev, irq, milbeaut_hdmac_interrupt,
> > +IRQF_SHARED, irq_name, mc);
>
> I tend to dislike using devm_request_irq(), we have no control over when
> the irq is freed and what is a spirious irq is running while we are
> unrolling, so IMHO it make sense to free up and ensure all tasklets are
> quiesced when remove returns
>
If the code is written clean and tight we need not be so paranoid.
> > + if (ret)
> > + return ret;
> > +
> > + mc->mdev = mdev;
> > + mc->reg_ch_base = mdev->reg_base + MLB_HDMAC_CH_STRIDE * (chan_id +
> > 1);
> > + mc->vc.desc_free = milbeaut_hdmac_desc_free;
> > + vchan_init(>vc, >ddev);
>
> who kills the vc->task?
>
vchan_synchronize() called from milbeaut_hdmac_synchronize()
> > +static int milbeaut_hdmac_remove(struct platform_device *pdev)
> > +{
> > + struct milbeaut_hdmac_device *mdev = platform_get_drvdata(pdev);
> > + struct dma_chan *chan;
> > + int ret;
> > +
> > + /*
> > + * Before reaching here, almost all descriptors have been freed by the
> > + * ->device_free_chan_resources() hook. However, each channel might
> > + * be still holding one descriptor that was on-flight at that moment.
> > + * Terminate it to make sure this hardware is no longer running. Then,
> > + * free the channel resources once again to avoid memory leak.
> > + */
> > + list_for_each_entry(chan, >ddev.channels, device_node) {
> > + ret = dmaengine_terminate_sync(chan);
> > + if (ret)
> > + return ret;
> > + milbeaut_hdmac_free_chan_resources(chan);
> > + }
> > +
> > + of_dma_controller_free(pdev->dev.of_node);
> > + dma_async_device_unregister(>ddev);
> > + clk_disable_unprepare(mdev->clk);
>
> And as suspected we have active tasklets and irq at this time :(
>
Not sure how is that
thanks.
Re: [PATCH 1/4] mailbox: arm_mhuv2: add device tree binding documentation
On Wed, Aug 14, 2019 at 5:05 AM Sudeep Holla wrote: > > On Tue, Aug 13, 2019 at 11:36:56AM -0500, Jassi Brar wrote: > [...] > > > > >> > > > >> As mentioned in the response to your initial comment, the driver does > > > >> not currently support mixing protocols. > > > >> > > > > Thanks for acknowledging that limitation. But lets also address it. > > > > > > > > > > We are hesitant to dedicate time to developing mixing protocols given > > > that we don't have any current usecase nor any current platform which > > > would support this. > > > > > Can you please share the client code against which you tested this driver? > > From my past experience, I realise it is much more efficient to tidyup > > the code myself, than endlessly trying to explain the benefits. > > > > Thanks for the patience and offer. > Ok, but the offer is to Morten for MHUv2 driver. > Can we try the same with MHUv1 and SCMI > upstream driver. > MHUv1 driver is fine as it is. I did try my best to keep you from messing the SCMI driver, without success https://lkml.org/lkml/2017/8/7/924
Re: [PATCH 1/4] mailbox: arm_mhuv2: add device tree binding documentation
On Fri, Aug 2, 2019 at 5:41 AM Morten Borup Petersen wrote: > > > > On 7/31/19 9:31 AM, Jassi Brar wrote: > > On Sun, Jul 28, 2019 at 4:28 PM Morten Borup Petersen > > wrote: > >> > >> > >> > >> On 7/25/19 7:49 AM, Jassi Brar wrote: > >>> On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar > >>> wrote: > >>>> > >>>> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal > >>>> wrote: > >>>> > >>>>> diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt > >>>>> b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt > >>>>> new file mode 100644 > >>>>> index ..3a05593414bc > >>>>> --- /dev/null > >>>>> +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt > >>>>> @@ -0,0 +1,108 @@ > >>>>> +Arm MHUv2 Mailbox Driver > >>>>> + > >>>>> + > >>>>> +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller > >>>>> that has > >>>>> +between 1 and 124 channel windows to provide unidirectional > >>>>> communication with > >>>>> +remote processor(s). > >>>>> + > >>>>> +Given the unidirectional nature of the device, an MHUv2 mailbox may > >>>>> only be > >>>>> +written to or read from. If a pair of MHU devices is implemented > >>>>> between two > >>>>> +processing elements to provide bidirectional communication, these must > >>>>> be > >>>>> +specified as two separate mailboxes. > >>>>> + > >>>>> +A device tree node for an Arm MHUv2 device must specify either a > >>>>> receiver frame > >>>>> +or a sender frame, indicating which end of the unidirectional MHU > >>>>> device which > >>>>> +the device node entry describes. > >>>>> + > >>>>> +An MHU device must be specified with a transport protocol. The > >>>>> transport > >>>>> +protocol of an MHU device determines the method of data transmission > >>>>> as well as > >>>>> +the number of provided mailboxes. > >>>>> +Following are the possible transport protocol types: > >>>>> +- Single-word: An MHU device implements as many mailboxes as it > >>>>> + provides channel windows. Data is transmitted through > >>>>> + the MHU registers. > >>>>> +- Multi-word: An MHU device implements a single mailbox. All channel > >>>>> windows > >>>>> + will be used during transmission. Data is transmitted > >>>>> through > >>>>> + the MHU registers. > >>>>> +- Doorbell:An MHU device implements as many mailboxes as there are > >>>>> flag > >>>>> + bits available in its channel windows. Optionally, data > >>>>> may > >>>>> + be transmitted through a shared memory region, wherein > >>>>> the MHU > >>>>> + is used strictly as an interrupt generation mechanism. > >>>>> + > >>>>> +Mailbox Device Node: > >>>>> + > >>>>> + > >>>>> +Required properties: > >>>>> + > >>>>> +- compatible: Shall be "arm,mhuv2" & "arm,primecell" > >>>>> +- reg: Contains the mailbox register address range (base > >>>>> + address and length) > >>>>> +- #mbox-cells Shall be 1 - the index of the channel needed. > >>>>> +- mhu-frameFrame type of the device. > >>>>> + Shall be either "sender" or "receiver" > >>>>> +- mhu-protocol Transport protocol of the device. Shall be one of the > >>>>> + following: "single-word", "multi-word", "doorbell" > >>>>> + > >>>>> +Required properties (receiver frame): > >>>>> +- > >>>>> +- interrupts: Contains the interrupt information corr
Re: [PATCH 1/4] mailbox: arm_mhuv2: add device tree binding documentation
On Sun, Jul 28, 2019 at 4:28 PM Morten Borup Petersen wrote:
>
>
>
> On 7/25/19 7:49 AM, Jassi Brar wrote:
> > On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar wrote:
> >>
> >> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
> >> wrote:
> >>
> >>> diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> >>> b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> >>> new file mode 100644
> >>> index ..3a05593414bc
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> >>> @@ -0,0 +1,108 @@
> >>> +Arm MHUv2 Mailbox Driver
> >>> +
> >>> +
> >>> +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller
> >>> that has
> >>> +between 1 and 124 channel windows to provide unidirectional
> >>> communication with
> >>> +remote processor(s).
> >>> +
> >>> +Given the unidirectional nature of the device, an MHUv2 mailbox may only
> >>> be
> >>> +written to or read from. If a pair of MHU devices is implemented between
> >>> two
> >>> +processing elements to provide bidirectional communication, these must be
> >>> +specified as two separate mailboxes.
> >>> +
> >>> +A device tree node for an Arm MHUv2 device must specify either a
> >>> receiver frame
> >>> +or a sender frame, indicating which end of the unidirectional MHU device
> >>> which
> >>> +the device node entry describes.
> >>> +
> >>> +An MHU device must be specified with a transport protocol. The transport
> >>> +protocol of an MHU device determines the method of data transmission as
> >>> well as
> >>> +the number of provided mailboxes.
> >>> +Following are the possible transport protocol types:
> >>> +- Single-word: An MHU device implements as many mailboxes as it
> >>> + provides channel windows. Data is transmitted through
> >>> + the MHU registers.
> >>> +- Multi-word: An MHU device implements a single mailbox. All channel
> >>> windows
> >>> + will be used during transmission. Data is transmitted
> >>> through
> >>> + the MHU registers.
> >>> +- Doorbell:An MHU device implements as many mailboxes as there are
> >>> flag
> >>> + bits available in its channel windows. Optionally, data
> >>> may
> >>> + be transmitted through a shared memory region, wherein
> >>> the MHU
> >>> + is used strictly as an interrupt generation mechanism.
> >>> +
> >>> +Mailbox Device Node:
> >>> +
> >>> +
> >>> +Required properties:
> >>> +
> >>> +- compatible: Shall be "arm,mhuv2" & "arm,primecell"
> >>> +- reg: Contains the mailbox register address range (base
> >>> + address and length)
> >>> +- #mbox-cells Shall be 1 - the index of the channel needed.
> >>> +- mhu-frameFrame type of the device.
> >>> + Shall be either "sender" or "receiver"
> >>> +- mhu-protocol Transport protocol of the device. Shall be one of the
> >>> + following: "single-word", "multi-word", "doorbell"
> >>> +
> >>> +Required properties (receiver frame):
> >>> +-
> >>> +- interrupts: Contains the interrupt information corresponding to the
> >>> + combined interrupt of the receiver frame
> >>> +
> >>> +Example:
> >>> +
> >>> +
> >>> + mbox_mw_tx: mhu@1000 {
> >>> + compatible = "arm,mhuv2","arm,primecell";
> >>> + reg = <0x1000 0x1000>;
> >>> + clocks = <>;
> >>> + clock-names = "apb_pclk";
> >>> + #mbox-cells = <1>;
> >>> + mhu-protocol = "multi-word";
> >>> + mhu-frame = "sender";
> >>> + };
> >>> +
> >>> + mbox_sw_tx: mhu@1000 {
&
Re: [PATCH 1/4] mailbox: arm_mhuv2: add device tree binding documentation
On Sun, Jul 21, 2019 at 4:58 PM Jassi Brar wrote:
>
> On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
> wrote:
>
> > diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> > b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> > new file mode 100644
> > index ..3a05593414bc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> > @@ -0,0 +1,108 @@
> > +Arm MHUv2 Mailbox Driver
> > +
> > +
> > +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that
> > has
> > +between 1 and 124 channel windows to provide unidirectional communication
> > with
> > +remote processor(s).
> > +
> > +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
> > +written to or read from. If a pair of MHU devices is implemented between
> > two
> > +processing elements to provide bidirectional communication, these must be
> > +specified as two separate mailboxes.
> > +
> > +A device tree node for an Arm MHUv2 device must specify either a receiver
> > frame
> > +or a sender frame, indicating which end of the unidirectional MHU device
> > which
> > +the device node entry describes.
> > +
> > +An MHU device must be specified with a transport protocol. The transport
> > +protocol of an MHU device determines the method of data transmission as
> > well as
> > +the number of provided mailboxes.
> > +Following are the possible transport protocol types:
> > +- Single-word: An MHU device implements as many mailboxes as it
> > + provides channel windows. Data is transmitted through
> > + the MHU registers.
> > +- Multi-word: An MHU device implements a single mailbox. All channel
> > windows
> > + will be used during transmission. Data is transmitted
> > through
> > + the MHU registers.
> > +- Doorbell:An MHU device implements as many mailboxes as there are flag
> > + bits available in its channel windows. Optionally, data may
> > + be transmitted through a shared memory region, wherein the
> > MHU
> > + is used strictly as an interrupt generation mechanism.
> > +
> > +Mailbox Device Node:
> > +
> > +
> > +Required properties:
> > +
> > +- compatible: Shall be "arm,mhuv2" & "arm,primecell"
> > +- reg: Contains the mailbox register address range (base
> > + address and length)
> > +- #mbox-cells Shall be 1 - the index of the channel needed.
> > +- mhu-frameFrame type of the device.
> > + Shall be either "sender" or "receiver"
> > +- mhu-protocol Transport protocol of the device. Shall be one of the
> > + following: "single-word", "multi-word", "doorbell"
> > +
> > +Required properties (receiver frame):
> > +-
> > +- interrupts: Contains the interrupt information corresponding to the
> > + combined interrupt of the receiver frame
> > +
> > +Example:
> > +
> > +
> > + mbox_mw_tx: mhu@1000 {
> > + compatible = "arm,mhuv2","arm,primecell";
> > + reg = <0x1000 0x1000>;
> > + clocks = <>;
> > + clock-names = "apb_pclk";
> > + #mbox-cells = <1>;
> > + mhu-protocol = "multi-word";
> > + mhu-frame = "sender";
> > + };
> > +
> > + mbox_sw_tx: mhu@1000 {
> > + compatible = "arm,mhuv2","arm,primecell";
> > + reg = <0x1100 0x1000>;
> > + clocks = <>;
> > + clock-names = "apb_pclk";
> > + #mbox-cells = <1>;
> > + mhu-protocol = "single-word";
> > + mhu-frame = "sender";
> > + };
> > +
> > + mbox_db_rx: mhu@1000 {
> > + compatible = "arm,mhuv2","arm,primecell";
> > + reg = <0x1200 0x1000>;
> > + clocks = <>;
> > + clock-names = "apb_pclk";
> > + #mbox-cells = <1>;
> > + interrupts = <0 45 4>;
> &
Re: [PATCH 1/4] mailbox: arm_mhuv2: add device tree binding documentation
On Wed, Jul 17, 2019 at 2:26 PM Tushar Khandelwal
wrote:
> diff --git a/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> new file mode 100644
> index ..3a05593414bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm,mhuv2.txt
> @@ -0,0 +1,108 @@
> +Arm MHUv2 Mailbox Driver
> +
> +
> +The Arm Message-Handling-Unit (MHU) Version 2 is a mailbox controller that
> has
> +between 1 and 124 channel windows to provide unidirectional communication
> with
> +remote processor(s).
> +
> +Given the unidirectional nature of the device, an MHUv2 mailbox may only be
> +written to or read from. If a pair of MHU devices is implemented between two
> +processing elements to provide bidirectional communication, these must be
> +specified as two separate mailboxes.
> +
> +A device tree node for an Arm MHUv2 device must specify either a receiver
> frame
> +or a sender frame, indicating which end of the unidirectional MHU device
> which
> +the device node entry describes.
> +
> +An MHU device must be specified with a transport protocol. The transport
> +protocol of an MHU device determines the method of data transmission as well
> as
> +the number of provided mailboxes.
> +Following are the possible transport protocol types:
> +- Single-word: An MHU device implements as many mailboxes as it
> + provides channel windows. Data is transmitted through
> + the MHU registers.
> +- Multi-word: An MHU device implements a single mailbox. All channel windows
> + will be used during transmission. Data is transmitted through
> + the MHU registers.
> +- Doorbell:An MHU device implements as many mailboxes as there are flag
> + bits available in its channel windows. Optionally, data may
> + be transmitted through a shared memory region, wherein the MHU
> + is used strictly as an interrupt generation mechanism.
> +
> +Mailbox Device Node:
> +
> +
> +Required properties:
> +
> +- compatible: Shall be "arm,mhuv2" & "arm,primecell"
> +- reg: Contains the mailbox register address range (base
> + address and length)
> +- #mbox-cells Shall be 1 - the index of the channel needed.
> +- mhu-frameFrame type of the device.
> + Shall be either "sender" or "receiver"
> +- mhu-protocol Transport protocol of the device. Shall be one of the
> + following: "single-word", "multi-word", "doorbell"
> +
> +Required properties (receiver frame):
> +-
> +- interrupts: Contains the interrupt information corresponding to the
> + combined interrupt of the receiver frame
> +
> +Example:
> +
> +
> + mbox_mw_tx: mhu@1000 {
> + compatible = "arm,mhuv2","arm,primecell";
> + reg = <0x1000 0x1000>;
> + clocks = <>;
> + clock-names = "apb_pclk";
> + #mbox-cells = <1>;
> + mhu-protocol = "multi-word";
> + mhu-frame = "sender";
> + };
> +
> + mbox_sw_tx: mhu@1000 {
> + compatible = "arm,mhuv2","arm,primecell";
> + reg = <0x1100 0x1000>;
> + clocks = <>;
> + clock-names = "apb_pclk";
> + #mbox-cells = <1>;
> + mhu-protocol = "single-word";
> + mhu-frame = "sender";
> + };
> +
> + mbox_db_rx: mhu@1000 {
> + compatible = "arm,mhuv2","arm,primecell";
> + reg = <0x1200 0x1000>;
> + clocks = <>;
> + clock-names = "apb_pclk";
> + #mbox-cells = <1>;
> + interrupts = <0 45 4>;
> + interrupt-names = "mhu_rx";
> + mhu-protocol = "doorbell";
> + mhu-frame = "receiver";
> + };
> +
> + mhu_client: scb@2e00 {
> + compatible = "fujitsu,mb86s70-scb-1.0";
> + reg = <0 0x2e00 0x4000>;
> + mboxes =
> + // For multi-word frames, client may only instantiate a single
> + // mailbox for a mailbox controller
> + <_mw_tx 0>,
> +
> + // For single-word frames, client may instantiate as many
> + // mailboxes as there are channel windows in the MHU
> +<_sw_tx 0>,
> +<_sw_tx 1>,
> +<_sw_tx 2>,
> +<_sw_tx 3>,
> +
> + // For doorbell frames, client may instantiate as many
> mailboxes
> + // as there are bits available in the combined number of
> channel
> + // windows ((channel windows * 32) mailboxes)
> +,
> +,
> +...
> +;
> + };
If the mhuv2 instance implements, say, 3 channel
[GIT PULL] Mailbox changes for v5.3
Hi Linus, The following changes since commit 6fbc7275c7a9ba97877050335f290341a1fd8dbf: Linux 5.2-rc7 (2019-06-30 11:25:36 +0800) are available in the Git repository at: git://git.linaro.org/landing-teams/working/fujitsu/integration.git tags/mailbox-v5.3 for you to fetch changes up to 25777e5784a7b417967460d4fcf9660d05a0c320: mailbox: handle failed named mailbox channel request (2019-07-11 10:19:00 -0500) - stm32: race fix by adding a spinlock - mhu: trim included headers - omap: add support for K3 SoCs - imx: Irq disable fix - bcm: tidy up extracting driver data - tegra: make resume 'noirq' - api: fix error handling Arnaud Pouliquen (1): mailbox: stm32_ipcc: add spinlock to fix channels concurrent access Bitan Biswas (2): mailbox: tegra: hsp: add noirq resume mailbox: tegra: avoid resume NULL mailboxes Daniel Baluta (1): mailbox: imx: Clear GIEn bit at shutdown Fuqian Huang (1): mailbox: bcm-flexrm-mailbox: using dev_get_drvdata directly Sudeep Holla (1): mailbox: arm_mhu: reorder header inclusion and drop unneeded ones Suman Anna (2): dt-bindings: mailbox: omap: Update bindings for TI K3 SoCs mailbox: omap: Add support for TI K3 SoCs morten petersen (1): mailbox: handle failed named mailbox channel request .../devicetree/bindings/mailbox/omap-mailbox.txt | 59 ++ drivers/mailbox/Kconfig| 2 +- drivers/mailbox/arm_mhu.c | 11 ++-- drivers/mailbox/bcm-flexrm-mailbox.c | 6 +-- drivers/mailbox/imx-mailbox.c | 4 +- drivers/mailbox/mailbox.c | 6 ++- drivers/mailbox/omap-mailbox.c | 43 +--- drivers/mailbox/stm32-ipcc.c | 37 ++ drivers/mailbox/tegra-hsp.c| 20 ++-- include/linux/omap-mailbox.h | 4 +- 10 files changed, 134 insertions(+), 58 deletions(-)
Re: [PATCH 1/2] dt-bindings: milbeaut-m10v-hdmac: Add Socionext Milbeaut HDMAC bindings
On Tue, Jul 9, 2019 at 9:34 AM Rob Herring wrote:
>
> On Wed, Jun 12, 2019 at 07:52:37PM -0500, jassisinghb...@gmail.com wrote:
> > From: Jassi Brar
> >
> > Document the devicetree bindings for Socionext Milbeaut HDMAC
> > controller. Controller has upto 8 floating channels, that need
> > a predefined slave-id to work from a set of slaves.
> >
> > Signed-off-by: Jassi Brar
> > ---
> > .../bindings/dma/milbeaut-m10v-hdmac.txt | 54 +++
> > 1 file changed, 54 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/dma/milbeaut-m10v-hdmac.txt
> >
> > diff --git a/Documentation/devicetree/bindings/dma/milbeaut-m10v-hdmac.txt
> > b/Documentation/devicetree/bindings/dma/milbeaut-m10v-hdmac.txt
> > new file mode 100644
> > index ..a104fcb9e73d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/dma/milbeaut-m10v-hdmac.txt
> > @@ -0,0 +1,51 @@
> > +* Milbeaut AHB DMA Controller
> > +
> > +Milbeaut AHB DMA controller has transfer capability bellow.
> > + - memory to memory transfer
> > + - device to memory transfer
> > + - memory to device transfer
> > +
> > +Required property:
> > +- compatible: Should be "socionext,milbeaut-m10v-hdmac"
> > +- reg: Should contain DMA registers location and length.
> > +- interrupts: Should contain all of the per-channel DMA interrupts.
>
> How many?
>
Each channel has an IRQ line. And the number of channels is
configurable. So instead of having some explicit property like
'dma-channels', we infer that from the number of irqs registered.
> > +- #dma-cells: Should be 1. Specify the ID of the slave.
> > +- clocks: Phandle to the clock used by the HDMAC module.
> > +
> > +
> > +Example:
> > +
> > + hdmac1: hdmac@1e11 {
>
> dma-controller@...
>
OK
> > + compatible = "socionext,milbeaut-m10v-hdmac";
> > + reg = <0x1e11 0x1>;
> > + interrupts = <0 132 4>,
> > + <0 133 4>,
> > + <0 134 4>,
> > + <0 135 4>,
> > + <0 136 4>,
> > + <0 137 4>,
> > + <0 138 4>,
> > + <0 139 4>;
> > + #dma-cells = <1>;
> > + clocks = <_clk>;
> > + };
> > +
> > +* DMA client
> > +
> > +Clients have to specify the DMA requests with phandles in a list.
>
> Nothing specific to this binding here and the client side is already
> documented, so drop this section.
>
OK.
Thanks
Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
On Thu, Jun 27, 2019 at 4:09 AM Sudeep Holla wrote:
>
> On Wed, Jun 26, 2019 at 01:27:41PM -0500, Jassi Brar wrote:
> > On Wed, Jun 26, 2019 at 11:44 AM Florian Fainelli
> > wrote:
> > >
> > > On 6/26/19 6:31 AM, Peng Fan wrote:
> > > >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> > > >>> So add an optional func-id to let smc mailbox driver could
> > > >>> use smc SiP func id.
> > > >>>
> > > >> There is no end to conforming to protocols. Controller drivers should
> > > >> be written having no particular client in mind.
> > > >
> > > > If the func-id needs be passed from user, then the chan_id suggested
> > > > by Sudeep should also be passed from user, not in mailbox driver.
> > > >
> > > > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > > > to firmware, right?
> > > >
> > > > Sudeep, Andre, Florian,
> > > >
> > > > What's your suggestion? SCMI not support, do you have
> > > > plan to add smc transport in SCMI?
> > >
> > > On the platforms that I work with, we have taken the liberty of
> > > implementing SCMI in our monitor firmware because the other MCU we use
> > > for dynamic voltage and frequency scaling did not have enough memory to
> > > support that and we still had the ability to make that firmware be
> > > trusted enough we could give it power management responsibilities. I
> > > would certainly feel more comfortable if the SCMI specification was
> > > amended to indicate that the Agent could be such a software entity,
> > > still residing on the same host CPU as the Platform(s), but if not,
> > > that's fine.
> > >
> > > This has lead us to implement a mailbox driver that uses a proprietary
> > > SMC call for the P2A path ("tx" channel) and the return being done via
> > > either that same SMC or through SGI. You can take a look at it in our
> > > downstream tree here actually:
> > >
> > > https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
> > >
> > > If we can get rid of our own driver and uses a standard SMC based
> > > mailbox driver that supports our use case that involves interrupts (we
> > > can always change their kind without our firmware/boot loader since FDT
> > > is generated from that component), that would be great.
> > >
> > static irqreturn_t brcm_isr(void)
> > {
> > mbox_chan_received_data([0], NULL);
> > return IRQ_HANDLED;
> > }
> >
> > Sorry, I fail to understand why the irq can't be moved inside the
> > client driver itself? There can't be more cost to it and there
> > definitely is no functionality lost.
>
> What if there are multiple clients ?
>
There is a flag IRQF_SHARED for such situations.
(good to see you considering multiple clients per channel as a legit scenario)
> And I assume you are referring to case like this where IRQ is not tied
> to the mailbox IP.
>
Yes, and that is the reason the irq should not be managed by the mailbox driver.
Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
On Wed, Jun 26, 2019 at 11:44 AM Florian Fainelli wrote:
>
> On 6/26/19 6:31 AM, Peng Fan wrote:
> >>> The firmware driver might not have func-id, such as SCMI/SCPI.
> >>> So add an optional func-id to let smc mailbox driver could
> >>> use smc SiP func id.
> >>>
> >> There is no end to conforming to protocols. Controller drivers should
> >> be written having no particular client in mind.
> >
> > If the func-id needs be passed from user, then the chan_id suggested
> > by Sudeep should also be passed from user, not in mailbox driver.
> >
> > Jassi, so from your point, arm_smc_send_data just send a0 - a6
> > to firmware, right?
> >
> > Sudeep, Andre, Florian,
> >
> > What's your suggestion? SCMI not support, do you have
> > plan to add smc transport in SCMI?
>
> On the platforms that I work with, we have taken the liberty of
> implementing SCMI in our monitor firmware because the other MCU we use
> for dynamic voltage and frequency scaling did not have enough memory to
> support that and we still had the ability to make that firmware be
> trusted enough we could give it power management responsibilities. I
> would certainly feel more comfortable if the SCMI specification was
> amended to indicate that the Agent could be such a software entity,
> still residing on the same host CPU as the Platform(s), but if not,
> that's fine.
>
> This has lead us to implement a mailbox driver that uses a proprietary
> SMC call for the P2A path ("tx" channel) and the return being done via
> either that same SMC or through SGI. You can take a look at it in our
> downstream tree here actually:
>
> https://github.com/Broadcom/stblinux-4.9/blob/master/linux/drivers/mailbox/brcmstb-mailbox.c
>
> If we can get rid of our own driver and uses a standard SMC based
> mailbox driver that supports our use case that involves interrupts (we
> can always change their kind without our firmware/boot loader since FDT
> is generated from that component), that would be great.
>
static irqreturn_t brcm_isr(void)
{
mbox_chan_received_data([0], NULL);
return IRQ_HANDLED;
}
Sorry, I fail to understand why the irq can't be moved inside the
client driver itself? There can't be more cost to it and there
definitely is no functionality lost.
Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
On Wed, Jun 26, 2019 at 8:31 AM Peng Fan wrote:
>
>
> Hi All,
>
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> >
> > On Tue, Jun 25, 2019 at 2:30 AM Peng Fan wrote:
> > >
> > > Hi Jassi
> > >
> > > > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> > > >
> > > > On Mon, Jun 3, 2019 at 3:28 AM wrote:
> > > > >
> > > > > From: Peng Fan
> > > > >
> > > > > This mailbox driver implements a mailbox which signals transmitted
> > > > > data via an ARM smc (secure monitor call) instruction. The mailbox
> > > > > receiver is implemented in firmware and can synchronously return
> > > > > data when it returns execution to the non-secure world again.
> > > > > An asynchronous receive path is not implemented.
> > > > > This allows the usage of a mailbox to trigger firmware actions on
> > > > > SoCs which either don't have a separate management processor or on
> > > > > which such a core is not available. A user of this mailbox could
> > > > > be the SCP interface.
> > > > >
> > > > > Modified from Andre Przywara's v2 patch https://lore
> > > > > .kernel.org%2Fpatchwork%2Fpatch%2F812999%2Fdata=02%7C0
> > 1%7
> > > > Cpeng.fa
> > > > >
> > > >
> > n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> > > > c6fa92cd
> > > > >
> > > >
> > 99c5c301635%7C0%7C0%7C636966462272457978sdata=Hzgeu43m5
> > > > ZkeRMtL8Bx
> > > > > gUm3%2B6FBObib1OPHPlSccE%2B0%3Dreserved=0
> > > > >
> > > > > Cc: Andre Przywara
> > > > > Signed-off-by: Peng Fan
> > > > > ---
> > > > >
> > > > > V2:
> > > > > Add interrupts notification support.
> > > > >
> > > > > drivers/mailbox/Kconfig | 7 ++
> > > > > drivers/mailbox/Makefile| 2 +
> > > > > drivers/mailbox/arm-smc-mailbox.c | 190
> > > >
> > > > > include/linux/mailbox/arm-smc-mailbox.h | 10 ++
> > > > > 4 files changed, 209 insertions(+)
> > > > > create mode 100644 drivers/mailbox/arm-smc-mailbox.c create
> > mode
> > > > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > > > >
> > > > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > > > 595542bfae85..c3bd0f1ddcd8 100644
> > > > > --- a/drivers/mailbox/Kconfig
> > > > > +++ b/drivers/mailbox/Kconfig
> > > > > @@ -15,6 +15,13 @@ config ARM_MHU
> > > > > The controller has 3 mailbox channels, the last of which can
> > be
> > > > > used in Secure mode only.
> > > > >
> > > > > +config ARM_SMC_MBOX
> > > > > + tristate "Generic ARM smc mailbox"
> > > > > + depends on OF && HAVE_ARM_SMCCC
> > > > > + help
> > > > > + Generic mailbox driver which uses ARM smc calls to call into
> > > > > + firmware for triggering mailboxes.
> > > > > +
> > > > > config IMX_MBOX
> > > > > tristate "i.MX Mailbox"
> > > > > depends on ARCH_MXC || COMPILE_TEST diff --git
> > > > > a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > > > > c22fad6f696b..93918a84c91b 100644
> > > > > --- a/drivers/mailbox/Makefile
> > > > > +++ b/drivers/mailbox/Makefile
> > > > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) +=
> > mailbox-test.o
> > > > >
> > > > > obj-$(CONFIG_ARM_MHU) += arm_mhu.o
> > > > >
> > > > > +obj-$(CONFIG_ARM_SMC_MBOX) += arm-smc-mailbox.o
> > > > > +
> > > > > obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> > > > >
> > > > > obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)+=
> > > > armada-37xx-rwtm-mailbox.o
> > > > > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > > > > b/drivers/mailbox/arm-smc-mailbox.c
> > > > > new file mode 100644
> > > > > index ..fef6e38d8b98
> > > > > --- /dev/null
> > > > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > > > @@ -0,0 +1,190 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (C) 2016,2017 ARM Ltd.
> > > > > + * Copyright 2019 NXP
> > > > > + */
> > > > > +
> > > > > +#include
> > > > > +#include
> > > > > +#include
> > > > > +#include
> > > > > +#include #include
> > > > > +
> > > > > +#include
> > > > > +#include
> > > > > +
> > > > > +#define ARM_SMC_MBOX_USE_HVC BIT(0)
> > > > > +#define ARM_SMC_MBOX_USB_IRQ BIT(1)
> > > > > +
> > > > IRQ bit is unused (and unnecessary IMO)
> > > >
> > > > > +struct arm_smc_chan_data {
> > > > > + u32 function_id;
> > > > > + u32 flags;
> > > > > + int irq;
> > > > > +};
> > > > > +
> > > > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > > > + struct arm_smc_chan_data *chan_data = link->con_priv;
> > > > > + struct arm_smccc_mbox_cmd *cmd = data;
> > > > > + struct arm_smccc_res res;
> > > > > + u32 function_id;
> > > > > +
> > > > > + if (chan_data->function_id != UINT_MAX)
> > > > > + function_id = chan_data->function_id;
> > > > > + else
> > > > > + function_id = cmd->a0;
> > > > > +
> > > > Not sure about chan_data->function_id. Why restrict from DT?
> >
Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
On Tue, Jun 25, 2019 at 2:30 AM Peng Fan wrote:
>
> Hi Jassi
>
> > Subject: Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
> >
> > On Mon, Jun 3, 2019 at 3:28 AM wrote:
> > >
> > > From: Peng Fan
> > >
> > > This mailbox driver implements a mailbox which signals transmitted
> > > data via an ARM smc (secure monitor call) instruction. The mailbox
> > > receiver is implemented in firmware and can synchronously return data
> > > when it returns execution to the non-secure world again.
> > > An asynchronous receive path is not implemented.
> > > This allows the usage of a mailbox to trigger firmware actions on SoCs
> > > which either don't have a separate management processor or on which
> > > such a core is not available. A user of this mailbox could be the SCP
> > > interface.
> > >
> > > Modified from Andre Przywara's v2 patch
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > > .kernel.org%2Fpatchwork%2Fpatch%2F812999%2Fdata=02%7C01%7
> > Cpeng.fa
> > >
> > n%40nxp.com%7C1237677cb01044ad714508d6f59f648f%7C686ea1d3bc2b4
> > c6fa92cd
> > >
> > 99c5c301635%7C0%7C0%7C636966462272457978sdata=Hzgeu43m5
> > ZkeRMtL8Bx
> > > gUm3%2B6FBObib1OPHPlSccE%2B0%3Dreserved=0
> > >
> > > Cc: Andre Przywara
> > > Signed-off-by: Peng Fan
> > > ---
> > >
> > > V2:
> > > Add interrupts notification support.
> > >
> > > drivers/mailbox/Kconfig | 7 ++
> > > drivers/mailbox/Makefile| 2 +
> > > drivers/mailbox/arm-smc-mailbox.c | 190
> >
> > > include/linux/mailbox/arm-smc-mailbox.h | 10 ++
> > > 4 files changed, 209 insertions(+)
> > > create mode 100644 drivers/mailbox/arm-smc-mailbox.c create mode
> > > 100644 include/linux/mailbox/arm-smc-mailbox.h
> > >
> > > diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index
> > > 595542bfae85..c3bd0f1ddcd8 100644
> > > --- a/drivers/mailbox/Kconfig
> > > +++ b/drivers/mailbox/Kconfig
> > > @@ -15,6 +15,13 @@ config ARM_MHU
> > > The controller has 3 mailbox channels, the last of which can be
> > > used in Secure mode only.
> > >
> > > +config ARM_SMC_MBOX
> > > + tristate "Generic ARM smc mailbox"
> > > + depends on OF && HAVE_ARM_SMCCC
> > > + help
> > > + Generic mailbox driver which uses ARM smc calls to call into
> > > + firmware for triggering mailboxes.
> > > +
> > > config IMX_MBOX
> > > tristate "i.MX Mailbox"
> > > depends on ARCH_MXC || COMPILE_TEST diff --git
> > > a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index
> > > c22fad6f696b..93918a84c91b 100644
> > > --- a/drivers/mailbox/Makefile
> > > +++ b/drivers/mailbox/Makefile
> > > @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o
> > >
> > > obj-$(CONFIG_ARM_MHU) += arm_mhu.o
> > >
> > > +obj-$(CONFIG_ARM_SMC_MBOX) += arm-smc-mailbox.o
> > > +
> > > obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
> > >
> > > obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)+=
> > armada-37xx-rwtm-mailbox.o
> > > diff --git a/drivers/mailbox/arm-smc-mailbox.c
> > > b/drivers/mailbox/arm-smc-mailbox.c
> > > new file mode 100644
> > > index ..fef6e38d8b98
> > > --- /dev/null
> > > +++ b/drivers/mailbox/arm-smc-mailbox.c
> > > @@ -0,0 +1,190 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2016,2017 ARM Ltd.
> > > + * Copyright 2019 NXP
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include #include
> > > +
> > > +#include
> > > +#include
> > > +
> > > +#define ARM_SMC_MBOX_USE_HVC BIT(0)
> > > +#define ARM_SMC_MBOX_USB_IRQ BIT(1)
> > > +
> > IRQ bit is unused (and unnecessary IMO)
> >
> > > +struct arm_smc_chan_data {
> > > + u32 function_id;
> > > + u32 flags;
> > > + int irq;
> > > +};
> > > +
> > > +static int arm_smc_send_data(struct mbox_chan *link, void *data) {
> > > + struct arm_smc_chan_data *chan_data = link->con_priv;
> > > + struct arm_smccc_mbox_cmd *cmd = data;
> > > + struct arm_smccc_res res;
> > > + u32 function_id;
> > > +
> > > + if (chan_data->function_id != UINT_MAX)
> > > + function_id = chan_data->function_id;
> > > + else
> > > + function_id = cmd->a0;
> > > +
> > Not sure about chan_data->function_id. Why restrict from DT?
> > 'a0' is the function_id register, let the user pass func-id via the 'a0'
> > like other
> > values via 'a[1-7]'
>
> Missed to reply this comment.
>
> The firmware driver might not have func-id, such as SCMI/SCPI.
> So add an optional func-id to let smc mailbox driver could
> use smc SiP func id.
>
There is no end to conforming to protocols. Controller drivers should
be written having no particular client in mind.
Re: [PATCH 0/2] Add Mailbox support for TI K3 SoCs
On Mon, Jun 24, 2019 at 3:39 PM Suman Anna wrote: > > Hi Jassi, > > On 6/4/19 12:01 PM, Suman Anna wrote: > > Hi Jassi, > > > > The following series adds the support for the Mailbox IP present > > within the Main NavSS module on the newer TI K3 AM65x and J721E SoCs. > > > > The Mailbox IP is similar to the previous generation IP on OMAP SoCs > > with a few differences: > > - Multiple IP instances from previous DRA7/AM57 family each form a > >cluster and are part of the same IP. The driver support will continue > >to be based on a cluster. > > - The IP is present within a Main NaVSS, and interrupts have to go > >through an Interrupt Router within Main NavSS before they reach the > >respective processor sub-system's interrupt controllers. > > - The register layout is mostly same, with difference in two registers > > > > Support is added by enhancing the existing OMAP Mailbox driver to > > support the K3 IP using a new compatible. The driver also has to be > > adjusted to deal with the 32-bit mailbox payloads vs the 64-bit > > pointers used by the Mailbox API on these Arm v8 platforms. > > > > DT nodes will be posted separately once the binding is acked. > > Can you please pick this series up for 5.3 merge window if you do not > have any comments. > I will. Usually I leave the code to cook in the open for as long as possible, more so when no acks are coming. Cheers!
Re: [PATCH V2 2/2] mailbox: introduce ARM SMC based mailbox
On Mon, Jun 3, 2019 at 3:28 AM wrote:
>
> From: Peng Fan
>
> This mailbox driver implements a mailbox which signals transmitted data
> via an ARM smc (secure monitor call) instruction. The mailbox receiver
> is implemented in firmware and can synchronously return data when it
> returns execution to the non-secure world again.
> An asynchronous receive path is not implemented.
> This allows the usage of a mailbox to trigger firmware actions on SoCs
> which either don't have a separate management processor or on which such
> a core is not available. A user of this mailbox could be the SCP
> interface.
>
> Modified from Andre Przywara's v2 patch
> https://lore.kernel.org/patchwork/patch/812999/
>
> Cc: Andre Przywara
> Signed-off-by: Peng Fan
> ---
>
> V2:
> Add interrupts notification support.
>
> drivers/mailbox/Kconfig | 7 ++
> drivers/mailbox/Makefile| 2 +
> drivers/mailbox/arm-smc-mailbox.c | 190
>
> include/linux/mailbox/arm-smc-mailbox.h | 10 ++
> 4 files changed, 209 insertions(+)
> create mode 100644 drivers/mailbox/arm-smc-mailbox.c
> create mode 100644 include/linux/mailbox/arm-smc-mailbox.h
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index 595542bfae85..c3bd0f1ddcd8 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -15,6 +15,13 @@ config ARM_MHU
> The controller has 3 mailbox channels, the last of which can be
> used in Secure mode only.
>
> +config ARM_SMC_MBOX
> + tristate "Generic ARM smc mailbox"
> + depends on OF && HAVE_ARM_SMCCC
> + help
> + Generic mailbox driver which uses ARM smc calls to call into
> + firmware for triggering mailboxes.
> +
> config IMX_MBOX
> tristate "i.MX Mailbox"
> depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index c22fad6f696b..93918a84c91b 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o
>
> obj-$(CONFIG_ARM_MHU) += arm_mhu.o
>
> +obj-$(CONFIG_ARM_SMC_MBOX) += arm-smc-mailbox.o
> +
> obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
>
> obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX)+= armada-37xx-rwtm-mailbox.o
> diff --git a/drivers/mailbox/arm-smc-mailbox.c
> b/drivers/mailbox/arm-smc-mailbox.c
> new file mode 100644
> index ..fef6e38d8b98
> --- /dev/null
> +++ b/drivers/mailbox/arm-smc-mailbox.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2016,2017 ARM Ltd.
> + * Copyright 2019 NXP
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define ARM_SMC_MBOX_USE_HVC BIT(0)
> +#define ARM_SMC_MBOX_USB_IRQ BIT(1)
> +
IRQ bit is unused (and unnecessary IMO)
> +struct arm_smc_chan_data {
> + u32 function_id;
> + u32 flags;
> + int irq;
> +};
> +
> +static int arm_smc_send_data(struct mbox_chan *link, void *data)
> +{
> + struct arm_smc_chan_data *chan_data = link->con_priv;
> + struct arm_smccc_mbox_cmd *cmd = data;
> + struct arm_smccc_res res;
> + u32 function_id;
> +
> + if (chan_data->function_id != UINT_MAX)
> + function_id = chan_data->function_id;
> + else
> + function_id = cmd->a0;
> +
Not sure about chan_data->function_id. Why restrict from DT?
'a0' is the function_id register, let the user pass func-id via the
'a0' like other values via 'a[1-7]'
> + if (chan_data->flags & ARM_SMC_MBOX_USE_HVC)
> + arm_smccc_hvc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> + cmd->a5, cmd->a6, cmd->a7, );
> + else
> + arm_smccc_smc(function_id, cmd->a1, cmd->a2, cmd->a3, cmd->a4,
> + cmd->a5, cmd->a6, cmd->a7, );
> +
> + if (chan_data->irq)
> + return 0;
> +
This irq thing seems like oob signalling, that is, a protocol thing.
And then it provides lesser info via chan_irq_handler (returns NULL)
than res.a0 - which can always be ignored if not needed.
So the irq should be implemented in the upper layer if the protocol needs it.
> + mbox_chan_received_data(link, (void *)res.a0);
> +
This is fine.
Re: [PATCH V2 1/2] DT: mailbox: add binding doc for the ARM SMC mailbox
On Thu, Jun 20, 2019 at 11:13 AM Andre Przywara wrote: > > On Thu, 20 Jun 2019 10:22:41 +0100 > Sudeep Holla wrote: > > > On Mon, Jun 03, 2019 at 04:30:04PM +0800, peng@nxp.com wrote: > > > From: Peng Fan > > > > > > The ARM SMC mailbox binding describes a firmware interface to trigger > > > actions in software layers running in the EL2 or EL3 exception levels. > > > The term "ARM" here relates to the SMC instruction as part of the ARM > > > instruction set, not as a standard endorsed by ARM Ltd. > > > > > > Signed-off-by: Peng Fan > > > --- > > > > > > V2: > > > Introduce interrupts as a property. > > > > > > V1: > > > arm,func-ids is still kept as an optional property, because there is no > > > defined SMC funciton id passed from SCMI. So in my test, I still use > > > arm,func-ids for ARM SIP service. > > > > > > .../devicetree/bindings/mailbox/arm-smc.txt| 101 > > > + > > > 1 file changed, 101 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.txt > > > > > > diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.txt > > > b/Documentation/devicetree/bindings/mailbox/arm-smc.txt > > > new file mode 100644 > > > index ..401887118c09 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.txt > > > @@ -0,0 +1,101 @@ > > > +ARM SMC Mailbox Interface > > > += > > > + > > > +This mailbox uses the ARM smc (secure monitor call) instruction to > > > trigger > > > +a mailbox-connected activity in firmware, executing on the very same core > > > +as the caller. By nature this operation is synchronous and this mailbox > > > +provides no way for asynchronous messages to be delivered the other way > > > +round, from firmware to the OS, but asynchronous notification could also > > > +be supported. However the value of r0/w0/x0 the firmware returns after > > > +the smc call is delivered as a received message to the mailbox framework, > > > +so a synchronous communication can be established, for a asynchronous > > > +notification, no value will be returned. The exact meaning of both the > > > +action the mailbox triggers as well as the return value is defined by > > > +their users and is not subject to this binding. > > > + > > > +One use case of this mailbox is the SCMI interface, which uses shared > > > memory > > > +to transfer commands and parameters, and a mailbox to trigger a function > > > +call. This allows SoCs without a separate management processor (or when > > > +such a processor is not available or used) to use this standardized > > > +interface anyway. > > > + > > > +This binding describes no hardware, but establishes a firmware interface. > > > +Upon receiving an SMC using one of the described SMC function > > > identifiers, > > > +the firmware is expected to trigger some mailbox connected functionality. > > > +The communication follows the ARM SMC calling convention[1]. > > > +Firmware expects an SMC function identifier in r0 or w0. The supported > > > +identifiers are passed from consumers, or listed in the the arm,func-ids > > > +properties as described below. The firmware can return one value in > > > +the first SMC result register, it is expected to be an error value, > > > +which shall be propagated to the mailbox client. > > > + > > > +Any core which supports the SMC or HVC instruction can be used, as long > > > as > > > +a firmware component running in EL3 or EL2 is handling these calls. > > > + > > > +Mailbox Device Node: > > > + > > > + > > > +This node is expected to be a child of the /firmware node. > > > + > > > +Required properties: > > > + > > > +- compatible: Shall be "arm,smc-mbox" > > > +- #mbox-cells Shall be 1 - the index of the channel needed. > > > +- arm,num-chansThe number of channels supported. > > > +- method: A string, either: > > > + "hvc": if the driver shall use an HVC call, or > > > + "smc": if the driver shall use an SMC call. > > > + > > > +Optional properties: > > > +- arm,func-ids An array of 32-bit values specifying the > > > function > > > + IDs used by each mailbox channel. Those function IDs > > > + follow the ARM SMC calling convention standard [1]. > > > + There is one identifier per channel and the number > > > + of supported channels is determined by the length > > > + of this array. > > > +- interrupts SPI interrupts may be listed for notification, > > > + each channel should use a dedicated interrupt > > > + line. > > > + > > > > I think SMC mailbox as mostly unidirectional/Tx only channel. And the > > interrupts here as stated are for notifications, so I prefer to keep > > them separate channel. I assume SMC call return indicates
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
On Thu, Jun 6, 2019 at 7:51 AM Sudeep Holla wrote: > > > BTW, this is not going to be the end of SCMI troubles (I believe > > that's what his client is). SCMI will eventually have to be broken up > > in layers (protocol and transport) for many legit platforms to use it. > > That is mbox_send_message() will have to be replaced by, say, > > platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the > > platforms have to have shmem and each mailbox controller driver (that > > could ever be used under scmi) will have to implement "doorbell > > emulation" mode. That is the reason I am not letting the way paved for > > such emulations. > > > > While I don't dislike or disagree with separate transport in SCMI which > I have invested time and realised that I will duplicate mailbox framework > at the end. > Can you please share the code? Or is it no more available? > So I am against it only because of duplication and extra > layer of indirection which has performance impact(we have this seen in > sched governor for DVFS). > I don't see why the overhead should increase noticeably. > So idea wise, it's good and I don't disagree > with practically seen performance impact. Hence I thought it's sane to > do something I am proposing. > Please suggest how is SCMI supposed to work on ~15 controllers upstream (except tegra-hsp) ? > It also avoids coming up with virtual DT > nodes for this layer of abstract which I am completely against. > I don't see why virtual DT nodes would be needed for platform layer.
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
On Wed, Jun 5, 2019 at 2:46 PM Mark Brown wrote: > > On Tue, Jun 04, 2019 at 10:44:24AM +0100, Sudeep Holla wrote: > > On Mon, Jun 03, 2019 at 08:39:46PM +0100, Mark Brown wrote: > > > > > > It feels like the issues with sharing access to the hardware and with the > > > API for talking to doorbell hardware are getting tied together and > > > confusing things. But like I say I might be missing something here. > > ... > > > So what I am trying to convey here is MHU controller hardware can be > > used choosing one of the different transport protocols available and > > that's platform choice based on the use-case. > > > The driver in the kernel should identify the same from the firmware/DT > > and configure it appropriately. > > > It may get inefficient and sometime impossible to address all use-case > > if we stick to one transport protocol in the driver and try to build > > an abstraction on top to use in different transport mode. > > Right, what I was trying to get at was that it feels like the discussion > is getting wrapped up in the specifics of the MHU rather than > representing this sort of controller with multiple modes in the > framework. > Usually when a controller could be used in more than one way, we implement the more generic usecase. And that's what was done for MHU. Implementing doorbell scheme would have disallowed mhu platforms that don't have any shmem between the endpoints. Now such platforms could use 32bits registers to pass/get data. Meanwhile doorbells could be emulated in client code. Also, next version of MHU has many (100?) such 32bit registers per interrupt. Clearly those are not meant to be seen as 3200 doorbells, but as message passing windows. (or maybe that is an almost different controller because of the differences) BTW, this is not going to be the end of SCMI troubles (I believe that's what his client is). SCMI will eventually have to be broken up in layers (protocol and transport) for many legit platforms to use it. That is mbox_send_message() will have to be replaced by, say, platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the platforms have to have shmem and each mailbox controller driver (that could ever be used under scmi) will have to implement "doorbell emulation" mode. That is the reason I am not letting the way paved for such emulations. Thanks
Re: [PATCH 1/6] mailbox: add support for doorbell/signal mode controllers
On Fri, May 31, 2019 at 9:33 AM Sudeep Holla wrote:
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index 38d9df3fb199..e26a079f8223 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -77,7 +77,10 @@ static void msg_submit(struct mbox_chan *chan)
> if (chan->cl->tx_prepare)
> chan->cl->tx_prepare(chan->cl, data);
> /* Try to submit a message to the MBOX controller */
> - err = chan->mbox->ops->send_data(chan, data);
> + if (chan->mbox->ops->send_data)
> + err = chan->mbox->ops->send_data(chan, data);
> + else
> + err = chan->mbox->ops->send_signal(chan);
> if (!err) {
> chan->active_req = data;
> chan->msg_count--;
>
The 'void *data' parameter in send_data() is controller specific.
The doorbell controllers should simply ignore that.
So signal/doorbell controllers are already supported fine. See
drivers/mailbox/tegra-hsp.c for example.
Thanks.
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
Hi Sudeep, On Fri, May 31, 2019 at 9:33 AM Sudeep Holla wrote: > > This is my another attempt to extend mailbox framework to support > doorbell mode mailbox hardware. It also adds doorbell support to ARM > MHU driver. > Nothing has really changed since the last time we discussed many months ago. MHU remains same, and so are my points. > > Brief hardware description about MHU > > > For each of the interrupt signals, the MHU drives the signal using a 32-bit > register, with all 32 bits logically ORed together. The MHU provides a set of > registers to enable software to SET, CLEAR, and check the STATUS of each of > the bits of this register independently. The use of 32 bits for each interrupt > line enables software to provide more information about the source of the > interrupt. > "32 bits for each interrupt line" => 32 virtual channels or 32bit data over one physical channel. And that is how the driver is currently written. > For example, each bit of the register can be associated with a type > of event that can contribute to raising the interrupt. > Sure there can be a usecase where each bit is associated with an event (virtual channel). As you said, this is just one example of how MHU can be used. There are other ways MHU is used already. Patch-2/6 looks good though. I will pick that up. Thanks.
Re: [PATCH 0/2] mailbox: arm: introduce smc triggered mailbox
On Thu, May 23, 2019 at 12:50 AM Peng Fan wrote: > > This is a modified version from Andre Przywara's patch series > https://lore.kernel.org/patchwork/cover/812997/. > Can you please specify exact modifications on top of Andre's last submission? As in "Changes since v1: " Thanks.
[GIT PULL] Mailbox changes for v5.2
Hi Linus, The following changes since commit 37624b58542fb9f2d9a70e6ea006ef8a5f66c30b: Linux 5.1-rc7 (2019-04-28 17:04:13 -0700) are available in the Git repository at: git://git.linaro.org/landing-teams/working/fujitsu/integration.git tags/mailbox-v5.2 for you to fetch changes up to 8fbbfd966efa67ef9aec37cb4ff412f9f26e1e84: mailbox: Add support for Armada 37xx rWTM mailbox (2019-05-09 00:41:00 -0500) - New driver: Armada 37xx mailbox controller - Misc: Use devm_ api for imx and platform_get_irq for stm32 Anson Huang (1): mailbox: imx: use devm_platform_ioremap_resource() to simplify code Fabien Dessenne (1): mailbox: stm32-ipcc: check invalid irq Marek Behun (2): dt-bindings: mailbox: Document armada-3700-rwtm-mailbox binding mailbox: Add support for Armada 37xx rWTM mailbox .../mailbox/marvell,armada-3700-rwtm-mailbox.txt | 16 ++ drivers/mailbox/Kconfig| 10 + drivers/mailbox/Makefile | 2 + drivers/mailbox/armada-37xx-rwtm-mailbox.c | 225 + drivers/mailbox/imx-mailbox.c | 4 +- drivers/mailbox/stm32-ipcc.c | 13 +- include/linux/armada-37xx-rwtm-mailbox.h | 23 +++ 7 files changed, 285 insertions(+), 8 deletions(-) create mode 100644 Documentation/devicetree/bindings/mailbox/marvell,armada-3700-rwtm-mailbox.txt create mode 100644 drivers/mailbox/armada-37xx-rwtm-mailbox.c create mode 100644 include/linux/armada-37xx-rwtm-mailbox.h
[GIT PULL] Mailbox changes for v5.1
Hi Linus, The following changes since commit 3717f613f48df0222311f974cf8a06c8a6c97bae: Merge branch 'core-rcu-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip (2019-03-05 14:49:11 -0800) are available in the Git repository at: git://git.linaro.org/landing-teams/working/fujitsu/integration.git tags/mailbox-v5.1 for you to fetch changes up to 17b860bbfc844a3d8e38135ef430d4af8e436b9e: mailbox: imx: keep MU irq working during suspend/resume (2019-03-11 02:51:43 -0500) - mailbox-test: support multiple controller instances - misc cleanup: IMX, STM32 and Tegra - new driver: ZynqMP IPI Anson Huang (1): mailbox: imx: keep MU irq working during suspend/resume Arnd Bergmann (1): mailbox: tegra-hsp: mark suspend function as __maybe_unused Fabien Dessenne (4): mailbox: mailbox-test: fix debugfs in multi-instances mailbox: mailbox-test: fix null pointer if no mmio mailbox: stm32-ipcc: do not enable wakeup source by default mailbox: stm32-ipcc: remove useless device_init_wakeup call Wendy Liang (2): mailbox: ZynqMP IPI mailbox controller dt-bindings: mailbox: Add Xilinx IPI Mailbox .../bindings/mailbox/xlnx,zynqmp-ipi-mailbox.txt | 127 drivers/mailbox/Kconfig| 11 + drivers/mailbox/Makefile | 2 + drivers/mailbox/imx-mailbox.c | 4 +- drivers/mailbox/mailbox-test.c | 26 +- drivers/mailbox/stm32-ipcc.c | 4 +- drivers/mailbox/tegra-hsp.c| 2 +- drivers/mailbox/zynqmp-ipi-mailbox.c | 725 + include/linux/mailbox/zynqmp-ipi-message.h | 20 + 9 files changed, 903 insertions(+), 18 deletions(-) create mode 100644 Documentation/devicetree/bindings/mailbox/xlnx,zynqmp-ipi-mailbox.txt create mode 100644 drivers/mailbox/zynqmp-ipi-mailbox.c create mode 100644 include/linux/mailbox/zynqmp-ipi-message.h
[GIT PULL] Mailbox fixes for v5.0
Hi Linus, The following changes since commit a3b22b9f11d9fbc48b0291ea92259a5a810e9438: Linux 5.0-rc7 (2019-02-17 18:46:40 -0800) are available in the Git repository at: git://git.linaro.org/landing-teams/working/fujitsu/integration.git tags/mailbox-fixes-v5.0-rc7 for you to fetch changes up to d7bf31a0f85faaf63c63c39d55154825a1eaaea9: mailbox: bcm-flexrm-mailbox: Fix FlexRM ring flush timeout issue (2019-02-18 10:40:58 -0600) - API: Fix build breakge by exporting the function mbox_flush - BRCM: Fix FlexRM ring flush timeout issue Rayagonda Kokatanur (1): mailbox: bcm-flexrm-mailbox: Fix FlexRM ring flush timeout issue Thierry Reding (1): mailbox: Export mbox_flush() drivers/mailbox/bcm-flexrm-mailbox.c | 4 ++-- drivers/mailbox/mailbox.c| 1 + 2 files changed, 3 insertions(+), 2 deletions(-)
Re: [PATCH 1/3] mailbox: Add ability for clients to abort data in channel
On Thu, Jan 17, 2019 at 2:00 AM CK Hu wrote:
>
> Hi, Jassi:
>
> On Wed, 2019-01-16 at 10:22 -0600, Jassi Brar wrote:
> > On Tue, Jan 15, 2019 at 11:07 PM CK Hu wrote:
> > >
> > > This patch supplies a new framework API, mbox_abort_channel(), and
> > > a new controller interface, abort_data().
> > >
> > > For some client's application, it need to clean up the data in channel
> > > but keep the channel so it could send data to channel later.
> > >
> > > Signed-off-by: CK Hu
> > > ---
> > > drivers/mailbox/mailbox.c | 23 +++
> > > include/linux/mailbox_client.h | 1 +
> > > include/linux/mailbox_controller.h | 4
> > > 3 files changed, 28 insertions(+)
> > >
> > > diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> > > index c6a7d4582dc6..281647162c76 100644
> > > --- a/drivers/mailbox/mailbox.c
> > > +++ b/drivers/mailbox/mailbox.c
> > > @@ -428,6 +428,29 @@ struct mbox_chan *mbox_request_channel_byname(struct
> > > mbox_client *cl,
> > > }
> > > EXPORT_SYMBOL_GPL(mbox_request_channel_byname);
> > >
> > > +/**
> > > + * mbox_abort_channel - The client abort all data in a mailbox
> > > + * channel by this call.
> > > + * @chan: The mailbox channel to be aborted.
> > > + */
> > > +void mbox_abort_channel(struct mbox_chan *chan)
> > > +{
> > > + unsigned long flags;
> > > +
> > > + if (!chan || !chan->cl)
> > > + return;
> > > +
> > > + if (chan->mbox->ops->abort_data)
> > > + chan->mbox->ops->abort_data(chan);
> > > +
> > > + /* The queued TX requests are simply aborted, no callbacks are
> > > made */
> > > + spin_lock_irqsave(>lock, flags);
> > > + chan->cl = NULL;
> > > + chan->active_req = NULL;
> > > + spin_unlock_irqrestore(>lock, flags);
> > > +}
> > >
> > Why not just release and then request channel again ?
> > mbox_abort_channel() is just a copy of mbox_free_channel() and if the
> > abort can sleep, that is more reason to just do that.
>
> The cursor may change position 300 times in one second, so I still
> concern the processing time. Request and free channel looks too heavy to
> do 300 times per second.
You send data 300Hz, not abort ... which is supposed to be a rare event.
And then your shutdown/startup is currently empty, while abort isn't
quite lightweight.
> Conceptually, I just want to clean up the
> channel rather than free and request it, so they are somewhat different.
> I say it may sleep because mbox_abort_channel() is a copy of
> mbox_free_channel(). I could change it to 'must not sleep' because
> Mediatek controller does not sleep in abort callback.
>
No please. Just use the shutdown/startup.
Thanks.
Re: [PATCH 1/3] mailbox: Add ability for clients to abort data in channel
On Tue, Jan 15, 2019 at 11:07 PM CK Hu wrote:
>
> This patch supplies a new framework API, mbox_abort_channel(), and
> a new controller interface, abort_data().
>
> For some client's application, it need to clean up the data in channel
> but keep the channel so it could send data to channel later.
>
> Signed-off-by: CK Hu
> ---
> drivers/mailbox/mailbox.c | 23 +++
> include/linux/mailbox_client.h | 1 +
> include/linux/mailbox_controller.h | 4
> 3 files changed, 28 insertions(+)
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index c6a7d4582dc6..281647162c76 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -428,6 +428,29 @@ struct mbox_chan *mbox_request_channel_byname(struct
> mbox_client *cl,
> }
> EXPORT_SYMBOL_GPL(mbox_request_channel_byname);
>
> +/**
> + * mbox_abort_channel - The client abort all data in a mailbox
> + * channel by this call.
> + * @chan: The mailbox channel to be aborted.
> + */
> +void mbox_abort_channel(struct mbox_chan *chan)
> +{
> + unsigned long flags;
> +
> + if (!chan || !chan->cl)
> + return;
> +
> + if (chan->mbox->ops->abort_data)
> + chan->mbox->ops->abort_data(chan);
> +
> + /* The queued TX requests are simply aborted, no callbacks are made */
> + spin_lock_irqsave(>lock, flags);
> + chan->cl = NULL;
> + chan->active_req = NULL;
> + spin_unlock_irqrestore(>lock, flags);
> +}
>
Why not just release and then request channel again ?
mbox_abort_channel() is just a copy of mbox_free_channel() and if the
abort can sleep, that is more reason to just do that.
