[PATCH 00/10] net: stmmac: Add sun7i GMAC glue layer

[PATCH 00/10] net: stmmac: Add sun7i GMAC glue layer

[Chen-Yu Tsai]

Hi,

This patch series adds Allwinner sun7i support to stmmac.
The Allwinner sun7i SoC A20 integrates an early version of
dwmac IP from Synopsys. On top of that is a hardware glue
layer. This layer needs to be configured before the dwmac
can be used.

The glue layer is implemented with existing callbacks in
stmmac platform data. Hardware feature flags are also passed
to the driver through platform data. As suggested by Maxime,
the platform data is tied to our compatible string.
Minimal modifications to stmmac core are made.

This version of dwmac IP requires store and forward DMA mode.
The relevant device tree property was documented, but not
implemented. The second patch adds this to stmmac platform
driver.

During testing, with stmmac built as a module, loading the
driver fails during the hardware probe phase. The main clock
is not enabled, and the hardware returns bogus results.
The first patch addresses this.

Comments?

Thanks,

wens


Chen-Yu Tsai (10):
  net: stmmac: Enable stmmac main clock when probing hardware
  net: stmmac: Honor DT parameter to force DMA store and forward mode
  net: stmmac: Use platform data tied with compatible strings
  net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
  ARM: dts: sun7i: Add GMAC controller node to sun7i DTSI
  ARM: dts: sun7i: Add pin muxing options for the GMAC
  ARM: dts: sun7i: cubietruck: Enable the GMAC
  ARM: dts: sun7i: cubieboard2: Enable GMAC instead of EMAC
  ARM: dts: sun7i: olinuxino-micro: Enable GMAC instead of EMAC
  ARM: dts: sun7i: Add ethernet alias for GMAC

 .../bindings/net/allwinner,sun7i-gmac.txt          | 22 +++++++
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts        | 23 +++----
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts         |  8 +++
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts    | 23 +++----
 arch/arm/boot/dts/sun7i-a20.dtsi                   | 40 ++++++++++++
 drivers/net/ethernet/stmicro/stmmac/Kconfig        | 12 ++++
 drivers/net/ethernet/stmicro/stmmac/Makefile       |  1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  | 76 ++++++++++++++++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  3 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 24 ++++---
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  | 33 +++++++---
 11 files changed, 215 insertions(+), 50 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
 create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c

-- 
1.8.5.1

[PATCH 01/10] net: stmmac: Enable stmmac main clock when probing hardware

[Chen-Yu Tsai]

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---

Guiseppe previously stated that the "stmmaceth" clock is the
main clock that drives the IP. The stmmac driver does not
enable this clock during the probe phase. When the driver is
built in to the kernel, this is fine because the clock maybe
on by default, or the boot loader had enabled it.

If stmmac is built as a module, when the module is loaded,
the clock may be found unused and disabled by the kernel.

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 24 +++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8d4ccd3..7da71ed 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2688,10 +2688,17 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
 	if ((phyaddr >= 0) && (phyaddr <= 31))
 		priv->plat->phy_addr = phyaddr;
 
+	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
+	if (IS_ERR(priv->stmmac_clk)) {
+		pr_warn("%s: warning: cannot get CSR clock\n", __func__);
+		goto error_clk_get;
+	}
+	clk_prepare_enable(priv->stmmac_clk);
+
 	/* Init MAC and get the capabilities */
 	ret = stmmac_hw_init(priv);
 	if (ret)
-		goto error_free_netdev;
+		goto error_hw_init;
 
 	ndev->netdev_ops = &stmmac_netdev_ops;
 
@@ -2729,12 +2736,6 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
 		goto error_netdev_register;
 	}
 
-	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
-	if (IS_ERR(priv->stmmac_clk)) {
-		pr_warn("%s: warning: cannot get CSR clock\n", __func__);
-		goto error_clk_get;
-	}
-
 	/* If a specific clk_csr value is passed from the platform
 	 * this means that the CSR Clock Range selection cannot be
 	 * changed at run-time and it is fixed. Viceversa the driver'll try to
@@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
 		}
 	}
 
+	clk_disable_unprepare(priv->stmmac_clk);
+
 	return priv;
 
 error_mdio_register:
-	clk_put(priv->stmmac_clk);
-error_clk_get:
 	unregister_netdev(ndev);
 error_netdev_register:
 	netif_napi_del(&priv->napi);
-error_free_netdev:
+error_hw_init:
+	clk_disable_unprepare(priv->stmmac_clk);
+	clk_put(priv->stmmac_clk);
+error_clk_get:
 	free_netdev(ndev);
 
 	return NULL;
-- 
1.8.5.1

Re: [PATCH 01/10] net: stmmac: Enable stmmac main clock when probing hardware

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2827 bytes --]

Hi,

On Sat, Dec 07, 2013 at 01:29:34AM +0800, Chen-Yu Tsai wrote:
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> ---
> 
> Guiseppe previously stated that the "stmmaceth" clock is the
> main clock that drives the IP. The stmmac driver does not
> enable this clock during the probe phase. When the driver is
> built in to the kernel, this is fine because the clock maybe
> on by default, or the boot loader had enabled it.
> 
> If stmmac is built as a module, when the module is loaded,
> the clock may be found unused and disabled by the kernel.

This should be your commit log.

And this is actually not related to the fact that we are building it
as a module or not. You'd face the same issue with a statically built
kernel if the bootloader didn't enable it.

> 
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 24 +++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8d4ccd3..7da71ed 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2688,10 +2688,17 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>  	if ((phyaddr >= 0) && (phyaddr <= 31))
>  		priv->plat->phy_addr = phyaddr;
>  
> +	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);

You can probably use devm_clk_get to make the exit path smaller.

> +	if (IS_ERR(priv->stmmac_clk)) {
> +		pr_warn("%s: warning: cannot get CSR clock\n", __func__);

And dev_warn here.

> +		goto error_clk_get;
> +	}
> +	clk_prepare_enable(priv->stmmac_clk);
> +
>  	/* Init MAC and get the capabilities */
>  	ret = stmmac_hw_init(priv);
>  	if (ret)
> -		goto error_free_netdev;
> +		goto error_hw_init;
>  	ndev->netdev_ops = &stmmac_netdev_ops;
>  
> @@ -2729,12 +2736,6 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>  		goto error_netdev_register;
>  	}
>  
> -	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
> -	if (IS_ERR(priv->stmmac_clk)) {
> -		pr_warn("%s: warning: cannot get CSR clock\n", __func__);
> -		goto error_clk_get;
> -	}
> -
>  	/* If a specific clk_csr value is passed from the platform
>  	 * this means that the CSR Clock Range selection cannot be
>  	 * changed at run-time and it is fixed. Viceversa the driver'll try to
> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>  		}
>  	}
>  
> +	clk_disable_unprepare(priv->stmmac_clk);
> +

Hu? Why do you disable the clock? don't you need it afterwards?

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 01/10] net: stmmac: Enable stmmac main clock when probing hardware

[Chen-Yu Tsai]

Hi,

On Sat, Dec 7, 2013 at 6:33 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> On Sat, Dec 07, 2013 at 01:29:34AM +0800, Chen-Yu Tsai wrote:
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>>
>> Guiseppe previously stated that the "stmmaceth" clock is the
>> main clock that drives the IP. The stmmac driver does not
>> enable this clock during the probe phase. When the driver is
>> built in to the kernel, this is fine because the clock maybe
>> on by default, or the boot loader had enabled it.
>>
>> If stmmac is built as a module, when the module is loaded,
>> the clock may be found unused and disabled by the kernel.
>
> This should be your commit log.
>
> And this is actually not related to the fact that we are building it
> as a module or not. You'd face the same issue with a statically built
> kernel if the bootloader didn't enable it.

Noted. Will revise commit log.

>
>>
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 24 +++++++++++++----------
>>  1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 8d4ccd3..7da71ed 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -2688,10 +2688,17 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>>       if ((phyaddr >= 0) && (phyaddr <= 31))
>>               priv->plat->phy_addr = phyaddr;
>>
>> +     priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
>
> You can probably use devm_clk_get to make the exit path smaller.

As it stands, the driver does not call clk_put in the exit path.
Using devm_clk_get here would be a good fix.

>
>> +     if (IS_ERR(priv->stmmac_clk)) {
>> +             pr_warn("%s: warning: cannot get CSR clock\n", __func__);
>
> And dev_warn here.
>
>> +             goto error_clk_get;
>> +     }
>> +     clk_prepare_enable(priv->stmmac_clk);
>> +
>>       /* Init MAC and get the capabilities */
>>       ret = stmmac_hw_init(priv);
>>       if (ret)
>> -             goto error_free_netdev;
>> +             goto error_hw_init;
>>       ndev->netdev_ops = &stmmac_netdev_ops;
>>
>> @@ -2729,12 +2736,6 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>>               goto error_netdev_register;
>>       }
>>
>> -     priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
>> -     if (IS_ERR(priv->stmmac_clk)) {
>> -             pr_warn("%s: warning: cannot get CSR clock\n", __func__);
>> -             goto error_clk_get;
>> -     }
>> -
>>       /* If a specific clk_csr value is passed from the platform
>>        * this means that the CSR Clock Range selection cannot be
>>        * changed at run-time and it is fixed. Viceversa the driver'll try to
>> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>>               }
>>       }
>>
>> +     clk_disable_unprepare(priv->stmmac_clk);
>> +
>
> Hu? Why do you disable the clock? don't you need it afterwards?

The clock is enabled in *_open (when the network interface is used),
and disabled in *_close.

Re: Re: [PATCH 01/10] net: stmmac: Enable stmmac main clock when probing hardware

[Hans de Goede]

Hi Chen-Yu,

It seems this patch is not enough, gmac won't work for me
with this patchset (*), unless I boot with an uboot which
also has the gmac patches.

Regards,

Hans


*) see my sunxi-test tree where I've dropped your
old version and added this version.

Re: [PATCH 01/10] net: stmmac: Enable stmmac main clock when probing hardware

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

Hi,

On Mon, Dec 09, 2013 at 10:43:29AM +0800, Chen-Yu Tsai wrote:
> >> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
> >>               }
> >>       }
> >>
> >> +     clk_disable_unprepare(priv->stmmac_clk);
> >> +
> >
> > Hu? Why do you disable the clock? don't you need it afterwards?
> 
> The clock is enabled in *_open (when the network interface is used),
> and disabled in *_close.

Maybe it is the real issue then.

Why don't you move the clk_disable to _remove then?

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 01/10] net: stmmac: Enable stmmac main clock when probing hardware

[Chen-Yu Tsai]

Hi,

On Wed, Dec 11, 2013 at 4:05 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> On Mon, Dec 09, 2013 at 10:43:29AM +0800, Chen-Yu Tsai wrote:
>> >> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>> >>               }
>> >>       }
>> >>
>> >> +     clk_disable_unprepare(priv->stmmac_clk);
>> >> +
>> >
>> > Hu? Why do you disable the clock? don't you need it afterwards?
>>
>> The clock is enabled in *_open (when the network interface is used),
>> and disabled in *_close.
>
> Maybe it is the real issue then.
>
> Why don't you move the clk_disable to _remove then?

I wasn't sure this was the proper way. However, looking around, it
seems other drivers enable the clock in, _probe, and disable it in
_remove. I will modify stmmac to do so as well.


ChenYu

Re: [PATCH 01/10] net: stmmac: Enable stmmac main clock when probing hardware

[Giuseppe CAVALLARO]

Hello Chen-Yu

On 12/6/2013 6:29 PM, Chen-Yu Tsai wrote:
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>
> Guiseppe previously stated that the "stmmaceth" clock is the
> main clock that drives the IP. The stmmac driver does not
> enable this clock during the probe phase. When the driver is
> built in to the kernel, this is fine because the clock maybe
> on by default, or the boot loader had enabled it.
>
> If stmmac is built as a module, when the module is loaded,
> the clock may be found unused and disabled by the kernel.

the clk_prepare_enable is then called in the open.
Is it not working for you?
Do you mean that the stmmac_hw_init fails if you do not move
the clk_get and clk_prepare_enable on top of the stmmac_dvr_probe?

Peppe

>
>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 24 +++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8d4ccd3..7da71ed 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2688,10 +2688,17 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>   	if ((phyaddr >= 0) && (phyaddr <= 31))
>   		priv->plat->phy_addr = phyaddr;
>
> +	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
> +	if (IS_ERR(priv->stmmac_clk)) {
> +		pr_warn("%s: warning: cannot get CSR clock\n", __func__);
> +		goto error_clk_get;
> +	}
> +	clk_prepare_enable(priv->stmmac_clk);
> +
>   	/* Init MAC and get the capabilities */
>   	ret = stmmac_hw_init(priv);
>   	if (ret)
> -		goto error_free_netdev;
> +		goto error_hw_init;
>
>   	ndev->netdev_ops = &stmmac_netdev_ops;
>
> @@ -2729,12 +2736,6 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>   		goto error_netdev_register;
>   	}
>
> -	priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
> -	if (IS_ERR(priv->stmmac_clk)) {
> -		pr_warn("%s: warning: cannot get CSR clock\n", __func__);
> -		goto error_clk_get;
> -	}
> -
>   	/* If a specific clk_csr value is passed from the platform
>   	 * this means that the CSR Clock Range selection cannot be
>   	 * changed at run-time and it is fixed. Viceversa the driver'll try to
> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device *device,
>   		}
>   	}
>
> +	clk_disable_unprepare(priv->stmmac_clk);
> +
>   	return priv;
>
>   error_mdio_register:
> -	clk_put(priv->stmmac_clk);
> -error_clk_get:
>   	unregister_netdev(ndev);
>   error_netdev_register:
>   	netif_napi_del(&priv->napi);
> -error_free_netdev:
> +error_hw_init:
> +	clk_disable_unprepare(priv->stmmac_clk);
> +	clk_put(priv->stmmac_clk);
> +error_clk_get:
>   	free_netdev(ndev);
>
>   	return NULL;
>

Re: [PATCH 01/10] net: stmmac: Enable stmmac main clock when probing hardware

[Chen-Yu Tsai]

Hi Peppe,

On Mon, Dec 9, 2013 at 3:14 PM, Giuseppe CAVALLARO
<peppe.cavallaro-qxv4g6HH51o@public.gmane.org> wrote:
> Hello Chen-Yu
>
>
> On 12/6/2013 6:29 PM, Chen-Yu Tsai wrote:
>>
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>>
>> Guiseppe previously stated that the "stmmaceth" clock is the
>> main clock that drives the IP. The stmmac driver does not
>> enable this clock during the probe phase. When the driver is
>> built in to the kernel, this is fine because the clock maybe
>> on by default, or the boot loader had enabled it.
>>
>> If stmmac is built as a module, when the module is loaded,
>> the clock may be found unused and disabled by the kernel.
>
>
> the clk_prepare_enable is then called in the open.
> Is it not working for you?
> Do you mean that the stmmac_hw_init fails if you do not move
> the clk_get and clk_prepare_enable on top of the stmmac_dvr_probe?
>

Exactly. The clock needs to be enabled prior to stmmac_dvr_probe.
Otherwise, stmmac_mdio_register will fail to find a usable PHY.

The DWMAC core I am working with does not support chip ID or
HW capability flags. I suspect those would fail, too.

Disabling the clock at the end of stmmac_dvr_probe is to make
sure it plays nice with existing power management callbacks.

Chen-Yu

>>
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 24
>> +++++++++++++----------
>>   1 file changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 8d4ccd3..7da71ed 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -2688,10 +2688,17 @@ struct stmmac_priv *stmmac_dvr_probe(struct device
>> *device,
>>         if ((phyaddr >= 0) && (phyaddr <= 31))
>>                 priv->plat->phy_addr = phyaddr;
>>
>> +       priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
>> +       if (IS_ERR(priv->stmmac_clk)) {
>> +               pr_warn("%s: warning: cannot get CSR clock\n", __func__);
>> +               goto error_clk_get;
>> +       }
>> +       clk_prepare_enable(priv->stmmac_clk);
>> +
>>         /* Init MAC and get the capabilities */
>>         ret = stmmac_hw_init(priv);
>>         if (ret)
>> -               goto error_free_netdev;
>> +               goto error_hw_init;
>>
>>         ndev->netdev_ops = &stmmac_netdev_ops;
>>
>> @@ -2729,12 +2736,6 @@ struct stmmac_priv *stmmac_dvr_probe(struct device
>> *device,
>>                 goto error_netdev_register;
>>         }
>>
>> -       priv->stmmac_clk = clk_get(priv->device, STMMAC_RESOURCE_NAME);
>> -       if (IS_ERR(priv->stmmac_clk)) {
>> -               pr_warn("%s: warning: cannot get CSR clock\n", __func__);
>> -               goto error_clk_get;
>> -       }
>> -
>>         /* If a specific clk_csr value is passed from the platform
>>          * this means that the CSR Clock Range selection cannot be
>>          * changed at run-time and it is fixed. Viceversa the driver'll
>> try to
>> @@ -2759,15 +2760,18 @@ struct stmmac_priv *stmmac_dvr_probe(struct device
>> *device,
>>                 }
>>         }
>>
>> +       clk_disable_unprepare(priv->stmmac_clk);
>> +
>>         return priv;
>>
>>   error_mdio_register:
>> -       clk_put(priv->stmmac_clk);
>> -error_clk_get:
>>         unregister_netdev(ndev);
>>   error_netdev_register:
>>         netif_napi_del(&priv->napi);
>> -error_free_netdev:
>> +error_hw_init:
>> +       clk_disable_unprepare(priv->stmmac_clk);
>> +       clk_put(priv->stmmac_clk);
>> +error_clk_get:
>>         free_netdev(ndev);
>>
>>         return NULL;
>>
>

[PATCH 02/10] net: stmmac: Honor DT parameter to force DMA store and forward mode

[Chen-Yu Tsai]

"snps,force_sf_dma_mode" is documented in stmmac device tree bindings,
but is never handled by the driver.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 51c9069..74c7aef 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -47,6 +47,7 @@ static int stmmac_probe_config_dt(struct platform_device *pdev,
 		plat->bus_id = 0;
 
 	of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr);
+	plat->force_sf_dma_mode = of_property_read_bool(np, "snps,force_sf_dma_mode");
 
 	plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
 					   sizeof(struct stmmac_mdio_bus_data),
-- 
1.8.5.1

Re: [PATCH 02/10] net: stmmac: Honor DT parameter to force DMA store and forward mode

[David Miller]

From: Chen-Yu Tsai <wens@csie.org>
Date: Sat,  7 Dec 2013 01:29:35 +0800

> @@ -47,6 +47,7 @@ static int stmmac_probe_config_dt(struct platform_device *pdev,
>  		plat->bus_id = 0;
>  
>  	of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr);
> +	plat->force_sf_dma_mode = of_property_read_bool(np, "snps,force_sf_dma_mode");

Will this do the right thing for when the property is not present?
Right now the force_sf_dma_mode value is always false.

In fact won't it override the explicit settings done elsewhere in the
driver?

Re: [PATCH 02/10] net: stmmac: Honor DT parameter to force DMA store and forward mode

[Chen-Yu Tsai]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]

On Sat, Dec 7, 2013 at 5:26 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> Date: Sat,  7 Dec 2013 01:29:35 +0800
>
>> @@ -47,6 +47,7 @@ static int stmmac_probe_config_dt(struct
platform_device *pdev,
>>               plat->bus_id = 0;
>>
>>       of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr);
>> +     plat->force_sf_dma_mode = of_property_read_bool(np,
"snps,force_sf_dma_mode");
>
> Will this do the right thing for when the property is not present?
> Right now the force_sf_dma_mode value is always false.

of_property_read_bool will return false when the property is not present.

> In fact won't it override the explicit settings done elsewhere in the
> driver?

Point taken. The current implementation will override settings passed from
platform data. ORing the two would be better.


Thanks

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/groups/opt_out.

[-- Attachment #2: Type: text/html, Size: 1756 bytes --]

Re: [PATCH 02/10] net: stmmac: Honor DT parameter to force DMA store and forward mode

[Chen-Yu Tsai]

Resending reply due to incorrect MIME type.

On Sat, Dec 7, 2013 at 5:26 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
>
> From: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> Date: Sat,  7 Dec 2013 01:29:35 +0800
>
> > @@ -47,6 +47,7 @@ static int stmmac_probe_config_dt(struct platform_device *pdev,
> >               plat->bus_id = 0;
> >
> >       of_property_read_u32(np, "snps,phy-addr", &plat->phy_addr);
> > +     plat->force_sf_dma_mode = of_property_read_bool(np, "snps,force_sf_dma_mode");
>
> Will this do the right thing for when the property is not present?
> Right now the force_sf_dma_mode value is always false.

of_property_read_bool will return false when the property is not present.

> In fact won't it override the explicit settings done elsewhere in the
> driver?

Point taken. The current implementation will override settings passed from
platform data. ORing the two would be better.


Thanks

Re: [PATCH 02/10] net: stmmac: Honor DT parameter to force DMA store and forward mode

[maxime.ripard]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

On Sat, Dec 07, 2013 at 09:23:27AM +0800, Chen-Yu Tsai wrote:
> Point taken. The current implementation will override settings passed from
> platform data. ORing the two would be better.

Platform_data and DT-based configuration are pretty unlikely to be
used together, so ORing it doesn't have much sense.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 02/10] net: stmmac: Honor DT parameter to force DMA store and forward mode

[Tomasz Figa]

On Saturday 07 of December 2013 11:07:37 maxime.ripard wrote:
> On Sat, Dec 07, 2013 at 09:23:27AM +0800, Chen-Yu Tsai wrote:
> > Point taken. The current implementation will override settings passed from
> > platform data. ORing the two would be better.
> 
> Platform_data and DT-based configuration are pretty unlikely to be
> used together, so ORing it doesn't have much sense.

In fact, the recommended way is to always use platform data alone if it is
present or try to parse DT otherwise, so no mixing of data from these two
sources should be done.

Best regards,
Tomasz

Re: Re: [PATCH 02/10] net: stmmac: Honor DT parameter to force DMA store and forward mode

[Chen-Yu Tsai]

On Sat, Dec 7, 2013 at 7:06 PM, Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Saturday 07 of December 2013 11:07:37 maxime.ripard wrote:
>> On Sat, Dec 07, 2013 at 09:23:27AM +0800, Chen-Yu Tsai wrote:
>> > Point taken. The current implementation will override settings passed from
>> > platform data. ORing the two would be better.
>>
>> Platform_data and DT-based configuration are pretty unlikely to be
>> used together, so ORing it doesn't have much sense.
>
> In fact, the recommended way is to always use platform data alone if it is
> present or try to parse DT otherwise, so no mixing of data from these two
> sources should be done.

Would binding platform data with compatibles, as I did so in this patch
series, be a bad idea then?

Re: Re: [PATCH 02/10] net: stmmac: Honor DT parameter to force DMA store and forward mode

[maxime.ripard]

[-- Attachment #1: Type: text/plain, Size: 1274 bytes --]

On Mon, Dec 09, 2013 at 10:59:38AM +0800, Chen-Yu Tsai wrote:
> On Sat, Dec 7, 2013 at 7:06 PM, Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Saturday 07 of December 2013 11:07:37 maxime.ripard wrote:
> >> On Sat, Dec 07, 2013 at 09:23:27AM +0800, Chen-Yu Tsai wrote:
> >> > Point taken. The current implementation will override settings passed from
> >> > platform data. ORing the two would be better.
> >>
> >> Platform_data and DT-based configuration are pretty unlikely to be
> >> used together, so ORing it doesn't have much sense.
> >
> > In fact, the recommended way is to always use platform data alone if it is
> > present or try to parse DT otherwise, so no mixing of data from these two
> > sources should be done.
> 
> Would binding platform data with compatibles, as I did so in this patch
> series, be a bad idea then?

What I meant was that you'll either be probed will pdev->dev.of_node
or pdev->dev.platform_data filled, but not both at the same time, so
ORing it isn't really sensible.

But I don't see why you couldn't reuse the stmmac_platform_data
structure in your patch.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 03/10] net: stmmac: Use platform data tied with compatible strings

[Chen-Yu Tsai]

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  | 29 ++++++++++++++--------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 74c7aef..df3fd1c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -26,8 +26,19 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_net.h>
+#include <linux/of_device.h>
 #include "stmmac.h"
 
+static const struct of_device_id stmmac_dt_ids[] = {
+	{ .compatible = "st,spear600-gmac"},
+	{ .compatible = "snps,dwmac-3.610"},
+	{ .compatible = "snps,dwmac-3.70a"},
+	{ .compatible = "snps,dwmac-3.710"},
+	{ .compatible = "snps,dwmac"},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, stmmac_dt_ids);
+
 #ifdef CONFIG_OF
 static int stmmac_probe_config_dt(struct platform_device *pdev,
 				  struct plat_stmmacenet_data *plat,
@@ -35,10 +46,18 @@ static int stmmac_probe_config_dt(struct platform_device *pdev,
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct stmmac_dma_cfg *dma_cfg;
+	const struct of_device_id *device;
 
 	if (!np)
 		return -ENODEV;
 
+	device = of_match_device(stmmac_dt_ids, &pdev->dev);
+	if (!device)
+		return -ENODEV;
+
+	if (device->data)
+	    memcpy(plat, device->data, sizeof(*plat));
+
 	*mac = of_get_mac_address(np);
 	plat->interface = of_get_phy_mode(np);
 
@@ -257,16 +276,6 @@ static const struct dev_pm_ops stmmac_pltfr_pm_ops = {
 static const struct dev_pm_ops stmmac_pltfr_pm_ops;
 #endif /* CONFIG_PM */
 
-static const struct of_device_id stmmac_dt_ids[] = {
-	{ .compatible = "st,spear600-gmac"},
-	{ .compatible = "snps,dwmac-3.610"},
-	{ .compatible = "snps,dwmac-3.70a"},
-	{ .compatible = "snps,dwmac-3.710"},
-	{ .compatible = "snps,dwmac"},
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, stmmac_dt_ids);
-
 struct platform_driver stmmac_pltfr_driver = {
 	.probe = stmmac_pltfr_probe,
 	.remove = stmmac_pltfr_remove,
-- 
1.8.5.1

Re: [PATCH 03/10] net: stmmac: Use platform data tied with compatible strings

[David Miller]

From: Chen-Yu Tsai <wens@csie.org>
Date: Sat,  7 Dec 2013 01:29:36 +0800

> +	device = of_match_device(stmmac_dt_ids, &pdev->dev);
> +	if (!device)
> +		return -ENODEV;
> +
> +	if (device->data)
> +	    memcpy(plat, device->data, sizeof(*plat));
> +

Don't invent your own indentation using spaces, this memcpy()
call is not indented properly, it should be two TABs.

Re: [PATCH 03/10] net: stmmac: Use platform data tied with compatible strings

[Chen-Yu Tsai]

On Sat, Dec 7, 2013 at 5:26 AM, David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> wrote:
> From: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> Date: Sat,  7 Dec 2013 01:29:36 +0800
>
>> +     device = of_match_device(stmmac_dt_ids, &pdev->dev);
>> +     if (!device)
>> +             return -ENODEV;
>> +
>> +     if (device->data)
>> +         memcpy(plat, device->data, sizeof(*plat));
>> +
>
> Don't invent your own indentation using spaces, this memcpy()
> call is not indented properly, it should be two TABs.

Noted. Will fix.

[PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Chen-Yu Tsai]

The Allwinner A20 has an ethernet controller that seems to be
an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
which is supported by the stmmac driver.

Allwinner's GMAC requires setting additional registers in the SoC's
clock control unit.

The exact version of the DWMAC IP that Allwinner uses is unknown,
thus the exact feature set is unknown.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 .../bindings/net/allwinner,sun7i-gmac.txt          | 22 +++++++
 drivers/net/ethernet/stmicro/stmmac/Kconfig        | 12 ++++
 drivers/net/ethernet/stmicro/stmmac/Makefile       |  1 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  | 76 ++++++++++++++++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  3 +
 .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  3 +
 6 files changed, 117 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
new file mode 100644
index 0000000..271554a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
@@ -0,0 +1,22 @@
+* Allwinner GMAC ethernet controller
+
+This device is a platform glue layer for stmmac.
+Please see stmmac.txt for the other unchanged properties.
+
+Required properties:
+ - compatible:  Should be "allwinner,sun7i-gmac"
+ - reg: Address and length of register set for the device and corresponding
+   clock control
+
+Examples:
+
+	gmac: ethernet@01c50000 {
+		compatible = "allwinner,sun7i-gmac";
+		reg = <0x01c50000 0x10000>,
+		      <0x01c20164 0x4>;
+		interrupts = <0 85 1>;
+		interrupt-names = "macirq";
+		clocks = <&ahb_gates 49>;
+		clock-names = "stmmaceth";
+		phy-mode = "mii";
+	};
diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 6e52c0f..6d71210 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -25,6 +25,18 @@ config STMMAC_PLATFORM
 
 	  If unsure, say N.
 
+config DWMAC_SUNXI
+	bool "Allwinner GMAC support"
+	depends on STMMAC_PLATFORM
+	depends on ARCH_SUNXI
+	default y
+	---help---
+	  Support for Allwinner A20 GMAC ethernet driver.
+
+	  This selects Allwinner SoC glue layer support for the
+	  stmmac device driver. This driver is used for A20 GMAC
+	  ethernet controller.
+
 config STMMAC_PCI
 	bool "STMMAC PCI bus support"
 	depends on STMMAC_ETH && PCI
diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
index 356a9dd..ecadece 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Makefile
+++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
@@ -1,6 +1,7 @@
 obj-$(CONFIG_STMMAC_ETH) += stmmac.o
 stmmac-$(CONFIG_STMMAC_PLATFORM) += stmmac_platform.o
 stmmac-$(CONFIG_STMMAC_PCI) += stmmac_pci.o
+stmmac-$(CONFIG_DWMAC_SUNXI) += dwmac-sunxi.o
 stmmac-objs:= stmmac_main.o stmmac_ethtool.o stmmac_mdio.o ring_mode.o	\
 	      chain_mode.o dwmac_lib.o dwmac1000_core.o  dwmac1000_dma.o \
 	      dwmac100_core.o dwmac100_dma.o enh_desc.o  norm_desc.o \
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
new file mode 100644
index 0000000..6c9fdb0
--- /dev/null
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
@@ -0,0 +1,76 @@
+/**
+ * dwmac-sunxi.c - Allwinner sunxi DWMAC specific glue layer
+ *
+ * Copyright (C) 2013 Chen-Yu Tsai
+ *
+ * Chen-Yu Tsai  <wens-jdAy2FN1RRM@public.gmane.org>
+ *
+ * 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 <linux/clk.h>
+#include <linux/phy.h>
+#include <linux/stmmac.h>
+
+#define GMAC_IF_TYPE_RGMII	0x4
+
+#define GMAC_TX_CLK_MASK	0x3
+#define GMAC_TX_CLK_MII		0x0
+#define GMAC_TX_CLK_RGMII_INT	0x2
+
+static int sun7i_gmac_init(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct device *dev = &pdev->dev;
+	void __iomem *addr = NULL;
+	struct plat_stmmacenet_data *plat_dat = NULL;
+	u32 priv_clk_reg;
+
+	plat_dat = dev_get_platdata(&pdev->dev);
+	if (!plat_dat)
+		return -EINVAL;
+
+	/* Get GMAC clock register in CCU */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	addr = devm_ioremap_resource(dev, res);
+	if (IS_ERR(addr))
+		return PTR_ERR(addr);
+
+	priv_clk_reg = readl(addr);
+
+	/* Set GMAC interface port mode */
+	if (plat_dat->interface == PHY_INTERFACE_MODE_RGMII)
+		priv_clk_reg |= GMAC_IF_TYPE_RGMII;
+	else
+		priv_clk_reg &= ~GMAC_IF_TYPE_RGMII;
+
+	/* Set GMAC transmit clock source. */
+	priv_clk_reg &= ~GMAC_TX_CLK_MASK;
+	if (plat_dat->interface == PHY_INTERFACE_MODE_RGMII
+			|| plat_dat->interface == PHY_INTERFACE_MODE_GMII)
+		priv_clk_reg |= GMAC_TX_CLK_RGMII_INT;
+	else
+		priv_clk_reg |= GMAC_TX_CLK_MII;
+
+	writel(priv_clk_reg, addr);
+
+	/* mask out phy addr 0x0 */
+	plat_dat->mdio_bus_data->phy_mask = 0x1;
+
+	return 0;
+}
+
+const struct plat_stmmacenet_data sun7i_gmac_data = {
+	.has_gmac = 1,
+	.tx_coe = 1,
+	.init = sun7i_gmac_init,
+};
+
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index f16a9bd..c6e1f93 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -130,6 +130,9 @@ void stmmac_disable_eee_mode(struct stmmac_priv *priv);
 bool stmmac_eee_init(struct stmmac_priv *priv);
 
 #ifdef CONFIG_STMMAC_PLATFORM
+#ifdef CONFIG_DWMAC_SUNXI
+extern const struct plat_stmmacenet_data sun7i_gmac_data;
+#endif
 extern struct platform_driver stmmac_pltfr_driver;
 static inline int stmmac_register_platform(void)
 {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index df3fd1c..6cf8292 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -35,6 +35,9 @@ static const struct of_device_id stmmac_dt_ids[] = {
 	{ .compatible = "snps,dwmac-3.70a"},
 	{ .compatible = "snps,dwmac-3.710"},
 	{ .compatible = "snps,dwmac"},
+#ifdef CONFIG_DWMAC_SUNXI
+	{ .compatible = "allwinner,sun7i-gmac", .data = &sun7i_gmac_data},
+#endif
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, stmmac_dt_ids);
-- 
1.8.5.1

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2531 bytes --]

Chen-Yu, Mike,

On Sat, Dec 07, 2013 at 01:29:37AM +0800, Chen-Yu Tsai wrote:
> The Allwinner A20 has an ethernet controller that seems to be
> an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
> which is supported by the stmmac driver.
> 
> Allwinner's GMAC requires setting additional registers in the SoC's
> clock control unit.
> 
> The exact version of the DWMAC IP that Allwinner uses is unknown,
> thus the exact feature set is unknown.
> 
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> ---
>  .../bindings/net/allwinner,sun7i-gmac.txt          | 22 +++++++
>  drivers/net/ethernet/stmicro/stmmac/Kconfig        | 12 ++++
>  drivers/net/ethernet/stmicro/stmmac/Makefile       |  1 +
>  drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  | 76 ++++++++++++++++++++++
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  3 +
>  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  3 +
>  6 files changed, 117 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
> new file mode 100644
> index 0000000..271554a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
> @@ -0,0 +1,22 @@
> +* Allwinner GMAC ethernet controller
> +
> +This device is a platform glue layer for stmmac.
> +Please see stmmac.txt for the other unchanged properties.
> +
> +Required properties:
> + - compatible:  Should be "allwinner,sun7i-gmac"

Please use sun7i-a20-gmac here.

> + - reg: Address and length of register set for the device and corresponding
> +   clock control
>
> +Examples:
> +
> +	gmac: ethernet@01c50000 {
> +		compatible = "allwinner,sun7i-gmac";
> +		reg = <0x01c50000 0x10000>,
> +		      <0x01c20164 0x4>;

This is actually a clock, and should probably be registered in the
common clock framework.

Mike: This small register actually is a regular muxer/divider, except
that it has some bits that are of interest to the ethernet controller
(for example to set wether it's using GMII or RGMII to communicate
with the phy), that, as far as I'm aware of, aren't really fitting
into the CCF.

Do you have some recommendation on how to proceed?

Maybe make a thin "real" clock driver in this hardware glue, that
provides !exported function to set this *GMII thing.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Tomasz Figa]

On Saturday 07 of December 2013 11:27:10 Maxime Ripard wrote:
> Chen-Yu, Mike,
> 
> On Sat, Dec 07, 2013 at 01:29:37AM +0800, Chen-Yu Tsai wrote:
> > The Allwinner A20 has an ethernet controller that seems to be
> > an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
> > which is supported by the stmmac driver.
> > 
> > Allwinner's GMAC requires setting additional registers in the SoC's
> > clock control unit.
> > 
> > The exact version of the DWMAC IP that Allwinner uses is unknown,
> > thus the exact feature set is unknown.
> > 
> > Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> > ---
> >  .../bindings/net/allwinner,sun7i-gmac.txt          | 22 +++++++
> >  drivers/net/ethernet/stmicro/stmmac/Kconfig        | 12 ++++
> >  drivers/net/ethernet/stmicro/stmmac/Makefile       |  1 +
> >  drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  | 76 ++++++++++++++++++++++
> >  drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  3 +
> >  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  3 +
> >  6 files changed, 117 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
> > new file mode 100644
> > index 0000000..271554a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
> > @@ -0,0 +1,22 @@
> > +* Allwinner GMAC ethernet controller
> > +
> > +This device is a platform glue layer for stmmac.
> > +Please see stmmac.txt for the other unchanged properties.
> > +
> > +Required properties:
> > + - compatible:  Should be "allwinner,sun7i-gmac"
> 
> Please use sun7i-a20-gmac here.
> 
> > + - reg: Address and length of register set for the device and corresponding
> > +   clock control
> >
> > +Examples:
> > +
> > +	gmac: ethernet@01c50000 {
> > +		compatible = "allwinner,sun7i-gmac";
> > +		reg = <0x01c50000 0x10000>,
> > +		      <0x01c20164 0x4>;
> 
> This is actually a clock, and should probably be registered in the
> common clock framework.
> 
> Mike: This small register actually is a regular muxer/divider, except
> that it has some bits that are of interest to the ethernet controller
> (for example to set wether it's using GMII or RGMII to communicate
> with the phy), that, as far as I'm aware of, aren't really fitting
> into the CCF.
> 
> Do you have some recommendation on how to proceed?
> 
> Maybe make a thin "real" clock driver in this hardware glue, that
> provides !exported function to set this *GMII thing.

Is this register part of a bigger IP block that manages clocks for other
IP blocks than stmmac as well? If not, I don't see a point of exporting
a clock from inside of the GMAC "domain" just to feed it back into it
as the only user.

Still, without any actual knowledge about this register and registers
next to it, I can be only guessing.

Best regards,
Tomasz

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3355 bytes --]

On Sat, Dec 07, 2013 at 12:12:26PM +0100, Tomasz Figa wrote:
> On Saturday 07 of December 2013 11:27:10 Maxime Ripard wrote:
> > Chen-Yu, Mike,
> > 
> > On Sat, Dec 07, 2013 at 01:29:37AM +0800, Chen-Yu Tsai wrote:
> > > The Allwinner A20 has an ethernet controller that seems to be
> > > an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
> > > which is supported by the stmmac driver.
> > > 
> > > Allwinner's GMAC requires setting additional registers in the SoC's
> > > clock control unit.
> > > 
> > > The exact version of the DWMAC IP that Allwinner uses is unknown,
> > > thus the exact feature set is unknown.
> > > 
> > > Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> > > ---
> > >  .../bindings/net/allwinner,sun7i-gmac.txt          | 22 +++++++
> > >  drivers/net/ethernet/stmicro/stmmac/Kconfig        | 12 ++++
> > >  drivers/net/ethernet/stmicro/stmmac/Makefile       |  1 +
> > >  drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  | 76 ++++++++++++++++++++++
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  3 +
> > >  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  3 +
> > >  6 files changed, 117 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
> > > new file mode 100644
> > > index 0000000..271554a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
> > > @@ -0,0 +1,22 @@
> > > +* Allwinner GMAC ethernet controller
> > > +
> > > +This device is a platform glue layer for stmmac.
> > > +Please see stmmac.txt for the other unchanged properties.
> > > +
> > > +Required properties:
> > > + - compatible:  Should be "allwinner,sun7i-gmac"
> > 
> > Please use sun7i-a20-gmac here.
> > 
> > > + - reg: Address and length of register set for the device and corresponding
> > > +   clock control
> > >
> > > +Examples:
> > > +
> > > +	gmac: ethernet@01c50000 {
> > > +		compatible = "allwinner,sun7i-gmac";
> > > +		reg = <0x01c50000 0x10000>,
> > > +		      <0x01c20164 0x4>;
> > 
> > This is actually a clock, and should probably be registered in the
> > common clock framework.
> > 
> > Mike: This small register actually is a regular muxer/divider, except
> > that it has some bits that are of interest to the ethernet controller
> > (for example to set wether it's using GMII or RGMII to communicate
> > with the phy), that, as far as I'm aware of, aren't really fitting
> > into the CCF.
> > 
> > Do you have some recommendation on how to proceed?
> > 
> > Maybe make a thin "real" clock driver in this hardware glue, that
> > provides !exported function to set this *GMII thing.
> 
> Is this register part of a bigger IP block that manages clocks for other
> IP blocks than stmmac as well? If not, I don't see a point of exporting
> a clock from inside of the GMAC "domain" just to feed it back into it
> as the only user.

This register is actually part of the SoC clock controller. So it sits
right beside the other clocks registers controlling the clocks of the
other devices, and is not part of the GMAC IP itself.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Tomasz Figa]

