[PATCH v4 0/5] add xhci-exynos driver

[PATCH v4 0/5] add xhci-exynos driver

[Daehwan Jung]

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

[PATCH v4 1/5] usb: host: export symbols for xhci-exynos to use xhci hooks

[Daehwan Jung]

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 <dh10.jung@samsung.com>
Signed-off-by: Jack Pham <jackp@codeaurora.org>
Signed-off-by: Howard Yen <howardyen@google.com>
---
 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

Re: [PATCH v4 1/5] usb: host: export symbols for xhci-exynos to use xhci hooks

[Greg Kroah-Hartman]

On Tue, Apr 26, 2022 at 06:18:44PM +0900, Daehwan Jung wrote:
> 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.
> 

For the static functions that you are now exporting, they need to have
their functions declared in a .h file.  If not, you now get warnings
when you run sparse after applying this commit, right?

thanks,

greg k-h

Re: [PATCH v4 1/5] usb: host: export symbols for xhci-exynos to use xhci hooks

[Jung Daehwan]

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

On Tue, Apr 26, 2022 at 11:54:16AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 26, 2022 at 06:18:44PM +0900, Daehwan Jung wrote:
> > 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.
> > 
> 
> For the static functions that you are now exporting, they need to have
> their functions declared in a .h file.  If not, you now get warnings
> when you run sparse after applying this commit, right?
> 
> thanks,
> 
> greg k-h
> 

I didn't get warnings in my environment. I tested to build default config +
xhci_exynos by make command. Is there compile option to check it?

Best Regards,
Jung Daehwan

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

Re: [PATCH v4 1/5] usb: host: export symbols for xhci-exynos to use xhci hooks

[Greg Kroah-Hartman]

On Tue, Apr 26, 2022 at 07:27:09PM +0900, Jung Daehwan wrote:
> On Tue, Apr 26, 2022 at 11:54:16AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 26, 2022 at 06:18:44PM +0900, Daehwan Jung wrote:
> > > 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.
> > > 
> > 
> > For the static functions that you are now exporting, they need to have
> > their functions declared in a .h file.  If not, you now get warnings
> > when you run sparse after applying this commit, right?
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 
> I didn't get warnings in my environment. I tested to build default config +
> xhci_exynos by make command. Is there compile option to check it?

make "C=1"

Re: [PATCH v4 1/5] usb: host: export symbols for xhci-exynos to use xhci hooks

[Greg Kroah-Hartman]

On Tue, Apr 26, 2022 at 12:31:33PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 26, 2022 at 07:27:09PM +0900, Jung Daehwan wrote:
> > On Tue, Apr 26, 2022 at 11:54:16AM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Apr 26, 2022 at 06:18:44PM +0900, Daehwan Jung wrote:
> > > > 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.
> > > > 
> > > 
> > > For the static functions that you are now exporting, they need to have
> > > their functions declared in a .h file.  If not, you now get warnings
> > > when you run sparse after applying this commit, right?
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > > 
> > 
> > I didn't get warnings in my environment. I tested to build default config +
> > xhci_exynos by make command. Is there compile option to check it?
> 
> make "C=1"

And now 0-day reported it as well:
	https://lore.kernel.org/r/202204262306.mzMIKFKO-lkp@intel.com

Re: [PATCH v4 1/5] usb: host: export symbols for xhci-exynos to use xhci hooks

[Jung Daehwan]

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

On Tue, Apr 26, 2022 at 08:40:07PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 26, 2022 at 12:31:33PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 26, 2022 at 07:27:09PM +0900, Jung Daehwan wrote:
> > > On Tue, Apr 26, 2022 at 11:54:16AM +0200, Greg Kroah-Hartman wrote:
> > > > On Tue, Apr 26, 2022 at 06:18:44PM +0900, Daehwan Jung wrote:
> > > > > 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.
> > > > > 
> > > > 
> > > > For the static functions that you are now exporting, they need to have
> > > > their functions declared in a .h file.  If not, you now get warnings
> > > > when you run sparse after applying this commit, right?
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > > 
> > > 
> > > I didn't get warnings in my environment. I tested to build default config +
> > > xhci_exynos by make command. Is there compile option to check it?
> > 
> > make "C=1"
> 
> And now 0-day reported it as well:
> 	https://protect2.fireeye.com/v1/url?k=eb7c3909-8af72c33-eb7db246-74fe4860008a-ea28471ebd7c5af8&q=1&e=c40c7ad3-ab59-48ea-92a0-19c7ed0d5fd9&u=https%3A%2F%2Flore.kernel.org%2Fr%2F202204262306.mzMIKFKO-lkp%40intel.com
> 
>
I agree they should be added in header instead manually including xhci-exynos.
I will modify it on next submission.

