Re: [PATCH v5] ethernet/arc/arc_emac - Add new driver

[Andy Shevchenko]

On Wed, Jun 19, 2013 at 11:12 AM, Alexey Brodkin
<Alexey.Brodkin@xxxxxxxxxxxx> wrote:
> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
> ARCAngel4/ML50x.
>
> This is based off of current Linus tree.

This line should go away from commit message.

> please take a look at 5th re-spin of the patch.

Few small things I commented below.

> +++ b/drivers/net/ethernet/arc/emac_main.c
> @@ -0,0 +1,828 @@

> +static int arc_emac_rx(struct net_device *ndev, int budget)
> +{
> +       struct arc_emac_priv *priv = netdev_priv(ndev);
> +       unsigned int work_done;
> +
> +       for (work_done = 0; work_done <= budget;) {
> +               unsigned int *last_rx_bd = &priv->last_rx_bd;
> +               struct net_device_stats *stats = &priv->stats;
> +               struct buffer_state *rx_buff = &priv->rx_buff[*last_rx_bd];
> +               struct arc_emac_bd *rxbd = &priv->rxbd[*last_rx_bd];
> +               unsigned int buflen = EMAC_BUFFER_SIZE;
> +               unsigned int pktlen, info = __le32_to_cpu(rxbd->info);
> +               struct sk_buff *skb;
> +               dma_addr_t addr;
> +
> +               if (unlikely((info & OWN_MASK) == FOR_EMAC))
> +                       break;

> +               work_done++;

Why it can't be inside for()?

> +               if (unlikely((info & FRST_OR_LAST_MASK) != FRST_OR_LAST_MASK)) {
> +                       /* Buffer chaining results in a significant
> +                        * amount of additional bus overhead and thus
> +                        * a higher CLK frequency or larger FIFOs are required.
> +                        * Because of that fact we don't support
> +                        * chaining of receive packets. We pre-allocate
> +                        * buffers of MTU size so incoming packets
> +                        * won't be chained.
> +                        */

Could that comment take less LOC?

> +                       /* Return ownership to EMAC */
> +                       rxbd->info = __cpu_to_le32(FOR_EMAC | buflen);

Here and in the whole file. Parhaps cpu_to_le32()?

> +               /* Once EMAC sees he is an owner of this BD, EMAC will start to
> +                * use it for receiving or transmitting packets,
> +                * depending on BD: Tx or Rx.
> +                * That's why we need to make sure "data" has proper address
> +                * before writing "info" word with owner flag.
> +                */

Less LOC?

> +       if (status & ERR_MASK) {
> +               /* MSER/RXCR/RXFR/RXFL interrupt fires on corresponding
> +                * 8-bit error counter overrun.
> +                * Because of this fact we add 256 (0x100) items each time
> +                * overrun interrupt happens.
> +                */

Ditto.

> +static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
> +{

> +       len = max_t(unsigned int, ETH_ZLEN, skb->len);
> +
> +       /* EMAC still holds this buffer in its possession.
> +        * CPU must not modify this buffer descriptor
> +        */
> +       if (unlikely((__le32_to_cpu(*info) & OWN_MASK) == FOR_EMAC)) {

Perhaps le32_to_cpu() here and in whole file?

> +static int arc_emac_probe(struct platform_device *pdev)
> +{

> +       err = of_address_to_resource(pdev->dev.of_node, 0, &res_regs);

> +       if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> +                                &clock_frequency)) {

> +       err = of_irq_to_resource(pdev->dev.of_node, 0, &res_irq);

You may save few lines if you move those checks before allocation of the netdev.

> +       err = arc_mdio_probe(pdev->dev.of_node, priv);
> +       if (err) {
> +               dev_err(&pdev->dev, "failed to probe MII bus\n");
> +               goto out;
> +       }
> +
> +       priv->phy_dev = of_phy_connect(ndev, phy_node, arc_emac_adjust_link, 0,
> +                                      PHY_INTERFACE_MODE_MII);
> +       if (!priv->phy_dev) {
> +               netdev_err(ndev, "of_phy_connect() failed\n");

Can we be consistent and use here dev_err()?

> +++ b/drivers/net/ethernet/arc/emac_mdio.c
> @@ -0,0 +1,151 @@

> +static int arc_mdio_complete_wait(struct arc_emac_priv *priv)
> +{
> +       unsigned int i;
> +
> +       for (i = 0; i < ARC_MDIO_COMPLETE_POLL_COUNT * 40; i++) {

> +               msleep(25);

So, in the worst case it takes ARC_MDIO_COMPLETE_POLL_COUNT seconds to
wait. Quite a long time.
Does user expect such a delay?

> +int arc_mdio_probe(struct device_node *dev_node, struct arc_emac_priv *priv)
> +{

> +       snprintf(bus->id, MII_BUS_ID_SIZE, "%.8x", (unsigned int)priv->regs);

Is bus->id exposed to user-space somehow?

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/