[U-Boot] [PATCH 1/9] net: Revert "tftp: adjust settings to be suitable for 100Mbit ethernet"

[U-Boot] [PATCH 1/9] net: Revert "tftp: adjust settings to be suitable for 100Mbit ethernet"

[Bin Meng]

Testing either pch_gbe or e1000 driver via tftp command on Intel
Crown Bay board, shows the following failure.

    TFTP error: 'Unsupported option(s) requested' (8)

It turns out commit 620776d causes this. Revert this commit for now.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 net/tftp.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/tftp.c b/net/tftp.c
index 18ce84c..89be32a 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -19,10 +19,10 @@
 /* Well known TFTP port # */
 #define WELL_KNOWN_PORT	69
 /* Millisecs to timeout for lost pkt */
-#define TIMEOUT		100UL
+#define TIMEOUT		5000UL
 #ifndef	CONFIG_NET_RETRY_COUNT
 /* # of timeouts before giving up */
-# define TIMEOUT_COUNT	1000
+# define TIMEOUT_COUNT	10
 #else
 # define TIMEOUT_COUNT  (CONFIG_NET_RETRY_COUNT * 2)
 #endif
@@ -711,10 +711,10 @@ void tftp_start(enum proto_t protocol)
 	if (ep != NULL)
 		timeout_ms = simple_strtol(ep, NULL, 10);
 
-	if (timeout_ms < 10) {
-		printf("TFTP timeout (%ld ms) too low, set min = 10 ms\n",
+	if (timeout_ms < 1000) {
+		printf("TFTP timeout (%ld ms) too low, set min = 1000 ms\n",
 		       timeout_ms);
-		timeout_ms = 10;
+		timeout_ms = 1000;
 	}
 
 	debug("TFTP blocksize = %i, timeout = %ld ms\n",
-- 
1.8.2.1

[U-Boot] [PATCH 2/9] net: e1000: Fix build warnings for 32-bit

[Bin Meng]

commit 6497e37 "net: e1000: Support 64-bit physical address" causes
compiler warnings on 32-bit U-Boot build below.

drivers/net/e1000.c: In function 'e1000_configure_tx':
drivers/net/e1000.c:4982:2: warning: right shift count >= width of type [enabled by default]
drivers/net/e1000.c: In function 'e1000_configure_rx':
drivers/net/e1000.c:5126:2: warning: right shift count >= width of type [enabled by default]

This commit fixes the build warnings.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/net/e1000.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
index 6f74d30..a467280 100644
--- a/drivers/net/e1000.c
+++ b/drivers/net/e1000.c
@@ -4977,9 +4977,10 @@ e1000_configure_tx(struct e1000_hw *hw)
 	unsigned long tctl;
 	unsigned long tipg, tarc;
 	uint32_t ipgr1, ipgr2;
+	uint64_t tdba = (unsigned long)tx_base;
 
-	E1000_WRITE_REG(hw, TDBAL, (unsigned long)tx_base & 0xffffffff);
-	E1000_WRITE_REG(hw, TDBAH, (unsigned long)tx_base >> 32);
+	E1000_WRITE_REG(hw, TDBAL, (uint32_t)(tdba & 0xffffffff));
+	E1000_WRITE_REG(hw, TDBAH, (uint32_t)(tdba >> 32));
 
 	E1000_WRITE_REG(hw, TDLEN, 128);
 
@@ -5103,6 +5104,8 @@ e1000_configure_rx(struct e1000_hw *hw)
 {
 	unsigned long rctl, ctrl_ext;
 	rx_tail = 0;
+	uint64_t rdba = (unsigned long)rx_base;
+
 	/* make sure receives are disabled while setting up the descriptors */
 	rctl = E1000_READ_REG(hw, RCTL);
 	E1000_WRITE_REG(hw, RCTL, rctl & ~E1000_RCTL_EN);
@@ -5122,8 +5125,8 @@ e1000_configure_rx(struct e1000_hw *hw)
 		E1000_WRITE_FLUSH(hw);
 	}
 	/* Setup the Base and Length of the Rx Descriptor Ring */
-	E1000_WRITE_REG(hw, RDBAL, (unsigned long)rx_base & 0xffffffff);
-	E1000_WRITE_REG(hw, RDBAH, (unsigned long)rx_base >> 32);
+	E1000_WRITE_REG(hw, RDBAL, (uint32_t)(rdba & 0xffffffff));
+	E1000_WRITE_REG(hw, RDBAH, (uint32_t)(rdba >> 32));
 
 	E1000_WRITE_REG(hw, RDLEN, 128);
 
-- 
1.8.2.1

[U-Boot] [PATCH 3/9] dm: eth: Do not call board_eth_init() or cpu_eth_init()

[Bin Meng]

With driver model, board_eth_init() or cpu_eth_init() is not needed.
Remove the call to these in eth_common_init().

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 net/eth.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/eth.c b/net/eth.c
index d3ec8d6..0b4b08a 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -96,6 +96,7 @@ static void eth_common_init(void)
 	phy_init();
 #endif
 
+#ifndef CONFIG_DM_ETH
 	/*
 	 * If board-specific initialization exists, call it.
 	 * If not, call a CPU-specific one
@@ -109,6 +110,7 @@ static void eth_common_init(void)
 	} else {
 		printf("Net Initialization Skipped\n");
 	}
+#endif
 }
 
 #ifdef CONFIG_DM_ETH
-- 
1.8.2.1

[U-Boot] [PATCH 4/9] dm: eth: Correctly detect alias in eth_get_dev_by_name()

[Bin Meng]

When given a device name string, we should test if it contains "eth"
before we treat it as an alias.

With this commit, now we are really able to rotate between network
interfaces with driver model (previously it was broken).

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

 net/eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/eth.c b/net/eth.c
index 0b4b08a..fbf30b0 100644
--- a/net/eth.c
+++ b/net/eth.c
@@ -197,7 +197,7 @@ struct udevice *eth_get_dev_by_name(const char *devname)
 	struct uclass *uc;
 
 	/* Must be longer than 3 to be an alias */
-	if (strlen(devname) > strlen("eth")) {
+	if (strstr(devname, "eth") && strlen(devname) > strlen("eth")) {
 		startp = devname + strlen("eth");
 		seq = simple_strtoul(startp, &endp, 10);
 	}
-- 
1.8.2.1

[U-Boot] [PATCH 5/9] x86: crownbay: Convert to use CONFIG_DM_USB

[Bin Meng]

Move to driver model for USB on Intel Crown Bay.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 configs/crownbay_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configs/crownbay_defconfig b/configs/crownbay_defconfig
index 4fc1827..6edd710 100644
--- a/configs/crownbay_defconfig
+++ b/configs/crownbay_defconfig
@@ -23,6 +23,8 @@ CONFIG_NETDEVICES=y
 CONFIG_E1000=y
 CONFIG_VIDEO_VESA=y
 CONFIG_FRAMEBUFFER_SET_VESA_MODE=y
+CONFIG_USB=y
+CONFIG_DM_USB=y
 CONFIG_DM_RTC=y
 CONFIG_USE_PRIVATE_LIBGCC=y
 CONFIG_SYS_VSNPRINTF=y
-- 
1.8.2.1

[U-Boot] [PATCH 6/9] x86: crownbay: Convert to use CONFIG_DM_ETH for E1000

[Bin Meng]

Since E1000 driver has been converted to driver model, enable it
on Intel Crown Bay. But the Intel Topcliff GbE driver has not been
converted to driver model yet, disable it for now.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 board/intel/crownbay/crownbay.c | 6 ------
 configs/crownbay_defconfig      | 2 +-
 include/configs/crownbay.h      | 1 -
 3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/board/intel/crownbay/crownbay.c b/board/intel/crownbay/crownbay.c
index d6de9fa..3a79e69 100644
--- a/board/intel/crownbay/crownbay.c
+++ b/board/intel/crownbay/crownbay.c
@@ -7,7 +7,6 @@
 #include <common.h>
 #include <asm/ibmpc.h>
 #include <asm/pnp_def.h>
-#include <netdev.h>
 #include <smsc_lpc47m.h>
 
 int board_early_init_f(void)
@@ -24,8 +23,3 @@ void setup_pch_gpios(u16 gpiobase, const struct pch_gpio_map *gpio)
 {
 	return;
 }
-
-int board_eth_init(bd_t *bis)
-{
-	return pci_eth_init(bis);
-}
diff --git a/configs/crownbay_defconfig b/configs/crownbay_defconfig
index 6edd710..f027faf 100644
--- a/configs/crownbay_defconfig
+++ b/configs/crownbay_defconfig
@@ -19,7 +19,7 @@ CONFIG_OF_CONTROL=y
 CONFIG_CPU=y
 CONFIG_DM_PCI=y
 CONFIG_SPI_FLASH=y
-CONFIG_NETDEVICES=y
+CONFIG_DM_ETH=y
 CONFIG_E1000=y
 CONFIG_VIDEO_VESA=y
 CONFIG_FRAMEBUFFER_SET_VESA_MODE=y
diff --git a/include/configs/crownbay.h b/include/configs/crownbay.h
index 998da78..a344c85 100644
--- a/include/configs/crownbay.h
+++ b/include/configs/crownbay.h
@@ -50,7 +50,6 @@
 #define CONFIG_CMD_MMC
 
 /* Topcliff Gigabit Ethernet */
-#define CONFIG_PCH_GBE
 #define CONFIG_PHYLIB
 
 /* Environment configuration */
-- 
1.8.2.1

[U-Boot] [PATCH 7/9] net: pch_gbe: Convert to driver model

[Bin Meng]

This commit converts pch_gbe ethernet driver to driver model.

Since this driver is only used by Intel Crown Bay board, the
conversion does not keep the non-dm version.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/net/pch_gbe.c | 133 +++++++++++++++++++++++++++-----------------------
 drivers/net/pch_gbe.h |   2 -
 include/netdev.h      |   4 --
 3 files changed, 73 insertions(+), 66 deletions(-)

diff --git a/drivers/net/pch_gbe.c b/drivers/net/pch_gbe.c
index a03bdc0..004fcf8 100644
--- a/drivers/net/pch_gbe.c
+++ b/drivers/net/pch_gbe.c
@@ -7,10 +7,10 @@
  */
 
 #include <common.h>
+#include <dm.h>
 #include <errno.h>
 #include <asm/io.h>
 #include <pci.h>
-#include <malloc.h>
 #include <miiphy.h>
 #include "pch_gbe.h"
 
@@ -19,7 +19,7 @@
 #endif
 
 static struct pci_device_id supported[] = {
-	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TCF_GBE },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_TCF_GBE) },
 	{ }
 };
 
@@ -62,9 +62,10 @@ static int pch_gbe_mac_write(struct pch_gbe_regs *mac_regs, u8 *addr)
 	return -ETIME;
 }
 
