RE: [PATCH net-next V2 2/2] net: axienet: Add support for 2500base-X only configuration.

[Gupta, Suraj]

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Thursday, March 13, 2025 6:17 PM
> To: Gupta, Suraj <Suraj.Gupta2@xxxxxxx>
> Cc: Russell King (Oracle) <linux@xxxxxxxxxxxxxxx>; Pandey, Radhey Shyam
> <radhey.shyam.pandey@xxxxxxx>; andrew+netdev@xxxxxxx;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx;
> Simek, Michal <michal.simek@xxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>; Katakam, Harini
> <harini.katakam@xxxxxxx>
> Subject: Re: [PATCH net-next V2 2/2] net: axienet: Add support for 2500base-X only
> configuration.
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> On Thu, Mar 13, 2025 at 07:34:55AM +0000, Gupta, Suraj wrote:
> > [AMD Official Use Only - AMD Internal Distribution Only]
> >
> > > -----Original Message-----
> > > From: Gupta, Suraj
> > > Sent: Thursday, March 13, 2025 9:01 AM
> > > To: Andrew Lunn <andrew@xxxxxxx>; Russell King (Oracle)
> > > <linux@xxxxxxxxxxxxxxx>
> > > Cc: Pandey, Radhey Shyam <radhey.shyam.pandey@xxxxxxx>;
> > > andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> > > kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; robh@xxxxxxxxxx;
> > > krzk+dt@xxxxxxxxxx;
> > > conor+dt@xxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>;
> > > netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > > git (AMD-Xilinx) <git@xxxxxxx>; Katakam, Harini
> > > <harini.katakam@xxxxxxx>
> > > Subject: RE: [PATCH net-next V2 2/2] net: axienet: Add support for
> > > 2500base-X only configuration.
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Andrew Lunn <andrew@xxxxxxx>
> > > > Sent: Thursday, March 13, 2025 3:41 AM
> > > > To: Russell King (Oracle) <linux@xxxxxxxxxxxxxxx>
> > > > Cc: Gupta, Suraj <Suraj.Gupta2@xxxxxxx>; Pandey, Radhey Shyam
> > > > <radhey.shyam.pandey@xxxxxxx>; andrew+netdev@xxxxxxx;
> > > > davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> > > > pabeni@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> > > > conor+dt@xxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>;
> > > > netdev@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> > > > linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> > > > kernel@xxxxxxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>;
> > > > Katakam, Harini <harini.katakam@xxxxxxx>
> > > > Subject: Re: [PATCH net-next V2 2/2] net: axienet: Add support for
> > > > 2500base-X only configuration.
> > > >
> > > > Caution: This message originated from an External Source. Use
> > > > proper caution when opening attachments, clicking links, or responding.
> > > >
> > > >
> > > > > This is not an approach that works with the Linux kernel, sorry.
> > > > >
> > > > > What we have today is a driver that works for people's hardware
> > > > > - and we don't know what the capabilities of that hardware is.
> > > > >
> > > > > If there's hardware out there today which has XAE_ABILITY_2_5G
> > > > > set, but defaults to <=1G mode, this will work with the current driver.
> > > > > However, with your patch applied, it stops working because
> > > > > instead of the driver indicating MAC_10FD | MAC_100FD |
> > > > > MAC_1000FD, it only indicates MAC_2500FD. If this happens, it
> > > > > will regress users setups, and that is something we try not to do.
> > > > >
> > > > > Saying "someone else needs to add the code for their FPGA logic"
> > > > > misses the point - there may not be "someone else" to do that,
> > > > > which means the only option is to revert your change if it were merged.
> > > > > That in itself can cause its own user regressions because
> > > > > obviously stuff that works with this patch stops working.
> > > > >
> > > > > This is why we're being cautious, and given your responses, it's
> > > > > not making Andrew or myself feel that there's a reasonable
> > > > > approach being taken here.
> > > > >
> > > > > >From everything you have said, I am getting the feeling that
> > > > > >using
> > > > > XAE_ABILITY_2_5G to decide which of (1) or (2) is supported is
> > > > > just wrong. Given that we're talking about an implementation
> > > > > that has been synthesized at 2.5G and can't operate slower,
> > > > > maybe there's some way that could be created to specify that in DT?
> > > > >
> > > > > e.g. (and I'm sure the DT folk aren't going to like it)...
> > > > >
> > > > >       xlnx,axi-ethernet-X.YY.Z-2.5G
> > > > >
> > > > > (where X.YY.Z is the version) for implementations that can
> > > > > _only_ do 2.5G, and leave all other implementations only doing 1G and
> below.
> > > > >
> > > > > Or maybe some DT property. Or something else.
> > > >
> > > > Given that AMD has been talking about an FPGA, not silicon, i
> > > > actually think it would be best to change the IP to explicitly
> > > > enumerate how it has been synthesised. Make use of some register
> > > > bits which currently read as 0. Current IP would then remain as
> > > > 1000BaseX/SGMII, independent of how they have been synthesised.
> > > > Newer versions of the IP will then set the bits if they have been
> > > > synthesised as 2) or 3), and the driver can then enable that
> > > > capability, without breaking current generation systems. Plus
> > > > there needs to be big fat warning for anybody upgrading to the
> > > > latest version of the IP for bug fixes to ensure they
> > > correctly set the synthesis options because it now actually matters.
> > > >
> > > >          Andrew
> > >
> > > Synthesis options I mentioned in comment might sound confusing, let me clear it
> up.
> > > Actual synthesis options (as seen from configuration UI) IP provides are (1) and
> (2).
> > > When a user selects (2), IP comes with default 2.5G but also
> > > contains 1G capabilities which can be enabled and work with by
> > > adding switching FPGA logic (that makes it (3)).
> > >
> > > So, in short  if a user selects (1): It's <=1G only.
> > > If it selects (2): It's 2.5G only but can be made (3) by FPGA logic
> > > changes. So whatever existing systems for (3) would be working at default (2).
> > >
> > > This is the reason we didn't described (3) in V1 series as that is
> > > not provided by IP but can be synthesized after FPGA changes.
> > > Hope I'm able to answer your questions.
> > >
> >
> > I understand your concerns that current solution might break if any existing system
> uses (3).
> >
> > Russel's suggestion to use DT compatible we can try to send as RFC and check if
> that is accepted by DT maintainers.
> > Andrew's suggestion is complete IP solution and will involve IP changes to correct
> ability register behaviour based on synthesis time selection in a new IP version. But
> this will need internal discussions and IP rework and might take few months.
> >
> > Please let me know your thoughts on it.
>
> Given your comment:
>
> > It's 2.5G only but can be made (3) by FPGA logic changes
>
> It sounds like the design is not ideal at the moment, and you will be making changes
> anyway to clean this up. Being fixed in 2.5G mode is not nice, you need a PHY
> doing rate adaptation in order to support the slower speeds.
>
> You should also be thinking forwards. If you add support for fixed 2.5G now, can you
> cleanly integrate switching between 1000BaseX/SGMII and 2500BaseX in the future
> without breaking existing systems? You probably at least need a paper design of
> how this will work, so you can say you have thought through all the user cases: New
> IP old driver, Old IP new driver, etc...
>
>         Andrew

