| Message ID | E1pex8a-00Dvo3-G7@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | Another attempt at moving mv88e6xxx forward | expand |
On Wed, Mar 22, 2023 at 12:00:16PM +0000, Russell King (Oracle) wrote: > When a DSA driver (e.g. mv88e6xxx) provides a default configuration, > avoid validating the DT description as missing elements will be > provided by the DSA driver. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > net/dsa/port.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/dsa/port.c b/net/dsa/port.c > index c30e3a7d2145..23d9970c02d3 100644 > --- a/net/dsa/port.c > +++ b/net/dsa/port.c > @@ -1951,6 +1951,9 @@ static void dsa_shared_port_validate_of(struct dsa_port *dp, > *missing_phy_mode = false; > *missing_link_description = false; > > + if (dp->ds->ops->port_get_fwnode) > + return; I wounder if you should actually call it for the given port, and ensure it does not return -EOPNOTSUPP, or -EINVAL, etc, because it is not going to override that port? Then the DT values should be validated? Andrew
On Wed, Mar 22, 2023 at 07:51:22PM +0100, Andrew Lunn wrote: > On Wed, Mar 22, 2023 at 12:00:16PM +0000, Russell King (Oracle) wrote: > > When a DSA driver (e.g. mv88e6xxx) provides a default configuration, > > avoid validating the DT description as missing elements will be > > provided by the DSA driver. > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > --- > > net/dsa/port.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/net/dsa/port.c b/net/dsa/port.c > > index c30e3a7d2145..23d9970c02d3 100644 > > --- a/net/dsa/port.c > > +++ b/net/dsa/port.c > > @@ -1951,6 +1951,9 @@ static void dsa_shared_port_validate_of(struct dsa_port *dp, > > *missing_phy_mode = false; > > *missing_link_description = false; > > > > + if (dp->ds->ops->port_get_fwnode) > > + return; > > I wounder if you should actually call it for the given port, and > ensure it does not return -EOPNOTSUPP, or -EINVAL, etc, because it is > not going to override that port? Then the DT values should be > validated? Won't that mean that we need to implement the method for all DSA drivers?
On Wed, Mar 22, 2023 at 08:09:12PM +0000, Russell King (Oracle) wrote: > On Wed, Mar 22, 2023 at 07:51:22PM +0100, Andrew Lunn wrote: > > On Wed, Mar 22, 2023 at 12:00:16PM +0000, Russell King (Oracle) wrote: > > > When a DSA driver (e.g. mv88e6xxx) provides a default configuration, > > > avoid validating the DT description as missing elements will be > > > provided by the DSA driver. > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > --- > > > net/dsa/port.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/net/dsa/port.c b/net/dsa/port.c > > > index c30e3a7d2145..23d9970c02d3 100644 > > > --- a/net/dsa/port.c > > > +++ b/net/dsa/port.c > > > @@ -1951,6 +1951,9 @@ static void dsa_shared_port_validate_of(struct dsa_port *dp, > > > *missing_phy_mode = false; > > > *missing_link_description = false; > > > > > > + if (dp->ds->ops->port_get_fwnode) > > > + return; > > > > I wounder if you should actually call it for the given port, and > > ensure it does not return -EOPNOTSUPP, or -EINVAL, etc, because it is > > not going to override that port? Then the DT values should be > > validated? > > Won't that mean that we need to implement the method for all DSA > drivers? I mean call it if it exists. We should be only providing overrides for CPU and DSA ports, i think. So i expect it returns an error for user ports? And then we want to continue with the validation with what actually is in DT for user ports. Andrew
On Wed, Mar 22, 2023 at 09:14:31PM +0100, Andrew Lunn wrote: > On Wed, Mar 22, 2023 at 08:09:12PM +0000, Russell King (Oracle) wrote: > > On Wed, Mar 22, 2023 at 07:51:22PM +0100, Andrew Lunn wrote: > > > On Wed, Mar 22, 2023 at 12:00:16PM +0000, Russell King (Oracle) wrote: > > > > When a DSA driver (e.g. mv88e6xxx) provides a default configuration, > > > > avoid validating the DT description as missing elements will be > > > > provided by the DSA driver. > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > --- > > > > net/dsa/port.c | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/net/dsa/port.c b/net/dsa/port.c > > > > index c30e3a7d2145..23d9970c02d3 100644 > > > > --- a/net/dsa/port.c > > > > +++ b/net/dsa/port.c > > > > @@ -1951,6 +1951,9 @@ static void dsa_shared_port_validate_of(struct dsa_port *dp, > > > > *missing_phy_mode = false; > > > > *missing_link_description = false; > > > > > > > > + if (dp->ds->ops->port_get_fwnode) > > > > + return; > > > > > > I wounder if you should actually call it for the given port, and > > > ensure it does not return -EOPNOTSUPP, or -EINVAL, etc, because it is > > > not going to override that port? Then the DT values should be > > > validated? > > > > Won't that mean that we need to implement the method for all DSA > > drivers? > > I mean call it if it exists. We should be only providing overrides for > CPU and DSA ports, i think. So i expect it returns an error for user > ports? And then we want to continue with the validation with what > actually is in DT for user ports. I suppose we could do - but before adding that complexity, I think we should have a real use case for it.
diff --git a/net/dsa/port.c b/net/dsa/port.c index c30e3a7d2145..23d9970c02d3 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -1951,6 +1951,9 @@ static void dsa_shared_port_validate_of(struct dsa_port *dp, *missing_phy_mode = false; *missing_link_description = false; + if (dp->ds->ops->port_get_fwnode) + return; + if (of_get_phy_mode(dn, &mode)) { *missing_phy_mode = true; dev_err(ds->dev,
When a DSA driver (e.g. mv88e6xxx) provides a default configuration, avoid validating the DT description as missing elements will be provided by the DSA driver. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- net/dsa/port.c | 3 +++ 1 file changed, 3 insertions(+)