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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 4F7BFC021A2 for ; Mon, 10 Feb 2025 12:17:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type:MIME-Version:References:Message-ID: Subject:Cc: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=f87nlZVEzgjO19B1BV0yen9Sdb/bQrUoVpDE+DzExuc=; b=oB3MSdHIzqULdAsUrCCFd78qG/ KOfFf8wXZv9dBMnTsypRJ+WppO7rNH+Oaa4CwoSb65/XzlwAhpBtt3sebd9Dp2/VLun8QuYUaOVtB 7nRdU25rYqiTUky247eLdYh06fB79XE0hGHMdiZhhQOCl5Cc0bX6j7wkkQrVj6lBQElt/pRR3Xufi tF6iJv0qj/LiDOszfGfse3kHfPnN1Fha1ItAeE+5tBMxybdXL1geyXggBYUo3J40mUFVCZQBn+YxF IRuOtwNt5GSTZsYbN+03VHBdv7L2GLz6sIt2Nq26LlecsC3QUadN5p6V7WxXZmQcLfun0fhTEknoR kilUnSHw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1thSjP-0000000HN6t-0jks; Mon, 10 Feb 2025 12:17:43 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1thSfT-0000000HMIi-1xlb for linux-arm-kernel@lists.infradead.org; Mon, 10 Feb 2025 12:13:40 +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 3A07012FC; Mon, 10 Feb 2025 04:13:58 -0800 (PST) Received: from bogus (e133711.arm.com [10.1.196.55]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 430773F5A1; Mon, 10 Feb 2025 04:13:35 -0800 (PST) Date: Mon, 10 Feb 2025 12:13:32 +0000 From: Sudeep Holla To: Paul Benoit Cc: linux-kernel@vger.kernel.org, Mark Rutland , Sudeep Holla , Lorenzo Pieralisi , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3] firmware: smccc: Support optional Arm SMC SOC_ID name Message-ID: References: <20241114030452.10149-1-paul@os.amperecomputing.com> <20241218001338.6247-1-paul@os.amperecomputing.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20241218001338.6247-1-paul@os.amperecomputing.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250210_041339_597644_2EE5BE43 X-CRM114-Status: GOOD ( 33.02 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Mostly minor coding style comments from me, otherwise LGTM. On Tue, Dec 17, 2024 at 04:13:38PM -0800, Paul Benoit wrote: Split the commit into multiple paragraphs, it looks too crowded 😄. > Issue Number 1.6 of the Arm SMC Calling Convention introduces an > optional SOC_ID name string. > If available, point the 'machine' field of ^^^^ I prefer implemented instead of available. > the SoC Device Attributes at this string so that it will appear under > /sys/bus/soc/devices/soc0/machine. Break into new paragraph here. > On Arm SMC compliant SoCs, this will > allow things like 'lscpu' to eventually get a SoC provider model name > from there rather than each tool/utility needing to get a possibly > inconsistent, obsolete, or incorrect model/machine name from its own > hardcoded model/machine name table. > > Signed-off-by: Paul Benoit > Cc: Mark Rutland > Cc: Lorenzo Pieralisi > Cc: Sudeep Holla > Cc: linux-arm-kernel@lists.infradead.org > --- > > v2->v3: Add conditionalization to exclude SOC_ID Name from 32-bit builds. > v1->v2: Address code review identified issues. > > drivers/firmware/smccc/soc_id.c | 79 +++++++++++++++++++++++++++++++++ > include/linux/arm-smccc.h | 37 +++++++++++++++ > 2 files changed, 116 insertions(+) > > diff --git a/drivers/firmware/smccc/soc_id.c b/drivers/firmware/smccc/soc_id.c > index 1990263fbba0..3b50ff5d2cbd 100644 > --- a/drivers/firmware/smccc/soc_id.c > +++ b/drivers/firmware/smccc/soc_id.c > @@ -32,6 +32,12 @@ > static struct soc_device *soc_dev; > static struct soc_device_attribute *soc_dev_attr; > > +static char __init *smccc_soc_name_init(void); > + Not really needed if you move you code before smccc_soc_init() > +#ifdef CONFIG_ARM64 > +static char __ro_after_init smccc_soc_id_name[136] = ""; Move all in one block under #ifdef, details below. > +#endif > + > static int __init smccc_soc_init(void) > { > int soc_id_rev, soc_id_version; > @@ -72,6 +78,7 @@ static int __init smccc_soc_init(void) > soc_dev_attr->soc_id = soc_id_str; > soc_dev_attr->revision = soc_id_rev_str; > soc_dev_attr->family = soc_id_jep106_id_str; > + soc_dev_attr->machine = smccc_soc_name_init(); > > soc_dev = soc_device_register(soc_dev_attr); > if (IS_ERR(soc_dev)) { > @@ -93,3 +100,75 @@ static void __exit smccc_soc_exit(void) > kfree(soc_dev_attr); > } > module_exit(smccc_soc_exit); Generally it good to have module_{init,exit} at the end of the file. Move you additions above these. > + > + > +#ifdef CONFIG_ARM64 > +static inline void str_fragment_from_reg(char *dst, unsigned long reg) > +{ > + dst[0] = (reg >> 0) & 0xff; > + dst[1] = (reg >> 8) & 0xff; > + dst[2] = (reg >> 16) & 0xff; > + dst[3] = (reg >> 24) & 0xff; > + dst[4] = (reg >> 32) & 0xff; > + dst[5] = (reg >> 40) & 0xff; > + dst[6] = (reg >> 48) & 0xff; > + dst[7] = (reg >> 56) & 0xff; > +} > +#endif > + > +static char __init *smccc_soc_name_init(void) > +{ > +#ifdef CONFIG_ARM64 > + struct arm_smccc_1_2_regs args; > + struct arm_smccc_1_2_regs res; > + size_t len; > + > + /* > + * Issue Number 1.6 of the Arm SMC Calling Convention > + * specification introduces an optional "name" string > + * to the ARM_SMCCC_ARCH_SOC_ID function. Fetch it if > + * available. > + */ > + args.a0 = ARM_SMCCC_ARCH_SOC_ID; > + args.a1 = 2; /* SOC_ID name */ > + arm_smccc_1_2_invoke(&args, &res); > + if ((u32)res.a0 == 0) { > + const unsigned int regsize = sizeof(res.a1); > + > + /* > + * Copy res.a1..res.a17 to the smccc_soc_id_name string > + * 8 bytes at a time. As per Issue 1.6 of the Arm SMC > + * Calling Convention, the string will be NUL terminated > + * and padded, from the end of the string to the end of the > + * 136 byte buffer, with NULs. > + */ > + str_fragment_from_reg(smccc_soc_id_name + 0*regsize, res.a1); > + str_fragment_from_reg(smccc_soc_id_name + 1*regsize, res.a2); > + str_fragment_from_reg(smccc_soc_id_name + 2*regsize, res.a3); > + str_fragment_from_reg(smccc_soc_id_name + 3*regsize, res.a4); > + str_fragment_from_reg(smccc_soc_id_name + 4*regsize, res.a5); > + str_fragment_from_reg(smccc_soc_id_name + 5*regsize, res.a6); > + str_fragment_from_reg(smccc_soc_id_name + 6*regsize, res.a7); > + str_fragment_from_reg(smccc_soc_id_name + 7*regsize, res.a8); > + str_fragment_from_reg(smccc_soc_id_name + 8*regsize, res.a9); > + str_fragment_from_reg(smccc_soc_id_name + 9*regsize, res.a10); > + str_fragment_from_reg(smccc_soc_id_name + 10*regsize, res.a11); > + str_fragment_from_reg(smccc_soc_id_name + 11*regsize, res.a12); > + str_fragment_from_reg(smccc_soc_id_name + 12*regsize, res.a13); > + str_fragment_from_reg(smccc_soc_id_name + 13*regsize, res.a14); > + str_fragment_from_reg(smccc_soc_id_name + 14*regsize, res.a15); > + str_fragment_from_reg(smccc_soc_id_name + 15*regsize, res.a16); > + str_fragment_from_reg(smccc_soc_id_name + 16*regsize, res.a17); > + > + len = strnlen(smccc_soc_id_name, sizeof(smccc_soc_id_name)); > + if (len) { > + if (len == sizeof(smccc_soc_id_name)) > + pr_warn(FW_BUG "Ignoring improperly formatted Name\n"); > + else > + return smccc_soc_id_name; > + } > + } > +#endif > + > + return NULL; > +} Can we improve readability with #ifdef CONFIG_ARM64 static char __ro_after_init smccc_soc_id_name[136] = ""; #else static char __init *smccc_soc_name_init(void) { return NULL; } #endif > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h > index 67f6fdf2e7cd..9d444e5862fe 100644 > --- a/include/linux/arm-smccc.h > +++ b/include/linux/arm-smccc.h > @@ -607,6 +607,12 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1, > ___res->a0 = SMCCC_RET_NOT_SUPPORTED; \ > } while (0) > > +#define __fail_smccc_1_2(___res) \ > + do { \ > + if (___res) \ > + ___res->a0 = SMCCC_RET_NOT_SUPPORTED; \ > + } while (0) > + > /* > * arm_smccc_1_1_invoke() - make an SMCCC v1.1 compliant call > * > @@ -639,5 +645,36 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1, > method; \ > }) > > +/* > + * arm_smccc_1_2_invoke() - make an SMCCC v1.2 compliant call > + * > + * @args: SMC args are in the a0..a17 fields of the arm_smcc_1_2_regs structure > + * @res: result values from registers 0 to 17 > + * > + * This macro will make either an HVC call or an SMC call depending on the > + * current SMCCC conduit. If no valid conduit is available then -1 > + * (SMCCC_RET_NOT_SUPPORTED) is returned in @res.a0 (if supplied). > + * > + * The return value also provides the conduit that was used. > + */ > +#define arm_smccc_1_2_invoke(args, res) ({ \ > + struct arm_smccc_1_2_regs *__args = args; \ I think we can move this macro and the above under CONFIG_ARM64 as arm_smccc_1_2_regs is defined only for ARM64 for now. Otherwise one could use this macro and get undefined compiler errors for the structure. -- Regards, Sudeep