Re: [PATCH] [v5] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Rob Herring wrote:

>+   interrupt-parent = <>;
>+   #interrupt-cells = <1>;
>+   interrupts = <0 1>;
>+   interrupt-map-mask = <0x>;
>+   interrupt-map = <0  0 76 0
>+1  0 80 0>;

Why? This looks unnecessary.


It may have made sense with an earlier version of the driver that had 
more complex interrupts.  I'll fix it in v6.


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

Re: [PATCH] [v5] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Shanker Donthineni wrote:

 /* Set dma_mask and coherent_dma_mask to 64-bits,
  * if xHC supports 64-bit addressing */
 if (HCC_64BIT_ADDR(xhci->hcc_params) &&
 !dma_set_mask(dev, DMA_BIT_MASK(64))) {
 xhci_dbg(xhci, "Enabling 64-bit DMA addresses.\n");
 dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
 } else {
 /*
  * This is to avoid error in cases where a 32-bit USB
  * controller is used on a 64-bit capable system.
  */
 retval = dma_set_mask(dev, DMA_BIT_MASK(32));


I'm not sure this example is valid because HCC_64BIT_ADDR is part of the 
XCHI specification, so there's an architected way determine whether the 
platform is 64-bit capable or not.  The EMAC has nothing like that.


I can do this:

ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64));
if (ret)
dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32))

but this has always seemed wrong to me, because it doesn't make sense to 
me that DMA_BIT_MASK(64) could ever fail.  DMA_BIT_MASK(64) says that 
the device can handle any physical address, so the device does not 
impose any limitations.  How could that fail?  I have has this question 
multiple times, and I have never gotten a satisfactory answer.


Also, I don't know if I should be using dma_set_mask_and_coherent or 
dma_coerce_mask_and_coherent.  The comment for 
dma_coerce_mask_and_coherent says this:


/*
 * Similar to the above, except it deals with the case where the device
 * does not have dev->dma_mask appropriately setup.
 */

How can I know if the device has dev->dma_mask "appropriately setup"?

Remember, I need a solution that works for DT and ACPI.

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

Re: [PATCH] [v5] net: emac: emac gigabit ethernet controller driver

[Shanker Donthineni]

Timur,

I think, the device driver is responsible for setting the right DMA_MASK 
based on the underlying hardware capability if your driver wants to 
support 64bit DMA.


Example code in drivers/usb/host/xhci.c:

/* Set dma_mask and coherent_dma_mask to 64-bits,
 * if xHC supports 64-bit addressing */
