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?
> Signed-off-by: Kishore Batta <kishore.batta@xxxxxxxxxxxxxxxx>
> ---
> 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?
>
> 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.
> + 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.
> + * 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?
> +}
> +
> 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
>
> if (!mhi_result->transaction_status) {
> context->rx_size = mhi_result->bytes_xferd;
> --
> 2.34.1
>
