Hi Russel,
On 10/9/2024 2:49 PM, Russell King (Oracle) wrote:
On Wed, Oct 09, 2024 at 11:09:45AM +0530, Vineeth Karumanchi wrote:
HS Mac configuration steps:
- Configure speed and serdes rate bits of USX_CONTROL register from
user specified speed in the device-tree.
- Enable HS Mac for 5G and 10G speeds.
- Reset RX receive path to achieve USX block lock for the
configured serdes rate.
- Wait for USX block lock synchronization.
Move the initialization instances to macb_usx_pcs_link_up().
It only partly moves stuff there, creating what I can only call a mess
which probably doesn't work correctly.
Please consider the MAC and PCS as two separate boxes - register
settings controlled in one box should not be touched by the other box.
For example, macb_mac_config() now does this:
old_ncr = ncr = macb_or_gem_readl(bp, NCR);
...
} else if (macb_is_gem(bp)) {
...
ncr &= ~GEM_BIT(ENABLE_HS_MAC);
...
if (old_ncr ^ ncr)
macb_or_gem_writel(bp, NCR, ncr);
meanwhile:
@@ -564,14 +565,59 @@ static void macb_usx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode,
int duplex)
{
struct macb *bp = container_of(pcs, struct macb, phylink_usx_pcs);
...
+ /* Enable HS MAC for high speeds */
+ if (hs_mac) {
+ config = macb_or_gem_readl(bp, NCR);
+ config |= GEM_BIT(ENABLE_HS_MAC);
+ macb_or_gem_writel(bp, NCR, config);
+ }
Arguably, the time that this would happen is when the interface mode
changes which would cause a full reconfiguration and thus both of
these functions will be called, but it's not easy to follow that's
what is going on here.
It also looks like you're messing with MAC registers in the PCS code,
setting the MAC speed there. Are the PCS and MAC so integrated together
that abstracting the PCS into its own separate code block leads to
problems?
Agreed, Since our current hardware configuration lacks AN and PHY, I've
relocated the ENABLE_HS_MAC configuration into PCS to
allow speed changes using ethtool. When more hardware with a PHY that
supports AN becomes available,
the phylink will invoke macb_mac_config() with the communicated speed
(phylinkstate->speed).
It is possible to make ENABLE_HS_MAC conditional based on speed.
Currently, for fixed-link, will keep the earlier implementation.
Please let me know your thoughts and comments.
--
🙏 vineeth
