From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: qcpas(4) mdt init: add retry
To: Marcus Glocker <marcus@nazgul.ch>
Cc: tech@openbsd.org
Date: Sat, 16 May 2026 22:39:48 +0200

On Sat, 16 May 2026 18:12:25 +0200,
Marcus Glocker <marcus@nazgul.ch> wrote:
> 
> On the Samsung Galaxy Book4 Edge every since I am getting intermittent
> mdt initialization errors from qcpas(4) looking like
> 
> 	qcpas0: failed to shutdown lite firmware
> 	qcpas0: failed to receive ready signal
> 	qcpas0: failed to boot coprocessor
> 
> leaving me without the qcpas(4) HW sensors (they anyway ship no data
> on the Samsung currently, but that's a different story I want to tackle
> next).
> 
> Implementing a retry loop will help to overcome this situation:
> 
> 	qcpas0: failed to shutdown lite firmware
> 	qcpas0: failed to receive ready signal
> 	qcpas0: coprocessor boot attempt 2
> 	qcpas0: failed to receive ready signal
> 	qcpas0: coprocessor boot attempt 3
> 
> Feedback, regression testers, OKs?
>

Tested on my honor, it doesn't solve my issue, but it also do not introduce
any regression. If it fixes your machine I think it make sense to commit and
I OK with it, but I suggest to wait more tests and feedback.

On my device it errors like that:

qcpas0: failed to receive ready signal
qcpas0: coprocessor boot attempt 2
qcpas0: failed to receive ready signal
qcpas0: coprocessor boot attempt 3
qcpas0: failed to receive ready signal
qcpas0: failed to boot coprocessor

I've tried increase number of attempts, add pause... nothing.

> 
> Index: sys/dev/fdt/qcpas.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/fdt/qcpas.c,v
> diff -u -p -u -p -r1.12 qcpas.c
> --- sys/dev/fdt/qcpas.c	1 Aug 2025 08:55:09 -0000	1.12
> +++ sys/dev/fdt/qcpas.c	16 May 2026 15:50:14 -0000
> @@ -211,7 +211,7 @@ qcpas_mountroot(struct device *self)
>  	char fwname[128];
>  	size_t fwlen, dtb_fwlen;
>  	u_char *fw, *dtb_fw;
> -	int node, ret;
> +	int i, node, ret;
>  	int error;
>  
>  	if (qcpas_map_memory(sc) != 0)
> @@ -272,7 +272,19 @@ qcpas_mountroot(struct device *self)
>  		free(dtb_fw, M_DEVBUF, dtb_fwlen);
>  	}
>  
> -	ret = qcpas_mdt_init(sc, sc->sc_pas_id, fw, fwlen);
> +	/*
> +	 * Coprocessor boot is intermittent on some platforms
> +	 * (e.g. Samsung Galaxy Book4 Edge): all of the SCM calls
> +	 * succeed but the firmware sometimes silently dies before
> +	 * signalling ready.  Retry up to a few times -- when it
> +	 * works, the ready signal arrives within a few hundred ms.
> +	 */
> +	for (ret = -1, i = 0; ret != 0 && i < 3; i++) {
> +		if (i > 0)
> +			printf("%s: coprocessor boot attempt %d\n",
> +			    sc->sc_dev.dv_xname, i + 1);
> +		ret = qcpas_mdt_init(sc, sc->sc_pas_id, fw, fwlen);
> +	}
>  	free(fw, M_DEVBUF, fwlen);
>  	if (ret != 0) {
>  		printf("%s: failed to boot coprocessor\n",
> @@ -467,6 +479,7 @@ qcpas_mdt_init(struct qcpas_softc *sc, i
>  	    QCPAS_DMA_DVA(sc->sc_metadata[idx])) != 0) {
>  		printf("%s: init image failed\n", sc->sc_dev.dv_xname);
>  		qcpas_dmamem_free(sc, sc->sc_metadata[idx]);
> +		sc->sc_metadata[idx] = NULL;
>  		return EINVAL;
>  	}
>  
> @@ -474,6 +487,7 @@ qcpas_mdt_init(struct qcpas_softc *sc, i
>  	    sc->sc_mem_phys[idx], maxpa - minpa) != 0) {
>  		printf("%s: mem setup failed\n", sc->sc_dev.dv_xname);
>  		qcpas_dmamem_free(sc, sc->sc_metadata[idx]);
> +		sc->sc_metadata[idx] = NULL;
>  		return EINVAL;
>  	}
>  
> @@ -509,6 +523,7 @@ qcpas_mdt_init(struct qcpas_softc *sc, i
>  	if (qcscm_pas_auth_and_reset(pas_id) != 0) {
>  		printf("%s: auth and reset failed\n", sc->sc_dev.dv_xname);
>  		qcpas_dmamem_free(sc, sc->sc_metadata[idx]);
> +		sc->sc_metadata[idx] = NULL;
>  		return EINVAL;
>  	}
>  
> @@ -519,6 +534,14 @@ qcpas_mdt_init(struct qcpas_softc *sc, i
>  	if (error) {
>  		printf("%s: failed to receive ready signal\n",
>  		    sc->sc_dev.dv_xname);
> +		/*
> +		 * The coprocessor was started but never signalled ready.
> +		 * Shut it down and release the metadata buffer so the
> +		 * caller can retry from scratch.
> +		 */
> +		qcscm_pas_shutdown(pas_id);
> +		qcpas_dmamem_free(sc, sc->sc_metadata[idx]);
> +		sc->sc_metadata[idx] = NULL;
>  		return error;
>  	}
> 

-- 
wbr, Kirill