| Message ID | 20230925065915.3467964-1-quic_devipriy@quicinc.com |
|---|---|
| Headers | show |
| Series | Add PWM support for IPQ chipsets | expand |
On 9/25/2023 12:41 PM, Krzysztof Kozlowski wrote: > On 25/09/2023 08:59, Devi Priya wrote: >> DT binding for the PWM block in Qualcomm IPQ6018 SoC. >> >> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> >> Co-developed-by: Baruch Siach <baruch.siach@siklu.com> >> Signed-off-by: Baruch Siach <baruch.siach@siklu.com> >> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com> > > ... > >> diff --git a/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml >> new file mode 100644 >> index 000000000000..857086ad539e >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml > > Filename matching compatible, so qcom,ipq6018-pwm.yaml okay > >> @@ -0,0 +1,53 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pwm/ipq-pwm.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm IPQ6018 PWM controller >> + >> +maintainers: >> + - Baruch Siach <baruch@tkos.co.il> >> + >> +properties: >> + "#pwm-cells": >> + const: 2 >> + >> + compatible: >> + const: qcom,ipq6018-pwm > > compatible is always the first property. okay > >> + >> + reg: >> + description: Offset of PWM register in the TCSR block. >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - reg >> + - clocks >> + - "#pwm-cells" > > And this order must be the same as in properties. okay > >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/qcom,gcc-ipq6018.h> >> + >> + syscon@1937000 { >> + compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd"; >> + reg = <0x01937000 0x21000>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges = <0 0x1937000 0x21000>; > > Drop this node, not related. The parent binding could have full example, > on the other hand. Additionally, I have doubts that you really tested > the parent binding. Sure, will drop the syscon node Thanks, Devi Priya > >> + >> + pwm: pwm@a010 { >> + compatible = "qcom,ipq6018-pwm"; > > Best regards, > Krzysztof >
On 9/25/2023 12:29 PM, Devi Priya wrote: > Describe the PWM block on IPQ6018. > > The PWM is in the TCSR area. Make &tcsr "simple-mfd" compatible, and add > &pwm as child of &tcsr. > > Add also ipq6018 specific compatible string. > > Co-developed-by: Baruch Siach <baruch.siach@siklu.com> > Signed-off-by: Baruch Siach <baruch.siach@siklu.com> > Signed-off-by: Devi Priya <quic_devipriy@quicinc.com> > --- > v12: > > No change > > v11: > > No change > > v10: > > No change > > v9: > > Add 'ranges' property (Rob) > > v8: > > Add size cell to 'reg' (Rob) > > v7: > > Use 'reg' instead of 'offset' (Rob) > > Add qcom,tcsr-ipq6018 (Rob) > > Drop clock-names (Bjorn) > > v6: > > Make the PWM node child of TCSR (Rob Herring) > > Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König) > > v5: Use qcom,pwm-regs for TCSR phandle instead of direct regs > > v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring) > > arch/arm64/boot/dts/qcom/ipq6018.dtsi | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi > index 47b8b1d6730a..cadd2c583526 100644 > --- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi > @@ -398,8 +398,21 @@ tcsr_mutex: hwlock@1905000 { > }; > > tcsr: syscon@1937000 { > - compatible = "qcom,tcsr-ipq6018", "syscon"; > + compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd"; > reg = <0x0 0x01937000 0x0 0x21000>; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges = <0x0 0x0 0x01937000 0x21000>; > + Hi Krzysztof, Referring to https://lore.kernel.org/all/20220909091056.128949-1-krzysztof.kozlowski@linaro.org/, it seems that the TCSR block should not have any child nodes. Could you pls provide your suggestions on pwm being added as the child node? Thanks, Devi Priya > + pwm: pwm@a010 { > + compatible = "qcom,ipq6018-pwm"; > + reg = <0xa010 0x20>; > + clocks = <&gcc GCC_ADSS_PWM_CLK>; > + assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>; > + assigned-clock-rates = <100000000>; > + #pwm-cells = <2>; > + status = "disabled"; > + }; > }; > > usb2: usb@70f8800 {
On 29/09/2023 10:56, Devi Priya wrote: >>>> diff --git a/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml >>>> b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml >>>> new file mode 100644 >>>> index 000000000000..857086ad539e >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml >>> >>> Filename matching compatible, so qcom,ipq6018-pwm.yaml >> okay > We would have other ipq compatibles (ipq9574 & ipq5332) being added to > the binding in the upcoming series. > So, shall we rename the binding to qcom,ipq-pwm.yaml I prefer not. Best regards, Krzysztof
On 29/09/2023 13:47, Devi Priya wrote: > > > On 9/25/2023 12:29 PM, Devi Priya wrote: >> Describe the PWM block on IPQ6018. >> >> The PWM is in the TCSR area. Make &tcsr "simple-mfd" compatible, and add >> &pwm as child of &tcsr. >> >> Add also ipq6018 specific compatible string. >> >> Co-developed-by: Baruch Siach <baruch.siach@siklu.com> >> Signed-off-by: Baruch Siach <baruch.siach@siklu.com> >> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com> >> --- >> v12: >> >> No change >> >> v11: >> >> No change >> >> v10: >> >> No change >> >> v9: >> >> Add 'ranges' property (Rob) >> >> v8: >> >> Add size cell to 'reg' (Rob) >> >> v7: >> >> Use 'reg' instead of 'offset' (Rob) >> >> Add qcom,tcsr-ipq6018 (Rob) >> >> Drop clock-names (Bjorn) >> >> v6: >> >> Make the PWM node child of TCSR (Rob Herring) >> >> Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König) >> >> v5: Use qcom,pwm-regs for TCSR phandle instead of direct regs >> >> v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring) >> >> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi >> index 47b8b1d6730a..cadd2c583526 100644 >> --- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi >> +++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi >> @@ -398,8 +398,21 @@ tcsr_mutex: hwlock@1905000 { >> }; >> >> tcsr: syscon@1937000 { >> - compatible = "qcom,tcsr-ipq6018", "syscon"; >> + compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd"; >> reg = <0x0 0x01937000 0x0 0x21000>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges = <0x0 0x0 0x01937000 0x21000>; >> + > Hi Krzysztof, > Referring to > https://lore.kernel.org/all/20220909091056.128949-1-krzysztof.kozlowski@linaro.org/, > it seems that the TCSR block should > not have any child nodes. Could you pls provide your suggestions on pwm > being added as the child node? If you are sure that TCSR contains PWM and all registers are there, then feel free to add proper binding. Sending untested patch is not the way to go. Best regards, Krzysztof
On 9/30/2023 9:05 PM, Krzysztof Kozlowski wrote: > On 29/09/2023 13:47, Devi Priya wrote: >> >> >> On 9/25/2023 12:29 PM, Devi Priya wrote: >>> Describe the PWM block on IPQ6018. >>> >>> The PWM is in the TCSR area. Make &tcsr "simple-mfd" compatible, and add >>> &pwm as child of &tcsr. >>> >>> Add also ipq6018 specific compatible string. >>> >>> Co-developed-by: Baruch Siach <baruch.siach@siklu.com> >>> Signed-off-by: Baruch Siach <baruch.siach@siklu.com> >>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com> >>> --- >>> v12: >>> >>> No change >>> >>> v11: >>> >>> No change >>> >>> v10: >>> >>> No change >>> >>> v9: >>> >>> Add 'ranges' property (Rob) >>> >>> v8: >>> >>> Add size cell to 'reg' (Rob) >>> >>> v7: >>> >>> Use 'reg' instead of 'offset' (Rob) >>> >>> Add qcom,tcsr-ipq6018 (Rob) >>> >>> Drop clock-names (Bjorn) >>> >>> v6: >>> >>> Make the PWM node child of TCSR (Rob Herring) >>> >>> Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König) >>> >>> v5: Use qcom,pwm-regs for TCSR phandle instead of direct regs >>> >>> v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring) >>> >>> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 15 ++++++++++++++- >>> 1 file changed, 14 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi >>> index 47b8b1d6730a..cadd2c583526 100644 >>> --- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi >>> @@ -398,8 +398,21 @@ tcsr_mutex: hwlock@1905000 { >>> }; >>> >>> tcsr: syscon@1937000 { >>> - compatible = "qcom,tcsr-ipq6018", "syscon"; >>> + compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd"; >>> reg = <0x0 0x01937000 0x0 0x21000>; >>> + #address-cells = <1>; >>> + #size-cells = <1>; >>> + ranges = <0x0 0x0 0x01937000 0x21000>; >>> + >> Hi Krzysztof, >> Referring to >> https://lore.kernel.org/all/20220909091056.128949-1-krzysztof.kozlowski@linaro.org/, >> it seems that the TCSR block should >> not have any child nodes. Could you pls provide your suggestions on pwm >> being added as the child node? > > If you are sure that TCSR contains PWM and all registers are there, then > feel free to add proper binding. Sending untested patch is not the way > to go. Sure, okay Thanks, Devi Priya > > Best regards, > Krzysztof >