[PATCH 0/2] Define NPCM7XX PWRON bit fields

[PATCH 0/2] Define NPCM7XX PWRON bit fields

[Hao Wu]

Currently, the PWRON STRAP values in NPCM7XX boards are magic
numbers. Similar to the aspeed ones in hw/arm/aspeed.c, we
define bit fields constants for them and use these fields instead
of the magic numbers in the current implementation. The code
should behave exactly the same as the existing one.

Hao Wu (2):
  hw/misc: Add PWRON STRAP bit fields in GCR module
  hw/arm: Use bit fields for NPCM7XX PWRON STRAPs

 hw/arm/npcm7xx_boards.c       | 24 +++++++++++++++++++-----
 include/hw/misc/npcm7xx_gcr.h | 30 ++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 5 deletions(-)

-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 1/2] hw/misc: Add PWRON STRAP bit fields in GCR module

[Hao Wu]

Similar to the Aspeed code in include/misc/aspeed_scu.h, we define
the PWRON STRAP fields in their corresponding module for NPCM7XX.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
Reviewed-by: Patrick Venture <venture@google.com>
---
 include/hw/misc/npcm7xx_gcr.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/include/hw/misc/npcm7xx_gcr.h b/include/hw/misc/npcm7xx_gcr.h
index 13109d9d32..98da5d171f 100644
--- a/include/hw/misc/npcm7xx_gcr.h
+++ b/include/hw/misc/npcm7xx_gcr.h
@@ -19,6 +19,36 @@
 #include "exec/memory.h"
 #include "hw/sysbus.h"
 
+/*
+ * NPCM7XX PWRON STRAP bit fields
+ * 12: SPI0 powered by VSBV3 at 1.8V
+ * 11: System flash attached to BMC
+ * 10: BSP alternative pins.
+ * 9:8: Flash UART command route enabled.
+ * 7: Security enabled.
+ * 6: HI-Z state control.
+ * 5: ECC disabled.
+ * 4: Reserved
+ * 3: JTAG2 enabled.
+ * 2:0: CPU and DRAM clock frequency.
+ */
+#define NPCM7XX_PWRON_STRAP_SPI0F18                 BIT(12)
+#define NPCM7XX_PWRON_STRAP_SFAB                    BIT(11)
+#define NPCM7XX_PWRON_STRAP_BSPA                    BIT(10)
+#define NPCM7XX_PWRON_STRAP_FUP(x)                  ((x) << 8)
+#define     FUP_NORM_UART2      3
+#define     FUP_PROG_UART3      2
+#define     FUP_PROG_UART2      1
+#define     FUP_NORM_UART3      0
+#define NPCM7XX_PWRON_STRAP_SECEN                   BIT(7)
+#define NPCM7XX_PWRON_STRAP_HIZ                     BIT(6)
+#define NPCM7XX_PWRON_STRAP_ECC                     BIT(5)
+#define NPCM7XX_PWRON_STRAP_RESERVE1                BIT(4)
+#define NPCM7XX_PWRON_STRAP_J2EN                    BIT(3)
+#define NPCM7XX_PWRON_STRAP_CKFRQ(x)                ((x) << 8)
+#define     CKFRQ_SKIPINIT      (0x000)
+#define     CKFRQ_DEFAULT       (0x111)
+
 /*
  * Number of registers in our device state structure. Don't change this without
  * incrementing the version_id in the vmstate.
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 2/2] hw/arm: Use bit fields for NPCM7XX PWRON STRAPs

[Hao Wu]

This patch uses the defined fields to describe PWRON STRAPs for
better readability.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
Reviewed-by: Patrick Venture <venture@google.com>
---
 hw/arm/npcm7xx_boards.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
index 0678a56156..62d4092657 100644
--- a/hw/arm/npcm7xx_boards.c
+++ b/hw/arm/npcm7xx_boards.c
@@ -30,11 +30,25 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/block-backend.h"
 
-#define NPCM750_EVB_POWER_ON_STRAPS 0x00001ff7
-#define QUANTA_GSJ_POWER_ON_STRAPS 0x00001fff
-#define QUANTA_GBS_POWER_ON_STRAPS 0x000017ff
-#define KUDO_BMC_POWER_ON_STRAPS 0x00001fff
-#define MORI_BMC_POWER_ON_STRAPS 0x00001fff
+#define NPCM7XX_POWER_ON_STRAPS_DEFAULT (           \
+        NPCM7XX_PWRON_STRAP_SPI0F18 |               \
+        NPCM7XX_PWRON_STRAP_SFAB |                  \
+        NPCM7XX_PWRON_STRAP_BSPA |                  \
+        NPCM7XX_PWRON_STRAP_FUP(FUP_NORM_UART2) |   \
+        NPCM7XX_PWRON_STRAP_SECEN |                 \
+        NPCM7XX_PWRON_STRAP_HIZ |                   \
+        NPCM7XX_PWRON_STRAP_ECC |                   \
+        NPCM7XX_PWRON_STRAP_RESERVE1 |              \
+        NPCM7XX_PWRON_STRAP_J2EN |                  \
+        NPCM7XX_PWRON_STRAP_CKFRQ(CKFRQ_DEFAULT))   \
+
+#define NPCM750_EVB_POWER_ON_STRAPS ( \
+        NPCM7XX_POWER_ON_STRAPS_DEFAULT & ~NPCM7XX_PWRON_STRAP_RESERVE1)
+#define QUANTA_GSJ_POWER_ON_STRAPS NPCM7XX_POWER_ON_STRAPS_DEFAULT
+#define QUANTA_GBS_POWER_ON_STRAPS ( \
+        NPCM7XX_POWER_ON_STRAPS_DEFAULT & ~NPCM7XX_PWRON_STRAP_SFAB)
+#define KUDO_BMC_POWER_ON_STRAPS NPCM7XX_POWER_ON_STRAPS_DEFAULT
+#define MORI_BMC_POWER_ON_STRAPS NPCM7XX_POWER_ON_STRAPS_DEFAULT
 
 static const char npcm7xx_default_bootrom[] = "npcm7xx_bootrom.bin";
 
-- 
2.35.1.1094.g7c7d902a7c-goog

Re: [PATCH 1/2] hw/misc: Add PWRON STRAP bit fields in GCR module

[Peter Maydell]

On Wed, 6 Apr 2022 at 23:34, Hao Wu <wuhaotsh@google.com> wrote:
>
> Similar to the Aspeed code in include/misc/aspeed_scu.h, we define
> the PWRON STRAP fields in their corresponding module for NPCM7XX.
>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> Reviewed-by: Patrick Venture <venture@google.com>
> ---
>  include/hw/misc/npcm7xx_gcr.h | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/include/hw/misc/npcm7xx_gcr.h b/include/hw/misc/npcm7xx_gcr.h
> index 13109d9d32..98da5d171f 100644
> --- a/include/hw/misc/npcm7xx_gcr.h
> +++ b/include/hw/misc/npcm7xx_gcr.h
> @@ -19,6 +19,36 @@
>  #include "exec/memory.h"
>  #include "hw/sysbus.h"
>
> +/*
> + * NPCM7XX PWRON STRAP bit fields
> + * 12: SPI0 powered by VSBV3 at 1.8V
> + * 11: System flash attached to BMC
> + * 10: BSP alternative pins.
> + * 9:8: Flash UART command route enabled.
> + * 7: Security enabled.
> + * 6: HI-Z state control.
> + * 5: ECC disabled.
> + * 4: Reserved
> + * 3: JTAG2 enabled.
> + * 2:0: CPU and DRAM clock frequency.
> + */
> +#define NPCM7XX_PWRON_STRAP_SPI0F18                 BIT(12)
> +#define NPCM7XX_PWRON_STRAP_SFAB                    BIT(11)
> +#define NPCM7XX_PWRON_STRAP_BSPA                    BIT(10)
> +#define NPCM7XX_PWRON_STRAP_FUP(x)                  ((x) << 8)
> +#define     FUP_NORM_UART2      3
> +#define     FUP_PROG_UART3      2
> +#define     FUP_PROG_UART2      1
> +#define     FUP_NORM_UART3      0
> +#define NPCM7XX_PWRON_STRAP_SECEN                   BIT(7)
> +#define NPCM7XX_PWRON_STRAP_HIZ                     BIT(6)
> +#define NPCM7XX_PWRON_STRAP_ECC                     BIT(5)
> +#define NPCM7XX_PWRON_STRAP_RESERVE1                BIT(4)
> +#define NPCM7XX_PWRON_STRAP_J2EN                    BIT(3)
> +#define NPCM7XX_PWRON_STRAP_CKFRQ(x)                ((x) << 8)

Comment says clock frequency is bits [2:0] but macro definition
puts them in bits [9:8]...

> +#define     CKFRQ_SKIPINIT      (0x000)
> +#define     CKFRQ_DEFAULT       (0x111)

These don't need parentheses around them.

thanks
-- PMM

Re: [PATCH 2/2] hw/arm: Use bit fields for NPCM7XX PWRON STRAPs

[Peter Maydell]

On Wed, 6 Apr 2022 at 23:34, Hao Wu <wuhaotsh@google.com> wrote:
>
> This patch uses the defined fields to describe PWRON STRAPs for
> better readability.
>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> Reviewed-by: Patrick Venture <venture@google.com>
> ---
>  hw/arm/npcm7xx_boards.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c
> index 0678a56156..62d4092657 100644
> --- a/hw/arm/npcm7xx_boards.c
> +++ b/hw/arm/npcm7xx_boards.c
> @@ -30,11 +30,25 @@
>  #include "sysemu/sysemu.h"
>  #include "sysemu/block-backend.h"
>
> -#define NPCM750_EVB_POWER_ON_STRAPS 0x00001ff7
> -#define QUANTA_GSJ_POWER_ON_STRAPS 0x00001fff
> -#define QUANTA_GBS_POWER_ON_STRAPS 0x000017ff
> -#define KUDO_BMC_POWER_ON_STRAPS 0x00001fff
> -#define MORI_BMC_POWER_ON_STRAPS 0x00001fff
> +#define NPCM7XX_POWER_ON_STRAPS_DEFAULT (           \
> +        NPCM7XX_PWRON_STRAP_SPI0F18 |               \
> +        NPCM7XX_PWRON_STRAP_SFAB |                  \
> +        NPCM7XX_PWRON_STRAP_BSPA |                  \
> +        NPCM7XX_PWRON_STRAP_FUP(FUP_NORM_UART2) |   \
> +        NPCM7XX_PWRON_STRAP_SECEN |                 \
> +        NPCM7XX_PWRON_STRAP_HIZ |                   \
> +        NPCM7XX_PWRON_STRAP_ECC |                   \
> +        NPCM7XX_PWRON_STRAP_RESERVE1 |              \
> +        NPCM7XX_PWRON_STRAP_J2EN |                  \
> +        NPCM7XX_PWRON_STRAP_CKFRQ(CKFRQ_DEFAULT))   \

You don't want the trailing '\' on this last line.

> +
> +#define NPCM750_EVB_POWER_ON_STRAPS ( \
> +        NPCM7XX_POWER_ON_STRAPS_DEFAULT & ~NPCM7XX_PWRON_STRAP_RESERVE1)

This was 0x00001ff7, but the new macro definition makes it
0x1fef (since NPCM7XX_POWER_ON_STRAPS_DEFAULT sets all bits [12:0]
and we then clear bit 4).

> +#define QUANTA_GSJ_POWER_ON_STRAPS NPCM7XX_POWER_ON_STRAPS_DEFAULT
> +#define QUANTA_GBS_POWER_ON_STRAPS ( \
> +        NPCM7XX_POWER_ON_STRAPS_DEFAULT & ~NPCM7XX_PWRON_STRAP_SFAB)
> +#define KUDO_BMC_POWER_ON_STRAPS NPCM7XX_POWER_ON_STRAPS_DEFAULT
> +#define MORI_BMC_POWER_ON_STRAPS NPCM7XX_POWER_ON_STRAPS_DEFAULT
>
>  static const char npcm7xx_default_bootrom[] = "npcm7xx_bootrom.bin";

thanks
-- PMM