[V3,1/5] thermal: qoriq: Add clock operations
diff mbox series

Message ID 20190730022126.17883-1-Anson.Huang@nxp.com
State Mainlined, archived
Headers show
Series
  • [V3,1/5] thermal: qoriq: Add clock operations
Related show

Commit Message

Anson Huang July 30, 2019, 2:21 a.m. UTC
From: Anson Huang <Anson.Huang@nxp.com>

Some platforms like i.MX8MQ has clock control for this module,
need to add clock operations to make sure the driver is working
properly.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
Reviewed-by: Guido Günther <agx@sigxcpu.org>
---
Changes since V2:
	- move this patch as first patch in the series;
	- add clock disable handling in error path of probe.
---
 drivers/thermal/qoriq_thermal.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Aisheng Dong Aug. 5, 2019, 7 a.m. UTC | #1
> From: Anson Huang <Anson.Huang@nxp.com>
> 
> Some platforms like i.MX8MQ has clock control for this module, need to add
> clock operations to make sure the driver is working properly.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> Reviewed-by: Guido Günther <agx@sigxcpu.org>

Reviewed-by: Dong Aisheng <aisheng.dong@nxp.com>

Regards
Aisheng
Leonard Crestez Aug. 26, 2019, 2:45 p.m. UTC | #2
On 7/30/2019 5:31 AM, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
> 
> Some platforms like i.MX8MQ has clock control for this module,
> need to add clock operations to make sure the driver is working
> properly.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> Reviewed-by: Guido Günther <agx@sigxcpu.org>

This series looks good, do you think it can be merged in time for v5.4? 
Today was v5.3-rc6.

In an earlier series the CLK_IS_CRITICAL flags was removed from the TMU 
clock so if the thermal driver doesn't explicitly enable it the system 
will hang on probe. This is what happens in linux-next right now!

Unless this patches is merged soon we'll end up with a 5.4-rc1 that 
doesn't boot on imx8mq. An easy fix would be to drop/revert commit 
951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for 
IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.

Merging patches out-of-order when they have hard (boot-breaking) 
dependencies also breaks bisect.

--
Regards,
Leonard
Anson Huang Aug. 27, 2019, 1:51 a.m. UTC | #3
> On 7/30/2019 5:31 AM, Anson.Huang@nxp.com wrote:
> > From: Anson Huang <Anson.Huang@nxp.com>
> >
> > Some platforms like i.MX8MQ has clock control for this module, need to
> > add clock operations to make sure the driver is working properly.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > Reviewed-by: Guido Günther <agx@sigxcpu.org>
> 
> This series looks good, do you think it can be merged in time for v5.4?
> Today was v5.3-rc6.

If the question is for me, then I am NOT sure, the thermal patches are pending
there for almost half year and I did NOT receive any response, looks like no one
is maintaining the thermal sub-system?

> 
> In an earlier series the CLK_IS_CRITICAL flags was removed from the TMU
> clock so if the thermal driver doesn't explicitly enable it the system will hang
> on probe. This is what happens in linux-next right now!

The thermal driver should be built with module, so default kernel should can boot
up, do you modify the thermal driver as built-in?

> 
> Unless this patches is merged soon we'll end up with a 5.4-rc1 that doesn't
> boot on imx8mq. An easy fix would be to drop/revert commit
> 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.

If the thermal driver is built as module, I think no need to revert the commit, but
if by default thermal driver is built-in or mod probed, then yes, it should NOT break
kernel boot up.

Anson.

> 
> Merging patches out-of-order when they have hard (boot-breaking)
> dependencies also breaks bisect.
> 
> --
> Regards,
> Leonard
Zhang Rui Aug. 27, 2019, 3:09 a.m. UTC | #4
On Tue, 2019-08-27 at 01:51 +0000, Anson Huang wrote:
> > On 7/30/2019 5:31 AM, Anson.Huang@nxp.com wrote:
> > > From: Anson Huang <Anson.Huang@nxp.com>
> > > 
> > > Some platforms like i.MX8MQ has clock control for this module,
> > > need to
> > > add clock operations to make sure the driver is working properly.
> > > 
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > Reviewed-by: Guido Günther <agx@sigxcpu.org>
> > 
> > This series looks good, do you think it can be merged in time for
> > v5.4?
> > Today was v5.3-rc6.
> 
> If the question is for me, then I am NOT sure, the thermal patches
> are pending
> there for almost half year and I did NOT receive any response,

which patch series you're referring to?

>  looks like no one
> is maintaining the thermal sub-system?
> 

Eduardo is maintaining all the thermal-soc driver changes. Thus I
usually filtered out the soc driver patches in my mailbox.

The last email from Eduardo is that he is offline during this July and
will be back and taking patches in August.

I will double check with Eduardo anyway.

thanks,
rui


> > 
> > In an earlier series the CLK_IS_CRITICAL flags was removed from the
> > TMU
> > clock so if the thermal driver doesn't explicitly enable it the
> > system will hang
> > on probe. This is what happens in linux-next right now!
> 
> The thermal driver should be built with module, so default kernel
> should can boot
> up, do you modify the thermal driver as built-in?
> 
> > 
> > Unless this patches is merged soon we'll end up with a 5.4-rc1 that
> > doesn't
> > boot on imx8mq. An easy fix would be to drop/revert commit
> > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.
> 
> If the thermal driver is built as module, I think no need to revert
> the commit, but
> if by default thermal driver is built-in or mod probed, then yes, it
> should NOT break
> kernel boot up.
> 
> Anson.
> 
> > 
> > Merging patches out-of-order when they have hard (boot-breaking)
> > dependencies also breaks bisect.
> > 
> > --
> > Regards,
> > Leonard
Anson Huang Aug. 27, 2019, 3:24 a.m. UTC | #5
Hi, Rui

> On Tue, 2019-08-27 at 01:51 +0000, Anson Huang wrote:
> > > On 7/30/2019 5:31 AM, Anson.Huang@nxp.com wrote:
> > > > From: Anson Huang <Anson.Huang@nxp.com>
> > > >
> > > > Some platforms like i.MX8MQ has clock control for this module,
> > > > need to add clock operations to make sure the driver is working
> > > > properly.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > > Reviewed-by: Guido Günther <agx@sigxcpu.org>
> > >
> > > This series looks good, do you think it can be merged in time for
> > > v5.4?
> > > Today was v5.3-rc6.
> >
> > If the question is for me, then I am NOT sure, the thermal patches are
> > pending there for almost half year and I did NOT receive any response,
> 
> which patch series you're referring to?

Below i.MX8QXP patch series, I meant I started it from last year, the latest resend
version is 2 months ago: 

https://patchwork.kernel.org/patch/11000809/ 


> 
> >  looks like no one
> > is maintaining the thermal sub-system?

Below two patch series are also pending there for some time:

https://patchwork.kernel.org/patch/11032777/ 

https://patchwork.kernel.org/patch/11031297/ 

> >
> 
> Eduardo is maintaining all the thermal-soc driver changes. Thus I usually
> filtered out the soc driver patches in my mailbox.
> 
> The last email from Eduardo is that he is offline during this July and will be
> back and taking patches in August.
> 
> I will double check with Eduardo anyway.

Thanks, especially our i.MX8QXP thermal driver patch, I started it from last year and
due to some different opinion from different reviewers, this patch series version is
up to v15, but I did NOT receive response since June, appreciated if you guys can take
a look at this, I ping it many times but no feedback. Thanks!

https://patchwork.kernel.org/patch/11000809/

Anson.

> 
> thanks,
> rui
> 
> 
> > >
> > > In an earlier series the CLK_IS_CRITICAL flags was removed from the
> > > TMU clock so if the thermal driver doesn't explicitly enable it the
> > > system will hang on probe. This is what happens in linux-next right
> > > now!
> >
> > The thermal driver should be built with module, so default kernel
> > should can boot up, do you modify the thermal driver as built-in?
> >
> > >
> > > Unless this patches is merged soon we'll end up with a 5.4-rc1 that
> > > doesn't boot on imx8mq. An easy fix would be to drop/revert commit
> > > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.
> >
> > If the thermal driver is built as module, I think no need to revert
> > the commit, but if by default thermal driver is built-in or mod
> > probed, then yes, it should NOT break kernel boot up.
> >
> > Anson.
> >
> > >
> > > Merging patches out-of-order when they have hard (boot-breaking)
> > > dependencies also breaks bisect.
> > >
> > > --
> > > Regards,
> > > Leonard
Leonard Crestez Aug. 27, 2019, 12:41 p.m. UTC | #6
On 27.08.2019 04:51, Anson Huang wrote:
>> In an earlier series the CLK_IS_CRITICAL flags was removed from the TMU
>> clock so if the thermal driver doesn't explicitly enable it the system will hang
>> on probe. This is what happens in linux-next right now!
> 
> The thermal driver should be built with module, so default kernel should can boot
> up, do you modify the thermal driver as built-in?
> 
>> Unless this patches is merged soon we'll end up with a 5.4-rc1 that doesn't
>> boot on imx8mq. An easy fix would be to drop/revert commit
>> 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
>> IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.
> 
> If the thermal driver is built as module, I think no need to revert the commit, but
> if by default thermal driver is built-in or mod probed, then yes, it should NOT break
> kernel boot up.

The qoriq_thermal driver is built as a module in defconfig and when 
modules are properly installed in rootfs they will be automatically be 
probed on boot and cause a hang.

I usually run nfsroot with modules:

     make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-root

--
Regards,
Leonard
Zhang Rui Aug. 28, 2019, 8:32 a.m. UTC | #7
On Tue, 2019-08-27 at 12:41 +0000, Leonard Crestez wrote:
> On 27.08.2019 04:51, Anson Huang wrote:
> > > In an earlier series the CLK_IS_CRITICAL flags was removed from
> > > the TMU
> > > clock so if the thermal driver doesn't explicitly enable it the
> > > system will hang
> > > on probe. This is what happens in linux-next right now!
> > 
> > The thermal driver should be built with module, so default kernel
> > should can boot
> > up, do you modify the thermal driver as built-in?
> > 
> > > Unless this patches is merged soon we'll end up with a 5.4-rc1
> > > that doesn't
> > > boot on imx8mq. An easy fix would be to drop/revert commit
> > > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.
> > 
> > If the thermal driver is built as module, I think no need to revert
> > the commit, but
> > if by default thermal driver is built-in or mod probed, then yes,
> > it should NOT break
> > kernel boot up.
> 
> The qoriq_thermal driver is built as a module in defconfig and when 
> modules are properly installed in rootfs they will be automatically
> be 
> probed on boot and cause a hang.
> 
> I usually run nfsroot with modules:
> 
>      make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-root

