Re: [PATCH v3 12/20] hw/mips/r4k: Explicit CPU frequency is 200 MHz

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 12 Oct 2020 10:34:48 +0200
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Hi Huacai,
> 
> On 10/11/20 5:52 AM, chen huacai wrote:
> > Hi, Philippe,
> > 
> > On Sun, Oct 11, 2020 at 4:43 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:  
> >>
> >> Since its introduction in commit 6af0bf9c7c3,
> >> the 'r4k' machine runs at 200 MHz.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >>   hw/mips/r4k.c | 8 +++++++-
> >>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/mips/r4k.c b/hw/mips/r4k.c
> >> index 3487013a4a1..e64687b505a 100644
> >> --- a/hw/mips/r4k.c
> >> +++ b/hw/mips/r4k.c
> >> @@ -37,6 +37,7 @@
> >>   #include "sysemu/reset.h"
> >>   #include "sysemu/runstate.h"
> >>   #include "qemu/error-report.h"
> >> +#include "hw/qdev-clock.h"
> >>
> >>   #define MAX_IDE_BUS 2
> >>
> >> @@ -184,6 +185,7 @@ void mips_r4k_init(MachineState *machine)
> >>       int bios_size;
> >>       MIPSCPU *cpu;
> >>       CPUMIPSState *env;
> >> +    Clock *cpuclk;
> >>       ResetData *reset_info;
> >>       int i;
> >>       qemu_irq *i8259;
> >> @@ -193,7 +195,11 @@ void mips_r4k_init(MachineState *machine)
> >>       int be;
> >>
> >>       /* init CPUs */
> >> -    cpu = MIPS_CPU(cpu_create(machine->cpu_type));
> >> +    cpu = MIPS_CPU(object_new(machine->cpu_type));
> >> +    cpuclk = clock_new(OBJECT(machine), "cpu-refclk");
> >> +    clock_set_hz(cpuclk, 200000000); /* 200 MHz */
> >> +    qdev_connect_clock_in(DEVICE(cpu), "clk-in", cpuclk);
> >> +    qdev_realize(DEVICE(cpu), NULL, &error_abort);  
>  >
> > Can we add a new parameter to cpu_create() and set the freq in the core code?  
> 
> Adding a new parameter seems a good idea.
> 
> Both maintainers of the core code are reluctant to add
> a CPU clock to the core code, see:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg747589.html
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg747612.html
> Which is why I restricted that to the MIPS CPUs.
> 
the reason for that is that it's used only by a handful of devices
and not widespread.

> On ARM, Damien started to use clocks on the Zynq SoC (merged):
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg694604.html
> Luc is working on adding a clock manager to the Broadcom SoC:
> https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg02840.html
> 
> I also started converting one UART devices:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg727972.html
> 
> So IMO the core code will soon require this. Maybe we will add it
> during the next development window.
> 
> Meanwhile I can add a mips_cpu_create_with_clock() in preparation

boards that need property settings in between object_new() and realize()
typically open code just that and/or use plug/preplug handler(callback)
to set properties. preplug handler should work with cpu_create() just fine.

If you have multiple mips boards that will new clock mechanism, 
mips_cpu_create_with_clock() wrapper would work as well (it's a bit less
complicated compared to preplug handler but not too much)

> of cpu_create_with_clock().
> 
> > 
> > Huacai  
> >>       env = &cpu->env;
> >>
> >>       reset_info = g_malloc0(sizeof(ResetData));
> >> --
> >> 2.26.2
> >>  
> > 
> >   
>