| Message ID | 20240502045503.36298-1-rengarajan.s@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1] lan78xx: Enable 125 MHz CLK and Auto Speed configuration for LAN7801 if NO EEPROM is detected | expand |
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). ...
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
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.
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_) {
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(-)