Re: [PATCH] ASoC: codec: pcm1681: Acquire sck when bound to a card

[Shawn Guo <shengchao.guo@oss.qualcomm.com>]

+ Srini

On Thu, Nov 05, 2026 at 05:42:03PM +0000, Mark Brown wrote:
> > The sck clock is taken once at i2c probe and held for the lifetime of
> > the device. On Qualcomm boards it is an LPASS clock exported by the
> > q6prm clock controller, which lives on the DSP and is unregistered and
> > registered anew whenever the ADSP is stopped and restarted. fw_devlink
> > cannot tie pcm1681 to that provider, since the provider node only gets
> > a device once the DSP is up, so it falls back to the remoteproc device,
> > which stays bound across a DSP restart. pcm1681 is therefore never
> > unbound and keeps a clk handle to the dead provider.
> 
> This seems like something that ought to be dealt with on the Qualcomm
> or fw_devlink side TBH, registering the clocks and returning -EBUSY on
> attempts to use them would be less disruptive to other components in the
> system.  I don't understand what fw_devlink would be doing with
> reparenting clocks...

Thanks for the review, Mark! I agree this belongs in the Qualcomm clock
provider, not in pcm1681, so I'll drop this patch.

Sorry, my wording about fw_devlink was misleading. No clock gets
reparented. My point was only that pcm1681 is never unbound when the DSP
restarts. The q6prm clock-controller device only exists while the DSP is
up. So fw_devlink links pcm1681 to the remoteproc device instead, which
stays bound across the restart.

Today the clocks can't stay registered across a DSP restart. When the DSP
stops, the GPR bus unregisters the q6prm device and its clock-controller
child, and that unregisters the clocks. The clk core then switches them
to clk_nodrv_ops, which return -ENXIO. That's where the "Failed to enable
sck: -6" in my report comes from. The consumer's struct clk handle can't
be reconnected after that.

So I'd like to change q6prm along the lines you suggested:

- Keep the clocks registered, on a device that is not removed when
  the DSP stops.
- While the DSP is down, make prepare/enable return -EBUSY.
- When the DSP comes back, re-send the requests for clocks that are still
  enabled, at their last set rate. This also fixes the rate being reset
  to the 19.2 MHz default.

The part I'm unsure about is which device should own the clocks:

a) A platform device that q6prm creates on its first probe and keeps
   until the module is unloaded. This needs no DT change.

b) The ADSP remoteproc device, which already survives restarts.
   It's cleaner, but probably needs a binding change, since the clock
   controller node is a child of qcom,q6prm today.

Srini, Mark, do you prefer a), b), or something else?

Shawn

> 
> > +static int pcm1681_component_probe(struct snd_soc_component *component)
> > +{
> > +	struct pcm1681_private *priv = snd_soc_component_get_drvdata(component);
> > +	struct device *dev = component->dev;
> > +	int ret;
> > +
> > +	priv->sck = clk_get_optional(dev, "sck");
> > +	if (IS_ERR(priv->sck)) {
> > +		ret = PTR_ERR(priv->sck);
> > +		priv->sck = NULL;
> > +		return dev_err_probe(dev, ret, "Failed to get sck\n");
> > +	}
> 
> This is the oppositie of the standard advice for drivers, and we don't
> deal well with probe deferral from component probes - in particular,
> nothing will trigger a rebind of the card if we don't probe a new
> component and that's not going to happen if the CODEC already bound and
> isn't waiting for the clock.
> 
> Also dev_err_probe() will only DTRT from device model probe.
> 
> -- 
> Mark Brown <broonie@kernel.org>