Hi Russell, On Thu, Dec 28, 2017 at 06:59:21PM +0000, Russell King - ARM Linux wrote: > On Thu, Dec 28, 2017 at 11:04:16AM +0100, Antoine Tenart wrote: > > > > That's not what I remembered. You had some valid points, and others > > related to PHY modes the driver wasn't supporting before the phylink > > transition. My understanding of this was that you wanted a full > > featured support while I only wanted to convert the already supported > > modes. > > You are mistaken - you can get a full refresher on where things were > at via https://patchwork.kernel.org/patch/9963971/
I read it again and still have the same feeling. There's been a misunderstanding at some point. Anyway, let's move forward :) > 1. I asked for details about what mvpp2.c supports that phylink does > not (as you indicated that there were certain things that mvpp2 > supports that phylink does not.) I'm still awaiting a response. I don't remember PHY modes supported in the PPv2 driver that aren't supported in PHYLINK. I think this point is the main misunderstanding. I thought you wanted me to support modes unsupported in the PPv2 driver before. But you explained quite well what these comments were about below. So I guess this point is resolved (aka I'll have to take your comments into account for the v2). > 2. 25th Sept, you indicated that you would get someone to test > an issue related to in-band AN. No results of that testing have > been forthcoming. That's right. I asked someone to make a test, but did not get an answer. And because the PHYLINK patch stalled on my side I kinda forget about it. I'll try again to have this test made. > I am not after a full featured support, what I'm after is ensuring > that phylink is (a) used correctly and (b) implementations using it > are correct. Part of that is ensuring that users don't introduce > unexpected failure conditions. > > So, when you do this in the validate() callback: > > + phylink_set(mask, 1000baseX_Full); > > and then do this in the mac_config() callback: > > + if (!phy_interface_mode_is_rgmii(port->phy_interface) && > + port->phy_interface != PHY_INTERFACE_MODE_SGMII) > + return; > > and this in the link_state() callback: > > + if (!phy_interface_mode_is_rgmii(port->phy_interface) && > + port->phy_interface != PHY_INTERFACE_MODE_SGMII) > + return 0; > > the result is that phylink thinks that you support 1000base-X modes, > and it will call mac_config() asking for 1000base-X, but you silently > ignore that, leaving the hardware configured in whatever state it was. > That leads to a silent failure as far as the user is concerned. > > So, if you do not intend to support 1000base-X initially, don't > allow it in the validate callback until you do. > > It gets worse, because the return in link_state() means that phylink > thinks that the link is up if it has requested 1000base-X, which it > won't be unless you've properly configured it. > > It's this kind of unreliability that I was concerned about in your > patch. I'm not demanding "full featured implementation" but I do > want you to use it correctly. Thanks for the detailed explanations! > > > What I'm most concerned about, given the bindings for comphy that > > > have been merged, is that Free Electrons is pushing forward seemingly > > > with no regard to the requirement that the serdes lanes are dynamically > > > reconfigurable, and that's a basic requirement for SFP, and for the > > > 88x3310 PHYs on the Macchiatobin platform. > > > > The main idea behind the comphy driver is to provide a way to > > reconfigure the serdes lanes at runtime. Could you develop what are > > blocking points to properly support SFP, regarding the current comphy > > support? > > If it supports serdes lane mode reconfiguration (iow, switching between > 1000base-X, 2500base-X, SGMII, 10G-KR), then that's all that's required. It does, and the PPv2 driver already ask the COMPHY driver to perform these reconfigurations (when using the 10G/1G interface on the mcbin for example). Thanks! Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
