Re: [PATCH v10 13/33] remoteproc: k3: Refactor .kick rproc ops into common driver

[Beleswar Prasad Padhi <b-padhi@ti.com>]

On 21/04/25 20:28, Andrew Davis wrote:
> On 4/17/25 1:19 PM, Beleswar Padhi wrote:
>> The .kick rproc ops implementations in TI K3 R5, DSP and M4 remoteproc
>> drivers sends a mailbox message to the remote processor in the same
>> way. Refactor the implementations into a common function
>> 'k3_rproc_kick()' in the ti_k3_common.c driver.
>>
>> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
>> ---
>> v10: Changelog:
>> None
>>
>> Link to v9:
>> https://lore.kernel.org/all/20250317120622.1746415-12-b-padhi@ti.com/
>>
>>   drivers/remoteproc/ti_k3_common.c         | 25 ++++++++++++++++++++++
>>   drivers/remoteproc/ti_k3_common.h         |  1 +
>>   drivers/remoteproc/ti_k3_dsp_remoteproc.c | 22 +------------------
>>   drivers/remoteproc/ti_k3_m4_remoteproc.c  | 26 +----------------------
>>   drivers/remoteproc/ti_k3_r5_remoteproc.c  | 17 +--------------
>>   5 files changed, 29 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/remoteproc/ti_k3_common.c b/drivers/remoteproc/ti_k3_common.c
>> index 7b45e3b416186..aace308b49b0e 100644
>> --- a/drivers/remoteproc/ti_k3_common.c
>> +++ b/drivers/remoteproc/ti_k3_common.c
>> @@ -80,5 +80,30 @@ void k3_rproc_mbox_callback(struct mbox_client *client, void *data)
>>   }
>>   EXPORT_SYMBOL_GPL(k3_rproc_mbox_callback);
>>   +/*
>> + * Kick the remote processor to notify about pending unprocessed messages.
>> + * The vqid usage is not used and is inconsequential, as the kick is performed
>> + * through a simulated GPIO (a bit in an IPC interrupt-triggering register),
>> + * the remote processor is expected to process both its Tx and Rx virtqueues.
>> + */
>
> This comment is wrong. Looks like this is a copy paste error that ended up
> in every ti_k3_*_remoteproc.c driver. This whole "simulated GPIO" thing
> is only true for the K2 DSP (keystone_remoteproc.c), all the K3 devices
> have proper mailbox interrupts.
>
> Anyway, no need to remove it here, this patch is just a refactor and since
> it was already in every driver you are factoring this out from I'd suggest
> fixing it in a later patch.


Sure, I can put a patch later to fix this comment.

Thanks,
Beleswar

