[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241108085320.fqbell5bfx3roey4@DEN-DL-M70577>
Date: Fri, 8 Nov 2024 08:53:20 +0000
From: Daniel Machon <daniel.machon@...rochip.com>
To: Andrew Lunn <andrew@...n.ch>
CC: <UNGLinuxDriver@...rochip.com>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "Lars
Povlsen" <lars.povlsen@...rochip.com>, Steen Hegelund
<Steen.Hegelund@...rochip.com>, Horatiu Vultur
<horatiu.vultur@...rochip.com>, Russell King <linux@...linux.org.uk>,
<jacob.e.keller@...el.com>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH net-next 7/7] net: lan969x: add function for configuring
RGMII port devices
Hi Andrew,
> > The lan969x switch device includes two RGMII interfaces (port 28 and 29)
> > supporting data speeds of 1 Gbps, 100 Mbps and 10 Mbps.
> >
> > Add new function: rgmii_config() to the match data ops, and use it to
> > configure RGMII port devices when doing a port config. On Sparx5, the
> > RGMII configuration will always be skipped, as the is_port_rgmii() will
> > return false.
> >
> > Reviewed-by: Steen Hegelund <Steen.Hegelund@...rochip.com>
> > Reviewed-by: Horatiu Vultur <horatiu.vultur@...rochip.com>
> > Signed-off-by: Daniel Machon <daniel.machon@...rochip.com>
> > ---
> > drivers/net/ethernet/microchip/lan969x/lan969x.c | 105 +++++++++++++++++++++
> > .../net/ethernet/microchip/sparx5/sparx5_main.h | 2 +
> > .../net/ethernet/microchip/sparx5/sparx5_port.c | 3 +
> > 3 files changed, 110 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan969x/lan969x.c b/drivers/net/ethernet/microchip/lan969x/lan969x.c
> > index cfd57eb42c04..0681913a05d4 100644
> > --- a/drivers/net/ethernet/microchip/lan969x/lan969x.c
> > +++ b/drivers/net/ethernet/microchip/lan969x/lan969x.c
> > @@ -9,6 +9,17 @@
> > #define LAN969X_SDLB_GRP_CNT 5
> > #define LAN969X_HSCH_LEAK_GRP_CNT 4
> >
> > +#define LAN969X_RGMII_TX_CLK_DISABLE 0 /* Disable TX clock generation*/
> > +#define LAN969X_RGMII_TX_CLK_125MHZ 1 /* 1000Mbps */
> > +#define LAN969X_RGMII_TX_CLK_25MHZ 2 /* 100Mbps */
> > +#define LAN969X_RGMII_TX_CLK_2M5MHZ 3 /* 10Mbps */
> > +#define LAN969X_RGMII_PORT_START_IDX 28 /* Index of the first RGMII port */
> > +#define LAN969X_RGMII_PORT_RATE 2 /* 1000Mbps */
> > +#define LAN969X_RGMII_SHIFT_90DEG 3 /* Phase shift 90deg. (2 ns @ 125MHz) */
> > +#define LAN969X_RGMII_IFG_TX 4 /* TX Inter Frame Gap value */
> > +#define LAN969X_RGMII_IFG_RX1 5 /* RX1 Inter Frame Gap value */
> > +#define LAN969X_RGMII_IFG_RX2 1 /* RX2 Inter Frame Gap value */
> > +
> > static const struct sparx5_main_io_resource lan969x_main_iomap[] = {
> > { TARGET_CPU, 0xc0000, 0 }, /* 0xe00c0000 */
> > { TARGET_FDMA, 0xc0400, 0 }, /* 0xe00c0400 */
> > @@ -293,6 +304,99 @@ static irqreturn_t lan969x_ptp_irq_handler(int irq, void *args)
> > return IRQ_HANDLED;
> > }
> >
> > +static int lan969x_port_config_rgmii(struct sparx5 *sparx5,
> > + struct sparx5_port *port,
> > + struct sparx5_port_config *conf)
> > +{
> > + int tx_clk_freq, idx = port->portno - LAN969X_RGMII_PORT_START_IDX;
> > + enum sparx5_port_max_tags max_tags = port->max_vlan_tags;
> > + enum sparx5_vlan_port_type vlan_type = port->vlan_type;
> > + bool dtag, dotag, tx_delay = false, rx_delay = false;
> > + u32 etype;
> > +
> > + tx_clk_freq = (conf->speed == SPEED_10 ? LAN969X_RGMII_TX_CLK_2M5MHZ :
> > + conf->speed == SPEED_100 ? LAN969X_RGMII_TX_CLK_25MHZ :
> > + LAN969X_RGMII_TX_CLK_125MHZ);
>
> https://www.spinics.net/lists/netdev/msg1040925.html
>
> Once it is merged, i think this does what you want.
>
Nice! Thanks for letting me know.
> > + if (conf->phy_mode == PHY_INTERFACE_MODE_RGMII ||
> > + conf->phy_mode == PHY_INTERFACE_MODE_RGMII_TXID)
> > + rx_delay = true;
> > +
> > + if (conf->phy_mode == PHY_INTERFACE_MODE_RGMII ||
> > + conf->phy_mode == PHY_INTERFACE_MODE_RGMII_RXID)
> > + tx_delay = true;
>
> O.K, now warning bells are ringing in this reviews head.
>
> What i don't see is the value you pass to the PHY? You obviously need
> to mask out what the MAC is doing when talking to the PHY, otherwise
> both ends will add delays.
>
What value should be passed to the PHY?
We (the MAC) add the delays based on the PHY modes - so does the PHY.
RGMII, we add both delays.
RGMII_ID, the PHY adds both delays.
RGMII_TXID, we add the rx delay, the PHY adds the tx delay.
RGMII_RXID, we add the tx delay, the PHY adds the rx delay.
Am I missing something here? :-)
> And in general in Linux, we have the PHY add the delays, not the
> MAC. It is somewhat arbitrary, but the vast majority of systems do
> that. The exception is systems where the PHY is too dumb/cheap to add
> the delays and so the MAC has to do it. I'm don't know of any
> Microchip PHYs which don't support RGMII delays.
Ack.
>
> Andrew
/Daniel
Powered by blists - more mailing lists