so we need this patch shipped in the beginning of the merge window,
right?
if there is hard dependency between patches, it's better to send them
in one series, and get shipped via either tree.

BTW, who is maintaining qoriq driver from NXP? If Anson is maintaining
and developing this driver, it's better to update this in the driver or
the MAINTAINER file, I will take the driver specific patches as long as
we have ACK/Reviewed-By from the driver maintainer.

thanks,
rui

> 
> --
> Regards,
> Leonard
Zhang Rui Aug. 28, 2019, 8:35 a.m. UTC | #8
On Wed, 2019-08-28 at 16:32 +0800, Zhang Rui wrote:
> On Tue, 2019-08-27 at 12:41 +0000, Leonard Crestez wrote:
> > On 27.08.2019 04:51, Anson Huang wrote:
> > > > In an earlier series the CLK_IS_CRITICAL flags was removed from
> > > > the TMU
> > > > clock so if the thermal driver doesn't explicitly enable it the
> > > > system will hang
> > > > on probe. This is what happens in linux-next right now!
> > > 
> > > The thermal driver should be built with module, so default kernel
> > > should can boot
> > > up, do you modify the thermal driver as built-in?
> > > 
> > > > Unless this patches is merged soon we'll end up with a 5.4-rc1
> > > > that doesn't
> > > > boot on imx8mq. An easy fix would be to drop/revert commit
> > > > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.
> > > 
> > > If the thermal driver is built as module, I think no need to
> > > revert
> > > the commit, but
> > > if by default thermal driver is built-in or mod probed, then yes,
> > > it should NOT break
> > > kernel boot up.
> > 
> > The qoriq_thermal driver is built as a module in defconfig and
> > when 
> > modules are properly installed in rootfs they will be automatically
> > be 
> > probed on boot and cause a hang.
> > 
> > I usually run nfsroot with modules:
> > 
> >      make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-root
> 
> so we need this patch shipped in the beginning of the merge window,
> right?
> if there is hard dependency between patches, it's better to send them
> in one series, and get shipped via either tree.
> 
> BTW, who is maintaining qoriq driver from NXP? If Anson is
> maintaining
> and developing this driver, it's better to update this in the driver
> or
> the MAINTAINER file, I will take the driver specific patches as long
> as
> we have ACK/Reviewed-By from the driver maintainer.

And also, can you provide your feedback for this one?
https://patchwork.kernel.org/patch/10974147/

thanks,
rui
> 
> thanks,
> rui
> 
> > 
> > --
> > Regards,
> > Leonard
Anson Huang Aug. 28, 2019, 8:49 a.m. UTC | #9
Hi, Rui

> On Wed, 2019-08-28 at 16:32 +0800, Zhang Rui wrote:
> > On Tue, 2019-08-27 at 12:41 +0000, Leonard Crestez wrote:
> > > On 27.08.2019 04:51, Anson Huang wrote:
> > > > > In an earlier series the CLK_IS_CRITICAL flags was removed from
> > > > > the TMU clock so if the thermal driver doesn't explicitly enable
> > > > > it the system will hang on probe. This is what happens in
> > > > > linux-next right now!
> > > >
> > > > The thermal driver should be built with module, so default kernel
> > > > should can boot up, do you modify the thermal driver as built-in?
> > > >
> > > > > Unless this patches is merged soon we'll end up with a 5.4-rc1
> > > > > that doesn't boot on imx8mq. An easy fix would be to drop/revert
> > > > > commit
> > > > > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > > > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.
> > > >
> > > > If the thermal driver is built as module, I think no need to
> > > > revert the commit, but if by default thermal driver is built-in or
> > > > mod probed, then yes, it should NOT break kernel boot up.
> > >
> > > The qoriq_thermal driver is built as a module in defconfig and when
> > > modules are properly installed in rootfs they will be automatically
> > > be probed on boot and cause a hang.
> > >
> > > I usually run nfsroot with modules:
> > >
> > >      make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-root
> >
> > so we need this patch shipped in the beginning of the merge window,
> > right?
> > if there is hard dependency between patches, it's better to send them
> > in one series, and get shipped via either tree.
> >
> > BTW, who is maintaining qoriq driver from NXP? If Anson is maintaining
> > and developing this driver, it's better to update this in the driver
> > or the MAINTAINER file, I will take the driver specific patches as
> > long as we have ACK/Reviewed-By from the driver maintainer.

I am NOT sure who is the qoriq driver from NXP, some of our i.MX SoCs use
qoriq thermal IP, so I have to add support for them.  The first maintainer for
this driver is hongtao.jia@nxp.com, but I can NOT find it from NXP's mail system,
NOT sure if he is still in NXP. The MAINTAINER file does NOT have info for this
Driver's maintainer, so how to update it? Just change the name in driver? Or leave
it as what it is?

 MODULE_AUTHOR("Jia Hongtao <hongtao.jia@nxp.com>");
 MODULE_DESCRIPTION("QorIQ Thermal Monitoring Unit driver");
 MODULE_LICENSE("GPL v2");

