[PATCH v7 net-next 2/2] net: airoha: Introduce ethernet support for EN7581 SoC

[Jakub Kicinski]

On Fri, 12 Jul 2024 15:47:38 +0200 Lorenzo Bianconi wrote:
> > On Wed, 10 Jul 2024 10:47:41 +0200 Lorenzo Bianconi wrote:  
> > > Add airoha_eth driver in order to introduce ethernet support for
> > > Airoha EN7581 SoC available on EN7581 development board (en7581-evb).
> > > en7581-evb networking architecture is composed by airoha_eth as mac
> > > controller (cpu port) and a mt7530 dsa based switch.
> > > EN7581 mac controller is mainly composed by Frame Engine (FE) and
> > > QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic
> > > functionalities are supported now) while QDMA is used for DMA operation
> > > and QOS functionalities between mac layer and the dsa switch (hw QoS is
> > > not available yet and it will be added in the future).
> > > Currently only hw lan features are available, hw wan will be added with
> > > subsequent patches.  
> > 
> > It seems a bit unusual for DSA to have multiple ports, isn't it?
> > Can you explain how this driver differs from normal DSA a little 
> > more in the commit message?  
> 
> The Airoha eth SoC architecture is similar to mtk_eth_soc one (e.g MT7988a).
> The FrameEngine (FE) module has multiple GDM ports that are connected to
> different blocks. Current airoha_eth driver supports just GDM1 that is connected
> to a MT7530 DSA switch (I have not posted a tiny patch for mt7530 driver yet).
> In the future we will support even GDM{2,3,4} that will connect to differ
> phy modues (e.g. 2.5Gbps phy).

What I'm confused by is the mentioned of DSA. You put the port in the
descriptor, and there can only be one switch on the other side, right?

> > > +	q = &eth->q_tx[qid];
> > > +	if (WARN_ON_ONCE(!q->ndesc))
> > > +		goto error;
> > > +
> > > +	spin_lock_bh(&q->lock);
> > > +
> > > +	nr_frags = 1 + sinfo->nr_frags;
> > > +	if (q->queued + nr_frags > q->ndesc) {
> > > +		/* not enough space in the queue */
> > > +		spin_unlock_bh(&q->lock);
> > > +		return NETDEV_TX_BUSY;  
> > 
> > no need to stop the queue?  
> 
> reviewing this chunk, I guess we can get rid of it since we already block the
> txq at the end of airoha_dev_xmit() if we do not have enough space for the next
> packet:
> 
> 	if (q->ndesc - q->queued < q->free_thr)
> 		netif_tx_stop_queue(txq);

But @q is shared in your case isn't it? Unless we walk and stop all
ports this won't save us. Coincidentally not sure how useful BQL can
be in a setup like this :( It will have no way to figure out the real
egress rate given that each netdev only sees a (non-)random sample
of traffic sharing the queue :(