| Message ID | a98d1f7f94e91a42796b7d91e9153a7eaa3d1c44.1635541329.git.balaton@eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | More SH4 clean ups (including code style series) | expand |
On 10/29/21 23:02, BALATON Zoltan wrote: > This function is very simple and provides no advantage. Call sites > become simpler without it so just write it in line and drop the > separate function. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/intc/sh_intc.c | 54 ++++++++++++++++------------------------ > hw/sh4/sh7750.c | 4 +-- > include/hw/sh4/sh_intc.h | 2 +- > 3 files changed, 25 insertions(+), 35 deletions(-) > static void sh_intc_register_source(struct intc_desc *desc, > intc_enum source, > struct intc_group *groups, > int nr_groups) > { > unsigned int i, k; > - struct intc_source *s; > + intc_enum id; > Maybe: assert(source != UNUSED); > if (desc->mask_regs) { > for (i = 0; i < desc->nr_mask_regs; i++) { > struct intc_mask_reg *mr = &desc->mask_regs[i]; > > for (k = 0; k < ARRAY_SIZE(mr->enum_ids); k++) { > - if (mr->enum_ids[k] != source) { > - continue; > - } > - s = sh_intc_source(desc, mr->enum_ids[k]); > - if (s) { > - s->enable_max++; > + id = mr->enum_ids[k]; > + if (id && id == source) { Then you can drop the 'id' checks. > + desc->sources[id].enable_max++; > } > } > }
On Sat, 30 Oct 2021, Philippe Mathieu-Daudé wrote: > On 10/29/21 23:02, BALATON Zoltan wrote: >> This function is very simple and provides no advantage. Call sites >> become simpler without it so just write it in line and drop the >> separate function. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/intc/sh_intc.c | 54 ++++++++++++++++------------------------ >> hw/sh4/sh7750.c | 4 +-- >> include/hw/sh4/sh_intc.h | 2 +- >> 3 files changed, 25 insertions(+), 35 deletions(-) > >> static void sh_intc_register_source(struct intc_desc *desc, >> intc_enum source, >> struct intc_group *groups, >> int nr_groups) >> { >> unsigned int i, k; >> - struct intc_source *s; >> + intc_enum id; >> > > Maybe: > > assert(source != UNUSED); > >> if (desc->mask_regs) { >> for (i = 0; i < desc->nr_mask_regs; i++) { >> struct intc_mask_reg *mr = &desc->mask_regs[i]; >> >> for (k = 0; k < ARRAY_SIZE(mr->enum_ids); k++) { >> - if (mr->enum_ids[k] != source) { >> - continue; >> - } >> - s = sh_intc_source(desc, mr->enum_ids[k]); >> - if (s) { >> - s->enable_max++; >> + id = mr->enum_ids[k]; >> + if (id && id == source) { > > Then you can drop the 'id' checks. I've tried to preserve the original brhaviour in this patch and not change it for now. This will need to be rewritten anyway beause it does not handle priorities and hard to QOM-ify as it is so I'll come back to this where these will probably change, so for now just leave it to keep the existing behaviour. Then we can revise it later in separate patch. Thanks for taking the time to review my patches, much appreciated. Regards, BALATON Zoltan >> + desc->sources[id].enable_max++; >> } >> } >> } > >
On 10/30/21 01:48, BALATON Zoltan wrote: > On Sat, 30 Oct 2021, Philippe Mathieu-Daudé wrote: >> On 10/29/21 23:02, BALATON Zoltan wrote: >>> This function is very simple and provides no advantage. Call sites >>> become simpler without it so just write it in line and drop the >>> separate function. >>> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >>> hw/intc/sh_intc.c | 54 ++++++++++++++++------------------------ >>> hw/sh4/sh7750.c | 4 +-- >>> include/hw/sh4/sh_intc.h | 2 +- >>> 3 files changed, 25 insertions(+), 35 deletions(-) >> >>> static void sh_intc_register_source(struct intc_desc *desc, >>> intc_enum source, >>> struct intc_group *groups, >>> int nr_groups) >>> { >>> unsigned int i, k; >>> - struct intc_source *s; >>> + intc_enum id; >>> >> >> Maybe: >> >> assert(source != UNUSED); >> >>> if (desc->mask_regs) { >>> for (i = 0; i < desc->nr_mask_regs; i++) { >>> struct intc_mask_reg *mr = &desc->mask_regs[i]; >>> >>> for (k = 0; k < ARRAY_SIZE(mr->enum_ids); k++) { >>> - if (mr->enum_ids[k] != source) { >>> - continue; >>> - } >>> - s = sh_intc_source(desc, mr->enum_ids[k]); >>> - if (s) { >>> - s->enable_max++; >>> + id = mr->enum_ids[k]; >>> + if (id && id == source) { >> >> Then you can drop the 'id' checks. > > I've tried to preserve the original brhaviour in this patch and not > change it for now. This will need to be rewritten anyway beause it does > not handle priorities and hard to QOM-ify as it is so I'll come back to > this where these will probably change, so for now just leave it to keep > the existing behaviour. Then we can revise it later in separate patch. Fine. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > Thanks for taking the time to review my patches, much appreciated. > > Regards, > BALATON Zoltan > >>> + desc->sources[id].enable_max++; >>> } >>> } >>> } >> >>
diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c index 2e0704608d..8fa8fcd99f 100644 --- a/hw/intc/sh_intc.c +++ b/hw/intc/sh_intc.c @@ -263,33 +263,22 @@ static const MemoryRegionOps sh_intc_ops = { .endianness = DEVICE_NATIVE_ENDIAN, }; -struct intc_source *sh_intc_source(struct intc_desc *desc, intc_enum id) -{ - if (id) { - return &desc->sources[id]; - } - return NULL; -} - static void sh_intc_register_source(struct intc_desc *desc, intc_enum source, struct intc_group *groups, int nr_groups) { unsigned int i, k; - struct intc_source *s; + intc_enum id; if (desc->mask_regs) { for (i = 0; i < desc->nr_mask_regs; i++) { struct intc_mask_reg *mr = &desc->mask_regs[i]; for (k = 0; k < ARRAY_SIZE(mr->enum_ids); k++) { - if (mr->enum_ids[k] != source) { - continue; - } - s = sh_intc_source(desc, mr->enum_ids[k]); - if (s) { - s->enable_max++; + id = mr->enum_ids[k]; + if (id && id == source) { + desc->sources[id].enable_max++; } } } @@ -300,12 +289,9 @@ static void sh_intc_register_source(struct intc_desc *desc, struct intc_prio_reg *pr = &desc->prio_regs[i]; for (k = 0; k < ARRAY_SIZE(pr->enum_ids); k++) { - if (pr->enum_ids[k] != source) { - continue; - } - s = sh_intc_source(desc, pr->enum_ids[k]); - if (s) { - s->enable_max++; + id = pr->enum_ids[k]; + if (id && id == source) { + desc->sources[id].enable_max++; } } } @@ -316,12 +302,9 @@ static void sh_intc_register_source(struct intc_desc *desc, struct intc_group *gr = &groups[i]; for (k = 0; k < ARRAY_SIZE(gr->enum_ids); k++) { - if (gr->enum_ids[k] != source) { - continue; - } - s = sh_intc_source(desc, gr->enum_ids[k]); - if (s) { - s->enable_max++; + id = gr->enum_ids[k]; + if (id && id == source) { + desc->sources[id].enable_max++; } } } @@ -336,14 +319,16 @@ void sh_intc_register_sources(struct intc_desc *desc, int nr_groups) { unsigned int i, k; + intc_enum id; struct intc_source *s; for (i = 0; i < nr_vectors; i++) { struct intc_vect *vect = &vectors[i]; sh_intc_register_source(desc, vect->enum_id, groups, nr_groups); - s = sh_intc_source(desc, vect->enum_id); - if (s) { + id = vect->enum_id; + if (id) { + s = &desc->sources[id]; s->vect = vect->vect; trace_sh_intc_register("source", vect->enum_id, s->vect, s->enable_count, s->enable_max); @@ -354,14 +339,16 @@ void sh_intc_register_sources(struct intc_desc *desc, for (i = 0; i < nr_groups; i++) { struct intc_group *gr = &groups[i]; - s = sh_intc_source(desc, gr->enum_id); + id = gr->enum_id; + s = &desc->sources[id]; s->next_enum_id = gr->enum_ids[0]; for (k = 1; k < ARRAY_SIZE(gr->enum_ids); k++) { if (!gr->enum_ids[k]) { continue; } - s = sh_intc_source(desc, gr->enum_ids[k - 1]); + id = gr->enum_ids[k - 1]; + s = &desc->sources[id]; s->next_enum_id = gr->enum_ids[k]; } trace_sh_intc_register("group", gr->enum_id, 0xffff, @@ -463,7 +450,10 @@ void sh_intc_set_irl(void *opaque, int n, int level) { struct intc_source *s = opaque; int i, irl = level ^ 15; - for (i = 0; (s = sh_intc_source(s->parent, s->next_enum_id)); i++) { + intc_enum id = s->next_enum_id; + + for (i = 0; id; id = s->next_enum_id, i++) { + s = &s->parent->sources[id]; if (i == irl) { sh_intc_toggle_source(s, s->enable_count ? 0 : 1, s->asserted ? 0 : 1); diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c index 7b21f1ce44..9218d0bade 100644 --- a/hw/sh4/sh7750.c +++ b/hw/sh4/sh7750.c @@ -899,6 +899,6 @@ SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem) qemu_irq sh7750_irl(SH7750State *s) { - sh_intc_toggle_source(sh_intc_source(&s->intc, IRL), 1, 0); /* enable */ - return qemu_allocate_irq(sh_intc_set_irl, sh_intc_source(&s->intc, IRL), 0); + sh_intc_toggle_source(&s->intc.sources[IRL], 1, 0); /* enable */ + return qemu_allocate_irq(sh_intc_set_irl, &s->intc.sources[IRL], 0); } diff --git a/include/hw/sh4/sh_intc.h b/include/hw/sh4/sh_intc.h index 65f3425057..f62d5c5e13 100644 --- a/include/hw/sh4/sh_intc.h +++ b/include/hw/sh4/sh_intc.h @@ -58,7 +58,7 @@ struct intc_desc { }; int sh_intc_get_pending_vector(struct intc_desc *desc, int imask); -struct intc_source *sh_intc_source(struct intc_desc *desc, intc_enum id); + void sh_intc_toggle_source(struct intc_source *source, int enable_adj, int assert_adj);
This function is very simple and provides no advantage. Call sites become simpler without it so just write it in line and drop the separate function. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/intc/sh_intc.c | 54 ++++++++++++++++------------------------ hw/sh4/sh7750.c | 4 +-- include/hw/sh4/sh_intc.h | 2 +- 3 files changed, 25 insertions(+), 35 deletions(-)