| Message ID | 20200310210434.31544-3-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Wire up USB controllers in i.MX6 emulations | expand |
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
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
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
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
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,
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(+)