The Lenovo Iomega ix4-300d is a 4-Bay sata NAS with dual Gb,
USB2.0 & 3.0, powered by a Marvell Armada XP MV78230 dual core CPU.
http://shop.lenovo.com/fr/fr/servers/network-storage/lenovoemc/ix4-300d/
Signed-off-by: Benoit Masson <[email protected]>
---
arch/arm/boot/dts/Makefile | 3 +-
arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts | 284 ++++++++++++++++++++++++
2 files changed, 286 insertions(+), 1 deletion(-)
create mode 100644 arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index adb5ed9..4429495 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -437,8 +437,9 @@ dtb-$(CONFIG_MACH_ARMADA_XP) += \
armada-xp-axpwifiap.dtb \
armada-xp-db.dtb \
armada-xp-gp.dtb \
- armada-xp-netgear-rn2120.dtb \
+ armada-xp-lenovo-ix4-300d.dtb \
armada-xp-matrix.dtb \
+ armada-xp-netgear-rn2120.dtb \
armada-xp-openblocks-ax3-4.dtb
dtb-$(CONFIG_MACH_DOVE) += dove-cm-a510.dtb \
dove-cubox.dtb \
diff --git a/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts b/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
new file mode 100644
index 0000000..1f33cbc
--- /dev/null
+++ b/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
@@ -0,0 +1,284 @@
+/*
+ * Device Tree file for Lenovo Iomega ix4-300d
+ *
+ * Copyright (C) 2014, Benoit Masson <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/gpio/gpio.h>
+#include "armada-xp-mv78230.dtsi"
+
+/ {
+ model = "Lenovo Iomega ix4-300d";
+ compatible = "lenovo,ix4-300d", "marvell,armadaxp-mv78230",
+ "marvell,armadaxp", "marvell,armada-370-xp";
+
+ chosen {
+ bootargs = "console=ttyS0,115200 earlyprintk";
+ stdout-path = "/soc/internal-regs/serial@12000";
+ };
+
+ memory {
+ device_type = "memory";
+ reg = <0 0x00000000 0 0x20000000>; /* 512MB */
+ };
+
+ soc {
+ ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000
+ MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>;
+
+ pcie-controller {
+ status = "okay";
+
+ /* Quad port sata: Marvell 88SX7042 */
+ pcie@1,0 {
+ /* Port 0, Lane 0 */
+ status = "okay";
+ };
+
+ /* USB 3.0 xHCI controller: NEC D720200F1 */
+ pcie@5,0 {
+ /* Port 1, Lane 0 */
+ status = "okay";
+ };
+ };
+
+ internal-regs {
+ pinctrl {
+ poweroff_pin: poweroff-pin {
+ marvell,pins = "mpp24";
+ marvell,function = "gpio";
+ };
+
+ power_button_pin: power-button-pin {
+ marvell,pins = "mpp44";
+ marvell,function = "gpio";
+ };
+
+ reset_button_pin: reset-button-pin {
+ marvell,pins = "mpp45";
+ marvell,function = "gpio";
+ };
+ select_button_pin: select-button-pin {
+ marvell,pins = "mpp41";
+ marvell,function = "gpio";
+ };
+
+ scroll_button_pin: scroll-button-pin {
+ marvell,pins = "mpp42";
+ marvell,function = "gpio";
+ };
+
+ hdd_led_pin: hdd-led-pin {
+ marvell,pins = "mpp26";
+ marvell,function = "gpio";
+ };
+ };
+
+ serial@12000 {
+ status = "okay";
+ };
+
+ mdio {
+ phy0: ethernet-phy@0 { /* Marvell 88E1318 */
+ reg = <0>;
+ };
+
+ phy1: ethernet-phy@1 { /* Marvell 88E1318 */
+ reg = <1>;
+ };
+ };
+
+ ethernet@70000 {
+ status = "okay";
+ phy = <&phy0>;
+ phy-mode = "rgmii-id";
+ };
+
+ ethernet@74000 {
+ status = "okay";
+ phy = <&phy1>;
+ phy-mode = "rgmii-id";
+ };
+
+ usb@50000 {
+ status = "okay";
+ };
+
+ usb@51000 {
+ status = "okay";
+ };
+
+ i2c@11000 {
+ compatible = "marvell,mv78230-a0-i2c",
+ "marvell,mv64xxx-i2c";
+ clock-frequency = <400000>;
+ status = "okay";
+
+ adt7473@2e {
+ compatible = "adi,adt7473";
+ reg = <0x2e>;
+ };
+
+ pcf8563@51 {
+ compatible = "nxp,pcf8563";
+ reg = <0x51>;
+ };
+
+ };
+
+ nand@d0000 {
+ status = "okay";
+ num-cs = <1>;
+ marvell,nand-keep-config;
+ marvell,nand-enable-arbiter;
+ nand-on-flash-bbt;
+
+ partition@0 {
+ label = "u-boot";
+ reg = <0x0000000 0xe0000>;
+ read-only;
+ };
+
+ partition@e0000 {
+ label = "u-boot-env";
+ reg = <0xe0000 0x20000>;
+ read-only;
+ };
+
+ partition@100000 {
+ label = "u-boot-env2";
+ reg = <0x100000 0x20000>;
+ read-only;
+ };
+
+ partition@120000 {
+ label = "zImage";
+ reg = <0x120000 0x400000>;
+ };
+
+ partition@520000 {
+ label = "initrd";
+ reg = <0x520000 0x400000>;
+ };
+
+ partition@xE00000 {
+ label = "boot";
+ reg = <0xE00000 0x3F200000>;
+ };
+
+ partition@flash {
+ label = "flash";
+ reg = <0x0 0x40000000>;
+ };
+ };
+ };
+ };
+
+ gpio-keys {
+ compatible = "gpio-keys";
+ pinctrl-0 = <&power_button_pin &reset_button_pin
+ &select_button_pin &scroll_button_pin>;
+ pinctrl-names = "default";
+
+ power-button {
+ label = "Power Button";
+ linux,code = <KEY_POWER>;
+ gpios = <&gpio1 12 GPIO_ACTIVE_HIGH>;
+ };
+
+ reset-button {
+ label = "Reset Button";
+ linux,code = <KEY_RESTART>;
+ gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
+ };
+
+ select-button {
+ label = "Select Button";
+ linux,code = <BTN_SELECT>;
+ gpios = <&gpio1 9 GPIO_ACTIVE_LOW>;
+ };
+
+ scroll-button {
+ label = "Scroll Button";
+ linux,code = <KEY_SCROLLDOWN>;
+ gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
+ };
+ };
+
+ spi3 {
+ compatible = "spi-gpio";
+ status = "okay";
+ gpio-sck = <&gpio0 25 0>;
+ gpio-mosi = <&gpio1 15 0>; /*gpio 47*/
+ cs-gpios = <&gpio0 27 0 >;
+ num-chipselects = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ gpio2: gpio2@0 {
+ compatible = "fairchild,74hc595";
+ gpio-controller;
+ #gpio-cells = <2>;
+ reg = <0>;
+ registers-number = <2>;
+ spi-max-frequency = <100000>;
+ };
+ };
+
+ gpio-leds {
+ compatible = "gpio-leds";
+ pinctrl-0 = <&hdd_led_pin>;
+ pinctrl-names = "default";
+
+ hdd-led {
+ label = "ix4-300d:hdd:blue";
+ gpios = <&gpio0 26 GPIO_ACTIVE_HIGH>;
+ default-state = "off";
+ };
+
+ power-led {
+ label = "ix4-300d:power:white";
+ gpios = <&gpio2 1 GPIO_ACTIVE_LOW>;
+ /* init blinking while booting */
+ linux,default-trigger = "timer";
+ default-state = "on";
+ };
+
+ sysfail-led {
+ label = "ix4-300d:sysfail:red";
+ gpios = <&gpio2 2 GPIO_ACTIVE_HIGH>;
+ default-state = "off";
+ };
+
+ sys-led {
+ label = "ix4-300d:sys:blue";
+ gpios = <&gpio2 3 GPIO_ACTIVE_HIGH>;
+ default-state = "off";
+ };
+
+ hddfail-led {
+ label = "ix4-300d:hddfail:red";
+ gpios = <&gpio2 4 GPIO_ACTIVE_HIGH>;
+ default-state = "off";
+ };
+
+ };
+
+ /* Warning: you need both eth1 & 0 PHY initialized
+ (i.e having them up does the tweak)
+ for poweroff to shutdown otherwise it reboots */
+ gpio-poweroff {
+ compatible = "gpio-poweroff";
+ pinctrl-0 = <&poweroff_pin>;
+ pinctrl-names = "default";
+ gpios = <&gpio0 24 GPIO_ACTIVE_HIGH>;
+ };
+};
--
1.9.1
On Thu, Jul 24, 2014 at 01:11:12AM +0200, Benoit Masson wrote:
> Le 24 juil. 2014 ? 00:58, Andrew Lunn <[email protected]> a ?crit :
>
> >> For the marvell,mv78230-a0-i2c unfortunately i've spend 3 hours
> >> trying to understand this but it only works with this on the
> >> ix4-300d :(. There was multiple patch around this and maybe one
> >> broke the auto-detect part of this, I've tried compiling with some
> >> 3.10 or lower kernel but no luck here I still have to put this a0
> >> option.
> >
> > Lets first confirm you have an a0 SoC.
> >
> > At boot time, it should print:
> >
> > pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);
> >
> > What revision do you have?
> >
> > If the auto detect code really is broken, Gregory will likely take a
> > look.
>
> I sure do,
>
> confirmed by u-boot output below:
>
> U-Boot 2009.08 (Mar 04 2013 - 11:13:04) Marvell version: 2.3.2 PQ
> U-Boot Addressing:
> Code:..00600000:006BFFF0
> BSS:..00708EC0
> Stack:..0x5fff70
> PageTable:.0x8e0000
> Heap address:.0x900000:0xe00000
> Board: DB-78230-BP rev 2.0 Wistron
> SoC: MV78230 A0
>
> From kernel I get:
>
> mvebu-soc-id: MVEBU SoC ID=0x7823, Rev=0x1
Well, isn't that a peach? :) Gregory?
thx,
Jason.
Benoit,
This looks a lot better, thanks for turning it around so quickly!
My only general comment is just for the future. When submitting new
versions of patches, please add a 'V2' inside the brackets on the
Subject line. Or v3, or v4, or v14 in rare cases ;-)
It really helps us patch wranglers keep track of which version to apply.
One small comment below.
On Wed, Jul 23, 2014 at 03:52:53PM -0700, Benoit Masson wrote:
> The Lenovo Iomega ix4-300d is a 4-Bay sata NAS with dual Gb,
> USB2.0 & 3.0, powered by a Marvell Armada XP MV78230 dual core CPU.
>
> http://shop.lenovo.com/fr/fr/servers/network-storage/lenovoemc/ix4-300d/
> Signed-off-by: Benoit Masson <[email protected]>
> ---
> arch/arm/boot/dts/Makefile | 3 +-
> arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts | 284 ++++++++++++++++++++++++
> 2 files changed, 286 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
...
> diff --git a/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts b/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
> new file mode 100644
> index 0000000..1f33cbc
> --- /dev/null
> +++ b/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
...
> + /* Warning: you need both eth1 & 0 PHY initialized
> + (i.e having them up does the tweak)
> + for poweroff to shutdown otherwise it reboots */
nit: multi-line comments are like this:
/*
* Warning: you need both eth1 & 0 PHY initialized (i.e having
* them up does the tweak) for poweroff to shutdown otherwise it
* reboots
*/
If that's the only thing left, I'll fix it up when I pull it in. No
need to respin just for this.
thx,
Jason.
> On Wed, Jul 23, 2014 at 03:52:53PM -0700, Benoit Masson wrote:
> > The Lenovo Iomega ix4-300d is a 4-Bay sata NAS with dual Gb,
> > USB2.0 & 3.0, powered by a Marvell Armada XP MV78230 dual core CPU.
> >
> > http://shop.lenovo.com/fr/fr/servers/network-storage/lenovoemc/ix4-300d/
> > Signed-off-by: Benoit Masson <[email protected]>
> > ---
> > arch/arm/boot/dts/Makefile | 3 +-
> > arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts | 284 ++++++++++++++++++++++++
> > 2 files changed, 286 insertions(+), 1 deletion(-)
> > create mode 100644 arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
> ...
> > diff --git a/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts b/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
> > new file mode 100644
> > index 0000000..1f33cbc
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
> ...
> > + /* Warning: you need both eth1 & 0 PHY initialized
> > + (i.e having them up does the tweak)
> > + for poweroff to shutdown otherwise it reboots */
>
> nit: multi-line comments are like this:
>
> /*
> * Warning: you need both eth1 & 0 PHY initialized (i.e having
> * them up does the tweak) for poweroff to shutdown otherwise it
> * reboots
> */
>
> If that's the only thing left, I'll fix it up when I pull it in. No
> need to respin just for this.
Hi Jason
We should not really leave the i2c compatibility string as it is.
Hopefully the OEM moved onto B1 stepping at some point. Although B1
devices will work with the workaround, you get better performance
without it.
We should wait for Gregory to look at the quirk and soc-id code.
Andrew
On Thu, Jul 24, 2014 at 01:29:09AM +0200, Andrew Lunn wrote:
> > On Wed, Jul 23, 2014 at 03:52:53PM -0700, Benoit Masson wrote:
> > > The Lenovo Iomega ix4-300d is a 4-Bay sata NAS with dual Gb,
> > > USB2.0 & 3.0, powered by a Marvell Armada XP MV78230 dual core CPU.
> > >
> > > http://shop.lenovo.com/fr/fr/servers/network-storage/lenovoemc/ix4-300d/
> > > Signed-off-by: Benoit Masson <[email protected]>
> > > ---
> > > arch/arm/boot/dts/Makefile | 3 +-
> > > arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts | 284 ++++++++++++++++++++++++
> > > 2 files changed, 286 insertions(+), 1 deletion(-)
> > > create mode 100644 arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
> > ...
> > > diff --git a/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts b/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
> > > new file mode 100644
> > > index 0000000..1f33cbc
> > > --- /dev/null
> > > +++ b/arch/arm/boot/dts/armada-xp-lenovo-ix4-300d.dts
> > ...
> > > + /* Warning: you need both eth1 & 0 PHY initialized
> > > + (i.e having them up does the tweak)
> > > + for poweroff to shutdown otherwise it reboots */
> >
> > nit: multi-line comments are like this:
> >
> > /*
> > * Warning: you need both eth1 & 0 PHY initialized (i.e having
> > * them up does the tweak) for poweroff to shutdown otherwise it
> > * reboots
> > */
> >
> > If that's the only thing left, I'll fix it up when I pull it in. No
> > need to respin just for this.
>
> Hi Jason
>
> We should not really leave the i2c compatibility string as it is.
Agreed.
> Hopefully the OEM moved onto B1 stepping at some point. Although B1
> devices will work with the workaround, you get better performance
> without it.
>
> We should wait for Gregory to look at the quirk and soc-id code.
Yep.
thx,
Jason.
Hi Benoit,
On Wed, Jul 23, 2014 at 03:52:53PM -0700, Benoit Masson wrote:
> The Lenovo Iomega ix4-300d is a 4-Bay sata NAS with dual Gb,
> USB2.0 & 3.0, powered by a Marvell Armada XP MV78230 dual core CPU.
>
> http://shop.lenovo.com/fr/fr/servers/network-storage/lenovoemc/ix4-300d/
I guess most users would prefer an English URL:
http://shop.lenovo.com/us/en/servers/network-storage/lenovoemc/ix4-300d/
Also, the accepted convention is to add a blank line between the commit log
body and the Signed-off-by line.
> Signed-off-by: Benoit Masson <[email protected]>
baruch
--
http://baruch.siach.name/blog/ ~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -
Benoit,
> On Thu, Jul 24, 2014 at 01:29:09AM +0200, Andrew Lunn wrote:
> >
> > We should not really leave the i2c compatibility string as it is.
> >
If you were to apply
ed2d859119f9 ARM: mvebu: Fix broken SoC ID detection
And then remove the -a0 i2c compatible string, does your board boot?
The referenced patch has been in mainline since v3.16-rc3, so you could
just try that kernel w/o the compatible string.
thx,
Jason.
On 24/07/2014 01:15, Jason Cooper wrote:
> On Thu, Jul 24, 2014 at 01:11:12AM +0200, Benoit Masson wrote:
>> Le 24 juil. 2014 ? 00:58, Andrew Lunn <[email protected]> a ?crit :
>>
>>>> For the marvell,mv78230-a0-i2c unfortunately i've spend 3 hours
>>>> trying to understand this but it only works with this on the
>>>> ix4-300d :(. There was multiple patch around this and maybe one
>>>> broke the auto-detect part of this, I've tried compiling with some
>>>> 3.10 or lower kernel but no luck here I still have to put this a0
>>>> option.
>>>
>>> Lets first confirm you have an a0 SoC.
>>>
>>> At boot time, it should print:
>>>
>>> pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);
>>>
>>> What revision do you have?
>>>
>>> If the auto detect code really is broken, Gregory will likely take a
>>> look.
>>
>> I sure do,
>>
>> confirmed by u-boot output below:
>>
>> U-Boot 2009.08 (Mar 04 2013 - 11:13:04) Marvell version: 2.3.2 PQ
>> U-Boot Addressing:
>> Code:..00600000:006BFFF0
>> BSS:..00708EC0
>> Stack:..0x5fff70
>> PageTable:.0x8e0000
>> Heap address:.0x900000:0xe00000
>> Board: DB-78230-BP rev 2.0 Wistron
>> SoC: MV78230 A0
>>
>> From kernel I get:
>>
>> mvebu-soc-id: MVEBU SoC ID=0x7823, Rev=0x1
>
> Well, isn't that a peach? :) Gregory?
A peach?? For me it is either a fruit or a princess, so I am puzzled!
Well about the issue, we patch the device tree for the i2c quirk only for
the openblock AX3. If I remember well it was because the device tree was already
written for this board before we found there was an issue with the A0 version of the
CPU. The rule was that for new boards then they have to set the marvell,mv78230-a0-i2c
compatible string. I also didn't expect that we found "new" product using the A0 version.
We have 3 options now:
- remove the check on the openblock AX3 board and always try to apply the quirck for A0 version
- add a check for this new board in the mvebu_dt_init function
- let the compatible string marvell,mv78230-a0-i2c in this dts
I would prefer the option 1 but I fear that Arnd would prefer the 2 other options.
Gregory
>
> thx,
>
> Jason.
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On Thu, Jul 24, 2014 at 02:21:03PM +0200, Gregory CLEMENT wrote:
> On 24/07/2014 01:15, Jason Cooper wrote:
> > On Thu, Jul 24, 2014 at 01:11:12AM +0200, Benoit Masson wrote:
> >> Le 24 juil. 2014 ? 00:58, Andrew Lunn <[email protected]> a ?crit :
> >>
> >>>> For the marvell,mv78230-a0-i2c unfortunately i've spend 3 hours
> >>>> trying to understand this but it only works with this on the
> >>>> ix4-300d :(. There was multiple patch around this and maybe one
> >>>> broke the auto-detect part of this, I've tried compiling with some
> >>>> 3.10 or lower kernel but no luck here I still have to put this a0
> >>>> option.
> >>>
> >>> Lets first confirm you have an a0 SoC.
> >>>
> >>> At boot time, it should print:
> >>>
> >>> pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);
> >>>
> >>> What revision do you have?
> >>>
> >>> If the auto detect code really is broken, Gregory will likely take a
> >>> look.
> >>
> >> I sure do,
> >>
> >> confirmed by u-boot output below:
> >>
> >> U-Boot 2009.08 (Mar 04 2013 - 11:13:04) Marvell version: 2.3.2 PQ
> >> U-Boot Addressing:
> >> Code:..00600000:006BFFF0
> >> BSS:..00708EC0
> >> Stack:..0x5fff70
> >> PageTable:.0x8e0000
> >> Heap address:.0x900000:0xe00000
> >> Board: DB-78230-BP rev 2.0 Wistron
> >> SoC: MV78230 A0
> >>
> >> From kernel I get:
> >>
> >> mvebu-soc-id: MVEBU SoC ID=0x7823, Rev=0x1
> >
> > Well, isn't that a peach? :) Gregory?
>
> A peach?? For me it is either a fruit or a princess, so I am puzzled!
Doc Holiday quote from the movie Tombstone. The full quote was "Well,
isn't that a peach of a hand?" while sitting at the poker table.
> Well about the issue, we patch the device tree for the i2c quirk only for
> the openblock AX3.
Ahhh, that's right. I need to slow down and dig a bit more. :(
> If I remember well it was because the device tree was already written
> for this board before we found there was an issue with the A0 version
> of the CPU.
No, it's because we didn't think any manufacturers would be silly enough
to use the a0 in production. That assumption worked out well. :-P
> The rule was that for new boards then they have to set the marvell,mv78230-a0-i2c
> compatible string. I also didn't expect that we found "new" product using the A0 version.
>
> We have 3 options now:
>
> - remove the check on the openblock AX3 board and always try to apply the quirck for A0 version
> - add a check for this new board in the mvebu_dt_init function
> - let the compatible string marvell,mv78230-a0-i2c in this dts
>
> I would prefer the option 1 but I fear that Arnd would prefer the 2 other options.
I like option 1 and 3. Option 3 is a *correct* description of the
hardware, and should be done. Option 1 makes Linux user-friendly for boards
that are exactly the same, but changed out SoC stepping mid-production.
Option 2 needs to be undone. We shouldn't need to change compiled code
for every new board that comes along. Which was the whole point of DT,
right?
thx,
Jason.
> > Well about the issue, we patch the device tree for the i2c quirk only for
> > the openblock AX3.
>
> Ahhh, that's right. I need to slow down and dig a bit more. :(
>
> > If I remember well it was because the device tree was already written
> > for this board before we found there was an issue with the A0 version
> > of the CPU.
>
> No, it's because we didn't think any manufacturers would be silly enough
> to use the a0 in production. That assumption worked out well. :-P
>
> > The rule was that for new boards then they have to set the marvell,mv78230-a0-i2c
> > compatible string. I also didn't expect that we found "new" product using the A0 version.
> >
> > We have 3 options now:
> >
> > - remove the check on the openblock AX3 board and always try to apply the quirck for A0 version
> > - add a check for this new board in the mvebu_dt_init function
> > - let the compatible string marvell,mv78230-a0-i2c in this dts
> >
> > I would prefer the option 1 but I fear that Arnd would prefer the 2 other options.
>
> I like option 1 and 3. Option 3 is a *correct* description of the
> hardware, and should be done. Option 1 makes Linux user-friendly for boards
> that are exactly the same, but changed out SoC stepping mid-production.
I would prefer 1 as well. It is the SoC that has the problem, not the
board. So we should not be making the quirk board specific.
Andrew
Benoit,
Please try to avoid top-posting. I've re-flowed it, below.
Arnd, Andrew, question for both of you below.
On Thu, Jul 24, 2014 at 03:40:42PM +0200, Benoit Masson wrote:
> Le 24 juil. 2014 ? 14:45, Jason Cooper <[email protected]> a ?crit :
>
> > On Thu, Jul 24, 2014 at 02:21:03PM +0200, Gregory CLEMENT wrote:
> >> On 24/07/2014 01:15, Jason Cooper wrote:
> >>> On Thu, Jul 24, 2014 at 01:11:12AM +0200, Benoit Masson wrote:
> >>>> Le 24 juil. 2014 ? 00:58, Andrew Lunn <[email protected]> a ?crit :
> >>>>
> >>>>>> For the marvell,mv78230-a0-i2c unfortunately i've spend 3 hours
> >>>>>> trying to understand this but it only works with this on the
> >>>>>> ix4-300d :(. There was multiple patch around this and maybe one
> >>>>>> broke the auto-detect part of this, I've tried compiling with some
> >>>>>> 3.10 or lower kernel but no luck here I still have to put this a0
> >>>>>> option.
> >>>>>
> >>>>> Lets first confirm you have an a0 SoC.
> >>>>>
> >>>>> At boot time, it should print:
> >>>>>
> >>>>> pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);
> >>>>>
> >>>>> What revision do you have?
> >>>>>
> >>>>> If the auto detect code really is broken, Gregory will likely take a
> >>>>> look.
> >>>>
> >>>> I sure do,
> >>>>
> >>>> confirmed by u-boot output below:
> >>>>
> >>>> U-Boot 2009.08 (Mar 04 2013 - 11:13:04) Marvell version: 2.3.2 PQ
> >>>> U-Boot Addressing:
> >>>> Code:..00600000:006BFFF0
> >>>> BSS:..00708EC0
> >>>> Stack:..0x5fff70
> >>>> PageTable:.0x8e0000
> >>>> Heap address:.0x900000:0xe00000
> >>>> Board: DB-78230-BP rev 2.0 Wistron
> >>>> SoC: MV78230 A0
> >>>>
> >>>> From kernel I get:
> >>>>
> >>>> mvebu-soc-id: MVEBU SoC ID=0x7823, Rev=0x1
> >>>
> >>> Well, isn't that a peach? :) Gregory?
> >>
> >> A peach?? For me it is either a fruit or a princess, so I am puzzled!
> >
> > Doc Holiday quote from the movie Tombstone. The full quote was "Well,
> > isn't that a peach of a hand?" while sitting at the poker table.
> >
> >> Well about the issue, we patch the device tree for the i2c quirk only for
> >> the openblock AX3.
> >
> > Ahhh, that's right. I need to slow down and dig a bit more. :(
^^^^^ This is the important bit.
> >
> >> If I remember well it was because the device tree was already written
> >> for this board before we found there was an issue with the A0 version
> >> of the CPU.
> >
> > No, it's because we didn't think any manufacturers would be silly enough
> > to use the a0 in production. That assumption worked out well. :-P
> >
> >> The rule was that for new boards then they have to set the marvell,mv78230-a0-i2c
> >> compatible string. I also didn't expect that we found "new" product using the A0 version.
> >>
> >> We have 3 options now:
> >>
> >> - remove the check on the openblock AX3 board and always try to apply the quirck for A0 version
> >> - add a check for this new board in the mvebu_dt_init function
> >> - let the compatible string marvell,mv78230-a0-i2c in this dts
> >>
> >> I would prefer the option 1 but I fear that Arnd would prefer the 2 other options.
> >
> > I like option 1 and 3. Option 3 is a *correct* description of the
> > hardware, and should be done. Option 1 makes Linux user-friendly for boards
> > that are exactly the same, but changed out SoC stepping mid-production.
> >
> > Option 2 needs to be undone. We shouldn't need to change compiled code
> > for every new board that comes along. Which was the whole point of DT,
> > right?
> >
> So I'm a bit puzzled now. First email from Jason mention that the
> final patch was doe in 3.16rc3 and indeed I made my dts using 3.15,
> now I'm on 3.16 mainline, but didn't tested since to remove the a0
> compatible string. Should I give it a try or from Gregory's email
> understand that the patch was only for ax3 board ?
Ignore what I said before about the patch. It doesn't apply to your
board. We need to fix the quirk to trigger on SoC revision, not board
compatible, as it currently does.
The only outstanding point (Arnd?), is that I think it's ok to have the
i2c...a0 compatible string in the dts files, but Andrew seems to think
otherwise. Is that still true Andrew?
> Feel free as long as the device is still opened, uart connected, to
> send me your test as I'll soon have to turn into a production NAS ...
Hopefully, we'll hear from one of them today, and we can merge your
patch as-is with the minor comment reformatting.
thx,
Jason.
> The only outstanding point (Arnd?), is that I think it's ok to have the
> i2c...a0 compatible string in the dts files, but Andrew seems to think
> otherwise. Is that still true Andrew?
Hi Jason
I can live with i2c...a0 compatible string, but it has minor problems:
1) The binding Documentation says not to do it. So we are ignoring our
own documentation.
2) It seems likely that at some point the OEM will swap to B1 revision
SoCs. The i2c device then does not require this quirk, but we have
hard coded in the DT file that it is required. B1 revision would
work, but not optimally.
So i would prefer not to explicitly enable the quirk, but determine at
run time if the quirk is needed for the SoC revision it is running on.
Andrew
On Thu, Jul 24, 2014 at 04:29:18PM +0200, Andrew Lunn wrote:
> > The only outstanding point (Arnd?), is that I think it's ok to have the
> > i2c...a0 compatible string in the dts files, but Andrew seems to think
> > otherwise. Is that still true Andrew?
>
> Hi Jason
>
> I can live with i2c...a0 compatible string, but it has minor problems:
>
> 1) The binding Documentation says not to do it. So we are ignoring our
> own documentation.
This can be updated.
> 2) It seems likely that at some point the OEM will swap to B1 revision
> SoCs. The i2c device then does not require this quirk, but we have
> hard coded in the DT file that it is required. B1 revision would
> work, but not optimally.
The quirk can go both ways. eg, we can detect that we *aren't* on an A0
and need to remove the compatible string.
> So i would prefer not to explicitly enable the quirk, but determine at
> run time if the quirk is needed for the SoC revision it is running on.
I agree, but that is Linux-centric. We need to handle it coherently in
the binding docs for *BSD, bootloaders, etc.
I suggest we update the binding to allow using the compatible string,
and advise that to avoid end-user frustration, implementations should
detect the SoC revision at runtime and either add or remove the
compatible string.
thx,
Jason.