Re: [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts
On 12/2/19 4:20 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde wrote:
>>
>> Switch the slcr to multi-phase reset and add some clocks:
>> + the main input clock (ps_clk)
>> + the reference clock outputs for each uart (uart0 & 1)
>>
>> The clock frequencies are computed using the internal pll & uart
>> configuration
>> registers and the ps_clk frequency.
>>
>> Signed-off-by: Damien Hedde
>
> Review of this and the following two patches by some Xilinx
> person would be nice. I've just looked them over for general
> issues, and haven't checked against the hardware specs.
>
>> ---
>
>
>> +/*
>> + * return the output frequency of a clock given:
>> + * + the frequencies in an array corresponding to mux's indexes
>> + * + the register xxx_CLK_CTRL value
>> + * + enable bit index in ctrl register
>> + *
>> + * This function make the assumption that ctrl_reg value is organized as
>> follow:
>
> "makes"; "that the"; "follows"
>
>> + * + bits[13:8] clock divisor
>> + * + bits[5:4] clock mux selector (index in array)
>> + * + bits[index] clock enable
>> + */
>> +static uint64_t zynq_slcr_compute_clock(const uint64_t mux[],
>> +uint32_t ctrl_reg,
>> +unsigned index)
>> +{
>> +uint32_t srcsel = extract32(ctrl_reg, 4, 2); /* bits [5:4] */
>> +uint32_t divisor = extract32(ctrl_reg, 8, 6); /* bits [13:8] */
>> +
>> +/* first, check if clock is enabled */
>> +if (((ctrl_reg >> index) & 1u) == 0) {
>> +return 0;
>> +}
>> +
>> +/*
>> + * according to the Zynq technical ref. manual UG585 v1.12.2 in
>> + * "Clocks" chapter, section 25.10.1 page 705" the range of the divisor
>> + * is [1;63].
>
> Is this the range notation the spec doc uses?
The exact terms is:
"The 6-bit divider provides a divide range of 1 to 63"
At the time, I checked also the kernel sources, and this is the behavior
implemented in the driver as well (1 based timer and allowing 0 special
value for bypass). The bypass is undocumented as far as I can tell.
>
>> + * So divide the source while avoiding division-by-zero.
>> + */
>> +return mux[srcsel] / (divisor ? divisor : 1u);
>> +}
>> +
>
>> +static const ClockPortInitArray zynq_slcr_clocks = {
>> +QDEV_CLOCK_IN(ZynqSLCRState, ps_clk, zynq_slcr_ps_clk_callback),
>> +QDEV_CLOCK_OUT(ZynqSLCRState, uart0_ref_clk),
>> +QDEV_CLOCK_OUT(ZynqSLCRState, uart1_ref_clk),
>> +QDEV_CLOCK_END
>> +};
>> +
>> static void zynq_slcr_init(Object *obj)
>> {
>> ZynqSLCRState *s = ZYNQ_SLCR(obj);
>> @@ -425,6 +559,8 @@ static void zynq_slcr_init(Object *obj)
>> memory_region_init_io(&s->iomem, obj, &slcr_ops, s, "slcr",
>>ZYNQ_SLCR_MMIO_SIZE);
>> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>> +
>> +qdev_init_clocks(DEVICE(obj), zynq_slcr_clocks);
>> }
>>
>> static const VMStateDescription vmstate_zynq_slcr = {
>> @@ -440,9 +576,12 @@ static const VMStateDescription vmstate_zynq_slcr = {
>> static void zynq_slcr_class_init(ObjectClass *klass, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(klass);
>> +ResettableClass *rc = RESETTABLE_CLASS(klass);
>>
>> dc->vmsd = &vmstate_zynq_slcr;
>> -dc->reset = zynq_slcr_reset;
>> +rc->phases.init = zynq_slcr_reset_init;
>> +rc->phases.hold = zynq_slcr_reset_hold;
>> +rc->phases.exit = zynq_slcr_reset_exit;
>> }
>
> We're adding an input clock, so doesn't the migration
> state struct need to be updated to migrate it ?
Yes, we can. Although this input clock is really not expected to change.
>
> thanks
> -- PMM
>
Re: [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts
On Wed, 4 Sep 2019 at 13:56, Damien Hedde wrote:
>
> Switch the slcr to multi-phase reset and add some clocks:
> + the main input clock (ps_clk)
> + the reference clock outputs for each uart (uart0 & 1)
>
> The clock frequencies are computed using the internal pll & uart configuration
> registers and the ps_clk frequency.
>
> Signed-off-by: Damien Hedde
Review of this and the following two patches by some Xilinx
person would be nice. I've just looked them over for general
issues, and haven't checked against the hardware specs.
> ---
> +/*
> + * return the output frequency of a clock given:
> + * + the frequencies in an array corresponding to mux's indexes
> + * + the register xxx_CLK_CTRL value
> + * + enable bit index in ctrl register
> + *
> + * This function make the assumption that ctrl_reg value is organized as
> follow:
"makes"; "that the"; "follows"
> + * + bits[13:8] clock divisor
> + * + bits[5:4] clock mux selector (index in array)
> + * + bits[index] clock enable
> + */
> +static uint64_t zynq_slcr_compute_clock(const uint64_t mux[],
> +uint32_t ctrl_reg,
> +unsigned index)
> +{
> +uint32_t srcsel = extract32(ctrl_reg, 4, 2); /* bits [5:4] */
> +uint32_t divisor = extract32(ctrl_reg, 8, 6); /* bits [13:8] */
> +
> +/* first, check if clock is enabled */
> +if (((ctrl_reg >> index) & 1u) == 0) {
> +return 0;
> +}
> +
> +/*
> + * according to the Zynq technical ref. manual UG585 v1.12.2 in
> + * "Clocks" chapter, section 25.10.1 page 705" the range of the divisor
> + * is [1;63].
Is this the range notation the spec doc uses?
> + * So divide the source while avoiding division-by-zero.
> + */
> +return mux[srcsel] / (divisor ? divisor : 1u);
> +}
> +
> +static const ClockPortInitArray zynq_slcr_clocks = {
> +QDEV_CLOCK_IN(ZynqSLCRState, ps_clk, zynq_slcr_ps_clk_callback),
> +QDEV_CLOCK_OUT(ZynqSLCRState, uart0_ref_clk),
> +QDEV_CLOCK_OUT(ZynqSLCRState, uart1_ref_clk),
> +QDEV_CLOCK_END
> +};
> +
> static void zynq_slcr_init(Object *obj)
> {
> ZynqSLCRState *s = ZYNQ_SLCR(obj);
> @@ -425,6 +559,8 @@ static void zynq_slcr_init(Object *obj)
> memory_region_init_io(&s->iomem, obj, &slcr_ops, s, "slcr",
>ZYNQ_SLCR_MMIO_SIZE);
> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> +
> +qdev_init_clocks(DEVICE(obj), zynq_slcr_clocks);
> }
>
> static const VMStateDescription vmstate_zynq_slcr = {
> @@ -440,9 +576,12 @@ static const VMStateDescription vmstate_zynq_slcr = {
> static void zynq_slcr_class_init(ObjectClass *klass, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> +ResettableClass *rc = RESETTABLE_CLASS(klass);
>
> dc->vmsd = &vmstate_zynq_slcr;
> -dc->reset = zynq_slcr_reset;
> +rc->phases.init = zynq_slcr_reset_init;
> +rc->phases.hold = zynq_slcr_reset_hold;
> +rc->phases.exit = zynq_slcr_reset_exit;
> }
We're adding an input clock, so doesn't the migration
state struct need to be updated to migrate it ?
thanks
-- PMM
[Qemu-devel] [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts
Switch the slcr to multi-phase reset and add some clocks:
+ the main input clock (ps_clk)
+ the reference clock outputs for each uart (uart0 & 1)
The clock frequencies are computed using the internal pll & uart configuration
registers and the ps_clk frequency.
Signed-off-by: Damien Hedde
---
hw/misc/zynq_slcr.c | 145 +++-
1 file changed, 142 insertions(+), 3 deletions(-)
diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index b9a38272d9..fa2ac608eb 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -22,6 +22,7 @@
#include "qemu/log.h"
#include "qemu/module.h"
#include "hw/registerfields.h"
+#include "hw/qdev-clock.h"
#ifndef ZYNQ_SLCR_ERR_DEBUG
#define ZYNQ_SLCR_ERR_DEBUG 0
@@ -45,6 +46,12 @@ REG32(LOCKSTA, 0x00c)
REG32(ARM_PLL_CTRL, 0x100)
REG32(DDR_PLL_CTRL, 0x104)
REG32(IO_PLL_CTRL, 0x108)
+/* fields for [ARM|DDR|IO]_PLL_CTRL registers */
+FIELD(xxx_PLL_CTRL, PLL_RESET, 0, 1)
+FIELD(xxx_PLL_CTRL, PLL_PWRDWN, 1, 1)
+FIELD(xxx_PLL_CTRL, PLL_BYPASS_QUAL, 3, 1)
+FIELD(xxx_PLL_CTRL, PLL_BYPASS_FORCE, 4, 1)
+FIELD(xxx_PLL_CTRL, PLL_FPDIV, 12, 7)
REG32(PLL_STATUS, 0x10c)
REG32(ARM_PLL_CFG, 0x110)
REG32(DDR_PLL_CFG, 0x114)
@@ -64,6 +71,10 @@ REG32(SMC_CLK_CTRL, 0x148)
REG32(LQSPI_CLK_CTRL, 0x14c)
REG32(SDIO_CLK_CTRL, 0x150)
REG32(UART_CLK_CTRL, 0x154)
+FIELD(UART_CLK_CTRL, CLKACT0, 0, 1)
+FIELD(UART_CLK_CTRL, CLKACT1, 1, 1)
+FIELD(UART_CLK_CTRL, SRCSEL, 4, 2)
+FIELD(UART_CLK_CTRL, DIVISOR, 8, 6)
REG32(SPI_CLK_CTRL, 0x158)
REG32(CAN_CLK_CTRL, 0x15c)
REG32(CAN_MIOCLK_CTRL, 0x160)
@@ -179,11 +190,106 @@ typedef struct ZynqSLCRState {
MemoryRegion iomem;
uint32_t regs[ZYNQ_SLCR_NUM_REGS];
+
+ClockIn *ps_clk;
+ClockOut *uart0_ref_clk;
+ClockOut *uart1_ref_clk;
} ZynqSLCRState;
-static void zynq_slcr_reset(DeviceState *d)
+/*
+ * return the output frequency of ARM/DDR/IO pll
+ * using input frequency and PLL_CTRL register
+ */
+static uint64_t zynq_slcr_compute_pll(uint64_t input, uint32_t ctrl_reg)
+{
+uint32_t mult = ((ctrl_reg & R_xxx_PLL_CTRL_PLL_FPDIV_MASK) >>
+R_xxx_PLL_CTRL_PLL_FPDIV_SHIFT);
+
+/* first, check if pll is bypassed */
+if (ctrl_reg & R_xxx_PLL_CTRL_PLL_BYPASS_FORCE_MASK) {
+return input;
+}
+
+/* is pll disabled ? */
+if (ctrl_reg & (R_xxx_PLL_CTRL_PLL_RESET_MASK |
+R_xxx_PLL_CTRL_PLL_PWRDWN_MASK)) {
+return 0;
+}
+
+return input * mult;
+}
+
+/*
+ * return the output frequency of a clock given:
+ * + the frequencies in an array corresponding to mux's indexes
+ * + the register xxx_CLK_CTRL value
+ * + enable bit index in ctrl register
+ *
+ * This function make the assumption that ctrl_reg value is organized as
follow:
+ * + bits[13:8] clock divisor
+ * + bits[5:4] clock mux selector (index in array)
+ * + bits[index] clock enable
+ */
+static uint64_t zynq_slcr_compute_clock(const uint64_t mux[],
+uint32_t ctrl_reg,
+unsigned index)
+{
+uint32_t srcsel = extract32(ctrl_reg, 4, 2); /* bits [5:4] */
+uint32_t divisor = extract32(ctrl_reg, 8, 6); /* bits [13:8] */
+
+/* first, check if clock is enabled */
+if (((ctrl_reg >> index) & 1u) == 0) {
+return 0;
+}
+
+/*
+ * according to the Zynq technical ref. manual UG585 v1.12.2 in
+ * "Clocks" chapter, section 25.10.1 page 705" the range of the divisor
+ * is [1;63].
+ * So divide the source while avoiding division-by-zero.
+ */
+return mux[srcsel] / (divisor ? divisor : 1u);
+}
+
+/*
+ * macro helper around zynq_slcr_compute_clock to avoid repeating
+ * the register name.
+ */
+#define ZYNQ_COMPUTE_CLOCK(_state, _plls, _reg, _enable_field) \
+zynq_slcr_compute_clock((_plls), (_state)->regs[R_ ## _reg], \
+R_ ## _reg ## _ ## _enable_field ## _SHIFT)
+
+static void zynq_slcr_compute_clocks(ZynqSLCRState *s)
+{
+uint64_t ps_clk = clock_get_frequency(s->ps_clk);
+
+/* consider all output clocks are disabled while in reset */
+if (device_is_resetting(DEVICE(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_frequency(s->uart0_ref_clk,
+ZYNQ_COMPUTE_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT0));
+clock_set_frequency(s->uart1_ref_clk,
+ZYNQ_COMPUTE_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT1));
+}
+
+static void zynq_slcr_ps_clk_callback(void *opaque)
+{
+ZynqSLCRState *s = (ZynqSLCRState *) opaque;
+zynq_slcr_compute_clocks(s);
+}
+
+static void zynq_slcr_reset_init(Object *obj, ResetType type)
{
[Qemu-devel] [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts
From: Damien Hedde
Switch the slcr to multi-phase reset and add some clocks:
+ the main input clock (ps_clk)
+ the reference clock outputs for each uart (uart0 & 1)
The clock frequencies are computed using the internal pll & uart configuration
registers and the ps_clk frequency.
Signed-off-by: Damien Hedde
---
hw/misc/zynq_slcr.c | 145 +++-
1 file changed, 142 insertions(+), 3 deletions(-)
diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index b9a38272d9..fa2ac608eb 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -22,6 +22,7 @@
#include "qemu/log.h"
#include "qemu/module.h"
#include "hw/registerfields.h"
+#include "hw/qdev-clock.h"
#ifndef ZYNQ_SLCR_ERR_DEBUG
#define ZYNQ_SLCR_ERR_DEBUG 0
@@ -45,6 +46,12 @@ REG32(LOCKSTA, 0x00c)
REG32(ARM_PLL_CTRL, 0x100)
REG32(DDR_PLL_CTRL, 0x104)
REG32(IO_PLL_CTRL, 0x108)
+/* fields for [ARM|DDR|IO]_PLL_CTRL registers */
+FIELD(xxx_PLL_CTRL, PLL_RESET, 0, 1)
+FIELD(xxx_PLL_CTRL, PLL_PWRDWN, 1, 1)
+FIELD(xxx_PLL_CTRL, PLL_BYPASS_QUAL, 3, 1)
+FIELD(xxx_PLL_CTRL, PLL_BYPASS_FORCE, 4, 1)
+FIELD(xxx_PLL_CTRL, PLL_FPDIV, 12, 7)
REG32(PLL_STATUS, 0x10c)
REG32(ARM_PLL_CFG, 0x110)
REG32(DDR_PLL_CFG, 0x114)
@@ -64,6 +71,10 @@ REG32(SMC_CLK_CTRL, 0x148)
REG32(LQSPI_CLK_CTRL, 0x14c)
REG32(SDIO_CLK_CTRL, 0x150)
REG32(UART_CLK_CTRL, 0x154)
+FIELD(UART_CLK_CTRL, CLKACT0, 0, 1)
+FIELD(UART_CLK_CTRL, CLKACT1, 1, 1)
+FIELD(UART_CLK_CTRL, SRCSEL, 4, 2)
+FIELD(UART_CLK_CTRL, DIVISOR, 8, 6)
REG32(SPI_CLK_CTRL, 0x158)
REG32(CAN_CLK_CTRL, 0x15c)
REG32(CAN_MIOCLK_CTRL, 0x160)
@@ -179,11 +190,106 @@ typedef struct ZynqSLCRState {
MemoryRegion iomem;
uint32_t regs[ZYNQ_SLCR_NUM_REGS];
+
+ClockIn *ps_clk;
+ClockOut *uart0_ref_clk;
+ClockOut *uart1_ref_clk;
} ZynqSLCRState;
-static void zynq_slcr_reset(DeviceState *d)
+/*
+ * return the output frequency of ARM/DDR/IO pll
+ * using input frequency and PLL_CTRL register
+ */
+static uint64_t zynq_slcr_compute_pll(uint64_t input, uint32_t ctrl_reg)
+{
+uint32_t mult = ((ctrl_reg & R_xxx_PLL_CTRL_PLL_FPDIV_MASK) >>
+R_xxx_PLL_CTRL_PLL_FPDIV_SHIFT);
+
+/* first, check if pll is bypassed */
+if (ctrl_reg & R_xxx_PLL_CTRL_PLL_BYPASS_FORCE_MASK) {
+return input;
+}
+
+/* is pll disabled ? */
+if (ctrl_reg & (R_xxx_PLL_CTRL_PLL_RESET_MASK |
+R_xxx_PLL_CTRL_PLL_PWRDWN_MASK)) {
+return 0;
+}
+
+return input * mult;
+}
+
+/*
+ * return the output frequency of a clock given:
+ * + the frequencies in an array corresponding to mux's indexes
+ * + the register xxx_CLK_CTRL value
+ * + enable bit index in ctrl register
+ *
+ * This function make the assumption that ctrl_reg value is organized as
follow:
+ * + bits[13:8] clock divisor
+ * + bits[5:4] clock mux selector (index in array)
+ * + bits[index] clock enable
+ */
+static uint64_t zynq_slcr_compute_clock(const uint64_t mux[],
+uint32_t ctrl_reg,
+unsigned index)
+{
+uint32_t srcsel = extract32(ctrl_reg, 4, 2); /* bits [5:4] */
+uint32_t divisor = extract32(ctrl_reg, 8, 6); /* bits [13:8] */
+
+/* first, check if clock is enabled */
+if (((ctrl_reg >> index) & 1u) == 0) {
+return 0;
+}
+
+/*
+ * according to the Zynq technical ref. manual UG585 v1.12.2 in
+ * "Clocks" chapter, section 25.10.1 page 705" the range of the divisor
+ * is [1;63].
+ * So divide the source while avoiding division-by-zero.
+ */
+return mux[srcsel] / (divisor ? divisor : 1u);
+}
+
+/*
+ * macro helper around zynq_slcr_compute_clock to avoid repeating
+ * the register name.
+ */
+#define ZYNQ_COMPUTE_CLOCK(_state, _plls, _reg, _enable_field) \
+zynq_slcr_compute_clock((_plls), (_state)->regs[R_ ## _reg], \
+R_ ## _reg ## _ ## _enable_field ## _SHIFT)
+
+static void zynq_slcr_compute_clocks(ZynqSLCRState *s)
+{
+uint64_t ps_clk = clock_get_frequency(s->ps_clk);
+
+/* consider all output clocks are disabled while in reset */
+if (device_is_resetting(DEVICE(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_frequency(s->uart0_ref_clk,
+ZYNQ_COMPUTE_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT0));
+clock_set_frequency(s->uart1_ref_clk,
+ZYNQ_COMPUTE_CLOCK(s, uart_mux, UART_CLK_CTRL, CLKACT1));
+}
+
+static void zynq_slcr_ps_clk_callback(void *opaque)
+{
+ZynqSLCRState *s = (ZynqSLCRState *) opaque;
+zynq_slcr_compute_clocks(s);
+}
+
+static void zynq_slcr_reset_init(Object *obj
