zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.83k stars 6.6k forks source link

dts: arm: st: standardize pwm default property st,prescaler to 0 #31290

Closed ABOSTM closed 3 years ago

ABOSTM commented 3 years ago

Looking at pwm property st,prescaler accross STM32 dtsi files, it appears that for some timer instances, value is 0, for other instances it is 10000. It seems that there is no reason to have either 0 or 10000, just some inheritance.

Proposal is to harmonize this default value for all instances, and set 0 to give the maximum speed.

Warning: this change may impact user applications, and this is the reason why we propose to do that after next LTS release, i.e. do that for release 3.0

ABOSTM commented 3 years ago

^^ @erwango @gmarull

ric96 commented 3 years ago

+1 I was testing PWM on the Blue Pill and found the same bug. PWM_1 seems to be set at 10000 prescaler and that meant a pwm pulse of 62500U nanoseconds gave a pwm output of 16HZ instead of 16KHZ. Setting the prescaler back to 0 solved the issue. So as of the current master branch this issue does introduce a PWM Bug due to the output being widely inaccurate.

erwango commented 3 years ago

@galak, I wonder what could be the best process for this change. Do you think it relates to https://github.com/zephyrproject-rtos/zephyr/issues/32225 ?

If not, then I think we should wait for 3.0 anymore (as this would be long now it is delayed). So maybe a warning on mailing list should be fine.

FRASTM commented 3 years ago

In that case where all st,prescaler = <0>; what is the benefit to have a dts entry with default value ?

FRASTM commented 3 years ago

Should this be extended to all the PWM prescaler : stm32f1, stm32f0 , stm32f3, stm32f4, stm32f7, stm32g4 , stm32h7, stm32l4, stm32wb soc devices which still have st,prescaler = <10000>;

erwango commented 3 years ago

In that case where all st,prescaler = <0>; what is the benefit to have a dts entry with default value ?

Point is to define default value to 0 in soc.dtsi files, which can be then be configured later on at board/application level. But having st,prescaler = <1000>; set as default is confusing for users. Specially because this is done arbitrarily in soc.dtsi depending on the timer instances.

Should this be extended to all the PWM prescaler : stm32f1, stm32f0 , stm32f3, stm32f4, stm32f7, stm32g4 , stm32h7, stm32l4, stm32wb soc devices which still have st,prescaler = <10000>;

This is indeed the request. Though, please note that this can't be done simply in a PR. There are users for these already. Changing these default values will have impact on their applications. This should be considered as an API change and should be notified by mail at least.

sarthsmart commented 3 years ago

@erwango @FRASTM So for now submitting PR, what st,prescaler value should I go with?

Already the values that are used from the reference of SOC series.

ric96 commented 3 years ago

Any updates on this or the related pr https://github.com/zephyrproject-rtos/zephyr/pull/32646 ?

The issue still seems to persist. The PWM LED fade sample works with prescaler value of 10000 but when mapping values between using MIN_PERIOD_NSEC MAX_PERIOD_NSEC marcos it only works if prescaler is set to 0. So this line does not works with 10000 prescaler pwm_pin_set_nsec(pwm_tim, pwm_chan , MAX_PERIOD_NSEC, pwm_val, 0);

EDIT: this seems to be for all stm32 parts. I have tested it between some stm32f0 and stm32f4 variants

erwango commented 3 years ago

@ric96 nothing has been done for now. But I plan to get this fixed for v2.7.0

Peddaahh commented 2 years ago

As you may have seen in discord @erwango @FRASTM.

Board

STM32 Nucleo H743ZI2 /zephyr/boards/arm/nucleo_h743zi/nucleo_h743zi.dts

Problem

I did this to set the period frequency

/ {
    servo: servo {
        compatible = "pwm-servo";
        pwms = <&pwm12 1 PWM_USEC(100) PWM_POLARITY_NORMAL>;
        min-pulse = <PWM_USEC(0)>;
        max-pulse = <PWM_USEC(100)>;
    };
};

when setting the period to 10kHz (100µsec) i get no pwm output anymore

/ {
    servo: servo {
        compatible = "pwm-servo";
        pwms = <&pwm12 1 PWM_MSEC(1) PWM_POLARITY_NORMAL>;
        min-pulse = <PWM_USEC(0)>;
        max-pulse = <PWM_USEC(1000)>;
    };
};

whenever i set it to 1kHz (1ms) it works. same for 100Hz (10ms)

Solution

There's still a prescaler set in dts... default: 10000...(/zephyr/boards/arm/nucleo_h743zi/nucleo_h743zi.dts).. i reset it to 0... no problems now.

/ {
    servo: servo {
        compatible = "pwm-servo";
        pwms = <&pwm12 1 PWM_KHZ(10) PWM_POLARITY_NORMAL>;
        min-pulse = <PWM_USEC(0)>;
        max-pulse = <PWM_USEC(100)>;
    };
};

&timers12 {
    st,prescaler = <0>;
};

In case someone encounters this issue.