Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1316964 > unrolled thread
| Started by | Doug Anderson <dianders@google.com> |
|---|---|
| First post | 2016-01-25 17:30 +0100 |
| Last post | 2016-01-25 20:00 +0100 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v3 00/12] pwm: add support for atomic update Doug Anderson <dianders@google.com> - 2016-01-25 17:30 +0100
Re: [PATCH v3 00/12] pwm: add support for atomic update Thierry Reding <thierry.reding@gmail.com> - 2016-01-25 18:20 +0100
Re: [PATCH v3 00/12] pwm: add support for atomic update Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-01-25 19:00 +0100
Re: [PATCH v3 00/12] pwm: add support for atomic update Doug Anderson <dianders@google.com> - 2016-01-25 20:00 +0100
| From | Doug Anderson <dianders@google.com> |
|---|---|
| Date | 2016-01-25 17:30 +0100 |
| Subject | Re: [PATCH v3 00/12] pwm: add support for atomic update |
| Message-ID | <qUSbE-6CP-41@gated-at.bofh.it> |
Thierry and Boris, On Tue, Nov 10, 2015 at 9:34 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Oct 19, 2015 at 12:12:12PM +0200, Heiko Stübner wrote: >> Hi Thierry, >> >> Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon: >> > Hello, >> > >> > This series adds support for atomic PWM update, or IOW, the capability >> > to update all the parameters of a PWM device (enabled/disabled, period, >> > duty and polarity) in one go. >> >> is anything more blocking this series? It's now sitting on the lists for >> nearly a month and everybody seems happy with it, so it would be really nice >> to have in mainline :-) . >> >> Especially as this also makes it possible for Rockchip Chromebooks to actually >> control the logic-regulator that is implemented as pwm-regulator there. > > Last time I tried to put this into linux-next I got immediately > bombarded by a number of build failures, so I backed things out. The > current plan is to give this another try after v4.4-rc1. We're now into the 4.5 timeframe. Does anyone have a concrete set of things that need to happen before this patch series makes it into mainline? From searching I see that the latest version of this series is v4 and there are a smattering of comments on the 24-patch series. Presumably a v5 needs to be posted to address those things. ...but it looks like the big sticking point is that Boris is waiting for a response to his questions in <https://patchwork.kernel.org/patch/7622881/>. Thierry: can you give Boris some direction for what else he needs to do? We need to come up with _some_ solution since this series gets us much better support for PWM regulators. Without this series or some other solution, PWM regulators aren't usable in mainline on any system that uses them for system-critical rails. Nearly all Rockchip reference boards and shipping devices uses a PWM regulator for the system-critidal "logic" rail. That means any patches which need to change this rail in Linux are blocked. If there's already been off-list discussion and Boris already knows what the next steps are then my apologies and I'll wait patiently for the next series. ;) Thanks! -Doug
[toc] | [next] | [standalone]
| From | Thierry Reding <thierry.reding@gmail.com> |
|---|---|
| Date | 2016-01-25 18:20 +0100 |
| Message-ID | <qUSY4-7hD-55@gated-at.bofh.it> |
| In reply to | #1316964 |
[Multipart message — attachments visible in raw view] — view raw
On Mon, Jan 25, 2016 at 08:28:31AM -0800, Doug Anderson wrote: > Thierry and Boris, > > On Tue, Nov 10, 2015 at 9:34 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Mon, Oct 19, 2015 at 12:12:12PM +0200, Heiko Stübner wrote: > >> Hi Thierry, > >> > >> Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon: > >> > Hello, > >> > > >> > This series adds support for atomic PWM update, or IOW, the capability > >> > to update all the parameters of a PWM device (enabled/disabled, period, > >> > duty and polarity) in one go. > >> > >> is anything more blocking this series? It's now sitting on the lists for > >> nearly a month and everybody seems happy with it, so it would be really nice > >> to have in mainline :-) . > >> > >> Especially as this also makes it possible for Rockchip Chromebooks to actually > >> control the logic-regulator that is implemented as pwm-regulator there. > > > > Last time I tried to put this into linux-next I got immediately > > bombarded by a number of build failures, so I backed things out. The > > current plan is to give this another try after v4.4-rc1. > > We're now into the 4.5 timeframe. Does anyone have a concrete set of > things that need to happen before this patch series makes it into > mainline? I think the current status is that we're more or less blocked on the decision on what the reset state of the PWM should be. The question is what to do if the PWM hardware readout differs from the settings found in DT. > From searching I see that the latest version of this series is v4 and > there are a smattering of comments on the 24-patch series. Presumably > a v5 needs to be posted to address those things. > > ...but it looks like the big sticking point is that Boris is waiting > for a response to his questions in > <https://patchwork.kernel.org/patch/7622881/>. Thierry: can you give > Boris some direction for what else he needs to do? We need to come up > with _some_ solution since this series gets us much better support for > PWM regulators. Without this series or some other solution, PWM > regulators aren't usable in mainline on any system that uses them for > system-critical rails. Nearly all Rockchip reference boards and > shipping devices uses a PWM regulator for the system-critidal "logic" > rail. That means any patches which need to change this rail in Linux > are blocked. I really don't understand this design decision. I presume that the PWM controlling this system-critical logic is driven by the SoC? So if the regulator is system-critical, doesn't that make it a chicken and egg problem? How can the SoC turn the PWM on if it doesn't have power? But perhaps I'm completely misunderstanding what you're saying. Perhaps if somebody could summarize how exactly this works, it would help better understand the requirements or what's the correct thing to do. > If there's already been off-list discussion and Boris already knows > what the next steps are then my apologies and I'll wait patiently for > the next series. ;) I don't think we reached a conclusion on this. And to be honest, I'm not sure what the right way forward is in this situation. So in order to make some forward progress I suggest we start a discussion, hopefully that will clarify the situation and help lead to the conclusion. Let me recap where we are: Boris' series has two goals: 1) allow seamless hand-off from firmware to kernel of a PWM channel and 2) apply changes to a regulator in a single atomic operation. To achieve this the concept of PWM state is introduced which encapsulates the settings of a PWM channel. On driver probe the current state will be read from hardware and when one or more parameters are to be changed, the current state is duplicated, the new values set in the state and the new state applied. The problem that we've encountered is that since the PWM parameters are specified in DT (or board files), there is the possibility of the PWM hardware state and the board parameters disagreeing. To resolve such situations there must be a point in time where both hardware state and software state must be synchronized. Now the most straightforward way to do that would be to simply apply the software state and be done with it. However the software state initially lacks the duty cycle because it is a parameter that usually depends on the use-case (for backlight for instance it controls the brightness, for regulators it controls the output voltage, ...). Applying the software state as-is also means that there's no reason at all to read out the hardware state in the first place, because it will simply be discarded. An alternative would be to discard the software state and trust the hardware to be configured correctly. That's somewhat risky because we don't know if the hardware is properly configured. Or Linux might have different requirements from the firmware and hence needs to configure the PWM differently. Neither of the above are very attractive options. The best I've been able to come up with so far is to completely remove this decision from the PWM subsystem and let users handle this. That is, a PWM regulator driver would have to have all the knowledge about how to configure the PWM for its needs. So upon probe, the PWM regulator driver would inspect the current state of the PWM and adjust if necessary, then apply again. Ideally of course it wouldn't have to do anything because the hardware PWM state would match the software configuration. The idea here is that the PWM regulator driver knows exactly what duty cycle to configure to obtain the desired output voltage. That doesn't really get us closer, though. There is still the issue of the user having to deal with two states: the current hardware state and the software state as configured in DT or board files. Like I said, I'm on the fence about this, so I'd appreciate any comments and perhaps insight from user subsystem maintainers on how they'd like this to look, or how this has been done with other resources (GPIOs, ...?) Thierry
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2016-01-25 19:00 +0100 |
| Message-ID | <qUTAL-7zc-21@gated-at.bofh.it> |
| In reply to | #1317082 |
Hi Thierry, On Mon, 25 Jan 2016 18:08:55 +0100 Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Jan 25, 2016 at 08:28:31AM -0800, Doug Anderson wrote: > > Thierry and Boris, > > > > On Tue, Nov 10, 2015 at 9:34 AM, Thierry Reding > > <thierry.reding@gmail.com> wrote: > > > On Mon, Oct 19, 2015 at 12:12:12PM +0200, Heiko Stübner wrote: > > >> Hi Thierry, > > >> > > >> Am Montag, 21. September 2015, 11:33:17 schrieb Boris Brezillon: > > >> > Hello, > > >> > > > >> > This series adds support for atomic PWM update, or IOW, the capability > > >> > to update all the parameters of a PWM device (enabled/disabled, period, > > >> > duty and polarity) in one go. > > >> > > >> is anything more blocking this series? It's now sitting on the lists for > > >> nearly a month and everybody seems happy with it, so it would be really nice > > >> to have in mainline :-) . > > >> > > >> Especially as this also makes it possible for Rockchip Chromebooks to actually > > >> control the logic-regulator that is implemented as pwm-regulator there. > > > > > > Last time I tried to put this into linux-next I got immediately > > > bombarded by a number of build failures, so I backed things out. The > > > current plan is to give this another try after v4.4-rc1. > > > > We're now into the 4.5 timeframe. Does anyone have a concrete set of > > things that need to happen before this patch series makes it into > > mainline? > > I think the current status is that we're more or less blocked on the > decision on what the reset state of the PWM should be. The question is > what to do if the PWM hardware readout differs from the settings found > in DT. I think I already explained my PoV regarding the default/reference setting (or what you later call the software state). To me, this has never matched the state of the PWM device, it's just a reference duty-cycle and polarity configuration that PWM users can decide to apply or not. Currently, there's no mechanism to apply the reference PWM config when a user request a PWM (which is the only moment we could be able to apply this config, since reference configs are per users and not attached to the PWM device itself). All I'm trying to do in this series is teach some PWM users to make use of hardware read-out state instead of blindly applying a default config that can generate glitches in the PWM signal. > > > From searching I see that the latest version of this series is v4 and > > there are a smattering of comments on the 24-patch series. Presumably > > a v5 needs to be posted to address those things. > > > > ...but it looks like the big sticking point is that Boris is waiting > > for a response to his questions in > > <https://patchwork.kernel.org/patch/7622881/>. Thierry: can you give > > Boris some direction for what else he needs to do? We need to come up > > with _some_ solution since this series gets us much better support for > > PWM regulators. Without this series or some other solution, PWM > > regulators aren't usable in mainline on any system that uses them for > > system-critical rails. Nearly all Rockchip reference boards and > > shipping devices uses a PWM regulator for the system-critidal "logic" > > rail. That means any patches which need to change this rail in Linux > > are blocked. > > I really don't understand this design decision. I presume that the PWM > controlling this system-critical logic is driven by the SoC? So if the > regulator is system-critical, doesn't that make it a chicken and egg > problem? How can the SoC turn the PWM on if it doesn't have power? But > perhaps I'm completely misunderstanding what you're saying. Perhaps if > somebody could summarize how exactly this works, it would help better > understand the requirements or what's the correct thing to do. > > > If there's already been off-list discussion and Boris already knows > > what the next steps are then my apologies and I'll wait patiently for > > the next series. ;) > > I don't think we reached a conclusion on this. And to be honest, I'm not > sure what the right way forward is in this situation. So in order to > make some forward progress I suggest we start a discussion, hopefully > that will clarify the situation and help lead to the conclusion. Let me > recap where we are: > > Boris' series has two goals: 1) allow seamless hand-off from firmware to > kernel of a PWM channel and 2) apply changes to a regulator in a single > atomic operation. To achieve this the concept of PWM state is introduced > which encapsulates the settings of a PWM channel. On driver probe the > current state will be read from hardware and when one or more parameters > are to be changed, the current state is duplicated, the new values set > in the state and the new state applied. > > The problem that we've encountered is that since the PWM parameters are > specified in DT (or board files), there is the possibility of the PWM > hardware state and the board parameters disagreeing. To resolve such > situations there must be a point in time where both hardware state and > software state must be synchronized. Maybe we're just hunting ghosts here. There's only one valid state, and this is the 'hardware state'. I reused the pwm_state struct to store the reference config (what you call software state), but actually that's not a state, that's a configuration template, it will only become the PWM state after being applied. Maybe I should use another struct to clarify that. > Now the most straightforward way to > do that would be to simply apply the software state and be done with it. Why that? I mean what's the problem of having a reference/template config that PWM users can rely on if they want to, and still keep the real state retrieved from hardware read-out? This way PWM users can decide which one they want to use. > However the software state initially lacks the duty cycle because it is > a parameter that usually depends on the use-case (for backlight for > instance it controls the brightness, for regulators it controls the > output voltage, ...). Yes, and I perfectly understand that, we don't want to encode the default duty cycle in the DT. > > Applying the software state as-is also means that there's no reason at > all to read out the hardware state in the first place, because it will > simply be discarded. Yes, but as stated above, this can be a source of glitches, which is exactly what we're trying to avoid. > > An alternative would be to discard the software state and trust the > hardware to be configured correctly. That's somewhat risky because we > don't know if the hardware is properly configured. Or Linux might have > different requirements from the firmware and hence needs to configure > the PWM differently. Yep, I agree on that too. > > Neither of the above are very attractive options. The best I've been > able to come up with so far is to completely remove this decision from > the PWM subsystem and let users handle this. That is, a PWM regulator > driver would have to have all the knowledge about how to configure the > PWM for its needs. So upon probe, the PWM regulator driver would inspect > the current state of the PWM and adjust if necessary, then apply again. > Ideally of course it wouldn't have to do anything because the hardware > PWM state would match the software configuration. The idea here is that > the PWM regulator driver knows exactly what duty cycle to configure to > obtain the desired output voltage. That complicates a lot of things. I also suggested another approach: provide an API to express the duty-cycle value relatively to the period value (I actually sent a proposal for that, but you didn't comment on it). This would keep the PWM user implementations simple, and let the PWM core switch from the bootloader/firmware period/polarity config to the DT/PWM-lookup-table one on the first pwm_set_rel_duty_cycle() call. > > That doesn't really get us closer, though. There is still the issue of > the user having to deal with two states: the current hardware state and > the software state as configured in DT or board files. At the risk of repeating myself, I don't think we ever had a software state, and existing PWM users are just assuming that the PWM devices they are requesting were not used before. IOW, yes, patching PWM users to support smoother transitions on the PWM devices require some changes, but in the other hand, that's not like they were doing any better before that series. And they still have the choice to completely ignore the current PWM state and blindly apply their own config (extracted from the reference PWM config) Best Regards, Boris > > Like I said, I'm on the fence about this, so I'd appreciate any comments > and perhaps insight from user subsystem maintainers on how they'd like > this to look, or how this has been done with other resources (GPIOs, > ...?) > > Thierry -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
[toc] | [prev] | [next] | [standalone]
| From | Doug Anderson <dianders@google.com> |
|---|---|
| Date | 2016-01-25 20:00 +0100 |
| Message-ID | <qUUwP-8gR-31@gated-at.bofh.it> |
| In reply to | #1317082 |
Hi, On Mon, Jan 25, 2016 at 9:08 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > I really don't understand this design decision. I presume that the PWM > controlling this system-critical logic is driven by the SoC? So if the > regulator is system-critical, doesn't that make it a chicken and egg > problem? How can the SoC turn the PWM on if it doesn't have power? But > perhaps I'm completely misunderstanding what you're saying. Perhaps if > somebody could summarize how exactly this works, it would help better > understand the requirements or what's the correct thing to do. Sure, here's how the dang thing works, as I understand it. First, an overview of PWM regulator in general (maybe you know this, but to get us on the same page). There's an external regulator on the system. Looking on at least one board I see a TLV62565 specifically. From the docs of TLV62565, I see it describe the situation as the chip being able to provide an adjustable output voltage configurable via an external resistor divider. In simplified terms words you can adjust the output voltage of the regulator by tweaking the inputs to one of its pins. I'm just a software guy so I can't explain all the details of it, but the net-net of the situation is is that you can hook this configuration pin up to the output of a PWM (with a bunch of well balanced resistors and capacitors) and then you can set the voltage based on the output of the PWM. OK, so what happens at bootup? At bootup most of the pins of the rk3288 (including the PWM) are configured as inputs with a pull. The particular pin hooked up to this PWM has a pulldown. Remember that we've got this nicely balanced set of resistors and capacitors hooked up to the output of our PWM pin? So what happens when we have this pin configured as an input? As I understand it / remember it: * input w/ no pull: equivalent to 50% duty cycle on the PWM * input w/ pull down: equivalent to slightly higher voltage than 50% duty cycle on the PWM * input w/ pull up: equivalent to slightly lower voltage than 50% duty cycle on the PWM On our particular board that means that the rail comes up with roughly 1.1V. If you drive the PWM at 100% (or set the pin to output high) you get .86V and if you drive the PWM at 0% (or set the pin to output low) you get 1.36V. Now, 1.1V is plenty of voltage to boot the system. In fact most of the logic within the SoC can run as low as 0.95V I think. ...but 0.86 V is not enough to run the logic parts of the system (even at their default bootup frequencies) 1.1V is _definitely_ not enough to run the SDRAM memory controller at full speed. So the bootloader wants to run the system fast so it can boot fast. It increases the CPU rails (as is typical for a bootloader) and moves the ARM CPU to 1.8GHz (from the relatively slow boot frequency) and also raises the logic rail to 1.2V (or I think 1.15 V on systems w/ different memory configs) and inits the SDRAM controller to run at full speed. Then it boots Linux. Note: apparently in U-Boot they actually boot system slower (this was at least true 1.5 years ago with some reference U-Boot Rockchip provided). If I understand correctly they _didn't_ init the SDRAM controller as full speed in the bootloader and just left the logic rail at its bootup default. If everyone had done that then our job would be "easier" because we wouldn't need to read in the voltage provided by the bootloader (by reading the PWM and cros-referencing with our table), though even in that case we'd have to be very careful not to glitch the line (since .86 V is too low). Of course all of those systems are stuck running at a very slow memory speed until Linux gets DDR Frequency support for Rockchip whereas systems with our bootloader not only boot faster but also get to use the full memory speed even without any Linux DDRFreq drivers. In any case: I think I've demonstrated how a critical system rail can be using a PWM regulator and how glitching that PWM regulator at boot time can be catastrophic. Possibly it's not critical to be able to "read" the voltage that that bootloader left things configured at (it's mostly nice for debugging purposes), but it's definitely important to make sure we don't set it to some default and important to never glitch it. Said another way, presumably a DDR Freq driver would be able to switch the memory controller frequency sanely by reading the memory controller frequency and using that to figure out whether it needed to up the logic rail before or after the DDR Freq change. >> If there's already been off-list discussion and Boris already knows >> what the next steps are then my apologies and I'll wait patiently for >> the next series. ;) > > I don't think we reached a conclusion on this. And to be honest, I'm not > sure what the right way forward is in this situation. So in order to > make some forward progress I suggest we start a discussion, hopefully > that will clarify the situation and help lead to the conclusion. Let me > recap where we are: > > Boris' series has two goals: 1) allow seamless hand-off from firmware to > kernel of a PWM channel and 2) apply changes to a regulator in a single > atomic operation. To achieve this the concept of PWM state is introduced > which encapsulates the settings of a PWM channel. On driver probe the > current state will be read from hardware and when one or more parameters > are to be changed, the current state is duplicated, the new values set > in the state and the new state applied. At at even higher level the goal is to support PWM regulator for a system-critical rail without ever glitching. If we could solve that problem in some other way that would also be fine too, I think. > The problem that we've encountered is that since the PWM parameters are > specified in DT (or board files), there is the possibility of the PWM > hardware state and the board parameters disagreeing. To resolve such > situations there must be a point in time where both hardware state and > software state must be synchronized. Now the most straightforward way to > do that would be to simply apply the software state and be done with it. > However the software state initially lacks the duty cycle because it is > a parameter that usually depends on the use-case (for backlight for > instance it controls the brightness, for regulators it controls the > output voltage, ...). Excuse me for not knowing all details that have been talked about before, but... A) The software state here is the period and flags (AKA "inverted), right? It does seem possible that you could apply the period and flags while keeping the calculated bootup duty cycle percentage (presuming that the PWM was actually enabled at probe time and there was a bootup duty cycle at all). That would basically say that whenever you set the period of a PWM then the duty cycle of the PWM should remain the same percentage. That actually seems quite sane IMHO. It seems much saner than trying to keep the duty cycle "ns" when the period changes or resetting the PWM to some default when the period changes. B) Alternatively, I'd also say that setting a period without a duty cycle doesn't make a lot of sense. ...so you could just apply the period at the same time that you apply the duty cycle the first time. Presumably you'd want to "lie" to the callers of the PWM subsystem and tell them that you already changed the period even though the change won't really take effect until they actually set the duty cycle. If anyone cared to find out the true hardware period we could add a new pwm_get_hw_period(). ...or since the only reason you'd want to know the hardware period would be if you're trying to read the current duty cycle percentage, you could instead add "pwm_get_hw_state()" and have that return both the hardware period ns and duty cycle ns (which is the most accurate way to return the "percentage" without using fix or floating point math). Both of the above options seems like it could be sensible. The 2nd seems cleaner because it doesn't require you to recalculate / approximate the old duty cycle using a new period, but it's slightly uglier because it no longer returns the true hardware state from pwm_get_period(). > Applying the software state as-is also means that there's no reason at > all to read out the hardware state in the first place, because it will > simply be discarded. Pretty sure we can't discard the hardware duty cycle at bootup, as per above. > An alternative would be to discard the software state and trust the > hardware to be configured correctly. That's somewhat risky because we > don't know if the hardware is properly configured. Or Linux might have > different requirements from the firmware and hence needs to configure > the PWM differently. Doesn't seem like a good idea either. > Neither of the above are very attractive options. The best I've been > able to come up with so far is to completely remove this decision from > the PWM subsystem and let users handle this. That is, a PWM regulator > driver would have to have all the knowledge about how to configure the > PWM for its needs. So upon probe, the PWM regulator driver would inspect > the current state of the PWM and adjust if necessary, then apply again. > Ideally of course it wouldn't have to do anything because the hardware > PWM state would match the software configuration. The idea here is that > the PWM regulator driver knows exactly what duty cycle to configure to > obtain the desired output voltage. I think this is like my suggestion B), right? AKA the PWM regulator would be the sole caller of pwm_get_hw_state() and it would use this to figure out the existing duty cycle percentage. Then it would translate that into "ns" and would set the duty cycle. Upon the first set of the duty cycle both the period and duty cycle would be applied at the same time. > That doesn't really get us closer, though. There is still the issue of > the user having to deal with two states: the current hardware state and > the software state as configured in DT or board files. I think the only users that need to deal with this are one that need a seamless transition from bootup settings. Adding a new API call to support a new feature like this doesn't seem insane, and anyone who doesn't want this new feature can just never call the new API. The only thing that would "change" from the point of view of old drivers is that the PWM period wouldn't change at bootup until the duty cycle was set. IMHO this is probably a bug fix. AKA, for a PWM backlight, imagine: 1. Firmware sets period to 20000 ns, duty cycle to 8000 ns (40%) 2. Linux boots up and sets period to 10000 ns. Brightness of backlight instantly goes to 80%. 3. Eventually something decides to set the backlight duty cycle and it goes to the proper rate. Skipping #2 seems like the right move. ...or did I misunderstand how something works? > Like I said, I'm on the fence about this, so I'd appreciate any comments > and perhaps insight from user subsystem maintainers on how they'd like > this to look, or how this has been done with other resources (GPIOs, > ...?) -Doug
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web