[RFC PATCH 0/3] provide imx rproc driver

[RFC PATCH 0/3] provide imx rproc driver

[Oleksij Rempel]

Hallo all,

this is RFC patchset to provide remoteproc functionality on
imx7d SoC.
Since current kernel do not have devicetrees for board which
I used for testing, this RFC patchset includes this too.

For testing I used this simple counter written in ASM:
======================================
        .syntax unified
        .text
        .thumb
        .int 0x10020000                 @ Initial SP value
        .int reset + 1

reset:

        mov     r0, #0x55
        ldr     r1, =(0x40)
1:
        str     r0, [r1]
        add     r0, 1
        b       1b

        /* Dummy data, required by remoteproc loader */
        /* Please FIXME, this part seem to be incorrect */
        .data
        .section .resource_table, "aw"
        .word   1, 0, 0, 0      /* struct resource_table base */
        .word   0               /* uint32_t offset[1] */
============================================================
compiled with:
${CROSS}as -o imx7m4.o imx7m4.S
${CROSS}ld -Ttext=0x0 -o imx7m4.elf imx7m4.o
cp imx7m4.elf /srv/nfs/sid-armhf/lib/firmware/rproc-imx_rproc-fw

Functionality was confirmed with current OpenOCD master.
OpenOCD cfg file can be found here:
https://github.com/olerem/openocd/blob/imx7-2017.06.14/tcl/target/imx7.cfg

Comment and suggestions are welcome.

Regards,
Oleksij

Oleksij Rempel (3):
  ARM: dts: imx7d: add imx7d-phyboard-zeta
  remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver
  ARM: dts: imx7s: add rproc node

 arch/arm/boot/dts/Makefile                |   1 +
 arch/arm/boot/dts/imx7d-pba-c-09.dtsi     | 272 ++++++++++++++++++++++++++++++
 arch/arm/boot/dts/imx7d-peb-av-02.dtsi    | 104 ++++++++++++
 arch/arm/boot/dts/imx7d-peb-eval-02.dtsi  | 130 ++++++++++++++
 arch/arm/boot/dts/imx7d-phyboard-zeta.dts | 144 ++++++++++++++++
 arch/arm/boot/dts/imx7d-phycore-som.dtsi  | 272 ++++++++++++++++++++++++++++++
 arch/arm/boot/dts/imx7d-pinfunc-lpsr.h    |  76 +++++++++
 arch/arm/boot/dts/imx7s.dtsi              |   9 +
 drivers/remoteproc/Kconfig                |   8 +
 drivers/remoteproc/Makefile               |   1 +
 drivers/remoteproc/imx_rproc.c            | 264 +++++++++++++++++++++++++++++
 11 files changed, 1281 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx7d-pba-c-09.dtsi
 create mode 100644 arch/arm/boot/dts/imx7d-peb-av-02.dtsi
 create mode 100644 arch/arm/boot/dts/imx7d-peb-eval-02.dtsi
 create mode 100644 arch/arm/boot/dts/imx7d-phyboard-zeta.dts
 create mode 100644 arch/arm/boot/dts/imx7d-phycore-som.dtsi
 create mode 100644 arch/arm/boot/dts/imx7d-pinfunc-lpsr.h
 create mode 100644 drivers/remoteproc/imx_rproc.c

-- 
2.11.0

[RFC PATCH 1/3] ARM: dts: imx7d: add imx7d-phyboard-zeta

[Oleksij Rempel]

From: Oleksij Rempel <linux@rempel-privat.de>

Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
---
 arch/arm/boot/dts/Makefile                |   1 +
 arch/arm/boot/dts/imx7d-pba-c-09.dtsi     | 272 ++++++++++++++++++++++++++++++
 arch/arm/boot/dts/imx7d-peb-av-02.dtsi    | 104 ++++++++++++
 arch/arm/boot/dts/imx7d-peb-eval-02.dtsi  | 130 ++++++++++++++
 arch/arm/boot/dts/imx7d-phyboard-zeta.dts | 144 ++++++++++++++++
 arch/arm/boot/dts/imx7d-phycore-som.dtsi  | 272 ++++++++++++++++++++++++++++++
 arch/arm/boot/dts/imx7d-pinfunc-lpsr.h    |  76 +++++++++
 arch/arm/boot/dts/imx7s.dtsi              |   1 +
 8 files changed, 1000 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx7d-pba-c-09.dtsi
 create mode 100644 arch/arm/boot/dts/imx7d-peb-av-02.dtsi
 create mode 100644 arch/arm/boot/dts/imx7d-peb-eval-02.dtsi
 create mode 100644 arch/arm/boot/dts/imx7d-phyboard-zeta.dts
 create mode 100644 arch/arm/boot/dts/imx7d-phycore-som.dtsi
 create mode 100644 arch/arm/boot/dts/imx7d-pinfunc-lpsr.h

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 9c5e1d944d1c..cdd4bdb3a7c2 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -476,6 +476,7 @@ dtb-$(CONFIG_SOC_IMX7D) += \
 	imx7d-cl-som-imx7.dtb \
 	imx7d-colibri-eval-v3.dtb \
 	imx7d-nitrogen7.dtb \
+	imx7d-phyboard-zeta.dtb \
 	imx7d-sbc-imx7.dtb \
 	imx7d-sdb.dtb \
 	imx7d-sdb-sht11.dtb \
