[U-Boot] [PATCH v7] net: ll_temac: Add LL TEMAC driver to u-boot

[Andy Fleming]

On Thu, Dec 1, 2011 at 2:39 PM, Stephan Linz <linz at li-pro.net> wrote:
> +static int phywrite(struct eth_device *dev, u8 phy_addr,
> +                                       u8 reg_addr, u16 phy_data)
> +{
> +       struct temac_reg *regs = (struct temac_reg *)dev->iobase;
> +
> +       out_be32(&regs->lsw, (phy_data & LSW_REGDAT_MASK));
> +       out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMWD);
> +
> +       out_be32(&regs->lsw,
> +               ((phy_addr << LSW_PHYAD_POS) & LSW_PHYAD_MASK) |
> +               (reg_addr & LSW_REGAD_MASK));
> +       out_be32(&regs->ctl, CTL_WEN | TEMAC_MIIMAI);
> +
> +       if (check_status(regs, RSE_MIIM_WR))
> +               return 1;
> +
> +       return 0;
> +}
> +
> +/*
> + * Indirect MII PHY read via ll_temac.
> + *
> + * http://www.xilinx.com/support/documentation/ip_documentation/xps_ll_temac.pdf
> + * page 67, Using the MII Management to Access PHY Registers
> + */
> +static int phyread(struct eth_device *dev, u8 phy_addr,
> +                                       u8 reg_addr, u16 *value)


phywrite() and phyread() are very generic function names that nearly
override the PHY Lib's phy_read() and phy_write(). I also notice that
these functions don't appear to implement the PHY Lib read/write
interface.


> +
> +/* setting sub-controller and ll_temac to proper setting */
> +static int setup_ctrl(struct eth_device *dev)
> +{
> +       struct ll_temac *ll_temac = dev->priv;
> +
> +       if (ll_temac->ctrlreset) {
> +
> +               if (ll_temac->ctrlreset(dev))
> +                       return 0;
> +
> +       }


Minor nit - this could be:

if (ll_temac->ctrlreset && ll_temac->ctrlreset(dev))
        return 0;

Certainly it needs neither the 2 empty lines, nor the braces.


> +
> +       if (ll_temac->ctrlinit) {
> +
> +               if (ll_temac->ctrlinit(dev))
> +                       return 0;
> +
> +       }


Same here.


> +static int ll_temac_miiphy_read(const char *devname, unsigned char addr,
> +                               unsigned char reg, unsigned short *value)
> +{
> +       int ret;
> +       struct eth_device *dev = eth_get_dev();
> +
> +       ret = phyread(dev, addr, reg, value);
> +       debug("%s 0x%x, 0x%x, 0x%x\n", __func__, addr, reg, *value);
> +       return ret;
> +}
> +
> +static int ll_temac_miiphy_write(const char *devname, unsigned char addr,
> +                               unsigned char reg, unsigned short value)
> +{
> +       struct eth_device *dev = eth_get_dev();
> +
> +       debug("%s 0x%x, 0x%x, 0x%x\n", __func__, addr, reg, value);
> +       return phywrite(dev, addr, reg, value);
> +}


Please don't implement these functions. They are deprecated. This way
of doing things will cause problems if eth_get_dev() doesn't return
the ethernet interface you want, and makes it very difficult to do
MDIO via something that's not an ethernet interface (Like gpio pins).


> +
> +/*
> + * bis:                board information
> + * base_addr:  LL TEMAC register bank
> + * ctrl_addr:  LL TEMAC sub-controller register bank (FIFO or SDMA)
> + * mode:       driver mode bit flags (see xilinx_ll_temac.h)
> + */
> +int xilinx_ll_temac_initialize(bd_t *bis, unsigned long base_addr,
> +                               int mode, unsigned long ctrl_addr)
> +{
> +       struct eth_device *dev;
> +       struct ll_temac *ll_temac;
> +
> +       dev = calloc(1, sizeof(*dev));
> +       if (dev == NULL)
> +               return -1;
> +
> +       dev->priv = calloc(1, sizeof(struct ll_temac));
> +       if (dev->priv == NULL) {
> +               free(dev);
> +               return -1;
> +       }
> +
> +       ll_temac = dev->priv;
> +
> +       sprintf(dev->name, "Xlltem.%lx", base_addr);
> +
> +       dev->iobase = base_addr;
> +       ll_temac->ctrladdr = ctrl_addr;
> +       ll_temac->rx_bp = rx_buffer;
> +       ll_temac->tx_bp = tx_buffer;
> +       ll_temac->rx_dp = &rx_descr;
> +       ll_temac->tx_dp = &tx_descr;
> +       ll_temac->phyaddr = CONFIG_PHY_ADDR;
> +
> +       dev->init = ll_temac_init;
> +       dev->halt = ll_temac_halt;
> +       dev->write_hwaddr = ll_temac_addr_setup;
> +
> +       if (mode & M_SDMA) {
> +#if defined(CONFIG_XILINX_440) || defined(CONFIG_XILINX_405)
> +               if (mode & M_DCR) {
> +                       ll_temac_collect_xldcr_sdma_reg_addr(dev);
> +                       ll_temac->in32 = ll_temac_xldcr_in32;
> +                       ll_temac->out32 = ll_temac_xldcr_out32;
> +               } else
> +#endif
> +               {
> +                       ll_temac_collect_xlplb_sdma_reg_addr(dev);
> +                       ll_temac->in32 = ll_temac_xlplb_in32;
> +                       ll_temac->out32 = ll_temac_xlplb_out32;
> +               }
> +               ll_temac->ctrlinit = ll_temac_init_sdma;
> +               ll_temac->ctrlhalt = ll_temac_halt_sdma;
> +               ll_temac->ctrlreset = ll_temac_reset_sdma;
> +               dev->recv = ll_temac_recv_sdma;
> +               dev->send = ll_temac_send_sdma;
> +       } else {
> +               ll_temac->in32 = NULL;
> +               ll_temac->out32 = NULL;
> +               ll_temac->ctrlinit = NULL;
> +               ll_temac->ctrlhalt = NULL;
> +               ll_temac->ctrlreset = ll_temac_reset_fifo;
> +               dev->recv = ll_temac_recv_fifo;
> +               dev->send = ll_temac_send_fifo;
> +       }
> +
> +       eth_register(dev);
> +
> +       miiphy_register(dev->name, ll_temac_miiphy_read, ll_temac_miiphy_write);
> +       ll_temac->bus = miiphy_get_dev_by_name(dev->name);
> +       ll_temac->bus->reset = NULL;


This is fairly broken. miiphy_register() is a deprecated interface,
and to use it with PHYLIB is backwards. Use the mdio_register()
function, and implement the proper PHY read/write callbacks which are
passed a bus handle instead of an ethernet name.

Andy