Re: [PATCH 1/2] ARM: kirkwood: Basic support for DNS-320 and DNS-325

On Mon, Mar 12, 2012 at 05:59:50PM -0000, Jamie Lentin wrote:

On Mon, 12 Mar 2012 03:02:32 -0000, Jason Cooper
[off-list ref] wrote:

quoted

On Sun, Mar 11, 2012 at 02:33:25PM +0000, Jamie Lentin wrote:

quoted

Add support for the DNS-320 and DNS-325. Describe as much as
currently possible
in the devicetree files, leave everything else in board-dt.c to
be patched
later.

Signed-off-by: Jamie Lentin <jm-Pj/HzkgeCk7QXOPxS62xeg@public.gmane.org>

Great work!  comments, below.

quoted

---
arch/arm/boot/dts/kirkwood-dns320.dts |   54 ++++++
arch/arm/boot/dts/kirkwood-dns325.dts |   40 +++++
arch/arm/mach-kirkwood/Kconfig        |   18 ++
arch/arm/mach-kirkwood/Makefile.boot  |    2 +
arch/arm/mach-kirkwood/board-dt.c     |  294
+++++++++++++++++++++++++++++++++
5 files changed, 408 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/boot/dts/kirkwood-dns320.dts
create mode 100644 arch/arm/boot/dts/kirkwood-dns325.dts

diff --git a/arch/arm/boot/dts/kirkwood-dns320.dts

b/arch/arm/boot/dts/kirkwood-dns320.dts
new file mode 100644
index 0000000..c039041

--- /dev/null
+++ b/arch/arm/boot/dts/kirkwood-dns320.dts

@@ -0,0 +1,54 @@
+/dts-v1/;
+
+/include/ "kirkwood.dtsi"
+
+/ {
+	model = "D-Link DNS-320 NAS (Rev A1)";
+	compatible = "dlink,dns-320-a1", "dlink,dns-320",

"dlink,dns-kirkwood", "marvell,kirkwood-88f6281",
"marvell,kirkwood";

s/marvell/mrvl/g for compatible properties.

Okay.

quoted

quoted

+	memory {
+		device_type = "memory";
+		reg = <0x00000000 0x8000000>;
+	};
+
+	chosen {
+		bootargs = "console=ttyS0,115200n8 earlyprintk";
+	};
+
+	wdt@fed20300 {
+		compatible = "mrvl,orion-wdt";
+		reg = <0xfed20300 0x28>;
+		clock-frequency = <166666667>;
+	};

This hasn't made it into my stable branch yet, waiting on common clock
to land (may be 3.5).  Please use kirkwood_wdt_init() in the mean time.

So just to be clear kirkwood_dt_stable on git.infradead.org, as
opposed to any branches on arm-soc?

For now.  I'm still learning the git integration/upstream piece.  Give
me a few more hours and I should have a new version up for review.  It's
a reshuffle and edit of the whole series.

It also redoes serial@ based on Arnd's comments to your patches.

quoted

quoted

+	serial@f1012000 {
+		compatible = "ns16550a";
+		reg = <0xf1012000 0xff>;
+		reg-shift = <2>;
+		interrupts = <33>;
+		clock-frequency = <166666667>;
+	};
+
+	serial@f1012100 {
+		compatible = "ns16550a";
+		reg = <0xf1012100 0xff>;
+		reg-shift = <2>;
+		interrupts = <34>;
+		clock-frequency = <166666667>;
+	};
+
+	sata@f1080000 {
+		compatible = "mrvl,orion-sata";
+		reg = <0xf1080000 0x5000>;
+		interrupts = <21>;
+		nr-ports = <2>;
+	};
+
+	ehci@f1050000 {
+		compatible = "mrvl,orion-ehci";
+		reg = <0xf1050000 0x1000>;
+		interrupts = <19>;
+		dma-mask = <0xffffffff>;
+		phy-version = "";
+	};

sata and ehci both require working dma and interrupt controller.  I
should have the interrupt controller done soon.  mv_cesa will then work.
dma will be after that.

I found this out the hard way, but saw that dma-mask was supported
in staging/kirkwood/dt on arm-soc and managed to get working USB
using it. I'll switch back for now.

Yes, that "support" was a mistake on my part.  It does not appear to be
the correct answer.

quoted

Basically, anything in mach-kirkwood/common.c that passes kirkwood_tclk
is on hold for devicetree.

Including serial ports?

serial is different because it's not a kirkwood/orion-specific driver.
It's clock-frequency is not (as my understanding goes) tied to the
system clock, it just happens to be so in this case.

The other drivers want tclk because they want to know the system clock.

...

quoted

quoted

diff --git a/arch/arm/mach-kirkwood/board-dt.c

b/arch/arm/mach-kirkwood/board-dt.c
index 12dec38..0885713 100644

--- a/arch/arm/mach-kirkwood/board-dt.c
+++ b/arch/arm/mach-kirkwood/board-dt.c

@@ -15,6 +15,7 @@

#include <linux/platform_device.h>
#include <linux/irqdomain.h>
#include <linux/mtd/partitions.h>
+#include <linux/i2c.h>
#include <linux/ata_platform.h>
#include <linux/mv643xx_eth.h>
#include <linux/of.h>

@@ -23,6 +24,9 @@

#include <linux/of_irq.h>
#include <linux/of_platform.h>
#include <linux/gpio.h>
+#include <linux/input.h>
+#include <linux/gpio_keys.h>
+#include <linux/gpio-fan.h>
#include <linux/leds.h>
#include <linux/mtd/physmap.h>
#include <linux/spi/flash.h>

@@ -150,6 +154,291 @@ static void __init dreamplug_init(void)

	platform_device_register(&dreamplug_leds);
}

+/*****************************************************************************
+ * DNS-320 & DNS-325 specifics
+ ****************************************************************************/
+
+static struct mtd_partition dnskw_nand_parts[] = {
+	{
+		.name		= "u-boot",
+		.offset		= 0,
+		.size		= SZ_1M,
+		.mask_flags	= MTD_WRITEABLE
+	}, {
+		.name		= "uImage",
+		.offset		= MTDPART_OFS_NXTBLK,
+		.size		= 5 * SZ_1M
+	}, {
+		.name		= "ramdisk",
+		.offset		= MTDPART_OFS_NXTBLK,
+		.size		= 5 * SZ_1M
+	}, {
+		.name		= "image",
+		.offset		= MTDPART_OFS_NXTBLK,
+		.size		= 102 * SZ_1M
+	}, {
+		.name		= "mini firmware",
+		.offset		= MTDPART_OFS_NXTBLK,
+		.size		= 10 * SZ_1M
+	}, {
+		.name		= "config",
+		.offset		= MTDPART_OFS_NXTBLK,
+		.size		= 5 * SZ_1M
+	},
+};

You should probably squeeze in a partition for the dtb.  ;-)

Is the config partition the u-boot environment?  That usually is much
smaller than what you have alotted.  So, you might be able to split that
to make room for the dtb.

The partition scheme is the same as D-Link's kernel. The config area
is a jffs2 filesystem that contains their config files. It seemed to
me that copying the partition table was the right thing to do to
allow the NAND to be read, even though in practice it's not that
useful---I'm not sure even D-Link pay much attention to it, there's
all sorts of odd offsets in their code.

The other option would be to ignore the nonsense and replace it with
something useful. This would be more convenient for me because it'd
mean I don't have to replace it with my partition scheme :). Any
opinions?

No, reverse-compatible is a good thing if we can swing it.  How about
giving uImage 4MB and then 1MB for the dtb?  Most of my dtbs haven't
even reached 2k...

Although, that'll change device numbering, eg ramdisk = /dev/mtdblock3
where it was /dev/mtdblock2 before.

Is there any unallocated space at the end of the flash?

...

quoted

quoted

+
+	/* Register power off routine */
+	kirkwood_pcie_id(&dev, &rev);
+	if (dev == MV88F6281_DEV_ID) {
+		pr_info("PCI-E Device ID: MV88F6281, configuring power-off");
+		if (gpio_request(36, "dnskw:power:off") == 0 &&
+		    gpio_direction_output(36, 0) == 0)
+			pm_power_off = dnskw_power_off;
+		else
+			pr_err("dnskw: failed to configure power-off GPIO\n");
+	} else {
+		/*
+		 * Dlink code also defines 0x6192, and sets LOW_BASE +
+		 * 0x01000000 high. Either cargo-culted code or another model.
+		 */

I'm just curious, what is cargo-culted?

http://en.wikipedia.org/wiki/Cargo_cult_programming

It's okay, feel free to link me to lmgtfy.  ;-)

In this case, I originally thought that the line was relevant to
supporting the DNS-320 (I assumed it had a MV88F6192 based on it's
lower TCLK), but it seems it uses the MV88F6281 too. So either
D-link have been blindly including this logic or there is a model
somewhere that uses the different SoC.

On reflection, it's probably best to remove this entirely. Testing
the pcie_id isn't achieving anything and is likely to confuse when
it comes to porting the power-off to devicetree.

quoted

quoted

+		pr_err("Unknown PCI-E Device ID %x, no power-off", dev);
+	}
+
+	/*
+	 * Supply power to HDDs. Bootloader should have turned on
sata0 already
+	 */

Don't assume the bootloader does or doesn't do anything.  Put the
peripheral into a known state and work from there.

It doesn't, it turns both HDDs on. I'll clarify the comment though.

              ^^ bootloader or kernel?

Users (especially me) may roll their own u-boot, or swapout for redboot,
whatever.

quoted

quoted

+	dnskw_gpio_register(39, "dnskw:power:sata0", 1);
+	dnskw_gpio_register(40, "dnskw:power:sata1", 1);
+
+	/* Set NAS to turn back on after a power failure */
+	dnskw_gpio_register(37, "dnskw:power:recover", 1);
+}
+

I doubt this board is the only one that wants to be added while the
conversion to devicetree is progressing.  It makes more sense to me to
break the above out to a separate file, eg board-dnskw.c.  I'll follow
and break out the dreamplug code to board-dreamplug.c.  Otherwise, I see
board-dt.c becoming a long mess.

Arnd, sound okay to you?

quoted

static void __init kirkwood_dt_init(void)
{
	struct device_node *node;

@@ -186,11 +475,16 @@ static void __init kirkwood_dt_init(void)

	if (of_machine_is_compatible("globalscale,dreamplug"))
		dreamplug_init();

+	if (of_machine_is_compatible("dlink,dns-kirkwood"))
+		dnskw_init();
+
	of_platform_populate(NULL, kirkwood_dt_match_table, NULL, NULL);
}

static const char *kirkwood_dt_board_compat[] = {
	"globalscale,dreamplug",
+	"dlink,dns-320",
+	"dlink,dns-325",
	NULL
};

--
1.7.9.1

-- 
Jamie Lentin