[PATCH net-next] net: phy: micrel: Add config_init for LAN8814

Add config_init for LAN8814. This function is required for the following
reasons:
- we need to make sure that the PHY is reset,
- disable ANEG with QSGMII PCS Host side
- swap the MDI-X A,B transmit so that there will not be any link flip-flaps
  when the PHY gets a link.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/phy/micrel.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 44a24b99c894..f080312032cf 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c

@@ -1565,6 +1565,14 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev,
 #define LAN_EXT_PAGE_ACCESS_ADDRESS_DATA		0x17
 #define LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC		0x4000
 
+#define LAN8814_QSGMII_SOFT_RESET			0x43
+#define LAN8814_QSGMII_SOFT_RESET_BIT			BIT(0)
+#define LAN8814_QSGMII_PCS1G_ANEG_CONFIG		0x13
+#define LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA	BIT(3)
+#define LAN8814_ALIGN_SWAP				0x4a
+#define LAN8814_ALIGN_TX_A_B_SWAP			0x1
+#define LAN8814_ALIGN_TX_A_B_SWAP_MASK			GENMASK(2, 0)
+
 #define LAN8804_ALIGN_SWAP				0x4a
 #define LAN8804_ALIGN_TX_A_B_SWAP			0x1
 #define LAN8804_ALIGN_TX_A_B_SWAP_MASK			GENMASK(2, 0)

@@ -1601,6 +1609,29 @@ static int lanphy_write_page_reg(struct phy_device *phydev, int page, u16 addr,
 	return 0;
 }
 
+static int lan8814_config_init(struct phy_device *phydev)
+{
+	int val;
+
+	/* Reset the PHY */
+	val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
+	val |= LAN8814_QSGMII_SOFT_RESET_BIT;
+	lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
+
+	/* Disable ANEG with QSGMII PCS Host side */
+	val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
+	val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
+	lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val);
+
+	/* MDI-X setting for swap A,B transmit */
+	val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP);
+	val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK;
+	val |= LAN8814_ALIGN_TX_A_B_SWAP;
+	lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val);
+
+	return 0;
+}
+
 static int lan8804_config_init(struct phy_device *phydev)
 {
 	int val;

@@ -1793,6 +1824,7 @@ static struct phy_driver ksphy_driver[] = {
 	.phy_id		= PHY_ID_LAN8814,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Microchip INDY Gigabit Quad PHY",
+	.config_init	= lan8814_config_init,
 	.driver_data	= &ksz9021_type,
 	.probe		= kszphy_probe,
 	.soft_reset	= genphy_soft_reset,

-- 
2.33.0

On 26.11.2021 11:38, Horatiu Vultur wrote:

Not directly related to just this patch:
Did you consider implementing the read_page and write_page PHY driver
callbacks? Then you could use phylib functions like phy_modify_paged et al
and you wouldn't have to open-code the paged register operations.

I think write_page would just be
phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC));

and read_page
phy_read(phydev, LAN_EXT_PAGE_ACCESS_CONTROL);

On Fri, Nov 26, 2021 at 12:57:33PM +0100, Heiner Kallweit wrote:

Not directly related to just this patch:
Did you consider implementing the read_page and write_page PHY driver
callbacks? Then you could use phylib functions like phy_modify_paged et al
and you wouldn't have to open-code the paged register operations.

I think write_page would just be
phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC));

and read_page
phy_read(phydev, LAN_EXT_PAGE_ACCESS_CONTROL);

Remember that read_page() and write_page() must be implemented using
the unlocked accessors since the MDIO bus lock is held prior to calling
them. So these should be __phy_write() and __phy_read().

The use of the helpers you mention above also bring greater safety to
the read-modify-write accesses, since with these accessors, the whole
set of accesses are done while holding the bus lock.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

- swap the MDI-X A,B transmit so that there will not be any link flip-flaps
  when the PHY gets a link.

Isn't this a board issue, rather than generic? Or does the datasheet
have the pins labelled wrongly and all boards following the datasheet
are wrong?

	Andrew

The 11/26/2021 12:57, Heiner Kallweit wrote:

Hi Heiner,

On 26.11.2021 11:38, Horatiu Vultur wrote:

quoted

+static int lan8814_config_init(struct phy_device *phydev)
+{
+     int val;
+
+     /* Reset the PHY */
+     val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
+     val |= LAN8814_QSGMII_SOFT_RESET_BIT;
+     lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
+
+     /* Disable ANEG with QSGMII PCS Host side */
+     val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
+     val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
+     lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val);
+
+     /* MDI-X setting for swap A,B transmit */
+     val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP);
+     val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK;
+     val |= LAN8814_ALIGN_TX_A_B_SWAP;
+     lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val);
+

Not directly related to just this patch:
Did you consider implementing the read_page and write_page PHY driver
callbacks? Then you could use phylib functions like phy_modify_paged et al
and you wouldn't have to open-code the paged register operations.

I think write_page would just be
phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC));

and read_page
phy_read(phydev, LAN_EXT_PAGE_ACCESS_CONTROL);

