Page MenuHomeFreeBSD

Generic OHCI and Allwinner OHCI
ClosedPublic

Authored by manu_bidouilliste.com on Feb 28 2016, 3:23 AM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jmcneill_invisible.ca added inline comments.
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

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

Update patch to reflect jmcneill comments.

Forgot one comment.

manu_bidouilliste.com marked 8 inline comments as done.Feb 28 2016, 1:20 PM
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

Fix increment/decrement
Fix variable name.

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.

Split clock function for USBPHY
Rename USBPHY reg.

manu_bidouilliste.com marked 4 inline comments as done.Feb 29 2016, 1:06 AM
jmcneill added inline comments.Feb 29 2016, 1:10 AM
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.

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

Remove unneeded check.

manu_bidouilliste.com marked 10 inline comments as done.Feb 29 2016, 1:37 AM
jmcneill added inline comments.Mar 1 2016, 11:03 PM
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

sys/conf/files
2572 ↗(On Diff #13865)

And rename generic_ohci to ohci_fdt too ?

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

manu_bidouilliste.com marked 2 inline comments as done.Mar 2 2016, 11:30 AM
jmcneill added inline comments.Mar 4 2016, 9:47 PM
sys/arm/allwinner/a10_clk.h
135 ↗(On Diff #13976)

Sorting

manu_bidouilliste.com marked an inline comment as done.Mar 5 2016, 8:36 AM
jmcneill added inline comments.Mar 5 2016, 11:34 AM
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.

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 edited edge metadata.Mar 6 2016, 10:07 AM

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 marked 5 inline comments as done.Mar 6 2016, 10:35 AM

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.

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 added a subscriber: mmel.Mar 6 2016, 6:27 PM

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 ?

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 edited edge metadata.

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 added inline comments.Mar 26 2016, 11:33 AM
sys/dev/usb/controller/generic_ohci_fdt.c
64–81 ↗(On Diff #14170)

This should be removed as it's unused.

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",

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 ?

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 added inline comments.Apr 7 2016, 7:55 PM
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 marked 7 inline comments as done.

Save ext_resources in softc to release them correctly.

manu_bidouilliste.com marked an inline comment as done.Apr 27 2016, 3:17 PM

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.

jmcneill accepted this revision.May 16 2016, 10:45 AM
jmcneill added a reviewer: jmcneill.
This revision is now accepted and ready to land.May 16 2016, 10:45 AM
hselasky accepted this revision.May 16 2016, 11:00 AM
hselasky edited edge metadata.
This revision was automatically updated to reflect the committed changes.