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

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

[Sergei Shtylyov]

Hello.

On 06/24/2014 07:51 PM, Chen, Alvin wrote:

> From: Bryan O'Donoghue <bryan.odonoghue@intel.com>

> This patch is to enable USB host controller for Intel Quark X1000. Add pci quirks
> to adjust the packet buffer in/out threshold value, and ensure EHCI packet buffer
> i/o threshold value is reconfigured to half.

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@intel.com>
> Signed-off-by: Alvin (Weike) Chen <alvin.chen@intel.com>
> ---
>   drivers/usb/host/ehci-pci.c   |    4 ++++
>   drivers/usb/host/pci-quirks.c |   42 +++++++++++++++++++++++++++++++++++++++++
>   drivers/usb/host/pci-quirks.h |    2 ++
>   3 files changed, 48 insertions(+)

> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> index 3e86bf4..33cfa23 100644
> --- a/drivers/usb/host/ehci-pci.c
> +++ b/drivers/usb/host/ehci-pci.c
> @@ -50,6 +50,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(unlikely(usb_is_intel_qrk(pdev)))

    Please run your patch thru scripts/checkpatch.pl -- space needed after *if*.

> +		usb_set_qrk_bulk_thresh(pdev);
> +
>   	return 0;
>   }
>
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 00661d3..1ea8803 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -823,6 +823,48 @@ static int handshake(void __iomem *ptr, u32 mask, u32 done,
>   	return -ETIMEDOUT;
>   }
>
> +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC		0x0939
> +bool usb_is_intel_qrk(struct pci_dev *pdev)
> +{
> +	return pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +		pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
> +
> +}
> +EXPORT_SYMBOL_GPL(usb_is_intel_qrk);

    Why not use DECLARE_PCI_FIXUP_FINAL() instead?

> +
> +#define EHCI_INSNREG01		0x84
> +#define EHCI_INSNREG01_THRESH	0x007F007F	/* Threshold value */
> +void usb_set_qrk_bulk_thresh(struct pci_dev *pdev)
> +{
> +	void __iomem *base, *op_reg_base;
> +	u8 cap_length;
> +	u32 val;
> +
> +	if (!mmio_resource_enabled(pdev, 0))
> +		return;
> +
> +	base = pci_ioremap_bar(pdev, 0);
> +	if (base == NULL)
> +		return;
> +
> +	cap_length = readb(base);
> +	op_reg_base = base + cap_length;
> +
> +	val = readl(op_reg_base + EHCI_INSNREG01);
> +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);
> +
> +	val = EHCI_INSNREG01_THRESH;
> +
> +	writel(val, op_reg_base + EHCI_INSNREG01);
> +
> +	val = readl(op_reg_base + EHCI_INSNREG01);
> +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);

    I doubt these dev_printk() calls are really useful. But if the are, it's 
worth merging them into one call.

WBR, Sergei

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

[Greg Kroah-Hartman]

On Tue, Jun 24, 2014 at 08:51:43AM -0700, Chen, Alvin wrote:
> From: Bryan O'Donoghue <bryan.odonoghue@intel.com>
> 
> This patch is to enable USB host controller for Intel Quark X1000. Add pci quirks
> to adjust the packet buffer in/out threshold value, and ensure EHCI packet buffer
> i/o threshold value is reconfigured to half.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@intel.com>
> Signed-off-by: Alvin (Weike) Chen <alvin.chen@intel.com>
> ---
>  drivers/usb/host/ehci-pci.c   |    4 ++++
>  drivers/usb/host/pci-quirks.c |   42 +++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/host/pci-quirks.h |    2 ++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> index 3e86bf4..33cfa23 100644
> --- a/drivers/usb/host/ehci-pci.c
> +++ b/drivers/usb/host/ehci-pci.c
> @@ -50,6 +50,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(unlikely(usb_is_intel_qrk(pdev)))

Why unlikely()?  Have you measured a speed difference here?  And this is
a _very_ slow path, what does that speed difference you measured help
with?

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Tue, Jun 24, 2014 at 08:51:43AM -0700, Chen, Alvin wrote:
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 00661d3..1ea8803 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -823,6 +823,48 @@ static int handshake(void __iomem *ptr, u32 mask, u32 done,
>  	return -ETIMEDOUT;
>  }
>  
> +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC		0x0939
> +bool usb_is_intel_qrk(struct pci_dev *pdev)
> +{
> +	return pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +		pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
> +
> +}
> +EXPORT_SYMBOL_GPL(usb_is_intel_qrk);

There is no lack of vowels available to you, please use them.

greg k-h

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

[Alan Stern]

On Tue, 24 Jun 2014, Chen, Alvin wrote:

> From: Bryan O'Donoghue <bryan.odonoghue@intel.com>
> 
> This patch is to enable USB host controller for Intel Quark X1000. Add pci quirks
> to adjust the packet buffer in/out threshold value, and ensure EHCI packet buffer
> i/o threshold value is reconfigured to half.

What is the packet buffer in/out threshold value and why does it need
to be reconfigured to half?

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@intel.com>
> Signed-off-by: Alvin (Weike) Chen <alvin.chen@intel.com>
> ---
>  drivers/usb/host/ehci-pci.c   |    4 ++++
>  drivers/usb/host/pci-quirks.c |   42 +++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/host/pci-quirks.h |    2 ++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> index 3e86bf4..33cfa23 100644
> --- a/drivers/usb/host/ehci-pci.c
> +++ b/drivers/usb/host/ehci-pci.c
> @@ -50,6 +50,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(unlikely(usb_is_intel_qrk(pdev)))
> +		usb_set_qrk_bulk_thresh(pdev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 00661d3..1ea8803 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -823,6 +823,48 @@ static int handshake(void __iomem *ptr, u32 mask, u32 done,
>  	return -ETIMEDOUT;
>  }
>  
> +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC		0x0939
> +bool usb_is_intel_qrk(struct pci_dev *pdev)
> +{
> +	return pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +		pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
> +
> +}
> +EXPORT_SYMBOL_GPL(usb_is_intel_qrk);
> +
> +#define EHCI_INSNREG01		0x84
> +#define EHCI_INSNREG01_THRESH	0x007F007F	/* Threshold value */
> +void usb_set_qrk_bulk_thresh(struct pci_dev *pdev)
> +{
> +	void __iomem *base, *op_reg_base;
> +	u8 cap_length;
> +	u32 val;
> +
> +	if (!mmio_resource_enabled(pdev, 0))
> +		return;
> +
> +	base = pci_ioremap_bar(pdev, 0);
> +	if (base == NULL)
> +		return;
> +
> +	cap_length = readb(base);
> +	op_reg_base = base + cap_length;
> +
> +	val = readl(op_reg_base + EHCI_INSNREG01);
> +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);
> +
> +	val = EHCI_INSNREG01_THRESH;
> +
> +	writel(val, op_reg_base + EHCI_INSNREG01);
> +
> +	val = readl(op_reg_base + EHCI_INSNREG01);
> +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);

What good will these log messages do anybody?  Is there any reason not 
to make them debug messages?  Or even leave them out entirely, since 
you pretty much know beforehand what they're going to say?

> +
> +	iounmap(base);
> +
> +}
> +EXPORT_SYMBOL_GPL(usb_set_qrk_bulk_thresh);

None of this material belongs in pci-quirks.c.  Please move it into 
ehci-pci.c.

Alan Stern

[PATCH] 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.
But the exsiting EHCI-PCI framework doesn't support it. Thus, we enable it to support
Intel Quark X1000 USB host controller by adding pci quirks to configure buffer i/o threshold 
value to half.

Bryan O'Donoghue (1):
  Quark USB host

 drivers/usb/host/ehci-pci.c   |    4 ++++
 drivers/usb/host/pci-quirks.c |   42 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/pci-quirks.h |    2 ++
 3 files changed, 48 insertions(+)

-- 
1.7.9.5

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

[Chen, Alvin]

From: Bryan O'Donoghue <bryan.odonoghue@intel.com>

This patch is to enable USB host controller for Intel Quark X1000. Add pci quirks
to adjust the packet buffer in/out threshold value, and ensure EHCI packet buffer
i/o threshold value is reconfigured to half.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@intel.com>
Signed-off-by: Alvin (Weike) Chen <alvin.chen@intel.com>
---
 drivers/usb/host/ehci-pci.c   |    4 ++++
 drivers/usb/host/pci-quirks.c |   42 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/pci-quirks.h |    2 ++
 3 files changed, 48 insertions(+)

diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 3e86bf4..33cfa23 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -50,6 +50,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(unlikely(usb_is_intel_qrk(pdev)))
+		usb_set_qrk_bulk_thresh(pdev);
+
 	return 0;
 }
 
diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 00661d3..1ea8803 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -823,6 +823,48 @@ static int handshake(void __iomem *ptr, u32 mask, u32 done,
 	return -ETIMEDOUT;
 }
 
+#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC		0x0939
+bool usb_is_intel_qrk(struct pci_dev *pdev)
+{
+	return pdev->vendor == PCI_VENDOR_ID_INTEL &&
+		pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
+
+}
+EXPORT_SYMBOL_GPL(usb_is_intel_qrk);
+
+#define EHCI_INSNREG01		0x84
+#define EHCI_INSNREG01_THRESH	0x007F007F	/* Threshold value */
+void usb_set_qrk_bulk_thresh(struct pci_dev *pdev)
+{
+	void __iomem *base, *op_reg_base;
+	u8 cap_length;
+	u32 val;
+
+	if (!mmio_resource_enabled(pdev, 0))
+		return;
+
+	base = pci_ioremap_bar(pdev, 0);
+	if (base == NULL)
+		return;
+
+	cap_length = readb(base);
+	op_reg_base = base + cap_length;
+
+	val = readl(op_reg_base + EHCI_INSNREG01);
+	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);
+
+	val = EHCI_INSNREG01_THRESH;
+
+	writel(val, op_reg_base + EHCI_INSNREG01);
+
+	val = readl(op_reg_base + EHCI_INSNREG01);
+	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);
+
+	iounmap(base);
+
+}
+EXPORT_SYMBOL_GPL(usb_set_qrk_bulk_thresh);
+
 /*
  * Intel's Panther Point chipset has two host controllers (EHCI and xHCI) that
  * share some number of ports.  These ports can be switched between either
diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
index c622ddf..617c22b 100644
--- a/drivers/usb/host/pci-quirks.h
+++ b/drivers/usb/host/pci-quirks.h
@@ -14,6 +14,8 @@ void usb_amd_quirk_pll_enable(void);
 void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
 void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
 void sb800_prefetch(struct device *dev, int on);
+bool usb_is_intel_qrk(struct pci_dev *pdev);
+void usb_set_qrk_bulk_thresh(struct pci_dev *pdev);
 #else
 struct pci_dev;
 static inline void usb_amd_quirk_pll_disable(void) {}
-- 
1.7.9.5

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

[Jingoo Han]

On Wednesday, June 25, 2014 12:52 AM, Alvin Chen wrote:
> 
> From: Bryan O'Donoghue <bryan.odonoghue@intel.com>
> 
> This patch is to enable USB host controller for Intel Quark X1000. Add pci quirks
> to adjust the packet buffer in/out threshold value, and ensure EHCI packet buffer
> i/o threshold value is reconfigured to half.

Please add more detailed description. For example, why is it necessary to
reconfigure the threshold value?

> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@intel.com>
> Signed-off-by: Alvin (Weike) Chen <alvin.chen@intel.com>
> ---
>  drivers/usb/host/ehci-pci.c   |    4 ++++
>  drivers/usb/host/pci-quirks.c |   42 +++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/host/pci-quirks.h |    2 ++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> index 3e86bf4..33cfa23 100644
> --- a/drivers/usb/host/ehci-pci.c
> +++ b/drivers/usb/host/ehci-pci.c
> @@ -50,6 +50,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(unlikely(usb_is_intel_qrk(pdev)))
> +		usb_set_qrk_bulk_thresh(pdev);
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 00661d3..1ea8803 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -823,6 +823,48 @@ static int handshake(void __iomem *ptr, u32 mask, u32 done,
>  	return -ETIMEDOUT;
>  }
> 
> +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC		0x0939
> +bool usb_is_intel_qrk(struct pci_dev *pdev)
> +{
> +	return pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +		pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
> +
> +}
> +EXPORT_SYMBOL_GPL(usb_is_intel_qrk);
> +
> +#define EHCI_INSNREG01		0x84
> +#define EHCI_INSNREG01_THRESH	0x007F007F	/* Threshold value */

What does this magic number mean?
Would you make it more readable?

