Add iopctl driver for RT 3 digital platforms #81086
Conversation
lucien-nxp
commented
Nov 7, 2024
•
lucien-nxp
commented
- add new iopctl driver for RT 3 digital platforms
- use iopctl name as pin config IP in dts files for RT500/600
- adapt new driver for RT500/600
zephyrbot
added
area: Pinctrl
platform: NXP Drivers
zephyrbot
requested review from
butok,
danieldegrasse,
dbaluta,
decsny,
DerekSnell,
EmilioCBen,
gmarull,
manuargue,
MarkWangChinese and
yvanderv
|
The following west manifest projects have been modified in this Pull Request:
Note: This message is automatically posted and updated by the Manifest GitHub Action. |
zephyrbot
added
manifest
manifest-hal_nxp
DNM
lucien-nxp
force-pushed
the
develop/change-pinctrl-model-for-RT3-digital-platforms
branch
2 times, most recently
from
2e45a34 to
eeb7a35
Compare
|
Hi @hakehuang, |
@Lucien-Zhao one issues found in testing. which is ok in v4.0-rc2 |
drivers/pinctrl/pinctrl_iopctl.c
Outdated
| #if DT_NUM_INST_STATUS_OKAY(DT_DRV_COMPAT) > 1 | ||
| (uint32_t *)DT_REG_ADDR(DT_NODELABEL(iopctl1)), | ||
| #endif | ||
| #if DT_NUM_INST_STATUS_OKAY(DT_DRV_COMPAT) > 2 |
There was a problem hiding this comment.
why not just check if node exists? this looks fragile
There was a problem hiding this comment.
Updated. Thank you.
There was a problem hiding this comment.
We may want to add a note in the migration guide for 4.1 that this binding has been renamed, and that the Kconfig for the RT500/RT600 pin control has changed to PINCTRL_NXP_IOPCTL from PINCTRL_NXP_IOCON
There was a problem hiding this comment.
Yes, which doc should I update? Could you tell me the file path?
There was a problem hiding this comment.
https://github.com/zephyrproject-rtos/zephyr/blob/main/doc/releases/migration-guide-4.1.rst#L1
Look at the migration guide for 4.0 to get an idea of what to add- you would place this under the "Device Drivers and Devicetree" section
There was a problem hiding this comment.
Thank you. Changes have been recorded in migration-guide-4.1.
lucien-nxp
force-pushed
the
develop/change-pinctrl-model-for-RT3-digital-platforms
branch
from
eeb7a35 to
1bfbe48
Compare
Hi @hakehuang , PASS - test_pm_singlethread in 2.288 secondsTESTSUITE power_mgmt succeeded ------ TESTSUITE SUMMARY START ------ SUITE PASS - 100.00% [power_mgmt]: pass = 3, fail = 0, skip = 0, total = 3 duration = 5.717 seconds
------ TESTSUITE SUMMARY END ------ =================================================================== |
|
Details log show below: |
lucien-nxp
force-pushed
the
develop/change-pinctrl-model-for-RT3-digital-platforms
branch
from
1bfbe48 to
b04ad88
Compare
|
@lucien-nxp ok, I find I need power cycle the board after flash to make this case pass. |
| int pinctrl_configure_pins(const pinctrl_soc_pin_t *pins, uint8_t pin_cnt, uintptr_t reg) | ||
| { | ||
| for (uint8_t i = 0; i < pin_cnt; i++) { | ||
| uint32_t pin_mux = pins[i]; |
There was a problem hiding this comment.
Do we need to have a mask here as well? It seems like we should not be writing all the bits of the pinmux setting, since the upper bits are used for the pin/offset
There was a problem hiding this comment.
Yes, I have added code to use masked value to write into iopctl register. Thank you.
lucien-nxp
force-pushed
the
develop/change-pinctrl-model-for-RT3-digital-platforms
branch
2 times, most recently
from
ef48459 to
dcf5c29
Compare
doc/releases/migration-guide-4.1.rst
Outdated
| @@ -39,6 +39,26 @@ LVGL | |||
| Device Drivers and Devicetree | |||
| ***************************** | |||
|
|
|||
| * The :dtcompatible:`nxp,lpc-iocon` and `nxp,rt-iocon-pinctrl` driver won't be used | |||
There was a problem hiding this comment.
Take a look at how dtcompatible is used in other examples (IE 4.0 release notes):
zephyr/doc/releases/migration-guide-4.0.rst
Line 121 in f7ff830
You need to enclose the old (unavailable) DT bindings in double backticks, like so:
``nxp,rt-iocon-pinctrl``
There was a problem hiding this comment.
Thank you for pointing out the issue. I have updated it in migration guide, thank you.
RT3 digital platforms won't reuse lpc_iocon driver, new iopctl driver have been developed for iopctl IP. Due to RT700 have multi iopctl instances, add code to handle the index value recorded in new pinctrl header file Signed-off-by: Lucien Zhao <[email protected]>
Change pin register name from iocon to iopctl Delete pin type mask macro, there is no need to add these macros to adapt iocon IP structure. Signed-off-by: Lucien Zhao <[email protected]>
track hal_nxp 460 PR Signed-off-by: Lucien Zhao <[email protected]>
lucien-nxp
force-pushed
the
develop/change-pinctrl-model-for-RT3-digital-platforms
branch
from
dcf5c29 to
0d357bb
Compare
|
@lucien-nxp, @mmahadevan108 and I discussed internally and we think there might be value to continuing to use the existing IOCON driver, and updating the pin control data for LPC5* parts that use the IOCON driver as well. What are your thoughts on this approach? |
Yes, IOPCTL driver is only added for RT 3 digital platforms. IOCON driver will be existed and serviced for LPC platforms. |
Sure, what I'm asking is if we could continue using the IOCON driver for the RT 3 digit platforms. It seems we could, right? We would need to update pin control macros for the LPC platforms, but that should not be too difficult. The reason for this request is that it would result in one fewer pin control driver to maintain. We could store all the relevant pin data like so: It seems like with a scheme like this we would easily be able to store everything related to the pin data without creating a new driver, right? |
|
It does currently yes. However if we moved to encoding the instance in a separate set of bits, we could encode pin offset in 0-31, right? I'm not aware of a IOCON or IOPCTL IP block with more than 32 pins per instance. Some IOCON blocks encode all pins as a continuous block, but we could easily define those pins as multiple separate blocks that just happened to be contiguous, right?
Correct- I am suggesting that we update the IOCON driver to handle both the 3 pin type registers of the IOCON IP and multi-instance case of the IOPCTL on the RT700. Based on what I can tell, it should be possible, right?
So this is a deeper question. When I first encountered the IOPCTL IP on the RT500/RT600, it looked a lot like an evolution of the IOCON IP, which is why I reused the driver. I guess my view is that we should support as many IP types with the IOCON driver as possible- if we revise the IOPCTL IP again, then perhaps we need to define an IOPCTL driver for that IP revision. Do you think we would revise the IOPCTL IP without renaming it though? We renamed the IOCON IP to IOPCTL on the RT500/RT600, and the only major IP change I am aware of was the removal of the I/D/A type pins. |
I agree that we should optimize IOCON driver to cover LPC and RTxxx SOCs. For long term, it is much better to maintain 1 driver instead of 2. |
