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 7AE5E34AAF7 for ; Tue, 7 Apr 2026 23:45:39 +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=1775605540; cv=none; b=TRybz6c3E8xge9zw9+zTyxC9l2POQyHN0VuRRVG/YagGR3xz0RPXozZEH8TAwdAfb6WoNYZHW73k1Ti9yTXvpH/pLi7NNNyRViGz5IUEp8xFovUiO4hEoq738maPuVtm1wvFlnYYCPps2deO/OSf5d0P4/R6wzzLcTUckcsgnvQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1775605540; c=relaxed/simple; bh=5RiwWyfTRmY+ICHwSz2ehVlihQE4jCeKt9H823nu7G0=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=KblZsj8Jmd9cbDtgb0NOxNuLM/vZvvZNC3p1qUjH2P/vLtaqY0pC0f20F78BmeRXBZcgBqpbr4J3BUGsCqE0JBG78cphn61G78TQ0rjtzeDLL3FD9wJKZ35a2UA7hFEyevqgDX1N+kNiuVldBHeYii7Kpu+sMQKDm7zoh/ggoSo= 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=FgJGOP/F; dkim=pass (2048-bit key) header.d=oss.qualcomm.com header.i=@oss.qualcomm.com header.b=BUq0QN6N; 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="FgJGOP/F"; dkim=pass (2048-bit key) header.d=oss.qualcomm.com header.i=@oss.qualcomm.com header.b="BUq0QN6N" 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 637M3JQ01619682 for ; Tue, 7 Apr 2026 23:45:38 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=qualcomm.com; h= cc:content-type:date:from:in-reply-to:message-id:mime-version :references:subject:to; s=qcppdkim1; bh=dqnafUrvON7HZA8zHAS3PedH DWzr2LNEiDtUvABj9vA=; b=FgJGOP/FAFMePg582rzGfkWLseQ3HPEe2HxcpqK0 CZAp+I6li1p/cEviB05ZplP1eggT5wEd72FQ+V2HU/20KPuzOmr37kDdiuEKNybu ERepe6f0imAUDZMGgNH5LCXxfXffA+fj94vYPby7H4K+fv46wHHoItMtCu3zotTA RZMxYP77GGgnM8D3e+K4BOCdNfpW1uaYKLMKh6X0z96Q9DjTlfxNoy6awkHo0mr/ /5+uQVLVYjJ2FSrwk4TZXmbVNUVfn9LNzm2Ca3yQ02RbPjDbQrK8kov642ZUKSQi 8FGwLxiIZ9ozPzI8xypEmP+yDbDs0KDiF3PYKXokeKNVQw== Received: from mail-pf1-f198.google.com (mail-pf1-f198.google.com [209.85.210.198]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 4ddacrg7vn-1 (version=TLSv1.3 cipher=TLS_AES_128_GCM_SHA256 bits=128 verify=NOT) for ; Tue, 07 Apr 2026 23:45:38 +0000 (GMT) Received: by mail-pf1-f198.google.com with SMTP id d2e1a72fcca58-82a6906e35fso289599b3a.0 for ; Tue, 07 Apr 2026 16:45:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oss.qualcomm.com; s=google; t=1775605537; x=1776210337; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=dqnafUrvON7HZA8zHAS3PedHDWzr2LNEiDtUvABj9vA=; b=BUq0QN6NGNaEtOFTJ60Z3IUYXKMxqmv8B3BnqPIToIu18AVm61ALy30H5JgXlo9F0M WyKYtjh8Md4n5WJgJz3WrDMaxg/YyL+ozjhN6DtBu2LL/nt5aRwYDKtei8+Kgnkh/Px3 yI5QTd6XNK8KX+K287wT/dXt/yuNHE0TFd+3zsclFjVlIoyjPlnKVuqNx/uduBvIIiGn gRf9Vy+sLKJIwznybmSOz5qUE9Up3fBhFtHKnM19ydjHHaTu2iDG57a0aXrTJEiiy1lB n3/odQmaKtCJTFw3VdVFEVZYuF7Tluyz4XTLbelshqVDVZbfCIam97IU8PqmLr1u5Qci C1Qw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1775605537; x=1776210337; h=in-reply-to: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=dqnafUrvON7HZA8zHAS3PedHDWzr2LNEiDtUvABj9vA=; b=XZdrK+mT3o4OyImQmpvEEuqq5yKSfwU1uoDsS1VeuA4JclHJ+tBlHS6UXDLFqPeg+v GdhZXc9n8eaxuLeYtJ6k2eKBKIU+ht1VNJQ0i7v1nz2RtgSF/srfSRCaJaoBBVYx4Vym rl0jZCJZwoJz45dIXzbs2QNbdekQgK9dKyYLcTV2K44gAasxRe1jaBogeByN9v5TTp63 FHnId0BuMGezpDrdb/rvnC8JukFjbdzPHWiiZjcsrPOr02CxUPFIpMzHysk/BUre8UHh p9l7YPms1NKJLL162NNriwwcjDpnWMFT7av/6kgjYMGKmJlBGFMoz73hRmepvnvjiOYC h0eQ== X-Forwarded-Encrypted: i=1; AJvYcCU1WqUgAzASMckJj6AMGX0ktSUde3R+ID8/f+cXQSz7ZVo32GT60UQ57SX/1UNo3AROP+8VljdDsTMihrQ=@vger.kernel.org X-Gm-Message-State: AOJu0YxuM53CO59J8hXtN/9Sgp6DHZSWGs55yrqCzitmTfd50TsyDAx3 g/Hyn6qheWr63PdLTFc49eNiQ+CV8eFD+6UnRVzpwIvxpSDjoWUc7deprKIYGcDkyDydSLycS/T 1uwJsj1s+oYldvlrXnCSmxmXu2sDrpysNPZrMuDKPF96x0amKL7d++yQQLiwdy5Rn5pY= X-Gm-Gg: AeBDiet3SsiQtPB+m0bMjfcve2YiPqtSlV127qUTMxnJUi8bFkoWcp4A/vXQriAl/d7 EOnUqrml6E6c6/OdzYTZBLdHR2lCnaCbrGq0zdiAYloJY/xKSrkI1hNdyyL7Nz5xpFnrsRAKr5+ 8NR0rv2oq4z5x/OCO6Lz2gWw0LDdqntrFjBGWjEnCoO28Yd7bwle8TWVW3QXGefI+Egb5an4AVm 5z/iO8Lnh8FLwHESpUdweHu8hZ3K8bW1pfyKcrovdzX4E+yguWLt2we/EgVS4nLs7QiMy1901Zs S7tyihxeRZi9nq4/ojlgktmWIycnWEdAOm+aDyg3p78EzlW0/d9hM7Gg7aHMHLgsnR6lrlmBWhj ngiUpJZiP6LidHVwpCwksdlqYPsbcmNJ/VaUumVT2qauybKu/Z7Jppya2cjE= X-Received: by 2002:a05:6a00:896:b0:827:26b6:c11f with SMTP id d2e1a72fcca58-82d003b275fmr19984158b3a.31.1775605537111; Tue, 07 Apr 2026 16:45:37 -0700 (PDT) X-Received: by 2002:a05:6a00:896:b0:827:26b6:c11f with SMTP id d2e1a72fcca58-82d003b275fmr19984129b3a.31.1775605536479; Tue, 07 Apr 2026 16:45:36 -0700 (PDT) Received: from hu-arakshit-hyd.qualcomm.com ([202.46.22.19]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-82cf9b3baffsm18939943b3a.14.2026.04.07.16.45.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Apr 2026 16:45:36 -0700 (PDT) Date: Wed, 8 Apr 2026 05:15:28 +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> <0375e235-0b8a-458c-a797-d5b341dc60b9@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=us-ascii Content-Disposition: inline In-Reply-To: <0375e235-0b8a-458c-a797-d5b341dc60b9@oss.qualcomm.com> X-Proofpoint-ORIG-GUID: 2WGUm_J1ccdvMsrR2fXBUnTkA7o4G4z7 X-Proofpoint-GUID: 2WGUm_J1ccdvMsrR2fXBUnTkA7o4G4z7 X-Authority-Analysis: v=2.4 cv=WZs8rUhX c=1 sm=1 tr=0 ts=69d59722 cx=c_pps a=m5Vt/hrsBiPMCU0y4gIsQw==:117 a=fChuTYTh2wq5r3m49p7fHw==:17 a=kj9zAlcOel0A:10 a=A5OVakUREuEA:10 a=s4-Qcg_JpJYA:10 a=VkNPw1HP01LnGYTKEx00:22 a=u7WPNUs3qKkmUXheDGA7:22 a=yx91gb_oNiZeI1HMLzn7:22 a=VwQbUJbxAAAA:8 a=CkLZM6L_lappvOHulz4A:9 a=CjuIK1q_8ugA:10 a=IoOABgeZipijB_acs4fv:22 X-Proofpoint-Spam-Details-Enc: AW1haW4tMjYwNDA3MDIxNyBTYWx0ZWRfX03HbRZqb0Zh4 niwuWxjiwPSp4GwaDH93PB3pVg+pJvOgtxy49HTs/ljecgxU3bVWu/5XUN7+QJDOtZkIavXNVom imYtFlOSbOUxsCUAw9FjFB8X8jBrYuwpLR0eNAQ0QRjX+ALSB2N62Qr0EIt7wKv0ljaFeVWz8Vi 4PqInkrGEOGY1vfBj/KoFoPBRETgl9Opa1fGYy7mjppLwB1ajCc/zxYBHnrNnaV2qplqJfZll+c LhuCWX6HUrQtSAgG50IIJC+mkoUxiYfCP0Awy21k3uZl2riGzVkSGzhCml7pbTeqGrIcGu7oT1y WeWyLlFN+nireAMsG07sPil+GT+O6mYfR0/77FSpJefksnYM6lq5pCeS7aJuj/BFZULUTq+eFr9 5fbZHmj26H18c9vW82pO2s0YiPWsLD3mNOJ8TDkYJ3RBwLaWGvlRJIzgLmf3cU0VoJEOZizDMiB FbkVUeeKLqTH/XPgNeA== 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-07_05,2026-04-07_05,2025-10-01_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=0 malwarescore=0 adultscore=0 phishscore=0 clxscore=1015 bulkscore=0 lowpriorityscore=0 spamscore=0 priorityscore=1501 impostorscore=0 classifier=typeunknown authscore=0 authtc= authcc= route=outbound adjust=0 reason=mlx scancount=1 engine=8.22.0-2604010000 definitions=main-2604070217 On Fri, Apr 03, 2026 at 10:50:29PM +0530, Harshal Dev wrote: > >>> +{ > >>> + unsigned long ice_freq = target_freq; > >>> + struct dev_pm_opp *opp; > >>> + int ret; > >>> + > >>> + if (!ice->has_opp) > >>> + return -EOPNOTSUPP; > >>> + > > [...] > > >>> + > >>> 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. > > > > Seems like a suggestion from Krzysztof and not something based on strong opinion. Again, > you can choose to do this if you spin a v8, I feel it's cleaner. This comment raises an important design point around whether the OPP table should be registered in qcom_ice_create versus qcom_ice_probe. I agree that moving OPP-table registration to qcom_ice_probe would be a cleaner and more maintainable approach. Doing so avoids the need to distinguish between legacy and non-legacy bindings at the API level, and keeps qcom_ice_create reusable for both cases. This also aligns well with the intent of qcom_ice_create, which should focus purely on common/basic hardware initialization. Additionally, qcom_ice_create currently has no dependency on the OPP table being registered beforehand. Clock scaling is a performance optimization and does not impose a hard requirement for ICE operation or enablement. >From that perspective, there is no technical necessity for OPP registration to occur within qcom_ice_create. Ack, will update it in v8 patchset. > > 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. > > I thought only new DT nodes will be specifying the OPP table requiring us to store the > clk rate and restore later. If legacy DT nodes also possess the OPP table, then ignore > this comment. No, clk_rate is not needed for legacy bindings. It is only needed for DVFS operation across suspend resume cycles. Hence, its value makes sense only for non-legacy bindings. It shoud not be faked for legacy bindings as clk rates can also be scaled from storage driver in-case of legacy bindings. Ack, will update in patchset v8. > > > >>> { > >>> 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. > > My suggestion is you either explain this in concise comments or simplify the assignment of has_opp > to make it obvious. Sure, will add appropriate comment here. Abhinaba Rakshit