Re: [PATCH 7/7] arm64: dts: exynos: Add initial E850-96 board support

On Wed, 15 Dec 2021 at 19:01, Krzysztof Kozlowski
[off-list ref] wrote:

On 15/12/2021 17:09, Sam Protsenko wrote:

quoted

E850-96 is a 96boards development board manufactured by WinLink. It
incorporates Samsung Exynos850 SoC, and is compatible with 96boards
mezzanine boards [1], as it follows 96boards standards.

This patch adds minimal support for E850-96 board. Next features are
enabled in board dts file and verified with minimal BusyBox rootfs:

 * User buttons
 * LEDs
 * Serial console
 * Watchdog timers
 * RTC
 * eMMC

[1] https://www.96boards.org/products/mezzanine/

Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
 arch/arm64/boot/dts/exynos/Makefile           |   3 +-
 .../boot/dts/exynos/exynos850-e850-96.dts     | 157 ++++++++++++++++++
 2 files changed, 159 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/boot/dts/exynos/exynos850-e850-96.dts

diff --git a/arch/arm64/boot/dts/exynos/Makefile b/arch/arm64/boot/dts/exynos/Makefile
index b41e86df0a84..803548ccc537 100644
--- a/arch/arm64/boot/dts/exynos/Makefile
+++ b/arch/arm64/boot/dts/exynos/Makefile

@@ -3,4 +3,5 @@ dtb-$(CONFIG_ARCH_EXYNOS) += \
      exynos5433-tm2.dtb      \
      exynos5433-tm2e.dtb     \
      exynos7-espresso.dtb    \
-     exynosautov9-sadk.dtb
+     exynosautov9-sadk.dtb   \
+     exynos850-e850-96.dtb

Alphabetical order please, so before autov9.

quoted

diff --git a/arch/arm64/boot/dts/exynos/exynos850-e850-96.dts b/arch/arm64/boot/dts/exynos/exynos850-e850-96.dts
new file mode 100644
index 000000000000..fd611906d81c
--- /dev/null
+++ b/arch/arm64/boot/dts/exynos/exynos850-e850-96.dts

@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * WinLink E850-96 board device tree source
+ *
+ * Copyright (C) 2018 Samsung Electronics Co., Ltd.
+ * Copyright (C) 2021 Linaro Ltd.
+ *
+ * Device tree source file for WinLink's E850-96 board which is based on
+ * Samsung Exynos850 SoC.
+ */
+
+/dts-v1/;
+
+#include "exynos850.dtsi"
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+
+#define BOARD_ID     0x0
+#define BOARD_REV    0x2

No need for define for single-used constant.

quoted

+
+/ {
+     model = "WinLink E850-96 board";
+     compatible = "winlink,e850-96", "samsung,exynos850";
+     board_id = <BOARD_ID>;
+     board_rev = <BOARD_REV>;

Unknown properties. They need dtschema.

Those are not really needed in case of upstream linux (only one board
revision is added and no dtbo to merge in bootloader). Will remove
those in v2.

quoted

+
+     chosen {
+             stdout-path = &serial_0;
+     };
+
+     gpio-keys {
+             compatible = "gpio-keys";
+             pinctrl-names = "default";
+             pinctrl-0 = <&key_voldown_pins &key_volup_pins>;
+
+             volume-down-key {
+                     label = "Volume Down";
+                     linux,code = <KEY_VOLUMEDOWN>;
+                     gpios = <&gpa1 0 GPIO_ACTIVE_LOW>;
+             };
+
+             volume-up-key {
+                     label = "Volume Up";
+                     linux,code = <KEY_VOLUMEUP>;
+                     gpios = <&gpa0 7 GPIO_ACTIVE_LOW>;
+             };
+     };
+
+     leds {
+             compatible = "gpio-leds";
+
+             /* HEART_BEAT_LED */
+             user_led1: led-1 {
+                     label = "yellow:user1";

Add where applicable:
1. function, e.g. LED_FUNCTION_HEARTBEAT, LED_FUNCTION_WLAN, etc,
2. color constants.

I actually had those defined initially :) But then specifically
decided to remove those, as those are not very helpful when "label"
and "linux,default-trigger" are already defined (and not many other
boards seem to provide it). But ok, I'll pull those back in v2.

quoted

+                     gpios = <&gpg2 2 GPIO_ACTIVE_HIGH>;
+                     linux,default-trigger = "heartbeat";
+             };
+
+             /* eMMC_LED */
+             user_led2: led-2 {
+                     label = "yellow:user2";
+                     gpios = <&gpg2 3 GPIO_ACTIVE_HIGH>;
+                     linux,default-trigger = "mmc0";
+             };
+
+             /* SD_LED */
+             user_led3: led-3 {
+                     label = "white:user3";
+                     gpios = <&gpg2 4 GPIO_ACTIVE_HIGH>;
+                     linux,default-trigger = "mmc2";
+             };
+
+             /* WIFI_LED */
+             wlan_active_led: led-4 {
+                     label = "yellow:wlan";
+                     gpios = <&gpg2 6 GPIO_ACTIVE_HIGH>;
+                     linux,default-trigger = "phy0tx";
+                     default-state = "off";
+             };
+
+             /* BLUETOOTH_LED */
+             bt_active_led: led-5 {
+                     label = "blue:bt";
+                     gpios = <&gpg2 7 GPIO_ACTIVE_HIGH>;
+                     linux,default-trigger = "hci0rx";
+                     default-state = "off";
+             };
+     };
+};
+
+&oscclk {> + clock-frequency = <26000000>;
+};
+
+&rtcclk {
+     clock-frequency = <32768>;
+};
+
+&usi_uart {
+     samsung,clkreq-on; /* needed for UART mode */
+     status = "okay";
+};
+
+&serial_0 {

Order all phandle overrides by phandle name, so:
&oscclk
&rtcclk
&serial_0
&usi_uart
...

quoted

+     status = "okay";
+     pinctrl-names = "default";
+     pinctrl-0 = <&uart1_pins>;
+};
+
+&watchdog_cl0 {
+     status = "okay";
+};
+
+&watchdog_cl1 {
+     status = "okay";
+};
+
+&rtc {
+     status = "okay";
+};
+
+&mmc_0 {
+     status = "okay";
+     mmc-hs200-1_8v;
+     mmc-hs400-1_8v;
+     cap-mmc-highspeed;
+     non-removable;
+     broken-cd;

Is it correct to have non-removable (typical for eMMC) and broken CD?

Nice catch, not sure how I missed that. It's just ignored in dw_mmc
driver in case of "non-removable", but that property just doesn't make
any sense here.

This and all above comments will be addressed in v2.

quoted

+     mmc-hs400-enhanced-strobe;
+     card-detect-delay = <200>;
+     clock-frequency = <800000000>;
+     bus-width = <8>;
+     samsung,dw-mshc-ciu-div = <3>;
+     samsung,dw-mshc-sdr-timing = <0 4>;
+     samsung,dw-mshc-ddr-timing = <2 4>;
+     samsung,dw-mshc-hs400-timing = <0 2>;
+
+     pinctrl-names = "default";
+     pinctrl-0 = <&sd0_clk_pins &sd0_cmd_pins &sd0_rdqs_pins &sd0_nreset_pins
+                  &sd0_bus1_pins &sd0_bus4_pins &sd0_bus8_pins>;
+};
+

Best regards,
Krzysztof