| Message ID | 20240611094233.865234-1-rengarajan.s@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,v1] lan78xx: lan7801 MAC support with lan8841 | expand |
On Tue, 11 Jun 2024 15:12:33 +0530 Rengarajan S wrote: > /* define external phy id */ > #define PHY_LAN8835 (0x0007C130) > +#define PHY_LAN8841 (0x00221650) For whatever reason the existing code uses a tab between define and its name, so let's stick to that? > #define PHY_KSZ9031RNX (0x00221620) > > /* use ethtool to change the level for any given device */ > @@ -2327,6 +2328,13 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev) > netdev_err(dev->net, "Failed to register fixup for PHY_LAN8835\n"); > return NULL; > } > + /* external PHY fixup for LAN8841 */ > + ret = phy_register_fixup_for_uid(PHY_LAN8841, 0xfffffff0, > + lan8835_fixup); > + if (ret < 0) { > + netdev_err(dev->net, "Failed to register fixup for PHY_LAN8841\n"); Don't you have to unregister the previous fixup on the error path here? In fact the existing error path for PHY_LAN8835 is missing an unregsiter for PHY_KSZ9031RNX. Could you please send a separate fix for that with a Fixes tag? > + return NULL; > + }
On Tue, Jun 11, 2024 at 03:12:33PM +0530, Rengarajan S wrote: > Add lan7801 MAC only support with lan8841. The PHY fixup is registered > for lan8841 and the initializations are done using lan8835_fixup since > the register configs are similar for both lann8841 and lan8835. What exactly does this fixup do? Looking at it, what protects it from being used on some other device which also happens to use the same PHY? Is there something to guarantee: struct lan78xx_net *dev = netdev_priv(phydev->attached_dev); really is a lan78xx_net * ? Andrew
Hi Jakub, Apologies for the delay in reply. Thanks for reviewing the patch and please find my comments inline. On Wed, 2024-06-12 at 18:33 -0700, Jakub Kicinski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Tue, 11 Jun 2024 15:12:33 +0530 Rengarajan S wrote: > > /* define external phy id */ > > #define PHY_LAN8835 (0x0007C130) > > +#define PHY_LAN8841 (0x00221650) > > For whatever reason the existing code uses a tab between define and > its > name, so let's stick to that? Sure. Will address the change in the next patch revision. > > > #define PHY_KSZ9031RNX (0x00221620) > > > > /* use ethtool to change the level for any given device */ > > @@ -2327,6 +2328,13 @@ static struct phy_device > > *lan7801_phy_init(struct lan78xx_net *dev) > > netdev_err(dev->net, "Failed to register > > fixup for PHY_LAN8835\n"); > > return NULL; > > } > > + /* external PHY fixup for LAN8841 */ > > + ret = phy_register_fixup_for_uid(PHY_LAN8841, > > 0xfffffff0, > > + lan8835_fixup); > > + if (ret < 0) { > > + netdev_err(dev->net, "Failed to register > > fixup for PHY_LAN8841\n"); > > Don't you have to unregister the previous fixup on the error path > here? > In fact the existing error path for PHY_LAN8835 is missing an > unregsiter > for PHY_KSZ9031RNX. There is a seperate register and unregister done for PHY_LAN8835 and PHY_KSZ9031RNX. Also, if the ret < 0 the fixup is not registered and there is no need to unregister it further. Can you please elaborate on what is missing in case of unregistering PHY_KSZ9031RNX. > > Could you please send a separate fix for that with a Fixes tag? > > > + return NULL; > > + }
Hi Andrew, Apologies for the delay in reply. Thanks for reviewing the patch. Please find my comments inline. On Thu, 2024-06-13 at 22:46 +0200, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Tue, Jun 11, 2024 at 03:12:33PM +0530, Rengarajan S wrote: > > Add lan7801 MAC only support with lan8841. The PHY fixup is > > registered > > for lan8841 and the initializations are done using lan8835_fixup > > since > > the register configs are similar for both lann8841 and lan8835. > > What exactly does this fixup do? Fixup related to the phy handle and manage the configuration and status registers of a particular phy. In this patch it is used to handle the configuration registers of LAN8841 which are similar to registers in LAN8835. > > Looking at it, what protects it from being used on some other device > which also happens to use the same PHY? Is there something to > guarantee: > > struct lan78xx_net *dev = netdev_priv(phydev->attached_dev); > > really is a lan78xx_net * ? In this case fixup is called through lan78xx only when interfacing the phy with lan78xx MAC. Since this will not be called on interfacing with other devices, it prevents them from accessing the registers.
On Thu, Jun 20, 2024 at 05:48:31AM +0000, Rengarajan.S@microchip.com wrote: > Hi Andrew, > > Apologies for the delay in reply. Thanks for reviewing the patch. > Please find my comments inline. > > On Thu, 2024-06-13 at 22:46 +0200, Andrew Lunn wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > know the content is safe > > > > On Tue, Jun 11, 2024 at 03:12:33PM +0530, Rengarajan S wrote: > > > Add lan7801 MAC only support with lan8841. The PHY fixup is > > > registered > > > for lan8841 and the initializations are done using lan8835_fixup > > > since > > > the register configs are similar for both lann8841 and lan8835. > > > > What exactly does this fixup do? > > Fixup related to the phy handle and manage the configuration and status > registers of a particular phy. In this patch it is used to handle the > configuration registers of LAN8841 which are similar to registers in > LAN8835. Details please, not hand waving. What does the errata say? Why is this specific to your USB dongle, and not all cases where this PHY is used? > > Looking at it, what protects it from being used on some other device > > which also happens to use the same PHY? Is there something to > > guarantee: > > > > struct lan78xx_net *dev = netdev_priv(phydev->attached_dev); > > > > really is a lan78xx_net * ? > > In this case fixup is called through lan78xx only when interfacing the > phy with lan78xx MAC. Since this will not be called on interfacing with > other devices, it prevents them from accessing the registers. Please give me a details explanation why this fixup will not be applied to other instances of this PHY in the system. Andrew
Hi Andrew, Thanks for the comments and apologies for the delay in reply. Please find my comments inline. On Thu, 2024-06-20 at 15:49 +0200, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > On Thu, Jun 20, 2024 at 05:48:31AM +0000, Rengarajan.S@microchip.com > wrote: > > Hi Andrew, > > > > Apologies for the delay in reply. Thanks for reviewing the patch. > > Please find my comments inline. > > > > On Thu, 2024-06-13 at 22:46 +0200, Andrew Lunn wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you > > > know the content is safe > > > > > > On Tue, Jun 11, 2024 at 03:12:33PM +0530, Rengarajan S wrote: > > > > Add lan7801 MAC only support with lan8841. The PHY fixup is > > > > registered > > > > for lan8841 and the initializations are done using > > > > lan8835_fixup > > > > since > > > > the register configs are similar for both lann8841 and lan8835. > > > > > > What exactly does this fixup do? > > > > Fixup related to the phy handle and manage the configuration and > > status > > registers of a particular phy. In this patch it is used to handle > > the > > configuration registers of LAN8841 which are similar to registers > > in > > LAN8835. > > Details please, not hand waving. What does the errata say? Why is > this > specific to your USB dongle, and not all cases where this PHY is > used? Although, there is no specific errata available for adding fixup specific to lan78xx USB dongle, we have added the fixup for handing specific configurations to ensure the PHY operates correctly with the MAC. In this case while transmitting from MAC to PHY the device does not add the delay locally at its TX input pins. It expects the TXC delay to be provided by on-chip MAC. Since the delay calculated in this case is specific to the lan78xx USB dongle it is not possible to use this fixup for interfacing with generic MAC. Similarly for ksz9131 we add fixed 2ns delay, and the additional delay is provided by pad skew registers. In this case too the delay value calculated is specific to lan78xx USB and is not applicable for other MACs. > > > > Looking at it, what protects it from being used on some other > > > device > > > which also happens to use the same PHY? Is there something to > > > guarantee: > > > > > > struct lan78xx_net *dev = netdev_priv(phydev->attached_dev); > > > > > > really is a lan78xx_net * ? > > > > In this case fixup is called through lan78xx only when interfacing > > the > > phy with lan78xx MAC. Since this will not be called on interfacing > > with > > other devices, it prevents them from accessing the registers. > > Please give me a details explanation why this fixup will not be > applied to other instances of this PHY in the system. As stated above, the TXC delay calculated for the PHY is specific to the lan78xx on-chip MAC. This delay ensures that both the phy and MAC clock delay timing is met. Any other MACs connected will need a different delay values to be synchronized with MAC and hence these instances will get failed. > > Andrew
> Although, there is no specific errata available for adding fixup > specific to lan78xx USB dongle, we have added the fixup for handing > specific configurations to ensure the PHY operates correctly with the > MAC. In this case while transmitting from MAC to PHY the device does > not add the delay locally at its TX input pins. It expects the TXC > delay to be provided by on-chip MAC. Since the delay calculated in this > case is specific to the lan78xx USB dongle it is not possible to use > this fixup for interfacing with generic MAC. Have you tried PHY_INTERFACE_MODE_RGMII_TXID when connecting to the PHY? The four PHY_INTERFACE_MODE_RGMII_* values are the official way to ask the PHY to insert delays, or not. If that is all you are doing, i don't think you need these fixups at all. > > Please give me a details explanation why this fixup will not be > > applied to other instances of this PHY in the system. > > As stated above, the TXC delay calculated for the PHY is specific to > the lan78xx on-chip MAC. This delay ensures that both the phy and MAC > clock delay timing is met. Any other MACs connected will need a > different delay values to be synchronized with MAC and hence these > instances will get failed. You did not answer my question. Show me the code path which prevents this being applied to other PHYs. Is there a comparison to netdev somewhere when applying the fixup? Give me the file:line number. Andrew
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 5add4145d9fc..ab6f0c42b4d9 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -479,6 +479,7 @@ struct lan78xx_net { /* define external phy id */ #define PHY_LAN8835 (0x0007C130) +#define PHY_LAN8841 (0x00221650) #define PHY_KSZ9031RNX (0x00221620) /* use ethtool to change the level for any given device */ @@ -2327,6 +2328,13 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev) netdev_err(dev->net, "Failed to register fixup for PHY_LAN8835\n"); return NULL; } + /* external PHY fixup for LAN8841 */ + ret = phy_register_fixup_for_uid(PHY_LAN8841, 0xfffffff0, + lan8835_fixup); + if (ret < 0) { + netdev_err(dev->net, "Failed to register fixup for PHY_LAN8841\n"); + return NULL; + } /* add more external PHY fixup here if needed */ phydev->is_internal = false; @@ -2390,6 +2398,8 @@ static int lan78xx_phy_init(struct lan78xx_net *dev) 0xfffffff0); phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0); + phy_unregister_fixup_for_uid(PHY_LAN8841, + 0xfffffff0); } } return -EIO; @@ -4239,6 +4249,7 @@ static void lan78xx_disconnect(struct usb_interface *intf) phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0); phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0); + phy_unregister_fixup_for_uid(PHY_LAN8841, 0xfffffff0); phy_disconnect(net->phydev);
Add lan7801 MAC only support with lan8841. The PHY fixup is registered for lan8841 and the initializations are done using lan8835_fixup since the register configs are similar for both lann8841 and lan8835. The PHY is unregistered at two instances, one during init and other during disconnect. Signed-off-by: Rengarajan S <rengarajan.s@microchip.com> --- drivers/net/usb/lan78xx.c | 11 +++++++++++ 1 file changed, 11 insertions(+)