-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Refactor nordic socs/drivers to remove GPD and decouple pin retention. #90754
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.
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
Refactor nordic socs/drivers to remove GPD and decouple pin retention. #90754
Conversation
afa9528
to
87e9f3a
Compare
f737ae0
to
8654ac9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't look at SWEXT changes, but the gpio port/pad power domain handling looks good.
Will this trigger more power domain requests/releases?
95015ed
to
1de2f64
Compare
3acab47
to
9163316
Compare
126e8cb
to
3e4dca4
Compare
e2079fd
to
fb766d7
Compare
Rebased to solve merge conflict |
ping @anangl @nika-nordic |
fb766d7
to
af1ee21
Compare
Rebased to include cross domain changes for SPI and UARTE |
ping @anangl |
Introduce the NRFS GDPWR (Global Domain Power Request) device driver and devicetree binding. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
af1ee21
to
70d5d49
Compare
controller which manages the the pad group, | ||
named pad-group. The pad group's nodelabel is | ||
named gpio_pad_group<GPIO number>. | ||
controller which manages the pad group, named | ||
pad-group. The pad group's nodelabel is named | ||
gpio_pad_group<GPIO number>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This correction should be done directly in the commit that introduces the binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
dts/vendor/nordic/nrf54h20.dtsi
Outdated
open-loop-startup-time-us = <200>; /* To be measured */ | ||
clocks = <&hfxo>, <&lfxo>; | ||
clock-names = "hfxo", "lfxo"; | ||
status = "disabled"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it would be probably better to have this right after compatible
, like in most of the other nodes here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
soc/nordic/common/pinctrl_soc.h
Outdated
* | ||
* @param pincfg Pin configuration bit field. | ||
*/ | ||
#define NRF_GET_PORT_PIN(pincfg) (NRF_GET_PORT(pincfg) % 32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define NRF_GET_PORT_PIN(pincfg) (NRF_GET_PORT(pincfg) % 32) | |
#define NRF_GET_PORT_PIN(pincfg) (NRF_GET_PIN(pincfg) & 0x1F) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
( \ | ||
IS_EQ( \ | ||
DT_DEP_ORD(DT_PHANDLE(node_id, power_domains)), \ | ||
DT_DEP_ORD(DT_NODELABEL(gdpwr_fast_active_1)) \ | ||
) \ | ||
), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
( \ | |
IS_EQ( \ | |
DT_DEP_ORD(DT_PHANDLE(node_id, power_domains)), \ | |
DT_DEP_ORD(DT_NODELABEL(gdpwr_fast_active_1)) \ | |
) \ | |
), \ | |
(DT_SAME_NODE(DT_PHANDLE(node_id, power_domains), \ | |
DT_NODELABEL(gdpwr_fast_active_1))), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work, DT_SAME_NODE uses ==
, it does not evaluate to 0 or 1 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
zephyr/include/zephyr/devicetree.h
Lines 692 to 693 in 00e217d
#define DT_SAME_NODE(node_id1, node_id2) \ | |
IS_EQ(DT_DEP_ORD(node_id1), DT_DEP_ORD(node_id2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, this was fixed after I made the commit :D ba48d83
compatible = "nordic,nrf-spim"; | ||
status = "okay"; | ||
def-char = <0x00>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? It's a SPIM, not SPIS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some driver needs def-char
to be set, this was an attempt at trying to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue was with SPIS_NODE macro which checked if an instance was FAST to determine if the nodelabel was spix or spisx, this can't be done since we need to know the nodelabel first, to be able to check if its fast or not, changed to just test if the nodelabel exists:
#define SPIS_NODE(idx) \
COND_CODE_1(DT_NODE_EXISTS(DT_NODELABEL(spis##idx)), (spis##idx), (spi##idx))
dts/vendor/nordic/nrf54h20.dtsi
Outdated
swext: swext { | ||
compatible = "nordic,nrfs-swext"; | ||
max-current-ua = <10000>; | ||
current-limit-ua = <10000>; | ||
zephyr,pm-device-runtime-auto; | ||
#power-domain-cells = <0>; | ||
status = "disabled"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this added intentionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, It should have been removed as I dropped the swext commit :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
drivers/spi/spi_nrfx_spim.c
Outdated
CONFIG_SPI_INIT_PRIORITY < CONFIG_CLOCK_CONTROL_NRF_HSFLL_GLOBAL_INIT_PRIORITY | ||
#define SPIM_INIT_PRIORITY(idx) \ | ||
COND_CODE_1(SPIM_REQUESTS_CLOCK(SPIM(idx)), \ | ||
COND_CODE_1(INSTANCE_IS_FAST_PD(_, /*empty*/, idx, _), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COND_CODE_1(INSTANCE_IS_FAST_PD(_, /*empty*/, idx, _), \ | |
COND_CODE_1(INSTANCE_IS_FAST(_, /*empty*/, idx, _), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Introduce the NRF GPIO Pad Group device driver and binding. The pad group device represents the GPIO pads (pins), contrary to a GPIO controller, which is one of the many devices which can be muxed to pads in the pad group. The pad group belong to a power domain, which is not neccesarily the same power domain as devices being muxed to the pads, like GPIO or UART. If no ACTIVE device is using any of the pads in the pad group, the pad groups power domain may be SUSPENDED. Before the pad groups power domain is SUSPENDED, pad config retention must be enabled to prevent the pads from loosing their state. That's what this device driver manages. Once retained, the pad configs and outputs are locked, even when their power domain is SUSPENDED. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
Transition nrf54h away from the soc specific gpd (global power domain) driver which mixed power domains, pinctrl and gpio pin retention into a non scalable solution, forcing soc specific logic to bleed into nrf drivers. The new solution uses zephyrs PM_DEVICE based power domains to properly model the hardware layout of device and pin power domains, and moves pin retention logic out of drivers into pinctrl and gpio, which are the components which manage pins (pads). Signed-off-by: Bjarki Arge Andreasen <[email protected]>
e47f3a6
to
9d6df0d
Compare
Remove the deprecated GPD (Global Power Domain) driver. Signed-off-by: Bjarki Arge Andreasen <[email protected]>
9d6df0d
to
e4f5b0d
Compare
|
ping @masz-nordic |
Replace highly coupled use of GPD (gpd.h) from device drivers, with power domain device drivers using zephyr's PM_DEVICE_POWER_DOMAIN infrastructure.
The gpd.h currently is used for two things:
Nordic device drivers currently have to do this manually, conditionally, which is adding immense complexity to them, only relevant for certain SoCs, and which is in theory duplicate code. In reality, device drivers do their best, but since gpd is not well understood, device drivers implement it slightly differently, in some cases working by design, some cases working by accident, and in some cases working, but breaking something else.
No more!
Both pin retention and power domains are actually designed in a way that is entirely logical, and can be expressed using power domains and the devicetree. So, using pinctrl and power domains, we can entirely decouple devices from this complexity :)
What does the refactor do?
power-domains
property from breaking user's applications if they "dare" to use zephyrs power domain framework. See first commit.