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

[Florinel Iordache <florinel.iordache@nxp.com>]

> 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@nxp.com>
> > +/* 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;

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.
These non-standard devices will have just to define their particular
registers offsets in structure kr_mdio_info and then the rest of the driver
can be used without other modifications.

Florin.