> 
> And also, can you provide your feedback for this one?
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.kernel.org%2Fpatch%2F10974147%2F&amp;data=02%7C01%7Canson.h
> uang%40nxp.com%7C887e7c90f7c943ff0d9b08d72b92aea1%7C686ea1d3bc2
> b4c6fa92cd99c5c301635%7C0%7C0%7C637025781325203384&amp;sdata=Xg
> tX6mPdA50Nbb%2FmnS2om2bJNepTd1th6HmfwGuU9Hw%3D&amp;reserve
> d=0

I can take a look at it later.

Thanks,
Anson
Anson Huang Aug. 28, 2019, 8:51 a.m. UTC | #10
Hi, Rui

> On Tue, 2019-08-27 at 12:41 +0000, Leonard Crestez wrote:
> > On 27.08.2019 04:51, Anson Huang wrote:
> > > > In an earlier series the CLK_IS_CRITICAL flags was removed from
> > > > the TMU clock so if the thermal driver doesn't explicitly enable
> > > > it the system will hang on probe. This is what happens in
> > > > linux-next right now!
> > >
> > > The thermal driver should be built with module, so default kernel
> > > should can boot up, do you modify the thermal driver as built-in?
> > >
> > > > Unless this patches is merged soon we'll end up with a 5.4-rc1
> > > > that doesn't boot on imx8mq. An easy fix would be to drop/revert
> > > > commit
> > > > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.
> > >
> > > If the thermal driver is built as module, I think no need to revert
> > > the commit, but if by default thermal driver is built-in or mod
> > > probed, then yes, it should NOT break kernel boot up.
> >
> > The qoriq_thermal driver is built as a module in defconfig and when
> > modules are properly installed in rootfs they will be automatically be
> > probed on boot and cause a hang.
> >
> > I usually run nfsroot with modules:
> >
> >      make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-root
> 
> so we need this patch shipped in the beginning of the merge window, right?
> if there is hard dependency between patches, it's better to send them in one
> series, and get shipped via either tree.

There is no hard dependency in this patch series. Previous for the TMU clock disabled
patch, since thermal driver is built as module so I did NOT found the issue. The patch
series is the correct fix.

Thanks,
Anson 

> 
> BTW, who is maintaining qoriq driver from NXP? If Anson is maintaining and
> developing this driver, it's better to update this in the driver or the
> MAINTAINER file, I will take the driver specific patches as long as we have
> ACK/Reviewed-By from the driver maintainer.
> 
> thanks,
> rui
> 
> >
> > --
> > Regards,
> > Leonard
Zhang Rui Aug. 28, 2019, 9:01 a.m. UTC | #11
On Wed, 2019-08-28 at 08:49 +0000, Anson Huang wrote:
> Hi, Rui
> 
> > On Wed, 2019-08-28 at 16:32 +0800, Zhang Rui wrote:
> > > On Tue, 2019-08-27 at 12:41 +0000, Leonard Crestez wrote:
> > > > On 27.08.2019 04:51, Anson Huang wrote:
> > > > > > In an earlier series the CLK_IS_CRITICAL flags was removed
> > > > > > from
> > > > > > the TMU clock so if the thermal driver doesn't explicitly
> > > > > > enable
> > > > > > it the system will hang on probe. This is what happens in
> > > > > > linux-next right now!
> > > > > 
> > > > > The thermal driver should be built with module, so default
> > > > > kernel
> > > > > should can boot up, do you modify the thermal driver as
> > > > > built-in?
> > > > > 
> > > > > > Unless this patches is merged soon we'll end up with a 5.4-
> > > > > > rc1
> > > > > > that doesn't boot on imx8mq. An easy fix would be to
> > > > > > drop/revert
> > > > > > commit
> > > > > > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > > > > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are
> > > > > > accepted.
> > > > > 
> > > > > If the thermal driver is built as module, I think no need to
> > > > > revert the commit, but if by default thermal driver is built-
> > > > > in or
> > > > > mod probed, then yes, it should NOT break kernel boot up.
> > > > 
> > > > The qoriq_thermal driver is built as a module in defconfig and
> > > > when
> > > > modules are properly installed in rootfs they will be
> > > > automatically
> > > > be probed on boot and cause a hang.
> > > > 
> > > > I usually run nfsroot with modules:
> > > > 
> > > >      make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-root
> > > 
> > > so we need this patch shipped in the beginning of the merge
> > > window,
> > > right?
> > > if there is hard dependency between patches, it's better to send
> > > them
> > > in one series, and get shipped via either tree.
> > > 
> > > BTW, who is maintaining qoriq driver from NXP? If Anson is
> > > maintaining
> > > and developing this driver, it's better to update this in the
> > > driver
> > > or the MAINTAINER file, I will take the driver specific patches
> > > as
> > > long as we have ACK/Reviewed-By from the driver maintainer.
> 
> I am NOT sure who is the qoriq driver from NXP, some of our i.MX SoCs
> use
> qoriq thermal IP, so I have to add support for them.  The first
> maintainer for
> this driver is hongtao.jia@nxp.com, but I can NOT find it from NXP's
> mail system,
> NOT sure if he is still in NXP. The MAINTAINER file does NOT have
> info for this
> Driver's maintainer, so how to update it? Just change the name in
> driver? Or leave
> it as what it is?
> 
>  MODULE_AUTHOR("Jia Hongtao <hongtao.jia@nxp.com>");
>  MODULE_DESCRIPTION("QorIQ Thermal Monitoring Unit driver");
>  MODULE_LICENSE("GPL v2");
> 
I see several people are actively working on this driver from NXP.
If you're okay, I'd like to get your comments on all the patches for
this driver before I take them, and I can update the MAINTAINER file
later so that you're CCed for all the qoriq thermal driver changes. 