-static int pch_gbe_reset(struct eth_device *dev)
+static int pch_gbe_reset(struct udevice *dev)
 {
-	struct pch_gbe_priv *priv = dev->priv;
+	struct pch_gbe_priv *priv = dev_get_priv(dev);
+	struct eth_pdata *plat = dev_get_platdata(dev);
 	struct pch_gbe_regs *mac_regs = priv->mac_regs;
 	ulong start;
 
@@ -97,7 +98,7 @@ static int pch_gbe_reset(struct eth_device *dev)
 			 * so we have to reload MAC address here in order to
 			 * make linux pch_gbe driver happy.
 			 */
-			return pch_gbe_mac_write(mac_regs, dev->enetaddr);
+			return pch_gbe_mac_write(mac_regs, plat->enetaddr);
 		}
 
 		udelay(10);
@@ -107,9 +108,9 @@ static int pch_gbe_reset(struct eth_device *dev)
 	return -ETIME;
 }
 
-static void pch_gbe_rx_descs_init(struct eth_device *dev)
+static void pch_gbe_rx_descs_init(struct udevice *dev)
 {
-	struct pch_gbe_priv *priv = dev->priv;
+	struct pch_gbe_priv *priv = dev_get_priv(dev);
 	struct pch_gbe_regs *mac_regs = priv->mac_regs;
 	struct pch_gbe_rx_desc *rx_desc = &priv->rx_desc[0];
 	int i;
@@ -128,9 +129,9 @@ static void pch_gbe_rx_descs_init(struct eth_device *dev)
 	       &mac_regs->rx_dsc_sw_p);
 }
 
-static void pch_gbe_tx_descs_init(struct eth_device *dev)
+static void pch_gbe_tx_descs_init(struct udevice *dev)
 {
-	struct pch_gbe_priv *priv = dev->priv;
+	struct pch_gbe_priv *priv = dev_get_priv(dev);
 	struct pch_gbe_regs *mac_regs = priv->mac_regs;
 	struct pch_gbe_tx_desc *tx_desc = &priv->tx_desc[0];
 
@@ -183,9 +184,9 @@ static void pch_gbe_adjust_link(struct pch_gbe_regs *mac_regs,
 	return;
 }
 
-static int pch_gbe_init(struct eth_device *dev, bd_t *bis)
+static int pch_gbe_start(struct udevice *dev)
 {
-	struct pch_gbe_priv *priv = dev->priv;
+	struct pch_gbe_priv *priv = dev_get_priv(dev);
 	struct pch_gbe_regs *mac_regs = priv->mac_regs;
 
 	if (pch_gbe_reset(dev))
@@ -226,18 +227,18 @@ static int pch_gbe_init(struct eth_device *dev, bd_t *bis)
 	return 0;
 }
 
-static void pch_gbe_halt(struct eth_device *dev)
+static void pch_gbe_stop(struct udevice *dev)
 {
-	struct pch_gbe_priv *priv = dev->priv;
+	struct pch_gbe_priv *priv = dev_get_priv(dev);
 
 	pch_gbe_reset(dev);
 
 	phy_shutdown(priv->phydev);
 }
 
-static int pch_gbe_send(struct eth_device *dev, void *packet, int length)
+static int pch_gbe_send(struct udevice *dev, void *packet, int length)
 {
-	struct pch_gbe_priv *priv = dev->priv;
+	struct pch_gbe_priv *priv = dev_get_priv(dev);
 	struct pch_gbe_regs *mac_regs = priv->mac_regs;
 	struct pch_gbe_tx_desc *tx_head, *tx_desc;
 	u16 frame_ctrl = 0;
@@ -277,15 +278,13 @@ static int pch_gbe_send(struct eth_device *dev, void *packet, int length)
 	return -ETIME;
 }
 
-static int pch_gbe_recv(struct eth_device *dev)
+static int pch_gbe_recv(struct udevice *dev, int flags, uchar **packetp)
 {
-	struct pch_gbe_priv *priv = dev->priv;
+	struct pch_gbe_priv *priv = dev_get_priv(dev);
 	struct pch_gbe_regs *mac_regs = priv->mac_regs;
-	struct pch_gbe_rx_desc *rx_head, *rx_desc;
+	struct pch_gbe_rx_desc *rx_desc;
 	u32 hw_desc, buffer_addr, length;
-	int rx_swp;
 
-	rx_head = &priv->rx_desc[0];
 	rx_desc = &priv->rx_desc[priv->rx_idx];
 
 	readl(&mac_regs->int_st);
@@ -293,11 +292,21 @@ static int pch_gbe_recv(struct eth_device *dev)
 
 	/* Just return if not receiving any packet */
 	if ((u32)rx_desc == hw_desc)
-		return 0;
+		return -EAGAIN;
 
 	buffer_addr = pci_mem_to_phys(priv->bdf, rx_desc->buffer_addr);
+	*packetp = (uchar *)buffer_addr;
 	length = rx_desc->rx_words_eob - 3 - ETH_FCS_LEN;
-	net_process_received_packet((uchar *)buffer_addr, length);
+
+	return length;
+}
+
+static int pch_gbe_free_pkt(struct udevice *dev, uchar *packet, int length)
+{
+	struct pch_gbe_priv *priv = dev_get_priv(dev);
+	struct pch_gbe_regs *mac_regs = priv->mac_regs;
+	struct pch_gbe_rx_desc *rx_head = &priv->rx_desc[0];
+	int rx_swp;
 
 	/* Test the wrap-around condition */
 	if (++priv->rx_idx >= PCH_GBE_DESC_NUM)
@@ -309,7 +318,7 @@ static int pch_gbe_recv(struct eth_device *dev)
 	writel(pci_phys_to_mem(priv->bdf, (u32)(rx_head + rx_swp)),
 	       &mac_regs->rx_dsc_sw_p);
 
-	return length;
+	return 0;
 }
 
 static int pch_gbe_mdio_ready(struct pch_gbe_regs *mac_regs)
@@ -365,7 +374,7 @@ static int pch_gbe_mdio_write(struct mii_dev *bus, int addr, int devad,
 		return 0;
 }
 
-static int pch_gbe_mdio_init(char *name, struct pch_gbe_regs *mac_regs)
+static int pch_gbe_mdio_init(const char *name, struct pch_gbe_regs *mac_regs)
 {
 	struct mii_dev *bus;
 
@@ -384,13 +393,14 @@ static int pch_gbe_mdio_init(char *name, struct pch_gbe_regs *mac_regs)
 	return mdio_register(bus);
 }
 
-static int pch_gbe_phy_init(struct eth_device *dev)
+static int pch_gbe_phy_init(struct udevice *dev)
 {
-	struct pch_gbe_priv *priv = dev->priv;
+	struct pch_gbe_priv *priv = dev_get_priv(dev);
+	struct eth_pdata *plat = dev_get_platdata(dev);
 	struct phy_device *phydev;
 	int mask = 0xffffffff;
 
-	phydev = phy_find_by_mask(priv->bus, mask, priv->interface);
+	phydev = phy_find_by_mask(priv->bus, mask, plat->phy_interface);
 	if (!phydev) {
 		printf("pch_gbe: cannot find the phy\n");
 		return -1;
@@ -404,63 +414,66 @@ static int pch_gbe_phy_init(struct eth_device *dev)
 	priv->phydev = phydev;
 	phy_config(phydev);
 
-	return 1;
+	return 0;
 }
 
-int pch_gbe_register(bd_t *bis)
+int pch_gbe_probe(struct udevice *dev)
 {
-	struct eth_device *dev;
 	struct pch_gbe_priv *priv;
+	struct eth_pdata *plat = dev_get_platdata(dev);
 	pci_dev_t devno;
 	u32 iobase;
 
-	devno = pci_find_devices(supported, 0);
-	if (devno == -1)
-		return -ENODEV;
-
-	dev = (struct eth_device *)malloc(sizeof(*dev));
-	if (!dev)
-		return -ENOMEM;
-	memset(dev, 0, sizeof(*dev));
+	devno = pci_get_bdf(dev);
 
 	/*
 	 * The priv structure contains the descriptors and frame buffers which
-	 * need a strict buswidth alignment (64 bytes)
+	 * need a strict buswidth alignment (64 bytes). This is guaranteed by
+	 * DM_FLAG_ALLOC_PRIV_DMA flag in the U_BOOT_DRIVER.
 	 */
-	priv = (struct pch_gbe_priv *)memalign(PCH_GBE_ALIGN_SIZE,
-					       sizeof(*priv));
-	if (!priv) {
-		free(dev);
-		return -ENOMEM;
-	}
-	memset(priv, 0, sizeof(*priv));
+	priv = dev_get_priv(dev);
 
-	dev->priv = priv;
-	priv->dev = dev;
 	priv->bdf = devno;
 
 	pci_read_config_dword(devno, PCI_BASE_ADDRESS_1, &iobase);
 	iobase &= PCI_BASE_ADDRESS_MEM_MASK;
 	iobase = pci_mem_to_phys(devno, iobase);
 
-	dev->iobase = iobase;
+	plat->iobase = iobase;
 	priv->mac_regs = (struct pch_gbe_regs *)iobase;
 
-	sprintf(dev->name, "pch_gbe");
-
 	/* Read MAC address from SROM and initialize dev->enetaddr with it */
-	pch_gbe_mac_read(priv->mac_regs, dev->enetaddr);
-
-	dev->init = pch_gbe_init;
-	dev->halt = pch_gbe_halt;
-	dev->send = pch_gbe_send;
-	dev->recv = pch_gbe_recv;
+	pch_gbe_mac_read(priv->mac_regs, plat->enetaddr);
 
-	eth_register(dev);
-
-	priv->interface = PHY_INTERFACE_MODE_RGMII;
+	plat->phy_interface = PHY_INTERFACE_MODE_RGMII;
 	pch_gbe_mdio_init(dev->name, priv->mac_regs);
 	priv->bus = miiphy_get_dev_by_name(dev->name);
 
 	return pch_gbe_phy_init(dev);
 }
+
+static const struct eth_ops pch_gbe_ops = {
+	.start = pch_gbe_start,
+	.send = pch_gbe_send,
+	.recv = pch_gbe_recv,
+	.free_pkt = pch_gbe_free_pkt,
+	.stop = pch_gbe_stop,
+};
+
+static const struct udevice_id pch_gbe_ids[] = {
+	{ .compatible = "intel,pch-gbe" },
+	{ }
+};
+
+U_BOOT_DRIVER(eth_pch_gbe) = {
+	.name = "pch_gbe",
+	.id = UCLASS_ETH,
+	.of_match = pch_gbe_ids,
+	.probe = pch_gbe_probe,
+	.ops = &pch_gbe_ops,
+	.priv_auto_alloc_size = sizeof(struct pch_gbe_priv),
+	.platdata_auto_alloc_size = sizeof(struct eth_pdata),
+	.flags = DM_FLAG_ALLOC_PRIV_DMA,
+};
+
+U_BOOT_PCI_DEVICE(eth_pch_gbe, supported);
diff --git a/drivers/net/pch_gbe.h b/drivers/net/pch_gbe.h
index 11329d4..afcb03d 100644
--- a/drivers/net/pch_gbe.h
+++ b/drivers/net/pch_gbe.h
@@ -287,12 +287,10 @@ struct pch_gbe_priv {
 	struct pch_gbe_rx_desc rx_desc[PCH_GBE_DESC_NUM];
 	struct pch_gbe_tx_desc tx_desc[PCH_GBE_DESC_NUM];
 	char rx_buff[PCH_GBE_DESC_NUM][PCH_GBE_RX_FRAME_LEN];
-	struct eth_device *dev;
 	struct phy_device *phydev;
 	struct mii_dev *bus;
 	struct pch_gbe_regs *mac_regs;
 	pci_dev_t bdf;
-	u32 interface;
 	int rx_idx;
 	int tx_idx;
 };
diff --git a/include/netdev.h b/include/netdev.h
index 662d173..3d5a54f 100644
--- a/include/netdev.h
+++ b/include/netdev.h
@@ -70,7 +70,6 @@ int natsemi_initialize(bd_t *bis);
 int ne2k_register(void);
 int npe_initialize(bd_t *bis);
 int ns8382x_initialize(bd_t *bis);
-int pch_gbe_register(bd_t *bis);
 int pcnet_initialize(bd_t *bis);
 int ppc_4xx_eth_initialize (bd_t *bis);
 int rtl8139_initialize(bd_t *bis);
@@ -123,9 +122,6 @@ static inline int pci_eth_init(bd_t *bis)
 #ifdef CONFIG_E1000
 	num += e1000_initialize(bis);
 #endif
-#ifdef CONFIG_PCH_GBE
-	num += pch_gbe_register(bis);
-#endif
 #ifdef CONFIG_PCNET
 	num += pcnet_initialize(bis);
 #endif
-- 
1.8.2.1

[U-Boot] [PATCH 8/9] net: pch_gbe: Add Kconfig option

[Bin Meng]

Add Kconfig option in preparation for moving board to use Kconfig.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 drivers/net/Kconfig | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 7367d9e..ccaf675 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -79,4 +79,13 @@ config ETH_DESIGNWARE
 	  100Mbit and 1 Gbit operation. You must enable CONFIG_PHYLIB to
 	  provide the PHY (physical media interface).
 
+config PCH_GBE
+	bool "Intel Platform Controller Hub EG20T GMAC driver"
+	depends on DM_PCI
+	default n
+	help
+	  This MAC is present in Intel Platform Controller Hub EG20T. It
+	  supports 10/100/1000 Mbps operation. You must enable CONFIG_PHYLIB
+	  to provide the PHY (physical media interface).
+
 endif # NETDEVICES
-- 
1.8.2.1

[U-Boot] [PATCH 9/9] x86: crownbay: Enable CONFIG_PCH_GBE

[Bin Meng]

Now that we have converted the pch_gbe driver to driver moel,
enable it on Intel Crown Bay board.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 configs/crownbay_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/crownbay_defconfig b/configs/crownbay_defconfig
index f027faf..f328159 100644
--- a/configs/crownbay_defconfig
+++ b/configs/crownbay_defconfig
@@ -21,6 +21,7 @@ CONFIG_DM_PCI=y
 CONFIG_SPI_FLASH=y
 CONFIG_DM_ETH=y
 CONFIG_E1000=y
+CONFIG_PCH_GBE=y
 CONFIG_VIDEO_VESA=y
 CONFIG_FRAMEBUFFER_SET_VESA_MODE=y
 CONFIG_USB=y
-- 
1.8.2.1

[U-Boot] [PATCH 1/9] net: Revert "tftp: adjust settings to be suitable for 100Mbit ethernet"

[Bin Meng]

Hi Joe,

On Tue, Aug 25, 2015 at 3:22 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Testing either pch_gbe or e1000 driver via tftp command on Intel
> Crown Bay board, shows the following failure.
>
>     TFTP error: 'Unsupported option(s) requested' (8)
>
> It turns out commit 620776d causes this. Revert this commit for now.

Please check http://lists.denx.de/pipermail/u-boot/2015-August/225187.html
on why this commit should be reverted.

Let me know if you have different thoughts (eg: I can respin a v2 to
explicitly mention in the commit message that commit 620776d is a spec
violation to RTC 2349 thus we need revert it)

>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  net/tftp.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/net/tftp.c b/net/tftp.c
> index 18ce84c..89be32a 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -19,10 +19,10 @@
>  /* Well known TFTP port # */
>  #define WELL_KNOWN_PORT        69
>  /* Millisecs to timeout for lost pkt */
> -#define TIMEOUT                100UL
> +#define TIMEOUT                5000UL
>  #ifndef        CONFIG_NET_RETRY_COUNT
>  /* # of timeouts before giving up */
> -# define TIMEOUT_COUNT 1000
> +# define TIMEOUT_COUNT 10
>  #else
>  # define TIMEOUT_COUNT  (CONFIG_NET_RETRY_COUNT * 2)
>  #endif
> @@ -711,10 +711,10 @@ void tftp_start(enum proto_t protocol)
>         if (ep != NULL)
>                 timeout_ms = simple_strtol(ep, NULL, 10);
>
> -       if (timeout_ms < 10) {
> -               printf("TFTP timeout (%ld ms) too low, set min = 10 ms\n",
> +       if (timeout_ms < 1000) {
> +               printf("TFTP timeout (%ld ms) too low, set min = 1000 ms\n",
>                        timeout_ms);
> -               timeout_ms = 10;
> +               timeout_ms = 1000;
>         }
>
>         debug("TFTP blocksize = %i, timeout = %ld ms\n",
> --

Regards,
Bin

[U-Boot] [PATCH 1/9] net: Revert "tftp: adjust settings to be suitable for 100Mbit ethernet"

[Joe Hershberger]

Hi Bin,

On Tue, Aug 25, 2015 at 4:26 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Tue, Aug 25, 2015 at 3:22 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Testing either pch_gbe or e1000 driver via tftp command on Intel
>> Crown Bay board, shows the following failure.
>>
>>     TFTP error: 'Unsupported option(s) requested' (8)
>>
>> It turns out commit 620776d causes this. Revert this commit for now.
>
> Please check http://lists.denx.de/pipermail/u-boot/2015-August/225187.html
> on why this commit should be reverted.
>
> Let me know if you have different thoughts (eg: I can respin a v2 to
> explicitly mention in the commit message that commit 620776d is a spec
> violation to RTC 2349 thus we need revert it)

I agree we should revert it for this release. Please send a v2 that
describes as much detail from the RFC as needed and references the RFC
as well. We will revert it and figure out if there is a compliant way
we can improve performance in a high-load situation.

By pushing this to next release we will get much more testing and not
risk losing compatibility in this release.

-Joe

[U-Boot] [PATCH 2/9] net: e1000: Fix build warnings for 32-bit

[Scott Wood]

On Tue, 2015-08-25 at 00:22 -0700, Bin Meng wrote:
> commit 6497e37 "net: e1000: Support 64-bit physical address" causes
> compiler warnings on 32-bit U-Boot build below.
> 
> drivers/net/e1000.c: In function 'e1000_configure_tx':
> drivers/net/e1000.c:4982:2: warning: right shift count >= width of type 
> [enabled by default]
> drivers/net/e1000.c: In function 'e1000_configure_rx':
> drivers/net/e1000.c:5126:2: warning: right shift count >= width of type 
> [enabled by default]
> 
> This commit fixes the build warnings.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>  drivers/net/e1000.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/e1000.c b/drivers/net/e1000.c
> index 6f74d30..a467280 100644
> --- a/drivers/net/e1000.c
> +++ b/drivers/net/e1000.c
> @@ -4977,9 +4977,10 @@ e1000_configure_tx(struct e1000_hw *hw)
>       unsigned long tctl;
>       unsigned long tipg, tarc;
>       uint32_t ipgr1, ipgr2;
> +     uint64_t tdba = (unsigned long)tx_base;
>  
> -     E1000_WRITE_REG(hw, TDBAL, (unsigned long)tx_base & 0xffffffff);
> -     E1000_WRITE_REG(hw, TDBAH, (unsigned long)tx_base >> 32);
> +     E1000_WRITE_REG(hw, TDBAL, (uint32_t)(tdba & 0xffffffff));
> +     E1000_WRITE_REG(hw, TDBAH, (uint32_t)(tdba >> 32));

You could use upper_32_bits()/lower_32_bits().

-Scott

[U-Boot] [PATCH 3/9] dm: eth: Do not call board_eth_init() or cpu_eth_init()

[Joe Hershberger]

Hi Bin,

On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> With driver model, board_eth_init() or cpu_eth_init() is not needed.
> Remove the call to these in eth_common_init().

I'm pretty sure Simon needed this when he ported some allwinner board
originally.

3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing
Ethernet init for driver model)

Cheers,
-Joe

[U-Boot] [PATCH 4/9] dm: eth: Correctly detect alias in eth_get_dev_by_name()

[Joe Hershberger]

Hi Bin,

On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> When given a device name string, we should test if it contains "eth"
> before we treat it as an alias.
>
> With this commit, now we are really able to rotate between network
> interfaces with driver model (previously it was broken).

I believe there is a test for this (dm_test_eth_rotate). Apparently it
is not a sufficient test. Please describe the way in which this
behavior is broken and add a test case to test/dm/eth.c that would
fail without your change.

> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
>  net/eth.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/eth.c b/net/eth.c
> index 0b4b08a..fbf30b0 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -197,7 +197,7 @@ struct udevice *eth_get_dev_by_name(const char *devname)
>         struct uclass *uc;
>
>         /* Must be longer than 3 to be an alias */
> -       if (strlen(devname) > strlen("eth")) {
> +       if (strstr(devname, "eth") && strlen(devname) > strlen("eth")) {
>                 startp = devname + strlen("eth");
>                 seq = simple_strtoul(startp, &endp, 10);
>         }
> --
> 1.8.2.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 5/9] x86: crownbay: Convert to use CONFIG_DM_USB

[Joe Hershberger]

Hi Bin,

On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Move to driver model for USB on Intel Crown Bay.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 6/9] x86: crownbay: Convert to use CONFIG_DM_ETH for E1000

[Joe Hershberger]

Hi Bin,

On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Since E1000 driver has been converted to driver model, enable it
> on Intel Crown Bay. But the Intel Topcliff GbE driver has not been
> converted to driver model yet, disable it for now.

If you reorder your series a bit you can squash this into the last
patch, right? That would be more clear. You can simply move patch 7 &
8 to before this one and you can squash 9 into 6.

> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 7/9] net: pch_gbe: Convert to driver model

[Joe Hershberger]

Hi Bin,

On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> This commit converts pch_gbe ethernet driver to driver model.
>
> Since this driver is only used by Intel Crown Bay board, the
> conversion does not keep the non-dm version.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

This looks great.

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 9/9] x86: crownbay: Enable CONFIG_PCH_GBE

[Joe Hershberger]

Hi Bin,

On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Now that we have converted the pch_gbe driver to driver moel,
> enable it on Intel Crown Bay board.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Looks good if squashed with patch 6.

-Joe

[U-Boot] [PATCH 8/9] net: pch_gbe: Add Kconfig option

[Joe Hershberger]

Hi Bin,

(with list this time)

On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Add Kconfig option in preparation for moving board to use Kconfig.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/net/Kconfig | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 7367d9e..ccaf675 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -79,4 +79,13 @@ config ETH_DESIGNWARE
>           100Mbit and 1 Gbit operation. You must enable CONFIG_PHYLIB to
>           provide the PHY (physical media interface).
>
> +config PCH_GBE
> +       bool "Intel Platform Controller Hub EG20T GMAC driver"
> +       depends on DM_PCI
> +       default n
> +       help
> +         This MAC is present in Intel Platform Controller Hub EG20T. It
> +         supports 10/100/1000 Mbps operation. You must enable CONFIG_PHYLIB
> +         to provide the PHY (physical media interface).

You should not have statements like this in the help. You should
either "select" or "depends on" PHYLIB in this config (I would use
select). Also this depends DM_ETH as well.

> +
>  endif # NETDEVICES
> --
> 1.8.2.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 5/9] x86: crownbay: Convert to use CONFIG_DM_USB

[Simon Glass]

On 25 August 2015 at 01:22, Bin Meng <bmeng.cn@gmail.com> wrote:
> Move to driver model for USB on Intel Crown Bay.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  configs/crownbay_defconfig | 2 ++
>  1 file changed, 2 insertions(+)

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

[U-Boot] [PATCH 7/9] net: pch_gbe: Convert to driver model

[Simon Glass]

On 25 August 2015 at 01:22, Bin Meng <bmeng.cn@gmail.com> wrote:
> This commit converts pch_gbe ethernet driver to driver model.
>
> Since this driver is only used by Intel Crown Bay board, the
> conversion does not keep the non-dm version.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  drivers/net/pch_gbe.c | 133 +++++++++++++++++++++++++++-----------------------
>  drivers/net/pch_gbe.h |   2 -
>  include/netdev.h      |   4 --
>  3 files changed, 73 insertions(+), 66 deletions(-)

Nice!

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

[U-Boot] [PATCH 9/9] x86: crownbay: Enable CONFIG_PCH_GBE

[Simon Glass]

On 25 August 2015 at 01:22, Bin Meng <bmeng.cn@gmail.com> wrote:
> Now that we have converted the pch_gbe driver to driver moel,
> enable it on Intel Crown Bay board.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  configs/crownbay_defconfig | 1 +
>  1 file changed, 1 insertion(+)

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

[U-Boot] [PATCH 8/9] net: pch_gbe: Add Kconfig option

[Bin Meng]

Hi Joe,

On Wed, Aug 26, 2015 at 3:10 AM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> Hi Bin,
>
> (with list this time)
>
> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Add Kconfig option in preparation for moving board to use Kconfig.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  drivers/net/Kconfig | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>> index 7367d9e..ccaf675 100644
>> --- a/drivers/net/Kconfig
>> +++ b/drivers/net/Kconfig
>> @@ -79,4 +79,13 @@ config ETH_DESIGNWARE
>>           100Mbit and 1 Gbit operation. You must enable CONFIG_PHYLIB to
>>           provide the PHY (physical media interface).
>>
>> +config PCH_GBE
>> +       bool "Intel Platform Controller Hub EG20T GMAC driver"
>> +       depends on DM_PCI
>> +       default n
>> +       help
>> +         This MAC is present in Intel Platform Controller Hub EG20T. It
>> +         supports 10/100/1000 Mbps operation. You must enable CONFIG_PHYLIB
>> +         to provide the PHY (physical media interface).
>
> You should not have statements like this in the help. You should
> either "select" or "depends on" PHYLIB in this config (I would use
> select). Also this depends DM_ETH as well.
>

Yep, looks that I need prepare a new patch to convert PHYLIB to a
Kconfig option first. As for dependency on DM_ETH, I think this is
already indicated by NETDEVICES which is default y if DM_ETH.

Regards,
Bin

[U-Boot] [PATCH 3/9] dm: eth: Do not call board_eth_init() or cpu_eth_init()

[Bin Meng]

Hi Joe,

On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> Hi Bin,
>
> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> With driver model, board_eth_init() or cpu_eth_init() is not needed.
>> Remove the call to these in eth_common_init().
>
> I'm pretty sure Simon needed this when he ported some allwinner board
> originally.
>
> 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing
> Ethernet init for driver model)
>

I think my patch does not break Simon's. My patch only comments out
the call to board_eth_init() or cpu_eth_init() which is not needed for
driver model. Other stuff in eth_common_init() is still there. In
fact, my patch series also needs phy_init() call (required by pch_gbe
driver).

Regards,
Bin

[U-Boot] [PATCH 3/9] dm: eth: Do not call board_eth_init() or cpu_eth_init()

[Bin Meng]

Hi Simon,

On Wed, Aug 26, 2015 at 9:29 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Bin,
>>
>> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> With driver model, board_eth_init() or cpu_eth_init() is not needed.
>>> Remove the call to these in eth_common_init().
>>
>> I'm pretty sure Simon needed this when he ported some allwinner board
>> originally.
>>
>> 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing
>> Ethernet init for driver model)
>>
>
> I think my patch does not break Simon's. My patch only comments out
> the call to board_eth_init() or cpu_eth_init() which is not needed for
> driver model. Other stuff in eth_common_init() is still there. In
> fact, my patch series also needs phy_init() call (required by pch_gbe
> driver).
>

Would you confirm this does not break your boards?

Regards,
Bin

[U-Boot] [PATCH 8/9] net: pch_gbe: Add Kconfig option

[Joe Hershberger]

Hi Bin,

On Tue, Aug 25, 2015 at 8:25 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Wed, Aug 26, 2015 at 3:10 AM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Bin,
>>
>> (with list this time)
>>
>> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Add Kconfig option in preparation for moving board to use Kconfig.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>>
>>>  drivers/net/Kconfig | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>>> index 7367d9e..ccaf675 100644
>>> --- a/drivers/net/Kconfig
>>> +++ b/drivers/net/Kconfig
>>> @@ -79,4 +79,13 @@ config ETH_DESIGNWARE
>>>           100Mbit and 1 Gbit operation. You must enable CONFIG_PHYLIB to
>>>           provide the PHY (physical media interface).
>>>
>>> +config PCH_GBE
>>> +       bool "Intel Platform Controller Hub EG20T GMAC driver"
>>> +       depends on DM_PCI
>>> +       default n
>>> +       help
>>> +         This MAC is present in Intel Platform Controller Hub EG20T. It
>>> +         supports 10/100/1000 Mbps operation. You must enable CONFIG_PHYLIB
>>> +         to provide the PHY (physical media interface).
>>
>> You should not have statements like this in the help. You should
>> either "select" or "depends on" PHYLIB in this config (I would use
>> select). Also this depends DM_ETH as well.
>>
>
> Yep, looks that I need prepare a new patch to convert PHYLIB to a
> Kconfig option first. As for dependency on DM_ETH, I think this is
> already indicated by NETDEVICES which is default y if DM_ETH.

It's not the same thing since NETDEVICES is only default y if
DM_ETH... NETDEVICES does not select DM_ETH.

-Joe

[U-Boot] [PATCH 6/9] x86: crownbay: Convert to use CONFIG_DM_ETH for E1000

[Bin Meng]

Hi Joe,

On Wed, Aug 26, 2015 at 2:59 AM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> Hi Bin,
>
> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Since E1000 driver has been converted to driver model, enable it
>> on Intel Crown Bay. But the Intel Topcliff GbE driver has not been
>> converted to driver model yet, disable it for now.
>
> If you reorder your series a bit you can squash this into the last
> patch, right? That would be more clear. You can simply move patch 7 &
> 8 to before this one and you can squash 9 into 6.

I cannot squash this into the last patch because when patch 7 comes,
it breaks Intel Crown Bay board build as non-dm version is gone.
That's why you see in this patch when I turned on CONFIG_DM_ETH, I
also disabled CONFIG_PCH_GBE (non-dm driver). Then after patch 7
converts the pch_gbe driver to dm, I added that driver back in patch
9. This way it passes buildman testing without breaking bisectability.

But if you think we can break such kind of bisectability, I can
reorder these patches.

>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Regards,
Bin

[U-Boot] [PATCH 3/9] dm: eth: Do not call board_eth_init() or cpu_eth_init()

[Joe Hershberger]

Hi Bin,

On Tue, Aug 25, 2015 at 8:29 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Bin,
>>
>> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> With driver model, board_eth_init() or cpu_eth_init() is not needed.
>>> Remove the call to these in eth_common_init().
>>
>> I'm pretty sure Simon needed this when he ported some allwinner board
>> originally.
>>
>> 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing
>> Ethernet init for driver model)
>>
>
> I think my patch does not break Simon's. My patch only comments out
> the call to board_eth_init() or cpu_eth_init() which is not needed for
> driver model. Other stuff in eth_common_init() is still there. In
> fact, my patch series also needs phy_init() call (required by pch_gbe
> driver).

Even if it doesn't break Simon's board, why remove the ability for a
board to add eth_init code. You're trying to say that there is no case
where a DM board would need to init anything related to eth. That
seems unlikely.

Also, why is it hurting your board to have an optional call to such a
function. Presumably you just don't define those functions and you're
fine, right?

I guess it can just be put back when such a board is converted.

-Joe

[U-Boot] [PATCH 6/9] x86: crownbay: Convert to use CONFIG_DM_ETH for E1000

[Simon Glass]

Hi,

On 25 August 2015 at 19:20, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Wed, Aug 26, 2015 at 2:59 AM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Bin,
>>
>> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Since E1000 driver has been converted to driver model, enable it
>>> on Intel Crown Bay. But the Intel Topcliff GbE driver has not been
>>> converted to driver model yet, disable it for now.
>>
>> If you reorder your series a bit you can squash this into the last
>> patch, right? That would be more clear. You can simply move patch 7 &
>> 8 to before this one and you can squash 9 into 6.
>
> I cannot squash this into the last patch because when patch 7 comes,
> it breaks Intel Crown Bay board build as non-dm version is gone.
> That's why you see in this patch when I turned on CONFIG_DM_ETH, I
> also disabled CONFIG_PCH_GBE (non-dm driver). Then after patch 7
> converts the pch_gbe driver to dm, I added that driver back in patch
> 9. This way it passes buildman testing without breaking bisectability.
>
> But if you think we can break such kind of bisectability, I can
> reorder these patches.

No, we should keep bisectability. It's just such a pain chasing down
regressions otherwise.

>
>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> Regards,
> Bin

Regards,
Simon

[U-Boot] [PATCH 6/9] x86: crownbay: Convert to use CONFIG_DM_ETH for E1000

[Joe Hershberger]

On Tue, Aug 25, 2015 at 9:28 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On 25 August 2015 at 19:20, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Joe,
>>
>> On Wed, Aug 26, 2015 at 2:59 AM, Joe Hershberger
>> <joe.hershberger@gmail.com> wrote:
>>> Hi Bin,
>>>
>>> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> Since E1000 driver has been converted to driver model, enable it
>>>> on Intel Crown Bay. But the Intel Topcliff GbE driver has not been
>>>> converted to driver model yet, disable it for now.
>>>
>>> If you reorder your series a bit you can squash this into the last
>>> patch, right? That would be more clear. You can simply move patch 7 &
>>> 8 to before this one and you can squash 9 into 6.
>>
>> I cannot squash this into the last patch because when patch 7 comes,
>> it breaks Intel Crown Bay board build as non-dm version is gone.
>> That's why you see in this patch when I turned on CONFIG_DM_ETH, I
>> also disabled CONFIG_PCH_GBE (non-dm driver). Then after patch 7
>> converts the pch_gbe driver to dm, I added that driver back in patch
>> 9. This way it passes buildman testing without breaking bisectability.
>>
>> But if you think we can break such kind of bisectability, I can
>> reorder these patches.
>
> No, we should keep bisectability. It's just such a pain chasing down
> regressions otherwise.

Yep

[U-Boot] [PATCH 3/9] dm: eth: Do not call board_eth_init() or cpu_eth_init()

[Bin Meng]

Hi Joe,

On Wed, Aug 26, 2015 at 10:24 AM, Joe Hershberger
<joe.hershberger@gmail.com> wrote:
> Hi Bin,
>
> On Tue, Aug 25, 2015 at 8:29 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Joe,
>>
>> On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger
>> <joe.hershberger@gmail.com> wrote:
>>> Hi Bin,
>>>
>>> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> With driver model, board_eth_init() or cpu_eth_init() is not needed.
>>>> Remove the call to these in eth_common_init().
>>>
>>> I'm pretty sure Simon needed this when he ported some allwinner board
>>> originally.
>>>
>>> 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing
>>> Ethernet init for driver model)
>>>
>>
>> I think my patch does not break Simon's. My patch only comments out
>> the call to board_eth_init() or cpu_eth_init() which is not needed for
>> driver model. Other stuff in eth_common_init() is still there. In
>> fact, my patch series also needs phy_init() call (required by pch_gbe
>> driver).
>
> Even if it doesn't break Simon's board, why remove the ability for a
> board to add eth_init code. You're trying to say that there is no case
> where a DM board would need to init anything related to eth. That
> seems unlikely.
>

