Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions drivers/clock_control/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_MCUX_SCG_K4 clock_cont
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_MCUX_SIM clock_control_mcux_sim.c)
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_MCUX_SYSCON clock_control_mcux_syscon.c)
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_NPCM clock_control_npcm.c)
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_MSPM0 clock_control_mspm0.c)
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_NPCX clock_control_npcx.c)
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_NRF clock_control_nrf.c)
zephyr_library_sources_ifdef(CONFIG_CLOCK_CONTROL_NRF_DRIVER_CALIBRATION nrf_clock_calibration.c)
Expand Down
2 changes: 2 additions & 0 deletions drivers/clock_control/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ source "drivers/clock_control/Kconfig.mcux_syscon"

source "drivers/clock_control/Kconfig.npcm"

source "drivers/clock_control/Kconfig.mspm0"

source "drivers/clock_control/Kconfig.npcx"

source "drivers/clock_control/Kconfig.rv32m1"
Expand Down
17 changes: 17 additions & 0 deletions drivers/clock_control/Kconfig.mspm0
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# TI MSPM0 Family

# Copyright (c) 2024, Texas Instruments Inc.
# SPDX-License-Identifier: Apache-2.0

config CLOCK_CONTROL_MSPM0
bool "TI MSPM0 clock"
default y
depends on SOC_FAMILY_TI_MSPM0
help
This option enables the TI MSPM0 Clock Control Enabler
Copy link
Member

Choose a reason for hiding this comment

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

TRM calls it as Clock Module (CKM). May be Enable driver for Clock Module (CKM) found in MSPM0 family of MCUs


config CLOCK_CONTROL_MSPM0_USE_PLL
Copy link
Member

Choose a reason for hiding this comment

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

code consumes it by checking #if DT_NODE_HAS_STATUS(DT_NODELABEL(pll), okay), do we need this Kconfig entry still?

bool "TI MSPM0 Use PLL"
default n
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
default n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented spacing correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

Not fixed

help
This option enables the PLL on MSPM0 devices (if equipped)
134 changes: 134 additions & 0 deletions drivers/clock_control/clock_control_mspm0.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
/*
* Copyright (c) 2024 Texas Instruments
*
* SPDX-License-Identifier: Apache-2.0
*/

#include <zephyr/drivers/clock_control.h>
#include <zephyr/drivers/clock_control/mspm0_clock_control.h>

#include <ti/driverlib/driverlib.h>

#define ULPCLK_DIV CONCAT(DL_SYSCTL_ULPCLK_DIV_, DT_PROP(DT_NODELABEL(clkmux), uclk_div))

#if DT_NODE_HAS_STATUS(DT_NODELABEL(pll), okay)
#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??


static int clock_mspm0_on(const struct device *dev, clock_control_subsys_t sys)
{
return 0;
}

static int clock_mspm0_off(const struct device *dev, clock_control_subsys_t sys)
{
return 0;
}

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

Choose a reason for hiding this comment

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

redundant


static int clock_mspm0_get_rate(const struct device *dev, clock_control_subsys_t sys,
uint32_t *rate)
{
struct mspm0_clockSys *clockSys = (struct mspm0_clockSys *)sys;
Copy link
Member

Choose a reason for hiding this comment

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

s/clockSys/clocksys/g

Copy link

@robertinant robertinant Dec 7, 2024

Choose a reason for hiding this comment

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

Not everyone might be familiar with s(search)/<find>/<replace>/g(global)

For those that are not, the reviewer was asking to replace all occurrences of clockSys with clocksys.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rule being that Zephyr does not allow camelCase type of syntax (unless it's an external API being used, not belonging to Zephyr like a HAL etc...).

So this rule has to be applied everywhere in this PR (saw issues in serial driver as well for instance)

Copy link
Contributor

Choose a reason for hiding this comment

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

snake case only

uint8_t rateNotFound = 0;

switch (clockSys->bus) {
case MSPM0_CLOCK_BUS_LFCLK:
*rate = 32768;
break;
case MSPM0_CLOCK_BUS_ULPCLK:
*rate = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC /
DT_PROP(DT_NODELABEL(clkmux), uclk_div);
break;
case MSPM0_CLOCK_BUS_MCLK:
*rate = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;
break;
case MSPM0_CLOCK_BUS_MFPCLK:
*rate = 4000000;
break;
case MSPM0_CLOCK_BUS_MFCLK:
case MSPM0_CLOCK_BUS_CANCLK:
default:
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.

return -ENOTSUP; and no need of additional variable here.

Copy link
Member

Choose a reason for hiding this comment

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

noCamelCase please

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..

return -ENOTSUP;
} else {
return 0;
}
}

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;
}
Comment on lines +69 to +78
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


