[PATCH net-next v1] lan78xx: Enable 125 MHz CLK and Auto Speed configuration for LAN7801 if NO EEPROM is detected

[PATCH net-next v1] lan78xx: Enable 125 MHz CLK and Auto Speed configuration for LAN7801 if NO EEPROM is detected

[Rengarajan S]

The 125MHz and 25MHz clock configurations are done in the initialization
regardless of EEPROM (125MHz is needed for RGMII 1000Mbps operation). After
a lite reset (lan78xx_reset), these contents go back to defaults(all 0, so
no 125MHz or 25MHz clock and no ASD/ADD). Also, after the lite reset, the
LAN7800 enables the ASD/ADD in the absence of EEPROM. There is no such
check for LAN7801.

Signed-off-by: Rengarajan S <rengarajan.s@microchip.com>
---
 drivers/net/usb/lan78xx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index ba6c8ac2a736..62dbfff8dad4 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2944,6 +2944,8 @@ static int lan78xx_reset(struct lan78xx_net *dev)
 		return ret;
 
 	buf |= HW_CFG_MEF_;
+	buf |= HW_CFG_CLK125_EN_;
+	buf |= HW_CFG_REFCLK25_EN_;
 
 	ret = lan78xx_write_reg(dev, HW_CFG, buf);
 	if (ret < 0)
@@ -3032,8 +3034,11 @@ static int lan78xx_reset(struct lan78xx_net *dev)
 		return ret;
 
 	/* LAN7801 only has RGMII mode */
-	if (dev->chipid == ID_REV_CHIP_ID_7801_)
+	if (dev->chipid == ID_REV_CHIP_ID_7801_) {
 		buf &= ~MAC_CR_GMII_EN_;
+		/* Enable Auto Duplex and Auto speed */
+		buf |= MAC_CR_AUTO_DUPLEX_ | MAC_CR_AUTO_SPEED_;
+	}
 
 	if (dev->chipid == ID_REV_CHIP_ID_7800_ ||
 	    dev->chipid == ID_REV_CHIP_ID_7850_) {
-- 
2.25.1

Re: [PATCH net-next v1] lan78xx: Enable 125 MHz CLK and Auto Speed configuration for LAN7801 if NO EEPROM is detected

[Simon Horman]

On Thu, May 02, 2024 at 10:25:03AM +0530, Rengarajan S wrote:
> The 125MHz and 25MHz clock configurations are done in the initialization
> regardless of EEPROM (125MHz is needed for RGMII 1000Mbps operation). After
> a lite reset (lan78xx_reset), these contents go back to defaults(all 0, so
> no 125MHz or 25MHz clock and no ASD/ADD). Also, after the lite reset, the
> LAN7800 enables the ASD/ADD in the absence of EEPROM. There is no such
> check for LAN7801.
> 
> Signed-off-by: Rengarajan S <rengarajan.s@microchip.com>

Hi Rengarajan,

This patch seems address two issues.
So I think it would be best to split it into two patches.

Also, are these problems bugs - do they have adverse effect visible by
users? If so perhaps they should be targeted at 'net' rather than
'net-next', and an appropriate Fixes tag should appear just above
the Signed-off-by line (no blank line in between).

...

-- 
pw-bot: under-review

Re: [PATCH net-next v1] lan78xx: Enable 125 MHz CLK and Auto Speed configuration for LAN7801 if NO EEPROM is detected

[Rengarajan.S]

Hi Simon,

Apologies for the delay in response. Thanks for reviewing the patch.
Please find my comments inline.

On Sat, 2024-05-04 at 09:49 +0100, Simon Horman wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Thu, May 02, 2024 at 10:25:03AM +0530, Rengarajan S wrote:
> > The 125MHz and 25MHz clock configurations are done in the
> > initialization
> > regardless of EEPROM (125MHz is needed for RGMII 1000Mbps
> > operation). After
> > a lite reset (lan78xx_reset), these contents go back to
> > defaults(all 0, so
> > no 125MHz or 25MHz clock and no ASD/ADD). Also, after the lite
> > reset, the
> > LAN7800 enables the ASD/ADD in the absence of EEPROM. There is no
> > such
> > check for LAN7801.
> > 
> > Signed-off-by: Rengarajan S <rengarajan.s@microchip.com>
> 
> Hi Rengarajan,
> 
> This patch seems address two issues.
> So I think it would be best to split it into two patches.

Sure. Will split the patch into two and will submit the updated patch
in the next revision shortly,

> 
> Also, are these problems bugs - do they have adverse effect visible
> by
> users? If so perhaps they should be targeted at 'net' rather than
> 'net-next', and an appropriate Fixes tag should appear just above
> the Signed-off-by line (no blank line in between).

The changes listed in the patch are feature additions where we give an
option of configuring the clock and speed in the absence of the EEPROM.
The current code does not have any bugs related to this. Since, these
are the additional features/requirements, we are targeting at 'net-
next' rather than 'net'.

> 
> ...
> 
> --
> pw-bot: under-review

Re: [PATCH net-next v1] lan78xx: Enable 125 MHz CLK and Auto Speed configuration for LAN7801 if NO EEPROM is detected

[Simon Horman]

On Thu, May 09, 2024 at 06:59:03AM +0000, Rengarajan.S@microchip.com wrote:
> Hi Simon,
> 
> Apologies for the delay in response. Thanks for reviewing the patch.
> Please find my comments inline.
> 
> On Sat, 2024-05-04 at 09:49 +0100, Simon Horman wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> > 
> > On Thu, May 02, 2024 at 10:25:03AM +0530, Rengarajan S wrote:
> > > The 125MHz and 25MHz clock configurations are done in the
> > > initialization
> > > regardless of EEPROM (125MHz is needed for RGMII 1000Mbps
> > > operation). After
> > > a lite reset (lan78xx_reset), these contents go back to
> > > defaults(all 0, so
> > > no 125MHz or 25MHz clock and no ASD/ADD). Also, after the lite
> > > reset, the
> > > LAN7800 enables the ASD/ADD in the absence of EEPROM. There is no
> > > such
> > > check for LAN7801.
> > > 
> > > Signed-off-by: Rengarajan S <rengarajan.s@microchip.com>
> > 
> > Hi Rengarajan,
> > 
> > This patch seems address two issues.
> > So I think it would be best to split it into two patches.
> 
> Sure. Will split the patch into two and will submit the updated patch
> in the next revision shortly,
> 
> > 
> > Also, are these problems bugs - do they have adverse effect visible
> > by
> > users? If so perhaps they should be targeted at 'net' rather than
> > 'net-next', and an appropriate Fixes tag should appear just above
> > the Signed-off-by line (no blank line in between).
> 
> The changes listed in the patch are feature additions where we give an
> option of configuring the clock and speed in the absence of the EEPROM.
> The current code does not have any bugs related to this. Since, these
> are the additional features/requirements, we are targeting at 'net-
> next' rather than 'net'.

Thanks, I agree net-next is appropriate for such changes.