Instead of on/off you should use a counter here. Otherwise detaching a single instance of ehci/ohci will deactivate the others.

I think these should be sorted by bit number.

Is this needed?

Is this needed?

condvar.h might be unnecessary as well

ohci_detach writes to some registers, I don't think you want to be doing that after you've disabled clocks

Leftover from the convertion, thanks

Leftover from the convertion, thanks

It's needed by usb_busdma.h

This won't activate the device on the first call, you need ++sc->ehci_cnt

if (--sc->ehci_cnt == 0) {

++var

Should this be ohci_cnt?

--ohci_cnt

It looks like some of the clock gate and resets are shared between EHCI and OHCI. You'll need to track references to those separately.

You need to call this before dropping the lock, and pass in sc for convenience.

Call this before dropping the lock.

Call this before dropping the lock.

Call this before dropping the lock.

This can be simplified if you add softc as a parameter.

Instead of acquiring the lock here, assert that it is held.

Pass sc as parameter to simplify.

Same as activate -- assert that the lock is held instead.

Thinking these should be named ohci_fdt.c / ohci_fdt_if.m for consistency as the rest of the FDT specific drivers in that directory are also named <foo>_fdt.c

And rename generic_ohci to ohci_fdt too ?

Sorting

4 spaces for multi-line if

if (!sc->sc_io_res) {

if (!sc->sc_irq_res) {

if (!sc->sc_bus.bdev) {

Assigning err here but the error path always returns ENXIO.

I cannot agree. This is driver for single device named 'generic ohci', not a generic driver for all FDT based OHCI devices.
For me, consistent name is "generic_ohci' or 'generic_ohci_fdt'. In any case 'ohci_fdt' is misleading and unrelated to driver name.
Also, please, use standard way how drivers are declared in board config file. 'device generic_ohci' in board config file and 'optional ohci generic_ohci' in 'sys/conf/files' are OK, but including this driver for all kernels with FDT and OHCI defined doesn't looks right.

Here, and on may other places.
imho, style(9) allows use '!' only on boolean variables.

static ?

I understand your point. The thing is that this driver could also be subclassed for SoC who doesn't have generic_ohci, (If you look at the one for at91 or lpc for example switching to generic_ohci will be easy). but yeah this could be done even is the driver is renamed to generic_ohci. I'll change that.

Thanks, I'll look again at style(9) for this.

Because I need this struct when subclassing, if there is another method please let me know.

This should be removed as it's unused.

I've tested this on arm64. Could you implement this, even if the clock are not handled correctly. It would be useful there on at least one platform with fixed clocks.

I don't think this is FDT specific, it should move to be a generic USB controller interface.

This should be .name = "ohci",

I'm waiting for D5752 to implement the clocks.
We need the clock on Allwinner because otherwise driver will match and fail (hence the return (ENXIO) for now.

So rename the driver from generic_ohci_fdt to generic_ohci and interface from GENERIC_USB_FDT to GENERIC_USB ?

You should capture the clk and hwreset handles at attach time, otherwise you will leak the references on detach.