Re: [PATCH 2/3] net: hisilicon: new hip04 MDIO driver

2014-03-18 1:40 GMT-07:00 Zhangfei Gao [off-list ref]:

This does not look correct, you should iterate over all possible PHYs:
PHY_MAX_ADDR instead of hardcoding the loop to 2.

I think we might want to remove the mdio bus reset callback in general
as the PHY library should already take care of software resetting the
PHY to put it in a sane state, as well as waiting for the appropriate
delay before using, unlike here, where you do not poll for BMCR_RESET
to be cleared by the PHY.

+       return 0;
+}
+
+static int hip04_mdio_probe(struct platform_device *pdev)
+{
+       struct resource *r;
+       struct mii_bus *bus;
+       struct hip04_mdio_priv *priv;
+       int ret;
+
+       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       if (!r) {
+               dev_err(&pdev->dev, "No SMI register address given\n");
+               return -ENODEV;
+       }
+
+       bus = mdiobus_alloc_size(sizeof(struct hip04_mdio_priv));
+       if (!bus) {
+               dev_err(&pdev->dev, "Cannot allocate MDIO bus\n");
+               return -ENOMEM;
+       }
+
+       bus->name = "hip04_mdio_bus";
+       bus->read = hip04_mdio_read;
+       bus->write = hip04_mdio_write;
+       bus->reset = hip04_mdio_reset;
+       snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii",
+                dev_name(&pdev->dev));
+       bus->parent = &pdev->dev;
+       priv = bus->priv;
+       priv->base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
+       if (!priv->base) {
+               dev_err(&pdev->dev, "Unable to remap SMI register\n");
+               ret = -ENODEV;
+               goto out_mdio;
+       }
+
+       ret = of_mdiobus_register(bus, pdev->dev.of_node);
+       if (ret < 0) {
+               dev_err(&pdev->dev, "Cannot register MDIO bus (%d)\n", ret);
+               goto out_mdio;
+       }
+
+       platform_set_drvdata(pdev, bus);
+
+       return 0;
+
+out_mdio:
+       mdiobus_free(bus);
+       return ret;
+}
+
+static int hip04_mdio_remove(struct platform_device *pdev)
+{
+       struct mii_bus *bus = platform_get_drvdata(pdev);
+
+       mdiobus_unregister(bus);
+       mdiobus_free(bus);
+
+       return 0;
+}
+
+static const struct of_device_id hip04_mdio_match[] = {
+       { .compatible = "hisilicon,hip04-mdio" },
+       { }
+};
+MODULE_DEVICE_TABLE(of, hip04_mdio_match);
+
+static struct platform_driver hip04_mdio_driver = {
+       .probe = hip04_mdio_probe,
+       .remove = hip04_mdio_remove,
+       .driver = {
+               .name = "hip04-mdio",
+               .owner = THIS_MODULE,
+               .of_match_table = hip04_mdio_match,
+       },
+};
+
+module_platform_driver(hip04_mdio_driver);
+
+MODULE_DESCRIPTION("HISILICON P04 MDIO interface driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:hip04-mdio");
--
1.7.9.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dear Florian

On Wed, Mar 19, 2014 at 1:28 AM, Florian Fainelli [off-list ref] wrote:

2014-03-18 1:40 GMT-07:00 Zhangfei Gao [off-list ref]:

quoted

