[U-Boot,2/2] net: add driver for Synopsys Ethernet QoS device

From: Stephen Warren <swarren@nvidia.com>

This driver supports the Synopsys Designware Ethernet QoS (Quality of
Service) a/k/a eqos IP block, which is a different design than the HW
supported by the existing designware.c driver. The IP supports many
options for bus type, clocking/reset structure, and feature list. This
driver currently supports the specific configuration used in NVIDIA's
Tegra186 chip, but should be extensible to other combinations quite
easily, as explained in the source.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 drivers/net/Kconfig       |   11 +
 drivers/net/Makefile      |    1 +
 drivers/net/dwc_eth_qos.c | 1492 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1504 insertions(+)
 create mode 100644 drivers/net/dwc_eth_qos.c

On 12 September 2016 at 11:48, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> This driver supports the Synopsys Designware Ethernet QoS (Quality of
> Service) a/k/a eqos IP block, which is a different design than the HW
> supported by the existing designware.c driver. The IP supports many
> options for bus type, clocking/reset structure, and feature list. This
> driver currently supports the specific configuration used in NVIDIA's
> Tegra186 chip, but should be extensible to other combinations quite
> easily, as explained in the source.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/net/Kconfig       |   11 +
>  drivers/net/Makefile      |    1 +
>  drivers/net/dwc_eth_qos.c | 1492 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1504 insertions(+)
>  create mode 100644 drivers/net/dwc_eth_qos.c

Reviewed-by: Simon Glass <sjg@chromium.org>

Hi Stephen,

Thanks for sending this! I have some comments below.

Cheers,
-Joe

On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> This driver supports the Synopsys Designware Ethernet QoS (Quality of
> Service) a/k/a eqos IP block, which is a different design than the HW
> supported by the existing designware.c driver. The IP supports many
> options for bus type, clocking/reset structure, and feature list. This
> driver currently supports the specific configuration used in NVIDIA's
> Tegra186 chip, but should be extensible to other combinations quite
> easily, as explained in the source.
>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
>  drivers/net/Kconfig       |   11 +
>  drivers/net/Makefile      |    1 +
>  drivers/net/dwc_eth_qos.c | 1492 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1504 insertions(+)
>  create mode 100644 drivers/net/dwc_eth_qos.c
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index be3ed73e5221..8b35415a04ce 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -64,6 +64,17 @@ config ALTERA_TSE
>           Please find details on the "Triple-Speed Ethernet MegaCore Function
>           Resource Center" of Altera.
>
> +config DWC_ETH_QOS
> +       bool "Synopsys DWC Ethernet QOS device support"
> +       depends on DM_ETH
> +       select PHYLIB
> +       help
> +         This driver supports the Synopsys Designware Ethernet QOS (Quality
> +         Of Service) IP block. The IP supports many options for bus type,
> +         clocking/reset structure, and feature list. This driver currently
> +         supports the specific configuration used in NVIDIA's Tegra186 chip,
> +         but should be extensible to other combinations quite easily.
> +
>  config E1000
>         bool "Intel PRO/1000 Gigabit Ethernet support"
>         help
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index a4485266d457..9a7bfc6d5b05 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -76,3 +76,4 @@ obj-$(CONFIG_FSL_MC_ENET) += ldpaa_eth/
>  obj-$(CONFIG_FSL_MEMAC) += fm/memac_phy.o
>  obj-$(CONFIG_VSC9953) += vsc9953.o
>  obj-$(CONFIG_PIC32_ETH) += pic32_mdio.o pic32_eth.o
> +obj-$(CONFIG_DWC_ETH_QOS) += dwc_eth_qos.o
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> new file mode 100644
> index 000000000000..3be7a187d3e1
> --- /dev/null
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -0,0 +1,1492 @@
> +/*
> + * Copyright (c) 2016, NVIDIA CORPORATION.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + *
> + * Portions based on U-Boot's rtl8169.c.
> + */
> +
> +/*
> + * This driver supports the Synopsys Designware Ethernet QOS (Quality Of
> + * Service) IP block. The IP supports multiple options for bus type, clocking/
> + * reset structure, and feature list. This driver currently supports the
> + * specific configuration used in NVIDIA's Tegra186 chip.
> + *
> + * The driver is written such that generic core logic is kept separate from
> + * configuration-specific logic. Code that interacts with configuration-
> + * specific resources is split out into separate functions to avoid polluting
> + * common code. If/when this driver is enhanced to support multiple
> + * configurations, the core code should be adapted to call all configuration-
> + * specific functions through function pointers, with the definition of those
> + * function pointers being supplied by struct udevice_id eqos_ids[]'s .data
> + * field.
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <memalign.h>
> +#include <miiphy.h>
> +#include <net.h>
> +#include <netdev.h>
> +#include <phy.h>
> +#include <reset.h>
> +#include <asm/gpio.h>
> +#include <asm/io.h>
> +
> +/* Core registers */

Why are registers not defined as a struct? That is the common way that
is accepted to represent registers in a driver.

> +
> +#define EQOS_MAC_CONFIGURATION                         0
> +#define EQOS_MAC_CONFIGURATION_GPSLCE                  BIT(23)
> +#define EQOS_MAC_CONFIGURATION_CST                     BIT(21)
> +#define EQOS_MAC_CONFIGURATION_ACS                     BIT(20)
> +#define EQOS_MAC_CONFIGURATION_WD                      BIT(19)
> +#define EQOS_MAC_CONFIGURATION_JD                      BIT(17)
> +#define EQOS_MAC_CONFIGURATION_JE                      BIT(16)
> +#define EQOS_MAC_CONFIGURATION_PS                      BIT(15)
> +#define EQOS_MAC_CONFIGURATION_FES                     BIT(14)
> +#define EQOS_MAC_CONFIGURATION_DM                      BIT(13)
> +#define EQOS_MAC_CONFIGURATION_TE                      BIT(1)
> +#define EQOS_MAC_CONFIGURATION_RE                      BIT(0)
> +
> +#define EQOS_MAC_Q0_TX_FLOW_CTRL                       0x70
> +#define EQOS_MAC_Q0_TX_FLOW_CTRL_PT_SHIFT              16
> +#define EQOS_MAC_Q0_TX_FLOW_CTRL_PT_MASK               0xffff
> +#define EQOS_MAC_Q0_TX_FLOW_CTRL_TFE                   BIT(1)
> +
> +#define EQOS_MAC_RX_FLOW_CTRL                          0x90
> +#define EQOS_MAC_RX_FLOW_CTRL_RFE                      BIT(0)
> +
> +#define EQOS_MAC_TXQ_PRTY_MAP0                         0x98
> +#define EQOS_MAC_TXQ_PRTY_MAP0_PSTQ0_SHIFT             0
> +#define EQOS_MAC_TXQ_PRTY_MAP0_PSTQ0_MASK              0xff
> +
> +#define EQOS_MAC_RXQ_CTRL0                             0xa0
> +#define EQOS_MAC_RXQ_CTRL0_RXQ0EN_SHIFT                        0
> +#define EQOS_MAC_RXQ_CTRL0_RXQ0EN_MASK                 3
> +#define EQOS_MAC_RXQ_CTRL0_RXQ0EN_NOT_ENABLED          0
> +#define EQOS_MAC_RXQ_CTRL0_RXQ0EN_ENABLED_DCB          2
> +
> +#define EQOS_MAC_RXQ_CTRL2                             0xa8
> +#define EQOS_MAC_RXQ_CTRL2_PSRQ0_SHIFT                 0
> +#define EQOS_MAC_RXQ_CTRL2_PSRQ0_MASK                  0xff
> +
> +#define EQOS_MAC_1US_TIC_COUNTER                       0xdc
> +
> +#define EQOS_MAC_HW_FEATURE0                           0x11c
> +
> +#define EQOS_MAC_HW_FEATURE1                           0x120
> +#define EQOS_MAC_HW_FEATURE1_TXFIFOSIZE_SHIFT          6
> +#define EQOS_MAC_HW_FEATURE1_TXFIFOSIZE_MASK           0x1f
> +#define EQOS_MAC_HW_FEATURE1_RXFIFOSIZE_SHIFT          0
> +#define EQOS_MAC_HW_FEATURE1_RXFIFOSIZE_MASK           0x1f
> +
> +#define EQOS_MAC_HW_FEATURE2                           0x124
> +
> +#define EQOS_MAC_MDIO_ADDRESS                          0x200
> +#define EQOS_MAC_MDIO_ADDRESS_PA_SHIFT                 21
> +#define EQOS_MAC_MDIO_ADDRESS_RDA_SHIFT                        16
> +#define EQOS_MAC_MDIO_ADDRESS_CR_SHIFT                 8
> +#define EQOS_MAC_MDIO_ADDRESS_CR_20_35                 2
> +#define EQOS_MAC_MDIO_ADDRESS_SKAP                     BIT(4)
> +#define EQOS_MAC_MDIO_ADDRESS_GOC_SHIFT                        2
> +#define EQOS_MAC_MDIO_ADDRESS_GOC_READ                 3
> +#define EQOS_MAC_MDIO_ADDRESS_GOC_WRITE                        1
> +#define EQOS_MAC_MDIO_ADDRESS_C45E                     BIT(1)
> +#define EQOS_MAC_MDIO_ADDRESS_GB                       BIT(0)
> +
> +#define EQOS_MAC_MDIO_DATA                             0x204
> +#define EQOS_MAC_MDIO_DATA_GD_MASK                     0xffff
> +
> +#define EQOS_MAC_ADDRESS0_HIGH                         0x300
> +
> +#define EQOS_MAC_ADDRESS0_LOW                          0x304
> +
> +#define EQOS_MTL_TXQ0_OPERATION_MODE                   0xd00
> +#define EQOS_MTL_TXQ0_OPERATION_MODE_TQS_SHIFT         16
> +#define EQOS_MTL_TXQ0_OPERATION_MODE_TQS_MASK          0x1ff
> +#define EQOS_MTL_TXQ0_OPERATION_MODE_TXQEN_SHIFT       2
> +#define EQOS_MTL_TXQ0_OPERATION_MODE_TXQEN_MASK                3
> +#define EQOS_MTL_TXQ0_OPERATION_MODE_TXQEN_ENABLED     2
> +#define EQOS_MTL_TXQ0_OPERATION_MODE_TSF               BIT(1)
> +#define EQOS_MTL_TXQ0_OPERATION_MODE_FTQ               BIT(0)
> +
> +#define EQOS_MTL_TXQ0_DEBUG                            0xd08
> +#define EQOS_MTL_TXQ0_DEBUG_TXQSTS                     BIT(4)
> +#define EQOS_MTL_TXQ0_DEBUG_TRCSTS_SHIFT               1
> +#define EQOS_MTL_TXQ0_DEBUG_TRCSTS_MASK                        3
> +
> +#define EQOS_MTL_TXQ0_QUANTUM_WEIGHT                   0xd18
> +
> +#define EQOS_MTL_RXQ0_OPERATION_MODE                   0xd30
> +#define EQOS_MTL_RXQ0_OPERATION_MODE_RQS_SHIFT         20
> +#define EQOS_MTL_RXQ0_OPERATION_MODE_RQS_MASK          0x3ff
> +#define EQOS_MTL_RXQ0_OPERATION_MODE_RFD_SHIFT         14
> +#define EQOS_MTL_RXQ0_OPERATION_MODE_RFD_MASK          0x3f
> +#define EQOS_MTL_RXQ0_OPERATION_MODE_RFA_SHIFT         8
> +#define EQOS_MTL_RXQ0_OPERATION_MODE_RFA_MASK          0x3f
> +#define EQOS_MTL_RXQ0_OPERATION_MODE_EHFC              BIT(7)
> +#define EQOS_MTL_RXQ0_OPERATION_MODE_RSF               BIT(5)
> +
> +#define EQOS_MTL_RXQ0_DEBUG                            0xd38
> +#define EQOS_MTL_RXQ0_DEBUG_PRXQ_SHIFT                 16
> +#define EQOS_MTL_RXQ0_DEBUG_PRXQ_MASK                  0x7fff
> +#define EQOS_MTL_RXQ0_DEBUG_RXQSTS_SHIFT               4
> +#define EQOS_MTL_RXQ0_DEBUG_RXQSTS_MASK                        3
> +
> +#define EQOS_DMA_MODE                                  0x1000
> +#define EQOS_DMA_MODE_SWR                              BIT(0)
> +
> +#define EQOS_DMA_SYSBUS_MODE                           0x1004
> +#define EQOS_DMA_SYSBUS_MODE_RD_OSR_LMT_SHIFT          16
> +#define EQOS_DMA_SYSBUS_MODE_RD_OSR_LMT_MASK           0xf
> +#define EQOS_DMA_SYSBUS_MODE_EAME                      BIT(11)
> +#define EQOS_DMA_SYSBUS_MODE_BLEN16                    BIT(3)
> +#define EQOS_DMA_SYSBUS_MODE_BLEN8                     BIT(2)
> +#define EQOS_DMA_SYSBUS_MODE_BLEN4                     BIT(1)
> +
> +#define EQOS_DMA_CH0_CONTROL                           0x1100
> +#define EQOS_DMA_CH0_CONTROL_PBLX8                     BIT(16)
> +
> +#define EQOS_DMA_CH0_TX_CONTROL                                0x1104
> +#define EQOS_DMA_CH0_TX_CONTROL_TXPBL_SHIFT            16
> +#define EQOS_DMA_CH0_TX_CONTROL_TXPBL_MASK             0x3f
> +#define EQOS_DMA_CH0_TX_CONTROL_OSP                    BIT(4)
> +#define EQOS_DMA_CH0_TX_CONTROL_ST                     BIT(0)
> +
> +#define EQOS_DMA_CH0_RX_CONTROL                                0x1108
> +#define EQOS_DMA_CH0_RX_CONTROL_RXPBL_SHIFT            16
> +#define EQOS_DMA_CH0_RX_CONTROL_RXPBL_MASK             0x3f
> +#define EQOS_DMA_CH0_RX_CONTROL_RBSZ_SHIFT             1
> +#define EQOS_DMA_CH0_RX_CONTROL_RBSZ_MASK              0x3fff
> +#define EQOS_DMA_CH0_RX_CONTROL_SR                     BIT(0)
> +
> +#define EQOS_DMA_CH0_TXDESC_LIST_HADDRESS              0x1110
> +
> +#define EQOS_DMA_CH0_TXDESC_LIST_ADDRESS               0x1114
> +
> +#define EQOS_DMA_CH0_RXDESC_LIST_HADDRESS              0x1118
> +
> +#define EQOS_DMA_CH0_RXDESC_LIST_ADDRESS               0x111c
> +
> +#define EQOS_DMA_CH0_TXDESC_TAIL_POINTER               0x1120
> +
> +#define EQOS_DMA_CH0_RXDESC_TAIL_POINTER               0x1128
> +
> +#define EQOS_DMA_CH0_TXDESC_RING_LENGTH                        0x112c
> +
> +#define EQOS_DMA_CH0_RXDESC_RING_LENGTH                        0x1130
> +
> +/* Registers located at 0x8000 and later are Tegra-specific */
> +
> +#define EQOS_SDMEMCOMPPADCTRL                          0x8800
> +#define EQOS_SDMEMCOMPPADCTRL_PAD_E_INPUT_OR_E_PWRD    BIT(31)
> +
> +#define EQOS_AUTO_CAL_CONFIG                           0x8804
> +#define EQOS_AUTO_CAL_CONFIG_START                     BIT(31)
> +#define EQOS_AUTO_CAL_CONFIG_ENABLE                    BIT(29)
> +
> +#define EQOS_AUTO_CAL_STATUS                           0x880c
> +#define EQOS_AUTO_CAL_STATUS_ACTIVE                    BIT(31)
> +
> +/* Descriptors */
> +
> +#define EQOS_DESCRIPTOR_WORDS  4
> +#define EQOS_DESCRIPTOR_SIZE   (EQOS_DESCRIPTOR_WORDS * 4)
> +/* We assume ARCH_DMA_MINALIGN >= 16; 16 is the EQOS HW minimum */
> +#define EQOS_DESCRIPTOR_ALIGN  ARCH_DMA_MINALIGN
> +#define EQOS_DESCRIPTORS_TX    4
> +#define EQOS_DESCRIPTORS_RX    4
> +#define EQOS_DESCRIPTORS_NUM   (EQOS_DESCRIPTORS_TX + EQOS_DESCRIPTORS_RX)
> +#define EQOS_DESCRIPTORS_SIZE  ALIGN(EQOS_DESCRIPTORS_NUM * \
> +                                     EQOS_DESCRIPTOR_SIZE, ARCH_DMA_MINALIGN)
> +#define EQOS_BUFFER_ALIGN      ARCH_DMA_MINALIGN
> +#define EQOS_MAX_PACKET_SIZE   ALIGN(1568, ARCH_DMA_MINALIGN)
> +#define EQOS_RX_BUFFER_SIZE    (EQOS_DESCRIPTORS_RX * EQOS_MAX_PACKET_SIZE)
> +
> +/*
> + * Warn if the cache-line size is larger than the descriptor size. In such
> + * cases the driver will likely fail because the CPU needs to flush the cache
> + * when requeuing RX buffers, therefore descriptors written by the hardware
> + * may be discarded.
> + *
> + * This can be fixed by defining CONFIG_SYS_NONCACHED_MEMORY which will cause
> + * the driver to allocate descriptors from a pool of non-cached memory.
> + */
> +#if EQOS_DESCRIPTOR_SIZE < ARCH_DMA_MINALIGN
> +#if !defined(CONFIG_SYS_NONCACHED_MEMORY) && \
> +       !defined(CONFIG_SYS_DCACHE_OFF) && !defined(CONFIG_X86)

Please include an explanation for why x86 is special.

> +#warning Cache line size is larger than descriptor size
> +#endif
> +#endif
> +
> +#define EQOS_DESC3_OWN         BIT(31)
> +#define EQOS_DESC3_FD          BIT(29)
> +#define EQOS_DESC3_LD          BIT(28)
> +#define EQOS_DESC3_BUF1V       BIT(24)
> +
> +struct eqos_priv {
> +       struct udevice *dev;
> +       fdt_addr_t regs;
> +       struct reset_ctl reset_ctl;
> +       struct gpio_desc phy_reset_gpio;
> +       struct clk clk_master_bus;
> +       struct clk clk_rx;
> +       struct clk clk_ptp_ref;
> +       struct clk clk_tx;
> +       struct clk clk_slave_bus;
> +       struct mii_dev *mii;
> +       struct phy_device *phy;
> +       u32 *descs, *tx_descs, *rx_descs;
> +       int tx_desc_idx, rx_desc_idx;
> +       void *tx_dma_buf;
> +       void *rx_dma_buf;
> +       void *rx_pkt;
> +       bool started;
> +};
> +
> +/*
> + * TX and RX descriptors are 16 bytes. This causes problems with the cache
> + * maintenance on CPUs where the cache-line size exceeds the size of these
> + * descriptors. What will happen is that when the driver receives a packet
> + * it will be immediately requeued for the hardware to reuse. The CPU will
> + * therefore need to flush the cache-line containing the descriptor, which
> + * will cause all other descriptors in the same cache-line to be flushed
> + * along with it. If one of those descriptors had been written to by the
> + * device those changes (and the associated packet) will be lost.
> + *
> + * To work around this, we make use of non-cached memory if available. If
> + * descriptors are mapped uncached there's no need to manually flush them
> + * or invalidate them.
> + *
> + * Note that this only applies to descriptors. The packet data buffers do
> + * not have the same constraints since they are 1536 bytes large, so they
> + * are unlikely to share cache-lines.
> + */
> +static u32 *eqos_alloc_descs(unsigned int num)
> +{
> +#ifdef CONFIG_SYS_NONCACHED_MEMORY
> +       return (u32 *)noncached_alloc(EQOS_DESCRIPTORS_SIZE,
> +                                     EQOS_DESCRIPTOR_ALIGN);
> +#else
> +       return memalign(EQOS_DESCRIPTOR_ALIGN, EQOS_DESCRIPTORS_SIZE);
> +#endif
> +}
> +
> +static void eqos_free_descs(u32 *descs)
> +{
> +#ifdef CONFIG_SYS_NONCACHED_MEMORY
> +       /* FIXME: noncached_alloc() has no opposite */
> +#else
> +       free(descs);
> +#endif
> +}
> +
> +static void eqos_inval_desc(u32 *desc)
> +{
> +#ifndef CONFIG_SYS_NONCACHED_MEMORY
> +       unsigned long start = (unsigned long)desc & ~(ARCH_DMA_MINALIGN - 1);
> +       unsigned long end = ALIGN(start + EQOS_DESCRIPTOR_SIZE,
> +                                 ARCH_DMA_MINALIGN);
> +
> +       invalidate_dcache_range(start, end);
> +#endif
> +}
> +
> +static void eqos_flush_desc(u32 *desc)
> +{
> +#ifndef CONFIG_SYS_NONCACHED_MEMORY
> +       flush_cache((unsigned long)desc, EQOS_DESCRIPTOR_SIZE);
> +#endif
> +}
> +
> +static void eqos_inval_buffer(void *buf, size_t size)
> +{
> +       unsigned long start = (unsigned long)buf & ~(ARCH_DMA_MINALIGN - 1);
> +       unsigned long end = ALIGN(start + size, ARCH_DMA_MINALIGN);
> +
> +       invalidate_dcache_range(start, end);
> +}
> +
> +static void eqos_flush_buffer(void *buf, size_t size)
> +{
> +       flush_cache((unsigned long)buf, size);
> +}
> +
> +static int eqos_mdio_wait_idle(struct eqos_priv *eqos)
> +{
> +       int i;
> +       u32 val;
> +
> +       for (i = 0; i < 1000000; i++) {
> +               if (i)
> +                       udelay(1);
> +               val = readl(eqos->regs + EQOS_MAC_MDIO_ADDRESS);
> +               if (!(val & EQOS_MAC_MDIO_ADDRESS_GB))
> +                       return 0;

Please use wait_bit here.

> +       }
> +
> +       return -ETIMEDOUT;
> +}
> +
> +static int eqos_mdio_read(struct mii_dev *bus, int mdio_addr, int mdio_devad,
> +                         int mdio_reg)
> +{
> +       struct eqos_priv *eqos = bus->priv;
> +       u32 val;
> +       int ret;
> +
> +       debug("%s(dev=%p, addr=%x, reg=%d):\n", __func__, eqos->dev, mdio_addr,
> +             mdio_reg);
> +
> +       ret = eqos_mdio_wait_idle(eqos);
> +       if (ret) {
> +               error("MDIO not idle at entry");
> +               return ret;
> +       }
> +
> +       val = readl(eqos->regs + EQOS_MAC_MDIO_ADDRESS);
> +       val &= EQOS_MAC_MDIO_ADDRESS_SKAP |
> +               EQOS_MAC_MDIO_ADDRESS_C45E;
> +       val |= (mdio_addr << EQOS_MAC_MDIO_ADDRESS_PA_SHIFT) |
> +               (mdio_reg << EQOS_MAC_MDIO_ADDRESS_RDA_SHIFT) |
> +               (EQOS_MAC_MDIO_ADDRESS_CR_20_35 <<
> +                EQOS_MAC_MDIO_ADDRESS_CR_SHIFT) |
> +               (EQOS_MAC_MDIO_ADDRESS_GOC_READ <<
> +                EQOS_MAC_MDIO_ADDRESS_GOC_SHIFT) |
> +               EQOS_MAC_MDIO_ADDRESS_GB;
> +       writel(val, eqos->regs + EQOS_MAC_MDIO_ADDRESS);
> +
> +       udelay(10);
> +
> +       ret = eqos_mdio_wait_idle(eqos);
> +       if (ret) {
> +               error("MDIO read didn't complete");
> +               return ret;
> +       }
> +
> +       val = readl(eqos->regs + EQOS_MAC_MDIO_DATA);
> +       val &= EQOS_MAC_MDIO_DATA_GD_MASK;
> +
> +       debug("%s: val=%x\n", __func__, val);
> +
> +       return val;
> +}
> +
> +static int eqos_mdio_write(struct mii_dev *bus, int mdio_addr, int mdio_devad,
> +                          int mdio_reg, u16 mdio_val)
> +{
> +       struct eqos_priv *eqos = bus->priv;
> +       u32 val;
> +       int ret;
> +
> +       debug("%s(dev=%p, addr=%x, reg=%d, val=%x):\n", __func__, eqos->dev,
> +             mdio_addr, mdio_reg, mdio_val);
> +
> +       ret = eqos_mdio_wait_idle(eqos);
> +       if (ret) {
> +               error("MDIO not idle at entry");
> +               return ret;
> +       }
> +
> +       writel(mdio_val, eqos->regs + EQOS_MAC_MDIO_DATA);
> +
> +       val = readl(eqos->regs + EQOS_MAC_MDIO_ADDRESS);
> +       val &= EQOS_MAC_MDIO_ADDRESS_SKAP |
> +               EQOS_MAC_MDIO_ADDRESS_C45E;
> +       val |= (mdio_addr << EQOS_MAC_MDIO_ADDRESS_PA_SHIFT) |
> +               (mdio_reg << EQOS_MAC_MDIO_ADDRESS_RDA_SHIFT) |
> +               (EQOS_MAC_MDIO_ADDRESS_CR_20_35 <<
> +                EQOS_MAC_MDIO_ADDRESS_CR_SHIFT) |
> +               (EQOS_MAC_MDIO_ADDRESS_GOC_WRITE <<
> +                EQOS_MAC_MDIO_ADDRESS_GOC_SHIFT) |
> +               EQOS_MAC_MDIO_ADDRESS_GB;
> +       writel(val, eqos->regs + EQOS_MAC_MDIO_ADDRESS);
> +
> +       udelay(10);
> +
> +       ret = eqos_mdio_wait_idle(eqos);
> +       if (ret) {
> +               error("MDIO read didn't complete");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int eqos_start_clks_tegra(struct udevice *dev)

Why is this not in a board file and a separate patch?

> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       int ret;
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       ret = clk_enable(&eqos->clk_slave_bus);
> +       if (ret < 0) {
> +               error("clk_enable(clk_slave_bus) failed: %d", ret);
> +               goto err;
> +       }
> +
> +       ret = clk_enable(&eqos->clk_master_bus);
> +       if (ret < 0) {
> +               error("clk_enable(clk_master_bus) failed: %d", ret);
> +               goto err_disable_clk_slave_bus;
> +       }
> +
> +       ret = clk_enable(&eqos->clk_rx);
> +       if (ret < 0) {
> +               error("clk_enable(clk_rx) failed: %d", ret);
> +               goto err_disable_clk_master_bus;
> +       }
> +
> +       ret = clk_enable(&eqos->clk_ptp_ref);
> +       if (ret < 0) {
> +               error("clk_enable(clk_ptp_ref) failed: %d", ret);
> +               goto err_disable_clk_rx;
> +       }
> +
> +       ret = clk_set_rate(&eqos->clk_ptp_ref, 125 * 1000 * 1000);
> +       if (ret < 0) {
> +               error("clk_set_rate(clk_ptp_ref) failed: %d", ret);
> +               goto err_disable_clk_ptp_ref;
> +       }
> +
> +       ret = clk_enable(&eqos->clk_tx);
> +       if (ret < 0) {
> +               error("clk_enable(clk_tx) failed: %d", ret);
> +               goto err_disable_clk_ptp_ref;
> +       }
> +
> +       debug("%s: OK\n", __func__);
> +       return 0;
> +
> +err_disable_clk_ptp_ref:
> +       clk_disable(&eqos->clk_ptp_ref);
> +err_disable_clk_rx:
> +       clk_disable(&eqos->clk_rx);
> +err_disable_clk_master_bus:
> +       clk_disable(&eqos->clk_master_bus);
> +err_disable_clk_slave_bus:
> +       clk_disable(&eqos->clk_slave_bus);
> +err:
> +       debug("%s: FAILED: %d\n", __func__, ret);
> +       return ret;
> +}
> +
> +void eqos_stop_clks_tegra(struct udevice *dev)

Ditto for all *_tegra()

> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       clk_disable(&eqos->clk_tx);
> +       clk_disable(&eqos->clk_ptp_ref);
> +       clk_disable(&eqos->clk_rx);
> +       clk_disable(&eqos->clk_master_bus);
> +       clk_disable(&eqos->clk_slave_bus);
> +
> +       debug("%s: OK\n", __func__);
> +}
> +
> +static int eqos_start_resets_tegra(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       int ret;
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 1);
> +       if (ret < 0) {
> +               error("dm_gpio_set_value(phy_reset, assert) failed: %d", ret);
> +               return ret;
> +       }
> +
> +       udelay(2);
> +
> +       ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0);
> +       if (ret < 0) {
> +               error("dm_gpio_set_value(phy_reset, deassert) failed: %d", ret);
> +               return ret;
> +       }
> +
> +       ret = reset_assert(&eqos->reset_ctl);
> +       if (ret < 0) {
> +               error("reset_assert() failed: %d", ret);
> +               return ret;
> +       }
> +
> +       udelay(2);
> +
> +       ret = reset_deassert(&eqos->reset_ctl);
> +       if (ret < 0) {
> +               error("reset_deassert() failed: %d", ret);
> +               return ret;
> +       }
> +
> +       debug("%s: OK\n", __func__);
> +       return 0;
> +}
> +
> +static int eqos_stop_resets_tegra(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       reset_assert(&eqos->reset_ctl);
> +       dm_gpio_set_value(&eqos->phy_reset_gpio, 1);
> +
> +       return 0;
> +}
> +
> +static int eqos_calibrate_pads_tegra(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       u32 val;
> +       int i, ret;
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       setbits_le32(eqos->regs + EQOS_SDMEMCOMPPADCTRL,
> +                    EQOS_SDMEMCOMPPADCTRL_PAD_E_INPUT_OR_E_PWRD);
> +
> +       udelay(1);
> +
> +       setbits_le32(eqos->regs + EQOS_AUTO_CAL_CONFIG,
> +                    EQOS_AUTO_CAL_CONFIG_START | EQOS_AUTO_CAL_CONFIG_ENABLE);
> +
> +       for (i = 0; i <= 10; i++) {
> +               if (i)
> +                       udelay(1);
> +               val = readl(eqos->regs + EQOS_AUTO_CAL_STATUS);
> +               if (val & EQOS_AUTO_CAL_STATUS_ACTIVE)
> +                       break;
> +       }
> +       if (!(val & EQOS_AUTO_CAL_STATUS_ACTIVE)) {
> +               error("calibrate didn't start");
> +               ret = -ETIMEDOUT;
> +               goto failed;
> +       }
> +
> +       for (i = 0; i <= 10; i++) {
> +               if (i)
> +                       udelay(20);
> +               val = readl(eqos->regs + EQOS_AUTO_CAL_STATUS);
> +               if (!(val & EQOS_AUTO_CAL_STATUS_ACTIVE))
> +                       break;
> +       }
> +       if (val & EQOS_AUTO_CAL_STATUS_ACTIVE) {
> +               error("calibrate didn't finish");
> +               ret = -ETIMEDOUT;
> +               goto failed;
> +       }
> +
> +       ret = 0;
> +
> +failed:
> +       clrbits_le32(eqos->regs + EQOS_SDMEMCOMPPADCTRL,
> +                    EQOS_SDMEMCOMPPADCTRL_PAD_E_INPUT_OR_E_PWRD);
> +
> +       debug("%s: returns %d\n", __func__, ret);
> +
> +       return ret;
> +}
> +
> +static int eqos_disable_calibration_tegra(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       clrbits_le32(eqos->regs + EQOS_AUTO_CAL_CONFIG,
> +                    EQOS_AUTO_CAL_CONFIG_ENABLE);
> +
> +       return 0;
> +}
> +
> +static ulong eqos_get_tick_clk_rate_tegra(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       return clk_get_rate(&eqos->clk_slave_bus);
> +}
> +
> +static int eqos_set_full_duplex(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       setbits_le32(eqos->regs + EQOS_MAC_CONFIGURATION,
> +                    EQOS_MAC_CONFIGURATION_DM);
> +
> +       return 0;
> +}
> +
> +static int eqos_set_half_duplex(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       clrbits_le32(eqos->regs + EQOS_MAC_CONFIGURATION,
> +                    EQOS_MAC_CONFIGURATION_DM);
> +
> +       /* WAR: Flush TX queue when switching to half-duplex */
> +       setbits_le32(eqos->regs + EQOS_MTL_TXQ0_OPERATION_MODE,
> +                    EQOS_MTL_TXQ0_OPERATION_MODE_FTQ);
> +
> +       return 0;
> +}
> +
> +static int eqos_set_gmii_speed(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       clrbits_le32(eqos->regs + EQOS_MAC_CONFIGURATION,
> +                    EQOS_MAC_CONFIGURATION_PS | EQOS_MAC_CONFIGURATION_FES);
> +
> +       return 0;
> +}
> +
> +static int eqos_set_mii_speed_100(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       setbits_le32(eqos->regs + EQOS_MAC_CONFIGURATION,
> +                    EQOS_MAC_CONFIGURATION_PS | EQOS_MAC_CONFIGURATION_FES);
> +
> +       return 0;
> +}
> +
> +static int eqos_set_mii_speed_10(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       clrsetbits_le32(eqos->regs + EQOS_MAC_CONFIGURATION,
> +                       EQOS_MAC_CONFIGURATION_FES, EQOS_MAC_CONFIGURATION_PS);
> +
> +       return 0;
> +}
> +
> +static int eqos_set_tx_clk_speed_tegra(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       ulong rate;
> +       int ret;
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       switch (eqos->phy->speed) {
> +       case SPEED_1000:
> +               rate = 125 * 1000 * 1000;
> +               break;
> +       case SPEED_100:
> +               rate = 25 * 1000 * 1000;
> +               break;
> +       case SPEED_10:
> +               rate = 2.5 * 1000 * 1000;
> +               break;
> +       default:
> +               error("invalid speed %d", eqos->phy->speed);
> +               return -EINVAL;
> +       }
> +
> +       ret = clk_set_rate(&eqos->clk_tx, rate);
> +       if (ret < 0) {
> +               error("clk_set_rate(tx_clk, %lu) failed: %d", rate, ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int eqos_adjust_link(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       int ret;
> +       bool en_calibration;
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       if (eqos->phy->duplex)
> +               ret = eqos_set_full_duplex(dev);
> +       else
> +               ret = eqos_set_half_duplex(dev);
> +       if (ret < 0) {
> +               error("eqos_set_*_duplex() failed: %d", ret);
> +               return ret;
> +       }
> +
> +       switch (eqos->phy->speed) {
> +       case SPEED_1000:
> +               en_calibration = true;
> +               ret = eqos_set_gmii_speed(dev);
> +               break;
> +       case SPEED_100:
> +               en_calibration = true;
> +               ret = eqos_set_mii_speed_100(dev);
> +               break;
> +       case SPEED_10:
> +               en_calibration = false;
> +               ret = eqos_set_mii_speed_10(dev);
> +               break;
> +       default:
> +               error("invalid speed %d", eqos->phy->speed);
> +               return -EINVAL;
> +       }
> +       if (ret < 0) {
> +               error("eqos_set_*mii_speed*() failed: %d", ret);
> +               return ret;
> +       }
> +
> +       if (en_calibration) {
> +               ret = eqos_calibrate_pads_tegra(dev);
> +               if (ret < 0) {
> +                       error("eqos_calibrate_pads_tegra() failed: %d", ret);
> +                       return ret;
> +               }
> +       } else {
> +               ret = eqos_disable_calibration_tegra(dev);
> +               if (ret < 0) {
> +                       error("eqos_disable_calibration_tegra() failed: %d",
> +                             ret);
> +                       return ret;
> +               }
> +       }
> +
> +       ret = eqos_set_tx_clk_speed_tegra(dev);
> +       if (ret < 0) {
> +               error("eqos_set_tx_clk_speed_tegra() failed: %d", ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int eqos_start(struct udevice *dev)
> +{
> +       struct eth_pdata *plat = dev_get_platdata(dev);
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       int ret, i;
> +       ulong rate;
> +       u32 val, tx_fifo_sz, rx_fifo_sz, tqs, rqs, pbl;
> +       ulong last_rx_desc;
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       eqos->tx_desc_idx = 0;
> +       eqos->rx_desc_idx = 0;
> +
> +       ret = eqos_start_clks_tegra(dev);
> +       if (ret < 0) {
> +               error("eqos_start_clks_tegra() failed: %d", ret);
> +               goto err;
> +       }
> +
> +       ret = eqos_start_resets_tegra(dev);
> +       if (ret < 0) {
> +               error("eqos_start_resets_tegra() failed: %d", ret);
> +               goto err_stop_clks;
> +       }
> +
> +       udelay(10);
> +
> +       for (i = 0; i < 1000000; i++) {
> +               if (i)
> +                       udelay(1);
> +               val = readl(eqos->regs + EQOS_DMA_MODE);
> +               if (!(val & EQOS_DMA_MODE_SWR))
> +                       break;
> +       }

Use wait_bit() here.

> +       if (val & EQOS_DMA_MODE_SWR) {
> +               error("EQOS_DMA_MODE_SWR stuck (0x%x)", val);
> +               ret = -ETIMEDOUT;
> +               goto err_stop_resets;
> +       }
> +
> +       ret = eqos_calibrate_pads_tegra(dev);
> +       if (ret < 0) {
> +               error("eqos_calibrate_pads_tegra() failed: %d", ret);
> +               goto err_stop_resets;
> +       }
> +
> +       rate = eqos_get_tick_clk_rate_tegra(dev);
> +       val = (rate / 1000000) - 1;
> +       writel(val, eqos->regs + EQOS_MAC_1US_TIC_COUNTER);
> +
> +       eqos->phy = phy_connect(eqos->mii, 0, dev, 0);
> +       if (!eqos->phy) {
> +               error("phy_connect() failed");
> +               goto err_stop_resets;
> +       }
> +       ret = phy_config(eqos->phy);
> +       if (ret < 0) {
> +               error("phy_config() failed: %d", ret);
> +               goto err_shutdown_phy;
> +       }
> +       ret = phy_startup(eqos->phy);
> +       if (ret < 0) {
> +               error("phy_startup() failed: %d", ret);
> +               goto err_shutdown_phy;
> +       }
> +
> +       if (!eqos->phy->link) {
> +               error("No link");
> +               goto err_shutdown_phy;
> +       }
> +
> +       ret = eqos_adjust_link(dev);
> +       if (ret < 0) {
> +               error("eqos_adjust_link() failed: %d", ret);
> +               goto err_shutdown_phy;
> +       }
> +
> +       /* Configure MTL */
> +
> +       /* Enable Store and Forward mode for TX */
> +       /* Program Tx operating mode */
> +       setbits_le32(eqos->regs + EQOS_MTL_TXQ0_OPERATION_MODE,
> +                    EQOS_MTL_TXQ0_OPERATION_MODE_TSF |
> +                    (EQOS_MTL_TXQ0_OPERATION_MODE_TXQEN_ENABLED <<
> +                     EQOS_MTL_TXQ0_OPERATION_MODE_TXQEN_SHIFT));
> +
> +       /* Transmit Queue weight */
> +       writel(0x10, eqos->regs + EQOS_MTL_TXQ0_QUANTUM_WEIGHT);
> +
> +       /* Enable Store and Forward mode for RX, since no jumbo frame */
> +       setbits_le32(eqos->regs + EQOS_MTL_RXQ0_OPERATION_MODE,
> +                    EQOS_MTL_RXQ0_OPERATION_MODE_RSF);
> +
> +       /* Transmit/Receive queue fifo size; use all RAM for 1 queue */
> +       val = readl(eqos->regs + EQOS_MAC_HW_FEATURE1);
> +       tx_fifo_sz = (val >> EQOS_MAC_HW_FEATURE1_TXFIFOSIZE_SHIFT) &
> +               EQOS_MAC_HW_FEATURE1_TXFIFOSIZE_MASK;
> +       rx_fifo_sz = (val >> EQOS_MAC_HW_FEATURE1_RXFIFOSIZE_SHIFT) &
> +               EQOS_MAC_HW_FEATURE1_RXFIFOSIZE_MASK;
> +
> +       /*
> +        * r/tx_fifo_sz is encoded as log2(n / 128). Undo that by shifting.
> +        * r/tqs is encoded as (n / 256) - 1.
> +        */
> +       tqs = (128 << tx_fifo_sz) / 256 - 1;
> +       rqs = (128 << rx_fifo_sz) / 256 - 1;
> +
> +       clrsetbits_le32(eqos->regs + EQOS_MTL_TXQ0_OPERATION_MODE,
> +                       EQOS_MTL_TXQ0_OPERATION_MODE_TQS_MASK <<
> +                       EQOS_MTL_TXQ0_OPERATION_MODE_TQS_SHIFT,
> +                       tqs << EQOS_MTL_TXQ0_OPERATION_MODE_TQS_SHIFT);
> +       clrsetbits_le32(eqos->regs + EQOS_MTL_RXQ0_OPERATION_MODE,
> +                       EQOS_MTL_RXQ0_OPERATION_MODE_RQS_MASK <<
> +                       EQOS_MTL_RXQ0_OPERATION_MODE_RQS_SHIFT,
> +                       rqs << EQOS_MTL_RXQ0_OPERATION_MODE_RQS_SHIFT);
> +
> +       /* Flow control used only if each channel gets 4KB or more FIFO */
> +       if (rqs >= ((4096 / 256) - 1)) {
> +               u32 rfd, rfa;
> +
> +               setbits_le32(eqos->regs + EQOS_MTL_RXQ0_OPERATION_MODE,
> +                            EQOS_MTL_RXQ0_OPERATION_MODE_EHFC);
> +
> +               /*
> +                * Set Threshold for Activating Flow Contol space for min 2
> +                * frames ie, (1500 * 1) = 1500 bytes.
> +                *
> +                * Set Threshold for Deactivating Flow Contol for space of
> +                * min 1 frame (frame size 1500bytes) in receive fifo
> +                */
> +               if (rqs == ((4096 / 256) - 1)) {
> +                       /*
> +                        * This violates the above formula because of FIFO size
> +                        * limit therefore overflow may occur inspite of this.
> +                        */
> +                       rfd = 0x3;      /* Full-3K */
> +                       rfa = 0x1;      /* Full-1.5K */
> +               } else if (rqs == ((8192 / 256) - 1)) {
> +                       rfd = 0x6;      /* Full-4K */
> +                       rfa = 0xa;      /* Full-6K */
> +               } else if (rqs == ((16384 / 256) - 1)) {
> +                       rfd = 0x6;      /* Full-4K */
> +                       rfa = 0x12;     /* Full-10K */
> +               } else {
> +                       rfd = 0x6;      /* Full-4K */
> +                       rfa = 0x1E;     /* Full-16K */
> +               }
> +
> +               clrsetbits_le32(eqos->regs + EQOS_MTL_RXQ0_OPERATION_MODE,
> +                               (EQOS_MTL_RXQ0_OPERATION_MODE_RFD_MASK <<
> +                                EQOS_MTL_RXQ0_OPERATION_MODE_RFD_SHIFT) |
> +                               (EQOS_MTL_RXQ0_OPERATION_MODE_RFA_MASK <<
> +                                EQOS_MTL_RXQ0_OPERATION_MODE_RFA_SHIFT),
> +                               (rfd <<
> +                                EQOS_MTL_RXQ0_OPERATION_MODE_RFD_SHIFT) |
> +                               (rfa <<
> +                                EQOS_MTL_RXQ0_OPERATION_MODE_RFA_SHIFT));
> +       }
> +
> +       /* Configure MAC */
> +
> +       clrsetbits_le32(eqos->regs + EQOS_MAC_RXQ_CTRL0,
> +                       EQOS_MAC_RXQ_CTRL0_RXQ0EN_MASK <<
> +                       EQOS_MAC_RXQ_CTRL0_RXQ0EN_SHIFT,
> +                       EQOS_MAC_RXQ_CTRL0_RXQ0EN_ENABLED_DCB <<
> +                       EQOS_MAC_RXQ_CTRL0_RXQ0EN_SHIFT);
> +
> +       /* Set TX flow control parameters */
> +       /* Set Pause Time */
> +       setbits_le32(eqos->regs + EQOS_MAC_Q0_TX_FLOW_CTRL,
> +                    0xffff << EQOS_MAC_Q0_TX_FLOW_CTRL_PT_SHIFT);
> +       /* Assign priority for TX flow control */
> +       clrbits_le32(eqos->regs + EQOS_MAC_TXQ_PRTY_MAP0,
> +                    EQOS_MAC_TXQ_PRTY_MAP0_PSTQ0_MASK <<
> +                    EQOS_MAC_TXQ_PRTY_MAP0_PSTQ0_SHIFT);
> +       /* Assign priority for RX flow control */
> +       clrbits_le32(eqos->regs + EQOS_MAC_RXQ_CTRL2,
> +                    EQOS_MAC_RXQ_CTRL2_PSRQ0_MASK <<
> +                    EQOS_MAC_RXQ_CTRL2_PSRQ0_SHIFT);
> +       /* Enable flow control */
> +       setbits_le32(eqos->regs + EQOS_MAC_Q0_TX_FLOW_CTRL,
> +                    EQOS_MAC_Q0_TX_FLOW_CTRL_TFE);
> +       setbits_le32(eqos->regs + EQOS_MAC_RX_FLOW_CTRL,
> +                    EQOS_MAC_RX_FLOW_CTRL_RFE);
> +
> +       clrsetbits_le32(eqos->regs + EQOS_MAC_CONFIGURATION,
> +                       EQOS_MAC_CONFIGURATION_GPSLCE |
> +                       EQOS_MAC_CONFIGURATION_WD |
> +                       EQOS_MAC_CONFIGURATION_JD |
> +                       EQOS_MAC_CONFIGURATION_JE,
> +                       EQOS_MAC_CONFIGURATION_CST |
> +                       EQOS_MAC_CONFIGURATION_ACS);
> +
> +       /* Update the MAC address */
> +       val = (plat->enetaddr[5] << 8) |
> +               (plat->enetaddr[4]);
> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
> +       val = (plat->enetaddr[3] << 24) |
> +               (plat->enetaddr[2] << 16) |
> +               (plat->enetaddr[1] << 8) |
> +               (plat->enetaddr[0]);
> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);

This should be implemented in write_hwaddr() op.

> +
> +       /* Configure DMA */
> +
> +       /* Enable OSP mode */
> +       setbits_le32(eqos->regs + EQOS_DMA_CH0_TX_CONTROL,
> +                    EQOS_DMA_CH0_TX_CONTROL_OSP);
> +
> +       /* RX buffer size. Must be a multiple of bus width */
> +       clrsetbits_le32(eqos->regs + EQOS_DMA_CH0_RX_CONTROL,
> +                       EQOS_DMA_CH0_RX_CONTROL_RBSZ_MASK <<
> +                       EQOS_DMA_CH0_RX_CONTROL_RBSZ_SHIFT,
> +                       EQOS_MAX_PACKET_SIZE <<
> +                       EQOS_DMA_CH0_RX_CONTROL_RBSZ_SHIFT);
> +
> +       setbits_le32(eqos->regs + EQOS_DMA_CH0_CONTROL,
> +                    EQOS_DMA_CH0_CONTROL_PBLX8);
> +
> +       /*
> +        * Burst length must be < 1/2 FIFO size.
> +        * FIFO size in tqs is encoded as (n / 256) - 1.
> +        * Each burst is n * 8 (PBLX8) * 16 (AXI width) == 128 bytes.
> +        * Half of n * 256 is n * 128, so pbl == tqs, modulo the -1.
> +        */
> +       pbl = tqs + 1;
> +       if (pbl > 32)
> +               pbl = 32;
> +       clrsetbits_le32(eqos->regs + EQOS_DMA_CH0_TX_CONTROL,
> +                       EQOS_DMA_CH0_TX_CONTROL_TXPBL_MASK <<
> +                       EQOS_DMA_CH0_TX_CONTROL_TXPBL_SHIFT,
> +                       pbl << EQOS_DMA_CH0_TX_CONTROL_TXPBL_SHIFT);
> +
> +       clrsetbits_le32(eqos->regs + EQOS_DMA_CH0_RX_CONTROL,
> +                       EQOS_DMA_CH0_RX_CONTROL_RXPBL_MASK <<
> +                       EQOS_DMA_CH0_RX_CONTROL_RXPBL_SHIFT,
> +                       8 << EQOS_DMA_CH0_RX_CONTROL_RXPBL_SHIFT);
> +
> +       /* DMA performance configuration */
> +       val = (2 << EQOS_DMA_SYSBUS_MODE_RD_OSR_LMT_SHIFT) |
> +               EQOS_DMA_SYSBUS_MODE_EAME | EQOS_DMA_SYSBUS_MODE_BLEN16 |
> +               EQOS_DMA_SYSBUS_MODE_BLEN8 | EQOS_DMA_SYSBUS_MODE_BLEN4;
> +       writel(val, eqos->regs + EQOS_DMA_SYSBUS_MODE);
> +
> +       /* Set up descriptors */
> +
> +       memset(eqos->descs, 0, EQOS_DESCRIPTORS_SIZE);
> +       for (i = 0; i < EQOS_DESCRIPTORS_RX; i++) {
> +               u32 *rx_desc = &(eqos->rx_descs[i * EQOS_DESCRIPTOR_WORDS]);
> +               rx_desc[0] = (u32)(ulong)(eqos->rx_dma_buf +
> +                                         (i * EQOS_MAX_PACKET_SIZE));
> +               rx_desc[3] |= EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
> +       }
> +       flush_cache((unsigned long)eqos->descs, EQOS_DESCRIPTORS_SIZE);
> +
> +       writel(0, eqos->regs + EQOS_DMA_CH0_TXDESC_LIST_HADDRESS);
> +       writel((ulong)eqos->tx_descs,
> +              eqos->regs + EQOS_DMA_CH0_TXDESC_LIST_ADDRESS);
> +       writel(EQOS_DESCRIPTORS_TX - 1,
> +              eqos->regs + EQOS_DMA_CH0_TXDESC_RING_LENGTH);
> +
> +       writel(0, eqos->regs + EQOS_DMA_CH0_RXDESC_LIST_HADDRESS);
> +       writel((ulong)eqos->rx_descs,
> +              eqos->regs + EQOS_DMA_CH0_RXDESC_LIST_ADDRESS);
> +       writel(EQOS_DESCRIPTORS_RX - 1,
> +              eqos->regs + EQOS_DMA_CH0_RXDESC_RING_LENGTH);
> +
> +       /* Enable everything */
> +
> +       setbits_le32(eqos->regs + EQOS_MAC_CONFIGURATION,
> +                    EQOS_MAC_CONFIGURATION_TE | EQOS_MAC_CONFIGURATION_RE);
> +
> +       setbits_le32(eqos->regs + EQOS_DMA_CH0_TX_CONTROL,
> +                    EQOS_DMA_CH0_TX_CONTROL_ST);
> +       setbits_le32(eqos->regs + EQOS_DMA_CH0_RX_CONTROL,
> +                    EQOS_DMA_CH0_RX_CONTROL_SR);
> +
> +       /* TX tail pointer not written until we need to TX a packet */
> +       /*
> +        * Point RX tail pointer at last descriptor. Ideally, we'd point at the
> +        * first descriptor, implying all descriptors were available. However,
> +        * that's not distinguishable from none of the descriptors being
> +        * available.
> +        */
> +       last_rx_desc = (ulong)&(eqos->rx_descs[(EQOS_DESCRIPTORS_RX - 1) *
> +                                              EQOS_DESCRIPTOR_WORDS]);
> +       writel(last_rx_desc, eqos->regs + EQOS_DMA_CH0_RXDESC_TAIL_POINTER);
> +
> +       eqos->started = true;
> +
> +       debug("%s: OK\n", __func__);
> +       return 0;
> +
> +err_shutdown_phy:
> +       phy_shutdown(eqos->phy);
> +       eqos->phy = NULL;
> +err_stop_resets:
> +       eqos_stop_resets_tegra(dev);
> +err_stop_clks:
> +       eqos_stop_clks_tegra(dev);
> +err:
> +       error("FAILED: %d", ret);
> +       return ret;
> +}
> +
> +void eqos_stop(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       int i;
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       if (!eqos->started)
> +               return;
> +       eqos->started = false;
> +
> +       /* Disable TX DMA */
> +       clrbits_le32(eqos->regs + EQOS_DMA_CH0_TX_CONTROL,
> +                    EQOS_DMA_CH0_TX_CONTROL_ST);
> +
> +       /* Wait for TX all packets to drain out of MTL */
> +       for (i = 0; i < 1000000; i++) {
> +               u32 val = readl(eqos->regs + EQOS_MTL_TXQ0_DEBUG);
> +               u32 trcsts = (val >> EQOS_MTL_TXQ0_DEBUG_TRCSTS_SHIFT) &
> +                       EQOS_MTL_TXQ0_DEBUG_TRCSTS_MASK;
> +               u32 txqsts = val & EQOS_MTL_TXQ0_DEBUG_TXQSTS;
> +               if ((trcsts != 1) && (!txqsts))
> +                       break;
> +       }
> +
> +       /* Turn off MAC TX and RX */
> +       clrbits_le32(eqos->regs + EQOS_MAC_CONFIGURATION,
> +                    EQOS_MAC_CONFIGURATION_TE | EQOS_MAC_CONFIGURATION_RE);
> +
> +       /* Wait for all RX packets to drain out of MTL */
> +       for (i = 0; i < 1000000; i++) {
> +               u32 val = readl(eqos->regs + EQOS_MTL_RXQ0_DEBUG);
> +               u32 prxq = (val >> EQOS_MTL_RXQ0_DEBUG_PRXQ_SHIFT) &
> +                       EQOS_MTL_RXQ0_DEBUG_PRXQ_MASK;
> +               u32 rxqsts = (val >> EQOS_MTL_RXQ0_DEBUG_RXQSTS_SHIFT) &
> +                       EQOS_MTL_RXQ0_DEBUG_RXQSTS_MASK;
> +               if ((!prxq) && (!rxqsts))
> +                       break;
> +       }
> +
> +       /* Turn off RX DMA */
> +       clrbits_le32(eqos->regs + EQOS_DMA_CH0_RX_CONTROL,
> +                    EQOS_DMA_CH0_RX_CONTROL_SR);
> +
> +       if (eqos->phy) {
> +               phy_shutdown(eqos->phy);
> +               eqos->phy = NULL;
> +       }
> +       eqos_stop_resets_tegra(dev);
> +       eqos_stop_clks_tegra(dev);
> +
> +       debug("%s: OK\n", __func__);
> +}
> +
> +int eqos_send(struct udevice *dev, void *packet, int length)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       u32 *tx_desc;

Please make this a struct.

> +       int i;
> +
> +       debug("%s(dev=%p, packet=%p, length=%d):\n", __func__, dev, packet,
> +             length);
> +
> +       memcpy(eqos->tx_dma_buf, packet, length);
> +       eqos_flush_buffer(eqos->tx_dma_buf, length);
> +
> +       tx_desc = &(eqos->tx_descs[eqos->tx_desc_idx * EQOS_DESCRIPTOR_WORDS]);
> +       eqos->tx_desc_idx++;
> +       eqos->tx_desc_idx %= EQOS_DESCRIPTORS_TX;
> +
> +       tx_desc[0] = (ulong)eqos->tx_dma_buf;
> +       tx_desc[1] = 0;
> +       tx_desc[2] = length;
> +       /*
> +        * Make sure that if HW sees the _OWN write below, it will see all the
> +        * writes to the rest of the descriptor too.
> +        */
> +       mb();
> +       tx_desc[3] = EQOS_DESC3_OWN | EQOS_DESC3_FD | EQOS_DESC3_LD | length;
> +       eqos_flush_desc(tx_desc);
> +
> +       writel((ulong)(tx_desc + 4),
> +              eqos->regs + EQOS_DMA_CH0_TXDESC_TAIL_POINTER);
> +
> +       for (i = 0; i < 1000000; i++) {
> +               eqos_inval_desc(tx_desc);
> +               if (!(readl(&tx_desc[3]) & EQOS_DESC3_OWN))
> +                       return 0;

Use wait_bit() here.

> +               udelay(1);
> +       }
> +
> +       debug("%s: TX timeout\n", __func__);
> +
> +       return -ETIMEDOUT;
> +}
> +
> +int eqos_recv(struct udevice *dev, int flags, uchar **packetp)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       u32 *rx_desc;

Please make this a struct.

> +       int length;
> +
> +       debug("%s(dev=%p, flags=%x):\n", __func__, dev, flags);
> +
> +       rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx * EQOS_DESCRIPTOR_WORDS]);
> +       if (rx_desc[3] & EQOS_DESC3_OWN) {
> +               debug("%s: RX packet not available\n", __func__);
> +               return -EAGAIN;
> +       }
> +
> +       *packetp = eqos->rx_dma_buf +
> +               (eqos->rx_desc_idx * EQOS_MAX_PACKET_SIZE);
> +       length = rx_desc[3] & 0x7fff;
> +       debug("%s: *packetp=%p, length=%d\n", __func__, *packetp, length);
> +
> +       eqos_inval_buffer(*packetp, length);
> +
> +       return length;
> +}
> +
> +int eqos_free_pkt(struct udevice *dev, uchar *packet, int length)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       uchar *packet_expected;
> +       u32 *rx_desc;

Please make this a struct.

> +
> +       debug("%s(packet=%p, length=%d)\n", __func__, packet, length);
> +
> +       packet_expected = eqos->rx_dma_buf +
> +               (eqos->rx_desc_idx * EQOS_MAX_PACKET_SIZE);
> +       if (packet != packet_expected) {
> +               debug("%s: Unexpected packet (expected %p)\n", __func__,
> +                     packet_expected);
> +               return -EINVAL;
> +       }
> +
> +       rx_desc = &(eqos->rx_descs[eqos->rx_desc_idx * EQOS_DESCRIPTOR_WORDS]);
> +       rx_desc[0] = (u32)(ulong)packet;
> +       rx_desc[1] = 0;
> +       rx_desc[2] = 0;
> +       /*
> +        * Make sure that if HW sees the _OWN write below, it will see all the
> +        * writes to the rest of the descriptor too.
> +        */
> +       mb();
> +       rx_desc[3] |= EQOS_DESC3_OWN | EQOS_DESC3_BUF1V;
> +       eqos_flush_desc(rx_desc);
> +
> +       writel((ulong)rx_desc, eqos->regs + EQOS_DMA_CH0_RXDESC_TAIL_POINTER);
> +
> +       eqos->rx_desc_idx++;
> +       eqos->rx_desc_idx %= EQOS_DESCRIPTORS_RX;
> +
> +       return 0;
> +}
> +
> +static int eqos_probe_resources_core(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       int ret;
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       eqos->descs = eqos_alloc_descs(EQOS_DESCRIPTORS_TX +
> +                                      EQOS_DESCRIPTORS_RX);
> +       if (!eqos->descs) {
> +               debug("%s: eqos_alloc_descs() failed\n", __func__);
> +               ret = -ENOMEM;
> +               goto err;
> +       }
> +       eqos->tx_descs = eqos->descs;
> +       eqos->rx_descs = eqos->tx_descs +
> +               (EQOS_DESCRIPTORS_TX * EQOS_DESCRIPTOR_WORDS);
> +       debug("%s: tx_descs=%p, rx_descs=%p\n", __func__, eqos->tx_descs,
> +             eqos->rx_descs);
> +
> +       eqos->tx_dma_buf = memalign(EQOS_BUFFER_ALIGN, EQOS_MAX_PACKET_SIZE);
> +       if (!eqos->tx_dma_buf) {
> +               debug("%s: memalign(tx_dma_buf) failed\n", __func__);
> +               ret = -ENOMEM;
> +               goto err_free_descs;
> +       }
> +       debug("%s: rx_dma_buf=%p\n", __func__, eqos->rx_dma_buf);
> +
> +       eqos->rx_dma_buf = memalign(EQOS_BUFFER_ALIGN, EQOS_RX_BUFFER_SIZE);
> +       if (!eqos->rx_dma_buf) {
> +               debug("%s: memalign(rx_dma_buf) failed\n", __func__);
> +               ret = -ENOMEM;
> +               goto err_free_tx_dma_buf;
> +       }
> +       debug("%s: tx_dma_buf=%p\n", __func__, eqos->tx_dma_buf);
> +
> +       eqos->rx_pkt = malloc(EQOS_MAX_PACKET_SIZE);
> +       if (!eqos->rx_pkt) {
> +               debug("%s: malloc(rx_pkt) failed\n", __func__);
> +               ret = -ENOMEM;
> +               goto err_free_rx_dma_buf;
> +       }
> +       debug("%s: rx_pkt=%p\n", __func__, eqos->rx_pkt);
> +
> +       debug("%s: OK\n", __func__);
> +       return 0;
> +
> +err_free_rx_dma_buf:
> +       free(eqos->rx_dma_buf);
> +err_free_tx_dma_buf:
> +       free(eqos->tx_dma_buf);
> +err_free_descs:
> +       eqos_free_descs(eqos->descs);
> +err:
> +
> +       debug("%s: returns %d\n", __func__, ret);
> +       return ret;
> +}
> +
> +static int eqos_remove_resources_core(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       free(eqos->rx_pkt);
> +       free(eqos->rx_dma_buf);
> +       free(eqos->tx_dma_buf);
> +       eqos_free_descs(eqos->descs);
> +
> +       debug("%s: OK\n", __func__);
> +       return 0;
> +}
> +
> +static int eqos_probe_resources_tegra(struct udevice *dev)

Sure would be great to not have this in here.

> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       int ret;
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       ret = reset_get_by_name(dev, "eqos", &eqos->reset_ctl);
> +       if (ret) {
> +               error("reset_get_by_name(rst) failed: %d", ret);
> +               return ret;
> +       }
> +
> +       ret = gpio_request_by_name(dev, "phy-reset-gpios", 0,
> +                                  &eqos->phy_reset_gpio,
> +                                  GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE);
> +       if (ret) {
> +               error("gpio_request_by_name(phy reset) failed: %d", ret);
> +               goto err_free_reset_eqos;
> +       }
> +
> +       ret = clk_get_by_name(dev, "slave_bus", &eqos->clk_slave_bus);
> +       if (ret) {
> +               error("clk_get_by_name(slave_bus) failed: %d", ret);
> +               goto err_free_gpio_phy_reset;
> +       }
> +
> +       ret = clk_get_by_name(dev, "master_bus", &eqos->clk_master_bus);
> +       if (ret) {
> +               error("clk_get_by_name(master_bus) failed: %d", ret);
> +               goto err_free_clk_slave_bus;
> +       }
> +
> +       ret = clk_get_by_name(dev, "rx", &eqos->clk_rx);
> +       if (ret) {
> +               error("clk_get_by_name(rx) failed: %d", ret);
> +               goto err_free_clk_master_bus;
> +       }
> +
> +       ret = clk_get_by_name(dev, "ptp_ref", &eqos->clk_ptp_ref);
> +       if (ret) {
> +               error("clk_get_by_name(ptp_ref) failed: %d", ret);
> +               goto err_free_clk_rx;
> +               return ret;
> +       }
> +
> +       ret = clk_get_by_name(dev, "tx", &eqos->clk_tx);
> +       if (ret) {
> +               error("clk_get_by_name(tx) failed: %d", ret);
> +               goto err_free_clk_ptp_ref;
> +       }
> +
> +       debug("%s: OK\n", __func__);
> +       return 0;
> +
> +err_free_clk_ptp_ref:
> +       clk_free(&eqos->clk_ptp_ref);
> +err_free_clk_rx:
> +       clk_free(&eqos->clk_rx);
> +err_free_clk_master_bus:
> +       clk_free(&eqos->clk_master_bus);
> +err_free_clk_slave_bus:
> +       clk_free(&eqos->clk_slave_bus);
> +err_free_gpio_phy_reset:
> +       dm_gpio_free(dev, &eqos->phy_reset_gpio);
> +err_free_reset_eqos:
> +       reset_free(&eqos->reset_ctl);
> +
> +       debug("%s: returns %d\n", __func__, ret);
> +       return ret;
> +}
> +
> +static int eqos_remove_resources_tegra(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       clk_free(&eqos->clk_tx);
> +       clk_free(&eqos->clk_ptp_ref);
> +       clk_free(&eqos->clk_rx);
> +       clk_free(&eqos->clk_slave_bus);
> +       clk_free(&eqos->clk_master_bus);
> +       dm_gpio_free(dev, &eqos->phy_reset_gpio);
> +       reset_free(&eqos->reset_ctl);
> +
> +       debug("%s: OK\n", __func__);
> +       return 0;
> +}
> +
> +static int eqos_probe(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +       int ret;
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       eqos->dev = dev;
> +
> +       eqos->regs = dev_get_addr(dev);
> +       if (eqos->regs == FDT_ADDR_T_NONE) {
> +               error("dev_get_addr() failed");
> +               return -ENODEV;
> +       }
> +
> +       ret = eqos_probe_resources_core(dev);
> +       if (ret < 0) {
> +               error("eqos_probe_resources_core() failed: %d", ret);
> +               return ret;
> +       }
> +
> +       ret = eqos_probe_resources_tegra(dev);
> +       if (ret < 0) {
> +               error("eqos_probe_resources_tegra() failed: %d", ret);
> +               goto err_remove_resources_core;
> +       }
> +
> +       eqos->mii = mdio_alloc();
> +       if (!eqos->mii) {
> +               error("mdio_alloc() failed");
> +               goto err_remove_resources_tegra;
> +       }
> +       eqos->mii->read = eqos_mdio_read;
> +       eqos->mii->write = eqos_mdio_write;
> +       eqos->mii->priv = eqos;
> +       strcpy(eqos->mii->name, dev->name);
> +
> +       ret = mdio_register(eqos->mii);
> +       if (ret < 0) {
> +               error("mdio_register() failed: %d", ret);
> +               goto err_free_mdio;
> +       }
> +
> +       debug("%s: OK\n", __func__);
> +       return 0;
> +
> +err_free_mdio:
> +       mdio_free(eqos->mii);
> +err_remove_resources_tegra:
> +       eqos_remove_resources_tegra(dev);
> +err_remove_resources_core:
> +       eqos_remove_resources_core(dev);
> +
> +       debug("%s: returns %d\n", __func__, ret);
> +       return ret;
> +}
> +
> +static int eqos_remove(struct udevice *dev)
> +{
> +       struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +       debug("%s(dev=%p):\n", __func__, dev);
> +
> +       mdio_unregister(eqos->mii);
> +       mdio_free(eqos->mii);
> +       eqos_remove_resources_tegra(dev);
> +       eqos_probe_resources_core(dev);
> +
> +       debug("%s: OK\n", __func__);
> +       return 0;
> +}
> +
> +static const struct eth_ops eqos_ops = {
> +       .start = eqos_start,
> +       .stop = eqos_stop,
> +       .send = eqos_send,
> +       .recv = eqos_recv,
> +       .free_pkt = eqos_free_pkt,
> +};
> +
> +static const struct udevice_id eqos_ids[] = {
> +       { .compatible = "nvidia,tegra186-eqos" },
> +       { }
> +};
> +
> +U_BOOT_DRIVER(eth_eqos) = {
> +       .name = "eth_eqos",
> +       .id = UCLASS_ETH,
> +       .of_match = eqos_ids,
> +       .probe = eqos_probe,
> +       .remove = eqos_remove,
> +       .ops = &eqos_ops,
> +       .priv_auto_alloc_size = sizeof(struct eqos_priv),
> +       .platdata_auto_alloc_size = sizeof(struct eth_pdata),
> +};
> --
> 2.9.3
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

