Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758214AbcJQI0c (ORCPT ); Mon, 17 Oct 2016 04:26:32 -0400 Received: from mail-lf0-f45.google.com ([209.85.215.45]:35163 "EHLO mail-lf0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757662AbcJQI0Z (ORCPT ); Mon, 17 Oct 2016 04:26:25 -0400 Subject: Re: [PATCH v4 2/8] scpi: Add alternative legacy structures, functions and macros To: Sudeep Holla , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <1475652814-30619-1-git-send-email-narmstrong@baylibre.com> <1475652814-30619-3-git-send-email-narmstrong@baylibre.com> <7bf3b68c-19aa-85c3-8b20-66b7380eaf3f@arm.com> Cc: linux-amlogic@lists.infradead.org, khilman@baylibre.com, heiko@sntech.de, wxt@rock-chips.com, frank.wang@rock-chips.com From: Neil Armstrong Organization: Baylibre Message-ID: Date: Mon, 17 Oct 2016 10:25:29 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <7bf3b68c-19aa-85c3-8b20-66b7380eaf3f@arm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10572 Lines: 291 On 10/10/2016 04:36 PM, Sudeep Holla wrote: > Hi Neil, > > Sorry, I could not reply to your response on v3. Anyways I will review v4. > > On 05/10/16 08:33, Neil Armstrong wrote: >> This patch adds support for the Legacy SCPI protocol in early JUNO versions and >> shipped Amlogic ARMv8 based SoCs. Some Rockchip SoC are also known to use this >> version of protocol with extended vendor commands >> . >> In order to support the legacy SCPI protocol variant, add back the structures >> and macros that varies against the final specification. >> Then add indirection table for legacy commands. >> Finally Add bitmap field for channel selection since the Legacy protocol mandates to >> send a selected subset of the commands on the high priority channel instead of the >> low priority channel. >> >> The message sending path differs from the final SCPI procotocol because the >> Amlogic SCP firmware always reply 1 instead of a special value containing the command >> byte and replied rx data length. >> For this reason commands queuing cannot be used and we assume the reply command is >> the head of the rx_pending list since we ensure sequential command sending with a >> separate dedicated mutex. >> >> Signed-off-by: Neil Armstrong >> --- >> drivers/firmware/arm_scpi.c | 221 +++++++++++++++++++++++++++++++++++++++----- >> 1 file changed, 199 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c >> index 498afa0..6244eb1 100644 >> --- a/drivers/firmware/arm_scpi.c >> +++ b/drivers/firmware/arm_scpi.c > > [...] > >> @@ -307,21 +398,46 @@ static void scpi_process_cmd(struct scpi_chan *ch, u32 cmd) >> return; >> } >> >> - list_for_each_entry(t, &ch->rx_pending, node) >> - if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) { >> - list_del(&t->node); >> - match = t; >> - break; >> - } >> + /* Command type is not replied by the SCP Firmware in legacy Mode >> + * We should consider that command is the head of pending RX commands >> + * if the list is not empty. In TX only mode, the list would be empty. >> + */ >> + if (scpi_info->is_legacy) { >> + match = list_first_entry(&ch->rx_pending, struct scpi_xfer, >> + node); >> + list_del(&match->node); >> + } else { >> + list_for_each_entry(t, &ch->rx_pending, node) >> + if (CMD_XTRACT_UNIQ(t->cmd) == CMD_XTRACT_UNIQ(cmd)) { >> + list_del(&t->node); >> + match = t; >> + break; >> + } >> + } >> /* check if wait_for_completion is in progress or timed-out */ >> if (match && !completion_done(&match->done)) { >> - struct scpi_shared_mem *mem = ch->rx_payload; >> - unsigned int len = min(match->rx_len, CMD_SIZE(cmd)); >> + unsigned int len; >> + >> + if (scpi_info->is_legacy) { >> + struct legacy_scpi_shared_mem *mem = ch->rx_payload; >> + >> + /* RX Length is not replied by the lagcy Firmware */ >> + len = match->rx_len; >> + >> + match->status = le32_to_cpu(mem->status); >> + memcpy_fromio(match->rx_buf, mem->payload, len); > > The above 2 seems common to both, no ? No, the shared_mem structure differs. > >> + } else { >> + struct scpi_shared_mem *mem = ch->rx_payload; >> + >> + len = min(match->rx_len, CMD_SIZE(cmd)); >> + >> + match->status = le32_to_cpu(mem->status); >> + memcpy_fromio(match->rx_buf, mem->payload, len); >> + } >> >> - match->status = le32_to_cpu(mem->status); >> - memcpy_fromio(match->rx_buf, mem->payload, len); >> if (match->rx_len > len) >> memset(match->rx_buf + len, 0, match->rx_len - len); >> + > > Spurious ? Yep > >> complete(&match->done); >> } >> spin_unlock_irqrestore(&ch->rx_lock, flags); >> @@ -331,7 +447,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 (scpi_info->is_legacy) >> + cmd = *(u32 *)msg; > > Do we need do this if it doesn't contain command ? No, will remove. > >> + else >> + cmd = le32_to_cpu(mem->command); >> >> scpi_process_cmd(ch, cmd); >> } >> @@ -343,17 +464,26 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg) >> struct scpi_chan *ch = container_of(c, struct scpi_chan, cl); >> struct scpi_shared_mem *mem = (struct scpi_shared_mem *)ch->tx_payload; >> >> - if (t->tx_buf) >> - memcpy_toio(mem->payload, t->tx_buf, t->tx_len); >> + if (t->tx_buf) { >> + if (scpi_info->is_legacy) >> + memcpy_toio(ch->tx_payload, t->tx_buf, t->tx_len); >> + else >> + memcpy_toio(mem->payload, t->tx_buf, t->tx_len); >> + } >> + >> if (t->rx_buf) { >> if (!(++ch->token)) >> ++ch->token; >> ADD_SCPI_TOKEN(t->cmd, ch->token); >> + if (scpi_info->is_legacy) >> + t->slot = t->cmd; > > I thought passing token was not an issue from your previous response, > but you are overriding it here, why ? Indeed, I can leave it, but it's useless since it won't serve to distinguish multiple similar commands. > >> spin_lock_irqsave(&ch->rx_lock, flags); >> list_add_tail(&t->node, &ch->rx_pending); >> spin_unlock_irqrestore(&ch->rx_lock, flags); >> } >> - mem->command = cpu_to_le32(t->cmd); >> + >> + if (!scpi_info->is_legacy) >> + mem->command = cpu_to_le32(t->cmd); >> } >> >> static struct scpi_xfer *get_scpi_xfer(struct scpi_chan *ch) >> @@ -396,21 +526,37 @@ static int scpi_send_message(unsigned int offset, void *tx_buf, >> >> cmd = scpi_info->scpi_cmds[offset]; >> >> - 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) { >> + msg->cmd = PACK_LEGACY_SCPI_CMD(cmd, tx_len); >> + msg->slot = msg->cmd; >> + } else { >> + msg->slot = BIT(SCPI_SLOT); >> + msg->cmd = PACK_SCPI_CMD(cmd, tx_len); >> + } >> msg->tx_buf = tx_buf; >> msg->tx_len = tx_len; >> msg->rx_buf = rx_buf; >> msg->rx_len = rx_len; >> init_completion(&msg->done); >> >> + /* Since we cannot distinguish the original command in the >> + * MHU reply stat value from a Legacy SCP firmware, ensure >> + * sequential command sending to the firmware. >> + */ > > OK this comment now questions the existence of this extra lock. > The mailbox will always send the commands in the sequential order. > It's only firmware that can re-order the response. Since that can't > happen in you case, I really don't see the need for this. > > Please explain the race you would see without this locking. Yes I > understand that only one command is supposed to be sent to firmware at a > time. Suppose you allow more callers here, all will wait on the > completion flags and the first in the list gets unblocked right ? > I am just trying to understand if there's real need for this extra > lock when we already have that from the list. In my current tests I have huge kernel hang when having multiple callers, I must find out where this issue comes from... In any case, we have an issue about the command sequencing. If we push a tx-only command and then right after a tx-rx command, the mailbox callback from the first command won't be able to distinguish which command is handled ! In this case, the rx_pending list will not be empty, some garbage will be returned to the second command handler and the real data from the second command handling will be lost thinking it's a tx-only command. We have two choices here : - Also push the tx-only commands to the rx_pending list, and also wait for their completion - Add an extra lock What is your preferred scheme ? >> + if (scpi_info->is_legacy) >> + mutex_lock(&scpi_chan->legacy_lock); >> + >> ret = mbox_send_message(scpi_chan->chan, msg); >> if (ret < 0 || !rx_buf) >> goto out; >> @@ -421,9 +567,13 @@ static int scpi_send_message(unsigned int offset, void *tx_buf, >> /* first status word */ >> ret = msg->status; >> out: >> - if (ret < 0 && rx_buf) /* remove entry from the list if timed-out */ >> + if (ret < 0 && rx_buf) >> + /* remove entry from the list if timed-out */ >> scpi_process_cmd(scpi_chan, msg->cmd); >> >> + if (scpi_info->is_legacy) >> + mutex_unlock(&scpi_chan->legacy_lock); >> + >> put_scpi_xfer(msg, scpi_chan); >> /* SCPI error codes > 0, translate them to Linux scale*/ >> return ret > 0 ? scpi_to_linux_errno(ret) : ret; > > [...] > >> @@ -525,7 +687,6 @@ static struct scpi_dvfs_info *scpi_dvfs_get_info(u8 domain) >> >> info->count = DVFS_OPP_COUNT(buf.header); >> info->latency = DVFS_LATENCY(buf.header) * 1000; /* uS to nS */ >> - > > Spurious ? Indeed. > >> info->opps = kcalloc(info->count, sizeof(*opp), GFP_KERNEL); >> if (!info->opps) { >> kfree(info); >> @@ -580,9 +741,13 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val) >> >> ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id), >> &buf, sizeof(buf)); >> - if (!ret) >> - *val = (u64)le32_to_cpu(buf.hi_val) << 32 | >> - le32_to_cpu(buf.lo_val); >> + if (!ret) { >> + if (scpi_info->is_legacy) >> + *val = (u64)le32_to_cpu(buf.lo_val); >> + else >> + *val = (u64)le32_to_cpu(buf.hi_val) << 32 | >> + le32_to_cpu(buf.lo_val); >> + } > > Not required as I have mentioned couple of times in previous versions, > it's zero filled by the driver. > OK I will fix the issues, but I need your advice for the locking scheme. I really want this to be merged and be able to go forward ! Thanks, Neil