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

["Edgar E. Iglesias" <edgar.iglesias@xilinx.com>]

On Tue, Dec 01, 2020 at 11:34:25AM +0000, Peter Maydell wrote:
> 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.
>

Hi Sai, I beleive I had already reviewed a previous version of this
patch so after you fix the stuff the Peter pointed out feel free to add my
Rb:

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Cheers,
Edgar