Add TI J721e R5 and BeagleBone AI64 R5 initial support. And fix up current PINCTRL selection for other TI boards. #71527
Conversation
zephyrbot
added
area: Devicetree Binding
glneo
force-pushed
the
ai64-r5
branch
2 times, most recently
from
0a82a6f to
16ae360
Compare
glneo
force-pushed
the
ai64-r5
branch
2 times, most recently
from
8d0b8ce to
eb8019b
Compare
soc/ti/k3/am6x/Kconfig
Outdated
| config SOC_SERIES_AM6X | ||
| select PINCTRL |
There was a problem hiding this comment.
this is not correct, the drivers need to select it
There was a problem hiding this comment.
@nordicjm I am a bit confused here, since I think beagleplay and beagleconnect freedom are also currently doing this wrong. There is select PINCTRL in soc Kconfig:
From your comment it seems that drivers are supposed to do this. However, where should pinctrl be enabled in case the board initialization needs it ?(the antenna switch in bcf and play case: https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/beagle/beagleconnect_freedom/board_antenna.c)
There was a problem hiding this comment.
Inside Kconfig of the board if the board needs it
There was a problem hiding this comment.
We use the PINCTRL for the uart_ns16550.c driver, but that driver doesn't select it, should I go and fix that up everywhere?
There was a problem hiding this comment.
That won't work either, as some users of uart_ns16550.c need PINCTRL and others do not. We can only know at the board level if pinmux will be needed. Not sure I know a good solution.
There was a problem hiding this comment.
This would need added for every current K3 <soc_name>, and then updated for every new one added, sounds like constant churn in the serial driver kconfig. Having it selected by the symbol UART_NS16550_TI_K3 which is in Kconfig.ns16550 and already selected by all the K3 SoCs seems to have the same effect (all K3 SoCs have pinmux) without listing each and every SoC in each and every driver's kconfig.
There was a problem hiding this comment.
Except if a user disables the UART, you're forcing on an option that is not needed because your dependencies are not correct
There was a problem hiding this comment.
If the user disables SERIAL then UART_NS16550_TI_K3 will also not be enabled, and so PINCTRL would not be selected.
There was a problem hiding this comment.
BTW I just wanted to note that I do agree with what you are saying here.
If we want to do the right thing in all cases, then one must also note that PINCTRL is really only needed on boards that don't use the default pinmux settings for the SoC (all SoC pins have a default mux setting, if the board uses those then PINCTRL would also not be needed). This can only be known at the board level. But also as you note, only some drivers support PINCTRL in the first place (all should at some point, or it should be moved to a higher level like in Linux and done for drivers automatically in the default case).
So we only select PINCTRL if 3 things are true, the board's DT has a pinmux node associated with a driver, that driver uses pinmux, and that driver is enabled. If any of these or not true for all drivers used by a board, only then can PINCTRL be not selected.
I'm thinking there should be some DT parsing magic that can help with this. For all driver DT nodes that contain an enabled pinmux property we define DT_HAS_<DRIVER>_PINMUX_ENABLED. Then in Kconfig for drivers we can do this:
config <DRIVER>
depends on DT_HAS_<DRIVER>_ENABLED
select PINCTRL if DT_HAS_<DRIVER>_PINMUX_ENABLED
There was a problem hiding this comment.
PINCTRL needs to be selected if the associated code makes use of the pinctrl API, that’s all you need to take care of.
boards/beagle/beaglebone_ai64/beaglebone_ai64_j721e_main_r5f0_0.dts
Outdated
Show resolved
Hide resolved
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # Zephyr Kernel Configuration | ||
| CONFIG_XIP=n |
There was a problem hiding this comment.
Why is this not XIP? Is there some relocation happening?
There was a problem hiding this comment.
Kinda, we don't have FLASH memory to execute from, we always run out of RAM. Setting XIP causes some other issues as we do not have a hard-coded flash memory region. At some point we could consider the location Linux loads the binary to as the "flash" region, but that would need other refactoring and since we are loaded into memory that is not ROM we don't get much from XIP being set.
Add initial SoC support for the TI J721E SoC series Cortex-R5 core. TRM for J721e https://www.ti.com/lit/zip/spruil1 File: spruil1c.pdf Signed-off-by: Prashanth S <slpp95prashanth@yahoo.com> Signed-off-by: Andrew Davis <afd@ti.com>
Add initial BeagleBone AI-64 support. BeagleBone AI-64: https://www.beagleboard.org/boards/beaglebone-ai-64 Signed-off-by: Prashanth S <slpp95prashanth@yahoo.com> Signed-off-by: Andrew Davis <afd@ti.com>
The default configuration for PINCTRL should not be set with the other default configurations in .defconfig, instead select a default value as part of defining the UART driver. Signed-off-by: Andrew Davis <afd@ti.com>
|
@nordicjm @gmarull have you reviewed since latest push? Do we have a good pattern for PINCTRL? I'll be working with @Ayush1325 and all to get fixes for other Beagles. |
Drivers that need it must select it, it must not be in any defconfig files. If a board needs it for a board's .c file then it must be selected by the board |
| config SOC_SERIES | ||
| default "am6x" if SOC_SERIES_AM6X | ||
|
|
||
| config SOC | ||
| default "am6234" if SOC_AM6234_M4 || SOC_AM6234_A53 | ||
| default "am6442" if SOC_AM6442_M4 | ||
| default "j721e" if SOC_J721E_MAIN_R5F0_0 |
There was a problem hiding this comment.
This seems inconsistent with soc/ti/k3/am6x/Kconfig SOC_PART_NUMBER. Should it be leading caps, all caps or lower-case? Why would the SOC_PART_NUMBER be different than SOC?
There was a problem hiding this comment.
For SOC I found the following in the porting guide:
config SOC
default "SoC name" if SOC_<SOC_NAME>
Notice that ``SOC_NAME`` is a pure upper case version of the SoC name.
Notice that the string value must match the SoC name used in the :file:`soc.yml` file.
To me that seems to indicate all lower to match what we do in soc.yml.
For SOC_PART_NUMBER, to be honest I have no clue, I don't see it used anywhere either, I just added it based on what the other TI SoCs had done, but maybe they didn't need it either?
There was a problem hiding this comment.
SOC must be lower, SOC_PART_NUMBER does not exist, i.e. it's a custom field, it has no relation to anything and is not used by anything, it can be safely removed unless you have HAL code using it
|
@gmarull it seems the updates were made. Any changes remaining? |
This PR adds SoC support for TI J721e, specifically the R5 cores found in the MAIN domain (there is an R5 in the MCU domain we will add support for later). We also add board support for the Beaglebone AI64 which uses the J721e SoC.
TRM for J721e: https://www.ti.com/lit/zip/spruil1
BeagleBone AI-64: https://beagleboard.org/ai-64
Note: this is the continuation of the PR #59191 which went stale and was closed. Updates include HWMv2 and other small bug fixes. This is based on #71526 which is the first patch here (included here also so this series can still build).