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);
}
/*
--
2.34.1On 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).
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.
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.
Andrew
> }
>
> /*
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...
>
> 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
>
>> }
>> /*
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
>>
>>> }
>>> /*
On 22/04/25 19:51, Andrew Davis wrote:
> 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.
Sure I will do that in revision.
>
> 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].
My bad. Will address in revision.. Do you have any other review comments for the series before I re-spin?
Thanks,
Beleswar
>
> 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
>>>
>>>> }
>>>> /*
© 2016 - 2026 Red Hat, Inc.