* [PATCH] net: ftgmac100: Request clock and set speed
@ 2017-10-10 4:49 Joel Stanley
2017-10-10 5:04 ` Florian Fainelli
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Joel Stanley @ 2017-10-10 4:49 UTC (permalink / raw)
To: David S . Miller, Benjamin Herrenschmidt
Cc: netdev, linux-kernel, Andrew Jeffery
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 <joel@jms.id.au>
---
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 <linux/clk.h>
#include <linux/dma-mapping.h>
#include <linux/etherdevice.h>
#include <linux/ethtool.h>
@@ -59,6 +60,9 @@
/* Min number of tx ring entries before stopping queue */
#define TX_THRESHOLD (MAX_SKB_FRAGS + 1)
+#define FTGMAC_100MHZ 100000000
+#define FTGMAC_25MHZ 25000000
+
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 = &pdev->dev;
INIT_WORK(&priv->reset_task, ftgmac100_reset_task);
+ /* Enable clock if present */
+ priv->clk = devm_clk_get(&pdev->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(&pdev->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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] net: ftgmac100: Request clock and set speed
2017-10-10 4:49 [PATCH] net: ftgmac100: Request clock and set speed Joel Stanley
@ 2017-10-10 5:04 ` Florian Fainelli
2017-10-10 5:08 ` Joel Stanley
2017-10-10 8:14 ` Benjamin Herrenschmidt
2017-10-11 19:08 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2017-10-10 5:04 UTC (permalink / raw)
To: Joel Stanley, David S . Miller, Benjamin Herrenschmidt
Cc: netdev, linux-kernel, Andrew Jeffery
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 <joel@jms.id.au>
> ---
> @@ -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 = &pdev->dev;
> INIT_WORK(&priv->reset_task, ftgmac100_reset_task);
>
> + /* Enable clock if present */
> + priv->clk = devm_clk_get(&pdev->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(&pdev->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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ftgmac100: Request clock and set speed
2017-10-10 5:04 ` Florian Fainelli
@ 2017-10-10 5:08 ` Joel Stanley
0 siblings, 0 replies; 6+ messages in thread
From: Joel Stanley @ 2017-10-10 5:08 UTC (permalink / raw)
To: Florian Fainelli
Cc: David S . Miller, Benjamin Herrenschmidt, netdev,
Linux Kernel Mailing List, Andrew Jeffery
On Tue, Oct 10, 2017 at 2:34 PM, Florian Fainelli <f.fainelli@gmail.com> 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 <joel@jms.id.au>
>> ---
>
>> @@ -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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ftgmac100: Request clock and set speed
2017-10-10 4:49 [PATCH] net: ftgmac100: Request clock and set speed Joel Stanley
2017-10-10 5:04 ` Florian Fainelli
@ 2017-10-10 8:14 ` Benjamin Herrenschmidt
2017-10-12 3:02 ` Joel Stanley
2017-10-11 19:08 ` David Miller
2 siblings, 1 reply; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2017-10-10 8:14 UTC (permalink / raw)
To: Joel Stanley, David S . Miller; +Cc: netdev, linux-kernel, Andrew Jeffery
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 <joel@jms.id.au>
> ---
> 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 <linux/clk.h>
> #include <linux/dma-mapping.h>
> #include <linux/etherdevice.h>
> #include <linux/ethtool.h>
> @@ -59,6 +60,9 @@
> /* Min number of tx ring entries before stopping queue */
> #define TX_THRESHOLD (MAX_SKB_FRAGS + 1)
>
> +#define FTGMAC_100MHZ 100000000
> +#define FTGMAC_25MHZ 25000000
> +
> 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 = &pdev->dev;
> INIT_WORK(&priv->reset_task, ftgmac100_reset_task);
>
> + /* Enable clock if present */
> + priv->clk = devm_clk_get(&pdev->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(&pdev->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.
> */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: ftgmac100: Request clock and set speed
2017-10-10 8:14 ` Benjamin Herrenschmidt
@ 2017-10-12 3:02 ` Joel Stanley
0 siblings, 0 replies; 6+ messages in thread
From: Joel Stanley @ 2017-10-12 3:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: David S . Miller, netdev, Linux Kernel Mailing List,
Andrew Jeffery
On Tue, Oct 10, 2017 at 4:14 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ftgmac100: Request clock and set speed
2017-10-10 4:49 [PATCH] net: ftgmac100: Request clock and set speed Joel Stanley
2017-10-10 5:04 ` Florian Fainelli
2017-10-10 8:14 ` Benjamin Herrenschmidt
@ 2017-10-11 19:08 ` David Miller
2 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2017-10-11 19:08 UTC (permalink / raw)
To: joel; +Cc: benh, netdev, linux-kernel, andrew
From: Joel Stanley <joel@jms.id.au>
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 <joel@jms.id.au>
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-10-12 3:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-10 4:49 [PATCH] net: ftgmac100: Request clock and set speed Joel Stanley
2017-10-10 5:04 ` Florian Fainelli
2017-10-10 5:08 ` Joel Stanley
2017-10-10 8:14 ` Benjamin Herrenschmidt
2017-10-12 3:02 ` Joel Stanley
2017-10-11 19:08 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).