| Message ID | 1669860811-171746-3-git-send-email-dh10.jung@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v1,1/2] dt-bindings: usb: samsung,exynos-xhci: support Samsung Exynos xHCI Controller | expand |
On Thu, Dec 01, 2022 at 11:13:31AM +0900, Daehwan Jung wrote: > This driver works with xhci platform driver. It needs to override > functions of xhci_plat_hc_driver. Wakelocks are used for sleep/wakeup > scenario of system. So this means that no other platform xhci driver can be supported in the same system at the same time. Which kind of makes sense as that's not anything a normal system would have, BUT it feels very odd. This whole idea of "override the platform driver" feels fragile, why not make these just real platform drivers and have the xhci platform code be a library that the other ones can use? That way you have more control overall, right? thanks, greg k-h
On 1.12.2022 11.01, Arnd Bergmann wrote: > On Thu, Dec 1, 2022, at 09:06, Greg Kroah-Hartman wrote: >> On Thu, Dec 01, 2022 at 11:13:31AM +0900, Daehwan Jung wrote: >>> This driver works with xhci platform driver. It needs to override >>> functions of xhci_plat_hc_driver. Wakelocks are used for sleep/wakeup >>> scenario of system. >> >> So this means that no other platform xhci driver can be supported in the >> same system at the same time. >> >> Which kind of makes sense as that's not anything a normal system would >> have, BUT it feels very odd. This whole idea of "override the platform >> driver" feels fragile, why not make these just real platform drivers and >> have the xhci platform code be a library that the other ones can use? >> That way you have more control overall, right? Agree that overriding the generic platform driver xhci_hc_platform_driver from this exynos driver is odd. But I don't understand how this works. Where are the hcds created and added when this xhci-exonys driver binds to the device? all this driver does in probe is the overriding? Am I missing something here? > > Agreed, having another layer here (hcd -> xhci -> xhcd_platform -> > xhcd_exynos) would fit perfectly well into how other SoC specific > drivers are abstracted. This could potentially also help reduce > the amount of code duplication between other soc specific variants > (mtk, tegra, mvebu, ...) that are all platform drivers but don't > share code with xhci-plat.c. > > Alternatively, it seems that all of the xhci-exynos support could > just be part of the generic xhci-platform driver: as far as I can > tell, none of the added code is exynos specific at all, instead it > is a generic xhci that is using the wakeup_source framework. Sounds reasonable as well, and if some exynos specific code is needed then just create a xhci_plat_priv struct for exynos and pass it in of_device_id data like other vendors that use the generic xhci-platform driver do. -Mathias
On 02/12/2022 13:22, Mathias Nyman wrote: > On 1.12.2022 11.01, Arnd Bergmann wrote: >> On Thu, Dec 1, 2022, at 09:06, Greg Kroah-Hartman wrote: >>> On Thu, Dec 01, 2022 at 11:13:31AM +0900, Daehwan Jung wrote: >>>> This driver works with xhci platform driver. It needs to override >>>> functions of xhci_plat_hc_driver. Wakelocks are used for sleep/wakeup >>>> scenario of system. >>> >>> So this means that no other platform xhci driver can be supported in the >>> same system at the same time. >>> >>> Which kind of makes sense as that's not anything a normal system would >>> have, BUT it feels very odd. This whole idea of "override the platform >>> driver" feels fragile, why not make these just real platform drivers and >>> have the xhci platform code be a library that the other ones can use? >>> That way you have more control overall, right? > > Agree that overriding the generic platform driver xhci_hc_platform_driver > from this exynos driver is odd. > > But I don't understand how this works. > Where are the hcds created and added when this xhci-exonys driver binds to > the device? all this driver does in probe is the overriding? > > Am I missing something here? Because it is not a driver for Exynos... it's a driver for wakelocks for their specific Android use-cases which the manufacturer ships for their Android devices. Due to Google GKI, they try to squeeze into upstream. But this is huge misconception what should go to upstream and Samsung does not want to keep discussing. They just send random patches and disappear... Best regards, Krzysztof
On Fri, Dec 02, 2022 at 01:23:56PM +0100, Krzysztof Kozlowski wrote: > On 02/12/2022 13:22, Mathias Nyman wrote: > > On 1.12.2022 11.01, Arnd Bergmann wrote: > >> On Thu, Dec 1, 2022, at 09:06, Greg Kroah-Hartman wrote: > >>> On Thu, Dec 01, 2022 at 11:13:31AM +0900, Daehwan Jung wrote: > >>>> This driver works with xhci platform driver. It needs to override > >>>> functions of xhci_plat_hc_driver. Wakelocks are used for sleep/wakeup > >>>> scenario of system. > >>> > >>> So this means that no other platform xhci driver can be supported in the > >>> same system at the same time. > >>> > >>> Which kind of makes sense as that's not anything a normal system would > >>> have, BUT it feels very odd. This whole idea of "override the platform > >>> driver" feels fragile, why not make these just real platform drivers and > >>> have the xhci platform code be a library that the other ones can use? > >>> That way you have more control overall, right? > > > > Agree that overriding the generic platform driver xhci_hc_platform_driver > > from this exynos driver is odd. > > > > But I don't understand how this works. > > Where are the hcds created and added when this xhci-exonys driver binds to > > the device? all this driver does in probe is the overriding? > > > > Am I missing something here? > > Because it is not a driver for Exynos... it's a driver for wakelocks for > their specific Android use-cases which the manufacturer ships for their > Android devices. Due to Google GKI, they try to squeeze into upstream. GKI has nothing to do with this, this is Samsung not understanding how to properly submit code upstream. Odd that it comes down to them only as this same driver is used by _many_ OEMs who have good teams that know how to upstream code properly. All the blame shouldn't be on Samsung right now (see Google's last attempt at getting USB hooks accepted for this same hardware IP block...) thanks, greg k-h
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 8d799d23c476..007c7706ddeb 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -104,6 +104,14 @@ config USB_XHCI_TEGRA Say 'Y' to enable the support for the xHCI host controller found in NVIDIA Tegra124 and later SoCs. +config USB_XHCI_EXYNOS + tristate "xHCI support for Samsung Exynos SoC Series" + depends on USB_XHCI_PLATFORM + depends on ARCH_EXYNOS || COMPILE_TEST + help + Say 'Y' to enable the support for the xHCI host controller + found in Samsung Exynos SoCs. + endif # USB_XHCI_HCD config USB_EHCI_BRCMSTB diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile index 6d8ee264c9b2..c682834f4260 100644 --- a/drivers/usb/host/Makefile +++ b/drivers/usb/host/Makefile @@ -86,3 +86,4 @@ obj-$(CONFIG_USB_HCD_BCMA) += bcma-hcd.o obj-$(CONFIG_USB_HCD_SSB) += ssb-hcd.o obj-$(CONFIG_USB_MAX3421_HCD) += max3421-hcd.o obj-$(CONFIG_USB_XEN_HCD) += xen-hcd.o +obj-$(CONFIG_USB_XHCI_EXYNOS) += xhci-exynos.o diff --git a/drivers/usb/host/xhci-exynos.c b/drivers/usb/host/xhci-exynos.c new file mode 100644 index 000000000000..5e2323aee996 --- /dev/null +++ b/drivers/usb/host/xhci-exynos.c @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * xhci-exynos.c - xHCI host controller driver platform Bus Glue for Exynos. + * + * Copyright (C) 2022 Samsung Electronics Incorporated - http://www.samsung.com + * Author: Daehwan Jung <dh10.jung@samsung.com> + * + */ +#include <linux/platform_device.h> + +#include "xhci.h" +#include "xhci-plat.h" + +struct xhci_hcd_exynos { + struct wakeup_source *main_wakelock; /* Wakelock for HS HCD */ + struct wakeup_source *shared_wakelock; /* Wakelock for SS HCD */ +}; + +static struct xhci_hcd_exynos xhci_exynos_data; + +static int xhci_exynos_start(struct usb_hcd *hcd); +static int xhci_exynos_setup(struct usb_hcd *hcd); +static int xhci_exynos_bus_suspend(struct usb_hcd *hcd); +static int xhci_exynos_bus_resume(struct usb_hcd *hcd); + +static const struct xhci_driver_overrides xhci_exynos_overrides = { + .extra_priv_size = sizeof(struct xhci_plat_priv), + .reset = xhci_exynos_setup, + .start = xhci_exynos_start, + .bus_suspend = xhci_exynos_bus_suspend, + .bus_resume = xhci_exynos_bus_resume, +}; + +static void xhci_exynos_quirks(struct device *dev, struct xhci_hcd *xhci) +{ + xhci->quirks |= XHCI_PLAT; +} + +static int xhci_exynos_setup(struct usb_hcd *hcd) +{ + return xhci_gen_setup(hcd, xhci_exynos_quirks); +} + +static int xhci_exynos_start(struct usb_hcd *hcd) +{ + __pm_stay_awake(xhci_exynos_data.main_wakelock); + __pm_stay_awake(xhci_exynos_data.shared_wakelock); + + return xhci_run(hcd); +} + +static void xhci_exynos_wake_lock(struct xhci_hcd *xhci, int is_main_hcd, int is_lock) +{ + struct wakeup_source *main_wakelock, *shared_wakelock; + + main_wakelock = xhci_exynos_data.main_wakelock; + shared_wakelock = xhci_exynos_data.shared_wakelock; + + if (xhci->xhc_state & XHCI_STATE_REMOVING) + return; + + if (is_lock) { + if (is_main_hcd) + __pm_stay_awake(main_wakelock); + else + __pm_stay_awake(shared_wakelock); + } else { + if (is_main_hcd) + __pm_relax(main_wakelock); + else + __pm_relax(shared_wakelock); + } +} + +static int xhci_exynos_bus_suspend(struct usb_hcd *hcd) +{ + struct xhci_hcd *xhci = hcd_to_xhci(hcd); + int ret, main_hcd; + + ret = xhci_bus_suspend(hcd); + + if (!ret) { + main_hcd = (hcd == xhci->main_hcd) ? 1 : 0; + xhci_exynos_wake_lock(xhci, main_hcd, 0); + } + + return ret; +} + +static int xhci_exynos_bus_resume(struct usb_hcd *hcd) +{ + struct xhci_hcd *xhci = hcd_to_xhci(hcd); + int ret, main_hcd; + + ret = xhci_bus_resume(hcd); + + if (!ret) { + main_hcd = (hcd == xhci->main_hcd) ? 1 : 0; + xhci_exynos_wake_lock(xhci, main_hcd, 1); + } + + return ret; +} + +static int xhci_exynos_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + + xhci_exynos_data.main_wakelock = wakeup_source_register(dev, dev_name(dev)); + xhci_exynos_data.shared_wakelock = wakeup_source_register(dev, dev_name(dev)); + + xhci_plat_override_driver(&xhci_exynos_overrides); + + return 0; +} + +static int xhci_exynos_remove(struct platform_device *dev) +{ + wakeup_source_unregister(xhci_exynos_data.main_wakelock); + wakeup_source_unregister(xhci_exynos_data.shared_wakelock); + + return 0; +} + +static const struct of_device_id exynos_xhci_of_match[] = { + { .compatible = "samsung,exynos-xhci"}, + { }, +}; +MODULE_DEVICE_TABLE(of, exynos_xhci_of_match); + +static struct platform_driver exynos_xhci_driver = { + .probe = xhci_exynos_probe, + .remove = xhci_exynos_remove, + .driver = { + .name = "xhci-exynos", + .of_match_table = exynos_xhci_of_match, + }, +}; + +static int __init xhci_exynos_init(void) +{ + return platform_driver_register(&exynos_xhci_driver); +} +module_init(xhci_exynos_init); + +static void __exit xhci_exynos_exit(void) +{ + platform_driver_unregister(&exynos_xhci_driver); +} +module_exit(xhci_exynos_exit); + +MODULE_AUTHOR("Daehwan Jung <dh10.jung@samsung.com>"); +MODULE_DESCRIPTION("xHCI Exynos Host Controller Driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index 4619d5e89d5b..878c2c05055a 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -1824,6 +1824,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd) return 0; } +EXPORT_SYMBOL_GPL(xhci_bus_suspend); /* * Workaround for missing Cold Attach Status (CAS) if device re-plugged in S3. @@ -1968,6 +1969,7 @@ int xhci_bus_resume(struct usb_hcd *hcd) spin_unlock_irqrestore(&xhci->lock, flags); return 0; } +EXPORT_SYMBOL_GPL(xhci_bus_resume); unsigned long xhci_get_resuming_ports(struct usb_hcd *hcd) { diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 5fb55bf19493..1cb24f8e0153 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -173,6 +173,12 @@ static const struct of_device_id usb_xhci_of_match[] = { MODULE_DEVICE_TABLE(of, usb_xhci_of_match); #endif +void xhci_plat_override_driver(const struct xhci_driver_overrides *xhci_hc_driver_overrides) +{ + xhci_init_driver(&xhci_plat_hc_driver, xhci_hc_driver_overrides); +} +EXPORT_SYMBOL_GPL(xhci_plat_override_driver); + static int xhci_plat_probe(struct platform_device *pdev) { const struct xhci_plat_priv *priv_match; diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h index 1fb149d1fbce..16436f72c5c4 100644 --- a/drivers/usb/host/xhci-plat.h +++ b/drivers/usb/host/xhci-plat.h @@ -21,4 +21,6 @@ struct xhci_plat_priv { #define hcd_to_xhci_priv(h) ((struct xhci_plat_priv *)hcd_to_xhci(h)->priv) #define xhci_to_priv(x) ((struct xhci_plat_priv *)(x)->priv) + +void xhci_plat_override_driver(const struct xhci_driver_overrides *xhci_hc_driver_overrides); #endif /* _XHCI_PLAT_H */ 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 cc084d9505cd..30e60c752c28 100644 --- a/drivers/usb/host/xhci.h +++ b/drivers/usb/host/xhci.h @@ -1943,6 +1943,8 @@ struct xhci_driver_overrides { struct usb_host_endpoint *ep); int (*check_bandwidth)(struct usb_hcd *, struct usb_device *); void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *); + int (*bus_suspend)(struct usb_hcd *hcd); + int (*bus_resume)(struct usb_hcd *hcd); }; #define XHCI_CFC_DELAY 10
This driver works with xhci platform driver. It needs to override functions of xhci_plat_hc_driver. Wakelocks are used for sleep/wakeup scenario of system. Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> --- drivers/usb/host/Kconfig | 8 ++ drivers/usb/host/Makefile | 1 + drivers/usb/host/xhci-exynos.c | 154 +++++++++++++++++++++++++++++++++ drivers/usb/host/xhci-hub.c | 2 + drivers/usb/host/xhci-plat.c | 6 ++ drivers/usb/host/xhci-plat.h | 2 + drivers/usb/host/xhci.c | 4 + drivers/usb/host/xhci.h | 2 + 8 files changed, 179 insertions(+) create mode 100644 drivers/usb/host/xhci-exynos.c