Page MenuHomeFreeBSD

Generic OHCI and Allwinner OHCI
ClosedPublic

Authored by manu_bidouilliste.com on Feb 28 2016, 3:23 AM.
Tags
Referenced Files
Unknown Object (File)
Tue, Dec 24, 9:41 AM
Unknown Object (File)
Tue, Dec 24, 5:27 AM
Unknown Object (File)
Tue, Dec 24, 1:15 AM
Unknown Object (File)
Tue, Dec 24, 12:53 AM
Unknown Object (File)
Sat, Dec 21, 11:49 AM
Unknown Object (File)
Mon, Dec 9, 8:00 AM
Unknown Object (File)
Sat, Dec 7, 5:57 AM
Unknown Object (File)
Thu, Dec 5, 9:16 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 - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2729
Build 2750: arc lint + arc unit

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
166

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

I think these should be sorted by bit number.

sys/arm/allwinner/aw_ohci.c
31

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
31

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.

sys/arm/allwinner/a10_clk.c
153

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

165

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

180

++var

Should this be ohci_cnt?

194

--ohci_cnt

Fix increment/decrement
Fix variable name.

sys/arm/allwinner/a10_clk.c
156

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.

sys/arm/allwinner/a10_clk.c
168

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

182

Call this before dropping the lock.

215

Call this before dropping the lock.

247

Call this before dropping the lock.

255

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

261

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

281

Pass sc as parameter to simplify.

287

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

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

sys/conf/files
2572

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

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.

sys/arm/allwinner/a10_clk.h
135

Sorting

sys/dev/usb/controller/ohci_fdt.c
98

4 spaces for multi-line if

105

if (!sc->sc_io_res) {

117

if (!sc->sc_irq_res) {

121

if (!sc->sc_bus.bdev) {

136

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

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?

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

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

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
117

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

221

static ?

See aw_ohci.c for subclassed driver.

sys/conf/files
2572

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
117

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

221

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.

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

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.

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.

This revision is now accepted and ready to land.May 16 2016, 10:45 AM
This revision was automatically updated to reflect the committed changes.