Add support for GPIO internal pull up/down configuration on RPi4 family. #746
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BCM2711 SoC on 4th generation Raspberry Pi changed the way to configure its GPIO pins' internal pull up/down resistors. NetBSD already have support for this change, and port it to FreeBSD is trivial. This patch, based on the NetBSD commit adds the appropriate method for BCM2711 and now we can properly configure the GPIO pins' pull status. PR: 256372 Reference: NetBSD/src@bb88cfa
gmshake
reviewed
May 19, 2023
| BCM_GPIO_WRITE(sc, BCM_GPIO_GPPUDCLK(bank), BCM_GPIO_MASK(pin)); | ||
| BCM_GPIO_WRITE(sc, BCM_GPIO_GPPUD(0), 0); | ||
| BCM_GPIO_WRITE(sc, BCM_GPIO_GPPUDCLK(bank), 0); | ||
| if (sc->sc_is2711 == false) { /* BCM2835 */ |
There was a problem hiding this comment.
'sc_is2711' is boolean so no need to compare it to false.
if (!sc->sc_is2711) {} is more readable.
There was a problem hiding this comment.
Or put the true case first
if (sc->sc_is2711) {
u_int mask = BCM2711_GPIO_MASK(pin);
u_int regid = BCM2711_GPIO_REGID(pin);
uint32_t reg;
...
} else { /* BCM2835 */
...
}
emaste
reviewed
May 19, 2023
| /* Find our node. */ | ||
| gpio = ofw_bus_get_node(sc->sc_dev); | ||
| if (!OF_hasprop(gpio, "gpio-controller")) | ||
| /* Node is not a GPIO controller. */ | ||
| goto fail; | ||
| /* Guess I'm BCM2711 or not. */ | ||
| sc->sc_is2711 = ofw_bus_node_is_compatible(gpio, "brcm,bcm2711-gpio") |
There was a problem hiding this comment.
Dropping the ? true : false is clearer IMO
sc->sc_is2711 = ofw_bus_node_is_compatible(gpio, "brcm,bcm2711-gpio");
mhorne
reviewed
May 23, 2023
| #define BCM2835_GPIO_GPPUDCLK(_bank) (0x98 + _bank * 4) /* Pin Pull up clock */ | ||
|
|
||
| #define BCM2711_GPIO_GPPUD(x) (0x0e4 + (x) * sizeof(uint32_t)) /* Pin Pull up/down */ | ||
| #define BCM2711_GPIO_MASK(n) (0x3 << ((n) % 16)*2) |
There was a problem hiding this comment.
If you add:
#define BCM2711_GPIO_PIN_SHIFT(n) (((n) % 16) * 2)
Then you can use it as part of this definition, and below (see my other comment).
Comment on lines
326
to
328
| #define __LOWEST_SET_BIT(__mask) ((((__mask) - 1) & (__mask)) ^ (__mask)) | ||
| #define __SHIFTIN(__x, __mask) ((__x) * __LOWEST_SET_BIT(__mask)) | ||
| reg |= __SHIFTIN(state, mask); |
There was a problem hiding this comment.
I don't love these defines, they over-complicate what you are trying to do, which is shift the state value the appropriate number of bits for pin. You have already selected the correct register. It could just be:
reg |= state << BCM2711_GPIO_PIN_SHIFT(pin);
|
|
||
| bank = BCM_GPIO_BANK(pin); | ||
| BCM_GPIO_WRITE(sc, BCM2835_GPIO_GPPUD(0), state); | ||
| BCM_GPIO_WRITE(sc, BCM2835_GPIO_GPPUDCLK(bank), BCM_GPIO_MASK(pin)); |
There was a problem hiding this comment.
Should BCM_GPIO_MASK() be named BCM2835_GPIO_MASK() now?
mhorne
reviewed
May 25, 2023
| @@ -109,6 +115,8 @@ struct bcm_gpio_softc { | |||
| device_t sc_busdev; | |||
| struct mtx sc_mtx; | |||
| struct resource * sc_res[BCM_GPIO_IRQS + 1]; | |||
| bool sc_is2711; | |||
| u_int sc_maxpins; | |||
There was a problem hiding this comment.
sc_res and sc_bst/sc_bsh are functionally related, so we shouldn't insert the new struct members between them.
I would suggest placing the new ones right above sc_gpio_npins.
Comment on lines
313
to
323
| switch (state) { | ||
| case BCM2835_PUD_OFF: | ||
| state = BCM2711_PUD_OFF; | ||
| break; | ||
| case BCM2835_PUD_DOWN: | ||
| state = BCM2711_PUD_DOWN; | ||
| break; | ||
| case BCM2835_PUD_UP: | ||
| state = BCM2711_PUD_UP; | ||
| break; | ||
| } |
There was a problem hiding this comment.
We have an unusual style for switch statements in FreeBSD, in which the case lines should not be indented. Meaning:
switch (state) {
case BCM2835_PUD_OFF:
state = BCM2711_PUD_OFF;
break;
case BCM2835_PUD_DOWN:
...
Style(9) conformance.
mhorne
approved these changes
May 26, 2023
mhorne
pushed a commit
to mhorne/freebsd
that referenced
this pull request
May 29, 2023
Add support for GPIO internal pull up/down configuration on RPi4 family. BCM2711 SoC on 4th generation Raspberry Pi changed the way to configure its GPIO pins' internal pull up/down resistors. NetBSD already have support for this change, and port it to FreeBSD is trivial. This patch, based on the NetBSD commit adds the appropriate method for BCM2711 and now we can properly configure the GPIO pins' pull status. PR: 256372 Reviewed by: mhorne Obtained from: NetBSD bb88cfa64ad8 Pull Request: freebsd#746
|
I have merged the change, so this PR can be closed. It does not seem like I am able to do it myself. |
|
Thanks! Changes had been merged and this request should be closed. |
hardenedbsd-services
pushed a commit
to HardenedBSD/hardenedBSD
that referenced
this pull request
May 30, 2023
Add support for GPIO internal pull up/down configuration on RPi4 family. BCM2711 SoC on 4th generation Raspberry Pi changed the way to configure its GPIO pins' internal pull up/down resistors. NetBSD already have support for this change, and port it to FreeBSD is trivial. This patch, based on the NetBSD commit adds the appropriate method for BCM2711 and now we can properly configure the GPIO pins' pull status. PR: 256372 Reviewed by: mhorne Obtained from: NetBSD bb88cfa64ad8 Pull Request: freebsd#746
mhorne
pushed a commit
to mhorne/freebsd
that referenced
this pull request
Jun 9, 2023
Add support for GPIO internal pull up/down configuration on RPi4 family. BCM2711 SoC on 4th generation Raspberry Pi changed the way to configure its GPIO pins' internal pull up/down resistors. NetBSD already have support for this change, and port it to FreeBSD is trivial. This patch, based on the NetBSD commit adds the appropriate method for BCM2711 and now we can properly configure the GPIO pins' pull status. PR: 256372 Reviewed by: mhorne Obtained from: NetBSD bb88cfa64ad8 Pull Request: freebsd#746 (cherry picked from commit 9d35469)
freebsd-git
pushed a commit
that referenced
this pull request
Jun 9, 2023
Add support for GPIO internal pull up/down configuration on RPi4 family. BCM2711 SoC on 4th generation Raspberry Pi changed the way to configure its GPIO pins' internal pull up/down resistors. NetBSD already have support for this change, and port it to FreeBSD is trivial. This patch, based on the NetBSD commit adds the appropriate method for BCM2711 and now we can properly configure the GPIO pins' pull status. PR: 256372 Reviewed by: mhorne Obtained from: NetBSD bb88cfa64ad8 Pull Request: #746 (cherry picked from commit 9d35469)
bsdjhb
pushed a commit
to bsdjhb/cheribsd
that referenced
this pull request
Aug 11, 2023
Add support for GPIO internal pull up/down configuration on RPi4 family. BCM2711 SoC on 4th generation Raspberry Pi changed the way to configure its GPIO pins' internal pull up/down resistors. NetBSD already have support for this change, and port it to FreeBSD is trivial. This patch, based on the NetBSD commit adds the appropriate method for BCM2711 and now we can properly configure the GPIO pins' pull status. PR: 256372 Reviewed by: mhorne Obtained from: NetBSD bb88cfa64ad8 Pull Request: freebsd/freebsd-src#746
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
BCM2711 SoC on 4th generation Raspberry Pi changed the way to configure its GPIO pins' internal pull up/down resistors. NetBSD already have support for this change, and port it to FreeBSD is trivial.
This patch, based on the NetBSD commit adds the appropriate method for BCM2711 and now we can properly configure the GPIO pins' pull status.
PR: 256372
Reference: NetBSD/src@bb88cfa