Hi Maxime,

Please don't put the whole world in To, that makes replying to you much
harder. You can put maintainers in CC instead.

Am 20.02.2015 um 19:01 schrieb Maxime Coquelin:

Add a suitable license header here? There had been attempts to
dual-license as GPL and MIT/X11.

+/dts-v1/;
+#include "stm32f429.dtsi"
+
+/ {
+	model = "STMicroelectronics's STM32F429i-DISCO board";

"'s" seems uncommon here and "s's" looks wrong. Just use
"STMicroelectronics STM32F429I-DISCO board"?

+	compatible = "st,stm32f429i-disco", "st,stm32f429";
+
+	chosen {
+		bootargs = "console=ttyS0,115200 root=/dev/ram rdinit=/linuxrc";
+		linux,stdout-path = &usart1;

I have actually been using USART3, following a guide I found on GitHub.
Which pins do you use for USART1, and is one better than the other?

+	};
+
+	memory {
+		reg = <0xd0000000 0x800000>;

I have it at 0x90000000!

+	};
+
+	aliases {
+		ttyS0 = &usart1;

Why ttyS0 here? Shouldn't that be serial0 by convention?

+	};
+
+	soc {
+		usart1: usart at 40011000 {
+			status = "okay";
+		};

This is a new target, so please use the new-style &usart1 {...};
reference rather than replicating the hierarchy.

+
+		leds {

These LEDs are definitely not on the SoC, they're on the board. Please
move one level up.

+			compatible = "gpio-leds";

In this file you're being parsimonious with newline, yet in the .dtsi
you unnecessarily add newlines before the trailing status property.

+			red {
+				#gpio-cells = <2>;

This looks weird. Drop it?

+				label = "Front Panel LED";

"Front Panel"? Both LEDs are on the front, and several other
architectures avoid spaces in the label for accessing it from /sys.

+				gpios = <&gpiog 14 0>;

GPIO_ACTIVE_HIGH

+				linux,default-trigger = "heartbeat";

Suggest to leave both off by default.

+			};
+			green {
+				#gpio-cells = <2>;

Ditto.

+				gpios = <&gpiog 13 0>;

Ditto.

License?

+ */
+#include "armv7-m.dtsi"
+#include <dt-bindings/pinctrl/pinctrl-stm32.h>
+#include <dt-bindings/reset/st,stm32f429.h>
+
+/ {
+
+	aliases {
+		gpio0 = &gpioa;
+		gpio1 = &gpiob;
+		gpio2 = &gpioc;
+		gpio3 = &gpiod;
+		gpio4 = &gpioe;
+		gpio5 = &gpiof;
+		gpio6 = &gpiog;
+		gpio7 = &gpioh;
+		gpio8 = &gpioi;
+		gpio9 = &gpioj;
+		gpio10 = &gpiok;
+	};
+
+	clocks {
+		clk_sysclk: clk-sysclk {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <180000000>;
+		};
+
+		clk_hclk: clk-hclk {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <180000000>;
+		};
+
+		clk_pclk1: clk-pclk1 {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <45000000>;
+		};
+
+		clk_pclk2: clk-pclk2 {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <90000000>;
+		};
+
+		clk_pmtr1: clk-pmtr1 {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <90000000>;
+		};
+
+		clk_pmtr2: clk-pmtr2 {
+			#clock-cells = <0>;
+			compatible = "fixed-clock";
+			clock-frequency = <180000000>;
+		};
+
+		clk_systick: clk-systick {
+			compatible = "fixed-factor-clock";
+			clocks = <&clk_hclk>;
+			#clock-cells = <0>;
+			clock-div = <8>;
+			clock-mult = <1>;
+		};

Most of these should be replaceable with my clk driver.

+	};
+
+	systick: timer at e000e010 {
+		clocks = <&clk_systick>;
+
+		status = "okay";
+	};

&systick {...};

+
+	soc {
+		reset: reset at 40023810 {
+			#reset-cells = <1>;
+			compatible = "st,stm32-reset";
+			reg = <0x40023810 0x18>;
+		};

Still, this feels wrong, given that it's an RCC IP block...

+
+		pin-controller {
+			#address-cells = <1>;
+			#size-cells = <1>;
+			compatible = "st,stm32-pinctrl";
+			ranges = <0 0x40020000 0x3000>;
+
+			gpioa: gpio at 40020000 {
+				gpio-controller;
+				#gpio-cells = <2>;
+				reg = <0x0 0x400>;
+				resets = <&reset GPIOA_RESET>;
+				st,bank-name = "GPIOA";
+			};
+
+			gpiob: gpio at 40020400 {
+				gpio-controller;
+				#gpio-cells = <2>;
+				reg = <0x400 0x400>;
+				resets = <&reset GPIOB_RESET>;
+				st,bank-name = "GPIOB";
+			};
+
+			gpioc: gpio at 40020800 {
+				gpio-controller;
+				#gpio-cells = <2>;
+				reg = <0x800 0x400>;
+				resets = <&reset GPIOC_RESET>;
+				st,bank-name = "GPIOC";
+			};
+
+			gpiod: gpio at 40020c00 {
+				gpio-controller;
+				#gpio-cells = <2>;
+				reg = <0xc00 0x400>;
+				resets = <&reset GPIOD_RESET>;
+				st,bank-name = "GPIOD";
+			};
+
+			gpioe: gpio at 40021000 {
+				gpio-controller;
+				#gpio-cells = <2>;
+				reg = <0x1000 0x400>;
+				resets = <&reset GPIOE_RESET>;
+				st,bank-name = "GPIOE";
+			};
+
+			gpiof: gpio at 40021400 {
+				gpio-controller;
+				#gpio-cells = <2>;
+				reg = <0x1400 0x400>;
+				resets = <&reset GPIOF_RESET>;
+				st,bank-name = "GPIOF";
+			};
+
+			gpiog: gpio at 40021800 {
+				gpio-controller;
+				#gpio-cells = <2>;
+				reg = <0x1800 0x400>;
+				resets = <&reset GPIOG_RESET>;
+				st,bank-name = "GPIOG";
+			};
+
+			gpioh: gpio at 40021c00 {
+				gpio-controller;
+				#gpio-cells = <2>;
+				reg = <0x1c00 0x400>;
+				resets = <&reset GPIOH_RESET>;
+				st,bank-name = "GPIOH";
+			};
+
+			gpioi: gpio at 40022000 {
+				gpio-controller;
+				#gpio-cells = <2>;
+				reg = <0x2000 0x400>;
+				resets = <&reset GPIOI_RESET>;
+				st,bank-name = "GPIOI";
+			};
+
+			gpioj: gpio at 40022400 {
+				gpio-controller;
+				#gpio-cells = <2>;
+				reg = <0x2400 0x400>;
+				resets = <&reset GPIOJ_RESET>;
+				st,bank-name = "GPIOJ";
+			};
+
+			gpiok: gpio at 40022800 {
+				gpio-controller;
+				#gpio-cells = <2>;
+				reg = <0x2800 0x400>;
+				resets = <&reset GPIOK_RESET>;
+				st,bank-name = "GPIOK";
+			};
+
+			usart1 {
+				pinctrl_usart1: usart1-0 {
+					st,pins {
+						tx = <&gpioa 9 ALT7 NO_PULL PUSH_PULL LOW_SPEED>;
+						rx = <&gpioa 10 ALT7 NO_PULL>;
+					};
+				};
+			};
+
+			usart2 {
+				pinctrl_usart2: usart2-0 {
+					st,pins {
+						tx = <&gpioa 2 ALT7 NO_PULL PUSH_PULL LOW_SPEED>;
+						rx = <&gpioa 3 ALT7 NO_PULL>;
+					};
+				};
+			};
+
+			usart3 {
+				pinctrl_usart3: usart3-0 {
+					st,pins {
+						tx = <&gpiob 10 ALT7 NO_PULL PUSH_PULL LOW_SPEED>;
+						rx = <&gpiob 11 ALT7 NO_PULL>;
+					};
+				};
+			};
+
+			usart4 {
+				pinctrl_usart4: usart4-0 {
+					st,pins {
+						tx = <&gpioa 0 ALT8 NO_PULL PUSH_PULL LOW_SPEED>;
+						rx = <&gpioa 1 ALT8 NO_PULL>;
+					};
+				};
+			};
+
+			usart5 {
+				pinctrl_usart5: usart5-0 {
+					st,pins {
+						tx = <&gpioc 12 ALT8 NO_PULL PUSH_PULL LOW_SPEED>;
+						rx = <&gpiod 2 ALT8 NO_PULL>;
+					};
+				};
+			};
+
+			usart6 {
+				pinctrl_usart6: usart6-0 {
+					st,pins {
+						tx = <&gpioc 6 ALT8 NO_PULL PUSH_PULL LOW_SPEED>;
+						rx = <&gpioc 7 ALT8 NO_PULL>;
+					};
+				};
+			};
+
+			usart7 {
+				pinctrl_usart7: usart7-0 {
+					st,pins {
+						tx = <&gpioe 8 ALT8 NO_PULL PUSH_PULL LOW_SPEED>;
+						rx = <&gpioe 7 ALT8 NO_PULL>;
+					};
+				};
+			};
+
+			usart8 {
+				pinctrl_usart8: usart8-0 {
+					st,pins {
+						tx = <&gpioe 1 ALT8 NO_PULL PUSH_PULL LOW_SPEED>;
+						rx = <&gpioe 0 ALT8 NO_PULL>;
+					};
+				};
+			};

Can't the outer level be avoided here to save space and indentation?

+		};
+
+		timer2: timer at 40000000 {
+			compatible = "st,stm32-timer";
+			reg = <0x40000000 0x400>;
+			interrupts = <28>;
+			resets = <&reset TIM2_RESET>;
+			clocks = <&clk_pmtr1>;
+
+			status = "disabled";
+		};
+
+		timer3: timer at 40000400 {
+			compatible = "st,stm32-timer";
+			reg = <0x40000400 0x400>;
+			interrupts = <29>;
+			resets = <&reset TIM3_RESET>;
+			clocks = <&clk_pmtr1>;
+
+			status = "disabled";
+		};
+
+		timer4: timer at 40000800 {
+			compatible = "st,stm32-timer";
+			reg = <0x40000800 0x400>;
+			interrupts = <30>;
+			resets = <&reset TIM4_RESET>;
+			clocks = <&clk_pmtr1>;
+
+			status = "disabled";
+		};
+
+		timer5: timer at 40000c00 {
+			compatible = "st,stm32-timer";
+			reg = <0x40000c00 0x400>;
+			interrupts = <50>;
+			resets = <&reset TIM5_RESET>;
+			clocks = <&clk_pmtr1>;
+		};
+
+		timer6: timer at 40001000 {
+			compatible = "st,stm32-timer";
+			reg = <0x40001000 0x400>;
+			interrupts = <54>;
+			resets = <&reset TIM6_RESET>;
+			clocks = <&clk_pmtr1>;
+
+			status = "disabled";
+		};
+
+		timer7: timer at 40001400 {
+			compatible = "st,stm32-timer";
+			reg = <0x40001400 0x400>;
+			interrupts = <55>;
+			resets = <&reset TIM7_RESET>;
+			clocks = <&clk_pmtr1>;
+
+			status = "disabled";
+		};
+
+		usart1: usart at 40011000 {
+			compatible = "st,stm32-usart";
+			reg = <0x40011000 0x400>;
+			interrupts = <37>;
+			clocks = <&clk_pclk2>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usart1>;
+
+			status = "disabled";
+		};
+
+		usart2: usart at 40004400 {
+			compatible = "st,stm32-usart";
+			reg = <0x40004400 0x400>;
+			interrupts = <38>;
+			clocks = <&clk_pclk1>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usart1>;

Copy&paste, also below. Is this really .dtsi material?

+
+			status = "disabled";
+		};
+
+		usart3: usart at 40004800 {
+			compatible = "st,stm32-usart";
+			reg = <0x40004800 0x400>;
+			interrupts = <39>;
+			clocks = <&clk_pclk1>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usart1>;
+
+			status = "disabled";
+		};
+
+		usart4: usart at 40004c00 {
+			compatible = "st,stm32-usart";
+			reg = <0x40004c00 0x400>;
+			interrupts = <52>;
+			clocks = <&clk_pclk1>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usart1>;
+
+			status = "disabled";
+		};
+
+		usart5: usart at 40005000 {
+			compatible = "st,stm32-usart";
+			reg = <0x40005000 0x400>;
+			interrupts = <53>;
+			clocks = <&clk_pclk1>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usart1>;
+
+			status = "disabled";
+		};
+
+		usart6: usart at 40011400 {
+			compatible = "st,stm32-usart";
+			reg = <0x40011400 0x400>;
+			interrupts = <71>;
+			clocks = <&clk_pclk2>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usart1>;
+
+			status = "disabled";
+		};
+
+		usart7: usart at 40007800 {

Please order all nodes by unit address, not by label.

+			compatible = "st,stm32-usart";
+			reg = <0x40007800 0x400>;
+			interrupts = <82>;
+			clocks = <&clk_pclk1>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usart1>;
+
+			status = "disabled";
+		};
+
+		usart8: usart at 40007c00 {
+			compatible = "st,stm32-usart";
+			reg = <0x40007c00 0x400>;
+			interrupts = <83>;
+			clocks = <&clk_pclk1>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usart1>;
+
+			status = "disabled";
+		};

I remember two of them not being USART but UART? Similarly, two of them
support flow control (or two don't?), for which we would either need a
property or two different compatible strings.

+	};
+};

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG N?rnberg)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150302/1315830b/attachment-0001.sig>