2014-03-18 15:16 GMT-07:00 Byungho An <[email protected]>:
> Florian Fainelli <[email protected]> wrote:
>> 2014-03-18 11:19 GMT-07:00 Byungho An <[email protected]>:
>> > From: Siva Reddy <[email protected]>
>> >
>> > This patch adds support for Samsung 10Gb ethernet driver(sxgbe).
>> >
>> > - sxgbe core initialization
>> > - Tx and Rx support
>> > - MDIO support
>> > - ISRs for Tx and Rx
>> > - ifconfig support to driver
>>
>> Too many files to review at once, the diffstat was around 5000+ lines!
>> You should split this into logical parts, or just submit the basic bits for
>> now and
>> add the features later.
> Yes. but I think it is one logically, after this patch there won't be much
> code at once.
Since you are adding multiple objects to the Makefile at once, you
could very well split the patches to reflect that and submit one
object as a separate patch. Also, sometimes, even if the driver is
feature complete, it helps submitting the basic support first, and
then start adding new features to it progressively.
>
>>
>> [snip]
>>
>> > + ret = register_netdev(ndev);
>> > + if (ret) {
>> > + pr_err("%s: ERROR %i registering the device\n", __func__,
>> > ret);
>> > + goto error_netdev_register;
>> > + }
>> > +
>> > + priv->sxgbe_clk = clk_get(priv->device, SXGBE_RESOURCE_NAME);
>> > + if (IS_ERR(priv->sxgbe_clk)) {
>> > + netdev_warn(ndev, "%s: warning: cannot get CSR clock\n",
>> > + __func__);
>> > + goto error_clk_get;
>> > + }
>>
>> This is racy, after register_netdev() is called, the network stack is free
>> to use
>> the interface, which means that as much as possible needs to be initialized
>> before you call it, including clocks, and MDIO.
>>
>> > +
>> > + /* If a specific clk_csr value is passed from the platform
>> > + * this means that the CSR Clock Range selection cannot be
>> > + * changed at run-time and it is fixed. Viceversa the driver'll
>> > try to
>> > + * set the MDC clock dynamically according to the csr actual
>> > + * clock input.
>> > + */
>> > + if (!priv->plat->clk_csr)
>> > + sxgbe_clk_csr_set(priv);
>> > + else
>> > + priv->clk_csr = priv->plat->clk_csr;
>> > +
>> > + /* MDIO bus Registration */
>> > + ret = sxgbe_mdio_register(ndev);
>> > + if (ret < 0) {
>> > + netdev_dbg(ndev, "%s: MDIO bus (id: %d) registration
>> > failed\n",
>> > + __func__, priv->plat->bus_id);
>> > + goto error_mdio_register;
>> > + }
>>
>> Don't do this, register your MDIO bus before calling register_netdev().
> OK.
>
>>
>> > +
>> > + sxgbe_check_ether_addr(priv);
>> > +
>> > + return priv;
>> > +
>> > +error_mdio_register:
>> > + clk_put(priv->sxgbe_clk);
>> > +error_clk_get:
>> > + unregister_netdev(ndev);
>> > +error_netdev_register:
>> > + netif_napi_del(&priv->napi);
>> > +error_free_netdev:
>> > + free_netdev(ndev);
>> > +
>> > + return NULL;
>> > +}
>> > +
>> > +/**
>> > + * sxgbe_dvr_remove
>> > + * @ndev: net device pointer
>> > + * Description: this function resets the TX/RX processes, disables
>> > +the MAC RX/TX
>> > + * changes the link status, releases the DMA descriptor rings.
>> > + */
>> > +int sxgbe_dvr_remove(struct net_device *ndev) {
>> > + struct sxgbe_priv_data *priv = netdev_priv(ndev);
>> > +
>> > + netdev_info(ndev, "%s: removing driver\n", __func__);
>> > +
>> > + priv->hw->dma->stop_rx(priv->ioaddr, SXGBE_RX_QUEUES);
>> > + priv->hw->dma->stop_tx(priv->ioaddr, SXGBE_TX_QUEUES);
>> > +
>> > + priv->hw->mac->enable_tx(priv->ioaddr, false);
>> > + priv->hw->mac->enable_rx(priv->ioaddr, false);
>> > +
>> > + netif_napi_del(&priv->napi);
>> > +
>> > + sxgbe_mdio_unregister(ndev);
>> > +
>> > + netif_carrier_off(ndev);
>>
>> This is not required, both the PHY library and the network stack will do
>> that for
>> you.
> You mean netif_napi_del and netif_carrier_off? or all of sxgbe_dvr_remove
> function?
> I think dma/mac control are needed.
I mean netif_carrier_off() is not required, everything else is.
>
>>
>> [snip]
>>
>> > + /* register with kernel subsystem */
>> > + err = mdiobus_register(mdio_bus);
>> > + if (err != 0) {
>> > + netdev_err(ndev, "mdiobus register failed\n");
>> > + goto mdiobus_err;
>> > + }
>> > +
>> > + for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++) {
>> > + struct phy_device *phy = mdio_bus->phy_map[phy_addr];
>> > + if (phy) {
>> > + char irq_num[4];
>> > + char *irq_str;
>> > + /* If an IRQ was provided to be assigned after
>> > + * the bus probe, do it here.
>> > + */
>> > + if ((mdio_data->irqs == NULL) &&
>> > + (mdio_data->probed_phy_irq > 0)) {
>> > + irqlist[phy_addr] =
>> > mdio_data->probed_phy_irq;
>> > + phy->irq = mdio_data->probed_phy_irq;
>> > + }
>> > +
>> > + /* If we're going to bind the MAC to this PHY bus,
>> > + * and no PHY number was provided to the MAC,
>> > + * use the one probed here.
>> > + */
>> > + if (priv->plat->phy_addr == -1)
>> > + priv->plat->phy_addr = phy_addr;
>> > +
>> > + act = (priv->plat->phy_addr == phy_addr);
>> > + switch (phy->irq) {
>> > + case PHY_POLL:
>> > + irq_str = "POLL";
>> > + break;
>> > + case PHY_IGNORE_INTERRUPT:
>> > + irq_str = "IGNORE";
>> > + break;
>> > + default:
>> > + sprintf(irq_num, "%d", phy->irq);
>> > + irq_str = irq_num;
>> > + break;
>> > + }
>> > + netdev_info(ndev, "PHY ID %08x at %d IRQ %s
>> > (%s)%s\n",
>> > + phy->phy_id, phy_addr, irq_str,
>> > + dev_name(&phy->dev), act ? " active" :
>> > "");
>> > + phy_found = 1;
>> > + }
>>
>> This is clunky, and you are duplicating what of_mdiobus_register() will do
>> for
>> you, but since you are not using the standard Ethernet PHY device tree node,
>> you could not use that code. So please use the standard Ethernet PHY device
>> tree node such that you can remove that code and leverage it instead.
> I'll consider it. but I think after this patch set.
Humm, I would rather you do this know this will eliminate all this
code in your driver which duplicates an existing function in the
kernel.
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
