From: Mathias Nyman
> Sent: 12 June 2017 15:49
Commenting on this copy because it is handy...
> On 09.06.2017 05:33, Jiahau Chang wrote:
> > v2 : fix coding format
> >
> > When USB Ethernet is plugged in ASMEDIA ASM1042A xHCI host, bad
> > performance was manifesting in Web browser use (like download
> > large file such as ISO image). It is known limitation of
> > ASM1042A that is not compatible with driver scheduling,
> > As a workaround we can modify flow control handling of ASM1042A.
I think you need to be more explicit both in the commit message
and in the code about what the actual problem is, and what the
actual effect of the control register writes is.
> >
> > Signed-off-by: Jiahau Chang <Lars_chang@xxxxxxxxxxxxxx>
> > ---
> > drivers/usb/host/pci-quirks.c | 62 +++++++++++++++++++++++++++++++++++++++++++
> > drivers/usb/host/pci-quirks.h | 1 +
> > drivers/usb/host/xhci-pci.c | 5 ++++
> > drivers/usb/host/xhci.c | 3 +++
> > drivers/usb/host/xhci.h | 1 +
> > 5 files changed, 72 insertions(+)
> >
> > diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> > index a9a1e4c..bdeaf75 100644
> > --- a/drivers/usb/host/pci-quirks.c
> > +++ b/drivers/usb/host/pci-quirks.c
> > @@ -77,6 +77,12 @@
> > #define USB_INTEL_USB3_PSSEN 0xD8
> > #define USB_INTEL_USB3PRM 0xDC
> >
> > +/*ASMEDIA quirk use*/
> > +#define ASMT_DATA_WRITE0_REG 0xF8
> > +#define ASMT_DATA_WRITE1_REG 0xFC
> > +#define ASMT_CONTROL_REG 0xE0
> > +#define ASMT_CONTROL_WRITE_BIT 0x02
> > +
> > /*
> > * amd_chipset_gen values represent AMD different chipset generations
> > */
> > @@ -412,6 +418,62 @@ void usb_amd_quirk_pll_disable(void)
> > }
> > EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_disable);
> >
> > +void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev)
> > +{
> > + u32 value_low, value_high;
> > + unsigned char value;
> > + unsigned long wait_time_count;
> > +
> > + wait_time_count = 1000;
> > + while (wait_time_count) {
> > + pci_read_config_byte(pdev, ASMT_CONTROL_REG, &value);
> > + if (value == 0xff) {
> > + dev_dbg(&pdev->dev, "%s: wait_write_ready IO_ERROR, value=%x\n",
> > + __func__, value);
>
> Printing value=%x isn't very useful here. We know it's 0xff.
>
> > + goto err_exit;
No need for else after goto.
> > + } else if ((value & ASMT_CONTROL_WRITE_BIT) == 0) {
> > + break;
> > + }
> > + wait_time_count--;
> > + udelay(50);
>
> 1000 * 50us = 50ms delay
> plus another 50ms later on equals 100ms worst case.
>
> If that is what it takes then it can't be helped.
If this is a probe function, can it at least be arranged to sleep?
>
> > + }
> > + if (wait_time_count == 0) {
> > + dev_dbg(&pdev->dev, "%s: wait_write_ready timeout\n",
> > + __func__);
> > + goto err_exit;
> > + }
> > + value_low = 0x00010423;
> > + value_high = 0xFA30;
> > + pci_write_config_dword(pdev, ASMT_DATA_WRITE0_REG, value_low);
> > + pci_write_config_dword(pdev, ASMT_DATA_WRITE1_REG, value_high);
> > + pci_write_config_byte(pdev, ASMT_CONTROL_REG, ASMT_CONTROL_WRITE_BIT);
>
> From here onwards:
>
> > + wait_time_count = 1000;
> > + while (wait_time_count) {
> > + pci_read_config_byte(pdev, ASMT_CONTROL_REG, &value);
> > + if (value == 0xff) {
> > + dev_dbg(&pdev->dev, "%s: wait_write_ready IO_ERROR, value=%x\n",
> > + __func__, value);
> > + goto err_exit;
> > + } else if ((value & ASMT_CONTROL_WRITE_BIT) == 0) {
> > + break;
> > + }
> > + wait_time_count--;
> > + udelay(50);
> > + }
> > + if (wait_time_count == 0) {
> > + dev_dbg(&pdev->dev, "%s: wait_write_ready timeout\n",
> > + __func__);
> > + goto err_exit;
>
> To here, we have the exact same code duplicated as the first loop.
> Maybe restructure the code it a bit to avoid that.
>
> Even the debug messages are exactly the same, easier to debug if they were a bit
> different.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
