| Message ID | 82ca7d39f47685f35f735e40ab1dfa8fa1b86b9b.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: > The timer memory region is only accessed via aliases that are 0x1000 > bytes long, no need to have the timer region larger than that. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > hw/timer/sh_timer.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c > index 250ad41b48..a6445092e4 100644 > --- a/hw/timer/sh_timer.c > +++ b/hw/timer/sh_timer.c > @@ -350,8 +350,7 @@ void tmu012_init(MemoryRegion *sysmem, hwaddr base, int feat, uint32_t freq, > ch2_irq0); /* ch2_irq1 not supported */ > } > > - memory_region_init_io(&s->iomem, NULL, &tmu012_ops, s, > - "timer", 0x100000000ULL); > + memory_region_init_io(&s->iomem, NULL, &tmu012_ops, s, "timer", 0x1000); Per the manual (R01UH0457EJ0401 Rev. 4.01 [*]) Page 317/1128, Table 12.2 "TMU Registers" the first 3 timers (implemented by the tmu012_state structure) fit in a region of 0x30 bytes. Looking at hw/timer/sh_timer.c I only see a maximum access of 0x40, where 0x1000 comes from? The P4/A7 aliases? If you have a way to test and ack, I can replace by 0x40 when applying. [*] https://www.renesas.com/us/en/document/mah/sh7751-group-sh7751r-group-users-manual-hardware?language=en&r=1055171
On Sat, 30 Oct 2021, Philippe Mathieu-Daudé wrote: > On 10/29/21 23:02, BALATON Zoltan wrote: >> The timer memory region is only accessed via aliases that are 0x1000 >> bytes long, no need to have the timer region larger than that. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> hw/timer/sh_timer.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c >> index 250ad41b48..a6445092e4 100644 >> --- a/hw/timer/sh_timer.c >> +++ b/hw/timer/sh_timer.c >> @@ -350,8 +350,7 @@ void tmu012_init(MemoryRegion *sysmem, hwaddr base, int feat, uint32_t freq, >> ch2_irq0); /* ch2_irq1 not supported */ >> } >> >> - memory_region_init_io(&s->iomem, NULL, &tmu012_ops, s, >> - "timer", 0x100000000ULL); >> + memory_region_init_io(&s->iomem, NULL, &tmu012_ops, s, "timer", 0x1000); > > Per the manual (R01UH0457EJ0401 Rev. 4.01 [*]) Page 317/1128, Table > 12.2 "TMU Registers" the first 3 timers (implemented by the tmu012_state > structure) fit in a region of 0x30 bytes. Sent a v7 of this patch only changing it to 0x30 with which the Linux image I've tested still boots but don't know if it uses a timer at all. > Looking at hw/timer/sh_timer.c I only see a maximum access of 0x40, Where do you see 0x40? > where 0x1000 comes from? The P4/A7 aliases? Yes, as the commit message said. Since this was a last minute change I tried to be safe and not change anything guest visible at this point. > If you have a way to test and ack, I can replace by 0x40 when applying. If you think 0x40 is better then I'm fine with that but I don't see a register after 0x2c which is 32 bits so 0x30 length should be enough according to that. Regards. BALATON Zoltan > [*] > https://www.renesas.com/us/en/document/mah/sh7751-group-sh7751r-group-users-manual-hardware?language=en&r=1055171 > >
On 10/30/21 01:36, BALATON Zoltan wrote: > On Sat, 30 Oct 2021, Philippe Mathieu-Daudé wrote: >> On 10/29/21 23:02, BALATON Zoltan wrote: >>> The timer memory region is only accessed via aliases that are 0x1000 >>> bytes long, no need to have the timer region larger than that. >>> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> --- >>> hw/timer/sh_timer.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c >>> index 250ad41b48..a6445092e4 100644 >>> --- a/hw/timer/sh_timer.c >>> +++ b/hw/timer/sh_timer.c >>> @@ -350,8 +350,7 @@ void tmu012_init(MemoryRegion *sysmem, hwaddr >>> base, int feat, uint32_t freq, >>> ch2_irq0); /* ch2_irq1 not >>> supported */ >>> } >>> >>> - memory_region_init_io(&s->iomem, NULL, &tmu012_ops, s, >>> - "timer", 0x100000000ULL); >>> + memory_region_init_io(&s->iomem, NULL, &tmu012_ops, s, "timer", >>> 0x1000); >> >> Per the manual (R01UH0457EJ0401 Rev. 4.01 [*]) Page 317/1128, Table >> 12.2 "TMU Registers" the first 3 timers (implemented by the tmu012_state >> structure) fit in a region of 0x30 bytes. > > Sent a v7 of this patch only changing it to 0x30 with which the Linux > image I've tested still boots but don't know if it uses a timer at all. > >> Looking at hw/timer/sh_timer.c I only see a maximum access of 0x40, > > Where do you see 0x40? I just looked at tmu012_read(), not sh_timer_read(). Now looking at it, the later only parses a 0x10 region, so total is 0x30 indeed. > >> where 0x1000 comes from? The P4/A7 aliases? > > Yes, as the commit message said. Since this was a last minute change I > tried to be safe and not change anything guest visible at this point. We should avoid last minute changes days before soft freeze ;) >> If you have a way to test and ack, I can replace by 0x40 when applying. > > If you think 0x40 is better then I'm fine with that but I don't see a > register after 0x2c which is 32 bits so 0x30 length should be enough > according to that. Yes, 0x30 is correct, I'll take your v7 patch. Thanks, Phil.
diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c index 250ad41b48..a6445092e4 100644 --- a/hw/timer/sh_timer.c +++ b/hw/timer/sh_timer.c @@ -350,8 +350,7 @@ void tmu012_init(MemoryRegion *sysmem, hwaddr base, int feat, uint32_t freq, ch2_irq0); /* ch2_irq1 not supported */ } - memory_region_init_io(&s->iomem, NULL, &tmu012_ops, s, - "timer", 0x100000000ULL); + memory_region_init_io(&s->iomem, NULL, &tmu012_ops, s, "timer", 0x1000); memory_region_init_alias(&s->iomem_p4, NULL, "timer-p4", &s->iomem, 0, 0x1000);
The timer memory region is only accessed via aliases that are 0x1000 bytes long, no need to have the timer region larger than that. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/timer/sh_timer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)