[PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg]

From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>

The cm-fx6 module has an on-board spi-flash chip for its firmware, an
eeprom (containing e.g. the mac address of the on-board Ethernet),
a sata port, a pcie controller, an USB hub, and an USB otg port.
Enable support for them. In addition, enable syscon poweroff support.

Signed-off-by: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
---
 arch/arm/boot/dts/imx6q-cm-fx6.dts | 136 +++++++++++++++++++++++++++++++++++++
 1 file changed, 136 insertions(+)

diff --git a/arch/arm/boot/dts/imx6q-cm-fx6.dts b/arch/arm/boot/dts/imx6q-cm-fx6.dts
index 99b46f8..f4fc22e 100644
--- a/arch/arm/boot/dts/imx6q-cm-fx6.dts
+++ b/arch/arm/boot/dts/imx6q-cm-fx6.dts
@@ -31,6 +31,61 @@
 			linux,default-trigger = "heartbeat";
 		};
 	};
+
+	regulators {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		reg_usb_otg_vbus: usb_otg_vbus {
+			compatible = "regulator-fixed";
+			regulator-name = "usb_otg_vbus";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			gpio = <&gpio3 22 0>;
+			enable-active-high;
+		};
+
+		reg_usb_h1_vbus: usb_h1_vbus {
+			compatible = "regulator-fixed";
+			regulator-name = "usb_h1_vbus";
+			regulator-min-microvolt = <5000000>;
+			regulator-max-microvolt = <5000000>;
+			gpio = <&gpio7 8 0>;
+			enable-active-high;
+		};
+	};
+};
+
+&ecspi1 {
+	fsl,spi-num-chipselects = <2>;
+	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_ecspi1>;
+	status = "okay";
+
+	flash: m25p80@0 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "st,m25p", "jedec,spi-nor";
+		spi-max-frequency = <20000000>;
+		reg = <0>;
+
+		partition@0 {
+			label = "uboot";
+			reg = <0x0 0xc0000>;
+		};
+
+		partition@c0000 {
+			label = "uboot environment";
+			reg = <0xc0000 0x40000>;
+		};
+
+		partition@100000 {
+			label = "reserved";
+			reg = <0x100000 0x100000>;
+		};
+	};
 };
 
 &fec {
@@ -46,8 +101,31 @@
 	status = "okay";
 };
 
+&i2c3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c3>;
+	status = "okay";
+	clock-frequency = <100000>;
+
+	eeprom@50 {
+		compatible = "at24,24c02";
+		reg = <0x50>;
+		pagesize = <16>;
+	};
+};
+
 &iomuxc {
 	imx6q-cm-fx6 {
+		pinctrl_ecspi1: ecspi1grp {
+			fsl,pins = <
+				MX6QDL_PAD_EIM_D16__ECSPI1_SCLK		 0x100b1
+				MX6QDL_PAD_EIM_D17__ECSPI1_MISO		 0x100b1
+				MX6QDL_PAD_EIM_D18__ECSPI1_MOSI		 0x100b1
+				MX6QDL_PAD_EIM_EB2__GPIO2_IO30		0x100b1
+				MX6QDL_PAD_EIM_D19__GPIO3_IO19		0x100b1
+			>;
+		};
+
 		pinctrl_enet: enetgrp {
 			fsl,pins = <
 				MX6QDL_PAD_RGMII_RXC__RGMII_RXC		0x1b0b0
@@ -91,17 +169,75 @@
 			>;
 		};
 
+		pinctrl_i2c3: i2c3grp {
+			fsl,pins = <
+				MX6QDL_PAD_GPIO_3__I2C3_SCL 0x4001b8b1
+				MX6QDL_PAD_GPIO_6__I2C3_SDA 0x4001b8b1
+			>;
+		};
+
+		pinctrl_pcie: pciegrp {
+			fsl,pins = <
+				MX6QDL_PAD_ENET_RXD1__GPIO1_IO26 0x80000000
+				MX6QDL_PAD_EIM_CS1__GPIO2_IO24 0x80000000
+			>;
+		};
+
 		pinctrl_uart4: uart4grp {
 			fsl,pins = <
 				MX6QDL_PAD_KEY_COL0__UART4_TX_DATA	0x1b0b1
 				MX6QDL_PAD_KEY_ROW0__UART4_RX_DATA	0x1b0b1
 			>;
 		};
+
+		pinctrl_usbh1: usbh1grp {
+			fsl,pins = <
+				MX6QDL_PAD_SD3_RST__GPIO7_IO08 0x80000000
+			>;
+		};
+
+		pinctrl_usbotg: usbotggrp {
+			fsl,pins = <
+				MX6QDL_PAD_ENET_RX_ER__USB_OTG_ID 0x17059
+				MX6QDL_PAD_EIM_D22__GPIO3_IO22 0x80000000
+			>;
+		};
 	};
 };
 
+&pcie {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_pcie>;
+	reset-gpio = <&gpio1 26 0>;
+	power-on-gpio = <&gpio2 24 0>;
+	status = "okay";
+};
+
+&sata {
+	status = "okay";
+};
+
+&snvs_poweroff {
+	status = "okay";
+};
+
 &uart4 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart4>;
 	status = "okay";
 };
+
+&usbotg {
+	vbus-supply = <&reg_usb_otg_vbus>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_usbotg>;
+	dr_mode = "otg";
+	status = "okay";
+};
+
+&usbh1 {
+	vbus-supply = <&reg_usb_h1_vbus>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_usbh1>;
+	status = "okay";
+};
-- 
2.8.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Lucas Stach]

Am Montag, den 23.05.2016, 00:47 +0200 schrieb
christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org:
> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
> 
> The cm-fx6 module has an on-board spi-flash chip for its firmware, an
> eeprom (containing e.g. the mac address of the on-board Ethernet),
> a sata port, a pcie controller, an USB hub, and an USB otg port.
> Enable support for them. In addition, enable syscon poweroff support.
> 
> Signed-off-by: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
> ---
>  arch/arm/boot/dts/imx6q-cm-fx6.dts | 136 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 136 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6q-cm-fx6.dts b/arch/arm/boot/dts/imx6q-cm-fx6.dts
> index 99b46f8..f4fc22e 100644
> --- a/arch/arm/boot/dts/imx6q-cm-fx6.dts
> +++ b/arch/arm/boot/dts/imx6q-cm-fx6.dts
> @@ -31,6 +31,61 @@
>  			linux,default-trigger = "heartbeat";
>  		};
>  	};
> +
> +	regulators {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +

Please remove this regulator pseudo-bus. It doesn't exist in hardware.
The regulators are direct children of the module.

> +		reg_usb_otg_vbus: usb_otg_vbus {
> +			compatible = "regulator-fixed";
> +			regulator-name = "usb_otg_vbus";
> +			regulator-min-microvolt = <5000000>;
> +			regulator-max-microvolt = <5000000>;
> +			gpio = <&gpio3 22 0>;
> +			enable-active-high;
> +		};
> +
> +		reg_usb_h1_vbus: usb_h1_vbus {
> +			compatible = "regulator-fixed";
> +			regulator-name = "usb_h1_vbus";
> +			regulator-min-microvolt = <5000000>;
> +			regulator-max-microvolt = <5000000>;
> +			gpio = <&gpio7 8 0>;
> +			enable-active-high;
> +		};
> +	};
> +};
> +
> +&ecspi1 {
> +	fsl,spi-num-chipselects = <2>;
> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_ecspi1>;
> +	status = "okay";
> +
> +	flash: m25p80@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "st,m25p", "jedec,spi-nor";
> +		spi-max-frequency = <20000000>;
> +		reg = <0>;
> +
> +		partition@0 {
> +			label = "uboot";
> +			reg = <0x0 0xc0000>;
> +		};
> +
> +		partition@c0000 {
> +			label = "uboot environment";
> +			reg = <0xc0000 0x40000>;
> +		};
> +
> +		partition@100000 {
> +			label = "reserved";
> +			reg = <0x100000 0x100000>;
> +		};

Partition layouts don't belong in the upstream DT, as it a device
configuration thing. Please kep them in the bootloader/firmware and make
this one pass the partition layout to the kernel.

> +	};
>  };
>  
>  &fec {
> @@ -46,8 +101,31 @@
>  	status = "okay";
>  };
>  
> +&i2c3 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c3>;
> +	status = "okay";
> +	clock-frequency = <100000>;
> +
> +	eeprom@50 {
> +		compatible = "at24,24c02";
> +		reg = <0x50>;
> +		pagesize = <16>;
> +	};
> +};
> +
>  &iomuxc {
>  	imx6q-cm-fx6 {
> +		pinctrl_ecspi1: ecspi1grp {
> +			fsl,pins = <
> +				MX6QDL_PAD_EIM_D16__ECSPI1_SCLK		 0x100b1
> +				MX6QDL_PAD_EIM_D17__ECSPI1_MISO		 0x100b1
> +				MX6QDL_PAD_EIM_D18__ECSPI1_MOSI		 0x100b1
> +				MX6QDL_PAD_EIM_EB2__GPIO2_IO30		0x100b1
> +				MX6QDL_PAD_EIM_D19__GPIO3_IO19		0x100b1
> +			>;
> +		};
> +
>  		pinctrl_enet: enetgrp {
>  			fsl,pins = <
>  				MX6QDL_PAD_RGMII_RXC__RGMII_RXC		0x1b0b0
> @@ -91,17 +169,75 @@
>  			>;
>  		};
>  
> +		pinctrl_i2c3: i2c3grp {
> +			fsl,pins = <
> +				MX6QDL_PAD_GPIO_3__I2C3_SCL 0x4001b8b1
> +				MX6QDL_PAD_GPIO_6__I2C3_SDA 0x4001b8b1
> +			>;
> +		};
> +
> +		pinctrl_pcie: pciegrp {
> +			fsl,pins = <
> +				MX6QDL_PAD_ENET_RXD1__GPIO1_IO26 0x80000000
> +				MX6QDL_PAD_EIM_CS1__GPIO2_IO24 0x80000000
> +			>;
> +		};
> +
>  		pinctrl_uart4: uart4grp {
>  			fsl,pins = <
>  				MX6QDL_PAD_KEY_COL0__UART4_TX_DATA	0x1b0b1
>  				MX6QDL_PAD_KEY_ROW0__UART4_RX_DATA	0x1b0b1
>  			>;
>  		};
> +
> +		pinctrl_usbh1: usbh1grp {
> +			fsl,pins = <
> +				MX6QDL_PAD_SD3_RST__GPIO7_IO08 0x80000000
> +			>;
> +		};
> +
> +		pinctrl_usbotg: usbotggrp {
> +			fsl,pins = <
> +				MX6QDL_PAD_ENET_RX_ER__USB_OTG_ID 0x17059
> +				MX6QDL_PAD_EIM_D22__GPIO3_IO22 0x80000000
> +			>;
> +		};
>  	};
>  };
>  
> +&pcie {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_pcie>;
> +	reset-gpio = <&gpio1 26 0>;
> +	power-on-gpio = <&gpio2 24 0>;

There is no power-on-gpio in the upstream binding.

> +	status = "okay";
> +};
> +
> +&sata {
> +	status = "okay";
> +};
> +
> +&snvs_poweroff {
> +	status = "okay";
> +};
> +
>  &uart4 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_uart4>;
>  	status = "okay";
>  };
> +
> +&usbotg {
> +	vbus-supply = <&reg_usb_otg_vbus>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_usbotg>;
> +	dr_mode = "otg";
> +	status = "okay";
> +};
> +
> +&usbh1 {
> +	vbus-supply = <&reg_usb_h1_vbus>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_usbh1>;
> +	status = "okay";
> +};


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Christopher Spinrath]

