Re: [PATCH net-next 18/20] net: ethernet: qualcomm: Add PPE MAC support for phylink

[Lei Wei]

On 1/10/2024 8:18 PM, Russell King (Oracle) wrote:
> On Wed, Jan 10, 2024 at 07:40:30PM +0800, Luo Jie wrote:
> > +static void ppe_phylink_mac_link_up(struct ppe_device *ppe_dev, int port,
> > +				    struct phy_device *phy,
> > +				    unsigned int mode, phy_interface_t interface,
> > +				    int speed, int duplex, bool tx_pause, bool rx_pause)
> > +{
> > +	struct phylink_pcs *pcs = ppe_phylink_mac_select_pcs(ppe_dev, port, interface);
> > +	struct ppe_uniphy *uniphy = pcs_to_ppe_uniphy(pcs);
> > +	struct ppe_port *ppe_port = ppe_port_get(ppe_dev, port);
> > +
> > +	/* Wait uniphy auto-negotiation completion */
> > +	ppe_uniphy_autoneg_complete_check(uniphy, port);
>
> Way too late...


Yes agree, this will be removed. If inband autoneg is used, 
pcs_get_state should report the link status.  Then this function call 
should not be needed and should be removed.

> > @@ -352,6 +1230,12 @@ static int ppe_port_maxframe_set(struct ppe_device *ppe_dev,
> >   }
> >   
> >   static struct ppe_device_ops qcom_ppe_ops = {
> > +	.phylink_setup = ppe_phylink_setup,
> > +	.phylink_destroy = ppe_phylink_destroy,
> > +	.phylink_mac_config = ppe_phylink_mac_config,
> > +	.phylink_mac_link_up = ppe_phylink_mac_link_up,
> > +	.phylink_mac_link_down = ppe_phylink_mac_link_down,
> > +	.phylink_mac_select_pcs = ppe_phylink_mac_select_pcs,
> >   	.set_maxframe = ppe_port_maxframe_set,
> >   };
>
> Why this extra layer of abstraction? If you need separate phylink
> operations, why not implement separate phylink_mac_ops structures?

This PPE driver will serve as the base driver for higher level drivers
such as the ethernet DMA (EDMA) driver and the DSA switch driver. The
ppe_device_ops is exported to these higher level drivers, to allow 
access to PPE operations. For example, the EDMA driver (ethernet 
netdevice driver to be pushed for review after the PPE driver) will use 
the phylink_setup/destroy ops for managing netdevice to PHY linkage. The 
set_maxframe op is also to be used by the EDMA driver during MTU change 
operation on the ethernet port.

I also mentioned it in the section "Exported PPE Device Operations" in 
PPE driver documentation:
https://lore.kernel.org/netdev/20240110114033.32575-2-quic_luoj@xxxxxxxxxxx/

Whereas the PPE DSA switch driver is expected to use the phylink_mac 
ops. However，we will remove the phylink_mac ops from this patch now 
since it is currently unused.