| Message ID | 20200226135229.24929-1-matthias.schiffer@ew.tq-group.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/4] pwm: pca9685: remove unused duty_cycle struct element | expand |
Hello Matthias, as you seem to have this hardware on your desk, it would be great if you could answer the following questions: - Does the hardware complete the currently running period before applying a new setting? - Is this racy somehow (i.e. can it happen that when going from duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000 the output is 1000/10000 (or 4000/5000) for one cycle)? - Does the hardware complete the currently running period before .enabled = false is configured? - How does the output pin behave on a disabled PWM. (Usual candidates are: freeze where is just happens to be, constant inactive and High-Z). Best regards Uwe
On Wed, 2020-02-26 at 16:10 +0100, Uwe Kleine-König wrote: > Hello Matthias, > > as you seem to have this hardware on your desk, it would be great if > you > could answer the following questions: > > - Does the hardware complete the currently running period before > applying a new setting? The datasheet claims: > Because the loading of the LEDn_ON and LEDn_OFF registers is via the > I 2 C-bus, and > asynchronous to the internal oscillator, we want to ensure that we do > not see any visual > artifacts of changing the ON and OFF values. This is achieved by > updating the changes at > the end of the LOW cycle. My interpretation is that the hardware will complete its period before applying the new settings. I might check with a scope tomorrow-ish. > - Is this racy somehow (i.e. can it happen that when going from > duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000 > the > output is 1000/10000 (or 4000/5000) for one cycle)? It currently is racy. It should be possible to fix that either by updating all 4 registers in a single I2C write, or by using the "update on ACK" mode which requires all 4 registers to be updated before the new setting is applied (I'm not sure if this mode would require using a single I2C write as well though). > - Does the hardware complete the currently running period before > .enabled = false is configured? As my interpretation is that new settings are applied asynchronously, I assume that the final running period is completed after .enabled is set to false. > - How does the output pin behave on a disabled PWM. (Usual > candidates > are: freeze where is just happens to be, constant inactive and > High-Z). Constant inactive. This is also the case when the chip is put into sleep mode. Note that the interpretation of "inactive" depends in the invert flag in the MODE2 register. As it turns out, this driver is broken in yet another way I didn't find before: For changing the global prescaler the chip needs to be put into sleep mode, but the driver doesn't follow the restart sequence described in the datasheet when waking it back up. In consequence, changing the period of one PWM does not only modify the period of all PWMs (which is bad enough, but can't be avoided with this hardware), but it also leaves all PWMs disabled... As this hardware only has a single prescaler for all PWMs, should changing the period for individual PWMs even be allowed at all? Maybe only when all other PWMs are inactive? I could imagine setting it in DTS instead (but I'm not sure what to do about non-OF users of this driver, for example when configured via ACPI). Regards, Matthias > > Best regards > Uwe >
On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote: > On Wed, 2020-02-26 at 16:10 +0100, Uwe Kleine-König wrote: > > Hello Matthias, > > > > as you seem to have this hardware on your desk, it would be great if > > you > > could answer the following questions: > > > > - Does the hardware complete the currently running period before > > applying a new setting? > > The datasheet claims: > > > Because the loading of the LEDn_ON and LEDn_OFF registers is via the > > I 2 C-bus, and > > asynchronous to the internal oscillator, we want to ensure that we do > > not see any visual > > artifacts of changing the ON and OFF values. This is achieved by > > updating the changes at > > the end of the LOW cycle. > > My interpretation is that the hardware will complete its period before > applying the new settings. I might check with a scope tomorrow-ish. I agree given that you can update duty_cycle and period in a single write as you considered below. Maybe it is worth playing with small periods and a slow i2c bus speed (or hijack the bus by simulating a clock stretch). > > - Is this racy somehow (i.e. can it happen that when going from > > duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000 the > > output is 1000/10000 (or 4000/5000) for one cycle)? > > It currently is racy. It should be possible to fix that either by > updating all 4 registers in a single I2C write, or by using the "update > on ACK" mode which requires all 4 registers to be updated before the > new setting is applied (I'm not sure if this mode would require using a > single I2C write as well though). I can offer a second pair of eyeballs to interpret the datasheet. Will take a look tomorrow. > > - Does the hardware complete the currently running period before > > .enabled = false is configured? > > As my interpretation is that new settings are applied asynchronously, I > assume that the final running period is completed after .enabled is set > to false. > > > - How does the output pin behave on a disabled PWM. (Usual candidates > > are: freeze where is just happens to be, constant inactive and > > High-Z). > > Constant inactive. This is also the case when the chip is put into > sleep mode. Note that the interpretation of "inactive" depends in the > invert flag in the MODE2 register. This is optimal. > As it turns out, this driver is broken in yet another way I didn't find > before: For changing the global prescaler the chip needs to be put into > sleep mode, but the driver doesn't follow the restart sequence > described in the datasheet when waking it back up. In consequence, > changing the period of one PWM does not only modify the period of all > PWMs (which is bad enough, but can't be avoided with this hardware), > but it also leaves all PWMs disabled... > > As this hardware only has a single prescaler for all PWMs, should > changing the period for individual PWMs even be allowed at all? Maybe > only when all other PWMs are inactive? yes, that is the general approach. Please document this in a Limitiations: paragraph. See drivers/pwm/pwm-imx-tpm.c which has a similar problem. > I could imagine setting it in DTS instead (but I'm not sure what to do > about non-OF users of this driver, for example when configured via > ACPI). I don't like fixing the period in the device tree. This isn't a hardware property and it is less flexible than possible. Best regards Uwe
On Wed, 2020-02-26 at 20:21 +0100, Uwe Kleine-König wrote: > On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote: > > On Wed, 2020-02-26 at 16:10 +0100, Uwe Kleine-König wrote: > > > Hello Matthias, > > > > > > as you seem to have this hardware on your desk, it would be great > > > if > > > you > > > could answer the following questions: > > > > > > - Does the hardware complete the currently running period before > > > applying a new setting? > > > > The datasheet claims: > > > > > Because the loading of the LEDn_ON and LEDn_OFF registers is via > > > the > > > I 2 C-bus, and > > > asynchronous to the internal oscillator, we want to ensure that > > > we do > > > not see any visual > > > artifacts of changing the ON and OFF values. This is achieved by > > > updating the changes at > > > the end of the LOW cycle. > > > > My interpretation is that the hardware will complete its period > > before > > applying the new settings. I might check with a scope tomorrow-ish. > > I agree given that you can update duty_cycle and period in a single > write as you considered below. Maybe it is worth playing with small > periods and a slow i2c bus speed (or hijack the bus by simulating a > clock stretch). > > > > - Is this racy somehow (i.e. can it happen that when going from > > > duty_cycle/period = 1000/5000 to duty_cycle/period = > > > 4000/10000 the > > > output is 1000/10000 (or 4000/5000) for one cycle)? > > > > It currently is racy. It should be possible to fix that either by > > updating all 4 registers in a single I2C write, or by using the > > "update > > on ACK" mode which requires all 4 registers to be updated before > > the > > new setting is applied (I'm not sure if this mode would require > > using a > > single I2C write as well though). > > I can offer a second pair of eyeballs to interpret the datasheet. > Will > take a look tomorrow. > > > > - Does the hardware complete the currently running period before > > > .enabled = false is configured? > > > > As my interpretation is that new settings are applied > > asynchronously, I > > assume that the final running period is completed after .enabled is > > set > > to false. > > > > > - How does the output pin behave on a disabled PWM. (Usual > > > candidates > > > are: freeze where is just happens to be, constant inactive and > > > High-Z). > > > > Constant inactive. This is also the case when the chip is put into > > sleep mode. Note that the interpretation of "inactive" depends in > > the > > invert flag in the MODE2 register. > > This is optimal. > > > As it turns out, this driver is broken in yet another way I didn't > > find > > before: For changing the global prescaler the chip needs to be put > > into > > sleep mode, but the driver doesn't follow the restart sequence > > described in the datasheet when waking it back up. In consequence, > > changing the period of one PWM does not only modify the period of > > all > > PWMs (which is bad enough, but can't be avoided with this > > hardware), > > but it also leaves all PWMs disabled... > > > > As this hardware only has a single prescaler for all PWMs, should > > changing the period for individual PWMs even be allowed at all? > > Maybe > > only when all other PWMs are inactive? > > yes, that is the general approach. Please document this in a > Limitiations: paragraph. See drivers/pwm/pwm-imx-tpm.c which has a > similar problem. This raises the question what to do about the GPIO mode supported by the driver: While the period does not affect GPIO usage of PWMs, changing the period would put the chip in sleep mode and thus briefly disable active GPIOs. I assume that this should preclude changing the period when there are any PWMs requsted as GPIOs, but now the order in which things are initialized becomes crucial: - All references to PWMs of the same PCA9685 must specify the same period - When requesting a PWM as GPIO, no period can be specified => When a PWM referenced as GPIO is requested before the first actual PWM, setting the correct period becomes impossible. I can't think of a nice solution that doesn't require serious rework - maybe we need at least an optional period property in DTS to support this case? This could either be implemented as a default period or a fixed period. A more elaborate solution could be to remove the GPIO code from PCA9685 and implement a generic GPIO-PWM driver instead that could be configured in DTS (again, I have no idea how to support non-DTS platforms). Unfortunately, I assume I won't have time to realize such a solution myself. Matthias > > > I could imagine setting it in DTS instead (but I'm not sure what to > > do > > about non-OF users of this driver, for example when configured via > > ACPI). > > I don't like fixing the period in the device tree. This isn't a > hardware > property and it is less flexible than possible. > > Best regards > Uwe >
On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote: > duty_cycle was only set, never read. > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > --- > drivers/pwm/pwm-pca9685.c | 4 ---- > 1 file changed, 4 deletions(-) Applied, thanks. Thierry
On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote: > > duty_cycle was only set, never read. > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > --- > > drivers/pwm/pwm-pca9685.c | 4 ---- > > 1 file changed, 4 deletions(-) > > Applied, thanks. I'm not sure this patch is correct. We already have broken GPIO in this driver. Do we need more breakage?
Hi, On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote: > As it turns out, this driver is broken in yet another way I didn't find > before: For changing the global prescaler the chip needs to be put into > sleep mode, but the driver doesn't follow the restart sequence > described in the datasheet when waking it back up. In consequence, > changing the period of one PWM does not only modify the period of all > PWMs (which is bad enough, but can't be avoided with this hardware), > but it also leaves all PWMs disabled... I am unable to reproduce this: If I set a specific duty cycle on a channel and then change the period, the channel stays active. I can see the brightness of an LED decrease if I increase the period. This is expected, because after the SLEEP bit is set, we wait for 500usecs and then write to the LED ON/OFF registers. This leaves the channel enabled with the new period (but with old duty_ns value => different ratio) A few years ago, I played around with the idea of remembering the duty_ns to period_ns ratio and setting it accordingly after a period change, possibly also with a shortcut of setting the RESTART bit if the ratio did not change. Maybe after the switch to the atomic API, this would be a nice improvement. Best regards, Clemens
On Mon, Mar 30, 2020 at 04:18:22PM +0300, Andy Shevchenko wrote: > On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote: > > > duty_cycle was only set, never read. > > > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > > --- > > > drivers/pwm/pwm-pca9685.c | 4 ---- > > > 1 file changed, 4 deletions(-) > > > > Applied, thanks. > > I'm not sure this patch is correct. What makes you say that? If you look at the code, the driver sets this field to either 0 or some duty cycle value but ends up never using it. Why would it be wrong to remove that code? > We already have broken GPIO in this driver. Do we need more breakage? My understanding is that nobody was able to pinpoint exactly when this regressed, or if this only worked by accident to begin with. It sounds like Clemens has a way of testing this driver, so perhaps we can solve that GPIO issue while we're at it. The last discussion on this seems to have been around the time when you posted a fix for it: https://patchwork.ozlabs.org/patch/1156012/ But then Sven had concerns that that also wasn't guaranteed to work: https://lkml.org/lkml/2019/6/2/73 So I think we could either apply your patch to restore the old behaviour which I assume you tested, so at least it seems to work in practice, even if there's still a potential race that Sven pointed out in the above link. I'd prefer something alternative because it's obviously confusing and completely undocumented. Mika had already proposed something that's a little bit better, though still somewhat confusing. Oh... actually reading further through those threads there seems to be a patch from Sven that was reviewed by Mika but then nothing happened: https://lkml.org/lkml/2019/6/4/1039 with the corresponding patchwork URL: https://patchwork.ozlabs.org/patch/1110083/ Andy, Clemens, do you have a way of testing the GPIO functionality of this driver? If so, it'd be great if you could check the above patch from Sven to fix PWM/GPIO interop. Thierry
On Mon, Mar 30, 2020 at 06:02:38PM +0200, Thierry Reding wrote: > On Mon, Mar 30, 2020 at 04:18:22PM +0300, Andy Shevchenko wrote: > > On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote: > > > > duty_cycle was only set, never read. > > > > > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > > > --- > > > > drivers/pwm/pwm-pca9685.c | 4 ---- > > > > 1 file changed, 4 deletions(-) > > > > > > Applied, thanks. > > > > I'm not sure this patch is correct. > > What makes you say that? If you look at the code, the driver sets this > field to either 0 or some duty cycle value but ends up never using it. > Why would it be wrong to remove that code? > > > We already have broken GPIO in this driver. Do we need more breakage? > > My understanding is that nobody was able to pinpoint exactly when this > regressed, or if this only worked by accident to begin with. It sounds > like Clemens has a way of testing this driver, so perhaps we can solve > that GPIO issue while we're at it. > > The last discussion on this seems to have been around the time when you > posted a fix for it: > > https://patchwork.ozlabs.org/patch/1156012/ > > But then Sven had concerns that that also wasn't guaranteed to work: > > https://lkml.org/lkml/2019/6/2/73 > > So I think we could either apply your patch to restore the old behaviour > which I assume you tested, so at least it seems to work in practice, > even if there's still a potential race that Sven pointed out in the > above link. > > I'd prefer something alternative because it's obviously confusing and > completely undocumented. Mika had already proposed something that's a > little bit better, though still somewhat confusing. > > Oh... actually reading further through those threads there seems to be a > patch from Sven that was reviewed by Mika but then nothing happened: > > https://lkml.org/lkml/2019/6/4/1039 > > with the corresponding patchwork URL: > > https://patchwork.ozlabs.org/patch/1110083/ > > Andy, Clemens, do you have a way of testing the GPIO functionality of > this driver? If so, it'd be great if you could check the above patch > from Sven to fix PWM/GPIO interop. Yes. I'll have a look and report back in a few days. Clemens
On Mon, Mar 30, 2020 at 06:02:38PM +0200, Thierry Reding wrote: > On Mon, Mar 30, 2020 at 04:18:22PM +0300, Andy Shevchenko wrote: > > On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote: > > > > duty_cycle was only set, never read. > > > > > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > > > --- > > > > drivers/pwm/pwm-pca9685.c | 4 ---- > > > > 1 file changed, 4 deletions(-) > > > > > > Applied, thanks. > > > > I'm not sure this patch is correct. > > What makes you say that? If you look at the code, the driver sets this > field to either 0 or some duty cycle value but ends up never using it. > Why would it be wrong to remove that code? > > > We already have broken GPIO in this driver. Do we need more breakage? > > My understanding is that nobody was able to pinpoint exactly when this > regressed, or if this only worked by accident to begin with. It sounds > like Clemens has a way of testing this driver, so perhaps we can solve > that GPIO issue while we're at it. > > The last discussion on this seems to have been around the time when you > posted a fix for it: > > https://patchwork.ozlabs.org/patch/1156012/ > > But then Sven had concerns that that also wasn't guaranteed to work: > > https://lkml.org/lkml/2019/6/2/73 > > So I think we could either apply your patch to restore the old behaviour > which I assume you tested, so at least it seems to work in practice, > even if there's still a potential race that Sven pointed out in the > above link. > > I'd prefer something alternative because it's obviously confusing and > completely undocumented. Mika had already proposed something that's a > little bit better, though still somewhat confusing. > > Oh... actually reading further through those threads there seems to be a > patch from Sven that was reviewed by Mika but then nothing happened: > > https://lkml.org/lkml/2019/6/4/1039 > > with the corresponding patchwork URL: > > https://patchwork.ozlabs.org/patch/1110083/ > > Andy, Clemens, do you have a way of testing the GPIO functionality of > this driver? If so, it'd be great if you could check the above patch > from Sven to fix PWM/GPIO interop. Looks good. Tested it today and I can no longer reproduce the issues when switching between PWM and GPIO modes. It did not apply cleanly on the current mainline or for-next branch, so I'll send a fixed up version of the patch with my Tested-by tag shortly. I noticed an unrelated issue when disabling and enabling the channel though, for which I will either send a patch or maybe try to convert the driver to the atomic API first and then look if it is still a problem. (Issue is that if you disable the channel, the LED_OFF counter is cleared, which means you have to reconfigure the duty cycle after reenabling. It's probably better if only the FULL_OFF bit is toggled in enable/disable as it has precedence over the others anyway and then the previous state would not be changed..?) Clemens
On Wed, Apr 01, 2020 at 06:36:40PM +0200, Clemens Gruber wrote: > On Mon, Mar 30, 2020 at 06:02:38PM +0200, Thierry Reding wrote: > > On Mon, Mar 30, 2020 at 04:18:22PM +0300, Andy Shevchenko wrote: > > > On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer wrote: > > > > > duty_cycle was only set, never read. > > > > > > > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> > > > > > --- > > > > > drivers/pwm/pwm-pca9685.c | 4 ---- > > > > > 1 file changed, 4 deletions(-) > > > > > > > > Applied, thanks. > > > > > > I'm not sure this patch is correct. > > > > What makes you say that? If you look at the code, the driver sets this > > field to either 0 or some duty cycle value but ends up never using it. > > Why would it be wrong to remove that code? > > > > > We already have broken GPIO in this driver. Do we need more breakage? > > > > My understanding is that nobody was able to pinpoint exactly when this > > regressed, or if this only worked by accident to begin with. It sounds > > like Clemens has a way of testing this driver, so perhaps we can solve > > that GPIO issue while we're at it. > > > > The last discussion on this seems to have been around the time when you > > posted a fix for it: > > > > https://patchwork.ozlabs.org/patch/1156012/ > > > > But then Sven had concerns that that also wasn't guaranteed to work: > > > > https://lkml.org/lkml/2019/6/2/73 > > > > So I think we could either apply your patch to restore the old behaviour > > which I assume you tested, so at least it seems to work in practice, > > even if there's still a potential race that Sven pointed out in the > > above link. > > > > I'd prefer something alternative because it's obviously confusing and > > completely undocumented. Mika had already proposed something that's a > > little bit better, though still somewhat confusing. > > > > Oh... actually reading further through those threads there seems to be a > > patch from Sven that was reviewed by Mika but then nothing happened: > > > > https://lkml.org/lkml/2019/6/4/1039 > > > > with the corresponding patchwork URL: > > > > https://patchwork.ozlabs.org/patch/1110083/ > > > > Andy, Clemens, do you have a way of testing the GPIO functionality of > > this driver? If so, it'd be great if you could check the above patch > > from Sven to fix PWM/GPIO interop. > > Looks good. Tested it today and I can no longer reproduce the issues > when switching between PWM and GPIO modes. > It did not apply cleanly on the current mainline or for-next branch, so > I'll send a fixed up version of the patch with my Tested-by tag shortly. Awesome, thank you! > I noticed an unrelated issue when disabling and enabling the channel > though, for which I will either send a patch or maybe try to convert the > driver to the atomic API first and then look if it is still a problem. > (Issue is that if you disable the channel, the LED_OFF counter is > cleared, which means you have to reconfigure the duty cycle after > reenabling. It's probably better if only the FULL_OFF bit is toggled in > enable/disable as it has precedence over the others anyway and then the > previous state would not be changed..?) Converting to the atomic API would certainly be beneficial because it gives you much more control over what you want to program to hardware. Other than that, yes, if ->enable() called after ->disable() doesn't put the PWM into the same state that it was before ->disable(), then that'd be a bug. From what you're saying this will make the driver only work if there's a ->config() call after ->disable(). That's wrong. So yes, setting only that LED_FULL bit in ->disable() and clearing it in ->enable() sounds like a better option than the current implementation. Thierry
On Wed, 2020-04-01 at 19:45 +0200, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Wed, Apr 01, 2020 at 06:36:40PM +0200, Clemens Gruber wrote: > > On Mon, Mar 30, 2020 at 06:02:38PM +0200, Thierry Reding wrote: > > > On Mon, Mar 30, 2020 at 04:18:22PM +0300, Andy Shevchenko wrote: > > > > On Mon, Mar 30, 2020 at 4:09 PM Thierry Reding < > > > > thierry.reding@gmail.com> wrote: > > > > > > > > > > On Wed, Feb 26, 2020 at 02:52:26PM +0100, Matthias Schiffer > > > > > wrote: > > > > > > duty_cycle was only set, never read. > > > > > > > > > > > > Signed-off-by: Matthias Schiffer < > > > > > > matthias.schiffer@ew.tq-group.com> > > > > > > --- > > > > > > drivers/pwm/pwm-pca9685.c | 4 ---- > > > > > > 1 file changed, 4 deletions(-) > > > > > > > > > > Applied, thanks. > > > > > > > > I'm not sure this patch is correct. > > > > > > What makes you say that? If you look at the code, the driver sets > > > this > > > field to either 0 or some duty cycle value but ends up never > > > using it. > > > Why would it be wrong to remove that code? > > > > > > > We already have broken GPIO in this driver. Do we need more > > > > breakage? > > > > > > My understanding is that nobody was able to pinpoint exactly when > > > this > > > regressed, or if this only worked by accident to begin with. It > > > sounds > > > like Clemens has a way of testing this driver, so perhaps we can > > > solve > > > that GPIO issue while we're at it. > > > > > > The last discussion on this seems to have been around the time > > > when you > > > posted a fix for it: > > > > > > https://patchwork.ozlabs.org/patch/1156012/ > > > > > > But then Sven had concerns that that also wasn't guaranteed to > > > work: > > > > > > https://lkml.org/lkml/2019/6/2/73 > > > > > > So I think we could either apply your patch to restore the old > > > behaviour > > > which I assume you tested, so at least it seems to work in > > > practice, > > > even if there's still a potential race that Sven pointed out in > > > the > > > above link. > > > > > > I'd prefer something alternative because it's obviously confusing > > > and > > > completely undocumented. Mika had already proposed something > > > that's a > > > little bit better, though still somewhat confusing. > > > > > > Oh... actually reading further through those threads there seems > > > to be a > > > patch from Sven that was reviewed by Mika but then nothing > > > happened: > > > > > > https://lkml.org/lkml/2019/6/4/1039 > > > > > > with the corresponding patchwork URL: > > > > > > https://patchwork.ozlabs.org/patch/1110083/ > > > > > > Andy, Clemens, do you have a way of testing the GPIO > > > functionality of > > > this driver? If so, it'd be great if you could check the above > > > patch > > > from Sven to fix PWM/GPIO interop. > > > > Looks good. Tested it today and I can no longer reproduce the > > issues > > when switching between PWM and GPIO modes. > > It did not apply cleanly on the current mainline or for-next > > branch, so > > I'll send a fixed up version of the patch with my Tested-by tag > > shortly. > > Awesome, thank you! > > > I noticed an unrelated issue when disabling and enabling the > > channel > > though, for which I will either send a patch or maybe try to > > convert the > > driver to the atomic API first and then look if it is still a > > problem. > > (Issue is that if you disable the channel, the LED_OFF counter is > > cleared, which means you have to reconfigure the duty cycle after > > reenabling. It's probably better if only the FULL_OFF bit is > > toggled in > > enable/disable as it has precedence over the others anyway and then > > the > > previous state would not be changed..?) > > Converting to the atomic API would certainly be beneficial because it > gives you much more control over what you want to program to > hardware. > > Other than that, yes, if ->enable() called after ->disable() doesn't > put > the PWM into the same state that it was before ->disable(), then > that'd > be a bug. From what you're saying this will make the driver only work > if > there's a ->config() call after ->disable(). That's wrong. > > So yes, setting only that LED_FULL bit in ->disable() and clearing it > in > ->enable() sounds like a better option than the current > implementation. I think what you're seeing is the same bug that prompted me to send my patchset - patch 4/4 moves to the atomic API and changes the logic to fix the interaction between config() and enable()/disable(). As it seems like 2/4 will have to be reverted though and my 4/4 depends on the cleanup of 2/4 and 3/4 (and 3/4 and 4/4 still need some work as well), feel free to have a jab at the issue yourself, as I don't know when I'll be able to get back to it. Thanks, Matthias > > Thierry > > * Unknown Key > * 0x7F3EB3A1
On Wed, Feb 26, 2020 at 12:04 PM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > > > - Is this racy somehow (i.e. can it happen that when going from > > duty_cycle/period = 1000/5000 to duty_cycle/period = 4000/10000 > > the > > output is 1000/10000 (or 4000/5000) for one cycle)? > > It currently is racy. It should be possible to fix that either by > updating all 4 registers in a single I2C write, or by using the "update > on ACK" mode which requires all 4 registers to be updated before the > new setting is applied (I'm not sure if this mode would require using a > single I2C write as well though). Matthias, did you verify experimentally that changing the period is racy? Looking at the datasheet and driver code, it shouldn't be. This is because the OFF time is set as a proportion of the counter range. When the period changes from 5000 to 10000, then 5000*20%/5000 and 10000*20%/10000 will result in the same 20% ratio (disregarding rounding errors). This is documented at the beginning of the driver: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-pca9685.c?h=v5.6#n25 Should we move that comment to pwm_config(), so future versions of ourselves won't overlook it?
On Mon, Mar 30, 2020 at 11:12 AM Clemens Gruber <clemens.gruber@pqgruber.com> wrote: > > On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote: > > As it turns out, this driver is broken in yet another way I didn't find > > before: For changing the global prescaler the chip needs to be put into > > sleep mode, but the driver doesn't follow the restart sequence > > described in the datasheet when waking it back up. In consequence, > > changing the period of one PWM does not only modify the period of all > > PWMs (which is bad enough, but can't be avoided with this hardware), > > but it also leaves all PWMs disabled... > > I am unable to reproduce this: If I set a specific duty cycle on a > channel and then change the period, the channel stays active. > I can see the brightness of an LED decrease if I increase the period. What happens when pwm channels 0 and 1 are both enabled, and you change the pwm period of channel 0. Does channel 1 remain on?
Hi, On Fri, Apr 03, 2020 at 07:50:07PM -0400, Sven Van Asbroeck wrote: > On Mon, Mar 30, 2020 at 11:12 AM Clemens Gruber > <clemens.gruber@pqgruber.com> wrote: > > > > On Wed, Feb 26, 2020 at 06:03:02PM +0100, Matthias Schiffer wrote: > > > As it turns out, this driver is broken in yet another way I didn't find > > > before: For changing the global prescaler the chip needs to be put into > > > sleep mode, but the driver doesn't follow the restart sequence > > > described in the datasheet when waking it back up. In consequence, > > > changing the period of one PWM does not only modify the period of all > > > PWMs (which is bad enough, but can't be avoided with this hardware), > > > but it also leaves all PWMs disabled... > > > > I am unable to reproduce this: If I set a specific duty cycle on a > > channel and then change the period, the channel stays active. > > I can see the brightness of an LED decrease if I increase the period. > > What happens when pwm channels 0 and 1 are both enabled, and > you change the pwm period of channel 0. Does channel 1 remain > on? Yes. Both channels remain on. Let's say I configure a period of 5ms for both channels 0 and 1, as well as a duty cycle of 4ms, meaning a relative duty cycle of 80%. If I then increase the period of channel 0 to 10ms, there will be a relative duty cycle of 40% on channel 0, but channel 1 will remain at a relative duty cycle of 80%. This is due to the relative nature of the internal ON/OFF times. For the channel with the period change however, we recalculate the duty_ns to period_ns ratio and reprogram the ON/OFF registers, because the user might have already given us a different duty cycle in .config / .apply. As the user is setting the duty cycle in nanoseconds, it makes sense that the relative duty cycle decreases in an absolute period increase. As for the behavior that the other channels remain at the same relative duty cycle: Not sure how we can avoid this, other than reprogramming all 15 other channels if one of them is changed and that's not really acceptable, I think. Clemens
On Sat, Apr 4, 2020 at 1:35 PM Clemens Gruber <clemens.gruber@pqgruber.com> wrote: > > As the user is setting the duty cycle in nanoseconds, it makes sense > that the relative duty cycle decreases in an absolute period increase. > As for the behavior that the other channels remain at the same relative > duty cycle: Not sure how we can avoid this, other than reprogramming all > 15 other channels if one of them is changed and that's not really > acceptable, I think. Thank you for the explanation, Clemens. Yes, it does make sense that the relative duty cycle changes when we change the period. A relative duty cycle of duty_cycle / period is what the user would expect to see. It also kind-of makes sense that the relative duty cycles of the other pwm channels do not change: after all, the user is not touching them, so would not expect them to change. However, the following does not make sense to me. Imagine pwm0 and pwm1 are both active and at 50%: period=5000000, duty_cycle=2500000. Then, change the period on pwm0: $ echo 10000000 > pwm0/period Then pwm0 gets dimmer (makes sense) and pwm1 keeps the same relative duty cycle (makes sense). However, if we now read out sysfs for pwm1, we get: $ echo pwm1/period 5000000 (wrong!) $ echo pwm1/duty_cycle 2500000 (wrong! although relative duty cycle is correct) Although the pwm1 period has changed, the API calls do not reflect this. This makes it next to impossible for users to know what the current period is set to. Moving to the atomic API won't help, because .get_state is called only once, when the chip is registered. It does look like we have a square peg (this chip) in a round hole (the standard assumptions the pwm core makes) ?
On Sat, Apr 04, 2020 at 04:17:00PM -0400, Sven Van Asbroeck wrote: > On Sat, Apr 4, 2020 at 1:35 PM Clemens Gruber > <clemens.gruber@pqgruber.com> wrote: > > > > As the user is setting the duty cycle in nanoseconds, it makes sense > > that the relative duty cycle decreases in an absolute period increase. > > As for the behavior that the other channels remain at the same relative > > duty cycle: Not sure how we can avoid this, other than reprogramming all > > 15 other channels if one of them is changed and that's not really > > acceptable, I think. > > Thank you for the explanation, Clemens. > > Yes, it does make sense that the relative duty cycle changes when we change > the period. A relative duty cycle of duty_cycle / period is what the user would > expect to see. > > It also kind-of makes sense that the relative duty cycles of the other > pwm channels > do not change: after all, the user is not touching them, so would not expect > them to change. > > However, the following does not make sense to me. Imagine pwm0 and pwm1 > are both active and at 50%: period=5000000, duty_cycle=2500000. Then, change > the period on pwm0: > > $ echo 10000000 > pwm0/period > > Then pwm0 gets dimmer (makes sense) and pwm1 keeps the same relative duty > cycle (makes sense). However, if we now read out sysfs for pwm1, we get: > > $ echo pwm1/period > 5000000 (wrong!) > $ echo pwm1/duty_cycle > 2500000 (wrong! although relative duty cycle is correct) > > Although the pwm1 period has changed, the API calls do not reflect this. > This makes it next to impossible for users to know what the current period > is set to. > > Moving to the atomic API won't help, because .get_state is called only > once, when the chip is registered. The .get_state() callback is actually called when the PWM is requested, which could be much later than when the chip is registered. That doesn't change the fact that it would be useless for this case, though. > It does look like we have a square peg (this chip) in a round hole (the > standard assumptions the pwm core makes) ? There are other chips where a single period is shared across multiple PWM channels. Typically what we do there is once a period is configured for a given channel, all subsequent PWM channel configurations must use the same period, or otherwise the driver will return an error code. See for example: - stm32_pwm_config() in drivers/pwm/pwm-stm32.c - lpc18xx_pwm_config() in drivers/pwm/pwm-lpc18xx-sct.c - pwm_imx_tpm_apply_hw() in drivers/pwm/pwm-imx-tpm.c - fsl_pwm_apply_config() in drivers/pwm/pwm-fsl-ftm.c The rationale behind that is that we must not change a PWM configuration without a consumer having explicitly requested it. It seems like PCA9685 is somewhere halfway between in that it will actually keep the same duty-cycle/period ratio, but that doesn't mean it is automatically okay to do this. The problem is that the duty-cycle to period ratio is only relevant in some cases. If all you care about is the power output of the PWM signal, which admittedly seems to be about 95% of all cases, then yes, this behaviour would be okay. But what if we have a consumer that relies on a particular width of the PWM pulse in absolute terms rather than relative to the period? Thierry
On Mon, 2020-04-06 at 11:51 +0200, Thierry Reding wrote: > > On Sat, Apr 04, 2020 at 04:17:00PM -0400, Sven Van Asbroeck wrote: > > > > It does look like we have a square peg (this chip) in a round hole > > (the > > standard assumptions the pwm core makes) ? > > There are other chips where a single period is shared across multiple > PWM channels. Typically what we do there is once a period is > configured > for a given channel, all subsequent PWM channel configurations must > use > the same period, or otherwise the driver will return an error code. > > See for example: > > - stm32_pwm_config() in drivers/pwm/pwm-stm32.c > - lpc18xx_pwm_config() in drivers/pwm/pwm-lpc18xx-sct.c > - pwm_imx_tpm_apply_hw() in drivers/pwm/pwm-imx-tpm.c > - fsl_pwm_apply_config() in drivers/pwm/pwm-fsl-ftm.c > > The rationale behind that is that we must not change a PWM > configuration > without a consumer having explicitly requested it. These implementations don't deal with the issue in a consistent way either though: 1) stm32_pwm_config() only checks for channels that are actually enabled, regardless if they're requested 2) lpc18xx_pwm_config() checks requested channels instead "2)" seems more correct to me, as the parameters of a requested channel can't be changed by another user, but it seems to prevent certain sequences. I don't have a good grasp of the the usual PWM request control flow - is it correct that the PWM state is not updated with the PWM args from OF when the PWM is requested? I only see pwm_adjust_config() doing something like that, which is not used in many places (and nothing preventing races) - but I might be overlooking something. Meaning, is it possible that two drivers request PWMs from the same pwmchip of type "2)" (or even a single driver using two PWMs) without configuring them right away may get into a situation where neither can set up a period at all even when all users want to use the same period? Matthias
On Fri, 2020-04-03 at 19:47 -0400, Sven Van Asbroeck wrote: > On Wed, Feb 26, 2020 at 12:04 PM Matthias Schiffer > <matthias.schiffer@ew.tq-group.com> wrote: > > > > > - Is this racy somehow (i.e. can it happen that when going from > > > duty_cycle/period = 1000/5000 to duty_cycle/period = > > > 4000/10000 > > > the > > > output is 1000/10000 (or 4000/5000) for one cycle)? > > > > It currently is racy. It should be possible to fix that either by > > updating all 4 registers in a single I2C write, or by using the > > "update > > on ACK" mode which requires all 4 registers to be updated before > > the > > new setting is applied (I'm not sure if this mode would require > > using a > > single I2C write as well though). > > Matthias, did you verify experimentally that changing the period is > racy? > > Looking at the datasheet and driver code, it shouldn't be. This is > because > the OFF time is set as a proportion of the counter range. When the > period > changes from 5000 to 10000, then 5000*20%/5000 and 10000*20%/10000 > will result in the same 20% ratio (disregarding rounding errors). > > This is documented at the beginning of the driver: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-pca9685.c?h=v5.6#n25 > > Should we move that comment to pwm_config(), so future versions of > ourselves won't overlook it? You are right, this results in the same ratio - the absolute on/off times may be wrong for a moment though when the period is changed. In the attached image, I have changed the period, but kept the absolute duty cycle fixed (note: this is in inverted mode, so the duty cycle controls the low time). It can be seen that after a too long high time (chip is in sleep mode) one period with too long low time follows (new period, old relative duty cycle), before the counter is reprogrammed to match the previous absolute duty cycle. I don't care too much about the details of the behaviour, as I only control LEDs using this chip and don't need to change the period after initial setup, but we should accurately document the shortcomings of the hardware and the driver (when we have decided how to fix some of the driver issues). Matthias
On Tue, 2020-04-07 at 16:46 +0200, Matthias Schiffer wrote: > On Fri, 2020-04-03 at 19:47 -0400, Sven Van Asbroeck wrote: > > On Wed, Feb 26, 2020 at 12:04 PM Matthias Schiffer > > <matthias.schiffer@ew.tq-group.com> wrote: > > > > > > > - Is this racy somehow (i.e. can it happen that when going > > > > from > > > > duty_cycle/period = 1000/5000 to duty_cycle/period = > > > > 4000/10000 > > > > the > > > > output is 1000/10000 (or 4000/5000) for one cycle)? > > > > > > It currently is racy. It should be possible to fix that either by > > > updating all 4 registers in a single I2C write, or by using the > > > "update > > > on ACK" mode which requires all 4 registers to be updated before > > > the > > > new setting is applied (I'm not sure if this mode would require > > > using a > > > single I2C write as well though). > > > > Matthias, did you verify experimentally that changing the period is > > racy? > > > > Looking at the datasheet and driver code, it shouldn't be. This is > > because > > the OFF time is set as a proportion of the counter range. When the > > period > > changes from 5000 to 10000, then 5000*20%/5000 and 10000*20%/10000 > > will result in the same 20% ratio (disregarding rounding errors). > > > > This is documented at the beginning of the driver: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-pca9685.c?h=v5.6#n25 > > > > Should we move that comment to pwm_config(), so future versions of > > ourselves won't overlook it? > > You are right, this results in the same ratio - the absolute on/off > times may be wrong for a moment though when the period is changed. > > In the attached image, I have changed the period, but kept the > absolute > duty cycle fixed (note: this is in inverted mode, so the duty cycle > controls the low time). It can be seen that after a too long high > time > (chip is in sleep mode) one period with too long low time follows > (new > period, old relative duty cycle), before the counter is reprogrammed > to > match the previous absolute duty cycle. > > I don't care too much about the details of the behaviour, as I only > control LEDs using this chip and don't need to change the period > after > initial setup, but we should accurately document the shortcomings of > the hardware and the driver (when we have decided how to fix some of > the driver issues). > > Matthias And another kind of race condition that should be possible, although I haven't seen it in practice: High and low bits of the OFF counter currently aren't programmed atomically, so with unlucky timing we get a cycle using new lower 8 bits with old upper 4 bits of the duty cycle.
On Mon, Apr 6, 2020 at 5:51 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > There are other chips where a single period is shared across multiple > PWM channels. Typically what we do there is once a period is configured > for a given channel, all subsequent PWM channel configurations must use > the same period, or otherwise the driver will return an error code. Thank you for the clarification Thierry. Do you think this method would be appropriate for this chip?
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c index b07bdca3d510..19ac97108a64 100644 --- a/drivers/pwm/pwm-pca9685.c +++ b/drivers/pwm/pwm-pca9685.c @@ -69,7 +69,6 @@ struct pca9685 { struct pwm_chip chip; struct regmap *regmap; - int duty_ns; int period_ns; #if IS_ENABLED(CONFIG_GPIOLIB) struct mutex lock; @@ -272,8 +271,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, } } - pca->duty_ns = duty_ns; - if (duty_ns < 1) { if (pwm->hwpwm >= PCA9685_MAXCHAN) reg = PCA9685_ALL_LED_OFF_H; @@ -449,7 +446,6 @@ static int pca9685_pwm_probe(struct i2c_client *client, ret); return ret; } - pca->duty_ns = 0; pca->period_ns = PCA9685_DEFAULT_PERIOD; i2c_set_clientdata(client, pca);
duty_cycle was only set, never read. Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- drivers/pwm/pwm-pca9685.c | 4 ---- 1 file changed, 4 deletions(-)