[PATCH v1 0/3] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[PATCH v1 0/3] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[Huisong Li]

Fix some incorrect format strings and add failure log for no _CRS method.
Add support for the platform with PCC type3 and interrupt ack.

Huisong Li (3):
  soc: hisilicon: kunpeng_hccs: Fix some incorrect format strings
  soc: hisilicon: kunpeng_hccs: Add failure log for no _CRS method
  soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and
    interrupt ack

 drivers/soc/hisilicon/kunpeng_hccs.c | 153 ++++++++++++++++++++-------
 drivers/soc/hisilicon/kunpeng_hccs.h |   2 +
 2 files changed, 119 insertions(+), 36 deletions(-)

-- 
2.33.0

[PATCH v1 1/3] soc: hisilicon: kunpeng_hccs: Fix some incorrect format strings

[Huisong Li]

Fix some incorrect format strings.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index e31791659560..dad6235dbf1a 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -155,7 +155,7 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 		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-%d.\n",
+			dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n",
 				hdev->chan_id);
 			rc = -ENOMEM;
 			goto err_mbx_channel_free;
@@ -1097,7 +1097,7 @@ static int hccs_create_hccs_dir(struct hccs_dev *hdev,
 	int ret;
 
 	ret = kobject_init_and_add(&port->kobj, &hccs_port_type,
-				   &die->kobj, "hccs%d", port->port_id);
+				   &die->kobj, "hccs%u", port->port_id);
 	if (ret) {
 		kobject_put(&port->kobj);
 		return ret;
@@ -1115,7 +1115,7 @@ static int hccs_create_die_dir(struct hccs_dev *hdev,
 	u16 i;
 
 	ret = kobject_init_and_add(&die->kobj, &hccs_die_type,
-				   &chip->kobj, "die%d", die->die_id);
+				   &chip->kobj, "die%u", die->die_id);
 	if (ret) {
 		kobject_put(&die->kobj);
 		return ret;
@@ -1125,7 +1125,7 @@ static int hccs_create_die_dir(struct hccs_dev *hdev,
 		port = &die->ports[i];
 		ret = hccs_create_hccs_dir(hdev, die, port);
 		if (ret) {
-			dev_err(hdev->dev, "create hccs%d dir failed.\n",
+			dev_err(hdev->dev, "create hccs%u dir failed.\n",
 				port->port_id);
 			goto err;
 		}
@@ -1147,7 +1147,7 @@ static int hccs_create_chip_dir(struct hccs_dev *hdev,
 	u16 id;
 
 	ret = kobject_init_and_add(&chip->kobj, &hccs_chip_type,
-				   &hdev->dev->kobj, "chip%d", chip->chip_id);
+				   &hdev->dev->kobj, "chip%u", chip->chip_id);
 	if (ret) {
 		kobject_put(&chip->kobj);
 		return ret;
@@ -1178,7 +1178,7 @@ static int hccs_create_topo_dirs(struct hccs_dev *hdev)
 		chip = &hdev->chips[id];
 		ret = hccs_create_chip_dir(hdev, chip);
 		if (ret) {
-			dev_err(hdev->dev, "init chip%d dir failed!\n", id);
+			dev_err(hdev->dev, "init chip%u dir failed!\n", id);
 			goto err;
 		}
 	}
-- 
2.33.0

[PATCH v1 2/3] soc: hisilicon: kunpeng_hccs: Add failure log for no _CRS method

[Huisong Li]

Driver gets the PCC channel id by using the PCC GAS in _CRS.
But, currently, if the firmware has no _CRS method on platform, there
is not any failure log. So this patch adds the log for this.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index dad6235dbf1a..fd3ca0eb8175 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -85,8 +85,10 @@ static int hccs_get_pcc_chan_id(struct hccs_dev *hdev)
 	struct hccs_register_ctx ctx = {0};
 	acpi_status status;
 
-	if (!acpi_has_method(handle, METHOD_NAME__CRS))
+	if (!acpi_has_method(handle, METHOD_NAME__CRS)) {
+		dev_err(hdev->dev, "No _CRS method.\n");
 		return -ENODEV;
+	}
 
 	ctx.dev = hdev->dev;
 	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
-- 
2.33.0

[PATCH v1 3/3] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[Huisong Li]

Support the platform with PCC type3 and interrupt ack.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 137 +++++++++++++++++++++------
 drivers/soc/hisilicon/kunpeng_hccs.h |   2 +
 2 files changed, 110 insertions(+), 29 deletions(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index fd3ca0eb8175..96cdac7be244 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -38,6 +38,11 @@
 #define HCCS_PCC_CMD_WAIT_RETRIES_NUM		500ULL
 #define HCCS_POLL_STATUS_TIME_INTERVAL_US	3
 
+enum hccs_hw_device_version {
+	HCCS_HW_DEVICE_V1 = 0,
+	HCCS_HW_DEVICE_V2 = 1,
+};
+
 static struct hccs_port_info *kobj_to_port_info(struct kobject *k)
 {
 	return container_of(k, struct hccs_port_info, kobj);
@@ -100,6 +105,21 @@ static int hccs_get_pcc_chan_id(struct hccs_dev *hdev)
 	return 0;
 }
 
+static int hccs_get_device_version(struct hccs_dev *hdev)
+{
+	const struct acpi_device_id *acpi_id;
+
+	acpi_id = acpi_match_device(hdev->dev->driver->acpi_match_table,
+				    hdev->dev);
+	if (!acpi_id) {
+		dev_err(hdev->dev, "get device version failed.");
+		return -EINVAL;
+	}
+
+	hdev->dev_ver = (u8)acpi_id->driver_data;
+	return 0;
+}
+
 static void hccs_chan_tx_done(struct mbox_client *cl, void *msg, int ret)
 {
 	if (ret < 0)
@@ -110,6 +130,14 @@ static void hccs_chan_tx_done(struct mbox_client *cl, void *msg, int ret)
 			 *(u8 *)msg, ret);
 }
 
+static void hccs_pcc_rx_callback(struct mbox_client *cl, void *m)
+{
+	struct hccs_mbox_client_info *cl_info =
+			container_of(cl, struct hccs_mbox_client_info, client);
+
+	complete(&cl_info->done);
+}
+
 static void hccs_unregister_pcc_channel(struct hccs_dev *hdev)
 {
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
@@ -131,6 +159,11 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 	cl->tx_block = false;
 	cl->knows_txdone = true;
 	cl->tx_done = hccs_chan_tx_done;
+	if (hdev->dev_ver == HCCS_HW_DEVICE_V2) {
+		cl->rx_callback = hccs_pcc_rx_callback;
+		init_completion(&cl_info->done);
+	}
+
 	pcc_chan = pcc_mbox_request_channel(cl, hdev->chan_id);
 	if (IS_ERR(pcc_chan)) {
 		dev_err(dev, "PPC channel request failed.\n");
@@ -147,10 +180,16 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 	 */
 	cl_info->deadline_us =
 			HCCS_PCC_CMD_WAIT_RETRIES_NUM * pcc_chan->latency;
-	if (cl_info->mbox_chan->mbox->txdone_irq) {
+	if (hdev->dev_ver == HCCS_HW_DEVICE_V1 &&
+	    cl_info->mbox_chan->mbox->txdone_irq) {
 		dev_err(dev, "PCC IRQ in PCCT is enabled.\n");
 		rc = -EINVAL;
 		goto err_mbx_channel_free;
+	} else if (hdev->dev_ver == HCCS_HW_DEVICE_V2 &&
+		   !cl_info->mbox_chan->mbox->txdone_irq) {
+		dev_err(dev, "PCC IRQ in PCCT isn't supported.\n");
+		rc = -EINVAL;
+		goto err_mbx_channel_free;
 	}
 
 	if (pcc_chan->shmem_base_addr) {
@@ -175,49 +214,81 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
 {
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
-	struct acpi_pcct_shared_memory __iomem *comm_base =
-							cl_info->pcc_comm_addr;
+	struct acpi_pcct_shared_memory __iomem *comm_base;
+	int ret = 0;
 	u16 status;
-	int ret;
 
 	/*
 	 * Poll PCC status register every 3us(delay_us) for maximum of
 	 * deadline_us(timeout_us) until PCC command complete bit is set(cond)
 	 */
-	ret = readw_poll_timeout(&comm_base->status, status,
-				 status & PCC_STATUS_CMD_COMPLETE,
-				 HCCS_POLL_STATUS_TIME_INTERVAL_US,
-				 cl_info->deadline_us);
-	if (unlikely(ret))
-		dev_err(hdev->dev, "poll PCC status failed, ret = %d.\n", ret);
+	if (hdev->dev_ver == HCCS_HW_DEVICE_V1) {
+		comm_base = cl_info->pcc_comm_addr;
+		ret = readw_poll_timeout(&comm_base->status, status,
+					status & PCC_STATUS_CMD_COMPLETE,
+					HCCS_POLL_STATUS_TIME_INTERVAL_US,
+					cl_info->deadline_us);
+		if (unlikely(ret))
+			dev_err(hdev->dev, "poll PCC status failed, ret = %d.\n", ret);
+	} else {
+		if (!wait_for_completion_timeout(&cl_info->done,
+				usecs_to_jiffies(cl_info->deadline_us))) {
+			dev_err(hdev->dev, "PCC command executed timeout!\n");
+			ret = -ETIMEDOUT;
+		}
+	}
 
 	return ret;
 }
 
+static void hccs_fill_pcc_shared_mem_region(struct hccs_dev *hdev, u8 cmd,
+					    struct hccs_desc *desc,
+					    void __iomem *comm_space,
+					    u16 space_size)
+{
+	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
+	struct acpi_pcct_ext_pcc_shared_memory tmp1 = {0};
+	struct acpi_pcct_shared_memory tmp2 = {0};
+
+	if (hdev->dev_ver == HCCS_HW_DEVICE_V1) {
+		tmp2.signature = PCC_SIGNATURE | hdev->chan_id;
+		tmp2.command = cmd;
+		tmp2.status = 0;
+		memcpy_toio(cl_info->pcc_comm_addr, (void *)&tmp2,
+			    sizeof(struct acpi_pcct_shared_memory));
+	} else {
+		tmp1.signature = PCC_SIGNATURE | hdev->chan_id;
+		tmp1.command = cmd;
+		tmp1.flags = PCC_CMD_COMPLETION_NOTIFY;
+		tmp1.length = HCCS_PCC_SHARE_MEM_BYTES;
+		memcpy_toio(cl_info->pcc_comm_addr, (void *)&tmp1,
+			    sizeof(struct acpi_pcct_ext_pcc_shared_memory));
+	}
+
+	/* Copy the message to the PCC comm space */
+	memcpy_toio(comm_space, (void *)desc, space_size);
+}
+
 static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 			     struct hccs_desc *desc)
 {
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
-	void __iomem *comm_space = cl_info->pcc_comm_addr +
-					sizeof(struct acpi_pcct_shared_memory);
 	struct hccs_fw_inner_head *fw_inner_head;
-	struct acpi_pcct_shared_memory tmp = {0};
-	u16 comm_space_size;
+	void __iomem *comm_space;
+	u16 shared_mem_size;
+	u16 space_size;
 	int ret;
 
-	/* Write signature for this subspace */
-	tmp.signature = PCC_SIGNATURE | hdev->chan_id;
-	/* Write to the shared command region */
-	tmp.command = cmd;
-	/* Clear cmd complete bit */
-	tmp.status = 0;
-	memcpy_toio(cl_info->pcc_comm_addr, (void *)&tmp,
-			sizeof(struct acpi_pcct_shared_memory));
+	shared_mem_size = hdev->dev_ver == HCCS_HW_DEVICE_V1 ?
+			sizeof(struct acpi_pcct_shared_memory) :
+			sizeof(struct acpi_pcct_ext_pcc_shared_memory);
+	comm_space = cl_info->pcc_comm_addr + shared_mem_size;
+	space_size = HCCS_PCC_SHARE_MEM_BYTES - shared_mem_size;
 
-	/* Copy the message to the PCC comm space */
-	comm_space_size = HCCS_PCC_SHARE_MEM_BYTES -
-				sizeof(struct acpi_pcct_shared_memory);
-	memcpy_toio(comm_space, (void *)desc, comm_space_size);
+	hccs_fill_pcc_shared_mem_region(hdev, cmd, desc,
+					comm_space, space_size);
+	if (cl_info->mbox_chan->mbox->txdone_irq)
+		reinit_completion(&cl_info->done);
 
 	/* Ring doorbell */
 	ret = mbox_send_message(cl_info->mbox_chan, &cmd);
@@ -233,7 +304,7 @@ static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 		goto end;
 
 	/* Copy response data */
-	memcpy_fromio((void *)desc, comm_space, comm_space_size);
+	memcpy_fromio((void *)desc, comm_space, space_size);
 	fw_inner_head = &desc->rsp.fw_inner_head;
 	if (fw_inner_head->retStatus) {
 		dev_err(hdev->dev, "Execute PCC command failed, error code = %u.\n",
@@ -242,7 +313,10 @@ static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 	}
 
 end:
-	mbox_client_txdone(cl_info->mbox_chan, ret);
+	if (cl_info->mbox_chan->mbox->txdone_irq)
+		mbox_chan_txdone(cl_info->mbox_chan, ret);
+	else
+		mbox_client_txdone(cl_info->mbox_chan, ret);
 	return ret;
 }
 
@@ -1214,6 +1288,10 @@ static int hccs_probe(struct platform_device *pdev)
 	hdev->dev = &pdev->dev;
 	platform_set_drvdata(pdev, hdev);
 
+	rc = hccs_get_device_version(hdev);
+	if (rc)
+		return rc;
+
 	mutex_init(&hdev->lock);
 	rc = hccs_get_pcc_chan_id(hdev);
 	if (rc)
@@ -1251,7 +1329,8 @@ static void hccs_remove(struct platform_device *pdev)
 }
 
 static const struct acpi_device_id hccs_acpi_match[] = {
-	{ "HISI04B1"},
+	{ "HISI04B1", HCCS_HW_DEVICE_V1},
+	{ "HISI04B2", HCCS_HW_DEVICE_V2},
 	{ ""},
 };
 MODULE_DEVICE_TABLE(acpi, hccs_acpi_match);
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
index 6012d2776028..bbb1aada0c6c 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.h
+++ b/drivers/soc/hisilicon/kunpeng_hccs.h
@@ -51,12 +51,14 @@ struct hccs_mbox_client_info {
 	struct pcc_mbox_chan *pcc_chan;
 	u64 deadline_us;
 	void __iomem *pcc_comm_addr;
+	struct completion done;
 };
 
 struct hccs_dev {
 	struct device *dev;
 	struct acpi_device *acpi_dev;
 	u64 caps;
+	u8 dev_ver;
 	u8 chip_num;
 	struct hccs_chip_info *chips;
 	u8 chan_id;
-- 
2.33.0

Re: [PATCH v1 2/3] soc: hisilicon: kunpeng_hccs: Add failure log for no _CRS method

[Jonathan Cameron]

On Thu, 9 Nov 2023 13:45:25 +0800
Huisong Li <lihuisong@huawei.com> wrote:

> Driver gets the PCC channel id by using the PCC GAS in _CRS.
> But, currently, if the firmware has no _CRS method on platform, there
> is not any failure log. So this patch adds the log for this.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
Make sense to me.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v1 1/3] soc: hisilicon: kunpeng_hccs: Fix some incorrect format strings

[Jonathan Cameron]

On Thu, 9 Nov 2023 13:45:24 +0800
Huisong Li <lihuisong@huawei.com> wrote:

> Fix some incorrect format strings.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
These are indeed all unsigned.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH v1 3/3] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[Jonathan Cameron]

On Thu, 9 Nov 2023 13:45:26 +0800
Huisong Li <lihuisong@huawei.com> wrote:

> Support the platform with PCC type3 and interrupt ack.
Probably mention this is version 2 as that's what you call it
in the code.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>

Hi.

Main comment in here is that it is almost always better to use
a version specific structure with callbacks / data etc rather than
have instances of if (version1) do_x; else if (version2) do_y;

It ends up pulling all the differences into one place + allows a
great deal more flexibility.  See inline for details.

Otherwise looks fine to me

Jonathan

> ---
>  drivers/soc/hisilicon/kunpeng_hccs.c | 137 +++++++++++++++++++++------
>  drivers/soc/hisilicon/kunpeng_hccs.h |   2 +
>  2 files changed, 110 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
> index fd3ca0eb8175..96cdac7be244 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
> @@ -38,6 +38,11 @@
...

>  
> +static int hccs_get_device_version(struct hccs_dev *hdev)
> +{
> +	const struct acpi_device_id *acpi_id;
> +
> +	acpi_id = acpi_match_device(hdev->dev->driver->acpi_match_table,
> +				    hdev->dev);


Why not just have
hdev->dev_ver = (u8)acpi_device_get_match_data(&hdev->dev);
inline where this is called?

You probably don't even need the error check as you can't get here
without an appropriate match as the driver would never be matched.

> +	if (!acpi_id) {
> +		dev_err(hdev->dev, "get device version failed.");
> +		return -EINVAL;
> +	}
> +
> +	hdev->dev_ver = (u8)acpi_id->driver_data;
> +	return 0;
> +}
> +

...

>  static void hccs_unregister_pcc_channel(struct hccs_dev *hdev)
>  {
>  	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
> @@ -131,6 +159,11 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
>  	cl->tx_block = false;
>  	cl->knows_txdone = true;
>  	cl->tx_done = hccs_chan_tx_done;
> +	if (hdev->dev_ver == HCCS_HW_DEVICE_V2) {

I'd prefer to see this done as data. That is, instead of having an enum
used in the ACPI match data, have a pointer to a struct with the callback.
Then this will become something like

	hcc_info = acpi_device_get_match_data(hdev);
	cl->rx_callback = hcc_info->rx_callback;
	init_completion(&cl_info->done);

Initializing the completion is harmless if it's not used, so just do it
unconditionally.

> +		cl->rx_callback = hccs_pcc_rx_callback;
> +		init_completion(&cl_info->done);
> +	}
> +
>  	pcc_chan = pcc_mbox_request_channel(cl, hdev->chan_id);
>  	if (IS_ERR(pcc_chan)) {
>  		dev_err(dev, "PPC channel request failed.\n");
> @@ -147,10 +180,16 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
>  	 */
>  	cl_info->deadline_us =
>  			HCCS_PCC_CMD_WAIT_RETRIES_NUM * pcc_chan->latency;
> -	if (cl_info->mbox_chan->mbox->txdone_irq) {
> +	if (hdev->dev_ver == HCCS_HW_DEVICE_V1 &&
> +	    cl_info->mbox_chan->mbox->txdone_irq) {

Also data in hcc_info would be better than version number based
code flow.
	if (hcc_info->has_txdone_irq &&
	    cl_info->mbox_chan->mbox->rx_done_irq) {
	....
	} else if (!hcc_info->has_txdone_irq &&
		   !cl_info->mbox_chan->mbox->tx_done_irq) {
	...
	}
>  		dev_err(dev, "PCC IRQ in PCCT is enabled.\n");
>  		rc = -EINVAL;
>  		goto err_mbx_channel_free;
> +	} else if (hdev->dev_ver == HCCS_HW_DEVICE_V2 &&
> +		   !cl_info->mbox_chan->mbox->txdone_irq) {
> +		dev_err(dev, "PCC IRQ in PCCT isn't supported.\n");
> +		rc = -EINVAL;
> +		goto err_mbx_channel_free;
>  	}
>  
>  	if (pcc_chan->shmem_base_addr) {
> @@ -175,49 +214,81 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
>  static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
>  {
>  	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
> -	struct acpi_pcct_shared_memory __iomem *comm_base =
> -							cl_info->pcc_comm_addr;
> +	struct acpi_pcct_shared_memory __iomem *comm_base;
> +	int ret = 0;
>  	u16 status;
> -	int ret;
>  
>  	/*
>  	 * Poll PCC status register every 3us(delay_us) for maximum of
>  	 * deadline_us(timeout_us) until PCC command complete bit is set(cond)
>  	 */
> -	ret = readw_poll_timeout(&comm_base->status, status,
> -				 status & PCC_STATUS_CMD_COMPLETE,
> -				 HCCS_POLL_STATUS_TIME_INTERVAL_US,
> -				 cl_info->deadline_us);
> -	if (unlikely(ret))
> -		dev_err(hdev->dev, "poll PCC status failed, ret = %d.\n", ret);
> +	if (hdev->dev_ver == HCCS_HW_DEVICE_V1) {

As above. I'd prefer to see this as a call back in an info structure rather
than code here.

> +		comm_base = cl_info->pcc_comm_addr;
> +		ret = readw_poll_timeout(&comm_base->status, status,
> +					status & PCC_STATUS_CMD_COMPLETE,
> +					HCCS_POLL_STATUS_TIME_INTERVAL_US,
> +					cl_info->deadline_us);
> +		if (unlikely(ret))
> +			dev_err(hdev->dev, "poll PCC status failed, ret = %d.\n", ret);
> +	} else {
> +		if (!wait_for_completion_timeout(&cl_info->done,
> +				usecs_to_jiffies(cl_info->deadline_us))) {
> +			dev_err(hdev->dev, "PCC command executed timeout!\n");
> +			ret = -ETIMEDOUT;
> +		}
> +	}
>  
>  	return ret;
>  }
>  
> +static void hccs_fill_pcc_shared_mem_region(struct hccs_dev *hdev, u8 cmd,
> +					    struct hccs_desc *desc,
> +					    void __iomem *comm_space,
> +					    u16 space_size)
> +{
> +	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
> +	struct acpi_pcct_ext_pcc_shared_memory tmp1 = {0};
> +	struct acpi_pcct_shared_memory tmp2 = {0};
> +
> +	if (hdev->dev_ver == HCCS_HW_DEVICE_V1) {
1) tmp1 and temp2 are local to the two forks of this statement
so pull them down here.
2) Use c99 style struct init to make this cleaner.
		struct acpi_pcct_shared_memory = {
			.signature = x,
			.command = cmd,
		};

I'd also like this to be a callback in the version specific info
structure rather than done as an if / else here that really doesn't
extend well if we get a lot more versions doing it differently.

> +		tmp2.signature = PCC_SIGNATURE | hdev->chan_id;
> +		tmp2.command = cmd;
> +		tmp2.status = 0;
> +		memcpy_toio(cl_info->pcc_comm_addr, (void *)&tmp2,
> +			    sizeof(struct acpi_pcct_shared_memory));
> +	} else {
> +		tmp1.signature = PCC_SIGNATURE | hdev->chan_id;
> +		tmp1.command = cmd;
> +		tmp1.flags = PCC_CMD_COMPLETION_NOTIFY;
> +		tmp1.length = HCCS_PCC_SHARE_MEM_BYTES;
> +		memcpy_toio(cl_info->pcc_comm_addr, (void *)&tmp1,
> +			    sizeof(struct acpi_pcct_ext_pcc_shared_memory));
> +	}
> +
> +	/* Copy the message to the PCC comm space */
> +	memcpy_toio(comm_space, (void *)desc, space_size);
> +}
...


> @@ -1214,6 +1288,10 @@ static int hccs_probe(struct platform_device *pdev)
>  	hdev->dev = &pdev->dev;
>  	platform_set_drvdata(pdev, hdev);
>  
> +	rc = hccs_get_device_version(hdev);
> +	if (rc)
> +		return rc;
> +
>  	mutex_init(&hdev->lock);
>  	rc = hccs_get_pcc_chan_id(hdev);
>  	if (rc)
> @@ -1251,7 +1329,8 @@ static void hccs_remove(struct platform_device *pdev)
>  }
>  
>  static const struct acpi_device_id hccs_acpi_match[] = {
> -	{ "HISI04B1"},
> +	{ "HISI04B1", HCCS_HW_DEVICE_V1},
> +	{ "HISI04B2", HCCS_HW_DEVICE_V2},
>  	{ ""},
Side comment, but 
	{}
So no content (rely on c initializing it to be zero filled anyway and no comma
as we don't want anything after this point.

>  };
>  MODULE_DEVICE_TABLE(acpi, hccs_acpi_match);
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
> index 6012d2776028..bbb1aada0c6c 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.h
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.h
> @@ -51,12 +51,14 @@ struct hccs_mbox_client_info {
>  	struct pcc_mbox_chan *pcc_chan;
>  	u64 deadline_us;
>  	void __iomem *pcc_comm_addr;
> +	struct completion done;
>  };
>  
>  struct hccs_dev {
>  	struct device *dev;
>  	struct acpi_device *acpi_dev;
>  	u64 caps;
> +	u8 dev_ver;
See above, but I'd rather see
	const struct hcc_verspecific_info *info;
here that encodes the differences as callbacks and data and rather than code.
>  	u8 chip_num;
>  	struct hccs_chip_info *chips;
>  	u8 chan_id;

Re: [PATCH v1 3/3] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[lihuisong (C)]

在 2023/11/28 23:44, Jonathan Cameron 写道:
> On Thu, 9 Nov 2023 13:45:26 +0800
> Huisong Li <lihuisong@huawei.com> wrote:
>
>> Support the platform with PCC type3 and interrupt ack.
> Probably mention this is version 2 as that's what you call it
> in the code.
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Hi.
>
> Main comment in here is that it is almost always better to use
> a version specific structure with callbacks / data etc rather than
> have instances of if (version1) do_x; else if (version2) do_y;
>
> It ends up pulling all the differences into one place + allows a
> great deal more flexibility.  See inline for details.
Hi Jonathan,

Thanks for reviewing this patch.
It is a good idea to use a version specific structure as driver data.
will be fixed in next version. Please take a look at it later.😁
Thanks for your advice.

BR,
Huisong
>> ---
>>   drivers/soc/hisilicon/kunpeng_hccs.c | 137 +++++++++++++++++++++------
>>   drivers/soc/hisilicon/kunpeng_hccs.h |   2 +
>>   2 files changed, 110 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
>> index fd3ca0eb8175..96cdac7be244 100644
>> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
>> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
>> @@ -38,6 +38,11 @@
> ...
>
>>   
>> +static int hccs_get_device_version(struct hccs_dev *hdev)
>> +{
>> +	const struct acpi_device_id *acpi_id;
>> +
>> +	acpi_id = acpi_match_device(hdev->dev->driver->acpi_match_table,
>> +				    hdev->dev);
>
> Why not just have
> hdev->dev_ver = (u8)acpi_device_get_match_data(&hdev->dev);
> inline where this is called?
>
> You probably don't even need the error check as you can't get here
> without an appropriate match as the driver would never be matched.
Yes, it could be ok.
This will be replaced by a version sepcific info according to your advice.
>
>> +	if (!acpi_id) {
>> +		dev_err(hdev->dev, "get device version failed.");
>> +		return -EINVAL;
>> +	}
>> +
>> +	hdev->dev_ver = (u8)acpi_id->driver_data;
>> +	return 0;
>> +}
>> +
> ...
>
>>   static void hccs_unregister_pcc_channel(struct hccs_dev *hdev)
>>   {
>>   	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
>> @@ -131,6 +159,11 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
>>   	cl->tx_block = false;
>>   	cl->knows_txdone = true;
>>   	cl->tx_done = hccs_chan_tx_done;
>> +	if (hdev->dev_ver == HCCS_HW_DEVICE_V2) {
> I'd prefer to see this done as data. That is, instead of having an enum
> used in the ACPI match data, have a pointer to a struct with the callback.
> Then this will become something like
>
> 	hcc_info = acpi_device_get_match_data(hdev);
> 	cl->rx_callback = hcc_info->rx_callback;
> 	init_completion(&cl_info->done);
>
> Initializing the completion is harmless if it's not used, so just do it
> unconditionally.
Ack
>
>> +		cl->rx_callback = hccs_pcc_rx_callback;
>> +		init_completion(&cl_info->done);
>> +	}
>> +
>>   	pcc_chan = pcc_mbox_request_channel(cl, hdev->chan_id);
>>   	if (IS_ERR(pcc_chan)) {
>>   		dev_err(dev, "PPC channel request failed.\n");
>> @@ -147,10 +180,16 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
>>   	 */
>>   	cl_info->deadline_us =
>>   			HCCS_PCC_CMD_WAIT_RETRIES_NUM * pcc_chan->latency;
>> -	if (cl_info->mbox_chan->mbox->txdone_irq) {
>> +	if (hdev->dev_ver == HCCS_HW_DEVICE_V1 &&
>> +	    cl_info->mbox_chan->mbox->txdone_irq) {
> Also data in hcc_info would be better than version number based
> code flow.
> 	if (hcc_info->has_txdone_irq &&
> 	    cl_info->mbox_chan->mbox->rx_done_irq) {
> 	....
> 	} else if (!hcc_info->has_txdone_irq &&
> 		   !cl_info->mbox_chan->mbox->tx_done_irq) {
> 	...
> 	}
Thanks for your example.
>>   		dev_err(dev, "PCC IRQ in PCCT is enabled.\n");
>>   		rc = -EINVAL;
>>   		goto err_mbx_channel_free;
>> +	} else if (hdev->dev_ver == HCCS_HW_DEVICE_V2 &&
>> +		   !cl_info->mbox_chan->mbox->txdone_irq) {
>> +		dev_err(dev, "PCC IRQ in PCCT isn't supported.\n");
>> +		rc = -EINVAL;
>> +		goto err_mbx_channel_free;
>>   	}
>>   
>>   	if (pcc_chan->shmem_base_addr) {
>> @@ -175,49 +214,81 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
>>   static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
>>   {
>>   	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
>> -	struct acpi_pcct_shared_memory __iomem *comm_base =
>> -							cl_info->pcc_comm_addr;
>> +	struct acpi_pcct_shared_memory __iomem *comm_base;
>> +	int ret = 0;
>>   	u16 status;
>> -	int ret;
>>   
>>   	/*
>>   	 * Poll PCC status register every 3us(delay_us) for maximum of
>>   	 * deadline_us(timeout_us) until PCC command complete bit is set(cond)
>>   	 */
>> -	ret = readw_poll_timeout(&comm_base->status, status,
>> -				 status & PCC_STATUS_CMD_COMPLETE,
>> -				 HCCS_POLL_STATUS_TIME_INTERVAL_US,
>> -				 cl_info->deadline_us);
>> -	if (unlikely(ret))
>> -		dev_err(hdev->dev, "poll PCC status failed, ret = %d.\n", ret);
>> +	if (hdev->dev_ver == HCCS_HW_DEVICE_V1) {
> As above. I'd prefer to see this as a call back in an info structure rather
> than code here.
Ack
>> +		comm_base = cl_info->pcc_comm_addr;
>> +		ret = readw_poll_timeout(&comm_base->status, status,
>> +					status & PCC_STATUS_CMD_COMPLETE,
>> +					HCCS_POLL_STATUS_TIME_INTERVAL_US,
>> +					cl_info->deadline_us);
>> +		if (unlikely(ret))
>> +			dev_err(hdev->dev, "poll PCC status failed, ret = %d.\n", ret);
>> +	} else {
>> +		if (!wait_for_completion_timeout(&cl_info->done,
>> +				usecs_to_jiffies(cl_info->deadline_us))) {
>> +			dev_err(hdev->dev, "PCC command executed timeout!\n");
>> +			ret = -ETIMEDOUT;
>> +		}
>> +	}
>>   
>>   	return ret;
>>   }
>>   
>> +static void hccs_fill_pcc_shared_mem_region(struct hccs_dev *hdev, u8 cmd,
>> +					    struct hccs_desc *desc,
>> +					    void __iomem *comm_space,
>> +					    u16 space_size)
>> +{
>> +	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
>> +	struct acpi_pcct_ext_pcc_shared_memory tmp1 = {0};
>> +	struct acpi_pcct_shared_memory tmp2 = {0};
>> +
>> +	if (hdev->dev_ver == HCCS_HW_DEVICE_V1) {
> 1) tmp1 and temp2 are local to the two forks of this statement
> so pull them down here.
> 2) Use c99 style struct init to make this cleaner.
> 		struct acpi_pcct_shared_memory = {
> 			.signature = x,
> 			.command = cmd,
> 		};
>
> I'd also like this to be a callback in the version specific info
> structure rather than done as an if / else here that really doesn't
> extend well if we get a lot more versions doing it differently.
It is both be ok. but will fixed.
>
>> +		tmp2.signature = PCC_SIGNATURE | hdev->chan_id;
>> +		tmp2.command = cmd;
>> +		tmp2.status = 0;
>> +		memcpy_toio(cl_info->pcc_comm_addr, (void *)&tmp2,
>> +			    sizeof(struct acpi_pcct_shared_memory));
>> +	} else {
>> +		tmp1.signature = PCC_SIGNATURE | hdev->chan_id;
>> +		tmp1.command = cmd;
>> +		tmp1.flags = PCC_CMD_COMPLETION_NOTIFY;
>> +		tmp1.length = HCCS_PCC_SHARE_MEM_BYTES;
>> +		memcpy_toio(cl_info->pcc_comm_addr, (void *)&tmp1,
>> +			    sizeof(struct acpi_pcct_ext_pcc_shared_memory));
>> +	}
>> +
>> +	/* Copy the message to the PCC comm space */
>> +	memcpy_toio(comm_space, (void *)desc, space_size);
>> +}
> ...
>
>
>> @@ -1214,6 +1288,10 @@ static int hccs_probe(struct platform_device *pdev)
>>   	hdev->dev = &pdev->dev;
>>   	platform_set_drvdata(pdev, hdev);
>>   
>> +	rc = hccs_get_device_version(hdev);
>> +	if (rc)
>> +		return rc;
>> +
>>   	mutex_init(&hdev->lock);
>>   	rc = hccs_get_pcc_chan_id(hdev);
>>   	if (rc)
>> @@ -1251,7 +1329,8 @@ static void hccs_remove(struct platform_device *pdev)
>>   }
>>   
>>   static const struct acpi_device_id hccs_acpi_match[] = {
>> -	{ "HISI04B1"},
>> +	{ "HISI04B1", HCCS_HW_DEVICE_V1},
>> +	{ "HISI04B2", HCCS_HW_DEVICE_V2},
>>   	{ ""},
> Side comment, but
> 	{}
> So no content (rely on c initializing it to be zero filled anyway and no comma
> as we don't want anything after this point.
correct.
>
>>   };
>>   MODULE_DEVICE_TABLE(acpi, hccs_acpi_match);
>> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
>> index 6012d2776028..bbb1aada0c6c 100644
>> --- a/drivers/soc/hisilicon/kunpeng_hccs.h
>> +++ b/drivers/soc/hisilicon/kunpeng_hccs.h
>> @@ -51,12 +51,14 @@ struct hccs_mbox_client_info {
>>   	struct pcc_mbox_chan *pcc_chan;
>>   	u64 deadline_us;
>>   	void __iomem *pcc_comm_addr;
>> +	struct completion done;
>>   };
>>   
>>   struct hccs_dev {
>>   	struct device *dev;
>>   	struct acpi_device *acpi_dev;
>>   	u64 caps;
>> +	u8 dev_ver;
> See above, but I'd rather see
> 	const struct hcc_verspecific_info *info;
> here that encodes the differences as callbacks and data and rather than code.

Good idea, so ack.

>>   	u8 chip_num;
>>   	struct hccs_chip_info *chips;
>>   	u8 chan_id;
> .

[PATCH v2 0/4] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[Huisong Li]

The main purpose of this series is to support the platform with PCC type3
and interrupt ack. At the same time, this series also fix some incorrect
format strings, add failure log for no _CRS method and remove an unused
blank line.

---
 v2:
  - using a version specific structure to replace device version according
    to Jonathan's advice.
  - add a new patch that remove an unused blank line.

Huisong Li (4):
  soc: hisilicon: kunpeng_hccs: Fix some incorrect format strings
  soc: hisilicon: kunpeng_hccs: Add failure log for no _CRS method
  soc: hisilicon: kunpeng_hccs: remove an unused blank line
  soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and
    interrupt ack

 drivers/soc/hisilicon/kunpeng_hccs.c | 153 +++++++++++++++++++++------
 drivers/soc/hisilicon/kunpeng_hccs.h |  15 +++
 2 files changed, 135 insertions(+), 33 deletions(-)

-- 
2.33.0

[PATCH v2 1/4] soc: hisilicon: kunpeng_hccs: Fix some incorrect format strings

[Huisong Li]

Fix some incorrect format strings.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index e31791659560..dad6235dbf1a 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -155,7 +155,7 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 		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-%d.\n",
+			dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n",
 				hdev->chan_id);
 			rc = -ENOMEM;
 			goto err_mbx_channel_free;
@@ -1097,7 +1097,7 @@ static int hccs_create_hccs_dir(struct hccs_dev *hdev,
 	int ret;
 
 	ret = kobject_init_and_add(&port->kobj, &hccs_port_type,
-				   &die->kobj, "hccs%d", port->port_id);
+				   &die->kobj, "hccs%u", port->port_id);
 	if (ret) {
 		kobject_put(&port->kobj);
 		return ret;
@@ -1115,7 +1115,7 @@ static int hccs_create_die_dir(struct hccs_dev *hdev,
 	u16 i;
 
 	ret = kobject_init_and_add(&die->kobj, &hccs_die_type,
-				   &chip->kobj, "die%d", die->die_id);
+				   &chip->kobj, "die%u", die->die_id);
 	if (ret) {
 		kobject_put(&die->kobj);
 		return ret;
@@ -1125,7 +1125,7 @@ static int hccs_create_die_dir(struct hccs_dev *hdev,
 		port = &die->ports[i];
 		ret = hccs_create_hccs_dir(hdev, die, port);
 		if (ret) {
-			dev_err(hdev->dev, "create hccs%d dir failed.\n",
+			dev_err(hdev->dev, "create hccs%u dir failed.\n",
 				port->port_id);
 			goto err;
 		}
@@ -1147,7 +1147,7 @@ static int hccs_create_chip_dir(struct hccs_dev *hdev,
 	u16 id;
 
 	ret = kobject_init_and_add(&chip->kobj, &hccs_chip_type,
-				   &hdev->dev->kobj, "chip%d", chip->chip_id);
+				   &hdev->dev->kobj, "chip%u", chip->chip_id);
 	if (ret) {
 		kobject_put(&chip->kobj);
 		return ret;
@@ -1178,7 +1178,7 @@ static int hccs_create_topo_dirs(struct hccs_dev *hdev)
 		chip = &hdev->chips[id];
 		ret = hccs_create_chip_dir(hdev, chip);
 		if (ret) {
-			dev_err(hdev->dev, "init chip%d dir failed!\n", id);
+			dev_err(hdev->dev, "init chip%u dir failed!\n", id);
 			goto err;
 		}
 	}
-- 
2.33.0

[PATCH v2 2/4] soc: hisilicon: kunpeng_hccs: Add failure log for no _CRS method

[Huisong Li]

Driver gets the PCC channel id by using the PCC GAS in _CRS.
But, currently, if the firmware has no _CRS method on platform, there
is not any failure log. So this patch adds the log for this.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index dad6235dbf1a..fd3ca0eb8175 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -85,8 +85,10 @@ static int hccs_get_pcc_chan_id(struct hccs_dev *hdev)
 	struct hccs_register_ctx ctx = {0};
 	acpi_status status;
 
-	if (!acpi_has_method(handle, METHOD_NAME__CRS))
+	if (!acpi_has_method(handle, METHOD_NAME__CRS)) {
+		dev_err(hdev->dev, "No _CRS method.\n");
 		return -ENODEV;
+	}
 
 	ctx.dev = hdev->dev;
 	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
-- 
2.33.0

[PATCH v2 3/4] soc: hisilicon: kunpeng_hccs: remove an unused blank line

[Huisong Li]

Remove an unused blank line.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index fd3ca0eb8175..15125f1e0f3e 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -529,7 +529,6 @@ static int hccs_get_all_port_info_on_die(struct hccs_dev *hdev,
 
 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;
-- 
2.33.0

[PATCH v2 4/4] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[Huisong Li]

Support the platform with PCC type3 and interrupt ack. And a version
specific structure is introduced to handle the difference between the
device in the code.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 136 ++++++++++++++++++++++-----
 drivers/soc/hisilicon/kunpeng_hccs.h |  15 +++
 2 files changed, 126 insertions(+), 25 deletions(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index 15125f1e0f3e..d2302ff8c0e9 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -110,6 +110,14 @@ static void hccs_chan_tx_done(struct mbox_client *cl, void *msg, int ret)
 			 *(u8 *)msg, ret);
 }
 
+static void hccs_pcc_rx_callback(struct mbox_client *cl, void *mssg)
+{
+	struct hccs_mbox_client_info *cl_info =
+			container_of(cl, struct hccs_mbox_client_info, client);
+
+	complete(&cl_info->done);
+}
+
 static void hccs_unregister_pcc_channel(struct hccs_dev *hdev)
 {
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
@@ -131,6 +139,9 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 	cl->tx_block = false;
 	cl->knows_txdone = true;
 	cl->tx_done = hccs_chan_tx_done;
+	cl->rx_callback = hdev->verspec_data->rx_callback;
+	init_completion(&cl_info->done);
+
 	pcc_chan = pcc_mbox_request_channel(cl, hdev->chan_id);
 	if (IS_ERR(pcc_chan)) {
 		dev_err(dev, "PPC channel request failed.\n");
@@ -147,10 +158,16 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 	 */
 	cl_info->deadline_us =
 			HCCS_PCC_CMD_WAIT_RETRIES_NUM * pcc_chan->latency;
-	if (cl_info->mbox_chan->mbox->txdone_irq) {
+	if (!hdev->verspec_data->has_txdone_irq &&
+	    cl_info->mbox_chan->mbox->txdone_irq) {
 		dev_err(dev, "PCC IRQ in PCCT is enabled.\n");
 		rc = -EINVAL;
 		goto err_mbx_channel_free;
+	} else if (hdev->verspec_data->has_txdone_irq &&
+		   !cl_info->mbox_chan->mbox->txdone_irq) {
+		dev_err(dev, "PCC IRQ in PCCT isn't supported.\n");
+		rc = -EINVAL;
+		goto err_mbx_channel_free;
 	}
 
 	if (pcc_chan->shmem_base_addr) {
@@ -172,7 +189,7 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 	return rc;
 }
 
-static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
+static inline int hccs_wait_cmd_complete_by_poll(struct hccs_dev *hdev)
 {
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
 	struct acpi_pcct_shared_memory __iomem *comm_base =
@@ -194,30 +211,75 @@ static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
 	return ret;
 }
 
+static inline int hccs_wait_cmd_complete_by_irq(struct hccs_dev *hdev)
+{
+	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
+	int ret = 0;
+
+	if (!wait_for_completion_timeout(&cl_info->done,
+			usecs_to_jiffies(cl_info->deadline_us))) {
+		dev_err(hdev->dev, "PCC command executed timeout!\n");
+		ret = -ETIMEDOUT;
+	}
+
+	return ret;
+}
+
+static inline void hccs_fill_pcc_shared_mem_region(struct hccs_dev *hdev,
+						   u8 cmd,
+						   struct hccs_desc *desc,
+						   void __iomem *comm_space,
+						   u16 space_size)
+{
+	struct acpi_pcct_shared_memory tmp = {
+		.signature = PCC_SIGNATURE | hdev->chan_id,
+		.command = cmd,
+		.status = 0,
+	};
+
+	memcpy_toio(hdev->cl_info.pcc_comm_addr, (void *)&tmp,
+		    sizeof(struct acpi_pcct_shared_memory));
+
+	/* Copy the message to the PCC comm space */
+	memcpy_toio(comm_space, (void *)desc, space_size);
+}
+
+static inline void hccs_fill_ext_pcc_shared_mem_region(struct hccs_dev *hdev,
+						       u8 cmd,
+						       struct hccs_desc *desc,
+						       void __iomem *comm_space,
+						       u16 space_size)
+{
+	struct acpi_pcct_ext_pcc_shared_memory tmp = {
+		.signature = PCC_SIGNATURE | hdev->chan_id,
+		.flags = PCC_CMD_COMPLETION_NOTIFY,
+		.length = HCCS_PCC_SHARE_MEM_BYTES,
+		.command = cmd,
+	};
+
+	memcpy_toio(hdev->cl_info.pcc_comm_addr, (void *)&tmp,
+		    sizeof(struct acpi_pcct_ext_pcc_shared_memory));
+
+	/* Copy the message to the PCC comm space */
+	memcpy_toio(comm_space, (void *)desc, space_size);
+}
+
 static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 			     struct hccs_desc *desc)
 {
+	const struct hccs_verspecific_data *verspec_data = hdev->verspec_data;
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
-	void __iomem *comm_space = cl_info->pcc_comm_addr +
-					sizeof(struct acpi_pcct_shared_memory);
 	struct hccs_fw_inner_head *fw_inner_head;
-	struct acpi_pcct_shared_memory tmp = {0};
-	u16 comm_space_size;
+	void __iomem *comm_space;
+	u16 space_size;
 	int ret;
 
-	/* Write signature for this subspace */
-	tmp.signature = PCC_SIGNATURE | hdev->chan_id;
-	/* Write to the shared command region */
-	tmp.command = cmd;
-	/* Clear cmd complete bit */
-	tmp.status = 0;
-	memcpy_toio(cl_info->pcc_comm_addr, (void *)&tmp,
-			sizeof(struct acpi_pcct_shared_memory));
-
-	/* Copy the message to the PCC comm space */
-	comm_space_size = HCCS_PCC_SHARE_MEM_BYTES -
-				sizeof(struct acpi_pcct_shared_memory);
-	memcpy_toio(comm_space, (void *)desc, comm_space_size);
+	comm_space = cl_info->pcc_comm_addr + verspec_data->shared_mem_size;
+	space_size = HCCS_PCC_SHARE_MEM_BYTES - verspec_data->shared_mem_size;
+	verspec_data->fill_pcc_shared_mem(hdev, cmd, desc,
+					  comm_space, space_size);
+	if (verspec_data->has_txdone_irq)
+		reinit_completion(&cl_info->done);
 
 	/* Ring doorbell */
 	ret = mbox_send_message(cl_info->mbox_chan, &cmd);
@@ -227,13 +289,12 @@ static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 		goto end;
 	}
 
-	/* Wait for completion */
-	ret = hccs_check_chan_cmd_complete(hdev);
+	ret = verspec_data->wait_cmd_complete(hdev);
 	if (ret)
 		goto end;
 
 	/* Copy response data */
-	memcpy_fromio((void *)desc, comm_space, comm_space_size);
+	memcpy_fromio((void *)desc, comm_space, space_size);
 	fw_inner_head = &desc->rsp.fw_inner_head;
 	if (fw_inner_head->retStatus) {
 		dev_err(hdev->dev, "Execute PCC command failed, error code = %u.\n",
@@ -242,7 +303,10 @@ static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 	}
 
 end:
-	mbox_client_txdone(cl_info->mbox_chan, ret);
+	if (verspec_data->has_txdone_irq)
+		mbox_chan_txdone(cl_info->mbox_chan, ret);
+	else
+		mbox_client_txdone(cl_info->mbox_chan, ret);
 	return ret;
 }
 
@@ -1213,6 +1277,11 @@ static int hccs_probe(struct platform_device *pdev)
 	hdev->dev = &pdev->dev;
 	platform_set_drvdata(pdev, hdev);
 
+	/*
+	 * Here would never be failure as the driver and device has been matched.
+	 */
+	hdev->verspec_data = acpi_device_get_match_data(hdev->dev);
+
 	mutex_init(&hdev->lock);
 	rc = hccs_get_pcc_chan_id(hdev);
 	if (rc)
@@ -1249,9 +1318,26 @@ static void hccs_remove(struct platform_device *pdev)
 	hccs_unregister_pcc_channel(hdev);
 }
 
+static const struct hccs_verspecific_data hisi04b1_verspec_data = {
+	.rx_callback = NULL,
+	.wait_cmd_complete = hccs_wait_cmd_complete_by_poll,
+	.fill_pcc_shared_mem = hccs_fill_pcc_shared_mem_region,
+	.shared_mem_size = sizeof(struct acpi_pcct_shared_memory),
+	.has_txdone_irq = false,
+};
+
+static const struct hccs_verspecific_data hisi04b2_verspec_data = {
+	.rx_callback = hccs_pcc_rx_callback,
+	.wait_cmd_complete = hccs_wait_cmd_complete_by_irq,
+	.fill_pcc_shared_mem = hccs_fill_ext_pcc_shared_mem_region,
+	.shared_mem_size = sizeof(struct acpi_pcct_ext_pcc_shared_memory),
+	.has_txdone_irq = true,
+};
+
 static const struct acpi_device_id hccs_acpi_match[] = {
-	{ "HISI04B1"},
-	{ ""},
+	{ "HISI04B1", (unsigned long)&hisi04b1_verspec_data},
+	{ "HISI04B2", (unsigned long)&hisi04b2_verspec_data},
+	{ }
 };
 MODULE_DEVICE_TABLE(acpi, hccs_acpi_match);
 
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
index 6012d2776028..c3adbc01b471 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.h
+++ b/drivers/soc/hisilicon/kunpeng_hccs.h
@@ -51,11 +51,26 @@ struct hccs_mbox_client_info {
 	struct pcc_mbox_chan *pcc_chan;
 	u64 deadline_us;
 	void __iomem *pcc_comm_addr;
+	struct completion done;
+};
+
+struct hccs_desc;
+
+struct hccs_verspecific_data {
+	void (*rx_callback)(struct mbox_client *cl, void *mssg);
+	int (*wait_cmd_complete)(struct hccs_dev *hdev);
+	void (*fill_pcc_shared_mem)(struct hccs_dev *hdev,
+				    u8 cmd, struct hccs_desc *desc,
+				    void __iomem *comm_space,
+				    u16 space_size);
+	u16 shared_mem_size;
+	bool has_txdone_irq;
 };
 
 struct hccs_dev {
 	struct device *dev;
 	struct acpi_device *acpi_dev;
+	const struct hccs_verspecific_data *verspec_data;
 	u64 caps;
 	u8 chip_num;
 	struct hccs_chip_info *chips;
-- 
2.33.0

Re: [PATCH v2 3/4] soc: hisilicon: kunpeng_hccs: remove an unused blank line

[Jonathan Cameron]

On Thu, 30 Nov 2023 21:45:49 +0800
Huisong Li <lihuisong@huawei.com> wrote:

> Remove an unused blank line.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
FWIW
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/soc/hisilicon/kunpeng_hccs.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
> index fd3ca0eb8175..15125f1e0f3e 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
> @@ -529,7 +529,6 @@ static int hccs_get_all_port_info_on_die(struct hccs_dev *hdev,
>  
>  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;

Re: [PATCH v2 4/4] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[Jonathan Cameron]

On Thu, 30 Nov 2023 21:45:50 +0800
Huisong Li <lihuisong@huawei.com> wrote:

> Support the platform with PCC type3 and interrupt ack. And a version
> specific structure is introduced to handle the difference between the
> device in the code.
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>

Hi.

A few trivial things inline but now looks good to me!

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/soc/hisilicon/kunpeng_hccs.c | 136 ++++++++++++++++++++++-----
>  drivers/soc/hisilicon/kunpeng_hccs.h |  15 +++
>  2 files changed, 126 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
> index 15125f1e0f3e..d2302ff8c0e9 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c

...

>  
> -static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
> +static inline int hccs_wait_cmd_complete_by_poll(struct hccs_dev *hdev)

Why inline?  Do we have numbers to support this hint to the compiler being
useful?

>  {
>  	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
>  	struct acpi_pcct_shared_memory __iomem *comm_base =
> @@ -194,30 +211,75 @@ static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
>  	return ret;
>  }
>  
> +static inline int hccs_wait_cmd_complete_by_irq(struct hccs_dev *hdev)
> +{
> +	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
> +	int ret = 0;

Drop ret...

> +
> +	if (!wait_for_completion_timeout(&cl_info->done,
> +			usecs_to_jiffies(cl_info->deadline_us))) {
> +		dev_err(hdev->dev, "PCC command executed timeout!\n");
> +		ret = -ETIMEDOUT;
		return -TIMEDOUT;
...
> +	}
> +
> +	return ret;
return 0;

> +}

> +static const struct hccs_verspecific_data hisi04b1_verspec_data = {
> +	.rx_callback = NULL,

It's harmless but no need to set callback to NULL. Maybe it acts as documentation?
It's common practice to just let C spec guarantees initialize any not implemented callbacks
to 0 without them needing to be done explicitly.

> +	.wait_cmd_complete = hccs_wait_cmd_complete_by_poll,
> +	.fill_pcc_shared_mem = hccs_fill_pcc_shared_mem_region,
> +	.shared_mem_size = sizeof(struct acpi_pcct_shared_memory),
> +	.has_txdone_irq = false,
> +};
> +
> +static const struct hccs_verspecific_data hisi04b2_verspec_data = {
> +	.rx_callback = hccs_pcc_rx_callback,
> +	.wait_cmd_complete = hccs_wait_cmd_complete_by_irq,
> +	.fill_pcc_shared_mem = hccs_fill_ext_pcc_shared_mem_region,
> +	.shared_mem_size = sizeof(struct acpi_pcct_ext_pcc_shared_memory),
> +	.has_txdone_irq = true,
> +};

Re: [PATCH v2 4/4] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[lihuisong (C)]

在 2023/11/30 22:49, Jonathan Cameron 写道:
> On Thu, 30 Nov 2023 21:45:50 +0800
> Huisong Li <lihuisong@huawei.com> wrote:
>
>> Support the platform with PCC type3 and interrupt ack. And a version
>> specific structure is introduced to handle the difference between the
>> device in the code.
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Hi.
>
> A few trivial things inline but now looks good to me!
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
>> ---
>>   drivers/soc/hisilicon/kunpeng_hccs.c | 136 ++++++++++++++++++++++-----
>>   drivers/soc/hisilicon/kunpeng_hccs.h |  15 +++
>>   2 files changed, 126 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
>> index 15125f1e0f3e..d2302ff8c0e9 100644
>> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
>> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
> ...
>
>>   
>> -static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
>> +static inline int hccs_wait_cmd_complete_by_poll(struct hccs_dev *hdev)
> Why inline?  Do we have numbers to support this hint to the compiler being
> useful?
No testing for this, but here might not be really useful.
So will remove this inline.
>>   {
>>   	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
>>   	struct acpi_pcct_shared_memory __iomem *comm_base =
>> @@ -194,30 +211,75 @@ static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
>>   	return ret;
>>   }
>>   
>> +static inline int hccs_wait_cmd_complete_by_irq(struct hccs_dev *hdev)
>> +{
>> +	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
>> +	int ret = 0;
> Drop ret...
Ack
>
>> +
>> +	if (!wait_for_completion_timeout(&cl_info->done,
>> +			usecs_to_jiffies(cl_info->deadline_us))) {
>> +		dev_err(hdev->dev, "PCC command executed timeout!\n");
>> +		ret = -ETIMEDOUT;
> 		return -TIMEDOUT;
> ...
>> +	}
>> +
>> +	return ret;
> return 0;
Ack
>> +}
>> +static const struct hccs_verspecific_data hisi04b1_verspec_data = {
>> +	.rx_callback = NULL,
> It's harmless but no need to set callback to NULL. Maybe it acts as documentation?
Just to explicitly assign value and show the difference between the devices.
> It's common practice to just let C spec guarantees initialize any not implemented callbacks
> to 0 without them needing to be done explicitly.
Correct, but It's harmless.
>
>> +	.wait_cmd_complete = hccs_wait_cmd_complete_by_poll,
>> +	.fill_pcc_shared_mem = hccs_fill_pcc_shared_mem_region,
>> +	.shared_mem_size = sizeof(struct acpi_pcct_shared_memory),
>> +	.has_txdone_irq = false,
>> +};
>> +
>> +static const struct hccs_verspecific_data hisi04b2_verspec_data = {
>> +	.rx_callback = hccs_pcc_rx_callback,
>> +	.wait_cmd_complete = hccs_wait_cmd_complete_by_irq,
>> +	.fill_pcc_shared_mem = hccs_fill_ext_pcc_shared_mem_region,
>> +	.shared_mem_size = sizeof(struct acpi_pcct_ext_pcc_shared_memory),
>> +	.has_txdone_irq = true,
>> +};
> .

[PATCH v3 0/5] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[Huisong Li]

The main purpose of this series is to support the platform with PCC type3
and interrupt ack. At the same time, this series also fix some clean codes
and modify the incorrect email domain name.

---
 v3:
  - remove inline tag of two wait_cmd_complete function.
  - delete variable 'ret' in hccs_wait_cmd_complete_by_irq()
  - add a new patch which fix incorrect email domain name in document.

 v2:
  - using a version specific structure to replace device version according
    to Jonathan's advice.
  - add a new patch that remove an unused blank line.

Huisong Li (5):
  soc: hisilicon: kunpeng_hccs: Fix some incorrect format strings
  soc: hisilicon: kunpeng_hccs: Add failure log for no _CRS method
  soc: hisilicon: kunpeng_hccs: Remove an unused blank line
  doc: kunpeng_hccs: Fix incorrect email domain name
  soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and
    interrupt ack

 .../sysfs-devices-platform-kunpeng_hccs       |   6 +-
 drivers/soc/hisilicon/kunpeng_hccs.c          | 152 ++++++++++++++----
 drivers/soc/hisilicon/kunpeng_hccs.h          |  15 ++
 3 files changed, 137 insertions(+), 36 deletions(-)

-- 
2.33.0

[PATCH v3 1/5] soc: hisilicon: kunpeng_hccs: Fix some incorrect format strings

[Huisong Li]

Fix some incorrect format strings.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index e31791659560..dad6235dbf1a 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -155,7 +155,7 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 		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-%d.\n",
+			dev_err(dev, "Failed to ioremap PCC communication region for channel-%u.\n",
 				hdev->chan_id);
 			rc = -ENOMEM;
 			goto err_mbx_channel_free;
@@ -1097,7 +1097,7 @@ static int hccs_create_hccs_dir(struct hccs_dev *hdev,
 	int ret;
 
 	ret = kobject_init_and_add(&port->kobj, &hccs_port_type,
-				   &die->kobj, "hccs%d", port->port_id);
+				   &die->kobj, "hccs%u", port->port_id);
 	if (ret) {
 		kobject_put(&port->kobj);
 		return ret;
@@ -1115,7 +1115,7 @@ static int hccs_create_die_dir(struct hccs_dev *hdev,
 	u16 i;
 
 	ret = kobject_init_and_add(&die->kobj, &hccs_die_type,
-				   &chip->kobj, "die%d", die->die_id);
+				   &chip->kobj, "die%u", die->die_id);
 	if (ret) {
 		kobject_put(&die->kobj);
 		return ret;
@@ -1125,7 +1125,7 @@ static int hccs_create_die_dir(struct hccs_dev *hdev,
 		port = &die->ports[i];
 		ret = hccs_create_hccs_dir(hdev, die, port);
 		if (ret) {
-			dev_err(hdev->dev, "create hccs%d dir failed.\n",
+			dev_err(hdev->dev, "create hccs%u dir failed.\n",
 				port->port_id);
 			goto err;
 		}
@@ -1147,7 +1147,7 @@ static int hccs_create_chip_dir(struct hccs_dev *hdev,
 	u16 id;
 
 	ret = kobject_init_and_add(&chip->kobj, &hccs_chip_type,
-				   &hdev->dev->kobj, "chip%d", chip->chip_id);
+				   &hdev->dev->kobj, "chip%u", chip->chip_id);
 	if (ret) {
 		kobject_put(&chip->kobj);
 		return ret;
@@ -1178,7 +1178,7 @@ static int hccs_create_topo_dirs(struct hccs_dev *hdev)
 		chip = &hdev->chips[id];
 		ret = hccs_create_chip_dir(hdev, chip);
 		if (ret) {
-			dev_err(hdev->dev, "init chip%d dir failed!\n", id);
+			dev_err(hdev->dev, "init chip%u dir failed!\n", id);
 			goto err;
 		}
 	}
-- 
2.33.0

[PATCH v3 2/5] soc: hisilicon: kunpeng_hccs: Add failure log for no _CRS method

[Huisong Li]

Driver gets the PCC channel id by using the PCC GAS in _CRS.
But, currently, if the firmware has no _CRS method on platform, there
is not any failure log. So this patch adds the log for this.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index dad6235dbf1a..fd3ca0eb8175 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -85,8 +85,10 @@ static int hccs_get_pcc_chan_id(struct hccs_dev *hdev)
 	struct hccs_register_ctx ctx = {0};
 	acpi_status status;
 
-	if (!acpi_has_method(handle, METHOD_NAME__CRS))
+	if (!acpi_has_method(handle, METHOD_NAME__CRS)) {
+		dev_err(hdev->dev, "No _CRS method.\n");
 		return -ENODEV;
+	}
 
 	ctx.dev = hdev->dev;
 	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
-- 
2.33.0

[PATCH v3 3/5] soc: hisilicon: kunpeng_hccs: Remove an unused blank line

[Huisong Li]

Remove an unused blank line.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index fd3ca0eb8175..15125f1e0f3e 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -529,7 +529,6 @@ static int hccs_get_all_port_info_on_die(struct hccs_dev *hdev,
 
 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;
-- 
2.33.0

[PATCH v3 4/5] doc: kunpeng_hccs: Fix incorrect email domain name

[Huisong Li]

The email domain name in Contact is wrong. So this patch has to modify it.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 .../ABI/testing/sysfs-devices-platform-kunpeng_hccs         | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
index fdb4e36310fb..1666340820f7 100644
--- a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
+++ b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
@@ -3,7 +3,7 @@ What:		/sys/devices/platform/HISI04Bx:00/chipX/linked_full_lane
 What:		/sys/devices/platform/HISI04Bx:00/chipX/crc_err_cnt
 Date:		November 2023
 KernelVersion:	6.6
-Contact:	Huisong Li <lihuisong@huawei.org>
+Contact:	Huisong Li <lihuisong@huawei.com>
 Description:
 		The /sys/devices/platform/HISI04Bx:00/chipX/ directory
 		contains read-only attributes exposing some summarization
@@ -26,7 +26,7 @@ What:		/sys/devices/platform/HISI04Bx:00/chipX/dieY/linked_full_lane
 What:		/sys/devices/platform/HISI04Bx:00/chipX/dieY/crc_err_cnt
 Date:		November 2023
 KernelVersion:	6.6
-Contact:	Huisong Li <lihuisong@huawei.org>
+Contact:	Huisong Li <lihuisong@huawei.com>
 Description:
 		The /sys/devices/platform/HISI04Bx:00/chipX/dieY/ directory
 		contains read-only attributes exposing some summarization
@@ -54,7 +54,7 @@ What:		/sys/devices/platform/HISI04Bx:00/chipX/dieY/hccsN/lane_mask
 What:		/sys/devices/platform/HISI04Bx:00/chipX/dieY/hccsN/crc_err_cnt
 Date:		November 2023
 KernelVersion:	6.6
-Contact:	Huisong Li <lihuisong@huawei.org>
+Contact:	Huisong Li <lihuisong@huawei.com>
 Description:
 		The /sys/devices/platform/HISI04Bx/chipX/dieX/hccsN/ directory
 		contains read-only attributes exposing information about
-- 
2.33.0

[PATCH v3 5/5] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[Huisong Li]

Support the platform with PCC type3 and interrupt ack. And a version
specific structure is introduced to handle the difference between the
device in the code.

Signed-off-by: Huisong Li <lihuisong@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/soc/hisilicon/kunpeng_hccs.c | 135 ++++++++++++++++++++++-----
 drivers/soc/hisilicon/kunpeng_hccs.h |  15 +++
 2 files changed, 125 insertions(+), 25 deletions(-)

diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
index 15125f1e0f3e..9ff70b38e5e9 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.c
+++ b/drivers/soc/hisilicon/kunpeng_hccs.c
@@ -110,6 +110,14 @@ static void hccs_chan_tx_done(struct mbox_client *cl, void *msg, int ret)
 			 *(u8 *)msg, ret);
 }
 
+static void hccs_pcc_rx_callback(struct mbox_client *cl, void *mssg)
+{
+	struct hccs_mbox_client_info *cl_info =
+			container_of(cl, struct hccs_mbox_client_info, client);
+
+	complete(&cl_info->done);
+}
+
 static void hccs_unregister_pcc_channel(struct hccs_dev *hdev)
 {
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
@@ -131,6 +139,9 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 	cl->tx_block = false;
 	cl->knows_txdone = true;
 	cl->tx_done = hccs_chan_tx_done;
+	cl->rx_callback = hdev->verspec_data->rx_callback;
+	init_completion(&cl_info->done);
+
 	pcc_chan = pcc_mbox_request_channel(cl, hdev->chan_id);
 	if (IS_ERR(pcc_chan)) {
 		dev_err(dev, "PPC channel request failed.\n");
@@ -147,10 +158,16 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 	 */
 	cl_info->deadline_us =
 			HCCS_PCC_CMD_WAIT_RETRIES_NUM * pcc_chan->latency;
-	if (cl_info->mbox_chan->mbox->txdone_irq) {
+	if (!hdev->verspec_data->has_txdone_irq &&
+	    cl_info->mbox_chan->mbox->txdone_irq) {
 		dev_err(dev, "PCC IRQ in PCCT is enabled.\n");
 		rc = -EINVAL;
 		goto err_mbx_channel_free;
+	} else if (hdev->verspec_data->has_txdone_irq &&
+		   !cl_info->mbox_chan->mbox->txdone_irq) {
+		dev_err(dev, "PCC IRQ in PCCT isn't supported.\n");
+		rc = -EINVAL;
+		goto err_mbx_channel_free;
 	}
 
 	if (pcc_chan->shmem_base_addr) {
@@ -172,7 +189,7 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
 	return rc;
 }
 
-static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
+static int hccs_wait_cmd_complete_by_poll(struct hccs_dev *hdev)
 {
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
 	struct acpi_pcct_shared_memory __iomem *comm_base =
@@ -194,30 +211,74 @@ static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
 	return ret;
 }
 
+static int hccs_wait_cmd_complete_by_irq(struct hccs_dev *hdev)
+{
+	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
+
+	if (!wait_for_completion_timeout(&cl_info->done,
+			usecs_to_jiffies(cl_info->deadline_us))) {
+		dev_err(hdev->dev, "PCC command executed timeout!\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
+static inline void hccs_fill_pcc_shared_mem_region(struct hccs_dev *hdev,
+						   u8 cmd,
+						   struct hccs_desc *desc,
+						   void __iomem *comm_space,
+						   u16 space_size)
+{
+	struct acpi_pcct_shared_memory tmp = {
+		.signature = PCC_SIGNATURE | hdev->chan_id,
+		.command = cmd,
+		.status = 0,
+	};
+
+	memcpy_toio(hdev->cl_info.pcc_comm_addr, (void *)&tmp,
+		    sizeof(struct acpi_pcct_shared_memory));
+
+	/* Copy the message to the PCC comm space */
+	memcpy_toio(comm_space, (void *)desc, space_size);
+}
+
+static inline void hccs_fill_ext_pcc_shared_mem_region(struct hccs_dev *hdev,
+						       u8 cmd,
+						       struct hccs_desc *desc,
+						       void __iomem *comm_space,
+						       u16 space_size)
+{
+	struct acpi_pcct_ext_pcc_shared_memory tmp = {
+		.signature = PCC_SIGNATURE | hdev->chan_id,
+		.flags = PCC_CMD_COMPLETION_NOTIFY,
+		.length = HCCS_PCC_SHARE_MEM_BYTES,
+		.command = cmd,
+	};
+
+	memcpy_toio(hdev->cl_info.pcc_comm_addr, (void *)&tmp,
+		    sizeof(struct acpi_pcct_ext_pcc_shared_memory));
+
+	/* Copy the message to the PCC comm space */
+	memcpy_toio(comm_space, (void *)desc, space_size);
+}
+
 static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 			     struct hccs_desc *desc)
 {
+	const struct hccs_verspecific_data *verspec_data = hdev->verspec_data;
 	struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
-	void __iomem *comm_space = cl_info->pcc_comm_addr +
-					sizeof(struct acpi_pcct_shared_memory);
 	struct hccs_fw_inner_head *fw_inner_head;
-	struct acpi_pcct_shared_memory tmp = {0};
-	u16 comm_space_size;
+	void __iomem *comm_space;
+	u16 space_size;
 	int ret;
 
-	/* Write signature for this subspace */
-	tmp.signature = PCC_SIGNATURE | hdev->chan_id;
-	/* Write to the shared command region */
-	tmp.command = cmd;
-	/* Clear cmd complete bit */
-	tmp.status = 0;
-	memcpy_toio(cl_info->pcc_comm_addr, (void *)&tmp,
-			sizeof(struct acpi_pcct_shared_memory));
-
-	/* Copy the message to the PCC comm space */
-	comm_space_size = HCCS_PCC_SHARE_MEM_BYTES -
-				sizeof(struct acpi_pcct_shared_memory);
-	memcpy_toio(comm_space, (void *)desc, comm_space_size);
+	comm_space = cl_info->pcc_comm_addr + verspec_data->shared_mem_size;
+	space_size = HCCS_PCC_SHARE_MEM_BYTES - verspec_data->shared_mem_size;
+	verspec_data->fill_pcc_shared_mem(hdev, cmd, desc,
+					  comm_space, space_size);
+	if (verspec_data->has_txdone_irq)
+		reinit_completion(&cl_info->done);
 
 	/* Ring doorbell */
 	ret = mbox_send_message(cl_info->mbox_chan, &cmd);
@@ -227,13 +288,12 @@ static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 		goto end;
 	}
 
-	/* Wait for completion */
-	ret = hccs_check_chan_cmd_complete(hdev);
+	ret = verspec_data->wait_cmd_complete(hdev);
 	if (ret)
 		goto end;
 
 	/* Copy response data */
-	memcpy_fromio((void *)desc, comm_space, comm_space_size);
+	memcpy_fromio((void *)desc, comm_space, space_size);
 	fw_inner_head = &desc->rsp.fw_inner_head;
 	if (fw_inner_head->retStatus) {
 		dev_err(hdev->dev, "Execute PCC command failed, error code = %u.\n",
@@ -242,7 +302,10 @@ static int hccs_pcc_cmd_send(struct hccs_dev *hdev, u8 cmd,
 	}
 
 end:
-	mbox_client_txdone(cl_info->mbox_chan, ret);
+	if (verspec_data->has_txdone_irq)
+		mbox_chan_txdone(cl_info->mbox_chan, ret);
+	else
+		mbox_client_txdone(cl_info->mbox_chan, ret);
 	return ret;
 }
 
@@ -1213,6 +1276,11 @@ static int hccs_probe(struct platform_device *pdev)
 	hdev->dev = &pdev->dev;
 	platform_set_drvdata(pdev, hdev);
 
+	/*
+	 * Here would never be failure as the driver and device has been matched.
+	 */
+	hdev->verspec_data = acpi_device_get_match_data(hdev->dev);
+
 	mutex_init(&hdev->lock);
 	rc = hccs_get_pcc_chan_id(hdev);
 	if (rc)
@@ -1249,9 +1317,26 @@ static void hccs_remove(struct platform_device *pdev)
 	hccs_unregister_pcc_channel(hdev);
 }
 
+static const struct hccs_verspecific_data hisi04b1_verspec_data = {
+	.rx_callback = NULL,
+	.wait_cmd_complete = hccs_wait_cmd_complete_by_poll,
+	.fill_pcc_shared_mem = hccs_fill_pcc_shared_mem_region,
+	.shared_mem_size = sizeof(struct acpi_pcct_shared_memory),
+	.has_txdone_irq = false,
+};
+
+static const struct hccs_verspecific_data hisi04b2_verspec_data = {
+	.rx_callback = hccs_pcc_rx_callback,
+	.wait_cmd_complete = hccs_wait_cmd_complete_by_irq,
+	.fill_pcc_shared_mem = hccs_fill_ext_pcc_shared_mem_region,
+	.shared_mem_size = sizeof(struct acpi_pcct_ext_pcc_shared_memory),
+	.has_txdone_irq = true,
+};
+
 static const struct acpi_device_id hccs_acpi_match[] = {
-	{ "HISI04B1"},
-	{ ""},
+	{ "HISI04B1", (unsigned long)&hisi04b1_verspec_data},
+	{ "HISI04B2", (unsigned long)&hisi04b2_verspec_data},
+	{ }
 };
 MODULE_DEVICE_TABLE(acpi, hccs_acpi_match);
 
diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
index 6012d2776028..c3adbc01b471 100644
--- a/drivers/soc/hisilicon/kunpeng_hccs.h
+++ b/drivers/soc/hisilicon/kunpeng_hccs.h
@@ -51,11 +51,26 @@ struct hccs_mbox_client_info {
 	struct pcc_mbox_chan *pcc_chan;
 	u64 deadline_us;
 	void __iomem *pcc_comm_addr;
+	struct completion done;
+};
+
+struct hccs_desc;
+
+struct hccs_verspecific_data {
+	void (*rx_callback)(struct mbox_client *cl, void *mssg);
+	int (*wait_cmd_complete)(struct hccs_dev *hdev);
+	void (*fill_pcc_shared_mem)(struct hccs_dev *hdev,
+				    u8 cmd, struct hccs_desc *desc,
+				    void __iomem *comm_space,
+				    u16 space_size);
+	u16 shared_mem_size;
+	bool has_txdone_irq;
 };
 
 struct hccs_dev {
 	struct device *dev;
 	struct acpi_device *acpi_dev;
+	const struct hccs_verspecific_data *verspec_data;
 	u64 caps;
 	u8 chip_num;
 	struct hccs_chip_info *chips;
-- 
2.33.0

Re: [PATCH v3 0/5] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack

[Wei Xu]

Hi Huisong,

On 2023/12/1 11:45, Huisong Li wrote:
> The main purpose of this series is to support the platform with PCC type3
> and interrupt ack. At the same time, this series also fix some clean codes
> and modify the incorrect email domain name.
> 
> ---
>  v3:
>   - remove inline tag of two wait_cmd_complete function.
>   - delete variable 'ret' in hccs_wait_cmd_complete_by_irq()
>   - add a new patch which fix incorrect email domain name in document.
> 
>  v2:
>   - using a version specific structure to replace device version according
>     to Jonathan's advice.
>   - add a new patch that remove an unused blank line.
> 
> Huisong Li (5):
>   soc: hisilicon: kunpeng_hccs: Fix some incorrect format strings
>   soc: hisilicon: kunpeng_hccs: Add failure log for no _CRS method
>   soc: hisilicon: kunpeng_hccs: Remove an unused blank line
>   doc: kunpeng_hccs: Fix incorrect email domain name
>   soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and
>     interrupt ack
> 
>  .../sysfs-devices-platform-kunpeng_hccs       |   6 +-
>  drivers/soc/hisilicon/kunpeng_hccs.c          | 152 ++++++++++++++----
>  drivers/soc/hisilicon/kunpeng_hccs.h          |  15 ++
>  3 files changed, 137 insertions(+), 36 deletions(-)
> 

Thanks!
Series applied to the HiSilicon driver tree.

Best Regards,
Wei