I also don't get warnings in my environment when I run sparse.
Let me check my environment again.

Best Regards,
Jung Daehwan


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

Re: [PATCH v4 1/5] usb: host: export symbols for xhci-exynos to use xhci hooks

[kernel test robot]

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/20220426/202204262306.mzMIKFKO-lkp@intel.com/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/05a4937860cedb97b77200b7312a6f009aca2fc6
        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 05a4937860cedb97b77200b7312a6f009aca2fc6
        # 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 <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/usb/host/xhci-mem.c:68:6: warning: no previous prototype for 'xhci_segment_free' [-Wmissing-prototypes]
      68 | void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg)
         |      ^~~~~~~~~~~~~~~~~
   drivers/usb/host/xhci-mem.c:100:6: warning: no previous prototype for 'xhci_link_segments' [-Wmissing-prototypes]
     100 | void xhci_link_segments(struct xhci_segment *prev,
         |      ^~~~~~~~~~~~~~~~~~
>> drivers/usb/host/xhci-mem.c:261:6: warning: no previous prototype for 'xhci_remove_stream_mapping' [-Wmissing-prototypes]
     261 | void xhci_remove_stream_mapping(struct xhci_ring *ring)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/usb/host/xhci-mem.c:1974:5: warning: no previous prototype for 'xhci_check_trb_in_td_math' [-Wmissing-prototypes]
    1974 | int xhci_check_trb_in_td_math(struct xhci_hcd *xhci)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~


vim +/xhci_remove_stream_mapping +261 drivers/usb/host/xhci-mem.c

   260	
 > 261	void xhci_remove_stream_mapping(struct xhci_ring *ring)
   262	{
   263		struct xhci_segment *seg;
   264	
   265		if (WARN_ON_ONCE(ring->trb_address_map == NULL))
   266			return;
   267	
   268		seg = ring->first_seg;
   269		do {
   270			xhci_remove_segment_mapping(ring->trb_address_map, seg);
   271			seg = seg->next;
   272		} while (seg != ring->first_seg);
   273	}
   274	EXPORT_SYMBOL_GPL(xhci_remove_stream_mapping);
   275	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH v4 2/5] usb: host: add xhci hooks for xhci-exynos

[Daehwan Jung]

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 <dh10.jung@samsung.com>
Signed-off-by: J. Avila <elavila@google.com>
Signed-off-by: Puma Hsu <pumahsu@google.com>
Signed-off-by: Howard Yen <howardyen@google.com>
---
 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

Re: [PATCH v4 2/5] usb: host: add xhci hooks for xhci-exynos

[Greg Kroah-Hartman]

On Tue, Apr 26, 2022 at 06:18:45PM +0900, Daehwan Jung wrote:
> 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 <dh10.jung@samsung.com>
> Signed-off-by: J. Avila <elavila@google.com>
> Signed-off-by: Puma Hsu <pumahsu@google.com>
> Signed-off-by: Howard Yen <howardyen@google.com>
> ---
>  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(-)

Why do you need to "override" anything?  Why can't these just be added
to the current xhci_plat_priv structure and used that way like the
current xhci platform interface works?

> 
> 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);

Shouldn't the function have spit out an error if there was a problem?

> +
>  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);

Why the three options here?  Why does ep_index matter?

And no documentiaon for these global function?

thanks,

greg k-h

Re: [PATCH v4 2/5] usb: host: add xhci hooks for xhci-exynos

[Jung Daehwan]

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

On Tue, Apr 26, 2022 at 12:19:17PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 26, 2022 at 06:18:45PM +0900, Daehwan Jung wrote:
> > 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 <dh10.jung@samsung.com>
> > Signed-off-by: J. Avila <elavila@google.com>
> > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > Signed-off-by: Howard Yen <howardyen@google.com>
> > ---
> >  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(-)
> 
> Why do you need to "override" anything?  Why can't these just be added
> to the current xhci_plat_priv structure and used that way like the
> current xhci platform interface works?
> 

"override" means above xhci hooks? Above hooks are for ring management.
In fact, xhci platform doesn't care ring management. That's why I've added hooks
not used xhci_plat_priv.

> > 
> > 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);
> 
> Shouldn't the function have spit out an error if there was a problem?

It just reads and sync information about device context. That's why I think
it's not critical to go error routime. But it needs to discuss.

