[U-Boot] [PATCH 2/2] net: lpc32xx: add RMII phy mode support

[Vladimir Zapolskiy <vz@mleia.com>]

Hi Sylvain,

On 06.07.2015 21:50, LEMIEUX, SYLVAIN wrote:
> Hi Vladimir,
> 
> I tested the patch and it is working.
> 
> However, in the legacy BSP from NXP, a 10ms delay was present after the soft reset release (to let the MII talk to the PHY) in RMII phy mode.

there is 10ms delay in lpc32xx_eth_initialize() and 2ms delay in
lpc32xx_eth_halt(), do we need another delay in lpc32xx_eth_init()?

In fact I believe lpc32xx_eth_halt() realization may be improved, I
think that soft reset is not needed there, just stopping TX/RX should be
fine. But this is another patch and it needs thorough testing.

> In my tests, the download rate was 20% to 25% slower if the 10ms delay was not present.
> 

Do you compare performance of my change against my change plus added 10?
Or my change against legacy BSP? Could you please send me a proposed
change for testing, I'm not sure I have access to the legacy BSP from NXP.

Myself I tested a big number of adjusted/added delays and updates to
lpc32xx_eth_init() and lpc32xx_eth_halt(), but my primary optimized
value, as you can see from the commit message of the previous patch, is
time to complete autonegotiation. Download rate can be optimized
separately, may be you can send a patch on top of this series to let me
test it on my side? Also which PHY do you test?

All in all thank you so much for testing and shared info!

With best wishes,
Vladimir

> 
> Sylvain Lemieux
> Tyco
> 
> -----Original Message-----
> From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Vladimir Zapolskiy
> Sent: 6-Jul-15 12:22 AM
> To: Albert ARIBAUD; Joe Hershberger
> Cc: Tom Rini; u-boot at lists.denx.de
> Subject: [U-Boot] [PATCH 2/2] net: lpc32xx: add RMII phy mode support
> 
> LPC32xx MAC and clock control configuration requires some minor quirks to deal with a phy connected by RMII.
> 
> It's worth to mention that the kernel and legacy BSP from NXP sets SUPP_RESET_RMII == (1 << 11) bit, however the description of this bit is missing in shared LPC32x0 User Manual UM10326 Rev. 3, July 22, 2011 and in LPC32x0 Draft User Mannual Rev. 00.27, November 20, 2008, also in my tests an SMSC LAN8700 phy device connected over RMII seems to work correctly without touching this bit.
> 
> Add support of RMII, if CONFIG_RMII is defined, this option is aligned with a number of boards, which already define the same config value.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  arch/arm/cpu/arm926ejs/lpc32xx/devices.c |  7 ++++++-
>  drivers/net/lpc32xx_eth.c                | 20 +++++++++++++++++---
>  2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> index 5a453e3..9c8d655 100644
> --- a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> +++ b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> @@ -45,7 +45,12 @@ void lpc32xx_mac_init(void)  {
>         /* Enable MAC interface */
>         writel(CLK_MAC_REG | CLK_MAC_SLAVE | CLK_MAC_MASTER
> -               | CLK_MAC_MII, &clk->macclk_ctrl);
> +#if defined(CONFIG_RMII)
> +               | CLK_MAC_RMII,
> +#else
> +               | CLK_MAC_MII,
> +#endif
> +               &clk->macclk_ctrl);
>  }
> 
>  void lpc32xx_mlc_nand_init(void)
> diff --git a/drivers/net/lpc32xx_eth.c b/drivers/net/lpc32xx_eth.c index f883a25..f3ab0f4 100644
> --- a/drivers/net/lpc32xx_eth.c
> +++ b/drivers/net/lpc32xx_eth.c
> @@ -168,6 +168,7 @@ struct lpc32xx_eth_registers {
>  #define COMMAND_RXENABLE      0x00000001
>  #define COMMAND_TXENABLE      0x00000002
>  #define COMMAND_PASSRUNTFRAME 0x00000040
> +#define COMMAND_RMII          0x00000200
>  #define COMMAND_FULL_DUPLEX   0x00000400
>  /* Helper: general reset */
>  #define COMMAND_RESETS        0x00000038
> @@ -201,6 +202,7 @@ struct lpc32xx_eth_device {
>         struct eth_device dev;
>         struct lpc32xx_eth_registers *regs;
>         struct lpc32xx_eth_buffers *bufs;
> +       bool phy_rmii;
>  };
> 
>  #define LPC32XX_ETH_DEVICE_SIZE (sizeof(struct lpc32xx_eth_device)) @@ -359,7 +361,10 @@ int lpc32xx_eth_phy_write(struct mii_dev *bus, int phy_addr, int dev_addr,
> 
>  static struct lpc32xx_eth_device lpc32xx_eth = {
>         .regs = (struct lpc32xx_eth_registers *)LPC32XX_ETH_BASE,
> -       .bufs = (struct lpc32xx_eth_buffers *)LPC32XX_ETH_BUFS
> +       .bufs = (struct lpc32xx_eth_buffers *)LPC32XX_ETH_BUFS, #if
> +defined(CONFIG_RMII)
> +       .phy_rmii = true,
> +#endif
>  };
> 
>  #define TX_TIMEOUT 10000
> @@ -471,7 +476,10 @@ static int lpc32xx_eth_init(struct eth_device *dev)
>         writel(0x0012, &regs->ipgr);
> 
>         /* pass runt (smaller than 64 bytes) frames */
> -       writel(COMMAND_PASSRUNTFRAME, &regs->command);
> +       if (lpc32xx_eth_device->phy_rmii)
> +               writel(COMMAND_PASSRUNTFRAME | COMMAND_RMII, &regs->command);
> +       else
> +               writel(COMMAND_PASSRUNTFRAME, &regs->command);
> 
>         /* Configure Full/Half Duplex mode */
>         if (miiphy_duplex(dev->name, CONFIG_PHY_ADDR) == FULL) { @@ -559,6 +567,8 @@ static int lpc32xx_eth_halt(struct eth_device *dev)  #if defined(CONFIG_PHYLIB)  int lpc32xx_eth_phylib_init(struct eth_device *dev, int phyid)  {
> +       struct lpc32xx_eth_device *lpc32xx_eth_device =
> +               container_of(dev, struct lpc32xx_eth_device, dev);
>         struct mii_dev *bus;
>         struct phy_device *phydev;
>         int ret;
> @@ -579,7 +589,11 @@ int lpc32xx_eth_phylib_init(struct eth_device *dev, int phyid)
>                 return -ENOMEM;
>         }
> 
> -       phydev = phy_connect(bus, phyid, dev, PHY_INTERFACE_MODE_MII);
> +       if (lpc32xx_eth_device->phy_rmii)
> +               phydev = phy_connect(bus, phyid, dev, PHY_INTERFACE_MODE_RMII);
> +       else
> +               phydev = phy_connect(bus, phyid, dev, PHY_INTERFACE_MODE_MII);
> +
>         if (!phydev) {
>                 printf("phy_connect failed\n");
>                 return -ENODEV;
> --
> 2.1.4
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 
> ________________________________
> 
> This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you are not the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any action in respect of any information contained in it. If you have received this e-mail in error, please notify the sender immediately by e-mail and immediately destroy this e-mail and its attachments.
>