Message ID | FRYP281MB0158389968A2A1C231F3A585ABFB0@FRYP281MB0158.DEUP281.PROD.OUTLOOK.COM |
---|---|
State | New |
Headers | show |
Series | Initialize Zynq7000 UART clocks on reset | expand |
Cc'ing qemu-arm list too. On 11/24/20 5:52 PM, Michael Peter wrote: > Pass an additional argument to zynq_slcr_compute_clocks that indicates > whether an reset-exit condition > applies. If called from zynq_slcr_reset_exit, external clocks are > assumed to be active, even if the > device state indicates a reset state. > > Signed-off-by: Michael Peter <michael.peter@hensoldt-cyber.de> > --- > hw/misc/zynq_slcr.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c > index a2b28019e3..073122b934 100644 > --- a/hw/misc/zynq_slcr.c > +++ b/hw/misc/zynq_slcr.c > @@ -269,12 +269,12 @@ static uint64_t zynq_slcr_compute_clock(const > uint64_t periods[], > * But do not propagate them further. Connected clocks > * will not receive any updates (See zynq_slcr_compute_clocks()) > */ > -static void zynq_slcr_compute_clocks(ZynqSLCRState *s) > +static void zynq_slcr_compute_clocks(ZynqSLCRState *s, bool ignore_reset) > { > uint64_t ps_clk = clock_get(s->ps_clk); > > /* consider outputs clocks are disabled while in reset */ > - if (device_is_in_reset(DEVICE(s))) { > + if (!ignore_reset && device_is_in_reset(DEVICE(s))) { > ps_clk = 0; > } > > @@ -305,7 +305,7 @@ static void zynq_slcr_propagate_clocks(ZynqSLCRState *s) > static void zynq_slcr_ps_clk_callback(void *opaque) > { > ZynqSLCRState *s = (ZynqSLCRState *) opaque; > - zynq_slcr_compute_clocks(s); > + zynq_slcr_compute_clocks(s, false); > zynq_slcr_propagate_clocks(s); > } > > @@ -410,7 +410,7 @@ static void zynq_slcr_reset_hold(Object *obj) > ZynqSLCRState *s = ZYNQ_SLCR(obj); > > /* will disable all output clocks */ > - zynq_slcr_compute_clocks(s); > + zynq_slcr_compute_clocks(s, false); > zynq_slcr_propagate_clocks(s); > } > > @@ -419,7 +419,7 @@ static void zynq_slcr_reset_exit(Object *obj) > ZynqSLCRState *s = ZYNQ_SLCR(obj); > > /* will compute output clocks according to ps_clk and registers */ > - zynq_slcr_compute_clocks(s); > + zynq_slcr_compute_clocks(s, true); > zynq_slcr_propagate_clocks(s); > } > > @@ -558,7 +558,7 @@ static void zynq_slcr_write(void *opaque, hwaddr offset, > case R_ARM_PLL_CTRL: > case R_DDR_PLL_CTRL: > case R_UART_CLK_CTRL: > - zynq_slcr_compute_clocks(s); > + zynq_slcr_compute_clocks(s, false); > zynq_slcr_propagate_clocks(s); > break; > } > -- > 2.17.1 > > >
Alistair/Edgar/Damien -- could I get a review from one of you for this Xilinx clock-gen related patch, please? thanks -- PMM On Tue, 24 Nov 2020 at 18:54, Michael Peter <michael.peter@hensoldt-cyber.de> wrote: > > Pass an additional argument to zynq_slcr_compute_clocks that indicates whether an reset-exit condition > applies. If called from zynq_slcr_reset_exit, external clocks are assumed to be active, even if the > device state indicates a reset state. > > Signed-off-by: Michael Peter <michael.peter@hensoldt-cyber.de> > --- > hw/misc/zynq_slcr.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c > index a2b28019e3..073122b934 100644 > --- a/hw/misc/zynq_slcr.c > +++ b/hw/misc/zynq_slcr.c > @@ -269,12 +269,12 @@ static uint64_t zynq_slcr_compute_clock(const uint64_t periods[], > * But do not propagate them further. Connected clocks > * will not receive any updates (See zynq_slcr_compute_clocks()) > */ > -static void zynq_slcr_compute_clocks(ZynqSLCRState *s) > +static void zynq_slcr_compute_clocks(ZynqSLCRState *s, bool ignore_reset) > { > uint64_t ps_clk = clock_get(s->ps_clk); > > /* consider outputs clocks are disabled while in reset */ > - if (device_is_in_reset(DEVICE(s))) { > + if (!ignore_reset && device_is_in_reset(DEVICE(s))) { > ps_clk = 0; > } > > @@ -305,7 +305,7 @@ static void zynq_slcr_propagate_clocks(ZynqSLCRState *s) > static void zynq_slcr_ps_clk_callback(void *opaque) > { > ZynqSLCRState *s = (ZynqSLCRState *) opaque; > - zynq_slcr_compute_clocks(s); > + zynq_slcr_compute_clocks(s, false); > zynq_slcr_propagate_clocks(s); > } > > @@ -410,7 +410,7 @@ static void zynq_slcr_reset_hold(Object *obj) > ZynqSLCRState *s = ZYNQ_SLCR(obj); > > /* will disable all output clocks */ > - zynq_slcr_compute_clocks(s); > + zynq_slcr_compute_clocks(s, false); > zynq_slcr_propagate_clocks(s); > } > > @@ -419,7 +419,7 @@ static void zynq_slcr_reset_exit(Object *obj) > ZynqSLCRState *s = ZYNQ_SLCR(obj); > > /* will compute output clocks according to ps_clk and registers */ > - zynq_slcr_compute_clocks(s); > + zynq_slcr_compute_clocks(s, true); > zynq_slcr_propagate_clocks(s); > } > > @@ -558,7 +558,7 @@ static void zynq_slcr_write(void *opaque, hwaddr offset, > case R_ARM_PLL_CTRL: > case R_DDR_PLL_CTRL: > case R_UART_CLK_CTRL: > - zynq_slcr_compute_clocks(s); > + zynq_slcr_compute_clocks(s, false); > zynq_slcr_propagate_clocks(s); > break; > } > -- > 2.17.1
This is ok but I'm afraid we may end up doing this kind of thing in a lot of devices. So maybe we should consider changing the behavior of device_is_in_reset() so that it returns false in the reset-exit case. What do you think ? (I've a patch for this, which make this one useless) But this patch does not harm so, anyway: Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> On 1/7/21 9:00 PM, Peter Maydell wrote: > Alistair/Edgar/Damien -- could I get a review from one of you > for this Xilinx clock-gen related patch, please? > > thanks > -- PMM > > On Tue, 24 Nov 2020 at 18:54, Michael Peter > <michael.peter@hensoldt-cyber.de> wrote: >> >> Pass an additional argument to zynq_slcr_compute_clocks that indicates whether an reset-exit condition >> applies. If called from zynq_slcr_reset_exit, external clocks are assumed to be active, even if the >> device state indicates a reset state. >> >> Signed-off-by: Michael Peter <michael.peter@hensoldt-cyber.de> >> --- >> hw/misc/zynq_slcr.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c >> index a2b28019e3..073122b934 100644 >> --- a/hw/misc/zynq_slcr.c >> +++ b/hw/misc/zynq_slcr.c >> @@ -269,12 +269,12 @@ static uint64_t zynq_slcr_compute_clock(const uint64_t periods[], >> * But do not propagate them further. Connected clocks >> * will not receive any updates (See zynq_slcr_compute_clocks()) >> */ >> -static void zynq_slcr_compute_clocks(ZynqSLCRState *s) >> +static void zynq_slcr_compute_clocks(ZynqSLCRState *s, bool ignore_reset) >> { >> uint64_t ps_clk = clock_get(s->ps_clk); >> >> /* consider outputs clocks are disabled while in reset */ >> - if (device_is_in_reset(DEVICE(s))) { >> + if (!ignore_reset && device_is_in_reset(DEVICE(s))) { >> ps_clk = 0; >> } >> >> @@ -305,7 +305,7 @@ static void zynq_slcr_propagate_clocks(ZynqSLCRState *s) >> static void zynq_slcr_ps_clk_callback(void *opaque) >> { >> ZynqSLCRState *s = (ZynqSLCRState *) opaque; >> - zynq_slcr_compute_clocks(s); >> + zynq_slcr_compute_clocks(s, false); >> zynq_slcr_propagate_clocks(s); >> } >> >> @@ -410,7 +410,7 @@ static void zynq_slcr_reset_hold(Object *obj) >> ZynqSLCRState *s = ZYNQ_SLCR(obj); >> >> /* will disable all output clocks */ >> - zynq_slcr_compute_clocks(s); >> + zynq_slcr_compute_clocks(s, false); >> zynq_slcr_propagate_clocks(s); >> } >> >> @@ -419,7 +419,7 @@ static void zynq_slcr_reset_exit(Object *obj) >> ZynqSLCRState *s = ZYNQ_SLCR(obj); >> >> /* will compute output clocks according to ps_clk and registers */ >> - zynq_slcr_compute_clocks(s); >> + zynq_slcr_compute_clocks(s, true); >> zynq_slcr_propagate_clocks(s); >> } >> >> @@ -558,7 +558,7 @@ static void zynq_slcr_write(void *opaque, hwaddr offset, >> case R_ARM_PLL_CTRL: >> case R_DDR_PLL_CTRL: >> case R_UART_CLK_CTRL: >> - zynq_slcr_compute_clocks(s); >> + zynq_slcr_compute_clocks(s, false); >> zynq_slcr_propagate_clocks(s); >> break; >> } >> -- >> 2.17.1 >
On Wed, 13 Jan 2021, 11:19 Damien Hedde, <damien.hedde@greensocs.com> wrote: > > This is ok but I'm afraid we may end up doing this kind of thing in a > lot of devices. So maybe we should consider changing the behavior of > device_is_in_reset() so that it returns false in the reset-exit case. > What do you think ? (I've a patch for this, which make this one useless) > Thanks Damien, IMO, a central fix for this would be better, I agree with you. Thanks, Edgar > But this patch does not harm so, anyway: > Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> > > On 1/7/21 9:00 PM, Peter Maydell wrote: > > Alistair/Edgar/Damien -- could I get a review from one of you > > for this Xilinx clock-gen related patch, please? > > > > thanks > > -- PMM > > > > On Tue, 24 Nov 2020 at 18:54, Michael Peter > > <michael.peter@hensoldt-cyber.de> wrote: > >> > >> Pass an additional argument to zynq_slcr_compute_clocks that indicates > whether an reset-exit condition > >> applies. If called from zynq_slcr_reset_exit, external clocks are > assumed to be active, even if the > >> device state indicates a reset state. > >> > >> Signed-off-by: Michael Peter <michael.peter@hensoldt-cyber.de> > >> --- > >> hw/misc/zynq_slcr.c | 12 ++++++------ > >> 1 file changed, 6 insertions(+), 6 deletions(-) > >> > >> diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c > >> index a2b28019e3..073122b934 100644 > >> --- a/hw/misc/zynq_slcr.c > >> +++ b/hw/misc/zynq_slcr.c > >> @@ -269,12 +269,12 @@ static uint64_t zynq_slcr_compute_clock(const > uint64_t periods[], > >> * But do not propagate them further. Connected clocks > >> * will not receive any updates (See zynq_slcr_compute_clocks()) > >> */ > >> -static void zynq_slcr_compute_clocks(ZynqSLCRState *s) > >> +static void zynq_slcr_compute_clocks(ZynqSLCRState *s, bool > ignore_reset) > >> { > >> uint64_t ps_clk = clock_get(s->ps_clk); > >> > >> /* consider outputs clocks are disabled while in reset */ > >> - if (device_is_in_reset(DEVICE(s))) { > >> + if (!ignore_reset && device_is_in_reset(DEVICE(s))) { > >> ps_clk = 0; > >> } > >> > >> @@ -305,7 +305,7 @@ static void > zynq_slcr_propagate_clocks(ZynqSLCRState *s) > >> static void zynq_slcr_ps_clk_callback(void *opaque) > >> { > >> ZynqSLCRState *s = (ZynqSLCRState *) opaque; > >> - zynq_slcr_compute_clocks(s); > >> + zynq_slcr_compute_clocks(s, false); > >> zynq_slcr_propagate_clocks(s); > >> } > >> > >> @@ -410,7 +410,7 @@ static void zynq_slcr_reset_hold(Object *obj) > >> ZynqSLCRState *s = ZYNQ_SLCR(obj); > >> > >> /* will disable all output clocks */ > >> - zynq_slcr_compute_clocks(s); > >> + zynq_slcr_compute_clocks(s, false); > >> zynq_slcr_propagate_clocks(s); > >> } > >> > >> @@ -419,7 +419,7 @@ static void zynq_slcr_reset_exit(Object *obj) > >> ZynqSLCRState *s = ZYNQ_SLCR(obj); > >> > >> /* will compute output clocks according to ps_clk and registers */ > >> - zynq_slcr_compute_clocks(s); > >> + zynq_slcr_compute_clocks(s, true); > >> zynq_slcr_propagate_clocks(s); > >> } > >> > >> @@ -558,7 +558,7 @@ static void zynq_slcr_write(void *opaque, hwaddr > offset, > >> case R_ARM_PLL_CTRL: > >> case R_DDR_PLL_CTRL: > >> case R_UART_CLK_CTRL: > >> - zynq_slcr_compute_clocks(s); > >> + zynq_slcr_compute_clocks(s, false); > >> zynq_slcr_propagate_clocks(s); > >> break; > >> } > >> -- > >> 2.17.1 > > >
Damien, On Wed, 2021-01-13 at 11:19 +0100, Damien Hedde wrote: > This is ok but I'm afraid we may end up doing this kind of thing in a > lot of devices. So maybe we should consider changing the behavior of > device_is_in_reset() so that it returns false in the reset-exit case. > What do you think ? (I've a patch for this, which make this one > useless) I concur that a general solution would be preferable. My patch was only meant to highlight the issue by providing an ad-hoc solution. Thank you for picking up the topic. Best regards, Michael > > But this patch does not harm so, anyway: > Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> > > On 1/7/21 9:00 PM, Peter Maydell wrote: > > Alistair/Edgar/Damien -- could I get a review from one of you > > for this Xilinx clock-gen related patch, please? > > > > thanks > > -- PMM > > > > On Tue, 24 Nov 2020 at 18:54, Michael Peter > > <michael.peter@hensoldt-cyber.de> wrote: > > > > > > Pass an additional argument to zynq_slcr_compute_clocks that > > > indicates whether an reset-exit condition > > > applies. If called from zynq_slcr_reset_exit, external clocks are > > > assumed to be active, even if the > > > device state indicates a reset state. > > > > > > Signed-off-by: Michael Peter <michael.peter@hensoldt-cyber.de> > > > --- > > > hw/misc/zynq_slcr.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c > > > index a2b28019e3..073122b934 100644 > > > --- a/hw/misc/zynq_slcr.c > > > +++ b/hw/misc/zynq_slcr.c > > > @@ -269,12 +269,12 @@ static uint64_t > > > zynq_slcr_compute_clock(const uint64_t periods[], > > > * But do not propagate them further. Connected clocks > > > * will not receive any updates (See zynq_slcr_compute_clocks()) > > > */ > > > -static void zynq_slcr_compute_clocks(ZynqSLCRState *s) > > > +static void zynq_slcr_compute_clocks(ZynqSLCRState *s, bool > > > ignore_reset) > > > { > > > uint64_t ps_clk = clock_get(s->ps_clk); > > > > > > /* consider outputs clocks are disabled while in reset */ > > > - if (device_is_in_reset(DEVICE(s))) { > > > + if (!ignore_reset && device_is_in_reset(DEVICE(s))) { > > > ps_clk = 0; > > > } > > > > > > @@ -305,7 +305,7 @@ static void > > > zynq_slcr_propagate_clocks(ZynqSLCRState *s) > > > static void zynq_slcr_ps_clk_callback(void *opaque) > > > { > > > ZynqSLCRState *s = (ZynqSLCRState *) opaque; > > > - zynq_slcr_compute_clocks(s); > > > + zynq_slcr_compute_clocks(s, false); > > > zynq_slcr_propagate_clocks(s); > > > } > > > > > > @@ -410,7 +410,7 @@ static void zynq_slcr_reset_hold(Object *obj) > > > ZynqSLCRState *s = ZYNQ_SLCR(obj); > > > > > > /* will disable all output clocks */ > > > - zynq_slcr_compute_clocks(s); > > > + zynq_slcr_compute_clocks(s, false); > > > zynq_slcr_propagate_clocks(s); > > > } > > > > > > @@ -419,7 +419,7 @@ static void zynq_slcr_reset_exit(Object *obj) > > > ZynqSLCRState *s = ZYNQ_SLCR(obj); > > > > > > /* will compute output clocks according to ps_clk and > > > registers */ > > > - zynq_slcr_compute_clocks(s); > > > + zynq_slcr_compute_clocks(s, true); > > > zynq_slcr_propagate_clocks(s); > > > } > > > > > > @@ -558,7 +558,7 @@ static void zynq_slcr_write(void *opaque, > > > hwaddr offset, > > > case R_ARM_PLL_CTRL: > > > case R_DDR_PLL_CTRL: > > > case R_UART_CLK_CTRL: > > > - zynq_slcr_compute_clocks(s); > > > + zynq_slcr_compute_clocks(s, false); > > > zynq_slcr_propagate_clocks(s); > > > break; > > > } > > > -- > > > 2.17.1
diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c index a2b28019e3..073122b934 100644 --- a/hw/misc/zynq_slcr.c +++ b/hw/misc/zynq_slcr.c @@ -269,12 +269,12 @@ static uint64_t zynq_slcr_compute_clock(const uint64_t periods[], * But do not propagate them further. Connected clocks * will not receive any updates (See zynq_slcr_compute_clocks()) */ -static void zynq_slcr_compute_clocks(ZynqSLCRState *s) +static void zynq_slcr_compute_clocks(ZynqSLCRState *s, bool ignore_reset) { uint64_t ps_clk = clock_get(s->ps_clk); /* consider outputs clocks are disabled while in reset */ - if (device_is_in_reset(DEVICE(s))) { + if (!ignore_reset && device_is_in_reset(DEVICE(s))) { ps_clk = 0; } @@ -305,7 +305,7 @@ static void zynq_slcr_propagate_clocks(ZynqSLCRState *s) static void zynq_slcr_ps_clk_callback(void *opaque) { ZynqSLCRState *s = (ZynqSLCRState *) opaque; - zynq_slcr_compute_clocks(s); + zynq_slcr_compute_clocks(s, false); zynq_slcr_propagate_clocks(s); } @@ -410,7 +410,7 @@ static void zynq_slcr_reset_hold(Object *obj) ZynqSLCRState *s = ZYNQ_SLCR(obj); /* will disable all output clocks */ - zynq_slcr_compute_clocks(s); + zynq_slcr_compute_clocks(s, false); zynq_slcr_propagate_clocks(s); } @@ -419,7 +419,7 @@ static void zynq_slcr_reset_exit(Object *obj) ZynqSLCRState *s = ZYNQ_SLCR(obj); /* will compute output clocks according to ps_clk and registers */ - zynq_slcr_compute_clocks(s); + zynq_slcr_compute_clocks(s, true); zynq_slcr_propagate_clocks(s); } @@ -558,7 +558,7 @@ static void zynq_slcr_write(void *opaque, hwaddr offset, case R_ARM_PLL_CTRL: case R_DDR_PLL_CTRL: case R_UART_CLK_CTRL: - zynq_slcr_compute_clocks(s); + zynq_slcr_compute_clocks(s, false); zynq_slcr_propagate_clocks(s); break; }
Pass an additional argument to zynq_slcr_compute_clocks that indicates whether an reset-exit condition applies. If called from zynq_slcr_reset_exit, external clocks are assumed to be active, even if the device state indicates a reset state. Signed-off-by: Michael Peter <michael.peter@hensoldt-cyber.de> --- hw/misc/zynq_slcr.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) -- 2.17.1