diff --git a/arch/arm/boot/dts/imx7d-pba-c-09.dtsi b/arch/arm/boot/dts/imx7d-pba-c-09.dtsi
new file mode 100644
index 000000000000..4150c16684b7
--- /dev/null
+++ b/arch/arm/boot/dts/imx7d-pba-c-09.dtsi
@@ -0,0 +1,272 @@
+/*
+ * Copyright (C) 2015 PHYTEC America, LLC
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/ {
+	model = "Phytec i.MX7 phyBOARD-Zeta";
+	compatible = "phytec,imx7d-pba-c-09", "phytec,imx7d-phycore-som", "fsl,imx7d";
+
+	regulators {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		reg_usb_otg1_vbus: regulator@0 {
+			compatible = "regulator-fixed";
+			reg = <0>;
+			regulator-name = "usb_otg1_vbus";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			gpio = <&gpio1 5 GPIO_ACTIVE_HIGH>;
+			enable-active-high;
+		};
+
+		reg_usb_otg2_vbus: regulator@1 {
+			compatible = "regulator-fixed";
+			reg = <1>;
+			regulator-name = "usb_otg2_vbus";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			gpio = <&gpio4 7 GPIO_ACTIVE_HIGH>;
+			enable-active-high;
+		};
+
+		/* Enable if R9 is populated. Conflicts with userbtn2 on PEB-EVAL-02 */
+		/*
+		reg_can1_3v3: regulator@2 {
+			compatible = "regulator-fixed";
+			reg = <2>;
+			regulator-name = "can1-3v3";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			gpio = <&gpio5 2 GPIO_ACTIVE_HIGH>;
+			enable-active-high;
+		};
+		*/
+	};
+};
+
+&iomuxc {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_hog_1 &pinctrl_hog_2 &pinctrl_hog_lcd>;
+
+	pinctrl_hog_2: hoggrp-2 {
+		fsl,pins = <
+			MX7D_PAD_SD1_CD_B__GPIO5_IO0		0x59 /* SD1 CD */
+			MX7D_PAD_SD1_WP__GPIO5_IO1		0x59 /* PCIe Disable */
+			MX7D_PAD_SAI1_RX_BCLK__GPIO6_IO17	0x59 /* PCIe Reset */
+			MX7D_PAD_UART3_CTS_B__GPIO4_IO7		0x14 /* USB2 pwr */
+			MX7D_PAD_GPIO1_IO09__GPIO1_IO9 		0x59 /* ETH2 Int_N */
+			MX7D_PAD_EPDC_PWR_COM__GPIO2_IO30	0x59 /* ETH2 Reset_n */
+			MX7D_PAD_EPDC_DATA10__GPIO2_IO10	0x59 /* User Button */
+			MX7D_PAD_EPDC_DATA13__GPIO2_IO13	0x39 /* Boot Circuit Buffer Enable
+									5K pull-up  */
+		>;
+	};
+
+	pinctrl_usdhc1: usdhc1grp {
+		fsl,pins = <
+			MX7D_PAD_SD1_CMD__SD1_CMD		0x59
+			MX7D_PAD_SD1_CLK__SD1_CLK		0x19
+			MX7D_PAD_SD1_DATA0__SD1_DATA0		0x59
+			MX7D_PAD_SD1_DATA1__SD1_DATA1		0x59
+			MX7D_PAD_SD1_DATA2__SD1_DATA2		0x59
+			MX7D_PAD_SD1_DATA3__SD1_DATA3		0x59
+		>;
+	};
+
+	pinctrl_flexcan1: flexcan1grp {
+		fsl,pins = <
+			MX7D_PAD_GPIO1_IO12__FLEXCAN1_RX	0x59
+			MX7D_PAD_GPIO1_IO13__FLEXCAN1_TX	0x59
+		>;
+	};
+
+	pinctrl_enet2: enet2grp {
+		fsl,pins = <
+			MX7D_PAD_EPDC_SDCE0__ENET2_RGMII_RX_CTL	0x5
+			MX7D_PAD_EPDC_SDCE1__ENET2_RGMII_RXC 	0x5
+			MX7D_PAD_EPDC_SDCLK__ENET2_RGMII_RD0	0x5
+			MX7D_PAD_EPDC_SDLE__ENET2_RGMII_RD1	0x5
+			MX7D_PAD_EPDC_SDOE__ENET2_RGMII_RD2	0x5
+			MX7D_PAD_EPDC_SDSHR__ENET2_RGMII_RD3	0x5
+			MX7D_PAD_EPDC_SDCE2__ENET2_RGMII_TD0	0x5
+			MX7D_PAD_EPDC_SDCE3__ENET2_RGMII_TD1	0x5
+			MX7D_PAD_EPDC_GDCLK__ENET2_RGMII_TD2	0x5
+			MX7D_PAD_EPDC_GDOE__ENET2_RGMII_TD3	0x5
+			MX7D_PAD_EPDC_GDRL__ENET2_RGMII_TX_CTL	0x5
+			MX7D_PAD_EPDC_GDSP__ENET2_RGMII_TXC	0x5
+		>;
+	};
+};
+
+&iomuxc_lpsr {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_hog_lpsr_1 &pinctrl_hog_lpsr_lcd>;
+
+	pinctrl_hog_lpsr_1: hoggrp-lpsr_1 {
+		fsl,pins = <
+			MX7D_PAD_GPIO1_IO05__GPIO1_IO5		0x14 /* USB1 pwr */
+		>;
+	};
+
+	pinctrl_uart5: uart5grp {
+		fsl,pins = <
+			MX7D_PAD_GPIO1_IO06__UART5_DCE_RX	0x79
+			MX7D_PAD_GPIO1_IO07__UART5_DCE_TX	0x79
+		>;
+	};
+
+	pinctrl_wdog: wdoggrp {
+		fsl,pins = <
+			MX7D_PAD_GPIO1_IO00__WDOD1_WDOG_B	0x74
+		>;
+	};
+};
+
+&uart5 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart5>;
+	assigned-clocks = <&clks IMX7D_UART5_ROOT_SRC>;
+	assigned-clock-parents = <&clks IMX7D_OSC_24M_CLK>;
+	status = "disabled";
+};
+
+&usdhc1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_usdhc1>;
+	cd-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+	no-1-8-v; /* Fixed voltage supply, doesn't support vsel */
+	enable-sdio-wakeup;
+	keep-power-in-suspend;
+	status = "disabled";
+};
+
+&flexcan1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_flexcan1>;
+	/* Enable the following if SD1_RESET_B is used to enable/disable CAN xceiver
+	 *  xceiver-supply = <&reg_can1_3v3>;
+	 */
+	status = "disabled";
+};
+
+&fec2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_enet2>;
+	assigned-clocks = <&clks IMX7D_ENET2_TIME_ROOT_SRC>,
+			<&clks IMX7D_ENET2_TIME_ROOT_CLK>;
+	assigned-clock-parents = <&clks IMX7D_PLL_ENET_MAIN_100M_CLK>;
+	assigned-clock-rates = <0>, <100000000>;
+	phy-mode = "rgmii";
+	phy-handle = <&ethphy1>;
+	fsl,magic-packet;
+	phy-reset-gpios = <&gpio2 30 GPIO_ACTIVE_LOW>;
+	status = "disabled";
+};
+
+/* same MDIO bus as PHY on phyCORE SOM */
+&mdio {
+	ethphy1: ethernet-phy@2 {
+		compatible = "ethernet-phy-ieee802.3-c22";
+		interrupt-parent = <&gpio1>;
+		interrupts = <9 0>;
+		reg = <2>;
+		rxdv-skew-ps = <0>;
+		txen-skew-ps = <0>;
+		rxd0-skew-ps = <0>;
+		rxd1-skew-ps = <0>;
+		rxd2-skew-ps = <0>;
+		rxd3-skew-ps = <0>;
+		rxc-skew-ps = <1860>;
+		txc-skew-ps = <1860>;
+	};
+};
+
+&usbotg1 {
+	vbus-supply = <&reg_usb_otg1_vbus>;
+	dr_mode = "host";
+	status = "disabled";
+};
+
+&usbotg2 {
+	vbus-supply = <&reg_usb_otg2_vbus>;
+	status = "disabled";
+};
+
+&wdog1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdog>;
+	fsl,wdog_b;
+};
+
+/* DTS pinmuxing and bindings for LCD adapter PEB-AV-02 */
+
+&iomuxc {
+	pinctrl_hog_lcd: hog_lcdgrp {
+		fsl,pins = <
+			MX7D_PAD_LCD_RESET__GPIO3_IO4		0x79
+		>;
+	};
+
+	pinctrl_i2c2: i2c2grp {
+		fsl,pins = <
+			MX7D_PAD_I2C2_SCL__I2C2_SCL		0x4000007f
+			MX7D_PAD_I2C2_SDA__I2C2_SDA		0x4000007f
+		>;
+	};
+
+	pinctrl_edt_ts_irq: tsirqgrp {
+		fsl,pins = <
+			MX7D_PAD_EPDC_DATA14__GPIO2_IO14	0x59
+		>;
+	};
+};
+
+&iomuxc_lpsr {
+	pinctrl_pwm3: pwmgrp {
+		fsl,pins = <
+			MX7D_PAD_GPIO1_IO03__PWM3_OUT		0x30
+		>;
+	};
+
+	pinctrl_hog_lpsr_lcd: hoggrp_lpsr_lcd {
+		fsl,pins = <
+			MX7D_PAD_GPIO1_IO01__GPIO1_IO1		0x59
+		>;
+	};
+};
+
+&i2c2 {
+	clock-frequency = <400000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c2>;
+	status = "disabled";
+
+	ft5406: ft5406@38 {
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_edt_ts_irq>;
+		interrupt-parent = <&gpio2>;
+		interrupts = <14 0>;
+		reset-gpios = <&gpio3 4 GPIO_ACTIVE_LOW>;
+		status = "disabled";
+	};
+};
+
+#include "imx7d-peb-av-02.dtsi"
+
+&pwm3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_pwm3>;
+	status = "disabled";
+};
+
+&backlight {
+	pwms = <&pwm3 0 5000000>;
+	enable-gpios = <&gpio1 1 0>;
+	status = "disabled";
+};
diff --git a/arch/arm/boot/dts/imx7d-peb-av-02.dtsi b/arch/arm/boot/dts/imx7d-peb-av-02.dtsi
new file mode 100644
index 000000000000..dcf117c71a92
--- /dev/null
+++ b/arch/arm/boot/dts/imx7d-peb-av-02.dtsi
@@ -0,0 +1,104 @@
+/*
+ * Copyright (C) 2015 PHYTEC America, LLC
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/ {
+	backlight: backlight {
+		compatible = "pwm-backlight";
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <6>;
+		power-supply = <&lcd_3v3>;
+		status = "disabled";
+	};
+
+	lcd_3v3: fixedregulator-lcd {
+		compatible = "regulator-fixed";
+		regulator-name = "lcd_3v3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		regulator-boot-on;
+	};
+};
+
+&iomuxc {
+	pinctrl_lcdif_ctrl: lcdifctrlgrp {
+		fsl,pins = <
+			MX7D_PAD_LCD_CLK__LCD_CLK	0x7e
+			MX7D_PAD_LCD_ENABLE__LCD_ENABLE	0x7e
+			MX7D_PAD_LCD_HSYNC__LCD_HSYNC	0x7e
+			MX7D_PAD_LCD_VSYNC__LCD_VSYNC	0x7e
+		>;
+	};
+
+	pinctrl_lcdif_dat: lcdifdatgrp {
+		fsl,pins = <
+			MX7D_PAD_LCD_DATA00__LCD_DATA0	0x7e
+			MX7D_PAD_LCD_DATA01__LCD_DATA1	0x7e
+			MX7D_PAD_LCD_DATA02__LCD_DATA2	0x7e
+			MX7D_PAD_LCD_DATA03__LCD_DATA3	0x7e
+			MX7D_PAD_LCD_DATA04__LCD_DATA4	0x7e
+			MX7D_PAD_LCD_DATA05__LCD_DATA5	0x7e
+			MX7D_PAD_LCD_DATA06__LCD_DATA6	0x7e
+			MX7D_PAD_LCD_DATA07__LCD_DATA7	0x7e
+			MX7D_PAD_LCD_DATA08__LCD_DATA8	0x7e
+			MX7D_PAD_LCD_DATA09__LCD_DATA9	0x7e
+			MX7D_PAD_LCD_DATA10__LCD_DATA10	0x7e
+			MX7D_PAD_LCD_DATA11__LCD_DATA11	0x7e
+			MX7D_PAD_LCD_DATA12__LCD_DATA12	0x7e
+			MX7D_PAD_LCD_DATA13__LCD_DATA13	0x7e
+			MX7D_PAD_LCD_DATA14__LCD_DATA14	0x7e
+			MX7D_PAD_LCD_DATA15__LCD_DATA15	0x7e
+			MX7D_PAD_LCD_DATA16__LCD_DATA16	0x7e
+			MX7D_PAD_LCD_DATA17__LCD_DATA17	0x7e
+			MX7D_PAD_LCD_DATA18__LCD_DATA18	0x7e
+			MX7D_PAD_LCD_DATA19__LCD_DATA19	0x7e
+			MX7D_PAD_LCD_DATA20__LCD_DATA20	0x7e
+			MX7D_PAD_LCD_DATA21__LCD_DATA21	0x7e
+			MX7D_PAD_LCD_DATA22__LCD_DATA22	0x7e
+			MX7D_PAD_LCD_DATA23__LCD_DATA23	0x7e
+		>;
+	};
+};
+
+&lcdif {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_lcdif_dat
+		&pinctrl_lcdif_ctrl>;
+	display = <&display0>;
+	lcd-supply = <&lcd_3v3>;
+	status = "disabled";
+
+	display0: display {
+		bits-per-pixel = <32>;
+		bus-width = <24>;
+
+		display-timings {
+			native-mode = <&timing0>;
+			timing0: ETM0700G0DH6 {
+				clock-frequency = <33000000>;
+				hactive = <800>;
+				vactive = <480>;
+				hfront-porch = <40>;
+				hback-porch = <216>;
+				hsync-len = <128>;
+				vback-porch = <35>;
+				vfront-porch = <10>;
+				vsync-len = <2>;
+				hsync-active = <0>;
+				vsync-active = <0>;
+				de-active = <1>;
+				pixelclk-active = <1>;
+			};
+		};
+	};
+};
+
+&ft5406 {
+	compatible = "edt,edt-ft5406", "edt,edt-ft5x06";
+	reg = <0x38>;
+	status = "disabled";
+};
\ No newline at end of file
diff --git a/arch/arm/boot/dts/imx7d-peb-eval-02.dtsi b/arch/arm/boot/dts/imx7d-peb-eval-02.dtsi
new file mode 100644
index 000000000000..8bde5b13e702
--- /dev/null
+++ b/arch/arm/boot/dts/imx7d-peb-eval-02.dtsi
@@ -0,0 +1,130 @@
+/*
+ * Copyright (C) 2015 PHYTEC America, LLC
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/ {
+	phytec_leds: leds {
+		compatible = "gpio-leds";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_leds_eval>;
+		status = "disabled";
+
+		led@0 {
+			label = "eval_led_1";
+			gpios = <&gpio2 8 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "gpio";
+			default-state = "on";
+		};
+
+		led@1 {
+			label = "eval_led_2";
+			gpios = <&gpio4 4 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "gpio";
+			default-state = "on";
+		};
+
+		led@2 {
+			label = "eval_led_3";
+			gpios = <&gpio2 15 GPIO_ACTIVE_HIGH>;
+			linux,default-trigger = "gpio";
+			default-state = "on";
+		};
+	};
+
+	phytec_buttons: gpio-keys {
+		compatible = "gpio-keys";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_btns_eval>;
+		status = "disabled";
+
+		userbtn@0 {
+			label = "eval_button_1";
+			gpios = <&gpio2 9 GPIO_ACTIVE_HIGH>;
+			linux,code = <0x100>;	/* BTN_MISC */
+		};
+		userbtn@1 {
+			label = "eval_button_2";
+			gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>;
+			linux,code = <0x100>;   /* BTN_MISC */
+		};
+
+		userbtn@2 {
+			label = "eval_button_3";
+			gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>;
+			linux,code = <0x100>;	/* BTN_MISC */
+		};
+	};
+};
+
+&iomuxc {
+	pinctrl_i2c4: i2c4grp {
+		fsl,pins = <
+			MX7D_PAD_I2C4_SCL__I2C4_SCL		0x4000007f
+			MX7D_PAD_I2C4_SDA__I2C4_SDA		0x4000007f
+		>;
+	};
+
+	pinctrl_leds_eval: leds_evalgrp {
+		fsl,pins = <
+			MX7D_PAD_EPDC_DATA08__GPIO2_IO8		0x79	/* Labeled UART6_RX on schematic */
+			MX7D_PAD_UART3_RX_DATA__GPIO4_IO4	0x79
+			MX7D_PAD_EPDC_DATA15__GPIO2_IO15	0x79	/* Labeled EXP_CONN_MUX5 on schematic */
+		>;
+	};
+
+	pinctrl_btns_eval: btns_evalgrp {
+		fsl,pins = <
+			MX7D_PAD_EPDC_DATA09__GPIO2_IO9		0x79	/* Labeled UART6_TX on schematic */
+			MX7D_PAD_UART3_TX_DATA__GPIO4_IO5	0x79
+			MX7D_PAD_SD1_RESET_B__GPIO5_IO2		0x79	/* Labeled EXP_CONN_MUX3 on schematic */
+		>;
+	};
+
+	pinctrl_uart1: uart1grp {
+		fsl,pins = <
+			MX7D_PAD_UART1_RX_DATA__UART1_DCE_RX	0x79
+			MX7D_PAD_UART1_TX_DATA__UART1_DCE_TX	0x79
+		>;
+	};
+
+	pinctrl_uart2: uart2grp {
+		fsl,pins = <
+			MX7D_PAD_UART2_RX_DATA__UART2_DCE_RX	0x79
+			MX7D_PAD_UART2_TX_DATA__UART2_DCE_TX	0x79
+		>;
+	};
+};
+
+&i2c4 {
+	clock-frequency = <400000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c4>;
+	status = "disabled";
+
+	i2c4_eeprom: eeprom@56 {
+		compatible = "onnn,24c32";
+		reg = <0x56>;
+		pagesize = <32>;
+		status = "disabled";
+	};
+};
+
+&uart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart1>;
+	assigned-clocks = <&clks IMX7D_UART1_ROOT_SRC>;
+	assigned-clock-parents = <&clks IMX7D_PLL_SYS_MAIN_240M_CLK>;
+	status = "disabled";
+};
+
+&uart2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart2>;
+	assigned-clocks = <&clks IMX7D_UART2_ROOT_SRC>;
+	assigned-clock-parents = <&clks IMX7D_PLL_SYS_MAIN_240M_CLK>;
+	status = "disabled";
+};
\ No newline at end of file
diff --git a/arch/arm/boot/dts/imx7d-phyboard-zeta.dts b/arch/arm/boot/dts/imx7d-phyboard-zeta.dts
new file mode 100644
index 000000000000..16cf12244f16
--- /dev/null
+++ b/arch/arm/boot/dts/imx7d-phyboard-zeta.dts
@@ -0,0 +1,144 @@
+/*
+ * Copyright (C) 2015 PHYTEC America, LLC
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+/dts-v1/;
+
+#include "imx7d-phycore-som.dtsi"
+#include "imx7d-pba-c-09.dtsi"
+#include "imx7d-peb-eval-02.dtsi"
+
+#include "imx7s.dtsi"
+
+/ {
+	chosen {
+		stdout-path = &uart5;
+
+		environment@0 {
+			compatible = "barebox,environment";
+			device-path = &bareboxenv;
+		};
+	};
+
+	memory {
+		device_type = "memory";
+		reg = <0x80000000 0x40000000>;
+	};
+};
+
+/**** SOM - PCM-061 ****/
+
+&fec1 {
+	status = "okay";
+};
+
+/* eMMC */
+&usdhc3 {
+	status = "okay";
+
+	boot0-partitions {
+		compatible = "fixed-partitions";
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		barebox@0 {
+			label = "barebox";
+			reg = <0x0 0x300000>;
+		};
+
+		bareboxenv: bareboxenv@300000 {
+			label = "bareboxenv";
+			reg = <0x300000 0x0>;
+		};
+	};
+};
+
+&i2c_eeprom {
+	status = "okay";
+};
+
+&i2c_rtc {
+	status = "okay";
+};
+
+/**** Carrier Board - PBA-C-09 ****/
+
+&uart5 {
+	status = "okay";
+};
+
+&usdhc1 {
+	status = "okay";
+};
+
+&fec2 {
+	status = "okay";
+};
+
+/* Host mode */
+&usbotg1 {
+	status = "okay";
+};
+
+/* OTG mode */
+&usbotg2 {
+	status = "okay";
+};
+
+&flexcan1 {
+	status = "okay";
+};
+
+&wdog1 {
+	status = "okay";
+};
+
+/**** PEB-AV-02: touch controller ft5406, LCD and PWM backlight control ****/
+&i2c2 {
+	status = "okay";
+};
+
+&ft5406 {
+	status = "okay";
+};
+
+&lcdif {
+	status = "okay";
+};
+
+&pwm3 {
+	status = "okay";
+};
+
+&backlight {
+	status = "okay";
+};
+
+/**** Interfaces on PEB-EVAL-02 ****/
+
+&i2c4 {
+	status = "okay";
+};
+
+&i2c4_eeprom {
+	status = "okay";
+};
+
+&phytec_leds {
+	status = "okay";
+};
+
+&phytec_buttons {
+	status = "okay";
+};
+
+&uart1 {
+	status = "okay";
+};
+
+&uart2 {
+	status = "okay";
+};
\ No newline at end of file
diff --git a/arch/arm/boot/dts/imx7d-phycore-som.dtsi b/arch/arm/boot/dts/imx7d-phycore-som.dtsi
new file mode 100644
index 000000000000..ea8c801f3852
--- /dev/null
+++ b/arch/arm/boot/dts/imx7d-phycore-som.dtsi
@@ -0,0 +1,272 @@
+/*
+ * Copyright (C) 2015 PHYTEC America, LLC
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <dt-bindings/input/input.h>
+#include <arm/imx7d.dtsi>
+
+/ {
+	model = "Phytec i.MX7D phyCORE";
+	compatible = "phytec,imx7d-phycore-som", "fsl,imx7d";
+
+	memory {
+		reg = <0x80000000 0x80000000>;
+	};
+};
+
+&cpu0 {
+	arm-supply = <&sw1a_reg>;
+};
+
+&i2c1 {
+	clock-frequency = <400000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c1>;
+	status = "okay";
+
+	pmic: pfuze3000@08 {
+		compatible = "fsl,pfuze3000";
+		reg = <0x08>;
+
+		regulators {
+			sw1a_reg: sw1a {
+				regulator-min-microvolt = <700000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-ramp-delay = <6250>;
+			};
+
+			/* use sw1c_reg to align with pfuze100/pfuze200 */
+			sw1c_reg: sw1b {
+				regulator-min-microvolt = <700000>;
+				regulator-max-microvolt = <1475000>;
+				regulator-boot-on;
+				regulator-always-on;
+				regulator-ramp-delay = <6250>;
+			};
+
+			sw2_reg: sw2 {
+				regulator-min-microvolt = <1500000>;
+				regulator-max-microvolt = <1850000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			sw3a_reg: sw3 {
+				regulator-min-microvolt = <900000>;
+				regulator-max-microvolt = <1650000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			swbst_reg: swbst {
+				regulator-min-microvolt = <5000000>;
+				regulator-max-microvolt = <5150000>;
+			};
+
+			snvs_reg: vsnvs {
+				regulator-min-microvolt = <1000000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			vref_reg: vrefddr {
+				regulator-boot-on;
+				regulator-always-on;
+			};
+
+			vgen1_reg: vldo1 {
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+
+			vgen2_reg: vldo2 {
+				regulator-min-microvolt = <800000>;
+				regulator-max-microvolt = <1550000>;
+				regulator-always-on;
+			};
+
+			vgen3_reg: vccsd {
+				regulator-min-microvolt = <2850000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+
+			vgen4_reg: v33 {
+				regulator-min-microvolt = <2850000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+
+			vgen5_reg: vldo3 {
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+
+			vgen6_reg: vldo4 {
+				regulator-min-microvolt = <1800000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-always-on;
+			};
+		};
+	};
+	i2c_eeprom: eeprom@50 {
+		compatible = "atmel,24c32";
+		pagesize = <32>;
+		reg = <0x50>;
+		status = "disabled";
+	};
+
+	i2c_rtc: rtc@68 {
+		compatible = "mc,rv4162";
+		reg=<0x68>;
+		status = "disabled";
+	};
+};
+
+&iomuxc {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_hog_1>;
+
+	pinctrl_hog_1: hoggrp-1 {
+		fsl,pins = <
+			MX7D_PAD_GPIO1_IO08__SD1_VSELECT	0x59 /* PMIC VSELECT */
+			MX7D_PAD_EPDC_BDR0__GPIO2_IO28		0x59 /* ENET1_RESET_B */
+			MX7D_PAD_EPDC_BDR1__GPIO2_IO29		0x59 /* ENET1_INT_B */
+		>;
+	};
+
+	pinctrl_enet1: enet1grp {
+		fsl,pins = <
+			MX7D_PAD_GPIO1_IO10__ENET1_MDIO			0x7
+			MX7D_PAD_GPIO1_IO11__ENET1_MDC			0x7
+			MX7D_PAD_ENET1_RGMII_TXC__ENET1_RGMII_TXC	0x5
+			MX7D_PAD_ENET1_RGMII_TD0__ENET1_RGMII_TD0	0x5
+			MX7D_PAD_ENET1_RGMII_TD1__ENET1_RGMII_TD1	0x5
+			MX7D_PAD_ENET1_RGMII_TD2__ENET1_RGMII_TD2	0x5
+			MX7D_PAD_ENET1_RGMII_TD3__ENET1_RGMII_TD3	0x5
+			MX7D_PAD_ENET1_RGMII_TX_CTL__ENET1_RGMII_TX_CTL	0x5
+			MX7D_PAD_ENET1_RGMII_RXC__ENET1_RGMII_RXC	0x5
+			MX7D_PAD_ENET1_RGMII_RD0__ENET1_RGMII_RD0	0x5
+			MX7D_PAD_ENET1_RGMII_RD1__ENET1_RGMII_RD1	0x5
+			MX7D_PAD_ENET1_RGMII_RD2__ENET1_RGMII_RD2	0x5
+			MX7D_PAD_ENET1_RGMII_RD3__ENET1_RGMII_RD3	0x5
+			MX7D_PAD_ENET1_RGMII_RX_CTL__ENET1_RGMII_RX_CTL	0x5
+		>;
+	};
+
+	pinctrl_i2c1: i2c1grp {
+		fsl,pins = <
+			MX7D_PAD_I2C1_SDA__I2C1_SDA	0x4000007f
+			MX7D_PAD_I2C1_SCL__I2C1_SCL	0x4000007f
+		>;
+	};
+
+	pinctrl_usdhc3: usdhc3grp {
+		fsl,pins = <
+			MX7D_PAD_SD3_CMD__SD3_CMD	0x5d
+			MX7D_PAD_SD3_CLK__SD3_CLK	0x1d
+			MX7D_PAD_SD3_DATA0__SD3_DATA0	0x5d
+			MX7D_PAD_SD3_DATA1__SD3_DATA1	0x5d
+			MX7D_PAD_SD3_DATA2__SD3_DATA2	0x5d
+			MX7D_PAD_SD3_DATA3__SD3_DATA3	0x5d
+			MX7D_PAD_SD3_DATA4__SD3_DATA4	0x5d
+			MX7D_PAD_SD3_DATA5__SD3_DATA5	0x5d
+			MX7D_PAD_SD3_DATA6__SD3_DATA6	0x5d
+			MX7D_PAD_SD3_DATA7__SD3_DATA7	0x5d
+		>;
+	};
+
+	pinctrl_usdhc3_100mhz: usdhc3grp_100mhz {
+		fsl,pins = <
+			MX7D_PAD_SD3_CMD__SD3_CMD	0x5e
+			MX7D_PAD_SD3_CLK__SD3_CLK	0x1e
+			MX7D_PAD_SD3_DATA0__SD3_DATA0	0x5e
+			MX7D_PAD_SD3_DATA1__SD3_DATA1	0x5e
+			MX7D_PAD_SD3_DATA2__SD3_DATA2	0x5e
+			MX7D_PAD_SD3_DATA3__SD3_DATA3	0x5e
+			MX7D_PAD_SD3_DATA4__SD3_DATA4	0x5e
+			MX7D_PAD_SD3_DATA5__SD3_DATA5	0x5e
+			MX7D_PAD_SD3_DATA6__SD3_DATA6	0x5e
+			MX7D_PAD_SD3_DATA7__SD3_DATA7	0x5e
+		>;
+	};
+
+	pinctrl_usdhc3_200mhz: usdhc3grp_200mhz {
+		fsl,pins = <
+			MX7D_PAD_SD3_CMD__SD3_CMD	0x5f
+			MX7D_PAD_SD3_CLK__SD3_CLK	0x1f
+			MX7D_PAD_SD3_DATA0__SD3_DATA0	0x5f
+			MX7D_PAD_SD3_DATA1__SD3_DATA1	0x5f
+			MX7D_PAD_SD3_DATA2__SD3_DATA2	0x5f
+			MX7D_PAD_SD3_DATA3__SD3_DATA3	0x5f
+			MX7D_PAD_SD3_DATA4__SD3_DATA4	0x5f
+			MX7D_PAD_SD3_DATA5__SD3_DATA5	0x5f
+			MX7D_PAD_SD3_DATA6__SD3_DATA6	0x5f
+			MX7D_PAD_SD3_DATA7__SD3_DATA7	0x5f
+		>;
+	};
+
+	pinctrl_qspi1_1: qspi1grp_1 {
+		fsl,pins = <
+			MX7D_PAD_EPDC_DATA00__QSPI_A_DATA0	0x51
+			MX7D_PAD_EPDC_DATA01__QSPI_A_DATA1	0x51
+			MX7D_PAD_EPDC_DATA02__QSPI_A_DATA2	0x51
+			MX7D_PAD_EPDC_DATA03__QSPI_A_DATA3	0x51
+			MX7D_PAD_EPDC_DATA05__QSPI_A_SCLK	0x51
+			MX7D_PAD_EPDC_DATA06__QSPI_A_SS0_B	0x51
+		>;
+	};
+};
+
+&sdma {
+	status = "okay";
+};
+
+&fec1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_enet1>;
+	assigned-clocks = <&clks IMX7D_ENET1_TIME_ROOT_SRC>,
+			<&clks IMX7D_ENET1_TIME_ROOT_CLK>;
+	assigned-clock-parents = <&clks IMX7D_PLL_ENET_MAIN_100M_CLK>;
+	assigned-clock-rates = <0>, <100000000>;
+	phy-mode = "rgmii";
+	phy-handle = <&ethphy0>;
+	fsl,magic-packet;
+	phy-reset-gpios = <&gpio2 28 GPIO_ACTIVE_LOW>;
+	status = "disabled";
+
+	mdio: mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		/*ETH1 PHY on SOM, 25MHz crystal */
+		ethphy0: ethernet-phy@1 {
+			compatible = "ethernet-phy-ieee802.3-c22";
+			interrupt-parent = <&gpio2>;
+			interrupts = <29 0>;
+			reg = <1>;
+		};
+	};
+};
+
+&usdhc3 {
+	pinctrl-names = "default", "state_100mhz", "state_200mhz";
+	pinctrl-0 = <&pinctrl_usdhc3>;
+	pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
+	pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
+	assigned-clocks = <&clks IMX7D_USDHC3_ROOT_CLK>;
+	assigned-clock-rates = <400000000>;
+	bus-width = <8>;
+	tuning-step = <2>;
+	non-removable;
+	status = "disabled";
+};
\ No newline at end of file
diff --git a/arch/arm/boot/dts/imx7d-pinfunc-lpsr.h b/arch/arm/boot/dts/imx7d-pinfunc-lpsr.h
new file mode 100644
index 000000000000..378694ee05c2
--- /dev/null
+++ b/arch/arm/boot/dts/imx7d-pinfunc-lpsr.h
@@ -0,0 +1,76 @@
+/*
+ * Copyright (C) 2016 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __DTS_IMX7D_PINFUNC_LPSR_H
+#define __DTS_IMX7D_PINFUNC_LPSR_H
+
+/*
+ * The pin function ID is a tuple of
+ * <mux_reg conf_reg input_reg mux_mode input_val>
+ *
+ * NOTE: imx7d-lpsr pin groups should be put under &iomuxc_lpsr node when used
+ */
+
+#define MX7D_PAD_GPIO1_IO00__GPIO1_IO0				  0x0000 0x0030 0x0000 0x0 0x0
+#define MX7D_PAD_GPIO1_IO00__PWM4_OUT				  0x0000 0x0030 0x0000 0x1 0x0
+#define MX7D_PAD_GPIO1_IO00__WDOD1_WDOG_ANY			  0x0000 0x0030 0x0000 0x2 0x0
+#define MX7D_PAD_GPIO1_IO00__WDOD1_WDOG_B			  0x0000 0x0030 0x0000 0x3 0x0
+#define MX7D_PAD_GPIO1_IO00__WDOD1_WDOG__RST_B_DEB		  0x0000 0x0030 0x0000 0x4 0x0
+#define MX7D_PAD_GPIO1_IO01__GPIO1_IO1				  0x0004 0x0034 0x0000 0x0 0x0
+#define MX7D_PAD_GPIO1_IO01__PWM1_OUT				  0x0004 0x0034 0x0000 0x1 0x0
+#define MX7D_PAD_GPIO1_IO01__CCM_ENET_REF_CLK3			  0x0004 0x0034 0x0000 0x2 0x0
+#define MX7D_PAD_GPIO1_IO01__SAI1_MCLK				  0x0004 0x0034 0x0000 0x3 0x0
+#define MX7D_PAD_GPIO1_IO01__ANATOP_24M_OUT			  0x0004 0x0034 0x0000 0x4 0x0
+#define MX7D_PAD_GPIO1_IO01__OBSERVE0_OUT			  0x0004 0x0034 0x0000 0x6 0x0
+#define MX7D_PAD_GPIO1_IO02__GPIO1_IO2				  0x0008 0x0038 0x0000 0x0 0x0
+#define MX7D_PAD_GPIO1_IO02__PWM2_OUT				  0x0008 0x0038 0x0000 0x1 0x0
+#define MX7D_PAD_GPIO1_IO02__CCM_ENET_REF_CLK1			  0x0008 0x0038 0x0564 0x2 0x3
+#define MX7D_PAD_GPIO1_IO02__SAI2_MCLK				  0x0008 0x0038 0x0000 0x3 0x0
+#define MX7D_PAD_GPIO1_IO02__CCM_CLKO1				  0x0008 0x0038 0x0000 0x5 0x0
+#define MX7D_PAD_GPIO1_IO02__OBSERVE1_OUT			  0x0008 0x0038 0x0000 0x6 0x0
+#define MX7D_PAD_GPIO1_IO02__USB_OTG1_ID			  0x0008 0x0038 0x0734 0x7 0x3
+#define MX7D_PAD_GPIO1_IO03__GPIO1_IO3				  0x000C 0x003C 0x0000 0x0 0x0
+#define MX7D_PAD_GPIO1_IO03__PWM3_OUT				  0x000C 0x003C 0x0000 0x1 0x0
+#define MX7D_PAD_GPIO1_IO03__CCM_ENET_REF_CLK2			  0x000C 0x003C 0x0570 0x2 0x3
+#define MX7D_PAD_GPIO1_IO03__SAI3_MCLK				  0x000C 0x003C 0x0000 0x3 0x0
+#define MX7D_PAD_GPIO1_IO03__CCM_CLKO2				  0x000C 0x003C 0x0000 0x5 0x0
+#define MX7D_PAD_GPIO1_IO03__OBSERVE2_OUT			  0x000C 0x003C 0x0000 0x6 0x0
+#define MX7D_PAD_GPIO1_IO03__USB_OTG2_ID			  0x000C 0x003C 0x0730 0x7 0x3
+#define MX7D_PAD_GPIO1_IO04__GPIO1_IO4				  0x0010 0x0040 0x0000 0x0 0x0
+#define MX7D_PAD_GPIO1_IO04__USB_OTG1_OC			  0x0010 0x0040 0x072C 0x1 0x1
+#define MX7D_PAD_GPIO1_IO04__FLEXTIMER1_CH4			  0x0010 0x0040 0x0594 0x2 0x1
+#define MX7D_PAD_GPIO1_IO04__UART5_DCE_CTS			  0x0010 0x0040 0x0000 0x3 0x0
+#define MX7D_PAD_GPIO1_IO04__UART5_DTE_RTS			  0x0010 0x0040 0x0710 0x3 0x4
+#define MX7D_PAD_GPIO1_IO04__I2C1_SCL				  0x0010 0x0040 0x05D4 0x4 0x2
+#define MX7D_PAD_GPIO1_IO04__OBSERVE3_OUT			  0x0010 0x0040 0x0000 0x6 0x0
+#define MX7D_PAD_GPIO1_IO05__GPIO1_IO5				  0x0014 0x0044 0x0000 0x0 0x0
+#define MX7D_PAD_GPIO1_IO05__USB_OTG1_PWR			  0x0014 0x0044 0x0000 0x1 0x0
+#define MX7D_PAD_GPIO1_IO05__FLEXTIMER1_CH5			  0x0014 0x0044 0x0598 0x2 0x1
+#define MX7D_PAD_GPIO1_IO05__UART5_DCE_RTS			  0x0014 0x0044 0x0710 0x3 0x5
+#define MX7D_PAD_GPIO1_IO05__UART5_DTE_CTS			  0x0014 0x0044 0x0000 0x3 0x0
+#define MX7D_PAD_GPIO1_IO05__I2C1_SDA				  0x0014 0x0044 0x05D8 0x4 0x2
+#define MX7D_PAD_GPIO1_IO05__OBSERVE4_OUT			  0x0014 0x0044 0x0000 0x6 0x0
+#define MX7D_PAD_GPIO1_IO06__GPIO1_IO6				  0x0018 0x0048 0x0000 0x0 0x0
+#define MX7D_PAD_GPIO1_IO06__USB_OTG2_OC			  0x0018 0x0048 0x0728 0x1 0x1
+#define MX7D_PAD_GPIO1_IO06__FLEXTIMER1_CH6			  0x0018 0x0048 0x059C 0x2 0x1
+#define MX7D_PAD_GPIO1_IO06__UART5_DCE_RX			  0x0018 0x0048 0x0714 0x3 0x4
+#define MX7D_PAD_GPIO1_IO06__UART5_DTE_TX			  0x0018 0x0048 0x0000 0x3 0x0
+#define MX7D_PAD_GPIO1_IO06__I2C2_SCL				  0x0018 0x0048 0x05DC 0x4 0x2
+#define MX7D_PAD_GPIO1_IO06__CCM_WAIT				  0x0018 0x0048 0x0000 0x5 0x0
+#define MX7D_PAD_GPIO1_IO06__KPP_ROW4				  0x0018 0x0048 0x0624 0x6 0x1
+#define MX7D_PAD_GPIO1_IO07__GPIO1_IO7				  0x001C 0x004C 0x0000 0x0 0x0
+#define MX7D_PAD_GPIO1_IO07__USB_OTG2_PWR			  0x001C 0x004C 0x0000 0x1 0x0
+#define MX7D_PAD_GPIO1_IO07__FLEXTIMER1_CH7			  0x001C 0x004C 0x05A0 0x2 0x1
+#define MX7D_PAD_GPIO1_IO07__UART5_DCE_TX			  0x001C 0x004C 0x0000 0x3 0x0
+#define MX7D_PAD_GPIO1_IO07__UART5_DTE_RX			  0x001C 0x004C 0x0714 0x3 0x5
+#define MX7D_PAD_GPIO1_IO07__I2C2_SDA				  0x001C 0x004C 0x05E0 0x4 0x2
+#define MX7D_PAD_GPIO1_IO07__CCM_STOP				  0x001C 0x004C 0x0000 0x5 0x0
+#define MX7D_PAD_GPIO1_IO07__KPP_COL4				  0x001C 0x004C 0x0604 0x6 0x1
+
+#endif /* __DTS_IMX7D_PINFUNC_LPSR_H */
diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index c4f12fd2e044..c138b0fb410a 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -46,6 +46,7 @@
 #include <dt-bindings/input/input.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include "imx7d-pinfunc.h"
+#include "imx7d-pinfunc-lpsr.h"
 
 / {
 	#address-cells = <1>;
-- 
2.11.0

[RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Oleksij Rempel]

From: Oleksij Rempel <linux@rempel-privat.de>

this driver was tested on NXP imx7d but should work on
imx6sx as well.
It will upload firmware to OCRAM, which shared memory between
Cortex A7 and Cortex M4, then turn M4 on.

Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
---
 drivers/remoteproc/Kconfig     |   8 ++
 drivers/remoteproc/Makefile    |   1 +
 drivers/remoteproc/imx_rproc.c | 264 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 273 insertions(+)
 create mode 100644 drivers/remoteproc/imx_rproc.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index faad69a1a597..8d33bff4f530 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
 	tristate
 	depends on REMOTEPROC
 
+config IMX_REMOTEPROC
+	tristate "IMX6/7 remoteproc support"
+	depends on SOC_IMX6SX || SOC_IMX7D
+	depends on REMOTEPROC
+	help
+	  TODO, write some thing here
+	  This can be either built-in or a loadable module.
+
 endif # REMOTEPROC
 
 endmenu
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index ffc5e430df27..d351c25cdb4e 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -19,3 +19,4 @@ qcom_wcnss_pil-y			+= qcom_wcnss.o
 qcom_wcnss_pil-y			+= qcom_wcnss_iris.o
 obj-$(CONFIG_ST_REMOTEPROC)		+= st_remoteproc.o
 obj-$(CONFIG_ST_SLIM_REMOTEPROC)	+= st_slim_rproc.o
+obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
new file mode 100644
index 000000000000..6bef85237a27
--- /dev/null
+++ b/drivers/remoteproc/imx_rproc.c
@@ -0,0 +1,264 @@
+/*
+ * Oleksij Rempel <o.rempel@pengutronix.de>
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/remoteproc.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+
+#define IMX7D_ENABLE_M4			BIT(3)
+#define IMX7D_SW_M4P_RST		BIT(2)
+#define IMX7D_SW_M4C_RST		BIT(1)
+#define IMX7D_SW_M4C_NON_SCLR_RST	BIT(0)
+
+#define IMX7D_M4_RST_MASK		0xf
+
+
+#define IMX7D_RPROC_MEM_MAX		2
+enum {
+	IMX7D_RPROC_IMEM,
+	IMX7D_RPROC_DMEM,
+};
+
+static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
+	[IMX7D_RPROC_IMEM]	= "imem",
+	[IMX7D_RPROC_DMEM]	= "dmem",
+};
+
+/**
+ * struct imx_rproc_mem - slim internal memory structure
+ * @cpu_addr: MPU virtual address of the memory region
+ * @bus_addr: Bus address used to access the memory region
+ * @size: Size of the memory region
+ */
+struct imx_rproc_mem {
+	void __iomem *cpu_addr;
+	phys_addr_t bus_addr;
+	size_t size;
+};
+
+struct imx_rproc_dcfg {
+	int offset;
+};
+
+struct imx_rproc {
+	struct device			*dev;
+	struct regmap			*regmap;
+	struct rproc			*rproc;
+	const struct imx_rproc_dcfg	*dcfg;
+	struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
+	struct clk		*clk;
+};
+
+static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
+	.offset = 0xc,
+};
+
+static int imx_rproc_start(struct rproc *rproc)
+{
+	struct imx_rproc *priv = rproc->priv;
+	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
+	struct device *dev = priv->dev;
+	int ret;
+
+	ret = clk_enable(priv->clk);
+	if (ret) {
+		dev_err(&rproc->dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	ret = regmap_update_bits(priv->regmap, dcfg->offset,
+				 IMX7D_M4_RST_MASK,
+				 IMX7D_SW_M4C_RST | IMX7D_SW_M4P_RST | IMX7D_ENABLE_M4);
+	if (ret) {
+		dev_err(dev, "Filed to enable M4!\n");
+		clk_disable(priv->clk);
+	}
+
+	return ret;
+}
+
+static int imx_rproc_stop(struct rproc *rproc)
+{
+	struct imx_rproc *priv = rproc->priv;
+	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
+	struct device *dev = priv->dev;
+	int ret;
+
+	ret = regmap_update_bits(priv->regmap, dcfg->offset,
+				 IMX7D_M4_RST_MASK,
+				 IMX7D_SW_M4C_NON_SCLR_RST);
+	if (ret)
+		dev_err(dev, "Filed to stop M4!\n");
+
+	clk_disable(priv->clk);
+
+	return ret;
+}
+
+static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
+{
+	struct imx_rproc *priv = rproc->priv;
+	void *va = NULL;
+	int i;
+
+	for (i = 0; i < IMX7D_RPROC_MEM_MAX; i++) {
+		if (da != priv->mem[i].bus_addr)
+			continue;
+
+		if (len <= priv->mem[i].size) {
+			/* __force to make sparse happy with type conversion */
+			va = (__force void *)priv->mem[i].cpu_addr;
+			break;
+		}
+	}
+
+	dev_dbg(&rproc->dev, "da = 0x%llx len = 0x%x va = 0x%p\n", da, len, va);
+
+	return va;
+}
+
+static const struct rproc_ops imx_rproc_ops = {
+	.start		= imx_rproc_start,
+	.stop		= imx_rproc_stop,
+	.da_to_va       = imx_rproc_da_to_va,
+};
+
+static const struct of_device_id imx_rproc_of_match[] = {
+	{ .compatible = "fsl,imx7d-rproc", .data = &imx_rproc_cfg_imx7d },
+	{},
+};
+MODULE_DEVICE_TABLE(of, imx_rproc_of_match);
+
+static int imx_rproc_addr_init(struct imx_rproc *priv,
+			       struct platform_device *pdev)
+{
+	int i, err;
+	struct resource *res;
+
+	/* get imem and dmem */
+	for (i = 0; i < ARRAY_SIZE(mem_names); i++) {
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						mem_names[i]);
+		if (!res)
+			continue;
+
+		priv->mem[i].cpu_addr = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(priv->mem[i].cpu_addr)) {
+			dev_err(&pdev->dev, "devm_ioremap_resource failed\n");
+			err = PTR_ERR(priv->mem[i].cpu_addr);
+			return err;
+		}
+		priv->mem[i].bus_addr = res->start & 0xffff;
+		priv->mem[i].size = resource_size(res);
+	}
+
+	return 0;
+}
+
+static int imx_rproc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct imx_rproc *priv;
+	struct rproc *rproc;
+	struct regmap_config config = { .name = "imx_rproc" };
+	const struct imx_rproc_dcfg *dcfg;
+	struct regmap *regmap;
+	int ret;
+
+	regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "failed to find syscon\n");
+		return PTR_ERR(regmap);
+	}
+        regmap_attach_dev(dev, regmap, &config);
+
+	/* set some other name then imx */
+	rproc = rproc_alloc(dev, "imx_rproc", &imx_rproc_ops, NULL, sizeof(*priv));
+	if (!rproc) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	dcfg = of_device_get_match_data(dev);
+	if (!dcfg)
+		return -EINVAL;
+
+	priv = rproc->priv;
+	priv->rproc = rproc;
+	priv->regmap = regmap;
+	priv->dcfg = dcfg;
+
+	dev_set_drvdata(dev, rproc);
+
+	ret = imx_rproc_addr_init(priv, pdev);
+	if (ret) {
+		dev_err(dev, "filed on imx_rproc_addr_init\n");
+		goto err_put_rproc;
+	}
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		dev_err(dev, "Failed to get clock\n");
+		return PTR_ERR(priv->clk);
+	}
+
+	ret = rproc_add(rproc);
+	if (ret) {
+		dev_err(dev, "rproc_add failed\n");
+		goto err_put_rproc;
+	}
+
+	ret = clk_prepare(priv->clk);
+	if (ret)
+		dev_err(dev, "failed to get clock\n");
+
+	return ret;
+
+err_put_rproc:
+	rproc_free(rproc);
+err:
+	return ret;
+}
+
+static int imx_rproc_remove(struct platform_device *pdev)
+{
+	struct rproc *rproc = platform_get_drvdata(pdev);
+
+	rproc_del(rproc);
+	rproc_free(rproc);
+
+	return 0;
+}
+
+static struct platform_driver imx_rproc_driver = {
+	.probe = imx_rproc_probe,
+	.remove = imx_rproc_remove,
+	.driver = {
+		.name = "imx_rproc",
+		.of_match_table = imx_rproc_of_match,
+	},
+};
+
+module_platform_driver(imx_rproc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("IMX6/7 remote processor control driver");
+MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
-- 
2.11.0

[RFC PATCH 3/3] ARM: dts: imx7s: add rproc node

[Oleksij Rempel]

From: Oleksij Rempel <linux@rempel-privat.de>

Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
---
 arch/arm/boot/dts/imx7s.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index c138b0fb410a..8b84cd3cdd28 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -566,6 +566,14 @@
 			};
 		};
 
+		imx_rproc: imx7d-rp0@0 {
+			compatible	= "fsl,imx7d-rproc";
+			reg		= <0x00180000 0x4000>, <0x00184000 0x4000>;
+			reg-names	= "imem", "dmem";
+			syscon		= <&src>;
+			clocks		= <&clks IMX7D_ARM_M4_ROOT_CLK>;
+		};
+
 		aips2: aips-bus@30400000 {
 			compatible = "fsl,aips-bus", "simple-bus";
 			#address-cells = <1>;
-- 
2.11.0

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

Hi Oleksij,

On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
> From: Oleksij Rempel <linux@rempel-privat.de>
> 
> this driver was tested on NXP imx7d but should work on
> imx6sx as well.
> It will upload firmware to OCRAM, which shared memory between
> Cortex A7 and Cortex M4, then turn M4 on.

Mostly looks fine, need to address few comments. I take it that you 
haven't added the binding since this is just an RFC.

> 
> Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
> ---
>   drivers/remoteproc/Kconfig     |   8 ++
>   drivers/remoteproc/Makefile    |   1 +
>   drivers/remoteproc/imx_rproc.c | 264 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 273 insertions(+)
>   create mode 100644 drivers/remoteproc/imx_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index faad69a1a597..8d33bff4f530 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>   	tristate
>   	depends on REMOTEPROC
>   
> +config IMX_REMOTEPROC
> +	tristate "IMX6/7 remoteproc support"
> +	depends on SOC_IMX6SX || SOC_IMX7D
> +	depends on REMOTEPROC
> +	help
> +	  TODO, write some thing here
> +	  This can be either built-in or a loadable module.
> +
>   endif # REMOTEPROC
>   
>   endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index ffc5e430df27..d351c25cdb4e 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y			+= qcom_wcnss.o
>   qcom_wcnss_pil-y			+= qcom_wcnss_iris.o
>   obj-$(CONFIG_ST_REMOTEPROC)		+= st_remoteproc.o
>   obj-$(CONFIG_ST_SLIM_REMOTEPROC)	+= st_slim_rproc.o
> +obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> new file mode 100644
> index 000000000000..6bef85237a27
> --- /dev/null
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -0,0 +1,264 @@
> +/*
> + * Oleksij Rempel <o.rempel@pengutronix.de>

Please add a blank line here.

> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define IMX7D_ENABLE_M4			BIT(3)
> +#define IMX7D_SW_M4P_RST		BIT(2)
> +#define IMX7D_SW_M4C_RST		BIT(1)
> +#define IMX7D_SW_M4C_NON_SCLR_RST	BIT(0)
> +
> +#define IMX7D_M4_RST_MASK		0xf
> +
> +
> +#define IMX7D_RPROC_MEM_MAX		2
> +enum {
> +	IMX7D_RPROC_IMEM,
> +	IMX7D_RPROC_DMEM,
> +};
> +
> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
> +	[IMX7D_RPROC_IMEM]	= "imem",
> +	[IMX7D_RPROC_DMEM]	= "dmem",
> +};

Do you really need these to be globally defined? You only need them in 
the addr_init function, they can be made local to that function.

> +
> +/**
> + * struct imx_rproc_mem - slim internal memory structure
> + * @cpu_addr: MPU virtual address of the memory region
> + * @bus_addr: Bus address used to access the memory region
> + * @size: Size of the memory region
> + */
> +struct imx_rproc_mem {
> +	void __iomem *cpu_addr;
> +	phys_addr_t bus_addr;

Are the rproc-view of these regions same as the bus addresses or
do they have their own local addresses? If latter, images that are built 
for those will be using those addresses in which case you need some 
offset calculation in the da_to_va ops?

> +	size_t size;
> +};
> +
> +struct imx_rproc_dcfg {
> +	int offset;
> +};
> +
> +struct imx_rproc {
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct rproc			*rproc;
> +	const struct imx_rproc_dcfg	*dcfg;

No need of aligning the variables, you can get rid of the additional 
whitespaces and maintain consistent style.

> +	struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
> +	struct clk		*clk;
> +};
> +
> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
> +	.offset = 0xc,
> +};
> +
> +static int imx_rproc_start(struct rproc *rproc)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> +	struct device *dev = priv->dev;
> +	int ret;
> +
> +	ret = clk_enable(priv->clk);
> +	if (ret) {
> +		dev_err(&rproc->dev, "Failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(priv->regmap, dcfg->offset,
> +				 IMX7D_M4_RST_MASK,
> +				 IMX7D_SW_M4C_RST | IMX7D_SW_M4P_RST | IMX7D_ENABLE_M4);
> +	if (ret) {
> +		dev_err(dev, "Filed to enable M4!\n");
> +		clk_disable(priv->clk);
> +	}
> +
> +	return ret;
> +}
> +
> +static int imx_rproc_stop(struct rproc *rproc)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> +	struct device *dev = priv->dev;
> +	int ret;
> +
> +	ret = regmap_update_bits(priv->regmap, dcfg->offset,
> +				 IMX7D_M4_RST_MASK,
> +				 IMX7D_SW_M4C_NON_SCLR_RST);
> +	if (ret)
> +		dev_err(dev, "Filed to stop M4!\n");
> +
> +	clk_disable(priv->clk);
> +
> +	return ret;
> +}
> +
> +static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	void *va = NULL;
> +	int i;
> +
> +	for (i = 0; i < IMX7D_RPROC_MEM_MAX; i++) {
> +		if (da != priv->mem[i].bus_addr)
> +			continue;
> +
> +		if (len <= priv->mem[i].size) {
> +			/* __force to make sparse happy with type conversion */
> +			va = (__force void *)priv->mem[i].cpu_addr;
> +			break;
> +		}
> +	}
> +
> +	dev_dbg(&rproc->dev, "da = 0x%llx len = 0x%x va = 0x%p\n", da, len, va);
> +
> +	return va;
> +}
> +
> +static const struct rproc_ops imx_rproc_ops = {
> +	.start		= imx_rproc_start,
> +	.stop		= imx_rproc_stop,
> +	.da_to_va       = imx_rproc_da_to_va,
> +};
> +
> +static const struct of_device_id imx_rproc_of_match[] = {
> +	{ .compatible = "fsl,imx7d-rproc", .data = &imx_rproc_cfg_imx7d },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, imx_rproc_of_match);
> +
> +static int imx_rproc_addr_init(struct imx_rproc *priv,
> +			       struct platform_device *pdev)
> +{
> +	int i, err;
> +	struct resource *res;
> +
> +	/* get imem and dmem */
> +	for (i = 0; i < ARRAY_SIZE(mem_names); i++) {
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						mem_names[i]);
> +		if (!res)
> +			continue;
> +
> +		priv->mem[i].cpu_addr = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(priv->mem[i].cpu_addr)) {
> +			dev_err(&pdev->dev, "devm_ioremap_resource failed\n");
> +			err = PTR_ERR(priv->mem[i].cpu_addr);
> +			return err;
> +		}
> +		priv->mem[i].bus_addr = res->start & 0xffff;
> +		priv->mem[i].size = resource_size(res);
> +	}
> +
> +	return 0;
> +}
> +
> +static int imx_rproc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct imx_rproc *priv;
> +	struct rproc *rproc;
> +	struct regmap_config config = { .name = "imx_rproc" };
> +	const struct imx_rproc_dcfg *dcfg;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "failed to find syscon\n");
> +		return PTR_ERR(regmap);
> +	}
> +        regmap_attach_dev(dev, regmap, &config);
> +
> +	/* set some other name then imx */
> +	rproc = rproc_alloc(dev, "imx_rproc", &imx_rproc_ops, NULL, sizeof(*priv));
> +	if (!rproc) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	dcfg = of_device_get_match_data(dev);
> +	if (!dcfg)
> +		return -EINVAL;

move this up to the beginning of the function. You can avoid the cleanup 
(you are missing an rproc_free() here)

> +
> +	priv = rproc->priv;
> +	priv->rproc = rproc;
> +	priv->regmap = regmap;
> +	priv->dcfg = dcfg;
> +
> +	dev_set_drvdata(dev, rproc);
> +
> +	ret = imx_rproc_addr_init(priv, pdev);
> +	if (ret) {
> +		dev_err(dev, "filed on imx_rproc_addr_init\n");
> +		goto err_put_rproc;
> +	}
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		dev_err(dev, "Failed to get clock\n");
> +		return PTR_ERR(priv->clk);
> +	}
> +
> +	ret = rproc_add(rproc);
> +	if (ret) {
> +		dev_err(dev, "rproc_add failed\n");
> +		goto err_put_rproc;
> +	}
> +
> +	ret = clk_prepare(priv->clk);
> +	if (ret)
> +		dev_err(dev, "failed to get clock\n");

How are your internal memories clocked? If they need the same clock, 
rproc_add() would need the clock to be enabled for loading section data 
into those regions as part of rproc_boot(), and your current enable in 
imx_rproc_start() will be too late. Also, do not see a balancing 
clk_unprepare() call in remove, and a missing rproc_del() on failure here.

regards
Suman

> +
> +	return ret;
> +
> +err_put_rproc:
> +	rproc_free(rproc);
> +err:
> +	return ret;
> +}
> +
> +static int imx_rproc_remove(struct platform_device *pdev)
> +{
> +	struct rproc *rproc = platform_get_drvdata(pdev);
> +
> +	rproc_del(rproc);
> +	rproc_free(rproc);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver imx_rproc_driver = {
> +	.probe = imx_rproc_probe,
> +	.remove = imx_rproc_remove,
> +	.driver = {
> +		.name = "imx_rproc",
> +		.of_match_table = imx_rproc_of_match,
> +	},
> +};
> +
> +module_platform_driver(imx_rproc_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("IMX6/7 remote processor control driver");
> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
> 

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

On 06/15/2017 02:01 PM, Suman Anna wrote:
> Hi Oleksij,
> 
> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
>> From: Oleksij Rempel <linux@rempel-privat.de>
>>
>> this driver was tested on NXP imx7d but should work on
>> imx6sx as well.
>> It will upload firmware to OCRAM, which shared memory between
>> Cortex A7 and Cortex M4, then turn M4 on.
> 
> Mostly looks fine, need to address few comments. I take it that you
> haven't added the binding since this is just an RFC.
> 
>>
>> Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
>> ---
>>   drivers/remoteproc/Kconfig     |   8 ++
>>   drivers/remoteproc/Makefile    |   1 +
>>   drivers/remoteproc/imx_rproc.c | 264
>> +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 273 insertions(+)
>>   create mode 100644 drivers/remoteproc/imx_rproc.c
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index faad69a1a597..8d33bff4f530 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>>       tristate
>>       depends on REMOTEPROC
>>   +config IMX_REMOTEPROC
>> +    tristate "IMX6/7 remoteproc support"
>> +    depends on SOC_IMX6SX || SOC_IMX7D
>> +    depends on REMOTEPROC
>> +    help
>> +      TODO, write some thing here
>> +      This can be either built-in or a loadable module.
>> +
>>   endif # REMOTEPROC
>>     endmenu
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index ffc5e430df27..d351c25cdb4e 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y            += qcom_wcnss.o
>>   qcom_wcnss_pil-y            += qcom_wcnss_iris.o
>>   obj-$(CONFIG_ST_REMOTEPROC)        += st_remoteproc.o
>>   obj-$(CONFIG_ST_SLIM_REMOTEPROC)    += st_slim_rproc.o
>> +obj-$(CONFIG_IMX_REMOTEPROC)        += imx_rproc.o
>> diff --git a/drivers/remoteproc/imx_rproc.c
>> b/drivers/remoteproc/imx_rproc.c
>> new file mode 100644
>> index 000000000000..6bef85237a27
>> --- /dev/null
>> +++ b/drivers/remoteproc/imx_rproc.c
>> @@ -0,0 +1,264 @@
>> +/*
>> + * Oleksij Rempel <o.rempel@pengutronix.de>
> 
> Please add a blank line here.
> 
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/remoteproc.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define IMX7D_ENABLE_M4            BIT(3)
>> +#define IMX7D_SW_M4P_RST        BIT(2)
>> +#define IMX7D_SW_M4C_RST        BIT(1)
>> +#define IMX7D_SW_M4C_NON_SCLR_RST    BIT(0)
>> +
>> +#define IMX7D_M4_RST_MASK        0xf
>> +
>> +
>> +#define IMX7D_RPROC_MEM_MAX        2
>> +enum {
>> +    IMX7D_RPROC_IMEM,
>> +    IMX7D_RPROC_DMEM,
>> +};
>> +
>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
>> +    [IMX7D_RPROC_IMEM]    = "imem",
>> +    [IMX7D_RPROC_DMEM]    = "dmem",
>> +};
> 
> Do you really need these to be globally defined? You only need them in
> the addr_init function, they can be made local to that function.
> 
>> +
>> +/**
>> + * struct imx_rproc_mem - slim internal memory structure
>> + * @cpu_addr: MPU virtual address of the memory region
>> + * @bus_addr: Bus address used to access the memory region
>> + * @size: Size of the memory region
>> + */
>> +struct imx_rproc_mem {
>> +    void __iomem *cpu_addr;
>> +    phys_addr_t bus_addr;
> 
> Are the rproc-view of these regions same as the bus addresses or
> do they have their own local addresses? If latter, images that are built
> for those will be using those addresses in which case you need some
> offset calculation in the da_to_va ops?
> 
>> +    size_t size;
>> +};
>> +
>> +struct imx_rproc_dcfg {
>> +    int offset;
>> +};
>> +
>> +struct imx_rproc {
>> +    struct device            *dev;
>> +    struct regmap            *regmap;
>> +    struct rproc            *rproc;
>> +    const struct imx_rproc_dcfg    *dcfg;
> 
> No need of aligning the variables, you can get rid of the additional
> whitespaces and maintain consistent style.
> 
>> +    struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
>> +    struct clk        *clk;
>> +};
>> +
>> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
>> +    .offset = 0xc,
>> +};
>> +
>> +static int imx_rproc_start(struct rproc *rproc)
>> +{
>> +    struct imx_rproc *priv = rproc->priv;
>> +    const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>> +    struct device *dev = priv->dev;
>> +    int ret;
>> +
>> +    ret = clk_enable(priv->clk);
>> +    if (ret) {
>> +        dev_err(&rproc->dev, "Failed to enable clock\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = regmap_update_bits(priv->regmap, dcfg->offset,
>> +                 IMX7D_M4_RST_MASK,
>> +                 IMX7D_SW_M4C_RST | IMX7D_SW_M4P_RST | IMX7D_ENABLE_M4);
>> +    if (ret) {
>> +        dev_err(dev, "Filed to enable M4!\n");

typo for failed..

>> +        clk_disable(priv->clk);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static int imx_rproc_stop(struct rproc *rproc)
>> +{
>> +    struct imx_rproc *priv = rproc->priv;
>> +    const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>> +    struct device *dev = priv->dev;
>> +    int ret;
>> +
>> +    ret = regmap_update_bits(priv->regmap, dcfg->offset,
>> +                 IMX7D_M4_RST_MASK,
>> +                 IMX7D_SW_M4C_NON_SCLR_RST);
>> +    if (ret)
>> +        dev_err(dev, "Filed to stop M4!\n");
>> +
>> +    clk_disable(priv->clk);
>> +
>> +    return ret;
>> +}
>> +
>> +static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>> +{
>> +    struct imx_rproc *priv = rproc->priv;
>> +    void *va = NULL;
>> +    int i;
>> +
>> +    for (i = 0; i < IMX7D_RPROC_MEM_MAX; i++) {
>> +        if (da != priv->mem[i].bus_addr)
>> +            continue;
>> +
>> +        if (len <= priv->mem[i].size) {
>> +            /* __force to make sparse happy with type conversion */
>> +            va = (__force void *)priv->mem[i].cpu_addr;
>> +            break;
>> +        }

missed this yesterday, but this implementation is wrong, you need to be
doing a range check and translate here accordingly. See for an example a
patch on a driver I submitted recently,
https://patchwork.kernel.org/patch/9773687/

regards
Suman

>> +    }
>> +
>> +    dev_dbg(&rproc->dev, "da = 0x%llx len = 0x%x va = 0x%p\n", da,
>> len, va);
>> +
>> +    return va;
>> +}
>> +
>> +static const struct rproc_ops imx_rproc_ops = {
>> +    .start        = imx_rproc_start,
>> +    .stop        = imx_rproc_stop,
>> +    .da_to_va       = imx_rproc_da_to_va,
>> +};
>> +
>> +static const struct of_device_id imx_rproc_of_match[] = {
>> +    { .compatible = "fsl,imx7d-rproc", .data = &imx_rproc_cfg_imx7d },
>> +    {},
>> +};
>> +MODULE_DEVICE_TABLE(of, imx_rproc_of_match);
>> +
>> +static int imx_rproc_addr_init(struct imx_rproc *priv,
>> +                   struct platform_device *pdev)
>> +{
>> +    int i, err;
>> +    struct resource *res;
>> +
>> +    /* get imem and dmem */
>> +    for (i = 0; i < ARRAY_SIZE(mem_names); i++) {
>> +        res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +                        mem_names[i]);
>> +        if (!res)
>> +            continue;
>> +
>> +        priv->mem[i].cpu_addr = devm_ioremap_resource(&pdev->dev, res);
>> +        if (IS_ERR(priv->mem[i].cpu_addr)) {
>> +            dev_err(&pdev->dev, "devm_ioremap_resource failed\n");
>> +            err = PTR_ERR(priv->mem[i].cpu_addr);
>> +            return err;
>> +        }
>> +        priv->mem[i].bus_addr = res->start & 0xffff;
>> +        priv->mem[i].size = resource_size(res);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int imx_rproc_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct device_node *np = dev->of_node;
>> +    struct imx_rproc *priv;
>> +    struct rproc *rproc;
>> +    struct regmap_config config = { .name = "imx_rproc" };
>> +    const struct imx_rproc_dcfg *dcfg;
>> +    struct regmap *regmap;
>> +    int ret;
>> +
>> +    regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
>> +    if (IS_ERR(regmap)) {
>> +        dev_err(dev, "failed to find syscon\n");
>> +        return PTR_ERR(regmap);
>> +    }
>> +        regmap_attach_dev(dev, regmap, &config);
>> +
>> +    /* set some other name then imx */
>> +    rproc = rproc_alloc(dev, "imx_rproc", &imx_rproc_ops, NULL,
>> sizeof(*priv));
>> +    if (!rproc) {
>> +        ret = -ENOMEM;
>> +        goto err;
>> +    }
>> +
>> +    dcfg = of_device_get_match_data(dev);
>> +    if (!dcfg)
>> +        return -EINVAL;
> 
> move this up to the beginning of the function. You can avoid the cleanup
> (you are missing an rproc_free() here)
> 
>> +
>> +    priv = rproc->priv;
>> +    priv->rproc = rproc;
>> +    priv->regmap = regmap;
>> +    priv->dcfg = dcfg;
>> +
>> +    dev_set_drvdata(dev, rproc);
>> +
>> +    ret = imx_rproc_addr_init(priv, pdev);
>> +    if (ret) {
>> +        dev_err(dev, "filed on imx_rproc_addr_init\n");
>> +        goto err_put_rproc;
>> +    }
>> +
>> +    priv->clk = devm_clk_get(dev, NULL);
>> +    if (IS_ERR(priv->clk)) {
>> +        dev_err(dev, "Failed to get clock\n");
>> +        return PTR_ERR(priv->clk);
>> +    }
>> +
>> +    ret = rproc_add(rproc);
>> +    if (ret) {
>> +        dev_err(dev, "rproc_add failed\n");
>> +        goto err_put_rproc;
>> +    }
>> +
>> +    ret = clk_prepare(priv->clk);
>> +    if (ret)
>> +        dev_err(dev, "failed to get clock\n");
> 
> How are your internal memories clocked? If they need the same clock,
> rproc_add() would need the clock to be enabled for loading section data
> into those regions as part of rproc_boot(), and your current enable in
> imx_rproc_start() will be too late. Also, do not see a balancing
> clk_unprepare() call in remove, and a missing rproc_del() on failure here.
> 
> regards
> Suman
> 
>> +
>> +    return ret;
>> +
>> +err_put_rproc:
>> +    rproc_free(rproc);
>> +err:
>> +    return ret;
>> +}
>> +
>> +static int imx_rproc_remove(struct platform_device *pdev)
>> +{
>> +    struct rproc *rproc = platform_get_drvdata(pdev);
>> +
>> +    rproc_del(rproc);
>> +    rproc_free(rproc);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct platform_driver imx_rproc_driver = {
>> +    .probe = imx_rproc_probe,
>> +    .remove = imx_rproc_remove,
>> +    .driver = {
>> +        .name = "imx_rproc",
>> +        .of_match_table = imx_rproc_of_match,
>> +    },
>> +};
>> +
>> +module_platform_driver(imx_rproc_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("IMX6/7 remote processor control driver");
>> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
>>
> 

Re: [RFC PATCH 0/3] provide imx rproc driver

[Sanchayan]

Hello Oleksij,

On 17-06-14 22:48:52, Oleksij Rempel wrote:
> Hallo all,
> 
> this is RFC patchset to provide remoteproc functionality on
> imx7d SoC.
> Since current kernel do not have devicetrees for board which
> I used for testing, this RFC patchset includes this too.
> 
> For testing I used this simple counter written in ASM:
> ======================================
>         .syntax unified
>         .text
>         .thumb
>         .int 0x10020000                 @ Initial SP value
>         .int reset + 1
> 
> reset:
> 
>         mov     r0, #0x55
>         ldr     r1, =(0x40)
> 1:
>         str     r0, [r1]
>         add     r0, 1
>         b       1b
> 
>         /* Dummy data, required by remoteproc loader */
>         /* Please FIXME, this part seem to be incorrect */
>         .data
>         .section .resource_table, "aw"
>         .word   1, 0, 0, 0      /* struct resource_table base */
>         .word   0               /* uint32_t offset[1] */
> ============================================================
> compiled with:
> ${CROSS}as -o imx7m4.o imx7m4.S
> ${CROSS}ld -Ttext=0x0 -o imx7m4.elf imx7m4.o
> cp imx7m4.elf /srv/nfs/sid-armhf/lib/firmware/rproc-imx_rproc-fw
> 
> Functionality was confirmed with current OpenOCD master.
> OpenOCD cfg file can be found here:
> https://github.com/olerem/openocd/blob/imx7-2017.06.14/tcl/target/imx7.cfg
> 
> Comment and suggestions are welcome.

Have you by chance also tried testing this with FreeRTOS code running on the
M4 side?

Regards,
Sanchayan.

> 
> Regards,
> Oleksij
> 
> Oleksij Rempel (3):
>   ARM: dts: imx7d: add imx7d-phyboard-zeta
>   remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver
>   ARM: dts: imx7s: add rproc node
> 
>  arch/arm/boot/dts/Makefile                |   1 +
>  arch/arm/boot/dts/imx7d-pba-c-09.dtsi     | 272 ++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/imx7d-peb-av-02.dtsi    | 104 ++++++++++++
>  arch/arm/boot/dts/imx7d-peb-eval-02.dtsi  | 130 ++++++++++++++
>  arch/arm/boot/dts/imx7d-phyboard-zeta.dts | 144 ++++++++++++++++
>  arch/arm/boot/dts/imx7d-phycore-som.dtsi  | 272 ++++++++++++++++++++++++++++++
>  arch/arm/boot/dts/imx7d-pinfunc-lpsr.h    |  76 +++++++++
>  arch/arm/boot/dts/imx7s.dtsi              |   9 +
>  drivers/remoteproc/Kconfig                |   8 +
>  drivers/remoteproc/Makefile               |   1 +
>  drivers/remoteproc/imx_rproc.c            | 264 +++++++++++++++++++++++++++++
>  11 files changed, 1281 insertions(+)
>  create mode 100644 arch/arm/boot/dts/imx7d-pba-c-09.dtsi
>  create mode 100644 arch/arm/boot/dts/imx7d-peb-av-02.dtsi
>  create mode 100644 arch/arm/boot/dts/imx7d-peb-eval-02.dtsi
>  create mode 100644 arch/arm/boot/dts/imx7d-phyboard-zeta.dts
>  create mode 100644 arch/arm/boot/dts/imx7d-phycore-som.dtsi
>  create mode 100644 arch/arm/boot/dts/imx7d-pinfunc-lpsr.h
>  create mode 100644 drivers/remoteproc/imx_rproc.c
> 
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/3] provide imx rproc driver

[Oleksij Rempel]

Hi Sanchayan,

On 19.06.2017 07:13, Sanchayan wrote:
> Hello Oleksij,
>
> On 17-06-14 22:48:52, Oleksij Rempel wrote:
>> Hallo all,
>>
>> this is RFC patchset to provide remoteproc functionality on
>> imx7d SoC.
>> Since current kernel do not have devicetrees for board which
>> I used for testing, this RFC patchset includes this too.
>>
>> For testing I used this simple counter written in ASM:
>> ======================================
>>         .syntax unified
>>         .text
>>         .thumb
>>         .int 0x10020000                 @ Initial SP value
>>         .int reset + 1
>>
>> reset:
>>
>>         mov     r0, #0x55
>>         ldr     r1, =(0x40)
>> 1:
>>         str     r0, [r1]
>>         add     r0, 1
>>         b       1b
>>
>>         /* Dummy data, required by remoteproc loader */
>>         /* Please FIXME, this part seem to be incorrect */
>>         .data
>>         .section .resource_table, "aw"
>>         .word   1, 0, 0, 0      /* struct resource_table base */
>>         .word   0               /* uint32_t offset[1] */
>> ============================================================
>> compiled with:
>> ${CROSS}as -o imx7m4.o imx7m4.S
>> ${CROSS}ld -Ttext=0x0 -o imx7m4.elf imx7m4.o
>> cp imx7m4.elf /srv/nfs/sid-armhf/lib/firmware/rproc-imx_rproc-fw
>>
>> Functionality was confirmed with current OpenOCD master.
>> OpenOCD cfg file can be found here:
>> https://github.com/olerem/openocd/blob/imx7-2017.06.14/tcl/target/imx7.cfg
>>
>> Comment and suggestions are welcome.
>
> Have you by chance also tried testing this with FreeRTOS code running on the
> M4 side?

no, currently my priority is to provide basic functionality with easy 
understandable target code and dependencies.

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Oleksij Rempel]

Hi,

Thank you for your review!

On 15.06.2017 21:01, Suman Anna wrote:
> Hi Oleksij,
>
> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
>> From: Oleksij Rempel <linux@rempel-privat.de>
>>
>> this driver was tested on NXP imx7d but should work on
>> imx6sx as well.
>> It will upload firmware to OCRAM, which shared memory between
>> Cortex A7 and Cortex M4, then turn M4 on.
>
> Mostly looks fine, need to address few comments. I take it that you
> haven't added the binding since this is just an RFC.
>
>>
>> Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
>> ---
>>   drivers/remoteproc/Kconfig     |   8 ++
>>   drivers/remoteproc/Makefile    |   1 +
>>   drivers/remoteproc/imx_rproc.c | 264
>> +++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 273 insertions(+)
>>   create mode 100644 drivers/remoteproc/imx_rproc.c
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index faad69a1a597..8d33bff4f530 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>>       tristate
>>       depends on REMOTEPROC
>>   +config IMX_REMOTEPROC
>> +    tristate "IMX6/7 remoteproc support"
>> +    depends on SOC_IMX6SX || SOC_IMX7D
>> +    depends on REMOTEPROC
>> +    help
>> +      TODO, write some thing here
>> +      This can be either built-in or a loadable module.
>> +
>>   endif # REMOTEPROC
>>     endmenu
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index ffc5e430df27..d351c25cdb4e 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y            += qcom_wcnss.o
>>   qcom_wcnss_pil-y            += qcom_wcnss_iris.o
>>   obj-$(CONFIG_ST_REMOTEPROC)        += st_remoteproc.o
>>   obj-$(CONFIG_ST_SLIM_REMOTEPROC)    += st_slim_rproc.o
>> +obj-$(CONFIG_IMX_REMOTEPROC)        += imx_rproc.o
>> diff --git a/drivers/remoteproc/imx_rproc.c
>> b/drivers/remoteproc/imx_rproc.c
>> new file mode 100644
>> index 000000000000..6bef85237a27
>> --- /dev/null
>> +++ b/drivers/remoteproc/imx_rproc.c
>> @@ -0,0 +1,264 @@
>> +/*
>> + * Oleksij Rempel <o.rempel@pengutronix.de>
>
> Please add a blank line here.
>
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/remoteproc.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define IMX7D_ENABLE_M4            BIT(3)
>> +#define IMX7D_SW_M4P_RST        BIT(2)
>> +#define IMX7D_SW_M4C_RST        BIT(1)
>> +#define IMX7D_SW_M4C_NON_SCLR_RST    BIT(0)
>> +
>> +#define IMX7D_M4_RST_MASK        0xf
>> +
>> +
>> +#define IMX7D_RPROC_MEM_MAX        2
>> +enum {
>> +    IMX7D_RPROC_IMEM,
>> +    IMX7D_RPROC_DMEM,
>> +};
>> +
>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
>> +    [IMX7D_RPROC_IMEM]    = "imem",
>> +    [IMX7D_RPROC_DMEM]    = "dmem",
>> +};
>
> Do you really need these to be globally defined? You only need them in
> the addr_init function, they can be made local to that function.

I don't needed. At least not with my testing remote code.
It is mostly copy/paste from existing drivers.

But according to this page, there are multiple memory regions, with 
different mapping for data and code.
http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas

"The Cortex-M4 CPU has two buses connected to the main interconnect 
(modified Harvard architecture). One bus is meant to fetch data (system 
bus) whereas the other bus is meant to fetch instructions (code bus). To 
get optimal performance, the program code should be located and linked 
for a region which is going to be fetched through the code bus, while 
the data area (e.g. bss or data section) should be located in a region 
which is fetched through the system bus. There are multiple example 
linker files in the platform/devices/MCIMX7D/linker/ sub directory which 
can be used and/or modified. All example firmware below use the 
MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region 
for data)."

What is the proper way to implement it with remoteproc?

>> +
>> +/**
>> + * struct imx_rproc_mem - slim internal memory structure
>> + * @cpu_addr: MPU virtual address of the memory region
>> + * @bus_addr: Bus address used to access the memory region
>> + * @size: Size of the memory region
>> + */
>> +struct imx_rproc_mem {
>> +    void __iomem *cpu_addr;
>> +    phys_addr_t bus_addr;
>
> Are the rproc-view of these regions same as the bus addresses or
> do they have their own local addresses? If latter, images that are built
> for those will be using those addresses in which case you need some
> offset calculation in the da_to_va ops?
>
>> +    size_t size;
>> +};
>> +
>> +struct imx_rproc_dcfg {
>> +    int offset;
>> +};
>> +
>> +struct imx_rproc {
>> +    struct device            *dev;
>> +    struct regmap            *regmap;
>> +    struct rproc            *rproc;
>> +    const struct imx_rproc_dcfg    *dcfg;
>
> No need of aligning the variables, you can get rid of the additional
> whitespaces and maintain consistent style.
>
>> +    struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
>> +    struct clk        *clk;
>> +};
>> +
>> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
>> +    .offset = 0xc,
>> +};
>> +
>> +static int imx_rproc_start(struct rproc *rproc)
>> +{
>> +    struct imx_rproc *priv = rproc->priv;
>> +    const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>> +    struct device *dev = priv->dev;
>> +    int ret;
>> +
>> +    ret = clk_enable(priv->clk);
>> +    if (ret) {
>> +        dev_err(&rproc->dev, "Failed to enable clock\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = regmap_update_bits(priv->regmap, dcfg->offset,
>> +                 IMX7D_M4_RST_MASK,
>> +                 IMX7D_SW_M4C_RST | IMX7D_SW_M4P_RST | IMX7D_ENABLE_M4);
>> +    if (ret) {
>> +        dev_err(dev, "Filed to enable M4!\n");
>> +        clk_disable(priv->clk);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static int imx_rproc_stop(struct rproc *rproc)
>> +{
>> +    struct imx_rproc *priv = rproc->priv;
>> +    const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>> +    struct device *dev = priv->dev;
>> +    int ret;
>> +
>> +    ret = regmap_update_bits(priv->regmap, dcfg->offset,
>> +                 IMX7D_M4_RST_MASK,
>> +                 IMX7D_SW_M4C_NON_SCLR_RST);
>> +    if (ret)
>> +        dev_err(dev, "Filed to stop M4!\n");
>> +
>> +    clk_disable(priv->clk);
>> +
>> +    return ret;
>> +}
>> +
>> +static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>> +{
>> +    struct imx_rproc *priv = rproc->priv;
>> +    void *va = NULL;
>> +    int i;
>> +
>> +    for (i = 0; i < IMX7D_RPROC_MEM_MAX; i++) {
>> +        if (da != priv->mem[i].bus_addr)
>> +            continue;
>> +
>> +        if (len <= priv->mem[i].size) {
>> +            /* __force to make sparse happy with type conversion */
>> +            va = (__force void *)priv->mem[i].cpu_addr;
>> +            break;
>> +        }
>> +    }
>> +
>> +    dev_dbg(&rproc->dev, "da = 0x%llx len = 0x%x va = 0x%p\n", da,
>> len, va);
>> +
>> +    return va;
>> +}
>> +
>> +static const struct rproc_ops imx_rproc_ops = {
>> +    .start        = imx_rproc_start,
>> +    .stop        = imx_rproc_stop,
>> +    .da_to_va       = imx_rproc_da_to_va,
>> +};
>> +
>> +static const struct of_device_id imx_rproc_of_match[] = {
>> +    { .compatible = "fsl,imx7d-rproc", .data = &imx_rproc_cfg_imx7d },
>> +    {},
>> +};
>> +MODULE_DEVICE_TABLE(of, imx_rproc_of_match);
>> +
>> +static int imx_rproc_addr_init(struct imx_rproc *priv,
>> +                   struct platform_device *pdev)
>> +{
>> +    int i, err;
>> +    struct resource *res;
>> +
>> +    /* get imem and dmem */
>> +    for (i = 0; i < ARRAY_SIZE(mem_names); i++) {
>> +        res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +                        mem_names[i]);
>> +        if (!res)
>> +            continue;
>> +
>> +        priv->mem[i].cpu_addr = devm_ioremap_resource(&pdev->dev, res);
>> +        if (IS_ERR(priv->mem[i].cpu_addr)) {
>> +            dev_err(&pdev->dev, "devm_ioremap_resource failed\n");
>> +            err = PTR_ERR(priv->mem[i].cpu_addr);
>> +            return err;
>> +        }
>> +        priv->mem[i].bus_addr = res->start & 0xffff;
>> +        priv->mem[i].size = resource_size(res);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int imx_rproc_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct device_node *np = dev->of_node;
>> +    struct imx_rproc *priv;
>> +    struct rproc *rproc;
>> +    struct regmap_config config = { .name = "imx_rproc" };
>> +    const struct imx_rproc_dcfg *dcfg;
>> +    struct regmap *regmap;
>> +    int ret;
>> +
>> +    regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
>> +    if (IS_ERR(regmap)) {
>> +        dev_err(dev, "failed to find syscon\n");
>> +        return PTR_ERR(regmap);
>> +    }
>> +        regmap_attach_dev(dev, regmap, &config);
>> +
>> +    /* set some other name then imx */
>> +    rproc = rproc_alloc(dev, "imx_rproc", &imx_rproc_ops, NULL,
>> sizeof(*priv));
>> +    if (!rproc) {
>> +        ret = -ENOMEM;
>> +        goto err;
>> +    }
>> +
>> +    dcfg = of_device_get_match_data(dev);
>> +    if (!dcfg)
>> +        return -EINVAL;
>
> move this up to the beginning of the function. You can avoid the cleanup
> (you are missing an rproc_free() here)
>
>> +
>> +    priv = rproc->priv;
>> +    priv->rproc = rproc;
>> +    priv->regmap = regmap;
>> +    priv->dcfg = dcfg;
>> +
>> +    dev_set_drvdata(dev, rproc);
>> +
>> +    ret = imx_rproc_addr_init(priv, pdev);
>> +    if (ret) {
>> +        dev_err(dev, "filed on imx_rproc_addr_init\n");
>> +        goto err_put_rproc;
>> +    }
>> +
>> +    priv->clk = devm_clk_get(dev, NULL);
>> +    if (IS_ERR(priv->clk)) {
>> +        dev_err(dev, "Failed to get clock\n");
>> +        return PTR_ERR(priv->clk);
>> +    }
>> +
>> +    ret = rproc_add(rproc);
>> +    if (ret) {
>> +        dev_err(dev, "rproc_add failed\n");
>> +        goto err_put_rproc;
>> +    }
>> +
>> +    ret = clk_prepare(priv->clk);
>> +    if (ret)
>> +        dev_err(dev, "failed to get clock\n");
>
> How are your internal memories clocked? If they need the same clock,
> rproc_add() would need the clock to be enabled for loading section data
> into those regions as part of rproc_boot(), and your current enable in
> imx_rproc_start() will be too late. Also, do not see a balancing
> clk_unprepare() call in remove, and a missing rproc_del() on failure here.
>
> regards
> Suman
>
>> +
>> +    return ret;
>> +
>> +err_put_rproc:
>> +    rproc_free(rproc);
>> +err:
>> +    return ret;
>> +}
>> +
>> +static int imx_rproc_remove(struct platform_device *pdev)
>> +{
>> +    struct rproc *rproc = platform_get_drvdata(pdev);
>> +
>> +    rproc_del(rproc);
>> +    rproc_free(rproc);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct platform_driver imx_rproc_driver = {
>> +    .probe = imx_rproc_probe,
>> +    .remove = imx_rproc_remove,
>> +    .driver = {
>> +        .name = "imx_rproc",
>> +        .of_match_table = imx_rproc_of_match,
>> +    },
>> +};
>> +
>> +module_platform_driver(imx_rproc_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("IMX6/7 remote processor control driver");
>> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
>>
>
>

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

Hi Oleksij,

On 06/19/2017 02:43 AM, Oleksij Rempel wrote:
> Hi,
> 
> Thank you for your review!
> 
> On 15.06.2017 21:01, Suman Anna wrote:
>> Hi Oleksij,
>>
>> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
>>> From: Oleksij Rempel <linux@rempel-privat.de>
>>>
>>> this driver was tested on NXP imx7d but should work on
>>> imx6sx as well.
>>> It will upload firmware to OCRAM, which shared memory between
>>> Cortex A7 and Cortex M4, then turn M4 on.
>>
>> Mostly looks fine, need to address few comments. I take it that you
>> haven't added the binding since this is just an RFC.
>>
>>>
>>> Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
>>> ---
>>>   drivers/remoteproc/Kconfig     |   8 ++
>>>   drivers/remoteproc/Makefile    |   1 +
>>>   drivers/remoteproc/imx_rproc.c | 264
>>> +++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 273 insertions(+)
>>>   create mode 100644 drivers/remoteproc/imx_rproc.c
>>>
>>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>>> index faad69a1a597..8d33bff4f530 100644
>>> --- a/drivers/remoteproc/Kconfig
>>> +++ b/drivers/remoteproc/Kconfig
>>> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>>>       tristate
>>>       depends on REMOTEPROC
>>>   +config IMX_REMOTEPROC
>>> +    tristate "IMX6/7 remoteproc support"
>>> +    depends on SOC_IMX6SX || SOC_IMX7D
>>> +    depends on REMOTEPROC
>>> +    help
>>> +      TODO, write some thing here
>>> +      This can be either built-in or a loadable module.
>>> +
>>>   endif # REMOTEPROC
>>>     endmenu
>>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>>> index ffc5e430df27..d351c25cdb4e 100644
>>> --- a/drivers/remoteproc/Makefile
>>> +++ b/drivers/remoteproc/Makefile
>>> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y            += qcom_wcnss.o
>>>   qcom_wcnss_pil-y            += qcom_wcnss_iris.o
>>>   obj-$(CONFIG_ST_REMOTEPROC)        += st_remoteproc.o
>>>   obj-$(CONFIG_ST_SLIM_REMOTEPROC)    += st_slim_rproc.o
>>> +obj-$(CONFIG_IMX_REMOTEPROC)        += imx_rproc.o
>>> diff --git a/drivers/remoteproc/imx_rproc.c
>>> b/drivers/remoteproc/imx_rproc.c
>>> new file mode 100644
>>> index 000000000000..6bef85237a27
>>> --- /dev/null
>>> +++ b/drivers/remoteproc/imx_rproc.c
>>> @@ -0,0 +1,264 @@
>>> +/*
>>> + * Oleksij Rempel <o.rempel@pengutronix.de>
>>
>> Please add a blank line here.
>>
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License
>>> + * version 2 as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/err.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/remoteproc.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/mfd/syscon.h>
>>> +
>>> +#define IMX7D_ENABLE_M4            BIT(3)
>>> +#define IMX7D_SW_M4P_RST        BIT(2)
>>> +#define IMX7D_SW_M4C_RST        BIT(1)
>>> +#define IMX7D_SW_M4C_NON_SCLR_RST    BIT(0)
>>> +
>>> +#define IMX7D_M4_RST_MASK        0xf
>>> +
>>> +
>>> +#define IMX7D_RPROC_MEM_MAX        2
>>> +enum {
>>> +    IMX7D_RPROC_IMEM,
>>> +    IMX7D_RPROC_DMEM,
>>> +};
>>> +
>>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
>>> +    [IMX7D_RPROC_IMEM]    = "imem",
>>> +    [IMX7D_RPROC_DMEM]    = "dmem",
>>> +};
>>
>> Do you really need these to be globally defined? You only need them in
>> the addr_init function, they can be made local to that function.
> 
> I don't needed. At least not with my testing remote code.
> It is mostly copy/paste from existing drivers.
> 
> But according to this page, there are multiple memory regions, with
> different mapping for data and code.
> http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas
> 
> 
> "The Cortex-M4 CPU has two buses connected to the main interconnect
> (modified Harvard architecture). One bus is meant to fetch data (system
> bus) whereas the other bus is meant to fetch instructions (code bus). To
> get optimal performance, the program code should be located and linked
> for a region which is going to be fetched through the code bus, while
> the data area (e.g. bss or data section) should be located in a region
> which is fetched through the system bus.

Yeah that's standard Cortex-M4 address/bus access architecture based on
memory addresses it sees, and the addresses are as per what the CPU
views them at.

 There are multiple example
> linker files in the platform/devices/MCIMX7D/linker/ sub directory which
> can be used and/or modified. All example firmware below use the
> MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region
> for data)."
> 
> What is the proper way to implement it with remoteproc?

So, TCML and TCMU looks to be internal memories within the Cortex-M4
subsystem from the link you shared. The rproc_da_to_va() is being used
today to provide the translations from the CPU device address (da) to
the kernel virtual address for the same memory (va) so that memcpy can
be used for loading the section data into those memories. The ioremap
from the A7 should be using the bus addresses.


>>> +
>>> +/**
>>> + * struct imx_rproc_mem - slim internal memory structure
>>> + * @cpu_addr: MPU virtual address of the memory region
>>> + * @bus_addr: Bus address used to access the memory region
>>> + * @size: Size of the memory region
>>> + */
>>> +struct imx_rproc_mem {
>>> +    void __iomem *cpu_addr;
>>> +    phys_addr_t bus_addr;
>>
>> Are the rproc-view of these regions same as the bus addresses or
>> do they have their own local addresses? If latter, images that are built
>> for those will be using those addresses in which case you need some
>> offset calculation in the da_to_va ops?

Your .da_to_va() implementation is wrong atm as I pointed out, and it
probably worked for you since you only had one section(s) starting at
the base of the region(s).

regards
Suman

>>
>>> +    size_t size;
>>> +};
>>> +
>>> +struct imx_rproc_dcfg {
>>> +    int offset;
>>> +};
>>> +
>>> +struct imx_rproc {
>>> +    struct device            *dev;
>>> +    struct regmap            *regmap;
>>> +    struct rproc            *rproc;
>>> +    const struct imx_rproc_dcfg    *dcfg;
>>
>> No need of aligning the variables, you can get rid of the additional
>> whitespaces and maintain consistent style.
>>
>>> +    struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
>>> +    struct clk        *clk;
>>> +};
>>> +
>>> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
>>> +    .offset = 0xc,
>>> +};
>>> +
>>> +static int imx_rproc_start(struct rproc *rproc)
>>> +{
>>> +    struct imx_rproc *priv = rproc->priv;
>>> +    const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>>> +    struct device *dev = priv->dev;
>>> +    int ret;
>>> +
>>> +    ret = clk_enable(priv->clk);
>>> +    if (ret) {
>>> +        dev_err(&rproc->dev, "Failed to enable clock\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = regmap_update_bits(priv->regmap, dcfg->offset,
>>> +                 IMX7D_M4_RST_MASK,
>>> +                 IMX7D_SW_M4C_RST | IMX7D_SW_M4P_RST |
>>> IMX7D_ENABLE_M4);
>>> +    if (ret) {
>>> +        dev_err(dev, "Filed to enable M4!\n");
>>> +        clk_disable(priv->clk);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int imx_rproc_stop(struct rproc *rproc)
>>> +{
>>> +    struct imx_rproc *priv = rproc->priv;
>>> +    const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>>> +    struct device *dev = priv->dev;
>>> +    int ret;
>>> +
>>> +    ret = regmap_update_bits(priv->regmap, dcfg->offset,
>>> +                 IMX7D_M4_RST_MASK,
>>> +                 IMX7D_SW_M4C_NON_SCLR_RST);
>>> +    if (ret)
>>> +        dev_err(dev, "Filed to stop M4!\n");
>>> +
>>> +    clk_disable(priv->clk);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>>> +{
>>> +    struct imx_rproc *priv = rproc->priv;
>>> +    void *va = NULL;
>>> +    int i;
>>> +
>>> +    for (i = 0; i < IMX7D_RPROC_MEM_MAX; i++) {
>>> +        if (da != priv->mem[i].bus_addr)
>>> +            continue;
>>> +
>>> +        if (len <= priv->mem[i].size) {
>>> +            /* __force to make sparse happy with type conversion */
>>> +            va = (__force void *)priv->mem[i].cpu_addr;
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    dev_dbg(&rproc->dev, "da = 0x%llx len = 0x%x va = 0x%p\n", da,
>>> len, va);
>>> +
>>> +    return va;
>>> +}
>>> +
>>> +static const struct rproc_ops imx_rproc_ops = {
>>> +    .start        = imx_rproc_start,
>>> +    .stop        = imx_rproc_stop,
>>> +    .da_to_va       = imx_rproc_da_to_va,
>>> +};
>>> +
>>> +static const struct of_device_id imx_rproc_of_match[] = {
>>> +    { .compatible = "fsl,imx7d-rproc", .data = &imx_rproc_cfg_imx7d },
>>> +    {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, imx_rproc_of_match);
>>> +
>>> +static int imx_rproc_addr_init(struct imx_rproc *priv,
>>> +                   struct platform_device *pdev)
>>> +{
>>> +    int i, err;
>>> +    struct resource *res;
>>> +
>>> +    /* get imem and dmem */
>>> +    for (i = 0; i < ARRAY_SIZE(mem_names); i++) {
>>> +        res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>> +                        mem_names[i]);
>>> +        if (!res)
>>> +            continue;
>>> +
>>> +        priv->mem[i].cpu_addr = devm_ioremap_resource(&pdev->dev, res);
>>> +        if (IS_ERR(priv->mem[i].cpu_addr)) {
>>> +            dev_err(&pdev->dev, "devm_ioremap_resource failed\n");
>>> +            err = PTR_ERR(priv->mem[i].cpu_addr);
>>> +            return err;
>>> +        }
>>> +        priv->mem[i].bus_addr = res->start & 0xffff;
>>> +        priv->mem[i].size = resource_size(res);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int imx_rproc_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct device_node *np = dev->of_node;
>>> +    struct imx_rproc *priv;
>>> +    struct rproc *rproc;
>>> +    struct regmap_config config = { .name = "imx_rproc" };
>>> +    const struct imx_rproc_dcfg *dcfg;
>>> +    struct regmap *regmap;
>>> +    int ret;
>>> +
>>> +    regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
>>> +    if (IS_ERR(regmap)) {
>>> +        dev_err(dev, "failed to find syscon\n");
>>> +        return PTR_ERR(regmap);
>>> +    }
>>> +        regmap_attach_dev(dev, regmap, &config);
>>> +
>>> +    /* set some other name then imx */
>>> +    rproc = rproc_alloc(dev, "imx_rproc", &imx_rproc_ops, NULL,
>>> sizeof(*priv));
>>> +    if (!rproc) {
>>> +        ret = -ENOMEM;
>>> +        goto err;
>>> +    }
>>> +
>>> +    dcfg = of_device_get_match_data(dev);
>>> +    if (!dcfg)
>>> +        return -EINVAL;
>>
>> move this up to the beginning of the function. You can avoid the cleanup
>> (you are missing an rproc_free() here)
>>
>>> +
>>> +    priv = rproc->priv;
>>> +    priv->rproc = rproc;
>>> +    priv->regmap = regmap;
>>> +    priv->dcfg = dcfg;
>>> +
>>> +    dev_set_drvdata(dev, rproc);
>>> +
>>> +    ret = imx_rproc_addr_init(priv, pdev);
>>> +    if (ret) {
>>> +        dev_err(dev, "filed on imx_rproc_addr_init\n");
>>> +        goto err_put_rproc;
>>> +    }
>>> +
>>> +    priv->clk = devm_clk_get(dev, NULL);
>>> +    if (IS_ERR(priv->clk)) {
>>> +        dev_err(dev, "Failed to get clock\n");
>>> +        return PTR_ERR(priv->clk);
>>> +    }
>>> +
>>> +    ret = rproc_add(rproc);
>>> +    if (ret) {
>>> +        dev_err(dev, "rproc_add failed\n");
>>> +        goto err_put_rproc;
>>> +    }
>>> +
>>> +    ret = clk_prepare(priv->clk);
>>> +    if (ret)
>>> +        dev_err(dev, "failed to get clock\n");
>>
>> How are your internal memories clocked? If they need the same clock,
>> rproc_add() would need the clock to be enabled for loading section data
>> into those regions as part of rproc_boot(), and your current enable in
>> imx_rproc_start() will be too late. Also, do not see a balancing
>> clk_unprepare() call in remove, and a missing rproc_del() on failure
>> here.
>>
>> regards
>> Suman
>>
>>> +
>>> +    return ret;
>>> +
>>> +err_put_rproc:
>>> +    rproc_free(rproc);
>>> +err:
>>> +    return ret;
>>> +}
>>> +
>>> +static int imx_rproc_remove(struct platform_device *pdev)
>>> +{
>>> +    struct rproc *rproc = platform_get_drvdata(pdev);
>>> +
>>> +    rproc_del(rproc);
>>> +    rproc_free(rproc);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static struct platform_driver imx_rproc_driver = {
>>> +    .probe = imx_rproc_probe,
>>> +    .remove = imx_rproc_remove,
>>> +    .driver = {
>>> +        .name = "imx_rproc",
>>> +        .of_match_table = imx_rproc_of_match,
>>> +    },
>>> +};
>>> +
>>> +module_platform_driver(imx_rproc_driver);
>>> +
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_DESCRIPTION("IMX6/7 remote processor control driver");
>>> +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
>>>
>>
>>

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Oleksij Rempel]

On Wed, Jun 21, 2017 at 04:09:26PM -0500, Suman Anna wrote:
> Hi Oleksij,
> 
> On 06/19/2017 02:43 AM, Oleksij Rempel wrote:
> > Hi,
> > 
> > Thank you for your review!
> > 
> > On 15.06.2017 21:01, Suman Anna wrote:
> >> Hi Oleksij,
> >>
> >> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
> >>> From: Oleksij Rempel <linux@rempel-privat.de>
> >>>
> >>> this driver was tested on NXP imx7d but should work on
> >>> imx6sx as well.
> >>> It will upload firmware to OCRAM, which shared memory between
> >>> Cortex A7 and Cortex M4, then turn M4 on.
> >>
> >> Mostly looks fine, need to address few comments. I take it that you
> >> haven't added the binding since this is just an RFC.
> >>
.....
> >>> +
> >>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
> >>> +    [IMX7D_RPROC_IMEM]    = "imem",
> >>> +    [IMX7D_RPROC_DMEM]    = "dmem",
> >>> +};
> >>
> >> Do you really need these to be globally defined? You only need them in
> >> the addr_init function, they can be made local to that function.
> > 
> > I don't needed. At least not with my testing remote code.
> > It is mostly copy/paste from existing drivers.
> > 
> > But according to this page, there are multiple memory regions, with
> > different mapping for data and code.
> > http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas
> > 
> > 
> > "The Cortex-M4 CPU has two buses connected to the main interconnect
> > (modified Harvard architecture). One bus is meant to fetch data (system
> > bus) whereas the other bus is meant to fetch instructions (code bus). To
> > get optimal performance, the program code should be located and linked
> > for a region which is going to be fetched through the code bus, while
> > the data area (e.g. bss or data section) should be located in a region
> > which is fetched through the system bus.
> 
> Yeah that's standard Cortex-M4 address/bus access architecture based on
> memory addresses it sees, and the addresses are as per what the CPU
> views them at.
> 
>  There are multiple example
> > linker files in the platform/devices/MCIMX7D/linker/ sub directory which
> > can be used and/or modified. All example firmware below use the
> > MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region
> > for data)."
> > 
> > What is the proper way to implement it with remoteproc?
> 
> So, TCML and TCMU looks to be internal memories within the Cortex-M4
> subsystem from the link you shared. The rproc_da_to_va() is being used
> today to provide the translations from the CPU device address (da) to
> the kernel virtual address for the same memory (va) so that memcpy can
> be used for loading the section data into those memories. The ioremap
> from the A7 should be using the bus addresses.

I was reading linker and Make files provided by the git repo in the page
 posted before. So far:
- different app examples can use different linker configurations
  (MCIMX7D_M4_ddr.ld, MCIMX7D_M4_ocram.ld or MCIMX7D_M4_tcm.ld)
- only one configuration was used at the time.

I assume, to cover all this cases:
- for each momory range should be created own DT node, with probably own
  clock entry.  At leas *_tcm.ld and *_ocram.ld seems to fit in this pattern.

Should actually all possible variants be covered?

what is the best practice to proceed with shared ddr space? Should it be
reserved-memory node in DT?
Should the translation map created in DeviceTree or in driver?

> 
> >>> +
> >>> +/**
> >>> + * struct imx_rproc_mem - slim internal memory structure
> >>> + * @cpu_addr: MPU virtual address of the memory region
> >>> + * @bus_addr: Bus address used to access the memory region
> >>> + * @size: Size of the memory region
> >>> + */
> >>> +};
> >>> +
> >>> +struct imx_rproc_dcfg {
> >>> +    int offset;
> >>> +};
> >>> +
> >>> +struct imx_rproc {
> >>> +    struct device            *dev;
> >>> +    struct regmap            *regmap;
> >>> +    struct rproc            *rproc;
> >>> +    const struct imx_rproc_dcfg    *dcfg;
> >>

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Suman Anna]

On 06/23/2017 09:35 AM, Oleksij Rempel wrote:
> On Wed, Jun 21, 2017 at 04:09:26PM -0500, Suman Anna wrote:
>> Hi Oleksij,
>>
>> On 06/19/2017 02:43 AM, Oleksij Rempel wrote:
>>> Hi,
>>>
>>> Thank you for your review!
>>>
>>> On 15.06.2017 21:01, Suman Anna wrote:
>>>> Hi Oleksij,
>>>>
>>>> On 06/14/2017 03:48 PM, Oleksij Rempel wrote:
>>>>> From: Oleksij Rempel <linux@rempel-privat.de>
>>>>>
>>>>> this driver was tested on NXP imx7d but should work on
>>>>> imx6sx as well.
>>>>> It will upload firmware to OCRAM, which shared memory between
>>>>> Cortex A7 and Cortex M4, then turn M4 on.
>>>>
>>>> Mostly looks fine, need to address few comments. I take it that you
>>>> haven't added the binding since this is just an RFC.
>>>>
> .....
>>>>> +
>>>>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
>>>>> +    [IMX7D_RPROC_IMEM]    = "imem",
>>>>> +    [IMX7D_RPROC_DMEM]    = "dmem",
>>>>> +};
>>>>
>>>> Do you really need these to be globally defined? You only need them in
>>>> the addr_init function, they can be made local to that function.
>>>
>>> I don't needed. At least not with my testing remote code.
>>> It is mostly copy/paste from existing drivers.
>>>
>>> But according to this page, there are multiple memory regions, with
>>> different mapping for data and code.
>>> http://developer.toradex.com/knowledge-base/freertos-on-the-cortex-m4-of-a-colibri-imx7#Memory_areas
>>>
>>>
>>> "The Cortex-M4 CPU has two buses connected to the main interconnect
>>> (modified Harvard architecture). One bus is meant to fetch data (system
>>> bus) whereas the other bus is meant to fetch instructions (code bus). To
>>> get optimal performance, the program code should be located and linked
>>> for a region which is going to be fetched through the code bus, while
>>> the data area (e.g. bss or data section) should be located in a region
>>> which is fetched through the system bus.
>>
>> Yeah that's standard Cortex-M4 address/bus access architecture based on
>> memory addresses it sees, and the addresses are as per what the CPU
>> views them at.
>>
>>  There are multiple example
>>> linker files in the platform/devices/MCIMX7D/linker/ sub directory which
>>> can be used and/or modified. All example firmware below use the
>>> MCIMX7D_M4_tcm.ld linker file (TCML region for code, and the TCMU region
>>> for data)."
>>>
>>> What is the proper way to implement it with remoteproc?
>>
>> So, TCML and TCMU looks to be internal memories within the Cortex-M4
>> subsystem from the link you shared. The rproc_da_to_va() is being used
>> today to provide the translations from the CPU device address (da) to
>> the kernel virtual address for the same memory (va) so that memcpy can
>> be used for loading the section data into those memories. The ioremap
>> from the A7 should be using the bus addresses.
> 
> I was reading linker and Make files provided by the git repo in the page
>  posted before. So far:
> - different app examples can use different linker configurations
>   (MCIMX7D_M4_ddr.ld, MCIMX7D_M4_ocram.ld or MCIMX7D_M4_tcm.ld)
> - only one configuration was used at the time.
> 
> I assume, to cover all this cases:
> - for each momory range should be created own DT node, with probably own
>   clock entry.  At leas *_tcm.ld and *_ocram.ld seems to fit in this pattern.
> 
> Should actually all possible variants be covered?

The TCML and TCMU are internal to your Cortex-M4 subsystem, so I expect
them to be defined within the corresponding node. This looks like the
minimum you would want to start with (given that it seems to be most
used in those examples). Obviously, the others depends on what you want
to support.

> 
> what is the best practice to proceed with shared ddr space? Should it be
> reserved-memory node in DT?

Yeah, reserved-memory node (CMA or carveout) in board files is what most
of the rproc drivers have been using so far. OCRAM and DDR usage may
vary from your use-case to use-case. I assume the OCRAM is the on-chip
RAM, so I take it that it will have its own DTS node (mmio-sram), and
reference it using a phandle in your remoteproc node. Usage will depend
on whether you need a fixed location or if you can allocate memory
dynamically.

> Should the translation map created in DeviceTree or in driver?

On TI devices, I have provided it through the driver.

regards
Suman

> 
>>
>>>>> +
>>>>> +/**
>>>>> + * struct imx_rproc_mem - slim internal memory structure
>>>>> + * @cpu_addr: MPU virtual address of the memory region
>>>>> + * @bus_addr: Bus address used to access the memory region
>>>>> + * @size: Size of the memory region
>>>>> + */
>>>>> +};
>>>>> +
>>>>> +struct imx_rproc_dcfg {
>>>>> +    int offset;
>>>>> +};
>>>>> +
>>>>> +struct imx_rproc {
>>>>> +    struct device            *dev;
>>>>> +    struct regmap            *regmap;
>>>>> +    struct rproc            *rproc;
>>>>> +    const struct imx_rproc_dcfg    *dcfg;
>>>>
> 

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Bjorn Andersson]

On Wed 14 Jun 13:48 PDT 2017, Oleksij Rempel wrote:

> From: Oleksij Rempel <linux@rempel-privat.de>
> 
> this driver was tested on NXP imx7d but should work on
> imx6sx as well.
> It will upload firmware to OCRAM, which shared memory between
> Cortex A7 and Cortex M4, then turn M4 on.
> 

This looks very generic, are there any IMX specific parts or is this the
"default" way of integrating a M4? Could we perhaps come up with a
common M4-rproc driver?


Even though it will be rather short, you need a DT binding document
defining the compatible and the few properties that you use.

> Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
> ---
>  drivers/remoteproc/Kconfig     |   8 ++
>  drivers/remoteproc/Makefile    |   1 +
>  drivers/remoteproc/imx_rproc.c | 264 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 273 insertions(+)
>  create mode 100644 drivers/remoteproc/imx_rproc.c
> 
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index faad69a1a597..8d33bff4f530 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>  	tristate
>  	depends on REMOTEPROC
>  
> +config IMX_REMOTEPROC

Please make an attempt to keep entries in the Kconfig and Makefile
alphabetically sorted.

> +	tristate "IMX6/7 remoteproc support"
> +	depends on SOC_IMX6SX || SOC_IMX7D
> +	depends on REMOTEPROC
> +	help
> +	  TODO, write some thing here
> +	  This can be either built-in or a loadable module.
> +
>  endif # REMOTEPROC
>  
>  endmenu
> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> index ffc5e430df27..d351c25cdb4e 100644
> --- a/drivers/remoteproc/Makefile
> +++ b/drivers/remoteproc/Makefile
> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y			+= qcom_wcnss.o
>  qcom_wcnss_pil-y			+= qcom_wcnss_iris.o
>  obj-$(CONFIG_ST_REMOTEPROC)		+= st_remoteproc.o
>  obj-$(CONFIG_ST_SLIM_REMOTEPROC)	+= st_slim_rproc.o
> +obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> new file mode 100644
> index 000000000000..6bef85237a27
> --- /dev/null
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -0,0 +1,264 @@
> +/*
> + * Oleksij Rempel <o.rempel@pengutronix.de>
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/remoteproc.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define IMX7D_ENABLE_M4			BIT(3)
> +#define IMX7D_SW_M4P_RST		BIT(2)
> +#define IMX7D_SW_M4C_RST		BIT(1)
> +#define IMX7D_SW_M4C_NON_SCLR_RST	BIT(0)
> +
> +#define IMX7D_M4_RST_MASK		0xf
> +
> +
> +#define IMX7D_RPROC_MEM_MAX		2
> +enum {
> +	IMX7D_RPROC_IMEM,
> +	IMX7D_RPROC_DMEM,
> +};
> +
> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
> +	[IMX7D_RPROC_IMEM]	= "imem",
> +	[IMX7D_RPROC_DMEM]	= "dmem",
> +};
> +
> +/**
> + * struct imx_rproc_mem - slim internal memory structure
> + * @cpu_addr: MPU virtual address of the memory region
> + * @bus_addr: Bus address used to access the memory region
> + * @size: Size of the memory region
> + */
> +struct imx_rproc_mem {
> +	void __iomem *cpu_addr;
> +	phys_addr_t bus_addr;
> +	size_t size;
> +};
> +
> +struct imx_rproc_dcfg {
> +	int offset;
> +};

Unless you foresee this being expanded in the very near future I would
prefer that you just inline the offset in imx_rproc and put (void*)0xc
as .data in your of_match (typecast the of_device_get_match_data result
to unsigned long).

> +
> +struct imx_rproc {
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct rproc			*rproc;
> +	const struct imx_rproc_dcfg	*dcfg;
> +	struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
> +	struct clk		*clk;
> +};
> +
> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
> +	.offset = 0xc,
> +};
> +
> +static int imx_rproc_start(struct rproc *rproc)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> +	struct device *dev = priv->dev;
> +	int ret;
> +
> +	ret = clk_enable(priv->clk);
> +	if (ret) {
> +		dev_err(&rproc->dev, "Failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(priv->regmap, dcfg->offset,
> +				 IMX7D_M4_RST_MASK,
> +				 IMX7D_SW_M4C_RST | IMX7D_SW_M4P_RST | IMX7D_ENABLE_M4);
> +	if (ret) {
> +		dev_err(dev, "Filed to enable M4!\n");
> +		clk_disable(priv->clk);
> +	}
> +
> +	return ret;
> +}
> +
> +static int imx_rproc_stop(struct rproc *rproc)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
> +	struct device *dev = priv->dev;
> +	int ret;
> +
> +	ret = regmap_update_bits(priv->regmap, dcfg->offset,
> +				 IMX7D_M4_RST_MASK,
> +				 IMX7D_SW_M4C_NON_SCLR_RST);
> +	if (ret)
> +		dev_err(dev, "Filed to stop M4!\n");
> +
> +	clk_disable(priv->clk);
> +
> +	return ret;
> +}
> +
> +static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
> +{
> +	struct imx_rproc *priv = rproc->priv;
> +	void *va = NULL;
> +	int i;
> +
> +	for (i = 0; i < IMX7D_RPROC_MEM_MAX; i++) {
> +		if (da != priv->mem[i].bus_addr)
> +			continue;
> +
> +		if (len <= priv->mem[i].size) {
> +			/* __force to make sparse happy with type conversion */
> +			va = (__force void *)priv->mem[i].cpu_addr;
> +			break;
> +		}
> +	}
> +
> +	dev_dbg(&rproc->dev, "da = 0x%llx len = 0x%x va = 0x%p\n", da, len, va);
> +
> +	return va;
> +}
> +
> +static const struct rproc_ops imx_rproc_ops = {
> +	.start		= imx_rproc_start,
> +	.stop		= imx_rproc_stop,
> +	.da_to_va       = imx_rproc_da_to_va,
> +};
> +
> +static const struct of_device_id imx_rproc_of_match[] = {
> +	{ .compatible = "fsl,imx7d-rproc", .data = &imx_rproc_cfg_imx7d },

.data = (void*)0xc

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, imx_rproc_of_match);

By using of_device_get_match_data() you don't need to reference your
of_device_id table, so please move it down to your definition of
imx_rproc_driver.

[..]
> +static int imx_rproc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct imx_rproc *priv;
> +	struct rproc *rproc;
> +	struct regmap_config config = { .name = "imx_rproc" };
> +	const struct imx_rproc_dcfg *dcfg;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "failed to find syscon\n");
> +		return PTR_ERR(regmap);
> +	}
> +        regmap_attach_dev(dev, regmap, &config);

Indentation...

> +
> +	/* set some other name then imx */
> +	rproc = rproc_alloc(dev, "imx_rproc", &imx_rproc_ops, NULL, sizeof(*priv));
> +	if (!rproc) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	dcfg = of_device_get_match_data(dev);

priv->cfg_offset = (unsigned long)of_device_get_match_data(dev);

> +	if (!dcfg)
> +		return -EINVAL;
> +
> +	priv = rproc->priv;
> +	priv->rproc = rproc;
> +	priv->regmap = regmap;
> +	priv->dcfg = dcfg;
> +
> +	dev_set_drvdata(dev, rproc);
> +
> +	ret = imx_rproc_addr_init(priv, pdev);
> +	if (ret) {
> +		dev_err(dev, "filed on imx_rproc_addr_init\n");
> +		goto err_put_rproc;
> +	}
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		dev_err(dev, "Failed to get clock\n");

rproc_free()

> +		return PTR_ERR(priv->clk);
> +	}
> +
> +	ret = rproc_add(rproc);
> +	if (ret) {
> +		dev_err(dev, "rproc_add failed\n");
> +		goto err_put_rproc;
> +	}
> +
> +	ret = clk_prepare(priv->clk);

1) You're not unpreparing this clock in remove.

2) Why do you prepare the clock here instead of prepare_enable &
disable_unprepare it during start/stop?

3) This is racy as you could have start() being called between
rproc_add() and clk_prepare() (although highly unlikely...).

> +	if (ret)
> +		dev_err(dev, "failed to get clock\n");

s/get/prepare/

Regards,
Bjorn

Re: [RFC PATCH 2/3] remoteproc: imx_rproc: add a NXP/Freescale imx rproc driver

[Oleksij Rempel]

Hi Bjorn,

thank you for review,

On 25.06.2017 22:41, Bjorn Andersson wrote:
> On Wed 14 Jun 13:48 PDT 2017, Oleksij Rempel wrote:
>
>> From: Oleksij Rempel <linux@rempel-privat.de>
>>
>> this driver was tested on NXP imx7d but should work on
>> imx6sx as well.
>> It will upload firmware to OCRAM, which shared memory between
>> Cortex A7 and Cortex M4, then turn M4 on.
>>
>
> This looks very generic, are there any IMX specific parts or is this the
> "default" way of integrating a M4? Could we perhaps come up with a
> common M4-rproc driver?

It is kind of generic. The differences are:
- platform specific clocks/reset/mbox configuration. So, maybe generic 
driver just should provide pre and post start/stop bindings?
- arrays for address translations. Naming seems not to be not really 
important. In my case:
  1) M4 own RAM for best performance. This can be requested immediately 
