diff mbox series

[v1,03/11] dt-bindings: pwm: rockchip: add rockchip,rk3128-pwm

Message ID f5dd0ee4-d97e-d878-ffde-c06e9b233e38@gmail.com
State Accepted
Headers show
Series Add more Rockchip rk3128 compatible strings | expand

Commit Message

Johan Jonker Sept. 9, 2022, 10:02 p.m. UTC
Add rockchip,rk3128-pwm compatible string.

Signed-off-by: Johan Jonker <jbx6244@gmail.com>
---
 Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml | 1 +
 1 file changed, 1 insertion(+)

Comments

Rob Herring Sept. 10, 2022, 2:53 p.m. UTC | #1
On Sat, 10 Sep 2022 00:02:22 +0200, Johan Jonker wrote:
> Add rockchip,rk3128-pwm compatible string.
> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/


pwm@10280000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm/boot/dts/rv1108-elgin-r1.dtb
	arch/arm/boot/dts/rv1108-evb.dtb

pwm@10280010: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm/boot/dts/rv1108-elgin-r1.dtb
	arch/arm/boot/dts/rv1108-evb.dtb

pwm@10280020: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm/boot/dts/rv1108-elgin-r1.dtb
	arch/arm/boot/dts/rv1108-evb.dtb

pwm@10280030: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm/boot/dts/rv1108-elgin-r1.dtb
	arch/arm/boot/dts/rv1108-evb.dtb

pwm@20040000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm/boot/dts/rv1108-elgin-r1.dtb
	arch/arm/boot/dts/rv1108-evb.dtb

pwm@20040010: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm/boot/dts/rv1108-elgin-r1.dtb
	arch/arm/boot/dts/rv1108-evb.dtb

pwm@20040020: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm/boot/dts/rv1108-elgin-r1.dtb
	arch/arm/boot/dts/rv1108-evb.dtb

pwm@20040030: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm/boot/dts/rv1108-elgin-r1.dtb
	arch/arm/boot/dts/rv1108-evb.dtb

pwm@ff1b0030: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
	arch/arm64/boot/dts/rockchip/rk3318-a95x-z2.dtb
	arch/arm64/boot/dts/rockchip/rk3328-a1.dtb
	arch/arm64/boot/dts/rockchip/rk3328-evb.dtb
	arch/arm64/boot/dts/rockchip/rk3328-nanopi-r2s.dtb
	arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dtb
	arch/arm64/boot/dts/rockchip/rk3328-rock64.dtb
	arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dtb
	arch/arm64/boot/dts/rockchip/rk3328-roc-pc.dtb
Johan Jonker Sept. 10, 2022, 7:48 p.m. UTC | #2
Reduced CC.

Hi Rob,

The rk3328 and rv1108 PWM interrupt is chaired between blocks I think.
For rv1108 the same interrupt is used for all PWM nodes.
For rk3328 only added to one PWM node.
Currently not in use in a Linux drivers??

No consensus yet...on removing or parent node, so it stays as it is...
Maybe if you have ideas things will change. ;)

Johan

===

See discussion:
https://lore.kernel.org/linux-rockchip/20b7c702-9412-93b4-3174-e8633bc413d7@gmail.com/ 

