Re: [RFC PATCH 0/3] acpi: Add acpi mdio support code

[Timur Tabi]

On 11/8/18 5:23 PM, Andrew Lunn wrote:

I don't know much about ACPI. I do know DT. MDIO busses can have
multiple PHYs on them. Is the following valid to list two PHYs?

  Device (MDIO) {
  Name (_DSD, Package () {
  ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
  Package () { Package () { "ethernet-phy@0", PHY0 }, }
  })
  Name (PHY0, Package() {
  ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
  Package () { Package () { "reg", 0x0 }, }
  })
  Name (_DSD, Package () {
  ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
  Package () { Package () { "ethernet-phy@10", PHY1 }, }
  })
  Name (PHY1, Package() {
  ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
  Package () { Package () { "reg", 0x10 }, }
  })
  }


You can't have the same DSD twice.  It would need to look like this:

 Name (PHY1, Package() {
 ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
 Package () { Package () { "reg", 0, 0x10 }, }
 })

Re: [PATCH v3 2/2] net: qcom/emac: add phy-handle support for ACPI

[Timur Tabi]

On 10/25/18 9:18 PM, Wang, Dongsheng wrote:

But when I was reading Documentation/acpi/DSD-properties-rules.txt, my
understanding is we should try to conform to DT bindings. So maybe ACPI
doesn't have such a document, just DT bindings.


There was an attempt to document DSDs, but it was abandoned after a while.

https://github.com/ahs3/dsd

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

[Timur Tabi]

On 9/19/18 10:20 AM, Andrew Lunn wrote:

I suspect that is not going to be easy. Last time i looked, the ACPI
standard had nothing about MDIO busses or PHYs. Marcin Wojtas did some
work in this area a while back for the mvpp2, but if i remember
correctly, he worked around this by simply not having a PHY when using
ACPI, and making use of a MAC interrupt which indicated when there was
link.

Whoever implements this first needs to be an ACPI expert and probably
needs to write it up and submit it as an amendment to the ACPI
standard.


If that's what it takes, then so be it.  But adding DT support for a 
device that is only used on ACPI platforms is not a worthwhile endeavor.


After ACPI support is merged, Dongsheng can choose to add DT support to 
maintain parity, if he wants.  Maybe one day the MSM developers will use 
the upstream driver on one of their SOCs.

Re: [PATCH v2 2/4] dt-bindings: net: qcom: Add binding for shared mdio bus

[Timur Tabi]

On 9/19/18 7:25 AM, Andrew Lunn wrote:

ACPI is completely separate and should not affect the DT binding.
I've not yet looked at the ACPI changes you added.


Just FYI, there is no device tree platform on which the upstream EMAC 
driver works.  All of the DT code in the driver is theoretical.  It 
worked once on a prototype platform, when I originally wrote the code, 
but since then DT support is mostly a guess.


The focus of any patches for the EMAC should be ACPI, not DT.  If 
anything, ACPI support should come first.  No one should be writing or 
reviewing DT code before ACPI code.


The upstream EMAC driver is only known to work on the QDF2400, which is 
an ACPI-only chip.  I feel like I've been repeating this too often.

Re: [PATCH 2/2] net: qcom/emac: add shared mdio bus support

[Timur Tabi]

On 9/13/18 7:42 AM, Andrew Lunn wrote:

This is a pretty big patch, and is hard to review. Could you try to
break it up into a number of smaller patches. You could for example
first refactor emacs_phy_config(), without making any functional
changes. Then add the sharing. Maybe do OF an ACPI in different
patches?


Yes, please.

Re: [PATCH, net-next] qcom-emag: hide ACPI specific functions

[Timur Tabi]

On 5/25/18 7:22 PM, Timur Tabi wrote:


-phy->open = emac_sgmii_open;
-phy->close = emac_sgmii_close;
-phy->link_up = emac_sgmii_link_up;
-phy->link_down = emac_sgmii_link_down;

I'll take it look at it next week when I'm back in the office.


I posted a patch that fixes this problem and also retains device-tree
support.

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] net: qcom/emac: fix device tree initialization

[Timur Tabi]

Commit "net: qcom/emac: Encapsulate sgmii ops under one structure"
introduced the sgmii_ops structure, but did not correctly initialize
it on device tree platforms.  This resulted in compiler warnings when
ACPI is not enabled.

Reported-by: Arnd Bergmann <a...@arndb.de>
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 16 +---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index 562420b834df..e78e5db39458 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -273,6 +273,14 @@ static int emac_sgmii_common_link_change(struct 
emac_adapter *adpt, bool linkup)
return 0;
 }
 
+static struct sgmii_ops fsm9900_ops = {
+   .init = emac_sgmii_init_fsm9900,
+   .open = emac_sgmii_common_open,
+   .close = emac_sgmii_common_close,
+   .link_change = emac_sgmii_common_link_change,
+   .reset = emac_sgmii_common_reset,
+};
+
 static struct sgmii_ops qdf2432_ops = {
.init = emac_sgmii_init_qdf2432,
.open = emac_sgmii_common_open,
@@ -281,6 +289,7 @@ static int emac_sgmii_common_link_change(struct 
emac_adapter *adpt, bool linkup)
.reset = emac_sgmii_common_reset,
 };
 
+#ifdef CONFIG_ACPI
 static struct sgmii_ops qdf2400_ops = {
.init = emac_sgmii_init_qdf2400,
.open = emac_sgmii_common_open,
@@ -288,6 +297,7 @@ static int emac_sgmii_common_link_change(struct 
emac_adapter *adpt, bool linkup)
.link_change = emac_sgmii_common_link_change,
.reset = emac_sgmii_common_reset,
 };
+#endif
 
 static int emac_sgmii_acpi_match(struct device *dev, void *data)
 {
@@ -335,11 +345,11 @@ static int emac_sgmii_acpi_match(struct device *dev, void 
*data)
 static const struct of_device_id emac_sgmii_dt_match[] = {
{
.compatible = "qcom,fsm9900-emac-sgmii",
-   .data = emac_sgmii_init_fsm9900,
+   .data = _ops,
},
{
.compatible = "qcom,qdf2432-emac-sgmii",
-   .data = emac_sgmii_init_qdf2432,
+   .data = _ops,
},
{}
 };
@@ -386,7 +396,7 @@ int emac_sgmii_config(struct platform_device *pdev, struct 
emac_adapter *adpt)
goto error_put_device;
}
 
-   phy->sgmii_ops->init = match->data;
+   phy->sgmii_ops = (struct sgmii_ops *)match->data;
}
 
/* Base address is the first address */
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH, net-next] qcom-emag: hide ACPI specific functions

[Timur Tabi]

On 5/25/18 4:37 PM, Arnd Bergmann wrote:

+#ifdef CONFIG_ACPI
  static int emac_sgmii_irq_clear(struct emac_adapter *adpt, u8 irq_bits)
  {
struct emac_sgmii *phy = >phy;
@@ -288,6 +289,7 @@ static struct sgmii_ops qdf2400_ops = {
.link_change = emac_sgmii_common_link_change,
.reset = emac_sgmii_common_reset,
  };
+#endif


This seems wrong.  The SGMII interrupt handler should still be viable on 
a device-tree system.  There is a DT compatibility entry for the qdf2432.


Looks like that most recent patch on net-next broke DT support, when it 
removed these lines:


-   phy->open = emac_sgmii_open;
-   phy->close = emac_sgmii_close;
-   phy->link_up = emac_sgmii_link_up;
-   phy->link_down = emac_sgmii_link_down;

I'll take it look at it next week when I'm back in the office.

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] net: qcom/emac: Allocate buffers from local node

[Timur Tabi]

On 5/17/18 3:28 AM, Hemanth Puranik wrote:

Currently we use non-NUMA aware allocation for TPD and RRD buffers,
this patch modifies to use NUMA friendly allocation.

Signed-off-by: Hemanth Puranik<hpura...@codeaurora.org>


Acked-by: Timur Tabi <ti...@codeaurora.org>

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] net: qcom/emac: Encapsulate sgmii ops under one structure

[Timur Tabi]

On 5/15/18 8:10 PM, Hemanth Puranik wrote:

This patch introduces ops structure for sgmii, This by ensures that
we do not need dummy functions in case of emulation platforms.

Signed-off-by: Hemanth Puranik<hpura...@codeaurora.org>


Acked-by: Timur Tabi <ti...@codeaurora.org>

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Timur Tabi]

On Fri, Mar 16, 2018 at 11:25 PM, Sinan Kaya  wrote:
> @@ -477,15 +477,16 @@ static inline void t4_ring_sq_db(struct t4_wq *wq, u16 
> inc, union t4_wr *wqe)
>  (u64 *)wqe);
> } else {
> pr_debug("DB wq->sq.pidx = %d\n", wq->sq.pidx);
> -   writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
> -  wq->sq.bar2_va + SGE_UDB_KDOORBELL);
> +   __raw_writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),

I think Jason might be right that drivers shouldn't be calling the
__raw_write functions.  You definitely need to run this by the PPC
developers, specifically Ben Herrenschmidt.


-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Timur Tabi]

On 3/16/18 6:04 PM, Steve Wise wrote:

Anybody understand why the PPC implementation of writeX_relaxed() isn't
relaxed?


You probably should ask that on the linuxppc-...@lists.ozlabs.org 
mailing list.


I've always wondered why PowerPC has non-standard I/O accessors.

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 7/7] ixgbevf: eliminate duplicate barriers on weakly-ordered archs

[Timur Tabi]

On 3/13/18 10:20 PM, Sinan Kaya wrote:

+/* Assumes caller has executed a write barrier to order memory and device
+ * requests.
+ */
  static inline void ixgbevf_write_tail(struct ixgbevf_ring *ring, u32 value)
  {
-   writel(value, ring->tail);
+   writel_relaxed(value, ring->tail);
  }


Why not put the wmb() in this function, or just get rid of the wmb() in 
the rest of the file and keep this as writel?  That way, you can avoid 
the comment and the risk that comes with it.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] net: qcom/emac: Use proper free methods during TX

[Timur Tabi]

On 3/5/18 8:48 PM, Hemanth Puranik wrote:

This patch fixes the warning messages/call traces seen if DMA debug is
enabled, In case of fragmented skb's memory was allocated using
dma_map_page but freed using dma_unmap_single. This patch modifies buffer
allocations in TX path to use dma_map_page in all the places and
dma_unmap_page while freeing the buffers.

Signed-off-by: Hemanth Puranik<hpura...@codeaurora.org>


Acked-by: Timur Tabi <ti...@codeaurora.org>

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [RFC] net: qcom/emac: mdiobus-dev fwnode should point to emac-adev

[Timur Tabi]

On 01/25/2018 09:59 AM, Andrew Lunn wrote:

I expect we will implement something like acpi_mdiobus_register(), and
it will take a pointer to an ACPI node. And maybe on top of
of_mdiobus_register() and of_mdiobus_register() we will add a
device_mdiobus_register().


Makes sense.  If you remember, please CC me on any patches.


What i'm trying to avoid is drivers ending up with different ACPI
bindings. If you don't want to add an ACPI node/property then no
problems, just don't expect to be able to use any of the optional
features of the MDIO core, like the GPIOs for reset.


Well, if a new binding is created, we will update our ACPI tables and 
drivers use it.  But we may need to keep the legacy code in emac-phy.c 
for backwards compatibility with older firmware.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [RFC] net: qcom/emac: mdiobus-dev fwnode should point to emac-adev

[Timur Tabi]

On 01/25/2018 08:15 AM, Andrew Lunn wrote:

If i'm reading your patch correctly, you are looking for the MDIO
reset in the MAC node. This is wrong. It is an MDIO property, so
should be in the MDIO device. Once we have figured out how to
represent MDIO busses in ACPI, the reset will be in the MDIO node.


Just FYI, the MDIO controller in the EMAC is integrated, so I can't see 
us creating a separate Device Tree or ACPI node/property for it. 
Granted, the code in emac-phy.c:emac_phy_config() that registers the 
MDIO bus is convoluted, so maybe there's an opportunity to replace 
some/all of that code with some generic API.  Maybe we need something 
like acpi_mdiobus_register() like we have of_mdiobus_register().


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [RFC] net: qcom/emac: mdiobus-dev fwnode should point to emac-adev

[Timur Tabi]

On 01/25/2018 12:14 AM, Wang Dongsheng wrote:

mdiobus always try to get a GPIO "reset" consumer, based on ACPI
the GPIO should be described in emac-adev _DSD or _CRS.


Are you talking about this:

/* de-assert bus level PHY GPIO reset */
gpiod = devm_gpiod_get_optional(>dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(gpiod)) {
dev_err(>dev, "mii_bus %s couldn't get reset GPIO\n",
bus->id);
return PTR_ERR(gpiod);

If so, I don't think we support named gpios in ACPI.  Do you have an 
actual test case for your patch?  Our ACPI tables don't specify any 
GPIOs for EMAC.  I wouldn't even know what that should look like.



diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c 
b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 53dbf1e..69171d5 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -117,6 +117,10 @@ int emac_phy_config(struct platform_device *pdev, struct 
emac_adapter *adpt)
  
  	if (has_acpi_companion(>dev)) {

u32 phy_addr;
+   struct fwnode_handle *fwnode;
+
+   fwnode = acpi_fwnode_handle(ACPI_COMPANION(>dev));
+   mii_bus->dev.fwnode = fwnode;


You don't need fwnode:

mii_bus->dev.fwnode =
acpi_fwnode_handle(ACPI_COMPANION(>dev));



--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v4] net: qcom/emac: extend DMA mask to 46bits

[Timur Tabi]

On 1/22/18 10:25 PM, Wang Dongsheng wrote:

Bit TPD3[31] is used as a timestamp bit if PTP is enabled, but
it's used as an address bit if PTP is disabled.  Since PTP isn't
supported by the driver, we can extend the DMA address to 46 bits.

Signed-off-by: Wang Dongsheng<dongsheng.w...@hxt-semitech.com>


Acked-by: Timur Tabi <ti...@codeaurora.org>

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] net: qcom/emac: Change the order of mac up and sgmii open

[Timur Tabi]

On 12/17/2017 11:57 PM, Hemanth Puranik wrote:

This patch fixes the order of mac_up and sgmii_open for the
reasons noted below:

- If open takes more time(if the SGMII block is not responding or
   if we want to do some delay based task) in this situation we
   will hit NETDEV watchdog
- The main reason : We should signal to upper layers that we are
   ready to receive packets "only" when the entire path is initialized
   not the other way around, this is followed in the reset path where
   we do mac_down, sgmii_reset and mac_up. This also makes the driver
   uniform across the reset and open paths.
- In the future there may be need for delay based tasks to be done in
   sgmii open which will result in NETDEV watchdog
- As per the documentation the order of init should be sgmii, mac, rings
   and DMA

Signed-off-by: Hemanth Puranik<hpura...@codeaurora.org>


Acked-by: Timur Tabi <ti...@codeaurora.org>

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] net: qcom/emac: Reduce timeout for mdio read/write

[Timur Tabi]

On 12/15/2017 08:35 AM, Hemanth Puranik wrote:

Currently mdio read/write takes around ~115us as the timeout
between status check is set to 100us.
By reducing the timeout to 1us mdio read/write takes ~15us to
complete. This improves the link up event response.

Signed-off-by: Hemanth Puranik<hpura...@codeaurora.org>


Acked-by: Timur Tabi <ti...@codeaurora.org>

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3] net: qcom/emac: extend DMA mask to 46bits

[Timur Tabi]

This is much better.  Can you give me a few days to test it on some 
internal platforms?


Also, this is a candidate for 4.16, so you need to wait until net-next 
is open anyway (http://vger.kernel.org/~davem/net-next.html).


On 11/20/17 7:48 AM, Wang Dongsheng wrote:

Since PTP doesn't support yet, so extend the DMA address to 46bits.


This needs to be longer, since it's not clear that TPD3[31] is bot a 
timestamp bit and an address bit.  How about this:


Bit TPD3[31] is used as a timestamp bit if PTP is enabled,
but it's used as an address bit if PTP is disabled.  Since PTP isn't 
supported by the driver, we can extend the DMA address to 46 bits.




--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2] net: qcom/emac: extend DMA mask to 46bits

[Timur Tabi]

On 11/17/17 3:56 AM, Wang Dongsheng wrote:

-#define TPD_BUFFER_ADDR_H_SET(tpd, val)BITS_SET((tpd)->word[3], 18, 
30, val)
+#define TPD_BUFFER_ADDR_H_SET(adpt, tpd, val)  BITS_SET((tpd)->word[3], 18, \
+   TX_TS_ENABLE & \
+   readl((adpt)->csr + EMAC_EMAC_WRAPPER_CSR1) ? \
+   30 : 31, val)


NAK.

Sorry, but this is terrible.  Every write to the TPD forces a secret 
read from another register?  No thank you.


Just do what you had in v1, but also set the DMA mask.  Add a comment 
saying that we can support 46 bits because we never enable timestamping.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] net: qcom/emac: fix the error of tpd buff address valid bit

[Timur Tabi]

On 11/10/2017 07:24 AM, Wang, Dongsheng wrote:


On QDF2400, EMAC TPD buff address size is [45:0]. buff address_l [31:0], 
buff address_h [31:18].
The address_h should change from [30:18] to [31:18]. So TPD buff address 
should has 46bits.


Bit 31 of the Word 3 in the TPD is the Timestamp_save.  BUFFER_ADDR_H is 
[30:18], not [31:18].


I'm curious to know where you get your information.  For one thing, if 
you're a Qualcomm employee, you should be using your codeaurora.org 
email address.  You would know that if you took LOST training.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] net: qcom/emac: fix the error of tpd buff address valid bit

[Timur Tabi]

On 11/10/17 3:49 AM, Wang Dongsheng wrote:

TPD has 46-bits as buff address valid bit. So fix the buff address
from 45-bits to 46-bits.


NAK.

The TPD has 45 bits.  Why do you say it was 46?

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] Revert "net: qcom/emac: enforce DMA address restrictions"

[Timur Tabi]

This reverts commit df1ec1b9d0df57e96011f175418dc95b1af46821.

It turns out that memory allocated via dma_alloc_coherent is always
aligned to the size of the buffer, so there's no way the RRD and RFD
can ever be in separate 32-bit regions.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 39 +++
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 0f5ece5d9507..9cbb27263742 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -734,11 +734,6 @@ static int emac_rx_descs_alloc(struct emac_adapter *adpt)
rx_q->rrd.size = rx_q->rrd.count * (adpt->rrd_size * 4);
rx_q->rfd.size = rx_q->rfd.count * (adpt->rfd_size * 4);
 
-   /* Check if the RRD and RFD are aligned properly, and if not, adjust. */
-   if (upper_32_bits(ring_header->dma_addr) !=
-   upper_32_bits(ring_header->dma_addr + ALIGN(rx_q->rrd.size, 8)))
-   ring_header->used = ALIGN(rx_q->rrd.size, 8);
-
rx_q->rrd.dma_addr = ring_header->dma_addr + ring_header->used;
rx_q->rrd.v_addr   = ring_header->v_addr + ring_header->used;
ring_header->used += ALIGN(rx_q->rrd.size, 8);
@@ -772,18 +767,11 @@ int emac_mac_rx_tx_rings_alloc_all(struct emac_adapter 
*adpt)
 
/* Ring DMA buffer. Each ring may need up to 8 bytes for alignment,
 * hence the additional padding bytes are allocated.
-*
-* Also double the memory allocated for the RRD so that we can
-* re-align it if necessary.  The EMAC has a restriction that the
-* upper 32 bits of the base addresses for the RFD and RRD rings
-* must be the same.  It is extremely unlikely that this is not the
-* case, since the rings are only a few KB in size.  However, we
-* need to check for this anyway, and if the two rings are not
-* compliant, then we re-align.
 */
-   ring_header->size = ALIGN(num_tx_descs * (adpt->tpd_size * 4), 8) +
-   ALIGN(num_rx_descs * (adpt->rfd_size * 4), 8) +
-   ALIGN(num_rx_descs * (adpt->rrd_size * 4), 8) * 2;
+   ring_header->size = num_tx_descs * (adpt->tpd_size * 4) +
+   num_rx_descs * (adpt->rfd_size * 4) +
+   num_rx_descs * (adpt->rrd_size * 4) +
+   8 + 2 * 8; /* 8 byte per one Tx and two Rx rings */
 
ring_header->used = 0;
ring_header->v_addr = dma_zalloc_coherent(dev, ring_header->size,
@@ -792,23 +780,26 @@ int emac_mac_rx_tx_rings_alloc_all(struct emac_adapter 
*adpt)
if (!ring_header->v_addr)
return -ENOMEM;
 
-   ret = emac_rx_descs_alloc(adpt);
-   if (ret) {
-   netdev_err(adpt->netdev, "error: Rx Queue alloc failed\n");
-   goto err_alloc_rx;
-   }
+   ring_header->used = ALIGN(ring_header->dma_addr, 8) -
+   ring_header->dma_addr;
 
ret = emac_tx_q_desc_alloc(adpt, >tx_q);
if (ret) {
-   netdev_err(adpt->netdev, "transmit queue allocation failed\n");
+   netdev_err(adpt->netdev, "error: Tx Queue alloc failed\n");
goto err_alloc_tx;
}
 
+   ret = emac_rx_descs_alloc(adpt);
+   if (ret) {
+   netdev_err(adpt->netdev, "error: Rx Queue alloc failed\n");
+   goto err_alloc_rx;
+   }
+
return 0;
 
-err_alloc_tx:
-   emac_rx_q_bufs_free(adpt);
 err_alloc_rx:
+   emac_tx_q_bufs_free(adpt);
+err_alloc_tx:
dma_free_coherent(dev, ring_header->size,
  ring_header->v_addr, ring_header->dma_addr);
 
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[Timur Tabi]

On 10/12/2017 11:58 AM, David Miller wrote:

I'm pretty sure that kzalloc does not make that guarantee, and I don't
think dma_alloc_coherent does either.

Both make that guarantee, even when an IOMMU is used.



Ok, Dave, then can you drop this patch?

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[Timur Tabi]

On 10/12/2017 11:20 AM, David Laight wrote:

I'm pretty sure that kzalloc does not make that guarantee, and I don't
think dma_alloc_coherent does either.



dma_alloc_coherent() definitely does.
And I've a driver that relies on it (for 16k blocks).


What about when an IOMMU is used?  The DMA address that gets returned is 
not necessarily the physical address of the memory buffer.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[Timur Tabi]

On 10/12/17 4:30 AM, David Laight wrote:

Isn't the memory allocated by a single kzalloc() call?


dma_alloc_coherenent, actually.


IIRC that guarantees it doesn't cross a power or 2 boundary less than
the size.


I'm pretty sure that kzalloc does not make that guarantee, and I don't 
think dma_alloc_coherent does either.



So if you allocate any size between 4k and 8k it won't cross an odd
4k boundary (etc).

So these checks are entirely pointless.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 4/4] net: qcom/emac: clean up some TX/RX error messages

[Timur Tabi]

Some of the error messages that are printed by the interrupt handlers
are poorly written.  For example, many don't include a device prefix,
so there's no indication that they are EMAC errors.

Also use rate limiting for all messages that could be printed from
interrupt context.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 15 ++-
 drivers/net/ethernet/qualcomm/emac/emac.c   |  8 
 2 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index 29ba37a08372..e8ab512ee7e3 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -68,10 +68,10 @@ static void emac_sgmii_link_init(struct emac_adapter *adpt)
writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
 }
 
-static int emac_sgmii_irq_clear(struct emac_adapter *adpt, u32 irq_bits)
+static int emac_sgmii_irq_clear(struct emac_adapter *adpt, u8 irq_bits)
 {
struct emac_sgmii *phy = >phy;
-   u32 status;
+   u8 status;
 
writel_relaxed(irq_bits, phy->base + EMAC_SGMII_PHY_INTERRUPT_CLEAR);
writel_relaxed(IRQ_GLOBAL_CLEAR, phy->base + EMAC_SGMII_PHY_IRQ_CMD);
@@ -86,9 +86,8 @@ static int emac_sgmii_irq_clear(struct emac_adapter *adpt, 
u32 irq_bits)
  EMAC_SGMII_PHY_INTERRUPT_STATUS,
  status, !(status & irq_bits), 1,
  SGMII_PHY_IRQ_CLR_WAIT_TIME)) {
-   netdev_err(adpt->netdev,
-  "error: failed clear SGMII irq: status:0x%x 
bits:0x%x\n",
-  status, irq_bits);
+   net_err_ratelimited("%s: failed to clear SGMII irq: status:0x%x 
bits:0x%x\n",
+   adpt->netdev->name, status, irq_bits);
return -EIO;
}
 
@@ -109,7 +108,7 @@ static irqreturn_t emac_sgmii_interrupt(int irq, void *data)
 {
struct emac_adapter *adpt = data;
struct emac_sgmii *phy = >phy;
-   u32 status;
+   u8 status;
 
status = readl(phy->base + EMAC_SGMII_PHY_INTERRUPT_STATUS);
status &= SGMII_ISR_MASK;
@@ -139,10 +138,8 @@ static irqreturn_t emac_sgmii_interrupt(int irq, void 
*data)
atomic_set(>decode_error_count, 0);
}
 
-   if (emac_sgmii_irq_clear(adpt, status)) {
-   netdev_warn(adpt->netdev, "failed to clear SGMII interrupt\n");
+   if (emac_sgmii_irq_clear(adpt, status))
schedule_work(>work_thread);
-   }
 
return IRQ_HANDLED;
 }
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index ee6f2d27502c..70c92b649b29 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -148,9 +148,8 @@ static irqreturn_t emac_isr(int _irq, void *data)
goto exit;
 
if (status & ISR_ERROR) {
-   netif_warn(adpt,  intr, adpt->netdev,
-  "warning: error irq status 0x%lx\n",
-  status & ISR_ERROR);
+   net_err_ratelimited("%s: error interrupt 0x%lx\n",
+   adpt->netdev->name, status & ISR_ERROR);
/* reset MAC */
schedule_work(>work_thread);
}
@@ -169,7 +168,8 @@ static irqreturn_t emac_isr(int _irq, void *data)
emac_mac_tx_process(adpt, >tx_q);
 
if (status & ISR_OVER)
-   net_warn_ratelimited("warning: TX/RX overflow\n");
+   net_warn_ratelimited("%s: TX/RX overflow interrupt\n",
+adpt->netdev->name);
 
 exit:
/* enable the interrupt */
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/4] net: qcom/emac: remove unused address arrays

[Timur Tabi]

The EMAC is capable of multiple TX and RX rings, but the driver only
supports one ring for each.  One function had some left-over unused
code that supports multiple rings, but all it did was make the code
harder to read.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 16 +++-
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 3ed9033e56db..9cbb27263742 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -309,22 +309,12 @@ void emac_mac_mode_config(struct emac_adapter *adpt)
 /* Config descriptor rings */
 static void emac_mac_dma_rings_config(struct emac_adapter *adpt)
 {
-   static const unsigned short tpd_q_offset[] = {
-   EMAC_DESC_CTRL_8,EMAC_H1TPD_BASE_ADDR_LO,
-   EMAC_H2TPD_BASE_ADDR_LO, EMAC_H3TPD_BASE_ADDR_LO};
-   static const unsigned short rfd_q_offset[] = {
-   EMAC_DESC_CTRL_2,EMAC_DESC_CTRL_10,
-   EMAC_DESC_CTRL_12,   EMAC_DESC_CTRL_13};
-   static const unsigned short rrd_q_offset[] = {
-   EMAC_DESC_CTRL_5,EMAC_DESC_CTRL_14,
-   EMAC_DESC_CTRL_15,   EMAC_DESC_CTRL_16};
-
/* TPD (Transmit Packet Descriptor) */
writel(upper_32_bits(adpt->tx_q.tpd.dma_addr),
   adpt->base + EMAC_DESC_CTRL_1);
 
writel(lower_32_bits(adpt->tx_q.tpd.dma_addr),
-  adpt->base + tpd_q_offset[0]);
+  adpt->base + EMAC_DESC_CTRL_8);
 
writel(adpt->tx_q.tpd.count & TPD_RING_SIZE_BMSK,
   adpt->base + EMAC_DESC_CTRL_9);
@@ -334,9 +324,9 @@ static void emac_mac_dma_rings_config(struct emac_adapter 
*adpt)
   adpt->base + EMAC_DESC_CTRL_0);
 
writel(lower_32_bits(adpt->rx_q.rfd.dma_addr),
-  adpt->base + rfd_q_offset[0]);
+  adpt->base + EMAC_DESC_CTRL_2);
writel(lower_32_bits(adpt->rx_q.rrd.dma_addr),
-  adpt->base + rrd_q_offset[0]);
+  adpt->base + EMAC_DESC_CTRL_5);
 
writel(adpt->rx_q.rfd.count & RFD_RING_SIZE_BMSK,
   adpt->base + EMAC_DESC_CTRL_3);
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 0/4] net: qcom/emac: various minor fixes

[Timur Tabi]

A set of patches for 4.15 that clean up some code, apply minors fixes,
and so on.  Some of the code also prepares the driver for a future 
version of the EMAC controller.

Timur Tabi (4):
  net: qcom/emac: specify the correct DMA mask
  net: qcom/emac: remove unused address arrays
  net: qcom/emac: enforce DMA address restrictions
  net: qcom/emac: clean up some TX/RX error messages

 drivers/net/ethernet/qualcomm/emac/emac-mac.c   | 55 -
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 15 +++
 drivers/net/ethernet/qualcomm/emac/emac.c   | 25 ---
 3 files changed, 41 insertions(+), 54 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 3/4] net: qcom/emac: enforce DMA address restrictions

[Timur Tabi]

The EMAC has a restriction that the upper 32 bits of the base addresses
for the RFD and RRD rings must be the same.  The ensure that restriction,
we allocate twice the space for the RRD and locate it at an appropriate
address.

We also re-arrange the allocations so that invalid addresses are even
less likely.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 39 ---
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 9cbb27263742..0f5ece5d9507 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -734,6 +734,11 @@ static int emac_rx_descs_alloc(struct emac_adapter *adpt)
rx_q->rrd.size = rx_q->rrd.count * (adpt->rrd_size * 4);
rx_q->rfd.size = rx_q->rfd.count * (adpt->rfd_size * 4);
 
+   /* Check if the RRD and RFD are aligned properly, and if not, adjust. */
+   if (upper_32_bits(ring_header->dma_addr) !=
+   upper_32_bits(ring_header->dma_addr + ALIGN(rx_q->rrd.size, 8)))
+   ring_header->used = ALIGN(rx_q->rrd.size, 8);
+
rx_q->rrd.dma_addr = ring_header->dma_addr + ring_header->used;
rx_q->rrd.v_addr   = ring_header->v_addr + ring_header->used;
ring_header->used += ALIGN(rx_q->rrd.size, 8);
@@ -767,11 +772,18 @@ int emac_mac_rx_tx_rings_alloc_all(struct emac_adapter 
*adpt)
 
/* Ring DMA buffer. Each ring may need up to 8 bytes for alignment,
 * hence the additional padding bytes are allocated.
+*
+* Also double the memory allocated for the RRD so that we can
+* re-align it if necessary.  The EMAC has a restriction that the
+* upper 32 bits of the base addresses for the RFD and RRD rings
+* must be the same.  It is extremely unlikely that this is not the
+* case, since the rings are only a few KB in size.  However, we
+* need to check for this anyway, and if the two rings are not
+* compliant, then we re-align.
 */
-   ring_header->size = num_tx_descs * (adpt->tpd_size * 4) +
-   num_rx_descs * (adpt->rfd_size * 4) +
-   num_rx_descs * (adpt->rrd_size * 4) +
-   8 + 2 * 8; /* 8 byte per one Tx and two Rx rings */
+   ring_header->size = ALIGN(num_tx_descs * (adpt->tpd_size * 4), 8) +
+   ALIGN(num_rx_descs * (adpt->rfd_size * 4), 8) +
+   ALIGN(num_rx_descs * (adpt->rrd_size * 4), 8) * 2;
 
ring_header->used = 0;
ring_header->v_addr = dma_zalloc_coherent(dev, ring_header->size,
@@ -780,26 +792,23 @@ int emac_mac_rx_tx_rings_alloc_all(struct emac_adapter 
*adpt)
if (!ring_header->v_addr)
return -ENOMEM;
 
-   ring_header->used = ALIGN(ring_header->dma_addr, 8) -
-   ring_header->dma_addr;
-
-   ret = emac_tx_q_desc_alloc(adpt, >tx_q);
-   if (ret) {
-   netdev_err(adpt->netdev, "error: Tx Queue alloc failed\n");
-   goto err_alloc_tx;
-   }
-
ret = emac_rx_descs_alloc(adpt);
if (ret) {
netdev_err(adpt->netdev, "error: Rx Queue alloc failed\n");
goto err_alloc_rx;
}
 
+   ret = emac_tx_q_desc_alloc(adpt, >tx_q);
+   if (ret) {
+   netdev_err(adpt->netdev, "transmit queue allocation failed\n");
+   goto err_alloc_tx;
+   }
+
return 0;
 
-err_alloc_rx:
-   emac_tx_q_bufs_free(adpt);
 err_alloc_tx:
+   emac_rx_q_bufs_free(adpt);
+err_alloc_rx:
dma_free_coherent(dev, ring_header->size,
  ring_header->v_addr, ring_header->dma_addr);
 
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/4] net: qcom/emac: specify the correct DMA mask

[Timur Tabi]

The 64/32-bit DMA mask hackery in the EMAC driver is not actually necessary,
and is technically not accurate.  The EMAC hardware is limted to a 45-bit
DMA address.  Although no EMAC-enabled system can have that much DDR,
an IOMMU could possible provide a larger address.  Rather than play games
with the DMA mappings, the driver should provide a correct value and
trust the DMA/IOMMU layers to do the right thing.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac.c | 17 -
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index f477ba29c569..ee6f2d27502c 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -615,20 +615,11 @@ static int emac_probe(struct platform_device *pdev)
u32 reg;
int ret;
 
