Re: [PATCH v10 15/33] remoteproc: k3: Refactor rproc_reset() implementation into common driver

[Andrew Davis <afd@ti.com>]

On 4/22/25 12:53 AM, Beleswar Prasad Padhi wrote:
> Hi Andrew,
> 
> On 21/04/25 20:12, Andrew Davis wrote:
>> On 4/17/25 1:19 PM, Beleswar Padhi wrote:
>>> The rproc_reset() implementations in TI K3 DSP and M4 remoteproc drivers
>>> assert reset in the same way. Refactor the above function into the
>>> ti_k3_common.c driver as k3_rproc_reset() and use it throughout DSP and
>>> M4 drivers for resetting the remote processor.
>>>
>>> Signed-off-by: Beleswar Padhi <b-padhi@ti.com>
>>> ---
>>> v10: Changelog:
>>> 1. Split [v9 12/26] into [v10 14/33] and [v10 15/33] patches.
>>>
>>> Link to v9:
>>> https://lore.kernel.org/all/20250317120622.1746415-13-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 | 28 ++---------------------
>>>    drivers/remoteproc/ti_k3_m4_remoteproc.c  | 16 +++----------
>>>    4 files changed, 31 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/ti_k3_common.c b/drivers/remoteproc/ti_k3_common.c
>>> index aace308b49b0e..19bb6c337af77 100644
>>> --- a/drivers/remoteproc/ti_k3_common.c
>>> +++ b/drivers/remoteproc/ti_k3_common.c
>>> @@ -105,5 +105,30 @@ void k3_rproc_kick(struct rproc *rproc, int vqid)
>>>    }
>>>    EXPORT_SYMBOL_GPL(k3_rproc_kick);
>>>    +/* Put the remote processor into reset */
>>> +int k3_rproc_reset(struct k3_rproc *kproc)
>>> +{
>>> +    struct device *dev = kproc->dev;
>>> +    int ret;
>>> +
>>> +    if (kproc->data->uses_lreset) {
>>> +        ret = reset_control_assert(kproc->reset);
>>> +        if (ret)
>>> +            dev_err(dev, "local-reset assert failed (%pe)\n", ERR_PTR(ret));
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>>> +                            kproc->ti_sci_id);
>>> +    if (ret) {
>>> +        dev_err(dev, "module-reset assert failed (%pe)\n", ERR_PTR(ret));
>>> +        if (reset_control_deassert(kproc->reset))
>>> +            dev_warn(dev, "local-reset deassert back failed\n");
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(k3_rproc_reset);
>>> +
>>>    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 6ae7ac4ec5696..f3400fc774766 100644
>>> --- a/drivers/remoteproc/ti_k3_common.h
>>> +++ b/drivers/remoteproc/ti_k3_common.h
>>> @@ -90,4 +90,5 @@ struct k3_rproc {
>>>      void k3_rproc_mbox_callback(struct mbox_client *client, void *data);
>>>    void k3_rproc_kick(struct rproc *rproc, int vqid);
>>> +int k3_rproc_reset(struct k3_rproc *kproc);
>>>    #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 0a8c9e61393d2..f8a5282df5b71 100644
>>> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>>> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
>>> @@ -24,30 +24,6 @@
>>>      #define KEYSTONE_RPROC_LOCAL_ADDRESS_MASK    (SZ_16M - 1)
>>>    -/* Put the DSP processor into reset */
>>> -static int k3_dsp_rproc_reset(struct k3_rproc *kproc)
>>> -{
>>> -    struct device *dev = kproc->dev;
>>> -    int ret;
>>> -
>>> -    if (kproc->data->uses_lreset) {
>>> -        ret = reset_control_assert(kproc->reset);
>>> -        if (ret)
>>> -            dev_err(dev, "local-reset assert failed (%pe)\n", ERR_PTR(ret));
>>> -        return ret;
>>> -    }
>>> -
>>> -    ret = kproc->ti_sci->ops.dev_ops.put_device(kproc->ti_sci,
>>> -                            kproc->ti_sci_id);
>>> -    if (ret) {
>>> -        dev_err(dev, "module-reset assert failed (%pe)\n", ERR_PTR(ret));
>>> -        if (reset_control_deassert(kproc->reset))
>>> -            dev_warn(dev, "local-reset deassert back failed\n");
>>> -    }
>>> -
>>> -    return ret;
>>> -}
>>> -
>>>    /* Release the DSP processor from reset */
>>>    static int k3_dsp_rproc_release(struct k3_rproc *kproc)
>>>    {
>>> @@ -201,7 +177,7 @@ static int k3_dsp_rproc_stop(struct rproc *rproc)
>>>    {
>>>        struct k3_rproc *kproc = rproc->priv;
>>>    -    k3_dsp_rproc_reset(kproc);
>>> +    k3_rproc_reset(kproc);
>>>          return 0;
>>>    }
>>> @@ -565,7 +541,7 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev)
>>>                    return dev_err_probe(dev, ret, "failed to get reset status\n");
>>>                } else if (ret == 0) {
>>>                    dev_warn(dev, "local reset is deasserted for device\n");
>>> -                k3_dsp_rproc_reset(kproc);
>>> +                k3_rproc_reset(kproc);
>>>                }
>>>            }
>>>        }
>>> diff --git a/drivers/remoteproc/ti_k3_m4_remoteproc.c b/drivers/remoteproc/ti_k3_m4_remoteproc.c
>>> index 8a6917259ce60..7d5b75be2e4f8 100644
>>> --- a/drivers/remoteproc/ti_k3_m4_remoteproc.c
>>> +++ b/drivers/remoteproc/ti_k3_m4_remoteproc.c
>>> @@ -65,11 +65,9 @@ static int k3_m4_rproc_prepare(struct rproc *rproc)
>>>         * Ensure the local reset is asserted so the core doesn't
>>>         * execute bogus code when the module reset is released.
>>>         */
>>> -    ret = reset_control_assert(kproc->reset);
>>> -    if (ret) {
>>> -        dev_err(dev, "could not assert local reset\n");
>>> +    ret = k3_rproc_reset(kproc);
>>> +    if (ret)
>>>            return ret;
>>> -    }
>>>          ret = reset_control_status(kproc->reset);
>>>        if (ret <= 0) {
>>> @@ -374,16 +372,8 @@ static int k3_m4_rproc_start(struct rproc *rproc)
>>>    static int k3_m4_rproc_stop(struct rproc *rproc)
>>>    {
>>>        struct k3_rproc *kproc = rproc->priv;
>>> -    struct device *dev = kproc->dev;
>>> -    int ret;
>>>    -    ret = reset_control_assert(kproc->reset);
>>> -    if (ret) {
>>> -        dev_err(dev, "local-reset assert failed, ret = %d\n", ret);
>>> -        return ret;
>>> -    }
>>> -
>>> -    return 0;
>>> +    return k3_rproc_reset(kproc);
>>
>> This doesn't feel right. The new common k3_rproc_reset() function
>> matches what ti_k3_dsp_remoteproc.c did for reset, you made it that
>> way in the previous patch [14/33]. But it doesn't match what this
>> M4 version does (yes I know logically they are the same as `uses_lreset`
>> will be always true for M4). Maybe you want to do the same as you
>> did for DSP to the M4 driver first, before you make this change so
>> it is 100% clear the code is the same (and so bisect lands on the
>> right patch should someday this be an issue).
> 
> 
> Sure, I can make that change in next revision...
> 
>>
>> Also, the common k3_rproc_reset() calls put_device() unconditionally.
>> Something that wasn't done at all here in the M4 prepare() and stop()
>> functions.
> 
> 
> There is a 'return ret' in the 'if (kproc->data->uses_lreset)' condition flow in k3_rproc_reset().
> 
> put_device() should not be unconditional...
> 

Ah, I must have looked right past the return statement. So it is one or
the other, but not both? Might be good to put the put_device() side into
an `else` block.

Then it seems in the !uses_lreset path you still undo the reset_control_assert()
by calling reset_control_deassert() if put_device() fails. Think you messed
this one up in back in [14/33].

Andrew

>>
>> These two changes make this patch not strictly a pure "refactor"
>> patch, which IMHO should in no way change the calls being made nor
>> the logical flow, only the code structure.
> 
> 
> Got it. Will address in revision. Will wait for more reviews (if any) before re-spinning.
> 
> Thanks,
> Beleswar
> 
>>
>> Andrew
>>
>>>    }
>>>      /*