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

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Wed, Sep 17, 2025 at 10:13:27PM +0200, Pierre-Louis Bossart wrote:
> 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.

This is the other timeout I was asking about after you so
usefully pointed out the DisCo property I was missing for the
UMPs.

Presuming you are referring to the FDL status. That will keep
track of the progress of a single FDL set. But I can't see
anything that prevents a device requesting a second set (and in
fact our device does). So after the first FDL has been complete
one has to wait to see if the device initiates any further FDL.

All the spec says in step 9, 13.1, v1.1 is "Wait until there are
no new requests for File Download". But if I am missing some
additional status field that indicates the device is happy and
ready to rock that would be awesome?

> >  /**
> >   * 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.

These are not so much concepts from the state machine as just the
naive interpretation of their names. Begin means an FDL has
begun, and done means that FDL has completed. We track the begin
because as far as I can see the only signal the device is fully
ready is that it doesn't initiate an FDL for a while.

> 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.

I think what I am missing here is what is the mechanism for that
telling you it no longer needs firmware? As far as I can see the
only mechanism is an IRQ on the FDL UMP buffer, so we wait for
that.

> > +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?

This is just an arbitrary number of retries before we give up.
Basically for the UMP timeout case you wanted added, we now reset
the function, so hopefully it will succeed on the next go.

Sorry what do you mean by 8 sets with 3 bits for the status? I
am a little concerned I am missing something here. As far as
I can see Set_Index is a 1 byte control, so 256 possible set
numbers, and the FDL status is also 1 byte, so 8 bits for
the status.

> > +	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?

There probably is a single interrupt, but you need to find it
first. This is called from the function boot, so it doesn't
automatically have access to what that XU is, it is faster to
search the IRQs than to search all the entities, as there is a
lot less of them typically.

Also I don't think it is a spec requirement that there is only a
single XU FDLing, so the code supports more.

> > +			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?

Perhaps I am misunderstanding here, but NEEDS_SET contains no
data, it just tells you to read the FDL_Set_Index control. Which
then gives you a single set index to download. The device may
then go on to request a second set?

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

lol oops, as accidentally hilarious as this is I will fix.

Thanks,
Charles