| Message ID | 20230310163420.7582-1-quic_kriskura@quicinc.com |
|---|---|
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
On Fri, Mar 10, 2023, Krishna Kurapati wrote: > Currently host-only capable DWC3 controllers support Multiport. Temporarily > map XHCI address space for host-only controllers and parse XHCI Extended > Capabilities registers to read number of physical usb ports connected to the > multiport controller (presuming each port is at least HS capable) and extract > info on how many of these ports are Super Speed capable. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 75 +++++++++++++++++++++++++++++++++++++++++ > drivers/usb/dwc3/core.h | 9 +++++ > 2 files changed, 84 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 476b63618511..076c0f8a4441 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -37,6 +37,7 @@ > #include "core.h" > #include "gadget.h" > #include "io.h" > +#include "../host/xhci.h" I think better to duplicate some of the logic in dwc3 driver and avoid any direct dependency with the xhci driver. > > #include "debug.h" > > @@ -1750,6 +1751,65 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc) > return edev; > } > > +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res) > +{ > + void __iomem *regs; > + struct resource dwc_res; > + u32 offset; > + u32 temp; > + u8 major_revision; > + int ret = 0; > + > + /* > + * Remap xHCI address space to access XHCI ext cap regs, > + * since it is needed to get port info. > + */ > + dwc_res = *res; > + dwc_res.start += 0; > + dwc_res.end = dwc->xhci_resources[0].start + > + DWC3_XHCI_REGS_END; Isn't dwc->xhci_resources[0] already setup at this point? Can we use dwc->xhci_resources[0] directly without copy the setting in dwc_res? > + > + regs = ioremap(dwc_res.start, resource_size(&dwc_res)); > + if (IS_ERR(regs)) > + return PTR_ERR(regs); > + > + offset = xhci_find_next_ext_cap(regs, 0, > + XHCI_EXT_CAPS_PROTOCOL); > + while (offset) { > + temp = readl(regs + offset); > + major_revision = XHCI_EXT_PORT_MAJOR(temp); > + > + temp = readl(regs + offset + 0x08); > + if (major_revision == 0x03) { > + dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp); > + } else if (major_revision <= 0x02) { > + dwc->num_ports += XHCI_EXT_PORT_COUNT(temp); > + } else { > + dev_err(dwc->dev, "port revision seems wrong\n"); > + ret = -EINVAL; > + goto unmap_reg; > + } > + > + offset = xhci_find_next_ext_cap(regs, offset, > + XHCI_EXT_CAPS_PROTOCOL); > + } > + > + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); > + if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) { > + dev_err(dwc->dev, "inconsistency in port info\n"); > + ret = -EINVAL; > + goto unmap_reg; > + } > + > + dev_info(dwc->dev, > + "num-ports: %d ss-capable: %d\n", dwc->num_ports, dwc->num_ss_ports); The end user doesn't need to know this info. This should be a debug message. Perhaps it can be a tracepoint if needed? > + > +unmap_reg: > + iounmap(regs); > + return ret; > +} > + > static int dwc3_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1757,6 +1817,7 @@ static int dwc3_probe(struct platform_device *pdev) > struct dwc3 *dwc; > > int ret; > + unsigned int hw_mode; > > void __iomem *regs; > > @@ -1880,6 +1941,20 @@ static int dwc3_probe(struct platform_device *pdev) > goto disable_clks; > } > > + /* > + * Currently DWC3 controllers that are host-only capable > + * support Multiport. > + */ > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > + ret = dwc3_read_port_info(dwc, res); > + if (ret) > + goto disable_clks; > + } else { > + dwc->num_ports = 1; > + dwc->num_ss_ports = 1; > + } > + > spin_lock_init(&dwc->lock); > mutex_init(&dwc->mutex); > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 582ebd9cf9c2..74386d6a0277 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -35,6 +35,9 @@ > > #define DWC3_MSG_MAX 500 > > +/* XHCI Reg constants */ > +#define DWC3_XHCI_HCSPARAMS1 0x04 > + > /* Global constants */ > #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ > #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ > @@ -1023,6 +1026,10 @@ struct dwc3_scratchpad_array { > * @usb_psy: pointer to power supply interface. > * @usb2_phy: pointer to USB2 PHY > * @usb3_phy: pointer to USB3 PHY > + * @num_ports: Indicates the number of physical USB ports present on HW > + * presuming each port is at least HS capable This isn't the number of physical USB ports right? That's the number of usb2 ports the controller is configured with right?. Perhaps we can use num_usb2_ports and num_usb3_ports? > + * @num_ss_ports: Indicates the number of USB ports present on HW that are > + * SS Capable > * @usb2_generic_phy: pointer to USB2 PHY > * @usb3_generic_phy: pointer to USB3 PHY > * @phys_ready: flag to indicate that PHYs are ready > @@ -1158,6 +1165,8 @@ struct dwc3 { > struct usb_phy *usb2_phy; > struct usb_phy *usb3_phy; > > + u32 num_ports; > + u32 num_ss_ports; > struct phy *usb2_generic_phy; > struct phy *usb3_generic_phy; > > -- > 2.39.0 > Thanks, Thinh
On 3/11/2023 5:25 AM, Thinh Nguyen wrote: > On Fri, Mar 10, 2023, Krishna Kurapati wrote: >> Currently host-only capable DWC3 controllers support Multiport. Temporarily >> map XHCI address space for host-only controllers and parse XHCI Extended >> Capabilities registers to read number of physical usb ports connected to the >> multiport controller (presuming each port is at least HS capable) and extract >> info on how many of these ports are Super Speed capable. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 75 +++++++++++++++++++++++++++++++++++++++++ >> drivers/usb/dwc3/core.h | 9 +++++ >> 2 files changed, 84 insertions(+) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 476b63618511..076c0f8a4441 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -37,6 +37,7 @@ >> #include "core.h" >> #include "gadget.h" >> #include "io.h" >> +#include "../host/xhci.h" > > I think better to duplicate some of the logic in dwc3 driver and avoid > any direct dependency with the xhci driver. > >> >> #include "debug.h" >> >> @@ -1750,6 +1751,65 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc) >> return edev; >> } >> >> +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res) >> +{ >> + void __iomem *regs; >> + struct resource dwc_res; >> + u32 offset; >> + u32 temp; >> + u8 major_revision; >> + int ret = 0; >> + >> + /* >> + * Remap xHCI address space to access XHCI ext cap regs, >> + * since it is needed to get port info. >> + */ >> + dwc_res = *res; >> + dwc_res.start += 0; >> + dwc_res.end = dwc->xhci_resources[0].start + >> + DWC3_XHCI_REGS_END; > > Isn't dwc->xhci_resources[0] already setup at this point? Can we use > dwc->xhci_resources[0] directly without copy the setting in dwc_res? > >> + >> + regs = ioremap(dwc_res.start, resource_size(&dwc_res)); >> + if (IS_ERR(regs)) >> + return PTR_ERR(regs); >> + >> + offset = xhci_find_next_ext_cap(regs, 0, >> + XHCI_EXT_CAPS_PROTOCOL); >> + while (offset) { >> + temp = readl(regs + offset); >> + major_revision = XHCI_EXT_PORT_MAJOR(temp); >> + >> + temp = readl(regs + offset + 0x08); >> + if (major_revision == 0x03) { >> + dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp); >> + } else if (major_revision <= 0x02) { >> + dwc->num_ports += XHCI_EXT_PORT_COUNT(temp); >> + } else { >> + dev_err(dwc->dev, "port revision seems wrong\n"); >> + ret = -EINVAL; >> + goto unmap_reg; >> + } >> + >> + offset = xhci_find_next_ext_cap(regs, offset, >> + XHCI_EXT_CAPS_PROTOCOL); >> + } >> + >> + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); >> + if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) { >> + dev_err(dwc->dev, "inconsistency in port info\n"); >> + ret = -EINVAL; >> + goto unmap_reg; >> + } >> + >> + dev_info(dwc->dev, >> + "num-ports: %d ss-capable: %d\n", dwc->num_ports, dwc->num_ss_ports); > > The end user doesn't need to know this info. This should be a debug > message. Perhaps it can be a tracepoint if needed? > >> + >> +unmap_reg: >> + iounmap(regs); >> + return ret; >> +} >> + >> static int dwc3_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -1757,6 +1817,7 @@ static int dwc3_probe(struct platform_device *pdev) >> struct dwc3 *dwc; >> >> int ret; >> + unsigned int hw_mode; >> >> void __iomem *regs; >> >> @@ -1880,6 +1941,20 @@ static int dwc3_probe(struct platform_device *pdev) >> goto disable_clks; >> } >> >> + /* >> + * Currently DWC3 controllers that are host-only capable >> + * support Multiport. >> + */ >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { >> + ret = dwc3_read_port_info(dwc, res); >> + if (ret) >> + goto disable_clks; >> + } else { >> + dwc->num_ports = 1; >> + dwc->num_ss_ports = 1; >> + } >> + >> spin_lock_init(&dwc->lock); >> mutex_init(&dwc->mutex); >> >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 582ebd9cf9c2..74386d6a0277 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -35,6 +35,9 @@ >> >> #define DWC3_MSG_MAX 500 >> >> +/* XHCI Reg constants */ >> +#define DWC3_XHCI_HCSPARAMS1 0x04 >> + >> /* Global constants */ >> #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ >> #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ >> @@ -1023,6 +1026,10 @@ struct dwc3_scratchpad_array { >> * @usb_psy: pointer to power supply interface. >> * @usb2_phy: pointer to USB2 PHY >> * @usb3_phy: pointer to USB3 PHY >> + * @num_ports: Indicates the number of physical USB ports present on HW >> + * presuming each port is at least HS capable > > This isn't the number of physical USB ports right? That's the number of > usb2 ports the controller is configured with right?. Perhaps we can use > num_usb2_ports and num_usb3_ports? > Hi Thinh, Yes, naming this might have created a little confusion. num_ports is supposed to indicate number of usb2 ports in the controller. Incase of sa8295 (4 port controller with first two ports having ss capability), num_ports would be 4 and num_ss_ports would be 2. (and not 6 as what num_ports usually sounds). I can rename them accordingly in the next version and update the description as well. Regards, Krishna, >> + * @num_ss_ports: Indicates the number of USB ports present on HW that are >> + * SS Capable >> * @usb2_generic_phy: pointer to USB2 PHY >> * @usb3_generic_phy: pointer to USB3 PHY >> * @phys_ready: flag to indicate that PHYs are ready >> @@ -1158,6 +1165,8 @@ struct dwc3 { >> struct usb_phy *usb2_phy; >> struct usb_phy *usb3_phy; >> >> + u32 num_ports; >> + u32 num_ss_ports; >> struct phy *usb2_generic_phy; >> struct phy *usb3_generic_phy; >> >> -- >> 2.39.0 >> > > Thanks, > Thinh
On 3/11/2023 8:24 AM, Krishna Kurapati PSSNV wrote: > > > On 3/11/2023 5:25 AM, Thinh Nguyen wrote: >> On Fri, Mar 10, 2023, Krishna Kurapati wrote: >>> Currently host-only capable DWC3 controllers support Multiport. >>> Temporarily >>> map XHCI address space for host-only controllers and parse XHCI Extended >>> Capabilities registers to read number of physical usb ports connected >>> to the >>> multiport controller (presuming each port is at least HS capable) and >>> extract >>> info on how many of these ports are Super Speed capable. >>> >>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>> --- >>> drivers/usb/dwc3/core.c | 75 +++++++++++++++++++++++++++++++++++++++++ >>> drivers/usb/dwc3/core.h | 9 +++++ >>> 2 files changed, 84 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>> index 476b63618511..076c0f8a4441 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -37,6 +37,7 @@ >>> #include "core.h" >>> #include "gadget.h" >>> #include "io.h" >>> +#include "../host/xhci.h" >> >> I think better to duplicate some of the logic in dwc3 driver and avoid >> any direct dependency with the xhci driver. >> >>> #include "debug.h" >>> @@ -1750,6 +1751,65 @@ static struct extcon_dev >>> *dwc3_get_extcon(struct dwc3 *dwc) >>> return edev; >>> } >>> +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res) >>> +{ >>> + void __iomem *regs; >>> + struct resource dwc_res; >>> + u32 offset; >>> + u32 temp; >>> + u8 major_revision; >>> + int ret = 0; >>> + >>> + /* >>> + * Remap xHCI address space to access XHCI ext cap regs, >>> + * since it is needed to get port info. >>> + */ >>> + dwc_res = *res; >>> + dwc_res.start += 0; >>> + dwc_res.end = dwc->xhci_resources[0].start + >>> + DWC3_XHCI_REGS_END; >> >> Isn't dwc->xhci_resources[0] already setup at this point? Can we use >> dwc->xhci_resources[0] directly without copy the setting in dwc_res? >> >>> + >>> + regs = ioremap(dwc_res.start, resource_size(&dwc_res)); >>> + if (IS_ERR(regs)) >>> + return PTR_ERR(regs); >>> + >>> + offset = xhci_find_next_ext_cap(regs, 0, >>> + XHCI_EXT_CAPS_PROTOCOL); >>> + while (offset) { >>> + temp = readl(regs + offset); >>> + major_revision = XHCI_EXT_PORT_MAJOR(temp); >>> + >>> + temp = readl(regs + offset + 0x08); >>> + if (major_revision == 0x03) { >>> + dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp); >>> + } else if (major_revision <= 0x02) { >>> + dwc->num_ports += XHCI_EXT_PORT_COUNT(temp); >>> + } else { >>> + dev_err(dwc->dev, "port revision seems wrong\n"); >>> + ret = -EINVAL; >>> + goto unmap_reg; >>> + } >>> + >>> + offset = xhci_find_next_ext_cap(regs, offset, >>> + XHCI_EXT_CAPS_PROTOCOL); >>> + } >>> + >>> + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); >>> + if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) { >>> + dev_err(dwc->dev, "inconsistency in port info\n"); >>> + ret = -EINVAL; >>> + goto unmap_reg; >>> + } >>> + >>> + dev_info(dwc->dev, >>> + "num-ports: %d ss-capable: %d\n", dwc->num_ports, >>> dwc->num_ss_ports); >> >> The end user doesn't need to know this info. This should be a debug >> message. Perhaps it can be a tracepoint if needed? >> >>> + >>> +unmap_reg: >>> + iounmap(regs); >>> + return ret; >>> +} >>> + >>> static int dwc3_probe(struct platform_device *pdev) >>> { >>> struct device *dev = &pdev->dev; >>> @@ -1757,6 +1817,7 @@ static int dwc3_probe(struct platform_device >>> *pdev) >>> struct dwc3 *dwc; >>> int ret; >>> + unsigned int hw_mode; >>> void __iomem *regs; >>> @@ -1880,6 +1941,20 @@ static int dwc3_probe(struct platform_device >>> *pdev) >>> goto disable_clks; >>> } >>> + /* >>> + * Currently DWC3 controllers that are host-only capable >>> + * support Multiport. >>> + */ >>> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >>> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { >>> + ret = dwc3_read_port_info(dwc, res); >>> + if (ret) >>> + goto disable_clks; >>> + } else { >>> + dwc->num_ports = 1; >>> + dwc->num_ss_ports = 1; >>> + } >>> + >>> spin_lock_init(&dwc->lock); >>> mutex_init(&dwc->mutex); >>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>> index 582ebd9cf9c2..74386d6a0277 100644 >>> --- a/drivers/usb/dwc3/core.h >>> +++ b/drivers/usb/dwc3/core.h >>> @@ -35,6 +35,9 @@ >>> #define DWC3_MSG_MAX 500 >>> +/* XHCI Reg constants */ >>> +#define DWC3_XHCI_HCSPARAMS1 0x04 >>> + >>> /* Global constants */ >>> #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ >>> #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ >>> @@ -1023,6 +1026,10 @@ struct dwc3_scratchpad_array { >>> * @usb_psy: pointer to power supply interface. >>> * @usb2_phy: pointer to USB2 PHY >>> * @usb3_phy: pointer to USB3 PHY >>> + * @num_ports: Indicates the number of physical USB ports present on HW >>> + * presuming each port is at least HS capable >> >> This isn't the number of physical USB ports right? That's the number of >> usb2 ports the controller is configured with right?. Perhaps we can use >> num_usb2_ports and num_usb3_ports? >> > Hi Thinh, > > Yes, naming this might have created a little confusion. > num_ports is supposed to indicate number of usb2 ports in the controller. > > Incase of sa8295 (4 port controller with first two ports having ss > capability), num_ports would be 4 and num_ss_ports would be 2. (and not > 6 as what num_ports usually sounds). > I can rename them accordingly in the next version and update the > description as well. > > Regards, > Krishna, > Hi Thinh, One reason I didn't mention something like num_hs_ports and sticked to num_ports is because in core driver, wherever we need to do phy operations like: for (i = 0; i < num_ports; i++) { phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); } The intention is as follows: If number of usb2 ports is 4, the loop can go from 0-3 and its fine. If number of usb3-ports is 2, we don't know for sure, if the first 2 ports are SS capable or some other ports like (3 and 4) are SS capable. So instead, I looped all phy operations around all usb2_generic_phy's and usb3_generic_phy's. If they are NULL, we just bail out inside phy operation. While doing so, looping SS Phy operations around num_usb2_ports didn't sound good. From code view, it would be like we are looping usb3_phy ops around num_usb2_ports value (logically it is still correct as each port is atleast HS capable). So to avoid this, I named the variable as num_ports instead of num_usb2_ports Regards, Krishna, >>> + * @num_ss_ports: Indicates the number of USB ports present on HW >>> that are >>> + * SS Capable >>> * @usb2_generic_phy: pointer to USB2 PHY >>> * @usb3_generic_phy: pointer to USB3 PHY >>> * @phys_ready: flag to indicate that PHYs are ready >>> @@ -1158,6 +1165,8 @@ struct dwc3 { >>> struct usb_phy *usb2_phy; >>> struct usb_phy *usb3_phy; >>> + u32 num_ports; >>> + u32 num_ss_ports; >>> struct phy *usb2_generic_phy; >>> struct phy *usb3_generic_phy; >>> -- >>> 2.39.0 >>> >> >> Thanks, >> Thinh
Hello! On 3/10/23 7:34 PM, Krishna Kurapati wrote: > Enable teritiary controller for SA8295P (based on SC8280XP). Tertiary? > Add pinctrl support for usb ports to provide VBUS to connected peripherals. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> [...] MBR, Sergey
On 3/10/23 7:34 PM, Krishna Kurapati wrote: > On some SoC's like SA8295P where the teritiary controller is host-only Likewise, tertiary. :-) > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. > Trying to setup them up during core_init leads to a crash. > > For DRD/Peripheral supported controllers, event buffer setup is done > again in gadget_pullup. Skip setup or cleanup of event buffers if > controller is host-only capable. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> [...] MBR, Sergey
On Sat, Mar 11, 2023 at 12:40 AM Krishna Kurapati <quic_kriskura@quicinc.com> wrote: > > On some SoC's like SA8295P where the teritiary controller is host-only > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. > Trying to setup them up during core_init leads to a crash. > > For DRD/Peripheral supported controllers, event buffer setup is done > again in gadget_pullup. Skip setup or cleanup of event buffers if > controller is host-only capable. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 076c0f8a4441..1ca9fa40a66e 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -840,7 +840,11 @@ static void dwc3_clk_disable(struct dwc3 *dwc) > > static void dwc3_core_exit(struct dwc3 *dwc) > { > - dwc3_event_buffers_cleanup(dwc); > + unsigned int hw_mode; > + > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) > + dwc3_event_buffers_cleanup(dwc); quick question about dwc3_event_buffers_cleanup, there are other similar sites calling this function. C symbol: dwc3_event_buffers_cleanup File Function Line 0 core.h <global> 1546 void dwc3_event_buffers_cleanup(struct dwc3 *dwc); 1 core.c __dwc3_set_mode 152 dwc3_event_buffers_cleanup(dwc); 2 core.c dwc3_event_buffers_cleanup 522 void dwc3_event_buffers_cleanup(struct dwc3 *dwc) 3 core.c dwc3_core_exit 842 dwc3_event_buffers_cleanup(dwc); 4 core.c dwc3_probe 1936 dwc3_event_buffers_cleanup(dwc); 5 drd.c dwc3_otg_update 363 dwc3_event_buffers_cleanup(dwc); 6 drd.c dwc3_drd_exit 607 dwc3_event_buffers_cleanup(dwc); For 1, 5, and 6, any need to take care of this situation? > > usb_phy_set_suspend(dwc->usb2_phy, 1); > usb_phy_set_suspend(dwc->usb3_phy, 1); > @@ -1177,10 +1181,12 @@ static int dwc3_core_init(struct dwc3 *dwc) > if (ret < 0) > goto err3; > > - ret = dwc3_event_buffers_setup(dwc); > - if (ret) { > - dev_err(dwc->dev, "failed to setup event buffers\n"); > - goto err4; > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) { > + ret = dwc3_event_buffers_setup(dwc); > + if (ret) { > + dev_err(dwc->dev, "failed to setup event buffers\n"); > + goto err4; > + } > } > > /* > @@ -2008,7 +2014,9 @@ static int dwc3_probe(struct platform_device *pdev) > > err5: > dwc3_debugfs_exit(dwc); > - dwc3_event_buffers_cleanup(dwc); > + > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) > + dwc3_event_buffers_cleanup(dwc); > > usb_phy_set_suspend(dwc->usb2_phy, 1); > usb_phy_set_suspend(dwc->usb3_phy, 1); > -- > 2.39.0 >
On 3/13/2023 7:37 AM, Dongliang Mu wrote: > On Sat, Mar 11, 2023 at 12:40 AM Krishna Kurapati > <quic_kriskura@quicinc.com> wrote: >> >> On some SoC's like SA8295P where the teritiary controller is host-only >> capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. >> Trying to setup them up during core_init leads to a crash. >> >> For DRD/Peripheral supported controllers, event buffer setup is done >> again in gadget_pullup. Skip setup or cleanup of event buffers if >> controller is host-only capable. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 20 ++++++++++++++------ >> 1 file changed, 14 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 076c0f8a4441..1ca9fa40a66e 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -840,7 +840,11 @@ static void dwc3_clk_disable(struct dwc3 *dwc) >> >> static void dwc3_core_exit(struct dwc3 *dwc) >> { >> - dwc3_event_buffers_cleanup(dwc); >> + unsigned int hw_mode; >> + >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) >> + dwc3_event_buffers_cleanup(dwc); > > quick question about dwc3_event_buffers_cleanup, there are other > similar sites calling this function. > > C symbol: dwc3_event_buffers_cleanup > > File Function Line > 0 core.h <global> 1546 void > dwc3_event_buffers_cleanup(struct dwc3 *dwc); > 1 core.c __dwc3_set_mode 152 dwc3_event_buffers_cleanup(dwc); > 2 core.c dwc3_event_buffers_cleanup 522 void > dwc3_event_buffers_cleanup(struct dwc3 *dwc) > 3 core.c dwc3_core_exit 842 dwc3_event_buffers_cleanup(dwc); > 4 core.c dwc3_probe 1936 dwc3_event_buffers_cleanup(dwc); > 5 drd.c dwc3_otg_update 363 dwc3_event_buffers_cleanup(dwc); > 6 drd.c dwc3_drd_exit 607 dwc3_event_buffers_cleanup(dwc); > > For 1, 5, and 6, any need to take care of this situation? > Hi Dongliang, Thanks for the review. In the other places mentioned like set_mode otg_update or drd_exit, cleanup is called if we are in device mode and we want to exit that mode. Since for MP, we have a host only controller those paths won't be accessed I believe. Regards, Krishna, >> >> usb_phy_set_suspend(dwc->usb2_phy, 1); >> usb_phy_set_suspend(dwc->usb3_phy, 1); >> @@ -1177,10 +1181,12 @@ static int dwc3_core_init(struct dwc3 *dwc) >> if (ret < 0) >> goto err3; >> >> - ret = dwc3_event_buffers_setup(dwc); >> - if (ret) { >> - dev_err(dwc->dev, "failed to setup event buffers\n"); >> - goto err4; >> + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) { >> + ret = dwc3_event_buffers_setup(dwc); >> + if (ret) { >> + dev_err(dwc->dev, "failed to setup event buffers\n"); >> + goto err4; >> + } >> } >> >> /* >> @@ -2008,7 +2014,9 @@ static int dwc3_probe(struct platform_device *pdev) >> >> err5: >> dwc3_debugfs_exit(dwc); >> - dwc3_event_buffers_cleanup(dwc); >> + >> + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) >> + dwc3_event_buffers_cleanup(dwc); >> >> usb_phy_set_suspend(dwc->usb2_phy, 1); >> usb_phy_set_suspend(dwc->usb3_phy, 1); >> -- >> 2.39.0 >>
On Sat, Mar 11, 2023, Krishna Kurapati PSSNV wrote: > > > On 3/11/2023 8:24 AM, Krishna Kurapati PSSNV wrote: > > > > > > On 3/11/2023 5:25 AM, Thinh Nguyen wrote: > > > On Fri, Mar 10, 2023, Krishna Kurapati wrote: > > > > Currently host-only capable DWC3 controllers support Multiport. > > > > Temporarily > > > > map XHCI address space for host-only controllers and parse XHCI Extended > > > > Capabilities registers to read number of physical usb ports > > > > connected to the > > > > multiport controller (presuming each port is at least HS > > > > capable) and extract > > > > info on how many of these ports are Super Speed capable. > > > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > --- > > > > drivers/usb/dwc3/core.c | 75 +++++++++++++++++++++++++++++++++++++++++ > > > > drivers/usb/dwc3/core.h | 9 +++++ > > > > 2 files changed, 84 insertions(+) > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > index 476b63618511..076c0f8a4441 100644 > > > > --- a/drivers/usb/dwc3/core.c > > > > +++ b/drivers/usb/dwc3/core.c > > > > @@ -37,6 +37,7 @@ > > > > #include "core.h" > > > > #include "gadget.h" > > > > #include "io.h" > > > > +#include "../host/xhci.h" > > > > > > I think better to duplicate some of the logic in dwc3 driver and avoid > > > any direct dependency with the xhci driver. > > > > > > > #include "debug.h" > > > > @@ -1750,6 +1751,65 @@ static struct extcon_dev > > > > *dwc3_get_extcon(struct dwc3 *dwc) > > > > return edev; > > > > } > > > > +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res) > > > > +{ > > > > + void __iomem *regs; > > > > + struct resource dwc_res; > > > > + u32 offset; > > > > + u32 temp; > > > > + u8 major_revision; > > > > + int ret = 0; > > > > + > > > > + /* > > > > + * Remap xHCI address space to access XHCI ext cap regs, > > > > + * since it is needed to get port info. > > > > + */ > > > > + dwc_res = *res; > > > > + dwc_res.start += 0; > > > > + dwc_res.end = dwc->xhci_resources[0].start + > > > > + DWC3_XHCI_REGS_END; > > > > > > Isn't dwc->xhci_resources[0] already setup at this point? Can we use > > > dwc->xhci_resources[0] directly without copy the setting in dwc_res? > > > > > > > + > > > > + regs = ioremap(dwc_res.start, resource_size(&dwc_res)); > > > > + if (IS_ERR(regs)) > > > > + return PTR_ERR(regs); > > > > + > > > > + offset = xhci_find_next_ext_cap(regs, 0, > > > > + XHCI_EXT_CAPS_PROTOCOL); > > > > + while (offset) { > > > > + temp = readl(regs + offset); > > > > + major_revision = XHCI_EXT_PORT_MAJOR(temp); > > > > + > > > > + temp = readl(regs + offset + 0x08); > > > > + if (major_revision == 0x03) { > > > > + dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp); > > > > + } else if (major_revision <= 0x02) { > > > > + dwc->num_ports += XHCI_EXT_PORT_COUNT(temp); > > > > + } else { > > > > + dev_err(dwc->dev, "port revision seems wrong\n"); > > > > + ret = -EINVAL; > > > > + goto unmap_reg; > > > > + } > > > > + > > > > + offset = xhci_find_next_ext_cap(regs, offset, > > > > + XHCI_EXT_CAPS_PROTOCOL); > > > > + } > > > > + > > > > + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); > > > > + if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) { > > > > + dev_err(dwc->dev, "inconsistency in port info\n"); > > > > + ret = -EINVAL; > > > > + goto unmap_reg; > > > > + } > > > > + > > > > + dev_info(dwc->dev, > > > > + "num-ports: %d ss-capable: %d\n", dwc->num_ports, > > > > dwc->num_ss_ports); > > > > > > The end user doesn't need to know this info. This should be a debug > > > message. Perhaps it can be a tracepoint if needed? > > > > > > > + > > > > +unmap_reg: > > > > + iounmap(regs); > > > > + return ret; > > > > +} > > > > + > > > > static int dwc3_probe(struct platform_device *pdev) > > > > { > > > > struct device *dev = &pdev->dev; > > > > @@ -1757,6 +1817,7 @@ static int dwc3_probe(struct > > > > platform_device *pdev) > > > > struct dwc3 *dwc; > > > > int ret; > > > > + unsigned int hw_mode; > > > > void __iomem *regs; > > > > @@ -1880,6 +1941,20 @@ static int dwc3_probe(struct > > > > platform_device *pdev) > > > > goto disable_clks; > > > > } > > > > + /* > > > > + * Currently DWC3 controllers that are host-only capable > > > > + * support Multiport. > > > > + */ > > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > > > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > > > > + ret = dwc3_read_port_info(dwc, res); > > > > + if (ret) > > > > + goto disable_clks; > > > > + } else { > > > > + dwc->num_ports = 1; > > > > + dwc->num_ss_ports = 1; > > > > + } > > > > + > > > > spin_lock_init(&dwc->lock); > > > > mutex_init(&dwc->mutex); > > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > > > index 582ebd9cf9c2..74386d6a0277 100644 > > > > --- a/drivers/usb/dwc3/core.h > > > > +++ b/drivers/usb/dwc3/core.h > > > > @@ -35,6 +35,9 @@ > > > > #define DWC3_MSG_MAX 500 > > > > +/* XHCI Reg constants */ > > > > +#define DWC3_XHCI_HCSPARAMS1 0x04 > > > > + > > > > /* Global constants */ > > > > #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ > > > > #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ > > > > @@ -1023,6 +1026,10 @@ struct dwc3_scratchpad_array { > > > > * @usb_psy: pointer to power supply interface. > > > > * @usb2_phy: pointer to USB2 PHY > > > > * @usb3_phy: pointer to USB3 PHY > > > > + * @num_ports: Indicates the number of physical USB ports present on HW > > > > + * presuming each port is at least HS capable > > > > > > This isn't the number of physical USB ports right? That's the number of > > > usb2 ports the controller is configured with right?. Perhaps we can use > > > num_usb2_ports and num_usb3_ports? > > > > > Hi Thinh, > > > > Yes, naming this might have created a little confusion. > > num_ports is supposed to indicate number of usb2 ports in the controller. > > > > Incase of sa8295 (4 port controller with first two ports having ss > > capability), num_ports would be 4 and num_ss_ports would be 2. (and not > > 6 as what num_ports usually sounds). > > I can rename them accordingly in the next version and update the > > description as well. > > > > Regards, > > Krishna, > > > Hi Thinh, > > One reason I didn't mention something like num_hs_ports and sticked to > num_ports is because in core driver, wherever we need to do phy operations > like: > > for (i = 0; i < num_ports; i++) > { > phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); > phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); > } > > The intention is as follows: > If number of usb2 ports is 4, the loop can go from 0-3 and its fine. > If number of usb3-ports is 2, we don't know for sure, if the first 2 ports > are SS capable or some other ports like (3 and 4) are SS capable. > So instead, I looped all phy operations around all usb2_generic_phy's and > usb3_generic_phy's. If they are NULL, we just bail out inside phy operation. > > While doing so, looping SS Phy operations around num_usb2_ports didn't sound > good. From code view, it would be like we are looping usb3_phy ops around > num_usb2_ports value (logically it is still correct as each port is atleast > HS capable). So to avoid this, I named the variable as num_ports instead of > num_usb2_ports > Hi Krishna, I think it's clearer if add this note along with using num_usb2_ports. We just need to note this once and I think it's sufficient. Thanks, Thinh
Hi Krishna, I'm unable to apply your patch series, it looks like patch 2 is malformed. 'git am' prints the following: Applying: dt-bindings: usb: Add bindings for multiport properties on DWC3 controller Applying: usb: dwc3: core: Access XHCI address space temporarily to read port info error: corrupt patch at line 83 Patch failed at 0002 usb: dwc3: core: Access XHCI address space temporarily to read port info Are you able to apply the series on your side? Best, Adrien
On 3/15/2023 2:02 AM, Adrien Thierry wrote: > Hi Krishna, > > I'm unable to apply your patch series, it looks like patch 2 is malformed. > 'git am' prints the following: > > Applying: dt-bindings: usb: Add bindings for multiport properties on DWC3 controller > Applying: usb: dwc3: core: Access XHCI address space temporarily to read port info > error: corrupt patch at line 83 > Patch failed at 0002 usb: dwc3: core: Access XHCI address space temporarily to read port info > > Are you able to apply the series on your side? > > Best, > > Adrien > Hi Adrien, I rebased them last week before sending them out. Probably code got updated causing conflicts. I will rebase them again this week and send v6 addressing review comments. Regards, Krishna,
Currently the DWC3 driver supports only single port controller which requires at most two PHYs ie HS and SS PHYs. There are SoCs that has DWC3 controller with multiple ports that can operate in host mode. Some of the port supports both SS+HS and other port supports only HS mode. This change primarily refactors the Phy logic in core driver to allow multiport support with Generic Phy's. Chananges have been tested on QCOM SoC SA8295P which has 4 ports (2 are HS+SS capable and 2 are HS only capable). Changes in current version: Added DT support for first port of Teritiary USB controller on SA8540-Ride Added support for reading port info from XHCI Extended Params registers. Changes in RFC v4: Added DT support for SA8295p. Changes in RFC v3: Incase any PHY init fails, then clear/exit the PHYs that are already initialized. Changes in RFC v2: Changed dwc3_count_phys to return the number of PHY Phandles in the node. This will be used now in dwc3_extract_num_phys to increment num_usb2_phy and num_usb3_phy. Added new parameter "ss_idx" in dwc3_core_get_phy_ny_node and changed its structure such that the first half is for HS-PHY and second half is for SS-PHY. In dwc3_core_get_phy, for multiport controller, only if SS-PHY phandle is present, pass proper SS_IDX else pass -1. Link to RFC v4: https://lore.kernel.org/all/20230115114146.12628-1-quic_kriskura@quicinc.com/ Link to RFC v3: https://lore.kernel.org/all/1654709787-23686-1-git-send-email-quic_harshq@quicinc.com/#r Link to RFC v2: https://lore.kernel.org/all/1653560029-6937-1-git-send-email-quic_harshq@quicinc.com/#r Krishna Kurapati (8): dt-bindings: usb: Add bindings for multiport properties on DWC3 controller usb: dwc3: core: Access XHCI address space temporarily to read port info usb: dwc3: core: Skip setting event buffers for host only controllers usb: dwc3: core: Refactor PHY logic to support Multiport Controller usb: dwc3: qcom: Add multiport controller support for qcom wrapper arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280 arm64: dts: qcom: sa8295p: Enable teritiary controller and its 4 USB ports arm64: dts: qcom: sa8540-ride: Enable first port of teritiary usb controller .../devicetree/bindings/usb/snps,dwc3.yaml | 13 +- arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 47 +++ arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 22 ++ arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 58 +++ drivers/usb/dwc3/core.c | 374 ++++++++++++++---- drivers/usb/dwc3/core.h | 20 +- drivers/usb/dwc3/drd.c | 13 +- drivers/usb/dwc3/dwc3-qcom.c | 28 +- 8 files changed, 473 insertions(+), 102 deletions(-)