| Message ID | 20231127104045.96722-4-ramon.nordin.rodriguez@ferroamp.se (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: microchip_t1s: additional phy support and collision detect handling | expand |
Hi, This implementation was introduced in the below patch itself. https://lore.kernel.org/netdev/20230426205049.xlfqluzwcvlm6ihh@soft-dev3-1/T/#m9a52b6c03b7fa637f70aed306b50b442590e24a3 As it is recommended to do it in a separate patch and also the datasheets of LAN867X Rev.B1 and LAN865X Rev.B0 internal PHY have these register is reserved, we were working for a feasible solution to describe this for customer and mainline. By the time many other things messed up and couldn't reach the mainline on time. We also implemented LAN867X Rev.C1 support already in the driver and published in our product site and in the process of preparing mainline patches. But unfortunately it took little more time to make it. https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/CodeExamples/EVB-LAN8670-USB_Linux_Driver_1v0.zip Anyway, thank you for the support. Good luck! Best regards, Parthiban V On 27/11/23 4:10 pm, Ramón N.Rodriguez wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > From: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se> > > This commit conditionally sets the collision detection bit on lan867x > and lan865x phys on changing PLCA enabled on/off. The intended realworld > scenario is that all nodes on the network run the same settings with > regards to plca, and when plca is enabled the physical layer guarantees > that no collisions should occur. > In a practical setting where it was tested running one node with > collision detection on and other off, the node with collision detection > on dropped a lot of packets, leading to a poor performing link. > Worth noting here is that the phys default/reset to plca disabled and > collision detection enabled. Thus this change would only have an effect > when changing settings via ethtool. > > Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se> > --- > drivers/net/phy/microchip_t1s.c | 29 ++++++++++++++++++++++++++--- > 1 file changed, 26 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c > index db84d850b165..3b1e82ecdf69 100644 > --- a/drivers/net/phy/microchip_t1s.c > +++ b/drivers/net/phy/microchip_t1s.c > @@ -23,8 +23,10 @@ > #define LAN865X_REG_CFGPARAM_DATA 0x00D9 > #define LAN865X_REG_CFGPARAM_CTRL 0x00DA > #define LAN865X_REG_STS2 0x0019 > +#define LAN86XX_REG_COLLISION_DETECT 0x0087 > > #define LAN865X_CFGPARAM_READ_ENABLE BIT(1) > +#define LAN86XX_COLLISION_DETECT_ENABLE BIT(15) > > /* The arrays below are pulled from the following table from AN1699 > * Access MMD Address Value Mask > @@ -363,6 +365,27 @@ static int lan86xx_read_status(struct phy_device *phydev) > return 0; > } > > +static int lan86xx_plca_set_cfg(struct phy_device *phydev, > + const struct phy_plca_cfg *plca_cfg) > +{ > + int err; > + > + err = genphy_c45_plca_set_cfg(phydev, plca_cfg); > + if (err) > + return err; > + > + /* Disable collision detect on the phy when PLCA is enabled. > + * Noise can be picked up as a false positive for collisions > + * leading to the phy dropping legitimate packets. > + * No collisions should be possible when all nodes are setup > + * for running PLCA. > + */ > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > + LAN86XX_REG_COLLISION_DETECT, > + LAN86XX_COLLISION_DETECT_ENABLE, > + plca_cfg->enabled ? 0 : LAN86XX_COLLISION_DETECT_ENABLE); > +} > + > static struct phy_driver microchip_t1s_driver[] = { > { > PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1), > @@ -371,7 +394,7 @@ static struct phy_driver microchip_t1s_driver[] = { > .config_init = lan867x_revb1_config_init, > .read_status = lan86xx_read_status, > .get_plca_cfg = genphy_c45_plca_get_cfg, > - .set_plca_cfg = genphy_c45_plca_set_cfg, > + .set_plca_cfg = lan86xx_plca_set_cfg, > .get_plca_status = genphy_c45_plca_get_status, > }, > { > @@ -381,7 +404,7 @@ static struct phy_driver microchip_t1s_driver[] = { > .config_init = lan867x_revc1_config_init, > .read_status = lan86xx_read_status, > .get_plca_cfg = genphy_c45_plca_get_cfg, > - .set_plca_cfg = genphy_c45_plca_set_cfg, > + .set_plca_cfg = lan86xx_plca_set_cfg, > .get_plca_status = genphy_c45_plca_get_status, > }, > { > @@ -391,7 +414,7 @@ static struct phy_driver microchip_t1s_driver[] = { > .config_init = lan865x_revb0_config_init, > .read_status = lan86xx_read_status, > .get_plca_cfg = genphy_c45_plca_get_cfg, > - .set_plca_cfg = genphy_c45_plca_set_cfg, > + .set_plca_cfg = lan86xx_plca_set_cfg, > .get_plca_status = genphy_c45_plca_get_status, > }, > }; > -- > 2.40.1 > >
On Mon, Nov 27, 2023 at 04:00:18PM +0000, Parthiban.Veerasooran@microchip.com wrote: > Hi, > > This implementation was introduced in the below patch itself. > > https://lore.kernel.org/netdev/20230426205049.xlfqluzwcvlm6ihh@soft-dev3-1/T/#m9a52b6c03b7fa637f70aed306b50b442590e24a3 > But the change was dropped in that patchset right? It's not present in netdev-next. > As it is recommended to do it in a separate patch and also the > datasheets of LAN867X Rev.B1 and LAN865X Rev.B0 internal PHY have these > register is reserved, we were working for a feasible solution to > describe this for customer and mainline. By the time many other things > messed up and couldn't reach the mainline on time. > Far as I can tell 'collision detect' is described in the following sections of respective datasheet: * 11.5.51 - LAN8650 * 5.4.48 - LAN8670 The rest of the bits are reserved though. The change I propose only manipulate the documented (bit 15) collision bit. Is your point that the lan8670 datasheet is only valid for rev.c and not rev.b? Andrew suggested on the cover letter that it be interesting to look at completly disabling collision detect, any strings you can pull at Microchip to investigate that? Also any input on my suggested testing methodology is more than welcome. > We also implemented LAN867X Rev.C1 support already in the driver and > published in our product site and in the process of preparing mainline > patches. But unfortunately it took little more time to make it. > > https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/CodeExamples/EVB-LAN8670-USB_Linux_Driver_1v0.zip I'm aware, we've been using a derivative of that work at ferroamp for development. But it's been driving me nuts, being the 't1s guy' at work, and maintaining out of tree drivers for weird dev boxes. It's not my intention to beat you to the punch, I just want a mainlined driver so that I can spend less of my time on plumbing. > > Anyway, thank you for the support. Good luck! Likewise! R
Hi, On 27/11/23 10:02 pm, Ramón Nordin Rodriguez wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Nov 27, 2023 at 04:00:18PM +0000, Parthiban.Veerasooran@microchip.com wrote: >> Hi, >> >> This implementation was introduced in the below patch itself. >> >> https://lore.kernel.org/netdev/20230426205049.xlfqluzwcvlm6ihh@soft-dev3-1/T/#m9a52b6c03b7fa637f70aed306b50b442590e24a3 >> > > But the change was dropped in that patchset right? It's not present in > netdev-next. Yes, it was dropped. The reason why I gave this info is, you mentioned in the cover letter that it took some time for you to find this in the datasheet. > >> As it is recommended to do it in a separate patch and also the >> datasheets of LAN867X Rev.B1 and LAN865X Rev.B0 internal PHY have these >> register is reserved, we were working for a feasible solution to >> describe this for customer and mainline. By the time many other things >> messed up and couldn't reach the mainline on time. >> > > Far as I can tell 'collision detect' is described in the following > sections of respective datasheet: > > * 11.5.51 - LAN8650 > * 5.4.48 - LAN8670 > > The rest of the bits are reserved though. The change I propose only > manipulate the documented (bit 15) collision bit. > > Is your point that the lan8670 datasheet is only valid for rev.c and not > rev.b? It is valid for rev.b1 as well but the current datasheet for rev.c1 doesn't show that info. > > Andrew suggested on the cover letter that it be interesting to look at > completly disabling collision detect, any strings you can pull at > Microchip to investigate that? Unfortunately I can't commit anything from my side as we are occupied with other activities. But definitely I will try my level best if time permits. Alternatively you can contact our Microchip customer support team if you are interested to do this testing at Microchip. > Also any input on my suggested testing methodology is more than welcome. > >> We also implemented LAN867X Rev.C1 support already in the driver and >> published in our product site and in the process of preparing mainline >> patches. But unfortunately it took little more time to make it. >> >> https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/CodeExamples/EVB-LAN8670-USB_Linux_Driver_1v0.zip > > I'm aware, we've been using a derivative of that work at ferroamp for > development. But it's been driving me nuts, being the 't1s guy' at work, > and maintaining out of tree drivers for weird dev boxes. > > It's not my intention to beat you to the punch, I just want a mainlined > driver so that I can spend less of my time on plumbing. I completely understand. Also it was not my intention too. Just to let you know why it is delayed in reaching mainline and a quick reference for the existing implementation. Enjoy! Best regards, Parthiban V > >> >> Anyway, thank you for the support. Good luck! > > Likewise! > R
> > > > But the change was dropped in that patchset right? It's not present in > > netdev-next. > Yes, it was dropped. The reason why I gave this info is, you mentioned > in the cover letter that it took some time for you to find this in the > datasheet. Ha, sometimes I have bad luck while thinking. I guess I never understood that change and subsequently forgot about it. > > > >> As it is recommended to do it in a separate patch and also the > >> datasheets of LAN867X Rev.B1 and LAN865X Rev.B0 internal PHY have these > >> register is reserved, we were working for a feasible solution to > >> describe this for customer and mainline. By the time many other things > >> messed up and couldn't reach the mainline on time. > >> > > > > Far as I can tell 'collision detect' is described in the following > > sections of respective datasheet: > > > > * 11.5.51 - LAN8650 > > * 5.4.48 - LAN8670 > > > > The rest of the bits are reserved though. The change I propose only > > manipulate the documented (bit 15) collision bit. > > > > Is your point that the lan8670 datasheet is only valid for rev.c and not > > rev.b? > It is valid for rev.b1 as well but the current datasheet for rev.c1 > doesn't show that info. Thank you for clearing that up! So if I get you correctly this change would in fact be correct for both lan867x rev.b and rev.c. > > > > Andrew suggested on the cover letter that it be interesting to look at > > completly disabling collision detect, any strings you can pull at > > Microchip to investigate that? > Unfortunately I can't commit anything from my side as we are occupied > with other activities. But definitely I will try my level best if time > permits. Alternatively you can contact our Microchip customer support > team if you are interested to do this testing at Microchip. I get that, might do as you suggest. > > Also any input on my suggested testing methodology is more than welcome. > > > >> We also implemented LAN867X Rev.C1 support already in the driver and > >> published in our product site and in the process of preparing mainline > >> patches. But unfortunately it took little more time to make it. > >> > >> https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/CodeExamples/EVB-LAN8670-USB_Linux_Driver_1v0.zip > > > > I'm aware, we've been using a derivative of that work at ferroamp for > > development. But it's been driving me nuts, being the 't1s guy' at work, > > and maintaining out of tree drivers for weird dev boxes. > > > > It's not my intention to beat you to the punch, I just want a mainlined > > driver so that I can spend less of my time on plumbing. > I completely understand. Also it was not my intention too. Just to let > you know why it is delayed in reaching mainline and a quick reference > for the existing implementation. Enjoy! I get that, in an ideal world FOSS would be the top prio for all industry actors. I'm lucky enough to get some time reserved for it. R
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c index db84d850b165..3b1e82ecdf69 100644 --- a/drivers/net/phy/microchip_t1s.c +++ b/drivers/net/phy/microchip_t1s.c @@ -23,8 +23,10 @@ #define LAN865X_REG_CFGPARAM_DATA 0x00D9 #define LAN865X_REG_CFGPARAM_CTRL 0x00DA #define LAN865X_REG_STS2 0x0019 +#define LAN86XX_REG_COLLISION_DETECT 0x0087 #define LAN865X_CFGPARAM_READ_ENABLE BIT(1) +#define LAN86XX_COLLISION_DETECT_ENABLE BIT(15) /* The arrays below are pulled from the following table from AN1699 * Access MMD Address Value Mask @@ -363,6 +365,27 @@ static int lan86xx_read_status(struct phy_device *phydev) return 0; } +static int lan86xx_plca_set_cfg(struct phy_device *phydev, + const struct phy_plca_cfg *plca_cfg) +{ + int err; + + err = genphy_c45_plca_set_cfg(phydev, plca_cfg); + if (err) + return err; + + /* Disable collision detect on the phy when PLCA is enabled. + * Noise can be picked up as a false positive for collisions + * leading to the phy dropping legitimate packets. + * No collisions should be possible when all nodes are setup + * for running PLCA. + */ + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, + LAN86XX_REG_COLLISION_DETECT, + LAN86XX_COLLISION_DETECT_ENABLE, + plca_cfg->enabled ? 0 : LAN86XX_COLLISION_DETECT_ENABLE); +} + static struct phy_driver microchip_t1s_driver[] = { { PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1), @@ -371,7 +394,7 @@ static struct phy_driver microchip_t1s_driver[] = { .config_init = lan867x_revb1_config_init, .read_status = lan86xx_read_status, .get_plca_cfg = genphy_c45_plca_get_cfg, - .set_plca_cfg = genphy_c45_plca_set_cfg, + .set_plca_cfg = lan86xx_plca_set_cfg, .get_plca_status = genphy_c45_plca_get_status, }, { @@ -381,7 +404,7 @@ static struct phy_driver microchip_t1s_driver[] = { .config_init = lan867x_revc1_config_init, .read_status = lan86xx_read_status, .get_plca_cfg = genphy_c45_plca_get_cfg, - .set_plca_cfg = genphy_c45_plca_set_cfg, + .set_plca_cfg = lan86xx_plca_set_cfg, .get_plca_status = genphy_c45_plca_get_status, }, { @@ -391,7 +414,7 @@ static struct phy_driver microchip_t1s_driver[] = { .config_init = lan865x_revb0_config_init, .read_status = lan86xx_read_status, .get_plca_cfg = genphy_c45_plca_get_cfg, - .set_plca_cfg = genphy_c45_plca_set_cfg, + .set_plca_cfg = lan86xx_plca_set_cfg, .get_plca_status = genphy_c45_plca_get_status, }, };