linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vladimir Oltean <olteanv@gmail.com>
To: Prasanna Vengateshan <prasanna.vengateshan@microchip.com>
Cc: andrew@lunn.ch, netdev@vger.kernel.org, robh+dt@kernel.org,
	UNGLinuxDriver@microchip.com, Woojung.Huh@microchip.com,
	hkallweit1@gmail.com, linux@armlinux.org.uk, davem@davemloft.net,
	kuba@kernel.org, linux-kernel@vger.kernel.org,
	vivien.didelot@gmail.com, f.fainelli@gmail.com,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v3 net-next 05/10] net: dsa: microchip: add DSA support for microchip lan937x
Date: Mon, 2 Aug 2021 15:15:50 +0300	[thread overview]
Message-ID: <20210802121550.gqgbipqdvp5x76ii@skbuf> (raw)
In-Reply-To: <49678cce02ac03edc6bbbd1afb5f67606ac3efc2.camel@microchip.com>

On Mon, Aug 02, 2021 at 04:15:08PM +0530, Prasanna Vengateshan wrote:
> On Sat, 2021-07-31 at 18:04 +0300, Vladimir Oltean wrote:
> > > +void lan937x_mac_config(struct ksz_device *dev, int port,
> > > +                     phy_interface_t interface)
> > > +{
> > > +     u8 data8;
> > > +
> > > +     lan937x_pread8(dev, port, REG_PORT_XMII_CTRL_1, &data8);
> > > +
> > > +     /* clear MII selection & set it based on interface later */
> > > +     data8 &= ~PORT_MII_SEL_M;
> > > +
> > > +     /* configure MAC based on interface */
> > > +     switch (interface) {
> > > +     case PHY_INTERFACE_MODE_MII:
> > > +             lan937x_config_gbit(dev, false, &data8);
> > > +             data8 |= PORT_MII_SEL;
> > > +             break;
> > > +     case PHY_INTERFACE_MODE_RMII:
> > > +             lan937x_config_gbit(dev, false, &data8);
> > > +             data8 |= PORT_RMII_SEL;
> > > +             break;
> > > +     case PHY_INTERFACE_MODE_RGMII:
> > > +     case PHY_INTERFACE_MODE_RGMII_ID:
> > > +     case PHY_INTERFACE_MODE_RGMII_TXID:
> > > +     case PHY_INTERFACE_MODE_RGMII_RXID:
> > > +             lan937x_config_gbit(dev, true, &data8);
> > > +             data8 |= PORT_RGMII_SEL;
> > > +
> > > +             /* Add RGMII internal delay for cpu port*/
> > > +             if (dsa_is_cpu_port(dev->ds, port)) {
> >
> > Why only for the CPU port? I would like Andrew/Florian to have a look
> > here, I guess the assumption is that if the port has a phy-handle, the
> > RGMII delays should be dealt with by the PHY, but the logic seems to be
> > "is a CPU port <=> has a phy-handle / isn't a CPU port <=> doesn't have
> > a phy-handle"? What if it's a fixed-link port connected to a downstream
> > switch, for which this one is a DSA master?
>
> Thanks for reviewing the patches. My earlier proposal here was to check if there
> is no phydev (dp->slave->phydev) or if PHY is genphy, then apply RGMII delays
> assuming delays should be dealt with the phy driver if available. What do you
> think of that?

I don't know what to suggest, this is a bit of a minefield.
A while ago and in a different thread, Russell King said that
PHY_INTERFACE_MODE_RGMII_TXID means that the MAC should behave as if it
is connected to a PHY which has applied RGMII delays in the TX direction,
regardless of whether it is in fixed-link or not. So if the MAC was to
add any internal delays in PHY_INTERFACE_MODE_RGMII_TXID mode, those
would have to be RX delays, because the phy-mode specifies what was
already added and nothing more.
However, that is yet another problem. "what is already added" does not
mean "what more needs to be added". The fact that internal delays were
added in the TX direction doesn't necessarily mean that they still need
to be added in the RX direction. If you have a PHY which can only delay
the clock that it drives (the RX clock), and this is connected to a MAC
that cannot add any delays of its own, then you would end up adding PCB
traces on the board in the TX direction. But if you specify the phy-mode
as "rgmii-rxid", the MAC driver would complain that it can't add delays
in the TX direction (under the assumption that these are needed to work),
and if you specify the phy-mode as "rgmii-id", the PHY driver would
complain that it can't add delays in the TX direction.
So you'd have a system that works from a hardware PoV, but you wouldn't
have any way to describe it to Linux, which sucks.

I think the only way to do things correctly today and have a way to
describe any possible hardware setup is to parse the explicit
rx-internal-delay-ps and tx-internal-delay-ps properties in the MAC
driver, as defined in Documentation/bindings/net/ethernet-controller.yaml.
Then only let the MAC add the internal delays if the device tree has
added those properties, leave nothing down to assumptions.

  reply	other threads:[~2021-08-02 12:15 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-23 17:30 [PATCH v3 net-next 00/10] net: dsa: microchip: DSA driver support for LAN937x switch Prasanna Vengateshan
2021-07-23 17:30 ` [PATCH v3 net-next 01/10] dt-bindings: net: dsa: dt bindings for microchip lan937x Prasanna Vengateshan
2021-07-26 22:49   ` Rob Herring
2021-07-23 17:31 ` [PATCH v3 net-next 02/10] net: dsa: move mib->cnt_ptr reset code to ksz_common.c Prasanna Vengateshan
2021-07-23 18:53   ` Vladimir Oltean
2021-07-23 17:31 ` [PATCH v3 net-next 03/10] net: phy: Add support for LAN937x T1 phy driver Prasanna Vengateshan
2021-08-11 17:52   ` Prasanna Vengateshan
2021-07-23 17:31 ` [PATCH v3 net-next 04/10] net: dsa: tag_ksz: add tag handling for Microchip LAN937x Prasanna Vengateshan
2021-07-23 19:23   ` Vladimir Oltean
2021-07-23 17:31 ` [PATCH v3 net-next 05/10] net: dsa: microchip: add DSA support for microchip lan937x Prasanna Vengateshan
2021-07-31 15:04   ` Vladimir Oltean
2021-07-31 22:05     ` Andrew Lunn
2021-08-02 21:33       ` Vladimir Oltean
2021-08-03 14:43         ` Andrew Lunn
2021-08-03 15:05           ` Vladimir Oltean
2021-08-02 10:45     ` Prasanna Vengateshan
2021-08-02 12:15       ` Vladimir Oltean [this message]
2021-08-02 13:13         ` Andrew Lunn
2021-08-02 13:59           ` Vladimir Oltean
2021-08-02 20:47             ` Andrew Lunn
2021-08-03 16:54             ` Prasanna Vengateshan
2021-08-03 23:54               ` Vladimir Oltean
2021-08-04  9:59                 ` Russell King (Oracle)
2021-08-04 10:46                   ` Vladimir Oltean
2021-08-04 14:28                     ` Prasanna Vengateshan
2021-08-04 14:51                       ` Vladimir Oltean
2021-08-07 15:40                     ` Andrew Lunn
2021-08-07 17:00                       ` Vladimir Oltean
2021-08-11 17:44                       ` Prasanna Vengateshan
2021-08-11 18:23                         ` Andrew Lunn
2021-08-11 20:14                           ` Russell King (Oracle)
2021-08-11 20:20                             ` Vladimir Oltean
2021-08-11 20:22                               ` Andrew Lunn
2021-07-23 17:31 ` [PATCH v3 net-next 06/10] net: dsa: microchip: add support for phylink management Prasanna Vengateshan
2021-07-31 15:27   ` Vladimir Oltean
2021-08-03 17:04     ` Prasanna Vengateshan
2021-07-23 17:31 ` [PATCH v3 net-next 07/10] net: dsa: microchip: add support for ethtool port counters Prasanna Vengateshan
2021-07-23 17:31 ` [PATCH v3 net-next 08/10] net: dsa: microchip: add support for port mirror operations Prasanna Vengateshan
2021-07-31 15:24   ` Vladimir Oltean
2021-07-23 17:31 ` [PATCH v3 net-next 09/10] net: dsa: microchip: add support for fdb and mdb management Prasanna Vengateshan
2021-07-31 15:19   ` Vladimir Oltean
2021-07-23 17:31 ` [PATCH v3 net-next 10/10] net: dsa: microchip: add support for vlan operations Prasanna Vengateshan
2021-07-31 15:08   ` Vladimir Oltean
2021-08-02 10:48     ` Prasanna Vengateshan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210802121550.gqgbipqdvp5x76ii@skbuf \
    --to=olteanv@gmail.com \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=Woojung.Huh@microchip.com \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=f.fainelli@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=netdev@vger.kernel.org \
    --cc=prasanna.vengateshan@microchip.com \
    --cc=robh+dt@kernel.org \
    --cc=vivien.didelot@gmail.com \
    --subject='Re: [PATCH v3 net-next 05/10] net: dsa: microchip: add DSA support for microchip lan937x' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).