| Message ID | 20210812093356.1946-20-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm: Get rid of system_clock_scale global | expand |
On 8/12/21 11:33 AM, Peter Maydell wrote: > Instead of passing the MSF2 SoC an integer property specifying the > CPU clock rate, pass it a Clock instead. This lets us wire that > clock up to the armv7m object. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > include/hw/arm/msf2-soc.h | 3 ++- > hw/arm/msf2-soc.c | 28 +++++++++++++++++----------- > hw/arm/msf2-som.c | 7 ++++++- > 3 files changed, 25 insertions(+), 13 deletions(-) > > diff --git a/include/hw/arm/msf2-soc.h b/include/hw/arm/msf2-soc.h > index 38e10ce20aa..01f904cec47 100644 > --- a/include/hw/arm/msf2-soc.h > +++ b/include/hw/arm/msf2-soc.h > @@ -30,6 +30,7 @@ > #include "hw/misc/msf2-sysreg.h" > #include "hw/ssi/mss-spi.h" > #include "hw/net/msf2-emac.h" > +#include "hw/clock.h" > #include "qom/object.h" > > #define TYPE_MSF2_SOC "msf2-soc" > @@ -57,7 +58,7 @@ struct MSF2State { > uint64_t envm_size; > uint64_t esram_size; > > - uint32_t m3clk; > + Clock *m3clk; > uint8_t apb0div; > uint8_t apb1div; > > diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c > index f36788054b3..0a1e594aee6 100644 > --- a/hw/arm/msf2-soc.c > +++ b/hw/arm/msf2-soc.c > @@ -29,6 +29,7 @@ > #include "hw/char/serial.h" > #include "hw/arm/msf2-soc.h" > #include "hw/misc/unimp.h" > +#include "hw/qdev-clock.h" > #include "sysemu/sysemu.h" > > #define MSF2_TIMER_BASE 0x40004000 > @@ -73,6 +74,8 @@ static void m2sxxx_soc_initfn(Object *obj) > } > > object_initialize_child(obj, "emac", &s->emac, TYPE_MSS_EMAC); > + > + s->m3clk = qdev_init_clock_in(DEVICE(obj), "m3clk", NULL, NULL, 0); > } > > static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) > @@ -84,6 +87,11 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) > > MemoryRegion *system_memory = get_system_memory(); > > + if (!clock_has_source(s->m3clk)) { > + error_setg(errp, "m3clk must be wired up by the board code"); > + return; > + } > + > memory_region_init_rom(&s->nvm, OBJECT(dev_soc), "MSF2.eNVM", s->envm_size, > &error_fatal); > /* > @@ -106,19 +114,14 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) > qdev_prop_set_uint32(armv7m, "num-irq", 81); > qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type); > qdev_prop_set_bit(armv7m, "enable-bitband", true); > + qdev_connect_clock_in(armv7m, "cpuclk", s->m3clk); > object_property_set_link(OBJECT(&s->armv7m), "memory", > OBJECT(get_system_memory()), &error_abort); > if (!sysbus_realize(SYS_BUS_DEVICE(&s->armv7m), errp)) { > return; > } > > - if (!s->m3clk) { > - error_setg(errp, "Invalid m3clk value"); > - error_append_hint(errp, "m3clk can not be zero\n"); > - return; > - } > - > - system_clock_scale = NANOSECONDS_PER_SECOND / s->m3clk; > + system_clock_scale = clock_ticks_to_ns(s->m3clk, 1); > > for (i = 0; i < MSF2_NUM_UARTS; i++) { > if (serial_hd(i)) { > @@ -129,8 +132,13 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) > } > > dev = DEVICE(&s->timer); > - /* APB0 clock is the timer input clock */ > - qdev_prop_set_uint32(dev, "clock-frequency", s->m3clk / s->apb0div); > + /* > + * APB0 clock is the timer input clock. > + * TODO: ideally the MSF2 timer device should use a Clock rather than a > + * clock-frequency integer property. > + */ > + qdev_prop_set_uint32(dev, "clock-frequency", > + clock_get_hz(s->m3clk) / s->apb0div); > if (!sysbus_realize(SYS_BUS_DEVICE(&s->timer), errp)) { > return; > } > @@ -207,8 +215,6 @@ static Property m2sxxx_soc_properties[] = { > DEFINE_PROP_UINT64("eNVM-size", MSF2State, envm_size, MSF2_ENVM_MAX_SIZE), > DEFINE_PROP_UINT64("eSRAM-size", MSF2State, esram_size, > MSF2_ESRAM_MAX_SIZE), > - /* Libero GUI shows 100Mhz as default for clocks */ > - DEFINE_PROP_UINT32("m3clk", MSF2State, m3clk, 100 * 1000000), > /* default divisors in Libero GUI */ > DEFINE_PROP_UINT8("apb0div", MSF2State, apb0div, 2), > DEFINE_PROP_UINT8("apb1div", MSF2State, apb1div, 2), > diff --git a/hw/arm/msf2-som.c b/hw/arm/msf2-som.c > index 343ec977c07..396e8b99138 100644 > --- a/hw/arm/msf2-som.c > +++ b/hw/arm/msf2-som.c > @@ -29,6 +29,7 @@ > #include "hw/boards.h" > #include "hw/qdev-properties.h" > #include "hw/arm/boot.h" > +#include "hw/qdev-clock.h" > #include "exec/address-spaces.h" > #include "hw/arm/msf2-soc.h" > > @@ -49,6 +50,7 @@ static void emcraft_sf2_s2s010_init(MachineState *machine) > BusState *spi_bus; > MemoryRegion *sysmem = get_system_memory(); > MemoryRegion *ddr = g_new(MemoryRegion, 1); > + Clock *m3clk; > > if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) { > error_report("This board can only be used with CPU %s", > @@ -72,7 +74,10 @@ static void emcraft_sf2_s2s010_init(MachineState *machine) > * in Libero. CPU clock is divided by APB0 and APB1 divisors for > * peripherals. Emcraft's SoM kit comes with these settings by default. > */ > - qdev_prop_set_uint32(dev, "m3clk", 142 * 1000000); > + /* This clock doesn't need migration because it is fixed-frequency */ > + m3clk = clock_new(OBJECT(machine), "m3clk"); > + clock_set_hz(m3clk, 142 * 1000000); Maybe something could be added in the commit message to say that M3_CLK is changed from 100MHz to 142MHz. I do not know the SmartFusion2 but the clocking guide seems to agree with 142MHz: https://www.microsemi.com/document-portal/doc_download/132012-ug0449-smartfusion2-and-igloo2-clocking-resources-user-guide > + qdev_connect_clock_in(dev, "m3clk", m3clk); > qdev_prop_set_uint32(dev, "apb0div", 2); > qdev_prop_set_uint32(dev, "apb1div", 2); > > Reviewed-by: Alexandre Iooss <erdnaxe@crans.org> Thanks, -- Alexandre
On Sat, 14 Aug 2021 at 10:20, Alexandre IOOSS <erdnaxe@crans.org> wrote: > > > On 8/12/21 11:33 AM, Peter Maydell wrote: > > Instead of passing the MSF2 SoC an integer property specifying the > > CPU clock rate, pass it a Clock instead. This lets us wire that > > clock up to the armv7m object. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > @@ -72,7 +74,10 @@ static void emcraft_sf2_s2s010_init(MachineState *machine) > > * in Libero. CPU clock is divided by APB0 and APB1 divisors for > > * peripherals. Emcraft's SoM kit comes with these settings by default. > > */ > > - qdev_prop_set_uint32(dev, "m3clk", 142 * 1000000); > > + /* This clock doesn't need migration because it is fixed-frequency */ > > + m3clk = clock_new(OBJECT(machine), "m3clk"); > > + clock_set_hz(m3clk, 142 * 1000000); > > Maybe something could be added in the commit message to say that M3_CLK > is changed from 100MHz to 142MHz. I'm not sure what you mean here? This commit doesn't change the frequency: we previously set the m3clk property to "142 * 1000000" and now we set the clock's hz setting to the same thing. The old m3clk property had a default value of 100 * 1000000, but nothing ever used that because the only user of the device (this board code) set the property explicitly to some value. With the new Clock-based setup there is no default value at all, because the board code must always connect a clock, and will set its frequency to whatever is right for that board. -- PMM
On 8/14/21 12:11 PM, Peter Maydell wrote: > On Sat, 14 Aug 2021 at 10:20, Alexandre IOOSS <erdnaxe@crans.org> wrote: >> >> >> On 8/12/21 11:33 AM, Peter Maydell wrote: >>> Instead of passing the MSF2 SoC an integer property specifying the >>> CPU clock rate, pass it a Clock instead. This lets us wire that >>> clock up to the armv7m object. >>> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > >>> @@ -72,7 +74,10 @@ static void emcraft_sf2_s2s010_init(MachineState *machine) >>> * in Libero. CPU clock is divided by APB0 and APB1 divisors for >>> * peripherals. Emcraft's SoM kit comes with these settings by default. >>> */ >>> - qdev_prop_set_uint32(dev, "m3clk", 142 * 1000000); >>> + /* This clock doesn't need migration because it is fixed-frequency */ >>> + m3clk = clock_new(OBJECT(machine), "m3clk"); >>> + clock_set_hz(m3clk, 142 * 1000000); >> >> Maybe something could be added in the commit message to say that M3_CLK >> is changed from 100MHz to 142MHz. > > I'm not sure what you mean here? This commit doesn't change the frequency: > we previously set the m3clk property to "142 * 1000000" and now we set the > clock's hz setting to the same thing. My bad, I did not realize the board was already setting the frequency to 142MHz. Thanks, -- Alexandre
diff --git a/include/hw/arm/msf2-soc.h b/include/hw/arm/msf2-soc.h index 38e10ce20aa..01f904cec47 100644 --- a/include/hw/arm/msf2-soc.h +++ b/include/hw/arm/msf2-soc.h @@ -30,6 +30,7 @@ #include "hw/misc/msf2-sysreg.h" #include "hw/ssi/mss-spi.h" #include "hw/net/msf2-emac.h" +#include "hw/clock.h" #include "qom/object.h" #define TYPE_MSF2_SOC "msf2-soc" @@ -57,7 +58,7 @@ struct MSF2State { uint64_t envm_size; uint64_t esram_size; - uint32_t m3clk; + Clock *m3clk; uint8_t apb0div; uint8_t apb1div; diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c index f36788054b3..0a1e594aee6 100644 --- a/hw/arm/msf2-soc.c +++ b/hw/arm/msf2-soc.c @@ -29,6 +29,7 @@ #include "hw/char/serial.h" #include "hw/arm/msf2-soc.h" #include "hw/misc/unimp.h" +#include "hw/qdev-clock.h" #include "sysemu/sysemu.h" #define MSF2_TIMER_BASE 0x40004000 @@ -73,6 +74,8 @@ static void m2sxxx_soc_initfn(Object *obj) } object_initialize_child(obj, "emac", &s->emac, TYPE_MSS_EMAC); + + s->m3clk = qdev_init_clock_in(DEVICE(obj), "m3clk", NULL, NULL, 0); } static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) @@ -84,6 +87,11 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) MemoryRegion *system_memory = get_system_memory(); + if (!clock_has_source(s->m3clk)) { + error_setg(errp, "m3clk must be wired up by the board code"); + return; + } + memory_region_init_rom(&s->nvm, OBJECT(dev_soc), "MSF2.eNVM", s->envm_size, &error_fatal); /* @@ -106,19 +114,14 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) qdev_prop_set_uint32(armv7m, "num-irq", 81); qdev_prop_set_string(armv7m, "cpu-type", s->cpu_type); qdev_prop_set_bit(armv7m, "enable-bitband", true); + qdev_connect_clock_in(armv7m, "cpuclk", s->m3clk); object_property_set_link(OBJECT(&s->armv7m), "memory", OBJECT(get_system_memory()), &error_abort); if (!sysbus_realize(SYS_BUS_DEVICE(&s->armv7m), errp)) { return; } - if (!s->m3clk) { - error_setg(errp, "Invalid m3clk value"); - error_append_hint(errp, "m3clk can not be zero\n"); - return; - } - - system_clock_scale = NANOSECONDS_PER_SECOND / s->m3clk; + system_clock_scale = clock_ticks_to_ns(s->m3clk, 1); for (i = 0; i < MSF2_NUM_UARTS; i++) { if (serial_hd(i)) { @@ -129,8 +132,13 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp) } dev = DEVICE(&s->timer); - /* APB0 clock is the timer input clock */ - qdev_prop_set_uint32(dev, "clock-frequency", s->m3clk / s->apb0div); + /* + * APB0 clock is the timer input clock. + * TODO: ideally the MSF2 timer device should use a Clock rather than a + * clock-frequency integer property. + */ + qdev_prop_set_uint32(dev, "clock-frequency", + clock_get_hz(s->m3clk) / s->apb0div); if (!sysbus_realize(SYS_BUS_DEVICE(&s->timer), errp)) { return; } @@ -207,8 +215,6 @@ static Property m2sxxx_soc_properties[] = { DEFINE_PROP_UINT64("eNVM-size", MSF2State, envm_size, MSF2_ENVM_MAX_SIZE), DEFINE_PROP_UINT64("eSRAM-size", MSF2State, esram_size, MSF2_ESRAM_MAX_SIZE), - /* Libero GUI shows 100Mhz as default for clocks */ - DEFINE_PROP_UINT32("m3clk", MSF2State, m3clk, 100 * 1000000), /* default divisors in Libero GUI */ DEFINE_PROP_UINT8("apb0div", MSF2State, apb0div, 2), DEFINE_PROP_UINT8("apb1div", MSF2State, apb1div, 2), diff --git a/hw/arm/msf2-som.c b/hw/arm/msf2-som.c index 343ec977c07..396e8b99138 100644 --- a/hw/arm/msf2-som.c +++ b/hw/arm/msf2-som.c @@ -29,6 +29,7 @@ #include "hw/boards.h" #include "hw/qdev-properties.h" #include "hw/arm/boot.h" +#include "hw/qdev-clock.h" #include "exec/address-spaces.h" #include "hw/arm/msf2-soc.h" @@ -49,6 +50,7 @@ static void emcraft_sf2_s2s010_init(MachineState *machine) BusState *spi_bus; MemoryRegion *sysmem = get_system_memory(); MemoryRegion *ddr = g_new(MemoryRegion, 1); + Clock *m3clk; if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) { error_report("This board can only be used with CPU %s", @@ -72,7 +74,10 @@ static void emcraft_sf2_s2s010_init(MachineState *machine) * in Libero. CPU clock is divided by APB0 and APB1 divisors for * peripherals. Emcraft's SoM kit comes with these settings by default. */ - qdev_prop_set_uint32(dev, "m3clk", 142 * 1000000); + /* This clock doesn't need migration because it is fixed-frequency */ + m3clk = clock_new(OBJECT(machine), "m3clk"); + clock_set_hz(m3clk, 142 * 1000000); + qdev_connect_clock_in(dev, "m3clk", m3clk); qdev_prop_set_uint32(dev, "apb0div", 2); qdev_prop_set_uint32(dev, "apb1div", 2);
Instead of passing the MSF2 SoC an integer property specifying the CPU clock rate, pass it a Clock instead. This lets us wire that clock up to the armv7m object. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- include/hw/arm/msf2-soc.h | 3 ++- hw/arm/msf2-soc.c | 28 +++++++++++++++++----------- hw/arm/msf2-som.c | 7 ++++++- 3 files changed, 25 insertions(+), 13 deletions(-) -- 2.20.1