static int clock_mspm0_init(const struct device *dev)
{
/* setup clocks based on specific rates */
DL_SYSCTL_setSYSOSCFreq(DL_SYSCTL_SYSOSC_FREQ_BASE);

DL_SYSCTL_configSYSPLL((DL_SYSCTL_SYSPLLConfig *)&clock_mspm0_cfg_syspll);

DL_SYSCTL_setULPCLKDivider(ULPCLK_DIV);
DL_SYSCTL_setMCLKSource(SYSOSC, HSCLK, DL_SYSCTL_HSCLK_SOURCE_SYSPLL);

return 0;
}

static const struct clock_control_driver_api clock_mspm0_driver_api = {
.on = clock_mspm0_on,
.off = clock_mspm0_off,
.get_status = clock_mspm0_get_status,
.get_rate = clock_mspm0_get_rate,
.set_rate = clock_mspm0_set_rate,
.configure = clock_mspm0_configure};

DEVICE_DT_DEFINE(DT_NODELABEL(clkmux), &clock_mspm0_init, NULL, NULL, NULL, PRE_KERNEL_1,
Copy link
Member

Choose a reason for hiding this comment

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

More of a general question, when using DT_NODELABEL for device define, there is no real purpose of compatible in the driver file. I see the same across multiple other files as well. With this DT_DRV_COMPAT for matching the correct driver isn't the thing.

CONFIG_CLOCK_CONTROL_INIT_PRIORITY, &clock_mspm0_driver_api);

#if MSPM0_PLL_ENABLED
Copy link
Member

Choose a reason for hiding this comment

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

Disabling PLL will not boot the device at this point of time. Do we need to conditionally include the below configurations?


/* basic checks of the devicetree to follow */
#if (DT_NODE_HAS_PROP(DT_NODELABEL(pll), clk2x_div) && \
DT_NODE_HAS_PROP(DT_NODELABEL(pll), clk0_div))
#error "Only CLK2X or CLK0 can be enabled at a time on the PLL"
#endif

#define GENERATE_PLL_STRUCT() \
static const DL_SYSCTL_SYSPLLConfig clock_mspm0_cfg_syspll = { \
.inputFreq = DL_SYSCTL_SYSPLL_INPUT_FREQ_32_48_MHZ, \
.rDivClk2x = (DT_PROP_OR(DT_NODELABEL(pll), clk2x_div, 1) - 1), \
.rDivClk1 = (DT_PROP_OR(DT_NODELABEL(pll), clk1_div, 1) - 1), \
.rDivClk0 = (DT_PROP_OR(DT_NODELABEL(pll), clk0_div, 1) - 1), \
.qDiv = (DT_PROP(DT_NODELABEL(pll), q_div) - 1), \
.pDiv = CONCAT(DL_SYSCTL_SYSPLL_PDIV_, DT_PROP(DT_NODELABEL(pll), p_div)), \
.sysPLLMCLK = COND_CODE_1(DT_NODE_HAS_PROP(DT_NODELABEL(pll), clk2x_div), \
(DL_SYSCTL_SYSPLL_MCLK_CLK2X), (DL_SYSCTL_SYSPLL_MCLK_CLK0)), \
.enableCLK2x = COND_CODE_1(DT_NODE_HAS_PROP(DT_NODELABEL(pll), clk2x_div), \
(DL_SYSCTL_SYSPLL_CLK2X_ENABLE), (DL_SYSCTL_SYSPLL_CLK2X_DISABLE)), \
.enableCLK1 = COND_CODE_1(DT_NODE_HAS_PROP(DT_NODELABEL(pll), clk1_div), \
(DL_SYSCTL_SYSPLL_CLK1_ENABLE), (DL_SYSCTL_SYSPLL_CLK1_DISABLE)), \
.enableCLK0 = COND_CODE_1(DT_NODE_HAS_PROP(DT_NODELABEL(pll), clk0_div), \
(DL_SYSCTL_SYSPLL_CLK0_ENABLE), (DL_SYSCTL_SYSPLL_CLK0_DISABLE)), \
.sysPLLRef = COND_CODE_1(DT_CLOCKS_CELL(DT_NODELABEL(pll), clocks), \
(DL_SYSCTL_SYSPLL_REF_HFCLK), (DL_SYSCTL_SYSPLL_REF_SYSOSC)), \
};

GENERATE_PLL_STRUCT()

#endif /* MSPM0_PLL_ENABLED */
4 changes: 4 additions & 0 deletions dts/arm/ti/mspm0g1x0x_g3x0x/mspm0g1106.dtsi
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/* SPDX-License-Identifier: Apache-2.0 */

#include <ti/mspm0g1x0x_g3x0x/mspm0g1x0x_g3x0x.dtsi>
#include <ti/mspm0g1x0x_g3x0x/mspm0gxxx6.dtsi>
4 changes: 4 additions & 0 deletions dts/arm/ti/mspm0g1x0x_g3x0x/mspm0g1107.dtsi
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/* SPDX-License-Identifier: Apache-2.0 */

#include <ti/mspm0g1x0x_g3x0x/mspm0g1x0x_g3x0x.dtsi>
#include <ti/mspm0g1x0x_g3x0x/mspm0gxxx7.dtsi>
1 change: 1 addition & 0 deletions dts/arm/ti/mspm0g1x0x_g3x0x/mspm0g110x-pinctrl.dtsi
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#include <ti/mspm0g1x0x_g3x0x/mspm0g1x0x_g3x0x-pinctrl.dtsi>
4 changes: 4 additions & 0 deletions dts/arm/ti/mspm0g1x0x_g3x0x/mspm0g1506.dtsi
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/* SPDX-License-Identifier: Apache-2.0 */

#include <ti/mspm0g1x0x_g3x0x/mspm0g1x0x_g3x0x.dtsi>
#include <ti/mspm0g1x0x_g3x0x/mspm0gxxx6.dtsi>
4 changes: 4 additions & 0 deletions dts/arm/ti/mspm0g1x0x_g3x0x/mspm0g1507.dtsi
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/* SPDX-License-Identifier: Apache-2.0 */

#include <ti/mspm0g1x0x_g3x0x/mspm0g1x0x_g3x0x.dtsi>
#include <ti/mspm0g1x0x_g3x0x/mspm0gxxx7.dtsi>
1 change: 1 addition & 0 deletions dts/arm/ti/mspm0g1x0x_g3x0x/mspm0g150x-pinctrl.dtsi
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
#include <ti/mspm0g1x0x_g3x0x/mspm0g1x0x_g3x0x-pinctrl.dtsi>
142 changes: 142 additions & 0 deletions dts/arm/ti/mspm0g1x0x_g3x0x/mspm0g1x0x_g3x0x-pinctrl.dtsi
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
#include <dt-bindings/pinctrl/mspm0-pinctrl.h>

