From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> To: Claudiu.Beznea@microchip.com Cc: mark.rutland@arm.com, linux-pwm@vger.kernel.org, alexandre.belloni@bootlin.com, shc_work@mail.ru, corbet@lwn.net, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, robh+dt@kernel.org, thierry.reding@gmail.com, linux-arm-kernel@lists.infradead.org Subject: Re: [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes Date: Fri, 26 Oct 2018 14:56:45 +0200 [thread overview] Message-ID: <20181026125644.ivfw6xefc7wqt2vw@pengutronix.de> (raw) In-Reply-To: <f19251eb-80e1-aea1-2d8a-971fe53242d0@microchip.com> Hello Claudiu, On Fri, Oct 26, 2018 at 10:44:43AM +0000, Claudiu.Beznea@microchip.com wrote: > Thank you for your inputs and sorry for the late response. Please see my > answers inline. No problems, I didn't held my breath :-) > On 22.10.2018 11:29, Uwe Kleine-König wrote: > > On Tue, Aug 28, 2018 at 04:01:17PM +0300, Claudiu Beznea wrote: > >> Since PWMs with more than one output per channel could be used as one > >> output PWM the normal mode is the default mode for all PWMs (if not > >> specified otherwise). > >> > >> Complementary mode - for PWM channels with two outputs; output waveforms > >> for a PWM channel in complementary mode looks line this: > >> __ __ __ __ > >> PWMH1 __| |__| |__| |__| |__ > >> __ __ __ __ __ > >> PWML1 |__| |__| |__| |__| > >> <--T--> > >> > >> Where T is the signal period. > > > > So this translates to (I think): > > > > __ __ __ __ __ > > PWMH __/ \_____/ \_____/ \_____/ \_____/ \_____/ > > __ _____ _____ _____ _____ _____ > > PWML \__/ \__/ \__/ \__/ \__/ \ > > ^ ^ ^ ^ ^ ^ > > > > That is PWML always pulls in the opposite direction of PWMH. Maybe we > > could come up with better terms than PWMH and PWML (which might be > > specific for the Atmel implementation?). > > Yes, this is Atmel implementation naming. > > > Maybe "normal" and > > "complement"? > > I will think about it try to come with new naming. Normal and Complement > may be confusing for users with regards to PWM modes. > > > > >> Push-pull mode - for PWM channels with two outputs; output waveforms for a > >> PWM channel in push-pull mode with normal polarity looks like this: > >> __ __ > >> PWMH __| |________| |________ > >> __ __ > >> PWML ________| |________| |__ > >> <--T--> > > > > That again with the alternative display method and duty cycle 1/3: > > > > __ __ __ > > PWMA __/ \______________/ \______________/ \______ > > __ __ > > PWMB ___________/ \______________/ \______________/ > > ^ ^ ^ ^ ^ ^ > Ok. > > > That is PWMA and PWMB are active only every 2nd period taking alternate > > turns, right? > > Yes. > > > > > > >> If polarity is inversed: > >> __ ________ ________ > >> PWMH |__| |__| > >> ________ ________ __ > >> PWML |__| |__| > >> <--T--> > > > > That's again with duty cycle 1/3: > > > > __ ______________ ______________ ______ > > PWMA \__/ \__/ \__/ > > ___________ ______________ ______________ > > PWMB \__/ \__/ \ > > ^ ^ ^ ^ ^ ^ > > > > Ok. > > > Given that the start of period isn't externally visible this is > > equivalent to using a duty cycle 2/3 and not inverting which results in: > > > > __ ______________ ______________ ______ > > PWMA \__/ \__/ \__/ > > ___________ ______________ ______________ > > PWMB \__/ \__/ \ > > ^ ^ ^ ^ ^ > > > > I would really like if a more detailed description of the modes would be > > created as part of this series. > > Sure, I will try to document it better. Note here I just leaked my belief that the PWM framework shouldn't necessarily expose inversion to users which is part of my discussion with Thierry. I think it would be sensible to drop it here. > > Currently there are a few implied > > properties hidden in the PWM API (unrelated to this series) which I try > > to resolve together with Thierry. Getting this documented right from the > > start would be great here. > > Could you tell me if you want something specific to be touch as part of > documentation process for these PWM modes? At least having something about the expectations in Documenation/ would be great. > > I didn't look in detail into the driver implementation, but from the > > PWMs implemented in the STM32F4 family I would have chosen a different > > model which makes me wonder if we should stick to a more general way to > > describe two outputs from a single PWM channel. > > > > I would use four values with nanosecond resolution to describe these: > > > > .period > > .duty_cycle > > .alt_duty_cycle > > .alt_offset > > > > period and duty_cycle is as before for the primary output and then the > > alt_* values describe offset and duty cycle of the secondary output. > > > > What you called "normal mode" would then be specified using > > > > .period = $period > > .duty_cycle = $duty_cycle > > .alt_duty_cycle = 0 > > .alt_offset = dontcare > > > > Your "push pull mode" would be: > > > > .period = 2 * $period > > .duty_cycle = $duty_cycle > > .alt_duty_cycle = $duty_cycle > > .alt_offset = $period > > > > and complementary mode would be specified using: > > > > .period = $period > > .duty_cycle = $duty_cycle > > .alt_duty_cycle = $period - $duty_cycle > > .alt_offset = $duty_cycle > > > > On Atmel PWM controller the push-pull mode is hardware generated based on > period and duty cycles that are setup for only one channel. The hardware > will take care of the synchronization b/w the outputs so that the push-pull > characteristic to be generated. > > Having different configuration for every output part of the push-pull > waveform will allow users to generate every kind of outputs. But for IPs > that are capable of push-pull or complementary modes the generation of the > 2 outputs are done in hardware (true in case of Atmel PWM controller). In > case of STM32F4 as far as I can see from [1] "The advanced-control timers > (TIM1 and TIM8 ) can output two complementary signals and > manage the switching-off and the switching-on instants of the outputs." > Maybe, in this case, if there are 2 hardware blocks that could be synced to > work together, e.g. in complementary mode, the setting of these two timers > should be done in driver so that the hardware blocks to be configured > together, atomically, so that the complementary characteristics to be obtained. The upside I see in my approach with .alt_duty_cycle and .alt_offset over your .mode is that it allows to describe more use-cases. If I wanted to support complementary with dead-time I'd need another member in pwm_state to specify the dead time. Then the next controller can only add dead time on one end of secondary output needing yet another mode enum. With my approach I think you can specify all sensible(TM) waveforms already now. Then a driver must not be adapted again when someone adds support for another mode. The downside is that if your PWM only supports complementary mode with no dead-time you have to find out from .alt_duty_cycle and .alt_offset that the specified wave form is indeed matching complementary mode. From from the framework's POV this is more elegant though (but YMMV). > > With this abstraction stuff like "complementary output with dead-time > > insertion" (something like: > > > > __ __ __ > > PWMA __/ \______________/ \______________/ \______ > > __________ __________ __ > > PWMB _______/ \______/ \______/ > > ^ ^ ^ > > > > ) could be modelled. > > Same for this, my opinion is that we should implement generic things in > core and drivers should configure properly the IP so that it generates the > proper signals. This is common to both our approaches. Just the way the PWM user specifies his/her wishes (and so the input for hw drivers) is different. > >> The PWM working modes are per PWM channel registered as PWM's capabilities. > >> The driver registers itself to PWM core a get_caps() function, in > >> struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities. > >> If this function is not registered in driver's probe, a default function > >> will be used to retrieve PWM capabilities. Currently, the default > >> capabilities includes only PWM normal mode. > > > > In the i2c framework this is a function, too, and I wonder if simplicity > > is better served when this is just a flags member in the pwm_ops > > structure. > > Thierry proposed this so that we could retrieve capabilities per PWM channel. I don't have a complete overview over the different hardware implementations, but I'd expect that if two different implementations share the operations then the return value of .get_caps is shared by both. As long this is the case not introducing a callback for that is the easier path. Adding a callback later when (and if) this is required is trivial then. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
WARNING: multiple messages have this Message-ID (diff)
From: u.kleine-koenig@pengutronix.de (Uwe Kleine-König) To: linux-arm-kernel@lists.infradead.org Subject: [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes Date: Fri, 26 Oct 2018 14:56:45 +0200 [thread overview] Message-ID: <20181026125644.ivfw6xefc7wqt2vw@pengutronix.de> (raw) In-Reply-To: <f19251eb-80e1-aea1-2d8a-971fe53242d0@microchip.com> Hello Claudiu, On Fri, Oct 26, 2018 at 10:44:43AM +0000, Claudiu.Beznea at microchip.com wrote: > Thank you for your inputs and sorry for the late response. Please see my > answers inline. No problems, I didn't held my breath :-) > On 22.10.2018 11:29, Uwe Kleine-K?nig wrote: > > On Tue, Aug 28, 2018 at 04:01:17PM +0300, Claudiu Beznea wrote: > >> Since PWMs with more than one output per channel could be used as one > >> output PWM the normal mode is the default mode for all PWMs (if not > >> specified otherwise). > >> > >> Complementary mode - for PWM channels with two outputs; output waveforms > >> for a PWM channel in complementary mode looks line this: > >> __ __ __ __ > >> PWMH1 __| |__| |__| |__| |__ > >> __ __ __ __ __ > >> PWML1 |__| |__| |__| |__| > >> <--T--> > >> > >> Where T is the signal period. > > > > So this translates to (I think): > > > > __ __ __ __ __ > > PWMH __/ \_____/ \_____/ \_____/ \_____/ \_____/ > > __ _____ _____ _____ _____ _____ > > PWML \__/ \__/ \__/ \__/ \__/ \ > > ^ ^ ^ ^ ^ ^ > > > > That is PWML always pulls in the opposite direction of PWMH. Maybe we > > could come up with better terms than PWMH and PWML (which might be > > specific for the Atmel implementation?). > > Yes, this is Atmel implementation naming. > > > Maybe "normal" and > > "complement"? > > I will think about it try to come with new naming. Normal and Complement > may be confusing for users with regards to PWM modes. > > > > >> Push-pull mode - for PWM channels with two outputs; output waveforms for a > >> PWM channel in push-pull mode with normal polarity looks like this: > >> __ __ > >> PWMH __| |________| |________ > >> __ __ > >> PWML ________| |________| |__ > >> <--T--> > > > > That again with the alternative display method and duty cycle 1/3: > > > > __ __ __ > > PWMA __/ \______________/ \______________/ \______ > > __ __ > > PWMB ___________/ \______________/ \______________/ > > ^ ^ ^ ^ ^ ^ > Ok. > > > That is PWMA and PWMB are active only every 2nd period taking alternate > > turns, right? > > Yes. > > > > > > >> If polarity is inversed: > >> __ ________ ________ > >> PWMH |__| |__| > >> ________ ________ __ > >> PWML |__| |__| > >> <--T--> > > > > That's again with duty cycle 1/3: > > > > __ ______________ ______________ ______ > > PWMA \__/ \__/ \__/ > > ___________ ______________ ______________ > > PWMB \__/ \__/ \ > > ^ ^ ^ ^ ^ ^ > > > > Ok. > > > Given that the start of period isn't externally visible this is > > equivalent to using a duty cycle 2/3 and not inverting which results in: > > > > __ ______________ ______________ ______ > > PWMA \__/ \__/ \__/ > > ___________ ______________ ______________ > > PWMB \__/ \__/ \ > > ^ ^ ^ ^ ^ > > > > I would really like if a more detailed description of the modes would be > > created as part of this series. > > Sure, I will try to document it better. Note here I just leaked my belief that the PWM framework shouldn't necessarily expose inversion to users which is part of my discussion with Thierry. I think it would be sensible to drop it here. > > Currently there are a few implied > > properties hidden in the PWM API (unrelated to this series) which I try > > to resolve together with Thierry. Getting this documented right from the > > start would be great here. > > Could you tell me if you want something specific to be touch as part of > documentation process for these PWM modes? At least having something about the expectations in Documenation/ would be great. > > I didn't look in detail into the driver implementation, but from the > > PWMs implemented in the STM32F4 family I would have chosen a different > > model which makes me wonder if we should stick to a more general way to > > describe two outputs from a single PWM channel. > > > > I would use four values with nanosecond resolution to describe these: > > > > .period > > .duty_cycle > > .alt_duty_cycle > > .alt_offset > > > > period and duty_cycle is as before for the primary output and then the > > alt_* values describe offset and duty cycle of the secondary output. > > > > What you called "normal mode" would then be specified using > > > > .period = $period > > .duty_cycle = $duty_cycle > > .alt_duty_cycle = 0 > > .alt_offset = dontcare > > > > Your "push pull mode" would be: > > > > .period = 2 * $period > > .duty_cycle = $duty_cycle > > .alt_duty_cycle = $duty_cycle > > .alt_offset = $period > > > > and complementary mode would be specified using: > > > > .period = $period > > .duty_cycle = $duty_cycle > > .alt_duty_cycle = $period - $duty_cycle > > .alt_offset = $duty_cycle > > > > On Atmel PWM controller the push-pull mode is hardware generated based on > period and duty cycles that are setup for only one channel. The hardware > will take care of the synchronization b/w the outputs so that the push-pull > characteristic to be generated. > > Having different configuration for every output part of the push-pull > waveform will allow users to generate every kind of outputs. But for IPs > that are capable of push-pull or complementary modes the generation of the > 2 outputs are done in hardware (true in case of Atmel PWM controller). In > case of STM32F4 as far as I can see from [1] "The advanced-control timers > (TIM1 and TIM8 ) can output two complementary signals and > manage the switching-off and the switching-on instants of the outputs." > Maybe, in this case, if there are 2 hardware blocks that could be synced to > work together, e.g. in complementary mode, the setting of these two timers > should be done in driver so that the hardware blocks to be configured > together, atomically, so that the complementary characteristics to be obtained. The upside I see in my approach with .alt_duty_cycle and .alt_offset over your .mode is that it allows to describe more use-cases. If I wanted to support complementary with dead-time I'd need another member in pwm_state to specify the dead time. Then the next controller can only add dead time on one end of secondary output needing yet another mode enum. With my approach I think you can specify all sensible(TM) waveforms already now. Then a driver must not be adapted again when someone adds support for another mode. The downside is that if your PWM only supports complementary mode with no dead-time you have to find out from .alt_duty_cycle and .alt_offset that the specified wave form is indeed matching complementary mode. From from the framework's POV this is more elegant though (but YMMV). > > With this abstraction stuff like "complementary output with dead-time > > insertion" (something like: > > > > __ __ __ > > PWMA __/ \______________/ \______________/ \______ > > __________ __________ __ > > PWMB _______/ \______/ \______/ > > ^ ^ ^ > > > > ) could be modelled. > > Same for this, my opinion is that we should implement generic things in > core and drivers should configure properly the IP so that it generates the > proper signals. This is common to both our approaches. Just the way the PWM user specifies his/her wishes (and so the input for hw drivers) is different. > >> The PWM working modes are per PWM channel registered as PWM's capabilities. > >> The driver registers itself to PWM core a get_caps() function, in > >> struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities. > >> If this function is not registered in driver's probe, a default function > >> will be used to retrieve PWM capabilities. Currently, the default > >> capabilities includes only PWM normal mode. > > > > In the i2c framework this is a function, too, and I wonder if simplicity > > is better served when this is just a flags member in the pwm_ops > > structure. > > Thierry proposed this so that we could retrieve capabilities per PWM channel. I don't have a complete overview over the different hardware implementations, but I'd expect that if two different implementations share the operations then the return value of .get_caps is shared by both. As long this is the case not introducing a callback for that is the easier path. Adding a callback later when (and if) this is required is trivial then. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
next prev parent reply other threads:[~2018-10-26 12:56 UTC|newest] Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-08-28 13:01 [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` [RESEND PATCH v5 1/9] pwm: extend PWM framework with " Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-10-16 12:25 ` Thierry Reding 2018-10-16 12:25 ` Thierry Reding 2018-10-17 12:42 ` Claudiu.Beznea 2018-10-17 12:42 ` Claudiu.Beznea at microchip.com 2018-10-17 12:42 ` Claudiu.Beznea 2018-10-18 15:32 ` Thierry Reding 2018-10-18 15:32 ` Thierry Reding 2018-10-19 11:18 ` Claudiu Beznea 2018-10-19 11:18 ` Claudiu Beznea 2018-10-19 11:18 ` Claudiu Beznea 2018-08-28 13:01 ` [RESEND PATCH v5 2/9] pwm: clps711x: populate PWM mode in of_xlate function Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` [RESEND PATCH v5 3/9] pwm: cros-ec: " Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` [RESEND PATCH v5 4/9] pwm: pxa: " Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` [RESEND PATCH v5 5/9] pwm: add PWM modes Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 22:27 ` Rob Herring 2018-08-28 22:27 ` Rob Herring 2018-08-28 13:01 ` [RESEND PATCH v5 6/9] pwm: atmel: add pwm capabilities Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` [RESEND PATCH v5 7/9] pwm: add push-pull mode support Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` [RESEND PATCH v5 8/9] pwm: add documentation for pwm push-pull mode Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-10-12 12:15 ` Thierry Reding 2018-10-12 12:15 ` Thierry Reding 2018-10-12 13:05 ` Claudiu.Beznea 2018-10-12 13:05 ` Claudiu.Beznea at microchip.com 2018-10-12 13:05 ` Claudiu.Beznea 2018-08-28 13:01 ` [RESEND PATCH v5 9/9] pwm: atmel: add push-pull mode support Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-08-28 13:01 ` Claudiu Beznea 2018-09-14 16:20 ` [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes Nicolas Ferre 2018-09-14 16:20 ` Nicolas Ferre 2018-09-14 16:20 ` Nicolas Ferre 2018-10-03 12:49 ` Nicolas Ferre 2018-10-03 12:49 ` Nicolas Ferre 2018-10-03 12:49 ` Nicolas Ferre 2018-10-16 12:03 ` Thierry Reding 2018-10-16 12:03 ` Thierry Reding 2018-10-17 12:41 ` Claudiu.Beznea 2018-10-17 12:41 ` Claudiu.Beznea at microchip.com 2018-10-17 12:41 ` Claudiu.Beznea 2018-10-18 16:00 ` Thierry Reding 2018-10-18 16:00 ` Thierry Reding 2018-10-19 11:18 ` Claudiu Beznea 2018-10-19 11:18 ` Claudiu Beznea 2018-10-19 11:18 ` Claudiu Beznea 2018-10-22 8:29 ` Uwe Kleine-König 2018-10-22 8:29 ` Uwe Kleine-König 2018-10-26 10:44 ` Claudiu.Beznea 2018-10-26 10:44 ` Claudiu.Beznea at microchip.com 2018-10-26 10:44 ` Claudiu.Beznea 2018-10-26 12:56 ` Uwe Kleine-König [this message] 2018-10-26 12:56 ` Uwe Kleine-König
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20181026125644.ivfw6xefc7wqt2vw@pengutronix.de \ --to=u.kleine-koenig@pengutronix.de \ --cc=Claudiu.Beznea@microchip.com \ --cc=alexandre.belloni@bootlin.com \ --cc=corbet@lwn.net \ --cc=devicetree@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=robh+dt@kernel.org \ --cc=shc_work@mail.ru \ --cc=thierry.reding@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.