[v6,9/9] hw/arm/xilinx_zynq: connect uart clocks to slcr

Commit Message

Damien Hedde Sept. 4, 2019, 12:55 p.m. UTC

Add the connection between the slcr's output clocks and the uarts inputs.

Also add the main board clock 'ps_clk', which is hard-coded to 33.33MHz
(the default frequency). This clock is used to feed the slcr's input
clock.

Signed-off-by: Damien Hedde <[e-mail]>
---
 hw/arm/xilinx_zynq.c | 64 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 8 deletions(-)

Comments

Peter Maydell Dec. 2, 2019, 3:34 p.m. UTC | #1

On Wed, 4 Sep 2019 at 13:56, Damien Hedde <[e-mail]> wrote:
>
> Add the connection between the slcr's output clocks and the uarts inputs.
>
> Also add the main board clock 'ps_clk', which is hard-coded to 33.33MHz
> (the default frequency). This clock is used to feed the slcr's input
> clock.
>
> Signed-off-by: Damien Hedde <[e-mail]>

Nothing obviously wrong in the body of the patch, but as with
7 and 8, review from a Xilinx person would be helpful.

/* board base frequency: 33.333333 MHz */
#define PS_CLK_FREQUENCY (100 * 1000 * 1000 / 3)

This is interesting, because it's not an integer... I'll come back
to this topic in a reply to the cover letter in a moment.

thanks
-- PMM

Damien Hedde Dec. 3, 2019, 2:59 p.m. UTC | #2

On 12/2/19 4:34 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <[e-mail]> wrote:
>>
>> Add the connection between the slcr's output clocks and the uarts inputs.
>>
>> Also add the main board clock 'ps_clk', which is hard-coded to 33.33MHz
>> (the default frequency). This clock is used to feed the slcr's input
>> clock.
>>
>> Signed-off-by: Damien Hedde <[e-mail]>
> 
> Nothing obviously wrong in the body of the patch, but as with
> 7 and 8, review from a Xilinx person would be helpful.
> 
> /* board base frequency: 33.333333 MHz */
> #define PS_CLK_FREQUENCY (100 * 1000 * 1000 / 3)
> 
> This is interesting, because it's not an integer... I'll come back
> to this topic in a reply to the cover letter in a moment.

For this precise case, what I wanted is the resulting integer which I
got from the device trees in linux (btw I should probably add this point
in  comment). Just thought it was more readable this way than "33333333".

--
Damien

Philippe Mathieu-Daudé Dec. 3, 2019, 3:29 p.m. UTC | #3

On 12/3/19 3:59 PM, Damien Hedde wrote:
> On 12/2/19 4:34 PM, Peter Maydell wrote:
>> On Wed, 4 Sep 2019 at 13:56, Damien Hedde <[e-mail]> wrote:
>>>
>>> Add the connection between the slcr's output clocks and the uarts inputs.
>>>
>>> Also add the main board clock 'ps_clk', which is hard-coded to 33.33MHz
>>> (the default frequency). This clock is used to feed the slcr's input
>>> clock.
>>>
>>> Signed-off-by: Damien Hedde <[e-mail]>
>>
>> Nothing obviously wrong in the body of the patch, but as with
>> 7 and 8, review from a Xilinx person would be helpful.
>>
>> /* board base frequency: 33.333333 MHz */
>> #define PS_CLK_FREQUENCY (100 * 1000 * 1000 / 3)
>>
>> This is interesting, because it's not an integer... I'll come back
>> to this topic in a reply to the cover letter in a moment.
> 
> For this precise case, what I wanted is the resulting integer which I
> got from the device trees in linux (btw I should probably add this point
> in  comment). Just thought it was more readable this way than "33333333".

FWIW I'm auditing if it is possible to use the float type for 
frequencies (before to ask on the list if this makes sense), because in 
hw/core/ptimer we use timers with periods, and loose some precision 
using 1/freq again.

Also we have MiB/KiB in "qemu/units.h" and I'd like to introduce MHz/KHz.

Patch

diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 89da34808b..4d5d214ea9 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -33,6 +33,15 @@ 
 #include "hw/char/cadence_uart.h"
 #include "hw/net/cadence_gem.h"
 #include "hw/cpu/a9mpcore.h"
+#include "hw/qdev-clock.h"
+#include "sysemu/reset.h"
+
+#define TYPE_ZYNQ_MACHINE MACHINE_TYPE_NAME("xilinx-zynq-a9")
+#define ZYNQ_MACHINE(obj) \
+    OBJECT_CHECK(ZynqMachineState, (obj), TYPE_ZYNQ_MACHINE)
+
+/* board base frequency: 33.333333 MHz */
+#define PS_CLK_FREQUENCY (100 * 1000 * 1000 / 3)
 
 #define NUM_SPI_FLASHES 4
 #define NUM_QSPI_FLASHES 2
