Re: [PATCH v14 4/4] arm: xlnx-versal: Connect usb to virt-versal

[Peter Maydell]

On Tue, 1 Dec 2020 at 08:34, Sai Pavan Boddu <sai.pavan.boddu@xilinx.com> wrote:
>
> From: Vikram Garhwal <fnu.vikram@xilinx.com>
>
> Connect VersalUbs2 subsystem to xlnx-versal SOC, its placed

Typo : "VersalUSB2".


> in iou of lpd domain and configure it as dual port host controller.
> Add the respective guest dts nodes for "xlnx-versal-virt" machine.
>
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>

Code looks OK but I'll let somebody else from Xilinx review the detail.

> +static void fdt_add_usb_xhci_nodes(VersalVirt *s)
> +{
> +    const char clocknames[] = "bus_clk\0ref_clk";
> +    char *name = g_strdup_printf("/usb@%" PRIx32, MM_USB2_CTRL_REGS);
> +    const char compat[] = "xlnx,versal-dwc3";
> +
> +    qemu_fdt_add_subnode(s->fdt, name);
> +    qemu_fdt_setprop(s->fdt, name, "compatible",
> +                         compat, sizeof(compat));
> +    qemu_fdt_setprop_sized_cells(s->fdt, name, "reg",
> +                                 2, MM_USB2_CTRL_REGS,
> +                                 2, MM_USB2_CTRL_REGS_SIZE);
> +    qemu_fdt_setprop(s->fdt, name, "clock-names",
> +                         clocknames, sizeof(clocknames));
> +    qemu_fdt_setprop_cells(s->fdt, name, "clocks",
> +                               s->phandle.clk_25Mhz, s->phandle.clk_125Mhz);
> +    qemu_fdt_setprop(s->fdt, name, "ranges", NULL, 0);
> +    qemu_fdt_setprop_cell(s->fdt, name, "#address-cells", 2);
> +    qemu_fdt_setprop_cell(s->fdt, name, "#size-cells", 2);
> +    qemu_fdt_setprop_cell(s->fdt, name, "phandle", s->phandle.usb);
> +    g_free(name);
> +
> +    {
> +        const char irq_name[] = "dwc_usb3";
> +        const char compat[] = "snps,dwc3";

Minor coding style side note, but I'm not hugely fond of
code blocks in the middle of functions just for declaring
variables. You could either put these variable declarations
at the top of the function, or if you think the code in the
block is self contained and worth putting in its own function
you could do that.

thanks
-- PMM