Re: [PATCH 2/2] hw/ppc/ppc440_uc: Remove dead l2sram_update_mappings()

[BALATON Zoltan]

On Wed, 11 Oct 2023, Philippe Mathieu-Daudé wrote:
> Hi Zoltan,
>
> On 11/10/23 15:31, BALATON Zoltan wrote:
> > On Wed, 11 Oct 2023, Philippe Mathieu-Daudé wrote:
> > > Apparently l2sram_update_mappings() bit-rotted over time,
> > > when defining MAP_L2SRAM we get:
> > >
> > >  hw/ppc/ppc440_uc.c:83:17: error: no member named 'isarc' in 'struct 
> > > ppc4xx_l2sram_t'
> > >      if (l2sram->isarc != isarc ||
> > >          ~~~~~~  ^
> > >  hw/ppc/ppc440_uc.c:84:18: error: no member named 'isacntl' in 'struct 
> > > ppc4xx_l2sram_t'
> > >          (l2sram->isacntl & 0x80000000) != (isacntl & 0x80000000)) {
> > >           ~~~~~~  ^
> > >  hw/ppc/ppc440_uc.c:85:21: error: no member named 'isacntl' in 'struct 
> > > ppc4xx_l2sram_t'
> > >          if (l2sram->isacntl & 0x80000000) {
> > >              ~~~~~~  ^
> > >  hw/ppc/ppc440_uc.c:88:50: error: no member named 'isarc_ram' in 'struct 
> > > ppc4xx_l2sram_t'
> > >                                          &l2sram->isarc_ram);
> > >                                           ~~~~~~  ^
> > >  hw/ppc/ppc440_uc.c:93:50: error: no member named 'isarc_ram' in 'struct 
> > > ppc4xx_l2sram_t'
> > >                                          &l2sram->isarc_ram);
> > >                                           ~~~~~~  ^
> > >  hw/ppc/ppc440_uc.c:96:17: error: no member named 'dsarc' in 'struct 
> > > ppc4xx_l2sram_t'
> > >      if (l2sram->dsarc != dsarc ||
> > >          ~~~~~~  ^
> > >  hw/ppc/ppc440_uc.c:97:18: error: no member named 'dsacntl' in 'struct 
> > > ppc4xx_l2sram_t'
> > >          (l2sram->dsacntl & 0x80000000) != (dsacntl & 0x80000000)) {
> > >           ~~~~~~  ^
> > >  hw/ppc/ppc440_uc.c:98:21: error: no member named 'dsacntl' in 'struct 
> > > ppc4xx_l2sram_t'
> > >          if (l2sram->dsacntl & 0x80000000) {
> > >              ~~~~~~  ^
> > >  hw/ppc/ppc440_uc.c:100:52: error: no member named 'dsarc' in 'struct 
> > > ppc4xx_l2sram_t'
> > >              if (!(isacntl & 0x80000000) || l2sram->dsarc != isarc) {
> > >                                             ~~~~~~  ^
> > >  hw/ppc/ppc440_uc.c:103:54: error: no member named 'dsarc_ram' in 'struct 
> > > ppc4xx_l2sram_t'
> > >                                              &l2sram->dsarc_ram);
> > >                                               ~~~~~~  ^
> > >  hw/ppc/ppc440_uc.c:111:54: error: no member named 'dsarc_ram' in 'struct 
> > > ppc4xx_l2sram_t'
> > >                                              &l2sram->dsarc_ram);
> > >                                               ~~~~~~  ^
> > >
> > > Remove that dead code.
>
> I missed to remove:
>
> -- >8 --
> diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> index 3a66b0c7f7..1312aa2080 100644
> --- a/hw/ppc/ppc440_uc.c
> +++ b/hw/ppc/ppc440_uc.c
> @@ -154,7 +154,6 @@ static void dcr_write_l2sram(void *opaque, int dcrn, 
> uint32_t val)
>         /*l2sram->isram1[dcrn - DCR_L2CACHE_BASE] = val;*/
>         break;
>     }
> -    /*l2sram_update_mappings(l2sram, isarc, isacntl, dsarc, dsacntl);*/
> }

Well, all of this func does nothing and just here so accessing these DCRs 
won't crash but it already has a FIXME comment at the beginning noting 
that, so in that case it's probably OK to remove the unfinished func as we 
still have a reminder here. So then:

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Regards,
BALATON Zoltan

> static void l2sram_reset(void *opaque)
> @@ -164,7 +163,6 @@ static void l2sram_reset(void *opaque)
>     memset(l2sram->l2cache, 0, sizeof(l2sram->l2cache));
>     l2sram->l2cache[DCR_L2CACHE_STAT - DCR_L2CACHE_BASE] = 0x80000000;
>     memset(l2sram->isram0, 0, sizeof(l2sram->isram0));
> -    /*l2sram_update_mappings(l2sram, isarc, isacntl, dsarc, dsacntl);*/
> }
> ---
>
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > > hw/ppc/ppc440_uc.c | 40 ----------------------------------------
> > > 1 file changed, 40 deletions(-)
> > >
> > > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c
> > > index 4181c843a8..643a79e330 100644
> > > --- a/hw/ppc/ppc440_uc.c
> > > +++ b/hw/ppc/ppc440_uc.c
> > > @@ -73,46 +73,6 @@ typedef struct ppc4xx_l2sram_t {
> > >     uint32_t isram0[11];
> > > } ppc4xx_l2sram_t;
> > >
> > > -#ifdef MAP_L2SRAM
> > > -static void l2sram_update_mappings(ppc4xx_l2sram_t *l2sram,
> > > -                                   uint32_t isarc, uint32_t isacntl,
> > > -                                   uint32_t dsarc, uint32_t dsacntl)
> >
> > If you remove this then nobody will remember this could be modelled or may 
> > be fixed so maybe leave it as a reminder for now.
>
> We can keep this code if someone fix it and enable it (convert the
> definition to a static boolean). Some APIs are being modified, we can
> not test modifications in such dead code. Even converting to a comment
> doesn't seem useful.
>
> Maybe you can add a comment "If you are interested in ..., see
> l2sram_update_mappings() draft implementation in
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg04261.html";?
>
> > Regards,
> > BALATON Zoltan