From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DF0C4C433EF for ; Sat, 16 Oct 2021 19:19:35 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A0F3460E98 for ; Sat, 16 Oct 2021 19:19:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org A0F3460E98 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=svenpeter.dev Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Subject:Cc:To:From:Date:References: In-Reply-To:Message-Id:Mime-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=pVHIwoITs2N2AgpYmTQWN27AK/QlKyHz4RzVzRpL9dg=; b=hlTrw4MV01jwCR DKUltfG1E6Chfzc40KAMQjVBIrS/6BPqurqHIn1VsUbNL7//OKwnPLy3RT+zQk9Kk5xVkWfT17v66 ku1Rok1Y0ZQ+c3u1KJpNECH5m094Fp+eDlKBsprBXpy2gqLH8+1BjqWGnYPO9BfgTNxogOq+JgCUc 3SQrnC9olY8HVZM6CUv7p0xSaNh+BHBqAmGxuRTHx+t3aU7uSd7L7b8jYytJYq2rTa861AC1n22qL xZ+mq+lY4UvdUv1raYQQk0RU8yXfh/jsRk9l6HH8hQ4DdAxrxHv3y1EHOs/wRnTGPC2NptCV1BsKS +32rJN+O+/A/Ru/K2Kjg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mbpC5-00BBZE-G3; Sat, 16 Oct 2021 19:18:09 +0000 Received: from out5-smtp.messagingengine.com ([66.111.4.29]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mbpC1-00BBXW-Oz for linux-arm-kernel@lists.infradead.org; Sat, 16 Oct 2021 19:18:07 +0000 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id EC7FE5C00B9; Sat, 16 Oct 2021 15:18:02 -0400 (EDT) Received: from imap47 ([10.202.2.97]) by compute1.internal (MEProxy); Sat, 16 Oct 2021 15:18:02 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=svenpeter.dev; h=mime-version:message-id:in-reply-to:references:date:from:to :cc:subject:content-type; s=fm2; bh=p+3fJpf6HsLcqnYgYGZWWNKUWfCV k/9sOVpR4wT6xDE=; b=iyM6CgwiL8L4K3qxv8twtQsIHtWyStR0Pkaug/vgOMfX y/9TPdhVHmNLQkLVVU2L+huLud9vyzygfLAE+a+iZFEiPlHAy1+RJwgSFFQ/6e9p //my73qVLbjzG02ciupQ/C4TCuKxIbwHOF5EnyCcHaaYbJt3rJ0eZGr2EbdSyhFP //rw8s0InEaiptLEXgp4xLLcQX8gyab9f7eL+DOx1cCHBHuk38QDyaIExROhqI5t 03ZMiQj3qddFWMH8pNRJ/J+gnKHGL6yZVdZ37zwiDXhKk4eSBRd+O3MJDUh/Kkyk QDnC3IDR9NlUZEkwiMuTIoXZUZQlyxVYVNavQ+slEw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=p+3fJp f6HsLcqnYgYGZWWNKUWfCVk/9sOVpR4wT6xDE=; b=maLpXOgCpJEVD49lPeN4l0 2ijgKcNsDvU0qQUVRRiMhzGgPS5YfeK1IGBTEoxGjqTsZ32GPcvrIV6HXmQAkrl3 2E3Tft+VcuY7LEncgbP2zfrL/kOdpTgu/2cncBCbmLB0Nta/SLiR2O9AZ/Tj3MzM iLqVgrqASaWpozepL8WqIdtrteL7iBZ/5xMcLMxe+P03oDw4mwITmRbBHU+7TeSb Lic3A0Eknm1PbwdVkd5iah8AcUW0CrCUhp2DsF012j0CfVmrUpKlhK875gimmaih bO+tARmYC+5vvQFY/ItFQOlzRPvpRWTZgOlU/vyZQ3oFtPPzZ1JVYb1hc4sDFh1w == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrvdduiedguddvlecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefofgggkfgjfhffhffvufgtsehttdertderredtnecuhfhrohhmpedfufhv vghnucfrvghtvghrfdcuoehsvhgvnhesshhvvghnphgvthgvrhdruggvvheqnecuggftrf grthhtvghrnhepvedvgeevuddvvedvgfelfeegiedvgeehieeutdelvedvieevveeljeef vedtleehnecuffhomhgrihhnpehkvghrnhgvlhdrohhrghenucevlhhushhtvghrufhiii gvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehsvhgvnhesshhvvghnphgvthgvrhdr uggvvh X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 501) id 681BB2740061; Sat, 16 Oct 2021 15:18:02 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.5.0-alpha0-1345-g8441cd7852-fm-20211006.001-g8441cd78 Mime-Version: 1.0 Message-Id: <33b20615-14bb-49e7-ac2a-4bfdfabeaaa4@www.fastmail.com> In-Reply-To: References: <20210916154911.3168-1-sven@svenpeter.dev> <20210916154911.3168-3-sven@svenpeter.dev> Date: Sat, 16 Oct 2021 21:17:41 +0200 From: "Sven Peter" To: "Jassi Brar" Cc: "Rob Herring" , "Mark Kettenis" , "Hector Martin" , "Alyssa Rosenzweig" , "Mohamed Mediouni" , "Stan Skowronek" , "Devicetree List" , linux-arm-kernel , "Linux Kernel Mailing List" Subject: Re: [PATCH v2 2/2] mailbox: apple: Add driver for Apple mailboxes X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211016_121805_977735_E9F2D5BC X-CRM114-Status: GOOD ( 21.36 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Jassi, Thanks a lot for the review! On Sat, Oct 16, 2021, at 21:04, Jassi Brar wrote: > On Thu, Sep 16, 2021 at 10:49 AM Sven Peter wrote: > > .... >> +static const struct apple_mbox_hw apple_mbox_asc_hw = { >> + .control_full = APPLE_ASC_MBOX_CONTROL_FULL, >> + .control_empty = APPLE_ASC_MBOX_CONTROL_EMPTY, >> + >> + .a2i_control = APPLE_ASC_MBOX_A2I_CONTROL, >> + .a2i_send0 = APPLE_ASC_MBOX_A2I_SEND0, >> + .a2i_send1 = APPLE_ASC_MBOX_A2I_SEND1, >> + >> + .i2a_control = APPLE_ASC_MBOX_I2A_CONTROL, >> + .i2a_recv0 = APPLE_ASC_MBOX_I2A_RECV0, >> + .i2a_recv1 = APPLE_ASC_MBOX_I2A_RECV1, >> + >> + .has_irq_controls = false, >> +}; >> + >> +static const struct apple_mbox_hw apple_mbox_m3_hw = { >> + .control_full = APPLE_M3_MBOX_CONTROL_FULL, >> + .control_empty = APPLE_M3_MBOX_CONTROL_EMPTY, >> + >> + .a2i_control = APPLE_M3_MBOX_A2I_CONTROL, >> + .a2i_send0 = APPLE_M3_MBOX_A2I_SEND0, >> + .a2i_send1 = APPLE_M3_MBOX_A2I_SEND1, >> + >> + .i2a_control = APPLE_M3_MBOX_I2A_CONTROL, >> + .i2a_recv0 = APPLE_M3_MBOX_I2A_RECV0, >> + .i2a_recv1 = APPLE_M3_MBOX_I2A_RECV1, >> + >> + .has_irq_controls = true, >> + .irq_enable = APPLE_M3_MBOX_IRQ_ENABLE, >> + .irq_ack = APPLE_M3_MBOX_IRQ_ACK, >> + .irq_bit_recv_not_empty = APPLE_M3_MBOX_IRQ_I2A_NOT_EMPTY, >> + .irq_bit_send_empty = APPLE_M3_MBOX_IRQ_A2I_EMPTY, >> +}; >> + >> +static const struct of_device_id apple_mbox_of_match[] = { >> + { .compatible = "apple,t8103-asc-mailbox", .data = &apple_mbox_asc_hw }, >> + { .compatible = "apple,t8103-m3-mailbox", .data = &apple_mbox_m3_hw }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, apple_mbox_of_match); >> + > Traditionally, these go at the end of file. Ack. > > .... >> + >> +static int apple_mbox_hw_send(struct apple_mbox *apple_mbox, >> + struct apple_mbox_msg *msg) >> +{ >> + if (!apple_mbox_hw_can_send(apple_mbox)) >> + return -EBUSY; >> + >> + dev_dbg(apple_mbox->dev, "> TX %016llx %08x\n", msg->msg0, msg->msg1); >> + >> + /* >> + * This message may be related to a shared memory buffer and we must >> + * ensure all previous writes to normal memory are visible before >> + * submitting it. >> + */ >> + dma_wmb(); >> + > This may cause unnecessary overhead. > mbox_client.tx_prepare() is where the SHMEM data is copied, which > should also call dma_wmb() just before return. > ...... > Ok, I'll just drop it here then. >> + >> +static int apple_mbox_hw_recv(struct apple_mbox *apple_mbox, >> + struct apple_mbox_msg *msg) >> +{ >> + if (!apple_mbox_hw_can_recv(apple_mbox)) >> + return -ENOMSG; >> + >> + msg->msg0 = readq_relaxed(apple_mbox->regs + apple_mbox->hw->i2a_recv0); >> + msg->msg1 = FIELD_GET( >> + APPLE_MBOX_MSG1_MSG, >> + readq_relaxed(apple_mbox->regs + apple_mbox->hw->i2a_recv1)); >> + >> + dev_dbg(apple_mbox->dev, "< RX %016llx %08x\n", msg->msg0, msg->msg1); >> + >> + /* >> + * This message may be related to a shared memory buffer and we must >> + * ensure any following reads from normal memory only happen after >> + * having read this message. >> + */ >> + dma_rmb(); >> + > .... similarly should be done by the client as the first thing in recv callback. Ok. > > >> +static struct mbox_chan *apple_mbox_of_xlate(struct mbox_controller *mbox, >> + const struct of_phandle_args *spec) >> +{ >> + struct apple_mbox *apple_mbox = dev_get_drvdata(mbox->dev); >> + >> + if (spec->args_count != 0) >> + return ERR_PTR(-EINVAL); >> + if (apple_mbox->chan.con_priv) >> + return ERR_PTR(-EBUSY); >> + >> + apple_mbox->chan.con_priv = apple_mbox; >> + return &apple_mbox->chan; >> +} >> + > we could do without of_xlate callback, by assigning chan.con_priv > already in the .probe() Sure, will do that. > > >> diff --git a/include/linux/apple-mailbox.h b/include/linux/apple-mailbox.h >> + >> +struct apple_mbox_msg { >> + u64 msg0; >> + u32 msg1; >> +}; >> + > Aren't msg0/1 the Tx and Rx channels? If so you may want to separate > them out as such. But of course, I don't know the h/w details so I may > be wrong. This hardware essentially only has a single channel. Depending on the firmware running on the other side sometimes msg1 is used as an endpoint index and sometimes 8bits of msg0 are. This is similar to the discussion about the raspberry pi mailbox hardware [1]. Thanks, Sven [1] https://lore.kernel.org/all/550C7534.8070100@wwwdotorg.org/ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel