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.

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

TI MSPM0 Support #79673

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

TI MSPM0 Support #79673

wants to merge 9 commits into from

Conversation

JFMSP
Copy link

@JFMSP JFMSP commented Oct 10, 2024

This is a continuation and re-opening of the previous PR #68503. All comments on that PR should be addressed at this point.

I apologize for the delay in getting this PR out, but I am able to respond to comments and would like your help to get this merged.

-JFMSP

@zephyrbot zephyrbot added area: Clock Control platform: TI SimpleLink Texas Instruments SimpleLink MCU area: Pinctrl area: GPIO area: UART Universal Asynchronous Receiver-Transmitter labels Oct 10, 2024
@JFMSP JFMSP marked this pull request as draft October 10, 2024 18:45
@zephyrbot
Copy link
Collaborator

zephyrbot commented Oct 10, 2024

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
hal_ti zephyrproject-rtos/hal_ti@2e7b95a zephyrproject-rtos/hal_ti#51 zephyrproject-rtos/hal_ti#51/files

Additional metadata changed:

Name URL Submodules West cmds module.yml
hal_ti

DNM label due to: 1 project with PR revision and 1 project with metadata changes

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_ti DNM This PR should not be merged (Do Not Merge) labels Oct 10, 2024
Copy link

Hello @JFMSP, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@msp-ti msp-ti force-pushed the mspm0_upstream branch 2 times, most recently from a2b7ff1 to ca5f253 Compare October 11, 2024 19:50
@JFMSP JFMSP marked this pull request as ready for review October 11, 2024 19:51
@vaishnavachath vaishnavachath self-requested a review October 11, 2024 20:05
@vaishnavachath vaishnavachath dismissed their stale review October 11, 2024 20:05

Changes addressed

@msp-ti msp-ti force-pushed the mspm0_upstream branch 2 times, most recently from fef4852 to 7c960e4 Compare October 11, 2024 21:16
bias-high-impedance;
};

/omit-if-no-ref/ uart0_tx_pa10: uart_tx_pa10 {

Choose a reason for hiding this comment

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

uart0_tx_pa10 insread of uart_tx_pa10?


/omit-if-no-ref/ i2c0_sda_pa0_pull_up: i2c0_sda_pa0_pull_up {
pinmux = <MSP_PINMUX(1,MSPM0_PIN_FUNCTION_3)>;
input-enable;
Copy link
Member

Choose a reason for hiding this comment

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

bias-pull-up; is missing

drive-open-drain;
};

/omit-if-no-ref/ i2c1_sda_pa10: i2c1_sda_pa10 {
Copy link
Member

Choose a reason for hiding this comment

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

Am not sure why _pull_down/up is missing here. Is it because the pin is High-Drive? Otherwise we don't need to keep a separate suffix for pull_up for all the I2C.

@Ayush1325
Copy link
Member

Is there a reson why mspm0l1105 is missing? Just wondering if it Zephyr was not usable with 32k of ram or if it just wasn't a priority.

@Ayush1325
Copy link
Member

@JFMSP any updates.

@JFMSP
Copy link
Author

JFMSP commented Jan 16, 2025

Is there a reson why mspm0l1105 is missing? Just wondering if it Zephyr was not usable with 32k of ram or if it just wasn't a priority.

Hey Ayush, MSPM0L1105 has 32kB of Flash. It has 4kB of RAM

@Ayush1325
Copy link
Member

Ayush1325 commented Jan 17, 2025

Is there a reson why mspm0l1105 is missing? Just wondering if it Zephyr was not usable with 32k of ram or if it just wasn't a priority.

Hey Ayush, MSPM0L1105 has 32kB of Flash. It has 4kB of RAM

Sorry for the misstype. It seems it really is because of RAM, at least from discord conversation. It would be nice if there was some way to have Zephyr on it. Upcoming Pocketbeagle2 has an mspm0l1105.

Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Apply comments to every commit in this PR, they might have only been raised on some or one instance of the error

Comment on lines 3 to 19
if(CONFIG_SOC_MSPM0G1106)
zephyr_compile_definitions(-D__MSPM0G1106__)
endif()
if(CONFIG_SOC_MSPM0G1107)
zephyr_compile_definitions(-D__MSPM0G1107__)
endif()
if(CONFIG_SOC_MSPM0G1506)
zephyr_compile_definitions(-D__MSPM0G1506__)
endif()
if(CONFIG_SOC_MSPM0G1507)
zephyr_compile_definitions(-D__MSPM0G1507__)
endif()
if(CONFIG_SOC_MSPM0G3106)
zephyr_compile_definitions(-D__MSPM0G3106__)
endif()
if(CONFIG_SOC_MSPM0G3107)
zephyr_compile_definitions(-D__MSPM0G3107__)
endif()
if(CONFIG_SOC_MSPM0G3506)
zephyr_compile_definitions(-D__MSPM0G3506__)
endif()
if(CONFIG_SOC_MSPM0G3507)
zephyr_compile_definitions(-D__MSPM0G3507__)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if(CONFIG_SOC_MSPM0G1106)
zephyr_compile_definitions(-D__MSPM0G1106__)
endif()
if(CONFIG_SOC_MSPM0G1107)
zephyr_compile_definitions(-D__MSPM0G1107__)
endif()
if(CONFIG_SOC_MSPM0G1506)
zephyr_compile_definitions(-D__MSPM0G1506__)
endif()
if(CONFIG_SOC_MSPM0G1507)
zephyr_compile_definitions(-D__MSPM0G1507__)
endif()
if(CONFIG_SOC_MSPM0G3106)
zephyr_compile_definitions(-D__MSPM0G3106__)
endif()
if(CONFIG_SOC_MSPM0G3107)
zephyr_compile_definitions(-D__MSPM0G3107__)
endif()
if(CONFIG_SOC_MSPM0G3506)
zephyr_compile_definitions(-D__MSPM0G3506__)
endif()
if(CONFIG_SOC_MSPM0G3507)
zephyr_compile_definitions(-D__MSPM0G3507__)
endif()
if(CONFIG_SOC_MSPM0G1106)
zephyr_compile_definitions(-D__MSPM0G1106__)
elseif(CONFIG_SOC_MSPM0G1107)
zephyr_compile_definitions(-D__MSPM0G1107__)
elseif(CONFIG_SOC_MSPM0G1506)
...

Copy link
Member

Choose a reason for hiding this comment

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

looks like a requirement for HAL? If so please, push this into modules/hal...

return 0;
}

SYS_INIT(ti_mspm0g_init, PRE_KERNEL_1, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

use soc hooks

extern "C" {
#endif

/* clang-format off */
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is clang format disabled?

gpio-controller;
#gpio-cells = <2>;
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change


struct uart_mspm0_data {
/* UART clock structure */
DL_UART_Main_ClockConfig UART_ClockConfig;
Copy link
Collaborator

Choose a reason for hiding this comment

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

snake case

Comment on lines 112 to 115
#ifdef CONFIG_UART_INTERRUPT_DRIVEN

#define UART_MSPM0_TX_INTERRUPTS (DL_UART_MAIN_INTERRUPT_TX | DL_UART_MAIN_INTERRUPT_EOT_DONE)
#define UART_MSPM0_RX_INTERRUPTS (DL_UART_MAIN_INTERRUPT_RX)
Copy link
Collaborator

Choose a reason for hiding this comment

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

defines to top of file

:align: center
:alt: MSPM0G3507 LaunchPad development board

Zephyr uses the ``lp_mspm0g3507`` board configuration for building
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Zephyr uses the ``lp_mspm0g3507`` board configuration for building
Zephyr uses the ``lp_mspm0g3507`` board for building

Supported Features
==================

The MSPM0G3507 LaunchPad development board configuration supports the following hardware features:
Copy link
Collaborator

Choose a reason for hiding this comment

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

not fixed??

The Zephyr ``lp_mspm0g3507`` board supports the...


leds {
compatible = "gpio-leds";
led0: led_2_b {
Copy link
Collaborator

Choose a reason for hiding this comment

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

inconsistent spacing

@JFMSP
Copy link
Author

JFMSP commented Jan 21, 2025

#79673 (comment)

I don't know why I can't reply in the conversation itself, but I'm assuming @nordicjm that by "fixed" you mean following the format lp_mspm0g3507/mspm0g3507 of board/silicon like that.

After talking with some other members at TI and @parthitce, TI only plans to support the mspm0g3507 silicon with the LP_MSPM0G3507 board, and so there should be a direct 1:1 mapping of board to silicon, as other devices would not share that board name. We don't tend to support new silicon parts on existing boards, as we'd rename them.

I know this changes our flexibility story currently, but I don't see it being an issue with our roadmap.

JFMSP added 8 commits January 21, 2025 17:34
Added initial SOC support for TI MSPM0 family

Signed-off-by: Jackson Farley <[email protected]>
added TI MSPM0 device support to the devicetree

Signed-off-by: Jackson Farley <[email protected]>
added pinctrl data for TI MSPM0 Family

Signed-off-by: Jackson Farley <[email protected]>
added clock_control support for TI MSPM0 Family

Signed-off-by: Jackson Farley <[email protected]>
added pinctrl driver support for MSPM0 Family

Signed-off-by: Jackson Farley <[email protected]>
Added GPIO driver support for TI MSPM0 family

Signed-off-by: Jackson Farley <[email protected]>
added the Serial (uart) driver for MSPM0 family

Signed-off-by: Jackson Farley <[email protected]>
added Texas Instruments LP_MSPM0G3507 launchpad

Signed-off-by: Jackson Farley <[email protected]>
Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

You've re-requested my review, so I go and look through to find basically none of the parts I left have been addressed? Fix them ?

};

soc {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

default: 1
description: |
Divider for MCLK (system clock) only when source with SYSOSC with a
frequency value of 4 MHz. Disabled otherwise. Valid values are 1-16.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not addressed

required: true
type: int
description: |
default frequency in Hz for clock output
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
default frequency in Hz for clock output
Default frequency in Hz for clock output

#include <dt-bindings/pinctrl/mspm0-pinctrl.h>

&pinctrl {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not addressed

/omit-if-no-ref/ adc1_pb19: adc1_pb19 {
pinmux = <MSP_PINMUX(45,MSPM0_PIN_FUNCTION_ANALOG)>;
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not addressed

#include <dt-bindings/pinctrl/mspm0-pinctrl.h>

&pinctrl {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

stopped reviewing after observing some major flaws, please, fix them (also check some other drivers as a reference) and re-request review.

Comment on lines +156 to +167
This section shows how to debug the MSPM0G3507 LaunchPad board using `CCS IDE`_. More information
on debugging using CCS can be found in `CCS User's Guide`_.

In general, the steps for debugging in CCS are:

1. Open CCS
2. Go to :menuselection:`Window --> Show View --> Target Configruation`
3. Import target confguration by right clicking User Defined, selecting Import target configuration and pointing to the lp_mspm0g3507/support/MSPM0G3507.ccxml
4. Launch target configuration by right clicking the new MSPM0G3507.ccxml file and clicking Launch target configuration
5. Plug in the device and connect to it by going to :menuselection:`Run --> Connect Target`
6. Go to :menuselection:`Run --> Load --> Load Symbols and load in the zephyr.elf file loaded`
7. Use CCS to debug
Copy link
Member

Choose a reason for hiding this comment

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

why should we add vendor specific IDE instructions here? doesn't west debug just work?

Comment on lines +63 to +68
clock-frequency = <DT_FREQ_M(80)>;
};

&clkmux {
clock-source = <&pll>;
clock-frequency = <DT_FREQ_M(80)>;
Copy link
Member

Choose a reason for hiding this comment

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

duplicate frequencies here?

Comment on lines +15 to +16
# Enable Clock Control
CONFIG_CLOCK_CONTROL=y
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't drivers select this?

@@ -0,0 +1,16 @@
# SPDX-License-Identifier: Apache-2.0

CONFIG_CORTEX_M_SYSTICK=y
Copy link
Member

Choose a reason for hiding this comment

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

why is this needed? isn't DT enough?

#define MSPM0_PLL_ENABLED 1
#endif

static const DL_SYSCTL_SYSPLLConfig clock_mspm0_cfg_syspll;
Copy link
Member

Choose a reason for hiding this comment

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

static const, uninitialized??

rateNotFound = 1;
break;
}
if (rateNotFound == 1) {
Copy link
Member

Choose a reason for hiding this comment

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

please, use booleans..

Comment on lines +30 to +34
static enum clock_control_status clock_mspm0_get_status(const struct device *dev,
clock_control_subsys_t sys)
{
return CLOCK_CONTROL_STATUS_UNKNOWN;
}
Copy link
Member

Choose a reason for hiding this comment

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

redundant

Comment on lines +69 to +78
static int clock_mspm0_set_rate(const struct device *dev, clock_control_subsys_t sys,
clock_control_subsys_rate_t rate)
{
return -ENOTSUP;
}

static int clock_mspm0_configure(const struct device *dev, clock_control_subsys_t sys, void *data)
{
return -ENOTSUP;
}
Copy link
Member

Choose a reason for hiding this comment

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

wrong, pls check API

#include <zephyr/drivers/gpio.h>
#include <zephyr/drivers/gpio/gpio_utils.h>
#include <zephyr/irq.h>
#include <soc.h>
Copy link
Member

Choose a reason for hiding this comment

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

no soc.h please, soc.h is only meant for cmsis

IOMUX_PINCM5, IOMUX_PINCM6,
};
#else
#throw "series lookup table not supported"
Copy link
Member

Choose a reason for hiding this comment

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

what's "throw"

Comment on lines +3 to +19
if(CONFIG_SOC_MSPM0G1106)
zephyr_compile_definitions(-D__MSPM0G1106__)
elseif(CONFIG_SOC_MSPM0G1107)
zephyr_compile_definitions(-D__MSPM0G1107__)
elseif(CONFIG_SOC_MSPM0G1506)
zephyr_compile_definitions(-D__MSPM0G1506__)
elseif(CONFIG_SOC_MSPM0G1507)
zephyr_compile_definitions(-D__MSPM0G1507__)
elseif(CONFIG_SOC_MSPM0G3106)
zephyr_compile_definitions(-D__MSPM0G3106__)
elseif(CONFIG_SOC_MSPM0G3107)
zephyr_compile_definitions(-D__MSPM0G3107__)
elseif(CONFIG_SOC_MSPM0G3506)
zephyr_compile_definitions(-D__MSPM0G3506__)
elseif(CONFIG_SOC_MSPM0G3507)
zephyr_compile_definitions(-D__MSPM0G3507__)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

lookd like a HAL requirement, move to glue code.

Comment on lines +10 to +35
#define SYSCONFIG_WEAK __attribute__((weak))

#include <ti/devices/msp/msp.h>
#include <ti/driverlib/driverlib.h>
#include <ti/driverlib/m0p/dl_core.h>

#ifdef __cplusplus
extern "C" {
#endif

/*
* Per TRM Section 2.2.7 Peripheral Power Enable Control:
*
* After setting the ENABLE | KEY bits in the PWREN Register to enable a
* peripheral, wait at least 4 ULPCLK clock cycles before accessing the rest of
* the peripheral's memory-mapped registers. The 4 cycles allow for the bus
* isolation signals at the peripheral's bus interface to update.
*
* ULPCLK will either be equivalent or half of the main MCLK and CPUCLK,
* yielding the delay time of 8 cycles
*/
#define POWER_STARTUP_DELAY (8)

#ifdef __cplusplus
}
#endif
Copy link
Member

Choose a reason for hiding this comment

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

soc.h is purely for CMSIS glueing, this looks wrong

@fabiobaltieri fabiobaltieri added DNM (manifest) This PR should not be merged (controlled by action-manifest) and removed DNM This PR should not be merged (Do Not Merge) labels Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Clock Control area: Comparator area: GPIO area: Pinctrl area: UART Universal Asynchronous Receiver-Transmitter DNM (manifest) This PR should not be merged (controlled by action-manifest) manifest manifest-hal_ti platform: TI SimpleLink Texas Instruments SimpleLink MCU
Projects
None yet
Development

Successfully merging this pull request may close these issues.