Re: I/O bus reset to fix CMD MSCP controllers (and probably others)

[Hans Rosenfeld <hans%netbsd.org@localhost>]

On Sat, Mar 29, 2025 at 01:20:40PM +0100, Johnny Billquist wrote:
> On 2025-03-29 10:05, Hans Rosenfeld wrote:
> > On Sat, Mar 29, 2025 at 09:11:25AM +0100, Anders Magnusson wrote:
> > > The MP_STEP1 parts you quote below is just that.  It resets the uda and sees
> > > if it gets any answer.
> > > If it succeeds then STEP1MASK etc is checked later on to walk through the
> > > initialization process.
> > > 
> > > I do not know where Robert's CMD controller fails,
> > 
> > After the writing IP, repeated reads of SA read 0. I've put in a loop
> > after writing IP which reads SA once a second up to 60s and sometimes
> > there was no reaction at all until the retry.
> 
> You mean in that code, or some other test code?

In udamatch(), sys/dev/qbus/uda.c. I didn't test with any other code.

> mscp_waitstep is basically waiting 10 seconds for any kind of reply that
> match what is expected. The problem is that as long as SA and MASK is not
> equal to RESULT, it will continue waiting. So if you have an error
> condition, it will wait those 10 seconds.

I know. That's why I fixed this early on. It just turned out this wasn't
in any way relevant to the odd behaviour of the CMD controller.

> The same if something else picked up the completion of step 1.
> And since step 1 is enabling interrupts, it might get an interrupt in before
> you poll, at which point you're dependent on what the interrupt handler is
> doing. Or is this running with interrupts generally disabled?

Interrupts aren't enabled yet in any meaningful sense. As far as I
understand, the uba driver and/or the autoconf framework enabled
interrupts to catch any interrupts during autoconfiguration to detect
which devices use which interrupt vectors.

The match/probe functions of drivers are supposed to poke the device to
see whether it's there and whether it's actually what they expect it to
be. In general, they also try to cause an interrupt to discover the
vector being used, but in case of udamatch() it just programs the next
free vector it got passed from the autoconf framework, and that's where
the interrupt caused by udamatch() ultimately goes.

But none of that happens in udamatch() on a CMD controller because we
don't even get to step 1. Even if we fix early return and add a retry,
that's usually not enough to get it to step 1.

> Hmm, and by the way, when we say "step 1", are we talking about the
> controller even indicating that it is at step 1, or the response after we
> write the data in step 1?

I'm talking about SA indicating it's at step 1, following the write to IP:

	tries = 0;
again:

	bus_space_write_2(mi.mi_iot, mi.mi_iph, 0, 0); /* Start init */
	if (mscp_waitstep(&mi, MP_STEP1, MP_STEP1) == 0)
		return 0; /* Nothing here... */

> I think mscp_waitstep really should be modified to detect error states.

Sure, I have a patch attached to do that. It just isn't enough to get
the CMD controller going after boot. But it shouldn't hurt either, so
I'll probably just go and commit that, and the fix to udamatch() to try
again if we don't get to step 1 as well.

> Small question, why are we enabling interrupts in the initialization phase?
> Since we have code which is basically polling all through initialization, it
> would seem more natural to just not have interrupts involved here.

Thats really just the interrupt vector detection mechanism of autoconf,
at least as far as I have understood it. That goes back all the way to
4BSD if I'm not mistaken.

The real initialization of the controller happens in udaattach(),
including the real interrupt setup and all that. But for the CMD
controller, we don't even get there because udamatch() can't detect it.


Hans


-- 
%SYSTEM-F-ANARCHISM, The operating system has been overthrown

commit 35af6bc6397324da032281d334430051d63aaca8
Author: Hans Rosenfeld <rosenfeld%grumpf.hope-2000.org@localhost>
Date:   Thu Mar 27 20:10:44 2025 +0100

    mscp: return quickly from mscp_waitstep() if an error is detected

diff --git a/sys/dev/mscp/mscp_subr.c b/sys/dev/mscp/mscp_subr.c
index 70218c7d8df7..5e8ba36bc7f2 100644
--- a/sys/dev/mscp/mscp_subr.c
+++ b/sys/dev/mscp/mscp_subr.c
@@ -139,20 +139,22 @@ mscp_free_workitems(struct mscp_softc *mi)
 int
 mscp_waitstep(struct mscp_softc *mi, int mask, int result)
 {
-	int	status = 1;
+	int count;
+	int sa;
 
-	if ((READ_SA & mask) != result) {
-		volatile int count = 0;
-		while ((READ_SA & mask) != result) {
-			DELAY(10000);
-			count += 1;
-			if (count > DELAYTEN)
-				break;
-		}
-		if (count > DELAYTEN)
-			status = 0;
+	for (count = 0; count <= DELAYTEN; count++) {
+		sa = READ_SA;
+
+		if ((sa & MP_ERR) == MP_ERR)
+			return (0);
+
+		if ((sa & mask) == result)
+			return (1);
+
+		DELAY(10000);
 	}
-	return status;
+
+	return (0);
 }
 
 int