Skip to contentSkip to conversation

Add support for WD MyCloud EX2 Ultra (+ versatile UART-based restart/poweroff drivers)

PATCH In review Evgeny Kolesnikov 4 participants 12 messages Jul 22, 2019 – Aug 12, 2019

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover Add support for WD MyCloud EX2 Ultra (+ versatile UART-based restart/poweroff drivers)

This patchset consists of the DTS, which describes the WD MyCloud EX2 Ultra device, 'poweroff' and 'resert' drivers for power-managing MCUs connected to a board via UART (these drivers are more versatile than qnap-poweroff and could be used as a substitude), and DT bindings for these drivers.

The difference between uart-poweroff and qnap-poweroff is small, but important:
uart-poweroff is able to send to an MCU a command of arbitrary length, and the command itself is defined in a DTS file for a specific device/board, thus making this driver applicable to wider range of devices.

Evgeny Kolesnikov (5):
power: reset: Add UART-based MCU poweroff DT bindings
power: reset: Add UART-based MCU restart DT bindings power/reset: Add a power off driver for UART-based PM MCUs power/reset: Add a restart driver for UART-based PM MCUs
ARM: dts: armada385-wd-mcex2u: Add DTS file for WD My Cloud EX2 Ultra

 .../bindings/power/reset/uart-poweroff.txt    |  38 +++
 .../bindings/power/reset/uart-restart.txt     |  39 +++
 arch/arm/boot/dts/armada-385-wd-mcex2u.dts    | 313 ++++++++++++++++++
 drivers/power/reset/Kconfig                   |  14 +
 drivers/power/reset/Makefile                  |   2 +
 drivers/power/reset/uart-poweroff.c           | 155 +++++++++
 drivers/power/reset/uart-restart.c            | 204 ++++++++++++
 7 files changed, 765 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/uart-poweroff.txt
 create mode 100644 Documentation/devicetree/bindings/power/reset/uart-restart.txt
 create mode 100644 arch/arm/boot/dts/armada-385-wd-mcex2u.dts
 create mode 100644 drivers/power/reset/uart-poweroff.c
 create mode 100644 drivers/power/reset/uart-restart.c
Signature · 1 line
2.21.0
#1 lore raw

PATCH 1/5 power: reset: Add UART-based MCU poweroff DT bindings

This adds device tree bindings of the poweroff driver for power managing micro controller units that are connected to a board via the UART interface.

---
 .../bindings/power/reset/uart-poweroff.txt    | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/uart-poweroff.txt
Documentation/devicetree/bindings/power/reset/uart-poweroff.txt new file +38 -0
new file mode 100644
index 000000000000..86d036271b51
@@ -0,0 +1,38 @@
+* UART-based PM MCU power off driver
+
+Some devices have a microcontroller controlling the main power
+supply. This microcontroller is connected to UART of the SoC.
+Sending a sequence of characters tells the MCU to turn
+the power off.
+
+The length of the sequence, baud rate and the timeout to process
+the command may vary from device to device.
+
+This driver could be used for WD My Cloud (Marvell SoCs), QNAP
+(Kirkwood and Orion5x SoCs) and Synology NAS devices.
+
+Required properties:
+- compatible: Should be "uart-poweroff"
+- reg: Address and length of the register set for UART
+- clocks: The tclk clock
+- cmd: Array of bytes, the command to send to the MCU
+- baud: Baud rate [75..460800]
+
+Optional properties:
+- byte-delay: A delay after each byte of a command, could be useful
+              for sloppy MCUs [ms], default value is 5
+- timeout: A timeout to wait for the MCU to process the command [ms],
+           default value is 1000
+- override: Boolean flag that indicates if the driver should
+            prioritize itself over any existing power off driver,
+            default behaviour is not to interfere
+
+Example:
+	poweroff@12100 {
+		compatible = "uart-poweroff";
+		reg = <0x12100 0x100>;
+		clocks = <&coreclk 0>;
+		baud = <19200>;
+		cmd = [fa 03 03 01 00 00 fb];
+		status = "okay";
+	};
Signature · 1 line
2.21.0
maintainer
#2 lore raw

Re: [PATCH 1/5] power: reset: Add UART-based MCU poweroff DT bindings

Quoted 16 lines · Evgeny Kolesnikov

This adds device tree bindings of the poweroff driver for power managing micro controller units that are connected to a board via the UART interface.

---
 .../bindings/power/reset/uart-poweroff.txt    | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/uart-poweroff.txt
