Re: [PATCH v1 net-next] net: stmmac: Add support for MDIO interrupts

Re: [PATCH v1 net-next] net: stmmac: Add support for MDIO interrupts

[Andrew Lunn]

On Tue, Aug 27, 2019 at 09:45:20AM +0800, Voon Weifeng wrote:
> From: "Chuah, Kim Tatt" <kim.tatt.chuah@intel.com>
> 
> DW EQoS v5.xx controllers added capability for interrupt generation
> when MDIO interface is done (GMII Busy bit is cleared).
> This patch adds support for this interrupt on supported HW to avoid
> polling on GMII Busy bit.
> 
> stmmac_mdio_read() & stmmac_mdio_write() will sleep until wake_up() is
> called by the interrupt handler.

Hi Voon

I _think_ there are some order of operation issues here. The mdiobus
is registered in the probe function. As soon as of_mdiobus_register()
is called, the MDIO bus must work. At that point MDIO read/writes can
start to happen.

As far as i can see, the interrupt handler is only requested in
stmmac_open(). So it seems like any MDIO operations after probe, but
before open are going to fail?

Thanks
       Andrew

Re: [PATCH v1 net-next] net: stmmac: Add support for MDIO interrupts

[Florian Fainelli]

On 8/26/19 11:47 AM, Andrew Lunn wrote:
> On Tue, Aug 27, 2019 at 09:45:20AM +0800, Voon Weifeng wrote:
>> From: "Chuah, Kim Tatt" <kim.tatt.chuah@intel.com>
>>
>> DW EQoS v5.xx controllers added capability for interrupt generation
>> when MDIO interface is done (GMII Busy bit is cleared).
>> This patch adds support for this interrupt on supported HW to avoid
>> polling on GMII Busy bit.
>>
>> stmmac_mdio_read() & stmmac_mdio_write() will sleep until wake_up() is
>> called by the interrupt handler.
> 
> Hi Voon
> 
> I _think_ there are some order of operation issues here. The mdiobus
> is registered in the probe function. As soon as of_mdiobus_register()
> is called, the MDIO bus must work. At that point MDIO read/writes can
> start to happen.
> 
> As far as i can see, the interrupt handler is only requested in
> stmmac_open(). So it seems like any MDIO operations after probe, but
> before open are going to fail?

AFAIR, wait_event_timeout() will continue to busy loop and wait until
the timeout, but not return an error because the polled condition was
true, at least that is my recollection from having the same issue with
the bcmgenet driver before it was moved to connecting to the PHY in the
ndo_open() function.
-- 
Florian

[PATCH v1 net-next] net: stmmac: Add support for MDIO interrupts

[Voon Weifeng]

From: "Chuah, Kim Tatt" <kim.tatt.chuah@intel.com>

DW EQoS v5.xx controllers added capability for interrupt generation
when MDIO interface is done (GMII Busy bit is cleared).
This patch adds support for this interrupt on supported HW to avoid
polling on GMII Busy bit.

stmmac_mdio_read() & stmmac_mdio_write() will sleep until wake_up() is
called by the interrupt handler.

Reviewed-by: Voon Weifeng <weifeng.voon@intel.com>
Reviewed-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
Reviewed-by: Ong Boon Leong <boon.leong.ong@intel.com>
Signed-off-by: Chuah, Kim Tatt <kim.tatt.chuah@intel.com>
Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com>
Signed-off-by: Voon Weifeng <weifeng.voon@intel.com>

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 49aa56ca09cc..775a1c114b1a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -448,6 +448,8 @@ struct mac_device_info {
 	unsigned int pcs;
 	unsigned int pmt;
 	unsigned int ps;
+	bool mdio_intr_en;
+	wait_queue_head_t mdio_busy_wait;
 };
 
 struct stmmac_rx_routing {
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
index 2ed11a581d80..1be6a8a88b8f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
@@ -106,6 +106,7 @@ enum dwmac4_irq_status {
 	mmc_irq = 0x00000100,
 	lpi_irq = 0x00000020,
 	pmt_irq = 0x00000010,
+	mdio_irq = 0x00040000,
 };
 
 /* MAC PMT bitmap */
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
index fc9954e4a772..54adad616b90 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c
@@ -59,6 +59,9 @@ static void dwmac4_core_init(struct mac_device_info *hw,
 	if (hw->pcs)
 		value |= GMAC_PCS_IRQ_DEFAULT;
 
+	if (hw->mdio_intr_en)
+		value |= GMAC_INT_MDIO_EN;
+
 	writel(value, ioaddr + GMAC_INT_EN);
 }
 
@@ -629,6 +632,9 @@ static int dwmac4_irq_status(struct mac_device_info *hw,
 			x->irq_rx_path_exit_lpi_mode_n++;
 	}
 
+	if (intr_status & mdio_irq)
+		wake_up(&hw->mdio_busy_wait);
+
 	dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x);
 	if (intr_status & PCS_RGSMIIIS_IRQ)
 		dwmac4_phystatus(ioaddr, x);
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
index 775db776b3cc..48550d617b01 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac5.h
@@ -72,6 +72,9 @@
 #define TCEIE				BIT(0)
 #define DMA_ECC_INT_STATUS		0x00001088
 
