| Message ID | 20200330135345.4361-1-matthias.schiffer@ew.tq-group.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,1/4] net: dsa: allow switch drivers to override default slave PHY addresses | expand |
On 3/30/2020 6:53 AM, Matthias Schiffer wrote: > Avoid having to define a PHY for every physical port when PHY addresses > are fixed, but port index != PHY address. > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> You could do this much more elegantly by doing this with Device Tree and specifying the built-in PHYs to be hanging off the switch's internal MDIO bus and specifying the port to PHY address mapping, you would only patch #4 then.
On Mon, 2020-03-30 at 20:04 -0700, Florian Fainelli wrote: > > On 3/30/2020 6:53 AM, Matthias Schiffer wrote: > > Avoid having to define a PHY for every physical port when PHY > > addresses > > are fixed, but port index != PHY address. > > > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com > > > > > You could do this much more elegantly by doing this with Device Tree > and > specifying the built-in PHYs to be hanging off the switch's internal > MDIO bus and specifying the port to PHY address mapping, you would > only > patch #4 then. This does work indeed, but it seems we have different ideas on elegance. I'm not happy about the fact that an implementor needs to study the switch manual in great detail to find out about things like the PHY address offsets when the driver could just to the right thing by default. Requiring this only for some switch configurations, while others work fine with the defaults, doesn't make this any less confusing (I'd even argue that it would be better if there weren't any default PHY and IRQ mappings for the switch ports, but I also understand that this can't easily be removed at this point...) In particular when PHY IRQ support is desired (not implemented on the PHY driver side for this switch yet; not sure if my current project will require it), indices are easy to get wrong - which might not be noticed as long as there is no PHY driver with IRQ support for the port PHYs, potentially breaking existing Device Trees with future kernel updates. For this reason, I think at least patch #2 should be considered even if #1 and #3 are rejected. Kind regards, Matthias
On Tue, 2020-03-31 at 11:09 +0200, Matthias Schiffer wrote: > On Mon, 2020-03-30 at 20:04 -0700, Florian Fainelli wrote: > > > > On 3/30/2020 6:53 AM, Matthias Schiffer wrote: > > > Avoid having to define a PHY for every physical port when PHY > > > addresses > > > are fixed, but port index != PHY address. > > > > > > Signed-off-by: Matthias Schiffer < > > > matthias.schiffer@ew.tq-group.com > > > > > > > > You could do this much more elegantly by doing this with Device > > Tree > > and > > specifying the built-in PHYs to be hanging off the switch's > > internal > > MDIO bus and specifying the port to PHY address mapping, you would > > only > > patch #4 then. > > This does work indeed, but it seems we have different ideas on > elegance. > > I'm not happy about the fact that an implementor needs to study the > switch manual in great detail to find out about things like the PHY > address offsets when the driver could just to the right thing by > default. Requiring this only for some switch configurations, while > others work fine with the defaults, doesn't make this any less > confusing (I'd even argue that it would be better if there weren't > any > default PHY and IRQ mappings for the switch ports, but I also > understand that this can't easily be removed at this point...) > > In particular when PHY IRQ support is desired (not implemented on the > PHY driver side for this switch yet; not sure if my current project > will require it), indices are easy to get wrong - which might not be > noticed as long as there is no PHY driver with IRQ support for the > port > PHYs, potentially breaking existing Device Trees with future kernel > updates. For this reason, I think at least patch #2 should be > considered even if #1 and #3 are rejected. > > Kind regards, > Matthias net-next is open again, and I'd like to come to a conclusion regarding this patch series before I resend it (or parts of it). It is my impression that a detailed configuration in the Device Tree is most useful when the driver that it configures is very generic, so different values are useful in practice. The mv88e6xxx driver is not generic in this sense: It already lists every single piece of supported hardware, often using completely different code for different chips - which is already not configurable in the Device Tree (and being able to wouldn't make much sense IMO). Having the additional pieces of sane defaults in the driver that are introduced by the patches 1..3 of this series avoids mistakes in the DT configuration, for settings that never need to be modified for a given switch model supported by mv88e6xxx. Kind regards, Matthias
diff --git a/include/net/dsa.h b/include/net/dsa.h index aeb411e77b9a..8216f3687799 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -391,6 +391,7 @@ struct dsa_switch_ops { int (*setup)(struct dsa_switch *ds); void (*teardown)(struct dsa_switch *ds); + int (*get_phy_address)(struct dsa_switch *ds, int port); u32 (*get_phy_flags)(struct dsa_switch *ds, int port); /* diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 8ced165a7908..1c78f8cae9e9 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1546,7 +1546,7 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev) struct dsa_switch *ds = dp->ds; phy_interface_t mode; u32 phy_flags = 0; - int ret; + int addr, ret; ret = of_get_phy_mode(port_dn, &mode); if (ret) @@ -1578,7 +1578,13 @@ static int dsa_slave_phy_setup(struct net_device *slave_dev) /* We could not connect to a designated PHY or SFP, so try to * use the switch internal MDIO bus instead */ - ret = dsa_slave_phy_connect(slave_dev, dp->index); + + if (ds->ops->get_phy_address) + addr = ds->ops->get_phy_address(ds, dp->index); + else + addr = dp->index; + + ret = dsa_slave_phy_connect(slave_dev, addr); if (ret) { netdev_err(slave_dev, "failed to connect to port %d: %d\n",
Avoid having to define a PHY for every physical port when PHY addresses are fixed, but port index != PHY address. Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- include/net/dsa.h | 1 + net/dsa/slave.c | 10 ++++++++-- 2 files changed, 9 insertions(+), 2 deletions(-)