diff mbox series

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

Message ID 1650964728-175347-6-git-send-email-dh10.jung@samsung.com (mailing list archive)
State New, archived
Headers show
Series add xhci-exynos driver | expand

Commit Message

Jung Daehwan April 26, 2022, 9:18 a.m. UTC
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

Comments

Greg Kroah-Hartman April 26, 2022, 10:20 a.m. UTC | #1
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
Greg Kroah-Hartman April 26, 2022, 10:21 a.m. UTC | #2
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
Krzysztof Kozlowski April 26, 2022, 12:59 p.m. UTC | #3
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
kernel test robot April 26, 2022, 5:55 p.m. UTC | #4
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
Jung Daehwan April 27, 2022, 9:24 a.m. UTC | #5
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
>
Greg Kroah-Hartman April 27, 2022, 9:37 a.m. UTC | #6
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
Mathias Nyman April 27, 2022, 4:25 p.m. UTC | #7
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
Jung Daehwan April 28, 2022, 1:29 a.m. UTC | #8
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
>
Jung Daehwan April 28, 2022, 3:03 a.m. UTC | #9
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
>
Jung Daehwan April 28, 2022, 3:26 a.m. UTC | #10
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
Jung Daehwan April 28, 2022, 5:15 a.m. UTC | #11
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
>
Krzysztof Kozlowski April 28, 2022, 5:19 a.m. UTC | #12
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
Jung Daehwan April 28, 2022, 6:36 a.m. UTC | #13
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
>
Greg Kroah-Hartman April 28, 2022, 6:45 a.m. UTC | #14
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
Krzysztof Kozlowski April 28, 2022, 7:31 a.m. UTC | #15
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
Jung Daehwan April 28, 2022, 7:45 a.m. UTC | #16
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
>
Jung Daehwan April 28, 2022, 7:53 a.m. UTC | #17
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
Krzysztof Kozlowski April 28, 2022, 8:26 a.m. UTC | #18
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
Mathias Nyman April 28, 2022, 12:28 p.m. UTC | #19
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
Jung Daehwan May 3, 2022, 8:41 a.m. UTC | #20
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
>
diff mbox series

Patch

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