Message-ID: <20210624125304.36636a44@ktm>
Date:   Thu, 24 Jun 2021 12:53:04 +0200
From:   Lukasz Majewski <lukma@...x.de>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Joakim Zhang <qiangqing.zhang@....com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>, netdev@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>,
        Mark Einon <mark.einon@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC 2/3] net: Provide switchdev driver for NXP's More Than IP
 L2 switch

Hi Andrew,

> > > Using volatile is generally wrong. Why do you need it?  
> > 
> > This was the code, which I took from the legacy driver. I will
> > adjust it.  
> 
> It is called 'vendor crap' for a reason.

:-)

> 
> > > > +	for_each_available_child_of_node(p, port) {
> > > > +		if (of_property_read_u32(port, "reg",
> > > > &port_num))
> > > > +			continue;
> > > > +
> > > > +		priv->n_ports = port_num;
> > > > +
> > > > +		fep_np = of_parse_phandle(port, "phy-handle",
> > > > 0);    
> > > 
> > > As i said, phy-handle points to a phy. It minimum, you need to
> > > call this mac-handle. But that then makes this switch driver very
> > > different to every other switch driver.  
> > 
> > Other drivers (DSA for example) use "ethernet" or "link" properties.
> > Maybe those can be reused?  
> 
> Not really. They have well known meanings and they are nothing like
> what you are trying to do. You need a new name. Maybe 'mac-handle'?

Ok.

> 
> 
> > > > +		pdev = of_find_device_by_node(fep_np);
> > > > +		ndev = platform_get_drvdata(pdev);
> > > > +		priv->fep[port_num - 1] = netdev_priv(ndev);
> > > >  
> > > 
> > > What happens when somebody puts reg=<42>; in DT?  
> > 
> > I do guess that this will break the code.
> > 
> > However, DSA DT descriptions also rely on the exact numbering [1]
> > (via e.g. reg property) of the ports. I've followed this paradigm.  
> 
> DSA does a range check:
> 
>         for_each_available_child_of_node(ports, port) {
>                 err = of_property_read_u32(port, "reg", &reg);
>                 if (err)
>                         goto out_put_node;
> 
>                 if (reg >= ds->num_ports) {
>                         dev_err(ds->dev, "port %pOF index %u exceeds
> num_ports (%zu)\n", port, reg, ds->num_ports);
>                         err = -EINVAL;
>                         goto out_put_node;
>                 }
> 

Ok.

> > > I would say, your basic structure needs to change, to make it more
> > > like other switchdev drivers. You need to replace the two FEC
> > > device instances with one switchdev driver.  
> > 
> > I've used the cpsw_new.c as the example.
> >   
> > > The switchdev driver will then
> > > instantiate the two netdevs for the two external MACs.  
> > 
> > Then there is a question - what about eth[01], which already
> > exists?  
> 
> They don't exist. cpsw_new is not used at the same time as cpsw, it
> replaces it. This new driver would replace the FEC driver.

I see.

> cpsw_new
> makes use of some of the code in the cpsw driver to implement two
> netdevs. This new FEC switch driver would do the same, make use of
> some of the low level code, e.g. for DMA access, MDIO bus, etc.

I'm not sure if the imx28 switch is similar to one from TI (cpsw-3g)
- it looks to me that the bypass mode for both seems to be very
different. For example, on NXP when switch is disabled we need to
handle two DMA[01]. When it is enabled, only one is used. The approach
with two DMAs is best handled with FEC driver instantiation.

> 
> > To be honest - such driver for L2 switch already has been forward
> > ported by me [2] to v4.19.  
> 
> Which is fine, you can do whatever you want in your own fork. But for
> mainline, we need a clean architecture.

This code is a forward port of vendor's (Freescale) old driver. It uses
the _wrong_ approach, but it can (still) be used in production after
some adjustments.

> I'm not convinced your code is
> that clean,

The code from [2] needs some vendor ioctl based tool (or hardcode) to
configure the switch. 

> and how well future features can be added. Do you have
> support for VLANS? Adding and removing entries to the lookup tables?
> How will IGMP snooping work? How will STP work?

This can be easily added with serving netstack hooks (as it is already
done with cpsw_new) in the new switchdev based version [3] (based on
v5.12).

> 
>     Andrew

Links:

[3] -
https://source.denx.de/linux/linux-imx28-l2switch/-/commits/imx28-v5.12-L2-upstream-switchdev-RFC_v1


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists