TWIM drivers don't work with power management runtime enabled

[RobHel]

Hello. I'm trying to get my I2C peripheral (I2C1) working with Zephyr's Power Management feature. If I make my i2c1 peripheral compatible with "nordic,nrf-twi" the driver works, but the driver does not support Zephyr's CONFIG_PM functionality. 

If I make the driver compatible with "nordic,nrf-twim", which does support Zephyr's CONFIG_PM functionality, the driver does not work at all. Line 153 in i2c_nrfx_twim.c:

ret = k_sem_take(&dev_data->completion_sync, I2C_TRANSFER_TIMEOUT_MSEC);

assigns -EAGAIN to ret. Increasing the I2C_TRANSFER_TIMEOUT_MSEC doesn't fix the problem, so it appears the semaphore gets reset during the waiting period. 

Has the twim driver been tested and validated with CONFIG_PM=y?

My *-pinctrl.dtsi file and .dts file snippet are attached.

Thanks,

Rob

[ovrebekk]

Hi Rob

How are you using the driver from your application? 

Is there any chance you are calling driver functions from an interrupt? 
(keeping in mind that many of the callbacks triggered by various libraries will run in interrupt context)

I am not aware of any issues using the nrfx_twim driver when you have PM enabled, at worst it should just ignore any PM commands, but I can take a look at the implementation if you can confirm whether or not the driver is called from an interrupt. 

Best regards
Torbjørn 

[RobHel]

Hi ovrebekk 

I don't believe anything is happening in an interrupt context. I can't find a way to attach my .c file for you to view. 

On init, the PM action handler function gets called to init the device. In my PM action handler, when entering the case for the action PM_DEVICE_ACTION_RESUME, I do an I2C write. This happens immediatly upon startup. I can confirm this .c file is being initialized after and PM and I2C low level drivers. This I2C write is never successful and I have an assertion in my code to catch this unsuccessful write. 

Why would this be the case? By the way, I can test the I2C writing and reading seperatly using the shell and it works, but doesn't seem to work in the PM action handler function. Is this expected?

Thanks,

Rob

[ovrebekk]

Hi Rob

You should be able to drag drop files into the message box directly, or click Insert -> Image/video/file at the bottom. 

If you want to share a lot of files it is best to zip them first. 

Since you mentioned the PM action handler I assume you are writing your own driver. Looking at the overlay you shared earlier I notice you include ".yaml" in the compatible property, which I think you should remove. The property should just be the company and driver name: "ti,lp5030". 

Once you share the files I will take a closer look. 

Best regards
Torbjørn

[RobHel]

Hello ovrebekk 

Thanks for catching that. It didn't solve the overall problem, however. Attached is my lp5030 driver. It's the only peripheral on the I2C bus.

Thanks in advance.

Rob

Fullscreen lp5030.c Download

/*
 * Copyright (c) 2023 The Humble Transistor
 * Author: Rob Helvestine <[email protected]>
 *
 * @file TI LP5030 LED Driver
 *
 */

#include <stdbool.h>
#include <stdint.h>

#include <zephyr/kernel.h>
#include <zephyr/device.h>
#include <zephyr/pm/device.h>
#include <zephyr/pm/device_runtime.h>
#include <zephyr/drivers/i2c.h>

#include "lp5030.h"
#include "utils.h"
#include "lp5030_regs.h"

#include <zephyr/logging/log.h>
LOG_MODULE_REGISTER(lp5030, CONFIG_LOG_DEFAULT_LEVEL);

#define IS_OKAY(_nodename_)                                                    \
        DT_NODE_EXISTS(DT_NODELABEL(_nodename_)) &&                            \
    DT_NODE_HAS_STATUS(DT_NODELABEL(_nodename_), okay)

#define LOW_BRIGHTNESS 0x10

struct lp5030_config
{
    uint16_t addr_7b;
};

struct lp5030_data
{
    const struct device *i2c;
};

#define DEV_CFG(dev)							\
    ((struct lp5030_config * const)(dev)->config)

#define DEV_DATA(dev)							\
    ((struct lp5030_data *)(dev)->data)

static bool lp5030_write(const struct device *dev, const uint8_t offset,
        const uint8_t *data, const uint8_t length)
{
    struct lp5030_data *ddata = DEV_DATA(dev);
    const struct lp5030_config *cfg = DEV_CFG(dev);
    uint16_t r = 0;

    __ASSERT(length < LP5030_MAX_COMMS_SIZE, "%s: length must be less than %d\n",
            __FUNCTION__, LP5030_MAX_COMMS_SIZE);

    __ASSERT_NO_MSG(ddata->i2c);

    r |= i2c_burst_write(ddata->i2c, cfg->addr_7b, offset, data, length);

    if(r != 0) {
      LOG_WRN("%s: result: 0x%04x", __FUNCTION__, r);
    }

    return r == 0;
}

static bool lp5030_read(const struct device *dev, const uint8_t offset,
        uint8_t *data, const uint8_t length)
{
    struct lp5030_data *ddata = DEV_DATA(dev);
    const struct lp5030_config *cfg = DEV_CFG(dev);
    uint16_t r = 0;

    __ASSERT(length < (LP5030_MAX_COMMS_SIZE + 1), "%s: length must be less than %d\n",
            __FUNCTION__, (LP5030_MAX_COMMS_SIZE + 1));

    __ASSERT_NO_MSG(ddata->i2c);

    r |= i2c_burst_read(ddata->i2c, cfg->addr_7b, offset, data, length);

    if(r != 0) {
      LOG_WRN("%s: result: 0x%04x", __FUNCTION__, r);
    }

    return r == 0;
}

static void lp5030_enable_ic(const struct device *dev, uint8_t val)
{
    bool ret;
    uint8_t reg;
    __ASSERT(val < 2, "Invalid enable variable %d", val);

    reg = val ? LP5030__DEVICE_CONFIG0__CHIPEN__Enable_mask : 0;

    ret = lp5030_write(dev, LP5030__DEVICE_CONFIG0, &reg, 1);
    __ASSERT_NO_MSG(ret);
}

static void lp5030_enable(const struct device *dev, uint8_t val)
{
    __ASSERT(val < 2, "Invalid enable variable %d", val);
    int ret = 0;

    if(val) {
        ret = pm_device_runtime_get(dev);
        if(ret != 0){
            LOG_ERR("Error resuming the led driver's active power state");
        }
    }
    else {
        pm_device_runtime_put(dev);
    }
}

static void lp5030_control(const struct device *dev, uint16_t led_bitmask, 
    enum lp5030_control_mode mode)
{
    bool ret;
    uint16_t reg16;
    uint8_t led_config[2];

    lp5030_read(dev, LP5030__LED_CONFIG0, &led_config[0], 2);
    reg16 = ((uint16_t)led_config[1] << 8) | led_config[0];

    if(mode == BANK) {
        reg16 |= led_bitmask;
    }
    else {
        reg16 &= ~led_bitmask;
    }

    led_config[0] = reg16 & 0xff;
    led_config[1] = (reg16 >> 8) & 0xff;
    ret = lp5030_write(dev, LP5030__LED_CONFIG0, &led_config[0], 2);
    __ASSERT_NO_MSG(ret);
}

static void lp5030_bank_color(const struct device *dev, enum lp5030_color color, 
    const uint8_t mix)
{
    uint8_t reg;
    bool ret;

    switch(color) {
        case RED:
            reg = LP5030__BANK_C_COLOR;
            break;
        case GREEN:
            reg = LP5030__BANK_B_COLOR;
            break;
        case BLUE:
            reg = LP5030__BANK_A_COLOR;
            break;
        default:
            __ASSERT(false, "invalid color");
            break;
    }

    ret = lp5030_write(dev, reg, &mix, 1);
    __ASSERT_NO_MSG(ret);
}

static inline void on_state_led(const struct device *dev)
{
    bool ret;
    uint8_t reg;

    reg = LOW_BRIGHTNESS;
    ret = lp5030_write(dev, LP5030__OUT0_COLOR, &reg, 1);
    __ASSERT_NO_MSG(ret);
}

static inline void lp5030_configure_IC(const struct device *dev)
{
    bool ret;
    uint8_t reg;

    reg = LP5030__RESET__ALL;
    ret = lp5030_write(dev, LP5030__RESET, &reg, 1);
    __ASSERT_NO_MSG(ret);

    lp5030_control(dev, ALL_LEDS, INDEPENDENT);

    on_state_led(dev);
}

static int lp5030_init(const struct device *dev)
{
    pm_device_init_suspended(dev);
	pm_device_runtime_enable(dev);

    lp5030_enable(dev, 1);

    return 0;
}

static int lp5030_pm_action(const struct device *dev, enum pm_device_action action)
{
    int ret = 0;

    /* Validate that blocking API's can be used */
    if (!k_can_yield())
    {
        LOG_ERR("Blocking actions cannot run in this context");
        return -ENOTSUP;
    }

    switch (action) {
        case PM_DEVICE_ACTION_RESUME:
            lp5030_configure_IC(dev);
            lp5030_enable_ic(dev, 1);
            LOG_DBG("%s is now ON", dev->name);
            break;
        case PM_DEVICE_ACTION_SUSPEND:
            lp5030_enable_ic(dev, 0);
            LOG_DBG("%s is now OFF", dev->name);
            break;
        case PM_DEVICE_ACTION_TURN_ON:
            LOG_DBG("%s is OFF and powered", dev->name);
            break;
        case PM_DEVICE_ACTION_TURN_OFF:
            LOG_DBG("%s is OFF and not powered", dev->name);
            break;
        default:
            ret = -ENOTSUP;
    }

    return ret;
}

static const struct lp5030_api lp5030_api =
{
    .write              = lp5030_write,
    .read               = lp5030_read,
    .enable             = lp5030_enable,
    .control            = lp5030_control,
    .bank_color         = lp5030_bank_color,
};

#define LP5030_INIT(_nodename_)                                              \
    static struct lp5030_data _nodename_##_data = {                          \
        .i2c                = DEVICE_DT_GET(DT_BUS(DT_NODELABEL(_nodename_))), \
    };                                                                         \
    static const struct lp5030_config _nodename_##_config = {                \
        .addr_7b            = DT_REG_ADDR(DT_NODELABEL(_nodename_)),               \
    };                                                                         \
    PM_DEVICE_DT_DEFINE(DT_NODELABEL(_nodename_), lp5030_pm_action);           \
    DEVICE_DT_DEFINE(DT_NODELABEL(_nodename_), &lp5030_init, PM_DEVICE_DT_GET(DT_NODELABEL(_nodename_)),  \
            &_nodename_##_data, &_nodename_##_config,                                  \
            APPLICATION, CONFIG_LP5030_INIT_PRIORITY, &lp5030_api);          \

#if IS_OKAY(lp5030)
LP5030_INIT(lp5030)
#endif

 lp5030_regs.hlp5030.h

[ovrebekk]

Hi Rob

Do you have both CONFIG_PM_DEVICE and CONFIG_PM_DEVICE_RUNTIME enabled in your project configuration?

I checked with the developer, and he confirmed that the nrfx_twim driver is tested with PM enabled. It should automatically go to sleep if it is not used. 

Does the issue occur at any particular point in your application?

If you comment out the I2C calls inside the lp5030_pm_action(..) function, will the issue still occur? 

Best regards
Torbjørn

[RobHel]

Hi ovrebekk 

I do have both CONFIG_PM_DEVICE and CONFIG_PM_DEVICE_RUNTIME enabled. 

When I use the twi driver (not twim), my I2C calls work as expected. See the image below. 

When I use the twim driver (not twi), the I2C calls never seem to work. See the image for an I2C read. 

The twim driver behaves the same with CONFIG_PM_DEVICE and CONFIG_PM_DEVICE_RUNTIME both enabled and both disabled. 

I would like to use the twim driver because it has built in support for PM, whereas the twi driver does not. 

What could I be doing wrong that prevents the twim driver from reading / writing as expected? I only have one peripheral on the I2C bus, and the HW is validated as shown with the twi driver. 

Thanks,

Rob

[ovrebekk]

Hi Rob

I tried to recreate your issue last week, but so far I haven't been able to do so. I will have to spend some more time on this and get back to you in a day or two with an update. 

Best regards
Torbjørn