| Message ID | 560EAA7C.3070302@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Mason <slash.tmp@free.fr> writes: > diff --git a/arch/arm/boot/dts/tango4.dtsi b/arch/arm/boot/dts/tango4.dtsi > new file mode 100644 > index 000000000000..7336fcc3ac1d > --- /dev/null > +++ b/arch/arm/boot/dts/tango4.dtsi > @@ -0,0 +1,117 @@ > +#include <dt-bindings/interrupt-controller/irq.h> > + > +/ { > + compatible = "sigma,tango4-soc"; > + > + #address-cells = <1>; > + #size-cells = <1>; > + > + clocks { > + ranges; > + #address-cells = <1>; > + #size-cells = <1>; > + > + xtal: xtal { > + compatible = "fixed-clock"; > + clock-frequency = <27000000>; > + #clock-cells = <0>; > + }; > + > + sysclk: sysclk { > + compatible = "fixed-clock"; > + clock-frequency = <396000000>; > + #clock-cells = <0>; > + }; > + > + cpuclk: cpuclk { > + compatible = "fixed-clock"; > + clock-frequency = <999000000>; > + #clock-cells = <0>; > + }; > + > + periphclk: periphclk { > + compatible = "fixed-factor-clock"; > + clocks = <&cpuclk>; > + clock-mult = <1>; > + clock-div = <2>; > + #clock-cells = <0>; > + }; > + }; Once more with feeling, why don't you want to use the fine clock driver I wrote? > + gic: gic@20001000 { > + compatible = "arm,cortex-a9-gic"; > + interrupt-controller; > + #interrupt-cells = <3>; > + reg = <0x20001000 0x1000>, > + <0x20000100 0x0100>; > + }; > + > + twd-timer@20000600 { > + compatible = "arm,cortex-a9-twd-timer"; > + reg = <0x20000600 0x10>; > + interrupts = <1 13 0xf01>; > + interrupt-parent = <&gic>; > + clocks = <&periphclk>; > + twd_never_stops; > + }; > + > + soc { > + compatible = "simple-bus"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + xtal_in_cnt { > + compatible = "sigma,xtal_in_cnt"; > + reg = <0x10048 0x4>; > + clocks = <&xtal>; > + }; > + > + uart0 { > + compatible = "ralink,rt2880-uart"; > + reg = <0x10700 0x100>; > + clock-frequency = <7372800>; > + reg-shift = <2>; > +/* fifo-size = <16>; BROKEN */ Either fix whatever is broken or drop that line. > + }; > + > + eth0: eth0 { > + compatible = "sigma,smp8640-emac"; > + reg = <0x26000 0x800>; > + interrupts = <38 4>; > + interrupt-parent = <&irq0>; > + mac-address = [ 00 16 e8 02 08 42 ]; mac-address should not be hardcoded here or anywhere else. > + clocks = <&sysclk>; > + }; > + > + intc: intc@e000 { > + compatible = "sigma,tango-intc"; Why do you insist on using other names than the ones I've been using for months? Just want to leave your own mark on the code? > diff --git a/arch/arm/mach-tangox/Kconfig b/arch/arm/mach-tangox/Kconfig > new file mode 100644 > index 000000000000..152cdd487056 > --- /dev/null > +++ b/arch/arm/mach-tangox/Kconfig > @@ -0,0 +1,12 @@ > +# Tango3 was based on MIPS 74kf. Tango4 is based on ARM Cortex A9 MPCore. This comment isn't relevant to the contents of the file.
On 02/10/2015 18:10, Måns Rullgård wrote: > Mason writes: > >> diff --git a/arch/arm/boot/dts/tango4.dtsi b/arch/arm/boot/dts/tango4.dtsi >> new file mode 100644 >> index 000000000000..7336fcc3ac1d >> --- /dev/null >> +++ b/arch/arm/boot/dts/tango4.dtsi > > Once more with feeling, why don't you want to use the fine clock driver > I wrote? I'll send you my clock tree driver next week. Then we can discuss. >> + uart0 { >> + compatible = "ralink,rt2880-uart"; >> + reg = <0x10700 0x100>; >> + clock-frequency = <7372800>; >> + reg-shift = <2>; >> +/* fifo-size = <16>; BROKEN */ > > Either fix whatever is broken or drop that line. I can't leave TODO reminders in the platform Kconfig? Even as comments? >> + }; >> + >> + eth0: eth0 { >> + compatible = "sigma,smp8640-emac"; >> + reg = <0x26000 0x800>; >> + interrupts = <38 4>; >> + interrupt-parent = <&irq0>; >> + mac-address = [ 00 16 e8 02 08 42 ]; > > mac-address should not be hardcoded here or anywhere else. Sorry. I missed that in my clean up. >> + clocks = <&sysclk>; >> + }; >> + >> + intc: intc@e000 { >> + compatible = "sigma,tango-intc"; > > Why do you insist on using other names than the ones I've been using for > months? Just want to leave your own mark on the code? You're using "sigma,smp8640-intc". The SMP8640 is a Tango3 (MIPS-based) platform. It makes no sense to have references to Tango3 in tango4.dtsi Aside from the CPU difference, Tango3 and Tango4 have a lot in common though. Which reminds me that I forgot to change "sigma,smp8640-emac" I'll send an updated patch. Regards.
Mason <slash.tmp@free.fr> writes: >>> + uart0 { >>> + compatible = "ralink,rt2880-uart"; >>> + reg = <0x10700 0x100>; >>> + clock-frequency = <7372800>; >>> + reg-shift = <2>; >>> +/* fifo-size = <16>; BROKEN */ >> >> Either fix whatever is broken or drop that line. > > I can't leave TODO reminders in the platform Kconfig? > Even as comments? Never mind, I've sent a patch fixing the problem. >>> + intc: intc@e000 { >>> + compatible = "sigma,tango-intc"; >> >> Why do you insist on using other names than the ones I've been using for >> months? Just want to leave your own mark on the code? > > You're using "sigma,smp8640-intc". > The SMP8640 is a Tango3 (MIPS-based) platform. > It makes no sense to have references to Tango3 in tango4.dtsi > Aside from the CPU difference, Tango3 and Tango4 have a lot in common though. It's commonplace to refer to peripherals by the earliest (supported) chip using them. This avoids naming conflicts if a future chip uses a different component. Some bits are incompatible even between different devices in the tango3 family.
On Fri, Oct 02, 2015 at 06:33:48PM +0200, Mason wrote: > On 02/10/2015 18:10, Måns Rullgård wrote: > > > Mason writes: > > > >> + intc: intc@e000 { > >> + compatible = "sigma,tango-intc"; > > > > Why do you insist on using other names than the ones I've been using for > > months? Just want to leave your own mark on the code? > > You're using "sigma,smp8640-intc". > The SMP8640 is a Tango3 (MIPS-based) platform. If it's the same device, then there's nothing wrong with re-using the compatible. The compatible property actually accepts multiple values, so you can do: compatible = "sigma,tango4-intc", "sigma,smp8640-intc"; See the ePAPR spec - I'll include the relevent bit: Property: compatible Value type: <stringlist> Description: The compatible property value consists of one or more strings that define the specific programming model for the device. This list of strings should be used by a client program for device driver selection. The property value consists of a concatenated list of null terminated strings, from most specific to most general. They allow a device to express its compatibility with a family of similar devices, potentially allowing a single device driver to match against several devices. ... Example: compatible = "fsl,mpc8641-uart", "ns16550"; In this example, an operating system would first try to locate a device driver that supported fsl,mpc8641-uart. If a driver was not found, it would then try to locate a driver that supported the more general ns16550 device type. Note also that vendor prefixes should be listed in Documentation/devicetree/bindings/vendor-prefixes.txt. If it's not there, you need to propose a separate patch (to the devicetree mailing list) to add it, which must be done with their agreement. Right now, the use of "sigma" as a prefix is entirely non-standard and not acceptable in DT files until this is done.
On 02/10/2015 18:55, Måns Rullgård wrote: > Mason writes: > >>>> + uart0 { >>>> + compatible = "ralink,rt2880-uart"; >>>> + reg = <0x10700 0x100>; >>>> + clock-frequency = <7372800>; >>>> + reg-shift = <2>; >>>> +/* fifo-size = <16>; BROKEN */ >>> >>> Either fix whatever is broken or drop that line. >> >> I can't leave TODO reminders in the platform Kconfig [and DT files]? > > Never mind, I've sent a patch fixing the problem. That's great news. Although back-porting to 3.14 is looking more and more time-consuming. (But that's my problem.) The question of comments in Kconfig and DT files still stands. (No need to state your position again.) >>>> + intc: intc@e000 { >>>> + compatible = "sigma,tango-intc"; >>> >>> Why do you insist on using other names than the ones I've been using for >>> months? Just want to leave your own mark on the code? >> >> You're using "sigma,smp8640-intc". >> The SMP8640 is a Tango3 (MIPS-based) platform. >> It makes no sense to have references to Tango3 in tango4.dtsi >> Aside from the CPU difference, Tango3 and Tango4 have a lot in common though. > > It's commonplace to refer to peripherals by the earliest (supported) > chip using them. This avoids naming conflicts if a future chip uses a > different component. Some bits are incompatible even between different > devices in the tango3 family. I'm confused. I like the way ARM did it in smp-twd.c CLOCKSOURCE_OF_DECLARE(arm_twd_a9, "arm,cortex-a9-twd-timer", twd_local_timer_of_register); CLOCKSOURCE_OF_DECLARE(arm_twd_a5, "arm,cortex-a5-twd-timer", twd_local_timer_of_register); CLOCKSOURCE_OF_DECLARE(arm_twd_11mp, "arm,arm11mp-twd-timer", twd_local_timer_of_register); They have several definitions for the different supported platforms. They have several versions of the GIC: IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init); IRQCHIP_DECLARE(arm11mp_gic, "arm,arm11mp-gic", gic_of_init); IRQCHIP_DECLARE(arm1176jzf_dc_gic, "arm,arm1176jzf-devchip-gic", gic_of_init); IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init); IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init); IRQCHIP_DECLARE(cortex_a7_gic, "arm,cortex-a7-gic", gic_of_init); IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); IRQCHIP_DECLARE(gic_v3, "arm,gic-v3", gic_of_init); For example, I know the interrupt controller was changed for Tango5. So I was planning on using: "sigma,tango-intc" for Tango3/Tango4 "sigma,tango-intc-v2" for Tango5 Would you change your code to accommodate this nomenclature? Regards.
On 02/10/2015 19:13, Russell King - ARM Linux wrote: > On Fri, Oct 02, 2015 at 06:33:48PM +0200, Mason wrote: >> On 02/10/2015 18:10, Måns Rullgård wrote: >> >>> Mason writes: >>> >>>> + intc: intc@e000 { >>>> + compatible = "sigma,tango-intc"; >>> >>> Why do you insist on using other names than the ones I've been using for >>> months? Just want to leave your own mark on the code? >> >> You're using "sigma,smp8640-intc". >> The SMP8640 is a Tango3 (MIPS-based) platform. > > If it's the same device, then there's nothing wrong with re-using the > compatible. The compatible property actually accepts multiple values, > so you can do: > > compatible = "sigma,tango4-intc", "sigma,smp8640-intc"; I have access to resources unavailable to Mans. Since I know the interrupt controller is the same on Tango2, Tango3 and Tango4, doesn't it make sense to use the string "sigma,tango-intc" given that the driver is not even upstream yet? (If worse comes to worst, I suppose I can always write my own driver from scratch; I just find it silly to duplicate work.) > See the ePAPR spec - I'll include the relevant bit: > > Property: compatible > Value type: <stringlist> > Description: The compatible property value consists of one or more strings > that define the specific programming model for the device. This list of > strings should be used by a client program for device driver selection. > The property value consists of a concatenated list of null terminated > strings, from most specific to most general. They allow a device to > express its compatibility with a family of similar devices, potentially > allowing a single device driver to match against several devices. ... > Example: compatible = "fsl,mpc8641-uart", "ns16550"; > In this example, an operating system would first try to locate a device > driver that supported fsl,mpc8641-uart. If a driver was not found, it > would then try to locate a driver that supported the more general > ns16550 device type. > > Note also that vendor prefixes should be listed in > Documentation/devicetree/bindings/vendor-prefixes.txt. If it's not there, > you need to propose a separate patch (to the devicetree mailing list) to > add it, which must be done with their agreement. Right now, the use of > "sigma" as a prefix is entirely non-standard and not acceptable in DT > files until this is done. As far as the upstreaming process is concerned, I speak for Sigma. Regards.
On Fri, Oct 02, 2015 at 08:09:39PM +0200, Mason wrote: > On 02/10/2015 19:13, Russell King - ARM Linux wrote: > > On Fri, Oct 02, 2015 at 06:33:48PM +0200, Mason wrote: > >> On 02/10/2015 18:10, Måns Rullgård wrote: > >> > >>> Mason writes: > >>> > >>>> + intc: intc@e000 { > >>>> + compatible = "sigma,tango-intc"; > >>> > >>> Why do you insist on using other names than the ones I've been using for > >>> months? Just want to leave your own mark on the code? > >> > >> You're using "sigma,smp8640-intc". > >> The SMP8640 is a Tango3 (MIPS-based) platform. > > > > If it's the same device, then there's nothing wrong with re-using the > > compatible. The compatible property actually accepts multiple values, > > so you can do: > > > > compatible = "sigma,tango4-intc", "sigma,smp8640-intc"; > > I have access to resources unavailable to Mans. Since I know the > interrupt controller is the same on Tango2, Tango3 and Tango4, > doesn't it make sense to use the string "sigma,tango-intc" > given that the driver is not even upstream yet? You could do: compatible = "sigma,tango4-intc", "sigma,tango-intc", "sigma,smp8640-intc"; The advantage of using the most specific first is that if you then find the hardware contains bugs, you can _just_ modify the device driver to match on the specific ID, and implement workarounds based on that. Older device trees will then pick up those same workarounds without needing to be modified. Remember, device trees are supposed to describe the hardware. > (If worse comes to worst, I suppose I can always write my own > driver from scratch; I just find it silly to duplicate work.) It's not about writing a separate driver. What the above says is that this device is first and foremost a "sigma,tango4-intc" device, but if we don't have such a driver, it can be driven by a driver for "sigma,tango-intc", but if we don't have one of those, then it can be driven by a "sigma,smp8640-intc" driver. Please read the quoted bit of the spec on this to get a proper understanding of how these compatible strings are supposed to work: > > See the ePAPR spec - I'll include the relevant bit: > > > > Property: compatible > > Value type: <stringlist> > > Description: The compatible property value consists of one or more strings > > that define the specific programming model for the device. This list of > > strings should be used by a client program for device driver selection. > > The property value consists of a concatenated list of null terminated > > strings, from most specific to most general. They allow a device to > > express its compatibility with a family of similar devices, potentially > > allowing a single device driver to match against several devices. ... > > Example: compatible = "fsl,mpc8641-uart", "ns16550"; > > In this example, an operating system would first try to locate a device > > driver that supported fsl,mpc8641-uart. If a driver was not found, it > > would then try to locate a driver that supported the more general > > ns16550 device type. > > > > Note also that vendor prefixes should be listed in > > Documentation/devicetree/bindings/vendor-prefixes.txt. If it's not there, > > you need to propose a separate patch (to the devicetree mailing list) to > > add it, which must be done with their agreement. Right now, the use of > > "sigma" as a prefix is entirely non-standard and not acceptable in DT > > files until this is done. > > As far as the upstreaming process is concerned, I speak for Sigma. It doesn't matter who you speak for. Your first patch should be to _only_ add the vendor ID to that file above, and to get it acked by the device tree maintainers. That makes it "official" in the eyes of the developers responsible for maintaining the sanity of device tree. However, that has an impact on the above: you should therefore have access to the folk who know the origins of the interrupt controller, and whether it is a derivative of "sigma,smp8640-intc" or something else. If "sigma,smp8640-intc" and "sigma,tango-intc" are jointly derived from a common ancestor, then you should not mention "sigma,smp8640-intc" at all.
On 02/10/2015 20:53, Russell King - ARM Linux wrote: > On Fri, Oct 02, 2015 at 08:09:39PM +0200, Mason wrote: >> On 02/10/2015 19:13, Russell King - ARM Linux wrote: >> >>> Note also that vendor prefixes should be listed in >>> Documentation/devicetree/bindings/vendor-prefixes.txt. If it's not there, >>> you need to propose a separate patch (to the devicetree mailing list) to >>> add it, which must be done with their agreement. Right now, the use of >>> "sigma" as a prefix is entirely non-standard and not acceptable in DT >>> files until this is done. >> >> As far as the upstreaming process is concerned, I speak for Sigma. > > It doesn't matter who you speak for. Your first patch should be to > _only_ add the vendor ID to that file above, and to get it acked by > the device tree maintainers. That makes it "official" in the eyes of > the developers responsible for maintaining the sanity of device tree. OK. Mans took care of that in "devicetree: add Sigma Designs vendor prefix" > However, that has an impact on the above: you should therefore have > access to the folk who know the origins of the interrupt controller, > and whether it is a derivative of "sigma,smp8640-intc" or something > else. If "sigma,smp8640-intc" and "sigma,tango-intc" are jointly > derived from a common ancestor, then you should not mention > "sigma,smp8640-intc" at all. I think there is some confusion surrounding Sigma's SoCs. Briefly, Sigma Designs has gone through 4 major revisions of its SoC architecture, Tango1 through Tango4. (Let's forget Tango1 and Tango2, as they have fallen into oblivion.) Within a major architecture, Sigma produces different designs, sometimes just blowing fuses to differentiate packages, sometimes actually adding hardware, or tweaking the design. These designs are given "SMP8xxx" names, typically SMP86xx for Tango3 (MIPS) and SMP87xx for Tango4 (ARM). For example, SMP8756 is an ARM-based design, with a single Cortex A9 core, while SMP8758 has two A9 cores. SMP8640 was just one Tango3 SoC out of several. It's not special in any way, as far as the interrupt controller is concerned. I'll have to check the docs, but I seem to remember it has remained unchanged throughout the Tango2-Tango4 period. (But it will change in Tango5.) This is why I insist on not committing to the smp8640-* nomenclature. Because SMP8640 is nothing special in the Tango family. Regards.
On Friday 02 October 2015 18:02:04 Mason wrote: > Add support for Tango4-based SoCs (SMP8756, SMP8758) > > Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> Please write a proper changelog text here that tells us more about the patch than the subject line. > --- a/arch/arm/boot/dts/Makefile > +++ b/arch/arm/boot/dts/Makefile > @@ -605,6 +605,8 @@ dtb-$(CONFIG_MACH_SUN8I) += \ > dtb-$(CONFIG_MACH_SUN9I) += \ > sun9i-a80-optimus.dtb \ > sun9i-a80-cubieboard4.dtb > +dtb-$(CONFIG_ARCH_TANGOX) += \ > + vantage-1172.dtb This file name should start with the name of the chip, e.g. 'tango4-vantage-1172.dtb', to make it easier to find. > diff --git a/arch/arm/boot/dts/tango4.dtsi b/arch/arm/boot/dts/tango4.dtsi > new file mode 100644 > index 000000000000..7336fcc3ac1d > --- /dev/null > +++ b/arch/arm/boot/dts/tango4.dtsi > @@ -0,0 +1,117 @@ > +#include <dt-bindings/interrupt-controller/irq.h> > + > +/ { > + compatible = "sigma,tango4-soc"; Move the root comaptible strings into the board specific file, and add the name of the machine as a more specific one. > + > + soc { > + compatible = "simple-bus"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + xtal_in_cnt { > + compatible = "sigma,xtal_in_cnt"; > + reg = <0x10048 0x4>; > + clocks = <&xtal>; > + }; > + > + uart0 { > + compatible = "ralink,rt2880-uart"; > + reg = <0x10700 0x100>; > + clock-frequency = <7372800>; > + reg-shift = <2>; > +/* fifo-size = <16>; BROKEN */ > + }; Please use standard node names here, the uart should be named 'serial@10700', the other names should be 'ethernet@26000', 'interrupt-controller@6e000' etc. If the SoC contains more than one UART, please list them all here, and mark them as 'status="disabled"' in the .dtsi file, then add the appropriate aliases and 'status="okay"' override for the ones that are actually used on that board. > + > + eth0: eth0 { > + compatible = "sigma,smp8640-emac"; > + reg = <0x26000 0x800>; > + interrupts = <38 4>; > + interrupt-parent = <&irq0>; > + mac-address = [ 00 16 e8 02 08 42 ]; > + clocks = <&sysclk>; > + }; > + > + intc: intc@e000 { > + compatible = "sigma,tango-intc"; > + reg = <0x6e000 0x1000>; > + interrupt-parent = <&gic>; > + interrupt-controller; > + #address-cells = <1>; > + #size-cells = <1>; > + > + irq0: irq0@000 { > + reg = <0x000 0x100>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupts = <0 2 4>; > + }; You are missing a ranges property that describes what address space these addresses are in. > + irq1: irq1@100 { > + reg = <0x100 0x100>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupts = <0 3 4>; > + }; > + > + irq2: irq2@300 { > + reg = <0x300 0x100>; > + interrupt-controller; > + #interrupt-cells = <2>; > + interrupts = <0 4 4>; > + }; > + }; > + }; > +}; > diff --git a/arch/arm/boot/dts/vantage-1172.dts b/arch/arm/boot/dts/vantage-1172.dts > new file mode 100644 > index 000000000000..56f6babe7093 > --- /dev/null > +++ b/arch/arm/boot/dts/vantage-1172.dts > @@ -0,0 +1,8 @@ > +/dts-v1/; > + > +#include "tango4.dtsi" > + > +ð0 { > + phy-connection-type = "rgmii"; > + max-speed = <1000>; > +}; You should normally have /chosen and /aliases nodes here as well. > diff --git a/arch/arm/mach-tangox/Kconfig b/arch/arm/mach-tangox/Kconfig > new file mode 100644 > index 000000000000..152cdd487056 > --- /dev/null > +++ b/arch/arm/mach-tangox/Kconfig > @@ -0,0 +1,12 @@ > +# Tango3 was based on MIPS 74kf. Tango4 is based on ARM Cortex A9 MPCore. > + > +config ARCH_TANGOX > + bool "Sigma Designs Tango4 (SMP87xx)" if ARCH_MULTI_V7 > + select ARCH_HAS_HOLES_MEMORYMODEL > + select ARM_ERRATA_754322 > + select ARM_ERRATA_764369 if SMP > + select ARM_GIC > + select GENERIC_IRQ_CHIP > + select HAVE_ARM_SCU > + select HAVE_ARM_TWD > + select SERIAL_8250_RT288X if SERIAL_8250 Do not 'select' the uart driver, that can just be part of the defconfig file. Arnd
On 02/10/2015 21:56, Arnd Bergmann wrote: > On Friday 02 October 2015 18:02:04 Mason wrote: >> Add support for Tango4-based SoCs (SMP8756, SMP8758) >> >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> > > Please write a proper changelog text here that tells us more about > the patch than the subject line. Sorry, I'm quite unimaginative. What kind of information do you consider required? >> --- a/arch/arm/boot/dts/Makefile >> +++ b/arch/arm/boot/dts/Makefile >> @@ -605,6 +605,8 @@ dtb-$(CONFIG_MACH_SUN8I) += \ >> dtb-$(CONFIG_MACH_SUN9I) += \ >> sun9i-a80-optimus.dtb \ >> sun9i-a80-cubieboard4.dtb >> +dtb-$(CONFIG_ARCH_TANGOX) += \ >> + vantage-1172.dtb > > This file name should start with the name of the chip, > e.g. 'tango4-vantage-1172.dtb', to make it easier to find. OK. (Why doesn't arm do it like mips, with per-manufacturer folders?) >> diff --git a/arch/arm/boot/dts/tango4.dtsi b/arch/arm/boot/dts/tango4.dtsi >> new file mode 100644 >> index 000000000000..7336fcc3ac1d >> --- /dev/null >> +++ b/arch/arm/boot/dts/tango4.dtsi >> @@ -0,0 +1,117 @@ >> +#include <dt-bindings/interrupt-controller/irq.h> >> + >> +/ { >> + compatible = "sigma,tango4-soc"; > > Move the root compatible strings into the board specific file, > and add the name of the machine as a more specific one. I don't understand this. >> + soc { >> + compatible = "simple-bus"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + >> + xtal_in_cnt { >> + compatible = "sigma,xtal_in_cnt"; >> + reg = <0x10048 0x4>; >> + clocks = <&xtal>; >> + }; >> + >> + uart0 { >> + compatible = "ralink,rt2880-uart"; >> + reg = <0x10700 0x100>; >> + clock-frequency = <7372800>; >> + reg-shift = <2>; >> +/* fifo-size = <16>; BROKEN */ >> + }; > > Please use standard node names here, the uart should be > named 'serial@10700', the other names should be > 'ethernet@26000', 'interrupt-controller@6e000' etc. Where are the standard node names documented? Can I still name labels freely? Or is there a naming convention for labels? Why is the base address duplicated? Can't the DTC figure out the address from the reg property? >> + eth0: eth0 { >> + compatible = "sigma,smp8640-emac"; >> + reg = <0x26000 0x800>; >> + interrupts = <38 4>; >> + interrupt-parent = <&irq0>; >> + mac-address = [ 00 16 e8 02 08 42 ]; >> + clocks = <&sysclk>; >> + }; >> + >> + intc: intc@e000 { >> + compatible = "sigma,tango-intc"; >> + reg = <0x6e000 0x1000>; >> + interrupt-parent = <&gic>; >> + interrupt-controller; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + >> + irq0: irq0@000 { >> + reg = <0x000 0x100>; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + interrupts = <0 2 4>; >> + }; > > You are missing a ranges property that describes what address > space these addresses are in. ranges; is not hierarchically inherited? >> + irq1: irq1@100 { >> + reg = <0x100 0x100>; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + interrupts = <0 3 4>; >> + }; >> + >> + irq2: irq2@300 { >> + reg = <0x300 0x100>; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + interrupts = <0 4 4>; >> + }; >> + }; >> + }; >> +}; >> diff --git a/arch/arm/boot/dts/vantage-1172.dts b/arch/arm/boot/dts/vantage-1172.dts >> new file mode 100644 >> index 000000000000..56f6babe7093 >> --- /dev/null >> +++ b/arch/arm/boot/dts/vantage-1172.dts >> @@ -0,0 +1,8 @@ >> +/dts-v1/; >> + >> +#include "tango4.dtsi" >> + >> +ð0 { >> + phy-connection-type = "rgmii"; >> + max-speed = <1000>; >> +}; > > You should normally have /chosen and /aliases nodes here as well. Sigh. I don't know what they are for. http://devicetree.org/Device_Tree_Usage#Special_Nodes So with aliases, I don't need to define labels in the .dtsi? "Typically the chosen node is left empty in .dts source files and populated at boot time." Should I put my current bootargs there? >> diff --git a/arch/arm/mach-tangox/Kconfig b/arch/arm/mach-tangox/Kconfig >> new file mode 100644 >> index 000000000000..152cdd487056 >> --- /dev/null >> +++ b/arch/arm/mach-tangox/Kconfig >> @@ -0,0 +1,12 @@ >> +# Tango3 was based on MIPS 74kf. Tango4 is based on ARM Cortex A9 MPCore. >> + >> +config ARCH_TANGOX >> + bool "Sigma Designs Tango4 (SMP87xx)" if ARCH_MULTI_V7 >> + select ARCH_HAS_HOLES_MEMORYMODEL >> + select ARM_ERRATA_754322 >> + select ARM_ERRATA_764369 if SMP >> + select ARM_GIC >> + select GENERIC_IRQ_CHIP >> + select HAVE_ARM_SCU >> + select HAVE_ARM_TWD >> + select SERIAL_8250_RT288X if SERIAL_8250 > > Do not 'select' the uart driver, that can just be part of the defconfig > file. Do you mean I should provide a defconfig with my patch? Picking SERIAL_8250 but not SERIAL_8250_RT288X makes the kernel panic. Doesn't that qualify for selecting it? (I don't understand the rationale behind most conventions in kernel dev.) Regards.
On Friday 02 October 2015 22:53:11 Mason wrote: > On 02/10/2015 21:56, Arnd Bergmann wrote: > > On Friday 02 October 2015 18:02:04 Mason wrote: > >> Add support for Tango4-based SoCs (SMP8756, SMP8758) > >> > >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> > > > > Please write a proper changelog text here that tells us more about > > the patch than the subject line. > > Sorry, I'm quite unimaginative. What kind of information do you > consider required? The line you have there is not needed, but it would be nice to have a link to the data sheet (if available) and some information about what SoCs they are. For most patches, you describe what the original code does wrong first, followed by how your patch addresses the situation, but for a new platform that is a bit different. > >> --- a/arch/arm/boot/dts/Makefile > >> +++ b/arch/arm/boot/dts/Makefile > >> @@ -605,6 +605,8 @@ dtb-$(CONFIG_MACH_SUN8I) += \ > >> dtb-$(CONFIG_MACH_SUN9I) += \ > >> sun9i-a80-optimus.dtb \ > >> sun9i-a80-cubieboard4.dtb > >> +dtb-$(CONFIG_ARCH_TANGOX) += \ > >> + vantage-1172.dtb > > > > This file name should start with the name of the chip, > > e.g. 'tango4-vantage-1172.dtb', to make it easier to find. > > OK. > > (Why doesn't arm do it like mips, with per-manufacturer folders?) Historic mistake on our side, and it's become too hard to fix now without breaking hundreds of patches that people are trying to get merged. > >> diff --git a/arch/arm/boot/dts/tango4.dtsi b/arch/arm/boot/dts/tango4.dtsi > >> new file mode 100644 > >> index 000000000000..7336fcc3ac1d > >> --- /dev/null > >> +++ b/arch/arm/boot/dts/tango4.dtsi > >> @@ -0,0 +1,117 @@ > >> +#include <dt-bindings/interrupt-controller/irq.h> > >> + > >> +/ { > >> + compatible = "sigma,tango4-soc"; > > > > Move the root compatible strings into the board specific file, > > and add the name of the machine as a more specific one. > > I don't understand this. The compatible list should list both generic and specific names if applicable. For an SoC based platform, that almost always translates into board name, soc name and sometimes soc family name. > >> + soc { > >> + compatible = "simple-bus"; > >> + #address-cells = <1>; > >> + #size-cells = <1>; > >> + ranges; > >> + > >> + xtal_in_cnt { > >> + compatible = "sigma,xtal_in_cnt"; > >> + reg = <0x10048 0x4>; > >> + clocks = <&xtal>; > >> + }; > >> + > >> + uart0 { > >> + compatible = "ralink,rt2880-uart"; > >> + reg = <0x10700 0x100>; > >> + clock-frequency = <7372800>; > >> + reg-shift = <2>; > >> +/* fifo-size = <16>; BROKEN */ > >> + }; > > > > Please use standard node names here, the uart should be > > named 'serial@10700', the other names should be > > 'ethernet@26000', 'interrupt-controller@6e000' etc. > > Where are the standard node names documented? ePAPR has a list, for others look at the existing dts files. > Can I still name labels freely? Labels can be freely assigned, they are only used as syntactic sugar to let you write phandle references in a more compact way, but names are what ends up in the dts and they should follow convention as much as possible. > Or is there a naming convention for labels? > > Why is the base address duplicated? Can't the DTC figure > out the address from the reg property? I don't know what exactly has led to this, I believe it was like this in original open firmware, but not always enforced, but we probably needed to represent nodes from real firmware in dtb files. > >> + eth0: eth0 { > >> + compatible = "sigma,smp8640-emac"; > >> + reg = <0x26000 0x800>; > >> + interrupts = <38 4>; > >> + interrupt-parent = <&irq0>; > >> + mac-address = [ 00 16 e8 02 08 42 ]; > >> + clocks = <&sysclk>; > >> + }; > >> + > >> + intc: intc@e000 { > >> + compatible = "sigma,tango-intc"; > >> + reg = <0x6e000 0x1000>; > >> + interrupt-parent = <&gic>; > >> + interrupt-controller; > >> + #address-cells = <1>; > >> + #size-cells = <1>; > >> + > >> + irq0: irq0@000 { > >> + reg = <0x000 0x100>; > >> + interrupt-controller; > >> + #interrupt-cells = <2>; > >> + interrupts = <0 2 4>; > >> + }; > > > > You are missing a ranges property that describes what address > > space these addresses are in. > > ranges; is not hierarchically inherited? 'ranges;' would be wrong here, as the interrupt controller is not a bus. If you have no ranges property, the bus is interpreted as having its own address space with no relation to the parent bus (e.g. an I2C bus uses addresses that are not memory mapped). Just list the addresses that are actually decoded by child devices here. > >> diff --git a/arch/arm/boot/dts/vantage-1172.dts b/arch/arm/boot/dts/vantage-1172.dts > >> new file mode 100644 > >> index 000000000000..56f6babe7093 > >> --- /dev/null > >> +++ b/arch/arm/boot/dts/vantage-1172.dts > >> @@ -0,0 +1,8 @@ > >> +/dts-v1/; > >> + > >> +#include "tango4.dtsi" > >> + > >> +ð0 { > >> + phy-connection-type = "rgmii"; > >> + max-speed = <1000>; > >> +}; > > > > You should normally have /chosen and /aliases nodes here as well. > > Sigh. I don't know what they are for. > http://devicetree.org/Device_Tree_Usage#Special_Nodes > > So with aliases, I don't need to define labels in the .dtsi? labels are always optional, you can write the aliases node using either labels or open-coded paths, like /aliases { serial0 = &uart_3; serial1 = "/soc/serial@10700"; }; > "Typically the chosen node is left empty in .dts source files and populated at boot time." > Should I put my current bootargs there? I would put only the stdout-path property in there, as long as you can boot with any bootargs. > >> diff --git a/arch/arm/mach-tangox/Kconfig b/arch/arm/mach-tangox/Kconfig > >> new file mode 100644 > >> index 000000000000..152cdd487056 > >> --- /dev/null > >> +++ b/arch/arm/mach-tangox/Kconfig > >> @@ -0,0 +1,12 @@ > >> +# Tango3 was based on MIPS 74kf. Tango4 is based on ARM Cortex A9 MPCore. > >> + > >> +config ARCH_TANGOX > >> + bool "Sigma Designs Tango4 (SMP87xx)" if ARCH_MULTI_V7 > >> + select ARCH_HAS_HOLES_MEMORYMODEL > >> + select ARM_ERRATA_754322 > >> + select ARM_ERRATA_764369 if SMP > >> + select ARM_GIC > >> + select GENERIC_IRQ_CHIP > >> + select HAVE_ARM_SCU > >> + select HAVE_ARM_TWD > >> + select SERIAL_8250_RT288X if SERIAL_8250 > > > > Do not 'select' the uart driver, that can just be part of the defconfig > > file. > > Do you mean I should provide a defconfig with my patch? > > Picking SERIAL_8250 but not SERIAL_8250_RT288X makes the kernel > panic. Doesn't that qualify for selecting it? (I don't understand > the rationale behind most conventions in kernel dev.) Add it to multi_v7_defconfig Arnd
On 02/10/2015 23:11, Arnd Bergmann wrote: > On Friday 02 October 2015 22:53:11 Mason wrote: >> On 02/10/2015 21:56, Arnd Bergmann wrote: >>> On Friday 02 October 2015 18:02:04 Mason wrote: >>>> Add support for Tango4-based SoCs (SMP8756, SMP8758) >>>> >>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> >>> >>> Please write a proper changelog text here that tells us more about >>> the patch than the subject line. >> >> Sorry, I'm quite unimaginative. What kind of information do you >> consider required? > > The line you have there is not needed, but it would be nice to have > a link to the data sheet (if available) and some information about > what SoCs they are. I'll ask, but I'm afraid there is no public documentation :-( Is this the place to state that Tango4 SoCs are based on ARM Cortex A9 MPCore? (Whereas Tango3 was based on MIPS.) > For most patches, you describe what the original code does wrong > first, followed by how your patch addresses the situation, but for > a new platform that is a bit different. > >>>> --- a/arch/arm/boot/dts/Makefile >>>> +++ b/arch/arm/boot/dts/Makefile >>>> @@ -605,6 +605,8 @@ dtb-$(CONFIG_MACH_SUN8I) += \ >>>> dtb-$(CONFIG_MACH_SUN9I) += \ >>>> sun9i-a80-optimus.dtb \ >>>> sun9i-a80-cubieboard4.dtb >>>> +dtb-$(CONFIG_ARCH_TANGOX) += \ >>>> + vantage-1172.dtb >>> >>> This file name should start with the name of the chip, >>> e.g. 'tango4-vantage-1172.dtb', to make it easier to find. >> >> OK. >> >> (Why doesn't arm do it like mips, with per-manufacturer folders?) > > Historic mistake on our side, and it's become too hard to fix now > without breaking hundreds of patches that people are trying to > get merged. OK. And even new platforms are not put in a separate folder? (For consistency, I imagine.) >>>> diff --git a/arch/arm/boot/dts/tango4.dtsi b/arch/arm/boot/dts/tango4.dtsi >>>> new file mode 100644 >>>> index 000000000000..7336fcc3ac1d >>>> --- /dev/null >>>> +++ b/arch/arm/boot/dts/tango4.dtsi >>>> @@ -0,0 +1,117 @@ >>>> +#include <dt-bindings/interrupt-controller/irq.h> >>>> + >>>> +/ { >>>> + compatible = "sigma,tango4-soc"; >>> >>> Move the root compatible strings into the board specific file, >>> and add the name of the machine as a more specific one. >> >> I don't understand this. > > The compatible list should list both generic and specific names > if applicable. For an SoC based platform, that almost always > translates into board name, soc name and sometimes soc family name. In my case, board name = vantage-1172 soc name would be the specific package? "SMP8758" soc family name would be tango4? or just tango? tangox? Note1: the same DT should work on "SMP8756" (single core Tango4) Note2: Mans is using a slightly different package (SMP8759 IIUC) >>>> + soc { >>>> + compatible = "simple-bus"; >>>> + #address-cells = <1>; >>>> + #size-cells = <1>; >>>> + ranges; >>>> + >>>> + xtal_in_cnt { >>>> + compatible = "sigma,xtal_in_cnt"; >>>> + reg = <0x10048 0x4>; >>>> + clocks = <&xtal>; >>>> + }; >>>> + >>>> + uart0 { >>>> + compatible = "ralink,rt2880-uart"; >>>> + reg = <0x10700 0x100>; >>>> + clock-frequency = <7372800>; >>>> + reg-shift = <2>; >>>> +/* fifo-size = <16>; BROKEN */ >>>> + }; >>> >>> Please use standard node names here, the uart should be >>> named 'serial@10700', the other names should be >>> 'ethernet@26000', 'interrupt-controller@6e000' etc. >> >> Where are the standard node names documented? > > ePAPR has a list, for others look at the existing dts files. > >> Can I still name labels freely? > > Labels can be freely assigned, they are only used as syntactic > sugar to let you write phandle references in a more compact > way, but names are what ends up in the dts and they should > follow convention as much as possible. OK. >> Or is there a naming convention for labels? >> >> Why is the base address duplicated? Can't the DTC figure >> out the address from the reg property? > > I don't know what exactly has led to this, I believe it was > like this in original open firmware, but not always enforced, > but we probably needed to represent nodes from real firmware > in dtb files. OK. >>>> + eth0: eth0 { >>>> + compatible = "sigma,smp8640-emac"; >>>> + reg = <0x26000 0x800>; >>>> + interrupts = <38 4>; >>>> + interrupt-parent = <&irq0>; >>>> + mac-address = [ 00 16 e8 02 08 42 ]; >>>> + clocks = <&sysclk>; >>>> + }; >>>> + >>>> + intc: intc@e000 { >>>> + compatible = "sigma,tango-intc"; >>>> + reg = <0x6e000 0x1000>; >>>> + interrupt-parent = <&gic>; >>>> + interrupt-controller; >>>> + #address-cells = <1>; >>>> + #size-cells = <1>; >>>> + >>>> + irq0: irq0@000 { >>>> + reg = <0x000 0x100>; >>>> + interrupt-controller; >>>> + #interrupt-cells = <2>; >>>> + interrupts = <0 2 4>; >>>> + }; >>> >>> You are missing a ranges property that describes what address >>> space these addresses are in. >> >> ranges; is not hierarchically inherited? > > 'ranges;' would be wrong here, as the interrupt controller is > not a bus. If you have no ranges property, the bus is interpreted > as having its own address space with no relation to the parent bus > (e.g. an I2C bus uses addresses that are not memory mapped). > > Just list the addresses that are actually decoded by child > devices here. Sorry, I really don't understand the "ranges" property. I used "subsections" like "clocks" and "soc" to group related nodes together, not because they require special handling for address translation, or some other need. I mean the gic is at physical address 0x20000600, and the UART is at physical address 0x10700. I was going to say there is nothing different, but that's not completely accurate. Talking to the GIC doesn't leave the CPU, while talking to the UART goes over the global bus. But the interrupt-controller is no different than the UART or eth. Why does this one need special care and not the others? >>>> diff --git a/arch/arm/boot/dts/vantage-1172.dts b/arch/arm/boot/dts/vantage-1172.dts >>>> new file mode 100644 >>>> index 000000000000..56f6babe7093 >>>> --- /dev/null >>>> +++ b/arch/arm/boot/dts/vantage-1172.dts >>>> @@ -0,0 +1,8 @@ >>>> +/dts-v1/; >>>> + >>>> +#include "tango4.dtsi" >>>> + >>>> +ð0 { >>>> + phy-connection-type = "rgmii"; >>>> + max-speed = <1000>; >>>> +}; >>> >>> You should normally have /chosen and /aliases nodes here as well. >> >> Sigh. I don't know what they are for. >> http://devicetree.org/Device_Tree_Usage#Special_Nodes >> >> So with aliases, I don't need to define labels in the .dtsi? > > labels are always optional, you can write the aliases node using > either labels or open-coded paths, like I meant: in the .dtsi I wrote e.g eth0: eth0 (making an eth0 label) just so I could write ð0 in the .dts If I define the eth0 alias, I don't need the label anymore? Sorry for these dumb questions. In my head, DT is really a mess of arbitrary rules (thus hard to follow). > /aliases { > serial0 = &uart_3; > serial1 = "/soc/serial@10700"; > }; > >> "Typically the chosen node is left empty in .dts source files and populated at boot time." >> Should I put my current bootargs there? > > I would put only the stdout-path property in there, as long as you can > boot with any bootargs. I'm pretty sure I have to specify the amount of memory Linux can use. (I do that on the command line now.) My current boot command is: ip=dhcp root=/dev/nfs rdinit=/none console=ttyS0,115200 mem=640M debug >>>> diff --git a/arch/arm/mach-tangox/Kconfig b/arch/arm/mach-tangox/Kconfig >>>> new file mode 100644 >>>> index 000000000000..152cdd487056 >>>> --- /dev/null >>>> +++ b/arch/arm/mach-tangox/Kconfig >>>> @@ -0,0 +1,12 @@ >>>> +# Tango3 was based on MIPS 74kf. Tango4 is based on ARM Cortex A9 MPCore. >>>> + >>>> +config ARCH_TANGOX >>>> + bool "Sigma Designs Tango4 (SMP87xx)" if ARCH_MULTI_V7 >>>> + select ARCH_HAS_HOLES_MEMORYMODEL >>>> + select ARM_ERRATA_754322 >>>> + select ARM_ERRATA_764369 if SMP >>>> + select ARM_GIC >>>> + select GENERIC_IRQ_CHIP >>>> + select HAVE_ARM_SCU >>>> + select HAVE_ARM_TWD >>>> + select SERIAL_8250_RT288X if SERIAL_8250 >>> >>> Do not 'select' the uart driver, that can just be part of the defconfig >>> file. >> >> Do you mean I should provide a defconfig with my patch? >> >> Picking SERIAL_8250 but not SERIAL_8250_RT288X makes the kernel >> panic. Doesn't that qualify for selecting it? (I don't understand >> the rationale behind most conventions in kernel dev.) > > Add it to multi_v7_defconfig OK. And I can make a tango4_defconfig too, right? Regards.
On Friday 02 October 2015 23:57:07 Mason wrote: > On 02/10/2015 23:11, Arnd Bergmann wrote: > > On Friday 02 October 2015 22:53:11 Mason wrote: > > The line you have there is not needed, but it would be nice to have > > a link to the data sheet (if available) and some information about > > what SoCs they are. > > I'll ask, but I'm afraid there is no public documentation :-( > > Is this the place to state that Tango4 SoCs are based on > ARM Cortex A9 MPCore? (Whereas Tango3 was based on MIPS.) Yes. > >> (Why doesn't arm do it like mips, with per-manufacturer folders?) > > > > Historic mistake on our side, and it's become too hard to fix now > > without breaking hundreds of patches that people are trying to > > get merged. > > OK. And even new platforms are not put in a separate folder? > (For consistency, I imagine.) Right. > >>>> diff --git a/arch/arm/boot/dts/tango4.dtsi b/arch/arm/boot/dts/tango4.dtsi > >>>> new file mode 100644 > >>>> index 000000000000..7336fcc3ac1d > >>>> --- /dev/null > >>>> +++ b/arch/arm/boot/dts/tango4.dtsi > >>>> @@ -0,0 +1,117 @@ > >>>> +#include <dt-bindings/interrupt-controller/irq.h> > >>>> + > >>>> +/ { > >>>> + compatible = "sigma,tango4-soc"; > >>> > >>> Move the root compatible strings into the board specific file, > >>> and add the name of the machine as a more specific one. > >> > >> I don't understand this. > > > > The compatible list should list both generic and specific names > > if applicable. For an SoC based platform, that almost always > > translates into board name, soc name and sometimes soc family name. > > In my case, > board name = vantage-1172 > soc name would be the specific package? "SMP8758" > soc family name would be tango4? or just tango? tangox? > > Note1: the same DT should work on "SMP8756" (single core Tango4) > Note2: Mans is using a slightly different package (SMP8759 IIUC) As many of them as you find reasonable. I don't know if the 'x' in tangox is just a wildcard of if that is the official name of the SoC line. If it's a wildcard, don't put it into a compatible string. It could be something like compatible = "sigma,tango4-smp8758-vantage-1172", "sigma,tango4-vantage-1172", "sigma,tango4", "sigma,tango"; > >>>> + eth0: eth0 { > >>>> + compatible = "sigma,smp8640-emac"; > >>>> + reg = <0x26000 0x800>; > >>>> + interrupts = <38 4>; > >>>> + interrupt-parent = <&irq0>; > >>>> + mac-address = [ 00 16 e8 02 08 42 ]; > >>>> + clocks = <&sysclk>; > >>>> + }; > >>>> + > >>>> + intc: intc@e000 { > >>>> + compatible = "sigma,tango-intc"; > >>>> + reg = <0x6e000 0x1000>; > >>>> + interrupt-parent = <&gic>; > >>>> + interrupt-controller; > >>>> + #address-cells = <1>; > >>>> + #size-cells = <1>; > >>>> + > >>>> + irq0: irq0@000 { > >>>> + reg = <0x000 0x100>; > >>>> + interrupt-controller; > >>>> + #interrupt-cells = <2>; > >>>> + interrupts = <0 2 4>; > >>>> + }; > >>> > >>> You are missing a ranges property that describes what address > >>> space these addresses are in. > >> > >> ranges; is not hierarchically inherited? > > > > 'ranges;' would be wrong here, as the interrupt controller is > > not a bus. If you have no ranges property, the bus is interpreted > > as having its own address space with no relation to the parent bus > > (e.g. an I2C bus uses addresses that are not memory mapped). > > > > Just list the addresses that are actually decoded by child > > devices here. > > Sorry, I really don't understand the "ranges" property. > I used "subsections" like "clocks" and "soc" to group related > nodes together, not because they require special handling > for address translation, or some other need. > > I mean the gic is at physical address 0x20000600, and > the UART is at physical address 0x10700. I was going to > say there is nothing different, but that's not completely > accurate. Talking to the GIC doesn't leave the CPU, while > talking to the UART goes over the global bus. > > But the interrupt-controller is no different than the > UART or eth. Why does this one need special care and > not the others? The intc@e000 (interrupt-controller@6e000 really) node has child nodes, the other devices are leaf nodes. If you want to interpret the "reg" property of the irq0@000 (interrupt-controller@0) node, you need to know how the 000 translates into an address of the root bus. I assume you meant to write ranges = <0 0x6e000 0x300>; to say that address 0x000 of the child node is at address 0x6e000 of the parent bus. > >>>> diff --git a/arch/arm/boot/dts/vantage-1172.dts b/arch/arm/boot/dts/vantage-1172.dts > >>>> new file mode 100644 > >>>> index 000000000000..56f6babe7093 > >>>> --- /dev/null > >>>> +++ b/arch/arm/boot/dts/vantage-1172.dts > >>>> @@ -0,0 +1,8 @@ > >>>> +/dts-v1/; > >>>> + > >>>> +#include "tango4.dtsi" > >>>> + > >>>> +ð0 { > >>>> + phy-connection-type = "rgmii"; > >>>> + max-speed = <1000>; > >>>> +}; > >>> > >>> You should normally have /chosen and /aliases nodes here as well. > >> > >> Sigh. I don't know what they are for. > >> http://devicetree.org/Device_Tree_Usage#Special_Nodes > >> > >> So with aliases, I don't need to define labels in the .dtsi? > > > > labels are always optional, you can write the aliases node using > > either labels or open-coded paths, like > > I meant: in the .dtsi I wrote e.g eth0: eth0 (making an eth0 > label) just so I could write ð0 in the .dts > If I define the eth0 alias, I don't need the label anymore? > Sorry for these dumb questions. In my head, DT is really a > mess of arbitrary rules (thus hard to follow). The aliases are mainly useful when something else refers to them. uart drivers tend to use them to pick the right minor device numbers, but a lot of other drivers don't look at them. The stdout-path property can use the alias. > > /aliases { > > serial0 = &uart_3; > > serial1 = "/soc/serial@10700"; > > }; > > > >> "Typically the chosen node is left empty in .dts source files and populated at boot time." > >> Should I put my current bootargs there? > > > > I would put only the stdout-path property in there, as long as you can > > boot with any bootargs. > > I'm pretty sure I have to specify the amount of memory Linux > can use. (I do that on the command line now.) > My current boot command is: > ip=dhcp root=/dev/nfs rdinit=/none console=ttyS0,115200 mem=640M debug The mem= and console= arguments should not be needed here and go through other nodes (/memory/reg and /chosen/stdout-path). "debug" should not be part of this normally, and the rootfs should be set by the bootloader according to its configuration. > >>>> diff --git a/arch/arm/mach-tangox/Kconfig b/arch/arm/mach-tangox/Kconfig > >>>> new file mode 100644 > >>>> index 000000000000..152cdd487056 > >>>> --- /dev/null > >>>> +++ b/arch/arm/mach-tangox/Kconfig > >>>> @@ -0,0 +1,12 @@ > >>>> +# Tango3 was based on MIPS 74kf. Tango4 is based on ARM Cortex A9 MPCore. > >>>> + > >>>> +config ARCH_TANGOX > >>>> + bool "Sigma Designs Tango4 (SMP87xx)" if ARCH_MULTI_V7 > >>>> + select ARCH_HAS_HOLES_MEMORYMODEL > >>>> + select ARM_ERRATA_754322 > >>>> + select ARM_ERRATA_764369 if SMP > >>>> + select ARM_GIC > >>>> + select GENERIC_IRQ_CHIP > >>>> + select HAVE_ARM_SCU > >>>> + select HAVE_ARM_TWD > >>>> + select SERIAL_8250_RT288X if SERIAL_8250 > >>> > >>> Do not 'select' the uart driver, that can just be part of the defconfig > >>> file. > >> > >> Do you mean I should provide a defconfig with my patch? > >> > >> Picking SERIAL_8250 but not SERIAL_8250_RT288X makes the kernel > >> panic. Doesn't that qualify for selecting it? (I don't understand > >> the rationale behind most conventions in kernel dev.) > > > > Add it to multi_v7_defconfig > > OK. And I can make a tango4_defconfig too, right? I'd make a tangox_defconfig, if there is (or will likely be) a tango5 that is compatible enough to work with the same kernel. We try to have only one defconfig per vendor, to keep the amount of our own testing on defconfig files reasonable. Arnd
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 1c5021002fe4..94a1a0277c94 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -934,6 +934,8 @@ source "arch/arm/mach-sunxi/Kconfig" source "arch/arm/mach-prima2/Kconfig" +source "arch/arm/mach-tangox/Kconfig" + source "arch/arm/mach-tegra/Kconfig" source "arch/arm/mach-u300/Kconfig" diff --git a/arch/arm/Makefile b/arch/arm/Makefile index 7451b447cc2d..7fcb4c63cdf7 100644 --- a/arch/arm/Makefile +++ b/arch/arm/Makefile @@ -203,6 +203,7 @@ machine-$(CONFIG_ARCH_SOCFPGA) += socfpga machine-$(CONFIG_ARCH_STI) += sti machine-$(CONFIG_ARCH_STM32) += stm32 machine-$(CONFIG_ARCH_SUNXI) += sunxi +machine-$(CONFIG_ARCH_TANGOX) += tangox machine-$(CONFIG_ARCH_TEGRA) += tegra machine-$(CONFIG_ARCH_U300) += u300 machine-$(CONFIG_ARCH_U8500) += ux500 diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile index 246473a244f6..42ba8b1be0d5 100644 --- a/arch/arm/boot/dts/Makefile +++ b/arch/arm/boot/dts/Makefile @@ -605,6 +605,8 @@ dtb-$(CONFIG_MACH_SUN8I) += \ dtb-$(CONFIG_MACH_SUN9I) += \ sun9i-a80-optimus.dtb \ sun9i-a80-cubieboard4.dtb +dtb-$(CONFIG_ARCH_TANGOX) += \ + vantage-1172.dtb dtb-$(CONFIG_ARCH_TEGRA_2x_SOC) += \ tegra20-harmony.dtb \ tegra20-iris-512.dtb \ diff --git a/arch/arm/boot/dts/tango4.dtsi b/arch/arm/boot/dts/tango4.dtsi new file mode 100644 index 000000000000..7336fcc3ac1d --- /dev/null +++ b/arch/arm/boot/dts/tango4.dtsi @@ -0,0 +1,117 @@ +#include <dt-bindings/interrupt-controller/irq.h> + +/ { + compatible = "sigma,tango4-soc"; + + #address-cells = <1>; + #size-cells = <1>; + + clocks { + ranges; + #address-cells = <1>; + #size-cells = <1>; + + xtal: xtal { + compatible = "fixed-clock"; + clock-frequency = <27000000>; + #clock-cells = <0>; + }; + + sysclk: sysclk { + compatible = "fixed-clock"; + clock-frequency = <396000000>; + #clock-cells = <0>; + }; + + cpuclk: cpuclk { + compatible = "fixed-clock"; + clock-frequency = <999000000>; + #clock-cells = <0>; + }; + + periphclk: periphclk { + compatible = "fixed-factor-clock"; + clocks = <&cpuclk>; + clock-mult = <1>; + clock-div = <2>; + #clock-cells = <0>; + }; + }; + + gic: gic@20001000 { + compatible = "arm,cortex-a9-gic"; + interrupt-controller; + #interrupt-cells = <3>; + reg = <0x20001000 0x1000>, + <0x20000100 0x0100>; + }; + + twd-timer@20000600 { + compatible = "arm,cortex-a9-twd-timer"; + reg = <0x20000600 0x10>; + interrupts = <1 13 0xf01>; + interrupt-parent = <&gic>; + clocks = <&periphclk>; + twd_never_stops; + }; + + soc { + compatible = "simple-bus"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + xtal_in_cnt { + compatible = "sigma,xtal_in_cnt"; + reg = <0x10048 0x4>; + clocks = <&xtal>; + }; + + uart0 { + compatible = "ralink,rt2880-uart"; + reg = <0x10700 0x100>; + clock-frequency = <7372800>; + reg-shift = <2>; +/* fifo-size = <16>; BROKEN */ + }; + + eth0: eth0 { + compatible = "sigma,smp8640-emac"; + reg = <0x26000 0x800>; + interrupts = <38 4>; + interrupt-parent = <&irq0>; + mac-address = [ 00 16 e8 02 08 42 ]; + clocks = <&sysclk>; + }; + + intc: intc@e000 { + compatible = "sigma,tango-intc"; + reg = <0x6e000 0x1000>; + interrupt-parent = <&gic>; + interrupt-controller; + #address-cells = <1>; + #size-cells = <1>; + + irq0: irq0@000 { + reg = <0x000 0x100>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <0 2 4>; + }; + + irq1: irq1@100 { + reg = <0x100 0x100>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <0 3 4>; + }; + + irq2: irq2@300 { + reg = <0x300 0x100>; + interrupt-controller; + #interrupt-cells = <2>; + interrupts = <0 4 4>; + }; + }; + }; +}; diff --git a/arch/arm/boot/dts/vantage-1172.dts b/arch/arm/boot/dts/vantage-1172.dts new file mode 100644 index 000000000000..56f6babe7093 --- /dev/null +++ b/arch/arm/boot/dts/vantage-1172.dts @@ -0,0 +1,8 @@ +/dts-v1/; + +#include "tango4.dtsi" + +ð0 { + phy-connection-type = "rgmii"; + max-speed = <1000>; +}; diff --git a/arch/arm/mach-tangox/Kconfig b/arch/arm/mach-tangox/Kconfig new file mode 100644 index 000000000000..152cdd487056 --- /dev/null +++ b/arch/arm/mach-tangox/Kconfig @@ -0,0 +1,12 @@ +# Tango3 was based on MIPS 74kf. Tango4 is based on ARM Cortex A9 MPCore. + +config ARCH_TANGOX + bool "Sigma Designs Tango4 (SMP87xx)" if ARCH_MULTI_V7 + select ARCH_HAS_HOLES_MEMORYMODEL + select ARM_ERRATA_754322 + select ARM_ERRATA_764369 if SMP + select ARM_GIC + select GENERIC_IRQ_CHIP + select HAVE_ARM_SCU + select HAVE_ARM_TWD + select SERIAL_8250_RT288X if SERIAL_8250 diff --git a/arch/arm/mach-tangox/Makefile b/arch/arm/mach-tangox/Makefile new file mode 100644 index 000000000000..2b9dba458932 --- /dev/null +++ b/arch/arm/mach-tangox/Makefile @@ -0,0 +1 @@ +obj-y += setup.o diff --git a/arch/arm/mach-tangox/setup.c b/arch/arm/mach-tangox/setup.c new file mode 100644 index 000000000000..14baf14bbd49 --- /dev/null +++ b/arch/arm/mach-tangox/setup.c @@ -0,0 +1,7 @@ +#include <asm/mach/arch.h> + +static const char *tango_dt_compat[] = { "sigma,tango4-soc", NULL }; + +DT_MACHINE_START(TANGO_DT, "Sigma TangoX DT") + .dt_compat = tango_dt_compat, +MACHINE_END
Add support for Tango4-based SoCs (SMP8756, SMP8758) Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> --- NOTE: Mans reviewed the patch, and noted that the proposed clock tree is too simplified. I have an upcoming patch fixing that issue. --- arch/arm/Kconfig | 2 + arch/arm/Makefile | 1 + arch/arm/boot/dts/Makefile | 2 + arch/arm/boot/dts/tango4.dtsi | 117 +++++++++++++++++++++++++++++++++++++ arch/arm/boot/dts/vantage-1172.dts | 8 +++ arch/arm/mach-tangox/Kconfig | 12 ++++ arch/arm/mach-tangox/Makefile | 1 + arch/arm/mach-tangox/setup.c | 7 +++ 8 files changed, 150 insertions(+) create mode 100644 arch/arm/boot/dts/tango4.dtsi create mode 100644 arch/arm/boot/dts/vantage-1172.dts create mode 100644 arch/arm/mach-tangox/Kconfig create mode 100644 arch/arm/mach-tangox/Makefile create mode 100644 arch/arm/mach-tangox/setup.c