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

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Fri, Mar 27, 2020 at 03:00:22PM +0000, Florinel Iordache wrote:
> > On Thu, Mar 26, 2020 at 03:51:15PM +0200, Florinel Iordache wrote:
> > > Add ethernet backplane device tree bindings
> > 
> > > +  - |
> > > +    /* 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 effectivel ybecomes a slave to the
copper PHY.)

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 - 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.

> 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
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up