On 09/23/2016 03:49 PM, Joe Hershberger wrote:
> Hi Stephen,
>
> Thanks for sending this! I have some comments below.
>
> Cheers,
> -Joe
>
> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> This driver supports the Synopsys Designware Ethernet QoS (Quality of
>> Service) a/k/a eqos IP block, which is a different design than the HW
>> supported by the existing designware.c driver. The IP supports many
>> options for bus type, clocking/reset structure, and feature list. This
>> driver currently supports the specific configuration used in NVIDIA's
>> Tegra186 chip, but should be extensible to other combinations quite
>> easily, as explained in the source.

>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c

>> +/* Core registers */
>
> Why are registers not defined as a struct? That is the common way that
> is accepted to represent registers in a driver.

It is common, but I find using structs has significant disadvantages, 
which outweigh any advantages, so I strongly prefer not to use them.

Disadvantages of structs:

* Drivers often have to deal with different but similar HW variants. 
Similar enough that it makes sense to use the same driver for them, but 
different enough that certain registers are placed differently in the 
memory map. With structs, there's little choice but to use ifdefs to 
pick a particular HW variant and bake that into the driver. This doesn't 
work well for drivers that must pick the variant at run-time rather than 
compile-time, e.g. drivers for USB or PCIe devices, or when you wish to 
build a single SW image that can run on multiple SoCs.

A common variant on this theme is device with different register layouts 
due to IP integration issues, such as a UART with byte-wide registers 
that appear at consecutive byte offsets in some SoCs, but at separate 
word addresses in other SoCs.

This issue is relevant here since the EQoS block is a generic IP block 
with many options that may have different SoC- or IP-version-specific 
differences between SoCs.

* The register space for the EQoS driver is quite sparse, and U-Boot 
uses a subset of the registers effectively making it even more sparse. 
Defining a struct to describe this will be a frustrating exercise in 
calculating the right amount of padding to place into the struct to get 
the correct offsets. Mistakes will be made and it will be annoying.

* Structs don't translate well to interactive debugging. It's common to 
read/write registers using md/mw U-Boot shell commands. With structs, 
you need to either (a) use some other data source for the register 
offsets, (b) manually add up field sizes (c) write a comment indicating 
the offset next to each field. (a) and (b) are both annoying, and by the 
time you've done (c) (which is quite complicated in the face of ifdefs) 
you may as well have simply used #defines in the first place.

>> +/*
>> + * Warn if the cache-line size is larger than the descriptor size. In such
>> + * cases the driver will likely fail because the CPU needs to flush the cache
>> + * when requeuing RX buffers, therefore descriptors written by the hardware
>> + * may be discarded.
>> + *
>> + * This can be fixed by defining CONFIG_SYS_NONCACHED_MEMORY which will cause
>> + * the driver to allocate descriptors from a pool of non-cached memory.
>> + */
>> +#if EQOS_DESCRIPTOR_SIZE < ARCH_DMA_MINALIGN
>> +#if !defined(CONFIG_SYS_NONCACHED_MEMORY) && \
>> +       !defined(CONFIG_SYS_DCACHE_OFF) && !defined(CONFIG_X86)
>
> Please include an explanation for why x86 is special.

Sure. Do note though that this exact text already exists in other U-Boot 
drivers.

>> +static int eqos_start_clks_tegra(struct udevice *dev)
>
> Why is this not in a board file and a separate patch?

The clocks (and other resources) that this driver operates on are 
directly related to the EQoS IP block itself. Hence, it's completely 
appropriate for this driver to deal with them.

Perhaps the name "tegra" is slightly misleading; it refers to "the 
configuration of the core EQoS block that Tegra happens to have chosen" 
rather than "something entirely Tegra-specific that is unrelated to this 
device/driver". However, I'm not sure what name would be better than 
"tegra" here; the correct name is something like 
axi_ahb_dma_mtl_core_single_phy_rgmii, but that's far too long for a 
symbol name. I attempted to describe this in the comment regarding IP 
configuration at the beginning of the file. Is there a specific 
shortcoming in that comment that could be changed to address this better?

>> +static int eqos_calibrate_pads_tegra(struct udevice *dev)

