subject:"\[PATCH 05\/21\] net\-next\: stmmac\: Add dwmac\-sun8i"

Re: [PATCH 05/21] net-next: stmmac: Add dwmac-sun8i

2017-02-21 Thread Maxime Ripard

On Fri, Feb 17, 2017 at 02:18:02PM +0100, Corentin Labbe wrote:
> On Thu, Feb 16, 2017 at 08:05:24PM +0100, Maxime Ripard wrote:
> > Hi,
> > 
> 
> [...]
> > > +
> > > +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".

i'm not sure what you mean here, sorry.

> My fear is that something (uboot) modify it (keep it activated)
> before driver load.

You could have the same argument there then for the board that require
reading it. What if U-boot modified it to some non-functional state?

Either you trust the value there, and you read it, or you don't, and
then you never read it. But being stuck in between doesn't seem that
great.

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

Most of them seem to use regmap though, that has an internal lock.

> The few functions that need locking already got it on the calling
> stmmac side.

Ok.

> > > +
> > > + 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", )) {
> > 
> > How do you compute it? Can't this be done through auto-training?
> 
> The value is the same as used in vendor BSP kernel.

This is not really usable though. I've had already three boards that
never got any BSP kernel. You need to be able at least to document
some way to compute it (even if it's based on manual, trial and error
process).

> I do not understand what you mean by auto-training.

Being able to automatically detect the optimal settings at boot time.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 05/21] net-next: stmmac: Add dwmac-sun8i

2017-02-17 Thread Corentin Labbe

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

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

[...]
> > +static void sun8i_dwmac_dump_regs(void __iomem *ioaddr)
> > +{
> > +   int i;
> > +
> > +   pr_info(" DMA registers\n");
> 
> Logging this as pr_info is bad already...
> 
> > +   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.

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

[...]
> > +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(>dev, "Fail to enable regulator\n");
> > +   return ret;
> > +   }
> > +   }
> > +
> > +   ret = clk_prepare_enable(gmac->tx_clk);
> > +   if (ret) {
> > +   dev_err(>dev, "Could not enable AHB clock\n");
> 
> If that call fails, you leave the regulator (if there was any) enabled.
> 

I will fix it

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

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

> > +
> > +   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", )) {
> 
> 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.

> > +   dev_info(priv->device, "set tx-delay to %x\n", val);
> 
> change the logging level here too.
> 

I agree

> > +   if (val <= SYSCON_ETXDC_MASK) {
> > +

Re: [PATCH 05/21] net-next: stmmac: Add dwmac-sun8i

2017-02-17 Thread Maxime Ripard

Hi,

On Thu, Feb 16, 2017 at 01:48:43PM +0100, Corentin Labbe wrote:
> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> allwinner.
> In fact the only common part is the descriptor management and the first
> register function.
> 
> Signed-off-by: Corentin Labbe 
> ---
>  drivers/net/ethernet/stmicro/stmmac/Kconfig|  11 +
>  drivers/net/ethernet/stmicro/stmmac/Makefile   |   1 +
>  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c  | 892 
> +
>  .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   |   3 +
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  27 +-
>  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |   9 +-
>  include/linux/stmmac.h |   1 +
>  7 files changed, 941 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig 
> b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index cfbe363..85c0e41 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -145,6 +145,17 @@ config DWMAC_SUNXI
> This selects Allwinner SoC glue layer support for the
> stmmac device driver. This driver is used for A20/A31
> GMAC ethernet controller.
> +
> +config DWMAC_SUN8I
> + tristate "Allwinner sun8i GMAC support"
> + default ARCH_SUNXI
> + depends on OF && (ARCH_SUNXI || COMPILE_TEST)
> + ---help---
> +   Support for Allwinner H3 A83T A64 EMAC ethernet controllers.
> +
> +   This selects Allwinner SoC glue layer support for the
> +   stmmac device driver. This driver is used for H3/A83T/A64
> +   EMAC ethernet controller.
>  endif
>  
>  config STMMAC_PCI
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile 
> b/drivers/net/ethernet/stmicro/stmmac/Makefile
> index 700c603..fd4937a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_DWMAC_SOCFPGA) += dwmac-altr-socfpga.o
>  obj-$(CONFIG_DWMAC_STI)  += dwmac-sti.o
>  obj-$(CONFIG_DWMAC_STM32)+= dwmac-stm32.o
>  obj-$(CONFIG_DWMAC_SUNXI)+= dwmac-sunxi.o
> +obj-$(CONFIG_DWMAC_SUN8I)+= dwmac-sun8i.o
>  obj-$(CONFIG_DWMAC_DWC_QOS_ETH)  += dwmac-dwc-qos-eth.o
>  obj-$(CONFIG_DWMAC_GENERIC)  += dwmac-generic.o
>  stmmac-platform-objs:= stmmac_platform.o
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c 
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> new file mode 100644
> index 000..0951eb9
> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> @@ -0,0 +1,892 @@
> +/*
> + * dwmac-sun8i.c - Allwinner sun8i DWMAC specific glue layer
> + *
> + * Copyright (C) 2017 Corentin Labbe 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "stmmac.h"
> +#include "stmmac_platform.h"
> +
> +struct emac_variant {
> + u32 default_syscon_value;

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

> + int internal_phy;
> + bool support_mii;
> + bool support_rmii;
> + bool support_rgmii;
> +};
> +
> +struct sunxi_priv_data {
> + struct clk *tx_clk;
> + struct clk *ephy_clk;
> + struct regulator *regulator;
> + struct reset_control *rst_ephy;
> + const struct emac_variant *variant;
> + bool use_internal_phy;
> + struct regmap *regmap;
> +};
> +
> +static const struct emac_variant emac_variant_h3 = {
> + .default_syscon_value = 0x58000,
> + .internal_phy = PHY_INTERFACE_MODE_MII,
> + .support_mii = true,
> + .support_rmii = true,
> + .support_rgmii = true
> +};
> +
> +static const struct emac_variant emac_variant_a83t = {
> + .default_syscon_value = 0,
> + .internal_phy = 0,
> + .support_mii = true,
> + .support_rgmii = true
> +};
> +
> +static const struct emac_variant emac_variant_a64 = {
> + .default_syscon_value = 0,
> + .internal_phy = 0,
> + .support_mii = true,
> + .support_rmii = true,
> + .support_rgmii = true
> +};
> +
> +#define EMAC_BASIC_CTL0 0x00
> +#define EMAC_BASIC_CTL1 0x04
> +#define EMAC_INT_STA0x08
> +#define EMAC_INT_EN 0x0C
> +#define

[PATCH 05/21] net-next: stmmac: Add dwmac-sun8i

2017-02-16 Thread Corentin Labbe

The dwmac-sun8i is a heavy hacked version of stmmac hardware by
allwinner.
In fact the only common part is the descriptor management and the first
register function.

Signed-off-by: Corentin Labbe 
---
 drivers/net/ethernet/stmicro/stmmac/Kconfig|  11 +
 drivers/net/ethernet/stmicro/stmmac/Makefile   |   1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c  | 892 +
 .../net/ethernet/stmicro/stmmac/stmmac_ethtool.c   |   3 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  |  27 +-
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |   9 +-
 include/linux/stmmac.h |   1 +
 7 files changed, 941 insertions(+), 3 deletions(-)
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig 
b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index cfbe363..85c0e41 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -145,6 +145,17 @@ config DWMAC_SUNXI
  This selects Allwinner SoC glue layer support for the
  stmmac device driver. This driver is used for A20/A31
  GMAC ethernet controller.
+
+config DWMAC_SUN8I
+   tristate "Allwinner sun8i GMAC support"
+   default ARCH_SUNXI
+   depends on OF && (ARCH_SUNXI || COMPILE_TEST)
+   ---help---
+ Support for Allwinner H3 A83T A64 EMAC ethernet controllers.
+
+ This selects Allwinner SoC glue layer support for the
+ stmmac device driver. This driver is used for H3/A83T/A64
+ EMAC ethernet controller.
 endif
 
 config STMMAC_PCI
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile 
b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 700c603..fd4937a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_DWMAC_SOCFPGA)   += dwmac-altr-socfpga.o
 obj-$(CONFIG_DWMAC_STI)+= dwmac-sti.o
 obj-$(CONFIG_DWMAC_STM32)  += dwmac-stm32.o
 obj-$(CONFIG_DWMAC_SUNXI)  += dwmac-sunxi.o
+obj-$(CONFIG_DWMAC_SUN8I)  += dwmac-sun8i.o
 obj-$(CONFIG_DWMAC_DWC_QOS_ETH)+= dwmac-dwc-qos-eth.o
 obj-$(CONFIG_DWMAC_GENERIC)+= dwmac-generic.o
 stmmac-platform-objs:= stmmac_platform.o
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c 
b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
new file mode 100644
index 000..0951eb9
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -0,0 +1,892 @@
+/*
+ * dwmac-sun8i.c - Allwinner sun8i DWMAC specific glue layer
+ *
+ * Copyright (C) 2017 Corentin Labbe 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "stmmac.h"
+#include "stmmac_platform.h"
+
+struct emac_variant {
+   u32 default_syscon_value;
+   int internal_phy;
+   bool support_mii;
+   bool support_rmii;
+   bool support_rgmii;
+};
+
+struct sunxi_priv_data {
+   struct clk *tx_clk;
+   struct clk *ephy_clk;
+   struct regulator *regulator;
+   struct reset_control *rst_ephy;
+   const struct emac_variant *variant;
+   bool use_internal_phy;
+   struct regmap *regmap;
+};
+
+static const struct emac_variant emac_variant_h3 = {
+   .default_syscon_value = 0x58000,
+   .internal_phy = PHY_INTERFACE_MODE_MII,
+   .support_mii = true,
+   .support_rmii = true,
+   .support_rgmii = true
+};
+
+static const struct emac_variant emac_variant_a83t = {
+   .default_syscon_value = 0,
+   .internal_phy = 0,
+   .support_mii = true,
+   .support_rgmii = true
+};
+
+static const struct emac_variant emac_variant_a64 = {
+   .default_syscon_value = 0,
+   .internal_phy = 0,
+   .support_mii = true,
+   .support_rmii = true,
+   .support_rgmii = true
+};
+
+#define EMAC_BASIC_CTL0 0x00
+#define EMAC_BASIC_CTL1 0x04
+#define EMAC_INT_STA0x08
+#define EMAC_INT_EN 0x0C
+#define EMAC_TX_CTL00x10
+#define EMAC_TX_CTL10x14
+#define EMAC_TX_FLOW_CTL0x1C
+#define EMAC_TX_DESC_LIST 0x20
+#define EMAC_RX_CTL00x24
+#define EMAC_RX_CTL10x28
+#define EMAC_RX_DESC_LIST 0x34
+#define EMAC_RX_FRM_FLT 0x38
+#define EMAC_MDIO_CMD   0x48
+#define EMAC_MDIO_DATA  0x4C
+#define EMAC_MACADDR_HI(reg) (0x50 + (reg) * 8)

Re: [PATCH 05/21] net-next: stmmac: Add dwmac-sun8i

Re: [PATCH 05/21] net-next: stmmac: Add dwmac-sun8i

Re: [PATCH 05/21] net-next: stmmac: Add dwmac-sun8i

[PATCH 05/21] net-next: stmmac: Add dwmac-sun8i

4 matches

Site Navigation

Mail list logo

Footer information