| Message ID | 1375195462-19566-5-git-send-email-maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Maxime, Overall this looks good to me, but I have some small comments: El 30/07/13 11:44, Maxime Ripard escribió: > Now that the clock driver has support for the A31 clocks, we can add > them to the DTSI and start using them in the relevant hardware blocks. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > arch/arm/boot/dts/sun6i-a31.dtsi | 137 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 127 insertions(+), 10 deletions(-) > > diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi > index 0d13dc6..c6a3a91 100644 > --- a/arch/arm/boot/dts/sun6i-a31.dtsi > +++ b/arch/arm/boot/dts/sun6i-a31.dtsi > @@ -51,13 +51,130 @@ > > clocks { > #address-cells = <1>; > - #size-cells = <0>; > + #size-cells = <1>; > + ranges; > > - osc: oscillator { > + osc24M: hosc { Please use osc24M and osc32k instead of hosc and losc, respectively. > #clock-cells = <0>; > compatible = "fixed-clock"; > clock-frequency = <24000000>; Is osc24M not gatable on A31? > }; > + > + osc32k: losc { > + #clock-cells = <0>; > + compatible = "fixed-clock"; > + clock-frequency = <32768>; > + }; > + > + pll1: pll1@01c20000 { > + #clock-cells = <0>; > + compatible = "allwinner,sun6i-pll1-clk"; > + reg = <0x01c20000 0x4>; > + clocks = <&osc24M>; > + }; > + > + /* > + * This is a dummy clock, to be used as placeholder on > + * other mux clocks when a specific parent clock is not > + * yet implemented. It should be dropped when the driver > + * is complete. > + */ > + pll6: pll6 { > + #clock-cells = <0>; > + compatible = "fixed-clock"; > + clock-frequency = <0>; > + }; > + > + cpu: cpu@01c20050 { > + #clock-cells = <0>; > + compatible = "allwinner,sun4i-cpu-clk"; > + reg = <0x01c20050 0x4>; > + clocks = <&osc32k>, <&osc24M>, <&pll1>, <&pll1>; Listing pll1 twice doesn't sound correct, but vendor code seems to indicate so. A comment to clarify it's not a typo would be good I think. > + }; > + > + axi: axi@01c20050 { > + #clock-cells = <0>; > + compatible = "allwinner,sun4i-axi-clk"; > + reg = <0x01c20050 0x4>; > + clocks = <&cpu>; > + }; > + > + ahb1_mux: ahb1_mux@01c20054 { > + #clock-cells = <0>; > + compatible = "allwinner,sun6i-ahb1-mux-clk"; > + reg = <0x01c20054 0x4>; > + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>; > + }; > + > + ahb1: ahb1@01c20054 { > + #clock-cells = <0>; > + compatible = "allwinner,sun4i-ahb-clk"; > + reg = <0x01c20054 0x4>; > + clocks = <&ahb1_mux>; > + }; Depending on when this lands, I believe these two above could be merged into one with the refactoring introduced on my patchset. > + > + ahb1_gates: ahb1_gates@01c20060 { > + #clock-cells = <1>; > + compatible = "allwinner,sun6i-a31-ahb1-gates-clk"; > + reg = <0x01c20060 0x8>; > + clocks = <&ahb1>; > + clock-output-names = "ahb1_mipidsi", "ahb1_ss", > + "ahb1_dma", "ahb1_mmc0", "ahb1_mmc1", > + "ahb1_mmc2", "ahb1_mmc3", "ahb1_nand1", > + "ahb1_nand0", "ahb1_sdram", > + "ahb1_gmac", "ahb1_ts", "ahb1_hstimer", > + "ahb1_spi0", "ahb1_spi1", "ahb1_spi2", > + "ahb1_spi3", "ahb1_otg", "ahb1_ehci0", > + "ahb1_ehci1", "ahb1_ohci0", > + "ahb1_ohci1", "ahb1_ohci2", "ahb1_ve", > + "ahb1_lcd0", "ahb1_lcd1", "ahb1_csi", > + "ahb1_hdmi", "ahb1_de0", "ahb1_de1", > + "ahb1_fe0", "ahb1_fe1", "ahb1_mp", > + "ahb1_gpu", "ahb1_deu0", "ahb1_deu1", > + "ahb1_drc0", "ahb1_drc1"; > + }; > + > + apb1: apb1@01c20054 { > + #clock-cells = <0>; > + compatible = "allwinner,sun4i-apb0-clk"; > + reg = <0x01c20054 0x4>; > + clocks = <&ahb1>; > + }; > + > + apb1_gates: apb1_gates@01c20060 { > + #clock-cells = <1>; > + compatible = "allwinner,sun6i-a31-apb1-gates-clk"; > + reg = <0x01c20068 0x4>; > + clocks = <&apb1>; > + clock-output-names = "apb1_codec", "apb1_digital_mic", > + "apb1_pio", "apb1_daudio0", > + "apb1_daudio1"; > + }; > + > + apb2_mux: apb2_mux@01c20058 { > + #clock-cells = <0>; > + compatible = "allwinner,sun4i-apb1-mux-clk"; > + reg = <0x01c20058 0x4>; > + clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>; > + }; > + > + apb2: apb2@01c20058 { > + #clock-cells = <0>; > + compatible = "allwinner,sun6i-apb2-div-clk"; > + reg = <0x01c20058 0x4>; > + clocks = <&apb2_mux>; > + }; Ditto for apb2_mux and apb2. > + > + apb2_gates: apb2_gates@01c2006c { > + #clock-cells = <1>; > + compatible = "allwinner,sun6i-a31-apb2-gates-clk"; > + reg = <0x01c2006c 0x8>; > + clocks = <&apb2>; > + clock-output-names = "apb2_i2c0", "apb2_i2c1", > + "apb2_i2c2", "apb2_i2c3", "apb2_uart0", > + "apb2_uart1", "apb2_uart2", "apb2_uart3", > + "apb2_uart4", "apb2_uart5"; > + }; > }; > > soc@01c20000 { > @@ -69,7 +186,7 @@ > compatible = "allwinner,sun6i-a31-pinctrl"; > reg = <0x01c20800 0x400>; > interrupts = <0 11 1>, <0 15 1>, <0 16 1>, <0 17 1>; > - clocks = <&osc>; > + clocks = <&apb1_gates 5>; > gpio-controller; > interrupt-controller; > #address-cells = <1>; > @@ -92,7 +209,7 @@ > <0 20 1>, > <0 21 1>, > <0 22 1>; > - clocks = <&osc>; > + clocks = <&osc24M>; > }; > > wdt1: watchdog@01c20ca0 { > @@ -106,7 +223,7 @@ > interrupts = <0 0 1>; > reg-shift = <2>; > reg-io-width = <4>; > - clocks = <&osc>; > + clocks = <&apb2_gates 16>; > status = "disabled"; > }; > > @@ -116,7 +233,7 @@ > interrupts = <0 1 1>; > reg-shift = <2>; > reg-io-width = <4>; > - clocks = <&osc>; > + clocks = <&apb2_gates 17>; > status = "disabled"; > }; > > @@ -126,7 +243,7 @@ > interrupts = <0 2 1>; > reg-shift = <2>; > reg-io-width = <4>; > - clocks = <&osc>; > + clocks = <&apb2_gates 18>; > status = "disabled"; > }; > > @@ -136,7 +253,7 @@ > interrupts = <0 3 1>; > reg-shift = <2>; > reg-io-width = <4>; > - clocks = <&osc>; > + clocks = <&apb2_gates 19>; > status = "disabled"; > }; > > @@ -146,7 +263,7 @@ > interrupts = <0 4 1>; > reg-shift = <2>; > reg-io-width = <4>; > - clocks = <&osc>; > + clocks = <&apb2_gates 20>; > status = "disabled"; > }; > > @@ -156,7 +273,7 @@ > interrupts = <0 5 1>; > reg-shift = <2>; > reg-io-width = <4>; > - clocks = <&osc>; > + clocks = <&apb2_gates 21>; > status = "disabled"; > }; > > Thanks for working on this! Emilio
Hi Emilio, On Tue, Jul 30, 2013 at 10:36:46PM -0300, Emilio López wrote: > Hi Maxime, > > Overall this looks good to me, but I have some small comments: > > El 30/07/13 11:44, Maxime Ripard escribió: > > Now that the clock driver has support for the A31 clocks, we can add > > them to the DTSI and start using them in the relevant hardware blocks. > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > arch/arm/boot/dts/sun6i-a31.dtsi | 137 ++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 127 insertions(+), 10 deletions(-) > > > > diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi > > index 0d13dc6..c6a3a91 100644 > > --- a/arch/arm/boot/dts/sun6i-a31.dtsi > > +++ b/arch/arm/boot/dts/sun6i-a31.dtsi > > @@ -51,13 +51,130 @@ > > > > clocks { > > #address-cells = <1>; > > - #size-cells = <0>; > > + #size-cells = <1>; > > + ranges; > > > > - osc: oscillator { > > + osc24M: hosc { > > Please use osc24M and osc32k instead of hosc and losc, respectively. The problem is a bit more complex than that. On the A31, the losc clock is actually a mux between an external oscillator running at 32kHz, and the internal oscillator running at 667MHz, that would be scaled down. Support for this mux is not quite there yet, since I've not seen any documentation for it, but this would allow to just rearrange losc parents and compatible when we will had such support. Hence why I chose these names. > > #clock-cells = <0>; > > compatible = "fixed-clock"; > > clock-frequency = <24000000>; > > Is osc24M not gatable on A31? Nope. > > }; > > + > > + osc32k: losc { > > + #clock-cells = <0>; > > + compatible = "fixed-clock"; > > + clock-frequency = <32768>; > > + }; > > + > > + pll1: pll1@01c20000 { > > + #clock-cells = <0>; > > + compatible = "allwinner,sun6i-pll1-clk"; > > + reg = <0x01c20000 0x4>; > > + clocks = <&osc24M>; > > + }; > > + > > + /* > > + * This is a dummy clock, to be used as placeholder on > > + * other mux clocks when a specific parent clock is not > > + * yet implemented. It should be dropped when the driver > > + * is complete. > > + */ > > + pll6: pll6 { > > + #clock-cells = <0>; > > + compatible = "fixed-clock"; > > + clock-frequency = <0>; > > + }; > > + > > + cpu: cpu@01c20050 { > > + #clock-cells = <0>; > > + compatible = "allwinner,sun4i-cpu-clk"; > > + reg = <0x01c20050 0x4>; > > + clocks = <&osc32k>, <&osc24M>, <&pll1>, <&pll1>; > > Listing pll1 twice doesn't sound correct, but vendor code seems to > indicate so. A comment to clarify it's not a typo would be good I think. Yes, I suspect an error in the datasheet, but until proven otherwise, it's that way. I'll add a comment. > > + }; > > + > > + axi: axi@01c20050 { > > + #clock-cells = <0>; > > + compatible = "allwinner,sun4i-axi-clk"; > > + reg = <0x01c20050 0x4>; > > + clocks = <&cpu>; > > + }; > > + > > + ahb1_mux: ahb1_mux@01c20054 { > > + #clock-cells = <0>; > > + compatible = "allwinner,sun6i-ahb1-mux-clk"; > > + reg = <0x01c20054 0x4>; > > + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>; > > + }; > > + > > + ahb1: ahb1@01c20054 { > > + #clock-cells = <0>; > > + compatible = "allwinner,sun4i-ahb-clk"; > > + reg = <0x01c20054 0x4>; > > + clocks = <&ahb1_mux>; > > + }; > > Depending on when this lands, I believe these two above could be merged > into one with the refactoring introduced on my patchset. Since your patchset is still in RFC and we had no comments from Mike so far, while this one looks pretty similar to the one we had before, I guess the safest thing to do would be to rebase your patches on top of this ones. But it's right those clocks (AHB1 and APB2) will benefit from your work as well :) Maxime
Hi, El 31/07/13 05:11, kevin.z.m escribió: > Hi, Maxime, > >> The problem is a bit more complex than that. > >> On the A31, the losc clock is actually a mux between an external >> oscillator running at 32kHz, and the internal oscillator running at >> 667MHz, that would be scaled down. > >> Support for this mux is not quite there yet, since I've not seen any >> documentation for it, but this would allow to just rearrange losc >> parents and compatible when we will had such support. > > I think there is some misunderstanding. All allwinner's platforms have > 2 losc clock source. One is the external 32KHz oscillator, and the other > is internal 32kHz R/C circuit. The internal 32k R/C circuit can't provide > a very exact clock on 32KHz. The mux is just for select external osc or > internal > 32KHz R/C. If there is no external 32kHz oscillator, we can replace it with > internal 32kHz R/C for some low speed but not exact clock requirement. Thanks for the clarification :) (snip) >> > + ahb1_mux: ahb1_mux@01c20054 { >> > + #clock-cells = <0>; >> > + compatible = "allwinner,sun6i-ahb1-mux-clk"; >> > + reg = <0x01c20054 0x4>; >> > + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>; >> > + }; >> > + >> > + ahb1: ahb1@01c20054 { >> > + #clock-cells = <0>; >> > + compatible = "allwinner,sun4i-ahb-clk"; >> > + reg = <0x01c20054 0x4>; >> > + clocks = <&ahb1_mux>; >> > + }; >> >> Depending on when this lands, I believe these two above could be merged >> into one with the refactoring introduced on my patchset. > > Since your patchset is still in RFC and we had no comments from Mike so > far, while this one looks pretty similar to the one we had before, I > guess the safest thing to do would be to rebase your patches on top of > this ones. Ok, I'll do that. Cheers, Emilio
Hi Kevin, On Wed, Jul 31, 2013 at 04:11:11PM +0800, kevin.z.m wrote: > Hi, Maxime, > > > The problem is a bit more complex than that. > > > On the A31, the losc clock is actually a mux between an external > > oscillator running at 32kHz, and the internal oscillator running at > > 667MHz, that would be scaled down. > > > Support for this mux is not quite there yet, since I've not seen any > > documentation for it, but this would allow to just rearrange losc > > parents and compatible when we will had such support. > > I think there is some misunderstanding. All allwinner's platforms have > 2 losc clock source. One is the external 32KHz oscillator, and the other > is internal 32kHz R/C circuit. The internal 32k R/C circuit can't provide > a very exact clock on 32KHz. The mux is just for select external osc or internal > 32KHz R/C. If there is no external 32kHz oscillator, we can replace it with > internal 32kHz R/C for some low speed but not exact clock requirement. Thanks for the clarification. How does one select either the internal or the external 32kHz oscillator? Is this done in software (I can't find any reference to that in the user manuals) or in hardware? Thanks, Maxime
Hi, Maxime, > How does one select either the internal or the external 32kHz > oscillator? Is this done in software (I can't find any reference to that > in the user manuals) or in hardware? For A20, it should be in the Timer module, and there is a register named "LOSC_CTRL_REG". For A31, it is in the RTC module, same name of "LOSC_CTRL_REG". Best Regards, kevin.z.m
On Wed, Jul 31, 2013 at 08:10:32PM +0800, kevin.z.m wrote: > > How does one select either the internal or the external 32kHz > > oscillator? Is this done in software (I can't find any reference to that > > in the user manuals) or in hardware? > For A20, it should be in the Timer module, and there is a register named "LOSC_CTRL_REG". > For A31, it is in the RTC module, same name of "LOSC_CTRL_REG". Ok, just so that we're on the same page, we have two oscillators running at 32kHz, the internal being always there, and the external that could or could not be there, and it's each different hardware block that can be wired to one, the other or both? There's no global mux that could use either one of the two 32k to provide the only 32kHz source in the system, right?
Hi Kevin, On Thu, Aug 01, 2013 at 08:34:35AM +0800, kevin.z.m wrote: > > Ok, just so that we're on the same page, we have two oscillators running > > at 32kHz, the internal being always there, and the external that could > > or could not be there, and it's each different hardware block that can > > be wired to one, the other or both? > The selection of internal or external is a global setting for system. > Hardware blocks ?such as cpu?twi for ex.) just see a 32kHz clock > source, but they don't care where the 32kHz clock > from. Ok. > > There's no global mux that could use either one of the two 32k to > > provide the only 32kHz source in the system, right? > There is a global mux to select internal or external, but no mux for > every hardware blocks. I think that the mux need not be managed in the > clock driver. It should be set earlier in the boot loader. > And the selection is keeping either system is power-on or power-off, > when there is a power supply with battery or DC. Because the mux is in > RTC power domain. Yes, in that case, we mostly don't care for now. Maybe we will at some point if we need to add power management features, but we'd better consider a fixed-rate clock for now and not worry too much about it. Thanks for your help! Maxime
diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi index 0d13dc6..c6a3a91 100644 --- a/arch/arm/boot/dts/sun6i-a31.dtsi +++ b/arch/arm/boot/dts/sun6i-a31.dtsi @@ -51,13 +51,130 @@ clocks { #address-cells = <1>; - #size-cells = <0>; + #size-cells = <1>; + ranges; - osc: oscillator { + osc24M: hosc { #clock-cells = <0>; compatible = "fixed-clock"; clock-frequency = <24000000>; }; + + osc32k: losc { + #clock-cells = <0>; + compatible = "fixed-clock"; + clock-frequency = <32768>; + }; + + pll1: pll1@01c20000 { + #clock-cells = <0>; + compatible = "allwinner,sun6i-pll1-clk"; + reg = <0x01c20000 0x4>; + clocks = <&osc24M>; + }; + + /* + * This is a dummy clock, to be used as placeholder on + * other mux clocks when a specific parent clock is not + * yet implemented. It should be dropped when the driver + * is complete. + */ + pll6: pll6 { + #clock-cells = <0>; + compatible = "fixed-clock"; + clock-frequency = <0>; + }; + + cpu: cpu@01c20050 { + #clock-cells = <0>; + compatible = "allwinner,sun4i-cpu-clk"; + reg = <0x01c20050 0x4>; + clocks = <&osc32k>, <&osc24M>, <&pll1>, <&pll1>; + }; + + axi: axi@01c20050 { + #clock-cells = <0>; + compatible = "allwinner,sun4i-axi-clk"; + reg = <0x01c20050 0x4>; + clocks = <&cpu>; + }; + + ahb1_mux: ahb1_mux@01c20054 { + #clock-cells = <0>; + compatible = "allwinner,sun6i-ahb1-mux-clk"; + reg = <0x01c20054 0x4>; + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>; + }; + + ahb1: ahb1@01c20054 { + #clock-cells = <0>; + compatible = "allwinner,sun4i-ahb-clk"; + reg = <0x01c20054 0x4>; + clocks = <&ahb1_mux>; + }; + + ahb1_gates: ahb1_gates@01c20060 { + #clock-cells = <1>; + compatible = "allwinner,sun6i-a31-ahb1-gates-clk"; + reg = <0x01c20060 0x8>; + clocks = <&ahb1>; + clock-output-names = "ahb1_mipidsi", "ahb1_ss", + "ahb1_dma", "ahb1_mmc0", "ahb1_mmc1", + "ahb1_mmc2", "ahb1_mmc3", "ahb1_nand1", + "ahb1_nand0", "ahb1_sdram", + "ahb1_gmac", "ahb1_ts", "ahb1_hstimer", + "ahb1_spi0", "ahb1_spi1", "ahb1_spi2", + "ahb1_spi3", "ahb1_otg", "ahb1_ehci0", + "ahb1_ehci1", "ahb1_ohci0", + "ahb1_ohci1", "ahb1_ohci2", "ahb1_ve", + "ahb1_lcd0", "ahb1_lcd1", "ahb1_csi", + "ahb1_hdmi", "ahb1_de0", "ahb1_de1", + "ahb1_fe0", "ahb1_fe1", "ahb1_mp", + "ahb1_gpu", "ahb1_deu0", "ahb1_deu1", + "ahb1_drc0", "ahb1_drc1"; + }; + + apb1: apb1@01c20054 { + #clock-cells = <0>; + compatible = "allwinner,sun4i-apb0-clk"; + reg = <0x01c20054 0x4>; + clocks = <&ahb1>; + }; + + apb1_gates: apb1_gates@01c20060 { + #clock-cells = <1>; + compatible = "allwinner,sun6i-a31-apb1-gates-clk"; + reg = <0x01c20068 0x4>; + clocks = <&apb1>; + clock-output-names = "apb1_codec", "apb1_digital_mic", + "apb1_pio", "apb1_daudio0", + "apb1_daudio1"; + }; + + apb2_mux: apb2_mux@01c20058 { + #clock-cells = <0>; + compatible = "allwinner,sun4i-apb1-mux-clk"; + reg = <0x01c20058 0x4>; + clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>; + }; + + apb2: apb2@01c20058 { + #clock-cells = <0>; + compatible = "allwinner,sun6i-apb2-div-clk"; + reg = <0x01c20058 0x4>; + clocks = <&apb2_mux>; + }; + + apb2_gates: apb2_gates@01c2006c { + #clock-cells = <1>; + compatible = "allwinner,sun6i-a31-apb2-gates-clk"; + reg = <0x01c2006c 0x8>; + clocks = <&apb2>; + clock-output-names = "apb2_i2c0", "apb2_i2c1", + "apb2_i2c2", "apb2_i2c3", "apb2_uart0", + "apb2_uart1", "apb2_uart2", "apb2_uart3", + "apb2_uart4", "apb2_uart5"; + }; }; soc@01c20000 { @@ -69,7 +186,7 @@ compatible = "allwinner,sun6i-a31-pinctrl"; reg = <0x01c20800 0x400>; interrupts = <0 11 1>, <0 15 1>, <0 16 1>, <0 17 1>; - clocks = <&osc>; + clocks = <&apb1_gates 5>; gpio-controller; interrupt-controller; #address-cells = <1>; @@ -92,7 +209,7 @@ <0 20 1>, <0 21 1>, <0 22 1>; - clocks = <&osc>; + clocks = <&osc24M>; }; wdt1: watchdog@01c20ca0 { @@ -106,7 +223,7 @@ interrupts = <0 0 1>; reg-shift = <2>; reg-io-width = <4>; - clocks = <&osc>; + clocks = <&apb2_gates 16>; status = "disabled"; }; @@ -116,7 +233,7 @@ interrupts = <0 1 1>; reg-shift = <2>; reg-io-width = <4>; - clocks = <&osc>; + clocks = <&apb2_gates 17>; status = "disabled"; }; @@ -126,7 +243,7 @@ interrupts = <0 2 1>; reg-shift = <2>; reg-io-width = <4>; - clocks = <&osc>; + clocks = <&apb2_gates 18>; status = "disabled"; }; @@ -136,7 +253,7 @@ interrupts = <0 3 1>; reg-shift = <2>; reg-io-width = <4>; - clocks = <&osc>; + clocks = <&apb2_gates 19>; status = "disabled"; }; @@ -146,7 +263,7 @@ interrupts = <0 4 1>; reg-shift = <2>; reg-io-width = <4>; - clocks = <&osc>; + clocks = <&apb2_gates 20>; status = "disabled"; }; @@ -156,7 +273,7 @@ interrupts = <0 5 1>; reg-shift = <2>; reg-io-width = <4>; - clocks = <&osc>; + clocks = <&apb2_gates 21>; status = "disabled"; };
Now that the clock driver has support for the A31 clocks, we can add them to the DTSI and start using them in the relevant hardware blocks. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- arch/arm/boot/dts/sun6i-a31.dtsi | 137 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 127 insertions(+), 10 deletions(-)