+/* MDIO interrupt enable in MAC_Interrupt_Enable register */
+#define GMAC_INT_MDIO_EN		BIT(18)
+
 int dwmac5_safety_feat_config(void __iomem *ioaddr, unsigned int asp);
 int dwmac5_safety_feat_irq_status(struct net_device *ndev,
 		void __iomem *ioaddr, unsigned int asp,
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
index 3af2e5015245..11c7f92e99b4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
@@ -73,6 +73,7 @@ static int stmmac_dwmac4_quirks(struct stmmac_priv *priv)
 	bool gmac;
 	bool gmac4;
 	bool xgmac;
+	bool mdio_intr_en;
 	u32 min_id;
 	const struct stmmac_regs_off regs;
 	const void *desc;
@@ -90,6 +91,7 @@ static int stmmac_dwmac4_quirks(struct stmmac_priv *priv)
 		.gmac = false,
 		.gmac4 = false,
 		.xgmac = false,
+		.mdio_intr_en = false,
 		.min_id = 0,
 		.regs = {
 			.ptp_off = PTP_GMAC3_X_OFFSET,
@@ -108,6 +110,7 @@ static int stmmac_dwmac4_quirks(struct stmmac_priv *priv)
 		.gmac = true,
 		.gmac4 = false,
 		.xgmac = false,
+		.mdio_intr_en = false,
 		.min_id = 0,
 		.regs = {
 			.ptp_off = PTP_GMAC3_X_OFFSET,
@@ -126,6 +129,7 @@ static int stmmac_dwmac4_quirks(struct stmmac_priv *priv)
 		.gmac = false,
 		.gmac4 = true,
 		.xgmac = false,
+		.mdio_intr_en = false,
 		.min_id = 0,
 		.regs = {
 			.ptp_off = PTP_GMAC4_OFFSET,
@@ -144,6 +148,7 @@ static int stmmac_dwmac4_quirks(struct stmmac_priv *priv)
 		.gmac = false,
 		.gmac4 = true,
 		.xgmac = false,
+		.mdio_intr_en = false,
 		.min_id = DWMAC_CORE_4_00,
 		.regs = {
 			.ptp_off = PTP_GMAC4_OFFSET,
@@ -162,6 +167,7 @@ static int stmmac_dwmac4_quirks(struct stmmac_priv *priv)
 		.gmac = false,
 		.gmac4 = true,
 		.xgmac = false,
+		.mdio_intr_en = false,
 		.min_id = DWMAC_CORE_4_10,
 		.regs = {
 			.ptp_off = PTP_GMAC4_OFFSET,
@@ -180,6 +186,7 @@ static int stmmac_dwmac4_quirks(struct stmmac_priv *priv)
 		.gmac = false,
 		.gmac4 = true,
 		.xgmac = false,
+		.mdio_intr_en = true,
 		.min_id = DWMAC_CORE_5_10,
 		.regs = {
 			.ptp_off = PTP_GMAC4_OFFSET,
@@ -198,6 +205,7 @@ static int stmmac_dwmac4_quirks(struct stmmac_priv *priv)
 		.gmac = false,
 		.gmac4 = false,
 		.xgmac = true,
+		.mdio_intr_en = false,
 		.min_id = DWXGMAC_CORE_2_10,
 		.regs = {
 			.ptp_off = PTP_XGMAC_OFFSET,
@@ -276,6 +284,10 @@ int stmmac_hwif_init(struct stmmac_priv *priv)
 		mac->mode = mac->mode ? : entry->mode;
 		mac->tc = mac->tc ? : entry->tc;
 		mac->mmc = mac->mmc ? : entry->mmc;
+		mac->mdio_intr_en = mac->mdio_intr_en ? : entry->mdio_intr_en;
+
+		if (mac->mdio_intr_en)
+			init_waitqueue_head(&mac->mdio_busy_wait);
 
 		priv->hw = mac;
 		priv->ptpaddr = priv->ioaddr + entry->regs.ptp_off;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 40c42637ad75..c9a23218307b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -142,6 +142,15 @@ static int stmmac_xgmac2_mdio_write(struct mii_bus *bus, int phyaddr,
 				  !(tmp & MII_XGMAC_BUSY), 100, 10000);
 }
 
+static bool stmmac_mdio_intr_done(struct mii_bus *bus)
+{
+	struct net_device *ndev = bus->priv;
+	struct stmmac_priv *priv = netdev_priv(ndev);
+	unsigned int mii_address = priv->hw->mii.addr;
+
+	return !(readl(priv->ioaddr + mii_address) & MII_BUSY);
+}
+
 /**
  * stmmac_mdio_read
  * @bus: points to the mii_bus structure
@@ -181,16 +190,26 @@ static int stmmac_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
 		}
 	}
 
-	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v & MII_BUSY),
-			       100, 10000))
+	if (priv->hw->mdio_intr_en) {
+		if (!wait_event_timeout(priv->hw->mdio_busy_wait,
+					stmmac_mdio_intr_done(bus), HZ / 100))
+			return -EBUSY;
+	} else if (readl_poll_timeout(priv->ioaddr + mii_address, v,
+				      !(v & MII_BUSY), 100, 10000)) {
 		return -EBUSY;
+	}
 
 	writel(data, priv->ioaddr + mii_data);
 	writel(value, priv->ioaddr + mii_address);
 
-	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v & MII_BUSY),
-			       100, 10000))
+	if (priv->hw->mdio_intr_en) {
+		if (!wait_event_timeout(priv->hw->mdio_busy_wait,
+					stmmac_mdio_intr_done(bus), HZ / 100))
+			return -EBUSY;
+	} else if (readl_poll_timeout(priv->ioaddr + mii_address, v,
+				      !(v & MII_BUSY), 100, 10000)) {
 		return -EBUSY;
+	}
 
 	/* Read the data from the MII data register */
 	data = (int)readl(priv->ioaddr + mii_data) & MII_DATA_MASK;
@@ -240,17 +259,30 @@ static int stmmac_mdio_write(struct mii_bus *bus, int phyaddr, int phyreg,
 	}
 
 	/* Wait until any existing MII operation is complete */
-	if (readl_poll_timeout(priv->ioaddr + mii_address, v, !(v & MII_BUSY),
-			       100, 10000))
+	if (priv->hw->mdio_intr_en) {
+		if (!wait_event_timeout(priv->hw->mdio_busy_wait,
+					stmmac_mdio_intr_done(bus), HZ / 100))
+			return -EBUSY;
+	} else if (readl_poll_timeout(priv->ioaddr + mii_address, v,
+				      !(v & MII_BUSY), 100, 10000)) {
 		return -EBUSY;
+	}
 
 	/* Set the MII address register to write */
 	writel(data, priv->ioaddr + mii_data);
 	writel(value, priv->ioaddr + mii_address);
 
 	/* Wait until any existing MII operation is complete */
-	return readl_poll_timeout(priv->ioaddr + mii_address, v, !(v & MII_BUSY),
-				  100, 10000);
+	if (priv->hw->mdio_intr_en) {
+		if (!wait_event_timeout(priv->hw->mdio_busy_wait,
+					stmmac_mdio_intr_done(bus), HZ / 100))
+			return -EBUSY;
+	} else if (readl_poll_timeout(priv->ioaddr + mii_address, v,
+				      !(v & MII_BUSY), 100, 10000)) {
+		return -EBUSY;
+	}
+
+	return 0;
 }
 
 /**
-- 
1.9.1

RE: [PATCH v1 net-next] net: stmmac: Add support for MDIO interrupts

[Voon, Weifeng]

> >> DW EQoS v5.xx controllers added capability for interrupt generation
> >> when MDIO interface is done (GMII Busy bit is cleared).
> >> This patch adds support for this interrupt on supported HW to avoid
> >> polling on GMII Busy bit.
> >>
> >> stmmac_mdio_read() & stmmac_mdio_write() will sleep until wake_up()
> >> is called by the interrupt handler.
> >
> > Hi Voon
> >
> > I _think_ there are some order of operation issues here. The mdiobus
> > is registered in the probe function. As soon as of_mdiobus_register()
> > is called, the MDIO bus must work. At that point MDIO read/writes can
> > start to happen.
> >
> > As far as i can see, the interrupt handler is only requested in
> > stmmac_open(). So it seems like any MDIO operations after probe, but
> > before open are going to fail?
> 
> AFAIR, wait_event_timeout() will continue to busy loop and wait until
> the timeout, but not return an error because the polled condition was
> true, at least that is my recollection from having the same issue with
> the bcmgenet driver before it was moved to connecting to the PHY in the
> ndo_open() function.
> --
> Florian

Florian is right as the poll condition is still true after the timeout. 
Hence, any mdio operation after probe and before ndo_open will still work.
The only cons here is that attaching the PHY will takes a full length of 
timeout time for each mdio_read and mdio_write. 
So we should attach the phy only after the interrupt handler is requested?
 

Re: [PATCH v1 net-next] net: stmmac: Add support for MDIO interrupts

[Florian Fainelli]

On 8/28/19 10:07 AM, Voon, Weifeng wrote:
>>>> DW EQoS v5.xx controllers added capability for interrupt generation
>>>> when MDIO interface is done (GMII Busy bit is cleared).
>>>> This patch adds support for this interrupt on supported HW to avoid
>>>> polling on GMII Busy bit.
>>>>
>>>> stmmac_mdio_read() & stmmac_mdio_write() will sleep until wake_up()
>>>> is called by the interrupt handler.
>>>
>>> Hi Voon
>>>
>>> I _think_ there are some order of operation issues here. The mdiobus
>>> is registered in the probe function. As soon as of_mdiobus_register()
>>> is called, the MDIO bus must work. At that point MDIO read/writes can
>>> start to happen.
>>>
>>> As far as i can see, the interrupt handler is only requested in
>>> stmmac_open(). So it seems like any MDIO operations after probe, but
>>> before open are going to fail?
>>
>> AFAIR, wait_event_timeout() will continue to busy loop and wait until
>> the timeout, but not return an error because the polled condition was
>> true, at least that is my recollection from having the same issue with
>> the bcmgenet driver before it was moved to connecting to the PHY in the
>> ndo_open() function.
>> --
>> Florian
> 
> Florian is right as the poll condition is still true after the timeout. 
> Hence, any mdio operation after probe and before ndo_open will still work.
> The only cons here is that attaching the PHY will takes a full length of 
> timeout time for each mdio_read and mdio_write. 
> So we should attach the phy only after the interrupt handler is requested?

From a power management/resource utilization perspective, it is better
to initialize as close as possible from the time where you are actually
going to use the hardware, therefore ndo_open().

This may not be convenient or possible given how widely use stmmac is,
and I do not know if parts of the Ethernet MAC require the PHY to supply
the clock, in which case, you may have some chicke and egg conditions if
the design does not allow for MDIO to work independently from the data
plane. Also, I would be worried about introducing bugs.

You could do a couple of things:

- continue to probe the device with interrupts disabled and add a
condition around the call to wait_event_timeout() to do a busy-loop
without going to the maximum defined timeout, if the interrupt line is
requested, use wait_event_timeout()

- request the interrupt during the probe function, but only
unmask/enable the MDIO interrupts for the probe to succeed and leave the
data path interrupts for a later enabling during ndo_open()
-- 
Florian

Re: [PATCH v1 net-next] net: stmmac: Add support for MDIO interrupts

[David Miller]

From: Voon Weifeng <weifeng.voon@intel.com>
Date: Tue, 27 Aug 2019 09:45:20 +0800

> From: "Chuah, Kim Tatt" <kim.tatt.chuah@intel.com>
> 
> DW EQoS v5.xx controllers added capability for interrupt generation
> when MDIO interface is done (GMII Busy bit is cleared).
> This patch adds support for this interrupt on supported HW to avoid
> polling on GMII Busy bit.
> 
> stmmac_mdio_read() & stmmac_mdio_write() will sleep until wake_up() is
> called by the interrupt handler.
> 
> Reviewed-by: Voon Weifeng <weifeng.voon@intel.com>
> Reviewed-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
> Reviewed-by: Ong Boon Leong <boon.leong.ong@intel.com>
> Signed-off-by: Chuah, Kim Tatt <kim.tatt.chuah@intel.com>
> Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com>
> Signed-off-by: Voon Weifeng <weifeng.voon@intel.com>

I know there are some design changes that will occur with this patch but
coding style wise:

> @@ -276,6 +284,10 @@ int stmmac_hwif_init(struct stmmac_priv *priv)
>  		mac->mode = mac->mode ? : entry->mode;
>  		mac->tc = mac->tc ? : entry->tc;
>  		mac->mmc = mac->mmc ? : entry->mmc;
> +		mac->mdio_intr_en = mac->mdio_intr_en ? : entry->mdio_intr_en;
> +
> +		if (mac->mdio_intr_en)
> +			init_waitqueue_head(&mac->mdio_busy_wait);

I'd say always unconditionally initialize wait queues, mutexes, etc.

> +static bool stmmac_mdio_intr_done(struct mii_bus *bus)
> +{
> +	struct net_device *ndev = bus->priv;
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	unsigned int mii_address = priv->hw->mii.addr;

Reverse christmas tree here, please.