On Saturday 07 of December 2013 12:46:16 Maxime Ripard wrote:
> On Sat, Dec 07, 2013 at 12:12:26PM +0100, Tomasz Figa wrote:
> > On Saturday 07 of December 2013 11:27:10 Maxime Ripard wrote:
> > > Chen-Yu, Mike,
> > > 
> > > On Sat, Dec 07, 2013 at 01:29:37AM +0800, Chen-Yu Tsai wrote:
> > > > The Allwinner A20 has an ethernet controller that seems to be
> > > > an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
> > > > which is supported by the stmmac driver.
> > > > 
> > > > Allwinner's GMAC requires setting additional registers in the SoC's
> > > > clock control unit.
> > > > 
> > > > The exact version of the DWMAC IP that Allwinner uses is unknown,
> > > > thus the exact feature set is unknown.
> > > > 
> > > > Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> > > > ---
> > > >  .../bindings/net/allwinner,sun7i-gmac.txt          | 22 +++++++
> > > >  drivers/net/ethernet/stmicro/stmmac/Kconfig        | 12 ++++
> > > >  drivers/net/ethernet/stmicro/stmmac/Makefile       |  1 +
> > > >  drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  | 76 ++++++++++++++++++++++
> > > >  drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  3 +
> > > >  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  3 +
> > > >  6 files changed, 117 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
> > > > new file mode 100644
> > > > index 0000000..271554a
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
> > > > @@ -0,0 +1,22 @@
> > > > +* Allwinner GMAC ethernet controller
> > > > +
> > > > +This device is a platform glue layer for stmmac.
> > > > +Please see stmmac.txt for the other unchanged properties.
> > > > +
> > > > +Required properties:
> > > > + - compatible:  Should be "allwinner,sun7i-gmac"
> > > 
> > > Please use sun7i-a20-gmac here.
> > > 
> > > > + - reg: Address and length of register set for the device and corresponding
> > > > +   clock control
> > > >
> > > > +Examples:
> > > > +
> > > > +	gmac: ethernet@01c50000 {
> > > > +		compatible = "allwinner,sun7i-gmac";
> > > > +		reg = <0x01c50000 0x10000>,
> > > > +		      <0x01c20164 0x4>;
> > > 
> > > This is actually a clock, and should probably be registered in the
> > > common clock framework.
> > > 
> > > Mike: This small register actually is a regular muxer/divider, except
> > > that it has some bits that are of interest to the ethernet controller
> > > (for example to set wether it's using GMII or RGMII to communicate
> > > with the phy), that, as far as I'm aware of, aren't really fitting
> > > into the CCF.
> > > 
> > > Do you have some recommendation on how to proceed?
> > > 
> > > Maybe make a thin "real" clock driver in this hardware glue, that
> > > provides !exported function to set this *GMII thing.
> > 
> > Is this register part of a bigger IP block that manages clocks for other
> > IP blocks than stmmac as well? If not, I don't see a point of exporting
> > a clock from inside of the GMAC "domain" just to feed it back into it
> > as the only user.
> 
> This register is actually part of the SoC clock controller. So it sits
> right beside the other clocks registers controlling the clocks of the
> other devices, and is not part of the GMAC IP itself.

Is there any description for GMAC_IF_TYPE_RGMII and GMAC_TX_CLK fields?

Name of the latter sounds like a normal clock mux, but the former is
just a mystery (especially why it is a part of the clock controller).

Best regards,
Tomasz

Re: Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Emilio López]

El 07/12/13 09:50, Tomasz Figa escribió:
> On Saturday 07 of December 2013 12:46:16 Maxime Ripard wrote:
>> On Sat, Dec 07, 2013 at 12:12:26PM +0100, Tomasz Figa wrote:
>>> On Saturday 07 of December 2013 11:27:10 Maxime Ripard wrote:
>>>> Chen-Yu, Mike,
>>>>
>>>> On Sat, Dec 07, 2013 at 01:29:37AM +0800, Chen-Yu Tsai wrote:
>>>>> The Allwinner A20 has an ethernet controller that seems to be
>>>>> an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
>>>>> which is supported by the stmmac driver.
>>>>>
>>>>> Allwinner's GMAC requires setting additional registers in the SoC's
>>>>> clock control unit.
>>>>>
>>>>> The exact version of the DWMAC IP that Allwinner uses is unknown,
>>>>> thus the exact feature set is unknown.
>>>>>
>>>>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>>>>> ---
>>>>>   .../bindings/net/allwinner,sun7i-gmac.txt          | 22 +++++++
>>>>>   drivers/net/ethernet/stmicro/stmmac/Kconfig        | 12 ++++
>>>>>   drivers/net/ethernet/stmicro/stmmac/Makefile       |  1 +
>>>>>   drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  | 76 ++++++++++++++++++++++
>>>>>   drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  3 +
>>>>>   .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  3 +
>>>>>   6 files changed, 117 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
>>>>> new file mode 100644
>>>>> index 0000000..271554a
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
>>>>> @@ -0,0 +1,22 @@
>>>>> +* Allwinner GMAC ethernet controller
>>>>> +
>>>>> +This device is a platform glue layer for stmmac.
>>>>> +Please see stmmac.txt for the other unchanged properties.
>>>>> +
>>>>> +Required properties:
>>>>> + - compatible:  Should be "allwinner,sun7i-gmac"
>>>>
>>>> Please use sun7i-a20-gmac here.
>>>>
>>>>> + - reg: Address and length of register set for the device and corresponding
>>>>> +   clock control
>>>>>
>>>>> +Examples:
>>>>> +
>>>>> +	gmac: ethernet@01c50000 {
>>>>> +		compatible = "allwinner,sun7i-gmac";
>>>>> +		reg = <0x01c50000 0x10000>,
>>>>> +		      <0x01c20164 0x4>;
>>>>
>>>> This is actually a clock, and should probably be registered in the
>>>> common clock framework.
>>>>
>>>> Mike: This small register actually is a regular muxer/divider, except
>>>> that it has some bits that are of interest to the ethernet controller
>>>> (for example to set wether it's using GMII or RGMII to communicate
>>>> with the phy), that, as far as I'm aware of, aren't really fitting
>>>> into the CCF.
>>>>
>>>> Do you have some recommendation on how to proceed?
>>>>
>>>> Maybe make a thin "real" clock driver in this hardware glue, that
>>>> provides !exported function to set this *GMII thing.
>>>
>>> Is this register part of a bigger IP block that manages clocks for other
>>> IP blocks than stmmac as well? If not, I don't see a point of exporting
>>> a clock from inside of the GMAC "domain" just to feed it back into it
>>> as the only user.
>>
>> This register is actually part of the SoC clock controller. So it sits
>> right beside the other clocks registers controlling the clocks of the
>> other devices, and is not part of the GMAC IP itself.
>
> Is there any description for GMAC_IF_TYPE_RGMII and GMAC_TX_CLK fields?
>
> Name of the latter sounds like a normal clock mux, but the former is
> just a mystery (especially why it is a part of the clock controller).

You can find the register documented on page 89, table 1.5.4.65 of

http://dl.linux-sunxi.org/A20/A20%20User%20Manual%202013-03-22.pdf

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[srinivas kandagatla]

Hi Chen,
Good to know that Allwinner uses gmac.

On ST SoC, we have very similar requirements, before we merge any of
these changes I think we need to come up with common way to solve both
Allwinner and ST SOCs use cases.

I have already posted few patches on to net-dev
http://lkml.org/lkml/2013/11/12/243 to add Glue driver on top of stmmac
driver.


There are few reasons for the way I have done it.

1> I did not want to modify gmac driver in any sense to when a new SOC
support is added.
2> As the SOC specific glue logic sits on top of standard GMAC IP, it
makes sense to represent it proper harware hierarchy.
3> Did not want to change any generic gmac bindings for SOC specific
stuff as requirements if SOC glue would be very different in each case.

I see issues with this approach, which are addressed in my patches.

Please have a look at the http://lkml.org/lkml/2013/11/12/462 patch on
STi SOC glue, which does implement a very similar glue driver.

Do you see any issues not to do that way?

On 06/12/13 17:29, Chen-Yu Tsai wrote:
> The Allwinner A20 has an ethernet controller that seems to be
> an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
> which is supported by the stmmac driver.
> 
> Allwinner's GMAC requires setting additional registers in the SoC's
> clock control unit.
> 
> The exact version of the DWMAC IP that Allwinner uses is unknown,
> thus the exact feature set is unknown.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  .../bindings/net/allwinner,sun7i-gmac.txt          | 22 +++++++
>  drivers/net/ethernet/stmicro/stmmac/Kconfig        | 12 ++++
>  drivers/net/ethernet/stmicro/stmmac/Makefile       |  1 +
>  drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  | 76 ++++++++++++++++++++++
>  drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  3 +lot
>  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  3 +
>  6 files changed, 117 insertions(+)
> http://lkml.org/lkml/2013/11/12/462
> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt

> --- /dev/null
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
> @@ -0,0 +1,76 @@
> +/**
> + * dwmac-sunxi.c - Allwinner sunxi DWMAC specific glue layer
> + *
> + * Copyright (C) 2013 Chen-Yu Tsai
> + *
> + * Chen-Yu Tsai  <wens@csie.org>
> + *
> + * 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, orhttp://lkml.org/lkml/2013/11/12/462
> + * (at your option) any later version.
> + *http://lkml.org/lkml/2013/11/12/462
> + * This program is distributed in the hope that it will be useful,http://lkml.org/lkml/2013/11/12/462
> + * 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 <linux/clk.h>
> +#include <linux/phy.h>
> +#include <linux/stmmac.h>
> +
> +#define GMAC_IF_TYPE_RGMII	0x4
> +
> +#define GMAC_TX_CLK_MASK	0x3
> +#define GMAC_TX_CLK_MII		0x0
> +#define GMAC_TX_CLK_RGMII_INT	0x2
> +
> +static int sun7i_gmac_init(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *addr = NULL;
> +	struct plat_stmmacenet_data *plat_dat = NULL;
> +	u32 priv_clk_reg;
> +
> +	plat_dat = dev_get_platdata(&pdev->dev);
This is not valid for DT case.
In DT case stmmac maintains its own instance of platform data.

Setting platform data dynamically after probe to a device is not the
right way to do.

> +	if (!plat_dat)
> +		return -EINVAL;
> +
> +	/* Get GMAC clock register in CCU */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	addr = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(addr))
> +		return PTR_ERR(addr);
> +
> +	priv_clk_reg = readl(addr);
> +
> +	/* Set GMAC interface port mode */
> +	if (plat_dat->interface == PHY_INTERFACE_MODE_RGMII)
> +		priv_clk_reg |= GMAC_IF_TYPE_RGMII;
> +	else
> +		priv_clk_reg &= ~GMAC_IF_TYPE_RGMII;
> +
> +	/* Set GMAC transmit clock source. */
> +	priv_clk_reg &= ~GMAC_TX_CLK_MASK;
> +	if (plat_dat->interface == PHY_INTERFACE_MODE_RGMII
> +			|| plat_dat->interface == PHY_INTERFACE_MODE_GMII)
> +		priv_clk_reg |= GMAC_TX_CLK_RGMII_INT;
> +	else
> +		priv_clk_reg |= GMAC_TX_CLK_MII;
> +
> +	writel(priv_clk_reg, addr);
> +
> +	/* mask out phy addr 0x0 */
> +	plat_dat->mdio_bus_data->phy_mask = 0x1;
> +per
> +	return 0;
> +}
> +
This routine would not scale..

stmmac can call init function multiple times, so you would be parsing DT
and also remapping the address multiple times.


[---
> +const struct plat_stmmacenet_data sun7i_gmac_data = {
> +	.has_gmac = 1,
> +	.tx_coe = 1,
> +	.init = sun7i_gmac_init,
> +};
> +
---]

This looks exactly like a AUXDATA way of doing things.
Again stmmac driver has to make a copy of this so that I would not get a
same copy for multiple instances.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index f16a9bd..c6e1f93 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -130,6 +130,9 @@ void stmmac_disable_eee_mode(struct stmmac_priv *priv);
>  bool stmmac_eee_init(struct stmmac_priv *priv);
>  


[...
>  #ifdef CONFIG_STMMAC_PLATFORM
> +#ifdef CONFIG_DWMAC_SUNXI
> +extern const struct plat_stmmacenet_data sun7i_gmac_data;
> +#endif
>  extern struct platform_driver stmmac_pltfr_driver;
>  static inline int stmmac_register_platform(void)
>  {
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index df3fd1c..6cf8292 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -35,6 +35,9 @@ static const struct of_device_id stmmac_dt_ids[] = {
>  	{ .compatible = "snps,dwmac-3.70a"},
>  	{ .compatible = "snps,dwmac-3.710"},
>  	{ .compatible = "snps,dwmac"},
> +#ifdef CONFIG_DWMAC_SUNXI
> +	{ .compatible = "allwinner,sun7i-gmac", .data = &sun7i_gmac_data},
> +#endif
...]

Personally, I did not want to do this kind of stuff in stmmac, As this
list would keep growing, and this file need to be edited for every new
SOC or every different type of glue logic.

Please let me know what your opnion on doing Allwinner glue driver in
line with http://lkml.org/lkml/2013/11/12/462


Thanks,
srini

>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, stmmac_dt_ids);
> 

Re: Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Hans de Goede]

Hi,

On 12/09/2013 12:10 PM, srinivas kandagatla wrote:
> Hi Chen,
> Good to know that Allwinner uses gmac.
>
> On ST SoC, we have very similar requirements, before we merge any of
> these changes I think we need to come up with common way to solve both
> Allwinner and ST SOCs use cases.
>
> I have already posted few patches on to net-dev
> http://lkml.org/lkml/2013/11/12/243 to add Glue driver on top of stmmac
> driver.
>
>
> There are few reasons for the way I have done it.
>
> 1> I did not want to modify gmac driver in any sense to when a new SOC
> support is added.
> 2> As the SOC specific glue logic sits on top of standard GMAC IP, it
> makes sense to represent it proper harware hierarchy.

On top often is not the correct term / how things are done in practice.

Most of the time the glue are modifications to the ip block, iow in
hardware they are not nested / hierarchical at all. We've had similar
constructs for ahci platform drivers and there we are actively trying to
move away from the whole nested platform devices as that has various
issues:

1) It is wrong / does not reflect reality
2) It breaks deferred probing which is often used on SOCs

I actually think Wens' approach using a SOC specific init function
in platform data is sound, and this is also used a lot else where.

As for using the nested approach elsewhere, I only know about AHCI
platform driver doing that, and there we are actively trying to move
away from it.


Now reading this has also made me take a closer look at wens' patch
for this. Wens, I see that you directly modify registers in the ccm
that is a big no-no instead you should add a helper function to
sunxi-clk.c and use that, see ie:
https://bitbucket.org/emiliolopez/linux/commits/2b95847d9aa4aa13317dd7358ffcbd951dcb5eff?at=master


Giuseppe, I get the feeling the 0x148 registers st-gmac is using is
the same story, but I could be wrong. One important reason why accessing
regs like this directly is a big no no is because it is impossible to
do proper locking between multiple users, so 2 users doing read / modify / write
could end up racing. So accesses to such registers should always happen
through a single driver.


Regards,

Hans

Re: Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Chen-Yu Tsai]

Hi,

On Tue, Dec 10, 2013 at 12:16 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Hi,
>
>
> On 12/09/2013 12:10 PM, srinivas kandagatla wrote:
>>
>> Hi Chen,
>> Good to know that Allwinner uses gmac.
>>
>> On ST SoC, we have very similar requirements, before we merge any of
>> these changes I think we need to come up with common way to solve both
>> Allwinner and ST SOCs use cases.
>>
>> I have already posted few patches on to net-dev
>> http://lkml.org/lkml/2013/11/12/243 to add Glue driver on top of stmmac
>> driver.
>>
>>
>> There are few reasons for the way I have done it.
>>
>> 1> I did not want to modify gmac driver in any sense to when a new SOC
>> support is added.
>> 2> As the SOC specific glue logic sits on top of standard GMAC IP, it
>> makes sense to represent it proper harware hierarchy.
>
>
> On top often is not the correct term / how things are done in practice.
>
> Most of the time the glue are modifications to the ip block, iow in
> hardware they are not nested / hierarchical at all. We've had similar
> constructs for ahci platform drivers and there we are actively trying to
> move away from the whole nested platform devices as that has various
> issues:
>
> 1) It is wrong / does not reflect reality
> 2) It breaks deferred probing which is often used on SOCs
>
> I actually think Wens' approach using a SOC specific init function
> in platform data is sound, and this is also used a lot else where.
>
> As for using the nested approach elsewhere, I only know about AHCI
> platform driver doing that, and there we are actively trying to move
> away from it.
>
>
> Now reading this has also made me take a closer look at wens' patch
> for this. Wens, I see that you directly modify registers in the ccm
> that is a big no-no instead you should add a helper function to
> sunxi-clk.c and use that, see ie:
> https://bitbucket.org/emiliolopez/linux/commits/2b95847d9aa4aa13317dd7358ffcbd951dcb5eff?at=master

Yes, this has been raised by Maxime. The odd "GMAC_IF_TYPE_RGMII" or
"gmac interface type bit" has been bugging me.

Additionally, the TX clock has 2 inputs (not counting MII [1]).
The internal one is most likely controlled by the GMAC. The clock
rate is set internally to match the link speed. The external clock
source has controllable dividers to get the correct clock rate.
This shouldn't be hard to model with CCF though.

In hardware, this is probably a mux between the GMAC clock generator
and the GMAC data transmit logic.

My current plan is to choose MII when the clock is disabled,
and choose either of the inputs when it is enabled. I will
have to learn more about the CCF first.

[1] In MII mode, the TX clock is sent by the PHY to the MAC.
    From the MAC's viewpoint, it seems like a clock source.
    However, from the PHY's viewpoint, it is the provider.
    In RGMII mode, the MAC provides the TX clock to the PHY,
    and times the TX data lines to the clock. So in a way,
    the PHY is the true consumer of the TX clock.


ChenYu

> Giuseppe, I get the feeling the 0x148 registers st-gmac is using is
> the same story, but I could be wrong. One important reason why accessing
> regs like this directly is a big no no is because it is impossible to
> do proper locking between multiple users, so 2 users doing read / modify /
> write
> could end up racing. So accesses to such registers should always happen
> through a single driver.
>
>
> Regards,
>
> Hans

Re: Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Hans de Goede]

Hi,

On 12/09/2013 06:56 PM, Chen-Yu Tsai wrote:
> Hi,
>
> On Tue, Dec 10, 2013 at 12:16 AM, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> Hi,
>>
>>
>> On 12/09/2013 12:10 PM, srinivas kandagatla wrote:
>>>
>>> Hi Chen,
>>> Good to know that Allwinner uses gmac.
>>>
>>> On ST SoC, we have very similar requirements, before we merge any of
>>> these changes I think we need to come up with common way to solve both
>>> Allwinner and ST SOCs use cases.
>>>
>>> I have already posted few patches on to net-dev
>>> http://lkml.org/lkml/2013/11/12/243 to add Glue driver on top of stmmac
>>> driver.
>>>
>>>
>>> There are few reasons for the way I have done it.
>>>
>>> 1> I did not want to modify gmac driver in any sense to when a new SOC
>>> support is added.
>>> 2> As the SOC specific glue logic sits on top of standard GMAC IP, it
>>> makes sense to represent it proper harware hierarchy.
>>
>>
>> On top often is not the correct term / how things are done in practice.
>>
>> Most of the time the glue are modifications to the ip block, iow in
>> hardware they are not nested / hierarchical at all. We've had similar
>> constructs for ahci platform drivers and there we are actively trying to
>> move away from the whole nested platform devices as that has various
>> issues:
>>
>> 1) It is wrong / does not reflect reality
>> 2) It breaks deferred probing which is often used on SOCs
>>
>> I actually think Wens' approach using a SOC specific init function
>> in platform data is sound, and this is also used a lot else where.
>>
>> As for using the nested approach elsewhere, I only know about AHCI
>> platform driver doing that, and there we are actively trying to move
>> away from it.
>>
>>
>> Now reading this has also made me take a closer look at wens' patch
>> for this. Wens, I see that you directly modify registers in the ccm
>> that is a big no-no instead you should add a helper function to
>> sunxi-clk.c and use that, see ie:
>> https://bitbucket.org/emiliolopez/linux/commits/2b95847d9aa4aa13317dd7358ffcbd951dcb5eff?at=master
>
> Yes, this has been raised by Maxime. The odd "GMAC_IF_TYPE_RGMII" or
> "gmac interface type bit" has been bugging me.
>
> Additionally, the TX clock has 2 inputs (not counting MII [1]).
> The internal one is most likely controlled by the GMAC. The clock
> rate is set internally to match the link speed. The external clock
> source has controllable dividers to get the correct clock rate.
> This shouldn't be hard to model with CCF though.
>
> In hardware, this is probably a mux between the GMAC clock generator
> and the GMAC data transmit logic.
>
> My current plan is to choose MII when the clock is disabled,
> and choose either of the inputs when it is enabled. I will
> have to learn more about the CCF first.

OK, in this case I would be tempted to just go with a custom sunxi
function in the sunx-clk mode like what we have for the mmc stuff,
but if you think you can model this with the regular clock stuff,
that is of course fine too :)

Regards,

Hans

Re: Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1781 bytes --]

