| Message ID | 20220905101730.29951-1-Divya.Koppera@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net-next] net: phy: micrel: Adding SQI support for lan8814 phy | expand |
On Mon, Sep 05, 2022 at 03:47:30PM +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. It is not supported for 10Mbps speed > > Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com> > --- > v1 -> v2 > - Given SQI support for all pairs of wires in 1000/100 base-T phy's > uAPI may run through all instances in future. At present returning > only first instance as uAPI supports for only 1 pair. > - SQI is not supported for 10Mbps speed, handled accordingly. I would prefer you solve the problem of returning all pairs. I'm not sure how useful the current implementation is, especially at 100Mbps, where pair 0 could actually be the transmit pair. Does it give a sensible value in that case? > +static int lan8814_get_sqi(struct phy_device *phydev) > +{ > + int ret, val, pair; > + int sqi_val[4]; > + > + if (phydev->speed == SPEED_10) > + return -EOPNOTSUPP; > + > + for (pair = 0; pair < 4; pair++) { > + val = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_CTRL); > + if (val < 0) > + return val; > + > + val &= ~LAN8814_DCQ_CTRL_CHANNEL_MASK; > + val |= pair; > + val |= LAN8814_DCQ_CTRL_READ_CAPTURE_; > + ret = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val); > + if (ret < 0) > + return ret; > + > + ret = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI); > + if (ret < 0) > + return ret; > + > + sqi_val[pair] = FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, ret); > + } > + > + return *sqi_val; How is this going to work in the future? sqi_val is on the stack. You cannot return a pointer to it. So this function is going to need modifications. If you really want to prepare for a future implementation which could return all four, i would probably make this a helper which takes a pair number. And then have a function call it once for pair 0. Andrew
Hi Andrew, > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Monday, September 5, 2022 6:40 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 v2 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 Mon, Sep 05, 2022 at 03:47:30PM +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. It is not supported > > for 10Mbps speed > > > > Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com> > > --- > > v1 -> v2 > > - Given SQI support for all pairs of wires in 1000/100 base-T phy's > > uAPI may run through all instances in future. At present returning > > only first instance as uAPI supports for only 1 pair. > > - SQI is not supported for 10Mbps speed, handled accordingly. > > I would prefer you solve the problem of returning all pairs. > I will try to improve uAPI. The other way I can try is the point you mentioned below to write helper with pair number and having function cal with pair 0. > I'm not sure how useful the current implementation is, especially at > 100Mbps, where pair 0 could actually be the transmit pair. Does it give a > sensible value in that case? > We do have SQI support for 100Mbps to pair 0 only. For other pairs SQI values are invalid values. > > +static int lan8814_get_sqi(struct phy_device *phydev) { > > + int ret, val, pair; > > + int sqi_val[4]; > > + > > + if (phydev->speed == SPEED_10) > > + return -EOPNOTSUPP; > > + > > + for (pair = 0; pair < 4; pair++) { > > + val = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_CTRL); > > + if (val < 0) > > + return val; > > + > > + val &= ~LAN8814_DCQ_CTRL_CHANNEL_MASK; > > + val |= pair; > > + val |= LAN8814_DCQ_CTRL_READ_CAPTURE_; > > + ret = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val); > > + if (ret < 0) > > + return ret; > > + > > + ret = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI); > > + if (ret < 0) > > + return ret; > > + > > + sqi_val[pair] = FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, ret); > > + } > > + > > + return *sqi_val; > > How is this going to work in the future? sqi_val is on the stack. You cannot > return a pointer to it. So this function is going to need modifications. > > If you really want to prepare for a future implementation which could return > all four, i would probably make this a helper which takes a pair number. And > then have a function call it once for pair 0. > I Will go for resolving this if I couldn't resolve that 4 pair issue in uAPI. > Andrew Thanks, Divya
> We do have SQI support for 100Mbps to pair 0 only. For other pairs > SQI values are invalid values. And you have tested this with auto-cross over, so that the pairs get swapped? Andrew
> On Mon, Sep 05, 2022 at 03:47:30PM +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. It is not supported for 10Mbps speed >> >> Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com> >> --- >> v1 -> v2 >> - Given SQI support for all pairs of wires in 1000/100 base-T phy's >> uAPI may run through all instances in future. At present returning >> only first instance as uAPI supports for only 1 pair. >> - SQI is not supported for 10Mbps speed, handled accordingly. > > I would prefer you solve the problem of returning all pairs. > > I'm not sure how useful the current implementation is, especially at > 100Mbps, where pair 0 could actually be the transmit pair. Does it > give a sensible value in that case? It is good practice to CC the patches to the ones who gave feedback on the previous versions. Not everyone is subscribed to all the high traffic mailinglist. Thanks, -michael
Hi Michael, > -----Original Message----- > From: Michael Walle <michael@walle.cc> > Sent: Wednesday, September 7, 2022 2:38 PM > To: Divya Koppera - I30481 <Divya.Koppera@microchip.com> > Cc: andrew@lunn.ch; UNGLinuxDriver <UNGLinuxDriver@microchip.com>; > 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; Oleksij Rempel > <o.rempel@pengutronix.de>; Michael Walle <michael@walle.cc> > Subject: Re: [PATCH v2 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 Mon, Sep 05, 2022 at 03:47:30PM +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. It is not > >> supported for 10Mbps speed > >> > >> Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com> > >> --- > >> v1 -> v2 > >> - Given SQI support for all pairs of wires in 1000/100 base-T phy's > >> uAPI may run through all instances in future. At present returning > >> only first instance as uAPI supports for only 1 pair. > >> - SQI is not supported for 10Mbps speed, handled accordingly. > > > > I would prefer you solve the problem of returning all pairs. > > > > I'm not sure how useful the current implementation is, especially at > > 100Mbps, where pair 0 could actually be the transmit pair. Does it > > give a sensible value in that case? > > It is good practice to CC the patches to the ones who gave feedback on the > previous versions. Not everyone is subscribed to all the high traffic > mailinglist. > Sorry I missed adding you. > Thanks, > -michael
On Tue, Sep 06, 2022 at 04:02:19PM +0200, Andrew Lunn wrote: > > We do have SQI support for 100Mbps to pair 0 only. For other pairs > > SQI values are invalid values. > > And you have tested this with auto-cross over, so that the pairs get > swapped? auto-cross is probably the default option. You'll need to force MDI or MDI-X mode. Regards, Oleksij
Hi, > -----Original Message----- > From: Oleksij Rempel <o.rempel@pengutronix.de> > Sent: Wednesday, September 7, 2022 3:00 PM > To: Andrew Lunn <andrew@lunn.ch> > Cc: Divya Koppera - I30481 <Divya.Koppera@microchip.com>; > 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 v2 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 Tue, Sep 06, 2022 at 04:02:19PM +0200, Andrew Lunn wrote: > > > We do have SQI support for 100Mbps to pair 0 only. For other pairs > > > SQI values are invalid values. > > > > And you have tested this with auto-cross over, so that the pairs get > > swapped? > > auto-cross is probably the default option. You'll need to force MDI or MDI-X > mode. > Yes, its default option. Sure will test in this way. Also I'll discuss with internal team regarding 1 pair access for 100Base-tx. May be its wrong and it needs to be 2 pairs. Thanks Divya > Regards, > Oleksij > -- > 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 |
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 7b8c5c8d013e..37845efe2cb6 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; @@ -2933,6 +2940,41 @@ static int lan8814_probe(struct phy_device *phydev) return 0; } +static int lan8814_get_sqi(struct phy_device *phydev) +{ + int ret, val, pair; + int sqi_val[4]; + + if (phydev->speed == SPEED_10) + return -EOPNOTSUPP; + + for (pair = 0; pair < 4; pair++) { + val = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_CTRL); + if (val < 0) + return val; + + val &= ~LAN8814_DCQ_CTRL_CHANNEL_MASK; + val |= pair; + val |= LAN8814_DCQ_CTRL_READ_CAPTURE_; + ret = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val); + if (ret < 0) + return ret; + + ret = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI); + if (ret < 0) + return ret; + + sqi_val[pair] = FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, ret); + } + + return *sqi_val; +} + +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, @@ -3123,6 +3165,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. It is not supported for 10Mbps speed Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com> --- v1 -> v2 - Given SQI support for all pairs of wires in 1000/100 base-T phy's uAPI may run through all instances in future. At present returning only first instance as uAPI supports for only 1 pair. - SQI is not supported for 10Mbps speed, handled accordingly. --- drivers/net/phy/micrel.c | 44 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)