Thanks for the suggestion, but unfortunately it would not work.
The reason is that in the callback 'write_page' I don't actually get
also the address in the page that is needed to be read/write.

If this issue would be fixed, then there is another problem.
To read/write the data in the extended page is required to access the
register LAN_EXT_PAGE_ACCESS_ADDRESS_DATA. But that would not happen
when using the phy_read_paged, it would read actually the register in
page 0.

If I have missed something, please let me know.

quoted

+     return 0;
+}
+
 static int lan8804_config_init(struct phy_device *phydev)
 {
      int val;

@@ -1793,6 +1824,7 @@ static struct phy_driver ksphy_driver[] = {
      .phy_id         = PHY_ID_LAN8814,
      .phy_id_mask    = MICREL_PHY_ID_MASK,
      .name           = "Microchip INDY Gigabit Quad PHY",
+     .config_init    = lan8814_config_init,
      .driver_data    = &ksz9021_type,
      .probe          = kszphy_probe,
      .soft_reset     = genphy_soft_reset,

-- 
/Horatiu

The 11/26/2021 16:15, Andrew Lunn wrote:

Hi Andrew,

quoted

- swap the MDI-X A,B transmit so that there will not be any link flip-flaps
  when the PHY gets a link.

Isn't this a board issue, rather than generic? Or does the datasheet
have the pins labelled wrongly and all boards following the datasheet
are wrong?

From my understanding it is not a board issue. This is needed in case
the link partners can't do the A/B swapping based on MDI/MDIX. It would
not harm if this is set also for link partners that can do this.

        Andrew

-- 
/Horatiu

On 29.11.2021 09:29, Horatiu Vultur wrote:

The 11/26/2021 12:57, Heiner Kallweit wrote:

Hi Heiner,

quoted

On 26.11.2021 11:38, Horatiu Vultur wrote:

quoted

+static int lan8814_config_init(struct phy_device *phydev)
+{
+     int val;
+
+     /* Reset the PHY */
+     val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
+     val |= LAN8814_QSGMII_SOFT_RESET_BIT;
+     lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
+
+     /* Disable ANEG with QSGMII PCS Host side */
+     val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
+     val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
+     lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val);
+
+     /* MDI-X setting for swap A,B transmit */
+     val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP);
+     val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK;
+     val |= LAN8814_ALIGN_TX_A_B_SWAP;
+     lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val);
+

Not directly related to just this patch:
Did you consider implementing the read_page and write_page PHY driver
callbacks? Then you could use phylib functions like phy_modify_paged et al
and you wouldn't have to open-code the paged register operations.

I think write_page would just be
phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC));

and read_page
phy_read(phydev, LAN_EXT_PAGE_ACCESS_CONTROL);

Thanks for the suggestion, but unfortunately it would not work.
The reason is that in the callback 'write_page' I don't actually get
also the address in the page that is needed to be read/write.

If this issue would be fixed, then there is another problem.
To read/write the data in the extended page is required to access the
register LAN_EXT_PAGE_ACCESS_ADDRESS_DATA. But that would not happen
when using the phy_read_paged, it would read actually the register in
page 0.

If I have missed something, please let me know.

Right, after reading the sequence in lanphy_read_page_reg() more
carefully I agree. This PHY re-uses the more complex MMD access
mechanism for paged access.

quoted

quoted

+     return 0;
+}
+
 static int lan8804_config_init(struct phy_device *phydev)
 {
      int val;

@@ -1793,6 +1824,7 @@ static struct phy_driver ksphy_driver[] = {
      .phy_id         = PHY_ID_LAN8814,
      .phy_id_mask    = MICREL_PHY_ID_MASK,
      .name           = "Microchip INDY Gigabit Quad PHY",
+     .config_init    = lan8814_config_init,
      .driver_data    = &ksz9021_type,
      .probe          = kszphy_probe,
      .soft_reset     = genphy_soft_reset,

The 11/26/2021 11:38, Horatiu Vultur wrote:

Hi,

Sorry for reviving this old thread. I can see this patch was marked as
"Changes Requested" [1]. The change that Heiner proposed, will not worked as
we already discussed. It is using a different mechanism to access extend
pages.
Should I just try to resend the patch or is possible to get this one?

[1] https://patchwork.kernel.org/project/netdevbpf/patch/20211126103833.3609945-1-horatiu.vultur@microchip.com/

-- 
/Horatiu

On Wed, 22 Dec 2021 12:48:20 +0100 Horatiu Vultur wrote:

The 11/26/2021 11:38, Horatiu Vultur wrote:

Sorry for reviving this old thread. I can see this patch was marked as
"Changes Requested" [1]. The change that Heiner proposed, will not worked as
we already discussed. It is using a different mechanism to access extend
pages.
Should I just try to resend the patch or is possible to get this one?

[1] https://patchwork.kernel.org/project/netdevbpf/patch/20211126103833.3609945-1-horatiu.vultur@microchip.com/

Please resend (preferably with a short note on why in the change log).