On Mon, Dec 09, 2013 at 08:04:21PM +0100, Hans de Goede wrote:
> >>Now reading this has also made me take a closer look at wens' patch
> >>for this. Wens, I see that you directly modify registers in the ccm
> >>that is a big no-no instead you should add a helper function to
> >>sunxi-clk.c and use that, see ie:
> >>https://bitbucket.org/emiliolopez/linux/commits/2b95847d9aa4aa13317dd7358ffcbd951dcb5eff?at=master
> >
> >Yes, this has been raised by Maxime. The odd "GMAC_IF_TYPE_RGMII" or
> >"gmac interface type bit" has been bugging me.
> >
> >Additionally, the TX clock has 2 inputs (not counting MII [1]).
> >The internal one is most likely controlled by the GMAC. The clock
> >rate is set internally to match the link speed. The external clock
> >source has controllable dividers to get the correct clock rate.
> >This shouldn't be hard to model with CCF though.
> >
> >In hardware, this is probably a mux between the GMAC clock generator
> >and the GMAC data transmit logic.
> >
> >My current plan is to choose MII when the clock is disabled,
> >and choose either of the inputs when it is enabled. I will
> >have to learn more about the CCF first.
> 
> OK, in this case I would be tempted to just go with a custom sunxi
> function in the sunx-clk mode like what we have for the mmc stuff,
> but if you think you can model this with the regular clock stuff,
> that is of course fine too :)

Note that it's also ok to implement the clock driver out of
drivers/clk if it makes more sense.

So, if it's more convenient for you to declare both drivers (clock and
gmac), say in the glue file, I'm totally fine whith that.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Chen-Yu Tsai]

Hi,

On Mon, Dec 9, 2013 at 7:10 PM, srinivas kandagatla
<srinivas.kandagatla-qxv4g6HH51o@public.gmane.org> wrote:
> Hi Chen,
> Good to know that Allwinner uses gmac.

To my knowledge, Allwinner has never confirmed this.

> On ST SoC, we have very similar requirements, before we merge any of
> these changes I think we need to come up with common way to solve both
> Allwinner and ST SOCs use cases.
>
> I have already posted few patches on to net-dev
> http://lkml.org/lkml/2013/11/12/243 to add Glue driver on top of stmmac
> driver.
>
>
> There are few reasons for the way I have done it.
>
> 1> I did not want to modify gmac driver in any sense to when a new SOC
> support is added.

My approach would add one line (compatible strings) each time.

> 2> As the SOC specific glue logic sits on top of standard GMAC IP, it
> makes sense to represent it proper harware hierarchy.

My feeling is that the SoC specifics are not a glue layer around dwmac,
rather an interconnected extension. Arguably I do not have the whole
picture. Allwinner has refused to provide us with any specifics beyond
the SoC manual and drivers. As such I can only make educated guesses on
how the dwmac core interacts with the other modules.

> 3> Did not want to change any generic gmac bindings for SOC specific
> stuff as requirements if SOC glue would be very different in each case.

We could try to define a standard set of clocks and reset controllers.
The dwmac does not have much additional tunables in the driver.
Anything else should be SoC specific, put in an extension, and
documented as such.

> I see issues with this approach, which are addressed in my patches.
>
> Please have a look at the http://lkml.org/lkml/2013/11/12/462 patch on
> STi SOC glue, which does implement a very similar glue driver.
>
> Do you see any issues not to do that way?

The glue layer driver would not have access to any of dwmac driver's
internals, nor could it respond to any state changes.

For example, .fix_mac_speed is called whenever auto-negotiation happens,
or when the link goes down or up. Maybe some future SoC would need this.

The Allwinner A20 has the option to use the external 125MHz clock
provided by the PHY for the tx clock under RGMII mode, with multiple
dividers yielding 125, 25, or 2.5 MHz. It seems this would require
.fix_mac_speed, though I have not tested it. My goal was to produce
a usable driver, rather than test all the possibilities.

The other issue is when the glue layer needs to set SoC specific
implementation details of the dwmac, when it is not possible to
add a generic dwmac compatible. See next section for more on this.

> On 06/12/13 17:29, Chen-Yu Tsai wrote:
>> The Allwinner A20 has an ethernet controller that seems to be
>> an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
>> which is supported by the stmmac driver.
>>
>> Allwinner's GMAC requires setting additional registers in the SoC's
>> clock control unit.
>>
>> The exact version of the DWMAC IP that Allwinner uses is unknown,
>> thus the exact feature set is unknown.
>>
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>>  .../bindings/net/allwinner,sun7i-gmac.txt          | 22 +++++++
>>  drivers/net/ethernet/stmicro/stmmac/Kconfig        | 12 ++++
>>  drivers/net/ethernet/stmicro/stmmac/Makefile       |  1 +
>>  drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  | 76 ++++++++++++++++++++++
>>  drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  3 +lot
>>  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  3 +
>>  6 files changed, 117 insertions(+)
>> http://lkml.org/lkml/2013/11/12/462
>> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
>
>> --- /dev/null
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
>> @@ -0,0 +1,76 @@
>> +/**
>> + * dwmac-sunxi.c - Allwinner sunxi DWMAC specific glue layer
>> + *
>> + * Copyright (C) 2013 Chen-Yu Tsai
>> + *
>> + * Chen-Yu Tsai  <wens-jdAy2FN1RRM@public.gmane.org>
>> + *
>> + * 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, orhttp://lkml.org/lkml/2013/11/12/462
>> + * (at your option) any later version.
>> + *http://lkml.org/lkml/2013/11/12/462
>> + * This program is distributed in the hope that it will be useful,http://lkml.org/lkml/2013/11/12/462
>> + * 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 <linux/clk.h>
>> +#include <linux/phy.h>
>> +#include <linux/stmmac.h>
>> +
>> +#define GMAC_IF_TYPE_RGMII   0x4
>> +
>> +#define GMAC_TX_CLK_MASK     0x3
>> +#define GMAC_TX_CLK_MII              0x0
>> +#define GMAC_TX_CLK_RGMII_INT        0x2
>> +
>> +static int sun7i_gmac_init(struct platform_device *pdev)
>> +{
>> +     struct resource *res;
>> +     struct device *dev = &pdev->dev;
>> +     void __iomem *addr = NULL;
>> +     struct plat_stmmacenet_data *plat_dat = NULL;
>> +     u32 priv_clk_reg;
>> +
>> +     plat_dat = dev_get_platdata(&pdev->dev);
> This is not valid for DT case.
> In DT case stmmac maintains its own instance of platform data.
>
> Setting platform data dynamically after probe to a device is not the
> right way to do.

I see. And stmmac's own instance of platform data is associated with the
device in stmmac_dvr_probe. To make this work, we would have to move
setting dev->priv->plat in front of the .init callback.

>> +     if (!plat_dat)
>> +             return -EINVAL;
>> +
>> +     /* Get GMAC clock register in CCU */
>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +     addr = devm_ioremap_resource(dev, res);
>> +     if (IS_ERR(addr))
>> +             return PTR_ERR(addr);
>> +
>> +     priv_clk_reg = readl(addr);
>> +
>> +     /* Set GMAC interface port mode */
>> +     if (plat_dat->interface == PHY_INTERFACE_MODE_RGMII)
>> +             priv_clk_reg |= GMAC_IF_TYPE_RGMII;
>> +     else
>> +             priv_clk_reg &= ~GMAC_IF_TYPE_RGMII;
>> +
>> +     /* Set GMAC transmit clock source. */
>> +     priv_clk_reg &= ~GMAC_TX_CLK_MASK;
>> +     if (plat_dat->interface == PHY_INTERFACE_MODE_RGMII
>> +                     || plat_dat->interface == PHY_INTERFACE_MODE_GMII)
>> +             priv_clk_reg |= GMAC_TX_CLK_RGMII_INT;
>> +     else
>> +             priv_clk_reg |= GMAC_TX_CLK_MII;
>> +
>> +     writel(priv_clk_reg, addr);
>> +
>> +     /* mask out phy addr 0x0 */
>> +     plat_dat->mdio_bus_data->phy_mask = 0x1;
>> +per
>> +     return 0;
>> +}
>> +
> This routine would not scale..
>
> stmmac can call init function multiple times, so you would be parsing DT
> and also remapping the address multiple times.

I should save the mapped address, and check if it was mapped before.
Or we could split it into .probe and .init, where .probe is call in
stmmac_pltfr_probe, and .init is called in stmmac_open.

> [---
>> +const struct plat_stmmacenet_data sun7i_gmac_data = {
>> +     .has_gmac = 1,
>> +     .tx_coe = 1,
>> +     .init = sun7i_gmac_init,
>> +};
>> +
> ---]
>
> This looks exactly like a AUXDATA way of doing things.
> Again stmmac driver has to make a copy of this so that I would not get a
> same copy for multiple instances.

In the patch series, stmmac will make a copy.
Though that part needs some fixes.

On a side note, the impression I got for not using AUXDATA is that it pushes
what is supposed to be driver extensions to the SoC/board specific code areas.

>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> index f16a9bd..c6e1f93 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>> @@ -130,6 +130,9 @@ void stmmac_disable_eee_mode(struct stmmac_priv *priv);
>>  bool stmmac_eee_init(struct stmmac_priv *priv);
>>
>
>
> [...
>>  #ifdef CONFIG_STMMAC_PLATFORM
>> +#ifdef CONFIG_DWMAC_SUNXI
>> +extern const struct plat_stmmacenet_data sun7i_gmac_data;
>> +#endif
>>  extern struct platform_driver stmmac_pltfr_driver;
>>  static inline int stmmac_register_platform(void)
>>  {
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> index df3fd1c..6cf8292 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>> @@ -35,6 +35,9 @@ static const struct of_device_id stmmac_dt_ids[] = {
>>       { .compatible = "snps,dwmac-3.70a"},
>>       { .compatible = "snps,dwmac-3.710"},
>>       { .compatible = "snps,dwmac"},
>> +#ifdef CONFIG_DWMAC_SUNXI
>> +     { .compatible = "allwinner,sun7i-gmac", .data = &sun7i_gmac_data},
>> +#endif
> ...]
>
> Personally, I did not want to do this kind of stuff in stmmac, As this
> list would keep growing, and this file need to be edited for every new
> SOC or every different type of glue logic.
>
> Please let me know what your opnion on doing Allwinner glue driver in
> line with http://lkml.org/lkml/2013/11/12/462


I needed to set some fields in the platform data,
as was set in the driver provided by Allwinner, and
out of our own needs.

1. .tx_coe
   This is not exported in the DT bindings.
   Looking at stmmac code, not setting this seems to disable all
   checksum offloading.

2. .force_sf_dma_mode
   I have added DT property handling for this.

3. .mdio_bus_data
   The PHY mask is something we would like to have at the board level.
   The PHY on one of our boards, RTL8211E from Realtek, responds to
   commands sent to phy address 0, regardless what it's actual address is.
   This is documented in RTL8211E's datasheet:

      The RTL8211E-VB(VL)/RTL8211EG-VB support PHY addresses from 00001 to
      00111. PHY address 0 is a broadcast from the MAC; each PHY device
      should respond.

   Without the PHY mask, probing the MDIO bus would yield two devices,
   when in reality there is only one. This results in some confusion.

   This should definitely be exported as a DT binding.

I am also unable to add a generic compatible string for our particular
dwmac, because I do not know the exact version of the IP, only that
it is earlier than 3.50. No hardware DMA capability flags.

Using something like "snps,dwmac-pre-3.50" is IMO worse.

>>       { /* sentinel */ }
>>  };
>>  MODULE_DEVICE_TABLE(of, stmmac_dt_ids);
>>
>

ChenYu

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[srinivas kandagatla]

On 09/12/13 17:34, Chen-Yu Tsai wrote:
> Hi,
> 
> On Mon, Dec 9, 2013 at 7:10 PM, srinivas kandagatla
> <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org> wrote:
>> Hi Chen,
>> Good to know that Allwinner uses gmac.
> 
> To my knowledge, Allwinner has never confirmed this.
> 
>> On ST SoC, we have very similar requirements, before we merge any of
>> these changes I think we need to come up with common way to solve both
>> Allwinner and ST SOCs use cases.
>>
>> I have already posted few patches on to net-dev
>> http://lkml.org/lkml/2013/11/12/243 to add Glue driver on top of stmmac
>> driver.
>>
>>
>> There are few reasons for the way I have done it.
>>
>> 1> I did not want to modify gmac driver in any sense to when a new SOC
>> support is added.
> 
> My approach would add one line (compatible strings) each time.

This would be a best case, But in reality the defination of the
configuration register can change per each soc, so the data associated
with it will also change and hence you will have to change more than
then just compatible strings.


> 
>> 2> As the SOC specific glue logic sits on top of standard GMAC IP, it
>> makes sense to represent it proper harware hierarchy.
> 
> My feeling is that the SoC specifics are not a glue layer around dwmac,
> rather an interconnected extension. Arguably I do not have the whole
> picture. Allwinner has refused to provide us with any specifics beyond
> the SoC manual and drivers. As such I can only make educated guesses on
> how the dwmac core interacts with the other modules.

I would be good to get actual picture of this hw setup, On ST the
additional glue logic which sits on top of the GMAC is to resposible for
selecting the correct retime clock.

> 
>> 3> Did not want to change any generic gmac bindings for SOC specific
>> stuff as requirements if SOC glue would be very different in each case.
> 
> We could try to define a standard set of clocks and reset controllers.
> The dwmac does not have much additional tunables in the driver.
> Anything else should be SoC specific, put in an extension, and
> documented as such.

I think it will be impossible to standardize SOC specifics as this
glue/logic keeps changing across SOCs/Vendors.

> 
>> I see issues with this approach, which are addressed in my patches.
>>
>> Please have a look at the http://lkml.org/lkml/2013/11/12/462 patch on
>> STi SOC glue, which does implement a very similar glue driver.
>>
>> Do you see any issues not to do that way?
> 
> The glue layer driver would not have access to any of dwmac driver's
> internals, nor could it respond to any state changes.

Should it have access to this in the first place?
Should SOC Glue driver actually change the internal data structures of
the stmmac? I guess No.

I think all you need is to know the interface type which can be derived
from a child node as I did in my glue driver.

> 
> For example, .fix_mac_speed is called whenever auto-negotiation happens,
> or when the link goes down or up. Maybe some future SoC would need this.#
Makes sense..

Yes, On ST we need this too, but max-speed would limit the device
capability. But I think its more complicated than just setting up
divider, most of the configuration depends on board hw setup.

Ex: TX clk can come from external clk or phyclk or txclk itself...

So the call I have taken here is to support only speeds based on the
max-speed property.

> 
> The Allwinner A20 has the option to use the external 125MHz clock
> provided by the PHY for the tx clock under RGMII mode, with multiple
> dividers yielding 125, 25, or 2.5 MHz. It seems this would require

Why should not the this be represented in a proper clock( "phyclk" )
which can be setup and driven the mac driver it self.

> .fix_mac_speed, though I have not tested it. My goal was to produce
> a usable driver, rather than test all the possibilities.
> 
> The other issue is when the glue layer needs to set SoC specific
> implementation details of the dwmac, when it is not possible to
> add a generic dwmac compatible. See next section for more on this.
> 
>> On 06/12/13 17:29, Chen-Yu Tsai wrote:as 
>>> The Allwinner A20 has an ethernet controller that seems to be
>>> an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
>>> which is supported by the stmmac driver.
>>>
>>> Allwinner's GMAC requires setting additional registers in the SoC's
>>> clock control unit.
>>>
>>> The exact version of the DWMAC IP that Allwinner uses is unknown,
>>> thus the exact feature set is unknown.
>>>
>>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>>> ---
>>>  .../bindings/net/allwinner,sun7i-gmac.txt          | 22 +++++++
>>>  drivers/net/ethernet/stmicro/stmmac/Kconfig        | 12 ++++
>>>  drivers/net/ethernet/stmicro/stmmac/Makefile       |  1 +
>>>  drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  | 76 ++++++++++++++++++++++
>>>  drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  3 +lot
>>>  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  3 +
>>>  6 files changed, 117 insertions(+)
>>> http://lkml.org/lkml/2013/11/12/462
>>> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
>>
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
>>> @@ -0,0 +1,76 @@
>>> +/**
>>> + * dwmac-sunxi.c - Allwinner sunxi DWMAC specific glue layer
>>> + *
>>> + * Copyright (C) 2013 Chen-Yu Tsai
>>> + *
>>> + * Chen-Yu Tsai  <wens-jdAy2FN1RRM@public.gmane.org>
>>> + *
>>> + * 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, orhttp://lkml.org/lkml/2013/11/12/462
>>> + * (at your option) any later version.
>>> + *http://lkml.org/lkml/2013/11/12/462
>>> + * This program is distributed in the hope that it will be useful,http://lkml.org/lkml/2013/11/12/462
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty ofas 
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/phy.h>
>>> +#include <linux/stmmac.h>
>>> +
>>> +#define GMAC_IF_TYPE_RGMII   0x4
>>> +
>>> +#define GMAC_TX_CLK_MASK     0x3
>>> +#define GMAC_TX_CLK_MII              0x0
>>> +#define GMAC_TX_CLK_RGMII_INT        0x2
>>> +
>>> +static int sun7i_gmac_init(struct platform_device *pdev)
>>> +{
>>> +     struct resource *res;
>>> +     struct device *dev = &pdev->dev;
>>> +     void __iomem *addr = NULL;
>>> +     struct plat_stmmacenet_data *plat_dat = NULL;
>>> +     u32 priv_clk_reg;
>>> +
>>> +     plat_dat = dev_get_platdata(&pdev->dev);
>> This is not valid for DT case.
>> In DT case stmmac maintains its own instance of platform data.
>>
>> Setting platform data dynamically after probe to a device is not the
>> right way to do.
> 
> I see. And stmmac's own instance of platform data is associated with the
> device in stmmac_dvr_probe. To make this work, we would have to move
> setting dev->priv->plat in front of the .init callback.

No it would not work either, as platform data is stmmac is stored in its
private data structure.

> 
>>> +     if (!plat_dat)as 
>>> +             return -EINVAL;
>>> +
>>> +     /* Get GMAC clock register in CCU */
>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>> +     addr = devm_ioremap_resource(dev, res);
>>> +     if (IS_ERR(addr))
>>> +             return PTR_ERR(addr);
>>> +
>>> +     priv_clk_reg = readl(addr);
>>> +
>>> +     /* Set GMAC interface port mode */
>>> +     if (plat_dat->interface == PHY_INTERFACE_MODE_RGMII)
>>> +             priv_clk_reg |= GMAC_IF_TYPE_RGMII;
>>> +     else
>>> +             priv_clk_reg &= ~GMAC_IF_TYPE_RGMII;
>>> +
>>> +     /* Set GMAC transmit clock source. */
>>> +     priv_clk_reg &= ~GMAC_TX_CLK_MASK;
>>> +     if (plat_dat->interface == PHY_INTERFACE_MODE_RGMII
>>> +                     || plat_dat->interface == PHY_INTERFACE_MODE_GMII)
>>> +             priv_clk_reg |= GMAC_TX_CLK_RGMII_INT;
>>> +     else
>>> +             priv_clk_reg |= GMAC_TX_CLK_MII;
>>> +
>>> +     writel(priv_clk_reg, addr);
>>> +
>>> +     /* mask out phy addr 0x0 */
>>> +     plat_dat->mdio_bus_data->phy_mask = 0x1;
>>> +per
>>> +     return 0;
>>> +}
>>> +
>> This routine would not scale..
>>
>> stmmac can call init function multiple times, so you would be parsing DT
>> and also remapping the address multiple times.
> 
> I should save the mapped address, and check if it was mapped before.
> Or we could split it into .probe and .init, where .probe is call in
> stmmac_pltfr_probe, and .init is called in stmmac_open.
> 
>> [---
>>> +const struct plat_stmmacenet_data sun7i_gmac_data = {
>>> +     .has_gmac = 1,
>>> +     .tx_coe = 1,
>>> +     .init = sun7i_gmac_init,
>>> +};
>>> +
>> ---]
>>
>> This looks exactly like a AUXDATA way of doing things.
>> Again stmmac driver has to make a copy of this so that I would not get a
>> same copy for multiple instances.
> 
> In the patch series, stmmac will make a copy.
> Though that part needs some fixes.
> 
> On a side note, the impression I got for not using AUXDATA is that it pushes
> what is supposed to be driver extensions to the SoC/board specific code areas.
> 
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>>> index f16a9bd..c6e1f93 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>>> @@ -130,6 +130,9 @@ void stmmac_disable_eee_mode(struct stmmac_priv *priv);
>>>  bool stmmac_eee_init(struct stmmac_priv *priv);
>>>
>>
>>
>> [...
>>>  #ifdef CONFIG_STMMAC_PLATFORM
>>> +#ifdef CONFIG_DWMAC_SUNXI
>>> +extern const struct plat_stmmacenet_data sun7i_gmac_data;
>>> +#endif
>>>  extern struct platform_driver stmmac_pltfr_driver;
>>>  static inline int stmmac_register_platform(void)
>>>  {
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> index df3fd1c..6cf8292 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> @@ -35,6 +35,9 @@ static const struct of_device_id stmmac_dt_ids[] = {
>>>       { .compatible = "snps,dwmac-3.70a"},
>>>       { .compatible = "snps,dwmac-3.710"},
>>>       { .compatible = "snps,dwmac"},
>>> +#ifdef CONFIG_DWMAC_SUNXI
>>> +     { .compatible = "allwinner,sun7i-gmac", .data = &sun7i_gmac_data},
>>> +#endif
>> ...]
>>
>> Personally, I did not want to do this kind of stuff in stmmac, As this
>> list would keep growing, and this file need to be edited for every new
>> SOC or every different type of glue logic.
>>
>> Please let me know what your opnion on doing Allwinner glue driver in
>> line with http://lkml.org/lkml/2013/11/12/462
> 
> 
> I needed to set some fields in the platform data,
> as was set in the driver provided by Allwinner, and
> out of our own needs.

> 
> 1. .tx_coe
>    This is not exported in the DT bindings.
>    Looking at stmmac code, not setting this seems to disable all
>    checksum offloading.

Why cant this go via DT as well?
I think, this will be overriden by stmmac on MACs > 3.50a.

> 
> 2. .force_sf_dma_mode
>    I have added DT property handling for this.
Ok,
> 
> 3. .mdio_bus_data
>    The PHY mask is something we would like to have at the board level.
>    The PHY on one of our boards, RTL8211E from Realtek, responds to
>    commands sent to phy address 0, regardless what it's actual address is.

Sounds correct.as

>    This is documented in RTL8211E's datasheet:
> 
>       The RTL8211E-VB(VL)/RTL8211EG-VB support PHY addresses from 00001 to
>       00111. PHY address 0 is a broadcast from the MAC; each PHY device
>       should respond.
> 
>    Without the PHY mask, probing the MDIO bus would yield two devices,
>    when in reality there is only one. This results in some confusion.
Can you explain the issue in more detail.

Why is it a confusion? PHYs are supposed to respond to address 00000.


MDIO bus could have multiple PHYS on the bus, it does not matter as long
as you address them with correct PHYID.


Thanks,
srini
> 
>    This should definitely be exported as a DT binding.
> 
> I am also unable to add a generic compatible string for our particular
> dwmac, because I do not know the exact version of the IP, only that
> it is earlier than 3.50. No hardware DMA capability flags.
> 
> Using something like "snps,dwmac-pre-3.50" is IMO worse.
> 
>>>       { /* sentinel */ }
>>>  };
>>>  MODULE_DEVICE_TABLE(of, stmmac_dt_ids);
>>>
>>
> 
> ChenYu
> 
> 

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4463 bytes --]

On Tue, Dec 10, 2013 at 02:59:57PM +0000, srinivas kandagatla wrote:
> 
> On 09/12/13 17:34, Chen-Yu Tsai wrote:
> > Hi,
> > 
> > On Mon, Dec 9, 2013 at 7:10 PM, srinivas kandagatla
> > <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org> wrote:
> >> Hi Chen,
> >> Good to know that Allwinner uses gmac.
> > 
> > To my knowledge, Allwinner has never confirmed this.
> > 
> >> On ST SoC, we have very similar requirements, before we merge any of
> >> these changes I think we need to come up with common way to solve both
> >> Allwinner and ST SOCs use cases.
> >>
> >> I have already posted few patches on to net-dev
> >> http://lkml.org/lkml/2013/11/12/243 to add Glue driver on top of stmmac
> >> driver.
> >>
> >>
> >> There are few reasons for the way I have done it.
> >>
> >> 1> I did not want to modify gmac driver in any sense to when a new SOC
> >> support is added.
> > 
> > My approach would add one line (compatible strings) each time.
> 
> This would be a best case, But in reality the defination of the
> configuration register can change per each soc, so the data associated
> with it will also change and hence you will have to change more than
> then just compatible strings.
> 
> 
> > 
> >> 2> As the SOC specific glue logic sits on top of standard GMAC IP, it
> >> makes sense to represent it proper harware hierarchy.
> > 
> > My feeling is that the SoC specifics are not a glue layer around dwmac,
> > rather an interconnected extension. Arguably I do not have the whole
> > picture. Allwinner has refused to provide us with any specifics beyond
> > the SoC manual and drivers. As such I can only make educated guesses on
> > how the dwmac core interacts with the other modules.
> 
> I would be good to get actual picture of this hw setup, On ST the
> additional glue logic which sits on top of the GMAC is to resposible for
> selecting the correct retime clock.
> 
> > 
> >> 3> Did not want to change any generic gmac bindings for SOC specific
> >> stuff as requirements if SOC glue would be very different in each case.
> > 
> > We could try to define a standard set of clocks and reset controllers.
> > The dwmac does not have much additional tunables in the driver.
> > Anything else should be SoC specific, put in an extension, and
> > documented as such.
> 
> I think it will be impossible to standardize SOC specifics as this
> glue/logic keeps changing across SOCs/Vendors.

In our case, it's barely just an additional clock (and a reset line in
the not-yet-submitted A31 GMAC).

So I can't really get what is !standard about that. We already have
defined bindings for such cases, that we can totally make optionnal.

We don't have anything like you seem to have in ST SoCs, where you
actually have some hardware around the GMAC IP. We don't. It's really
only about clocks and reset.

[...]

> >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> >>> @@ -35,6 +35,9 @@ static const struct of_device_id stmmac_dt_ids[] = {
> >>>       { .compatible = "snps,dwmac-3.70a"},
> >>>       { .compatible = "snps,dwmac-3.710"},
> >>>       { .compatible = "snps,dwmac"},
> >>> +#ifdef CONFIG_DWMAC_SUNXI
> >>> +     { .compatible = "allwinner,sun7i-gmac", .data = &sun7i_gmac_data},
> >>> +#endif
> >> ...]
> >>
> >> Personally, I did not want to do this kind of stuff in stmmac, As this
> >> list would keep growing, and this file need to be edited for every new
> >> SOC or every different type of glue logic.
> >>
> >> Please let me know what your opnion on doing Allwinner glue driver in
> >> line with http://lkml.org/lkml/2013/11/12/462
> > 
> > 
> > I needed to set some fields in the platform data,
> > as was set in the driver provided by Allwinner, and
> > out of our own needs.
> 
> > 
> > 1. .tx_coe
> >    This is not exported in the DT bindings.
> >    Looking at stmmac code, not setting this seems to disable all
> >    checksum offloading.
> 
> Why cant this go via DT as well?
> I think, this will be overriden by stmmac on MACs > 3.50a.

How is that about hardware? Especially if we can derive it from the
version of the IP, and hence from the compatible, putting it into the
DT would be both redundant and useless.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Chen-Yu Tsai]

Hi,

On Tue, Dec 10, 2013 at 10:59 PM, srinivas kandagatla
<srinivas.kandagatla-qxv4g6HH51o@public.gmane.org> wrote:
>
> On 09/12/13 17:34, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Mon, Dec 9, 2013 at 7:10 PM, srinivas kandagatla
>> <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org> wrote:
>>> Hi Chen,
>>> Good to know that Allwinner uses gmac.
>>
>> To my knowledge, Allwinner has never confirmed this.
>>
>>> On ST SoC, we have very similar requirements, before we merge any of
>>> these changes I think we need to come up with common way to solve both
>>> Allwinner and ST SOCs use cases.
>>>
>>> I have already posted few patches on to net-dev
>>> http://lkml.org/lkml/2013/11/12/243 to add Glue driver on top of stmmac
>>> driver.
>>>
>>>
>>> There are few reasons for the way I have done it.
>>>
>>> 1> I did not want to modify gmac driver in any sense to when a new SOC
>>> support is added.
>>
>> My approach would add one line (compatible strings) each time.
>
> This would be a best case, But in reality the defination of the
> configuration register can change per each soc, so the data associated
> with it will also change and hence you will have to change more than
> then just compatible strings.

This could also be delegated to .data with an extra data structure.
See drivers/i2c/busses/i2c-mv64xxx.c for an example, which was done
by Maxime.

>>> 2> As the SOC specific glue logic sits on top of standard GMAC IP, it
>>> makes sense to represent it proper harware hierarchy.
>>
>> My feeling is that the SoC specifics are not a glue layer around dwmac,
>> rather an interconnected extension. Arguably I do not have the whole
>> picture. Allwinner has refused to provide us with any specifics beyond
>> the SoC manual and drivers. As such I can only make educated guesses on
>> how the dwmac core interacts with the other modules.
>
> I would be good to get actual picture of this hw setup, On ST the
> additional glue logic which sits on top of the GMAC is to resposible for
> selecting the correct retime clock.

I would have liked to look at the internal design, how the dwmac core
is connected to the clock control, but that is out of the question.
Still, based on the documents, I think our clock controller is partially
intertwined with the GMAC. It takes GMAC's internally generated clock
as one of several inputs, then sends it back to the GMAC to time tx data.

Judging by the register definitions listed in the A20 manual,
the SoC glue layer clocks is something like this:

                               _________________________
  MII TX clock from PHY >-----|___________    _________|----> to GMAC core
  GMAC Int. RGMII TX clk >----|___________\__/__gate---|----> to PHY
  Ext. 125MHz RGMII TX clk >--|__divider__/            |
                              |________________________|


For MII mode, the glue layer should select the TX clock from the PHY.
The gate to the PHY should be disabled.

For RGMII mode, either the internal clock generated by the GMAC core,
or the external 125MHz reference generated by the PHY can be selected.
And the clock gate to the PHY should be enabled.
If the 125MHz reference is used, the glue layer should select the proper
divider (/1, /5, /50) based on the link speed.

For GMII mode, under 10/100 speeds, the operation matches MII mode.
For gigabit speeds, should use a 125MHz clock (internal or external)
and enable the output gate.

The glue layer may indeed sit on top or around the GMAC core.
Nevertheless, its operational state does depend on the GMAC.
The current callbacks present in the stmmac driver are a good model
for this.

>>> 3> Did not want to change any generic gmac bindings for SOC specific
>>> stuff as requirements if SOC glue would be very different in each case.
>>
>> We could try to define a standard set of clocks and reset controllers.
>> The dwmac does not have much additional tunables in the driver.
>> Anything else should be SoC specific, put in an extension, and
>> documented as such.
>
> I think it will be impossible to standardize SOC specifics as this
> glue/logic keeps changing across SOCs/Vendors.

If the GMAC core accepts external inputs for reset and tx clock,
they should be part of the generic driver core.

I think that is what the ST platform needs as well. Is it possible
to model the ST platform tx clock selection as a clock?

>>> I see issues with this approach, which are addressed in my patches.
>>>
>>> Please have a look at the http://lkml.org/lkml/2013/11/12/462 patch on
>>> STi SOC glue, which does implement a very similar glue driver.
>>>
>>> Do you see any issues not to do that way?
>>
>> The glue layer driver would not have access to any of dwmac driver's
>> internals, nor could it respond to any state changes.
>
> Should it have access to this in the first place?
> Should SOC Glue driver actually change the internal data structures of
> the stmmac? I guess No.
>
> I think all you need is to know the interface type which can be derived
> from a child node as I did in my glue driver.

The interface type is not enough. In GMII mode, you would have to change
the tx clock parent to match the link speed, as I mentioned in a previous
section.

>>
>> For example, .fix_mac_speed is called whenever auto-negotiation happens,
>> or when the link goes down or up. Maybe some future SoC would need this.#
> Makes sense..
>
> Yes, On ST we need this too, but max-speed would limit the device
> capability. But I think its more complicated than just setting up
> divider, most of the configuration depends on board hw setup.
>
> Ex: TX clk can come from external clk or phyclk or txclk itself...
>
> So the call I have taken here is to support only speeds based on the
> max-speed property.

So you would be supporting 10/100 for MII, and 10/100/1000 or 10/100
for RGMII, depending on the available clock input.
What about GMII?

Doesn't this seem a bit self-limiting? You have a gigabit capable
MAC and PHY, but you are not using it to its full potential.

>> The Allwinner A20 has the option to use the external 125MHz clock
>> provided by the PHY for the tx clock under RGMII mode, with multiple
>> dividers yielding 125, 25, or 2.5 MHz. It seems this would require
>
> Why should not the this be represented in a proper clock( "phyclk" )
> which can be setup and driven the mac driver it self.

This I am working on. Here is what I propose:

stmmac takes a second optional clock, named "tx_clk".
The clock provider should:

  1. put the glue layer into MII mode when the clock is disabled
  2. put the glue layer into RGMII mode when the clock is enabled
  3. support clk_set_rate at 125, 25, or 2.5 MHz for RGMII link
     speed changes. This could be a no-op for the internal clock.

While we're at it, we can also make stmmac take an optional reset handler.

I think this solves both our problems, while being both generic and
part of the gmac core hardware.

The rest of my requirements for Allwinner A20/A31 gmac can be solved
with proper PHY node support, which I will implement, and
"snps,force_sf_dma_mode" and "snps,tx-coe" properties.

