From patchwork Mon Sep 19 15:24:57 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sudeep Holla X-Patchwork-Id: 9339685 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 C0AA4607D0 for ; Mon, 19 Sep 2016 15:27:13 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B0674292D1 for ; Mon, 19 Sep 2016 15:27:13 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A4C2C2941D; Mon, 19 Sep 2016 15:27:13 +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=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id E38C3292EE for ; Mon, 19 Sep 2016 15:27:10 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bm0TJ-0004JE-0T; Mon, 19 Sep 2016 15:27:05 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1bm0Rg-0001lY-1y; Mon, 19 Sep 2016 15:25:31 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5A70080D; Mon, 19 Sep 2016 08:25:03 -0700 (PDT) Received: from [10.1.210.28] (e107155-lin.cambridge.arm.com [10.1.210.28]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4588A3F251; Mon, 19 Sep 2016 08:25:00 -0700 (PDT) Subject: Re: [PATCH v3 2/8] scpi: Add alternative legacy structures, functions and macros To: Neil Armstrong , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <1473262477-18045-1-git-send-email-narmstrong@baylibre.com> <1473262477-18045-3-git-send-email-narmstrong@baylibre.com> From: Sudeep Holla Organization: ARM Message-ID: Date: Mon, 19 Sep 2016 16:24:57 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1473262477-18045-3-git-send-email-narmstrong@baylibre.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20160919_082524_531454_FA365F12 X-CRM114-Status: GOOD ( 20.66 ) X-BeenThere: linux-amlogic@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: heiko@sntech.de, frank.wang@rock-chips.com, khilman@baylibre.com, Sudeep Holla , linux-amlogic@lists.infradead.org, wxt@rock-chips.com Sender: "linux-amlogic" Errors-To: linux-amlogic-bounces+patchwork-linux-amlogic=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On 07/09/16 16:34, Neil Armstrong wrote: > In order to support the legacy SCPI protocol variant, add back the structures > and macros that varies against the final specification. > Add indirection table for legacy commands. > Add bitmap field for channel selection > Add support for legacy in scpi_send_message. > > Signed-off-by: Neil Armstrong > --- > drivers/firmware/arm_scpi.c | 218 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 211 insertions(+), 7 deletions(-) > > diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c > index 9a87687..9ba1020 100644 > --- a/drivers/firmware/arm_scpi.c > +++ b/drivers/firmware/arm_scpi.c [..] > @@ -336,6 +424,39 @@ static void scpi_handle_remote_msg(struct mbox_client *c, void *msg) > scpi_process_cmd(ch, cmd); > } > > +static void legacy_scpi_process_cmd(struct scpi_chan *ch) > +{ > + unsigned long flags; > + struct scpi_xfer *t; > + > + spin_lock_irqsave(&ch->rx_lock, flags); > + if (list_empty(&ch->rx_pending)) { > + spin_unlock_irqrestore(&ch->rx_lock, flags); > + return; > + } > + > + t = list_first_entry(&ch->rx_pending, struct scpi_xfer, node); > + list_del(&t->node); > + This is a bad assumption that it will be always first. The legacy SCPI did support multiple commands at a time and they can be reordered when SCP responds to them. Except this it's almost same scpi_process_cmd. You should be able to use it as is if you pass the command. > + /* check if wait_for_completion is in progress or timed-out */ > + if (t && !completion_done(&t->done)) { > + struct legacy_scpi_shared_mem *mem = ch->rx_payload; > + unsigned int len = t->rx_len; > + > + t->status = le32_to_cpu(mem->status); > + memcpy_fromio(t->rx_buf, mem->payload, len); > + complete(&t->done); > + } > + spin_unlock_irqrestore(&ch->rx_lock, flags); > +} > + > +static void legacy_scpi_handle_remote_msg(struct mbox_client *c, void *_msg) > +{ > + struct scpi_chan *ch = container_of(c, struct scpi_chan, cl); > + > + legacy_scpi_process_cmd(ch); You will get the command in *_msg IIRC. So you can just pass that to scpi_process_cmd. You can even reuse scpi_handle_remote_msg } > +} > + > static void scpi_tx_prepare(struct mbox_client *c, void *msg) > { > unsigned long flags; > @@ -356,6 +477,21 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg) > mem->command = cpu_to_le32(t->cmd); > } > > +static void legacy_scpi_tx_prepare(struct mbox_client *c, void *msg) > +{ > + unsigned long flags; > + struct scpi_xfer *t = msg; > + struct scpi_chan *ch = container_of(c, struct scpi_chan, cl); > + > + if (t->tx_buf) > + memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len); > + if (t->rx_buf) { > + spin_lock_irqsave(&ch->rx_lock, flags); > + list_add_tail(&t->node, &ch->rx_pending); > + spin_unlock_irqrestore(&ch->rx_lock, flags); > + } > +} Again here the only difference is token addition. I think we should retain that as it's helpful in debugging and I don't think it will have any issues. Worst case we can make it conditional but let's check if we can retain it first. > @@ -386,15 +522,25 @@ static int scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len, > struct scpi_xfer *msg; > struct scpi_chan *scpi_chan; > > - chan = atomic_inc_return(&scpi_info->next_chan) % scpi_info->num_chans; > + if (scpi_info->is_legacy) > + chan = test_bit(cmd, scpi_info->cmd_priority) ? 1 : 0; > + else > + chan = atomic_inc_return(&scpi_info->next_chan) % > + scpi_info->num_chans; > scpi_chan = scpi_info->channels + chan; > > msg = get_scpi_xfer(scpi_chan); > if (!msg) > return -ENOMEM; > > - msg->slot = BIT(SCPI_SLOT); > - msg->cmd = PACK_SCPI_CMD(cmd, tx_len); > + if (scpi_info->is_legacy) { > + mutex_lock(&scpi_chan->xfers_lock); Why does legacy need a different locking scheme ? [...] > @@ -635,6 +804,24 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val) > return ret; > } > > +static int legacy_scpi_sensor_get_value(u16 sensor, u64 *val) > +{ > + __le16 id = cpu_to_le16(sensor); > + struct sensor_value buf; > + int ret; > + > + ret = check_cmd(CMD_SENSOR_VALUE); > + if (ret) > + return ret; > + > + ret = scpi_send_message(scpi_info->scpi_cmds[CMD_SENSOR_VALUE], > + &id, sizeof(id), &buf, sizeof(buf)); > + if (!ret) > + *val = (u64)le32_to_cpu(buf.lo_val); > + This is not needed as it's backward compatible as discussed before. Any particular reason you retained it here ? diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c index edf1a3327041..165f2fc3b627 100644 --- i/drivers/firmware/arm_scpi.c +++ w/drivers/firmware/arm_scpi.c @@ -419,7 +419,12 @@ static void scpi_handle_remote_msg(struct mbox_client *c, void *msg) { struct scpi_chan *ch = container_of(c, struct scpi_chan, cl); struct scpi_shared_mem *mem = ch->rx_payload; - u32 cmd = le32_to_cpu(mem->command); + u32 cmd; + + if (ch->is_legacy) + cmd = *(u32 *)msg; + else + cmd = le32_to_cpu(mem->command); scpi_process_cmd(ch, cmd);