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