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 X-Spam-Level: X-Spam-Status: No, score=-9.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 29A3CC433EF for ; Tue, 7 Sep 2021 18:59:39 +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 E8A8461108 for ; Tue, 7 Sep 2021 18:59:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E8A8461108 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=rosenzweig.io 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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=IZpYa26mOqvIDMF9ALLMR+mFrSwOPJpt802UZXD6CqU=; b=dZMISoL62TweZr yQmhieruTwlgbJkeXIZIBio8nwvkwBduHBeMSBLkhs5YJUT99pIxA2oRCHyp1dAwfSdGtBNjCBBSe rhPl2UQvQEyMwXhB2gqogxRpfJPO140rldA79LXW7N46qGbgJ4b3/Ykv5L6dg10BMkMjzdqbwSyNH rgdbsYtJMSgH4+mG1sKB+osOR0DaqwM1ufvbPiGKvVtWdxcLq9KgB2bW5z2ddkJDIKZ+2NaPgw7ob sDwUJnHMk6UQqrjgaZrYFG0Btz5uf8R4WmPuETV0QXHFecmkGpf2oMbME4jW0Nw68vkH1JnPU17DG 5t4Lh1GrQ458PX72GfrA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mNgHp-004UDD-5G; Tue, 07 Sep 2021 18:57:37 +0000 Received: from rosenzweig.io ([138.197.143.207]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mNgHb-004UAx-TU for linux-arm-kernel@lists.infradead.org; Tue, 07 Sep 2021 18:57:25 +0000 Date: Tue, 7 Sep 2021 14:54:40 -0400 From: Alyssa Rosenzweig To: Sven Peter Cc: Jassi Brar , Rob Herring , Mark Kettenis , Hector Martin , Mohamed Mediouni , Stan Skowronek , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/3] mailbox: apple: Add driver for Apple mailboxes Message-ID: References: <20210907145501.69161-1-sven@svenpeter.dev> <20210907145501.69161-4-sven@svenpeter.dev> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210907145501.69161-4-sven@svenpeter.dev> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210907_115724_099357_706F63F0 X-CRM114-Status: GOOD ( 36.65 ) 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 > + Apple SoCs have various co-processors that need to be started and > + initialized for certain peripherals to work (NVMe, display controller, > + etc.). This driver adds support for the mailbox controller used to > + communicate with those. This makes it seem like it's just a boot time issue. Maybe that's the case for NVMe? In general the mailbox is needed 100% of the time. I suggest something like: Apple SoCs have various co-processors required for certain peripherals to work (NVMe, display controller, etc.). This driver adds support for the mailbox controller used to communicate with those. > + * Copyright (C) 2021 The Asahi Linux Contributors Isn't this all you? > + * Various clients implement different IPC protocols based on these simple s/clients/coprocessors/ or firmware or something? > + * Both the main CPU and the co-processor see the same set of registers but > + * the first FIFO (A2I) is always used to transfer messages from the application > + * processor (us) to the I/O processor and the second one (I2A) for the > + * other direction. Do we actually know what the copro sees? It seems obvious, but *know*? > +#define APPLE_ASC_MBOX_CONTROL_FULL BIT(16) > +#define APPLE_ASC_MBOX_CONTROL_EMPTY BIT(17) > + > +#define APPLE_ASC_MBOX_A2I_CONTROL 0x110 > +#define APPLE_ASC_MBOX_A2I_SEND0 0x800 > +#define APPLE_ASC_MBOX_A2I_SEND1 0x808 > +#define APPLE_ASC_MBOX_A2I_RECV0 0x810 > +#define APPLE_ASC_MBOX_A2I_RECV1 0x818 > + > +#define APPLE_ASC_MBOX_I2A_CONTROL 0x114 > +#define APPLE_ASC_MBOX_I2A_SEND0 0x820 > +#define APPLE_ASC_MBOX_I2A_SEND1 0x828 > +#define APPLE_ASC_MBOX_I2A_RECV0 0x830 > +#define APPLE_ASC_MBOX_I2A_RECV1 0x838 > + > +#define APPLE_M3_MBOX_A2I_CONTROL 0x50 > +#define APPLE_M3_MBOX_A2I_SEND0 0x60 > +#define APPLE_M3_MBOX_A2I_SEND1 0x68 > +#define APPLE_M3_MBOX_A2I_RECV0 0x70 > +#define APPLE_M3_MBOX_A2I_RECV1 0x78 > + > +#define APPLE_M3_MBOX_I2A_CONTROL 0x80 > +#define APPLE_M3_MBOX_I2A_SEND0 0x90 > +#define APPLE_M3_MBOX_I2A_SEND1 0x98 > +#define APPLE_M3_MBOX_I2A_RECV0 0xa0 > +#define APPLE_M3_MBOX_I2A_RECV1 0xa8 > + > +#define APPLE_M3_MBOX_CONTROL_FULL BIT(16) > +#define APPLE_M3_MBOX_CONTROL_EMPTY BIT(17) The ordering here is asymmetric (control bits on top or on bottom). Also might be nicer to align things. > +struct apple_mbox { > + void __iomem *regs; > + const struct apple_mbox_hw *hw; > + ... > +}; This requires a lot of pointer chasing to send/receive messages. Will this become a perf issue in any case? It'd be good to get ballparks on how often these mboxes are used. (For DCP, it doesn't matter, only a few hundred messages per second. Other copros may have different behaviour) > +static bool apple_mbox_hw_can_send(struct apple_mbox *apple_mbox) > +{ > + u32 mbox_ctrl = > + readl_relaxed(apple_mbox->regs + apple_mbox->hw->a2i_control); > + > + return !(mbox_ctrl & apple_mbox->hw->control_full); > +} If you do the pointer chasing, I suspect you want accessor functions/macros at least to make it less intrusive... > + dev_dbg(apple_mbox->dev, "> TX %016llx %08x\n", msg->msg0, msg->msg1); Isn't "TX" redundant here? > + dev_dbg(apple_mbox->dev, "got FIFO empty IRQ\n"); I realize it's a dev_dbg but this still seems unnecessarily noisy. > +static irqreturn_t apple_mbox_recv_irq(int irq, void *data) > +{ > + struct apple_mbox *apple_mbox = data; > + struct apple_mbox_msg msg; > + > + while (apple_mbox_hw_can_recv(apple_mbox)) { > + apple_mbox_hw_recv(apple_mbox, &msg); > + mbox_chan_received_data(&apple_mbox->chan, (void *)&msg); > + } ``` A comment is warranted why this loop is safe and will always terminate, especially given this is an IRQ context. (Ah... can a malicious coprocessor DoS the AP by sending messages faster than the AP can process them? freezing the system since preemption might be disabled here? I suppose thee's no good way to protect against that level of goofy.) > + * There's no race if a message comes in between the check in the while > + * loop above and the ack below: If a new messages arrives inbetween > + * those two the interrupt will just fire again immediately after the > + * ack since it's level sensitive. I don't quite understand the reasoning. Why have IRQ controls at all then on the M3? > + if (apple_mbox->hw->has_irq_controls) > + writel_relaxed(apple_mbox->hw->irq_bit_recv_not_empty, > + apple_mbox->regs + apple_mbox->hw->irq_ack); Nit: { braces } since this is two lines. Unless kernel likes this sort of scary aesthetic. Would go away with an accessor, of course. > + /* > + * Only some variants of this mailbox HW provide interrupt control > + * at the mailbox level. We therefore need to handle enabling/disabling > + * interrupts at the main interrupt controller anyway for hardware that > + * doesn't. Just always keep the interrupts we care about enabled at > + * the mailbox level so that both hardware revisions behave almost > + * the same. > + */ Cute. Does macOS do this? Are there any tradeoffs? > + irqname = devm_kasprintf(dev, GFP_KERNEL, "%s-recv", dev_name(dev)); ... > + irqname = devm_kasprintf(dev, GFP_KERNEL, "%s-send", dev_name(dev)); Not sure this is better or worse than specifying IRQ names in the device tree... probably less greppable. > +++ b/include/linux/apple-mailbox.h > @@ -0,0 +1,18 @@ > +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */ > +/* > + * Apple mailbox message format > + * > + * Copyright (C) 2021 The Asahi Linux Contributors > + */ > + > +#ifndef _LINUX_APPLE_MAILBOX_H_ > +#define _LINUX_APPLE_MAILBOX_H_ > + > +#include > + > +struct apple_mbox_msg { > + u64 msg0; > + u32 msg1; > +}; > + > +#endif Seems like such a waste to have a dedicated file for a single structure :'( I guess it's needed for the rtkit library but still .... _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel