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

Guenter Roeck posted 3 patches 5 years, 6 months ago
Maintainers: Gerd Hoffmann <kraxel@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Jean-Christophe Dubois <jcd@tribudubois.net>
There is a newer version of this series
[PATCH v2 2/3] hw/arm/fsl-imx6ul: Wire up USB controllers
Posted by Guenter Roeck 5 years, 6 months ago
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
index c405b68d1d..ef2a7a87e8 100644
--- a/hw/arm/fsl-imx6ul.c
+++ b/hw/arm/fsl-imx6ul.c
@@ -20,6 +20,7 @@
 #include "qapi/error.h"
 #include "hw/arm/fsl-imx6ul.h"
 #include "hw/misc/unimp.h"
+#include "hw/usb/imx-usb-phy.h"
 #include "hw/boards.h"
 #include "sysemu/sysemu.h"
 #include "qemu/error-report.h"
@@ -133,6 +134,16 @@ static void fsl_imx6ul_init(Object *obj)
                               TYPE_IMX_ENET);
     }
 
+    /* USB */
+    for (i = 0; i < FSL_IMX6UL_NUM_USBS; i++) {
+        snprintf(name, NAME_SIZE, "usb%d", i);
+        sysbus_init_child_obj(obj, name, &s->usb[i], sizeof(s->usb[i]),
+                              TYPE_CHIPIDEA);
+        snprintf(name, NAME_SIZE, "usbphy%d", i);
+        sysbus_init_child_obj(obj, name, &s->usbphy[i], sizeof(s->usbphy[i]),
+                              TYPE_IMX_USBPHY);
+    }
+
     /*
      * SDHCI
      */
@@ -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,
+        };
+
+        object_property_set_bool(OBJECT(&s->usbphy[i]), true, "realized",
+                                 &error_abort);
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->usbphy[i]), 0,
+                        FSL_IMX6UL_USBPHY1_ADDR + i * 0x1000);
+
+        object_property_set_bool(OBJECT(&s->usb[i]), true, "realized",
+                                 &error_abort);
+        sysbus_mmio_map(SYS_BUS_DEVICE(&s->usb[i]), 0,
+                        FSL_IMX6UL_USBO2_USB_ADDR + i * 0x200);
+
+        sysbus_connect_irq(SYS_BUS_DEVICE(&s->usb[i]), 0,
+                           qdev_get_gpio_in(DEVICE(&s->a7mpcore),
+                                            FSL_IMX6UL_USBn_IRQ[i]));
+    }
+
     /*
      * USDHC
      */
diff --git a/include/hw/arm/fsl-imx6ul.h b/include/hw/arm/fsl-imx6ul.h
index eda389aec7..6969911a2a 100644
--- a/include/hw/arm/fsl-imx6ul.h
+++ b/include/hw/arm/fsl-imx6ul.h
@@ -34,6 +34,8 @@
 #include "hw/sd/sdhci.h"
 #include "hw/ssi/imx_spi.h"
 #include "hw/net/imx_fec.h"
+#include "hw/usb/chipidea.h"
+#include "hw/usb/imx-usb-phy.h"
 #include "exec/memory.h"
 #include "cpu.h"
 
@@ -54,6 +56,7 @@ enum FslIMX6ULConfiguration {
     FSL_IMX6UL_NUM_I2CS         = 4,
     FSL_IMX6UL_NUM_ECSPIS       = 4,
     FSL_IMX6UL_NUM_ADCS         = 2,
+    FSL_IMX6UL_NUM_USBS         = 2,
 };
 
 typedef struct FslIMX6ULState {
@@ -77,6 +80,8 @@ typedef struct FslIMX6ULState {
     IMXFECState        eth[FSL_IMX6UL_NUM_ETHS];
     SDHCIState         usdhc[FSL_IMX6UL_NUM_USDHCS];
     IMX2WdtState       wdt[FSL_IMX6UL_NUM_WDTS];
+    IMXUSBPHYState     usbphy[FSL_IMX6UL_NUM_USBS];
+    ChipideaState      usb[FSL_IMX6UL_NUM_USBS];
     MemoryRegion       rom;
     MemoryRegion       caam;
     MemoryRegion       ocram;
@@ -145,6 +150,10 @@ enum FslIMX6ULMemoryMap {
     FSL_IMX6UL_EPIT2_ADDR           = 0x020D4000,
     FSL_IMX6UL_EPIT1_ADDR           = 0x020D0000,
     FSL_IMX6UL_SNVS_HP_ADDR         = 0x020CC000,
+    FSL_IMX6UL_USBPHY2_ADDR         = 0x020CA000,
+    FSL_IMX6UL_USBPHY2_SIZE         = (4 * 1024),
+    FSL_IMX6UL_USBPHY1_ADDR         = 0x020C9000,
+    FSL_IMX6UL_USBPHY1_SIZE         = (4 * 1024),
     FSL_IMX6UL_ANALOG_ADDR          = 0x020C8000,
     FSL_IMX6UL_CCM_ADDR             = 0x020C4000,
     FSL_IMX6UL_WDOG2_ADDR           = 0x020C0000,
-- 
2.17.1


Re: [PATCH v2 2/3] hw/arm/fsl-imx6ul: Wire up USB controllers
Posted by Peter Maydell 5 years, 6 months ago
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.

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" ?

thanks
-- PMM

Re: [PATCH v2 2/3] hw/arm/fsl-imx6ul: Wire up USB controllers
Posted by Guenter Roeck 5 years, 6 months ago
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

Re: [PATCH v2 2/3] hw/arm/fsl-imx6ul: Wire up USB controllers
Posted by Peter Maydell 5 years, 6 months ago
On Thu, 12 Mar 2020 at 16:55, Guenter Roeck <linux@roeck-us.net> wrote:
>
> 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:
> > > 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.

I guess we should check the datasheet and see if we just
have our #define names the wrong way around...

> > 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 ?

Yes please.

thanks
-- PMM

Re: [PATCH v2 2/3] hw/arm/fsl-imx6ul: Wire up USB controllers
Posted by Guenter Roeck 5 years, 6 months ago
On Thu, Mar 12, 2020 at 05:05:25PM +0000, Peter Maydell wrote:
> On Thu, 12 Mar 2020 at 16:55, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > 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:
> > > > 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.
> 
> I guess we should check the datasheet and see if we just
> have our #define names the wrong way around...
> 

From "i.MX 6UltraLite Applications Processor Reference Manual":

74 USB USBO2 USB OTG2
75 USB USBO2 USB OTG1
76 USB_PHY UTMI0 interrupt request
77 USB_PHY UTMI1 interrupt request

So it looks like the dts files in the Linux kernel are correct.

> > > 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 ?
> 
> Yes please.
> 
Ok, will do. And, sorry, I should have done that in the first place.

Thanks,
Guenter