Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1420697

Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver

Show all headers | View raw

On Monday, June 13, 2016 2:07:56 PM CEST Dongpo Li wrote:

> +- reset-names: should contain the reset signal name "mac_reset"(required)
> +	and "phy_reset"(optional).

Maybe just name the resets 'mac' and 'phy'? The '_reset' part is
implied by the property.

I gave the driver a brief review and basically everything looks
great, very nice work!

There are two small things that I noticed:

> +
> +       do {
> +               hisi_femac_xmit_reclaim(dev);
> +               num = hisi_femac_rx(dev, task);
> +               work_done += num;
> +               task -= num;
> +               if ((work_done >= budget) || (num == 0))
> +                       break;
> +
> +               ints = readl(priv->glb_base + GLB_IRQ_STAT);
> +               writel(ints & DEF_INT_MASK,
> +                      priv->glb_base + GLB_IRQ_RAW);
> +       } while (ints & DEF_INT_MASK);
> +
> +       if (work_done < budget) {
> +               napi_complete(napi);
> +               hisi_femac_irq_enable(priv, DEF_INT_MASK);
> +       }

You tx function uses BQL to optimize the queue length, and that
is great. You also check xmit reclaim for rx interrupts, so
as long as you have both rx and tx traffic, this should work
great.

However, I notice that you only have a 'tx fifo empty'
interrupt triggering the napi poll, so I guess on a tx-only
workload you will always end up pushing packets into the
queue until BQL throttles tx, and then get the interrupt
after all packets have been sent, which will cause BQL to
make the queue longer up to the maximum queue size, and that
negates the effect of BQL.

Is there any way you can get a tx interrupt earlier than
this in order to get a more balanced queue, or is it ok
to just rely on rx packets to come in occasionally, and
just use the tx fifo empty interrupt as a fallback?

> +	priv->phy_mode = of_get_phy_mode(node);
> +	if (priv->phy_mode < 0) {
> +		dev_err(dev, "not find phy-mode\n");
> +		ret = -EINVAL;
> +		goto out_disable_clk;
> +	}
> +
> +	priv->phy_node = of_parse_phandle(node, "phy-handle", 0);
> +	if (!priv->phy_node) {
> +		dev_err(dev, "not find phy-handle\n");
> +		ret = -EINVAL;
> +		goto out_disable_clk;
> +	}
> +
> +	priv->phy = of_phy_connect(ndev, priv->phy_node,
> +				   hisi_femac_adjust_link, 0, priv->phy_mode);
> +	if (!(priv->phy) || IS_ERR(priv->phy)) {
> +		dev_err(dev, "connect to PHY failed!\n");
> +		ret = -ENODEV;
> +		goto out_phy_node;
> +	}

I wonder if we could generalize this set of three calls, I
get the impression that we duplicate this across several
drivers that shouldn't need to bother with the specific
phy-handle and phy-mode properties.

	Arnd

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/3] Add Hisilicon MDIO bus driver and FEMAC driver Dongpo Li <lidongpo@hisilicon.com> - 2016-06-13 08:20 +0200
  [PATCH 2/3] ethtool: Add common functions for get and set settings Dongpo Li <lidongpo@hisilicon.com> - 2016-06-13 08:20 +0200
    Re: [PATCH 2/3] ethtool: Add common functions for get and set  settings kbuild test robot <lkp@intel.com> - 2016-06-13 09:40 +0200
    Re: [PATCH 2/3] ethtool: Add common functions for get and set  settings kbuild test robot <lkp@intel.com> - 2016-06-13 10:20 +0200
    Re: [PATCH 2/3] ethtool: Add common functions for get and set  settings kbuild test robot <lkp@intel.com> - 2016-06-13 10:40 +0200
  [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver Dongpo Li <lidongpo@hisilicon.com> - 2016-06-13 08:20 +0200
    Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver Arnd Bergmann <arnd@arndb.de> - 2016-06-13 12:40 +0200
      Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver Li Dongpo <lidongpo@hisilicon.com> - 2016-06-14 15:20 +0200
        Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver Arnd Bergmann <arnd@arndb.de> - 2016-06-14 23:20 +0200
          Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver Dongpo Li <lidongpo@hisilicon.com> - 2016-06-15 12:00 +0200
    Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver Rob Herring <robh@kernel.org> - 2016-06-15 00:40 +0200
      Re: [PATCH 3/3] net: hisilicon: Add Fast Ethernet MAC driver Dongpo Li <lidongpo@hisilicon.com> - 2016-06-15 12:20 +0200

csiph-web