From: Geert Uytterhoeven <geert@linux-m68k.org> To: Siddharth Vadapalli <s-vadapalli@ti.com> Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, robh+dt@kernel.org, krzysztof.kozlowski@linaro.org, krzysztof.kozlowski+dt@linaro.org, linux@armlinux.org.uk, vladimir.oltean@nxp.com, vigneshr@ti.com, nsekhar@ti.com, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, srk@ti.com Subject: Re: [PATCH net-next v6 3/3] net: ethernet: ti: am65-cpsw: Add support for SERDES configuration Date: Tue, 17 Jan 2023 14:55:49 +0100 [thread overview] Message-ID: <CAMuHMdWiXu9OJxH4mRnneC3jhqTEcYXek3kbr7svhJ3cnPPwcw@mail.gmail.com> (raw) In-Reply-To: <20230104103432.1126403-4-s-vadapalli@ti.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Geert Uytterhoeven <geert@linux-m68k.org> To: Siddharth Vadapalli <s-vadapalli@ti.com> Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, robh+dt@kernel.org, krzysztof.kozlowski@linaro.org, krzysztof.kozlowski+dt@linaro.org, linux@armlinux.org.uk, vladimir.oltean@nxp.com, vigneshr@ti.com, nsekhar@ti.com, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, srk@ti.com Subject: Re: [PATCH net-next v6 3/3] net: ethernet: ti: am65-cpsw: Add support for SERDES configuration Date: Tue, 17 Jan 2023 14:55:49 +0100 [thread overview] Message-ID: <CAMuHMdWiXu9OJxH4mRnneC3jhqTEcYXek3kbr7svhJ3cnPPwcw@mail.gmail.com> (raw) In-Reply-To: <20230104103432.1126403-4-s-vadapalli@ti.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2023-01-17 13:57 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-01-04 10:34 [PATCH net-next v6 0/3] Add support for QSGMII mode for J721e CPSW9G to am65-cpsw driver Siddharth Vadapalli 2023-01-04 10:34 ` Siddharth Vadapalli 2023-01-04 10:34 ` [PATCH net-next v6 1/3] dt-bindings: net: ti: k3-am654-cpsw-nuss: Add J721e CPSW9G support Siddharth Vadapalli 2023-01-04 10:34 ` Siddharth Vadapalli 2023-01-17 13:45 ` Geert Uytterhoeven 2023-01-17 13:45 ` Geert Uytterhoeven 2023-01-17 17:17 ` Krzysztof Kozlowski 2023-01-17 17:17 ` Krzysztof Kozlowski 2023-01-18 17:21 ` Raghavendra, Vignesh 2023-01-18 17:21 ` Raghavendra, Vignesh 2023-01-04 10:34 ` [PATCH net-next v6 2/3] net: ethernet: ti: am65-cpsw: Enable QSGMII mode for J721e CPSW9G Siddharth Vadapalli 2023-01-04 10:34 ` Siddharth Vadapalli 2023-01-04 10:34 ` [PATCH net-next v6 3/3] net: ethernet: ti: am65-cpsw: Add support for SERDES configuration Siddharth Vadapalli 2023-01-04 10:34 ` Siddharth Vadapalli 2023-01-17 13:55 ` Geert Uytterhoeven [this message] 2023-01-17 13:55 ` Geert Uytterhoeven 2023-01-18 5:47 ` Siddharth Vadapalli 2023-01-18 5:47 ` Siddharth Vadapalli 2023-01-18 10:27 ` Geert Uytterhoeven 2023-01-18 10:27 ` Geert Uytterhoeven 2023-01-18 11:27 ` Siddharth Vadapalli 2023-01-18 11:27 ` Siddharth Vadapalli 2023-01-05 11:30 ` [PATCH net-next v6 0/3] Add support for QSGMII mode for J721e CPSW9G to am65-cpsw driver patchwork-bot+netdevbpf 2023-01-05 11:30 ` patchwork-bot+netdevbpf
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAMuHMdWiXu9OJxH4mRnneC3jhqTEcYXek3kbr7svhJ3cnPPwcw@mail.gmail.com \ --to=geert@linux-m68k.org \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=edumazet@google.com \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=krzysztof.kozlowski@linaro.org \ --cc=kuba@kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=netdev@vger.kernel.org \ --cc=nsekhar@ti.com \ --cc=pabeni@redhat.com \ --cc=robh+dt@kernel.org \ --cc=s-vadapalli@ti.com \ --cc=srk@ti.com \ --cc=vigneshr@ti.com \ --cc=vladimir.oltean@nxp.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.