&pinctrl {

Copy link
Member

@parthitce parthitce Nov 27, 2024

Choose a reason for hiding this comment

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

Idea is to add all the pin functions? or the ones added in this PR? It's mixed now. Not all is added and also includes pin function for the ones which are not supported in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

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/ i2c0_scl_pa1: i2c0_scl_pa1 {
pinmux = <MSP_PINMUX(2,MSPM0_PIN_FUNCTION_3)>;
Copy link
Member

Choose a reason for hiding this comment

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

nit, spacing and the same across this file
pinmux = <MSP_PINMUX(2, MSPM0_PIN_FUNCTION_3)>;

input-enable;
drive-open-drain;
};

/omit-if-no-ref/ i2c0_sda_pa0: i2c0_sda_pa0 {
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.

pinmux = <MSP_PINMUX(21,MSPM0_PIN_FUNCTION_8)>;
input-enable;
drive-open-drain;
};

/omit-if-no-ref/ i2c1_scl_pa11: i2c1_scl_pa11 {
pinmux = <MSP_PINMUX(22,MSPM0_PIN_FUNCTION_8)>;
input-enable;
drive-open-drain;
};

/omit-if-no-ref/ i2c1_scl_pb2: i2c1_scl_pb2 {
pinmux = <MSP_PINMUX(15,MSPM0_PIN_FUNCTION_4)>;
input-enable;
drive-open-drain;
};

/omit-if-no-ref/ i2c1_sda_pb3: i2c1_sda_pb3 {
pinmux = <MSP_PINMUX(16,MSPM0_PIN_FUNCTION_4)>;
input-enable;
drive-open-drain;
};

/omit-if-no-ref/ i2c1_sda_pa16: i2c1_sda_pa16 {
pinmux = <MSP_PINMUX(38,MSPM0_PIN_FUNCTION_4)>;
input-enable;
drive-open-drain;
};

/omit-if-no-ref/ i2c1_scl_pa17: i2c1_scl_pa17 {
pinmux = <MSP_PINMUX(39,MSPM0_PIN_FUNCTION_4)>;
input-enable;
drive-open-drain;
};

/omit-if-no-ref/ i2c1_scl_pa29: i2c1_scl_pa29 {
pinmux = <MSP_PINMUX(4,MSPM0_PIN_FUNCTION_2)>;
input-enable;
drive-open-drain;
};

/omit-if-no-ref/ i2c1_sda_pa30: i2c1_sda_pa30 {
pinmux = <MSP_PINMUX(5,MSPM0_PIN_FUNCTION_2)>;
input-enable;
drive-open-drain;
};

/omit-if-no-ref/ i2c0_sda_pa28: i2c0_sda_pa28 {
pinmux = <MSP_PINMUX(3,MSPM0_PIN_FUNCTION_3)>;
input-enable;
drive-open-drain;
};

/omit-if-no-ref/ i2c0_scl_pa31: i2c0_scl_pa31 {
pinmux = <MSP_PINMUX(6,MSPM0_PIN_FUNCTION_3)>;
input-enable;
drive-open-drain;
};

/omit-if-no-ref/ uart0_tx_pa10: uart0_tx_pa10 {
pinmux = <MSP_PINMUX(21,MSPM0_PIN_FUNCTION_2)>;
drive-strength = <20>;
};

/omit-if-no-ref/ uart0_rx_pa11: uart0_rx_pa11 {
pinmux = <MSP_PINMUX(22,MSPM0_PIN_FUNCTION_2)>;
drive-strength = <20>;
input-enable;
};

/omit-if-no-ref/ uart0_rts_pa8: uart0_rts_pa8 {
pinmux = <MSP_PINMUX(19,MSPM0_PIN_FUNCTION_4)>;
};

/omit-if-no-ref/ uart0_cts_pb19: uart0_cts_pb19 {
pinmux = <MSP_PINMUX(19,MSPM0_PIN_FUNCTION_5)>;
input-enable;
};

/omit-if-no-ref/ spi1_sclk_pb9: spi1_sclk_pb9 {
pinmux = <MSP_PINMUX(26,MSPM0_PIN_FUNCTION_3)>;
};

/omit-if-no-ref/ spi1_pico_pb8: spi1_pico_pb8 {
pinmux = <MSP_PINMUX(25,MSPM0_PIN_FUNCTION_3)>;
};

/omit-if-no-ref/ spi1_poci_pb7: spi1_poci_pb7 {
pinmux = <MSP_PINMUX(24,MSPM0_PIN_FUNCTION_3)>;
input-enable;
};

/omit-if-no-ref/ spi1_cs0_pb6: spi1_cs0_pb6 {
pinmux = <MSP_PINMUX(23,MSPM0_PIN_FUNCTION_3)>;
};

/omit-if-no-ref/ spi1_cs1_pb17: spi1_cs1_pb17 {
pinmux = <MSP_PINMUX(43,MSPM0_PIN_FUNCTION_4)>;
};

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

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

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

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

Choose a reason for hiding this comment

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

Not addressed

};
Loading