| Message ID | 20210205170019.25319-7-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hw/arm: New board model mps3-an524 | expand |
On Fri, 5 Feb 2021 at 17:00, Peter Maydell <peter.maydell@linaro.org> wrote: > > MPS3 boards have an extra SWITCH register in the FPGAIO block which > reports the value of some switches. Implement this, governed by a > property the board code can use to specify whether whether it exists. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/hw/misc/mps2-fpgaio.h | 1 + > hw/misc/mps2-fpgaio.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) I changed my mind about the property/struct field name here, I think "has" is what we tend to use rather than "have". Trivial change to squash into this patch: diff --git a/include/hw/misc/mps2-fpgaio.h b/include/hw/misc/mps2-fpgaio.h index 83c6e18a4ee..0d3c8eef56c 100644 --- a/include/hw/misc/mps2-fpgaio.h +++ b/include/hw/misc/mps2-fpgaio.h @@ -38,7 +38,7 @@ struct MPS2FPGAIO { MemoryRegion iomem; LEDState *led[MPS2FPGAIO_MAX_LEDS]; uint32_t num_leds; - bool have_switches; + bool has_switches; uint32_t led0; uint32_t prescale; diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c index b54657a4f07..acbd0be9f4b 100644 --- a/hw/misc/mps2-fpgaio.c +++ b/hw/misc/mps2-fpgaio.c @@ -158,7 +158,7 @@ static uint64_t mps2_fpgaio_read(void *opaque, hwaddr offset, unsigned size) r = s->pscntr; break; case A_SWITCH: - if (!s->have_switches) { + if (!s->has_switches) { goto bad_offset; } /* User-togglable board switches. We don't model that, so report 0. */ @@ -327,7 +327,7 @@ static Property mps2_fpgaio_properties[] = { DEFINE_PROP_UINT32("prescale-clk", MPS2FPGAIO, prescale_clk, 20000000), /* Number of LEDs controlled by LED0 register */ DEFINE_PROP_UINT32("num-leds", MPS2FPGAIO, num_leds, 2), - DEFINE_PROP_BOOL("have-switches", MPS2FPGAIO, have_switches, false), + DEFINE_PROP_BOOL("has-switches", MPS2FPGAIO, has_switches, false), DEFINE_PROP_END_OF_LIST(), }; thanks -- PMM
On 2/12/21 2:45 PM, Peter Maydell wrote: > On Fri, 5 Feb 2021 at 17:00, Peter Maydell <peter.maydell@linaro.org> wrote: >> >> MPS3 boards have an extra SWITCH register in the FPGAIO block which >> reports the value of some switches. Implement this, governed by a >> property the board code can use to specify whether whether it exists. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> include/hw/misc/mps2-fpgaio.h | 1 + >> hw/misc/mps2-fpgaio.c | 10 ++++++++++ >> 2 files changed, 11 insertions(+) > > I changed my mind about the property/struct field name here, I think > "has" is what we tend to use rather than "have". Trivial change > to squash into this patch: What about "use-switches"? use-x: 12 occurences has-x: 9. Is there a difference in the meaning? Maybe have refers to something internal, while use to something external? $ git grep -F 'DEFINE_PROP_BOOL("use-' hw/audio/hda-codec.c:848: DEFINE_PROP_BOOL("use-timer", HDAAudioState, use_timer, true), hw/block/nvme.c:4556: DEFINE_PROP_BOOL("use-intel-id", NvmeCtrl, params.use_intel_id, false), hw/intc/ppc-uic.c:278: DEFINE_PROP_BOOL("use-vectors", PPCUIC, use_vectors, true), hw/ppc/spapr_rng.c:135: DEFINE_PROP_BOOL("use-kvm", SpaprRngState, use_kvm, false), hw/virtio/virtio.c:3722: DEFINE_PROP_BOOL("use-started", VirtIODevice, use_started, true), hw/virtio/virtio.c:3723: DEFINE_PROP_BOOL("use-disabled-flag", VirtIODevice, use_disabled_flag, true), target/microblaze/cpu.c:292: DEFINE_PROP_BOOL("use-stack-protection", MicroBlazeCPU, cfg.stackprot, target/microblaze/cpu.c:311: DEFINE_PROP_BOOL("use-barrel", MicroBlazeCPU, cfg.use_barrel, true), target/microblaze/cpu.c:312: DEFINE_PROP_BOOL("use-div", MicroBlazeCPU, cfg.use_div, true), target/microblaze/cpu.c:313: DEFINE_PROP_BOOL("use-msr-instr", MicroBlazeCPU, cfg.use_msr_instr, true), target/microblaze/cpu.c:314: DEFINE_PROP_BOOL("use-pcmp-instr", MicroBlazeCPU, cfg.use_pcmp_instr, true), target/microblaze/cpu.c:315: DEFINE_PROP_BOOL("use-mmu", MicroBlazeCPU, cfg.use_mmu, true), $ git grep -F 'DEFINE_PROP_BOOL("has-' hw/gpio/imx_gpio.c:295: DEFINE_PROP_BOOL("has-edge-sel", IMXGPIOState, has_edge_sel, true), hw/gpio/imx_gpio.c:296: DEFINE_PROP_BOOL("has-upper-pin-irq", IMXGPIOState, has_upper_pin_irq, hw/intc/arm_gic_common.c:357: DEFINE_PROP_BOOL("has-security-extensions", GICState, security_extn, 0), hw/intc/arm_gic_common.c:359: DEFINE_PROP_BOOL("has-virtualization-extensions", GICState, virt_extn, 0), hw/intc/arm_gicv3_common.c:497: DEFINE_PROP_BOOL("has-security-extensions", GICv3State, security_extn, 0), hw/misc/macio/macio.c:430: DEFINE_PROP_BOOL("has-pmu", NewWorldMacIOState, has_pmu, false), hw/misc/macio/macio.c:431: DEFINE_PROP_BOOL("has-adb", NewWorldMacIOState, has_adb, false), hw/misc/macio/pmu.c:782: DEFINE_PROP_BOOL("has-adb", PMUState, has_adb, true), target/arm/cpu.c:1110: DEFINE_PROP_BOOL("has-mpu", ARMCPU, has_mpu, true);
On Fri, 12 Feb 2021 at 13:51, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 2/12/21 2:45 PM, Peter Maydell wrote: > > On Fri, 5 Feb 2021 at 17:00, Peter Maydell <peter.maydell@linaro.org> wrote: > >> > >> MPS3 boards have an extra SWITCH register in the FPGAIO block which > >> reports the value of some switches. Implement this, governed by a > >> property the board code can use to specify whether whether it exists. > >> > >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > >> --- > >> include/hw/misc/mps2-fpgaio.h | 1 + > >> hw/misc/mps2-fpgaio.c | 10 ++++++++++ > >> 2 files changed, 11 insertions(+) > > > > I changed my mind about the property/struct field name here, I think > > "has" is what we tend to use rather than "have". Trivial change > > to squash into this patch: > > What about "use-switches"? > > use-x: 12 occurences > has-x: 9. > > Is there a difference in the meaning? Maybe have refers to > something internal, while use to something external? Generally 'has' (or 'have') means "configure the object to possess this thing", whereas "use" means "the object has this thing; configure it to actually make use of it". thanks -- PMM
On 2/5/21 6:00 PM, Peter Maydell wrote: > MPS3 boards have an extra SWITCH register in the FPGAIO block which > reports the value of some switches. Implement this, governed by a > property the board code can use to specify whether whether it exists. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/hw/misc/mps2-fpgaio.h | 1 + > hw/misc/mps2-fpgaio.c | 10 ++++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/include/hw/misc/mps2-fpgaio.h b/include/hw/misc/mps2-fpgaio.h > index bfe73134e78..83c6e18a4ee 100644 > --- a/include/hw/misc/mps2-fpgaio.h > +++ b/include/hw/misc/mps2-fpgaio.h > @@ -38,6 +38,7 @@ struct MPS2FPGAIO { > MemoryRegion iomem; > LEDState *led[MPS2FPGAIO_MAX_LEDS]; > uint32_t num_leds; > + bool have_switches; > > uint32_t led0; > uint32_t prescale; > diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c > index b28a1be22cc..b54657a4f07 100644 > --- a/hw/misc/mps2-fpgaio.c > +++ b/hw/misc/mps2-fpgaio.c > @@ -35,6 +35,7 @@ REG32(CLK100HZ, 0x14) > REG32(COUNTER, 0x18) > REG32(PRESCALE, 0x1c) > REG32(PSCNTR, 0x20) > +REG32(SWITCH, 0x28) > REG32(MISC, 0x4c) > > static uint32_t counter_from_tickoff(int64_t now, int64_t tick_offset, int frq) > @@ -156,7 +157,15 @@ static uint64_t mps2_fpgaio_read(void *opaque, hwaddr offset, unsigned size) > resync_counter(s); > r = s->pscntr; > break; > + case A_SWITCH: > + if (!s->have_switches) { > + goto bad_offset; > + } > + /* User-togglable board switches. We don't model that, so report 0. */ We should and probably will at some point... This is a feature I'm thinking about and which could be implemented the same way as the TempSensor series. My latest problem is to have QOM names (full path) consistent. That way we can toggle a switch at runtime via (at least) a QMP command. Anyway to your patch (including change squashed): Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/include/hw/misc/mps2-fpgaio.h b/include/hw/misc/mps2-fpgaio.h index bfe73134e78..83c6e18a4ee 100644 --- a/include/hw/misc/mps2-fpgaio.h +++ b/include/hw/misc/mps2-fpgaio.h @@ -38,6 +38,7 @@ struct MPS2FPGAIO { MemoryRegion iomem; LEDState *led[MPS2FPGAIO_MAX_LEDS]; uint32_t num_leds; + bool have_switches; uint32_t led0; uint32_t prescale; diff --git a/hw/misc/mps2-fpgaio.c b/hw/misc/mps2-fpgaio.c index b28a1be22cc..b54657a4f07 100644 --- a/hw/misc/mps2-fpgaio.c +++ b/hw/misc/mps2-fpgaio.c @@ -35,6 +35,7 @@ REG32(CLK100HZ, 0x14) REG32(COUNTER, 0x18) REG32(PRESCALE, 0x1c) REG32(PSCNTR, 0x20) +REG32(SWITCH, 0x28) REG32(MISC, 0x4c) static uint32_t counter_from_tickoff(int64_t now, int64_t tick_offset, int frq) @@ -156,7 +157,15 @@ static uint64_t mps2_fpgaio_read(void *opaque, hwaddr offset, unsigned size) resync_counter(s); r = s->pscntr; break; + case A_SWITCH: + if (!s->have_switches) { + goto bad_offset; + } + /* User-togglable board switches. We don't model that, so report 0. */ + r = 0; + break; default: + bad_offset: qemu_log_mask(LOG_GUEST_ERROR, "MPS2 FPGAIO read: bad offset %x\n", (int) offset); r = 0; @@ -318,6 +327,7 @@ static Property mps2_fpgaio_properties[] = { DEFINE_PROP_UINT32("prescale-clk", MPS2FPGAIO, prescale_clk, 20000000), /* Number of LEDs controlled by LED0 register */ DEFINE_PROP_UINT32("num-leds", MPS2FPGAIO, num_leds, 2), + DEFINE_PROP_BOOL("have-switches", MPS2FPGAIO, have_switches, false), DEFINE_PROP_END_OF_LIST(), };
MPS3 boards have an extra SWITCH register in the FPGAIO block which reports the value of some switches. Implement this, governed by a property the board code can use to specify whether whether it exists. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/misc/mps2-fpgaio.h | 1 + hw/misc/mps2-fpgaio.c | 10 ++++++++++ 2 files changed, 11 insertions(+) -- 2.20.1