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

[Andrew Lunn]

> > > +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.

I've not looked at the datasheet yet, but maybe add some ASCII art
diagram of the architecture in the commit message, or even a .rst file
somewhere under Documentation. It is hard to get the big picture
looking at just the code, and only the MAC driver without all the
other parts.

> > > +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?

If it will be needed, then keep it. But it is the sort of thing which
raises questions, so its good to explain it, either in the commit
message, or in the code.

	Andrew