linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: John Youn <John.Youn@synopsys.com>
To: Christian Lamparter <chunkeey@googlemail.com>,
	John Youn <John.Youn@synopsys.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Felipe Balbi <felipe.balbi@linux.intel.com>,
	"linux-mips@linux-mips.org" <linux-mips@linux-mips.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"a.seppala@gmail.com" <a.seppala@gmail.com>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>
Subject: Re: usb: dwc2: regression on MyBook Live Duo / Canyonlands since 4.3.0-rc4
Date: Thu, 12 May 2016 13:55:49 -0700	[thread overview]
Message-ID: <5734EDD5.7030204@synopsys.com> (raw)
In-Reply-To: <50455529.fZie4vOnRh@debian64>

On 5/12/2016 1:39 PM, 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@googlemail.com>
>>>>>
>>>>> 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 I think I can do that. But I would really like to get the
> regression out of the way. So for that: I back Arnd's patch.
> It explains the problem much better and doesn't kill MIPS 
> like the revert I was doing in my initial post to the MLs.
> Also, another bonus: his patch is suited to port to stable.
> 
> The auto-detection approach is not that easy to get right,
> given all the stuff that's going on with BE8, LE4, ... So
> can we have your "blessing" for Arnd's patch for now? since
> that way, I can base my patch on top of his work about the
> issues of endiannes? (Just say: ACK :) )
> 

I agree Arnd's patch is best for stable. We can also apply it to
mainline until we get the autodection working as well.

Unless Felipe has objections.


> Arnd: do you have a version with the #ifdef lower/uppercase
> fix? Or should I give it a try (and fail in a different way ;) )
>  
>>>> Felipe just had another idea, to change the endianess of the dwc2
>>>> block by setting a registers (if that exists). That would indeed
>>>> be preferable, then we can just revert the broken change that
>>>> went into 4.4 and backport that fix instead.
>>> Just a quick reply. I have the docs for the thing. There's something
>>> like that in GAHBCFG at Bit 24... BUT it only switches the endiannes
>>> for the DMA descriptors (which is not always used, there are devices
>>> with PIO only)! It doesn't deal with the MMIO access at all. 
>>
>> That's correct. It only affects descriptor endianness for DMA
>> descriptor mode of operation.
> 
> Ok. The funny thing is that for the MyBook Live Duo this setting might
> be important since the PLB_DMA engine is not part of the DWC library...
> Instead it's from IBM and operates in: Big Endian :-D.
> 

Are you sure the controller is using descriptor DMA? It's more likely
using buffer DMA which this setting doesn't affect. DWC2 doesn't
support Descriptor DMA in device mode on mainline yet. If it's a host
then it might be.

Regards,
John

  parent reply	other threads:[~2016-05-12 20:55 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-05-07 22:54 usb: dwc2: regression on MyBook Live Duo / Canyonlands since 4.3.0-rc4 Christian Lamparter
2016-05-08 10:40 ` Benjamin Herrenschmidt
2016-05-08 11:44   ` Christian Lamparter
2016-05-09  0:23     ` Benjamin Herrenschmidt
2016-05-09 10:36       ` Arnd Bergmann
2016-05-09 10:39         ` Felipe Balbi
2016-05-09 15:08           ` Arnd Bergmann
2016-05-09 19:06             ` Christian Lamparter
2016-05-09 20:10               ` Arnd Bergmann
2016-05-09 22:43               ` Benjamin Herrenschmidt
2016-05-09 22:37             ` Benjamin Herrenschmidt
2016-05-10  7:23               ` Arnd Bergmann
2016-05-12  9:58                 ` Christian Lamparter
2016-05-12 11:55                   ` Arnd Bergmann
2016-05-12 13:30                     ` Christian Lamparter
2016-05-12 18:40                       ` John Youn
2016-05-12 20:39                         ` Christian Lamparter
2016-05-12 20:50                           ` Arnd Bergmann
2016-05-12 20:55                           ` John Youn [this message]
2016-05-14 13:11                         ` Christian Lamparter
2016-05-14 19:45                           ` Arnd Bergmann
2016-05-17 23:50                           ` John Youn
2016-05-18 19:14                             ` Christian Lamparter
2016-05-18 21:09                               ` Arnd Bergmann
2016-05-19  0:36                               ` John Youn
2016-05-12 22:17                   ` Benjamin Herrenschmidt
2016-05-09 22:33           ` Benjamin Herrenschmidt
2016-05-09 14:02         ` Benjamin Herrenschmidt
2016-05-09 20:22         ` John Youn
2016-05-09 20:38           ` Arnd Bergmann
2016-05-09 21:11             ` John Youn
2016-05-09 21:30               ` Arnd Bergmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5734EDD5.7030204@synopsys.com \
    --to=john.youn@synopsys.com \
    --cc=a.seppala@gmail.com \
    --cc=arnd@arndb.de \
    --cc=benh@kernel.crashing.org \
    --cc=chunkeey@googlemail.com \
    --cc=felipe.balbi@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@linux-mips.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).