| Message ID | 20240823031059.32579-7-lihuisong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] soc: hisilicon: kunpeng_hccs: Fix a PCC typo | expand |
On Fri, 23 Aug 2024 11:10:59 +0800 Huisong Li <lihuisong@huawei.com> 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 <lihuisong@huawei.com> 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 <Jonathan.Cameron@huawei.com> 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 <lihuisong@huawei.com> > 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 <lihuisong@huawei.com> > +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)
Hi Jonathan, 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. Huisong 在 2024/8/23 16:58, Jonathan Cameron 写道: > On Fri, 23 Aug 2024 11:10:59 +0800 > Huisong Li <lihuisong@huawei.com> 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 <lihuisong@huawei.com> > 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 You are correct. > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > 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 <lihuisong@huawei.com> >> 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 <lihuisong@huawei.com> >> +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. Ack > >> + 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? It's ok. And then we can directly return failure if timeout. > >> + >> + 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? Yes > >> + 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. This is a good way. very nice and simple. But many sysfs interfaces in this driver have used mutex_lock/mutex_unlock. So is it better for us to keep the same mutex lock way in this patch and use guard() for all sysfs interface in furture patch? >> + 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? Ack > > >> +} >> +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. Ack > >> + 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) > .
On Tue, 27 Aug 2024 19:48:32 +0800 "lihuisong (C)" <lihuisong@huawei.com> wrote: > Hi Jonathan, > > 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 > > Huisong > > > 在 2024/8/23 16:58, Jonathan Cameron 写道: > > On Fri, 23 Aug 2024 11:10:59 +0800 > > Huisong Li <lihuisong@huawei.com> 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 <lihuisong@huawei.com> > > 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 > You are correct. > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > 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 <lihuisong@huawei.com> > >> 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 <lihuisong@huawei.com> > >> +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. > Ack > > > >> + 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? > > It's ok. And then we can directly return failure if timeout. > > > > >> + > >> + 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? > Yes > > > >> + 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. > This is a good way. very nice and simple. > But many sysfs interfaces in this driver have used mutex_lock/mutex_unlock. > So is it better for us to keep the same mutex lock way in this patch and > use guard() for all sysfs interface in furture patch? > >> + 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? > Ack > > > > > >> +} > >> +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. > Ack > > > >> + 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) > > .
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 <lihuisong@huawei.com> 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 <lihuisong@huawei.com> +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. + 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. + ============================= ==== ================================ diff --git a/drivers/soc/hisilicon/Kconfig b/drivers/soc/hisilicon/Kconfig index 4b0a099b28cc..6d7c244d2e78 100644 --- a/drivers/soc/hisilicon/Kconfig +++ b/drivers/soc/hisilicon/Kconfig @@ -13,9 +13,12 @@ config KUNPENG_HCCS interconnection bus protocol. The performance of application may be affected if some HCCS ports are not in full lane status, have a large number of CRC - errors and so on. + errors and so on. This may support for reducing system power + consumption if there are HCCS ports supported low power feature + on platform. Say M here if you want to include support for querying the - health status and port information of HCCS on Kunpeng SoC. + health status and port information of HCCS, or reducing system + power consumption on Kunpeng SoC. endmenu diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c index 623e7b7ed39a..4e9f8034e8ac 100644 --- a/drivers/soc/hisilicon/kunpeng_hccs.c +++ b/drivers/soc/hisilicon/kunpeng_hccs.c @@ -23,10 +23,18 @@ * - CRC error count sum * * - Retrieve all HCCS types used on the platform. + * + * - Support low power feature for all specified HCCS type ports, and + * provide the following interface: + * - query HCCS types supported increasing and decreasing lane number. + * - decrease lane of number all specified HCCS type ports on idle state. + * - increase lane number of all specified HCCS type ports. */ #include <linux/acpi.h> +#include <linux/delay.h> #include <linux/iopoll.h> #include <linux/platform_device.h> +#include <linux/stringify.h> #include <linux/sysfs.h> #include <linux/types.h> @@ -65,6 +73,33 @@ static struct hccs_dev *device_kobj_to_hccs_dev(struct kobject *k) return platform_get_drvdata(pdev); } +static char *hccs_port_type_to_name(struct hccs_dev *hdev, u8 type) +{ + u16 i; + + for (i = 0; i < hdev->used_type_num; i++) { + if (hdev->type_name_maps[i].type == type) + return hdev->type_name_maps[i].name; + } + + return NULL; +} + +static int hccs_name_to_port_type(struct hccs_dev *hdev, + const char *name, u8 *type) +{ + u16 i; + + for (i = 0; i < hdev->used_type_num; i++) { + if (strcmp(hdev->type_name_maps[i].name, name) == 0) { + *type = hdev->type_name_maps[i].type; + return 0; + } + } + + return -EINVAL; +} + struct hccs_register_ctx { struct device *dev; u8 chan_id; @@ -1195,6 +1230,309 @@ static const struct kobj_type hccs_chip_type = { .default_groups = hccs_chip_default_groups, }; +static int hccs_parse_pm_port_type(struct hccs_dev *hdev, const char *buf, + u8 *port_type) +{ + char hccs_name[HCCS_NAME_MAX_LEN + 1] = ""; + u8 type; + int ret; + + ret = sscanf(buf, "%" __stringify(HCCS_NAME_MAX_LEN) "s", hccs_name); + if (ret != 1) + return -EINVAL; + + ret = hccs_name_to_port_type(hdev, hccs_name, &type); + if (ret) { + dev_dbg(hdev->dev, "input invalid, please get the available types from 'used_types'.\n"); + return ret; + } + + if (type == HCCS_V2 && hdev->caps & HCCS_CAPS_HCCS_V2_PM) { + *port_type = type; + return 0; + } + + dev_dbg(hdev->dev, "%s doesn't support for increasing and decreasing lane.\n", + hccs_name); + + return -EOPNOTSUPP; +} + +static int hccs_query_port_idle_status(struct hccs_dev *hdev, + struct hccs_port_info *port, u8 *idle) +{ + const struct hccs_die_info *die = port->die; + const struct hccs_chip_info *chip = die->chip; + struct hccs_port_comm_req_param *req_param; + struct hccs_desc desc; + int ret; + + hccs_init_req_desc(&desc); + req_param = (struct hccs_port_comm_req_param *)desc.req.data; + req_param->chip_id = chip->chip_id; + req_param->die_id = die->die_id; + req_param->port_id = port->port_id; + ret = hccs_pcc_cmd_send(hdev, HCCS_GET_PORT_IDLE_STATUS, &desc); + if (ret) { + dev_err(hdev->dev, + "get port idle status failed, ret = %d.\n", ret); + return ret; + } + + *idle = *((u8 *)desc.rsp.data); + return 0; +} + +static int hccs_get_all_spec_port_idle_sta(struct hccs_dev *hdev, u8 port_type, + bool *all_idle) +{ + struct hccs_chip_info *chip; + struct hccs_port_info *port; + struct hccs_die_info *die; + int ret = 0; + u8 i, j, k; + u8 idle; + + *all_idle = false; + for (i = 0; i < hdev->chip_num; i++) { + chip = &hdev->chips[i]; + for (j = 0; j < chip->die_num; j++) { + die = &chip->dies[j]; + for (k = 0; k < die->port_num; k++) { + port = &die->ports[k]; + if (port->port_type != port_type) + continue; + ret = hccs_query_port_idle_status(hdev, port, + &idle); + if (ret) { + dev_err(hdev->dev, + "hccs%u on chip%u/die%u get idle status failed, ret = %d.\n", + k, i, j, ret); + return ret; + } else if (idle == 0) { + dev_info(hdev->dev, "hccs%u on chip%u/die%u is busy.\n", + k, i, j); + return 0; + } + } + } + } + *all_idle = true; + + return 0; +} + +static int hccs_get_all_spec_port_full_lane_sta(struct hccs_dev *hdev, + u8 port_type, bool *full_lane) +{ + struct hccs_link_status status = {0}; + struct hccs_chip_info *chip; + struct hccs_port_info *port; + struct hccs_die_info *die; + u8 i, j, k; + int ret; + + *full_lane = false; + for (i = 0; i < hdev->chip_num; i++) { + chip = &hdev->chips[i]; + for (j = 0; j < chip->die_num; j++) { + die = &chip->dies[j]; + for (k = 0; k < die->port_num; k++) { + port = &die->ports[k]; + if (port->port_type != port_type) + continue; + ret = hccs_query_port_link_status(hdev, port, + &status); + if (ret) + return ret; + if (status.lane_num != port->max_lane_num) + return 0; + } + } + } + *full_lane = true; + + return 0; +} + +static int hccs_prepare_inc_lane(struct hccs_dev *hdev, u8 type) +{ + struct hccs_inc_lane_req_param *req_param; + struct hccs_desc desc; + int ret; + + 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_PREPARE_INC_LANE; + ret = hccs_pcc_cmd_send(hdev, HCCS_PM_INC_LANE, &desc); + if (ret) + dev_err(hdev->dev, "prepare for increasing lane failed, ret = %d.\n", + ret); + + return ret; +} + +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; + + msleep(HCCS_QUERY_ADAPT_RES_DELAY_MS); + } while (--wait_cnt); + + if (adapt_res != HCCS_SERDES_ADAPT_OK) { + dev_err(hdev->dev, "wait for adapting completed timeout.\n"); + return -ETIMEDOUT; + } + + return ret; +} + +static int hccs_start_hpcs_retraining(struct hccs_dev *hdev, u8 type) +{ + struct hccs_inc_lane_req_param *req_param; + struct hccs_desc desc; + int ret; + + 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_START_RETRAINING; + ret = hccs_pcc_cmd_send(hdev, HCCS_PM_INC_LANE, &desc); + if (ret) + dev_err(hdev->dev, "start hpcs retraining failed, ret = %d.\n", + ret); + + return ret; +} + +static int hccs_start_inc_lane(struct hccs_dev *hdev, u8 type) +{ + int ret; + + ret = hccs_prepare_inc_lane(hdev, type); + if (ret) + return ret; + + ret = hccs_wait_serdes_adapt_completed(hdev, type); + if (ret) + return ret; + + return hccs_start_hpcs_retraining(hdev, type); +} + +static int hccs_start_dec_lane(struct hccs_dev *hdev, u8 type) +{ + struct hccs_desc desc; + u8 *port_type; + int ret; + + hccs_init_req_desc(&desc); + port_type = (u8 *)desc.req.data; + *port_type = type; + ret = hccs_pcc_cmd_send(hdev, HCCS_PM_DEC_LANE, &desc); + if (ret) + dev_err(hdev->dev, "start to decrease lane failed, ret = %d.\n", + ret); + + return ret; +} + +static ssize_t dec_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 all_in_idle; + u8 port_type; + int ret; + + ret = hccs_parse_pm_port_type(hdev, buf, &port_type); + if (ret) + return ret; + + mutex_lock(&hdev->lock); + ret = hccs_get_all_spec_port_idle_sta(hdev, port_type, &all_in_idle); + if (ret) + goto out; + if (!all_in_idle) { + ret = -EBUSY; + dev_err(hdev->dev, "please don't decrese lanes on high load with %s, ret = %d.\n", + hccs_port_type_to_name(hdev, port_type), ret); + goto out; + } + + ret = hccs_start_dec_lane(hdev, port_type); +out: + mutex_unlock(&hdev->lock); + + return ret == 0 ? count : ret; +} +static struct kobj_attribute dec_lane_of_type_attr = + __ATTR(dec_lane_of_type, 0200, NULL, dec_lane_of_type_store); + +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); + 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"); +} +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; + 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) diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h index 401df4694aec..dc267136919b 100644 --- a/drivers/soc/hisilicon/kunpeng_hccs.h +++ b/drivers/soc/hisilicon/kunpeng_hccs.h @@ -80,10 +80,13 @@ struct hccs_verspecific_data { bool has_txdone_irq; }; +#define HCCS_CAPS_HCCS_V2_PM BIT_ULL(0) + struct hccs_dev { struct device *dev; struct acpi_device *acpi_dev; const struct hccs_verspecific_data *verspec_data; + /* device capabilities from firmware, like HCCS_CAPS_xxx. */ u64 caps; u8 chip_num; struct hccs_chip_info *chips; @@ -106,6 +109,9 @@ enum hccs_subcmd_type { HCCS_GET_DIE_PORTS_LANE_STA, HCCS_GET_DIE_PORTS_LINK_STA, HCCS_GET_DIE_PORTS_CRC_ERR_CNT, + HCCS_GET_PORT_IDLE_STATUS, + HCCS_PM_DEC_LANE, + HCCS_PM_INC_LANE, HCCS_SUB_CMD_MAX = 255, }; @@ -149,6 +155,14 @@ struct hccs_port_comm_req_param { u8 port_id; }; +#define HCCS_PREPARE_INC_LANE 1 +#define HCCS_GET_ADAPT_RES 2 +#define HCCS_START_RETRAINING 3 +struct hccs_inc_lane_req_param { + u8 port_type; + u8 opt_type; +}; + #define HCCS_PORT_RESET 1 #define HCCS_PORT_SETUP 2 #define HCCS_PORT_CONFIG 3
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 <lihuisong@huawei.com> --- .../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(-)