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 D609DC54735 for ; Tue, 27 Aug 2024 12:00:30 +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:Content-Transfer-Encoding: Content-Type: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=HonaJuulVOy/fSyJwOjBmQ9bTGe52EInaR/O7vx3JFM=; b=WV38v15UjfiHz/ZCrlsFcLTMsy 8LPGfkIy8A8WwZwBwYR+ea8WlG6UYY5ZVxqCJphY5tdPzr71dMagWZLPOYNJ7y0rCGrHMRrieiWrR hR35PXfzQTRfELV/TLj30XJcXdn9uopUBiEffaU4yMDn+FH7UN1PBPutZ4QJmBST7Gyi0T2tyUywV NrMyDN1MxT9M54/28Ar5W2wIq55GSWWNvq0F5McInPAy1n0+bZZEPQEqY/fl1cO6c8iqU9zIDKvqM +fhMFr9gTtWpSqo08mlA3qRKYwrrfpP6EJndW40q/5bt8BX3qndPMIxG/8sz1mCgCGaUUK1/bpYkz PCnpDgbg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1siurs-0000000B7sj-2RIf; Tue, 27 Aug 2024 12:00:12 +0000 Received: from frasgout.his.huawei.com ([185.176.79.56]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1siur2-0000000B7gN-1z1h for linux-arm-kernel@lists.infradead.org; Tue, 27 Aug 2024 11:59:22 +0000 Received: from mail.maildlp.com (unknown [172.18.186.216]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4WtQw143fmz6J7Zj; Tue, 27 Aug 2024 19:55:13 +0800 (CST) Received: from lhrpeml500005.china.huawei.com (unknown [7.191.163.240]) by mail.maildlp.com (Postfix) with ESMTPS id 9C2AE140B39; Tue, 27 Aug 2024 19:59:11 +0800 (CST) Received: from localhost (10.203.177.66) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Tue, 27 Aug 2024 12:59:11 +0100 Date: Tue, 27 Aug 2024 12:59:10 +0100 From: Jonathan Cameron To: "lihuisong (C)" CC: , , , , , , Subject: Re: [PATCH v2 6/6] soc: hisilicon: kunpeng_hccs: Support low power feature for the specified HCCS type Message-ID: <20240827125910.00007cdd@Huawei.com> In-Reply-To: References: <20240718071134.31155-1-lihuisong@huawei.com> <20240823031059.32579-1-lihuisong@huawei.com> <20240823031059.32579-7-lihuisong@huawei.com> <20240823095851.0000004e@Huawei.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Originating-IP: [10.203.177.66] X-ClientProxiedBy: lhrpeml100004.china.huawei.com (7.191.162.219) To lhrpeml500005.china.huawei.com (7.191.163.240) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240827_045920_826599_B7BDA5D5 X-CRM114-Status: GOOD ( 47.13 ) 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 On Tue, 27 Aug 2024 19:48:32 +0800 "lihuisong (C)" wrote: > Hi Jonathan, >=20 > Thanks for your review again. > Your proposal are good and are also more worth to enhance code. > How about use guard() for all sysfs interface in furture patch? > I want to support this feature first. Sure, that's fine and why I gave an RB tag even with comments. Thanks, Jonathan >=20 > Huisong >=20 >=20 > =E5=9C=A8 2024/8/23 16:58, Jonathan Cameron =E5=86=99=E9=81=93: > > On Fri, 23 Aug 2024 11:10:59 +0800 > > Huisong Li wrote: > > =20 > >> Add the low power feature for the specified HCCS type by increasing > >> and decreasing the used lane number of these HCCS ports on platform. > >> > >> Signed-off-by: Huisong Li =20 > > Hi Huisong, > > > > A few comments inline, but all minor things. > > > > With at least the "none" string print dropped as it's in an error path > > that shouldn't be hit you can add =20 > You are correct. > > Reviewed-by: Jonathan Cameron > > > > The early return comment and whitespace suggestion are things you > > can act on if you liek for v2. > > > > Jonathan > > =20 > >> --- > >> .../sysfs-devices-platform-kunpeng_hccs | 37 ++ > >> drivers/soc/hisilicon/Kconfig | 7 +- > >> drivers/soc/hisilicon/kunpeng_hccs.c | 378 ++++++++++++++++= +- > >> drivers/soc/hisilicon/kunpeng_hccs.h | 14 + > >> 4 files changed, 433 insertions(+), 3 deletions(-) > >> > >> diff --git a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_= hccs b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs > >> index d4c355e0e0bb..d1b3a95a5518 100644 > >> --- a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs > >> +++ b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs > >> @@ -87,3 +87,40 @@ Contact: Huisong Li > >> Description: > >> This interface is used to show all HCCS types used on the > >> platform, like, HCCS-v1, HCCS-v2 and so on. > >> + > >> +What: /sys/devices/platform/HISI04Bx:00/available_inc_dec_lane_types > >> +What: /sys/devices/platform/HISI04Bx:00/dec_lane_of_type > >> +What: /sys/devices/platform/HISI04Bx:00/inc_lane_of_type > >> +Date: August 2024 > >> +KernelVersion: 6.12 > >> +Contact: Huisong Li > >> +Description: > >> + These interfaces under /sys/devices/platform/HISI04Bx/ are > >> + used to support the low power consumption feature of some > >> + HCCS types by changing the number of lanes used. The interfaces > >> + changing the number of lanes used are 'dec_lane_of_type' and > >> + 'inc_lane_of_type' which require root privileges. These > >> + interfaces aren't exposed if no HCCS type on platform support > >> + this feature. Please note that decreasing lane number is only > >> + allowed if all the specified HCCS ports are not busy. > >> + > >> + The low power consumption interfaces are as follows: > >> + > >> + =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > >> + available_inc_dec_lane_types: (RO) available HCCS types (string) to > >> + increase and decrease the number > >> + of lane used, e.g. HCCS-v2. =20 > > See below. There is an apparent value of 'none' available, but I think = in reality the > > interface doesn't exist if that is present. So drop it as it will just = cause confusion. =20 > Ack > > =20 > >> + dec_lane_of_type: (WO) input HCCS type supported > >> + decreasing lane to decrease the > >> + used lane number of all specified > >> + HCCS type ports on platform to > >> + the minimum. > >> + You can query the 'cur_lane_num' > >> + to get the minimum lane number > >> + after executing successfully. > >> + inc_lane_of_type: (WO) input HCCS type supported > >> + increasing lane to increase the > >> + used lane number of all specified > >> + HCCS type ports on platform to > >> + the full lane state. > >> + =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D =3D=3D=3D=3D =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > >> +static int hccs_wait_serdes_adapt_completed(struct hccs_dev *hdev, u8= type) > >> +{ > >> +#define HCCS_MAX_WAIT_CNT_FOR_ADAPT 10 > >> +#define HCCS_QUERY_ADAPT_RES_DELAY_MS 100 > >> +#define HCCS_SERDES_ADAPT_OK 0 > >> + > >> + struct hccs_inc_lane_req_param *req_param; > >> + u8 wait_cnt =3D HCCS_MAX_WAIT_CNT_FOR_ADAPT; > >> + struct hccs_desc desc; > >> + u8 adapt_res; > >> + int ret; > >> + > >> + do { > >> + hccs_init_req_desc(&desc); > >> + req_param =3D (struct hccs_inc_lane_req_param *)desc.req.data; > >> + req_param->port_type =3D type; > >> + req_param->opt_type =3D HCCS_GET_ADAPT_RES; > >> + ret =3D hccs_pcc_cmd_send(hdev, HCCS_PM_INC_LANE, &desc); > >> + if (ret) { > >> + dev_err(hdev->dev, "query adapting result failed, ret =3D %d.\n", > >> + ret); > >> + return ret; > >> + } > >> + adapt_res =3D *((u8 *)&desc.rsp.data); > >> + if (adapt_res =3D=3D HCCS_SERDES_ADAPT_OK) > >> + break; =20 > > return 0; here perhaps? =20 >=20 > It's ok. And then we can directly return failure if timeout. >=20 > > =20 > >> + > >> + msleep(HCCS_QUERY_ADAPT_RES_DELAY_MS); > >> + } while (--wait_cnt); > >> + > >> + if (adapt_res !=3D HCCS_SERDES_ADAPT_OK) { =20 > > With above early exit in good path, this can be unconditional perhaps? = =20 > Yes > > =20 > >> + dev_err(hdev->dev, "wait for adapting completed timeout.\n"); > >> + return -ETIMEDOUT; > >> + } > >> + > >> + return ret; > >> +} > >> +static ssize_t inc_lane_of_type_store(struct kobject *kobj, struct ko= bj_attribute *attr, > >> + const char *buf, size_t count) > >> +{ > >> + struct hccs_dev *hdev =3D device_kobj_to_hccs_dev(kobj); > >> + bool full_lane; > >> + u8 port_type; > >> + int ret; > >> + > >> + ret =3D hccs_parse_pm_port_type(hdev, buf, &port_type); > >> + if (ret) > >> + return ret; > >> + > >> + mutex_lock(&hdev->lock); =20 > > Another comment for a future patch series perhaps. > > > > guard(mutex)(&hdev->lock); in all these will make the code quite a bit = cleaner. =20 > This is a good way. very nice and simple. > But many sysfs interfaces in this driver have used mutex_lock/mutex_unloc= k. > So is it better for us to keep the same mutex lock way in this patch and= =20 > use guard() for all sysfs interface in furture patch? > >> + ret =3D hccs_get_all_spec_port_full_lane_sta(hdev, port_type, &full_= lane); > >> + if (ret || full_lane) > >> + goto out; > >> + > >> + ret =3D hccs_start_inc_lane(hdev, port_type); > >> +out: > >> + mutex_unlock(&hdev->lock); > >> + return ret =3D=3D 0 ? count : ret; > >> +} > >> +static struct kobj_attribute inc_lane_of_type_attr =3D > >> + __ATTR(inc_lane_of_type, 0200, NULL, inc_lane_of_type_store); > >> + > >> +static ssize_t available_inc_dec_lane_types_show(struct kobject *kobj, > >> + struct kobj_attribute *attr, > >> + char *buf) > >> +{ > >> + struct hccs_dev *hdev =3D device_kobj_to_hccs_dev(kobj); > >> + > >> + if (hdev->caps & HCCS_CAPS_HCCS_V2_PM) > >> + return sysfs_emit(buf, "%s\n", > >> + hccs_port_type_to_name(hdev, HCCS_V2)); > >> + > >> + return sysfs_emit(buf, "%s\n", "none"); =20 > > Can we get here? I thought this was only registered if the condition > > above is true? > > > > Maybe worth keeping a fallback here as a code hardening measure, but > > perhaps return -EINVAL; is fine? =20 > Ack > > > > =20 > >> +} > >> +static struct kobj_attribute available_inc_dec_lane_types_attr =3D > >> + __ATTR(available_inc_dec_lane_types, 0444, > >> + available_inc_dec_lane_types_show, NULL); > >> =20 > >> static ssize_t used_types_show(struct kobject *kobj, > >> struct kobj_attribute *attr, char *buf) > >> @@ -1215,11 +1553,49 @@ static struct kobj_attribute used_types_attr = =3D > >> static void hccs_remove_misc_sysfs(struct hccs_dev *hdev) > >> { > >> sysfs_remove_file(&hdev->dev->kobj, &used_types_attr.attr); > >> + > >> + if (!(hdev->caps & HCCS_CAPS_HCCS_V2_PM)) > >> + return; > >> + > >> + sysfs_remove_file(&hdev->dev->kobj, > >> + &available_inc_dec_lane_types_attr.attr); > >> + sysfs_remove_file(&hdev->dev->kobj, &dec_lane_of_type_attr.attr); > >> + sysfs_remove_file(&hdev->dev->kobj, &inc_lane_of_type_attr.attr); > >> } > >> =20 > >> static int hccs_add_misc_sysfs(struct hccs_dev *hdev) > >> { > >> - return sysfs_create_file(&hdev->dev->kobj, &used_types_attr.attr); > >> + int ret; > >> + > >> + ret =3D sysfs_create_file(&hdev->dev->kobj, &used_types_attr.attr); > >> + if (ret) > >> + return ret; > >> + > >> + if (!(hdev->caps & HCCS_CAPS_HCCS_V2_PM)) > >> + return 0; > >> + > >> + ret =3D sysfs_create_file(&hdev->dev->kobj, > >> + &available_inc_dec_lane_types_attr.attr); > >> + if (ret) > >> + goto used_types_remove; > >> + > >> + ret =3D sysfs_create_file(&hdev->dev->kobj, &dec_lane_of_type_attr.a= ttr); > >> + if (ret) > >> + goto inc_dec_lane_types_remove; =20 > > I can sort of see why no line break makes some sense here given these > > two files are closely related, but I'd still add one here as I think > > visual consistency is more important for readability reasons. =20 > Ack > > =20 > >> + ret =3D sysfs_create_file(&hdev->dev->kobj, &inc_lane_of_type_attr.a= ttr); > >> + if (ret) > >> + goto dec_lane_of_type_remove; > >> + > >> + return 0; > >> + > >> +dec_lane_of_type_remove: > >> + sysfs_remove_file(&hdev->dev->kobj, &dec_lane_of_type_attr.attr); > >> +inc_dec_lane_types_remove: > >> + sysfs_remove_file(&hdev->dev->kobj, > >> + &available_inc_dec_lane_types_attr.attr); > >> +used_types_remove: > >> + sysfs_remove_file(&hdev->dev->kobj, &used_types_attr.attr); > >> + return ret; > >> } > >> =20 > >> static void hccs_remove_die_dir(struct hccs_die_info *die) =20 > > . =20