[U-Boot] [PATCH] net: davinci_emac: convert to using the driver model
Adam Ford
aford173 at gmail.com
Mon Jun 3 13:02:59 UTC 2019
On Mon, Jun 3, 2019 at 3:12 AM Bartosz Golaszewski <brgl at bgdev.pl> wrote:
>
> sob., 1 cze 2019 o 05:24 Adam Ford <aford173 at gmail.com> napisał(a):
> >
> > On Fri, May 31, 2019 at 8:32 AM Bartosz Golaszewski <brgl at bgdev.pl> wrote:
> > >
> > > From: Bartosz Golaszewski <bgolaszewski at baylibre.com>
> > >
> > > Now that we removed all legacy boards selecting TI_EMAC we can
> > > completely convert the driver code to using the driver model.
> > > This patch also updates all remaining users of davinci_emac.
> > >
> >
> > I took a break from this to come back, and I'm going to give some
> > feedback about how the driver was written. I still do not know why I
> > cannot get an IP address with this patch on the AM3517-evm.
> >
>
> Hi Adam,
>
> thanks for all the testing. Unfortunately I can only test with
> da850-evm and da850-lcdk.
>
> I was wondering if it is possible that the problem is caused by
> cpu_eth_init() from ./arch/arm/mach-omap2/omap3/emac. not being called
> with CONFIG_DM_ETH? The comments say that it brings the module out of
> reset, so maybe this is what causes the problem you're seeing?
I looked into that nearly right away, but there is a chunk of code in
board/logicpd/am3517evm/am3517evm.c which has a function called
misc_init_r() which does the same thing. Looking through the debug
data, it appears as if the drive is communicating for a bit, but at
some point it dies. I'm not going to be able to look at it for a few
days. Is there any way you can submit a V2 to use #ifdef's to allow
users to use the same driver with and without DM_ETH? That would give
the da850/L138 boards the DM_ETH, and give me some time to
troubleshoot the am3517-evm. I know Sekhar works for TI and the AM3517
was the official development kit for TI back in the day. If TI has
some ideas, I'm open to trying them as well when I can get back to it.
adam
>
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski at baylibre.com>
> > > ---
> > > arch/arm/mach-davinci/cpu.c | 13 -----
> > > arch/arm/mach-omap2/omap3/emac.c | 3 +-
> > > board/davinci/da8xxevm/da850evm.c | 6 --
> > > board/davinci/da8xxevm/omapl138_lcdk.c | 14 -----
> > > board/logicpd/am3517evm/am3517evm.c | 1 -
> > > board/ti/ti816x/evm.c | 3 +-
> > > configs/am3517_evm_defconfig | 1 +
> > > configs/da850evm_defconfig | 1 +
> > > configs/da850evm_direct_nor_defconfig | 1 +
> > > configs/da850evm_nand_defconfig | 1 +
> > > configs/omapl138_lcdk_defconfig | 1 +
> > > configs/ti816x_evm_defconfig | 1 +
> > > drivers/net/ti/davinci_emac.c | 77 ++++++++++++++------------
> > > include/netdev.h | 1 -
> > > 14 files changed, 51 insertions(+), 73 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-davinci/cpu.c b/arch/arm/mach-davinci/cpu.c
> > > index f97ad3fc74..9fd6564d04 100644
> > > --- a/arch/arm/mach-davinci/cpu.c
> > > +++ b/arch/arm/mach-davinci/cpu.c
> > > @@ -5,7 +5,6 @@
> > > */
> > >
> > > #include <common.h>
> > > -#include <netdev.h>
> > > #include <asm/arch/hardware.h>
> > > #include <asm/io.h>
> > >
> > > @@ -90,15 +89,3 @@ int set_cpu_clk_info(void)
> > > gd->bd->bi_dsp_freq = 0;
> > > return 0;
> > > }
> > > -
> > > -/*
> > > - * Initializes on-chip ethernet controllers.
> > > - * to override, implement board_eth_init()
> > > - */
> > > -int cpu_eth_init(bd_t *bis)
> > > -{
> > > -#if defined(CONFIG_DRIVER_TI_EMAC)
> > > - davinci_emac_initialize();
> > > -#endif
> > > - return 0;
> > > -}
> > > diff --git a/arch/arm/mach-omap2/omap3/emac.c b/arch/arm/mach-omap2/omap3/emac.c
> > > index c79e870183..fb0c9188f5 100644
> > > --- a/arch/arm/mach-omap2/omap3/emac.c
> > > +++ b/arch/arm/mach-omap2/omap3/emac.c
> > > @@ -7,7 +7,6 @@
> > > */
> > >
> > > #include <common.h>
> > > -#include <netdev.h>
> > > #include <asm/io.h>
> > > #include <asm/arch/am35x_def.h>
> > >
> > > @@ -24,5 +23,5 @@ int cpu_eth_init(bd_t *bis)
> > > reset &= ~CPGMACSS_SW_RST;
> > > writel(reset, &am35x_scm_general_regs->ip_sw_reset);
> > >
> > > - return davinci_emac_initialize();
> > > + return 0;
> > > }
> > > diff --git a/board/davinci/da8xxevm/da850evm.c b/board/davinci/da8xxevm/da850evm.c
> > > index 1bc26828bf..d090c00466 100644
> > > --- a/board/davinci/da8xxevm/da850evm.c
> > > +++ b/board/davinci/da8xxevm/da850evm.c
> > > @@ -13,7 +13,6 @@
> > > #include <environment.h>
> > > #include <i2c.h>
> > > #include <net.h>
> > > -#include <netdev.h>
> > > #include <spi.h>
> > > #include <spi_flash.h>
> > > #include <asm/arch/hardware.h>
> > > @@ -472,11 +471,6 @@ int board_eth_init(bd_t *bis)
> > > if (rmii_hw_init())
> > > printf("RMII hardware init failed!!!\n");
> > > #endif
> > > - if (!davinci_emac_initialize()) {
> > > - printf("Error: Ethernet init failed!\n");
> > > - return -1;
> > > - }
> > > -
> > > return 0;
> > > }
> > > #endif /* CONFIG_DRIVER_TI_EMAC */
> > > diff --git a/board/davinci/da8xxevm/omapl138_lcdk.c b/board/davinci/da8xxevm/omapl138_lcdk.c
> > > index 2c2f885d43..ef9656add8 100644
> > > --- a/board/davinci/da8xxevm/omapl138_lcdk.c
> > > +++ b/board/davinci/da8xxevm/omapl138_lcdk.c
> > > @@ -11,7 +11,6 @@
> > > #include <common.h>
> > > #include <i2c.h>
> > > #include <net.h>
> > > -#include <netdev.h>
> > > #include <spi.h>
> > > #include <spi_flash.h>
> > > #include <asm/arch/hardware.h>
> > > @@ -229,19 +228,6 @@ int board_init(void)
> > >
> > > #ifdef CONFIG_DRIVER_TI_EMAC
> > >
> > > -/*
> > > - * Initializes on-board ethernet controllers.
> > > - */
> > > -int board_eth_init(bd_t *bis)
> > > -{
> > > - if (!davinci_emac_initialize()) {
> > > - printf("Error: Ethernet init failed!\n");
> > > - return -1;
> > > - }
> > > -
> > > - return 0;
> > > -}
> > > -
> > > #endif /* CONFIG_DRIVER_TI_EMAC */
> > >
> > > #define CFG_MAC_ADDR_SPI_BUS 0
> > > diff --git a/board/logicpd/am3517evm/am3517evm.c b/board/logicpd/am3517evm/am3517evm.c
> > > index 10031a4801..bfd4e78274 100644
> > > --- a/board/logicpd/am3517evm/am3517evm.c
> > > +++ b/board/logicpd/am3517evm/am3517evm.c
> > > @@ -28,7 +28,6 @@
> > > #include <linux/usb/gadget.h>
> > > #include <linux/usb/musb.h>
> > > #include <i2c.h>
> > > -#include <netdev.h>
> > > #include "am3517evm.h"
> > >
> > > DECLARE_GLOBAL_DATA_PTR;
> > > diff --git a/board/ti/ti816x/evm.c b/board/ti/ti816x/evm.c
> > > index 07a084bab8..240df8cbe1 100644
> > > --- a/board/ti/ti816x/evm.c
> > > +++ b/board/ti/ti816x/evm.c
> > > @@ -9,7 +9,6 @@
> > > #include <common.h>
> > > #include <environment.h>
> > > #include <spl.h>
> > > -#include <netdev.h>
> > > #include <asm/cache.h>
> > > #include <asm/io.h>
> > > #include <asm/arch/clock.h>
> > > @@ -56,7 +55,7 @@ int board_eth_init(bd_t *bis)
> > > printf("Unable to read MAC address. Set <ethaddr>\n");
> > > }
> > >
> > > - return davinci_emac_initialize();
> > > + return 0;
> > > }
> > >
> > > #ifdef CONFIG_SPL_BUILD
> > > diff --git a/configs/am3517_evm_defconfig b/configs/am3517_evm_defconfig
> > > index b9f59f3291..5cb76322df 100644
> > > --- a/configs/am3517_evm_defconfig
> > > +++ b/configs/am3517_evm_defconfig
> > > @@ -44,6 +44,7 @@ CONFIG_SYS_NAND_BUSWIDTH_16BIT=y
> > > CONFIG_SYS_NAND_U_BOOT_LOCATIONS=y
> > > CONFIG_SYS_NAND_U_BOOT_OFFS=0x80000
> > > CONFIG_SPL_NAND_SIMPLE=y
> > > +CONFIG_DM_ETH=y
> > > CONFIG_MII=y
> > > CONFIG_DRIVER_TI_EMAC=y
> > > CONFIG_PINCTRL=y
> > > diff --git a/configs/da850evm_defconfig b/configs/da850evm_defconfig
> > > index 8c16d5c4f5..ca304ed78a 100644
> > > --- a/configs/da850evm_defconfig
> > > +++ b/configs/da850evm_defconfig
> > > @@ -60,6 +60,7 @@ CONFIG_SF_DEFAULT_SPEED=30000000
> > > CONFIG_SPI_FLASH_STMICRO=y
> > > CONFIG_SPI_FLASH_WINBOND=y
> > > CONFIG_SPI_FLASH_MTD=y
> > > +CONFIG_DM_ETH=y
> > > CONFIG_MII=y
> > > CONFIG_DRIVER_TI_EMAC=y
> > > CONFIG_PINCTRL=y
> > > diff --git a/configs/da850evm_direct_nor_defconfig b/configs/da850evm_direct_nor_defconfig
> > > index 166e77b8e3..9b1da07384 100644
> > > --- a/configs/da850evm_direct_nor_defconfig
> > > +++ b/configs/da850evm_direct_nor_defconfig
> > > @@ -50,6 +50,7 @@ CONFIG_DM_SPI_FLASH=y
> > > CONFIG_SPI_FLASH=y
> > > CONFIG_SPI_FLASH_STMICRO=y
> > > CONFIG_SPI_FLASH_WINBOND=y
> > > +CONFIG_DM_ETH=y
> > > CONFIG_MII=y
> > > CONFIG_DRIVER_TI_EMAC=y
> > > CONFIG_PINCTRL=y
> > > diff --git a/configs/da850evm_nand_defconfig b/configs/da850evm_nand_defconfig
> > > index b8eac0e659..eaf5d73dd8 100644
> > > --- a/configs/da850evm_nand_defconfig
> > > +++ b/configs/da850evm_nand_defconfig
> > > @@ -60,6 +60,7 @@ CONFIG_SPI_FLASH=y
> > > CONFIG_SPI_FLASH_STMICRO=y
> > > CONFIG_SPI_FLASH_WINBOND=y
> > > CONFIG_SPI_FLASH_MTD=y
> > > +CONFIG_DM_ETH=y
> > > CONFIG_PINCTRL=y
> > > CONFIG_PINCTRL_SINGLE=y
> > > CONFIG_DM_SERIAL=y
> > > diff --git a/configs/omapl138_lcdk_defconfig b/configs/omapl138_lcdk_defconfig
> > > index e43141844a..f93a06083e 100644
> > > --- a/configs/omapl138_lcdk_defconfig
> > > +++ b/configs/omapl138_lcdk_defconfig
> > > @@ -50,6 +50,7 @@ CONFIG_SPI_FLASH=y
> > > CONFIG_SF_DEFAULT_SPEED=30000000
> > > CONFIG_SPI_FLASH_STMICRO=y
> > > CONFIG_SPI_FLASH_WINBOND=y
> > > +CONFIG_DM_ETH=y
> > > CONFIG_MII=y
> > > CONFIG_DRIVER_TI_EMAC=y
> > > CONFIG_DM_SERIAL=y
> > > diff --git a/configs/ti816x_evm_defconfig b/configs/ti816x_evm_defconfig
> > > index bf877f596b..b233ab8cc9 100644
> > > --- a/configs/ti816x_evm_defconfig
> > > +++ b/configs/ti816x_evm_defconfig
> > > @@ -45,6 +45,7 @@ CONFIG_SYS_I2C_OMAP24XX=y
> > > CONFIG_MMC_OMAP_HS=y
> > > CONFIG_NAND=y
> > > CONFIG_SYS_NAND_BUSWIDTH_16BIT=y
> > > +CONFIG_DM_ETH=y
> > > CONFIG_MII=y
> > > CONFIG_DRIVER_TI_EMAC=y
> > > CONFIG_SYS_NS16550=y
> > > diff --git a/drivers/net/ti/davinci_emac.c b/drivers/net/ti/davinci_emac.c
> > > index 9d53984973..4b0a98a4aa 100644
> > > --- a/drivers/net/ti/davinci_emac.c
> > > +++ b/drivers/net/ti/davinci_emac.c
> > > @@ -26,7 +26,6 @@
> > > #include <net.h>
> > > #include <miiphy.h>
> > > #include <malloc.h>
> > > -#include <netdev.h>
> > > #include <linux/compiler.h>
> > > #include <asm/arch/emac_defs.h>
> > > #include <asm/io.h>
> > > @@ -107,8 +106,9 @@ static u_int8_t num_phy;
> > >
> > > phy_t phy[CONFIG_SYS_DAVINCI_EMAC_PHY_COUNT];
> > >
> > > -static int davinci_eth_set_mac_addr(struct eth_device *dev)
> > > +static int davinci_emac_write_hwaddr(struct udevice *dev)
> > > {
> > > + struct eth_pdata *pdata = dev_get_platdata(dev);
> > > unsigned long mac_hi;
> > > unsigned long mac_lo;
> > >
> > > @@ -118,12 +118,12 @@ static int davinci_eth_set_mac_addr(struct eth_device *dev)
> > > * Using channel 0 only - other channels are disabled
> > > * */
> > > writel(0, &adap_emac->MACINDEX);
> > > - mac_hi = (dev->enetaddr[3] << 24) |
> > > - (dev->enetaddr[2] << 16) |
> > > - (dev->enetaddr[1] << 8) |
> > > - (dev->enetaddr[0]);
> > > - mac_lo = (dev->enetaddr[5] << 8) |
> > > - (dev->enetaddr[4]);
> > > + mac_hi = (pdata->enetaddr[3] << 24) |
> > > + (pdata->enetaddr[2] << 16) |
> > > + (pdata->enetaddr[1] << 8) |
> > > + (pdata->enetaddr[0]);
> > > + mac_lo = (pdata->enetaddr[5] << 8) |
> > > + (pdata->enetaddr[4]);
> > >
> > > writel(mac_hi, &adap_emac->MACADDRHI);
> > > #if defined(DAVINCI_EMAC_VERSION2)
> > > @@ -411,7 +411,7 @@ static void __attribute__((unused)) davinci_eth_gigabit_enable(int phy_addr)
> > > }
> > >
> > > /* Eth device open */
> > > -static int davinci_eth_open(struct eth_device *dev, bd_t *bis)
> > > +static int davinci_emac_start(struct udevice *dev)
> > > {
> > > dv_reg_p addr;
> > > u_int32_t clkdiv, cnt, mac_control;
> > > @@ -447,7 +447,7 @@ static int davinci_eth_open(struct eth_device *dev, bd_t *bis)
> > > writel(1, &adap_emac->TXCONTROL);
> > > writel(1, &adap_emac->RXCONTROL);
> > >
> > > - davinci_eth_set_mac_addr(dev);
> > > + davinci_emac_write_hwaddr(dev);
> > >
> > > /* Set DMA 8 TX / 8 RX Head pointers to 0 */
> > > addr = &adap_emac->TX0HDP;
> > > @@ -588,7 +588,7 @@ static void davinci_eth_ch_teardown(int ch)
> > > }
> > >
> > > /* Eth device close */
> > > -static void davinci_eth_close(struct eth_device *dev)
> > > +static void davinci_emac_stop(struct udevice *dev)
> > > {
> > > debug_emac("+ emac_close\n");
> > >
> > > @@ -619,8 +619,8 @@ static int tx_send_loop = 0;
> > > * This function sends a single packet on the network and returns
> > > * positive number (number of bytes transmitted) or negative for error
> > > */
> > > -static int davinci_eth_send_packet (struct eth_device *dev,
> > > - void *packet, int length)
> > > +static int davinci_emac_send(struct udevice *dev,
> > > + void *packet, int length)
> > > {
> > > int ret_status = -1;
> > > int index;
> > > @@ -672,7 +672,7 @@ static int davinci_eth_send_packet (struct eth_device *dev,
> > > /*
> > > * This function handles receipt of a packet from the network
> > > */
> > > -static int davinci_eth_rcv_packet (struct eth_device *dev)
> > > +static int davinci_emac_recv(struct udevice *dev, int flags, uchar **packetp)
> > > {
> > > volatile emac_desc *rx_curr_desc;
> > > volatile emac_desc *curr_desc;
> > > @@ -742,6 +742,9 @@ static int davinci_eth_rcv_packet (struct eth_device *dev)
> > > }
> > > return (ret);
> > > }
> > > +
> > > + *packetp = rx_curr_desc->buffer;
> > > +
> > > return (0);
> > > }
> > >
> > > @@ -750,30 +753,12 @@ static int davinci_eth_rcv_packet (struct eth_device *dev)
> > > * EMAC modules power or pin multiplexors, that is done by board_init()
> > > * much earlier in bootup process. Returns 1 on success, 0 otherwise.
> > > */
> > > -int davinci_emac_initialize(void)
> > > +static int davinci_emac_probe(struct udevice *dev)
> >
> > From the device tree, we should be able to remove the hard-coded
> > values referencing:
> >
> > EMAC_BASE_ADDR;
> > EMAC_WRAPPER_BASE_ADDR;
> >
> > /* EMAC descriptors */
> > EMAC_WRAPPER_RAM_ADDR + EMAC_RX_DESC_BASE);
> > EMAC_WRAPPER_RAM_ADDR + EMAC_TX_DESC_BASE);
> >
> > From the device tree, EMAC_BASE_ADDR should point to the address of
> > the emac node. In the case of the AM3517, it would be 5c000000 and
> > this is how the name appears, so we should be able to use that.
> > EMAC_WRAPPER_BASE_ADDR should the also read by the device tree as an
> > offset from the base address by reading "ti,davinci-ctrl-reg-offset"
> > and "ti,davinci-ctrl-mod-reg-offset" in the case of AM3517, 0x5c000000
> > + 0x10000 + 0 give the same correct address as the
> > EMAC_WRAPPER_BASE_ADDR currently does.
> >
> > The EMAC_WRAPPER_RAM_ADDR is read by reading the
> > "ti,davinci-ctrl-ram-offset" which for am3517 is 0x20000 and 5c000000
> > + 0x20000 matches the value currenty set for 0x5C020000.
> >
> > I may have some of the math wrong a bit, but the linux driver doc
> > explains how these tree entries are defined, and I would think the
> > emac driver could be modeled after the Linux implementation.
> >
> > I think we should try to extract these and calculator the addresses
> > this way we can remove the hard-coded values and make the driver more
> > generic.
>
> Definitely and it's on my list, but first I wanted to do the
> conversion, make sure it works and then gradually improve the code.
>
> Best regards,
> Bartosz Golaszewski
>
> > I am going to bed now, but I'll try to beat on the da850 on Sunday.
> >
> > adam
> > > {
> > > u_int32_t phy_id;
> > > u_int16_t tmp;
> > > int i;
> > > int ret;
> > > - struct eth_device *dev;
> > > -
> > > - dev = malloc(sizeof *dev);
> > > -
> > > - if (dev == NULL)
> > > - return -1;
> > > -
> > > - memset(dev, 0, sizeof *dev);
> > > - strcpy(dev->name, "DaVinci-EMAC");
> > > -
> > > - dev->iobase = 0;
> > > - dev->init = davinci_eth_open;
> > > - dev->halt = davinci_eth_close;
> > > - dev->send = davinci_eth_send_packet;
> > > - dev->recv = davinci_eth_rcv_packet;
> > > - dev->write_hwaddr = davinci_eth_set_mac_addr;
> > > -
> > > - eth_register(dev);
> > >
> > > davinci_eth_mdio_enable();
> > >
> > > @@ -854,5 +839,29 @@ int davinci_emac_initialize(void)
> > > phy[i].auto_negotiate(i);
> > > }
> > > #endif
> > > - return(1);
> > > + return 0;
> > > }
> > > +
> > > +static const struct eth_ops davinci_emac_ops = {
> > > + .start = davinci_emac_start,
> > > + .send = davinci_emac_send,
> > > + .recv = davinci_emac_recv,
> > > + .stop = davinci_emac_stop,
> > > + .write_hwaddr = davinci_emac_write_hwaddr,
> > > +};
> > > +
> > > +static const struct udevice_id davinci_emac_ids[] = {
> > > + { .compatible = "ti,davinci-dm6467-emac" },
> > > + { .compatible = "ti,am3517-emac", },
> > > + { .compatible = "ti,dm816-emac", },
> > > + { }
> > > +};
> > > +
> > > +U_BOOT_DRIVER(davinci_emac) = {
> > > + .name = "davinci_emac",
> > > + .id = UCLASS_ETH,
> > > + .of_match = davinci_emac_ids,
> > > + .probe = davinci_emac_probe,
> > > + .ops = &davinci_emac_ops,
> > > + .platdata_auto_alloc_size = sizeof(struct eth_pdata),
> > > +};
> > > diff --git a/include/netdev.h b/include/netdev.h
> > > index 0a1a3a2d8d..a40c4adaad 100644
> > > --- a/include/netdev.h
> > > +++ b/include/netdev.h
> > > @@ -30,7 +30,6 @@ int bcm_sf2_eth_register(bd_t *bis, u8 dev_num);
> > > int bfin_EMAC_initialize(bd_t *bis);
> > > int calxedaxgmac_initialize(u32 id, ulong base_addr);
> > > int cs8900_initialize(u8 dev_num, int base_addr);
> > > -int davinci_emac_initialize(void);
> > > int dc21x4x_initialize(bd_t *bis);
> > > int designware_initialize(ulong base_addr, u32 interface);
> > > int dm9000_initialize(bd_t *bis);
> > > --
> > > 2.21.0
> > >
More information about the U-Boot
mailing list