+static int hip04_mdio_reset(struct mii_bus *bus)
+{
+       int temp, err, i;
+
+       for (i = 0; i < 2; i++) {
+               hip04_mdio_write(bus, i, 22, 0);
+               temp = hip04_mdio_read(bus, i, MII_BMCR);
+               temp |= BMCR_RESET;
+               err = hip04_mdio_write(bus, i, MII_BMCR, temp);
+               if (err < 0)
+                       return err;
+       }
+
+       mdelay(500);

This does not look correct, you should iterate over all possible PHYs:
PHY_MAX_ADDR instead of hardcoding the loop to 2.

OK, got it.
Use 2 is since only have 2 phy in the board, will use PHY_MAX_ADDR instead.

I think we might want to remove the mdio bus reset callback in general
as the PHY library should already take care of software resetting the
PHY to put it in a sane state, as well as waiting for the appropriate
delay before using, unlike here, where you do not poll for BMCR_RESET
to be cleared by the PHY.

Do you mean will move BMCR_RESET to common code, that's would be great.
The mdio_reset is added here to get phy_id, otherwise the phy_id can
not be read as well as detection.

Thanks

2014-03-20 3:53 GMT-07:00 Zhangfei Gao [off-list ref]:

Dear Florian

On Wed, Mar 19, 2014 at 1:28 AM, Florian Fainelli [off-list ref] wrote:

quoted

2014-03-18 1:40 GMT-07:00 Zhangfei Gao [off-list ref]:

quoted

quoted

+static int hip04_mdio_reset(struct mii_bus *bus)
+{
+       int temp, err, i;
+
+       for (i = 0; i < 2; i++) {
+               hip04_mdio_write(bus, i, 22, 0);
+               temp = hip04_mdio_read(bus, i, MII_BMCR);
+               temp |= BMCR_RESET;
+               err = hip04_mdio_write(bus, i, MII_BMCR, temp);
+               if (err < 0)
+                       return err;
+       }
+
+       mdelay(500);

This does not look correct, you should iterate over all possible PHYs:
PHY_MAX_ADDR instead of hardcoding the loop to 2.

OK, got it.
Use 2 is since only have 2 phy in the board, will use PHY_MAX_ADDR instead.

quoted

I think we might want to remove the mdio bus reset callback in general
as the PHY library should already take care of software resetting the
PHY to put it in a sane state, as well as waiting for the appropriate
delay before using, unlike here, where you do not poll for BMCR_RESET
to be cleared by the PHY.

Do you mean will move BMCR_RESET to common code, that's would be great.
The mdio_reset is added here to get phy_id, otherwise the phy_id can
not be read as well as detection.

Oh I see, thanks for mentioning that, this is not properly covered
here today as we need to get the PHY id before we assign it a
phy_device structure, let me cook a patch for this.
-- 
Florian

On Fri, Mar 21, 2014 at 1:59 AM, Florian Fainelli [off-list ref] wrote:

2014-03-20 3:53 GMT-07:00 Zhangfei Gao [off-list ref]:

quoted

quoted

quoted

+static int hip04_mdio_reset(struct mii_bus *bus)
+{
+       int temp, err, i;
+
+       for (i = 0; i < 2; i++) {
+               hip04_mdio_write(bus, i, 22, 0);
+               temp = hip04_mdio_read(bus, i, MII_BMCR);
+               temp |= BMCR_RESET;
+               err = hip04_mdio_write(bus, i, MII_BMCR, temp);
+               if (err < 0)
+                       return err;
+       }
+
+       mdelay(500);

This does not look correct, you should iterate over all possible PHYs:
PHY_MAX_ADDR instead of hardcoding the loop to 2.

OK, got it.
Use 2 is since only have 2 phy in the board, will use PHY_MAX_ADDR instead.

quoted

I think we might want to remove the mdio bus reset callback in general
as the PHY library should already take care of software resetting the
PHY to put it in a sane state, as well as waiting for the appropriate
delay before using, unlike here, where you do not poll for BMCR_RESET
to be cleared by the PHY.

Do you mean will move BMCR_RESET to common code, that's would be great.
The mdio_reset is added here to get phy_id, otherwise the phy_id can
not be read as well as detection.

Oh I see, thanks for mentioning that, this is not properly covered
here today as we need to get the PHY id before we assign it a
phy_device structure, let me cook a patch for this.
--

Should I keep hip04_mdio_reset in the next submit or remove it directly?

Thanks