Message-ID: <20240823095851.0000004e@Huawei.com>
Date: Fri, 23 Aug 2024 09:58:51 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Huisong Li <lihuisong@...wei.com>
CC: <xuwei5@...ilicon.com>, <linux-kernel@...r.kernel.org>, <soc@...nel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <krzk@...nel.org>,
	<wanghuiqiang@...wei.com>, <liuyonglong@...wei.com>
Subject: Re: [PATCH v2 6/6] soc: hisilicon: kunpeng_hccs: Support low power
 feature for the specified HCCS type

On Fri, 23 Aug 2024 11:10:59 +0800
Huisong Li <lihuisong@...wei.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@...wei.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@...wei.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@...wei.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@...wei.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)

Powered by blists - more mailing lists