> From: David Gwynne <[email protected]>
> Date: Fri, 4 Jan 2019 08:54:17 +1000
>
> > On 3 Jan 2019, at 20:54, Mark Kettenis <[email protected]> wrote:
> >
> >> + /* Map and establish the interrupt. */
> >> + splassert(IPL_USB);
> >> + sc->sc_ih = fdt_intr_establish(faa->fa_node, IPL_USB,
> >> + ohci_intr, &sc->sc, devname);
> >> + if (sc->sc_ih == NULL) {
> >> + printf(": can't establish interrupt\n");
> >> + goto disable_clocks;
> >> + }
> >> + printf(": fdt");
> >
> > Don't really want this.
>
> I intend to tweak ohci_handover() so it doesnt print a preceding ", ".
> Until then, the extra fdt just gives the comma something to sit
> next to.
Ah, right. Think you mean ohci_checkrev() though. On other busses we
print an ionterrupt description here. Never bothered with that for
fdt, but perhaps we should. But that's a whole other can of worms.
Yes, I think the caller should be printing the ", " here instead. But
that can be addresses in a separate diff.
> >> +
> >> + strlcpy(sc->sc.sc_vendor, "Generic", sizeof(sc->sc.sc_vendor));
> >> +
> >> + /* Display revision and perform legacy emulation handover. */
> >> + if (ohci_checkrev(&sc->sc) != USBD_NORMAL_COMPLETION ||
> >> + ohci_handover(&sc->sc) != USBD_NORMAL_COMPLETION) {
> >> + goto disestablish_intr;
> >> + }
> >
> > I suspect that calling ohci_handover() here isn't really necessary.
> > Perhaps leave that out (and adjust the comment).
>
> Because there's no smm to take over from?
Right. In theory there could be "secure" firmware on arm64 systems
that does similar trickery, but the main reason SMM gets involved is
to emulate a PC keyboard controller and there is no need to do that on
non-PC platforms.
> You could argue that checkrev might not be needed either, all it
> really does is set the usbrev on the bus.
Well, the information that it prints might be somewhat useful. And at
least it makes things consistent with how we print things on other
systems.
So your diff without the static and with the handover call taken out
is ok kettenis@
