PWM waveform, axi-pwmgen backport #2640
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
threexc
requested review from
nunojsa,
mhennerich,
dbogdan,
btogorean,
chegbeli,
buha,
amiclaus,
rbolboac,
danmois,
CiprianRegus and
machschmitt
as code owners
threexc
changed the title
|
The backport broke the pulsar driver. These amount of backports in subsystems is a bit unsettling but I'll keep an open mind (also because a lot of the new stuff we have in the pipeline depend on the new PWM stuff). Oh well, hopefully this does not bring too much churn in my next merge. |
|
Sorry about that. I'll see if I can fix it up, since I have some cleanup to do in the commit messages anyway. |
threexc
force-pushed
the
pwm_waveform_backport
branch
from
49dec64 to
5bb9ba4
Compare
|
Updated the PR with:
For the last point, I can confirm they both build OK, but I don't have the specific hardware to test them on. |
This isn't finalized upstream yet, so probably best to not backport (unless we really need it?).
IIRC, this one should be similar to ad7625 that you worked on? So should be easy to convert to use the new waveform stuff.
Having worked on similar chips (ad7944, ad4000), I have strong doubts that this actually is using the phase feature. There is only one PWM used by the driver, so it doesn't seem like phase is actually doing anything there. Likely we can just delete that line. |
|
Found one more driver using phase: ad4630. That one uses 2 PWMs so we should convert it to the waveform APIs. |
|
FYI: Will be returning to this shortly, just getting some patches for the ad4695 driver ready to go upstream first. |
threexc
force-pushed
the
pwm_waveform_backport
branch
from
5bb9ba4 to
b10a8f8
Compare
|
Rebased, not ready for review again yet. |
threexc
force-pushed
the
pwm_waveform_backport
branch
2 times, most recently
from
f68d9d3 to
928f828
Compare
|
Rebased again, removed the old patch that preserved the phase field, and added three new patches to adjust the |
You're right, this probably isn't needed, but I was using it to test the basic PWM functionality. I'll remove it before marking this as ready for review again. |
threexc
force-pushed
the
pwm_waveform_backport
branch
4 times, most recently
from
d547156 to
37f8442
Compare
|
@nunojsa @dlech let me know what you think of the latest ad4630 waveform version - this makes it behave similarly to how the ad7625 does upstream (and eventually in the backport), but I've omitted calls to Still trying to avoid making too many changes compared to baseline, but if you think it needs to be more drastic I can do that. |
dlech
reviewed
Dec 13, 2024
threexc
force-pushed
the
pwm_waveform_backport
branch
from
78e2afa to
8ea2bf4
Compare
dlech
reviewed
Dec 13, 2024
threexc
force-pushed
the
pwm_waveform_backport
branch
2 times, most recently
from
c028d14 to
e2c953d
Compare
ukleinek
reviewed
Dec 16, 2024
6 tasks
This should help reduce future merge conflicts as more gets backported from upstream, since backporting the rename from there doesn't apply cleanly and might cause issues with other backports. Signed-off-by: Trevor Gamblin <[email protected]>
core.c includes sysfs.c logic now, so remove the latter. Signed-off-by: Trevor Gamblin <[email protected]>
Add the tracing changes to match the core logic improvements in the previous patch. Signed-off-by: Trevor Gamblin <[email protected]>
This allows to simplify drivers that use clk_rate_exclusive_get() in their probe routine as calling clk_rate_exclusive_put() is cared for automatically. Signed-off-by: Uwe Kleine-König <[email protected]> Link: https://lore.kernel.org/r/[email protected] Acked-by: Russell King (Oracle) <[email protected]> Signed-off-by: Stephen Boyd <[email protected]> Backport from upstream so that the axi-pwmgen driver can continue making use of it. Signed-off-by: Trevor Gamblin <[email protected]> Origin: v6.12-rc5 commit: b0cde62
Signed-off-by: Trevor Gamblin <[email protected]>
This ensures that a pwm_chip that has no corresponding driver isn't used and that a driver doesn't go away while a callback is still running. In the presence of device links this isn't necessary yet (so this is no fix) but for pwm character device support this is needed. To not serialize all pwm_apply_state() calls, this introduces a per chip lock. An additional complication is that for atomic chips a mutex cannot be used (as pwm_apply_atomic() must not sleep) and a spinlock cannot be held while calling an operation for a sleeping chip. So depending on the chip being atomic or not a spinlock or a mutex is used. An additional change implemented here is that on driver remove the .free() callback is called for each requested pwm_device. This is the right time because later (e.g. when the consumer calls pwm_put()) the free function is (maybe) not available any more. Signed-off-by: Uwe Kleine-König <[email protected]> Link: https://lore.kernel.org/r/026aa891c8270a11723a1ba7e4256f456f7e1e86.1726819463.git.u.kleine-koenig@baylibre.com Signed-off-by: Uwe Kleine-König <[email protected]> Backport from upstream and adjust to apply without the __counted_by() macro. Signed-off-by: Trevor Gamblin <[email protected]> Origin: pwm/duty_offset-for-6.13-rc1 commit: 1cc2e1f
Up to now the configuration of a PWM setting is described exclusively by a struct pwm_state which contains information about period, duty_cycle, polarity and if the PWM is enabled. (There is another member usage_power which doesn't completely fit into pwm_state, I ignore it here for simplicity.) Instead of a polarity the new abstraction has a member duty_offset_ns that defines when the rising edge happens after the period start. This is more general, as with a pwm_state the rising edge can only happen at the period's start or such that the falling edge is at the end of the period (i.e. duty_offset_ns == 0 or duty_offset_ns == period_length_ns - duty_length_ns). A disabled PWM is modeled by .period_length_ns = 0. In my eyes this is a nice usage of that otherwise unusable setting, as it doesn't define anything about the future which matches the fact that consumers should consider the state of the output as undefined and it's just there to say "No further requirements about the output, you can save some power.". Further I renamed period and duty_cycle to period_length_ns and duty_length_ns. In the past there was confusion from time to time about duty_cycle being measured in nanoseconds because people expected a percentage of period instead. With "length_ns" as suffix the semantic should be more obvious to people unfamiliar with the pwm subsystem. period is renamed to period_length_ns for consistency. The API for consumers doesn't change yet, but lowlevel drivers can implement callbacks that work with pwm_waveforms instead of pwm_states. A new thing about these callbacks is that the calculation of hardware settings needed to implement a certain waveform is separated from actually writing these settings. The motivation for that is that this allows a consumer to query the hardware capabilities without actually modifying the hardware state. The rounding rules that are expected to be implemented in the round_waveform_tohw() are: First pick the biggest possible period not bigger than wf->period_length_ns. For that period pick the biggest possible duty setting not bigger than wf->duty_length_ns. Third pick the biggest possible offset not bigger than wf->duty_offset_ns. If the requested period is too small for the hardware, it's expected that a setting with the minimal period and duty_length_ns = duty_offset_ns = 0 is returned and this fact is signaled by a return value of 1. Signed-off-by: Uwe Kleine-König <[email protected]> Tested-by: Trevor Gamblin <[email protected]> Link: https://lore.kernel.org/r/df0faa33bf9e7c9e2e5eab8d31bbf61e861bd401.1726819463.git.u.kleine-koenig@baylibre.com [ukleinek: Update pwm_check_rounding() to return bool instead of int.] Signed-off-by: Uwe Kleine-König <[email protected]> Signed-off-by: Trevor Gamblin <[email protected]> Origin: pwm/duty_offset-for-6.13-rc1, commit: 17e40c2
Provide API functions for consumers to work with waveforms. Note that one relevant difference between pwm_get_state() and pwm_get_waveform*() is that the latter yields the actually configured hardware state, while the former yields the last state passed to pwm_apply*() and so doesn't account for hardware specific rounding. Signed-off-by: Uwe Kleine-König <[email protected]> Tested-by: Trevor Gamblin <[email protected]> Link: https://lore.kernel.org/r/6c97d27682853f603e18e9196043886dd671845d.1726819463.git.u.kleine-koenig@baylibre.com Signed-off-by: Uwe Kleine-König <[email protected]> Signed-off-by: Trevor Gamblin <[email protected]> Origin: pwm/duty_offset-for-6.13-rc1, commit: 6c5126c
This adds trace events for the recently introduced waveform callbacks. With the introduction of some helper macros consistency among the different events is ensured. Signed-off-by: Uwe Kleine-König <[email protected]> Link: https://lore.kernel.org/r/1d71879b0de3bf01459c7a9d0f040d43eb5ace56.1726819463.git.u.kleine-koenig@baylibre.com Signed-off-by: Uwe Kleine-König <[email protected]> Signed-off-by: Trevor Gamblin <[email protected]> Origin: pwm/duty_offset-for-6.13-rc1, commit: 1afd01d
Convert the axi-pwmgen driver to use the new callbacks for hardware programming. Signed-off-by: Uwe Kleine-König <[email protected]> Tested-by: Trevor Gamblin <[email protected]> Link: https://lore.kernel.org/r/922277f07b1d1fb9c9cd915b1ec3fdeec888a916.1726819463.git.u.kleine-koenig@baylibre.com Signed-off-by: Uwe Kleine-König <[email protected]> Signed-off-by: Trevor Gamblin <[email protected]> Origin: pwm/duty_offset-for-6.13-rc1, commit: eb18504
This moves pwm_get() and friends above the functions handling registration of pwmchips. The motivation is that character device support needs pwm_get() and pwm_put() and so ideally is defined below these and when a pwmchip is registered this registers the character device. So the natural order is pwm_get() and friend pwm character device symbols pwm_chip functions . The advantage of having these in their natural order is that static functions don't need to be forward declared. Note that the diff that git produces for this change some functions are moved down instead. This is technically equivalent, but not how this change was created. Signed-off-by: Uwe Kleine-König <[email protected]> Link: https://lore.kernel.org/r/193b3d933294da34e020650bff93b778de46b1c5.1726819463.git.u.kleine-koenig@baylibre.com Signed-off-by: Uwe Kleine-König <[email protected]> Signed-off-by: Trevor Gamblin <[email protected]> Origin: pwm/duty_offset-for-6.13-rc1, commit: 65406de
The callbacks for lowlevel pwm drivers were expanded to handle the new waveform abstraction. When doing that I missed to expand the kernel doc description. This is catched up here. Reported-by: Stephen Rothwell <[email protected]> Link: https://lore.kernel.org/linux-next/[email protected] Fixes: 17e40c2 ("pwm: New abstraction for PWM waveforms") Signed-off-by: Uwe Kleine-König <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Uwe Kleine-König <[email protected]> Signed-off-by: Trevor Gamblin <[email protected]> Origin: pwm/duty_offset-for-6.13-rc1, commit: dab9cd4
…from a chip Compared to direct calls to pwmchip_get_drvdata() a dedicated function has two upsides: A better name and the right type. So the code becomes easier to read and the new function is harder to use wrongly. Another side effect (which is the secret motivation for this patch, but shhh) is that the driver becomes a bit easier to backport to kernel versions that don't have devm_pwmchip_alloc() yet. Signed-off-by: Uwe Kleine-König <[email protected]> Reviewed-by: Trevor Gamblin <[email protected]> Link: https://lore.kernel.org/r/[email protected] [ukleinek: added an * to the new function's prototype to make the compiler happy] Signed-off-by: Uwe Kleine-König <[email protected]> Signed-off-by: Trevor Gamblin <[email protected]> Origin: pwm/for-next, commit: 22f032c7900ca457f4facdd711eee61c9648c7c1
Rename the 0x10 register from REG_CONFIG to REG_RSTN. Also rename the associated bit macros accordingly. While touching this, move the bit macros close to the register address macro for better organization. According to [1], the name of the 0x10 register is REG_RSTN, and there is a different register named REG_CONFIG (0x18). So we should not be using REG_CONFIG for the 0x10 register to avoid confusion. [1]: http://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html Signed-off-by: David Lechner <[email protected]> Reviewed-by: Nuno Sa <[email protected]> Link: https://lore.kernel.org/r/20241009-pwm-axi-pwmgen-enable-force_align-v1-1-5d6ad8cbf5b4@baylibre.com Signed-off-by: Uwe Kleine-König <[email protected]> Signed-off-by: Trevor Gamblin <[email protected]> Origin: pwm/for-next, commit: 2e82d58c7ba8f7b4bb273ca5859b682b65654f9e
Enable the FORCE_ALIGN flag by default in the AXI PWMGEN driver. This
flag makes the behavior of the PWM output consistent with the
description at the top of the driver file.
* Limitations:
* - The writes to registers for period and duty are shadowed until
* LOAD_CONFIG is written to AXI_PWMGEN_REG_RSTN, at which point
* they take effect.
* - Writing LOAD_CONFIG also has the effect of re-synchronizing all
* enabled channels, which could cause glitching on other channels. It
* is therefore expected that channels are assigned harmonic periods
* and all have a single user coordinating this.
Without this flag, the PWM output does not change until the period of
all PWM output channels has run out, which makes the PWM impossible to
use in some cases because it takes too long to change the output.
Signed-off-by: David Lechner <[email protected]>
Reviewed-by: Nuno Sa <[email protected]>
Link: https://lore.kernel.org/r/20241009-pwm-axi-pwmgen-enable-force_align-v1-2-5d6ad8cbf5b4@baylibre.com
Signed-off-by: Uwe Kleine-König <[email protected]>
Signed-off-by: Trevor Gamblin <[email protected]>
Origin: pwm/for-next, commit: 15effedc481e69a23b374ba516d73a2bc665abe6
It is being set but likely isn't used, so remove it to avoid build errors now that the new PWM waveform API is included. Signed-off-by: Trevor Gamblin <[email protected]>
Signed-off-by: Trevor Gamblin <[email protected]>
Reserve setting of PWMs for ad4630_buffer_preenable(), and ad4630_update_sample_fetch_trigger(). Disable them with pwm_disable() in two places: ad4630_pwm_get() (so that the PWM outputs aren't on after initialization) and ad4630_buffer_postdisable(). Some instances of the driver state had the const qualifiers removed since they now actually need to be modified. Since the PWM rounding API outputs may vary depending on the reference clock used, add a loop to continually try rounding the conversion waveform until an appropriate duty_length_ns value is obtained. Change AD4630_TQUIET_CNV_DELAY_PS to AD4630_TQUIET_CNV_DELAY_NS and the nearest appropriate value, so that we don't have to round when calculating PWM offsets now. Do the same rounding process for the fetch waveform's duty_offset_ns as for the conv waveform's duty_length_ns. Also add a bit more logic to buffer_preenable() to solve a bug where we weren't re-entering config mode on error if the write for exiting the mode failed. Signed-off-by: Trevor Gamblin <[email protected]>
threexc
force-pushed
the
pwm_waveform_backport
branch
from
e2c953d to
10ec3a6
Compare
|
Addressed everything except for the EDIT: It was because I was only looking in the current form of the code, and it had been removed in the patch set :) |
|
Looks like there are more issues with other PWM devices I'll have to fix. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Description
This PR includes several backports from upstream v6.12 and v6.13 branches around new PWM consumer APIs and waveform support, in preparation for the subsequent backport of the upstream ad7625 driver. Highlights:
pwm_apply_state()topwm_apply_might_sleep()pwm/core.c,include/linux/pwm.h,include/trace/events/pwm.hdrivers/pwm/sysfs.c(now incorporated indrivers/pwm/core.c)devm_clk_rate_exclusive_get()additiondrivers/pwm/pwm-axi-pwmgen.cpwm/core.csymbol reorderingThis has been tested locally using a Zedboard with a customized BOOT.BIN to route PWM signals to a PMOD header and the libpwm tooling found at https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git/, which allows setting PWM waveform properties via the newly-added character device interface.
PR Type
PR Checklist