[net-next PATCH 00/13] Multiple improvement for qca8337 switch

[net-next PATCH 00/13] Multiple improvement for qca8337 switch

[Ansuel Smith]

This series is the final step of a long process of porting 80+ devices
to use the new qca8k driver instead of the hacky qca one based on never
merged swconfig platform.
Some background to justify all these additions.
QCA used a special binding to declare raw initval to set the swich. I
made a script to convert all these magic values and convert 80+ dts and
scan all the needed "unsupported regs". We find a baseline where we
manage to find the common and used regs so in theory hopefully we don't
have to add anymore things.
We discovered lots of things with this, especially about how differently
qca8327 works compared to qca8337.

In short, we found that qca8327 have some problem with suspend/resume for
their internal phy. It instead sets some dedicated regs that suspend the
phy without setting the standard bit. First 4 patch are to fix this.
There is also a patch about preferring master. This is directly from the
original driver and it seems to be needed to prevent some problem with
the pause frame.

Every ipq806x target sets the mac power sel and this specific reg
regulates the output voltage of the regulator. Without this some
instability can occur.

Some configuration (for some reason) swap mac6 with mac0. We add support
for this.
Also, we discovered that some device doesn't work at all with pll enabled
for sgmii line. In the original code this was based on the switch
revision. In later revision the pll regs were decided based on the switch
type (disabled for qca8327 and enabled for qca8337) but still some
device had that disabled in the initval regs.
Considering we found at least one qca8337 device that required pll
disabled to work (no traffic problem) we decided to introduce a binding
to enable pll and set it only with that.

Lastly, we add support for led open drain that require the power-on-sel
to set. Also, some device have only the power-on-sel set in the initval
so we add also support for that. This is needed for the correct function
of the switch leds.
Qca8327 have a special reg in the pws regs that set it to a reduced
48pin layout. This is needed or the switch doesn't work.

These are all the special configuration we find on all these devices that
are from various targets. Mostly ath79, ipq806x and bcm53xx.

Ansuel Smith (13):
  drivers: net: phy: at803x: fix resume for QCA8327 phy
  drivers: net: phy: at803x: add DAC amplitude fix for 8327 phy
  drivers: net: phy: at803x: enable prefer master for 83xx internal phy
  drivers: net: phy: at803x: better describe debug regs
  net: dsa: qca8k: add mac_power_sel support
  Documentation: devicetree: net: dsa: qca8k: document rgmii_1_8v
    bindings
  net: dsa: qca8k: add support for mac6_exchange, sgmii falling edge
  dt-bindings: net: dsa: qca8k: Add MAC swap and clock phase properties
  net: dsa: qca8k: check rgmii also on port 6 if exchanged
  net: dsa: qca8k: add explicit SGMII PLL enable
  devicetree: net: dsa: qca8k: Document qca,sgmii-enable-pll
  drivers: net: dsa: qca8k: add support for pws config reg
  Documentation: devicetree: net: dsa: qca8k: document open drain
    binding

 .../devicetree/bindings/net/dsa/qca8k.txt     |  11 ++
 drivers/net/dsa/qca8k.c                       | 111 ++++++++++++++-
 drivers/net/dsa/qca8k.h                       |  11 ++
 drivers/net/phy/at803x.c                      | 127 +++++++++++++++---
 4 files changed, 240 insertions(+), 20 deletions(-)

-- 
2.32.0

[net-next PATCH 01/13] drivers: net: phy: at803x: fix resume for QCA8327 phy

[Ansuel Smith]

From Documentation phy resume triggers phy reset and restart
auto-negotiation. Add a dedicated function to wait reset to finish as
it was notice a regression where port sometime are not reliable after a
suspend/resume session. The reset wait logic is copied from phy_poll_reset.
Add dedicated suspend function to use genphy_suspend only with QCA8337
phy and set only additional debug settings for QCA8327. With more test
it was reported that QCA8327 doesn't proprely support this mode and
using this cause the unreliability of the switch ports, especially the
malfunction of the port0.

Fixes: 52a6cdbe43a3 ("net: phy: at803x: add resume/suspend function to qca83xx phy")
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 75 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 66 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 3feee4d59030..9090f384c29e 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -92,9 +92,14 @@
 #define AT803X_DEBUG_REG_5			0x05
 #define AT803X_DEBUG_TX_CLK_DLY_EN		BIT(8)
 
+#define AT803X_DEBUG_REG_HIB_CTRL		0x0b
+#define   AT803X_DEBUG_HIB_CTRL_SEL_RST_80U	BIT(10)
+#define   AT803X_DEBUG_HIB_CTRL_EN_ANY_CHANGE	BIT(13)
+
 #define AT803X_DEBUG_REG_3C			0x3C
 
-#define AT803X_DEBUG_REG_3D			0x3D
+#define AT803X_DEBUG_REG_GREEN			0x3D
+#define   AT803X_DEBUG_GATE_CLK_IN1000		BIT(6)
 
 #define AT803X_DEBUG_REG_1F			0x1F
 #define AT803X_DEBUG_PLL_ON			BIT(2)
@@ -1295,7 +1300,7 @@ static int qca83xx_config_init(struct phy_device *phydev)
 		/* For 100M waveform */
 		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_0, 0x02ea);
 		/* Turn on Gigabit clock */
-		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_3D, 0x68a0);
+		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_GREEN, 0x68a0);
 		break;
 
 	case 2:
@@ -1303,7 +1308,7 @@ static int qca83xx_config_init(struct phy_device *phydev)
 		fallthrough;
 	case 4:
 		phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_AZ_DEBUG, 0x803f);
-		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_3D, 0x6860);
+		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_GREEN, 0x6860);
 		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_5, 0x2c46);
 		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_3C, 0x6000);
 		break;
@@ -1312,6 +1317,58 @@ static int qca83xx_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int qca83xx_resume(struct phy_device *phydev)
+{
+	int ret, val;
+
+	/* Skip reset if not suspended */
+	if (!phydev->suspended)
+		return 0;
+
+	/* Reinit the port, reset values set by suspend */
+	qca83xx_config_init(phydev);
+
+	/* Reset the port on port resume */
+	phy_set_bits(phydev, MII_BMCR, BMCR_RESET | BMCR_ANENABLE);
+
+	/* On resume from suspend the switch execute a reset and
+	 * restart auto-negotiation. Wait for reset to complete.
+	 */
+	ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
+				    50000, 600000, true);
+	if (ret)
+		return ret;
+
+	msleep(1);
+
+	return 0;
+}
+
+static int qca83xx_suspend(struct phy_device *phydev)
+{
+	u16 mask = 0;
+
+	/* Only QCA8337 support actual suspend.
+	 * QCA8327 cause port unreliability when phy suspend
+	 * is set.
+	 */
+	if (phydev->drv->phy_id == QCA8337_PHY_ID) {
+		genphy_suspend(phydev);
+	} else {
+		mask |= ~(BMCR_SPEED1000 | BMCR_FULLDPLX);
+		phy_modify(phydev, MII_BMCR, mask, 0);
+	}
+
+	at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_GREEN,
+			      AT803X_DEBUG_GATE_CLK_IN1000, 0);
+
+	at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
+			      AT803X_DEBUG_HIB_CTRL_EN_ANY_CHANGE |
+			      AT803X_DEBUG_HIB_CTRL_SEL_RST_80U, 0);
+
+	return 0;
+}
+
 static struct phy_driver at803x_driver[] = {
 {
 	/* Qualcomm Atheros AR8035 */
@@ -1421,8 +1478,8 @@ static struct phy_driver at803x_driver[] = {
 	.get_sset_count		= at803x_get_sset_count,
 	.get_strings		= at803x_get_strings,
 	.get_stats		= at803x_get_stats,
-	.suspend		= genphy_suspend,
-	.resume			= genphy_resume,
+	.suspend		= qca83xx_suspend,
+	.resume			= qca83xx_resume,
 }, {
 	/* QCA8327-A from switch QCA8327-AL1A */
 	.phy_id			= QCA8327_A_PHY_ID,
@@ -1436,8 +1493,8 @@ static struct phy_driver at803x_driver[] = {
 	.get_sset_count		= at803x_get_sset_count,
 	.get_strings		= at803x_get_strings,
 	.get_stats		= at803x_get_stats,
-	.suspend		= genphy_suspend,
-	.resume			= genphy_resume,
+	.suspend		= qca83xx_suspend,
+	.resume			= qca83xx_resume,
 }, {
 	/* QCA8327-B from switch QCA8327-BL1A */
 	.phy_id			= QCA8327_B_PHY_ID,
@@ -1451,8 +1508,8 @@ static struct phy_driver at803x_driver[] = {
 	.get_sset_count		= at803x_get_sset_count,
 	.get_strings		= at803x_get_strings,
 	.get_stats		= at803x_get_stats,
-	.suspend		= genphy_suspend,
-	.resume			= genphy_resume,
+	.suspend		= qca83xx_suspend,
+	.resume			= qca83xx_resume,
 }, };
 
 module_phy_driver(at803x_driver);
-- 
2.32.0

Re: [net-next PATCH 01/13] drivers: net: phy: at803x: fix resume for QCA8327 phy

[Andrew Lunn]

On Thu, Oct 07, 2021 at 12:35:51AM +0200, Ansuel Smith wrote:
> >From Documentation phy resume triggers phy reset and restart
> auto-negotiation. Add a dedicated function to wait reset to finish as
> it was notice a regression where port sometime are not reliable after a
> suspend/resume session. The reset wait logic is copied from phy_poll_reset.
> Add dedicated suspend function to use genphy_suspend only with QCA8337
> phy and set only additional debug settings for QCA8327. With more test
> it was reported that QCA8327 doesn't proprely support this mode and
> using this cause the unreliability of the switch ports, especially the
> malfunction of the port0.
> 
> Fixes: 52a6cdbe43a3 ("net: phy: at803x: add resume/suspend function to qca83xx phy")

Please base it on net, since it is a fix.

> -#define AT803X_DEBUG_REG_3D			0x3D
> +#define AT803X_DEBUG_REG_GREEN			0x3D

Fixes are supposed to be minimal. So i would not include the rename of
3D to GREEN in the fix. Do that in a patch for net-next.

   Andrew

[net-next PATCH 02/13] drivers: net: phy: at803x: add DAC amplitude fix for 8327 phy

[Ansuel Smith]

QCA8327 internal phy require DAC amplitude adjustement set to +6% with
100m speed. Also add additional define to report a change of the same
reg in QCA8337. (different scope it does set 1000m voltage)
Add link_change_notify function to set the proper amplitude adjustement
on PHY_RUNNING state and disable on any other state.

Fixes: c6bcec0d6928 ("net: phy: at803x: add support for qca 8327 A variant internal phy")
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 9090f384c29e..71237a4e85a3 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -87,6 +87,8 @@
 #define AT803X_PSSR_MR_AN_COMPLETE		0x0200
 
 #define AT803X_DEBUG_REG_0			0x00
+#define QCA8327_DEBUG_MANU_CTRL_EN		BIT(2)
+#define QCA8337_DEBUG_MANU_CTRL_EN		GENMASK(3, 2)
 #define AT803X_DEBUG_RX_CLK_DLY_EN		BIT(15)
 
 #define AT803X_DEBUG_REG_5			0x05
@@ -1314,9 +1316,37 @@ static int qca83xx_config_init(struct phy_device *phydev)
 		break;
 	}
 
+	/* QCA8327 require DAC amplitude adjustment for 100m set to +6%.
+	 * Disable on init and enable only with 100m speed following
+	 * qca original source code.
+	 */
+	if (phydev->drv->phy_id == QCA8327_A_PHY_ID ||
+	    phydev->drv->phy_id == QCA8327_B_PHY_ID)
+		at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
+				      QCA8327_DEBUG_MANU_CTRL_EN, 0);
+
 	return 0;
 }
 
