Use PHY framework APIs to initialize the SERDES PHY connected to CPSW MAC.

Define the functions am65_cpsw_disable_phy(), am65_cpsw_enable_phy(),
am65_cpsw_disable_serdes_phy() and am65_cpsw_enable_serdes_phy().

Add new member "serdes_phy" to struct "am65_cpsw_slave_data" to store the
SERDES PHY for each port, if it exists. Use it later while disabling the
SERDES PHY for each port.

Power on and initialize the SerDes PHY in am65_cpsw_nuss_init_slave_ports()
by invoking am65_cpsw_enable_serdes_phy().

Power off the SerDes PHY in am65_cpsw_nuss_remove() by invoking
am65_cpsw_disable_serdes_phy().

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/net/ethernet/ti/am65-cpsw-nuss.c | 68 ++++++++++++++++++++++++
 drivers/net/ethernet/ti/am65-cpsw-nuss.h |  1 +
 2 files changed, 69 insertions(+)

diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
index 06912363d5d5..1bd11166dc28 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
@@ -1416,6 +1416,68 @@ static const struct net_device_ops am65_cpsw_nuss_netdev_ops = {
 	.ndo_setup_tc           = am65_cpsw_qos_ndo_setup_tc,
 };
 
+static void am65_cpsw_disable_phy(struct phy *phy)
+{
+	phy_power_off(phy);
+	phy_exit(phy);
+}
+
+static int am65_cpsw_enable_phy(struct phy *phy)
+{
+	int ret;
+
+	ret = phy_init(phy);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_power_on(phy);
+	if (ret < 0) {
+		phy_exit(phy);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void am65_cpsw_disable_serdes_phy(struct am65_cpsw_common *common)
+{
+	struct am65_cpsw_port *port;
+	struct phy *phy;
+	int i;
+
+	for (i = 0; i < common->port_num; i++) {
+		port = &common->ports[i];
+		phy = port->slave.serdes_phy;
+		if (phy)
+			am65_cpsw_disable_phy(phy);
+	}
+}
+
+static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *port_np,
+				     struct am65_cpsw_port *port)
+{
+	const char *name = "serdes-phy";
+	struct phy *phy;
+	int ret;
+
+	phy = devm_of_phy_get(dev, port_np, name);
+	if (PTR_ERR(phy) == -ENODEV)
+		return 0;
+
+	/* Serdes PHY exists. Store it. */
+	port->slave.serdes_phy = phy;
+
+	ret =  am65_cpsw_enable_phy(phy);
+	if (ret < 0)
+		goto err_phy;
+
+	return 0;
+
+err_phy:
+	devm_phy_put(dev, phy);
+	return ret;
+}
+
 static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned int mode,
 				      const struct phylink_link_state *state)
 {
@@ -1959,6 +2021,11 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
 			goto of_node_put;
 		}
 
+		/* Initialize the Serdes PHY for the port */
+		ret = am65_cpsw_init_serdes_phy(dev, port_np, port);
+		if (ret)
+			return ret;
+
 		port->slave.mac_only =
 				of_property_read_bool(port_np, "ti,mac-only");
 
@@ -2878,6 +2945,7 @@ static int am65_cpsw_nuss_remove(struct platform_device *pdev)
 	 */
 	am65_cpsw_nuss_cleanup_ndev(common);
 	am65_cpsw_nuss_phylink_cleanup(common);
+	am65_cpsw_disable_serdes_phy(common);
 
 	of_platform_device_destroy(common->mdio_dev, NULL);
 
diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.h b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
index 4b75620f8d28..ed26768a6e51 100644
--- a/drivers/net/ethernet/ti/am65-cpsw-nuss.h
+++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.h
@@ -32,6 +32,7 @@ struct am65_cpsw_slave_data {
 	struct device_node		*phy_node;
 	phy_interface_t			phy_if;
 	struct phy			*ifphy;
+	struct phy			*serdes_phy;
 	bool				rx_pause;
 	bool				tx_pause;
 	u8				mac_addr[ETH_ALEN];
-- 
2.25.1

Hi Siddharth,

On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
> Use PHY framework APIs to initialize the SERDES PHY connected to CPSW MAC.
>
> Define the functions am65_cpsw_disable_phy(), am65_cpsw_enable_phy(),
> am65_cpsw_disable_serdes_phy() and am65_cpsw_enable_serdes_phy().
>
> Add new member "serdes_phy" to struct "am65_cpsw_slave_data" to store the
> SERDES PHY for each port, if it exists. Use it later while disabling the
> SERDES PHY for each port.
>
> Power on and initialize the SerDes PHY in am65_cpsw_nuss_init_slave_ports()
> by invoking am65_cpsw_enable_serdes_phy().
>
> Power off the SerDes PHY in am65_cpsw_nuss_remove() by invoking
> am65_cpsw_disable_serdes_phy().
>
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>

Thanks for your patch, which is now commit dab2b265dd23ef8f ("net:
ethernet: ti: am65-cpsw: Add support for SERDES configuration")
in net-next.

> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> @@ -1416,6 +1416,68 @@ static const struct net_device_ops am65_cpsw_nuss_netdev_ops = {
>         .ndo_setup_tc           = am65_cpsw_qos_ndo_setup_tc,
>  };
>
> +static void am65_cpsw_disable_phy(struct phy *phy)
> +{
> +       phy_power_off(phy);
> +       phy_exit(phy);
> +}
> +
> +static int am65_cpsw_enable_phy(struct phy *phy)
> +{
> +       int ret;
> +
> +       ret = phy_init(phy);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = phy_power_on(phy);
> +       if (ret < 0) {
> +               phy_exit(phy);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static void am65_cpsw_disable_serdes_phy(struct am65_cpsw_common *common)
> +{
> +       struct am65_cpsw_port *port;
> +       struct phy *phy;
> +       int i;
> +
> +       for (i = 0; i < common->port_num; i++) {
> +               port = &common->ports[i];
> +               phy = port->slave.serdes_phy;
> +               if (phy)
> +                       am65_cpsw_disable_phy(phy);
> +       }
> +}
> +
> +static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *port_np,
> +                                    struct am65_cpsw_port *port)
> +{
> +       const char *name = "serdes-phy";
> +       struct phy *phy;
> +       int ret;
> +
> +       phy = devm_of_phy_get(dev, port_np, name);
> +       if (PTR_ERR(phy) == -ENODEV)
> +               return 0;
> +
> +       /* Serdes PHY exists. Store it. */

"phy" may be a different error here (e.g. -EPROBE_DEFER)...

> +       port->slave.serdes_phy = phy;
> +
> +       ret =  am65_cpsw_enable_phy(phy);

... so it will crash when dereferencing phy in phy_init().

I think you want to add an extra check above:

    if (IS_ERR(phy))
            return PTR_ERR(phy);

> +       if (ret < 0)
> +               goto err_phy;
> +
> +       return 0;
> +
> +err_phy:
> +       devm_phy_put(dev, phy);
> +       return ret;
> +}
> +
>  static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned int mode,
>                                       const struct phylink_link_state *state)
>  {
> @@ -1959,6 +2021,11 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)

Right out of context we have:

                port->slave.ifphy = devm_of_phy_get(dev, port_np, NULL);
                if (IS_ERR(port->slave.ifphy)) {
                        ret = PTR_ERR(port->slave.ifphy);
                        dev_err(dev, "%pOF error retrieving port phy: %d\n",
                                port_np, ret);

So if there is only one PHY (named "serdes-phy") in DT, it will be
used for both ifphy and serdes_phy. Is that intentional?

>                         goto of_node_put;
>                 }
>
> +               /* Initialize the Serdes PHY for the port */
> +               ret = am65_cpsw_init_serdes_phy(dev, port_np, port);
> +               if (ret)
> +                       return ret;
> +
>                 port->slave.mac_only =
>                                 of_property_read_bool(port_np, "ti,mac-only");
>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Hello Geert,

Thank you for reviewing the patch.

On 17/01/23 19:25, Geert Uytterhoeven wrote:
> Hi Siddharth,
> 
> On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
>> Use PHY framework APIs to initialize the SERDES PHY connected to CPSW MAC.
>>
>> Define the functions am65_cpsw_disable_phy(), am65_cpsw_enable_phy(),
>> am65_cpsw_disable_serdes_phy() and am65_cpsw_enable_serdes_phy().
>>
>> Add new member "serdes_phy" to struct "am65_cpsw_slave_data" to store the
>> SERDES PHY for each port, if it exists. Use it later while disabling the
>> SERDES PHY for each port.
>>
>> Power on and initialize the SerDes PHY in am65_cpsw_nuss_init_slave_ports()
>> by invoking am65_cpsw_enable_serdes_phy().
>>
>> Power off the SerDes PHY in am65_cpsw_nuss_remove() by invoking
>> am65_cpsw_disable_serdes_phy().
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> 
> Thanks for your patch, which is now commit dab2b265dd23ef8f ("net:
> ethernet: ti: am65-cpsw: Add support for SERDES configuration")
> in net-next.
> 
>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> @@ -1416,6 +1416,68 @@ static const struct net_device_ops am65_cpsw_nuss_netdev_ops = {
>>         .ndo_setup_tc           = am65_cpsw_qos_ndo_setup_tc,
>>  };
>>
>> +static void am65_cpsw_disable_phy(struct phy *phy)
>> +{
>> +       phy_power_off(phy);
>> +       phy_exit(phy);
>> +}
>> +
>> +static int am65_cpsw_enable_phy(struct phy *phy)
>> +{
>> +       int ret;
>> +
>> +       ret = phy_init(phy);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ret = phy_power_on(phy);
>> +       if (ret < 0) {
>> +               phy_exit(phy);
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static void am65_cpsw_disable_serdes_phy(struct am65_cpsw_common *common)
>> +{
>> +       struct am65_cpsw_port *port;
>> +       struct phy *phy;
>> +       int i;
>> +
>> +       for (i = 0; i < common->port_num; i++) {
>> +               port = &common->ports[i];
>> +               phy = port->slave.serdes_phy;
>> +               if (phy)
>> +                       am65_cpsw_disable_phy(phy);
>> +       }
>> +}
>> +
>> +static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *port_np,
>> +                                    struct am65_cpsw_port *port)
>> +{
>> +       const char *name = "serdes-phy";
>> +       struct phy *phy;
>> +       int ret;
>> +
>> +       phy = devm_of_phy_get(dev, port_np, name);
>> +       if (PTR_ERR(phy) == -ENODEV)
>> +               return 0;
>> +
>> +       /* Serdes PHY exists. Store it. */
> 
> "phy" may be a different error here (e.g. -EPROBE_DEFER)...

The Serdes is automatically configured for multi-link protocol (Example: PCIe +
QSGMII) by the Serdes driver, due to which it is not necessary to invoke the
Serdes configuration via phy_init(). However, for single-link protocol (Example:
Serdes has to be configured only for SGMII), the Serdes driver doesn't configure
the Serdes unless requested. For this case, the am65-cpsw driver explicitly
invokes phy_init() for the Serdes to be configured, by looking up the optional
device-tree phy named "serdes-phy". For this reason, the above section of code
is actually emulating a non-existent "devm_of_phy_optional_get()". The
"devm_of_phy_optional_get()" function is similar to the
"devm_phy_optional_get()" function in the sense that the "serdes-phy" phy in the
device-tree is optional and it is not truly an error if the property isn't present.

Thank you for pointing out that if the Serdes driver is built as a module and
the am65-cpsw driver runs first, then the "phy" returned for "serdes-phy" will
be "-EPROBE_DEFER".

> 
>> +       port->slave.serdes_phy = phy;
>> +
>> +       ret =  am65_cpsw_enable_phy(phy);
> 
> ... so it will crash when dereferencing phy in phy_init().
> 
> I think you want to add an extra check above:
> 
>     if (IS_ERR(phy))
>             return PTR_ERR(phy);

Please let me know if posting a "Fixes" patch for fixing this net-next commit is
the right process to address this.

> 
>> +       if (ret < 0)
>> +               goto err_phy;
>> +
>> +       return 0;
>> +
>> +err_phy:
>> +       devm_phy_put(dev, phy);
>> +       return ret;
>> +}
>> +
>>  static void am65_cpsw_nuss_mac_config(struct phylink_config *config, unsigned int mode,
>>                                       const struct phylink_link_state *state)
>>  {
>> @@ -1959,6 +2021,11 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
> 
> Right out of context we have:
> 
>                 port->slave.ifphy = devm_of_phy_get(dev, port_np, NULL);
>                 if (IS_ERR(port->slave.ifphy)) {
>                         ret = PTR_ERR(port->slave.ifphy);
>                         dev_err(dev, "%pOF error retrieving port phy: %d\n",
>                                 port_np, ret);
> 
> So if there is only one PHY (named "serdes-phy") in DT, it will be
> used for both ifphy and serdes_phy. Is that intentional?

The PHY corresponding to "ifphy" is meant to be the CPSW MAC's PHY and not the
Serdes PHY. The CPSW MAC's PHY is configured by the
drivers/phy/ti/phy-gmii-sel.c driver and this is NOT an optional PHY, unlike the
Serdes PHY. Therefore, it is assumed that the CPSW MAC's PHY is always provided
in the device-tree, while the Serdes PHY is optional, depending on whether the
Serdes is being configured for single-link protocol or multi-link protocol.
Please let me know if this appears to be an issue and I will fix it based on
your suggestion.

Regards,
Siddharth.

Hi Siddarth,

On Wed, Jan 18, 2023 at 6:48 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
> On 17/01/23 19:25, Geert Uytterhoeven wrote:
> > On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
> >> Use PHY framework APIs to initialize the SERDES PHY connected to CPSW MAC.
> >>
> >> Define the functions am65_cpsw_disable_phy(), am65_cpsw_enable_phy(),
> >> am65_cpsw_disable_serdes_phy() and am65_cpsw_enable_serdes_phy().
> >>
> >> Add new member "serdes_phy" to struct "am65_cpsw_slave_data" to store the
> >> SERDES PHY for each port, if it exists. Use it later while disabling the
> >> SERDES PHY for each port.
> >>
> >> Power on and initialize the SerDes PHY in am65_cpsw_nuss_init_slave_ports()
> >> by invoking am65_cpsw_enable_serdes_phy().
> >>
> >> Power off the SerDes PHY in am65_cpsw_nuss_remove() by invoking
> >> am65_cpsw_disable_serdes_phy().
> >>
> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> >
> > Thanks for your patch, which is now commit dab2b265dd23ef8f ("net:
> > ethernet: ti: am65-cpsw: Add support for SERDES configuration")
> > in net-next.
> >
> >> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> >> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c

> >> +static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *port_np,
> >> +                                    struct am65_cpsw_port *port)
> >> +{
> >> +       const char *name = "serdes-phy";
> >> +       struct phy *phy;
> >> +       int ret;
> >> +
> >> +       phy = devm_of_phy_get(dev, port_np, name);
> >> +       if (PTR_ERR(phy) == -ENODEV)
> >> +               return 0;
> >> +
> >> +       /* Serdes PHY exists. Store it. */
> >
> > "phy" may be a different error here (e.g. -EPROBE_DEFER)...
>
> The Serdes is automatically configured for multi-link protocol (Example: PCIe +
> QSGMII) by the Serdes driver, due to which it is not necessary to invoke the
> Serdes configuration via phy_init(). However, for single-link protocol (Example:
> Serdes has to be configured only for SGMII), the Serdes driver doesn't configure
> the Serdes unless requested. For this case, the am65-cpsw driver explicitly
> invokes phy_init() for the Serdes to be configured, by looking up the optional
> device-tree phy named "serdes-phy". For this reason, the above section of code
> is actually emulating a non-existent "devm_of_phy_optional_get()". The
> "devm_of_phy_optional_get()" function is similar to the
> "devm_phy_optional_get()" function in the sense that the "serdes-phy" phy in the
> device-tree is optional and it is not truly an error if the property isn't present.

Yeah, I noticed while adding devm_phy_optional_get(), and looking for
possible users.
See "[PATCH treewide 0/7] phy: Add devm_of_phy_optional_get() helper"
https://lore.kernel.org/all/cover.1674036164.git.geert+renesas@glider.be

> Thank you for pointing out that if the Serdes driver is built as a module and
> the am65-cpsw driver runs first, then the "phy" returned for "serdes-phy" will
> be "-EPROBE_DEFER".
>
> >
> >> +       port->slave.serdes_phy = phy;
> >> +
> >> +       ret =  am65_cpsw_enable_phy(phy);
> >
> > ... so it will crash when dereferencing phy in phy_init().
> >
> > I think you want to add an extra check above:
> >
> >     if (IS_ERR(phy))
> >             return PTR_ERR(phy);
>
> Please let me know if posting a "Fixes" patch for fixing this net-next commit is
> the right process to address this.

I think it is, as devm_of_phy_optional_get() might not make it in time.

> >> @@ -1959,6 +2021,11 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
> >
> > Right out of context we have:
> >
> >                 port->slave.ifphy = devm_of_phy_get(dev, port_np, NULL);
> >                 if (IS_ERR(port->slave.ifphy)) {
> >                         ret = PTR_ERR(port->slave.ifphy);
> >                         dev_err(dev, "%pOF error retrieving port phy: %d\n",
> >                                 port_np, ret);
> >
> > So if there is only one PHY (named "serdes-phy") in DT, it will be
> > used for both ifphy and serdes_phy. Is that intentional?
>
> The PHY corresponding to "ifphy" is meant to be the CPSW MAC's PHY and not the
> Serdes PHY. The CPSW MAC's PHY is configured by the
> drivers/phy/ti/phy-gmii-sel.c driver and this is NOT an optional PHY, unlike the
> Serdes PHY. Therefore, it is assumed that the CPSW MAC's PHY is always provided
> in the device-tree, while the Serdes PHY is optional, depending on whether the
> Serdes is being configured for single-link protocol or multi-link protocol.
> Please let me know if this appears to be an issue and I will fix it based on
> your suggestion.

Hence this should be documented in the DT bindings. Please document
there can be 1 or 2 phys, with an optional "phys-names" property,
listing "ifphy" and "serdes-phy" (the DT people might request a rename).

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Hello Geert,

On 18/01/23 15:57, Geert Uytterhoeven wrote:
> Hi Siddarth,
> 
> On Wed, Jan 18, 2023 at 6:48 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
>> On 17/01/23 19:25, Geert Uytterhoeven wrote:
>>> On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <s-vadapalli@ti.com> wrote:
>>>> Use PHY framework APIs to initialize the SERDES PHY connected to CPSW MAC.
>>>>
>>>> Define the functions am65_cpsw_disable_phy(), am65_cpsw_enable_phy(),
>>>> am65_cpsw_disable_serdes_phy() and am65_cpsw_enable_serdes_phy().
>>>>
>>>> Add new member "serdes_phy" to struct "am65_cpsw_slave_data" to store the
>>>> SERDES PHY for each port, if it exists. Use it later while disabling the
>>>> SERDES PHY for each port.
>>>>
>>>> Power on and initialize the SerDes PHY in am65_cpsw_nuss_init_slave_ports()
>>>> by invoking am65_cpsw_enable_serdes_phy().
>>>>
>>>> Power off the SerDes PHY in am65_cpsw_nuss_remove() by invoking
>>>> am65_cpsw_disable_serdes_phy().
>>>>
>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>>
>>> Thanks for your patch, which is now commit dab2b265dd23ef8f ("net:
>>> ethernet: ti: am65-cpsw: Add support for SERDES configuration")
>>> in net-next.
>>>
>>>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>>>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
> 
>>>> +static int am65_cpsw_init_serdes_phy(struct device *dev, struct device_node *port_np,
>>>> +                                    struct am65_cpsw_port *port)
>>>> +{
>>>> +       const char *name = "serdes-phy";
>>>> +       struct phy *phy;
>>>> +       int ret;
>>>> +
>>>> +       phy = devm_of_phy_get(dev, port_np, name);
>>>> +       if (PTR_ERR(phy) == -ENODEV)
>>>> +               return 0;
>>>> +
>>>> +       /* Serdes PHY exists. Store it. */
>>>
>>> "phy" may be a different error here (e.g. -EPROBE_DEFER)...
>>
>> The Serdes is automatically configured for multi-link protocol (Example: PCIe +
>> QSGMII) by the Serdes driver, due to which it is not necessary to invoke the
>> Serdes configuration via phy_init(). However, for single-link protocol (Example:
>> Serdes has to be configured only for SGMII), the Serdes driver doesn't configure
>> the Serdes unless requested. For this case, the am65-cpsw driver explicitly
>> invokes phy_init() for the Serdes to be configured, by looking up the optional
>> device-tree phy named "serdes-phy". For this reason, the above section of code
>> is actually emulating a non-existent "devm_of_phy_optional_get()". The
>> "devm_of_phy_optional_get()" function is similar to the
>> "devm_phy_optional_get()" function in the sense that the "serdes-phy" phy in the
>> device-tree is optional and it is not truly an error if the property isn't present.
> 
> Yeah, I noticed while adding devm_phy_optional_get(), and looking for
> possible users.
> See "[PATCH treewide 0/7] phy: Add devm_of_phy_optional_get() helper"
> https://lore.kernel.org/all/cover.1674036164.git.geert+renesas@glider.be

Thank you for working on this.

> 
>> Thank you for pointing out that if the Serdes driver is built as a module and
>> the am65-cpsw driver runs first, then the "phy" returned for "serdes-phy" will
>> be "-EPROBE_DEFER".
>>
>>>
>>>> +       port->slave.serdes_phy = phy;
>>>> +
>>>> +       ret =  am65_cpsw_enable_phy(phy);
>>>
>>> ... so it will crash when dereferencing phy in phy_init().
>>>
>>> I think you want to add an extra check above:
>>>
>>>     if (IS_ERR(phy))
>>>             return PTR_ERR(phy);
>>
>> Please let me know if posting a "Fixes" patch for fixing this net-next commit is
>> the right process to address this.
> 
> I think it is, as devm_of_phy_optional_get() might not make it in time.

I posted the patch at:
https://lore.kernel.org/r/20230118112136.213061-1-s-vadapalli@ti.com

> 
>>>> @@ -1959,6 +2021,11 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
>>>
>>> Right out of context we have:
>>>
>>>                 port->slave.ifphy = devm_of_phy_get(dev, port_np, NULL);
>>>                 if (IS_ERR(port->slave.ifphy)) {
>>>                         ret = PTR_ERR(port->slave.ifphy);
>>>                         dev_err(dev, "%pOF error retrieving port phy: %d\n",
>>>                                 port_np, ret);
>>>
>>> So if there is only one PHY (named "serdes-phy") in DT, it will be
>>> used for both ifphy and serdes_phy. Is that intentional?
>>
>> The PHY corresponding to "ifphy" is meant to be the CPSW MAC's PHY and not the
>> Serdes PHY. The CPSW MAC's PHY is configured by the
>> drivers/phy/ti/phy-gmii-sel.c driver and this is NOT an optional PHY, unlike the
>> Serdes PHY. Therefore, it is assumed that the CPSW MAC's PHY is always provided
>> in the device-tree, while the Serdes PHY is optional, depending on whether the
>> Serdes is being configured for single-link protocol or multi-link protocol.
>> Please let me know if this appears to be an issue and I will fix it based on
>> your suggestion.
> 
> Hence this should be documented in the DT bindings. Please document
> there can be 1 or 2 phys, with an optional "phys-names" property,
> listing "ifphy" and "serdes-phy" (the DT people might request a rename).

I will work on this.

Regards,
Siddharth.