Re: [RFC PATCH v11 net-next 06/10] net: dsa: microchip: add support for phylink management

[Vladimir Oltean <olteanv@gmail.com>]

On Fri, Mar 25, 2022 at 10:23:37PM +0530, Prasanna Vengateshan wrote:
> phylink_get_caps() is implemented and reused KSZ commmon API for
> phylink_mac_link_down() operation
> 
> lan937x_phylink_mac_config configures the interface using
> lan937x_mac_config and lan937x_phylink_mac_link_up configures
> the speed/duplex/flow control.
> 
> Currently SGMII & in-band neg are not supported & it will be
> added later.

If SGMII is also an option, then what you're doing in
lan937x_parse_dt_rgmii_delay() looks wrong:

		/* skip for internal ports */
		if (lan937x_is_internal_phy_port(dev, p))
			continue;

		if (of_property_read_u32(port, "rx-internal-delay-ps", &val))
			val = 0;

		err = lan937x_set_rgmii_delay(dev, p, val, false);
		if (err)
			break;

Right now you assume that "isn't internal PHY" is the same as "is RGMII",
but this is not actually the future-proof way of doing things. Also
please consider that the driver you write now may end up booting on a DT
blob from the future. Do you do something sane when a port is configured
for a phy-mode you don't recognize? For one thing, you try to configure
RGMII delays on it, as far as I can see.

> +static void lan937x_phylink_get_caps(struct dsa_switch *ds, int port,
> +				     struct phylink_config *config)
> +{
> +	struct ksz_device *dev = ds->priv;
> +
> +	/* non legacy driver */
> +	config->legacy_pre_march2020 = false;
> +
> +	config->mac_capabilities = MAC_100FD;
> +
> +	/* internal T1 PHY */
> +	if (lan937x_is_internal_base_t1_phy_port(dev, port)) {
> +		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
> +			  config->supported_interfaces);
> +	} else if (lan937x_is_rgmii_port(dev, port)) {
> +		/* MII/RMII/RGMII ports */
> +		config->mac_capabilities |= MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +					    MAC_100HD | MAC_10 | MAC_1000FD;
> +		phy_interface_set_rgmii(config->supported_interfaces);
> +
> +		__set_bit(PHY_INTERFACE_MODE_MII,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_RMII,
> +			  config->supported_interfaces);
> +	}

No supported_interfaces for the 100base-TX internal PHY port? Does it
pass validation?

> +}
> +
>  const struct dsa_switch_ops lan937x_switch_ops = {
>  	.get_tag_protocol = lan937x_get_tag_protocol,
>  	.setup = lan937x_setup,
> @@ -307,6 +369,10 @@ const struct dsa_switch_ops lan937x_switch_ops = {
>  	.port_fast_age = ksz_port_fast_age,
>  	.port_max_mtu = lan937x_get_max_mtu,
>  	.port_change_mtu = lan937x_change_mtu,
> +	.phylink_get_caps = lan937x_phylink_get_caps,
> +	.phylink_mac_link_down = ksz_mac_link_down,
> +	.phylink_mac_config = lan937x_phylink_mac_config,
> +	.phylink_mac_link_up = lan937x_phylink_mac_link_up,
>  };
>  
>  int lan937x_switch_register(struct ksz_device *dev)
> -- 
> 2.30.2
>