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

From: lihuisong (C)
Date: Sun Aug 06 2023 - 21:41:14 EST


Hi Zenghui,

Please ignore the previous email and take a look at this.


在 2023/8/6 23:09, Zenghui Yu 写道:
A few trivial comments inline.
Many thanks for reviewing my patch carefully.😁

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"?
Yes, this is more exact.
Will be fixed to "get port info on chip%u/die%u 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.
Yes, you are right. It's my fault.
Appreciate you so much for pointing it out.
I will also check other commands again.

+/*
+ * 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?
The "max port id" normally depends on HW implementation.
And there are no so many numbers of port on one die.
But HW implementation specification is possiable to change in furture SoC.
Driver should be compatible with it.
So "max port id" here comes from the communication interface and way with firmware.

+#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.
This macroes were used in previous version.
Later, the place where it is used was deleted, now it is unused indeed.
will delete it.

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
Ok, I will split this patch in next version. Thanks.
.
/Huisong