+static void qca83xx_link_change_notify(struct phy_device *phydev)
+{
+	/* QCA8337 doesn't require DAC Amplitude adjustement */
+	if (phydev->drv->phy_id == QCA8337_PHY_ID)
+		return;
+
+	/* Set DAC Amplitude adjustment to +6% for 100m on link running */
+	if (phydev->state == PHY_RUNNING) {
+		if (phydev->speed == SPEED_100)
+			at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
+					      QCA8327_DEBUG_MANU_CTRL_EN,
+					      QCA8327_DEBUG_MANU_CTRL_EN);
+	} else {
+		/* Reset DAC Amplitude adjustment */
+		at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
+				      QCA8327_DEBUG_MANU_CTRL_EN, 0);
+	}
+}
+
 static int qca83xx_resume(struct phy_device *phydev)
 {
 	int ret, val;
@@ -1471,6 +1501,7 @@ static struct phy_driver at803x_driver[] = {
 	.phy_id_mask		= QCA8K_PHY_ID_MASK,
 	.name			= "Qualcomm Atheros 8337 internal PHY",
 	/* PHY_GBIT_FEATURES */
+	.link_change_notify	= qca83xx_link_change_notify,
 	.probe			= at803x_probe,
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
@@ -1486,6 +1517,7 @@ static struct phy_driver at803x_driver[] = {
 	.phy_id_mask		= QCA8K_PHY_ID_MASK,
 	.name			= "Qualcomm Atheros 8327-A internal PHY",
 	/* PHY_GBIT_FEATURES */
+	.link_change_notify	= qca83xx_link_change_notify,
 	.probe			= at803x_probe,
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
@@ -1501,6 +1533,7 @@ static struct phy_driver at803x_driver[] = {
 	.phy_id_mask		= QCA8K_PHY_ID_MASK,
 	.name			= "Qualcomm Atheros 8327-B internal PHY",
 	/* PHY_GBIT_FEATURES */
+	.link_change_notify	= qca83xx_link_change_notify,
 	.probe			= at803x_probe,
 	.flags			= PHY_IS_INTERNAL,
 	.config_init		= qca83xx_config_init,
-- 
2.32.0

Re: [net-next PATCH 02/13] drivers: net: phy: at803x: add DAC amplitude fix for 8327 phy

[Andrew Lunn]

On Thu, Oct 07, 2021 at 12:35:52AM +0200, Ansuel Smith wrote:
> QCA8327 internal phy require DAC amplitude adjustement set to +6% with
> 100m speed. Also add additional define to report a change of the same
> reg in QCA8337. (different scope it does set 1000m voltage)
> Add link_change_notify function to set the proper amplitude adjustement
> on PHY_RUNNING state and disable on any other state.
> 
> Fixes: c6bcec0d6928 ("net: phy: at803x: add support for qca 8327 A variant internal phy")
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

Since this is a fix, you might want to send it on its own, based on
net.

> +	/* QCA8327 require DAC amplitude adjustment for 100m set to +6%.
> +	 * Disable on init and enable only with 100m speed following
> +	 * qca original source code.
> +	 */
> +	if (phydev->drv->phy_id == QCA8327_A_PHY_ID ||
> +	    phydev->drv->phy_id == QCA8327_B_PHY_ID)
> +		at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> +				      QCA8327_DEBUG_MANU_CTRL_EN, 0);
> +
>  	return 0;
>  }
>  
> +static void qca83xx_link_change_notify(struct phy_device *phydev)
> +{
> +	/* QCA8337 doesn't require DAC Amplitude adjustement */
> +	if (phydev->drv->phy_id == QCA8337_PHY_ID)
> +		return;
> +
> +	/* Set DAC Amplitude adjustment to +6% for 100m on link running */
> +	if (phydev->state == PHY_RUNNING) {
> +		if (phydev->speed == SPEED_100)
> +			at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> +					      QCA8327_DEBUG_MANU_CTRL_EN,
> +					      QCA8327_DEBUG_MANU_CTRL_EN);
> +	} else {
> +		/* Reset DAC Amplitude adjustment */
> +		at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> +				      QCA8327_DEBUG_MANU_CTRL_EN, 0);

Here you don't make it conditional on QCA8327_A_PHY_ID and
QCA8327_B_PHY_ID, where as above you do?

	  Andrew

Re: [net-next PATCH 02/13] drivers: net: phy: at803x: add DAC amplitude fix for 8327 phy

[Ansuel Smith]

On Thu, Oct 07, 2021 at 01:59:55AM +0200, Andrew Lunn wrote:
> On Thu, Oct 07, 2021 at 12:35:52AM +0200, Ansuel Smith wrote:
> > QCA8327 internal phy require DAC amplitude adjustement set to +6% with
> > 100m speed. Also add additional define to report a change of the same
> > reg in QCA8337. (different scope it does set 1000m voltage)
> > Add link_change_notify function to set the proper amplitude adjustement
> > on PHY_RUNNING state and disable on any other state.
> > 
> > Fixes: c6bcec0d6928 ("net: phy: at803x: add support for qca 8327 A variant internal phy")
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> 
> Since this is a fix, you might want to send it on its own, based on
> net.
> 
> > +	/* QCA8327 require DAC amplitude adjustment for 100m set to +6%.
> > +	 * Disable on init and enable only with 100m speed following
> > +	 * qca original source code.
> > +	 */
> > +	if (phydev->drv->phy_id == QCA8327_A_PHY_ID ||
> > +	    phydev->drv->phy_id == QCA8327_B_PHY_ID)
> > +		at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> > +				      QCA8327_DEBUG_MANU_CTRL_EN, 0);
> > +
> >  	return 0;
> >  }
> >  
> > +static void qca83xx_link_change_notify(struct phy_device *phydev)
> > +{
> > +	/* QCA8337 doesn't require DAC Amplitude adjustement */
> > +	if (phydev->drv->phy_id == QCA8337_PHY_ID)
> > +		return;
> > +
> > +	/* Set DAC Amplitude adjustment to +6% for 100m on link running */
> > +	if (phydev->state == PHY_RUNNING) {
> > +		if (phydev->speed == SPEED_100)
> > +			at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> > +					      QCA8327_DEBUG_MANU_CTRL_EN,
> > +					      QCA8327_DEBUG_MANU_CTRL_EN);
> > +	} else {
> > +		/* Reset DAC Amplitude adjustment */
> > +		at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> > +				      QCA8327_DEBUG_MANU_CTRL_EN, 0);
> 
> Here you don't make it conditional on QCA8327_A_PHY_ID and
> QCA8327_B_PHY_ID, where as above you do?
>
>  	  Andrew

