RE: [PATCH net-next v3 2/2] net: lan743x: add support for RMII interface

[Thangaraj.S]

Hi Simon/Paolo,
Thanks for the comments.

> -----Original Message-----
> From: Paolo Abeni <pabeni@xxxxxxxxxx>
> Sent: Saturday, June 13, 2026 12:55 PM
> To: Simon Horman <horms@xxxxxxxxxx>; Thangaraj Samynathan - I53494
> <Thangaraj.S@xxxxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; Bryan Whitehead - C21958
> <Bryan.Whitehead@xxxxxxxxxxxxx>; UNGLinuxDriver
> <UNGLinuxDriver@xxxxxxxxxxxxx>; andrew+netdev@xxxxxxx;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next v3 2/2] net: lan743x: add support for RMII
> interface
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 6/10/26 6:26 PM, Simon Horman wrote:
> > From: 'Simon Horman' <horms@xxxxxxxxxx>
> >
> > This is an AI-generated review of your patch. The human sending this
> > email has considered the AI review valid, or at least plausible.
> > Full review at: https://sashiko.dev
> > ---
> > net: lan743x: add support for RMII interface
> >
> > This commit adds RMII interface support to the lan743x driver, wiring
> > up phy_interface selection, phylink supported_interfaces and MAC
> > capabilities, and enabling the RMII front-end via a new RMII_CTL
> > register write in lan743x_hardware_init().
> >
> >> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> >> b/drivers/net/ethernet/microchip/lan743x_main.c
> >> index 0798f3f1f435..ad3f833c8d86 100644
> >> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> >> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> >
> > [ ... ]
> >
> >> @@ -3562,6 +3574,11 @@ static int lan743x_hardware_init(struct
> lan743x_adapter *adapter,
> >>                      sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
> >>              }
> >>              lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
> >> +            if (adapter->is_rmii_en) {
> >> +                    rmii_ctl = lan743x_csr_read(adapter, RMII_CTL);
> >> +                    rmii_ctl |= RMII_CTL_RMII_ENABLE_;
> >> +                    lan743x_csr_write(adapter, RMII_CTL, rmii_ctl);
> >> +            }
> >
> > [Medium]
> > Should the RMII_CTL programming mirror the symmetric pattern used for
> > SGMII_CTL just above it?
[Thangaraj Samynathan] No, The RMII CTL register has no such bit similar to SGMII.
Setting Only RMII Enable is enough. 
> >
> > The SGMII block sets or clears the enable/power-down bits in both
> > directions:
> >
> >       sgmii_ctl = lan743x_csr_read(adapter, SGMII_CTL);
> >       if (adapter->is_sgmii_en) {
> >               sgmii_ctl |= SGMII_CTL_SGMII_ENABLE_;
> >               sgmii_ctl &= ~SGMII_CTL_SGMII_POWER_DN_;
> >       } else {
> >               sgmii_ctl &= ~SGMII_CTL_SGMII_ENABLE_;
> >               sgmii_ctl |= SGMII_CTL_SGMII_POWER_DN_;
> >       }
> >       lan743x_csr_write(adapter, SGMII_CTL, sgmii_ctl);
> >
> > The new RMII block only sets RMII_CTL_RMII_ENABLE_ when is_rmii_en is
> > true and never clears it on the other paths.
> >
> > If RMII_CTL_RMII_ENABLE_ is left set by a previous agent (for example,
> > a prior driver instance with a different strap, board firmware, or
> > state preserved across a warm reset where the driver itself sets
> > HW_CFG_RST_PROTECT_ during PM suspend, and
> pci11x1x_strap_get_status()
> > already accounts for cfg_load/HW_CFG_RST_PROTECT_), can the RMII
> > front-end remain enabled while the driver is configured for RGMII or
> > SGMII?
> >
> > Would adding an explicit clear in the !is_rmii_en path, mirroring the
> > SGMII handling, give the register a known state on every probe?
> 
> I think this one is worth addressing, as it's reported by both sashikos and could
> break functionality in hard-to-debug scenarios.
[Thangaraj Samynathan]  No, The hardware takes care of this. The register gets updated
On reset based on strap.
> 
Thanks,
Thangaraj Samynathan