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
>