> +void usb_set_qrk_bulk_thresh(struct pci_dev *pdev)
> +{
> +	void __iomem *base, *op_reg_base;
> +	u8 cap_length;
> +	u32 val;
> +
> +	if (!mmio_resource_enabled(pdev, 0))
> +		return;
> +
> +	base = pci_ioremap_bar(pdev, 0);
> +	if (base == NULL)
> +		return;
> +
> +	cap_length = readb(base);
> +	op_reg_base = base + cap_length;
> +
> +	val = readl(op_reg_base + EHCI_INSNREG01);
> +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);
> +
> +	val = EHCI_INSNREG01_THRESH;
> +
> +	writel(val, op_reg_base + EHCI_INSNREG01);
> +
> +	val = readl(op_reg_base + EHCI_INSNREG01);
> +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);

As Alan Stern said, These INFO message are noisy.
DEBUG level looks better.

Best regards,
Jingoo Han

> +
> +	iounmap(base);
> +
> +}
> +EXPORT_SYMBOL_GPL(usb_set_qrk_bulk_thresh);
> +
>  /*
>   * Intel's Panther Point chipset has two host controllers (EHCI and xHCI) that
>   * share some number of ports.  These ports can be switched between either
> diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
> index c622ddf..617c22b 100644
> --- a/drivers/usb/host/pci-quirks.h
> +++ b/drivers/usb/host/pci-quirks.h
> @@ -14,6 +14,8 @@ void usb_amd_quirk_pll_enable(void);
>  void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
>  void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
>  void sb800_prefetch(struct device *dev, int on);
> +bool usb_is_intel_qrk(struct pci_dev *pdev);
> +void usb_set_qrk_bulk_thresh(struct pci_dev *pdev);
>  #else
>  struct pci_dev;
>  static inline void usb_amd_quirk_pll_disable(void) {}
> --
> 1.7.9.5

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

[Chen, Alvin]

See my comments below:

> > +	/* Reset the threshold limit */
> > +	if(unlikely(usb_is_intel_qrk(pdev)))
> 
>     Please run your patch thru scripts/checkpatch.pl -- space needed after
> *if*.
OK, I will improve it in the PATCH v2.

> 
> > +		usb_set_qrk_bulk_thresh(pdev);
> > +
> >   	return 0;
> >   }
> >
> > diff --git a/drivers/usb/host/pci-quirks.c
> > b/drivers/usb/host/pci-quirks.c index 00661d3..1ea8803 100644
> > --- a/drivers/usb/host/pci-quirks.c
> > +++ b/drivers/usb/host/pci-quirks.c
> > @@ -823,6 +823,48 @@ static int handshake(void __iomem *ptr, u32 mask,
> u32 done,
> >   	return -ETIMEDOUT;
> >   }
> >
> > +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC		0x0939
> > +bool usb_is_intel_qrk(struct pci_dev *pdev) {
> > +	return pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > +		pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
> > +
> > +}
> > +EXPORT_SYMBOL_GPL(usb_is_intel_qrk);
> 
>     Why not use DECLARE_PCI_FIXUP_FINAL() instead?
> 
Alan Stern suggests me to move 'usb_is_intel_qrk' and 'usb_set_qrk_bulk_thresh' to ehci-pci.c. 
Since no other modules call these two functions, I will move them to ehci-pci.c as static functions, so no necessary to export them out.

> > +
> > +#define EHCI_INSNREG01		0x84
> > +#define EHCI_INSNREG01_THRESH	0x007F007F	/* Threshold value */
> > +void usb_set_qrk_bulk_thresh(struct pci_dev *pdev) {
> > +	void __iomem *base, *op_reg_base;
> > +	u8 cap_length;
> > +	u32 val;
> > +
> > +	if (!mmio_resource_enabled(pdev, 0))
> > +		return;
> > +
> > +	base = pci_ioremap_bar(pdev, 0);
> > +	if (base == NULL)
> > +		return;
> > +
> > +	cap_length = readb(base);
> > +	op_reg_base = base + cap_length;
> > +
> > +	val = readl(op_reg_base + EHCI_INSNREG01);
> > +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);
> > +
> > +	val = EHCI_INSNREG01_THRESH;
> > +
> > +	writel(val, op_reg_base + EHCI_INSNREG01);
> > +
> > +	val = readl(op_reg_base + EHCI_INSNREG01);
> > +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);
> 
>     I doubt these dev_printk() calls are really useful. But if the are, it's worth
> merging them into one call.
Actually, the dev_printk calls is not necessary, I will remove them in the PATCH v2.

