Re: [PATCH v5 4/5] xhci: mediatek: support MTK xHCI host controller

From: Mathias Nyman <mathias.nyman@linux.intel.com>
To: Chunfeng Yun <chunfeng.yun@mediatek.com>,
	Mathias Nyman <mathias.nyman@intel.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Felipe Balbi <balbi@ti.com>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Roger Quadros <rogerq@ti.com>,
	linux-usb@vger.kernel.org, linux-mediatek@lists.infradead.org,
	John Crispin <blogic@openwrt.org>,
	Daniel Kurtz <djkurtz@chromium.org>,
	Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>,
	Kishon Vijay Abraham I <kishon@ti.com>
Subject: Re: [PATCH v5 4/5] xhci: mediatek: support MTK xHCI host controller
Date: Mon, 17 Aug 2015 18:07:35 +0300	[thread overview]
Message-ID: <55D1F8B7.9060306@linux.intel.com> (raw)
In-Reply-To: <1438950644-27290-5-git-send-email-chunfeng.yun@mediatek.com>

Hi

On 07.08.2015 15:30, Chunfeng Yun wrote:
> MTK xhci host controller defines some extra SW scheduling
> parameters for HW to minimize the scheduling effort for
> synchronous and interrupt endpoints. The parameters are
> put into reseved DWs of slot context and endpoint context
>
>

...

> + * The TD size is the number of max packet sized packets remaining in the TD
> + * (including this TRB), right shifted by 10.
> + * It must fit in bits 21:17, so it can't be bigger than 31.
> + */
> +u32 xhci_mtk_td_remainder_quirk(unsigned int td_running_total,
> +	unsigned trb_buffer_length, struct urb *urb)
> +{
> +	u32 max = 31;
> +	int remainder, td_packet_count, packet_transferred;
> +	unsigned int td_transfer_size = urb->transfer_buffer_length;
> +	unsigned int maxp;
> +
> +	maxp = GET_MAX_PACKET(usb_endpoint_maxp(&urb->ep->desc));
> +
> +	/* 0 for the last TRB */
> +	if (td_running_total + trb_buffer_length == td_transfer_size)
> +		return 0;
> +
> +	packet_transferred = td_running_total / maxp;
> +	td_packet_count = DIV_ROUND_UP(td_transfer_size, maxp);
> +	remainder = td_packet_count - packet_transferred;
> +
> +	if (remainder > max)
> +		return max << 17;
> +	else
> +		return remainder << 17;
> +}

I started looking at this xhci_mtk_td_remainder() function, one
of the places this patch touches the existing xhci code.

The remainder functions in xhci are already bit too messy, and
adding one more function which does almost the same thing makes
it even messier.

For example queuing a bulk transfer will end up like this:

>   		/* Set the TRB length, TD size, and interrupter fields. */
>   		if (xhci->hci_version < 0x100) {
> -			remainder = xhci_td_remainder(
> +			if (xhci->quirks & XHCI_MTK_HOST) {
> +				remainder = xhci_mtk_td_remainder_quirk(
> +					running_total, trb_buff_len, urb);
> +			} else {
> +				remainder = xhci_td_remainder(
>   					urb->transfer_buffer_length -
>   					running_total);
> +			}
>   		} else {
>   			remainder = xhci_v1_0_td_remainder(running_total,
>   					trb_buff_len, total_packet_count, urb,

and similar for isoc and control transfers.

I'll see if I can simplify the existing remainder calculations into one function, then it should
be enough to just add something like this into it:

if (xhci->quirks & XHCI_MTK_HOST)
	trb_buff_len = 0;

This way we can skip all the extra hassle that comes with a new exported quirk function

Is the Mediatek xhci really a xHCI 0.96 or older controller? (hci_version < 0x100)
Not a xHCI 1.0 or 1.1 ?

-Mathias