| Message ID | 20231011124312.60476-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/ppc/ppc440_uc: Maintain and remove dead code | expand |
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. > > 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. Regards, BALATON Zoltan > -{ > - if (l2sram->isarc != isarc || > - (l2sram->isacntl & 0x80000000) != (isacntl & 0x80000000)) { > - if (l2sram->isacntl & 0x80000000) { > - /* Unmap previously assigned memory region */ > - memory_region_del_subregion(get_system_memory(), > - &l2sram->isarc_ram); > - } > - if (isacntl & 0x80000000) { > - /* Map new instruction memory region */ > - memory_region_add_subregion(get_system_memory(), isarc, > - &l2sram->isarc_ram); > - } > - } > - if (l2sram->dsarc != dsarc || > - (l2sram->dsacntl & 0x80000000) != (dsacntl & 0x80000000)) { > - if (l2sram->dsacntl & 0x80000000) { > - /* Beware not to unmap the region we just mapped */ > - if (!(isacntl & 0x80000000) || l2sram->dsarc != isarc) { > - /* Unmap previously assigned memory region */ > - memory_region_del_subregion(get_system_memory(), > - &l2sram->dsarc_ram); > - } > - } > - if (dsacntl & 0x80000000) { > - /* Beware not to remap the region we just mapped */ > - if (!(isacntl & 0x80000000) || dsarc != isarc) { > - /* Map new data memory region */ > - memory_region_add_subregion(get_system_memory(), dsarc, > - &l2sram->dsarc_ram); > - } > - } > - } > -} > -#endif > - > static uint32_t dcr_read_l2sram(void *opaque, int dcrn) > { > ppc4xx_l2sram_t *l2sram = opaque; >
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);*/ } 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
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 > > >
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 (l2sram->isarc != isarc || - (l2sram->isacntl & 0x80000000) != (isacntl & 0x80000000)) { - if (l2sram->isacntl & 0x80000000) { - /* Unmap previously assigned memory region */ - memory_region_del_subregion(get_system_memory(), - &l2sram->isarc_ram); - } - if (isacntl & 0x80000000) { - /* Map new instruction memory region */ - memory_region_add_subregion(get_system_memory(), isarc, - &l2sram->isarc_ram); - } - } - if (l2sram->dsarc != dsarc || - (l2sram->dsacntl & 0x80000000) != (dsacntl & 0x80000000)) { - if (l2sram->dsacntl & 0x80000000) { - /* Beware not to unmap the region we just mapped */ - if (!(isacntl & 0x80000000) || l2sram->dsarc != isarc) { - /* Unmap previously assigned memory region */ - memory_region_del_subregion(get_system_memory(), - &l2sram->dsarc_ram); - } - } - if (dsacntl & 0x80000000) { - /* Beware not to remap the region we just mapped */ - if (!(isacntl & 0x80000000) || dsarc != isarc) { - /* Map new data memory region */ - memory_region_add_subregion(get_system_memory(), dsarc, - &l2sram->dsarc_ram); - } - } - } -} -#endif - static uint32_t dcr_read_l2sram(void *opaque, int dcrn) { ppc4xx_l2sram_t *l2sram = opaque;
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. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/ppc/ppc440_uc.c | 40 ---------------------------------------- 1 file changed, 40 deletions(-)