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 2B64EC52D7C for ; Fri, 23 Aug 2024 09:10:24 +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=pjBDefQq+sc5t/SsGmGPajafAwyMkWLaE6cD1XhrbEE=; b=XqVNUhoXFbWiOgV1jaf8VdFX1f PfcVHgDmpqjV8Z1QN8H/4eLmfm+j8v/d/XIqmSX1/OB6+NLAFBtZQgbNu6tV8Bz6yq2HBI6GEB60n ZckbQ5CpaOVE8X3pjCC0GWGgcP83OfWvmiRtuK7Bn+TVfNn6BujZ5TX+vcfzvikXnyYRPmIbI2L/k qxJsOJtAtdB+WMv9fPI7DYa70lB9WR0Z1gqsYMqhXmS3Pv7j8kB8/7vO86eaFXg/T4NdayGMrfFwX 5zUWU1OFm7TNuDmA+S12r8PMo2QVlfqL+PJ1mHQEf/C4NsFj4mWeFHeLl15Hvwjdr1EtKvhR0bYrT UB5TM3Pg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1shQJ7-0000000G2FV-1ghK; Fri, 23 Aug 2024 09:10:09 +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 1shQ8K-0000000FzWq-0kst for linux-arm-kernel@lists.infradead.org; Fri, 23 Aug 2024 08:59:02 +0000 Received: from mail.maildlp.com (unknown [172.18.186.31]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4Wqv6423sfz6F98r; Fri, 23 Aug 2024 16:55:08 +0800 (CST) Received: from lhrpeml500005.china.huawei.com (unknown [7.191.163.240]) by mail.maildlp.com (Postfix) with ESMTPS id D7B3914065B; Fri, 23 Aug 2024 16:58:52 +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; Fri, 23 Aug 2024 09:58:52 +0100 Date: Fri, 23 Aug 2024 09:58:51 +0100 From: Jonathan Cameron To: Huisong Li CC: , , , , , , Subject: Re: [PATCH v2 6/6] soc: hisilicon: kunpeng_hccs: Support low power feature for the specified HCCS type Message-ID: <20240823095851.0000004e@Huawei.com> In-Reply-To: <20240823031059.32579-7-lihuisong@huawei.com> References: <20240718071134.31155-1-lihuisong@huawei.com> <20240823031059.32579-1-lihuisong@huawei.com> <20240823031059.32579-7-lihuisong@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="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.203.177.66] X-ClientProxiedBy: lhrpeml500003.china.huawei.com (7.191.162.67) 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-20240823_015900_763972_BA1F6161 X-CRM114-Status: GOOD ( 34.10 ) 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 Fri, 23 Aug 2024 11:10:59 +0800 Huisong Li wrote: > 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 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 Reviewed-by: Jonathan Cameron The early return comment and whitespace suggestion are things you can act on if you liek for v2. Jonathan > --- > .../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: > + > + ============================= ==== ================================ > + available_inc_dec_lane_types: (RO) available HCCS types (string) to > + increase and decrease the number > + of lane used, e.g. HCCS-v2. 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. > + 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. > + ============================= ==== ================================ > +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 = HCCS_MAX_WAIT_CNT_FOR_ADAPT; > + struct hccs_desc desc; > + u8 adapt_res; > + int ret; > + > + do { > + hccs_init_req_desc(&desc); > + req_param = (struct hccs_inc_lane_req_param *)desc.req.data; > + req_param->port_type = type; > + req_param->opt_type = HCCS_GET_ADAPT_RES; > + ret = hccs_pcc_cmd_send(hdev, HCCS_PM_INC_LANE, &desc); > + if (ret) { > + dev_err(hdev->dev, "query adapting result failed, ret = %d.\n", > + ret); > + return ret; > + } > + adapt_res = *((u8 *)&desc.rsp.data); > + if (adapt_res == HCCS_SERDES_ADAPT_OK) > + break; return 0; here perhaps? > + > + msleep(HCCS_QUERY_ADAPT_RES_DELAY_MS); > + } while (--wait_cnt); > + > + if (adapt_res != HCCS_SERDES_ADAPT_OK) { With above early exit in good path, this can be unconditional perhaps? > + 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 kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + struct hccs_dev *hdev = device_kobj_to_hccs_dev(kobj); > + bool full_lane; > + u8 port_type; > + int ret; > + > + ret = hccs_parse_pm_port_type(hdev, buf, &port_type); > + if (ret) > + return ret; > + > + mutex_lock(&hdev->lock); Another comment for a future patch series perhaps. guard(mutex)(&hdev->lock); in all these will make the code quite a bit cleaner. > + ret = hccs_get_all_spec_port_full_lane_sta(hdev, port_type, &full_lane); > + if (ret || full_lane) > + goto out; > + > + ret = hccs_start_inc_lane(hdev, port_type); > +out: > + mutex_unlock(&hdev->lock); > + return ret == 0 ? count : ret; > +} > +static struct kobj_attribute inc_lane_of_type_attr = > + __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 = 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"); 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? > +} > +static struct kobj_attribute available_inc_dec_lane_types_attr = > + __ATTR(available_inc_dec_lane_types, 0444, > + available_inc_dec_lane_types_show, NULL); > > 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 = > 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); > } > > static int hccs_add_misc_sysfs(struct hccs_dev *hdev) > { > - return sysfs_create_file(&hdev->dev->kobj, &used_types_attr.attr); > + int ret; > + > + ret = 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 = sysfs_create_file(&hdev->dev->kobj, > + &available_inc_dec_lane_types_attr.attr); > + if (ret) > + goto used_types_remove; > + > + ret = sysfs_create_file(&hdev->dev->kobj, &dec_lane_of_type_attr.attr); > + if (ret) > + goto inc_dec_lane_types_remove; 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. > + ret = sysfs_create_file(&hdev->dev->kobj, &inc_lane_of_type_attr.attr); > + 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; > } > > static void hccs_remove_die_dir(struct hccs_die_info *die)