Page MenuHomeFreeBSD
Differential D33170 Diff 107476 sys/dev/usb/controller/dwc3.c

Changeset View

Standalone View

View Options

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),
bzAuthorUnsubmitted
Done
Inline Actions

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).

bz: Can someone please test this with FDT? ofwdump is fine on the running system but the…
kdUnsubmitted
Not Done
Inline Actions

With 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 Actions

Thanks. 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
xhci0: Found dr_mode 'tsoh' but only 'host' supported. s=5
xhci0: Found dr_mode 'tsoh' but only 'host' supported. s=5
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 Actions

device_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

device_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.

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 Actions

I'd suggest opening a review and make this a parent review so we can continue here once that is sorted?
PS: I am not sure I'd want an endless amount of BUS functions for all kinds of types. Maybe we can have wrappers in subr_bus and pass a type along to the ... I'll leave that for there.

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); DEVICE_PROP_BUFFER);
OF_getprop(sc->node, "dr_mode", sc->dr_mode, sizeof(sc->dr_mode)); if (s == -1) {
if (strcmp(sc->dr_mode, "host") != 0) { device_printf(dev, "Cannot determine dr_mode\n");
device_printf(dev, "Only host mode is supported\n");
return (ENXIO); return (ENXIO);
} }
if (strcmp(dr_mode, "host") != 0) {
device_printf(dev,
"Found dr_mode '%s' but only 'host' supported. s=%zd\n",
dr_mode, s);
mwUnsubmitted
Done
Inline Actions

Do 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_…
bzAuthorUnsubmitted
Done
Inline Actions

Getting 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…
mwUnsubmitted
Not Done
Inline Actions

Thanks, 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:
char dr_mode[16] = {0};
and check what happens with DT and ACPI?

mw: Thanks, this is what I suspected - we obtain raw buffer from ACPI. I thought about it and in…
mwUnsubmitted
Done
Inline Actions

Do you think the string breaking can be avoided here?

mw: Do you think the string breaking can be avoided here?
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)
{ {
Show All 11 Linessnps_dwc3_attach(device_t dev)
} }
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 23 Lines