[RFC PATCH v1 0/2] add xhci-exynos to support Samsung Exynos SOCs

[RFC PATCH v1 0/2] add xhci-exynos to support Samsung Exynos SOCs

[Daehwan Jung]

This patchset is to support xHCI Controller on Samsung Exynos SOCs.

Daehwan Jung (2):
  dt-bindings: usb: samsung,exynos-xhci: support Samsung Exynos xHCI
    Controller
  usb: host: add xhci-exynos to support Exynos SOCs

 .../bindings/usb/samsung,exynos-xhci.yaml     |  25 +++
 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 +
 9 files changed, 204 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
 create mode 100644 drivers/usb/host/xhci-exynos.c

-- 
2.31.1

[RFC PATCH v1 1/2] dt-bindings: usb: samsung,exynos-xhci: support Samsung Exynos xHCI Controller

[Daehwan Jung]

Add the Samsung Exynos xHCI Controller bindings with DT schema format.

Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
---
 .../bindings/usb/samsung,exynos-xhci.yaml     | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml

diff --git a/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
new file mode 100644
index 000000000000..c5dde53b6491
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
@@ -0,0 +1,25 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/samsung,exynos-xhci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung Exynos xHCI
+
+maintainers:
+  - Daehwan Jung <dh10.jung@samsung.com>
+
+properties:
+  compatible:
+    const: samsung,exynos-xhci
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    xhci {
+        compatible = "samsung,exynos-xhci";
+    };
-- 
2.31.1

[RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Daehwan Jung]

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

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
-- 
2.31.1

Re: [RFC PATCH v1 0/2] add xhci-exynos to support Samsung Exynos SOCs

[Greg Kroah-Hartman]

On Thu, Dec 01, 2022 at 11:13:29AM +0900, Daehwan Jung wrote:
> This patchset is to support xHCI Controller on Samsung Exynos SOCs.
> 
> Daehwan Jung (2):
>   dt-bindings: usb: samsung,exynos-xhci: support Samsung Exynos xHCI
>     Controller
>   usb: host: add xhci-exynos to support Exynos SOCs

Why is this a "RFC" and not a real submission?  What needs to be done to
it to get it into mergable shape?

And thank you for posting this, I've wanted to see this merged for a
very long time given the millions of devices already using it.

thanks,

greg k-h

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Greg Kroah-Hartman]

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

Re: [RFC PATCH v1 0/2] add xhci-exynos to support Samsung Exynos SOCs

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

On Thu, Dec 01, 2022 at 09:03:01AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Dec 01, 2022 at 11:13:29AM +0900, Daehwan Jung wrote:
> > This patchset is to support xHCI Controller on Samsung Exynos SOCs.
> > 
> > Daehwan Jung (2):
> >   dt-bindings: usb: samsung,exynos-xhci: support Samsung Exynos xHCI
> >     Controller
> >   usb: host: add xhci-exynos to support Exynos SOCs
> 
> Why is this a "RFC" and not a real submission?  What needs to be done to
> it to get it into mergable shape?
> 
> And thank you for posting this, I've wanted to see this merged for a
> very long time given the millions of devices already using it.
> 
> thanks,
> 
> greg k-h
> 

Hi greg,

That's because It's my first time to submit the patchset with dt bindigs.
I've been trying not to miss anything needed but I want to check whether
it's acceptable or not. I will resend it if there's no problem.

Best Regards,
Jung Daehwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v1 1/2] dt-bindings: usb: samsung,exynos-xhci: support Samsung Exynos xHCI Controller

[Krzysztof Kozlowski]

On 01/12/2022 03:13, Daehwan Jung wrote:
> Add the Samsung Exynos xHCI Controller bindings with DT schema format.
> 
> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> ---
>  .../bindings/usb/samsung,exynos-xhci.yaml     | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
> new file mode 100644
> index 000000000000..c5dde53b6491
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
> @@ -0,0 +1,25 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/samsung,exynos-xhci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung Exynos xHCI
> +
> +maintainers:
> +  - Daehwan Jung <dh10.jung@samsung.com>
> +
> +properties:
> +  compatible:
> +    const: samsung,exynos-xhci
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +

These do not look like complete bindings... What type of device has no
resources at all, just compatible?

Best regards,
Krzysztof

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Arnd Bergmann]

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

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Krzysztof Kozlowski]

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

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Mathias Nyman]

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

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Krzysztof Kozlowski]

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

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Greg Kroah-Hartman]

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

Re: [RFC PATCH v1 1/2] dt-bindings: usb: samsung,exynos-xhci: support Samsung Exynos xHCI Controller

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]

On Thu, Dec 01, 2022 at 09:59:06AM +0100, Krzysztof Kozlowski wrote:
> On 01/12/2022 03:13, Daehwan Jung wrote:
> > Add the Samsung Exynos xHCI Controller bindings with DT schema format.
> > 
> > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> > ---
> >  .../bindings/usb/samsung,exynos-xhci.yaml     | 25 +++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
> > new file mode 100644
> > index 000000000000..c5dde53b6491
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
> > @@ -0,0 +1,25 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: https://protect2.fireeye.com/v1/url?k=7899b46f-19e45c17-78983f20-74fe485fffb1-728a1b33a5d009dd&q=1&e=bdc50247-e986-43da-a15e-03ac6c3a25e8&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fusb%2Fsamsung%2Cexynos-xhci.yaml%23
> > +$schema: https://protect2.fireeye.com/v1/url?k=ea1282f0-8b6f6a88-ea1309bf-74fe485fffb1-536f21757c62f28b&q=1&e=bdc50247-e986-43da-a15e-03ac6c3a25e8&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
> > +
> > +title: Samsung Exynos xHCI
> > +
> > +maintainers:
> > +  - Daehwan Jung <dh10.jung@samsung.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: samsung,exynos-xhci
> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> 
> These do not look like complete bindings... What type of device has no
> resources at all, just compatible?
> 
> Best regards,
> Krzysztof
> 
>

It gets resources from dwc->xhci_resources as you can see
dwc3_host_init(usb/dwc3/host.c). I think it doesn't need to get another resource.

Best Regards,
Jung Daehwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 1876 bytes --]

On Thu, Dec 01, 2022 at 10:01:44AM +0100, 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.
> 
> 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
> 

Currently there's no other platforms using wakelock. I wanted to add
xhci-exynos as I think Exynos use it specially. I also agree we can add it
on xhci platform driver if needed.

Best Regards,
Jung Daehwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]

On Fri, Dec 02, 2022 at 02:22:39PM +0200, 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?
> 

This works mainly with xhci platform driver. But xhci-exynos needs to override
some funtions. xhci-exynos probes first with override own functons and
it works with xhci platform driver.

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

I considered using existing overrides like xhci_plat_priv but I couldn't
find a solution. My driver invokes probing xhci platform driver in
source code not device tree. Allocation of platform device is done
in dwc3_host_init(usb/dwc3/host.c). That's why I can't pass device data
to xhci platform driver.

> -Mathias
> 
> 

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]

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

Best Regards,
Jung Daehwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]

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.

Best Regard,
Jung Daehwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v1 1/2] dt-bindings: usb: samsung,exynos-xhci: support Samsung Exynos xHCI Controller

[Krzysztof Kozlowski]

On 05/12/2022 03:06, Jung Daehwan wrote:
> On Thu, Dec 01, 2022 at 09:59:06AM +0100, Krzysztof Kozlowski wrote:
>> On 01/12/2022 03:13, Daehwan Jung wrote:
>>> Add the Samsung Exynos xHCI Controller bindings with DT schema format.
>>>
>>> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
>>> ---
>>>  .../bindings/usb/samsung,exynos-xhci.yaml     | 25 +++++++++++++++++++
>>>  1 file changed, 25 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
>>> new file mode 100644
>>> index 000000000000..c5dde53b6491
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
>>> @@ -0,0 +1,25 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: https://protect2.fireeye.com/v1/url?k=7899b46f-19e45c17-78983f20-74fe485fffb1-728a1b33a5d009dd&q=1&e=bdc50247-e986-43da-a15e-03ac6c3a25e8&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fusb%2Fsamsung%2Cexynos-xhci.yaml%23
>>> +$schema: https://protect2.fireeye.com/v1/url?k=ea1282f0-8b6f6a88-ea1309bf-74fe485fffb1-536f21757c62f28b&q=1&e=bdc50247-e986-43da-a15e-03ac6c3a25e8&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>>> +
>>> +title: Samsung Exynos xHCI
>>> +
>>> +maintainers:
>>> +  - Daehwan Jung <dh10.jung@samsung.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: samsung,exynos-xhci
>>> +
>>> +required:
>>> +  - compatible
>>> +
>>> +additionalProperties: false
>>> +
>>
>> These do not look like complete bindings... What type of device has no
>> resources at all, just compatible?
>>
>> Best regards,
>> Krzysztof
>>
>>
> 
> It gets resources from dwc->xhci_resources as you can see
> dwc3_host_init(usb/dwc3/host.c). I think it doesn't need to get another resource.

You refer to driver, but we talk about hardware. Not driver. Your
hardware has no resources, so this does not look like complete binding.

Best regards,
Krzysztof

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Krzysztof Kozlowski]

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

Re: [RFC PATCH v1 1/2] dt-bindings: usb: samsung,exynos-xhci: support Samsung Exynos xHCI Controller

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 2506 bytes --]

On Mon, Dec 05, 2022 at 08:31:46AM +0100, Krzysztof Kozlowski wrote:
> On 05/12/2022 03:06, Jung Daehwan wrote:
> > On Thu, Dec 01, 2022 at 09:59:06AM +0100, Krzysztof Kozlowski wrote:
> >> On 01/12/2022 03:13, Daehwan Jung wrote:
> >>> Add the Samsung Exynos xHCI Controller bindings with DT schema format.
> >>>
> >>> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> >>> ---
> >>>  .../bindings/usb/samsung,exynos-xhci.yaml     | 25 +++++++++++++++++++
> >>>  1 file changed, 25 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
> >>> new file mode 100644
> >>> index 000000000000..c5dde53b6491
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
> >>> @@ -0,0 +1,25 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: https://protect2.fireeye.com/v1/url?k=7899b46f-19e45c17-78983f20-74fe485fffb1-728a1b33a5d009dd&q=1&e=bdc50247-e986-43da-a15e-03ac6c3a25e8&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fusb%2Fsamsung%2Cexynos-xhci.yaml%23
> >>> +$schema: https://protect2.fireeye.com/v1/url?k=ea1282f0-8b6f6a88-ea1309bf-74fe485fffb1-536f21757c62f28b&q=1&e=bdc50247-e986-43da-a15e-03ac6c3a25e8&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
> >>> +
> >>> +title: Samsung Exynos xHCI
> >>> +
> >>> +maintainers:
> >>> +  - Daehwan Jung <dh10.jung@samsung.com>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: samsung,exynos-xhci
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>
> >> These do not look like complete bindings... What type of device has no
> >> resources at all, just compatible?
> >>
> >> Best regards,
> >> Krzysztof
> >>
> >>
> > 
> > It gets resources from dwc->xhci_resources as you can see
> > dwc3_host_init(usb/dwc3/host.c). I think it doesn't need to get another resource.
> 
> You refer to driver, but we talk about hardware. Not driver. Your
> hardware has no resources, so this does not look like complete binding.
> 
> Best regards,
> Krzysztof
> 
> 

It actually doesn't get new resources but shares resources of dwc3 driver.
You mean it's not complete binding without resources? Is it okay if I
add description about it? If not, could you suggest a good way?

Best regards,
Jung Daehwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Jung Daehwan]

[-- Attachment #1: Type: text/plain, Size: 1380 bytes --]

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
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?

Best Regards,
Jung Daehwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [RFC PATCH v1 1/2] dt-bindings: usb: samsung,exynos-xhci: support Samsung Exynos xHCI Controller

[Krzysztof Kozlowski]

On 05/12/2022 08:48, Jung Daehwan wrote:
> On Mon, Dec 05, 2022 at 08:31:46AM +0100, Krzysztof Kozlowski wrote:
>> On 05/12/2022 03:06, Jung Daehwan wrote:
>>> On Thu, Dec 01, 2022 at 09:59:06AM +0100, Krzysztof Kozlowski wrote:
>>>> On 01/12/2022 03:13, Daehwan Jung wrote:
>>>>> Add the Samsung Exynos xHCI Controller bindings with DT schema format.
>>>>>
>>>>> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
>>>>> ---
>>>>>  .../bindings/usb/samsung,exynos-xhci.yaml     | 25 +++++++++++++++++++
>>>>>  1 file changed, 25 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..c5dde53b6491
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/usb/samsung,exynos-xhci.yaml
>>>>> @@ -0,0 +1,25 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: https://protect2.fireeye.com/v1/url?k=7899b46f-19e45c17-78983f20-74fe485fffb1-728a1b33a5d009dd&q=1&e=bdc50247-e986-43da-a15e-03ac6c3a25e8&u=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fusb%2Fsamsung%2Cexynos-xhci.yaml%23
>>>>> +$schema: https://protect2.fireeye.com/v1/url?k=ea1282f0-8b6f6a88-ea1309bf-74fe485fffb1-536f21757c62f28b&q=1&e=bdc50247-e986-43da-a15e-03ac6c3a25e8&u=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23
>>>>> +
>>>>> +title: Samsung Exynos xHCI
>>>>> +
>>>>> +maintainers:
>>>>> +  - Daehwan Jung <dh10.jung@samsung.com>
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: samsung,exynos-xhci
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>
>>>> These do not look like complete bindings... What type of device has no
>>>> resources at all, just compatible?
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>>
>>>
>>> It gets resources from dwc->xhci_resources as you can see
>>> dwc3_host_init(usb/dwc3/host.c). I think it doesn't need to get another resource.
>>
>> You refer to driver, but we talk about hardware. Not driver. Your
>> hardware has no resources, so this does not look like complete binding.
>>
>> Best regards,
>> Krzysztof
>>
>>
> 
> It actually doesn't get new resources but shares resources of dwc3 driver.
> You mean it's not complete binding without resources? Is it okay if I
> add description about it? If not, could you suggest a good way?

No, description is not okay, because I doubt that your device does not
have IO space, clocks, interrupts and phys.

Therefore description changes nothing - binding still does not describe
the hardware. How to proceed? Write binding for a real hardware, not
wakelock-Android-driver-override.

Best regards,
Krzysztof

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Krzysztof Kozlowski]

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

Re: [RFC PATCH v1 2/2] usb: host: add xhci-exynos to support Exynos SOCs

[Greg Kroah-Hartman]

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