Re: [PATCH v2 16/19] ASoC: SDCA: Add completion for FDL start and stop

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

On 9/12/25 12:35, Charles Keepax wrote:
> Add some completions and a helper function to allow other parts of the
> system to wait for FDL to complete. The sdca_fdl_sync() function will
> wait until it completes a full time out without a new FDL request
> happening, this ensures that even parts requiring multiple rounds of FDL
> should be fully downloaded before the driver boot continues.

I couldn't figure out what the last sentence means. In theory we have a status that can be checked to know if all the firmware sets have been processed by the device. so why do we need a 'timeout without a new FDL request'. The status tells you exactly when the FDL iterations are over.

 
>  /**
>   * struct fdl_state - FDL state structure to keep data between interrupts
> + * @begin: Completion indicating the start of an FDL download cycle.
> + * @done: Completion indicating the end of an FDL download cycle.
>   * @set: Pointer to the FDL set currently being downloaded.
>   * @file_index: Index of the current file being processed.
>   */
>  struct fdl_state {
> +	struct completion begin;
> +	struct completion done;
> +

maybe explain why you need to track the 'begin' phase? This isn't a concept I can map to the FDL state machine.

IIRC the device tells you if it needs firmware or not. It could very well be that it kept all the context and doesn't need to be re-initialized always. Adding a timeout in all cases doesn't seem quite right - or it should be something done for specific hardware that always needs to be re-initialized with firmware.


> +/**
> + * sdca_fdl_sync - wait for a function to finish FDL
> + * @dev: Device pointer for error messages.
> + * @function: Pointer to the SDCA Function.
> + * @info: Pointer to the SDCA interrupt info for this device.
> + *
> + * Return: Zero on success or a negative error code.
> + */
> +int sdca_fdl_sync(struct device *dev, struct sdca_function_data *function,
> +		  struct sdca_interrupt_info *info)
> +{
> +	static const int max_fdls = 10;

I think you can have 8 sets with 3 bits for the status. where does the 10 come from?

> +	unsigned long begin_timeout = msecs_to_jiffies(100);
> +	unsigned long done_timeout = msecs_to_jiffies(4000);
probably some defines would be nicer

> +	int nfdl;
> +	int i, j;
> +
> +	for (i = 0; i < max_fdls; i++) {
> +		nfdl = 0;
> +
> +		for (j = 0; j < SDCA_MAX_INTERRUPTS; j++) {
> +			struct sdca_interrupt *interrupt = &info->irqs[j];

I don't fully understand why you have the interrupt in the inner loop. For a given XU, one might think that there's a single interrupt for the XU UMP used for FDL, and that each set is sent with the same mechanism?

> +			struct fdl_state *fdl_state;
> +			unsigned long time;
> +
> +			if (interrupt->function != function ||
> +			    !interrupt->entity || !interrupt->control ||
> +			    interrupt->entity->type != SDCA_ENTITY_TYPE_XU ||
> +			    interrupt->control->sel != SDCA_CTL_XU_FDL_CURRENTOWNER)
> +				continue;
> +
> +			fdl_state = interrupt->priv;
> +			nfdl++;
> +
> +			/*
> +			 * Looking for timeout without any new FDL requests
> +			 * to imply the device has completed initial
> +			 * firmware setup. Alas the specification doesn't
> +			 * have any mechanism to detect this.
> +			 */
> +			time = wait_for_completion_timeout(&fdl_state->begin,
> +							   begin_timeout);
> +			if (!time) {
> +				dev_dbg(dev, "no new FDL starts\n");
> +				nfdl--;
> +				continue;
> +			}
> +
> +			time = wait_for_completion_timeout(&fdl_state->done,
> +							   done_timeout);
> +			if (!time) {
> +				dev_err(dev, "timed out waiting for FDL to complete\n");
> +				return -ETIMEDOUT;
> +			}
> +		}
> +
> +		if (!nfdl)
> +			return 0;
> +	}

Why don't you use the mask provided in the NEEDS_SET message?

> +
> +	dev_err(dev, "too many FDL quests\n");

requests?

> +	return -ETIMEDOUT;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_fdl_sync, "SND_SOC_SDCA");
> +
>  static char *fdl_get_sku_filename(struct device *dev,
>  				  struct sdca_fdl_file *fdl_file)
>  {
> @@ -225,6 +291,9 @@ static void fdl_end(struct sdca_interrupt *interrupt)
>  
>  	fdl_state->set = NULL;
>  
> +	pm_runtime_put(interrupt->dev);
> +	complete(&fdl_state->done);
> +
>  	dev_dbg(interrupt->dev, "completed FDL process\n");
>  }
>  
> @@ -238,6 +307,9 @@ static unsigned int fdl_status_process(struct sdca_interrupt *interrupt,
>  	case SDCA_CTL_XU_FDLD_NEEDS_SET:
>  		dev_dbg(interrupt->dev, "starting FDL process...\n");
>  
> +		pm_runtime_get(interrupt->dev);
> +		complete(&fdl_state->begin);
> +
>  		fdl_state->file_index = 0;
>  		fdl_state->set = fdl_get_set(interrupt);
>  		fallthrough;
> @@ -363,6 +435,9 @@ int sdca_fdl_alloc_state(struct sdca_interrupt *interrupt)
>  	if (!fdl_state)
>  		return -ENOMEM;
>  
> +	init_completion(&fdl_state->begin);
> +	init_completion(&fdl_state->done);
> +
>  	interrupt->priv = fdl_state;
>  
>  	return 0;