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