Hi Lucas,

Thanks for your review!

On 05/23/2016 11:03 AM, Lucas Stach wrote:
> Am Montag, den 23.05.2016, 00:47 +0200 schrieb
> christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org:
>> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
>>
>> The cm-fx6 module has an on-board spi-flash chip for its firmware, an
>> eeprom (containing e.g. the mac address of the on-board Ethernet),
>> a sata port, a pcie controller, an USB hub, and an USB otg port.
>> Enable support for them. In addition, enable syscon poweroff support.
>>
>> Signed-off-by: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/imx6q-cm-fx6.dts | 136 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 136 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx6q-cm-fx6.dts b/arch/arm/boot/dts/imx6q-cm-fx6.dts
>> index 99b46f8..f4fc22e 100644
>> --- a/arch/arm/boot/dts/imx6q-cm-fx6.dts
>> +++ b/arch/arm/boot/dts/imx6q-cm-fx6.dts
>> @@ -31,6 +31,61 @@
>>  			linux,default-trigger = "heartbeat";
>>  		};
>>  	};
>> +
>> +	regulators {
>> +		compatible = "simple-bus";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
> 
> Please remove this regulator pseudo-bus. It doesn't exist in hardware.
> The regulators are direct children of the module.
> 

OK, I will remove it. I added it because it seems to be common (e.g.
sabrelite and wandboard use it).

>> +		reg_usb_otg_vbus: usb_otg_vbus {
>> +			compatible = "regulator-fixed";
>> +			regulator-name = "usb_otg_vbus";
>> +			regulator-min-microvolt = <5000000>;
>> +			regulator-max-microvolt = <5000000>;
>> +			gpio = <&gpio3 22 0>;
>> +			enable-active-high;
>> +		};
>> +
>> +		reg_usb_h1_vbus: usb_h1_vbus {
>> +			compatible = "regulator-fixed";
>> +			regulator-name = "usb_h1_vbus";
>> +			regulator-min-microvolt = <5000000>;
>> +			regulator-max-microvolt = <5000000>;
>> +			gpio = <&gpio7 8 0>;
>> +			enable-active-high;
>> +		};
>> +	};
>> +};
>> +
>> +&ecspi1 {
>> +	fsl,spi-num-chipselects = <2>;
>> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>> +	status = "okay";
>> +
>> +	flash: m25p80@0 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		compatible = "st,m25p", "jedec,spi-nor";
>> +		spi-max-frequency = <20000000>;
>> +		reg = <0>;
>> +
>> +		partition@0 {
>> +			label = "uboot";
>> +			reg = <0x0 0xc0000>;
>> +		};
>> +
>> +		partition@c0000 {
>> +			label = "uboot environment";
>> +			reg = <0xc0000 0x40000>;
>> +		};
>> +
>> +		partition@100000 {
>> +			label = "reserved";
>> +			reg = <0x100000 0x100000>;
>> +		};
> 
> Partition layouts don't belong in the upstream DT, as it a device
> configuration thing. Please kep them in the bootloader/firmware and make
> this one pass the partition layout to the kernel.
> 
OK, removed (although I do not like to patch the firmware/uboot).

>> +	};
>>  };
>>  
>>  &fec {
>> @@ -46,8 +101,31 @@
>>  	status = "okay";
>>  };
>>  
>> +&i2c3 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_i2c3>;
>> +	status = "okay";
>> +	clock-frequency = <100000>;
>> +
>> +	eeprom@50 {
>> +		compatible = "at24,24c02";
>> +		reg = <0x50>;
>> +		pagesize = <16>;
>> +	};
>> +};
>> +
>>  &iomuxc {
>>  	imx6q-cm-fx6 {
>> +		pinctrl_ecspi1: ecspi1grp {
>> +			fsl,pins = <
>> +				MX6QDL_PAD_EIM_D16__ECSPI1_SCLK		 0x100b1
>> +				MX6QDL_PAD_EIM_D17__ECSPI1_MISO		 0x100b1
>> +				MX6QDL_PAD_EIM_D18__ECSPI1_MOSI		 0x100b1
>> +				MX6QDL_PAD_EIM_EB2__GPIO2_IO30		0x100b1
>> +				MX6QDL_PAD_EIM_D19__GPIO3_IO19		0x100b1
>> +			>;
>> +		};
>> +
>>  		pinctrl_enet: enetgrp {
>>  			fsl,pins = <
>>  				MX6QDL_PAD_RGMII_RXC__RGMII_RXC		0x1b0b0
>> @@ -91,17 +169,75 @@
>>  			>;
>>  		};
>>  
>> +		pinctrl_i2c3: i2c3grp {
>> +			fsl,pins = <
>> +				MX6QDL_PAD_GPIO_3__I2C3_SCL 0x4001b8b1
>> +				MX6QDL_PAD_GPIO_6__I2C3_SDA 0x4001b8b1
>> +			>;
>> +		};
>> +
>> +		pinctrl_pcie: pciegrp {
>> +			fsl,pins = <
>> +				MX6QDL_PAD_ENET_RXD1__GPIO1_IO26 0x80000000
>> +				MX6QDL_PAD_EIM_CS1__GPIO2_IO24 0x80000000
>> +			>;
>> +		};
>> +
>>  		pinctrl_uart4: uart4grp {
>>  			fsl,pins = <
>>  				MX6QDL_PAD_KEY_COL0__UART4_TX_DATA	0x1b0b1
>>  				MX6QDL_PAD_KEY_ROW0__UART4_RX_DATA	0x1b0b1
>>  			>;
>>  		};
>> +
>> +		pinctrl_usbh1: usbh1grp {
>> +			fsl,pins = <
>> +				MX6QDL_PAD_SD3_RST__GPIO7_IO08 0x80000000
>> +			>;
>> +		};
>> +
>> +		pinctrl_usbotg: usbotggrp {
>> +			fsl,pins = <
>> +				MX6QDL_PAD_ENET_RX_ER__USB_OTG_ID 0x17059
>> +				MX6QDL_PAD_EIM_D22__GPIO3_IO22 0x80000000
>> +			>;
>> +		};
>>  	};
>>  };
>>  
>> +&pcie {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_pcie>;
>> +	reset-gpio = <&gpio1 26 0>;
>> +	power-on-gpio = <&gpio2 24 0>;
> 
> There is no power-on-gpio in the upstream binding.
> 

uh, this one slipped through my clean up. Removed.

>> +	status = "okay";
>> +};
>> +
>> +&sata {
>> +	status = "okay";
>> +};
>> +
>> +&snvs_poweroff {
>> +	status = "okay";
>> +};
>> +
>>  &uart4 {
>>  	pinctrl-names = "default";
>>  	pinctrl-0 = <&pinctrl_uart4>;
>>  	status = "okay";
>>  };
>> +
>> +&usbotg {
>> +	vbus-supply = <&reg_usb_otg_vbus>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_usbotg>;
>> +	dr_mode = "otg";
>> +	status = "okay";
>> +};
>> +
>> +&usbh1 {
>> +	vbus-supply = <&reg_usb_h1_vbus>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_usbh1>;
>> +	status = "okay";
>> +};

Cheers,
Christopher
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Igor Grinberg]

Hi Lucas,

Thanks for reviewing the patch(es).

On 05/23/2016 12:03 PM, Lucas Stach wrote:
> Am Montag, den 23.05.2016, 00:47 +0200 schrieb
> christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org:
>> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>

[...]

>> +&ecspi1 {
>> +	fsl,spi-num-chipselects = <2>;
>> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>> +	status = "okay";
>> +
>> +	flash: m25p80@0 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		compatible = "st,m25p", "jedec,spi-nor";
>> +		spi-max-frequency = <20000000>;
>> +		reg = <0>;
>> +
>> +		partition@0 {
>> +			label = "uboot";
>> +			reg = <0x0 0xc0000>;
>> +		};
>> +
>> +		partition@c0000 {
>> +			label = "uboot environment";
>> +			reg = <0xc0000 0x40000>;
>> +		};
>> +
>> +		partition@100000 {
>> +			label = "reserved";
>> +			reg = <0x100000 0x100000>;
>> +		};
> 
> Partition layouts don't belong in the upstream DT, as it a device
> configuration thing. Please kep them in the bootloader/firmware and make
> this one pass the partition layout to the kernel.

I don't completely agree with this.
We have lots of partition layouts in the upstream DT.
Moreover, this is the default layout and changing it, will
result in incompatibilities and also might result in device "bricking".
Those can be changed from the boot loader in case you need those
the other way around.
Another question of mine is, why should you?


-- 
Regards,
Igor.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Lucas Stach]

Hi Igor,

