[net-next] net: mvpp2: add support for mii
Commit Message
Currently, mvpp2 only supports RGMII. This commit adds support for MII.
The description in Marvell's functional specification seems to be wrong.
To enable MII, we need to set GENCONF_CTRL0_PORT3_RGMII, while for RGMII
we need to clear it. This is also how U-Boot handles it.
Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
---
.../net/ethernet/marvell/mvpp2/mvpp2_main.c | 24 ++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
Comments
Hello Stefan,
On Wed, 6 Dec 2023 17:01:25 +0100
Stefan Eichenberger <eichest@gmail.com> wrote:
> Currently, mvpp2 only supports RGMII. This commit adds support for MII.
> The description in Marvell's functional specification seems to be wrong.
> To enable MII, we need to set GENCONF_CTRL0_PORT3_RGMII, while for RGMII
> we need to clear it. This is also how U-Boot handles it.
>
> Signed-off-by: Stefan Eichenberger <eichest@gmail.com>
> ---
> .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 24 ++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index 93137606869e..6f136f42e2bf 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -1513,10 +1513,21 @@ static void mvpp22_gop_init_rgmii(struct mvpp2_port *port)
> regmap_write(priv->sysctrl_base, GENCONF_PORT_CTRL0, val);
>
> regmap_read(priv->sysctrl_base, GENCONF_CTRL0, &val);
> - if (port->gop_id == 2)
> + if (port->gop_id == 2) {
> val |= GENCONF_CTRL0_PORT2_RGMII;
> - else if (port->gop_id == 3)
> + } else if (port->gop_id == 3) {
> val |= GENCONF_CTRL0_PORT3_RGMII_MII;
> +
> + /* According to the specification, GENCONF_CTRL0_PORT3_RGMII
> + * should be set to 1 for RGMII and 0 for MII. However, tests
> + * show that it is the other way around. This is also what
> + * U-Boot does for mvpp2, so it is assumed to be correct.
> + */
> + if (port->phy_interface == PHY_INTERFACE_MODE_MII)
> + val |= GENCONF_CTRL0_PORT3_RGMII;
> + else
> + val &= ~GENCONF_CTRL0_PORT3_RGMII;
> + }
> regmap_write(priv->sysctrl_base, GENCONF_CTRL0, val);
> }
>
> @@ -1615,6 +1626,7 @@ static int mvpp22_gop_init(struct mvpp2_port *port, phy_interface_t interface)
> return 0;
>
> switch (interface) {
> + case PHY_INTERFACE_MODE_MII:
> case PHY_INTERFACE_MODE_RGMII:
> case PHY_INTERFACE_MODE_RGMII_ID:
> case PHY_INTERFACE_MODE_RGMII_RXID:
> @@ -6948,8 +6960,11 @@ static int mvpp2_port_probe(struct platform_device *pdev,
> MAC_10000FD;
> }
>
> - if (mvpp2_port_supports_rgmii(port))
> + if (mvpp2_port_supports_rgmii(port)) {
> phy_interface_set_rgmii(port->phylink_config.supported_interfaces);
> + __set_bit(PHY_INTERFACE_MODE_MII,
> + port->phylink_config.supported_interfaces);
> + }
>
> if (comphy) {
> /* If a COMPHY is present, we can support any of the
> @@ -6973,6 +6988,9 @@ static int mvpp2_port_probe(struct platform_device *pdev,
> port->phylink_config.supported_interfaces);
> __set_bit(PHY_INTERFACE_MODE_SGMII,
> port->phylink_config.supported_interfaces);
> + } else if (phy_mode == PHY_INTERFACE_MODE_MII) {
> + __set_bit(PHY_INTERFACE_MODE_100BASEX,
> + port->phylink_config.supported_interfaces);
Can you explain that part ? I don't understand why 100BaseX is being
reported as a supported mode here. This whole section of the function
is about detecting what can be reported based on the presence or not of
a comphy driver / hardcoded comphy config. I don't think the comphy
here has anything to do with MII / 100BaseX
If 100BaseX can be carried on MII (which I don't know), shouldn't it be
reported no matter what ?
Thanks,
Maxime
Hi Maxime,
On Wed, Dec 06, 2023 at 06:27:05PM +0100, Maxime Chevallier wrote:
> > @@ -6973,6 +6988,9 @@ static int mvpp2_port_probe(struct platform_device *pdev,
> > port->phylink_config.supported_interfaces);
> > __set_bit(PHY_INTERFACE_MODE_SGMII,
> > port->phylink_config.supported_interfaces);
> > + } else if (phy_mode == PHY_INTERFACE_MODE_MII) {
> > + __set_bit(PHY_INTERFACE_MODE_100BASEX,
> > + port->phylink_config.supported_interfaces);
>
> Can you explain that part ? I don't understand why 100BaseX is being
> reported as a supported mode here. This whole section of the function
> is about detecting what can be reported based on the presence or not of
> a comphy driver / hardcoded comphy config. I don't think the comphy
> here has anything to do with MII / 100BaseX
>
> If 100BaseX can be carried on MII (which I don't know), shouldn't it be
> reported no matter what ?
I missunderstood that part, I thought it is a translation from interface
type to speed but it is obviously not. I already verfied that everything
works without this part and will remove it in version 2 of the patch.
Thanks a lot for the review!
Regards,
Stefan
Hello Stefan,
On Thu, 7 Dec 2023 10:01:08 +0100
Stefan Eichenberger <eichest@gmail.com> wrote:
> Hi Maxime,
>
> On Wed, Dec 06, 2023 at 06:27:05PM +0100, Maxime Chevallier wrote:
> > > @@ -6973,6 +6988,9 @@ static int mvpp2_port_probe(struct platform_device *pdev,
> > > port->phylink_config.supported_interfaces);
> > > __set_bit(PHY_INTERFACE_MODE_SGMII,
> > > port->phylink_config.supported_interfaces);
> > > + } else if (phy_mode == PHY_INTERFACE_MODE_MII) {
> > > + __set_bit(PHY_INTERFACE_MODE_100BASEX,
> > > + port->phylink_config.supported_interfaces);
> >
> > Can you explain that part ? I don't understand why 100BaseX is being
> > reported as a supported mode here. This whole section of the function
> > is about detecting what can be reported based on the presence or not of
> > a comphy driver / hardcoded comphy config. I don't think the comphy
> > here has anything to do with MII / 100BaseX
> >
> > If 100BaseX can be carried on MII (which I don't know), shouldn't it be
> > reported no matter what ?
>
> I missunderstood that part, I thought it is a translation from interface
> type to speed but it is obviously not. I already verfied that everything
> works without this part and will remove it in version 2 of the patch.
> Thanks a lot for the review!
No problem, thanks for the patch :)
Maxime
@@ -1513,10 +1513,21 @@ static void mvpp22_gop_init_rgmii(struct mvpp2_port *port)
regmap_write(priv->sysctrl_base, GENCONF_PORT_CTRL0, val);
regmap_read(priv->sysctrl_base, GENCONF_CTRL0, &val);
- if (port->gop_id == 2)
+ if (port->gop_id == 2) {
val |= GENCONF_CTRL0_PORT2_RGMII;
- else if (port->gop_id == 3)
+ } else if (port->gop_id == 3) {
val |= GENCONF_CTRL0_PORT3_RGMII_MII;
+
+ /* According to the specification, GENCONF_CTRL0_PORT3_RGMII
+ * should be set to 1 for RGMII and 0 for MII. However, tests
+ * show that it is the other way around. This is also what
+ * U-Boot does for mvpp2, so it is assumed to be correct.
+ */
+ if (port->phy_interface == PHY_INTERFACE_MODE_MII)
+ val |= GENCONF_CTRL0_PORT3_RGMII;
+ else
+ val &= ~GENCONF_CTRL0_PORT3_RGMII;
+ }
regmap_write(priv->sysctrl_base, GENCONF_CTRL0, val);
}
@@ -1615,6 +1626,7 @@ static int mvpp22_gop_init(struct mvpp2_port *port, phy_interface_t interface)
return 0;
switch (interface) {
+ case PHY_INTERFACE_MODE_MII:
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII_RXID:
@@ -6948,8 +6960,11 @@ static int mvpp2_port_probe(struct platform_device *pdev,
MAC_10000FD;
}
- if (mvpp2_port_supports_rgmii(port))
+ if (mvpp2_port_supports_rgmii(port)) {
phy_interface_set_rgmii(port->phylink_config.supported_interfaces);
+ __set_bit(PHY_INTERFACE_MODE_MII,
+ port->phylink_config.supported_interfaces);
+ }
if (comphy) {
/* If a COMPHY is present, we can support any of the
@@ -6973,6 +6988,9 @@ static int mvpp2_port_probe(struct platform_device *pdev,
port->phylink_config.supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_SGMII,
port->phylink_config.supported_interfaces);
+ } else if (phy_mode == PHY_INTERFACE_MODE_MII) {
+ __set_bit(PHY_INTERFACE_MODE_100BASEX,
+ port->phylink_config.supported_interfaces);
}
phylink = phylink_create(&port->phylink_config, port_fwnode,