| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <00ba399d-7a26-4ced-8f77-334839bb54c6@ti.com>
Date: Mon, 21 Apr 2025 09:42:25 -0500
From: Andrew Davis <afd@...com>
To: Beleswar Padhi <b-padhi@...com>, <andersson@...nel.org>,
<mathieu.poirier@...aro.org>
CC: <hnagalla@...com>, <u-kumar1@...com>, <jm@...com>,
<jan.kiszka@...mens.com>, <christophe.jaillet@...adoo.fr>,
<jkangas@...hat.com>, <eballetbo@...hat.com>,
<linux-remoteproc@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v10 15/33] remoteproc: k3: Refactor rproc_reset()
implementation into common driver
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@...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
> }
>
> /*
Powered by blists - more mailing lists