This function is the only part that's truly Tegra-specific, rather than 
specific to the standard IP configuration Tegra happens to use. However, 
I still believe it's entirely appropriate for this driver to include the 
logic to control these registers; they are part of the Ethernet IP block 
itself, even if NVIDIA added them as customer registers. The registers 
can't be controlled from any board/... file without very tight 
co-ordination with the driver; they aren't something that a board/... 
file could set up once at boot time in readiness for this driver to run 
later. The only way we could separate out this function from the driver 
would be to provide hook/callback functions from this driver to board 
logic. I don't believe that would make the code any cleaner. For one 
thing, it would spread related code across multiple locations making it 
hard to find. Another issue would be determining when to call the 
hook/callback functions for a specific device; they may only be 
applicable to a single instance of the device in a multi-device system. 
It's entirely possible someone could place an EQoS IP block into a PCI 
device, and then you'd potentially need a separate hook implementation 
for the in-SoC and the on-PCI instance. The driver can work that out 
reasonably easily itself based on how it's instantiated, but finding and 
distinguishing the different external implementations of the hook 
functions would be challenging.

>> +static int eqos_start(struct udevice *dev)

>> +       /* Update the MAC address */
>> +       val = (plat->enetaddr[5] << 8) |
>> +               (plat->enetaddr[4]);
>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>> +       val = (plat->enetaddr[3] << 24) |
>> +               (plat->enetaddr[2] << 16) |
>> +               (plat->enetaddr[1] << 8) |
>> +               (plat->enetaddr[0]);
>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>
> This should be implemented in write_hwaddr() op.

Does the network core code guarantee that start() is called before 
write_hwaddr() (so that clocks are active) and write_hwaddr() is called 
before any packets are sent/received? If so, I can move this.

I note that there are other existing network drivers that set the MAC 
address in start(); see rtl8169.c for example.

>> +int eqos_send(struct udevice *dev, void *packet, int length)
>> +{
>> +       struct eqos_priv *eqos = dev_get_priv(dev);
>> +       u32 *tx_desc;
>
> Please make this a struct.

IIRC I'd avoided that because there are a variety of different formats 
based on the type, which will involve a mess of unions. I'll take a look 
at the details and convert them if it doesn't turn out massively 
horrible; they're small enough that it sounds reasonable to do so on the 
surface.

On 09/23/2016 03:49 PM, Joe Hershberger wrote:
> Hi Stephen,
>
> Thanks for sending this! I have some comments below.
>
> Cheers,
> -Joe
>
> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> This driver supports the Synopsys Designware Ethernet QoS (Quality of
>> Service) a/k/a eqos IP block, which is a different design than the HW
>> supported by the existing designware.c driver. The IP supports many
>> options for bus type, clocking/reset structure, and feature list. This
>> driver currently supports the specific configuration used in NVIDIA's
>> Tegra186 chip, but should be extensible to other combinations quite
>> easily, as explained in the source.

>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c

> +static int eqos_start(struct udevice *dev)

>> +       /* Update the MAC address */
>> +       val = (plat->enetaddr[5] << 8) |
>> +               (plat->enetaddr[4]);
>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>> +       val = (plat->enetaddr[3] << 24) |
>> +               (plat->enetaddr[2] << 16) |
>> +               (plat->enetaddr[1] << 8) |
>> +               (plat->enetaddr[0]);
>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>
> This should be implemented in write_hwaddr() op.

That op is never called because this driver is only instantiated by 
device tree. Since this code can't be skipped, it can't be moved to that op.

>> +int eqos_send(struct udevice *dev, void *packet, int length)

>> +       for (i = 0; i < 1000000; i++) {
>> +               eqos_inval_desc(tx_desc);
>> +               if (!(readl(&tx_desc[3]) & EQOS_DESC3_OWN))
>> +                       return 0;
>
> Use wait_bit() here.

That function won't work here due to the need to call 
eqos_inval_desc(tx_desc) inside the loop.

On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>
>> Hi Stephen,
>>
>> Thanks for sending this! I have some comments below.
>>
>> Cheers,
>> -Joe
>>
>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren <swarren@wwwdotorg.org>
>> wrote:
>>>
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> This driver supports the Synopsys Designware Ethernet QoS (Quality of
>>> Service) a/k/a eqos IP block, which is a different design than the HW
>>> supported by the existing designware.c driver. The IP supports many
>>> options for bus type, clocking/reset structure, and feature list. This
>>> driver currently supports the specific configuration used in NVIDIA's
>>> Tegra186 chip, but should be extensible to other combinations quite
>>> easily, as explained in the source.
>
>
>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>
>
>> +static int eqos_start(struct udevice *dev)
>
>
>>> +       /* Update the MAC address */
>>> +       val = (plat->enetaddr[5] << 8) |
>>> +               (plat->enetaddr[4]);
>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>> +       val = (plat->enetaddr[3] << 24) |
>>> +               (plat->enetaddr[2] << 16) |
>>> +               (plat->enetaddr[1] << 8) |
>>> +               (plat->enetaddr[0]);
>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>
>>
>> This should be implemented in write_hwaddr() op.
>
>
> That op is never called because this driver is only instantiated by device
> tree. Since this code can't be skipped, it can't be moved to that op.

I don't understand what you're saying here. That op is called in
eth_initialize() on every device found in device tree.

>>> +int eqos_send(struct udevice *dev, void *packet, int length)
>
>
>>> +       for (i = 0; i < 1000000; i++) {
>>> +               eqos_inval_desc(tx_desc);
>>> +               if (!(readl(&tx_desc[3]) & EQOS_DESC3_OWN))
>>> +                       return 0;
>>
>>
>> Use wait_bit() here.
>
>
> That function won't work here due to the need to call
> eqos_inval_desc(tx_desc) inside the loop.

Hi Stephen,

On Tue, Sep 27, 2016 at 12:02 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>
>> Hi Stephen,
>>
>> Thanks for sending this! I have some comments below.
>>
>> Cheers,
>> -Joe
>>
>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren <swarren@wwwdotorg.org>
>> wrote:
>>>
>>> From: Stephen Warren <swarren@nvidia.com>
>>>
>>> This driver supports the Synopsys Designware Ethernet QoS (Quality of
>>> Service) a/k/a eqos IP block, which is a different design than the HW
>>> supported by the existing designware.c driver. The IP supports many
>>> options for bus type, clocking/reset structure, and feature list. This
>>> driver currently supports the specific configuration used in NVIDIA's
>>> Tegra186 chip, but should be extensible to other combinations quite
>>> easily, as explained in the source.
>
>
>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>
>
>>> +/* Core registers */
>>
>>
>> Why are registers not defined as a struct? That is the common way that
>> is accepted to represent registers in a driver.
>
>
> It is common, but I find using structs has significant disadvantages, which
> outweigh any advantages, so I strongly prefer not to use them.
>
> Disadvantages of structs:
>
> * Drivers often have to deal with different but similar HW variants. Similar
> enough that it makes sense to use the same driver for them, but different
> enough that certain registers are placed differently in the memory map. With
> structs, there's little choice but to use ifdefs to pick a particular HW
> variant and bake that into the driver. This doesn't work well for drivers
> that must pick the variant at run-time rather than compile-time, e.g.
> drivers for USB or PCIe devices, or when you wish to build a single SW image
> that can run on multiple SoCs.

Is that really the case here or are you just making a broad statement?
Are registers really moving based on IP configuration? That sounds
like broken IP / IP generator.

> A common variant on this theme is device with different register layouts due
> to IP integration issues, such as a UART with byte-wide registers that
> appear at consecutive byte offsets in some SoCs, but at separate word
> addresses in other SoCs.

Again, this sounds like a generic argument that doesn't apply here.

> This issue is relevant here since the EQoS block is a generic IP block with
> many options that may have different SoC- or IP-version-specific differences
> between SoCs.

But simply additive, no? Included features add registers or bitfields.

> * The register space for the EQoS driver is quite sparse, and U-Boot uses a
> subset of the registers effectively making it even more sparse. Defining a
> struct to describe this will be a frustrating exercise in calculating the
> right amount of padding to place into the struct to get the correct offsets.
> Mistakes will be made and it will be annoying.

It's also organized into a few blocks. It's preferable to group
registers into a struct that represents each block instead of one huge
struct. It also hurts nothing to have registers defined in the struct
that apply to configurations that may not be enabled in a given
instance... the switching can be done at the spot where the register
is accessed. And the accesses don't have to be compile-time options so
you can have your built-in and your PCIe variants.

> * Structs don't translate well to interactive debugging. It's common to
> read/write registers using md/mw U-Boot shell commands. With structs, you
> need to either (a) use some other data source for the register offsets, (b)
> manually add up field sizes (c) write a comment indicating the offset next
> to each field. (a) and (b) are both annoying, and by the time you've done
> (c) (which is quite complicated in the face of ifdefs) you may as well have
> simply used #defines in the first place.

(a) is not so bad, but not ideal.  I agree that (b) is annoying. I
don't agree that you have to ifdef it, see above. I think (c) is a bit
nicer than (a), but either are fine. I think the resulting code that
is accessing the registers looks more concise and readable than a
defined constant that is going to need to contain all of the context /
scope in its name for disambiguation in the global namespace. A
register name in a struct is scoped to that block and a local variable
can be much shorter such that you don't have to keep repeating the
same text throughout the function.

>>> +/*
>>> + * Warn if the cache-line size is larger than the descriptor size. In
>>> such
>>> + * cases the driver will likely fail because the CPU needs to flush the
>>> cache
>>> + * when requeuing RX buffers, therefore descriptors written by the
>>> hardware
>>> + * may be discarded.
>>> + *
>>> + * This can be fixed by defining CONFIG_SYS_NONCACHED_MEMORY which will
>>> cause
>>> + * the driver to allocate descriptors from a pool of non-cached memory.
>>> + */
>>> +#if EQOS_DESCRIPTOR_SIZE < ARCH_DMA_MINALIGN
>>> +#if !defined(CONFIG_SYS_NONCACHED_MEMORY) && \
>>> +       !defined(CONFIG_SYS_DCACHE_OFF) && !defined(CONFIG_X86)
>>
>>
>> Please include an explanation for why x86 is special.
>
>
> Sure. Do note though that this exact text already exists in other U-Boot
> drivers.

That's unfortunate. At least someone can find the explanation here
after you add it.

>>> +static int eqos_start_clks_tegra(struct udevice *dev)
>>
>>
>> Why is this not in a board file and a separate patch?
>
>
> The clocks (and other resources) that this driver operates on are directly
> related to the EQoS IP block itself. Hence, it's completely appropriate for
> this driver to deal with them.
>
> Perhaps the name "tegra" is slightly misleading; it refers to "the
> configuration of the core EQoS block that Tegra happens to have chosen"
> rather than "something entirely Tegra-specific that is unrelated to this
> device/driver". However, I'm not sure what name would be better than "tegra"
> here; the correct name is something like
> axi_ahb_dma_mtl_core_single_phy_rgmii, but that's far too long for a symbol
> name. I attempted to describe this in the comment regarding IP configuration
> at the beginning of the file. Is there a specific shortcoming in that
> comment that could be changed to address this better?

I think the comment should list the configuration with at least the
level of detail that you used to create that long symbol name.

Do you anticipate that this set of configurations will with high
confidence be used in Tegra going forward? Or would it be better to
call it tegra186?

>>> +static int eqos_calibrate_pads_tegra(struct udevice *dev)
>
>
> This function is the only part that's truly Tegra-specific, rather than
> specific to the standard IP configuration Tegra happens to use. However, I
> still believe it's entirely appropriate for this driver to include the logic
> to control these registers; they are part of the Ethernet IP block itself,
> even if NVIDIA added them as customer registers. The registers can't be
> controlled from any board/... file without very tight co-ordination with the
> driver; they aren't something that a board/... file could set up once at
> boot time in readiness for this driver to run later. The only way we could
> separate out this function from the driver would be to provide hook/callback
> functions from this driver to board logic. I don't believe that would make
> the code any cleaner. For one thing, it would spread related code across
> multiple locations making it hard to find. Another issue would be
> determining when to call the hook/callback functions for a specific device;
> they may only be applicable to a single instance of the device in a
> multi-device system. It's entirely possible someone could place an EQoS IP
> block into a PCI device, and then you'd potentially need a separate hook
> implementation for the in-SoC and the on-PCI instance. The driver can work
> that out reasonably easily itself based on how it's instantiated, but
> finding and distinguishing the different external implementations of the
> hook functions would be challenging.

OK, I'm sold. The registers are part of this IP block, then the code
should be in this driver. Thanks.

>>> +static int eqos_start(struct udevice *dev)
>
>
>>> +       /* Update the MAC address */
>>> +       val = (plat->enetaddr[5] << 8) |
>>> +               (plat->enetaddr[4]);
>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>> +       val = (plat->enetaddr[3] << 24) |
>>> +               (plat->enetaddr[2] << 16) |
>>> +               (plat->enetaddr[1] << 8) |
>>> +               (plat->enetaddr[0]);
>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>
>>
>> This should be implemented in write_hwaddr() op.
>
>
> Does the network core code guarantee that start() is called before
> write_hwaddr() (so that clocks are active) and write_hwaddr() is called
> before any packets are sent/received? If so, I can move this.

start() is not called first, it's called after. But it can also be
called other times later, so it makes sense to register it even if you
call it again in start().

Maybe I need to add a flag that defers this until after start.
However, for most boards, it is undesirable to require start. From
net/eth-uclass.c:

        /*
         * Devices need to write the hwaddr even if not started so that Linux
         * will have access to the hwaddr that u-boot stored for the device.
         * This is accomplished by attempting to probe each device and calling
         * their write_hwaddr() operation.
         */

It sounds like you have a peculiar situation where you aren't starting
your peripheral clocks in SPL or some early board init, so you can't
even access registers around probe time. Is that really required?

> I note that there are other existing network drivers that set the MAC
> address in start(); see rtl8169.c for example.

I believe that the reset that is done in the rtl8169 driver clears the
MAC address, so it needs to be set on each start, but that is not that
common. The real question is why those drivers need to reset the
hardware for every network operation instead of just in probe or so.
Realistically, the network stack needs to be rearchitected to allow
multiple active MACs simultaneously. Along with that would be the
elimination of the nuclear option of resetting the IP before every
operation.

Thanks,
-Joe

On 10/11/2016 04:48 PM, Joe Hershberger wrote:
> On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>>
>>> Hi Stephen,
>>>
>>> Thanks for sending this! I have some comments below.
>>>
>>> Cheers,
>>> -Joe
>>>
>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren <swarren@wwwdotorg.org>
>>> wrote:
>>>>
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> This driver supports the Synopsys Designware Ethernet QoS (Quality of
>>>> Service) a/k/a eqos IP block, which is a different design than the HW
>>>> supported by the existing designware.c driver. The IP supports many
>>>> options for bus type, clocking/reset structure, and feature list. This
>>>> driver currently supports the specific configuration used in NVIDIA's
>>>> Tegra186 chip, but should be extensible to other combinations quite
>>>> easily, as explained in the source.
>>
>>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>>
>>> +static int eqos_start(struct udevice *dev)
>>
>>>> +       /* Update the MAC address */
>>>> +       val = (plat->enetaddr[5] << 8) |
>>>> +               (plat->enetaddr[4]);
>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>>> +       val = (plat->enetaddr[3] << 24) |
>>>> +               (plat->enetaddr[2] << 16) |
>>>> +               (plat->enetaddr[1] << 8) |
>>>> +               (plat->enetaddr[0]);
>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>>
>>>
>>> This should be implemented in write_hwaddr() op.
>>
>> That op is never called because this driver is only instantiated by device
>> tree. Since this code can't be skipped, it can't be moved to that op.
>
> I don't understand what you're saying here. That op is called in
> eth_initialize() on every device found in device tree.

Oh, so it is. I must have screwed up my tracing before.

Anyway, I still don't believe using write_hwaddr() is correct for this 
HW. It's marked optional in include/net.h; it would be implemented in 
cases where the MAC address should be passed to subsequent SW in 
Ethernet controller registers. That's not the case here. The master 
location for the MAC address is in an unrelated EEPROM that all drivers 
must read. Resetting the device (as any driver would do to guarantee 
correct operation no matter what SW ran before), or never initializing 
it in the first place (as is the case now without any driver in U-Boot) 
means that those registers cannot be assumed to contain valid data. Thus 
SW after U-Boot won't rely on the MAC address register values, and hence 
there's no need to implement .write_hwaddr() separately, since that's 
the only reason it exists separately according to the comments in 
include/net.h.

On Fri, Oct 14, 2016 at 1:35 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/11/2016 04:48 PM, Joe Hershberger wrote:
>>
>> On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren <swarren@wwwdotorg.org>
>> wrote:
>>>
>>> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>>>
>>>>
>>>> Hi Stephen,
>>>>
>>>> Thanks for sending this! I have some comments below.
>>>>
>>>> Cheers,
>>>> -Joe
>>>>
>>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren <swarren@wwwdotorg.org>
>>>> wrote:
>>>>>
>>>>>
>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>
>>>>> This driver supports the Synopsys Designware Ethernet QoS (Quality of
>>>>> Service) a/k/a eqos IP block, which is a different design than the HW
>>>>> supported by the existing designware.c driver. The IP supports many
>>>>> options for bus type, clocking/reset structure, and feature list. This
>>>>> driver currently supports the specific configuration used in NVIDIA's
>>>>> Tegra186 chip, but should be extensible to other combinations quite
>>>>> easily, as explained in the source.
>>>
>>>
>>>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>>>
>>>
>>>> +static int eqos_start(struct udevice *dev)
>>>
>>>
>>>>> +       /* Update the MAC address */
>>>>> +       val = (plat->enetaddr[5] << 8) |
>>>>> +               (plat->enetaddr[4]);
>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>>>> +       val = (plat->enetaddr[3] << 24) |
>>>>> +               (plat->enetaddr[2] << 16) |
>>>>> +               (plat->enetaddr[1] << 8) |
>>>>> +               (plat->enetaddr[0]);
>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>>>
>>>>
>>>>
>>>> This should be implemented in write_hwaddr() op.
>>>
>>>
>>> That op is never called because this driver is only instantiated by
>>> device
>>> tree. Since this code can't be skipped, it can't be moved to that op.
>>
>>
>> I don't understand what you're saying here. That op is called in
>> eth_initialize() on every device found in device tree.
>
>
> Oh, so it is. I must have screwed up my tracing before.
>
> Anyway, I still don't believe using write_hwaddr() is correct for this HW.
> It's marked optional in include/net.h; it would be implemented in cases
> where the MAC address should be passed to subsequent SW in Ethernet
> controller registers. That's not the case here. The master location for the
> MAC address is in an unrelated EEPROM that all drivers must read.

That sounds more like a NV storage location for a read_rom_hwaddr() op
to get a default mac addr that can be overridden with the env. The
write_hwaddr is about what the mac uses to filter for limiting packet
ingress. One reason to support it as an op is so that when the env var
for the mac address is changed, the mac filter in the hw is also
updated.

> Resetting
> the device (as any driver would do to guarantee correct operation no matter
> what SW ran before), or never initializing it in the first place (as is the
> case now without any driver in U-Boot) means that those registers cannot be
> assumed to contain valid data.

That's what the comment I quoted in my last mail is directly
addressing. It does make that data valid.

> Thus SW after U-Boot won't rely on the MAC
> address register values,

And yet it does in most cases, hence the comment I quoted in the last mail.

> and hence there's no need to implement
> .write_hwaddr() separately, since that's the only reason it exists
> separately according to the comments in include/net.h.

On 10/13/2016 05:46 PM, Joe Hershberger wrote:
> On Fri, Oct 14, 2016 at 1:35 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 10/11/2016 04:48 PM, Joe Hershberger wrote:
>>>
>>> On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren <swarren@wwwdotorg.org>
>>> wrote:
>>>>
>>>> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>>>>
>>>>>
>>>>> Hi Stephen,
>>>>>
>>>>> Thanks for sending this! I have some comments below.
>>>>>
>>>>> Cheers,
>>>>> -Joe
>>>>>
>>>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren <swarren@wwwdotorg.org>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>>
>>>>>> This driver supports the Synopsys Designware Ethernet QoS (Quality of
>>>>>> Service) a/k/a eqos IP block, which is a different design than the HW
>>>>>> supported by the existing designware.c driver. The IP supports many
>>>>>> options for bus type, clocking/reset structure, and feature list. This
>>>>>> driver currently supports the specific configuration used in NVIDIA's
>>>>>> Tegra186 chip, but should be extensible to other combinations quite
>>>>>> easily, as explained in the source.
>>>>
>>>>
>>>>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>>>>
>>>>
>>>>> +static int eqos_start(struct udevice *dev)
>>>>
>>>>
>>>>>> +       /* Update the MAC address */
>>>>>> +       val = (plat->enetaddr[5] << 8) |
>>>>>> +               (plat->enetaddr[4]);
>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>>>>> +       val = (plat->enetaddr[3] << 24) |
>>>>>> +               (plat->enetaddr[2] << 16) |
>>>>>> +               (plat->enetaddr[1] << 8) |
>>>>>> +               (plat->enetaddr[0]);
>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>>>>
>>>>>
>>>>>
>>>>> This should be implemented in write_hwaddr() op.
>>>>
>>>>
>>>> That op is never called because this driver is only instantiated by
>>>> device
>>>> tree. Since this code can't be skipped, it can't be moved to that op.
>>>
>>>
>>> I don't understand what you're saying here. That op is called in
>>> eth_initialize() on every device found in device tree.
>>
>>
>> Oh, so it is. I must have screwed up my tracing before.
>>
>> Anyway, I still don't believe using write_hwaddr() is correct for this HW.
>> It's marked optional in include/net.h; it would be implemented in cases
>> where the MAC address should be passed to subsequent SW in Ethernet
>> controller registers. That's not the case here. The master location for the
>> MAC address is in an unrelated EEPROM that all drivers must read.
>
> That sounds more like a NV storage location for a read_rom_hwaddr() op
> to get a default mac addr that can be overridden with the env.

If the EQoS HW module contained the interface to this EEPROM, such that 
all instances of the HW module always accessed the EEPROM in the same 
way and the layout of data in the EEPROM was fixed by the HW module, 
then yes.

However, the EqoS HW module doesn't define any mechanism for 
non-volatile MAC address storage; only the runtime registers. So, we 
can't implement read_rom_hwaddr() inside the EQoS driver unfortunately.

> The
> write_hwaddr is about what the mac uses to filter for limiting packet
> ingress. One reason to support it as an op is so that when the env var
> for the mac address is changed, the mac filter in the hw is also
> updated.

I believe that every time the Ethernet device is used, the start() op is 
called first, followed by packet transfer, followed by the stop() op. If 
start() always programs the MAC address, the driver will always end up 
using the value requested by the user.

>> Resetting
>> the device (as any driver would do to guarantee correct operation no matter
>> what SW ran before), or never initializing it in the first place (as is the
>> case now without any driver in U-Boot) means that those registers cannot be
>> assumed to contain valid data.
>
> That's what the comment I quoted in my last mail is directly
> addressing. It does make that data valid.

Resetting the device zeros out the MAC address value in the runtime 
registers. Perhaps we mean different things by "make the data valid", 
but resetting the device certainly doesn't load/activate any useful MAC 
address.

>> Thus SW after U-Boot won't rely on the MAC
>> address register values,
>
> And yet it does in most cases, hence the comment I quoted in the last mail.

This certainly isn't true for this HW at least, and I believe aside from 
any platform-specific hacks is true fairly generally across other 
Ethernet devices too. I've checked:

a) The mainline kernel's EQoS driver.

b) The Synopsis-supplied EQoS driver as used in NVIDIA's downstream 
Tegra kernel.

In both cases, the driver retrieves the desired MAC address from sources 
other than the EQoS registers (i.e. device tree, or a system-specific 
user-space application which sets the MAC address before enabling the 
interface), and unconditionally programs that value into the EQoS 
runtime registers.

I also talked to the only user of the mainline Linux EQoS driver, and he 
also is of the opinion that we can't rely on transferring the MAC 
address between U-Boot (or any FW/...) and Linux using the EQoS registers.

Hi Stephen,

On Mon, Oct 17, 2016 at 1:32 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/13/2016 05:46 PM, Joe Hershberger wrote:
>>
>> On Fri, Oct 14, 2016 at 1:35 AM, Stephen Warren <swarren@wwwdotorg.org>
>> wrote:
>>>
>>> On 10/11/2016 04:48 PM, Joe Hershberger wrote:
>>>>
>>>>
>>>> On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren <swarren@wwwdotorg.org>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Hi Stephen,
>>>>>>
>>>>>> Thanks for sending this! I have some comments below.
>>>>>>
>>>>>> Cheers,
>>>>>> -Joe
>>>>>>
>>>>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren
>>>>>> <swarren@wwwdotorg.org>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>>>>
>>>>>>> This driver supports the Synopsys Designware Ethernet QoS (Quality of
>>>>>>> Service) a/k/a eqos IP block, which is a different design than the HW
>>>>>>> supported by the existing designware.c driver. The IP supports many
>>>>>>> options for bus type, clocking/reset structure, and feature list.
>>>>>>> This
>>>>>>> driver currently supports the specific configuration used in NVIDIA's
>>>>>>> Tegra186 chip, but should be extensible to other combinations quite
>>>>>>> easily, as explained in the source.
>>>>>
>>>>>
>>>>>
>>>>>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>>>>>
>>>>>
>>>>>
>>>>>> +static int eqos_start(struct udevice *dev)
>>>>>
>>>>>
>>>>>
>>>>>>> +       /* Update the MAC address */
>>>>>>> +       val = (plat->enetaddr[5] << 8) |
>>>>>>> +               (plat->enetaddr[4]);
>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>>>>>> +       val = (plat->enetaddr[3] << 24) |
>>>>>>> +               (plat->enetaddr[2] << 16) |
>>>>>>> +               (plat->enetaddr[1] << 8) |
>>>>>>> +               (plat->enetaddr[0]);
>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> This should be implemented in write_hwaddr() op.
>>>>>
>>>>>
>>>>>
>>>>> That op is never called because this driver is only instantiated by
>>>>> device
>>>>> tree. Since this code can't be skipped, it can't be moved to that op.
>>>>
>>>>
>>>>
>>>> I don't understand what you're saying here. That op is called in
>>>> eth_initialize() on every device found in device tree.
>>>
>>>
>>>
>>> Oh, so it is. I must have screwed up my tracing before.
>>>
>>> Anyway, I still don't believe using write_hwaddr() is correct for this
>>> HW.
>>> It's marked optional in include/net.h; it would be implemented in cases
>>> where the MAC address should be passed to subsequent SW in Ethernet
>>> controller registers. That's not the case here. The master location for
>>> the
>>> MAC address is in an unrelated EEPROM that all drivers must read.
>>
>>
>> That sounds more like a NV storage location for a read_rom_hwaddr() op
>> to get a default mac addr that can be overridden with the env.
>
>
> If the EQoS HW module contained the interface to this EEPROM, such that all
> instances of the HW module always accessed the EEPROM in the same way and
> the layout of data in the EEPROM was fixed by the HW module, then yes.
>
> However, the EqoS HW module doesn't define any mechanism for non-volatile
> MAC address storage; only the runtime registers. So, we can't implement
> read_rom_hwaddr() inside the EQoS driver unfortunately.

OK.

>> The
>> write_hwaddr is about what the mac uses to filter for limiting packet
>> ingress. One reason to support it as an op is so that when the env var
>> for the mac address is changed, the mac filter in the hw is also
>> updated.
>
>
> I believe that every time the Ethernet device is used, the start() op is
> called first, followed by packet transfer, followed by the stop() op. If
> start() always programs the MAC address, the driver will always end up using
> the value requested by the user.

That may be. I still don't understand the reluctance to implement it.
When the behavior of start is restructured, it will be one fewer place
to be forced to retrofit.

>>> Resetting
>>> the device (as any driver would do to guarantee correct operation no
>>> matter
>>> what SW ran before), or never initializing it in the first place (as is
>>> the
>>> case now without any driver in U-Boot) means that those registers cannot
>>> be
>>> assumed to contain valid data.
>>
>>
>> That's what the comment I quoted in my last mail is directly
>> addressing. It does make that data valid.
>
>
> Resetting the device zeros out the MAC address value in the runtime
> registers. Perhaps we mean different things by "make the data valid", but
> resetting the device certainly doesn't load/activate any useful MAC address.

The point that comment was making was to initialize those registers in
all cases, so as to make them valid. Of course resetting undoes that.
At least some of the devices I've worked with use those registers to
communicate the MAC address from U-Boot to kernel. If that is not true
of this device, so be it.

>>> Thus SW after U-Boot won't rely on the MAC
>>> address register values,
>>
>>
>> And yet it does in most cases, hence the comment I quoted in the last
>> mail.
>
>
> This certainly isn't true for this HW at least, and I believe aside from any
> platform-specific hacks is true fairly generally across other Ethernet
> devices too. I've checked:

OK

> a) The mainline kernel's EQoS driver.
>
> b) The Synopsis-supplied EQoS driver as used in NVIDIA's downstream Tegra
> kernel.