-   /* The EMAC itself is capable of 64-bit DMA, so try that first. */
-   ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(64));
+   /* The TPD buffer address is limited to 45 bits. */
+   ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(45));
if (ret) {
-   /* Some platforms may restrict the EMAC's address bus to less
-* then the size of DDR. In this case, we need to try a
-* smaller mask.  We could try every possible smaller mask,
-* but that's overkill.  Instead, just fall to 32-bit, which
-* should always work.
-*/
-   ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32));
-   if (ret) {
-   dev_err(>dev, "could not set DMA mask\n");
-   return ret;
-   }
+   dev_err(>dev, "could not set DMA mask\n");
+   return ret;
}
 
netdev = alloc_etherdev(sizeof(struct emac_adapter));
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] net: qcom/emac: make function emac_isr static

[Timur Tabi]

On 10/05/2017 04:10 AM, Colin King wrote:

From: Colin Ian King

The function emac_isr is local to the source and does not need to
be in global scope, so make it static.

Cleans up sparse warnings:
symbol 'emac_isr' was not declared. Should it be static?

Signed-off-by: Colin Ian King


ACK

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] [for 4.14] net: qcom/emac: specify the correct size when mapping a DMA buffer

[Timur Tabi]

When mapping the RX DMA buffers, the driver was accidentally specifying
zero for the buffer length.  Under normal circumstances, SWIOTLB does not
need to allocate a bounce buffer, so the address is just mapped without
checking the size field.  This is why the error was not detected earlier.

Fixes: b9b17debc69d ("net: emac: emac gigabit ethernet controller driver")
Cc: sta...@vger.kernel.org
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 0ea3ca09c689..3ed9033e56db 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -898,7 +898,8 @@ static void emac_mac_rx_descs_refill(struct emac_adapter 
*adpt,
 
curr_rxbuf->dma_addr =
dma_map_single(adpt->netdev->dev.parent, skb->data,
-  curr_rxbuf->length, DMA_FROM_DEVICE);
+  adpt->rxbuf_size, DMA_FROM_DEVICE);
+
ret = dma_mapping_error(adpt->netdev->dev.parent,
curr_rxbuf->dma_addr);
if (ret) {
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] [RESEND][for 4.14] net: qcom/emac: add software control for pause frame mode

[Timur Tabi]

The EMAC has the option of sending only a single pause frame when
flow control is enabled and the RX queue is full.  Although sending
only one pause frame has little value, this would allow admins to
enable automatic flow control without having to worry about the EMAC
flooding nearby switches with pause frames if the kernel hangs.

The option is enabled by using the single-pause-mode private flag.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-ethtool.c | 30 +++
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 22 +
 drivers/net/ethernet/qualcomm/emac/emac.c |  3 +++
 drivers/net/ethernet/qualcomm/emac/emac.h |  3 +++
 4 files changed, 58 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c 
b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
index bbe24639aa5a..c8c6231b87f3 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
@@ -88,6 +88,8 @@ static void emac_set_msglevel(struct net_device *netdev, u32 
data)
 static int emac_get_sset_count(struct net_device *netdev, int sset)
 {
switch (sset) {
+   case ETH_SS_PRIV_FLAGS:
+   return 1;
case ETH_SS_STATS:
return EMAC_STATS_LEN;
default:
@@ -100,6 +102,10 @@ static void emac_get_strings(struct net_device *netdev, 
u32 stringset, u8 *data)
unsigned int i;
 
switch (stringset) {
+   case ETH_SS_PRIV_FLAGS:
+   strcpy(data, "single-pause-mode");
+   break;
+
case ETH_SS_STATS:
for (i = 0; i < EMAC_STATS_LEN; i++) {
strlcpy(data, emac_ethtool_stat_strings[i],
@@ -230,6 +236,27 @@ static int emac_get_regs_len(struct net_device *netdev)
return EMAC_MAX_REG_SIZE * sizeof(u32);
 }
 
+#define EMAC_PRIV_ENABLE_SINGLE_PAUSE  BIT(0)
+
+static int emac_set_priv_flags(struct net_device *netdev, u32 flags)
+{
+   struct emac_adapter *adpt = netdev_priv(netdev);
+
+   adpt->single_pause_mode = !!(flags & EMAC_PRIV_ENABLE_SINGLE_PAUSE);
+
+   if (netif_running(netdev))
+   return emac_reinit_locked(adpt);
+
+   return 0;
+}
+
+static u32 emac_get_priv_flags(struct net_device *netdev)
+{
+   struct emac_adapter *adpt = netdev_priv(netdev);
+
+   return adpt->single_pause_mode ? EMAC_PRIV_ENABLE_SINGLE_PAUSE : 0;
+}
+
 static const struct ethtool_ops emac_ethtool_ops = {
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
@@ -253,6 +280,9 @@ static int emac_get_regs_len(struct net_device *netdev)
 
.get_regs_len= emac_get_regs_len,
.get_regs= emac_get_regs,
+
+   .set_priv_flags = emac_set_priv_flags,
+   .get_priv_flags = emac_get_priv_flags,
 };
 
 void emac_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index bcd4708b3745..0ea3ca09c689 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -551,6 +551,28 @@ static void emac_mac_start(struct emac_adapter *adpt)
mac &= ~(HUGEN | VLAN_STRIP | TPAUSE | SIMR | HUGE | MULTI_ALL |
 DEBUG_MODE | SINGLE_PAUSE_MODE);
 
+   /* Enable single-pause-frame mode if requested.
+*
+* If enabled, the EMAC will send a single pause frame when the RX
+* queue is full.  This normally leads to packet loss because
+* the pause frame disables the remote MAC only for 33ms (the quanta),
+* and then the remote MAC continues sending packets even though
+* the RX queue is still full.
+*
+* If disabled, the EMAC sends a pause frame every 31ms until the RX
+* queue is no longer full.  Normally, this is the preferred
+* method of operation.  However, when the system is hung (e.g.
+* cores are halted), the EMAC interrupt handler is never called
+* and so the RX queue fills up quickly and stays full.  The resuling
+* non-stop "flood" of pause frames sometimes has the effect of
+* disabling nearby switches.  In some cases, other nearby switches
+* are also affected, shutting down the entire network.
+*
+* The user can enable or disable single-pause-frame mode
+* via ethtool.
+*/
+   mac |= adpt->single_pause_mode ? SINGLE_PAUSE_MODE : 0;
+
writel_relaxed(csr1, adpt->csr + EMAC_EMAC_WRAPPER_CSR1);
 
writel_relaxed(mac, adpt->base + EMAC_MAC_CTRL);
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index 60850bfa3d32..759543512117 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/

Re: [PATCH 2/2] net: qcom/emac: add software control for pause frame mode

[Timur Tabi]

On 08/01/2017 04:37 PM, Timur Tabi wrote:

The EMAC has the option of sending only a single pause frame when
flow control is enabled and the RX queue is full.  Although sending
only one pause frame has little value, this would allow admins to
enable automatic flow control without having to worry about the EMAC
flooding nearby switches with pause frames if the kernel hangs.

The option is enabled by using the single-pause-mode private flag.

Signed-off-by: Timur Tabi<ti...@codeaurora.org>


Dave,

I don't see this patch in net-next.  Can you pick it up for 4.14?

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 8/2/17 6:15 PM, David Miller wrote:


So you don't want to run a proper watchdog on your systems?
You want them to just hang there and wait for someone to
notice instead?


This is for internal development.  We noticed the problem first during 
debugging, when we would halt a core for more than a couple minutes. 
Then the entire lab network would go down.


--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 08/02/2017 01:35 PM, David Miller wrote:

Again, any serious installation will have a system watchdog enabled
which will break the pause frame bomb.


Oh well.  I guess I'll have to carry this patch internally.

What about patch #2?

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 08/02/2017 12:54 PM, David Miller wrote:

And if this kind of thing matters to the user, they will have a
software or hardware watchdog driver enabled to break out of this
situation.


The problem is that the user is not going to expect that the EMAC can 
disable the nearby switch(es) when the kernel is hung and not rebooted 
quickly enough.  Internally, this bug/feature has caused quite a bit of 
mayhem, so the problem is real.  No cares about enabling flow control -- 
it just happens to be enabled on some systems where the switch agrees to 
it.  So random individuals can't debug the hardware because suddenly the 
EMAC has gone haywire and disabled the local network.



Turning off flow control by default has so many negative ramifications
and don't try to convince me that users will be "aware" of this and
turn it back on.


What are the negative ramifications?  It's practically impossible to 
overload the chip such that it can't process the incoming packets fast 
enough.  I don't know of any real-world situation where the EMAC needs 
to transmit pause frames.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 08/02/2017 09:51 AM, David Laight wrote:

Sending pause frames just tells the adjacent switch not to send you packets
(because you'll discard them).
Since the idea is to avoid the discards, the switch will buffer the
packets it would have sent.
The buffers in the switch then fill up with packets it isn't sending you.


I was under the impression that the switch forwards the pause frames to 
other devices, so that the transmitting NIC can stop sending the data, 
but your explanation makes a lot more sense.  If the EMAC never stops 
sending pause frames, then the switch's buffers will fill up, disabling 
all other devices.  If the switch does not have per-port buffers, then 
it makes sense when the buffer is full, it blocks all ports.



The switch then runs out of buffers, it has 2 choices:
1) Throw the packets away.
2) Send 'pause' frames to the sources.
If it sends 'pause' frames the entire network will very quickly lock up.
If it discards the packets they might as well have been discarded by the
receiving MAC.

Doesn't this mean that pause frames are 99.999% useless??


Pause frames are intended for situations where the receiving CPU is 
temporarily overwhelmed and just needs a second or two to resume 
processing incoming packets.  That makes sense on a dinky single-core 
32-bit CPU.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 8/2/17 8:48 AM, David Laight wrote:

If the nearby switches cannot handle pause frames, then the MAC shouldn't
be sending them at all.


There's no way for me to know whether the switches can handle the pause 
frames or not.  You would think that sending one multicast pause frame 
ever 33ms would not overload a switch, but in our lab it does.


This is why I changed the default to disable sending pause frames.


They

I suspect that they should only ever be sent if the phy autonegotiation
indicates that they are supported.


Or if you manually enable them.  That's why I changed to default: the 
MAC is now configured by default to accept pause frames but not send them.


--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 8/1/17 9:58 PM, Andrew Lunn wrote:

  The PHY does not participate directly in flow control/pause frames except by
  making sure that the SUPPORTED_Pause and SUPPORTED_AsymPause bits are set in
  MII_ADVERTISE to indicate towards the link partner that the Ethernet MAC
  controller supports such a thing. Since flow control/pause frames generation
  involves the Ethernet MAC driver, it is recommended that this driver takes 
care
  of properly indicating advertisement and support for such features by setting
  the SUPPORTED_Pause and SUPPORTED_AsymPause bits accordingly. This can be done
  either before or after phy_connect() and/or as a result of implementing the
  ethtool::set_pauseparam feature.

So just check if the MAC driver is setting SUPPORTED_Pause and
SUPPORTED_AsymPause.


I think this was covered in this thread: 
http://www.spinics.net/lists/netdev/msg408978.html


--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 8/1/17 6:15 PM, Andrew Lunn wrote:

Pause frames are something you can auto-negotiate at the PHY
level. Should you also be clearing some bits in the phydev, so the
peer knows pause frames are not supported?


When pause frame autonegotiation is enabled in the driver, that only 
means that the driver looks at what the PHY has autonegotiated, and then 
configures the MAC to match that.


The driver doesn't touch the PHY at all.  It leaves all that to phylib.

Now if autonegotiation is disabled in the driver, then it just 
hard-codes those TX/RX settings in the driver.  Are you saying I should 
program the PHY at the point to disable autonegotiation on the PHY 
level?  If so, then I don't know how to do that.  I just assumed that 
the MAC never tells the PHY what to do.


--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 08/01/2017 04:55 PM, Florian Fainelli wrote:

This is not specific to your EMAC, a lot of adapters have this problem
actually.

I wonder if it would make sense to reach for a broader solution where we
could have a networking stack panic/oops notifier which will actively
clean up the active network devices' RX queue(s) and if tx_pause was
enabled, disable it. We could have drivers announce themselves as
needing this either via NETIF_F_* feature bit or some other private flag.


Unfortunately, the problem occurs only when Linux hangs, to the point 
where the driver's interrupt handlers are blocked.  The RX queue is 256 
entries, and the processor has 48 cores, so the EMAC is never going to 
send pause frames in any real-world situation.


The only time I've seen pause frames sent out is in the lab when I halt 
the cores with a hardware debugger, and only if I have enough network 
traffic that the EMAC picks up.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] net: qcom/emac: add software control for pause frame mode

[Timur Tabi]

On 08/01/2017 04:51 PM, Florian Fainelli wrote:


A few adapters (bcmgenet, bcmsysport) support configuring the pause
quanta so it would not be inconceivable to try to update
ethtool_pauseparam to include additional fields such as:


Wouldn't this require a change to the user space tool?


- number of pause frames to send where we define an arbitrary high
default value (e.g: 0x), N < 0x is something drivers can test
for whether they support it, and 0 is only valid if pause is already
disabled

- pause quanta (16-bits)

Private flags are not usually that great and there could be more
adapters capable of doing the same pause frame number configuration, but
since there is no available knob it's hard to know.


Well, for the EMAC, the quanta in this case would be either 1 or 
infinite.  For other devices, it could be any combination of values.  In 
a future revision of the hardware, we might support a variable quanta. 
And I suspect that some devices measure the quanta in time, not count.


How would the user know what the acceptable values are?  If I set the 
quanta to 10 via user space, and my driver truncates that to 1, I don't 
think that would be acceptable.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

The EMAC has a curious qwirk when RX flow control is enabled and the
kernel hangs.  With the kernel hung, the EMAC's RX queue soon fills.
If RX flow control is enabled, the EMAC will then send a non-stop
stream of pause frames until the system is reset.  The EMAC does not
have a built-in watchdog.

In various tests, the pause frame stream sometimes overloads nearby
switches, effectively disabling the network.  Since the RX queue is
large and the host processor is more than capable of handling incoming
packets quickly, the only time the EMAC will send any pause frames is
when the kernel is hung and unrecoverable.

To avoid all these problems, we disable flow control autonegotiation
by default, and only enable receiving pause frames.

Cc: sta...@vger.kernel.org # 4.11.x
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index 60850bfa3d32..475c0ea29235 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -441,8 +441,13 @@ static void emac_init_adapter(struct emac_adapter *adpt)
/* others */
adpt->preamble = EMAC_PREAMBLE_DEF;
 
-   /* default to automatic flow control */
-   adpt->automatic = true;
+   /* Disable transmission of pause frames by default, to avoid the
+* risk of a pause frame flood that can occur if the kernel hangs.
+* We still want to be able to respond to them, however.
+*/
+   adpt->automatic = false;
+   adpt->tx_flow_control = false;
+   adpt->rx_flow_control = true;
 }
 
 /* Get the clock */
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 0/2] net: qcom/emac: fixes for pause frame floods

[Timur Tabi]

The first patch is for 4.13.  It's changes the default behavior of the
EMAC driver so that it doesn't send pause frames unless the user 
enables them.

The second patch is for 4.14, but it can be applied to 4.13 if you
want.  It adds the ability for the user to enable a special "single
pause frame" mode that could be useful in some situations.

Timur Tabi (2):
  [for 4.13] net: qcom/emac: disable flow control autonegotiation by
default
  net: qcom/emac: add software control for pause frame mode

 drivers/net/ethernet/qualcomm/emac/emac-ethtool.c | 30 +++
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 22 +
 drivers/net/ethernet/qualcomm/emac/emac.c | 12 +++--
 drivers/net/ethernet/qualcomm/emac/emac.h |  3 +++
 4 files changed, 65 insertions(+), 2 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/2] net: qcom/emac: add software control for pause frame mode

[Timur Tabi]

The EMAC has the option of sending only a single pause frame when
flow control is enabled and the RX queue is full.  Although sending
only one pause frame has little value, this would allow admins to
enable automatic flow control without having to worry about the EMAC
flooding nearby switches with pause frames if the kernel hangs.

The option is enabled by using the single-pause-mode private flag.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-ethtool.c | 30 +++
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 22 +
 drivers/net/ethernet/qualcomm/emac/emac.c |  3 +++
 drivers/net/ethernet/qualcomm/emac/emac.h |  3 +++
 4 files changed, 58 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c 
