diff mbox series

[RFC,v2,linux-next,03/14] net: dsa: sja1105: the 0x1F0000 SGMII "base address" is actually MDIO_MMD_VEND2

Message ID 20210526135535.2515123-4-vladimir.oltean@nxp.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Add NXP SJA1110 support to the sja1105 DSA driver | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Vladimir Oltean May 26, 2021, 1:55 p.m. UTC
Looking at the SGMII PCS from SJA1110, which is accessed indirectly
through a different base address as can be seen in the next patch, it
appears odd that the address accessed through indirection still
references the base address from the SJA1105S register map (first MDIO
register is at 0x1f0000), when it could index the SGMII registers
starting from zero.

Except that the 0x1f0000 is not a base address at all, it seems. It is
0x1f << 16 | 0x0000, and 0x1f is coding for the vendor-specific MMD2.
So, it turns out, the Synopsys PCS implements all its registers inside
the vendor-specific MMDs 1 and 2 (0x1e and 0x1f). This explains why the
PCS has no overlaps (for the other MMDs) with other register regions of
the switch (because no other MMDs are implemented).

Change the code to remove the SGMII "base address" and explicitly encode
the MMD for reads/writes. This will become necessary for SJA1110 support.

Cc: Russell King <linux@armlinux.org.uk>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes in v2:
None.

 drivers/net/dsa/sja1105/sja1105.h      |  1 -
 drivers/net/dsa/sja1105/sja1105_main.c | 31 +++++++++++++-------------
 drivers/net/dsa/sja1105/sja1105_spi.c  |  1 -
 3 files changed, 16 insertions(+), 17 deletions(-)

Comments

Russell King (Oracle) May 26, 2021, 3:24 p.m. UTC | #1
On Wed, May 26, 2021 at 04:55:24PM +0300, Vladimir Oltean wrote:
> -	const struct sja1105_regs *regs = priv->info->regs;
> +	u64 addr = (mmd << 16) | pcs_reg;

What is the reason for using "u64" here. pcs_reg is 16-bits, and mmd is
five bits, which is well below 32 bits. So, why not u32?
Vladimir Oltean May 26, 2021, 3:34 p.m. UTC | #2
Hi Russell,

On Wed, May 26, 2021 at 04:24:54PM +0100, Russell King (Oracle) wrote:
> On Wed, May 26, 2021 at 04:55:24PM +0300, Vladimir Oltean wrote:
> > -	const struct sja1105_regs *regs = priv->info->regs;
> > +	u64 addr = (mmd << 16) | pcs_reg;
> 
> What is the reason for using "u64" here. pcs_reg is 16-bits, and mmd is
> five bits, which is well below 32 bits. So, why not u32?

The "addr" variable holds a SPI address, and in the sja1105 driver, the
SPI addresses are universally held in u64 variables, mainly because of
the packing() API (Documentation/core-api/packing.rst).

In this case, the "addr" is passed to the "u64 reg_addr" parameter of
sja1105_xfer_u32, which ends up being packed into bits 24:4 of the SPI
message header in sja1105_spi_message_pack().

You might ask: is the SPI address simply derived from (mmd << 16 | pcs_reg)?
The answer is yes, I'm a bit surprised by that too. The PCS doesn't
overlap with other SPI memory regions because only 2 MMDs are implemented
(this is explained in the commit message).

I would probably reconsider some things if I were to write the driver
again, including some accessors which are more streamlined than packing(),
but currently this is what we have.
Russell King (Oracle) May 26, 2021, 3:42 p.m. UTC | #3
On Wed, May 26, 2021 at 06:34:47PM +0300, Vladimir Oltean wrote:
> Hi Russell,
> 
> On Wed, May 26, 2021 at 04:24:54PM +0100, Russell King (Oracle) wrote:
> > On Wed, May 26, 2021 at 04:55:24PM +0300, Vladimir Oltean wrote:
> > > -	const struct sja1105_regs *regs = priv->info->regs;
> > > +	u64 addr = (mmd << 16) | pcs_reg;
> > 
> > What is the reason for using "u64" here. pcs_reg is 16-bits, and mmd is
> > five bits, which is well below 32 bits. So, why not u32?
> 
> The "addr" variable holds a SPI address, and in the sja1105 driver, the
> SPI addresses are universally held in u64 variables, mainly because of
> the packing() API (Documentation/core-api/packing.rst).

As you are passing it into a function, the argument of which is a u64,
the compiler will promote the u32 to a u64 by itself. I guess it
doesn't actually matter, but the current code just looks really weird.
diff mbox series

Patch

diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
index 2ec03917feb3..830ea5ca359f 100644
--- a/drivers/net/dsa/sja1105/sja1105.h
+++ b/drivers/net/dsa/sja1105/sja1105.h
@@ -48,7 +48,6 @@  struct sja1105_regs {
 	u64 rgu;
 	u64 vl_status;
 	u64 config;
-	u64 sgmii;
 	u64 rmii_pll1;
 	u64 ptppinst;
 	u64 ptppindur;
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index 1a49cfce9611..292490a2ea0e 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -898,36 +898,34 @@  static int sja1105_parse_dt(struct sja1105_private *priv,
 	return rc;
 }
 
-static int sja1105_sgmii_read(struct sja1105_private *priv, int port,
+static int sja1105_sgmii_read(struct sja1105_private *priv, int port, int mmd,
 			      int pcs_reg)
 {
-	const struct sja1105_regs *regs = priv->info->regs;
+	u64 addr = (mmd << 16) | pcs_reg;
 	u32 val;
 	int rc;
 
 	if (port != SJA1105_SGMII_PORT)
 		return -ENODEV;
 
-	rc = sja1105_xfer_u32(priv, SPI_READ, regs->sgmii + pcs_reg,
-			      &val, NULL);
+	rc = sja1105_xfer_u32(priv, SPI_READ, addr, &val, NULL);
 	if (rc < 0)
 		return rc;
 
 	return val;
 }
 
-static int sja1105_sgmii_write(struct sja1105_private *priv, int port,
+static int sja1105_sgmii_write(struct sja1105_private *priv, int port, int mmd,
 			       int pcs_reg, u16 pcs_val)
 {
-	const struct sja1105_regs *regs = priv->info->regs;
+	u64 addr = (mmd << 16) | pcs_reg;
 	u32 val = pcs_val;
 	int rc;
 
 	if (port != SJA1105_SGMII_PORT)
 		return -ENODEV;
 
-	rc = sja1105_xfer_u32(priv, SPI_WRITE, regs->sgmii + pcs_reg,
-			      &val, NULL);
+	rc = sja1105_xfer_u32(priv, SPI_WRITE, addr, &val, NULL);
 	if (rc < 0)
 		return rc;
 
@@ -943,24 +941,24 @@  static void sja1105_sgmii_pcs_config(struct sja1105_private *priv, int port,
 	 * stop the clock during LPI mode, make the MAC reconfigure
 	 * autonomously after PCS autoneg is done, flush the internal FIFOs.
 	 */
-	sja1105_sgmii_write(priv, port, SJA1105_DC1,
+	sja1105_sgmii_write(priv, port, MDIO_MMD_VEND2, SJA1105_DC1,
 			    SJA1105_DC1_EN_VSMMD1 |
 			    SJA1105_DC1_CLOCK_STOP_EN |
 			    SJA1105_DC1_MAC_AUTO_SW |
 			    SJA1105_DC1_INIT);
 	/* DIGITAL_CONTROL_2: No polarity inversion for TX and RX lanes */
-	sja1105_sgmii_write(priv, port, SJA1105_DC2,
+	sja1105_sgmii_write(priv, port, MDIO_MMD_VEND2, SJA1105_DC2,
 			    SJA1105_DC2_TX_POL_INV_DISABLE);
 	/* AUTONEG_CONTROL: Use SGMII autoneg */
 	if (an_master)
 		ac |= SJA1105_AC_PHY_MODE | SJA1105_AC_SGMII_LINK;
-	sja1105_sgmii_write(priv, port, SJA1105_AC, ac);
+	sja1105_sgmii_write(priv, port, MDIO_MMD_VEND2, SJA1105_AC, ac);
 	/* BASIC_CONTROL: enable in-band AN now, if requested. Otherwise,
 	 * sja1105_sgmii_pcs_force_speed must be called later for the link
 	 * to become operational.
 	 */
 	if (an_enabled)
-		sja1105_sgmii_write(priv, port, MII_BMCR,
+		sja1105_sgmii_write(priv, port, MDIO_MMD_VEND2, MDIO_CTRL1,
 				    BMCR_ANENABLE | BMCR_ANRESTART);
 }
 
@@ -983,7 +981,8 @@  static void sja1105_sgmii_pcs_force_speed(struct sja1105_private *priv,
 		dev_err(priv->ds->dev, "Invalid speed %d\n", speed);
 		return;
 	}
-	sja1105_sgmii_write(priv, port, MII_BMCR, pcs_speed | BMCR_FULLDPLX);
+	sja1105_sgmii_write(priv, port, MDIO_MMD_VEND2, MDIO_CTRL1,
+			    pcs_speed | BMCR_FULLDPLX);
 }
 
 /* Convert link speed from SJA1105 to ethtool encoding */
@@ -1201,7 +1200,7 @@  static int sja1105_mac_pcs_get_state(struct dsa_switch *ds, int port,
 	int ais;
 
 	/* Read the vendor-specific AUTONEG_INTR_STATUS register */
-	ais = sja1105_sgmii_read(priv, port, SJA1105_AIS);
+	ais = sja1105_sgmii_read(priv, port, MDIO_MMD_VEND2, SJA1105_AIS);
 	if (ais < 0)
 		return ais;
 
@@ -1905,7 +1904,9 @@  int sja1105_static_config_reload(struct sja1105_private *priv,
 		mac[i].speed = SJA1105_SPEED_AUTO;
 
 		if (sja1105_supports_sgmii(priv, i))
-			bmcr[i] = sja1105_sgmii_read(priv, i, MII_BMCR);
+			bmcr[i] = sja1105_sgmii_read(priv, i,
+						     MDIO_MMD_VEND2,
+						     MDIO_CTRL1);
 	}
 
 	/* No PTP operations can run right now */
diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c
index d0bc6cf90bfd..615e0906b1fa 100644
--- a/drivers/net/dsa/sja1105/sja1105_spi.c
+++ b/drivers/net/dsa/sja1105/sja1105_spi.c
@@ -440,7 +440,6 @@  static struct sja1105_regs sja1105pqrs_regs = {
 	.pad_mii_tx = {0x100800, 0x100802, 0x100804, 0x100806, 0x100808},
 	.pad_mii_rx = {0x100801, 0x100803, 0x100805, 0x100807, 0x100809},
 	.pad_mii_id = {0x100810, 0x100811, 0x100812, 0x100813, 0x100814},
-	.sgmii = 0x1F0000,
 	.rmii_pll1 = 0x10000A,
 	.cgu_idiv = {0x10000B, 0x10000C, 0x10000D, 0x10000E, 0x10000F},
 	.stats[MAC] = {0x200, 0x202, 0x204, 0x206, 0x208},