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=-15.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,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 7CFCFC63697 for ; Thu, 19 Nov 2020 16:09:57 +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 0695E22259 for ; Thu, 19 Nov 2020 16:09:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="e7xO3Geg" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0695E22259 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=fGlAjxwQInHNWOg2GCgFda1VOGgXqoI8PH3wDZbaPFE=; b=e7xO3Geg8nMsLzFfbEt04KLXz W980znoeuQT7FihifSjUc4QEbbP6nME1IO0pIJZwzoMMn7/QuV5lDzKA5iRG5oNqVnVdB1/Kp27ur lCiHL82j6cfLEJtIhmZ+pv5l+Hh+HiItckqanbGzsHOVNtDaHAcVnWgCx13gGw1HF2T3X8TO9x4TE 8E5eoVD53rVPs5pF0RYsQB4ilGPYMPDPNGRateh7dtOsK7hh2IgZR/rwhbc0gBLy3CvkR+zoxfHIn BlLxvP1s0fK0Ghx/mmC9UPR1PgduDjTHf1Vq5kxRT5lHtEbxHw0RLEuuCx+dBSZvFCltG/MmgX/Dy arV0m5hPw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kfmUC-0000tR-PY; Thu, 19 Nov 2020 16:08:40 +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 1kfmU4-0000rj-Gf for linux-arm-kernel@lists.infradead.org; Thu, 19 Nov 2020 16:08:34 +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 938271478; Thu, 19 Nov 2020 08:08:29 -0800 (PST) Received: from bogus (unknown [10.57.54.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F2B453F719; Thu, 19 Nov 2020 08:08:26 -0800 (PST) Date: Thu, 19 Nov 2020 16:08:24 +0000 From: Sudeep Holla To: Cristian Marussi Subject: Re: [PATCH v5 1/5] firmware: arm_scmi: Add Voltage Domain Support Message-ID: <20201119160824.3vztaax75p3fg4vl@bogus> References: <20201117123415.55105-1-cristian.marussi@arm.com> <20201117123415.55105-2-cristian.marussi@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20201117123415.55105-2-cristian.marussi@arm.com> User-Agent: NeoMutt/20171215 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201119_110832_734127_3593D717 X-CRM114-Status: GOOD ( 29.80 ) 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: devicetree@vger.kernel.org, satyakim@qti.qualcomm.com, f.fainelli@gmail.com, vincent.guittot@linaro.org, robh@kernel.org, linux-kernel@vger.kernel.org, broonie@kernel.org, linux-arm-kernel@lists.infradead.org, Jonathan.Cameron@Huawei.com, souvik.chakravarty@arm.com, etienne.carriere@linaro.org, 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 On Tue, Nov 17, 2020 at 12:34:11PM +0000, Cristian Marussi wrote: > Add SCMI Voltage Domain protocol support. > > Signed-off-by: Cristian Marussi > --- > v4 --> v5 > - removed inline > - moved segmented intervals defines > - fixed some macros complaints by checkpatch > > v3 --> v4 > - avoid fixed sized typing in voltage_info > - avoid coccinelle falde complaints about pointer-sized allocations > > v2 --> v3 > - restrict segmented voltage domain descriptors to one triplet > - removed unneeded inline > - free allocated resources for invalid voltage domain > - added __must_check to info_get voltage operations > - added a few comments > - removed fixed size typing from struct voltage_info > > v1 --> v2 > - fix voltage levels query loop to reload full cmd description > between iterations as reported by Etienne Carriere > - ensure rx buffer is properly sized calli scmi_reset_rx_to_maxsz > between transfers > --- > drivers/firmware/arm_scmi/Makefile | 2 +- > drivers/firmware/arm_scmi/common.h | 1 + > drivers/firmware/arm_scmi/driver.c | 2 + > drivers/firmware/arm_scmi/voltage.c | 397 ++++++++++++++++++++++++++++ > include/linux/scmi_protocol.h | 64 +++++ > 5 files changed, 465 insertions(+), 1 deletion(-) > create mode 100644 drivers/firmware/arm_scmi/voltage.c > > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile > index bc0d54f8e861..6a2ef63306d6 100644 > --- a/drivers/firmware/arm_scmi/Makefile > +++ b/drivers/firmware/arm_scmi/Makefile > @@ -4,7 +4,7 @@ scmi-driver-y = driver.o notify.o > scmi-transport-y = shmem.o > scmi-transport-$(CONFIG_MAILBOX) += mailbox.o > scmi-transport-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smc.o > -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o > +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o voltage.o > scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \ > $(scmi-transport-y) > obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > index 65063fa948d4..c0fb45e7c3e8 100644 > --- a/drivers/firmware/arm_scmi/common.h > +++ b/drivers/firmware/arm_scmi/common.h > @@ -169,6 +169,7 @@ DECLARE_SCMI_REGISTER_UNREGISTER(perf); > DECLARE_SCMI_REGISTER_UNREGISTER(power); > DECLARE_SCMI_REGISTER_UNREGISTER(reset); > DECLARE_SCMI_REGISTER_UNREGISTER(sensors); > +DECLARE_SCMI_REGISTER_UNREGISTER(voltage); > DECLARE_SCMI_REGISTER_UNREGISTER(system); > > #define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(id, name) \ > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index 3dfd8b6a0ebf..ada35e63feae 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -946,6 +946,7 @@ static int __init scmi_driver_init(void) > scmi_power_register(); > scmi_reset_register(); > scmi_sensors_register(); > + scmi_voltage_register(); > scmi_system_register(); > > return platform_driver_register(&scmi_driver); > @@ -961,6 +962,7 @@ static void __exit scmi_driver_exit(void) > scmi_power_unregister(); > scmi_reset_unregister(); > scmi_sensors_unregister(); > + scmi_voltage_unregister(); > scmi_system_unregister(); > > platform_driver_unregister(&scmi_driver); > diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c > new file mode 100644 > index 000000000000..6b71589e0846 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/voltage.c > @@ -0,0 +1,397 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * System Control and Management Interface (SCMI) Voltage Protocol > + * > + * Copyright (C) 2020 ARM Ltd. > + */ > + > +#include > + > +#include "common.h" > + > +#define VOLTAGE_DOMS_NUM_MASK GENMASK(15, 0) > +#define REMAINING_LEVELS_MASK GENMASK(31, 16) > +#define RETURNED_LEVELS_MASK GENMASK(11, 0) > + > +enum scmi_voltage_protocol_cmd { > + VOLTAGE_DOMAIN_ATTRIBUTES = 0x3, > + VOLTAGE_DESCRIBE_LEVELS = 0x4, > + VOLTAGE_CONFIG_SET = 0x5, > + VOLTAGE_CONFIG_GET = 0x6, > + VOLTAGE_LEVEL_SET = 0x7, > + VOLTAGE_LEVEL_GET = 0x8, > +}; > + > +struct scmi_msg_resp_protocol_attributes { > + __le32 attr; > +#define NUM_VOLTAGE_DOMAINS(x) ((u16)(FIELD_GET(VOLTAGE_DOMS_NUM_MASK, (x)))) > +}; > + Sorry but same annoying comment again, drop one element structures. > +struct scmi_msg_resp_domain_attributes { > + __le32 attr; > + u8 name[SCMI_MAX_STR_SIZE]; > +}; > + > +struct scmi_msg_cmd_describe_levels { > + __le32 domain_id; > + __le32 level_index; > +}; > + > +struct scmi_msg_resp_describe_levels { > + __le32 flags; > +#define NUM_REMAINING_LEVELS(f) ((u16)(FIELD_GET(REMAINING_LEVELS_MASK, (f)))) > +#define NUM_RETURNED_LEVELS(f) ((u16)(FIELD_GET(RETURNED_LEVELS_MASK, (f)))) > +#define SUPPORTS_SEGMENTED_LEVELS(f) ((f) & BIT(12)) > + __le32 voltage[]; > +}; > + > +struct scmi_msg_cmd_config_set { > + __le32 domain_id; > + __le32 config; > +}; > + > +struct scmi_msg_cmd_level_set { > + __le32 domain_id; > + __le32 flags; > + __le32 voltage_level; > +}; > + > +struct voltage_info { > + unsigned int version; > + unsigned int num_domains; > + struct scmi_voltage_info **domains; > +}; > + > +static int scmi_protocol_attributes_get(const struct scmi_handle *handle, > + struct voltage_info *vinfo) > +{ > + int ret; > + struct scmi_xfer *t; > + struct scmi_msg_resp_protocol_attributes *resp; > + > + ret = scmi_xfer_get_init(handle, PROTOCOL_ATTRIBUTES, > + SCMI_PROTOCOL_VOLTAGE, 0, sizeof(*resp), &t); > + if (ret) > + return ret; > + > + resp = t->rx.buf; > + ret = scmi_do_xfer(handle, t); > + if (!ret) > + vinfo->num_domains = > + NUM_VOLTAGE_DOMAINS(le32_to_cpu(resp->attr)); > + > + scmi_xfer_put(handle, t); > + return ret; > +} > + > +static int scmi_init_voltage_levels(struct device *dev, > + struct scmi_voltage_info *v, > + u32 flags, u32 num_returned, > + u32 num_remaining) > +{ > + bool segmented; > + u32 num_levels; > + Why can't you pass the above 2 directly from the caller to this function since they are just used to obtain them here. [...] > +static int scmi_voltage_descriptors_get(const struct scmi_handle *handle, > + struct voltage_info *vinfo) > +{ > + int ret, dom; > + struct scmi_xfer *td, *tl; > + struct device *dev = handle->dev; > + struct scmi_msg_resp_domain_attributes *resp_dom; > + struct scmi_msg_resp_describe_levels *resp_levels; > + > + ret = scmi_xfer_get_init(handle, VOLTAGE_DOMAIN_ATTRIBUTES, > + SCMI_PROTOCOL_VOLTAGE, sizeof(__le32), > + sizeof(*resp_dom), &td); > + if (ret) > + return ret; > + resp_dom = td->rx.buf; > + > + ret = scmi_xfer_get_init(handle, VOLTAGE_DESCRIBE_LEVELS, > + SCMI_PROTOCOL_VOLTAGE, sizeof(__le64), 0, &tl); > + if (ret) > + goto outd; > + resp_levels = tl->rx.buf; > + > + for (dom = 0; dom < vinfo->num_domains; dom++) { > + u32 desc_index = 0; > + u16 num_returned = 0, num_remaining = 0; > + struct scmi_msg_cmd_describe_levels *cmd; > + struct scmi_voltage_info *v; > + > + /* Retrieve domain attributes at first ... */ > + put_unaligned_le32(dom, td->tx.buf); > + ret = scmi_do_xfer(handle, td); > + /* Skip domain on comms error */ > + if (ret) > + continue; > + > + v = devm_kzalloc(dev, sizeof(*v), GFP_KERNEL); > + if (!v) { > + ret = -ENOMEM; > + break; > + } > + Why can't we allocate vinfo->domains real structure instead of pointers and indirection there ? I understand that it helps to manage holes easily but I think that would simplify the dynamic allocation and error handling. It doesn't have to be this complicated(not much but still) IMO. May be scmi_voltage_info_get can use num_levels to either return NULL or vinfo->domains[domain_id] ? Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel