Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add reference counting to GPIO pins to init/deinit clock control to port #139

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add reference counting to GPIO pins to init/deinit clock control to port #139

wants to merge 1 commit into from

Conversation

tullom
Copy link
Contributor

@tullom tullom commented Oct 23, 2024 •
edited
Loading

Resolves #61
GPIO port clocks are now disabled if no pin from that port is active.

gpio::init() has been changed to disable all clocks and only explicitly enable the port clock when a gpio pin (Input, Output, Flex) is instantiated.

@tullom tullom force-pushed the gpio-clock-init-and-deinit-ref-counting branch from ebbf70c to f0c462d Compare October 23, 2024 23:02
tullom
tullom commented Oct 24, 2024
View reviewed changes
Copy link
Contributor Author

@tullom tullom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some kicking-off questions

@felipebalbi felipebalbi added the enhancement New feature or request label Oct 29, 2024
@tullom tullom force-pushed the gpio-clock-init-and-deinit-ref-counting branch 3 times, most recently from b0a661e to 49ccb21 Compare November 1, 2024 02:35
@tullom tullom marked this pull request as ready for review November 1, 2024 02:36
@tullom tullom requested a review from felipebalbi November 1, 2024 02:36
madeleyneVaca
madeleyneVaca requested changes Nov 6, 2024
View reviewed changes
@tullom tullom requested a review from madeleyneVaca November 8, 2024 18:22
@tullom tullom force-pushed the gpio-clock-init-and-deinit-ref-counting branch from 20fa507 to 9f8c6a4 Compare November 8, 2024 18:44
@tullom tullom closed this Nov 8, 2024
@tullom tullom force-pushed the gpio-clock-init-and-deinit-ref-counting branch from 9f8c6a4 to 1d57614 Compare November 8, 2024 18:48
@tullom tullom reopened this Nov 8, 2024
felipebalbi
felipebalbi requested changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@felipebalbi felipebalbi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as an overall comment, since you're refcounting the clocks for GPIO, why isn't this done as part of the clock driver? We already have a good trait, IIRC, for modelling syscon peripherals. Could that be used together with refcounting to have globally refcounted clocks?

src/gpio.rs Outdated
Comment on lines 115 to 123
disable::<peripherals::HSGPIO0>();
disable::<peripherals::HSGPIO1>();
disable::<peripherals::HSGPIO2>();
disable::<peripherals::HSGPIO3>();
disable::<peripherals::HSGPIO4>();
disable::<peripherals::HSGPIO5>();
disable::<peripherals::HSGPIO6>();
disable::<peripherals::HSGPIO7>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you disable the peripheral during initialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some testing and it looks like the gpio clocks are being enabled somewhere in our embassy_imxrt::init() function, so enable_and_resetting() here technically does nothing. I double checked this with asserting cc1.pscctl1().read().hsgpio0_clk().is_disable_clock() before instantiating the first pin. Since we want to save power, i think these should be off by default. But to your general comment, if we move this to the clock code then this will probably disappear as we can (maybe) disable the clock there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: see comment below for justification for why i don't think moving to the clock driver is correct here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clocks are enabled as part of gpio::init() which is called by the application (should it be, though?). Either way, disabling the clocks here is the wrong thing to do. Rather, you should review the current clock usage for this driver and make sure it matches what you're trying to achieve, fixing any discrepancies along the way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should just remove the gpio::init() from embassy_imxrt::init() if now it is automatically enabled and disabled with your changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the interrupt logic in the artifacts of init() be moved into the refcounting logic? aka, calling unpend() and enable() when initing and pend() and disable() when deiniting?

jerrysxie
jerrysxie requested changes Nov 15, 2024
View reviewed changes
@tullom tullom force-pushed the gpio-clock-init-and-deinit-ref-counting branch 4 times, most recently from 63ea072 to 5062c65 Compare November 22, 2024 18:26
@tullom
Copy link
Contributor Author

tullom commented Nov 22, 2024

To @felipebalbi's point as to why this isn't done as part of the clock driver, i think that implementing this on the peripheral level allows for more flexibility since we are using the clocks::enable_and_reset() clock fn's in the new() function of the peripherals. In order to set up refcounting for those, we would need to implement drop for those peripherals as well (to call clocks::disable()), which i think is beyond the scope of this PR

@felipebalbi
Copy link
Contributor

To @felipebalbi's point as to why this isn't done as part of the clock driver, i think that implementing this on the peripheral level allows for more flexibility since we are using the clocks::enable_and_reset() clock fn's in the new() function of the peripherals. In order to set up refcounting for those, we would need to implement drop for those peripherals as well (to call clocks::disable()), which i think is beyond the scope of this PR

once could argue that a PR implementing refcounting always maintains it balanced, regardless of the method used for balacing those calls. Rust just happens to provide this nice Drop trait which can be used for this. Additionally, we should disable clocks when the peripheral is dropped (or, decrement the refcount).

jerrysxie
jerrysxie requested changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@jerrysxie jerrysxie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need a rebase on main as there is conflicts.

src/gpio.rs Outdated
Comment on lines 115 to 123
disable::<peripherals::HSGPIO0>();
disable::<peripherals::HSGPIO1>();
disable::<peripherals::HSGPIO2>();
disable::<peripherals::HSGPIO3>();
disable::<peripherals::HSGPIO4>();
disable::<peripherals::HSGPIO5>();
disable::<peripherals::HSGPIO6>();
disable::<peripherals::HSGPIO7>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should just remove the gpio::init() from embassy_imxrt::init() if now it is automatically enabled and disabled with your changes.

felipebalbi
felipebalbi reviewed Dec 3, 2024
View reviewed changes
@tullom tullom force-pushed the gpio-clock-init-and-deinit-ref-counting branch 2 times, most recently from 215fb9b to c0642ca Compare December 9, 2024 17:20
@tullom
Copy link
Contributor Author

tullom commented Dec 9, 2024

Yeah, we should just remove the gpio::init() from embassy_imxrt::init() if now it is automatically enabled and disabled with your changes.

Can the interrupt logic in the artifacts of init() be moved into the refcounting logic? aka, calling unpend() and enable() when initing and pend() and disable() when deiniting?

jerrysxie
jerrysxie approved these changes Dec 10, 2024
View reviewed changes
Copy link
Contributor

@jerrysxie jerrysxie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Please coordinate with @madeleyneVaca to have them test their ADO SoC repos against this change before uptake this. This introduces the drop semantic for gpio so if they are not holding onto the ownership of the pins. It could produce bugs.

@tullom tullom force-pushed the gpio-clock-init-and-deinit-ref-counting branch from c0642ca to 0fa0e82 Compare December 11, 2024 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerrysxie jerrysxie jerrysxie approved these changes

@JamesHuard JamesHuard Awaiting requested review from JamesHuard JamesHuard is a code owner

@madeleyneVaca madeleyneVaca Awaiting requested review from madeleyneVaca madeleyneVaca is a code owner

@felipebalbi felipebalbi Awaiting requested review from felipebalbi felipebalbi is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@tullom tullom

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create GPIO clock init and de-init api with reference counting
4 participants
@tullom @felipebalbi @jerrysxie @madeleyneVaca