| Message ID | 20220825080549.9444-1-Divya.Koppera@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: micrel: Adding SQI support for lan8814 phy | expand |
On Thu, Aug 25, 2022 at 01:35:49PM +0530, Divya Koppera wrote: > 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. This driver supports 16 PHY devices. You only add this to one. Are you sure it does not work with others? Andrew
> +#define LAN8814_DCQ_CTRL 0xe6 > +#define LAN8814_DCQ_CTRL_READ_CAPTURE_ BIT(15) > +#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; I just took a quick look at the datasheet. It says: All registers references in this section are in MMD Device Address 1 So you should be using phy_read_mmd(phydev, MDIO_MMD_PMAPMD, xxx) to read/write these registers. The datasheet i have however is missing the register map, so i've no idea if it is still 0xe6. Andrew
Hi Andrew, > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Thursday, August 25, 2022 6:50 PM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; UNGLinuxDriver > <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH net-next] net: phy: micrel: Adding SQI support for > lan8814 phy > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Thu, Aug 25, 2022 at 01:35:49PM +0530, Divya Koppera wrote: > > 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. > > This driver supports 16 PHY devices. You only add this to one. Are you sure it > does not work with others? I don't have other hardware to test or check. I have only lan8814, where I have tested. Also the register access or register address may not be same for other phy's. The one who has ownership May provide support for other phy's. > > Andrew
Hi, > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Friday, August 26, 2022 3:24 AM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; UNGLinuxDriver > <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH net-next] net: phy: micrel: Adding SQI support for > lan8814 phy > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > > +#define LAN8814_DCQ_CTRL 0xe6 > > +#define LAN8814_DCQ_CTRL_READ_CAPTURE_ BIT(15) > > +#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; > > I just took a quick look at the datasheet. It says: > I'm not sure the datasheet you looked into is the right one. Could you please crosscheck if its lan8814 or lan8841. Lan8814 is quad port phy where register access are of extended page. Lan8841 is 1 port phy where register access are mmd access. > All registers references in this section are in MMD Device Address 1 > > So you should be using phy_read_mmd(phydev, MDIO_MMD_PMAPMD, > xxx) to read/write these registers. The datasheet i have however is missing > the register map, so i've no idea if it is still 0xe6. > I hope above explanation gives answer. > Andrew
Hi, > 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> > --- > drivers/net/phy/micrel.c | 35 +++++++++++++++++++++++++++++++++++ > 1 file changed, 35 insertions(+) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index e78d0bf69bc3..3775da7afc64 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -1975,6 +1975,13 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev, > #define LAN8814_CLOCK_MANAGEMENT 0xd > #define LAN8814_LINK_QUALITY 0x8e > > +#define LAN8814_DCQ_CTRL 0xe6 > +#define LAN8814_DCQ_CTRL_READ_CAPTURE_ BIT(15) Why does it end with an underscore? > +#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. So I can only guess that this "channel mask" is for the 4 rx/tx pairs on GbE? And you only seem to evaluate one of them. Is that the correct thing to do here? -michael > + val |= LAN8814_DCQ_CTRL_READ_CAPTURE_; > + rc = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val); > + if (rc < 0) > + return rc; > + > + rc = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI); > + if (rc < 0) > + return rc; > + > + return FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, rc); > +}
Hi Michael, > -----Original Message----- > From: Michael Walle <michael@walle.cc> > Sent: Friday, August 26, 2022 2:13 PM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: UNGLinuxDriver <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; Michael Walle > <michael@walle.cc> > Subject: Re: [PATCH net-next] net: phy: micrel: Adding SQI support for > lan8814 phy > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi, > > > 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> > > --- > > drivers/net/phy/micrel.c | 35 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 35 insertions(+) > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index > > e78d0bf69bc3..3775da7afc64 100644 > > --- a/drivers/net/phy/micrel.c > > +++ b/drivers/net/phy/micrel.c > > @@ -1975,6 +1975,13 @@ static int ksz886x_cable_test_get_status(struct > phy_device *phydev, > > #define LAN8814_CLOCK_MANAGEMENT 0xd > > #define LAN8814_LINK_QUALITY 0x8e > > > > +#define LAN8814_DCQ_CTRL 0xe6 > > +#define LAN8814_DCQ_CTRL_READ_CAPTURE_ BIT(15) > > Why does it end with an underscore? > All LAN8814 Macros that carries bit numbers are ending with _ in this driver. So following same. > > +#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. > 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/ In lan8814 phy only channel 0 is used for 100base-tx. So returning SQI value for channel 0. > -michael > > > > + val |= LAN8814_DCQ_CTRL_READ_CAPTURE_; > > + rc = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val); > > + if (rc < 0) > > + return rc; > > + > > + rc = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI); > > + if (rc < 0) > > + return rc; > > + > > + return FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, rc); }
[+ 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. -michael
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. Regards, Oleksij
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 |
> > I just took a quick look at the datasheet. It says: > > > > I'm not sure the datasheet you looked into is the right one. Could you please crosscheck if its lan8814 or lan8841. > Lan8814 is quad port phy where register access are of extended page. Lan8841 is 1 port phy where register access are mmd access. > > > All registers references in this section are in MMD Device Address 1 > > > > So you should be using phy_read_mmd(phydev, MDIO_MMD_PMAPMD, > > xxx) to read/write these registers. The datasheet i have however is missing > > the register map, so i've no idea if it is still 0xe6. https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/DS-LAN8814-00003592C.pdf 5.13.4 OPEN ALLIANCE TC1/TC12 DCQ SIGNAL QUALITY INDEX Note: All registers references in this section are in MMD Device Address 1. This section defines the implementation of section 6.1.2 of the TC1 and TC12 specifications. This mode builds upon the OPEN Alliance TC1/TC12 DCQ Mean Square Error method by mapping the MSE value onto a simple quality index. This mode is enabled by setting the sqi_enable bit, in the DCQ Configuration register. The MSE value is compared to the thresholds set in the DCQ SQI Table Registers to provide an SQI value between 0 (worst value) and 7 (best value) as follows: In order to capture the SQI value, the DCQ Read Capture bit in the DCQ Configuration register needs to be written as a high with the desired cable pair specified in the DCQ Channel Number field of the same register. The DCQ Read Capture bit will immediately self-clear and the result will be available in the DCQ SQI register. In addition to the current SQI, the worst case (lowest) SQI since the last read is available in the SQI Worst Case field. The correlation between the SQI values stored in the DCQ SQI register and an according Signal to Noise Ratio (SNR) based on Additive White Gaussian (AWG) noise (bandwidth of 80 MHz @ 100 Mbps / 550 MHz @ 1000 Mbps) is shown in Table 5-5. The bit error rates to be expected in the case of white noise as interference signal is shown in the table as well for information purposes. I had a quick look at OPEN ALLIANCE specification. It seems to specify how each of these registers should look. It just failed to specify where in the address map they are. So if you look at drivers implementing SQI, you see most poke around in MDIO_MMD_VEND1. I wounder if we can actually share the implementation between drivers, those that follow the standard, with some paramatirisation where the registers are. Andrew
> > > 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. I don't know if i should trust the datasheet i found, but it suggests the register in MMD device 1 space has a field to indicate which cable pair should be measured. So for 1000Base-T all pairs are both Rx and Tx, so i would expect 4 values are returned. That then might need an uAPI extension, if you were focused on T1 :-) Andrew
Hi Andrew, > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Saturday, August 27, 2022 1:13 AM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; UNGLinuxDriver > <UNGLinuxDriver@microchip.com> > Subject: Re: [PATCH net-next] net: phy: micrel: Adding SQI support for > lan8814 phy > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > > > I just took a quick look at the datasheet. It says: > > > > > > > I'm not sure the datasheet you looked into is the right one. Could you > please crosscheck if its lan8814 or lan8841. > > Lan8814 is quad port phy where register access are of extended page. > Lan8841 is 1 port phy where register access are mmd access. > > > > > All registers references in this section are in MMD Device Address 1 > > > > > > So you should be using phy_read_mmd(phydev, > MDIO_MMD_PMAPMD, > > > xxx) to read/write these registers. The datasheet i have however is > > > missing the register map, so i've no idea if it is still 0xe6. > > https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ > ProductDocuments/DataSheets/DS-LAN8814-00003592C.pdf > > 5.13.4 OPEN ALLIANCE TC1/TC12 DCQ SIGNAL QUALITY INDEX > > Note: All registers references in this section are in MMD Device Address 1. > Ah.. I too looked into it. Its a mistake. I suggested for correction of document internally. Also requested team to add SQI register set in register document that is published. > This section defines the implementation of section 6.1.2 of the TC1 and TC12 > specifications. This mode builds upon the OPEN Alliance > TC1/TC12 DCQ Mean Square Error method by mapping the MSE value onto a > simple quality index. This mode is enabled by setting the sqi_enable bit, in > the DCQ Configuration register. > > The MSE value is compared to the thresholds set in the DCQ SQI Table > Registers to provide an SQI value between 0 (worst value) and 7 (best > value) as follows: > > In order to capture the SQI value, the DCQ Read Capture bit in the DCQ > Configuration register needs to be written as a high with the desired cable > pair specified in the DCQ Channel Number field of the same register. The > DCQ Read Capture bit will immediately self-clear and the result will be > available in the DCQ SQI register. In addition to the current SQI, the worst > case (lowest) SQI since the last read is available in the SQI Worst Case field. > The correlation between the SQI values stored in the DCQ SQI register and an > according Signal to Noise Ratio (SNR) based on Additive White Gaussian > (AWG) noise (bandwidth of 80 MHz @ 100 Mbps / 550 MHz @ 1000 Mbps) is > shown in Table 5-5. The bit error rates to be expected in the case of white > noise as interference signal is shown in the table as well for information > purposes. > > I had a quick look at OPEN ALLIANCE specification. It seems to specify how > each of these registers should look. It just failed to specify where in the > address map they are. So if you look at drivers implementing SQI, you see > most poke around in MDIO_MMD_VEND1. I wounder if we can actually > share the implementation between drivers, those that follow the standard, > with some paramatirisation where the registers are. > I don't think it will work, each phy may have different register set and may or may not supports SQI. Also it contains drivers of too old and that may not support SQI. Apart from this lan8814 is quad port phy and register set is completely different from other drivers. > Andrew Thanks, Divya
Hi Michael, > -----Original Message----- > From: Michael Walle <michael@walle.cc> > Sent: Friday, August 26, 2022 2:56 PM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>; Oleksij Rempel > <o.rempel@pengutronix.de> > Cc: UNGLinuxDriver <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 > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > [+ 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. > I just looked into it, it doesn't have SQI registers. I requested internal team to add SQI register set in published document. > > > >> 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? > Yes Channel 0 is correct. > 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. > Yeah, It needs uAPI extension. > -michael
> Yes Channel 0 is correct. > > > 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. > > > > Yeah, It needs uAPI extension. I think the current uAPI actually allows it, sort of. You can have multiple instances of a netlink property in a netlink message. So simply add 2 or 4 ETHTOOL_A_LINKSTATE_SQI properties. The existing user space tools will likely just print the first value it finds. Newer versions can walk the messages and print them all. The alternative is to add a new nest, like i did for cable test results: +-+-------------------------------------------+--------+---------------------+ | | ``ETHTOOL_A_CABLE_NEST_RESULT`` | nested | cable test result | +-+-+-----------------------------------------+--------+---------------------+ | | | ``ETHTOOL_A_CABLE_RESULTS_PAIR`` | u8 | pair number | +-+-+-----------------------------------------+--------+---------------------+ | | | ``ETHTOOL_A_CABLE_RESULTS_CODE`` | u8 | result code | +-+-+-----------------------------------------+--------+---------------------+ You can then explicitly indicate which cable pair the SQI value corresponds to. In order to keep backwards compatibility, you would still need to provide ETHTOOL_A_LINKSTATE_SQI, and then additionally have these nests. Andrew
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index e78d0bf69bc3..3775da7afc64 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -1975,6 +1975,13 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev, #define LAN8814_CLOCK_MANAGEMENT 0xd #define LAN8814_LINK_QUALITY 0x8e +#define LAN8814_DCQ_CTRL 0xe6 +#define LAN8814_DCQ_CTRL_READ_CAPTURE_ BIT(15) +#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; + val |= LAN8814_DCQ_CTRL_READ_CAPTURE_; + rc = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val); + if (rc < 0) + return rc; + + rc = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI); + if (rc < 0) + return rc; + + return FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, rc); +} + +static int lan8814_get_sqi_max(struct phy_device *phydev) +{ + return LAN8814_DCQ_SQI_MAX; +} + static struct phy_driver ksphy_driver[] = { { .phy_id = PHY_ID_KS8737, @@ -3117,6 +3150,8 @@ static struct phy_driver ksphy_driver[] = { .resume = kszphy_resume, .config_intr = lan8814_config_intr, .handle_interrupt = lan8814_handle_interrupt, + .get_sqi = lan8814_get_sqi, + .get_sqi_max = lan8814_get_sqi_max, }, { .phy_id = PHY_ID_LAN8804, .phy_id_mask = MICREL_PHY_ID_MASK,
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> --- drivers/net/phy/micrel.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+)