drivers: regulator: add LDO/DCDC support for Smartbond. #65226

kasjer · 2023-11-15T12:24:29Z

This add regulator driver for Smartbond DA1469X SOC. Driver can control VDD, V14, V18, V18P, V30 rails, full voltage range supported by SOC is covered.
For VDD, V14, V18, V18P DCDC can be configured.

Special VDD_CLAMP (always on) and VDD_SLEPP are added to allow configuration of VDD in sleep modes.

Whole configuration can be done in device tree and alter changed by Zephyr regulator API.

kasjer · 2023-11-16T06:46:39Z

@gmarull separate PR as requested

gmarull · 2023-11-16T08:57:18Z

drivers/regulator/Kconfig.da1469x

+
+config REGULATOR_DA1469X_INIT_PRIORITY
+	int "Renesas DA1469x regulators driver init priority"
+	default KERNEL_INIT_PRIORITY_OBJECTS


this is a device, please use a number or KERNEL_INIT_PRIORITY_DEVICE

gmarull · 2023-11-16T08:58:53Z

drivers/regulator/regulator_da1469x.c

+static const uint32_t voltages_vdd[] = {
+	900000, 1000000, 1100000, 1200000
+};
+
+static const uint32_t voltages_vdd_sleep[] = {
+	750000, 800000, 850000, 900000,
+};
+
+static const uint32_t voltages_v14[] = {
+	1200000, 1250000, 1300000, 1350000,
+	1400000, 1450000, 1500000, 1550000
+};
+
+static const uint32_t voltages_v30[] = {
+	3000000, 3300000
+};
+
+static const uint32_t voltages_v18[] = {
+	1200000, 1800000
+};


these are all linear, so linear range can be used for these. See an example where some are linear and some are not: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/regulator/regulator_npm6001.c

V30 was non-liner till 3.45 was remove per @ioannis-karachalios request.
Voltages are selected by indices in registers since at lease one is non-linear all are treated the same with district values kept in code.
If Renesas decides yet again that V30 (or other rail) can have other values code will not require extensive change.

Linear range API can support non-contiguous ranges, e.g:

zephyr/drivers/sensor/npm1300_charger/npm1300_charger.c

Lines 131 to 132 in 063ce9c

static const struct linear_range charger_volt_ranges[] = {

LINEAR_RANGE_INIT(3500000, 50000, 0U, 3U), LINEAR_RANGE_INIT(4000000, 50000, 4U, 13U)};

changed to linear ranges

gmarull · 2023-11-16T08:59:35Z

drivers/regulator/regulator_da1469x.c

+	default:
+		break;


gmarull · 2023-11-16T08:59:50Z

drivers/regulator/regulator_da1469x.c

