From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 215B43C3BFE for ; Fri, 3 Apr 2026 14:17:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=205.220.180.131 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775225847; cv=none; b=oO1Sh666awQeVCmV53Pp5efpsU56K0j6EDNdMpJBlRO411Y+S7ZbkSopwMJxmG//zWsvI/YLZWAE810fKkcZ5nMYrES/6C9H/H0/Q5LcfRlp8kaJopS5NPYDwozaQxJdO/W0dWn0ANbfFMMLCUUWE3m0Y6jDF3Wf1POSGX0LRc0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775225847; c=relaxed/simple; bh=x1GMrzZmo29qt8wAIiATRsd4Ew8aTktyUtdtWzc8bns=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=hIp3KGVckFMnDXRuKU62zM6hxKqCsJ156edDHexOrT19UnAQwiaGyxSLbKMFPrUtiEK/rUewyZsgTLan520tc11GAIk8b+XJ4YPGs54w/5w7qMXKlhS822ZfWuQucS5iTevRq2ZVMTPtHN4g2+WHIeKZFiGhzIkLFwAyMEZRBmQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=oss.qualcomm.com; spf=pass smtp.mailfrom=oss.qualcomm.com; dkim=pass (2048-bit key) header.d=qualcomm.com header.i=@qualcomm.com header.b=JdVjTWHZ; dkim=pass (2048-bit key) header.d=oss.qualcomm.com header.i=@oss.qualcomm.com header.b=BH5ixiLn; arc=none smtp.client-ip=205.220.180.131 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=oss.qualcomm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=oss.qualcomm.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=qualcomm.com header.i=@qualcomm.com header.b="JdVjTWHZ"; dkim=pass (2048-bit key) header.d=oss.qualcomm.com header.i=@oss.qualcomm.com header.b="BH5ixiLn" Received: from pps.filterd (m0279872.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.18.1.11/8.18.1.11) with ESMTP id 6337UmD62707214 for ; Fri, 3 Apr 2026 14:17:25 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qualcomm.com; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to; s=qcppdkim1; bh= 5gzzElj7aj1o4ak21+gPFFPTZzAOuQGBCbltKvS4sOY=; b=JdVjTWHZhnX/UUTs US/THmy3mz2Xa8Y2GMeul+tYXJQVpgljkJE9SbpFpUb1v/n4a26CXodZsXV4morx f2jBz9GJ50hx+L7u9KYSkAKzUlDX0odKG2mQ5pAnv2P5UBInmtshHlL+8p3RhjnA TeZSHgE7QVhS/215Bu92zbv87M9mDFofgjcM7jOjXdgV9ULnhdHak5N3LhCGhjSc VV0xyQWlR+RhUkJOvtgPfmZvQiE9/2Ix2vdDB7u7+Wi8TMvk0wxXY+5YPCdzgSqj eFbaSlMCPNDFHADUmbTblqwcHGWvUzHkcwDRodCcbY9Gv7hbQvNgMpPnwwg3B0MR lNIN0w== Received: from mail-pl1-f200.google.com (mail-pl1-f200.google.com [209.85.214.200]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 4d9yfjjpxd-1 (version=TLSv1.3 cipher=TLS_AES_128_GCM_SHA256 bits=128 verify=NOT) for ; Fri, 03 Apr 2026 14:17:25 +0000 (GMT) Received: by mail-pl1-f200.google.com with SMTP id d9443c01a7336-2b242b9359aso18830265ad.0 for ; Fri, 03 Apr 2026 07:17:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oss.qualcomm.com; s=google; t=1775225844; x=1775830644; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=5gzzElj7aj1o4ak21+gPFFPTZzAOuQGBCbltKvS4sOY=; b=BH5ixiLncUHMnAHNIYIOuQoCUIH+KxoXKgsRSWPLYXMPJoA1GTmDAe8XsOqXawAayv IqWW44SqeLVvdx+WWzpYb/OXG9MGCvkewJWn0RSRALXtNj43RRYXaOOLA7YjGrw2NANT FLJDwx0jGhv022qsB7K/IRNXtUXSAzkKIxe9ekY38BTFRMlw/vFBOQSzPPZH/bf1ZTeP D0kl4ja/6ugYBe1pVZ121sIZktYuv/vIooFxVbqUE+cuQbDydWJVRMad7L1G3HI6y8E3 VXpV5mO/oCXS5IInLra/XfxWzROMCBA12H/pdFR+asye7WrfpUJZMayfANaizvIlWnz3 F2jw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1775225844; x=1775830644; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=5gzzElj7aj1o4ak21+gPFFPTZzAOuQGBCbltKvS4sOY=; b=Er2o5+B0dAcX/t+LdKHvAVJNCNl0YnIy7D8/1qlAlKULOSuuW2neY4XHh55fKr1MTi O5IcJHQmYkHLVg7rwBcsnVZrVJJTOUHmy9Lw+dzMD303sJIv6arVnVb1f6MK+z0oJfNK BOMtrXnAOCvXtCZmY90u3nwXEHsjzWtpqRioxUuA/rdeqFWD6JJBLsstz0vtP2/OxLTa s2EBFFMegxXFTjVH2LIRnUfd1DpxJNsefJ1mBIS2an0khctvd2TrFtp3PrbLdsX5Isx0 jffxHc1szRcx5IONLXLOd3jS9S9H9ubDn5PSaTQYbc/O5SDRkVlYV+6m9jdo6xn/jBEl 6R/Q== X-Forwarded-Encrypted: i=1; AJvYcCWP+WfTDQOnIxEJsu8pAscPGrJpRGKxgt1dGKaYIHC8G0nNS3Z+BsqB/Lwg0lAOLjzF+YwVQQ3RiMTJq6Q=@vger.kernel.org X-Gm-Message-State: AOJu0YwOcu2pKwsVEk/d8ehKVRTxPC2+1fUe/Cz84TN1P0JtUL32Qp32 HXZvLOFd8VWKqgWAO8H9+Mnq6Oexl343CsU2rC32glfBPpIMJUhrLr13EhENg8PAN61pmu5ua7w a+R8f/KUyAYgO975ff5a3mJy6XSnX28JbwZx4jImwiRwf1tVd9CiXXPbLuIRg57Xw37Q= X-Gm-Gg: AeBDietGd4dtj6ColiRt2kEi32vYFV5pxRUmAI1FuQRTFCUq7SRmBjOs3vHyUVSBPUu jJnj+zw/D8Fb84GTjfUEDnL7rDkergBRKTZgHfq4YB7+gj9gSMTL+ibE5wHhCyvhdYPkUee1t0u aGnQdayJ27OCt9UwM9PDWFcgvB3PQQTvyY/fsnKBlKof1/3z5FqgrN7G66vYaFliPlfI4NHuPyR RjjkDDum6O+Z30qHnvBMMKx0zyxm2/12tEDMt7vriOJmp0He/xJwZkGYo4jX3RH5V1CLkEngDRL cviZ8CN1V4Uj2u6ZFsaE6sriCyBF07xsApeL0A2XL1fN6syrI/QKr57hr3re6Dcb1fmFwEh9HYz NMfMB5l/q1qk1fVLAaoK2gBdmigSxBZScfRz41/inpGZKRONQCzAd6HoDYMw= X-Received: by 2002:a17:902:f552:b0:2b0:7d6f:4627 with SMTP id d9443c01a7336-2b2817ad6camr33018885ad.37.1775225843847; Fri, 03 Apr 2026 07:17:23 -0700 (PDT) X-Received: by 2002:a17:902:f552:b0:2b0:7d6f:4627 with SMTP id d9443c01a7336-2b2817ad6camr33018455ad.37.1775225843132; Fri, 03 Apr 2026 07:17:23 -0700 (PDT) Received: from hu-arakshit-hyd.qualcomm.com ([202.46.22.19]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-2b2749c38adsm75917555ad.69.2026.04.03.07.17.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 Apr 2026 07:17:22 -0700 (PDT) Date: Fri, 3 Apr 2026 19:47:15 +0530 From: Abhinaba Rakshit To: Harshal Dev Cc: Herbert Xu , "David S. Miller" , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Bjorn Andersson , Konrad Dybcio , Manivannan Sadhasivam , "James E.J. Bottomley" , "Martin K. Petersen" , Neeraj Soni , linux-arm-msm@vger.kernel.org, linux-crypto@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org Subject: Re: [PATCH v7 1/3] soc: qcom: ice: Add OPP-based clock scaling support for ICE Message-ID: References: <20260302-enable-ufs-ice-clock-scaling-v7-0-669b96ecadd8@oss.qualcomm.com> <20260302-enable-ufs-ice-clock-scaling-v7-1-669b96ecadd8@oss.qualcomm.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Proofpoint-GUID: J5i35DHeiUkYtnd61LnH94RiEf9-8Wg1 X-Proofpoint-Spam-Details-Enc: AW1haW4tMjYwNDAzMDEyNyBTYWx0ZWRfXyLAHYBYSjd3I a16TQkjwD2lqxC6oEfGVH4sxuo86odoOKmQg1Avf/FTydIjAB5u0BZ3rmu/V2RE0dIBIe43rxrI my8udMjRdbcuZF9WIS/k3wQ+O90L/KGuKcva/jGg2+Pdq38FremOVpN8Pwq9gEmUrvOMhHmBrXK WxrT8Ild3Dl0OtGs7Yp+gn1Ohd2xO5gjRpp0ahuQZeBjGrSTwMWxTKXDwrhavQzifZG3c04iDhG Wxg2pj4O5PQt0/GIKZWItAQQ+4e7tQ6fIpV7X79FnPTL2pi7v+RaCMdQHgx4yep0mb2gc5YAQz3 ++rhN+JRX26hwfwZ3CDjkp2p7juaArLBH2gd096JFdu0lG6lslRaMBIYj1/06mn5/tspY9TBwpw oPa9rzdvhpEFfXSj4dZ8UO94WnySp8tVlI77g8S38b3rJw6ka14vuS5Y5U7lkK21w0kUHAYlJNN tH5OZKjF6wy63adgFYA== X-Authority-Analysis: v=2.4 cv=OrpCCi/t c=1 sm=1 tr=0 ts=69cfcbf5 cx=c_pps a=IZJwPbhc+fLeJZngyXXI0A==:117 a=fChuTYTh2wq5r3m49p7fHw==:17 a=IkcTkHD0fZMA:10 a=A5OVakUREuEA:10 a=s4-Qcg_JpJYA:10 a=VkNPw1HP01LnGYTKEx00:22 a=u7WPNUs3qKkmUXheDGA7:22 a=yx91gb_oNiZeI1HMLzn7:22 a=VwQbUJbxAAAA:8 a=EUspDBNiAAAA:8 a=32UUs-Vz8nlSIkZsdJkA:9 a=3ZKOabzyN94A:10 a=QEXdDO2ut3YA:10 a=uG9DUKGECoFWVXl0Dc02:22 X-Proofpoint-ORIG-GUID: J5i35DHeiUkYtnd61LnH94RiEf9-8Wg1 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.293,Aquarius:18.0.1143,Hydra:6.1.51,FMLib:17.12.100.49 definitions=2026-04-03_04,2026-04-03_01,2025-10-01_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 malwarescore=0 adultscore=0 suspectscore=0 phishscore=0 lowpriorityscore=0 spamscore=0 bulkscore=0 clxscore=1015 priorityscore=1501 classifier=typeunknown authscore=0 authtc= authcc= route=outbound adjust=0 reason=mlx scancount=1 engine=8.22.0-2603050001 definitions=main-2604030127 On Mon, Mar 30, 2026 at 08:09:35PM +0530, Harshal Dev wrote: > > +/** > > + * qcom_ice_scale_clk() - Scale ICE clock for DVFS-aware operations > > + * @ice: ICE driver data > > + * @target_freq: requested frequency in Hz > > + * @round_ceil: when true, selects nearest freq >= @target_freq; > > + * otherwise, selects nearest freq <= @target_freq > > + * > > + * Selects an OPP frequency based on @target_freq and the rounding direction > > + * specified by @round_ceil, then programs it using dev_pm_opp_set_rate(), > > + * including any voltage or power-domain transitions handled by the OPP > > + * framework. Updates ice->core_clk_freq on success. > > + * > > + * Return: 0 on success; -EOPNOTSUPP if no OPP table; -EINVAL in-case of > > + * incorrect flags; or error from dev_pm_opp_set_rate()/OPP lookup. > > + */ > > +int qcom_ice_scale_clk(struct qcom_ice *ice, unsigned long target_freq, > > + bool round_ceil) > > Any particular reason for choosing round_ceil? Using round_floor would have > saved the need for caller to pass negation of scale_up. There isn’t a strong technical reason for choosing round_ceil specifically. The choice was mainly influenced by the earlier discussion here: https://lore.kernel.org/all/15495f8a-37b0-4768-9ee1-05fd6c70034e@oss.qualcomm.com/ Also, this helper isn’t necessarily limited to the current caller. We might see additional users in the future where the semantics align more naturally with flags like scale_down, which map cleanly to a round_ceil‑style selection. That said, I agree that using round_floor could simplify the current callsite by avoiding the negation of scale_up. I don’t have a strong objection to switching it if you feel that would be more cleaner for now. > > +{ > > + unsigned long ice_freq = target_freq; > > + struct dev_pm_opp *opp; > > + int ret; > > + > > + if (!ice->has_opp) > > + return -EOPNOTSUPP; > > + > > + if (round_ceil) > > + opp = dev_pm_opp_find_freq_ceil(ice->dev, &ice_freq); > > + else > > + opp = dev_pm_opp_find_freq_floor(ice->dev, &ice_freq); > > + > > + if (IS_ERR(opp)) > > + return PTR_ERR(opp); > > + dev_pm_opp_put(opp); > > + > > + ret = dev_pm_opp_set_rate(ice->dev, ice_freq); > > + if (!ret) > > + ice->core_clk_freq = ice_freq; > > Nit: Follow same error handling pattern everywhere in the driver. > if (ret) { > dev_err(dev, "error"); > return ret; > } Ack > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(qcom_ice_scale_clk); > > + > > static struct qcom_ice *qcom_ice_create(struct device *dev, > > - void __iomem *base) > > + void __iomem *base, > > + bool is_legacy_binding) > > You don't need to introduce is_legacy_binding. > > Since you only need to add the OPP table when this function gets called from ICE probe, > you should not touch this function. Instead, you should call devm_pm_opp_of_add_table() > in ICE probe before calling qcom_ice_create() then once qcom_ice_create() is success, you > can store the clk rate in the returned qcom_ice *engine ptr by calling clk_get_rate(). This was added as part of the review comment from Krzysztof: https://lore.kernel.org/all/20260128-daft-seriema-of-promotion-c50eb5@quoll/ While I agree moving this to qcom_ice_probe would be more cleaner without needing to change the API, most of our initializing code for driver by parsing the DT node happens through qcom_ice_create, which keeps qcom_ice_probe much simpler. Please let me know, if you think otherwise. Also, I don't see any reason for moving the clk_get_rate() logic to qcom_ice_probe though as it will not be set on legacy targets in that case. > > { > > struct qcom_ice *engine; > > + int err; > > > > if (!qcom_scm_is_available()) > > return ERR_PTR(-EPROBE_DEFER); > > @@ -584,6 +640,26 @@ static struct qcom_ice *qcom_ice_create(struct device *dev, > > if (IS_ERR(engine->core_clk)) > > return ERR_CAST(engine->core_clk); > > > > + /* > > + * Register the OPP table only when ICE is described as a standalone > > + * device node. Older platforms place ICE inside the storage controller > > + * node, so they don't need an OPP table here, as they are handled in > > + * storage controller. > > + */ > > + if (!is_legacy_binding) { > > + /* OPP table is optional */ > > + err = devm_pm_opp_of_add_table(dev); > > + if (err && err != -ENODEV) { > > + dev_err(dev, "Invalid OPP table in Device tree\n"); > > + return ERR_PTR(err); > > + } > > + engine->has_opp = (err == 0); > > Let's keep it readable and simple. engine->has_opps = true; here and false in error handle above. Well there are 3 cases to it: 1. err == 0 which implies devm_pm_opp_of_add_table is successful and we can set engine->has_opp =true. 2. err == -ENODEV which implies there is no opp table in the DT node. In that case, we don't fail the driver simply go ahead and log in the check below. This is done since OPP-table is optional. 3. err == any other error code. Something very wrong happened with devm_pm_opp_of_add_table and driver should fail. Hence, we have the condition (err == 0) for setting has_opp flag. > > + > > + if (!engine->has_opp) > > + dev_info(dev, "ICE OPP table is not registered, please update your DT\n"); > > Since OPP table is optional, I don't understand the reason for requesting the user to add one. This was added as part of the review comment from Konrad: https://lore.kernel.org/all/15495f8a-37b0-4768-9ee1-05fd6c70034e@oss.qualcomm.com/ OPP-table are mostly optional across kernel and I guess, this warning helps developers to go ahead and update with the OPP-table. Abhinaba Rakshit