* [PATCH] hwmon: (aspeed-pwm-tacho) Use 24MHz clock @ 2018-05-08 9:39 Lei YU 2018-05-08 13:51 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Lei YU @ 2018-05-08 9:39 UTC (permalink / raw) To: linux-aspeed 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 aspeed_set_clock_source(priv->regmap, 0); + priv->clk_freq = 24000000; aspeed_create_type(priv); -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] hwmon: (aspeed-pwm-tacho) Use 24MHz clock 2018-05-08 9:39 [PATCH] hwmon: (aspeed-pwm-tacho) Use 24MHz clock Lei YU @ 2018-05-08 13:51 ` Guenter Roeck 2018-05-09 3:18 ` Lei YU 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2018-05-08 13:51 UTC (permalink / raw) To: linux-aspeed 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); > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] hwmon: (aspeed-pwm-tacho) Use 24MHz clock 2018-05-08 13:51 ` Guenter Roeck @ 2018-05-09 3:18 ` Lei YU 2018-05-09 3:43 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Lei YU @ 2018-05-09 3:18 UTC (permalink / raw) To: linux-aspeed 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); >> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] hwmon: (aspeed-pwm-tacho) Use 24MHz clock 2018-05-09 3:18 ` Lei YU @ 2018-05-09 3:43 ` Guenter Roeck 2018-05-09 3:50 ` Lei YU 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2018-05-09 3:43 UTC (permalink / raw) To: linux-aspeed 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); >>> >> >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] hwmon: (aspeed-pwm-tacho) Use 24MHz clock 2018-05-09 3:43 ` Guenter Roeck @ 2018-05-09 3:50 ` Lei YU 2018-05-09 4:03 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 6+ messages in thread From: Lei YU @ 2018-05-09 3:50 UTC (permalink / raw) To: linux-aspeed 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! ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] hwmon: (aspeed-pwm-tacho) Use 24MHz clock 2018-05-09 3:50 ` Lei YU @ 2018-05-09 4:03 ` Benjamin Herrenschmidt 0 siblings, 0 replies; 6+ messages in thread From: Benjamin Herrenschmidt @ 2018-05-09 4:03 UTC (permalink / raw) To: linux-aspeed 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-09 4:03 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-05-08 9:39 [PATCH] hwmon: (aspeed-pwm-tacho) Use 24MHz clock Lei YU 2018-05-08 13:51 ` Guenter Roeck 2018-05-09 3:18 ` Lei YU 2018-05-09 3:43 ` Guenter Roeck 2018-05-09 3:50 ` Lei YU 2018-05-09 4:03 ` Benjamin Herrenschmidt
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).