| Message ID | 20191211070835.83489-1-jian.hu@amlogic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: meson: add A1 periphs and PLL clock nodes | expand |
On Wed 11 Dec 2019 at 08:08, Jian Hu <jian.hu@amlogic.com> wrote: > Add A1 periphs and PLL clock controller nodes, Some clocks > in periphs controller are the parents of PLL clocks, Meanwhile > some clocks in PLL controller are those of periphs clocks. > They rely on each other. > Compared with the previous series, > the register region is only for the clock. So syscon is not > used in A1. Again, while this is valuable information for the maintainer to keep up, it is not something that should appear in the commit description. The evolution of your commit should be described after the '---' Also, this obviously depends on another series. It should be mentioned accordingly > > Signed-off-by: Jian Hu <jian.hu@amlogic.com> > --- > arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 26 +++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi > index 7210ad049d1d..de43a010fa6e 100644 > --- a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi > +++ b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi > @@ -5,6 +5,8 @@ > > #include <dt-bindings/interrupt-controller/irq.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/clock/a1-pll-clkc.h> > +#include <dt-bindings/clock/a1-clkc.h> When possible, please order the includes alpha-numerically > > / { > compatible = "amlogic,a1"; > @@ -74,6 +76,30 @@ > #size-cells = <2>; > ranges = <0x0 0x0 0x0 0xfe000000 0x0 0x1000000>; > > + clkc_periphs: periphs-clock-controller@800 { ^ From DT spec: "The name of a node should be somewhat generic, reflecting the function of the device and not its precise programming model." Here, an appropriate node name would be "clock-controller", not "periphs-clock-controller" > + compatible = "amlogic,a1-periphs-clkc"; > + #clock-cells = <1>; > + reg = <0 0x800 0 0x104>; > + clocks = <&clkc_pll CLKID_FCLK_DIV2>, > + <&clkc_pll CLKID_FCLK_DIV3>, > + <&clkc_pll CLKID_FCLK_DIV5>, > + <&clkc_pll CLKID_FCLK_DIV7>, > + <&clkc_pll CLKID_HIFI_PLL>, > + <&xtal>; > + clock-names = "fclk_div2", "fclk_div3", > + "fclk_div5", "fclk_div7", > + "hifi_pll", "xtal"; > + }; > + > + clkc_pll: pll-clock-controller@7c80 { Please order nodes by address when they have one. This clock controller should appear after the uarts > + compatible = "amlogic,a1-pll-clkc"; > + #clock-cells = <1>; > + reg = <0 0x7c80 0 0x21c>; > + clocks = <&clkc_periphs CLKID_XTAL_FIXPLL>, > + <&clkc_periphs CLKID_XTAL_HIFIPLL>; > + clock-names = "xtal_fixpll", "xtal_hifipll"; > + }; > + > uart_AO: serial@1c00 { > compatible = "amlogic,meson-gx-uart", > "amlogic,meson-ao-uart";
On 2019/12/11 17:43, Jerome Brunet wrote: > > On Wed 11 Dec 2019 at 08:08, Jian Hu <jian.hu@amlogic.com> wrote: > >> Add A1 periphs and PLL clock controller nodes, Some clocks >> in periphs controller are the parents of PLL clocks, Meanwhile >> some clocks in PLL controller are those of periphs clocks. >> They rely on each other. > >> Compared with the previous series, >> the register region is only for the clock. So syscon is not >> used in A1. > > Again, while this is valuable information for the maintainer to keep up, > it is not something that should appear in the commit description. > > The evolution of your commit should be described after the '---' > OK, I will put the compared message after the '---' > Also, this obviously depends on another series. It should be mentioned > accordingly OK, I will add the dependent clock patchset. > >> >> Signed-off-by: Jian Hu <jian.hu@amlogic.com> >> --- >> arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 26 +++++++++++++++++++++++ >> 1 file changed, 26 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi >> index 7210ad049d1d..de43a010fa6e 100644 >> --- a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi >> +++ b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi >> @@ -5,6 +5,8 @@ >> >> #include <dt-bindings/interrupt-controller/irq.h> >> #include <dt-bindings/interrupt-controller/arm-gic.h> >> +#include <dt-bindings/clock/a1-pll-clkc.h> >> +#include <dt-bindings/clock/a1-clkc.h> > > When possible, please order the includes alpha-numerically > OK, I will reorder it. >> >> / { >> compatible = "amlogic,a1"; >> @@ -74,6 +76,30 @@ >> #size-cells = <2>; >> ranges = <0x0 0x0 0x0 0xfe000000 0x0 0x1000000>; >> >> + clkc_periphs: periphs-clock-controller@800 { > ^ >>From DT spec: "The name of a node should be somewhat generic, reflecting > the function of the device and not its precise programming model." > > Here, an appropriate node name would be "clock-controller", not > "periphs-clock-controller" OK, I will change the node name. > >> + compatible = "amlogic,a1-periphs-clkc"; >> + #clock-cells = <1>; >> + reg = <0 0x800 0 0x104>; >> + clocks = <&clkc_pll CLKID_FCLK_DIV2>, >> + <&clkc_pll CLKID_FCLK_DIV3>, >> + <&clkc_pll CLKID_FCLK_DIV5>, >> + <&clkc_pll CLKID_FCLK_DIV7>, >> + <&clkc_pll CLKID_HIFI_PLL>, >> + <&xtal>; >> + clock-names = "fclk_div2", "fclk_div3", >> + "fclk_div5", "fclk_div7", >> + "hifi_pll", "xtal"; >> + }; >> + >> + clkc_pll: pll-clock-controller@7c80 { > > Please order nodes by address when they have one. > This clock controller should appear after the uarts OK, I will reorder it. > >> + compatible = "amlogic,a1-pll-clkc"; >> + #clock-cells = <1>; >> + reg = <0 0x7c80 0 0x21c>; >> + clocks = <&clkc_periphs CLKID_XTAL_FIXPLL>, >> + <&clkc_periphs CLKID_XTAL_HIFIPLL>; >> + clock-names = "xtal_fixpll", "xtal_hifipll"; >> + }; >> + >> uart_AO: serial@1c00 { >> compatible = "amlogic,meson-gx-uart", >> "amlogic,meson-ao-uart"; > > . >
diff --git a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi index 7210ad049d1d..de43a010fa6e 100644 --- a/arch/arm64/boot/dts/amlogic/meson-a1.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-a1.dtsi @@ -5,6 +5,8 @@ #include <dt-bindings/interrupt-controller/irq.h> #include <dt-bindings/interrupt-controller/arm-gic.h> +#include <dt-bindings/clock/a1-pll-clkc.h> +#include <dt-bindings/clock/a1-clkc.h> / { compatible = "amlogic,a1"; @@ -74,6 +76,30 @@ #size-cells = <2>; ranges = <0x0 0x0 0x0 0xfe000000 0x0 0x1000000>; + clkc_periphs: periphs-clock-controller@800 { + compatible = "amlogic,a1-periphs-clkc"; + #clock-cells = <1>; + reg = <0 0x800 0 0x104>; + clocks = <&clkc_pll CLKID_FCLK_DIV2>, + <&clkc_pll CLKID_FCLK_DIV3>, + <&clkc_pll CLKID_FCLK_DIV5>, + <&clkc_pll CLKID_FCLK_DIV7>, + <&clkc_pll CLKID_HIFI_PLL>, + <&xtal>; + clock-names = "fclk_div2", "fclk_div3", + "fclk_div5", "fclk_div7", + "hifi_pll", "xtal"; + }; + + clkc_pll: pll-clock-controller@7c80 { + compatible = "amlogic,a1-pll-clkc"; + #clock-cells = <1>; + reg = <0 0x7c80 0 0x21c>; + clocks = <&clkc_periphs CLKID_XTAL_FIXPLL>, + <&clkc_periphs CLKID_XTAL_HIFIPLL>; + clock-names = "xtal_fixpll", "xtal_hifipll"; + }; + uart_AO: serial@1c00 { compatible = "amlogic,meson-gx-uart", "amlogic,meson-ao-uart";
Add A1 periphs and PLL clock controller nodes, Some clocks in periphs controller are the parents of PLL clocks, Meanwhile some clocks in PLL controller are those of periphs clocks. They rely on each other. Compared with the previous series, the register region is only for the clock. So syscon is not used in A1. Signed-off-by: Jian Hu <jian.hu@amlogic.com> --- arch/arm64/boot/dts/amlogic/meson-a1.dtsi | 26 +++++++++++++++++++++++ 1 file changed, 26 insertions(+)