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 83700C35274 for ; Thu, 21 Dec 2023 12:50:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: 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=zvNLJ9cVcz/8aeFKogj1I+hRzkMosFRJM++wTmyOQOo=; b=Mz4ldUJxf62vyK J2+delwjXZT7uOOIMMBXcvZxZ5/a+dzR1fctz7XGdGyOFKixz3eS3sAHH/y9UxSngzJa7yN5yKEZ2 Xyl4ZTT3br43uTpra7/GbXU5WnKaMY6+ZVvOQYccNpP2PIns7vF0ZBqPK4qYZxk2oDHmolAjNkER3 fy/TX29uxXarf7D/kM3Q8DPLJkJw6GUZFTHBFdpyNYDU62iQuVs8o+9zWtmovfAZcgs3TddUO/okn FQhTBp1sZoALC884Wqfb4RbXQ5Q8Vn+MREnL47pkRY1NATU6QQf3Hsh37hGCmlswR/eIUMuCJgI9M PXdkmAGZWRjTs33IjinA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rGIV9-002pv1-2S; Thu, 21 Dec 2023 12:50:11 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rGIV5-002psb-1U; Thu, 21 Dec 2023 12:50:09 +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 D90762F4; Thu, 21 Dec 2023 04:50:46 -0800 (PST) Received: from donnerap.manchester.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7D29D3F738; Thu, 21 Dec 2023 04:49:59 -0800 (PST) Date: Thu, 21 Dec 2023 12:49:57 +0000 From: Andre Przywara To: Brandon Cheo Fusi Cc: Rob Herring , Krzysztof Kozlowski , Conor Dooley , Paul Walmsley , Palmer Dabbelt , Albert Ou , Chen-Yu Tsai , Jernej Skrabec , Samuel Holland , Yangtao Li , "Rafael J . Wysocki" , Viresh Kumar , Stephen Rothwell , devicetree@vger.kernel.org, linux-riscv@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org Subject: Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding Message-ID: <20231221124957.27fa9922@donnerap.manchester.arm.com> In-Reply-To: <20231221101013.67204-3-fusibrandon13@gmail.com> References: <20231221101013.67204-1-fusibrandon13@gmail.com> <20231221101013.67204-3-fusibrandon13@gmail.com> Organization: ARM X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.32; aarch64-unknown-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231221_045007_612905_FCA298E4 X-CRM114-Status: GOOD ( 28.56 ) X-BeenThere: linux-riscv@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-riscv" Errors-To: linux-riscv-bounces+linux-riscv=archiver.kernel.org@lists.infradead.org On Thu, 21 Dec 2023 11:10:12 +0100 Brandon Cheo Fusi wrote: Hi Brandon, thanks for the quick turnaround, and for splitting this code up, that makes reasoning about this much easier! > Adds support for decoding the efuse value read from D1 efuse speed > bins, and factors out equivalent code for sun50i. > > The algorithm is gotten from > > https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338 > > and maps an efuse value to either 0 or 1, with 1 meaning stable at > a lower supply voltage for the same clock frequency. > > Signed-off-by: Brandon Cheo Fusi > --- > drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c > index fc509fc49..b1cb95308 100644 > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c > @@ -29,6 +29,33 @@ struct sunxi_cpufreq_data { > u32 (*efuse_xlate)(u32 *speedbin, size_t len); > }; > > +static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len) I feel like this prototype can be shortened to: static u32 sun20i_efuse_xlate(u32 speedbin) See below. > +{ > + u32 ret, efuse_value = 0; > + int i; > + > + for (i = 0; i < len; i++) > + efuse_value |= ((u32)speedbin[i] << (i * 8)); The cast is not needed. Looking deeper into the original code you linked to, cell_value[] there is an array of u8, so they assemble a little endian 32-bit integer from *up to* four 8-bit values read from the nvmem. So I think this code here is wrong, len is the size of the nvmem cells holding the bin identifier, in *bytes*, so the idea here is to just read the (lowest) 16 bits (in the D1 case, cf. "reg = <0x00 0x2>;" in the next patch) from this nvmem cell. Here you are combining two 32-bit words into efuse_value. So I think this whole part above is actually not necessary: we are expecting maximum 32 bits, and nvmem_cell_read() should take care of masking off unrequested bits, so we get the correct value back already. So can you try to remove the loop above, and use ... > + > + switch (efuse_value) { switch (*speedbin & 0xffff) { here instead? Or drop the pointer at all, and just use one u32 value, see the above prototype. Cheers, Andre P.S. This is just a "peephole review" of this patch, I haven't got around to look at this whole scheme in whole yet, to see if we actually need this or can simplify this or clean it up. > + case 0x5e00: > + /* QFN package */ > + ret = 0; > + break; > + case 0x5c00: > + case 0x7400: > + /* QFN package */ > + ret = 1; > + break; > + case 0x5000: > + default: > + /* BGA package */ > + ret = 0; > + } > + > + return ret; > +} > + > static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len) > { > u32 efuse_value = 0; > @@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len) > return 0; > } > > +struct sunxi_cpufreq_data sun20i_cpufreq_data = { > + .efuse_xlate = sun20i_efuse_xlate, > +}; > + > struct sunxi_cpufreq_data sun50i_cpufreq_data = { > .efuse_xlate = sun50i_efuse_xlate, > }; > @@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = { > { .compatible = "allwinner,sun50i-h6-operating-points", > .data = &sun50i_cpufreq_data, > }, > + { .compatible = "allwinner,sun20i-d1-operating-points", > + .data = &sun20i_cpufreq_data, > + }, > {} > }; > _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv