Re: [PATCH v1 3/3] soc: hisilicon: kunpeng_hccs: Support the platform with PCC type3 and interrupt ack
From: lihuisong (C)
Date: Wed Nov 29 2023 - 20:16:47 EST
在 2023/11/28 23:44, Jonathan Cameron 写道:
On Thu, 9 Nov 2023 13:45:26 +0800
Huisong Li <lihuisong@xxxxxxxxxx> 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@xxxxxxxxxx>
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;
.