Subject: Re: [PATCH net-next 2/9] dt-bindings: net: add backplane dt
bindings

On Fri, Mar 27, 2020 at 03:00:22PM +0000, Florinel Iordache wrote:

quoted

quoted

On Thu, Mar 26, 2020 at 03:51:15PM +0200, Florinel Iordache wrote:

quoted

Add ethernet backplane device tree bindings

quoted

+  - |
+    /* Backplane configurations for specific setup */
+    &mdio9 {
+        bpphy6: ethernet-phy@0 {
+            compatible = "ethernet-phy-ieee802.3-c45";
+            reg = <0x0>;
+            lane-handle = <&lane_d>; /* use lane D */
+            eq-algorithm = "bee";
+            /* 10G Short cables setup: up to 30 cm cable */
+            eq-init = <0x2 0x5 0x29>;
+            eq-params = <0>;
+        };
+    };

So you are modelling this as just another PHY? Does the driver get
loaded based on the PHY ID in registers 2 and 3? Does the standard
define these IDs or are they vendor specific?

Thanks
        Andrew

Hi Andrew,
Thank you all for the feedback.
I am currently working to address the entire feedback received so far
for this new Backplane driver.

Yes, we are modelling backplane driver as a phy driver.

I think we need to think very carefully about that, and consider whether that
really is a good idea.  phylib is currently built primarily around copper PHYs,
although there are some which also support fiber as well in weird "non-
standard" forms.

What worries me is the situation which I've been working on, where we want
access to the PCS PHYs, and we can't have the PCS PHYs represented as a phylib
PHY because we may have a copper PHY behind the PCS PHY, and we want to be
talking to the copper PHY in the first instance (the PCS PHY effectively becomes
a slave to the copper PHY.)

We should think about the case when the PCS is the only transceiver on the local
board, as is happening in the backplane case, and the Ethernet driver does not
support phylink but rather phylib. By suggesting to not register the PCS with
phylib, you're effectively implying that the interface should operate as a fixed-link.
This PCS is shared for DPAA1 and DPAA2, and only one of those drivers uses phylink.

My worry is that we're ending up with conflicting implementations for the same
hardware which may only end up causing problems down the line.

Please can you look at my DPAA2 PCS series which has been previously posted to
netdev -

I had a go today with your DPAA2 PCS patches and tried to see how one could
extend your approach in order to use it in combination with quad PCSs.

As I mentioned yesterday, in case of QSGMII all the 4 PCSs sit on the first MAC's
internal MDIO. This leads to an error in probing the second MAC from the group
of 4 since the mdio_device_register() will fail when trying with the same internal
MDIO bus the second time.

I cannot see how this limitation can be overcome going forward if we still pass the
entire internal MDIO bus as a handle, as you are doing, and not just the specific
PCS node as the current patch set is proposing.

it's rather difficult to work out who in NXP should be copied, because
that information is not visible to those of us in the community - we only find that
out after someone inside NXP posts patches, and even then the MAINTAINERS
file doesn't seem to get updated.

It's also worth mentioning that on other SoCs, such as Marvell SoCs, the serdes
and "PCS" are entirely separate hardware blocks, and the implementation in the
kernel, which works very well, is to use the drivers/phy for the serdes/comphy as
they call it, and the ethernet driver binds to the comphy to control the serdes
settings, whereas the ethernet driver looks after the PCS.  I haven't been able to
look at your code enough yet to work out if that would be possible.

I also wonder whether we want a separate class of MDIO device for PCS PHYs,
just as we have things like DSA switches implemented entirely separately from
phylib - they're basically different sub- classes of a mdio device.

I think we have around 20 or so weeks to hash this out, since it's clear that the
10gbase-kr (10GKR) phy interface mode can't be used until we've eliminated it
from existing dts files.

quoted

The driver is loaded based on PHY ID in registers 2 and 3 which are
specified by the standard but it is a vendor specific value:
32-Bit identifier composed of the 3rd through 24th bits of the
Organizationally Unique Identifier (OUI) assigned to the device
manufacturer by the IEEE, plus a six-bit model number, plus a four-bit
revision number.
This is done in the device specific code and not in backplane generic
driver.
You can check support for QorIQ devices where qoriq_backplane_driver
is registered as a phy_driver:

@file: qoriq_backplane.c
+static struct phy_driver qoriq_backplane_driver[] = {
+	{
+	.phy_id		= PCS_PHY_DEVICE_ID,
+	.name		= QORIQ_BACKPLANE_DRIVER_NAME,
+	.phy_id_mask	= PCS_PHY_DEVICE_ID_MASK,
+	.features       = BACKPLANE_FEATURES,
+	.probe          = qoriq_backplane_probe,
+	.remove         = backplane_remove,
+	.config_init    = qoriq_backplane_config_init,
+	.aneg_done      = backplane_aneg_done,

Here we register the particular phy device ID/mask and driver name
specific for qoriq devices.
Also we can use generic routines provided by generic backplane driver
if they are suitable for particular qoriq device or otherwise we can
use more specialized specific routines like:
qoriq_backplane_config_init

--