> > 
> > And also, can you provide your feedback for this one?
> > 
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> > work.kernel.org%2Fpatch%2F10974147%2F&amp;data=02%7C01%7Canson.h
> > uang%40nxp.com%7C887e7c90f7c943ff0d9b08d72b92aea1%7C686ea1d3bc2
> > b4c6fa92cd99c5c301635%7C0%7C0%7C637025781325203384&amp;sdata=Xg
> > tX6mPdA50Nbb%2FmnS2om2bJNepTd1th6HmfwGuU9Hw%3D&amp;reserve
> > d=0
> 
> I can take a look at it later.
> 
thanks!

-rui
> Thanks,
> Anson
Zhang Rui Aug. 28, 2019, 9:03 a.m. UTC | #12
On Wed, 2019-08-28 at 08:51 +0000, Anson Huang wrote:
> Hi, Rui
> 
> > On Tue, 2019-08-27 at 12:41 +0000, Leonard Crestez wrote:
> > > On 27.08.2019 04:51, Anson Huang wrote:
> > > > > In an earlier series the CLK_IS_CRITICAL flags was removed
> > > > > from
> > > > > the TMU clock so if the thermal driver doesn't explicitly
> > > > > enable
> > > > > it the system will hang on probe. This is what happens in
> > > > > linux-next right now!
> > > > 
> > > > The thermal driver should be built with module, so default
> > > > kernel
> > > > should can boot up, do you modify the thermal driver as built-
> > > > in?
> > > > 
> > > > > Unless this patches is merged soon we'll end up with a 5.4-
> > > > > rc1
> > > > > that doesn't boot on imx8mq. An easy fix would be to
> > > > > drop/revert
> > > > > commit
> > > > > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > > > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.
> > > > 
> > > > If the thermal driver is built as module, I think no need to
> > > > revert
> > > > the commit, but if by default thermal driver is built-in or mod
> > > > probed, then yes, it should NOT break kernel boot up.
> > > 
> > > The qoriq_thermal driver is built as a module in defconfig and
> > > when
> > > modules are properly installed in rootfs they will be
> > > automatically be
> > > probed on boot and cause a hang.
> > > 
> > > I usually run nfsroot with modules:
> > > 
> > >      make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-root
> > 
> > so we need this patch shipped in the beginning of the merge window,
> > right?
> > if there is hard dependency between patches, it's better to send
> > them in one
> > series, and get shipped via either tree.
> 
> There is no hard dependency in this patch series. Previous for the
> TMU clock disabled
> patch, since thermal driver is built as module so I did NOT found the
> issue. The patch
> series is the correct fix.
> 
Got it.
the clock patch is also queued for 5.4-rc1, right?
I will apply this series and try to push it as early as possible during
the merge window.

thanks,
rui
> Thanks,
> Anson 
> 
> > 
> > BTW, who is maintaining qoriq driver from NXP? If Anson is
> > maintaining and
> > developing this driver, it's better to update this in the driver or
> > the
> > MAINTAINER file, I will take the driver specific patches as long as
> > we have
> > ACK/Reviewed-By from the driver maintainer.
> > 
> > thanks,
> > rui
> > 
> > > 
> > > --
> > > Regards,
> > > Leonard
> 
>
Anson Huang Aug. 28, 2019, 9:10 a.m. UTC | #13
Hi, Rui

> On Wed, 2019-08-28 at 08:51 +0000, Anson Huang wrote:
> > Hi, Rui
> >
> > > On Tue, 2019-08-27 at 12:41 +0000, Leonard Crestez wrote:
> > > > On 27.08.2019 04:51, Anson Huang wrote:
> > > > > > In an earlier series the CLK_IS_CRITICAL flags was removed
> > > > > > from the TMU clock so if the thermal driver doesn't explicitly
> > > > > > enable it the system will hang on probe. This is what happens
> > > > > > in linux-next right now!
> > > > >
> > > > > The thermal driver should be built with module, so default
> > > > > kernel should can boot up, do you modify the thermal driver as
> > > > > built- in?
> > > > >
> > > > > > Unless this patches is merged soon we'll end up with a 5.4-
> > > > > > rc1
> > > > > > that doesn't boot on imx8mq. An easy fix would be to
> > > > > > drop/revert commit
> > > > > > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > > > > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are accepted.
> > > > >
> > > > > If the thermal driver is built as module, I think no need to
> > > > > revert the commit, but if by default thermal driver is built-in
> > > > > or mod probed, then yes, it should NOT break kernel boot up.
> > > >
> > > > The qoriq_thermal driver is built as a module in defconfig and
> > > > when modules are properly installed in rootfs they will be
> > > > automatically be probed on boot and cause a hang.
> > > >
> > > > I usually run nfsroot with modules:
> > > >
> > > >      make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-root
> > >
> > > so we need this patch shipped in the beginning of the merge window,
> > > right?
> > > if there is hard dependency between patches, it's better to send
> > > them in one series, and get shipped via either tree.
> >
> > There is no hard dependency in this patch series. Previous for the TMU
> > clock disabled patch, since thermal driver is built as module so I did
> > NOT found the issue. The patch series is the correct fix.
> >
> Got it.
> the clock patch is also queued for 5.4-rc1, right?
> I will apply this series and try to push it as early as possible during the merge
> window.

The clock patch is as below in Linux-next tree, while I did NOT see it in v5.3-rc6,
so it should be queued for 5.4-rc1, right?
Thanks for taking the patch series!


commit 951c1aef9691491ddf4dd5aab76f2665d56bd5d3
Author: Anson Huang <Anson.Huang@nxp.com>
Date:   Fri Jul 5 12:56:11 2019 +0800

    clk: imx8mq: Remove CLK_IS_CRITICAL flag for IMX8MQ_CLK_TMU_ROOT

    IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
    should manage this clock, so no need to have CLK_IS_CRITICAL flag
    set.

    Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
    Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
    Acked-by: Stephen Boyd <sboyd@kernel.org>
    Signed-off-by: Shawn Guo <shawnguo@kernel.org>

drivers/clk/imx/clk-imx8mq.c


Thanks,
Anson
Anson Huang Aug. 28, 2019, 9:12 a.m. UTC | #14
Hi, Rui

> On Wed, 2019-08-28 at 08:49 +0000, Anson Huang wrote:
> > Hi, Rui
> >
> > > On Wed, 2019-08-28 at 16:32 +0800, Zhang Rui wrote:
> > > > On Tue, 2019-08-27 at 12:41 +0000, Leonard Crestez wrote:
> > > > > On 27.08.2019 04:51, Anson Huang wrote:
> > > > > > > In an earlier series the CLK_IS_CRITICAL flags was removed
> > > > > > > from the TMU clock so if the thermal driver doesn't
> > > > > > > explicitly enable it the system will hang on probe. This is
> > > > > > > what happens in linux-next right now!
> > > > > >
> > > > > > The thermal driver should be built with module, so default
> > > > > > kernel should can boot up, do you modify the thermal driver as
> > > > > > built-in?
> > > > > >
> > > > > > > Unless this patches is merged soon we'll end up with a 5.4-
> > > > > > > rc1
> > > > > > > that doesn't boot on imx8mq. An easy fix would be to
> > > > > > > drop/revert commit
> > > > > > > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > > > > > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are
> > > > > > > accepted.
> > > > > >
> > > > > > If the thermal driver is built as module, I think no need to
> > > > > > revert the commit, but if by default thermal driver is built-
> > > > > > in or mod probed, then yes, it should NOT break kernel boot
> > > > > > up.
> > > > >
> > > > > The qoriq_thermal driver is built as a module in defconfig and
> > > > > when modules are properly installed in rootfs they will be
> > > > > automatically be probed on boot and cause a hang.
> > > > >
> > > > > I usually run nfsroot with modules:
> > > > >
> > > > >      make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-root
> > > >
> > > > so we need this patch shipped in the beginning of the merge
> > > > window, right?
> > > > if there is hard dependency between patches, it's better to send
> > > > them in one series, and get shipped via either tree.
> > > >
> > > > BTW, who is maintaining qoriq driver from NXP? If Anson is
> > > > maintaining and developing this driver, it's better to update this
> > > > in the driver or the MAINTAINER file, I will take the driver
> > > > specific patches as long as we have ACK/Reviewed-By from the
> > > > driver maintainer.
> >
> > I am NOT sure who is the qoriq driver from NXP, some of our i.MX SoCs
> > use qoriq thermal IP, so I have to add support for them.  The first
> > maintainer for this driver is hongtao.jia@nxp.com, but I can NOT find
> > it from NXP's mail system, NOT sure if he is still in NXP. The
> > MAINTAINER file does NOT have info for this Driver's maintainer, so
> > how to update it? Just change the name in driver? Or leave it as what
> > it is?
> >
> >  MODULE_AUTHOR("Jia Hongtao <hongtao.jia@nxp.com>");
> > MODULE_DESCRIPTION("QorIQ Thermal Monitoring Unit driver");
> > MODULE_LICENSE("GPL v2");
> >
> I see several people are actively working on this driver from NXP.
> If you're okay, I'd like to get your comments on all the patches for this driver
> before I take them, and I can update the MAINTAINER file later so that you're
> CCed for all the qoriq thermal driver changes.

I am OK, but it may take me some days to do it, I will make it ASAP, thanks.

Anson

> 
> > >
> > > And also, can you provide your feedback for this one?
> > >
> https://patch
> > >
> work.kernel.org%2Fpatch%2F10974147%2F&amp;data=02%7C01%7Canson.h
> > >
> uang%40nxp.com%7C887e7c90f7c943ff0d9b08d72b92aea1%7C686ea1d3bc2
> > >
> b4c6fa92cd99c5c301635%7C0%7C0%7C637025781325203384&amp;sdata=Xg
> > >
> tX6mPdA50Nbb%2FmnS2om2bJNepTd1th6HmfwGuU9Hw%3D&amp;reserve
> > > d=0
> >
> > I can take a look at it later.
> >
> thanks!
> 
> -rui
> > Thanks,
> > Anson
Anson Huang Aug. 29, 2019, 2:49 a.m. UTC | #15
Hi, Rui

