.../sysfs-devices-platform-kunpeng_hccs | 26 + drivers/soc/hisilicon/Kconfig | 7 +- drivers/soc/hisilicon/kunpeng_hccs.c | 489 +++++++++++++++++- drivers/soc/hisilicon/kunpeng_hccs.h | 29 ++ 4 files changed, 545 insertions(+), 6 deletions(-)
This series are intended to support the low power feature for specified
HCCS and add used HCCS type sysfs. In addition, fix a PCC typo and add the
verification for die and port number.
Huisong Li (5):
soc: hisilicon: kunpeng_hccs: fix a PCC typo
soc: hisilicon: kunpeng_hccs: return failure on having not die or port
information
soc: hisilicon: kunpeng_hccs: add used HCCS type sysfs on platform
soc: hisilicon: kunpeng_hccs: support low power feature for specified
HCCS
doc: soc: hisilicon: kunpeng_hccs: add low power interface description
for HCCS
.../sysfs-devices-platform-kunpeng_hccs | 26 +
drivers/soc/hisilicon/Kconfig | 7 +-
drivers/soc/hisilicon/kunpeng_hccs.c | 489 +++++++++++++++++-
drivers/soc/hisilicon/kunpeng_hccs.h | 29 ++
4 files changed, 545 insertions(+), 6 deletions(-)
--
2.22.0
This series are intended to support the low power feature for specified
HCCS and add used HCCS types sysfs. In addition, fix some bugfix and
enhance some codes.
---
v2:
- remove "this patch" words in commit log suggested by Krzyszto.
- use for_each_set_bit to replace the cycle scanning all HCCS IP.
- add a patch to rename the 'lane_mode' to 'max_lane_num' to make it
easy to see.
- add doc description into the code patch.
- rename the name of the low power interface.
- adjust the increasing and decreasing lane interface description.
- do not create available_inc_dec_lane_types when no HCCS type support
low power.
---
Huisong Li (6):
soc: hisilicon: kunpeng_hccs: Fix a PCC typo
soc: hisilicon: kunpeng_hccs: Return failure on having not die or port
information
soc: hisilicon: kunpeng_hccs: Add the check for base address and size
of shared memory
soc: hisilicon: kunpeng_hccs: Fix the 'lane_mode' field name in port
info structure to 'max_lane_num'
soc: hisilicon: kunpeng_hccs: Add used HCCS types sysfs
soc: hisilicon: kunpeng_hccs: Support low power feature for the
specified HCCS type
.../sysfs-devices-platform-kunpeng_hccs | 45 ++
drivers/soc/hisilicon/Kconfig | 7 +-
drivers/soc/hisilicon/kunpeng_hccs.c | 516 +++++++++++++++++-
drivers/soc/hisilicon/kunpeng_hccs.h | 33 +-
4 files changed, 582 insertions(+), 19 deletions(-)
--
2.22.0
On Fri, 23 Aug 2024 11:10:53 +0800 Huisong Li <lihuisong@huawei.com> wrote: > This series are intended to support the low power feature for specified > HCCS and add used HCCS types sysfs. In addition, fix some bugfix and > enhance some codes. Quick process thing. Don't send a v2 like this in reply to v1 (for most kernel subsystems anyway, maybe there are some that request this?). Most people still review in email clients and they often start at latest and work back until they run out of time. Thus a reply to an earlier thread is not read! Jonathan > > --- > v2: > - remove "this patch" words in commit log suggested by Krzyszto. > - use for_each_set_bit to replace the cycle scanning all HCCS IP. > - add a patch to rename the 'lane_mode' to 'max_lane_num' to make it > easy to see. > - add doc description into the code patch. > - rename the name of the low power interface. > - adjust the increasing and decreasing lane interface description. > - do not create available_inc_dec_lane_types when no HCCS type support > low power. > --- > > Huisong Li (6): > soc: hisilicon: kunpeng_hccs: Fix a PCC typo > soc: hisilicon: kunpeng_hccs: Return failure on having not die or port > information > soc: hisilicon: kunpeng_hccs: Add the check for base address and size > of shared memory > soc: hisilicon: kunpeng_hccs: Fix the 'lane_mode' field name in port > info structure to 'max_lane_num' > soc: hisilicon: kunpeng_hccs: Add used HCCS types sysfs > soc: hisilicon: kunpeng_hccs: Support low power feature for the > specified HCCS type > > .../sysfs-devices-platform-kunpeng_hccs | 45 ++ > drivers/soc/hisilicon/Kconfig | 7 +- > drivers/soc/hisilicon/kunpeng_hccs.c | 516 +++++++++++++++++- > drivers/soc/hisilicon/kunpeng_hccs.h | 33 +- > 4 files changed, 582 insertions(+), 19 deletions(-) >
在 2024/8/23 17:02, Jonathan Cameron 写道: > On Fri, 23 Aug 2024 11:10:53 +0800 > Huisong Li <lihuisong@huawei.com> wrote: > >> This series are intended to support the low power feature for specified >> HCCS and add used HCCS types sysfs. In addition, fix some bugfix and >> enhance some codes. > Quick process thing. Don't send a v2 like this in reply to v1 > (for most kernel subsystems anyway, maybe there are some that request > this?). > > Most people still review in email clients and they often start at latest > and work back until they run out of time. Thus a reply to an earlier > thread is not read! Got it. Thanks for suggestion. > > Jonathan > >> --- >> v2: >> - remove "this patch" words in commit log suggested by Krzyszto. >> - use for_each_set_bit to replace the cycle scanning all HCCS IP. >> - add a patch to rename the 'lane_mode' to 'max_lane_num' to make it >> easy to see. >> - add doc description into the code patch. >> - rename the name of the low power interface. >> - adjust the increasing and decreasing lane interface description. >> - do not create available_inc_dec_lane_types when no HCCS type support >> low power. >> --- >> >> Huisong Li (6): >> soc: hisilicon: kunpeng_hccs: Fix a PCC typo >> soc: hisilicon: kunpeng_hccs: Return failure on having not die or port >> information >> soc: hisilicon: kunpeng_hccs: Add the check for base address and size >> of shared memory >> soc: hisilicon: kunpeng_hccs: Fix the 'lane_mode' field name in port >> info structure to 'max_lane_num' >> soc: hisilicon: kunpeng_hccs: Add used HCCS types sysfs >> soc: hisilicon: kunpeng_hccs: Support low power feature for the >> specified HCCS type >> >> .../sysfs-devices-platform-kunpeng_hccs | 45 ++ >> drivers/soc/hisilicon/Kconfig | 7 +- >> drivers/soc/hisilicon/kunpeng_hccs.c | 516 +++++++++++++++++- >> drivers/soc/hisilicon/kunpeng_hccs.h | 33 +- >> 4 files changed, 582 insertions(+), 19 deletions(-) >> > .
Fix a PCC typo.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/soc/hisilicon/kunpeng_hccs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index e882a61636ec..c4a57328f22a 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -144,7 +144,7 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
pcc_chan = pcc_mbox_request_channel(cl, hdev->chan_id);
if (IS_ERR(pcc_chan)) {
- dev_err(dev, "PPC channel request failed.\n");
+ dev_err(dev, "PCC channel request failed.\n");
rc = -ENODEV;
goto out;
}
--
2.22.0On Fri, 23 Aug 2024 11:10:54 +0800
Huisong Li <lihuisong@huawei.com> wrote:
> Fix a PCC typo.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
FWIW.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/soc/hisilicon/kunpeng_hccs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
> index e882a61636ec..c4a57328f22a 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
> @@ -144,7 +144,7 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
>
> pcc_chan = pcc_mbox_request_channel(cl, hdev->chan_id);
> if (IS_ERR(pcc_chan)) {
> - dev_err(dev, "PPC channel request failed.\n");
> + dev_err(dev, "PCC channel request failed.\n");
> rc = -ENODEV;
> goto out;
> }
Driver is unavailable if all die number or all port number obtained from
firmware are zero. So return failure in this case.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/soc/hisilicon/kunpeng_hccs.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index c4a57328f22a..6e88f597f267 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -451,6 +451,7 @@ static int hccs_query_all_die_info_on_platform(struct hccs_dev *hdev)
struct device *dev = hdev->dev;
struct hccs_chip_info *chip;
struct hccs_die_info *die;
+ bool has_die_info = false;
u8 i, j;
int ret;
@@ -459,6 +460,7 @@ static int hccs_query_all_die_info_on_platform(struct hccs_dev *hdev)
if (!chip->die_num)
continue;
+ has_die_info = true;
chip->dies = devm_kzalloc(hdev->dev,
chip->die_num * sizeof(struct hccs_die_info),
GFP_KERNEL);
@@ -480,7 +482,7 @@ static int hccs_query_all_die_info_on_platform(struct hccs_dev *hdev)
}
}
- return 0;
+ return has_die_info ? 0 : -EINVAL;
}
static int hccs_get_bd_info(struct hccs_dev *hdev, u8 opcode,
@@ -601,6 +603,7 @@ static int hccs_query_all_port_info_on_platform(struct hccs_dev *hdev)
struct device *dev = hdev->dev;
struct hccs_chip_info *chip;
struct hccs_die_info *die;
+ bool has_port_info = false;
u8 i, j;
int ret;
@@ -611,6 +614,7 @@ static int hccs_query_all_port_info_on_platform(struct hccs_dev *hdev)
if (!die->port_num)
continue;
+ has_port_info = true;
die->ports = devm_kzalloc(dev,
die->port_num * sizeof(struct hccs_port_info),
GFP_KERNEL);
@@ -629,7 +633,7 @@ static int hccs_query_all_port_info_on_platform(struct hccs_dev *hdev)
}
}
- return 0;
+ return has_port_info ? 0 : -EINVAL;
}
static int hccs_get_hw_info(struct hccs_dev *hdev)
--
2.22.0On Fri, 23 Aug 2024 11:10:55 +0800 Huisong Li <lihuisong@huawei.com> wrote: > Driver is unavailable if all die number or all port number obtained from > firmware are zero. So return failure in this case. Perhaps should include a little info on whether there are firmware's out there that do this or not? I.e. Fix, or hardening? > > Signed-off-by: Huisong Li <lihuisong@huawei.com> Otherwise, this lgtm. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/soc/hisilicon/kunpeng_hccs.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c > index c4a57328f22a..6e88f597f267 100644 > --- a/drivers/soc/hisilicon/kunpeng_hccs.c > +++ b/drivers/soc/hisilicon/kunpeng_hccs.c > @@ -451,6 +451,7 @@ static int hccs_query_all_die_info_on_platform(struct hccs_dev *hdev) > struct device *dev = hdev->dev; > struct hccs_chip_info *chip; > struct hccs_die_info *die; > + bool has_die_info = false; > u8 i, j; > int ret; > > @@ -459,6 +460,7 @@ static int hccs_query_all_die_info_on_platform(struct hccs_dev *hdev) > if (!chip->die_num) > continue; > > + has_die_info = true; > chip->dies = devm_kzalloc(hdev->dev, > chip->die_num * sizeof(struct hccs_die_info), > GFP_KERNEL); > @@ -480,7 +482,7 @@ static int hccs_query_all_die_info_on_platform(struct hccs_dev *hdev) > } > } > > - return 0; > + return has_die_info ? 0 : -EINVAL; > } > > static int hccs_get_bd_info(struct hccs_dev *hdev, u8 opcode, > @@ -601,6 +603,7 @@ static int hccs_query_all_port_info_on_platform(struct hccs_dev *hdev) > struct device *dev = hdev->dev; > struct hccs_chip_info *chip; > struct hccs_die_info *die; > + bool has_port_info = false; > u8 i, j; > int ret; > > @@ -611,6 +614,7 @@ static int hccs_query_all_port_info_on_platform(struct hccs_dev *hdev) > if (!die->port_num) > continue; > > + has_port_info = true; > die->ports = devm_kzalloc(dev, > die->port_num * sizeof(struct hccs_port_info), > GFP_KERNEL); > @@ -629,7 +633,7 @@ static int hccs_query_all_port_info_on_platform(struct hccs_dev *hdev) > } > } > > - return 0; > + return has_port_info ? 0 : -EINVAL; > } > > static int hccs_get_hw_info(struct hccs_dev *hdev)
在 2024/8/23 16:33, Jonathan Cameron 写道: > On Fri, 23 Aug 2024 11:10:55 +0800 > Huisong Li <lihuisong@huawei.com> wrote: > >> Driver is unavailable if all die number or all port number obtained from >> firmware are zero. So return failure in this case. > Perhaps should include a little info on whether there are firmware's out > there that do this or not? I.e. Fix, or hardening? Ack just hardening code. > >> Signed-off-by: Huisong Li <lihuisong@huawei.com> > Otherwise, this lgtm. > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >> --- >> drivers/soc/hisilicon/kunpeng_hccs.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c >> index c4a57328f22a..6e88f597f267 100644 >> --- a/drivers/soc/hisilicon/kunpeng_hccs.c >> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c >> @@ -451,6 +451,7 @@ static int hccs_query_all_die_info_on_platform(struct hccs_dev *hdev) >> struct device *dev = hdev->dev; >> struct hccs_chip_info *chip; >> struct hccs_die_info *die; >> + bool has_die_info = false; >> u8 i, j; >> int ret; >> >> @@ -459,6 +460,7 @@ static int hccs_query_all_die_info_on_platform(struct hccs_dev *hdev) >> if (!chip->die_num) >> continue; >> >> + has_die_info = true; >> chip->dies = devm_kzalloc(hdev->dev, >> chip->die_num * sizeof(struct hccs_die_info), >> GFP_KERNEL); >> @@ -480,7 +482,7 @@ static int hccs_query_all_die_info_on_platform(struct hccs_dev *hdev) >> } >> } >> >> - return 0; >> + return has_die_info ? 0 : -EINVAL; >> } >> >> static int hccs_get_bd_info(struct hccs_dev *hdev, u8 opcode, >> @@ -601,6 +603,7 @@ static int hccs_query_all_port_info_on_platform(struct hccs_dev *hdev) >> struct device *dev = hdev->dev; >> struct hccs_chip_info *chip; >> struct hccs_die_info *die; >> + bool has_port_info = false; >> u8 i, j; >> int ret; >> >> @@ -611,6 +614,7 @@ static int hccs_query_all_port_info_on_platform(struct hccs_dev *hdev) >> if (!die->port_num) >> continue; >> >> + has_port_info = true; >> die->ports = devm_kzalloc(dev, >> die->port_num * sizeof(struct hccs_port_info), >> GFP_KERNEL); >> @@ -629,7 +633,7 @@ static int hccs_query_all_port_info_on_platform(struct hccs_dev *hdev) >> } >> } >> >> - return 0; >> + return has_port_info ? 0 : -EINVAL; >> } >> >> static int hccs_get_hw_info(struct hccs_dev *hdev) > .
If the shmem_base_addr from PCCT is zero, hccs_register_pcc_channel will
return success. And then driver will access to illegal address when send
PCC command. In addition, the size of shared memory used for communication
between driver and platform is fixed, namely 64 Bytes which is
unchangeable. So add the verification for them.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/soc/hisilicon/kunpeng_hccs.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index 6e88f597f267..6055e5091cbd 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -170,15 +170,21 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
goto err_mbx_channel_free;
}
- if (pcc_chan->shmem_base_addr) {
- cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr,
- pcc_chan->shmem_size);
- if (!cl_info->pcc_comm_addr) {
- dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n",
- hdev->chan_id);
- rc = -ENOMEM;
- goto err_mbx_channel_free;
- }
+ if (!pcc_chan->shmem_base_addr ||
+ pcc_chan->shmem_size != HCCS_PCC_SHARE_MEM_BYTES) {
+ dev_err(dev, "The base address or size (%llu) of PCC communication region is invalid.\n",
+ pcc_chan->shmem_size);
+ rc = -EINVAL;
+ goto err_mbx_channel_free;
+ }
+
+ cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr,
+ pcc_chan->shmem_size);
+ if (!cl_info->pcc_comm_addr) {
+ dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n",
+ hdev->chan_id);
+ rc = -ENOMEM;
+ goto err_mbx_channel_free;
}
return 0;
--
2.22.0On Fri, 23 Aug 2024 11:10:56 +0800
Huisong Li <lihuisong@huawei.com> wrote:
> If the shmem_base_addr from PCCT is zero, hccs_register_pcc_channel will
> return success. And then driver will access to illegal address when send
> PCC command. In addition, the size of shared memory used for communication
> between driver and platform is fixed, namely 64 Bytes which is
> unchangeable. So add the verification for them.
>
As with previous, make it clear if this hardening or fix
fix to catch a problem on shipping hardware (I assume not, but you never
know!)
A comment on existing code inline. Not a suggestion for a change
in this series, but maybe for the future. There are a lot
of goto err_mbx_channel_Free in here already and this patch adds
another. The cleanup there is trivial so DEFINE_FREE() and __free
usage will make this code quite a bit nicer to read.
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/soc/hisilicon/kunpeng_hccs.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
> index 6e88f597f267..6055e5091cbd 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
> @@ -170,15 +170,21 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
> goto err_mbx_channel_free;
> }
>
> - if (pcc_chan->shmem_base_addr) {
> - cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr,
> - pcc_chan->shmem_size);
> - if (!cl_info->pcc_comm_addr) {
> - dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n",
> - hdev->chan_id);
> - rc = -ENOMEM;
> - goto err_mbx_channel_free;
> - }
> + if (!pcc_chan->shmem_base_addr ||
> + pcc_chan->shmem_size != HCCS_PCC_SHARE_MEM_BYTES) {
> + dev_err(dev, "The base address or size (%llu) of PCC communication region is invalid.\n",
> + pcc_chan->shmem_size);
> + rc = -EINVAL;
> + goto err_mbx_channel_free;
Worth considering for the future: Maybe a DEFINE_FREE for pcc_mbox_free_channel) makes sense,
though if you do you should only assign cl_info->pcc_chan after all possible error paths.
> + }
> +
> + cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr,
> + pcc_chan->shmem_size);
> + if (!cl_info->pcc_comm_addr) {
> + dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n",
> + hdev->chan_id);
> + rc = -ENOMEM;
> + goto err_mbx_channel_free;
> }
>
> return 0;
在 2024/8/23 16:38, Jonathan Cameron 写道:
> On Fri, 23 Aug 2024 11:10:56 +0800
> Huisong Li <lihuisong@huawei.com> wrote:
>
>> If the shmem_base_addr from PCCT is zero, hccs_register_pcc_channel will
>> return success. And then driver will access to illegal address when send
>> PCC command. In addition, the size of shared memory used for communication
>> between driver and platform is fixed, namely 64 Bytes which is
>> unchangeable. So add the verification for them.
>>
> As with previous, make it clear if this hardening or fix
> fix to catch a problem on shipping hardware (I assume not, but you never
> know!)
Ack
>
> A comment on existing code inline. Not a suggestion for a change
> in this series, but maybe for the future. There are a lot
> of goto err_mbx_channel_Free in here already and this patch adds
> another. The cleanup there is trivial so DEFINE_FREE() and __free
> usage will make this code quite a bit nicer to read.
Yeah, it's a good way to simplify code on error path.
I will take into account it. thanks for your good suggestion.
>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
>> ---
>> drivers/soc/hisilicon/kunpeng_hccs.c | 24 +++++++++++++++---------
>> 1 file changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
>> index 6e88f597f267..6055e5091cbd 100644
>> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
>> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
>> @@ -170,15 +170,21 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
>> goto err_mbx_channel_free;
>> }
>>
>> - if (pcc_chan->shmem_base_addr) {
>> - cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr,
>> - pcc_chan->shmem_size);
>> - if (!cl_info->pcc_comm_addr) {
>> - dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n",
>> - hdev->chan_id);
>> - rc = -ENOMEM;
>> - goto err_mbx_channel_free;
>> - }
>> + if (!pcc_chan->shmem_base_addr ||
>> + pcc_chan->shmem_size != HCCS_PCC_SHARE_MEM_BYTES) {
>> + dev_err(dev, "The base address or size (%llu) of PCC communication region is invalid.\n",
>> + pcc_chan->shmem_size);
>> + rc = -EINVAL;
>> + goto err_mbx_channel_free;
> Worth considering for the future: Maybe a DEFINE_FREE for pcc_mbox_free_channel) makes sense,
> though if you do you should only assign cl_info->pcc_chan after all possible error paths.
Ack
>
>> + }
>> +
>> + cl_info->pcc_comm_addr = ioremap(pcc_chan->shmem_base_addr,
>> + pcc_chan->shmem_size);
>> + if (!cl_info->pcc_comm_addr) {
>> + dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n",
>> + hdev->chan_id);
>> + rc = -ENOMEM;
>> + goto err_mbx_channel_free;
>> }
>>
>> return 0;
> .
The lane mode of HCCS port is an information to user, and actually comes
from the maximum lane number. But it is good and easy for driver to use
the maximum lane number. So fix the 'lane_mode' field name in port info
structure to 'max_lane_num'.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
drivers/soc/hisilicon/kunpeng_hccs.c | 4 ++--
drivers/soc/hisilicon/kunpeng_hccs.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index 6055e5091cbd..418e4ee5d9e5 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -594,7 +594,7 @@ static int hccs_get_all_port_info_on_die(struct hccs_dev *hdev,
port = &die->ports[i];
port->port_id = attrs[i].port_id;
port->port_type = attrs[i].port_type;
- port->lane_mode = attrs[i].lane_mode;
+ port->max_lane_num = attrs[i].max_lane_num;
port->enable = attrs[i].enable;
port->die = die;
}
@@ -839,7 +839,7 @@ static ssize_t lane_mode_show(struct kobject *kobj, struct kobj_attribute *attr,
{
const struct hccs_port_info *port = kobj_to_port_info(kobj);
- return sysfs_emit(buf, "x%u\n", port->lane_mode);
+ return sysfs_emit(buf, "x%u\n", port->max_lane_num);
}
static struct kobj_attribute lane_mode_attr = __ATTR_RO(lane_mode);
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
index c3adbc01b471..5e12a1e1474e 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.h
+++ b/drivers/soc/hisilicon/kunpeng_hccs.h
@@ -19,7 +19,7 @@
struct hccs_port_info {
u8 port_id;
u8 port_type;
- u8 lane_mode;
+ u8 max_lane_num;
bool enable; /* if the port is enabled */
struct kobject kobj;
bool dir_created;
@@ -113,7 +113,7 @@ struct hccs_die_info_rsp_data {
struct hccs_port_attr {
u8 port_id;
u8 port_type;
- u8 lane_mode;
+ u8 max_lane_num;
u8 enable : 1; /* if the port is enabled */
u16 rsv[2];
};
--
2.22.0On Fri, 23 Aug 2024 11:10:57 +0800
Huisong Li <lihuisong@huawei.com> wrote:
> The lane mode of HCCS port is an information to user, and actually comes
> from the maximum lane number. But it is good and easy for driver to use
> the maximum lane number. So fix the 'lane_mode' field name in port info
> structure to 'max_lane_num'.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
It's unfortunate we missed the ABI in the first place
as that's still confusingly names, but at least this improves things
in the driver.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/soc/hisilicon/kunpeng_hccs.c | 4 ++--
> drivers/soc/hisilicon/kunpeng_hccs.h | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
> index 6055e5091cbd..418e4ee5d9e5 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
> @@ -594,7 +594,7 @@ static int hccs_get_all_port_info_on_die(struct hccs_dev *hdev,
> port = &die->ports[i];
> port->port_id = attrs[i].port_id;
> port->port_type = attrs[i].port_type;
> - port->lane_mode = attrs[i].lane_mode;
> + port->max_lane_num = attrs[i].max_lane_num;
> port->enable = attrs[i].enable;
> port->die = die;
> }
> @@ -839,7 +839,7 @@ static ssize_t lane_mode_show(struct kobject *kobj, struct kobj_attribute *attr,
> {
> const struct hccs_port_info *port = kobj_to_port_info(kobj);
>
> - return sysfs_emit(buf, "x%u\n", port->lane_mode);
> + return sysfs_emit(buf, "x%u\n", port->max_lane_num);
> }
> static struct kobj_attribute lane_mode_attr = __ATTR_RO(lane_mode);
>
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
> index c3adbc01b471..5e12a1e1474e 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.h
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.h
> @@ -19,7 +19,7 @@
> struct hccs_port_info {
> u8 port_id;
> u8 port_type;
> - u8 lane_mode;
> + u8 max_lane_num;
> bool enable; /* if the port is enabled */
> struct kobject kobj;
> bool dir_created;
> @@ -113,7 +113,7 @@ struct hccs_die_info_rsp_data {
> struct hccs_port_attr {
> u8 port_id;
> u8 port_type;
> - u8 lane_mode;
> + u8 max_lane_num;
> u8 enable : 1; /* if the port is enabled */
> u16 rsv[2];
> };
在 2024/8/23 16:40, Jonathan Cameron 写道:
> On Fri, 23 Aug 2024 11:10:57 +0800
> Huisong Li <lihuisong@huawei.com> wrote:
>
>> The lane mode of HCCS port is an information to user, and actually comes
>> from the maximum lane number. But it is good and easy for driver to use
>> the maximum lane number. So fix the 'lane_mode' field name in port info
>> structure to 'max_lane_num'.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> It's unfortunate we missed the ABI in the first place
> as that's still confusingly names, but at least this improves things
> in the driver.
But it is ok for an external interface to use the 'lane_mode' name. It's
similar to PCIE's x8.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
>> ---
>> drivers/soc/hisilicon/kunpeng_hccs.c | 4 ++--
>> drivers/soc/hisilicon/kunpeng_hccs.h | 4 ++--
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
>> index 6055e5091cbd..418e4ee5d9e5 100644
>> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
>> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
>> @@ -594,7 +594,7 @@ static int hccs_get_all_port_info_on_die(struct hccs_dev *hdev,
>> port = &die->ports[i];
>> port->port_id = attrs[i].port_id;
>> port->port_type = attrs[i].port_type;
>> - port->lane_mode = attrs[i].lane_mode;
>> + port->max_lane_num = attrs[i].max_lane_num;
>> port->enable = attrs[i].enable;
>> port->die = die;
>> }
>> @@ -839,7 +839,7 @@ static ssize_t lane_mode_show(struct kobject *kobj, struct kobj_attribute *attr,
>> {
>> const struct hccs_port_info *port = kobj_to_port_info(kobj);
>>
>> - return sysfs_emit(buf, "x%u\n", port->lane_mode);
>> + return sysfs_emit(buf, "x%u\n", port->max_lane_num);
>> }
>> static struct kobj_attribute lane_mode_attr = __ATTR_RO(lane_mode);
>>
>> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
>> index c3adbc01b471..5e12a1e1474e 100644
>> --- a/drivers/soc/hisilicon/kunpeng_hccs.h
>> +++ b/drivers/soc/hisilicon/kunpeng_hccs.h
>> @@ -19,7 +19,7 @@
>> struct hccs_port_info {
>> u8 port_id;
>> u8 port_type;
>> - u8 lane_mode;
>> + u8 max_lane_num;
>> bool enable; /* if the port is enabled */
>> struct kobject kobj;
>> bool dir_created;
>> @@ -113,7 +113,7 @@ struct hccs_die_info_rsp_data {
>> struct hccs_port_attr {
>> u8 port_id;
>> u8 port_type;
>> - u8 lane_mode;
>> + u8 max_lane_num;
>> u8 enable : 1; /* if the port is enabled */
>> u16 rsv[2];
>> };
> .
Current, to find which HCC types are used on the platform the user needs
to scan the type attribute of all ports, which is unfriendly to the user.
So add the sysfs to show all HCCS types used on the platform.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
.../sysfs-devices-platform-kunpeng_hccs | 8 ++
drivers/soc/hisilicon/kunpeng_hccs.c | 102 +++++++++++++++++-
drivers/soc/hisilicon/kunpeng_hccs.h | 15 +++
3 files changed, 124 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
index 1666340820f7..d4c355e0e0bb 100644
--- a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
+++ b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
@@ -79,3 +79,11 @@ Description:
indicates a lane.
crc_err_cnt: (RO) CRC err count on this port.
============= ==== =============================================
+
+What: /sys/devices/platform/HISI04Bx:00/used_types
+Date: August 2024
+KernelVersion: 6.12
+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.
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index 418e4ee5d9e5..623e7b7ed39a 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -21,11 +21,14 @@
* - if all enabled ports are in linked
* - if all linked ports are in full lane
* - CRC error count sum
+ *
+ * - Retrieve all HCCS types used on the platform.
*/
#include <linux/acpi.h>
#include <linux/iopoll.h>
#include <linux/platform_device.h>
#include <linux/sysfs.h>
+#include <linux/types.h>
#include <acpi/pcc.h>
@@ -53,6 +56,15 @@ static struct hccs_chip_info *kobj_to_chip_info(struct kobject *k)
return container_of(k, struct hccs_chip_info, kobj);
}
+static struct hccs_dev *device_kobj_to_hccs_dev(struct kobject *k)
+{
+ struct device *dev = container_of(k, struct device, kobj);
+ struct platform_device *pdev =
+ container_of(dev, struct platform_device, dev);
+
+ return platform_get_drvdata(pdev);
+}
+
struct hccs_register_ctx {
struct device *dev;
u8 chan_id;
@@ -670,6 +682,55 @@ static int hccs_get_hw_info(struct hccs_dev *hdev)
return 0;
}
+static u16 hccs_calc_used_type_num(struct hccs_dev *hdev,
+ unsigned long *hccs_ver)
+{
+ struct hccs_chip_info *chip;
+ struct hccs_port_info *port;
+ struct hccs_die_info *die;
+ u16 used_type_num = 0;
+ u16 i, j, k;
+
+ 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];
+ set_bit(port->port_type, hccs_ver);
+ }
+ }
+ }
+
+ for_each_set_bit(i, hccs_ver, HCCS_IP_MAX + 1)
+ used_type_num++;
+
+ return used_type_num;
+}
+
+static int hccs_init_type_name_maps(struct hccs_dev *hdev)
+{
+ DECLARE_BITMAP(hccs_ver, HCCS_IP_MAX + 1) = {};
+ unsigned int i;
+ u16 idx = 0;
+
+ hdev->used_type_num = hccs_calc_used_type_num(hdev, hccs_ver);
+ hdev->type_name_maps = devm_kcalloc(hdev->dev, hdev->used_type_num,
+ sizeof(struct hccs_type_name_map),
+ GFP_KERNEL);
+ if (!hdev->type_name_maps)
+ return -ENOMEM;
+
+ for_each_set_bit(i, hccs_ver, HCCS_IP_MAX + 1) {
+ hdev->type_name_maps[idx].type = i;
+ sprintf(hdev->type_name_maps[idx].name,
+ "%s%u", HCCS_IP_PREFIX, i);
+ idx++;
+ }
+
+ return 0;
+}
+
static int hccs_query_port_link_status(struct hccs_dev *hdev,
const struct hccs_port_info *port,
struct hccs_link_status *link_status)
@@ -830,7 +891,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
{
const struct hccs_port_info *port = kobj_to_port_info(kobj);
- return sysfs_emit(buf, "HCCS-v%u\n", port->port_type);
+ return sysfs_emit(buf, "%s%u\n", HCCS_IP_PREFIX, port->port_type);
}
static struct kobj_attribute hccs_type_attr = __ATTR_RO(type);
@@ -1134,6 +1195,33 @@ static const struct kobj_type hccs_chip_type = {
.default_groups = hccs_chip_default_groups,
};
+
+static ssize_t used_types_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ struct hccs_dev *hdev = device_kobj_to_hccs_dev(kobj);
+ int len = 0;
+ u16 i;
+
+ for (i = 0; i < hdev->used_type_num - 1; i++)
+ len += sysfs_emit(&buf[len], "%s ", hdev->type_name_maps[i].name);
+ len += sysfs_emit(&buf[len], "%s\n", hdev->type_name_maps[i].name);
+
+ return len;
+}
+static struct kobj_attribute used_types_attr =
+ __ATTR(used_types, 0444, used_types_show, NULL);
+
+static void hccs_remove_misc_sysfs(struct hccs_dev *hdev)
+{
+ sysfs_remove_file(&hdev->dev->kobj, &used_types_attr.attr);
+}
+
+static int hccs_add_misc_sysfs(struct hccs_dev *hdev)
+{
+ return sysfs_create_file(&hdev->dev->kobj, &used_types_attr.attr);
+}
+
static void hccs_remove_die_dir(struct hccs_die_info *die)
{
struct hccs_port_info *port;
@@ -1168,6 +1256,8 @@ static void hccs_remove_topo_dirs(struct hccs_dev *hdev)
for (i = 0; i < hdev->chip_num; i++)
hccs_remove_chip_dir(&hdev->chips[i]);
+
+ hccs_remove_misc_sysfs(hdev);
}
static int hccs_create_hccs_dir(struct hccs_dev *hdev,
@@ -1263,6 +1353,12 @@ static int hccs_create_topo_dirs(struct hccs_dev *hdev)
}
}
+ ret = hccs_add_misc_sysfs(hdev);
+ if (ret) {
+ dev_err(hdev->dev, "create misc sysfs interface failed, ret = %d\n", ret);
+ goto err;
+ }
+
return 0;
err:
for (k = 0; k < id; k++)
@@ -1313,6 +1409,10 @@ static int hccs_probe(struct platform_device *pdev)
if (rc)
goto unregister_pcc_chan;
+ rc = hccs_init_type_name_maps(hdev);
+ if (rc)
+ goto unregister_pcc_chan;
+
rc = hccs_create_topo_dirs(hdev);
if (rc)
goto unregister_pcc_chan;
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
index 5e12a1e1474e..401df4694aec 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.h
+++ b/drivers/soc/hisilicon/kunpeng_hccs.h
@@ -10,6 +10,19 @@
* | P0 | P1 | P2 | P3 | P0 | P1 | P2 | P3 | P0 | P1 | P2 | P3 |P0 | P1 | P2 | P3 |
*/
+enum hccs_port_type {
+ HCCS_V1 = 1,
+ HCCS_V2,
+};
+
+#define HCCS_IP_PREFIX "HCCS-v"
+#define HCCS_IP_MAX 255
+#define HCCS_NAME_MAX_LEN 9
+struct hccs_type_name_map {
+ u8 type;
+ char name[HCCS_NAME_MAX_LEN + 1];
+};
+
/*
* This value cannot be 255, otherwise the loop of the multi-BD communication
* case cannot end.
@@ -74,6 +87,8 @@ struct hccs_dev {
u64 caps;
u8 chip_num;
struct hccs_chip_info *chips;
+ u16 used_type_num;
+ struct hccs_type_name_map *type_name_maps;
u8 chan_id;
struct mutex lock;
struct hccs_mbox_client_info cl_info;
--
2.22.0On Fri, 23 Aug 2024 11:10:58 +0800
Huisong Li <lihuisong@huawei.com> wrote:
> Current, to find which HCC types are used on the platform the user needs
> to scan the type attribute of all ports, which is unfriendly to the user.
> So add the sysfs to show all HCCS types used on the platform.
Really minor point, but it might be nice to add a little info here on
the sort of changes that occur in the interface between versions?
Even if that's just a reference to next patch which shows that the
HCCS-V2 has power control that original version didn't.
That will help motivate the patch.
Also good to argue why it is worth aggregating this info in one attribute
rather than just letting user space search for it in the topology below
this point. (Something about global controls I guess?)
Also, for future, the hcc_unregister_pcc_channel() would be nicely
handled with a devm_add_action_or_reset() removing the need
for the various gotos in probe.
Similar applied to hcc_remove_top_dirs and remove() callback can
go away entirely which would be an added bonus.
Jonathan
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> .../sysfs-devices-platform-kunpeng_hccs | 8 ++
> drivers/soc/hisilicon/kunpeng_hccs.c | 102 +++++++++++++++++-
> drivers/soc/hisilicon/kunpeng_hccs.h | 15 +++
> 3 files changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
> index 1666340820f7..d4c355e0e0bb 100644
> --- a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
> +++ b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
> @@ -79,3 +79,11 @@ Description:
> indicates a lane.
> crc_err_cnt: (RO) CRC err count on this port.
> ============= ==== =============================================
> +
> +What: /sys/devices/platform/HISI04Bx:00/used_types
> +Date: August 2024
> +KernelVersion: 6.12
> +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.
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
> index 418e4ee5d9e5..623e7b7ed39a 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
> @@ -21,11 +21,14 @@
> * - if all enabled ports are in linked
> * - if all linked ports are in full lane
> * - CRC error count sum
> + *
> + * - Retrieve all HCCS types used on the platform.
> */
> #include <linux/acpi.h>
> #include <linux/iopoll.h>
> #include <linux/platform_device.h>
> #include <linux/sysfs.h>
> +#include <linux/types.h>
>
> #include <acpi/pcc.h>
>
> @@ -53,6 +56,15 @@ static struct hccs_chip_info *kobj_to_chip_info(struct kobject *k)
> return container_of(k, struct hccs_chip_info, kobj);
> }
>
> +static struct hccs_dev *device_kobj_to_hccs_dev(struct kobject *k)
> +{
> + struct device *dev = container_of(k, struct device, kobj);
> + struct platform_device *pdev =
> + container_of(dev, struct platform_device, dev);
> +
> + return platform_get_drvdata(pdev);
> +}
> +
> struct hccs_register_ctx {
> struct device *dev;
> u8 chan_id;
> @@ -670,6 +682,55 @@ static int hccs_get_hw_info(struct hccs_dev *hdev)
> return 0;
> }
>
> +static u16 hccs_calc_used_type_num(struct hccs_dev *hdev,
> + unsigned long *hccs_ver)
> +{
> + struct hccs_chip_info *chip;
> + struct hccs_port_info *port;
> + struct hccs_die_info *die;
> + u16 used_type_num = 0;
> + u16 i, j, k;
> +
> + 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];
> + set_bit(port->port_type, hccs_ver);
> + }
> + }
> + }
> +
> + for_each_set_bit(i, hccs_ver, HCCS_IP_MAX + 1)
> + used_type_num++;
> +
> + return used_type_num;
> +}
> +
> +static int hccs_init_type_name_maps(struct hccs_dev *hdev)
> +{
> + DECLARE_BITMAP(hccs_ver, HCCS_IP_MAX + 1) = {};
> + unsigned int i;
> + u16 idx = 0;
> +
> + hdev->used_type_num = hccs_calc_used_type_num(hdev, hccs_ver);
> + hdev->type_name_maps = devm_kcalloc(hdev->dev, hdev->used_type_num,
> + sizeof(struct hccs_type_name_map),
> + GFP_KERNEL);
> + if (!hdev->type_name_maps)
> + return -ENOMEM;
> +
> + for_each_set_bit(i, hccs_ver, HCCS_IP_MAX + 1) {
> + hdev->type_name_maps[idx].type = i;
> + sprintf(hdev->type_name_maps[idx].name,
> + "%s%u", HCCS_IP_PREFIX, i);
> + idx++;
> + }
> +
> + return 0;
> +}
> +
> static int hccs_query_port_link_status(struct hccs_dev *hdev,
> const struct hccs_port_info *port,
> struct hccs_link_status *link_status)
> @@ -830,7 +891,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
> {
> const struct hccs_port_info *port = kobj_to_port_info(kobj);
>
> - return sysfs_emit(buf, "HCCS-v%u\n", port->port_type);
> + return sysfs_emit(buf, "%s%u\n", HCCS_IP_PREFIX, port->port_type);
> }
> static struct kobj_attribute hccs_type_attr = __ATTR_RO(type);
>
> @@ -1134,6 +1195,33 @@ static const struct kobj_type hccs_chip_type = {
> .default_groups = hccs_chip_default_groups,
> };
>
> +
> +static ssize_t used_types_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct hccs_dev *hdev = device_kobj_to_hccs_dev(kobj);
> + int len = 0;
> + u16 i;
> +
> + for (i = 0; i < hdev->used_type_num - 1; i++)
> + len += sysfs_emit(&buf[len], "%s ", hdev->type_name_maps[i].name);
> + len += sysfs_emit(&buf[len], "%s\n", hdev->type_name_maps[i].name);
> +
> + return len;
> +}
> +static struct kobj_attribute used_types_attr =
> + __ATTR(used_types, 0444, used_types_show, NULL);
> +
> +static void hccs_remove_misc_sysfs(struct hccs_dev *hdev)
> +{
> + sysfs_remove_file(&hdev->dev->kobj, &used_types_attr.attr);
> +}
> +
> +static int hccs_add_misc_sysfs(struct hccs_dev *hdev)
> +{
> + return sysfs_create_file(&hdev->dev->kobj, &used_types_attr.attr);
> +}
> +
> static void hccs_remove_die_dir(struct hccs_die_info *die)
> {
> struct hccs_port_info *port;
> @@ -1168,6 +1256,8 @@ static void hccs_remove_topo_dirs(struct hccs_dev *hdev)
>
> for (i = 0; i < hdev->chip_num; i++)
> hccs_remove_chip_dir(&hdev->chips[i]);
> +
> + hccs_remove_misc_sysfs(hdev);
> }
>
> static int hccs_create_hccs_dir(struct hccs_dev *hdev,
> @@ -1263,6 +1353,12 @@ static int hccs_create_topo_dirs(struct hccs_dev *hdev)
> }
> }
>
> + ret = hccs_add_misc_sysfs(hdev);
> + if (ret) {
> + dev_err(hdev->dev, "create misc sysfs interface failed, ret = %d\n", ret);
> + goto err;
> + }
> +
> return 0;
> err:
> for (k = 0; k < id; k++)
> @@ -1313,6 +1409,10 @@ static int hccs_probe(struct platform_device *pdev)
> if (rc)
> goto unregister_pcc_chan;
>
> + rc = hccs_init_type_name_maps(hdev);
> + if (rc)
> + goto unregister_pcc_chan;
> +
> rc = hccs_create_topo_dirs(hdev);
> if (rc)
> goto unregister_pcc_chan;
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
> index 5e12a1e1474e..401df4694aec 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.h
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.h
> @@ -10,6 +10,19 @@
> * | P0 | P1 | P2 | P3 | P0 | P1 | P2 | P3 | P0 | P1 | P2 | P3 |P0 | P1 | P2 | P3 |
> */
>
> +enum hccs_port_type {
> + HCCS_V1 = 1,
> + HCCS_V2,
> +};
> +
> +#define HCCS_IP_PREFIX "HCCS-v"
> +#define HCCS_IP_MAX 255
> +#define HCCS_NAME_MAX_LEN 9
> +struct hccs_type_name_map {
> + u8 type;
> + char name[HCCS_NAME_MAX_LEN + 1];
> +};
> +
> /*
> * This value cannot be 255, otherwise the loop of the multi-BD communication
> * case cannot end.
> @@ -74,6 +87,8 @@ struct hccs_dev {
> u64 caps;
> u8 chip_num;
> struct hccs_chip_info *chips;
> + u16 used_type_num;
> + struct hccs_type_name_map *type_name_maps;
> u8 chan_id;
> struct mutex lock;
> struct hccs_mbox_client_info cl_info;
在 2024/8/23 16:47, Jonathan Cameron 写道:
> On Fri, 23 Aug 2024 11:10:58 +0800
> Huisong Li <lihuisong@huawei.com> wrote:
>
>> Current, to find which HCC types are used on the platform the user needs
>> to scan the type attribute of all ports, which is unfriendly to the user.
>> So add the sysfs to show all HCCS types used on the platform.
> Really minor point, but it might be nice to add a little info here on
> the sort of changes that occur in the interface between versions?
> Even if that's just a reference to next patch which shows that the
> HCCS-V2 has power control that original version didn't.
Yes, you are right. I will add some words like this.
>
> That will help motivate the patch.
>
> Also good to argue why it is worth aggregating this info in one attribute
> rather than just letting user space search for it in the topology below
> this point. (Something about global controls I guess?)
Good point. Thanks.
>
> Also, for future, the hcc_unregister_pcc_channel() would be nicely
> handled with a devm_add_action_or_reset() removing the need
> for the various gotos in probe.
>
> Similar applied to hcc_remove_top_dirs and remove() callback can
> go away entirely which would be an added bonus.
The devm_add action or reset you proposed is good way to keep simply code.
It is more recommanded if we add new code. I like it.
The current removing way is a traditional one and also understand easier.
So I'm not very motivated to modify it. Thanks for your suggestion.😁
>
> Jonathan
>
>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> .../sysfs-devices-platform-kunpeng_hccs | 8 ++
>> drivers/soc/hisilicon/kunpeng_hccs.c | 102 +++++++++++++++++-
>> drivers/soc/hisilicon/kunpeng_hccs.h | 15 +++
>> 3 files changed, 124 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
>> index 1666340820f7..d4c355e0e0bb 100644
>> --- a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
>> +++ b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
>> @@ -79,3 +79,11 @@ Description:
>> indicates a lane.
>> crc_err_cnt: (RO) CRC err count on this port.
>> ============= ==== =============================================
>> +
>> +What: /sys/devices/platform/HISI04Bx:00/used_types
>> +Date: August 2024
>> +KernelVersion: 6.12
>> +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.
>> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
>> index 418e4ee5d9e5..623e7b7ed39a 100644
>> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
>> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
>> @@ -21,11 +21,14 @@
>> * - if all enabled ports are in linked
>> * - if all linked ports are in full lane
>> * - CRC error count sum
>> + *
>> + * - Retrieve all HCCS types used on the platform.
>> */
>> #include <linux/acpi.h>
>> #include <linux/iopoll.h>
>> #include <linux/platform_device.h>
>> #include <linux/sysfs.h>
>> +#include <linux/types.h>
>>
>> #include <acpi/pcc.h>
>>
>> @@ -53,6 +56,15 @@ static struct hccs_chip_info *kobj_to_chip_info(struct kobject *k)
>> return container_of(k, struct hccs_chip_info, kobj);
>> }
>>
>> +static struct hccs_dev *device_kobj_to_hccs_dev(struct kobject *k)
>> +{
>> + struct device *dev = container_of(k, struct device, kobj);
>> + struct platform_device *pdev =
>> + container_of(dev, struct platform_device, dev);
>> +
>> + return platform_get_drvdata(pdev);
>> +}
>> +
>> struct hccs_register_ctx {
>> struct device *dev;
>> u8 chan_id;
>> @@ -670,6 +682,55 @@ static int hccs_get_hw_info(struct hccs_dev *hdev)
>> return 0;
>> }
>>
>> +static u16 hccs_calc_used_type_num(struct hccs_dev *hdev,
>> + unsigned long *hccs_ver)
>> +{
>> + struct hccs_chip_info *chip;
>> + struct hccs_port_info *port;
>> + struct hccs_die_info *die;
>> + u16 used_type_num = 0;
>> + u16 i, j, k;
>> +
>> + 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];
>> + set_bit(port->port_type, hccs_ver);
>> + }
>> + }
>> + }
>> +
>> + for_each_set_bit(i, hccs_ver, HCCS_IP_MAX + 1)
>> + used_type_num++;
>> +
>> + return used_type_num;
>> +}
>> +
>> +static int hccs_init_type_name_maps(struct hccs_dev *hdev)
>> +{
>> + DECLARE_BITMAP(hccs_ver, HCCS_IP_MAX + 1) = {};
>> + unsigned int i;
>> + u16 idx = 0;
>> +
>> + hdev->used_type_num = hccs_calc_used_type_num(hdev, hccs_ver);
>> + hdev->type_name_maps = devm_kcalloc(hdev->dev, hdev->used_type_num,
>> + sizeof(struct hccs_type_name_map),
>> + GFP_KERNEL);
>> + if (!hdev->type_name_maps)
>> + return -ENOMEM;
>> +
>> + for_each_set_bit(i, hccs_ver, HCCS_IP_MAX + 1) {
>> + hdev->type_name_maps[idx].type = i;
>> + sprintf(hdev->type_name_maps[idx].name,
>> + "%s%u", HCCS_IP_PREFIX, i);
>> + idx++;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int hccs_query_port_link_status(struct hccs_dev *hdev,
>> const struct hccs_port_info *port,
>> struct hccs_link_status *link_status)
>> @@ -830,7 +891,7 @@ static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
>> {
>> const struct hccs_port_info *port = kobj_to_port_info(kobj);
>>
>> - return sysfs_emit(buf, "HCCS-v%u\n", port->port_type);
>> + return sysfs_emit(buf, "%s%u\n", HCCS_IP_PREFIX, port->port_type);
>> }
>> static struct kobj_attribute hccs_type_attr = __ATTR_RO(type);
>>
>> @@ -1134,6 +1195,33 @@ static const struct kobj_type hccs_chip_type = {
>> .default_groups = hccs_chip_default_groups,
>> };
>>
>> +
>> +static ssize_t used_types_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + struct hccs_dev *hdev = device_kobj_to_hccs_dev(kobj);
>> + int len = 0;
>> + u16 i;
>> +
>> + for (i = 0; i < hdev->used_type_num - 1; i++)
>> + len += sysfs_emit(&buf[len], "%s ", hdev->type_name_maps[i].name);
>> + len += sysfs_emit(&buf[len], "%s\n", hdev->type_name_maps[i].name);
>> +
>> + return len;
>> +}
>> +static struct kobj_attribute used_types_attr =
>> + __ATTR(used_types, 0444, used_types_show, NULL);
>> +
>> +static void hccs_remove_misc_sysfs(struct hccs_dev *hdev)
>> +{
>> + sysfs_remove_file(&hdev->dev->kobj, &used_types_attr.attr);
>> +}
>> +
>> +static int hccs_add_misc_sysfs(struct hccs_dev *hdev)
>> +{
>> + return sysfs_create_file(&hdev->dev->kobj, &used_types_attr.attr);
>> +}
>> +
>> static void hccs_remove_die_dir(struct hccs_die_info *die)
>> {
>> struct hccs_port_info *port;
>> @@ -1168,6 +1256,8 @@ static void hccs_remove_topo_dirs(struct hccs_dev *hdev)
>>
>> for (i = 0; i < hdev->chip_num; i++)
>> hccs_remove_chip_dir(&hdev->chips[i]);
>> +
>> + hccs_remove_misc_sysfs(hdev);
>> }
>>
>> static int hccs_create_hccs_dir(struct hccs_dev *hdev,
>> @@ -1263,6 +1353,12 @@ static int hccs_create_topo_dirs(struct hccs_dev *hdev)
>> }
>> }
>>
>> + ret = hccs_add_misc_sysfs(hdev);
>> + if (ret) {
>> + dev_err(hdev->dev, "create misc sysfs interface failed, ret = %d\n", ret);
>> + goto err;
>> + }
>> +
>> return 0;
>> err:
>> for (k = 0; k < id; k++)
>> @@ -1313,6 +1409,10 @@ static int hccs_probe(struct platform_device *pdev)
>> if (rc)
>> goto unregister_pcc_chan;
>>
>> + rc = hccs_init_type_name_maps(hdev);
>> + if (rc)
>> + goto unregister_pcc_chan;
>> +
>> rc = hccs_create_topo_dirs(hdev);
>> if (rc)
>> goto unregister_pcc_chan;
>> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
>> index 5e12a1e1474e..401df4694aec 100644
>> --- a/drivers/soc/hisilicon/kunpeng_hccs.h
>> +++ b/drivers/soc/hisilicon/kunpeng_hccs.h
>> @@ -10,6 +10,19 @@
>> * | P0 | P1 | P2 | P3 | P0 | P1 | P2 | P3 | P0 | P1 | P2 | P3 |P0 | P1 | P2 | P3 |
>> */
>>
>> +enum hccs_port_type {
>> + HCCS_V1 = 1,
>> + HCCS_V2,
>> +};
>> +
>> +#define HCCS_IP_PREFIX "HCCS-v"
>> +#define HCCS_IP_MAX 255
>> +#define HCCS_NAME_MAX_LEN 9
>> +struct hccs_type_name_map {
>> + u8 type;
>> + char name[HCCS_NAME_MAX_LEN + 1];
>> +};
>> +
>> /*
>> * This value cannot be 255, otherwise the loop of the multi-BD communication
>> * case cannot end.
>> @@ -74,6 +87,8 @@ struct hccs_dev {
>> u64 caps;
>> u8 chip_num;
>> struct hccs_chip_info *chips;
>> + u16 used_type_num;
>> + struct hccs_type_name_map *type_name_maps;
>> u8 chan_id;
>> struct mutex lock;
>> struct hccs_mbox_client_info cl_info;
> .
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(-)
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
--
2.22.0On 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)
> > .
© 2016 - 2025 Red Hat, Inc.