>> .fix_mac_speed, though I have not tested it. My goal was to produce
>> a usable driver, rather than test all the possibilities.
>>
>> The other issue is when the glue layer needs to set SoC specific
>> implementation details of the dwmac, when it is not possible to
>> add a generic dwmac compatible. See next section for more on this.
>>
>>> On 06/12/13 17:29, Chen-Yu Tsai wrote:as
>>>> The Allwinner A20 has an ethernet controller that seems to be
>>>> an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
>>>> which is supported by the stmmac driver.
>>>>
>>>> Allwinner's GMAC requires setting additional registers in the SoC's
>>>> clock control unit.
>>>>
>>>> The exact version of the DWMAC IP that Allwinner uses is unknown,
>>>> thus the exact feature set is unknown.
>>>>
>>>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>>>> ---
>>>>  .../bindings/net/allwinner,sun7i-gmac.txt          | 22 +++++++
>>>>  drivers/net/ethernet/stmicro/stmmac/Kconfig        | 12 ++++
>>>>  drivers/net/ethernet/stmicro/stmmac/Makefile       |  1 +
>>>>  drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c  | 76 ++++++++++++++++++++++
>>>>  drivers/net/ethernet/stmicro/stmmac/stmmac.h       |  3 +lot
>>>>  .../net/ethernet/stmicro/stmmac/stmmac_platform.c  |  3 +
>>>>  6 files changed, 117 insertions(+)
>>>> http://lkml.org/lkml/2013/11/12/462
>>>> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
>>>
>>>> --- /dev/null
>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c
>>>> @@ -0,0 +1,76 @@
>>>> +/**
>>>> + * dwmac-sunxi.c - Allwinner sunxi DWMAC specific glue layer
>>>> + *
>>>> + * Copyright (C) 2013 Chen-Yu Tsai
>>>> + *
>>>> + * Chen-Yu Tsai  <wens-jdAy2FN1RRM@public.gmane.org>
>>>> + *
>>>> + * 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, orhttp://lkml.org/lkml/2013/11/12/462
>>>> + * (at your option) any later version.
>>>> + *http://lkml.org/lkml/2013/11/12/462
>>>> + * This program is distributed in the hope that it will be useful,http://lkml.org/lkml/2013/11/12/462
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty ofas
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/phy.h>
>>>> +#include <linux/stmmac.h>
>>>> +
>>>> +#define GMAC_IF_TYPE_RGMII   0x4
>>>> +
>>>> +#define GMAC_TX_CLK_MASK     0x3
>>>> +#define GMAC_TX_CLK_MII              0x0
>>>> +#define GMAC_TX_CLK_RGMII_INT        0x2
>>>> +
>>>> +static int sun7i_gmac_init(struct platform_device *pdev)
>>>> +{
>>>> +     struct resource *res;
>>>> +     struct device *dev = &pdev->dev;
>>>> +     void __iomem *addr = NULL;
>>>> +     struct plat_stmmacenet_data *plat_dat = NULL;
>>>> +     u32 priv_clk_reg;
>>>> +
>>>> +     plat_dat = dev_get_platdata(&pdev->dev);
>>> This is not valid for DT case.
>>> In DT case stmmac maintains its own instance of platform data.
>>>
>>> Setting platform data dynamically after probe to a device is not the
>>> right way to do.
>>
>> I see. And stmmac's own instance of platform data is associated with the
>> device in stmmac_dvr_probe. To make this work, we would have to move
>> setting dev->priv->plat in front of the .init callback.
>
> No it would not work either, as platform data is stmmac is stored in its
> private data structure.

With these modifications, the .init and .exit callbacks will have access
to stmmac's private data structure, as they are passed dev as the sole
argument.

>>>> +     if (!plat_dat)as
>>>> +             return -EINVAL;
>>>> +
>>>> +     /* Get GMAC clock register in CCU */
>>>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>> +     addr = devm_ioremap_resource(dev, res);
>>>> +     if (IS_ERR(addr))
>>>> +             return PTR_ERR(addr);
>>>> +
>>>> +     priv_clk_reg = readl(addr);
>>>> +
>>>> +     /* Set GMAC interface port mode */
>>>> +     if (plat_dat->interface == PHY_INTERFACE_MODE_RGMII)
>>>> +             priv_clk_reg |= GMAC_IF_TYPE_RGMII;
>>>> +     else
>>>> +             priv_clk_reg &= ~GMAC_IF_TYPE_RGMII;
>>>> +
>>>> +     /* Set GMAC transmit clock source. */
>>>> +     priv_clk_reg &= ~GMAC_TX_CLK_MASK;
>>>> +     if (plat_dat->interface == PHY_INTERFACE_MODE_RGMII
>>>> +                     || plat_dat->interface == PHY_INTERFACE_MODE_GMII)
>>>> +             priv_clk_reg |= GMAC_TX_CLK_RGMII_INT;
>>>> +     else
>>>> +             priv_clk_reg |= GMAC_TX_CLK_MII;
>>>> +
>>>> +     writel(priv_clk_reg, addr);
>>>> +
>>>> +     /* mask out phy addr 0x0 */
>>>> +     plat_dat->mdio_bus_data->phy_mask = 0x1;
>>>> +per
>>>> +     return 0;
>>>> +}
>>>> +
>>> This routine would not scale..
>>>
>>> stmmac can call init function multiple times, so you would be parsing DT
>>> and also remapping the address multiple times.
>>
>> I should save the mapped address, and check if it was mapped before.
>> Or we could split it into .probe and .init, where .probe is call in
>> stmmac_pltfr_probe, and .init is called in stmmac_open.
>>
>>> [---
>>>> +const struct plat_stmmacenet_data sun7i_gmac_data = {
>>>> +     .has_gmac = 1,
>>>> +     .tx_coe = 1,
>>>> +     .init = sun7i_gmac_init,
>>>> +};
>>>> +
>>> ---]
>>>
>>> This looks exactly like a AUXDATA way of doing things.
>>> Again stmmac driver has to make a copy of this so that I would not get a
>>> same copy for multiple instances.
>>
>> In the patch series, stmmac will make a copy.
>> Though that part needs some fixes.
>>
>> On a side note, the impression I got for not using AUXDATA is that it pushes
>> what is supposed to be driver extensions to the SoC/board specific code areas.
>>
>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>>>> index f16a9bd..c6e1f93 100644
>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
>>>> @@ -130,6 +130,9 @@ void stmmac_disable_eee_mode(struct stmmac_priv *priv);
>>>>  bool stmmac_eee_init(struct stmmac_priv *priv);
>>>>
>>>
>>>
>>> [...
>>>>  #ifdef CONFIG_STMMAC_PLATFORM
>>>> +#ifdef CONFIG_DWMAC_SUNXI
>>>> +extern const struct plat_stmmacenet_data sun7i_gmac_data;
>>>> +#endif
>>>>  extern struct platform_driver stmmac_pltfr_driver;
>>>>  static inline int stmmac_register_platform(void)
>>>>  {
>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>>> index df3fd1c..6cf8292 100644
>>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>>> @@ -35,6 +35,9 @@ static const struct of_device_id stmmac_dt_ids[] = {
>>>>       { .compatible = "snps,dwmac-3.70a"},
>>>>       { .compatible = "snps,dwmac-3.710"},
>>>>       { .compatible = "snps,dwmac"},
>>>> +#ifdef CONFIG_DWMAC_SUNXI
>>>> +     { .compatible = "allwinner,sun7i-gmac", .data = &sun7i_gmac_data},
>>>> +#endif
>>> ...]
>>>
>>> Personally, I did not want to do this kind of stuff in stmmac, As this
>>> list would keep growing, and this file need to be edited for every new
>>> SOC or every different type of glue logic.
>>>
>>> Please let me know what your opnion on doing Allwinner glue driver in
>>> line with http://lkml.org/lkml/2013/11/12/462
>>
>>
>> I needed to set some fields in the platform data,
>> as was set in the driver provided by Allwinner, and
>> out of our own needs.
>
>>
>> 1. .tx_coe
>>    This is not exported in the DT bindings.
>>    Looking at stmmac code, not setting this seems to disable all
>>    checksum offloading.
>
> Why cant this go via DT as well?

If you and Giuseppe are OK with this, why not?
There are quite a few options not exposed to DT.

Or we could add a compatible like "snps,dwmac-tx-coe" to signal
it can use checksum offloading. I'm not sure which is better.

> I think, this will be overriden by stmmac on MACs > 3.50a.

It will, because of the DMA hardware capabilities register.
But the MAC in A20 is not > 3.50a, hence the need to set it
manually.

>>
>> 2. .force_sf_dma_mode
>>    I have added DT property handling for this.
> Ok,
>>
>> 3. .mdio_bus_data
>>    The PHY mask is something we would like to have at the board level.
>>    The PHY on one of our boards, RTL8211E from Realtek, responds to
>>    commands sent to phy address 0, regardless what it's actual address is.
>
> Sounds correct.as
>
>>    This is documented in RTL8211E's datasheet:
>>
>>       The RTL8211E-VB(VL)/RTL8211EG-VB support PHY addresses from 00001 to
>>       00111. PHY address 0 is a broadcast from the MAC; each PHY device
>>       should respond.
>>
>>    Without the PHY mask, probing the MDIO bus would yield two devices,
>>    when in reality there is only one. This results in some confusion.
> Can you explain the issue in more detail.
>
> Why is it a confusion? PHYs are supposed to respond to address 00000.

If you look under /sys/bus/mdio_bus/devices/, for PHYs supporting
address 00000 as broadcast, you would see 2 devices.
stmmac debug messages will also report 2 devices found.

Also, not all PHY devices respond to address 00000.
The Realtek RTL8201CP PHY on one of our other boards, does not.

I could not find a reference to "supposed to respond to address 00000".
IEEE 802.3 only defines the address space, not intended uses.

> MDIO bus could have multiple PHYS on the bus, it does not matter as long
> as you address them with correct PHYID.

Correct. I assume by PHYID you mean PHY address.
Florian has pointed out that "snps,phy-addr" is !standard, and we
should replace it with standard PHY node support. I can address this
in the patch series.

Thanks,
ChenYu

> Thanks,
> srini
>>
>>    This should definitely be exported as a DT binding.
>>
>> I am also unable to add a generic compatible string for our particular
>> dwmac, because I do not know the exact version of the IP, only that
>> it is earlier than 3.50. No hardware DMA capability flags.
>>
>> Using something like "snps,dwmac-pre-3.50" is IMO worse.
>>
>>>>       { /* sentinel */ }
>>>>  };
>>>>  MODULE_DEVICE_TABLE(of, stmmac_dt_ids);
>>>>
>>>
>>
>> ChenYu
>>
>>
>

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[srinivas kandagatla]

Hi Chen,

On 11/12/13 12:17, Chen-Yu Tsai wrote:

>>
>> I would be good to get actual picture of this hw setup, On ST the
>> additional glue logic which sits on top of the GMAC is to resposible for
>> selecting the correct retime clock.
> 
> I would have liked to look at the internal design, how the dwmac core
> is connected to the clock control, but that is out of the question.
> Still, based on the documents, I think our clock controller is partially
> intertwined with the GMAC. It takes GMAC's internally generated clock
> as one of several inputs, then sends it back to the GMAC to time tx data.
> 
This is very much similar to ST glue, one of the selected clk is used
for retime the tx data lines. This selection is more of board dependent.
It totally depends on how the GMAC is wired up with PHY.

> Judging by the register definitions listed in the A20 manual,
> the SoC glue layer clocks is something like this:
> 
>                                _________________________
>   MII TX clock from PHY >-----|___________    _________|----> to GMAC core
>   GMAC Int. RGMII TX clk >----|___________\__/__gate---|----> to PHY
>   Ext. 125MHz RGMII TX clk >--|__divider__/            |
>                               |________________________|
> 
> 
> For MII mode, the glue layer should select the TX clock from the PHY.
> The gate to the PHY should be disabled.
> 
> For RGMII mode, either the internal clock generated by the GMAC core,
> or the external 125MHz reference generated by the PHY can be selected.
> And the clock gate to the PHY should be enabled.
> If the 125MHz reference is used, the glue layer should select the proper
> divider (/1, /5, /50) based on the link speed.
> 
> For GMII mode, under 10/100 speeds, the operation matches MII mode.
> For gigabit speeds, should use a 125MHz clock (internal or external)
> and enable the output gate.
> 
> The glue layer may indeed sit on top or around the GMAC core.
> Nevertheless, its operational state does depend on the GMAC.
> The current callbacks present in the stmmac driver are a good model
> for this.

Callbacks are OK with me, as they give good level of abstraction as you
said.

But I don't like the idea of glue drivers passing the full platform data
to stmmac or glue driver parsing the platform data, which is going to
look as very ugly fixups.

Also, currently callbacks just take pdev, which seems to be forcing glue
drivers to use platform data as the only data structure to pass information.

My recommendation would be to add new parameter to these callbacks ,
which can be used for to store glue private datastructure, we could
actually use bsp_priv variable from platform data.

So the of_data structure would have some thing like:

struct stmmac_of_data {
        void * (*setup)(struct platform_device *pdev);
        void (*bus_setup)(struct platform_device *pdev, void *priv, void
__iomem *ioaddr);
        int (*init)(struct platform_device *pdev, void *priv);
        void (*exit)(struct platform_device *pdev, void *priv);
        void (*fix_mac_speed)(struct platform_device *pdev, void *priv,
unnsigned int speed);

};

setup() would return a private data struct of glue driver which can be
stored in plat->bsp_priv. Should be done at DT parsing level.

Regarding the bindings, If Peppe is happy to allow optional SOC specific
binding in it, it is Ok with me too.

But all SOC specific resources names and properties have to be properly
prefexed so that its not confused with dwmac properties.

Regarding reset, I think we can add the support in stmmac driver itself.

Regarding clocks, on STi glue we can not represent the configuration in
proper clock infrastructure.

Am happy to change sti glue driver to this interface style, if you are
Ok with this approach Or if you have any other better ideas, lets discuss.

Feel free to change the above proposed new APIs..

> 
>>>> I see issues with this approach, which are addressed in my patches.
>>>>
>>>> Please have a look at the http://lkml.org/lkml/2013/11/12/462 patch on
>>>> STi SOC glue, which does implement a very similar glue driver.
>>>>
>>>> Do you see any issues not to do that way?
>>>
>>> The glue layer driver would not have access to any of dwmac driver's
>>> internals, nor could it respond to any state changes.
>>
>> Should it have access to this in the first place?
>> Should SOC Glue driver actually change the internal data structures of
>> the stmmac? I guess No.
>>
>> I think all you need is to know the interface type which can be derived
>> from a child node as I did in my glue driver.
> 
> The interface type is not enough. In GMII mode, you would have to change
> the tx clock parent to match the link speed, as I mentioned in a previous
> section.

Ok, this should be specific to the glue driver I guess.


> 
> This I am working on. Here is what I propose:
> 
> stmmac takes a second optional clock, named "tx_clk".

> The clock provider should:
> 
[---
>   1. put the glue layer into MII mode when the clock is disabled
>   2. put the glue layer into RGMII mode when the clock is enabled
>   3. support clk_set_rate at 125, 25, or 2.5 MHz for RGMII link
>      speed changes. This could be a no-op for the internal clock.
>
---]
I think this is capturing your glue implementation details which might
not be true with other glue drivers.

> While we're at it, we can also make stmmac take an optional reset handler.
> 
> I think this solves both our problems, while being both generic and
> part of the gmac core hardware.
> 
> The rest of my requirements for Allwinner A20/A31 gmac can be solved
> with proper PHY node support, which I will implement, and
> "snps,force_sf_dma_mode" and "snps,tx-coe" properties.

This makes sense, I think this aread would need a cleanup any way.

> 
>>>>> +     plat_dat = dev_get_platdata(&pdev->dev);
>>>> This is not valid for DT case.
>>>> In DT case stmmac maintains its own instance of platform data.
>>>>
>>>> Setting platform data dynamically after probe to a device is not the
>>>> right way to do.
>>>
>>> I see. And stmmac's own instance of platform data is associated with the
>>> device in stmmac_dvr_probe. To make this work, we would have to move
>>> setting dev->priv->plat in front of the .init callback.
>>
>> No it would not work either, as platform data is stmmac is stored in its
>> private data structure.
> 
> With these modifications, the .init and .exit callbacks will have access
> to stmmac's private data structure, as they are passed dev as the sole
> argument.

Your suggestion looks like a hack, lets do it properly, as I suggested
and at the start.

>>>
>>> 1. .tx_coe
>>>    This is not exported in the DT bindings.
>>>    Looking at stmmac code, not setting this seems to disable all
>>>    checksum offloading.
>>
>> Why cant this go via DT as well?
> 
> If you and Giuseppe are OK with this, why not?
Am Ok with it.

>>> 3. .mdio_bus_data
>>>    The PHY mask is something we would like to have at the board level.
>>>    The PHY on one of our boards, RTL8211E from Realtek, responds to
>>>    commands sent to phy address 0, regardless what it's actual address is.
>>
>> Sounds correct.as
>>
>>>    This is documented in RTL8211E's datasheet:
>>>
>>>       The RTL8211E-VB(VL)/RTL8211EG-VB support PHY addresses from 00001 to
>>>       00111. PHY address 0 is a broadcast from the MAC; each PHY device
>>>       should respond.
>>>
>>>    Without the PHY mask, probing the MDIO bus would yield two devices,
>>>    when in reality there is only one. This results in some confusion.
>> Can you explain the issue in more detail.
>>
>> Why is it a confusion? PHYs are supposed to respond to address 00000.
> 
> If you look under /sys/bus/mdio_bus/devices/, for PHYs supporting
> address 00000 as broadcast, you would see 2 devices.
> stmmac debug messages will also report 2 devices found.
> 
> Also, not all PHY devices respond to address 00000.
> The Realtek RTL8201CP PHY on one of our other boards, does not.
> 
> I could not find a reference to "supposed to respond to address 00000".
> IEEE 802.3 only defines the address space, not intended uses.
Here is the reference:

"22.2.4.5.5 PHYAD (PHY Address): The PHY Address is five bits, allowing
32 unique PHY addresses. The first PHY address bit transmitted and
received is the MSB of the address. A PHY that is connected to the
station management entity via the mechanical interface defined in 22.6
shall always respond to transactions addressed to PHY Address zero
<00000>. A station management entity that is attached to multiple PHYs
must have prior knowledge of the appropriate PHY Address for each PHY."


> 
>> MDIO bus could have multiple PHYS on the bus, it does not matter as long
>> as you address them with correct PHYID.
> 
> Correct. I assume by PHYID you mean PHY address.
> Florian has pointed out that "snps,phy-addr" is !standard, and we
> should replace it with standard PHY node support. I can address this
> in the patch series.

I agree, if you have bandwidth cleanup in this area would be good.

Anyway I have limited access to emails for the next 3 weeks, so I can
not respond quickly.

Hope you have a good Christmas and new year..

Will talk to you after new year.

Thanks,
srini
> 
> Thanks,
> ChenYu
> 
>> Thanks,
>> srini
>>>
>>>    This should definitely be exported as a DT binding.
>>>
>>> I am also unable to add a generic compatible string for our particular
>>> dwmac, because I do not know the exact version of the IP, only that
>>> it is earlier than 3.50. No hardware DMA capability flags.
>>>
>>> Using something like "snps,dwmac-pre-3.50" is IMO worse.
>>>
>>>>>       { /* sentinel */ }
>>>>>  };
>>>>>  MODULE_DEVICE_TABLE(of, stmmac_dt_ids);
>>>>>
>>>>
>>>
>>> ChenYu
>>>
>>>
>>
> 
> 

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Chen-Yu Tsai]

Hi,

On Wed, Dec 11, 2013 at 10:45 PM, srinivas kandagatla
<srinivas.kandagatla-qxv4g6HH51o@public.gmane.org> wrote:
> Hi Chen,
>
> On 11/12/13 12:17, Chen-Yu Tsai wrote:
>
>>>
>>> I would be good to get actual picture of this hw setup, On ST the
>>> additional glue logic which sits on top of the GMAC is to resposible for
>>> selecting the correct retime clock.
>>
>> I would have liked to look at the internal design, how the dwmac core
>> is connected to the clock control, but that is out of the question.
>> Still, based on the documents, I think our clock controller is partially
>> intertwined with the GMAC. It takes GMAC's internally generated clock
>> as one of several inputs, then sends it back to the GMAC to time tx data.
>>
> This is very much similar to ST glue, one of the selected clk is used
> for retime the tx data lines. This selection is more of board dependent.
> It totally depends on how the GMAC is wired up with PHY.
>
>> Judging by the register definitions listed in the A20 manual,
>> the SoC glue layer clocks is something like this:
>>
>>                                _________________________
>>   MII TX clock from PHY >-----|___________    _________|----> to GMAC core
>>   GMAC Int. RGMII TX clk >----|___________\__/__gate---|----> to PHY
>>   Ext. 125MHz RGMII TX clk >--|__divider__/            |
>>                               |________________________|
>>
>>
>> For MII mode, the glue layer should select the TX clock from the PHY.
>> The gate to the PHY should be disabled.
>>
>> For RGMII mode, either the internal clock generated by the GMAC core,
>> or the external 125MHz reference generated by the PHY can be selected.
>> And the clock gate to the PHY should be enabled.
>> If the 125MHz reference is used, the glue layer should select the proper
>> divider (/1, /5, /50) based on the link speed.
>>
>> For GMII mode, under 10/100 speeds, the operation matches MII mode.
>> For gigabit speeds, should use a 125MHz clock (internal or external)
>> and enable the output gate.
>>
>> The glue layer may indeed sit on top or around the GMAC core.
>> Nevertheless, its operational state does depend on the GMAC.
>> The current callbacks present in the stmmac driver are a good model
>> for this.
>
> Callbacks are OK with me, as they give good level of abstraction as you
> said.
>
> But I don't like the idea of glue drivers passing the full platform data
> to stmmac or glue driver parsing the platform data, which is going to
> look as very ugly fixups.
>
> Also, currently callbacks just take pdev, which seems to be forcing glue
> drivers to use platform data as the only data structure to pass information.
>
> My recommendation would be to add new parameter to these callbacks ,
> which can be used for to store glue private datastructure, we could
> actually use bsp_priv variable from platform data.

I agree. The original design provided .custom_data, .custom_cfg,
.bsp_priv fields in the platform data for the callbacks.

I am not aware of any users of these fields in the current kernel.
Maybe the intended users, ST platforms, have migrated to DT.

Merging the three fields would be nice, but may break some unsuspecting
user.

> So the of_data structure would have some thing like:
>
> struct stmmac_of_data {
>         void * (*setup)(struct platform_device *pdev);
>         void (*bus_setup)(struct platform_device *pdev, void *priv, void
> __iomem *ioaddr);
>         int (*init)(struct platform_device *pdev, void *priv);
>         void (*exit)(struct platform_device *pdev, void *priv);
>         void (*fix_mac_speed)(struct platform_device *pdev, void *priv,
> unnsigned int speed);
>
> };
>
> setup() would return a private data struct of glue driver which can be
> stored in plat->bsp_priv. Should be done at DT parsing level.

So this would be called at the end of stmmac_probe_config_dt.
And for non-DT platforms, they should provide .bsp_priv themselves.

> Regarding the bindings, If Peppe is happy to allow optional SOC specific
> binding in it, it is Ok with me too.
>
> But all SOC specific resources names and properties have to be properly
> prefexed so that its not confused with dwmac properties.

I agree. SOC specific bindings should have different prefixes and
documented separately, along with the compatible strings.

> Regarding reset, I think we can add the support in stmmac driver itself.

Will do.

> Regarding clocks, on STi glue we can not represent the configuration in
> proper clock infrastructure.

I see. Could you give me a description of the 4 tx clock inputs?
I would like to learn a bit more about STi glue.

> Am happy to change sti glue driver to this interface style, if you are
> Ok with this approach Or if you have any other better ideas, lets discuss.
>
> Feel free to change the above proposed new APIs..

I think the original .fix_mac_speed (without *pdev) is ok.
It likely requires link speed, interface type, and any SoC data.
interface type is buried in platform data, so .setup should take
care to copy it into .bsp_priv.

About .bus_setup, I am unfamiliar with the use cases. Unable to comment
on this one.

>>
>>>>> I see issues with this approach, which are addressed in my patches.
>>>>>
>>>>> Please have a look at the http://lkml.org/lkml/2013/11/12/462 patch on
>>>>> STi SOC glue, which does implement a very similar glue driver.
>>>>>
>>>>> Do you see any issues not to do that way?
>>>>
>>>> The glue layer driver would not have access to any of dwmac driver's
>>>> internals, nor could it respond to any state changes.
>>>
>>> Should it have access to this in the first place?
>>> Should SOC Glue driver actually change the internal data structures of
>>> the stmmac? I guess No.
>>>
>>> I think all you need is to know the interface type which can be derived
>>> from a child node as I did in my glue driver.
>>
>> The interface type is not enough. In GMII mode, you would have to change
>> the tx clock parent to match the link speed, as I mentioned in a previous
>> section.
>
> Ok, this should be specific to the glue driver I guess.

.fix_mac_speed callback will take care of this.

>>
>> This I am working on. Here is what I propose:
>>
>> stmmac takes a second optional clock, named "tx_clk".
>
>> The clock provider should:
>>
> [---
>>   1. put the glue layer into MII mode when the clock is disabled
>>   2. put the glue layer into RGMII mode when the clock is enabled
>>   3. support clk_set_rate at 125, 25, or 2.5 MHz for RGMII link
>>      speed changes. This could be a no-op for the internal clock.
>>
> ---]
> I think this is capturing your glue implementation details which might
> not be true with other glue drivers.
>
>> While we're at it, we can also make stmmac take an optional reset handler.
>>
>> I think this solves both our problems, while being both generic and
>> part of the gmac core hardware.
>>
>> The rest of my requirements for Allwinner A20/A31 gmac can be solved
>> with proper PHY node support, which I will implement, and
>> "snps,force_sf_dma_mode" and "snps,tx-coe" properties.
>
> This makes sense, I think this aread would need a cleanup any way.
>
>>
>>>>>> +     plat_dat = dev_get_platdata(&pdev->dev);
>>>>> This is not valid for DT case.
>>>>> In DT case stmmac maintains its own instance of platform data.
>>>>>
>>>>> Setting platform data dynamically after probe to a device is not the
>>>>> right way to do.
>>>>
>>>> I see. And stmmac's own instance of platform data is associated with the
>>>> device in stmmac_dvr_probe. To make this work, we would have to move
>>>> setting dev->priv->plat in front of the .init callback.
>>>
>>> No it would not work either, as platform data is stmmac is stored in its
>>> private data structure.
>>
>> With these modifications, the .init and .exit callbacks will have access
>> to stmmac's private data structure, as they are passed dev as the sole
>> argument.
>
> Your suggestion looks like a hack, lets do it properly, as I suggested
> and at the start.

Agreed. Jumping through layers of pointers is not nice.

>>>>
>>>> 1. .tx_coe
>>>>    This is not exported in the DT bindings.
>>>>    Looking at stmmac code, not setting this seems to disable all
>>>>    checksum offloading.
>>>
>>> Why cant this go via DT as well?
>>
>> If you and Giuseppe are OK with this, why not?
> Am Ok with it.
>
>>>> 3. .mdio_bus_data
>>>>    The PHY mask is something we would like to have at the board level.
>>>>    The PHY on one of our boards, RTL8211E from Realtek, responds to
>>>>    commands sent to phy address 0, regardless what it's actual address is.
>>>
>>> Sounds correct.as
>>>
>>>>    This is documented in RTL8211E's datasheet:
>>>>
>>>>       The RTL8211E-VB(VL)/RTL8211EG-VB support PHY addresses from 00001 to
>>>>       00111. PHY address 0 is a broadcast from the MAC; each PHY device
>>>>       should respond.
>>>>
>>>>    Without the PHY mask, probing the MDIO bus would yield two devices,
>>>>    when in reality there is only one. This results in some confusion.
>>> Can you explain the issue in more detail.
>>>
>>> Why is it a confusion? PHYs are supposed to respond to address 00000.
>>
>> If you look under /sys/bus/mdio_bus/devices/, for PHYs supporting
>> address 00000 as broadcast, you would see 2 devices.
>> stmmac debug messages will also report 2 devices found.
>>
>> Also, not all PHY devices respond to address 00000.
>> The Realtek RTL8201CP PHY on one of our other boards, does not.
>>
>> I could not find a reference to "supposed to respond to address 00000".
>> IEEE 802.3 only defines the address space, not intended uses.
> Here is the reference:
>
> "22.2.4.5.5 PHYAD (PHY Address): The PHY Address is five bits, allowing
> 32 unique PHY addresses. The first PHY address bit transmitted and
> received is the MSB of the address. A PHY that is connected to the
> station management entity via the mechanical interface defined in 22.6
> shall always respond to transactions addressed to PHY Address zero
> <00000>. A station management entity that is attached to multiple PHYs
> must have prior knowledge of the appropriate PHY Address for each PHY."
>
>
>>
>>> MDIO bus could have multiple PHYS on the bus, it does not matter as long
>>> as you address them with correct PHYID.
>>
>> Correct. I assume by PHYID you mean PHY address.
>> Florian has pointed out that "snps,phy-addr" is !standard, and we
>> should replace it with standard PHY node support. I can address this
>> in the patch series.
>
> I agree, if you have bandwidth cleanup in this area would be good.
>
> Anyway I have limited access to emails for the next 3 weeks, so I can
> not respond quickly.
>
> Hope you have a good Christmas and new year..
>
> Will talk to you after new year.

I will get to work and get a new patch series out.

Wish you a happy holiday.

ChenYu

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

Hi,

On Wed, Dec 11, 2013 at 02:45:08PM +0000, srinivas kandagatla wrote:
> >>> 1. .tx_coe
> >>>    This is not exported in the DT bindings.
> >>>    Looking at stmmac code, not setting this seems to disable all
> >>>    checksum offloading.
> >>
> >> Why cant this go via DT as well?
> > 
> > If you and Giuseppe are OK with this, why not?
> Am Ok with it.

Please note that I'm opposed to this until someone explain why putting
it in the DT is relevant (and not just convenient).

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Chen-Yu Tsai]

Hi,

On Thu, Dec 12, 2013 at 5:04 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> On Wed, Dec 11, 2013 at 02:45:08PM +0000, srinivas kandagatla wrote:
>> >>> 1. .tx_coe
>> >>>    This is not exported in the DT bindings.
>> >>>    Looking at stmmac code, not setting this seems to disable all
>> >>>    checksum offloading.
>> >>
>> >> Why cant this go via DT as well?
>> >
>> > If you and Giuseppe are OK with this, why not?
>> Am Ok with it.
>
> Please note that I'm opposed to this until someone explain why putting
> it in the DT is relevant (and not just convenient).

Checksum offloading is an optional feature[1], implemented starting
from version 3.20a. It is not tied to a specific IP version. As such,
using a "snps,dwmac-<version>" compatible isn't a good fit here.

stmmac does auto-detection for optional features on MAC version > 3.50a.
This is what Srinivas was referring to.

Unfortunately, our MAC is < 3.50a. No auto-detection. We could add a
"snps,dwmac-tx-coe" compatible for this, or the seperate DT property.

The other way would be to pass the flags in the initial .data with the
SoC specific compatible. Other SoCs with the same feature won't be
able to reuse the same compatible though.


ChenYu

[1] http://www.synopsys.com/dw/dwtb.php?a=ethernet_mac

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1878 bytes --]

On Thu, Dec 12, 2013 at 06:31:43PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Thu, Dec 12, 2013 at 5:04 PM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > Hi,
> >
> > On Wed, Dec 11, 2013 at 02:45:08PM +0000, srinivas kandagatla wrote:
> >> >>> 1. .tx_coe
> >> >>>    This is not exported in the DT bindings.
> >> >>>    Looking at stmmac code, not setting this seems to disable all
> >> >>>    checksum offloading.
> >> >>
> >> >> Why cant this go via DT as well?
> >> >
> >> > If you and Giuseppe are OK with this, why not?
> >> Am Ok with it.
> >
> > Please note that I'm opposed to this until someone explain why putting
> > it in the DT is relevant (and not just convenient).
> 
> Checksum offloading is an optional feature[1], implemented starting
> from version 3.20a. It is not tied to a specific IP version. As such,
> using a "snps,dwmac-<version>" compatible isn't a good fit here.

No, but we're not in such case. Since we have a compatible of our own,
we can derive it from that. Putting a property in the DT would only be
redundant.

> stmmac does auto-detection for optional features on MAC version > 3.50a.
> This is what Srinivas was referring to.
> 
> Unfortunately, our MAC is < 3.50a. No auto-detection. We could add a
> "snps,dwmac-tx-coe" compatible for this, or the seperate DT property.
> 
> The other way would be to pass the flags in the initial .data with the
> SoC specific compatible. Other SoCs with the same feature won't be
> able to reuse the same compatible though.

Which is already pretty much the case, since we have to deal with
Allwinner specific code and features.

A new compatible is cheap to maintain, a new property is not.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Chen-Yu Tsai]

Hi,

On Fri, Dec 13, 2013 at 6:38 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Thu, Dec 12, 2013 at 06:31:43PM +0800, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Thu, Dec 12, 2013 at 5:04 PM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> > Hi,
>> >
>> > On Wed, Dec 11, 2013 at 02:45:08PM +0000, srinivas kandagatla wrote:
>> >> >>> 1. .tx_coe
>> >> >>>    This is not exported in the DT bindings.
>> >> >>>    Looking at stmmac code, not setting this seems to disable all
>> >> >>>    checksum offloading.
>> >> >>
>> >> >> Why cant this go via DT as well?
>> >> >
>> >> > If you and Giuseppe are OK with this, why not?
>> >> Am Ok with it.
>> >
>> > Please note that I'm opposed to this until someone explain why putting
>> > it in the DT is relevant (and not just convenient).
>>
>> Checksum offloading is an optional feature[1], implemented starting
>> from version 3.20a. It is not tied to a specific IP version. As such,
>> using a "snps,dwmac-<version>" compatible isn't a good fit here.
>
> No, but we're not in such case. Since we have a compatible of our own,
> we can derive it from that. Putting a property in the DT would only be
> redundant.
>
>> stmmac does auto-detection for optional features on MAC version > 3.50a.
>> This is what Srinivas was referring to.
>>
>> Unfortunately, our MAC is < 3.50a. No auto-detection. We could add a
>> "snps,dwmac-tx-coe" compatible for this, or the seperate DT property.
>>
>> The other way would be to pass the flags in the initial .data with the
>> SoC specific compatible. Other SoCs with the same feature won't be
>> able to reuse the same compatible though.
>
> Which is already pretty much the case, since we have to deal with
> Allwinner specific code and features.
>
> A new compatible is cheap to maintain, a new property is not.

Srinivas,

Let's keep platform data as of_data, so SoC compatibles can pass
hardware feature flags for cores that don't support auto-detection.

We can adjust the callbacks as you suggested, and I added a .free
to complement .setup. Driver private data and platform data are
off limits to the callbacks. My version of the callbacks:

        void (*fix_mac_speed)(void *priv, unsigned int speed);
        void (*bus_setup)(void __iomem *ioaddr);
        void *(*setup)(struct platform_device *pdev);
        void (*free)(struct platform_device *pdev, void *priv);
        int (*init)(struct platform_device *pdev, void *priv);
        void (*exit)(struct platform_device *pdev, void *priv);


Cheers,

ChenYu

Re: Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[srinivas kandagatla]

Hi Chen,

On 24/12/13 03:27, Chen-Yu Tsai wrote:
> Srinivas,
> 
> Let's keep platform data as of_data, so SoC compatibles can pass
> hardware feature flags for cores that don't support auto-detection.

I understand your concern here, But making platform_data as of_data
would just open a wide door for other hacky stuff too. So other glue
drivers would just use this interface instead, ignoring standard
interfaces. Also there would be issues on who takes the priority if the
parameters are specified in multiple places.


Can't this field/s be one of the variable in the struct stmmac_of_data
rather than reusing platform data?

> 
> We can adjust the callbacks as you suggested, and I added a .free
> to complement .setup. Driver private data and platform data are
> off limits to the callbacks. My version of the callbacks:
> 
>         void (*fix_mac_speed)(void *priv, unsigned int speed);
>         void (*bus_setup)(void __iomem *ioaddr);

In reply to your question in last email:
bus_setup this is something very specific to ST which configures the bus
parameters of AMBA-to-STBUS converter. This register falls inside the
memory map of stmmac.

>         void *(*setup)(struct platform_device *pdev);
>         void (*free)(struct platform_device *pdev, void *priv);
>         int (*init)(struct platform_device *pdev, void *priv);
>         void (*exit)(struct platform_device *pdev, void *priv);
> 

The callbacks to Ok to me, unless Peppe has more comments on this.

Thanks,
srini


> 
> Cheers,
> 
> ChenYu

Re: Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Chen-Yu Tsai]

Hi,

On Thu, Jan 2, 2014 at 9:11 PM, srinivas kandagatla
<srinivas.kandagatla-qxv4g6HH51o@public.gmane.org> wrote:
> Hi Chen,
>
> On 24/12/13 03:27, Chen-Yu Tsai wrote:
>> Srinivas,
>>
>> Let's keep platform data as of_data, so SoC compatibles can pass
>> hardware feature flags for cores that don't support auto-detection.
>
> I understand your concern here, But making platform_data as of_data
> would just open a wide door for other hacky stuff too. So other glue
> drivers would just use this interface instead, ignoring standard
> interfaces. Also there would be issues on who takes the priority if the
> parameters are specified in multiple places.
>
>
> Can't this field/s be one of the variable in the struct stmmac_of_data
> rather than reusing platform data?

We (sunxi) need .has_gmac and .tx_coe.

To be generic and usable to other SoCs, it seems like I would be
adding most of the fields from platform data. But maybe future
glue layers will only need the callbacks. I can not be sure.

Also since snps,phy-addr should be deprecated, I am thinking of
changing the default phy address to auto-detect, until
Giuseppe merges his phy node support work.

I will post a v2 series this week.

>>
>> We can adjust the callbacks as you suggested, and I added a .free
>> to complement .setup. Driver private data and platform data are
>> off limits to the callbacks. My version of the callbacks:
>>
>>         void (*fix_mac_speed)(void *priv, unsigned int speed);
>>         void (*bus_setup)(void __iomem *ioaddr);
>
> In reply to your question in last email:
> bus_setup this is something very specific to ST which configures the bus
> parameters of AMBA-to-STBUS converter. This register falls inside the
> memory map of stmmac.

I see. IMHO it could be merged into .init?

>
>>         void *(*setup)(struct platform_device *pdev);
>>         void (*free)(struct platform_device *pdev, void *priv);
>>         int (*init)(struct platform_device *pdev, void *priv);
>>         void (*exit)(struct platform_device *pdev, void *priv);
>>
>
> The callbacks to Ok to me, unless Peppe has more comments on this.

Ok.


Cheers
ChenYu

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[srinivas kandagatla]

On 06/12/13 17:29, Chen-Yu Tsai wrote:
> +static int sun7i_gmac_init(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *addr = NULL;
> +	struct plat_stmmacenet_data *plat_dat = NULL;
> +	u32 priv_clk_reg;
> +
> +	plat_dat = dev_get_platdata(&pdev->dev);
> +	if (!plat_dat)
> +		return -EINVAL;
dev_get_platdata will return NULL for DT, So this function will fail all
the time.

How is it supposed to work?
Am I missing some thing?

--srini
> +
> +	/* Get GMAC clock register in CCU */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	addr = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(addr))

Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Sergei Shtylyov]

Hello.

On 09-12-2013 15:21, srinivas kandagatla wrote:

>> +static int sun7i_gmac_init(struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +	struct device *dev = &pdev->dev;
>> +	void __iomem *addr = NULL;
>> +	struct plat_stmmacenet_data *plat_dat = NULL;

    No need to initialize it since you're assigning to it right below.

>> +	u32 priv_clk_reg;
>> +
>> +	plat_dat = dev_get_platdata(&pdev->dev);
>> +	if (!plat_dat)
>> +		return -EINVAL;

> dev_get_platdata will return NULL for DT, So this function will fail all
> the time.

    Indeed, unless the probe() method assigns it from the 'data' field of
'struct of_device_id'.

> How is it supposed to work?
> Am I missing some thing?

    Look at stmmac_pltfr_probe().

> --srini

WBR, Sergei

Re: Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's

[Chen-Yu Tsai]

Hi,

On Mon, Dec 9, 2013 at 9:44 PM, Sergei Shtylyov
<sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org> wrote:
> Hello.
>
>
> On 09-12-2013 15:21, srinivas kandagatla wrote:
>
>>> +static int sun7i_gmac_init(struct platform_device *pdev)
>>> +{
>>> +       struct resource *res;
>>> +       struct device *dev = &pdev->dev;
>>> +       void __iomem *addr = NULL;
>>> +       struct plat_stmmacenet_data *plat_dat = NULL;
>
>
>    No need to initialize it since you're assigning to it right below.
>
>
>>> +       u32 priv_clk_reg;
>>> +
>>> +       plat_dat = dev_get_platdata(&pdev->dev);
>>> +       if (!plat_dat)
>>> +               return -EINVAL;
>
>
>> dev_get_platdata will return NULL for DT, So this function will fail all
>> the time.
>
>
>    Indeed, unless the probe() method assigns it from the 'data' field of
> 'struct of_device_id'.
>
>
>> How is it supposed to work?
>> Am I missing some thing?
>
>
>    Look at stmmac_pltfr_probe().

Actually, Srinivas is right.
The platform data assigned from the 'data field' is not the same one as
dev_get_platdata().

This is something I missed when I was reworking the patches.


ChenYu

[PATCH 05/10] ARM: dts: sun7i: Add GMAC controller node to sun7i DTSI

[Chen-Yu Tsai]

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index e46cfed..ebb2cce 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -395,6 +395,20 @@
 			status = "disabled";
 		};
 
+		gmac: ethernet@01c50000 {
+			compatible = "allwinner,sun7i-gmac", "snps,dwmac";
+			reg = <0x01c50000 0x10000>,
+			      <0x01c20164 0x4>;
+			interrupts = <0 85 1>;
+			interrupt-names = "macirq";
+			clocks = <&ahb_gates 49>;
+			clock-names = "stmmaceth";
+			snps,pbl = <2>;
+			snps,fixed-burst;
+			snps,force_sf_dma_mode;
+			status = "disabled";
+		};
+
 		gic: interrupt-controller@01c81000 {
 			compatible = "arm,cortex-a7-gic", "arm,cortex-a15-gic";
 			reg = <0x01c81000 0x1000>,
-- 
1.8.5.1

[PATCH 06/10] ARM: dts: sun7i: Add pin muxing options for the GMAC

[Chen-Yu Tsai]

The A20 has EMAC and GMAC muxed on the same pins.
Add pin sets with gmac function for MII and RGMII mode to the DTSI.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index ebb2cce..64b1391 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -246,6 +246,28 @@
 				allwinner,drive = <0>;
 				allwinner,pull = <0>;
 			};
+
+			gmac_pins_mii: gmac_mii {
+				allwinner,pins = "PA0", "PA1", "PA2",
+						"PA3", "PA4", "PA5", "PA6",
+						"PA7", "PA8", "PA9", "PA10",
+						"PA11", "PA12", "PA13", "PA14",
+						"PA15", "PA16";
+				allwinner,function = "gmac";
+				allwinner,drive = <3>;
+				allwinner,pull = <0>;
+			};
+
+			gmac_pins_rgmii: gmac_rgmii {
+				allwinner,pins = "PA0", "PA1", "PA2",
+						"PA3", "PA4", "PA5", "PA6",
+						"PA7", "PA8", "PA10",
+						"PA11", "PA12", "PA13",
+						"PA15", "PA16";
+				allwinner,function = "gmac";
+				allwinner,drive = <3>;
+				allwinner,pull = <0>;
+			};
 		};
 
 		timer@01c20c00 {
-- 
1.8.5.1

[PATCH 07/10] ARM: dts: sun7i: cubietruck: Enable the GMAC

[Chen-Yu Tsai]

The CubieTruck uses the GMAC with an RGMII phy.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
index 8a1009d..af212a2 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
@@ -33,6 +33,14 @@
 			pinctrl-0 = <&uart0_pins_a>;
 			status = "okay";
 		};
