Re: [PATCH v2 27/28] target/ppc: Remove id_tlbs flag from CPU env

[BALATON Zoltan]

On Tue, 7 May 2024, Nicholas Piggin wrote:
> On Thu May 2, 2024 at 9:43 AM AEST, BALATON Zoltan wrote:
> > This flag for split instruction/data TLBs is only set for 6xx soft TLB
> > MMU model and not used otherwise so no need to have a separate flag
> > for that.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > ---
> >  hw/ppc/pegasos2.c        |  2 +-
> >  target/ppc/cpu.h         |  1 -
> >  target/ppc/cpu_init.c    | 19 +++++--------------
> >  target/ppc/helper_regs.c |  1 -
> >  target/ppc/mmu_common.c  | 10 ++--------
> >  target/ppc/mmu_helper.c  | 12 ++----------
> >  6 files changed, 10 insertions(+), 35 deletions(-)
> >
> > diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
> > index 04d6decb2b..dfc6fab180 100644
> > --- a/hw/ppc/pegasos2.c
> > +++ b/hw/ppc/pegasos2.c
> > @@ -984,7 +984,7 @@ static void *build_fdt(MachineState *machine, int *fdt_size)
> >                            cpu->env.icache_line_size);
> >      qemu_fdt_setprop_cell(fdt, cp, "i-cache-line-size",
> >                            cpu->env.icache_line_size);
> > -    if (cpu->env.id_tlbs) {
> > +    if (cpu->env.tlb_type == TLB_6XX) {
>
> Want to just add the standard comment here?
>
>    /* 6xx has separate TLBs for instructions and data */

I think that comment would be redundant here because it's clear from the 
i-tlb, d-tlb this adds so I can do without a comment in this machine if 
you don't mind. (If this was not in my machine I would not mind adding a 
comment but I'd keep this one simple.) I think comments should only be 
added for things that are not clear from code.

Regards,
BALATON Zoltan

> Otherwise looks good
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> >          qemu_fdt_setprop_cell(fdt, cp, "i-tlb-sets", cpu->env.nb_ways);
> >          qemu_fdt_setprop_cell(fdt, cp, "i-tlb-size", cpu->env.tlb_per_way);
> >          qemu_fdt_setprop_cell(fdt, cp, "d-tlb-sets", cpu->env.nb_ways);