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-pa...@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-pa...@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 > >> } >> /*
