From patchwork Wed Feb 28 18:56:40 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Samuel Holland X-Patchwork-Id: 10249351 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 93F8660384 for ; Wed, 28 Feb 2018 18:57:07 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 7E6FB2890E for ; Wed, 28 Feb 2018 18:57:07 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7340F28E0A; Wed, 28 Feb 2018 18:57:07 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id CAD4F2890E for ; Wed, 28 Feb 2018 18:57:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=1edNO6JuODG/ZoxrujWuKOdeOplr/STNnKGDNFA0H/A=; b=GVltYzQjT+maOv 7lqVkBhcGlb1RGH/5Trl0m5pOvSVRZyccQRHEb4YZ//U7m4wWObn0gGxnQHhntTt155nu11h//EB5 v0CNvoLrOqVC13/7QAemaijgwA3CtQGEC8LCtCqdJqxoPY4VjWZ03Uyil6HF4bEoE1OloK2Hia5CT +EFpAjVPR/A9b0ze1vIaCfl2WthH9rWgGgrWbkCIMnbKmMpk23/r11yDJFnlMA+WEP3AeRIOKas1j oT1ubfdRK8QO4PnDINgw74RIYU68IY+/vFXwRaCS7kduxVqZJQKDGQNzuvRsPq+r8rpaBpxYgQoDX Lxoi2F0fmTxdxJK9X4Ig==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.89 #1 (Red Hat Linux)) id 1er6uQ-0005JD-OP; Wed, 28 Feb 2018 18:56:58 +0000 Received: from out5-smtp.messagingengine.com ([66.111.4.29]) by bombadil.infradead.org with esmtps (Exim 4.89 #1 (Red Hat Linux)) id 1er6uM-0005GK-I5; Wed, 28 Feb 2018 18:56:56 +0000 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id 039E920C1F; Wed, 28 Feb 2018 13:56:43 -0500 (EST) Received: from frontend1 ([10.202.2.160]) by compute5.internal (MEProxy); Wed, 28 Feb 2018 13:56:43 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sholland.org; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc; s=fm2; bh=u6SlFJiD+MbtfZgzd24VezZAak2dy +SwXxC1Lc4cEIE=; b=OKhrMv/E3n1dA0+95s2yqJjZsP189IIiucImectbIZOGz GzqD+UxCh+fD1VpyzvSG1ORjBZ0ZN+tsTlKdpExrGVNuNDmnhpYX6o9fpRXrJv9u 5OzJ19egE95BwLQSQcldGfjaWcWuqWOBrQcYykP/Yw4WlhAp5/NyDDYsx8UdBcdF bBCodLgtzQ5sHeh4rNkraH7WizZbiyS8Fhi1i0DKjo6d31JIZF7pKjo4JhcoToJp Lhi9HMDzrLKEfprcvFQGqsqjJLHOSrk528JKP4FMj8/DQjvOuj2IE8JxLlmw+53H gwfDr/23Rh43cnZ5aiUVzZO2JP84M4HLegD3Tu2Ug== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; bh=u6SlFJ iD+MbtfZgzd24VezZAak2dy+SwXxC1Lc4cEIE=; b=CwqffgGcWpYj6jRTgNChab xQSyX+Bdc9id+48T6hfKCdNOQZ+N80dE+N0BYW6Uggme1MbhncPiRXzsLp8LGPCv N8Hdv720hjw7wrROu06ByRvc3FqFZutAVimG4noB9AuGrZ3K8lhPxLmndNbkd9sU Pvjp3J9vkBzS+pmdgiiFNI3PDY6jJCSKtAB4pbk1VF1MxrfMfnc+tNQfV0VB2VFG psnqkPw+OHWsB8gHkX+QWiav+pKBsQsRr3gMFifLNcMPH710ghCLiqUyJxHEz/VJ TCYnWrD0SfDWb0RwLKMzOYI8yuWv01ElkkM3y2Q9fUshjMQECF7egLdF01ppyQBg == X-ME-Sender: Received: from [10.7.33.159] (unknown [161.130.188.198]) by mail.messagingengine.com (Postfix) with ESMTPA id 2B6D87E0ED; Wed, 28 Feb 2018 13:56:42 -0500 (EST) Subject: Re: [PATCH 3/3] mailbox: sunxi-msgbox: Add a new mailbox driver To: Jassi Brar References: <20180228022714.30068-1-samuel@sholland.org> <20180228022714.30068-4-samuel@sholland.org> From: Samuel Holland Message-ID: Date: Wed, 28 Feb 2018 12:56:40 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20180228_105654_756584_B3FF7887 X-CRM114-Status: GOOD ( 23.07 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Devicetree List , Andre Przywara , Linux Kernel Mailing List , Chen-Yu Tsai , Rob Herring , srv_heupstream , ", linux-arm-kernel"@lists.infradead.org, Maxime Ripard , srv_heupstream Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On 02/28/18 12:14, Jassi Brar wrote: > On Wed, Feb 28, 2018 at 11:21 PM, Samuel Holland wrote: >> Hi, >> >> On 02/28/18 03:16, Jassi Brar wrote: >>> On Wed, Feb 28, 2018 at 7:57 AM, Samuel Holland wrote: >>> .... >>> >>>> +/* >>>> + * The message box hardware provides 8 unidirectional channels. As the mailbox >>>> + * framework expects them to be bidirectional >>>> >>> That is incorrect. Mailbox framework does not require a channel to be >>> TX and RX capable. >> >> Sorry, it would be more accurate to say that the intended mailbox _client_ >> expects the channels to be bidirectional. >> > Mailbox clients are very mailbox provider specific. Your client driver > is unlikely to be reuseable over another controller+remote combo. > Your client has to know already what a physical channel can do (RX, TX > or RXTX). There is no api to provide that info. There's a public specification for SCPI available[1]. I'm writing the remote endpoint to follow that protocol specification. (There's even an explicitly platform-agnostic update to the protocol called SCMI[2]). Ideally, I should be able to reuse the existing Linux client drivers for that protocol. Are you suggesting that I need to write a copy of the arm_scpi driver, completely identical except that it requests two mailbox channels per client (sending on one and receiving on the other) instead of one? In the >1000 line file, this is all that I would need to change: Obviously this is just proof-of-concept code and would need to be cleaned up a bit. The are platform-agnostic protocols using mailbox hardware. There's no reason that the drivers for them need to be platform-specific. I'd really like to avoid duplicating large amounts of code unnecessarily. I'm willing to prepare a patch series adding this functionality to the mailbox API, if it would be accepted. Something like: bool mbox_chan_is_bidirectional(struct mbox_chan *chan); Implemented in drivers like: struct mbox_controller { ... bool bidirectional_chans; }; or: struct mbox_chan_ops { ... bool (*is_bidirectional)(struct mbox_chan *chan); }; > thanks Regards, Samuel [1]: http://infocenter.arm.com/help/topic/com.arm.doc.dui0922g/scp_message_interface_v1_2_DUI0922G_en.pdf [2]: http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/DEN0056A_System_Control_and_Management_Interface.pdf --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -257,7 +257,8 @@ struct scpi_xfer { struct scpi_chan { struct mbox_client cl; - struct mbox_chan *chan; + struct mbox_chan *tx_chan; + struct mbox_chan *rx_chan; void __iomem *tx_payload; void __iomem *rx_payload; struct list_head rx_pending; @@ -541,7 +542,7 @@ msg->rx_len = rx_len; reinit_completion(&msg->done); - ret = mbox_send_message(scpi_chan->chan, msg); + ret = mbox_send_message(scpi_chan->tx_chan, msg); if (ret < 0 || !rx_buf) goto out; @@ -894,8 +895,10 @@ { int i; - for (i = 0; i < count && pchan->chan; i++, pchan++) { - mbox_free_channel(pchan->chan); + for (i = 0; i < count && pchan->tx_chan; i++, pchan++) { + if (pchan->rx_chan && pchan->rx_chan != pchan->tx_chan) + mbox_free_channel(pchan->rx_chan); + mbox_free_channel(pchan->tx_chan); devm_kfree(dev, pchan->xfers); devm_iounmap(dev, pchan->rx_payload); } @@ -968,6 +971,7 @@ dev_err(dev, "no mboxes property in '%pOF'\n", np); return -ENODEV; } + count /= 2; scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL); if (!scpi_chan) @@ -1009,13 +1013,19 @@ ret = scpi_alloc_xfer_list(dev, pchan); if (!ret) { - pchan->chan = mbox_request_channel(cl, idx); - if (!IS_ERR(pchan->chan)) + pchan->tx_chan = mbox_request_channel(cl, idx * 2); + pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1); + /* This isn't quite right, but the idea stands. */ + if (!IS_ERR(pchan->tx_chan) && !IS_ERR(pchan->rx_chan)) continue; - ret = PTR_ERR(pchan->chan); + ret = PTR_ERR(pchan->tx_chan); if (ret != -EPROBE_DEFER) dev_err(dev, "failed to get channel%d err %d\n", - idx, ret); + idx * 2, ret); + ret = PTR_ERR(pchan->rx_chan); + if (ret != -EPROBE_DEFER) + dev_err(dev, "failed to get channel%d err %d\n", + idx * 2 + 1, ret); } err: scpi_free_channels(dev, scpi_chan, idx); But then there are two copies of almost exactly the same driver! If there was an API for determining if a channel was unidirectional or bidirectional, than it would be trivial to support both models in one driver: --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -953,7 +955,7 @@ static int scpi_probe(struct platform_device *pdev) { - int count, idx, ret; + int count, idx, mbox_idx, ret; struct resource res; struct scpi_chan *scpi_chan; struct device *dev = &pdev->dev; @@ -971,13 +973,12 @@ dev_err(dev, "no mboxes property in '%pOF'\n", np); return -ENODEV; } - count /= 2; scpi_chan = devm_kcalloc(dev, count, sizeof(*scpi_chan), GFP_KERNEL); if (!scpi_chan) return -ENOMEM; - for (idx = 0; idx < count; idx++) { + for (idx = 0, mbox_idx = 0; mbox_idx < count; idx++) { resource_size_t size; struct scpi_chan *pchan = scpi_chan + idx; struct mbox_client *cl = &pchan->cl; @@ -1014,7 +1015,13 @@ ret = scpi_alloc_xfer_list(dev, pchan); if (!ret) { pchan->tx_chan = mbox_request_channel(cl, idx * 2); - pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1); + if (mbox_chan_is_bidirectional(pchan->tx_chan)) { + pchan->rx_chan = pchan->tx_chan; + mbox_idx += 1; + } else { + pchan->rx_chan = mbox_request_channel(cl, idx * 2 + 1); + mbox_idx += 2; + } /* This isn't quite right, but the idea stands. */ if (!IS_ERR(pchan->tx_chan) && !IS_ERR(pchan->rx_chan)) continue; @@ -1034,7 +1041,7 @@ } scpi_info->channels = scpi_chan; - scpi_info->num_chans = count; + scpi_info->num_chans = idx; scpi_info->commands = scpi_std_commands; platform_set_drvdata(pdev, scpi_info);