mvneta: comphy regression with SolidRun ClearFog

mvneta: comphy regression with SolidRun ClearFog

[Joel Johnson]

In updating recently I'm encountering a regression with the mvneta 
driver on SolidRun ClearFog Base devices. I originally filed the bug 
with Debian (https://bugs.debian.org/951409) since I was using distro 
provided packages, but after further investigation I have isolated the 
issue as related to comphy support added during development for kernel 
version 5.1.

When booting stock kernels up to 5.0 everything works as expected with 
three ethernet devices identified and functional. However, running any 
kernel 5.1 or later, I only have a single ethernet device available. The 
single device available appears to be the one attached to the SoC itself 
and not connected via SerDes lanes using comphy, i.e. the one defined at 
f1070000.ethernet.

With some log/diff assisted bisecting, I've been able to confirm that 
the "tipping point" changeset is f548ced15f90, which actually performs 
the DT change for the ClearFog family of devices. That's the commit at 
which the failure starts, but is just the final enablement of the added 
feature in the overall series. I've also tested booting the same kernel 
binary (including up to v5.6-rc1) and only swapping the dtb for one 
excluding the problematic commit and confirmed that simply changing the 
dtb results in all three devices being functional, albeit obviously 
without comphy support.

The original patch series applied (as best I can tell) is below, merged 
in commit a4751093a26c.

     
https://lore.kernel.org/netdev/20190207161825.ueinmyf6ygjiqzzy@shell.armlinux.org.uk

I'm not overly Device Tree savvy, but a cursory inspection of 
f548ced15f90 at least matches my U-Boot SerDes lane configuration, with 
comphy1 and comphy5 expected to match lane #1 and #5 respectively.

I have been running a locally patched pre-upstream merged v2020.04-rc2 
U-Boot version, but I also tested with a clean build of upstream U-Boot 
v2020.01 as well as the latest SolidRun vendor build [1], with all 
configurations yielding the same result. I happen to be booting off of 
internal SPI, but I doubt that is related. I'm also running both 
ethernet SGMII SerDes lanes at the default 1.25Gbps speed as configured 
in U-Boot.

The only notable difference I can see in /sys/firmware/devicetree is 
expected given the change in dtb, with the following new entries:

     hexdump -C 
/sys/firmware/devicetree/base/soc/internal-regs/ethernet@30000/phys
     00000000  00 00 00 0e 00 00 00 01                           
|........|

     hexdump -C 
/sys/firmware/devicetree/base/soc/internal-regs/ethernet@34000/phys
     00000000  00 00 00 10 00 00 00 02                           
|........|

Likely unrelated, but a difference that also stood out is that 
armada-388-clearfog.dts contains a managed = "in-band-status" entry for 
eth1 but not eth2.

I plan on filing a kernel bugzilla issue (once I get my password reset). 
In the meantime, any further guidance on testing or troubleshooting 
would be greatly appreciated. I'd be happy to provide further details or 
test builds with patches, but as mentioned above am not very savvy with 
Device Tree handling functionality. In the meantime I'll add some trace 
logging messages in the comphy handling changes to dig further, but if 
there's something obvious I'd love to avoid it if it's just busy work.

Thanks,
Joel

[1] 
https://images.solid-build.xyz/A38X/U-Boot/u-boot-clearfog-base-spi.kwb

Re: mvneta: comphy regression with SolidRun ClearFog

[Willy Tarreau]

Hi Joel,

On Tue, Feb 18, 2020 at 10:14:48PM -0700, Joel Johnson wrote:
> In updating recently I'm encountering a regression with the mvneta driver on
> SolidRun ClearFog Base devices. I originally filed the bug with Debian
> (https://bugs.debian.org/951409) since I was using distro provided packages,
> but after further investigation I have isolated the issue as related to
> comphy support added during development for kernel version 5.1.
> 
> When booting stock kernels up to 5.0 everything works as expected with three
> ethernet devices identified and functional. However, running any kernel 5.1
> or later, I only have a single ethernet device available. The single device
> available appears to be the one attached to the SoC itself and not connected
> via SerDes lanes using comphy, i.e. the one defined at f1070000.ethernet.

When you say "or later", what most recent version did you try ? My
clearfog works perfectly on 5.4 with the new comphy. I'm having the 2
RJ45 ports working at 1 Gbps and the SFP port working at 1 and 2.5 Gbps.

> I'm not overly Device Tree savvy, but a cursory inspection of f548ced15f90
> at least matches my U-Boot SerDes lane configuration, with comphy1 and
> comphy5 expected to match lane #1 and #5 respectively.

I used to have to modify the device tree in the past, but haven't been
doing so for a while (well in fact I do have a small change there just
in order to enable eMMC which I have on my SOM, and I have just rechecked
that *only* the emmc stuff differs from the regular clearfog-base).

> The only notable difference I can see in /sys/firmware/devicetree is
> expected given the change in dtb, with the following new entries:
> 
>     hexdump -C
> /sys/firmware/devicetree/base/soc/internal-regs/ethernet@30000/phys
>     00000000  00 00 00 0e 00 00 00 01                           |........|
> 
>     hexdump -C
> /sys/firmware/devicetree/base/soc/internal-regs/ethernet@34000/phys
>     00000000  00 00 00 10 00 00 00 02                           |........|

I've just checked and have exactly the same values there.

> Likely unrelated, but a difference that also stood out is that
> armada-388-clearfog.dts contains a managed = "in-band-status" entry for eth1
> but not eth2.

If I remember well it's because with this port being attached to the
switch on the clearfog pro, there's no link status.

I used to have issues in the past with the PHY stuff on this board (up
to 4.9), and *seem* to remember that I once ended up in a similar
situation as yours due to a config issue, though I don't remmeber which
one. Here's what I have matching PHY in my config:

   root@clearfog:~# zgrep ^CONFIG.*PHY /proc/config.gz 
   CONFIG_ARM_PATCH_PHYS_VIRT=y
   CONFIG_ARCH_HAS_PHYS_TO_DMA=y
   CONFIG_NETFILTER_XT_MATCH_PHYSDEV=m
   CONFIG_PHYLINK=y
   CONFIG_PHYLIB=y
   CONFIG_SWPHY=y
   CONFIG_FIXED_PHY=y
   CONFIG_MARVELL_PHY=y
   CONFIG_GENERIC_PHY=y
   CONFIG_PHY_MVEBU_A38X_COMPHY=y
   CONFIG_DEBUG_UART_PHYS=0xf1012000
   root@clearfog:~# uname -a
   Linux clearfog 5.4.2-clearfog #10 SMP Sun Dec 8 00:10:40 CET 2019 armv7l GNU/Linux

I'm suspecting it was the FIXED_PHY that I was missing once but I would
be saying crap.

Hoping this helps,
Willy

Re: mvneta: comphy regression with SolidRun ClearFog

[Russell King - ARM Linux admin]

On Tue, Feb 18, 2020 at 10:14:48PM -0700, Joel Johnson wrote:
> In updating recently I'm encountering a regression with the mvneta driver on
> SolidRun ClearFog Base devices. I originally filed the bug with Debian
> (https://bugs.debian.org/951409) since I was using distro provided packages,
> but after further investigation I have isolated the issue as related to
> comphy support added during development for kernel version 5.1.
> 
> When booting stock kernels up to 5.0 everything works as expected with three
> ethernet devices identified and functional. However, running any kernel 5.1
> or later, I only have a single ethernet device available. The single device
> available appears to be the one attached to the SoC itself and not connected
> via SerDes lanes using comphy, i.e. the one defined at f1070000.ethernet.
> 
> With some log/diff assisted bisecting, I've been able to confirm that the
> "tipping point" changeset is f548ced15f90, which actually performs the DT
> change for the ClearFog family of devices. That's the commit at which the
> failure starts, but is just the final enablement of the added feature in the
> overall series. I've also tested booting the same kernel binary (including
> up to v5.6-rc1) and only swapping the dtb for one excluding the problematic
> commit and confirmed that simply changing the dtb results in all three
> devices being functional, albeit obviously without comphy support.

Does debian have support for the comphy enabled in their kernel,
which is controlled by CONFIG_PHY_MVEBU_A38X_COMPHY ?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: mvneta: comphy regression with SolidRun ClearFog

[Joel Johnson]

On 2020-02-19 02:22, Russell King - ARM Linux admin wrote:
> On Tue, Feb 18, 2020 at 10:14:48PM -0700, Joel Johnson wrote:
>> In updating recently I'm encountering a regression with the mvneta 
>> driver on
>> SolidRun ClearFog Base devices. I originally filed the bug with Debian
>> (https://bugs.debian.org/951409) since I was using distro provided 
>> packages,
>> but after further investigation I have isolated the issue as related 
>> to
>> comphy support added during development for kernel version 5.1.
>> 
>> When booting stock kernels up to 5.0 everything works as expected with 
>> three
>> ethernet devices identified and functional. However, running any 
>> kernel 5.1
>> or later, I only have a single ethernet device available. The single 
>> device
>> available appears to be the one attached to the SoC itself and not 
>> connected
>> via SerDes lanes using comphy, i.e. the one defined at 
>> f1070000.ethernet.
>> 
>> With some log/diff assisted bisecting, I've been able to confirm that 
>> the
>> "tipping point" changeset is f548ced15f90, which actually performs the 
>> DT
>> change for the ClearFog family of devices. That's the commit at which 
>> the
>> failure starts, but is just the final enablement of the added feature 
>> in the
>> overall series. I've also tested booting the same kernel binary 
>> (including
>> up to v5.6-rc1) and only swapping the dtb for one excluding the 
>> problematic
>> commit and confirmed that simply changing the dtb results in all three
>> devices being functional, albeit obviously without comphy support.
> 
> Does debian have support for the comphy enabled in their kernel,
> which is controlled by CONFIG_PHY_MVEBU_A38X_COMPHY ?

Well, doh! I stared at the patch series for way to long, but the added 
Kconfig symbol failed to register mentally somehow. I had been using the 
last known good Debian config with make olddefconfig, but it obviously 
wasn't included in earlier configs and not enabled by default.

I tested a build with v5.6-rc1 and actually enabled the platform driver 
and it worked as expected, including log output of "configuring for 
sgmii link mode". Back to moving forward on other testing. Sorry for the 
noise, I'll update the Debian bug with a patch to enable the config 
symbol for armmp kernels.

Many thanks to you and Willy Tarreau for pointing out my glaring 
omission!

Joel

Re: mvneta: comphy regression with SolidRun ClearFog

[Russell King - ARM Linux admin]

On Wed, Feb 19, 2020 at 06:49:51AM -0700, Joel Johnson wrote:
> On 2020-02-19 02:22, Russell King - ARM Linux admin wrote:
> > On Tue, Feb 18, 2020 at 10:14:48PM -0700, Joel Johnson wrote:
> > > In updating recently I'm encountering a regression with the mvneta
> > > driver on
> > > SolidRun ClearFog Base devices. I originally filed the bug with Debian
> > > (https://bugs.debian.org/951409) since I was using distro provided
> > > packages,
> > > but after further investigation I have isolated the issue as related
> > > to
> > > comphy support added during development for kernel version 5.1.
> > > 
> > > When booting stock kernels up to 5.0 everything works as expected
> > > with three
> > > ethernet devices identified and functional. However, running any
> > > kernel 5.1
> > > or later, I only have a single ethernet device available. The single
> > > device
> > > available appears to be the one attached to the SoC itself and not
> > > connected
> > > via SerDes lanes using comphy, i.e. the one defined at
> > > f1070000.ethernet.
> > > 
> > > With some log/diff assisted bisecting, I've been able to confirm
> > > that the
> > > "tipping point" changeset is f548ced15f90, which actually performs
> > > the DT
> > > change for the ClearFog family of devices. That's the commit at
> > > which the
> > > failure starts, but is just the final enablement of the added
> > > feature in the
> > > overall series. I've also tested booting the same kernel binary
> > > (including
> > > up to v5.6-rc1) and only swapping the dtb for one excluding the
> > > problematic
> > > commit and confirmed that simply changing the dtb results in all three
> > > devices being functional, albeit obviously without comphy support.
> > 
> > Does debian have support for the comphy enabled in their kernel,
> > which is controlled by CONFIG_PHY_MVEBU_A38X_COMPHY ?
> 
> Well, doh! I stared at the patch series for way to long, but the added
> Kconfig symbol failed to register mentally somehow. I had been using the
> last known good Debian config with make olddefconfig, but it obviously
> wasn't included in earlier configs and not enabled by default.
> 
> I tested a build with v5.6-rc1 and actually enabled the platform driver and
> it worked as expected, including log output of "configuring for sgmii link
> mode". Back to moving forward on other testing. Sorry for the noise, I'll
> update the Debian bug with a patch to enable the config symbol for armmp
> kernels.
> 
> Many thanks to you and Willy Tarreau for pointing out my glaring omission!

Thanks for letting us know that you've fixed it now.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: mvneta: comphy regression with SolidRun ClearFog

[Joel Johnson]

On 2020-02-20 03:12, Russell King - ARM Linux admin wrote:
> On Wed, Feb 19, 2020 at 06:49:51AM -0700, Joel Johnson wrote:
>> On 2020-02-19 02:22, Russell King - ARM Linux admin wrote:
>> > On Tue, Feb 18, 2020 at 10:14:48PM -0700, Joel Johnson wrote:
>> > Does debian have support for the comphy enabled in their kernel,
>> > which is controlled by CONFIG_PHY_MVEBU_A38X_COMPHY ?
>> 
>> Well, doh! I stared at the patch series for way to long, but the added
>> Kconfig symbol failed to register mentally somehow. I had been using 
>> the
>> last known good Debian config with make olddefconfig, but it obviously
>> wasn't included in earlier configs and not enabled by default.
>> 
>> Many thanks to you and Willy Tarreau for pointing out my glaring 
>> omission!
> 
> Thanks for letting us know that you've fixed it now.

Sure thing, I've submitted a Debian patch, and was pointed to an 
existing Debian bug with the same issue and patch, so hopefully that 
will get incorporated soon. I'll also keep an eye on OpenWRT when they 
move to an affected kernel version to make sure it's included.

One lingering question that wasn't clear to me is the apparent 
inconsistency in default enablement for PHYs in 
drivers/phy/marvell/Kconfig. Is there a technical reason why 
PHY_MVEBU_A3700_COMPHY defaults to 'y' but PHY_MVEBU_A38X_COMPHY (and 
PHY_MVEBU_CP110_COMPHY) default to 'n', or is it just an artifact of 
being added at different times? Similarly, is there a reason that 
PHY_MVEBU_A3700_COMPHY and PHY_MVEBU_A3700_UTMI default to 'y' instead 
of 'm' for all ARCH_MVEBU builds? In my testing, building with 
PHY_MVEBU_A38X_COMPHY as a module still seemed to autoload the module as 
needed on boot, so modules for different platforms seems off-hand more 
lightweight that building the driver in for all MVEBU boards which don't 
use all drivers.

With the current defaults, it seems like PHY_MVEBU_CP110_COMPHY may be 
affected in Debian the same way as PHY_MVEBU_A38X_COMPHY, but I don't 
have available Armada 7K/8K hardware yet to confirm.

Joel

Re: mvneta: comphy regression with SolidRun ClearFog

[Russell King - ARM Linux admin]

On Thu, Feb 20, 2020 at 08:34:07AM -0700, Joel Johnson wrote:
> On 2020-02-20 03:12, Russell King - ARM Linux admin wrote:
> > On Wed, Feb 19, 2020 at 06:49:51AM -0700, Joel Johnson wrote:
> > > On 2020-02-19 02:22, Russell King - ARM Linux admin wrote:
> > > > On Tue, Feb 18, 2020 at 10:14:48PM -0700, Joel Johnson wrote:
> > > > Does debian have support for the comphy enabled in their kernel,
> > > > which is controlled by CONFIG_PHY_MVEBU_A38X_COMPHY ?
> > > 
> > > Well, doh! I stared at the patch series for way to long, but the added
> > > Kconfig symbol failed to register mentally somehow. I had been using
> > > the
> > > last known good Debian config with make olddefconfig, but it obviously
> > > wasn't included in earlier configs and not enabled by default.
> > > 
> > > Many thanks to you and Willy Tarreau for pointing out my glaring
> > > omission!
> > 
> > Thanks for letting us know that you've fixed it now.
> 
> Sure thing, I've submitted a Debian patch, and was pointed to an existing
> Debian bug with the same issue and patch, so hopefully that will get
> incorporated soon. I'll also keep an eye on OpenWRT when they move to an
> affected kernel version to make sure it's included.
> 
> One lingering question that wasn't clear to me is the apparent inconsistency
> in default enablement for PHYs in drivers/phy/marvell/Kconfig. Is there a
> technical reason why PHY_MVEBU_A3700_COMPHY defaults to 'y' but
> PHY_MVEBU_A38X_COMPHY (and PHY_MVEBU_CP110_COMPHY) default to 'n', or is it
> just an artifact of being added at different times? Similarly, is there a
> reason that PHY_MVEBU_A3700_COMPHY and PHY_MVEBU_A3700_UTMI default to 'y'
> instead of 'm' for all ARCH_MVEBU builds? In my testing, building with
> PHY_MVEBU_A38X_COMPHY as a module still seemed to autoload the module as
> needed on boot, so modules for different platforms seems off-hand more
> lightweight that building the driver in for all MVEBU boards which don't use
> all drivers.
> 
> With the current defaults, it seems like PHY_MVEBU_CP110_COMPHY may be
> affected in Debian the same way as PHY_MVEBU_A38X_COMPHY, but I don't have
> available Armada 7K/8K hardware yet to confirm.

There is no clear answer to whether should something default to Y,
M or N - different people have different ideas and different levels
of frustration with which-ever are picked.

The root problem is that there are way too many configuration
options that it's become quite time consuming to put together the
proper kernel configuration for any particular platform, and things
get even more interesting when it comes to a kernel supporting
multiple platforms, where one may wish to avoid having a huge
kernel image, but want to use modules for the platform specific
bits.

I wonder whether we ought to be considering something like:

menuconfig ARCH_MVEBU_DEFAULTS
	tristate "Marvell Engineering Business Unit (MVEBU) SoCs"

config ARCH_MVEBU
	def_bool y if ARCH_MVEBU_DEFAULTS
	...

and then have mvebu drivers default to the state of
ARCH_MVEBU_DEFAULTS.  That means, if you want to build the
platform with modular drivers, or built-in drivers there's one top
level config that will default all the appropriate options that way,
and any new drivers added later can pick up on the state of that
option.

Just a thought.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: mvneta: comphy regression with SolidRun ClearFog

[Willy Tarreau]

On Thu, Feb 20, 2020 at 03:45:21PM +0000, Russell King - ARM Linux admin wrote:
> > With the current defaults, it seems like PHY_MVEBU_CP110_COMPHY may be
> > affected in Debian the same way as PHY_MVEBU_A38X_COMPHY, but I don't have
> > available Armada 7K/8K hardware yet to confirm.

On this point I can confirm that my mcbin does require
CONFIG_PHY_MVEBU_CP110_COMPHY to work correctly.

> There is no clear answer to whether should something default to Y,
> M or N - different people have different ideas and different levels
> of frustration with which-ever are picked.
> 
> The root problem is that there are way too many configuration
> options that it's become quite time consuming to put together the
> proper kernel configuration for any particular platform, and things
> get even more interesting when it comes to a kernel supporting
> multiple platforms, where one may wish to avoid having a huge
> kernel image, but want to use modules for the platform specific
> bits.
> 
> I wonder whether we ought to be considering something like:
> 
> menuconfig ARCH_MVEBU_DEFAULTS
> 	tristate "Marvell Engineering Business Unit (MVEBU) SoCs"
> 
> config ARCH_MVEBU
> 	def_bool y if ARCH_MVEBU_DEFAULTS
> 	...
> 
> and then have mvebu drivers default to the state of
> ARCH_MVEBU_DEFAULTS.  That means, if you want to build the
> platform with modular drivers, or built-in drivers there's one top
> level config that will default all the appropriate options that way,
> and any new drivers added later can pick up on the state of that
> option.
> 
> Just a thought.

I found that phys are actually a very obscure area for many users, a
bit like codecs for sound, and that even when you think you know what
you're doing you can get it wrong. Part of the reason is the common
total disconnection between certain phy chips and the places they're
used (sometimes different vendors for the PHY and the MAC), and actually
a very good approach would be to clearly enumerate a list of candidate
platforms instead of families. Typically for A38X and CP110 it's clearly
mentioned in the help messages that they're for Armada 38x/7k/8k but
often it's obscure (e.g. the USB phy descriptions in the same Kconfig
are less easy to figure, such as "28nm SoC" or "Berlin SoCs").

I tend to also agree with Russell you that having an option to turn
on sane defaults for a given platform that's well maintained like
mvebu could save quite some time.

Willy