From patchwork Wed Feb 15 00:16:30 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christian Lamparter X-Patchwork-Id: 728018 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3vNKdk2619z9ryQ for ; Wed, 15 Feb 2017 11:18:50 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=googlemail.com header.i=@googlemail.com header.b="AmZSqktN"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750931AbdBOASq (ORCPT ); Tue, 14 Feb 2017 19:18:46 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:34434 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750812AbdBOASo (ORCPT ); Tue, 14 Feb 2017 19:18:44 -0500 Received: by mail-wm0-f66.google.com with SMTP id c85so5904042wmi.1 for ; Tue, 14 Feb 2017 16:18:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20161025; h=from:to:cc:subject:date:message-id:user-agent:in-reply-to :references:mime-version:content-transfer-encoding; bh=Y/ZdGP5RgLNhHbae47wH6XGs0+9iSnkdFPFzb9SwbRw=; b=AmZSqktN4eQWk6Qf92Wss+mzHGW4llgWrnPtnDLLWHeNh2eW7gr9SrPMnVPYEwiFsK kGVGEuiCKNt2pWFqcV8aZ3BZm3XUFNitMPDBlR6LzjH8mBxiol6v1ZjoC6RDwOZdKM1e XOS7I5m0IDArKTu1baWSbA3+5gvrSdNpw+GGg7SDDZy4+5MoDI3XoyhlRzUwKp2lUzdx iFUaBW/OtriRbb3iQgSs1/DwdwV3RMD535eba26dUfyOjnykncZyYzAuVIOmthJeA4Gz b3q1Q+QrFRFUgDAca4Ec3qStZ488P4WsqzDjKhQqO2p4kALKAoEwrSnscXWNckBqo99Z 8wMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:user-agent :in-reply-to:references:mime-version:content-transfer-encoding; bh=Y/ZdGP5RgLNhHbae47wH6XGs0+9iSnkdFPFzb9SwbRw=; b=atoXYlx1si+9T7R2dtI1sBiq0E0CuHdbwFpm6SARm8nfsnOwz1sMUJiTpawps9nund ogHAeI7L0ud5yJnloglBVGLCOPOCCOOWFhHq1nk2wz094rjs4TAeEgWZ0oMAw9BhVBZ/ SVUAPT57/uq2x5VP2WGZ6TBnfuHlFClSXcv32cFx66kHLizD2y9V1Kw5gbpfKXQTupWm qQR6xwlNbGkOr8XZ8PSBJPgmFvsKANyZX0nPHAND54/dWUpypU3PSuAl+CETrMBCIWJy 56bloQ04MI/gmuMJCY/ccxWJEK+meGfCF7I9CZjHeyGyp1Uk7Vay2LMVpYyOIrNV5oLb hRew== X-Gm-Message-State: AMke39luO9G/Ofw3nPlXdZIHHSIX7SDglRSSyEuafLAn3yNlUqagsgSJpUvsVQ4k8mEG4Q== X-Received: by 10.28.86.214 with SMTP id k205mr5186331wmb.26.1487117922252; Tue, 14 Feb 2017 16:18:42 -0800 (PST) Received: from debian64.daheim (p5B0D7F93.dip0.t-ipconnect.de. [91.13.127.147]) by smtp.gmail.com with ESMTPSA id 18sm2680384wrb.14.2017.02.14.16.18.40 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 14 Feb 2017 16:18:40 -0800 (PST) Received: from localhost.daheim ([127.0.0.1] helo=debian64.localnet) by debian64.daheim with esmtp (Exim 4.89_RC5) (envelope-from ) id 1cdnGo-0006sA-Jp; Wed, 15 Feb 2017 01:16:30 +0100 From: Christian Lamparter To: Florian Fainelli Cc: netdev@vger.kernel.org, Christian Lamparter , "David S . Miller" , Ivan Mikhaylov , vivien.didelot@savoirfairelinux.com, andrew@lunn.ch Subject: Re: [RFC 2/2] net: emac: add support for device-tree based PHY discovery and setup Date: Wed, 15 Feb 2017 01:16:30 +0100 Message-ID: <2409103.Lage0nq55N@debian64> User-Agent: KMail/5.2.3 (Linux/4.10.0-rc7-wt-wo+; KDE/5.28.0; x86_64; ; ) In-Reply-To: <2122984.F6mzWerJKz@debian64> References: <2122984.F6mzWerJKz@debian64> MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Tuesday, February 14, 2017 12:38:42 AM CET Christian Lamparter wrote: > > >>> diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c > > >>> --- a/drivers/net/ethernet/ibm/emac/core.c > > >>> +++ b/drivers/net/ethernet/ibm/emac/core.c > > >>> @@ -2420,6 +2421,179 @@ static int emac_read_uint_prop(struct device_node *np, const char *name, > > >>> [...] > > >>> +static int emac_mii_bus_reset(struct mii_bus *bus) > > >>> +{ > > >>> + struct emac_instance *dev = netdev_priv(bus->priv); > > >>> + > > >>> + emac_mii_reset_phy(&dev->phy); > > >> > > >> This seems wrong, emac_mii_reset_phy() does a BMCR software reset, which > > >> PHYLIB is already going to do (phy_init_hw), yet you do this here at the > > >> MDIO bus level towards a specify PHY, whereas this should be affecting > > >> the MDIO bus itself (and/or *all* PHY child devices for quirks). > > > Ah, this is a good point. The emac driver has a emac_reset() function > > > that does disable and enabled the phy clocks. That said, this is already > > > done by the emac driver during init too. So if I added it, the bus is > > > reset twice (since it doesn't hurt - I added it back). > > > > > > The emac_mii_phy_reset() was added because of the Meraki MX60(W). > > > This is because Cisco's bootloader disables the switch port > > > (probably to prevent WAN<->LAN leakage during boot) > > > > > > [bootlog from the MX60(W)] > > > |Disabling port 0 > > > |Disabling port 1 > > > |Disabling port 2 > > > |Disabling port 3 > > > |ENET Speed is 1000 Mbps - FULL duplex connection (EMAC0) > > > > > > Without emac_mii_reset_phy(), the mdiobus_scan() function, which > > > is called by mdiobus_register will fail with -ENODEV. > > > | /plb/opb/ethernet@ef600c00: failed to attach dt phy (-19). > > > This is because get_phy_id() will "mostly read mostly Fs" and abort. > > > > Is the PHY just powered down by chance (BMCR_PWRDN set?) and resetting > > it implicitly clears the power down that seems to be what is going on. > > Yes, the PHY is just in the BMCR_PDOWN state. I can do the same > on the WNDR4700, by messing with u-boot: > > | => mii write 0 0 0x0800 > | => mii dump > | 0. (ffff) -- PHY control register -- > | (8000:8000) 0.15 = 1 reset > | (4000:4000) 0.14 = 1 loopback > | (2040:2040) 0. 6,13 = b11 speed selection = ??? Mbps > | (1000:1000) 0.12 = 1 A/N enable > | (0800:0800) 0.11 = 1 power-down > | (0400:0400) 0.10 = 1 isolate > | (0200:0200) 0. 9 = 1 restart A/N > | (0100:0100) 0. 8 = 1 duplex = full > | (0080:0080) 0. 7 = 1 collision test enable > | (003f:003f) 0. 5- 0 = 63 (reserved) > > On the Meraki, the port disabled by the bootloader. > The reset is still needed. > > > Keep in mind that MDIO address 16 is the switch's pseudo PHY address > > here, so if you are telling PHYLIB to probe for that address and you > > don't get the expected MII_PHYSID1/2 value in return, that usually means > > that there was a PHY fixup registered to intercept these reads and make > > us return the switch's unique identifier. Reading from the switch's > > pseudo PHY at address 16 registers 2/3 (MII_PHYSID1/2) is not guaranteed > > to return the switch's unique identifer. > > > > With a MDIO device driver this won't happen because you will be probed > > by address, and you can read any switch register you want to and from > > there move on with the initialization. > > > > > > > > > > > With emac_mii_reset_phy() in place, it gets detected: > > > | switch0: Atheros AR8327 rev. 4 switch registered on emac_mdio > > > > > > Furthermore, this is probably not the only device which need it. > > > Currently, emac's own phy.c code does call emac_mii_reset_phy() > > > as well as part of its probe procedure. > > > > > > > > > Ideally, we would like to reset only the ports which are registered in the DT. > > > > Which you would get for free if you did extend qca8k to support the > > 8327, because qca8k does implicitly tell the DSA layer to register a > > dsa_slave_mii_bus which will probe and attach to per-port built-in PHYs > > and that happens only for the ports enabled on your specific board. > No, the Meraki and the modified WNDR4700 still refuse to work. Just >one< > of the phys at address 0-4 need to be powered down to get the following > error: > [ 4.425618] DSA: switch 0 0 parsed > [ 4.429034] DSA: tree 0 parsed > [ 4.435416] qca8k: probe of 4ef600c00.ethern:10 failed with error -5 > > I'll report back, what exactly is causing the error in this case. Ok, the -EIO error is coming from this line: get_phy_id() gets called as part of mdiobus_register() in dsa_ds_apply() which is called as part of the dsa_switch_register(). So, is there a good place to reset the switch/ports? Sure, doing it in qca8k has the advantage that we know it's 5 ports. However, I'm wondering, if the dsa or phy driver the right place for this? Note: behind the mdiobus_read() at is emac's emac_mdio_read does returning -EREMOTEIO. I don't see that hiding the error code behind a return 0xffff will help much: --- For example I disabled just phy2: | Generic PHY 4ef600c00.ethern:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=4ef600c00.ethern:00, irq=-1) | Generic PHY dsa-0.0:01: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:01, irq=-1) | qca8k 4ef600c00.ethern:10 lan2: no phy at 2 | qca8k 4ef600c00.ethern:10 lan2: failed to connect to phy2: -19 | emac 4ef600c00.ethernet eth0: error -19 setting up slave phy | qca8k 4ef600c00.ethern:10: Failed to create slave 3: -19 | Generic PHY dsa-0.0:03: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:03, irq=-1) | Generic PHY dsa-0.0:04: attached PHY driver [Generic PHY] (mii_bus:phy_addr=dsa-0.0:04, irq=-1) And as a result: the switch "lost" the port. When I tried to unbind the "faulty" switch in order to reset it and rebind: root@LEDE:/sys/bus/mdio_bus/drivers/qca8k# echo 4ef600c00.ethern:10 > unbind [ 2435.970104] Unable to handle kernel paging request for data at address 0x00000050 [ 2435.977580] Faulting instruction address: 0xc0359ae8 [ 2435.982532] Oops: Kernel access of bad area, sig: 11 [#1] [ 2435.987901] WNDR4700 Platform [ 2436.116263] CPU: 0 PID: 663 Comm: ash Not tainted 4.9.8 #0 [ 2436.121729] task: cf974000 task.stack: ce42a000 [ 2436.126238] NIP: c0359ae8 LR: c036a534 CTR: c029b338 [ 2436.131180] REGS: ce42bd10 TRAP: 0300 Not tainted (4.9.8) [ 2436.136811] MSR: 00029000 [ 2436.140260] CR: 24002288 XER: 00000000 [ 2436.144251] DEAR: 00000050 ESR: 00000000 GPR00: c036a534 ce42bdc0 cf974000 cfb02800 c03f5553 00000000 cf9e19e0 00000001 GPR08: cf9e1b00 00000040 00000000 00000001 00780100 00000000 00000000 00000000 GPR16: 00000000 00000000 10060000 1005cf90 b79a2495 1005cf70 b7a354c4 00000000 GPR24: cfaf2508 00000001 cfaf24fc cfffbca8 cfaf228c 00000003 cfaf2400 cfb02800 NIP [c0359ae8] dsa_slave_destroy+0x28/0x78 [ 2436.180491] LR [c036a534] dsa_dst_unapply.part.7+0xa4/0xb70 [ 2436.186033] Call Trace: [ 2436.188475] [ce42bdc0] [c0359c48] dsa_port_is_cpu+0x1c/0x50 (unreliable) [ 2436.195161] [ce42bdd0] [c036a534] dsa_dst_unapply.part.7+0xa4/0xb70 [ 2436.201409] [ce42be00] [c0359d60] dsa_unregister_switch+0x3c/0x60 [ 2436.207485] [ce42be20] [c0238a18] mdio_remove+0x24/0x40 [ 2436.212701] [ce42be30] [c01c6544] __device_release_driver+0xa4/0x13c [ 2436.219034] [ce42be40] [c01c6604] device_release_driver+0x28/0x40 [ 2436.225107] [ce42be50] [c01c4c94] unbind_store+0x6c/0xac [ 2436.230417] [ce42be70] [c00f8838] kernfs_fop_write+0x128/0x1ac [ 2436.236247] [ce42be90] [c00a8ce4] __vfs_write+0x28/0x134 [ 2436.241542] [ce42bef0] [c00a9a00] vfs_write+0xd0/0x17c [ 2436.246665] [ce42bf10] [c00aa758] SyS_write+0x4c/0xa4 [ 2436.251705] [ce42bf40] [c000a848] ret_from_syscall+0x0/0x3c [ 2436.257266] --- interrupt: c01 at 0xb7a0edc0 [ 2436.257266] LR = 0xb7a00df0 [ 2436.264631] Instruction dump: [ 2436.267597] 38210030 4e800020 7c0802a6 9421fff0 bfc10008 7c7f1b78 90010014 892304e8 [ 2436.275397] 814304e4 39290004 55292036 7d2a4a14 <83c90010> 4bf46641 807f04ec 2f830000 [ 2436.283367] ---[ end trace e5787cb2a55a0fbd ]--- [ 2436.289726] [ 2437.291295] Kernel panic - not syncing: Fatal exception The panic is caused by dsa_slave_create() not clearing the dangling ds->ports[port].netdev pointer. The function dsa_user_port_unapply() relies on it being NULL in order to skip unregistered entries. --- --- Thanks, Christian diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 09fc3e9462c1..0dae29eb95d6 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1460,6 +1460,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent, ret = dsa_slave_phy_setup(p, slave_dev); if (ret) { netdev_err(master, "error %d setting up slave phy\n", ret); + ds->ports[port].netdev = NULL; unregister_netdev(slave_dev); free_netdev(slave_dev); return ret;