2022-09-22 21:39:43

by Mauri Sandberg

[permalink] [raw]
Subject: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

Add D-Link DNS-323 that is based on Device Tree.

Signed-off-by: Mauri Sandberg <[email protected]>
---
changes from v1
- split patches, this one modifies source code
- add DT based dns323 board file
- don't remove any existing code
---
arch/arm/mach-orion5x/Kconfig | 7 +
arch/arm/mach-orion5x/Makefile | 1 +
arch/arm/mach-orion5x/board-dns323.c | 208 +++++++++++++++++++++++++++
arch/arm/mach-orion5x/board-dt.c | 3 +
arch/arm/mach-orion5x/common.h | 6 +
5 files changed, 225 insertions(+)
create mode 100644 arch/arm/mach-orion5x/board-dns323.c

diff --git a/arch/arm/mach-orion5x/Kconfig b/arch/arm/mach-orion5x/Kconfig
index 0044b2823710..1ee0d7e06828 100644
--- a/arch/arm/mach-orion5x/Kconfig
+++ b/arch/arm/mach-orion5x/Kconfig
@@ -68,6 +68,13 @@ config MACH_DNS323
Say 'Y' here if you want your kernel to support the
D-Link DNS-323 platform.

+config MACH_DNS323_DT
+ bool "D-Link DNS-323 (Flattened Device Tree)"
+ select ARCH_ORION5X_DT
+ help
+ Say 'Y' here if you want your kernel to support the
+ D-Link DNS-323 platform.
+
config MACH_TS209
bool "QNAP TS-109/TS-209"
depends on ATAGS
diff --git a/arch/arm/mach-orion5x/Makefile b/arch/arm/mach-orion5x/Makefile
index 1a585a62d5e6..2ed6bafa7acb 100644
--- a/arch/arm/mach-orion5x/Makefile
+++ b/arch/arm/mach-orion5x/Makefile
@@ -22,5 +22,6 @@ obj-$(CONFIG_MACH_RD88F6183AP_GE) += rd88f6183ap-ge-setup.o