Hi Andrew, Russell,

Thank you for your guidance.

Based on our internal discussions, pushing for IP changes would be difficult and time-taking. As an alternative, we propose the following driver modifications to support the current 2.5G-only design while considering existing functionality and potential future switching logic:

1) The driver will advertise speeds based on the Temac ability register-both 1G and 2.5G for the current 2.5G-only design.

2) In axienet_pcs_config(), based on the return type of phylink_mii_c22_pcs_config() (as switching between 1G and 2.5G involves an interface change), if a speed change fails, the driver will print a warning:
"Check for potential missing switching logic in the design if 1G ↔ 2.5G switching is attempted."

Expected Effects:
1) Existing 1G Design
        New Driver:
        (a) Advertises 1G
        (b)If an attempt to change the configuration fails, a warning is printed regarding missing switching logic.

2) 2.5G-Only Design
        Old Driver: Fails, as it attempts to advertise only 1G.
        New Driver:
        (a) Advertises both 1G and 2.5G
        (b)If a configuration change attempt fails, a warning is printed regarding missing switching logic.

3) Custom Design with Switching Logic
        (a) Users will need to implement speed/interface change logic.
        (b) If a configuration change attempt fails, a warning is printed regarding potential missing switching logic.

I understand this approach might generate false warnings if phylink_mii_c22_pcs_config() fails due to other reasons. However, we can explore maintaining current interface / speed in axienet_local structure and track 1G <-> 2.5G switching.

Please let me know your thoughts.

Regards,
Suraj