Changeset View
Standalone View
sys/dev/usb/controller/dwc3.c
| Show First 20 Lines • Show All 67 Lines • ▼ Show 20 Lines | |||||
| static struct ofw_compat_data compat_data[] = { | static struct ofw_compat_data compat_data[] = { | ||||
| { "snps,dwc3", 1 }, | { "snps,dwc3", 1 }, | ||||
| { NULL, 0 } | { NULL, 0 } | ||||
| }; | }; | ||||
| struct snps_dwc3_softc { | struct snps_dwc3_softc { | ||||
| struct xhci_softc sc; | struct xhci_softc sc; | ||||
| device_t dev; | device_t dev; | ||||
| char dr_mode[16]; | |||||
| struct resource * mem_res; | struct resource * mem_res; | ||||
| bus_space_tag_t bst; | bus_space_tag_t bst; | ||||
| bus_space_handle_t bsh; | bus_space_handle_t bsh; | ||||
| phandle_t node; | phandle_t node; | ||||
| phy_t usb2_phy; | phy_t usb2_phy; | ||||
| phy_t usb3_phy; | phy_t usb3_phy; | ||||
| }; | }; | ||||
| ▲ Show 20 Lines • Show All 154 Lines • ▼ Show 20 Lines | |||||
| } | } | ||||
| static void | static void | ||||
| snps_dwc3_do_quirks(struct snps_dwc3_softc *sc) | snps_dwc3_do_quirks(struct snps_dwc3_softc *sc) | ||||
| { | { | ||||
| uint32_t reg; | uint32_t reg; | ||||
| reg = DWC3_READ(sc, DWC3_GUSB2PHYCFG0); | reg = DWC3_READ(sc, DWC3_GUSB2PHYCFG0); | ||||
| if (OF_hasprop(sc->node, "snps,dis-u2-freeclk-exists-quirk")) | if (device_has_property(sc->dev, "snps,dis-u2-freeclk-exists-quirk")) | ||||
| reg &= ~DWC3_GUSB2PHYCFG0_U2_FREECLK_EXISTS; | reg &= ~DWC3_GUSB2PHYCFG0_U2_FREECLK_EXISTS; | ||||
| else | else | ||||
| reg |= DWC3_GUSB2PHYCFG0_U2_FREECLK_EXISTS; | reg |= DWC3_GUSB2PHYCFG0_U2_FREECLK_EXISTS; | ||||
| if (OF_hasprop(sc->node, "snps,dis_u2_susphy_quirk")) | if (device_has_property(sc->dev, "snps,dis_u2_susphy_quirk")) | ||||
| reg &= ~DWC3_GUSB2PHYCFG0_SUSPENDUSB20; | reg &= ~DWC3_GUSB2PHYCFG0_SUSPENDUSB20; | ||||
| else | else | ||||
| reg |= DWC3_GUSB2PHYCFG0_SUSPENDUSB20; | reg |= DWC3_GUSB2PHYCFG0_SUSPENDUSB20; | ||||
| if (OF_hasprop(sc->node, "snps,dis_enblslpm_quirk")) | if (device_has_property(sc->dev, "snps,dis_enblslpm_quirk")) | ||||
| reg &= ~DWC3_GUSB2PHYCFG0_ENBLSLPM; | reg &= ~DWC3_GUSB2PHYCFG0_ENBLSLPM; | ||||
| else | else | ||||
| reg |= DWC3_GUSB2PHYCFG0_ENBLSLPM; | reg |= DWC3_GUSB2PHYCFG0_ENBLSLPM; | ||||
| DWC3_WRITE(sc, DWC3_GUSB2PHYCFG0, reg); | DWC3_WRITE(sc, DWC3_GUSB2PHYCFG0, reg); | ||||
| reg = DWC3_READ(sc, DWC3_GUCTL1); | reg = DWC3_READ(sc, DWC3_GUCTL1); | ||||
| if (OF_hasprop(sc->node, "snps,dis-tx-ipgap-linecheck-quirk")) | if (device_has_property(sc->dev, "snps,dis-tx-ipgap-linecheck-quirk")) | ||||
| reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS; | reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS; | ||||
| DWC3_WRITE(sc, DWC3_GUCTL1, reg); | DWC3_WRITE(sc, DWC3_GUCTL1, reg); | ||||
| reg = DWC3_READ(sc, DWC3_GUSB3PIPECTL0); | reg = DWC3_READ(sc, DWC3_GUSB3PIPECTL0); | ||||
| if (OF_hasprop(sc->node, "snps,dis-del-phy-power-chg-quirk")) | if (device_has_property(sc->dev, "snps,dis-del-phy-power-chg-quirk")) | ||||
| reg |= DWC3_GUSB3PIPECTL0_DELAYP1TRANS; | reg |= DWC3_GUSB3PIPECTL0_DELAYP1TRANS; | ||||
| if (OF_hasprop(sc->node, "snps,dis_rxdet_inp3_quirk")) | if (device_has_property(sc->dev, "snps,dis_rxdet_inp3_quirk")) | ||||
| reg |= DWC3_GUSB3PIPECTL0_DISRXDETINP3; | reg |= DWC3_GUSB3PIPECTL0_DISRXDETINP3; | ||||
| DWC3_WRITE(sc, DWC3_GUSB3PIPECTL0, reg); | DWC3_WRITE(sc, DWC3_GUSB3PIPECTL0, reg); | ||||
| } | } | ||||
| static int | static int | ||||
| snps_dwc3_probe(device_t dev) | snps_dwc3_probe(device_t dev) | ||||
| { | { | ||||
| struct snps_dwc3_softc *sc; | char dr_mode[16]; | ||||
| ssize_t s; | |||||
| if (!ofw_bus_status_okay(dev)) | if (!ofw_bus_status_okay(dev)) | ||||
| return (ENXIO); | return (ENXIO); | ||||
| if (ofw_bus_search_compatible(dev, compat_data)->ocd_data == 0) | if (ofw_bus_search_compatible(dev, compat_data)->ocd_data == 0) | ||||
| return (ENXIO); | return (ENXIO); | ||||
| sc = device_get_softc(dev); | s = device_get_property(dev, "dr_mode", dr_mode, sizeof(dr_mode)); | ||||
bz: Can someone please test this with FDT?
ofwdump is fine on the running system but the… | |||||
kdUnsubmitted Not Done Inline ActionsWith this patch applied XHCI fails to attach with: xhci0: Only host mode is supported simplebus0: <usb@3100000> mem 0x3100000-0x310ffff irq 36 compat fsl,ls1028a-dwc3 (no driver attached) xhci0: Only host mode is supported simplebus0: <usb@3110000> mem 0x3110000-0x311ffff irq 37 compat fsl,ls1028a-dwc3 (no driver attached) I tried it on LS1028A board. kd: With this patch applied XHCI fails to attach with:
```
xhci0: Only host mode is supported… | |||||
bzAuthorUnsubmitted Done Inline ActionsThanks. That was re-assuring that my nanopi is not at fault. I locally adjusted the code and I get: xhci0: Found dr_mode 'tsoh' but only 'host' supported. s=5 Seems something is not doing the right thing there in simplebus/FDT land for a string with device_get_property()? bz: Thanks. That was re-assuring that my nanopi is not at fault.
I locally adjusted the code and I… | |||||
andrewUnsubmitted Not Done Inline Actionsdevice_get_property is broken when reading a string on little endian OFW/FDT systems. It calls OF_getencprop which swaps endian on from big to host endian on each 4 byte block of data. This is needed for integer types that are stored in 4 byte blocks in big endian, but not for strings. You are reading 16 bytes of data so would not have triggered the (length % 4) == 0 assertion. andrew: `device_get_property` is broken when reading a string on little endian OFW/FDT systems. It… | |||||
mwUnsubmitted Not Done Inline Actions
After internal discussion and some research, it seems to be impossible to handle all types in runtime for OF_ (contrary to ACPI_). Therefore the most feasible and simple solution is to add another method (eg. device_get_string_property) for handling string properties and let user pick the appropriate one depending on a processed binding. Linux does exactly that and has various available types of routines, see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/property.h?h=v5.16-rc3#n37 mw: > `device_get_property` is broken when reading a string on little endian OFW/FDT systems. It… | |||||
bzAuthorUnsubmitted Done Inline ActionsI'd suggest opening a review and make this a parent review so we can continue here once that is sorted? bz: I'd suggest opening a review and make this a parent review so we can continue here once that is… | |||||
| sc->node = ofw_bus_get_node(dev); | if (s == -1) { | ||||
| OF_getprop(sc->node, "dr_mode", sc->dr_mode, sizeof(sc->dr_mode)); | device_printf(dev, "Cannot determine dr_mode\n"); | ||||
| if (strcmp(sc->dr_mode, "host") != 0) { | return (ENXIO); | ||||
| } | |||||
| if (strcmp(dr_mode, "host") != 0) { | |||||
| device_printf(dev, "Only host mode is supported\n"); | device_printf(dev, "Only host mode is supported\n"); | ||||
Done Inline ActionsDo we need to "zero" the variable? I think it should be rather safe to rely on ACPI/OF_ internals. mw: Do we need to "zero" the variable? I think it should be rather safe to rely on ACPI/OF_… | |||||
Done Inline ActionsGetting back to this; yes! or we need to change to comparing the return length using strncmp(dr_mode, "host", s) != 0 snps_dwc3_acpi0: Found dr_mode 'hostÿÿÿÿpèû' but only 'host' supported. s=4 snps_dwc3_acpi0: Found dr_mode 'hostÿÿÿÿpèû' but only 'host' supported. s=4 bz: Getting back to this; yes! or we need to change to comparing the return length using strncmp… | |||||
Not Done Inline ActionsThanks, this is what I suspected - we obtain raw buffer from ACPI. I thought about it and in this case, I'd refrain from modifying the acpi_get_property callback. Anyway, I think this change belongs more to https://reviews.freebsd.org/D32256 instead of this ofw->device migration. I think strncmp option is a bit cleaner. Instead of memset, wouldn't declaring a zeroed array suffice? Can you please leave code as is and just declare dr_mode like this: mw: Thanks, this is what I suspected - we obtain raw buffer from ACPI. I thought about it and in… | |||||
| return (ENXIO); | return (ENXIO); | ||||
| } | } | ||||
| device_set_desc(dev, "Synopsys Designware DWC3"); | device_set_desc(dev, "Synopsys Designware DWC3"); | ||||
| return (BUS_PROBE_DEFAULT); | return (BUS_PROBE_DEFAULT); | ||||
| } | } | ||||
| static int | static int | ||||
| snps_dwc3_attach(device_t dev) | snps_dwc3_attach(device_t dev) | ||||
Done Inline ActionsDo you think the string breaking can be avoided here? mw: Do you think the string breaking can be avoided here? | |||||
| { | { | ||||
| struct snps_dwc3_softc *sc; | struct snps_dwc3_softc *sc; | ||||
| int rid = 0; | int rid = 0; | ||||
| sc = device_get_softc(dev); | sc = device_get_softc(dev); | ||||
| sc->dev = dev; | sc->dev = dev; | ||||
| sc->mem_res = bus_alloc_resource_any(dev, SYS_RES_MEMORY, &rid, | sc->mem_res = bus_alloc_resource_any(dev, SYS_RES_MEMORY, &rid, | ||||
| RF_ACTIVE); | RF_ACTIVE); | ||||
| if (sc->mem_res == NULL) { | if (sc->mem_res == NULL) { | ||||
| device_printf(dev, "Failed to map memory\n"); | device_printf(dev, "Failed to map memory\n"); | ||||
| return (ENXIO); | return (ENXIO); | ||||
| } | } | ||||
| sc->bst = rman_get_bustag(sc->mem_res); | sc->bst = rman_get_bustag(sc->mem_res); | ||||
| sc->bsh = rman_get_bushandle(sc->mem_res); | sc->bsh = rman_get_bushandle(sc->mem_res); | ||||
| if (bootverbose) | if (bootverbose) | ||||
| device_printf(dev, "snps id: %x\n", DWC3_READ(sc, DWC3_GSNPSID)); | device_printf(dev, "snps id: %x\n", DWC3_READ(sc, DWC3_GSNPSID)); | ||||
| /* Get the phys */ | /* Get the phys */ | ||||
| sc->node = ofw_bus_get_node(dev); | |||||
| phy_get_by_ofw_name(dev, sc->node, "usb2-phy", &sc->usb2_phy); | phy_get_by_ofw_name(dev, sc->node, "usb2-phy", &sc->usb2_phy); | ||||
| phy_get_by_ofw_name(dev, sc->node, "usb3-phy", &sc->usb3_phy); | phy_get_by_ofw_name(dev, sc->node, "usb3-phy", &sc->usb3_phy); | ||||
| snps_dwc3_reset(sc); | snps_dwc3_reset(sc); | ||||
| snps_dwc3_configure_host(sc); | snps_dwc3_configure_host(sc); | ||||
| snps_dwc3_configure_phy(sc); | snps_dwc3_configure_phy(sc); | ||||
| snps_dwc3_do_quirks(sc); | snps_dwc3_do_quirks(sc); | ||||
| #if 0 | #if 0 | ||||
| Show All 24 Lines | |||||
Can someone please test this with FDT?
ofwdump is fine on the running system but the device_get_property here returns garbage and no error on a nanopi-m4v2 that I resurrected using an overlay to set dr_mode to host. I might debug it some more tomorrow if needed (but I don't trust that nanopi much as it has other memory issues).