On 12/7/2024 12:28 AM, Russell King (Oracle) wrote:
On Sat, Dec 07, 2024 at 12:20:25AM +0800, Lei Wei wrote:
On 12/4/2024 11:28 PM, Russell King (Oracle) wrote:
On Wed, Dec 04, 2024 at 10:43:55PM +0800, Lei Wei wrote:
+static int ipq_pcs_enable(struct phylink_pcs *pcs)
+{
+ struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+ struct ipq_pcs *qpcs = qpcs_mii->qpcs;
+ int index = qpcs_mii->index;
+ int ret;
+
+ ret = clk_prepare_enable(qpcs_mii->rx_clk);
+ if (ret) {
+ dev_err(qpcs->dev, "Failed to enable MII %d RX clock\n", index);
+ return ret;
+ }
+
+ ret = clk_prepare_enable(qpcs_mii->tx_clk);
+ if (ret) {
+ dev_err(qpcs->dev, "Failed to enable MII %d TX clock\n", index);
+ clk_disable_unprepare(qpcs_mii->rx_clk);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void ipq_pcs_disable(struct phylink_pcs *pcs)
+{
+ struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+
+ if (__clk_is_enabled(qpcs_mii->rx_clk))
+ clk_disable_unprepare(qpcs_mii->rx_clk);
+
+ if (__clk_is_enabled(qpcs_mii->tx_clk))
+ clk_disable_unprepare(qpcs_mii->tx_clk);
Why do you need the __clk_is_enabled() calls here? Phylink should be
calling pcs_enable() once when the PCS when starting to use the PCS,
and then pcs_disable() when it stops using it - it won't call
pcs_disable() without a preceeding call to pcs_enable().
Are you seeing something different?
Yes, understand that phylink won't call pcs_disable() without a preceeding
call to pcs_enable(). However, the "clk_prepare_enable" may fail in the
pcs_enable() method, so I added the __clk_is_enabled() check in
pcs_disable() method. This is because the phylink_major_config() function
today does not interpret the return value of phylink_pcs_enable().
Right, because failure is essentially fatal in that path - we have no
context to return an error. I suppose we could stop processing at
that point, but then it brings up the question of how to unwind anything
we've already done, which is basically impossible at that point.
Sure, understand. I will remove the checks.
+static void ipq_pcs_get_state(struct phylink_pcs *pcs,
+ struct phylink_link_state *state)
+{
+ struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+ struct ipq_pcs *qpcs = qpcs_mii->qpcs;
+ int index = qpcs_mii->index;
+
+ switch (state->interface) {
+ case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_QSGMII:
+ ipq_pcs_get_state_sgmii(qpcs, index, state);
+ break;
+ default:
+ break;
...
+static int ipq_pcs_config(struct phylink_pcs *pcs,
+ unsigned int neg_mode,
+ phy_interface_t interface,
+ const unsigned long *advertising,
+ bool permit)
+{
+ struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+ struct ipq_pcs *qpcs = qpcs_mii->qpcs;
+ int index = qpcs_mii->index;
+
+ switch (interface) {
+ case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_QSGMII:
+ return ipq_pcs_config_sgmii(qpcs, index, neg_mode, interface);
+ default:
+ dev_err(qpcs->dev,
+ "Unsupported interface %s\n", phy_modes(interface));
+ return -EOPNOTSUPP;
+ };
+}
+
+static void ipq_pcs_link_up(struct phylink_pcs *pcs,
+ unsigned int neg_mode,
+ phy_interface_t interface,
+ int speed, int duplex)
+{
+ struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
+ struct ipq_pcs *qpcs = qpcs_mii->qpcs;
+ int index = qpcs_mii->index;
+ int ret;
+
+ switch (interface) {
+ case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_QSGMII:
+ ret = ipq_pcs_link_up_config_sgmii(qpcs, index,
+ neg_mode, speed);
+ break;
+ default:
+ dev_err(qpcs->dev,
+ "Unsupported interface %s\n", phy_modes(interface));
+ return;
+ }
So you only support SGMII and QSGMII. Rather than checking this in every
method implementation, instead provide a .pcs_validate method that
returns an error for unsupported interfaces please.
Yes, I can add the pcs_validate() method to validate the link
configurations. This will catch invalid interface mode during the PCS
initialization time, earlier than the pcs_config and pcs_link_up contexts.
But after of the PCS init, if at a later point the PHY interface mode
changes, it seems phylink today is not calling the pcs_validate() op to
validate the changed new interface mode at the time of "phylink_resolve".
... because by that time it's way too late. Phylink will have already
looked at what the PHY can do when the PHY is attached, and eliminated
any link modes that would cause an invalid configuration (provided
phylink knows what the PHY is capable of.)
However, that assumes phylink knows what the details are of the PCS,
which is dependent on the .pcs_validate method being implemented.
Yes, agree that pcs_validate() is necessary to be implemented and
Phylink will validate the PHY when the PHY is attached. I will implement
this method in the next update. Thanks for pointing to the details here.
We will also remove the debug error print for the 'default' case.
However, I would like to retain the switch statement since we have
different routines for SGMII/USXGMII modes, and we plan to add more
interfaces modes later when we enhance the driver for other IPQ SoC.
