From mboxrd@z Thu Jan 1 00:00:00 1970 From: jbrunet@baylibre.com (Jerome Brunet) Date: Fri, 06 Jan 2017 11:11:36 +0100 Subject: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue In-Reply-To: <20170105232508.GU14217@n2100.armlinux.org.uk> References: <1480348229-25672-1-git-send-email-jbrunet@baylibre.com> <049b1efc-3bad-92e0-45ef-0563dc5d81de@gmail.com> <20170105232508.GU14217@n2100.armlinux.org.uk> Message-ID: <1483697496.28003.40.camel@baylibre.com> To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org On Thu, 2017-01-05 at 23:25 +0000, Russell King - ARM Linux wrote: > On Mon, Nov 28, 2016 at 09:54:28AM -0800, Florian Fainelli wrote: > > > > If we start supporting generic "enable", "disable" type of > > properties > > with values that map directly to register definitions of the HW, we > > leave too much room for these properties to be utilized to > > implement a > > specific policy, and this is not acceptable. > > Another concern with this patch is that the existing phylib "set_eee" > code is horribly buggy - it just translates the modes from userspace > into the register value and writes them directly to the register with > no validation.??So it's possible to set modes in the register that > the > hardware doesn't support, and have them advertised to the link > partner. Hi Russell, The purpose of this patch is to provide a way to mark as broken a particular eee mode. At first, it had nothing to do with "set_eee" but, as Florian rightly pointed out, users shouldn't be able to re-enable a broken mode. At first, I was thinking about returning -ENOSUP if a broken mode was requested. Then I noticed the behavior you just described: A user can request anything of "set_eee", he won't necessarily get what he asked but won't get an error either. To avoid mixing different topic in a single patch, I kept the same behavior, not returning an error, just silently discarding broken modes I agree with you, we should probably validate a bit more what we asked of the hardware in set_eee. I wonder if we should return an error though. With the current implementation, user space application could simply ask to activate everything, excepting the kernel to sort it out and return an error only if something horribly wrong happened. If we start returning an error for unsupported modes, we could break things. I guess we should just silently filter the requested modes. > > I have a patch which fixes that, restricting (as we do elsewhere) the > advert according to the EEE supported capabilities retrieved from the > PCS Could be interesting :) > - maybe the problem here is that the PCS doesn't support support > EEE in 1000baseT mode? It does, and that's kind of the problem. EEE in ON for 100Tx and 1000T by default with this PHY. I have several platform with the same MAC-PHY combination. Only the OdroidC2 shows this particular issue with 1000T- EEE As explained in other mails in this thread. The problem does not come from the MAC entering LPI. It actually comes from the link partner entering LPI on the Rx path under significant Tx transfer. For some reason, this completely mess up our PHY. > > Out of interest, which PHY is used on this platform? The PHY is the Realtek RTL8211F > > On the SolidRun boards, they're using AR8035, and have suffered this > occasional link drop problem.??What has been found is that it seems > to > be to do with the timing parameters, and it seemed to only be 1000bT > that was affected.??I don't remember off hand exactly which or what > the change was they made to stabilise it though, but I can probabily > find out tomorrow. > Since the same combination of MAC-PHY works well on other designs, it is also my feeling that is has something to do with some timing parameter, maybe related to this particular PCB. While debugging this issue, we tried to play with all the parameters we could think of but never found anything worth mentioning. If you have any ideas, I'd be happy to try. Jerome From mboxrd@z Thu Jan 1 00:00:00 1970 From: jbrunet@baylibre.com (Jerome Brunet) Date: Fri, 06 Jan 2017 11:11:36 +0100 Subject: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue In-Reply-To: <20170105232508.GU14217@n2100.armlinux.org.uk> References: <1480348229-25672-1-git-send-email-jbrunet@baylibre.com> <049b1efc-3bad-92e0-45ef-0563dc5d81de@gmail.com> <20170105232508.GU14217@n2100.armlinux.org.uk> Message-ID: <1483697496.28003.40.camel@baylibre.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, 2017-01-05 at 23:25 +0000, Russell King - ARM Linux wrote: > On Mon, Nov 28, 2016 at 09:54:28AM -0800, Florian Fainelli wrote: > > > > If we start supporting generic "enable", "disable" type of > > properties > > with values that map directly to register definitions of the HW, we > > leave too much room for these properties to be utilized to > > implement a > > specific policy, and this is not acceptable. > > Another concern with this patch is that the existing phylib "set_eee" > code is horribly buggy - it just translates the modes from userspace > into the register value and writes them directly to the register with > no validation.??So it's possible to set modes in the register that > the > hardware doesn't support, and have them advertised to the link > partner. Hi Russell, The purpose of this patch is to provide a way to mark as broken a particular eee mode. At first, it had nothing to do with "set_eee" but, as Florian rightly pointed out, users shouldn't be able to re-enable a broken mode. At first, I was thinking about returning -ENOSUP if a broken mode was requested. Then I noticed the behavior you just described: A user can request anything of "set_eee", he won't necessarily get what he asked but won't get an error either. To avoid mixing different topic in a single patch, I kept the same behavior, not returning an error, just silently discarding broken modes I agree with you, we should probably validate a bit more what we asked of the hardware in set_eee. I wonder if we should return an error though. With the current implementation, user space application could simply ask to activate everything, excepting the kernel to sort it out and return an error only if something horribly wrong happened. If we start returning an error for unsupported modes, we could break things. I guess we should just silently filter the requested modes. > > I have a patch which fixes that, restricting (as we do elsewhere) the > advert according to the EEE supported capabilities retrieved from the > PCS Could be interesting :) > - maybe the problem here is that the PCS doesn't support support > EEE in 1000baseT mode? It does, and that's kind of the problem. EEE in ON for 100Tx and 1000T by default with this PHY. I have several platform with the same MAC-PHY combination. Only the OdroidC2 shows this particular issue with 1000T- EEE As explained in other mails in this thread. The problem does not come from the MAC entering LPI. It actually comes from the link partner entering LPI on the Rx path under significant Tx transfer. For some reason, this completely mess up our PHY. > > Out of interest, which PHY is used on this platform? The PHY is the Realtek RTL8211F > > On the SolidRun boards, they're using AR8035, and have suffered this > occasional link drop problem.??What has been found is that it seems > to > be to do with the timing parameters, and it seemed to only be 1000bT > that was affected.??I don't remember off hand exactly which or what > the change was they made to stabilise it though, but I can probabily > find out tomorrow. > Since the same combination of MAC-PHY works well on other designs, it is also my feeling that is has something to do with some timing parameter, maybe related to this particular PCB. While debugging this issue, we tried to play with all the parameters we could think of but never found anything worth mentioning. If you have any ideas, I'd be happy to try. Jerome From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerome Brunet Subject: Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue Date: Fri, 06 Jan 2017 11:11:36 +0100 Message-ID: <1483697496.28003.40.camel@baylibre.com> References: <1480348229-25672-1-git-send-email-jbrunet@baylibre.com> <049b1efc-3bad-92e0-45ef-0563dc5d81de@gmail.com> <20170105232508.GU14217@n2100.armlinux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20170105232508.GU14217@n2100.armlinux.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Russell King - ARM Linux , Florian Fainelli Cc: Andrew Lunn , Alexandre TORGUE , Neil Armstrong , Martin Blumenstingl , netdev@vger.kernel.org, Giuseppe Cavallaro , linux-kernel@vger.kernel.org, Yegor Yefremov , Julia Lawall , devicetree@vger.kernel.org, Andre Roth , Kevin Hilman , Carlo Caione , linux-amlogic@lists.infradead.org, Andreas =?ISO-8859-1?Q?F=E4rber?= , linux-arm-kernel@lists.infradead.org List-Id: devicetree@vger.kernel.org T24gVGh1LCAyMDE3LTAxLTA1IGF0IDIzOjI1ICswMDAwLCBSdXNzZWxsIEtpbmcgLSBBUk0gTGlu dXggd3JvdGU6Cj4gT24gTW9uLCBOb3YgMjgsIDIwMTYgYXQgMDk6NTQ6MjhBTSAtMDgwMCwgRmxv cmlhbiBGYWluZWxsaSB3cm90ZToKPiA+IAo+ID4gSWYgd2Ugc3RhcnQgc3VwcG9ydGluZyBnZW5l cmljICJlbmFibGUiLCAiZGlzYWJsZSIgdHlwZSBvZgo+ID4gcHJvcGVydGllcwo+ID4gd2l0aCB2 YWx1ZXMgdGhhdCBtYXAgZGlyZWN0bHkgdG8gcmVnaXN0ZXIgZGVmaW5pdGlvbnMgb2YgdGhlIEhX LCB3ZQo+ID4gbGVhdmUgdG9vIG11Y2ggcm9vbSBmb3IgdGhlc2UgcHJvcGVydGllcyB0byBiZSB1 dGlsaXplZCB0bwo+ID4gaW1wbGVtZW50IGEKPiA+IHNwZWNpZmljIHBvbGljeSwgYW5kIHRoaXMg aXMgbm90IGFjY2VwdGFibGUuCj4gCj4gQW5vdGhlciBjb25jZXJuIHdpdGggdGhpcyBwYXRjaCBp cyB0aGF0IHRoZSBleGlzdGluZyBwaHlsaWIgInNldF9lZWUiCj4gY29kZSBpcyBob3JyaWJseSBi dWdneSAtIGl0IGp1c3QgdHJhbnNsYXRlcyB0aGUgbW9kZXMgZnJvbSB1c2Vyc3BhY2UKPiBpbnRv IHRoZSByZWdpc3RlciB2YWx1ZSBhbmQgd3JpdGVzIHRoZW0gZGlyZWN0bHkgdG8gdGhlIHJlZ2lz dGVyIHdpdGgKPiBubyB2YWxpZGF0aW9uLsKgwqBTbyBpdCdzIHBvc3NpYmxlIHRvIHNldCBtb2Rl cyBpbiB0aGUgcmVnaXN0ZXIgdGhhdAo+IHRoZQo+IGhhcmR3YXJlIGRvZXNuJ3Qgc3VwcG9ydCwg YW5kIGhhdmUgdGhlbSBhZHZlcnRpc2VkIHRvIHRoZSBsaW5rCj4gcGFydG5lci4KCkhpIFJ1c3Nl bGwsCgpUaGUgcHVycG9zZSBvZiB0aGlzIHBhdGNoIGlzIHRvIHByb3ZpZGUgYSB3YXkgdG8gbWFy ayBhcyBicm9rZW4gYQpwYXJ0aWN1bGFyIGVlZSBtb2RlLiBBdCBmaXJzdCwgaXQgaGFkIG5vdGhp bmcgdG8gZG8gd2l0aCAic2V0X2VlZSIgYnV0LAphcyBGbG9yaWFuIHJpZ2h0bHkgcG9pbnRlZCBv dXQsIHVzZXJzIHNob3VsZG4ndCBiZSBhYmxlIHRvIHJlLWVuYWJsZSBhCmJyb2tlbiBtb2RlLgoK QXQgZmlyc3QsIEkgd2FzIHRoaW5raW5nIGFib3V0IHJldHVybmluZyAtRU5PU1VQIGlmIGEgYnJv a2VuIG1vZGUgd2FzCnJlcXVlc3RlZC4gVGhlbiBJIG5vdGljZWQgdGhlIGJlaGF2aW9yIHlvdSBq dXN0IGRlc2NyaWJlZDogQSB1c2VyIGNhbgpyZXF1ZXN0IGFueXRoaW5nIG9mICJzZXRfZWVlIiwg aGUgd29uJ3QgbmVjZXNzYXJpbHkgZ2V0IHdoYXQgaGUgYXNrZWQKYnV0IHdvbid0IGdldCBhbiBl cnJvciBlaXRoZXIuCgpUbyBhdm9pZCBtaXhpbmcgZGlmZmVyZW50IHRvcGljIGluIGEgc2luZ2xl IHBhdGNoLCBJIGtlcHQgdGhlIHNhbWUKYmVoYXZpb3IsIG5vdCByZXR1cm5pbmcgYW4gZXJyb3Is IGp1c3Qgc2lsZW50bHkgZGlzY2FyZGluZyBicm9rZW4gbW9kZXMKCkkgYWdyZWUgd2l0aCB5b3Us IHdlIHNob3VsZCBwcm9iYWJseSB2YWxpZGF0ZSBhIGJpdCBtb3JlIHdoYXQgd2UgYXNrZWQKb2Yg dGhlIGhhcmR3YXJlIGluIHNldF9lZWUuCgpJIHdvbmRlciBpZiB3ZSBzaG91bGQgcmV0dXJuIGFu IGVycm9yIHRob3VnaC4gV2l0aCB0aGUgY3VycmVudAppbXBsZW1lbnRhdGlvbiwgdXNlciBzcGFj ZSBhcHBsaWNhdGlvbiBjb3VsZCBzaW1wbHkgYXNrIHRvIGFjdGl2YXRlCmV2ZXJ5dGhpbmcsIGV4 Y2VwdGluZyB0aGUga2VybmVsIHRvIHNvcnQgaXQgb3V0IGFuZCByZXR1cm4gYW4gZXJyb3IKb25s eSBpZiBzb21ldGhpbmcgaG9ycmlibHkgd3JvbmcgaGFwcGVuZWQuIElmIHdlIHN0YXJ0IHJldHVy bmluZyBhbgplcnJvciBmb3IgdW5zdXBwb3J0ZWQgbW9kZXMsIHdlIGNvdWxkIGJyZWFrIHRoaW5n cy4gSSBndWVzcyB3ZSBzaG91bGQKanVzdCBzaWxlbnRseSBmaWx0ZXIgdGhlIHJlcXVlc3RlZCBt b2Rlcy4KCj4gCj4gSSBoYXZlIGEgcGF0Y2ggd2hpY2ggZml4ZXMgdGhhdCwgcmVzdHJpY3Rpbmcg KGFzIHdlIGRvIGVsc2V3aGVyZSkgdGhlCj4gYWR2ZXJ0IGFjY29yZGluZyB0byB0aGUgRUVFIHN1 cHBvcnRlZCBjYXBhYmlsaXRpZXMgcmV0cmlldmVkIGZyb20gdGhlCj4gUENTCgpDb3VsZCBiZSBp bnRlcmVzdGluZyA6KQoKPiAgLSBtYXliZSB0aGUgcHJvYmxlbSBoZXJlIGlzIHRoYXQgdGhlIFBD UyBkb2Vzbid0IHN1cHBvcnQgc3VwcG9ydAo+IEVFRSBpbiAxMDAwYmFzZVQgbW9kZT8KCgpJdCBk b2VzLCBhbmQgdGhhdCdzIGtpbmQgb2YgdGhlIHByb2JsZW0uIEVFRSBpbiBPTiBmb3IgMTAwVHgg YW5kIDEwMDBUCmJ5IGRlZmF1bHQgd2l0aCB0aGlzIFBIWS4gSSBoYXZlIHNldmVyYWwgcGxhdGZv cm0gd2l0aCB0aGUgc2FtZSBNQUMtUEhZIApjb21iaW5hdGlvbi4gT25seSB0aGUgT2Ryb2lkQzIg c2hvd3MgdGhpcyBwYXJ0aWN1bGFyIGlzc3VlIHdpdGggMTAwMFQtCkVFRQoKQXMgZXhwbGFpbmVk IGluIG90aGVyIG1haWxzIGluIHRoaXMgdGhyZWFkLiBUaGUgcHJvYmxlbSBkb2VzIG5vdCBjb21l CmZyb20gdGhlIE1BQyBlbnRlcmluZyBMUEkuIEl0IGFjdHVhbGx5IGNvbWVzIGZyb20gdGhlIGxp bmsgcGFydG5lcgplbnRlcmluZyBMUEkgb24gdGhlIFJ4IHBhdGggdW5kZXIgc2lnbmlmaWNhbnQg VHggdHJhbnNmZXIuIEZvciBzb21lCnJlYXNvbiwgdGhpcyBjb21wbGV0ZWx5IG1lc3MgdXAgb3Vy IFBIWS4KCj4gCj4gT3V0IG9mIGludGVyZXN0LCB3aGljaCBQSFkgaXMgdXNlZCBvbiB0aGlzIHBs YXRmb3JtPwoKVGhlIFBIWSBpcyB0aGUgUmVhbHRlayBSVEw4MjExRgoKPiAKPiBPbiB0aGUgU29s aWRSdW4gYm9hcmRzLCB0aGV5J3JlIHVzaW5nIEFSODAzNSwgYW5kIGhhdmUgc3VmZmVyZWQgdGhp cwo+IG9jY2FzaW9uYWwgbGluayBkcm9wIHByb2JsZW0uwqDCoFdoYXQgaGFzIGJlZW4gZm91bmQg aXMgdGhhdCBpdCBzZWVtcwo+IHRvCj4gYmUgdG8gZG8gd2l0aCB0aGUgdGltaW5nIHBhcmFtZXRl cnMsIGFuZCBpdCBzZWVtZWQgdG8gb25seSBiZSAxMDAwYlQKPiB0aGF0IHdhcyBhZmZlY3RlZC7C oMKgSSBkb24ndCByZW1lbWJlciBvZmYgaGFuZCBleGFjdGx5IHdoaWNoIG9yIHdoYXQKPiB0aGUg Y2hhbmdlIHdhcyB0aGV5IG1hZGUgdG8gc3RhYmlsaXNlIGl0IHRob3VnaCwgYnV0IEkgY2FuIHBy b2JhYmlseQo+IGZpbmQgb3V0IHRvbW9ycm93Lgo+IAoKU2luY2UgdGhlIHNhbWUgY29tYmluYXRp b24gb2YgTUFDLVBIWSB3b3JrcyB3ZWxsIG9uIG90aGVyIGRlc2lnbnMsIGl0CmlzIGFsc28gbXkg ZmVlbGluZyB0aGF0IGlzIGhhcyBzb21ldGhpbmcgdG8gZG8gd2l0aCBzb21lIHRpbWluZwpwYXJh bWV0ZXIsIG1heWJlIHJlbGF0ZWQgdG8gdGhpcyBwYXJ0aWN1bGFyIFBDQi4KCldoaWxlIGRlYnVn Z2luZyB0aGlzIGlzc3VlLCB3ZSB0cmllZCB0byBwbGF5IHdpdGggYWxsIHRoZSBwYXJhbWV0ZXJz IHdlCmNvdWxkIHRoaW5rIG9mIGJ1dCBuZXZlciBmb3VuZCBhbnl0aGluZyB3b3J0aCBtZW50aW9u aW5nLgoKSWYgeW91IGhhdmUgYW55IGlkZWFzLCBJJ2QgYmUgaGFwcHkgdG8gdHJ5LgoKSmVyb21l CgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1h cm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5v cmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0t a2VybmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S970071AbdAFKMY (ORCPT ); Fri, 6 Jan 2017 05:12:24 -0500 Received: from mail-wm0-f45.google.com ([74.125.82.45]:38623 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761190AbdAFKLk (ORCPT ); Fri, 6 Jan 2017 05:11:40 -0500 Message-ID: <1483697496.28003.40.camel@baylibre.com> Subject: Re: [PATCH net-next v4 0/4] Fix OdroidC2 Gigabit Tx link issue From: Jerome Brunet To: Russell King - ARM Linux , Florian Fainelli Cc: netdev@vger.kernel.org, devicetree@vger.kernel.org, Andrew Lunn , Alexandre TORGUE , Neil Armstrong , Martin Blumenstingl , Kevin Hilman , linux-kernel@vger.kernel.org, Yegor Yefremov , Julia Lawall , Andre Roth , linux-amlogic@lists.infradead.org, Carlo Caione , Giuseppe Cavallaro , Andreas =?ISO-8859-1?Q?F=E4rber?= , linux-arm-kernel@lists.infradead.org Date: Fri, 06 Jan 2017 11:11:36 +0100 In-Reply-To: <20170105232508.GU14217@n2100.armlinux.org.uk> References: <1480348229-25672-1-git-send-email-jbrunet@baylibre.com> <049b1efc-3bad-92e0-45ef-0563dc5d81de@gmail.com> <20170105232508.GU14217@n2100.armlinux.org.uk> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 (3.20.5-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2017-01-05 at 23:25 +0000, Russell King - ARM Linux wrote: > On Mon, Nov 28, 2016 at 09:54:28AM -0800, Florian Fainelli wrote: > > > > If we start supporting generic "enable", "disable" type of > > properties > > with values that map directly to register definitions of the HW, we > > leave too much room for these properties to be utilized to > > implement a > > specific policy, and this is not acceptable. > > Another concern with this patch is that the existing phylib "set_eee" > code is horribly buggy - it just translates the modes from userspace > into the register value and writes them directly to the register with > no validation.  So it's possible to set modes in the register that > the > hardware doesn't support, and have them advertised to the link > partner. Hi Russell, The purpose of this patch is to provide a way to mark as broken a particular eee mode. At first, it had nothing to do with "set_eee" but, as Florian rightly pointed out, users shouldn't be able to re-enable a broken mode. At first, I was thinking about returning -ENOSUP if a broken mode was requested. Then I noticed the behavior you just described: A user can request anything of "set_eee", he won't necessarily get what he asked but won't get an error either. To avoid mixing different topic in a single patch, I kept the same behavior, not returning an error, just silently discarding broken modes I agree with you, we should probably validate a bit more what we asked of the hardware in set_eee. I wonder if we should return an error though. With the current implementation, user space application could simply ask to activate everything, excepting the kernel to sort it out and return an error only if something horribly wrong happened. If we start returning an error for unsupported modes, we could break things. I guess we should just silently filter the requested modes. > > I have a patch which fixes that, restricting (as we do elsewhere) the > advert according to the EEE supported capabilities retrieved from the > PCS Could be interesting :) > - maybe the problem here is that the PCS doesn't support support > EEE in 1000baseT mode? It does, and that's kind of the problem. EEE in ON for 100Tx and 1000T by default with this PHY. I have several platform with the same MAC-PHY combination. Only the OdroidC2 shows this particular issue with 1000T- EEE As explained in other mails in this thread. The problem does not come from the MAC entering LPI. It actually comes from the link partner entering LPI on the Rx path under significant Tx transfer. For some reason, this completely mess up our PHY. > > Out of interest, which PHY is used on this platform? The PHY is the Realtek RTL8211F > > On the SolidRun boards, they're using AR8035, and have suffered this > occasional link drop problem.  What has been found is that it seems > to > be to do with the timing parameters, and it seemed to only be 1000bT > that was affected.  I don't remember off hand exactly which or what > the change was they made to stabilise it though, but I can probabily > find out tomorrow. > Since the same combination of MAC-PHY works well on other designs, it is also my feeling that is has something to do with some timing parameter, maybe related to this particular PCB. While debugging this issue, we tried to play with all the parameters we could think of but never found anything worth mentioning. If you have any ideas, I'd be happy to try. Jerome