From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757850AbcHWOn1 (ORCPT ); Tue, 23 Aug 2016 10:43:27 -0400 Received: from foss.arm.com ([217.140.101.70]:37657 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752554AbcHWOnZ (ORCPT ); Tue, 23 Aug 2016 10:43:25 -0400 Subject: Re: [PATCH 03/13] scpi: Add legacy send, prepare and handle remote functions To: Neil Armstrong , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org References: <1471515066-3626-1-git-send-email-narmstrong@baylibre.com> <1471515066-3626-4-git-send-email-narmstrong@baylibre.com> <3d384710-c2c1-974f-c217-a2198d98ea22@baylibre.com> Cc: Sudeep Holla , linux-amlogic@lists.infradead.org, khilman@baylibre.com, heiko@sntech.de, wxt@rock-chips.com, frank.wang@rock-chips.com From: Sudeep Holla Organization: ARM Message-ID: <7dcd921d-e48f-ca46-2b86-4df3e332ff44@arm.com> Date: Tue, 23 Aug 2016 15:42:30 +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: <3d384710-c2c1-974f-c217-a2198d98ea22@baylibre.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 23/08/16 09:15, Neil Armstrong wrote: > On 08/19/2016 06:13 PM, Sudeep Holla wrote: >> >> >> On 18/08/16 11:10, Neil Armstrong wrote: >>> In order to support the legacy SCPI procotol, add specific message_send, >>> prepare_tx and handle_remote functions since the legacy procotol >>> do not support message queuing and does not store the command word in the >>> tx_payload data. >>> >>> Signed-off-by: Neil Armstrong >>> --- >>> drivers/firmware/arm_scpi.c | 69 +++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 69 insertions(+) >>> >>> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c >>> index 0bb6134..50b1297 100644 >>> --- a/drivers/firmware/arm_scpi.c >>> +++ b/drivers/firmware/arm_scpi.c [..] >>> >>> +static void legacy_scpi_tx_prepare(struct mbox_client *c, void *__msg) >>> +{ >>> + struct scpi_chan *ch = >>> + container_of(c, struct scpi_chan, cl); >>> + >>> + if (ch->t->tx_buf && ch->t->tx_len) >>> + memcpy_toio(ch->tx_payload, ch->t->tx_buf, ch->t->tx_len); >> >> >> I see that you are not using the list. Any particular reason for that ? >> >> IMO, that *might* help to reuse more code, but I may be wrong. Let's see >> Some commands like DVFS take more time compared to simple query type of >> commands. Queuing does help there instead of blocking the channel until >> the receipt of response. > > I'll like to use the list, but, the "cmd" value is not stored in the shared tx > memory, so we cannot recover the original tranfer from reading the tx memory cmd. > Even in the current driver we read the mem->command and search the list in scpi_process_cmd. Instead *(u32 *)msg gives the command value, no ? > This is why I added a "struct scpi_xfer *t;" in the scpi_chan structure to store > the current transfer. > I don't like that. I am trying to get rid of that. 1. list is not being used 2. scpi_xfer is stashed even though we have only one command in progress at any time in your case. >> >>> +} >>> + >>> static int legacy_high_priority_cmds[] = { >>> LEGACY_SCPI_CMD_GET_CSS_PWR_STATE, >>> LEGACY_SCPI_CMD_CFG_PWR_STATE_STAT, >>> @@ -434,6 +461,48 @@ static void put_scpi_xfer(struct scpi_xfer *t, struct scpi_chan *ch) >>> mutex_unlock(&ch->xfers_lock); >>> } >>> >>> +static int legacy_scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len, >>> + void *rx_buf, unsigned int rx_len) >>> +{ >>> + int ret; >>> + u8 chan; >>> + struct scpi_xfer *msg; >>> + struct scpi_chan *scpi_chan; >>> + >>> + chan = legacy_scpi_get_chan(cmd); >>> + scpi_chan = scpi_info->channels + chan; >>> + >>> + msg = get_scpi_xfer(scpi_chan); >>> + if (!msg) >>> + return -ENOMEM; >>> + >>> + mutex_lock(&scpi_chan->xfers_lock); >>> + >> >> May be you can copy msg->cmd to msg->slot and that may help to reuse >> more code or worst-case keep them aligned. > > Yes, it could be. But since the msg is not reused bu the tx_prepare and handle_response, > we can pass anything here. IIUC, we should be able to handle it using msg pte in handle_remote_msg > And for the rockchip case, we must pass an xfer unrelated pointer here since they need > a specially crafted memory structure for the mailbox. > We can consider that when they upstream the driver. Let's not consider it now. >> >>> + msg->cmd = PACK_LEGACY_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); >>> + scpi_chan->t = msg; >>> + >>> + ret = mbox_send_message(scpi_chan->chan, &msg->cmd); >> >> If slot is initialized to cmd, then you can pass msg itself above. >> Then you can evaluate how much this function deviates from >> scpi_send_message and try to re-use. > > The function deviates quite a lot since the queing is not used. > You have not given me the reason for not using the list yet. If the msg ptr is used in scpi_handle_remote_msg, you should be able to make use of it. >> >>> + if (ret < 0) >>> + goto out; >>> + >>> + if (!wait_for_completion_timeout(&msg->done, MAX_RX_TIMEOUT)) >>> + ret = -ETIMEDOUT; >>> + else >>> + /* first status word */ >>> + ret = msg->status; >>> +out: >>> + mutex_unlock(&scpi_chan->xfers_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; >>> +} >>> + >>> static int __scpi_send_message(u8 cmd, void *tx_buf, unsigned int tx_len, >>> void *rx_buf, unsigned int rx_len, bool extn) >>> { >>> >> >> [Nit]: Not sure if we need this as a separate patch. It might just >> generate warnings, anyways we can merge into one later. > > I'll prefer to have functionnaly separate patches for now to clarify the changes. > I'll eventually merge them for the final apply if needed. > Ah OK, then fine. -- Regards, Sudeep