On 10/5/20 7:44 PM, Eduardo Habkost wrote: > On Mon, Oct 05, 2020 at 06:40:09PM +0200, Igor Mammedov wrote: >> On Wed, 30 Sep 2020 12:16:53 +0200 >> Philippe Mathieu-Daudé <f4...@amsat.org> wrote: >> >>> +arm/ppc/riscv folks >>> >>> On 9/30/20 9:43 AM, Igor Mammedov wrote: >>>> On Mon, 28 Sep 2020 19:15:24 +0200 >>>> Philippe Mathieu-Daudé <f4...@amsat.org> wrote: >>>> >>>>> Let CPUState have a clock source (named 'clk') and CPUClass > > The language here confuses me: is this a clock source inside the > CPU, or just a clock input that can be connected to a clock > source somewhere?
2nd description, "somewhere". I'll reword. > > See also comment below[1]. > >>>>> have a clock_update() callback. The clock can be optionally >>>>> set Using qdev_connect_clock_in() from the Clock API. >>>>> If the clock changes, the optional clock_update() will be >>>>> called. > > What does "clock change" means? Is this just about the > frequency, or something else? A frequency changes -- which can be because a parent (in the clock tree) changed its source using a MUX. > > (By reading the Clock API documentation, it looks like it only > represents the clock frequency, but I'm not sure) > >>>> >>>> the sole user of it is mips cpu, so question is why >>>> you are making it part of generic CPUm instead of >>>> MIPSCPUClass/MIPSCPU? >>> >>> This is a feature of the CPU, regardless its architecture. >>> >>> I expect the other archs to start using it soon. >> >> if there aren't any plans to actually to do that, >> I'd keep it to MIPS class and generalize later when there is demand. No problem. > > I normally don't mind if a feature is generic from the beginning. > But in this case I'm inclined to agree with Igor. Unless we > expect to see arch-independent code to use CPUState.clock soon > (do we?), having CPUState.clock existing but unused by most > architectures would be misleading. > > Also, at least on x86 there are so many different clock sources, > that I'm not sure it would be a desirable to have a generic clock > input named "clk". Well X86 is the arch I'm less confident with. Anyhow if it has multiple clock sources, I'd expect a Clock MUX block to select an unique clock to feed the CPU. > >> >>> >>>> >>>>> >>>>> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> >>>>> --- >>>>> include/hw/core/cpu.h | 5 +++++ >>>>> hw/core/cpu.c | 12 ++++++++++++ >>>>> 2 files changed, 17 insertions(+) >>>>> >>>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >>>>> index 6c34798c8b3..6989d90c193 100644 >>>>> --- a/include/hw/core/cpu.h >>>>> +++ b/include/hw/core/cpu.h >>>>> @@ -31,6 +31,7 @@ >>>>> #include "qemu/thread.h" >>>>> #include "qemu/plugin.h" >>>>> #include "qom/object.h" >>>>> +#include "hw/clock.h" >>>>> >>>>> typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size, >>>>> void *opaque); >>>>> @@ -155,6 +156,7 @@ struct TranslationBlock; >>>>> * @disas_set_info: Setup architecture specific components of >>>>> disassembly info >>>>> * @adjust_watchpoint_address: Perform a target-specific adjustment to an >>>>> * address before attempting to match it against watchpoints. >>>>> + * @clock_update: Callback for input clock changes >>>>> * >>>>> * Represents a CPU family or model. >>>>> */ >>>>> @@ -176,6 +178,7 @@ struct CPUClass { >>>>> unsigned size, MMUAccessType >>>>> access_type, >>>>> int mmu_idx, MemTxAttrs attrs, >>>>> MemTxResult response, uintptr_t >>>>> retaddr); >>>>> + void (*clock_update)(CPUState *cpu); >>>>> bool (*virtio_is_big_endian)(CPUState *cpu); >>>>> int (*memory_rw_debug)(CPUState *cpu, vaddr addr, >>>>> uint8_t *buf, int len, bool is_write); >>>>> @@ -316,6 +319,7 @@ struct qemu_work_item; >>>>> * QOM parent. >>>>> * @nr_cores: Number of cores within this CPU package. >>>>> * @nr_threads: Number of threads within this CPU. >>>>> + * @clock: this CPU source clock (an output clock of another device) > > [1] > > What does "source clock" means? Is this the same as "clock input"? Yes, for clocks it is common to use source/sink instead of input/output. I'll try to reword. > > >>>>> * @running: #true if CPU is currently running (lockless). >>>>> * @has_waiter: #true if a CPU is currently waiting for the cpu_exec_end; >>>>> * valid under cpu_list_lock. >>>>> @@ -400,6 +404,7 @@ struct CPUState { >>>>> int num_ases; >>>>> AddressSpace *as; >>>>> MemoryRegion *memory; >>>>> + Clock *clock; >>>>> >>>>> void *env_ptr; /* CPUArchState */ >>>>> IcountDecr *icount_decr_ptr; >>>>> diff --git a/hw/core/cpu.c b/hw/core/cpu.c >>>>> index c55c09f734c..37fcff3ec64 100644 >>>>> --- a/hw/core/cpu.c >>>>> +++ b/hw/core/cpu.c >>>>> @@ -30,6 +30,7 @@ >>>>> #include "qemu/qemu-print.h" >>>>> #include "sysemu/tcg.h" >>>>> #include "hw/boards.h" >>>>> +#include "hw/qdev-clock.h" >>>>> #include "hw/qdev-properties.h" >>>>> #include "trace/trace-root.h" >>>>> #include "qemu/plugin.h" >>>>> @@ -247,6 +248,16 @@ void cpu_reset(CPUState *cpu) >>>>> trace_guest_cpu_reset(cpu); >>>>> } >>>>> >>>>> +static void cpu_clk_update(void *opaque) >>>>> +{ >>>>> + CPUState *cpu = opaque; >>>>> + CPUClass *cc = CPU_GET_CLASS(cpu); >>>>> + >>>>> + if (cc->clock_update) { >>>>> + cc->clock_update(cpu); >>>>> + } >>>>> +} >>>>> + >>>>> static void cpu_common_reset(DeviceState *dev) >>>>> { >>>>> CPUState *cpu = CPU(dev); >>>>> @@ -367,6 +378,7 @@ static void cpu_common_initfn(Object *obj) >>>>> /* the default value is changed by qemu_init_vcpu() for softmmu */ >>>>> cpu->nr_cores = 1; >>>>> cpu->nr_threads = 1; >>>>> + cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk", cpu_clk_update, >>>>> cpu); >>>>> >>>>> qemu_mutex_init(&cpu->work_mutex); >>>>> QSIMPLEQ_INIT(&cpu->work_list); >>>> >>> >> >