Re: [PATCH] riscv: dts: spacemit: k3: Add pwm support

Hi

On 23:59 Mon 11 May     , [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Device tree node name collision causes silent overwrite of PWM0 pin configuration.
> - [Medium] PWM nodes specify two clocks, violating the schema for the fallback compatible and leaving the bus clock unmanaged.
> - [Low] Widespread copy-paste errors in pinctrl child node names.
> --
> 
> commit 9a4ae96f46c2dc497874c74f2a45fcc9d68ce685
> Author: Yixun Lan <[email protected]>
> riscv: dts: spacemit: k3: Add pwm support
> 
> This commit populates the PWM device tree nodes for the SpacemiT K3 SoC.
> It also adds the corresponding pinctrl configurations to enable PWM support
> in the future.
> 
> > diff --git a/arch/riscv/boot/dts/spacemit/k3-pinctrl.dtsi b/arch/riscv/boot/dts/spacemit/k3-pinctrl.dtsi
> > index 23899d3f308a0..1fd39502071be 100644
> > --- a/arch/riscv/boot/dts/spacemit/k3-pinctrl.dtsi
> > +++ b/arch/riscv/boot/dts/spacemit/k3-pinctrl.dtsi
> > @@ -56,6 +56,596 @@ i2c8-pins {
> >  		};
> >  	};
> >  
> > +	/omit-if-no-ref/
> > +	pwm0_0_cfg: pwm0-0-cfg {
> > +		pwm0-0-pins {
> > +			pinmux = <K3_PADCONF(0, 3)>;	/* pwm0 */
> > +
> > +			bias-pull-up = <0>;
> > +			drive-strength = <25>;
> > +		};
> > +	};
> 
> [ ... ]
> 
> > +	/omit-if-no-ref/
> > +	pwm1_0_cfg: pwm1-0-cfg {
> > +		pwm1-0-pins {
> > +			pinmux = <K3_PADCONF(1, 3)>;	/* pwm1 */
> > +
> > +			bias-pull-up = <0>;
> > +			drive-strength = <25>;
> > +		};
> > +	};
> > +
> > +	/omit-if-no-ref/
> > +	pwm1_1_cfg: pwm0-0-cfg {
> 
> Will this cause a node name collision that silently overwrites the PWM0 pin
> configuration?
> 
> The Device Tree Compiler merges nodes with identical names at the same level.
> Because this is named pwm0-0-cfg instead of pwm1-1-cfg, the pinmux property
> of pwm0_0_cfg will be overwritten by this node. If a board device tree
> references pwm0_0_cfg, will it receive the pin configuration for PWM1 instead
> of PWM0?
> 
> > +		pwm0-0-pins {
> 
> This isn't a bug, but there appear to be widespread copy-paste errors in the
> pinctrl child node names.
> 
Yes..
> For example, pwm1_1_cfg uses pwm0-0-pins, pwm1_2_cfg uses pwm1-0-pins, and
> pwm2_1_cfg uses pwm2-0-pins. While this does not cause functional issues, it
> creates a confusing device tree structure.
> 
right, this is a typo, will fix
> > +			pinmux = <K3_PADCONF(43, 6)>;	/* pwm1 */
> > +
> > +			bias-pull-up = <0>;
> > +			drive-strength = <25>;
> > +		};
> > +	};
> 
> [ ... ]
> 
> > diff --git a/arch/riscv/boot/dts/spacemit/k3.dtsi b/arch/riscv/boot/dts/spacemit/k3.dtsi
> > index 815debd16409b..07e43b6b256ec 100644
> > --- a/arch/riscv/boot/dts/spacemit/k3.dtsi
> > +++ b/arch/riscv/boot/dts/spacemit/k3.dtsi
> > @@ -797,6 +797,226 @@ i2c8: i2c@d401d800 {
> >  			status = "disabled";
> >  		};
> >  
> > +		pwm0: pwm@d401a000 {
> > +			compatible = "spacemit,k3-pwm", "marvell,pxa910-pwm";
> > +			reg = <0x0 0xd401a000 0x0 0x10>;
> > +			clocks = <&syscon_apbc CLK_APBC_PWM0>,
> > +				 <&syscon_apbc CLK_APBC_PWM0_BUS>;
> > +			clock-names = "func", "bus";
> 
> Could these clock definitions violate the schema for the fallback compatible
> and leave the bus clock unmanaged?
> 
> The dt-binding for marvell,pxa-pwm.yaml restricts clocks to maxItems: 1 and
> prohibits clock-names.
> 
> Additionally, the fallback driver (drivers/pwm/pwm-pxa.c) calls
> devm_clk_get(dev, NULL), which will only acquire the first func clock. If the
> SoC requires the APB bus clock to be explicitly enabled for register access,
> could this result in a bus fault when the fallback driver probes?
> 
I think this is false positive error, which due to the pwm driver change for
K3 was not merged into mainline.. will ping pwm maintainer for inclusion

https://lore.kernel.org/r/[email protected]

> > +			resets = <&syscon_apbc RESET_APBC_PWM0>;
> > +			#pwm-cells = <3>;
> > +			status = "disabled";
> > +		};
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

-- 
Yixun Lan (dlan)