Re: [PATCH] net: ftgmac100: Request clock and set speed

[Joel Stanley]

On Tue, Oct 10, 2017 at 4:14 PM, Benjamin Herrenschmidt
 wrote:
> On Tue, 2017-10-10 at 15:19 +1030, Joel Stanley wrote:
>> According to the ASPEED datasheet, gigabit speeds require a clock of
>> 100MHz or higher. Other speeds require 25MHz or higher.
>
> Did you try "live" changing by either using ethtool or plugging into
> switches/hubs at different speed ?
>
> Also this is aspeed'isms, we should probably keep that under an
> is_aspeed test.
>
> My assumption is that we wouldn't bother, and just leave the freq
> set based on whether there's a physical gigabit capable connection or
> not (ie, real gigabit PHY vs. NC-SI really). But if it can help save a
> few milliwatts..

I didn't try changing the link speed at runtime. I don't have a setup
that lets me precisely measure power consumption, so it's hard to know
what the benefits are. In the future I can revisit this and do those
measurements.

I'll change it to be as you suggest; 100MHz for PHY and 50MHz for NC-SI.

Cheers,

Joel

Re: [PATCH] net: ftgmac100: Request clock and set speed

[Joel Stanley]

On Tue, Oct 10, 2017 at 4:14 PM, Benjamin Herrenschmidt
 wrote:
> On Tue, 2017-10-10 at 15:19 +1030, Joel Stanley wrote:
>> According to the ASPEED datasheet, gigabit speeds require a clock of
>> 100MHz or higher. Other speeds require 25MHz or higher.
>
> Did you try "live" changing by either using ethtool or plugging into
> switches/hubs at different speed ?
>
> Also this is aspeed'isms, we should probably keep that under an
> is_aspeed test.
>
> My assumption is that we wouldn't bother, and just leave the freq
> set based on whether there's a physical gigabit capable connection or
> not (ie, real gigabit PHY vs. NC-SI really). But if it can help save a
> few milliwatts..

I didn't try changing the link speed at runtime. I don't have a setup
that lets me precisely measure power consumption, so it's hard to know
what the benefits are. In the future I can revisit this and do those
measurements.

I'll change it to be as you suggest; 100MHz for PHY and 50MHz for NC-SI.

Cheers,

Joel

Re: [PATCH] net: ftgmac100: Request clock and set speed

[David Miller]

From: Joel Stanley 
Date: Tue, 10 Oct 2017 15:19:25 +1030

> According to the ASPEED datasheet, gigabit speeds require a clock of
> 100MHz or higher. Other speeds require 25MHz or higher.
> 
> Signed-off-by: Joel Stanley 

Hey Joel, it seems that Benjamin would like you to guard this new
logic with whether we have an aspeed configuration or not.

Thank you.

Re: [PATCH] net: ftgmac100: Request clock and set speed

[David Miller]

From: Joel Stanley 
Date: Tue, 10 Oct 2017 15:19:25 +1030

> According to the ASPEED datasheet, gigabit speeds require a clock of
> 100MHz or higher. Other speeds require 25MHz or higher.
> 
> Signed-off-by: Joel Stanley 

Hey Joel, it seems that Benjamin would like you to guard this new
logic with whether we have an aspeed configuration or not.

Thank you.

Re: [PATCH] net: ftgmac100: Request clock and set speed

[Benjamin Herrenschmidt]

On Tue, 2017-10-10 at 15:19 +1030, Joel Stanley wrote:
> According to the ASPEED datasheet, gigabit speeds require a clock of
> 100MHz or higher. Other speeds require 25MHz or higher.

Did you try "live" changing by either using ethtool or plugging into
switches/hubs at different speed ?

Also this is aspeed'isms, we should probably keep that under an
is_aspeed test.

My assumption is that we wouldn't bother, and just leave the freq
set based on whether there's a physical gigabit capable connection or
not (ie, real gigabit PHY vs. NC-SI really). But if it can help save a
few milliwatts..

