From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751234AbdJDRh3 (ORCPT ); Wed, 4 Oct 2017 13:37:29 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:35956 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750779AbdJDRh2 (ORCPT ); Wed, 4 Oct 2017 13:37:28 -0400 Cc: Sudeep Holla , ALKML , LKML , DTML , Roy Franz , Harb Abdulhamid , Nishanth Menon , Loc Ho , Alexey Klimov , Ryan Harkin , Jassi Brar Subject: Re: [PATCH v3 04/22] firmware: arm_scmi: add basic driver infrastructure for SCMI To: Arnd Bergmann References: <1506604306-20739-1-git-send-email-sudeep.holla@arm.com> <1506604306-20739-5-git-send-email-sudeep.holla@arm.com> From: Sudeep Holla Organization: ARM Message-ID: Date: Wed, 4 Oct 2017 18:37:22 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/10/17 11:59, Arnd Bergmann wrote: > On Thu, Sep 28, 2017 at 3:11 PM, Sudeep Holla wrote: > >> +/** >> + * struct scmi_msg_hdr - Message(Tx/Rx) header >> + * >> + * @id: The identifier of the command being sent >> + * @protocol_id: The identifier of the protocol used to send @id command >> + * @seq: The token to identify the message. when a message/command returns, >> + * the platform returns the whole message header unmodified including >> + * the token. >> + */ >> +struct scmi_msg_hdr { >> + u8 id; >> + u8 protocol_id; >> + u16 seq; >> + u32 status; >> + bool poll_completion; >> +}; > > Is this structure part of the protocol, or just part of the linux > implementation? > If this is in the protocol, you should not have a 'bool' member in there, which > does not have a well-defined binary representation across architectures. > No, it's not part of specification just linux, added to support polling based DVFS messages. >> +/* >> + * The SCP firmware providing SCM interface to OSPM and other agents must >> + * execute only in little-endian mode as per SCMI specification, so any buffers >> + * shared through SCMI should have their contents converted to little-endian >> + */ > > That is a very odd thing to put into a specification, are you sure it requires > a specific runtime endian-mode? I would bet that it only requires the protocol > to use little-endian data, so better describe it like that. > Right, my bad. Not sure where I copied that text from, but as you mention specification just expects data to be in LE format. [..] > >> +#if IS_REACHABLE(CONFIG_ARM_SCMI_PROTOCOL) >> +int scmi_handle_put(const struct scmi_handle *handle); >> +const struct scmi_handle *scmi_handle_get(struct device *dev); >> +const struct scmi_handle *devm_scmi_handle_get(struct device *dev); > > IS_REACHABLE() can easily lead to confusion when the driver is > a loadable module but never gets used by a built-in driver. Maybe use > IS_ENABLED() here, and add a Kconfig symbol that other drivers > can depend on if you want them to optionally use it, like: > > config MAYBE_ARM_SCMI_PROTOCOL > default y if ARM_SCMI_PROTOCOL=n > default ARM_SCMI_PROTOCOL OK, will check, we may not need that yet. -- Regards, Sudeep