@@ -73,6 +82,11 @@  static const int dma_irqs[8] = {
     0xe3401000 + ARMV7_IMM16(extract32((val), 16, 16)), /* movt r1 ... */ \
     0xe5801000 + (addr)
 
+typedef struct ZynqMachineState {
+    MachineState parent;
+    ClockOut *ps_clk;
+} ZynqMachineState;
+
 static void zynq_write_board_setup(ARMCPU *cpu,
                                    const struct arm_boot_info *info)
 {
@@ -157,6 +171,7 @@  static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq,
 
 static void zynq_init(MachineState *machine)
 {
+    ZynqMachineState *zynq_machine = ZYNQ_MACHINE(machine);
     ram_addr_t ram_size = machine->ram_size;
     const char *kernel_filename = machine->kernel_filename;
     const char *kernel_cmdline = machine->kernel_cmdline;
@@ -165,7 +180,7 @@  static void zynq_init(MachineState *machine)
     MemoryRegion *address_space_mem = get_system_memory();
     MemoryRegion *ext_ram = g_new(MemoryRegion, 1);
     MemoryRegion *ocm_ram = g_new(MemoryRegion, 1);
-    DeviceState *dev;
+    DeviceState *dev, *slcr;
     SysBusDevice *busdev;
     qemu_irq pic[64];
     int n;
@@ -210,9 +225,17 @@  static void zynq_init(MachineState *machine)
                           1, 0x0066, 0x0022, 0x0000, 0x0000, 0x0555, 0x2aa,
                           0);
 
-    dev = qdev_create(NULL, "xilinx,zynq_slcr");
-    qdev_init_nofail(dev);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 0xF8000000);
+    /* Create slcr, keep a pointer to connect clocks */
+    slcr = qdev_create(NULL, "xilinx,zynq_slcr");
+    qdev_init_nofail(slcr);
+    sysbus_mmio_map(SYS_BUS_DEVICE(slcr), 0, 0xF8000000);
+
+    /* Create the main clock source, and feed slcr with it */
+    zynq_machine->ps_clk = CLOCK_OUT(object_new(TYPE_CLOCK_OUT));
+    object_property_add_child(OBJECT(zynq_machine), "ps_clk",
+                              OBJECT(zynq_machine->ps_clk), &error_abort);
+    object_unref(OBJECT(zynq_machine->ps_clk));
+    clock_connect(qdev_get_clock_in(slcr, "ps_clk"), zynq_machine->ps_clk);
 
     dev = qdev_create(NULL, TYPE_A9MPCORE_PRIV);
     qdev_prop_set_uint32(dev, "num-cpu", 1);
@@ -233,8 +256,12 @@  static void zynq_init(MachineState *machine)
     sysbus_create_simple("xlnx,ps7-usb", 0xE0002000, pic[53-IRQ_OFFSET]);
     sysbus_create_simple("xlnx,ps7-usb", 0xE0003000, pic[76-IRQ_OFFSET]);
 
-    cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hd(0));
-    cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
+    dev = cadence_uart_create(0xE0000000, pic[59 - IRQ_OFFSET], serial_hd(0));
+    qdev_connect_clock_out(slcr, "uart0_ref_clk",
+                           qdev_get_clock_in(dev, "refclk"), &error_abort);
+    dev = cadence_uart_create(0xE0001000, pic[82 - IRQ_OFFSET], serial_hd(1));
+    qdev_connect_clock_out(slcr, "uart1_ref_clk",
+                           qdev_get_clock_in(dev, "refclk"), &error_abort);
 
     sysbus_create_varargs("cadence_ttc", 0xF8001000,
             pic[42-IRQ_OFFSET], pic[43-IRQ_OFFSET], pic[44-IRQ_OFFSET], NULL);
@@ -315,14 +342,35 @@  static void zynq_init(MachineState *machine)
     arm_load_kernel(ARM_CPU(first_cpu), &zynq_binfo);
 }
 
-static void zynq_machine_init(MachineClass *mc)
+static void zynq_reset(MachineState *machine)
+{
+    ZynqMachineState *zynq_machine = ZYNQ_MACHINE(machine);
+    qemu_devices_reset();
+    clock_set_frequency(zynq_machine->ps_clk, PS_CLK_FREQUENCY);
+}
+
+static void zynq_machine_class_init(ObjectClass *oc, void *data)
 {
+    MachineClass *mc = MACHINE_CLASS(oc);
     mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
     mc->init = zynq_init;
+    mc->reset = zynq_reset;
     mc->max_cpus = 1;
     mc->no_sdcard = 1;
     mc->ignore_memory_transaction_failures = true;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
 }
 
-DEFINE_MACHINE("xilinx-zynq-a9", zynq_machine_init)
+static const TypeInfo zynq_machine_type = {
+    .name = TYPE_ZYNQ_MACHINE,
+    .parent = TYPE_MACHINE,
+    .class_init = zynq_machine_class_init,
+    .instance_size = sizeof(ZynqMachineState),
+};
+
+static void zynq_machine_register_types(void)
+{
+    type_register_static(&zynq_machine_type);
+}
+
+type_init(zynq_machine_register_types)