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 |
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
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
On 26/04/2022 11:18, Daehwan Jung wrote: > This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat > driver mainly and extends some functions by xhci hooks and overrides. > > It supports USB Audio offload with Co-processor. It only cares DCBAA, > Device Context, Transfer Ring, Event Ring, and ERST. They are allocated > on specific address with xhci hooks. Co-processor could use them directly > without xhci driver after then. This does not look like developed in current Linux kernel, but something out-of-tree, with some other unknown modifications. This is not how the code should be developed. Please rebase on linux-next and drop any unrelated modifications (these which are not sent with this patchset). (...) > + > +static int xhci_exynos_suspend(struct device *dev) > +{ > + struct usb_hcd *hcd = dev_get_drvdata(dev); > + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > + > + /* TODO: AP sleep scenario*/ Shall the patchset be called RFC? > + > + return xhci_suspend(xhci, device_may_wakeup(dev)); > +} > + > +static int xhci_exynos_resume(struct device *dev) > +{ > + struct usb_hcd *hcd = dev_get_drvdata(dev); > + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > + int ret; > + > + /* TODO: AP resume scenario*/ > + > + ret = xhci_resume(xhci, 0); > + if (ret) > + return ret; > + > + pm_runtime_disable(dev); > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + > + return 0; > +} > + > +static const struct dev_pm_ops xhci_exynos_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(xhci_exynos_suspend, xhci_exynos_resume) > +}; > + > +MODULE_DESCRIPTION("xHCI Exynos Host Controller Driver"); > +MODULE_LICENSE("GPL"); You don't have list of compatibles (and missing bindings), driver definition, driver registration. Entire solution is not used - nothing calls xhci_exynos_vendor_init(), because nothign uses "ops". This does not work and it makes it impossible to test it. Please provide proper XHCI Exynos driver, assuming you need it and it is not part of regular Exynos XHCI drivers (DWC3 and so on). Best regards, Krzysztof
Hi Daehwan, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on usb/usb-testing] [also build test WARNING on krzk/for-next char-misc/char-misc-testing v5.18-rc4 next-20220426] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/intel-lab-lkp/linux/commits/Daehwan-Jung/usb-host-export-symbols-for-xhci-exynos-to-use-xhci-hooks/20220426-181936 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220427/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
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 >
On Wed, Apr 27, 2022 at 06:24:26PM +0900, Jung Daehwan wrote: > On Tue, Apr 26, 2022 at 12:21:40PM +0200, Greg Kroah-Hartman wrote: > > On Tue, Apr 26, 2022 at 06:18:48PM +0900, Daehwan Jung wrote: > > > +int xhci_check_trb_in_td_math(struct xhci_hcd *xhci); > > > +void xhci_segment_free(struct xhci_hcd *xhci, struct xhci_segment *seg); > > > +void xhci_link_segments(struct xhci_segment *prev, > > > + struct xhci_segment *next, > > > + enum xhci_ring_type type, bool chain_links); > > > +void xhci_initialize_ring_info(struct xhci_ring *ring, > > > + unsigned int cycle_state); > > > +void xhci_remove_stream_mapping(struct xhci_ring *ring); > > > > Why does your single driver have global functions? That should not be > > needed, right? > > > > And these are odd for a driver's namespace... > > > > Those are exported in 1st patches. it's not good to include here as you said. If you export a function, you also have to add it to a .h file somewhere, otherwise it really is not exported. thanks, greg k-h
On 26.4.2022 12.18, Daehwan Jung wrote: > This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat > driver mainly and extends some functions by xhci hooks and overrides. > > It supports USB Audio offload with Co-processor. It only cares DCBAA, > Device Context, Transfer Ring, Event Ring, and ERST. They are allocated > on specific address with xhci hooks. Co-processor could use them directly > without xhci driver after then. > > Signed-off-by: Daehwan Jung <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
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 >
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 >
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
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 >
On 28/04/2022 03:29, Jung Daehwan wrote: > On Tue, Apr 26, 2022 at 02:59:57PM +0200, Krzysztof Kozlowski wrote: >> On 26/04/2022 11:18, Daehwan Jung wrote: >>> This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat >>> driver mainly and extends some functions by xhci hooks and overrides. >>> >>> It supports USB Audio offload with Co-processor. It only cares DCBAA, >>> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated >>> on specific address with xhci hooks. Co-processor could use them directly >>> without xhci driver after then. >> >> This does not look like developed in current Linux kernel, but something >> out-of-tree, with some other unknown modifications. This is not how the >> code should be developed. Please rebase on linux-next and drop any >> unrelated modifications (these which are not sent with this patchset). >> > > I've been developing on linux-next and I rebase before submitting. > Could you tell me one of dropped modifications or patches? > >> (...) >> >>> + >>> +static int xhci_exynos_suspend(struct device *dev) >>> +{ >>> + struct usb_hcd *hcd = dev_get_drvdata(dev); >>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd); >>> + >>> + /* TODO: AP sleep scenario*/ >> >> Shall the patchset be called RFC? >> > OK. I will add RFC for this patch on next submission. > >>> + >>> + return xhci_suspend(xhci, device_may_wakeup(dev)); >>> +} >>> + >>> +static int xhci_exynos_resume(struct device *dev) >>> +{ >>> + struct usb_hcd *hcd = dev_get_drvdata(dev); >>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd); >>> + int ret; >>> + >>> + /* TODO: AP resume scenario*/ >>> + >>> + ret = xhci_resume(xhci, 0); >>> + if (ret) >>> + return ret; >>> + >>> + pm_runtime_disable(dev); >>> + pm_runtime_set_active(dev); >>> + pm_runtime_enable(dev); >>> + >>> + return 0; >>> +} >>> + >>> +static const struct dev_pm_ops xhci_exynos_pm_ops = { >>> + SET_SYSTEM_SLEEP_PM_OPS(xhci_exynos_suspend, xhci_exynos_resume) >>> +}; >>> + >>> +MODULE_DESCRIPTION("xHCI Exynos Host Controller Driver"); >>> +MODULE_LICENSE("GPL"); >> >> You don't have list of compatibles (and missing bindings), driver >> definition, driver registration. Entire solution is not used - nothing >> calls xhci_exynos_vendor_init(), because nothign uses "ops". >> > > xhci_exynos_vendor_init is called in xhci-plat.c (xhci_vendor_init) > [v4,2/5] usb: host: add xhci hooks for xhci-exynos > ops are used in some files(xhci-mem.c, xhci.c ..) and the body of ops is in > all xhci-exynos. Nothing uses the "ops" except xhci_exynos_register_vendor_ops() which is not called anywhere, so the xhci_vendor_init() does not call xhci_exynos_vendor_init(). > > xhci-exynos is not a standalone driver. It could be enabled when other module > makes xhci platform driver probed as it uses xhci platform mainly. It "could be" or "will be"? We do not talk here about theoretical usage of the driver, but a real one. > I thought I just used existing compltible not adding new one. > I will add them if needed. Since you called everything here as "exynos" it is specific to one hardware and not-reusable on anything else. How can then you use some other compatible? It would be a misuse of Devicetree bindings. Unless this is not specific to "exynos", but then please remove any exynos and vendor references and make it integrated in generic XHCI working for all drivers. >> This does not work and it makes it impossible to test it. Please provide >> proper XHCI Exynos driver, assuming you need it and it is not part of >> regular Exynos XHCI drivers (DWC3 and so on). >> > > What makes you think it doesn't work? Not possible to probe. The code cannot be loaded. > I think it's almost proper. I just removed > other IPs code like Co-Processor(we call it abox) or Power Management because > it would make build-error. I've added hooking points in some files(xhci-mem.c, > xhci.c..) and ops are implemented in xhci-exynos. It's mainly operated with > xhci platform driver. Best regards, Krzysztof
On 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 >
On Thu, Apr 28, 2022 at 03:36:34PM +0900, Jung Daehwan wrote: > On Thu, Apr 28, 2022 at 07:19:04AM +0200, Krzysztof Kozlowski wrote: > > On 28/04/2022 03:29, Jung Daehwan wrote: > > > On Tue, Apr 26, 2022 at 02:59:57PM +0200, Krzysztof Kozlowski wrote: > > >> On 26/04/2022 11:18, Daehwan Jung wrote: > > >>> This driver is for Samsung Exynos xhci host conroller. It uses xhci-plat > > >>> driver mainly and extends some functions by xhci hooks and overrides. > > >>> > > >>> It supports USB Audio offload with Co-processor. It only cares DCBAA, > > >>> Device Context, Transfer Ring, Event Ring, and ERST. They are allocated > > >>> on specific address with xhci hooks. Co-processor could use them directly > > >>> without xhci driver after then. > > >> > > >> This does not look like developed in current Linux kernel, but something > > >> out-of-tree, with some other unknown modifications. This is not how the > > >> code should be developed. Please rebase on linux-next and drop any > > >> unrelated modifications (these which are not sent with this patchset). > > >> > > > > > > I've been developing on linux-next and I rebase before submitting. > > > Could you tell me one of dropped modifications or patches? > > > > > >> (...) > > >> > > >>> + > > >>> +static int xhci_exynos_suspend(struct device *dev) > > >>> +{ > > >>> + struct usb_hcd *hcd = dev_get_drvdata(dev); > > >>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > >>> + > > >>> + /* TODO: AP sleep scenario*/ > > >> > > >> Shall the patchset be called RFC? > > >> > > > OK. I will add RFC for this patch on next submission. > > > > > >>> + > > >>> + return xhci_suspend(xhci, device_may_wakeup(dev)); > > >>> +} > > >>> + > > >>> +static int xhci_exynos_resume(struct device *dev) > > >>> +{ > > >>> + struct usb_hcd *hcd = dev_get_drvdata(dev); > > >>> + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > > >>> + int ret; > > >>> + > > >>> + /* TODO: AP resume scenario*/ > > >>> + > > >>> + ret = xhci_resume(xhci, 0); > > >>> + if (ret) > > >>> + return ret; > > >>> + > > >>> + pm_runtime_disable(dev); > > >>> + pm_runtime_set_active(dev); > > >>> + pm_runtime_enable(dev); > > >>> + > > >>> + return 0; > > >>> +} > > >>> + > > >>> +static const struct dev_pm_ops xhci_exynos_pm_ops = { > > >>> + SET_SYSTEM_SLEEP_PM_OPS(xhci_exynos_suspend, xhci_exynos_resume) > > >>> +}; > > >>> + > > >>> +MODULE_DESCRIPTION("xHCI Exynos Host Controller Driver"); > > >>> +MODULE_LICENSE("GPL"); > > >> > > >> You don't have list of compatibles (and missing bindings), driver > > >> definition, driver registration. Entire solution is not used - nothing > > >> calls xhci_exynos_vendor_init(), because nothign uses "ops". > > >> > > > > > > xhci_exynos_vendor_init is called in xhci-plat.c (xhci_vendor_init) > > > [v4,2/5] usb: host: add xhci hooks for xhci-exynos > > > ops are used in some files(xhci-mem.c, xhci.c ..) and the body of ops is in > > > all xhci-exynos. > > > > > > Nothing uses the "ops" except xhci_exynos_register_vendor_ops() which is > > not called anywhere, so the xhci_vendor_init() does not call > > xhci_exynos_vendor_init(). > > > > You are right. xhci_exynos_register_vendor_ops should be called by other > module. It's only thing not called anywhere in this patchset. I don't uses > xhci-exynos alone in my scenario. Other module loads this on runtime. > > > > > > > xhci-exynos is not a standalone driver. It could be enabled when other module > > > makes xhci platform driver probed as it uses xhci platform mainly. > > > > It "could be" or "will be"? We do not talk here about theoretical usage > > of the driver, but a real one. > > > > > I thought I just used existing compltible not adding new one. > > > I will add them if needed. > > > > Since you called everything here as "exynos" it is specific to one > > hardware and not-reusable on anything else. How can then you use some > > other compatible? It would be a misuse of Devicetree bindings. > > > > I got it. Let me add them. Is it still necessary if it is only used by > other module on runtime as I said above? Please submit a full, working driver so these changes can be able to be properly reviewed. Otherwise it is just a waste of time for us to even read them, right? We do not add changes to the kernel that do not work or do anything, that would be pointless, and cause us extra work and maintenance. thanks, greg k-h
On 28/04/2022 08:36, Jung Daehwan wrote: >> >> Since you called everything here as "exynos" it is specific to one >> hardware and not-reusable on anything else. How can then you use some >> other compatible? It would be a misuse of Devicetree bindings. >> > > I got it. Let me add them. Is it still necessary if it is only used by > other module on runtime as I said above? Except what Greg wrote, if by "other module" you mean out-of-tree, then the patchset will not be accepted as it is unusable for Linux users. Basically it would be a dead code in Linux kernel. Best regards, Krzysztof
On 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 >
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
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
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
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 --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);
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