obj-$(CONFIG_ARCH_ORION5X_DT) += board-dt.o
obj-$(CONFIG_MACH_D2NET_DT) += board-d2net.o
+obj-$(CONFIG_MACH_DNS323_DT) += board-dns323.o
obj-$(CONFIG_MACH_MSS2_DT) += board-mss2.o
obj-$(CONFIG_MACH_RD88F5182_DT) += board-rd88f5182.o
diff --git a/arch/arm/mach-orion5x/board-dns323.c b/arch/arm/mach-orion5x/board-dns323.c
new file mode 100644
index 000000000000..72a1f3e228b3
--- /dev/null
+++ b/arch/arm/mach-orion5x/board-dns323.c
@@ -0,0 +1,208 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Mauri Sandberg <[email protected]>
+ *
+ * Flattened Device Tree board initialization
+ *
+ * This is adapted from existing mach files and most of the source code is
+ * originally written by:
+ * Copyright (C) 2007 Herbert Valerio Riedel <[email protected]>
+ * Copyright (C) 2010 Benjamin Herrenschmidt <[email protected]>
+ * Copyright 2012 (C), Jason Cooper <[email protected]>
+ */
+
+#include <linux/of.h>
+#include <linux/phy.h>
+#include <linux/marvell_phy.h>
+#include <linux/of_net.h>
+#include <linux/clk.h>
+#include "bridge-regs.h"
+
+/* Exposed to userspace, do not change */
+enum {
+ DNS323_REV_A1, /* 0 */
+ DNS323_REV_B1, /* 1 */
+ DNS323_REV_C1, /* 2 */
+};
+
+/****************************************************************************
+ * Fix-ups
+ */
+
+static int dns323c_phy_fixup(struct phy_device *phy)
+{
+ phy->dev_flags |= MARVELL_PHY_M1118_DNS323_LEDS;
+
+ return 0;
+}
+
+/****************************************************************************
+ * Ethernet
+ */
+
+/* dns323_parse_hex_*() taken from tsx09-common.c; should a common copy of these
+ * functions be kept somewhere?
+ */
+static int __init dns323_parse_hex_nibble(char n)
+{
+ if (n >= '0' && n <= '9')
+ return n - '0';
+
+ if (n >= 'A' && n <= 'F')
+ return n - 'A' + 10;
+
+ if (n >= 'a' && n <= 'f')
+ return n - 'a' + 10;
+
+ return -1;
+}
+
+static int __init dns323_parse_hex_byte(const char *b)
+{
+ int hi;
+ int lo;
+
+ hi = dns323_parse_hex_nibble(b[0]);
+ lo = dns323_parse_hex_nibble(b[1]);
+
+ if (hi < 0 || lo < 0)
+ return -1;
+
+ return (hi << 4) | lo;
+}
+
+#define DNS323_NOR_BOOT_BASE 0xf4000000
+
+static int __init dns323_read_mac_addr(u8 *addr)
+{
+ int i;
+ char *mac_page;
+
+ /* MAC address is stored as a regular ol' string in /dev/mtdblock4
+ * (0x007d0000-0x00800000) starting at offset 196480 (0x2ff80).
+ */
+ mac_page = ioremap(DNS323_NOR_BOOT_BASE + 0x7d0000 + 196480, 1024);
+ if (!mac_page)
+ return -ENOMEM;
+
+ /* Sanity check the string we're looking at */
+ for (i = 0; i < 5; i++) {
+ if (*(mac_page + (i * 3) + 2) != ':')
+ goto error_fail;
+ }
+
+ for (i = 0; i < ETH_ALEN; i++) {
+ int byte;
+
+ byte = dns323_parse_hex_byte(mac_page + (i * 3));
+ if (byte < 0)
+ goto error_fail;
+
+ addr[i] = byte;
+ }
+
+ iounmap(mac_page);
+
+ return 0;
+
+error_fail:
+ iounmap(mac_page);
+ return -EINVAL;
+}
+
+static void __init dns323_dt_eth_fixup(void)
+{
+ struct device_node *np;
+ u8 addr[ETH_ALEN];
+ int ret;
+
+ /*
+ * The ethernet interfaces forget the MAC address assigned by u-boot
+ * if the clocks are turned off. Usually, u-boot on orion boards
+ * has no DT support to properly set local-mac-address property.
+ * As a workaround, we get the MAC address that is stored in flash
+ * and update the port device node if no valid MAC address is set.
+ */
+ ret = dns323_read_mac_addr(addr);
+
+ if (ret) {
+ pr_warn("Unable to find MAC address in flash memory\n");
+ return;
+ }
+
+ np = of_find_compatible_node(NULL, NULL, "marvell,orion-eth-port");
+
+ if (!IS_ERR(np)) {
+ struct device_node *pnp = of_get_parent(np);
+ struct clk *clk;
+ struct property *pmac;
+ u8 tmpmac[ETH_ALEN];
+ u8 *macaddr;
+ int i;
+
+ if (!pnp)
+ return;
+
+ /* skip disabled nodes or nodes with valid MAC address*/
+ if (!of_device_is_available(pnp) ||
+ !of_get_mac_address(np, tmpmac))
+ goto eth_fixup_skip;
+
+ clk = of_clk_get(pnp, 0);
+ if (IS_ERR(clk))
+ goto eth_fixup_skip;
+
+ /* ensure port clock is not gated to not hang CPU */
+ clk_prepare_enable(clk);
+
+ /* store MAC address register contents in local-mac-address */
+ pmac = kzalloc(sizeof(*pmac) + 6, GFP_KERNEL);
+ if (!pmac)
+ goto eth_fixup_no_mem;
+
+ pmac->value = pmac + 1;
+ pmac->length = ETH_ALEN;
+ pmac->name = kstrdup("local-mac-address", GFP_KERNEL);
+ if (!pmac->name) {
+ kfree(pmac);
+ goto eth_fixup_no_mem;
+ }
+
+ macaddr = pmac->value;
+ for (i = 0; i < ETH_ALEN; i++)
+ macaddr[i] = addr[i];
+
+ of_update_property(np, pmac);
+
+eth_fixup_no_mem:
+ clk_disable_unprepare(clk);
+ clk_put(clk);
+eth_fixup_skip:
+ of_node_put(pnp);
+ }
+}
+
+void __init dns323_init_dt(void)
+{
+ if (of_machine_is_compatible("dlink,dns323a1")) {
+ writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */
+ } else if (of_machine_is_compatible("dlink,dns323c1") &&
+ IS_BUILTIN(CONFIG_PHYLIB)) {
+ /* Register fixup for the PHY LEDs */
+ phy_register_fixup_for_uid(MARVELL_PHY_ID_88E1118,
+ MARVELL_PHY_ID_MASK,
+ dns323c_phy_fixup);
+
+ /* Now, -this- should theorically be done by the sata_mv driver
+ * once I figure out what's going on there. Maybe the behaviour
+ * of the LEDs should be somewhat passed via the platform_data.
+ * for now, just whack the register and make the LEDs happy
+ *
+ * Note: AFAIK, rev B1 needs the same treatement but I'll let
+ * somebody else test it.
+ */
+ writel(0x5, ORION5X_SATA_VIRT_BASE + 0x2c);
+ }
+
+ dns323_dt_eth_fixup();
+}
diff --git a/arch/arm/mach-orion5x/board-dt.c b/arch/arm/mach-orion5x/board-dt.c
index e3736ffc8347..670bff5e53f6 100644
--- a/arch/arm/mach-orion5x/board-dt.c
+++ b/arch/arm/mach-orion5x/board-dt.c
@@ -57,6 +57,9 @@ static void __init orion5x_dt_init(void)
cpu_idle_poll_ctrl(true);
}

+ if (of_machine_is_compatible("dlink,dns323"))
+ dns323_init_dt();
+
if (of_machine_is_compatible("maxtor,shared-storage-2"))
mss2_init();

diff --git a/arch/arm/mach-orion5x/common.h b/arch/arm/mach-orion5x/common.h
index eb96009e21c4..7a21f7216c65 100644
--- a/arch/arm/mach-orion5x/common.h
+++ b/arch/arm/mach-orion5x/common.h
@@ -75,6 +75,12 @@ extern void mss2_init(void);
static inline void mss2_init(void) {}
#endif

+#ifdef CONFIG_MACH_DNS323_DT
+extern void dns323_init_dt(void);
+#else
+static inline void dns323_init_dt(void) {}
+#endif
+
/*****************************************************************************
* Helpers to access Orion registers
****************************************************************************/
--
2.25.1


2022-09-22 21:53:11

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

On Thu, Sep 22, 2022, at 10:24 PM, Mauri Sandberg wrote:
> +
> +/* dns323_parse_hex_*() taken from tsx09-common.c; should a common
> copy of these
> + * functions be kept somewhere?
> + */
> +static int __init dns323_parse_hex_nibble(char n)
> +{
> + if (n >= '0' && n <= '9')
> + return n - '0';
> +
> + if (n >= 'A' && n <= 'F')
> + return n - 'A' + 10;
> +
> + if (n >= 'a' && n <= 'f')
> + return n - 'a' + 10;
> +
> + return -1;
> +}
> +
> +static int __init dns323_parse_hex_byte(const char *b)
> +{
> + int hi;
> + int lo;
> +
> + hi = dns323_parse_hex_nibble(b[0]);
> + lo = dns323_parse_hex_nibble(b[1]);
> +
> + if (hi < 0 || lo < 0)
> + return -1;
> +
> + return (hi << 4) | lo;
> +}
> +

Can you use simple_strntoull() to parse the address into a u64 instead?

> +static int __init dns323_read_mac_addr(u8 *addr)
> +{
> + int i;
> + char *mac_page;
> +
> + /* MAC address is stored as a regular ol' string in /dev/mtdblock4
> + * (0x007d0000-0x00800000) starting at offset 196480 (0x2ff80).
> + */
> + mac_page = ioremap(DNS323_NOR_BOOT_BASE + 0x7d0000 + 196480, 1024);
> + if (!mac_page)
> + return -ENOMEM;

It would be nicer to use of_iomap() on the nor device than a
hardcoded physical address here, at least if that doesn't add
too much extra complexity.

Arnd

2022-09-22 22:03:55

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

> +static void __init dns323_dt_eth_fixup(void)
> +{
> + struct device_node *np;
> + u8 addr[ETH_ALEN];
> + int ret;
> +
> + /*
> + * The ethernet interfaces forget the MAC address assigned by u-boot
> + * if the clocks are turned off. Usually, u-boot on orion boards
> + * has no DT support to properly set local-mac-address property.
> + * As a workaround, we get the MAC address that is stored in flash
> + * and update the port device node if no valid MAC address is set.
> + */

This is true for Kirkwood, but orion5x does not have any clocks to
gate. So i'm pretty sure this is not true. You copied this code for a
different reason. Please document here the real reason for this code.

> + ret = dns323_read_mac_addr(addr);
> +
> + if (ret) {
> + pr_warn("Unable to find MAC address in flash memory\n");
> + return;
> + }
> +
> + np = of_find_compatible_node(NULL, NULL, "marvell,orion-eth-port");
> +
> + if (!IS_ERR(np)) {
> + struct device_node *pnp = of_get_parent(np);
> + struct clk *clk;
> + struct property *pmac;
> + u8 tmpmac[ETH_ALEN];
> + u8 *macaddr;
> + int i;
> +
> + if (!pnp)
> + return;
> +
> + /* skip disabled nodes or nodes with valid MAC address*/
> + if (!of_device_is_available(pnp) ||
> + !of_get_mac_address(np, tmpmac))
> + goto eth_fixup_skip;
> +
> + clk = of_clk_get(pnp, 0);
> + if (IS_ERR(clk))
> + goto eth_fixup_skip;
> +
> + /* ensure port clock is not gated to not hang CPU */
> + clk_prepare_enable(clk);

I'm pretty sure this clock stuff is not needed. Please comment it out
and see if the machine locks up. Kirkwood just stops dead if you
access registers when there clocks are disabled. For Orion5x, the
ethernet should always have a clock.

> +
> + /* store MAC address register contents in local-mac-address */
> + pmac = kzalloc(sizeof(*pmac) + 6, GFP_KERNEL);
> + if (!pmac)
> + goto eth_fixup_no_mem;
> +
> + pmac->value = pmac + 1;
> + pmac->length = ETH_ALEN;
> + pmac->name = kstrdup("local-mac-address", GFP_KERNEL);
> + if (!pmac->name) {
> + kfree(pmac);
> + goto eth_fixup_no_mem;
> + }
> +
> + macaddr = pmac->value;
> + for (i = 0; i < ETH_ALEN; i++)
> + macaddr[i] = addr[i];
> +
> + of_update_property(np, pmac);
> +
> +eth_fixup_no_mem:
> + clk_disable_unprepare(clk);
> + clk_put(clk);
> +eth_fixup_skip:
> + of_node_put(pnp);
> + }
> +}
> +
> +void __init dns323_init_dt(void)
> +{
> + if (of_machine_is_compatible("dlink,dns323a1")) {
> + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */

I spotted this in dns323-setup.c as well. Do you have any idea what it
does?

Andrew

2022-09-23 09:10:46

by Mauri Sandberg

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

On 23.9.2022 00:10, Andrew Lunn wrote:
>> +static void __init dns323_dt_eth_fixup(void)
>> +{
>> + struct device_node *np;
>> + u8 addr[ETH_ALEN];
>> + int ret;
>> +
>> + /*
>> + * The ethernet interfaces forget the MAC address assigned by u-boot
>> + * if the clocks are turned off. Usually, u-boot on orion boards
>> + * has no DT support to properly set local-mac-address property.
>> + * As a workaround, we get the MAC address that is stored in flash
>> + * and update the port device node if no valid MAC address is set.
>> + */
>
> This is true for Kirkwood, but orion5x does not have any clocks to
> gate. So i'm pretty sure this is not true. You copied this code for a
> different reason. Please document here the real reason for this code.
>

Yes, will do. To my understanding it looks like uboot does not pass
anything
to the kernel.

>> + ret = dns323_read_mac_addr(addr);
>> +
>> + if (ret) {
>> + pr_warn("Unable to find MAC address in flash memory\n");
>> + return;
>> + }
>> +
>> + np = of_find_compatible_node(NULL, NULL, "marvell,orion-eth-port");
>> +
>> + if (!IS_ERR(np)) {
>> + struct device_node *pnp = of_get_parent(np);
>> + struct clk *clk;
>> + struct property *pmac;
>> + u8 tmpmac[ETH_ALEN];
>> + u8 *macaddr;
>> + int i;
>> +
>> + if (!pnp)
>> + return;
>> +
>> + /* skip disabled nodes or nodes with valid MAC address*/
>> + if (!of_device_is_available(pnp) ||
>> + !of_get_mac_address(np, tmpmac))
>> + goto eth_fixup_skip;
>> +
>> + clk = of_clk_get(pnp, 0);
>> + if (IS_ERR(clk))
>> + goto eth_fixup_skip;
>> +
>> + /* ensure port clock is not gated to not hang CPU */
>> + clk_prepare_enable(clk);
>
> I'm pretty sure this clock stuff is not needed. Please comment it out
> and see if the machine locks up. Kirkwood just stops dead if you
> access registers when there clocks are disabled. For Orion5x, the
> ethernet should always have a clock.
>

Will do.

>> +
>> + /* store MAC address register contents in local-mac-address */
>> + pmac = kzalloc(sizeof(*pmac) + 6, GFP_KERNEL);
>> + if (!pmac)
>> + goto eth_fixup_no_mem;
>> +
>> + pmac->value = pmac + 1;
>> + pmac->length = ETH_ALEN;
>> + pmac->name = kstrdup("local-mac-address", GFP_KERNEL);
>> + if (!pmac->name) {
>> + kfree(pmac);
>> + goto eth_fixup_no_mem;
>> + }
>> +
>> + macaddr = pmac->value;
>> + for (i = 0; i < ETH_ALEN; i++)
>> + macaddr[i] = addr[i];
>> +
>> + of_update_property(np, pmac);
>> +
>> +eth_fixup_no_mem:
>> + clk_disable_unprepare(clk);
>> + clk_put(clk);
>> +eth_fixup_skip:
>> + of_node_put(pnp);
>> + }
>> +}
>> +
>> +void __init dns323_init_dt(void)
>> +{
>> + if (of_machine_is_compatible("dlink,dns323a1")) {
>> + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */
>
> I spotted this in dns323-setup.c as well. Do you have any idea what it
> does?
>

No idea. I have tried to replicate what was in dns323-setup.c as exactly
as possible.
I can try to leave it out and see if anything changes.

2022-09-23 10:00:04

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

On Fri, Sep 23, 2022, at 11:13 AM, [email protected] wrote:
> On 23.9.2022 00:39, Arnd Bergmann wrote:
>> Can you use simple_strntoull() to parse the address into a u64 instead?
>>
> Nice idea. Its current replacement seems to be kstrtoull().

Right.

> I'll have to do
> it byte by byte, right? Or what do you have in mind with 64bit unsigned?

I misread your code and assumed it was stored as a 12-digit
hexadecimal number, but you are right: with the colons you still
need to iterate the bytes.

I looked a little further and found the mac_pton() function
that I think does exactly what you need.

Arnd

2022-09-23 10:01:26

by Mauri Sandberg

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

On 23.9.2022 00:39, Arnd Bergmann wrote:
> On Thu, Sep 22, 2022, at 10:24 PM, Mauri Sandberg wrote:
>> +
>> +/* dns323_parse_hex_*() taken from tsx09-common.c; should a common
>> copy of these
>> + * functions be kept somewhere?
>> + */
>> +static int __init dns323_parse_hex_nibble(char n)
>> +{
>> + if (n >= '0' && n <= '9')
>> + return n - '0';
>> +
>> + if (n >= 'A' && n <= 'F')
>> + return n - 'A' + 10;
>> +
>> + if (n >= 'a' && n <= 'f')
>> + return n - 'a' + 10;
>> +
>> + return -1;
>> +}
>> +
>> +static int __init dns323_parse_hex_byte(const char *b)
>> +{
>> + int hi;
>> + int lo;
>> +
>> + hi = dns323_parse_hex_nibble(b[0]);
>> + lo = dns323_parse_hex_nibble(b[1]);
>> +
>> + if (hi < 0 || lo < 0)
>> + return -1;
>> +
>> + return (hi << 4) | lo;
>> +}
>> +
>
> Can you use simple_strntoull() to parse the address into a u64 instead?
>
Nice idea. Its current replacement seems to be kstrtoull(). I'll have to
do
it byte by byte, right? Or what do you have in mind with 64bit unsigned?

>> +static int __init dns323_read_mac_addr(u8 *addr)
>> +{
>> + int i;
>> + char *mac_page;
>> +
>> + /* MAC address is stored as a regular ol' string in /dev/mtdblock4
>> + * (0x007d0000-0x00800000) starting at offset 196480 (0x2ff80).
>> + */
>> + mac_page = ioremap(DNS323_NOR_BOOT_BASE + 0x7d0000 + 196480, 1024);
>> + if (!mac_page)
>> + return -ENOMEM;
>
> It would be nicer to use of_iomap() on the nor device than a
> hardcoded physical address here, at least if that doesn't add
> too much extra complexity.
>

I'll look into this.

2022-09-23 12:35:24

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

> > > + if (of_machine_is_compatible("dlink,dns323a1")) {
> > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */
> >
> > I spotted this in dns323-setup.c as well. Do you have any idea what it
> > does?
> >
>
> No idea. I have tried to replicate what was in dns323-setup.c as exactly as
> possible.
> I can try to leave it out and see if anything changes.

It is best to keep what we don't understand. It will be there for a
reason.

Andrew

2022-09-23 18:43:40

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

On Friday 23 September 2022 14:12:24 Andrew Lunn wrote:
> > > > + if (of_machine_is_compatible("dlink,dns323a1")) {
> > > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */
> > >
> > > I spotted this in dns323-setup.c as well. Do you have any idea what it
> > > does?
> > >
> >
> > No idea. I have tried to replicate what was in dns323-setup.c as exactly as
> > possible.
> > I can try to leave it out and see if anything changes.
>
> It is best to keep what we don't understand. It will be there for a
> reason.
>
> Andrew

Hello! I tried to index all publicly available Marvell SoC
documentations into kernel documentation subfolder:
https://docs.kernel.org/arm/marvell.html

For Orion there is linked Datasheet and User Manual, so you could try to
find in those documents that mentioned register and check what it is
doing.

2022-09-26 14:15:43

by Mauri Sandberg

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

On 23.9.2022 21:02, Pali Rohár wrote:
> On Friday 23 September 2022 14:12:24 Andrew Lunn wrote:
>> > > > + if (of_machine_is_compatible("dlink,dns323a1")) {
>> > > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */
>> > >
>> > > I spotted this in dns323-setup.c as well. Do you have any idea what it
>> > > does?
>> > >
>> >
>> > No idea. I have tried to replicate what was in dns323-setup.c as exactly as
>> > possible.
>> > I can try to leave it out and see if anything changes.
>>
>> It is best to keep what we don't understand. It will be there for a
>> reason.
>>
>> Andrew
>
> Hello! I tried to index all publicly available Marvell SoC
> documentations into kernel documentation subfolder:
> https://docs.kernel.org/arm/marvell.html
>
> For Orion there is linked Datasheet and User Manual, so you could try
> to
> find in those documents that mentioned register and check what it is
> doing.

MPP_DEV_CTRL refers to register at address 0x10008. According to the
88F5152 user manual it's
'Device Multiplex Control Register' Offset: 0x10008.

Bits Field Type/InitVal Description
[31:0] Reserved RES 0x03FF0000 Reserved. NOTE: Must be 0x03FF0000'.

DEV_D[31:16] receives no hits in the documentation, only to DEV_D[15:0]
are referred.

Maybe 88F5151 is different, hard to say.

2022-09-26 14:48:18

by Pali Rohár

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

Hello Elad! I hope that would not bothering you. We are doing here
cleanup of kernel code for older Marvell SoCs (Orion) and there one
unknown thing about 88F5181's 0x10008 register. See below.

On Monday 26 September 2022 14:56:48 [email protected] wrote:
> On 23.9.2022 21:02, Pali Rohár wrote:
> > On Friday 23 September 2022 14:12:24 Andrew Lunn wrote:
> > > > > > + if (of_machine_is_compatible("dlink,dns323a1")) {
> > > > > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */
> > > > >
> > > > > I spotted this in dns323-setup.c as well. Do you have any idea what it
> > > > > does?
> > > > >
> > > >
> > > > No idea. I have tried to replicate what was in dns323-setup.c as exactly as
> > > > possible.
> > > > I can try to leave it out and see if anything changes.
> > >
> > > It is best to keep what we don't understand. It will be there for a
> > > reason.
> > >
> > > Andrew
> >
> > Hello! I tried to index all publicly available Marvell SoC
> > documentations into kernel documentation subfolder:
> > https://docs.kernel.org/arm/marvell.html
> >
> > For Orion there is linked Datasheet and User Manual, so you could try to
> > find in those documents that mentioned register and check what it is
> > doing.
>
> MPP_DEV_CTRL refers to register at address 0x10008. According to the 88F5152
> user manual it's
> 'Device Multiplex Control Register' Offset: 0x10008.
>
> Bits Field Type/InitVal Description
> [31:0] Reserved RES 0x03FF0000 Reserved. NOTE: Must be 0x03FF0000'.
>
> DEV_D[31:16] receives no hits in the documentation, only to DEV_D[15:0] are
> referred.

In linked public document I found same thing. Register is for 88F5182
reserved. (You have typo in comment, it is 88F5182, not *52).

> Maybe 88F5151 is different, hard to say.

I have feeling that for 88F5181 it is not reserved and has to be
configured correctly. (Also typo in your comment, it is 88F5181, not *51).
But I have not found any copy of 88F5181 user manual document on internet.

In past 88F518x and 88F528x documents and user manuals were available
publicly on Marvell website, visible from web archive:
https://web.archive.org/web/20080607215437/http://www.marvell.com/products/media/index.jsp

But Marvell deleted these documents from their public website and for
kernel developers they are now probably lost. I do not know about any
other backups.


Elad, could you please help us? Do you have access to functional
specifications / user manuals for 88F518x and 88F528x or have
information how kernel developers can get access to those documents?
Hopefully they were not totally lost. We just need explanation for
register 'Device Multiplex Control Register' Offset: 0x10008.

2022-09-28 14:39:16

by Elad Nachman

[permalink] [raw]
Subject: RE: [EXT] Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

Hi Pali,

I do not have documentation for this controller, as it is almost 20 years old...

I did manage, however, to find some very old u-boot code for it.

From reverse engineering this u-boot code, it looks like this is a "DEV" MPP function register, similar to the MPP0_7, MPP8_15 and the MPP16_23 registers.

Basically, setting bits of this registers assign the pin to the special purpose, while clearing it makes it a GPP (General Purpose Pin).

For all of the boards (over half a dozen) support by this u-boot, this register is set to zero (see above).
From user guides I have found for few of these boards, only MPPs up to MPP19 are used, hence it make sense to leave these MPPs as GPPs .

Hopefully this helps in some way.

FYI,

Elad.


-----Original Message-----
From: Pali Rohár <[email protected]>
Sent: Monday, September 26, 2022 3:23 PM
To: Elad Nachman <[email protected]>
Cc: [email protected]; Andrew Lunn <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [EXT] Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

External Email

----------------------------------------------------------------------
Hello Elad! I hope that would not bothering you. We are doing here cleanup of kernel code for older Marvell SoCs (Orion) and there one unknown thing about 88F5181's 0x10008 register. See below.

On Monday 26 September 2022 14:56:48 [email protected] wrote:
> On 23.9.2022 21:02, Pali Rohár wrote:
> > On Friday 23 September 2022 14:12:24 Andrew Lunn wrote:
> > > > > > + if (of_machine_is_compatible("dlink,dns323a1")) {
> > > > > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */
> > > > >
> > > > > I spotted this in dns323-setup.c as well. Do you have any idea
> > > > > what it does?
> > > > >
> > > >
> > > > No idea. I have tried to replicate what was in dns323-setup.c as
> > > > exactly as possible.
> > > > I can try to leave it out and see if anything changes.
> > >
> > > It is best to keep what we don't understand. It will be there for
> > > a reason.
> > >
> > > Andrew
> >
> > Hello! I tried to index all publicly available Marvell SoC
> > documentations into kernel documentation subfolder:
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.kernel.org
> > _arm_marvell.html&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-TxXc
> > zjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=QnvtICgYrknBcrJ4SBYkL8zUxNtqo3A40bjE
> > TmCHhBbdWQOUaRffkiMgtuRkQ2WE&s=QiNvxcOSpDNOTgiK8nuCZ18pgJRKBtgVu-SeG
> > E9n7CY&e=
> >
> > For Orion there is linked Datasheet and User Manual, so you could
> > try to find in those documents that mentioned register and check
> > what it is doing.
>
> MPP_DEV_CTRL refers to register at address 0x10008. According to the
> 88F5152 user manual it's 'Device Multiplex Control Register' Offset:
> 0x10008.
>
> Bits Field Type/InitVal Description
> [31:0] Reserved RES 0x03FF0000 Reserved. NOTE: Must be 0x03FF0000'.
>
> DEV_D[31:16] receives no hits in the documentation, only to
> DEV_D[15:0] are referred.

In linked public document I found same thing. Register is for 88F5182 reserved. (You have typo in comment, it is 88F5182, not *52).

> Maybe 88F5151 is different, hard to say.

I have feeling that for 88F5181 it is not reserved and has to be configured correctly. (Also typo in your comment, it is 88F5181, not *51).
But I have not found any copy of 88F5181 user manual document on internet.

In past 88F518x and 88F528x documents and user manuals were available publicly on Marvell website, visible from web archive:
https://urldefense.proofpoint.com/v2/url?u=https-3A__web.archive.org_web_20080607215437_http-3A__www.marvell.com_products_media_index.jsp&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-TxXczjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=QnvtICgYrknBcrJ4SBYkL8zUxNtqo3A40bjETmCHhBbdWQOUaRffkiMgtuRkQ2WE&s=k1vn2-NVEU2OsJYVTmuWMRKdN2t1MQ9pduTkGaasU4s&e=

But Marvell deleted these documents from their public website and for kernel developers they are now probably lost. I do not know about any other backups.


Elad, could you please help us? Do you have access to functional specifications / user manuals for 88F518x and 88F528x or have information how kernel developers can get access to those documents?
Hopefully they were not totally lost. We just need explanation for register 'Device Multiplex Control Register' Offset: 0x10008.

2022-09-30 16:10:48

by Pali Rohár

[permalink] [raw]
Subject: Re: [EXT] Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree

Hello Elad! Thank you very much for this information!

With information from other resources, I think I understood it.


Andrew & Mauri: here is my recap:

On https://docs.kernel.org/arm/marvell.html is Datasheet document for
88F5281 SoC (it is different! not 88F5181!) and in its section 3.2
Device Pins Multiplexing it is documented. SoC pins named MPP[0], ...
MPP[19] are configured via registers 0x10000, 0x10004 and 0x10050 as
documented in User Manual. And SoC pins named DEV_D[16], ... DEV_D[31]
are configured via that undocumented register 0x10008.

Normally 32 DEV_D pins on 88F5281 SoC are used for Device Bus Interface
but when SoC has configured only 16-bit or 8-bit Device Bus or it does
not use Device Bus Interface at all then pins DEV_D[16] ... DEV_D[31]
can be used as GPIOs.

Elad wrote that clearing particular bit _i_ in 0x10008 sets DEV_D[i] on
88F5181 to GPIO but datasheet for 88F5281 says that clearing bit i
(value 0x0) sets DEV_D[i] to Device Bus mode.

I have no idea if 88F5281 and 88F5181 have inverted logic or if
documentation has bugs. But at least it this explanation makes sense for
me.

So code "writel(0, MPP_DEV_CTRL);" either changes all DEV_D pins to GPIO
mode or to Device Bus Interface mode.

In most cases Device Bus is used for connecting Parallel NAND or any
similar Flash memory device. Mauri, you can check if your board has
such memory. Or if it uses GPIOs connected on DEV_D pins. If not then it
does not matter how you set that register.

Anyway, just for completeness, the "proper" way for using MPP_DEV_CTRL
should have been in pinctrl/mvebu/pinctrl-orion.c driver. But I think it
does not make sense to spend another time for this old board to convert
this code into proper pinctrl driver.

Hopes that this helps to finally understand that old undocumented mystery.

On Wednesday 28 September 2022 13:32:27 Elad Nachman wrote:
> Hi Pali,
>
> I do not have documentation for this controller, as it is almost 20 years old...
>
> I did manage, however, to find some very old u-boot code for it.
>
> From reverse engineering this u-boot code, it looks like this is a "DEV" MPP function register, similar to the MPP0_7, MPP8_15 and the MPP16_23 registers.
>
> Basically, setting bits of this registers assign the pin to the special purpose, while clearing it makes it a GPP (General Purpose Pin).
>
> For all of the boards (over half a dozen) support by this u-boot, this register is set to zero (see above).
> From user guides I have found for few of these boards, only MPPs up to MPP19 are used, hence it make sense to leave these MPPs as GPPs .
>
> Hopefully this helps in some way.
>
> FYI,
>
> Elad.
>
>
> -----Original Message-----
> From: Pali Rohár <[email protected]>
> Sent: Monday, September 26, 2022 3:23 PM
> To: Elad Nachman <[email protected]>
> Cc: [email protected]; Andrew Lunn <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
> Subject: [EXT] Re: [PATCH v2 3/3] ARM: orion5x: Add D-Link DNS-323 based on Device Tree
>
> External Email
>
> ----------------------------------------------------------------------
> Hello Elad! I hope that would not bothering you. We are doing here cleanup of kernel code for older Marvell SoCs (Orion) and there one unknown thing about 88F5181's 0x10008 register. See below.
>
> On Monday 26 September 2022 14:56:48 [email protected] wrote:
> > On 23.9.2022 21:02, Pali Rohár wrote:
> > > On Friday 23 September 2022 14:12:24 Andrew Lunn wrote:
> > > > > > > + if (of_machine_is_compatible("dlink,dns323a1")) {
> > > > > > > + writel(0, MPP_DEV_CTRL); /* DEV_D[31:16] */
> > > > > >
> > > > > > I spotted this in dns323-setup.c as well. Do you have any idea
> > > > > > what it does?
> > > > > >
> > > > >
> > > > > No idea. I have tried to replicate what was in dns323-setup.c as
> > > > > exactly as possible.
> > > > > I can try to leave it out and see if anything changes.
> > > >
> > > > It is best to keep what we don't understand. It will be there for
> > > > a reason.
> > > >
> > > > Andrew
> > >
> > > Hello! I tried to index all publicly available Marvell SoC
> > > documentations into kernel documentation subfolder:
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__docs.kernel.org
> > > _arm_marvell.html&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-TxXc
> > > zjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=QnvtICgYrknBcrJ4SBYkL8zUxNtqo3A40bjE
> > > TmCHhBbdWQOUaRffkiMgtuRkQ2WE&s=QiNvxcOSpDNOTgiK8nuCZ18pgJRKBtgVu-SeG
> > > E9n7CY&e=
> > >
> > > For Orion there is linked Datasheet and User Manual, so you could
> > > try to find in those documents that mentioned register and check
> > > what it is doing.
> >
> > MPP_DEV_CTRL refers to register at address 0x10008. According to the
> > 88F5152 user manual it's 'Device Multiplex Control Register' Offset:
> > 0x10008.
> >
> > Bits Field Type/InitVal Description
> > [31:0] Reserved RES 0x03FF0000 Reserved. NOTE: Must be 0x03FF0000'.
> >
> > DEV_D[31:16] receives no hits in the documentation, only to
> > DEV_D[15:0] are referred.
>
> In linked public document I found same thing. Register is for 88F5182 reserved. (You have typo in comment, it is 88F5182, not *52).
>
> > Maybe 88F5151 is different, hard to say.
>
> I have feeling that for 88F5181 it is not reserved and has to be configured correctly. (Also typo in your comment, it is 88F5181, not *51).
> But I have not found any copy of 88F5181 user manual document on internet.
>
> In past 88F518x and 88F528x documents and user manuals were available publicly on Marvell website, visible from web archive:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__web.archive.org_web_20080607215437_http-3A__www.marvell.com_products_media_index.jsp&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-TxXczjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=QnvtICgYrknBcrJ4SBYkL8zUxNtqo3A40bjETmCHhBbdWQOUaRffkiMgtuRkQ2WE&s=k1vn2-NVEU2OsJYVTmuWMRKdN2t1MQ9pduTkGaasU4s&e=
>
> But Marvell deleted these documents from their public website and for kernel developers they are now probably lost. I do not know about any other backups.
>
>
> Elad, could you please help us? Do you have access to functional specifications / user manuals for 88F518x and 88F528x or have information how kernel developers can get access to those documents?
> Hopefully they were not totally lost. We just need explanation for register 'Device Multiplex Control Register' Offset: 0x10008.