Re: [PATCH 1/7] hw/intc: sifive_plic: Add a reset function

[Alistair Francis]

On Wed, Dec 8, 2021 at 10:00 PM Philippe Mathieu-Daudé  wrote:
>
> Hi Alistair,
>
> On 12/8/21 07:42, Alistair Francis wrote:
> > From: Alistair Francis 
> >
> > Signed-off-by: Alistair Francis 
> > ---
> >  hw/intc/sifive_plic.c | 12 
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
> > index 877e76877c..35f097799a 100644
> > --- a/hw/intc/sifive_plic.c
> > +++ b/hw/intc/sifive_plic.c
> > @@ -355,6 +355,17 @@ static const MemoryRegionOps sifive_plic_ops = {
> >  }
> >  };
> >
> > +static void sifive_plic_reset(DeviceState *dev)
> > +{
> > +SiFivePLICState *s = SIFIVE_PLIC(dev);
> > +
> > +memset(s->source_priority, 0, sizeof(uint32_t) * s->num_sources);
> > +memset(s->target_priority, 0, sizeof(uint32_t) * s->num_addrs);
> > +memset(s->pending, 0, sizeof(uint32_t) * s->bitfield_words);
> > +memset(s->claimed, 0, sizeof(uint32_t) * s->bitfield_words);
> > +memset(s->enable, 0, sizeof(uint32_t) * s->num_enables);
>
> Looking at sifive_plic_realize():
>
> - Should we reset the external IRQs in a default state?

Good point, I'll add that.

> - Shouldn't riscv_cpu_claim_interrupts() be called at reset?

I don't think so. riscv_cpu_claim_interrupts is a once and done call.

Alistair

>
> Note: parse_hart_config() name is slightly confusing since
> beside parsing, it also allocates addr_config. Maybe consider
> renaming?

Re: [PATCH 1/7] hw/intc: sifive_plic: Add a reset function

[Philippe Mathieu-Daudé]

Hi Alistair,

On 12/8/21 07:42, Alistair Francis wrote:
> From: Alistair Francis 
> 
> Signed-off-by: Alistair Francis 
> ---
>  hw/intc/sifive_plic.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
> index 877e76877c..35f097799a 100644
> --- a/hw/intc/sifive_plic.c
> +++ b/hw/intc/sifive_plic.c
> @@ -355,6 +355,17 @@ static const MemoryRegionOps sifive_plic_ops = {
>  }
>  };
>  
> +static void sifive_plic_reset(DeviceState *dev)
> +{
> +SiFivePLICState *s = SIFIVE_PLIC(dev);
> +
> +memset(s->source_priority, 0, sizeof(uint32_t) * s->num_sources);
> +memset(s->target_priority, 0, sizeof(uint32_t) * s->num_addrs);
> +memset(s->pending, 0, sizeof(uint32_t) * s->bitfield_words);
> +memset(s->claimed, 0, sizeof(uint32_t) * s->bitfield_words);
> +memset(s->enable, 0, sizeof(uint32_t) * s->num_enables);

Looking at sifive_plic_realize():

- Should we reset the external IRQs in a default state?
- Shouldn't riscv_cpu_claim_interrupts() be called at reset?

Note: parse_hart_config() name is slightly confusing since
beside parsing, it also allocates addr_config. Maybe consider
renaming?

[PATCH 1/7] hw/intc: sifive_plic: Add a reset function

[Alistair Francis]

From: Alistair Francis 

Signed-off-by: Alistair Francis 
---
 hw/intc/sifive_plic.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/hw/intc/sifive_plic.c b/hw/intc/sifive_plic.c
index 877e76877c..35f097799a 100644
--- a/hw/intc/sifive_plic.c
+++ b/hw/intc/sifive_plic.c
@@ -355,6 +355,17 @@ static const MemoryRegionOps sifive_plic_ops = {
 }
 };
 
+static void sifive_plic_reset(DeviceState *dev)
+{
+SiFivePLICState *s = SIFIVE_PLIC(dev);
+
+memset(s->source_priority, 0, sizeof(uint32_t) * s->num_sources);
+memset(s->target_priority, 0, sizeof(uint32_t) * s->num_addrs);
+memset(s->pending, 0, sizeof(uint32_t) * s->bitfield_words);
+memset(s->claimed, 0, sizeof(uint32_t) * s->bitfield_words);
+memset(s->enable, 0, sizeof(uint32_t) * s->num_enables);
+}
+
 /*
  * parse PLIC hart/mode address offset config
  *
@@ -501,6 +512,7 @@ static void sifive_plic_class_init(ObjectClass *klass, void 
*data)
 {
 DeviceClass *dc = DEVICE_CLASS(klass);
 
+dc->reset = sifive_plic_reset;
 device_class_set_props(dc, sifive_plic_properties);
 dc->realize = sifive_plic_realize;
 dc->vmsd = _sifive_plic;
-- 
2.31.1