diff mbox series

[06/13] hw/misc/mps2-scc: Factor out which-board conditionals

Message ID 20240206132931.38376-7-peter.maydell@linaro.org
State Superseded
Headers show
Series hw/arm: Implement new machine mps3-an536 (Cortex-R52 MPS3 AN536 FPGA image) | expand

Commit Message

Peter Maydell Feb. 6, 2024, 1:29 p.m. UTC 
The MPS SCC device has a lot of different flavours for the various
different MPS FPGA images, which look mostly similar but have
differences in how particular registers are handled.  Currently we
deal with this with a lot of open-coded checks on scc_partno(), but
as we add more board types this is getting a bit hard to read.

Factor out the conditions into some functions which we can
give more descriptive names to.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/mps2-scc.c | 45 +++++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 14 deletions(-)

Comments

Philippe Mathieu-Daudé Feb. 6, 2024, 3:56 p.m. UTC | #1 
Hi Peter,

On 6/2/24 14:29, Peter Maydell wrote:
> The MPS SCC device has a lot of different flavours for the various
> different MPS FPGA images, which look mostly similar but have
> differences in how particular registers are handled.  Currently we
> deal with this with a lot of open-coded checks on scc_partno(), but
> as we add more board types this is getting a bit hard to read.
> 
> Factor out the conditions into some functions which we can
> give more descriptive names to.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/misc/mps2-scc.c | 45 +++++++++++++++++++++++++++++++--------------
>   1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/misc/mps2-scc.c b/hw/misc/mps2-scc.c
> index 6c1b1cd3795..02a80bacd71 100644
> --- a/hw/misc/mps2-scc.c
> +++ b/hw/misc/mps2-scc.c
> @@ -59,6 +59,30 @@ static int scc_partno(MPS2SCC *s)
>       return extract32(s->id, 4, 8);
>   }
>   
> +/* Is CFG_REG2 present? */
> +static bool have_cfg2(MPS2SCC *s)
> +{
> +    return scc_partno(s) == 0x524 || scc_partno(s) == 0x547;
> +}
> +
> +/* Is CFG_REG3 present? */
> +static bool have_cfg3(MPS2SCC *s)
> +{
> +    return scc_partno(s) != 0x524 && scc_partno(s) != 0x547;
> +}
> +
> +/* Is CFG_REG5 present? */
> +static bool have_cfg5(MPS2SCC *s)
> +{
> +    return scc_partno(s) == 0x524 || scc_partno(s) == 0x547;
> +}
> +
> +/* Is CFG_REG6 present? */
> +static bool have_cfg6(MPS2SCC *s)
> +{
> +    return scc_partno(s) == 0x524;
> +}

I'd rather QOM-decline TYPE_MPS2_SCC per board and have
MPS2SCCClass::have_cfgX fields set in each class_init.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Richard Henderson Feb. 6, 2024, 9:47 p.m. UTC | #2 
On 2/6/24 23:29, Peter Maydell wrote:
> The MPS SCC device has a lot of different flavours for the various
> different MPS FPGA images, which look mostly similar but have
> differences in how particular registers are handled.  Currently we
> deal with this with a lot of open-coded checks on scc_partno(), but
> as we add more board types this is getting a bit hard to read.
> 
> Factor out the conditions into some functions which we can
> give more descriptive names to.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/misc/mps2-scc.c | 45 +++++++++++++++++++++++++++++++--------------
>   1 file changed, 31 insertions(+), 14 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Philippe Mathieu-Daudé Feb. 7, 2024, 8:47 a.m. UTC | #3 
On 6/2/24 14:29, Peter Maydell wrote:
> The MPS SCC device has a lot of different flavours for the various
> different MPS FPGA images, which look mostly similar but have
> differences in how particular registers are handled.  Currently we
> deal with this with a lot of open-coded checks on scc_partno(), but
> as we add more board types this is getting a bit hard to read.
> 
> Factor out the conditions into some functions which we can
> give more descriptive names to.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/misc/mps2-scc.c | 45 +++++++++++++++++++++++++++++++--------------
>   1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/misc/mps2-scc.c b/hw/misc/mps2-scc.c
> index 6c1b1cd3795..02a80bacd71 100644
> --- a/hw/misc/mps2-scc.c
> +++ b/hw/misc/mps2-scc.c
> @@ -59,6 +59,30 @@ static int scc_partno(MPS2SCC *s)
>       return extract32(s->id, 4, 8);
>   }
>   
> +/* Is CFG_REG2 present? */
> +static bool have_cfg2(MPS2SCC *s)
> +{
> +    return scc_partno(s) == 0x524 || scc_partno(s) == 0x547;
> +}
> +
> +/* Is CFG_REG3 present? */
> +static bool have_cfg3(MPS2SCC *s)
> +{
> +    return scc_partno(s) != 0x524 && scc_partno(s) != 0x547;
> +}

Looking at next patch and nitpicking, if we want to avoid
a switch() case use, this style is easier to review IMHO:

    static bool have_cfg2(MPS2SCC *s)
    {
        return    scc_partno(s) == 0x524
               || scc_partno(s) == 0x547;
    }

Anyway, already R-b.
diff mbox series

Patch

diff --git a/hw/misc/mps2-scc.c b/hw/misc/mps2-scc.c
index 6c1b1cd3795..02a80bacd71 100644
--- a/hw/misc/mps2-scc.c
+++ b/hw/misc/mps2-scc.c
@@ -59,6 +59,30 @@  static int scc_partno(MPS2SCC *s)
     return extract32(s->id, 4, 8);
 }
 
+/* Is CFG_REG2 present? */
+static bool have_cfg2(MPS2SCC *s)
+{
+    return scc_partno(s) == 0x524 || scc_partno(s) == 0x547;
+}
+
+/* Is CFG_REG3 present? */
+static bool have_cfg3(MPS2SCC *s)
+{
+    return scc_partno(s) != 0x524 && scc_partno(s) != 0x547;
+}
+
+/* Is CFG_REG5 present? */
+static bool have_cfg5(MPS2SCC *s)
+{
+    return scc_partno(s) == 0x524 || scc_partno(s) == 0x547;
+}
+
+/* Is CFG_REG6 present? */
+static bool have_cfg6(MPS2SCC *s)
+{
+    return scc_partno(s) == 0x524;
+}
+
 /* Handle a write via the SYS_CFG channel to the specified function/device.
  * Return false on error (reported to guest via SYS_CFGCTRL ERROR bit).
  */
@@ -111,15 +135,13 @@  static uint64_t mps2_scc_read(void *opaque, hwaddr offset, unsigned size)
         r = s->cfg1;
         break;
     case A_CFG2:
-        if (scc_partno(s) != 0x524 && scc_partno(s) != 0x547) {
-            /* CFG2 reserved on other boards */
+        if (!have_cfg2(s)) {
             goto bad_offset;
         }
         r = s->cfg2;
         break;
     case A_CFG3:
-        if (scc_partno(s) == 0x524 || scc_partno(s) == 0x547) {
-            /* CFG3 reserved on AN524 */
+        if (!have_cfg3(s)) {
             goto bad_offset;
         }
         /* These are user-settable DIP switches on the board. We don't
@@ -131,15 +153,13 @@  static uint64_t mps2_scc_read(void *opaque, hwaddr offset, unsigned size)
         r = s->cfg4;
         break;
     case A_CFG5:
-        if (scc_partno(s) != 0x524 && scc_partno(s) != 0x547) {
-            /* CFG5 reserved on other boards */
+        if (!have_cfg5(s)) {
             goto bad_offset;
         }
         r = s->cfg5;
         break;
     case A_CFG6:
-        if (scc_partno(s) != 0x524) {
-            /* CFG6 reserved on other boards */
+        if (!have_cfg6(s)) {
             goto bad_offset;
         }
         r = s->cfg6;
@@ -202,24 +222,21 @@  static void mps2_scc_write(void *opaque, hwaddr offset, uint64_t value,
         }
         break;
     case A_CFG2:
-        if (scc_partno(s) != 0x524 && scc_partno(s) != 0x547) {
-            /* CFG2 reserved on other boards */
+        if (!have_cfg2(s)) {
             goto bad_offset;
         }
         /* AN524: QSPI Select signal */
         s->cfg2 = value;
         break;
     case A_CFG5:
-        if (scc_partno(s) != 0x524 && scc_partno(s) != 0x547) {
-            /* CFG5 reserved on other boards */
+        if (!have_cfg5(s)) {
             goto bad_offset;
         }
         /* AN524: ACLK frequency in Hz */
         s->cfg5 = value;
         break;
     case A_CFG6:
-        if (scc_partno(s) != 0x524) {
-            /* CFG6 reserved on other boards */
+        if (!have_cfg6(s)) {
             goto bad_offset;
         }
         /* AN524: Clock divider for BRAM */