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

From: Lorenzo Bianconi <[email-masked]>
Date: Mon, 24 Jun 2024 01:55:10 +0200

> > +static int airoha_fe_set_pse_oq_rsv(struct airoha_eth *eth,
> > +				    u32 port, u32 queue, u32 val)
> > +{
> > +	u32 orig_val, tmp, all_rsv, fq_limit;
> > +	const u32 pse_port_oq_id[] = {
> > +		PSE_PORT0_QUEUE,
> > +		PSE_PORT1_QUEUE,
> > +		PSE_PORT2_QUEUE,
> > +		PSE_PORT3_QUEUE,
> > +		PSE_PORT4_QUEUE,
> > +		PSE_PORT5_QUEUE,
> > +		PSE_PORT6_QUEUE,
> > +		PSE_PORT7_QUEUE,
> > +		PSE_PORT8_QUEUE,
> > +		PSE_PORT9_QUEUE,
> > +		PSE_PORT10_QUEUE
> > +	};
> 
> > +static void airoha_fe_oq_rsv_init(struct airoha_eth *eth)
> > +{
> > +	int i;
> > +
> > +	/* hw misses PPE2 oq rsv */
> > +	airoha_fe_set(eth, REG_FE_PSE_BUF_SET,
> > +		      PSE_DEF_RSV_PAGE * PSE_PORT8_QUEUE);
> > +
> > +	for (i = 0; i < PSE_PORT0_QUEUE; i++)
> > +		airoha_fe_set_pse_oq_rsv(eth, 0, i, 0x40);
> > +	for (i = 0; i < PSE_PORT1_QUEUE; i++)
> > +		airoha_fe_set_pse_oq_rsv(eth, 1, i, 0x40);
> > +
> > +	for (i = 6; i < PSE_PORT2_QUEUE; i++)
> > +		airoha_fe_set_pse_oq_rsv(eth, 2, i, 0);
> > +
> > +	for (i = 0; i < PSE_PORT3_QUEUE; i++)
> > +		airoha_fe_set_pse_oq_rsv(eth, 3, i, 0x40);
> 
> Code like this is making me wounder about the split between MAC
> driver, DSA driver and DSA tag driver. Or if it should actually be a
> pure switchdev driver?

airoha_eth driver implements just MAC features (FE and QDMA). Currently we only
support the connection to the DSA switch (GDM1). EN7581 SoC relies on mt7530 driver
for DSA (I have not posted the patch for mt7530 yet, I will do after airoha_eth
ones).

> 
> If there some open architecture documentation for this device?
> 
> What are these ports about?

airoha_fe_oq_rsv_init() (we can improve naming here :) is supposed to configure
hw pre-allocated memory for each queue available in Packet Switching Engine
(PSE) ports. PSE ports are not switch ports, but SoC internal ports used to
connect PSE to different modules. In particular, we are currently implementing
just the two connections below:
- CDM1 (port0) connects PSE to QDMA1
- GDM1 (port1) connects PSE to MT7530 DSA switch

In the future we will post support for GDM2, GDM3 and GDM4 ports that are
connecting PSE to exteranl PHY modules.

> 
> > +static void airoha_qdma_clenaup_rx_queue(struct airoha_queue *q)
> 
> cleanup?

ack, I will fix it.

> 
> > +static int airoha_dev_open(struct net_device *dev)
> > +{
> > +	struct airoha_eth *eth = netdev_priv(dev);
> > +	int err;
> > +
> > +	if (netdev_uses_dsa(dev))
> > +		airoha_fe_set(eth, REG_GDM1_INGRESS_CFG, GDM1_STAG_EN_MASK);
> > +	else
> > +		airoha_fe_clear(eth, REG_GDM1_INGRESS_CFG, GDM1_STAG_EN_MASK);
> 
> Does that imply both instances of the GMAC are not connected to the
> switch? Can one be used with a PHY?

The check above is used to support configuration where MT7530 DSA switch module
is not loaded (I tested this configuration removing the MT7530 DSA switch from
board dts and resetting the switch). Since for the moment we just support GDM1
port (PSE port connected to the switch) we can probably assume it is always the
case and remove this check. In the future we will need this configuration to support
GDM2 or GDM3 (PSE port connected to external phy modules). Do you prefer to
always set GDM1_STAG_EN_MASK for the moment?

Regards,
Lorenzo

> 	Andrew