Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ABOSTM opened this issue Jan 13, 2021 · 10 comments · Fixed by #36133
Closed

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

ABOSTM opened this issue Jan 13, 2021 · 10 comments · Fixed by #36133
Labels
area: PWM Pulse Width Modulation Enhancement Changes/Updates/Additions to existing features platform: STM32 ST Micro STM32
Milestone

Comments

@ABOSTM
Copy link
Collaborator

ABOSTM commented Jan 13, 2021

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 ABOSTM added Enhancement Changes/Updates/Additions to existing features area: PWM Pulse Width Modulation platform: STM32 ST Micro STM32 labels Jan 13, 2021
@ABOSTM ABOSTM added this to the future milestone Jan 13, 2021
@ABOSTM
Copy link
Collaborator Author

ABOSTM commented Jan 13, 2021

^^ @erwango @gmarull

@ric96
Copy link
Contributor

ric96 commented Jan 17, 2021

+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
Copy link
Member

erwango commented Mar 11, 2021

@galak, I wonder what could be the best process for this change. Do you think it relates to #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
Copy link
Collaborator

FRASTM commented Apr 14, 2021

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

@FRASTM
Copy link
Collaborator

FRASTM commented Apr 14, 2021

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
Copy link
Member

erwango commented Apr 14, 2021

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
Copy link
Contributor

sarthsmart commented Apr 14, 2021

@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
Copy link
Contributor

ric96 commented Jun 4, 2021

Any updates on this or the related pr #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
Copy link
Member

erwango commented Jun 4, 2021

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

@Peddaahh
Copy link

Peddaahh commented Aug 18, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: PWM Pulse Width Modulation Enhancement Changes/Updates/Additions to existing features platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants