| Message ID | 20200313014551.12554-2-linux@roeck-us.net |
|---|---|
| State | New |
| Headers | show |
| Series | Wire up USB controllers in i.MX6 emulations | expand |
On 3/13/20 2:45 AM, Guenter Roeck wrote: > Add basic USB PHY support as implemented in i.MX23, i.MX28, i.MX6, > and i.MX7 SoCs. > > The only support really needed - at least to boot Linux - is support > for soft reset, which needs to reset various registers to their initial > value. Otherwise, just record register values. > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > v3: Added Reviewed-by:; > dropped duplicate "the" in comments; > added new files to MAINTAINERS > v2: New patch, replacing dummy STMP register support with basic USB PHY > emulation. > > MAINTAINERS | 2 + > hw/arm/Kconfig | 1 + > hw/usb/Kconfig | 5 + > hw/usb/Makefile.objs | 2 + > hw/usb/imx-usb-phy.c | 225 +++++++++++++++++++++++++++++++++++ > include/hw/usb/imx-usb-phy.h | 53 +++++++++ > 6 files changed, 288 insertions(+) > create mode 100644 hw/usb/imx-usb-phy.c > create mode 100644 include/hw/usb/imx-usb-phy.h > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig > index bc54fd61f9..21c627c3b7 100644 > --- a/hw/arm/Kconfig > +++ b/hw/arm/Kconfig > @@ -361,6 +361,7 @@ config FSL_IMX6 > select IMX > select IMX_FEC > select IMX_I2C > + select IMX_USBPHY > select SDHCI I know it is merged, but FYI this change belongs to patch 5 of this series "hw/arm/fsl-imx6: Wire up USB controllers" where you add the dependency to the machine.
On Fri, 13 Mar 2020 at 01:45, Guenter Roeck <linux@roeck-us.net> wrote: > > Add basic USB PHY support as implemented in i.MX23, i.MX28, i.MX6, > and i.MX7 SoCs. > > The only support really needed - at least to boot Linux - is support > for soft reset, which needs to reset various registers to their initial > value. Otherwise, just record register values. > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> Hi Guenter; we've had a fuzzer report that this device model accesses off the end of the usbphy[] array: https://gitlab.com/qemu-project/qemu/-/issues/1408 > +static uint64_t imx_usbphy_read(void *opaque, hwaddr offset, unsigned size) > +{ > + IMXUSBPHYState *s = (IMXUSBPHYState *)opaque; > + uint32_t index = offset >> 2; > + uint32_t value; > + default: > + value = s->usbphy[index]; No bounds check in the default case (or ditto in the write function)... > + break; > + } > + return (uint64_t)value; > +} > +static void imx_usbphy_realize(DeviceState *dev, Error **errp) > +{ > + IMXUSBPHYState *s = IMX_USBPHY(dev); > + > + memory_region_init_io(&s->iomem, OBJECT(s), &imx_usbphy_ops, s, > + "imx-usbphy", 0x1000); ...and the memory region is created at size 0x1000 so the read/write fns can be called with offsets up to that size... > + sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem); > +} > +enum IMXUsbPhyRegisters { > + USBPHY_PWD, > + USBPHY_PWD_SET, > + USBPHY_PWD_CLR, > + USBPHY_PWD_TOG, > + USBPHY_TX, > + USBPHY_TX_SET, > + USBPHY_TX_CLR, > + USBPHY_TX_TOG, > + USBPHY_RX, > + USBPHY_RX_SET, > + USBPHY_RX_CLR, > + USBPHY_RX_TOG, > + USBPHY_CTRL, > + USBPHY_CTRL_SET, > + USBPHY_CTRL_CLR, > + USBPHY_CTRL_TOG, > + USBPHY_STATUS, > + USBPHY_DEBUG = 0x14, > + USBPHY_DEBUG_SET, > + USBPHY_DEBUG_CLR, > + USBPHY_DEBUG_TOG, > + USBPHY_DEBUG0_STATUS, > + USBPHY_DEBUG1 = 0x1c, > + USBPHY_DEBUG1_SET, > + USBPHY_DEBUG1_CLR, > + USBPHY_DEBUG1_TOG, > + USBPHY_VERSION, > + USBPHY_MAX > +}; > + > +#define USBPHY_CTRL_SFTRST BIT(31) > + > +#define TYPE_IMX_USBPHY "imx.usbphy" > +#define IMX_USBPHY(obj) OBJECT_CHECK(IMXUSBPHYState, (obj), TYPE_IMX_USBPHY) > + > +typedef struct IMXUSBPHYState { > + /* <private> */ > + SysBusDevice parent_obj; > + > + /* <public> */ > + MemoryRegion iomem; > + > + uint32_t usbphy[USBPHY_MAX]; ...but the array is only created with USBPHY_MAX entries. Do you know what the device is supposed to do with these off-the-end acceses? We could either reduce the memory region size or bounds check and RAZ/WI the out-of-range accesses. thanks -- PMM
Hi Peter, On 3/16/23 06:41, Peter Maydell wrote: > On Fri, 13 Mar 2020 at 01:45, Guenter Roeck <linux@roeck-us.net> wrote: >> >> Add basic USB PHY support as implemented in i.MX23, i.MX28, i.MX6, >> and i.MX7 SoCs. >> >> The only support really needed - at least to boot Linux - is support >> for soft reset, which needs to reset various registers to their initial >> value. Otherwise, just record register values. >> >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > Hi Guenter; we've had a fuzzer report that this device model > accesses off the end of the usbphy[] array: > https://gitlab.com/qemu-project/qemu/-/issues/1408 > Good catch. And an obvious bug, sorry. >> +static uint64_t imx_usbphy_read(void *opaque, hwaddr offset, unsigned size) >> +{ >> + IMXUSBPHYState *s = (IMXUSBPHYState *)opaque; >> + uint32_t index = offset >> 2; >> + uint32_t value; > > >> + default: >> + value = s->usbphy[index]; > > No bounds check in the default case (or ditto in the write function)... > >> + break; >> + } >> + return (uint64_t)value; >> +} > >> +static void imx_usbphy_realize(DeviceState *dev, Error **errp) >> +{ >> + IMXUSBPHYState *s = IMX_USBPHY(dev); >> + >> + memory_region_init_io(&s->iomem, OBJECT(s), &imx_usbphy_ops, s, >> + "imx-usbphy", 0x1000); > > ...and the memory region is created at size 0x1000 so the read/write > fns can be called with offsets up to that size... > >> + sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem); >> +} > >> +enum IMXUsbPhyRegisters { >> + USBPHY_PWD, >> + USBPHY_PWD_SET, >> + USBPHY_PWD_CLR, >> + USBPHY_PWD_TOG, >> + USBPHY_TX, >> + USBPHY_TX_SET, >> + USBPHY_TX_CLR, >> + USBPHY_TX_TOG, >> + USBPHY_RX, >> + USBPHY_RX_SET, >> + USBPHY_RX_CLR, >> + USBPHY_RX_TOG, >> + USBPHY_CTRL, >> + USBPHY_CTRL_SET, >> + USBPHY_CTRL_CLR, >> + USBPHY_CTRL_TOG, >> + USBPHY_STATUS, >> + USBPHY_DEBUG = 0x14, >> + USBPHY_DEBUG_SET, >> + USBPHY_DEBUG_CLR, >> + USBPHY_DEBUG_TOG, >> + USBPHY_DEBUG0_STATUS, >> + USBPHY_DEBUG1 = 0x1c, >> + USBPHY_DEBUG1_SET, >> + USBPHY_DEBUG1_CLR, >> + USBPHY_DEBUG1_TOG, >> + USBPHY_VERSION, >> + USBPHY_MAX >> +}; >> + >> +#define USBPHY_CTRL_SFTRST BIT(31) >> + >> +#define TYPE_IMX_USBPHY "imx.usbphy" >> +#define IMX_USBPHY(obj) OBJECT_CHECK(IMXUSBPHYState, (obj), TYPE_IMX_USBPHY) >> + >> +typedef struct IMXUSBPHYState { >> + /* <private> */ >> + SysBusDevice parent_obj; >> + >> + /* <public> */ >> + MemoryRegion iomem; >> + >> + uint32_t usbphy[USBPHY_MAX]; > > ...but the array is only created with USBPHY_MAX entries. > > Do you know what the device is supposed to do with these > off-the-end acceses? We could either reduce the memory region > size or bounds check and RAZ/WI the out-of-range accesses. > I have no idea what the real hardware would do. The datasheets (at least the ones I checked) don't say, only that the region size is 4k. I would suggest a bounds check, ignore out-of-bounds writes (maybe with a log message), and return 0 for reads (which I think is what you suggest with RAZ/WI). Want me to send a patch ? Thanks, Guenter
On Thu, 16 Mar 2023 at 14:12, Guenter Roeck <linux@roeck-us.net> wrote: > > Hi Peter, > > On 3/16/23 06:41, Peter Maydell wrote: > > On Fri, 13 Mar 2020 at 01:45, Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> Add basic USB PHY support as implemented in i.MX23, i.MX28, i.MX6, > >> and i.MX7 SoCs. > >> > >> The only support really needed - at least to boot Linux - is support > >> for soft reset, which needs to reset various registers to their initial > >> value. Otherwise, just record register values. > >> > >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > > > Hi Guenter; we've had a fuzzer report that this device model > > accesses off the end of the usbphy[] array: > > https://gitlab.com/qemu-project/qemu/-/issues/1408 > > > > Good catch. And an obvious bug, sorry. > > > Do you know what the device is supposed to do with these > > off-the-end acceses? We could either reduce the memory region > > size or bounds check and RAZ/WI the out-of-range accesses. > > > > I have no idea what the real hardware would do. The datasheets (at > least the ones I checked) don't say, only that the region size is 4k. > I would suggest a bounds check, ignore out-of-bounds writes (maybe > with a log message), and return 0 for reads (which I think is what > you suggest with RAZ/WI). > > Want me to send a patch ? If you have the time, that would be great. I expect you're better set up to test it than I am... thanks -- PMM
On Thu, Mar 16, 2023 at 02:51:23PM +0000, Peter Maydell wrote: > On Thu, 16 Mar 2023 at 14:12, Guenter Roeck <linux@roeck-us.net> wrote: > > > > Hi Peter, > > > > On 3/16/23 06:41, Peter Maydell wrote: > > > On Fri, 13 Mar 2020 at 01:45, Guenter Roeck <linux@roeck-us.net> wrote: > > >> > > >> Add basic USB PHY support as implemented in i.MX23, i.MX28, i.MX6, > > >> and i.MX7 SoCs. > > >> > > >> The only support really needed - at least to boot Linux - is support > > >> for soft reset, which needs to reset various registers to their initial > > >> value. Otherwise, just record register values. > > >> > > >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > > > > > Hi Guenter; we've had a fuzzer report that this device model > > > accesses off the end of the usbphy[] array: > > > https://gitlab.com/qemu-project/qemu/-/issues/1408 > > > > > > > Good catch. And an obvious bug, sorry. > > > > > > > Do you know what the device is supposed to do with these > > > off-the-end acceses? We could either reduce the memory region > > > size or bounds check and RAZ/WI the out-of-range accesses. > > > > > > > I have no idea what the real hardware would do. The datasheets (at > > least the ones I checked) don't say, only that the region size is 4k. > > I would suggest a bounds check, ignore out-of-bounds writes (maybe > > with a log message), and return 0 for reads (which I think is what > > you suggest with RAZ/WI). > > > > Want me to send a patch ? > > If you have the time, that would be great. I expect you're > better set up to test it than I am... > I prepared a patch. Currently testing. Guenter
diff --git a/MAINTAINERS b/MAINTAINERS index d881ba7d9c..1cfdeeae32 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -748,6 +748,8 @@ F: hw/arm/sabrelite.c F: hw/arm/fsl-imx6.c F: hw/misc/imx6_*.c F: hw/ssi/imx_spi.c +F: hw/usb/imx-usb-phy.c +F: include/hw/usb/imx-usb-phy.h F: include/hw/arm/fsl-imx6.h F: include/hw/misc/imx6_*.h F: include/hw/ssi/imx_spi.h diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig index bc54fd61f9..21c627c3b7 100644 --- a/hw/arm/Kconfig +++ b/hw/arm/Kconfig @@ -361,6 +361,7 @@ config FSL_IMX6 select IMX select IMX_FEC select IMX_I2C + select IMX_USBPHY select SDHCI config ASPEED_SOC diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig index 5e70ed5f7b..464348ba14 100644 --- a/hw/usb/Kconfig +++ b/hw/usb/Kconfig @@ -91,3 +91,8 @@ config USB_STORAGE_MTP bool default y depends on USB + +config IMX_USBPHY + bool + default y + depends on USB diff --git a/hw/usb/Makefile.objs b/hw/usb/Makefile.objs index 2b10868937..66835e5bf7 100644 --- a/hw/usb/Makefile.objs +++ b/hw/usb/Makefile.objs @@ -61,3 +61,5 @@ common-obj-$(CONFIG_XEN) += xen-usb.o xen-usb.o-cflags := $(LIBUSB_CFLAGS) xen-usb.o-libs := $(LIBUSB_LIBS) endif + +common-obj-$(CONFIG_IMX_USBPHY) += imx-usb-phy.o diff --git a/hw/usb/imx-usb-phy.c b/hw/usb/imx-usb-phy.c new file mode 100644 index 0000000000..e705a03a1f --- /dev/null +++ b/hw/usb/imx-usb-phy.c @@ -0,0 +1,225 @@ +/* + * i.MX USB PHY + * + * Copyright (c) 2020 Guenter Roeck <linux@roeck-us.net> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + * + * We need to implement basic reset control in the PHY control register. + * For everything else, it is sufficient to set whatever is written. + */ + +#include "qemu/osdep.h" +#include "hw/usb/imx-usb-phy.h" +#include "migration/vmstate.h" +#include "qemu/log.h" +#include "qemu/module.h" + +static const VMStateDescription vmstate_imx_usbphy = { + .name = TYPE_IMX_USBPHY, + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT32_ARRAY(usbphy, IMXUSBPHYState, USBPHY_MAX), + VMSTATE_END_OF_LIST() + }, +}; + +static void imx_usbphy_softreset(IMXUSBPHYState *s) +{ + s->usbphy[USBPHY_PWD] = 0x001e1c00; + s->usbphy[USBPHY_TX] = 0x10060607; + s->usbphy[USBPHY_RX] = 0x00000000; + s->usbphy[USBPHY_CTRL] = 0xc0200000; +} + +static void imx_usbphy_reset(DeviceState *dev) +{ + IMXUSBPHYState *s = IMX_USBPHY(dev); + + s->usbphy[USBPHY_STATUS] = 0x00000000; + s->usbphy[USBPHY_DEBUG] = 0x7f180000; + s->usbphy[USBPHY_DEBUG0_STATUS] = 0x00000000; + s->usbphy[USBPHY_DEBUG1] = 0x00001000; + s->usbphy[USBPHY_VERSION] = 0x04020000; + + imx_usbphy_softreset(s); +} + +static uint64_t imx_usbphy_read(void *opaque, hwaddr offset, unsigned size) +{ + IMXUSBPHYState *s = (IMXUSBPHYState *)opaque; + uint32_t index = offset >> 2; + uint32_t value; + + switch (index) { + case USBPHY_PWD_SET: + case USBPHY_TX_SET: + case USBPHY_RX_SET: + case USBPHY_CTRL_SET: + case USBPHY_DEBUG_SET: + case USBPHY_DEBUG1_SET: + /* + * All REG_NAME_SET register access are in fact targeting the + * REG_NAME register. + */ + value = s->usbphy[index - 1]; + break; + case USBPHY_PWD_CLR: + case USBPHY_TX_CLR: + case USBPHY_RX_CLR: + case USBPHY_CTRL_CLR: + case USBPHY_DEBUG_CLR: + case USBPHY_DEBUG1_CLR: + /* + * All REG_NAME_CLR register access are in fact targeting the + * REG_NAME register. + */ + value = s->usbphy[index - 2]; + break; + case USBPHY_PWD_TOG: + case USBPHY_TX_TOG: + case USBPHY_RX_TOG: + case USBPHY_CTRL_TOG: + case USBPHY_DEBUG_TOG: + case USBPHY_DEBUG1_TOG: + /* + * All REG_NAME_TOG register access are in fact targeting the + * REG_NAME register. + */ + value = s->usbphy[index - 3]; + break; + default: + value = s->usbphy[index]; + break; + } + return (uint64_t)value; +} + +static void imx_usbphy_write(void *opaque, hwaddr offset, uint64_t value, + unsigned size) +{ + IMXUSBPHYState *s = (IMXUSBPHYState *)opaque; + uint32_t index = offset >> 2; + + switch (index) { + case USBPHY_CTRL: + s->usbphy[index] = value; + if (value & USBPHY_CTRL_SFTRST) { + imx_usbphy_softreset(s); + } + break; + case USBPHY_PWD: + case USBPHY_TX: + case USBPHY_RX: + case USBPHY_STATUS: + case USBPHY_DEBUG: + case USBPHY_DEBUG1: + s->usbphy[index] = value; + break; + case USBPHY_CTRL_SET: + s->usbphy[index - 1] |= value; + if (value & USBPHY_CTRL_SFTRST) { + imx_usbphy_softreset(s); + } + break; + case USBPHY_PWD_SET: + case USBPHY_TX_SET: + case USBPHY_RX_SET: + case USBPHY_DEBUG_SET: + case USBPHY_DEBUG1_SET: + /* + * All REG_NAME_SET register access are in fact targeting the + * REG_NAME register. So we change the value of the REG_NAME + * register, setting bits passed in the value. + */ + s->usbphy[index - 1] |= value; + break; + case USBPHY_PWD_CLR: + case USBPHY_TX_CLR: + case USBPHY_RX_CLR: + case USBPHY_CTRL_CLR: + case USBPHY_DEBUG_CLR: + case USBPHY_DEBUG1_CLR: + /* + * All REG_NAME_CLR register access are in fact targeting the + * REG_NAME register. So we change the value of the REG_NAME + * register, unsetting bits passed in the value. + */ + s->usbphy[index - 2] &= ~value; + break; + case USBPHY_CTRL_TOG: + s->usbphy[index - 3] ^= value; + if ((value & USBPHY_CTRL_SFTRST) && + (s->usbphy[index - 3] & USBPHY_CTRL_SFTRST)) { + imx_usbphy_softreset(s); + } + break; + case USBPHY_PWD_TOG: + case USBPHY_TX_TOG: + case USBPHY_RX_TOG: + case USBPHY_DEBUG_TOG: + case USBPHY_DEBUG1_TOG: + /* + * All REG_NAME_TOG register access are in fact targeting the + * REG_NAME register. So we change the value of the REG_NAME + * register, toggling bits passed in the value. + */ + s->usbphy[index - 3] ^= value; + break; + default: + /* Other registers are read-only */ + break; + } +} + +static const struct MemoryRegionOps imx_usbphy_ops = { + .read = imx_usbphy_read, + .write = imx_usbphy_write, + .endianness = DEVICE_NATIVE_ENDIAN, + .valid = { + /* + * Our device would not work correctly if the guest was doing + * unaligned access. This might not be a limitation on the real + * device but in practice there is no reason for a guest to access + * this device unaligned. + */ + .min_access_size = 4, + .max_access_size = 4, + .unaligned = false, + }, +}; + +static void imx_usbphy_realize(DeviceState *dev, Error **errp) +{ + IMXUSBPHYState *s = IMX_USBPHY(dev); + + memory_region_init_io(&s->iomem, OBJECT(s), &imx_usbphy_ops, s, + "imx-usbphy", 0x1000); + sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem); +} + +static void imx_usbphy_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->reset = imx_usbphy_reset; + dc->vmsd = &vmstate_imx_usbphy; + dc->desc = "i.MX USB PHY Module"; + dc->realize = imx_usbphy_realize; +} + +static const TypeInfo imx_usbphy_info = { + .name = TYPE_IMX_USBPHY, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(IMXUSBPHYState), + .class_init = imx_usbphy_class_init, +}; + +static void imx_usbphy_register_types(void) +{ + type_register_static(&imx_usbphy_info); +} + +type_init(imx_usbphy_register_types) diff --git a/include/hw/usb/imx-usb-phy.h b/include/hw/usb/imx-usb-phy.h new file mode 100644 index 0000000000..07f0235d10 --- /dev/null +++ b/include/hw/usb/imx-usb-phy.h @@ -0,0 +1,53 @@ +#ifndef IMX_USB_PHY_H +#define IMX_USB_PHY_H + +#include "hw/sysbus.h" +#include "qemu/bitops.h" + +enum IMXUsbPhyRegisters { + USBPHY_PWD, + USBPHY_PWD_SET, + USBPHY_PWD_CLR, + USBPHY_PWD_TOG, + USBPHY_TX, + USBPHY_TX_SET, + USBPHY_TX_CLR, + USBPHY_TX_TOG, + USBPHY_RX, + USBPHY_RX_SET, + USBPHY_RX_CLR, + USBPHY_RX_TOG, + USBPHY_CTRL, + USBPHY_CTRL_SET, + USBPHY_CTRL_CLR, + USBPHY_CTRL_TOG, + USBPHY_STATUS, + USBPHY_DEBUG = 0x14, + USBPHY_DEBUG_SET, + USBPHY_DEBUG_CLR, + USBPHY_DEBUG_TOG, + USBPHY_DEBUG0_STATUS, + USBPHY_DEBUG1 = 0x1c, + USBPHY_DEBUG1_SET, + USBPHY_DEBUG1_CLR, + USBPHY_DEBUG1_TOG, + USBPHY_VERSION, + USBPHY_MAX +}; + +#define USBPHY_CTRL_SFTRST BIT(31) + +#define TYPE_IMX_USBPHY "imx.usbphy" +#define IMX_USBPHY(obj) OBJECT_CHECK(IMXUSBPHYState, (obj), TYPE_IMX_USBPHY) + +typedef struct IMXUSBPHYState { + /* <private> */ + SysBusDevice parent_obj; + + /* <public> */ + MemoryRegion iomem; + + uint32_t usbphy[USBPHY_MAX]; +} IMXUSBPHYState; + +#endif /* IMX_USB_PHY_H */