[OpenWrt-Devel] [PATCH 1/2] swconfig: implement (PHY) generic PORT_LINK setter

[Jonas Gorski]

Hi,

On 21 January 2016 at 14:55, Rafał Miłecki <zajec5 at gmail.com> wrote:
> It's quite common for switches to have PHY per port so we may use a
> generic function for setting port link. We just need an API to access
> PHYs which this patch also adds.
>
> Signed-off-by: Rafał Miłecki <zajec5 at gmail.com>
> ---
>  .../linux/generic/files/drivers/net/phy/swconfig.c | 44 ++++++++++++++++++++--
>  target/linux/generic/files/include/linux/switch.h  |  3 ++
>  2 files changed, 44 insertions(+), 3 deletions(-)
>
> diff --git a/target/linux/generic/files/drivers/net/phy/swconfig.c b/target/linux/generic/files/drivers/net/phy/swconfig.c
> index 9a5f1e9..8b9bb51 100644
> --- a/target/linux/generic/files/drivers/net/phy/swconfig.c
> +++ b/target/linux/generic/files/drivers/net/phy/swconfig.c
> @@ -25,6 +25,7 @@
>  #include <linux/switch.h>
>  #include <linux/of.h>
>  #include <linux/version.h>
> +#include <uapi/linux/mii.h>
>
>  #define SWCONFIG_DEVNAME       "switch%d"
>
> @@ -131,10 +132,47 @@ static int
>  swconfig_set_link(struct switch_dev *dev, const struct switch_attr *attr,
>                         struct switch_val *val)
>  {
> -       if (!dev->ops->set_port_link)
> -               return -EOPNOTSUPP;
> +       struct switch_port_link *link = val->value.link;
> +       int port = val->port_vlan;
> +
> +       if (port == dev->cpu_port)
> +               return -EINVAL;

The cpu port might not be the only port that may not be modified;
sometimes there is more than one fixed connection, sometimes the phy
ports aren't contiguous.

I think it would make more sense to add a function for switch drivers
to call than to do it directly in the callback, so they can do
something like

int b53_set_link(...)
{
       /* TODO: BCM63XX requires special handling as it can have
external phys, and ports might be GE or only FE */
       if (is63xx(dev))
              return -EINVAL;

      if (port == dev->CPU_PORT)
              return -EINVAL;

      if (!(BIT(port) & dev->enabled_ports))
              return -EINVAL;

       if (link->speed == SWITCH_PORT_SPEED_1000 && (is5325() || is5365())
              return -EINVAL;

       if (link->speed == SWITCH_PORT_SPEED_1000 && !link->duplex)
              return -EINVAL;

       return switch_generic_set_link(...);
}

> +
> +       /* Custom implementation */
> +       if (dev->ops->set_port_link)
> +               return dev->ops->set_port_link(dev, port, link);
> +

And the following being the generic function to call:

> +       /* Chceck if we can use generic implementation */

*Check

> +       if (!dev->ops->phy_write16)
> +               return -ENOTSUPP;
> +

this-^ one maybe with a WARN_ON() to spot misusage.

> +       /* Generic implementation */
> +       if (link->aneg) {
> +               dev->ops->phy_write16(dev, port, MII_BMCR, 0x0000);
> +               dev->ops->phy_write16(dev, port, MII_BMCR, BMCR_ANENABLE | BMCR_ANRESTART);
> +       } else {
> +               u16 bmcr = 0;
>
> -       return dev->ops->set_port_link(dev, val->port_vlan, val->value.link);
> +               if (link->duplex)
> +                       bmcr |= BMCR_FULLDPLX;
> +
> +               switch (link->speed) {
> +               case SWITCH_PORT_SPEED_10:
> +                       break;
> +               case SWITCH_PORT_SPEED_100:
> +                       bmcr |= BMCR_SPEED100;
> +                       break;
> +               case SWITCH_PORT_SPEED_1000:
> +                       bmcr |= BMCR_SPEED1000;
> +                       break;
> +               default:
> +                       return -ENOTSUPP;
> +               }
> +
> +               dev->ops->phy_write16(dev, port, MII_BMCR, bmcr);
> +       }
> +
> +       return 0;
>  }
>
>  static int
> diff --git a/target/linux/generic/files/include/linux/switch.h b/target/linux/generic/files/include/linux/switch.h
> index 4ada0e5..ab587ea 100644
> --- a/target/linux/generic/files/include/linux/switch.h
> +++ b/target/linux/generic/files/include/linux/switch.h
> @@ -99,6 +99,9 @@ struct switch_dev_ops {
>                              struct switch_port_link *link);
>         int (*get_port_stats)(struct switch_dev *dev, int port,
>                               struct switch_port_stats *stats);
> +
> +       int (*phy_read16)(struct switch_dev *dev, int addr, u8 reg, u16 *value);
> +       int (*phy_write16)(struct switch_dev *dev, int addr, u8 reg, u16 value);
>  };
>
>  struct switch_dev {
> --
> 1.8.4.5


Jonas
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel