Re: [RFC PATCH net-next 4/5] net: macb: Configure High Speed Mac for given speed.

["Karumanchi, Vineeth" <vineeth@amd.com>]

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