| Message ID | 1445444428-4652-1-git-send-email-jenskuske@gmail.com |
|---|---|
| State | Under Review, archived |
| Headers | show |
On Wed, Oct 21, 2015 at 06:20:26PM +0200, Jens Kuske wrote: > Adding a new compatible allows us to define SoC specific behaviour > if necessary, for example forcing a particular device out of reset > even if no driver is actually using it. > > Signed-off-by: Jens Kuske <jenskuske@gmail.com> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Thanks! Maxime
Hi, On Wed, Oct 21, 2015 at 06:20:27PM +0200, Jens Kuske wrote: > The Allwinner H3 is a home entertainment system oriented SoC with > four Cortex-A7 cores and a Mali-400MP2 GPU. > > Signed-off-by: Jens Kuske <jenskuske@gmail.com> > --- > arch/arm/boot/dts/sun8i-h3.dtsi | 499 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 499 insertions(+) > create mode 100644 arch/arm/boot/dts/sun8i-h3.dtsi > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi > new file mode 100644 > index 0000000..4114e17 > --- /dev/null > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > @@ -0,0 +1,499 @@ > +/* > + * Copyright (C) 2015 Jens Kuske <jenskuske@gmail.com> > + * > + * This file is dual-licensed: you can use it either under the terms > + * of the GPL or the X11 license, at your option. Note that this dual > + * licensing only applies to this file, and not this project as a > + * whole. > + * > + * a) This file is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of the > + * License, or (at your option) any later version. > + * > + * This file is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * Or, alternatively, > + * > + * b) Permission is hereby granted, free of charge, to any person > + * obtaining a copy of this software and associated documentation > + * files (the "Software"), to deal in the Software without > + * restriction, including without limitation the rights to use, > + * copy, modify, merge, publish, distribute, sublicense, and/or > + * sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following > + * conditions: > + * > + * The above copyright notice and this permission notice shall be > + * included in all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES > + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT > + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, > + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > + * OTHER DEALINGS IN THE SOFTWARE. > + */ > + > +#include "skeleton.dtsi" > + > +#include <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/pinctrl/sun4i-a10.h> > + > +/ { > + interrupt-parent = <&gic>; > + > + cpus { > + #address-cells = <1>; > + #size-cells = <0>; > + > + cpu@0 { > + compatible = "arm,cortex-a7"; > + device_type = "cpu"; > + reg = <0>; > + }; > + > + cpu@1 { > + compatible = "arm,cortex-a7"; > + device_type = "cpu"; > + reg = <1>; > + }; > + > + cpu@2 { > + compatible = "arm,cortex-a7"; > + device_type = "cpu"; > + reg = <2>; > + }; > + > + cpu@3 { > + compatible = "arm,cortex-a7"; > + device_type = "cpu"; > + reg = <3>; > + }; > + }; > + > + timer { > + compatible = "arm,armv7-timer"; > + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>; > + clock-frequency = <24000000>; > + arm,cpu-registers-not-fw-configured; > + }; > + > + memory { > + reg = <0x40000000 0x80000000>; > + }; > + > + clocks { > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + osc24M: osc24M_clk { > + #clock-cells = <0>; > + compatible = "fixed-clock"; > + clock-frequency = <24000000>; > + clock-output-names = "osc24M"; > + }; > + > + osc32k: osc32k_clk { > + #clock-cells = <0>; > + compatible = "fixed-clock"; > + clock-frequency = <32768>; > + clock-output-names = "osc32k"; > + }; > + > + pll1: clk@01c20000 { > + #clock-cells = <0>; > + compatible = "allwinner,sun8i-a23-pll1-clk"; > + reg = <0x01c20000 0x4>; > + clocks = <&osc24M>; > + clock-output-names = "pll1"; > + }; > + > + /* dummy clock until actually implemented */ > + pll5: pll5_clk { > + #clock-cells = <0>; > + compatible = "fixed-clock"; > + clock-frequency = <0>; > + clock-output-names = "pll5"; > + }; > + > + pll6: clk@01c20028 { > + #clock-cells = <1>; > + compatible = "allwinner,sun6i-a31-pll6-clk"; > + reg = <0x01c20028 0x4>; > + clocks = <&osc24M>; > + clock-output-names = "pll6", "pll6x2", "pll6d2"; > + }; > + > + pll8: clk@01c20044 { > + #clock-cells = <0>; > + compatible = "allwinner,sun6i-a31-pll6-clk"; > + reg = <0x01c20044 0x4>; > + clocks = <&osc24M>; > + clock-output-names = "pll8", "pll8x2"; > + }; > + > + cpu: cpu_clk@01c20050 { > + #clock-cells = <0>; > + compatible = "allwinner,sun4i-a10-cpu-clk"; > + reg = <0x01c20050 0x4>; > + clocks = <&osc32k>, <&osc24M>, <&pll1>, <&pll1>; > + clock-output-names = "cpu"; > + }; > + > + axi: axi_clk@01c20050 { > + #clock-cells = <0>; > + compatible = "allwinner,sun4i-a10-axi-clk"; > + reg = <0x01c20050 0x4>; > + clocks = <&cpu>; > + clock-output-names = "axi"; > + }; > + > + ahb1: ahb1_clk@01c20054 { > + #clock-cells = <0>; > + compatible = "allwinner,sun6i-a31-ahb1-clk"; > + reg = <0x01c20054 0x4>; > + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6 0>; > + clock-output-names = "ahb1"; > + }; > + > + ahb2: ahb2_clk@01c2005c { > + #clock-cells = <0>; > + compatible = "allwinner,sun8i-h3-ahb2-clk"; > + reg = <0x01c2005c 0x4>; > + clocks = <&ahb1>, <&pll6 2>; > + clock-output-names = "ahb2"; > + }; > + > + apb1: apb1_clk@01c20054 { > + #clock-cells = <0>; > + compatible = "allwinner,sun4i-a10-apb0-clk"; > + reg = <0x01c20054 0x4>; > + clocks = <&ahb1>; > + clock-output-names = "apb1"; > + }; > + > + apb2: apb2_clk@01c20058 { > + #clock-cells = <0>; > + compatible = "allwinner,sun4i-a10-apb1-clk"; > + reg = <0x01c20058 0x4>; > + clocks = <&osc32k>, <&osc24M>, <&pll6 0>, <&pll6 0>; > + clock-output-names = "apb2"; > + }; > + > + bus_gates: clk@01c20060 { > + #clock-cells = <1>; > + compatible = "allwinner,sun8i-h3-bus-gates-clk"; > + reg = <0x01c20060 0x14>; > + clock-indices = <5>, <6>, <8>, > + <9>, <10>, <13>, > + <14>, <17>, <18>, > + <19>, <20>, > + <21>, <23>, > + <24>, <25>, > + <26>, <27>, > + <28>, <29>, > + <30>, <31>, <32>, > + <35>, <36>, <37>, > + <40>, <41>, <43>, > + <44>, <52>, <53>, > + <54>, <64>, > + <65>, <69>, <72>, > + <76>, <77>, <78>, > + <96>, <97>, <98>, > + <112>, <113>, > + <114>, <115>, <116>, > + <128>, <135>; > + clocks = <&ahb1>, <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb2>, <&ahb1>, > + <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb2>, > + <&ahb2>, <&ahb2>, <&ahb1>, > + <&ahb1>, <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, <&ahb1>, > + <&ahb1>, <&apb1>, > + <&apb1>, <&apb1>, <&apb1>, > + <&apb1>, <&apb1>, <&apb1>, > + <&apb2>, <&apb2>, <&apb2>, > + <&apb2>, <&apb2>, > + <&apb2>, <&apb2>, <&apb2>, > + <&ahb1>, <&ahb1>; > + clock-output-names = "ahb1_ce", "ahb1_dma", "ahb1_mmc0", > + "ahb1_mmc1", "ahb1_mmc2", "ahb1_nand", > + "ahb1_sdram", "ahb2_gmac", "ahb1_ts", > + "ahb1_hstimer", "ahb1_spi0", > + "ahb1_spi1", "ahb1_otg", > + "ahb1_otg_ehci0", "ahb1_ehic1", > + "ahb1_ehic2", "ahb1_ehic3", > + "ahb1_otg_ohci0", "ahb2_ohic1", > + "ahb2_ohic2", "ahb2_ohic3", "ahb1_ve", > + "ahb1_lcd0", "ahb1_lcd1", "ahb1_deint", > + "ahb1_csi", "ahb1_tve", "ahb1_hdmi", > + "ahb1_de", "ahb1_gpu", "ahb1_msgbox", > + "ahb1_spinlock", "apb1_codec", > + "apb1_spdif", "apb1_pio", "apb1_ths", > + "apb1_i2s0", "apb1_i2s1", "apb1_i2s2", > + "apb2_i2c0", "apb2_i2c1", "apb2_i2c2", > + "apb2_uart0", "apb2_uart1", > + "apb2_uart2", "apb2_uart3", "apb2_scr", > + "ahb1_ephy", "ahb1_dbg"; > + }; > + > + mmc0_clk: clk@01c20088 { > + #clock-cells = <1>; > + compatible = "allwinner,sun4i-a10-mmc-clk"; > + reg = <0x01c20088 0x4>; > + clocks = <&osc24M>, <&pll6 0>, <&pll8 0>; > + clock-output-names = "mmc0", > + "mmc0_output", > + "mmc0_sample"; > + }; > + > + mmc1_clk: clk@01c2008c { > + #clock-cells = <1>; > + compatible = "allwinner,sun4i-a10-mmc-clk"; > + reg = <0x01c2008c 0x4>; > + clocks = <&osc24M>, <&pll6 0>, <&pll8 0>; > + clock-output-names = "mmc1", > + "mmc1_output", > + "mmc1_sample"; > + }; > + > + mmc2_clk: clk@01c20090 { > + #clock-cells = <1>; > + compatible = "allwinner,sun4i-a10-mmc-clk"; > + reg = <0x01c20090 0x4>; > + clocks = <&osc24M>, <&pll6 0>, <&pll8 0>; > + clock-output-names = "mmc2", > + "mmc2_output", > + "mmc2_sample"; > + }; > + > + mbus_clk: clk@01c2015c { > + #clock-cells = <0>; > + compatible = "allwinner,sun8i-a23-mbus-clk"; > + reg = <0x01c2015c 0x4>; > + clocks = <&osc24M>, <&pll6 1>, <&pll5>; > + clock-output-names = "mbus"; > + }; > + }; > + > + soc@01c00000 { We had some issues with this in the past, especially since it's wrong and the SoC registers definitions start at 0, with the SRAMs. It would be better if you removed it entirely like we did in the A80 DTSI. > + uart0: serial@01c28000 { > + compatible = "snps,dw-apb-uart"; > + reg = <0x01c28000 0x400>; > + interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>; > + reg-shift = <2>; > + reg-io-width = <4>; > + clocks = <&bus_gates 112>; > + resets = <&bus_rst 208>; It's a bit weird that the clocks and reset indices don't match, usually they do. What's even weirder is that there's a 96 offset between the two (4 * 32), is this expected? Thanks! Maxime
On Thu, 22 Oct 2015 10:05:08 +0200 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > > + uart0: serial@01c28000 { > > + compatible = "snps,dw-apb-uart"; > > + reg = <0x01c28000 0x400>; > > + interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>; > > + reg-shift = <2>; > > + reg-io-width = <4>; > > + clocks = <&bus_gates 112>; > > + resets = <&bus_rst 208>; > > It's a bit weird that the clocks and reset indices don't match, > usually they do. > > What's even weirder is that there's a 96 offset between the two (4 * > 32), is this expected? Yes, this is conform to the H3 documentation.
On Thu, Oct 22, 2015 at 10:29:59AM +0200, Jean-Francois Moine wrote: > On Thu, 22 Oct 2015 10:05:08 +0200 > Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > > > > + uart0: serial@01c28000 { > > > + compatible = "snps,dw-apb-uart"; > > > + reg = <0x01c28000 0x400>; > > > + interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>; > > > + reg-shift = <2>; > > > + reg-io-width = <4>; > > > + clocks = <&bus_gates 112>; > > > + resets = <&bus_rst 208>; > > > > It's a bit weird that the clocks and reset indices don't match, > > usually they do. > > > > What's even weirder is that there's a 96 offset between the two (4 * > > 32), is this expected? > > Yes, this is conform to the H3 documentation. Not really. The uart0 reset is the bit 16, in the reset register 4. 4 * 32 + 16 = 44. Not 112, but still not 208 either. Maxime
On Thu, 22 Oct 2015 10:47:35 +0200 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Not really. The uart0 reset is the bit 16, in the reset register 4. > > 4 * 32 + 16 = 44. > > Not 112, but still not 208 either. The registers are numbered 1..5, then (4 - 1) * 32 + 16 = 112
On Thu, Oct 22, 2015 at 10:57:45AM +0200, Jean-Francois Moine wrote: > On Thu, 22 Oct 2015 10:47:35 +0200 > Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > > > Not really. The uart0 reset is the bit 16, in the reset register 4. > > > > 4 * 32 + 16 = 44. > > > > Not 112, but still not 208 either. > > The registers are numbered 1..5, then > > (4 - 1) * 32 + 16 = 112 Not on my version, and even then, UARTs are on the last reset register, which would still make 144. Maxime
On 22/10/15 11:14, Maxime Ripard wrote: > On Thu, Oct 22, 2015 at 10:57:45AM +0200, Jean-Francois Moine wrote: >> On Thu, 22 Oct 2015 10:47:35 +0200 >> Maxime Ripard <maxime.ripard@free-electrons.com> wrote: >> >>> Not really. The uart0 reset is the bit 16, in the reset register 4. >>> >>> 4 * 32 + 16 = 44. >>> >>> Not 112, but still not 208 either. >> >> The registers are numbered 1..5, then >> >> (4 - 1) * 32 + 16 = 112 > > Not on my version, and even then, UARTs are on the last reset > register, which would still make 144. > > Maxime > There are holes between reg2 and reg3 and reg4 for some reason, but even if we would correct that with some of_xlate() function they won't completely line up with the gates. Jens -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 21 Oct 2015 18:20:27 +0200 Jens Kuske <jenskuske@gmail.com> wrote: > The Allwinner H3 is a home entertainment system oriented SoC with > four Cortex-A7 cores and a Mali-400MP2 GPU. > > Signed-off-by: Jens Kuske <jenskuske@gmail.com> > --- > arch/arm/boot/dts/sun8i-h3.dtsi | 499 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 499 insertions(+) > create mode 100644 arch/arm/boot/dts/sun8i-h3.dtsi > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi > new file mode 100644 > index 0000000..4114e17 > --- /dev/null > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > @@ -0,0 +1,499 @@ [snip] > + pll6: clk@01c20028 { > + #clock-cells = <1>; > + compatible = "allwinner,sun6i-a31-pll6-clk"; > + reg = <0x01c20028 0x4>; > + clocks = <&osc24M>; > + clock-output-names = "pll6", "pll6x2", "pll6d2"; > + }; > + > + pll8: clk@01c20044 { > + #clock-cells = <0>; Should be <1> > + compatible = "allwinner,sun6i-a31-pll6-clk"; > + reg = <0x01c20044 0x4>; > + clocks = <&osc24M>; > + clock-output-names = "pll8", "pll8x2"; > + }; > + > + cpu: cpu_clk@01c20050 { > + #clock-cells = <0>; > + compatible = "allwinner,sun4i-a10-cpu-clk"; > + reg = <0x01c20050 0x4>; > + clocks = <&osc32k>, <&osc24M>, <&pll1>, <&pll1>; > + clock-output-names = "cpu"; > + }; [snip]
On Thu, Oct 22, 2015 at 01:30:42PM +0200, Jens Kuske wrote: > On 22/10/15 11:14, Maxime Ripard wrote: > > On Thu, Oct 22, 2015 at 10:57:45AM +0200, Jean-Francois Moine wrote: > >> On Thu, 22 Oct 2015 10:47:35 +0200 > >> Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > >> > >>> Not really. The uart0 reset is the bit 16, in the reset register 4. > >>> > >>> 4 * 32 + 16 = 44. > >>> > >>> Not 112, but still not 208 either. > >> > >> The registers are numbered 1..5, then > >> > >> (4 - 1) * 32 + 16 = 112 > > > > Not on my version, and even then, UARTs are on the last reset > > register, which would still make 144. > > > > Maxime > > > > There are holes between reg2 and reg3 and reg4 for some reason, but even > if we would correct that with some of_xlate() function they won't > completely line up with the gates. Indeed. Still, dealing with the holes and sticking to what the datasheet says seems like the right solution. Maxime
On Wed, Oct 21, 2015 at 06:20:27PM +0200, Jens Kuske wrote: > + bus_gates: clk@01c20060 { > + #clock-cells = <1>; > + compatible = "allwinner,sun8i-h3-bus-gates-clk"; > + reg = <0x01c20060 0x14>; > + clock-indices = <5>, <6>, <8>, > + <9>, <10>, <13>, > + <14>, <17>, <18>, > + <19>, <20>, > + <21>, <23>, > + <24>, <25>, > + <26>, <27>, > + <28>, <29>, > + <30>, <31>, <32>, > + <35>, <36>, <37>, > + <40>, <41>, <43>, > + <44>, <52>, <53>, > + <54>, <64>, > + <65>, <69>, <72>, > + <76>, <77>, <78>, > + <96>, <97>, <98>, > + <112>, <113>, > + <114>, <115>, <116>, > + <128>, <135>; > + clocks = <&ahb1>, <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb2>, <&ahb1>, > + <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb2>, > + <&ahb2>, <&ahb2>, <&ahb1>, > + <&ahb1>, <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, <&ahb1>, > + <&ahb1>, <&ahb1>, <&ahb1>, > + <&ahb1>, <&apb1>, > + <&apb1>, <&apb1>, <&apb1>, > + <&apb1>, <&apb1>, <&apb1>, > + <&apb2>, <&apb2>, <&apb2>, > + <&apb2>, <&apb2>, > + <&apb2>, <&apb2>, <&apb2>, > + <&ahb1>, <&ahb1>; This is not really what I had in mind... This IP has 2 parents, and only two parents. The mapping between the IPs should be done in the driver itself, not in the DT where it is very error prone and barely readable. And note that I never have expected you to use clk-simple-gates either. This is a complicated clock, unlike the other we've seen so far, it definitely deserves a driver of its own. Thanks! Maxime
On Fri, 23 Oct 2015 20:14:06 +0200 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Wed, Oct 21, 2015 at 06:20:27PM +0200, Jens Kuske wrote: > > + bus_gates: clk@01c20060 { > > + #clock-cells = <1>; > > + compatible = "allwinner,sun8i-h3-bus-gates-clk"; > > + reg = <0x01c20060 0x14>; > > + clock-indices = <5>, <6>, <8>, > > + <9>, <10>, <13>, > > + <14>, <17>, <18>, > > + <19>, <20>, > > + <21>, <23>, > > + <24>, <25>, > > + <26>, <27>, > > + <28>, <29>, > > + <30>, <31>, <32>, > > + <35>, <36>, <37>, > > + <40>, <41>, <43>, > > + <44>, <52>, <53>, > > + <54>, <64>, > > + <65>, <69>, <72>, > > + <76>, <77>, <78>, > > + <96>, <97>, <98>, > > + <112>, <113>, > > + <114>, <115>, <116>, > > + <128>, <135>; > > + clocks = <&ahb1>, <&ahb1>, <&ahb1>, > > + <&ahb1>, <&ahb1>, <&ahb1>, > > + <&ahb1>, <&ahb2>, <&ahb1>, > > + <&ahb1>, <&ahb1>, > > + <&ahb1>, <&ahb1>, > > + <&ahb1>, <&ahb1>, > > + <&ahb1>, <&ahb1>, > > + <&ahb1>, <&ahb2>, > > + <&ahb2>, <&ahb2>, <&ahb1>, > > + <&ahb1>, <&ahb1>, <&ahb1>, > > + <&ahb1>, <&ahb1>, <&ahb1>, > > + <&ahb1>, <&ahb1>, <&ahb1>, > > + <&ahb1>, <&apb1>, > > + <&apb1>, <&apb1>, <&apb1>, > > + <&apb1>, <&apb1>, <&apb1>, > > + <&apb2>, <&apb2>, <&apb2>, > > + <&apb2>, <&apb2>, > > + <&apb2>, <&apb2>, <&apb2>, > > + <&ahb1>, <&ahb1>; > > This is not really what I had in mind... > > This IP has 2 parents, and only two parents. The mapping between the > IPs should be done in the driver itself, not in the DT where it is > very error prone and barely readable. > > And note that I never have expected you to use clk-simple-gates > either. This is a complicated clock, unlike the other we've seen so > far, it definitely deserves a driver of its own. It seems that Allwinner puts the gate definitions anywhere in the array of registers, so, I think that the H3 scheme will not be the last complicated one, and if the parent clocks are in the code instead of in the DT, we will have more and more code to develop. An other way to describe the gates would be to add containers per parent (with still a small patch in the clk-simple-gates): bus_gates: clk@01c20060 { #clock-cells = <1>; compatible = "allwinner,sun8i-h3-bus-gates-clk"; reg = <0x01c20060 0x14>; ahb1_gates { clocks = <&ahb1>; clock-indices = <5>, <6>, <8>, <9>, <10>, <13>, <14>, <18>, <19>, <20>, ...; }; clock-output-names = "ahb1_ce", "ahb1_dma", "ahb1_mmc0", "ahb1_mmc1", "ahb1_mmc2", "ahb1_nand", "ahb1_sdram", "ahb1_ts", "ahb1_hstimer", "ahb1_spi0", ...; }; ahb2_gates { clocks = <&ahb2>; clock-indices = <17>, <29>, <30>, <31>, <32>, ...; clock-output-names = "ahb2_gmac", "ahb2_ohic1", "ahb2_ohic2", "ahb2_ohic3", "ahb1_ve", ...; }; apb1_gates { ... }; apb2_gates { ... }; };
On Fri, Oct 23, 2015 at 09:20:13PM +0200, Jean-Francois Moine wrote: > On Fri, 23 Oct 2015 20:14:06 +0200 > Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > > > On Wed, Oct 21, 2015 at 06:20:27PM +0200, Jens Kuske wrote: > > > + bus_gates: clk@01c20060 { > > > + #clock-cells = <1>; > > > + compatible = "allwinner,sun8i-h3-bus-gates-clk"; > > > + reg = <0x01c20060 0x14>; > > > + clock-indices = <5>, <6>, <8>, > > > + <9>, <10>, <13>, > > > + <14>, <17>, <18>, > > > + <19>, <20>, > > > + <21>, <23>, > > > + <24>, <25>, > > > + <26>, <27>, > > > + <28>, <29>, > > > + <30>, <31>, <32>, > > > + <35>, <36>, <37>, > > > + <40>, <41>, <43>, > > > + <44>, <52>, <53>, > > > + <54>, <64>, > > > + <65>, <69>, <72>, > > > + <76>, <77>, <78>, > > > + <96>, <97>, <98>, > > > + <112>, <113>, > > > + <114>, <115>, <116>, > > > + <128>, <135>; > > > + clocks = <&ahb1>, <&ahb1>, <&ahb1>, > > > + <&ahb1>, <&ahb1>, <&ahb1>, > > > + <&ahb1>, <&ahb2>, <&ahb1>, > > > + <&ahb1>, <&ahb1>, > > > + <&ahb1>, <&ahb1>, > > > + <&ahb1>, <&ahb1>, > > > + <&ahb1>, <&ahb1>, > > > + <&ahb1>, <&ahb2>, > > > + <&ahb2>, <&ahb2>, <&ahb1>, > > > + <&ahb1>, <&ahb1>, <&ahb1>, > > > + <&ahb1>, <&ahb1>, <&ahb1>, > > > + <&ahb1>, <&ahb1>, <&ahb1>, > > > + <&ahb1>, <&apb1>, > > > + <&apb1>, <&apb1>, <&apb1>, > > > + <&apb1>, <&apb1>, <&apb1>, > > > + <&apb2>, <&apb2>, <&apb2>, > > > + <&apb2>, <&apb2>, > > > + <&apb2>, <&apb2>, <&apb2>, > > > + <&ahb1>, <&ahb1>; > > > > This is not really what I had in mind... > > > > This IP has 2 parents, and only two parents. The mapping between the > > IPs should be done in the driver itself, not in the DT where it is > > very error prone and barely readable. > > > > And note that I never have expected you to use clk-simple-gates > > either. This is a complicated clock, unlike the other we've seen so > > far, it definitely deserves a driver of its own. > > It seems that Allwinner puts the gate definitions anywhere in the array > of registers, so, I think that the H3 scheme will not be the last > complicated one, Maybe, but that's the first one. It doesn't prevent us from reusing the driver later if it happens. > and if the parent clocks are in the code instead of in the DT, we > will have more and more code to develop. I never asked that either. > An other way to describe the gates would be to add containers per parent > (with still a small patch in the clk-simple-gates): > > bus_gates: clk@01c20060 { > #clock-cells = <1>; > compatible = "allwinner,sun8i-h3-bus-gates-clk"; > reg = <0x01c20060 0x14>; > ahb1_gates { > clocks = <&ahb1>; > clock-indices = <5>, <6>, <8>, > <9>, <10>, <13>, > <14>, <18>, > <19>, <20>, > ...; > }; > clock-output-names = "ahb1_ce", "ahb1_dma", "ahb1_mmc0", > "ahb1_mmc1", "ahb1_mmc2", "ahb1_nand", > "ahb1_sdram", "ahb1_ts", > "ahb1_hstimer", "ahb1_spi0", > ...; > }; > ahb2_gates { > clocks = <&ahb2>; > clock-indices = <17>, <29>, > <30>, <31>, <32>, > ...; > clock-output-names = "ahb2_gmac", "ahb2_ohic1", > "ahb2_ohic2", "ahb2_ohic3", "ahb1_ve", > ...; > }; > apb1_gates { > ... > }; > apb2_gates { > ... > }; > }; Or simply bus_gates { clocks = <&ahb1>, <&ahb2>; clock-indices = <5>, <6>, <8>, ... clock-output-names = "bus_ce", "bus_dma", "bus_mmc0" }; Maxime
Hi, On 10/23/2015 08:14 PM, Maxime Ripard wrote: > On Wed, Oct 21, 2015 at 06:20:27PM +0200, Jens Kuske wrote: >> + bus_gates: clk@01c20060 { >> + #clock-cells = <1>; >> + compatible = "allwinner,sun8i-h3-bus-gates-clk"; >> + reg = <0x01c20060 0x14>; >> + clock-indices = <5>, <6>, <8>, >> + <9>, <10>, <13>, >> + <14>, <17>, <18>, >> + <19>, <20>, >> + <21>, <23>, >> + <24>, <25>, >> + <26>, <27>, >> + <28>, <29>, >> + <30>, <31>, <32>, >> + <35>, <36>, <37>, >> + <40>, <41>, <43>, >> + <44>, <52>, <53>, >> + <54>, <64>, >> + <65>, <69>, <72>, >> + <76>, <77>, <78>, >> + <96>, <97>, <98>, >> + <112>, <113>, >> + <114>, <115>, <116>, >> + <128>, <135>; >> + clocks = <&ahb1>, <&ahb1>, <&ahb1>, >> + <&ahb1>, <&ahb1>, <&ahb1>, >> + <&ahb1>, <&ahb2>, <&ahb1>, >> + <&ahb1>, <&ahb1>, >> + <&ahb1>, <&ahb1>, >> + <&ahb1>, <&ahb1>, >> + <&ahb1>, <&ahb1>, >> + <&ahb1>, <&ahb2>, >> + <&ahb2>, <&ahb2>, <&ahb1>, >> + <&ahb1>, <&ahb1>, <&ahb1>, >> + <&ahb1>, <&ahb1>, <&ahb1>, >> + <&ahb1>, <&ahb1>, <&ahb1>, >> + <&ahb1>, <&apb1>, >> + <&apb1>, <&apb1>, <&apb1>, >> + <&apb1>, <&apb1>, <&apb1>, >> + <&apb2>, <&apb2>, <&apb2>, >> + <&apb2>, <&apb2>, >> + <&apb2>, <&apb2>, <&apb2>, >> + <&ahb1>, <&ahb1>; > > This is not really what I had in mind... I came to the same solution independently, I took my inspiration from the rockchip clocks driver which is dealing with this problem in the same way, so there is precedent for doing things this way, and this does give us lot of flexibility. Given that I expect other new allwinnner SoCs to have the same problem I believe it is good to have that flexibility. > This IP has 2 parents, and only two parents. Nope it has 4: apb1, apb2, ahb1 and ahb2. > The mapping between the > IPs should be done in the driver itself, not in the DT where it is > very error prone and barely readable. It is just as error prone and barely readable in C-code, see Jens original patchset, he did an array of clock indices there (range 0-3 with an index into the parent clocks array), which is arguably even more unreadable since there is an extra level of indirection here. The problem with the unreadability simply comes from allwinner's decision to no longer have a gates register per bus but instead shove everything in a single bit-array in random order, there is nothing we can do to fix that. Also the argument "this belongs in the driver not in the DT" is a bit inconsistent with the moving of the mask of valid gates from the driver into the clock-indices in devicetree. The way things are done here actually are doing pretty much the same thing, putting info which could be derived from the compatible string into devicetree. Last as said already there is precedent for doing things this way in the rockchip driver, and given that 2 people have come up with this approach independently of of each other this clearly seems to be the most straight-foward / logical way to deal with this. > And note that I never have expected you to use clk-simple-gates > either. This is a complicated clock No it is not complicated, have you looked at the changes to the simple-clk-gates driver which Jean Francois Moine suggested? Those 5 extra lines (4 new lines) are all that is needed when going with the approach of listing a parent per gate. This is actually still a quite simple clock, we only need to find a way to specify a parent per gate, preferably via DT since this gives us greater flexibility which will be quite useful when adding support for other SoCs. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 24 Oct 2015 09:13:28 +0200 Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Or simply > > bus_gates { > clocks = <&ahb1>, <&ahb2>; > clock-indices = <5>, <6>, <8>, ... > clock-output-names = "bus_ce", "bus_dma", "bus_mmc0" > }; I don't understand: the apb1, apb2, ahb1 and ahb2 clocks may be programmed independently to different frequencies and you have to know which of them is the parent of each leaf clock. So, either you hard-code the parents as Jens did in a first proposal, or you define the full list of parents in the DT as in the last proposal, or you use a container per parent in the DT as I proposed. There could be an other solution using the output clock name to define the parent clock: bus_gates { clocks = <&ahb1>, <&ahb2>, <&apb1>, <&apb2>; clock-indices = <5>, <6>, <8>, ... clock-output-names = "ahb1_ce", "ahb1_dma", "ahb1_mmc0" }; with the documentation: "the clocks MUST be defined in order: ahb1, ahb2, apb1, apb2." and the code if (strncmp(clock_name, "ahb1", 4) == 0) clk_parent = of_clk_get_parent_name(node, 0); else if (..) but it seems a bit hacky.
On Sat, Oct 24, 2015 at 10:39:49AM +0200, Hans de Goede wrote: > Hi, > > On 10/23/2015 08:14 PM, Maxime Ripard wrote: > >On Wed, Oct 21, 2015 at 06:20:27PM +0200, Jens Kuske wrote: > >>+ bus_gates: clk@01c20060 { > >>+ #clock-cells = <1>; > >>+ compatible = "allwinner,sun8i-h3-bus-gates-clk"; > >>+ reg = <0x01c20060 0x14>; > >>+ clock-indices = <5>, <6>, <8>, > >>+ <9>, <10>, <13>, > >>+ <14>, <17>, <18>, > >>+ <19>, <20>, > >>+ <21>, <23>, > >>+ <24>, <25>, > >>+ <26>, <27>, > >>+ <28>, <29>, > >>+ <30>, <31>, <32>, > >>+ <35>, <36>, <37>, > >>+ <40>, <41>, <43>, > >>+ <44>, <52>, <53>, > >>+ <54>, <64>, > >>+ <65>, <69>, <72>, > >>+ <76>, <77>, <78>, > >>+ <96>, <97>, <98>, > >>+ <112>, <113>, > >>+ <114>, <115>, <116>, > >>+ <128>, <135>; > >>+ clocks = <&ahb1>, <&ahb1>, <&ahb1>, > >>+ <&ahb1>, <&ahb1>, <&ahb1>, > >>+ <&ahb1>, <&ahb2>, <&ahb1>, > >>+ <&ahb1>, <&ahb1>, > >>+ <&ahb1>, <&ahb1>, > >>+ <&ahb1>, <&ahb1>, > >>+ <&ahb1>, <&ahb1>, > >>+ <&ahb1>, <&ahb2>, > >>+ <&ahb2>, <&ahb2>, <&ahb1>, > >>+ <&ahb1>, <&ahb1>, <&ahb1>, > >>+ <&ahb1>, <&ahb1>, <&ahb1>, > >>+ <&ahb1>, <&ahb1>, <&ahb1>, > >>+ <&ahb1>, <&apb1>, > >>+ <&apb1>, <&apb1>, <&apb1>, > >>+ <&apb1>, <&apb1>, <&apb1>, > >>+ <&apb2>, <&apb2>, <&apb2>, > >>+ <&apb2>, <&apb2>, > >>+ <&apb2>, <&apb2>, <&apb2>, > >>+ <&ahb1>, <&ahb1>; > > > >This is not really what I had in mind... > > I came to the same solution independently, I took my inspiration from > the rockchip clocks driver which is dealing with this problem in the > same way, so there is precedent for doing things this way, and this > does give us lot of flexibility. Given that I expect other new allwinnner > SoCs to have the same problem I believe it is good to have that > flexibility. The rockchip clocks driver are very different from our own regarding the DT bindings. Being consistent within our own platform seems to bring much more value than getting bits and pieces here and there when it's convenient. Plus, you actually forgot in your argument to mention that this binding was documented as deprecated, and not used anywhere since 3.17... So apparently, someone tried that, and finally changed its mind. Again, this is clearly a workaround, with no way to easily identify if a given clock has the right parent afterwards. We can't review it properly, and it's going to be a pain to fix after the facts. Having some association masks in the driver itself, using the BIT macro, will be much easier to maintain in the long run. > >This IP has 2 parents, and only two parents. > > Nope it has 4: apb1, apb2, ahb1 and ahb2. The point still remains though. > >The mapping between the IPs should be done in the driver itself, > >not in the DT where it is very error prone and barely readable. > > It is just as error prone and barely readable in C-code, see Jens original > patchset, he did an array of clock indices there (range 0-3 with an index > into the parent clocks array), which is arguably even more unreadable since > there is an extra level of indirection here. I agree, and it's why I suggested another approach at the time. > The problem with the unreadability simply comes from allwinner's decision > to no longer have a gates register per bus but instead shove everything > in a single bit-array in random order, there is nothing we can do to fix > that. Indeed. Except one solution is easier to maintain than the other. > Also the argument "this belongs in the driver not in the DT" is a > bit inconsistent with the moving of the mask of valid gates from the > driver into the clock-indices in devicetree. Except I never used that argument. > The way things are done here actually are doing pretty much the same > thing, putting info which could be derived from the compatible > string into devicetree. > > Last as said already there is precedent for doing things this way > in the rockchip driver, and given that 2 people have come up > with this approach independently of of each other this clearly > seems to be the most straight-foward / logical way to deal with > this. > > >And note that I never have expected you to use clk-simple-gates > >either. This is a complicated clock > > No it is not complicated, have you looked at the changes to the > simple-clk-gates driver which Jean Francois Moine suggested? > > Those 5 extra lines (4 new lines) are all that is needed when > going with the approach of listing a parent per gate. This is > actually still a quite simple clock, we only need to find a way > to specify a parent per gate, preferably via DT since this gives > us greater flexibility which will be quite useful when adding > support for other SoCs. The problem boils down to this: so far, we've used one DT node per clock controller (and we won't change that, sorry). The clocks property is defined as "List of phandle and clock specifier pairs, one pair for each clock input to the device.". There's only 4 clock inputs. Really. What this clock controller controls is whether one of these four input will be output, and that's pretty much it. The routing between the input and output pins is internal to the controller, just like it should be internal to the driver. Maxime
On Sat, Oct 24, 2015 at 10:47:49AM +0200, Jean-Francois Moine wrote: > On Sat, 24 Oct 2015 09:13:28 +0200 > Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > > > Or simply > > > > bus_gates { > > clocks = <&ahb1>, <&ahb2>; > > clock-indices = <5>, <6>, <8>, ... > > clock-output-names = "bus_ce", "bus_dma", "bus_mmc0" > > }; > > I don't understand: the apb1, apb2, ahb1 and ahb2 clocks may be > programmed independently to different frequencies I don't understand why you're talking about frequencies here. > and you have to know which of them is the parent of each leaf clock. Indeed, but that's also doable here. Just not in the DT. > So, either you hard-code the parents as Jens did in a first proposal, > or you define the full list of parents in the DT as in the last > proposal, or you use a container per parent in the DT as I proposed. > > There could be an other solution using the output clock name to define > the parent clock: > > bus_gates { > clocks = <&ahb1>, <&ahb2>, <&apb1>, <&apb2>; > clock-indices = <5>, <6>, <8>, ... > clock-output-names = "ahb1_ce", "ahb1_dma", "ahb1_mmc0" > }; > > with the documentation: > > "the clocks MUST be defined in order: ahb1, ahb2, apb1, apb2." > > and the code > > if (strncmp(clock_name, "ahb1", 4) == 0) > clk_parent = of_clk_get_parent_name(node, 0); > else if (..) > > but it seems a bit hacky. It's exactly what I suggested, without the string comparison, but relying on the ID instead. Maxime
Hi, On 26-10-15 22:06, Maxime Ripard wrote: > On Sat, Oct 24, 2015 at 10:47:49AM +0200, Jean-Francois Moine wrote: >> On Sat, 24 Oct 2015 09:13:28 +0200 >> Maxime Ripard <maxime.ripard@free-electrons.com> wrote: >> >>> Or simply >>> >>> bus_gates { >>> clocks = <&ahb1>, <&ahb2>; >>> clock-indices = <5>, <6>, <8>, ... >>> clock-output-names = "bus_ce", "bus_dma", "bus_mmc0" >>> }; >> >> I don't understand: the apb1, apb2, ahb1 and ahb2 clocks may be >> programmed independently to different frequencies > > I don't understand why you're talking about frequencies here. > >> and you have to know which of them is the parent of each leaf clock. > > Indeed, but that's also doable here. Just not in the DT. > >> So, either you hard-code the parents as Jens did in a first proposal, >> or you define the full list of parents in the DT as in the last >> proposal, or you use a container per parent in the DT as I proposed. >> >> There could be an other solution using the output clock name to define >> the parent clock: >> >> bus_gates { >> clocks = <&ahb1>, <&ahb2>, <&apb1>, <&apb2>; >> clock-indices = <5>, <6>, <8>, ... >> clock-output-names = "ahb1_ce", "ahb1_dma", "ahb1_mmc0" >> }; >> >> with the documentation: >> >> "the clocks MUST be defined in order: ahb1, ahb2, apb1, apb2." >> >> and the code >> >> if (strncmp(clock_name, "ahb1", 4) == 0) >> clk_parent = of_clk_get_parent_name(node, 0); >> else if (..) >> >> but it seems a bit hacky. > > It's exactly what I suggested, without the string comparison, but > relying on the ID instead. I'm not following you here, what do you mean with "the ID" ? Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/10/15 09:58, Maxime Ripard wrote: > On Wed, Oct 21, 2015 at 06:20:26PM +0200, Jens Kuske wrote: >> Adding a new compatible allows us to define SoC specific behaviour >> if necessary, for example forcing a particular device out of reset >> even if no driver is actually using it. >> >> Signed-off-by: Jens Kuske <jenskuske@gmail.com> > > Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > Thanks! > Maxime > Hi, I've send a new version of this patch, please don't apply this version. Jens -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/reset/allwinner,sunxi-clock-reset.txt b/Documentation/devicetree/bindings/reset/allwinner,sunxi-clock-reset.txt index c8f7757..e11f023 100644 --- a/Documentation/devicetree/bindings/reset/allwinner,sunxi-clock-reset.txt +++ b/Documentation/devicetree/bindings/reset/allwinner,sunxi-clock-reset.txt @@ -8,6 +8,7 @@ Required properties: - compatible: Should be one of the following: "allwinner,sun6i-a31-ahb1-reset" "allwinner,sun6i-a31-clock-reset" + "allwinner,sun8i-h3-bus-reset" - reg: should be register base and length as documented in the datasheet - #reset-cells: 1, see below diff --git a/drivers/reset/reset-sunxi.c b/drivers/reset/reset-sunxi.c index 3d95c87..6f12b5c 100644 --- a/drivers/reset/reset-sunxi.c +++ b/drivers/reset/reset-sunxi.c @@ -124,6 +124,7 @@ err_alloc: */ static const struct of_device_id sunxi_early_reset_dt_ids[] __initdata = { { .compatible = "allwinner,sun6i-a31-ahb1-reset", }, + { .compatible = "allwinner,sun8i-h3-bus-reset", }, { /* sentinel */ }, };
Adding a new compatible allows us to define SoC specific behaviour if necessary, for example forcing a particular device out of reset even if no driver is actually using it. Signed-off-by: Jens Kuske <jenskuske@gmail.com> --- Documentation/devicetree/bindings/reset/allwinner,sunxi-clock-reset.txt | 1 + drivers/reset/reset-sunxi.c | 1 + 2 files changed, 2 insertions(+)