From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5809DC282C4 for ; Mon, 4 Feb 2019 23:55:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1204920820 for ; Mon, 4 Feb 2019 23:55:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="rnQ5xxQm" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727499AbfBDXzo (ORCPT ); Mon, 4 Feb 2019 18:55:44 -0500 Received: from mail-wm1-f65.google.com ([209.85.128.65]:55903 "EHLO mail-wm1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726083AbfBDXzo (ORCPT ); Mon, 4 Feb 2019 18:55:44 -0500 Received: by mail-wm1-f65.google.com with SMTP id y139so1737428wmc.5 for ; Mon, 04 Feb 2019 15:55:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=q5Y9euMhQTfFG563wa8OVhf4XsxCBnegg1Bzc2bY1/4=; b=rnQ5xxQmFj8TvQ9N9vjqji9NH1tJEpbrE37xDVRrfC/HDRr2J0FvCmb/PO6KGm2LfQ 1PBsgvzJgwK+hFsWUxZfCa17G9WDD71jbW9lGZTs29Njs9em8GPaonsEJRqTRtKY+gJO P/FPAAOgUrdvwnUzHK9yM5TGPLz2HnhKeUuLGj8+HazM2L6hGnfMveMCRBWjmQOuT2Ax mYyufbW8HU2aDF3wiODjC24t2ruqZfcTRwKNu3MFm/SuUFt98T4vlmDYDnVREF5Eizhg tE7NNgURiqjRx3rE2r/4Q4BvqoMVhVXXGLHck1OVjXrDLFMExAWgZpNPjZu43/5VLiZC kMew== 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:in-reply-to :references:mime-version:content-transfer-encoding; bh=q5Y9euMhQTfFG563wa8OVhf4XsxCBnegg1Bzc2bY1/4=; b=dYOW/XePZKbaukfamCkA0M13wiCpUEBmogP1xPC/FQSjmMEAy2W1YhbI21s0Z7H8Dc tEmGmYfRjp3b1sdJ6urZGEJgDpi6V4ULXDWNmEFPl+WFjXF807Yz8n95FQQQK3DroOju p8yzxH4zUU/Ud5ySh7VocDqdaY+2Z3TVtD4PnL9UYfEeFxXUTbxrC5iHZTJNaVbwvThi T4FTfqNQ8sMgv6VjDLHp8iH+sefGiKmKtXC62Sr4VEBDgQggvZjugJGF/2DfX1knvSkA UfdFWiScuOIQ1OLgtj7nvAvDbuFFOSVJ0eR5KucsK0nueUJ50amGne6HFGS0MbUjLgA2 D6zw== X-Gm-Message-State: AHQUAuaPwvzxTXhR4wkYA7I6LvAI9fvKJkIFMAsj+N46534sYYNO0zwt REecDMAW1gmWcV3KxhO40Ew= X-Google-Smtp-Source: AHgI3IaqszfphVixuQK3D9FQ9ZuMeYT1ecfTBYQwKJrtnajMBqS4RlY3keCEBYqaatZytv4phuK7WQ== X-Received: by 2002:a7b:c8d7:: with SMTP id f23mr1271377wml.121.1549324541703; Mon, 04 Feb 2019 15:55:41 -0800 (PST) Received: from debian64.daheim (p4FD09D33.dip0.t-ipconnect.de. [79.208.157.51]) by smtp.gmail.com with ESMTPSA id g9sm14305667wmg.44.2019.02.04.15.55.40 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 04 Feb 2019 15:55:40 -0800 (PST) Received: from localhost.daheim ([127.0.0.1] helo=debian64.localnet) by debian64.daheim with esmtp (Exim 4.92-RC5) (envelope-from ) id 1gqo5T-0003lY-Mq; Tue, 05 Feb 2019 00:55:39 +0100 From: Christian Lamparter To: Andrew Lunn Cc: netdev@vger.kernel.org, Florian Fainelli , Vivien Didelot Subject: Re: [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy translation Date: Tue, 05 Feb 2019 00:55:39 +0100 Message-ID: <7488926.h4gv5cmPiq@debian64> In-Reply-To: <20190204222641.GE3397@lunn.ch> References: <20190204213555.26054-1-chunkeey@gmail.com> <20190204222641.GE3397@lunn.ch> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Hello Andrew and Florian. I concated both replies into this post. On Monday, February 4, 2019 11:26:41 PM CET Andrew Lunn wrote: > On Mon, Feb 04, 2019 at 10:35:55PM +0100, Christian Lamparter wrote: > > The QCA8337 enumerates 5 PHYs on the MDC/MDIO access: PHY0-PHY4. > > Based on the System Block Diagram in Section 1.2 of the > > QCA8337's datasheet. These PHYs are internally connected > > to MACs of PORT 1 - PORT 5. > > Hi Christian > > Is the off-by-one the problem here? > Yes, this was triggered by a MII_PHYSID1 and MII_PHYSID2 read for a non-existent PHY at address >0x5< coming from dsa_register_switch() during boot. I added a WARN_ON to qca8k_phy_read() to see from where the reads are coming from: [ 6.218168] WARNING: CPU: 0 PID: 21 at qca8k_phy_read+0x3c/0x68 [ 6.224062] Modules linked in: [ 6.227107] CPU: 0 PID: 21 Comm: kworker/0:1 Tainted: G W 4.19.16 #0 [ 6.234732] Workqueue: events deferred_probe_work_func [ 6.239849] NIP: c0308528 LR: c0308528 CTR: c0257d30 [ 6.244878] REGS: cf485b80 TRAP: 0700 Tainted: G W (4.19.16) [ 6.252064] MSR: 00029000 CR: 28002222 XER: 00000000 [ 6.258224] [ 6.258224] GPR00: c0308528 cf485c30 cf47c000 00000005 00000000 00000215 c09d61e4 c09d3de0 [ 6.258224] GPR08: 00021000 c09bfb00 c09bfb00 00000007 24002444 00000000 c003f81c cf466060 [ 6.258224] GPR16: ffffffff cf633f60 cf633f6c 00000001 cf633f40 c0589f7c 006080c0 c09bede8 [ 6.258224] GPR24: cf633f40 00000000 00000000 fffff000 00000003 cdf21790 00000003 00000005 [ 6.292952] NIP [c0308528] qca8k_phy_read+0x3c/0x68 [ 6.297808] LR [c0308528] qca8k_phy_read+0x3c/0x68 [ 6.302574] Call Trace: [ 6.305013] [cf485c30] [c0308528] qca8k_phy_read+0x3c/0x68 (unreliable) [ 6.311602] [cf485c50] [c0300530] mdiobus_read+0x6c/0x9c [ 6.316894] [cf485c70] [c02ffdcc] get_phy_device+0x188/0x204 [ 6.322527] [cf485cd0] [c0300740] mdiobus_scan+0x20/0x160 [ 6.327901] [cf485d00] [c0300a3c] __mdiobus_register+0x1bc/0x2a8 [ 6.333884] [cf485d30] [c047e690] dsa_register_switch+0x6a0/0x904 [ 6.339954] [cf485db0] [c0300dd8] mdio_probe+0x40/0x88 [ 6.345070] [cf485dc0] [c026947c] really_probe+0x168/0x300 [ 6.350530] [cf485df0] [c0269b44] driver_probe_device+0x410/0x460 [ 6.356594] [cf485e10] [c02672cc] bus_for_each_drv+0x5c/0xc0 [ 6.362229] [cf485e40] [c0269270] __device_attach+0xa8/0x144 [ 6.367862] [cf485e70] [c0268430] bus_probe_device+0x3c/0xc0 [ 6.373495] [cf485e90] [c02689dc] deferred_probe_work_func+0x70/0xac [ 6.379821] [cf485eb0] [c003a2c4] process_one_work+0x25c/0x3b0 [ 6.385633] [cf485ed0] [c003a708] worker_thread+0x2f0/0x434 [ 6.391180] [cf485f10] [c003f950] kthread+0x134/0x138 [ 6.396209] [cf485f40] [c000d23c] ret_from_kernel_thread+0x14/0x1c [ 6.402357] Instruction dump: [ 6.405313] 93c10018 7cbe2b78 93e1001c 7c9f2378 93a10014 83a30018 4bfffe4d 3d20c058 [ 6.413027] 7c651b78 7fe4fb78 3869c970 4bd4e35d <0fe00000> 80010024 7fc5f378 807d0004 [ 6.420916] ---[ end trace 00aeb6767b21cd36 ]--- If I disable port@5 (see qca8k.txt example) then problem went away (but of course, this makes the WAN port useless). When I looked more into it, I started to notice that the mdiobus_scan started from address >1< (not 0). It was skipping PHY0 (which does exist!) and then the extract from qca8k.txt suddenly made sense: |The integrated switch subnode should be specified according to the binding |described in dsa/dsa.txt. As the QCA8K switches do not have a N:N mapping of |port and PHY id, each subnode describing a port needs to have a valid phandle |referencing the internal PHY connected to it. The CPU port of this switch is |always port 0. >From what I can tell, the slave mdio port-numbers (i.e 0 - 5/6 on the qca8k) are being used directly as phy-addresses on the slave-bus. And since the port0 is a cpu port it gets skipped when the ds->phy_mii_mask is created in dsa_switch_setup(): | ds->phys_mii_mask |= dsa_user_ports(ds); // 0x3E However, ds->phys_mii_mask is used to calculate the slave's phy_mask in dsa_slave_mii_bus_init() |ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask; // ~0x3E which is then later used by __mdiobus_register() to scan for the supposedly existing PHY at 0x1 - 0x5. | for (i = 0; i < PHY_MAX_ADDR; i++) { | if ((bus->phy_mask & (1 << i)) == 0) { | struct phy_device *phydev; | | phydev = mdiobus_scan(bus, i); | if (IS_ERR(phydev) && (PTR_ERR(phydev) != -ENODEV)) { | err = PTR_ERR(phydev); | goto error; | } | } | } So, I'm looking for a way to get this "-1" somewhere and this version was the best justification I came up with. Because as Florian said, this is supposed to work for different drivers as well. --- On Monday, February 4, 2019 11:11:41 PM CET Florian Fainelli wrote: > On 2/4/19 1:35 PM, Christian Lamparter wrote: > > The QCA8337 enumerates 5 PHYs on the MDC/MDIO access: PHY0-PHY4. > > Based on the System Block Diagram in Section 1.2 of the > > QCA8337's datasheet. These PHYs are internally connected > > to MACs of PORT 1 - PORT 5. However, neither qca8k's slave > > mdio access functions qca8k_phy_read()/qca8k_phy_write() > > nor the dsa framework is set up for that. > > > > This version of the patch uses the existing phy-handle > > properties of each specified DSA Port in the DT to map > > each PORT/MAC to its exposed PHY on the MDIO bus. This > > is supported by the current binding document qca8k.txt > > as well. > > I don't think you should have to do any of this translation, because you > can do a couple of things with DSA/Device Tree: > > - you can not provide a phy-handle property at all, in which case, the > core DSA layer assumes that the PHY is part of the switch's internal > MDIO bus which is implictly created by dsa_slave_mii_bus_create() > > - you can specify a phy-handle property and then the PHY device tree > node can be placed pretty much anywhere in Device Tree, including on a > separate MDIO bus Device Tre node which is "external" to the switch > > In either case, the PHY device's MDIO bus parent and its address are > taken care of by drivers/of/of_mdio.c. You can look at mx88e6xxx for how > it deals with its internal vs. external MDIO bus controller and that > driver is used on a wide variety of cconfiguration. Hm, this sounds to me like the qca8k driver might be cheating a bit. Though, I can't really tell, I found "stub" translation routines in the mv88e6060 driver called mv88e6060_port_to_phy_addr(). But it just checks the range. I think the issue here is that qca8k_phy_read/write don't actually operate on the "internal" mdio-bus of the switch. Instead the mdiobus_read/write in qca8k_phy_read/write operates on the provided mdio-bus (by the ethernet-mac or gpio-mdio, etc...). But let's see what else can be done (maybe qca8k_phy_read/write() is just wrong and should be dropped?). Regards, Christian