Is this different from a) for some reason?

> In both cases, the driver retrieves the desired MAC address from sources
> other than the EQoS registers (i.e. device tree, or a system-specific
> user-space application which sets the MAC address before enabling the
> interface), and unconditionally programs that value into the EQoS runtime
> registers.
>
> I also talked to the only user of the mainline Linux EQoS driver, and he
> also is of the opinion that we can't rely on transferring the MAC address
> between U-Boot (or any FW/...) and Linux using the EQoS registers.

I think you can choose to rely on it and make it work. It's done by
others, but does not have to be if you choose to go another way.

If you were to rely on it the way others do, then you would be able to
isolate the knowledge about how to determine what the MAC is in a
single location in U-Boot and be able to share that logic between
U-Boot and Linux, since U-Boot is the only one that needs to know
about the EEPROM (for instance).

-Joe

On 10/19/2016 12:29 PM, Joe Hershberger wrote:
> Hi Stephen,
>
> On Mon, Oct 17, 2016 at 1:32 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 10/13/2016 05:46 PM, Joe Hershberger wrote:
>>> On Fri, Oct 14, 2016 at 1:35 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On 10/11/2016 04:48 PM, Joe Hershberger wrote:
>>>>> On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>>> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>>>>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>>>>> This driver supports the Synopsys Designware Ethernet QoS (Quality of
>>>>>>>> Service) a/k/a eqos IP block, which is a different design than the HW
>>>>>>>> supported by the existing designware.c driver. The IP supports many
>>>>>>>> options for bus type, clocking/reset structure, and feature list.
>>>>>>>> This
>>>>>>>> driver currently supports the specific configuration used in NVIDIA's
>>>>>>>> Tegra186 chip, but should be extensible to other combinations quite
>>>>>>>> easily, as explained in the source.
...
>>>>>>>> +static int eqos_start(struct udevice *dev)
...
>>>>>>>> +       /* Update the MAC address */
>>>>>>>> +       val = (plat->enetaddr[5] << 8) |
>>>>>>>> +               (plat->enetaddr[4]);
>>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>>>>>>> +       val = (plat->enetaddr[3] << 24) |
>>>>>>>> +               (plat->enetaddr[2] << 16) |
>>>>>>>> +               (plat->enetaddr[1] << 8) |
>>>>>>>> +               (plat->enetaddr[0]);
>>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>>>>>>
>>>>>>> This should be implemented in write_hwaddr() op.
...
>>>> Anyway, I still don't believe using write_hwaddr() is correct for this HW.
>>>> It's marked optional in include/net.h; it would be implemented in cases
>>>> where the MAC address should be passed to subsequent SW in Ethernet
>>>> controller registers. That's not the case here. The master location for
>>>> the MAC address is in an unrelated EEPROM that all drivers must read.
>>>
>>> That sounds more like a NV storage location for a read_rom_hwaddr() op
>>> to get a default mac addr that can be overridden with the env.
>>
>> If the EQoS HW module contained the interface to this EEPROM, such that all
>> instances of the HW module always accessed the EEPROM in the same way and
>> the layout of data in the EEPROM was fixed by the HW module, then yes.
>>
>> However, the EqoS HW module doesn't define any mechanism for non-volatile
>> MAC address storage; only the runtime registers. So, we can't implement
>> read_rom_hwaddr() inside the EQoS driver unfortunately.
>
> OK.
>
>>> The
>>> write_hwaddr is about what the mac uses to filter for limiting packet
>>> ingress. One reason to support it as an op is so that when the env var
>>> for the mac address is changed, the mac filter in the hw is also
>>> updated.
>>
>> I believe that every time the Ethernet device is used, the start() op is
>> called first, followed by packet transfer, followed by the stop() op. If
>> start() always programs the MAC address, the driver will always end up using
>> the value requested by the user.
>
> That may be. I still don't understand the reluctance to implement it.

I don't want to implement it because it can't work.

write_hwaddr() is called before start() is called. At that point, clocks 
to the EQoS HW are not running and the EQoS HW is in reset, and hence it 
cannot accept any register accesses; attempting any accesses will hang 
the bus and CPU.

These are the possible solutions:

a) Don't implement write_hwaddr()

b) Make write_hwaddr() turn on the clock and clear the reset, program 
the register, then reset the device and assert the reset. Re-asserting 
reset is required so that setting the MAC address doesn't leave the 
clock running even when the device isn't in use.This is pointless since 
the written value will not last beyond the end of the function.

c) Make probe() start the clock and clear the reset. Then write_hwaddr() 
can successfully write the MAC address registers at any time. This would 
waste power running the clock to the device when it's not in use. Also, 
Simon Glass continually asks that U-Boot not initialize HW that the user 
hasn't attempted to use. I believe turning on the clocks in probe() 
violates this.

d) Modify the network core to only call write_hwaddr() between the 
device's start() and stop() functions. I haven't looked into this at 
all, but I imagine it will have a fairly significant impact across many 
parts of the network core and other drivers.

 > When the behavior of start is restructured, it will be one fewer place
 > to be forced to retrofit.

I believe that the extra work required to refactor write_hwaddr() will 
be miniscule compared to splitting probe(), start(), and stop() up into 
separate functions anyway.

...
>> a) The mainline kernel's EQoS driver.
>>
>> b) The Synopsis-supplied EQoS driver as used in NVIDIA's downstream Tegra
>> kernel.
>
> Is this different from a) for some reason?

Yes. Synopsis supplies a non-mainlined Linux driver for the EQoS HW. For 
whatever reason, we're using that in our downstream kernel.

>> In both cases, the driver retrieves the desired MAC address from sources
>> other than the EQoS registers (i.e. device tree, or a system-specific
>> user-space application which sets the MAC address before enabling the
>> interface), and unconditionally programs that value into the EQoS runtime
>> registers.
>>
>> I also talked to the only user of the mainline Linux EQoS driver, and he
>> also is of the opinion that we can't rely on transferring the MAC address
>> between U-Boot (or any FW/...) and Linux using the EQoS registers.
>
> I think you can choose to rely on it and make it work. It's done by
> others, but does not have to be if you choose to go another way.
>
> If you were to rely on it the way others do, then you would be able to
> isolate the knowledge about how to determine what the MAC is in a
> single location in U-Boot and be able to share that logic between
> U-Boot and Linux, since U-Boot is the only one that needs to know
> about the EEPROM (for instance).

The knowledge is already isolated; the mainline Linux kernel will take 
the MAC address from device tree. This is the standard mechanism (for 
systems that boot using DT) that works across all Ethernet devices.

Hi,

On 19 October 2016 at 15:14, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/19/2016 12:29 PM, Joe Hershberger wrote:
>>
>> Hi Stephen,
>>
>> On Mon, Oct 17, 2016 at 1:32 PM, Stephen Warren <swarren@wwwdotorg.org>
>> wrote:
>>>
>>> On 10/13/2016 05:46 PM, Joe Hershberger wrote:
>>>>
>>>> On Fri, Oct 14, 2016 at 1:35 AM, Stephen Warren <swarren@wwwdotorg.org>
>>>> wrote:
>>>>>
>>>>> On 10/11/2016 04:48 PM, Joe Hershberger wrote:
>>>>>>
>>>>>> On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren
>>>>>> <swarren@wwwdotorg.org> wrote:
>>>>>>>
>>>>>>> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>>>>>>>
>>>>>>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren
>>>>>>>> <swarren@wwwdotorg.org> wrote:
>>>>>>>>>
>>>>>>>>> This driver supports the Synopsys Designware Ethernet QoS (Quality
>>>>>>>>> of
>>>>>>>>> Service) a/k/a eqos IP block, which is a different design than the
>>>>>>>>> HW
>>>>>>>>> supported by the existing designware.c driver. The IP supports many
>>>>>>>>> options for bus type, clocking/reset structure, and feature list.
>>>>>>>>> This
>>>>>>>>> driver currently supports the specific configuration used in
>>>>>>>>> NVIDIA's
>>>>>>>>> Tegra186 chip, but should be extensible to other combinations quite
>>>>>>>>> easily, as explained in the source.
>
> ...
>>>>>>>>>
>>>>>>>>> +static int eqos_start(struct udevice *dev)
>
> ...
>>>>>>>>>
>>>>>>>>> +       /* Update the MAC address */
>>>>>>>>> +       val = (plat->enetaddr[5] << 8) |
>>>>>>>>> +               (plat->enetaddr[4]);
>>>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>>>>>>>> +       val = (plat->enetaddr[3] << 24) |
>>>>>>>>> +               (plat->enetaddr[2] << 16) |
>>>>>>>>> +               (plat->enetaddr[1] << 8) |
>>>>>>>>> +               (plat->enetaddr[0]);
>>>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>>>>>>>
>>>>>>>>
>>>>>>>> This should be implemented in write_hwaddr() op.
>
> ...
>>>>>
>>>>> Anyway, I still don't believe using write_hwaddr() is correct for this
>>>>> HW.
>>>>> It's marked optional in include/net.h; it would be implemented in cases
>>>>> where the MAC address should be passed to subsequent SW in Ethernet
>>>>> controller registers. That's not the case here. The master location for
>>>>> the MAC address is in an unrelated EEPROM that all drivers must read.
>>>>
>>>>
>>>> That sounds more like a NV storage location for a read_rom_hwaddr() op
>>>> to get a default mac addr that can be overridden with the env.
>>>
>>>
>>> If the EQoS HW module contained the interface to this EEPROM, such that
>>> all
>>> instances of the HW module always accessed the EEPROM in the same way and
>>> the layout of data in the EEPROM was fixed by the HW module, then yes.
>>>
>>> However, the EqoS HW module doesn't define any mechanism for non-volatile
>>> MAC address storage; only the runtime registers. So, we can't implement
>>> read_rom_hwaddr() inside the EQoS driver unfortunately.
>>
>>
>> OK.
>>
>>>> The
>>>> write_hwaddr is about what the mac uses to filter for limiting packet
>>>> ingress. One reason to support it as an op is so that when the env var
>>>> for the mac address is changed, the mac filter in the hw is also
>>>> updated.
>>>
>>>
>>> I believe that every time the Ethernet device is used, the start() op is
>>> called first, followed by packet transfer, followed by the stop() op. If
>>> start() always programs the MAC address, the driver will always end up
>>> using
>>> the value requested by the user.
>>
>>
>> That may be. I still don't understand the reluctance to implement it.
>
>
> I don't want to implement it because it can't work.
>
> write_hwaddr() is called before start() is called. At that point, clocks to
> the EQoS HW are not running and the EQoS HW is in reset, and hence it cannot
> accept any register accesses; attempting any accesses will hang the bus and
> CPU.
>
> These are the possible solutions:
>
> a) Don't implement write_hwaddr()
>
> b) Make write_hwaddr() turn on the clock and clear the reset, program the
> register, then reset the device and assert the reset. Re-asserting reset is
> required so that setting the MAC address doesn't leave the clock running
> even when the device isn't in use.This is pointless since the written value
> will not last beyond the end of the function.
>
> c) Make probe() start the clock and clear the reset. Then write_hwaddr() can
> successfully write the MAC address registers at any time. This would waste
> power running the clock to the device when it's not in use. Also, Simon
> Glass continually asks that U-Boot not initialize HW that the user hasn't
> attempted to use. I believe turning on the clocks in probe() violates this.

Not quite...or at least if I did I was mistaken. Of course we should
limit init in probe() to what is necessary, but it is the bind()
method which must not touch hardware.

It is fine to turn clocks on in probe if you want to.

However I am wondering whether we have something wrong in the Ethernet
uclass interface. Should we move the MAC setting to after start(), or
similar?

>
> d) Modify the network core to only call write_hwaddr() between the device's
> start() and stop() functions. I haven't looked into this at all, but I
> imagine it will have a fairly significant impact across many parts of the
> network core and other drivers.
>
>> When the behavior of start is restructured, it will be one fewer place
>> to be forced to retrofit.
>
> I believe that the extra work required to refactor write_hwaddr() will be
> miniscule compared to splitting probe(), start(), and stop() up into
> separate functions anyway.
>
> ...
>>>
>>> a) The mainline kernel's EQoS driver.
>>>
>>> b) The Synopsis-supplied EQoS driver as used in NVIDIA's downstream Tegra
>>> kernel.
>>
>>
>> Is this different from a) for some reason?
>
>
> Yes. Synopsis supplies a non-mainlined Linux driver for the EQoS HW. For
> whatever reason, we're using that in our downstream kernel.
>
>>> In both cases, the driver retrieves the desired MAC address from sources
>>> other than the EQoS registers (i.e. device tree, or a system-specific
>>> user-space application which sets the MAC address before enabling the
>>> interface), and unconditionally programs that value into the EQoS runtime
>>> registers.
>>>
>>> I also talked to the only user of the mainline Linux EQoS driver, and he
>>> also is of the opinion that we can't rely on transferring the MAC address
>>> between U-Boot (or any FW/...) and Linux using the EQoS registers.
>>
>>
>> I think you can choose to rely on it and make it work. It's done by
>> others, but does not have to be if you choose to go another way.
>>
>> If you were to rely on it the way others do, then you would be able to
>> isolate the knowledge about how to determine what the MAC is in a
>> single location in U-Boot and be able to share that logic between
>> U-Boot and Linux, since U-Boot is the only one that needs to know
>> about the EEPROM (for instance).
>
>
> The knowledge is already isolated; the mainline Linux kernel will take the
> MAC address from device tree. This is the standard mechanism (for systems
> that boot using DT) that works across all Ethernet devices.

Regards,
Simon

On 10/20/2016 12:48 PM, Simon Glass wrote:
> Hi,
>
> On 19 October 2016 at 15:14, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 10/19/2016 12:29 PM, Joe Hershberger wrote:
>>>
>>> Hi Stephen,
>>>
>>> On Mon, Oct 17, 2016 at 1:32 PM, Stephen Warren <swarren@wwwdotorg.org>
>>> wrote:
>>>>
>>>> On 10/13/2016 05:46 PM, Joe Hershberger wrote:
>>>>>
>>>>> On Fri, Oct 14, 2016 at 1:35 AM, Stephen Warren <swarren@wwwdotorg.org>
>>>>> wrote:
>>>>>>
>>>>>> On 10/11/2016 04:48 PM, Joe Hershberger wrote:
>>>>>>>
>>>>>>> On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren
>>>>>>> <swarren@wwwdotorg.org> wrote:
>>>>>>>>
>>>>>>>> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>>>>>>>>
>>>>>>>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren
>>>>>>>>> <swarren@wwwdotorg.org> wrote:
>>>>>>>>>>
>>>>>>>>>> This driver supports the Synopsys Designware Ethernet QoS (Quality
>>>>>>>>>> of
>>>>>>>>>> Service) a/k/a eqos IP block, which is a different design than the
>>>>>>>>>> HW
>>>>>>>>>> supported by the existing designware.c driver. The IP supports many
>>>>>>>>>> options for bus type, clocking/reset structure, and feature list.
>>>>>>>>>> This
>>>>>>>>>> driver currently supports the specific configuration used in
>>>>>>>>>> NVIDIA's
>>>>>>>>>> Tegra186 chip, but should be extensible to other combinations quite
>>>>>>>>>> easily, as explained in the source.
>>
>> ...
>>>>>>>>>>
>>>>>>>>>> +static int eqos_start(struct udevice *dev)
>>
>> ...
>>>>>>>>>>
>>>>>>>>>> +       /* Update the MAC address */
>>>>>>>>>> +       val = (plat->enetaddr[5] << 8) |
>>>>>>>>>> +               (plat->enetaddr[4]);
>>>>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>>>>>>>>> +       val = (plat->enetaddr[3] << 24) |
>>>>>>>>>> +               (plat->enetaddr[2] << 16) |
>>>>>>>>>> +               (plat->enetaddr[1] << 8) |
>>>>>>>>>> +               (plat->enetaddr[0]);
>>>>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This should be implemented in write_hwaddr() op.
>>
>> ...
>>>>>>
>>>>>> Anyway, I still don't believe using write_hwaddr() is correct for this
>>>>>> HW.
>>>>>> It's marked optional in include/net.h; it would be implemented in cases
>>>>>> where the MAC address should be passed to subsequent SW in Ethernet
>>>>>> controller registers. That's not the case here. The master location for
>>>>>> the MAC address is in an unrelated EEPROM that all drivers must read.
>>>>>
>>>>>
>>>>> That sounds more like a NV storage location for a read_rom_hwaddr() op
>>>>> to get a default mac addr that can be overridden with the env.
>>>>
>>>>
>>>> If the EQoS HW module contained the interface to this EEPROM, such that
>>>> all
>>>> instances of the HW module always accessed the EEPROM in the same way and
>>>> the layout of data in the EEPROM was fixed by the HW module, then yes.
>>>>
>>>> However, the EqoS HW module doesn't define any mechanism for non-volatile
>>>> MAC address storage; only the runtime registers. So, we can't implement
>>>> read_rom_hwaddr() inside the EQoS driver unfortunately.
>>>
>>>
>>> OK.
>>>
>>>>> The
>>>>> write_hwaddr is about what the mac uses to filter for limiting packet
>>>>> ingress. One reason to support it as an op is so that when the env var
>>>>> for the mac address is changed, the mac filter in the hw is also
>>>>> updated.
>>>>
>>>>
>>>> I believe that every time the Ethernet device is used, the start() op is
>>>> called first, followed by packet transfer, followed by the stop() op. If
>>>> start() always programs the MAC address, the driver will always end up
>>>> using
>>>> the value requested by the user.
>>>
>>>
>>> That may be. I still don't understand the reluctance to implement it.
>>
>>
>> I don't want to implement it because it can't work.
>>
>> write_hwaddr() is called before start() is called. At that point, clocks to
>> the EQoS HW are not running and the EQoS HW is in reset, and hence it cannot
>> accept any register accesses; attempting any accesses will hang the bus and
>> CPU.
>>
>> These are the possible solutions:
>>
>> a) Don't implement write_hwaddr()
>>
>> b) Make write_hwaddr() turn on the clock and clear the reset, program the
>> register, then reset the device and assert the reset. Re-asserting reset is
>> required so that setting the MAC address doesn't leave the clock running
>> even when the device isn't in use.This is pointless since the written value
>> will not last beyond the end of the function.
>>
>> c) Make probe() start the clock and clear the reset. Then write_hwaddr() can
>> successfully write the MAC address registers at any time. This would waste
>> power running the clock to the device when it's not in use. Also, Simon
>> Glass continually asks that U-Boot not initialize HW that the user hasn't
>> attempted to use. I believe turning on the clocks in probe() violates this.
>
> Not quite...or at least if I did I was mistaken. Of course we should
> limit init in probe() to what is necessary, but it is the bind()
> method which must not touch hardware.
>
> It is fine to turn clocks on in probe if you want to.

Even for a device that the user never ultimately makes use of? If so, 
this might be a reasonable solution. It feels like U-Boot should turn 
off the clocks before booting an OS though so that the overall system 
state isn't any different between the cases where this driver is present 
and enabled vs not. With the clocks manipulated by start()/stop() we 
already do this. If the clocks are enabled in probe() instead, this 
won't be the case.

> However I am wondering whether we have something wrong in the Ethernet
> uclass interface. Should we move the MAC setting to after start(), or
> similar?

That's option d right below.

Joe's objection to this is that for some hardware, downstream OSs expect 
the MAC address to be programmed into controller registers before the OS 
starts. This required write_hwaddr() to be called even if the user 
doesn't actually make use of the Ethernet device, and hence in cases 
where start()/stop() are never called. This is probably more common for 
e.g. PCI devices where the bus imposes a certain system structure 
including the ability to access device registers immediately after boot 
without manual clock programming, rather than SoC-based designs where 
all bets are off regarding consistency, and system configuration data is 
more typically passed by either out-of-band configuration data 
structures like DT, or via entirely custom mechanisms.

>> d) Modify the network core to only call write_hwaddr() between the device's
>> start() and stop() functions. I haven't looked into this at all, but I
>> imagine it will have a fairly significant impact across many parts of the
>> network core and other drivers.
>>
>>> When the behavior of start is restructured, it will be one fewer place
>>> to be forced to retrofit.
>>
>> I believe that the extra work required to refactor write_hwaddr() will be
>> miniscule compared to splitting probe(), start(), and stop() up into
>> separate functions anyway.
>>
>> ...
>>>>
>>>> a) The mainline kernel's EQoS driver.
>>>>
>>>> b) The Synopsis-supplied EQoS driver as used in NVIDIA's downstream Tegra
>>>> kernel.
>>>
>>>
>>> Is this different from a) for some reason?
>>
>>
>> Yes. Synopsis supplies a non-mainlined Linux driver for the EQoS HW. For
>> whatever reason, we're using that in our downstream kernel.
>>
>>>> In both cases, the driver retrieves the desired MAC address from sources
>>>> other than the EQoS registers (i.e. device tree, or a system-specific
>>>> user-space application which sets the MAC address before enabling the
>>>> interface), and unconditionally programs that value into the EQoS runtime
>>>> registers.
>>>>
>>>> I also talked to the only user of the mainline Linux EQoS driver, and he
>>>> also is of the opinion that we can't rely on transferring the MAC address
>>>> between U-Boot (or any FW/...) and Linux using the EQoS registers.
>>>
>>>
>>> I think you can choose to rely on it and make it work. It's done by
>>> others, but does not have to be if you choose to go another way.
>>>
>>> If you were to rely on it the way others do, then you would be able to
>>> isolate the knowledge about how to determine what the MAC is in a
>>> single location in U-Boot and be able to share that logic between
>>> U-Boot and Linux, since U-Boot is the only one that needs to know
>>> about the EEPROM (for instance).
>>
>>
>> The knowledge is already isolated; the mainline Linux kernel will take the
>> MAC address from device tree. This is the standard mechanism (for systems
>> that boot using DT) that works across all Ethernet devices.
>
> Regards,
> Simon
>

