lkml.org 
[lkml]   [2023]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v6 1/2] soc: hisilicon: Support HCCS driver on Kunpeng SoC
From
A few trivial comments inline.

On 2023/8/1 10:41, Huisong Li wrote:
> The Huawei Cache Coherence System (HCCS) is a multi-chip interconnection
> bus protocol. The performance of the application may be affected if some
> HCCS ports on platform are not in full lane status, have a large number
> of CRC errors and so on.
>
> This driver provides the query interface of the health status and
> port information of HCCS on Kunpeng SoC.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>

[...]

> +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;
> + u8 i, j;
> + int ret;
> +
> + for (i = 0; i < hdev->chip_num; i++) {
> + chip = &hdev->chips[i];
> + for (j = 0; j < chip->die_num; j++) {
> + die = &chip->dies[j];
> + if (!die->port_num)
> + continue;
> +
> + die->ports = devm_kzalloc(dev,
> + die->port_num * sizeof(struct hccs_port_info),
> + GFP_KERNEL);
> + if (!die->ports) {
> + dev_err(dev, "allocate ports memory on chip%u/die%u failed.\n",
> + i, die->die_id);
> + return -ENOMEM;
> + }
> +
> + ret = hccs_get_all_port_info_on_die(hdev, die);
> + if (ret) {
> + dev_err(dev, "get die(%u) info on chip%u failed, ret = %d.\n",

"get *port* info failed"?

> +static int hccs_get_die_all_link_status(struct hccs_dev *hdev,
> + const struct hccs_die_info *die,
> + u8 *all_linked)
> +{
> + struct hccs_die_comm_req_param *req_param;
> + struct hccs_desc desc;
> + int ret;
> +
> + if (die->port_num == 0) {
> + *all_linked = 1;
> + return 0;
> + }
> +
> + hccs_init_req_desc(&desc);
> + req_param = (struct hccs_die_comm_req_param *)desc.req.data;
> + req_param->chip_id = die->chip->chip_id;
> + req_param->die_id = die->die_id;
> + ret = hccs_pcc_cmd_send(hdev, HCCS_GET_DIE_PORTS_LANE_STA, &desc);

Typo? Looks like we intend to send a HCCS_GET_DIE_PORTS_LINK_STA
command.

> +/*
> + * This value cannot be 255, otherwise the loop of the multi-BD communication
> + * case cannot end.
> + */
> +#define HCCS_DIE_MAX_PORT_ID 254

This looks weird. Isn't the "max port id" depends on your HW
implementation?

> +#define hccs_get_field(origin, mask, shift) \
> + (((origin) & (mask)) >> (shift))
> +#define hccs_get_bit(origin, shift) \
> + hccs_get_field((origin), (0x1UL << (shift)), (shift))

Unused macroes.

P.S., I'd personally prefer splitting this patch in 2 to ease other
reviewer's work:

- deal with the HCCS HW (chip/die/port) probing
- focus on the sysfs/query things

Zenghui

\
 
 \ /
  Last update: 2023-08-06 17:18    [W:0.205 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site