| Message ID | 20210121190622.22000-18-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert CMSDK timer, watchdog, dualtimer to Clock framework | expand |
Hi Peter, On 1/21/21 8:06 PM, Peter Maydell wrote: > Create and connect the Clock input for the watchdog device on the > Stellaris boards. Because the Stellaris boards model the ability to > change the clock rate by programming PLL registers, we have to create > an output Clock on the ssys_state device and wire it up to the > watchdog. > > Note that the old comment on ssys_calculate_system_clock() got the > units wrong -- system_clock_scale is in nanoseconds, not > milliseconds. Improve the commentary to clarify how we are > calculating the period. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/arm/stellaris.c | 43 +++++++++++++++++++++++++++++++------------ > 1 file changed, 31 insertions(+), 12 deletions(-) ... > /* > - * Caculate the sys. clock period in ms. > + * Calculate the system clock period. We only want to propagate > + * this change to the rest of the system if we're not being called > + * from migration post-load. This part was not trivial to understand. I read the Clock API doc again then found: Care should be taken not to use ``clock_update[_ns|_hz]()`` or ``clock_propagate()`` during the whole migration procedure because it will trigger side effects to other devices in an unknown state. > */ > -static void ssys_calculate_system_clock(ssys_state *s) > +static void ssys_calculate_system_clock(ssys_state *s, bool propagate_clock) > { > + /* > + * SYSDIV field specifies divisor: 0 == /1, 1 == /2, etc. Input > + * clock is 200MHz, which is a period of 5 ns. Dividing the clock > + * frequency by X is the same as multiplying the period by X. > + */ > if (ssys_use_rcc2(s)) { > system_clock_scale = 5 * (((s->rcc2 >> 23) & 0x3f) + 1); > } else { > system_clock_scale = 5 * (((s->rcc >> 23) & 0xf) + 1); > } > + clock_set_ns(s->sysclk, system_clock_scale); > + if (propagate_clock) { > + clock_propagate(s->sysclk); > + } > } ... > static void stellaris_sys_reset_exit(Object *obj) > @@ -690,7 +704,7 @@ static int stellaris_sys_post_load(void *opaque, int version_id) > { > ssys_state *s = opaque; > > - ssys_calculate_system_clock(s); > + ssys_calculate_system_clock(s, false); So this makes sense. I'll keep reviewing and come back to this patch later. Regards, Phil.
On 19:06 Thu 21 Jan , Peter Maydell wrote: > Create and connect the Clock input for the watchdog device on the > Stellaris boards. Because the Stellaris boards model the ability to > change the clock rate by programming PLL registers, we have to create > an output Clock on the ssys_state device and wire it up to the > watchdog. > > Note that the old comment on ssys_calculate_system_clock() got the > units wrong -- system_clock_scale is in nanoseconds, not > milliseconds. Improve the commentary to clarify how we are > calculating the period. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Luc Michel <luc@lmichel.fr> > --- > hw/arm/stellaris.c | 43 +++++++++++++++++++++++++++++++------------ > 1 file changed, 31 insertions(+), 12 deletions(-) > > diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c > index 0194ede2fe0..9b67c739ef2 100644 > --- a/hw/arm/stellaris.c > +++ b/hw/arm/stellaris.c > @@ -26,6 +26,7 @@ > #include "hw/watchdog/cmsdk-apb-watchdog.h" > #include "migration/vmstate.h" > #include "hw/misc/unimp.h" > +#include "hw/qdev-clock.h" > #include "cpu.h" > #include "qom/object.h" > > @@ -377,6 +378,7 @@ struct ssys_state { > uint32_t clkvclr; > uint32_t ldoarst; > qemu_irq irq; > + Clock *sysclk; > /* Properties (all read-only registers) */ > uint32_t user0; > uint32_t user1; > @@ -555,15 +557,26 @@ static bool ssys_use_rcc2(ssys_state *s) > } > > /* > - * Caculate the sys. clock period in ms. > + * Calculate the system clock period. We only want to propagate > + * this change to the rest of the system if we're not being called > + * from migration post-load. > */ > -static void ssys_calculate_system_clock(ssys_state *s) > +static void ssys_calculate_system_clock(ssys_state *s, bool propagate_clock) > { > + /* > + * SYSDIV field specifies divisor: 0 == /1, 1 == /2, etc. Input > + * clock is 200MHz, which is a period of 5 ns. Dividing the clock > + * frequency by X is the same as multiplying the period by X. > + */ > if (ssys_use_rcc2(s)) { > system_clock_scale = 5 * (((s->rcc2 >> 23) & 0x3f) + 1); > } else { > system_clock_scale = 5 * (((s->rcc >> 23) & 0xf) + 1); > } > + clock_set_ns(s->sysclk, system_clock_scale); > + if (propagate_clock) { > + clock_propagate(s->sysclk); > + } > } > > static void ssys_write(void *opaque, hwaddr offset, > @@ -598,7 +611,7 @@ static void ssys_write(void *opaque, hwaddr offset, > s->int_status |= (1 << 6); > } > s->rcc = value; > - ssys_calculate_system_clock(s); > + ssys_calculate_system_clock(s, true); > break; > case 0x070: /* RCC2 */ > if (ssys_board_class(s) == DID0_CLASS_SANDSTORM) { > @@ -610,7 +623,7 @@ static void ssys_write(void *opaque, hwaddr offset, > s->int_status |= (1 << 6); > } > s->rcc2 = value; > - ssys_calculate_system_clock(s); > + ssys_calculate_system_clock(s, true); > break; > case 0x100: /* RCGC0 */ > s->rcgc[0] = value; > @@ -679,7 +692,8 @@ static void stellaris_sys_reset_hold(Object *obj) > { > ssys_state *s = STELLARIS_SYS(obj); > > - ssys_calculate_system_clock(s); > + /* OK to propagate clocks from the hold phase */ > + ssys_calculate_system_clock(s, true); > } > > static void stellaris_sys_reset_exit(Object *obj) > @@ -690,7 +704,7 @@ static int stellaris_sys_post_load(void *opaque, int version_id) > { > ssys_state *s = opaque; > > - ssys_calculate_system_clock(s); > + ssys_calculate_system_clock(s, false); > > return 0; > } > @@ -713,6 +727,7 @@ static const VMStateDescription vmstate_stellaris_sys = { > VMSTATE_UINT32_ARRAY(dcgc, ssys_state, 3), > VMSTATE_UINT32(clkvclr, ssys_state), > VMSTATE_UINT32(ldoarst, ssys_state), > + /* No field for sysclk -- handled in post-load instead */ > VMSTATE_END_OF_LIST() > } > }; > @@ -738,11 +753,12 @@ static void stellaris_sys_instance_init(Object *obj) > memory_region_init_io(&s->iomem, obj, &ssys_ops, s, "ssys", 0x00001000); > sysbus_init_mmio(sbd, &s->iomem); > sysbus_init_irq(sbd, &s->irq); > + s->sysclk = qdev_init_clock_out(DEVICE(s), "SYSCLK"); > } > > -static int stellaris_sys_init(uint32_t base, qemu_irq irq, > - stellaris_board_info * board, > - uint8_t *macaddr) > +static DeviceState *stellaris_sys_init(uint32_t base, qemu_irq irq, > + stellaris_board_info *board, > + uint8_t *macaddr) > { > DeviceState *dev = qdev_new(TYPE_STELLARIS_SYS); > SysBusDevice *sbd = SYS_BUS_DEVICE(dev); > @@ -774,7 +790,7 @@ static int stellaris_sys_init(uint32_t base, qemu_irq irq, > */ > device_cold_reset(dev); > > - return 0; > + return dev; > } > > /* I2C controller. */ > @@ -1341,6 +1357,7 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board) > int flash_size; > I2CBus *i2c; > DeviceState *dev; > + DeviceState *ssys_dev; > int i; > int j; > > @@ -1391,8 +1408,8 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board) > } > } > > - stellaris_sys_init(0x400fe000, qdev_get_gpio_in(nvic, 28), > - board, nd_table[0].macaddr.a); > + ssys_dev = stellaris_sys_init(0x400fe000, qdev_get_gpio_in(nvic, 28), > + board, nd_table[0].macaddr.a); > > > if (board->dc1 & (1 << 3)) { /* watchdog present */ > @@ -1401,6 +1418,8 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board) > /* system_clock_scale is valid now */ > uint32_t mainclk = NANOSECONDS_PER_SECOND / system_clock_scale; > qdev_prop_set_uint32(dev, "wdogclk-frq", mainclk); > + qdev_connect_clock_in(dev, "WDOGCLK", > + qdev_get_clock_out(ssys_dev, "SYSCLK")); > > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); > sysbus_mmio_map(SYS_BUS_DEVICE(dev), > -- > 2.20.1 > --
On 1/21/21 10:59 PM, Philippe Mathieu-Daudé wrote: > On 1/21/21 8:06 PM, Peter Maydell wrote: >> Create and connect the Clock input for the watchdog device on the >> Stellaris boards. Because the Stellaris boards model the ability to >> change the clock rate by programming PLL registers, we have to create >> an output Clock on the ssys_state device and wire it up to the >> watchdog. >> >> Note that the old comment on ssys_calculate_system_clock() got the >> units wrong -- system_clock_scale is in nanoseconds, not >> milliseconds. Improve the commentary to clarify how we are >> calculating the period. >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> hw/arm/stellaris.c | 43 +++++++++++++++++++++++++++++++------------ >> 1 file changed, 31 insertions(+), 12 deletions(-) > ... > >> /* >> - * Caculate the sys. clock period in ms. >> + * Calculate the system clock period. We only want to propagate >> + * this change to the rest of the system if we're not being called >> + * from migration post-load. > > This part was not trivial to understand. I read the Clock API > doc again then found: > > Care should be taken not to use ``clock_update[_ns|_hz]()`` or > ``clock_propagate()`` during the whole migration procedure because it > will trigger side effects to other devices in an unknown state. > >> */ >> -static void ssys_calculate_system_clock(ssys_state *s) >> +static void ssys_calculate_system_clock(ssys_state *s, bool propagate_clock) >> { >> + /* >> + * SYSDIV field specifies divisor: 0 == /1, 1 == /2, etc. Input >> + * clock is 200MHz, which is a period of 5 ns. Dividing the clock >> + * frequency by X is the same as multiplying the period by X. >> + */ >> if (ssys_use_rcc2(s)) { >> system_clock_scale = 5 * (((s->rcc2 >> 23) & 0x3f) + 1); >> } else { >> system_clock_scale = 5 * (((s->rcc >> 23) & 0xf) + 1); >> } >> + clock_set_ns(s->sysclk, system_clock_scale); >> + if (propagate_clock) { >> + clock_propagate(s->sysclk); >> + } >> } > ... >> static void stellaris_sys_reset_exit(Object *obj) >> @@ -690,7 +704,7 @@ static int stellaris_sys_post_load(void *opaque, int version_id) >> { >> ssys_state *s = opaque; >> >> - ssys_calculate_system_clock(s); >> + ssys_calculate_system_clock(s, false); > > So this makes sense. > > I'll keep reviewing and come back to this patch later. Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c index 0194ede2fe0..9b67c739ef2 100644 --- a/hw/arm/stellaris.c +++ b/hw/arm/stellaris.c @@ -26,6 +26,7 @@ #include "hw/watchdog/cmsdk-apb-watchdog.h" #include "migration/vmstate.h" #include "hw/misc/unimp.h" +#include "hw/qdev-clock.h" #include "cpu.h" #include "qom/object.h" @@ -377,6 +378,7 @@ struct ssys_state { uint32_t clkvclr; uint32_t ldoarst; qemu_irq irq; + Clock *sysclk; /* Properties (all read-only registers) */ uint32_t user0; uint32_t user1; @@ -555,15 +557,26 @@ static bool ssys_use_rcc2(ssys_state *s) } /* - * Caculate the sys. clock period in ms. + * Calculate the system clock period. We only want to propagate + * this change to the rest of the system if we're not being called + * from migration post-load. */ -static void ssys_calculate_system_clock(ssys_state *s) +static void ssys_calculate_system_clock(ssys_state *s, bool propagate_clock) { + /* + * SYSDIV field specifies divisor: 0 == /1, 1 == /2, etc. Input + * clock is 200MHz, which is a period of 5 ns. Dividing the clock + * frequency by X is the same as multiplying the period by X. + */ if (ssys_use_rcc2(s)) { system_clock_scale = 5 * (((s->rcc2 >> 23) & 0x3f) + 1); } else { system_clock_scale = 5 * (((s->rcc >> 23) & 0xf) + 1); } + clock_set_ns(s->sysclk, system_clock_scale); + if (propagate_clock) { + clock_propagate(s->sysclk); + } } static void ssys_write(void *opaque, hwaddr offset, @@ -598,7 +611,7 @@ static void ssys_write(void *opaque, hwaddr offset, s->int_status |= (1 << 6); } s->rcc = value; - ssys_calculate_system_clock(s); + ssys_calculate_system_clock(s, true); break; case 0x070: /* RCC2 */ if (ssys_board_class(s) == DID0_CLASS_SANDSTORM) { @@ -610,7 +623,7 @@ static void ssys_write(void *opaque, hwaddr offset, s->int_status |= (1 << 6); } s->rcc2 = value; - ssys_calculate_system_clock(s); + ssys_calculate_system_clock(s, true); break; case 0x100: /* RCGC0 */ s->rcgc[0] = value; @@ -679,7 +692,8 @@ static void stellaris_sys_reset_hold(Object *obj) { ssys_state *s = STELLARIS_SYS(obj); - ssys_calculate_system_clock(s); + /* OK to propagate clocks from the hold phase */ + ssys_calculate_system_clock(s, true); } static void stellaris_sys_reset_exit(Object *obj) @@ -690,7 +704,7 @@ static int stellaris_sys_post_load(void *opaque, int version_id) { ssys_state *s = opaque; - ssys_calculate_system_clock(s); + ssys_calculate_system_clock(s, false); return 0; } @@ -713,6 +727,7 @@ static const VMStateDescription vmstate_stellaris_sys = { VMSTATE_UINT32_ARRAY(dcgc, ssys_state, 3), VMSTATE_UINT32(clkvclr, ssys_state), VMSTATE_UINT32(ldoarst, ssys_state), + /* No field for sysclk -- handled in post-load instead */ VMSTATE_END_OF_LIST() } }; @@ -738,11 +753,12 @@ static void stellaris_sys_instance_init(Object *obj) memory_region_init_io(&s->iomem, obj, &ssys_ops, s, "ssys", 0x00001000); sysbus_init_mmio(sbd, &s->iomem); sysbus_init_irq(sbd, &s->irq); + s->sysclk = qdev_init_clock_out(DEVICE(s), "SYSCLK"); } -static int stellaris_sys_init(uint32_t base, qemu_irq irq, - stellaris_board_info * board, - uint8_t *macaddr) +static DeviceState *stellaris_sys_init(uint32_t base, qemu_irq irq, + stellaris_board_info *board, + uint8_t *macaddr) { DeviceState *dev = qdev_new(TYPE_STELLARIS_SYS); SysBusDevice *sbd = SYS_BUS_DEVICE(dev); @@ -774,7 +790,7 @@ static int stellaris_sys_init(uint32_t base, qemu_irq irq, */ device_cold_reset(dev); - return 0; + return dev; } /* I2C controller. */ @@ -1341,6 +1357,7 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board) int flash_size; I2CBus *i2c; DeviceState *dev; + DeviceState *ssys_dev; int i; int j; @@ -1391,8 +1408,8 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board) } } - stellaris_sys_init(0x400fe000, qdev_get_gpio_in(nvic, 28), - board, nd_table[0].macaddr.a); + ssys_dev = stellaris_sys_init(0x400fe000, qdev_get_gpio_in(nvic, 28), + board, nd_table[0].macaddr.a); if (board->dc1 & (1 << 3)) { /* watchdog present */ @@ -1401,6 +1418,8 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board) /* system_clock_scale is valid now */ uint32_t mainclk = NANOSECONDS_PER_SECOND / system_clock_scale; qdev_prop_set_uint32(dev, "wdogclk-frq", mainclk); + qdev_connect_clock_in(dev, "WDOGCLK", + qdev_get_clock_out(ssys_dev, "SYSCLK")); sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal); sysbus_mmio_map(SYS_BUS_DEVICE(dev),
Create and connect the Clock input for the watchdog device on the Stellaris boards. Because the Stellaris boards model the ability to change the clock rate by programming PLL registers, we have to create an output Clock on the ssys_state device and wire it up to the watchdog. Note that the old comment on ssys_calculate_system_clock() got the units wrong -- system_clock_scale is in nanoseconds, not milliseconds. Improve the commentary to clarify how we are calculating the period. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/arm/stellaris.c | 43 +++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-)