On Thu, Oct 20, 2016 at 2:19 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 10/20/2016 12:48 PM, Simon Glass wrote:
>>
>> Hi,
>>
>> On 19 October 2016 at 15:14, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>
>>> On 10/19/2016 12:29 PM, Joe Hershberger wrote:
>>>>
>>>>
>>>> Hi Stephen,
>>>>
>>>> On Mon, Oct 17, 2016 at 1:32 PM, Stephen Warren <swarren@wwwdotorg.org>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 10/13/2016 05:46 PM, Joe Hershberger wrote:
>>>>>>
>>>>>>
>>>>>> On Fri, Oct 14, 2016 at 1:35 AM, Stephen Warren
>>>>>> <swarren@wwwdotorg.org>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 10/11/2016 04:48 PM, Joe Hershberger wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren
>>>>>>>> <swarren@wwwdotorg.org> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren
>>>>>>>>>> <swarren@wwwdotorg.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> This driver supports the Synopsys Designware Ethernet QoS
>>>>>>>>>>> (Quality
>>>>>>>>>>> of
>>>>>>>>>>> Service) a/k/a eqos IP block, which is a different design than
>>>>>>>>>>> the
>>>>>>>>>>> HW
>>>>>>>>>>> supported by the existing designware.c driver. The IP supports
>>>>>>>>>>> many
>>>>>>>>>>> options for bus type, clocking/reset structure, and feature list.
>>>>>>>>>>> This
>>>>>>>>>>> driver currently supports the specific configuration used in
>>>>>>>>>>> NVIDIA's
>>>>>>>>>>> Tegra186 chip, but should be extensible to other combinations
>>>>>>>>>>> quite
>>>>>>>>>>> easily, as explained in the source.
>>>
>>>
>>> ...
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> +static int eqos_start(struct udevice *dev)
>>>
>>>
>>> ...
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> +       /* Update the MAC address */
>>>>>>>>>>> +       val = (plat->enetaddr[5] << 8) |
>>>>>>>>>>> +               (plat->enetaddr[4]);
>>>>>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>>>>>>>>>> +       val = (plat->enetaddr[3] << 24) |
>>>>>>>>>>> +               (plat->enetaddr[2] << 16) |
>>>>>>>>>>> +               (plat->enetaddr[1] << 8) |
>>>>>>>>>>> +               (plat->enetaddr[0]);
>>>>>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This should be implemented in write_hwaddr() op.
>>>
>>>
>>> ...
>>>>>>>
>>>>>>>
>>>>>>> Anyway, I still don't believe using write_hwaddr() is correct for
>>>>>>> this
>>>>>>> HW.
>>>>>>> It's marked optional in include/net.h; it would be implemented in
>>>>>>> cases
>>>>>>> where the MAC address should be passed to subsequent SW in Ethernet
>>>>>>> controller registers. That's not the case here. The master location
>>>>>>> for
>>>>>>> the MAC address is in an unrelated EEPROM that all drivers must read.
>>>>>>
>>>>>>
>>>>>>
>>>>>> That sounds more like a NV storage location for a read_rom_hwaddr() op
>>>>>> to get a default mac addr that can be overridden with the env.
>>>>>
>>>>>
>>>>>
>>>>> If the EQoS HW module contained the interface to this EEPROM, such that
>>>>> all
>>>>> instances of the HW module always accessed the EEPROM in the same way
>>>>> and
>>>>> the layout of data in the EEPROM was fixed by the HW module, then yes.
>>>>>
>>>>> However, the EqoS HW module doesn't define any mechanism for
>>>>> non-volatile
>>>>> MAC address storage; only the runtime registers. So, we can't implement
>>>>> read_rom_hwaddr() inside the EQoS driver unfortunately.
>>>>
>>>>
>>>>
>>>> OK.
>>>>
>>>>>> The
>>>>>> write_hwaddr is about what the mac uses to filter for limiting packet
>>>>>> ingress. One reason to support it as an op is so that when the env var
>>>>>> for the mac address is changed, the mac filter in the hw is also
>>>>>> updated.
>>>>>
>>>>>
>>>>>
>>>>> I believe that every time the Ethernet device is used, the start() op
>>>>> is
>>>>> called first, followed by packet transfer, followed by the stop() op.
>>>>> If
>>>>> start() always programs the MAC address, the driver will always end up
>>>>> using
>>>>> the value requested by the user.
>>>>
>>>>
>>>>
>>>> That may be. I still don't understand the reluctance to implement it.
>>>
>>>
>>>
>>> I don't want to implement it because it can't work.
>>>
>>> write_hwaddr() is called before start() is called. At that point, clocks
>>> to
>>> the EQoS HW are not running and the EQoS HW is in reset, and hence it
>>> cannot
>>> accept any register accesses; attempting any accesses will hang the bus
>>> and
>>> CPU.
>>>
>>> These are the possible solutions:
>>>
>>> a) Don't implement write_hwaddr()
>>>
>>> b) Make write_hwaddr() turn on the clock and clear the reset, program the
>>> register, then reset the device and assert the reset. Re-asserting reset
>>> is
>>> required so that setting the MAC address doesn't leave the clock running
>>> even when the device isn't in use.This is pointless since the written
>>> value
>>> will not last beyond the end of the function.
>>>
>>> c) Make probe() start the clock and clear the reset. Then write_hwaddr()
>>> can
>>> successfully write the MAC address registers at any time. This would
>>> waste
>>> power running the clock to the device when it's not in use. Also, Simon
>>> Glass continually asks that U-Boot not initialize HW that the user hasn't
>>> attempted to use. I believe turning on the clocks in probe() violates
>>> this.
>>
>>
>> Not quite...or at least if I did I was mistaken. Of course we should
>> limit init in probe() to what is necessary, but it is the bind()
>> method which must not touch hardware.
>>
>> It is fine to turn clocks on in probe if you want to.
>
>
> Even for a device that the user never ultimately makes use of? If so, this
> might be a reasonable solution. It feels like U-Boot should turn off the
> clocks before booting an OS though so that the overall system state isn't
> any different between the cases where this driver is present and enabled vs
> not. With the clocks manipulated by start()/stop() we already do this. If
> the clocks are enabled in probe() instead, this won't be the case.
>
>> However I am wondering whether we have something wrong in the Ethernet
>> uclass interface. Should we move the MAC setting to after start(), or
>> similar?
>
>
> That's option d right below.
>
> Joe's objection to this is that for some hardware, downstream OSs expect the
> MAC address to be programmed into controller registers before the OS starts.
> This required write_hwaddr() to be called even if the user doesn't actually
> make use of the Ethernet device, and hence in cases where start()/stop() are
> never called. This is probably more common for e.g. PCI devices where the
> bus imposes a certain system structure including the ability to access
> device registers immediately after boot without manual clock programming,
> rather than SoC-based designs where all bets are off regarding consistency,
> and system configuration data is more typically passed by either out-of-band
> configuration data structures like DT, or via entirely custom mechanisms.

Maybe we should just make it a config option to select between the two
behaviors. Or is there already something that says "This system is
structured" vs "This system is a SoC / all bets are off"?

>>> d) Modify the network core to only call write_hwaddr() between the
>>> device's
>>> start() and stop() functions. I haven't looked into this at all, but I
>>> imagine it will have a fairly significant impact across many parts of the
>>> network core and other drivers.

On 10/20/2016 01:30 PM, Joe Hershberger wrote:
> On Thu, Oct 20, 2016 at 2:19 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 10/20/2016 12:48 PM, Simon Glass wrote:
>>>
>>> Hi,
>>>
>>> On 19 October 2016 at 15:14, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>
>>>> On 10/19/2016 12:29 PM, Joe Hershberger wrote:
>>>>>
>>>>>
>>>>> Hi Stephen,
>>>>>
>>>>> On Mon, Oct 17, 2016 at 1:32 PM, Stephen Warren <swarren@wwwdotorg.org>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> On 10/13/2016 05:46 PM, Joe Hershberger wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Oct 14, 2016 at 1:35 AM, Stephen Warren
>>>>>>> <swarren@wwwdotorg.org>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/11/2016 04:48 PM, Joe Hershberger wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, Oct 4, 2016 at 12:13 AM, Stephen Warren
>>>>>>>>> <swarren@wwwdotorg.org> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren
>>>>>>>>>>> <swarren@wwwdotorg.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> This driver supports the Synopsys Designware Ethernet QoS
>>>>>>>>>>>> (Quality
>>>>>>>>>>>> of
>>>>>>>>>>>> Service) a/k/a eqos IP block, which is a different design than
>>>>>>>>>>>> the
>>>>>>>>>>>> HW
>>>>>>>>>>>> supported by the existing designware.c driver. The IP supports
>>>>>>>>>>>> many
>>>>>>>>>>>> options for bus type, clocking/reset structure, and feature list.
>>>>>>>>>>>> This
>>>>>>>>>>>> driver currently supports the specific configuration used in
>>>>>>>>>>>> NVIDIA's
>>>>>>>>>>>> Tegra186 chip, but should be extensible to other combinations
>>>>>>>>>>>> quite
>>>>>>>>>>>> easily, as explained in the source.
>>>>
>>>>
>>>> ...
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> +static int eqos_start(struct udevice *dev)
>>>>
>>>>
>>>> ...
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> +       /* Update the MAC address */
>>>>>>>>>>>> +       val = (plat->enetaddr[5] << 8) |
>>>>>>>>>>>> +               (plat->enetaddr[4]);
>>>>>>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_HIGH);
>>>>>>>>>>>> +       val = (plat->enetaddr[3] << 24) |
>>>>>>>>>>>> +               (plat->enetaddr[2] << 16) |
>>>>>>>>>>>> +               (plat->enetaddr[1] << 8) |
>>>>>>>>>>>> +               (plat->enetaddr[0]);
>>>>>>>>>>>> +       writel(val, eqos->regs + EQOS_MAC_ADDRESS0_LOW);
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> This should be implemented in write_hwaddr() op.
>>>>
>>>>
>>>> ...
>>>>>>>>
>>>>>>>>
>>>>>>>> Anyway, I still don't believe using write_hwaddr() is correct for
>>>>>>>> this
>>>>>>>> HW.
>>>>>>>> It's marked optional in include/net.h; it would be implemented in
>>>>>>>> cases
>>>>>>>> where the MAC address should be passed to subsequent SW in Ethernet
>>>>>>>> controller registers. That's not the case here. The master location
>>>>>>>> for
>>>>>>>> the MAC address is in an unrelated EEPROM that all drivers must read.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> That sounds more like a NV storage location for a read_rom_hwaddr() op
>>>>>>> to get a default mac addr that can be overridden with the env.
>>>>>>
>>>>>>
>>>>>>
>>>>>> If the EQoS HW module contained the interface to this EEPROM, such that
>>>>>> all
>>>>>> instances of the HW module always accessed the EEPROM in the same way
>>>>>> and
>>>>>> the layout of data in the EEPROM was fixed by the HW module, then yes.
>>>>>>
>>>>>> However, the EqoS HW module doesn't define any mechanism for
>>>>>> non-volatile
>>>>>> MAC address storage; only the runtime registers. So, we can't implement
>>>>>> read_rom_hwaddr() inside the EQoS driver unfortunately.
>>>>>
>>>>>
>>>>>
>>>>> OK.
>>>>>
>>>>>>> The
>>>>>>> write_hwaddr is about what the mac uses to filter for limiting packet
>>>>>>> ingress. One reason to support it as an op is so that when the env var
>>>>>>> for the mac address is changed, the mac filter in the hw is also
>>>>>>> updated.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I believe that every time the Ethernet device is used, the start() op
>>>>>> is
>>>>>> called first, followed by packet transfer, followed by the stop() op.
>>>>>> If
>>>>>> start() always programs the MAC address, the driver will always end up
>>>>>> using
>>>>>> the value requested by the user.
>>>>>
>>>>>
>>>>>
>>>>> That may be. I still don't understand the reluctance to implement it.
>>>>
>>>>
>>>>
>>>> I don't want to implement it because it can't work.
>>>>
>>>> write_hwaddr() is called before start() is called. At that point, clocks
>>>> to
>>>> the EQoS HW are not running and the EQoS HW is in reset, and hence it
>>>> cannot
>>>> accept any register accesses; attempting any accesses will hang the bus
>>>> and
>>>> CPU.
>>>>
>>>> These are the possible solutions:
>>>>
>>>> a) Don't implement write_hwaddr()
>>>>
>>>> b) Make write_hwaddr() turn on the clock and clear the reset, program the
>>>> register, then reset the device and assert the reset. Re-asserting reset
>>>> is
>>>> required so that setting the MAC address doesn't leave the clock running
>>>> even when the device isn't in use.This is pointless since the written
>>>> value
>>>> will not last beyond the end of the function.
>>>>
>>>> c) Make probe() start the clock and clear the reset. Then write_hwaddr()
>>>> can
>>>> successfully write the MAC address registers at any time. This would
>>>> waste
>>>> power running the clock to the device when it's not in use. Also, Simon
>>>> Glass continually asks that U-Boot not initialize HW that the user hasn't
>>>> attempted to use. I believe turning on the clocks in probe() violates
>>>> this.
>>>
>>>
>>> Not quite...or at least if I did I was mistaken. Of course we should
>>> limit init in probe() to what is necessary, but it is the bind()
>>> method which must not touch hardware.
>>>
>>> It is fine to turn clocks on in probe if you want to.
>>
>>
>> Even for a device that the user never ultimately makes use of? If so, this
>> might be a reasonable solution. It feels like U-Boot should turn off the
>> clocks before booting an OS though so that the overall system state isn't
>> any different between the cases where this driver is present and enabled vs
>> not. With the clocks manipulated by start()/stop() we already do this. If
>> the clocks are enabled in probe() instead, this won't be the case.
>>
>>> However I am wondering whether we have something wrong in the Ethernet
>>> uclass interface. Should we move the MAC setting to after start(), or
>>> similar?
>>
>>
>> That's option d right below.
>>
>> Joe's objection to this is that for some hardware, downstream OSs expect the
>> MAC address to be programmed into controller registers before the OS starts.
>> This required write_hwaddr() to be called even if the user doesn't actually
>> make use of the Ethernet device, and hence in cases where start()/stop() are
>> never called. This is probably more common for e.g. PCI devices where the
>> bus imposes a certain system structure including the ability to access
>> device registers immediately after boot without manual clock programming,
>> rather than SoC-based designs where all bets are off regarding consistency,
>> and system configuration data is more typically passed by either out-of-band
>> configuration data structures like DT, or via entirely custom mechanisms.
>
> Maybe we should just make it a config option to select between the two
> behaviors. Or is there already something that says "This system is
> structured" vs "This system is a SoC / all bets are off"?

Perhaps we should make a run-time decision rather than compile-time. 
This will allow a single system to contain multiple instances of the 
EQoS HW block with different behaviour, e.g. 1 embedded in the SoC, and 
1 plug-in PCIe device.

We can assume that for any PCIe device, we can implement write_hwaddr() 
without issue.

For devices instantiated from DT, the SoC-level integration details will 
drive whether there is SW control over the HW block's clock/reset 
signals. We can use struct udevice_id's .data field to provide 
parameters for each different DT compatible value that the driver 
supports, which can indicate how write_hwaddr() should work.

Then, we can implement roughly the following:

write_hwaddr:
     if (!dev->reg_access_ok && !dev->reg_access_always_ok)
         return 0;
     write MAC address registers

That should also allow write_hwaddr to operate correctly if 
start()/stop() are drivers are refactored not to start()/stop() for 
every separate network transaction, since presumably the code that sets 
reg_access_ok will be refactored as part of that.

On 10/11/2016 05:24 PM, Joe Hershberger wrote:
> Hi Stephen,
>
> On Tue, Sep 27, 2016 at 12:02 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 09/23/2016 03:49 PM, Joe Hershberger wrote:
>>>
>>> Hi Stephen,
>>>
>>> Thanks for sending this! I have some comments below.
>>>
>>> Cheers,
>>> -Joe
>>>
>>> On Mon, Sep 12, 2016 at 12:48 PM, Stephen Warren <swarren@wwwdotorg.org>
>>> wrote:
>>>>
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> This driver supports the Synopsys Designware Ethernet QoS (Quality of
>>>> Service) a/k/a eqos IP block, which is a different design than the HW
>>>> supported by the existing designware.c driver. The IP supports many
>>>> options for bus type, clocking/reset structure, and feature list. This
>>>> driver currently supports the specific configuration used in NVIDIA's
>>>> Tegra186 chip, but should be extensible to other combinations quite
>>>> easily, as explained in the source.
>>
>>>> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
>>
>>
>>>> +/* Core registers */
>>>
>>> Why are registers not defined as a struct? That is the common way that
>>> is accepted to represent registers in a driver.
>>
>>
>> It is common, but I find using structs has significant disadvantages, which
>> outweigh any advantages, so I strongly prefer not to use them.
>>
>> Disadvantages of structs:
>>
>> * Drivers often have to deal with different but similar HW variants. Similar
>> enough that it makes sense to use the same driver for them, but different
>> enough that certain registers are placed differently in the memory map. With
>> structs, there's little choice but to use ifdefs to pick a particular HW
>> variant and bake that into the driver. This doesn't work well for drivers
>> that must pick the variant at run-time rather than compile-time, e.g.
>> drivers for USB or PCIe devices, or when you wish to build a single SW image
>> that can run on multiple SoCs.
>
> Is that really the case here or are you just making a broad statement?
> Are registers really moving based on IP configuration? That sounds
> like broken IP / IP generator.

I think we should choose the best techniques and use them globally, even 
if a particular disadvantage of a non-optimal technique isn't relevant 
to a particular device. Thus, I don't think whether I'm making a broad 
statement is too relevant.

I know there are multiple versions of this HW block in existence. I 
don't know the details of the register compatibility between the 
different versions for sure.

There are many broken IPs and IP generators in existence, and I believe 
we need to accomodate that.

>> A common variant on this theme is device with different register layouts due
>> to IP integration issues, such as a UART with byte-wide registers that
>> appear at consecutive byte offsets in some SoCs, but at separate word
>> addresses in other SoCs.
>
> Again, this sounds like a generic argument that doesn't apply here.

I don't believe the distinction is appropriate.

>> This issue is relevant here since the EQoS block is a generic IP block with
>> many options that may have different SoC- or IP-version-specific differences
>> between SoCs.
>
> But simply additive, no? Included features add registers or bitfields.

As far as I know, NVIDIA's register additions to this particular version 
of this particular HW block are purely additions in a well segregated 
register block.

This certainly isn't true for other HW blocks in Tegra though (e.g. 
consider the Synopsys USB2 controller Tegra uses), and I'm sure the same 
applies to many other HW blocks in many other SoCs. I wouldn't be at all 
surprised if some SoC vendor does the same thing to this IP block. 
There's no way to tell until someone appears to integrate driver support 
into U-Boot.

>> * The register space for the EQoS driver is quite sparse, and U-Boot uses a
>> subset of the registers effectively making it even more sparse. Defining a
>> struct to describe this will be a frustrating exercise in calculating the
>> right amount of padding to place into the struct to get the correct offsets.
>> Mistakes will be made and it will be annoying.
>
> It's also organized into a few blocks. It's preferable to group
> registers into a struct that represents each block instead of one huge
> struct.

That will certainly help.

> It also hurts nothing to have registers defined in the struct
> that apply to configurations that may not be enabled in a given
> instance... the switching can be done at the spot where the register
> is accessed. And the accesses don't have to be compile-time options so
> you can have your built-in and your PCIe variants.

So long as there aren't conflicting register definitions, which is 
certainly not guaranteed in any way, what you say is true.

Anyway, I'll rewrite the driver using structs for now in the interests 
of moving it forward. If this causes issue down the road, whomever is 
adding the support for additional devices can worry about how to solve 
any issues that arise then.

>>>> +static int eqos_start_clks_tegra(struct udevice *dev)
>>>
>>> Why is this not in a board file and a separate patch?
>>
>> The clocks (and other resources) that this driver operates on are directly
>> related to the EQoS IP block itself. Hence, it's completely appropriate for
>> this driver to deal with them.
>>
>> Perhaps the name "tegra" is slightly misleading; it refers to "the
>> configuration of the core EQoS block that Tegra happens to have chosen"
>> rather than "something entirely Tegra-specific that is unrelated to this
>> device/driver". However, I'm not sure what name would be better than "tegra"
>> here; the correct name is something like
>> axi_ahb_dma_mtl_core_single_phy_rgmii, but that's far too long for a symbol
>> name. I attempted to describe this in the comment regarding IP configuration
>> at the beginning of the file. Is there a specific shortcoming in that
>> comment that could be changed to address this better?
>
> I think the comment should list the configuration with at least the
> level of detail that you used to create that long symbol name.
>
> Do you anticipate that this set of configurations will with high
> confidence be used in Tegra going forward? Or would it be better to
> call it tegra186?

It's too early to tell. I hope we'll maintain the same configuration in 
any future SoCs, but who knows.

I can rename it all to tegra186 if you want; we can simply call the 
tegra186 functions when running on any future SoCs that are 
backwards-compatible, which would be conceptually similar to the way 
Device Tree compatible values are extended.