Re: [PATCH v2 2/3] hw/arm/fsl-imx6ul: Wire up USB controllers

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Mar 12, 2020 at 01:19:41PM +0000, Peter Maydell wrote:
> On Tue, 10 Mar 2020 at 21:04, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > IMX6UL USB controllers are quite similar to IMX7 USB controllers.
> > Wire them up the same way.
> >
> > The only real difference is that wiring up phy devices is necessary
> > to avoid phy reset timeouts in the Linux kernel.
> >
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > v2: Use USB PHY emulation
> >
> >  hw/arm/fsl-imx6ul.c         | 33 +++++++++++++++++++++++++++++++++
> >  include/hw/arm/fsl-imx6ul.h |  9 +++++++++
> >  2 files changed, 42 insertions(+)
> >
> > diff --git a/hw/arm/fsl-imx6ul.c b/hw/arm/fsl-imx6ul.c
> > @@ -456,6 +467,28 @@ static void fsl_imx6ul_realize(DeviceState *dev, Error **errp)
> >                                              FSL_IMX6UL_ENETn_TIMER_IRQ[i]));
> >      }
> >
> > +    /* USB */
> > +    for (i = 0; i < FSL_IMX6UL_NUM_USBS; i++) {
> > +        static const int FSL_IMX6UL_USBn_IRQ[] = {
> > +            FSL_IMX6UL_USB2_IRQ,
> > +            FSL_IMX6UL_USB1_IRQ,
> > +        };
> 
> Do we really want to wire up USB1 to USB2_IRQ and USB2 to USB1_IRQ ?
> If so, a comment explaining that it is deliberate would be useful.
> 
Yes. I think the definitions may be incorrect (or the Linux dts files are
incorrect, but that seems unlikely). I tried the other way but then I get
unhandled interrupt errors when trying to access a USB flash drive.

> Side note: not used here, but in the header file we define:
>     FSL_IMX6UL_USB1_IRQ     = 42,
>     FSL_IMX6UL_USB2_IRQ     = 43,
>     FSL_IMX6UL_USB_PHY1_IRQ = 44,
>     FSL_IMX6UL_USB_PHY2_IRQ = 44,
> 
> Is that last one correct, or a cut-n-paste error that should be "45" ?
> 
From Linux devicetree files:

	usbphy1: usbphy@20c9000 {
        	compatible = "fsl,imx6ul-usbphy", "fsl,imx23-usbphy";
                reg = <0x020c9000 0x1000>;
                interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
	usbphy2: usbphy@20ca000 {
                compatible = "fsl,imx6ul-usbphy", "fsl,imx23-usbphy";
                reg = <0x020ca000 0x1000>;
                interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
	usbotg1: usb@2184000 {
                compatible = "fsl,imx6ul-usb", "fsl,imx27-usb";
                reg = <0x02184000 0x200>;
                interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
	usbotg2: usb@2184200 {
                compatible = "fsl,imx6ul-usb", "fsl,imx27-usb";
                reg = <0x02184200 0x200>;
                interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;

Should I maybe fix the definitions in a separate patch ?

Thanks,
Guenter