Date:   Thu, 28 Apr 2022 12:03:19 +0900
From:   Jung Daehwan <dh10.jung@...sung.com>
To:     Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc:     Mathias Nyman <mathias.nyman@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "open list:USB XHCI DRIVER" <linux-usb@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        Howard Yen <howardyen@...gle.com>,
        Jack Pham <jackp@...eaurora.org>,
        Puma Hsu <pumahsu@...gle.com>,
        "J . Avila" <elavila@...gle.com>, sc.suh@...sung.com,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
Subject: Re: [PATCH v4 5/5] usb: host: add xhci-exynos driver

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

Powered by blists - more mailing lists