On Thu, 9 Nov 2023 13:45:26 +0800
Huisong Li <[email protected]> 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 <[email protected]>
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;
在 2023/11/28 23:44, Jonathan Cameron 写道:
> On Thu, 9 Nov 2023 13:45:26 +0800
> Huisong Li <[email protected]> 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 <[email protected]>
> 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;
> .