Re: Re: [PATCH v14 3/5] pwm: dwc: add of/platform support

[Xuyang Dong]

> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] dwc_pwm_runtime_resume() writes the EN bit and timer configuration simultaneously, violating the hardware specification.
> --
> 
> --- Patch [3]: [PATCH v14 3/5] pwm: dwc: add of/platform support ---
> commit b441231de2fcc89e4187b672bde1919622ac7347
> Author: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> 
> pwm: dwc: add of/platform support
> 
> This commit adds platform and OF device tree support for the DesignWare PWM
> controller, enabling its use in non-PCI systems. It also implements polarity
> control using dual load registers and adds runtime PM callbacks to save and
> restore hardware state.
> 
> > diff --git a/drivers/pwm/pwm-dwc-of.c b/drivers/pwm/pwm-dwc-of.c
> > new file mode 100644
> > index 0000000000000..e96e5569eb81e
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-dwc-of.c
> [ ... ]
> > +static int dwc_pwm_runtime_resume(struct device *dev)
> > +{
> [ ... ]
> > +	for (idx = 0; idx < chip->npwm; idx++) {
> > +		dwc_pwm_writel(dwc, dwc->ctx[idx].cnt, DWC_TIM_LD_CNT(idx));
> > +		dwc_pwm_writel(dwc, dwc->ctx[idx].cnt2, DWC_TIM_LD_CNT2(idx));
> > +		dwc_pwm_writel(dwc, dwc->ctx[idx].ctrl, DWC_TIM_CTRL(idx));
> 
> [Severity: Medium]
> Does writing the saved control register directly in dwc_pwm_runtime_resume()
> violate the hardware initialization sequence?
> 

Hi Sashiko,

No, because if the Load Count register is written after the control 
register, it will be updated after the timer is enabled.

> The driver notes in __dwc_pwm_configure_timer() that the specification
> requires disabling the timer, programming it, and then enabling it.
> 
> Since ctx[idx].ctrl may have the DWC_TIM_CTRL_EN bit set from when the device
> was active, does this single write attempt to configure and enable the timer
> simultaneously? Could this cause unpredictable behavior upon resume?
> 

Although the single write attempts to configure and enable the timer 
simultaneously, the Load Count registers are updated before the timer is 
enabled. Therefore, this approach will not cause unpredictable behavior.

Best regards,
Xuyang Dong

> > +	}
> > +
> > +	return 0;
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260730111425.1680-1-dongxuyang@xxxxxxxxxxxxxxxxxx?part=3