+	const struct regulator_da1469x_config *config = dev->config;
+
+	switch (config->rail) {
+	case VDD:


__fallthrough;

I don't think this is required in Zephyr for series of cases with no code. __fallthrough would be needed if case with some statements was continued in next case without break.

gmarull · 2023-11-16T09:00:00Z

drivers/regulator/regulator_da1469x.c

+	default:
+		break;


gmarull · 2023-11-16T09:03:06Z

drivers/regulator/regulator_da1469x.c

+	case VDD:
+		DCDC_REG_SET(VDD, CUR_LIM_MAX_HV, ((max_ua / 30000) - 1));
+		DCDC_REG_SET(VDD, CUR_LIM_MAX_LV, ((max_ua / 30000) - 1));
+		DCDC_REG_SET(VDD, CUR_LIM_MIN, ((max_ua / 30000) - 1));
+		break;
+	case V14:
+		DCDC_REG_SET(V14, CUR_LIM_MAX_HV, ((max_ua / 30000) - 1));
+		DCDC_REG_SET(V14, CUR_LIM_MAX_LV, ((max_ua / 30000) - 1));
+		DCDC_REG_SET(V14, CUR_LIM_MIN, ((max_ua / 30000) - 1));
+		break;
+	case V18:
+		DCDC_REG_SET(V18, CUR_LIM_MAX_HV, ((max_ua / 30000) - 1));
+		DCDC_REG_SET(V18, CUR_LIM_MAX_LV, ((max_ua / 30000) - 1));
+		DCDC_REG_SET(V18, CUR_LIM_MIN, ((max_ua / 30000) - 1));
+		break;
+	case V18P:
+		DCDC_REG_SET(V18P, CUR_LIM_MAX_HV, ((max_ua / 30000) - 1));
+		DCDC_REG_SET(V18P, CUR_LIM_MAX_LV, ((max_ua / 30000) - 1));
+		DCDC_REG_SET(V18P, CUR_LIM_MIN, ((max_ua / 30000) - 1));
+		break;


magic numbers here, remember current is given in a range as well

changed to linear ranges

gmarull · 2023-11-16T09:03:20Z

drivers/regulator/regulator_da1469x.c

+		DCDC_REG_SET(V18P, CUR_LIM_MIN, ((max_ua / 30000) - 1));
+		break;
+	default:
+		ret = -ENOSYS;


incorrect usage of ENOSYS, should be ENOTSUP

gmarull · 2023-11-16T09:04:17Z

drivers/regulator/regulator_da1469x.c

+	regulator_common_data_init(dev);
+
+	if (config->rail == V30) {
+		POWER_REG_SET(LDO_3V0_REF, DT_INST_PROP_OR(DT_NODELABEL(V30),


this is unsafe: what if we have >1 instance of V30? please, store the value in driver's config

There is no point is storing it drivers config. There is only one V30 rail in this soc.

yes, but driver is designed to be multi-instance, and this construct is wrong.

valued set from config

gmarull · 2023-11-16T09:04:56Z

dts/arm/renesas/smartbond/da1469x.dtsi

+				regulator-init-microvolt = <706000>;
+			};
+			vdd_sleep: VDD_SLEEP {
+				regulator-boot-on;
+				regulator-init-microvolt = <750000>;


are all these init values board specific? why are they defined in soc dts files?

They are not board specific, they can be changed for board but they are soc specific values used here to achieve lowest power consumption.

aasinclair · 2023-11-17T10:35:57Z

drivers/regulator/regulator_da1469x.c

+#define DA1469X_MODE_LDO_VBAT_ON				0x10
+#define DA1469X_MODE_V30_VBAT_CLAMP_ON				0x20
+
+#define DCDC_REG_FIELD_MSK(dcdc, field) \


Have these macros come from an SDK, or were they written specifically for this driver?
I find them rather difficult to follow.

I think this would be better approached with a description structure for each regulator containing appropriate masks and fields, e.g:
https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/regulator/regulator_pca9420.c

All macros removed, pure SDK fields used across file

aasinclair · 2023-11-17T10:37:02Z

drivers/regulator/regulator_da1469x.c

+		DCDC_REG_SET(VDD, ENABLE_HV,
+			     !!(config->flags & DA1469X_MODE_DCDC_HIGH_BATTERY_VOLTAGE_ON));
+		DCDC_REG_SET(VDD, ENABLE_LV,
+			     !!(config->flags & DA1469X_MODE_DCDC_LOW_BATTERY_VOLTAGE_ON));


These could be combined into a single write.

after rework registers modified in one write

aasinclair

Thanks for reworking this, looks great.
Just a config nit from me remaining.

aasinclair · 2023-11-22T10:22:52Z

drivers/regulator/Kconfig.da1469x

+if REGULATOR_DA1469X
+
+config REGULATOR_DA1469X_INIT_PRIORITY
+	int "Renesas DA1469x regulators driver init priority"
+	default KERNEL_INIT_PRIORITY_DEVICE
+	help
+	  Init priority for the Renesas DA1469x regulators driver.
+
+endif # REGULATOR_DA1469X


Single configs should use depends

Suggested change

if REGULATOR_DA1469X

config REGULATOR_DA1469X_INIT_PRIORITY

int "Renesas DA1469x regulators driver init priority"

default KERNEL_INIT_PRIORITY_DEVICE

help

Init priority for the Renesas DA1469x regulators driver.

endif # REGULATOR_DA1469X

config REGULATOR_DA1469X_INIT_PRIORITY

int "Renesas DA1469x regulators driver init priority"

default KERNEL_INIT_PRIORITY_DEVICE

depends on REGULATOR_DA1469X

help

Init priority for the Renesas DA1469x regulators driver.

@aasinclair thanks for a hint, change applied as suggested

This add regulator driver for Smartbond DA1469X SOC. Driver can control VDD, V14, V18, V18P, V30 rails, full voltage range supported by SOC is covered. For VDD, V14, V18, V18P DCDC can be configured. Special VDD_CLAMP (always on) and VDD_SLEPP are added to allow configuration of VDD in sleep modes. Signed-off-by: Jerzy Kasenberg <jerzy.kasenberg@codecoup.pl>

kasjer requested review from galak and gmarull as code owners November 15, 2023 12:24

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding platform: Renesas SmartBond Renesas Electronics Corporation, SmartBond area: Regulators labels Nov 15, 2023

zephyrbot requested review from aasinclair, andrzej-kaczmarek and danieldegrasse November 15, 2023 12:25

zephyrbot assigned aasinclair Nov 15, 2023

kasjer force-pushed the kasjer/da1469x-regulators branch from 09d3ae0 to 23c607b Compare November 15, 2023 12:33

kasjer mentioned this pull request Nov 15, 2023

Renesas SmartBond(tm) power management #61857

Merged

gmarull requested changes Nov 16, 2023

View reviewed changes

aasinclair requested changes Nov 17, 2023

View reviewed changes

kasjer force-pushed the kasjer/da1469x-regulators branch 2 times, most recently from 2b9e663 to 161e2ac Compare November 21, 2023 09:28

aasinclair requested changes Nov 22, 2023

View reviewed changes

kasjer force-pushed the kasjer/da1469x-regulators branch from 161e2ac to 1bd1815 Compare November 22, 2023 10:41

aasinclair approved these changes Nov 22, 2023

View reviewed changes

aasinclair requested a review from gmarull November 22, 2023 11:10

gmarull approved these changes Nov 22, 2023

View reviewed changes

andrzej-kaczmarek approved these changes Nov 22, 2023

View reviewed changes

fabiobaltieri merged commit 46bbe05 into zephyrproject-rtos:main Nov 22, 2023
18 checks passed

kasjer deleted the kasjer/da1469x-regulators branch November 24, 2023 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: regulator: add LDO/DCDC support for Smartbond. #65226

drivers: regulator: add LDO/DCDC support for Smartbond. #65226

kasjer commented Nov 15, 2023

kasjer commented Nov 16, 2023

gmarull Nov 16, 2023

kasjer Nov 21, 2023

gmarull Nov 16, 2023

kasjer Nov 16, 2023

aasinclair Nov 16, 2023

kasjer Nov 21, 2023

gmarull Nov 16, 2023

kasjer Nov 21, 2023

gmarull Nov 16, 2023

kasjer Nov 16, 2023

gmarull Nov 16, 2023

kasjer Nov 21, 2023

gmarull Nov 16, 2023

kasjer Nov 21, 2023

gmarull Nov 16, 2023

kasjer Nov 21, 2023

gmarull Nov 16, 2023

kasjer Nov 20, 2023

gmarull Nov 20, 2023

kasjer Nov 21, 2023

gmarull Nov 16, 2023

kasjer Nov 20, 2023

aasinclair Nov 17, 2023

kasjer Nov 21, 2023

aasinclair Nov 17, 2023

kasjer Nov 21, 2023

aasinclair left a comment

aasinclair Nov 22, 2023

kasjer Nov 22, 2023

	static const struct linear_range charger_volt_ranges[] = {
	LINEAR_RANGE_INIT(3500000, 50000, 0U, 3U), LINEAR_RANGE_INIT(4000000, 50000, 4U, 13U)};

drivers: regulator: add LDO/DCDC support for Smartbond. #65226

drivers: regulator: add LDO/DCDC support for Smartbond. #65226

Conversation

kasjer commented Nov 15, 2023

kasjer commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aasinclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment