PM and samd21

[photonthunder]

With PR #7726 which removed the weak implementation of pm_set_lowest it now defaults to pm_set_lowest implemented in sys/pm_layered/pm.c. I am fine using this implementation but I don't have a handle for changing PM_NUM_MODES since it is set as a #define in periph_cpu_common.h (maybe change this to an #ifndef?

Also where can I modify PM_BLOCKER_INITIAL. For it would appear that you want it to start out as 0xFFFFFFFF so that all modes are unblocked and you can block modes as necessary (pm_block has an assert that is fired if the value is not 255) or set it to 0 and unblock as necessary.

Let me state that things work fine if I hack the riot code for my purposes. Just looking for a cleaner method of implementation.

[roberthartung]

Let me give you some thoughts.

#7726 does not always use pm_set_lowest from sys/pm_layered/pm.c. Only if the CPU explicitely selects pm_layered, it will be used. Otherwise the default/empty (prev. weak) version will be used. So no changes here.

PM_BLOCKER_INITIAL is just a define you can set (#ifndef). Ideally you would set it in your periph_cpu.h as stated in sys/pm_layered/pm.c.
PM_BLOCKER_INITIAL has to be 0x01010101 as CPUs who don't have correct pm_block/pm_unblock implemented should never go to any sleep modes. Which is what most CPUs/boards don't have atm.

If there is a correct pm implementation for a cpu/family. Blocker should be 0 and once peripherals get initialized, the block values increase. So this behaviour is totally fine.

What exactly did you have to hack? Bevore #7726 pm_layered was used for samd21 as well. So nothing should've changed here!

[roberthartung]

@haukepetersen @kaspar030 Any other thoughts?

[photonthunder]

@roberthartung thanks for the response. I am on the samd21, and I have been using the weak implementation to allow me to create my own pm_set_lowest. Since the weak implementation was removed and it know defaults to sys/pm_layered/pm.c I thought I would give it a try. What confused me was the asserts, why are the options limited to 0x00 or 0xFF for the mode value?

void pm_block(unsigned mode)
{
    assert(pm_blocker.val_u8[mode] != 255);

    unsigned state = irq_disable();
    pm_blocker.val_u8[mode]++;
    irq_restore(state);
}

void pm_unblock(unsigned mode)
{
    assert(pm_blocker.val_u8[mode] > 0);

    unsigned state = irq_disable();
    pm_blocker.val_u8[mode]--;
    irq_restore(state);
}

That would make it appear that the goal is to use 0xFF and 0x00 as block/unblock values? Thus, you would have to set PM_BLOCKER_INITIAL accordingly.

Also, the samd21 has 4 sleep modes but PM_NUM_MODES is set as 3 in sam0_common/include/periph_cpu_common.h (3 is the correct number of modes for the samL21).

[roberthartung]

0xFF is used to prevent an overflow. When you overflow from 255 to 0 the level is unblocked and results in a system failure.

It is not about how many actual modes there are. It is more about usable and meaningful levels. It is up to the developer to choose the right number.

a) I am confused, because samd always should have used pm_set_lowest from pm_layered. The only thing you should modify is pm_set because of pm_layered always provides pm_set_lowest, based on the available level! That you have to modify it sounds wrong!

b) In general, you can still provide pm_set_lowest, but not if pm_layered is used, because it uses the modes (layers) ;)

[photonthunder]

Thanks for the clarification, I was clearly reading that wrong and was getting confused.

a) Before #7726 the weak implementation would actually be the chosen function due to the order of files at compile time (#6709). Thus, I would just stick my own version of pm_set_lowest in board.c.

b) I am interested in using the pm_layered version of pm_set_lowest, sounds like I just need to push a PR so that PM_NUM_MODES is chip family specific?

[roberthartung]

a) As stated in #6709 / #6655 the build system was not correctly choosing the implementations it should! That's why we removed the weak definitions.

b) PM_NUM_MODES can be anything between 1 and 4 and has to match pm_set() which is already defined in cpu/samd21/periph/pm.c.

I had to look in the code of pm_layered. We will start at mode = PM_NUM_MODESwhich in our case is 3. Then we decrease mode while it is > 0. This means, that in case of the samd21 3 should be the idle mode! And this actually gives us 3 additional modes to the idle mode.

3 is correct here in my opinion and just matches the current implementation. The Summary I can see that standby is the mode with the highest power saved. This matches that we use 0 for the lowest level. The modes are defined in the Full Datasheet pp125. So all modes are already defined and should be usable right away.

[roberthartung]

However I agree, that this is not usefull, because the layers are not correctly blocked/unblocked. Feel free to create a PR that provides a full pm implementation for the samd21. I will to the same shortly for atmega_common.

[photonthunder]

Thanks again for the clarification, it is a little confusing that PM_NUM_MODES = 3 represents 4 modes but I follow your explanation. So if I want to use the standby sleep mode then I have to pm_unblock 3,2,1, and 0. Then if I only want to go to idle 1 mode then I can pm_block(3). I can then implement the pm_block/pm_unblock in my code to keep the cpu at different sleep states depending on which modes are on and off.

