[v3] Add boot-mode property for zynq

[PATCH v3 2/3] hw/arm/xilinx_zynq: Add boot-mode property

Posted by Sai Pavan Boddu 5 months, 1 week ago

Read boot-mode value as machine property and propagate that to
SLCR.BOOT_MODE register.

Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@amd.com>
Acked-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
---
 hw/arm/xilinx_zynq.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 7f7a3d23fbe..39f07e6dfd8 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -38,6 +38,7 @@
 #include "qom/object.h"
 #include "exec/tswap.h"
 #include "target/arm/cpu-qom.h"
+#include "qapi/visitor.h"
 
 #define TYPE_ZYNQ_MACHINE MACHINE_TYPE_NAME("xilinx-zynq-a9")
 OBJECT_DECLARE_SIMPLE_TYPE(ZynqMachineState, ZYNQ_MACHINE)
@@ -90,6 +91,7 @@ struct ZynqMachineState {
     MachineState parent;
     Clock *ps_clk;
     ARMCPU *cpu[ZYNQ_MAX_CPUS];
+    uint8_t boot_mode;
 };
 
 static void zynq_write_board_setup(ARMCPU *cpu,
@@ -176,6 +178,27 @@ static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
     return unit;
 }
 
+static void zynq_set_boot_mode(Object *obj, const char *str,
+                                               Error **errp)
+{
+    ZynqMachineState *m = ZYNQ_MACHINE(obj);
+    uint8_t mode = 0;
+
+    if (!strcasecmp(str, "QSPI")) {
+        mode = 1;
+    } else if (!strcasecmp(str, "SD")) {
+        mode = 5;
+    } else if (!strcasecmp(str, "NOR")) {
+        mode = 2;
+    } else if (!strcasecmp(str, "JTAG")) {
+        mode = 0;
+    } else {
+        error_setg(errp, "bootmode %s not supported", str);
+        return;
+    }
+    m->boot_mode = mode;
+}
+
 static void zynq_init(MachineState *machine)
 {
     ZynqMachineState *zynq_machine = ZYNQ_MACHINE(machine);
@@ -241,6 +264,7 @@ static void zynq_init(MachineState *machine)
     /* Create slcr, keep a pointer to connect clocks */
     slcr = qdev_new("xilinx-zynq_slcr");
     qdev_connect_clock_in(slcr, "ps_clk", zynq_machine->ps_clk);
+    qdev_prop_set_uint8(slcr, "boot-mode", zynq_machine->boot_mode);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(slcr), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000);
 
@@ -372,6 +396,7 @@ static void zynq_machine_class_init(ObjectClass *oc, void *data)
         NULL
     };
     MachineClass *mc = MACHINE_CLASS(oc);
+    ObjectProperty *prop;
     mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
     mc->init = zynq_init;
     mc->max_cpus = ZYNQ_MAX_CPUS;
@@ -379,6 +404,12 @@ static void zynq_machine_class_init(ObjectClass *oc, void *data)
     mc->ignore_memory_transaction_failures = true;
     mc->valid_cpu_types = valid_cpu_types;
     mc->default_ram_id = "zynq.ext_ram";
+    prop = object_class_property_add_str(oc, "boot-mode", NULL,
+                              zynq_set_boot_mode);
+    object_class_property_set_description(oc, "boot-mode",
+                                          "Supported boot modes:"
+                                          " JTAG QSPI SD NOR");
+    object_property_set_default_str(prop, "QSPI");
 }
 
 static const TypeInfo zynq_machine_type = {
-- 
2.34.1

Re: [PATCH v3 2/3] hw/arm/xilinx_zynq: Add boot-mode property

Posted by Francisco Iglesias 5 months ago

Hi Sai,

Some optional suggestions below and minor indentation correction. 

On Thu, Jun 20, 2024 at 04:11:38PM +0530, Sai Pavan Boddu wrote:
> Read boot-mode value as machine property and propagate that to
> SLCR.BOOT_MODE register.
> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@amd.com>
> Acked-by: Edgar E. Iglesias <edgar.iglesias@amd.com>
> ---
>  hw/arm/xilinx_zynq.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
> index 7f7a3d23fbe..39f07e6dfd8 100644
> --- a/hw/arm/xilinx_zynq.c
> +++ b/hw/arm/xilinx_zynq.c
> @@ -38,6 +38,7 @@
>  #include "qom/object.h"
>  #include "exec/tswap.h"
>  #include "target/arm/cpu-qom.h"
> +#include "qapi/visitor.h"
>  
>  #define TYPE_ZYNQ_MACHINE MACHINE_TYPE_NAME("xilinx-zynq-a9")
>  OBJECT_DECLARE_SIMPLE_TYPE(ZynqMachineState, ZYNQ_MACHINE)
> @@ -90,6 +91,7 @@ struct ZynqMachineState {
>      MachineState parent;
>      Clock *ps_clk;
>      ARMCPU *cpu[ZYNQ_MAX_CPUS];
> +    uint8_t boot_mode;
>  };
>  
>  static void zynq_write_board_setup(ARMCPU *cpu,
> @@ -176,6 +178,27 @@ static inline int zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
>      return unit;
>  }
>  
> +static void zynq_set_boot_mode(Object *obj, const char *str,
> +                                               Error **errp)
> +{
> +    ZynqMachineState *m = ZYNQ_MACHINE(obj);
> +    uint8_t mode = 0;
> +
> +    if (!strcasecmp(str, "QSPI")) {
> +        mode = 1;
> +    } else if (!strcasecmp(str, "SD")) {
> +        mode = 5;
> +    } else if (!strcasecmp(str, "NOR")) {
> +        mode = 2;
> +    } else if (!strcasecmp(str, "JTAG")) {

Suggestion 1 is to use strncasecmp above.

> +        mode = 0;
> +    } else {
> +        error_setg(errp, "bootmode %s not supported", str);

Suggestion 2 is to do s/bootmode/boot mode/ above (for matching the error
message in patch 1).

> +        return;
> +    }
> +    m->boot_mode = mode;
> +}
> +
>  static void zynq_init(MachineState *machine)
>  {
>      ZynqMachineState *zynq_machine = ZYNQ_MACHINE(machine);
> @@ -241,6 +264,7 @@ static void zynq_init(MachineState *machine)
>      /* Create slcr, keep a pointer to connect clocks */
>      slcr = qdev_new("xilinx-zynq_slcr");
>      qdev_connect_clock_in(slcr, "ps_clk", zynq_machine->ps_clk);
> +    qdev_prop_set_uint8(slcr, "boot-mode", zynq_machine->boot_mode);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(slcr), &error_fatal);
>      sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000);
>  
> @@ -372,6 +396,7 @@ static void zynq_machine_class_init(ObjectClass *oc, void *data)
>          NULL
>      };
>      MachineClass *mc = MACHINE_CLASS(oc);
> +    ObjectProperty *prop;
>      mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
>      mc->init = zynq_init;
>      mc->max_cpus = ZYNQ_MAX_CPUS;
> @@ -379,6 +404,12 @@ static void zynq_machine_class_init(ObjectClass *oc, void *data)
>      mc->ignore_memory_transaction_failures = true;
>      mc->valid_cpu_types = valid_cpu_types;
>      mc->default_ram_id = "zynq.ext_ram";
> +    prop = object_class_property_add_str(oc, "boot-mode", NULL,
> +                              zynq_set_boot_mode);

Indentation on above line needs be corrected.

With above corrected:

Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>


> +    object_class_property_set_description(oc, "boot-mode",
> +                                          "Supported boot modes:"
> +                                          " JTAG QSPI SD NOR");

Suggestion 3 use lower case on above to match patch 3.

> +    object_property_set_default_str(prop, "QSPI");
>  }
>  
>  static const TypeInfo zynq_machine_type = {
> -- 
> 2.34.1
>

[PATCH v3 1/3] hw/misc/zynq_slcr: Add boot-mode property
[PATCH v3 2/3] hw/arm/xilinx_zynq: Add boot-mode property
[PATCH v3 3/3] docs/system/arm: Add a doc for zynq board