| Message ID | 20240418191602.2017-1-raphael.poggi@lynxleap.co.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/core/clock: always iterate through childs in clock_propagate_period | expand |
Hi Raphael, On 18/4/24 21:16, Raphael Poggi wrote: > When dealing with few clocks depending with each others, sometimes > we might only want to update the multiplier/diviser on a specific clock > (cf clockB in drawing below) and call "clock_propagate(clockA)" to > update the childs period according to the potential new multiplier/diviser values. > > +--------+ +--------+ +--------+ > | clockA | --> | clockB | --> | clockC | > +--------+ +--------+ +--------+ > > The actual code would not allow that because, since we cannot call > "clock_propagate" directly on a child, it would exit on the > first child has the period has not changed for clockB, only clockC is Typo "as the period has not changed"? Why can't you call clock_propagate() on a child? > impacted in our example. > > Signed-off-by: Raphael Poggi <raphael.poggi@lynxleap.co.uk> > --- > hw/core/clock.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/hw/core/clock.c b/hw/core/clock.c > index a19c7db7df..85421f8b55 100644 > --- a/hw/core/clock.c > +++ b/hw/core/clock.c > @@ -101,8 +101,9 @@ static void clock_propagate_period(Clock *clk, bool call_callbacks) > if (call_callbacks) { > clock_call_callback(child, ClockUpdate); > } > - clock_propagate_period(child, call_callbacks); > } > + > + clock_propagate_period(child, call_callbacks); > } > } >
Hi Philippe, Le jeu. 18 avr. 2024 à 20:43, Philippe Mathieu-Daudé <philmd@linaro.org> a écrit : > > Hi Raphael, > > On 18/4/24 21:16, Raphael Poggi wrote: > > When dealing with few clocks depending with each others, sometimes > > we might only want to update the multiplier/diviser on a specific clock > > (cf clockB in drawing below) and call "clock_propagate(clockA)" to > > update the childs period according to the potential new multiplier/diviser values. > > > > +--------+ +--------+ +--------+ > > | clockA | --> | clockB | --> | clockC | > > +--------+ +--------+ +--------+ > > > > The actual code would not allow that because, since we cannot call > > "clock_propagate" directly on a child, it would exit on the > > first child has the period has not changed for clockB, only clockC is > > Typo "as the period has not changed"? That's a typo indeed, thanks! > > Why can't you call clock_propagate() on a child? There is an assert "assert(clk->source == NULL);" in clock_propagate(). If I am not wrong, clk->source is set when the clock has a parent. > > > impacted in our example. > > > > Signed-off-by: Raphael Poggi <raphael.poggi@lynxleap.co.uk> > > --- > > hw/core/clock.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/hw/core/clock.c b/hw/core/clock.c > > index a19c7db7df..85421f8b55 100644 > > --- a/hw/core/clock.c > > +++ b/hw/core/clock.c > > @@ -101,8 +101,9 @@ static void clock_propagate_period(Clock *clk, bool call_callbacks) > > if (call_callbacks) { > > clock_call_callback(child, ClockUpdate); > > } > > - clock_propagate_period(child, call_callbacks); > > } > > + > > + clock_propagate_period(child, call_callbacks); > > } > > } > > >
On Thu, 18 Apr 2024 at 21:39, Raphael Poggi <raphael.poggi@lynxleap.co.uk> wrote: > > Hi Philippe, > > Le jeu. 18 avr. 2024 à 20:43, Philippe Mathieu-Daudé > <philmd@linaro.org> a écrit : > > > > Hi Raphael, > > > > On 18/4/24 21:16, Raphael Poggi wrote: > > > When dealing with few clocks depending with each others, sometimes > > > we might only want to update the multiplier/diviser on a specific clock > > > (cf clockB in drawing below) and call "clock_propagate(clockA)" to > > > update the childs period according to the potential new multiplier/diviser values. > > > > > > +--------+ +--------+ +--------+ > > > | clockA | --> | clockB | --> | clockC | > > > +--------+ +--------+ +--------+ > > > > > > The actual code would not allow that because, since we cannot call > > > "clock_propagate" directly on a child, it would exit on the > > > first child has the period has not changed for clockB, only clockC is > > > > Typo "as the period has not changed"? > > That's a typo indeed, thanks! > > > > > Why can't you call clock_propagate() on a child? > > There is an assert "assert(clk->source == NULL);" in clock_propagate(). > If I am not wrong, clk->source is set when the clock has a parent. I think that assertion is probably there because we didn't originally have the idea of a clock having a multiplier/divider setting. So the idea was that calling clock_propagate() on a clock with a parent would always be wrong, because the only reason for its period to change would be if the parent had changed, and if the parent changes then clock_propagate() should be called on the parent. We added mul/div later, and we (I) didn't think through all the consequences. If you change the mul/div settings on clockB in this example then you need to call clock_propagate() on it, so we should remove that assert(). Then when you change the mul/div on clockB you can directly clock_propagate(clockB), and I don't think you need this patch at that point. thanks -- PMM
Hi Peter, Le ven. 19 avr. 2024 à 16:08, Peter Maydell <peter.maydell@linaro.org> a écrit : > > On Thu, 18 Apr 2024 at 21:39, Raphael Poggi > <raphael.poggi@lynxleap.co.uk> wrote: > > > > Hi Philippe, > > > > Le jeu. 18 avr. 2024 à 20:43, Philippe Mathieu-Daudé > > <philmd@linaro.org> a écrit : > > > > > > Hi Raphael, > > > > > > On 18/4/24 21:16, Raphael Poggi wrote: > > > > When dealing with few clocks depending with each others, sometimes > > > > we might only want to update the multiplier/diviser on a specific clock > > > > (cf clockB in drawing below) and call "clock_propagate(clockA)" to > > > > update the childs period according to the potential new multiplier/diviser values. > > > > > > > > +--------+ +--------+ +--------+ > > > > | clockA | --> | clockB | --> | clockC | > > > > +--------+ +--------+ +--------+ > > > > > > > > The actual code would not allow that because, since we cannot call > > > > "clock_propagate" directly on a child, it would exit on the > > > > first child has the period has not changed for clockB, only clockC is > > > > > > Typo "as the period has not changed"? > > > > That's a typo indeed, thanks! > > > > > > > > Why can't you call clock_propagate() on a child? > > > > There is an assert "assert(clk->source == NULL);" in clock_propagate(). > > If I am not wrong, clk->source is set when the clock has a parent. > > I think that assertion is probably there because we didn't > originally have the idea of a clock having a multiplier/divider > setting. So the idea was that calling clock_propagate() on a > clock with a parent would always be wrong, because the only > reason for its period to change would be if the parent had > changed, and if the parent changes then clock_propagate() > should be called on the parent. > > We added mul/div later, and we (I) didn't think through all > the consequences. If you change the mul/div settings on > clockB in this example then you need to call clock_propagate() > on it, so we should remove that assert(). Then when you change > the mul/div on clockB you can directly clock_propagate(clockB), > and I don't think you need this patch at that point. Alright, that makes sense, is that OK if I send a patch removing the assert ? Thanks, > > thanks > -- PMM
On Fri, 19 Apr 2024 at 17:09, Raphael Poggi <raphael.poggi@lynxleap.co.uk> wrote: > > Hi Peter, > > Le ven. 19 avr. 2024 à 16:08, Peter Maydell <peter.maydell@linaro.org> a écrit : > > > > On Thu, 18 Apr 2024 at 21:39, Raphael Poggi > > <raphael.poggi@lynxleap.co.uk> wrote: > > > There is an assert "assert(clk->source == NULL);" in clock_propagate(). > > > If I am not wrong, clk->source is set when the clock has a parent. > > > > I think that assertion is probably there because we didn't > > originally have the idea of a clock having a multiplier/divider > > setting. So the idea was that calling clock_propagate() on a > > clock with a parent would always be wrong, because the only > > reason for its period to change would be if the parent had > > changed, and if the parent changes then clock_propagate() > > should be called on the parent. > > > > We added mul/div later, and we (I) didn't think through all > > the consequences. If you change the mul/div settings on > > clockB in this example then you need to call clock_propagate() > > on it, so we should remove that assert(). Then when you change > > the mul/div on clockB you can directly clock_propagate(clockB), > > and I don't think you need this patch at that point. > > Alright, that makes sense, is that OK if I send a patch removing the assert ? Yes, please do. -- PMM
On 19/4/24 18:08, Raphael Poggi wrote: > Hi Peter, > > Le ven. 19 avr. 2024 à 16:08, Peter Maydell <peter.maydell@linaro.org> a écrit : >> >> On Thu, 18 Apr 2024 at 21:39, Raphael Poggi >> <raphael.poggi@lynxleap.co.uk> wrote: >>> >>> Hi Philippe, >>> >>> Le jeu. 18 avr. 2024 à 20:43, Philippe Mathieu-Daudé >>> <philmd@linaro.org> a écrit : >>>> >>>> Hi Raphael, >>>> >>>> On 18/4/24 21:16, Raphael Poggi wrote: >>>>> When dealing with few clocks depending with each others, sometimes >>>>> we might only want to update the multiplier/diviser on a specific clock >>>>> (cf clockB in drawing below) and call "clock_propagate(clockA)" to >>>>> update the childs period according to the potential new multiplier/diviser values. >>>>> >>>>> +--------+ +--------+ +--------+ >>>>> | clockA | --> | clockB | --> | clockC | >>>>> +--------+ +--------+ +--------+ >>>>> >>>>> The actual code would not allow that because, since we cannot call >>>>> "clock_propagate" directly on a child, it would exit on the >>>>> first child has the period has not changed for clockB, only clockC is >>>> >>>> Typo "as the period has not changed"? >>> >>> That's a typo indeed, thanks! >>> >>>> >>>> Why can't you call clock_propagate() on a child? >>> >>> There is an assert "assert(clk->source == NULL);" in clock_propagate(). >>> If I am not wrong, clk->source is set when the clock has a parent. >> >> I think that assertion is probably there because we didn't >> originally have the idea of a clock having a multiplier/divider >> setting. So the idea was that calling clock_propagate() on a >> clock with a parent would always be wrong, because the only >> reason for its period to change would be if the parent had >> changed, and if the parent changes then clock_propagate() >> should be called on the parent. >> >> We added mul/div later, and we (I) didn't think through all >> the consequences. If you change the mul/div settings on >> clockB in this example then you need to call clock_propagate() >> on it, so we should remove that assert(). Then when you change >> the mul/div on clockB you can directly clock_propagate(clockB), >> and I don't think you need this patch at that point. > > Alright, that makes sense, is that OK if I send a patch removing the assert ? Sure, that is welcomed :) Regards, Phil.
diff --git a/hw/core/clock.c b/hw/core/clock.c index a19c7db7df..85421f8b55 100644 --- a/hw/core/clock.c +++ b/hw/core/clock.c @@ -101,8 +101,9 @@ static void clock_propagate_period(Clock *clk, bool call_callbacks) if (call_callbacks) { clock_call_callback(child, ClockUpdate); } - clock_propagate_period(child, call_callbacks); } + + clock_propagate_period(child, call_callbacks); } }
When dealing with few clocks depending with each others, sometimes we might only want to update the multiplier/diviser on a specific clock (cf clockB in drawing below) and call "clock_propagate(clockA)" to update the childs period according to the potential new multiplier/diviser values. +--------+ +--------+ +--------+ | clockA | --> | clockB | --> | clockC | +--------+ +--------+ +--------+ The actual code would not allow that because, since we cannot call "clock_propagate" directly on a child, it would exit on the first child has the period has not changed for clockB, only clockC is impacted in our example. Signed-off-by: Raphael Poggi <raphael.poggi@lynxleap.co.uk> --- hw/core/clock.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)