On 2/28/2025 10:22 PM, Russell King (Oracle) wrote:
On Wed, Feb 19, 2025 at 06:46:57PM +0800, Lei Wei wrote:
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.
The PCS device in IPQ9574 chipset is built into the SoC chip and is not
pluggable. Also, the PCS driver module is not unloadable until the MAC
driver that depends on it is unloaded. Therefore, marking the driver
'.suppress_bind_attrs = true' to disable user unbind action may be good
enough to cover all possible scenarios of device going away for IPQ9574 PCS
driver.
What I am concerned about is the proliferation of these various PCS
specific "_get" methods. Where the PCS is looked up by firmware
reference, we should have a common way to do that, rather than all
these PCS specific ways.
I did start work on that, but I just haven't had the time to take it
forward. This is about as far as I'd got:
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index 4f7920618b90..0b670fee0757 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -1,6 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for Linux PCS drivers
+obj-$(CONFIG_PHYLINK) += pcs-core.o
+
pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-plat.o \
pcs-xpcs-nxp.o pcs-xpcs-wx.o
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 976e569feb70..1c5492dab00e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2483,6 +2483,15 @@ void phylink_pcs_change(struct phylink_pcs *pcs, bool up)
}
EXPORT_SYMBOL_GPL(phylink_pcs_change);
+/**
+ * phylink_pcs_remove() - notify phylink that a PCS is going away
+ * @pcs: PCS that is going away
+ */
+void phylink_pcs_remove(struct phylink_pcs *pcs)
+{
+
+}
+
static irqreturn_t phylink_link_handler(int irq, void *data)
{
struct phylink *pl = data;
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 071ed4683c8c..1e6b7ce0fa7a 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -1,6 +1,7 @@
#ifndef NETDEV_PCS_H
#define NETDEV_PCS_H
+#include <linux/list.h>
#include <linux/phy.h>
#include <linux/spinlock.h>
#include <linux/workqueue.h>
@@ -435,9 +436,11 @@ int mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
#endif
struct phylink_pcs_ops;
+struct pcs_lookup;
/**
* struct phylink_pcs - PHYLINK PCS instance
+ * @lookup: private member for PCS core management
* @supported_interfaces: describing which PHY_INTERFACE_MODE_xxx
* are supported by this PCS.
* @ops: a pointer to the &struct phylink_pcs_ops structure
@@ -455,6 +458,7 @@ struct phylink_pcs_ops;
* the PCS driver.
*/
struct phylink_pcs {
+ struct pcs_lookup *lookup;
DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
const struct phylink_pcs_ops *ops;
struct phylink *phylink;
@@ -692,6 +696,7 @@ int phylink_set_fixed_link(struct phylink *,
void phylink_mac_change(struct phylink *, bool up);
void phylink_pcs_change(struct phylink_pcs *, bool up);
+void phylink_pcs_remove(struct phylink_pcs *);
int phylink_pcs_pre_init(struct phylink *pl, struct phylink_pcs *pcs);
@@ -790,4 +795,11 @@ void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
void phylink_decode_usxgmii_word(struct phylink_link_state *state,
uint16_t lpa);
+
+/* PCS lookup */
+struct phylink_pcs *pcs_find(void *id);
+void pcs_remove(struct phylink_pcs *pcs);
+int pcs_add(struct phylink_pcs *pcs, void *id);
+int devm_pcs_add(struct device *dev, struct phylink_pcs *pcs, void *id);
+
#endif
The idea is that you add the device using whatever identifier you decide
(the pointer value is what's matched). For example, a fwnode. You can
then find it using pcs_find().
If it returns NULL, then it's not (yet) registered - if you know that it
should exist (e.g. because the fwnode is marked as available) then you
can return -EPROBE_DEFER or fail.
There is a hook present so phylink can do something on PCS removal -
that's still to be implemented with this. I envision keeping a list
of phylink instances, and walking that list to discover if any phylink
instances are currently using the PCS. If they are, then we can take
the link down.
Thanks for sharing the details about this, the approach looks correct.
Can you suggest whether we can go ahead with the current version of the
IPQ PCS driver, and update the driver later to use the common way, once
the infrastructure method is supported? Else (preferably) if the patch
for your change can be posted, I can modify the IPQ PCS driver patch to
use the common method and rebase on top of your patch. Please suggest.
I would like to clarify on the hardware supported configurations for the
UNIPHY PCS hardware instances. [Note: There are three instances of 'UNIPHY
PCS' in IPQ9574. However we take the example here for PCS0]
UNIPHY PCS0 --> pcs0_mii0..pcs0_mii4 (5 PCS MII channels maximum).
Possible combinations: QSGMII (4x 1 SGMII)
PSGMII (5 x 1 SGMII),
SGMII (1 x 1 SGMII)
USXGMII (1 x 1 USXGMII)
As we can see above, different PCS channels in a 'UNIPHY' PCS block working
in different PHY interface modes is not supported by the hardware. So, it
might not be necessary to detect that conflict. If the interface mode
changes from one to another, the same interface mode is applicable to all
the PCS channels that are associated with the UNIPHY PCS block.
Below is an example of a DTS configuration which depicts one board
configuration where one 'UNIPHY' (PCS0) is connected with a QCA8075 Quad
PHY, it has 4 MII channels enabled and connected with 4 PPE MAC ports, and
all the PCS MII channels are in QSGMII mode. For the 'UNIPHY' connected with
single SGMII or USXGMII PHY (PCS1), only one MII channel is enabled and
connected with one PPE MAC port.
PHY:
&mdio {
ethernet-phy-package@0 {
compatible = "qcom,qca8075-package";
#address-cells = <1>;
#size-cells = <0>;
reg = <0x10>;
qcom,package-mode = "qsgmii";
phy0: ethernet-phy@10 {
reg = <0x10>;
};
phy1: ethernet-phy@11 {
reg = <0x11>;
};
phy2: ethernet-phy@12 {
reg = <0x12>;
};
phy3: ethernet-phy@13 {
reg = <0x13>;
};
};
phy4: ethernet-phy@8 {
compatible ="ethernet-phy-ieee802.3-c45";
reg = <8>;
};
}
PCS:
pcs0: ethernet-pcs@7a00000 {
......
pcs0_mii0: pcs-mii@0 {
reg = <0>;
status = "enabled";
};
......
pcs0_mii3: pcs-mii@3 {
reg = <3>;
status = "enabled";
};
};
Given that this is a package of several PCS which have a global mode, I
think it would be a good idea to have a property like
"qcom,package-mode" which defines which of the four modes should be used
for all PCS.
Then the PCS driver initialises supported_interfaces for each of these
PCS to only contain that mode, thereby ensuring that unsupported
dissimilar modes can't be selected or the mode unexpectedly changed.
OK, I will add the "qcom,package-mode" property to restrict the
supported_interfaces for each of the MII PCS instances.
