| Message ID | 1525772367-20627-1-git-send-email-mine260309@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | hwmon: (aspeed-pwm-tacho) Use 24MHz clock | expand |
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); > >
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); >> > >
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); >>> >> >> >
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!
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.
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);
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(-)