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.
> 

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