Re: [Qemu-riscv] [Qemu-devel] [PATCH v3 23/28] riscv: sifive_u: Fix broken GEM support

[Alistair Francis]

On Sun, Aug 11, 2019 at 1:15 AM Bin Meng <address@hidden> wrote:
>
> At present the GEM support in sifive_u machine is seriously broken.
>
> - The GEM block register base was set to a weird number (0x100900FC),
>   which for no way could work with the cadence_gem model in QEMU.
> - The generated DT node for GEM has a "clocks-names" which is an
>   invalid property name.
>
> Not like other GEM variants, the FU540-specific GEM has a management
> block to control 10/100/1000Mbps link speed changes, that is mapped
> to 0x100a0000. We can simply map it into MMIO space without special
> handling using create_unimplemented_device().
>
> Update the GEM node compatible string to use the official name used
> by the upstream Linux kernel, and add the management block reg base
> & size to the <reg> property encoding.
>
> Tested with upstream U-Boot and Linux kernel MACB drivers.
>
> Signed-off-by: Bin Meng <address@hidden>

Reviewed-by: Alistair Francis <address@hidden>

Alistair

>
> ---
>
> Changes in v3: None
> Changes in v2:
> - use create_unimplemented_device() to create the GEM management
>   block instead of sifive_mmio_emulate()
> - add "phy-handle" property to the ethernet node
>
>  hw/riscv/sifive_u.c         | 23 ++++++++++++++++++-----
>  include/hw/riscv/sifive_u.h |  3 ++-
>  2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 0c1a89f..e8bef44 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (c) 2016-2017 Sagar Karandikar, address@hidden
>   * Copyright (c) 2017 SiFive, Inc.
> + * Copyright (c) 2019 Bin Meng <address@hidden>
>   *
>   * Provides a board compatible with the SiFive Freedom U SDK:
>   *
> @@ -11,6 +12,7 @@
>   * 2) PLIC (Platform Level Interrupt Controller)
>   * 3) PRCI (Power, Reset, Clock, Interrupt)
>   * 4) OTP (One-Time Programmable) memory with stored serial number
> + * 5) GEM (Gigabit Ethernet Controller) and management block
>   *
>   * This board currently generates devicetree dynamically that indicates at 
> least
>   * two harts and up to five harts.
> @@ -38,6 +40,7 @@
>  #include "hw/loader.h"
>  #include "hw/sysbus.h"
>  #include "hw/char/serial.h"
> +#include "hw/misc/unimp.h"
>  #include "target/riscv/cpu.h"
>  #include "hw/riscv/riscv_hart.h"
>  #include "hw/riscv/sifive_plic.h"
> @@ -69,7 +72,8 @@ static const struct MemmapEntry {
>      [SIFIVE_U_UART1] =    { 0x10011000,     0x1000 },
>      [SIFIVE_U_OTP] =      { 0x10070000,     0x1000 },
>      [SIFIVE_U_DRAM] =     { 0x80000000,        0x0 },
> -    [SIFIVE_U_GEM] =      { 0x100900FC,     0x2000 },
> +    [SIFIVE_U_GEM] =      { 0x10090000,     0x2000 },
> +    [SIFIVE_U_GEM_MGMT] = { 0x100a0000,     0x1000 },
>  };
>
>  #define SIFIVE_OTP_SERIAL   1
> @@ -84,7 +88,7 @@ static void create_fdt(SiFiveUState *s, const struct 
> MemmapEntry *memmap,
>      char *nodename;
>      char ethclk_names[] = "pclk\0hclk";
>      uint32_t plic_phandle, prci_phandle, phandle = 1;
> -    uint32_t hfclk_phandle, rtcclk_phandle;
> +    uint32_t hfclk_phandle, rtcclk_phandle, phy_phandle;
>
>      fdt = s->fdt = create_device_tree(&s->fdt_size);
>      if (!fdt) {
> @@ -242,20 +246,25 @@ static void create_fdt(SiFiveUState *s, const struct 
> MemmapEntry *memmap,
>      g_free(cells);
>      g_free(nodename);
>
> +    phy_phandle = phandle++;
>      nodename = g_strdup_printf("/soc/ethernet@%lx",
>          (long)memmap[SIFIVE_U_GEM].base);
>      qemu_fdt_add_subnode(fdt, nodename);
> -    qemu_fdt_setprop_string(fdt, nodename, "compatible", "cdns,macb");
> +    qemu_fdt_setprop_string(fdt, nodename, "compatible",
> +        "sifive,fu540-c000-gem");
>      qemu_fdt_setprop_cells(fdt, nodename, "reg",
>          0x0, memmap[SIFIVE_U_GEM].base,
> -        0x0, memmap[SIFIVE_U_GEM].size);
> +        0x0, memmap[SIFIVE_U_GEM].size,
> +        0x0, memmap[SIFIVE_U_GEM_MGMT].base,
> +        0x0, memmap[SIFIVE_U_GEM_MGMT].size);
>      qemu_fdt_setprop_string(fdt, nodename, "reg-names", "control");
>      qemu_fdt_setprop_string(fdt, nodename, "phy-mode", "gmii");
> +    qemu_fdt_setprop_cell(fdt, nodename, "phy-handle", phy_phandle);
>      qemu_fdt_setprop_cell(fdt, nodename, "interrupt-parent", plic_phandle);
>      qemu_fdt_setprop_cell(fdt, nodename, "interrupts", SIFIVE_U_GEM_IRQ);
>      qemu_fdt_setprop_cells(fdt, nodename, "clocks",
>          prci_phandle, PRCI_CLK_GEMGXLPLL, prci_phandle, PRCI_CLK_GEMGXLPLL);
> -    qemu_fdt_setprop(fdt, nodename, "clocks-names", ethclk_names,
> +    qemu_fdt_setprop(fdt, nodename, "clock-names", ethclk_names,
>          sizeof(ethclk_names));
>      qemu_fdt_setprop_cell(fdt, nodename, "#address-cells", 1);
>      qemu_fdt_setprop_cell(fdt, nodename, "#size-cells", 0);
> @@ -264,6 +273,7 @@ static void create_fdt(SiFiveUState *s, const struct 
> MemmapEntry *memmap,
>      nodename = g_strdup_printf("/soc/ethernet@%lx/ethernet-phy@0",
>          (long)memmap[SIFIVE_U_GEM].base);
>      qemu_fdt_add_subnode(fdt, nodename);
> +    qemu_fdt_setprop_cell(fdt, nodename, "phandle", phy_phandle);
>      qemu_fdt_setprop_cell(fdt, nodename, "reg", 0x0);
>      g_free(nodename);
>
> @@ -456,6 +466,9 @@ static void riscv_sifive_u_soc_realize(DeviceState *dev, 
> Error **errp)
>      sysbus_mmio_map(SYS_BUS_DEVICE(&s->gem), 0, memmap[SIFIVE_U_GEM].base);
>      sysbus_connect_irq(SYS_BUS_DEVICE(&s->gem), 0,
>                         plic_gpios[SIFIVE_U_GEM_IRQ]);
> +
> +    create_unimplemented_device("riscv.sifive.u.gem-mgmt",
> +        memmap[SIFIVE_U_GEM_MGMT].base, memmap[SIFIVE_U_GEM_MGMT].size);
>  }
>
>  static void riscv_sifive_u_machine_init(MachineClass *mc)
> diff --git a/include/hw/riscv/sifive_u.h b/include/hw/riscv/sifive_u.h
> index 2a7877e..27b87cf 100644
> --- a/include/hw/riscv/sifive_u.h
> +++ b/include/hw/riscv/sifive_u.h
> @@ -56,7 +56,8 @@ enum {
>      SIFIVE_U_UART1,
>      SIFIVE_U_OTP,
>      SIFIVE_U_DRAM,
> -    SIFIVE_U_GEM
> +    SIFIVE_U_GEM,
> +    SIFIVE_U_GEM_MGMT
>  };
>
>  enum {
> --
> 2.7.4
>
>