Received-SPF: pass (google.com: domain of linux-kernel+bounces-171207-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <247e4ce7-1ba2-43b8-8a11-ec70f99a4fc1@linaro.org>
Date: Tue, 7 May 2024 13:39:50 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1] slimbus: qcom-ngd-ctrl: Add stream disable support
To: Viken Dadhaniya <quic_vdadhani@quicinc.com>, andersson@kernel.org,
 srinivas.kandagatla@linaro.org, linux-arm-msm@vger.kernel.org,
 alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Cc: quic_msavaliy@quicinc.com, quic_vtanuku@quicinc.com,
 quic_anupkulk@quicinc.com, quic_cchiluve@quicinc.com
References: <20240507063004.21853-1-quic_vdadhani@quicinc.com>
Content-Language: en-US
From: Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <20240507063004.21853-1-quic_vdadhani@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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