[PATCH v2] USB: ehci-pci: Add support for Intel Quark X1000 USB host controller

[PATCH v2] USB: ehci-pci: Add support for Intel Quark X1000 USB host controller

[Chen, Alvin]

From: "Alvin (Weike) Chen" <alvin.chen@intel.com>

Hi,
Intel Quark X1000 consists of one USB host controller which can be PCI enumerated.
And the exsiting EHCI-PCI framework supports it with the default packet buffer in/out
threshold. We reconfigure the in/out threshold as maximal as possible.

Bryan O'Donoghue (1):
  USB: ehci-pci: USB host controller support for Intel Quark X1000

 drivers/usb/host/ehci-pci.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

-- 
1.7.9.5

[PATCH v2] USB: ehci-pci: USB host controller support for Intel Quark X1000

[Chen, Alvin]

From: Bryan O'Donoghue <bryan.odonoghue@intel.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.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@intel.com>
Signed-off-by: Alvin (Weike) Chen <alvin.chen@intel.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;
+
+}
+
+/* Register offset of in/out threshold */
+#define EHCI_INSNREG01		    0x84
+/* The maximal threshold is 0x80 Dword */
+#define EHCI_MAX_THRESHOLD      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))
+static void usb_set_qrk_bulk_thresh(struct pci_dev *pdev)
+{
+	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

Re: [PATCH v2] USB: ehci-pci: USB host controller support for Intel Quark X1000

[Jingoo Han]

On Friday, June 27, 2014 8:25 PM, Alvin Chen wrote:
> 
> From: Bryan O'Donoghue <bryan.odonoghue@intel.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@intel.com>
> Signed-off-by: Alvin (Weike) Chen <alvin.chen@intel.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

Re: [PATCH v2] USB: ehci-pci: USB host controller support for Intel Quark X1000

[Jingoo Han]

On Friday, June 27, 2014 1:33 PM, Jingoo Han wrote:
> On Friday, June 27, 2014 8:25 PM, Alvin Chen wrote:
> >
> > From: Bryan O'Donoghue <bryan.odonoghue@intel.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@intel.com>
> > Signed-off-by: Alvin (Weike) Chen <alvin.chen@intel.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);

Sorry, please refer to 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_508BYTES << EHCI_THRESHOLD_OUT_SHIFT |
		(EHCI_THRESHOLD_508BYTES << 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

RE: [PATCH v2] USB: ehci-pci: USB host controller support for Intel Quark X1000

[Chen, Alvin]

> > > 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?
> >
Yes, to maximize the performance, we set the threshold as maximal as possible. Intel Quark X1000 can support 0x80 dwords (512Bytes), but 0x7F dwords (508 Bytes) should be used since the isochronous/interrupt
transactions can be initiated by Intel Quark X1000 USB host controller. I will update the descriptions.

> > > /*------------------------------------------------------------------
> > > -------*/
> > > +#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.
OK, I will remove it.

> >
> > > +}
> > > +
> > > +/* 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);
> 
> Sorry, please refer to 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_508BYTES <<
> EHCI_THRESHOLD_OUT_SHIFT |
> 		(EHCI_THRESHOLD_508BYTES << EHCI_THRESHOLD_IN_SHIFT);
> 	writel(val, op_reg_base + EHCI_INSNREG01);
> 
I will improve the according to your suggestions.

RE: [PATCH v2] USB: ehci-pci: USB host controller support for Intel Quark X1000

[David Laight]

From: Jingoo Han

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

It would be better to define these using expressions. So:
#define EHCI_THRESHOLD_512BYTES		(512u / 8u)
#define EHCI_THRESHOLD_508BYTES		(508u / 8u)

Then you might decide to use:
#define EHCI_THRESHOLD(size) ((size) / 8u)

Then realise that the names are not generic EHCI, so need some
driver-specific prefix (for namespace reasons).

And that the defines are probably limit values, and should
be named as such.

	David

RE: [PATCH v2] USB: ehci-pci: USB host controller support for Intel Quark X1000

[Chen, Alvin]

> -----Original Message-----
> From: David Laight [mailto:David.Laight@ACULAB.COM]
> Sent: Friday, June 27, 2014 4:08 PM
> ...
> > /* The maximal threshold value is 0x80, which means 512 bytes */
> > #define EHCI_THRESHOLD_512BYTES		0x80
> > #define EHCI_THRESHOLD_508BYTES		0x79
> 
> It would be better to define these using expressions. So:
> #define EHCI_THRESHOLD_512BYTES		(512u / 8u)
> #define EHCI_THRESHOLD_508BYTES		(508u / 8u)
> 
Just to clarify, the threshold value set to register is in dword, so '0x80' means 0x80 dwords (512 bytes), and 508 bytes is not 0x79, it is 0x7F.