Am Donnerstag, den 26.05.2016, 11:50 +0300 schrieb Igor Grinberg:
> Hi Lucas,
> 
> Thanks for reviewing the patch(es).
> 
> On 05/23/2016 12:03 PM, Lucas Stach wrote:
> > Am Montag, den 23.05.2016, 00:47 +0200 schrieb
> > christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org:
> >> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
> 
> [...]
> 
> >> +&ecspi1 {
> >> +	fsl,spi-num-chipselects = <2>;
> >> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
> >> +	pinctrl-names = "default";
> >> +	pinctrl-0 = <&pinctrl_ecspi1>;
> >> +	status = "okay";
> >> +
> >> +	flash: m25p80@0 {
> >> +		#address-cells = <1>;
> >> +		#size-cells = <1>;
> >> +		compatible = "st,m25p", "jedec,spi-nor";
> >> +		spi-max-frequency = <20000000>;
> >> +		reg = <0>;
> >> +
> >> +		partition@0 {
> >> +			label = "uboot";
> >> +			reg = <0x0 0xc0000>;
> >> +		};
> >> +
> >> +		partition@c0000 {
> >> +			label = "uboot environment";
> >> +			reg = <0xc0000 0x40000>;
> >> +		};
> >> +
> >> +		partition@100000 {
> >> +			label = "reserved";
> >> +			reg = <0x100000 0x100000>;
> >> +		};
> > 
> > Partition layouts don't belong in the upstream DT, as it a device
> > configuration thing. Please kep them in the bootloader/firmware and make
> > this one pass the partition layout to the kernel.
> 
> I don't completely agree with this.
> We have lots of partition layouts in the upstream DT.

No, we don't. At least not for the i.MX6. There are some for the earlier
i.MX boards, but IMO it's wrong to put device configuration into the
upstream DT. Let's not start doing this again.

> Moreover, this is the default layout and changing it, will
> result in incompatibilities and also might result in device "bricking".
> Those can be changed from the boot loader in case you need those
> the other way around.
> Another question of mine is, why should you?
> 
Partition layout is device configuration, which is governed by the
device firmware. By not having the partition layout in the upstream DT
people are forced to set it from the firmware, which is exactly the
right thing to do, weather or not you plan to change it at any time.

Regards,
Lucas


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Igor Grinberg]

On 05/26/2016 12:50 PM, Lucas Stach wrote:
> Hi Igor,
> 
> Am Donnerstag, den 26.05.2016, 11:50 +0300 schrieb Igor Grinberg:
>> Hi Lucas,
>>
>> Thanks for reviewing the patch(es).
>>
>> On 05/23/2016 12:03 PM, Lucas Stach wrote:
>>> Am Montag, den 23.05.2016, 00:47 +0200 schrieb
>>> christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org:
>>>> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
>>
>> [...]
>>
>>>> +&ecspi1 {
>>>> +	fsl,spi-num-chipselects = <2>;
>>>> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>>>> +	status = "okay";
>>>> +
>>>> +	flash: m25p80@0 {
>>>> +		#address-cells = <1>;
>>>> +		#size-cells = <1>;
>>>> +		compatible = "st,m25p", "jedec,spi-nor";
>>>> +		spi-max-frequency = <20000000>;
>>>> +		reg = <0>;
>>>> +
>>>> +		partition@0 {
>>>> +			label = "uboot";
>>>> +			reg = <0x0 0xc0000>;
>>>> +		};
>>>> +
>>>> +		partition@c0000 {
>>>> +			label = "uboot environment";
>>>> +			reg = <0xc0000 0x40000>;
>>>> +		};
>>>> +
>>>> +		partition@100000 {
>>>> +			label = "reserved";
>>>> +			reg = <0x100000 0x100000>;
>>>> +		};
>>>
>>> Partition layouts don't belong in the upstream DT, as it a device
>>> configuration thing. Please kep them in the bootloader/firmware and make
>>> this one pass the partition layout to the kernel.
>>
>> I don't completely agree with this.
>> We have lots of partition layouts in the upstream DT.
> 
> No, we don't. At least not for the i.MX6. There are some for the earlier
> i.MX boards, but IMO it's wrong to put device configuration into the
> upstream DT. Let's not start doing this again.

Why not?
For i.MX6 there are 2 boards that have the partitioning scheme.
I'm not considering this a device configuration, but rather
a default partitioning layout/scheme.
Current case is for the firmware storage device that is not likely
to change.
Moreover, a DT is not really a part of the kernel, but lays along the kernel
sources for convenience and simplicity (at least IIRC as it was decided
about 5 years ago). It is more a part of the firmware for a device, than
an upstream kernel source code.
I think it is only a meter of time when Linus will decide that he does not
want it inside the kernel anymore...

> 
>> Moreover, this is the default layout and changing it, will
>> result in incompatibilities and also might result in device "bricking".
>> Those can be changed from the boot loader in case you need those
>> the other way around.
>> Another question of mine is, why should you?
>>
> Partition layout is device configuration, which is governed by the
> device firmware.

Yet again, DT is a part of device firmware.
Moreover, the firmware (in that case U-Boot), can be configured
using the very same DT code, so not having this in, might force
various w/a and hacks.

> By not having the partition layout in the upstream DT
> people are forced to set it from the firmware, which is exactly the
> right thing to do, weather or not you plan to change it at any time.

I might be ignorant, sorry for that.
Why? Why is it right and why would you want to force people to do that?


-- 
Regards,
Igor.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Igor Grinberg]

Hi Christopher,

On 05/26/2016 02:37 PM, Igor Grinberg wrote:
> On 05/26/2016 12:50 PM, Lucas Stach wrote:
>> Hi Igor,
>>
>> Am Donnerstag, den 26.05.2016, 11:50 +0300 schrieb Igor Grinberg:
>>> Hi Lucas,
>>>
>>> Thanks for reviewing the patch(es).
>>>
>>> On 05/23/2016 12:03 PM, Lucas Stach wrote:
>>>> Am Montag, den 23.05.2016, 00:47 +0200 schrieb
>>>> christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org:
>>>>> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
>>>
>>> [...]
>>>
>>>>> +&ecspi1 {
>>>>> +	fsl,spi-num-chipselects = <2>;
>>>>> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
>>>>> +	pinctrl-names = "default";
>>>>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>>>>> +	status = "okay";
>>>>> +
>>>>> +	flash: m25p80@0 {
>>>>> +		#address-cells = <1>;
>>>>> +		#size-cells = <1>;
>>>>> +		compatible = "st,m25p", "jedec,spi-nor";
>>>>> +		spi-max-frequency = <20000000>;
>>>>> +		reg = <0>;
>>>>> +
>>>>> +		partition@0 {
>>>>> +			label = "uboot";
>>>>> +			reg = <0x0 0xc0000>;
>>>>> +		};
>>>>> +
>>>>> +		partition@c0000 {
>>>>> +			label = "uboot environment";
>>>>> +			reg = <0xc0000 0x40000>;
>>>>> +		};
>>>>> +
>>>>> +		partition@100000 {
>>>>> +			label = "reserved";
>>>>> +			reg = <0x100000 0x100000>;
>>>>> +		};
>>>>
>>>> Partition layouts don't belong in the upstream DT, as it a device
>>>> configuration thing. Please kep them in the bootloader/firmware and make
>>>> this one pass the partition layout to the kernel.
>>>
>>> I don't completely agree with this.
>>> We have lots of partition layouts in the upstream DT.
>>
>> No, we don't. At least not for the i.MX6. There are some for the earlier
>> i.MX boards, but IMO it's wrong to put device configuration into the
>> upstream DT. Let's not start doing this again.
> 
> Why not?
> For i.MX6 there are 2 boards that have the partitioning scheme.
> I'm not considering this a device configuration, but rather
> a default partitioning layout/scheme.
> Current case is for the firmware storage device that is not likely
> to change.
> Moreover, a DT is not really a part of the kernel, but lays along the kernel
> sources for convenience and simplicity (at least IIRC as it was decided
> about 5 years ago). It is more a part of the firmware for a device, than
> an upstream kernel source code.
> I think it is only a meter of time when Linus will decide that he does not
> want it inside the kernel anymore...
> 
>>
>>> Moreover, this is the default layout and changing it, will
>>> result in incompatibilities and also might result in device "bricking".
>>> Those can be changed from the boot loader in case you need those
>>> the other way around.
>>> Another question of mine is, why should you?
>>>
>> Partition layout is device configuration, which is governed by the
>> device firmware.
> 
> Yet again, DT is a part of device firmware.
> Moreover, the firmware (in that case U-Boot), can be configured
> using the very same DT code, so not having this in, might force
> various w/a and hacks.
> 
>> By not having the partition layout in the upstream DT
>> people are forced to set it from the firmware, which is exactly the
>> right thing to do, weather or not you plan to change it at any time.
> 
> I might be ignorant, sorry for that.
> Why? Why is it right and why would you want to force people to do that?
> 
> 

No answer?
I think it is worth keeping this as a default firmware layout.


-- 
Regards,
Igor.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Christopher Spinrath]

Hi Igor,

On 06/05/2016 06:43 PM, Igor Grinberg wrote:
> Hi Christopher,
> 
> On 05/26/2016 02:37 PM, Igor Grinberg wrote:
>> On 05/26/2016 12:50 PM, Lucas Stach wrote:
>>> Hi Igor,
>>>
>>> Am Donnerstag, den 26.05.2016, 11:50 +0300 schrieb Igor Grinberg:
>>>> Hi Lucas,
>>>>
>>>> Thanks for reviewing the patch(es).
>>>>
>>>> On 05/23/2016 12:03 PM, Lucas Stach wrote:
>>>>> Am Montag, den 23.05.2016, 00:47 +0200 schrieb
>>>>> christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org:
>>>>>> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
>>>>
>>>> [...]
>>>>
>>>>>> +&ecspi1 {
>>>>>> +	fsl,spi-num-chipselects = <2>;
>>>>>> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
>>>>>> +	pinctrl-names = "default";
>>>>>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>>>>>> +	status = "okay";
>>>>>> +
>>>>>> +	flash: m25p80@0 {
>>>>>> +		#address-cells = <1>;
>>>>>> +		#size-cells = <1>;
>>>>>> +		compatible = "st,m25p", "jedec,spi-nor";
>>>>>> +		spi-max-frequency = <20000000>;
>>>>>> +		reg = <0>;
>>>>>> +
>>>>>> +		partition@0 {
>>>>>> +			label = "uboot";
>>>>>> +			reg = <0x0 0xc0000>;
>>>>>> +		};
>>>>>> +
>>>>>> +		partition@c0000 {
>>>>>> +			label = "uboot environment";
>>>>>> +			reg = <0xc0000 0x40000>;
>>>>>> +		};
>>>>>> +
>>>>>> +		partition@100000 {
>>>>>> +			label = "reserved";
>>>>>> +			reg = <0x100000 0x100000>;
>>>>>> +		};
>>>>>
>>>>> Partition layouts don't belong in the upstream DT, as it a device
>>>>> configuration thing. Please kep them in the bootloader/firmware and make
>>>>> this one pass the partition layout to the kernel.
>>>>
>>>> I don't completely agree with this.
>>>> We have lots of partition layouts in the upstream DT.
>>>
>>> No, we don't. At least not for the i.MX6. There are some for the earlier
>>> i.MX boards, but IMO it's wrong to put device configuration into the
>>> upstream DT. Let's not start doing this again.
>>
>> Why not?
>> For i.MX6 there are 2 boards that have the partitioning scheme.
>> I'm not considering this a device configuration, but rather
>> a default partitioning layout/scheme.
>> Current case is for the firmware storage device that is not likely
>> to change.
>> Moreover, a DT is not really a part of the kernel, but lays along the kernel
>> sources for convenience and simplicity (at least IIRC as it was decided
>> about 5 years ago). It is more a part of the firmware for a device, than
>> an upstream kernel source code.
>> I think it is only a meter of time when Linus will decide that he does not
>> want it inside the kernel anymore...
>>
>>>
>>>> Moreover, this is the default layout and changing it, will
>>>> result in incompatibilities and also might result in device "bricking".
>>>> Those can be changed from the boot loader in case you need those
>>>> the other way around.
>>>> Another question of mine is, why should you?
>>>>
>>> Partition layout is device configuration, which is governed by the
>>> device firmware.
>>
>> Yet again, DT is a part of device firmware.
>> Moreover, the firmware (in that case U-Boot), can be configured
>> using the very same DT code, so not having this in, might force
>> various w/a and hacks.
>>
>>> By not having the partition layout in the upstream DT
>>> people are forced to set it from the firmware, which is exactly the
>>> right thing to do, weather or not you plan to change it at any time.
>>
>> I might be ignorant, sorry for that.
>> Why? Why is it right and why would you want to force people to do that?
>>
>>
> 
> No answer?
> I think it is worth keeping this as a default firmware layout.
> 

I was not happy removing the layout at first, too. However, they are
added rarely (at least recently) and the binding documentation [1]
states that they shall only be added if there are "strong conventions".

Moreover, it is really easy to pass the layout via firmware/u-boot.
Unfortunately, the u-boot provided by CompuLab does not have the
mtdparts commands built-in but the layout can be passed via cmdline as
well. This seems more flexible to me (and I consider this to be good).

Even if we want to have the layout in the devicetree one day (e.g. to
use the same file for u-boot) we can send another follow-up patch adding
them (and discuss this matter in more detail). But let's not prevent the
remaining part of this patch series to enter mainline if there is a
good, easy and flexible alternative.

Cheers,
Christopher


[1] Documentation/devicetree/bindings/mtd/partition.txt (v4.7-rc1)
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Igor Grinberg]

Hi Christopher,

On 06/05/2016 08:32 PM, Christopher Spinrath wrote:
> Hi Igor,
> 
> On 06/05/2016 06:43 PM, Igor Grinberg wrote:
>> Hi Christopher,
>>
>> On 05/26/2016 02:37 PM, Igor Grinberg wrote:
>>> On 05/26/2016 12:50 PM, Lucas Stach wrote:
>>>> Hi Igor,
>>>>
>>>> Am Donnerstag, den 26.05.2016, 11:50 +0300 schrieb Igor Grinberg:
>>>>> Hi Lucas,
>>>>>
>>>>> Thanks for reviewing the patch(es).
>>>>>
>>>>> On 05/23/2016 12:03 PM, Lucas Stach wrote:
>>>>>> Am Montag, den 23.05.2016, 00:47 +0200 schrieb
>>>>>> christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org:
>>>>>>> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
>>>>>
>>>>> [...]
>>>>>
>>>>>>> +&ecspi1 {
>>>>>>> +	fsl,spi-num-chipselects = <2>;
>>>>>>> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
>>>>>>> +	pinctrl-names = "default";
>>>>>>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>>>>>>> +	status = "okay";
>>>>>>> +
>>>>>>> +	flash: m25p80@0 {
>>>>>>> +		#address-cells = <1>;
>>>>>>> +		#size-cells = <1>;
>>>>>>> +		compatible = "st,m25p", "jedec,spi-nor";
>>>>>>> +		spi-max-frequency = <20000000>;
>>>>>>> +		reg = <0>;
>>>>>>> +
>>>>>>> +		partition@0 {
>>>>>>> +			label = "uboot";
>>>>>>> +			reg = <0x0 0xc0000>;
>>>>>>> +		};
>>>>>>> +
>>>>>>> +		partition@c0000 {
>>>>>>> +			label = "uboot environment";
>>>>>>> +			reg = <0xc0000 0x40000>;
>>>>>>> +		};
>>>>>>> +
>>>>>>> +		partition@100000 {
>>>>>>> +			label = "reserved";
>>>>>>> +			reg = <0x100000 0x100000>;
>>>>>>> +		};
>>>>>>
>>>>>> Partition layouts don't belong in the upstream DT, as it a device
>>>>>> configuration thing. Please kep them in the bootloader/firmware and make
>>>>>> this one pass the partition layout to the kernel.
>>>>>
>>>>> I don't completely agree with this.
>>>>> We have lots of partition layouts in the upstream DT.
>>>>
>>>> No, we don't. At least not for the i.MX6. There are some for the earlier
>>>> i.MX boards, but IMO it's wrong to put device configuration into the
>>>> upstream DT. Let's not start doing this again.
>>>
>>> Why not?
>>> For i.MX6 there are 2 boards that have the partitioning scheme.
>>> I'm not considering this a device configuration, but rather
>>> a default partitioning layout/scheme.
>>> Current case is for the firmware storage device that is not likely
>>> to change.
>>> Moreover, a DT is not really a part of the kernel, but lays along the kernel
>>> sources for convenience and simplicity (at least IIRC as it was decided
>>> about 5 years ago). It is more a part of the firmware for a device, than
>>> an upstream kernel source code.
>>> I think it is only a meter of time when Linus will decide that he does not
>>> want it inside the kernel anymore...
>>>
>>>>
>>>>> Moreover, this is the default layout and changing it, will
>>>>> result in incompatibilities and also might result in device "bricking".
>>>>> Those can be changed from the boot loader in case you need those
>>>>> the other way around.
>>>>> Another question of mine is, why should you?
>>>>>
>>>> Partition layout is device configuration, which is governed by the
>>>> device firmware.
>>>
>>> Yet again, DT is a part of device firmware.
>>> Moreover, the firmware (in that case U-Boot), can be configured
>>> using the very same DT code, so not having this in, might force
>>> various w/a and hacks.
>>>
>>>> By not having the partition layout in the upstream DT
>>>> people are forced to set it from the firmware, which is exactly the
>>>> right thing to do, weather or not you plan to change it at any time.
>>>
>>> I might be ignorant, sorry for that.
>>> Why? Why is it right and why would you want to force people to do that?
>>>
>>>
>>
>> No answer?
>> I think it is worth keeping this as a default firmware layout.
>>
> 
> I was not happy removing the layout at first, too. However, they are
> added rarely (at least recently) and the binding documentation [1]
> states that they shall only be added if there are "strong conventions".

Well, not exactly, it says:

"Partitions can be represented by sub-nodes of an mtd device. This can be used
on platforms which have strong conventions about which portions of a flash are
used for what purposes, but which don't use an on-flash partition table such
as RedBoot."

In my understanding, "can" is _not_ exactly "shall only".

> 
> Moreover, it is really easy to pass the layout via firmware/u-boot.
> Unfortunately, the u-boot provided by CompuLab does not have the
> mtdparts commands built-in but the layout can be passed via cmdline as
> well. This seems more flexible to me (and I consider this to be good).

That means you need a different version of U-Boot if you want to use
an upstream kernel, or you need to play the bootargs games, or just leave
it a "black box".
So currently, from the three options above, the "black box" is chosen, right?
That's a pity, because I'd like things to be open...

> 
> Even if we want to have the layout in the devicetree one day (e.g. to
> use the same file for u-boot) we can send another follow-up patch adding
> them (and discuss this matter in more detail). But let's not prevent the
> remaining part of this patch series to enter mainline if there is a
> good, easy and flexible alternative.

That's a valid point from my POV.
I have never had an intent to prevent things going upstream.
This is something Compulab should have done years ago, but
unfortunately, due to some circumstances it haven't happened for cm-fx6...


-- 
Regards,
Igor.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Christopher Spinrath]

Hi Igor,

On 06/06/2016 08:49 AM, Igor Grinberg wrote:
> Hi Christopher,
> 
> On 06/05/2016 08:32 PM, Christopher Spinrath wrote:
>> Hi Igor,
>>
>> On 06/05/2016 06:43 PM, Igor Grinberg wrote:
>>> Hi Christopher,
>>>
>>> On 05/26/2016 02:37 PM, Igor Grinberg wrote:
>>>> On 05/26/2016 12:50 PM, Lucas Stach wrote:
>>>>> Hi Igor,
>>>>>
>>>>> Am Donnerstag, den 26.05.2016, 11:50 +0300 schrieb Igor Grinberg:
>>>>>> Hi Lucas,
>>>>>>
>>>>>> Thanks for reviewing the patch(es).
>>>>>>
>>>>>> On 05/23/2016 12:03 PM, Lucas Stach wrote:
>>>>>>> Am Montag, den 23.05.2016, 00:47 +0200 schrieb
>>>>>>> christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org:
>>>>>>>> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>> +&ecspi1 {
>>>>>>>> +	fsl,spi-num-chipselects = <2>;
>>>>>>>> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
>>>>>>>> +	pinctrl-names = "default";
>>>>>>>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>>>>>>>> +	status = "okay";
>>>>>>>> +
>>>>>>>> +	flash: m25p80@0 {
>>>>>>>> +		#address-cells = <1>;
>>>>>>>> +		#size-cells = <1>;
>>>>>>>> +		compatible = "st,m25p", "jedec,spi-nor";
>>>>>>>> +		spi-max-frequency = <20000000>;
>>>>>>>> +		reg = <0>;
>>>>>>>> +
>>>>>>>> +		partition@0 {
>>>>>>>> +			label = "uboot";
>>>>>>>> +			reg = <0x0 0xc0000>;
>>>>>>>> +		};
>>>>>>>> +
>>>>>>>> +		partition@c0000 {
>>>>>>>> +			label = "uboot environment";
>>>>>>>> +			reg = <0xc0000 0x40000>;
>>>>>>>> +		};
>>>>>>>> +
>>>>>>>> +		partition@100000 {
>>>>>>>> +			label = "reserved";
>>>>>>>> +			reg = <0x100000 0x100000>;
>>>>>>>> +		};
>>>>>>>
>>>>>>> Partition layouts don't belong in the upstream DT, as it a device
>>>>>>> configuration thing. Please kep them in the bootloader/firmware and make
>>>>>>> this one pass the partition layout to the kernel.
>>>>>>
>>>>>> I don't completely agree with this.
>>>>>> We have lots of partition layouts in the upstream DT.
>>>>>
>>>>> No, we don't. At least not for the i.MX6. There are some for the earlier
>>>>> i.MX boards, but IMO it's wrong to put device configuration into the
>>>>> upstream DT. Let's not start doing this again.
>>>>
>>>> Why not?
>>>> For i.MX6 there are 2 boards that have the partitioning scheme.
>>>> I'm not considering this a device configuration, but rather
>>>> a default partitioning layout/scheme.
>>>> Current case is for the firmware storage device that is not likely
>>>> to change.
>>>> Moreover, a DT is not really a part of the kernel, but lays along the kernel
>>>> sources for convenience and simplicity (at least IIRC as it was decided
>>>> about 5 years ago). It is more a part of the firmware for a device, than
>>>> an upstream kernel source code.
>>>> I think it is only a meter of time when Linus will decide that he does not
>>>> want it inside the kernel anymore...
>>>>
>>>>>
>>>>>> Moreover, this is the default layout and changing it, will
>>>>>> result in incompatibilities and also might result in device "bricking".
>>>>>> Those can be changed from the boot loader in case you need those
>>>>>> the other way around.
>>>>>> Another question of mine is, why should you?
>>>>>>
>>>>> Partition layout is device configuration, which is governed by the
>>>>> device firmware.
>>>>
>>>> Yet again, DT is a part of device firmware.
>>>> Moreover, the firmware (in that case U-Boot), can be configured
>>>> using the very same DT code, so not having this in, might force
>>>> various w/a and hacks.
>>>>
>>>>> By not having the partition layout in the upstream DT
>>>>> people are forced to set it from the firmware, which is exactly the
>>>>> right thing to do, weather or not you plan to change it at any time.
>>>>
>>>> I might be ignorant, sorry for that.
>>>> Why? Why is it right and why would you want to force people to do that?
>>>>
>>>>
>>>
>>> No answer?
>>> I think it is worth keeping this as a default firmware layout.
>>>
>>
>> I was not happy removing the layout at first, too. However, they are
>> added rarely (at least recently) and the binding documentation [1]
>> states that they shall only be added if there are "strong conventions".
> 
> Well, not exactly, it says:
> 
> "Partitions can be represented by sub-nodes of an mtd device. This can be used
> on platforms which have strong conventions about which portions of a flash are
> used for what purposes, but which don't use an on-flash partition table such
> as RedBoot."
> 
> In my understanding, "can" is _not_ exactly "shall only".
> 

Definitely, "state" was the wrong word here - sorry about that. But it's
how I interpret the documentation. It is the only one I have seen that
specifies an allowed use case. So for me it is more like "deny all,
allow ...".

>>
>> Moreover, it is really easy to pass the layout via firmware/u-boot.
>> Unfortunately, the u-boot provided by CompuLab does not have the
>> mtdparts commands built-in but the layout can be passed via cmdline as
>> well. This seems more flexible to me (and I consider this to be good).
> 
> That means you need a different version of U-Boot if you want to use
> an upstream kernel, or you need to play the bootargs games, or just leave
> it a "black box".
> So currently, from the three options above, the "black box" is chosen, right?
> That's a pity, because I'd like things to be open...
> 

Well, It wouldn't be the first ARM device where the newest/upstream
u-boot is required. Actually, you already have to use the newest version
if you want to use the igb ethernet.

Currently, IMO the most convenient way (for distributions) would be to
ship a boot.scr file changing the bootargs (which is not a "black box"
but a configuration file people are used to). If you don't want to mess
with bootargs/cmdline it is also possible to add the partition table via
boot.scr (or on-flash environment) with the fdt command; in this case it
wouldn't even be hidden in the u-boot binary (and no u-boot upgrade is
required to implement this).

But yes, if someone wants to use the upstream kernel the boot.scr has to
be there; but IMO this is something distributions/people providing
binary files have to take care about. People using their own kernel
should take into account that they have to provide a bootloader config.

>>
>> Even if we want to have the layout in the devicetree one day (e.g. to
>> use the same file for u-boot) we can send another follow-up patch adding
>> them (and discuss this matter in more detail). But let's not prevent the
>> remaining part of this patch series to enter mainline if there is a
>> good, easy and flexible alternative.
> 
> That's a valid point from my POV.
> I have never had an intent to prevent things going upstream.
I never thought so :-).

> This is something Compulab should have done years ago, but
> unfortunately, due to some circumstances it haven't happened for cm-fx6...
> 

Cheers,
Christopher

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Igor Grinberg]

Hi Christopher,

On 06/06/2016 01:37 PM, Christopher Spinrath wrote:
> Hi Igor,
> 
> On 06/06/2016 08:49 AM, Igor Grinberg wrote:
>> Hi Christopher,
>>
>> On 06/05/2016 08:32 PM, Christopher Spinrath wrote:
>>> Hi Igor,
>>>
>>> On 06/05/2016 06:43 PM, Igor Grinberg wrote:
>>>> Hi Christopher,
>>>>
>>>> On 05/26/2016 02:37 PM, Igor Grinberg wrote:
>>>>> On 05/26/2016 12:50 PM, Lucas Stach wrote:
>>>>>> Hi Igor,
>>>>>>
>>>>>> Am Donnerstag, den 26.05.2016, 11:50 +0300 schrieb Igor Grinberg:
>>>>>>> Hi Lucas,
>>>>>>>
>>>>>>> Thanks for reviewing the patch(es).
>>>>>>>
>>>>>>> On 05/23/2016 12:03 PM, Lucas Stach wrote:
>>>>>>>> Am Montag, den 23.05.2016, 00:47 +0200 schrieb
>>>>>>>> christopher.spinrath@rwth-aachen.de:
>>>>>>>>> From: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>> +&ecspi1 {
>>>>>>>>> +	fsl,spi-num-chipselects = <2>;
>>>>>>>>> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
>>>>>>>>> +	pinctrl-names = "default";
>>>>>>>>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>>>>>>>>> +	status = "okay";
>>>>>>>>> +
>>>>>>>>> +	flash: m25p80@0 {
>>>>>>>>> +		#address-cells = <1>;
>>>>>>>>> +		#size-cells = <1>;
>>>>>>>>> +		compatible = "st,m25p", "jedec,spi-nor";
>>>>>>>>> +		spi-max-frequency = <20000000>;
>>>>>>>>> +		reg = <0>;
>>>>>>>>> +
>>>>>>>>> +		partition@0 {
>>>>>>>>> +			label = "uboot";
>>>>>>>>> +			reg = <0x0 0xc0000>;
>>>>>>>>> +		};
>>>>>>>>> +
>>>>>>>>> +		partition@c0000 {
>>>>>>>>> +			label = "uboot environment";
>>>>>>>>> +			reg = <0xc0000 0x40000>;
>>>>>>>>> +		};
>>>>>>>>> +
>>>>>>>>> +		partition@100000 {
>>>>>>>>> +			label = "reserved";
>>>>>>>>> +			reg = <0x100000 0x100000>;
>>>>>>>>> +		};
>>>>>>>>
>>>>>>>> Partition layouts don't belong in the upstream DT, as it a device
>>>>>>>> configuration thing. Please kep them in the bootloader/firmware and make
>>>>>>>> this one pass the partition layout to the kernel.
>>>>>>>
>>>>>>> I don't completely agree with this.
>>>>>>> We have lots of partition layouts in the upstream DT.
>>>>>>
>>>>>> No, we don't. At least not for the i.MX6. There are some for the earlier
>>>>>> i.MX boards, but IMO it's wrong to put device configuration into the
>>>>>> upstream DT. Let's not start doing this again.
>>>>>
>>>>> Why not?
>>>>> For i.MX6 there are 2 boards that have the partitioning scheme.
>>>>> I'm not considering this a device configuration, but rather
>>>>> a default partitioning layout/scheme.
>>>>> Current case is for the firmware storage device that is not likely
>>>>> to change.
>>>>> Moreover, a DT is not really a part of the kernel, but lays along the kernel
>>>>> sources for convenience and simplicity (at least IIRC as it was decided
>>>>> about 5 years ago). It is more a part of the firmware for a device, than
>>>>> an upstream kernel source code.
>>>>> I think it is only a meter of time when Linus will decide that he does not
>>>>> want it inside the kernel anymore...
>>>>>
>>>>>>
>>>>>>> Moreover, this is the default layout and changing it, will
>>>>>>> result in incompatibilities and also might result in device "bricking".
>>>>>>> Those can be changed from the boot loader in case you need those
>>>>>>> the other way around.
>>>>>>> Another question of mine is, why should you?
>>>>>>>
>>>>>> Partition layout is device configuration, which is governed by the
>>>>>> device firmware.
>>>>>
>>>>> Yet again, DT is a part of device firmware.
>>>>> Moreover, the firmware (in that case U-Boot), can be configured
>>>>> using the very same DT code, so not having this in, might force
>>>>> various w/a and hacks.
>>>>>
>>>>>> By not having the partition layout in the upstream DT
>>>>>> people are forced to set it from the firmware, which is exactly the
>>>>>> right thing to do, weather or not you plan to change it at any time.
>>>>>
>>>>> I might be ignorant, sorry for that.
>>>>> Why? Why is it right and why would you want to force people to do that?
>>>>>
>>>>>
>>>>
>>>> No answer?
>>>> I think it is worth keeping this as a default firmware layout.
>>>>
>>>
>>> I was not happy removing the layout at first, too. However, they are
>>> added rarely (at least recently) and the binding documentation [1]
>>> states that they shall only be added if there are "strong conventions".
>>
>> Well, not exactly, it says:
>>
>> "Partitions can be represented by sub-nodes of an mtd device. This can be used
>> on platforms which have strong conventions about which portions of a flash are
>> used for what purposes, but which don't use an on-flash partition table such
>> as RedBoot."
>>
>> In my understanding, "can" is _not_ exactly "shall only".
>>
> 
> Definitely, "state" was the wrong word here - sorry about that. But it's
> how I interpret the documentation. It is the only one I have seen that
> specifies an allowed use case. So for me it is more like "deny all,
> allow ...".

That moves the discussion to what "strong conventions" are?
But you have already removed the layout, so this is meaningless.

> 
>>>
>>> Moreover, it is really easy to pass the layout via firmware/u-boot.
>>> Unfortunately, the u-boot provided by CompuLab does not have the
>>> mtdparts commands built-in but the layout can be passed via cmdline as
>>> well. This seems more flexible to me (and I consider this to be good).
>>
>> That means you need a different version of U-Boot if you want to use
>> an upstream kernel, or you need to play the bootargs games, or just leave
>> it a "black box".
>> So currently, from the three options above, the "black box" is chosen, right?
>> That's a pity, because I'd like things to be open...
>>
> 
> Well, It wouldn't be the first ARM device where the newest/upstream
> u-boot is required. Actually, you already have to use the newest version
> if you want to use the igb ethernet.

You mean, igb in U-Boot, right?
Unless I'm missing something, it should work just fine in Linux with
the same U-Boot Compulab provides.

> 
> Currently, IMO the most convenient way (for distributions) would be to
> ship a boot.scr file changing the bootargs (which is not a "black box"
> but a configuration file people are used to). If you don't want to mess
> with bootargs/cmdline it is also possible to add the partition table via
> boot.scr (or on-flash environment) with the fdt command; in this case it
> wouldn't even be hidden in the u-boot binary (and no u-boot upgrade is
> required to implement this).

That is exactly what I said: "to play the bootargs games".

> 
> But yes, if someone wants to use the upstream kernel the boot.scr has to
> be there; but IMO this is something distributions/people providing
> binary files have to take care about. People using their own kernel
> should take into account that they have to provide a bootloader config.

Yep. While if we provide the default layout in DT, all the above would be
unnecessary.

So, if one will neither use the latest U-Boot, nor play the bootargs
games, the flash will stay a kind of "black box" - and that's a pity.

-- 
Regards,
Igor.

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Christopher Spinrath]

Hi Igor,

On 06/07/2016 10:38 AM, Igor Grinberg wrote:
> Hi Christopher,
> 
> On 06/06/2016 01:37 PM, Christopher Spinrath wrote:
>> Hi Igor,
>>
>> On 06/06/2016 08:49 AM, Igor Grinberg wrote:
>>> Hi Christopher,
>>>
>>> On 06/05/2016 08:32 PM, Christopher Spinrath wrote:
>>>> Hi Igor,
>>>>
>>>> On 06/05/2016 06:43 PM, Igor Grinberg wrote:
>>>>> Hi Christopher,
>>>>>
>>>>> On 05/26/2016 02:37 PM, Igor Grinberg wrote:
>>>>>> On 05/26/2016 12:50 PM, Lucas Stach wrote:
>>>>>>> Hi Igor,
>>>>>>>
>>>>>>> Am Donnerstag, den 26.05.2016, 11:50 +0300 schrieb Igor Grinberg:
>>>>>>>> Hi Lucas,
>>>>>>>>
>>>>>>>> Thanks for reviewing the patch(es).
>>>>>>>>
>>>>>>>> On 05/23/2016 12:03 PM, Lucas Stach wrote:
>>>>>>>>> Am Montag, den 23.05.2016, 00:47 +0200 schrieb
>>>>>>>>> christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org:
>>>>>>>>>> From: Christopher Spinrath <christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>>> +&ecspi1 {
>>>>>>>>>> +	fsl,spi-num-chipselects = <2>;
>>>>>>>>>> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
>>>>>>>>>> +	pinctrl-names = "default";
>>>>>>>>>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>>>>>>>>>> +	status = "okay";
>>>>>>>>>> +
>>>>>>>>>> +	flash: m25p80@0 {
>>>>>>>>>> +		#address-cells = <1>;
>>>>>>>>>> +		#size-cells = <1>;
>>>>>>>>>> +		compatible = "st,m25p", "jedec,spi-nor";
>>>>>>>>>> +		spi-max-frequency = <20000000>;
>>>>>>>>>> +		reg = <0>;
>>>>>>>>>> +
>>>>>>>>>> +		partition@0 {
>>>>>>>>>> +			label = "uboot";
>>>>>>>>>> +			reg = <0x0 0xc0000>;
>>>>>>>>>> +		};
>>>>>>>>>> +
>>>>>>>>>> +		partition@c0000 {
>>>>>>>>>> +			label = "uboot environment";
>>>>>>>>>> +			reg = <0xc0000 0x40000>;
>>>>>>>>>> +		};
>>>>>>>>>> +
>>>>>>>>>> +		partition@100000 {
>>>>>>>>>> +			label = "reserved";
>>>>>>>>>> +			reg = <0x100000 0x100000>;
>>>>>>>>>> +		};
>>>>>>>>>
>>>>>>>>> Partition layouts don't belong in the upstream DT, as it a device
>>>>>>>>> configuration thing. Please kep them in the bootloader/firmware and make
>>>>>>>>> this one pass the partition layout to the kernel.
>>>>>>>>
>>>>>>>> I don't completely agree with this.
>>>>>>>> We have lots of partition layouts in the upstream DT.
>>>>>>>
>>>>>>> No, we don't. At least not for the i.MX6. There are some for the earlier
>>>>>>> i.MX boards, but IMO it's wrong to put device configuration into the
>>>>>>> upstream DT. Let's not start doing this again.
>>>>>>
>>>>>> Why not?
>>>>>> For i.MX6 there are 2 boards that have the partitioning scheme.
>>>>>> I'm not considering this a device configuration, but rather
>>>>>> a default partitioning layout/scheme.
>>>>>> Current case is for the firmware storage device that is not likely
>>>>>> to change.
>>>>>> Moreover, a DT is not really a part of the kernel, but lays along the kernel
>>>>>> sources for convenience and simplicity (at least IIRC as it was decided
>>>>>> about 5 years ago). It is more a part of the firmware for a device, than
>>>>>> an upstream kernel source code.
>>>>>> I think it is only a meter of time when Linus will decide that he does not
>>>>>> want it inside the kernel anymore...
>>>>>>
>>>>>>>
>>>>>>>> Moreover, this is the default layout and changing it, will
>>>>>>>> result in incompatibilities and also might result in device "bricking".
>>>>>>>> Those can be changed from the boot loader in case you need those
>>>>>>>> the other way around.
>>>>>>>> Another question of mine is, why should you?
>>>>>>>>
>>>>>>> Partition layout is device configuration, which is governed by the
>>>>>>> device firmware.
>>>>>>
>>>>>> Yet again, DT is a part of device firmware.
>>>>>> Moreover, the firmware (in that case U-Boot), can be configured
>>>>>> using the very same DT code, so not having this in, might force
>>>>>> various w/a and hacks.
>>>>>>
>>>>>>> By not having the partition layout in the upstream DT
>>>>>>> people are forced to set it from the firmware, which is exactly the
>>>>>>> right thing to do, weather or not you plan to change it at any time.
>>>>>>
>>>>>> I might be ignorant, sorry for that.
>>>>>> Why? Why is it right and why would you want to force people to do that?
>>>>>>
>>>>>>
>>>>>
>>>>> No answer?
>>>>> I think it is worth keeping this as a default firmware layout.
>>>>>
>>>>
>>>> I was not happy removing the layout at first, too. However, they are
>>>> added rarely (at least recently) and the binding documentation [1]
>>>> states that they shall only be added if there are "strong conventions".
>>>
>>> Well, not exactly, it says:
>>>
>>> "Partitions can be represented by sub-nodes of an mtd device. This can be used
>>> on platforms which have strong conventions about which portions of a flash are
>>> used for what purposes, but which don't use an on-flash partition table such
>>> as RedBoot."
>>>
>>> In my understanding, "can" is _not_ exactly "shall only".
>>>
>>
>> Definitely, "state" was the wrong word here - sorry about that. But it's
>> how I interpret the documentation. It is the only one I have seen that
>> specifies an allowed use case. So for me it is more like "deny all,
>> allow ...".
> 
> That moves the discussion to what "strong conventions" are?
> But you have already removed the layout, so this is meaningless.

Yes, as I said I want to have the non controversial stuff in first.
Afterwards, we can submit more controversial patches (per feature).

>>
>>>>
>>>> Moreover, it is really easy to pass the layout via firmware/u-boot.
>>>> Unfortunately, the u-boot provided by CompuLab does not have the
>>>> mtdparts commands built-in but the layout can be passed via cmdline as
>>>> well. This seems more flexible to me (and I consider this to be good).
>>>
>>> That means you need a different version of U-Boot if you want to use
>>> an upstream kernel, or you need to play the bootargs games, or just leave
>>> it a "black box".
>>> So currently, from the three options above, the "black box" is chosen, right?
>>> That's a pity, because I'd like things to be open...
>>>
>>
>> Well, It wouldn't be the first ARM device where the newest/upstream
>> u-boot is required. Actually, you already have to use the newest version
>> if you want to use the igb ethernet.
> 
> You mean, igb in U-Boot, right?
> Unless I'm missing something, it should work just fine in Linux with
> the same U-Boot Compulab provides.
> 
I mean in U-Boot and Linux (and with "newest U-Boot" I refer to the one
provided by CompuLab; as far as I know there is no upstream U-Boot
support for the Utilite). Although it seems to work "by accident". The
custom CompuLab code fails to put the mac address in the devicetree but
stores it in the correct environment variable such that the upstream
U-Boot magic can take over.

Anyway, are there any U-Boot updates scheduled (by CompuLab)? And is
there a way to contribute/discuss changes?

>>
>> Currently, IMO the most convenient way (for distributions) would be to
>> ship a boot.scr file changing the bootargs (which is not a "black box"
>> but a configuration file people are used to). If you don't want to mess
>> with bootargs/cmdline it is also possible to add the partition table via
>> boot.scr (or on-flash environment) with the fdt command; in this case it
>> wouldn't even be hidden in the u-boot binary (and no u-boot upgrade is
>> required to implement this).
> 
> That is exactly what I said: "to play the bootargs games".
> 
>>
>> But yes, if someone wants to use the upstream kernel the boot.scr has to
>> be there; but IMO this is something distributions/people providing
>> binary files have to take care about. People using their own kernel
>> should take into account that they have to provide a bootloader config.
> 
> Yep. While if we provide the default layout in DT, all the above would be
> unnecessary.
> 

On the other hand, it will be very hard to change it afterwards. Maybe
someone wants to upstream the U-Boot support and they insist on a
different partition layout (e.g. uboot-failsafe) ...
I never got why people decided to go without a on-device partition table
for most raw flash chips. It is a pity that it has to be hard coded in
the firmware/kernel/whatever.

> So, if one will neither use the latest U-Boot, nor play the bootargs
> games, the flash will stay a kind of "black box" - and that's a pity.
> 

As I stated before, IMHO using the latest U-Boot is no problem and is
already required (for full support).

Cheers,
Christopher
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6

[Igor Grinberg]

Hi Christopher,

On 06/07/2016 12:45 PM, Christopher Spinrath wrote:
> Hi Igor,
> 
> On 06/07/2016 10:38 AM, Igor Grinberg wrote:
>> Hi Christopher,
>>
>> On 06/06/2016 01:37 PM, Christopher Spinrath wrote:
>>> Hi Igor,
>>>
>>> On 06/06/2016 08:49 AM, Igor Grinberg wrote:
>>>> Hi Christopher,
>>>>
>>>> On 06/05/2016 08:32 PM, Christopher Spinrath wrote:
>>>>> Hi Igor,
>>>>>
>>>>> On 06/05/2016 06:43 PM, Igor Grinberg wrote:
>>>>>> Hi Christopher,
>>>>>>
>>>>>> On 05/26/2016 02:37 PM, Igor Grinberg wrote:
>>>>>>> On 05/26/2016 12:50 PM, Lucas Stach wrote:
>>>>>>>> Hi Igor,
>>>>>>>>
>>>>>>>> Am Donnerstag, den 26.05.2016, 11:50 +0300 schrieb Igor Grinberg:
>>>>>>>>> Hi Lucas,
>>>>>>>>>
>>>>>>>>> Thanks for reviewing the patch(es).
>>>>>>>>>
>>>>>>>>> On 05/23/2016 12:03 PM, Lucas Stach wrote:
>>>>>>>>>> Am Montag, den 23.05.2016, 00:47 +0200 schrieb
>>>>>>>>>> christopher.spinrath@rwth-aachen.de:
>>>>>>>>>>> From: Christopher Spinrath <christopher.spinrath@rwth-aachen.de>
>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>>> +&ecspi1 {
>>>>>>>>>>> +	fsl,spi-num-chipselects = <2>;
>>>>>>>>>>> +	cs-gpios = <&gpio2 30 0>, <&gpio3 19 0>;
>>>>>>>>>>> +	pinctrl-names = "default";
>>>>>>>>>>> +	pinctrl-0 = <&pinctrl_ecspi1>;
>>>>>>>>>>> +	status = "okay";
>>>>>>>>>>> +
>>>>>>>>>>> +	flash: m25p80@0 {
>>>>>>>>>>> +		#address-cells = <1>;
>>>>>>>>>>> +		#size-cells = <1>;
>>>>>>>>>>> +		compatible = "st,m25p", "jedec,spi-nor";
>>>>>>>>>>> +		spi-max-frequency = <20000000>;
>>>>>>>>>>> +		reg = <0>;
>>>>>>>>>>> +
>>>>>>>>>>> +		partition@0 {
>>>>>>>>>>> +			label = "uboot";
>>>>>>>>>>> +			reg = <0x0 0xc0000>;
>>>>>>>>>>> +		};
>>>>>>>>>>> +
>>>>>>>>>>> +		partition@c0000 {
>>>>>>>>>>> +			label = "uboot environment";
>>>>>>>>>>> +			reg = <0xc0000 0x40000>;
>>>>>>>>>>> +		};
>>>>>>>>>>> +
>>>>>>>>>>> +		partition@100000 {
>>>>>>>>>>> +			label = "reserved";
>>>>>>>>>>> +			reg = <0x100000 0x100000>;
>>>>>>>>>>> +		};
>>>>>>>>>>
>>>>>>>>>> Partition layouts don't belong in the upstream DT, as it a device
>>>>>>>>>> configuration thing. Please kep them in the bootloader/firmware and make
>>>>>>>>>> this one pass the partition layout to the kernel.
>>>>>>>>>
>>>>>>>>> I don't completely agree with this.
>>>>>>>>> We have lots of partition layouts in the upstream DT.
>>>>>>>>
>>>>>>>> No, we don't. At least not for the i.MX6. There are some for the earlier
>>>>>>>> i.MX boards, but IMO it's wrong to put device configuration into the
>>>>>>>> upstream DT. Let's not start doing this again.
>>>>>>>
>>>>>>> Why not?
>>>>>>> For i.MX6 there are 2 boards that have the partitioning scheme.
>>>>>>> I'm not considering this a device configuration, but rather
>>>>>>> a default partitioning layout/scheme.
>>>>>>> Current case is for the firmware storage device that is not likely
>>>>>>> to change.
>>>>>>> Moreover, a DT is not really a part of the kernel, but lays along the kernel
>>>>>>> sources for convenience and simplicity (at least IIRC as it was decided
>>>>>>> about 5 years ago). It is more a part of the firmware for a device, than
>>>>>>> an upstream kernel source code.
>>>>>>> I think it is only a meter of time when Linus will decide that he does not
>>>>>>> want it inside the kernel anymore...
>>>>>>>
>>>>>>>>
>>>>>>>>> Moreover, this is the default layout and changing it, will
>>>>>>>>> result in incompatibilities and also might result in device "bricking".
>>>>>>>>> Those can be changed from the boot loader in case you need those
>>>>>>>>> the other way around.
>>>>>>>>> Another question of mine is, why should you?
>>>>>>>>>
>>>>>>>> Partition layout is device configuration, which is governed by the
>>>>>>>> device firmware.
>>>>>>>
>>>>>>> Yet again, DT is a part of device firmware.
>>>>>>> Moreover, the firmware (in that case U-Boot), can be configured
>>>>>>> using the very same DT code, so not having this in, might force
>>>>>>> various w/a and hacks.
>>>>>>>
>>>>>>>> By not having the partition layout in the upstream DT
>>>>>>>> people are forced to set it from the firmware, which is exactly the
>>>>>>>> right thing to do, weather or not you plan to change it at any time.
>>>>>>>
>>>>>>> I might be ignorant, sorry for that.
>>>>>>> Why? Why is it right and why would you want to force people to do that?
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> No answer?
>>>>>> I think it is worth keeping this as a default firmware layout.
>>>>>>
>>>>>
>>>>> I was not happy removing the layout at first, too. However, they are
>>>>> added rarely (at least recently) and the binding documentation [1]
>>>>> states that they shall only be added if there are "strong conventions".
>>>>
>>>> Well, not exactly, it says:
>>>>
>>>> "Partitions can be represented by sub-nodes of an mtd device. This can be used
>>>> on platforms which have strong conventions about which portions of a flash are
>>>> used for what purposes, but which don't use an on-flash partition table such
>>>> as RedBoot."
>>>>
>>>> In my understanding, "can" is _not_ exactly "shall only".
>>>>
>>>
>>> Definitely, "state" was the wrong word here - sorry about that. But it's
>>> how I interpret the documentation. It is the only one I have seen that
>>> specifies an allowed use case. So for me it is more like "deny all,
>>> allow ...".
>>
>> That moves the discussion to what "strong conventions" are?
>> But you have already removed the layout, so this is meaningless.
> 
> Yes, as I said I want to have the non controversial stuff in first.
> Afterwards, we can submit more controversial patches (per feature).
> 
>>>
>>>>>
>>>>> Moreover, it is really easy to pass the layout via firmware/u-boot.
>>>>> Unfortunately, the u-boot provided by CompuLab does not have the
>>>>> mtdparts commands built-in but the layout can be passed via cmdline as
>>>>> well. This seems more flexible to me (and I consider this to be good).
>>>>
>>>> That means you need a different version of U-Boot if you want to use
>>>> an upstream kernel, or you need to play the bootargs games, or just leave
>>>> it a "black box".
>>>> So currently, from the three options above, the "black box" is chosen, right?
>>>> That's a pity, because I'd like things to be open...
>>>>
>>>
>>> Well, It wouldn't be the first ARM device where the newest/upstream
>>> u-boot is required. Actually, you already have to use the newest version
>>> if you want to use the igb ethernet.
>>
>> You mean, igb in U-Boot, right?
>> Unless I'm missing something, it should work just fine in Linux with
>> the same U-Boot Compulab provides.
>>
> I mean in U-Boot and Linux (and with "newest U-Boot" I refer to the one
> provided by CompuLab; as far as I know there is no upstream U-Boot
> support for the Utilite).

Not exactly...
board/compulab/cm_fx6 is the one.
That is off topic for this list, so we'd better go off list on this.

> Although it seems to work "by accident".

:-) no accidents here...

> The
> custom CompuLab code fails to put the mac address in the devicetree but
> stores it in the correct environment variable such that the upstream
> U-Boot magic can take over.

Interesting... off list.

> 
> Anyway, are there any U-Boot updates scheduled (by CompuLab)? And is
> there a way to contribute/discuss changes?

Of course. You have my email.

> 
>>>
>>> Currently, IMO the most convenient way (for distributions) would be to
>>> ship a boot.scr file changing the bootargs (which is not a "black box"
>>> but a configuration file people are used to). If you don't want to mess
>>> with bootargs/cmdline it is also possible to add the partition table via
>>> boot.scr (or on-flash environment) with the fdt command; in this case it
>>> wouldn't even be hidden in the u-boot binary (and no u-boot upgrade is
>>> required to implement this).
>>
>> That is exactly what I said: "to play the bootargs games".
>>
>>>
>>> But yes, if someone wants to use the upstream kernel the boot.scr has to
>>> be there; but IMO this is something distributions/people providing
>>> binary files have to take care about. People using their own kernel
>>> should take into account that they have to provide a bootloader config.
>>
>> Yep. While if we provide the default layout in DT, all the above would be
>> unnecessary.
>>
> 
> On the other hand, it will be very hard to change it afterwards. Maybe
> someone wants to upstream the U-Boot support and they insist on a
> different partition layout (e.g. uboot-failsafe) ...

U-Boot support is upstream already since v2014.10.

> I never got why people decided to go without a on-device partition table
> for most raw flash chips. It is a pity that it has to be hard coded in
> the firmware/kernel/whatever.

Right. I agree completely. I guess there are some historical reasons
back to the MTD subsystem 'invention'.

> 
>> So, if one will neither use the latest U-Boot, nor play the bootargs
>> games, the flash will stay a kind of "black box" - and that's a pity.
>>
> 
> As I stated before, IMHO using the latest U-Boot is no problem and is
> already required (for full support).
> 
> Cheers,
> Christopher
> 

-- 
Regards,
Igor.