On 9/10/22 16:53, Rob Herring wrote:
> On Sat, 10 Sep 2022 00:02:22 +0200, Johan Jonker wrote:
>> Add rockchip,rk3128-pwm compatible string.
>>
>> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
>> ---
>>  Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml | 1 +
>>  1 file changed, 1 insertion(+)
>>
> 
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.
> 
> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> This will change in the future.
> 
> Full log is available here: https://patchwork.ozlabs.org/patch/
> 
> 
> pwm@10280000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	arch/arm/boot/dts/rv1108-elgin-r1.dtb
> 	arch/arm/boot/dts/rv1108-evb.dtb
> 
> pwm@10280010: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	arch/arm/boot/dts/rv1108-elgin-r1.dtb
> 	arch/arm/boot/dts/rv1108-evb.dtb
> 
> pwm@10280020: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	arch/arm/boot/dts/rv1108-elgin-r1.dtb
> 	arch/arm/boot/dts/rv1108-evb.dtb
> 
> pwm@10280030: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	arch/arm/boot/dts/rv1108-elgin-r1.dtb
> 	arch/arm/boot/dts/rv1108-evb.dtb
> 
> pwm@20040000: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	arch/arm/boot/dts/rv1108-elgin-r1.dtb
> 	arch/arm/boot/dts/rv1108-evb.dtb
> 
> pwm@20040010: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	arch/arm/boot/dts/rv1108-elgin-r1.dtb
> 	arch/arm/boot/dts/rv1108-evb.dtb
> 
> pwm@20040020: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	arch/arm/boot/dts/rv1108-elgin-r1.dtb
> 	arch/arm/boot/dts/rv1108-evb.dtb
> 
> pwm@20040030: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	arch/arm/boot/dts/rv1108-elgin-r1.dtb
> 	arch/arm/boot/dts/rv1108-evb.dtb
> 
> pwm@ff1b0030: 'interrupts' does not match any of the regexes: 'pinctrl-[0-9]+'
> 	arch/arm64/boot/dts/rockchip/rk3318-a95x-z2.dtb
> 	arch/arm64/boot/dts/rockchip/rk3328-a1.dtb
> 	arch/arm64/boot/dts/rockchip/rk3328-evb.dtb
> 	arch/arm64/boot/dts/rockchip/rk3328-nanopi-r2s.dtb
> 	arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dtb
> 	arch/arm64/boot/dts/rockchip/rk3328-rock64.dtb
> 	arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dtb
> 	arch/arm64/boot/dts/rockchip/rk3328-roc-pc.dtb
>
Uwe Kleine-König Sept. 12, 2022, 8:35 a.m. UTC | #3
Hello,

On Sat, Sep 10, 2022 at 12:02:22AM +0200, Johan Jonker wrote:
> Add rockchip,rk3128-pwm compatible string.
> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>

Considering the problems pointed out by Rob as orthogonal to this
change:

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe
Rob Herring Sept. 12, 2022, 4:21 p.m. UTC | #4
On Sat, Sep 10, 2022 at 09:48:04PM +0200, Johan Jonker wrote:
> Reduced CC.
> 
> Hi Rob,
> 

Seemed like a simple enough warning to fix...

> The rk3328 and rv1108 PWM interrupt is chaired between blocks I think.
> For rv1108 the same interrupt is used for all PWM nodes.
> For rk3328 only added to one PWM node.
> Currently not in use in a Linux drivers??

How is that relevant to the binding? It's used in dts files.

> 
> No consensus yet...on removing or parent node, so it stays as it is...
> Maybe if you have ideas things will change. ;)

Only that existing issues should be addressed before adding new 
platforms especially if the binding might change in an incompatible way 
(splitting nodes).

Rob
Rob Herring Sept. 13, 2022, 2:26 p.m. UTC | #5
On Sat, 10 Sep 2022 00:02:22 +0200, Johan Jonker wrote:
> Add rockchip,rk3128-pwm compatible string.
> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>
Johan Jonker Sept. 13, 2022, 2:38 p.m. UTC | #6
On 9/12/22 18:21, Rob Herring wrote:
> On Sat, Sep 10, 2022 at 09:48:04PM +0200, Johan Jonker wrote:
>> Reduced CC.
>>
>> Hi Rob,
>>
> 
> Seemed like a simple enough warning to fix...

Some examples for comment.
Let us know what would be the better solution?

===========================================================================

option1:

	combpwm0: combpwm0 {
		compatible = "rockchip,rv1108-combpwm";
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
		#address-cells = <2>;
		#size-cells = <2>;

		pwm0: pwm@20040000 {
			compatible = "rockchip,rv1108-pwm";
			reg = <0x20040000 0x10>;
		};

		pwm1: pwm@20040010 {
			compatible = "rockchip,rv1108-pwm";
			reg = <0x20040010 0x10>;
		};

		pwm2: pwm@20040020 {
			compatible = "rockchip,rv1108-pwm";
			reg = <0x20040020 0x10>;
		};

		pwm3: pwm@20040030 {
			compatible = "rockchip,rv1108-pwm";
			reg = <0x20040030 0x10>;
		};
	};

PRO:
- Existing driver might still work.
CON:
- New compatible needed to service the combined interrupts.
- Driver change needed.

===========================================================================
option 2:

	combpwm0: pwm@10280000 {
		compatible = "rockchip,rv1108-pwm";
		reg = <0x10280000 0x40>;
		interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
		#address-cells = <1>;
		#size-cells = <0>;

		pwm4: pwm-4@0 {
			reg = <0x0>;
		};

		pwm5: pwm-5@10 {
			reg = <0x10>;
		};

		pwm6: pwm-6@20 {
			reg = <0x20>;
		};

		pwm7: pwm-7@30 {
			reg = <0x30>;
		};
	};

CON:
- Driver change needed.
- Not compatible with current drivers.

===========================================================================

Current situation:

	pwm0: pwm@20040000 {
		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
		reg = <0x20040000 0x10>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
	};

	pwm1: pwm@20040010 {
		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
		reg = <0x20040010 0x10>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
	};

	pwm2: pwm@20040020 {
		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
		reg = <0x20040020 0x10>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
	};

	pwm3: pwm@20040030 {
		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
		reg = <0x20040030 0x10>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
	};

CON:
- The property "interrupts 39" can only be claimed ones by one probe function at the time.
- Has a fall-back string for rk3288, but unknown identical behavior for interrupts ???


> 
>> The rk3328 and rv1108 PWM interrupt is chaired between blocks I think.
>> For rv1108 the same interrupt is used for all PWM nodes.
>> For rk3328 only added to one PWM node.
>> Currently not in use in a Linux drivers??
> 
> How is that relevant to the binding? It's used in dts files.
> 
>>
>> No consensus yet...on removing or parent node, so it stays as it is...
>> Maybe if you have ideas things will change. ;)
> 
> Only that existing issues should be addressed before adding new 
> platforms especially if the binding might change in an incompatible way 
> (splitting nodes).
> 
> Rob
Uwe Kleine-König Sept. 20, 2022, 6:21 a.m. UTC | #7
Hello,

On Sat, Sep 10, 2022 at 12:02:22AM +0200, Johan Jonker wrote:
> Add rockchip,rk3128-pwm compatible string.
> 
> Signed-off-by: Johan Jonker <jbx6244@gmail.com>

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Is the expectation that this goes in via PWM, or together with the other
patches via the rockchip maintainers?

Best regards
Uwe
Heiko Stübner Sept. 23, 2022, 10:22 a.m. UTC | #8
Am Dienstag, 20. September 2022, 08:21:49 CEST schrieb Uwe Kleine-König:
> Hello,
> 
> On Sat, Sep 10, 2022 at 12:02:22AM +0200, Johan Jonker wrote:
> > Add rockchip,rk3128-pwm compatible string.
> > 
> > Signed-off-by: Johan Jonker <jbx6244@gmail.com>
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Is the expectation that this goes in via PWM, or together with the other
> patches via the rockchip maintainers?

in general I think bindings always go through the subsystem that
they're targetting - PWM in this case.

Acked-by: Heiko Stuebner <heiko@sntech.de>


