Re: [RFC PATCH 1/3] arm: mvebu: add new dts file for old variant of Openblocks AX3-4

[Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

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 <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> ---
>>  .../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 <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> + *
>> + * 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 <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> + *
>> + * 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