On 01/16/2018 02:19 AM, Sebastian Reichel wrote:
> This adds support for enabling the internal PHY for a 'cpu' port.
> It has been tested on GE B850v3, B650v3 and B450v3, which have a
> built-in MV88E6240 switch hardwired to a PCIe based network card
> making use of the internal PHY. Since mv88e6xxx driver resets the
> chip during probe, the PHY is disabled without this patch resulting
> in missing link and non-functional switch device.
Apologies for the late review, the code is fine, but there is room for
improvement, see below:
>
> Signed-off-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>> ---
> net/dsa/dsa2.c | 25 +++++++++++++++++++------
> net/dsa/dsa_priv.h | 1 +
> net/dsa/port.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 1e287420ff49..f65938d10b6d 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -18,6 +18,7 @@
> #include <linux/rtnetlink.h>
> #include <linux/of.h>
> #include <linux/of_net.h>
> +#include <linux/of_mdio.h>
>
> #include "dsa_priv.h"
>
> @@ -271,11 +272,20 @@ static int dsa_port_setup(struct dsa_port *dp)
> break;
> case DSA_PORT_TYPE_CPU:
> case DSA_PORT_TYPE_DSA:
> - err = dsa_port_fixed_link_register_of(dp);
> - if (err) {
> - dev_err(ds->dev, "failed to register fixed link for
> port %d.%d\n",
> - ds->index, dp->index);
> - return err;
> + if (of_phy_is_fixed_link(dp->dn)) {
> + err = dsa_port_fixed_link_register_of(dp);
This function does exactly the same check you are adding, which
indicates that you should create a common function, e.g:
dsa_port_setup_link_of() which internally does check whether the PHY is
fixed or not and does the registration.
> + if (err) {
> + dev_err(ds->dev, "failed to register fixed link
> for port %d.%d\n",
> + ds->index, dp->index);
> + return err;
> + }
> + } else {
> + err = dsa_port_setup_phy_of(dp, true);
> + if (err) {
> + dev_err(ds->dev, "failed to enable phy for port
> %d.%d\n",
> + ds->index, dp->index);
> + return err;
> + }
> }
>
> break;
> @@ -301,7 +311,10 @@ static void dsa_port_teardown(struct dsa_port *dp)
> break;
> case DSA_PORT_TYPE_CPU:
> case DSA_PORT_TYPE_DSA:
> - dsa_port_fixed_link_unregister_of(dp);
> + if (of_phy_is_fixed_link(dp->dn))
> + dsa_port_fixed_link_unregister_of(dp);
> + else
> + dsa_port_setup_phy_of(dp, false);
Likewise, please rename dsa_port_fixed_link_unregister_of() into e.g:
dsa_port_teardown_link_of() and take care of the two cases.
The rest of the changes do look okay to me.
Note: there is still technically a misreprentation of how the PHY is
"attached" to the network device. In your DTSes, you have to have the
CPU port have a "phy-handle" to the internal PHY, while technically it
should be the i210 which has a "phy-handle" property to that PHY, and
even better, if the e1000e/idb drivers were PHYLIB capable, they could
manage it directly.
Since this is a link, which has two ends, it is probably acceptable to
make that shortcut with lack of a better solution.
--
Florian
