| Message ID | 1445107947-2888-1-git-send-email-vz@mleia.com |
|---|---|
| State | Accepted |
| Headers | show |
On Sun, Oct 18, 2015 at 12:22 AM, Vladimir Zapolskiy <vz@mleia.com> wrote: > If common clock framework is configured, the driver generates a warning, > which is fixed by this change: Maybe just describe the issue and the fix. Is it just a warning or the clk enable doesn't work ? I feel the crash log in the commit msg is not very informative. > > WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:727 clk_core_enable+0x2c/0xa4() > Modules linked in: > CPU: 0 PID: 1 Comm: swapper Not tainted 4.3.0-rc2+ #171 > Hardware name: LPC32XX SoC (Flattened Device Tree) > Backtrace: > [<>] (dump_backtrace) from [<>] (show_stack+0x18/0x1c) > [<>] (show_stack) from [<>] (dump_stack+0x20/0x28) > [<>] (dump_stack) from [<>] (warn_slowpath_common+0x90/0xb8) > [<>] (warn_slowpath_common) from [<>] (warn_slowpath_null+0x24/0x2c) > [<>] (warn_slowpath_null) from [<>] (clk_core_enable+0x2c/0xa4) > [<>] (clk_core_enable) from [<>] (clk_enable+0x24/0x38) > [<>] (clk_enable) from [<>] (i2c_pnx_probe+0x108/0x2a8) > [<>] (i2c_pnx_probe) from [<>] (platform_drv_probe+0x50/0xa0) > [<>] (platform_drv_probe) from [<>] (driver_probe_device+0x18c/0x408) > [<>] (driver_probe_device) from [<>] (__driver_attach+0x70/0x94) > [<>] (__driver_attach) from [<>] (bus_for_each_dev+0x74/0x98) > [<>] (bus_for_each_dev) from [<>] (driver_attach+0x20/0x28) > [<>] (driver_attach) from [<>] (bus_add_driver+0x11c/0x248) > [<>] (bus_add_driver) from [<>] (driver_register+0xa4/0xe8) > [<>] (driver_register) from [<>] (__platform_driver_register+0x50/0x64) > [<>] (__platform_driver_register) from [<>] (i2c_adap_pnx_init+0x18/0x20) > [<>] (i2c_adap_pnx_init) from [<>] (do_one_initcall+0x11c/0x1dc) > [<>] (do_one_initcall) from [<>] (kernel_init_freeable+0x10c/0x1d4) > [<>] (kernel_init_freeable) from [<>] (kernel_init+0x10/0xec) > [<>] (kernel_init) from [<>] (ret_from_fork+0x14/0x24) > > Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> > --- > drivers/i2c/busses/i2c-pnx.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c > index e814a36..6f8b446 100644 > --- a/drivers/i2c/busses/i2c-pnx.c > +++ b/drivers/i2c/busses/i2c-pnx.c > @@ -600,7 +600,7 @@ static int i2c_pnx_controller_suspend(struct device *dev) > { > struct i2c_pnx_algo_data *alg_data = dev_get_drvdata(dev); > > - clk_disable(alg_data->clk); > + clk_disable_unprepare(alg_data->clk); > > return 0; > } > @@ -609,7 +609,7 @@ static int i2c_pnx_controller_resume(struct device *dev) > { > struct i2c_pnx_algo_data *alg_data = dev_get_drvdata(dev); > > - return clk_enable(alg_data->clk); > + return clk_prepare_enable(alg_data->clk); > } > > static SIMPLE_DEV_PM_OPS(i2c_pnx_pm, > @@ -672,7 +672,7 @@ static int i2c_pnx_probe(struct platform_device *pdev) > if (IS_ERR(alg_data->ioaddr)) > return PTR_ERR(alg_data->ioaddr); > > - ret = clk_enable(alg_data->clk); > + ret = clk_prepare_enable(alg_data->clk); > if (ret) > return ret; > > @@ -726,7 +726,7 @@ static int i2c_pnx_probe(struct platform_device *pdev) > return 0; > > out_clock: > - clk_disable(alg_data->clk); > + clk_disable_unprepare(alg_data->clk); > return ret; > } > > @@ -735,7 +735,7 @@ static int i2c_pnx_remove(struct platform_device *pdev) > struct i2c_pnx_algo_data *alg_data = platform_get_drvdata(pdev); > > i2c_del_adapter(&alg_data->adapter); > - clk_disable(alg_data->clk); > + clk_disable_unprepare(alg_data->clk); > > return 0; > } > -- > 2.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19.10.2015 19:21, Shubhrajyoti Datta wrote: > On Sun, Oct 18, 2015 at 12:22 AM, Vladimir Zapolskiy <vz@mleia.com> wrote: >> If common clock framework is configured, the driver generates a warning, >> which is fixed by this change: > Maybe just describe the issue and the fix. > Is it just a warning or the clk enable doesn't work ? > > I feel the crash log in the commit msg is not very informative. It is not a crash, it is a WARN_ON(1) backtrace. The backtrace is informative enough IMHO, because if you check the code around given drivers/clk/clk.c:727 you may find the rootcause, the WARN_ON() and that the clock is not enabled as a result. CLK_COMMON selects HAVE_CLK_PREPARE and all drivers used on platforms with common clock framework must have clk_prepare/clk_unprepare, this information is omitted as well-known. But if you insist, I will improve the commit message, I'm interested in applying this trivial fix as soon as possible, then concentrate on doing something more fascinating. >> >> WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:727 clk_core_enable+0x2c/0xa4() >> Modules linked in: >> CPU: 0 PID: 1 Comm: swapper Not tainted 4.3.0-rc2+ #171 >> Hardware name: LPC32XX SoC (Flattened Device Tree) >> Backtrace: >> [<>] (dump_backtrace) from [<>] (show_stack+0x18/0x1c) >> [<>] (show_stack) from [<>] (dump_stack+0x20/0x28) >> [<>] (dump_stack) from [<>] (warn_slowpath_common+0x90/0xb8) >> [<>] (warn_slowpath_common) from [<>] (warn_slowpath_null+0x24/0x2c) >> [<>] (warn_slowpath_null) from [<>] (clk_core_enable+0x2c/0xa4) >> [<>] (clk_core_enable) from [<>] (clk_enable+0x24/0x38) >> [<>] (clk_enable) from [<>] (i2c_pnx_probe+0x108/0x2a8) >> [<>] (i2c_pnx_probe) from [<>] (platform_drv_probe+0x50/0xa0) >> [<>] (platform_drv_probe) from [<>] (driver_probe_device+0x18c/0x408) >> [<>] (driver_probe_device) from [<>] (__driver_attach+0x70/0x94) >> [<>] (__driver_attach) from [<>] (bus_for_each_dev+0x74/0x98) >> [<>] (bus_for_each_dev) from [<>] (driver_attach+0x20/0x28) >> [<>] (driver_attach) from [<>] (bus_add_driver+0x11c/0x248) >> [<>] (bus_add_driver) from [<>] (driver_register+0xa4/0xe8) >> [<>] (driver_register) from [<>] (__platform_driver_register+0x50/0x64) >> [<>] (__platform_driver_register) from [<>] (i2c_adap_pnx_init+0x18/0x20) >> [<>] (i2c_adap_pnx_init) from [<>] (do_one_initcall+0x11c/0x1dc) >> [<>] (do_one_initcall) from [<>] (kernel_init_freeable+0x10c/0x1d4) >> [<>] (kernel_init_freeable) from [<>] (kernel_init+0x10/0xec) >> [<>] (kernel_init) from [<>] (ret_from_fork+0x14/0x24) >> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> >> --- >> drivers/i2c/busses/i2c-pnx.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c >> index e814a36..6f8b446 100644 >> --- a/drivers/i2c/busses/i2c-pnx.c >> +++ b/drivers/i2c/busses/i2c-pnx.c >> @@ -600,7 +600,7 @@ static int i2c_pnx_controller_suspend(struct device *dev) >> { >> struct i2c_pnx_algo_data *alg_data = dev_get_drvdata(dev); >> >> - clk_disable(alg_data->clk); >> + clk_disable_unprepare(alg_data->clk); >> >> return 0; >> } >> @@ -609,7 +609,7 @@ static int i2c_pnx_controller_resume(struct device *dev) >> { >> struct i2c_pnx_algo_data *alg_data = dev_get_drvdata(dev); >> >> - return clk_enable(alg_data->clk); >> + return clk_prepare_enable(alg_data->clk); >> } >> >> static SIMPLE_DEV_PM_OPS(i2c_pnx_pm, >> @@ -672,7 +672,7 @@ static int i2c_pnx_probe(struct platform_device *pdev) >> if (IS_ERR(alg_data->ioaddr)) >> return PTR_ERR(alg_data->ioaddr); >> >> - ret = clk_enable(alg_data->clk); >> + ret = clk_prepare_enable(alg_data->clk); >> if (ret) >> return ret; >> >> @@ -726,7 +726,7 @@ static int i2c_pnx_probe(struct platform_device *pdev) >> return 0; >> >> out_clock: >> - clk_disable(alg_data->clk); >> + clk_disable_unprepare(alg_data->clk); >> return ret; >> } >> >> @@ -735,7 +735,7 @@ static int i2c_pnx_remove(struct platform_device *pdev) >> struct i2c_pnx_algo_data *alg_data = platform_get_drvdata(pdev); >> >> i2c_del_adapter(&alg_data->adapter); >> - clk_disable(alg_data->clk); >> + clk_disable_unprepare(alg_data->clk); >> >> return 0; >> } >> -- >> -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 19, 2015 at 11:49 PM, Vladimir Zapolskiy <vz@mleia.com> wrote: > On 19.10.2015 19:21, Shubhrajyoti Datta wrote: >> On Sun, Oct 18, 2015 at 12:22 AM, Vladimir Zapolskiy <vz@mleia.com> wrote: >>> If common clock framework is configured, the driver generates a warning, >>> which is fixed by this change: >> Maybe just describe the issue and the fix. >> Is it just a warning or the clk enable doesn't work ? >> >> I feel the crash log in the commit msg is not very informative. > > It is not a crash, it is a WARN_ON(1) backtrace. ok > > The backtrace is informative enough IMHO, because if you check the code > around given drivers/clk/clk.c:727 you may find the rootcause, the > WARN_ON() and that the clock is not enabled as a result. > > CLK_COMMON selects HAVE_CLK_PREPARE and all drivers used on platforms > with common clock framework must have clk_prepare/clk_unprepare, this > information is omitted as well-known. > > But if you insist, I will improve the commit message, I'm interested in > applying this trivial fix as soon as possible, then concentrate on doing > something more fascinating. My request will be to describe the issue. Like the probe fails or you access registers with clocks not enabled Or it is a warning fix and the controller works etc. > >>> -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20.10.2015 11:54, Shubhrajyoti Datta wrote: > On Mon, Oct 19, 2015 at 11:49 PM, Vladimir Zapolskiy <vz@mleia.com> wrote: >> On 19.10.2015 19:21, Shubhrajyoti Datta wrote: >>> On Sun, Oct 18, 2015 at 12:22 AM, Vladimir Zapolskiy <vz@mleia.com> wrote: >>>> If common clock framework is configured, the driver generates a warning, >>>> which is fixed by this change: >>> Maybe just describe the issue and the fix. >>> Is it just a warning or the clk enable doesn't work ? >>> >>> I feel the crash log in the commit msg is not very informative. >> >> It is not a crash, it is a WARN_ON(1) backtrace. > > ok > >> >> The backtrace is informative enough IMHO, because if you check the code >> around given drivers/clk/clk.c:727 you may find the rootcause, the >> WARN_ON() and that the clock is not enabled as a result. >> >> CLK_COMMON selects HAVE_CLK_PREPARE and all drivers used on platforms >> with common clock framework must have clk_prepare/clk_unprepare, this >> information is omitted as well-known. >> >> But if you insist, I will improve the commit message, I'm interested in >> applying this trivial fix as soon as possible, then concentrate on doing >> something more fascinating. > > My request will be to describe the issue. > Like the probe fails or you access registers with clocks not enabled > > Or it is a warning fix and the controller works etc. If I change initial description to the one below, would you be satisfied with it? The driver can not be used on a platform with common clock framework until clk_prepare/clk_unprepare calls are added, otherwise clk_enable calls will fail and a warning is generated. -- With best wishes, Vladimir -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Oct 17, 2015 at 09:52:27PM +0300, Vladimir Zapolskiy wrote: > If common clock framework is configured, the driver generates a warning, > which is fixed by this change: > > WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:727 clk_core_enable+0x2c/0xa4() > Modules linked in: > CPU: 0 PID: 1 Comm: swapper Not tainted 4.3.0-rc2+ #171 > Hardware name: LPC32XX SoC (Flattened Device Tree) > Backtrace: > [<>] (dump_backtrace) from [<>] (show_stack+0x18/0x1c) > [<>] (show_stack) from [<>] (dump_stack+0x20/0x28) > [<>] (dump_stack) from [<>] (warn_slowpath_common+0x90/0xb8) > [<>] (warn_slowpath_common) from [<>] (warn_slowpath_null+0x24/0x2c) > [<>] (warn_slowpath_null) from [<>] (clk_core_enable+0x2c/0xa4) > [<>] (clk_core_enable) from [<>] (clk_enable+0x24/0x38) > [<>] (clk_enable) from [<>] (i2c_pnx_probe+0x108/0x2a8) > [<>] (i2c_pnx_probe) from [<>] (platform_drv_probe+0x50/0xa0) > [<>] (platform_drv_probe) from [<>] (driver_probe_device+0x18c/0x408) > [<>] (driver_probe_device) from [<>] (__driver_attach+0x70/0x94) > [<>] (__driver_attach) from [<>] (bus_for_each_dev+0x74/0x98) > [<>] (bus_for_each_dev) from [<>] (driver_attach+0x20/0x28) > [<>] (driver_attach) from [<>] (bus_add_driver+0x11c/0x248) > [<>] (bus_add_driver) from [<>] (driver_register+0xa4/0xe8) > [<>] (driver_register) from [<>] (__platform_driver_register+0x50/0x64) > [<>] (__platform_driver_register) from [<>] (i2c_adap_pnx_init+0x18/0x20) > [<>] (i2c_adap_pnx_init) from [<>] (do_one_initcall+0x11c/0x1dc) > [<>] (do_one_initcall) from [<>] (kernel_init_freeable+0x10c/0x1d4) > [<>] (kernel_init_freeable) from [<>] (kernel_init+0x10/0xec) > [<>] (kernel_init) from [<>] (ret_from_fork+0x14/0x24) > > Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> Applied to for-next with your updated commit message, thanks!
> Applied to for-next with your updated commit message, thanks!
Actually, applied to for-current, thanks!
diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c index e814a36..6f8b446 100644 --- a/drivers/i2c/busses/i2c-pnx.c +++ b/drivers/i2c/busses/i2c-pnx.c @@ -600,7 +600,7 @@ static int i2c_pnx_controller_suspend(struct device *dev) { struct i2c_pnx_algo_data *alg_data = dev_get_drvdata(dev); - clk_disable(alg_data->clk); + clk_disable_unprepare(alg_data->clk); return 0; } @@ -609,7 +609,7 @@ static int i2c_pnx_controller_resume(struct device *dev) { struct i2c_pnx_algo_data *alg_data = dev_get_drvdata(dev); - return clk_enable(alg_data->clk); + return clk_prepare_enable(alg_data->clk); } static SIMPLE_DEV_PM_OPS(i2c_pnx_pm, @@ -672,7 +672,7 @@ static int i2c_pnx_probe(struct platform_device *pdev) if (IS_ERR(alg_data->ioaddr)) return PTR_ERR(alg_data->ioaddr); - ret = clk_enable(alg_data->clk); + ret = clk_prepare_enable(alg_data->clk); if (ret) return ret; @@ -726,7 +726,7 @@ static int i2c_pnx_probe(struct platform_device *pdev) return 0; out_clock: - clk_disable(alg_data->clk); + clk_disable_unprepare(alg_data->clk); return ret; } @@ -735,7 +735,7 @@ static int i2c_pnx_remove(struct platform_device *pdev) struct i2c_pnx_algo_data *alg_data = platform_get_drvdata(pdev); i2c_del_adapter(&alg_data->adapter); - clk_disable(alg_data->clk); + clk_disable_unprepare(alg_data->clk); return 0; }
If common clock framework is configured, the driver generates a warning, which is fixed by this change: WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:727 clk_core_enable+0x2c/0xa4() Modules linked in: CPU: 0 PID: 1 Comm: swapper Not tainted 4.3.0-rc2+ #171 Hardware name: LPC32XX SoC (Flattened Device Tree) Backtrace: [<>] (dump_backtrace) from [<>] (show_stack+0x18/0x1c) [<>] (show_stack) from [<>] (dump_stack+0x20/0x28) [<>] (dump_stack) from [<>] (warn_slowpath_common+0x90/0xb8) [<>] (warn_slowpath_common) from [<>] (warn_slowpath_null+0x24/0x2c) [<>] (warn_slowpath_null) from [<>] (clk_core_enable+0x2c/0xa4) [<>] (clk_core_enable) from [<>] (clk_enable+0x24/0x38) [<>] (clk_enable) from [<>] (i2c_pnx_probe+0x108/0x2a8) [<>] (i2c_pnx_probe) from [<>] (platform_drv_probe+0x50/0xa0) [<>] (platform_drv_probe) from [<>] (driver_probe_device+0x18c/0x408) [<>] (driver_probe_device) from [<>] (__driver_attach+0x70/0x94) [<>] (__driver_attach) from [<>] (bus_for_each_dev+0x74/0x98) [<>] (bus_for_each_dev) from [<>] (driver_attach+0x20/0x28) [<>] (driver_attach) from [<>] (bus_add_driver+0x11c/0x248) [<>] (bus_add_driver) from [<>] (driver_register+0xa4/0xe8) [<>] (driver_register) from [<>] (__platform_driver_register+0x50/0x64) [<>] (__platform_driver_register) from [<>] (i2c_adap_pnx_init+0x18/0x20) [<>] (i2c_adap_pnx_init) from [<>] (do_one_initcall+0x11c/0x1dc) [<>] (do_one_initcall) from [<>] (kernel_init_freeable+0x10c/0x1d4) [<>] (kernel_init_freeable) from [<>] (kernel_init+0x10/0xec) [<>] (kernel_init) from [<>] (ret_from_fork+0x14/0x24) Signed-off-by: Vladimir Zapolskiy <vz@mleia.com> --- drivers/i2c/busses/i2c-pnx.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)