> Signed-off-by: Joel Stanley 
> ---
>  drivers/net/ethernet/faraday/ftgmac100.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
> b/drivers/net/ethernet/faraday/ftgmac100.c
> index 9ed8e4b81530..870ebd857978 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -21,6 +21,7 @@
>  
>  #define pr_fmt(fmt)  KBUILD_MODNAME ": " fmt
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -59,6 +60,9 @@
>  /* Min number of tx ring entries before stopping queue */
>  #define TX_THRESHOLD (MAX_SKB_FRAGS + 1)
>  
> +#define FTGMAC_100MHZ1
> +#define FTGMAC_25MHZ 2500
> +
>  struct ftgmac100 {
>   /* Registers */
>   struct resource *res;
> @@ -96,6 +100,7 @@ struct ftgmac100 {
>   struct napi_struct napi;
>   struct work_struct reset_task;
>   struct mii_bus *mii_bus;
> + struct clk *clk;
>  
>   /* Link management */
>   int cur_speed;
> @@ -142,18 +147,22 @@ static int ftgmac100_reset_mac(struct ftgmac100 *priv, 
> u32 maccr)
>  static int ftgmac100_reset_and_config_mac(struct ftgmac100 *priv)
>  {
>   u32 maccr = 0;
> + int freq = 0;
>  
>   switch (priv->cur_speed) {
>   case SPEED_10:
>   case 0: /* no link */
> + freq = FTGMAC_25MHZ;
>   break;
>  
>   case SPEED_100:
>   maccr |= FTGMAC100_MACCR_FAST_MODE;
> + freq = FTGMAC_25MHZ;
>   break;
>  
>   case SPEED_1000:
>   maccr |= FTGMAC100_MACCR_GIGA_MODE;
> + freq = FTGMAC_100MHZ;
>   break;
>   default:
>   netdev_err(priv->netdev, "Unknown speed %d !\n",
> @@ -161,6 +170,9 @@ static int ftgmac100_reset_and_config_mac(struct 
> ftgmac100 *priv)
>   break;
>   }
>  
> + if (freq && priv->clk)
> + clk_set_rate(priv->clk, freq);
> +
>   /* (Re)initialize the queue pointers */
>   priv->rx_pointer = 0;
>   priv->tx_clean_pointer = 0;
> @@ -1775,6 +1787,13 @@ static int ftgmac100_probe(struct platform_device 
> *pdev)
>   priv->dev = >dev;
>   INIT_WORK(>reset_task, ftgmac100_reset_task);
>  
> + /* Enable clock if present */
> + priv->clk = devm_clk_get(>dev, NULL);
> + if (!IS_ERR(priv->clk))
> + clk_prepare_enable(priv->clk);
> + else
> + priv->clk = NULL;
> +
>   /* map io memory */
>   priv->res = request_mem_region(res->start, resource_size(res),
>  dev_name(>dev));
> @@ -1883,6 +1902,9 @@ static int ftgmac100_remove(struct platform_device 
> *pdev)
>  
>   unregister_netdev(netdev);
>  
> + if (priv->clk)
> + clk_disable_unprepare(priv->clk);
> +
>   /* There's a small chance the reset task will have been re-queued,
>* during stop, make sure it's gone before we free the structure.
>*/

Re: [PATCH] net: ftgmac100: Request clock and set speed

[Benjamin Herrenschmidt]

On Tue, 2017-10-10 at 15:19 +1030, Joel Stanley wrote:
> According to the ASPEED datasheet, gigabit speeds require a clock of
> 100MHz or higher. Other speeds require 25MHz or higher.

Did you try "live" changing by either using ethtool or plugging into
switches/hubs at different speed ?

Also this is aspeed'isms, we should probably keep that under an
is_aspeed test.

My assumption is that we wouldn't bother, and just leave the freq
set based on whether there's a physical gigabit capable connection or
not (ie, real gigabit PHY vs. NC-SI really). But if it can help save a
few milliwatts..

> Signed-off-by: Joel Stanley 
> ---
>  drivers/net/ethernet/faraday/ftgmac100.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
> b/drivers/net/ethernet/faraday/ftgmac100.c
> index 9ed8e4b81530..870ebd857978 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c
> @@ -21,6 +21,7 @@
>  
>  #define pr_fmt(fmt)  KBUILD_MODNAME ": " fmt
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -59,6 +60,9 @@
>  /* Min number of tx ring entries before stopping queue */
>  #define TX_THRESHOLD (MAX_SKB_FRAGS + 1)
>  
> +#define FTGMAC_100MHZ1
> +#define FTGMAC_25MHZ 2500
> +
>  struct ftgmac100 {
>   /* Registers */
>   struct resource *res;
> @@ -96,6 +100,7 @@ struct ftgmac100 {
>   struct napi_struct napi;
>   struct work_struct reset_task;
>   struct mii_bus *mii_bus;
> + struct clk *clk;
>  
>   /* Link management */
>   int cur_speed;
> @@ -142,18 +147,22 @@ static int ftgmac100_reset_mac(struct ftgmac100 *priv, 
> u32 maccr)
>  static int ftgmac100_reset_and_config_mac(struct ftgmac100 *priv)
>  {
>   u32 maccr = 0;
> + int freq = 0;
>  
>   switch (priv->cur_speed) {
>   case SPEED_10:
>   case 0: /* no link */
> + freq = FTGMAC_25MHZ;
>   break;
>  
>   case SPEED_100:
>   maccr |= FTGMAC100_MACCR_FAST_MODE;
> + freq = FTGMAC_25MHZ;
>   break;
>  
>   case SPEED_1000:
>   maccr |= FTGMAC100_MACCR_GIGA_MODE;
> + freq = FTGMAC_100MHZ;
>   break;
>   default:
>   netdev_err(priv->netdev, "Unknown speed %d !\n",
> @@ -161,6 +170,9 @@ static int ftgmac100_reset_and_config_mac(struct 
> ftgmac100 *priv)
>   break;
>   }
>  
> + if (freq && priv->clk)
> + clk_set_rate(priv->clk, freq);
> +
>   /* (Re)initialize the queue pointers */
>   priv->rx_pointer = 0;
>   priv->tx_clean_pointer = 0;
> @@ -1775,6 +1787,13 @@ static int ftgmac100_probe(struct platform_device 
> *pdev)
>   priv->dev = >dev;
>   INIT_WORK(>reset_task, ftgmac100_reset_task);
>  
> + /* Enable clock if present */
> + priv->clk = devm_clk_get(>dev, NULL);
> + if (!IS_ERR(priv->clk))
> + clk_prepare_enable(priv->clk);
> + else
> + priv->clk = NULL;
> +
>   /* map io memory */
>   priv->res = request_mem_region(res->start, resource_size(res),
>  dev_name(>dev));
> @@ -1883,6 +1902,9 @@ static int ftgmac100_remove(struct platform_device 
> *pdev)
>  
>   unregister_netdev(netdev);
>  
> + if (priv->clk)
> + clk_disable_unprepare(priv->clk);
> +
>   /* There's a small chance the reset task will have been re-queued,
>* during stop, make sure it's gone before we free the structure.
>*/

Re: [PATCH] net: ftgmac100: Request clock and set speed

[Joel Stanley]

On Tue, Oct 10, 2017 at 2:34 PM, Florian Fainelli  wrote:
>
>
> On 10/09/2017 09:49 PM, Joel Stanley wrote:
>> According to the ASPEED datasheet, gigabit speeds require a clock of
>> 100MHz or higher. Other speeds require 25MHz or higher.
>>
>> Signed-off-by: Joel Stanley 
>> ---
>
>> @@ -161,6 +170,9 @@ static int ftgmac100_reset_and_config_mac(struct 
>> ftgmac100 *priv)
>>   break;
>>   }
>>
>> + if (freq && priv->clk)
>> + clk_set_rate(priv->clk, freq);
>
> Checking for priv->clk should not be necessary all public clk APIs can
> deal with a NULL clock pointer.

The intention was to set ->clk to NULL to indicate that there was no
clk, and therefore there is no reason to attempt to set the rate or
call prepare/unprepare.

If the we prefer to call the clk apis unconditionally I will send a v2
with the checks removed.

Thanks for the review.

Cheers,

Joel

Re: [PATCH] net: ftgmac100: Request clock and set speed

[Joel Stanley]

On Tue, Oct 10, 2017 at 2:34 PM, Florian Fainelli  wrote:
>
>
> On 10/09/2017 09:49 PM, Joel Stanley wrote:
>> According to the ASPEED datasheet, gigabit speeds require a clock of
>> 100MHz or higher. Other speeds require 25MHz or higher.
>>
>> Signed-off-by: Joel Stanley 
>> ---
>
>> @@ -161,6 +170,9 @@ static int ftgmac100_reset_and_config_mac(struct 
>> ftgmac100 *priv)
>>   break;
>>   }
>>
>> + if (freq && priv->clk)
>> + clk_set_rate(priv->clk, freq);
>
> Checking for priv->clk should not be necessary all public clk APIs can
> deal with a NULL clock pointer.

The intention was to set ->clk to NULL to indicate that there was no
clk, and therefore there is no reason to attempt to set the rate or
call prepare/unprepare.

If the we prefer to call the clk apis unconditionally I will send a v2
with the checks removed.

Thanks for the review.

Cheers,

Joel

Re: [PATCH] net: ftgmac100: Request clock and set speed

[Florian Fainelli]

On 10/09/2017 09:49 PM, Joel Stanley wrote:
> According to the ASPEED datasheet, gigabit speeds require a clock of
> 100MHz or higher. Other speeds require 25MHz or higher.
> 
> Signed-off-by: Joel Stanley 
> ---

> @@ -161,6 +170,9 @@ static int ftgmac100_reset_and_config_mac(struct 
> ftgmac100 *priv)
>   break;
>   }
>  
> + if (freq && priv->clk)
> + clk_set_rate(priv->clk, freq);

Checking for priv->clk should not be necessary all public clk APIs can
deal with a NULL clock pointer.

> +
>   /* (Re)initialize the queue pointers */
>   priv->rx_pointer = 0;
>   priv->tx_clean_pointer = 0;
> @@ -1775,6 +1787,13 @@ static int ftgmac100_probe(struct platform_device 
> *pdev)
>   priv->dev = >dev;
>   INIT_WORK(>reset_task, ftgmac100_reset_task);
>  
> + /* Enable clock if present */
> + priv->clk = devm_clk_get(>dev, NULL);
> + if (!IS_ERR(priv->clk))
> + clk_prepare_enable(priv->clk);
> + else
> + priv->clk = NULL;

Same here.

> +
>   /* map io memory */
>   priv->res = request_mem_region(res->start, resource_size(res),
>  dev_name(>dev));
> @@ -1883,6 +1902,9 @@ static int ftgmac100_remove(struct platform_device 
> *pdev)
>  
>   unregister_netdev(netdev);
>  
> + if (priv->clk)
> + clk_disable_unprepare(priv->clk);

And here.

> +
>   /* There's a small chance the reset task will have been re-queued,
>* during stop, make sure it's gone before we free the structure.
>*/
> 

-- 
Florian

Re: [PATCH] net: ftgmac100: Request clock and set speed

[Florian Fainelli]

On 10/09/2017 09:49 PM, Joel Stanley wrote:
> According to the ASPEED datasheet, gigabit speeds require a clock of
> 100MHz or higher. Other speeds require 25MHz or higher.
> 
> Signed-off-by: Joel Stanley 
> ---

> @@ -161,6 +170,9 @@ static int ftgmac100_reset_and_config_mac(struct 
> ftgmac100 *priv)
>   break;
>   }
>  
> + if (freq && priv->clk)
> + clk_set_rate(priv->clk, freq);

Checking for priv->clk should not be necessary all public clk APIs can
deal with a NULL clock pointer.

> +
>   /* (Re)initialize the queue pointers */
>   priv->rx_pointer = 0;
>   priv->tx_clean_pointer = 0;
> @@ -1775,6 +1787,13 @@ static int ftgmac100_probe(struct platform_device 
> *pdev)
>   priv->dev = >dev;
>   INIT_WORK(>reset_task, ftgmac100_reset_task);
>  
> + /* Enable clock if present */
> + priv->clk = devm_clk_get(>dev, NULL);
> + if (!IS_ERR(priv->clk))
> + clk_prepare_enable(priv->clk);
> + else
> + priv->clk = NULL;

Same here.

> +
>   /* map io memory */
>   priv->res = request_mem_region(res->start, resource_size(res),
>  dev_name(>dev));
> @@ -1883,6 +1902,9 @@ static int ftgmac100_remove(struct platform_device 
> *pdev)
>  
>   unregister_netdev(netdev);
>  
> + if (priv->clk)
> + clk_disable_unprepare(priv->clk);

And here.

> +
>   /* There's a small chance the reset task will have been re-queued,
>* during stop, make sure it's gone before we free the structure.
>*/
> 

-- 
Florian

[PATCH] net: ftgmac100: Request clock and set speed

[Joel Stanley]

According to the ASPEED datasheet, gigabit speeds require a clock of
100MHz or higher. Other speeds require 25MHz or higher.

Signed-off-by: Joel Stanley 
---
 drivers/net/ethernet/faraday/ftgmac100.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 9ed8e4b81530..870ebd857978 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -21,6 +21,7 @@
 
 #define pr_fmt(fmt)KBUILD_MODNAME ": " fmt
 
+#include 
 #include 
 #include 
 #include 
@@ -59,6 +60,9 @@
 /* Min number of tx ring entries before stopping queue */
 #define TX_THRESHOLD   (MAX_SKB_FRAGS + 1)
 
+#define FTGMAC_100MHZ  1
+#define FTGMAC_25MHZ   2500
+
 struct ftgmac100 {
/* Registers */
struct resource *res;
@@ -96,6 +100,7 @@ struct ftgmac100 {
struct napi_struct napi;
struct work_struct reset_task;
struct mii_bus *mii_bus;
+   struct clk *clk;
 
/* Link management */
int cur_speed;
@@ -142,18 +147,22 @@ static int ftgmac100_reset_mac(struct ftgmac100 *priv, 
u32 maccr)
 static int ftgmac100_reset_and_config_mac(struct ftgmac100 *priv)
 {
u32 maccr = 0;
+   int freq = 0;
 
switch (priv->cur_speed) {
case SPEED_10:
case 0: /* no link */
+   freq = FTGMAC_25MHZ;
break;
 
case SPEED_100:
maccr |= FTGMAC100_MACCR_FAST_MODE;
+   freq = FTGMAC_25MHZ;
break;
 
case SPEED_1000:
maccr |= FTGMAC100_MACCR_GIGA_MODE;
+   freq = FTGMAC_100MHZ;
break;
default:
netdev_err(priv->netdev, "Unknown speed %d !\n",
@@ -161,6 +170,9 @@ static int ftgmac100_reset_and_config_mac(struct ftgmac100 
*priv)
break;
}
 
+   if (freq && priv->clk)
+   clk_set_rate(priv->clk, freq);
+
/* (Re)initialize the queue pointers */
priv->rx_pointer = 0;
priv->tx_clean_pointer = 0;
@@ -1775,6 +1787,13 @@ static int ftgmac100_probe(struct platform_device *pdev)
priv->dev = >dev;
INIT_WORK(>reset_task, ftgmac100_reset_task);
 
+   /* Enable clock if present */
+   priv->clk = devm_clk_get(>dev, NULL);
+   if (!IS_ERR(priv->clk))
+   clk_prepare_enable(priv->clk);
+   else
+   priv->clk = NULL;
+
/* map io memory */
priv->res = request_mem_region(res->start, resource_size(res),
   dev_name(>dev));
@@ -1883,6 +1902,9 @@ static int ftgmac100_remove(struct platform_device *pdev)
 
unregister_netdev(netdev);
 
+   if (priv->clk)
+   clk_disable_unprepare(priv->clk);
+
/* There's a small chance the reset task will have been re-queued,
 * during stop, make sure it's gone before we free the structure.
 */
-- 
2.14.1

[PATCH] net: ftgmac100: Request clock and set speed

[Joel Stanley]

According to the ASPEED datasheet, gigabit speeds require a clock of
100MHz or higher. Other speeds require 25MHz or higher.

Signed-off-by: Joel Stanley 
---
 drivers/net/ethernet/faraday/ftgmac100.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c 
b/drivers/net/ethernet/faraday/ftgmac100.c
index 9ed8e4b81530..870ebd857978 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -21,6 +21,7 @@
 
 #define pr_fmt(fmt)KBUILD_MODNAME ": " fmt
 
+#include 
 #include 
 #include 
 #include 
@@ -59,6 +60,9 @@
 /* Min number of tx ring entries before stopping queue */
 #define TX_THRESHOLD   (MAX_SKB_FRAGS + 1)
 
+#define FTGMAC_100MHZ  1
+#define FTGMAC_25MHZ   2500
+
 struct ftgmac100 {
/* Registers */
struct resource *res;
@@ -96,6 +100,7 @@ struct ftgmac100 {
struct napi_struct napi;
struct work_struct reset_task;
struct mii_bus *mii_bus;
+   struct clk *clk;
 
/* Link management */
int cur_speed;
@@ -142,18 +147,22 @@ static int ftgmac100_reset_mac(struct ftgmac100 *priv, 
u32 maccr)
 static int ftgmac100_reset_and_config_mac(struct ftgmac100 *priv)
 {
u32 maccr = 0;
+   int freq = 0;
 
switch (priv->cur_speed) {
case SPEED_10:
case 0: /* no link */
+   freq = FTGMAC_25MHZ;
break;
 
case SPEED_100:
maccr |= FTGMAC100_MACCR_FAST_MODE;
+   freq = FTGMAC_25MHZ;
break;
 
case SPEED_1000:
maccr |= FTGMAC100_MACCR_GIGA_MODE;
+   freq = FTGMAC_100MHZ;
break;
default:
netdev_err(priv->netdev, "Unknown speed %d !\n",
@@ -161,6 +170,9 @@ static int ftgmac100_reset_and_config_mac(struct ftgmac100 
*priv)
break;
}
 
+   if (freq && priv->clk)
+   clk_set_rate(priv->clk, freq);
+
/* (Re)initialize the queue pointers */
priv->rx_pointer = 0;
priv->tx_clean_pointer = 0;
@@ -1775,6 +1787,13 @@ static int ftgmac100_probe(struct platform_device *pdev)
priv->dev = >dev;
INIT_WORK(>reset_task, ftgmac100_reset_task);
 
+   /* Enable clock if present */
+   priv->clk = devm_clk_get(>dev, NULL);
+   if (!IS_ERR(priv->clk))
+   clk_prepare_enable(priv->clk);
+   else
+   priv->clk = NULL;
+
/* map io memory */
priv->res = request_mem_region(res->start, resource_size(res),
   dev_name(>dev));
@@ -1883,6 +1902,9 @@ static int ftgmac100_remove(struct platform_device *pdev)
 
unregister_netdev(netdev);
 
+   if (priv->clk)
+   clk_disable_unprepare(priv->clk);
+
/* There's a small chance the reset task will have been re-queued,
 * during stop, make sure it's gone before we free the structure.
 */
-- 
2.14.1