| Message ID | 20230104103432.1126403-4-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | dab2b265dd23ef8fa7c49aeefb580918eb4ae207 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for QSGMII mode for J721e CPSW9G to am65-cpsw driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: rogerq@kernel.org |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 85 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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.
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];
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(+)