lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 27 Jun 2014 13:33:04 +0900
From:	Jingoo Han <jg1.han@...sung.com>
To:	"'Chen, Alvin'" <alvin.chen@...el.com>,
	'Alan Stern' <stern@...land.harvard.edu>
Cc:	'Greg Kroah-Hartman' <gregkh@...uxfoundation.org>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	'Sergei Shtylyov' <sergei.shtylyov@...entembedded.com>,
	'David Laight' <David.Laight@...LAB.COM>,
	'Boon Leong Ong' <boon.leong.ong@...el.com>,
	'Jingoo Han' <jg1.han@...sung.com>
Subject: Re: [PATCH v2] USB: ehci-pci: USB host controller support for Intel
 Quark X1000

On Friday, June 27, 2014 8:25 PM, Alvin Chen wrote:
> 
> From: Bryan O'Donoghue <bryan.odonoghue@...el.com>
> 
> The EHCI packet buffer in/out threshold is programmable for Intel Quark X1000 USB host
> controller, and the default value is 0x20 dwords. The in/out threshold can be programmed
> to 0x80 dwords, but only when isochronous/interrupt transactions are not initiated by
> the USB host controller. This patch is to reconfigure the packet buffer in/out threshold
> as maximal as possible, and it is 0x7F dwords since the USB host controller initiates
> isochronous/interrupt transactions.

So, what is the reason to set the value as 0x80?
  1. The default value 0x20 makes some problems such as...
  2. To maximize the performance, ...
  3. Both
Please add the reason why 0x80 is necessary, as possible.

Then, 0x7F means 508 bytes? 'Intel Quark X1000 USB host controller'
can support 0x80 (512bytes), however, in this case, isochronous/interrupt
transactions cannot be initiated by 'Intel Quark X1000 USB host controller'.
Right?

So, 0x79 (508bytes?) should be used, because the isochronous/interrupt
transactions can be initiated by 'Intel Quark X1000 USB host controller'.
Right?

> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@...el.com>
> Signed-off-by: Alvin (Weike) Chen <alvin.chen@...el.com>
> ---
> changelog v2:
> *Change the function name from 'usb_is_intel_qrk' to 'usb_is_intel_quark_x1000'.
> *Move the functions 'usb_is_intel_quark_x1000' and 'usb_set_qrk_bulk_thresh'
>  from 'pci-quirks.c' to 'ehci-pci.c'.
> *Remove unnecessary kernel message in the function of 'usb_set_qrk_bulk_thresh'.
> *Remove 'unlikely' in the functions of 'ehci_pci_reinit'.
> *Add white space after 'if'.
> *Update the descriptions to make it more clearly.
> *Add Micros to avoid magic number.
> 
>  drivers/usb/host/ehci-pci.c |   45 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> index 3e86bf4..ca29f34 100644
> --- a/drivers/usb/host/ehci-pci.c
> +++ b/drivers/usb/host/ehci-pci.c
> @@ -35,6 +35,47 @@ static const char hcd_name[] = "ehci-pci";
>  #define PCI_DEVICE_ID_INTEL_CE4100_USB	0x2e70
> 
>  /*-------------------------------------------------------------------------*/
> +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC		0x0939
> +static inline bool usb_is_intel_quark_x1000(struct pci_dev *pdev)
> +{
> +	return pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +		pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
> +

Please don't add this unnecessary line.

> +}
> +
> +/* Register offset of in/out threshold */
> +#define EHCI_INSNREG01		    0x84
> +/* The maximal threshold is 0x80 Dword */
> +#define EHCI_MAX_THRESHOLD      0X80

s/0X80/0x80

0x80 means 512 bytes. So, how about mentioning '0x80 means 512 bytes'
in this comment or the commit message?

Maybe, how about the following?

/* The maximal threshold value is 0x80, which means 512 bytes */
#define EHCI_THRESHOLD_512BYTES		0x80

> +/* Lower word is 'in' threshold, and higher word is 'out' threshold*/
> +#define EHCI_INSNREG01_THRESH \
> +	((EHCI_MAX_THRESHOLD - 1)<<16 | (EHCI_MAX_THRESHOLD - 1))

Um, how about the following?

/* Register offset of in/out threshold */
#define EHCI_INSNREG01			0x84
/* The maximal threshold value is 0x80, which means 512 bytes */
#define EHCI_THRESHOLD_512BYTES		0x80
#define EHCI_THRESHOLD_508BYTES		0x79
#define EHCI_THRESHOLD_OUT_SHIFT		16
#define EHCI_THRESHOLD_IN_SHIFT		0

	......

	/*
	 * In order to support the isochronous/interrupt transactions,
	 * 508 bytes should be used as max threshold values */
	 */
	val = 	((EHCI_THRESHOLD_512BYTES - 1) << EHCI_THRESHOLD_OUT_SHIFT |
		(EHCI_THRESHOLD_512BYTES - 1) << EHCI_THRESHOLD_IN_SHIFT);
	writel(val, op_reg_base + EHCI_INSNREG01);


> +static void usb_set_qrk_bulk_thresh(struct pci_dev *pdev)

Please, use usb_set_qrk_bulk_threshold().
The 'threshold' looks better than 'thresh'.

> +{
> +	void __iomem *base, *op_reg_base;
> +	u8 cap_length;
> +	u32 val;
> +	u16 cmd;
> +
> +	if (!pci_resource_start(pdev, 0))
> +		return;
> +
> +	if (pci_read_config_word(pdev, PCI_COMMAND, &cmd)
> +		|| !(cmd & PCI_COMMAND_MEMORY))
> +		return;
> +
> +	base = pci_ioremap_bar(pdev, 0);
> +	if (base == NULL)
> +		return;
> +
> +	cap_length = readb(base);
> +	op_reg_base = base + cap_length;
> +
> +	val = EHCI_INSNREG01_THRESH;
> +	writel(val, op_reg_base + EHCI_INSNREG01);
> +
> +	iounmap(base);
> +}
> 
>  /* called after powerup, by probe or system-pm "wakeup" */
>  static int ehci_pci_reinit(struct ehci_hcd *ehci, struct pci_dev *pdev)
> @@ -50,6 +91,10 @@ static int ehci_pci_reinit(struct ehci_hcd *ehci, struct pci_dev *pdev)
>  	if (!retval)
>  		ehci_dbg(ehci, "MWI active\n");
> 
> +	/* Reset the threshold limit */
> +	if (usb_is_intel_quark_x1000(pdev))
> +		usb_set_qrk_bulk_thresh(pdev);
> +
>  	return 0;
>  }
> 
> --
> 1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists