Linux-Doc Archive on lore.kernel.org
 help / color / Atom feed
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
To: Florinel Iordache <florinel.iordache@nxp.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"f.fainelli@gmail.com" <f.fainelli@gmail.com>,
	"hkallweit1@gmail.com" <hkallweit1@gmail.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"corbet@lwn.net" <corbet@lwn.net>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	Leo Li <leoyang.li@nxp.com>,
	"Madalin Bucur (OSS)" <madalin.bucur@oss.nxp.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support
Date: Wed, 1 Apr 2020 15:14:48 +0100
Message-ID: <20200401141448.GU25745@shell.armlinux.org.uk> (raw)
In-Reply-To: <AM0PR04MB544326757B0B510C7C3C6417FBC90@AM0PR04MB5443.eurprd04.prod.outlook.com>

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

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

  parent reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-01 14:01 Florinel Iordache
2020-04-01 14:11 ` Andrew Lunn
2020-04-01 14:14 ` Russell King - ARM Linux admin [this message]
  -- strict thread matches above, loose matches on Subject: below --
2020-04-24 12:14 Florinel Iordache
2020-04-01 13:35 Florinel Iordache
2020-04-01 13:51 ` Andrew Lunn
2020-03-26 13:51 [PATCH net-next 0/9] net: ethernet backplane support Florinel Iordache
     [not found] ` <1585230682-24417-7-git-send-email-florinel.iordache@nxp.com>
2020-03-26 18:53   ` [PATCH net-next 6/9] net: phy: add backplane kr driver support David Miller
2020-03-26 18:55     ` Joe Perches
2020-03-26 19:07       ` David Miller
2020-03-26 19:42         ` Joe Perches
2020-03-27  1:07   ` Andrew Lunn
2020-03-27 17:43     ` Florian Fainelli
2020-03-27 14:22   ` Andrew Lunn
2020-03-27 18:25     ` Joe Perches
2020-03-27 14:28   ` Andrew Lunn
2020-03-27 14:33   ` Andrew Lunn
2020-03-27 14:38   ` Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200401141448.GU25745@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=andrew@lunn.ch \
    --cc=corbet@lwn.net \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=f.fainelli@gmail.com \
    --cc=florinel.iordache@nxp.com \
    --cc=hkallweit1@gmail.com \
    --cc=ioana.ciornei@nxp.com \
    --cc=kuba@kernel.org \
    --cc=leoyang.li@nxp.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=madalin.bucur@oss.nxp.com \
    --cc=mark.rutland@arm.com \
    --cc=netdev@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Doc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-doc/0 linux-doc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-doc linux-doc/ https://lore.kernel.org/linux-doc \
		linux-doc@vger.kernel.org
	public-inbox-index linux-doc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-doc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git