| Message ID | 20221123021346.18136-5-yu.tu@amlogic.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Add S4 SoC PLL and Peripheral clock controller | expand |
On 23/11/2022 03:13, Yu Tu wrote: > Added information about the S4 SOC Peripheral Clock controller in DT. > > Signed-off-by: Yu Tu <yu.tu@amlogic.com> > --- > arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi > index bd9c2ef83314..e7fab6e400be 100644 > --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi > +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi > @@ -6,6 +6,8 @@ > #include <dt-bindings/interrupt-controller/irq.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> > +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> > > / { > cpus { > @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { > #clock-cells = <1>; > }; > > + clkc_periphs: clock-controller { > + compatible = "amlogic,s4-peripherals-clkc"; > + reg = <0x0 0x0 0x0 0x49c>; This is broken... did you check for warnings? Best regards, Krzysztof
Hi Krzysztof, On 2022/11/23 18:10, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 03:13, Yu Tu wrote: >> Added information about the S4 SOC Peripheral Clock controller in DT. >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> --- >> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> index bd9c2ef83314..e7fab6e400be 100644 >> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> @@ -6,6 +6,8 @@ >> #include <dt-bindings/interrupt-controller/irq.h> >> #include <dt-bindings/interrupt-controller/arm-gic.h> >> #include <dt-bindings/gpio/gpio.h> >> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >> >> / { >> cpus { >> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >> #clock-cells = <1>; >> }; >> >> + clkc_periphs: clock-controller { >> + compatible = "amlogic,s4-peripherals-clkc"; >> + reg = <0x0 0x0 0x0 0x49c>; > > This is broken... did you check for warnings? Yes, i do. You can have a look at the results of my test, as follows. total: 0 errors, 0 warnings, 0 checks, 38 lines checked ../patch_clk_v5_1122/0004-arm64-dts-meson-add-S4-Soc-Peripheral-clock-controll.patch has no obvious style problems and is ready for submission. > > > Best regards, > Krzysztof > > .
On 23/11/2022 12:27, Yu Tu wrote: > Hi Krzysztof, > > On 2022/11/23 18:10, Krzysztof Kozlowski wrote: >> [ EXTERNAL EMAIL ] >> >> On 23/11/2022 03:13, Yu Tu wrote: >>> Added information about the S4 SOC Peripheral Clock controller in DT. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> --- >>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> index bd9c2ef83314..e7fab6e400be 100644 >>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> @@ -6,6 +6,8 @@ >>> #include <dt-bindings/interrupt-controller/irq.h> >>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>> #include <dt-bindings/gpio/gpio.h> >>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>> >>> / { >>> cpus { >>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>> #clock-cells = <1>; >>> }; >>> >>> + clkc_periphs: clock-controller { >>> + compatible = "amlogic,s4-peripherals-clkc"; >>> + reg = <0x0 0x0 0x0 0x49c>; >> >> This is broken... did you check for warnings? > Yes, i do. > You can have a look at the results of my test, as follows. > > total: 0 errors, 0 warnings, 0 checks, 38 lines checked > > ../patch_clk_v5_1122/0004-arm64-dts-meson-add-S4-Soc-Peripheral-clock-controll.patch > has no obvious style problems and is ready for submission. > This is a checkpatch output. I am talking about DTS broken. dtc should warn you. Best regards, Krzysztof
On 2022/11/23 21:02, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 12:27, Yu Tu wrote: >> Hi Krzysztof, >> >> On 2022/11/23 18:10, Krzysztof Kozlowski wrote: >>> [ EXTERNAL EMAIL ] >>> >>> On 23/11/2022 03:13, Yu Tu wrote: >>>> Added information about the S4 SOC Peripheral Clock controller in DT. >>>> >>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>> --- >>>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >>>> 1 file changed, 26 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> index bd9c2ef83314..e7fab6e400be 100644 >>>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> @@ -6,6 +6,8 @@ >>>> #include <dt-bindings/interrupt-controller/irq.h> >>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>> #include <dt-bindings/gpio/gpio.h> >>>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>>> >>>> / { >>>> cpus { >>>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>>> #clock-cells = <1>; >>>> }; >>>> >>>> + clkc_periphs: clock-controller { >>>> + compatible = "amlogic,s4-peripherals-clkc"; >>>> + reg = <0x0 0x0 0x0 0x49c>; >>> >>> This is broken... did you check for warnings? >> Yes, i do. >> You can have a look at the results of my test, as follows. >> >> total: 0 errors, 0 warnings, 0 checks, 38 lines checked >> >> ../patch_clk_v5_1122/0004-arm64-dts-meson-add-S4-Soc-Peripheral-clock-controll.patch >> has no obvious style problems and is ready for submission. >> > > This is a checkpatch output. I am talking about DTS broken. dtc should > warn you. Do you mean I will have wraning in compiling? I actually compiled without warning. ccf$ make ARCH=arm64 dtbs -j12 DTC arch/arm64/boot/dts/amlogic/meson-s4-s805x2-aq222.dtb > > Best regards, > Krzysztof > > .
On 23/11/2022 11:10, Krzysztof Kozlowski wrote: > On 23/11/2022 03:13, Yu Tu wrote: >> Added information about the S4 SOC Peripheral Clock controller in DT. >> >> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >> --- >> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> index bd9c2ef83314..e7fab6e400be 100644 >> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >> @@ -6,6 +6,8 @@ >> #include <dt-bindings/interrupt-controller/irq.h> >> #include <dt-bindings/interrupt-controller/arm-gic.h> >> #include <dt-bindings/gpio/gpio.h> >> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >> >> / { >> cpus { >> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >> #clock-cells = <1>; >> }; >> >> + clkc_periphs: clock-controller { >> + compatible = "amlogic,s4-peripherals-clkc"; >> + reg = <0x0 0x0 0x0 0x49c>; > > This is broken... did you check for warnings? This is actually fine, the parent node has a ranges property: https://github.com/torvalds/linux/blob/eb7081409f94a9a8608593d0fb63a1aa3d6f95d8/arch/arm64/boot/dts/amlogic/meson-s4.dtsi#L93 Neil > > > Best regards, > Krzysztof > > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic
Hi Neil, Thanks for your reply and explanation. On 2022/11/23 21:27, Neil Armstrong wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 11:10, Krzysztof Kozlowski wrote: >> On 23/11/2022 03:13, Yu Tu wrote: >>> Added information about the S4 SOC Peripheral Clock controller in DT. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> --- >>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> index bd9c2ef83314..e7fab6e400be 100644 >>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> @@ -6,6 +6,8 @@ >>> #include <dt-bindings/interrupt-controller/irq.h> >>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>> #include <dt-bindings/gpio/gpio.h> >>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>> / { >>> cpus { >>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>> #clock-cells = <1>; >>> }; >>> + clkc_periphs: clock-controller { >>> + compatible = "amlogic,s4-peripherals-clkc"; >>> + reg = <0x0 0x0 0x0 0x49c>; >> >> This is broken... did you check for warnings? > > This is actually fine, the parent node has a ranges property: > https://github.com/torvalds/linux/blob/eb7081409f94a9a8608593d0fb63a1aa3d6f95d8/arch/arm64/boot/dts/amlogic/meson-s4.dtsi#L93 > > > Neil > >> >> >> Best regards, >> Krzysztof >> >> >> _______________________________________________ >> linux-amlogic mailing list >> linux-amlogic@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-amlogic > > .
On 23/11/2022 14:23, Yu Tu wrote: >>> ../patch_clk_v5_1122/0004-arm64-dts-meson-add-S4-Soc-Peripheral-clock-controll.patch >>> has no obvious style problems and is ready for submission. >>> >> >> This is a checkpatch output. I am talking about DTS broken. dtc should >> warn you. > > Do you mean I will have wraning in compiling? > I actually compiled without warning. > ccf$ make ARCH=arm64 dtbs -j12 > DTC arch/arm64/boot/dts/amlogic/meson-s4-s805x2-aq222.dtb > Nope: ../arch/arm64/boot/dts/amlogic/meson-s4.dtsi:105.35-127.6: Warning (unit_address_vs_reg): /soc/apb4@fe000000/clock-controller: node has a reg or ranges property, but no unit name Test your patches better before using reviewers time for trivial compile-time fixes. The same rules for C code apply for DTS. Best regards, Krzysztof
On 23/11/2022 14:27, Neil Armstrong wrote: > On 23/11/2022 11:10, Krzysztof Kozlowski wrote: >> On 23/11/2022 03:13, Yu Tu wrote: >>> Added information about the S4 SOC Peripheral Clock controller in DT. >>> >>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>> --- >>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> index bd9c2ef83314..e7fab6e400be 100644 >>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>> @@ -6,6 +6,8 @@ >>> #include <dt-bindings/interrupt-controller/irq.h> >>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>> #include <dt-bindings/gpio/gpio.h> >>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>> >>> / { >>> cpus { >>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>> #clock-cells = <1>; >>> }; >>> >>> + clkc_periphs: clock-controller { >>> + compatible = "amlogic,s4-peripherals-clkc"; >>> + reg = <0x0 0x0 0x0 0x49c>; >> >> This is broken... did you check for warnings? > > This is actually fine, the parent node has a ranges property: > https://github.com/torvalds/linux/blob/eb7081409f94a9a8608593d0fb63a1aa3d6f95d8/arch/arm64/boot/dts/amlogic/meson-s4.dtsi#L93 The parent ranges do not change here anything. You cannot have a reg without unit address and the tools report it. No need to use reviewers for this... Best regards, Krzysztof
On 23/11/2022 15:13, Krzysztof Kozlowski wrote: > On 23/11/2022 14:27, Neil Armstrong wrote: >> On 23/11/2022 11:10, Krzysztof Kozlowski wrote: >>> On 23/11/2022 03:13, Yu Tu wrote: >>>> Added information about the S4 SOC Peripheral Clock controller in DT. >>>> >>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>> --- >>>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ >>>> 1 file changed, 26 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> index bd9c2ef83314..e7fab6e400be 100644 >>>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>> @@ -6,6 +6,8 @@ >>>> #include <dt-bindings/interrupt-controller/irq.h> >>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>> #include <dt-bindings/gpio/gpio.h> >>>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>>> >>>> / { >>>> cpus { >>>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>>> #clock-cells = <1>; >>>> }; >>>> >>>> + clkc_periphs: clock-controller { >>>> + compatible = "amlogic,s4-peripherals-clkc"; >>>> + reg = <0x0 0x0 0x0 0x49c>; >>> >>> This is broken... did you check for warnings? >> >> This is actually fine, the parent node has a ranges property: >> https://github.com/torvalds/linux/blob/eb7081409f94a9a8608593d0fb63a1aa3d6f95d8/arch/arm64/boot/dts/amlogic/meson-s4.dtsi#L93 > > The parent ranges do not change here anything. You cannot have a reg > without unit address and the tools report it. No need to use reviewers > for this... Oh I see now, I simply looked at the reg property, not the unit address. Sorry indeed it's wrong and DTC should complain. Yu, you should probably update your DTC version. Neil > > Best regards, > Krzysztof >
On 2022/11/23 22:12, Krzysztof Kozlowski wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 14:23, Yu Tu wrote: >>>> ../patch_clk_v5_1122/0004-arm64-dts-meson-add-S4-Soc-Peripheral-clock-controll.patch >>>> has no obvious style problems and is ready for submission. >>>> >>> >>> This is a checkpatch output. I am talking about DTS broken. dtc should >>> warn you. >> >> Do you mean I will have wraning in compiling? >> I actually compiled without warning. >> ccf$ make ARCH=arm64 dtbs -j12 >> DTC arch/arm64/boot/dts/amlogic/meson-s4-s805x2-aq222.dtb >> > > Nope: > > ../arch/arm64/boot/dts/amlogic/meson-s4.dtsi:105.35-127.6: Warning > (unit_address_vs_reg): /soc/apb4@fe000000/clock-controller: node has a > reg or ranges property, but no unit name > > Test your patches better before using reviewers time for trivial > compile-time fixes. The same rules for C code apply for DTS. > Like this: clkc_periphs: clock-controller@0 { compatible = "amlogic,s4-peripherals-clkc"; reg = <0x0 0x0 0x0 0x49c>; You mean it should be, right? > Best regards, > Krzysztof > > .
On 2022/11/23 22:21, neil.armstrong@linaro.org wrote: > [ EXTERNAL EMAIL ] > > On 23/11/2022 15:13, Krzysztof Kozlowski wrote: >> On 23/11/2022 14:27, Neil Armstrong wrote: >>> On 23/11/2022 11:10, Krzysztof Kozlowski wrote: >>>> On 23/11/2022 03:13, Yu Tu wrote: >>>>> Added information about the S4 SOC Peripheral Clock controller in DT. >>>>> >>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com> >>>>> --- >>>>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 >>>>> +++++++++++++++++++++++ >>>>> 1 file changed, 26 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>>> b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>>> index bd9c2ef83314..e7fab6e400be 100644 >>>>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi >>>>> @@ -6,6 +6,8 @@ >>>>> #include <dt-bindings/interrupt-controller/irq.h> >>>>> #include <dt-bindings/interrupt-controller/arm-gic.h> >>>>> #include <dt-bindings/gpio/gpio.h> >>>>> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> >>>>> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> >>>>> / { >>>>> cpus { >>>>> @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { >>>>> #clock-cells = <1>; >>>>> }; >>>>> + clkc_periphs: clock-controller { >>>>> + compatible = "amlogic,s4-peripherals-clkc"; >>>>> + reg = <0x0 0x0 0x0 0x49c>; >>>> >>>> This is broken... did you check for warnings? >>> >>> This is actually fine, the parent node has a ranges property: >>> https://github.com/torvalds/linux/blob/eb7081409f94a9a8608593d0fb63a1aa3d6f95d8/arch/arm64/boot/dts/amlogic/meson-s4.dtsi#L93 >>> >> >> The parent ranges do not change here anything. You cannot have a reg >> without unit address and the tools report it. No need to use reviewers >> for this... > > Oh I see now, I simply looked at the reg property, not the unit address. > > Sorry indeed it's wrong and DTC should complain. > > Yu, you should probably update your DTC version. Okay. > > Neil > >> >> Best regards, >> Krzysztof >> > > .
diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi index bd9c2ef83314..e7fab6e400be 100644 --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi @@ -6,6 +6,8 @@ #include <dt-bindings/interrupt-controller/irq.h> #include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/clock/amlogic,s4-pll-clkc.h> +#include <dt-bindings/clock/amlogic,s4-peripherals-clkc.h> / { cpus { @@ -100,6 +102,30 @@ clkc_pll: clock-controller@8000 { #clock-cells = <1>; }; + clkc_periphs: clock-controller { + compatible = "amlogic,s4-peripherals-clkc"; + reg = <0x0 0x0 0x0 0x49c>; + clocks = <&clkc_pll CLKID_FCLK_DIV2>, + <&clkc_pll CLKID_FCLK_DIV2P5>, + <&clkc_pll CLKID_FCLK_DIV3>, + <&clkc_pll CLKID_FCLK_DIV4>, + <&clkc_pll CLKID_FCLK_DIV5>, + <&clkc_pll CLKID_FCLK_DIV7>, + <&clkc_pll CLKID_HIFI_PLL>, + <&clkc_pll CLKID_GP0_PLL>, + <&clkc_pll CLKID_MPLL0>, + <&clkc_pll CLKID_MPLL1>, + <&clkc_pll CLKID_MPLL2>, + <&clkc_pll CLKID_MPLL3>, + <&clkc_pll CLKID_HDMI_PLL>, + <&xtal>; + clock-names = "fclk_div2", "fclk_div2p5", "fclk_div3", + "fclk_div4", "fclk_div5", "fclk_div7", + "hifi_pll", "gp0_pll", "mpll0", "mpll1", + "mpll2", "mpll3", "hdmi_pll", "xtal"; + #clock-cells = <1>; + }; + periphs_pinctrl: pinctrl@4000 { compatible = "amlogic,meson-s4-periphs-pinctrl"; #address-cells = <2>;
Added information about the S4 SOC Peripheral Clock controller in DT. Signed-off-by: Yu Tu <yu.tu@amlogic.com> --- arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 26 +++++++++++++++++++++++ 1 file changed, 26 insertions(+)