>
> For this patch,
>
> Acked-by: Andrew Davis <afd@ti.com>
>
>> +void k3_rproc_kick(struct rproc *rproc, int vqid)
>> +{
>> +    struct k3_rproc *kproc = rproc->priv;
>> +    struct device *dev = kproc->dev;
>> +    u32 msg = (u32)vqid;
>> +    int ret;
>> +
>> +    /*
>> +     * Send the index of the triggered virtqueue in the mailbox payload.
>> +     * NOTE: msg is cast to uintptr_t to prevent compiler warnings when
>> +     * void* is 64bit. It is safely cast back to u32 in the mailbox driver.
>> +     */
>> +    ret = mbox_send_message(kproc->mbox, (void *)(uintptr_t)msg);
>> +    if (ret < 0)
>> +        dev_err(dev, "failed to send mailbox message, status = %d\n",
>> +            ret);
>> +}
>> +EXPORT_SYMBOL_GPL(k3_rproc_kick);
>> +
>>   MODULE_LICENSE("GPL");
>>   MODULE_DESCRIPTION("TI K3 common Remoteproc code");
>> diff --git a/drivers/remoteproc/ti_k3_common.h b/drivers/remoteproc/ti_k3_common.h
>> index 785bb4b17d02f..6ae7ac4ec5696 100644
>> --- a/drivers/remoteproc/ti_k3_common.h
>> +++ b/drivers/remoteproc/ti_k3_common.h
>> @@ -89,4 +89,5 @@ struct k3_rproc {
>>   };
>>     void k3_rproc_mbox_callback(struct mbox_client *client, void *data);
>> +void k3_rproc_kick(struct rproc *rproc, int vqid);
>>   #endif /* REMOTEPROC_TI_K3_COMMON_H */
>> diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> index 7bd1d5a790cb2..476f4e69d2c11 100644
>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>> @@ -24,26 +24,6 @@
>>     #define KEYSTONE_RPROC_LOCAL_ADDRESS_MASK    (SZ_16M - 1)
>>   -/*
>> - * Kick the remote processor to notify about pending unprocessed messages.
>> - * The vqid usage is not used and is inconsequential, as the kick is performed
>> - * through a simulated GPIO (a bit in an IPC interrupt-triggering register),
>> - * the remote processor is expected to process both its Tx and Rx virtqueues.
>> - */
>> -static void k3_dsp_rproc_kick(struct rproc *rproc, int vqid)
>> -{
>> -    struct k3_rproc *kproc = rproc->priv;
>> -    struct device *dev = rproc->dev.parent;
>> -    mbox_msg_t msg = (mbox_msg_t)vqid;
>> -    int ret;
>> -
>> -    /* send the index of the triggered virtqueue in the mailbox payload */
>> -    ret = mbox_send_message(kproc->mbox, (void *)msg);
>> -    if (ret < 0)
>> -        dev_err(dev, "failed to send mailbox message (%pe)\n",
>> -            ERR_PTR(ret));
>> -}
>> -
>>   /* Put the DSP processor into reset */
>>   static int k3_dsp_rproc_reset(struct k3_rproc *kproc)
>>   {
>> @@ -342,7 +322,7 @@ static void *k3_dsp_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool
>>   static const struct rproc_ops k3_dsp_rproc_ops = {
>>       .start        = k3_dsp_rproc_start,
>>       .stop        = k3_dsp_rproc_stop,
>> -    .kick        = k3_dsp_rproc_kick,
>> +    .kick        = k3_rproc_kick,
>>       .da_to_va    = k3_dsp_rproc_da_to_va,
>>   };
>>   diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c b/drivers/remoteproc/ti_k3_m4_remoteproc.c
>> index a1bcc4b265dfe..8a6917259ce60 100644
>> --- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
>> @@ -21,30 +21,6 @@
>>   #include "ti_sci_proc.h"
>>   #include "ti_k3_common.h"
>>   -/*
>> - * Kick the remote processor to notify about pending unprocessed messages.
>> - * The vqid usage is not used and is inconsequential, as the kick is performed
>> - * through a simulated GPIO (a bit in an IPC interrupt-triggering register),
>> - * the remote processor is expected to process both its Tx and Rx virtqueues.
>> - */
>> -static void k3_m4_rproc_kick(struct rproc *rproc, int vqid)
>> -{
>> -    struct k3_rproc *kproc = rproc->priv;
>> -    struct device *dev = kproc->dev;
>> -    u32 msg = (u32)vqid;
>> -    int ret;
>> -
>> -    /*
>> -     * Send the index of the triggered virtqueue in the mailbox payload.
>> -     * NOTE: msg is cast to uintptr_t to prevent compiler warnings when
>> -     * void* is 64bit. It is safely cast back to u32 in the mailbox driver.
>> -     */
>> -    ret = mbox_send_message(kproc->mbox, (void *)(uintptr_t)msg);
>> -    if (ret < 0)
>> -        dev_err(dev, "failed to send mailbox message, status = %d\n",
>> -            ret);
>> -}
>> -
>>   static int k3_m4_rproc_ping_mbox(struct k3_rproc *kproc)
>>   {
>>       struct device *dev = kproc->dev;
>> @@ -448,7 +424,7 @@ static const struct rproc_ops k3_m4_rproc_ops = {
>>       .stop = k3_m4_rproc_stop,
>>       .attach = k3_m4_rproc_attach,
>>       .detach = k3_m4_rproc_detach,
>> -    .kick = k3_m4_rproc_kick,
>> +    .kick = k3_rproc_kick,
>>       .da_to_va = k3_m4_rproc_da_to_va,
>>       .get_loaded_rsc_table = k3_m4_get_loaded_rsc_table,
>>   };
>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> index a1dfbe383c13c..dedc9456983e0 100644
>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>> @@ -129,21 +129,6 @@ struct k3_r5_core {
>>       bool released_from_reset;
>>   };
>>   -/* kick a virtqueue */
>> -static void k3_r5_rproc_kick(struct rproc *rproc, int vqid)
>> -{
>> -    struct k3_rproc *kproc = rproc->priv;
>> -    struct device *dev = rproc->dev.parent;
>> -    mbox_msg_t msg = (mbox_msg_t)vqid;
>> -    int ret;
>> -
>> -    /* send the index of the triggered virtqueue in the mailbox payload */
>> -    ret = mbox_send_message(kproc->mbox, (void *)msg);
>> -    if (ret < 0)
>> -        dev_err(dev, "failed to send mailbox message, status = %d\n",
>> -            ret);
>> -}
>> -
>>   static int k3_r5_split_reset(struct k3_rproc *kproc)
>>   {
>>       int ret;
>> @@ -735,7 +720,7 @@ static const struct rproc_ops k3_r5_rproc_ops = {
>>       .unprepare    = k3_r5_rproc_unprepare,
>>       .start        = k3_r5_rproc_start,
>>       .stop        = k3_r5_rproc_stop,
>> -    .kick        = k3_r5_rproc_kick,
>> +    .kick        = k3_rproc_kick,
>>       .da_to_va    = k3_r5_rproc_da_to_va,
>>   };
>>