diff mbox series

[v3] hw/arm/aspeed: Unlock SCU when running kernel

Message ID 20171114122018.12204-1-joel@jms.id.au
State Accepted
Commit b6e70d1d7fea681306a3c8e74934b66dc9524969
Headers show
Series [v3] hw/arm/aspeed: Unlock SCU when running kernel | expand

Commit Message

Joel Stanley Nov. 14, 2017, 12:20 p.m. UTC
The ASPEED hardware contains a lock register for the SCU that disables
any writes to the SCU when it is locked. The machine comes up with the
lock enabled, but on all known hardware u-boot will unlock it and leave
it unlocked when loading the kernel.

This means the kernel expects the SCU to be unlocked. When booting from
an emulated ROM the normal u-boot unlock path is executed. Things don't
go well when booting using the -kernel command line, as u-boot does not
run first.

Change behaviour so that when a kernel is passed to the machine, set the
reset value of the SCU to be unlocked.

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

---
v3:
 - remove unused member from AspeedBoardConfig
v2:
 - use the correct value for the key
 - rename it ASPEED_SCU_PROT_KEY
---
 hw/arm/aspeed.c              | 9 +++++++++
 hw/arm/aspeed_soc.c          | 2 ++
 hw/misc/aspeed_scu.c         | 5 +++--
 include/hw/misc/aspeed_scu.h | 3 +++
 4 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.14.1

Comments

Cédric Le Goater Nov. 15, 2017, 8:18 a.m. UTC | #1
On 11/14/2017 01:20 PM, Joel Stanley wrote:
> The ASPEED hardware contains a lock register for the SCU that disables

> any writes to the SCU when it is locked. The machine comes up with the

> lock enabled, but on all known hardware u-boot will unlock it and leave

> it unlocked when loading the kernel.

> 

> This means the kernel expects the SCU to be unlocked. When booting from

> an emulated ROM the normal u-boot unlock path is executed. Things don't

> go well when booting using the -kernel command line, as u-boot does not

> run first.

> 

> Change behaviour so that when a kernel is passed to the machine, set the

> reset value of the SCU to be unlocked.

> 

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


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


Thanks,

C.


> ---

> v3:

>  - remove unused member from AspeedBoardConfig

> v2:

>  - use the correct value for the key

>  - rename it ASPEED_SCU_PROT_KEY

> ---

>  hw/arm/aspeed.c              | 9 +++++++++

>  hw/arm/aspeed_soc.c          | 2 ++

>  hw/misc/aspeed_scu.c         | 5 +++--

>  include/hw/misc/aspeed_scu.h | 3 +++

>  4 files changed, 17 insertions(+), 2 deletions(-)

> 

> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c

> index ab895ad490af..7088c907bd23 100644

> --- a/hw/arm/aspeed.c

> +++ b/hw/arm/aspeed.c

> @@ -186,6 +186,15 @@ static void aspeed_board_init(MachineState *machine,

>                              &error_abort);

>      object_property_set_int(OBJECT(&bmc->soc), cfg->num_cs, "num-cs",

>                              &error_abort);

> +    if (machine->kernel_filename) {

> +        /*

> +         * When booting with a -kernel command line there is no u-boot

> +         * that runs to unlock the SCU. In this case set the default to

> +         * be unlocked as the kernel expects

> +         */

> +        object_property_set_int(OBJECT(&bmc->soc), ASPEED_SCU_PROT_KEY,

> +                                "hw-prot-key", &error_abort);

> +    }

>      object_property_set_bool(OBJECT(&bmc->soc), true, "realized",

>                               &error_abort);

>  

> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c

> index 5aa3d2ddd9cd..c83b7e207b86 100644

> --- a/hw/arm/aspeed_soc.c

> +++ b/hw/arm/aspeed_soc.c

> @@ -154,6 +154,8 @@ static void aspeed_soc_init(Object *obj)

>                                "hw-strap1", &error_abort);

>      object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu),

>                                "hw-strap2", &error_abort);

> +    object_property_add_alias(obj, "hw-prot-key", OBJECT(&s->scu),

> +                              "hw-prot-key", &error_abort);

>  

>      object_initialize(&s->fmc, sizeof(s->fmc), sc->info->fmc_typename);

>      object_property_add_child(obj, "fmc", OBJECT(&s->fmc), NULL);

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

> index 95022d3607ad..74537ce9755d 100644

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

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

> @@ -85,7 +85,6 @@

>  #define BMC_REV              TO_REG(0x19C)

>  #define BMC_DEV_ID           TO_REG(0x1A4)

>  

> -#define PROT_KEY_UNLOCK 0x1688A8A8

>  #define SCU_IO_REGION_SIZE 0x1000

>  

>  static const uint32_t ast2400_a0_resets[ASPEED_SCU_NR_REGS] = {

> @@ -192,7 +191,7 @@ static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data,

>      }

>  

>      if (reg > PROT_KEY && reg < CPU2_BASE_SEG1 &&

> -            s->regs[PROT_KEY] != PROT_KEY_UNLOCK) {

> +            s->regs[PROT_KEY] != ASPEED_SCU_PROT_KEY) {

>          qemu_log_mask(LOG_GUEST_ERROR, "%s: SCU is locked!\n", __func__);

>          return;

>      }

> @@ -246,6 +245,7 @@ static void aspeed_scu_reset(DeviceState *dev)

>      s->regs[SILICON_REV] = s->silicon_rev;

>      s->regs[HW_STRAP1] = s->hw_strap1;

>      s->regs[HW_STRAP2] = s->hw_strap2;

> +    s->regs[PROT_KEY] = s->hw_prot_key;

>  }

>  

>  static uint32_t aspeed_silicon_revs[] = {

> @@ -299,6 +299,7 @@ static Property aspeed_scu_properties[] = {

>      DEFINE_PROP_UINT32("silicon-rev", AspeedSCUState, silicon_rev, 0),

>      DEFINE_PROP_UINT32("hw-strap1", AspeedSCUState, hw_strap1, 0),

>      DEFINE_PROP_UINT32("hw-strap2", AspeedSCUState, hw_strap2, 0),

> +    DEFINE_PROP_UINT32("hw-prot-key", AspeedSCUState, hw_prot_key, 0),

>      DEFINE_PROP_END_OF_LIST(),

>  };

>  

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

> index bd4ac013f997..d70cc0aeca61 100644

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

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

> @@ -29,6 +29,7 @@ typedef struct AspeedSCUState {

>      uint32_t silicon_rev;

>      uint32_t hw_strap1;

>      uint32_t hw_strap2;

> +    uint32_t hw_prot_key;

>  } AspeedSCUState;

>  

>  #define AST2400_A0_SILICON_REV   0x02000303U

> @@ -38,6 +39,8 @@ typedef struct AspeedSCUState {

>  

>  extern bool is_supported_silicon_rev(uint32_t silicon_rev);

>  

> +#define ASPEED_SCU_PROT_KEY      0x1688A8A8

> +

>  /*

>   * Extracted from Aspeed SDK v00.03.21. Fixes and extra definitions

>   * were added.

>
Peter Maydell Nov. 15, 2017, 11:56 a.m. UTC | #2
On 15 November 2017 at 08:18, Cédric Le Goater <clg@kaod.org> wrote:
> On 11/14/2017 01:20 PM, Joel Stanley wrote:

>> The ASPEED hardware contains a lock register for the SCU that disables

>> any writes to the SCU when it is locked. The machine comes up with the

>> lock enabled, but on all known hardware u-boot will unlock it and leave

>> it unlocked when loading the kernel.

>>

>> This means the kernel expects the SCU to be unlocked. When booting from

>> an emulated ROM the normal u-boot unlock path is executed. Things don't

>> go well when booting using the -kernel command line, as u-boot does not

>> run first.

>>

>> Change behaviour so that when a kernel is passed to the machine, set the

>> reset value of the SCU to be unlocked.

>>

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

>

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


Thanks. Should this go into 2.11 ? It sounds like the kind of
bugfix that ought to...

-- PMM
Cédric Le Goater Nov. 15, 2017, 1:13 p.m. UTC | #3
On 11/15/2017 11:56 AM, Peter Maydell wrote:
> On 15 November 2017 at 08:18, Cédric Le Goater <clg@kaod.org> wrote:

>> On 11/14/2017 01:20 PM, Joel Stanley wrote:

>>> The ASPEED hardware contains a lock register for the SCU that disables

>>> any writes to the SCU when it is locked. The machine comes up with the

>>> lock enabled, but on all known hardware u-boot will unlock it and leave

>>> it unlocked when loading the kernel.

>>>

>>> This means the kernel expects the SCU to be unlocked. When booting from

>>> an emulated ROM the normal u-boot unlock path is executed. Things don't

>>> go well when booting using the -kernel command line, as u-boot does not

>>> run first.

>>>

>>> Change behaviour so that when a kernel is passed to the machine, set the

>>> reset value of the SCU to be unlocked.

>>>

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

>>

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

> 

> Thanks. Should this go into 2.11 ? It sounds like the kind of

> bugfix that ought to...


Yes. Please.

Thanks,

C.
diff mbox series

Patch

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index ab895ad490af..7088c907bd23 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -186,6 +186,15 @@  static void aspeed_board_init(MachineState *machine,
                             &error_abort);
     object_property_set_int(OBJECT(&bmc->soc), cfg->num_cs, "num-cs",
                             &error_abort);
+    if (machine->kernel_filename) {
+        /*
+         * When booting with a -kernel command line there is no u-boot
+         * that runs to unlock the SCU. In this case set the default to
+         * be unlocked as the kernel expects
+         */
+        object_property_set_int(OBJECT(&bmc->soc), ASPEED_SCU_PROT_KEY,
+                                "hw-prot-key", &error_abort);
+    }
     object_property_set_bool(OBJECT(&bmc->soc), true, "realized",
                              &error_abort);
 
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 5aa3d2ddd9cd..c83b7e207b86 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -154,6 +154,8 @@  static void aspeed_soc_init(Object *obj)
                               "hw-strap1", &error_abort);
     object_property_add_alias(obj, "hw-strap2", OBJECT(&s->scu),
                               "hw-strap2", &error_abort);
+    object_property_add_alias(obj, "hw-prot-key", OBJECT(&s->scu),
+                              "hw-prot-key", &error_abort);
 
     object_initialize(&s->fmc, sizeof(s->fmc), sc->info->fmc_typename);
     object_property_add_child(obj, "fmc", OBJECT(&s->fmc), NULL);
diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 95022d3607ad..74537ce9755d 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -85,7 +85,6 @@ 
 #define BMC_REV              TO_REG(0x19C)
 #define BMC_DEV_ID           TO_REG(0x1A4)
 
-#define PROT_KEY_UNLOCK 0x1688A8A8
 #define SCU_IO_REGION_SIZE 0x1000
 
 static const uint32_t ast2400_a0_resets[ASPEED_SCU_NR_REGS] = {
@@ -192,7 +191,7 @@  static void aspeed_scu_write(void *opaque, hwaddr offset, uint64_t data,
     }
 
     if (reg > PROT_KEY && reg < CPU2_BASE_SEG1 &&
-            s->regs[PROT_KEY] != PROT_KEY_UNLOCK) {
+            s->regs[PROT_KEY] != ASPEED_SCU_PROT_KEY) {
         qemu_log_mask(LOG_GUEST_ERROR, "%s: SCU is locked!\n", __func__);
         return;
     }
@@ -246,6 +245,7 @@  static void aspeed_scu_reset(DeviceState *dev)
     s->regs[SILICON_REV] = s->silicon_rev;
     s->regs[HW_STRAP1] = s->hw_strap1;
     s->regs[HW_STRAP2] = s->hw_strap2;
+    s->regs[PROT_KEY] = s->hw_prot_key;
 }
 
 static uint32_t aspeed_silicon_revs[] = {
@@ -299,6 +299,7 @@  static Property aspeed_scu_properties[] = {
     DEFINE_PROP_UINT32("silicon-rev", AspeedSCUState, silicon_rev, 0),
     DEFINE_PROP_UINT32("hw-strap1", AspeedSCUState, hw_strap1, 0),
     DEFINE_PROP_UINT32("hw-strap2", AspeedSCUState, hw_strap2, 0),
+    DEFINE_PROP_UINT32("hw-prot-key", AspeedSCUState, hw_prot_key, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/misc/aspeed_scu.h b/include/hw/misc/aspeed_scu.h
index bd4ac013f997..d70cc0aeca61 100644
--- a/include/hw/misc/aspeed_scu.h
+++ b/include/hw/misc/aspeed_scu.h
@@ -29,6 +29,7 @@  typedef struct AspeedSCUState {
     uint32_t silicon_rev;
     uint32_t hw_strap1;
     uint32_t hw_strap2;
+    uint32_t hw_prot_key;
 } AspeedSCUState;
 
 #define AST2400_A0_SILICON_REV   0x02000303U
@@ -38,6 +39,8 @@  typedef struct AspeedSCUState {
 
 extern bool is_supported_silicon_rev(uint32_t silicon_rev);
 
+#define ASPEED_SCU_PROT_KEY      0x1688A8A8
+
 /*
  * Extracted from Aspeed SDK v00.03.21. Fixes and extra definitions
  * were added.