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 <[email protected]>
---
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
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 <[email protected]>
+ *
+ */
+#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 <[email protected]>");
+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
--
2.31.1
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 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?
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.
It should be possible to check at runtime whether an xhci-platform
instance uses the wakeup source or not, and then have the same
driver work across more platforms.
Arnd
On 01/12/2022 10: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?
>
> 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.
There is nothing here Exynos SoC related, it's only for the purpose of
using wakelocks. We do not use wakelocks in other drivers, so I wonder
what makes this one so special... It does not look like generic approach
to the problem (which is BTW not really described in commit).
Best regards,
Krzysztof
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 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
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 05/12/2022 03:34, Jung Daehwan wrote:
>>> 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
>>
>>
>
> No. It's driver for Exynos. Currently It only has wakelocks but I will
> submit one by one. Please think as the first patch of exynos not
> squeezed.
That's not how upstream kernel development works... Your code has
nothing for Exynos. It's Android driver, not Exynos. If you say there is
something for Exynos it must be visible here. Wakelocks are not relevant
to Exynos, so after dropping them there would be empty stub in upstream
kernel which obviously cannot be accepted.
Best regards,
Krzysztof
On Mon, Dec 05, 2022 at 12:30:34PM +0900, Jung Daehwan wrote:
> On Thu, Dec 01, 2022 at 09:06:55AM +0100, 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?
> >
> > thanks,
> >
> > greg k-h
> >
>
> Currently It seems there are 2 ways to excute own function.
> 1. xhci_plat_priv
> -> This is hard to use it if the driver invokes xhci platform driver from
> dwc3_host_init(usb/dwc/host.c). I can't pass driver data during probe.
> 2. xhci_driver_overrides
> -> This is only useful if I has own xhci driver.
>
> That's why I wanted to extend overriding concept of xhci platform driver.
> If some code is better to be directly in xhci platform driver than xhci-exynos,
> I will modify it.
Again, please restructure this so that there is no need to "override"
anything and instead, you use the xhci-platform code from your driver
instead.
thanks,
greg k-h
On 05/12/2022 08:53, Jung Daehwan wrote:
> On Mon, Dec 05, 2022 at 08:33:39AM +0100, Krzysztof Kozlowski wrote:
>> On 05/12/2022 03:34, Jung Daehwan wrote:
>>
>>>>> 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
>>>>
>>>>
>>>
>>> No. It's driver for Exynos. Currently It only has wakelocks but I will
>>> submit one by one. Please think as the first patch of exynos not
>>> squeezed.
>>
>> That's not how upstream kernel development works... Your code has
>> nothing for Exynos. It's Android driver, not Exynos. If you say there is
>> something for Exynos it must be visible here. Wakelocks are not relevant
>> to Exynos, so after dropping them there would be empty stub in upstream
>> kernel which obviously cannot be accepted.
>>
>> Best regards,
>> Krzysztof
>>
>>
>
> Well, Exynos only uses wakelocks when I see mainline because it seems no
Exynos does not use wakelocks at all. Please explain me for what
hardware feature the wakelocks are needed when you do not use Android on
Exynos? Stop mixing Exynos with Android. One is hardware, second is
operating system.
> other driver use it. That's why I thought it could be a exynos specific.
> Do you agree that if I put wakelocks into xhci platform driver?
It's not related problem. Whether it suits there, I don't know.
Best regards,
Krzysztof