* [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6
@ 2016-05-22 22:47 christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg
[not found] ` <709d6b3d522f4fcb8112830e0426dbb3-gtPewvpZjL8umhiu9RXYRl5UTUQ924AY@public.gmane.org>
[not found] ` <1c18d1b1f80b47cab2d06b1167407ae8@rwthex-s2-a.rwth-ad.de>
0 siblings, 2 replies; 13+ messages in thread
From: christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg @ 2016-05-22 22:47 UTC (permalink / raw)
To: shawnguo-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, linux-lFZ/pmaqli7XmaaqVzeoHQ,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA, Christopher Spinrath
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 = <®_usb_otg_vbus>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usbotg>;
+ dr_mode = "otg";
+ status = "okay";
+};
+
+&usbh1 {
+ vbus-supply = <®_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
^ permalink raw reply related [flat|nested] 13+ messages in thread[parent not found: <709d6b3d522f4fcb8112830e0426dbb3-gtPewvpZjL8umhiu9RXYRl5UTUQ924AY@public.gmane.org>]
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 [not found] ` <709d6b3d522f4fcb8112830e0426dbb3-gtPewvpZjL8umhiu9RXYRl5UTUQ924AY@public.gmane.org> @ 2016-05-23 9:03 ` Lucas Stach [not found] ` <1463994191.2370.3.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 0 siblings, 1 reply; 13+ messages in thread From: Lucas Stach @ 2016-05-23 9:03 UTC (permalink / raw) To: christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg Cc: shawnguo-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ, mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-lFZ/pmaqli7XmaaqVzeoHQ, pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, galak-sgV2jX0FEOL9JmXXK+q4OQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 = <®_usb_otg_vbus>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_usbotg>; > + dr_mode = "otg"; > + status = "okay"; > +}; > + > +&usbh1 { > + vbus-supply = <®_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 ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <1463994191.2370.3.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 [not found] ` <1463994191.2370.3.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> @ 2016-05-26 8:50 ` Igor Grinberg [not found] ` <5746B8C4.6020406-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org> 0 siblings, 1 reply; 13+ messages in thread From: Igor Grinberg @ 2016-05-26 8:50 UTC (permalink / raw) To: Lucas Stach, christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg Cc: mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-lFZ/pmaqli7XmaaqVzeoHQ, pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ, galak-sgV2jX0FEOL9JmXXK+q4OQ, shawnguo-DgEjT+Ai2ygdnm+yROfE0A, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <5746B8C4.6020406-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>]
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 [not found] ` <5746B8C4.6020406-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org> @ 2016-05-26 9:50 ` Lucas Stach [not found] ` <1464256217.14453.10.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 0 siblings, 1 reply; 13+ messages in thread From: Lucas Stach @ 2016-05-26 9:50 UTC (permalink / raw) To: Igor Grinberg Cc: christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg, mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-lFZ/pmaqli7XmaaqVzeoHQ, pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ, galak-sgV2jX0FEOL9JmXXK+q4OQ, shawnguo-DgEjT+Ai2ygdnm+yROfE0A, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <1464256217.14453.10.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>]
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 [not found] ` <1464256217.14453.10.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> @ 2016-05-26 11:37 ` Igor Grinberg [not found] ` <5746DFE2.9040903-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org> [not found] ` <d083d0308abd48efbd4b6042e1f9c470@rwthex-s2-a.rwth-ad.de> 0 siblings, 2 replies; 13+ messages in thread From: Igor Grinberg @ 2016-05-26 11:37 UTC (permalink / raw) To: Lucas Stach Cc: christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg, mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-lFZ/pmaqli7XmaaqVzeoHQ, pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ, galak-sgV2jX0FEOL9JmXXK+q4OQ, shawnguo-DgEjT+Ai2ygdnm+yROfE0A, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <5746DFE2.9040903-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>]
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 [not found] ` <5746DFE2.9040903-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org> @ 2016-06-05 16:43 ` Igor Grinberg 0 siblings, 0 replies; 13+ messages in thread From: Igor Grinberg @ 2016-06-05 16:43 UTC (permalink / raw) To: christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg Cc: Lucas Stach, mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA, pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ, galak-sgV2jX0FEOL9JmXXK+q4OQ, linux-lFZ/pmaqli7XmaaqVzeoHQ, shawnguo-DgEjT+Ai2ygdnm+yROfE0A, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <d083d0308abd48efbd4b6042e1f9c470@rwthex-s2-a.rwth-ad.de>]
[parent not found: <d083d0308abd48efbd4b6042e1f9c470-gtPewvpZjL+1MzRH+ruthl5UTUQ924AY@public.gmane.org>]
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 [not found] ` <d083d0308abd48efbd4b6042e1f9c470-gtPewvpZjL+1MzRH+ruthl5UTUQ924AY@public.gmane.org> @ 2016-06-05 17:32 ` Christopher Spinrath [not found] ` <d0329c33bee14d6e8be8abef607dce3e-gtPewvpZjL8umhiu9RXYRl5UTUQ924AY@public.gmane.org> 0 siblings, 1 reply; 13+ messages in thread From: Christopher Spinrath @ 2016-06-05 17:32 UTC (permalink / raw) To: Igor Grinberg Cc: Lucas Stach, mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA, pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ, galak-sgV2jX0FEOL9JmXXK+q4OQ, linux-lFZ/pmaqli7XmaaqVzeoHQ, shawnguo-DgEjT+Ai2ygdnm+yROfE0A, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Spinrath, Christopher 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <d0329c33bee14d6e8be8abef607dce3e-gtPewvpZjL8umhiu9RXYRl5UTUQ924AY@public.gmane.org>]
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 [not found] ` <d0329c33bee14d6e8be8abef607dce3e-gtPewvpZjL8umhiu9RXYRl5UTUQ924AY@public.gmane.org> @ 2016-06-06 6:49 ` Igor Grinberg [not found] ` <57551CF6.2020602-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org> 0 siblings, 1 reply; 13+ messages in thread From: Igor Grinberg @ 2016-06-06 6:49 UTC (permalink / raw) To: Christopher Spinrath Cc: Lucas Stach, mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA, pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ, galak-sgV2jX0FEOL9JmXXK+q4OQ, linux-lFZ/pmaqli7XmaaqVzeoHQ, shawnguo-DgEjT+Ai2ygdnm+yROfE0A, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <57551CF6.2020602-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>]
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 [not found] ` <57551CF6.2020602-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org> @ 2016-06-06 10:37 ` Christopher Spinrath 2016-06-07 8:38 ` Igor Grinberg [not found] ` <5d1bc50e59f749db8f825f5cb7e2438d@rwthex-w2-a.rwth-ad.de> 0 siblings, 2 replies; 13+ messages in thread From: Christopher Spinrath @ 2016-06-06 10:37 UTC (permalink / raw) To: Igor Grinberg Cc: Lucas Stach, mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA, pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ, galak-sgV2jX0FEOL9JmXXK+q4OQ, linux-lFZ/pmaqli7XmaaqVzeoHQ, shawnguo-DgEjT+Ai2ygdnm+yROfE0A, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Spinrath, Christopher 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 2016-06-06 10:37 ` Christopher Spinrath @ 2016-06-07 8:38 ` Igor Grinberg [not found] ` <5d1bc50e59f749db8f825f5cb7e2438d@rwthex-w2-a.rwth-ad.de> 1 sibling, 0 replies; 13+ messages in thread From: Igor Grinberg @ 2016-06-07 8:38 UTC (permalink / raw) To: Christopher Spinrath Cc: mark.rutland, devicetree, linux, pawel.moll, ijc+devicetree, robh+dt, kernel, galak, shawnguo, linux-arm-kernel, Lucas Stach 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <5d1bc50e59f749db8f825f5cb7e2438d@rwthex-w2-a.rwth-ad.de>]
[parent not found: <5d1bc50e59f749db8f825f5cb7e2438d-cBaz+nnMw1+1MzRH+ruthl5UTUQ924AY@public.gmane.org>]
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 [not found] ` <5d1bc50e59f749db8f825f5cb7e2438d-cBaz+nnMw1+1MzRH+ruthl5UTUQ924AY@public.gmane.org> @ 2016-06-07 9:45 ` Christopher Spinrath 2016-06-08 8:11 ` Igor Grinberg 0 siblings, 1 reply; 13+ messages in thread From: Christopher Spinrath @ 2016-06-07 9:45 UTC (permalink / raw) To: Igor Grinberg Cc: Lucas Stach, mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA, pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ, galak-sgV2jX0FEOL9JmXXK+q4OQ, linux-lFZ/pmaqli7XmaaqVzeoHQ, shawnguo-DgEjT+Ai2ygdnm+yROfE0A, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Spinrath, Christopher 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 2016-06-07 9:45 ` Christopher Spinrath @ 2016-06-08 8:11 ` Igor Grinberg 0 siblings, 0 replies; 13+ messages in thread From: Igor Grinberg @ 2016-06-08 8:11 UTC (permalink / raw) To: Christopher Spinrath Cc: mark.rutland, devicetree, linux, pawel.moll, ijc+devicetree, robh+dt, kernel, galak, shawnguo, linux-arm-kernel, Lucas Stach 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <1c18d1b1f80b47cab2d06b1167407ae8@rwthex-s2-a.rwth-ad.de>]
[parent not found: <1c18d1b1f80b47cab2d06b1167407ae8-gtPewvpZjL+1MzRH+ruthl5UTUQ924AY@public.gmane.org>]
* Re: [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 [not found] ` <1c18d1b1f80b47cab2d06b1167407ae8-gtPewvpZjL+1MzRH+ruthl5UTUQ924AY@public.gmane.org> @ 2016-05-23 22:37 ` Christopher Spinrath 0 siblings, 0 replies; 13+ messages in thread From: Christopher Spinrath @ 2016-05-23 22:37 UTC (permalink / raw) To: Lucas Stach Cc: shawnguo-DgEjT+Ai2ygdnm+yROfE0A, kernel-bIcnvbaLZ9MEGnE8C9+IrQ, mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-lFZ/pmaqli7XmaaqVzeoHQ, pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, galak-sgV2jX0FEOL9JmXXK+q4OQ, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r 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 = <®_usb_otg_vbus>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_usbotg>; >> + dr_mode = "otg"; >> + status = "okay"; >> +}; >> + >> +&usbh1 { >> + vbus-supply = <®_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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-06-08 8:11 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-22 22:47 [PATCH 1/2] ARM: dts: imx6q: extend support for the cm-fx6 christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg
[not found] ` <709d6b3d522f4fcb8112830e0426dbb3-gtPewvpZjL8umhiu9RXYRl5UTUQ924AY@public.gmane.org>
2016-05-23 9:03 ` Lucas Stach
[not found] ` <1463994191.2370.3.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2016-05-26 8:50 ` Igor Grinberg
[not found] ` <5746B8C4.6020406-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
2016-05-26 9:50 ` Lucas Stach
[not found] ` <1464256217.14453.10.camel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2016-05-26 11:37 ` Igor Grinberg
[not found] ` <5746DFE2.9040903-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
2016-06-05 16:43 ` Igor Grinberg
[not found] ` <d083d0308abd48efbd4b6042e1f9c470@rwthex-s2-a.rwth-ad.de>
[not found] ` <d083d0308abd48efbd4b6042e1f9c470-gtPewvpZjL+1MzRH+ruthl5UTUQ924AY@public.gmane.org>
2016-06-05 17:32 ` Christopher Spinrath
[not found] ` <d0329c33bee14d6e8be8abef607dce3e-gtPewvpZjL8umhiu9RXYRl5UTUQ924AY@public.gmane.org>
2016-06-06 6:49 ` Igor Grinberg
[not found] ` <57551CF6.2020602-UTxiZqZC01RS1MOuV/RT9w@public.gmane.org>
2016-06-06 10:37 ` Christopher Spinrath
2016-06-07 8:38 ` Igor Grinberg
[not found] ` <5d1bc50e59f749db8f825f5cb7e2438d@rwthex-w2-a.rwth-ad.de>
[not found] ` <5d1bc50e59f749db8f825f5cb7e2438d-cBaz+nnMw1+1MzRH+ruthl5UTUQ924AY@public.gmane.org>
2016-06-07 9:45 ` Christopher Spinrath
2016-06-08 8:11 ` Igor Grinberg
[not found] ` <1c18d1b1f80b47cab2d06b1167407ae8@rwthex-s2-a.rwth-ad.de>
[not found] ` <1c18d1b1f80b47cab2d06b1167407ae8-gtPewvpZjL+1MzRH+ruthl5UTUQ924AY@public.gmane.org>
2016-05-23 22:37 ` Christopher Spinrath
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).