Re: [PATCH v2 08/13] mailbox: pcc: Refactor and simplify check_and_ack()

lihuisong (C)
2025-03-11T12:19:36Z

在 2025/3/11 20:08, Sudeep Holla 写道:
> On Tue, Mar 11, 2025 at 07:47:39PM +0800, lihuisong (C) wrote:
>> 在 2025/3/6 0:38, Sudeep Holla 写道:
>>> The existing check_and_ack() function had unnecessary complexity. The
>>> logic could be streamlined to improve code readability and maintainability.
>>>
>>> The command update register needs to be updated in order to acknowledge
>>> the platform notification through type 4 channel. So it can be done
>>> unconditionally. Currently it is complicated just to make use of
>>> pcc_send_data() which also executes the same updation.
>>>
>>> In order to simplify, let us just ring the doorbell directly from
>>> check_and_ack() instead of calling into pcc_send_data(). While at it,
>>> rename it into pcc_chan_check_and_ack() to maintain consistency in the
>>> driver.
>> LGTM except for some trivial,
>> Acked-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>> ---
>>> drivers/mailbox/pcc.c | 37 +++++++++++++------------------------
>>> 1 file changed, 13 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
>>> index b3d133170aac7f8acfd1999564c69b7fe4f6d582..90d6f5e24df7e796f8c29705808eb6df2806c1f2 100644
>>> --- a/drivers/mailbox/pcc.c
>>> +++ b/drivers/mailbox/pcc.c
>>> @@ -117,8 +117,6 @@ struct pcc_chan_info {
>>> static struct pcc_chan_info *chan_info;
>>> static int pcc_chan_count;
>>> -static int pcc_send_data(struct mbox_chan *chan, void *data);
>>> -
>>> /*
>>> * PCC can be used with perf critical drivers such as CPPC
>>> * So it makes sense to locally cache the virtual address and
>>> @@ -288,33 +286,24 @@ static int pcc_mbox_error_check_and_clear(struct pcc_chan_info *pchan)
>>> return 0;
>>> }
>>> -static void check_and_ack(struct pcc_chan_info *pchan, struct mbox_chan *chan)
>>> +static void pcc_chan_check_and_ack(struct pcc_chan_info *pchan)
>> How about use pcc_chan_ack?
What do you think of this?
>>> {
>>> - struct acpi_pcct_ext_pcc_shared_memory pcc_hdr;
>>> + struct acpi_pcct_ext_pcc_shared_memory __iomem *pcc_hdr;
>>> if (pchan->type != ACPI_PCCT_TYPE_EXT_PCC_SLAVE_SUBSPACE)
>>> return;
>>> - /* If the memory region has not been mapped, we cannot
>>> - * determine if we need to send the message, but we still
>>> - * need to set the cmd_update flag before returning.
>>> - */
>>> - if (pchan->chan.shmem == NULL) {
>>> - pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>>> - return;
>>> - }
>>> - memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
>>> - sizeof(struct acpi_pcct_ext_pcc_shared_memory));
>>> +
>>> + pcc_chan_reg_read_modify_write(&pchan->cmd_update);
>>> +
>>> + pcc_hdr = pchan->chan.shmem;
>> Should use the original code?
>>
>> memcpy_fromio(&pcc_hdr, pchan->chan.shmem,
>> sizeof(struct acpi_pcct_ext_pcc_shared_memory));
>>
> ioread32(&pcc_hdr->flags) just reads 4 byte flag instead of copying entire
> header for no reason. It should be same as memcpy_fromio(.., .., 4)
>
get it.
>
> .