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

On 2016/6/13 17:06, Arnd Bergmann wrote:

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

quoted

+- 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.

Hi, Arnd,
Thank you for your advice. It's ok to omit the '_reset', I'll fix it in next version.

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

There are two small things that I noticed:

quoted

+
+       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?

In tx direction, there are only two kinds of interrupts, 'tx fifo empty'
and 'tx one packet finish'. I didn't use 'tx one packet finish' because
it would lead to high hardware interrupts rate. This has been verified in
our chips. It's ok to just use tx fifo empty interrupt.

quoted

+	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.

Some drivers only call 'of_phy_connect' when ndo_open called,
some call when driver probed. But 'phy_mode' and 'phy_node' are
usually initialized when driver probed.
So I think it's not suitable to combine 'of_phy_connect' with
'of_get_phy_mode' and 'of_parse_phandle'.
Do you have any more suggestions ?

	Arnd


.

	Regards,
	Dongpo


.