| Message ID | 20181002142443.30976-1-damien.hedde@greensocs.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Clock framework API. | expand |
Hi Damien, On 02/10/2018 16:24, Damien Hedde wrote: > This series aims to add a way to model clocks in qemu between devices. > This allows to model the clock tree of a platform allowing us to inspect clock > configuration and detect problems such as disabled clock or bad configured > pll. > > This series is a reroll of the v4 sent recently without the reset feature as > requested by Peter. I also took into account the reviews about migration and > other suggestions. > > This framework was originally discussed in 2017, here: > https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07218.html > > For the user, the framework is now very similar to the device's gpio API. > Clocks inputs and outputs can be added in devices during initialization phase. > Then an input can be connected to an output: it means every time the output > clock changes, a callback in the input is triggered allowing any action to be > taken. A difference with gpios is that several inputs can be connected to a > single output without doing any glue. > > Behind the scene, there is 2 objects: a clock input which is a placeholder for > a callback, and a clock output which is a list of inputs. The value transferred > between an output and an input is an integer representing the clock frequency. > The input clock callback is called every time the clock frequency changes. > The input side holds a cached value of the frequency (the output does not need > one). This allows a device to fetch its input clock frequency at any time > without caching it itself. > > This internal state is added to handle migration in order not to update and > propagate clocks during it (it adds cross-device and order-specific effects). > Each device handles its input clock migration by adding the clock frequency in > its own vmstate description. > > Regarding the migration strategy, discussion started in the v4 patches. > The problem is that we add some kind of wire between different devices and > we face propagation issue. > The chosen solution allows migration compatibility from a platform version > with no implemented clocks to a platform with clocks. A migrated device that > have a new input clock is responsible to initialize its frequency during > migration. Each input clock having its own state, such initialization will not > have any side-effect on other input clock connected to the same output. > Output clocks, having no state, are not set during migration: If a clock output > frequency changes due to migration (eg: clock computation bug-fix), the effects > will be delayed. Eventually the clock output will be updated after migration if > some (software) event trigger a clock update, but it can not be guaranteed. > > There is also the problem of initialization which is very much like the > migration. Currently, in the zynq example, clocks outputs are initialized in > the clock controller device_reset. According to Peter, outputs should not be > set in device_reset, so we need a better way. > > It is not simple, since we can have complicated cases with several propagation > step. > We can't initialize outputs (without propagating) during device init and uses > inputs value in device_reset to complete the initialization. > Consider the case where we have a device generating a frequency, another device > doing a clock division, then a device using this clock. > [DevClockGenerator] -> clk1 -> [DevClockDiv] -> clk2 -> [Dev] > I don't see how we can handle such initialization without doing some > propagation phase at some point. > > Regarding clock gating. The 0 frequency value means the clock is gated. > If need be, a gate device can be built taking an input gpio and clock and > generating an output clock. > > I've tested this patchset running Xilinx's Linux on the xilinx-zynq-a9 machine. > Clocks are correctly updated and we ends up with a configured baudrate of 115601 > on the console uart (for a theoretical 115200) which is nice. "cadence_uart*" and > "clock*" traces can be enabled to see what's going on in this platform. > > We are considering switching to a generic payload evolution of this API. > For example by specifying the qom carried type when adding an input/output to > a device. This would allow us, for example, to add a power input port to handle > power gating or others ports without modifying the device state structure. > > Any comments and suggestion are welcomed. How would you instanciate devices and connect their clocks from the command line (with the -device option)? Should clocked devices have DeviceClass::user_creatable = false by default? Thanks, Phil.
Hi Philippe, On 10/4/18 6:13 PM, Philippe Mathieu-Daudé wrote: > Hi Damien, > > On 02/10/2018 16:24, Damien Hedde wrote: >> This series aims to add a way to model clocks in qemu between devices. >> This allows to model the clock tree of a platform allowing us to inspect clock >> configuration and detect problems such as disabled clock or bad configured >> pll. >> >> [...] >> >> Any comments and suggestion are welcomed. > > How would you instanciate devices and connect their clocks from the > command line (with the -device option)? I didn't not thought about that. I'm not sure to understand how this is done for a gpio for example. Is this done by setting the link property manually ? If that's the case, I suppose we can somehow add a link property on the input side to the output side to achieve that. The other way seem not possible due to the fact the output side has a dynamically allocated list of inputs. > > Should clocked devices have DeviceClass::user_creatable = false by default? It would be a solution, but I don't think we want this ? > > Thanks, > > Phil. > Thanks, Damien
On 11 October 2018 at 17:20, Damien Hedde <damien.hedde@greensocs.com> wrote: > > Hi Philippe, > > On 10/4/18 6:13 PM, Philippe Mathieu-Daudé wrote: >> Hi Damien, >> >> On 02/10/2018 16:24, Damien Hedde wrote: >>> This series aims to add a way to model clocks in qemu between devices. >>> This allows to model the clock tree of a platform allowing us to inspect clock >>> configuration and detect problems such as disabled clock or bad configured >>> pll. >>> >>> [...] >>> >>> Any comments and suggestion are welcomed. >> >> How would you instanciate devices and connect their clocks from the >> command line (with the -device option)? > I didn't not thought about that. I'm not sure to understand how this is > done for a gpio for example. Is this done by setting the link property > manually ? You can't wire up GPIOs on the command line. I don't think we really need to be able to wire up clocks on the command line either, do we? > Should clocked devices have DeviceClass::user_creatable = false by default? How many devices have a clock and nothing else that would cause them to be non-user-creatable (ie no GPIOs, no IRQ lines, no memory-mapped memory regions) ? thanks -- PMM
On 11/10/2018 18:23, Peter Maydell wrote: > On 11 October 2018 at 17:20, Damien Hedde <damien.hedde@greensocs.com> wrote: >> >> Hi Philippe, >> >> On 10/4/18 6:13 PM, Philippe Mathieu-Daudé wrote: >>> Hi Damien, >>> >>> On 02/10/2018 16:24, Damien Hedde wrote: >>>> This series aims to add a way to model clocks in qemu between devices. >>>> This allows to model the clock tree of a platform allowing us to inspect clock >>>> configuration and detect problems such as disabled clock or bad configured >>>> pll. >>>> >>>> [...] >>>> >>>> Any comments and suggestion are welcomed. >>> >>> How would you instanciate devices and connect their clocks from the >>> command line (with the -device option)? >> I didn't not thought about that. I'm not sure to understand how this is >> done for a gpio for example. Is this done by setting the link property >> manually ? > > You can't wire up GPIOs on the command line. I don't think we > really need to be able to wire up clocks on the command line either, > do we? This would be a big mess. > >> Should clocked devices have DeviceClass::user_creatable = false by default? > > How many devices have a clock and nothing else that would cause > them to be non-user-creatable (ie no GPIOs, no IRQ lines, no > memory-mapped memory regions) ? I'm not sure I understood your question. But I understand than as devices consuming GPIO/IRQ, clocked device can't be instantiate from command line, and I am happy with that.
On 11 October 2018 at 18:00, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 11/10/2018 18:23, Peter Maydell wrote: >> How many devices have a clock and nothing else that would cause >> them to be non-user-creatable (ie no GPIOs, no IRQ lines, no >> memory-mapped memory regions) ? > > I'm not sure I understood your question. Sorry, let me try rephrasing. We started with the question of whether devices with clocks should be marked not user creatable. It's already the case that devices with any of IRQs, GPIOs or memory-mapped registers can't be user created. (In particular, any sysbus device is not user-creatable by default.) So the question was intended to ask how often it matters whether we can't wire up devices with clocks on the command line. Are there any devices which would have clocks, but aren't *already* ruled out of being user creatable for other reasons? If there aren't any, it doesn't really matter much that we don't have a mechanism for command line clock wiring. So I think the answer is: * almost all users of this API framework are going to be sysbus devices; those are already not user-creatable * any device that uses this framework, and which is not a sysbus device (but instead a plain old qdev device) will need to mark itself as not-user-creatable by hand, the same as if it uses the qdev gpio APIs thanks -- PMM
On 11/10/2018 19:12, Peter Maydell wrote: > On 11 October 2018 at 18:00, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> On 11/10/2018 18:23, Peter Maydell wrote: >>> How many devices have a clock and nothing else that would cause >>> them to be non-user-creatable (ie no GPIOs, no IRQ lines, no >>> memory-mapped memory regions) ? >> >> I'm not sure I understood your question. > > Sorry, let me try rephrasing. > > We started with the question of whether devices with clocks should be > marked not user creatable. It's already the case that devices with any > of IRQs, GPIOs or memory-mapped registers can't be user created. > (In particular, any sysbus device is not user-creatable by default.) > So the question was intended to ask how often it matters whether > we can't wire up devices with clocks on the command line. > Are there any devices which would have clocks, but aren't *already* > ruled out of being user creatable for other reasons? If there > aren't any, it doesn't really matter much that we don't have > a mechanism for command line clock wiring. Thank you for clearing this out. > > So I think the answer is: > * almost all users of this API framework are going to be > sysbus devices; those are already not user-creatable Yes. > * any device that uses this framework, and which is not > a sysbus device (but instead a plain old qdev device) > will need to mark itself as not-user-creatable by hand, > the same as if it uses the qdev gpio APIs OK, you answered my first question, it is now clearer to me :) Thank you! Phil.
Hi Peter, Sorry to bother you with this, but you said some time ago you would write something about reset. On 10/2/18 4:24 PM, Damien Hedde wrote: > There is also the problem of initialization which is very much like the > migration. Currently, in the zynq example, clocks outputs are initialized in > the clock controller device_reset. According to Peter, outputs should not be > set in device_reset, so we need a better way. To illustrate the problem, lets take an example with the zynq case in the patchset. We have a 2-stage clock tree: +--------+ +----------------+ +------+ | Machine|>>base_clk>>|Clock controller|>>uart_clock>>| UART | +--------+ +----------------+ +------+ At the end of the tree, the uart uses the clock to setup the backend baudrate. The problem is that we need the whole clock path initialized before having the right final frequency. I've had some thought about it and I see several solutions 1. Set clock outputs in device_reset. But It can trigger side effects in yet-non-reseted devices. 2. Have some kind of global 2nd stage reset to do all the output propagation. 3. Try to propagate init values at platform building when doing the clock connection. 4. (1) and (2) mixed. Have a per device 2nd stage reset "clock_reset" (or any better name) method called when device_reset has been called and all clock inputs have been initialized (by other device "clock_reset"). At the end of reset phase everything should have been propagated. (1) being not-an-option and also (2) I think. (3) seems complicated at best due to the unknown clock connection order. And I'm not sure clock connection is the right moment to do this. In the general case, a clock init value can depend on on some user-set/config properties which will be applied later on I suppose (But I don't have a clue at which point this operation -- the "realize" thing -- cab be done in the platform startup sequence) Do you think (4) is sensible ? It means any device requiring clock input value will need to implement this new method. Basically this "clock_reset" is just a part of the actual device_reset being delayed. Obviously if there is a clock loop, the method will never be called. -- Damien
On 12 October 2018 at 16:26, Damien Hedde <damien.hedde@greensocs.com> wrote: > Hi Peter, > > Sorry to bother you with this, but you said some time ago you would > write something about reset. Yeah, sorry about that. I haven't found (made) the time to think the issues through yet. Let me just dump to email the notes I had. This is a very "raw" set of notes, so it's not very organised and has a lot of 'todo' bits in it... Overall question -- does anybody know of other device modelling systems that do a good job of modelling reset, and how they do it? If we can borrow a proven design that might be better tha thrashing one out ourseles... My idea was roughly that we need a multi-phase reset, something like: PHASE 1: Devices reset internal state. No calls out to other devices (ie no setting IRQ line state, etc) Some common bit of code (dev base class?) marks the device reset as being in-progress by setting dev->resetting = true. PHASE 2: Devices update their output lines (calls to irq line setting functions, clock setting, etc) In this phase devices may be the recipients of callbacks for IRQ line level changes, etc, and must cope with this. A callback can tell it's being called for a level change during reset because dev->resetting will be true. (They can update their own outputs in response to input changes.) PHASE 3: Common code (dev base class) sets dev->resetting = false. (Individual devices don't get to do anything here, unless there's some painful case that requires it.) The theory is that by decoupling reset into multiple phases we can avoid most of the reset order issues, by having the two ends do things in the right phase and/or by having the destination end be able to identify when a change is happening in the middle of reset. Compatibility with existing device reset methods: essentially the current rules are the same as the "phase 1" rules, so we can just say that the current reset code in all devices is phase 1 reset. Then we can adjust anything that needs a phase 2 (which is probably not all that many devices). Places where reset order currently matters: (this is just a list of things we should make sure our design copes with) * wired-high IRQ lines, and device outputs that are asserted as the device comes out of reset * setting PC in the generic-loader device vs CPU reset * image load (rom image reset) vs Arm M-profile reset wanting to read the reset PC/SP from memory * Arm M-profile CPUs should have INITVTOR as a config signal input * x86 CPU vs APIC reset order ?? * anything else? We should have some sort of "reset domain" object which can contain devices and also other reset domains. A reset domain's reset is simple: its phase 1 reset is "for each child, do phase 1 reset for that child", similarly for phases 2 and 3. A reset domain has an overall "reset" method that says "do phase 1 for all children, then phase 2, then phase 3". This allows things like "this SoC has a reset line which resets all its internal devices". (Do we want to give reset domains input gpio lines to trigger reset, or is being able to call a method on it sufficient?) At the moment we have reset methods on devices: * each bus handles reset for the things on the bus * the (one and only) sysbus resets all the sysbus devices * devices on no bus don't get reset at all unless something takes special measures! (notably the CPUs are reset in a very ad-hoc way at the moment) I guess this should be cleaned up by making each bus be a 'reset domain', and having a "system" reset domain which gets anything which doesn't have a parent reset domain (and which we then use for whole-board power-on reset). Calling the system reset domain's overall reset method will then do the 3-phase reset for everything. How would direct qemu_register_reset() calls fit into this scheme? (Some of them are devices that could get QOM reset methods instead. Perhaps we could get away with dumping the rest into the 'master reset' domain, or leaving them as-is? Some are for CPU reset which we definitely should clean up.) We need to handle different types of reset somehow, in particular "cold/power on reset" versus "warm reset". Can we do this just with reset domain objects, or do we need/want to pass each device's reset method a "type" argument? thanks -- PMM
On 10/16/18 5:48 PM, Peter Maydell wrote: > On 12 October 2018 at 16:26, Damien Hedde <damien.hedde@greensocs.com> wrote: >> Hi Peter, >> >> Sorry to bother you with this, but you said some time ago you would >> write something about reset. > > Yeah, sorry about that. I haven't found (made) the time to think > the issues through yet. Let me just dump to email the notes I had. > This is a very "raw" set of notes, so it's not very organised and > has a lot of 'todo' bits in it... > > > Overall question -- does anybody know of other device modelling > systems that do a good job of modelling reset, and how they do it? > If we can borrow a proven design that might be better tha > thrashing one out ourseles... > > > My idea was roughly that we need a multi-phase reset, something like: > > PHASE 1: > Devices reset internal state. No calls out to other devices > (ie no setting IRQ line state, etc) > Some common bit of code (dev base class?) marks the device > reset as being in-progress by setting dev->resetting = true. > > PHASE 2: > Devices update their output lines (calls to irq line setting > functions, clock setting, etc) > In this phase devices may be the recipients of callbacks for > IRQ line level changes, etc, and must cope with this. A > callback can tell it's being called for a level change during > reset because dev->resetting will be true. > (They can update their own outputs in response to input changes.) > > PHASE 3: > Common code (dev base class) sets dev->resetting = false. > (Individual devices don't get to do anything here, unless > there's some painful case that requires it.) > > The theory is that by decoupling reset into multiple phases > we can avoid most of the reset order issues, by having the > two ends do things in the right phase and/or by having > the destination end be able to identify when a change is > happening in the middle of reset. This strategy seems fine to me. > > Compatibility with existing device reset methods: > essentially the current rules are the same as the "phase 1" > rules, so we can just say that the current reset code in > all devices is phase 1 reset. Then we can adjust anything > that needs a phase 2 (which is probably not all that many > devices). > > Places where reset order currently matters: > (this is just a list of things we should make sure our design > copes with) > * wired-high IRQ lines, and device outputs that are asserted > as the device comes out of reset > * setting PC in the generic-loader device vs CPU reset > * image load (rom image reset) vs Arm M-profile reset wanting > to read the reset PC/SP from memory > * Arm M-profile CPUs should have INITVTOR as a config signal input > * x86 CPU vs APIC reset order ?? > * anything else? > > We should have some sort of "reset domain" object which can > contain devices and also other reset domains. A reset domain's > reset is simple: its phase 1 reset is "for each child, do > phase 1 reset for that child", similarly for phases 2 and 3. > A reset domain has an overall "reset" method that says > "do phase 1 for all children, then phase 2, then phase 3". > This allows things like "this SoC has a reset line which > resets all its internal devices". (Do we want to give > reset domains input gpio lines to trigger reset, or is > being able to call a method on it sufficient?) Consider the following use case: a platform with a controller that holds a reset line on another device (starting with reset held). At the beginning, the main (cold) reset will be triggered to start the platform. During phase 2, the controller will set the reset line of the other device (a gpio). Then we have phase 3. How do we handle this ? Do we need to "delay" the phase 3 of the device until the controller releases its reset line or do we consider the internal gpio reset line to be outside of this reset scope ? If we do that, we have 2 reset "sources": the main reset and the internal gpio and there is some kind of OR operation to do between these 2 sources. > > At the moment we have reset methods on devices: > * each bus handles reset for the things on the bus > * the (one and only) sysbus resets all the sysbus devices > * devices on no bus don't get reset at all unless something > takes special measures! (notably the CPUs are reset in > a very ad-hoc way at the moment) > I guess this should be cleaned up by making each bus be > a 'reset domain', and having a "system" reset domain which gets > anything which doesn't have a parent reset domain (and which > we then use for whole-board power-on reset). Calling the > system reset domain's overall reset method will then do > the 3-phase reset for everything> > How would direct qemu_register_reset() calls fit into this > scheme? (Some of them are devices that could get QOM reset > methods instead. Perhaps we could get away with dumping the > rest into the 'master reset' domain, or leaving them as-is? > Some are for CPU reset which we definitely should clean up.) > > We need to handle different types of reset somehow, in > particular "cold/power on reset" versus "warm reset". > Can we do this just with reset domain objects, or do we > need/want to pass each device's reset method a "type" argument? Maybe phase 1 only needs the type, phase 2 and 3 can only depends on the device state. Anyway, this reset problem seems outside of the scope of this patch set. I think I'll do a reroll with the remark I've got so far, but it will still miss the reset part to be well integrated. I can also propose something in another rfc about the 3-phase reset to advance on that part. -- Damien