Re: [PATCH v1 08/12] drivers: soc: qcom: Support Sahara command mode packets(READY and EXECUTE)

[Kishore Batta <kishore.batta@oss.qualcomm.com>]

On 8/26/2025 4:28 AM, Bjorn Andersson wrote:
> On Mon, Aug 25, 2025 at 03:49:22PM +0530, Kishore Batta wrote:
>> During device boot, the device performs DDR training, and this training
>> data needs to be stored at the host end to improve subsequent boot times.
> As with the previous patch, I'd like to see this to be clarified. All
> devices? Some devices? The tail end of the sentence indicate that this
> is a performance improvement, is it?
>
> Describe when DDR training happens, how it relates to Sahara and what
> support a device that implements this needs from the host.
>
> Note also, that above and below text does not belong in the same
> paragraph, they are talking about different things.
>
>> The Sahara protocol specification indicates that DDR training data can
>> be stored at the host end using command mode packets. The device sends
>> the command mode packet to the host through the HELLO packet, and the
>> host changes its mode of operation accordingly.
>>
> Okay, so the HELLO packet contains the information about the mode...
>
>> Once the device determines that it needs to change the mode of operation
>> to command mode, it sends the command ready packet.
> ...but what does this then describe?
>
>> The host receives
>> the command ready packet and then sends command 8 to get the list of
>> commands supported by the device as per Sahara protocol specification.
>>
> And then what?
>
> Imagine that the reader doesn't know how the DDR training exchange over
> Sahara works when they start reading this, will they have developed that
> understanding when they get to the end of this text?
ACK. In v2, rewritten the commit message properly.
>> Signed-off-by: Kishore Batta <kishore.batta@oss.qualcomm.com>
>> ---
>>   drivers/soc/qcom/sahara/sahara.c | 91 +++++++++++++++++++++++++++++---
>>   1 file changed, 85 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/sahara/sahara.c b/drivers/soc/qcom/sahara/sahara.c
>> index df103473af3a..84327af48569 100644
>> --- a/drivers/soc/qcom/sahara/sahara.c
>> +++ b/drivers/soc/qcom/sahara/sahara.c
>> @@ -59,6 +59,9 @@
>>   #define SAHARA_RESET_LENGTH		0x8
>>   #define SAHARA_MEM_DEBUG64_LENGTH	0x18
>>   #define SAHARA_MEM_READ64_LENGTH	0x18
>> +#define SAHARA_COMMAND_READY_LENGTH	0x8
>> +#define SAHARA_COMMAND_EXECUTE_LENGTH	0xc
>> +#define SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST	0x8
>>   
>>   struct sahara_dev_trng_data {
>>   	void *trng_data;
>> @@ -66,6 +69,13 @@ struct sahara_dev_trng_data {
>>   	bool receiving_trng_data;
>>   };
>>   
>> +enum sahara_mode {
>> +	SAHARA_MODE_NONE,
>> +	SAHARA_MODE_MEM_DUMP,
>> +	SAHARA_MODE_CMD,
>> +	SAHARA_MODE_FW_DOWNLOAD,
>> +};
>> +
>>   struct sahara_packet {
>>   	__le32 cmd;
>>   	__le32 length;
>> @@ -100,6 +110,9 @@ struct sahara_packet {
>>   			__le64 memory_address;
>>   			__le64 memory_length;
>>   		} memory_read64;
>> +		struct {
>> +			__le32 client_command;
>> +		} command_execute;
>>   	};
>>   };
>>   
>> @@ -181,6 +194,7 @@ struct sahara_context {
>>   	void				*mem_dump_freespace;
>>   	u64				dump_images_left;
>>   	bool				is_mem_dump_mode;
>> +	enum sahara_mode		current_mode;
>>   };
>>   
>>   struct sahara_dev_driver_data {
>> @@ -282,8 +296,15 @@ static void sahara_release_image(struct sahara_context *context)
>>   static void sahara_send_reset(struct sahara_context *context)
>>   {
>>   	int ret;
>> +	struct sahara_dev_driver_data *sahara_data;
>> +	struct sahara_dev_trng_data *sdev;
>> +
>> +	sahara_data = dev_get_drvdata(&context->mhi_dev->dev);
>> +	sdev = sahara_data->sdev;
>>   
>>   	context->is_mem_dump_mode = false;
>> +	context->current_mode = SAHARA_MODE_NONE;
>> +	sdev->receiving_trng_data = false;
> This is never set to true. So yet another incomplete patch?
ACK. In v2, took care of having all assignments in single patch where 
its used.
>>   
>>   	context->tx[0]->cmd = cpu_to_le32(SAHARA_RESET_CMD);
>>   	context->tx[0]->length = cpu_to_le32(SAHARA_RESET_LENGTH);
>> @@ -297,6 +318,7 @@ static void sahara_send_reset(struct sahara_context *context)
>>   static void sahara_hello(struct sahara_context *context)
>>   {
>>   	int ret;
>> +	u32 mode;
>>   
>>   	dev_dbg(&context->mhi_dev->dev,
>>   		"HELLO cmd received. length:%d version:%d version_compat:%d max_length:%d mode:%d\n",
>> @@ -317,11 +339,17 @@ static void sahara_hello(struct sahara_context *context)
>>   		return;
>>   	}
>>   
>> -	if (le32_to_cpu(context->rx->hello.mode) != SAHARA_MODE_IMAGE_TX_PENDING &&
>> -	    le32_to_cpu(context->rx->hello.mode) != SAHARA_MODE_IMAGE_TX_COMPLETE &&
>> -	    le32_to_cpu(context->rx->hello.mode) != SAHARA_MODE_MEMORY_DEBUG) {
>> +	mode = le32_to_cpu(context->rx->hello.mode);
>> +
>> +	switch (mode) {
>> +	case SAHARA_MODE_IMAGE_TX_PENDING:
>> +	case SAHARA_MODE_IMAGE_TX_COMPLETE:
>> +	case SAHARA_MODE_MEMORY_DEBUG:
>> +	case SAHARA_MODE_COMMAND:
> You're effectively adding one more condition to the if statement. One
> can argue if the if statement or the switch is the cleaner way to write
> that, but when you replace 4 lines and add 11 it's hard to see that all
> you did was add one more accepted mode.
ACK.
>> +		break;
>> +	default:
>>   		dev_err(&context->mhi_dev->dev, "Unsupported hello packet - mode %d\n",
>> -			le32_to_cpu(context->rx->hello.mode));
>> +			mode);
>>   		return;
>>   	}
>>   
>> @@ -514,6 +542,46 @@ static void sahara_memory_debug64(struct sahara_context *context)
>>   		dev_err(&context->mhi_dev->dev, "Unable to send read for dump table %d\n", ret);
>>   }
>>   
>> +static void sahara_command_execute(struct sahara_context *context, u32 client_command)
>> +{
>> +	int ret;
>> +
>> +	context->tx[0]->cmd = cpu_to_le32(SAHARA_EXECUTE_CMD);
>> +	context->tx[0]->length = cpu_to_le32(SAHARA_COMMAND_EXECUTE_LENGTH);
>> +	context->tx[0]->command_execute.client_command = client_command;
>> +
>> +	ret = mhi_queue_buf(context->mhi_dev, DMA_TO_DEVICE, context->tx[0],
>> +			    SAHARA_COMMAND_EXECUTE_LENGTH, MHI_EOT);
>> +
>> +	if (ret)
>> +		dev_err(&context->mhi_dev->dev, "Unable to send command execute %d\n", ret);
>> +}
>> +
>> +static void sahara_command_ready(struct sahara_context *context)
>> +{
>> +	dev_dbg(&context->mhi_dev->dev,
>> +		"Command ready cmd received. Length:%d\n",
>> +		le32_to_cpu(context->rx->length));
>> +
>> +	if (le32_to_cpu(context->rx->length) != SAHARA_COMMAND_READY_LENGTH) {
>> +		dev_err(&context->mhi_dev->dev, "Malformed command ready packet - length %d\n",
>> +			le32_to_cpu(context->rx->length));
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * If the device sends the command ready packet, then its an indication
> "If the device sends" sounds conditional - but if you're here the device
> did send a command ready packet. And it's not an "indication", it really
> means that we're switching to command mode.
ACK.
>> +	 * to host that it needs to switch to command mode.
>> +	 */
>> +	context->current_mode = SAHARA_MODE_CMD;
>> +
>> +	/*
>> +	 * As per sahara spec, the host needs to send command ID 8 to get the
>> +	 * list of commands supported by the device.
>> +	 */
>> +	 sahara_command_execute(context, SAHARA_EXEC_CMD_GET_COMMAND_ID_LIST);
> What will the device send next? Where is this handled? In the next^2
> patch?
ACK. Handled all the logic properly in v2 under a patch so that things 
are clean.
>> +}
>> +
>>   static void sahara_processing(struct work_struct *work)
>>   {
>>   	struct sahara_context *context = container_of(work, struct sahara_context, fw_work);
>> @@ -538,6 +606,9 @@ static void sahara_processing(struct work_struct *work)
>>   	case SAHARA_MEM_DEBUG64_CMD:
>>   		sahara_memory_debug64(context);
>>   		break;
>> +	case SAHARA_CMD_READY_CMD:
>> +		sahara_command_ready(context);
>> +		break;
>>   	default:
>>   		dev_err(&context->mhi_dev->dev, "Unknown command %d\n",
>>   			le32_to_cpu(context->rx->cmd));
>> @@ -873,7 +944,11 @@ static int sahara_mhi_probe(struct mhi_device *mhi_dev, const struct mhi_device_
>>   
>>   static void sahara_mhi_remove(struct mhi_device *mhi_dev)
>>   {
>> -	struct sahara_context *context = dev_get_drvdata(&mhi_dev->dev);
>> +	struct sahara_dev_driver_data *sahara_data;
>> +	struct sahara_context *context;
>> +
>> +	sahara_data = dev_get_drvdata(&mhi_dev->dev);
>> +	context = sahara_data->context;
>>   
>>   	cancel_work_sync(&context->fw_work);
>>   	cancel_work_sync(&context->dump_work);
>> @@ -888,7 +963,11 @@ static void sahara_mhi_ul_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result
>>   
>>   static void sahara_mhi_dl_xfer_cb(struct mhi_device *mhi_dev, struct mhi_result *mhi_result)
>>   {
>> -	struct sahara_context *context = dev_get_drvdata(&mhi_dev->dev);
>> +	struct sahara_dev_driver_data *sahara_data;
>> +	struct sahara_context *context;
>> +
>> +	sahara_data = dev_get_drvdata(&mhi_dev->dev);
>> +	context = sahara_data->context;
> This was broken between the patches.
>
> Make sure you structure your patches such that:
> 1) One can reason about the whole thing that the patch introduces
> 2) Don't break the system inbetween any two patches - as this prevents
> the use of "git bisect"
>
> Regards,
> Bjorn
ACK.
>>   
>>   	if (!mhi_result->transaction_status) {
>>   		context->rx_size = mhi_result->bytes_xferd;
>> -- 
>> 2.34.1
>>