Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 26 Aug 2022 12:43:33 +0200
From:   Oleksij Rempel <o.rempel@pengutronix.de>
To:     Michael Walle <michael@walle.cc>
Cc:     Divya.Koppera@microchip.com, UNGLinuxDriver@microchip.com,
        andrew@lunn.ch, davem@davemloft.net, edumazet@google.com,
        hkallweit1@gmail.com, kuba@kernel.org,
        linux-kernel@vger.kernel.org, linux@armlinux.org.uk,
        netdev@vger.kernel.org, pabeni@redhat.com
Subject: Re: [PATCH net-next] net: phy: micrel: Adding SQI support for
 lan8814 phy
Message-ID: <20220826104333.GF2116@pengutronix.de>
References: <20220825080549.9444-1-Divya.Koppera@microchip.com>
 <20220826084249.1031557-1-michael@walle.cc>
 <CO1PR11MB477162C762EF35B0E115B952E2759@CO1PR11MB4771.namprd11.prod.outlook.com>
 <421712ea840fbe5edffcae4a6cb08150@walle.cc>
 <20220826095429.GE2116@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220826095429.GE2116@pengutronix.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Fri, Aug 26, 2022 at 11:54:29AM +0200, Oleksij Rempel wrote:
> Hi,
> 
> On Fri, Aug 26, 2022 at 11:26:12AM +0200, Michael Walle wrote:
> > [+ Oleksij Rempel]
> > 
> > Hi,
> > 
> > Am 2022-08-26 11:11, schrieb Divya.Koppera@microchip.com:
> > > > > Supports SQI(Signal Quality Index) for lan8814 phy, where it has SQI
> > > > > index of 0-7 values and this indicator can be used for cable integrity
> > > > > diagnostic and investigating other noise sources.
> > > > >
> > > > > Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
> > 
> > ..
> > 
> > > > > +#define LAN8814_DCQ_CTRL_CHANNEL_MASK                        GENMASK(1,
> > > > 0)
> > > > > +#define LAN8814_DCQ_SQI                                      0xe4
> > > > > +#define LAN8814_DCQ_SQI_MAX                          7
> > > > > +#define LAN8814_DCQ_SQI_VAL_MASK                     GENMASK(3, 1)
> > > > > +
> > > > >  static int lanphy_read_page_reg(struct phy_device *phydev, int page,
> > > > > u32 addr)  {
> > > > >       int data;
> > > > > @@ -2927,6 +2934,32 @@ static int lan8814_probe(struct phy_device
> > > > *phydev)
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > +static int lan8814_get_sqi(struct phy_device *phydev) {
> > > > > +     int rc, val;
> > > > > +
> > > > > +     val = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_CTRL);
> > > > > +     if (val < 0)
> > > > > +             return val;
> > > > > +
> > > > > +     val &= ~LAN8814_DCQ_CTRL_CHANNEL_MASK;
> > > > 
> > > > I do have a datasheet for this PHY, but it doesn't mention 0xe6 on
> > > > EP1.
> > > 
> > > This register values are present in GPHY hard macro as below
> > > 
> > > 4.2.225	DCQ Control Register
> > > Index (In Decimal):	EP 1.230	Size:	16 bits
> > > 
> > > Can you give me the name of the datasheet which you are following, so
> > > that I'll check and let you know the reason.
> > 
> > I have the AN4286/DS00004286A ("LAN8804/LAN8814 GPHY Register
> > Definitions"). Maybe there is a newer version of it.
> > 
> > > 
> > > > So I can only guess that this "channel mask" is for the 4 rx/tx
> > > > pairs on GbE?
> > > 
> > > Yes channel mask is for wire pair.
> > > 
> > > > And you only seem to evaluate one of them. Is that the correct thing
> > > > to do
> > > > here?
> > > > 
> > > 
> > > I found in below link is that, get_SQI returns sqi value for 100 base-t1
> > > phy's
> > > https://lore.kernel.org/netdev/20200519075200.24631-2-o.rempel@pengutronix.de/T/
> > 
> > That one is for the 100base-t1 which has only one pair.
> > 
> > > In lan8814 phy only channel 0 is used for 100base-tx. So returning SQI
> > > value for channel 0.
> > 
> > What if the other pairs are bad? Maybe Oleksij has an opinion here.
> > 
> > Also 100baseTX (and 10baseT) has two pairs, one for transmitting and one
> > for receiving. I guess you meassure the SQI on the receiving side. So is
> > channel 0 correct here?
> > 
> > Again this is the first time I hear about SQI but it puzzles me that
> > it only evaluate one pair in this case. So as a user who reads this
> > SQI might be misleaded.
> 
> Wow! I was so possessed with one-pair networks, that forgot to image
> that there is 1000Base-T with more then one pairs :D
> 
> Yes, your are right. We wont to have readings from all RX channels and
> be able to export them to the user space. In fact, if i see it
> correctly, the LAN8814_DCQ_CTRL_CHANNEL_MASK value should be synced with
> the MDI-X state. Otherwise we will be reading TX channels.

Just an idea not really related to this patch. It will be coll to be
able to generate diagnostic graphs like this:

Interface | pairs | data      | power | SQI | cable | selft
                  | direction |       |     | test  | test
eth0                                                  ok
            0     | RX        | NC    | 7/7 | ok    |
	    1     | TX        | NC    | NA  | ok    |
	    2     | NC        | + 10W | NA  | ok    |
	    3     | NC        | -     | NA  | ok    |

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |