This patchset is for Samsung Exynos xhci host conroller. It uses xhci-plat
driver mainly and extends some functions by xhci hooks and overrides.
This driver supports USB offload which makes Co-processor to use
some memories of xhci. Especially it's useful for USB Audio scenario.
Audio stream would get shortcut because Co-processor directly write/read
data in xhci memories. It could get speed-up using faster memory like SRAM.
That's why this gives vendors flexibilty of memory management. This feature
is done with xhci hooks and overrides.
Changes in v2 :
- Fix commit message by adding Signed-off-by in each patch.
- Fix conflict on latest.
Changes in v3 :
- Remove export symbols and xhci hooks which xhci-exynos don't need.
- Modify commit message to clarify why it needs to export symbols.
- Check compiling of xhci-exynos.
Changes in v4 :
- Modify commit message to clarify why it needs to export symbols.
- Add a function for override of hc driver in xhci-plat.
- Make xhci-exynos extending xhci-plat by xhci hooks and overrides.
(vendor_init / vendor_cleanup hooks are useful from here v4)
- Change the term (USB offload -> xhci-exynos) on subject of patches.
Daehwan Jung (5):
usb: host: export symbols for xhci-exynos to use xhci hooks
usb: host: add xhci hooks for xhci-exynos
usb: host: xhci-plat: support override of hc driver
usb: host: add some to xhci overrides for xhci-exynos
usb: host: add xhci-exynos driver
drivers/usb/host/Kconfig | 8 +
drivers/usb/host/Makefile | 1 +
drivers/usb/host/xhci-exynos.c | 567 +++++++++++++++++++++++++++++++++
drivers/usb/host/xhci-exynos.h | 50 +++
drivers/usb/host/xhci-hub.c | 7 +
drivers/usb/host/xhci-mem.c | 150 +++++++--
drivers/usb/host/xhci-plat.c | 50 ++-
drivers/usb/host/xhci-plat.h | 9 +
drivers/usb/host/xhci-ring.c | 1 +
drivers/usb/host/xhci.c | 90 +++++-
drivers/usb/host/xhci.h | 50 +++
11 files changed, 958 insertions(+), 25 deletions(-)
create mode 100644 drivers/usb/host/xhci-exynos.c
create mode 100644 drivers/usb/host/xhci-exynos.h
--
2.31.1
On Tue, Apr 26, 2022 at 06:18:43PM +0900, Daehwan Jung wrote:
> This patchset is for Samsung Exynos xhci host conroller. It uses xhci-plat
> driver mainly and extends some functions by xhci hooks and overrides.
>
> This driver supports USB offload which makes Co-processor to use
> some memories of xhci. Especially it's useful for USB Audio scenario.
> Audio stream would get shortcut because Co-processor directly write/read
> data in xhci memories. It could get speed-up using faster memory like SRAM.
> That's why this gives vendors flexibilty of memory management. This feature
> is done with xhci hooks and overrides.
>
> Changes in v2 :
> - Fix commit message by adding Signed-off-by in each patch.
> - Fix conflict on latest.
Thanks for reworking these and submitting them. I have some comments on
the individual commits from a non-xhci-knowledge point of view.
greg k-h
It helps xhci-plat driver increase usability. Vendors could use functions
in xhci-plat mostly and use some overrides to do what they wants without
modifying xhci-plat driver.
Signed-off-by: Daehwan Jung <[email protected]>
---
drivers/usb/host/xhci-plat.c | 6 ++++++
drivers/usb/host/xhci-plat.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index a5881ff945a6..6efa3169bf69 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_vendor_overrides)
+{
+ xhci_init_driver(&xhci_plat_hc_driver, xhci_vendor_overrides);
+}
+EXPORT_SYMBOL(xhci_plat_override_driver);
+
static struct xhci_plat_priv_overwrite xhci_plat_vendor_overwrite;
int xhci_plat_register_vendor_ops(struct xhci_vendor_ops *vendor_ops)
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index 8c204f3234d8..455e0018d5e6 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -28,5 +28,6 @@ struct xhci_plat_priv_overwrite {
};
int xhci_plat_register_vendor_ops(struct xhci_vendor_ops *vendor_ops);
+void xhci_plat_override_driver(const struct xhci_driver_overrides *xhci_vendor_overrides);
#endif /* _XHCI_PLAT_H */
--
2.31.1
This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat
driver mainly and extends some functions by xhci hooks and overrides.
It supports USB Audio offload with Co-processor. It only cares DCBAA,
Device Context, Transfer Ring, Event Ring, and ERST. They are allocated
on specific address with xhci hooks. Co-processor could use them directly
without xhci driver after then.
Signed-off-by: Daehwan Jung <[email protected]>
---
drivers/usb/host/Kconfig | 8 +
drivers/usb/host/Makefile | 1 +
drivers/usb/host/xhci-exynos.c | 567 +++++++++++++++++++++++++++++++++
drivers/usb/host/xhci-exynos.h | 50 +++
4 files changed, 626 insertions(+)
create mode 100644 drivers/usb/host/xhci-exynos.c
create mode 100644 drivers/usb/host/xhci-exynos.h
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 682b3d2da623..ccafcd9b4212 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 2948983618fb..300f22b6eb1b 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -86,3 +86,4 @@ obj-$(CONFIG_USB_HCD_SSB) += ssb-hcd.o
obj-$(CONFIG_USB_FOTG210_HCD) += fotg210-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..4f22f620d7e0
--- /dev/null
+++ b/drivers/usb/host/xhci-exynos.c
@@ -0,0 +1,567 @@
+// 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]>
+ *
+ * A lot of code borrowed from the Linux xHCI driver.
+ */
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+
+#include "xhci.h"
+#include "xhci-plat.h"
+#include "xhci-exynos.h"
+
+static const struct dev_pm_ops xhci_exynos_pm_ops;
+
+static struct xhci_vendor_ops ops;
+static void xhci_exynos_parse_endpoint(struct xhci_hcd *xhci, struct usb_device *udev,
+ struct usb_endpoint_descriptor *desc, struct xhci_container_ctx *ctx);
+static void xhci_exynos_free_event_ring(struct xhci_hcd *xhci);
+static struct xhci_ring *xhci_ring_alloc_uram(struct xhci_hcd *xhci,
+ unsigned int num_segs, unsigned int cycle_state,
+ enum xhci_ring_type type, unsigned int max_packet, gfp_t flags,
+ u32 endpoint_type);
+static void xhci_exynos_ring_free(struct xhci_hcd *xhci, struct xhci_ring *ring);
+static int xhci_exynos_alloc_event_ring(struct xhci_hcd *xhci, gfp_t flags);
+
+static int xhci_exynos_register_vendor_ops(struct xhci_vendor_ops *vendor_ops)
+{
+ return xhci_plat_register_vendor_ops(&ops);
+}
+
+static void xhci_exynos_quirks(struct device *dev, struct xhci_hcd *xhci)
+{
+ struct xhci_plat_priv *priv = xhci_to_priv(xhci);
+ struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
+
+ xhci->quirks |= XHCI_PLAT | xhci_exynos->quirks;
+}
+
+/* called during probe() after chip reset completes */
+static int xhci_exynos_setup(struct usb_hcd *hcd)
+{
+ int ret;
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+
+ ret = xhci_gen_setup(hcd, xhci_exynos_quirks);
+ xhci_exynos_alloc_event_ring(xhci, GFP_KERNEL);
+
+ return ret;
+}
+
+static void xhci_exynos_usb_offload_enable_event_ring(struct xhci_hcd *xhci)
+{
+ struct xhci_plat_priv *priv = xhci_to_priv(xhci);
+ struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
+
+ u32 temp;
+ u64 temp_64;
+
+ temp_64 = xhci_read_64(xhci, &xhci_exynos->ir_set_audio->erst_dequeue);
+ temp_64 &= ~ERST_PTR_MASK;
+ xhci_info(xhci, "ERST2 deq = 64'h%0lx", (unsigned long) temp_64);
+
+ xhci_info(xhci, "// [USB Audio] Set the interrupt modulation register");
+ temp = readl(&xhci_exynos->ir_set_audio->irq_control);
+ temp &= ~ER_IRQ_INTERVAL_MASK;
+ temp |= (u32)160;
+ writel(temp, &xhci_exynos->ir_set_audio->irq_control);
+
+ temp = readl(&xhci_exynos->ir_set_audio->irq_pending);
+ xhci_info(xhci, "// [USB Audio] Enabling event ring interrupter %p by writing 0x%x to irq_pending",
+ xhci_exynos->ir_set_audio, (unsigned int) ER_IRQ_ENABLE(temp));
+ writel(ER_IRQ_ENABLE(temp), &xhci_exynos->ir_set_audio->irq_pending);
+}
+
+static int xhci_exynos_start(struct usb_hcd *hcd)
+{
+ struct xhci_hcd *xhci;
+ int ret;
+
+ ret = xhci_run(hcd);
+
+ xhci = hcd_to_xhci(hcd);
+ xhci_exynos_usb_offload_enable_event_ring(xhci);
+
+ return ret;
+}
+
+static int xhci_exynos_add_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
+ struct usb_host_endpoint *ep)
+{
+ int ret;
+ struct xhci_hcd *xhci;
+ struct xhci_virt_device *virt_dev;
+
+ ret = xhci_add_endpoint(hcd, udev, ep);
+
+ if (!ret && udev->slot_id) {
+ xhci = hcd_to_xhci(hcd);
+ virt_dev = xhci->devs[udev->slot_id];
+ xhci_exynos_parse_endpoint(xhci, udev, &ep->desc, virt_dev->out_ctx);
+ }
+
+ return ret;
+}
+
+static int xhci_exynos_address_device(struct usb_hcd *hcd, struct usb_device *udev)
+{
+ struct xhci_hcd *xhci;
+ int ret;
+
+ ret = xhci_address_device(hcd, udev);
+ xhci = hcd_to_xhci(hcd);
+
+ /* TODO: store and pass hw info to Co-Processor here*/
+
+ return ret;
+}
+
+static int xhci_exynos_wake_lock(struct xhci_hcd_exynos *xhci_exynos,
+ int is_main_hcd, int is_lock)
+{
+ struct usb_hcd *hcd = xhci_exynos->hcd;
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ struct wakeup_source *main_wakelock, *shared_wakelock;
+
+ main_wakelock = xhci_exynos->main_wakelock;
+ shared_wakelock = xhci_exynos->shared_wakelock;
+
+ if (xhci->xhc_state & XHCI_STATE_REMOVING)
+ return -ESHUTDOWN;
+
+ 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);
+ }
+
+ return 0;
+}
+
+static int xhci_exynos_bus_suspend(struct usb_hcd *hcd)
+{
+ struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
+ struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+
+
+ int ret, main_hcd;
+
+ if (hcd == xhci->main_hcd)
+ main_hcd = 1;
+ else
+ main_hcd = 0;
+
+ ret = xhci_bus_suspend(hcd);
+ xhci_exynos_wake_lock(xhci_exynos, main_hcd, 0);
+
+ return ret;
+}
+
+static int xhci_exynos_bus_resume(struct usb_hcd *hcd)
+{
+ struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
+ struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+
+ int ret, main_hcd;
+
+ if (hcd == xhci->main_hcd)
+ main_hcd = 1;
+ else
+ main_hcd = 0;
+
+ ret = xhci_bus_resume(hcd);
+ xhci_exynos_wake_lock(xhci_exynos, main_hcd, 1);
+
+ return ret;
+}
+
+const struct xhci_driver_overrides xhci_exynos_overrides = {
+ .reset = xhci_exynos_setup,
+ .start = xhci_exynos_start,
+ .add_endpoint = xhci_exynos_add_endpoint,
+ .address_device = xhci_exynos_address_device,
+ .bus_suspend = xhci_exynos_bus_suspend,
+ .bus_resume = xhci_exynos_bus_resume,
+};
+
+static int xhci_exynos_vendor_init(struct xhci_hcd *xhci, struct device *dev)
+{
+ struct usb_hcd *hcd;
+ struct xhci_hcd_exynos *xhci_exynos;
+ struct xhci_plat_priv *priv;
+ struct wakeup_source *main_wakelock, *shared_wakelock;
+ struct platform_device *pdev;
+
+ pdev = to_platform_device(dev);
+
+ xhci_plat_override_driver(&xhci_exynos_overrides);
+ dev->driver->pm = &xhci_exynos_pm_ops;
+
+ main_wakelock = wakeup_source_register(dev, dev_name(dev));
+ __pm_stay_awake(main_wakelock);
+
+ /* Initialization shared wakelock for SS HCD */
+ shared_wakelock = wakeup_source_register(dev, dev_name(dev));
+ __pm_stay_awake(shared_wakelock);
+
+ hcd = xhci->main_hcd;
+
+ priv = hcd_to_xhci_priv(hcd);
+ xhci_exynos = priv->vendor_priv;
+ xhci_exynos->dev = dev;
+ xhci_exynos->main_wakelock = main_wakelock;
+ xhci_exynos->shared_wakelock = shared_wakelock;
+
+ return 0;
+}
+
+static void xhci_exynos_vendor_cleanup(struct xhci_hcd *xhci)
+{
+ xhci_exynos_free_event_ring(xhci);
+}
+
+static bool xhci_exynos_is_usb_offload_enabled(struct xhci_hcd *xhci,
+ struct xhci_virt_device *virt_dev, unsigned int ep_index)
+{
+ /* TODO */
+ return true;
+}
+
+static struct xhci_device_context_array *xhci_exynos_alloc_dcbaa(
+ struct xhci_hcd *xhci, gfp_t flags)
+{
+ int i;
+
+ xhci->dcbaa = ioremap(EXYNOS_URAM_DCBAA_ADDR,
+ sizeof(*xhci->dcbaa));
+ if (!xhci->dcbaa)
+ return NULL;
+ /* Clear DCBAA */
+ for (i = 0; i < MAX_HC_SLOTS; i++)
+ xhci->dcbaa->dev_context_ptrs[i] = 0x0;
+
+ xhci->dcbaa->dma = EXYNOS_URAM_DCBAA_ADDR;
+
+ return xhci->dcbaa;
+}
+
+static void xhci_exynos_free_dcbaa(struct xhci_hcd *xhci)
+{
+ iounmap(xhci->dcbaa);
+ xhci->dcbaa = NULL;
+}
+
+static struct xhci_ring *xhci_exynos_alloc_transfer_ring(struct xhci_hcd *xhci, u32 endpoint_type,
+ enum xhci_ring_type ring_type, unsigned int max_packet, gfp_t mem_flags)
+{
+ return xhci_ring_alloc_uram(xhci, 1, 1, ring_type, max_packet, mem_flags, endpoint_type);
+}
+
+static void xhci_exynos_free_transfer_ring(struct xhci_hcd *xhci, struct xhci_ring *ring,
+ unsigned int ep_index)
+{
+ xhci_exynos_ring_free(xhci, ring);
+}
+
+static void xhci_exynos_alloc_container_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx,
+ int type, gfp_t flags)
+{
+ /* Only first Device Context uses URAM */
+ int i;
+
+ ctx->bytes = ioremap(EXYNOS_URAM_DEVICE_CTX_ADDR, EXYNOS_URAM_CTX_SIZE);
+ if (!ctx->bytes)
+ return;
+
+ for (i = 0; i < EXYNOS_URAM_CTX_SIZE; i++)
+ ctx->bytes[i] = 0;
+
+ ctx->dma = EXYNOS_URAM_DEVICE_CTX_ADDR;
+}
+
+static void xhci_exynos_free_container_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx)
+{
+ /* Ignore dma_pool_free if it is allocated from URAM */
+ if (ctx->dma != EXYNOS_URAM_DEVICE_CTX_ADDR)
+ dma_pool_free(xhci->device_pool, ctx->bytes, ctx->dma);
+}
+
+static int xhci_exynos_sync_dev_ctx(struct xhci_hcd *xhci, unsigned int slot_id)
+{
+ struct xhci_plat_priv *priv = xhci_to_priv(xhci);
+ struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
+ struct xhci_virt_device *virt_dev;
+ struct xhci_slot_ctx *slot_ctx;
+
+ int i;
+ int last_ep;
+ int last_ep_ctx = 31;
+
+ virt_dev = xhci->devs[slot_id];
+ slot_ctx = xhci_get_slot_ctx(xhci, virt_dev->in_ctx);
+
+ last_ep = LAST_CTX_TO_EP_NUM(le32_to_cpu(slot_ctx->dev_info));
+
+ if (last_ep < 31)
+ last_ep_ctx = last_ep + 1;
+
+ for (i = 0; i < last_ep_ctx; ++i) {
+ unsigned int epaddr = xhci_get_endpoint_address(i);
+ struct xhci_ep_ctx *ep_ctx = xhci_get_ep_ctx(xhci, virt_dev->out_ctx, i);
+
+ if (epaddr == xhci_exynos->in_ep)
+ xhci_exynos->in_deq = ep_ctx->deq;
+ else if (epaddr == xhci_exynos->out_ep)
+ xhci_exynos->out_deq = ep_ctx->deq;
+ }
+
+ return 0;
+}
+static struct xhci_vendor_ops ops = {
+ .vendor_init = xhci_exynos_vendor_init,
+ .vendor_cleanup = xhci_exynos_vendor_cleanup,
+ .is_usb_offload_enabled = xhci_exynos_is_usb_offload_enabled,
+ .alloc_dcbaa = xhci_exynos_alloc_dcbaa,
+ .free_dcbaa = xhci_exynos_free_dcbaa,
+ .alloc_transfer_ring = xhci_exynos_alloc_transfer_ring,
+ .free_transfer_ring = xhci_exynos_free_transfer_ring,
+ .alloc_container_ctx = xhci_exynos_alloc_container_ctx,
+ .free_container_ctx = xhci_exynos_free_container_ctx,
+ .sync_dev_ctx = xhci_exynos_sync_dev_ctx,
+};
+
+
+static void xhci_exynos_parse_endpoint(struct xhci_hcd *xhci, struct usb_device *udev,
+ struct usb_endpoint_descriptor *desc, struct xhci_container_ctx *ctx)
+{
+ struct xhci_plat_priv *priv = xhci_to_priv(xhci);
+ struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
+ struct usb_endpoint_descriptor *d = desc;
+ unsigned int ep_index;
+ struct xhci_ep_ctx *ep_ctx;
+
+ ep_index = xhci_get_endpoint_index(d);
+ ep_ctx = xhci_get_ep_ctx(xhci, ctx, ep_index);
+
+ if ((d->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) ==
+ USB_ENDPOINT_XFER_ISOC) {
+ if (d->bEndpointAddress & USB_ENDPOINT_DIR_MASK)
+ xhci_exynos->in_ep = d->bEndpointAddress;
+ else
+ xhci_exynos->out_ep = d->bEndpointAddress;
+ }
+}
+
+static void xhci_exynos_segment_free_skip(struct xhci_hcd *xhci, struct xhci_segment *seg)
+{
+ kfree(seg->bounce_buf);
+ kfree(seg);
+}
+
+static void xhci_exynos_free_segments_for_ring(struct xhci_hcd *xhci,
+ struct xhci_segment *first)
+{
+ struct xhci_segment *seg;
+
+ seg = first->next;
+
+ while (seg != first) {
+ struct xhci_segment *next = seg->next;
+
+ xhci_exynos_segment_free_skip(xhci, seg);
+ seg = next;
+ }
+ xhci_exynos_segment_free_skip(xhci, first);
+}
+
+static void xhci_exynos_ring_free(struct xhci_hcd *xhci, struct xhci_ring *ring)
+{
+ if (!ring)
+ return;
+
+ if (ring->first_seg) {
+ if (ring->type == TYPE_STREAM)
+ xhci_remove_stream_mapping(ring);
+
+ xhci_exynos_free_segments_for_ring(xhci, ring->first_seg);
+ }
+
+ kfree(ring);
+}
+
+static void xhci_exynos_free_event_ring(struct xhci_hcd *xhci)
+{
+ struct xhci_plat_priv *priv = xhci_to_priv(xhci);
+ struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
+
+ xhci_exynos_ring_free(xhci, xhci_exynos->event_ring_audio);
+}
+
+static void xhci_exynos_set_hc_event_deq_audio(struct xhci_hcd *xhci)
+{
+ struct xhci_plat_priv *priv = xhci_to_priv(xhci);
+ struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
+
+ u64 temp;
+ dma_addr_t deq;
+
+ deq = xhci_trb_virt_to_dma(xhci_exynos->event_ring_audio->deq_seg,
+ xhci_exynos->event_ring_audio->dequeue);
+ if (deq == 0 && !in_interrupt())
+ xhci_warn(xhci, "WARN something wrong with SW event ring "
+ "dequeue ptr.\n");
+ /* Update HC event ring dequeue pointer */
+ temp = xhci_read_64(xhci, &xhci_exynos->ir_set_audio->erst_dequeue);
+ temp &= ERST_PTR_MASK;
+ /* Don't clear the EHB bit (which is RW1C) because
+ * there might be more events to service.
+ */
+ temp &= ~ERST_EHB;
+ xhci_info(xhci, "//[%s] Write event ring dequeue pointer = 0x%llx, "
+ "preserving EHB bit", __func__,
+ ((u64) deq & (u64) ~ERST_PTR_MASK) | temp);
+ xhci_write_64(xhci, ((u64) deq & (u64) ~ERST_PTR_MASK) | temp,
+ &xhci_exynos->ir_set_audio->erst_dequeue);
+
+}
+
+
+
+static int xhci_exynos_alloc_event_ring(struct xhci_hcd *xhci, gfp_t flags)
+{
+ struct xhci_plat_priv *priv = xhci_to_priv(xhci);
+ struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
+
+ if (xhci_check_trb_in_td_math(xhci) < 0)
+ goto fail;
+
+ xhci_exynos->event_ring_audio = xhci_ring_alloc(xhci, ERST_NUM_SEGS, 1,
+ TYPE_EVENT, 0, flags);
+ /* Set the event ring dequeue address */
+ xhci_exynos_set_hc_event_deq_audio(xhci);
+
+ return 0;
+fail:
+ return -1;
+}
+
+static int xhci_alloc_segments_for_ring_uram(struct xhci_hcd *xhci,
+ struct xhci_segment **first, struct xhci_segment **last,
+ unsigned int num_segs, unsigned int cycle_state,
+ enum xhci_ring_type type, unsigned int max_packet, gfp_t flags,
+ u32 endpoint_type)
+{
+ struct xhci_segment *prev;
+ bool chain_links = false;
+
+ while (num_segs > 0) {
+ struct xhci_segment *next = NULL;
+
+ if (!next) {
+ prev = *first;
+ while (prev) {
+ next = prev->next;
+ xhci_segment_free(xhci, prev);
+ prev = next;
+ }
+ return -ENOMEM;
+ }
+ xhci_link_segments(prev, next, type, chain_links);
+
+ prev = next;
+ num_segs--;
+ }
+ xhci_link_segments(prev, *first, type, chain_links);
+ *last = prev;
+
+ return 0;
+}
+
+static struct xhci_ring *xhci_ring_alloc_uram(struct xhci_hcd *xhci,
+ unsigned int num_segs, unsigned int cycle_state,
+ enum xhci_ring_type type, unsigned int max_packet, gfp_t flags,
+ u32 endpoint_type)
+{
+ struct xhci_ring *ring;
+ int ret;
+ struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
+
+ ring = kzalloc_node(sizeof(*ring), flags, dev_to_node(dev));
+ if (!ring)
+ return NULL;
+
+ ring->num_segs = num_segs;
+ ring->bounce_buf_len = max_packet;
+ INIT_LIST_HEAD(&ring->td_list);
+ ring->type = type;
+ if (num_segs == 0)
+ return ring;
+
+ ret = xhci_alloc_segments_for_ring_uram(xhci, &ring->first_seg,
+ &ring->last_seg, num_segs, cycle_state, type,
+ max_packet, flags, endpoint_type);
+ if (ret)
+ goto fail;
+
+ /* Only event ring does not use link TRB */
+ if (type != TYPE_EVENT) {
+ /* See section 4.9.2.1 and 6.4.4.1 */
+ ring->last_seg->trbs[TRBS_PER_SEGMENT - 1].link.control |=
+ cpu_to_le32(LINK_TOGGLE);
+ }
+ xhci_initialize_ring_info(ring, cycle_state);
+
+ return ring;
+
+fail:
+ kfree(ring);
+ return NULL;
+}
+
+static int xhci_exynos_suspend(struct device *dev)
+{
+ struct usb_hcd *hcd = dev_get_drvdata(dev);
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+
+ /* TODO: AP sleep scenario*/
+
+ return xhci_suspend(xhci, device_may_wakeup(dev));
+}
+
+static int xhci_exynos_resume(struct device *dev)
+{
+ struct usb_hcd *hcd = dev_get_drvdata(dev);
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ int ret;
+
+ /* TODO: AP resume scenario*/
+
+ ret = xhci_resume(xhci, 0);
+ if (ret)
+ return ret;
+
+ pm_runtime_disable(dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+
+ return 0;
+}
+
+static const struct dev_pm_ops xhci_exynos_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(xhci_exynos_suspend, xhci_exynos_resume)
+};
+
+MODULE_DESCRIPTION("xHCI Exynos Host Controller Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/usb/host/xhci-exynos.h b/drivers/usb/host/xhci-exynos.h
new file mode 100644
index 000000000000..4428f8f3a5ff
--- /dev/null
+++ b/drivers/usb/host/xhci-exynos.h
@@ -0,0 +1,50 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * xhci-exynos.h - xHCI host controller driver platform Bus Glue for Exynos.
+ *
+ * Copyright (C) 2022 Samsung Electronics Incorporated - http://www.samsung.com
+ * Author: Daehwan Jung <[email protected]>
+ *
+ * A lot of code borrowed from the Linux xHCI driver.
+ */
+
+#include "xhci.h"
+
+/* EXYNOS uram memory map */
+#define EXYNOS_URAM_ABOX_EVT_RING_ADDR 0x02a00000
+#define EXYNOS_URAM_ISOC_OUT_RING_ADDR 0x02a01000
+#define EXYNOS_URAM_ISOC_IN_RING_ADDR 0x02a02000
+#define EXYNOS_URAM_DEVICE_CTX_ADDR 0x02a03000
+#define EXYNOS_URAM_DCBAA_ADDR 0x02a03880
+#define EXYNOS_URAM_ABOX_ERST_SEG_ADDR 0x02a03C80
+#define EXYNOS_URAM_CTX_SIZE 2112
+
+struct xhci_hcd_exynos {
+ struct xhci_intr_reg __iomem *ir_set_audio;
+
+ struct xhci_ring *event_ring_audio;
+ struct xhci_erst erst_audio;
+
+ struct device *dev;
+ struct usb_hcd *hcd;
+ struct usb_hcd *shared_hcd;
+
+ struct wakeup_source *main_wakelock; /* Wakelock for HS HCD */
+ struct wakeup_source *shared_wakelock; /* Wakelock for SS HCD */
+
+ u32 in_ep;
+ u32 out_ep;
+ u32 in_deq;
+ u32 out_deq;
+
+ unsigned long long quirks;
+};
+
+int xhci_check_trb_in_td_math(struct xhci_hcd *xhci);
+void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg);
+void xhci_link_segments(struct xhci_segment *prev,
+ struct xhci_segment *next,
+ enum xhci_ring_type type, bool chain_links);
+void xhci_initialize_ring_info(struct xhci_ring *ring,
+ unsigned int cycle_state);
+void xhci_remove_stream_mapping(struct xhci_ring *ring);
--
2.31.1
On Tue, Apr 26, 2022 at 06:18:48PM +0900, Daehwan Jung wrote:
> +int xhci_check_trb_in_td_math(struct xhci_hcd *xhci);
> +void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg);
> +void xhci_link_segments(struct xhci_segment *prev,
> + struct xhci_segment *next,
> + enum xhci_ring_type type, bool chain_links);
> +void xhci_initialize_ring_info(struct xhci_ring *ring,
> + unsigned int cycle_state);
> +void xhci_remove_stream_mapping(struct xhci_ring *ring);
Why does your single driver have global functions? That should not be
needed, right?
And these are odd for a driver's namespace...
thanks,
greg k-h
To enable supporting for USB offload, define "offload" in usb controller
node of device tree. "offload" value can be used to determine which type
of offload was been enabled in the SoC.
For example:
&usbdrd_dwc3 {
...
/* support usb offloading, 0: disabled, 1: audio */
offload = <1>;
...
};
There are several vendor_ops introduced by this patch:
struct xhci_vendor_ops - function callbacks for vendor specific operations
{
@vendor_init:
- called for vendor init process during xhci-plat-hcd
probe.
@vendor_cleanup:
- called for vendor cleanup process during xhci-plat-hcd
remove.
@is_usb_offload_enabled:
- called to check if usb offload enabled.
@alloc_dcbaa:
- called when allocating vendor specific dcbaa during
memory initializtion.
@free_dcbaa:
- called to free vendor specific dcbaa when cleanup the
memory.
@alloc_transfer_ring:
- called when vendor specific transfer ring allocation is required
@free_transfer_ring:
- called to free vendor specific transfer ring
@sync_dev_ctx:
- called when synchronization for device context is required
}
The xhci hooks with prefix "xhci_vendor_" on the ops in xhci_vendor_ops.
For example, vendor_init ops will be invoked by xhci_vendor_init() hook,
is_usb_offload_enabled ops will be invoked by
xhci_vendor_is_usb_offload_enabled(), and so on.
Signed-off-by: Daehwan Jung <[email protected]>
Signed-off-by: J. Avila <[email protected]>
Signed-off-by: Puma Hsu <[email protected]>
Signed-off-by: Howard Yen <[email protected]>
---
drivers/usb/host/xhci-hub.c | 5 ++
drivers/usb/host/xhci-mem.c | 131 +++++++++++++++++++++++++++++++----
drivers/usb/host/xhci-plat.c | 44 +++++++++++-
drivers/usb/host/xhci-plat.h | 8 +++
drivers/usb/host/xhci.c | 80 ++++++++++++++++++++-
drivers/usb/host/xhci.h | 46 ++++++++++++
6 files changed, 296 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 841617952ac7..e07c9c132061 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -535,8 +535,13 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
cmd->status == COMP_COMMAND_RING_STOPPED) {
xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
ret = -ETIME;
+ goto cmd_cleanup;
}
+ ret = xhci_vendor_sync_dev_ctx(xhci, slot_id);
+ if (ret)
+ xhci_warn(xhci, "Sync device context failed, ret=%d\n", ret);
+
cmd_cleanup:
xhci_free_command(xhci, cmd);
return ret;
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 82b9f90c0f27..5ee0ffb676d3 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -365,6 +365,54 @@ static int xhci_alloc_segments_for_ring(struct xhci_hcd *xhci,
return 0;
}
+static void xhci_vendor_free_container_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx)
+{
+ struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
+
+ if (ops && ops->free_container_ctx)
+ ops->free_container_ctx(xhci, ctx);
+}
+
+static void xhci_vendor_alloc_container_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx,
+ int type, gfp_t flags)
+{
+ struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
+
+ if (ops && ops->alloc_container_ctx)
+ ops->alloc_container_ctx(xhci, ctx, type, flags);
+}
+
+static struct xhci_ring *xhci_vendor_alloc_transfer_ring(struct xhci_hcd *xhci,
+ u32 endpoint_type, enum xhci_ring_type ring_type,
+ unsigned int max_packet, gfp_t mem_flags)
+{
+ struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
+
+ if (ops && ops->alloc_transfer_ring)
+ return ops->alloc_transfer_ring(xhci, endpoint_type, ring_type,
+ max_packet, mem_flags);
+ return 0;
+}
+
+void xhci_vendor_free_transfer_ring(struct xhci_hcd *xhci,
+ struct xhci_ring *ring, unsigned int ep_index)
+{
+ struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
+
+ if (ops && ops->free_transfer_ring)
+ ops->free_transfer_ring(xhci, ring, ep_index);
+}
+
+bool xhci_vendor_is_usb_offload_enabled(struct xhci_hcd *xhci,
+ struct xhci_virt_device *virt_dev, unsigned int ep_index)
+{
+ struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
+
+ if (ops && ops->is_usb_offload_enabled)
+ return ops->is_usb_offload_enabled(xhci, virt_dev, ep_index);
+ return false;
+}
+
/*
* Create a new ring with zero or more segments.
*
@@ -417,7 +465,11 @@ void xhci_free_endpoint_ring(struct xhci_hcd *xhci,
struct xhci_virt_device *virt_dev,
unsigned int ep_index)
{
- xhci_ring_free(xhci, virt_dev->eps[ep_index].ring);
+ if (xhci_vendor_is_usb_offload_enabled(xhci, virt_dev, ep_index))
+ xhci_vendor_free_transfer_ring(xhci, virt_dev->eps[ep_index].ring, ep_index);
+ else
+ xhci_ring_free(xhci, virt_dev->eps[ep_index].ring);
+
virt_dev->eps[ep_index].ring = NULL;
}
@@ -475,6 +527,7 @@ struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd *xhci,
{
struct xhci_container_ctx *ctx;
struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
+ struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
if ((type != XHCI_CTX_TYPE_DEVICE) && (type != XHCI_CTX_TYPE_INPUT))
return NULL;
@@ -488,7 +541,12 @@ struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd *xhci,
if (type == XHCI_CTX_TYPE_INPUT)
ctx->size += CTX_SIZE(xhci->hcc_params);
- ctx->bytes = dma_pool_zalloc(xhci->device_pool, flags, &ctx->dma);
+ if (xhci_vendor_is_usb_offload_enabled(xhci, NULL, 0) &&
+ (ops && ops->alloc_container_ctx))
+ xhci_vendor_alloc_container_ctx(xhci, ctx, type, flags);
+ else
+ ctx->bytes = dma_pool_zalloc(xhci->device_pool, flags, &ctx->dma);
+
if (!ctx->bytes) {
kfree(ctx);
return NULL;
@@ -499,9 +557,16 @@ struct xhci_container_ctx *xhci_alloc_container_ctx(struct xhci_hcd *xhci,
void xhci_free_container_ctx(struct xhci_hcd *xhci,
struct xhci_container_ctx *ctx)
{
+ struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
+
if (!ctx)
return;
- dma_pool_free(xhci->device_pool, ctx->bytes, ctx->dma);
+ if (xhci_vendor_is_usb_offload_enabled(xhci, NULL, 0) &&
+ (ops && ops->free_container_ctx))
+ xhci_vendor_free_container_ctx(xhci, ctx);
+ else
+ dma_pool_free(xhci->device_pool, ctx->bytes, ctx->dma);
+
kfree(ctx);
}
@@ -894,7 +959,7 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int slot_id)
for (i = 0; i < 31; i++) {
if (dev->eps[i].ring)
- xhci_ring_free(xhci, dev->eps[i].ring);
+ xhci_free_endpoint_ring(xhci, dev, i);
if (dev->eps[i].stream_info)
xhci_free_stream_info(xhci,
dev->eps[i].stream_info);
@@ -1492,8 +1557,16 @@ int xhci_endpoint_init(struct xhci_hcd *xhci,
mult = 0;
/* Set up the endpoint ring */
- virt_dev->eps[ep_index].new_ring =
- xhci_ring_alloc(xhci, 2, 1, ring_type, max_packet, mem_flags);
+ if (xhci_vendor_is_usb_offload_enabled(xhci, virt_dev, ep_index) &&
+ usb_endpoint_xfer_isoc(&ep->desc)) {
+ virt_dev->eps[ep_index].new_ring =
+ xhci_vendor_alloc_transfer_ring(xhci, endpoint_type, ring_type,
+ max_packet, mem_flags);
+ } else {
+ virt_dev->eps[ep_index].new_ring =
+ xhci_ring_alloc(xhci, 2, 1, ring_type, max_packet, mem_flags);
+ }
+
if (!virt_dev->eps[ep_index].new_ring)
return -ENOMEM;
@@ -1837,6 +1910,24 @@ void xhci_free_erst(struct xhci_hcd *xhci, struct xhci_erst *erst)
erst->entries = NULL;
}
+static struct xhci_device_context_array *xhci_vendor_alloc_dcbaa(
+ struct xhci_hcd *xhci, gfp_t flags)
+{
+ struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
+
+ if (ops && ops->alloc_dcbaa)
+ return ops->alloc_dcbaa(xhci, flags);
+ return 0;
+}
+
+static void xhci_vendor_free_dcbaa(struct xhci_hcd *xhci)
+{
+ struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
+
+ if (ops && ops->free_dcbaa)
+ ops->free_dcbaa(xhci);
+}
+
void xhci_mem_cleanup(struct xhci_hcd *xhci)
{
struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
@@ -1888,9 +1979,13 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
"Freed medium stream array pool");
- if (xhci->dcbaa)
- dma_free_coherent(dev, sizeof(*xhci->dcbaa),
- xhci->dcbaa, xhci->dcbaa->dma);
+ if (xhci_vendor_is_usb_offload_enabled(xhci, NULL, 0)) {
+ xhci_vendor_free_dcbaa(xhci);
+ } else {
+ if (xhci->dcbaa)
+ dma_free_coherent(dev, sizeof(*xhci->dcbaa),
+ xhci->dcbaa, xhci->dcbaa->dma);
+ }
xhci->dcbaa = NULL;
scratchpad_free(xhci);
@@ -2427,15 +2522,21 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
* xHCI section 5.4.6 - Device Context array must be
* "physically contiguous and 64-byte (cache line) aligned".
*/
- xhci->dcbaa = dma_alloc_coherent(dev, sizeof(*xhci->dcbaa), &dma,
- flags);
- if (!xhci->dcbaa)
- goto fail;
- xhci->dcbaa->dma = dma;
+ if (xhci_vendor_is_usb_offload_enabled(xhci, NULL, 0)) {
+ xhci->dcbaa = xhci_vendor_alloc_dcbaa(xhci, flags);
+ if (!xhci->dcbaa)
+ goto fail;
+ } else {
+ xhci->dcbaa = dma_alloc_coherent(dev, sizeof(*xhci->dcbaa), &dma,
+ flags);
+ if (!xhci->dcbaa)
+ goto fail;
+ xhci->dcbaa->dma = dma;
+ }
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
"// Device context base array address = 0x%llx (DMA), %p (virt)",
(unsigned long long)xhci->dcbaa->dma, xhci->dcbaa);
- xhci_write_64(xhci, dma, &xhci->op_regs->dcbaa_ptr);
+ xhci_write_64(xhci, xhci->dcbaa->dma, &xhci->op_regs->dcbaa_ptr);
/*
* Initialize the ring segment pool. The ring must be a contiguous
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 649ffd861b44..a5881ff945a6 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -173,6 +173,41 @@ static const struct of_device_id usb_xhci_of_match[] = {
MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
#endif
+static struct xhci_plat_priv_overwrite xhci_plat_vendor_overwrite;
+
+int xhci_plat_register_vendor_ops(struct xhci_vendor_ops *vendor_ops)
+{
+ if (vendor_ops == NULL)
+ return -EINVAL;
+
+ xhci_plat_vendor_overwrite.vendor_ops = vendor_ops;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(xhci_plat_register_vendor_ops);
+
+static int xhci_vendor_init(struct xhci_hcd *xhci, struct device *dev)
+{
+ struct xhci_vendor_ops *ops = NULL;
+
+ if (xhci_plat_vendor_overwrite.vendor_ops)
+ ops = xhci->vendor_ops = xhci_plat_vendor_overwrite.vendor_ops;
+
+ if (ops && ops->vendor_init)
+ return ops->vendor_init(xhci, dev);
+ return 0;
+}
+
+static void xhci_vendor_cleanup(struct xhci_hcd *xhci)
+{
+ struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
+
+ if (ops && ops->vendor_cleanup)
+ ops->vendor_cleanup(xhci);
+
+ xhci->vendor_ops = NULL;
+}
+
static int xhci_plat_probe(struct platform_device *pdev)
{
const struct xhci_plat_priv *priv_match;
@@ -185,7 +220,6 @@ static int xhci_plat_probe(struct platform_device *pdev)
int irq;
struct xhci_plat_priv *priv = NULL;
-
if (usb_disabled())
return -ENODEV;
@@ -321,6 +355,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
goto put_usb3_hcd;
}
+ ret = xhci_vendor_init(xhci, &pdev->dev);
+ if (ret)
+ goto disable_usb_phy;
+
hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
xhci->shared_hcd->tpl_support = hcd->tpl_support;
if (priv && (priv->quirks & XHCI_SKIP_PHY_INIT))
@@ -393,8 +431,10 @@ static int xhci_plat_remove(struct platform_device *dev)
usb_phy_shutdown(hcd->usb_phy);
usb_remove_hcd(hcd);
- usb_put_hcd(shared_hcd);
+ xhci_vendor_cleanup(xhci);
+
+ usb_put_hcd(shared_hcd);
clk_disable_unprepare(clk);
clk_disable_unprepare(reg_clk);
usb_put_hcd(hcd);
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index 1fb149d1fbce..8c204f3234d8 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -17,8 +17,16 @@ struct xhci_plat_priv {
int (*init_quirk)(struct usb_hcd *);
int (*suspend_quirk)(struct usb_hcd *);
int (*resume_quirk)(struct usb_hcd *);
+ void *vendor_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)
+
+struct xhci_plat_priv_overwrite {
+ struct xhci_vendor_ops *vendor_ops;
+};
+
+int xhci_plat_register_vendor_ops(struct xhci_vendor_ops *vendor_ops);
+
#endif /* _XHCI_PLAT_H */
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index c06e8b21b724..5ccf1bbe8732 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2986,6 +2986,14 @@ static int xhci_configure_endpoint(struct xhci_hcd *xhci,
xhci_finish_resource_reservation(xhci, ctrl_ctx);
spin_unlock_irqrestore(&xhci->lock, flags);
}
+ if (ret)
+ goto failed;
+
+ ret = xhci_vendor_sync_dev_ctx(xhci, udev->slot_id);
+ if (ret)
+ xhci_warn(xhci, "sync device context failed, ret=%d", ret);
+
+failed:
return ret;
}
@@ -3129,7 +3137,11 @@ void xhci_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
for (i = 0; i < 31; i++) {
if (virt_dev->eps[i].new_ring) {
xhci_debugfs_remove_endpoint(xhci, virt_dev, i);
- xhci_ring_free(xhci, virt_dev->eps[i].new_ring);
+ if (xhci_vendor_is_usb_offload_enabled(xhci, virt_dev, i))
+ xhci_vendor_free_transfer_ring(xhci, virt_dev->eps[i].new_ring, i);
+ else
+ xhci_ring_free(xhci, virt_dev->eps[i].new_ring);
+
virt_dev->eps[i].new_ring = NULL;
}
}
@@ -3290,6 +3302,13 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
wait_for_completion(stop_cmd->completion);
+ err = xhci_vendor_sync_dev_ctx(xhci, udev->slot_id);
+ if (err) {
+ xhci_warn(xhci, "%s: Failed to sync device context failed, err=%d",
+ __func__, err);
+ goto cleanup;
+ }
+
spin_lock_irqsave(&xhci->lock, flags);
/* config ep command clears toggle if add and drop ep flags are set */
@@ -3321,6 +3340,11 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
wait_for_completion(cfg_cmd->completion);
+ err = xhci_vendor_sync_dev_ctx(xhci, udev->slot_id);
+ if (err)
+ xhci_warn(xhci, "%s: Failed to sync device context failed, err=%d",
+ __func__, err);
+
xhci_free_command(xhci, cfg_cmd);
cleanup:
xhci_free_command(xhci, stop_cmd);
@@ -3866,6 +3890,13 @@ static int xhci_discover_or_reset_device(struct usb_hcd *hcd,
/* Wait for the Reset Device command to finish */
wait_for_completion(reset_device_cmd->completion);
+ ret = xhci_vendor_sync_dev_ctx(xhci, slot_id);
+ if (ret) {
+ xhci_warn(xhci, "%s: Failed to sync device context failed, err=%d",
+ __func__, ret);
+ goto command_cleanup;
+ }
+
/* The Reset Device command can't fail, according to the 0.95/0.96 spec,
* unless we tried to reset a slot ID that wasn't enabled,
* or the device wasn't in the addressed or configured state.
@@ -4111,6 +4142,14 @@ int xhci_alloc_dev(struct usb_hcd *hcd, struct usb_device *udev)
xhci_warn(xhci, "Could not allocate xHCI USB device data structures\n");
goto disable_slot;
}
+
+ ret = xhci_vendor_sync_dev_ctx(xhci, slot_id);
+ if (ret) {
+ xhci_warn(xhci, "%s: Failed to sync device context failed, err=%d",
+ __func__, ret);
+ goto disable_slot;
+ }
+
vdev = xhci->devs[slot_id];
slot_ctx = xhci_get_slot_ctx(xhci, vdev->out_ctx);
trace_xhci_alloc_dev(slot_ctx);
@@ -4241,6 +4280,13 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
/* ctrl tx can take up to 5 sec; XXX: need more time for xHC? */
wait_for_completion(command->completion);
+ ret = xhci_vendor_sync_dev_ctx(xhci, udev->slot_id);
+ if (ret) {
+ xhci_warn(xhci, "%s: Failed to sync device context failed, err=%d",
+ __func__, ret);
+ goto out;
+ }
+
/* FIXME: From section 4.3.4: "Software shall be responsible for timing
* the SetAddress() "recovery interval" required by USB and aborting the
* command on a timeout.
@@ -4393,6 +4439,14 @@ static int __maybe_unused xhci_change_max_exit_latency(struct xhci_hcd *xhci,
return -ENOMEM;
}
+ ret = xhci_vendor_sync_dev_ctx(xhci, udev->slot_id);
+ if (ret) {
+ spin_unlock_irqrestore(&xhci->lock, flags);
+ xhci_warn(xhci, "%s: Failed to sync device context failed, err=%d",
+ __func__, ret);
+ return ret;
+ }
+
xhci_slot_copy(xhci, command->in_ctx, virt_dev->out_ctx);
spin_unlock_irqrestore(&xhci->lock, flags);
@@ -4420,6 +4474,21 @@ static int __maybe_unused xhci_change_max_exit_latency(struct xhci_hcd *xhci,
return ret;
}
+struct xhci_vendor_ops *xhci_vendor_get_ops(struct xhci_hcd *xhci)
+{
+ return xhci->vendor_ops;
+}
+EXPORT_SYMBOL_GPL(xhci_vendor_get_ops);
+
+int xhci_vendor_sync_dev_ctx(struct xhci_hcd *xhci, unsigned int slot_id)
+{
+ struct xhci_vendor_ops *ops = xhci_vendor_get_ops(xhci);
+
+ if (ops && ops->sync_dev_ctx)
+ return ops->sync_dev_ctx(xhci, slot_id);
+ return 0;
+}
+
#ifdef CONFIG_PM
/* BESL to HIRD Encoding array for USB2 LPM */
@@ -5144,6 +5213,15 @@ static int xhci_update_hub_device(struct usb_hcd *hcd, struct usb_device *hdev,
return -ENOMEM;
}
+ ret = xhci_vendor_sync_dev_ctx(xhci, hdev->slot_id);
+ if (ret) {
+ xhci_warn(xhci, "%s: Failed to sync device context failed, err=%d",
+ __func__, ret);
+ xhci_free_command(xhci, config_cmd);
+ spin_unlock_irqrestore(&xhci->lock, flags);
+ return ret;
+ }
+
xhci_slot_copy(xhci, config_cmd->in_ctx, vdev->out_ctx);
ctrl_ctx->add_flags |= cpu_to_le32(SLOT_FLAG);
slot_ctx = xhci_get_slot_ctx(xhci, config_cmd->in_ctx);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 473a33ce299e..3a414a2f41f0 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1929,6 +1929,9 @@ struct xhci_hcd {
struct list_head regset_list;
void *dbc;
+
+ struct xhci_vendor_ops *vendor_ops;
+
/* platform-specific data -- must come last */
unsigned long priv[] __aligned(sizeof(s64));
};
@@ -2207,6 +2210,49 @@ static inline struct xhci_ring *xhci_urb_to_transfer_ring(struct xhci_hcd *xhci,
urb->stream_id);
}
+/**
+ * struct xhci_vendor_ops - function callbacks for vendor specific operations
+ * @vendor_init: called for vendor init process
+ * @vendor_cleanup: called for vendor cleanup process
+ * @is_usb_offload_enabled: called to check if usb offload enabled
+ * @alloc_dcbaa: called when allocating vendor specific dcbaa
+ * @free_dcbaa: called to free vendor specific dcbaa
+ * @alloc_transfer_ring: called when remote transfer ring allocation is required
+ * @free_transfer_ring: called to free vendor specific transfer ring
+ * @sync_dev_ctx: called when synchronization for device context is required
+ * @alloc_container_ctx: called when allocating vendor specific container context
+ * @free_container_ctx: called to free vendor specific container context
+ */
+struct xhci_vendor_ops {
+ int (*vendor_init)(struct xhci_hcd *xhci, struct device *dev);
+ void (*vendor_cleanup)(struct xhci_hcd *xhci);
+ bool (*is_usb_offload_enabled)(struct xhci_hcd *xhci,
+ struct xhci_virt_device *vdev,
+ unsigned int ep_index);
+
+ struct xhci_device_context_array *(*alloc_dcbaa)(struct xhci_hcd *xhci,
+ gfp_t flags);
+ void (*free_dcbaa)(struct xhci_hcd *xhci);
+
+ struct xhci_ring *(*alloc_transfer_ring)(struct xhci_hcd *xhci,
+ u32 endpoint_type, enum xhci_ring_type ring_type,
+ unsigned int max_packet, gfp_t mem_flags);
+ void (*free_transfer_ring)(struct xhci_hcd *xhci,
+ struct xhci_ring *ring, unsigned int ep_index);
+ int (*sync_dev_ctx)(struct xhci_hcd *xhci, unsigned int slot_id);
+ void (*alloc_container_ctx)(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx,
+ int type, gfp_t flags);
+ void (*free_container_ctx)(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx);
+};
+
+struct xhci_vendor_ops *xhci_vendor_get_ops(struct xhci_hcd *xhci);
+
+int xhci_vendor_sync_dev_ctx(struct xhci_hcd *xhci, unsigned int slot_id);
+void xhci_vendor_free_transfer_ring(struct xhci_hcd *xhci,
+ struct xhci_ring *ring, unsigned int ep_index);
+bool xhci_vendor_is_usb_offload_enabled(struct xhci_hcd *xhci,
+ struct xhci_virt_device *virt_dev, unsigned int ep_index);
+
/*
* TODO: As per spec Isochronous IDT transmissions are supported. We bypass
* them anyways as we where unable to find a device that matches the
--
2.31.1
Export symbols for xhci hooks usage:
xhci_get_slot_ctx
xhci_get_endpoint_address
- Allow xhci hook to get ep_ctx from the xhci_container_ctx for
getting the ep_ctx information to know which ep is offloading and
comparing the context in remote subsystem memory if needed.
xhci_ring_alloc
- Allow xhci hook to allocate vendor specific ring. Vendors could
alloc additional event ring.
xhci_trb_virt_to_dma
- Used to retrieve the DMA address of vendor specific ring. Vendors
could get dequeue address of event ring.
xhci_segment_free
xhci_link_segments
- Allow xhci hook to handle vendor specific segment. Vendors could
directly free or link segments of vendor specific ring.
xhci_initialize_ring_info
- Allow xhci hook to initialize vendor specific ring.
xhci_check_trb_in_td_math
- Allow xhci hook to Check TRB math for validation. Vendors could
check trb when allocating vendor specific ring.
xhci_address_device
- Allow override to give configuration info to Co-processor.
xhci_bus_suspend
xhci_bus_resume
- Allow override of suspend and resume for power scenario.
xhci_remove_stream_mapping
- Allow to xhci hook to remove stream mapping. Vendors need to do it
when free-ing vendor specific ring if it's stream type.
Signed-off-by: Daehwan Jung <[email protected]>
Signed-off-by: Jack Pham <[email protected]>
Signed-off-by: Howard Yen <[email protected]>
---
drivers/usb/host/xhci-hub.c | 2 ++
drivers/usb/host/xhci-mem.c | 19 +++++++++++++------
drivers/usb/host/xhci-ring.c | 1 +
drivers/usb/host/xhci.c | 4 +++-
4 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index f65f1ba2b592..841617952ac7 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1812,6 +1812,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.
@@ -1956,6 +1957,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-mem.c b/drivers/usb/host/xhci-mem.c
index bbb27ee2c6a3..82b9f90c0f27 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -65,7 +65,7 @@ static struct xhci_segment *xhci_segment_alloc(struct xhci_hcd *xhci,
return seg;
}
-static void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg)
+void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg)
{
if (seg->trbs) {
dma_pool_free(xhci->segment_pool, seg->trbs, seg->dma);
@@ -74,6 +74,7 @@ static void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg)
kfree(seg->bounce_buf);
kfree(seg);
}
+EXPORT_SYMBOL_GPL(xhci_segment_free);
static void xhci_free_segments_for_ring(struct xhci_hcd *xhci,
struct xhci_segment *first)
@@ -96,9 +97,9 @@ static void xhci_free_segments_for_ring(struct xhci_hcd *xhci,
* DMA address of the next segment. The caller needs to set any Link TRB
* related flags, such as End TRB, Toggle Cycle, and no snoop.
*/
-static void xhci_link_segments(struct xhci_segment *prev,
- struct xhci_segment *next,
- enum xhci_ring_type type, bool chain_links)
+void xhci_link_segments(struct xhci_segment *prev,
+ struct xhci_segment *next,
+ enum xhci_ring_type type, bool chain_links)
{
u32 val;
@@ -118,6 +119,7 @@ static void xhci_link_segments(struct xhci_segment *prev,
prev->trbs[TRBS_PER_SEGMENT-1].link.control = cpu_to_le32(val);
}
}
+EXPORT_SYMBOL_GPL(xhci_link_segments);
/*
* Link the ring to the new segments.
@@ -256,7 +258,7 @@ static int xhci_update_stream_segment_mapping(
return ret;
}
-static void xhci_remove_stream_mapping(struct xhci_ring *ring)
+void xhci_remove_stream_mapping(struct xhci_ring *ring)
{
struct xhci_segment *seg;
@@ -269,6 +271,7 @@ static void xhci_remove_stream_mapping(struct xhci_ring *ring)
seg = seg->next;
} while (seg != ring->first_seg);
}
+EXPORT_SYMBOL_GPL(xhci_remove_stream_mapping);
static int xhci_update_stream_mapping(struct xhci_ring *ring, gfp_t mem_flags)
{
@@ -316,6 +319,7 @@ void xhci_initialize_ring_info(struct xhci_ring *ring,
*/
ring->num_trbs_free = ring->num_segs * (TRBS_PER_SEGMENT - 1) - 1;
}
+EXPORT_SYMBOL_GPL(xhci_initialize_ring_info);
/* Allocate segments and link them for a ring */
static int xhci_alloc_segments_for_ring(struct xhci_hcd *xhci,
@@ -407,6 +411,7 @@ struct xhci_ring *xhci_ring_alloc(struct xhci_hcd *xhci,
kfree(ring);
return NULL;
}
+EXPORT_SYMBOL_GPL(xhci_ring_alloc);
void xhci_free_endpoint_ring(struct xhci_hcd *xhci,
struct xhci_virt_device *virt_dev,
@@ -518,6 +523,7 @@ struct xhci_slot_ctx *xhci_get_slot_ctx(struct xhci_hcd *xhci,
return (struct xhci_slot_ctx *)
(ctx->bytes + CTX_SIZE(xhci->hcc_params));
}
+EXPORT_SYMBOL_GPL(xhci_get_slot_ctx);
struct xhci_ep_ctx *xhci_get_ep_ctx(struct xhci_hcd *xhci,
struct xhci_container_ctx *ctx,
@@ -1965,7 +1971,7 @@ static int xhci_test_trb_in_td(struct xhci_hcd *xhci,
}
/* TRB math checks for xhci_trb_in_td(), using the command and event rings. */
-static int xhci_check_trb_in_td_math(struct xhci_hcd *xhci)
+int xhci_check_trb_in_td_math(struct xhci_hcd *xhci)
{
struct {
dma_addr_t input_dma;
@@ -2085,6 +2091,7 @@ static int xhci_check_trb_in_td_math(struct xhci_hcd *xhci)
xhci_dbg(xhci, "TRB math tests passed.\n");
return 0;
}
+EXPORT_SYMBOL_GPL(xhci_check_trb_in_td_math);
static void xhci_set_hc_event_deq(struct xhci_hcd *xhci)
{
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f9707997969d..652b37cd9c5e 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -79,6 +79,7 @@ dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg,
return 0;
return seg->dma + (segment_offset * sizeof(*trb));
}
+EXPORT_SYMBOL_GPL(xhci_trb_virt_to_dma);
static bool trb_is_noop(union xhci_trb *trb)
{
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 25b87e99b4dd..c06e8b21b724 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1468,6 +1468,7 @@ unsigned int xhci_get_endpoint_address(unsigned int ep_index)
unsigned int direction = ep_index % 2 ? USB_DIR_OUT : USB_DIR_IN;
return direction | number;
}
+EXPORT_SYMBOL_GPL(xhci_get_endpoint_address);
/* Find the flag for this endpoint (for use in the control context). Use the
* endpoint index to create a bitmask. The slot context is bit 0, endpoint 0 is
@@ -4324,10 +4325,11 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
return ret;
}
-static int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
+int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev)
{
return xhci_setup_device(hcd, udev, SETUP_CONTEXT_ADDRESS);
}
+EXPORT_SYMBOL_GPL(xhci_address_device);
static int xhci_enable_device(struct usb_hcd *hcd, struct usb_device *udev)
{
--
2.31.1
Co-processor needs some information about connected usb device.
It's proper to pass information after usb device gets address when
getting "Set Address" command. It supports vendors to implement it
using xhci overrides. There're several power scenarios depending
on vendors. It gives vendors flexibilty to meet their power requirement.
They can override suspend and resume of root hub.
Signed-off-by: Daehwan Jung <[email protected]>
---
drivers/usb/host/xhci.c | 6 ++++++
drivers/usb/host/xhci.h | 4 ++++
2 files changed, 10 insertions(+)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 5ccf1bbe8732..8b3df1302650 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -5555,6 +5555,12 @@ 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->address_device)
+ drv->address_device = over->address_device;
+ 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 3a414a2f41f0..5bc621e77762 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1947,6 +1947,9 @@ 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 (*address_device)(struct usb_hcd *hcd, struct usb_device *udev);
+ int (*bus_suspend)(struct usb_hcd *hcd);
+ int (*bus_resume)(struct usb_hcd *hcd);
};
#define XHCI_CFC_DELAY 10
@@ -2103,6 +2106,7 @@ int xhci_drop_endpoint(struct usb_hcd *hcd, struct usb_device *udev,
struct usb_host_endpoint *ep);
int xhci_check_bandwidth(struct usb_hcd *hcd, struct usb_device *udev);
void xhci_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev);
+int xhci_address_device(struct usb_hcd *hcd, struct usb_device *udev);
int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id);
int xhci_ext_cap_init(struct xhci_hcd *xhci);
--
2.31.1
On 26/04/2022 11:18, Daehwan Jung wrote:
> This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat
> driver mainly and extends some functions by xhci hooks and overrides.
>
> It supports USB Audio offload with Co-processor. It only cares DCBAA,
> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated
> on specific address with xhci hooks. Co-processor could use them directly
> without xhci driver after then.
This does not look like developed in current Linux kernel, but something
out-of-tree, with some other unknown modifications. This is not how the
code should be developed. Please rebase on linux-next and drop any
unrelated modifications (these which are not sent with this patchset).
(...)
> +
> +static int xhci_exynos_suspend(struct device *dev)
> +{
> + struct usb_hcd *hcd = dev_get_drvdata(dev);
> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +
> + /* TODO: AP sleep scenario*/
Shall the patchset be called RFC?
> +
> + return xhci_suspend(xhci, device_may_wakeup(dev));
> +}
> +
> +static int xhci_exynos_resume(struct device *dev)
> +{
> + struct usb_hcd *hcd = dev_get_drvdata(dev);
> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> + int ret;
> +
> + /* TODO: AP resume scenario*/
> +
> + ret = xhci_resume(xhci, 0);
> + if (ret)
> + return ret;
> +
> + pm_runtime_disable(dev);
> + pm_runtime_set_active(dev);
> + pm_runtime_enable(dev);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops xhci_exynos_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(xhci_exynos_suspend, xhci_exynos_resume)
> +};
> +
> +MODULE_DESCRIPTION("xHCI Exynos Host Controller Driver");
> +MODULE_LICENSE("GPL");
You don't have list of compatibles (and missing bindings), driver
definition, driver registration. Entire solution is not used - nothing
calls xhci_exynos_vendor_init(), because nothign uses "ops".
This does not work and it makes it impossible to test it. Please provide
proper XHCI Exynos driver, assuming you need it and it is not part of
regular Exynos XHCI drivers (DWC3 and so on).
Best regards,
Krzysztof
Hi Daehwan,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on usb/usb-testing]
[also build test WARNING on krzk/for-next char-misc/char-misc-testing v5.18-rc4 next-20220426]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Daehwan-Jung/usb-host-export-symbols-for-xhci-exynos-to-use-xhci-hooks/20220426-181936
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220427/[email protected]/config)
compiler: arceb-elf-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/6beb993f823c7c9a71f0b539a34d0ef5c64bd73d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Daehwan-Jung/usb-host-export-symbols-for-xhci-exynos-to-use-xhci-hooks/20220426-181936
git checkout 6beb993f823c7c9a71f0b539a34d0ef5c64bd73d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arc SHELL=/bin/bash drivers/usb/host/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/usb/host/xhci-exynos.c: In function 'xhci_exynos_address_device':
>> drivers/usb/host/xhci-exynos.c:112:26: warning: variable 'xhci' set but not used [-Wunused-but-set-variable]
112 | struct xhci_hcd *xhci;
| ^~~~
drivers/usb/host/xhci-exynos.c: In function 'xhci_exynos_vendor_init':
>> drivers/usb/host/xhci-exynos.c:205:34: warning: variable 'pdev' set but not used [-Wunused-but-set-variable]
205 | struct platform_device *pdev;
| ^~~~
drivers/usb/host/xhci-exynos.c: In function 'xhci_exynos_parse_endpoint':
>> drivers/usb/host/xhci-exynos.c:353:29: warning: variable 'ep_ctx' set but not used [-Wunused-but-set-variable]
353 | struct xhci_ep_ctx *ep_ctx;
| ^~~~~~
At top level:
drivers/usb/host/xhci-exynos.c:543:12: warning: 'xhci_exynos_resume' defined but not used [-Wunused-function]
543 | static int xhci_exynos_resume(struct device *dev)
| ^~~~~~~~~~~~~~~~~~
drivers/usb/host/xhci-exynos.c:533:12: warning: 'xhci_exynos_suspend' defined but not used [-Wunused-function]
533 | static int xhci_exynos_suspend(struct device *dev)
| ^~~~~~~~~~~~~~~~~~~
drivers/usb/host/xhci-exynos.c:30:12: warning: 'xhci_exynos_register_vendor_ops' defined but not used [-Wunused-function]
30 | static int xhci_exynos_register_vendor_ops(struct xhci_vendor_ops *vendor_ops)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/xhci +112 drivers/usb/host/xhci-exynos.c
109
110 static int xhci_exynos_address_device(struct usb_hcd *hcd, struct usb_device *udev)
111 {
> 112 struct xhci_hcd *xhci;
113 int ret;
114
115 ret = xhci_address_device(hcd, udev);
116 xhci = hcd_to_xhci(hcd);
117
118 /* TODO: store and pass hw info to Co-Processor here*/
119
120 return ret;
121 }
122
123 static int xhci_exynos_wake_lock(struct xhci_hcd_exynos *xhci_exynos,
124 int is_main_hcd, int is_lock)
125 {
126 struct usb_hcd *hcd = xhci_exynos->hcd;
127 struct xhci_hcd *xhci = hcd_to_xhci(hcd);
128 struct wakeup_source *main_wakelock, *shared_wakelock;
129
130 main_wakelock = xhci_exynos->main_wakelock;
131 shared_wakelock = xhci_exynos->shared_wakelock;
132
133 if (xhci->xhc_state & XHCI_STATE_REMOVING)
134 return -ESHUTDOWN;
135
136 if (is_lock) {
137 if (is_main_hcd)
138 __pm_stay_awake(main_wakelock);
139 else
140 __pm_stay_awake(shared_wakelock);
141 } else {
142 if (is_main_hcd)
143 __pm_relax(main_wakelock);
144 else
145 __pm_relax(shared_wakelock);
146 }
147
148 return 0;
149 }
150
151 static int xhci_exynos_bus_suspend(struct usb_hcd *hcd)
152 {
153 struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
154 struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
155 struct xhci_hcd *xhci = hcd_to_xhci(hcd);
156
157
158 int ret, main_hcd;
159
160 if (hcd == xhci->main_hcd)
161 main_hcd = 1;
162 else
163 main_hcd = 0;
164
165 ret = xhci_bus_suspend(hcd);
166 xhci_exynos_wake_lock(xhci_exynos, main_hcd, 0);
167
168 return ret;
169 }
170
171 static int xhci_exynos_bus_resume(struct usb_hcd *hcd)
172 {
173 struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
174 struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
175 struct xhci_hcd *xhci = hcd_to_xhci(hcd);
176
177 int ret, main_hcd;
178
179 if (hcd == xhci->main_hcd)
180 main_hcd = 1;
181 else
182 main_hcd = 0;
183
184 ret = xhci_bus_resume(hcd);
185 xhci_exynos_wake_lock(xhci_exynos, main_hcd, 1);
186
187 return ret;
188 }
189
190 const struct xhci_driver_overrides xhci_exynos_overrides = {
191 .reset = xhci_exynos_setup,
192 .start = xhci_exynos_start,
193 .add_endpoint = xhci_exynos_add_endpoint,
194 .address_device = xhci_exynos_address_device,
195 .bus_suspend = xhci_exynos_bus_suspend,
196 .bus_resume = xhci_exynos_bus_resume,
197 };
198
199 static int xhci_exynos_vendor_init(struct xhci_hcd *xhci, struct device *dev)
200 {
201 struct usb_hcd *hcd;
202 struct xhci_hcd_exynos *xhci_exynos;
203 struct xhci_plat_priv *priv;
204 struct wakeup_source *main_wakelock, *shared_wakelock;
> 205 struct platform_device *pdev;
206
207 pdev = to_platform_device(dev);
208
209 xhci_plat_override_driver(&xhci_exynos_overrides);
210 dev->driver->pm = &xhci_exynos_pm_ops;
211
212 main_wakelock = wakeup_source_register(dev, dev_name(dev));
213 __pm_stay_awake(main_wakelock);
214
215 /* Initialization shared wakelock for SS HCD */
216 shared_wakelock = wakeup_source_register(dev, dev_name(dev));
217 __pm_stay_awake(shared_wakelock);
218
219 hcd = xhci->main_hcd;
220
221 priv = hcd_to_xhci_priv(hcd);
222 xhci_exynos = priv->vendor_priv;
223 xhci_exynos->dev = dev;
224 xhci_exynos->main_wakelock = main_wakelock;
225 xhci_exynos->shared_wakelock = shared_wakelock;
226
227 return 0;
228 }
229
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Tue, Apr 26, 2022 at 06:18:48PM +0900, Daehwan Jung wrote:
> This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat
> driver mainly and extends some functions by xhci hooks and overrides.
>
> It supports USB Audio offload with Co-processor. It only cares DCBAA,
> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated
> on specific address with xhci hooks. Co-processor could use them directly
> without xhci driver after then.
>
> Signed-off-by: Daehwan Jung <[email protected]>
> ---
> drivers/usb/host/Kconfig | 8 +
> drivers/usb/host/Makefile | 1 +
> drivers/usb/host/xhci-exynos.c | 567 +++++++++++++++++++++++++++++++++
> drivers/usb/host/xhci-exynos.h | 50 +++
Why do you need a .h file for a simple single driver like this? Just
put it all into the .c file please.
thanks,
greg k-h
On 26/04/2022 11:18, Daehwan Jung wrote:
> This patchset is for Samsung Exynos xhci host conroller. It uses xhci-plat
> driver mainly and extends some functions by xhci hooks and overrides.
>
> This driver supports USB offload which makes Co-processor to use
> some memories of xhci. Especially it's useful for USB Audio scenario.
> Audio stream would get shortcut because Co-processor directly write/read
> data in xhci memories. It could get speed-up using faster memory like SRAM.
> That's why this gives vendors flexibilty of memory management. This feature
> is done with xhci hooks and overrides.
>
> Changes in v2 :
> - Fix commit message by adding Signed-off-by in each patch.
> - Fix conflict on latest.
>
> Changes in v3 :
> - Remove export symbols and xhci hooks which xhci-exynos don't need.
> - Modify commit message to clarify why it needs to export symbols.
> - Check compiling of xhci-exynos.
>
> Changes in v4 :
> - Modify commit message to clarify why it needs to export symbols.
> - Add a function for override of hc driver in xhci-plat.
> - Make xhci-exynos extending xhci-plat by xhci hooks and overrides.
> (vendor_init / vendor_cleanup hooks are useful from here v4)
> - Change the term (USB offload -> xhci-exynos) on subject of patches.
>
You received comments already that you need to base your work on recent
Linux kernel and use scripts/get_maintainers.pl to notify necessary
parties. This is the fourth patchset and still you did not do it.
Maybe there is some misunderstanding or trouble using
scripts/get_maintainers.pl, so could you clarify:
1. What is this based on? Output of: git describe
2. What does the scripts/get_maintainers.pl print when you run on this
patchset?
Best regards,
Krzysztof
On Wed, Apr 27, 2022 at 06:24:26PM +0900, Jung Daehwan wrote:
> On Tue, Apr 26, 2022 at 12:21:40PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 26, 2022 at 06:18:48PM +0900, Daehwan Jung wrote:
> > > +int xhci_check_trb_in_td_math(struct xhci_hcd *xhci);
> > > +void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg);
> > > +void xhci_link_segments(struct xhci_segment *prev,
> > > + struct xhci_segment *next,
> > > + enum xhci_ring_type type, bool chain_links);
> > > +void xhci_initialize_ring_info(struct xhci_ring *ring,
> > > + unsigned int cycle_state);
> > > +void xhci_remove_stream_mapping(struct xhci_ring *ring);
> >
> > Why does your single driver have global functions? That should not be
> > needed, right?
> >
> > And these are odd for a driver's namespace...
> >
>
> Those are exported in 1st patches. it's not good to include here as you said.
If you export a function, you also have to add it to a .h file
somewhere, otherwise it really is not exported.
thanks,
greg k-h
On 26.4.2022 12.18, Daehwan Jung wrote:
> This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat
> driver mainly and extends some functions by xhci hooks and overrides.
>
> It supports USB Audio offload with Co-processor. It only cares DCBAA,
> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated
> on specific address with xhci hooks. Co-processor could use them directly
> without xhci driver after then.
>
> Signed-off-by: Daehwan Jung <[email protected]>
I have to agree with Krzysztof's comments, this is an odd driver stub.
Perhaps open up a bit how the Exynos offloading works so we can figure out
in more detail what the hardware needs from software.
(...)
> +
> +static void xhci_exynos_alloc_container_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx,
> + int type, gfp_t flags)
> +{
> + /* Only first Device Context uses URAM */
> + int i;
> +
> + ctx->bytes = ioremap(EXYNOS_URAM_DEVICE_CTX_ADDR, EXYNOS_URAM_CTX_SIZE);
> + if (!ctx->bytes)
> + return;
> +
> + for (i = 0; i < EXYNOS_URAM_CTX_SIZE; i++)
> + ctx->bytes[i] = 0;
> +
> + ctx->dma = EXYNOS_URAM_DEVICE_CTX_ADDR;
This can't work with more than one USB device.
This hardcodes the same context address for every usb device.
> +static void xhci_exynos_parse_endpoint(struct xhci_hcd *xhci, struct usb_device *udev,
> + struct usb_endpoint_descriptor *desc, struct xhci_container_ctx *ctx)
> +{
> + struct xhci_plat_priv *priv = xhci_to_priv(xhci);
> + struct xhci_hcd_exynos *xhci_exynos = priv->vendor_priv;
> + struct usb_endpoint_descriptor *d = desc;
> + unsigned int ep_index;
> + struct xhci_ep_ctx *ep_ctx;
> +
> + ep_index = xhci_get_endpoint_index(d);
> + ep_ctx = xhci_get_ep_ctx(xhci, ctx, ep_index);
> +
> + if ((d->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) ==
> + USB_ENDPOINT_XFER_ISOC) {
> + if (d->bEndpointAddress & USB_ENDPOINT_DIR_MASK)
> + xhci_exynos->in_ep = d->bEndpointAddress;
> + else
> + xhci_exynos->out_ep = d->bEndpointAddress;
> + }
This won't work if more than one device that has isoc endpoints, or even
if that device has more than one in/out isoc endpoint pair.
> +static int xhci_alloc_segments_for_ring_uram(struct xhci_hcd *xhci,
> + struct xhci_segment **first, struct xhci_segment **last,
> + unsigned int num_segs, unsigned int cycle_state,
> + enum xhci_ring_type type, unsigned int max_packet, gfp_t flags,
> + u32 endpoint_type)
> +{
> + struct xhci_segment *prev;
> + bool chain_links = false;
> +
> + while (num_segs > 0) {
> + struct xhci_segment *next = NULL;
> +
> + if (!next) {
> + prev = *first;
> + while (prev) {
> + next = prev->next;
> + xhci_segment_free(xhci, prev);
> + prev = next;
> + }
> + return -ENOMEM;
This always return -ENOMEM
Also this whole function never allocates or remaps any memory.
> + }
> + xhci_link_segments(prev, next, type, chain_links);
> +
> + prev = next;
> + num_segs--;
> + }
> + xhci_link_segments(prev, *first, type, chain_links);
> + *last = prev;
> +
> + return 0;
> +}
> +
> +static struct xhci_ring *xhci_ring_alloc_uram(struct xhci_hcd *xhci,
> + unsigned int num_segs, unsigned int cycle_state,
> + enum xhci_ring_type type, unsigned int max_packet, gfp_t flags,
> + u32 endpoint_type)
> +{
> + struct xhci_ring *ring;
> + int ret;
> + struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
> +
> + ring = kzalloc_node(sizeof(*ring), flags, dev_to_node(dev));
> + if (!ring)
> + return NULL;
> +
> + ring->num_segs = num_segs;
> + ring->bounce_buf_len = max_packet;
> + INIT_LIST_HEAD(&ring->td_list);
> + ring->type = type;
> + if (num_segs == 0)
> + return ring;
> +
> + ret = xhci_alloc_segments_for_ring_uram(xhci, &ring->first_seg,
> + &ring->last_seg, num_segs, cycle_state, type,
> + max_packet, flags, endpoint_type);
> + if (ret)
> + goto fail;
> +
> + /* Only event ring does not use link TRB */
> + if (type != TYPE_EVENT) {
> + /* See section 4.9.2.1 and 6.4.4.1 */
> + ring->last_seg->trbs[TRBS_PER_SEGMENT - 1].link.control |=
> + cpu_to_le32(LINK_TOGGLE);
No memory was allocated for trbs
A lot of this code seems to exists just to avoid xhci driver from allocating
dma capable memory, we can refactor the existing xhci_mem_init() and move
dcbaa and event ring allocation and other code to their own overridable
functions.
This way we can probably get rid of a lot of the code in this series.
Thanks
Mathias
On 27/04/2022 11:49, Jung Daehwan wrote:
>> 1. What is this based on? Output of: git describe
>
> url = https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next
> fetch = +refs/heads/*:refs/remotes/origin/*
>
> or
>
> url = https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> fetch = +refs/heads/*:refs/remotes/origin/*
Thanks, although it is not what I asked for. It's not the output of `git
describe`. To get the output of git describe, execute commands in the
shell in the Git repository on your branch with these commits:
$ git describe
>> 2. What does the scripts/get_maintainers.pl print when you run on this
>> patchset?
>
> I don't see your name in xhci even for whole usb/host directory.
> I see same result on above 2 gits.
>
> jdh@PlatFormDev3:~/works/mainline/linux-next$ ./scripts/get_maintainer.pl drivers/usb/host/
That's not the proper way to get list of people to Cc when submitting
patches because it does not include the contents of the directory and
contents of other parts of the kernel which you might change.
> Greg Kroah-Hartman <[email protected]> (supporter:USB SUBSYSTEM,commit_signer:170/184=92%)
> Mathias Nyman <[email protected]> (commit_signer:52/184=28%,authored:25/184=14%)
> Alan Stern <[email protected]> (commit_signer:30/184=16%)
> Chunfeng Yun <[email protected]> (commit_signer:23/184=12%,authored:21/184=11%)
> [email protected] (open list:USB SUBSYSTEM)
> [email protected] (open list)
So either you run it in wrong way (not on the patchset but on some parts
of tree) or you still have it based on some different tree.
I just applied your patchset on linux-next and as expected output is
entirely different:
$ git format-patch -5
$ scripts/get_maintainer.pl 0*
(... skipping entries which you pasted)
Krzysztof Kozlowski <[email protected]>
(maintainer:ARM/SAMSUNG S3C, S5P AND EXYNOS ARM
ARCHITECTURES,authored:1/7=14%,added_lines:4/25=16%,removed_lines:2/13=15%)
[email protected] (moderated list:ARM/SAMSUNG S3C,
S5P AND EXYNOS ARM ARCHITECTURES)
[email protected] (open list:ARM/SAMSUNG S3C, S5P AND
EXYNOS ARM ARCHITECTURES)
> In fact, I manually tried adding you as you commendted previous patchset.
> But, It seems you changed email id and domain..
Up to date email is printed by scripts/get_maintainers.pl. If you don't
use that tool but add addresses manually - might work, might not.
Anyway, it's not only about my email - you did not Cc relevant mailing
lists, which I mentioned weeks ago as well.
Best regards,
Krzysztof
On 28/04/2022 08:36, Jung Daehwan wrote:
>>
>> Since you called everything here as "exynos" it is specific to one
>> hardware and not-reusable on anything else. How can then you use some
>> other compatible? It would be a misuse of Devicetree bindings.
>>
>
> I got it. Let me add them. Is it still necessary if it is only used by
> other module on runtime as I said above?
Except what Greg wrote, if by "other module" you mean out-of-tree, then
the patchset will not be accepted as it is unusable for Linux users.
Basically it would be a dead code in Linux kernel.
Best regards,
Krzysztof
On 28/04/2022 03:29, Jung Daehwan wrote:
> On Tue, Apr 26, 2022 at 02:59:57PM +0200, Krzysztof Kozlowski wrote:
>> On 26/04/2022 11:18, Daehwan Jung wrote:
>>> This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat
>>> driver mainly and extends some functions by xhci hooks and overrides.
>>>
>>> It supports USB Audio offload with Co-processor. It only cares DCBAA,
>>> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated
>>> on specific address with xhci hooks. Co-processor could use them directly
>>> without xhci driver after then.
>>
>> This does not look like developed in current Linux kernel, but something
>> out-of-tree, with some other unknown modifications. This is not how the
>> code should be developed. Please rebase on linux-next and drop any
>> unrelated modifications (these which are not sent with this patchset).
>>
>
> I've been developing on linux-next and I rebase before submitting.
> Could you tell me one of dropped modifications or patches?
>
>> (...)
>>
>>> +
>>> +static int xhci_exynos_suspend(struct device *dev)
>>> +{
>>> + struct usb_hcd *hcd = dev_get_drvdata(dev);
>>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>> +
>>> + /* TODO: AP sleep scenario*/
>>
>> Shall the patchset be called RFC?
>>
> OK. I will add RFC for this patch on next submission.
>
>>> +
>>> + return xhci_suspend(xhci, device_may_wakeup(dev));
>>> +}
>>> +
>>> +static int xhci_exynos_resume(struct device *dev)
>>> +{
>>> + struct usb_hcd *hcd = dev_get_drvdata(dev);
>>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>> + int ret;
>>> +
>>> + /* TODO: AP resume scenario*/
>>> +
>>> + ret = xhci_resume(xhci, 0);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + pm_runtime_disable(dev);
>>> + pm_runtime_set_active(dev);
>>> + pm_runtime_enable(dev);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct dev_pm_ops xhci_exynos_pm_ops = {
>>> + SET_SYSTEM_SLEEP_PM_OPS(xhci_exynos_suspend, xhci_exynos_resume)
>>> +};
>>> +
>>> +MODULE_DESCRIPTION("xHCI Exynos Host Controller Driver");
>>> +MODULE_LICENSE("GPL");
>>
>> You don't have list of compatibles (and missing bindings), driver
>> definition, driver registration. Entire solution is not used - nothing
>> calls xhci_exynos_vendor_init(), because nothign uses "ops".
>>
>
> xhci_exynos_vendor_init is called in xhci-plat.c (xhci_vendor_init)
> [v4,2/5] usb: host: add xhci hooks for xhci-exynos
> ops are used in some files(xhci-mem.c, xhci.c ..) and the body of ops is in
> all xhci-exynos.
Nothing uses the "ops" except xhci_exynos_register_vendor_ops() which is
not called anywhere, so the xhci_vendor_init() does not call
xhci_exynos_vendor_init().
>
> xhci-exynos is not a standalone driver. It could be enabled when other module
> makes xhci platform driver probed as it uses xhci platform mainly.
It "could be" or "will be"? We do not talk here about theoretical usage
of the driver, but a real one.
> I thought I just used existing compltible not adding new one.
> I will add them if needed.
Since you called everything here as "exynos" it is specific to one
hardware and not-reusable on anything else. How can then you use some
other compatible? It would be a misuse of Devicetree bindings.
Unless this is not specific to "exynos", but then please remove any
exynos and vendor references and make it integrated in generic XHCI
working for all drivers.
>> This does not work and it makes it impossible to test it. Please provide
>> proper XHCI Exynos driver, assuming you need it and it is not part of
>> regular Exynos XHCI drivers (DWC3 and so on).
>>
>
> What makes you think it doesn't work?
Not possible to probe. The code cannot be loaded.
> I think it's almost proper. I just removed
> other IPs code like Co-Processor(we call it abox) or Power Management because
> it would make build-error. I've added hooking points in some files(xhci-mem.c,
> xhci.c..) and ops are implemented in xhci-exynos. It's mainly operated with
> xhci platform driver.
Best regards,
Krzysztof
On 28.4.2022 6.03, Jung Daehwan wrote:
> On Wed, Apr 27, 2022 at 07:25:21PM +0300, Mathias Nyman wrote:
>> On 26.4.2022 12.18, Daehwan Jung wrote:
>>> This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat
>>> driver mainly and extends some functions by xhci hooks and overrides.
>>>
>>> It supports USB Audio offload with Co-processor. It only cares DCBAA,
>>> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated
>>> on specific address with xhci hooks. Co-processor could use them directly
>>> without xhci driver after then.
>>>
>>> Signed-off-by: Daehwan Jung <[email protected]>
>>
>> I have to agree with Krzysztof's comments, this is an odd driver stub.
>>
>> Perhaps open up a bit how the Exynos offloading works so we can figure out
>> in more detail what the hardware needs from software.
>>
>> (...)
>
>>> +static int xhci_alloc_segments_for_ring_uram(struct xhci_hcd *xhci,
>>> + struct xhci_segment **first, struct xhci_segment **last,
>>> + unsigned int num_segs, unsigned int cycle_state,
>>> + enum xhci_ring_type type, unsigned int max_packet, gfp_t flags,
>>> + u32 endpoint_type)
>>> +{
>>> + struct xhci_segment *prev;
>>> + bool chain_links = false;
>>> +
>>> + while (num_segs > 0) {
>>> + struct xhci_segment *next = NULL;
>>> +
>>> + if (!next) {
>>> + prev = *first;
>>> + while (prev) {
>>> + next = prev->next;
>>> + xhci_segment_free(xhci, prev);
>>> + prev = next;
>>> + }
>>> + return -ENOMEM;
>>
>> This always return -ENOMEM
>
> Yes. it's right to return error here.
>
Still don't think that is the case.
So if the num_segs value passed to a function named
xhci_alloc_segments_for_ring_uram() is anything else than 0, it will
automatically return -ENOMEM?
>>
>> Also this whole function never allocates or remaps any memory.
>
> This fuctions is for link segments. Right below function(xhci_ring_alloc_uram)
> allocates.
Still doesn't allocate any ring segments.
Below function only allocates memory for the
ring structure that contains pointers to segments.
>
>>
>>> + }
>>> + xhci_link_segments(prev, next, type, chain_links);
>>> +
>>> + prev = next;
>>> + num_segs--;
>>> + }
>>> + xhci_link_segments(prev, *first, type, chain_links);
>>> + *last = prev;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static struct xhci_ring *xhci_ring_alloc_uram(struct xhci_hcd *xhci,
>>> + unsigned int num_segs, unsigned int cycle_state,
>>> + enum xhci_ring_type type, unsigned int max_packet, gfp_t flags,
>>> + u32 endpoint_type)
>>> +{
>>> + struct xhci_ring *ring;
>>> + int ret;
>>> + struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
>>> +
>>> + ring = kzalloc_node(sizeof(*ring), flags, dev_to_node(dev));
>>> + if (!ring)
>>> + return NULL;
>>> +
>>> + ring->num_segs = num_segs;
>>> + ring->bounce_buf_len = max_packet;
>>> + INIT_LIST_HEAD(&ring->td_list);
>>> + ring->type = type;
>>> + if (num_segs == 0)
>>> + return ring;
>>> +
>>> + ret = xhci_alloc_segments_for_ring_uram(xhci, &ring->first_seg,
>>> + &ring->last_seg, num_segs, cycle_state, type,
>>> + max_packet, flags, endpoint_type);
>>> + if (ret)
>>> + goto fail;
>>> +
>>> + /* Only event ring does not use link TRB */
>>> + if (type != TYPE_EVENT) {
>>> + /* See section 4.9.2.1 and 6.4.4.1 */
>>> + ring->last_seg->trbs[TRBS_PER_SEGMENT - 1].link.control |=
>>> + cpu_to_le32(LINK_TOGGLE);
>>
>> No memory was allocated for trbs
>
> Allcation function for trbs are missed. It's done by ioremap.
> I will add it on next submission. Thanks for the comment.
>
>>
>> A lot of this code seems to exists just to avoid xhci driver from allocating
>> dma capable memory, we can refactor the existing xhci_mem_init() and move
>> dcbaa and event ring allocation and other code to their own overridable
>> functions.
>>
>> This way we can probably get rid of a lot of the code in this series.
>
> Yes right. I think it's proper. Do you agree with it or have better way
> to do it?
Could be, but I don't have a good picture of how this Exynos audio offloading
works, so it's hard to guess.
Thanks
-Mathias
On Thu, Apr 28, 2022 at 03:36:34PM +0900, Jung Daehwan wrote:
> On Thu, Apr 28, 2022 at 07:19:04AM +0200, Krzysztof Kozlowski wrote:
> > On 28/04/2022 03:29, Jung Daehwan wrote:
> > > On Tue, Apr 26, 2022 at 02:59:57PM +0200, Krzysztof Kozlowski wrote:
> > >> On 26/04/2022 11:18, Daehwan Jung wrote:
> > >>> This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat
> > >>> driver mainly and extends some functions by xhci hooks and overrides.
> > >>>
> > >>> It supports USB Audio offload with Co-processor. It only cares DCBAA,
> > >>> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated
> > >>> on specific address with xhci hooks. Co-processor could use them directly
> > >>> without xhci driver after then.
> > >>
> > >> This does not look like developed in current Linux kernel, but something
> > >> out-of-tree, with some other unknown modifications. This is not how the
> > >> code should be developed. Please rebase on linux-next and drop any
> > >> unrelated modifications (these which are not sent with this patchset).
> > >>
> > >
> > > I've been developing on linux-next and I rebase before submitting.
> > > Could you tell me one of dropped modifications or patches?
> > >
> > >> (...)
> > >>
> > >>> +
> > >>> +static int xhci_exynos_suspend(struct device *dev)
> > >>> +{
> > >>> + struct usb_hcd *hcd = dev_get_drvdata(dev);
> > >>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > >>> +
> > >>> + /* TODO: AP sleep scenario*/
> > >>
> > >> Shall the patchset be called RFC?
> > >>
> > > OK. I will add RFC for this patch on next submission.
> > >
> > >>> +
> > >>> + return xhci_suspend(xhci, device_may_wakeup(dev));
> > >>> +}
> > >>> +
> > >>> +static int xhci_exynos_resume(struct device *dev)
> > >>> +{
> > >>> + struct usb_hcd *hcd = dev_get_drvdata(dev);
> > >>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > >>> + int ret;
> > >>> +
> > >>> + /* TODO: AP resume scenario*/
> > >>> +
> > >>> + ret = xhci_resume(xhci, 0);
> > >>> + if (ret)
> > >>> + return ret;
> > >>> +
> > >>> + pm_runtime_disable(dev);
> > >>> + pm_runtime_set_active(dev);
> > >>> + pm_runtime_enable(dev);
> > >>> +
> > >>> + return 0;
> > >>> +}
> > >>> +
> > >>> +static const struct dev_pm_ops xhci_exynos_pm_ops = {
> > >>> + SET_SYSTEM_SLEEP_PM_OPS(xhci_exynos_suspend, xhci_exynos_resume)
> > >>> +};
> > >>> +
> > >>> +MODULE_DESCRIPTION("xHCI Exynos Host Controller Driver");
> > >>> +MODULE_LICENSE("GPL");
> > >>
> > >> You don't have list of compatibles (and missing bindings), driver
> > >> definition, driver registration. Entire solution is not used - nothing
> > >> calls xhci_exynos_vendor_init(), because nothign uses "ops".
> > >>
> > >
> > > xhci_exynos_vendor_init is called in xhci-plat.c (xhci_vendor_init)
> > > [v4,2/5] usb: host: add xhci hooks for xhci-exynos
> > > ops are used in some files(xhci-mem.c, xhci.c ..) and the body of ops is in
> > > all xhci-exynos.
> >
> >
> > Nothing uses the "ops" except xhci_exynos_register_vendor_ops() which is
> > not called anywhere, so the xhci_vendor_init() does not call
> > xhci_exynos_vendor_init().
> >
>
> You are right. xhci_exynos_register_vendor_ops should be called by other
> module. It's only thing not called anywhere in this patchset. I don't uses
> xhci-exynos alone in my scenario. Other module loads this on runtime.
>
> > >
> > > xhci-exynos is not a standalone driver. It could be enabled when other module
> > > makes xhci platform driver probed as it uses xhci platform mainly.
> >
> > It "could be" or "will be"? We do not talk here about theoretical usage
> > of the driver, but a real one.
> >
> > > I thought I just used existing compltible not adding new one.
> > > I will add them if needed.
> >
> > Since you called everything here as "exynos" it is specific to one
> > hardware and not-reusable on anything else. How can then you use some
> > other compatible? It would be a misuse of Devicetree bindings.
> >
>
> I got it. Let me add them. Is it still necessary if it is only used by
> other module on runtime as I said above?
Please submit a full, working driver so these changes can be able to be
properly reviewed. Otherwise it is just a waste of time for us to even
read them, right?
We do not add changes to the kernel that do not work or do anything,
that would be pointless, and cause us extra work and maintenance.
thanks,
greg k-h
On 28/04/2022 09:53, Jung Daehwan wrote:
> On Thu, Apr 28, 2022 at 09:31:56AM +0200, Krzysztof Kozlowski wrote:
>> On 28/04/2022 08:36, Jung Daehwan wrote:
>>>>
>>>> Since you called everything here as "exynos" it is specific to one
>>>> hardware and not-reusable on anything else. How can then you use some
>>>> other compatible? It would be a misuse of Devicetree bindings.
>>>>
>>>
>>> I got it. Let me add them. Is it still necessary if it is only used by
>>> other module on runtime as I said above?
>>
>> Except what Greg wrote, if by "other module" you mean out-of-tree, then
>> the patchset will not be accepted as it is unusable for Linux users.
>> Basically it would be a dead code in Linux kernel.
>>
>> Best regards,
>> Krzysztof
>>
>
> I wanted to submit patches of just xhci. Let me add a patch together of other
> module(dwc3-exynos) that is in-tree on next submission.
>
> Is it still necessary to add compatible or bindings in this case?
The answer depends on:
1. Is it possible to use the code without compatible and bindings?
2. Does the new feature changes the hardware description or hardware
behavior? IOW, does it change the meaning of existing bindings?
3. How does it fit to the process explained in
bindings/submitting-patches.rst?
Best regards,
Krzysztof