Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support

[Russell King - ARM Linux admin]

On Wed, Apr 01, 2020 at 02:01:25PM +0000, Florinel Iordache wrote:
> > On Thu, Mar 26, 2020 at 03:51:19PM +0200, Florinel Iordache wrote:
> > > Add support for backplane kr generic driver including link training
> > > (ieee802.3ap/ba) and fixed equalization algorithm
> > >
> > > Signed-off-by: Florinel Iordache <florinel.iordache@xxxxxxx>
> > > +/* Read AN Link Status */
> > > +static int is_an_link_up(struct phy_device *bpphy) {
> > > +     struct backplane_phy_info *bp_phy = bpphy->priv;
> > > +     int ret, val = 0;
> > > +
> > > +     mutex_lock(&bp_phy->bpphy_lock);
> > > +
> > > +     /* Read twice because Link_Status is LL (Latched Low) bit */
> > > +     val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy-
> > >bp_dev.mdio.an_status);
> > > +     val = phy_read_mmd(bpphy, MDIO_MMD_AN,
> > > + bp_phy->bp_dev.mdio.an_status);
> > 
> > Why not just
> > 
> > val = phy_read_mmd(bpphy, MDIO_MMD_AN, MDIO_CTRL1);
> > 
> > Or is your hardware not actually conformant to the standard?
> > 
> > There has also been a lot of discussion of reading the status twice is correct or
> > not. Don't you care the link has briefly gone down and up again?
> > 
> >         Andrew
> 
> This could be changed to use directly the MDIO_STAT1 in order to read
> AN status (and use MDIO_CTRL1 for writing the control register) but this
> is more flexible and more readable since we defined the structure
> kr_mdio_info that contains all registers offsets required by backplane
> driver like: LT(link training) registers, AN registers, PMD registers.
> So we wanted to put all these together to be clear that all these
> offsets are essential for backplane driver and they can be setup
> automatically by calling the function: backplane_setup_mdio_c45.
> 
> + void backplane_setup_mdio_c45(struct backplane_kr_info *bpkr)
> + /* KX/KR AN registers: IEEE802.3 Clause 45 (MMD 7) */
> + bpkr->mdio.an_control = MDIO_CTRL1;
> + bpkr->mdio.an_status = MDIO_STAT1;
> + bpkr->mdio.an_ad_ability_0 = MDIO_PMA_EXTABLE_10GBKR;
> + bpkr->mdio.an_ad_ability_1 = MDIO_PMA_EXTABLE_10GBKR + 1;
> + bpkr->mdio.an_lp_base_page_ability_1 = MDIO_PMA_EXTABLE_10GBKR + 4;

Where they are IEEE 802.3 standard registers, just use the standard
definitions, do not indirect.

> This approach is more flexible because it lets open the possibility for
> extension on other non-standard devices (devices non-compliant with
> clause 45) to still use this driver for backplane operation.

That's an entirely false argument.  If something is going to be
non-standard, why do you think that the only thing they'll do is
have non-standard register offsets?  Wouldn't they also have
non-standard register contents as well - and if they do, your
"flexible" model will no longer work there.

This seems to me to be a classic case of over-design.

We have seem some PHYs with multiple different PHY blocks within the
clause 45 space, but these are merely at offsets and follow the
standard IEEE 802.3 register sets at various offsets.  The minimum
that would be required in that case would be to carry a single register
offset - but there is no point until we encounter a PHY that actually
requires that for this support.

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