Re: [PATCH 1/3] hw/misc: zynq_slcr: Correctly compute output clocks in the reset exit phase

[Alistair Francis]

On Mon, Aug 23, 2021 at 12:09 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> As of today, when booting upstream U-Boot for Xilinx Zynq, the UART
> does not receive anything. Debugging shows that the UART input clock
> frequency is zero which prevents the UART from receiving anything as
> per the logic in uart_receive().
>
> From zynq_slcr_reset_exit() comment, it intends to compute output
> clocks according to ps_clk and registers. zynq_slcr_compute_clocks()
> is called to accomplish the task, inside which device_is_in_reset()
> is called to actually make the attempt in vain.
>
> Rework reset_hold() and reset_exit() so that in the reset exit phase,
> the logic can really compute output clocks in reset_exit().
>
> With this change, upstream U-Boot boots properly again with:
>
> $ qemu-system-arm -M xilinx-zynq-a9 -m 1G -display none -serial null -serial 
> stdio \
>     -device loader,file=u-boot-dtb.bin,addr=0x4000000,cpu-num=0
>
> Fixes: 38867cb7ec90 ("hw/misc/zynq_slcr: add clock generation for uarts")
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>
>  hw/misc/zynq_slcr.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
> index 5086e6b7ed..8b70285961 100644
> --- a/hw/misc/zynq_slcr.c
> +++ b/hw/misc/zynq_slcr.c
> @@ -269,6 +269,21 @@ static uint64_t zynq_slcr_compute_clock(const uint64_t 
> periods[],
>      zynq_slcr_compute_clock((plls), (state)->regs[reg], \
>                              reg ## _ ## enable_field ## _SHIFT)
>
> +static void zynq_slcr_compute_clocks_internal(ZynqSLCRState *s, uint64_t 
> ps_clk)
> +{
> +    uint64_t io_pll = zynq_slcr_compute_pll(ps_clk, s->regs[R_IO_PLL_CTRL]);
> +    uint64_t arm_pll = zynq_slcr_compute_pll(ps_clk, 
> s->regs[R_ARM_PLL_CTRL]);
> +    uint64_t ddr_pll = zynq_slcr_compute_pll(ps_clk, 
> s->regs[R_DDR_PLL_CTRL]);
> +
> +    uint64_t uart_mux[4] = {io_pll, io_pll, arm_pll, ddr_pll};
> +
> +    /* compute uartX reference clocks */
> +    clock_set(s->uart0_ref_clk,
> +              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT0));
> +    clock_set(s->uart1_ref_clk,
> +              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT1));
> +}
> +
>  /**
>   * Compute and set the ouputs clocks periods.
>   * But do not propagate them further. Connected clocks
> @@ -283,17 +298,7 @@ static void zynq_slcr_compute_clocks(ZynqSLCRState *s)
>          ps_clk = 0;
>      }
>
> -    uint64_t io_pll = zynq_slcr_compute_pll(ps_clk, s->regs[R_IO_PLL_CTRL]);
> -    uint64_t arm_pll = zynq_slcr_compute_pll(ps_clk, 
> s->regs[R_ARM_PLL_CTRL]);
> -    uint64_t ddr_pll = zynq_slcr_compute_pll(ps_clk, 
> s->regs[R_DDR_PLL_CTRL]);
> -
> -    uint64_t uart_mux[4] = {io_pll, io_pll, arm_pll, ddr_pll};
> -
> -    /* compute uartX reference clocks */
> -    clock_set(s->uart0_ref_clk,
> -              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT0));
> -    clock_set(s->uart1_ref_clk,
> -              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT1));
> +    zynq_slcr_compute_clocks_internal(s, ps_clk);
>  }
>
>  /**
> @@ -416,7 +421,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_internal(s, 0);
>      zynq_slcr_propagate_clocks(s);
>  }
>
> @@ -425,7 +430,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_internal(s, clock_get(s->ps_clk));
>      zynq_slcr_propagate_clocks(s);
>  }
>
> --
> 2.25.1
>
>