diff mbox series

[06/24] hw/misc/mps2-fpgaio: Support SWITCH register

Message ID 20210205170019.25319-7-peter.maydell@linaro.org
State Superseded
Headers show
Series hw/arm: New board model mps3-an524 | expand

Commit Message

Peter Maydell Feb. 5, 2021, 5 p.m. UTC
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

Comments

Peter Maydell Feb. 12, 2021, 1:45 p.m. UTC | #1
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
Philippe Mathieu-Daudé Feb. 12, 2021, 1:51 p.m. UTC | #2
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);
Peter Maydell Feb. 12, 2021, 2:03 p.m. UTC | #3
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
Philippe Mathieu-Daudé Feb. 12, 2021, 6:23 p.m. UTC | #4
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 mbox series

Patch

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(),
 };