We skip the DAC Amplitude for 8337. We support 8327 a/b and 8337.
Anyway sending this and other patch to v2 with the asked changes.

-- 
	Ansuel

[net-next PATCH 03/13] drivers: net: phy: at803x: enable prefer master for 83xx internal phy

[Ansuel Smith]

From original QCA source code the port was set to prefer master as port
type in 1000BASE-T mode. Apply the same settings also here.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 71237a4e85a3..851d47b8a331 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -1325,6 +1325,9 @@ static int qca83xx_config_init(struct phy_device *phydev)
 		at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
 				      QCA8327_DEBUG_MANU_CTRL_EN, 0);
 
+	/* Following original QCA sourcecode set port to prefer master */
+	phy_set_bits(phydev, MII_CTRL1000, CTL1000_PREFER_MASTER);
+
 	return 0;
 }
 
-- 
2.32.0

Re: [net-next PATCH 03/13] drivers: net: phy: at803x: enable prefer master for 83xx internal phy

[Andrew Lunn]

On Thu, Oct 07, 2021 at 12:35:53AM +0200, Ansuel Smith wrote:
> >From original QCA source code the port was set to prefer master as port
> type in 1000BASE-T mode. Apply the same settings also here.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

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

    Andrew

[net-next PATCH 04/13] drivers: net: phy: at803x: better describe debug regs

[Ansuel Smith]

Give a name to known debug regs from Documentation instead of using
unknown hex values.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/phy/at803x.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 851d47b8a331..f40f17a632ad 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -86,12 +86,12 @@
 #define AT803X_PSSR				0x11	/*PHY-Specific Status Register*/
 #define AT803X_PSSR_MR_AN_COMPLETE		0x0200
 
-#define AT803X_DEBUG_REG_0			0x00
+#define AT803X_DEBUG_ANALOG_TEST_CTRL		0x00
 #define QCA8327_DEBUG_MANU_CTRL_EN		BIT(2)
 #define QCA8337_DEBUG_MANU_CTRL_EN		GENMASK(3, 2)
 #define AT803X_DEBUG_RX_CLK_DLY_EN		BIT(15)
 
-#define AT803X_DEBUG_REG_5			0x05
+#define AT803X_DEBUG_SYSTEM_CTRL_MODE		0x05
 #define AT803X_DEBUG_TX_CLK_DLY_EN		BIT(8)
 
 #define AT803X_DEBUG_REG_HIB_CTRL		0x0b
@@ -284,25 +284,25 @@ static int at803x_read_page(struct phy_device *phydev)
 
 static int at803x_enable_rx_delay(struct phy_device *phydev)
 {
-	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, 0,
+	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL, 0,
 				     AT803X_DEBUG_RX_CLK_DLY_EN);
 }
 
 static int at803x_enable_tx_delay(struct phy_device *phydev)
 {
-	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
+	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_SYSTEM_CTRL_MODE, 0,
 				     AT803X_DEBUG_TX_CLK_DLY_EN);
 }
 
 static int at803x_disable_rx_delay(struct phy_device *phydev)
 {
-	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
+	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
 				     AT803X_DEBUG_RX_CLK_DLY_EN, 0);
 }
 
 static int at803x_disable_tx_delay(struct phy_device *phydev)
 {
-	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
+	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_SYSTEM_CTRL_MODE,
 				     AT803X_DEBUG_TX_CLK_DLY_EN, 0);
 }
 
@@ -1300,7 +1300,7 @@ static int qca83xx_config_init(struct phy_device *phydev)
 	switch (switch_revision) {
 	case 1:
 		/* For 100M waveform */
-		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_0, 0x02ea);
+		at803x_debug_reg_write(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL, 0x02ea);
 		/* Turn on Gigabit clock */
 		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_GREEN, 0x68a0);
 		break;
@@ -1311,7 +1311,7 @@ static int qca83xx_config_init(struct phy_device *phydev)
 	case 4:
 		phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_AZ_DEBUG, 0x803f);
 		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_GREEN, 0x6860);
-		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_5, 0x2c46);
+		at803x_debug_reg_write(phydev, AT803X_DEBUG_SYSTEM_CTRL_MODE, 0x2c46);
 		at803x_debug_reg_write(phydev, AT803X_DEBUG_REG_3C, 0x6000);
 		break;
 	}
@@ -1322,7 +1322,7 @@ static int qca83xx_config_init(struct phy_device *phydev)
 	 */
 	if (phydev->drv->phy_id == QCA8327_A_PHY_ID ||
 	    phydev->drv->phy_id == QCA8327_B_PHY_ID)
-		at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
+		at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
 				      QCA8327_DEBUG_MANU_CTRL_EN, 0);
 
 	/* Following original QCA sourcecode set port to prefer master */
@@ -1340,12 +1340,12 @@ static void qca83xx_link_change_notify(struct phy_device *phydev)
 	/* Set DAC Amplitude adjustment to +6% for 100m on link running */
 	if (phydev->state == PHY_RUNNING) {
 		if (phydev->speed == SPEED_100)
-			at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
+			at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
 					      QCA8327_DEBUG_MANU_CTRL_EN,
 					      QCA8327_DEBUG_MANU_CTRL_EN);
 	} else {
 		/* Reset DAC Amplitude adjustment */
-		at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
+		at803x_debug_reg_mask(phydev, AT803X_DEBUG_ANALOG_TEST_CTRL,
 				      QCA8327_DEBUG_MANU_CTRL_EN, 0);
 	}
 }
-- 
2.32.0

Re: [net-next PATCH 04/13] drivers: net: phy: at803x: better describe debug regs

[Andrew Lunn]

On Thu, Oct 07, 2021 at 12:35:54AM +0200, Ansuel Smith wrote:
> Give a name to known debug regs from Documentation instead of using
> unknown hex values.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

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

    Andrew

[net-next PATCH 05/13] net: dsa: qca8k: add mac_power_sel support

[Ansuel Smith]

Add missing mac power sel support needed for some switch that requires
additional setup. ar8327 have a different setup than 8337.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 27 +++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.h |  5 +++++
 2 files changed, 32 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index bda5a9bf4f52..5bce7ac4dea7 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -950,6 +950,29 @@ qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv)
 	return 0;
 }
 
