In NCS, unable to set high drive mode on PWM LEDs anymore from v2.2.0 onwards

[dtp]

Hi, I am working with PWM LEDs and have pinpointed a difference in behaviour in the SDK upgrade between NCS v2.1.0 and v2.2.0 with regard to the pin output drive for PWM devices. Specifically, while in v2.1.0 I was able to set the drive to NRF_DRIVE_H0H1 in the pinctrl dtsi and correctly measure the difference in current (and brightness), in v2.2.0 I don't see any difference between the standard and high drive.

If I use GPIO LEDs, in both SDK versions I can use the flag NRF_GPIO_DRIVE_H0 in the source code and can observe the difference compared to the default standard drive (works as expected, no issue there).

My device is a custom board based on nRF52840 and

The issue is still present in v2.4.1.

Any idea on what the issue is? 

My <CUSTOM_BOARD>.dts file is setup this way:

/ {
	leds_pwm: leds_pwm0 {
		compatible = "pwm-leds";
		green_led_pwm: pwm_led_led201 {
			pwms = <&pwm0 0 PWM_MSEC(5) PWM_POLARITY_INVERTED>;
			label = "Green PWM LED";
		};
		yellow_led_pwm: pwm_led_led200 {
			pwms = <&pwm0 1 PWM_MSEC(5) PWM_POLARITY_INVERTED>;
			label = "Yellow PWM LED";
		};
	};
};

&pwm0 {
	status = "okay";
	pinctrl-0 = <&pwm0_default>;
	pinctrl-1 = <&pwm0_sleep>;
	pinctrl-names = "default", "sleep";
};

And my <CUSTOM_BOARD>-pinctrl.dtsi file:

&pinctrl {
	pwm0_default: pwm0_default {
		group1 {
			psels = <NRF_PSEL(PWM_OUT0, 0, 16)>,
					<NRF_PSEL(PWM_OUT1, 0, 24)>;
			nordic,drive-mode = <NRF_DRIVE_H0H1>;
			nordic,invert;
		};
	};

	pwm0_sleep: pwm0_sleep {
		group1 {
			psels = <NRF_PSEL(PWM_OUT0, 0, 16)>,
					<NRF_PSEL(PWM_OUT1, 0, 24)>;
			low-power-enable;
		};
	};
};

[Øyvind]

Hello, 

I'm currently looking into you question. Hope to have an answer by end of today or tomorrow. 

At first glance it does look like an issue with the pin controller.

Kind regards,
Øyvind

[Øyvind]

Hello, 

Our team have done a quick test and are not able to reproduce. Can you provide more information for us to test? E.g. your project?

Thanks.

Kind regards,
Øyvind

[dtp]

Hi Oyvind, thanks for the help. That's interesting. I am running a modified blinky that can be run with either PWM or GPIO LEDs. Here's the main.c:

/*
 * Copyright (c) 2016 Intel Corporation
 *
 * SPDX-License-Identifier: Apache-2.0
 */

#include <soc.h>
#include <stdio.h>
#include <zephyr/device.h>
#include <zephyr/drivers/gpio.h>
#include <zephyr/drivers/led.h>
#include <zephyr/drivers/pwm.h>
#include <zephyr/dt-bindings/gpio/nordic-nrf-gpio.h>
#include <zephyr/init.h>
#include <zephyr/kernel.h>
#include <zephyr/logging/log.h>
#include <zephyr/pm/device.h>
#include <zephyr/pm/pm.h>
#include <zephyr/pm/policy.h>

LOG_MODULE_REGISTER(blinky, LOG_LEVEL_INF);

#define SLEEP_TIME_MS 1000
#define DEEP_SLEEP_TIME_MS 10000

/* Enable to use GPIO LEDs rather than PWM LEDs */
// #define USE_GPIO_LEDS

enum led_colour {
	LED_GREEN,
	LED_AMBER,
};

#ifdef USE_GPIO_LEDS
static const struct gpio_dt_spec green_gpio_led = GPIO_DT_SPEC_GET(DT_NODELABEL(green_led), gpios);
static const struct gpio_dt_spec amber_gpio_led = GPIO_DT_SPEC_GET(DT_NODELABEL(yellow_led), gpios);
#else

#define GREEN_LED_PWM_PCT 50
#define AMBER_LED_PWM_PCT 30

static const struct pwm_dt_spec green_pwm_led = PWM_DT_SPEC_GET(DT_NODELABEL(green_led_pwm));
static const struct pwm_dt_spec amber_pwm_led = PWM_DT_SPEC_GET(DT_NODELABEL(yellow_led_pwm));
static const struct device *pwm_leds_dev = DEVICE_DT_GET(DT_PARENT(DT_NODELABEL(green_led_pwm)));
#endif /* USE_GPIO_LEDS */

/* Prevent deep sleep (system off) from being entered on long timeouts
 * or `K_FOREVER` due to the default residency policy.
 *
 * This has to be done before anything tries to sleep, which means
 * before the threading system starts up between PRE_KERNEL_2 and
 * POST_KERNEL.  Do it at the start of PRE_KERNEL_2.
 */
static int disable_ds_1(const struct device *dev)
{
	ARG_UNUSED(dev);

	pm_policy_state_lock_get(PM_STATE_SOFT_OFF, PM_ALL_SUBSTATES);
	return 0;
}

SYS_INIT(disable_ds_1, PRE_KERNEL_2, 0);

static void enter_deep_sleep(void)
{
	/* Above we disabled entry to deep sleep based on duration of
	 * controlled delay.  Here we need to override that, then
	 * force entry to deep sleep on any delay.
	 */
	pm_state_force(0u, &(struct pm_state_info){ PM_STATE_SOFT_OFF, 0, 0 });

	/* Now we need to go sleep. This will let the idle thread runs and
	 * the pm subsystem will use the forced state. To confirm that the
	 * forced state is used, lets set the same timeout used previously.
	 */
	k_msleep(DEEP_SLEEP_TIME_MS);
}

void led_on_turn(enum led_colour colour)
{
	switch (colour) {
	case LED_GREEN:
#ifdef USE_GPIO_LEDS
		gpio_pin_set_dt(&green_gpio_led, true);
#else
		led_set_brightness(pwm_leds_dev, green_pwm_led.channel, GREEN_LED_PWM_PCT);
#endif /* USE_GPIO_LEDS */
		break;
	case LED_AMBER:
#ifdef USE_GPIO_LEDS
		gpio_pin_set_dt(&amber_gpio_led, true);
#else
		led_set_brightness(pwm_leds_dev, amber_pwm_led.channel, AMBER_LED_PWM_PCT);
#endif /* USE_GPIO_LEDS */
		break;
	}
}

void led_off_turn(enum led_colour colour)
{
	switch (colour) {
	case LED_GREEN:
#ifdef USE_GPIO_LEDS
		gpio_pin_set_dt(&green_gpio_led, false);
#else
		led_off(pwm_leds_dev, green_pwm_led.channel);
#endif /* USE_GPIO_LEDS */
		break;
	case LED_AMBER:
#ifdef USE_GPIO_LEDS
		gpio_pin_set_dt(&amber_gpio_led, false);
#else
		led_off(pwm_leds_dev, amber_pwm_led.channel);
#endif /* USE_GPIO_LEDS */
		break;
	}
}

void main(void)
{
#ifdef USE_GPIO_LEDS
	if (!device_is_ready(green_gpio_led.port)) {
		return;
	}
	if (!device_is_ready(amber_gpio_led.port)) {
		return;
	}

	if (gpio_pin_configure_dt(&green_gpio_led, (GPIO_OUTPUT_INACTIVE | NRF_GPIO_DRIVE_H0H1)) < 0) {
		return;
	}
	if (gpio_pin_configure_dt(&amber_gpio_led, (GPIO_OUTPUT_INACTIVE | NRF_GPIO_DRIVE_H0H1)) < 0) {
		return;
	}
#else
	if (!device_is_ready(pwm_leds_dev)) {
		LOG_ERR("Device %s is not ready", pwm_leds_dev->name);
		return;
	}
	if (!device_is_ready(green_pwm_led.dev)) {
		LOG_ERR("Device %s is not ready", green_pwm_led.dev->name);
		return;
	}
	if (!device_is_ready(amber_pwm_led.dev)) {
		LOG_ERR("Device %s is not ready", amber_pwm_led.dev->name);
		return;
	}
#endif /* USE_GPIO_LEDS */

	/* Wait a bit before starting */
	k_msleep(DEEP_SLEEP_TIME_MS);

	int counter = 0;
	while (true) {
		if (counter == 40) {
			led_off_turn(LED_GREEN);
			led_off_turn(LED_AMBER);

			enter_deep_sleep();
		}

		if (counter % 4 == 0) {
			led_on_turn(LED_GREEN);
		} else if (counter % 4 == 1) {
			led_off_turn(LED_GREEN);
		} else if (counter % 4 == 2) {
			led_on_turn(LED_AMBER);
		} else if (counter % 4 == 3) {
			led_off_turn(LED_AMBER);
		}

		counter += 1;
		k_msleep(SLEEP_TIME_MS);
	}
}

prj.conf:

CONFIG_GPIO=y
CONFIG_SERIAL=n
CONFIG_LOG=n
CONFIG_CONSOLE=n
CONFIG_USE_SEGGER_RTT=n
CONFIG_PRINTK=n
CONFIG_BOOT_BANNER=n
CONFIG_PM=y
CONFIG_PM_DEVICE=y
CONFIG_LED=y
CONFIG_PWM=y
CONFIG_LED_PWM=y

When I measure the power consumption of the device running the modified blinky, when comparing the app run with GPIO LEDs, I see no difference between v2.1.0 and v2.3.0:

But then you can see the difference when using PWM LEDs:

[Øyvind]

Thanks for providing this information. Our developers were able to reproduce and find the root cuase. 

The problem is that for CONFIG_LED=y and an enabled "gpio-leds" node in devicetree, the led_gpio.c driver is enabled by default.

This has changed between NCS v2.1.0, where the LED_GPIO option was defined as:

config LED_GPIO
    bool "GPIO LED driver"
    depends on GPIO && $(dt_compat_enabled,gpio-leds)

and v2.2.0, since which it is defined in this way:

config LED_GPIO
    bool "GPIO LED driver"
    default y
    depends on GPIO && DT_HAS_GPIO_LEDS_ENABLED

And this driver is initialized after the pwm_nrfx.c one (which correctly configures the PWM output pins through pinctrl with the H0H1 drive), and it reconfigures the LED pins according to what is specified in child nodes of the "gpio-leds" node, where the NRF_GPIO_DRIVE_H0H1 flags are apparently not present. The solution is to disable the led_gpio.c driver, either by specifying CONFIG_LED_GPIO=n or by disabling the "gpio-leds" node in devicetree

[Øyvind]

Thanks for providing this information. Our developers were able to reproduce and find the root cuase. 

The problem is that for CONFIG_LED=y and an enabled "gpio-leds" node in devicetree, the led_gpio.c driver is enabled by default.

This has changed between NCS v2.1.0, where the LED_GPIO option was defined as:

config LED_GPIO
    bool "GPIO LED driver"
    depends on GPIO && $(dt_compat_enabled,gpio-leds)

and v2.2.0, since which it is defined in this way:

config LED_GPIO
    bool "GPIO LED driver"
    default y
    depends on GPIO && DT_HAS_GPIO_LEDS_ENABLED

And this driver is initialized after the pwm_nrfx.c one (which correctly configures the PWM output pins through pinctrl with the H0H1 drive), and it reconfigures the LED pins according to what is specified in child nodes of the "gpio-leds" node, where the NRF_GPIO_DRIVE_H0H1 flags are apparently not present. The solution is to disable the led_gpio.c driver, either by specifying CONFIG_LED_GPIO=n or by disabling the "gpio-leds" node in devicetree