Hi,

On 7/25/22 10:19, Philip Oberfichtner wrote:
To reduce code duplication, let the stm32 based DH boards use the common
code for setting up their MAC addresses.

Signed-off-by: Philip Oberfichtner <p...@denx.de>
Tested-by: Marek Vasut <ma...@denx.de>
Reviewed-by: Marek Vasut <ma...@denx.de>

---

Changes in v3:
         - Reviewed by Marek

Changes in v2:
         - convert to livetree (rebase on commit 5a605b7c86152)
         - Tested-by Marek

  board/dhelectronics/dh_stm32mp1/board.c | 102 +++++++++++-------------
  1 file changed, 45 insertions(+), 57 deletions(-)

diff --git a/board/dhelectronics/dh_stm32mp1/board.c 
b/board/dhelectronics/dh_stm32mp1/board.c
index 7a4c08cb7f..ab354e3e33 100644
--- a/board/dhelectronics/dh_stm32mp1/board.c
+++ b/board/dhelectronics/dh_stm32mp1/board.c
@@ -42,6 +42,7 @@
  #include <usb/dwc2_udc.h>
  #include <watchdog.h>
  #include <dm/ofnode.h>
+#include "../common/dh_common.h"
  #include "../../st/common/stpmic1.h"
/* SYSCFG registers */
@@ -84,36 +85,17 @@
  #define KS_CIDER      0xC0
  #define CIDER_ID      0x8870
-int setup_mac_address(void)
+static bool dh_stm32_mac_is_in_ks8851(void)
  {
-       unsigned char enetaddr[6];
-       bool skip_eth0 = false;
-       bool skip_eth1 = false;
-       struct udevice *dev;
-       int ret;
        ofnode node;
-
-       ret = eth_env_get_enetaddr("ethaddr", enetaddr);
-       if (ret)        /* ethaddr is already set */
-               skip_eth0 = true;
+       u32 reg, cider, ccr;
node = ofnode_path("ethernet1");
-       if (!ofnode_valid(node)) {
-               /* ethernet1 is not present in the system */
-               skip_eth1 = true;
-               goto out_set_ethaddr;
-       }
+       if (!ofnode_valid(node))
+               return false;
- ret = eth_env_get_enetaddr("eth1addr", enetaddr);
-       if (ret) {
-               /* eth1addr is already set */
-               skip_eth1 = true;
-               goto out_set_ethaddr;
-       }
-
-       ret = ofnode_device_is_compatible(node, "micrel,ks8851-mll");
-       if (ret)
-               goto out_set_ethaddr;
+       if (ofnode_device_is_compatible(node, "micrel,ks8851-mll"))
+               return false;
/*
         * KS8851 with EEPROM may use custom MAC from EEPROM, read
@@ -121,56 +103,62 @@ int setup_mac_address(void)
         * is present. If EEPROM is present, it must contain valid
         * MAC address.
         */
-       u32 reg, cider, ccr;
        reg = ofnode_get_addr(node);
        if (!reg)
-               goto out_set_ethaddr;
+               return false;
writew(KS_BE0 | KS_BE1 | KS_CIDER, reg + 2);
        cider = readw(reg);
-       if ((cider & 0xfff0) != CIDER_ID) {
-               skip_eth1 = true;
-               goto out_set_ethaddr;
-       }
+       if ((cider & 0xfff0) != CIDER_ID)
+               return true;
writew(KS_BE0 | KS_BE1 | KS_CCR, reg + 2);
        ccr = readw(reg);
-       if (ccr & KS_CCR_EEPROM) {
-               skip_eth1 = true;
-               goto out_set_ethaddr;
-       }
+       if (ccr & KS_CCR_EEPROM)
+               return true;
+
+       return false;
+}
-out_set_ethaddr:
-       if (skip_eth0 && skip_eth1)
+static int dh_stm32_setup_ethaddr(void)
+{
+       unsigned char enetaddr[6];
+
+       if (dh_mac_is_in_env("ethaddr"))
                return 0;
- node = ofnode_path("eeprom0");
-       if (!ofnode_valid(node)) {
-               printf("%s: No eeprom0 path offset\n", __func__);
-               return -ENOENT;
-       }
+       if (!dh_get_mac_from_eeprom(enetaddr, "eeprom0"))
+               return eth_env_set_enetaddr("ethaddr", enetaddr);
- ret = uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, node, &dev);
-       if (ret) {
-               printf("Cannot find EEPROM!\n");
-               return ret;
-       }
+       return -ENXIO;
+}
- ret = i2c_eeprom_read(dev, 0xfa, enetaddr, 0x6);
-       if (ret) {
-               printf("Error reading configuration EEPROM!\n");
-               return ret;
-       }
+static int dh_stm32_setup_eth1addr(void)
+{
+       unsigned char enetaddr[6];
- if (is_valid_ethaddr(enetaddr)) {
-               if (!skip_eth0)
-                       eth_env_set_enetaddr("ethaddr", enetaddr);
+       if (dh_mac_is_in_env("eth1addr"))
+               return 0;
+ if (dh_stm32_mac_is_in_ks8851())
+               return 0;
+
+       if (!dh_get_mac_from_eeprom(enetaddr, "eeprom0")) {
                enetaddr[5]++;
-               if (!skip_eth1)
-                       eth_env_set_enetaddr("eth1addr", enetaddr);
+               return eth_env_set_enetaddr("eth1addr", enetaddr);
        }
+ return -ENXIO;
+}
+
+int setup_mac_address(void)
+{
+       if (dh_stm32_setup_ethaddr())
+               printf("%s: Unable to setup ethaddr!\n", __func__);
+
+       if (dh_stm32_setup_eth1addr())
+               printf("%s: Unable to setup eth1addr!\n", __func__);
+
        return 0;
  }


Minor remarks on the last function:

   the 2 'printf' calls can be replaced by log_error() call (or log_debug ?)

    to correctly handle this trace with CONFIG_LOG_LEVEL

=> printf should avoid except in command result to handle log features during initialization


Anyway


Reviewed-by: Patrick Delaunay <patrick.delau...@foss.st.com>

Thanks
Patrick


Reply via email to