+static int
+qca8k_setup_mac_pwr_sel(struct qca8k_priv *priv)
+{
+	struct device_node *node = priv->dev->of_node;
+	u32 mask = 0;
+	int ret = 0;
+
+	if (of_property_read_bool(node, "qca,rgmii0-1-8v"))
+		mask |= QCA8K_MAC_PWR_RGMII0_1_8V;
+
+	if (of_property_read_bool(node, "qca,rgmii56-1-8v"))
+		mask |= QCA8K_MAC_PWR_RGMII1_1_8V;
+
+	if (mask) {
+		ret = qca8k_rmw(priv, QCA8K_REG_MAC_PWR_SEL,
+				QCA8K_MAC_PWR_RGMII0_1_8V |
+				QCA8K_MAC_PWR_RGMII1_1_8V,
+				mask);
+	}
+
+	return ret;
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
@@ -979,6 +1002,10 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	ret = qca8k_setup_mac_pwr_sel(priv);
+	if (ret)
+		return ret;
+
 	/* Enable CPU Port */
 	ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
 			    QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index ed3b05ad6745..fc7db94cc0c9 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -100,6 +100,11 @@
 #define   QCA8K_SGMII_MODE_CTRL_PHY			(1 << 22)
 #define   QCA8K_SGMII_MODE_CTRL_MAC			(2 << 22)
 
+/* MAC_PWR_SEL registers */
+#define QCA8K_REG_MAC_PWR_SEL				0x0e4
+#define   QCA8K_MAC_PWR_RGMII1_1_8V			BIT(18)
+#define   QCA8K_MAC_PWR_RGMII0_1_8V			BIT(19)
+
 /* EEE control registers */
 #define QCA8K_REG_EEE_CTRL				0x100
 #define  QCA8K_REG_EEE_CTRL_LPI_EN(_i)			((_i + 1) * 2)
-- 
2.32.0

[net-next PATCH 06/13] Documentation: devicetree: net: dsa: qca8k: document rgmii_1_8v bindings

[Ansuel Smith]

Document new qca,rgmii0_1_8v and qca,rgmii56_1_8v needed to setup
mac_pwr_sel register for ar8327 switch.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 Documentation/devicetree/bindings/net/dsa/qca8k.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index 8c73f67c43ca..1f6b7d2f609e 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -13,6 +13,8 @@ Required properties:
 Optional properties:
 
 - reset-gpios: GPIO to be used to reset the whole device
+- qca,rgmii0-1-8v: Set the internal regulator to supply 1.8v for MAC0 port
+- qca,rgmii56-1-8v: Set the internal regulator to supply 1.8v for MAC5/6 port
 
 Subnodes:
 
-- 
2.32.0

Re: [net-next PATCH 06/13] Documentation: devicetree: net: dsa: qca8k: document rgmii_1_8v bindings

[Andrew Lunn]

On Thu, Oct 07, 2021 at 12:35:56AM +0200, Ansuel Smith wrote:
> Document new qca,rgmii0_1_8v and qca,rgmii56_1_8v needed to setup
> mac_pwr_sel register for ar8327 switch.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> index 8c73f67c43ca..1f6b7d2f609e 100644
> --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> @@ -13,6 +13,8 @@ Required properties:
>  Optional properties:
>  
>  - reset-gpios: GPIO to be used to reset the whole device
> +- qca,rgmii0-1-8v: Set the internal regulator to supply 1.8v for MAC0 port
> +- qca,rgmii56-1-8v: Set the internal regulator to supply 1.8v for MAC5/6 port

What is the consumer of these 1.8v? The MACs are normally internal,
the regulators are internal, so why is a DT property needed?

    Andrew

Re: [net-next PATCH 06/13] Documentation: devicetree: net: dsa: qca8k: document rgmii_1_8v bindings

[Ansuel Smith]

On Thu, Oct 07, 2021 at 02:09:36AM +0200, Andrew Lunn wrote:
> On Thu, Oct 07, 2021 at 12:35:56AM +0200, Ansuel Smith wrote:
> > Document new qca,rgmii0_1_8v and qca,rgmii56_1_8v needed to setup
> > mac_pwr_sel register for ar8327 switch.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/net/dsa/qca8k.txt | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > index 8c73f67c43ca..1f6b7d2f609e 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > +++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
> > @@ -13,6 +13,8 @@ Required properties:
> >  Optional properties:
> >  
> >  - reset-gpios: GPIO to be used to reset the whole device
> > +- qca,rgmii0-1-8v: Set the internal regulator to supply 1.8v for MAC0 port
> > +- qca,rgmii56-1-8v: Set the internal regulator to supply 1.8v for MAC5/6 port
> 
> What is the consumer of these 1.8v? The MACs are normally internal,
> the regulators are internal, so why is a DT property needed?
> 
>     Andrew

Only some device require this, with these bit at 0, the internal
regulator provide 1.5v. It's not really a on/off but a toggle of the
different operating voltage. Some device require this and some doesn't.

-- 
	Ansuel

Re: [net-next PATCH 06/13] Documentation: devicetree: net: dsa: qca8k: document rgmii_1_8v bindings

[Andrew Lunn]

> Only some device require this, with these bit at 0, the internal
> regulator provide 1.5v. It's not really a on/off but a toggle of the
> different operating voltage. Some device require this and some doesn't.

Can you provide a list of devices which require it?

    Andrew

Re: [net-next PATCH 06/13] Documentation: devicetree: net: dsa: qca8k: document rgmii_1_8v bindings

[Ansuel Smith]

On Thu, Oct 07, 2021 at 06:47:10PM +0200, Andrew Lunn wrote:
> > Only some device require this, with these bit at 0, the internal
> > regulator provide 1.5v. It's not really a on/off but a toggle of the
> > different operating voltage. Some device require this and some doesn't.
> 
> Can you provide a list of devices which require it?
> 
>     Andrew

one bcm device the cisco meraki mx65 doesn't have them set.
no qca8327 device have them used.

device with qca,rgmii0-1-8v
netgear r7500v2
Askey RT4230W REV6

device with qca,rgmii56-1-8v
NEC Platforms Aterm WG2600HP3
TP-Link Talon AD7200
Qualcomm Technologies, Inc. IPQ8064/AP-148
AP-161
netgear d7800
dev board DB149
Linksys EA8500 WiFi Router
Linksys EA7500 V1 WiFi Router
ASRock G10
Netgear r7500
TP-Link Archer VR2600v
NEC Aterm WG2600HP
Compex WPQ864
Buffalo WXR-2533DHP

device with both enabled:
ZyXEL NBG6817
Netgear r7800

Here is the list I could have missed some user on the ath79 target but I
hope I made a point on how random this is and that we need dedicated
binding for this. Thanks for the review anyway.

-- 
	Ansuel

[net-next PATCH 07/13] net: dsa: qca8k: add support for mac6_exchange, sgmii falling edge

[Ansuel Smith]

Some device set the switch to exchange the mac0 port with mac6 port. Add
support for this in the qca8k driver. Also add support for SGMII rx/tx
clock falling edge. This is only present for pad0, pad5 and pad6 have
these bit reserved from Documentation.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
---
 drivers/net/dsa/qca8k.c | 33 +++++++++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.h |  3 +++
 2 files changed, 36 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 5bce7ac4dea7..3a040a3ed58e 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -973,6 +973,34 @@ qca8k_setup_mac_pwr_sel(struct qca8k_priv *priv)
 	return ret;
 }
 
