diff mbox series

[net-next,03/10] net: dsa: mv88e6xxx: move hidden registers operations in own file

Message ID 20190821232724.1544-4-marek.behun@nic.cz
State Changes Requested
Delegated to: David Miller
Headers show
Series net: dsa: mv88e6xxx: Peridot/Topaz SERDES changes | expand

Commit Message

Marek Behún Aug. 21, 2019, 11:27 p.m. UTC 
This patch moves the functions operating on the hidden debug registers
into it's own file, hidden.c.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 drivers/net/dsa/mv88e6xxx/Makefile |  1 +
 drivers/net/dsa/mv88e6xxx/chip.c   | 54 +-----------------------
 drivers/net/dsa/mv88e6xxx/hidden.c | 67 ++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/hidden.h | 31 ++++++++++++++
 drivers/net/dsa/mv88e6xxx/port.h   | 10 -----
 5 files changed, 100 insertions(+), 63 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/hidden.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/hidden.h

Comments

Andrew Lunn Aug. 22, 2019, 1:04 p.m. UTC | #1 
On Thu, Aug 22, 2019 at 01:27:17AM +0200, Marek Behún wrote:
> This patch moves the functions operating on the hidden debug registers
> into it's own file, hidden.c.
> 
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Andrew Lunn Aug. 22, 2019, 1:10 p.m. UTC | #2 
On Thu, Aug 22, 2019 at 01:27:17AM +0200, Marek Behún wrote:
> This patch moves the functions operating on the hidden debug registers
> into it's own file, hidden.c.

Humm, actually...

These are in the port register space. Maybe it would be better to move
them into port.c/port.h?

What you really need is that they have global scope within the driver
so you can call them. So add the functions definitions to port.h.

Vivien, what do you think?

	Andrew
Vivien Didelot Aug. 22, 2019, 5:21 p.m. UTC | #3 
Hi Marek, Andrew,

On Thu, 22 Aug 2019 15:10:47 +0200, Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, Aug 22, 2019 at 01:27:17AM +0200, Marek Behún wrote:
> > This patch moves the functions operating on the hidden debug registers
> > into it's own file, hidden.c.
> 
> Humm, actually...
> 
> These are in the port register space. Maybe it would be better to move
> them into port.c/port.h?
> 
> What you really need is that they have global scope within the driver
> so you can call them. So add the functions definitions to port.h.
> 
> Vivien, what do you think?

Andrew's correct. Code accessing internal registers in the mv88e6xxx driver
is split per internal SMI device. An internal SMI device is a "column" of 32
registers found in the datasheet, accessed via its dedicated internal SMI
device address, like ports, Global 1 (often 0x1b), Global 2 (often 0x1c),
and so on. Each internal SMI device has its unique header file, describing
all registers it contains. Then if the corresponding .c file has a portion
specific enough, it can be moved to its own .c file, like global1_atu.c,
global1_vtu.c, etc.

So keep these port registers definitions ordered in port.h with a naming as
closed to the documentation as possible, prefixing them with MV88E6XXX_PORT_
(or the model of reference if that is specific to a few models only), and
please describe the bits with an ordered 0x1234 format as well. The port.h
header is fortunately already a good example of how it should be done.

Then you can include it in a new port_hidden.c file, which implements
mv88e6xxx_port_hidden_* internal helpers (or mv88e6789_port_ if specific to a
model again) to access these hidden port registers, and use them as convenience
in chip.c:mv88e6xxx_errata_setup() or wherever a feature is implemented.


Thanks a lot,

	Vivien
diff mbox series

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
index e85755dde90b..40f52d8f478a 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -7,6 +7,7 @@  mv88e6xxx-objs += global1_vtu.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2) += global2.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2) += global2_avb.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_GLOBAL2) += global2_scratch.o
+mv88e6xxx-objs += hidden.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += hwtstamp.o
 mv88e6xxx-objs += phy.o
 mv88e6xxx-objs += port.o
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 176173d96512..2dab46ad1d63 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -34,6 +34,7 @@ 
 #include "chip.h"
 #include "global1.h"
 #include "global2.h"
