Re: [PATCH v6 1/2] soc: hisilicon: Support HCCS driver on Kunpeng SoC

[Zenghui Yu]

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@xxxxxxxxxx>

[...]

> +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