There already is a pm.c for the samd21 and I have been using it and it appears to work fine. I just obviously wasn't seeing how the pm_layered was setup so thanks for the explanation. I will do some playing but I think I have the pieces to implement this now in my code. I will post when I have something that works.

[roberthartung]

pm_block and pm_unblock are intended to be used in the periph drivers, not the user code if not necessary.

[photonthunder]

But how do you know which sleep mode the user wants to enter, since that is normally application specific? Do you make the mode value that goes into each function a value feed from periph_conf.h? So the user can then specify the USART and as part of that specification what sleep mode to enter?

[roberthartung]

Power management is not that easy! In fact, it is veeery complex.

The sleep modes are dependent on the hardware. My approach for atmega_common is to define the minimum level required for each peripheral component or more specificly, which mode is invalid and has to be blocked.

Please not that this is an old version

#define PM_NUM_MODES (6)

enum pm_sleep_modes {
    PM_SLEEPMODE_PWR_DOWN    = 0,
    PM_SLEEPMODE_STANDBY     = 1,
    PM_SLEEPMODE_PWR_SAVE    = 2,
    PM_SLEEPMODE_EXT_STANDBY = 3,
    PM_SLEEPMODE_ADC         = 4,
    PM_SLEEPMODE_IDLE        = 5,
};

/// The level where the SPI cannot operate anymore
#define PM_SLEEPMODE_INVALID_TWI     PM_SLEEPMODE_ADC
#define PM_SLEEPMODE_INVALID_SPI     PM_SLEEPMODE_ADC
#define PM_SLEEPMODE_INVALID_UART0   PM_SLEEPMODE_ADC
#define PM_SLEEPMODE_INVALID_UART1   PM_SLEEPMODE_ADC
#define PM_SLEEPMODE_INVALID_TIMER0  PM_SLEEPMODE_ADC
#define PM_SLEEPMODE_INVALID_TIMER1  PM_SLEEPMODE_ADC
#define PM_SLEEPMODE_INVALID_TIMER2  PM_SLEEPMODE_STANDBY
#define PM_SLEEPMODE_INVALID_TIMER3  PM_SLEEPMODE_ADC
//#define PM_SLEEPMODE_INVALID_RTC     PM_SLEEPMODE_PWR_DOWN
/* Watchdog is always running! not defining a level here will  */
//#define PM_SLEEPMODE_INVALID_WDT

The periph drivers will then simply block/unblock when power_on() or power_off() is used. e.g.:

void uart_poweron(uart_t uart) {
    switch(ctx[uart].num) {
#ifdef MEGA_UART0
    case 0 :
    #ifdef MODULE_PM_LAYERED
        pm_block(PM_SLEEPMODE_INVALID_UART0);
    #endif
        power_usart0_enable();
    break;
#endif
/* ... */
    }
/* ... */
}

void uart_poweroff(uart_t uart) {
/* ... */
    switch(ctx[uart].num) {
#ifdef MEGA_UART0
    case 0 :
        power_usart0_disable();
    #ifdef MODULE_PM_LAYERED
        pm_unblock(PM_SLEEPMODE_INVALID_UART0);
    #endif
    break;
#endif
/* ... */
    }
}

By default, atmega_common only uses the uart. So it will never go to sleep, which is absolutely correct for default applications. The user therefore has to decide when to turn off all peripherals. While for an ADC, I2C or SPI this might be easy, because you could turn it off if you don't need it, you might just turn off the uart at some point in your program.
This will result in the correct mode being used.

For the samd21 this means that you have to look into the Datasheet Table 15-4. and need to check which peripherals are dependent on which bus clock. Then peripherals will block modes, because they wouldn't work without.

[roberthartung]

FYI: I will update the wiki later, once this is completely implemented in atmega_common and I did extensive testing!

[photonthunder]

On the samd21 that will get very challenging since any peripheral can be hooked to any of the 9 clock generators. You also have to set the peripheral and clock specific to the desired sleep mode, for example you have to set the RUNSTBY and/or ONDEMAND register for the clock source, clock generator, and the peripheral.

[photonthunder]

Thinking more of the power management modes with regards to the samd21. The real control is the run standby and on demand settings (as shown in Table 16-4, p. 135). So what I would like to do would be to take the changes I made in PR #7315 (if I could ever get it merged) and then build on that for what sleep mode, if any, to block/unblock each peripheral, depending on the run standby and on demand settings. The run standby and on demand options would be further integrated with the different peripherals as has already begun in uart.c (see PR #7405 as an example).

So when there is enough interest (manifest by getting PR #7315 merged) then I will continue the process. However, until then I don't want to get too many steps away from main RIOT. Meanwhile, I will just throw the blocks and unblocks in my application code so I can control the levels. Thanks for the insight.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.