* Re: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-24 15:51 ` [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000 Chen, Alvin
@ 2014-06-24 12:38 ` Sergei Shtylyov
2014-06-25 9:22 ` Chen, Alvin
2014-06-24 13:24 ` Greg Kroah-Hartman
` (3 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Sergei Shtylyov @ 2014-06-24 12:38 UTC (permalink / raw)
To: Chen, Alvin, Alan Stern, Greg Kroah-Hartman
Cc: linux-usb, linux-kernel, Boon Leong Ong
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
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-24 12:38 ` Sergei Shtylyov
@ 2014-06-25 9:22 ` Chen, Alvin
0 siblings, 0 replies; 14+ messages in thread
From: Chen, Alvin @ 2014-06-25 9:22 UTC (permalink / raw)
To: Sergei Shtylyov, Alan Stern, Greg Kroah-Hartman
Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
Ong, Boon Leong
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-24 15:51 ` [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000 Chen, Alvin
2014-06-24 12:38 ` Sergei Shtylyov
@ 2014-06-24 13:24 ` Greg Kroah-Hartman
2014-06-25 9:24 ` Chen, Alvin
2014-06-24 13:25 ` Greg Kroah-Hartman
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Greg Kroah-Hartman @ 2014-06-24 13:24 UTC (permalink / raw)
To: Chen, Alvin; +Cc: Alan Stern, linux-usb, linux-kernel, Boon Leong Ong
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
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-24 13:24 ` Greg Kroah-Hartman
@ 2014-06-25 9:24 ` Chen, Alvin
0 siblings, 0 replies; 14+ messages in thread
From: Chen, Alvin @ 2014-06-25 9:24 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Alan Stern, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, Ong, Boon Leong
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-24 15:51 ` [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000 Chen, Alvin
2014-06-24 12:38 ` Sergei Shtylyov
2014-06-24 13:24 ` Greg Kroah-Hartman
@ 2014-06-24 13:25 ` Greg Kroah-Hartman
2014-06-25 9:26 ` Chen, Alvin
2014-06-24 14:05 ` Alan Stern
2014-06-25 1:10 ` Jingoo Han
4 siblings, 1 reply; 14+ messages in thread
From: Greg Kroah-Hartman @ 2014-06-24 13:25 UTC (permalink / raw)
To: Chen, Alvin; +Cc: Alan Stern, linux-usb, linux-kernel, Boon Leong Ong
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
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-24 13:25 ` Greg Kroah-Hartman
@ 2014-06-25 9:26 ` Chen, Alvin
2014-06-25 9:37 ` David Laight
0 siblings, 1 reply; 14+ messages in thread
From: Chen, Alvin @ 2014-06-25 9:26 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Alan Stern, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, Ong, Boon Leong
> -----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
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-25 9:26 ` Chen, Alvin
@ 2014-06-25 9:37 ` David Laight
2014-06-26 1:17 ` Chen, Alvin
0 siblings, 1 reply; 14+ messages in thread
From: David Laight @ 2014-06-25 9:37 UTC (permalink / raw)
To: 'Chen, Alvin', Greg Kroah-Hartman
Cc: Alan Stern, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, Ong, Boon Leong
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
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-25 9:37 ` David Laight
@ 2014-06-26 1:17 ` Chen, Alvin
0 siblings, 0 replies; 14+ messages in thread
From: Chen, Alvin @ 2014-06-26 1:17 UTC (permalink / raw)
To: David Laight, Greg Kroah-Hartman
Cc: Alan Stern, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, Ong, Boon Leong
> >
> > 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
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-24 15:51 ` [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000 Chen, Alvin
` (2 preceding siblings ...)
2014-06-24 13:25 ` Greg Kroah-Hartman
@ 2014-06-24 14:05 ` Alan Stern
2014-06-26 5:24 ` Chen, Alvin
2014-06-25 1:10 ` Jingoo Han
4 siblings, 1 reply; 14+ messages in thread
From: Alan Stern @ 2014-06-24 14:05 UTC (permalink / raw)
To: Chen, Alvin; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Boon Leong Ong
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
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-24 14:05 ` Alan Stern
@ 2014-06-26 5:24 ` Chen, Alvin
0 siblings, 0 replies; 14+ messages in thread
From: Chen, Alvin @ 2014-06-26 5:24 UTC (permalink / raw)
To: Alan Stern
Cc: Greg Kroah-Hartman, linux-usb@vger.kernel.org,
linux-kernel@vger.kernel.org, Ong, Boon Leong
> > 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-24 15:51 ` [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000 Chen, Alvin
` (3 preceding siblings ...)
2014-06-24 14:05 ` Alan Stern
@ 2014-06-25 1:10 ` Jingoo Han
2014-06-26 1:16 ` Chen, Alvin
4 siblings, 1 reply; 14+ messages in thread
From: Jingoo Han @ 2014-06-25 1:10 UTC (permalink / raw)
To: 'Chen, Alvin'
Cc: 'Alan Stern', 'Greg Kroah-Hartman', linux-usb,
linux-kernel, 'Boon Leong Ong', '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
^ permalink raw reply [flat|nested] 14+ messages in thread* RE: [PATCH] USB: ehci-pci: USB host controller support for Intel Quark X1000
2014-06-25 1:10 ` Jingoo Han
@ 2014-06-26 1:16 ` Chen, Alvin
0 siblings, 0 replies; 14+ messages in thread
From: Chen, Alvin @ 2014-06-26 1:16 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Alan Stern', 'Greg Kroah-Hartman',
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
Ong, Boon Leong
> > 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
^ permalink raw reply [flat|nested] 14+ messages in thread