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

["lihuisong (C)" <lihuisong@huawei.com>]

在 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.
>
> .