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.bo...@amd.com>
> Acked-by: Edgar E. Iglesias <edgar.igles...@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.igles...@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
>