+
+		gmac: ethernet@01c50000 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&gmac_pins_rgmii>;
+			snps,phy-addr = <1>;
+			phy-mode = "rgmii";
+			status = "okay";
+		};
 	};
 
 	leds {
-- 
1.8.5.1

Re: [PATCH 07/10] ARM: dts: sun7i: cubietruck: Enable the GMAC

[Florian Fainelli]

2013/12/6 Chen-Yu Tsai <wens@csie.org>:
> The CubieTruck uses the GMAC with an RGMII phy.
>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> index 8a1009d..af212a2 100644
> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
> @@ -33,6 +33,14 @@
>                         pinctrl-0 = <&uart0_pins_a>;
>                         status = "okay";
>                 };
> +
> +               gmac: ethernet@01c50000 {
> +                       pinctrl-names = "default";
> +                       pinctrl-0 = <&gmac_pins_rgmii>;
> +                       snps,phy-addr = <1>;

What is this snps,phy-addr property? Why is not a standard device tree
node for an Ethernet PHY node used?

> +                       phy-mode = "rgmii";

Especially since you use the relatively standard "phy-mode" property here?
--
Florian

Re: [PATCH 07/10] ARM: dts: sun7i: cubietruck: Enable the GMAC

[Chen-Yu Tsai]

On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>> The CubieTruck uses the GMAC with an RGMII phy.
>>
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> index 8a1009d..af212a2 100644
>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>> @@ -33,6 +33,14 @@
>>                         pinctrl-0 = <&uart0_pins_a>;
>>                         status = "okay";
>>                 };
>> +
>> +               gmac: ethernet@01c50000 {
>> +                       pinctrl-names = "default";
>> +                       pinctrl-0 = <&gmac_pins_rgmii>;
>> +                       snps,phy-addr = <1>;
>
> What is this snps,phy-addr property? Why is not a standard device tree
> node for an Ethernet PHY node used?

This property is implemented by stmmac and documented in the DT
bindings. stmmac has not been updated to use Ethernet PHY nodes.

Removing this property will not affect the function of the driver.
The driver probes its MDIO bus and selects the lowest available
address if not specified.

>> +                       phy-mode = "rgmii";
>
> Especially since you use the relatively standard "phy-mode" property here?

Used as specified in stmmac DT bindings.


Wens

Re: [PATCH 07/10] ARM: dts: sun7i: cubietruck: Enable the GMAC

[Florian Fainelli]

2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
> On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>>> The CubieTruck uses the GMAC with an RGMII phy.
>>>
>>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>>> ---
>>>  arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>> index 8a1009d..af212a2 100644
>>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>> @@ -33,6 +33,14 @@
>>>                         pinctrl-0 = <&uart0_pins_a>;
>>>                         status = "okay";
>>>                 };
>>> +
>>> +               gmac: ethernet@01c50000 {
>>> +                       pinctrl-names = "default";
>>> +                       pinctrl-0 = <&gmac_pins_rgmii>;
>>> +                       snps,phy-addr = <1>;
>>
>> What is this snps,phy-addr property? Why is not a standard device tree
>> node for an Ethernet PHY node used?
>
> This property is implemented by stmmac and documented in the DT
> bindings. stmmac has not been updated to use Ethernet PHY nodes.

This driver property should be removed and deprecated since there is
an ePAPR standardized Ethernet PHY node. What I am worried here is the
loss of information, the standard Ethernet DT node allows to specify
much more information (clause, maximum speed, compatible string
etc...).

>
> Removing this property will not affect the function of the driver.
> The driver probes its MDIO bus and selects the lowest available
> address if not specified.

So if this is just giving the driver a hint on where to  probe for a
PHY on the MDIO bus, then let's drop it and use the standard DT node
no?

>
>>> +                       phy-mode = "rgmii";
>>
>> Especially since you use the relatively standard "phy-mode" property here?
>
> Used as specified in stmmac DT bindings.

That one is kind of okay because of_get_phy_mode() knows about it,
even though ePAPR states this should be "phy-connection-type", so I am
okay with keeping that one because current practice and helpers are
here.
-- 
Florian

Re: [PATCH 07/10] ARM: dts: sun7i: cubietruck: Enable the GMAC

[Chen-Yu Tsai]

Florian, Giuseppe:

On Sat, Dec 7, 2013 at 9:57 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>> On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>>>> The CubieTruck uses the GMAC with an RGMII phy.
>>>>
>>>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>>>> ---
>>>>  arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>> index 8a1009d..af212a2 100644
>>>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>> @@ -33,6 +33,14 @@
>>>>                         pinctrl-0 = <&uart0_pins_a>;
>>>>                         status = "okay";
>>>>                 };
>>>> +
>>>> +               gmac: ethernet@01c50000 {
>>>> +                       pinctrl-names = "default";
>>>> +                       pinctrl-0 = <&gmac_pins_rgmii>;
>>>> +                       snps,phy-addr = <1>;
>>>
>>> What is this snps,phy-addr property? Why is not a standard device tree
>>> node for an Ethernet PHY node used?
>>
>> This property is implemented by stmmac and documented in the DT
>> bindings. stmmac has not been updated to use Ethernet PHY nodes.
>
> This driver property should be removed and deprecated since there is
> an ePAPR standardized Ethernet PHY node. What I am worried here is the
> loss of information, the standard Ethernet DT node allows to specify
> much more information (clause, maximum speed, compatible string
> etc...).

Giuseppe, any thoughts on this?

>> Removing this property will not affect the function of the driver.
>> The driver probes its MDIO bus and selects the lowest available
>> address if not specified.
>
> So if this is just giving the driver a hint on where to  probe for a
> PHY on the MDIO bus, then let's drop it and use the standard DT node
> no?

Sure. I will remove it from the DT.

The stmmac driver does not have a seperate MDIO bus driver, nor
does it support Ethernet PHY node bindings. So I will not add
a phy node at this moment.

>>>> +                       phy-mode = "rgmii";
>>>
>>> Especially since you use the relatively standard "phy-mode" property here?
>>
>> Used as specified in stmmac DT bindings.
>
> That one is kind of okay because of_get_phy_mode() knows about it,
> even though ePAPR states this should be "phy-connection-type", so I am
> okay with keeping that one because current practice and helpers are
> here.

It seems all ARM DTs currently use "phy-mode".

> --
> Florian

Chen-Yu

Re: [PATCH 07/10] ARM: dts: sun7i: cubietruck: Enable the GMAC

[Florian Fainelli]

2013/12/8 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
> Florian, Giuseppe:
>
> On Sat, Dec 7, 2013 at 9:57 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>>> On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>>>>> The CubieTruck uses the GMAC with an RGMII phy.
>>>>>
>>>>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>>>>> ---
>>>>>  arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++
>>>>>  1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>>> index 8a1009d..af212a2 100644
>>>>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>>> @@ -33,6 +33,14 @@
>>>>>                         pinctrl-0 = <&uart0_pins_a>;
>>>>>                         status = "okay";
>>>>>                 };
>>>>> +
>>>>> +               gmac: ethernet@01c50000 {
>>>>> +                       pinctrl-names = "default";
>>>>> +                       pinctrl-0 = <&gmac_pins_rgmii>;
>>>>> +                       snps,phy-addr = <1>;
>>>>
>>>> What is this snps,phy-addr property? Why is not a standard device tree
>>>> node for an Ethernet PHY node used?
>>>
>>> This property is implemented by stmmac and documented in the DT
>>> bindings. stmmac has not been updated to use Ethernet PHY nodes.
>>
>> This driver property should be removed and deprecated since there is
>> an ePAPR standardized Ethernet PHY node. What I am worried here is the
>> loss of information, the standard Ethernet DT node allows to specify
>> much more information (clause, maximum speed, compatible string
>> etc...).
>
> Giuseppe, any thoughts on this?
>
>>> Removing this property will not affect the function of the driver.
>>> The driver probes its MDIO bus and selects the lowest available
>>> address if not specified.
>>
>> So if this is just giving the driver a hint on where to  probe for a
>> PHY on the MDIO bus, then let's drop it and use the standard DT node
>> no?
>
> Sure. I will remove it from the DT.
>
> The stmmac driver does not have a seperate MDIO bus driver, nor
> does it support Ethernet PHY node bindings. So I will not add
> a phy node at this moment.

This will create needless churn in the DT if you do not do it now,
worse actually, we switch from a DT which specifically described
Ethernet PHY nodes properly to a version where it does, to ultimately
a newer version which does.

Considering that the absence of a "snps,phy-addr" property will still
result in the MDIO bus to be probed, keeping the existing Ethernet PHY
nodes, referencing them correclty with a "phy-handle" property, but
having no explicit support for these in the driver will not result in
a functional change, and will reduce the DT churn. Also, you could
still sneak a patch in this patchset which parses the standard
EThernet PHY node binding.

>
>>>>> +                       phy-mode = "rgmii";
>>>>
>>>> Especially since you use the relatively standard "phy-mode" property here?
>>>
>>> Used as specified in stmmac DT bindings.
>>
>> That one is kind of okay because of_get_phy_mode() knows about it,
>> even though ePAPR states this should be "phy-connection-type", so I am
>> okay with keeping that one because current practice and helpers are
>> here.
>
> It seems all ARM DTs currently use "phy-mode".

Which is why I said "is kind of okay", but this is not what the
standard came up with, ARM is not exactly the best DT-citizen out
there as people are prone to making up new properties when the
standard already defines some, anyway....
--
Florian

Re: [PATCH 07/10] ARM: dts: sun7i: cubietruck: Enable the GMAC

[Chen-Yu Tsai]

On Tue, Dec 10, 2013 at 1:48 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 2013/12/8 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>> Florian, Giuseppe:
>>
>> On Sat, Dec 7, 2013 at 9:57 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>>>> On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>>>>>> The CubieTruck uses the GMAC with an RGMII phy.
>>>>>>
>>>>>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>>>>>> ---
>>>>>>  arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++
>>>>>>  1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>>>> index 8a1009d..af212a2 100644
>>>>>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>>>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>>>> @@ -33,6 +33,14 @@
>>>>>>                         pinctrl-0 = <&uart0_pins_a>;
>>>>>>                         status = "okay";
>>>>>>                 };
>>>>>> +
>>>>>> +               gmac: ethernet@01c50000 {
>>>>>> +                       pinctrl-names = "default";
>>>>>> +                       pinctrl-0 = <&gmac_pins_rgmii>;
>>>>>> +                       snps,phy-addr = <1>;
>>>>>
>>>>> What is this snps,phy-addr property? Why is not a standard device tree
>>>>> node for an Ethernet PHY node used?
>>>>
>>>> This property is implemented by stmmac and documented in the DT
>>>> bindings. stmmac has not been updated to use Ethernet PHY nodes.
>>>
>>> This driver property should be removed and deprecated since there is
>>> an ePAPR standardized Ethernet PHY node. What I am worried here is the
>>> loss of information, the standard Ethernet DT node allows to specify
>>> much more information (clause, maximum speed, compatible string
>>> etc...).
>>
>> Giuseppe, any thoughts on this?
>>
>>>> Removing this property will not affect the function of the driver.
>>>> The driver probes its MDIO bus and selects the lowest available
>>>> address if not specified.
>>>
>>> So if this is just giving the driver a hint on where to  probe for a
>>> PHY on the MDIO bus, then let's drop it and use the standard DT node
>>> no?
>>
>> Sure. I will remove it from the DT.
>>
>> The stmmac driver does not have a seperate MDIO bus driver, nor
>> does it support Ethernet PHY node bindings. So I will not add
>> a phy node at this moment.
>
> This will create needless churn in the DT if you do not do it now,
> worse actually, we switch from a DT which specifically described
> Ethernet PHY nodes properly to a version where it does, to ultimately
> a newer version which does.
>
> Considering that the absence of a "snps,phy-addr" property will still
> result in the MDIO bus to be probed, keeping the existing Ethernet PHY
> nodes, referencing them correclty with a "phy-handle" property, but
> having no explicit support for these in the driver will not result in
> a functional change, and will reduce the DT churn. Also, you could
> still sneak a patch in this patchset which parses the standard
> EThernet PHY node binding.

I see. Adding the PHY node should be no trouble at all.
Is there a requirement for a seperate mdio bus in the DT?
I do not see it in the ePAPR. The stmmac mdio is not a seperate
driver, as is for most network controllers I presume.
Would attaching the PHY nodes directly under the ethernet controller
suffice?

We can then introduce PHY node support to the driver and
remove the "snps,phy-addr" property at the same time.

ChenYu

>>
>>>>>> +                       phy-mode = "rgmii";
>>>>>
>>>>> Especially since you use the relatively standard "phy-mode" property here?
>>>>
>>>> Used as specified in stmmac DT bindings.
>>>
>>> That one is kind of okay because of_get_phy_mode() knows about it,
>>> even though ePAPR states this should be "phy-connection-type", so I am
>>> okay with keeping that one because current practice and helpers are
>>> here.
>>
>> It seems all ARM DTs currently use "phy-mode".
>
> Which is why I said "is kind of okay", but this is not what the
> standard came up with, ARM is not exactly the best DT-citizen out
> there as people are prone to making up new properties when the
> standard already defines some, anyway....
> --
> Florian

Re: [PATCH 07/10] ARM: dts: sun7i: cubietruck: Enable the GMAC

[Florian Fainelli]

2013/12/9 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
> On Tue, Dec 10, 2013 at 1:48 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> 2013/12/8 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>>> Florian, Giuseppe:
>>>
>>> On Sat, Dec 7, 2013 at 9:57 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>>>>> On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>>>>>>> The CubieTruck uses the GMAC with an RGMII phy.
>>>>>>>
>>>>>>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>>>>>>> ---
>>>>>>>  arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++
>>>>>>>  1 file changed, 8 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>>>>> index 8a1009d..af212a2 100644
>>>>>>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>>>>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>>>>> @@ -33,6 +33,14 @@
>>>>>>>                         pinctrl-0 = <&uart0_pins_a>;
>>>>>>>                         status = "okay";
>>>>>>>                 };
>>>>>>> +
>>>>>>> +               gmac: ethernet@01c50000 {
>>>>>>> +                       pinctrl-names = "default";
>>>>>>> +                       pinctrl-0 = <&gmac_pins_rgmii>;
>>>>>>> +                       snps,phy-addr = <1>;
>>>>>>
>>>>>> What is this snps,phy-addr property? Why is not a standard device tree
>>>>>> node for an Ethernet PHY node used?
>>>>>
>>>>> This property is implemented by stmmac and documented in the DT
>>>>> bindings. stmmac has not been updated to use Ethernet PHY nodes.
>>>>
>>>> This driver property should be removed and deprecated since there is
>>>> an ePAPR standardized Ethernet PHY node. What I am worried here is the
>>>> loss of information, the standard Ethernet DT node allows to specify
>>>> much more information (clause, maximum speed, compatible string
>>>> etc...).
>>>
>>> Giuseppe, any thoughts on this?
>>>
>>>>> Removing this property will not affect the function of the driver.
>>>>> The driver probes its MDIO bus and selects the lowest available
>>>>> address if not specified.
>>>>
>>>> So if this is just giving the driver a hint on where to  probe for a
>>>> PHY on the MDIO bus, then let's drop it and use the standard DT node
>>>> no?
>>>
>>> Sure. I will remove it from the DT.
>>>
>>> The stmmac driver does not have a seperate MDIO bus driver, nor
>>> does it support Ethernet PHY node bindings. So I will not add
>>> a phy node at this moment.
>>
>> This will create needless churn in the DT if you do not do it now,
>> worse actually, we switch from a DT which specifically described
>> Ethernet PHY nodes properly to a version where it does, to ultimately
>> a newer version which does.
>>
>> Considering that the absence of a "snps,phy-addr" property will still
>> result in the MDIO bus to be probed, keeping the existing Ethernet PHY
>> nodes, referencing them correclty with a "phy-handle" property, but
>> having no explicit support for these in the driver will not result in
>> a functional change, and will reduce the DT churn. Also, you could
>> still sneak a patch in this patchset which parses the standard
>> EThernet PHY node binding.
>
> I see. Adding the PHY node should be no trouble at all.
> Is there a requirement for a seperate mdio bus in the DT?

Not really no, in that case the MDIO bus is implicitely implemented as
part of the Ethernet MAC controller, this is just fine.

> I do not see it in the ePAPR. The stmmac mdio is not a seperate
> driver, as is for most network controllers I presume.
> Would attaching the PHY nodes directly under the ethernet controller
> suffice?

Sure, that is what most Ethernet driver do actually. They register
their own MDIO bus and make sure PHYs get attached to it.

>
> We can then introduce PHY node support to the driver and
> remove the "snps,phy-addr" property at the same time.

Sure, sounds good, thanks!
-- 
Florian

Re: [PATCH 07/10] ARM: dts: sun7i: cubietruck: Enable the GMAC

[Giuseppe CAVALLARO]

On 12/7/2013 2:57 AM, Florian Fainelli wrote:
> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>> On Sat, Dec 7, 2013 at 5:09 AM, Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> 2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
>>>> The CubieTruck uses the GMAC with an RGMII phy.
>>>>
>>>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>>>> ---
>>>>   arch/arm/boot/dts/sun7i-a20-cubietruck.dts | 8 ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>> index 8a1009d..af212a2 100644
>>>> --- a/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>> +++ b/arch/arm/boot/dts/sun7i-a20-cubietruck.dts
>>>> @@ -33,6 +33,14 @@
>>>>                          pinctrl-0 = <&uart0_pins_a>;
>>>>                          status = "okay";
>>>>                  };
>>>> +
>>>> +               gmac: ethernet@01c50000 {
>>>> +                       pinctrl-names = "default";
>>>> +                       pinctrl-0 = <&gmac_pins_rgmii>;
>>>> +                       snps,phy-addr = <1>;
>>>
>>> What is this snps,phy-addr property? Why is not a standard device tree
>>> node for an Ethernet PHY node used?
>>
>> This property is implemented by stmmac and documented in the DT
>> bindings. stmmac has not been updated to use Ethernet PHY nodes.
>
> This driver property should be removed and deprecated since there is
> an ePAPR standardized Ethernet PHY node. What I am worried here is the
> loss of information, the standard Ethernet DT node allows to specify
> much more information (clause, maximum speed, compatible string
> etc...).


yes Florian you are right and I have already started updating the
driver. I do not sure that I'll be able to send all the patches soon
but for sure I can re-base them.

>>
>> Removing this property will not affect the function of the driver.
>> The driver probes its MDIO bus and selects the lowest available
>> address if not specified.
>
> So if this is just giving the driver a hint on where to  probe for a
> PHY on the MDIO bus, then let's drop it and use the standard DT node
> no?
>
>>
>>>> +                       phy-mode = "rgmii";
>>>
>>> Especially since you use the relatively standard "phy-mode" property here?
>>
>> Used as specified in stmmac DT bindings.
>
> That one is kind of okay because of_get_phy_mode() knows about it,
> even though ePAPR states this should be "phy-connection-type", so I am
> okay with keeping that one because current practice and helpers are
> here.
>

[PATCH 08/10] ARM: dts: sun7i: cubieboard2: Enable GMAC instead of EMAC

[Chen-Yu Tsai]

GMAC has better performance and fewer hardware issues.
Use the GMAC in MII mode for ethernet instead of the EMAC.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
index 5c51cb8..9ef5848 100644
--- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
+++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
@@ -19,21 +19,6 @@
 	compatible = "cubietech,cubieboard2", "allwinner,sun7i-a20";
 
 	soc@01c00000 {
-		emac: ethernet@01c0b000 {
-			pinctrl-names = "default";
-			pinctrl-0 = <&emac_pins_a>;
-			phy = <&phy1>;
-			status = "okay";
-		};
-
-		mdio@01c0b080 {
-			status = "okay";
-
-			phy1: ethernet-phy@1 {
-				reg = <1>;
-			};
-		};
-
 		pinctrl@01c20800 {
 			led_pins_cubieboard2: led_pins@0 {
 				allwinner,pins = "PH20", "PH21";
@@ -60,6 +45,14 @@
 			pinctrl-0 = <&i2c1_pins_a>;
 			status = "okay";
 		};
+
+		gmac: ethernet@01c50000 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&gmac_pins_mii>;
+			snps,phy-addr = <1>;
+			phy-mode = "mii";
+			status = "okay";
+		};
 	};
 
 	leds {
-- 
1.8.5.1

Re: [PATCH 08/10] ARM: dts: sun7i: cubieboard2: Enable GMAC instead of EMAC

[Florian Fainelli]

2013/12/6 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>:
> GMAC has better performance and fewer hardware issues.
> Use the GMAC in MII mode for ethernet instead of the EMAC.
>
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> ---
>  arch/arm/boot/dts/sun7i-a20-cubieboard2.dts | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> index 5c51cb8..9ef5848 100644
> --- a/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> +++ b/arch/arm/boot/dts/sun7i-a20-cubieboard2.dts
> @@ -19,21 +19,6 @@
>         compatible = "cubietech,cubieboard2", "allwinner,sun7i-a20";
>
>         soc@01c00000 {
> -               emac: ethernet@01c0b000 {
> -                       pinctrl-names = "default";
> -                       pinctrl-0 = <&emac_pins_a>;
> -                       phy = <&phy1>;
> -                       status = "okay";
> -               };
> -
> -               mdio@01c0b080 {
> -                       status = "okay";
> -
> -                       phy1: ethernet-phy@1 {
> -                               reg = <1>;
> -                       };
> -               };
> -
>                 pinctrl@01c20800 {
>                         led_pins_cubieboard2: led_pins@0 {
>                                 allwinner,pins = "PH20", "PH21";
> @@ -60,6 +45,14 @@
>                         pinctrl-0 = <&i2c1_pins_a>;
>                         status = "okay";
>                 };
> +
> +               gmac: ethernet@01c50000 {
> +                       pinctrl-names = "default";
> +                       pinctrl-0 = <&gmac_pins_mii>;
> +                       snps,phy-addr = <1>;

Same here, this is a custom property which has no reason to exist,
keep the existing ethernet PHY node please.
-- 
Florian

[PATCH 09/10] ARM: dts: sun7i: olinuxino-micro: Enable GMAC instead of EMAC

[Chen-Yu Tsai]

GMAC has better performance and fewer hardware issues.
Use the GMAC in MII mode for ethernet instead of the EMAC.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
index ead3013..185623f 100644
--- a/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
+++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-micro.dts
@@ -19,21 +19,6 @@
 	compatible = "olimex,a20-olinuxino-micro", "allwinner,sun7i-a20";
 
 	soc@01c00000 {
-		emac: ethernet@01c0b000 {
-			pinctrl-names = "default";
-			pinctrl-0 = <&emac_pins_a>;
-			phy = <&phy1>;
-			status = "okay";
-		};
-
-		mdio@01c0b080 {
-			status = "okay";
-
-			phy1: ethernet-phy@1 {
-				reg = <1>;
-			};
-		};
-
 		pinctrl@01c20800 {
 			led_pins_olinuxino: led_pins@0 {
 				allwinner,pins = "PH2";
@@ -78,6 +63,14 @@
 			pinctrl-0 = <&i2c2_pins_a>;
 			status = "okay";
 		};
+
+		gmac: ethernet@01c50000 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&gmac_pins_mii>;
+			snps,phy-addr = <1>;
+			phy-mode = "mii";
+			status = "okay";
+		};
 	};
 
 	leds {
-- 
1.8.5.1

[PATCH 10/10] ARM: dts: sun7i: Add ethernet alias for GMAC

[Chen-Yu Tsai]

U-Boot will insert MAC address into the device tree image.
It looks up ethernet[0-5] aliases to find the ethernet nodes.
Alias GMAC as ethernet0, as it is the only ethernet controller used.

Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
---
 arch/arm/boot/dts/sun7i-a20.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 64b1391..f7c57c6 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -16,6 +16,10 @@
 / {
 	interrupt-parent = <&gic>;
 
+	aliases {
+		ethernet0 = &gmac;
+	};
+
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
1.8.5.1

Re: [PATCH 10/10] ARM: dts: sun7i: Add ethernet alias for GMAC

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

Hi Chen-Yu,

On Sat, Dec 07, 2013 at 01:29:43AM +0800, Chen-Yu Tsai wrote:
> U-Boot will insert MAC address into the device tree image.
> It looks up ethernet[0-5] aliases to find the ethernet nodes.
> Alias GMAC as ethernet0, as it is the only ethernet controller used.
> 
> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> ---
>  arch/arm/boot/dts/sun7i-a20.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> index 64b1391..f7c57c6 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -16,6 +16,10 @@
>  / {
>  	interrupt-parent = <&gic>;
>  
> +	aliases {
> +		ethernet0 = &gmac;
> +	};
> +

Actually, it conflicts with
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/211695.html

Please rebase accordingly.

Thanks,
Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 10/10] ARM: dts: sun7i: Add ethernet alias for GMAC

[Chen-Yu Tsai]

I will rebase onto 3.13-rc3 or 3.13 when it comes out.

On Sat, Dec 7, 2013 at 6:15 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi Chen-Yu,
>
> On Sat, Dec 07, 2013 at 01:29:43AM +0800, Chen-Yu Tsai wrote:
>> U-Boot will insert MAC address into the device tree image.
>> It looks up ethernet[0-5] aliases to find the ethernet nodes.
>> Alias GMAC as ethernet0, as it is the only ethernet controller used.
>>
>> Signed-off-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/sun7i-a20.dtsi | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
>> index 64b1391..f7c57c6 100644
>> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
>> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
>> @@ -16,6 +16,10 @@
>>  / {
>>       interrupt-parent = <&gic>;
>>
>> +     aliases {
>> +             ethernet0 = &gmac;
>> +     };
>> +
>
> Actually, it conflicts with
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-November/211695.html
>
> Please rebase accordingly.
>
> Thanks,
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Re: [PATCH 00/10] net: stmmac: Add sun7i GMAC glue layer

[Michal Suchanek]

On 6 December 2013 18:29, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> Hi,
>
> This patch series adds Allwinner sun7i support to stmmac.
> The Allwinner sun7i SoC A20 integrates an early version of
> dwmac IP from Synopsys. On top of that is a hardware glue
> layer. This layer needs to be configured before the dwmac
> can be used.

...

> Comments?
>
> Thanks,
>
> wens
>
>
> Chen-Yu Tsai (10):
>   net: stmmac: Enable stmmac main clock when probing hardware
>   net: stmmac: Honor DT parameter to force DMA store and forward mode
>   net: stmmac: Use platform data tied with compatible strings
>   net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
>   ARM: dts: sun7i: Add GMAC controller node to sun7i DTSI
>   ARM: dts: sun7i: Add pin muxing options for the GMAC
>   ARM: dts: sun7i: cubietruck: Enable the GMAC
>   ARM: dts: sun7i: cubieboard2: Enable GMAC instead of EMAC
>   ARM: dts: sun7i: olinuxino-micro: Enable GMAC instead of EMAC
>   ARM: dts: sun7i: Add ethernet alias for GMAC

Tested-By: Michal Suchanek <hramrach-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Works for me with RGMII and MII phy on top of 3.13rc3.

Thanks

Michal