[PATCH 0/3] GE Bx50v3 cleanups

[PATCH 0/3] GE Bx50v3 cleanups

[Sebastian Reichel]

Here are some more cleanups for the GE Bx50v3 board(s):

PATCH 1+2: replace magic numbers by analyzing name string from
           the fitImage config node

PATCH 3: rely on PHY driver as much as possible

-- Sebastian

Sebastian Reichel (3):
  image: support board_fit_config_name_match
  board: ge: bx50v3: remove confidx magic numbers
  board: ge: bx50v3: cleanup phy config

 arch/arm/dts/imx6q-ba16.dtsi | 11 ++++++++
 board/ge/bx50v3/bx50v3.c     | 51 ++++++++++++++++--------------------
 common/image-fit.c           |  7 +++++
 include/configs/ge_bx50v3.h  |  4 +--
 4 files changed, 42 insertions(+), 31 deletions(-)

-- 
2.29.2

[PATCH 1/3] image: support board_fit_config_name_match

[Sebastian Reichel]

Support reusing board_fit_config_name_match() to automatically
select a sensible default configuration for booting fitImages
using 'bootm'.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 common/image-fit.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/common/image-fit.c b/common/image-fit.c
index c82d4d8015f0..52c194222735 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1741,6 +1741,12 @@ int fit_conf_get_node(const void *fit, const char *conf_uname)
 	if (conf_uname == NULL) {
 		/* get configuration unit name from the default property */
 		debug("No configuration specified, trying default...\n");
+#if !defined(USE_HOSTCC) && defined(CONFIG_MULTI_DTB_FIT)
+		noffset = fit_find_config_node(fit);
+		if (noffset < 0)
+			return noffset;
+		conf_uname = fdt_get_name(fit, noffset, NULL);
+#else
 		conf_uname = (char *)fdt_getprop(fit, confs_noffset,
 						 FIT_DEFAULT_PROP, &len);
 		if (conf_uname == NULL) {
@@ -1748,6 +1754,7 @@ int fit_conf_get_node(const void *fit, const char *conf_uname)
 				      len);
 			return len;
 		}
+#endif
 		debug("Found default configuration: '%s'\n", conf_uname);
 	}
 
-- 
2.29.2

[PATCH 2/3] board: ge: bx50v3: remove confidx magic numbers

[Sebastian Reichel]

Instead of hardcoding index magic numbers in the board code,
also rely on board_fit_config_name_match choosing the right
config for the fitImage containing the kernel.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 board/ge/bx50v3/bx50v3.c    | 16 ++++++++++------
 include/configs/ge_bx50v3.h |  4 ++--
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/board/ge/bx50v3/bx50v3.c b/board/ge/bx50v3/bx50v3.c
index 4754647fb4ad..a3ae037a82ef 100644
--- a/board/ge/bx50v3/bx50v3.c
+++ b/board/ge/bx50v3/bx50v3.c
@@ -356,15 +356,12 @@ static void process_vpd(struct vpd_cache *vpd)
 
 	switch (vpd->product_id) {
 	case VPD_PRODUCT_B450:
-		env_set("confidx", "1");
 		i210_index = 1;
 		break;
 	case VPD_PRODUCT_B650:
-		env_set("confidx", "2");
 		i210_index = 1;
 		break;
 	case VPD_PRODUCT_B850:
-		env_set("confidx", "3");
 		i210_index = 2;
 		break;
 	}
@@ -554,16 +551,23 @@ int ft_board_setup(void *blob, struct bd_info *bd)
 
 int board_fit_config_name_match(const char *name)
 {
+	const char *machine = name;
+
 	if (!vpd.is_read)
 		return strcmp(name, "imx6q-bx50v3");
 
+	if (!strncmp(machine, "Boot ", 5))
+		machine += 5;
+	if (!strncmp(machine, "imx6q-", 6))
+		machine += 6;
+
 	switch (vpd.product_id) {
 	case VPD_PRODUCT_B450:
-		return strcmp(name, "imx6q-b450v3");
+		return strcasecmp(machine, "b450v3");
 	case VPD_PRODUCT_B650:
-		return strcmp(name, "imx6q-b650v3");
+		return strcasecmp(machine, "b650v3");
 	case VPD_PRODUCT_B850:
-		return strcmp(name, "imx6q-b850v3");
+		return strcasecmp(machine, "b850v3");
 	default:
 		return -1;
 	}
diff --git a/include/configs/ge_bx50v3.h b/include/configs/ge_bx50v3.h
index e5c580b3f910..2d854af9a06d 100644
--- a/include/configs/ge_bx50v3.h
+++ b/include/configs/ge_bx50v3.h
@@ -62,7 +62,7 @@
         "networkboot=" \
                 "run setnetworkboot; " \
                 "nfs ${loadaddr} /srv/nfs/fitImage; " \
-                "bootm ${loadaddr}#conf@${confidx}\0" \
+                "bootm ${loadaddr}\0" \
 
 #define CONFIG_NETWORKBOOTCOMMAND \
 	"run networkboot; " \
@@ -111,7 +111,7 @@
 	"doboot=" \
 		"echo Booting from ${dev}:${devnum}:${partnum} ...; " \
 		"run setargs; " \
-		"bootm ${loadaddr}#conf@${confidx}\0" \
+		"bootm ${loadaddr}\0" \
 	"tryboot=" \
 		"setenv partnum 1; run hasfirstboot || setenv partnum 2; " \
 		"run loadimage || run swappartitions && run loadimage || " \
-- 
2.29.2

[PATCH 3/3] board: ge: bx50v3: cleanup phy config

[Sebastian Reichel]

The current PHY rework does the following things:

1. Configure 125MHz clock
2. Setup the TX clock delay (RX is enabled by default),
3. Setup reserved bits to avoid voltage peak

The clock delays are nowadays already configured by the
PHY driver (in ar803x_delay_config). The code for that
can simply be dropped. The clock speed can also be
configured by the PHY driver by adding the device tree
property "qca,clk-out-frequency".

What is left is setting up the undocumented reserved bits
to avoid the voltage peak problem. I slightly improved its
documentation while updating the board's PHY rework code.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm/dts/imx6q-ba16.dtsi | 11 +++++++++++
 board/ge/bx50v3/bx50v3.c     | 35 ++++++++++++-----------------------
 2 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/arch/arm/dts/imx6q-ba16.dtsi b/arch/arm/dts/imx6q-ba16.dtsi
index 7d8f61f2fd79..9da2bb6e869e 100644
--- a/arch/arm/dts/imx6q-ba16.dtsi
+++ b/arch/arm/dts/imx6q-ba16.dtsi
@@ -174,6 +174,17 @@
 	pinctrl-0 = <&pinctrl_enet>;
 	phy-mode = "rgmii-id";
 	status = "okay";
+	phy-handle = <&phy0>;
+
+	mdio {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		phy0: ethernet-phy at 4 {
+			reg = <4>;
+			qca,clk-out-frequency = <125000000>;
+		};
+	};
 };
 
 &hdmi {
diff --git a/board/ge/bx50v3/bx50v3.c b/board/ge/bx50v3/bx50v3.c
index a3ae037a82ef..3ea9425fd1ea 100644
--- a/board/ge/bx50v3/bx50v3.c
+++ b/board/ge/bx50v3/bx50v3.c
@@ -47,6 +47,10 @@ DECLARE_GLOBAL_DATA_PTR;
 #define VPD_PRODUCT_B650 2
 #define VPD_PRODUCT_B450 3
 
+#define AR8033_DBG_REG_ADDR		0x1d
+#define AR8033_DBG_REG_DATA		0x1e
+#define AR8033_SERDES_REG		0x5
+
 static int productid;  /* Default to generic. */
 static struct vpd_cache vpd;
 
@@ -61,31 +65,16 @@ int dram_init(void)
 	return 0;
 }
 
-static int mx6_rgmii_rework(struct phy_device *phydev)
-{
-	/* Configure AR8033 to ouput a 125MHz clk from CLK_25M */
-	/* set device address 0x7 */
-	phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x7);
-	/* offset 0x8016: CLK_25M Clock Select */
-	phy_write(phydev, MDIO_DEVAD_NONE, 0xe, 0x8016);
-	/* enable register write, no post increment, address 0x7 */
-	phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x4007);
-	/* set to 125 MHz from local PLL source */
-	phy_write(phydev, MDIO_DEVAD_NONE, 0xe, 0x18);
-
-	/* rgmii tx clock delay enable */
-	/* set debug port address: SerDes Test and System Mode Control */
-	phy_write(phydev, MDIO_DEVAD_NONE, 0x1d, 0x05);
-	/* enable rgmii tx clock delay */
-	/* set the reserved bits to avoid board specific voltage peak issue*/
-	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, 0x3D47);
-
-	return 0;
-}
-
 int board_phy_config(struct phy_device *phydev)
 {
-	mx6_rgmii_rework(phydev);
+	/*
+	 * Set reserved bits to avoid board specific voltage peak issue. The
+	 * value is a magic number provided directly by Qualcomm. Note, that
+	 * PHY driver will take control of BIT(8) in this register to control
+	 * TX clock delay, so we do not initialize that bit here.
+	 */
+	phy_write(phydev, MDIO_DEVAD_NONE, AR8033_DBG_REG_ADDR, AR8033_SERDES_REG);
+	phy_write(phydev, MDIO_DEVAD_NONE, AR8033_DBG_REG_DATA, 0x3c47);
 
 	if (phydev->drv->config)
 		phydev->drv->config(phydev);
-- 
2.29.2

[PATCH 1/3] image: support board_fit_config_name_match

[Simon Glass]

Hi Sebastian,

On Tue, 8 Dec 2020 at 11:10, Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Support reusing board_fit_config_name_match() to automatically
> select a sensible default configuration for booting fitImages
> using 'bootm'.
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  common/image-fit.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/common/image-fit.c b/common/image-fit.c
> index c82d4d8015f0..52c194222735 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -1741,6 +1741,12 @@ int fit_conf_get_node(const void *fit, const char *conf_uname)
>         if (conf_uname == NULL) {
>                 /* get configuration unit name from the default property */
>                 debug("No configuration specified, trying default...\n");
> +#if !defined(USE_HOSTCC) && defined(CONFIG_MULTI_DTB_FIT)

Is there a way to use 'if IS_ENABLED() 'instead? here Perhaps we need
a new host_build() function in a suitable header file that returns the
value of USE_HOSTCC?

> +               noffset = fit_find_config_node(fit);
> +               if (noffset < 0)
> +                       return noffset;
> +               conf_uname = fdt_get_name(fit, noffset, NULL);
> +#else
>                 conf_uname = (char *)fdt_getprop(fit, confs_noffset,
>                                                  FIT_DEFAULT_PROP, &len);
>                 if (conf_uname == NULL) {
> @@ -1748,6 +1754,7 @@ int fit_conf_get_node(const void *fit, const char *conf_uname)
>                                       len);
>                         return len;
>                 }
> +#endif
>                 debug("Found default configuration: '%s'\n", conf_uname);
>         }
>
> --
> 2.29.2
>

Regards,
Simon

[PATCH 1/3] image: support board_fit_config_name_match

[Sebastian Reichel]

Hello Simon,

On Sat, Dec 12, 2020 at 08:39:45AM -0700, Simon Glass wrote:
> > +#if !defined(USE_HOSTCC) && defined(CONFIG_MULTI_DTB_FIT)
> 
> Is there a way to use 'if IS_ENABLED() 'instead? Here Perhaps we need
> a new host_build() function in a suitable header file that returns the
> value of USE_HOSTCC?

That should work. I suppose host_build() could look like this, or do
you have a better suggestion?

--------------------------------------------------------------
diff --git a/include/compiler.h b/include/compiler.h
index 90b7afae5376..27b9843497a4 100644
--- a/include/compiler.h
+++ b/include/compiler.h
@@ -6,6 +6,7 @@
 #define __COMPILER_H__
 
 #include <stddef.h>
+#include <stdbool.h>
 
 #ifdef USE_HOSTCC
 
@@ -150,4 +151,12 @@ typedef unsigned long int uintptr_t;
 #define MEM_SUPPORT_64BIT_DATA 0
 #endif
 
+static inline bool host_build(void) {
+#ifdef USE_HOSTCC
+       return true;
+#else
+       return false;
+#endif
+}
+
 #endif
--------------------------------------------------------------

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201212/86c31023/attachment.sig>

[PATCH 1/3] image: support board_fit_config_name_match

[Simon Glass]

Hi Sebastian,

On Sat, 12 Dec 2020 at 10:05, Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hello Simon,
>
> On Sat, Dec 12, 2020 at 08:39:45AM -0700, Simon Glass wrote:
> > > +#if !defined(USE_HOSTCC) && defined(CONFIG_MULTI_DTB_FIT)
> >
> > Is there a way to use 'if IS_ENABLED() 'instead? Here Perhaps we need
> > a new host_build() function in a suitable header file that returns the
> > value of USE_HOSTCC?
>
> That should work. I suppose host_build() could look like this, or do
> you have a better suggestion?

LGTM

- Simon

>
> --------------------------------------------------------------
> diff --git a/include/compiler.h b/include/compiler.h
> index 90b7afae5376..27b9843497a4 100644
> --- a/include/compiler.h
> +++ b/include/compiler.h
> @@ -6,6 +6,7 @@
>  #define __COMPILER_H__
>
>  #include <stddef.h>
> +#include <stdbool.h>
>
>  #ifdef USE_HOSTCC
>
> @@ -150,4 +151,12 @@ typedef unsigned long int uintptr_t;
>  #define MEM_SUPPORT_64BIT_DATA 0
>  #endif
>
> +static inline bool host_build(void) {
> +#ifdef USE_HOSTCC
> +       return true;
> +#else
> +       return false;
> +#endif
> +}
> +
>  #endif
> --------------------------------------------------------------
>
> -- Sebastian