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

[Sasha Levin <sashal@kernel.org>]

On Mon, Aug 04, 2025 at 12:38:15PM +0200, Uwe Kleine-König wrote:
>Hello,
>
>On Sun, Aug 03, 2025 at 08:36:16PM -0400, Sasha Levin wrote:
>> From: Nylon Chen <nylon.chen@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@sifive.com>
>> Signed-off-by: Zong Li <zong.li@sifive.com>
>> Co-developed-by: Vincent Chen <vincent.chen@sifive.com>
>> Signed-off-by: Vincent Chen <vincent.chen@sifive.com>
>> Signed-off-by: Nylon Chen <nylon.chen@sifive.com>
>> Link: https://lore.kernel.org/r/20250529035341.51736-3-nylon.chen@sifive.com
>> Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
>> Signed-off-by: Sasha Levin <sashal@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.

Ack, I'll drop it. Thanks!

-- 
Thanks,
Sasha