+static int
+qca8k_setup_port0_pad_ctrl_reg(struct qca8k_priv *priv)
+{
+	struct device_node *node = priv->dev->of_node;
+	u32 mask = 0;
+	int ret = 0;
+
+	/* Swap MAC0-MAC6 */
+	if (of_property_read_bool(node, "qca,mac6-exchange"))
+		mask |= QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG;
+
+	/* SGMII Clock phase configuration */
+	if (of_property_read_bool(node, "qca,sgmii-rxclk-falling-edge"))
+		mask |= QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE;
+
+	if (of_property_read_bool(node, "qca,sgmii-txclk-falling-edge"))
+		mask |= QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE;
+
+	if (mask)
+		ret = qca8k_rmw(priv, QCA8K_REG_PORT0_PAD_CTRL,
+				QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG |
+				QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE |
+				QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE,
+				mask);
+
+	return ret;
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
@@ -1006,6 +1034,11 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	/* Configure additional PORT0_PAD_CTRL properties */
+	ret = qca8k_setup_port0_pad_ctrl_reg(priv);
+	if (ret)
+		return ret;
+
 	/* Enable CPU Port */
 	ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
 			    QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index fc7db94cc0c9..3fded69a6839 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -35,6 +35,9 @@
 #define   QCA8K_MASK_CTRL_DEVICE_ID_MASK		GENMASK(15, 8)
 #define   QCA8K_MASK_CTRL_DEVICE_ID(x)			((x) >> 8)
 #define QCA8K_REG_PORT0_PAD_CTRL			0x004
+#define   QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG		BIT(31)
+#define   QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE	BIT(19)
+#define   QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE	BIT(18)
 #define QCA8K_REG_PORT5_PAD_CTRL			0x008
 #define QCA8K_REG_PORT6_PAD_CTRL			0x00c
 #define   QCA8K_PORT_PAD_RGMII_EN			BIT(26)
-- 
2.32.0

Re: [net-next PATCH 07/13] net: dsa: qca8k: add support for mac6_exchange, sgmii falling edge

[Andrew Lunn]

On Thu, Oct 07, 2021 at 12:35:57AM +0200, Ansuel Smith wrote:
> Some device set the switch to exchange the mac0 port with mac6 port. Add
> support for this in the qca8k driver. Also add support for SGMII rx/tx
> clock falling edge. This is only present for pad0, pad5 and pad6 have
> these bit reserved from Documentation.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>

Who wrote this patch? The person submitting it should be last. If
Matthew actually wrote it, you want to play with git commit --author=
to set the correct author.

   Andrew

Re: [net-next PATCH 07/13] net: dsa: qca8k: add support for mac6_exchange, sgmii falling edge

[Ansuel Smith]

On Thu, Oct 07, 2021 at 02:14:18AM +0200, Andrew Lunn wrote:
> On Thu, Oct 07, 2021 at 12:35:57AM +0200, Ansuel Smith wrote:
> > Some device set the switch to exchange the mac0 port with mac6 port. Add
> > support for this in the qca8k driver. Also add support for SGMII rx/tx
> > clock falling edge. This is only present for pad0, pad5 and pad6 have
> > these bit reserved from Documentation.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> 
> Who wrote this patch? The person submitting it should be last. If
> Matthew actually wrote it, you want to play with git commit --author=
> to set the correct author.
> 
>    Andrew

I wrote it and Matthew did some very minor changes (binding name).
Should I use co-developed by ?

-- 
	Ansuel

Re: [net-next PATCH 07/13] net: dsa: qca8k: add support for mac6_exchange, sgmii falling edge

[Andrew Lunn]

On Thu, Oct 07, 2021 at 03:26:53PM +0200, Ansuel Smith wrote:
> On Thu, Oct 07, 2021 at 02:14:18AM +0200, Andrew Lunn wrote:
> > On Thu, Oct 07, 2021 at 12:35:57AM +0200, Ansuel Smith wrote:
> > > Some device set the switch to exchange the mac0 port with mac6 port. Add
> > > support for this in the qca8k driver. Also add support for SGMII rx/tx
> > > clock falling edge. This is only present for pad0, pad5 and pad6 have
> > > these bit reserved from Documentation.
> > > 
> > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > > Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> > 
> > Who wrote this patch? The person submitting it should be last. If
> > Matthew actually wrote it, you want to play with git commit --author=
> > to set the correct author.
> > 
> >    Andrew
> 
> I wrote it and Matthew did some very minor changes (binding name).
> Should I use co-developed by ?

In that case, just reverse the order of the two Signed-off-by, and
leave the author information as you.

      Andrew

Re: [net-next PATCH 07/13] net: dsa: qca8k: add support for mac6_exchange, sgmii falling edge

[Ansuel Smith]

On Thu, Oct 07, 2021 at 06:49:06PM +0200, Andrew Lunn wrote:
> On Thu, Oct 07, 2021 at 03:26:53PM +0200, Ansuel Smith wrote:
> > On Thu, Oct 07, 2021 at 02:14:18AM +0200, Andrew Lunn wrote:
> > > On Thu, Oct 07, 2021 at 12:35:57AM +0200, Ansuel Smith wrote:
> > > > Some device set the switch to exchange the mac0 port with mac6 port. Add
> > > > support for this in the qca8k driver. Also add support for SGMII rx/tx
> > > > clock falling edge. This is only present for pad0, pad5 and pad6 have
> > > > these bit reserved from Documentation.
> > > > 
> > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > > > Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> > > 
> > > Who wrote this patch? The person submitting it should be last. If
> > > Matthew actually wrote it, you want to play with git commit --author=
> > > to set the correct author.
> > > 
> > >    Andrew
> > 
> > I wrote it and Matthew did some very minor changes (binding name).
> > Should I use co-developed by ?
> 
> In that case, just reverse the order of the two Signed-off-by, and
> leave the author information as you.
> 
>       Andrew

Ok will fix in v2.

-- 
	Ansuel

Re: [net-next PATCH 07/13] net: dsa: qca8k: add support for mac6_exchange, sgmii falling edge

[Vladimir Oltean]

On Thu, Oct 07, 2021 at 12:35:57AM +0200, Ansuel Smith wrote:
> Some device set the switch to exchange the mac0 port with mac6 port. Add
> support for this in the qca8k driver. Also add support for SGMII rx/tx
> clock falling edge. This is only present for pad0, pad5 and pad6 have
> these bit reserved from Documentation.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 33 +++++++++++++++++++++++++++++++++
>  drivers/net/dsa/qca8k.h |  3 +++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 5bce7ac4dea7..3a040a3ed58e 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -973,6 +973,34 @@ qca8k_setup_mac_pwr_sel(struct qca8k_priv *priv)
>  	return ret;
>  }
>  
> +static int
> +qca8k_setup_port0_pad_ctrl_reg(struct qca8k_priv *priv)
> +{
> +	struct device_node *node = priv->dev->of_node;
> +	u32 mask = 0;
> +	int ret = 0;
> +
> +	/* Swap MAC0-MAC6 */
> +	if (of_property_read_bool(node, "qca,mac6-exchange"))
> +		mask |= QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG;
> +
> +	/* SGMII Clock phase configuration */
> +	if (of_property_read_bool(node, "qca,sgmii-rxclk-falling-edge"))
> +		mask |= QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE;
> +
> +	if (of_property_read_bool(node, "qca,sgmii-txclk-falling-edge"))
> +		mask |= QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE;
> +
> +	if (mask)
> +		ret = qca8k_rmw(priv, QCA8K_REG_PORT0_PAD_CTRL,
> +				QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG |
> +				QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE |
> +				QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE,
> +				mask);
> +
> +	return ret;
> +}
> +
>  static int
>  qca8k_setup(struct dsa_switch *ds)
>  {
> @@ -1006,6 +1034,11 @@ qca8k_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> +	/* Configure additional PORT0_PAD_CTRL properties */
> +	ret = qca8k_setup_port0_pad_ctrl_reg(priv);
> +	if (ret)
> +		return ret;
> +
>  	/* Enable CPU Port */
>  	ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
>  			    QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index fc7db94cc0c9..3fded69a6839 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -35,6 +35,9 @@
>  #define   QCA8K_MASK_CTRL_DEVICE_ID_MASK		GENMASK(15, 8)
>  #define   QCA8K_MASK_CTRL_DEVICE_ID(x)			((x) >> 8)
>  #define QCA8K_REG_PORT0_PAD_CTRL			0x004
> +#define   QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG		BIT(31)

Where can I find more information about this mac0/mac6 exchange? In this
document for ar8327, bit 31 of PORT0 PAD MODE CTRL is reserved.
http://www.datasheet.es/PDF/771154/AR8327-pdf.html

> +#define   QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE	BIT(19)
> +#define   QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE	BIT(18)
>  #define QCA8K_REG_PORT5_PAD_CTRL			0x008
>  #define QCA8K_REG_PORT6_PAD_CTRL			0x00c
>  #define   QCA8K_PORT_PAD_RGMII_EN			BIT(26)
> -- 
> 2.32.0
> 

Re: [net-next PATCH 07/13] net: dsa: qca8k: add support for mac6_exchange, sgmii falling edge

[Ansuel Smith]

On Sun, Oct 10, 2021 at 02:40:37PM +0300, Vladimir Oltean wrote:
> On Thu, Oct 07, 2021 at 12:35:57AM +0200, Ansuel Smith wrote:
> > Some device set the switch to exchange the mac0 port with mac6 port. Add
> > support for this in the qca8k driver. Also add support for SGMII rx/tx
> > clock falling edge. This is only present for pad0, pad5 and pad6 have
> > these bit reserved from Documentation.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
> > ---
> >  drivers/net/dsa/qca8k.c | 33 +++++++++++++++++++++++++++++++++
> >  drivers/net/dsa/qca8k.h |  3 +++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 5bce7ac4dea7..3a040a3ed58e 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -973,6 +973,34 @@ qca8k_setup_mac_pwr_sel(struct qca8k_priv *priv)
> >  	return ret;
> >  }
> >  
> > +static int
> > +qca8k_setup_port0_pad_ctrl_reg(struct qca8k_priv *priv)
> > +{
> > +	struct device_node *node = priv->dev->of_node;
> > +	u32 mask = 0;
> > +	int ret = 0;
> > +
> > +	/* Swap MAC0-MAC6 */
> > +	if (of_property_read_bool(node, "qca,mac6-exchange"))
> > +		mask |= QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG;
> > +
> > +	/* SGMII Clock phase configuration */
> > +	if (of_property_read_bool(node, "qca,sgmii-rxclk-falling-edge"))
> > +		mask |= QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE;
> > +
> > +	if (of_property_read_bool(node, "qca,sgmii-txclk-falling-edge"))
> > +		mask |= QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE;
> > +
> > +	if (mask)
> > +		ret = qca8k_rmw(priv, QCA8K_REG_PORT0_PAD_CTRL,
> > +				QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG |
> > +				QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE |
> > +				QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE,
> > +				mask);
> > +
> > +	return ret;
> > +}
> > +
> >  static int
> >  qca8k_setup(struct dsa_switch *ds)
> >  {
> > @@ -1006,6 +1034,11 @@ qca8k_setup(struct dsa_switch *ds)
> >  	if (ret)
> >  		return ret;
> >  
> > +	/* Configure additional PORT0_PAD_CTRL properties */
> > +	ret = qca8k_setup_port0_pad_ctrl_reg(priv);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* Enable CPU Port */
> >  	ret = qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
> >  			    QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index fc7db94cc0c9..3fded69a6839 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -35,6 +35,9 @@
> >  #define   QCA8K_MASK_CTRL_DEVICE_ID_MASK		GENMASK(15, 8)
> >  #define   QCA8K_MASK_CTRL_DEVICE_ID(x)			((x) >> 8)
> >  #define QCA8K_REG_PORT0_PAD_CTRL			0x004
> > +#define   QCA8K_PORT0_PAD_CTRL_MAC06_EXCHG		BIT(31)
> 
> Where can I find more information about this mac0/mac6 exchange? In this
> document for ar8327, bit 31 of PORT0 PAD MODE CTRL is reserved.
> http://www.datasheet.es/PDF/771154/AR8327-pdf.html
>

The documentation for mac06 exchange is in qca8337.
https://github.com/Deoptim/atheros/blob/master/QCA8337-datasheet.pdf
We have this theory that the bit is just reserved in qca8327 due to bad
documentation/qca hiding special stuff. But you could be right and 06
exchange is just not supported on qca8327.
We tought it was like that as many wiki report uncorrect switch in the
devices and by inspecting the real internal photo they were actually
qca8337 instead of qca8327. From initial check it seems that we actually
don't have device with qca8327 and that use mac06 exchange. (We had 8
before and they are all report the wrong switch in the wiki)