> 
> WBR, Sergei

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

[Chen, Alvin]

See my comments below:

> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, June 24, 2014 9:25 PM
> To: Chen, Alvin
> Cc: Alan Stern; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Ong,
> Boon Leong
> Subject: Re: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark
> X1000
> 
> On Tue, Jun 24, 2014 at 08:51:43AM -0700, Chen, Alvin wrote:
> > From: Bryan O'Donoghue <bryan.odonoghue@intel.com>
> >
> > This patch is to enable USB host controller for Intel Quark X1000. Add
> > pci quirks to adjust the packet buffer in/out threshold value, and
> > ensure EHCI packet buffer i/o threshold value is reconfigured to half.
> >
> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@intel.com>
> > Signed-off-by: Alvin (Weike) Chen <alvin.chen@intel.com>
> > ---
> >  drivers/usb/host/ehci-pci.c   |    4 ++++
> >  drivers/usb/host/pci-quirks.c |   42
> +++++++++++++++++++++++++++++++++++++++++
> >  drivers/usb/host/pci-quirks.h |    2 ++
> >  3 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> > index 3e86bf4..33cfa23 100644
> > --- a/drivers/usb/host/ehci-pci.c
> > +++ b/drivers/usb/host/ehci-pci.c
> > @@ -50,6 +50,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(unlikely(usb_is_intel_qrk(pdev)))
> 
> Why unlikely()?  Have you measured a speed difference here?  And this is a
> _very_ slow path, what does that speed difference you measured help with?
> 
Actually, 'unlikely' is not necessary, I will remove it in PATCH v2.

> thanks,
> 
> greg k-h

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

[Chen, Alvin]

> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, June 24, 2014 9:25 PM
> To: Chen, Alvin
> Cc: Alan Stern; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Ong,
> Boon Leong
> Subject: Re: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark
> X1000
> 
> On Tue, Jun 24, 2014 at 08:51:43AM -0700, Chen, Alvin wrote:
> > diff --git a/drivers/usb/host/pci-quirks.c
> > b/drivers/usb/host/pci-quirks.c index 00661d3..1ea8803 100644
> > --- a/drivers/usb/host/pci-quirks.c
> > +++ b/drivers/usb/host/pci-quirks.c
> > @@ -823,6 +823,48 @@ static int handshake(void __iomem *ptr, u32 mask,
> u32 done,
> >  	return -ETIMEDOUT;
> >  }
> >
> > +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC		0x0939
> > +bool usb_is_intel_qrk(struct pci_dev *pdev) {
> > +	return pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > +		pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
> > +
> > +}
> > +EXPORT_SYMBOL_GPL(usb_is_intel_qrk);
> 
> There is no lack of vowels available to you, please use them.
> 

OK, I will change ' usb_is_intel_qrk ' to ' usb_is_intel_quark'.

> greg k-h

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

[David Laight]

From: Chen, Alvin
> > On Tue, Jun 24, 2014 at 08:51:43AM -0700, Chen, Alvin wrote:
> > > diff --git a/drivers/usb/host/pci-quirks.c
> > > b/drivers/usb/host/pci-quirks.c index 00661d3..1ea8803 100644
> > > --- a/drivers/usb/host/pci-quirks.c
> > > +++ b/drivers/usb/host/pci-quirks.c
> > > @@ -823,6 +823,48 @@ static int handshake(void __iomem *ptr, u32 mask,
> > u32 done,
> > >  	return -ETIMEDOUT;
> > >  }
> > >
> > > +#define PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC		0x0939
> > > +bool usb_is_intel_qrk(struct pci_dev *pdev) {
> > > +	return pdev->vendor == PCI_VENDOR_ID_INTEL &&
> > > +		pdev->device == PCI_DEVICE_ID_INTEL_QUARK_X1000_SOC;
> > > +
> > > +}
> > > +EXPORT_SYMBOL_GPL(usb_is_intel_qrk);
> >
> > There is no lack of vowels available to you, please use them.
> >
> 
> OK, I will change ' usb_is_intel_qrk ' to ' usb_is_intel_quark'.