b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
index bbe24639aa5a..c8c6231b87f3 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
@@ -88,6 +88,8 @@ static void emac_set_msglevel(struct net_device *netdev, u32 
data)
 static int emac_get_sset_count(struct net_device *netdev, int sset)
 {
switch (sset) {
+   case ETH_SS_PRIV_FLAGS:
+   return 1;
case ETH_SS_STATS:
return EMAC_STATS_LEN;
default:
@@ -100,6 +102,10 @@ static void emac_get_strings(struct net_device *netdev, 
u32 stringset, u8 *data)
unsigned int i;
 
switch (stringset) {
+   case ETH_SS_PRIV_FLAGS:
+   strcpy(data, "single-pause-mode");
+   break;
+
case ETH_SS_STATS:
for (i = 0; i < EMAC_STATS_LEN; i++) {
strlcpy(data, emac_ethtool_stat_strings[i],
@@ -230,6 +236,27 @@ static int emac_get_regs_len(struct net_device *netdev)
return EMAC_MAX_REG_SIZE * sizeof(u32);
 }
 
+#define EMAC_PRIV_ENABLE_SINGLE_PAUSE  BIT(0)
+
+static int emac_set_priv_flags(struct net_device *netdev, u32 flags)
+{
+   struct emac_adapter *adpt = netdev_priv(netdev);
+
+   adpt->single_pause_mode = !!(flags & EMAC_PRIV_ENABLE_SINGLE_PAUSE);
+
+   if (netif_running(netdev))
+   return emac_reinit_locked(adpt);
+
+   return 0;
+}
+
+static u32 emac_get_priv_flags(struct net_device *netdev)
+{
+   struct emac_adapter *adpt = netdev_priv(netdev);
+
+   return adpt->single_pause_mode ? EMAC_PRIV_ENABLE_SINGLE_PAUSE : 0;
+}
+
 static const struct ethtool_ops emac_ethtool_ops = {
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
@@ -253,6 +280,9 @@ static int emac_get_regs_len(struct net_device *netdev)
 
.get_regs_len= emac_get_regs_len,
.get_regs= emac_get_regs,
+
+   .set_priv_flags = emac_set_priv_flags,
+   .get_priv_flags = emac_get_priv_flags,
 };
 
 void emac_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index bcd4708b3745..0ea3ca09c689 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -551,6 +551,28 @@ static void emac_mac_start(struct emac_adapter *adpt)
mac &= ~(HUGEN | VLAN_STRIP | TPAUSE | SIMR | HUGE | MULTI_ALL |
 DEBUG_MODE | SINGLE_PAUSE_MODE);
 
+   /* Enable single-pause-frame mode if requested.
+*
+* If enabled, the EMAC will send a single pause frame when the RX
+* queue is full.  This normally leads to packet loss because
+* the pause frame disables the remote MAC only for 33ms (the quanta),
+* and then the remote MAC continues sending packets even though
+* the RX queue is still full.
+*
+* If disabled, the EMAC sends a pause frame every 31ms until the RX
+* queue is no longer full.  Normally, this is the preferred
+* method of operation.  However, when the system is hung (e.g.
+* cores are halted), the EMAC interrupt handler is never called
+* and so the RX queue fills up quickly and stays full.  The resuling
+* non-stop "flood" of pause frames sometimes has the effect of
+* disabling nearby switches.  In some cases, other nearby switches
+* are also affected, shutting down the entire network.
+*
+* The user can enable or disable single-pause-frame mode
+* via ethtool.
+*/
+   mac |= adpt->single_pause_mode ? SINGLE_PAUSE_MODE : 0;
+
writel_relaxed(csr1, adpt->csr + EMAC_EMAC_WRAPPER_CSR1);
 
writel_relaxed(mac, adpt->base + EMAC_MAC_CTRL);
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index 475c0ea29235..6f5e858ffbf3 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/

Re: [PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues

[Timur Tabi]

On 7/21/17 8:29 AM, Marc Gonzalez wrote:

I don't understand what you're saying.

It is a correct observation that the code enabling
RGMII RX clock delay is a NOP, since that bit will
always be set at that point.

The spec for the 8035 (I haven't checked for 8030 and 8031,
is that what you meant by "other systems"?) states that
Sel_clk125m_dsp, which is described as:
"Control bit for rgmii interface rx clock delay"
is 1 after HW reset, 1 after SW reset.

So my statement "RX clock delay is enabled at reset"
is universally true. It's not just on some systems.


Ok, taken out of context, the comment doesn't really explain why the 
code is the way it is.  I'm not really happy about the word "assumes". 
Maybe you should add a sentence explaining when the code is NOT a no-op.


--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues

[Timur Tabi]

On 7/21/17 6:25 AM, Marc Gonzalez wrote:

+* NB: This code assumes that RGMII RX clock delay is disabled
+* at reset, but actually, RX clock delay is enabled at reset.


Could we change this to say, "RX clock delay is enabled at reset on some 
systems."?  Otherwise, it implies that the code is wrong 100% of the 
time and should be fixed, not documented.


--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Timur Tabi]

On 07/19/2017 10:31 AM, Marc Gonzalez wrote:

The current code supports enabling RGMII RX and TX clock delays.
The unstated assumption is that these settings are disabled by
default at reset, which is not the case.

RX clock delay is enabled at reset. And TX clock delay "survives"
across SW resets. Thus, if the bootloader enables TX clock delay,
it will remain enabled at reset in Linux.

Provide disable functions to configure the RGMII clock delays
exactly as specified in the fwspec.


I only use SGMII mode, and I tested and can confirm that this patch does 
not break SGMII, so:


Acked-by: Timur Tabi <ti...@codeaurora.org>

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] [for 4.13] net: qcom/emac: fix double free of SGMII IRQ during shutdown

[Timur Tabi]

If the interface is not up, then don't try to close it during a
shutdown.  This avoids possible double free of the IRQ, which
can happen during a shutdown.

Fixes: 03eb3eb4d4d5 ("net: qcom/emac: add shutdown function")
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index 746d94e..60850bf 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -766,11 +766,13 @@ static void emac_shutdown(struct platform_device *pdev)
struct emac_adapter *adpt = netdev_priv(netdev);
struct emac_sgmii *sgmii = >phy;
 
-   /* Closing the SGMII turns off its interrupts */
-   sgmii->close(adpt);
+   if (netdev->flags & IFF_UP) {
+   /* Closing the SGMII turns off its interrupts */
+   sgmii->close(adpt);
 
-   /* Resetting the MAC turns off all DMA and its interrupts */
-   emac_mac_reset(adpt);
+   /* Resetting the MAC turns off all DMA and its interrupts */
+   emac_mac_reset(adpt);
+   }
 }
 
 static struct platform_driver emac_platform_driver = {
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 3/3] net: qcom/emac: add support for emulation systems

[Timur Tabi]

On emulation systems, the EMAC's internal PHY ("SGMII") is not present,
but is not needed for network functionality.  So just display a warning
message and ignore the SGMII.

Tested-by: Philip Elcan <pel...@codeaurora.org>
Tested-by: Adam Wallis <awal...@codeaurora.org>
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 23 +--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index 18c184e..29ba37a 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -297,6 +297,14 @@ static int emac_sgmii_acpi_match(struct device *dev, void 
*data)
{}
 };
 
+/* Dummy function for systems without an internal PHY. This avoids having
+ * to check for NULL pointers before calling the functions.
+ */
+static int emac_sgmii_dummy(struct emac_adapter *adpt)
+{
+   return 0;
+}
+
 int emac_sgmii_config(struct platform_device *pdev, struct emac_adapter *adpt)
 {
struct platform_device *sgmii_pdev = NULL;
@@ -311,8 +319,19 @@ int emac_sgmii_config(struct platform_device *pdev, struct 
emac_adapter *adpt)
emac_sgmii_acpi_match);
 
if (!dev) {
-   dev_err(>dev, "cannot find internal phy node\n");
-   return -ENODEV;
+   dev_warn(>dev, "cannot find internal phy node\n");
+   /* There is typically no internal PHY on emulation
+* systems, so if we can't find the node, assume
+* we are on an emulation system and stub-out
+* support for the internal PHY.  These systems only
+* use ACPI.
+*/
+   phy->open = emac_sgmii_dummy;
+   phy->close = emac_sgmii_dummy;
+   phy->link_up = emac_sgmii_dummy;
+   phy->link_down = emac_sgmii_dummy;
+
+   return 0;
}
 
sgmii_pdev = to_platform_device(dev);
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/3] net: qcom/emac: add shutdown function

[Timur Tabi]

The shutdown function halts all DMA and interrupts, so that all
operations are discontinued when the system shuts down, e.g. via
kexec or a forced reboot.

Tested-by: Tyler Baicar <tbai...@codeaurora.org>
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index 98a326f..77c5c92 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -762,6 +762,19 @@ static int emac_remove(struct platform_device *pdev)
return 0;
 }
 
+static void emac_shutdown(struct platform_device *pdev)
+{
+   struct net_device *netdev = dev_get_drvdata(>dev);
+   struct emac_adapter *adpt = netdev_priv(netdev);
+   struct emac_sgmii *sgmii = >phy;
+
+   /* Closing the SGMII turns off its interrupts */
+   sgmii->close(adpt);
+
+   /* Resetting the MAC turns off all DMA and its interrupts */
+   emac_mac_reset(adpt);
+}
+
 static struct platform_driver emac_platform_driver = {
.probe  = emac_probe,
.remove = emac_remove,
@@ -770,6 +783,7 @@ static int emac_remove(struct platform_device *pdev)
.of_match_table = emac_dt_match,
.acpi_match_table = ACPI_PTR(emac_acpi_match),
},
+   .shutdown = emac_shutdown,
 };
 
 module_platform_driver(emac_platform_driver);
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/3][v2] net: qcom/emac: do not reset the EMAC during initialization

[Timur Tabi]

On ACPI systems, the driver depends on firmware pre-initializing the
EMAC because we don't have access to the clocks, and the EMAC has specific
clock programming requirements.  Therefore, we don't want to reset the
EMAC while we are completing the initialization.

Tested-by: Richard Ruigrok <rruig...@codeaurora.org>
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
v2: improve the patch description.

 drivers/net/ethernet/qualcomm/emac/emac.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index 77c5c92..746d94e 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -683,8 +683,6 @@ static int emac_probe(struct platform_device *pdev)
goto err_undo_mdiobus;
}
 
-   emac_mac_reset(adpt);
-
/* set hw features */
netdev->features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM |
NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_HW_VLAN_CTAG_RX |
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 0/3][v2] net: qcom/emac: various minor improvements

[Timur Tabi]

A collection of minor fixes and features to the Qualcomm Technologies
EMAC network driver.

Timur Tabi (3):
  net: qcom/emac: add shutdown function
  [v2] net: qcom/emac: do not reset the EMAC during initialization
  net: qcom/emac: add support for emulation systems

 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 23 +--
 drivers/net/ethernet/qualcomm/emac/emac.c   | 16 ++--
 2 files changed, 35 insertions(+), 4 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/3] net: qcom/emac: do not reset the EMAC during initialization

[Timur Tabi]

On 06/23/2017 01:00 PM, David Miller wrote:

What if the boot loader or something else left the chip in
a weird state?


We depend on the boot loader leaving the NIC in a very specific state 
already, otherwise the driver can't initialize the hardware.  The 
firmware has to pre-initialize the EMAC for us.


Not only that, but the driver was resetting the MAC *after* programming 
the clocks (on non-ACPI systems) and initializing both PHYs.



I'm not applying this.

If it's correct, the explanation in this commit message need
to be imporved.  The change must be better justified.


Since this is for ACPI systems, I could do this:

if (!has_acpi_companion(>dev))
emac_mac_reset(adpt);

But at the very least, the call should be moved to earlier in the function.

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 3/3] net: qcom/emac: add support for emulation systems

[Timur Tabi]

On emulation systems, the EMAC's internal PHY ("SGMII") is not present,
but is not needed for network functionality.  So just display a warning
message and ignore the SGMII.

Tested-by: Philip Elcan <pel...@codeaurora.org>
Tested-by: Adam Wallis <awal...@codeaurora.org>
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 23 +--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index 18c184e..29ba37a 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -297,6 +297,14 @@ static int emac_sgmii_acpi_match(struct device *dev, void 
*data)
{}
 };
 
+/* Dummy function for systems without an internal PHY. This avoids having
+ * to check for NULL pointers before calling the functions.
+ */
+static int emac_sgmii_dummy(struct emac_adapter *adpt)
+{
+   return 0;
+}
+
 int emac_sgmii_config(struct platform_device *pdev, struct emac_adapter *adpt)
 {
struct platform_device *sgmii_pdev = NULL;
@@ -311,8 +319,19 @@ int emac_sgmii_config(struct platform_device *pdev, struct 
emac_adapter *adpt)
emac_sgmii_acpi_match);
 
if (!dev) {
-   dev_err(>dev, "cannot find internal phy node\n");
-   return -ENODEV;
+   dev_warn(>dev, "cannot find internal phy node\n");
+   /* There is typically no internal PHY on emulation
+* systems, so if we can't find the node, assume
+* we are on an emulation system and stub-out
+* support for the internal PHY.  These systems only
+* use ACPI.
+*/
+   phy->open = emac_sgmii_dummy;
+   phy->close = emac_sgmii_dummy;
+   phy->link_up = emac_sgmii_dummy;
+   phy->link_down = emac_sgmii_dummy;
+
+   return 0;
}
 
sgmii_pdev = to_platform_device(dev);
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/3] net: qcom/emac: add shutdown function

[Timur Tabi]

The shutdown function halts all DMA and interrupts, so that all
operations are discontinued when the system shuts down, e.g. via
kexec or a forced reboot.

Tested-by: Tyler Baicar <tbai...@codeaurora.org>
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index 98a326f..77c5c92 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -762,6 +762,19 @@ static int emac_remove(struct platform_device *pdev)
return 0;
 }
 
+static void emac_shutdown(struct platform_device *pdev)
+{
+   struct net_device *netdev = dev_get_drvdata(>dev);
+   struct emac_adapter *adpt = netdev_priv(netdev);
+   struct emac_sgmii *sgmii = >phy;
+
+   /* Closing the SGMII turns off its interrupts */
+   sgmii->close(adpt);
+
+   /* Resetting the MAC turns off all DMA and its interrupts */
+   emac_mac_reset(adpt);
+}
+
 static struct platform_driver emac_platform_driver = {
.probe  = emac_probe,
.remove = emac_remove,
@@ -770,6 +783,7 @@ static int emac_remove(struct platform_device *pdev)
.of_match_table = emac_dt_match,
.acpi_match_table = ACPI_PTR(emac_acpi_match),
},
+   .shutdown = emac_shutdown,
 };
 
 module_platform_driver(emac_platform_driver);
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 0/3] net: qcom/emac: various minor improvements

[Timur Tabi]

A collection of minor fixes and features to the Qualcomm Technologies
EMAC network driver.

Timur Tabi (3):
  net: qcom/emac: add shutdown function
  net: qcom/emac: do not reset the EMAC during initialization
  net: qcom/emac: add support for emulation systems

 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 23 +--
 drivers/net/ethernet/qualcomm/emac/emac.c   | 16 ++--
 2 files changed, 35 insertions(+), 4 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/3] net: qcom/emac: do not reset the EMAC during initialization

[Timur Tabi]

It doesn't make sense to reset the EMAC in the middle of initializing
it during the probe.

Tested-by: Richard Ruigrok <rruig...@codeaurora.org>
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index 77c5c92..746d94e 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -683,8 +683,6 @@ static int emac_probe(struct platform_device *pdev)
goto err_undo_mdiobus;
}
 
-   emac_mac_reset(adpt);
-
/* set hw features */
netdev->features = NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM |
NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_HW_VLAN_CTAG_RX |
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[for 4.12] net: qcom/emac: do not use hardware mdio automatic polling

[Timur Tabi]

Use software polling (PHY_POLL) to check for link state changes instead
of relying on the EMAC's hardware polling feature.  Some PHY drivers
are unable to get a functioning link because the HW polling is not
robust enough.

The EMAC is able to poll the PHY on the MDIO bus looking for link state
changes (via the Link Status bit in the Status Register at address 0x1).
When the link state changes, the EMAC triggers an interrupt and tells the
driver what the new state is.  The feature eliminates the need for
software to poll the MDIO bus.

Unfortunately, this feature is incompatible with phylib, because it
ignores everything that the PHY core and PHY drivers are trying to do.
In particular:

1. It assumes a compatible register set, so PHYs with different registers
   may not work.

2. It doesn't allow for hardware errata that have work-arounds implemented
   in the PHY driver.

3. It doesn't support multiple register pages. If the PHY core switches
   the register set to another page, the EMAC won't know the page has
   changed and will still attempt to read the same PHY register.

4. It only checks the copper side of the link, not the SGMII side.  Some
   PHY drivers (e.g. at803x) may also check the SGMII side, and
   report the link as not ready during autonegotiation if the SGMII link
   is still down.  Phylib then waits for another interrupt to query
   the PHY again, but the EMAC won't send another interrupt because it
   thinks the link is up.

Cc: sta...@vger.kernel.org # 4.11.x
Tested-by: Manoj Iyer <manoj.i...@canonical.com>
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c |  2 +-
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 75 ++-
 drivers/net/ethernet/qualcomm/emac/emac.c | 22 +---
 3 files changed, 6 insertions(+), 93 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index e635605..1c0dbaa 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -932,7 +932,7 @@ int emac_mac_up(struct emac_adapter *adpt)
emac_mac_config(adpt);
emac_mac_rx_descs_refill(adpt, >rx_q);
 
-   adpt->phydev->irq = PHY_IGNORE_INTERRUPT;
+   adpt->phydev->irq = PHY_POLL;
ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
 PHY_INTERFACE_MODE_SGMII);
if (ret) {
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c 
b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 441c1936..18461fc 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -13,15 +13,11 @@
 /* Qualcomm Technologies, Inc. EMAC PHY Controller driver.
  */
 
-#include 
-#include 
-#include 
 #include 
 #include 
 #include 
 #include 
 #include "emac.h"
-#include "emac-mac.h"
 
 /* EMAC base register offsets */
 #define EMAC_MDIO_CTRL0x001414
@@ -52,62 +48,10 @@
 
 #define MDIO_WAIT_TIMES   1000
 
-#define EMAC_LINK_SPEED_DEFAULT (\
-   EMAC_LINK_SPEED_10_HALF  |\
-   EMAC_LINK_SPEED_10_FULL  |\
-   EMAC_LINK_SPEED_100_HALF |\
-   EMAC_LINK_SPEED_100_FULL |\
-   EMAC_LINK_SPEED_1GB_FULL)
-
-/**
- * emac_phy_mdio_autopoll_disable() - disable mdio autopoll
- * @adpt: the emac adapter
- *
- * The autopoll feature takes over the MDIO bus.  In order for
- * the PHY driver to be able to talk to the PHY over the MDIO
- * bus, we need to temporarily disable the autopoll feature.
- */
-static int emac_phy_mdio_autopoll_disable(struct emac_adapter *adpt)
-{
-   u32 val;
-
-   /* disable autopoll */
-   emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, MDIO_AP_EN, 0);
-
-   /* wait for any mdio polling to complete */
-   if (!readl_poll_timeout(adpt->base + EMAC_MDIO_CTRL, val,
-   !(val & MDIO_BUSY), 100, MDIO_WAIT_TIMES * 100))
-   return 0;
-
-   /* failed to disable; ensure it is enabled before returning */
-   emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, 0, MDIO_AP_EN);
-
-   return -EBUSY;
-}
-
-/**
- * emac_phy_mdio_autopoll_disable() - disable mdio autopoll
- * @adpt: the emac adapter
- *
- * The EMAC has the ability to poll the external PHY on the MDIO
- * bus for link state changes.  This eliminates the need for the
- * driver to poll the phy.  If if the link state does change,
- * the EMAC issues an interrupt on behalf of the PHY.
- */
-static void emac_phy_mdio_autopoll_enable(struct emac_adapter *adpt)
-{
-   emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, 0, MDIO_AP_EN);
-}
-
 static int emac_mdio_read(struct mii_bus *bus, int addr, int regnum)
 {
struct emac_adapter *adpt = bus->priv;
u32 reg;

[for 4.12] net: qcom/emac: do not use hardware mdio automatic polling

[Timur Tabi]

Use software polling (PHY_POLL) to check for link state changes instead
of relying on the EMAC's hardware polling feature.  Some PHY drivers
are unable to get a functioning link because the HW polling is not
robust enough.

The EMAC is able to poll the PHY on the MDIO bus looking for link state
changes (via the Link Status bit in the Status Register at address 0x1).
When the link state changes, the EMAC triggers an interrupt and tells the
driver what the new state is.  The feature eliminates the need for
software to poll the MDIO bus.

Unfortunately, this feature is incompatible with phylib, because it
ignores everything that the PHY core and PHY drivers are trying to do.
In particular:

1. It assumes a compatible register set, so PHYs with different registers
   may not work.

2. It doesn't allow for hardware errata that have work-arounds implemented
   in the PHY driver.

3. It doesn't support multiple register pages. If the PHY core switches
   the register set to another page, the EMAC won't know the page has
   changed and will still attempt to read the same PHY register.

4. It only checks the copper side of the link, not the SGMII side.  Some
   PHY drivers (e.g. at803x) may also check the SGMII side, and
   report the link as not ready during autonegotiation if the SGMII link
   is still down.  Phylib then waits for another interrupt to query
   the PHY again, but the EMAC won't send another interrupt because it
   thinks the link is up.

Cc: sta...@vger.kernel.org # 4.11.x
Tested-by: Manoj Iyer <manoj.i...@canonical.com>
Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c |  2 +-
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 75 ++-
 drivers/net/ethernet/qualcomm/emac/emac.c | 22 +---
 3 files changed, 6 insertions(+), 93 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index e635605..1c0dbaa 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -932,7 +932,7 @@ int emac_mac_up(struct emac_adapter *adpt)
emac_mac_config(adpt);
emac_mac_rx_descs_refill(adpt, >rx_q);
 
-   adpt->phydev->irq = PHY_IGNORE_INTERRUPT;
+   adpt->phydev->irq = PHY_POLL;
ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
 PHY_INTERFACE_MODE_SGMII);
if (ret) {
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c 
b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 441c1936..18461fc 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -13,15 +13,11 @@
 /* Qualcomm Technologies, Inc. EMAC PHY Controller driver.
  */
 
-#include 
-#include 
-#include 
 #include 
 #include 
 #include 
 #include 
 #include "emac.h"
-#include "emac-mac.h"
 
 /* EMAC base register offsets */
 #define EMAC_MDIO_CTRL0x001414
@@ -52,62 +48,10 @@
 
 #define MDIO_WAIT_TIMES   1000
 
-#define EMAC_LINK_SPEED_DEFAULT (\
-   EMAC_LINK_SPEED_10_HALF  |\
-   EMAC_LINK_SPEED_10_FULL  |\
-   EMAC_LINK_SPEED_100_HALF |\
-   EMAC_LINK_SPEED_100_FULL |\
-   EMAC_LINK_SPEED_1GB_FULL)
-
-/**
- * emac_phy_mdio_autopoll_disable() - disable mdio autopoll
- * @adpt: the emac adapter
- *
- * The autopoll feature takes over the MDIO bus.  In order for
- * the PHY driver to be able to talk to the PHY over the MDIO
- * bus, we need to temporarily disable the autopoll feature.
- */
-static int emac_phy_mdio_autopoll_disable(struct emac_adapter *adpt)
-{
-   u32 val;
-
-   /* disable autopoll */
-   emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, MDIO_AP_EN, 0);
-
-   /* wait for any mdio polling to complete */
-   if (!readl_poll_timeout(adpt->base + EMAC_MDIO_CTRL, val,
-   !(val & MDIO_BUSY), 100, MDIO_WAIT_TIMES * 100))
-   return 0;
-
-   /* failed to disable; ensure it is enabled before returning */
-   emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, 0, MDIO_AP_EN);
-
-   return -EBUSY;
-}
-
-/**
- * emac_phy_mdio_autopoll_disable() - disable mdio autopoll
- * @adpt: the emac adapter
- *
- * The EMAC has the ability to poll the external PHY on the MDIO
- * bus for link state changes.  This eliminates the need for the
- * driver to poll the phy.  If if the link state does change,
- * the EMAC issues an interrupt on behalf of the PHY.
- */
-static void emac_phy_mdio_autopoll_enable(struct emac_adapter *adpt)
-{
-   emac_reg_update32(adpt->base + EMAC_MDIO_CTRL, 0, MDIO_AP_EN);
-}
-
 static int emac_mdio_read(struct mii_bus *bus, int addr, int regnum)
 {
struct emac_adapter *adpt = bus->priv;
u32 reg;

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 06/01/2017 06:45 AM, Zefir Kurtisi wrote:

I guess we need to decide whether we generally need to handle permanent aneg
failures on the SGMII link. If we expect that it must not fail (like we assumed
until we saw it failing), I agree with Timur and support reverting of the 
related
commit f62265b53e. If otherwise we want this potential failure to be handled
correctly, things become arbitrary complex. Essentially, we need to handle such
PHYs as a combination of their two sides (copper + SGMII) as virtual sub-PHYs. 
The
phylib might support that in a future version, but for now this seems like a lot
of work required to handle a rare problem.


I'm about to post a patch that removes interrupt support from the EMAC 
driver and relies on software polling of the PHY.  With this patch, we 
don't see the "link is not okay" message from that at803x driver any more.


The link state is generally more reliable now, even when the at803x 
driver doesn't complain.


My theory is that the hardware polling of the PHY is just too 
aggressive.  I think it continuously reads the PHY status register at 
maximum speed and immediately issues an interrupt when the PHY says that 
it's up.


So I think we're okay with leaving the at803x driver as-is, since we 
appear to be no longer getting any false failures.


--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 05/24/2017 04:36 PM, Florian Fainelli wrote:

OK, and there is no way you can run into the following race condition:

CPU HW
MDIO read intent
polling starts
disable HW autopoll
polling continues


Disabling of the HW autopoll waits for the poll to actually stop before 
continuing.  You can see the code here:


http://elixir.free-electrons.com/linux/latest/source/drivers/net/ethernet/qualcomm/emac/emac-phy.c#L102


MDIO read is done
MDIO read done
polling stops
MDIO read value returned


if you disable autopolling in HW this is guaranteed to immediately stop
by the time the register value is seen in HW and your I/O read/write
returns?


It doesn't immediately stop, but the emac_phy_mdio_autopoll_disable() 
function waits for the MDIO bus to not be busy.  But low-level details 
of this feature are not documented, so who knows what it does exactly? 
The original code that used this feature only supported one PHY and 
never expected there to be any asynchronous MDIO transactios.


--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 05/24/2017 04:28 PM, Florian Fainelli wrote:


Yes phydev->lock which is used to serialize the state machine state changes.

Most PHYs have many more registers than the 15 standard exposed
directly, and so you need indirect reads/writes to access these
registers, which typically involve switching a particular page, doing
the indirect register access, and then flipping the page back. If you
interrupt that scheme one way or another, your reads and writes are all
messed up.


Ah, and the at803x is a device like that.

At worst, the autopoll feature could read a register from the wrong 
page, and think that the link state has changed when it hasn't.  But 
that's still bad, and all my problems do revolve around link states.



I forgot one detail.  Every time you do an MDIO read/write, it
temporarily disables the feature.  Although, I think that's not relevant
to your point.


Is that done by the HW itself, or is this under SW control exclusively.


Software.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 05/24/2017 04:15 PM, Andrew Lunn wrote:

My NIC has a feature called autopolling where it takes over the MDIO
bus and regularly polls the link state.  When it detects that the
link state has changed, it generates a MAC interrupt.  This is when
I call phy_mac_interrupt() normally.



Unfortunately, you need to keep this feature turned off. It will not
respect the phydev mutex. It has no idea what page has been currently
selected. It probably has no way to flip the page and see if the SGMII
link is up. etc.


phydev mutex?  And what do you mean by page?

I forgot one detail.  Every time you do an MDIO read/write, it 
temporarily disables the feature.  Although, I think that's not relevant 
to your point.


Disabling this feature and switching from PHY_IGNORE_INTERRUPT to 
PHY_POLL might fix everything.  I will try it.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 05/24/2017 02:34 PM, Andrew Lunn wrote:

Ok, I'm going to debug this some more.  It turns out that the MAC side of
the SGMII link can send an interrupt when it thinks that auto-negotiation is
done.  I might be able to use this.


You can use this for your board. But it still leaves the phy driver
broken for everybody else.


Wait, I thought you said the at803x driver was not broken, since it 
returns 0 when the SGMII side of the link hasn't finished auto-negotiating?



What function should my MAC driver call when it wants the phy core to call
at803x_aneg_done again to see if autonegotiation is done?


You want to trigger the PHY state machine. There is only one exported
API call to do this, phy_mac_interrupt(). But you are supposed to pass
the new link state. And your MAC driver has no idea of that, it does
not know if the copper side of the PHY is up.


My NIC has a feature called autopolling where it takes over the MDIO bus 
and regularly polls the link state.  When it detects that the link state 
has changed, it generates a MAC interrupt.  This is when I call 
phy_mac_interrupt() normally.


I think I can use the SGMII interrupt to also call phy_mac_interrupt(). 
The problem is the from the copper side, the link is already up, so if I 
call phy_mac_interrupt() again and say the link is up, the phy core is 
going to ignore that.



So it might be better if you export phy_trigger_machine().


I'll test that, but that does seem a bit hackish.


Also, is there a way for the MAC driver to know that at803x_aneg_done()
previously returned 0, and that it needs to tell the phy core to check again?


Not that i know of. The MAC layer is not supposed to be messing around
in the PHY layer. However, just triggering the PHY state machine
should be enough.


Can you tell my how PHY_HAS_INTERRUPT is supposed to work?  How does the 
PHY send an interrupt?


I'm starting to think that my NIC's autopolling feature is not 
compatible with phylib, and that I should use polling mode.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 05/24/2017 09:09 AM, Andrew Lunn wrote:
> It could be, the copper side is up, but the SGMII side is down, at the
> point at803x_aneg_done() is called. So it is correctly returning
> 0. Sometime later the SGMII side goes up, but there is not a second
> interrupt. Hence the phy core does not know that the full, 2 stage MAC
> to PHY to peer PHY link is now up.

Ok, I'm going to debug this some more.  It turns out that the MAC side of
the SGMII link can send an interrupt when it thinks that auto-negotiation is
done.  I might be able to use this.

What function should my MAC driver call when it wants the phy core to call
at803x_aneg_done again to see if autonegotiation is done?

Also, is there a way for the MAC driver to know that at803x_aneg_done()
previously returned 0, and that it needs to tell the phy core to check again?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 5/24/17 8:40 AM, Andrew Lunn wrote:


You need to prove this, that the link is not up. Any by link, we mean
both the copper and the SGMII link.


I can post the log of my iperf run showing that the, when 
at803x_aneg_done() returns zero, no packets can go through.  And then 
after I change at803x_aneg_done() so that it returns BMSR_ANEGCOMPLETE, 
then packets do go through.  Is that the proof you're looking for?


The current work-around that we're using internally is to blacklist the 
at803x driver.  This forces the kernel to use the genphy driver instead. 
 Everything works when we do this.



at803x_aneg_done() is never called again, and so I think
the kernel is disabling the interface is some secret way.


Well, the driver has told the core that the link is not up. So the
kernel is waiting for another interrupt indicating the link has gone
up. And probably, this second interrupt never happens.


Exxactly.  That's because the link IS up, and so there is no opportunity 
to receive another interrupt.



And it is not disabling the interface. Since the PHY is still down,
the core has not called netif_carrier_on().


Ok, I should have said "not enabled" instead of "disabled".  Thanks.

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 5/24/17 2:18 AM, Matthias May wrote:

With the patch: When the copper side is seen as up, it also checks if aneg of 
the SGMII link is done.
As far as i know SGMII can not be run without aneg, since it is always Gbit 
with aneg mandatory.
If SGMII aneg is not done, then, well aneg is not done and thus 0 is returned.

Internally we have this patch extended so we don't only report that aneg is not 
done but also reset the link.
Eventually aneg on the SGMII side can be completed and the link comes up.


I would really like to test this patch.


Why do you think that frames are able to go through when aneg is reported as 
not done by the PHY?


I have two theories:

1. The warning message is bogus.  The link actually is okay, but the 
driver thinks that it isn't.


2. The link is not okay, but it automatically fixes itself soon after 
the at803x_aneg_done() finishes.



Since aneg is mandatory for SGMII this can as well be seen as "link not up", 
not?


The problem is that even though the link is up, the driver has returned 
"0", so the kernel thinks that autonegotiation has not finished. 
at803x_aneg_done() is never called again, and so I think the kernel is 
disabling the interface is some secret way.


--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 05/23/2017 11:07 AM, Andrew Lunn wrote:
>> > I will test that to see what happens, but I believe the real problem is 
>> > that
>> > the at803x driver is lying when it says that the link is not okay.  I think
>> > the link is okay, and that's why I'm not getting any more interrupts.  I
>> > don't think I should have to drop interrupt support in my MAC driver 
>> > because
>> > one specific PHY driver is broken.
> If it turns out the PHY hardware is broken, the phy driver itself can
> force it back to polling by setting phydev->irq to PHY_POLL in its
> probe() function.

I don't think the hardware is broken, I think the driver is broken.  The
patch that sets aneg_done to 0 should be reverted or restricted somehow.

Even the developer of the patch admits that if the warning message is
displayed, the link will appear to be up, but no packets will go through.
Perhaps that's because the driver is returning 0 instead of BMSR_ANEGCOMPLETE?

Would it be okay for the PHY driver to query a property from the device tree
directly (e.g. "qca,check-sgmii-link"), and if present, only then implement
the sgmii link check?  So in at803x_probe(), I would do something like this:

if (device_property_read_bool(>mdio.dev,
"qca,check-sgmii-link")
priv->check_sgmii_link = true;

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 05/22/2017 04:32 PM, Andrew Lunn wrote:
>> I'll have to test this, but what do I do if I don't get another interrupt?
> It probably means interrupts cannot be used. Poll it.

I will test that to see what happens, but I believe the real problem is that
the at803x driver is lying when it says that the link is not okay.  I think
the link is okay, and that's why I'm not getting any more interrupts.  I
don't think I should have to drop interrupt support in my MAC driver because
one specific PHY driver is broken.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 05/22/2017 04:09 PM, Andrew Lunn wrote:
> Are you using interrupts? Or polling?

adpt->phydev->irq = PHY_IGNORE_INTERRUPT;
ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
 PHY_INTERFACE_MODE_SGMII);

Technically it's polling, except that it's my NIC's hardware that is polling
the MDIO bus, and then generating an interrupt when there's a link state change.

When the link state changes, I call phy_mac_interrupt()

if (status & ISR_GPHY_LINK)
phy_mac_interrupt(adpt->phydev, !!(status & GPHY_LINK_UP_INT));

> If polling, it should come back again 1 second later and see if
> auto-neg has completed. Hopefully the SGMII side comes up eventually.
> 
> If you are using interrupts, you need another interrupt when the SGMII
> side comes up, otherwise i think the state machine is stuck waiting.

I'll have to test this, but what do I do if I don't get another interrupt?
I have a suspicion that the link is actually okay, and that the error is bogus.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 05/22/2017 04:10 PM, Florian Fainelli wrote:
> Even a module argument would be rejected. If you need platform/SoC
> specific behavior propagated down to the PHY driver, several options exist:
> 
> - pass an agreed upon value for phy_flags to of_phy_connect() see
> drivers/net/ethernet/broadcom/tg3.c and
> drivers/net/ethernet/broadcom/genet/bcmgenet.c for instance and update
> the driver to act on that "flags" see drivers/net/phy/broadcom.c and
> drivers/net/phy/bcm7xxx.c

Will this work on ACPI systems as well?  I call phy_connect_direct() instead
of of_phy_connect().  I see some drivers set phydev->dev_flags before
calling phy_connect_direct().

My concern is that this problem occurs only on an at8031 chip, so having my
network driver passing an at8031-specific flag seems out of place.  What
happens if, on some other board, a different PHY is used, and that flag
means something else?

> - register a PHY fixup which is specific to the board/SoC, and have the
> PHY fixup do whatever is necessary for your platform (like setting
> specific registers)

Do you have an example of that?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] at803x: double check SGMII side autoneg

