Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1661483 > unrolled thread

Re: [PATCH 2/2] ARM: dts: imx6ul: Add imx6ul-tempmon

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] ARM: dts: imx6ul: Add imx6ul-tempmon Fabio Estevam <festevam@gmail.com> - 2017-06-08 18:50 +0200
    Re: [PATCH 2/2] ARM: dts: imx6ul: Add imx6ul-tempmon Fabio Estevam <festevam@gmail.com> - 2017-06-09 14:20 +0200
    Re: [PATCH 2/2] ARM: dts: imx6ul: Add imx6ul-tempmon Lothar Waßmann <LW@KARO-electronics.de> - 2017-06-09 16:00 +0200
      Re: [PATCH 2/2] ARM: dts: imx6ul: Add imx6ul-tempmon Lothar Waßmann <LW@KARO-electronics.de> - 2017-06-12 12:50 +0200
        Re: [PATCH 2/2] ARM: dts: imx6ul: Add imx6ul-tempmon Fabio Estevam <festevam@gmail.com> - 2017-06-12 15:30 +0200
          Re: [PATCH 2/2] ARM: dts: imx6ul: Add imx6ul-tempmon Lothar Waßmann <LW@KARO-electronics.de> - 2017-06-12 16:30 +0200
            Re: [PATCH 2/2] ARM: dts: imx6ul: Add imx6ul-tempmon Fabio Estevam <festevam@gmail.com> - 2017-06-12 16:40 +0200

#1661483 — Re: [PATCH 2/2] ARM: dts: imx6ul: Add imx6ul-tempmon

On Thu, Jun 8, 2017 at 1:26 PM, Leonard Crestez <leonard.crestez@nxp.com> wrote:

