Re: [PATCH] soc: hisilicon: Support HCCS driver on Kunpeng SoC

[Krzysztof Kozlowski <krzk@kernel.org>]

On 24/04/2023 09:30, Huisong Li wrote:
> The Huawei Cache-Coherent System (HCCS) is a bus protocol standard
> for ensuring cache coherent on HiSilicon SoC. The performance of
> the application may be affected if some hccs ports are in non-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>
> ---
>  MAINTAINERS                          |    6 +
>  drivers/soc/Kconfig                  |    1 +
>  drivers/soc/Makefile                 |    1 +
>  drivers/soc/hisilicon/Kconfig        |   18 +
>  drivers/soc/hisilicon/Makefile       |    2 +
>  drivers/soc/hisilicon/kunpeng_hccs.c | 1296 ++++++++++++++++++++++++++
>  drivers/soc/hisilicon/kunpeng_hccs.h |  204 ++++
>  7 files changed, 1528 insertions(+)
>  create mode 100644 drivers/soc/hisilicon/Kconfig
>  create mode 100644 drivers/soc/hisilicon/Makefile
>  create mode 100644 drivers/soc/hisilicon/kunpeng_hccs.c
>  create mode 100644 drivers/soc/hisilicon/kunpeng_hccs.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index eddbc48c61e9..fe0e796e8445 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9399,6 +9399,12 @@ S:	Maintained
>  W:	http://www.hisilicon.com
>  F:	drivers/spi/spi-hisi-sfc-v3xx.c
>  
> +HISILICON KUNPENG SOC HCCS DRIVER
> +M:	Huisong Li <lihuisong@huawei.com>
> +S:	Maintained
> +F:	drivers/soc/hisilicon/kunpeng_hccs.c
> +F:	drivers/soc/hisilicon/kunpeng_hccs.h
> +
>  HMM - Heterogeneous Memory Management
>  M:	Jérôme Glisse <jglisse@redhat.com>
>  L:	linux-mm@kvack.org
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 4e176280113a..d21e75d69294 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -10,6 +10,7 @@ source "drivers/soc/bcm/Kconfig"
>  source "drivers/soc/canaan/Kconfig"
>  source "drivers/soc/fsl/Kconfig"
>  source "drivers/soc/fujitsu/Kconfig"
> +source "drivers/soc/hisilicon/Kconfig"
>  source "drivers/soc/imx/Kconfig"
>  source "drivers/soc/ixp4xx/Kconfig"
>  source "drivers/soc/litex/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 3b0f9fb3b5c8..531f46f3ad98 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_MACH_DOVE)		+= dove/
>  obj-y				+= fsl/
>  obj-y				+= fujitsu/
>  obj-$(CONFIG_ARCH_GEMINI)	+= gemini/
> +obj-y				+= hisilicon/
>  obj-y				+= imx/
>  obj-y				+= ixp4xx/
>  obj-$(CONFIG_SOC_XWAY)		+= lantiq/
> diff --git a/drivers/soc/hisilicon/Kconfig b/drivers/soc/hisilicon/Kconfig
> new file mode 100644
> index 000000000000..81768d47f572
> --- /dev/null
> +++ b/drivers/soc/hisilicon/Kconfig
> @@ -0,0 +1,18 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +menu "Hisilicon SoC drivers"
> +	depends on ARCH_HISI
> +
> +config KUNPENG_HCCS
> +	tristate "HCCS driver on Kunpeng SoC"
> +	depends on ARM64 && ACPI
> +	help
> +	  The Huawei Cache-Coherent System (HCCS) is a bus protocol standard
> +	  for ensuring cache coherent on HiSilicon SoC. The performance of
> +	  the application may be affected if some hccs ports are in non-full
> +	  lane status, have a large number of CRC errors and so on.
> +
> +	  Say M here if you want to include support for querying the health
> +	  status and port information of HCCS on Kunpeng SoC.
> +
> +endmenu
> diff --git a/drivers/soc/hisilicon/Makefile b/drivers/soc/hisilicon/Makefile
> new file mode 100644
> index 000000000000..226e747e70d6
> --- /dev/null
> +++ b/drivers/soc/hisilicon/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_KUNPENG_HCCS)	+= kunpeng_hccs.o
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
> new file mode 100644
> index 000000000000..2b52f7dedc78
> --- /dev/null
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
> @@ -0,0 +1,1296 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * The Huawei Cache-Coherent System (HCCS) is a bus protocol standard for
> + * ensuring cache coherent on HiSilicon SoC.
> + *
> + * Copyright (c) 2023 Hisilicon Limited.
> + * Author: Huisong Li <lihuisong@huawei.com>
> + *
> + * HCCS driver for Kunpeng SoC provides the following features:
> + * - Retrieve info as belows each port:
> + *    - port type
> + *    - lane mode
> + *    - using status
> + *    - current lane mode
> + *    - link state machine
> + *    - lane mask
> + *    - CRC error count
> + *
> + * - Retrieve info as belows all ports on die or chip:
> + *    - if all used ports are in linked
> + *    - if all linked ports are in full lane
> + *    - CRC error count sum
> + */
> +#include <linux/sysfs.h>
> +#include <linux/acpi.h>
> +#include <linux/io.h>
> +#include <linux/kobject.h>
> +#include <linux/iopoll.h>
> +#include <linux/platform_device.h>
> +#include <acpi/pcc.h>
> +
> +#include "kunpeng_hccs.h"
> +
> +/* PCC defines */
> +#define HCCS_PCC_SIGNATURE_MASK		0x50434300
> +#define HCCS_PCC_STATUS_CMD_COMPLETE	BIT(0)
> +
> +/*
> + * Arbitrary retries in case the remote processor is slow to respond
> + * to PCC commands
> + */
> +#define HCCS_PCC_CMD_WAIT_RETRIES_NUM		500ULL
> +#define HCCS_POLL_STATUS_TIME_INTERVAL_US	3
> +
> +static struct hccs_port_info *kobj_to_port_info(struct kobject *k)
> +{
> +	return container_of(k, struct hccs_port_info, kobj);
> +}
> +
> +static struct hccs_die_info *kobj_to_die_info(struct kobject *k)
> +{
> +	return container_of(k, struct hccs_die_info, kobj);
> +}
> +
> +static struct hccs_chip_info *kobj_to_chip_info(struct kobject *k)
> +{
> +	return container_of(k, struct hccs_chip_info, kobj);
> +}
> +
> +static bool hccs_dev_property_supported(struct hccs_dev *hdev)
> +{
> +	struct device *dev = hdev->dev;
> +
> +	if (hdev->intr_mode) {
> +		dev_err(dev, "interrupt mode is unsupported.\n");
> +		return false;
> +	}
> +
> +	if (hdev->type >= ACPI_PCCT_TYPE_EXT_PCC_MASTER_SUBSPACE) {
> +		dev_err(dev, "PCC type-%u is unsupported.\n", hdev->type);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static int hccs_get_device_property(struct hccs_dev *hdev)
> +{
> +	struct device *dev = hdev->dev;
> +
> +	if (device_property_read_u32(dev, "device-flags", &hdev->flags)) {
> +		dev_err(hdev->dev, "no device-flags property.\n");
> +		return -ENODEV;
> +	}
> +
> +	if (device_property_read_u8(dev, "pcc-type", &hdev->type)) {
> +		dev_err(hdev->dev, "no pcc-type property.\n");
> +		return -ENODEV;
> +	}
> +
> +	if (device_property_read_u32(dev, "pcc-chan-id", &hdev->chan_id)) {
> +		dev_err(hdev->dev, "no pcc-channel property.\n");
> +		return -ENODEV;
> +	}
> +
> +	hdev->intr_mode = hccs_get_bit(hdev->flags, HCCS_DEV_FLAGS_INTR_B);
> +	if (!hccs_dev_property_supported(hdev))

Maybe leave a comment that all these are ACPI-only and are not allowed
in DT usage. Without bindings we are not going to review them. OTOH, I
think we don't have a process for such cases in general.

> +		return -EOPNOTSUPP;
> +
> +	return 0;
> +}
> +

(...)

> +static struct attribute *hccs_chip_default_attrs[] = {
> +	&all_linked_on_chip_attr.attr,
> +	&linked_full_lane_on_chip_attr.attr,
> +	&crc_err_cnt_sum_on_chip_attr.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(hccs_chip_default);
> +
> +static const struct kobj_type hccs_chip_type = {
> +	.sysfs_ops = &hccs_comm_ops,
> +	.default_groups = hccs_chip_default_groups,

Missing sysfs documentation.

This also looks like:
1. Duplicating parts of socinfo.
2. Open-coding coding sysfs API.

Anyway, after describing this API it might turn out it is not valid for
upstream usage...

> +};
> +
> +static void hccs_remove_die_dir(struct hccs_die_info *die)
> +{
> +	struct hccs_port_info *port;
> +	u8 i;
> +
> +	for (i = 0; i < die->port_num; i++) {
> +		port = &die->ports[i];
> +		if (port->dir_created)
> +			kobject_put(&port->kobj);
> +	}
> +
> +	kobject_put(&die->kobj);
> +}
> +
> +static void hccs_remove_chip_dir(struct hccs_chip_info *chip)
> +{
> +	struct hccs_die_info *die;
> +	u8 i;
> +
> +	for (i = 0; i < chip->die_num; i++) {
> +		die = &chip->dies[i];
> +		if (die->dir_created)
> +			hccs_remove_die_dir(die);
> +	}
> +
> +	kobject_put(&chip->kobj);
> +}
> +
> +static void hccs_remove_topo_dirs(struct hccs_dev *hdev)
> +{
> +	u8 i;
> +
> +	for (i = 0; i < hdev->chip_num; i++)
> +		hccs_remove_chip_dir(&hdev->chips[i]);
> +}
> +
> +static int hccs_create_hccs_dir(struct hccs_dev *hdev,
> +				struct hccs_die_info *die,
> +				struct hccs_port_info *port)
> +{
> +	int ret;
> +
> +	ret = kobject_init_and_add(&port->kobj, &hccs_port_type,
> +				   &die->kobj, "hccs%d", port->port_id);
> +	if (ret) {
> +		kobject_put(&port->kobj);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hccs_create_die_dir(struct hccs_dev *hdev,
> +			       struct hccs_chip_info *chip,
> +			       struct hccs_die_info *die)
> +{
> +	struct hccs_port_info *port;
> +	int ret;
> +	u16 i;
> +
> +	ret = kobject_init_and_add(&die->kobj, &hccs_die_type,
> +				   &chip->kobj, "die%d", die->die_id);
> +	if (ret) {
> +		kobject_put(&die->kobj);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < die->port_num; i++) {
> +		port = &die->ports[i];
> +		ret = hccs_create_hccs_dir(hdev, die, port);
> +		if (ret) {
> +			dev_err(hdev->dev, "create hccs%d dir failed.\n",
> +				port->port_id);
> +			goto err;
> +		}
> +		port->dir_created = true;
> +	}
> +
> +	return 0;
> +err:
> +	hccs_remove_die_dir(die);
> +
> +	return ret;
> +}
> +
> +static int hccs_create_chip_dir(struct hccs_dev *hdev,
> +				struct hccs_chip_info *chip)
> +{
> +	struct hccs_die_info *die;
> +	int ret;
> +	u16 id;
> +
> +	ret = kobject_init_and_add(&chip->kobj, &hccs_chip_type,
> +				   &hdev->dev->kobj, "chip%d", chip->chip_id);
> +	if (ret) {
> +		kobject_put(&chip->kobj);
> +		return ret;
> +	}
> +
> +	for (id = 0; id < chip->die_num; id++) {
> +		die = &chip->dies[id];
> +		ret = hccs_create_die_dir(hdev, chip, die);
> +		if (ret)
> +			goto err;
> +		die->dir_created = true;
> +	}
> +
> +	return 0;
> +err:
> +	hccs_remove_chip_dir(chip);
> +
> +	return ret;
> +}
> +
> +static int hccs_create_topo_dirs(struct hccs_dev *hdev)
> +{
> +	struct hccs_chip_info *chip;
> +	u8 id, k;
> +	int ret;
> +
> +	for (id = 0; id < hdev->chip_num; id++) {
> +		chip = &hdev->chips[id];
> +		ret = hccs_create_chip_dir(hdev, chip);
> +		if (ret) {
> +			dev_err(hdev->dev, "init chip%d dir failed!\n", id);
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> +err:
> +	for (k = 0; k < id; k++)
> +		hccs_remove_chip_dir(&hdev->chips[k]);
> +
> +	return ret;
> +}
> +
> +static int hccs_probe(struct platform_device *pdev)
> +{
> +	struct acpi_device *acpi_dev;
> +	struct hccs_dev *hdev;
> +	int rc;
> +
> +	pr_info("kunpeng_hccs is initializing.\n");

Drop.

> +
> +	if (acpi_disabled) {
> +		dev_err(&pdev->dev, "acpi is disabled.\n");

Is it possible for your ACPI-only driver?


> +		return -ENODEV;
> +	}
> +	acpi_dev = ACPI_COMPANION(&pdev->dev);
> +	if (!acpi_dev)
> +		return -ENODEV;
> +
> +	hdev = devm_kzalloc(&pdev->dev, sizeof(*hdev), GFP_KERNEL);
> +	if (!hdev)
> +		return -ENOMEM;
> +	hdev->acpi_dev = acpi_dev;
> +	hdev->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, hdev);
> +
> +	rc = hccs_get_device_property(hdev);
> +	if (rc)
> +		return rc;
> +
> +	mutex_init(&hdev->lock);
> +
> +	rc = hccs_register_pcc_channel(hdev);
> +	if (rc)
> +		return rc;
> +
> +	rc = hccs_get_dev_caps(hdev);
> +	if (rc)
> +		goto err_uninit_mbox_client;
> +
> +	rc = hccs_get_hw_info(hdev);
> +	if (rc)
> +		goto err_uninit_mbox_client;
> +
> +	rc = hccs_create_topo_dirs(hdev);
> +	if (rc)
> +		goto err_uninit_mbox_client;
> +
> +	dev_info(&pdev->dev, "HISI HCCS driver registered!\n");

Drop simple probe success msgs. There's already kernel infrastructure
giving you this information.

> +
> +	return 0;
> +
> +err_uninit_mbox_client:
> +	hccs_unregister_pcc_channel(hdev);
> +
> +	return rc;
> +}
> +
> +static int hccs_remove(struct platform_device *pdev)
> +{
> +	struct hccs_dev *hdev = platform_get_drvdata(pdev);
> +
> +	hccs_remove_topo_dirs(hdev);
> +	hccs_unregister_pcc_channel(hdev);
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id hccs_acpi_match[] = {
> +	{ "HISI04B1", },
> +};
> +
> +static struct platform_driver hccs_driver = {
> +	.probe = hccs_probe,
> +	.remove = hccs_remove,
> +	.driver = {
> +		.name = "kunpeng_hccs",
> +		.acpi_match_table = ACPI_PTR(hccs_acpi_match),

Drop ACPI_PTR as it is not balanced with __maybe_unused.

Best regards,
Krzysztof