> > +#define   QCA8K_PORT0_PAD_SGMII_RXCLK_FALLING_EDGE	BIT(19)
> > +#define   QCA8K_PORT0_PAD_SGMII_TXCLK_FALLING_EDGE	BIT(18)
> >  #define QCA8K_REG_PORT5_PAD_CTRL			0x008
> >  #define QCA8K_REG_PORT6_PAD_CTRL			0x00c
> >  #define   QCA8K_PORT_PAD_RGMII_EN			BIT(26)
> > -- 
> > 2.32.0
> > 

-- 
	Ansuel

[net-next PATCH 08/13] dt-bindings: net: dsa: qca8k: Add MAC swap and clock phase properties

[Ansuel Smith]

Add names and decriptions of additional PORT0_PAD_CTRL properties.
Document new binding qca,mac6_exchange that exchange the mac0 port
with mac6.
qca,sgmii-(rx|tx)clk-falling-edge are for setting the respective clock
phase to failling edge.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Signed-off-by: Matthew Hagan <mnhagan88@gmail.com>
---
 Documentation/devicetree/bindings/net/dsa/qca8k.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index 1f6b7d2f609e..780d1e60c425 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -13,6 +13,9 @@ Required properties:
 Optional properties:
 
 - reset-gpios: GPIO to be used to reset the whole device
+- qca,mac6-exchange: Internally swap MAC0 and MAC6.
+- qca,sgmii-rxclk-falling-edge: Set the receive clock phase to falling edge.
+- qca,sgmii-txclk-falling-edge: Set the transmit clock phase to falling edge.
 - qca,rgmii0-1-8v: Set the internal regulator to supply 1.8v for MAC0 port
 - qca,rgmii56-1-8v: Set the internal regulator to supply 1.8v for MAC5/6 port
 
-- 
2.32.0

[net-next PATCH 09/13] net: dsa: qca8k: check rgmii also on port 6 if exchanged

[Ansuel Smith]

Port 0 can be exchanged with port6. Handle this special case by also
checking the port6 if present.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 3a040a3ed58e..4d4f23f7f948 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -906,7 +906,20 @@ qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv)
 	if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
 	    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
 	    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
