Date:   Tue, 14 Feb 2017 16:24:01 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Christian Lamparter <chunkeey@...glemail.com>
Cc:     netdev@...r.kernel.org, Christian Lamparter <chunkeey@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Ivan Mikhaylov <ivan@...ibm.com>,
        vivien.didelot@...oirfairelinux.com, andrew@...n.ch
Subject: Re: [RFC 2/2] net: emac: add support for device-tree based PHY
 discovery and setup

On 02/14/2017 04:16 PM, Christian Lamparter wrote:
> 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@...00c00: 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.
>>>> <http://lxr.free-electrons.com/source/drivers/net/ethernet/ibm/emac/phy.c#L522>
>>>>
>>>> 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:
> <http://lxr.free-electrons.com/source/drivers/net/phy/phy_device.c?v=4.9#L496>
> 
> get_phy_id() gets called as part of mdiobus_register() in dsa_ds_apply()
> <http://lxr.free-electrons.com/source/net/dsa/dsa2.c?v=4.9#L322> 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 
> <http://lxr.free-electrons.com/source/drivers/net/phy/phy_device.c?v=4.9#L494>
> 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@...E:/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 <CE,EE,ME>[ 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.

Fixed that just recently:

https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=382e1eea2d983cd2343482c6a638f497bb44a636

and I am currently in the process of making the bind/unbind working
(without having the PHY detached from the network device), but that's a
stretch, and requires a ton of mucking around with driver references,
current state machine state, target state machine state etc...

Candidate patches for that are located here if you want:

https://github.com/ffainelli/linux/commit/e7e99a9ab998952b9f18e6e396779bb58626e9a5
https://github.com/ffainelli/linux/commit/e005be050cd876189f39c5a89fffa70ee8d6c512

> ---
> 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;
> ---
> 
> Thanks,
> Christian
> 


-- 
Florian

Powered by blists - more mailing lists