On 21 September 2018 at 06:39, Damien Hedde <damien.he...@greensocs.com> wrote: > On 09/19/2018 11:30 PM, Peter Maydell wrote: >> There are several possible approaches here I think: >> >> (1) the "clock" object holds no internal state; if a device on the >> destination end of a clock connection cares about clock state then >> it keeps and updates a copy of that state when the callback is called, >> and it is responsible for migrating that copy along with all its other >> state. This is how qemu_irq/gpio lines work. >> (2) the "clock" object does hold internal state, and it is owned >> by the source-end device, which is responsible for migrating that >> state. This is how ptimer objects work -- hw/core/ptimer.c defines >> a vmstate struct, but it is the devices that use a ptimer that >> put a VMSTATE_PTIMER entry in their vmstate structs to migrate the data. >> (3) the "clock" object can be a fully fledged device (ie a subclass >> of TYPE_DEVICE) which migrates its state entirely by itself. >> >> I don't have a firm view currently on which would be best here, >> but I guess I lean towards 2. 1 has the advantage of "just like >> qemu_irq" but the disadvantage that the destination end has no >> way to query the current clock value so has to manually track it >> itself. 3 is probably overkill here (and also makes it hard to >> retain migration backward compatibility when adding clock tree >> support to an existing machine model). > > I agree with you on doing approach 2. If the clock state needs to be at > the end, it seems best to put in inside the clock object. It will save > codelines in devices. Thanks for the tips about ptimer. > > I don't see how approach 3 solves the problem since the clock state will > still be migrated by another object (instead of begin the device which > generate the clock, it is now the clock input object). So a device (with > an input clock) has no guarantee on the clock value being correct when > it will handle its own migration. I think the clock vmstate entry needs > to be present in the device's vmsd (or am I missing something ?).
The point about (3) is that every TYPE_DEVICE object manages migration of its own state, so the device which has the clock output does not need to. No device should ever care about whether other devices in the system have had their state loaded on a migration or not yet: their migration load must affect only their own internal state. If you find yourself in a position where you need to care then you've probably got some part of the design wrong. (The difference between 2 and 3 is that in 2 the clock-object is not a full device, so it's just a part of the output-end device and the output-end device does its state migration. In 3 it is a full device and does its own migration.) > Regarding backward compatibility on migration, I think we have 2 options: > (A) keep updating outputs clocks in post_load. > (B) rely on device with an input clock to setup a "good" default value > to unmigrated input clocks. I think what you need (assuming a type 2 design) is for there to be a function on the clock object which says "here's your state, but don't tell the output end" (or just directly set the clock struct fields). That way the output end device can in its post-load function use that if it is doing a migration from an old version that didn't include the clock device. The input end device can't help you because it is not in a position to change the state of the clock object, which belongs to the output end. (Consider also the case where one clock connects to multiple inputs -- the input end can't set the value, because the different inputs might have different ideas of the right thing.) >> I don't really understand why reset is related here. Clock trees and >> reset domains don't sit in a 1-to-1 relationship, generally. Reset >> is a complicated and painful area and I think I would prefer to see >> a patchset which aimed to solve the clocktree modelling problem >> without dragging in the complexities of reset modelling. > > OK. > > Do you think I should do a reroll right now with this 2 modifications > without waiting further review ? I think that's probably a good idea, yes. (I have been thinking a bit about the reset problem this week and will see if I can write up my thoughts on it next week.) thanks -- PMM
