hwmon: (aspeed-pwm-tacho) Use 24MHz clock

Message ID 1525772367-20627-1-git-send-email-mine260309@gmail.com
State New
Headers show
Series
  • hwmon: (aspeed-pwm-tacho) Use 24MHz clock
Related show

Commit Message

Lei YU May 8, 2018, 9:39 a.m.
The clock source for aspeed pwm is set to 24MHz, so use the hard-coded
clock frequency instead of the one in device tree.

Otherwise, in case of the clock specified in device tree is not 24MHz,
the fan speed will be incorrect.

Signed-off-by: Lei YU <mine260309@gmail.com>
---
 drivers/hwmon/aspeed-pwm-tacho.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Guenter Roeck May 8, 2018, 1:51 p.m. | #1
On 05/08/2018 02:39 AM, Lei YU wrote:
> The clock source for aspeed pwm is set to 24MHz, so use the hard-coded
> clock frequency instead of the one in device tree.
> 
> Otherwise, in case of the clock specified in device tree is not 24MHz,
> the fan speed will be incorrect.
> 
> Signed-off-by: Lei YU <mine260309@gmail.com>
> ---
>   drivers/hwmon/aspeed-pwm-tacho.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
> index 693a3d5..e83b8df 100644
> --- a/drivers/hwmon/aspeed-pwm-tacho.c
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -922,7 +922,6 @@ static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
>   	void __iomem *regs;
>   	struct resource *res;
>   	struct device *hwmon;
> -	struct clk *clk;
>   	int ret;
>   
>   	np = dev->of_node;
> @@ -956,12 +955,10 @@ static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
>   	regmap_write(priv->regmap, ASPEED_PTCR_TACH_SOURCE, 0);
>   	regmap_write(priv->regmap, ASPEED_PTCR_TACH_SOURCE_EXT, 0);
>   
> -	clk = devm_clk_get(dev, NULL);
> -	if (IS_ERR(clk))
> -		return -ENODEV;
> -	priv->clk_freq = clk_get_rate(clk);
>   	aspeed_set_clock_enable(priv->regmap, true);
> +	// The clock source is set to 24MHz

No mixed C/C++ comments in hwmon drivers.

>   	aspeed_set_clock_source(priv->regmap, 0);
> +	priv->clk_freq = 24000000;
>   

Are you saying that clk_get_rate() is wrong ? Anyway, if the DT is bad, it should be fixed.
I am not a friend of hacking drivers to fix up bad DTs, and much less so without explanation.
Plus, how do we know that the next chip supported by the driver doesn't have a 32MHz clock ?
Really, please fix the DT.

Guenter

>   	aspeed_create_type(priv);
>   
>
Lei YU May 9, 2018, 3:18 a.m. | #2
On Tue, May 8, 2018 at 9:51 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> No mixed C/C++ comments in hwmon drivers.
>
>>         aspeed_set_clock_source(priv->regmap, 0);
>> +       priv->clk_freq = 24000000;
>>
>
>
> Are you saying that clk_get_rate() is wrong ? Anyway, if the DT is bad, it
> should be fixed.
Nope, clk_get_rate() is OK.
The reason I make this change is because the PWM supports two types of clock
source, the 24MHz or the clock from memory controller.
If the DT uses 24MHz clock, this code is OK.
But if the DT configs this pwm to use mclk (memory controller clk), this piece
of code becomes wrong, because the code
`aspeed_set_clock_source(priv->regmap, 0)` configs the device to use the 24MHz
clock.
So no matter what DT configs the clk, this driver *always* uses 24MHz clock.

That's why I want to make this change.

> I am not a friend of hacking drivers to fix up bad DTs, and much less so
> without explanation.
> Plus, how do we know that the next chip supported by the driver doesn't have
> a 32MHz clock ?
This driver currently supports ast2400 and ast2500, and they both use 24MHz
clock.
In case future device uses a different clock, we can update this code, right?

> Really, please fix the DT.
Sure, I will send patch to config the clock to use fixed 24MHz clock as well.

>
> Guenter
>
>>         aspeed_create_type(priv);
>>
>
>
Guenter Roeck May 9, 2018, 3:43 a.m. | #3
On 05/08/2018 08:18 PM, Lei YU wrote:
> On Tue, May 8, 2018 at 9:51 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> No mixed C/C++ comments in hwmon drivers.
>>
>>>          aspeed_set_clock_source(priv->regmap, 0);
>>> +       priv->clk_freq = 24000000;
>>>
>>
>>
>> Are you saying that clk_get_rate() is wrong ? Anyway, if the DT is bad, it
>> should be fixed.
> Nope, clk_get_rate() is OK.
> The reason I make this change is because the PWM supports two types of clock
> source, the 24MHz or the clock from memory controller.
> If the DT uses 24MHz clock, this code is OK.
> But if the DT configs this pwm to use mclk (memory controller clk), this piece
> of code becomes wrong, because the code
> `aspeed_set_clock_source(priv->regmap, 0)` configs the device to use the 24MHz
> clock.
> So no matter what DT configs the clk, this driver *always* uses 24MHz clock.
> 
> That's why I want to make this change.
> 
>> I am not a friend of hacking drivers to fix up bad DTs, and much less so
>> without explanation.
>> Plus, how do we know that the next chip supported by the driver doesn't have
>> a 32MHz clock ?
> This driver currently supports ast2400 and ast2500, and they both use 24MHz
> clock.
> In case future device uses a different clock, we can update this code, right?
> 

