| Message ID | 20240223192454.44789-2-asmaa@nvidia.com |
|---|---|
| State | New |
| Headers | show |
Hi Asmaa,
The email title doesn't seem entirely correct ("...v1 0/1..").
On Mon, Feb 26, 2024 at 2:35 AM Asmaa Mnebhi <asmaa@nvidia.com> wrote:
>
> BugLink: https://bugs.launchpad.net/bugs/2054845
>
> There is no external PHY connected to the OOB MAC on the Bobcat
> board and no access to the MDIO bus. The OOB MAC is directly connected
> to the Marvell switch. So support a "fake PHY" and simulate an MDIO bus.
>
> If "fixed-link" property is supported in the ACPI table, register the
> fixed PHY driver.
>
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> ---
> .../ethernet/mellanox/mlxbf_gige/mlxbf_gige.h | 2 -
> .../mellanox/mlxbf_gige/mlxbf_gige_main.c | 58 ++++++++++++++++---
> 2 files changed, 49 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h
> index a453b9cd9033..a85823a64d94 100644
> --- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h
> +++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h
> @@ -175,8 +175,6 @@ enum mlxbf_gige_res {
> int mlxbf_gige_mdio_probe(struct platform_device *pdev,
> struct mlxbf_gige *priv);
> void mlxbf_gige_mdio_remove(struct mlxbf_gige *priv);
> -irqreturn_t mlxbf_gige_mdio_handle_phy_interrupt(int irq, void *dev_id);
> -void mlxbf_gige_mdio_enable_phy_int(struct mlxbf_gige *priv);
The above change seems correct but please mention it in the patch description.
> void mlxbf_gige_set_mac_rx_filter(struct mlxbf_gige *priv,
> unsigned int index, u64 dmac);
> diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> index c0da9c05b12a..1f0e24e74e19 100644
> --- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> +++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c
> @@ -13,6 +13,7 @@
> #include <linux/iopoll.h>
> #include <linux/module.h>
> #include <linux/phy.h>
> +#include <linux/phy_fixed.h>
> #include <linux/platform_device.h>
> #include <linux/skbuff.h>
>
> @@ -364,6 +365,7 @@ static struct mlxbf_gige_link_cfg mlxbf_gige_link_cfgs[] = {
>
> static int mlxbf_gige_probe(struct platform_device *pdev)
> {
> + struct fixed_phy_status fphy_status = {};
> struct phy_device *phydev;
> struct net_device *netdev;
> struct mlxbf_gige *priv;
> @@ -428,16 +430,49 @@ static int mlxbf_gige_probe(struct platform_device *pdev)
> priv->rx_irq = platform_get_irq(pdev, MLXBF_GIGE_RECEIVE_PKT_INTR_IDX);
> priv->llu_plu_irq = platform_get_irq(pdev, MLXBF_GIGE_LLU_PLU_INTR_IDX);
>
> - phy_irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(&pdev->dev), "phy-gpios", 0);
> - if (phy_irq < 0) {
> - dev_err(&pdev->dev, "Error getting PHY irq. Use polling instead");
> + if (device_property_read_bool(&pdev->dev, "fixed-link")) {
> + fphy_status.link = 1;
> + err = device_property_read_u32(&pdev->dev, "full-duplex", &fphy_status.duplex);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to get duplex\n");
> + return -EINVAL;
> + }
> + err = device_property_read_u32(&pdev->dev, "speed", &fphy_status.speed);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to get speed\n");
> + return -EINVAL;
> + }
> + err = device_property_read_u32(&pdev->dev, "pause", &fphy_status.pause);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to get pause\n");
> + return -EINVAL;
> + }
> + err = device_property_read_u32(&pdev->dev, "asym-pause", &fphy_status.asym_pause);
> + if (err) {
> + dev_err(&pdev->dev, "Failed to get asym-pause\n");
> + return -EINVAL;
> + }
> +
> + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL);
> + if (IS_ERR(phydev)) {
> + dev_err(&pdev->dev, "Failed to register fixed PHY device\n");
> + err = PTR_ERR(phydev);
> + goto out;
Shouldn't the driver also do the cleanup ("goto out;") on ACPI table
read errors?
--
Best regards,
Bartlomiej
> + }
> +
> phy_irq = PHY_POLL;
> - }
> + } else {
> + phy_irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(&pdev->dev), "phy-gpios", 0);
> + if (phy_irq < 0) {
> + dev_err(&pdev->dev, "Error getting PHY irq. Use polling instead");
> + phy_irq = PHY_POLL;
> + }
>
> - phydev = phy_find_first(priv->mdiobus);
> - if (!phydev) {
> - err = -ENODEV;
> - goto out;
> + phydev = phy_find_first(priv->mdiobus);
> + if (!phydev) {
> + err = -ENODEV;
> + goto out;
> + }
> }
>
> addr = phydev->mdio.addr;
> @@ -474,9 +509,14 @@ static int mlxbf_gige_probe(struct platform_device *pdev)
> static int mlxbf_gige_remove(struct platform_device *pdev)
> {
> struct mlxbf_gige *priv = platform_get_drvdata(pdev);
> + struct phy_device *phydev = priv->netdev->phydev;
>
> unregister_netdev(priv->netdev);
> - phy_disconnect(priv->netdev->phydev);
> + phy_disconnect(phydev);
> +
> + if (phy_is_pseudo_fixed_link(phydev))
> + fixed_phy_unregister(phydev);
> +
> mlxbf_gige_mdio_remove(priv);
> platform_set_drvdata(pdev, NULL);
>
> Hi Asmaa, > > The email title doesn't seem entirely correct ("...v1 0/1.."). Will fix. > > > + if (device_property_read_bool(&pdev->dev, "fixed-link")) { > > + fphy_status.link = 1; > > + err = device_property_read_u32(&pdev->dev, "full-duplex", > &fphy_status.duplex); > > + if (err) { > > + dev_err(&pdev->dev, "Failed to get duplex\n"); > > + return -EINVAL; > > + } > > + err = device_property_read_u32(&pdev->dev, "speed", > &fphy_status.speed); > > + if (err) { > > + dev_err(&pdev->dev, "Failed to get speed\n"); > > + return -EINVAL; > > + } > > + err = device_property_read_u32(&pdev->dev, "pause", > &fphy_status.pause); > > + if (err) { > > + dev_err(&pdev->dev, "Failed to get pause\n"); > > + return -EINVAL; > > + } > > + err = device_property_read_u32(&pdev->dev, "asym-pause", > &fphy_status.asym_pause); > > + if (err) { > > + dev_err(&pdev->dev, "Failed to get asym-pause\n"); > > + return -EINVAL; > > + } > > + > > + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); > > + if (IS_ERR(phydev)) { > > + dev_err(&pdev->dev, "Failed to register fixed PHY device\n"); > > + err = PTR_ERR(phydev); > > + goto out; > > Shouldn't the driver also do the cleanup ("goto out;") on ACPI table read errors? > ACK. Will fix. Thank you!
diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h index a453b9cd9033..a85823a64d94 100644 --- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h +++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige.h @@ -175,8 +175,6 @@ enum mlxbf_gige_res { int mlxbf_gige_mdio_probe(struct platform_device *pdev, struct mlxbf_gige *priv); void mlxbf_gige_mdio_remove(struct mlxbf_gige *priv); -irqreturn_t mlxbf_gige_mdio_handle_phy_interrupt(int irq, void *dev_id); -void mlxbf_gige_mdio_enable_phy_int(struct mlxbf_gige *priv); void mlxbf_gige_set_mac_rx_filter(struct mlxbf_gige *priv, unsigned int index, u64 dmac); diff --git a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c index c0da9c05b12a..1f0e24e74e19 100644 --- a/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c +++ b/drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c @@ -13,6 +13,7 @@ #include <linux/iopoll.h> #include <linux/module.h> #include <linux/phy.h> +#include <linux/phy_fixed.h> #include <linux/platform_device.h> #include <linux/skbuff.h> @@ -364,6 +365,7 @@ static struct mlxbf_gige_link_cfg mlxbf_gige_link_cfgs[] = { static int mlxbf_gige_probe(struct platform_device *pdev) { + struct fixed_phy_status fphy_status = {}; struct phy_device *phydev; struct net_device *netdev; struct mlxbf_gige *priv; @@ -428,16 +430,49 @@ static int mlxbf_gige_probe(struct platform_device *pdev) priv->rx_irq = platform_get_irq(pdev, MLXBF_GIGE_RECEIVE_PKT_INTR_IDX); priv->llu_plu_irq = platform_get_irq(pdev, MLXBF_GIGE_LLU_PLU_INTR_IDX); - phy_irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(&pdev->dev), "phy-gpios", 0); - if (phy_irq < 0) { - dev_err(&pdev->dev, "Error getting PHY irq. Use polling instead"); + if (device_property_read_bool(&pdev->dev, "fixed-link")) { + fphy_status.link = 1; + err = device_property_read_u32(&pdev->dev, "full-duplex", &fphy_status.duplex); + if (err) { + dev_err(&pdev->dev, "Failed to get duplex\n"); + return -EINVAL; + } + err = device_property_read_u32(&pdev->dev, "speed", &fphy_status.speed); + if (err) { + dev_err(&pdev->dev, "Failed to get speed\n"); + return -EINVAL; + } + err = device_property_read_u32(&pdev->dev, "pause", &fphy_status.pause); + if (err) { + dev_err(&pdev->dev, "Failed to get pause\n"); + return -EINVAL; + } + err = device_property_read_u32(&pdev->dev, "asym-pause", &fphy_status.asym_pause); + if (err) { + dev_err(&pdev->dev, "Failed to get asym-pause\n"); + return -EINVAL; + } + + phydev = fixed_phy_register(PHY_POLL, &fphy_status, NULL); + if (IS_ERR(phydev)) { + dev_err(&pdev->dev, "Failed to register fixed PHY device\n"); + err = PTR_ERR(phydev); + goto out; + } + phy_irq = PHY_POLL; - } + } else { + phy_irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(&pdev->dev), "phy-gpios", 0); + if (phy_irq < 0) { + dev_err(&pdev->dev, "Error getting PHY irq. Use polling instead"); + phy_irq = PHY_POLL; + } - phydev = phy_find_first(priv->mdiobus); - if (!phydev) { - err = -ENODEV; - goto out; + phydev = phy_find_first(priv->mdiobus); + if (!phydev) { + err = -ENODEV; + goto out; + } } addr = phydev->mdio.addr; @@ -474,9 +509,14 @@ static int mlxbf_gige_probe(struct platform_device *pdev) static int mlxbf_gige_remove(struct platform_device *pdev) { struct mlxbf_gige *priv = platform_get_drvdata(pdev); + struct phy_device *phydev = priv->netdev->phydev; unregister_netdev(priv->netdev); - phy_disconnect(priv->netdev->phydev); + phy_disconnect(phydev); + + if (phy_is_pseudo_fixed_link(phydev)) + fixed_phy_unregister(phydev); + mlxbf_gige_mdio_remove(priv); platform_set_drvdata(pdev, NULL);