I believe with driver model, we should avoid these calls. All the
drive initialization needs to be done in the bind or probe phase, and
called by driver model automatically. Like pci_eth_init() which just
calls non-dm ethernet drivers, and pci_eth_init() can be called from
board_eth_init() or cpu_eth_init(). But I think you are right, there
might be some board-specific thing to be put there, even right now it
does not break any board. But that's maybe we don't have proper driver
model uclass to handle these misc things?

> Also, why is it hurting your board to have an optional call to such a
> function. Presumably you just don't define those functions and you're
> fine, right?
>

It does not hurt but I think at least we should comment out the
following printf for DM.

    } else {
        printf("Net Initialization Skipped\n");
    }

This is misleading message.

> I guess it can just be put back when such a board is converted.
>

Regards,
Bin

[U-Boot] [PATCH 3/9] dm: eth: Do not call board_eth_init() or cpu_eth_init()

[Joe Hershberger]

Hi Bin,

On Tue, Aug 25, 2015 at 9:36 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Wed, Aug 26, 2015 at 10:24 AM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Bin,
>>
>> On Tue, Aug 25, 2015 at 8:29 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Joe,
>>>
>>> On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger
>>> <joe.hershberger@gmail.com> wrote:
>>>> Hi Bin,
>>>>
>>>> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> With driver model, board_eth_init() or cpu_eth_init() is not needed.
>>>>> Remove the call to these in eth_common_init().
>>>>
>>>> I'm pretty sure Simon needed this when he ported some allwinner board
>>>> originally.
>>>>
>>>> 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing
>>>> Ethernet init for driver model)
>>>>
>>>
>>> I think my patch does not break Simon's. My patch only comments out
>>> the call to board_eth_init() or cpu_eth_init() which is not needed for
>>> driver model. Other stuff in eth_common_init() is still there. In
>>> fact, my patch series also needs phy_init() call (required by pch_gbe
>>> driver).
>>
>> Even if it doesn't break Simon's board, why remove the ability for a
>> board to add eth_init code. You're trying to say that there is no case
>> where a DM board would need to init anything related to eth. That
>> seems unlikely.
>>
>
> I believe with driver model, we should avoid these calls. All the
> drive initialization needs to be done in the bind or probe phase, and
> called by driver model automatically. Like pci_eth_init() which just
> calls non-dm ethernet drivers, and pci_eth_init() can be called from
> board_eth_init() or cpu_eth_init().

I agree we shouldn't use them if it makes sense not to.

> But I think you are right, there
> might be some board-specific thing to be put there, even right now it
> does not break any board. But that's maybe we don't have proper driver
> model uclass to handle these misc things?

I think we can evaluate that for each case. We don't necessarily want
the uclass to be a union of all crazy board features.

>> Also, why is it hurting your board to have an optional call to such a
>> function. Presumably you just don't define those functions and you're
>> fine, right?
>>
>
> It does not hurt but I think at least we should comment out the
> following printf for DM.
>
>     } else {
>         printf("Net Initialization Skipped\n");
>     }
>
> This is misleading message.

I agree.

>> I guess it can just be put back when such a board is converted.
>>
>
> Regards,
> Bin

[U-Boot] [PATCH 3/9] dm: eth: Do not call board_eth_init() or cpu_eth_init()

[Simon Glass]

Hi,

On 25 August 2015 at 19:36, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Joe,
>
> On Wed, Aug 26, 2015 at 10:24 AM, Joe Hershberger
> <joe.hershberger@gmail.com> wrote:
>> Hi Bin,
>>
>> On Tue, Aug 25, 2015 at 8:29 PM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Joe,
>>>
>>> On Wed, Aug 26, 2015 at 2:43 AM, Joe Hershberger
>>> <joe.hershberger@gmail.com> wrote:
>>>> Hi Bin,
>>>>
>>>> On Tue, Aug 25, 2015 at 2:22 AM, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> With driver model, board_eth_init() or cpu_eth_init() is not needed.
>>>>> Remove the call to these in eth_common_init().
>>>>
>>>> I'm pretty sure Simon needed this when he ported some allwinner board
>>>> originally.
>>>>
>>>> 3bc427006ac8d0661169ed771b3cac7e86f960e8 (dm: net: Use existing
>>>> Ethernet init for driver model)
>>>>
>>>
>>> I think my patch does not break Simon's. My patch only comments out
>>> the call to board_eth_init() or cpu_eth_init() which is not needed for
>>> driver model. Other stuff in eth_common_init() is still there. In
>>> fact, my patch series also needs phy_init() call (required by pch_gbe
>>> driver).
>>
>> Even if it doesn't break Simon's board, why remove the ability for a
>> board to add eth_init code. You're trying to say that there is no case
>> where a DM board would need to init anything related to eth. That
>> seems unlikely.
>>
>
> I believe with driver model, we should avoid these calls. All the
> drive initialization needs to be done in the bind or probe phase, and
> called by driver model automatically. Like pci_eth_init() which just
> calls non-dm ethernet drivers, and pci_eth_init() can be called from
> board_eth_init() or cpu_eth_init(). But I think you are right, there
> might be some board-specific thing to be put there, even right now it
> does not break any board. But that's maybe we don't have proper driver
> model uclass to handle these misc things?

Normally the board init should happen in board_init(). We don't
currently have a good way to handle 'associated init' for drivers.

For example, if an Ethernet port needs some pinmux settings, or some
clock settings, with driver model we currently have to init this in
board_init() so it is not 'lazy init', it always happens at start of
day.

I don't actually think this is a problem in practice, but it is not in
the spirit of driver model. Drivers should 'pull in' the init that
they need and we should make that work automatically.

But it is very important the we do not put this init into the drivers
themselves. The drivers need to be generic. E.g. an Ethernet driver
should be able to operate on any board that has that Ethernet
controller. The setup to make the controller available on pins, enable
it clock it correctly should not be in driver code.

Soon we will have pinctrl which will deal with the pinmux problem. We
already have a clock framework would could handle clocking. So
progress is being made.

I wonder whether we will end up wanting a way to request that the
resources for a driver be activated, in a general sense. But until we
have a use case I'm not keen to spend much time thinking about it.

>
>> Also, why is it hurting your board to have an optional call to such a
>> function. Presumably you just don't define those functions and you're
>> fine, right?
>>
>
> It does not hurt but I think at least we should comment out the
> following printf for DM.
>
>     } else {
>         printf("Net Initialization Skipped\n");
>     }
>
> This is misleading message.
>
>> I guess it can just be put back when such a board is converted.
>>
>
> Regards,
> Bin

Regards,
Simon