From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 51350C388F7 for ; Tue, 10 Nov 2020 17:05:47 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C98B3206D8 for ; Tue, 10 Nov 2020 17:05:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="jY98Od2N" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C98B3206D8 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.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:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=srbdlEwgHi2VtT/QhTvZ5jh0DqoJjyEDekIXlhpK/nQ=; b=jY98Od2N6j/pl7EA2Jx4gyjyC qAzkgZdVWS+CPoSEQetIanS+wj4GmBMjfF1pzMGVfKQHAMPEbESxRvCmD/FFDEazFyMDHelqoCZDM YvkodaVKaT7pKgVHGO68jRt4NM7iq7BX7CDHFp6lssRV26z9nKUX8/QiPO3FHCiEB8Gn5qK+PQ4ak YGHXUp6erX7xRUoMB13LkNDHXlvz6JsdB47JWHtINloYFc66IhtDT/MMwCFlNXapoJQL68RE/fm8p sI56YJ5bB4IojXcdALPLQ04WgGdB+8BSObXUZRkmkSChWYDjqfiopFmverdA+0Mq3V23Bj/5Zl99p 1fjPOalAg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kcX4R-0000MF-F7; Tue, 10 Nov 2020 17:04:39 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kcX4O-0000LS-JE for linux-arm-kernel@lists.infradead.org; Tue, 10 Nov 2020 17:04:37 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 221A01396; Tue, 10 Nov 2020 09:04:33 -0800 (PST) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 986CD3F7BB; Tue, 10 Nov 2020 09:04:31 -0800 (PST) Date: Tue, 10 Nov 2020 17:04:29 +0000 From: Cristian Marussi To: Peter Hilber Subject: Re: [PATCH v2 4/6] firmware: arm_scmi: add SCMIv3.0 Sensors timestamped reads Message-ID: <20201110170429.GD42652@e120937-lin> References: <20201026201007.23591-1-cristian.marussi@arm.com> <20201026201007.23591-5-cristian.marussi@arm.com> <69e5ba10-47a5-ca14-103a-03e94f410618@opensynergy.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <69e5ba10-47a5-ca14-103a-03e94f410618@opensynergy.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201110_120436_753199_B9BBAEC2 X-CRM114-Status: GOOD ( 34.39 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mikhail.golubev@opensynergy.com, Igor.Skalkin@opensynergy.com, jbhayana@google.com, sudeep.holla@arm.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, james.quinlan@broadcom.com, Jonathan.Cameron@Huawei.com, egranata@google.com, lukasz.luba@arm.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Peter thanks for the review. On Tue, Nov 10, 2020 at 05:01:26PM +0100, Peter Hilber wrote: > On 26.10.20 21:10, Cristian Marussi wrote: > > Add new .reading_get_timestamped() method to sensor_ops to support SCMIv3.0 > > timestamped reads. > > > > Signed-off-by: Cristian Marussi > > --- > > drivers/firmware/arm_scmi/sensors.c | 134 ++++++++++++++++++++++++++-- > > include/linux/scmi_protocol.h | 22 +++++ > > 2 files changed, 151 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c > > index 5a18f8c84bef..bdb0ed0df683 100644 > > --- a/drivers/firmware/arm_scmi/sensors.c > > +++ b/drivers/firmware/arm_scmi/sensors.c > > @@ -156,6 +156,27 @@ struct scmi_msg_sensor_reading_get { > > #define SENSOR_READ_ASYNC BIT(0) > > }; > > > > +struct scmi_resp_sensor_reading_get { > > + __le64 readings; > > +}; > > + > > +struct scmi_resp_sensor_reading_complete { > > + __le32 id; > > + __le64 readings; > > +}; > > In my understanding the id field is not present in the spec. The > implementation seems to have introduced it already before this patch. > Well, it is indeed defined in 4.7.3.1 "SENSOR_READING_COMPLETE" both in SCMI V3.0 and in V2.0: it is the async delayed response and this 'id' represents the sensor_id: in fact it is used only the in the async path in the reading funcs; the sync version uses directly sensor_reading_le. (which has no id n it) > > + > > +struct scmi_sensor_reading_le { > > + __le32 sensor_value_low; > > + __le32 sensor_value_high; > > + __le32 timestamp_low; > > + __le32 timestamp_high; > > +}; > > + > > +struct scmi_resp_sensor_reading_complete_v3 { > > + __le32 id; > > + struct scmi_sensor_reading_le readings[]; > > +}; > > As above, IMHO the id field is not present in the spec. > As said above it is for the delayed_responses, in this case for V3 messages. > > + > > struct scmi_sensor_trip_notify_payld { > > __le32 agent_id; > > __le32 sensor_id; > > @@ -576,6 +597,21 @@ scmi_sensor_trip_point_config(const struct scmi_handle *handle, u32 sensor_id, > > return ret; > > } > > > > +/** > > + * scmi_sensor_reading_get - Read scalar sensor value > > + * @handle: Platform handle > > + * @sensor_id: Sensor ID > > + * @value: The 64bit value sensor reading > > + * > > + * This function returns a single 64 bit reading value representing the sensor > > + * value; if the platform SCMI Protocol implementation and the sensor support > > + * multiple axis and timestamped-reads, this just returns the first axis while > > + * dropping the timestamp value. > > + * Use instead the @scmi_sensor_reading_get_timestamped to retrieve the array of > > + * timestamped multi-axis values. > > + * > > + * Return: 0 on Success > > + */ > > static int scmi_sensor_reading_get(const struct scmi_handle *handle, > > u32 sensor_id, u64 *value) > > { > > @@ -593,18 +629,105 @@ static int scmi_sensor_reading_get(const struct scmi_handle *handle, > > How about changing the scmi_xfer_get_init() rx_size to 0 (in the > immediately preceding, not shown lines)? An SCMI platform might not > expect to just have room for the first reading, excluding the timestamp. > Ah right, because this is the old v2.0 interface which I kept unchanged but now internally the same v3.0 SENSOR_READING_GET message on a v3.0 platform could return multiple per-axis timestamped values even if I just return the first u64 without timestamp. Is this that you mean ? I'll fix to 0. > > > > sensor = t->tx.buf; > > sensor->id = cpu_to_le32(sensor_id); > > + if (s->async) { > > + sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC); > > + ret = scmi_do_xfer_with_response(handle, t); > > + if (!ret) { > > + struct scmi_resp_sensor_reading_complete *resp; > > + > > + resp = t->rx.buf; > > + if (le32_to_cpu(resp->id) == sensor_id) > > + *value = le64_to_cpu(resp->readings); > > Maybe this le64_to_cpu() and the one a few lines below should be > replaced by get_unaligned_le64()? I'll check. > > > + else > > + ret = -EPROTO; > > + } > > + } else { > > + sensor->flags = cpu_to_le32(0); > > + ret = scmi_do_xfer(handle, t); > > + if (!ret) { > > + struct scmi_resp_sensor_reading_get *resp; > > + > > + resp = t->rx.buf; > > + *value = le64_to_cpu(resp->readings); > > + } > > + } > > > > + scmi_xfer_put(handle, t); > > + return ret; > > +} > > + > > [...] > > + > > /** > > * scmi_range_attrs - specifies a sensor or axis values' range > > * @min_range: The minimum value which can be represented by the sensor/axis. > > @@ -387,6 +401,11 @@ enum scmi_sensor_class { > > * @info_get: get the information of the specified sensor > > * @trip_point_config: selects and configures a trip-point of interest > > * @reading_get: gets the current value of the sensor > > + * @reading_get_timestamped: gets the current value and timestamp, when > > + * available, of the sensor. (as of v2.1 spec) > > Nitpicking: v2.1 -> v3.0 > Ok. > > + * Supports multi-axis sensors for sensors which > > + * supports it and if the @reading array size of > > + * @count entry equals the sensor num_axis > > */ > > struct scmi_sensor_ops { > > int (*count_get)(const struct scmi_handle *handle); > > @@ -396,6 +415,9 @@ struct scmi_sensor_ops { > > u32 sensor_id, u8 trip_id, u64 trip_value); > > int (*reading_get)(const struct scmi_handle *handle, u32 sensor_id, > > u64 *value); > > + int (*reading_get_timestamped)(const struct scmi_handle *handle, > > + u32 sensor_id, u8 count, > > + struct scmi_sensor_reading *readings); > > }; > > > > /** > > > > Best regards, > > Peter Thanks Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel