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

Hi.

On 2025-03-30 19:22, Hans Rosenfeld wrote:
> 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 theuda 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.

Ok. Good. Then I know where we're looking and how.

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

Ok. That diff looks good anyway.

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

Right. So in essence, not sure what the need for generating an interrupt 
is, and it could potentially be a strange/bad thing. But based on more 
information below, this is currently not relevant at all...

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

Oh. So we just write to IP, and nothing happens? That would be seriously 
broken.

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

And detecting errors are not even triggering? So it just sits and waits 
until timeout?

And one or two additional writes eventually gets it to a reset state?

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

Yeah. That fix should go in there. It might also be a good idea to 
somehow report if we get an error, I think.

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

I don't think it actually makes sense here. But this is clearly a 
different question as well. Setting the IE just means it would be doing 
interrupts for each initialization step. It have nothing to do with 
interrupts during normal operation.

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

Really strange. Are you booting from that controller? So it's currently 
operational when we get to udamatch() ?

   Johnny

-- 
Johnny Billquist                  || "I'm on a bus
                                   ||  on a psychedelic trip
email: [email protected]             ||  Reading murder books
pdp is alive!                     ||  tryin' to stay hip" - B. Idol