Add compatible to am65-cpsw driver for J721e CPSW9G, which contains 8
external ports and 1 internal host port.
Add support to power on and power off the SERDES PHY which is used by the
CPSW MAC.
=========
Changelog
=========
v5 -> v6:
1. Add member "serdes_phy" in struct "am65_cpsw_slave_data" to store the
SERDES PHY for each port, if present. This is done to cache the SERDES
PHY beforehand, without depending on devm_of_phy_get().
2. Rebase series on net-next tree.
v4 -> v5:
1. Update subject of all patches in the series to "PATCH net-next".
2. Rebase series on net-next tree.
v3 -> v4:
1. Fix subject of patch-1/3, updating it to:
"dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G support"
and collect Reviewed-by tag.
2. Rebase series on linux-next tree tagged: next-20221107.
v2 -> v3:
1. Run 'make DT_CHECKER_FLAGS=-m dt_binding_check' and fix errors and
warnings corresponding to the patch for:
Documentation/devicetree/bindings/net/ti,k3-am654-cpsw-nuss.yaml
with the latest dt-schema and yamllint.
v1 -> v2:
1. Drop all patches corresponding to SGMII mode. This is done since I do
not have a method to test SGMII in the standard mode which uses an
SGMII PHY. The previous series used SGMII in a fixed-link mode,
bypassing the SGMII PHY. I will post the SGMII patches in a future
series after testing them.
2. Drop all patches corresponding to fixed-link in the am65-cpsw driver.
This is done since PHYLINK takes care of fixed-link automatically and
there is no need to deal with fixed-link in a custom fashion.
3. Fix indentation errors in k3-am65-cpsw-nuss.yaml.
4. Remove the stale code which tries to power on and power off the CPSW
MAC's phy, since the CPSW MAC's phy driver does not support it.
5. Rename the function "am65_cpsw_init_phy()" to
"am65_cpsw_init_serdes_phy()", to indicate that the phy corresponds to
the SERDES.
6. Invoke "am65_cpsw_disable_serdes_phy()" as a part of the cleanup that
is associated with the "am65_cpsw_nuss_remove()" function.
v5:
https://lore.kernel.org/r/[email protected]/
v4:
https://lore.kernel.org/r/[email protected]/
v3:
https://lore.kernel.org/r/[email protected]/
v2:
https://lore.kernel.org/r/[email protected]/
v1:
https://lore.kernel.org/r/[email protected]/
Siddharth Vadapalli (3):
dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G support
net: ethernet: ti: am65-cpsw: Enable QSGMII mode for J721e CPSW9G
net: ethernet: ti: am65-cpsw: Add support for SERDES configuration
.../bindings/net/ti,k3-am654-cpsw-nuss.yaml | 33 +++++++-
drivers/net/ethernet/ti/am65-cpsw-nuss.c | 76 +++++++++++++++++++
drivers/net/ethernet/ti/am65-cpsw-nuss.h | 1 +
3 files changed, 106 insertions(+), 4 deletions(-)
--
2.25.1
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 <[email protected]>
---
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
Hello:
This series was applied to netdev/net-next.git (master)
by Paolo Abeni <[email protected]>:
On Wed, 4 Jan 2023 16:04:29 +0530 you wrote:
> Add compatible to am65-cpsw driver for J721e CPSW9G, which contains 8
> external ports and 1 internal host port.
>
> Add support to power on and power off the SERDES PHY which is used by the
> CPSW MAC.
>
> =========
> Changelog
> =========
> v5 -> v6:
> 1. Add member "serdes_phy" in struct "am65_cpsw_slave_data" to store the
> SERDES PHY for each port, if present. This is done to cache the SERDES
> PHY beforehand, without depending on devm_of_phy_get().
> 2. Rebase series on net-next tree.
>
> [...]
Here is the summary with links:
- [net-next,v6,1/3] dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G support
https://git.kernel.org/netdev/net-next/c/c85b53e32c8e
- [net-next,v6,2/3] net: ethernet: ti: am65-cpsw: Enable QSGMII mode for J721e CPSW9G
https://git.kernel.org/netdev/net-next/c/944131fa65d7
- [net-next,v6,3/3] net: ethernet: ti: am65-cpsw: Add support for SERDES configuration
https://git.kernel.org/netdev/net-next/c/dab2b265dd23
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hi Siddharth,
On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <[email protected]> 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 <[email protected]>
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 -- [email protected]
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 <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> wrote:
> On 17/01/23 19:25, Geert Uytterhoeven wrote:
> > On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <[email protected]> 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 <[email protected]>
> >
> > 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/[email protected]
> 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 -- [email protected]
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 <[email protected]> wrote:
>> On 17/01/23 19:25, Geert Uytterhoeven wrote:
>>> On Wed, Jan 4, 2023 at 11:37 AM Siddharth Vadapalli <[email protected]> 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 <[email protected]>
>>>
>>> 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/[email protected]
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/[email protected]
>
>>>> @@ -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.