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

[Edgar E. Iglesias]

On Mon, Aug 23, 2021 at 02:42:03PM +1000, Alistair Francis wrote:
> 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>

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.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
> >
> >