|
|
Re: [PATCH v6] lpc32xx: Added ethernet driver |
Hi Roland, Le 03/07/12 21:21, Roland Stigge a écrit :
This patch adds an ethernet driver for the LPC32xx ARM SoC. Signed-off-by: Roland Stigge<stigge@xxxxxxxxx>
I have a couple of phylib-related comments addressed inline.
--- Applies to v3.3-rc6 Changes since v5: * Indentation and whitespace fixes * Use __le32 for descriptors * Use bool for boolean function parameters * Removed unnecessary (u32) cast * Added comment regarding future device tree introduction Thanks to David Miller, Arnd Bergmann and Joe Perches for reviewing the last version! drivers/net/ethernet/Kconfig | 1 drivers/net/ethernet/Makefile | 1 drivers/net/ethernet/nxp/Kconfig | 8 drivers/net/ethernet/nxp/Makefile | 1 drivers/net/ethernet/nxp/lpc_eth.c | 1616 +++++++++++++++++++++++++++++++++++++ 5 files changed, 1627 insertions(+) --- linux-2.6.orig/drivers/net/ethernet/Kconfig +++ linux-2.6/drivers/net/ethernet/Kconfig
+
+static int lpc_mii_probe(struct net_device *ndev)
+{
+ struct netdata_local *pldat = netdev_priv(ndev);
+ struct phy_device *phydev = NULL;
+ int phy_addr;
+
+ /* find the first phy */
+ for (phy_addr = 0; phy_addr< PHY_MAX_ADDR; phy_addr++) {
+ if (pldat->mii_bus->phy_map[phy_addr]) {
+ phydev = pldat->mii_bus->phy_map[phy_addr];
+ break;
+ }
+ }
Please use phy_find_first() instead of open-coding this.
+
+ if (!phydev) {
+ netdev_err(ndev, "no PHY found\n");
+ return -ENODEV;
+ }
+
+ /* Attach to the PHY */
+ if (lpc_phy_interface_mode() == PHY_INTERFACE_MODE_MII)
+ netdev_info(ndev, "using MII interface\n");
+ else
+ netdev_info(ndev, "using RMII interface\n");
+ phydev = phy_connect(ndev, dev_name(&phydev->dev),
+ &lpc_handle_link_change, 0, lpc_phy_interface_mode());
+
+ if (IS_ERR(phydev)) {
+ netdev_err(ndev, "Could not attach to PHY\n");
+ return PTR_ERR(phydev);
+ }
+
+ /* mask with MAC supported features */
+ phydev->supported&= PHY_BASIC_FEATURES;
+
+ phydev->advertising = phydev->supported;
+
+ pldat->link = 0;
+ pldat->speed = 0;
+ pldat->duplex = -1;
+ pldat->phy_dev = phydev;
+
+ return 0;
+}
+
+static int lpc_mii_init(struct netdata_local *pldat)
+{
+ int err = -ENXIO, i;
+
+ pldat->mii_bus = mdiobus_alloc();
+ if (!pldat->mii_bus) {
+ err = -ENOMEM;
+ goto err_out;
+ }
+
+ /* Setup MII mode */
+ if (lpc_phy_interface_mode() == PHY_INTERFACE_MODE_MII)
+ writel(LPC_COMMAND_PASSRUNTFRAME,
+ LPC_ENET_COMMAND(pldat->net_base));
+ else {
+ writel((LPC_COMMAND_PASSRUNTFRAME | LPC_COMMAND_RMII),
+ LPC_ENET_COMMAND(pldat->net_base));
+ writel(LPC_SUPP_RESET_RMII, LPC_ENET_SUPP(pldat->net_base));
+ }
+
+ pldat->mii_bus->name = "lpc_mii_bus";
+ pldat->mii_bus->read =&lpc_mdio_read;
+ pldat->mii_bus->write =&lpc_mdio_write;
+ pldat->mii_bus->reset =&lpc_mdio_reset;
+ snprintf(pldat->mii_bus->id, MII_BUS_ID_SIZE, "%x", pldat->pdev->id);
No, makes this bus name unique in the system: snprnitf(pldat->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x", pldat->pdev->name, pldat->pdev->id);
+ pldat->mii_bus->priv = pldat; + pldat->mii_bus->parent =&pldat->pdev->dev; + pldat->mii_bus->phy_mask = 0xFFFFFFF0;
I would rather not hardcode the phy_mask here, just leave it to 0, and let phy_find_first() find the PHY devices for you, or make this platform configurable.
+
+ pldat->mii_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
+ if (!pldat->mii_bus->irq) {
+ err = -ENOMEM;
+ goto err_out_1;
+ }
+
+ for (i = 0; i< PHY_MAX_ADDR; i++)
+ pldat->mii_bus->irq[i] = PHY_POLL;
+
+ platform_set_drvdata(pldat->pdev, pldat->mii_bus);
+
+ if (mdiobus_register(pldat->mii_bus))
+ goto err_out_free_mdio_irq;
+
+ if (lpc_mii_probe(pldat->ndev) != 0)
+ goto err_out_unregister_bus;
+
+ return 0;
+
+err_out_unregister_bus:
+ mdiobus_unregister(pldat->mii_bus);
+err_out_free_mdio_irq:
+ kfree(pldat->mii_bus->irq);
+err_out_1:
+ mdiobus_free(pldat->mii_bus);
+err_out:
+ return err;
+}
+
[snip]
+
+static int lpc_eth_open(struct net_device *ndev)
+{
+ struct netdata_local *pldat = netdev_priv(ndev);
+
+ /* if the phy is not yet registered, retry later*/
+ if (!pldat->phy_dev)
+ return -EAGAIN;
You have already probed the mii bus in the driver's probe function, how can you end up here without a phy being attached here?
[snip]
+ if (lpc_mii_init(pldat) != 0) + goto err_out_unregister_netdev; + + netdev_info(ndev, "LPC mac at 0x%08x irq %d\n", + res->start, ndev->irq); + + phydev = pldat->phy_dev; + netdev_info(ndev, + "attached PHY driver [%s] (mii_bus:phy_addr=%s, irq=%d)\n", + phydev->drv->name, dev_name(&phydev->dev), phydev->irq);
Most drivers put this informational message in their mii probing function, please move it here as well.
+
+ device_init_wakeup(&pdev->dev, 1);
+ device_set_wakeup_enable(&pdev->dev, 0);
+
+ return 0;
+
+err_out_unregister_netdev:
+ platform_set_drvdata(pdev, NULL);
+ unregister_netdev(ndev);
+err_out_dma_unmap:
+ if (!use_iram_for_net() ||
+ pldat->dma_buff_size> lpc32xx_return_iram_size())
+ dma_free_coherent(&pldat->pdev->dev, pldat->dma_buff_size,
+ pldat->dma_buff_base_v,
+ pldat->dma_buff_base_p);
+err_out_free_irq:
+ free_irq(ndev->irq, ndev);
+err_out_iounmap:
+ iounmap(pldat->net_base);
+err_out_disable_clocks:
+ clk_disable(pldat->clk);
+ clk_put(pldat->clk);
+err_out_free_dev:
+ free_netdev(ndev);
+err_exit:
+ pr_err("%s: not found (%d).\n", MODNAME, ret);
+ return ret;
+}
+
+static int lpc_eth_drv_remove(struct platform_device *pdev)
+{
+ struct net_device *ndev = platform_get_drvdata(pdev);
+ struct netdata_local *pldat = netdev_priv(ndev);
+
+ unregister_netdev(ndev);
+ platform_set_drvdata(pdev, NULL);
+
+ if (!use_iram_for_net() ||
+ pldat->dma_buff_size> lpc32xx_return_iram_size())
+ dma_free_coherent(&pldat->pdev->dev, pldat->dma_buff_size,
+ pldat->dma_buff_base_v,
+ pldat->dma_buff_base_p);
+ free_irq(ndev->irq, ndev);
+ iounmap(pldat->net_base);
+ clk_disable(pldat->clk);
+ clk_put(pldat->clk);
+ free_netdev(ndev);
You are not freeing the mii bus in the driver's remove path. -- Florian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[Linux ARM (vger)] [Linux ARM MSM] [Linux Omap] [Linux Arm] [Linux Tegra] [Fedora ARM] [eCos] [Linux Fastboot] [Gcc Help] [Git] [DCCP] [IETF Announce] [Security] [PDAs] [Linux] [Linux MIPS] [Yosemite Campsites] [Photos]
|
|