| Message ID | 1672307866-25839-1-git-send-email-dh10.jung@samsung.com |
|---|---|
| Headers | show |
| Series | support Samsung Exynos xHCI Controller | expand |
On Thu, Dec 29, 2022 at 03:44:02PM +0100, Arnd Bergmann wrote: > On Thu, Dec 29, 2022, at 10:57, Daehwan Jung wrote: > > Currently, dwc3 invokes just xhci platform driver without any data. > > We add xhci node as child of dwc3 node in order to get data from > > device tree. It populates "xhci" child by name during initialization > > of host. This patch only effects if dwc3 node has a child named "xhci" > > not to disturb original path. > > Using child nodes is not the normal way of abstracting a soc specific > variant of a device, though there are some USB host drivers that > do this. Just use the node itself and add whatever samsung specific > properties are needed based on the compatible string. > I've tried to use existing platform drivers(dwc3, xhci-plat). Dwc3 has host mode and I think it's necessary to have child node. Currently we can't use compatible string but can just use xhci platform driver itself. That's why I modified to have a xhci child. It makes us to use specific properties. I don't find a better way even if it's not the normal way. I'm going to talk with other maintainers(dwc3, xhci-plat) to solve the problem. > > @@ -86,6 +90,33 @@ static void xhci_plat_quirks(struct device *dev, > > struct xhci_hcd *xhci) > > xhci->quirks |= XHCI_PLAT | priv->quirks; > > } > > > > +static int xhci_plat_bus_suspend(struct usb_hcd *hcd) > > +{ > > + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > + > > + if (xhci->quirks & XHCI_ROOTHUB_WAKEUP) { > > + if (hcd == xhci->main_hcd) > > + __pm_relax(xhci->main_wakelock); > > + else > > + __pm_relax(xhci->shared_wakelock); > > + } > > + > > + return xhci_bus_suspend(hcd); > > +} > > + > > +static int xhci_plat_bus_resume(struct usb_hcd *hcd) > > +{ > > + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > + > > + if (xhci->quirks & XHCI_ROOTHUB_WAKEUP) { > > + if (hcd == xhci->main_hcd) > > + __pm_stay_awake(xhci->main_wakelock); > > + else > > + __pm_stay_awake(xhci->shared_wakelock); > > + } > > + return xhci_bus_resume(hcd); > > +} > > It looks like these are no longer tied to the Samsung > device type, which would be a step in the right direction, > but I think adding this should be a separate patch since > it is not a hardware specific change but a new feature. > > Arnd > Thanks for the comment. I will separate and fix commit msg on next submission. Best Regards, Jung Deahwan
On 02/01/2023 07:24, Jung Daehwan wrote: > On Thu, Dec 29, 2022 at 11:25:58AM +0100, Krzysztof Kozlowski wrote: >> On 29/12/2022 10:57, Daehwan Jung wrote: >>> Currently, dwc3 invokes just xhci platform driver without any data. >>> We add xhci node as child of dwc3 node in order to get data from >>> device tree. It populates "xhci" child by name during initialization >>> of host. This patch only effects if dwc3 node has a child named "xhci" >>> not to disturb original path. >>> >>> We add "samsung,exynos-xhci" compatible in xhci platform driver >> >> Where? It is not documented. > > I submitted the patch of dt bindings on same patchset. > Is there any missing documentation? This is your first patch in the series and in this patch there is no such bindings. Re-order the patches to have proper order. > >> >>> to support Exynos SOCs. >> >> That's so not true. You do nothing to support Exynos SoC here. Please >> stop pasting incorrect and misleading commit msgs. > > I agree misleading commit msgs. I will modify it. > >> >>> We introduce roothub wakeup, which uses roothub >>> as system wakeup source. It needs xhci platform driver to override >>> roothub ops. >> >> You did not explain why you introduced wakelocks... >> > > I'm sorry I didn't write description enough. > I add it below. > >> >> (...) >> >>> if (shared_hcd) { >>> usb_remove_hcd(shared_hcd); >>> xhci->shared_hcd = NULL; >>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c >>> index 79d7931c048a..693495054001 100644 >>> --- a/drivers/usb/host/xhci.c >>> +++ b/drivers/usb/host/xhci.c >>> @@ -5502,6 +5502,10 @@ void xhci_init_driver(struct hc_driver *drv, >>> drv->check_bandwidth = over->check_bandwidth; >>> if (over->reset_bandwidth) >>> drv->reset_bandwidth = over->reset_bandwidth; >>> + if (over->bus_suspend) >>> + drv->bus_suspend = over->bus_suspend; >>> + if (over->bus_resume) >>> + drv->bus_resume = over->bus_resume; >>> } >>> } >>> EXPORT_SYMBOL_GPL(xhci_init_driver); >>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h >>> index c9f06c5e4e9d..cb9c54a6a22c 100644 >>> --- a/drivers/usb/host/xhci.h >>> +++ b/drivers/usb/host/xhci.h >>> @@ -1752,6 +1752,8 @@ struct xhci_hub { >>> struct xhci_hcd { >>> struct usb_hcd *main_hcd; >>> struct usb_hcd *shared_hcd; >>> + struct wakeup_source *main_wakelock; >>> + struct wakeup_source *shared_wakelock; >> >> Drop wakelocks. This is not related to USB and not needed here. Do you >> see anywhere else in core kernel code usage of the wakelocks? >> >> You got this comment already, didn't you? So why you do not address it? >> > > I want to add a new feature in xhci platform driver. I want to make it > possible to enter system sleep while usb host connected like USB Mouse. > It gets system enter sleep only if there's no usb transaction at all. > Deciding if there's tranaction or not is in root hub because it's parent > of all child usb devices. I have USB mouse connected to my system and the system enters suspend, thus I don't think this patch solves this particular issue. Best regards, Krzysztof
This patchset is to support xHCI Controller on Samsung Exynos SOCs. Thanks for all reviews on v1 and have tried to modify it well. I again added "RFC" because I haven't solved below problem when checking dt bindings. usb@4a000000: #size-cells:0:0: 0 was expected Documentation/devicetree/bindings/usb/snps,dwc3.yaml Changes in v2 : - Rename subject of cover letter ([RFC,v1,0/2] add xhci-exynos to support Samsung Exynos SOC) - Add Exynos compatible in xhci platform driver instead making new platform driver. - Make xhci platform driver as child of dwc3 with DT schema. - Override roothub ops for xhci platform driver. Daehwan Jung (3): usb: support Samsung Exynos xHCI Controller dt-bindings: usb: generic-xhci: add Samsung Exynos compatible dt-bindings: usb: snps,dwc3: add generic-xhci as child .../devicetree/bindings/usb/generic-xhci.yaml | 2 + .../devicetree/bindings/usb/snps,dwc3.yaml | 29 +++++++++ drivers/usb/dwc3/drd.c | 7 +++ drivers/usb/dwc3/host.c | 33 +++++++++- drivers/usb/host/xhci-plat.c | 60 ++++++++++++++++++- drivers/usb/host/xhci.c | 4 ++ drivers/usb/host/xhci.h | 5 ++ 7 files changed, 137 insertions(+), 3 deletions(-)