-		return 0;
+		/* Port 0 can be exchanged with port 6 */
+		dp = dsa_to_port(priv->ds, 6);
+		if (!dp)
+			return 0;
+
+		port_dn = dp->dn;
+
+		/* Check if port 6 is set to the correct type */
+		of_get_phy_mode(port_dn, &mode);
+		if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
+		    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
+		    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
+			return 0;
+		}
 	}
 
 	switch (mode) {
-- 
2.32.0

Re: [net-next PATCH 09/13] net: dsa: qca8k: check rgmii also on port 6 if exchanged

[Andrew Lunn]

On Thu, Oct 07, 2021 at 12:35:59AM +0200, Ansuel Smith wrote:
> Port 0 can be exchanged with port6. Handle this special case by also
> checking the port6 if present.

This is messy.

The DSA core has no idea the ports have been swapped, so the interface
names are going to be taken from DT unswaped. Now you appear to be
taking phy-mode from the other port in DT. That is inconsistent. All
the configuration for a port should come from the same place, nothing
gets swapped. Or everything needs to swap, which means you need to
change the DSA core.

    Andrew

Re: [net-next PATCH 09/13] net: dsa: qca8k: check rgmii also on port 6 if exchanged

[Ansuel Smith]

On Thu, Oct 07, 2021 at 02:24:08AM +0200, Andrew Lunn wrote:
> On Thu, Oct 07, 2021 at 12:35:59AM +0200, Ansuel Smith wrote:
> > Port 0 can be exchanged with port6. Handle this special case by also
> > checking the port6 if present.
> 
> This is messy.
> 
> The DSA core has no idea the ports have been swapped, so the interface
> names are going to be taken from DT unswaped. Now you appear to be
> taking phy-mode from the other port in DT. That is inconsistent. All
> the configuration for a port should come from the same place, nothing
> gets swapped. Or everything needs to swap, which means you need to
> change the DSA core.
> 
>     Andrew

The swap is internal. So from the dts side we still use port0 as port0,
it's just swapped internally in the switch.
The change here is required as this scan the rgmii delay and sets the
value to be set later in the phylink mac config.
We currently assume that only one cpu port is supported and that can be
sgmii or rgmii. This specific switch have 2 cpu port and we can have one
config with cpu port0 set to sgmii and cpu port6 set to rgmii-id.
This patch is to address this and to add the delay function to scan also
for the secondary cpu port. (again the real value will be set in the mac
config function)
Honestly i think we should just rework this and move the delay logic
directly in the mac_config function and scan there directly. What do you
think? That way we should be able to generalize this and drop the extra
if.

-- 
	Ansuel

Re: [net-next PATCH 09/13] net: dsa: qca8k: check rgmii also on port 6 if exchanged

[Florian Fainelli]

On 10/7/21 6:31 AM, Ansuel Smith wrote:
> On Thu, Oct 07, 2021 at 02:24:08AM +0200, Andrew Lunn wrote:
>> On Thu, Oct 07, 2021 at 12:35:59AM +0200, Ansuel Smith wrote:
>>> Port 0 can be exchanged with port6. Handle this special case by also
>>> checking the port6 if present.
>>
>> This is messy.
>>
>> The DSA core has no idea the ports have been swapped, so the interface
>> names are going to be taken from DT unswaped. Now you appear to be
>> taking phy-mode from the other port in DT. That is inconsistent. All
>> the configuration for a port should come from the same place, nothing
>> gets swapped. Or everything needs to swap, which means you need to
>> change the DSA core.
>>
>>     Andrew
> 
> The swap is internal. So from the dts side we still use port0 as port0,
> it's just swapped internally in the switch.
> The change here is required as this scan the rgmii delay and sets the
> value to be set later in the phylink mac config.
> We currently assume that only one cpu port is supported and that can be
> sgmii or rgmii. This specific switch have 2 cpu port and we can have one
> config with cpu port0 set to sgmii and cpu port6 set to rgmii-id.
> This patch is to address this and to add the delay function to scan also
> for the secondary cpu port. (again the real value will be set in the mac
> config function)
> Honestly i think we should just rework this and move the delay logic
> directly in the mac_config function and scan there directly. What do you
> think? That way we should be able to generalize this and drop the extra
> if.

Agreed, the whole port swapping business is really hairy and seems like
it will led to unpleasant surprises down the road.
-- 
Florian

[net-next PATCH 10/13] net: dsa: qca8k: add explicit SGMII PLL enable

[Ansuel Smith]

Support enabling PLL on the SGMII CPU port. Some device require this
special configuration or no traffic is transmitted and the switch
doesn't work at all. A dedicated binding is added to the CPU node
port to apply the correct reg on mac config.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 4d4f23f7f948..01b05dfeae2b 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1214,6 +1214,7 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			 const struct phylink_link_state *state)
 {
 	struct qca8k_priv *priv = ds->priv;
+	struct dsa_port *dp;
 	u32 reg, val;
 	int ret;
 
@@ -1282,6 +1283,8 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		break;
 	case PHY_INTERFACE_MODE_SGMII:
 	case PHY_INTERFACE_MODE_1000BASEX:
+		dp = dsa_to_port(ds, port);
+
 		/* Enable SGMII on the port */
 		qca8k_write(priv, reg, QCA8K_PORT_PAD_SGMII_EN);
 
@@ -1300,8 +1303,11 @@ qca8k_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		if (ret)
 			return;
 
-		val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
-			QCA8K_SGMII_EN_TX | QCA8K_SGMII_EN_SD;
+		val |= QCA8K_SGMII_EN_SD;
+
+		if (of_property_read_bool(dp->dn, "qca,sgmii-enable-pll"))
+			val |= QCA8K_SGMII_EN_PLL | QCA8K_SGMII_EN_RX |
+			       QCA8K_SGMII_EN_TX;
 
 		if (dsa_is_cpu_port(ds, port)) {
 			/* CPU port, we're talking to the CPU MAC, be a PHY */
-- 
2.32.0

Re: [net-next PATCH 10/13] net: dsa: qca8k: add explicit SGMII PLL enable

[Andrew Lunn]

On Thu, Oct 07, 2021 at 12:36:00AM +0200, Ansuel Smith wrote:
> Support enabling PLL on the SGMII CPU port. Some device require this
> special configuration or no traffic is transmitted and the switch
> doesn't work at all. A dedicated binding is added to the CPU node
> port to apply the correct reg on mac config.

Why not just enable this all the time when the CPU port is in SGMII
mode?

Is it also needed for 1000BaseX?

DT properties like this are hard to use. It would be better if the
switch can decide for itself if it needs the PLL enabled.

       Andrew

Re: [net-next PATCH 10/13] net: dsa: qca8k: add explicit SGMII PLL enable

[Ansuel Smith]

On Thu, Oct 07, 2021 at 02:29:46AM +0200, Andrew Lunn wrote:
> On Thu, Oct 07, 2021 at 12:36:00AM +0200, Ansuel Smith wrote:
> > Support enabling PLL on the SGMII CPU port. Some device require this
> > special configuration or no traffic is transmitted and the switch
> > doesn't work at all. A dedicated binding is added to the CPU node
> > port to apply the correct reg on mac config.
> 
> Why not just enable this all the time when the CPU port is in SGMII
> mode?

I don't know if you missed the cover letter with the reason. Sgmii PLL
is a mess. Some device needs it and some doesn't. With a wrong
configuration the result is not traffic. As it's all messy we decided to
set the PLL to be enabled with a dedicated binding and set it disabled
by default. We enouncer more device that require it disabled than device
that needs it enabled. (in the order of 70 that doesn't needed it and 2
that requires it enabled or port instability/no traffic/leds problem)

> 
> Is it also needed for 1000BaseX?
> 

We assume it really depends on the device.

> DT properties like this are hard to use. It would be better if the
> switch can decide for itself if it needs the PLL enabled.
> 

Again reason in the cover letter sgmii part. Some qca driver have some
logic based on switch revision. We tried that and it didn't work since
some device had no traffic with pll enabled (and with the revision set
to enable pll)

>        Andrew

-- 
	Ansuel

Re: [net-next PATCH 10/13] net: dsa: qca8k: add explicit SGMII PLL enable

[Andrew Lunn]

On Thu, Oct 07, 2021 at 03:35:54PM +0200, Ansuel Smith wrote:
> On Thu, Oct 07, 2021 at 02:29:46AM +0200, Andrew Lunn wrote:
> > On Thu, Oct 07, 2021 at 12:36:00AM +0200, Ansuel Smith wrote:
> > > Support enabling PLL on the SGMII CPU port. Some device require this
> > > special configuration or no traffic is transmitted and the switch
> > > doesn't work at all. A dedicated binding is added to the CPU node
> > > port to apply the correct reg on mac config.
> > 
> > Why not just enable this all the time when the CPU port is in SGMII
> > mode?
> 
> I don't know if you missed the cover letter with the reason. Sgmii PLL
> is a mess. Some device needs it and some doesn't. With a wrong
> configuration the result is not traffic. As it's all messy we decided to
> set the PLL to be enabled with a dedicated binding and set it disabled
> by default. We enouncer more device that require it disabled than device
> that needs it enabled. (in the order of 70 that doesn't needed it and 2
> that requires it enabled or port instability/no traffic/leds problem)

What exactly does this PLL do? Clock recovery of the SGMII clock, and
then using it in the opposite direction? What combinations of PHYs
need it, and which don't?

> > Is it also needed for 1000BaseX?
> > 
> 
> We assume it really depends on the device.

That i find surprising. 1000BaseX and SGMII are very similar. I would
expect a device with requires the PLL enabled for SGMII also needs it
for 1000BaseX.

> > DT properties like this are hard to use. It would be better if the
> > switch can decide for itself if it needs the PLL enabled.
> 
> Again reason in the cover letter sgmii part. Some qca driver have some
> logic based on switch revision. We tried that and it didn't work since
> some device had no traffic with pll enabled (and with the revision set
> to enable pll)

This is my main problem with this patchset. You are adding lots of
poorly documented properties which are proprietary to this switch. And
you are saying, please try all 2^N combinations and see what works
best. That is not very friendly at all.

So it would be good to explain each one in detail. Maybe given the
explanation, we can figure out a way to detect at runtime, and not
need the option. If not, you can add it to the DT binding to help
somebody pick a likely starting point for the 2^N search.

	 Andrew

Re: [net-next PATCH 10/13] net: dsa: qca8k: add explicit SGMII PLL enable

[Ansuel Smith]

On Thu, Oct 07, 2021 at 08:05:56PM +0200, Andrew Lunn wrote:
> 
> On Thu, Oct 07, 2021 at 03:35:54PM +0200, Ansuel Smith wrote:
> > On Thu, Oct 07, 2021 at 02:29:46AM +0200, Andrew Lunn wrote:
> > > On Thu, Oct 07, 2021 at 12:36:00AM +0200, Ansuel Smith wrote:
> > > > Support enabling PLL on the SGMII CPU port. Some device require this
> > > > special configuration or no traffic is transmitted and the switch
> > > > doesn't work at all. A dedicated binding is added to the CPU node
> > > > port to apply the correct reg on mac config.
> > > 
> > > Why not just enable this all the time when the CPU port is in SGMII
> > > mode?
> > 
> > I don't know if you missed the cover letter with the reason. Sgmii PLL
> > is a mess. Some device needs it and some doesn't. With a wrong
> > configuration the result is not traffic. As it's all messy we decided to
> > set the PLL to be enabled with a dedicated binding and set it disabled
> > by default. We enouncer more device that require it disabled than device
> > that needs it enabled. (in the order of 70 that doesn't needed it and 2
> > that requires it enabled or port instability/no traffic/leds problem)
> 
> What exactly does this PLL do? Clock recovery of the SGMII clock, and
> then using it in the opposite direction? What combinations of PHYs
> need it, and which don't?
>

I will quote from Documentation
bit 1: enable SGMII PLL (it's disabled by default)
bit 2: enable rx chain. By default it's disabled and CLK125M_RX and
DOUT_RX can be any logic of 1 or 0
bit 3: enable TX driver. By default is in idle and kept in 900mV
As you can see normally all these bit are disabled and I think forcing
them on seems wrong.
We can think about setting it based on the switch type and revision but
again we found some that needed it anyway and goes out of any logic.
The original idea is to add a biding to force the setting and make the
driver decide itself the correct option. But considering we found that
in 90% of the case, this is not needed, the switch default doesn't
enable it and that only 2-3 device require it, I see the binding the
only way to implement this and do not pollute the code with tons of
condition.

> > > Is it also needed for 1000BaseX?
> > > 
> > 
> > We assume it really depends on the device.
> 
> That i find surprising. 1000BaseX and SGMII are very similar. I would
> expect a device with requires the PLL enabled for SGMII also needs it
> for 1000BaseX.
> 

With assume I mean we have no device with 1000BaseX (and I honestly
don't know if it does exist) I think if it does require PLL for sgmii
then it does require it also for 1000BaseX

> > > DT properties like this are hard to use. It would be better if the
> > > switch can decide for itself if it needs the PLL enabled.
> > 
> > Again reason in the cover letter sgmii part. Some qca driver have some
> > logic based on switch revision. We tried that and it didn't work since
> > some device had no traffic with pll enabled (and with the revision set
> > to enable pll)
> 
> This is my main problem with this patchset. You are adding lots of
> poorly documented properties which are proprietary to this switch. And
> you are saying, please try all 2^N combinations and see what works
> best. That is not very friendly at all.

We have many device and we notice a common config for both qca8327 and
qca8337. Wonder if this would be improved by adding some hint in the
documentation on the correct configuration based on the switch model?
We waited so much to push this just to check if all this bad stuff was
actually needed or could be implemented differently, but we really find
any logic. So I think the only way is ""spam"" the documentation with
data on how to decide the correct option.

> 
> So it would be good to explain each one in detail. Maybe given the
> explanation, we can figure out a way to detect at runtime, and not
> need the option. If not, you can add it to the DT binding to help
> somebody pick a likely starting point for the 2^N search.
> 
> 	 Andrew

-- 
	Ansuel

[net-next PATCH 11/13] devicetree: net: dsa: qca8k: Document qca,sgmii-enable-pll

[Ansuel Smith]

Document qca,sgmii-enable-pll binding used in the CPU nodes to
enable SGMII PLL on MAC config.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 Documentation/devicetree/bindings/net/dsa/qca8k.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index 780d1e60c425..550f4313c6e0 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -42,6 +42,8 @@ A CPU port node has the following optional node:
                           managed entity. See
                           Documentation/devicetree/bindings/net/fixed-link.txt
                           for details.
+- qca,sgmii-enable-pll  : For SGMII CPU port, explicitly enable PLL, TX and RX
+                          chain along with Signal Detection.
 
 For QCA8K the 'fixed-link' sub-node supports only the following properties:
 
-- 
2.32.0

Re: [net-next PATCH 11/13] devicetree: net: dsa: qca8k: Document qca,sgmii-enable-pll

[Andrew Lunn]

> +- qca,sgmii-enable-pll  : For SGMII CPU port, explicitly enable PLL, TX and RX
> +                          chain along with Signal Detection.

Continuing on with the comment in the previous post. You might want to
give a hit when this is needed.

     Andrew

Re: [net-next PATCH 11/13] devicetree: net: dsa: qca8k: Document qca,sgmii-enable-pll

[Ansuel Smith]

On Thu, Oct 07, 2021 at 02:31:37AM +0200, Andrew Lunn wrote:
> > +- qca,sgmii-enable-pll  : For SGMII CPU port, explicitly enable PLL, TX and RX
> > +                          chain along with Signal Detection.
> 
> Continuing on with the comment in the previous post. You might want to
> give a hit when this is needed.
> 
>      Andrew

Ok I can put here all the finding with the common configuration found
with qca8327 and qca8337 so someone can try to guess the correct value.

-- 
	Ansuel

[net-next PATCH 12/13] drivers: net: dsa: qca8k: add support for pws config reg

[Ansuel Smith]

Some qca8327 switch require the power_on_sel enabled for the pws_reg or
sets the led to open drain.
Also qca8327 switch have a special mode to declare reduced 48pin layout.
This special mode is only present in qca8327 as qca8337 have a different
pws_reg table.
Introduce a new binding and support these special configuration.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 26 ++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.h |  3 +++
 2 files changed, 29 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 01b05dfeae2b..209f8d3c9ea8 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -1014,6 +1014,28 @@ qca8k_setup_port0_pad_ctrl_reg(struct qca8k_priv *priv)
 	return ret;
 }
 
+static int
+qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
+{
+	struct device_node *node = priv->dev->of_node;
+	u32 val = 0;
+
+	if (priv->switch_id == QCA8K_ID_QCA8327)
+		if (of_property_read_bool(node, "qca,package48"))
+			val |= QCA8327_PWS_PACKAGE48_EN;
+
+	if (of_property_read_bool(node, "qca,power-on-sel"))
+		val |= QCA8K_PWS_POWER_ON_SEL;
+
+	if (of_property_read_bool(node, "qca,led-open-drain"))
+		/* POWER_ON_SEL needs to be set when configuring led to open drain */
+		val |= QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL;
+
+	return qca8k_rmw(priv, QCA8K_REG_PWS,
+			QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL,
+			val);
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
@@ -1039,6 +1061,10 @@ qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	ret = qca8k_setup_of_pws_reg(priv);
+	if (ret)
+		return ret;
+
 	ret = qca8k_setup_of_rgmii_delay(priv);
 	if (ret)
 		return ret;
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 3fded69a6839..90f4616c33f1 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -48,6 +48,9 @@
 #define   QCA8K_MAX_DELAY				3
 #define   QCA8K_PORT_PAD_SGMII_EN			BIT(7)
 #define QCA8K_REG_PWS					0x010
+#define   QCA8K_PWS_POWER_ON_SEL			BIT(31)
+#define   QCA8327_PWS_PACKAGE48_EN			BIT(30)
+#define   QCA8K_PWS_LED_OPEN_EN_CSR			BIT(24)
 #define   QCA8K_PWS_SERDES_AEN_DIS			BIT(7)
 #define QCA8K_REG_MODULE_EN				0x030
 #define   QCA8K_MODULE_EN_MIB				BIT(0)
-- 
2.32.0

Re: [net-next PATCH 12/13] drivers: net: dsa: qca8k: add support for pws config reg

[Andrew Lunn]

> +static int
> +qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
> +{
> +	struct device_node *node = priv->dev->of_node;
> +	u32 val = 0;
> +
> +	if (priv->switch_id == QCA8K_ID_QCA8327)
> +		if (of_property_read_bool(node, "qca,package48"))
> +			val |= QCA8327_PWS_PACKAGE48_EN;

What does this actually do? How is PACKAGE48 different to normal mode?

> +
> +	if (of_property_read_bool(node, "qca,power-on-sel"))
> +		val |= QCA8K_PWS_POWER_ON_SEL;

What happens if you unconditionally do this? Why is a DT property
required?

> +
> +	if (of_property_read_bool(node, "qca,led-open-drain"))
> +		/* POWER_ON_SEL needs to be set when configuring led to open drain */
> +		val |= QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL;

This is getting into territory of adding LED support for PHYs, which
we want to do via the LED subsystem.

   Andrew

Re: [net-next PATCH 12/13] drivers: net: dsa: qca8k: add support for pws config reg

[Ansuel Smith]

On Thu, Oct 07, 2021 at 02:41:10AM +0200, Andrew Lunn wrote:
> > +static int
> > +qca8k_setup_of_pws_reg(struct qca8k_priv *priv)
> > +{
> > +	struct device_node *node = priv->dev->of_node;
> > +	u32 val = 0;
> > +
> > +	if (priv->switch_id == QCA8K_ID_QCA8327)
> > +		if (of_property_read_bool(node, "qca,package48"))
> > +			val |= QCA8327_PWS_PACKAGE48_EN;
> 
> What does this actually do? How is PACKAGE48 different to normal mode?
>

I actually made a typo.
Anyway the difference is that they made 2 different package version of
the qca8327. One with 176 pin and one with 148 pin. Setting the wrong
layout cause the switch malfunction (no traffic, we found this on one
xiaomi device). This is from Documenation and it does toggle the MAC
interface configuration for the 2 different package.

> > +
> > +	if (of_property_read_bool(node, "qca,power-on-sel"))
> > +		val |= QCA8K_PWS_POWER_ON_SEL;
> 
> What happens if you unconditionally do this? Why is a DT property
> required?
> 

This is needed to bypass the power on strapping and use the regs config.
The switch can use hardware pin to set eeprom presence and leds open
drain. Setting this bit on bypass the hardware strapping and sets these
2 thing based on the regs. We didn't add the eeprom binding as we didn't
find any switch using it and we don't have any support for it.

> > +
> > +	if (of_property_read_bool(node, "qca,led-open-drain"))
> > +		/* POWER_ON_SEL needs to be set when configuring led to open drain */
> > +		val |= QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL;
> 
> This is getting into territory of adding LED support for PHYs, which
> we want to do via the LED subsystem.
> 

Don't know if it would be the correct way. Without this the switch leds
chaese to work. I think this should be handled in a dedicated way than
defined in a binding in the leds configuration. But I could be wrong.

>    Andrew

-- 
	Ansuel

Re: [net-next PATCH 12/13] drivers: net: dsa: qca8k: add support for pws config reg

[Andrew Lunn]

> Anyway the difference is that they made 2 different package version of
> the qca8327. One with 176 pin and one with 148 pin.

I assume they have different product numbers. So you can quote them in
the DT binding? Are they BGP or QFP? Can somebody easily count the
pins?

> > > +
> > > +	if (of_property_read_bool(node, "qca,power-on-sel"))
> > > +		val |= QCA8K_PWS_POWER_ON_SEL;
> > 
> > What happens if you unconditionally do this? Why is a DT property
> > required?
> > 
> 
> This is needed to bypass the power on strapping and use the regs config.
> The switch can use hardware pin to set eeprom presence and leds open
> drain. Setting this bit on bypass the hardware strapping and sets these
> 2 thing based on the regs.

So first off, it sounds like you have the DT property named wrong. It
should be 'qca,ignore-power-on-sel'.

However, why do you even need this? Generally, strapping gives you the
defaults. Registers get loaded with a value determined by the
strapping. But after that, you can change the value, based on
additional information. Or are you saying the register is read only
when strapping is used?

> > > +	if (of_property_read_bool(node, "qca,led-open-drain"))
> > > +		/* POWER_ON_SEL needs to be set when configuring led to open drain */
> > > +		val |= QCA8K_PWS_LED_OPEN_EN_CSR | QCA8K_PWS_POWER_ON_SEL;

At minimum, you need to clearly document that qca,led-open-drain
implies 'qca,ignore-power-on-sel'. I would probably go further and
return -EINVAL if qca,led-open-drain is set and
'qca,ignore-power-on-sel' is not.

	Andrew

[net-next PATCH 13/13] Documentation: devicetree: net: dsa: qca8k: document open drain binding

[Ansuel Smith]

Document new binding qca,power_on_sel used to enable Power-on-strapping
select reg and qca,led_open_drain to set led to open drain mode.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 Documentation/devicetree/bindings/net/dsa/qca8k.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/qca8k.txt b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
index 550f4313c6e0..4f6ef9f9e024 100644
--- a/Documentation/devicetree/bindings/net/dsa/qca8k.txt
+++ b/Documentation/devicetree/bindings/net/dsa/qca8k.txt
@@ -13,6 +13,10 @@ Required properties:
 Optional properties:
 
 - reset-gpios: GPIO to be used to reset the whole device
+- qca,package48: Set to 48-pin mode required in some QCA8327 switches.
+- qca,power-on-sel: Enable Power-on-strapping select required in some QCA8327
+  switches.
+- qca,led-open-drain: Set leds to open-drain mode.
 - qca,mac6-exchange: Internally swap MAC0 and MAC6.
 - qca,sgmii-rxclk-falling-edge: Set the receive clock phase to falling edge.
 - qca,sgmii-txclk-falling-edge: Set the transmit clock phase to falling edge.
-- 
2.32.0