Dear Ilya Yanok,
In message <[email protected]> you wrote:
> Driver for Dave DNET ethernet controller (used on Dave/DENX
> QongEVB-LITE board).
...
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -69,6 +69,7 @@ COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o
> COBJS-$(CONFIG_XILINX_EMAC) += xilinx_emac.o
> COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
> COBJS-$(CONFIG_SH_ETHER) += sh_eth.o
> +COBJS-$(CONFIG_DRIVER_DNET) += dnet.o
Please omit the "DIVER_" part in the name.
> +struct dnet_device {
> + void *regs;
> +
> + const struct device *dev;
> + struct eth_device netdev;
> + unsigned short phy_addr;
> +};
Please no empty line in the struct declaration.
> +#define to_dnet(_nd) container_of(_nd, struct dnet_device, netdev)
Maybe a comment what this is good for?
> +static void dnet_mdio_write(struct dnet_device *dnet, u8 reg, u16 value)
> +{
> + u16 tmp;
> +
> + debug(DRIVERNAME "dnet_mdio_write %02x:%02x <- %04x\n",
> + dnet->phy_addr, reg, value);
> +
> + while (!(dnet_readw_mac(dnet, DNET_INTERNAL_GMII_MNG_CTL_REG) &
> + DNET_INTERNAL_GMII_MNG_CMD_FIN));
Please move the semicolon to a separate line.
> + while (!(dnet_readw_mac(dnet, DNET_INTERNAL_GMII_MNG_CTL_REG) &
> + DNET_INTERNAL_GMII_MNG_CMD_FIN));
Ditto.
> +static u16 dnet_mdio_read(struct dnet_device *dnet, u8 reg)
> +{
> + u16 value;
> +
> + while (!(dnet_readw_mac(dnet, DNET_INTERNAL_GMII_MNG_CTL_REG) &
> + DNET_INTERNAL_GMII_MNG_CMD_FIN));
> +
> + /* only 5 bits allowed for register offset*/
> + reg &= 0x1f;
> +
> + /* prepare reg_value for a read */
> + value = (dnet->phy_addr << 8);
> + value |= reg;
> +
> + /* write control word */
> + dnet_writew_mac(dnet, DNET_INTERNAL_GMII_MNG_CTL_REG, value);
> +
> + /* wait for end of transfer */
> + while (!(dnet_readw_mac(dnet, DNET_INTERNAL_GMII_MNG_CTL_REG) &
> + DNET_INTERNAL_GMII_MNG_CMD_FIN));
Ditto - and so on.
> +static int dnet_recv(struct eth_device *netdev)
> +{
> + struct dnet_device *dnet = to_dnet(netdev);
> + unsigned int * data_ptr;
> + int pkt_len, poll, i;
> + u32 cmd_word;
> +
> + debug("Waiting for pkt (polling)\n");
> + poll = 50;
> + while ((dnet_readl(dnet, RX_FIFO_WCNT) >> 16) == 0) {
> + udelay(10); // wait 10 usec
----------------------------^^^
> + --poll;
> + if (poll == 0)
> + return 0; // no pkt available
---------------------------------------^^^
No C++ comments, please.
> +/* Register access macros */
> +#define dnet_writel(port, value, reg) \
> + writel((value), (port)->regs + DNET_##reg)
Please do not swap arguments.
> +#define dnet_readl(port, reg) readl((port)->regs + DNET_##reg)
Hmm... Why do we need this?
> +/* ALL DNET FIFO REGISTERS */
> +#define DNET_RX_LEN_FIFO (0x000) /* RX_LEN_FIFO */
> +#define DNET_RX_DATA_FIFO (0x004) /* RX_DATA_FIFO */
> +#define DNET_TX_LEN_FIFO (0x008) /* TX_LEN_FIFO */
> +#define DNET_TX_DATA_FIFO (0x00C) /* TX_DATA_FIFO */
> +
> +/* ALL DNET CONTROL/STATUS REGISTERS OFFSETS */
> +#define DNET_VERCAPS (0x100) /* VERCAPS */
> +#define DNET_INTR_SRC (0x104) /* INTR_SRC */
> +#define DNET_INTR_ENB (0x108) /* INTR_ENB */
> +#define DNET_RX_STATUS (0x10C) /* RX_STATUS */
> +#define DNET_TX_STATUS (0x110) /* TX_STATUS */
> +#define DNET_RX_FRAMES_CNT (0x114) /* RX_FRAMES_CNT */
> +#define DNET_TX_FRAMES_CNT (0x118) /* TX_FRAMES_CNT */
> +#define DNET_RX_FIFO_TH (0x11C) /* RX_FIFO_TH */
> +#define DNET_TX_FIFO_TH (0x120) /* TX_FIFO_TH */
> +#define DNET_SYS_CTL (0x124) /* SYS_CTL */
> +#define DNET_PAUSE_TMR (0x128) /* PAUSE_TMR */
> +#define DNET_RX_FIFO_WCNT (0x12C) /* RX_FIFO_WCNT */
> +#define DNET_TX_FIFO_WCNT (0x130) /* TX_FIFO_WCNT */
...
I see.
Please do not operate on base register plus magic offset - implement a
proper C structure instead to describe the controller hardware.
> +/*
> + * Capabilities. Used by the driver to know the capabilities that
> + * the ethernet controller inside the FPGA have.
> + */
> +
> +#define DNET_HAS_MDIO (1 << 0)
> +#define DNET_HAS_IRQ (1 << 1)
> +#define DNET_HAS_GIGABIT (1 << 2)
> +#define DNET_HAS_DMA (1 << 3)
> +
> +#define DNET_HAS_MII (1 << 4) // or GMII
> +#define DNET_HAS_RMII (1 << 5) // or RGMII
No C++ comments, please.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: [email protected]
Killing is wrong.
-- Losira, "That Which Survives", stardate unknown
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
