Generic OHCI and Allwinner OHCI

[manu_bidouilliste.com]

Summary

Looking at our different embedded ohci driver they are almost the same except for the SoC clocks bits.
In the allwinner DTS the ohci controller have an additional compatible string set to "generic-ohci".
I've added a generic_ohci driver with an interface for init and deinit function that are responsible for enable/disable the clocks or others SoC bits.
We still can't use the "generic-ohci" driver directly because of clocks so method is disabled for now.
This also bring the driver for allwinner A10 and A20.

Test Plan

Apply patch and test that ohci is working.

[jmcneill_invisible.ca]

sys/arm/allwinner/a10_clk.c

153 ↗

(On Diff #13840)

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

sys/arm/allwinner/a10_clk.h

114 ↗

(On Diff #13840)

I think these should be sorted by bit number.

sys/arm/allwinner/aw_ohci.c

30 ↗

(On Diff #13840)

Is this needed?

sys/dev/usb/controller/generic_ohci.c

35 ↗

(On Diff #13840)

Is this needed?

41 ↗

(On Diff #13840)

condvar.h might be unnecessary as well

189 ↗

(On Diff #13840)

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

[manu_bidouilliste.com]

sys/arm/allwinner/aw_ohci.c

30 ↗

(On Diff #13840)

Leftover from the convertion, thanks

sys/dev/usb/controller/generic_ohci.c

35 ↗

(On Diff #13840)

Leftover from the convertion, thanks

41 ↗

(On Diff #13840)

It's needed by usb_busdma.h

[manu_bidouilliste.com]

Update patch to reflect jmcneill comments.

[manu_bidouilliste.com]

Forgot one comment.

[jmcneill_invisible.ca]

sys/arm/allwinner/a10_clk.c

145 ↗

(On Diff #13850)

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

176 ↗

(On Diff #13850)

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

208 ↗

(On Diff #13850)

++var

Should this be ohci_cnt?

241 ↗

(On Diff #13850)

--ohci_cnt

[manu_bidouilliste.com]

Fix increment/decrement
Fix variable name.

[jmcneill_invisible.ca]

sys/arm/allwinner/a10_clk.c

148 ↗

(On Diff #13851)

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.

[manu_bidouilliste.com]

Split clock function for USBPHY
Rename USBPHY reg.

[jmcneill]

sys/arm/allwinner/a10_clk.c

160 ↗

(On Diff #13863)

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

185 ↗

(On Diff #13863)

Call this before dropping the lock.

218 ↗

(On Diff #13863)

Call this before dropping the lock.

250 ↗

(On Diff #13863)

Call this before dropping the lock.

258 ↗

(On Diff #13863)

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

264 ↗

(On Diff #13863)

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

284 ↗

(On Diff #13863)

Pass sc as parameter to simplify.

290 ↗

(On Diff #13863)

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

[manu_bidouilliste.com]

Do not lock in usb/usbphy, assert instead.
Pass sc to usb/usbphy functions.

[manu_bidouilliste.com]

Remove unneeded check.

[jmcneill]

sys/conf/files

2572 ↗

(On Diff #13865)

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

[manu_bidouilliste.com]

sys/conf/files

2572 ↗

(On Diff #13865)

And rename generic_ohci to ohci_fdt too ?

[manu_bidouilliste.com]

Rename generic_ohci to ohci_fdt.
Merge last head and rename a10_clk_usb_activate to a10_clk_ehci_activate in a10_ehci.

[jmcneill]

sys/arm/allwinner/a10_clk.h

135 ↗

(On Diff #13976)

Sorting

[manu_bidouilliste.com]

Orders defines.

[jmcneill]

sys/dev/usb/controller/ohci_fdt.c

97 ↗

(On Diff #14093)

4 spaces for multi-line if

104 ↗

(On Diff #14093)

if (!sc->sc_io_res) {

116 ↗

(On Diff #14093)

if (!sc->sc_irq_res) {

120 ↗

(On Diff #14093)

if (!sc->sc_bus.bdev) {

135 ↗

(On Diff #14093)

Assigning err here but the error path always returns ENXIO.

[manu_bidouilliste.com]

Remove clocking bits as they have been commited to HEAD.
Fix a few style(9) issues.
Remove ohci_fdt device and use "ohci fdt" in sys/conf/files

[• hselasky]

Looks good to me. Did you resolve all the style issues?

Do you think the device init and deinit methods you're adding should be more generic, like covering XHCI/EHCI/UHCI and so on aswell?

[manu_bidouilliste.com]

In D5481#118748, @hselasky wrote:

Looks good to me. Did you resolve all the style issues?

Do you think the device init and deinit methods you're adding should be more generic, like covering XHCI/EHCI/UHCI and so on aswell?

All the style issues are resolved.
For EHCI/XHCI etc ... I want to do the same, add a generic driver + interface so yeah I guess that the interface could be more generic. I'll look at it soon.

[• hselasky]

Might be you want to put the additional interface methods inside the existing usb_if.m or maybe a new usb_controller_if.m

--HPS

[mmel]

Do you have any usage pattern for subclassing of ohci_fdt.c? I'm not able to get any way how to do it.

sys/conf/files

2572 ↗

(On Diff #14098)

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.

sys/dev/usb/controller/ohci_fdt.c

116 ↗

(On Diff #14098)

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

220 ↗

(On Diff #14098)

static ?

[manu_bidouilliste.com]

See aw_ohci.c for subclassed driver.

sys/conf/files

2572 ↗

(On Diff #14098)

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.

sys/dev/usb/controller/ohci_fdt.c

116 ↗

(On Diff #14098)

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

220 ↗

(On Diff #14098)

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

[manu_bidouilliste.com]

Rename driver generic_ohci_fdt
Rename the interface generic_usb_fdt_if as it could be use for an upcoming generic_ehci.
Fix a few style(9) issues.

[andrew]

sys/dev/usb/controller/generic_ohci_fdt.c

64–81 ↗

(On Diff #14170)

This should be removed as it's unused.

[andrew]

I've tested this on arm64 with some busdma fixes, it seems to work.

sys/dev/usb/controller/generic_ohci_fdt.c

64–81 ↗

(On Diff #14170)

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.

136 ↗

(On Diff #14170)

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

219 ↗

(On Diff #14170)

This should be .name = "ohci",

[manu_bidouilliste.com]

sys/dev/usb/controller/generic_ohci_fdt.c

64–81 ↗

(On Diff #14170)

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.

136 ↗

(On Diff #14170)

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

[manu_bidouilliste.com]

Rename to generic_ohci
Add support for clocks/reset if compiled with EXT_RESOURCES
Remove subclassed driver aw_ohci as this work with D5752

[jmcneill]

sys/dev/usb/controller/generic_ohci.c

143 ↗

(On Diff #14746)

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

[manu_bidouilliste.com]

Save ext_resources in softc to release them correctly.

[manu_bidouilliste.com]

Add generic_ohci.c and generic_usb_if.m in files.allwinner instead of sys/conf/files
Make them depend on ohci instead of generic_ohci.