On Thu, Feb 16, 2017 at 08:05:24PM +0100, Maxime Ripard wrote:

Hi,

[...]

quoted

+
+struct emac_variant {
+	u32 default_syscon_value;

Why do you need a default value? Can't you read it from the syscon
directly?

Why not, but you can see the default value as "value for disabled state".
My fear is that something (uboot) modify it (keep it activated) before driver load.

[...]

quoted

+static void sun8i_dwmac_dump_regs(void __iomem *ioaddr)
+{
+	int i;
+
+	pr_info(" DMA registers\n");

Logging this as pr_info is bad already...

quoted

+	for (i = 0; i < 0xC8; i += 4) {
+		if (i == 0x32 || i == 0x3C)
+			continue;
+		pr_err("Reg 0x%x: 0x%08x\n", i, readl(ioaddr + i));

... But this is worse.

Why do you need to do that? Can't you create a file in debugfs
instead?

I just do as other glue does. But yes this is uglyi, no excuse.
Reworking all stmmac register dump (ethtool, stmmac_ops->dump_regs and stmmac_dma_ops->dump_regs) was on my todo list,
but I postponed it.

I will propose something better based on debugfs.

[...]

quoted

+static void sun8i_dwmac_dma_start_tx(void __iomem *ioaddr)
+{
+	u32 v;
+
+	v = readl(ioaddr + EMAC_TX_CTL0);
+	v |= EMAC_TX_TRANSMITTER_EN;
+	writel(v, ioaddr + EMAC_TX_CTL0);
+
+	v = readl(ioaddr + EMAC_TX_CTL1);
+	v |= EMAC_TX_DMA_START;
+	v |= EMAC_TX_DMA_EN;
+	writel(v, ioaddr + EMAC_TX_CTL1);

This is a bit worrying. There's not a single lock in your driver,
while you have a significant number of read / modify / write.

Where is the locking handled?

All thoses function are handled by the "stmmac_ops framework", all other glue drivers does not lock anything.
The few functions that need locking already got it on the calling stmmac side.

[...]

quoted

+static int sun8i_dwmac_init(struct platform_device *pdev, void *priv)
+{
+	struct sunxi_priv_data *gmac = priv;
+	int ret;
+
+	if (gmac->regulator) {
+		ret = regulator_enable(gmac->regulator);
+		if (ret) {
+			dev_err(&pdev->dev, "Fail to enable regulator\n");
+			return ret;
+		}
+	}
+
+	ret = clk_prepare_enable(gmac->tx_clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Could not enable AHB clock\n");

If that call fails, you leave the regulator (if there was any) enabled.

I will fix it

quoted

+		return ret;
+	}
+
+	return 0;
+}
+
+static void sun8i_dwmac_core_init(struct mac_device_info *hw, int mtu)
+{
+	void __iomem *ioaddr = hw->pcsr;
+	u32 v;
+
+	v = (8 << 24);/* burst len */
+	writel(v, ioaddr + EMAC_BASIC_CTL1);

do you need an intermediate value? you should make a define for that
too.

I will fix it

[...]

quoted

+static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
+{
+	struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
+	struct device_node *node = priv->device->of_node;
+	int ret;
+	u32 reg, val;
+
+	reg = gmac->variant->default_syscon_value;
+
+	if (gmac->variant->internal_phy) {
+		if (!gmac->use_internal_phy) {
+			/* switch to external PHY interface */
+			reg &= ~H3_EPHY_SELECT;
+		} else {
+			reg |= H3_EPHY_SELECT;
+			reg &= ~H3_EPHY_SHUTDOWN;
+			dev_info(priv->device, "Select internal_phy %x\n", reg);

The logging level is too high

I will reduce it

quoted

+
+			if (of_property_read_bool(priv->plat->phy_node,
+						  "allwinner,leds-active-low"))
+				reg |= H3_EPHY_LED_POL;
+			else
+				reg &= ~H3_EPHY_LED_POL;
+
+			ret = of_mdio_parse_addr(priv->device,
+						 priv->plat->phy_node);
+			if (ret < 0) {
+				dev_err(priv->device, "Could not parse MDIO addr\n");
+				return ret;
+			}
+			/* of_mdio_parse_addr returns a valid (0 ~ 31) PHY
+			 * address. No need to mask it again.
+			 */
+			reg |= ret << H3_EPHY_ADDR_SHIFT;
+		}
+	}
+
+	if (!of_property_read_u32(node, "allwinner,tx-delay", &val)) {

How do you compute it? Can't this be done through auto-training?

The value is the same as used in vendor BSP kernel.
I do not understand what you mean by auto-training.

quoted

+		dev_info(priv->device, "set tx-delay to %x\n", val);

change the logging level here too.

I agree

quoted

+		if (val <= SYSCON_ETXDC_MASK) {
+			reg &= ~(SYSCON_ETXDC_MASK << SYSCON_ETXDC_SHIFT);
+			reg |= (val << SYSCON_ETXDC_SHIFT);
+		} else {
+			dev_warn(priv->device, "Invalid TX clock delay: %d\n",
+				 val);

If it's invalid, why don't you treat it as an error and return?

Ok

[...]

quoted

+static struct mac_device_info *sun8i_dwmac_setup(void __iomem *ioaddr,
+						 int mcbins,
+						 int perfect_uc_entries,
+						 int *synopsys_id)
+{
+	struct mac_device_info *mac;
+
+	mac = kzalloc(sizeof(const struct mac_device_info), GFP_KERNEL);
+	if (!mac)
+		return NULL;

Do you ever free that memory?

Good catch, I believed that the "stmmac framework" would free it.
I will send a fix for this memory leak.

[...]

quoted

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 5ff6bc4..11db658 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c

@@ -450,6 +450,9 @@ static void stmmac_ethtool_gregs(struct net_device *dev,
 		for (i = 0; i < 22; i++)
 			reg_space[i + 55] =
 			    readl(priv->ioaddr + (DMA_BUS_MODE + (i * 4)));
+	} else if (priv->plat->has_sun8i) {

Surely we don't want to add a new flag to the common structure for
every new platform supported.

Can't you base that on the compatible instead?

This part will be fixed with the debugfs speaked early in the mail.

But yes I have tried to avoid use of has_sun8i at maximum.

Thanks a lot for your work,
Maxime

Thanks for the review

Corentin Labbe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html