I am not going to accept this change, period. This is not one, it is five steps
backward. If "aspeed_set_clock_source(priv->regmap, 0)" changes the clock speed,
or the clock source, read it later, and attach to the correct clock. If that
doesn't work, fix the problem in the clock subsystem. Hacking the driver is just
plain wrong.

Also, if the idea in DT is to provide a different clock to the watchdog on purpose,
maybe the call to "aspeed_set_clock_source(priv->regmap, 0)" is wrong.

Guenter

>> Really, please fix the DT.
> Sure, I will send patch to config the clock to use fixed 24MHz clock as well.
> 
>>
>> Guenter
>>
>>>          aspeed_create_type(priv);
>>>
>>
>>
>
Lei YU May 9, 2018, 3:50 a.m. | #4
On Wed, May 9, 2018 at 11:43 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>
> I am not going to accept this change, period. This is not one, it is five
> steps
> backward. If "aspeed_set_clock_source(priv->regmap, 0)" changes the clock
> speed,
> or the clock source, read it later, and attach to the correct clock. If that
> doesn't work, fix the problem in the clock subsystem. Hacking the driver is
> just
> plain wrong.
>
> Also, if the idea in DT is to provide a different clock to the watchdog on
> purpose,
> maybe the call to "aspeed_set_clock_source(priv->regmap, 0)" is wrong.

Exactly!
My first change (not sent to mailing list) is to remove the call of
"aspeed_set_clock_source(priv->regmap, 0)", instead, checking the clock
source, and use either 24M or the memory controller clock.
But that make things a bit more complicated and Aspeed suggests to use 24M
clock. So I sent this simplified change.

It's OK to not accept this change, the fix in DT will fix the issue as well.
Please drop this.
Thanks!
Benjamin Herrenschmidt May 9, 2018, 4:03 a.m. | #5
On Wed, 2018-05-09 at 11:50 +0800, Lei YU wrote:
> On Wed, May 9, 2018 at 11:43 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > 
> > I am not going to accept this change, period. This is not one, it is five
> > steps
> > backward. If "aspeed_set_clock_source(priv->regmap, 0)" changes the clock
> > speed,
> > or the clock source, read it later, and attach to the correct clock. If that
> > doesn't work, fix the problem in the clock subsystem. Hacking the driver is
> > just
> > plain wrong.
> > 
> > Also, if the idea in DT is to provide a different clock to the watchdog on
> > purpose,
> > maybe the call to "aspeed_set_clock_source(priv->regmap, 0)" is wrong.
> 
> Exactly!
> My first change (not sent to mailing list) is to remove the call of
> "aspeed_set_clock_source(priv->regmap, 0)", instead, checking the clock
> source, and use either 24M or the memory controller clock.
> But that make things a bit more complicated and Aspeed suggests to use 24M
> clock. So I sent this simplified change.
> 
> It's OK to not accept this change, the fix in DT will fix the issue as well.

Another option is to give *both* clock sources in the DT, since in HW
they both eventually reach the IP block, and have the driver select
which one it wants to use (that itself can also be in the DT).

That way you can select 0 if you want and still use clk_get_rate() to
know how fast it's ticking.

Cheers,
Ben.

Patch

diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
index 693a3d5..e83b8df 100644
--- a/drivers/hwmon/aspeed-pwm-tacho.c
+++ b/drivers/hwmon/aspeed-pwm-tacho.c
@@ -922,7 +922,6 @@  static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
 	void __iomem *regs;
 	struct resource *res;
 	struct device *hwmon;
-	struct clk *clk;
 	int ret;
 
 	np = dev->of_node;
@@ -956,12 +955,10 @@  static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
 	regmap_write(priv->regmap, ASPEED_PTCR_TACH_SOURCE, 0);
 	regmap_write(priv->regmap, ASPEED_PTCR_TACH_SOURCE_EXT, 0);
 
-	clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(clk))
-		return -ENODEV;
-	priv->clk_freq = clk_get_rate(clk);
 	aspeed_set_clock_enable(priv->regmap, true);
+	// The clock source is set to 24MHz
 	aspeed_set_clock_source(priv->regmap, 0);
+	priv->clk_freq = 24000000;
 
 	aspeed_create_type(priv);