[platforms: PATCH v2 0/4] Armada 7k/8k - misc improvements pt.3

[platforms: PATCH v2 0/4] Armada 7k/8k - misc improvements pt.3

[Marcin Wojtas]

Hi,

The second version of the patchset brings the corrections
requested in the review. Details can be found in the changelog
below.

The patches are available in the github:
https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/misc-upstream-r20171201-2

I'm looking forward to your comments or remarks.

Best regards,
Marcin

Changelog:
v1 - > v2

* 1/4
    - Add tftp as a build-time selectable option
    - Update commit message

* 2/4
    - Add RB

* 3/4
    - Leave initial file format and reword commit message

* 4/4
    - Leave PHY_AUTONEGOTIATE_TIMEOUT unmodified and
      remove double definition of the timeout.

Marcin Wojtas (4):
  Marvell/Armada: Switch to dynamic tftp command
  Marvell/Armada: Fix watchdog control base
  Marvell/Applications: FirmwareUpdate: Fix usage information
  Marvell/Drivers: MvPhyDxe: Cleanup the header

 Platform/Marvell/Applications/FirmwareUpdate/FUpdate.uni | Bin 5146 -> 5190 bytes
 Platform/Marvell/Armada/Armada.dsc.inc                   |   6 +-
 Platform/Marvell/Armada/Armada70x0.dsc                   |   1 +
 Platform/Marvell/Armada/Armada70x0.fdf                   |   3 +
 Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c     |   2 +-
 Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h     | 152 ++++----------------
 6 files changed, 40 insertions(+), 124 deletions(-)

-- 
2.7.4

[platforms: PATCH v2 1/4] Marvell/Armada: Switch to dynamic tftp command

[Marcin Wojtas]

After removal of the tftp library in EDK2, the tftp was
disabled on Armada platform. Re-enable this functionality
as a dynamic command on Armada 70x0 DB board. For this
purpose add it as a build-time selectable option, depending
on a new INCLUDE_TFTP_COMMAND parameter.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Armada/Armada.dsc.inc | 4 ++++
 Platform/Marvell/Armada/Armada70x0.dsc | 1 +
 Platform/Marvell/Armada/Armada70x0.fdf | 3 +++
 3 files changed, 8 insertions(+)

diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
index 2a8a226..6040493 100644
--- a/Platform/Marvell/Armada/Armada.dsc.inc
+++ b/Platform/Marvell/Armada/Armada.dsc.inc
@@ -525,6 +525,10 @@
       gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
   }
 
+!ifdef $(INCLUDE_TFTP_COMMAND)
+  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
+!endif #$(INCLUDE_TFTP_COMMAND)
+
 [BuildOptions.common.EDKII.DXE_CORE,BuildOptions.common.EDKII.DXE_DRIVER,BuildOptions.common.EDKII.UEFI_DRIVER,BuildOptions.common.EDKII.UEFI_APPLICATION]
   GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
 
diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
index 8e4cdb2..4e7d43c 100644
--- a/Platform/Marvell/Armada/Armada70x0.dsc
+++ b/Platform/Marvell/Armada/Armada70x0.dsc
@@ -44,6 +44,7 @@
   BUILD_TARGETS                  = DEBUG|RELEASE
   SKUID_IDENTIFIER               = DEFAULT
   FLASH_DEFINITION               = Platform/Marvell/Armada/Armada70x0.fdf
+  DEFINE INCLUDE_TFTP_COMMAND    = 1
 
 !include Armada.dsc.inc
 
diff --git a/Platform/Marvell/Armada/Armada70x0.fdf b/Platform/Marvell/Armada/Armada70x0.fdf
index ca92c60..c03bc42 100644
--- a/Platform/Marvell/Armada/Armada70x0.fdf
+++ b/Platform/Marvell/Armada/Armada70x0.fdf
@@ -176,6 +176,9 @@ FvNameGuid         = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
 
   # UEFI application (Shell Embedded Boot Loader)
   INF ShellPkg/Application/Shell/Shell.inf
+!ifdef $(INCLUDE_TFTP_COMMAND)
+  INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
+!endif #$(INCLUDE_TFTP_COMMAND)
 
   # Bds
   INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
-- 
2.7.4

[platforms: PATCH v2 2/4] Marvell/Armada: Fix watchdog control base

[Marcin Wojtas]

Proper watchdog control base is 0xf0610000, so fix the
incorrect value.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 Platform/Marvell/Armada/Armada.dsc.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
index 6040493..4b29691 100644
--- a/Platform/Marvell/Armada/Armada.dsc.inc
+++ b/Platform/Marvell/Armada/Armada.dsc.inc
@@ -269,7 +269,7 @@
   gEmbeddedTokenSpaceGuid.PcdMetronomeTickPeriod|1000
 
   # ARM SBSA Watchdog
-  gArmTokenSpaceGuid.PcdGenericWatchdogControlBase|0xF0620000
+  gArmTokenSpaceGuid.PcdGenericWatchdogControlBase|0xF0610000
   gArmTokenSpaceGuid.PcdGenericWatchdogRefreshBase|0xF0600000
   gArmTokenSpaceGuid.PcdGenericWatchdogEl2IntrNum|34
 
-- 
2.7.4

[platforms: PATCH v2 3/4] Marvell/Applications: FirmwareUpdate: Fix usage information

[Marcin Wojtas]

fupdate command's usage information referred to a deprecated
'-f' flag in 'examples' section. It was a residue from the
initial version of the application, removed during review
before merging to upstream branch. Correct the help information.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Applications/FirmwareUpdate/FUpdate.uni | Bin 5146 -> 5190 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)

diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.uni b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.uni
index 9d52e590c6d239621d226a240a3f9755210f52fe..146f624737d96e2cd354857dbb63390ebe2f347e 100644
GIT binary patch
delta 56
zcmbQGaZF=_h>&<DLmq<yLoh=CgC~$qW5{7hWGH6Hm@FuixVb_of^o76yTD`vb`~(c
HAnXMIdh-p2

delta 28
kcmX@6F-v2Eh|pv`p*Ut;hP27{LYA8k2`Mp7{v+%K0D@Hs0RR91

-- 
2.7.4

[platforms: PATCH v2 4/4] Marvell/Drivers: MvPhyDxe: Cleanup the header

[Marcin Wojtas]

This patch removes unused macros defined in MvPhyDxe.h, as well
as improves the style and comments. Pick single definition
of the autonegotiation timeout - two different macros were used
for the same purpose.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Marcin Wojtas <mw@semihalf.com>
---
 Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c |   2 +-
 Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h | 152 ++++----------------
 2 files changed, 31 insertions(+), 123 deletions(-)

diff --git a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
index e776a91..dd2edae 100644
--- a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
+++ b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
@@ -328,7 +328,7 @@ MvPhyInit1512 (
 
     DEBUG((DEBUG_ERROR, "MvPhyDxe: Waiting for PHY auto negotiation... "));
     for (i = 0; !(Data & BMSR_ANEGCOMPLETE); i++) {
-      if (i > PHY_ANEG_TIMEOUT) {
+      if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
         DEBUG((DEBUG_ERROR, "timeout\n"));
         PhyDev->LinkUp = FALSE;
         return EFI_TIMEOUT;
diff --git a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h
index 0c3d935..66974bb 100644
--- a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h
+++ b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h
@@ -34,137 +34,45 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #ifndef __MV_PHY_DXE_H__
 #define __MV_PHY_DXE_H__
 
-#define MII_BMCR      0x00  /* Basic mode control Register */
-#define MII_BMSR      0x01  /* Basic mode status Register  */
-#define MII_PHYSID1      0x02  /* PHYS ID 1           */
-#define MII_PHYSID2      0x03  /* PHYS ID 2           */
-#define MII_ADVERTISE      0x04  /* Advertisement control Reg   */
-#define MII_LPA        0x05  /* Link partner ability Reg    */
-#define MII_EXPANSION      0x06  /* Expansion Register         */
-#define MII_CTRL1000      0x09  /* 1000BASE-T control         */
-#define MII_STAT1000      0x0a  /* 1000BASE-T status         */
-#define MII_ESTATUS      0x0f  /* Extended Status */
-#define MII_DCOUNTER      0x12  /* Disconnect counter         */
-#define MII_FCSCOUNTER      0x13  /* False carrier counter       */
-#define MII_NWAYTEST      0x14  /* N-way auto-neg test Reg     */
-#define MII_RERRCOUNTER     0x15  /* Receive error counter       */
-#define MII_SREVISION      0x16  /* Silicon revision         */
-#define MII_RESV1      0x17  /* Reserved...           */
-#define MII_LBRERROR      0x18  /* Lpback, rx, bypass error    */
-#define MII_PHYADDR      0x19  /* PHY address           */
-#define MII_RESV2      0x1a  /* Reserved...           */
-#define MII_TPISTATUS      0x1b  /* TPI status for 10mbps       */
-#define MII_NCONFIG      0x1c  /* Network interface config    */
-
-/* Basic mode control Register. */
-#define BMCR_RESV    0x003f  /* Unused...           */
-#define BMCR_SPEED1000    0x0040  /* MSB of Speed (1000)         */
-#define BMCR_CTST    0x0080  /* Collision test         */
-#define BMCR_FULLDPLX    0x0100  /* Full duplex           */
-#define BMCR_ANRESTART    0x0200  /* Auto negotiation restart    */
-#define BMCR_ISOLATE    0x0400  /* Disconnect DP83840 from MII */
-#define BMCR_PDOWN    0x0800  /* Powerdown the DP83840       */
-#define BMCR_ANENABLE    0x1000  /* Enable auto negotiation     */
-#define BMCR_SPEED100    0x2000  /* Select 100Mbps         */
-#define BMCR_LOOPBACK    0x4000  /* TXD loopback bits         */
-#define BMCR_RESET    0x8000  /* Reset the DP83840         */
-
-/* Basic mode status Register. */
-#define BMSR_ERCAP    0x0001  /* Ext-Reg capability         */
-#define BMSR_JCD    0x0002  /* Jabber detected         */
-#define BMSR_LSTATUS    0x0004  /* Link status           */
-#define BMSR_ANEGCAPABLE  0x0008  /* Able to do auto-negotiation */
-#define BMSR_RFAULT    0x0010  /* Remote fault detected       */
-#define BMSR_ANEGCOMPLETE  0x0020  /* Auto-negotiation complete   */
-#define BMSR_RESV    0x00c0  /* Unused...           */
-#define BMSR_ESTATEN    0x0100  /* Extended Status in R15 */
-#define BMSR_100HALF2    0x0200  /* Can do 100BASE-T2 HDX */
-#define BMSR_100FULL2    0x0400  /* Can do 100BASE-T2 FDX */
-#define BMSR_10HALF    0x0800  /* Can do 10mbps, half-duplex  */
-#define BMSR_10FULL    0x1000  /* Can do 10mbps, full-duplex  */
-#define BMSR_100HALF    0x2000  /* Can do 100mbps, half-duplex */
-#define BMSR_100FULL    0x4000  /* Can do 100mbps, full-duplex */
-#define BMSR_100BASE4    0x8000  /* Can do 100mbps, 4k packets  */
-
-#define PHY_ANEG_TIMEOUT 4000
-
-#define PHY_INTERFACE_MODE_RGMII 0
-#define PHY_INTERFACE_MODE_RGMII_ID 1
-#define PHY_INTERFACE_MODE_RGMII_RXID 2
-#define PHY_INTERFACE_MODE_RGMII_TXID 3
-#define PHY_INTERFACE_MODE_SGMII 4
-#define PHY_INTERFACE_MODE_RTBI 5
-
-#define PHY_AUTONEGOTIATE_TIMEOUT 5000
+#define MII_BMCR                       0x00  /* Basic mode control Register */
+#define MII_BMSR                       0x01  /* Basic mode status Register  */
+
+/* BMCR */
+#define BMCR_ANRESTART                 0x0200 /* 1 = Restart autonegotiation */
+#define BMCR_ISOLATE                   0x0400 /* 0 = Isolate PHY */
+#define BMCR_ANENABLE                  0x1000 /* 1 = Enable autonegotiation */
+#define BMCR_RESET                     0x8000 /* 1 = Reset the PHY */
+
+/* BSMR */
+#define BMSR_LSTATUS                   0x0004 /* 1 = Link up */
+#define BMSR_ANEGCAPABLE               0x0008 /* 1 = Able to perform auto-neg */
+#define BMSR_ANEGCOMPLETE              0x0020 /* 1 = Auto-neg complete */
+
+#define PHY_AUTONEGOTIATE_TIMEOUT      5000
 
 /* 88E1011 PHY Status Register */
-#define MIIM_88E1xxx_PHY_STATUS    0x11
-#define MIIM_88E1xxx_PHYSTAT_SPEED  0xc000
-#define MIIM_88E1xxx_PHYSTAT_GBIT  0x8000
-#define MIIM_88E1xxx_PHYSTAT_100  0x4000
-#define MIIM_88E1xxx_PHYSTAT_DUPLEX  0x2000
-#define MIIM_88E1xxx_PHYSTAT_SPDDONE  0x0800
-#define MIIM_88E1xxx_PHYSTAT_LINK  0x0400
-
-#define MIIM_88E1xxx_PHY_SCR    0x10
-#define MIIM_88E1xxx_PHY_MDI_X_AUTO  0x0060
-
-/* 88E1111 PHY LED Control Register */
-#define MIIM_88E1111_PHY_LED_CONTROL  24
-#define MIIM_88E1111_PHY_LED_DIRECT  0x4100
-#define MIIM_88E1111_PHY_LED_COMBINE  0x411C
+#define MIIM_88E1xxx_PHY_STATUS        0x11
+#define MIIM_88E1xxx_PHYSTAT_SPEED     0xc000
+#define MIIM_88E1xxx_PHYSTAT_GBIT      0x8000
+#define MIIM_88E1xxx_PHYSTAT_100       0x4000
+#define MIIM_88E1xxx_PHYSTAT_DUPLEX    0x2000
+#define MIIM_88E1xxx_PHYSTAT_SPDDONE   0x0800
+#define MIIM_88E1xxx_PHYSTAT_LINK      0x0400
 
 /* 88E1111 Extended PHY Specific Control Register */
-#define MIIM_88E1111_PHY_EXT_CR    0x14
-#define MIIM_88E1111_RX_DELAY    0x80
-#define MIIM_88E1111_TX_DELAY    0x2
+#define MIIM_88E1111_PHY_EXT_CR        0x14
+#define MIIM_88E1111_RX_DELAY          0x80
+#define MIIM_88E1111_TX_DELAY          0x02
 
 /* 88E1111 Extended PHY Specific Status Register */
-#define MIIM_88E1111_PHY_EXT_SR    0x1b
-#define MIIM_88E1111_HWCFG_MODE_MASK    0xf
+#define MIIM_88E1111_PHY_EXT_SR               0x1b
+#define MIIM_88E1111_HWCFG_MODE_MASK          0xf
 #define MIIM_88E1111_HWCFG_MODE_COPPER_RGMII  0xb
-#define MIIM_88E1111_HWCFG_MODE_FIBER_RGMII  0x3
+#define MIIM_88E1111_HWCFG_MODE_FIBER_RGMII   0x3
 #define MIIM_88E1111_HWCFG_MODE_SGMII_NO_CLK  0x4
-#define MIIM_88E1111_HWCFG_MODE_COPPER_RTBI  0x9
+#define MIIM_88E1111_HWCFG_MODE_COPPER_RTBI   0x9
 #define MIIM_88E1111_HWCFG_FIBER_COPPER_AUTO  0x8000
-#define MIIM_88E1111_HWCFG_FIBER_COPPER_RES  0x2000
-
-#define MIIM_88E1111_COPPER    0
-#define MIIM_88E1111_FIBER    1
-
-/* 88E1118 PHY defines */
-#define MIIM_88E1118_PHY_PAGE    22
-#define MIIM_88E1118_PHY_LED_PAGE  3
-
-/* 88E1121 PHY LED Control Register */
-#define MIIM_88E1121_PHY_LED_CTRL  16
-#define MIIM_88E1121_PHY_LED_PAGE  3
-#define MIIM_88E1121_PHY_LED_DEF  0x0030
-
-/* 88E1121 PHY IRQ Enable/Status Register */
-#define MIIM_88E1121_PHY_IRQ_EN    18
-#define MIIM_88E1121_PHY_IRQ_STATUS  19
-
-#define MIIM_88E1121_PHY_PAGE    22
-
-/* 88E1145 Extended PHY Specific Control Register */
-#define MIIM_88E1145_PHY_EXT_CR 20
-#define MIIM_M88E1145_RGMII_RX_DELAY  0x0080
-#define MIIM_M88E1145_RGMII_TX_DELAY  0x0002
-
-#define MIIM_88E1145_PHY_LED_CONTROL  24
-#define MIIM_88E1145_PHY_LED_DIRECT  0x4100
-
-#define MIIM_88E1145_PHY_PAGE  29
-#define MIIM_88E1145_PHY_CAL_OV 30
-
-#define MIIM_88E1149_PHY_PAGE  29
-
-/* 88E1310 PHY defines */
-#define MIIM_88E1310_PHY_LED_CTRL  16
-#define MIIM_88E1310_PHY_IRQ_EN    18
-#define MIIM_88E1310_PHY_RGMII_CTRL  21
-#define MIIM_88E1310_PHY_PAGE    22
+#define MIIM_88E1111_HWCFG_FIBER_COPPER_RES   0x2000
 
 typedef enum {
   MV_PHY_DEVICE_1512
-- 
2.7.4

Re: [platforms: PATCH v2 1/4] Marvell/Armada: Switch to dynamic tftp command

[Leif Lindholm]

On Fri, Dec 01, 2017 at 04:35:04PM +0100, Marcin Wojtas wrote:
> After removal of the tftp library in EDK2, the tftp was
> disabled on Armada platform. Re-enable this functionality
> as a dynamic command on Armada 70x0 DB board. For this
> purpose add it as a build-time selectable option, depending
> on a new INCLUDE_TFTP_COMMAND parameter.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Armada/Armada.dsc.inc | 4 ++++
>  Platform/Marvell/Armada/Armada70x0.dsc | 1 +
>  Platform/Marvell/Armada/Armada70x0.fdf | 3 +++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
> index 2a8a226..6040493 100644
> --- a/Platform/Marvell/Armada/Armada.dsc.inc
> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
> @@ -525,6 +525,10 @@
>        gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
>    }
>  
> +!ifdef $(INCLUDE_TFTP_COMMAND)
> +  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> +!endif #$(INCLUDE_TFTP_COMMAND)
> +
>  [BuildOptions.common.EDKII.DXE_CORE,BuildOptions.common.EDKII.DXE_DRIVER,BuildOptions.common.EDKII.UEFI_DRIVER,BuildOptions.common.EDKII.UEFI_APPLICATION]
>    GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
>  
> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
> index 8e4cdb2..4e7d43c 100644
> --- a/Platform/Marvell/Armada/Armada70x0.dsc
> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
> @@ -44,6 +44,7 @@
>    BUILD_TARGETS                  = DEBUG|RELEASE
>    SKUID_IDENTIFIER               = DEFAULT
>    FLASH_DEFINITION               = Platform/Marvell/Armada/Armada70x0.fdf
> +  DEFINE INCLUDE_TFTP_COMMAND    = 1

This looks very much to me like it will always include the tftp
command, which is not what I wanted.

Then again, you have done _exactly_ what I requested in emulating
what the Hisilicon platforms do (and will shortly stop doing).

My thoughts were that, since this is a debug/development feature, it
would be enabled at build time by adding -D INCLUDE_TFTP_COMMAND to
the build command line.

As such, do you have any issues with me deleting this hunk before
pushing? If not:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>  
>  !include Armada.dsc.inc
>  
> diff --git a/Platform/Marvell/Armada/Armada70x0.fdf b/Platform/Marvell/Armada/Armada70x0.fdf
> index ca92c60..c03bc42 100644
> --- a/Platform/Marvell/Armada/Armada70x0.fdf
> +++ b/Platform/Marvell/Armada/Armada70x0.fdf
> @@ -176,6 +176,9 @@ FvNameGuid         = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
>  
>    # UEFI application (Shell Embedded Boot Loader)
>    INF ShellPkg/Application/Shell/Shell.inf
> +!ifdef $(INCLUDE_TFTP_COMMAND)
> +  INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
> +!endif #$(INCLUDE_TFTP_COMMAND)
>  
>    # Bds
>    INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
> -- 
> 2.7.4
> 

Re: [platforms: PATCH v2 3/4] Marvell/Applications: FirmwareUpdate: Fix usage information

[Leif Lindholm]

On Fri, Dec 01, 2017 at 04:35:06PM +0100, Marcin Wojtas wrote:
> fupdate command's usage information referred to a deprecated
> '-f' flag in 'examples' section. It was a residue from the
> initial version of the application, removed during review
> before merging to upstream branch. Correct the help information.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>

I boggled slightly at how the file with deleted content is larger than
the original, so went to have a look, and what I see is:

--- pre.txt     2017-12-04 13:43:32.985446108 +0000
+++ post.txt    2017-12-04 13:44:30.433120594 +0000
@@ -50,10 +50,10 @@
 ".SH EXAMPLES\r\n"
 " \r\n"
 "EXAMPLES:\r\n"
-"Update firmware from file fs2:flash-image.bin\r\n"
-"  fupdate -f fs2:flash-image.bin\r\n"
+"Update firmware in SPI flash from file fs2:flash-image.bin\r\n"
+"  fupdate fs2:flash-image.bin\r\n"
 ".SH RETURNVALUES\r\n"
 " \r\n"
 "RETURN VALUES:\r\n"
 "  SHELL_SUCCESS             The action was completed as requested.\r\n"
-"  SHELL_ABORTED Error while processing command\r\n"
+"  SHELL_ABORTED             Error while processing command\r\n"


Now, I don't mind random format cleanup as much in .uni files as
elsewhere, since we don't get sane diffs anyway. But that also makes
detailed and accurate commit messages much more important.

So please, add something about the rewording and the whitespace fixes.

/
    Leif

> ---
>  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.uni | Bin 5146 -> 5190 bytes
>  1 file changed, 0 insertions(+), 0 deletions(-)
> 
> diff --git a/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.uni b/Platform/Marvell/Applications/FirmwareUpdate/FUpdate.uni
> index 9d52e590c6d239621d226a240a3f9755210f52fe..146f624737d96e2cd354857dbb63390ebe2f347e 100644
> GIT binary patch
> delta 56
> zcmbQGaZF=_h>&<DLmq<yLoh=CgC~$qW5{7hWGH6Hm@FuixVb_of^o76yTD`vb`~(c
> HAnXMIdh-p2
> 
> delta 28
> kcmX@6F-v2Eh|pv`p*Ut;hP27{LYA8k2`Mp7{v+%K0D@Hs0RR91
> 
> -- 
> 2.7.4
> 

Re: [platforms: PATCH v2 4/4] Marvell/Drivers: MvPhyDxe: Cleanup the header

[Leif Lindholm]

On Fri, Dec 01, 2017 at 04:35:07PM +0100, Marcin Wojtas wrote:
> This patch removes unused macros defined in MvPhyDxe.h, as well
> as improves the style and comments. Pick single definition
> of the autonegotiation timeout - two different macros were used
> for the same purpose.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c |   2 +-
>  Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h | 152 ++++----------------
>  2 files changed, 31 insertions(+), 123 deletions(-)
> 
> diff --git a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
> index e776a91..dd2edae 100644
> --- a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
> +++ b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
> @@ -328,7 +328,7 @@ MvPhyInit1512 (
>  
>      DEBUG((DEBUG_ERROR, "MvPhyDxe: Waiting for PHY auto negotiation... "));
>      for (i = 0; !(Data & BMSR_ANEGCOMPLETE); i++) {
> -      if (i > PHY_ANEG_TIMEOUT) {
> +      if (i > PHY_AUTONEGOTIATE_TIMEOUT) {

I think you've had a git accident somewhere:
The existing line upstream is still
         if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
This hunk should have just dropped out.

/
    Leif

Re: [platforms: PATCH v2 1/4] Marvell/Armada: Switch to dynamic tftp command

[Marcin Wojtas]

Hi Leif,

2017-12-04 14:32 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Fri, Dec 01, 2017 at 04:35:04PM +0100, Marcin Wojtas wrote:
>> After removal of the tftp library in EDK2, the tftp was
>> disabled on Armada platform. Re-enable this functionality
>> as a dynamic command on Armada 70x0 DB board. For this
>> purpose add it as a build-time selectable option, depending
>> on a new INCLUDE_TFTP_COMMAND parameter.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Platform/Marvell/Armada/Armada.dsc.inc | 4 ++++
>>  Platform/Marvell/Armada/Armada70x0.dsc | 1 +
>>  Platform/Marvell/Armada/Armada70x0.fdf | 3 +++
>>  3 files changed, 8 insertions(+)
>>
>> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
>> index 2a8a226..6040493 100644
>> --- a/Platform/Marvell/Armada/Armada.dsc.inc
>> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
>> @@ -525,6 +525,10 @@
>>        gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|8000
>>    }
>>
>> +!ifdef $(INCLUDE_TFTP_COMMAND)
>> +  ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
>> +!endif #$(INCLUDE_TFTP_COMMAND)
>> +
>>  [BuildOptions.common.EDKII.DXE_CORE,BuildOptions.common.EDKII.DXE_DRIVER,BuildOptions.common.EDKII.UEFI_DRIVER,BuildOptions.common.EDKII.UEFI_APPLICATION]
>>    GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
>>
>> diff --git a/Platform/Marvell/Armada/Armada70x0.dsc b/Platform/Marvell/Armada/Armada70x0.dsc
>> index 8e4cdb2..4e7d43c 100644
>> --- a/Platform/Marvell/Armada/Armada70x0.dsc
>> +++ b/Platform/Marvell/Armada/Armada70x0.dsc
>> @@ -44,6 +44,7 @@
>>    BUILD_TARGETS                  = DEBUG|RELEASE
>>    SKUID_IDENTIFIER               = DEFAULT
>>    FLASH_DEFINITION               = Platform/Marvell/Armada/Armada70x0.fdf
>> +  DEFINE INCLUDE_TFTP_COMMAND    = 1
>
> This looks very much to me like it will always include the tftp
> command, which is not what I wanted.
>
> Then again, you have done _exactly_ what I requested in emulating
> what the Hisilicon platforms do (and will shortly stop doing).

I took a look at Platform/Hisilicon/D02/Pv660D02.dsc and took the
DEFINE method from there.

>
> My thoughts were that, since this is a debug/development feature, it
> would be enabled at build time by adding -D INCLUDE_TFTP_COMMAND to
> the build command line.
>

It wasn't clear to me in such way :)

> As such, do you have any issues with me deleting this hunk before
> pushing? If not:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Sure, please remove this line.

Best regards,
Marcin

>>
>>  !include Armada.dsc.inc
>>
>> diff --git a/Platform/Marvell/Armada/Armada70x0.fdf b/Platform/Marvell/Armada/Armada70x0.fdf
>> index ca92c60..c03bc42 100644
>> --- a/Platform/Marvell/Armada/Armada70x0.fdf
>> +++ b/Platform/Marvell/Armada/Armada70x0.fdf
>> @@ -176,6 +176,9 @@ FvNameGuid         = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
>>
>>    # UEFI application (Shell Embedded Boot Loader)
>>    INF ShellPkg/Application/Shell/Shell.inf
>> +!ifdef $(INCLUDE_TFTP_COMMAND)
>> +  INF ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
>> +!endif #$(INCLUDE_TFTP_COMMAND)
>>
>>    # Bds
>>    INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>> --
>> 2.7.4
>>

Re: [platforms: PATCH v2 3/4] Marvell/Applications: FirmwareUpdate: Fix usage information

[Marcin Wojtas]

2017-12-04 14:52 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Fri, Dec 01, 2017 at 04:35:06PM +0100, Marcin Wojtas wrote:
>> fupdate command's usage information referred to a deprecated
>> '-f' flag in 'examples' section. It was a residue from the
>> initial version of the application, removed during review
>> before merging to upstream branch. Correct the help information.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>
> I boggled slightly at how the file with deleted content is larger than
> the original, so went to have a look, and what I see is:
>
> --- pre.txt     2017-12-04 13:43:32.985446108 +0000
> +++ post.txt    2017-12-04 13:44:30.433120594 +0000
> @@ -50,10 +50,10 @@
>  ".SH EXAMPLES\r\n"
>  " \r\n"
>  "EXAMPLES:\r\n"
> -"Update firmware from file fs2:flash-image.bin\r\n"
> -"  fupdate -f fs2:flash-image.bin\r\n"
> +"Update firmware in SPI flash from file fs2:flash-image.bin\r\n"
> +"  fupdate fs2:flash-image.bin\r\n"
>  ".SH RETURNVALUES\r\n"
>  " \r\n"
>  "RETURN VALUES:\r\n"
>  "  SHELL_SUCCESS             The action was completed as requested.\r\n"
> -"  SHELL_ABORTED Error while processing command\r\n"
> +"  SHELL_ABORTED             Error while processing command\r\n"
>
>
> Now, I don't mind random format cleanup as much in .uni files as
> elsewhere, since we don't get sane diffs anyway. But that also makes
> detailed and accurate commit messages much more important.
>
> So please, add something about the rewording and the whitespace fixes.
>

Sure, will do.

Thanks,
Marcin

Re: [platforms: PATCH v2 4/4] Marvell/Drivers: MvPhyDxe: Cleanup the header

[Marcin Wojtas]

Hi Leif,



2017-12-04 15:00 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Fri, Dec 01, 2017 at 04:35:07PM +0100, Marcin Wojtas wrote:
>> This patch removes unused macros defined in MvPhyDxe.h, as well
>> as improves the style and comments. Pick single definition
>> of the autonegotiation timeout - two different macros were used
>> for the same purpose.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c |   2 +-
>>  Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h | 152 ++++----------------
>>  2 files changed, 31 insertions(+), 123 deletions(-)
>>
>> diff --git a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
>> index e776a91..dd2edae 100644
>> --- a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
>> +++ b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
>> @@ -328,7 +328,7 @@ MvPhyInit1512 (
>>
>>      DEBUG((DEBUG_ERROR, "MvPhyDxe: Waiting for PHY auto negotiation... "));
>>      for (i = 0; !(Data & BMSR_ANEGCOMPLETE); i++) {
>> -      if (i > PHY_ANEG_TIMEOUT) {
>> +      if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
>
> I think you've had a git accident somewhere:
> The existing line upstream is still
>          if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
> This hunk should have just dropped out.
>

No, it's ok. It just turned out, there were two concurrent definitions
of the same thing. Please take a look at what's available on top of
the master branch:

Header:
[...]
#define PHY_ANEG_TIMEOUT 4000

#define PHY_INTERFACE_MODE_RGMII 0
#define PHY_INTERFACE_MODE_RGMII_ID 1
#define PHY_INTERFACE_MODE_RGMII_RXID 2
#define PHY_INTERFACE_MODE_RGMII_TXID 3
#define PHY_INTERFACE_MODE_SGMII 4
#define PHY_INTERFACE_MODE_RTBI 5

#define PHY_AUTONEGOTIATE_TIMEOUT 5000
[...]

Driver:
~Line 204:
    DEBUG((DEBUG_ERROR,"MvPhyDxe: Waiting for PHY realtime link"));
    while (!(Data & MIIM_88E1xxx_PHYSTAT_SPDDONE)) {
      if (i > PHY_AUTONEGOTIATE_TIMEOUT) {

~Line 327:
    DEBUG((DEBUG_ERROR, "MvPhyDxe: Waiting for PHY auto negotiation... "));
    for (i = 0; !(Data & BMSR_ANEGCOMPLETE); i++) {
      if (i > PHY_ANEG_TIMEOUT) {
        DEBUG((DEBUG_ERROR, "timeout\n"));

Best regards,
Marcin

Re: [platforms: PATCH v2 4/4] Marvell/Drivers: MvPhyDxe: Cleanup the header

[Leif Lindholm]

On Mon, Dec 04, 2017 at 04:22:33PM +0100, Marcin Wojtas wrote:
> Hi Leif,
> 
> 
> 
> 2017-12-04 15:00 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> > On Fri, Dec 01, 2017 at 04:35:07PM +0100, Marcin Wojtas wrote:
> >> This patch removes unused macros defined in MvPhyDxe.h, as well
> >> as improves the style and comments. Pick single definition
> >> of the autonegotiation timeout - two different macros were used
> >> for the same purpose.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> >> ---
> >>  Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c |   2 +-
> >>  Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h | 152 ++++----------------
> >>  2 files changed, 31 insertions(+), 123 deletions(-)
> >>
> >> diff --git a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
> >> index e776a91..dd2edae 100644
> >> --- a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
> >> +++ b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
> >> @@ -328,7 +328,7 @@ MvPhyInit1512 (
> >>
> >>      DEBUG((DEBUG_ERROR, "MvPhyDxe: Waiting for PHY auto negotiation... "));
> >>      for (i = 0; !(Data & BMSR_ANEGCOMPLETE); i++) {
> >> -      if (i > PHY_ANEG_TIMEOUT) {
> >> +      if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
> >
> > I think you've had a git accident somewhere:
> > The existing line upstream is still
> >          if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
> > This hunk should have just dropped out.
> >
> 
> No, it's ok. It just turned out, there were two concurrent definitions
> of the same thing. Please take a look at what's available on top of
> the master branch:

Aah, that's not the same location as in your v1, is it? :)

*grumble*

Sure:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> Header:
> [...]
> #define PHY_ANEG_TIMEOUT 4000
> 
> #define PHY_INTERFACE_MODE_RGMII 0
> #define PHY_INTERFACE_MODE_RGMII_ID 1
> #define PHY_INTERFACE_MODE_RGMII_RXID 2
> #define PHY_INTERFACE_MODE_RGMII_TXID 3
> #define PHY_INTERFACE_MODE_SGMII 4
> #define PHY_INTERFACE_MODE_RTBI 5
> 
> #define PHY_AUTONEGOTIATE_TIMEOUT 5000
> [...]
> 
> Driver:
> ~Line 204:
>     DEBUG((DEBUG_ERROR,"MvPhyDxe: Waiting for PHY realtime link"));
>     while (!(Data & MIIM_88E1xxx_PHYSTAT_SPDDONE)) {
>       if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
> 
> ~Line 327:
>     DEBUG((DEBUG_ERROR, "MvPhyDxe: Waiting for PHY auto negotiation... "));
>     for (i = 0; !(Data & BMSR_ANEGCOMPLETE); i++) {
>       if (i > PHY_ANEG_TIMEOUT) {
>         DEBUG((DEBUG_ERROR, "timeout\n"));
> 
> Best regards,
> Marcin

Re: [platforms: PATCH v2 4/4] Marvell/Drivers: MvPhyDxe: Cleanup the header

[Marcin Wojtas]

2017-12-04 17:10 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Mon, Dec 04, 2017 at 04:22:33PM +0100, Marcin Wojtas wrote:
>> Hi Leif,
>>
>>
>>
>> 2017-12-04 15:00 GMT+01:00 Leif Lindholm <leif.lindholm@linaro.org>:
>> > On Fri, Dec 01, 2017 at 04:35:07PM +0100, Marcin Wojtas wrote:
>> >> This patch removes unused macros defined in MvPhyDxe.h, as well
>> >> as improves the style and comments. Pick single definition
>> >> of the autonegotiation timeout - two different macros were used
>> >> for the same purpose.
>> >>
>> >> Contributed-under: TianoCore Contribution Agreement 1.1
>> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> >> ---
>> >>  Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c |   2 +-
>> >>  Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h | 152 ++++----------------
>> >>  2 files changed, 31 insertions(+), 123 deletions(-)
>> >>
>> >> diff --git a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
>> >> index e776a91..dd2edae 100644
>> >> --- a/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
>> >> +++ b/Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c
>> >> @@ -328,7 +328,7 @@ MvPhyInit1512 (
>> >>
>> >>      DEBUG((DEBUG_ERROR, "MvPhyDxe: Waiting for PHY auto negotiation... "));
>> >>      for (i = 0; !(Data & BMSR_ANEGCOMPLETE); i++) {
>> >> -      if (i > PHY_ANEG_TIMEOUT) {
>> >> +      if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
>> >
>> > I think you've had a git accident somewhere:
>> > The existing line upstream is still
>> >          if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
>> > This hunk should have just dropped out.
>> >
>>
>> No, it's ok. It just turned out, there were two concurrent definitions
>> of the same thing. Please take a look at what's available on top of
>> the master branch:
>
> Aah, that's not the same location as in your v1, is it? :)
>

Indeed :)

Thanks,
Marcin

> *grumble*
>
> Sure:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
>> Header:
>> [...]
>> #define PHY_ANEG_TIMEOUT 4000
>>
>> #define PHY_INTERFACE_MODE_RGMII 0
>> #define PHY_INTERFACE_MODE_RGMII_ID 1
>> #define PHY_INTERFACE_MODE_RGMII_RXID 2
>> #define PHY_INTERFACE_MODE_RGMII_TXID 3
>> #define PHY_INTERFACE_MODE_SGMII 4
>> #define PHY_INTERFACE_MODE_RTBI 5
>>
>> #define PHY_AUTONEGOTIATE_TIMEOUT 5000
>> [...]
>>
>> Driver:
>> ~Line 204:
>>     DEBUG((DEBUG_ERROR,"MvPhyDxe: Waiting for PHY realtime link"));
>>     while (!(Data & MIIM_88E1xxx_PHYSTAT_SPDDONE)) {
>>       if (i > PHY_AUTONEGOTIATE_TIMEOUT) {
>>
>> ~Line 327:
>>     DEBUG((DEBUG_ERROR, "MvPhyDxe: Waiting for PHY auto negotiation... "));
>>     for (i = 0; !(Data & BMSR_ANEGCOMPLETE); i++) {
>>       if (i > PHY_ANEG_TIMEOUT) {
>>         DEBUG((DEBUG_ERROR, "timeout\n"));
>>
>> Best regards,
>> Marcin