Re: [PATCH v2 16/18] ARM: dts: Introduce STM32F429 MCU

["Andreas Färber" <afaerber@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 14066 bytes --]

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:
> The STMicrolectornics's STM32F419 MCU has the following main features:
>  - Cortex-M4 core running up to @180MHz
>  - 2MB internal flash, 256KBytes internal RAM
>  - FMC controller to connect SDRAM, NOR and NAND memories
>  - SD/MMC/SDIO support
>  - Ethernet controller
>  - USB OTFG FS & HS controllers
>  - I2C, SPI, CAN busses support
>  - Several 16 & 32 bits general purpose timers
>  - Serial Audio interface
>  - LCD controller
> 
> Signed-off-by: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> ---
>  arch/arm/boot/dts/Makefile            |   1 +
>  arch/arm/boot/dts/stm32f429-disco.dts |  41 ++++
>  arch/arm/boot/dts/stm32f429.dtsi      | 396 ++++++++++++++++++++++++++++++++++
>  3 files changed, 438 insertions(+)
>  create mode 100644 arch/arm/boot/dts/stm32f429-disco.dts
>  create mode 100644 arch/arm/boot/dts/stm32f429.dtsi
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 91bd5bd..d7da0ef 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -442,6 +442,7 @@ dtb-$(CONFIG_ARCH_STI)+= stih407-b2120.dtb \
>  	stih416-b2000.dtb \
>  	stih416-b2020.dtb \
>  	stih416-b2020e.dtb
> +dtb-$(CONFIG_ARCH_STM32)+= stm32f429-disco.dtb
>  dtb-$(CONFIG_MACH_SUN4I) += \
>  	sun4i-a10-a1000.dtb \
>  	sun4i-a10-ba10-tvbox.dtb \
> diff --git a/arch/arm/boot/dts/stm32f429-disco.dts b/arch/arm/boot/dts/stm32f429-disco.dts
> new file mode 100644
> index 0000000..0e79cc1
> --- /dev/null
> +++ b/arch/arm/boot/dts/stm32f429-disco.dts
> @@ -0,0 +1,41 @@

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@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.

> +				default-state = "off";
> +			};
> +		};
> +	};
> +};
> diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
> new file mode 100644
> index 0000000..5b3442a
> --- /dev/null
> +++ b/arch/arm/boot/dts/stm32f429.dtsi
> @@ -0,0 +1,396 @@
> +/*
> + * Device tree for STM32F429

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@e000e010 {
> +		clocks = <&clk_systick>;
> +
> +		status = "okay";
> +	};

&systick {...};

> +
> +	soc {
> +		reset: reset@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@40020000 {
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				reg = <0x0 0x400>;
> +				resets = <&reset GPIOA_RESET>;
> +				st,bank-name = "GPIOA";
> +			};
> +
> +			gpiob: gpio@40020400 {
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				reg = <0x400 0x400>;
> +				resets = <&reset GPIOB_RESET>;
> +				st,bank-name = "GPIOB";
> +			};
> +
> +			gpioc: gpio@40020800 {
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				reg = <0x800 0x400>;
> +				resets = <&reset GPIOC_RESET>;
> +				st,bank-name = "GPIOC";
> +			};
> +
> +			gpiod: gpio@40020c00 {
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				reg = <0xc00 0x400>;
> +				resets = <&reset GPIOD_RESET>;
> +				st,bank-name = "GPIOD";
> +			};
> +
> +			gpioe: gpio@40021000 {
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				reg = <0x1000 0x400>;
> +				resets = <&reset GPIOE_RESET>;
> +				st,bank-name = "GPIOE";
> +			};
> +
> +			gpiof: gpio@40021400 {
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				reg = <0x1400 0x400>;
> +				resets = <&reset GPIOF_RESET>;
> +				st,bank-name = "GPIOF";
> +			};
> +
> +			gpiog: gpio@40021800 {
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				reg = <0x1800 0x400>;
> +				resets = <&reset GPIOG_RESET>;
> +				st,bank-name = "GPIOG";
> +			};
> +
> +			gpioh: gpio@40021c00 {
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				reg = <0x1c00 0x400>;
> +				resets = <&reset GPIOH_RESET>;
> +				st,bank-name = "GPIOH";
> +			};
> +
> +			gpioi: gpio@40022000 {
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				reg = <0x2000 0x400>;
> +				resets = <&reset GPIOI_RESET>;
> +				st,bank-name = "GPIOI";
> +			};
> +
> +			gpioj: gpio@40022400 {
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				reg = <0x2400 0x400>;
> +				resets = <&reset GPIOJ_RESET>;
> +				st,bank-name = "GPIOJ";
> +			};
> +
> +			gpiok: gpio@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@40000000 {
> +			compatible = "st,stm32-timer";
> +			reg = <0x40000000 0x400>;
> +			interrupts = <28>;
> +			resets = <&reset TIM2_RESET>;
> +			clocks = <&clk_pmtr1>;
> +
> +			status = "disabled";
> +		};
> +
> +		timer3: timer@40000400 {
> +			compatible = "st,stm32-timer";
> +			reg = <0x40000400 0x400>;
> +			interrupts = <29>;
> +			resets = <&reset TIM3_RESET>;
> +			clocks = <&clk_pmtr1>;
> +
> +			status = "disabled";
> +		};
> +
> +		timer4: timer@40000800 {
> +			compatible = "st,stm32-timer";
> +			reg = <0x40000800 0x400>;
> +			interrupts = <30>;
> +			resets = <&reset TIM4_RESET>;
> +			clocks = <&clk_pmtr1>;
> +
> +			status = "disabled";
> +		};
> +
> +		timer5: timer@40000c00 {
> +			compatible = "st,stm32-timer";
> +			reg = <0x40000c00 0x400>;
> +			interrupts = <50>;
> +			resets = <&reset TIM5_RESET>;
> +			clocks = <&clk_pmtr1>;
> +		};
> +
> +		timer6: timer@40001000 {
> +			compatible = "st,stm32-timer";
> +			reg = <0x40001000 0x400>;
> +			interrupts = <54>;
> +			resets = <&reset TIM6_RESET>;
> +			clocks = <&clk_pmtr1>;
> +
> +			status = "disabled";
> +		};
> +
> +		timer7: timer@40001400 {
> +			compatible = "st,stm32-timer";
> +			reg = <0x40001400 0x400>;
> +			interrupts = <55>;
> +			resets = <&reset TIM7_RESET>;
> +			clocks = <&clk_pmtr1>;
> +
> +			status = "disabled";
> +		};
> +
> +		usart1: usart@40011000 {
> +			compatible = "st,stm32-usart";
> +			reg = <0x40011000 0x400>;
> +			interrupts = <37>;
> +			clocks = <&clk_pclk2>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_usart1>;
> +
> +			status = "disabled";
> +		};
> +
> +		usart2: usart@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@40004800 {
> +			compatible = "st,stm32-usart";
> +			reg = <0x40004800 0x400>;
> +			interrupts = <39>;
> +			clocks = <&clk_pclk1>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_usart1>;
> +
> +			status = "disabled";
> +		};
> +
> +		usart4: usart@40004c00 {
> +			compatible = "st,stm32-usart";
> +			reg = <0x40004c00 0x400>;
> +			interrupts = <52>;
> +			clocks = <&clk_pclk1>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_usart1>;
> +
> +			status = "disabled";
> +		};
> +
> +		usart5: usart@40005000 {
> +			compatible = "st,stm32-usart";
> +			reg = <0x40005000 0x400>;
> +			interrupts = <53>;
> +			clocks = <&clk_pclk1>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_usart1>;
> +
> +			status = "disabled";
> +		};
> +
> +		usart6: usart@40011400 {
> +			compatible = "st,stm32-usart";
> +			reg = <0x40011400 0x400>;
> +			interrupts = <71>;
> +			clocks = <&clk_pclk2>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_usart1>;
> +
> +			status = "disabled";
> +		};
> +
> +		usart7: usart@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@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)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]