Re: [PATCH 1/2] configs: ARM: omap2plus: Enable OMAP3_THERMAL

[Adam Ford <aford173@gmail.com>]

On Wed, Oct 23, 2019 at 9:36 AM Tony Lindgren <tony@atomide.com> wrote:
>
> * H. Nikolaus Schaller <hns@goldelico.com> [191023 04:42]:
> >
> > > Am 23.10.2019 um 00:19 schrieb Tony Lindgren <tony@atomide.com>:
> > >
> > > * Adam Ford <aford173@gmail.com> [191022 19:01]:
> > >> On Tue, Oct 22, 2019 at 11:22 AM Tony Lindgren <tony@atomide.com> wrote:
> > >>>
> > >>> Hi,
> > >>>
> > >>> * Adam Ford <aford173@gmail.com> [191007 15:06]:
> > >>>> The some in the OMAP3 family have a bandgap thermal sensor, but
> > >>>> omap2plus has it disabled.
> > >>>>
> > >>>> This patch enables the OMAP3_THERMAL by default like the rest of
> > >>>> the OMAP family.
> > >>>
> > >>> Looks like this breaks off mode during idle for omap3, and that's
> > >>> probably why it never got enabled. The difference in power
> > >>> consumption during idle is about 7mW vs 32mW for the SoC as
> > >>> measured from torpedo shunt for main_battery_som.
> > >>>
> > >>> I think the right fix might be simply to add handling for
> > >>> CPU_CLUSTER_PM_ENTER to the related thermal driver to disable
> > >>> it during idle like we have for gpio-omap.c for example.
> > >>
> > >> I am not sure I know where to start on fixing that issue.  Would you
> > >> entertain enabling the driver if we set the device tree to 'disabled'
> > >> by default?  This way if people want to to use it, it can be enabled
> > >> on a per-device option.  Once the power stuff gets resolved, we might
> > >> be able to enable it by default.  For people who are planning on using
> > >> the DM3730 @ 1GHz in high temp environments, I am not sure they'll
> > >> care about low power.
> > >
> > > They should both work fine together though. They are not mutually
> > > exclusive features.
> > >
> > >> I'll try to look into it when I have time, but I was hoping a
> > >> compromise might be a reasonable work-around.
> > >
> > > It should be hopefully a trivial fix.. I have not looked at the
> > > driver code though.
> >
> > If I am taken right, it is the drivers/thermal/ti-soc-thermal/ti-*.c
> > which is a common driver for omap3, omap4, omap5. They only differ
> > in the thermal data and which registers and bits are used to access
> > the ADC.
>
> Yes so it seems. Enabling OMAP3_THERMAL adds support to
> of_ti_bandgap_match[] for omap3 and causes the issue.
>
> > So is this problem with off mode also known for omap4 and omap5?
>
> Probably. But we don't have off mode working for omap4, and
> it cannot be used for omap5 AFAIK.
>
> My guess is we need to call clk_disable() and call
> ti_bandgap_save_ctxt() on CPU_CLUSTER_PM_ENTER similar to
> what ti_bandgap_suspend does. And then restore it on
> CPU_CLUSTER_PM_EXIT.
>
> There's a similar example already in gpio_omap_cpu_notifier().
> Not sure if there is some related errata to deal with too,
> probably the old Nokia n900 or n9 would provide some hints
> on what exactly needs to be done.

I 'think' I have a patch ready that does what you're asking, but I
will fully admit that I don't completely grasp what's going on.

I'll submit it as an RFC, but I am not even sure I understand what to
put into the description, so if you're OK with reviewing the RFC, feel
free to mark up the actual commit message as well.

From what I can see, the changes haven't negatively impact stuff. I
didn't see the power consumption go up before, so I am not sure I can
replicate your findings.

It'll be posted shortly.

adam
>
> Regards,
>
> Tony