by rptoc driver.
  2) SRAM. Can be used by other parts of the system. Probably 
application specific, not good for mainline.
  3) limited range of DDR RAM. This should be dynamically allocated on 
request.

For (1) most drivers seem to have nearly identical code which IMO can be 
shared.

> Even though it will be rather short, you need a DT binding document
> defining the compatible and the few properties that you use.
>
>> Signed-off-by: Oleksij Rempel <linux@rempel-privat.de>
>> ---
>>  drivers/remoteproc/Kconfig     |   8 ++
>>  drivers/remoteproc/Makefile    |   1 +
>>  drivers/remoteproc/imx_rproc.c | 264 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 273 insertions(+)
>>  create mode 100644 drivers/remoteproc/imx_rproc.c
>>
>> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
>> index faad69a1a597..8d33bff4f530 100644
>> --- a/drivers/remoteproc/Kconfig
>> +++ b/drivers/remoteproc/Kconfig
>> @@ -130,6 +130,14 @@ config ST_SLIM_REMOTEPROC
>>  	tristate
>>  	depends on REMOTEPROC
>>
>> +config IMX_REMOTEPROC
>
> Please make an attempt to keep entries in the Kconfig and Makefile
> alphabetically sorted.
>
>> +	tristate "IMX6/7 remoteproc support"
>> +	depends on SOC_IMX6SX || SOC_IMX7D
>> +	depends on REMOTEPROC
>> +	help
>> +	  TODO, write some thing here
>> +	  This can be either built-in or a loadable module.
>> +
>>  endif # REMOTEPROC
>>
>>  endmenu
>> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
>> index ffc5e430df27..d351c25cdb4e 100644
>> --- a/drivers/remoteproc/Makefile
>> +++ b/drivers/remoteproc/Makefile
>> @@ -19,3 +19,4 @@ qcom_wcnss_pil-y			+= qcom_wcnss.o
>>  qcom_wcnss_pil-y			+= qcom_wcnss_iris.o
>>  obj-$(CONFIG_ST_REMOTEPROC)		+= st_remoteproc.o
>>  obj-$(CONFIG_ST_SLIM_REMOTEPROC)	+= st_slim_rproc.o
>> +obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
>> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
>> new file mode 100644
>> index 000000000000..6bef85237a27
>> --- /dev/null
>> +++ b/drivers/remoteproc/imx_rproc.c
>> @@ -0,0 +1,264 @@
>> +/*
>> + * Oleksij Rempel <o.rempel@pengutronix.de>
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/remoteproc.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define IMX7D_ENABLE_M4			BIT(3)
>> +#define IMX7D_SW_M4P_RST		BIT(2)
>> +#define IMX7D_SW_M4C_RST		BIT(1)
>> +#define IMX7D_SW_M4C_NON_SCLR_RST	BIT(0)
>> +
>> +#define IMX7D_M4_RST_MASK		0xf
>> +
>> +
>> +#define IMX7D_RPROC_MEM_MAX		2
>> +enum {
>> +	IMX7D_RPROC_IMEM,
>> +	IMX7D_RPROC_DMEM,
>> +};
>> +
>> +static const char *mem_names[IMX7D_RPROC_MEM_MAX] = {
>> +	[IMX7D_RPROC_IMEM]	= "imem",
>> +	[IMX7D_RPROC_DMEM]	= "dmem",
>> +};
>> +
>> +/**
>> + * struct imx_rproc_mem - slim internal memory structure
>> + * @cpu_addr: MPU virtual address of the memory region
>> + * @bus_addr: Bus address used to access the memory region
>> + * @size: Size of the memory region
>> + */
>> +struct imx_rproc_mem {
>> +	void __iomem *cpu_addr;
>> +	phys_addr_t bus_addr;
>> +	size_t size;
>> +};
>> +
>> +struct imx_rproc_dcfg {
>> +	int offset;
>> +};
>
> Unless you foresee this being expanded in the very near future I would
> prefer that you just inline the offset in imx_rproc and put (void*)0xc
> as .data in your of_match (typecast the of_device_get_match_data result
> to unsigned long).
>
>> +
>> +struct imx_rproc {
>> +	struct device			*dev;
>> +	struct regmap			*regmap;
>> +	struct rproc			*rproc;
>> +	const struct imx_rproc_dcfg	*dcfg;
>> +	struct imx_rproc_mem mem[IMX7D_RPROC_MEM_MAX];
>> +	struct clk		*clk;
>> +};
>> +
>> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx7d = {
>> +	.offset = 0xc,
>> +};
>> +
>> +static int imx_rproc_start(struct rproc *rproc)
>> +{
>> +	struct imx_rproc *priv = rproc->priv;
>> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>> +	struct device *dev = priv->dev;
>> +	int ret;
>> +
>> +	ret = clk_enable(priv->clk);
>> +	if (ret) {
>> +		dev_err(&rproc->dev, "Failed to enable clock\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = regmap_update_bits(priv->regmap, dcfg->offset,
>> +				 IMX7D_M4_RST_MASK,
>> +				 IMX7D_SW_M4C_RST | IMX7D_SW_M4P_RST | IMX7D_ENABLE_M4);
>> +	if (ret) {
>> +		dev_err(dev, "Filed to enable M4!\n");
>> +		clk_disable(priv->clk);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int imx_rproc_stop(struct rproc *rproc)
>> +{
>> +	struct imx_rproc *priv = rproc->priv;
>> +	const struct imx_rproc_dcfg *dcfg = priv->dcfg;
>> +	struct device *dev = priv->dev;
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(priv->regmap, dcfg->offset,
>> +				 IMX7D_M4_RST_MASK,
>> +				 IMX7D_SW_M4C_NON_SCLR_RST);
>> +	if (ret)
>> +		dev_err(dev, "Filed to stop M4!\n");
>> +
>> +	clk_disable(priv->clk);
>> +
>> +	return ret;
>> +}
>> +
>> +static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, int len)
>> +{
>> +	struct imx_rproc *priv = rproc->priv;
>> +	void *va = NULL;
>> +	int i;
>> +
>> +	for (i = 0; i < IMX7D_RPROC_MEM_MAX; i++) {
>> +		if (da != priv->mem[i].bus_addr)
>> +			continue;
>> +
>> +		if (len <= priv->mem[i].size) {
>> +			/* __force to make sparse happy with type conversion */
>> +			va = (__force void *)priv->mem[i].cpu_addr;
>> +			break;
>> +		}
>> +	}
>> +
>> +	dev_dbg(&rproc->dev, "da = 0x%llx len = 0x%x va = 0x%p\n", da, len, va);
>> +
>> +	return va;
>> +}
>> +
>> +static const struct rproc_ops imx_rproc_ops = {
>> +	.start		= imx_rproc_start,
>> +	.stop		= imx_rproc_stop,
>> +	.da_to_va       = imx_rproc_da_to_va,
>> +};
>> +
>> +static const struct of_device_id imx_rproc_of_match[] = {
>> +	{ .compatible = "fsl,imx7d-rproc", .data = &imx_rproc_cfg_imx7d },
>
> .data = (void*)0xc
>
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, imx_rproc_of_match);
>
> By using of_device_get_match_data() you don't need to reference your
> of_device_id table, so please move it down to your definition of
> imx_rproc_driver.
>
> [..]
>> +static int imx_rproc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct imx_rproc *priv;
>> +	struct rproc *rproc;
>> +	struct regmap_config config = { .name = "imx_rproc" };
>> +	const struct imx_rproc_dcfg *dcfg;
>> +	struct regmap *regmap;
>> +	int ret;
>> +
>> +	regmap = syscon_regmap_lookup_by_phandle(np, "syscon");
>> +	if (IS_ERR(regmap)) {
>> +		dev_err(dev, "failed to find syscon\n");
>> +		return PTR_ERR(regmap);
>> +	}
>> +        regmap_attach_dev(dev, regmap, &config);
>
> Indentation...
>
>> +
>> +	/* set some other name then imx */
>> +	rproc = rproc_alloc(dev, "imx_rproc", &imx_rproc_ops, NULL, sizeof(*priv));
>> +	if (!rproc) {
>> +		ret = -ENOMEM;
>> +		goto err;
>> +	}
>> +
>> +	dcfg = of_device_get_match_data(dev);
>
> priv->cfg_offset = (unsigned long)of_device_get_match_data(dev);
>
>> +	if (!dcfg)
>> +		return -EINVAL;
>> +
>> +	priv = rproc->priv;
>> +	priv->rproc = rproc;
>> +	priv->regmap = regmap;
>> +	priv->dcfg = dcfg;
>> +
>> +	dev_set_drvdata(dev, rproc);
>> +
>> +	ret = imx_rproc_addr_init(priv, pdev);
>> +	if (ret) {
>> +		dev_err(dev, "filed on imx_rproc_addr_init\n");
>> +		goto err_put_rproc;
>> +	}
>> +
>> +	priv->clk = devm_clk_get(dev, NULL);
>> +	if (IS_ERR(priv->clk)) {
>> +		dev_err(dev, "Failed to get clock\n");
>
> rproc_free()
>
>> +		return PTR_ERR(priv->clk);
>> +	}
>> +
>> +	ret = rproc_add(rproc);
>> +	if (ret) {
>> +		dev_err(dev, "rproc_add failed\n");
>> +		goto err_put_rproc;
>> +	}
>> +
>> +	ret = clk_prepare(priv->clk);
>
> 1) You're not unpreparing this clock in remove.
>
> 2) Why do you prepare the clock here instead of prepare_enable &
> disable_unprepare it during start/stop?
>
> 3) This is racy as you could have start() being called between
> rproc_add() and clk_prepare() (although highly unlikely...).
>
>> +	if (ret)
>> +		dev_err(dev, "failed to get clock\n");
>
> s/get/prepare/
>
> Regards,
> Bjorn
>