Hi Helmut,
On Tue, 16 Jun 2020 at 11:00, Helmut Grohne <helmut.gro...@intenta.de> wrote:
>
> The macb driver does not support configuring rgmii delays. At least for
> the Zynq GEM, delays are not supported by the hardware at all. However,
> the driver happily accepts and ignores any such delays.
>
> When operating in a mac to phy connection, the delay setting applies to
> the phy. Since the MAC does not support delays, the phy must provide
> them and the only supported mode is rgmii-id. However, in a fixed mac
> to mac connection, the delay applies to the mac itself. Therefore the
> only supported rgmii mode is rgmii.
>
> Link: https://lore.kernel.org/netdev/20200610081236.GA31659@laureti-dev/
> Signed-off-by: Helmut Grohne <helmut.gro...@intenta.de>
> ---
> drivers/net/ethernet/cadence/macb_main.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c
> b/drivers/net/ethernet/cadence/macb_main.c
> index 5b9d7c60eebc..bee5bf65e8b3 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -514,7 +514,7 @@ static void macb_validate(struct phylink_config *config,
> state->interface != PHY_INTERFACE_MODE_RMII &&
> state->interface != PHY_INTERFACE_MODE_GMII &&
> state->interface != PHY_INTERFACE_MODE_SGMII &&
> - !phy_interface_mode_is_rgmii(state->interface)) {
> + state->interface != PHY_INTERFACE_MODE_RGMII_ID) {
I don't think this change is correct though?
What if there were PCB traces in place, for whatever reason? Then the
driver would need to accept a phy with rgmii-txid, rgmii-rxid or
rgmii.
> bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> return;
> }
> @@ -694,6 +694,13 @@ static int macb_phylink_connect(struct macb *bp)
> struct phy_device *phydev;
> int ret;
>
> + if (of_phy_is_fixed_link(dn) &&
> + phy_interface_mode_is_rgmii(bp->phy_interface) &&
> + bp->phy_interface != PHY_INTERFACE_MODE_RGMII) {
> + netdev_err(dev, "RGMII delays are not supported\n");
> + return -EINVAL;
> + }
> +
Have you checked that this doesn't break any existing in-tree users?
> if (dn)
> ret = phylink_of_phy_connect(bp->phylink, dn, 0);
>
> --
> 2.20.1
>
Thanks,
-Vladimir