Documentation/devicetree/bindings/power/reset/uart-poweroff.txt new file +0 -0
new file mode 100644
index 000000000000..86d036271b51

Documentation/devicetree/bindings/power/reset/uart-poweroff.txt line 6

Quoted 6 lines
Documentation/devicetree/bindings/power/reset/uart-poweroff.txt new file +6 -0
@@ -0,0 +1,38 @@
+* UART-based PM MCU power off driver
+
+Some devices have a microcontroller controlling the main power
+supply. This microcontroller is connected to UART of the SoC.
+Sending a sequence of characters tells the MCU to turn
+the power off.

IMO, you should have a node representing the specific microcontroller. Generic binding attempts like this generally don't work well because you need a never ending addition of properties to deal with h/w (and f/w in this case) differences. The properties you already have are evidence of this.

Now, if you want a common driver, then that is a separate issue. You can have multiple, specific bindings map to a common driver (or not, it's up to the OS).

Rob

#3 lore raw

PATCH 2/5 power: reset: Add UART-based MCU restart DT bindings

In reply to Evgeny Kolesnikov

This adds device tree bindings of the restart driver for power managing micro controller units that are connected to a board via the UART interface.

---
 .../bindings/power/reset/uart-restart.txt     | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/uart-restart.txt
Documentation/devicetree/bindings/power/reset/uart-restart.txt new file +39 -0
new file mode 100644
index 000000000000..bf831d9bdff8
@@ -0,0 +1,39 @@
+* UART-based PM MCU restart driver
+
+Some devices have a microcontroller controlling the main power
+supply. This microcontroller is connected to UART of the SoC.
+Sending a sequence of characters tells the MCU to reset the SoC
+and/or MCU itself.
+
+The length of the sequence, baud rate and the timeout to process
+the command may vary from device to device.
+
+This driver could be used for WD My Cloud (Marvell SoCs), QNAP
+(Kirkwood and Orion5x SoCs) and Synology NAS devices.
+
+Required properties:
+- compatible: Should be "uart-restart"
+- reg: Address and length of the register set for UART
+- clocks: The tclk clock
+- cmd: Array of bytes, the command to send to the MCU
+- baud: Baud rate [75..460800]
+
+Optional properties:
+- byte-delay: A delay after each byte of a command, could be useful
+              for sloppy MCUs [ms], default value is 5
+- timeout: A timeout to wait for the MCU to process the command [ms],
+           default value is 1000
+- override: Boolean flag that indicates if the driver should
+            prioritize itself over any existing restart driver,
+            default behaviour is not to interfere
+
+Example:
+	restart@12100 {
+		compatible = "uart-restart";
+		reg = <0x12100 0x100>;
+		clocks = <&coreclk 0>;
+		baud = <19200>;
+		cmd = [fa 03 03 02 00 00 fb];
+		override;
+		status = "okay";
+	};
Signature · 1 line
2.21.0
#4 lore raw

PATCH 3/5 power/reset: Add a power off driver for UART-based PM MCUs

In reply to Evgeny Kolesnikov

This adds the poweroff driver for power managing micro controller units that are connected to a board via the UART interface.

---
 drivers/power/reset/Kconfig         |   7 ++
 drivers/power/reset/Makefile        |   1 +
 drivers/power/reset/uart-poweroff.c | 155 ++++++++++++++++++++++++++++
 3 files changed, 163 insertions(+)
 create mode 100644 drivers/power/reset/uart-poweroff.c
drivers/power/reset/Kconfig +7 -0
index a564237278ff..02fdf45e3988 100644
@@ -216,6 +216,13 @@ config POWER_RESET_RMOBILE
 	help
 	  Reboot support for Renesas R-Mobile and SH-Mobile SoCs.
 
+config POWER_RESET_UART_POWEROFF
+	tristate "UART-based PM MCU power off driver"
+	depends on OF_GPIO
+	help
+	  Power off support for boards with UART-based PM MCU
+	  such as WD My Cloud NAS, QNAP Turbo NAS, Synology devices.
+
 config POWER_RESET_ZX
 	tristate "ZTE SoCs reset driver"
 	depends on ARCH_ZX || COMPILE_TEST
drivers/power/reset/Makefile +1 -0
index 85da3198e4e0..efe8f25f463d 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_POWER_RESET_KEYSTONE) += keystone-reset.o
 obj-$(CONFIG_POWER_RESET_SYSCON) += syscon-reboot.o
 obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
 obj-$(CONFIG_POWER_RESET_RMOBILE) += rmobile-reset.o
+obj-$(CONFIG_POWER_RESET_UART_POWEROFF) += uart-poweroff.o
 obj-$(CONFIG_POWER_RESET_ZX) += zx-reboot.o
 obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o
 obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
drivers/power/reset/uart-poweroff.c new file +155 -0

155 lines not on this page · raw message

Signature · 1 line
2.21.0
#5 lore raw

PATCH 4/5 power/reset: Add a restart driver for UART-based PM MCUs

In reply to Evgeny Kolesnikov

This adds the restart driver for power managing micro controller units that are connected to a board via the UART interface.

---
 drivers/power/reset/Kconfig        |   7 +
 drivers/power/reset/Makefile       |   1 +
 drivers/power/reset/uart-restart.c | 204 +++++++++++++++++++++++++++++
 3 files changed, 212 insertions(+)
 create mode 100644 drivers/power/reset/uart-restart.c
drivers/power/reset/Kconfig +7 -0
index 02fdf45e3988..4b187af1fba6 100644
@@ -223,6 +223,13 @@ config POWER_RESET_UART_POWEROFF
 	  Power off support for boards with UART-based PM MCU
 	  such as WD My Cloud NAS, QNAP Turbo NAS, Synology devices.
 
+config POWER_RESET_UART
+	tristate "UART-based PM MCU restart driver"
+	depends on OF_GPIO
+	help
+	  Reboot support for boards with UART-based PM MCU
+	  such as WD My Cloud NAS, QNAP Turbo NAS, Synology devices.
+
 config POWER_RESET_ZX
 	tristate "ZTE SoCs reset driver"
 	depends on ARCH_ZX || COMPILE_TEST
drivers/power/reset/Makefile +1 -0
index efe8f25f463d..fa8a936d7a1a 100644
@@ -26,6 +26,7 @@ obj-$(CONFIG_POWER_RESET_SYSCON) += syscon-reboot.o
 obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
 obj-$(CONFIG_POWER_RESET_RMOBILE) += rmobile-reset.o
 obj-$(CONFIG_POWER_RESET_UART_POWEROFF) += uart-poweroff.o
+obj-$(CONFIG_POWER_RESET_UART) += uart-restart.o
 obj-$(CONFIG_POWER_RESET_ZX) += zx-reboot.o
 obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o
 obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o
drivers/power/reset/uart-restart.c new file +204 -0

204 lines not on this page · raw message

Signature · 1 line
2.21.0
#6 lore raw

PATCH 5/5 ARM: dts: armada385-wd-mcex2u: Add DTS file for WD My Cloud EX2 Ultra

In reply to Evgeny Kolesnikov

Add the DTS file to describe the Western Digital My Cloud EX2 Ultra NAS.

---
 arch/arm/boot/dts/armada-385-wd-mcex2u.dts | 313 +++++++++++++++++++++
 1 file changed, 313 insertions(+)
 create mode 100644 arch/arm/boot/dts/armada-385-wd-mcex2u.dts
arch/arm/boot/dts/armada-385-wd-mcex2u.dts new file +313 -0
new file mode 100644
index 000000000000..018e66adbb93
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/*
+ * Device Tree file for Western Digital My Cloud EX2 Ultra
+ * (BVBZ/Ranger Peak)
+ *
+ * Copyright (C) 2019 Evgeny Kolesnikov <[email protected]>
+ *
+ * Based on the code from:
+ *
+ * Copyright (C) 2016 Martin Mueller <[email protected]>
+ * Copyright (C) 2013 Gregory CLEMENT <[email protected]>
+ * Copyright (C) 2013 Thomas Petazzoni <[email protected]>
+ */
+
+/dts-v1/;
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/gpio/gpio.h>
+#include "armada-385.dtsi"
+
+/ {
+	model = "WD My Cloud EX2 Ultra (BVBZ/Ranger Peak)";
+	compatible = "wd,mcex2u", "marvell,armada385", "marvell,armada380";
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+		bootargs = "console=ttyS0,115200";
+	};
+
+	memory {
+		device_type = "memory";
+		reg = <0x00000000 0x40000000>; /* 1024 MB */
+	};
+
+	soc {
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0xf1000000 0x100000
+			  MBUS_ID(0x01, 0x1d) 0 0xfff00000 0x100000
+			  MBUS_ID(0x09, 0x19) 0 0xf1100000 0x10000
+			  MBUS_ID(0x09, 0x15) 0 0xf1110000 0x10000
+			  MBUS_ID(0x0c, 0x04) 0 0xf1200000 0x100000>;
+
+		internal-regs {
+			timer@c200 {
+				status = "disabled";
+			};
+
+			i2c0: i2c@11000 {
+				status = "okay";
+				clock-frequency = <100000>;
+			};
+
+			i2c1: i2c@11100 {
+				status = "okay";
+				clock-frequency = <100000>;
+			};
+
+			serial@12000 {
+				status = "okay";
+			};
+
+			/* Connected to Welltrend 6703F-OG240WT MCU
+			 * which controls power, fan and other things
+			 */
+			serial@12100 {
+				status = "okay";
+			};
+
+			poweroff@12100 {
+				compatible = "uart-poweroff";
+				reg = <0x12100 0x100>;
+				clocks = <&coreclk 0>;
+				baud = <19200>;
+				cmd = [fa 03 03 01 00 00 fb];
+				status = "okay";
+			};
+
+			restart@12100 {
+				compatible = "uart-restart";
+				reg = <0x12100 0x100>;
+				clocks = <&coreclk 0>;
+				baud = <19200>;
+				cmd = [fa 03 03 02 00 00 fb];
+				override;
+				status = "okay";
+			};
+
+			pinctrl@18000 {
+				uart1_pins: uart-pins-1 {
+					marvell,pins = "mpp19";
+					marvell,function = "ua1";
+				};
+
+				xhci0_vbus_pins: xhci0-vbus-pins {
+					marvell,pins = "mpp26";
+					marvell,function = "gpio";
+				};
+
+				xhci1_vbus_pins: xhci1-vbus-pins {
+					marvell,pins = "mpp27";
+					marvell,function = "gpio";
+				};
+
+				sata0_pins: sata-pins-0 {
+					marvell,pins = "mpp55";
+					marvell,function = "sata0";
+				};
+
+				sata1_pins: sata-pins-1 {
+					marvell,pins = "mpp56";
+					marvell,function = "sata1";
+				};
+
+				sata_leds: sata-leds {
+					marvell,pins = "mpp43", "mpp52",
+						       "mpp53", "mpp54";
+					marvell,function = "gpio";
+				};
+
+				btn_pins: btn-pins {
+					marvell,pins = "mpp50";
+					marvell,function = "gpio";
+				};
+			};
+
+			ethernet@34000 {
+				phy = <&phy0>;
+				phy-mode = "sgmii";
+				buffer-manager = <&bm>;
+				bm,pool-long = <0>;
+				bm,pool-short = <1>;
+				status = "okay";
+			};
+
+			usb@58000 {
+				status = "okay";
+			};
+
+			mdio@72004 {
+				phy0: ethernet-phy@0 {
+					/* Init ETH LEDs */
+					marvell,reg-init = <3 16 0 0x101e>;
+					reg = <0>;
+				};
+			};
+
+			crypto@9d000 {
+				status = "okay";
+			};
+
+			rtc@a3800 {
+				status = "okay";
+			};
+
+			sata@a8000 {
+				status = "okay";
+			};
+
+			bm@c8000 {
+				status = "okay";
+			};
+
+			nand-controller@d0000 {
+				status = "okay";
+
+				nand: nand@0 {
+					reg = <0>;
+					label = "pxa3xx_nand-0";
+					marvell,nand-keep-config;
+					nand-rb = <0>;
+					nand-on-flash-bbt;
+					nand-ecc-strength = <4>;
+					nand-ecc-step-size = <512>;
+
+					partitions {
+						compatible = "fixed-partitions";
+						#address-cells = <1>;
+						#size-cells = <1>;
+
+						partition@0000000 {
+							label = "U-Boot";
+							reg = <0x00000000 0x00500000>;
+							read-only;
+						};
+						partition@0500000 {
+							label = "kernel";
+							reg = <0x00500000 0x00500000>;
+						};
+						partition@0a00000 {
+							label = "uRamdisk";
+							reg = <0x00a00000 0x00500000>;
+							read-only;
+						};
+						partition@0f00000 {
+							label = "ubi";
+							reg = <0x00f00000 0x0b900000>;
+						};
+						partition@c800000 {
+							label = "Recovery";
+							reg = <0x0c800000 0x00f00000>;
+							read-only;
+						};
+						partition@d700000 {
+							label = "config";
+							reg = <0x0d700000 0x01400000>;
+						};
+						partition@eb00000 {
+							label = "reserve1";
+							reg = <0x0eb00000 0x00a00000>;
+							read-only;
+						};
+						partition@f500000 {
+							label = "reserve2";
+							reg = <0x0f500000 0x00a00000>;
+							read-only;
+						};
+					};
+				};
+			};
+
+			sdhci@d8000 {
+				status = "disabled";
+			};
+
+			usb3@f0000 {
+				usb-phy = <&usb3_0_phy>;
+				status = "okay";
+			};
+
+			usb3@f8000 {
+				usb-phy = <&usb3_1_phy>;
+				status = "okay";
+			};
+		};
+
+		bm-bppi {
+			status = "okay";
+		};
+	};
+
+	gpio-leds {
+		compatible = "gpio-leds";
+		pinctrl-names = "default";
+		pinctrl-0 = <&sata_leds>;
+
+		sata1-red-led {
+			label = "wdmcex2u:red:hdd1";
+			gpios = <&gpio1 11 GPIO_ACTIVE_HIGH>;
+		};
+		sata2-red-led {
+			label = "wdmcex2u:red:hdd2";
+			gpios = <&gpio1 20 GPIO_ACTIVE_HIGH>;
+		};
+		sata1-blue-led {
+			label = "wdmcex2u:blue:hdd1";
+			gpios = <&gpio1 21 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "default-on";
+		};
+		sata2-blue-led {
+			label = "wdmcex2u:blue:hdd2";
+			gpios = <&gpio1 22 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "default-on";
+		};
+	};
+
+	gpio-keys {
+		compatible = "gpio-keys";
+		pinctrl-names = "default";
+		pinctrl-0 = <&btn_pins>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		reset {
+			label = "reset";
+			linux,code = <KEY_RESTART>;
+			gpios = <&gpio1 18 GPIO_ACTIVE_LOW>;
+			debounce-interval = <60>;
+			wakeup-source;
+		};
+	};
+
+	usb3_0_phy: usb3_0_phy {
+		compatible = "usb-nop-xceiv";
+		vcc-supply = <&reg_usb3_0_vbus>;
+	};
+
+	usb3_1_phy: usb3_1_phy {
+		compatible = "usb-nop-xceiv";
+		vcc-supply = <&reg_usb3_1_vbus>;
+	};
+
+	reg_usb3_0_vbus: usb3-vbus0 {
+		compatible = "regulator-fixed";
+		regulator-name = "usb3-vbus0";
+		pinctrl-names = "default";
+		pinctrl-0 = <&xhci0_vbus_pins>;
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		enable-active-high;
+		regulator-always-on;
+		gpio = <&gpio0 26 GPIO_ACTIVE_HIGH>;
+	};
+
+	reg_usb3_1_vbus: usb3-vbus1 {
+		compatible = "regulator-fixed";
+		regulator-name = "usb3-vbus1";
+		pinctrl-names = "default";
+		pinctrl-0 = <&xhci1_vbus_pins>;
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		enable-active-high;
+		regulator-always-on;
+		gpio = <&gpio0 27 GPIO_ACTIVE_HIGH>;
+	};
+};
Signature · 1 line
2.21.0
#7 lore raw

