Paths

Table of Contentst

gpio: attach gpiobus when the controller is ready
ClosedPublic
Actions

Authored by vexeduxr on Sun, Jun 29, 12:24 PM.

Details

Reviewers

imp
manu
mmel
andrew
jhibbits
kevans

Commits

rGcdba3a8f9fce: gpio: attach gpiobus when the controller is ready
rG4695e3aa7c68: gpio: attach gpiobus when the controller is ready

Summary

Only attach gpiobus when the controller is fully initialized. Children
of gpiobus expect this to be the case.

Test Plan

Tested the pl061 change, the rest are only compile tested.

Note that most drivers in the tree already do this. These are just the few that don't.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 65126
Build 62009: arc lint + arc unit

Event Timeline

vexeduxr created this revision.Sun, Jun 29, 12:24 PM

Herald added subscribers: loos, emaste. · View Herald TranscriptSun, Jun 29, 12:24 PM

vexeduxr requested review of this revision.Sun, Jun 29, 12:24 PM

Harbormaster completed remote builds in B65126: Diff 157733.Sun, Jun 29, 12:24 PM

vexeduxr edited the test plan for this revision. (Show Details)Sun, Jun 29, 12:25 PM

One commit for moving gpiobus_attach_bus() after init (seems fine to me)
and another for changes to pl061 (the ACPI/WDT interrupt changes seems unrelated).
Removing bus_attach_children() is questionable ( and probably wrong)
and moving bus_setup_intr() before HW init is clearly wrong.

sys/arm/nvidia/as3722.c
314	This is clearly wrong, you can't set an interrupt without having fully initialized HW and driver data.
331	Why did you remove the bus_attach_children() function? Ditto for other drivers.
sys/arm64/apple/apple_pinctrl.c
164	Why?
sys/arm64/nvidia/tegra210/max77620.c
418	Again, wrong change.

This revision now requires changes to proceed.Tue, Jul 1, 12:42 PM

In D51088#1166439, @mmel wrote:

and another for changes to pl061 (the ACPI/WDT interrupt changes seems unrelated).

They're required to move the intr_pic_register call to pl061_attach.

sys/arm/nvidia/as3722.c
314	Hm, I should just move the `gpiobus_attach_bus` call out instead of moving the whole function.
331	Because `gpiobus_attach_bus` already calls it.
sys/arm64/apple/apple_pinctrl.c
164	The PIC needs to be registered before we attach gpiobus. If a gpiobus child gets attached in the same bus pass, and said child tried to setup an interrupt, it would fail.

andrew added inline comments.Tue, Jul 1, 1:27 PM

sys/arm/nvidia/as3722.c
331	Would it make sense to make that a separate change?
sys/dev/gpio/pl061_acpi.c
76–80	`error` is now unneeded here and in the fdt attach.

vexeduxr added inline comments.Tue, Jul 1, 2:19 PM

sys/arm/nvidia/as3722.c
331	Yeah, I'll move it to a separate review.

Move as3722_gpio_attach and max77620_gpio_attach back up, and instead swap them with the rtc functions.
It's hard to tell if the interrupt is needed by the controller's children (presumably it is), but
they're both currently unimplemented.

Simplify the pl061 attach functions by tail calling pl061_attach.

Move the removal of bus_attach_children calls to a parent commit.

Harbormaster completed remote builds in B65151: Diff 157788.Tue, Jul 1, 2:32 PM

vexeduxr edited the summary of this revision. (Show Details)Tue, Jul 1, 2:34 PM

vexeduxr added a parent revision: D51108: gpio: remove redundant calls to bus_attach_children.Tue, Jul 1, 2:36 PM

max77620 and as3722 are much more problematic. I think each function expects the other functions to have HW initialized before they can be consumed.

In other words, and I hope more clearly:
Having bus_attach_children() in gpiobus_attach_bus() is a nice shortcut for trivial cases, but a nightmare for complex ones. Personally, I vote for creating a 'clone' of gpiobus_attach_bus() without scanning children and using it in those cases along with explicit bus_attach_children() in the driver.

What do you think about that?

sys/arm64/apple/apple_pinctrl.c
164	However, you moved it before pinctrl configured the pins, so the result is again misordered. Imho, pinctrl init should be moved along with intr_pic_register().

In D51088#1166892, @mmel wrote:

max77620 and as3722 are much more problematic. I think each function expects the other functions to have HW initialized before they can be consumed.

I see, I'll revert the move.

In D51088#1166892, @mmel wrote:

In other words, and I hope more clearly:
Having bus_attach_children() in gpiobus_attach_bus() is a nice shortcut for trivial cases, but a nightmare for complex ones. Personally, I vote for creating a 'clone' of gpiobus_attach_bus() without scanning children and using it in those cases along with explicit bus_attach_children() in the driver.

What do you think about that?

Hmm, is there a reason we need to add a child in max77620_gpio_attach/as3722_gpio_attach specifically? I think we should just move the gpiobus_attach_bus call after hw init is done.

sys/arm64/apple/apple_pinctrl.c
164	Agreed. I think I just got confused since I saw other drivers doing it this way.

In D51088#1167027, @vexeduxr wrote:

Hmm, is there a reason we need to add a child in max77620_gpio_attach/as3722_gpio_attach specifically? I think we should just move the gpiobus_attach_bus call after hw init is done.

Actually, I've changed my mind. You're right, even if the max77620 and as3722 drivers don't *need* this functionality, adding a separate function is definitely a cleaner way to implement it.

Move max77620 and as3722's gpio_attach functions to their original place,
and use gpiobus_add_bus instead.

Move the pinctrl functions before gpiobus attachment too.
Also fix a few more drivers that do this.

Harbormaster completed remote builds in B65181: Diff 157852.Wed, Jul 2, 3:57 PM

vexeduxr added a parent revision: D51133: gpiobus: add a gpiobus_add_bus function.Wed, Jul 2, 3:57 PM

Thank you very much for your patience. Please don't forget the MFC for this whole series.

This revision is now accepted and ready to land.Thu, Jul 3, 7:06 AM

andrew accepted this revision.Thu, Jul 3, 8:34 AM

andrew added inline comments.

sys/arm/allwinner/aw_gpio.c
1160–1163 ↗	(On Diff #157852)	Do we need to undo this in the failure case?

vexeduxr added inline comments.Thu, Jul 3, 9:56 AM

sys/arm/allwinner/aw_gpio.c
1160–1163 ↗	(On Diff #157852)	All `fdt_pinctrl_register` does is call `OF_device_register_xref` on a few nodes, so I think that's fine. However, `fdt_pinctrl_configure_tree` calls `FDT_PINCTRL_CONFIGURE` to configure pins, which is controller specific. These controllers don't have an "unconfigure" function. IMO It's fine to configure pins then bail if an error occurs.