| Message ID | 1394719607-25018-2-git-send-email-antoine.tenart@free-electrons.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Mar 13, 2014 at 02:06:45PM +0000, Antoine Ténart wrote: > Adds initial support for the Marvell Armada 1500 pro (BG2Q) SoC (Berlin family). > The SoC has nodes for cpu, l2 cache controller, interrupt controllers, local > timer, apb timers and uarts for now. > > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > --- > arch/arm/boot/dts/berlin2q.dtsi | 167 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 167 insertions(+) > create mode 100644 arch/arm/boot/dts/berlin2q.dtsi > > diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi > new file mode 100644 > index 000000000000..1cb76031dfe6 > --- /dev/null > +++ b/arch/arm/boot/dts/berlin2q.dtsi > @@ -0,0 +1,167 @@ > +/* > + * Copyright (C) 2014 Antoine Ténart <antoine.tenart@free-electrons.com> > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include <dt-bindings/interrupt-controller/arm-gic.h> > + > +#include "skeleton.dtsi" > + > +/ { > + model = "Marvell Armada 1500 pro (BG2-Q) SoC"; > + compatible = "marvell,berlin2q", "marvell,berlin"; > + > + cpus { > + #address-cells = <1>; > + #size-cells = <0>; > + > + cpu@0 { > + compatible = "arm,cortex-a9"; > + device_type = "cpu"; > + next-level-cache = <&l2>; > + reg = <0>; > + }; > + > + cpu@1 { > + compatible = "arm,cortex-a9"; > + device_type = "cpu"; > + next-level-cache = <&l2>; > + reg = <1>; > + }; > + > + cpu@2 { > + compatible = "arm,cortex-a9"; > + device_type = "cpu"; > + next-level-cache = <&l2>; > + reg = <2>; > + }; > + > + cpu@3 { > + compatible = "arm,cortex-a9"; > + device_type = "cpu"; > + next-level-cache = <&l2>; > + reg = <3>; > + }; > + }; > + > + clocks { > + #address-cells = <0>; > + #size-cells = <0>; > + > + smclk: sysmgr-clock { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <25000000>; > + }; > + > + sysclk: system-clock { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <400000000>; > + }; > + }; I am very much not keen on having container nodes like this. A "clocks" container is non-standard, and not guarantee to probe. The fact these clocks probe currently is an artifact of the current organisation of Linux rather than any conscious decision. The #address-cells and #size-cells properties are useless, as no address space is defined. The node is not a simple-bus, has no ranges, and doesn't have a compatible string that implies an address space. Is there any reason you need to use a clocks container here and can't put these directly under the root? > + > + soc { > + compatible = "simple-bus"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + ranges = <0 0xf7000000 0x1000000>; > + interrupt-parent = <&gic>; > + > + l2: l2-cache-controller@ac0000 { > + compatible = "arm,pl310-cache"; > + reg = <0xac0000 0x1000>; > + cache-level = <2>; > + }; > + > + local-timer@ad0600 { > + compatible = "arm,cortex-a9-twd-timer"; > + reg = <0xad0600 0x20>; > + clocks = <&sysclk>; > + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>; > + status = "okay"; Given that nodes are assumed by default to be okay, this can be dropped. I think it only makes sense to have a status property in a common dtsi where the value is "disabled". Otherwise, this dtsi looks sane to me. Thanks, Mark.
On 03/13/2014 03:06 PM, Antoine Ténart wrote: > Adds initial support for the Marvell Armada 1500 pro (BG2Q) SoC (Berlin family). > The SoC has nodes for cpu, l2 cache controller, interrupt controllers, local > timer, apb timers and uarts for now. > > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com> > --- > arch/arm/boot/dts/berlin2q.dtsi | 167 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 167 insertions(+) > create mode 100644 arch/arm/boot/dts/berlin2q.dtsi > > diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi > new file mode 100644 > index 000000000000..1cb76031dfe6 > --- /dev/null > +++ b/arch/arm/boot/dts/berlin2q.dtsi [...] > + clocks { > + #address-cells = <0>; > + #size-cells = <0>; > + > + smclk: sysmgr-clock { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <25000000>; > + }; > + > + sysclk: system-clock { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <400000000>; > + }; > + }; > + > + soc { [...] > + local-timer@ad0600 { > + compatible = "arm,cortex-a9-twd-timer"; > + reg = <0xad0600 0x20>; > + clocks = <&sysclk>; If I understand Jisheng correctly, this should be cpuclk/3. When removing the clocks {} container above, please also take care of it. You can do cpuclk: cpu-clock { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <1200000000>; /* <- put correct freq here */ }; sysclk: system-clock { compatible = "fixed-factor-clock"; #clock-cells = <0>; clocks = <&cpuclk>; clock-multi = <1>; clock-div = <3>; }; Hopefully, we'll have proper clock drivers soon so we can just replace referenced "fixed-*" clocks. > + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>; > + status = "okay"; As Mark Rutland mentioned, get rid of status = "okay" in dtsi. > + }; > + [...] > + apb@e80000 { > + compatible = "simple-bus"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + ranges = <0 0xe80000 0x10000>; > + interrupt-parent = <&aic>; > + > + timer0: timer@2c00 { > + compatible = "snps,dw-apb-timer"; > + reg = <0x2c00 0x14>; > + interrupts = <8>; > + clock-freq = <100000000>; > + status = "okay"; > + }; > + > + timer1: timer@2c14 { > + compatible = "snps,dw-apb-timer"; > + reg = <0x2c14 0x14>; > + clock-freq = <100000000>; > + status = "disabled"; > + }; Please also add the remaining 6 apb timers. Sebastian
Sebastian, On 14/03/2014 10:31, Sebastian Hesselbarth wrote: > On 03/13/2014 03:06 PM, Antoine Ténart wrote: >> + clocks { >> + #address-cells = <0>; >> + #size-cells = <0>; >> + >> + smclk: sysmgr-clock { >> + compatible = "fixed-clock"; >> + #clock-cells = <0>; >> + clock-frequency = <25000000>; >> + }; >> + >> + sysclk: system-clock { >> + compatible = "fixed-clock"; >> + #clock-cells = <0>; >> + clock-frequency = <400000000>; >> + }; >> + }; >> + >> + soc { > [...] >> + local-timer@ad0600 { >> + compatible = "arm,cortex-a9-twd-timer"; >> + reg = <0xad0600 0x20>; >> + clocks = <&sysclk>; > > If I understand Jisheng correctly, this should be cpuclk/3. When > removing the clocks {} container above, please also take care of > it. > > You can do > > cpuclk: cpu-clock { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <1200000000>; /* <- put correct freq here */ > }; > > sysclk: system-clock { > compatible = "fixed-factor-clock"; > #clock-cells = <0>; > clocks = <&cpuclk>; > clock-multi = <1>; > clock-div = <3>; > }; > > Hopefully, we'll have proper clock drivers soon so we can just replace > referenced "fixed-*" clocks. Sure, this is why I only kept one fixed clock in this patch. I'll add the cpuclk here then. >> + timer0: timer@2c00 { >> + compatible = "snps,dw-apb-timer"; >> + reg = <0x2c00 0x14>; >> + interrupts = <8>; >> + clock-freq = <100000000>; >> + status = "okay"; >> + }; >> + >> + timer1: timer@2c14 { >> + compatible = "snps,dw-apb-timer"; >> + reg = <0x2c14 0x14>; >> + clock-freq = <100000000>; >> + status = "disabled"; >> + }; > > Please also add the remaining 6 apb timers. I don't have any information about the remaining 6 apb timers. I'll ask Jisheng. Antoine
On 03/14/2014 10:48 AM, Antoine Ténart wrote: > On 14/03/2014 10:31, Sebastian Hesselbarth wrote: >> On 03/13/2014 03:06 PM, Antoine Ténart wrote: >>> + clocks { >>> + #address-cells = <0>; >>> + #size-cells = <0>; >>> + >>> + smclk: sysmgr-clock { >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-frequency = <25000000>; >>> + }; >>> + >>> + sysclk: system-clock { >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-frequency = <400000000>; >>> + }; >>> + }; >>> + >>> + soc { >> [...] >>> + local-timer@ad0600 { >>> + compatible = "arm,cortex-a9-twd-timer"; >>> + reg = <0xad0600 0x20>; >>> + clocks = <&sysclk>; >> >> If I understand Jisheng correctly, this should be cpuclk/3. When >> removing the clocks {} container above, please also take care of >> it. >> >> You can do >> >> cpuclk: cpu-clock { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> clock-frequency = <1200000000>; /* <- put correct freq here */ >> }; >> >> sysclk: system-clock { >> compatible = "fixed-factor-clock"; >> #clock-cells = <0>; >> clocks = <&cpuclk>; >> clock-multi = <1>; >> clock-div = <3>; >> }; >> >> Hopefully, we'll have proper clock drivers soon so we can just replace >> referenced "fixed-*" clocks. > > Sure, this is why I only kept one fixed clock in this patch. > > I'll add the cpuclk here then. > >>> + timer0: timer@2c00 { >>> + compatible = "snps,dw-apb-timer"; >>> + reg = <0x2c00 0x14>; >>> + interrupts = <8>; >>> + clock-freq = <100000000>; >>> + status = "okay"; >>> + }; >>> + >>> + timer1: timer@2c14 { >>> + compatible = "snps,dw-apb-timer"; >>> + reg = <0x2c14 0x14>; >>> + clock-freq = <100000000>; >>> + status = "disabled"; >>> + }; >> >> Please also add the remaining 6 apb timers. > > I don't have any information about the remaining 6 apb timers. I'll ask > Jisheng. He replied to v1 of this patch with "We still have 8 apb timers" Sebastian
On 14/03/2014 at 10:31:55 +0100, Sebastian Hesselbarth wrote : > On 03/13/2014 03:06 PM, Antoine Ténart wrote: > >+ local-timer@ad0600 { > >+ compatible = "arm,cortex-a9-twd-timer"; > >+ reg = <0xad0600 0x20>; > >+ clocks = <&sysclk>; > > If I understand Jisheng correctly, this should be cpuclk/3. When > removing the clocks {} container above, please also take care of > it. > You understood correctly > You can do > > cpuclk: cpu-clock { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <1200000000>; /* <- put correct freq here */ > }; > > sysclk: system-clock { > compatible = "fixed-factor-clock"; > #clock-cells = <0>; > clocks = <&cpuclk>; > clock-multi = <1>; > clock-div = <3>; > }; > > Hopefully, we'll have proper clock drivers soon so we can just replace > referenced "fixed-*" clocks. > I'm working on that, do you have some code that you can share right now ?
On 03/14/2014 12:32 PM, Alexandre Belloni wrote: > On 14/03/2014 at 10:31:55 +0100, Sebastian Hesselbarth wrote : >> On 03/13/2014 03:06 PM, Antoine Ténart wrote: >>> + local-timer@ad0600 { >>> + compatible = "arm,cortex-a9-twd-timer"; >>> + reg = <0xad0600 0x20>; >>> + clocks = <&sysclk>; >> >> If I understand Jisheng correctly, this should be cpuclk/3. When >> removing the clocks {} container above, please also take care of >> it. >> > > You understood correctly > >> You can do >> >> cpuclk: cpu-clock { >> compatible = "fixed-clock"; >> #clock-cells = <0>; >> clock-frequency = <1200000000>; /* <- put correct freq here */ >> }; >> >> sysclk: system-clock { >> compatible = "fixed-factor-clock"; >> #clock-cells = <0>; >> clocks = <&cpuclk>; >> clock-multi = <1>; >> clock-div = <3>; >> }; >> >> Hopefully, we'll have proper clock drivers soon so we can just replace >> referenced "fixed-*" clocks. >> > > I'm working on that, do you have some code that you can share right now ? No, nothing specific. You can have a look at drivers/clk/mvebu/ and see how we deal with it on mvebu. For now, you can stick with bg2q and I'll see how we extend that drivers for bg2/bg2cd then. BTW: We really enjoy having a mvebu subfolder in drivers/clk as it makes things easier. If you prepare any clk drivers for berlin, I'd suggest to put them in drivers/clk/berlin from the beginning. Sebastian
diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi new file mode 100644 index 000000000000..1cb76031dfe6 --- /dev/null +++ b/arch/arm/boot/dts/berlin2q.dtsi @@ -0,0 +1,167 @@ +/* + * Copyright (C) 2014 Antoine Ténart <antoine.tenart@free-electrons.com> + * + * This file is licensed under the terms of the GNU General Public + * License version 2. This program is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#include <dt-bindings/interrupt-controller/arm-gic.h> + +#include "skeleton.dtsi" + +/ { + model = "Marvell Armada 1500 pro (BG2-Q) SoC"; + compatible = "marvell,berlin2q", "marvell,berlin"; + + cpus { + #address-cells = <1>; + #size-cells = <0>; + + cpu@0 { + compatible = "arm,cortex-a9"; + device_type = "cpu"; + next-level-cache = <&l2>; + reg = <0>; + }; + + cpu@1 { + compatible = "arm,cortex-a9"; + device_type = "cpu"; + next-level-cache = <&l2>; + reg = <1>; + }; + + cpu@2 { + compatible = "arm,cortex-a9"; + device_type = "cpu"; + next-level-cache = <&l2>; + reg = <2>; + }; + + cpu@3 { + compatible = "arm,cortex-a9"; + device_type = "cpu"; + next-level-cache = <&l2>; + reg = <3>; + }; + }; + + clocks { + #address-cells = <0>; + #size-cells = <0>; + + smclk: sysmgr-clock { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <25000000>; + }; + + sysclk: system-clock { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <400000000>; + }; + }; + + soc { + compatible = "simple-bus"; + #address-cells = <1>; + #size-cells = <1>; + + ranges = <0 0xf7000000 0x1000000>; + interrupt-parent = <&gic>; + + l2: l2-cache-controller@ac0000 { + compatible = "arm,pl310-cache"; + reg = <0xac0000 0x1000>; + cache-level = <2>; + }; + + local-timer@ad0600 { + compatible = "arm,cortex-a9-twd-timer"; + reg = <0xad0600 0x20>; + clocks = <&sysclk>; + interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL_HIGH>; + status = "okay"; + }; + + gic: interrupt-controller@ad1000 { + compatible = "arm,cortex-a9-gic"; + reg = <0xad1000 0x1000>, <0xad0100 0x100>; + interrupt-controller; + #interrupt-cells = <3>; + }; + + apb@e80000 { + compatible = "simple-bus"; + #address-cells = <1>; + #size-cells = <1>; + + ranges = <0 0xe80000 0x10000>; + interrupt-parent = <&aic>; + + timer0: timer@2c00 { + compatible = "snps,dw-apb-timer"; + reg = <0x2c00 0x14>; + interrupts = <8>; + clock-freq = <100000000>; + status = "okay"; + }; + + timer1: timer@2c14 { + compatible = "snps,dw-apb-timer"; + reg = <0x2c14 0x14>; + clock-freq = <100000000>; + status = "disabled"; + }; + + aic: interrupt-controller@3800 { + compatible = "snps,dw-apb-ictl"; + reg = <0x3800 0x30>; + interrupt-controller; + #interrupt-cells = <1>; + interrupt-parent = <&gic>; + interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>; + }; + }; + + apb@fc0000 { + compatible = "simple-bus"; + #address-cells = <1>; + #size-cells = <1>; + + ranges = <0 0xfc0000 0x10000>; + interrupt-parent = <&sic>; + + uart0: uart@9000 { + compatible = "snps,dw-apb-uart"; + reg = <0x9000 0x100>; + interrupt-parent = <&sic>; + interrupts = <8>; + clock-frequency = <25000000>; + reg-shift = <2>; + status = "disabled"; + }; + + uart1: uart@a000 { + compatible = "snps,dw-apb-uart"; + reg = <0xa000 0x100>; + interrupt-parent = <&sic>; + interrupts = <9>; + clock-frequency = <25000000>; + reg-shift = <2>; + status = "disabled"; + }; + + sic: interrupt-controller@e000 { + compatible = "snps,dw-apb-ictl"; + reg = <0xe000 0x30>; + interrupt-controller; + #interrupt-cells = <1>; + interrupt-parent = <&gic>; + interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>; + }; + }; + }; +};