> 
> > +
> >  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);
> 
> Why the three options here?  Why does ep_index matter?
> 
They are implemented in xhci-mem.c but they're used by other files.
ep_index could be deleted. Thanks.

> And no documentiaon for these global function?
> 

I thought there's no need to add documentation. They are just functions to call
vendor ops and there's documentation of vendor ops above. I could add it if needed.

Best Regards,
Jung Daehwan

> thanks,
> 
> greg k-h
> 

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

Re: [PATCH v4 2/5] usb: host: add xhci hooks for xhci-exynos

[Greg Kroah-Hartman]

On Wed, Apr 27, 2022 at 06:06:17PM +0900, Jung Daehwan wrote:
> On Tue, Apr 26, 2022 at 12:19:17PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 26, 2022 at 06:18:45PM +0900, Daehwan Jung wrote:
> > > 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 <dh10.jung@samsung.com>
> > > Signed-off-by: J. Avila <elavila@google.com>
> > > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > > Signed-off-by: Howard Yen <howardyen@google.com>
> > > ---
> > >  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(-)
> > 
> > Why do you need to "override" anything?  Why can't these just be added
> > to the current xhci_plat_priv structure and used that way like the
> > current xhci platform interface works?
> > 
> 
> "override" means above xhci hooks? Above hooks are for ring management.
> In fact, xhci platform doesn't care ring management. That's why I've added hooks
> not used xhci_plat_priv.

Why not add ring management ability to the platform interface instead?
That's what you want to control here, in your platform driver, right?

> > > 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);
> > 
> > Shouldn't the function have spit out an error if there was a problem?
> 
> It just reads and sync information about device context. That's why I think
> it's not critical to go error routime. But it needs to discuss.

Ok, it looks like this follows the other ways this driver works, that's
fine.

> > And no documentiaon for these global function?
> > 
> 
> I thought there's no need to add documentation. They are just functions to call
> vendor ops and there's documentation of vendor ops above. I could add it if needed.

Always try to add documentation for when you want others to use the new
functions, as it helps explain how to use them.

thanks,

greg k-h

Re: [PATCH v4 2/5] usb: host: add xhci hooks for xhci-exynos

[Jung Daehwan]

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

On Wed, Apr 27, 2022 at 11:19:25AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 27, 2022 at 06:06:17PM +0900, Jung Daehwan wrote:
> > On Tue, Apr 26, 2022 at 12:19:17PM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Apr 26, 2022 at 06:18:45PM +0900, Daehwan Jung wrote:
> > > > 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 <dh10.jung@samsung.com>
> > > > Signed-off-by: J. Avila <elavila@google.com>
> > > > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > > > Signed-off-by: Howard Yen <howardyen@google.com>
> > > > ---
> > > >  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(-)
> > > 
> > > Why do you need to "override" anything?  Why can't these just be added
> > > to the current xhci_plat_priv structure and used that way like the
> > > current xhci platform interface works?
> > > 
> > 
> > "override" means above xhci hooks? Above hooks are for ring management.
> > In fact, xhci platform doesn't care ring management. That's why I've added hooks
> > not used xhci_plat_priv.
> 
> Why not add ring management ability to the platform interface instead?
> That's what you want to control here, in your platform driver, right?
>

I think xhci platform driver is separated from ring management. I don't
want to disturb current design.

> > > > 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);
> > > 
> > > Shouldn't the function have spit out an error if there was a problem?
> > 
> > It just reads and sync information about device context. That's why I think
> > it's not critical to go error routime. But it needs to discuss.
> 
> Ok, it looks like this follows the other ways this driver works, that's
> fine.
> 

OK. Thanks.

> > > And no documentiaon for these global function?
> > > 
> > 
> > I thought there's no need to add documentation. They are just functions to call
> > vendor ops and there's documentation of vendor ops above. I could add it if needed.
> 
> Always try to add documentation for when you want others to use the new
> functions, as it helps explain how to use them.
> 

OK. I will add it on next submission.

Best Regards,
Jung Daehwan

> thanks,
> 
> greg k-h
> 

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

Re: [PATCH v4 2/5] usb: host: add xhci hooks for xhci-exynos

[Greg Kroah-Hartman]

On Thu, Apr 28, 2022 at 12:23:54PM +0900, Jung Daehwan wrote:
> On Wed, Apr 27, 2022 at 11:19:25AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Apr 27, 2022 at 06:06:17PM +0900, Jung Daehwan wrote:
> > > On Tue, Apr 26, 2022 at 12:19:17PM +0200, Greg Kroah-Hartman wrote:
> > > > On Tue, Apr 26, 2022 at 06:18:45PM +0900, Daehwan Jung wrote:
> > > > > 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 <dh10.jung@samsung.com>
> > > > > Signed-off-by: J. Avila <elavila@google.com>
> > > > > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > > > > Signed-off-by: Howard Yen <howardyen@google.com>
> > > > > ---
> > > > >  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(-)
> > > > 
> > > > Why do you need to "override" anything?  Why can't these just be added
> > > > to the current xhci_plat_priv structure and used that way like the
> > > > current xhci platform interface works?
> > > > 
> > > 
> > > "override" means above xhci hooks? Above hooks are for ring management.
> > > In fact, xhci platform doesn't care ring management. That's why I've added hooks
> > > not used xhci_plat_priv.
> > 
> > Why not add ring management ability to the platform interface instead?
> > That's what you want to control here, in your platform driver, right?
> >
> 
> I think xhci platform driver is separated from ring management. I don't
> want to disturb current design.

You are adding platform-specific changes to the driver, that seems like
it would belong in the platform interface instead of requiring another
api/interface to be created within the same driver, right?

Anyway, just trying to make it simpler overall.

thanks,

greg k-h

[PATCH v4 3/5] usb: host: xhci-plat: support override of hc driver

[Daehwan Jung]

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 <dh10.jung@samsung.com>
---
 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

Re: [PATCH v4 3/5] usb: host: xhci-plat: support override of hc driver

[Greg Kroah-Hartman]

On Tue, Apr 26, 2022 at 06:18:46PM +0900, Daehwan Jung wrote:
> 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 <dh10.jung@samsung.com>
> ---
>  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);

EXPORT_SYMBOL_GPL() please.

thanks,

greg k-h

Re: [PATCH v4 3/5] usb: host: xhci-plat: support override of hc driver

[Jung Daehwan]

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

On Tue, Apr 26, 2022 at 12:20:07PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 26, 2022 at 06:18:46PM +0900, Daehwan Jung wrote:
> > 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 <dh10.jung@samsung.com>
> > ---
> >  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);
> 
> EXPORT_SYMBOL_GPL() please.
> 
> thanks,
> 
> greg k-h
> 

OK. I will modify it on next submission.

Best Regards,
Jung Daehwan

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

[PATCH v4 4/5] usb: host: add some to xhci overrides for xhci-exynos

[Daehwan Jung]

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 <dh10.jung@samsung.com>
---
 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

Re: [PATCH v4 4/5] usb: host: add some to xhci overrides for xhci-exynos

[Greg Kroah-Hartman]

On Tue, Apr 26, 2022 at 06:18:47PM +0900, Daehwan Jung wrote:
> 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 <dh10.jung@samsung.com>
> ---
>  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);

You do not use this function in this change, why include it in here?

Please reorganize your patch series to only include what you need for
each step, as-is it's kind of out-of-order and might not build at each
step along the way (or it might, it's hard to determine...)

thanks,

greg k-h

Re: [PATCH v4 4/5] usb: host: add some to xhci overrides for xhci-exynos

[Jung Daehwan]

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

On Tue, Apr 26, 2022 at 12:23:30PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 26, 2022 at 06:18:47PM +0900, Daehwan Jung wrote:
> > 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 <dh10.jung@samsung.com>
> > ---
> >  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);
> 
> You do not use this function in this change, why include it in here?
> 
> Please reorganize your patch series to only include what you need for
> each step, as-is it's kind of out-of-order and might not build at each
> step along the way (or it might, it's hard to determine...)
> 

This patch is to add function pointers. xhci-exynos or other user could use it.
If I reorganize them as you said, all patches depend on xhci-exynos.
That's because hooks and override are only used in xhci-exynos for now.
I don't want user driver effects common files like xhci platform.
I've tried removing dependancies.. That's why I split patches like that.

Best Regards,
Jung Daehwan.

> thanks,
> 
> greg k-h
> 

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

Re: [PATCH v4 4/5] usb: host: add some to xhci overrides for xhci-exynos

[Greg Kroah-Hartman]

On Wed, Apr 27, 2022 at 06:19:01PM +0900, Jung Daehwan wrote:
> On Tue, Apr 26, 2022 at 12:23:30PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 26, 2022 at 06:18:47PM +0900, Daehwan Jung wrote:
> > > 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 <dh10.jung@samsung.com>
> > > ---
> > >  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);
> > 
> > You do not use this function in this change, why include it in here?
> > 
> > Please reorganize your patch series to only include what you need for
> > each step, as-is it's kind of out-of-order and might not build at each
> > step along the way (or it might, it's hard to determine...)
> > 
> 
> This patch is to add function pointers. xhci-exynos or other user could use it.

But this commit has nothing to do with xhci_address_device(), why add it
to the .h file here?

> If I reorganize them as you said, all patches depend on xhci-exynos.

Of course, we need real users of the functions you export, otherwise
they do not need to be exported.

You can export them in one commit, and then use them in the next, but
that's not what you are doing here at all.  You have a mix of .c and .h
changes across different commits that are not coordinated at all.

> That's because hooks and override are only used in xhci-exynos for now.
> I don't want user driver effects common files like xhci platform.
> I've tried removing dependancies.. That's why I split patches like that.

Splitting patches is the correct way, just do it so that it makes sense.
As you can see by the kbuild reports, what you have here is not correct.

thanks,

greg k-h

[PATCH v4 5/5] usb: host: add xhci-exynos driver

[Daehwan Jung]

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 <dh10.jung@samsung.com>
---
 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 <dh10.jung@samsung.com>
+ *
+ * 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 <dh10.jung@samsung.com>
+ *
+ * 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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Greg Kroah-Hartman]

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 <dh10.jung@samsung.com>
> ---
>  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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Jung Daehwan]

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

On Tue, Apr 26, 2022 at 12:20:56PM +0200, Greg Kroah-Hartman wrote:
> 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 <dh10.jung@samsung.com>
> > ---
> >  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
> 

Yes. I will do it on next submission.

Best Regards,
Jung Daehwan

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Greg Kroah-Hartman]

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Jung Daehwan]

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

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.
Let me declare in xhci.h instead in below patch.

[PATCH v4 1/5] usb: host: export symbols for xhci-exynos to use xhci hooks

Best Regards,
Jung Daehwan.

> thanks,
> 
> greg k-h
> 

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Greg Kroah-Hartman]

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Krzysztof Kozlowski]

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Jung Daehwan]

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

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.

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.
I thought I just used existing compltible not adding new one.
I will add them if needed.

> 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? 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,
Jung Daehwan

> Best regards,
> Krzysztof
> 

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Krzysztof Kozlowski]

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Jung Daehwan]

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

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?

Best Regards,
Jung Daehwan

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

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Greg Kroah-Hartman]

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Jung Daehwan]

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

On Thu, Apr 28, 2022 at 08:45:49AM +0200, Greg Kroah-Hartman wrote:
> 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.
> 

We have several drivers including this and some drivers depends on other
drivers. I can't submit all drivers at the same time and It would be harder
if I did. That's why I submitted only patches in xhci without any dependancy.
I wanted to submit our all drivers one by one after then.
I will add it on next submission. New patch would be about dwc3-exynos.c not
in xhci.

I'm sorry to bother you but I hope you wouldn't think it's waste of time..

Best Regards,
Jung Daehwan

> thanks,
> 
> greg k-h
> 

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Krzysztof Kozlowski]

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Jung Daehwan]

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

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?

Best Regards,
Jung Daehwan

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Krzysztof Kozlowski]

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[kernel test robot]

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/202204270151.wJnsbJBF-lkp@intel.com/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 <lkp@intel.com>

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Mathias Nyman]

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 <dh10.jung@samsung.com>

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Jung Daehwan]

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

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 <dh10.jung@samsung.com>
> 
> 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.

Yes. Only one USB device is supported as you said. I'm going to modify
it following normal sequence from 2nd 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

Yes. it's right to return error here.

> 
> Also this whole function never allocates or remaps any memory.

This fuctions is for link segments. Right below function(xhci_ring_alloc_uram)
allocates.

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

Best Regards,
Jung Deahwan.
> 
> Thanks
> Mathias
> 

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Mathias Nyman]

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 <dh10.jung@samsung.com>
>>
>> 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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Jung Daehwan]

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

On Thu, Apr 28, 2022 at 03:28:49PM +0300, Mathias Nyman wrote:
> 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 <dh10.jung@samsung.com>
> >>
> >> 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.
> 

When I re-check it, it has a problem as you said.
I will modify it on next submission. Thanks.

Best Regards,
Jung Daehwan

> > 
> >>
> >>> +		}
> >>> +		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
> 

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

Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

[Jung Daehwan]

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

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 <dh10.jung@samsung.com>
> 
> 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.  
> 
> (...)
> 