[Timur Tabi]

On 10/24/2016 05:40 AM, Zefir Kurtisi wrote:
> This commit adds a wrapper function for at8031
> that in case of operating in SGMII mode double
> checks SGMII link state when generic aneg_done()
> succeeds. It prints a warning on failure but
> intentionally does not try to recover from this
> state. As a result, if you ever see a warning
> '803x_aneg_done: SGMII link is not ok' you will
> end up having an Ethernet link up but won't get
> any data through. This should not happen, if it
> does, please contact the module maintainer.

I'm getting bitten by this one again.  We're now have several systems that
are reporting the link failure ("803x_aneg_done: SGMII link is not ok"), and
the interface comes up but is not functional.  I believe this is expected.

The problem, however, is not because of the link failure.  Instead, the
problem is this:

> + /* check if the SGMII link is OK. */
> + if (!(phy_read(phydev, AT803X_PSSR) & AT803X_PSSR_MR_AN_COMPLETE)) {
> + pr_warn("803x_aneg_done: SGMII link is not ok\n");
> + aneg_done = 0;

Returning zero is what breaks the interface.  If I comment-out this last
line, so that at803x_aneg_done() returns BMSR_ANEGCOMPLETE instead, then
everything works.

The documentation for phy_aneg_done() says this:

 * Description: Return the auto-negotiation status from this @phydev
 * Returns > 0 on success or < 0 on error. 0 means that auto-negotiation
 * is still pending.

So I think there are two issues here:

1. What exactly is supposed to happen when phy_aneg_done() returns a zero?
On our system, returning a zero results in a broken link, even though there
are no errors reported.  I just can't send any packets.

2. I'm preparing a patch that adds a command-line parameter to at803x that
makes this code conditional.  If you specify the parameter ("linkcheck")
then it will check the link and return 0 on failure.  Otherwise, it will
return whether genphy_aneg_done() returns.  The question is, should it still
print the message?

What I cannot determine is whether or not the link is actually okay.  It
appears to me that the driver says the link is not ok, but in truth it
actually is, and maybe the whole at803x_aneg_done() function based on a
false premise.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: Requirements for a shutdown function?

[Timur Tabi]

On 05/10/2017 04:47 PM, Florian Fainelli wrote:
> AFAIR kexec takes care of shutting down network devices explicitly
> (unless instructed otherwise with -x/--no-ifdown) so this may be where
> this is coming from.
> 
> Reading through drivers/base/core.c it does not appear that ->remove()
> is called and then ->shutdown() gets called, only ->shutdown() gets
> called from device_shutdown() called from kernel/reboot.c. It seems to
> me like if you want to be on the safe side you would want to implement a
> shutdown function that is identical to what your remove function does.

I finally found a testcase where the shutdown function is useful.  If you do
a "reboot -f", it will call shutdown but not close.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: Requirements for a shutdown function?

[Timur Tabi]

On 05/09/2017 02:06 PM, Florian Fainelli wrote:
> On 05/09/2017 11:51 AM, Timur Tabi wrote:

>> Is it possible that the network stack detects a kexec and automatically
>> stops all network devices?
> 
> No. why would it? However the device driver model does call into your
> driver's remove function and that one does a right job already because
> it does an network device unregister, and so on.

I ran some more tests.  When I launch kexec, the driver's
net_device_ops.ndo_stop function is called, which already stops the interface.

So it looks to me as if the network stack does close the interface during a
kexec.  With the interface closed, there's no point in having a shutdown
function, is there?

>> My in-house driver stops the RX and TX queues.  I'm guessing that's good
>> enough, but I don't have a failing test case to prove it.
>>
> 
> That's probably good enough, yes.

Except that it turns out that the queues are already stopped by then because
the emac_close() function has already been called.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: Requirements for a shutdown function?

[Timur Tabi]

On 05/09/2017 01:46 PM, Florian Fainelli wrote:
> A good test case for exercising a .shutdown() function is kexec'ing a
> new kernel for instance.

I tried that.  I run iperf in one window while launching kexec in another.
Even without a shutdown function, network traffic appear to halt on its own
and the kexec succeeds.

Is it possible that the network stack detects a kexec and automatically
stops all network devices?

> You should put your HW in a state where it won't be doing DMA, or have
> any adverse side effects to the system, putting it in a low power state
> is also a good approach.

My in-house driver stops the RX and TX queues.  I'm guessing that's good
enough, but I don't have a failing test case to prove it.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Requirements for a shutdown function?

[Timur Tabi]

I'm trying to add a platform_driver.shutdown function to my Ethernet driver
(drivers/net/ethernet/qualcomm/emac/*), but I can't find any definitive
information as to what a network driver shutdown callback is supposed to do.
 I also don't know what testcase I should use to verify that my function is
working.

I see only four instances of a platform_driver.shutdown function in
drivers/net/ethernet:

$ git grep -A 20 -w platform_driver | grep '\.shutdown'
apm/xgene-v2/main.c-.shutdown = xge_shutdown,
apm/xgene/xgene_enet_main.c-.shutdown = xgene_enet_shutdown,
marvell/mv643xx_eth.c-  .shutdown   = mv643xx_eth_shutdown,
marvell/pxa168_eth.c-   .shutdown = pxa168_eth_shutdown,

(Other shutdown functions are for pci_driver.shutdown).

For the xgene drivers, the shutdown function just calls the 'remove'
function.  Isn't that overkill?  Why bother with a shutdown function if it's
just the same thing as removing the driver outright?

mv643xx_eth_shutdown() seems more reasonable.  All it does is halt the TX
and RX queues.

pxa168_eth_shutdown() is a little more heavyweight: halts the queues, and
stops the DMA and calls phy_stop().

Can anyone help me figure out what my driver really should do?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] net: qcom/emac: optimize QDF2400 SGMII RX/TX impedence values

[Timur Tabi]

Adjust the impedance values of the RX and TX lanes in the SGMII block
so that they are closer to optimal values.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2400.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2400.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2400.c
index f62c215..7116be4 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2400.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2400.c
@@ -26,6 +26,7 @@
 
 /* SGMII digital lane registers */
 #define EMAC_SGMII_LN_DRVR_CTRL0   0x000C
+#define EMAC_SGMII_LN_DRVR_CTRL1   0x0010
 #define EMAC_SGMII_LN_DRVR_TAP_EN  0x0018
 #define EMAC_SGMII_LN_TX_MARGINING 0x001C
 #define EMAC_SGMII_LN_TX_PRE   0x0020
@@ -48,6 +49,7 @@
 #define EMAC_SGMII_LN_RX_EN_SIGNAL 0x02AC
 #define EMAC_SGMII_LN_RX_MISC_CNTRL0   0x02B8
 #define EMAC_SGMII_LN_DRVR_LOGIC_CLKDIV0x02C8
+#define EMAC_SGMII_LN_RX_RESECODE_OFFSET   0x02CC
 
 /* SGMII digital lane register values */
 #define UCDR_STEP_BY_TWO_MODE0 BIT(7)
@@ -73,6 +75,8 @@
 #define CML_GEAR_MODE(x)   (((x) & 7) << 3)
 #define CML2CMOS_IBOOST_MODE(x)((x) & 7)
 
+#define RESCODE_OFFSET(x)  ((x) & 0x1f)
+
 #define MIXER_LOADB_MODE(x)(((x) & 0xf) << 2)
 #define MIXER_DATARATE_MODE(x) ((x) & 3)
 
@@ -159,6 +163,8 @@ static void emac_reg_write_all(void __iomem *base,
{EMAC_SGMII_LN_PARALLEL_RATE, PARALLEL_RATE_MODE0(1)},
{EMAC_SGMII_LN_TX_BAND_MODE, BAND_MODE0(1)},
{EMAC_SGMII_LN_RX_BAND, BAND_MODE0(2)},
+   {EMAC_SGMII_LN_DRVR_CTRL1, RESCODE_OFFSET(7)},
+   {EMAC_SGMII_LN_RX_RESECODE_OFFSET, RESCODE_OFFSET(9)},
{EMAC_SGMII_LN_LANE_MODE, LANE_MODE(26)},
{EMAC_SGMII_LN_RX_RCVR_PATH1_MODE0, CDR_PD_SEL_MODE0(2) |
EN_DLL_MODE0 | EN_IQ_DCC_MODE0 | EN_IQCAL_MODE0},
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [patch net-next] net: qcom/emac: fix a sizeof() typo

[Timur Tabi]

Dan Carpenter wrote:

We had intended to say "sizeof(u32)" but the "u" is missing.
Fortunately, sizeof(32) is also 4, so the original code still works.

Fixes: c4e7beea2192 ("net: qcom/emac: add ethtool support for reading hardware 
registers")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>


Acked-by: Timur Tabi <ti...@codeaurora.org>

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [patch net-next] net: qcom/emac: fix a sizeof() typo

[Timur Tabi]

walter harms wrote:

The question is: why is a simple calculation const*const
separated into a function ?


This is a callback function.  That's just how it's defined.

It's rare, but there are drivers that use the parameter, like this one:

http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c#n591

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [patch net-next] net: qcom/emac: fix a sizeof() typo

[Timur Tabi]

walter harms wrote:

We have a function where the argument is ignored and the rest is const ?

emac_ethtool_get_regs_len seems the only user. So it would be fairly easy to
move that into that function.

@maintainer:
Is there a deeper logic behind this ?


I don't understand the question.

The patch is valid (I have no idea how I made that mistake).

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

[PATCH 1/2] net: qcom/emac: add ethtool support for reading hardware registers

[Timur Tabi]

Implement the get_regs_len and get_regs ethtool methods.  The driver
returns the values of selected hardware registers.

The make the register offsets known to emac_ethtool, the the register
offset macros are all combined into one header file.  They were
inexplicably and arbitrarily split between two files.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-ethtool.c |  40 
 drivers/net/ethernet/qualcomm/emac/emac-mac.c |  52 ---
 drivers/net/ethernet/qualcomm/emac/emac.h | 108 --
 3 files changed, 119 insertions(+), 81 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c 
b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
index c418a6e..abb9df5 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
@@ -170,6 +170,43 @@ static int emac_set_pauseparam(struct net_device *netdev,
return 0;
 }
 
+/* Selected registers that might want to track during runtime. */
+static const u16 emac_regs[] = {
+   EMAC_DMA_MAS_CTRL,
+   EMAC_MAC_CTRL,
+   EMAC_TXQ_CTRL_0,
+   EMAC_RXQ_CTRL_0,
+   EMAC_DMA_CTRL,
+   EMAC_INT_MASK,
+   EMAC_AXI_MAST_CTRL,
+   EMAC_CORE_HW_VERSION,
+   EMAC_MISC_CTRL,
+};
+
+/* Every time emac_regs[] above is changed, increase this version number. */
+#define EMAC_REGS_VERSION  0
+
+#define EMAC_MAX_REG_SIZE  ARRAY_SIZE(emac_regs)
+
+static void emac_get_regs(struct net_device *netdev,
+ struct ethtool_regs *regs, void *buff)
+{
+   struct emac_adapter *adpt = netdev_priv(netdev);
+   u32 *val = buff;
+   unsigned int i;
+
+   regs->version = EMAC_REGS_VERSION;
+   regs->len = EMAC_MAX_REG_SIZE * sizeof(u32);
+
+   for (i = 0; i < EMAC_MAX_REG_SIZE; i++)
+   val[i] = readl(adpt->base + emac_regs[i]);
+}
+
+static int emac_get_regs_len(struct net_device *netdev)
+{
+   return EMAC_MAX_REG_SIZE * sizeof(32);
+}
+
 static const struct ethtool_ops emac_ethtool_ops = {
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
@@ -189,6 +226,9 @@ static int emac_set_pauseparam(struct net_device *netdev,
.nway_reset = emac_nway_reset,
 
.get_link = ethtool_op_get_link,
+
+   .get_regs_len= emac_get_regs_len,
+   .get_regs= emac_get_regs,
 };
 
 void emac_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 4b3e014..cc065ff 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -25,58 +25,6 @@
 #include "emac.h"
 #include "emac-sgmii.h"
 
-/* EMAC base register offsets */
-#define EMAC_MAC_CTRL  0x001480
-#define EMAC_WOL_CTRL0 0x0014a0
-#define EMAC_RSS_KEY0  0x0014b0
-#define EMAC_H1TPD_BASE_ADDR_LO0x0014e0
-#define EMAC_H2TPD_BASE_ADDR_LO0x0014e4
-#define EMAC_H3TPD_BASE_ADDR_LO0x0014e8
-#define EMAC_INTER_SRAM_PART9  0x001534
-#define EMAC_DESC_CTRL_0   0x001540
-#define EMAC_DESC_CTRL_1   0x001544
-#define EMAC_DESC_CTRL_2   0x001550
-#define EMAC_DESC_CTRL_10  0x001554
-#define EMAC_DESC_CTRL_12  0x001558
-#define EMAC_DESC_CTRL_13  0x00155c
-#define EMAC_DESC_CTRL_3   0x001560
-#define EMAC_DESC_CTRL_4   0x001564
-#define EMAC_DESC_CTRL_5   0x001568
-#define EMAC_DESC_CTRL_14  0x00156c
-#define EMAC_DESC_CTRL_15  0x001570
-#define EMAC_DESC_CTRL_16  0x001574
-#define EMAC_DESC_CTRL_6   0x001578
-#define EMAC_DESC_CTRL_8   0x001580
-#define EMAC_DESC_CTRL_9   0x001584
-#define EMAC_DESC_CTRL_11  0x001588
-#define EMAC_TXQ_CTRL_00x001590
-#define EMAC_TXQ_CTRL_10x001594
-#define EMAC_TXQ_CTRL_20x001598
-#define EMAC_RXQ_CTRL_00x0015a0
-#define EMAC_RXQ_CTRL_10x0015a4
-#define EMAC_RXQ_CTRL_20x0015a8
-#define EMAC_RXQ_CTRL_30x0015ac
-#define EMAC_BASE_CPU_NUMBER   0x0015b8
-#define EMAC_DMA_CTRL  0x0015c0
-#define EMAC_MAILBOX_0 0x0015e0
-#define EMAC_MAILBOX_5 0x0015e4
-#define EMAC_MAILBOX_6 0x0015e8
-#define EMAC_MAILBOX_130x0015ec
-#define EMAC_MAILBOX_2 0x0015f4
-#define EMAC_MAILBOX_3 0x0015f8
-#define EMAC_MAILBOX_110x00160c
-#define EMAC_AXI_MAST_CTRL 0x001610
-#define EMAC_MAIL

[PATCH 2/2] net: qcom/emac: add ethtool support for setting ring parameters

[Timur Tabi]

Implement the set_ringparam method, which allows the user to specify
the size of the TX and RX descriptor rings.  The values are constrained
to the limits of the hardware.

Since the driver does not use separate queues for mini or jumbo frames,
attempts to set those values are rejected.

If the interface is already running when the setting is changed, then
the interface is reset.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-ethtool.c | 24 +++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c 
b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
index abb9df5..a3e2292 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
@@ -145,6 +145,29 @@ static void emac_get_ringparam(struct net_device *netdev,
ring->tx_pending = adpt->tx_desc_cnt;
 }
 
+static int emac_set_ringparam(struct net_device *netdev,
+ struct ethtool_ringparam *ring)
+{
+   struct emac_adapter *adpt = netdev_priv(netdev);
+
+   /* We don't have separate queues/rings for small/large frames, so
+* reject any attempt to specify those values separately.
+*/
+   if (ring->rx_mini_pending || ring->rx_jumbo_pending)
+   return -EINVAL;
+
+   adpt->tx_desc_cnt =
+   clamp_val(ring->tx_pending, EMAC_MIN_TX_DESCS, 
EMAC_MAX_TX_DESCS);
+
+   adpt->rx_desc_cnt =
+   clamp_val(ring->rx_pending, EMAC_MIN_RX_DESCS, 
EMAC_MAX_RX_DESCS);
+
+   if (netif_running(netdev))
+   return emac_reinit_locked(adpt);
+
+   return 0;
+}
+
 static void emac_get_pauseparam(struct net_device *netdev,
struct ethtool_pauseparam *pause)
 {
@@ -219,6 +242,7 @@ static int emac_get_regs_len(struct net_device *netdev)
.get_ethtool_stats = emac_get_ethtool_stats,
 
.get_ringparam = emac_get_ringparam,
+   .set_ringparam = emac_set_ringparam,
 
.get_pauseparam = emac_get_pauseparam,
.set_pauseparam = emac_set_pauseparam,
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 0/2] net: qcom/emac: add the last ethtool functions

[Timur Tabi]

These two patches implement the remaining two ethtool functions that
are of interest to the Qualcomm EMAC driver.  These are the last 
patches that will be submitted for the 4.11 merge window.

Timur Tabi (2):
  net: qcom/emac: add ethtool support for reading hardware registers
  net: qcom/emac: add ethtool support for setting ring parameters

 drivers/net/ethernet/qualcomm/emac/emac-ethtool.c |  64 +
 drivers/net/ethernet/qualcomm/emac/emac-mac.c |  52 ---
 drivers/net/ethernet/qualcomm/emac/emac.h | 108 --
 3 files changed, 143 insertions(+), 81 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] [net-next] net: qcom/emac: add ethool support for setting pause parameters

[Timur Tabi]

To support setting the pause parameters, the driver can no longer just
mirror the PHY.  The set_pauseparam feature allows the driver to
force the setting in the MAC, regardless of how the PHY is configured.
This means that we now need to maintain an internal state for pause
frame support, and so get_pauseparam also needs to be updated.

If the interface is already running when the setting is changed, then
the interface is reset.

Note that if the MAC is configured to enable RX pause frame support
(i.e. it transmits pause frames to throttle the other end), but the
PHY is configured to block those frames, then the feature will not work.

Also some buffer size initialization code into emac_init_adapter(),
so that it lives with similar code, including the initializtion of
pause frame support.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-ethtool.c | 30 ---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 18 ++
 drivers/net/ethernet/qualcomm/emac/emac.c | 11 ++---
 drivers/net/ethernet/qualcomm/emac/emac.h |  7 ++
 4 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c 
b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
index cfc57d2..c418a6e 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
@@ -148,16 +148,26 @@ static void emac_get_ringparam(struct net_device *netdev,
 static void emac_get_pauseparam(struct net_device *netdev,
struct ethtool_pauseparam *pause)
 {
-   struct phy_device *phydev = netdev->phydev;
+   struct emac_adapter *adpt = netdev_priv(netdev);
 
-   if (phydev) {
-   if (phydev->autoneg)
-   pause->autoneg = 1;
-   if (phydev->pause)
-   pause->rx_pause = 1;
-   if (phydev->pause != phydev->asym_pause)
-   pause->tx_pause = 1;
-   }
+   pause->autoneg = adpt->automatic ? AUTONEG_ENABLE : AUTONEG_DISABLE;
+   pause->rx_pause = adpt->rx_flow_control ? 1 : 0;
+   pause->tx_pause = adpt->tx_flow_control ? 1 : 0;;
+}
+
+static int emac_set_pauseparam(struct net_device *netdev,
+  struct ethtool_pauseparam *pause)
+{
+   struct emac_adapter *adpt = netdev_priv(netdev);
+
+   adpt->automatic = pause->autoneg == AUTONEG_ENABLE;
+   adpt->rx_flow_control = pause->rx_pause != 0;
+   adpt->tx_flow_control = pause->tx_pause != 0;
+
+   if (netif_running(netdev))
+   return emac_reinit_locked(adpt);
+
+   return 0;
 }
 
 static const struct ethtool_ops emac_ethtool_ops = {
@@ -172,7 +182,9 @@ static void emac_get_pauseparam(struct net_device *netdev,
.get_ethtool_stats = emac_get_ethtool_stats,
 
.get_ringparam = emac_get_ringparam,
+
.get_pauseparam = emac_get_pauseparam,
+   .set_pauseparam = emac_set_pauseparam,
 
.nway_reset = emac_nway_reset,
 
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index b991219..4b3e014 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -565,11 +565,19 @@ static void emac_mac_start(struct emac_adapter *adpt)
 
mac |= TXEN | RXEN; /* enable RX/TX */
 
-   /* Configure MAC flow control to match the PHY's settings. */
-   if (phydev->pause)
-   mac |= RXFC;
-   if (phydev->pause != phydev->asym_pause)
-   mac |= TXFC;
+   /* Configure MAC flow control. If set to automatic, then match
+* whatever the PHY does. Otherwise, enable or disable it, depending
+* on what the user configured via ethtool.
+*/
+   mac &= ~(RXFC | TXFC);
+
+   if (adpt->automatic) {
+   /* If it's set to automatic, then update our local values */
+   adpt->rx_flow_control = phydev->pause;
+   adpt->tx_flow_control = phydev->pause != phydev->asym_pause;
+   }
+   mac |= adpt->rx_flow_control ? RXFC : 0;
+   mac |= adpt->tx_flow_control ? TXFC : 0;
 
/* setup link speed */
mac &= ~SPEED_MASK;
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index 3387c0a..28a8cdc 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -436,6 +436,10 @@ static void emac_init_adapter(struct emac_adapter *adpt)
 {
u32 reg;
 
+   adpt->rrd_size = EMAC_RRD_SIZE;
+   adpt->tpd_size = EMAC_TPD_SIZE;
+   adpt->rfd_size = EMAC_RFD_SIZE;
+
/* descriptors */
adpt->tx_desc_cnt = EMAC_DEF_TX_DESCS;
adpt->rx_desc

Re: [PATCH 4.10-rc3 08/13] net: emac: fix build errors when linux/phy*.h is removed from net/dsa.h

[Timur Tabi]

On 01/31/2017 01:19 PM, Russell King wrote:

drivers/net/ethernet/qualcomm/emac/emac-sgmii.c:58:12: error: dereferencing 
pointer to incomplete type 'struct phy_device'

Add linux/phy.h to emac-sgmii.c

Signed-off-by: Russell King 
---
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 1 +


The version of emac-sgmii.c on net-next does not need this fixed.  I already 
removed all references to phy_device in commit "net: qcom/emac: always use 
autonegotiation to configure the SGMII link".


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 4/4] [net-next] net: qcom/emac: add an error interrupt handler for the sgmii

[Timur Tabi]

On 01/27/2017 04:43 PM, Timur Tabi wrote:

The SGMII (internal PHY) can report decode errors via an interrupt.  It
can also report autonegotiation status changes, but we don't need to track
those.  The SGMII can recover automatically from most decode errors, so
we only reset the interface if we get multiple consecutive errors.

It's possible for bogus decode errors to be reported while the link is
being brought up.  The interrupt is registered when the interface is
opened, and it's enabled after the link is up.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>


Sorry, this particular patch wasn't meant to be in the patchset.  Please 
ignore it.


--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 4/5] [net-next] net: qcom/emac: remove extraneous wake-on-lan code

[Timur Tabi]

The EMAC driver does not support wake-on-lan, but there is still
code left-over that partially enables it.  Remove that code and a few
macros that support it.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 10 --
 drivers/net/ethernet/qualcomm/emac/emac.h |  4 
 2 files changed, 14 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 3f3cd00..33d7ff1 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -103,14 +103,6 @@
 #define RXEN0x0002
 #define TXEN0x0001
 
-
-/* EMAC_WOL_CTRL0 */
-#define LK_CHG_PME 0x20
-#define LK_CHG_EN  0x10
-#define MG_FRAME_PME   0x8
-#define MG_FRAME_EN0x4
-#define WK_FRAME_EN0x1
-
 /* EMAC_DESC_CTRL_3 */
 #define RFD_RING_SIZE_BMSK   0xfff
 
@@ -619,8 +611,6 @@ static void emac_mac_start(struct emac_adapter *adpt)
 
emac_reg_update32(adpt->base + EMAC_ATHR_HEADER_CTRL,
  (HEADER_ENABLE | HEADER_CNT_EN), 0);
-
-   emac_reg_update32(adpt->csr + EMAC_EMAC_WRAPPER_CSR2, 0, WOL_EN);
 }
 
 void emac_mac_stop(struct emac_adapter *adpt)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.h 
b/drivers/net/ethernet/qualcomm/emac/emac.h
index 2725507..ef91dcc 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.h
+++ b/drivers/net/ethernet/qualcomm/emac/emac.h
@@ -167,10 +167,6 @@ enum emac_clk_id {
 
 #define EMAC_MAX_SETUP_LNK_CYCLE   100
 
-/* Wake On Lan */
-#define EMAC_WOL_PHY 0x0001 /* PHY Status Change */
-#define EMAC_WOL_MAGIC   0x0002 /* Magic Packet */
-
 struct emac_stats {
/* rx */
u64 rx_ok;  /* good packets */
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 3/5] [net-next] net: qcom/emac: do not call emac_mac_start twice

[Timur Tabi]

emac_mac_start() uses information from the external PHY to program
the MAC, so it makes no sense to call it before the link is up.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 2 +-
 drivers/net/ethernet/qualcomm/emac/emac-mac.h | 1 -
 drivers/net/ethernet/qualcomm/emac/emac.c | 2 --
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 155e273..3f3cd00 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -556,7 +556,7 @@ void emac_mac_reset(struct emac_adapter *adpt)
emac_reg_update32(adpt->base + EMAC_DMA_MAS_CTRL, 0, INT_RD_CLR_EN);
 }
 
-void emac_mac_start(struct emac_adapter *adpt)
+static void emac_mac_start(struct emac_adapter *adpt)
 {
struct phy_device *phydev = adpt->phydev;
u32 mac, csr1;
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.h 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.h
index f3aa24d..5028fb4 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.h
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.h
@@ -230,7 +230,6 @@ struct emac_tx_queue {
 int  emac_mac_up(struct emac_adapter *adpt);
 void emac_mac_down(struct emac_adapter *adpt);
 void emac_mac_reset(struct emac_adapter *adpt);
-void emac_mac_start(struct emac_adapter *adpt);
 void emac_mac_stop(struct emac_adapter *adpt);
 void emac_mac_mode_config(struct emac_adapter *adpt);
 void emac_mac_rx_process(struct emac_adapter *adpt, struct emac_rx_queue *rx_q,
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index 3e1be91..75305ad 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -280,8 +280,6 @@ static int emac_open(struct net_device *netdev)
return ret;
}
 
-   emac_mac_start(adpt);
-
return 0;
 }
 
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/5] [net-next] net: qcom/emac: always use autonegotiation to configure the SGMII link

[Timur Tabi]

Regardless of how the external PHY is configured, the internal PHY
(the "SGMII" block) is capable of configuring the SGMII link automatically.
When the external PHY link comes up, regardless of how it is configured,
the SGMII link is configured automatically.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 49 +
 1 file changed, 10 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index 0149b52..b5269c4 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -47,44 +47,19 @@
 
 #define SERDES_START_WAIT_TIMES100
 
-static int emac_sgmii_link_init(struct emac_adapter *adpt)
+/* Initialize the SGMII link between the internal and external PHYs. */
+static void emac_sgmii_link_init(struct emac_adapter *adpt)
 {
-   struct phy_device *phydev = adpt->phydev;
struct emac_sgmii *phy = >phy;
u32 val;
 
+   /* Always use autonegotiation. It works no matter how the external
+* PHY is configured.
+*/
val = readl(phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
-
-   if (phydev->autoneg == AUTONEG_ENABLE) {
-   val &= ~(FORCE_AN_RX_CFG | FORCE_AN_TX_CFG);
-   val |= AN_ENABLE;
-   writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
-   } else {
-   u32 speed_cfg;
-
-   switch (phydev->speed) {
-   case SPEED_10:
-   speed_cfg = SPDMODE_10;
-   break;
-   case SPEED_100:
-   speed_cfg = SPDMODE_100;
-   break;
-   case SPEED_1000:
-   speed_cfg = SPDMODE_1000;
-   break;
-   default:
-   return -EINVAL;
-   }
-
-   if (phydev->duplex == DUPLEX_FULL)
-   speed_cfg |= DUPLEX_MODE;
-
-   val &= ~AN_ENABLE;
-   writel(speed_cfg, phy->base + EMAC_SGMII_PHY_SPEED_CFG1);
-   writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
-   }
-
-   return 0;
+   val &= ~(FORCE_AN_RX_CFG | FORCE_AN_TX_CFG);
+   val |= AN_ENABLE;
+   writel(val, phy->base + EMAC_SGMII_PHY_AUTONEG_CFG2);
 }
 
 static int emac_sgmii_irq_clear(struct emac_adapter *adpt, u32 irq_bits)
@@ -145,12 +120,7 @@ void emac_sgmii_reset(struct emac_adapter *adpt)
int ret;
 
emac_sgmii_reset_prepare(adpt);
-
-   ret = emac_sgmii_link_init(adpt);
-   if (ret) {
-   netdev_err(adpt->netdev, "unsupported link speed\n");
-   return;
-   }
+   emac_sgmii_link_init(adpt);
 
ret = adpt->phy.initialize(adpt);
if (ret)
@@ -287,6 +257,7 @@ int emac_sgmii_config(struct platform_device *pdev, struct 
emac_adapter *adpt)
goto error;
 
emac_sgmii_irq_clear(adpt, SGMII_PHY_INTERRUPT_ERR);
+   emac_sgmii_link_init(adpt);
 
/* We've remapped the addresses, so we don't need the device any
 * more.  of_find_device_by_node() says we should release it.
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 5/5] [net-next] net: qcom/emac: add an error interrupt handler for the sgmii

[Timur Tabi]

The SGMII (internal PHY) can report decode errors via an interrupt.  It
can also report autonegotiation status changes, but we don't need to track
those.  The SGMII can recover automatically from most decode errors, so
we only reset the interface if we get multiple consecutive errors.

It's possible for bogus decode errors to be reported while the link is
being brought up.  The interrupt is registered when the interface is
opened, and it's enabled after the link is up.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c   |   8 +-
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 126 +++-
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.h |  16 ++-
 drivers/net/ethernet/qualcomm/emac/emac.c   |  10 ++
 4 files changed, 153 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 33d7ff1..b991219 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -951,12 +951,16 @@ static void emac_mac_rx_descs_refill(struct emac_adapter 
*adpt,
 static void emac_adjust_link(struct net_device *netdev)
 {
struct emac_adapter *adpt = netdev_priv(netdev);
+   struct emac_sgmii *sgmii = >phy;
struct phy_device *phydev = netdev->phydev;
 
-   if (phydev->link)
+   if (phydev->link) {
emac_mac_start(adpt);
-   else
+   sgmii->link_up(adpt);
+   } else {
+   sgmii->link_down(adpt);
emac_mac_stop(adpt);
+   }
 
phy_print_status(phydev);
 }
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index b5269c4..040b289 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -25,7 +25,9 @@
 #define EMAC_SGMII_PHY_SPEED_CFG1  0x0074
 #define EMAC_SGMII_PHY_IRQ_CMD 0x00ac
 #define EMAC_SGMII_PHY_INTERRUPT_CLEAR 0x00b0
+#define EMAC_SGMII_PHY_INTERRUPT_MASK  0x00b4
 #define EMAC_SGMII_PHY_INTERRUPT_STATUS0x00b8
+#define EMAC_SGMII_PHY_RX_CHK_STATUS   0x00d4
 
 #define FORCE_AN_TX_CFGBIT(5)
 #define FORCE_AN_RX_CFGBIT(4)
@@ -36,6 +38,8 @@
 #define SPDMODE_100BIT(0)
 #define SPDMODE_10 0
 
+#define CDR_ALIGN_DET  BIT(6)
+
 #define IRQ_GLOBAL_CLEAR   BIT(0)
 
 #define DECODE_CODE_ERRBIT(7)
@@ -44,6 +48,7 @@
 #define SGMII_PHY_IRQ_CLR_WAIT_TIME10
 
 #define SGMII_PHY_INTERRUPT_ERR(DECODE_CODE_ERR | 
DECODE_DISP_ERR)
+#define SGMII_ISR_MASK (SGMII_PHY_INTERRUPT_ERR)
 
 #define SERDES_START_WAIT_TIMES100
 
@@ -96,6 +101,51 @@ static int emac_sgmii_irq_clear(struct emac_adapter *adpt, 
u32 irq_bits)
return 0;
 }
 
+/* The number of decode errors that triggers a reset */
+#define DECODE_ERROR_LIMIT 2
+
+static irqreturn_t emac_sgmii_interrupt(int irq, void *data)
+{
+   struct emac_adapter *adpt = data;
+   struct emac_sgmii *phy = >phy;
+   u32 status;
+
+   status = readl(phy->base + EMAC_SGMII_PHY_INTERRUPT_STATUS);
+   status &= SGMII_ISR_MASK;
+   if (!status)
+   return IRQ_HANDLED;
+
+   /* If we get a decoding error and CDR is not locked, then try
+* resetting the internal PHY.  The internal PHY uses an embedded
+* clock with Clock and Data Recovery (CDR) to recover the
+* clock and data.
+*/
+   if (status & SGMII_PHY_INTERRUPT_ERR) {
+   int count;
+
+   /* The SGMII is capable of recovering from some decode
+* errors automatically.  However, if we get multiple
+* decode errors in a row, then assume that something
+* is wrong and reset the interface.
+*/
+   count = atomic_inc_return(>decode_error_count);
+   if (count == DECODE_ERROR_LIMIT) {
+   schedule_work(>work_thread);
+   atomic_set(>decode_error_count, 0);
+   }
+   } else {
+   /* We only care about consecutive decode errors. */
+   atomic_set(>decode_error_count, 0);
+   }
+
+   if (emac_sgmii_irq_clear(adpt, status)) {
+   netdev_warn(adpt->netdev, "failed to clear SGMII interrupt\n");
+   schedule_work(>work_thread);
+   }
+
+   return IRQ_HANDLED;
+}
+
 static void emac_sgmii_reset_prepare(struct emac_adapter *adpt)
 {
struct emac_sgmii *phy = >phy;
@@ -129,6 +179,68 @@ void emac_sgmii_reset(struct emac_adapter *ad

[PATCH 0/5] [net-next] net: qcom/emac:

[Timur Tabi]

Although not related, these patches affect the same files, so they should
be applied in order.

The first patch cleans up logging of when the the phy driver is attached.

The second patch always configures the SGMII to use autonegotiation mode.

The third patch removes a redundant call to emac_mac_start().

The fourth patch removes some extraneous non-functioning WOL code.

The fifth patch adds an error handler for the SGMII block.

Timur Tabi (5):
  [net-next] net: qcom/emac: display the phy driver info after we
connect
  [net-next] net: qcom/emac: always use autonegotiation to configure the
SGMII link
  [net-next] net: qcom/emac: do not call emac_mac_start twice
  [net-next] net: qcom/emac: remove extraneous wake-on-lan code
  [net-next] net: qcom/emac: add an error interrupt handler for the
sgmii

 drivers/net/ethernet/qualcomm/emac/emac-mac.c   |  24 ++--
 drivers/net/ethernet/qualcomm/emac/emac-mac.h   |   1 -
 drivers/net/ethernet/qualcomm/emac/emac-phy.c   |   3 -
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 175 ++--
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.h |  16 ++-
 drivers/net/ethernet/qualcomm/emac/emac.c   |  10 +-
 drivers/net/ethernet/qualcomm/emac/emac.h   |   4 -
 7 files changed, 166 insertions(+), 67 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 4/4] [net-next] net: qcom/emac: add an error interrupt handler for the sgmii

[Timur Tabi]

The SGMII (internal PHY) can report decode errors via an interrupt.  It
can also report autonegotiation status changes, but we don't need to track
those.  The SGMII can recover automatically from most decode errors, so
we only reset the interface if we get multiple consecutive errors.

It's possible for bogus decode errors to be reported while the link is
being brought up.  The interrupt is registered when the interface is
opened, and it's enabled after the link is up.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c   |   8 +-
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c | 126 +++-
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.h |  16 ++-
 drivers/net/ethernet/qualcomm/emac/emac.c   |  10 ++
 4 files changed, 153 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 3f3cd00..a0bc8a85 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -961,12 +961,16 @@ static void emac_mac_rx_descs_refill(struct emac_adapter 
*adpt,
 static void emac_adjust_link(struct net_device *netdev)
 {
struct emac_adapter *adpt = netdev_priv(netdev);
+   struct emac_sgmii *sgmii = >phy;
struct phy_device *phydev = netdev->phydev;
 
-   if (phydev->link)
+   if (phydev->link) {
emac_mac_start(adpt);
-   else
+   sgmii->link_up(adpt);
+   } else {
+   sgmii->link_down(adpt);
emac_mac_stop(adpt);
+   }
 
phy_print_status(phydev);
 }
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index b5269c4..040b289 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -25,7 +25,9 @@
 #define EMAC_SGMII_PHY_SPEED_CFG1  0x0074
 #define EMAC_SGMII_PHY_IRQ_CMD 0x00ac
 #define EMAC_SGMII_PHY_INTERRUPT_CLEAR 0x00b0
+#define EMAC_SGMII_PHY_INTERRUPT_MASK  0x00b4
 #define EMAC_SGMII_PHY_INTERRUPT_STATUS0x00b8
+#define EMAC_SGMII_PHY_RX_CHK_STATUS   0x00d4
 
 #define FORCE_AN_TX_CFGBIT(5)
 #define FORCE_AN_RX_CFGBIT(4)
@@ -36,6 +38,8 @@
 #define SPDMODE_100BIT(0)
 #define SPDMODE_10 0
 
+#define CDR_ALIGN_DET  BIT(6)
+
 #define IRQ_GLOBAL_CLEAR   BIT(0)
 
 #define DECODE_CODE_ERRBIT(7)
@@ -44,6 +48,7 @@
 #define SGMII_PHY_IRQ_CLR_WAIT_TIME10
 
 #define SGMII_PHY_INTERRUPT_ERR(DECODE_CODE_ERR | 
DECODE_DISP_ERR)
+#define SGMII_ISR_MASK (SGMII_PHY_INTERRUPT_ERR)
 
 #define SERDES_START_WAIT_TIMES100
 
@@ -96,6 +101,51 @@ static int emac_sgmii_irq_clear(struct emac_adapter *adpt, 
u32 irq_bits)
return 0;
 }
 
+/* The number of decode errors that triggers a reset */
+#define DECODE_ERROR_LIMIT 2
+
+static irqreturn_t emac_sgmii_interrupt(int irq, void *data)
+{
+   struct emac_adapter *adpt = data;
+   struct emac_sgmii *phy = >phy;
+   u32 status;
+
+   status = readl(phy->base + EMAC_SGMII_PHY_INTERRUPT_STATUS);
+   status &= SGMII_ISR_MASK;
+   if (!status)
+   return IRQ_HANDLED;
+
+   /* If we get a decoding error and CDR is not locked, then try
+* resetting the internal PHY.  The internal PHY uses an embedded
+* clock with Clock and Data Recovery (CDR) to recover the
+* clock and data.
+*/
+   if (status & SGMII_PHY_INTERRUPT_ERR) {
+   int count;
+
+   /* The SGMII is capable of recovering from some decode
+* errors automatically.  However, if we get multiple
+* decode errors in a row, then assume that something
+* is wrong and reset the interface.
+*/
+   count = atomic_inc_return(>decode_error_count);
+   if (count == DECODE_ERROR_LIMIT) {
+   schedule_work(>work_thread);
+   atomic_set(>decode_error_count, 0);
+   }
+   } else {
+   /* We only care about consecutive decode errors. */
+   atomic_set(>decode_error_count, 0);
+   }
+
+   if (emac_sgmii_irq_clear(adpt, status)) {
+   netdev_warn(adpt->netdev, "failed to clear SGMII interrupt\n");
+   schedule_work(>work_thread);
+   }
+
+   return IRQ_HANDLED;
+}
+
 static void emac_sgmii_reset_prepare(struct emac_adapter *adpt)
 {
struct emac_sgmii *phy = >phy;
@@ -129,6 +179,68 @@ void emac_sgmii_reset(struct emac_adapter *ad

[PATCH 1/5] [net-next] net: qcom/emac: display the phy driver info after we connect

[Timur Tabi]

The PHY driver is attached only when the driver calls
phy_connect_direct().  Calling phy_attached_print() to display
information about the PHY driver prior to that point is meaningless.
The interface can be brought down, a new PHY driver can be loaded,
and the interface then brought back up.  This is the correct time
to display information about the attached driver.

Since phy_attached_print() also prints information about the
interrupt, that needs to be set as well.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 4 +++-
 drivers/net/ethernet/qualcomm/emac/emac-phy.c | 3 ---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index e4793d7..155e273 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -981,6 +981,7 @@ int emac_mac_up(struct emac_adapter *adpt)
emac_mac_config(adpt);
emac_mac_rx_descs_refill(adpt, >rx_q);
 
+   adpt->phydev->irq = PHY_IGNORE_INTERRUPT;
ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
 PHY_INTERFACE_MODE_SGMII);
if (ret) {
@@ -988,11 +989,12 @@ int emac_mac_up(struct emac_adapter *adpt)
return ret;
}
 
+   phy_attached_print(adpt->phydev, NULL);
+
/* enable mac irq */
writel((u32)~DIS_INT, adpt->base + EMAC_INT_STATUS);
writel(adpt->irq.mask, adpt->base + EMAC_INT_MASK);
 
-   adpt->phydev->irq = PHY_IGNORE_INTERRUPT;
phy_start(adpt->phydev);
 
napi_enable(>rx_q.napi);
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c 
b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 1d7852f..441c1936 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -226,8 +226,5 @@ int emac_phy_config(struct platform_device *pdev, struct 
emac_adapter *adpt)
return -ENODEV;
}
 
-   if (adpt->phydev->drv)
-   phy_attached_print(adpt->phydev, NULL);
-
return 0;
 }
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH] [net-next] net: qcom/emac: rename emac_phy to emac_sgmii and move it

[Timur Tabi]

The EMAC has an internal PHY that is often called the "SGMII".  This
SGMII is also connected to an external PHY, which is managed by phylib.
These dual PHYs often cause confusion.  In this case, the data structure
for managing the SGMII was mis-named and located in the wrong header file.

Structure emac_phy is renamed to emac_sgmii to clearly indicate it applies
to the internal PHY only.  It also also moved from emac_phy.h (which
supports the external PHY) to emac_sgmii.h (where it belongs).

To keep the changes minimal, only the structure name is changed, not
the names of any variables of that type.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-phy.c   |  2 --
 drivers/net/ethernet/qualcomm/emac/emac-phy.h   | 13 -
 drivers/net/ethernet/qualcomm/emac/emac-sgmii-fsm9900.c |  2 +-
 drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2400.c |  2 +-
 drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2432.c |  2 +-
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c |  8 
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.h | 13 +
 drivers/net/ethernet/qualcomm/emac/emac.c   |  2 +-
 drivers/net/ethernet/qualcomm/emac/emac.h   |  3 ++-
 9 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.c 
b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
index 2851b4c..1d7852f 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.c
@@ -22,8 +22,6 @@
 #include 
 #include "emac.h"
 #include "emac-mac.h"
-#include "emac-phy.h"
-#include "emac-sgmii.h"
 
 /* EMAC base register offsets */
 #define EMAC_MDIO_CTRL0x001414
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-phy.h 
b/drivers/net/ethernet/qualcomm/emac/emac-phy.h
index 49f3701..c0c301c 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-phy.h
+++ b/drivers/net/ethernet/qualcomm/emac/emac-phy.h
@@ -13,19 +13,6 @@
 #ifndef _EMAC_PHY_H_
 #define _EMAC_PHY_H_
 
-typedef int (*emac_sgmii_initialize)(struct emac_adapter *adpt);
-
-/** emac_phy - internal emac phy
- * @base base address
- * @digital per-lane digital block
- * @initialize initialization function
- */
-struct emac_phy {
-   void __iomem*base;
-   void __iomem*digital;
-   emac_sgmii_initialize   initialize;
-};
-
 struct emac_adapter;
 
 int emac_phy_config(struct platform_device *pdev, struct emac_adapter *adpt);
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii-fsm9900.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii-fsm9900.c
index af690e1..10de8d0 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii-fsm9900.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii-fsm9900.c
@@ -214,7 +214,7 @@ static void emac_reg_write_all(void __iomem *base,
 
 int emac_sgmii_init_fsm9900(struct emac_adapter *adpt)
 {
-   struct emac_phy *phy = >phy;
+   struct emac_sgmii *phy = >phy;
unsigned int i;
 
emac_reg_write_all(phy->base, physical_coding_sublayer_programming,
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2400.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2400.c
index 5b84194..f62c215 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2400.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2400.c
@@ -174,7 +174,7 @@ static void emac_reg_write_all(void __iomem *base,
 
 int emac_sgmii_init_qdf2400(struct emac_adapter *adpt)
 {
-   struct emac_phy *phy = >phy;
+   struct emac_sgmii *phy = >phy;
void __iomem *phy_regs = phy->base;
void __iomem *laned = phy->digital;
unsigned int i;
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2432.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2432.c
index 6170200..b9c0df7 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2432.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii-qdf2432.c
@@ -167,7 +167,7 @@ static void emac_reg_write_all(void __iomem *base,
 
 int emac_sgmii_init_qdf2432(struct emac_adapter *adpt)
 {
-   struct emac_phy *phy = >phy;
+   struct emac_sgmii *phy = >phy;
void __iomem *phy_regs = phy->base;
void __iomem *laned = phy->digital;
unsigned int i;
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c 
b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
index bf722a9..0149b52 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
@@ -50,7 +50,7 @@
 static int emac_sgmii_link_init(struct emac_adapter *adpt)
 {
struct phy_device *phydev = adpt->phydev;
-   struct emac_phy *phy = >phy;
+   struct emac_sgmii *phy = >phy;
u32 val;
 
val = readl(phy->base + EMAC_SGMII_PHY_AUTO

[PATCH] [net-next][v2] net: qcom/emac: claim the irq only when the device is opened

[Timur Tabi]

During reset, functions emac_mac_down() and emac_mac_up() are called,
so we don't want to free and claim the IRQ unnecessarily.  Move those
operations to open/close.

Signed-off-by: Timur Tabi <ti...@codeaurora.org>
---

Notes:
v2: keep synchronize_irq call where it is

 drivers/net/ethernet/qualcomm/emac/emac-mac.c | 13 -
 drivers/net/ethernet/qualcomm/emac/emac.c | 11 +++
 drivers/net/ethernet/qualcomm/emac/emac.h |  1 -
 3 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c 
b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index 98570eb..e4793d7 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -314,8 +314,6 @@ struct emac_skb_cb {
RX_PKT_INT2 |\
RX_PKT_INT3)
 
-#define EMAC_MAC_IRQ_RES   "core0"
-
 void emac_mac_multicast_addr_set(struct emac_adapter *adpt, u8 *addr)
 {
u32 crc32, bit, reg, mta;
@@ -977,26 +975,16 @@ static void emac_adjust_link(struct net_device *netdev)
 int emac_mac_up(struct emac_adapter *adpt)
 {
struct net_device *netdev = adpt->netdev;
-   struct emac_irq *irq = >irq;
int ret;
 
emac_mac_rx_tx_ring_reset_all(adpt);
emac_mac_config(adpt);
-
-   ret = request_irq(irq->irq, emac_isr, 0, EMAC_MAC_IRQ_RES, irq);
-   if (ret) {
-   netdev_err(adpt->netdev, "could not request %s irq\n",
-  EMAC_MAC_IRQ_RES);
-   return ret;
-   }
-
emac_mac_rx_descs_refill(adpt, >rx_q);
 
ret = phy_connect_direct(netdev, adpt->phydev, emac_adjust_link,
 PHY_INTERFACE_MODE_SGMII);
if (ret) {
netdev_err(adpt->netdev, "could not connect phy\n");
-   free_irq(irq->irq, irq);
return ret;
}
 
@@ -1030,7 +1018,6 @@ void emac_mac_down(struct emac_adapter *adpt)
writel(DIS_INT, adpt->base + EMAC_INT_STATUS);
writel(0, adpt->base + EMAC_INT_MASK);
synchronize_irq(adpt->irq.irq);
-   free_irq(adpt->irq.irq, >irq);
 
phy_disconnect(adpt->phydev);
 
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c 
b/drivers/net/ethernet/qualcomm/emac/emac.c
index b74ec7f..3e1be91 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -256,18 +256,27 @@ static int emac_change_mtu(struct net_device *netdev, int 
new_mtu)
 static int emac_open(struct net_device *netdev)
 {
struct emac_adapter *adpt = netdev_priv(netdev);
+   struct emac_irq *irq = >irq;
int ret;
 
+   ret = request_irq(irq->irq, emac_isr, 0, "emac-core0", irq);
+   if (ret) {
+   netdev_err(adpt->netdev, "could not request emac-core0 irq\n");
+   return ret;
+   }
+
/* allocate rx/tx dma buffer & descriptors */
ret = emac_mac_rx_tx_rings_alloc_all(adpt);
if (ret) {
netdev_err(adpt->netdev, "error allocating rx/tx rings\n");
+   free_irq(irq->irq, irq);
return ret;
}
 
ret = emac_mac_up(adpt);
if (ret) {
emac_mac_rx_tx_rings_free_all(adpt);
+   free_irq(irq->irq, irq);
return ret;
}
 
@@ -286,6 +295,8 @@ static int emac_close(struct net_device *netdev)
emac_mac_down(adpt);
emac_mac_rx_tx_rings_free_all(adpt);
 
+   free_irq(adpt->irq.irq, >irq);
+
mutex_unlock(>reset_lock);
 
return 0;
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.h 
b/drivers/net/ethernet/qualcomm/emac/emac.h
index 1368440..2725507 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.h
+++ b/drivers/net/ethernet/qualcomm/emac/emac.h
@@ -331,7 +331,6 @@ struct emac_adapter {
 
 int emac_reinit_locked(struct emac_adapter *adpt);
 void emac_reg_update32(void __iomem *addr, u32 mask, u32 val);
-irqreturn_t emac_isr(int irq, void *data);
 
 void emac_set_ethtool_ops(struct net_device *netdev);
 void emac_update_hw_stats(struct emac_adapter *adpt);
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.