| Message ID | 4F172219764C784B84C2C1FF44E7DFB102FEDBC0@003FCH1MPN2-042.003f.mgd2.msft.net |
|---|---|
| State | Rejected |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
Hi Sylvain, On 07.07.2015 22:13, LEMIEUX, SYLVAIN wrote: > This is a minor update to LPC32xx MAC driver patch that add support for the RMII phy mode. > > In the legacy BSP from NXP, in RMII phy mode, an extra 10ms delay was present after the release of the soft reset. > > In my tests (connected using RMII phy mode to a Micrel KSZ8031RNL), the download rate was 20% to 25% slower if the extra 10ms delay was not present. I found some time to test this change, and actually on my environment I don't observe any improvement, below are some details: * tested on devkit3250 board with SMSC LAN8700 phy connected to LPC3250 MAC over RMII, * to download files to RAM I used tftp protocol, * file download is started immediately on boot, boot delay is set to 0, bootcmd runs dhcp and then tftp, * I downloaded files of 1 MiB and 48 MiB size, both cases produce the same download speed of about 1.15 MiB/second, presence/absence of the proposed 10ms delay in lpc32xx_eth_initialize() has no impact on download speed, * the speed is not limited by external network hardware or topology, absolutely in the same environment an iMX6 board is capable to download files over tftp at 3.5 - 4.0 MiB per second rate, * 20% to 25% download speed improvement (1.15 MiB/s to expected ~1.4 MiB/s) is not observed after applying your proposed change, probably in your environment this delay is needed due to your board, network connection or PHY specifics, is Micrel KSZ8031RNL support comes from legacy BSP?, * I can not confirm that an additional delay on board/MAC init is needed here regardless of its presence in legacy BSP since it defers board initialization by 10ms with no visual download speed improvement, also any possible relation between this delay and download speed is not obvious, * download speed in my legacy BSP dated by 2010 is about 3-4 times slower than in vanilla U-boot, unfortunately the exact figure is not reported. If the change has positive impact for you, I would propose to add a delay in your board file, e.g. in board_early_init_f() after lpc32xx_eth_initialize() call and retest, right at the moment I hesitate to add a 10ms delay to _all_ users of LPC32xx MAC / RMII phy. Also it would be nice to know the exact download speed on your end for * current version of lpc32xx_eth.c with my recent patches applied, * current version of lpc32xx_eth.c with my recent patches applied and your change, * legacy BSP. A couple of general recommendations regarding posting changes: * too long lines in commit message, * it seems that your mail server screws up tab symbols from the mail body, the patch can not be applied as is, * also it seems that your mail server repacks messages, so that a downloaded version of the sent patch is not a valid patch, from the mail headers: Content-Transfer-Encoding: base64 > This patch actually requires four other lpc32xx patches that are not yet available in u-boot/master or u-boot-arm/master: > 1) http://patchwork.ozlabs.org/patch/489100/ > 2) http://patchwork.ozlabs.org/patch/489190/ > 3) http://patchwork.ozlabs.org/patch/491419/ > 4) http://patchwork.ozlabs.org/patch/491420/ > > Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com> > --- > drivers/net/lpc32xx_eth.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/lpc32xx_eth.c b/drivers/net/lpc32xx_eth.c > index bb8d1ec..207d721 100644 > --- a/drivers/net/lpc32xx_eth.c > +++ b/drivers/net/lpc32xx_eth.c > @@ -634,6 +634,8 @@ int lpc32xx_eth_initialize(bd_t *bis) > > /* Release SOFT reset to let MII talk to PHY */ > clrbits_le32(®s->mac1, MAC1_SOFT_RESET); > + if (lpc32xx_eth.phy_rmii) > + udelay(10000); > > /* register driver before talking to phy */ > eth_register(dev); -- With best wishes, Vladimir
Hi Vladimir, Thanks for taking the time to test this patch. After further investigation, the issue was with our internal Micrel KSZ8031RNL phy implementation. The extra 10ms delay is not require. This patch should be discard. Thanks for the feedback regarding the mail system; - for an internal mail address, the "tab" is not translated in "space". - for an external e-mail, there is a translation that take place; we will look into the problem before sending the patch to support the Micrel KSZ8031RNL phy. Sylvain Lemieux -----Original Message----- From: Vladimir Zapolskiy [mailto:vz@mleia.com] Sent: 8-Jul-15 1:06 AM To: LEMIEUX, SYLVAIN Cc: joe.hershberger@ni.com; albert.aribaud@3adev.fr; u-boot@lists.denx.de Subject: Re: [PATCH 1/1] net: lpc32xx: improve download rate in RMII phy mode Hi Sylvain, On 07.07.2015 22:13, LEMIEUX, SYLVAIN wrote: > This is a minor update to LPC32xx MAC driver patch that add support for the RMII phy mode. > > In the legacy BSP from NXP, in RMII phy mode, an extra 10ms delay was present after the release of the soft reset. > > In my tests (connected using RMII phy mode to a Micrel KSZ8031RNL), the download rate was 20% to 25% slower if the extra 10ms delay was not present. I found some time to test this change, and actually on my environment I don't observe any improvement, below are some details: * tested on devkit3250 board with SMSC LAN8700 phy connected to LPC3250 MAC over RMII, * to download files to RAM I used tftp protocol, * file download is started immediately on boot, boot delay is set to 0, bootcmd runs dhcp and then tftp, * I downloaded files of 1 MiB and 48 MiB size, both cases produce the same download speed of about 1.15 MiB/second, presence/absence of the proposed 10ms delay in lpc32xx_eth_initialize() has no impact on download speed, * the speed is not limited by external network hardware or topology, absolutely in the same environment an iMX6 board is capable to download files over tftp at 3.5 - 4.0 MiB per second rate, * 20% to 25% download speed improvement (1.15 MiB/s to expected ~1.4 MiB/s) is not observed after applying your proposed change, probably in your environment this delay is needed due to your board, network connection or PHY specifics, is Micrel KSZ8031RNL support comes from legacy BSP?, * I can not confirm that an additional delay on board/MAC init is needed here regardless of its presence in legacy BSP since it defers board initialization by 10ms with no visual download speed improvement, also any possible relation between this delay and download speed is not obvious, * download speed in my legacy BSP dated by 2010 is about 3-4 times slower than in vanilla U-boot, unfortunately the exact figure is not reported. If the change has positive impact for you, I would propose to add a delay in your board file, e.g. in board_early_init_f() after lpc32xx_eth_initialize() call and retest, right at the moment I hesitate to add a 10ms delay to _all_ users of LPC32xx MAC / RMII phy. Also it would be nice to know the exact download speed on your end for * current version of lpc32xx_eth.c with my recent patches applied, * current version of lpc32xx_eth.c with my recent patches applied and your change, * legacy BSP. A couple of general recommendations regarding posting changes: * too long lines in commit message, * it seems that your mail server screws up tab symbols from the mail body, the patch can not be applied as is, * also it seems that your mail server repacks messages, so that a downloaded version of the sent patch is not a valid patch, from the mail headers: -- With best wishes, Vladimir
Hello SYLVAIN, On Wed, 8 Jul 2015 17:41:54 +0000, LEMIEUX, SYLVAIN <slemieux@Tycoint.com> wrote: > Hi Vladimir, > > Thanks for taking the time to test this patch. > > After further investigation, the issue was with our internal Micrel KSZ8031RNL phy implementation. > The extra 10ms delay is not require. This patch should be discard. Ok--I've marked it "Rejected" in patchwork (and assigned it to myself in case people later want to blame someone about the rejection). Amicalement,
diff --git a/drivers/net/lpc32xx_eth.c b/drivers/net/lpc32xx_eth.c index bb8d1ec..207d721 100644 --- a/drivers/net/lpc32xx_eth.c +++ b/drivers/net/lpc32xx_eth.c @@ -634,6 +634,8 @@ int lpc32xx_eth_initialize(bd_t *bis) /* Release SOFT reset to let MII talk to PHY */ clrbits_le32(®s->mac1, MAC1_SOFT_RESET); + if (lpc32xx_eth.phy_rmii) + udelay(10000); /* register driver before talking to phy */ eth_register(dev);
This is a minor update to LPC32xx MAC driver patch that add support for the RMII phy mode. In the legacy BSP from NXP, in RMII phy mode, an extra 10ms delay was present after the release of the soft reset. In my tests (connected using RMII phy mode to a Micrel KSZ8031RNL), the download rate was 20% to 25% slower if the extra 10ms delay was not present. This patch actually requires four other lpc32xx patches that are not yet available in u-boot/master or u-boot-arm/master: 1) http://patchwork.ozlabs.org/patch/489100/ 2) http://patchwork.ozlabs.org/patch/489190/ 3) http://patchwork.ozlabs.org/patch/491419/ 4) http://patchwork.ozlabs.org/patch/491420/ Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com> --- drivers/net/lpc32xx_eth.c | 2 ++ 1 file changed, 2 insertions(+) -- 1.7.12.4