diff mbox series

[2/7] aspeed_sdmc: Fix saved values

Message ID 20180807075757.7242-3-joel@jms.id.au
State Accepted
Commit d131bc28a6094191471fb935a0535ae5a4df4ab3
Headers show
Series arm: aspeed: Extend SDRAM controller | expand

Commit Message

Joel Stanley Aug. 7, 2018, 7:57 a.m. UTC
This fixes the intended protection of read-only values in the
configuration register. They were being always set to zero by mistake.

The read-only fields depend on the configured memory size of the system,
so they cannot be fixed at compile time. The most straight forward
option was to store them in the state structure.

Signed-off-by: Joel Stanley <joel@jms.id.au>

---
 hw/misc/aspeed_sdmc.c         | 27 ++++++++-------------------
 include/hw/misc/aspeed_sdmc.h |  1 +
 2 files changed, 9 insertions(+), 19 deletions(-)

-- 
2.17.1

Comments

Cédric Le Goater Aug. 7, 2018, 9:55 a.m. UTC | #1
On 08/07/2018 09:57 AM, Joel Stanley wrote:
> This fixes the intended protection of read-only values in the

> configuration register. They were being always set to zero by mistake.


yes :/

> The read-only fields depend on the configured memory size of the system,

> so they cannot be fixed at compile time. The most straight forward

> option was to store them in the state structure.


We could also use an array of init values for registers, like SCU does, but
this is fine for now.  

> Signed-off-by: Joel Stanley <joel@jms.id.au>



Reviewed-by: Cédric Le Goater <clg@kaod.org>


Thanks,

C.

> ---

>  hw/misc/aspeed_sdmc.c         | 27 ++++++++-------------------

>  include/hw/misc/aspeed_sdmc.h |  1 +

>  2 files changed, 9 insertions(+), 19 deletions(-)

> 

> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c

> index 0df008e52a18..24fd4aee2d82 100644

> --- a/hw/misc/aspeed_sdmc.c

> +++ b/hw/misc/aspeed_sdmc.c

> @@ -126,10 +126,12 @@ static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,

>          case AST2400_A0_SILICON_REV:

>          case AST2400_A1_SILICON_REV:

>              data &= ~ASPEED_SDMC_READONLY_MASK;

> +            data |= s->fixed_conf;

>              break;

>          case AST2500_A0_SILICON_REV:

>          case AST2500_A1_SILICON_REV:

>              data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;

> +            data |= s->fixed_conf;

>              break;

>          default:

>              g_assert_not_reached();

> @@ -198,25 +200,7 @@ static void aspeed_sdmc_reset(DeviceState *dev)

>      memset(s->regs, 0, sizeof(s->regs));

>  

>      /* Set ram size bit and defaults values */

> -    switch (s->silicon_rev) {

> -    case AST2400_A0_SILICON_REV:

> -    case AST2400_A1_SILICON_REV:

> -        s->regs[R_CONF] |=

> -            ASPEED_SDMC_VGA_COMPAT |

> -            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);

> -        break;

> -

> -    case AST2500_A0_SILICON_REV:

> -    case AST2500_A1_SILICON_REV:

> -        s->regs[R_CONF] |=

> -            ASPEED_SDMC_HW_VERSION(1) |

> -            ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |

> -            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);

> -        break;

> -

> -    default:

> -        g_assert_not_reached();

> -    }

> +    s->regs[R_CONF] = s->fixed_conf;

>  }

>  

>  static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)

> @@ -234,10 +218,15 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)

>      case AST2400_A0_SILICON_REV:

>      case AST2400_A1_SILICON_REV:

>          s->ram_bits = ast2400_rambits(s);

> +        s->fixed_conf = ASPEED_SDMC_VGA_COMPAT |

> +            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);

>          break;

>      case AST2500_A0_SILICON_REV:

>      case AST2500_A1_SILICON_REV:

>          s->ram_bits = ast2500_rambits(s);

> +        s->fixed_conf = ASPEED_SDMC_HW_VERSION(1) |

> +            ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |

> +            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);

>          break;

>      default:

>          g_assert_not_reached();

> diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h

> index 682f0f5d56dc..e079c66a7d73 100644

> --- a/include/hw/misc/aspeed_sdmc.h

> +++ b/include/hw/misc/aspeed_sdmc.h

> @@ -27,6 +27,7 @@ typedef struct AspeedSDMCState {

>      uint32_t silicon_rev;

>      uint32_t ram_bits;

>      uint64_t ram_size;

> +    uint32_t fixed_conf;

>  

>  } AspeedSDMCState;

>  

>
diff mbox series

Patch

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 0df008e52a18..24fd4aee2d82 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -126,10 +126,12 @@  static void aspeed_sdmc_write(void *opaque, hwaddr addr, uint64_t data,
         case AST2400_A0_SILICON_REV:
         case AST2400_A1_SILICON_REV:
             data &= ~ASPEED_SDMC_READONLY_MASK;
+            data |= s->fixed_conf;
             break;
         case AST2500_A0_SILICON_REV:
         case AST2500_A1_SILICON_REV:
             data &= ~ASPEED_SDMC_AST2500_READONLY_MASK;
+            data |= s->fixed_conf;
             break;
         default:
             g_assert_not_reached();
@@ -198,25 +200,7 @@  static void aspeed_sdmc_reset(DeviceState *dev)
     memset(s->regs, 0, sizeof(s->regs));
 
     /* Set ram size bit and defaults values */
-    switch (s->silicon_rev) {
-    case AST2400_A0_SILICON_REV:
-    case AST2400_A1_SILICON_REV:
-        s->regs[R_CONF] |=
-            ASPEED_SDMC_VGA_COMPAT |
-            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
-        break;
-
-    case AST2500_A0_SILICON_REV:
-    case AST2500_A1_SILICON_REV:
-        s->regs[R_CONF] |=
-            ASPEED_SDMC_HW_VERSION(1) |
-            ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
-            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
-        break;
-
-    default:
-        g_assert_not_reached();
-    }
+    s->regs[R_CONF] = s->fixed_conf;
 }
 
 static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
@@ -234,10 +218,15 @@  static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
     case AST2400_A0_SILICON_REV:
     case AST2400_A1_SILICON_REV:
         s->ram_bits = ast2400_rambits(s);
+        s->fixed_conf = ASPEED_SDMC_VGA_COMPAT |
+            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
         break;
     case AST2500_A0_SILICON_REV:
     case AST2500_A1_SILICON_REV:
         s->ram_bits = ast2500_rambits(s);
+        s->fixed_conf = ASPEED_SDMC_HW_VERSION(1) |
+            ASPEED_SDMC_VGA_APERTURE(ASPEED_SDMC_VGA_64MB) |
+            ASPEED_SDMC_DRAM_SIZE(s->ram_bits);
         break;
     default:
         g_assert_not_reached();
diff --git a/include/hw/misc/aspeed_sdmc.h b/include/hw/misc/aspeed_sdmc.h
index 682f0f5d56dc..e079c66a7d73 100644
--- a/include/hw/misc/aspeed_sdmc.h
+++ b/include/hw/misc/aspeed_sdmc.h
@@ -27,6 +27,7 @@  typedef struct AspeedSDMCState {
     uint32_t silicon_rev;
     uint32_t ram_bits;
     uint64_t ram_size;
+    uint32_t fixed_conf;
 
 } AspeedSDMCState;