SoCFPGA ethernet broken

DISCUSSION Dinh Nguyen 5 participants 30 messages Oct 15, 2015 – Dec 4, 2015

Subsystems (3)

Networking Network drivers Ethernet PHY

lore

Share & tools

lore

Dinh Nguyen

author

2015-10-15 19:09 UTC #0 lore raw

Hi,

commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not the bus' parent." seems to have broken ethernet support for the SoCFPGA platform which is using the stmmac ethernet driver.

It appears that during DHCP, it cannot get an IP address. This only happens if ethernet was not used by the bootloader to tftp an kernel image. If I use the bootloader to tftp an image then ethernet is working fine. So I think the PHY is not getting enabled properly.

If I revert this patch, then ethernet is back to working on the platform.

I just tested this in the v4.3-rc5.

Will continue to debug, but was wondering if anyone has seen this issue.

Thanks for any help/comments.

Dinh

Bootlog below.
----

[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Initializing cgroup subsys cpuset
[    0.000000] Linux version 4.3.0-rc5-00037-g5b5f145
(dinguyen@linux-builds1) (gcc version 4.7.3 20130226 (prerelease) (crosstool
-NG linaro-1.13.1-4.7-2013.03-20130313 - Linaro GCC 2013.03) ) #1 SMP
Thu Oct 15 13:55:48 CDT 2015
[    0.000000] CPU: ARMv7 Processor [413fc090] revision 0 (ARMv7),
cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[    0.000000] Machine model: Altera SOCFPGA Cyclone V SoC Development Kit
[    0.000000] Truncating RAM at 0x00000000-0x40000000 to -0x2f800000
[    0.000000] Consider using a HIGHMEM enabled kernel.
[    0.000000] Memory policy: Data cache writealloc
[    0.000000] On node 0 totalpages: 194560
[    0.000000] free_area_init_node: node 0, pgdat c067c080, node_mem_map
ef20b000
[    0.000000]   Normal zone: 1520 pages used for memmap
[    0.000000]   Normal zone: 0 pages reserved
[    0.000000]   Normal zone: 194560 pages, LIFO batch:31
[    0.000000] PERCPU: Embedded 13 pages/cpu @ef1df000 s21760 r8192
d23296 u53248
[    0.000000] pcpu-alloc: s21760 r8192 d23296 u53248 alloc=13*4096
[    0.000000] pcpu-alloc: [0] 0 [0] 1
[    0.000000] Built 1 zonelists in Zone order, mobility grouping on.
Total pages: 193040
[    0.000000] Kernel command line: console=ttyS0,115200 root=/dev/nfs
rw nfsroot=137.57.160.210:/home/dinguyen/rootfs_yocto ip=dh
cp debug
[    0.000000] PID hash table entries: 4096 (order: 2, 16384 bytes)
[    0.000000] Dentry cache hash table entries: 131072 (order: 7, 524288
bytes)
[    0.000000] Inode-cache hash table entries: 65536 (order: 6, 262144
bytes)
[    0.000000] Memory: 764368K/778240K available (4696K kernel code,
274K rwdata, 1328K rodata, 324K init, 131K bss, 13872K reserv
ed, 0K cma-reserved)
[    0.000000] Virtual kernel memory layout:
[    0.000000]     vector  : 0xffff0000 - 0xffff1000   (   4 kB)
[    0.000000]     fixmap  : 0xffc00000 - 0xfff00000   (3072 kB)
[    0.000000]     vmalloc : 0xf0000000 - 0xff000000   ( 240 MB)
[    0.000000]     lowmem  : 0xc0000000 - 0xef800000   ( 760 MB)
[    0.000000]     modules : 0xbf000000 - 0xc0000000   (  16 MB)
[    0.000000]       .text : 0xc0008000 - 0xc05ea5f4   (6026 kB)
[    0.000000]       .init : 0xc05eb000 - 0xc063c000   ( 324 kB)
[    0.000000]       .data : 0xc063c000 - 0xc0680b30   ( 275 kB)
[    0.000000]        .bss : 0xc0680b30 - 0xc06a1a38   ( 132 kB)
[    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2, Nodes=1
[    0.000000] Hierarchical RCU implementation.
[    0.000000]  Build-time adjustment of leaf fanout to 32.
[    0.000000] NR_IRQS:16 nr_irqs:16 16
[    0.000000] L2C-310 enabling early BRESP for Cortex-A9
[    0.000000] L2C-310 full line of zeros enabled for Cortex-A9
[    0.000000] L2C-310 ID prefetch enabled, offset 1 lines
[    0.000000] L2C-310 dynamic clock gating enabled, standby mode enabled
[    0.000000] L2C-310 cache controller enabled, 8 ways, 512 kB
[    0.000000] L2C-310: CACHE_ID 0x410030c9, AUX_CTRL 0x76060001
[    0.000000] clocksource: timer1: mask: 0xffffffff max_cycles:
0xffffffff, max_idle_ns: 19112604467 ns
[    0.000005] sched_clock: 32 bits at 100MHz, resolution 10ns, wraps
every 21474836475ns
[    0.000134] Console: colour dummy device 80x30
[    0.000151] Calibrating delay loop... 1836.64 BogoMIPS (lpj=9183232)
[    0.059867] pid_max: default: 32768 minimum: 301
[    0.059938] Mount-cache hash table entries: 2048 (order: 1, 8192 bytes)
[    0.059947] Mountpoint-cache hash table entries: 2048 (order: 1, 8192
bytes)
[    0.060411] CPU: Testing write buffer coherency: ok
[    0.060586] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
[    0.060735] Setting up static identity map for 0x8280 - 0x82d8
[    0.119895] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
[    0.119954] Brought up 2 CPUs
[    0.119968] SMP: Total of 2 processors activated (3679.84 BogoMIPS).
[    0.119974] CPU: All CPU(s) started in SVC mode.
[    0.120594] devtmpfs: initialized
[    0.124057] VFP support v0.3: implementor 41 architecture 3 part 30
variant 9 rev 4
[    0.124296] clocksource: jiffies: mask: 0xffffffff max_cycles:
0xffffffff, max_idle_ns: 19112604462750000 ns
[    0.125249] NET: Registered protocol family 16
[    0.125991] DMA: preallocated 256 KiB pool for atomic coherent
allocations
[    0.129791] hw-breakpoint: found 5 (+1 reserved) breakpoint and 1
watchpoint registers.
[    0.129803] hw-breakpoint: maximum watchpoint size is 4 bytes.
[    0.163465] SCSI subsystem initialized
[    0.163736] usbcore: registered new interface driver usbfs
[    0.163793] usbcore: registered new interface driver hub
[    0.163848] usbcore: registered new device driver usb
[    0.164469] pps_core: LinuxPPS API ver. 1 registered
[    0.164478] pps_core: Software ver. 5.3.6 - Copyright 2005-2007
Rodolfo Giometti <[email protected]>
[    0.164508] PTP clock support registered
[    0.165340] clocksource: Switched to clocksource timer1
[    0.188264] NET: Registered protocol family 2
[    0.188761] TCP established hash table entries: 8192 (order: 3, 32768
bytes)
[    0.188839] TCP bind hash table entries: 8192 (order: 4, 65536 bytes)
[    0.188961] TCP: Hash tables configured (established 8192 bind 8192)
[    0.189040] UDP hash table entries: 512 (order: 2, 16384 bytes)
[    0.189083] UDP-Lite hash table entries: 512 (order: 2, 16384 bytes)
[    0.189250] NET: Registered protocol family 1
[    0.189570] RPC: Registered named UNIX socket transport module.
[    0.189579] RPC: Registered udp transport module.
[    0.189584] RPC: Registered tcp transport module.
[    0.189589] RPC: Registered tcp NFSv4.1 backchannel transport module.
[    0.190622] futex hash table entries: 512 (order: 3, 32768 bytes)
[    0.199426] ntfs: driver 2.1.32 [Flags: R/W].
[    0.200257] io scheduler noop registered (default)
[    0.204012] Serial: 8250/16550 driver, 2 ports, IRQ sharing disabled
[    0.204920] console [ttyS0] disabled
[    0.204954] ffc02000.serial0: ttyS0 at MMIO 0xffc02000 (irq = 34,
base_baud = 6250000) is a 16550A
[    0.740840] console [ttyS0] enabled
[    0.744835] ffc03000.serial1: ttyS1 at MMIO 0xffc03000 (irq = 35,
base_baud = 6250000) is a 16550A
[    0.755333] brd: module loaded
[    0.758903] CAN device driver interface
[    0.763108] stmmac - user ID: 0x10, Synopsys ID: 0x37
[    0.768216]  Ring mode enabled
[    0.771259]  DMA HW capability register supported
[    0.775776]  Enhanced/Alternate descriptors
[    0.780122]  Enabled extended descriptors
[    0.784112]  RX Checksum Offload Engine supported (type 2)
[    0.789585]  TX Checksum insertion supported
[    0.793835]  Enable RX Mitigation via HW Watchdog Timer
[    0.804992] libphy: stmmac: probed
[    0.808410] eth0: PHY ID 00221611 at 4 IRQ POLL (stmmac-0:04) active
[    1.665377] dwc2 ffb40000.usb: DWC OTG Controller
[    1.670086] dwc2 ffb40000.usb: new USB bus registered, assigned bus
number 1
[    1.677138] dwc2 ffb40000.usb: irq 36, io mem 0x00000000
[    1.683139] hub 1-0:1.0: USB hub found
[    1.686917] hub 1-0:1.0: 1 port detected
[    1.691477] mousedev: PS/2 mouse device common for all mice
[    1.697271] i2c /dev entries driver
[    1.701325] Synopsys Designware Multimedia Card Interface Driver
[    1.707550] dw_mmc ff704000.dwmmc0: IDMAC supports 32-bit address mode.
[    1.714163] dw_mmc ff704000.dwmmc0: Using PIO mode.
[    1.719035] dw_mmc ff704000.dwmmc0: Version ID is 240a
[    1.724185] dw_mmc ff704000.dwmmc0: DW MMC controller at irq 30,32
bit host data width,1024 deep fifo
[    1.733502] dw_mmc ff704000.dwmmc0: Got CD GPIO
[    1.775329] dw_mmc ff704000.dwmmc0: 1 slots initialized
[    1.780769] usbcore: registered new interface driver usbhid
[    1.786338] usbhid: USB HID core driver
[    1.790327] oprofile: no performance counters
[    1.794750] oprofile: using timer interrupt.
[    1.799920] NET: Registered protocol family 10
[    1.805084] sit: IPv6 over IPv4 tunneling driver
[    1.810277] NET: Registered protocol family 17
[    1.814726] NET: Registered protocol family 15
[    1.819173] can: controller area network core (rev 20120528 abi 9)
[    1.825384] NET: Registered protocol family 29
[    1.829817] can: raw protocol (rev 20120528)
[    1.834073] can: broadcast manager protocol (rev 20120528 t)
[    1.839727] can: netlink gateway (rev 20130117) max_hops=1
[    1.845361] 8021q: 802.1Q VLAN Support v1.8
[    1.849630] ThumbEE CPU extension supported.
[    1.853900] Registering SWP/SWPB emulation handler
[    1.854029] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req
50000000Hz, actual 50000000HZ div = 0)
[    1.854079] mmc0: new high speed SDHC card at address aaaa
[    1.854496] mmcblk0: mmc0:aaaa SU32G 29.7 GiB
[    1.855953]  mmcblk0: p1 p2 p3
[    1.985343] Sending DHCP requests ..
[    5.955860] socfpga-dwmac ff702000.ethernet eth0: Link is Up -
1Gbps/Full - flow control rx/tx
[    9.105279] .... timed out!
[   85.166664] IP-Config: Retrying forever (NFS root)...
[   85.256666] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
[   89.245860] socfpga-dwmac ff702000.ethernet eth0: Link is Up -
1Gbps/Full - flow control rx/tx
[   89.255301] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
[   89.275329] Sending DHCP requests ...... timed out!
[  161.306641] IP-Config: Retrying forever (NFS root)...
[  161.396643] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
[  165.385861] socfpga-dwmac ff702000.ethernet eth0: Link is Up -
1Gbps/Full - flow control rx/tx
[  165.395301] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready

Florian Fainelli

2015-10-15 20:03 UTC #1 lore raw

Quoted 5 lines · Dinh Nguyen

Hi,
commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not the bus' parent." seems to have broken ethernet support for the SoCFPGA platform which is using the stmmac ethernet driver.

It is not clear to me how this relates to what you are seeing yet.

Quoted 7 lines

It appears that during DHCP, it cannot get an IP address. This only happens if ethernet was not used by the bootloader to tftp an kernel image. If I use the bootloader to tftp an image then ethernet is working fine. So I think the PHY is not getting enabled properly.
If I revert this patch, then ethernet is back to working on the platform.

Is the Device Tree source for this platform available somewhere to look at?

Quoted 192 lines

I just tested this in the v4.3-rc5.

Will continue to debug, but was wondering if anyone has seen this issue.

Thanks for any help/comments.

Dinh

Bootlog below.
----

[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Initializing cgroup subsys cpuset
[    0.000000] Linux version 4.3.0-rc5-00037-g5b5f145
(dinguyen@linux-builds1) (gcc version 4.7.3 20130226 (prerelease) (crosstool
-NG linaro-1.13.1-4.7-2013.03-20130313 - Linaro GCC 2013.03) ) #1 SMP
Thu Oct 15 13:55:48 CDT 2015
[    0.000000] CPU: ARMv7 Processor [413fc090] revision 0 (ARMv7),
cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing
instruction cache
[    0.000000] Machine model: Altera SOCFPGA Cyclone V SoC Development Kit
[    0.000000] Truncating RAM at 0x00000000-0x40000000 to -0x2f800000
[    0.000000] Consider using a HIGHMEM enabled kernel.
[    0.000000] Memory policy: Data cache writealloc
[    0.000000] On node 0 totalpages: 194560
[    0.000000] free_area_init_node: node 0, pgdat c067c080, node_mem_map
ef20b000
[    0.000000]   Normal zone: 1520 pages used for memmap
[    0.000000]   Normal zone: 0 pages reserved
[    0.000000]   Normal zone: 194560 pages, LIFO batch:31
[    0.000000] PERCPU: Embedded 13 pages/cpu @ef1df000 s21760 r8192
d23296 u53248
[    0.000000] pcpu-alloc: s21760 r8192 d23296 u53248 alloc=13*4096
[    0.000000] pcpu-alloc: [0] 0 [0] 1
[    0.000000] Built 1 zonelists in Zone order, mobility grouping on.
Total pages: 193040
[    0.000000] Kernel command line: console=ttyS0,115200 root=/dev/nfs
rw nfsroot=137.57.160.210:/home/dinguyen/rootfs_yocto ip=dh
cp debug
[    0.000000] PID hash table entries: 4096 (order: 2, 16384 bytes)
[    0.000000] Dentry cache hash table entries: 131072 (order: 7, 524288
bytes)
[    0.000000] Inode-cache hash table entries: 65536 (order: 6, 262144
bytes)
[    0.000000] Memory: 764368K/778240K available (4696K kernel code,
274K rwdata, 1328K rodata, 324K init, 131K bss, 13872K reserv
ed, 0K cma-reserved)
[    0.000000] Virtual kernel memory layout:
[    0.000000]     vector  : 0xffff0000 - 0xffff1000   (   4 kB)
[    0.000000]     fixmap  : 0xffc00000 - 0xfff00000   (3072 kB)
[    0.000000]     vmalloc : 0xf0000000 - 0xff000000   ( 240 MB)
[    0.000000]     lowmem  : 0xc0000000 - 0xef800000   ( 760 MB)
[    0.000000]     modules : 0xbf000000 - 0xc0000000   (  16 MB)
[    0.000000]       .text : 0xc0008000 - 0xc05ea5f4   (6026 kB)
[    0.000000]       .init : 0xc05eb000 - 0xc063c000   ( 324 kB)
[    0.000000]       .data : 0xc063c000 - 0xc0680b30   ( 275 kB)
[    0.000000]        .bss : 0xc0680b30 - 0xc06a1a38   ( 132 kB)
[    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=2, Nodes=1
[    0.000000] Hierarchical RCU implementation.
[    0.000000]  Build-time adjustment of leaf fanout to 32.
[    0.000000] NR_IRQS:16 nr_irqs:16 16
[    0.000000] L2C-310 enabling early BRESP for Cortex-A9
[    0.000000] L2C-310 full line of zeros enabled for Cortex-A9
[    0.000000] L2C-310 ID prefetch enabled, offset 1 lines
[    0.000000] L2C-310 dynamic clock gating enabled, standby mode enabled
[    0.000000] L2C-310 cache controller enabled, 8 ways, 512 kB
[    0.000000] L2C-310: CACHE_ID 0x410030c9, AUX_CTRL 0x76060001
[    0.000000] clocksource: timer1: mask: 0xffffffff max_cycles:
0xffffffff, max_idle_ns: 19112604467 ns
[    0.000005] sched_clock: 32 bits at 100MHz, resolution 10ns, wraps
every 21474836475ns
[    0.000134] Console: colour dummy device 80x30
[    0.000151] Calibrating delay loop... 1836.64 BogoMIPS (lpj=9183232)
[    0.059867] pid_max: default: 32768 minimum: 301
[    0.059938] Mount-cache hash table entries: 2048 (order: 1, 8192 bytes)
[    0.059947] Mountpoint-cache hash table entries: 2048 (order: 1, 8192
bytes)
[    0.060411] CPU: Testing write buffer coherency: ok
[    0.060586] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
[    0.060735] Setting up static identity map for 0x8280 - 0x82d8
[    0.119895] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001
[    0.119954] Brought up 2 CPUs
[    0.119968] SMP: Total of 2 processors activated (3679.84 BogoMIPS).
[    0.119974] CPU: All CPU(s) started in SVC mode.
[    0.120594] devtmpfs: initialized
[    0.124057] VFP support v0.3: implementor 41 architecture 3 part 30
variant 9 rev 4
[    0.124296] clocksource: jiffies: mask: 0xffffffff max_cycles:
0xffffffff, max_idle_ns: 19112604462750000 ns
[    0.125249] NET: Registered protocol family 16
[    0.125991] DMA: preallocated 256 KiB pool for atomic coherent
allocations
[    0.129791] hw-breakpoint: found 5 (+1 reserved) breakpoint and 1
watchpoint registers.
[    0.129803] hw-breakpoint: maximum watchpoint size is 4 bytes.
[    0.163465] SCSI subsystem initialized
[    0.163736] usbcore: registered new interface driver usbfs
[    0.163793] usbcore: registered new interface driver hub
[    0.163848] usbcore: registered new device driver usb
[    0.164469] pps_core: LinuxPPS API ver. 1 registered
[    0.164478] pps_core: Software ver. 5.3.6 - Copyright 2005-2007
Rodolfo Giometti <[email protected]>
[    0.164508] PTP clock support registered
[    0.165340] clocksource: Switched to clocksource timer1
[    0.188264] NET: Registered protocol family 2
[    0.188761] TCP established hash table entries: 8192 (order: 3, 32768
bytes)
[    0.188839] TCP bind hash table entries: 8192 (order: 4, 65536 bytes)
[    0.188961] TCP: Hash tables configured (established 8192 bind 8192)
[    0.189040] UDP hash table entries: 512 (order: 2, 16384 bytes)
[    0.189083] UDP-Lite hash table entries: 512 (order: 2, 16384 bytes)
[    0.189250] NET: Registered protocol family 1
[    0.189570] RPC: Registered named UNIX socket transport module.
[    0.189579] RPC: Registered udp transport module.
[    0.189584] RPC: Registered tcp transport module.
[    0.189589] RPC: Registered tcp NFSv4.1 backchannel transport module.
[    0.190622] futex hash table entries: 512 (order: 3, 32768 bytes)
[    0.199426] ntfs: driver 2.1.32 [Flags: R/W].
[    0.200257] io scheduler noop registered (default)
[    0.204012] Serial: 8250/16550 driver, 2 ports, IRQ sharing disabled
[    0.204920] console [ttyS0] disabled
[    0.204954] ffc02000.serial0: ttyS0 at MMIO 0xffc02000 (irq = 34,
base_baud = 6250000) is a 16550A
[    0.740840] console [ttyS0] enabled
[    0.744835] ffc03000.serial1: ttyS1 at MMIO 0xffc03000 (irq = 35,
base_baud = 6250000) is a 16550A
[    0.755333] brd: module loaded
[    0.758903] CAN device driver interface
[    0.763108] stmmac - user ID: 0x10, Synopsys ID: 0x37
[    0.768216]  Ring mode enabled
[    0.771259]  DMA HW capability register supported
[    0.775776]  Enhanced/Alternate descriptors
[    0.780122]  Enabled extended descriptors
[    0.784112]  RX Checksum Offload Engine supported (type 2)
[    0.789585]  TX Checksum insertion supported
[    0.793835]  Enable RX Mitigation via HW Watchdog Timer
[    0.804992] libphy: stmmac: probed
[    0.808410] eth0: PHY ID 00221611 at 4 IRQ POLL (stmmac-0:04) active
[    1.665377] dwc2 ffb40000.usb: DWC OTG Controller
[    1.670086] dwc2 ffb40000.usb: new USB bus registered, assigned bus
number 1
[    1.677138] dwc2 ffb40000.usb: irq 36, io mem 0x00000000
[    1.683139] hub 1-0:1.0: USB hub found
[    1.686917] hub 1-0:1.0: 1 port detected
[    1.691477] mousedev: PS/2 mouse device common for all mice
[    1.697271] i2c /dev entries driver
[    1.701325] Synopsys Designware Multimedia Card Interface Driver
[    1.707550] dw_mmc ff704000.dwmmc0: IDMAC supports 32-bit address mode.
[    1.714163] dw_mmc ff704000.dwmmc0: Using PIO mode.
[    1.719035] dw_mmc ff704000.dwmmc0: Version ID is 240a
[    1.724185] dw_mmc ff704000.dwmmc0: DW MMC controller at irq 30,32
bit host data width,1024 deep fifo
[    1.733502] dw_mmc ff704000.dwmmc0: Got CD GPIO
[    1.775329] dw_mmc ff704000.dwmmc0: 1 slots initialized
[    1.780769] usbcore: registered new interface driver usbhid
[    1.786338] usbhid: USB HID core driver
[    1.790327] oprofile: no performance counters
[    1.794750] oprofile: using timer interrupt.
[    1.799920] NET: Registered protocol family 10
[    1.805084] sit: IPv6 over IPv4 tunneling driver
[    1.810277] NET: Registered protocol family 17
[    1.814726] NET: Registered protocol family 15
[    1.819173] can: controller area network core (rev 20120528 abi 9)
[    1.825384] NET: Registered protocol family 29
[    1.829817] can: raw protocol (rev 20120528)
[    1.834073] can: broadcast manager protocol (rev 20120528 t)
[    1.839727] can: netlink gateway (rev 20130117) max_hops=1
[    1.845361] 8021q: 802.1Q VLAN Support v1.8
[    1.849630] ThumbEE CPU extension supported.
[    1.853900] Registering SWP/SWPB emulation handler
[    1.854029] mmc_host mmc0: Bus speed (slot 0) = 50000000Hz (slot req
50000000Hz, actual 50000000HZ div = 0)
[    1.854079] mmc0: new high speed SDHC card at address aaaa
[    1.854496] mmcblk0: mmc0:aaaa SU32G 29.7 GiB
[    1.855953]  mmcblk0: p1 p2 p3
[    1.985343] Sending DHCP requests ..
[    5.955860] socfpga-dwmac ff702000.ethernet eth0: Link is Up -
1Gbps/Full - flow control rx/tx
[    9.105279] .... timed out!
[   85.166664] IP-Config: Retrying forever (NFS root)...
[   85.256666] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
[   89.245860] socfpga-dwmac ff702000.ethernet eth0: Link is Up -
1Gbps/Full - flow control rx/tx
[   89.255301] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
[   89.275329] Sending DHCP requests ...... timed out!
[  161.306641] IP-Config: Retrying forever (NFS root)...
[  161.396643] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
[  165.385861] socfpga-dwmac ff702000.ethernet eth0: Link is Up -
1Gbps/Full - flow control rx/tx
[  165.395301] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready

Signature · 1 line

Florian

Dinh Nguyen

author

2015-10-15 19:59 UTC #2 lore raw

Quoted 10 lines · Florian Fainelli

Quoted 5 lines · Dinh Nguyen
Hi,
commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not the bus' parent." seems to have broken ethernet support for the SoCFPGA platform which is using the stmmac ethernet driver.
It is not clear to me how this relates to what you are seeing yet.
Quoted 1 line

Quoted 9 lines

Quoted 6 lines
It appears that during DHCP, it cannot get an IP address. This only happens if ethernet was not used by the bootloader to tftp an kernel image. If I use the bootloader to tftp an image then ethernet is working fine. So I think the PHY is not getting enabled properly.
If I revert this patch, then ethernet is back to working on the platform.
Is the Device Tree source for this platform available somewhere to look at?

Yes, I'm using the DTS that is in the mainline:

arch/arm/boot/dts/socfpga.dtsi
arch/arm/boot/dts/socfpga_cyclone5.dtsi
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts

Dinh

Florian Fainelli

2015-10-15 20:26 UTC #3 lore raw

Quoted 18 lines · Dinh Nguyen

Quoted 17 lines · Florian Fainelli
Quoted 5 lines · Dinh Nguyen
Hi,
commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not the bus' parent." seems to have broken ethernet support for the SoCFPGA platform which is using the stmmac ethernet driver.
It is not clear to me how this relates to what you are seeing yet.
Quoted 7 lines
It appears that during DHCP, it cannot get an IP address. This only happens if ethernet was not used by the bootloader to tftp an kernel image. If I use the bootloader to tftp an image then ethernet is working fine. So I think the PHY is not getting enabled properly.
If I revert this patch, then ethernet is back to working on the platform.

Quoted 8 lines

Quoted 2 lines
Is the Device Tree source for this platform available somewhere to look at?
Yes, I'm using the DTS that is in the mainline:
arch/arm/boot/dts/socfpga.dtsi
arch/arm/boot/dts/socfpga_cyclone5.dtsi
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts

There are no PHY devices in any of these DTS files, instead there is the non-standard "phy-addr" property which is set to 0xffffffff supposedly to indicate that the MDIO bus should be scanned. This is likely part of your problem. The stmmac driver seems to be looking for "snps,phy-addr" and not "phy-addr", so I am not even clear how this is supposed to work, and the driver mentions this custom property is deprecated anyway.

The core problem is in drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c::stmmac_mdio_register which manually detects the PHY, that is mostly fine, except that it does not really seem to work here for a reason that is still unclear to me.

Your Ethernet PHYs need to be declared in Device Tree, see Documentation/devicetree/bindings/net/phy.txt

but aside from that, what should be the PHY driver used by our platform? ID 00221611 does not seem to be claimed by any known PHY driver, so that leave us with the Generic PHY driver I guess?

Do you have specific PHY fixups required for this platform?

Signature · 1 line

Florian

David Daney

2015-10-15 20:35 UTC #4 lore raw

Quoted 28 lines · Florian Fainelli

Quoted 18 lines · Dinh Nguyen
Quoted 17 lines · Florian Fainelli
Quoted 5 lines · Dinh Nguyen
Hi,
commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not the bus' parent." seems to have broken ethernet support for the SoCFPGA platform which is using the stmmac ethernet driver.
It is not clear to me how this relates to what you are seeing yet.
Quoted 7 lines
It appears that during DHCP, it cannot get an IP address. This only happens if ethernet was not used by the bootloader to tftp an kernel image. If I use the bootloader to tftp an image then ethernet is working fine. So I think the PHY is not getting enabled properly.
If I revert this patch, then ethernet is back to working on the platform.
Quoted 8 lines
Quoted 2 lines
Is the Device Tree source for this platform available somewhere to look at?
Yes, I'm using the DTS that is in the mainline:
arch/arm/boot/dts/socfpga.dtsi
arch/arm/boot/dts/socfpga_cyclone5.dtsi
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts

Quoted 7 lines

There are no PHY devices in any of these DTS files, instead there is the non-standard "phy-addr" property which is set to 0xffffffff supposedly to indicate that the MDIO bus should be scanned. This is likely part of your problem. The stmmac driver seems to be looking for "snps,phy-addr" and not "phy-addr", so I am not even clear how this is supposed to work, and the driver mentions this custom property is deprecated anyway.

I think it is OK not to expose the PHYs in the device tree if they can be accurately probed without knowing information from the device tree.

Quoted 5 lines

The core problem is in drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c::stmmac_mdio_register which manually detects the PHY, that is mostly fine, except that it does not really seem to work here for a reason that is still unclear to me.

I agree with this analysis. I have also been looking at the code and cannot see anything that depends on what the parent device of the PHY is. So it is a bit mystifying.

I noticed in your original message you had in the boot log this:

.
.
.

[    0.804992] libphy: stmmac: probed
[    0.808410] eth0: PHY ID 00221611 at 4 IRQ POLL (stmmac-0:04) active

.
.
.

Does this text change with and without the 8b63ec1837fa patch?

Dinh Nguyen

author

2015-10-15 20:49 UTC #5 lore raw

Quoted 60 lines · David Daney

Quoted 32 lines · Florian Fainelli
Quoted 25 lines · Dinh Nguyen
Quoted 17 lines · Florian Fainelli
Quoted 6 lines · Dinh Nguyen
Hi,
commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not the bus' parent." seems to have broken ethernet support for the SoCFPGA
platform which is using the stmmac ethernet driver.
It is not clear to me how this relates to what you are seeing yet.
Quoted 7 lines
It appears that during DHCP, it cannot get an IP address. This only happens if ethernet was not used by the bootloader to tftp an kernel image. If I use the bootloader to tftp an image then ethernet is working
fine. So I think the PHY is not getting enabled properly.
Quoted 6 lines
Quoted 2 lines
If I revert this patch, then ethernet is back to working on the platform.
Is the Device Tree source for this platform available somewhere to look at?
Quoted 5 lines
Yes, I'm using the DTS that is in the mainline:
arch/arm/boot/dts/socfpga.dtsi
arch/arm/boot/dts/socfpga_cyclone5.dtsi
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
Quoted 7 lines
There are no PHY devices in any of these DTS files, instead there is the non-standard "phy-addr" property which is set to 0xffffffff supposedly to indicate that the MDIO bus should be scanned. This is likely part of your problem. The stmmac driver seems to be looking for "snps,phy-addr" and not "phy-addr", so I am not even clear how this is supposed to work, and the driver mentions this custom property is deprecated anyway.
I think it is OK not to expose the PHYs in the device tree if they can be accurately probed without knowing information from the device tree.
Quoted 5 lines
The core problem is in drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c::stmmac_mdio_register which manually detects the PHY, that is mostly fine, except that it does not really seem to work here for a reason that is still unclear to me.
I agree with this analysis. I have also been looking at the code and cannot see anything that depends on what the parent device of the PHY is. So it is a bit mystifying.
I noticed in your original message you had in the boot log this:
.
.
.

Quoted 7 lines

[    0.804992] libphy: stmmac: probed
[    0.808410] eth0: PHY ID 00221611 at 4 IRQ POLL (stmmac-0:04) active
.
.
.
Does this text change with and without the 8b63ec1837fa patch?

No, this text does not change with/without the 8b63ec1837fa patch.

Dinh

Florian Fainelli

2015-10-15 21:30 UTC #6 lore raw

Quoted 4 lines · Dinh Nguyen

Quoted 2 lines
Does this text change with and without the 8b63ec1837fa patch?
No, this text does not change with/without the 8b63ec1837fa patch.

Could you instrument mdiobus_scan(), get_phy_device() and phy_device_create/register to see if the parent is NULL, non-NULL?

So far, I cannot see what is wrong with David's changes, quite the contrary, and if there was something wrong with the PHY device creation, it should not get you that far.

You have not answered my previous question though, do you have PHY fixups registered for that ID?

Signature · 1 line

Florian

Dinh Nguyen

author

2015-10-16 02:32 UTC #7 lore raw

Quoted 9 lines · Florian Fainelli

Quoted 4 lines · Dinh Nguyen
Quoted 2 lines
Does this text change with and without the 8b63ec1837fa patch?
No, this text does not change with/without the 8b63ec1837fa patch.
Could you instrument mdiobus_scan(), get_phy_device() and phy_device_create/register to see if the parent is NULL, non-NULL?

Yes, I can do that.

Quoted 6 lines

So far, I cannot see what is wrong with David's changes, quite the contrary, and if there was something wrong with the PHY device creation, it should not get you that far.
You have not answered my previous question though, do you have PHY fixups registered for that ID?

By fixups, do you mean the skew values that are in the device tree? Those are the only fixups that I have the PHY.

Another debugging point, the SoCFPGA board has a Micrel ksz9021 PHY attached to the ethernet port. What I'm seeing is that with 8b63ec1837fa patch, when the call to ksz9021_config_init() is made both of_node and dev->parent->of_node are NULL, without the patch the dev->parent->of_node is a valid pointer. Thus the skew values get programmed to the phy.

BR,
Dinh

Andrew Lunn

maintainer

2015-10-16 03:31 UTC #8 lore raw

Quoted 5 lines

Another debugging point, the SoCFPGA board has a Micrel ksz9021 PHY attached to the ethernet port. What I'm seeing is that with 8b63ec1837fa patch, when the call to ksz9021_config_init() is made both of_node and dev->parent->of_node are NULL, without the patch the dev->parent->of_node is a valid pointer. Thus the skew values get programmed to the phy.

Ah!

You have the phy device tree parameters in the wrong place. These are phy paramters, so should really be in the phy node. But socfpga_cyclone5_socdk.dts has them in the MAC node.

There is nothing in Documentation/devicetree/bindings/net/micrel.txt which says you are allowed to place them in the MAC node. Obviously the code did allow this, which is what has now broken.

Andrew

Dinh Nguyen

author

2015-10-16 14:38 UTC #9 lore raw

Quoted 6 lines · Andrew Lunn

Quoted 5 lines
Another debugging point, the SoCFPGA board has a Micrel ksz9021 PHY attached to the ethernet port. What I'm seeing is that with 8b63ec1837fa patch, when the call to ksz9021_config_init() is made both of_node and dev->parent->of_node are NULL, without the patch the dev->parent->of_node is a valid pointer. Thus the skew values get programmed to the phy.

Quoted 6 lines

Ah!
You have the phy device tree parameters in the wrong place. These are phy paramters, so should really be in the phy node. But socfpga_cyclone5_socdk.dts has them in the MAC node.

Alright, let me see if I can rework the DTS.

Quoted 4 lines

There is nothing in Documentation/devicetree/bindings/net/micrel.txt which says you are allowed to place them in the MAC node. Obviously the code did allow this, which is what has now broken.

I was following Documentation/devicetree/bindings/net/micrel-ksz90x1.txt and in this document I was following the autodetected PHY example. Did I mis-interpret the example?

BR,
Dinh

Andrew Lunn

maintainer

2015-10-16 15:03 UTC #10 lore raw

Quoted 8 lines · Dinh Nguyen

Quoted 6 lines · Andrew Lunn
Quoted 5 lines
Another debugging point, the SoCFPGA board has a Micrel ksz9021 PHY attached to the ethernet port. What I'm seeing is that with 8b63ec1837fa patch, when the call to ksz9021_config_init() is made both of_node and dev->parent->of_node are NULL, without the patch the dev->parent->of_node is a valid pointer. Thus the skew values get programmed to the phy.

Quoted 8 lines

Quoted 6 lines
Ah!
You have the phy device tree parameters in the wrong place. These are phy paramters, so should really be in the phy node. But socfpga_cyclone5_socdk.dts has them in the MAC node.
Alright, let me see if I can rework the DTS.

Well, we are not supposed to break device tree bindings. So we should try to make this work again.

Quoted 7 lines

Quoted 3 lines
There is nothing in Documentation/devicetree/bindings/net/micrel.txt which says you are allowed to place them in the MAC node. Obviously the code did allow this, which is what has now broken.
I was following Documentation/devicetree/bindings/net/micrel-ksz90x1.txt and in this document I was following the autodetected PHY example. Did I mis-interpret the example?

I was looking at the wrong binding documentation. So yes, it is documented you can do this.

But i still think it is wrong. These are phy properties, implemented by the phy, so should be in the phy node. So moving them would be good. But as i said, we should fix backwards compatibility if possible.

Andrew

Dinh Nguyen

author

2015-10-16 15:31 UTC #11 lore raw

Quoted 16 lines · Andrew Lunn

Quoted 15 lines · Dinh Nguyen
Quoted 12 lines · Andrew Lunn
Quoted 5 lines
Another debugging point, the SoCFPGA board has a Micrel ksz9021 PHY attached to the ethernet port. What I'm seeing is that with 8b63ec1837fa patch, when the call to ksz9021_config_init() is made both of_node and dev->parent->of_node are NULL, without the patch the dev->parent->of_node is a valid pointer. Thus the skew values get programmed to the phy.
Ah!
You have the phy device tree parameters in the wrong place. These are phy paramters, so should really be in the phy node. But socfpga_cyclone5_socdk.dts has them in the MAC node.

Quoted 5 lines

Quoted 1 line
Alright, let me see if I can rework the DTS.
Well, we are not supposed to break device tree bindings. So we should try to make this work again.

Great..thanks for recognizing that point.

Quoted 8 lines

Quoted 7 lines
Quoted 3 lines
There is nothing in Documentation/devicetree/bindings/net/micrel.txt which says you are allowed to place them in the MAC node. Obviously the code did allow this, which is what has now broken.
I was following Documentation/devicetree/bindings/net/micrel-ksz90x1.txt and in this document I was following the autodetected PHY example. Did I mis-interpret the example?

Quoted 8 lines

I was looking at the wrong binding documentation. So yes, it is documented you can do this.
But i still think it is wrong. These are phy properties, implemented by the phy, so should be in the phy node. So moving them would be good. But as i said, we should fix backwards compatibility if possible.

I moved the phy settings to a separate phy node, but it did not seem to make a difference at all.

So I think I'll move to inspect what Florian had suggested, and that was to look
at: drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c::stmmac_mdio_register

BR,
Dinh

Andrew Lunn

maintainer

2015-10-16 15:56 UTC #12 lore raw

Quoted 2 lines

So I think I'll move to inspect what Florian had suggested, and that was to look
at: drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c::stmmac_mdio_register

I have a suspicion. If you look at the phy driver it does:

static int ksz9021_config_init(struct phy_device *phydev)

{
        const struct device *dev = &phydev->dev;
        const struct device_node *of_node = dev->of_node;

        if (!of_node && dev->parent->of_node)
                of_node = dev->parent->of_node;

In your case, you don't have a phy node in your device tree, so of_node is NULL. So it looks in the parent device.

phylib: Make PHYs children of their MDIO bus, not the bus' parent.

changed what the parent is. It is now the mdio device. Before, i suspect it was the MAC. Hence it found your properties in the MAC node.

What i think you might want to do is change this code. Rather than look a dev->parent->of_node; you might want phydev->attached_dev->dev->of_node.

This assumes the phy has been attached to the MAC. I've no idea of the ordering, so maybe it has not been attached yet?

dp83867.c has similar code. However quick grep did not find any mainline users with properties in the MAC node. If that is true, i would suggest removing the code looking in the parent for that phy driver.

Andrew

David Daney

Acked-by

2015-10-16 16:47 UTC #13 lore raw

Quoted 5 lines · Andrew Lunn

Quoted 2 lines
So I think I'll move to inspect what Florian had suggested, and that was to look
at: drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c::stmmac_mdio_register
I have a suspicion. If you look at the phy driver it does:

Quoted 8 lines

static int ksz9021_config_init(struct phy_device *phydev)

{
         const struct device *dev = &phydev->dev;
         const struct device_node *of_node = dev->of_node;

         if (!of_node && dev->parent->of_node)
                 of_node = dev->parent->of_node;

Maybe we need to walk up the hierarchy.

Perhaps something like:

const struct device *dev_walker;

dev_walker = &phydev->dev;
do {
of_node = dev_walker->of_node;
dev_walker = dev_walker->parent;
} while (!of_node && dev_walker);

An alternative would be to assign the bus the same of_node as the bus parent.

If either approach works, you can add:

Acked-by David Daney <[email protected]>

to the patch that implements it.

Quoted 24 lines

In your case, you don't have a phy node in your device tree, so of_node is NULL. So it looks in the parent device.
phylib: Make PHYs children of their MDIO bus, not the bus' parent.
changed what the parent is. It is now the mdio device. Before, i suspect it was the MAC. Hence it found your properties in the MAC node.
What i think you might want to do is change this code. Rather than look a dev->parent->of_node; you might want phydev->attached_dev->dev->of_node.
This assumes the phy has been attached to the MAC. I've no idea of the ordering, so maybe it has not been attached yet?
dp83867.c has similar code. However quick grep did not find any mainline users with properties in the MAC node. If that is true, i would suggest removing the code looking in the parent for that phy driver.
Andrew

Dinh Nguyen

author

2015-10-16 19:10 UTC #14 lore raw

Quoted 21 lines · David Daney

Quoted 7 lines · Andrew Lunn
Quoted 4 lines
So I think I'll move to inspect what Florian had suggested, and that was to look
at:
drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c::stmmac_mdio_register
I have a suspicion. If you look at the phy driver it does:
Quoted 8 lines
static int ksz9021_config_init(struct phy_device *phydev)
{
         const struct device *dev = &phydev->dev;
         const struct device_node *of_node = dev->of_node;

         if (!of_node && dev->parent->of_node)
                 of_node = dev->parent->of_node;
Maybe we need to walk up the hierarchy.
Perhaps something like:

Quoted 8 lines

const struct device *dev_walker;
dev_walker = &phydev->dev;
do {
   of_node = dev_walker->of_node;
   dev_walker = dev_walker->parent;
} while (!of_node && dev_walker);

The above code seems to have fixed the issue.

Quoted 33 lines

An alternative would be to assign the bus the same of_node as the bus parent.
If either approach works, you can add:
Acked-by David Daney <[email protected]>
to the patch that implements it.
Quoted 24 lines
In your case, you don't have a phy node in your device tree, so of_node is NULL. So it looks in the parent device.
phylib: Make PHYs children of their MDIO bus, not the bus' parent.
changed what the parent is. It is now the mdio device. Before, i suspect it was the MAC. Hence it found your properties in the MAC node.
What i think you might want to do is change this code. Rather than look a dev->parent->of_node; you might want phydev->attached_dev->dev->of_node.
This assumes the phy has been attached to the MAC. I've no idea of the ordering, so maybe it has not been attached yet?
dp83867.c has similar code. However quick grep did not find any mainline users with properties in the MAC node. If that is true, i would suggest removing the code looking in the parent for that phy driver.
Andrew

BR,
Dinh

Andrew Lunn

maintainer

2015-10-16 19:38 UTC #15 lore raw

Quoted 6 lines

Quoted 6 lines
Maybe we need to walk up the hierarchy.
Perhaps something like:
const struct device *dev_walker;

Quoted 8 lines

Quoted 6 lines
dev_walker = &phydev->dev;
do {
   of_node = dev_walker->of_node;
   dev_walker = dev_walker->parent;
} while (!of_node && dev_walker);
The above code seems to have fixed the issue.

What i don't like about this is that it allows you to put these properties in the mdio device node. These are phy properties, not mdio properties....

If phydev->attached_dev->dev->of_node works, that would be my preference.

Andrew

David Daney

2015-10-16 20:24 UTC #16 lore raw

Quoted 13 lines · Andrew Lunn

Quoted 13 lines
Quoted 12 lines
Maybe we need to walk up the hierarchy.
Perhaps something like:
const struct device *dev_walker;
dev_walker = &phydev->dev;
do {
    of_node = dev_walker->of_node;
    dev_walker = dev_walker->parent;
} while (!of_node && dev_walker);

Quoted 5 lines

Quoted 1 line
The above code seems to have fixed the issue.
What i don't like about this is that it allows you to put these properties in the mdio device node. These are phy properties, not mdio properties....

Yes, we know that the device tree is not correctly formed.

You have a choice:

A) Fix the device tree and any code that may have to change to work with a good device tree.

B) Change the code to work with the screwy existing device tree. The above seems to work, other things may also be possible.

I can't really make any decisions as to what the best way to proceed is, as I neither have the hardware in question, nor the time to work on it.

David Daney

Quoted 6 lines

If phydev->attached_dev->dev->of_node works, that would be my preference.
Andrew

Andrew Lunn

maintainer

2015-10-16 20:29 UTC #17 lore raw

Quoted 27 lines · David Daney

Quoted 13 lines · Andrew Lunn
Quoted 13 lines
Quoted 12 lines
Maybe we need to walk up the hierarchy.
Perhaps something like:
const struct device *dev_walker;
dev_walker = &phydev->dev;
do {
   of_node = dev_walker->of_node;
   dev_walker = dev_walker->parent;
} while (!of_node && dev_walker);
Quoted 5 lines
Quoted 1 line
The above code seems to have fixed the issue.
What i don't like about this is that it allows you to put these properties in the mdio device node. These are phy properties, not mdio properties....
Yes, we know that the device tree is not correctly formed.
You have a choice:
A) Fix the device tree and any code that may have to change to work with a good device tree.

Quoted 6 lines

B) Change the code to work with the screwy existing device tree. The above seems to work, other things may also be possible.
I can't really make any decisions as to what the best way to proceed is, as I neither have the hardware in question, nor the time to work on it.

Hi hope Dinh will test with phydev->attached_dev->dev->of_node.

Andrew

Dinh Nguyen

author

2015-10-19 15:14 UTC #18 lore raw

In reply to Andrew Lunn

+CC Giuseppe Cavallaro
+CC STi and Rockchip Maintainers

This is approaching beyond my breadth of knowledge on this subject, so I just wanted to get some further insight.

Quoted 13 lines · Andrew Lunn

Quoted 13 lines
Quoted 12 lines
Maybe we need to walk up the hierarchy.
Perhaps something like:
const struct device *dev_walker;
dev_walker = &phydev->dev;
do {
   of_node = dev_walker->of_node;
   dev_walker = dev_walker->parent;
} while (!of_node && dev_walker);

Quoted 6 lines

Quoted 1 line
The above code seems to have fixed the issue.
What i don't like about this is that it allows you to put these properties in the mdio device node. These are phy properties, not mdio properties....

AFAICT, the stmmac driver is allowing for the phy node to be part of the mdio. In the function, stmmac_init_phy(), there is a separate check of a standalone phy_node, and the case where the phy is part of the mdio.

commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not the bus' parent." is now placing a hard requirement to have a PHY as a separate node. For now this is only breaking SoCFPGA, but perhaps might have affected STi41x and RockChip, but maybe haven't been spotted because perhaps the testing has the bootloader setting up the PHYs?

Quoted 3 lines

If phydev->attached_dev->dev->of_node works, that would be my preference.

BR,
Dinh

Dinh Nguyen

author

2015-10-19 10:57 UTC #19 lore raw

On Mon, 19 Oct 2015, Dinh Nguyen wrote:

+CC Giuseppe Cavallaro
+CC STi and Rockchip Maintainers

This is approaching beyond my breadth of knowledge on this subject, so I just wanted to get some further insight.

Quoted 16 lines · Andrew Lunn

Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
Maybe we need to walk up the hierarchy.
Perhaps something like:
const struct device *dev_walker;
dev_walker = &phydev->dev;
do {
   of_node = dev_walker->of_node;
   dev_walker = dev_walker->parent;
} while (!of_node && dev_walker);

Quoted 7 lines

Quoted 6 lines
Quoted 1 line
The above code seems to have fixed the issue.
What i don't like about this is that it allows you to put these properties in the mdio device node. These are phy properties, not mdio properties....

commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not the bus' parent." is now now placing a hard requirement that the phy must be in a separate node. For now, this is breaking SoCFPGA, but I think it may also impact Rockchip and STi?

Quoted 3 lines

Quoted 3 lines
If phydev->attached_dev->dev->of_node works, that would be my preference.

BR,
Dinh

Florian Fainelli

2015-10-16 18:17 UTC #20 lore raw

In reply to Andrew Lunn

Quoted 19 lines · Andrew Lunn

Quoted 2 lines
So I think I'll move to inspect what Florian had suggested, and that was to look
at: drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c::stmmac_mdio_register
I have a suspicion. If you look at the phy driver it does:
static int ksz9021_config_init(struct phy_device *phydev)
{
        const struct device *dev = &phydev->dev;
        const struct device_node *of_node = dev->of_node;

        if (!of_node && dev->parent->of_node)
                of_node = dev->parent->of_node;
In your case, you don't have a phy node in your device tree, so of_node is NULL. So it looks in the parent device.
phylib: Make PHYs children of their MDIO bus, not the bus' parent.

Quoted 7 lines

changed what the parent is. It is now the mdio device. Before, i suspect it was the MAC. Hence it found your properties in the MAC node.
What i think you might want to do is change this code. Rather than look a dev->parent->of_node; you might want phydev->attached_dev->dev->of_node.

Yes, that would work, by the time config_init() executes, you are guaranteed to have a valid attached_dev pointer.

Quoted 8 lines

This assumes the phy has been attached to the MAC. I've no idea of the ordering, so maybe it has not been attached yet?
dp83867.c has similar code. However quick grep did not find any mainline users with properties in the MAC node. If that is true, i would suggest removing the code looking in the parent for that phy driver.

I think we should fix these drivers to look for these properties where they were used to, as a short term plan, and then make sure that their binding document is very specific about the fact that these properties, are by definition, properties of the Ethernet PHY, not the Ethernet MAC.

Signature · 1 line

Florian

Dinh Nguyen

author

2015-10-16 03:04 UTC #21 lore raw

In reply to Florian Fainelli

Quoted 9 lines · Florian Fainelli

Quoted 4 lines · Dinh Nguyen
Quoted 2 lines
Does this text change with and without the 8b63ec1837fa patch?
No, this text does not change with/without the 8b63ec1837fa patch.
Could you instrument mdiobus_scan(), get_phy_device() and phy_device_create/register to see if the parent is NULL, non-NULL?

[    0.800479] get_phy_device bus=0xeee26c00 addr=00000004 phy_id=00221611
[    0.806735] phy_device_create dev->dev.parent=0xeee26c50
[    0.811114] phy_device_register completed
[    0.814675] mdiobus_scan phydev=0xeee27000
[    1.979034] ksz9021_config_init of_node=00000000

---- Without patch ----

[    0.801294] get_phy_device bus=0xeeef6c00 addr=00000004 phy_id=00221611
[    0.807551] phy_device_create dev->dev.parent=0xeed6cc10
[    0.811929] phy_device_register completed
[    0.815510] mdiobus_scan phydev=0xeeef7000
[    1.979034] ksz9021_config_init of_node=0xef203538
[    1.988064] ksz9021_load_values_from_of
[    1.992485] ksz9021_load_values_from_of
[    1.996905] ksz9021_load_values_from_of

BR,
Dinh

Pavel Machek

2015-12-03 20:48 UTC #22 lore raw

In reply to Florian Fainelli

Quoted 35 lines · Florian Fainelli

Quoted 18 lines · Dinh Nguyen
Quoted 17 lines · Florian Fainelli
Quoted 5 lines · Dinh Nguyen
Hi,
commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not the bus' parent." seems to have broken ethernet support for the SoCFPGA platform which is using the stmmac ethernet driver.
It is not clear to me how this relates to what you are seeing yet.
Quoted 7 lines
It appears that during DHCP, it cannot get an IP address. This only happens if ethernet was not used by the bootloader to tftp an kernel image. If I use the bootloader to tftp an image then ethernet is working fine. So I think the PHY is not getting enabled properly.
If I revert this patch, then ethernet is back to working on the platform.
Quoted 8 lines
Quoted 2 lines
Is the Device Tree source for this platform available somewhere to look at?
Yes, I'm using the DTS that is in the mainline:
arch/arm/boot/dts/socfpga.dtsi
arch/arm/boot/dts/socfpga_cyclone5.dtsi
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
There are no PHY devices in any of these DTS files, instead there is the non-standard "phy-addr" property which is set to 0xffffffff supposedly to indicate that the MDIO bus should be scanned. This is likely part of your problem. The stmmac driver seems to be looking for "snps,phy-addr" and not "phy-addr", so I am not even clear how this is supposed to work, and the driver mentions this custom property is deprecated anyway.

Quoted 7 lines

The core problem is in drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c::stmmac_mdio_register which manually detects the PHY, that is mostly fine, except that it does not really seem to work here for a reason that is still unclear to me.
Your Ethernet PHYs need to be declared in Device Tree, see Documentation/devicetree/bindings/net/phy.txt

While updating DTS might be good idea, I don't think you can simply blame this on DTS. If it worked before the change, it is supposed to work after the change, otherwise we call that change a "regression" and revert the change.

Plus, DTS is supposed to be ABI. Old DTS should still work on new kernels in ideal world.

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

David Daney

2015-12-03 21:23 UTC #23 lore raw

Quoted 41 lines · Pavel Machek

Quoted 40 lines · Florian Fainelli
Quoted 18 lines · Dinh Nguyen
Quoted 17 lines · Florian Fainelli
Quoted 5 lines · Dinh Nguyen
Hi,
commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not the bus' parent." seems to have broken ethernet support for the SoCFPGA platform which is using the stmmac ethernet driver.
It is not clear to me how this relates to what you are seeing yet.
Quoted 7 lines
It appears that during DHCP, it cannot get an IP address. This only happens if ethernet was not used by the bootloader to tftp an kernel image. If I use the bootloader to tftp an image then ethernet is working fine. So I think the PHY is not getting enabled properly.
If I revert this patch, then ethernet is back to working on the platform.
Quoted 8 lines
Quoted 2 lines
Is the Device Tree source for this platform available somewhere to look at?
Yes, I'm using the DTS that is in the mainline:
arch/arm/boot/dts/socfpga.dtsi
arch/arm/boot/dts/socfpga_cyclone5.dtsi
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
There are no PHY devices in any of these DTS files, instead there is the non-standard "phy-addr" property which is set to 0xffffffff supposedly to indicate that the MDIO bus should be scanned. This is likely part of your problem. The stmmac driver seems to be looking for "snps,phy-addr" and not "phy-addr", so I am not even clear how this is supposed to work, and the driver mentions this custom property is deprecated anyway.
The core problem is in drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c::stmmac_mdio_register which manually detects the PHY, that is mostly fine, except that it does not really seem to work here for a reason that is still unclear to me.

Quoted 7 lines

Quoted 2 lines
Your Ethernet PHYs need to be declared in Device Tree, see Documentation/devicetree/bindings/net/phy.txt
While updating DTS might be good idea, I don't think you can simply blame this on DTS. If it worked before the change, it is supposed to work after the change, otherwise we call that change a "regression" and revert the change.

FWIW: My initial patch to address the failure worked with the original DTB.

Also: userspace wasn't broken. So, the commandment about not breaking userspace wasn't broken. Although admittedly, breaking the kernel isn't good either.

Quoted 3 lines

Plus, DTS is supposed to be ABI. Old DTS should still work on new kernels in ideal world.

If you supply the device tree file in the kernel tree, it is not an ABI.

If the device tree is not part of the kernel, and instead comes from the boot firmware of the board, then you could make the ABI claim.

David Daney

Dinh Nguyen

author

2015-12-03 23:17 UTC #24 lore raw

Quoted 50 lines · David Daney

Quoted 49 lines · Pavel Machek
Quoted 47 lines · Florian Fainelli
Quoted 26 lines · Dinh Nguyen
Quoted 18 lines · Florian Fainelli
Quoted 7 lines · Dinh Nguyen
Hi,
commit "8b63ec1837fa phylib: Make PHYs children of their MDIO bus, not
the bus' parent." seems to have broken ethernet support for the SoCFPGA
platform which is using the stmmac ethernet driver.
It is not clear to me how this relates to what you are seeing yet.
Quoted 7 lines
It appears that during DHCP, it cannot get an IP address. This only happens if ethernet was not used by the bootloader to tftp an kernel image. If I use the bootloader to tftp an image then ethernet is working
fine. So I think the PHY is not getting enabled properly.
Quoted 6 lines
Quoted 2 lines
If I revert this patch, then ethernet is back to working on the platform.
Is the Device Tree source for this platform available somewhere to look at?
Quoted 5 lines
Yes, I'm using the DTS that is in the mainline:
arch/arm/boot/dts/socfpga.dtsi
arch/arm/boot/dts/socfpga_cyclone5.dtsi
arch/arm/boot/dts/socfpga_cyclone5_socdk.dts
There are no PHY devices in any of these DTS files, instead there is the non-standard "phy-addr" property which is set to 0xffffffff supposedly to indicate that the MDIO bus should be scanned. This is likely part of your problem. The stmmac driver seems to be looking for "snps,phy-addr" and not "phy-addr", so I am not even clear how this is supposed to work, and the driver mentions this custom property is deprecated anyway.
The core problem is in drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c::stmmac_mdio_register which manually detects the PHY, that is mostly fine, except that it does not really seem to work here for a reason that is still unclear to me.
Your Ethernet PHYs need to be declared in Device Tree, see Documentation/devicetree/bindings/net/phy.txt

Quoted 7 lines

Quoted 4 lines
While updating DTS might be good idea, I don't think you can simply blame this on DTS. If it worked before the change, it is supposed to work after the change, otherwise we call that change a "regression" and revert the change.
FWIW: My initial patch to address the failure worked with the original DTB.

Can I ask what patch are you referring to? I was sidetracked for a while on this issue, but I still see it failing as of v4.4-rc3. I'll try to get back to debugging this.

Dinh

Andrew Lunn

maintainer

2015-12-04 01:10 UTC #25 lore raw

Quoted 5 lines

Quoted 1 line
FWIW: My initial patch to address the failure worked with the original DTB.
Can I ask what patch are you referring to? I was sidetracked for a while on this issue, but I still see it failing as of v4.4-rc3. I'll try to get back to debugging this.

Hi Dinh

There are two different patches:

https://lkml.org/lkml/2015/10/16/669

and

https://www.mail-archive.com/[email protected]/msg83183.html

Although the first one works, it keeps searching up and up and up, so you could in theory put the phy properties a lot higher than the MAC.

The second patch restricts where it looks for the phy properties to only the MAC. But it does not work. We would like to understand why it does not work.

Andrew

Andrew Lunn

maintainer

2015-12-04 01:50 UTC #26 lore raw

Quoted 14 lines · Andrew Lunn

Quoted 5 lines
Quoted 1 line
FWIW: My initial patch to address the failure worked with the original DTB.
Can I ask what patch are you referring to? I was sidetracked for a while on this issue, but I still see it failing as of v4.4-rc3. I'll try to get back to debugging this.
Hi Dinh
There are two different patches:
https://lkml.org/lkml/2015/10/16/669
and

Quoted 8 lines

https://www.mail-archive.com/[email protected]/msg83183.html
Although the first one works, it keeps searching up and up and up, so you could in theory put the phy properties a lot higher than the MAC.
The second patch restricts where it looks for the phy properties to only the MAC. But it does not work. We would like to understand why it does not work.

Hi Dinh

Please could you run this patch and let us know what it outputs.

Thanks
Andrew

drivers/net/phy/micrel.c +13 -2

index cf6312fafea5..d7ddc0bb0e7f 100644
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/phy.h>
 #include <linux/micrel_phy.h>
+#include <linux/netdevice.h>
 #include <linux/of.h>
 #include <linux/clk.h>

@@ -339,9 +340,19 @@ static int ksz9021_config_init(struct phy_device *phydev)
 {
 	const struct device *dev = &phydev->dev;
 	const struct device_node *of_node = dev->of_node;
+	const struct device *dev_walker;
 
-	if (!of_node && dev->parent->of_node)
-		of_node = dev->parent->of_node;
+	dev_info(dev, "dev->parent: %s\n", dev_name(dev->parent));
+	dev_info(dev, "phydev->attached_dev->dev: %s\n", dev_name(&phydev->attached_dev->dev));
+
+	dev_walker = &phydev->dev;
+	do {
+		of_node = dev_walker->of_node;
+		dev_info(dev, "walking: %s %p\n",
+			 dev_name(dev_walker), of_node);
+		dev_walker = dev_walker->parent;
+
+	} while (!of_node && dev_walker);
 
 	if (of_node) {
 		ksz9021_load_values_from_of(phydev, of_node,

Dinh Nguyen

author

2015-12-04 11:27 UTC #27 lore raw

Hi Andrew,

Quoted 56 lines · Andrew Lunn

Quoted 14 lines · Andrew Lunn
Quoted 5 lines
Quoted 1 line
FWIW: My initial patch to address the failure worked with the original DTB.
Can I ask what patch are you referring to? I was sidetracked for a while on this issue, but I still see it failing as of v4.4-rc3. I'll try to get back to debugging this.
Hi Dinh
There are two different patches:
https://lkml.org/lkml/2015/10/16/669
and
Quoted 8 lines
https://www.mail-archive.com/[email protected]/msg83183.html
Although the first one works, it keeps searching up and up and up, so you could in theory put the phy properties a lot higher than the MAC.
The second patch restricts where it looks for the phy properties to only the MAC. But it does not work. We would like to understand why it does not work.
Hi Dinh
Please could you run this patch and let us know what it outputs.
Thanks
Andrew
drivers/net/phy/micrel.c +6 -2
index cf6312fafea5..d7ddc0bb0e7f 100644
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/phy.h>
 #include <linux/micrel_phy.h>
+#include <linux/netdevice.h>
 #include <linux/of.h>
 #include <linux/clk.h>
 
@@ -339,9 +340,19 @@ static int ksz9021_config_init(struct phy_device *phydev)
 {
 	const struct device *dev = &phydev->dev;
 	const struct device_node *of_node = dev->of_node;
+	const struct device *dev_walker;
 
-	if (!of_node && dev->parent->of_node)
-		of_node = dev->parent->of_node;
+	dev_info(dev, "dev->parent: %s\n", dev_name(dev->parent));
+	dev_info(dev, "phydev->attached_dev->dev: %s\n", dev_name(&phydev->attached_dev->dev));
+
+	dev_walker = &phydev->dev;

drivers/net/phy/micrel.c static int ksz9021_config_init(struct phy_device *phydev) · line 358

Quoted 10 lines

drivers/net/phy/micrel.c +7 -0

+	do {
+		of_node = dev_walker->of_node;
+		dev_info(dev, "walking: %s %p\n",
+			 dev_name(dev_walker), of_node);
+		dev_walker = dev_walker->parent;
+
+	} while (!of_node && dev_walker);
 
 	if (of_node) {
 		ksz9021_load_values_from_of(phydev, of_node,

Here is the output from the above patch:

[    1.042049] mmc0: new high speed SDHC card at address aaaa
[    1.048017] mmcblk0: mmc0:aaaa SU32G 29.7 GiB
[    1.053506]  mmcblk0: p1 p2 p3 p4
[    1.057708] dwc2 ffb40000.usb: Configuration mismatch. Forcing host mode
[    1.064418] dwc2 ffb40000.usb: no platform data or transceiver defined
[    1.070966] Micrel KSZ9021 Gigabit PHY stmmac-0:04: dev->parent: stmmac-0
[    1.077746] Micrel KSZ9021 Gigabit PHY stmmac-0:04: phydev->attached_dev->dev: eth0
[    1.085389] Micrel KSZ9021 Gigabit PHY stmmac-0:04: walking: stmmac-0:04   (null)
[    1.092841] Micrel KSZ9021 Gigabit PHY stmmac-0:04: walking: stmmac-0   (null)
[    1.100042] Micrel KSZ9021 Gigabit PHY stmmac-0:04: walking: ff702000.ethernet ef9f3538
[    1.133638] Sending DHCP requests ..
[    5.104138] socfpga-dwmac ff702000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx

BR,
Dinh

Andrew Lunn

maintainer

2015-12-04 14:31 UTC #28 lore raw

Quoted 26 lines

Quoted 13 lines

drivers/net/phy/micrel.c +5 -2

@@ -339,9 +340,19 @@ static int ksz9021_config_init(struct phy_device *phydev)
 {
 	const struct device *dev = &phydev->dev;
 	const struct device_node *of_node = dev->of_node;
+	const struct device *dev_walker;
 
-	if (!of_node && dev->parent->of_node)
-		of_node = dev->parent->of_node;
+	dev_info(dev, "dev->parent: %s\n", dev_name(dev->parent));
+	dev_info(dev, "phydev->attached_dev->dev: %s\n", dev_name(&phydev->attached_dev->dev));
+
+	dev_walker = &phydev->dev;

drivers/net/phy/micrel.c static int ksz9021_config_init(struct phy_device *phydev) · line 358

Quoted 10 lines

drivers/net/phy/micrel.c +7 -0

+	do {
+		of_node = dev_walker->of_node;
+		dev_info(dev, "walking: %s %p\n",
+			 dev_name(dev_walker), of_node);
+		dev_walker = dev_walker->parent;
+
+	} while (!of_node && dev_walker);
 
 	if (of_node) {
 		ksz9021_load_values_from_of(phydev, of_node,

Here is the output from the above patch:

Quoted 10 lines

[    1.042049] mmc0: new high speed SDHC card at address aaaa
[    1.048017] mmcblk0: mmc0:aaaa SU32G 29.7 GiB
[    1.053506]  mmcblk0: p1 p2 p3 p4
[    1.057708] dwc2 ffb40000.usb: Configuration mismatch. Forcing host mode
[    1.064418] dwc2 ffb40000.usb: no platform data or transceiver defined
[    1.070966] Micrel KSZ9021 Gigabit PHY stmmac-0:04: dev->parent: stmmac-0
[    1.077746] Micrel KSZ9021 Gigabit PHY stmmac-0:04: phydev->attached_dev->dev: eth0
[    1.085389] Micrel KSZ9021 Gigabit PHY stmmac-0:04: walking: stmmac-0:04   (null)
[    1.092841] Micrel KSZ9021 Gigabit PHY stmmac-0:04: walking: stmmac-0   (null)
[    1.100042] Micrel KSZ9021 Gigabit PHY stmmac-0:04: walking: ff702000.ethernet ef9f3538

Ah! So we have an intermediary device in the
chain. phydev->attached_dev->dev points to this intermediate device, rather than the platform device, which is why my patch failed.

I don't know the network stack well enough. Is this consider broken? Is this valid? Is there any generic code which might try looking in netdev->dev.of_node? Do other MAC drivers have an intermediate device?

We could modify the stmac driver so that phydev->attached_dev->dev does point to the platform device, and then my patch works. But if there are other MAC drivers which are structured like this, they are also broken when used with the Micrel PHY. So then walking up the device tree is a better solution.

Comments from more knowledgeable people requested!

Andrew

Pavel Machek

2015-12-04 09:38 UTC #29 lore raw

In reply to David Daney

Quoted 11 lines

Quoted 5 lines
While updating DTS might be good idea, I don't think you can simply blame this on DTS. If it worked before the change, it is supposed to work after the change, otherwise we call that change a "regression" and revert the change.
FWIW: My initial patch to address the failure worked with the original DTB.
Also: userspace wasn't broken. So, the commandment about not breaking userspace wasn't broken. Although admittedly, breaking the kernel isn't good either.

You can't break neither kernel nor userspace.

Quoted 7 lines

Quoted 2 lines
Plus, DTS is supposed to be ABI. Old DTS should still work on new kernels in ideal world.
If you supply the device tree file in the kernel tree, it is not an ABI.
If the device tree is not part of the kernel, and instead comes from the boot firmware of the board, then you could make the ABI claim.

It is an ABI if it was declared so, and it was. Yes, it _can_ come from kernel tree. That does not mean it has to.

Pavel

Signature · 2 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html