On 11/6/2024 11:43 AM, Andrew Lunn wrote:
On Wed, Nov 06, 2024 at 11:16:37AM +0800, Lei Wei wrote:
On 11/1/2024 9:21 PM, Andrew Lunn wrote:
+static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
+ phy_interface_t interface)
+{
+ unsigned int val;
+ int ret;
+
+ /* Configure PCS interface mode */
+ switch (interface) {
+ case PHY_INTERFACE_MODE_SGMII:
+ /* Select Qualcomm SGMII AN mode */
+ ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
+ PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
+ PCS_MODE_SGMII);
How does Qualcomm SGMII AN mode differ from Cisco SGMII AN mode?
Qualcomm SGMII AN mode extends Cisco SGMII spec Revision 1.8 by adding pause
bit support in the SGMII word format. It re-uses two of the reserved bits
1..9 for this purpose. The PCS supports both Qualcomm SGMII AN and Cisco
SGMII AN modes.
Is Qualcomm SGMII AN actually needed? I assume it only works against a
Qualcomm PHY? What interoperability testing have you do against
non-Qualcomm PHYs?
I agree that using Cisco SGMII AN mode as default is more appropriate,
since is more commonly used with PHYs. I will make this change.
Qualcomm SGMII AN is an extension of top of Cisco SGMII AN (only
pause bits difference). So it is expected to be compatible with
non-Qualcomm PHYs which use Cisco SGMII AN.
+struct phylink_pcs *ipq_pcs_create(struct device_node *np)
+{
+ struct platform_device *pdev;
+ struct ipq_pcs_mii *qpcs_mii;
+ struct device_node *pcs_np;
+ struct ipq_pcs *qpcs;
+ int i, ret;
+ u32 index;
+
+ if (!of_device_is_available(np))
+ return ERR_PTR(-ENODEV);
+
+ if (of_property_read_u32(np, "reg", &index))
+ return ERR_PTR(-EINVAL);
+
+ if (index >= PCS_MAX_MII_NRS)
+ return ERR_PTR(-EINVAL);
+
+ pcs_np = of_get_parent(np);
+ if (!pcs_np)
+ return ERR_PTR(-ENODEV);
+
+ if (!of_device_is_available(pcs_np)) {
+ of_node_put(pcs_np);
+ return ERR_PTR(-ENODEV);
+ }
How have you got this far if the parent is not available?
This check can fail only if the parent node is disabled in the board DTS. I
think this error situation may not be caught earlier than this point.
However I agree, the above check is redundant, since this check is
immediately followed by a validity check on the 'pdev' of the parent node,
which should be able cover any such errors as well.
This was also because the driver does not work as i expected. I was
expecting the PCS driver to walk its own DT and instantiate the PCS
devices listed. If the parent is disabled, it is clearly not going to
start its own children. But it is in fact some other device which
walks the PCS DT blob, and as a result the child/parent relationship
is broken, a child could exist without its parent.
Currently the PCS driver walks the DT and instantiates the parent PCS
nodes during probe, where as the child PCS (the per-MII PCS instance) is
instantiated later by the network device that is associated with the MII.
Alternatively as you mention, we could instantiate the child PCS during
probe itself. The network driver when it comes up, can just issue a
'get' operation on the PCS driver, to retrieve the PCS phylink given the
PCS node associated with the MAC. Agree that this is architecturally
simpler for instantiating the PCS nodes, and the interaction between
network driver and PCS is simpler (single 'get_phylink_pcs' API exported
from PCS driver instead of PCS create/destroy API exported). The PCS
instances are freed up during platform device remove.
+ for (i = 0; i < PCS_MII_CLK_MAX; i++) {
+ qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
+ if (IS_ERR(qpcs_mii->clk[i])) {
+ dev_err(qpcs->dev,
+ "Failed to get MII %d interface clock %s\n",
+ index, pcs_mii_clk_name[i]);
+ goto err_clk_get;
+ }
+
+ ret = clk_prepare_enable(qpcs_mii->clk[i]);
+ if (ret) {
+ dev_err(qpcs->dev,
+ "Failed to enable MII %d interface clock %s\n",
+ index, pcs_mii_clk_name[i]);
+ goto err_clk_en;
+ }
+ }
Maybe devm_clk_bulk_get() etc will help you here? I've never actually
used them, so i don't know for sure.
We don't have a 'device' associated with the 'np', so we could not consider
using the "devm_clk_bulk_get()" API.
Another artefact of not have a child-parent relationship. I wounder if
it makes sense to change the architecture. Have the PCS driver
instantiate the PCS devices as its children. They then have a device
structure for calls like clk_bulk_get(), and a more normal
consumer/provider setup.
I think you may be suggesting to drop the child node usage in the DTS,
so that we can attach all the MII clocks to the single PCS node, to
facilitate usage of bulk get() API to retrieve the MII clocks for that
PCS. We reviewed this option, and believe that retaining the current
parent-child relationship is a better option instead. This is because
this option allows us the flexibility to enable/disable the MII channels
(child nodes) in the board DTS as per the ethernet/MII configuration
relevant for the board.
We would like to explain this using an example of SoC/board DTSI below.
For IPQ9574, port5 can be connected with PCS0 (PCS0: PSGMII, PCS1 not
used) or PCS1 (PCS0: QSGMII, PCS1: USXGMII).
IPQ9574 SoC DTSI for PCS0 (5 max MII channels) and PCS1:
--------------------------------------------------------
pcs0: ethernet-pcs@7a00000 {
clocks = <&gcc GCC_UNIPHY0_SYS_CLK>,
<&gcc GCC_UNIPHY0_AHB_CLK>;
clock-names = "sys",
"ahb";
status = "disabled";
pcs0_mii0: pcs-mii@0 {
reg = <0>;
status = "disabled";
};
......
pcs0_mii3: pcs-mii@3 {
reg = <3>;
status = "disabled";
};
pcs0_mii4: pcs-mii@3 {
reg = <4>;
status = "disabled";
};
};
pcs1: ethernet-pcs@7a10000 {
clocks = <&gcc GCC_UNIPHY1_SYS_CLK>,
<&gcc GCC_UNIPHY1_AHB_CLK>;
clock-names = "sys",
"ahb";
status = "disabled";
pcs1_mii0: pcs-mii@0 {
reg = <0>;
status = "disabled";
};
};
board1 DTS (PCS0 - QSGMII (port1 - port4), PCS1 USXGMII (port 5))
-----------------------------------------------------------------
&pcs0 {
status = "okay";
};
/* Enable only four MII channels for PCS0 for this board */
&pcs0_mii0 {
clocks = <&nsscc NSS_CC_UNIPHY_PORT1_RX_CLK>,
<&nsscc NSS_CC_UNIPHY_PORT1_TX_CLK>;
clock-names = "mii_rx",
"mii_tx";
status = "okay";
};
......
&pcs0_mii3 {
clocks = <&nsscc NSS_CC_UNIPHY_PORT4_RX_CLK>,
<&nsscc NSS_CC_UNIPHY_PORT4_TX_CLK>;
clock-names = "mii_rx",
"mii_tx";
status = "okay";
};
&pcs1 {
status = "okay";
};
&pcs1_mii0 {
clocks = <&nsscc NSS_CC_UNIPHY_PORT5_RX_CLK>,
<&nsscc NSS_CC_UNIPHY_PORT5_TX_CLK>;
clock-names = "mii_rx",
"mii_tx";
status = "okay";
};
board2 DTS: (PCS0 - PSGMII (port1 - port5), PCS1 - disabled):
-------------------------------------------------------------
&pcs0 {
status = "okay";
};
/* For PCS0, Enable all 5 MII channels for this board,
PCS1 is disabled */
&pcs0_mii0 {
clocks = <&nsscc NSS_CC_UNIPHY_PORT1_RX_CLK>,
<&nsscc NSS_CC_UNIPHY_PORT1_TX_CLK>;
clock-names = "mii_rx",
"mii_tx";
status = "okay";
};
......
&pcs0_mii4 {
clocks = <&nsscc NSS_CC_UNIPHY_PORT5_RX_CLK>,
<&nsscc NSS_CC_UNIPHY_PORT5_TX_CLK>;
clock-names = "mii_rx",
"mii_tx";
status = "okay";
};
If we drop the child node in DTS, then all MII clocks will have to be
combined with the SoC clocks (AHB/SYS) and added to the board DTS, which
may not be correct. Also, I think the child-parent relationship in DTS
will make it more clear to reflect the PCS-to--MII-channel relationship.
Kindly let us know if this approach is fine.
Andrew
