[PATCH AUTOSEL 6.1 24/51] pwm: sifive: Fix PWM algorithm and clarify inverted compare behavior

[Uwe Kleine-König]

Hello,

On Sun, Aug 03, 2025 at 08:36:16PM -0400, Sasha Levin wrote:
> From: Nylon Chen <nylon.chen at sifive.com>
> 
> [ Upstream commit 7dbc4432ea6bf9d709391eb57f1e9fb44e99845a ]
> 
> The `frac` variable represents the pulse inactive time, and the result
> of this algorithm is the pulse active time. Therefore, we must reverse
> the result.
> 
> Although the SiFive Reference Manual states "pwms >= pwmcmpX -> HIGH",
> the hardware behavior is inverted due to a fixed XNOR with 0. As a result,
> the pwmcmp register actually defines the low (inactive) portion of the pulse.
> 
> The reference is SiFive FU740-C000 Manual[0]
> 
> Link: https://sifive.cdn.prismic.io/sifive/1a82e600-1f93-4f41-b2d8-86ed8b16acba_fu740-c000-manual-v1p6.pdf [0]
> 
> Co-developed-by: Zong Li <zong.li at sifive.com>
> Signed-off-by: Zong Li <zong.li at sifive.com>
> Co-developed-by: Vincent Chen <vincent.chen at sifive.com>
> Signed-off-by: Vincent Chen <vincent.chen at sifive.com>
> Signed-off-by: Nylon Chen <nylon.chen at sifive.com>
> Link: https://lore.kernel.org/r/20250529035341.51736-3-nylon.chen@sifive.com
> Signed-off-by: Uwe Kleine-König <ukleinek at kernel.org>
> Signed-off-by: Sasha Levin <sashal at kernel.org>
> ---
> 
> LLM Generated explanations, may be completely bogus:
> 
> **Backport Status: YES**
> 
> This commit should be backported to stable kernel trees. Here's my
> extensive analysis:
> 
> ## Critical Bug Fix Analysis
> 
> 1. **The commit fixes a significant functional bug** - The PWM algorithm
>    was fundamentally broken:
>    - The `frac` variable was incorrectly used (lines 158-161 in
>      original, now 168-171)
>    - The driver was writing the duty cycle value directly to PWMCMP
>      register (line 193), but the hardware actually expects the
>      **inactive** (low) time
>    - This results in inverted PWM output behavior where users get the
>      opposite duty cycle from what they request
> 
> 2. **Hardware behavior discrepancy** - The commit reveals and fixes a
>    critical mismatch between documentation and actual hardware:
>    - The documentation states "pwms >= pwmcmpX -> HIGH"
>    - But the hardware has a hard-tied XNOR with 0 that inverts this
>      behavior
>    - The driver now correctly compensates for this hardware quirk
> 
> 3. **User-visible impact**:
>    - **In `pwm_sifive_get_state()`**: The driver now correctly inverts
>      the value read from hardware (lines 115->122-126)
>    - **In `pwm_sifive_apply()`**: The driver now inverts the duty cycle
>      before writing to hardware (lines 162->171)
>    - **Polarity change**: Changed from `PWM_POLARITY_INVERSED` to
>      `PWM_POLARITY_NORMAL` (lines 126->137, 142->152)
>    - **Documentation fix**: Updated comment from "cannot generate 100%
>      duty" to "cannot generate 0% duty" (lines 11->27, 160->170)
> 
> 4. **The fix is relatively small and contained**:
>    - Changes are isolated to the PWM algorithm logic
>    - No architectural changes or new features
>    - Simple mathematical inversion: `duty = (1U << PWM_SIFIVE_CMPWIDTH)
>      - 1 - inactive`
> 
> 5. **No risky side effects**:
>    - The change is straightforward and mathematically correct
>    - Doesn't affect other subsystems
>    - Maintains the same register interface
> 
> 6. **Affects all SiFive PWM users**:
>    - Any system using SiFive FU540 or FU740 SoCs would have incorrect
>      PWM output
>    - This includes various RISC-V development boards and embedded
>      systems
>    - Users would get inverted duty cycles, potentially breaking motor
>      controls, LED dimming, etc.
> 
> 7. **Clear documentation reference**:
>    - The commit references the official SiFive FU740-C000 Manual
>    - Provides clear explanation of the hardware behavior mismatch
> 
> The bug causes PWM outputs to be inverted from user expectations, which
> is a significant functional issue that would affect any system relying
> on proper PWM behavior for motor control, LED dimming, or other PWM-
> dependent functionality. The fix is clean, minimal, and addresses a
> clear hardware/software mismatch that exists in production systems.

What your LLM missed is that the device trees using this PWM relied on
this "bug" and so this commit should be applied either together with
f4bcf818e5d6 ("riscv: dts: sifive: unleashed/unmatched: Remove PWM
controlled LED's active-low properties") or not at all.

Given that there might be device trees in use that are not in mainline
and that break in the same way without a possiblity for us to fix that I
tend to prefer not to backport this breaking change to stable.

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-riscv/attachments/20250804/baa11852/attachment.sig>