Re: usb: dwc2: regression on MyBook Live Duo / Canyonlands since 4.3.0-rc4

[John Youn]

On 5/14/2016 6:11 AM, Christian Lamparter wrote:
> On Thursday, May 12, 2016 11:40:28 AM John Youn wrote:
>> On 5/12/2016 6:30 AM, Christian Lamparter wrote:
>>> On Thursday, May 12, 2016 01:55:44 PM Arnd Bergmann wrote:
>>>> On Thursday 12 May 2016 11:58:18 Christian Lamparter wrote:
>>>>>>>> Detecting the endianess of the
>>>>>>>> device is probably the best future-proof solution, but it's also
>>>>>>>> considerably more work to do in the driver, and comes with a
>>>>>>>> tiny runtime overhead.
>>>>>>>
>>>>>>> The runtime overhead is probably non-measurable compared with the cost
>>>>>>> of the actual MMIOs.
>>>>>>
>>>>>> Right. The code size increase is probably measurable (but still small),
>>>>>> the runtime overhead is not.
>>>>>
>>>>> Ok, so no rebuts or complains have been posted.
>>>>>
>>>>> I've tested the patch you made in: https://lkml.org/lkml/2016/5/9/354
>>>>> and it works: 
>>>>>
>>>>> Tested-by: Christian Lamparter <chunkeey@xxxxxxxxxxxxxx>
>>>>>
>>>>> So, how do we go from here? There is are two small issues with the
>>>>> original patch (#ifdef DWC2_LOG_WRITES got converted to lower case:
>>>>> #ifdef dwc2_log_writes) and I guess a proper subject would be nice.  
>>>>>
>>>>> Arnd, can you please respin and post it (cc'd stable as well)?
>>>>> So this is can be picked up? Or what's your plan?
>>>>
>>>> (I just realized my reply was stuck in my outbox, so the patch
>>>> went out first)
>>>>
>>>> If I recall correctly, the rough consensus was to go with your longer
>>>> patch in the future (fixed up for the comments that BenH and
>>>> I sent), and I'd suggest basing it on top of a fixed version of
>>>> my patch.
>>> Well, but it comes with the "overhead"! So this was just as I said:
>>> "Let's look at it and see if it's any good"... And I think it isn't
>>> since the usb/host/ehci people also opted for #ifdef CONFIG_BIG_ENDIAN
>>> archs etc...
>>
>> I slightly prefer the more general patch for future kernel versions.
>> The overhead will probably be negligible, but we can perform some
>> testing to make sure.
>>
>> Can you resubmit with all gathered feedback?
> 
> Yes, here are the changes.
> 
> I've tested it on my MyBook Live Duo. The usbotg comes right up:
> [12610.540004] dwc2 4bff80000.usbotg: USB bus 1 deregistered
> [12612.513934] dwc2 4bff80000.usbotg: Specified GNPTXFDEP=1024 > 256
> [12612.518756] dwc2 4bff80000.usbotg: EPs: 3, shared fifos, 2042 entries in SPRAM
> [12612.530112] dwc2 4bff80000.usbotg: DWC OTG Controller
> [12612.533948] dwc2 4bff80000.usbotg: new USB bus registered, assigned bus number 1
> [12612.540083] dwc2 4bff80000.usbotg: irq 33, io mem 0x00000000
> 
> John: Can you run some perf test with it?
> 
> I've based this on:
> 
> commit 6ea2fffc9057a67df1994d85a7c085d899eaa25a
> Author: Arnd Bergmann <arnd@xxxxxxxx>
> Date:   Fri May 13 15:52:27 2016 +0200
> 
>     usb: dwc2: fix regression on big-endian PowerPC/ARM systems
> 
> so naturally, it needs to be applied first.
> Most of the conversion work was done by the attached
> coccinelle semantic patches. 
> 
> I had to edit the __bic32 and __orr32 helpers by hand.
> As well as some debugfs code and stuff in gadget.c.
> 

Thanks Christian.

I'll keep this in our internal tree and send it to Felipe later. This
causes a bunch of conflicts that I have to fix up and I should do a
bit of testing as well.

And since there is a patch that fixes the regression this is can wait.

Regards,
John