I missed replying here on previous one.
OK. It seems to need more detailed explanation. I will explain my driver
more detail on next submission.

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

I missed it also on previous mail.
I've never seen a device that has several isoc endpoints.
It needs to add something if it would exist.

In fact, I've found a device that has feedback ep and I'm going to add
it. I've never seen other case yet.

Best Regards,
Jung Daehwan

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

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

Re: [PATCH v4 0/5] add xhci-exynos driver

[Greg Kroah-Hartman]

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

Re: [PATCH v4 0/5] add xhci-exynos driver

[Krzysztof Kozlowski]

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

Re: [PATCH v4 0/5] add xhci-exynos driver

[Jung Daehwan]

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

On Tue, Apr 26, 2022 at 02:46:14PM +0200, Krzysztof Kozlowski wrote:
> 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

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/*

> 
> 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/
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM,commit_signer:170/184=92%)
Mathias Nyman <mathias.nyman@linux.intel.com> (commit_signer:52/184=28%,authored:25/184=14%)
Alan Stern <stern@rowland.harvard.edu> (commit_signer:30/184=16%)
Chunfeng Yun <chunfeng.yun@mediatek.com> (commit_signer:23/184=12%,authored:21/184=11%)
linux-usb@vger.kernel.org (open list:USB SUBSYSTEM)
linux-kernel@vger.kernel.org (open list)

> 

In fact, I manually tried adding you as you commendted previous patchset.
But, It seems you changed email id and domain..

Best Regards,
Jung Daehwan

> 
> Best regards,
> Krzysztof
> 

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

Re: [PATCH v4 0/5] add xhci-exynos driver

[Krzysztof Kozlowski]

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 <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM,commit_signer:170/184=92%)
> Mathias Nyman <mathias.nyman@linux.intel.com> (commit_signer:52/184=28%,authored:25/184=14%)
> Alan Stern <stern@rowland.harvard.edu> (commit_signer:30/184=16%)
> Chunfeng Yun <chunfeng.yun@mediatek.com> (commit_signer:23/184=12%,authored:21/184=11%)
> linux-usb@vger.kernel.org (open list:USB SUBSYSTEM)
> linux-kernel@vger.kernel.org (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 <krzysztof.kozlowski@linaro.org>
(maintainer:ARM/SAMSUNG S3C, S5P AND EXYNOS ARM
ARCHITECTURES,authored:1/7=14%,added_lines:4/25=16%,removed_lines:2/13=15%)

linux-arm-kernel@lists.infradead.org (moderated list:ARM/SAMSUNG S3C,
S5P AND EXYNOS ARM ARCHITECTURES)

linux-samsung-soc@vger.kernel.org (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

Re: [PATCH v4 0/5] add xhci-exynos driver

[Jung Daehwan]

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

On Wed, Apr 27, 2022 at 08:24:04PM +0200, Krzysztof Kozlowski wrote:
> 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 <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM,commit_signer:170/184=92%)
> > Mathias Nyman <mathias.nyman@linux.intel.com> (commit_signer:52/184=28%,authored:25/184=14%)
> > Alan Stern <stern@rowland.harvard.edu> (commit_signer:30/184=16%)
> > Chunfeng Yun <chunfeng.yun@mediatek.com> (commit_signer:23/184=12%,authored:21/184=11%)
> > linux-usb@vger.kernel.org (open list:USB SUBSYSTEM)
> > linux-kernel@vger.kernel.org (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 <krzysztof.kozlowski@linaro.org>
> (maintainer:ARM/SAMSUNG S3C, S5P AND EXYNOS ARM
> ARCHITECTURES,authored:1/7=14%,added_lines:4/25=16%,removed_lines:2/13=15%)
> 
> linux-arm-kernel@lists.infradead.org (moderated list:ARM/SAMSUNG S3C,
> S5P AND EXYNOS ARM ARCHITECTURES)
> 
> linux-samsung-soc@vger.kernel.org (open list:ARM/SAMSUNG S3C, S5P AND
> EXYNOS ARM ARCHITECTURES)
> 
> 

I got it. I could find your name only on my last patch.
(v4-0005-usb-host-add-xhci-exynos-driver.patch)

When I submitted 1st(v1) patchset, there's no xhci-exynos driver patch.
That's why I couldn't find your name there. I've added it since v2 but I haven't 
got maintainer again but just use previous one. It's my fault.
Let me correct it on next submission. Thanks for the comment.

Best Regards,
Jung Daehwan

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

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