Re: [PATCH 5/5] ARM: dts: armada385-wd-mcex2u: Add DTS file for WD My Cloud EX2 Ultra

Quoted 4 lines
arch/arm/boot/dts/armada-385-wd-mcex2u.dts new file +4 -0
+		internal-regs {
+			timer@c200 {
+				status = "disabled";
+			};

arch/arm/boot/dts/armada-385-wd-mcex2u.dts line 54

Quoted 10 lines
arch/arm/boot/dts/armada-385-wd-mcex2u.dts new file +10 -0
+
+			i2c0: i2c@11000 {
+				status = "okay";
+				clock-frequency = <100000>;
+			};
+
+			i2c1: i2c@11100 {
+				status = "okay";
+				clock-frequency = <100000>;
+			};

It is a bit unusual to enable i2c busses when there is nothing on them.

arch/arm/boot/dts/armada-385-wd-mcex2u.dts line 125

Quoted 2 lines
arch/arm/boot/dts/armada-385-wd-mcex2u.dts new file +2 -0
+			ethernet@34000 {
+				phy = <&phy0>;

phy has been deprecated. Please use phy-handle.

Andrew

#8 lore raw
In reply to Evgeny Kolesnikov
Quoted 9 lines · Evgeny Kolesnikov

This patchset consists of the DTS, which describes the WD MyCloud EX2 Ultra device, 'poweroff' and 'resert' drivers for power-managing MCUs connected to a board via UART (these drivers are more versatile than qnap-poweroff and could be used as a substitude), and DT bindings for these drivers.

The difference between uart-poweroff and qnap-poweroff is small, but important:
uart-poweroff is able to send to an MCU a command of arbitrary length, and the command itself is defined in a DTS file for a specific device/board, thus making this driver applicable to wider range of devices.

Hi Evgeny

There is a lot of replicated code here, and in the original qnap-poweroff.c driver. Please consolidate it by extending the current driver. It should be easy to add a new compatible string, and turn power_off_cfg.cmd into an array.

Andrew

#9 lore raw
Quoted 11 lines · Andrew Lunn
Quoted 5 lines · Evgeny Kolesnikov

The difference between uart-poweroff and qnap-poweroff is small, but important:
uart-poweroff is able to send to an MCU a command of arbitrary length, and the command itself is defined in a DTS file for a specific device/board, thus making this driver applicable to wider range of devices.

There is a lot of replicated code here, and in the original qnap-poweroff.c driver. Please consolidate it by extending the current driver. It should be easy to add a new compatible string, and turn power_off_cfg.cmd into an array.

Hi, Andrew.

I've considered extending qnap driver, but I have some doubts about this approach.

First of all there is only a poweroff counterpart. As there is no
qnap-restart driver, what should I do with uart-restart? Is it OK to have xxx-restart-poweroff driver (never saw anything like that)?

While I can add cmd as a parameter to qnap driver (having it converted
into an array) it should be optional as original qnap relies on two hardcoded values for its devices. And having a non-qnap device with this driver in DT without defined cmd would not make any sense. It feels kinda ugly.

Wouldn't it be more fitting to have these two generic drivers and then retire old qnap driver while moving everything that uses it to the new one?

Thanks for the review.

EK.

#10 lore raw
Quoted 13 lines · Evgeny Kolesnikov
Quoted 11 lines · Andrew Lunn
Quoted 5 lines · Evgeny Kolesnikov

The difference between uart-poweroff and qnap-poweroff is small, but important:
uart-poweroff is able to send to an MCU a command of arbitrary length, and the command itself is defined in a DTS file for a specific device/board, thus making this driver applicable to wider range of devices.

There is a lot of replicated code here, and in the original qnap-poweroff.c driver. Please consolidate it by extending the current driver. It should be easy to add a new compatible string, and turn power_off_cfg.cmd into an array.

Quoted 8 lines

Hi, Andrew.

I've considered extending qnap driver, but I have some doubts about this approach.

First of all there is only a poweroff counterpart. As there is no qnap-restart driver, what should I do with uart-restart? Is it OK to have xxx-restart-poweroff driver (never saw anything like that)?

Hi Evgeny

There are a few options. You can refactor all the code into a library and small drivers which wrap around the library. Or you can make the driver handle both, using the compatible string to determine which it should do.

Quoted 3 lines

While I can add cmd as a parameter to qnap driver (having it converted into an array) it should be optional as original qnap relies on two hardcoded values for its devices.

That is not what i meant. You can make the current code more generic by changing the single byte in power_off_cfg to an array. DT should describe the hardware, not bytes you poke into registers. So it is perfectly valid to have the bytes hard coded in the driver.

Andrew

#11 lore raw
In reply to Evgeny Kolesnikov
Quoted 5 lines · Evgeny Kolesnikov

This patchset consists of the DTS, which describes the WD MyCloud EX2 Ultra device, 'poweroff' and 'resert' drivers for power-managing MCUs connected to a board via UART (these drivers are more versatile than qnap-poweroff and could be used as a substitude), and DT bindings for these drivers.

Quoted 7 lines

The difference between uart-poweroff and qnap-poweroff is small, but important:
uart-poweroff is able to send to an MCU a command of arbitrary length, and the command itself is defined in a DTS file for a specific device/board, thus making this driver applicable to wider range of devices.

 drivers/power/reset/uart-poweroff.c           | 155 +++++++++
 drivers/power/reset/uart-restart.c            | 204 ++++++++++++

These twon really need to be combined somehow.

Plus... does it have mini-16550 driver in it? Should it use serdev instead?
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html