> > On Wed, 2019-08-28 at 08:51 +0000, Anson Huang wrote:
> > > Hi, Rui
> > >
> > > > On Tue, 2019-08-27 at 12:41 +0000, Leonard Crestez wrote:
> > > > > On 27.08.2019 04:51, Anson Huang wrote:
> > > > > > > In an earlier series the CLK_IS_CRITICAL flags was removed
> > > > > > > from the TMU clock so if the thermal driver doesn't
> > > > > > > explicitly enable it the system will hang on probe. This is
> > > > > > > what happens in linux-next right now!
> > > > > >
> > > > > > The thermal driver should be built with module, so default
> > > > > > kernel should can boot up, do you modify the thermal driver as
> > > > > > built- in?
> > > > > >
> > > > > > > Unless this patches is merged soon we'll end up with a 5.4-
> > > > > > > rc1
> > > > > > > that doesn't boot on imx8mq. An easy fix would be to
> > > > > > > drop/revert commit
> > > > > > > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > > > > > > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are
> accepted.
> > > > > >
> > > > > > If the thermal driver is built as module, I think no need to
> > > > > > revert the commit, but if by default thermal driver is
> > > > > > built-in or mod probed, then yes, it should NOT break kernel boot
> up.
> > > > >
> > > > > The qoriq_thermal driver is built as a module in defconfig and
> > > > > when modules are properly installed in rootfs they will be
> > > > > automatically be probed on boot and cause a hang.
> > > > >
> > > > > I usually run nfsroot with modules:
> > > > >
> > > > >      make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-root
> > > >
> > > > so we need this patch shipped in the beginning of the merge
> > > > window, right?
> > > > if there is hard dependency between patches, it's better to send
> > > > them in one series, and get shipped via either tree.
> > >
> > > There is no hard dependency in this patch series. Previous for the
> > > TMU clock disabled patch, since thermal driver is built as module so
> > > I did NOT found the issue. The patch series is the correct fix.
> > >
> > Got it.
> > the clock patch is also queued for 5.4-rc1, right?
> > I will apply this series and try to push it as early as possible
> > during the merge window.
> 
> The clock patch is as below in Linux-next tree, while I did NOT see it in v5.3-
> rc6, so it should be queued for 5.4-rc1, right?
> Thanks for taking the patch series!

Sorry for pushing, so you will apply this patch series to avoid the i.MX8MQ kernel boot up hang
caused by insmod qoriq thermal driver, right? Then we no need to revert that TMU clock patch
951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for IMX8MQ_CLK_TMU_ROOT").

Thanks,
Anson

> 
> 
> commit 951c1aef9691491ddf4dd5aab76f2665d56bd5d3
> Author: Anson Huang <Anson.Huang@nxp.com>
> Date:   Fri Jul 5 12:56:11 2019 +0800
> 
>     clk: imx8mq: Remove CLK_IS_CRITICAL flag for IMX8MQ_CLK_TMU_ROOT
> 
>     IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
>     should manage this clock, so no need to have CLK_IS_CRITICAL flag
>     set.
> 
>     Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
>     Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
>     Acked-by: Stephen Boyd <sboyd@kernel.org>
>     Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> 
> drivers/clk/imx/clk-imx8mq.c
> 
> 
> Thanks,
> Anson
Zhang Rui Aug. 29, 2019, 3:01 a.m. UTC | #16
On Thu, 2019-08-29 at 02:49 +0000, Anson Huang wrote:
> Hi, Rui
> 
> > > On Wed, 2019-08-28 at 08:51 +0000, Anson Huang wrote:
> > > > Hi, Rui
> > > > 
> > > > > On Tue, 2019-08-27 at 12:41 +0000, Leonard Crestez wrote:
> > > > > > On 27.08.2019 04:51, Anson Huang wrote:
> > > > > > > > In an earlier series the CLK_IS_CRITICAL flags was
> > > > > > > > removed
> > > > > > > > from the TMU clock so if the thermal driver doesn't
> > > > > > > > explicitly enable it the system will hang on probe.
> > > > > > > > This is
> > > > > > > > what happens in linux-next right now!
> > > > > > > 
> > > > > > > The thermal driver should be built with module, so
> > > > > > > default
> > > > > > > kernel should can boot up, do you modify the thermal
> > > > > > > driver as
> > > > > > > built- in?
> > > > > > > 
> > > > > > > > Unless this patches is merged soon we'll end up with a
> > > > > > > > 5.4-
> > > > > > > > rc1
> > > > > > > > that doesn't boot on imx8mq. An easy fix would be to
> > > > > > > > drop/revert commit
> > > > > > > > 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag
> > > > > > > > for
> > > > > > > > IMX8MQ_CLK_TMU_ROOT") until the thermal patches are
> > 
> > accepted.
> > > > > > > 
> > > > > > > If the thermal driver is built as module, I think no need
> > > > > > > to
> > > > > > > revert the commit, but if by default thermal driver is
> > > > > > > built-in or mod probed, then yes, it should NOT break
> > > > > > > kernel boot
> > 
> > up.
> > > > > > 
> > > > > > The qoriq_thermal driver is built as a module in defconfig
> > > > > > and
> > > > > > when modules are properly installed in rootfs they will be
> > > > > > automatically be probed on boot and cause a hang.
> > > > > > 
> > > > > > I usually run nfsroot with modules:
> > > > > > 
> > > > > >      make modules_install INSTALL_MOD_PATH=/srv/nfs/imx8-
> > > > > > root
> > > > > 
> > > > > so we need this patch shipped in the beginning of the merge
> > > > > window, right?
> > > > > if there is hard dependency between patches, it's better to
> > > > > send
> > > > > them in one series, and get shipped via either tree.
> > > > 
> > > > There is no hard dependency in this patch series. Previous for
> > > > the
> > > > TMU clock disabled patch, since thermal driver is built as
> > > > module so
> > > > I did NOT found the issue. The patch series is the correct fix.
> > > > 
> > > 
> > > Got it.
> > > the clock patch is also queued for 5.4-rc1, right?
> > > I will apply this series and try to push it as early as possible
> > > during the merge window.
> > 
> > The clock patch is as below in Linux-next tree, while I did NOT see
> > it in v5.3-
> > rc6, so it should be queued for 5.4-rc1, right?
> > Thanks for taking the patch series!
> 
> Sorry for pushing, so you will apply this patch series to avoid the
> i.MX8MQ kernel boot up hang
> caused by insmod qoriq thermal driver, right? Then we no need to
> revert that TMU clock patch
> 951c1aef9691 ("clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> IMX8MQ_CLK_TMU_ROOT").
> 
right. I will queue it for 5.4-rc1.