Heiko
Thierry Reding Sept. 28, 2022, 11:59 a.m. UTC | #9
On Tue, Sep 13, 2022 at 04:38:32PM +0200, Johan Jonker wrote:
> 
> 
> On 9/12/22 18:21, Rob Herring wrote:
> > On Sat, Sep 10, 2022 at 09:48:04PM +0200, Johan Jonker wrote:
> >> Reduced CC.
> >>
> >> Hi Rob,
> >>
> > 
> > Seemed like a simple enough warning to fix...
> 
> Some examples for comment.
> Let us know what would be the better solution?
> 
> ===========================================================================
> 
> option1:
> 
> 	combpwm0: combpwm0 {
> 		compatible = "rockchip,rv1108-combpwm";
> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> 		#address-cells = <2>;
> 		#size-cells = <2>;
> 
> 		pwm0: pwm@20040000 {
> 			compatible = "rockchip,rv1108-pwm";
> 			reg = <0x20040000 0x10>;
> 		};
> 
> 		pwm1: pwm@20040010 {
> 			compatible = "rockchip,rv1108-pwm";
> 			reg = <0x20040010 0x10>;
> 		};
> 
> 		pwm2: pwm@20040020 {
> 			compatible = "rockchip,rv1108-pwm";
> 			reg = <0x20040020 0x10>;
> 		};
> 
> 		pwm3: pwm@20040030 {
> 			compatible = "rockchip,rv1108-pwm";
> 			reg = <0x20040030 0x10>;
> 		};
> 	};
> 
> PRO:
> - Existing driver might still work.
> CON:
> - New compatible needed to service the combined interrupts.
> - Driver change needed.
> 
> ===========================================================================
> option 2:
> 
> 	combpwm0: pwm@10280000 {
> 		compatible = "rockchip,rv1108-pwm";
> 		reg = <0x10280000 0x40>;
> 		interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		pwm4: pwm-4@0 {
> 			reg = <0x0>;
> 		};
> 
> 		pwm5: pwm-5@10 {
> 			reg = <0x10>;
> 		};
> 
> 		pwm6: pwm-6@20 {
> 			reg = <0x20>;
> 		};
> 
> 		pwm7: pwm-7@30 {
> 			reg = <0x30>;
> 		};
> 	};
> 
> CON:
> - Driver change needed.
> - Not compatible with current drivers.
> 
> ===========================================================================
> 
> Current situation:
> 
> 	pwm0: pwm@20040000 {
> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> 		reg = <0x20040000 0x10>;
> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> 	pwm1: pwm@20040010 {
> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> 		reg = <0x20040010 0x10>;
> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> 	pwm2: pwm@20040020 {
> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> 		reg = <0x20040020 0x10>;
> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> 	pwm3: pwm@20040030 {
> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> 		reg = <0x20040030 0x10>;
> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 
> CON:
> - The property "interrupts 39" can only be claimed ones by one probe function at the time.
> - Has a fall-back string for rk3288, but unknown identical behavior for interrupts ???

To be honest, all three descriptions look wrong to me. From the above it
looks like this is simply one PWM controller with four channels, so it
should really be described as such, i.e.:

	pwm@20040030 {
		compatible = "rockchip,rv1108-pwm";
		reg = <0x20040030 0x40>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
	};

Looking through existing Rockchip SoC DTSI files, though, it looks like
this has been done the wrong way since the beginning, so not sure if you
still want to fix it up.

This whole problem of dealing with a shared interrupt wouldn't be a
problem if this was described properly.

Thierry
Johan Jonker Sept. 29, 2022, 10:26 a.m. UTC | #10
On 9/28/22 13:59, Thierry Reding wrote:
> On Tue, Sep 13, 2022 at 04:38:32PM +0200, Johan Jonker wrote:
>>
>>
>> On 9/12/22 18:21, Rob Herring wrote:
>>> On Sat, Sep 10, 2022 at 09:48:04PM +0200, Johan Jonker wrote:
>>>> Reduced CC.
>>>>
>>>> Hi Rob,
>>>>
>>>
>>> Seemed like a simple enough warning to fix...
>>
>> Some examples for comment.
>> Let us know what would be the better solution?
>>
>> ===========================================================================
>>
>> option1:
>>
>> 	combpwm0: combpwm0 {
>> 		compatible = "rockchip,rv1108-combpwm";
>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> 		#address-cells = <2>;
>> 		#size-cells = <2>;
>>
>> 		pwm0: pwm@20040000 {
>> 			compatible = "rockchip,rv1108-pwm";
>> 			reg = <0x20040000 0x10>;
>> 		};
>>
>> 		pwm1: pwm@20040010 {
>> 			compatible = "rockchip,rv1108-pwm";
>> 			reg = <0x20040010 0x10>;
>> 		};
>>
>> 		pwm2: pwm@20040020 {
>> 			compatible = "rockchip,rv1108-pwm";
>> 			reg = <0x20040020 0x10>;
>> 		};
>>
>> 		pwm3: pwm@20040030 {
>> 			compatible = "rockchip,rv1108-pwm";
>> 			reg = <0x20040030 0x10>;
>> 		};
>> 	};
>>
>> PRO:
>> - Existing driver might still work.
>> CON:
>> - New compatible needed to service the combined interrupts.
>> - Driver change needed.
>>
>> ===========================================================================
>> option 2:
>>
>> 	combpwm0: pwm@10280000 {
>> 		compatible = "rockchip,rv1108-pwm";
>> 		reg = <0x10280000 0x40>;
>> 		interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>>
>> 		pwm4: pwm-4@0 {
>> 			reg = <0x0>;
>> 		};
>>
>> 		pwm5: pwm-5@10 {
>> 			reg = <0x10>;
>> 		};
>>
>> 		pwm6: pwm-6@20 {
>> 			reg = <0x20>;
>> 		};
>>
>> 		pwm7: pwm-7@30 {
>> 			reg = <0x30>;
>> 		};
>> 	};
>>
>> CON:
>> - Driver change needed.
>> - Not compatible with current drivers.
>>
>> ===========================================================================
>>
>> Current situation:
>>
>> 	pwm0: pwm@20040000 {
>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>> 		reg = <0x20040000 0x10>;
>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> 	};
>>
>> 	pwm1: pwm@20040010 {
>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>> 		reg = <0x20040010 0x10>;
>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> 	};
>>
>> 	pwm2: pwm@20040020 {
>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>> 		reg = <0x20040020 0x10>;
>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> 	};
>>
>> 	pwm3: pwm@20040030 {
>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>> 		reg = <0x20040030 0x10>;
>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> 	};
>>
>> CON:
>> - The property "interrupts 39" can only be claimed ones by one probe function at the time.
>> - Has a fall-back string for rk3288, but unknown identical behavior for interrupts ???
> 

> To be honest, all three descriptions look wrong to me. From the above it
> looks like this is simply one PWM controller with four channels, so it
> should really be described as such, i.e.:
> 
> 	pwm@20040030 {
> 		compatible = "rockchip,rv1108-pwm";
> 		reg = <0x20040030 0x40>;
> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> 	};
> 

Each PWM channel has it's own pinctrl.
Not all channel pins are always in use for PWM exclusively.
Your proposal would not allow pins to be used for other functions.
More ideas with this interrupt? Please advise.

===

The SoCs PWM are configurable to operate in continuous mode (default mainline) or one-shot mode or capture mode.
Is there any good example for one-shot mode interrupt use?


> Looking through existing Rockchip SoC DTSI files, though, it looks like
> this has been done the wrong way since the beginning, so not sure if you
> still want to fix it up.
> 
> This whole problem of dealing with a shared interrupt wouldn't be a
> problem if this was described properly.
> 
> Thierry
Robin Murphy Sept. 29, 2022, 3:41 p.m. UTC | #11
On 2022-09-29 11:26, Johan Jonker wrote:
> 
> 
> On 9/28/22 13:59, Thierry Reding wrote:
>> On Tue, Sep 13, 2022 at 04:38:32PM +0200, Johan Jonker wrote:
>>>
>>>
>>> On 9/12/22 18:21, Rob Herring wrote:
>>>> On Sat, Sep 10, 2022 at 09:48:04PM +0200, Johan Jonker wrote:
>>>>> Reduced CC.
>>>>>
>>>>> Hi Rob,
>>>>>
>>>>
>>>> Seemed like a simple enough warning to fix...
>>>
>>> Some examples for comment.
>>> Let us know what would be the better solution?
>>>
>>> ===========================================================================
>>>
>>> option1:
>>>
>>> 	combpwm0: combpwm0 {
>>> 		compatible = "rockchip,rv1108-combpwm";
>>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>>> 		#address-cells = <2>;
>>> 		#size-cells = <2>;
>>>
>>> 		pwm0: pwm@20040000 {
>>> 			compatible = "rockchip,rv1108-pwm";
>>> 			reg = <0x20040000 0x10>;
>>> 		};
>>>
>>> 		pwm1: pwm@20040010 {
>>> 			compatible = "rockchip,rv1108-pwm";
>>> 			reg = <0x20040010 0x10>;
>>> 		};
>>>
>>> 		pwm2: pwm@20040020 {
>>> 			compatible = "rockchip,rv1108-pwm";
>>> 			reg = <0x20040020 0x10>;
>>> 		};
>>>
>>> 		pwm3: pwm@20040030 {
>>> 			compatible = "rockchip,rv1108-pwm";
>>> 			reg = <0x20040030 0x10>;
>>> 		};
>>> 	};
>>>
>>> PRO:
>>> - Existing driver might still work.
>>> CON:
>>> - New compatible needed to service the combined interrupts.
>>> - Driver change needed.
>>>
>>> ===========================================================================
>>> option 2:
>>>
>>> 	combpwm0: pwm@10280000 {
>>> 		compatible = "rockchip,rv1108-pwm";
>>> 		reg = <0x10280000 0x40>;
>>> 		interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
>>> 		#address-cells = <1>;
>>> 		#size-cells = <0>;
>>>
>>> 		pwm4: pwm-4@0 {
>>> 			reg = <0x0>;
>>> 		};
>>>
>>> 		pwm5: pwm-5@10 {
>>> 			reg = <0x10>;
>>> 		};
>>>
>>> 		pwm6: pwm-6@20 {
>>> 			reg = <0x20>;
>>> 		};
>>>
>>> 		pwm7: pwm-7@30 {
>>> 			reg = <0x30>;
>>> 		};
>>> 	};
>>>
>>> CON:
>>> - Driver change needed.
>>> - Not compatible with current drivers.
>>>
>>> ===========================================================================
>>>
>>> Current situation:
>>>
>>> 	pwm0: pwm@20040000 {
>>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>>> 		reg = <0x20040000 0x10>;
>>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>>> 	};
>>>
>>> 	pwm1: pwm@20040010 {
>>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>>> 		reg = <0x20040010 0x10>;
>>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>>> 	};
>>>
>>> 	pwm2: pwm@20040020 {
>>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>>> 		reg = <0x20040020 0x10>;
>>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>>> 	};
>>>
>>> 	pwm3: pwm@20040030 {
>>> 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
>>> 		reg = <0x20040030 0x10>;
>>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>>> 	};
>>>
>>> CON:
>>> - The property "interrupts 39" can only be claimed ones by one probe function at the time.
>>> - Has a fall-back string for rk3288, but unknown identical behavior for interrupts ???
>>
> 
>> To be honest, all three descriptions look wrong to me. From the above it
>> looks like this is simply one PWM controller with four channels, so it
>> should really be described as such, i.e.:
>>
>> 	pwm@20040030 {
>> 		compatible = "rockchip,rv1108-pwm";
>> 		reg = <0x20040030 0x40>;
>> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
>> 	};
>>
> 
> Each PWM channel has it's own pinctrl.
> Not all channel pins are always in use for PWM exclusively.
> Your proposal would not allow pins to be used for other functions.

Why would you think that? It would just mean moving the pinctrl 
selection down to the board level like for GPIOs - we manage just fine 
with a single DT node per GPIO bank, and semantically PWMs have no 
reason do be different. In fact on newer SoCs some PWM channels can be 
muxed to multiple pins, so pinctrl really has to be at the board level 
already in those casesa.

The TRMs seem pretty clear that the "new" PWM block from RK3288 onwards 
is a single module with 4 channels, not 4 independent controllers, so it 
seems to have been an unfortunate mistake not to create a new binding 
for it at that point. It would be a little fiddly, but far from 
impossible, to make the driver support both the existing binding and a 
new one (and I don't see how we could use the interrupt on newer SoCs 
*without* a binding change, given that the interrupt status register is 
outside any channel's current "reg"), but an old kernel with a new DT 
would be more problematic. If we kept the existing compatibles then an 
old driver would always use channel 0 regardless of what the consumer 
requested; using new compatibles as well means the old kernel loses PWM 
functionality entirely, which is arguably "safe", but I'm not sure if 
it's really better or worse :/

Robin.

> More ideas with this interrupt? Please advise.
> 
> ===
> 
> The SoCs PWM are configurable to operate in continuous mode (default mainline) or one-shot mode or capture mode.
> Is there any good example for one-shot mode interrupt use?
> 
> 
>> Looking through existing Rockchip SoC DTSI files, though, it looks like
>> this has been done the wrong way since the beginning, so not sure if you
>> still want to fix it up.
>>
>> This whole problem of dealing with a shared interrupt wouldn't be a
>> problem if this was described properly.
>>
>> Thierry
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Biju Das Sept. 30, 2022, 6:13 a.m. UTC | #12
Hi Thierry Reding,

> Subject: Re: [PATCH v1 03/11] dt-bindings: pwm: rockchip: add
> rockchip,rk3128-pwm
> 
> On Tue, Sep 13, 2022 at 04:38:32PM +0200, Johan Jonker wrote:
> >
> >
> > On 9/12/22 18:21, Rob Herring wrote:
> > > On Sat, Sep 10, 2022 at 09:48:04PM +0200, Johan Jonker wrote:
> > >> Reduced CC.
> > >>
> > >> Hi Rob,
> > >>
> > >
> > > Seemed like a simple enough warning to fix...
> >
> > Some examples for comment.
> > Let us know what would be the better solution?
> >
> >
> ======================================================================
> > =====
> >
> > option1:
> >
> > 	combpwm0: combpwm0 {
> > 		compatible = "rockchip,rv1108-combpwm";
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 		#address-cells = <2>;
> > 		#size-cells = <2>;
> >
> > 		pwm0: pwm@20040000 {
> > 			compatible = "rockchip,rv1108-pwm";
> > 			reg = <0x20040000 0x10>;
> > 		};
> >
> > 		pwm1: pwm@20040010 {
> > 			compatible = "rockchip,rv1108-pwm";
> > 			reg = <0x20040010 0x10>;
> > 		};
> >
> > 		pwm2: pwm@20040020 {
> > 			compatible = "rockchip,rv1108-pwm";
> > 			reg = <0x20040020 0x10>;
> > 		};
> >
> > 		pwm3: pwm@20040030 {
> > 			compatible = "rockchip,rv1108-pwm";
> > 			reg = <0x20040030 0x10>;
> > 		};
> > 	};
> >
> > PRO:
> > - Existing driver might still work.
> > CON:
> > - New compatible needed to service the combined interrupts.
> > - Driver change needed.
> >
> >
> ======================================================================
> > =====
> > option 2:
> >
> > 	combpwm0: pwm@10280000 {
> > 		compatible = "rockchip,rv1108-pwm";
> > 		reg = <0x10280000 0x40>;
> > 		interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>;
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> >
> > 		pwm4: pwm-4@0 {
> > 			reg = <0x0>;
> > 		};
> >
> > 		pwm5: pwm-5@10 {
> > 			reg = <0x10>;
> > 		};
> >
> > 		pwm6: pwm-6@20 {
> > 			reg = <0x20>;
> > 		};
> >
> > 		pwm7: pwm-7@30 {
> > 			reg = <0x30>;
> > 		};
> > 	};
> >
> > CON:
> > - Driver change needed.
> > - Not compatible with current drivers.
> >
> >
> ======================================================================
> > =====
> >
> > Current situation:
> >
> > 	pwm0: pwm@20040000 {
> > 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> > 		reg = <0x20040000 0x10>;
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> >
> > 	pwm1: pwm@20040010 {
> > 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> > 		reg = <0x20040010 0x10>;
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> >
> > 	pwm2: pwm@20040020 {
> > 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> > 		reg = <0x20040020 0x10>;
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> >
> > 	pwm3: pwm@20040030 {
> > 		compatible = "rockchip,rv1108-pwm", "rockchip,rk3288-pwm";
> > 		reg = <0x20040030 0x10>;
> > 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> > 	};
> >
> > CON:
> > - The property "interrupts 39" can only be claimed ones by one probe
> function at the time.
> > - Has a fall-back string for rk3288, but unknown identical behavior
> for interrupts ???
> 
> To be honest, all three descriptions look wrong to me. From the above
> it looks like this is simply one PWM controller with four channels, so
> it should really be described as such, i.e.:
> 
> 	pwm@20040030 {
> 		compatible = "rockchip,rv1108-pwm";
> 		reg = <0x20040030 0x40>;
> 		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> 	};

Sorry to jump in.

Renesas GPT has also similar case where we have large PWM IP block
having 8 pwm channels. Each channel has it's Own pinctrl, unique registers, interrupts
for each channel. But there are 4 sharable external trigger input pins for all the channels.

If it is a single block like this, how will you associate pinctrl 
with each channel?

At board level if you specify <pin4 enabled>, without pwm channel
specific information how will you configure channel4?

Maybe something like this will help. Is it acceptable?

	pwm@20040030 {
		compatible = "rockchip,rv1108-pwm";
		reg = <0x20040030 0x40>;
		interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;

		pwm4: pwm-4@0 {
			reg = <0x0>;
		};

		pwm5: pwm-5@10 {
			reg = <0x10>;
		};

		pwm6: pwm-6@20 {
			reg = <0x20>;
		};

		pwm7: pwm-7@30 {
			reg = <0x30>;
		};
	};


Cheers,
Biju
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml b/Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml
index a336ff936..f86170871 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml
+++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.yaml
@@ -21,6 +21,7 @@  properties:
           - const: rockchip,rk2928-pwm
       - items:
           - enum:
+              - rockchip,rk3128-pwm
               - rockchip,rk3368-pwm
               - rockchip,rk3399-pwm
               - rockchip,rv1108-pwm