[PATCH v1] slimbus: qcom-ngd-ctrl: Add stream disable support
Currently slimbus driver doesn't support stream disable callback, it only supports stream enable callback.
In slimbus usecase, client is switching to new frequency with same channel and calling enable stream callback for new frequency but DSP subsystem is crashing as we are switching to new frequency with same channel without disabling stream for older frequency.
Ideally, before switching to another frequency, client should call disable stream callback and then enable stream for newer frequency.
Hence add support to disable stream via qcom_slim_ngd_disable_stream().
Signed-off-by: Viken Dadhaniya quic_vdadhani@quicinc.com --- drivers/slimbus/qcom-ngd-ctrl.c | 70 +++++++++++++++++++++++++++++++++ drivers/slimbus/slimbus.h | 13 ++++++ 2 files changed, 83 insertions(+)
diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c index e0b21f0f79c1..d952827d2e12 100644 --- a/drivers/slimbus/qcom-ngd-ctrl.c +++ b/drivers/slimbus/qcom-ngd-ctrl.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 // Copyright (c) 2011-2017, The Linux Foundation. All rights reserved. // Copyright (c) 2018, Linaro Limited +// Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
#include <linux/irq.h> #include <linux/kernel.h> @@ -1084,6 +1085,74 @@ static int qcom_slim_ngd_enable_stream(struct slim_stream_runtime *rt) return ret; }
+static int qcom_slim_ngd_disable_stream(struct slim_stream_runtime *rt) +{ + struct slim_device *sdev = rt->dev; + struct slim_controller *ctrl = sdev->ctrl; + struct slim_val_inf msg = {0}; + u8 wbuf[SLIM_MSGQ_BUF_LEN]; + u8 rbuf[SLIM_MSGQ_BUF_LEN]; + struct slim_msg_txn txn = {0,}; + int i, ret; + + txn.mt = SLIM_MSG_MT_DEST_REFERRED_USER; + txn.dt = SLIM_MSG_DEST_LOGICALADDR; + txn.la = SLIM_LA_MGR; + txn.ec = 0; + txn.msg = &msg; + txn.msg->num_bytes = 0; + txn.msg->wbuf = wbuf; + txn.msg->rbuf = rbuf; + + for (i = 0; i < rt->num_ports; i++) { + struct slim_port *port = &rt->ports[i]; + + if (txn.msg->num_bytes == 0) { + wbuf[txn.msg->num_bytes++] = (u8)(SLIM_CH_REMOVE << 6) + | (sdev->laddr & 0x1f); + + ret = slim_alloc_txn_tid(ctrl, &txn); + if (ret) { + dev_err(&sdev->dev, "Fail to allocate TID\n"); + return -ENXIO; + } + wbuf[txn.msg->num_bytes++] = txn.tid; + } + wbuf[txn.msg->num_bytes++] = port->ch.id; + } + + txn.mc = SLIM_USR_MC_CHAN_CTRL; + txn.rl = txn.msg->num_bytes + 4; + ret = qcom_slim_ngd_xfer_msg_sync(ctrl, &txn); + if (ret) { + slim_free_txn_tid(ctrl, &txn); + dev_err(&sdev->dev, "TX timed out:MC:0x%x,mt:0x%x ret:%d\n", + txn.mc, txn.mt, ret); + return ret; + } + + txn.mc = SLIM_USR_MC_RECONFIG_NOW; + txn.msg->num_bytes = 2; + wbuf[1] = sdev->laddr; + txn.rl = txn.msg->num_bytes + 4; + + ret = slim_alloc_txn_tid(ctrl, &txn); + if (ret) { + dev_err(ctrl->dev, "Fail to allocate TID ret:%d\n", ret); + return ret; + } + + wbuf[0] = txn.tid; + ret = qcom_slim_ngd_xfer_msg_sync(ctrl, &txn); + if (ret) { + slim_free_txn_tid(ctrl, &txn); + dev_err(&sdev->dev, "TX timed out:MC:0x%x,mt:0x%x ret:%d\n", + txn.mc, txn.mt, ret); + } + + return ret; +} + static int qcom_slim_ngd_get_laddr(struct slim_controller *ctrl, struct slim_eaddr *ea, u8 *laddr) { @@ -1642,6 +1711,7 @@ static int qcom_slim_ngd_ctrl_probe(struct platform_device *pdev) ctrl->ctrl.clkgear = SLIM_MAX_CLK_GEAR; ctrl->ctrl.get_laddr = qcom_slim_ngd_get_laddr; ctrl->ctrl.enable_stream = qcom_slim_ngd_enable_stream; + ctrl->ctrl.disable_stream = qcom_slim_ngd_disable_stream; ctrl->ctrl.xfer_msg = qcom_slim_ngd_xfer_msg; ctrl->ctrl.wakeup = NULL; ctrl->state = QCOM_SLIM_NGD_CTRL_DOWN; diff --git a/drivers/slimbus/slimbus.h b/drivers/slimbus/slimbus.h index 00a7f112574b..21543d125614 100644 --- a/drivers/slimbus/slimbus.h +++ b/drivers/slimbus/slimbus.h @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0 */ /* * Copyright (c) 2011-2017, The Linux Foundation + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. */
#ifndef _DRIVERS_SLIMBUS_H @@ -316,6 +317,18 @@ enum slim_transport_protocol { SLIM_PROTO_EXT_HALF_DUP, };
+/* + * enum slim_ch_control: Channel control. + * Activate will schedule channel and/or group of channels in the TDM frame. + * Suspend will keep the schedule but data-transfer won't happen. + * Remove will remove the channel/group from the TDM frame. + */ +enum slim_ch_control { + SLIM_CH_ACTIVATE, + SLIM_CH_SUSPEND, + SLIM_CH_REMOVE, +}; + /** * struct slim_stream_runtime - SLIMbus stream runtime instance *
On 5/7/24 08:30, Viken Dadhaniya wrote:
Currently slimbus driver doesn't support stream disable callback, it only supports stream enable callback.
In slimbus usecase, client is switching to new frequency with same channel and calling enable stream callback for new frequency but DSP subsystem is crashing as we are switching to new frequency with same channel without disabling stream for older frequency.
This is very difficult to read
but AFAICU comes down to: "Trying to switch frequencies without closing the channel results in an attempt to re-enable the channel without a clean shutdown, which then leads to a crash on the ADSP."
Ideally, before switching to another frequency, client should call disable stream callback and then enable stream for newer frequency.
Hence add support to disable stream via qcom_slim_ngd_disable_stream().
Signed-off-by: Viken Dadhaniya quic_vdadhani@quicinc.com
drivers/slimbus/qcom-ngd-ctrl.c | 70 +++++++++++++++++++++++++++++++++ drivers/slimbus/slimbus.h | 13 ++++++ 2 files changed, 83 insertions(+)
diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c index e0b21f0f79c1..d952827d2e12 100644 --- a/drivers/slimbus/qcom-ngd-ctrl.c +++ b/drivers/slimbus/qcom-ngd-ctrl.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 // Copyright (c) 2011-2017, The Linux Foundation. All rights reserved. // Copyright (c) 2018, Linaro Limited +// Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
#include <linux/irq.h> #include <linux/kernel.h> @@ -1084,6 +1085,74 @@ static int qcom_slim_ngd_enable_stream(struct slim_stream_runtime *rt) return ret; }
+static int qcom_slim_ngd_disable_stream(struct slim_stream_runtime *rt) +{
- struct slim_device *sdev = rt->dev;
- struct slim_controller *ctrl = sdev->ctrl;
- struct slim_val_inf msg = {0};
- u8 wbuf[SLIM_MSGQ_BUF_LEN];
- u8 rbuf[SLIM_MSGQ_BUF_LEN];
- struct slim_msg_txn txn = {0,};
{ 0 } is enough
Also, reverse-Christmas-tre, please
- int i, ret;
- txn.mt = SLIM_MSG_MT_DEST_REFERRED_USER;
- txn.dt = SLIM_MSG_DEST_LOGICALADDR;
- txn.la = SLIM_LA_MGR;
- txn.ec = 0;
- txn.msg = &msg;
- txn.msg->num_bytes = 0;
- txn.msg->wbuf = wbuf;
- txn.msg->rbuf = rbuf;
- for (i = 0; i < rt->num_ports; i++) {
struct slim_port *port = &rt->ports[i];if (txn.msg->num_bytes == 0) {wbuf[txn.msg->num_bytes++] = (u8)(SLIM_CH_REMOVE << 6)| (sdev->laddr & 0x1f);
Add a #define and use FIELD_PREP
ret = slim_alloc_txn_tid(ctrl, &txn);if (ret) {dev_err(&sdev->dev, "Fail to allocate TID\n");return -ENXIO;}wbuf[txn.msg->num_bytes++] = txn.tid;}wbuf[txn.msg->num_bytes++] = port->ch.id;- }
- txn.mc = SLIM_USR_MC_CHAN_CTRL;
- txn.rl = txn.msg->num_bytes + 4;
Why +4?
- ret = qcom_slim_ngd_xfer_msg_sync(ctrl, &txn);
- if (ret) {
slim_free_txn_tid(ctrl, &txn);dev_err(&sdev->dev, "TX timed out:MC:0x%x,mt:0x%x ret:%d\n",
Please clean this up, add commas to separate all three prints and a space after each comma
[...]
+/*
- enum slim_ch_control: Channel control.
- Activate will schedule channel and/or group of channels in the TDM frame.
- Suspend will keep the schedule but data-transfer won't happen.
- Remove will remove the channel/group from the TDM frame.
"will" suggests these are not immediate.
Konrad
On 5/7/2024 5:09 PM, Konrad Dybcio wrote:
On 5/7/24 08:30, Viken Dadhaniya wrote:
Currently slimbus driver doesn't support stream disable callback, it only supports stream enable callback.
In slimbus usecase, client is switching to new frequency with same channel and calling enable stream callback for new frequency but DSP subsystem is crashing as we are switching to new frequency with same channel without disabling stream for older frequency.
This is very difficult to read
but AFAICU comes down to: "Trying to switch frequencies without closing the channel results in an attempt to re-enable the channel without a clean shutdown, which then leads to a crash on the ADSP."
Sure, I will update and v2.
Ideally, before switching to another frequency, client should call disable stream callback and then enable stream for newer frequency.
Hence add support to disable stream via qcom_slim_ngd_disable_stream().
Signed-off-by: Viken Dadhaniya quic_vdadhani@quicinc.com
drivers/slimbus/qcom-ngd-ctrl.c | 70 +++++++++++++++++++++++++++++++++ drivers/slimbus/slimbus.h | 13 ++++++ 2 files changed, 83 insertions(+)
diff --git a/drivers/slimbus/qcom-ngd-ctrl.c b/drivers/slimbus/qcom-ngd-ctrl.c index e0b21f0f79c1..d952827d2e12 100644 --- a/drivers/slimbus/qcom-ngd-ctrl.c +++ b/drivers/slimbus/qcom-ngd-ctrl.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 // Copyright (c) 2011-2017, The Linux Foundation. All rights reserved. // Copyright (c) 2018, Linaro Limited +// Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. #include <linux/irq.h> #include <linux/kernel.h> @@ -1084,6 +1085,74 @@ static int qcom_slim_ngd_enable_stream(struct slim_stream_runtime *rt) return ret; } +static int qcom_slim_ngd_disable_stream(struct slim_stream_runtime *rt) +{ + struct slim_device *sdev = rt->dev; + struct slim_controller *ctrl = sdev->ctrl; + struct slim_val_inf msg = {0}; + u8 wbuf[SLIM_MSGQ_BUF_LEN]; + u8 rbuf[SLIM_MSGQ_BUF_LEN]; + struct slim_msg_txn txn = {0,};
{ 0 } is enough
Also, reverse-Christmas-tre, please
Sure, I will update and v2.
+ int i, ret;
+ txn.mt = SLIM_MSG_MT_DEST_REFERRED_USER; + txn.dt = SLIM_MSG_DEST_LOGICALADDR; + txn.la = SLIM_LA_MGR; + txn.ec = 0; + txn.msg = &msg; + txn.msg->num_bytes = 0; + txn.msg->wbuf = wbuf; + txn.msg->rbuf = rbuf;
+ for (i = 0; i < rt->num_ports; i++) { + struct slim_port *port = &rt->ports[i];
+ if (txn.msg->num_bytes == 0) { + wbuf[txn.msg->num_bytes++] = (u8)(SLIM_CH_REMOVE << 6) + | (sdev->laddr & 0x1f);
Add a #define and use FIELD_PREP
Sure, I will update and v2.
+ ret = slim_alloc_txn_tid(ctrl, &txn); + if (ret) { + dev_err(&sdev->dev, "Fail to allocate TID\n"); + return -ENXIO; + } + wbuf[txn.msg->num_bytes++] = txn.tid; + } + wbuf[txn.msg->num_bytes++] = port->ch.id; + }
+ txn.mc = SLIM_USR_MC_CHAN_CTRL; + txn.rl = txn.msg->num_bytes + 4;
Why +4?
Slimbus messages include a 4-byte header before the payload, so txn.rl adds 4 to cover that fixed header size.
+ ret = qcom_slim_ngd_xfer_msg_sync(ctrl, &txn); + if (ret) { + slim_free_txn_tid(ctrl, &txn); + dev_err(&sdev->dev, "TX timed out:MC:0x%x,mt:0x%x ret:%d\n",
Please clean this up, add commas to separate all three prints and a space after each comma
[...]
Sure, I will update and v2.
+/*
- enum slim_ch_control: Channel control.
- Activate will schedule channel and/or group of channels in the TDM frame.
- Suspend will keep the schedule but data-transfer won't happen.
- Remove will remove the channel/group from the TDM frame.
"will" suggests these are not immediate.
Sure, I will update and v2.
Konrad
participants (2)
-
Konrad Dybcio -
Viken Dadhaniya