Or even usb_is_intel_quark_x1000() ?

	David

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

[Chen, Alvin]

> > This patch is to enable USB host controller for Intel Quark X1000. Add
> > pci quirks to adjust the packet buffer in/out threshold value, and
> > ensure EHCI packet buffer i/o threshold value is reconfigured to half.
> 
> Please add more detailed description. For example, why is it necessary to
> reconfigure the threshold value?
> 
This patch try to reconfigure the threshold value as maximal (0x7F DWord) as possible since the default value is just 0x20 DWord, and I will update the description.

> >
> > +
> > +#define EHCI_INSNREG01		0x84
> > +#define EHCI_INSNREG01_THRESH	0x007F007F	/* Threshold value */
> 
> What does this magic number mean?
> Would you make it more readable?
0x84 is the register offset, and the high word of '0x007F007F' is the out threshold and the low word is the in threshold. I will use some micros to make it more readable to avoid magic number in PATCH v2.



> > +void usb_set_qrk_bulk_thresh(struct pci_dev *pdev) {
> > +	void __iomem *base, *op_reg_base;
> > +	u8 cap_length;
> > +	u32 val;
> > +
> > +	if (!mmio_resource_enabled(pdev, 0))
> > +		return;
> > +
> > +	base = pci_ioremap_bar(pdev, 0);
> > +	if (base == NULL)
> > +		return;
> > +
> > +	cap_length = readb(base);
> > +	op_reg_base = base + cap_length;
> > +
> > +	val = readl(op_reg_base + EHCI_INSNREG01);
> > +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);
> > +
> > +	val = EHCI_INSNREG01_THRESH;
> > +
> > +	writel(val, op_reg_base + EHCI_INSNREG01);
> > +
> > +	val = readl(op_reg_base + EHCI_INSNREG01);
> > +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);
> 
> As Alan Stern said, These INFO message are noisy.
> DEBUG level looks better.
These messages are not necessary, I will remove them.



> Best regards,
> Jingoo Han
> 
> > +
> > --
> > 1.7.9.5

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

[Chen, Alvin]

> >
> > OK, I will change ' usb_is_intel_qrk ' to ' usb_is_intel_quark'.
> 
> Or even usb_is_intel_quark_x1000() ?
> 
OK, I will change the function name as your suggestion to make it more specific.

> 	David
> 
> 

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

[Chen, Alvin]

> > This patch is to enable USB host controller for Intel Quark X1000. Add 
>> pci quirks to adjust the packet buffer in/out threshold value, and 
> >ensure EHCI packet buffer i/o threshold value is reconfigured to half
>
>
> What is the packet buffer in/out threshold value and why does it need to be
> reconfigured to half?
> 
I go through the hardware specification carefully again. The EHCI buffer packet depth can be 512 bytes, and the in/out threshold is programmable and can be programmed to 512 bytes (0x80 DWord) only when isochronous/interrupt transactions are not initiated by the host controller. The default threshold value for Quark X1000 is 0x20 DWord, so this patch try to program it as maximal as possible, and it is 0x7F Dword since the host controller initiates isochronous/interrupt transactions .

I will update the description in PATCH v2.

> > +	val = readl(op_reg_base + EHCI_INSNREG01);
> > +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);
> > +
> > +	val = EHCI_INSNREG01_THRESH;
> > +
> > +	writel(val, op_reg_base + EHCI_INSNREG01);
> > +
> > +	val = readl(op_reg_base + EHCI_INSNREG01);
> > +	dev_printk(KERN_INFO, &pdev->dev, "INSNREG01 is 0x%08x\n", val);
> 
> What good will these log messages do anybody?  Is there any reason not to
> make them debug messages?  Or even leave them out entirely, since you
> pretty much know beforehand what they're going to say?
> 
These messages are not necessary, I will remove them.

> > +
> > +	iounmap(base);
> > +
> > +}
> > +EXPORT_SYMBOL_GPL(usb_set_qrk_bulk_thresh);
> 
> None of this material belongs in pci-quirks.c.  Please move it into ehci-pci.c.
> 

OK. I will move them to ehci-pci.c, since these two functions will not be called by other modules.

> Alan Stern