if (HCC_64BIT_ADDR(xhci->hcc_params) &&
!dma_set_mask(dev, DMA_BIT_MASK(64))) {
xhci_dbg(xhci, "Enabling 64-bit DMA addresses.\n");
dma_set_coherent_mask(dev, DMA_BIT_MASK(64));
} else {
/*
 * This is to avoid error in cases where a 32-bit USB
 * controller is used on a 64-bit capable system.
 */
retval = dma_set_mask(dev, DMA_BIT_MASK(32));



On 06/20/2016 12:41 PM, Timur Tabi wrote:

Rob Herring wrote:

>+dma-ranges = <0 0 0x>;

I believe dma-ranges is supposed to be in the bus (parent) node.


Maybe I'm just going to be perpetually confused by dma-ranges, but how 
can I specify that the emac has a different DMA range from another SOC 
device, if dma-ranges is in the parent node?


The EMAC itself is capable of 64-bit DMA internally (I should have 
included a dma_set_mask call with DMA_BIT_MASK(64) in the driver). 
However, the platform typically limits this range.  On FSM9900 and 
QDF2432, it's 32 bits.  On the next server chip, it'll be the full 64 
bits.  I need some way to handle that.




--
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

Re: [PATCH] [v5] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Rob Herring wrote:

>+   dma-ranges = <0 0 0x>;

I believe dma-ranges is supposed to be in the bus (parent) node.


Maybe I'm just going to be perpetually confused by dma-ranges, but how 
can I specify that the emac has a different DMA range from another SOC 
device, if dma-ranges is in the parent node?


The EMAC itself is capable of 64-bit DMA internally (I should have 
included a dma_set_mask call with DMA_BIT_MASK(64) in the driver). 
However, the platform typically limits this range.  On FSM9900 and 
QDF2432, it's 32 bits.  On the next server chip, it'll be the full 64 
bits.  I need some way to handle that.


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

Re: [PATCH] [v5] net: emac: emac gigabit ethernet controller driver

[Rob Herring]

On Tue, Jun 14, 2016 at 05:22:35PM -0500, Timur Tabi wrote:
> Add supports for ethernet controller HW on Qualcomm Technologies, Inc. SoC.
> This driver supports the following features:
> 1) Checksum offload.
> 2) Interrupt coalescing support.
> 3) SGMII phy.
> 4) phylib interface for external phy
> 
> Based on original work by
>   Niranjana Vishwanathapura 
>   Gilad Avidov 
> 
> Signed-off-by: Timur Tabi 
> ---
> 
> v5:
>  - changed author to Timur, added MAINTAINERS entry
>  - use phylib, replacing internal phy code
>  - added support for EMAC internal SGMII v2
>  - fix ~DIS_INT warning
>  - update DT bindings, including removing unused properties
>  - removed interrupt handler for internal sgmii
>  - removed link status check handler/state (replaced with phylib)
>  - removed periodic timer handler (replaced with phylib)
>  - removed power management code (will be rewritten later)
>  - external phy is now required, not optional
>  - removed redundant EMAC_STATUS_DOWN status flag
>  - removed redundant link status and speed variables
>  - removed redundant status bits (vlan strip, promiscuous, loopback, etc)
>  - removed useless watchdog status
>  - removed command-line parameters
>  - cleaned up probe messages
>  - removed redundant params from emac_sgmii_link_init()
>  - always call netdev_completed_queue() (per review comment)
>  - fix emac_napi_rtx() (per review comment)
>  - removed max_ints loop in interrupt handler
>  - removed redundant mutex around phy read/write calls
>  - added lock for reading emac status (per review comment)
>  - generate random MAC address if it can't be read from firmware
>  - replace EMAC_DMA_ADDR_HI/LO with upper/lower_32_bits
>  - don't test return value from platform_get_resource (per review comment)
>  - use net_warn_ratelimited (per review comment)
>  - don't set the dma masks (will be set by DT or IORT code)
>  - remove unused emac_tx_tpd_ts_save()
>  - removed redundant local MTU variable
> 
> v4:
>  - add missing ipv6 header file
>  - correct compatible string
>  - fix spacing in emac_reg_write arrays
>  - drop unnecessary cell-index property
>  - remove unsupported DT properties from docs
>  - remove GPIO initialization and update docs
> 
> v3:
>  - remove most of the memory barriers by using the non xxx_relaxed() api.
>  - remove RSS and WOL support.
>  - correct comments from physical address to dma address.
>  - rearrange structs to make them packed.
>  - replace polling loops with readl_poll_timeout().
>  - remove unnecessary wrapper functions from phy layer.
>  - add blank line before return statements.
>  - set to null clocks after clk_put().
>  - use module_platform_driver() and dma_set_mask_and_coherent()
>  - replace long hex bitmasks with BIT() macro.
> 
> v2:
>  - replace hw bit fields to macros with bitwise operations.
>  - change all iterators to unsized types (int)
>  - some minor code flow improvements.
>  - change return type to void for functions which return value is never
>used.
>  - replace instance of l_relaxed() io followed by mb() with a
>readl()/writel().
> 
> 
>  .../devicetree/bindings/net/qcom-emac.txt  |   66 +
>  MAINTAINERS|6 +
>  drivers/net/ethernet/qualcomm/Kconfig  |   11 +
>  drivers/net/ethernet/qualcomm/Makefile |2 +
>  drivers/net/ethernet/qualcomm/emac/Makefile|7 +
>  drivers/net/ethernet/qualcomm/emac/emac-mac.c  | 1674 
> 
>  drivers/net/ethernet/qualcomm/emac/emac-mac.h  |  284 
>  drivers/net/ethernet/qualcomm/emac/emac-phy.c  |  211 +++
>  drivers/net/ethernet/qualcomm/emac/emac-phy.h  |   32 +
>  drivers/net/ethernet/qualcomm/emac/emac-sgmii.c|  699 
>  drivers/net/ethernet/qualcomm/emac/emac-sgmii.h|   24 +
>  drivers/net/ethernet/qualcomm/emac/emac.c  |  798 ++
>  drivers/net/ethernet/qualcomm/emac/emac.h  |  370 +
>  13 files changed, 4184 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/qcom-emac.txt
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/Makefile
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-mac.c
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-mac.h
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-phy.c
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-phy.h
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-sgmii.h
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac.c
>  create mode 100644 drivers/net/ethernet/qualcomm/emac/emac.h
> 
> diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt 
> b/Documentation/devicetree/bindings/net/qcom-emac.txt
> new file mode 100644
> index 000..e48a9b9
> --- /dev/null
> +++ 

Re: [PATCH] [v5] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

David Miller wrote:

Please always order local variable declarations from longest to shortest
line.

Audit your entire driver for this issue, thanks.


I will fix this in v6.

--
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] [v5] net: emac: emac gigabit ethernet controller driver

[David Miller]

From: Timur Tabi 
Date: Tue, 14 Jun 2016 17:22:35 -0500

> +/* Free all descriptors of given receive queue */
> +static void emac_rx_q_free_descs(struct emac_adapter *adpt)
> +{
> + struct emac_rx_queue *rx_q = >rx_q;
> + struct device *dev = adpt->netdev->dev.parent;
> + size_t size;
> + int i;

Please always order local variable declarations from longest to shortest
line.

Audit your entire driver for this issue, thanks.

[PATCH] [v5] net: emac: emac gigabit ethernet controller driver

[Timur Tabi]

Add supports for ethernet controller HW on Qualcomm Technologies, Inc. SoC.
This driver supports the following features:
1) Checksum offload.
2) Interrupt coalescing support.
3) SGMII phy.
4) phylib interface for external phy

Based on original work by
Niranjana Vishwanathapura 
Gilad Avidov 

Signed-off-by: Timur Tabi 
---

v5:
 - changed author to Timur, added MAINTAINERS entry
 - use phylib, replacing internal phy code
 - added support for EMAC internal SGMII v2
 - fix ~DIS_INT warning
 - update DT bindings, including removing unused properties
 - removed interrupt handler for internal sgmii
 - removed link status check handler/state (replaced with phylib)
 - removed periodic timer handler (replaced with phylib)
 - removed power management code (will be rewritten later)
 - external phy is now required, not optional
 - removed redundant EMAC_STATUS_DOWN status flag
 - removed redundant link status and speed variables
 - removed redundant status bits (vlan strip, promiscuous, loopback, etc)
 - removed useless watchdog status
 - removed command-line parameters
 - cleaned up probe messages
 - removed redundant params from emac_sgmii_link_init()
 - always call netdev_completed_queue() (per review comment)
 - fix emac_napi_rtx() (per review comment)
 - removed max_ints loop in interrupt handler
 - removed redundant mutex around phy read/write calls
 - added lock for reading emac status (per review comment)
 - generate random MAC address if it can't be read from firmware
 - replace EMAC_DMA_ADDR_HI/LO with upper/lower_32_bits
 - don't test return value from platform_get_resource (per review comment)
 - use net_warn_ratelimited (per review comment)
 - don't set the dma masks (will be set by DT or IORT code)
 - remove unused emac_tx_tpd_ts_save()
 - removed redundant local MTU variable

v4:
 - add missing ipv6 header file
 - correct compatible string
 - fix spacing in emac_reg_write arrays
 - drop unnecessary cell-index property
 - remove unsupported DT properties from docs
 - remove GPIO initialization and update docs

v3:
 - remove most of the memory barriers by using the non xxx_relaxed() api.
 - remove RSS and WOL support.
 - correct comments from physical address to dma address.
 - rearrange structs to make them packed.
 - replace polling loops with readl_poll_timeout().
 - remove unnecessary wrapper functions from phy layer.
 - add blank line before return statements.
 - set to null clocks after clk_put().
 - use module_platform_driver() and dma_set_mask_and_coherent()
 - replace long hex bitmasks with BIT() macro.

v2:
 - replace hw bit fields to macros with bitwise operations.
 - change all iterators to unsized types (int)
 - some minor code flow improvements.
 - change return type to void for functions which return value is never
   used.
 - replace instance of l_relaxed() io followed by mb() with a
   readl()/writel().


 .../devicetree/bindings/net/qcom-emac.txt  |   66 +
 MAINTAINERS|6 +
 drivers/net/ethernet/qualcomm/Kconfig  |   11 +
 drivers/net/ethernet/qualcomm/Makefile |2 +
 drivers/net/ethernet/qualcomm/emac/Makefile|7 +
 drivers/net/ethernet/qualcomm/emac/emac-mac.c  | 1674 
 drivers/net/ethernet/qualcomm/emac/emac-mac.h  |  284 
 drivers/net/ethernet/qualcomm/emac/emac-phy.c  |  211 +++
 drivers/net/ethernet/qualcomm/emac/emac-phy.h  |   32 +
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c|  699 
 drivers/net/ethernet/qualcomm/emac/emac-sgmii.h|   24 +
 drivers/net/ethernet/qualcomm/emac/emac.c  |  798 ++
 drivers/net/ethernet/qualcomm/emac/emac.h  |  370 +
 13 files changed, 4184 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/qcom-emac.txt
 create mode 100644 drivers/net/ethernet/qualcomm/emac/Makefile
 create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-mac.c
 create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-mac.h
 create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-phy.c
 create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-phy.h
 create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-sgmii.c
 create mode 100644 drivers/net/ethernet/qualcomm/emac/emac-sgmii.h
 create mode 100644 drivers/net/ethernet/qualcomm/emac/emac.c
 create mode 100644 drivers/net/ethernet/qualcomm/emac/emac.h

diff --git a/Documentation/devicetree/bindings/net/qcom-emac.txt 
b/Documentation/devicetree/bindings/net/qcom-emac.txt
new file mode 100644
index 000..e48a9b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/qcom-emac.txt
@@ -0,0 +1,66 @@
+Qualcomm EMAC Gigabit Ethernet Controller
+
+Required properties:
+- compatible : Should be "qcom,fsm9900-emac".
+- reg : Offset and length of the register regions for the device
+- reg-names : Register region names referenced in 'reg'