+#include "hidden.h"
 #include "hwtstamp.h"
 #include "phy.h"
 #include "port.h"
@@ -2317,59 +2318,6 @@  static int mv88e6xxx_stats_setup(struct mv88e6xxx_chip *chip)
 	return mv88e6xxx_g1_stats_clear(chip);
 }
 
-/* The mv88e6390 has some hidden registers used for debug and
- * development. The errata also makes use of them.
- */
-static int mv88e6390_hidden_write(struct mv88e6xxx_chip *chip, int port,
-				  int reg, u16 val)
-{
-	u16 ctrl;
-	int err;
-
-	err = mv88e6xxx_port_write(chip, PORT_RESERVED_1A_DATA_PORT,
-				   PORT_RESERVED_1A, val);
-	if (err)
-		return err;
-
-	ctrl = PORT_RESERVED_1A_BUSY | PORT_RESERVED_1A_WRITE |
-	       PORT_RESERVED_1A_BLOCK | port << PORT_RESERVED_1A_PORT_SHIFT |
-	       reg;
-
-	return mv88e6xxx_port_write(chip, PORT_RESERVED_1A_CTRL_PORT,
-				    PORT_RESERVED_1A, ctrl);
-}
-
-static int mv88e6390_hidden_wait(struct mv88e6xxx_chip *chip)
-{
-	int bit = __bf_shf(PORT_RESERVED_1A_BUSY);
-
-	return mv88e6xxx_wait_bit(chip, PORT_RESERVED_1A_CTRL_PORT,
-				  PORT_RESERVED_1A, bit, 0);
-}
-
-static int mv88e6390_hidden_read(struct mv88e6xxx_chip *chip, int port,
-				  int reg, u16 *val)
-{
-	u16 ctrl;
-	int err;
-
-	ctrl = PORT_RESERVED_1A_BUSY | PORT_RESERVED_1A_READ |
-	       PORT_RESERVED_1A_BLOCK | port << PORT_RESERVED_1A_PORT_SHIFT |
-	       reg;
-
-	err = mv88e6xxx_port_write(chip, PORT_RESERVED_1A_CTRL_PORT,
-				   PORT_RESERVED_1A, ctrl);
-	if (err)
-		return err;
-
-	err = mv88e6390_hidden_wait(chip);
-	if (err)
-		return err;
-
-	return 	mv88e6xxx_port_read(chip, PORT_RESERVED_1A_DATA_PORT,
-				    PORT_RESERVED_1A, val);
-}
-
 /* Check if the errata has already been applied. */
 static bool mv88e6390_setup_errata_applied(struct mv88e6xxx_chip *chip)
 {
diff --git a/drivers/net/dsa/mv88e6xxx/hidden.c b/drivers/net/dsa/mv88e6xxx/hidden.c
new file mode 100644
index 000000000000..6ea47b03679f
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/hidden.c
@@ -0,0 +1,67 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Marvell 88E6xxx Switch Hidden Registers support
+ *
+ * Copyright (c) 2008 Marvell Semiconductor
+ *
+ * Copyright (c) 2019 Andrew Lunn <andrew@lunn.ch>
+ */
+
+#include <linux/bitfield.h>
+
+#include "chip.h"
+#include "port.h"
+#include "hidden.h"
+
+/* The mv88e6390 and mv88e6341 have some hidden registers used for debug and
+ * development. The errata also makes use of them.
+ */
+int mv88e6390_hidden_write(struct mv88e6xxx_chip *chip, int port,
+			   int reg, u16 val)
+{
+	u16 ctrl;
+	int err;
+
+	err = mv88e6xxx_port_write(chip, PORT_RESERVED_1A_DATA_PORT,
+				   PORT_RESERVED_1A, val);
+	if (err)
+		return err;
+
+	ctrl = PORT_RESERVED_1A_BUSY | PORT_RESERVED_1A_WRITE |
+	       PORT_RESERVED_1A_BLOCK | port << PORT_RESERVED_1A_PORT_SHIFT |
+	       reg;
+
+	return mv88e6xxx_port_write(chip, PORT_RESERVED_1A_CTRL_PORT,
+				    PORT_RESERVED_1A, ctrl);
+}
+
+int mv88e6390_hidden_wait(struct mv88e6xxx_chip *chip)
+{
+	int bit = __bf_shf(PORT_RESERVED_1A_BUSY);
+
+	return mv88e6xxx_wait_bit(chip, PORT_RESERVED_1A_CTRL_PORT,
+				  PORT_RESERVED_1A, bit, 0);
+}
+
+int mv88e6390_hidden_read(struct mv88e6xxx_chip *chip, int port,
+			  int reg, u16 *val)
+{
+	u16 ctrl;
+	int err;
+
+	ctrl = PORT_RESERVED_1A_BUSY | PORT_RESERVED_1A_READ |
+	       PORT_RESERVED_1A_BLOCK | port << PORT_RESERVED_1A_PORT_SHIFT |
+	       reg;
+
+	err = mv88e6xxx_port_write(chip, PORT_RESERVED_1A_CTRL_PORT,
+				   PORT_RESERVED_1A, ctrl);
+	if (err)
+		return err;
+
+	err = mv88e6390_hidden_wait(chip);
+	if (err)
+		return err;
+
+	return mv88e6xxx_port_read(chip, PORT_RESERVED_1A_DATA_PORT,
+				   PORT_RESERVED_1A, val);
+}
diff --git a/drivers/net/dsa/mv88e6xxx/hidden.h b/drivers/net/dsa/mv88e6xxx/hidden.h
new file mode 100644
index 000000000000..5e2de0a7f22d
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/hidden.h
@@ -0,0 +1,31 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Marvell 88E6xxx Switch Hidden Registers support
+ *
+ * Copyright (c) 2008 Marvell Semiconductor
+ *
+ * Copyright (c) 2019 Andrew Lunn <andrew@lunn.ch>
+ */
+
+#ifndef _MV88E6XXX_HIDDEN_H
+#define _MV88E6XXX_HIDDEN_H
+
+#include "chip.h"
+
+/* Offset 0x1a: Magic undocumented errata register */
+#define PORT_RESERVED_1A			0x1a
+#define PORT_RESERVED_1A_BUSY			BIT(15)
+#define PORT_RESERVED_1A_WRITE			BIT(14)
+#define PORT_RESERVED_1A_READ			0
+#define PORT_RESERVED_1A_PORT_SHIFT		5
+#define PORT_RESERVED_1A_BLOCK			(0xf << 10)
+#define PORT_RESERVED_1A_CTRL_PORT		4
+#define PORT_RESERVED_1A_DATA_PORT		5
+
+int mv88e6390_hidden_write(struct mv88e6xxx_chip *chip, int port,
+			   int reg, u16 val);
+int mv88e6390_hidden_wait(struct mv88e6xxx_chip *chip);
+int mv88e6390_hidden_read(struct mv88e6xxx_chip *chip, int port,
+			  int reg, u16 *val);
+
+#endif /* _MV88E6XXX_HIDDEN_H */
diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
index 1abf5ea033e2..5c5e8e7397eb 100644
--- a/drivers/net/dsa/mv88e6xxx/port.h
+++ b/drivers/net/dsa/mv88e6xxx/port.h
@@ -260,16 +260,6 @@ 
 /* Offset 0x19: Port IEEE Priority Remapping Registers (4-7) */
 #define MV88E6095_PORT_IEEE_PRIO_REMAP_4567	0x19
 
-/* Offset 0x1a: Magic undocumented errata register */
-#define PORT_RESERVED_1A			0x1a
-#define PORT_RESERVED_1A_BUSY			BIT(15)
-#define PORT_RESERVED_1A_WRITE			BIT(14)
-#define PORT_RESERVED_1A_READ			0
-#define PORT_RESERVED_1A_PORT_SHIFT		5
-#define PORT_RESERVED_1A_BLOCK			(0xf << 10)
-#define PORT_RESERVED_1A_CTRL_PORT		4
-#define PORT_RESERVED_1A_DATA_PORT		5
-
 int mv88e6xxx_port_read(struct mv88e6xxx_chip *chip, int port, int reg,
 			u16 *val);
 int mv88e6xxx_port_write(struct mv88e6xxx_chip *chip, int port, int reg,