Hi Jason, I have just send a new series which I hope will supersede this one. However I will answer on some points:
On 01/01/2014 22:41, Jason Cooper wrote: > Gregory, > > Sorry, but we seem to have a fundamental mis-understanding here. First, > whatever we end up deciding for the compatible strings needs to be > documented. Which seems to have not made it into this series. > > Second, I'm having trouble explaining this (in my head), so I'm adding > the DT ml so hopefully someone there can chime in. > > AFAICT, the marvell,mv78230-i2c compatible string, added in v3.12, refers > to the IP block on the A0 revision of the SoC. Since we have set that, Actually in v3.12 the marvell,mv78230-i2c refer to the B0 revision as I only worked on it on board using this revision. That's why I proposed to introduced the marvell,mv78230-a0-i2c compatible string. My concerns was to not break the existing behavior. Currently with the marvell,mv78230-i2c compatible string the kernel use offload on B0 version and hang on A0 version. If we introduce the marvell,mv78230-a0-i2c compatible string, then the new kernel with the same dtb will have the same behavior. People using the A0 version are aware that there is a problem (the kernel hang) so they will be willing to switch to marvell,mv78230-a0-i2c and to change their dtb. If we introduce the marvell,mv78230-b0-i2c compatible string, then the new kernel with the same dtb will have different behavior. People using the A0 will have a booting kernel, but I fear that people using B0 revision won't be aware of the regression. Gregory > we've discovered that the A0 revision has an errata where offloading > doesn't work. The B0 revision of the SoC has fixed offloading for i2c. > > In my mind, this means that we should create a fix for the driver to > disable offloading unless it can determine it's running B0 or newer. > This is easy once we nail down the compatible strings. > > The second thing we need to do is update the binding documentation so > that the devicetree can describe the hardware accurately. I think we > should keep 'marvell,mv78230-i2c' as it is (the driver fix mentioned > above will disable offloading), and add a new compatible > string, 'marvell,mv78230-b0-i2c'. The driver can then be updated to > enable offloading when it sees this compatible string, or it can > determine the SoC revision itself (via mach code). > > Third, we need to be able to differentiate between the two shipped > AX3-4 boards by openblocks. I think this should follow the same pattern > we decide for the i2c compatible string. So, if we go with my proposal, > we would have plathome,openblocks-ax3-4 and > plathome,openblocks-ax3-4-b0. > > Basically, I'm really uneasy about dropping marvell,mv78230-i2c and > essentially re-defining it to marvell,mv78230-a0-i2c. It feels like > it's breaking backwards compatibility. But I'm having trouble clearly > describing how Gregory's proposed change exactly does break backwards > compatibility. Can a DT maintainer explain it more clearly than I? > > thx, > > Jason. > > PS- Well, this ended up being a toppost. Oops. Sorry. > > On Tue, Dec 31, 2013 at 05:44:51PM +0100, Gregory CLEMENT wrote: >> The first variants of Openblocks AX3-4 used the revision A0 of the >> Armada XP SoCs. These early variants have issues related to the i2c >> controller which prevent to use the offload mechanism and lead to a >> kernel hang during boot. >> >> The new dts file uses the compatible string marvell,mv78230-a0-i2c for >> the i2c controller, thanks to this the driver disable the offload >> mechanism and the kernel no more hangs on these boards. >> >> Signed-off-by: Gregory CLEMENT <[email protected]> >> --- >> .../arm/boot/dts/armada-xp-a0-openblocks-ax3-4.dts | 40 +++++ >> .../dts/armada-xp-common-openblocks-ax3-4.dtsi | 177 >> +++++++++++++++++++++ >> arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts | 164 +------------------ >> 3 files changed, 218 insertions(+), 163 deletions(-) >> create mode 100644 arch/arm/boot/dts/armada-xp-a0-openblocks-ax3-4.dts >> create mode 100644 arch/arm/boot/dts/armada-xp-common-openblocks-ax3-4.dtsi >> >> diff --git a/arch/arm/boot/dts/armada-xp-a0-openblocks-ax3-4.dts >> b/arch/arm/boot/dts/armada-xp-a0-openblocks-ax3-4.dts >> new file mode 100644 >> index 000000000000..b3ea65255c19 >> --- /dev/null >> +++ b/arch/arm/boot/dts/armada-xp-a0-openblocks-ax3-4.dts >> @@ -0,0 +1,40 @@ >> +/* >> + * Device Tree file for OpenBlocks AX3-4 board with A0 SoC >> + * >> + * Copyright (C) 2012 Marvell >> + * >> + * Gregory CLEMENT <[email protected]> >> + * >> + * This file is licensed under the terms of the GNU General Public >> + * License version 2. This program is licensed "as is" without any >> + * warranty of any kind, whether express or implied. >> + */ >> + >> +/dts-v1/; >> +#include "armada-xp-common-openblocks-ax3-4.dtsi" >> + >> +/ { >> + model = "PlatHome OpenBlocks AX3-4 board (A0 SoC)"; >> + compatible = "plathome,openblocks-ax3-4", "marvell,armadaxp-mv78260", >> "marvell,armadaxp", "marvell,armada-370-xp"; >> + >> + chosen { >> + bootargs = "console=ttyS0,115200 earlyprintk"; >> + }; >> + >> + memory { >> + device_type = "memory"; >> + reg = <0 0x00000000 0 0xC0000000>; /* 3 GB */ >> + }; >> + >> + soc { >> + >> + internal-regs { >> + i2c@11000 { >> + compatible = "marvell,mv78230-a0-i2c", >> "marvell,mv64xxx-i2c"; >> + }; >> + i2c@11100 { >> + compatible = "marvell,mv78230-a0-i2c", >> "marvell,mv64xxx-i2c"; >> + }; >> + }; >> + }; >> +}; >> diff --git a/arch/arm/boot/dts/armada-xp-common-openblocks-ax3-4.dtsi >> b/arch/arm/boot/dts/armada-xp-common-openblocks-ax3-4.dtsi >> new file mode 100644 >> index 000000000000..0d452b07baf5 >> --- /dev/null >> +++ b/arch/arm/boot/dts/armada-xp-common-openblocks-ax3-4.dtsi >> @@ -0,0 +1,177 @@ >> +/* >> + * Device Tree file for OpenBlocks AX3-4 board >> + * >> + * Copyright (C) 2012 Marvell >> + * >> + * Thomas Petazzoni <[email protected]> >> + * >> + * This file is licensed under the terms of the GNU General Public >> + * License version 2. This program is licensed "as is" without any >> + * warranty of any kind, whether express or implied. >> + */ >> + >> +#include "armada-xp-mv78260.dtsi" >> + >> +/ { >> + soc { >> + ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000 >> + MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000 >> + MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>; >> + >> + devbus-bootcs { >> + status = "okay"; >> + >> + /* Device Bus parameters are required */ >> + >> + /* Read parameters */ >> + devbus,bus-width = <8>; >> + devbus,turn-off-ps = <60000>; >> + devbus,badr-skew-ps = <0>; >> + devbus,acc-first-ps = <124000>; >> + devbus,acc-next-ps = <248000>; >> + devbus,rd-setup-ps = <0>; >> + devbus,rd-hold-ps = <0>; >> + >> + /* Write parameters */ >> + devbus,sync-enable = <0>; >> + devbus,wr-high-ps = <60000>; >> + devbus,wr-low-ps = <60000>; >> + devbus,ale-wr-ps = <60000>; >> + >> + /* NOR 128 MiB */ >> + nor@0 { >> + compatible = "cfi-flash"; >> + reg = <0 0x8000000>; >> + bank-width = <2>; >> + }; >> + }; >> + >> + pcie-controller { >> + status = "okay"; >> + /* Internal mini-PCIe connector */ >> + pcie@1,0 { >> + /* Port 0, Lane 0 */ >> + status = "okay"; >> + }; >> + }; >> + >> + internal-regs { >> + serial@12000 { >> + clock-frequency = <250000000>; >> + status = "okay"; >> + }; >> + serial@12100 { >> + clock-frequency = <250000000>; >> + status = "okay"; >> + }; >> + pinctrl { >> + led_pins: led-pins-0 { >> + marvell,pins = "mpp49", "mpp51", >> "mpp53"; >> + marvell,function = "gpio"; >> + }; >> + }; >> + leds { >> + compatible = "gpio-leds"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&led_pins>; >> + >> + red_led { >> + label = "red_led"; >> + gpios = <&gpio1 17 1>; >> + default-state = "off"; >> + }; >> + >> + yellow_led { >> + label = "yellow_led"; >> + gpios = <&gpio1 19 1>; >> + default-state = "off"; >> + }; >> + >> + green_led { >> + label = "green_led"; >> + gpios = <&gpio1 21 1>; >> + default-state = "off"; >> + linux,default-trigger = "heartbeat"; >> + }; >> + }; >> + >> + gpio_keys { >> + compatible = "gpio-keys"; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + button@1 { >> + label = "Init Button"; >> + linux,code = <116>; >> + gpios = <&gpio1 28 0>; >> + }; >> + }; >> + >> + mdio { >> + phy0: ethernet-phy@0 { >> + reg = <0>; >> + }; >> + >> + phy1: ethernet-phy@1 { >> + reg = <1>; >> + }; >> + >> + phy2: ethernet-phy@2 { >> + reg = <2>; >> + }; >> + >> + phy3: ethernet-phy@3 { >> + reg = <3>; >> + }; >> + }; >> + >> + ethernet@70000 { >> + status = "okay"; >> + phy = <&phy0>; >> + phy-mode = "sgmii"; >> + }; >> + ethernet@74000 { >> + status = "okay"; >> + phy = <&phy1>; >> + phy-mode = "sgmii"; >> + }; >> + ethernet@30000 { >> + status = "okay"; >> + phy = <&phy2>; >> + phy-mode = "sgmii"; >> + }; >> + ethernet@34000 { >> + status = "okay"; >> + phy = <&phy3>; >> + phy-mode = "sgmii"; >> + }; >> + i2c@11000 { >> + status = "okay"; >> + clock-frequency = <400000>; >> + }; >> + i2c@11100 { >> + status = "okay"; >> + clock-frequency = <400000>; >> + >> + s35390a: s35390a@30 { >> + compatible = "s35390a"; >> + reg = <0x30>; >> + }; >> + }; >> + sata@a0000 { >> + nr-ports = <2>; >> + status = "okay"; >> + }; >> + >> + /* Front side USB 0 */ >> + usb@50000 { >> + status = "okay"; >> + }; >> + >> + /* Front side USB 1 */ >> + usb@51000 { >> + status = "okay"; >> + }; >> + }; >> + }; >> +}; >> diff --git a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts >> b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts >> index 5695afcc04bf..1983de77c3ff 100644 >> --- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts >> +++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts >> @@ -11,7 +11,7 @@ >> */ >> >> /dts-v1/; >> -#include "armada-xp-mv78260.dtsi" >> +#include "armada-xp-common-openblocks-ax3-4.dtsi" >> >> / { >> model = "PlatHome OpenBlocks AX3-4 board"; >> @@ -25,166 +25,4 @@ >> device_type = "memory"; >> reg = <0 0x00000000 0 0xC0000000>; /* 3 GB */ >> }; >> - >> - soc { >> - ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000 >> - MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000 >> - MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x8000000>; >> - >> - devbus-bootcs { >> - status = "okay"; >> - >> - /* Device Bus parameters are required */ >> - >> - /* Read parameters */ >> - devbus,bus-width = <8>; >> - devbus,turn-off-ps = <60000>; >> - devbus,badr-skew-ps = <0>; >> - devbus,acc-first-ps = <124000>; >> - devbus,acc-next-ps = <248000>; >> - devbus,rd-setup-ps = <0>; >> - devbus,rd-hold-ps = <0>; >> - >> - /* Write parameters */ >> - devbus,sync-enable = <0>; >> - devbus,wr-high-ps = <60000>; >> - devbus,wr-low-ps = <60000>; >> - devbus,ale-wr-ps = <60000>; >> - >> - /* NOR 128 MiB */ >> - nor@0 { >> - compatible = "cfi-flash"; >> - reg = <0 0x8000000>; >> - bank-width = <2>; >> - }; >> - }; >> - >> - pcie-controller { >> - status = "okay"; >> - /* Internal mini-PCIe connector */ >> - pcie@1,0 { >> - /* Port 0, Lane 0 */ >> - status = "okay"; >> - }; >> - }; >> - >> - internal-regs { >> - serial@12000 { >> - clock-frequency = <250000000>; >> - status = "okay"; >> - }; >> - serial@12100 { >> - clock-frequency = <250000000>; >> - status = "okay"; >> - }; >> - pinctrl { >> - led_pins: led-pins-0 { >> - marvell,pins = "mpp49", "mpp51", >> "mpp53"; >> - marvell,function = "gpio"; >> - }; >> - }; >> - leds { >> - compatible = "gpio-leds"; >> - pinctrl-names = "default"; >> - pinctrl-0 = <&led_pins>; >> - >> - red_led { >> - label = "red_led"; >> - gpios = <&gpio1 17 1>; >> - default-state = "off"; >> - }; >> - >> - yellow_led { >> - label = "yellow_led"; >> - gpios = <&gpio1 19 1>; >> - default-state = "off"; >> - }; >> - >> - green_led { >> - label = "green_led"; >> - gpios = <&gpio1 21 1>; >> - default-state = "off"; >> - linux,default-trigger = "heartbeat"; >> - }; >> - }; >> - >> - gpio_keys { >> - compatible = "gpio-keys"; >> - #address-cells = <1>; >> - #size-cells = <0>; >> - >> - button@1 { >> - label = "Init Button"; >> - linux,code = <116>; >> - gpios = <&gpio1 28 0>; >> - }; >> - }; >> - >> - mdio { >> - phy0: ethernet-phy@0 { >> - reg = <0>; >> - }; >> - >> - phy1: ethernet-phy@1 { >> - reg = <1>; >> - }; >> - >> - phy2: ethernet-phy@2 { >> - reg = <2>; >> - }; >> - >> - phy3: ethernet-phy@3 { >> - reg = <3>; >> - }; >> - }; >> - >> - ethernet@70000 { >> - status = "okay"; >> - phy = <&phy0>; >> - phy-mode = "sgmii"; >> - }; >> - ethernet@74000 { >> - status = "okay"; >> - phy = <&phy1>; >> - phy-mode = "sgmii"; >> - }; >> - ethernet@30000 { >> - status = "okay"; >> - phy = <&phy2>; >> - phy-mode = "sgmii"; >> - }; >> - ethernet@34000 { >> - status = "okay"; >> - phy = <&phy3>; >> - phy-mode = "sgmii"; >> - }; >> - i2c@11000 { >> - status = "okay"; >> - clock-frequency = <400000>; >> - }; >> - i2c@11100 { >> - status = "okay"; >> - clock-frequency = <400000>; >> - >> - s35390a: s35390a@30 { >> - compatible = "s35390a"; >> - reg = <0x30>; >> - }; >> - }; >> - sata@a0000 { >> - nr-ports = <2>; >> - status = "okay"; >> - }; >> - >> - /* Front side USB 0 */ >> - usb@50000 { >> - status = "okay"; >> - }; >> - >> - /* Front side USB 1 */ >> - usb@51000 { >> - status = "okay"; >> - }; >> - }; >> - }; >> }; >> -- >> 1.8.1.2 >> -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