> +                       tempmon: tempmon {
> +                               compatible = "fsl,imx6ul-tempmon", "fsl,imx6sx-tempmon";
> +                               interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>;
> +                               fsl,tempmon = <&anatop>;
> +                               fsl,tempmon-data = <&ocotp>;
> +                               clocks = <&clks IMX6UL_CLK_PLL3_USB_OTG>;

Does the IMX6UL_CLK_PLL3_USB_OTG clock really control tempmon? Please
double check.

[toc] | [next] | [standalone]

#1662370

On Fri, Jun 9, 2017 at 7:58 AM, Leonard Crestez <leonard.crestez@nxp.com> wrote:

> Yes, as far as I can tell the tempmon block uses the 480 Mhz PLL3 clock
> directly. This is similar to other imx6 SOCs. This PLL is used for
> stuff like USB but not only that. My understanding is the _USB_OTG
> suffix is descriptive, similar to PLL4_AUDIO and PLL6_ENET. Other non-
> usb components use PLL3 (like UART) but through other gates/dividers.

Yes, PLL3 can be a parent for the UART clock, but UART has its own clock gate.

> Setting this to IMX6UL_CLK_DUMMY will cause temperature reads to fail.
> Even if PLL3 usually ends up being constantly enabled because of uarts
> this is not true at imx_thermal_probe time (or uarts can be disabled).

Ok, thanks for confirming. It was not obvious from reading the
reference manual that the PLL3 clock is the gate for tempmon.

[toc] | [prev] | [next] | [standalone]

#1662474

Hi,

On Fri, 9 Jun 2017 13:58:15 +0300 Leonard Crestez wrote:
> On Thu, 2017-06-08 at 13:45 -0300, Fabio Estevam wrote:
> > On Thu, Jun 8, 2017 at 1:26 PM, Leonard Crestez  wrote:
> > 
> > > 
> > > +                       tempmon: tempmon {
> > > +                               compatible = "fsl,imx6ul-tempmon", "fsl,imx6sx-tempmon";
> > > +                               interrupts = ;
> > > +                               fsl,tempmon = <&anatop>;
> > > +                               fsl,tempmon-data = <&ocotp>;
> > > +                               clocks = <&clks IMX6UL_CLK_PLL3_USB_OTG>;
> > Does the IMX6UL_CLK_PLL3_USB_OTG clock really control tempmon? Please
> > double check.
> 
> Yes, as far as I can tell the tempmon block uses the 480 Mhz PLL3 clock
> directly. This is similar to other imx6 SOCs. This PLL is used for
> stuff like USB but not only that. My understanding is the _USB_OTG
> suffix is descriptive, similar to PLL4_AUDIO and PLL6_ENET. Other non-
> usb components use PLL3 (like UART) but through other gates/dividers.
> 
> Setting this to IMX6UL_CLK_DUMMY will cause temperature reads to fail.
> Even if PLL3 usually ends up being constantly enabled because of uarts
> this is not true at imx_thermal_probe time (or uarts can be disabled).
>
Since the driver is accessing the OCOTP registers it definitely needs
the IMX6UL_CLK_OCOTP too!


Lothar Waßmann

[toc] | [prev] | [next] | [standalone]

#1663409

Hi,

On Fri, 9 Jun 2017 18:34:44 +0300 Leonard Crestez wrote:
> On Fri, 2017-06-09 at 15:46 +0200, Lothar Waßmann wrote:
> > On Fri, 9 Jun 2017 13:58:15 +0300 Leonard Crestez wrote:
> > > On Thu, 2017-06-08 at 13:45 -0300, Fabio Estevam wrote:
> > > > On Thu, Jun 8, 2017 at 1:26 PM, Leonard Crestez  wrote:
> > > > > +                       tempmon: tempmon {
> > > > > +                               compatible = "fsl,imx6ul-tempmon", "fsl,imx6sx-tempmon";
> > > > > +                               interrupts = ;
> > > > > +                               fsl,tempmon = <&anatop>;
> > > > > +                               fsl,tempmon-data = <&ocotp>;
> > > > > +                               clocks = <&clks IMX6UL_CLK_PLL3_USB_OTG>;
> > > > Does the IMX6UL_CLK_PLL3_USB_OTG clock really control tempmon? Please
> > > > double check.
> > > Yes, as far as I can tell the tempmon block uses the 480 Mhz PLL3 clock
> > > directly. This is similar to other imx6 SOCs. This PLL is used for
> > > stuff like USB but not only that. My understanding is the _USB_OTG
> > > suffix is descriptive, similar to PLL4_AUDIO and PLL6_ENET. Other non-
> > > usb components use PLL3 (like UART) but through other gates/dividers.
> > > 
> > > Setting this to IMX6UL_CLK_DUMMY will cause temperature reads to fail.
> > > Even if PLL3 usually ends up being constantly enabled because of uarts
> > > this is not true at imx_thermal_probe time (or uarts can be disabled).
> > > 
> > Since the driver is accessing the OCOTP registers it definitely needs
> > the IMX6UL_CLK_OCOTP too!
> > 
> I don't think so. OCOTP reads fuse values into shadows registers on
> chip reset and values seem to be available even if it's specific OCOTP
>
Shadow registers are registers nonetheless which require a clock for
accessing them.

> clock is off. I think that clock is only required for writes or shadow
> updates.
> 
Sometimes it helps to read the Reference Manual or take hardware and try
it out. The i.MX6(Q|UL) Reference Manual lists the required clocks as:
                                  Table 35-1. OCOTP Clocks
Clock name       Clock Root           Description
ipg_clk          ipg_clk_root         Peripheral clock
ipg_clk_s        ipg_clk_root         Peripheral access clock

and Table 18-3. "System Clocks, Gating, and Override (continued)"
for i.MX6UL shows:
OCOTP IPG_CLK   IPG_CLK_ROOT CCGR2[CG6] (IIM_CLK_ENABLE)
      IPG_CLK_S IPG_CLK_ROOT CCGR2[CG6] (IIM_CLK_ENABLE)

while the same table for i.MX6Q shows:
OCOTP ipg_clk   ipg_clk_root CCGR2[CG6] (iim_clk_enable)
      ipg_clk_s ipg_clk_root

with a blank entry in the "Clock Gating" column for the access clock.
So, for i.MX6Q there is no clock enable necessary for the access clock,
but for i.MX6UL there is!

You can simply verify it by trying to read any of the fuse registers
with the OCOTP clock disabled (which immediately hangs the processor).

> In theory perhaps tempmon could be made to read ocotp through the imx-
> ocotp nvmem driver? It is not clear in what scenario this would
> actually be required.
> 
> This discussion is not specific to 6ul/6ull. There are other reads from
> ocotp which don't enable it's clock, for example
> imx6q_opp_check_speed_grading.
> 
Which works, because i.MX6Q doesn't require to enable the OCOTP access
clock.


Lothar Waßmann

[toc] | [prev] | [next] | [standalone]

#1663506

On Mon, Jun 12, 2017 at 8:47 AM, Leonard Crestez
<leonard.crestez@nxp.com> wrote:

> However it seems that this might be accidental, it just happens that
> the OCOTP clock starts as enabled and is only disabled later in the

Most likely because U-Boot enabled the OCOTP clock as it reads the
speed grading fuse.

[toc] | [prev] | [next] | [standalone]

#1663548

Hi,

On Mon, 12 Jun 2017 10:22:45 -0300 Fabio Estevam wrote:
> On Mon, Jun 12, 2017 at 8:47 AM, Leonard Crestez
> <leonard.crestez@nxp.com> wrote:
> 
> > However it seems that this might be accidental, it just happens that
> > the OCOTP clock starts as enabled and is only disabled later in the
> 
> Most likely because U-Boot enabled the OCOTP clock as it reads the
> speed grading fuse.
>
No, the imx6q_opp_check_speed_grading() runs on an i.MX6Q which, as
mentioned in my other mail <20170612124029.1643469d@karo-electronics.de>,
does not require any clock to be enabled for accessing the OCOTP regs.


Lothar Waßmann

[toc] | [prev] | [next] | [standalone]

#1663574

On Mon, Jun 12, 2017 at 10:37 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:
> Hi,
>
> On Mon, 12 Jun 2017 10:22:45 -0300 Fabio Estevam wrote:
>> On Mon, Jun 12, 2017 at 8:47 AM, Leonard Crestez
>> <leonard.crestez@nxp.com> wrote:
>>
>> > However it seems that this might be accidental, it just happens that
>> > the OCOTP clock starts as enabled and is only disabled later in the
>>
>> Most likely because U-Boot enabled the OCOTP clock as it reads the
>> speed grading fuse.
>>
> No, the imx6q_opp_check_speed_grading() runs on an i.MX6Q which, as
> mentioned in my other mail <20170612124029.1643469d@karo-electronics.de>,
> does not require any clock to be enabled for accessing the OCOTP regs.

[Sorry I removed the list in my previous response by mistake]

What I was trying to say is that the OCOTP clock is enabled by U-Boot
on mx6ul evk board.

Please check:
http://git.denx.de/?p=u-boot.git;a=blob;f=board/freescale/mx6ul_14x14_evk/mx6ul_14x14_evk.c;h=a5746fe08688d5752bf1d69cff465a355404d6da;hb=HEAD#l833

This would explain why Leonard did not see the kernel hang on his test.

So I agree with you that the thermal driver needs the IMX6UL_CLK_OCOTP too.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web