| Message ID | 20250207-ipq_pcs_6-14_rc1-v5-0-be2ebec32921@quicinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add PCS support for Qualcomm IPQ9574 SoC | expand |
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?
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.
On 2/12/2025 6:19 PM, Russell King (Oracle) wrote: > 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. > Sorry for the delay in response. Below is a description of the dependencies between the PCS/NSSCC drivers during initialization time and how the clock relationships are set up. Based on this, there should not any issue due to circular dependency, but please let me know if any improvement is possible here given the hardware clock dependency. The module loading order is as follows: Step 1.) NSCC driver module Step 2.) PCS driver module Step 3.) Ethernet driver module The 'UNIPHY' PCS clocks (from Serdes to NSSCC) are not needed to be available at the time of registration of PCS MII clocks (NSSCC to PCS MII) by the NSSCC driver (Step 1). The PCS MII clocks is registered before 'UNIPHY' PCS clock is registered, since by default the parent is initialized to 'xo' clock. Below is the output of clock tree on the board before the PCS driver is loaded. xo-board-clk nss_cc_port1_rx_clk_src nss_cc_port1_rx_div_clk_src nss_cc_uniphy_port1_rx_clk nss_cc_port1_rx_clk The 'UNIPHY' PCS clock is later configured as a parent to the PCS MII clock at the time when the Ethernet and PCS drivers are enabled (step3) and the MAC links up. At link up time, the NSSCC driver sets the NSSCC port clock rate (by configuring the divider) based on the link speed, during which time the NSSCC port clock's parent is switched to 'UNIPHY' PCS clock. Below is the clock tree dump after this step. 7a00000.ethernet-pcs::rx_clk nss_cc_port1_rx_clk_src nss_cc_port1_rx_div_clk_src nss_cc_uniphy_port1_rx_clk nss_cc_port1_rx_clk > 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. To avoid looking up the device and getting its driver data (which is also seen in other PCS device drivers currently), a common infrastructure is certainly preferable for the longer term to have a consistent lookup. As far as I understand, the urgency for the common infrastructure for lookup is perhaps more to resolve the issue of hot-pluggable devices going away, and less for devices that do not support it. Also, the _get() API is only called once during MAC port initialization and never later, so if the device is not pluggable and unbind is not possible, there may not be any race concerns when accessing the driver data using the _get() API. Please let me know if this understanding is incorrect. > 3) doesn't populate supported_interfaces for the PCS - which would > make ipq_pcs_validate() unnecessary until patch 4 (but see 6 below.) > Agree, we will update the patch to advertise 'supported interfaces' and use the 'pcs_validate' op only for patch4 as you pointed (for filtering half duplex modes for USXGMII.). [The 'pcs_validate()' was suggested by you and added in the version 3 of this driver, and at that time, the pcs supported_interfaces is not introduced.] > 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. > OK, I will fix this function to handle both in-band neg enabled and disabled cases in next update. > 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. > I think you may be referring to both ipq_pcs_config_mode() and ipq_pcs_clk_rate_get() functions having the similar switch case to decide the clock rate based on the interface mode. I do agree, we can simplify this by saving the clock rate in ipq_pcs_config_mode() before the clk_set_rate() is called, and then simply returning this clock rate from the recalc_rate() op. > 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. > 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"; }; }; pcs1: ethernet-pcs@7a10000 { ...... pcs1_mii0: pcs-mii@0 { reg = <0>; status = "enabled"; }; }; MAC: port@1 { phy-mode = "qsgmii"; phy-handle = <&phy0>; pcs-handle = <&pcs0_mii0>; } port@2 { phy-mode = "qsgmii"; phy-handle = <&phy1>; pcs-handle = <&pcs0_mii1>; } port@3 { phy-mode = "qsgmii"; phy-handle = <&phy2>; pcs-handle = <&pcs0_mii2>; } port@4 { phy-mode = "qsgmii"; phy-handle = <&phy3>; pcs-handle = <&pcs0_mii3>; } port@5 { phy-mode = "usxgmii"; phy-handle = <&phy4>; pcs-handle = <&pcs1_mii0>; } > (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. > Yes, I have tested the PCS driver along with NSSCC driver and PPE Ethernet driver.
On 2/19/2025 6:46 PM, Lei Wei wrote: > > > On 2/12/2025 6:19 PM, Russell King (Oracle) wrote: >> 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. >> > > Sorry for the delay in response. Below is a description of the > dependencies between the PCS/NSSCC drivers during initialization time > and how the clock relationships are set up. Based on this, there should > not any issue due to circular dependency, but please let me know if any > improvement is possible here given the hardware clock dependency. The > module loading order is as follows: > > Step 1.) NSCC driver module > Step 2.) PCS driver module > Step 3.) Ethernet driver module > > The 'UNIPHY' PCS clocks (from Serdes to NSSCC) are not needed to be > available at the time of registration of PCS MII clocks (NSSCC to PCS > MII) by the NSSCC driver (Step 1). The PCS MII clocks is registered > before 'UNIPHY' PCS clock is registered, since by default the parent is > initialized to 'xo' clock. Below is the output of clock tree on the > board before the PCS driver is loaded. > > xo-board-clk > nss_cc_port1_rx_clk_src > nss_cc_port1_rx_div_clk_src > nss_cc_uniphy_port1_rx_clk > nss_cc_port1_rx_clk > > The 'UNIPHY' PCS clock is later configured as a parent to the PCS MII > clock at the time when the Ethernet and PCS drivers are enabled (step3) > and the MAC links up. At link up time, the NSSCC driver sets the NSSCC > port clock rate (by configuring the divider) based on the link speed, > during which time the NSSCC port clock's parent is switched to 'UNIPHY' > PCS clock. Below is the clock tree dump after this step. > > 7a00000.ethernet-pcs::rx_clk > nss_cc_port1_rx_clk_src > nss_cc_port1_rx_div_clk_src > nss_cc_uniphy_port1_rx_clk > nss_cc_port1_rx_clk > >> 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. > > To avoid looking up the device and getting its driver data (which is > also seen in other PCS device drivers currently), a common > infrastructure is certainly preferable for the longer term to have a > consistent lookup. As far as I understand, the urgency for the common > infrastructure for lookup is perhaps more to resolve the issue of hot- > pluggable devices going away, and less for devices that do not support it. > > Also, the _get() API is only called once during MAC port initialization > and never later, so if the device is not pluggable and unbind is not > possible, there may not be any race concerns when accessing the driver > data using the _get() API. Please let me know if this understanding is > incorrect. > >> 3) doesn't populate supported_interfaces for the PCS - which would >> make ipq_pcs_validate() unnecessary until patch 4 (but see 6 below.) >> > > Agree, we will update the patch to advertise 'supported interfaces' and > use the 'pcs_validate' op only for patch4 as you pointed (for filtering > half duplex modes for USXGMII.). > [The 'pcs_validate()' was suggested by you and added in the version 3 of > this driver, and at that time, the pcs supported_interfaces is not > introduced.] > >> 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. >> > > OK, I will fix this function to handle both in-band neg enabled and > disabled cases in next update. > >> 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. >> > > I think you may be referring to both ipq_pcs_config_mode() and > ipq_pcs_clk_rate_get() functions having the similar switch case to > decide the clock rate based on the interface mode. I do agree, we can > simplify this by saving the clock rate in ipq_pcs_config_mode() before > the clk_set_rate() is called, and then simply returning this clock rate > from the recalc_rate() op. > > >> 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. >> > > 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"; > }; > }; > > pcs1: ethernet-pcs@7a10000 { > ...... > > pcs1_mii0: pcs-mii@0 { > reg = <0>; > status = "enabled"; > }; > }; > > MAC: > port@1 { > phy-mode = "qsgmii"; > phy-handle = <&phy0>; > pcs-handle = <&pcs0_mii0>; > } > > port@2 { > phy-mode = "qsgmii"; > phy-handle = <&phy1>; > pcs-handle = <&pcs0_mii1>; > } > port@3 { > phy-mode = "qsgmii"; > phy-handle = <&phy2>; > pcs-handle = <&pcs0_mii2>; > } > port@4 { > phy-mode = "qsgmii"; > phy-handle = <&phy3>; > pcs-handle = <&pcs0_mii3>; > } > port@5 { > phy-mode = "usxgmii"; > phy-handle = <&phy4>; > pcs-handle = <&pcs1_mii0>; > } > >> (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. >> > > Yes, I have tested the PCS driver along with NSSCC driver and PPE > Ethernet driver. Hi Russell, Gentle reminder, to review my responses and provide your comments. Thank you in advance.
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.
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.
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. Signed-off-by: Lei Wei <quic_leiwei@quicinc.com> --- Changes in v5: - Add a comment in "ipq_pcs_enable" to note that the RX clock is not disabled as PHYLINK does not unwind the state back. - Add PCS device compatible string check for the device node in "ipq_pcs_get". - Link to v4: https://lore.kernel.org/r/20250108-ipq_pcs_net-next-v4-0-0de14cd2902b@quicinc.com Changes in v4: - Add "COMMON_CLK" to the Kconfig dependency option. - Optimize to avoid indentation in "ipq_pcs_config_usxgmii". - Remove the PCS config lock. - Add the "pcs_inband_caps" method. - Link to v3: https://lore.kernel.org/r/20241216-ipq_pcs_6-13_rc1-v3-0-3abefda0fc48@quicinc.com Changes in v3: - Remove the clk enabled check in "pcs_disable" method. - Add "pcs_validate" method to validate supported interface mode and duplex mode. - Use regmap_set_bits()/regmap_clear_bits() API where appropriate. - Collect Reviewed-by tag for dtbindings. - Link to v2: https://lore.kernel.org/r/20241204-ipq_pcs_rc1-v2-0-26155f5364a1@quicinc.com Changes in v2: - dtbindings updates a.) Rename dt-binding header file to match binding file name. b.) Drop unused labels and the redundant examples. c.) Rename "mii_rx"/"mii_tx" clock names to "rx"/"tx". - Rename "PCS_QCOM_IPQ" with specific name "PCS_QCOM_IPQ9574" in Kconfig. - Remove interface mode check for the PCS lock. - Use Cisco SGMII AN mode as default SGMII/QSGMII AN mode. - Instantiate MII PCS instances in probe and export "ipq_pcs_get" and "ipq_pcs_put" APIs. - Move MII RX and TX clock enable and disable to "pcs_enable" and "pcs_disable" methods. - Change "dev_dbg" to "dev_dbg_ratelimited" in "pcs_get_state" method. - Link to v1: https://lore.kernel.org/r/20241101-ipq_pcs_rc1-v1-0-fdef575620cf@quicinc.com --- Lei Wei (5): dt-bindings: net: pcs: Add Ethernet PCS for Qualcomm IPQ9574 SoC net: pcs: Add PCS driver for Qualcomm IPQ9574 SoC net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations net: pcs: qcom-ipq9574: Add USXGMII interface mode support MAINTAINERS: Add maintainer for Qualcomm IPQ9574 PCS driver .../bindings/net/pcs/qcom,ipq9574-pcs.yaml | 190 +++++ MAINTAINERS | 9 + drivers/net/pcs/Kconfig | 9 + drivers/net/pcs/Makefile | 1 + drivers/net/pcs/pcs-qcom-ipq9574.c | 884 +++++++++++++++++++++ include/dt-bindings/net/qcom,ipq9574-pcs.h | 15 + include/linux/pcs/pcs-qcom-ipq9574.h | 15 + 7 files changed, 1123 insertions(+) --- base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b change-id: 20250207-ipq_pcs_6-14_rc1-09216322b7ce Best regards,