| Message ID | 20190324220217.15813-1-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | pwm: meson: fix scheduling while atomic issue | expand |
Hello Martin, On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote: > Back in January a "BUG: scheduling while atomic" error showed up during > boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply). > The call trace comes down to: > __mutex_lock > clk_prepare_lock > clk_core_get_rate > meson_pwm_apply > .. > dev_pm_opp_set_rate > .. > > Jerome has also seen the same problem but from pwm-leds (instead of a > pwm-regulator). He posted a patch which replaces the spinlock with a > mutex. That works. I believe we can optimize this by reducing the time > where the lock is held - that also allows to keep the spin-lock. > > Analyzing this issue helped me understand the pwm-meson driver better. > My plan is to send some cleanups (with the goal of re-using more of the > goodies from the PWM core in the pwm-meson driver) after this single fix > is merged (they can be found here: [1]). I didn't look over these in detail, but I see an issue that according to the shortlogs isn't addressed: In the .apply callback there is (simplified): if (!state->enabled) { meson_pwm_disable(meson, pwm->hwpwm); return; } This results in the wrong output after: pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...}); pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...}); because the polarity isn't checked. If you want to implement further cleanups, my questions and propositions are: - Is there a publicly available manual for this hardware? If yes, you can add a link to it in the header of the driver. - Why do you handle reparenting of the PWM's clk in .request? Wouldn't this be more suitable in .apply? - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB register) freeze the output, or is the currently running period completed first? (The latter is the right behaviour.) - Please point out in the header that for changing period/duty cycle/polarity the hardware must be stopped. (I suggest to apply the style used in https://www.spinics.net/lists/linux-pwm/msg09262.html for some consistency.) Best regards Uwe
Hello, On Mon, Mar 25, 2019 at 09:41:53AM +0100, Uwe Kleine-König wrote: > If you want to implement further cleanups, my questions and propositions > are: > > - Is there a publicly available manual for this hardware? If yes, you > can add a link to it in the header of the driver. > > - Why do you handle reparenting of the PWM's clk in .request? Wouldn't > this be more suitable in .apply? > > - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB > register) freeze the output, or is the currently running period > completed first? (The latter is the right behaviour.) > > - Please point out in the header that for changing period/duty > cycle/polarity the hardware must be stopped. (I suggest to apply the > style used in https://www.spinics.net/lists/linux-pwm/msg09262.html > for some consistency.) Another thing I just noted: The .get_state callback only sets .enabled but nothing of the remaining information is provided. Best regards Uwe
On Sun, 2019-03-24 at 23:02 +0100, Martin Blumenstingl wrote: > Back in January a "BUG: scheduling while atomic" error showed up during > boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply). > The call trace comes down to: > __mutex_lock > clk_prepare_lock > clk_core_get_rate > meson_pwm_apply > .. > dev_pm_opp_set_rate > .. > > Jerome has also seen the same problem but from pwm-leds (instead of a > pwm-regulator). He posted a patch which replaces the spinlock with a > mutex. That works. I believe we can optimize this by reducing the time > where the lock is held - that also allows to keep the spin-lock. > > Analyzing this issue helped me understand the pwm-meson driver better. > My plan is to send some cleanups (with the goal of re-using more of the > goodies from the PWM core in the pwm-meson driver) after this single fix > is merged (they can be found here: [1]). Thanks for fixing this Martin. As for the future enhancement, I'd like to know what you have in mind. As I have told you previously, I think the clock bindings of this driver are not great. The global name of the input clocks are hard coded in this driver and it sucks. CCF is evolving to rely less on these global names. In addition, the 'clock' binding should be used to refer to the clock 'consumed' by the device, not to define a setting (as done now). 'assigned- clock' binding can be used for that. This would be a significant change in the binding meaning of this driver, which probably calls for a v2. Last, instead of specifying the parent to be used, I think we should come up with some code to let the driver pick the most appropriate parent for the period/duty requested. > > Dependencies: none > > Target version: please queue this for -fixes so it makes it's way into > v5.1-rc (so we can get it backported from there, because this issue has > existed since the pwm-meson driver was introduced). > > > [0] http://lists.infradead.org/pipermail/linux-amlogic/2019-January/009690.html > [1] https://github.com/xdarklight/linux/commits/meson-pwm-for-5.2-v0 > > > Martin Blumenstingl (1): > pwm: meson: use the spin-lock only to protect register modifications > > drivers/pwm/pwm-meson.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) >
Hello Uwe, On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Martin, > > On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote: > > Back in January a "BUG: scheduling while atomic" error showed up during > > boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply). > > The call trace comes down to: > > __mutex_lock > > clk_prepare_lock > > clk_core_get_rate > > meson_pwm_apply > > .. > > dev_pm_opp_set_rate > > .. > > > > Jerome has also seen the same problem but from pwm-leds (instead of a > > pwm-regulator). He posted a patch which replaces the spinlock with a > > mutex. That works. I believe we can optimize this by reducing the time > > where the lock is held - that also allows to keep the spin-lock. > > > > Analyzing this issue helped me understand the pwm-meson driver better. > > My plan is to send some cleanups (with the goal of re-using more of the > > goodies from the PWM core in the pwm-meson driver) after this single fix > > is merged (they can be found here: [1]). > > I didn't look over these in detail, but I see an issue that according > to the shortlogs isn't addressed: In the .apply callback there is > (simplified): > > if (!state->enabled) { > meson_pwm_disable(meson, pwm->hwpwm); > return; > } > > This results in the wrong output after: > > pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...}); > pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...}); > > because the polarity isn't checked. let me rephrase this to make sure I understand this correctly: - applying a PWM state with .enabled = true and PWM_POLARITY_NORMAL will enable the PWM output - applying a PWM state with .enabled = false and PWM_POLARITY_NORMAL will disable the PWM output - applying a PWM state with .enabled = true and PWM_POLARITY_INVERTED will disable the PWM output - applying a PWM state with .enabled = false and PWM_POLARITY_INVERTED will enable the PWM output in other words: the polarity doesn't only apply to period and duty_cycle but also to the enabled state. > If you want to implement further cleanups, my questions and propositions > are: > > - Is there a publicly available manual for this hardware? If yes, you > can add a link to it in the header of the driver. yes, it's documented in the public S912 datasheet [0] page 542 and following I'll add a patch which adds the link to the driver > - Why do you handle reparenting of the PWM's clk in .request? Wouldn't > this be more suitable in .apply? Jerome's answer (not long after yours) basically covers this: - the assigned-clock-parents property could have been used but it wasn't - the driver could automatically set the correct parent, but this isn't implemented (I assume this was done to keep it short and simple to for the first version of the driver) > - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB > register) freeze the output, or is the currently running period > completed first? (The latter is the right behaviour.) I don't know, I would have to measure this with a logic analyzer. can you please explain why this is important? > - Please point out in the header that for changing period/duty > cycle/polarity the hardware must be stopped. (I suggest to apply the > style used in https://www.spinics.net/lists/linux-pwm/msg09262.html > for some consistency.) I'm not sure about this. Amlogic's vendor kernel uses a modified version of this driver [1] which has an explicit comment not to disable the PWM output when changing the period/duty cycle. the PWM is configured with two separate registers (PWM_MISC_REG_AB contains the divider and PWM_PWM_A contains the high/low count). there's a short timeframe where the PWM output signal is neither the "old setting" nor the "new setting" (but rather a mix of both). what do other PWM drivers do in this case (if this is a common thing)? > Another thing I just noted: The .get_state callback only sets .enabled > but nothing of the remaining information is provided. as far as I can see the PWM core uses .get_state only during registration: this means we should read (and calculate) .duty_cycle and .period from the register values. polarity always has to be "normal" since there's no information about it in the registers. Regards Martin [0] https://dl.khadas.com/Hardware/VIM2/Datasheet/S912_Datasheet_V0.220170314publicversion-Wesion.pdf [1] https://github.com/hardkernel/linux/blob/9a4a1f4b14fe66d7ebd73323b39fbf3bda9e1356/drivers/amlogic/pwm/pwm_meson.c#L381
Hi Jerome, On Mon, Mar 25, 2019 at 10:35 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Sun, 2019-03-24 at 23:02 +0100, Martin Blumenstingl wrote: > > Back in January a "BUG: scheduling while atomic" error showed up during > > boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply). > > The call trace comes down to: > > __mutex_lock > > clk_prepare_lock > > clk_core_get_rate > > meson_pwm_apply > > .. > > dev_pm_opp_set_rate > > .. > > > > Jerome has also seen the same problem but from pwm-leds (instead of a > > pwm-regulator). He posted a patch which replaces the spinlock with a > > mutex. That works. I believe we can optimize this by reducing the time > > where the lock is held - that also allows to keep the spin-lock. > > > > Analyzing this issue helped me understand the pwm-meson driver better. > > My plan is to send some cleanups (with the goal of re-using more of the > > goodies from the PWM core in the pwm-meson driver) after this single fix > > is merged (they can be found here: [1]). > > Thanks for fixing this Martin. you're welcome! > As for the future enhancement, I'd like to know what you have in mind. > As I have told you previously, I think the clock bindings of this driver are > not great. > > The global name of the input clocks are hard coded in this driver and it > sucks. CCF is evolving to rely less on these global names. I fully agree with you on the clock setup, but I'm not sure if we have to break the dt-bindings for it. the datasheet notes: "Each PWM is driven by a programmable divider driven by a 4:1 clock selector". In my own words this means that each PWM controller has up to 8 clock inputs: - up to 4 inputs for the first channel (PWM_A) - up to 4 inputs for the second channel (PWM_B) the current pwm-meson driver assumes that both the inputs for both channels are identical. the "clock trees" section of the public S912 datasheet (page 65) clearly documents the clock inputs per PWM channel, not per PWM controller. Thus I believe we should name our clock-names (the inputs to the PWM controller) "pwm-a-clkin0", "pwm-a-clkin1", "pwm-b-clkin0", ... That way we don't have a conflict with the existing bindings (which already reserve "clkin0" and "clkin1"). > In addition, the 'clock' binding should be used to refer to the clock > 'consumed' by the device, not to define a setting (as done now). 'assigned- > clock' binding can be used for that. using the assigned-clock* properties requires self-referencing the PWM controller (which I'm not used to), for example: &pwm_ab { #clock-cells = <1>; assigned-clocks = <&pwm_ab 0>, <&pwm_ab 1>; /* references itself */ assigned-clock-parents = <&xtal>, <&clkc CLKID_FCLK_DIV5>; }; if we want to auto-detect the parent clock (like you suggested below) we need to consider if we can detect whether a .dts-author assigned a specific parent. I know that it's easy to detect this when the clock is passed in the "clocks" property, but I'm not sure if it's easy to parse it from the assigned-clocks/assigned-clock-parents properties. [...] > Last, instead of specifying the parent to be used, I think we should come up > with some code to let the driver pick the most appropriate parent for the period/duty requested. that will make it easier for .dts authors. I would like to postpone this until we have solved the other topics though. Regards Martin [0] https://dl.khadas.com/Hardware/VIM2/Datasheet/S912_Datasheet_V0.220170314publicversion-Wesion.pdf
Hello Martin, On Mon, Mar 25, 2019 at 06:41:57PM +0100, Martin Blumenstingl wrote: > On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote: > > > Analyzing this issue helped me understand the pwm-meson driver better. > > > My plan is to send some cleanups (with the goal of re-using more of the > > > goodies from the PWM core in the pwm-meson driver) after this single fix > > > is merged (they can be found here: [1]). > > > > I didn't look over these in detail, but I see an issue that according > > to the shortlogs isn't addressed: In the .apply callback there is > > (simplified): > > > > if (!state->enabled) { > > meson_pwm_disable(meson, pwm->hwpwm); > > return; > > } > > > > This results in the wrong output after: > > > > pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...}); > > pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...}); > > > > because the polarity isn't checked. > let me rephrase this to make sure I understand this correctly: > - applying a PWM state with .enabled = true and PWM_POLARITY_NORMAL > will enable the PWM output > - applying a PWM state with .enabled = false and PWM_POLARITY_NORMAL > will disable the PWM output > - applying a PWM state with .enabled = true and PWM_POLARITY_INVERTED > will disable the PWM output > - applying a PWM state with .enabled = false and PWM_POLARITY_INVERTED > will enable the PWM output > > in other words: the polarity doesn't only apply to period and > duty_cycle but also to the enabled state. You're wrong (I think): - if .enabled = true, you should configure the output to repeat the following pattern: be active for $duty_cycle ns and then inactive for the rest of $period ns. - if .enabled = false, you should configure the output to be constant inactive. - if .polarity = PWM_POLARITY_NORMAL we have: inactive = low, active = high - if .polarity = PWM_POLARITY_INVERTED we have: inactive = high, active = low So after the two pwm_apply_state above the expectation is that the output is constant high. But as the meson driver's apply function doesn't check for .polarity when .enabled = false is requested the result is probably constant low. (Unless the driver is still more broken and doesn't ensure the output gets inactive on .disable().) > > If you want to implement further cleanups, my questions and propositions > > are: > > > > - Is there a publicly available manual for this hardware? If yes, you > > can add a link to it in the header of the driver. > yes, it's documented in the public S912 datasheet [0] page 542 and following > I'll add a patch which adds the link to the driver > > > - Why do you handle reparenting of the PWM's clk in .request? Wouldn't > > this be more suitable in .apply? > Jerome's answer (not long after yours) basically covers this: > - the assigned-clock-parents property could have been used but it wasn't > - the driver could automatically set the correct parent, but this > isn't implemented > > (I assume this was done to keep it short and simple to for the first > version of the driver) I don't know how assigned-clock-parents works, but maybe it is even simpler to use than the hardcoding that currently is used in the driver? > > - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB > > register) freeze the output, or is the currently running period > > completed first? (The latter is the right behaviour.) > I don't know, I would have to measure this with a logic analyzer. In practise you can do this with a multimeter, too. Just do something like: pwm_apply_state({ .enabled = true, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL }); pwm_apply_state({ .enabled = false, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL }); (assuming the PWM supports periods that long). The expectation is that the last command takes nearly 5 s to complete and while it waits the output is high and on return it's low. If that isn't the case, there is a bug somewhere. > can you please explain why this is important? Well, that's the semantic that the PWM API promises to its users. Up to now this is poorly documented, there is an RFC patch waiting for review that improves the situation. > > - Please point out in the header that for changing period/duty > > cycle/polarity the hardware must be stopped. (I suggest to apply the > > style used in https://www.spinics.net/lists/linux-pwm/msg09262.html > > for some consistency.) > I'm not sure about this. Amlogic's vendor kernel uses a modified > version of this driver [1] which has an explicit comment not to > disable the PWM output when changing the period/duty cycle. That would be better as stopping the driver also violates the API's requirements. If this is not a hardware imposed limit, it would be great to fix the driver accordingly. > the PWM is configured with two separate registers (PWM_MISC_REG_AB > contains the divider and PWM_PWM_A contains the high/low count). > there's a short timeframe where the PWM output signal is neither the > "old setting" nor the "new setting" (but rather a mix of both). what > do other PWM drivers do in this case (if this is a common thing)? If this cannot be prevented in hardware, I think this should be documented clearly at the top of the driver. Up to now this kind of problem isn't (TTBOMK) well tracked and questions like "How many drivers suffer from $problem" cannot be easily answered. (A bit unrelated: I think the violation that disabling a PWM doesn't complete the currently running period is a common one. But because the corresponding questions where not asked for new driver submissions until recently, this is untracked and probably driver authors are not even aware of this requirement.) > > Another thing I just noted: The .get_state callback only sets .enabled > > but nothing of the remaining information is provided. > as far as I can see the PWM core uses .get_state only during registration: > this means we should read (and calculate) .duty_cycle and .period from > the register values. polarity always has to be "normal" since there's > no information about it in the registers. So you can change the polarity, but you cannot read that information back? That's another item for the list of limitations. Best regards Uwe
On Mon, 2019-03-25 at 19:04 +0100, Martin Blumenstingl wrote: > > Thanks for fixing this Martin. > you're welcome! > > > As for the future enhancement, I'd like to know what you have in mind. > > As I have told you previously, I think the clock bindings of this driver are > > not great. > > > > The global name of the input clocks are hard coded in this driver and it > > sucks. CCF is evolving to rely less on these global names. > I fully agree with you on the clock setup, but I'm not sure if we have > to break the dt-bindings for it. > > the datasheet notes: "Each PWM is driven by a programmable divider > driven by a 4:1 clock selector". > In my own words this means that each PWM controller has up to 8 clock inputs: > - up to 4 inputs for the first channel (PWM_A) > - up to 4 inputs for the second channel (PWM_B) Not from the pwm device POV. there is one device (PWM_AB) with 4 (max) input clocks. Those are consumed by two internal muxes. There would be 8 if the input was different between path A and B. > > the current pwm-meson driver assumes that both the inputs for both > channels are identical. > the "clock trees" section of the public S912 datasheet (page 65) > clearly documents the clock inputs per PWM channel, not per PWM > controller. > > Thus I believe we should name our clock-names (the inputs to the PWM > controller) "pwm-a-clkin0", "pwm-a-clkin1", "pwm-b-clkin0", ... > That way we don't have a conflict with the existing bindings (which > already reserve "clkin0" and "clkin1"). I think this is overkill an inaccurate. The experience of all the soc we have seen so far (meson8, gxbb, gxl, gxm, axg and g12) confirms the sources the are the same input clock for both path. The documentation just shows the clock src of each pwm. That just how the the table is presented. That does not change the fact the pwms are organized in modules (pairs) and the that the clock source are the same for each pwm of the module. IOW, there is only 4 lines of clocks getting to the IP, not 8. Feel free to ask amlogic if you want to make sure. The name clash is not really my point. The purpose of the clock binding would be different (from stating a setting to describing hw connection) > > > In addition, the 'clock' binding should be used to refer to the clock > > 'consumed' by the device, not to define a setting (as done now). 'assigned- > > clock' binding can be used for that. > using the assigned-clock* properties requires self-referencing the PWM > controller (which I'm not used to), for example: > &pwm_ab { > #clock-cells = <1>; > assigned-clocks = <&pwm_ab 0>, <&pwm_ab 1>; /* references itself */ > assigned-clock-parents = <&xtal>, <&clkc CLKID_FCLK_DIV5>; > }; > > if we want to auto-detect the parent clock (like you suggested below) > we need to consider if we can detect whether a .dts-author assigned a > specific parent. I (personally) don't want to keep supporting the manual assignment of the parent. If the driver can guarantee than it will pick the most appropriate parent, there is no reason to have that. > I know that it's easy to detect this when the clock is passed in the > "clocks" property, but I'm not sure if it's easy to parse it from the > assigned-clocks/assigned-clock-parents properties. Assigned parent is the poor man solution and not necessarily easier to implement (the pwm device would have to export its own clocks) ... I have just mentioned it to make the point that current method is not ideal > > [...] > > Last, instead of specifying the parent to be used, I think we should come up > > with some code to let the driver pick the most appropriate parent for the period/duty requested. > that will make it easier for .dts authors. > I would like to postpone this until we have solved the other topics though. I much prefer this last solution. Since the algorithm and the bindings would change, I think it would be easier (in DT) to just make v2 driver with a new compatible, progressively transition dts to it and finally remove the old driver.
On 26/03/2019 09:37, Jerome Brunet wrote: > On Mon, 2019-03-25 at 19:04 +0100, Martin Blumenstingl wrote: >>> Thanks for fixing this Martin. >> you're welcome! >> >>> As for the future enhancement, I'd like to know what you have in mind. >>> As I have told you previously, I think the clock bindings of this driver are >>> not great. >>> >>> The global name of the input clocks are hard coded in this driver and it >>> sucks. CCF is evolving to rely less on these global names. >> I fully agree with you on the clock setup, but I'm not sure if we have >> to break the dt-bindings for it. >> >> the datasheet notes: "Each PWM is driven by a programmable divider >> driven by a 4:1 clock selector". >> In my own words this means that each PWM controller has up to 8 clock inputs: >> - up to 4 inputs for the first channel (PWM_A) >> - up to 4 inputs for the second channel (PWM_B) > > Not from the pwm device POV. there is one device (PWM_AB) with 4 (max) input > clocks. Those are consumed by two internal muxes. There would be 8 if the > input was different between path A and B. The PWM pair is a imple duplicate of a PWM HW, sharing the same clock parents, as he driver is already designed, you can only have (for now) only 4 possible parents per pair. It may change in the future, but for SoC from Meson8 to G12B, it's how it's designed _now_, no need to discuss an eventual future change. > >> >> the current pwm-meson driver assumes that both the inputs for both >> channels are identical. >> the "clock trees" section of the public S912 datasheet (page 65) >> clearly documents the clock inputs per PWM channel, not per PWM >> controller. >> >> Thus I believe we should name our clock-names (the inputs to the PWM >> controller) "pwm-a-clkin0", "pwm-a-clkin1", "pwm-b-clkin0", ... >> That way we don't have a conflict with the existing bindings (which >> already reserve "clkin0" and "clkin1"). > > I think this is overkill an inaccurate. The experience of all the soc we have > seen so far (meson8, gxbb, gxl, gxm, axg and g12) confirms the sources the are > the same input clock for both path. > > The documentation just shows the clock src of each pwm. That just how the the > table is presented. That does not change the fact the pwms are organized in > modules (pairs) and the that the clock source are the same for each pwm of the > module. IOW, there is only 4 lines of clocks getting to the IP, not 8. Feel > free to ask amlogic if you want to make sure. Indeed, the possible parents for each pair on PWM is the same, so we would only need 4 input clocks per PWM pair. > > The name clash is not really my point. The purpose of the clock binding would > be different (from stating a setting to describing hw connection) > >> >>> In addition, the 'clock' binding should be used to refer to the clock >>> 'consumed' by the device, not to define a setting (as done now). 'assigned- >>> clock' binding can be used for that. >> using the assigned-clock* properties requires self-referencing the PWM >> controller (which I'm not used to), for example: >> &pwm_ab { >> #clock-cells = <1>; >> assigned-clocks = <&pwm_ab 0>, <&pwm_ab 1>; /* references itself */ >> assigned-clock-parents = <&xtal>, <&clkc CLKID_FCLK_DIV5>; >> }; >> >> if we want to auto-detect the parent clock (like you suggested below) >> we need to consider if we can detect whether a .dts-author assigned a >> specific parent. > > I (personally) don't want to keep supporting the manual assignment of the > parent. If the driver can guarantee than it will pick the most appropriate > parent, there is no reason to have that. > >> I know that it's easy to detect this when the clock is passed in the >> "clocks" property, but I'm not sure if it's easy to parse it from the >> assigned-clocks/assigned-clock-parents properties. > > Assigned parent is the poor man solution and not necessarily easier to > implement (the pwm device would have to export its own clocks) ... I have just > mentioned it to make the point that current method is not ideal > >> >> [...] >>> Last, instead of specifying the parent to be used, I think we should come up >>> with some code to let the driver pick the most appropriate parent for the period/duty requested. >> that will make it easier for .dts authors. >> I would like to postpone this until we have solved the other topics though. > > I much prefer this last solution. Since the algorithm and the bindings would > change, I think it would be easier (in DT) to just make v2 driver with a new > compatible, progressively transition dts to it and finally remove the old > driver. I'd prefer this, but to be frank, the parent only determines the precision of the PWM internal divider to give the most precise period possible. For this the highest frequency is the best. But you could also use an input clock with the lowest jitter, and this cannot be determined automatically. > > Neil
On 25/03/2019 18:41, Martin Blumenstingl wrote: > Hello Uwe, > > On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: >> >> Hello Martin, >> >> On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote: >>> Back in January a "BUG: scheduling while atomic" error showed up during >>> boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply). >>> The call trace comes down to: >>> __mutex_lock >>> clk_prepare_lock >>> clk_core_get_rate >>> meson_pwm_apply >>> .. >>> dev_pm_opp_set_rate >>> .. >>> >>> Jerome has also seen the same problem but from pwm-leds (instead of a >>> pwm-regulator). He posted a patch which replaces the spinlock with a >>> mutex. That works. I believe we can optimize this by reducing the time >>> where the lock is held - that also allows to keep the spin-lock. >>> >>> Analyzing this issue helped me understand the pwm-meson driver better. >>> My plan is to send some cleanups (with the goal of re-using more of the >>> goodies from the PWM core in the pwm-meson driver) after this single fix >>> is merged (they can be found here: [1]). >> >> I didn't look over these in detail, but I see an issue that according >> to the shortlogs isn't addressed: In the .apply callback there is >> (simplified): >> >> if (!state->enabled) { >> meson_pwm_disable(meson, pwm->hwpwm); >> return; >> } >> >> This results in the wrong output after: >> >> pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...}); >> pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...}); >> >> because the polarity isn't checked. > let me rephrase this to make sure I understand this correctly: > - applying a PWM state with .enabled = true and PWM_POLARITY_NORMAL > will enable the PWM output > - applying a PWM state with .enabled = false and PWM_POLARITY_NORMAL > will disable the PWM output > - applying a PWM state with .enabled = true and PWM_POLARITY_INVERTED > will disable the PWM output > - applying a PWM state with .enabled = false and PWM_POLARITY_INVERTED > will enable the PWM output > > in other words: the polarity doesn't only apply to period and > duty_cycle but also to the enabled state. Sorry I don't understand your point. If the apply state is disable, well we disable the PWM output, I don't see why the polarity changes the enable state. I'd like to point out the architecture of the PWM. The PWM is only a set of Gates, Dividers and Counters. We achieve a PWM output by calculating a clock that permits us to calculate 2 periods (low and high) and we set the counter to switch after N cycles for the first half period. We do not have an explicit "polarity" setting, we simply reverse the period cycles (the low length is inversed with the high length). To apply the dividers and counters, we need to disable the PWM input clock gate, set the dividers and counter and release the input gate. So yes, we should maybe stop disabling the PWM for a long period of time when we calculate the new settings, it can be fixed easily by calculating the settings and applying in a separate code path. But while re-reading the code, I can't find the issue you point at. Neil > >> If you want to implement further cleanups, my questions and propositions >> are: >> >> - Is there a publicly available manual for this hardware? If yes, you >> can add a link to it in the header of the driver. > yes, it's documented in the public S912 datasheet [0] page 542 and following > I'll add a patch which adds the link to the driver > >> - Why do you handle reparenting of the PWM's clk in .request? Wouldn't >> this be more suitable in .apply? > Jerome's answer (not long after yours) basically covers this: > - the assigned-clock-parents property could have been used but it wasn't > - the driver could automatically set the correct parent, but this > isn't implemented > > (I assume this was done to keep it short and simple to for the first > version of the driver) > >> - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB >> register) freeze the output, or is the currently running period >> completed first? (The latter is the right behaviour.) > I don't know, I would have to measure this with a logic analyzer. > can you please explain why this is important? > >> - Please point out in the header that for changing period/duty >> cycle/polarity the hardware must be stopped. (I suggest to apply the >> style used in https://www.spinics.net/lists/linux-pwm/msg09262.html >> for some consistency.) > I'm not sure about this. Amlogic's vendor kernel uses a modified > version of this driver [1] which has an explicit comment not to > disable the PWM output when changing the period/duty cycle. > the PWM is configured with two separate registers (PWM_MISC_REG_AB > contains the divider and PWM_PWM_A contains the high/low count). > there's a short timeframe where the PWM output signal is neither the > "old setting" nor the "new setting" (but rather a mix of both). what > do other PWM drivers do in this case (if this is a common thing)? > >> Another thing I just noted: The .get_state callback only sets .enabled >> but nothing of the remaining information is provided. > as far as I can see the PWM core uses .get_state only during registration: > this means we should read (and calculate) .duty_cycle and .period from > the register values. polarity always has to be "normal" since there's > no information about it in the registers. > > > Regards > Martin > > > [0] https://dl.khadas.com/Hardware/VIM2/Datasheet/S912_Datasheet_V0.220170314publicversion-Wesion.pdf > [1] https://github.com/hardkernel/linux/blob/9a4a1f4b14fe66d7ebd73323b39fbf3bda9e1356/drivers/amlogic/pwm/pwm_meson.c#L381 >
Hello, On Tue, Mar 26, 2019 at 10:06:31AM +0100, Neil Armstrong wrote: > On 25/03/2019 18:41, Martin Blumenstingl wrote: > > On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > >> On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote: > >>> Back in January a "BUG: scheduling while atomic" error showed up during > >>> boot on my Meson8b Odroid-C1 (which uses a PWM regulator as CPU supply). > >>> The call trace comes down to: > >>> __mutex_lock > >>> clk_prepare_lock > >>> clk_core_get_rate > >>> meson_pwm_apply > >>> .. > >>> dev_pm_opp_set_rate > >>> .. > >>> > >>> Jerome has also seen the same problem but from pwm-leds (instead of a > >>> pwm-regulator). He posted a patch which replaces the spinlock with a > >>> mutex. That works. I believe we can optimize this by reducing the time > >>> where the lock is held - that also allows to keep the spin-lock. > >>> > >>> Analyzing this issue helped me understand the pwm-meson driver better. > >>> My plan is to send some cleanups (with the goal of re-using more of the > >>> goodies from the PWM core in the pwm-meson driver) after this single fix > >>> is merged (they can be found here: [1]). > >> > >> I didn't look over these in detail, but I see an issue that according > >> to the shortlogs isn't addressed: In the .apply callback there is > >> (simplified): > >> > >> if (!state->enabled) { > >> meson_pwm_disable(meson, pwm->hwpwm); > >> return; > >> } > >> > >> This results in the wrong output after: > >> > >> pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...}); > >> pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...}); > >> > >> because the polarity isn't checked. > > let me rephrase this to make sure I understand this correctly: > > - applying a PWM state with .enabled = true and PWM_POLARITY_NORMAL > > will enable the PWM output > > - applying a PWM state with .enabled = false and PWM_POLARITY_NORMAL > > will disable the PWM output > > - applying a PWM state with .enabled = true and PWM_POLARITY_INVERTED > > will disable the PWM output > > - applying a PWM state with .enabled = false and PWM_POLARITY_INVERTED > > will enable the PWM output > > > > in other words: the polarity doesn't only apply to period and > > duty_cycle but also to the enabled state. > > Sorry I don't understand your point. > If the apply state is disable, well we disable the PWM output, I don't see why > the polarity changes the enable state. Martin's summary was at least misleading, I didn't understand what he meant. The relevant point is: When the PWM is disabled (either by pwm_disable or equivalent by pwm_apply_state(pwm, { .enabled = false, ... })) the expectation is that the output becomes "inactive". That means "constant low" for a normal PWM and "constant high" for an inverted PWM. Then as meson_pwm_apply doesn't check for state->polarity if state->enabled == false there is a bug. > I'd like to point out the architecture of the PWM. > The PWM is only a set of Gates, Dividers and Counters. > > We achieve a PWM output by calculating a clock that permits us to calculate > 2 periods (low and high) and we set the counter to switch after N cycles > for the first half period. > > We do not have an explicit "polarity" setting, we simply reverse the period > cycles (the low length is inversed with the high length). > > To apply the dividers and counters, we need to disable the PWM input clock > gate, set the dividers and counter and release the input gate. > > So yes, we should maybe stop disabling the PWM for a long period of time > when we calculate the new settings, it can be fixed easily by calculating > the settings and applying in a separate code path. If the hardware supports it the counter should not be stopped---most other PWMs in my bubble can at least change the duty cycle as required. (That is, complete the currently running period and that without delay start a new period with the new settings.) Best regards Uwe
Hello Uwe, On Mon, Mar 25, 2019 at 9:07 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Martin, > > On Mon, Mar 25, 2019 at 06:41:57PM +0100, Martin Blumenstingl wrote: > > On Mon, Mar 25, 2019 at 9:41 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > On Sun, Mar 24, 2019 at 11:02:16PM +0100, Martin Blumenstingl wrote: > > > > Analyzing this issue helped me understand the pwm-meson driver better. > > > > My plan is to send some cleanups (with the goal of re-using more of the > > > > goodies from the PWM core in the pwm-meson driver) after this single fix > > > > is merged (they can be found here: [1]). > > > > > > I didn't look over these in detail, but I see an issue that according > > > to the shortlogs isn't addressed: In the .apply callback there is > > > (simplified): > > > > > > if (!state->enabled) { > > > meson_pwm_disable(meson, pwm->hwpwm); > > > return; > > > } > > > > > > This results in the wrong output after: > > > > > > pwm_apply_state(pwm, { .enabled = true, .polarity = PWM_POLARITY_NORMAL, ...}); > > > pwm_apply_state(pwm, { .enabled = false, .polarity = PWM_POLARITY_INVERTED, ...}); > > > > > > because the polarity isn't checked. > > let me rephrase this to make sure I understand this correctly: > > - applying a PWM state with .enabled = true and PWM_POLARITY_NORMAL > > will enable the PWM output > > - applying a PWM state with .enabled = false and PWM_POLARITY_NORMAL > > will disable the PWM output > > - applying a PWM state with .enabled = true and PWM_POLARITY_INVERTED > > will disable the PWM output > > - applying a PWM state with .enabled = false and PWM_POLARITY_INVERTED > > will enable the PWM output > > > > in other words: the polarity doesn't only apply to period and > > duty_cycle but also to the enabled state. > > You're wrong (I think): > > - if .enabled = true, you should configure the output to repeat the > following pattern: be active for $duty_cycle ns and then inactive for > the rest of $period ns. > > - if .enabled = false, you should configure the output to be constant > inactive. > > - if .polarity = PWM_POLARITY_NORMAL we have: inactive = low, active = > high > > - if .polarity = PWM_POLARITY_INVERTED we have: inactive = high, active = > low thank you for explaining it again! now I see what you mean that we're missing the case with PWM_POLARITY_INVERTED and enabled = false: * PWM_POLARITY_INVERTED/PWM_POLARITY_NORMAL and enabled = true are managed in meson_pwm_calc() * PWM_POLARITY_NORMAL and enabled = false is managed in meson_pwm_apply() * logic for PWM_POLARITY_INVERTED and enabled = false is missing (current result: same as PWM_POLARITY_NORMAL and enabled = false which means: output is constant LOW, expected result: output is constant HIGH) > So after the two pwm_apply_state above the expectation is that the > output is constant high. But as the meson driver's apply function > doesn't check for .polarity when .enabled = false is requested the > result is probably constant low. (Unless the driver is still more broken > and doesn't ensure the output gets inactive on .disable().) I will put this on my TODO-list for my cleanups > > > If you want to implement further cleanups, my questions and propositions > > > are: > > > > > > - Is there a publicly available manual for this hardware? If yes, you > > > can add a link to it in the header of the driver. > > yes, it's documented in the public S912 datasheet [0] page 542 and following > > I'll add a patch which adds the link to the driver > > > > > - Why do you handle reparenting of the PWM's clk in .request? Wouldn't > > > this be more suitable in .apply? > > Jerome's answer (not long after yours) basically covers this: > > - the assigned-clock-parents property could have been used but it wasn't > > - the driver could automatically set the correct parent, but this > > isn't implemented > > > > (I assume this was done to keep it short and simple to for the first > > version of the driver) > > I don't know how assigned-clock-parents works, but maybe it is even > simpler to use than the hardcoding that currently is used in the driver? > > > > - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB > > > register) freeze the output, or is the currently running period > > > completed first? (The latter is the right behaviour.) > > I don't know, I would have to measure this with a logic analyzer. > > In practise you can do this with a multimeter, too. Just do something > like: > > pwm_apply_state({ .enabled = true, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL }); > pwm_apply_state({ .enabled = false, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL }); > > (assuming the PWM supports periods that long). The expectation is that > the last command takes nearly 5 s to complete and while it waits the > output is high and on return it's low. If that isn't the case, there is > a bug somewhere. thank you for explaining the test-case! I'll do the maths on the weekend and determine the longest supported period - then I'll measure this with a multimeter. > > can you please explain why this is important? > > Well, that's the semantic that the PWM API promises to its users. > Up to now this is poorly documented, there is an RFC patch waiting for > review that improves the situation. that makes sense, thank you for the explanation. > > > - Please point out in the header that for changing period/duty > > > cycle/polarity the hardware must be stopped. (I suggest to apply the > > > style used in https://www.spinics.net/lists/linux-pwm/msg09262.html > > > for some consistency.) > > I'm not sure about this. Amlogic's vendor kernel uses a modified > > version of this driver [1] which has an explicit comment not to > > disable the PWM output when changing the period/duty cycle. > > That would be better as stopping the driver also violates the API's > requirements. If this is not a hardware imposed limit, it would be great > to fix the driver accordingly. I found out that Bichao Zheng from Amlogic changed the PWM driver in the vendor kernel. He even provided useful details in the commit message (which I'll quote here for archive purposes): [0] pwm: meson: don't disable pwm when setting duty repeatedly There is an abnormally low about 20ms,when setting duty repeatedly. Because setting the duty will disable pwm and then enable.Delete this operation now. > > the PWM is configured with two separate registers (PWM_MISC_REG_AB > > contains the divider and PWM_PWM_A contains the high/low count). > > there's a short timeframe where the PWM output signal is neither the > > "old setting" nor the "new setting" (but rather a mix of both). what > > do other PWM drivers do in this case (if this is a common thing)? > > If this cannot be prevented in hardware, I think this should be > documented clearly at the top of the driver. Up to now this kind of > problem isn't (TTBOMK) well tracked and questions like "How many drivers > suffer from $problem" cannot be easily answered. > (A bit unrelated: I think the violation that disabling a PWM doesn't > complete the currently running period is a common one. But because the > corresponding questions where not asked for new driver submissions until > recently, this is untracked and probably driver authors are not even > aware of this requirement.) I will port Bichao Zheng's commit to the mainline driver and try it on one of my 32-bit Meson boards (where a PWM controlled regulator supplies the CPU cores) and one my 64-bit Meson boards (where a PWM clock is used for the 32.768kHz LPO clock for the Wifi chipset). I'm not sure if we still have a small timeframe where the clock divider gets reconfigured. However, I'm not sure if that's an actual problem for the use-cases that we have on the boards with Amlogic Meson SoCs: - maybe the controller is smart enough to read the clock frequency only when starting the PWM - for the pwm-regulator and pwm-clock use-cases we only configure the period once. thus we always get the same clock divider internally, so we don't run into issues (I still think that adding documentation for the actual behavior is good, but maybe we don't need a "fix" right now because it probably works fine anyways for our current use-cases) > > > Another thing I just noted: The .get_state callback only sets .enabled > > > but nothing of the remaining information is provided. > > as far as I can see the PWM core uses .get_state only during registration: > > this means we should read (and calculate) .duty_cycle and .period from > > the register values. polarity always has to be "normal" since there's > > no information about it in the registers. > > So you can change the polarity, but you cannot read that information > back? That's another item for the list of limitations. OK, noted (as Neil explained we swap the low/high duration in software, the hardware doesn't know about this) Regards Martin [0] https://github.com/hardkernel/linux/commit/11573cdb34ec790c3dd6f860442be4f7b4d651d5#diff-e432928020d0ce66e5bef757ba2c6f36
Hi Jerome, On Tue, Mar 26, 2019 at 9:37 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Mon, 2019-03-25 at 19:04 +0100, Martin Blumenstingl wrote: > > > Thanks for fixing this Martin. > > you're welcome! > > > > > As for the future enhancement, I'd like to know what you have in mind. > > > As I have told you previously, I think the clock bindings of this driver are > > > not great. > > > > > > The global name of the input clocks are hard coded in this driver and it > > > sucks. CCF is evolving to rely less on these global names. > > I fully agree with you on the clock setup, but I'm not sure if we have > > to break the dt-bindings for it. > > > > the datasheet notes: "Each PWM is driven by a programmable divider > > driven by a 4:1 clock selector". > > In my own words this means that each PWM controller has up to 8 clock inputs: > > - up to 4 inputs for the first channel (PWM_A) > > - up to 4 inputs for the second channel (PWM_B) > > Not from the pwm device POV. there is one device (PWM_AB) with 4 (max) input > clocks. Those are consumed by two internal muxes. There would be 8 if the > input was different between path A and B. > > > > > the current pwm-meson driver assumes that both the inputs for both > > channels are identical. > > the "clock trees" section of the public S912 datasheet (page 65) > > clearly documents the clock inputs per PWM channel, not per PWM > > controller. > > > > Thus I believe we should name our clock-names (the inputs to the PWM > > controller) "pwm-a-clkin0", "pwm-a-clkin1", "pwm-b-clkin0", ... > > That way we don't have a conflict with the existing bindings (which > > already reserve "clkin0" and "clkin1"). > > I think this is overkill an inaccurate. The experience of all the soc we have > seen so far (meson8, gxbb, gxl, gxm, axg and g12) confirms the sources the are > the same input clock for both path. > > The documentation just shows the clock src of each pwm. That just how the the > table is presented. That does not change the fact the pwms are organized in > modules (pairs) and the that the clock source are the same for each pwm of the > module. IOW, there is only 4 lines of clocks getting to the IP, not 8. Feel > free to ask amlogic if you want to make sure. interesting, the way you describe it also seems valid. I don't want to implement either way and find out that we have to change the bindings again later on. Bichao, Jianxin, can you answer the question whether each PWM controller has: - up to four clock inputs. clock parents for channels PWM_A and PWM_B are always the same - up to eight clock inputs. four parents for channel PWM_A and four more parents for PWM_B, where the parents of PWM_A and PWM_B could be different (for all known SoCs the parents of the channels PWM_A and PWM_B are identical, so we don't know how the hardware is modelled) > The name clash is not really my point. The purpose of the clock binding would > be different (from stating a setting to describing hw connection) OK, so your main concern is that we're breaking the DT bindings - so instead of coming up with a "quick fix" we might as well take the long but proper route? > > > > > In addition, the 'clock' binding should be used to refer to the clock > > > 'consumed' by the device, not to define a setting (as done now). 'assigned- > > > clock' binding can be used for that. > > using the assigned-clock* properties requires self-referencing the PWM > > controller (which I'm not used to), for example: > > &pwm_ab { > > #clock-cells = <1>; > > assigned-clocks = <&pwm_ab 0>, <&pwm_ab 1>; /* references itself */ > > assigned-clock-parents = <&xtal>, <&clkc CLKID_FCLK_DIV5>; > > }; > > > > if we want to auto-detect the parent clock (like you suggested below) > > we need to consider if we can detect whether a .dts-author assigned a > > specific parent. > > I (personally) don't want to keep supporting the manual assignment of the > parent. If the driver can guarantee than it will pick the most appropriate > parent, there is no reason to have that. > > > I know that it's easy to detect this when the clock is passed in the > > "clocks" property, but I'm not sure if it's easy to parse it from the > > assigned-clocks/assigned-clock-parents properties. > > Assigned parent is the poor man solution and not necessarily easier to > implement (the pwm device would have to export its own clocks) ... I have just > mentioned it to make the point that current method is not ideal > > > > > [...] > > > Last, instead of specifying the parent to be used, I think we should come up > > > with some code to let the driver pick the most appropriate parent for the period/duty requested. > > that will make it easier for .dts authors. > > I would like to postpone this until we have solved the other topics though. > > I much prefer this last solution. Since the algorithm and the bindings would > change, I think it would be easier (in DT) to just make v2 driver with a new > compatible, progressively transition dts to it and finally remove the old > driver. Neil has already raised the question whether clock jitter may be relevant. what also came to my mind is the "system suspend" use-case: I'm not sure if the fixed_pll (and it's post-dividers) are enabled during system suspend. in that case we probably need clock the PWM channel off the main XTAL. (I don't know if this use-case really exists, I want to bring it up so we can discuss it) Regards Martin
Hello Uwe, On Mon, Mar 25, 2019 at 9:07 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: [...] > > > - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB > > > register) freeze the output, or is the currently running period > > > completed first? (The latter is the right behaviour.) > > I don't know, I would have to measure this with a logic analyzer. > > In practise you can do this with a multimeter, too. Just do something > like: > > pwm_apply_state({ .enabled = true, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL }); > pwm_apply_state({ .enabled = false, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL }); > > (assuming the PWM supports periods that long). The expectation is that > the last command takes nearly 5 s to complete and while it waits the > output is high and on return it's low. If that isn't the case, there is > a bug somewhere. the longest supported period (using the 24MHz crystal as input, which is the slowest input clock and thus gives the longest possible duration) is 349514407ns (that's approx. 0.35 seconds). my multimeter isn't fast enough to measure this so I'm using my logic analyzer with puleseview instead: [0] I added the following code to meson_pwm_request: struct pwm_state enable = { .enabled = true, .period = 349514407U, .duty_cycle = 349514407U, .polarity = PWM_POLARITY_NORMAL }; struct pwm_state disable = { .enabled = false, .period = 349514407U, .duty_cycle = 349514407U, .polarity = PWM_POLARITY_NORMAL }; pwm_apply_state(pwm, &enable); pwm_apply_state(pwm, &disable); this returns immediately. my logic analyzer doesn't see signal change (I'm sampling at 1MHz). can you please confirm that my test code and measurement procedure is correct? if it is then my observation is that disabling the PWM does so immediately, without waiting for the current period to complete Regards Martin [0] https://sigrok.org/wiki/Lcsoft_Mini_Board
On Sat, Mar 30, 2019 at 08:29:35PM +0100, Martin Blumenstingl wrote: > Hello Uwe, > > On Mon, Mar 25, 2019 at 9:07 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > [...] > > > > - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB > > > > register) freeze the output, or is the currently running period > > > > completed first? (The latter is the right behaviour.) > > > I don't know, I would have to measure this with a logic analyzer. > > > > In practise you can do this with a multimeter, too. Just do something > > like: > > > > pwm_apply_state({ .enabled = true, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL }); > > pwm_apply_state({ .enabled = false, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL }); > > > > (assuming the PWM supports periods that long). The expectation is that > > the last command takes nearly 5 s to complete and while it waits the > > output is high and on return it's low. If that isn't the case, there is > > a bug somewhere. > the longest supported period (using the 24MHz crystal as input, which > is the slowest input clock and thus gives the longest possible > duration) is 349514407ns (that's approx. 0.35 seconds). my multimeter > isn't fast enough to measure this so I'm using my logic analyzer with > puleseview instead: [0] > > I added the following code to meson_pwm_request: > struct pwm_state enable = { > .enabled = true, > .period = 349514407U, > .duty_cycle = 349514407U, > .polarity = PWM_POLARITY_NORMAL }; > struct pwm_state disable = { > .enabled = false, > .period = 349514407U, > .duty_cycle = 349514407U, > .polarity = PWM_POLARITY_NORMAL }; > pwm_apply_state(pwm, &enable); > pwm_apply_state(pwm, &disable); > > this returns immediately. my logic analyzer doesn't see signal change > (I'm sampling at 1MHz). > > can you please confirm that my test code and measurement procedure is correct? > if it is then my observation is that disabling the PWM does so > immediately, without waiting for the current period to complete Ack, with the above two pwm_apply_state the output must be high when the first pwm_apply_state returns. Then it must stay high for n * 349514407 ns (for an natural n >= 1) and then go low. Best regards Uwe
On 30/03/2019 20:29, Martin Blumenstingl wrote: > Hello Uwe, > > On Mon, Mar 25, 2019 at 9:07 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > [...] >>>> - Does stopping the PWM (i.e. clearing MISC_{A,B}_EN in the MISC_AB >>>> register) freeze the output, or is the currently running period >>>> completed first? (The latter is the right behaviour.) >>> I don't know, I would have to measure this with a logic analyzer. >> >> In practise you can do this with a multimeter, too. Just do something >> like: >> >> pwm_apply_state({ .enabled = true, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL }); >> pwm_apply_state({ .enabled = false, .period = 5s, .duty_cycle = 5s, .polarity = PWM_POLARITY_NORMAL }); >> >> (assuming the PWM supports periods that long). The expectation is that >> the last command takes nearly 5 s to complete and while it waits the >> output is high and on return it's low. If that isn't the case, there is >> a bug somewhere. > the longest supported period (using the 24MHz crystal as input, which > is the slowest input clock and thus gives the longest possible > duration) is 349514407ns (that's approx. 0.35 seconds). my multimeter > isn't fast enough to measure this so I'm using my logic analyzer with > puleseview instead: [0] > > I added the following code to meson_pwm_request: > struct pwm_state enable = { > .enabled = true, > .period = 349514407U, > .duty_cycle = 349514407U, > .polarity = PWM_POLARITY_NORMAL }; > struct pwm_state disable = { > .enabled = false, > .period = 349514407U, > .duty_cycle = 349514407U, > .polarity = PWM_POLARITY_NORMAL }; > pwm_apply_state(pwm, &enable); > pwm_apply_state(pwm, &disable); > > this returns immediately. my logic analyzer doesn't see signal change > (I'm sampling at 1MHz). > > can you please confirm that my test code and measurement procedure is correct? > if it is then my observation is that disabling the PWM does so > immediately, without waiting for the current period to complete I'm pretty 100,00000000000000% sure the HW doesn't permit waiting for a period to finish. For disable states, we can either play on the period (high period at 0xffff for disable at PWM_POLARITY_INVERSED and high period at 0 for PWM_POLARITY_NORMAL, if the HW behaves correctly) or by adding some pinctrl states switching to GPIO modes and adding the enable output high and output low property. Neil > > > Regards > Martin > > > [0] https://sigrok.org/wiki/Lcsoft_Mini_Board >