1. Patchew
  2. QEMU
  3. View series

[PATCH] Initialize Zynq7000 UART clocks on reset

Michael Peter posted 1 patch 3 years, 5 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/FRYP281MB0158389968A2A1C231F3A585ABFB0@FRYP281MB0158.DEUP281.PROD.OUTLOOK.COM
Maintainers: Alistair Francis <alistair@alistair23.me>, Peter Maydell <peter.maydell@linaro.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
hw/misc/zynq_slcr.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
[PATCH] Initialize Zynq7000 UART clocks on reset
Posted by Michael Peter 3 years, 5 months ago 
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
Re: [PATCH] Initialize Zynq7000 UART clocks on reset
Posted by Philippe Mathieu-Daudé 3 years, 5 months ago 
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
> 
> 
>
Re: [PATCH] Initialize Zynq7000 UART clocks on reset
Posted by Peter Maydell 3 years, 3 months ago 
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
Re: [PATCH] Initialize Zynq7000 UART clocks on reset
Posted by Damien Hedde 3 years, 3 months ago 
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
>
Re: [PATCH] Initialize Zynq7000 UART clocks on reset
Posted by Edgar E. Iglesias 3 years, 3 months ago 
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
> >
>
Re: [PATCH] Initialize Zynq7000 UART clocks on reset
Posted by Michael Peter 3 years, 3 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.