> Then you might decide to use:
> #define EHCI_THRESHOLD(size) ((size) / 8u)
> 
> Then realise that the names are not generic EHCI, so need some driver-specific
> prefix (for namespace reasons).
> 
How about the following?
#define INTEL_QUARK_X1000_EHCI_THRESHOLD(size) ((size) / 4u)  /* threshold value is in dword*/

> And that the defines are probably limit values, and should be named as such.
> 
> 	David
> 
> 

Re: [PATCH v2] USB: ehci-pci: USB host controller support for Intel Quark X1000

[Alan Stern]

On Fri, 27 Jun 2014, Chen, Alvin wrote:

> From: Bryan O'Donoghue <bryan.odonoghue@intel.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.

This is still incomplete.  _Why_ do you want to increase the threshold?  
Does it fix a problem?  Does it improve performance?

Also, the lines are too long.  They should be wrapped before 80 
columns.

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@intel.com>
> Signed-off-by: Alvin (Weike) Chen <alvin.chen@intel.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;
> +
> +}

The "usb_" prefix in the name isn't needed, because this is a static 
routine.

> +
> +/* Register offset of in/out threshold */
> +#define EHCI_INSNREG01		    0x84
> +/* The maximal threshold is 0x80 Dword */
> +#define EHCI_MAX_THRESHOLD      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))

This looks a little peculiar, because someone reading the code would
expect you to use the maximal threshold.

Also the formatting isn't quite right.  There should be a space before 
the closing */ of a comment, and there should be spaces around the << 
operator.

> +static void usb_set_qrk_bulk_thresh(struct pci_dev *pdev)
> +{
> +	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);
> +}

This is much more complicted that it needs to be.  When this routine 
runs, the controller has already been memory-mapped.  All you need to 
do is:

	ehci_writel(ehci, EHCI_INSNREG01_THRESH,
			ehci->regs->insnreg01_thresh);

Since it's only one statement, you don't even need to put this in a
separate function.  It can go directly into ehci_pci_reinit().

Also, in include/linux/usb/ehci_defs.h, you'll have to add:

#define insnreg01_thresh	hostpc[0]

with an explanatory comment, near the definition of the HOSTPC 
register.

>  /* 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;
>  }

Alan Stern

RE: [PATCH v2] USB: ehci-pci: USB host controller support for Intel Quark X1000

[Chen, Alvin]

> > 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.
> 
> This is still incomplete.  _Why_ do you want to increase the threshold?
> Does it fix a problem?  Does it improve performance?
Try to set threshold as maximal as possible to maximize the performance. I will update the descriptions.

> Also, the lines are too long.  They should be wrapped before 80 columns.
Got you.
> > +#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;
> > +
> > +}
> 
> The "usb_" prefix in the name isn't needed, because this is a static routine.

OK, I will remove the 'usb_' prefix.



> > +static void usb_set_qrk_bulk_thresh(struct pci_dev *pdev) {
> > +	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);
> > +}
> 
> This is much more complicted that it needs to be.  When this routine runs, the
> controller has already been memory-mapped.  All you need to do is:
> 
> 	ehci_writel(ehci, EHCI_INSNREG01_THRESH,
> 			ehci->regs->insnreg01_thresh);
> 
> Since it's only one statement, you don't even need to put this in a separate
> function.  It can go directly into ehci_pci_reinit().
OK, I will remove ' usb_set_qrk_bulk_thresh', and change the code as the suggestions.

> Also, in include/linux/usb/ehci_defs.h, you'll have to add:
> 
> #define insnreg01_thresh	hostpc[0]
I will add it in ehci-pci.c instead of /linux/usb/ehci_defs.h, because it is not a generic micro, but just used for Intel Quark X1000.

> with an explanatory comment, near the definition of the HOSTPC register.
>