Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Mon, 27 Nov 2023 15:08:41 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Daniel Golle <daniel@makrotopia.org>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Chunfeng Yun <chunfeng.yun@mediatek.com>,
        Vinod Koul <vkoul@kernel.org>,
        Kishon Vijay Abraham I <kishon@kernel.org>,
        Felix Fietkau <nbd@nbd.name>, John Crispin <john@phrozen.org>,
        Sean Wang <sean.wang@mediatek.com>,
        Mark Lee <Mark-MC.Lee@mediatek.com>,
        Lorenzo Bianconi <lorenzo@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Alexander Couzens <lynxis@fe80.eu>,
        Philipp Zabel <p.zabel@pengutronix.de>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-phy@lists.infradead.org
Subject: Re: [RFC PATCH 6/8] net: pcs: add driver for MediaTek USXGMII PCS
Message-ID: <ZWSw+cecLhjbpX4o@shell.armlinux.org.uk>
References: <cover.1699565880.git.daniel@makrotopia.org>
 <b9f787f3e4aa36e148d7595495af60db53f74417.1699565880.git.daniel@makrotopia.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b9f787f3e4aa36e148d7595495af60db53f74417.1699565880.git.daniel@makrotopia.org>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Thu, Nov 09, 2023 at 09:51:57PM +0000, Daniel Golle wrote:
> Add driver for USXGMII PCS found in the MediaTek MT7988 SoC and supporting
> USXGMII, 10GBase-R and 5GBase-R interface modes. In order to support
> Cisco SGMII, 1000Base-X and 2500Base-X via the also present LynxI PCS
> create a wrapped PCS taking care of the components shared between the
> new USXGMII PCS and the legacy LynxI PCS.

What is the actual hardware setup here?

From what I can tell, it's something like this:

         .---- LynxI PCS ----.
 MAC ---+                     +--- PEXP --- external
         `--- USXGMII PCS ---'

Where PEXP is the serdes, handled by the drivers/phy layer in the
kernel. This is not an unusual setup, but we don't have the serdes PHY
controlled by the PCS driver.

You seem to be combining the whole lot into one driver, which seems
rather odd.

I would suggest that the serdes PHY is handled in the MAC driver, using
the mac_prepare(), mac_config() and mac_finish() methods, as well as
other parts of the driver:

- when the netdev is opened, call phy_power_on(pextp)
- when the netdev is closed, call phy_power_off(pextp)
- in mac_prepare(), if the interface has changed, call phy_reset(pextp)
- in mac_finish(), if the interface has changed, update your recorded
  interface mode to detect future changes in either mac_prepare() or
  mac_finish(), and call phy_set_mode_ext(pextp, PHY_MODE_ETHERNET,
  interface).

That will move most of what seems to be duplicated between the two PCS
instances out of the PCS driver and to MAC level, and then the PCS parts
become more about just driving the PCS hardware and nothing beyond that.
More specifically, the wrapping's only function then is to deal with the
sgmii reset. What exactly is that reset signal controlling? The reset to
the LynxI PCS or something else?

If you don't do that (and I prefer that you _do_ the above), then the
following comments apply to the code here:

1. the use of phy_power_on() without any calls to phy_power_off().
   These are counted calls, and after the first call to phy_power_on(),
   the only effect will be to increase the enable-counts of any
   associated regulator and the power count. So, basically you're
   missing calls to phy_power_off(). I suggest a call to phy_power_off()
   in the pcs_disable() function.

2. calling phy_power_on() in pcs_config() is entirely unnecessary.
   pcs_config() will not be called unless pcs_enable() has _already_
   been called, so the call to phy_power_on() in the pcs_enable()
   function is entirely sufficient.

With these two fixed, it means that the pextp PHY will be powered up
when one of the pcs_enable() functions is called, and powered down
when one of the pcs_disable() functions is called.

3. the complicated reset sequence, which is basically:
   - phy_reset(pextp)
   - reset_control_assert(sgmii or xfi reset)
   - *sleep* 100-500us (yes, sleep)
   - reset_control_deassert(sgmii or xfi reset)
   - *delay* 10ms (not sleep, but spin wait)
   If we are in a schedulable context (which the usleep_range() suggests
   we are) then why bother sleeping for the short delay, and
   spin-waiting for the longer delay? A bit of consistency seems to be
   needed here.

4. really needs to explain why it's necessary to repeatedly call the
   pcs_config() function at each get_state() if the link is down.

   Note that with the code the way it is, phy_power_on() will be
   repeatedly called, and at some point the "power_count" will overflow
   which would probably be bad. The counting in the regulator core will
   probably also overflow as well. So this is bad.

   Apart from the overflow issue, the only thing I can see that this
   achieves is to call the core of the pcs_config function. In the case
   of the lynxi, calling its pcs_config() repeatedly with the same
   parameters. Looking at pcs-mtk-lynxi.c, I can't see what this would
   achieve.

With the above issues dealt with, from the point of view of the lynxi /
sgmii code, the only things I can see that the wrapping achieves are:

a) when pcs_enable() is called, call phy_power_on(pextp)
b) when pcs_disable() is called, call phy_power_offpextp)
c) when pcs_config() is called, if the interface has changed:
   i)  call phy_reset() and assert/deassert the "sgmii" reset before
       calling the lynxi PCS
   ii) call phy_set_mode_ext(pextp) for the new interface mode after
       calling the lynxi PCS

I haven't picked through the usxgmii code completely, so I'm not
specifically commenting on it, although some of the above applies
there as well.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!