Message-ID: <Z6x1xD0krK0_eycB@shell.armlinux.org.uk>
Date: Wed, 12 Feb 2025 10:19:48 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Lei Wei <quic_leiwei@...cinc.com>, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, quic_kkumarcs@...cinc.com,
	quic_suruchia@...cinc.com, quic_pavir@...cinc.com,
	quic_linchen@...cinc.com, quic_luoj@...cinc.com,
	srinivas.kandagatla@...aro.org, bartosz.golaszewski@...aro.org,
	vsmuthu@....qualcomm.com, john@...ozen.org,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Subject: Re: [PATCH net-next v5 0/5] Add PCS support for Qualcomm IPQ9574 SoC

On Tue, Feb 11, 2025 at 07:59:34PM -0800, Jakub Kicinski wrote:
> On Fri, 7 Feb 2025 23:53:11 +0800 Lei Wei wrote:
> > The 'UNIPHY' PCS block in the Qualcomm IPQ9574 SoC provides Ethernet
> > PCS and SerDes functions. It supports 1Gbps mode PCS and 10-Gigabit
> > mode PCS (XPCS) functions, and supports various interface modes for
> > the connectivity between the Ethernet MAC and the external PHYs/Switch.
> > There are three UNIPHY (PCS) instances in IPQ9574, supporting the six
> > Ethernet ports.
> > 
> > This patch series adds base driver support for initializing the PCS,
> > and PCS phylink ops for managing the PCS modes/states. Support for
> > SGMII/QSGMII (PCS) and USXGMII (XPCS) modes is being added initially.
> > 
> > The Ethernet driver which handles the MAC operations will create the
> > PCS instances and phylink for the MAC, by utilizing the API exported
> > by this driver.
> > 
> > While support is being added initially for IPQ9574, the driver is
> > expected to be easily extendable later for other SoCs in the IPQ
> > family such as IPQ5332.
> 
> Could someone with PHY, or even, dare I say, phylink expertise
> take a look here?

I've not had the time, sorry. Looking at it now, I have lots of
questions over this.

1) clocks.

- Patch 2 provides clocks from this driver which are exported to the
  NSCCC block that are then used to provide the MII clocks.
- Patch 3 consumes clocks from the NSCCC block for use with each PCS.

Surely this leads to a circular dependency, where the MSCCC driver
can't get the clocks it needs until this driver has initialised, but
this driver can't get the clocks it needs for each PCS from the NSCCC
because the MSCCC driver needs this driver to initialise.

2) there's yet another open coded "_get" function for getting the
PCS given a DT node which is different from every other "_get"
function - this one checks the parent DT node has an appropriate
compatible whereas others don't. The whole poliferation of "_get"
methods that are specific to each PCS still needs solving, and I
still have the big question around what happens when the PCS driver
gets unbound - and whether that causes the kernel to oops. I'm also
not a fan of "look up the struct device and then get its driver data".
There is *no* locking over accessing the driver data.

3) doesn't populate supported_interfaces for the PCS - which would
make ipq_pcs_validate() unnecessary until patch 4 (but see 6 below.)

4)
"+       /* Nothing to do here as in-band autoneg mode is enabled
+        * by default for each PCS MII port."

"by default" doesn't matter - what if in-band is disabled and then
subsequently enabled.

5) there seems to be an open-coded decision about the clock rate but
there's also ipq_pcs_clk_rate_get() which seems to make the same
decision.

6) it seems this block has N PCS, but all PCS must operate in the same
mode (e.g. one PCS can't operate in SGMII mode, another in USXGMII
mode.) Currently, the last "config" wins over previous configs across
all interfaces. Is this the best solution? Should we be detecting
conflicting configurations? Unfortunately, pcs->supported_interfaces
can't really be changed after the PCS is being used, so I guess
any such restrictions would need to go in ipq_pcs_validate() which
should work fine - although it would mean that a MAC populating
its phylink_config->supported_interfaces using pcs->supported_interfaces
may end up with too many interface bits set.

(1), (2) and (6) are probably the major issues at the moment, and (2)
has been around for a while.

Given (1), I'm just left wondering whether this has been runtime
tested, and how the driver model's driver dependencies cope with it
if the NSCCC driver is both a clock consumer of/provider to this
driver.

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

Powered by blists - more mailing lists