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

[Sai Pavan Boddu]

Hi Peter/Edgar,

> -----Original Message-----
> From: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> Sent: Thursday, December 3, 2020 11:35 PM
> To: Peter Maydell <peter.maydell@linaro.org>
> Cc: Sai Pavan Boddu <saipava@xilinx.com>; Markus Armbruster
> <armbru@redhat.com>; Marc-André Lureau <marcandre.lureau@redhat.com>;
> Paolo Bonzini <pbonzini@redhat.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Edgar Iglesias <edgari@xilinx.com>; Francisco Eduardo Iglesias
> <figlesia@xilinx.com>; Alistair Francis <alistair.francis@wdc.com>; Eduardo
> Habkost <ehabkost@redhat.com>; Ying Fang <fangying1@huawei.com>;
> Philippe Mathieu-Daudé <philmd@redhat.com>; Vikram Garhwal
> <fnuv@xilinx.com>; Paul Zimmerman <pauldzim@gmail.com>; Sai Pavan Boddu
> <saipava@xilinx.com>; QEMU Developers <qemu-devel@nongnu.org>
> Subject: Re: [PATCH v14 4/4] arm: xlnx-versal: Connect usb to virt-versal
> 
> 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.
[Sai Pavan Boddu] Yeah. I could fix this in V15, Thanks.

> >
> 
> 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>
[Sai Pavan Boddu] Thanks Edgar.

Regards,
Sai Pavan
> 
> Cheers,
> Edgar