RE: [PATCH v14 4/4] arm: xlnx-versal: Connect usb to virt-versal
Hi Peter/Edgar,
> -Original Message-
> From: Edgar E. Iglesias
> Sent: Thursday, December 3, 2020 11:35 PM
> To: Peter Maydell
> Cc: Sai Pavan Boddu ; Markus Armbruster
> ; Marc-André Lureau ;
> Paolo Bonzini ; Gerd Hoffmann ;
> Edgar Iglesias ; Francisco Eduardo Iglesias
> ; Alistair Francis ; Eduardo
> Habkost ; Ying Fang ;
> Philippe Mathieu-Daudé ; Vikram Garhwal
> ; Paul Zimmerman ; Sai Pavan Boddu
> ; QEMU Developers
> Subject: Re: [PATCH v14 4/4] arm: xlnx-versal: Connect usb to virt-versal
>
> On Tue, Dec 01, 2020 at 11:34:25AM +, Peter Maydell wrote:
> > On Tue, 1 Dec 2020 at 08:34, Sai Pavan Boddu
> wrote:
> > >
> > > From: Vikram Garhwal
> > >
> > > 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
> > > Signed-off-by: Sai Pavan Boddu
> >
> > 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
[Sai Pavan Boddu] Thanks Edgar.
Regards,
Sai Pavan
>
> Cheers,
> Edgar
Re: [PATCH v14 4/4] arm: xlnx-versal: Connect usb to virt-versal
On Tue, Dec 01, 2020 at 11:34:25AM +, Peter Maydell wrote:
> On Tue, 1 Dec 2020 at 08:34, Sai Pavan Boddu
> wrote:
> >
> > From: Vikram Garhwal
> >
> > 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
> > Signed-off-by: Sai Pavan Boddu
>
> 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
Cheers,
Edgar
Re: [PATCH v14 4/4] arm: xlnx-versal: Connect usb to virt-versal
On Tue, 1 Dec 2020 at 08:34, Sai Pavan Boddu wrote:
>
> From: Vikram Garhwal
>
> 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
> Signed-off-by: Sai Pavan Boddu
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