thanks,
rui

> Thanks,
> Anson
> 
> > 
> > 
> > commit 951c1aef9691491ddf4dd5aab76f2665d56bd5d3
> > Author: Anson Huang <Anson.Huang@nxp.com>
> > Date:   Fri Jul 5 12:56:11 2019 +0800
> > 
> >     clk: imx8mq: Remove CLK_IS_CRITICAL flag for
> > IMX8MQ_CLK_TMU_ROOT
> > 
> >     IMX8MQ_CLK_TMU_ROOT is ONLY used for thermal module, the driver
> >     should manage this clock, so no need to have CLK_IS_CRITICAL
> > flag
> >     set.
> > 
> >     Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> >     Reviewed-by: Abel Vesa <abel.vesa@nxp.com>
> >     Acked-by: Stephen Boyd <sboyd@kernel.org>
> >     Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> > 
> > drivers/clk/imx/clk-imx8mq.c
> > 
> > 
> > Thanks,
> > Anson

Patch
diff mbox series

diff --git a/drivers/thermal/qoriq_thermal.c b/drivers/thermal/qoriq_thermal.c
index 7b36493..2893947 100644
--- a/drivers/thermal/qoriq_thermal.c
+++ b/drivers/thermal/qoriq_thermal.c
@@ -2,6 +2,7 @@ 
 //
 // Copyright 2016 Freescale Semiconductor, Inc.
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>
@@ -72,6 +73,7 @@  struct qoriq_sensor {
 
 struct qoriq_tmu_data {
 	struct qoriq_tmu_regs __iomem *regs;
+	struct clk *clk;
 	bool little_endian;
 	struct qoriq_sensor	*sensor[SITES_MAX];
 };
@@ -209,6 +211,16 @@  static int qoriq_tmu_probe(struct platform_device *pdev)
 		goto err_iomap;
 	}
 
+	data->clk = devm_clk_get_optional(&pdev->dev, NULL);
+	if (IS_ERR(data->clk))
+		return PTR_ERR(data->clk);
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to enable clock\n");
+		return ret;
+	}
+
 	qoriq_tmu_init_device(data);	/* TMU initialization */
 
 	ret = qoriq_tmu_calibration(pdev);	/* TMU calibration */
@@ -225,6 +237,7 @@  static int qoriq_tmu_probe(struct platform_device *pdev)
 	return 0;
 
 err_tmu:
+	clk_disable_unprepare(data->clk);
 	iounmap(data->regs);
 
 err_iomap:
@@ -241,6 +254,9 @@  static int qoriq_tmu_remove(struct platform_device *pdev)
 	tmu_write(data, TMR_DISABLE, &data->regs->tmr);
 
 	iounmap(data->regs);
+
+	clk_disable_unprepare(data->clk);
+
 	platform_set_drvdata(pdev, NULL);
 
 	return 0;
@@ -257,14 +273,21 @@  static int qoriq_tmu_suspend(struct device *dev)
 	tmr &= ~TMR_ME;
 	tmu_write(data, tmr, &data->regs->tmr);
 
+	clk_disable_unprepare(data->clk);
+
 	return 0;
 }
 
 static int qoriq_tmu_resume(struct device *dev)
 {
 	u32 tmr;
+	int ret;
 	struct qoriq_tmu_data *data = dev_get_drvdata(dev);
 
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		return ret;
+
 	/* Enable monitoring */
 	tmr = tmu_read(data, &data->regs->tmr);
 	tmr |= TMR_ME;