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 Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 033CFC00528 for ; Thu, 27 Jul 2023 01:07:51 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id CD20486785; Thu, 27 Jul 2023 03:07:49 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="gkhnQZeM"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id F2C4A86784; Thu, 27 Jul 2023 03:07:47 +0200 (CEST) Received: from mail-pf1-x430.google.com (mail-pf1-x430.google.com [IPv6:2607:f8b0:4864:20::430]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id C6EE486785 for ; Thu, 27 Jul 2023 03:07:43 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pf1-x430.google.com with SMTP id d2e1a72fcca58-6862d4a1376so107715b3a.0 for ; Wed, 26 Jul 2023 18:07:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1690420062; x=1691024862; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=tqjPuiCZ2gnISjbv2yVPSzghRQIbtgFCNwRYk8mrwqA=; b=gkhnQZeM9SQOelyl0SPb3elnfsSJuzzAmDOZax6WmtylxGsibnf6xMFL++DIxcK3st u9GollXp6T2N39qPZiLJiRo6AbCchUdOI22AaVLTb3c4KO8C2oNRBJhCWBd/GoAxUHP4 OSlO0g0HrUqzDanxO2+i6t7TCxBaPO0rqXTQMWAvR9Fg9aG2movPaln0yijTbCpb0d1U pOs4LwpV9WCw/1pk72Zha0QTrponrkhvzGebDFtIylnhiHvFNLEkqTOE7PhSXt4M0uoH BdGfzSVIozHQHy0Nw6yCR6/tk1885gSJsh2LvmOg15YRO/OSYaFOkZpCyqCkDqDhFl7X azWw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690420062; x=1691024862; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=tqjPuiCZ2gnISjbv2yVPSzghRQIbtgFCNwRYk8mrwqA=; b=FJlHKZ5b1xnem4B903/R2FU07/VacWCLgaPNGB+4+D6eBL/L5oxN7hmtY/5pbEb/Nm YchP/VcncAwWzsjjGRedp56t3gI4gYwX3Xp4bE4D4P/QyFmN5zz4DZUXE+k5f4ip8PuD z6e61gqJ2XolNiKq5nZ/5D4Sq50kfwPDzaG+5RtbojSfCJanewUHUujvigHh/yISRO5b YLtD9OE6HPYseS9sYQpftYHviAxOBZ+fJcqyN3NmzWfRbTof57SDTkaQOlOBicXpDuFE CBwyUBN+GX7Fk0S9SdMnJ92P/3HkFGZgSqxq5UWdVjUUBfRK4+qkKOjfjvantPgUEenX B4nQ== X-Gm-Message-State: ABy/qLbVirB0YVYO83Iyv28vD0cg3pFsGadB511dZFpK+LAfbb69qjev cRFy+EVvjS/jbjjl7uSmJSR00A== X-Google-Smtp-Source: APBJJlEjTpTkpEzOckQwUvD3diQikdH4wc4uWVNozg42OfMKeNibuA1Gh2L1KYAtoQPIBUQ/qN5jVQ== X-Received: by 2002:a05:6a21:6da1:b0:134:1671:6191 with SMTP id wl33-20020a056a216da100b0013416716191mr4601611pzb.0.1690420061941; Wed, 26 Jul 2023 18:07:41 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:2103:cbda:74f4:860a]) by smtp.gmail.com with ESMTPSA id x20-20020aa79194000000b00678cb337353sm190278pfa.208.2023.07.26.18.07.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 26 Jul 2023 18:07:41 -0700 (PDT) Date: Thu, 27 Jul 2023 10:07:38 +0900 From: AKASHI Takahiro To: Simon Glass Cc: trini@konsulko.com, etienne.carriere@st.com, u-boot@lists.denx.de Subject: Re: [PATCH v2 05/12] firmware: scmi: install base protocol to SCMI agent Message-ID: Mail-Followup-To: AKASHI Takahiro , Simon Glass , trini@konsulko.com, etienne.carriere@st.com, u-boot@lists.denx.de References: <20230726083808.140780-1-takahiro.akashi@linaro.org> <20230726083808.140780-6-takahiro.akashi@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean Hi Simon, Thank you for your extensive review. On Wed, Jul 26, 2023 at 06:50:23PM -0600, Simon Glass wrote: > Hi, > > On Wed, 26 Jul 2023 at 02:39, AKASHI Takahiro > wrote: > > > > SCMI base protocol is mandatory, and once SCMI node is found in a device > > tree, the protocol handle (udevice) is unconditionally installed to > > the agent. Then basic information will be retrieved from SCMI server via > > the protocol and saved into the agent instance's local storage. > > > > Signed-off-by: AKASHI Takahiro > > --- > > v2 > > * use helper functions, removing direct uses of ops > > --- > > drivers/firmware/scmi/scmi_agent-uclass.c | 116 ++++++++++++++++++++++ > > include/scmi_agent-uclass.h | 70 ++++++++++++- > > 2 files changed, 184 insertions(+), 2 deletions(-) > > > > Reviewed-by: Simon Glass > > > diff --git a/drivers/firmware/scmi/scmi_agent-uclass.c b/drivers/firmware/scmi/scmi_agent-uclass.c > > index 2244fcf487e8..279c2c218913 100644 > > --- a/drivers/firmware/scmi/scmi_agent-uclass.c > > +++ b/drivers/firmware/scmi/scmi_agent-uclass.c > > @@ -57,6 +57,9 @@ struct udevice *scmi_get_protocol(struct udevice *dev, > > } > > > > switch (id) { > > + case SCMI_PROTOCOL_ID_BASE: > > + proto = priv->base_dev; > > + break; > > case SCMI_PROTOCOL_ID_CLOCK: > > proto = priv->clock_dev; > > break; > > @@ -101,6 +104,9 @@ static int scmi_add_protocol(struct udevice *dev, > > } > > > > switch (proto_id) { > > + case SCMI_PROTOCOL_ID_BASE: > > + priv->base_dev = proto; > > + break; > > case SCMI_PROTOCOL_ID_CLOCK: > > priv->clock_dev = proto; > > break; > > @@ -229,6 +235,83 @@ int devm_scmi_process_msg(struct udevice *dev, struct scmi_msg *msg) > > return scmi_process_msg(parent, priv->channel, msg); > > } > > > > +/** > > + * scmi_fill_base_info - get base information about SCMI server > > + * @agent: SCMI agent device > > + * @dev: SCMI protocol device > > + * > > + * By using Base protocol commands, collect the base information > > + * about SCMI server. > > + * > > + * Return: 0 on success, error code otherwise > > + */ > > +static int scmi_fill_base_info(struct udevice *agent, struct udevice *dev) > > +{ > > + struct scmi_agent_priv *priv = dev_get_uclass_plat(agent); > > + int ret; > > + > > + ret = scmi_base_protocol_version(dev, &priv->version); > > + if (ret) { > > + dev_err(dev, "protocol_version() failed (%d)\n", ret); > > + return ret; > > + } > > + /* check for required version */ > > + if (priv->version < SCMI_BASE_PROTOCOL_VERSION) { > > + dev_err(dev, "base protocol version (%d) lower than expected\n", > > + priv->version); > > + return -EPROTO; > > + } > > + > > + ret = scmi_base_protocol_attrs(dev, &priv->num_agents, > > + &priv->num_protocols); > > + if (ret) { > > + dev_err(dev, "protocol_attrs() failed (%d)\n", ret); > > + return ret; > > + } > > + ret = scmi_base_discover_vendor(dev, priv->vendor); > > + if (ret) { > > + dev_err(dev, "base_discover_vendor() failed (%d)\n", ret); > > + return ret; > > + } > > + ret = scmi_base_discover_sub_vendor(dev, priv->sub_vendor); > > + if (ret) { > > + if (ret != -EOPNOTSUPP) { > > + dev_err(dev, "base_discover_sub_vendor() failed (%d)\n", > > + ret); > > + return ret; > > + } > > + strcpy(priv->sub_vendor, "NA"); > > + } > > + ret = scmi_base_discover_impl_version(dev, &priv->impl_version); > > + if (ret) { > > + dev_err(dev, "base_discover_impl_version() failed (%d)\n", > > + ret); > > + return ret; > > + } > > + > > + priv->agent_id = 0xffffffff; /* to avoid false claim */ > > + ret = scmi_base_discover_agent(dev, 0xffffffff, > > + &priv->agent_id, priv->agent_name); > > + if (ret) { > > + if (ret != -EOPNOTSUPP) { > > + dev_err(dev, > > + "base_discover_agent() failed for myself (%d)\n", > > + ret); > > + return ret; > > + } > > + priv->agent_name[0] = '\0'; > > + } > > + > > + ret = scmi_base_discover_list_protocols(dev, &priv->protocols); > > + if (ret != priv->num_protocols) { > > + dev_err(dev, "base_discover_list_protocols() failed (%d)\n", > > + ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > /* > > * SCMI agent devices binds devices of various uclasses depeding on > > * the FDT description. scmi_bind_protocol() is a generic bind sequence > > @@ -243,6 +326,39 @@ static int scmi_bind_protocols(struct udevice *dev) > > struct driver *drv; > > struct udevice *proto; > > > > + if (!uclass_get_device(UCLASS_SCMI_AGENT, 1, &agent)) { > > + /* This is a second SCMI agent */ > > + dev_err(dev, "Cannot have more than one SCMI agent\n"); > > + return -EEXIST; > > + } > > + > > + /* initialize the device from device tree */ > > + drv = DM_DRIVER_GET(scmi_base_drv); > > + name = "scmi-base.0"; > > + ret = device_bind(dev, drv, name, NULL, ofnode_null(), &proto); > > Why is this not in the devicetree too? I don't know the history of discussions, but it is because no binding for the base protocol is necessary, I guess, as it is a mandatory protocol without any customisable parameters. Please see kernel's Documentation/devicetree/bindings/firmware/arm,scmi.yaml. > > + if (ret) { > > + dev_err(dev, "failed to bind base protocol\n"); > > + return ret; > > + } > > + ret = scmi_add_protocol(dev, SCMI_PROTOCOL_ID_BASE, proto); > > + if (ret) { > > + dev_err(dev, "failed to add protocol: %s, ret: %d\n", > > + proto->name, ret); > > + return ret; > > + } > > + > > + ret = device_probe(proto); > > + if (ret) { > > + dev_err(dev, "failed to probe base protocol\n"); > > + return ret; > > + } > > + > > + ret = scmi_fill_base_info(dev, proto); > > + if (ret) { > > + dev_err(dev, "failed to get base information\n"); > > + return ret; > > + } > > + > > dev_for_each_subnode(node, dev) { > > u32 protocol_id; > > > > diff --git a/include/scmi_agent-uclass.h b/include/scmi_agent-uclass.h > > index f9e7d3f51de1..0043a93c8d90 100644 > > --- a/include/scmi_agent-uclass.h > > +++ b/include/scmi_agent-uclass.h > > @@ -5,6 +5,7 @@ > > #ifndef _SCMI_AGENT_UCLASS_H > > #define _SCMI_AGENT_UCLASS_H > > > > +#include > > #include > > > > struct scmi_msg; > > @@ -12,16 +13,81 @@ struct scmi_channel; > > > > /** > > * struct scmi_agent_priv - private data maintained by agent instance > > + * @version: Version > > + * @num_agents: Number of agents > > + * @num_protocols: Number of protocols > > + * @impl_version: Implementation version > > + * @protocols: Array of protocol IDs > > + * @vendor: Vendor name > > + * @sub_vendor: Sub-vendor name > > + * @agent_name: Agent name > > + * agent_id: Identifier of agent > > + * @base_dev: SCMI base protocol device > > * @clock_dev: SCMI clock protocol device > > - * @clock_dev: SCMI reset domain protocol device > > - * @clock_dev: SCMI voltage domain protocol device > > + * @resetdom_dev: SCMI reset domain protocol device > > + * @voltagedom_dev: SCMI voltage domain protocol device > > */ > > struct scmi_agent_priv { > > + u32 version; > > + u32 num_agents; > > + u32 num_protocols; > > + u32 impl_version; > > + u8 *protocols; > > + u8 vendor[SCMI_BASE_NAME_LENGTH_MAX]; > > + u8 sub_vendor[SCMI_BASE_NAME_LENGTH_MAX]; > > + u8 agent_name[SCMI_BASE_NAME_LENGTH_MAX]; > > + u32 agent_id; > > + struct udevice *base_dev; > > struct udevice *clock_dev; > > struct udevice *resetdom_dev; > > struct udevice *voltagedom_dev; > > }; > > > > +static inline u32 scmi_version(struct udevice *dev) > > +{ > > + return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->version; > > +} > > + > > +static inline u32 scmi_num_agents(struct udevice *dev) > > +{ > > + return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->num_agents; > > +} > > + > > +static inline u32 scmi_num_protocols(struct udevice *dev) > > +{ > > + return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->num_protocols; > > +} > > + > > +static inline u32 scmi_impl_version(struct udevice *dev) > > +{ > > + return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->impl_version; > > +} > > + > > +static inline u8 *scmi_protocols(struct udevice *dev) > > +{ > > + return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->protocols; > > +} > > + > > +static inline u8 *scmi_vendor(struct udevice *dev) > > +{ > > + return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->vendor; > > +} > > + > > +static inline u8 *scmi_sub_vendor(struct udevice *dev) > > +{ > > + return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->sub_vendor; > > +} > > + > > +static inline u8 *scmi_agent_name(struct udevice *dev) > > +{ > > + return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->agent_name; > > +} > > + > > +static inline u32 scmi_agent_id(struct udevice *dev) > > +{ > > + return ((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->agent_id; > > +} > > Why these helper functions? It seems better to avoid inlining access > to dev_get_uclass_plat(). Are you worried about exposing the priv > struct to clients? Well, simply wanted to avoid repeating "((struct scmi_agent_priv *)dev_get_uclass_plat(dev))->)" in the code. Thanks, -Takahiro Akashi > > + > > /** > > * struct scmi_transport_ops - The functions that a SCMI transport layer must implement. > > */ > > -- > > 2.41.0 > > > > Regards, > Simon