diff mbox series

[v5,08/18] thermal: sun8i: support mod clocks

Message ID 20190810052829.6032-9-tiny.windzz@gmail.com (mailing list archive)
State Superseded
Delegated to: Zhang Rui
Headers show
Series add thermal driver for h6 | expand

Commit Message

Yangtao Li Aug. 10, 2019, 5:28 a.m. UTC
H3 has extra clock, so introduce something in ths_thermal_chip/ths_device
and adds the process of the clock.

This is pre-work for supprt it.

Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
---
 drivers/thermal/sun8i_thermal.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Vasily Khoruzhick Aug. 10, 2019, 6:16 a.m. UTC | #1
On Fri, Aug 9, 2019 at 10:31 PM Yangtao Li <tiny.windzz@gmail.com> wrote:
>
> H3 has extra clock, so introduce something in ths_thermal_chip/ths_device
> and adds the process of the clock.
>
> This is pre-work for supprt it.
>
> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> ---
>  drivers/thermal/sun8i_thermal.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index b934bc81eba7..6f4294c2aba7 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c
> @@ -54,6 +54,7 @@ struct tsensor {
>  };
>
>  struct ths_thermal_chip {
> +       bool            has_mod_clk;
>         int             sensor_num;
>         int             offset;
>         int             scale;
> @@ -69,6 +70,7 @@ struct ths_device {
>         struct regmap                           *regmap;
>         struct reset_control                    *reset;
>         struct clk                              *bus_clk;
> +       struct clk                              *mod_clk;
>         struct tsensor                          sensor[MAX_SENSOR_NUM];
>  };
>
> @@ -274,6 +276,12 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
>         if (IS_ERR(tmdev->bus_clk))
>                 return PTR_ERR(tmdev->bus_clk);
>
> +       if (tmdev->chip->has_mod_clk) {
> +               tmdev->mod_clk = devm_clk_get(&pdev->dev, "mod");
> +               if (IS_ERR(tmdev->mod_clk))
> +                       return PTR_ERR(tmdev->mod_clk);
> +       }
> +
>         ret = reset_control_deassert(tmdev->reset);
>         if (ret)
>                 return ret;
> @@ -282,12 +290,18 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
>         if (ret)
>                 goto assert_reset;
>
> -       ret = sun50i_ths_calibrate(tmdev);
> +       ret = clk_prepare_enable(tmdev->mod_clk);

You have to set rate of modclk before enabling it since you can't rely
on whatever bootloader left for you.

Also I found that parameters you're using for PC_TEMP_PERIOD, ACQ0 and
ACQ1 are too aggressive and may result in high interrupt rate to the
point when it may stall RCU. I changed driver a bit to use params from
Philipp Rossak's work (modclk set to 4MHz, PC_TEMP_PERIOD is 7, ACQ0
is 255, ACQ1 is 63) and it fixed RCU stalls for me, see [1] for
details.

[1] https://github.com/anarsoul/linux-2.6/commit/46b8bb0fe2ccd1cd88fa9181a2ecbf79e8d513b2


>         if (ret)
>                 goto bus_disable;
>
> +       ret = sun50i_ths_calibrate(tmdev);
> +       if (ret)
> +               goto mod_disable;
> +
>         return 0;
>
> +mod_disable:
> +       clk_disable_unprepare(tmdev->mod_clk);
>  bus_disable:
>         clk_disable_unprepare(tmdev->bus_clk);
>  assert_reset:
> @@ -395,6 +409,7 @@ static int sun8i_ths_remove(struct platform_device *pdev)
>  {
>         struct ths_device *tmdev = platform_get_drvdata(pdev);
>
> +       clk_disable_unprepare(tmdev->mod_clk);
>         clk_disable_unprepare(tmdev->bus_clk);
>         reset_control_assert(tmdev->reset);
>
> --
> 2.17.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Yangtao Li Aug. 12, 2019, 11:46 p.m. UTC | #2
HI Vasily,

On Sat, Aug 10, 2019 at 2:17 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> On Fri, Aug 9, 2019 at 10:31 PM Yangtao Li <tiny.windzz@gmail.com> wrote:
> >
> > H3 has extra clock, so introduce something in ths_thermal_chip/ths_device
> > and adds the process of the clock.
> >
> > This is pre-work for supprt it.
> >
> > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> > ---
> >  drivers/thermal/sun8i_thermal.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> > index b934bc81eba7..6f4294c2aba7 100644
> > --- a/drivers/thermal/sun8i_thermal.c
> > +++ b/drivers/thermal/sun8i_thermal.c
> > @@ -54,6 +54,7 @@ struct tsensor {
> >  };
> >
> >  struct ths_thermal_chip {
> > +       bool            has_mod_clk;
> >         int             sensor_num;
> >         int             offset;
> >         int             scale;
> > @@ -69,6 +70,7 @@ struct ths_device {
> >         struct regmap                           *regmap;
> >         struct reset_control                    *reset;
> >         struct clk                              *bus_clk;
> > +       struct clk                              *mod_clk;
> >         struct tsensor                          sensor[MAX_SENSOR_NUM];
> >  };
> >
> > @@ -274,6 +276,12 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> >         if (IS_ERR(tmdev->bus_clk))
> >                 return PTR_ERR(tmdev->bus_clk);
> >
> > +       if (tmdev->chip->has_mod_clk) {
> > +               tmdev->mod_clk = devm_clk_get(&pdev->dev, "mod");
> > +               if (IS_ERR(tmdev->mod_clk))
> > +                       return PTR_ERR(tmdev->mod_clk);
> > +       }
> > +
> >         ret = reset_control_deassert(tmdev->reset);
> >         if (ret)
> >                 return ret;
> > @@ -282,12 +290,18 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> >         if (ret)
> >                 goto assert_reset;
> >
> > -       ret = sun50i_ths_calibrate(tmdev);
> > +       ret = clk_prepare_enable(tmdev->mod_clk);
>
> You have to set rate of modclk before enabling it since you can't rely
> on whatever bootloader left for you.
>
> Also I found that parameters you're using for PC_TEMP_PERIOD, ACQ0 and
> ACQ1 are too aggressive and may result in high interrupt rate to the
> point when it may stall RCU. I changed driver a bit to use params from
> Philipp Rossak's work (modclk set to 4MHz, PC_TEMP_PERIOD is 7, ACQ0
> is 255, ACQ1 is 63) and it fixed RCU stalls for me, see [1] for
> details.

Why is the RCU stall happening, is it caused by a deadlock?
Can you provide log information and your configuration?
I am a bit curious.

Thx,
Yangtao

>
> [1] https://github.com/anarsoul/linux-2.6/commit/46b8bb0fe2ccd1cd88fa9181a2ecbf79e8d513b2
>
>
> >         if (ret)
> >                 goto bus_disable;
> >
> > +       ret = sun50i_ths_calibrate(tmdev);
> > +       if (ret)
> > +               goto mod_disable;
> > +
> >         return 0;
> >
> > +mod_disable:
> > +       clk_disable_unprepare(tmdev->mod_clk);
> >  bus_disable:
> >         clk_disable_unprepare(tmdev->bus_clk);
> >  assert_reset:
> > @@ -395,6 +409,7 @@ static int sun8i_ths_remove(struct platform_device *pdev)
> >  {
> >         struct ths_device *tmdev = platform_get_drvdata(pdev);
> >
> > +       clk_disable_unprepare(tmdev->mod_clk);
> >         clk_disable_unprepare(tmdev->bus_clk);
> >         reset_control_assert(tmdev->reset);
> >
> > --
> > 2.17.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Vasily Khoruzhick Aug. 12, 2019, 11:54 p.m. UTC | #3
On Mon, Aug 12, 2019 at 4:46 PM Frank Lee <tiny.windzz@gmail.com> wrote:
>
> HI Vasily,
>
> On Sat, Aug 10, 2019 at 2:17 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> >
> > On Fri, Aug 9, 2019 at 10:31 PM Yangtao Li <tiny.windzz@gmail.com> wrote:
> > >
> > > H3 has extra clock, so introduce something in ths_thermal_chip/ths_device
> > > and adds the process of the clock.
> > >
> > > This is pre-work for supprt it.
> > >
> > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> > > ---
> > >  drivers/thermal/sun8i_thermal.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> > > index b934bc81eba7..6f4294c2aba7 100644
> > > --- a/drivers/thermal/sun8i_thermal.c
> > > +++ b/drivers/thermal/sun8i_thermal.c
> > > @@ -54,6 +54,7 @@ struct tsensor {
> > >  };
> > >
> > >  struct ths_thermal_chip {
> > > +       bool            has_mod_clk;
> > >         int             sensor_num;
> > >         int             offset;
> > >         int             scale;
> > > @@ -69,6 +70,7 @@ struct ths_device {
> > >         struct regmap                           *regmap;
> > >         struct reset_control                    *reset;
> > >         struct clk                              *bus_clk;
> > > +       struct clk                              *mod_clk;
> > >         struct tsensor                          sensor[MAX_SENSOR_NUM];
> > >  };
> > >
> > > @@ -274,6 +276,12 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> > >         if (IS_ERR(tmdev->bus_clk))
> > >                 return PTR_ERR(tmdev->bus_clk);
> > >
> > > +       if (tmdev->chip->has_mod_clk) {
> > > +               tmdev->mod_clk = devm_clk_get(&pdev->dev, "mod");
> > > +               if (IS_ERR(tmdev->mod_clk))
> > > +                       return PTR_ERR(tmdev->mod_clk);
> > > +       }
> > > +
> > >         ret = reset_control_deassert(tmdev->reset);
> > >         if (ret)
> > >                 return ret;
> > > @@ -282,12 +290,18 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> > >         if (ret)
> > >                 goto assert_reset;
> > >
> > > -       ret = sun50i_ths_calibrate(tmdev);
> > > +       ret = clk_prepare_enable(tmdev->mod_clk);
> >
> > You have to set rate of modclk before enabling it since you can't rely
> > on whatever bootloader left for you.
> >
> > Also I found that parameters you're using for PC_TEMP_PERIOD, ACQ0 and
> > ACQ1 are too aggressive and may result in high interrupt rate to the
> > point when it may stall RCU. I changed driver a bit to use params from
> > Philipp Rossak's work (modclk set to 4MHz, PC_TEMP_PERIOD is 7, ACQ0
> > is 255, ACQ1 is 63) and it fixed RCU stalls for me, see [1] for
> > details.
>
> Why is the RCU stall happening, is it caused by a deadlock?
> Can you provide log information and your configuration?
> I am a bit curious.

It's not deadlock, I believe it just can't handle that many interrupts
when running at lowest CPU frequency. Even with Philipp's settings
there's ~20 interrupts a second from ths. I don't remember how many
interrupts were there with your settings.

Unfortunately there's nothing interesting in backtraces, I'm using
Pine64-LTS board.

> Thx,
> Yangtao
>
> >
> > [1] https://github.com/anarsoul/linux-2.6/commit/46b8bb0fe2ccd1cd88fa9181a2ecbf79e8d513b2
> >
> >
> > >         if (ret)
> > >                 goto bus_disable;
> > >
> > > +       ret = sun50i_ths_calibrate(tmdev);
> > > +       if (ret)
> > > +               goto mod_disable;
> > > +
> > >         return 0;
> > >
> > > +mod_disable:
> > > +       clk_disable_unprepare(tmdev->mod_clk);
> > >  bus_disable:
> > >         clk_disable_unprepare(tmdev->bus_clk);
> > >  assert_reset:
> > > @@ -395,6 +409,7 @@ static int sun8i_ths_remove(struct platform_device *pdev)
> > >  {
> > >         struct ths_device *tmdev = platform_get_drvdata(pdev);
> > >
> > > +       clk_disable_unprepare(tmdev->mod_clk);
> > >         clk_disable_unprepare(tmdev->bus_clk);
> > >         reset_control_assert(tmdev->reset);
> > >
> > > --
> > > 2.17.1
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Ondřej Jirman Aug. 13, 2019, 8:06 p.m. UTC | #4
On Mon, Aug 12, 2019 at 04:54:15PM -0700, Vasily Khoruzhick wrote:
> On Mon, Aug 12, 2019 at 4:46 PM Frank Lee <tiny.windzz@gmail.com> wrote:
> >
> > HI Vasily,
> >
> > On Sat, Aug 10, 2019 at 2:17 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> > >
> > > On Fri, Aug 9, 2019 at 10:31 PM Yangtao Li <tiny.windzz@gmail.com> wrote:
> > > >
> > > > H3 has extra clock, so introduce something in ths_thermal_chip/ths_device
> > > > and adds the process of the clock.
> > > >
> > > > This is pre-work for supprt it.
> > > >
> > > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> > > > ---
> > > >  drivers/thermal/sun8i_thermal.c | 17 ++++++++++++++++-
> > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> > > > index b934bc81eba7..6f4294c2aba7 100644
> > > > --- a/drivers/thermal/sun8i_thermal.c
> > > > +++ b/drivers/thermal/sun8i_thermal.c
> > > > @@ -54,6 +54,7 @@ struct tsensor {
> > > >  };
> > > >
> > > >  struct ths_thermal_chip {
> > > > +       bool            has_mod_clk;
> > > >         int             sensor_num;
> > > >         int             offset;
> > > >         int             scale;
> > > > @@ -69,6 +70,7 @@ struct ths_device {
> > > >         struct regmap                           *regmap;
> > > >         struct reset_control                    *reset;
> > > >         struct clk                              *bus_clk;
> > > > +       struct clk                              *mod_clk;
> > > >         struct tsensor                          sensor[MAX_SENSOR_NUM];
> > > >  };
> > > >
> > > > @@ -274,6 +276,12 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> > > >         if (IS_ERR(tmdev->bus_clk))
> > > >                 return PTR_ERR(tmdev->bus_clk);
> > > >
> > > > +       if (tmdev->chip->has_mod_clk) {
> > > > +               tmdev->mod_clk = devm_clk_get(&pdev->dev, "mod");
> > > > +               if (IS_ERR(tmdev->mod_clk))
> > > > +                       return PTR_ERR(tmdev->mod_clk);
> > > > +       }
> > > > +
> > > >         ret = reset_control_deassert(tmdev->reset);
> > > >         if (ret)
> > > >                 return ret;
> > > > @@ -282,12 +290,18 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> > > >         if (ret)
> > > >                 goto assert_reset;
> > > >
> > > > -       ret = sun50i_ths_calibrate(tmdev);
> > > > +       ret = clk_prepare_enable(tmdev->mod_clk);
> > >
> > > You have to set rate of modclk before enabling it since you can't rely
> > > on whatever bootloader left for you.
> > >
> > > Also I found that parameters you're using for PC_TEMP_PERIOD, ACQ0 and
> > > ACQ1 are too aggressive and may result in high interrupt rate to the
> > > point when it may stall RCU. I changed driver a bit to use params from
> > > Philipp Rossak's work (modclk set to 4MHz, PC_TEMP_PERIOD is 7, ACQ0
> > > is 255, ACQ1 is 63) and it fixed RCU stalls for me, see [1] for
> > > details.
> >
> > Why is the RCU stall happening, is it caused by a deadlock?
> > Can you provide log information and your configuration?
> > I am a bit curious.
> 
> It's not deadlock, I believe it just can't handle that many interrupts
> when running at lowest CPU frequency. Even with Philipp's settings
> there's ~20 interrupts a second from ths. I don't remember how many
> interrupts were there with your settings.
> 
> Unfortunately there's nothing interesting in backtraces, I'm using
> Pine64-LTS board.

Recently there was a similar issue, with buggy CCU driver that caused
CIR interrupts being fired constantly, and it also resulted in RCU
stalls. Looks like a comon cause of RCU stalls.

THS timing settings probably need to be made specific to the SoC, because
I noticed that the same settings lead to wildly different timings on
different SoCs.

It would be good to measure how often ths interrupt fires with this driver
on various SoCs.

20 times a second and more sounds like overkill. I'd expect a useful
range to be at most 5-10 times a second. That should be enough to stop
overheating the SoC due to suddenly increased load, even without a
heatsink.

regards,
	o.

> > Thx,
> > Yangtao
> >
> > >
> > > [1] https://github.com/anarsoul/linux-2.6/commit/46b8bb0fe2ccd1cd88fa9181a2ecbf79e8d513b2
> > >
> > >
> > > >         if (ret)
> > > >                 goto bus_disable;
> > > >
> > > > +       ret = sun50i_ths_calibrate(tmdev);
> > > > +       if (ret)
> > > > +               goto mod_disable;
> > > > +
> > > >         return 0;
> > > >
> > > > +mod_disable:
> > > > +       clk_disable_unprepare(tmdev->mod_clk);
> > > >  bus_disable:
> > > >         clk_disable_unprepare(tmdev->bus_clk);
> > > >  assert_reset:
> > > > @@ -395,6 +409,7 @@ static int sun8i_ths_remove(struct platform_device *pdev)
> > > >  {
> > > >         struct ths_device *tmdev = platform_get_drvdata(pdev);
> > > >
> > > > +       clk_disable_unprepare(tmdev->mod_clk);
> > > >         clk_disable_unprepare(tmdev->bus_clk);
> > > >         reset_control_assert(tmdev->reset);
> > > >
> > > > --
> > > > 2.17.1
> > > >
> > > >
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Vasily Khoruzhick Aug. 14, 2019, 3:01 a.m. UTC | #5
On Tue, Aug 13, 2019 at 1:06 PM Ondřej Jirman <megous@megous.com> wrote:
>
> On Mon, Aug 12, 2019 at 04:54:15PM -0700, Vasily Khoruzhick wrote:
> > On Mon, Aug 12, 2019 at 4:46 PM Frank Lee <tiny.windzz@gmail.com> wrote:
> > >
> > > HI Vasily,
> > >
> > > On Sat, Aug 10, 2019 at 2:17 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> > > >
> > > > On Fri, Aug 9, 2019 at 10:31 PM Yangtao Li <tiny.windzz@gmail.com> wrote:
> > > > >
> > > > > H3 has extra clock, so introduce something in ths_thermal_chip/ths_device
> > > > > and adds the process of the clock.
> > > > >
> > > > > This is pre-work for supprt it.
> > > > >
> > > > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> > > > > ---
> > > > >  drivers/thermal/sun8i_thermal.c | 17 ++++++++++++++++-
> > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> > > > > index b934bc81eba7..6f4294c2aba7 100644
> > > > > --- a/drivers/thermal/sun8i_thermal.c
> > > > > +++ b/drivers/thermal/sun8i_thermal.c
> > > > > @@ -54,6 +54,7 @@ struct tsensor {
> > > > >  };
> > > > >
> > > > >  struct ths_thermal_chip {
> > > > > +       bool            has_mod_clk;
> > > > >         int             sensor_num;
> > > > >         int             offset;
> > > > >         int             scale;
> > > > > @@ -69,6 +70,7 @@ struct ths_device {
> > > > >         struct regmap                           *regmap;
> > > > >         struct reset_control                    *reset;
> > > > >         struct clk                              *bus_clk;
> > > > > +       struct clk                              *mod_clk;
> > > > >         struct tsensor                          sensor[MAX_SENSOR_NUM];
> > > > >  };
> > > > >
> > > > > @@ -274,6 +276,12 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> > > > >         if (IS_ERR(tmdev->bus_clk))
> > > > >                 return PTR_ERR(tmdev->bus_clk);
> > > > >
> > > > > +       if (tmdev->chip->has_mod_clk) {
> > > > > +               tmdev->mod_clk = devm_clk_get(&pdev->dev, "mod");
> > > > > +               if (IS_ERR(tmdev->mod_clk))
> > > > > +                       return PTR_ERR(tmdev->mod_clk);
> > > > > +       }
> > > > > +
> > > > >         ret = reset_control_deassert(tmdev->reset);
> > > > >         if (ret)
> > > > >                 return ret;
> > > > > @@ -282,12 +290,18 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> > > > >         if (ret)
> > > > >                 goto assert_reset;
> > > > >
> > > > > -       ret = sun50i_ths_calibrate(tmdev);
> > > > > +       ret = clk_prepare_enable(tmdev->mod_clk);
> > > >
> > > > You have to set rate of modclk before enabling it since you can't rely
> > > > on whatever bootloader left for you.
> > > >
> > > > Also I found that parameters you're using for PC_TEMP_PERIOD, ACQ0 and
> > > > ACQ1 are too aggressive and may result in high interrupt rate to the
> > > > point when it may stall RCU. I changed driver a bit to use params from
> > > > Philipp Rossak's work (modclk set to 4MHz, PC_TEMP_PERIOD is 7, ACQ0
> > > > is 255, ACQ1 is 63) and it fixed RCU stalls for me, see [1] for
> > > > details.
> > >
> > > Why is the RCU stall happening, is it caused by a deadlock?
> > > Can you provide log information and your configuration?
> > > I am a bit curious.
> >
> > It's not deadlock, I believe it just can't handle that many interrupts
> > when running at lowest CPU frequency. Even with Philipp's settings
> > there's ~20 interrupts a second from ths. I don't remember how many
> > interrupts were there with your settings.
> >
> > Unfortunately there's nothing interesting in backtraces, I'm using
> > Pine64-LTS board.
>
> Recently there was a similar issue, with buggy CCU driver that caused
> CIR interrupts being fired constantly, and it also resulted in RCU
> stalls. Looks like a comon cause of RCU stalls.
>
> THS timing settings probably need to be made specific to the SoC, because
> I noticed that the same settings lead to wildly different timings on
> different SoCs.
>
> It would be good to measure how often ths interrupt fires with this driver
> on various SoCs.
>
> 20 times a second and more sounds like overkill. I'd expect a useful
> range to be at most 5-10 times a second. That should be enough to stop
> overheating the SoC due to suddenly increased load, even without a
> heatsink.

Note that A64 has 3 sensors and each sensor has individual interrupt,
so technically it's 6-7 interrupts per sensor per second

> regards,
>         o.
>
> > > Thx,
> > > Yangtao
> > >
> > > >
> > > > [1] https://github.com/anarsoul/linux-2.6/commit/46b8bb0fe2ccd1cd88fa9181a2ecbf79e8d513b2
> > > >
> > > >
> > > > >         if (ret)
> > > > >                 goto bus_disable;
> > > > >
> > > > > +       ret = sun50i_ths_calibrate(tmdev);
> > > > > +       if (ret)
> > > > > +               goto mod_disable;
> > > > > +
> > > > >         return 0;
> > > > >
> > > > > +mod_disable:
> > > > > +       clk_disable_unprepare(tmdev->mod_clk);
> > > > >  bus_disable:
> > > > >         clk_disable_unprepare(tmdev->bus_clk);
> > > > >  assert_reset:
> > > > > @@ -395,6 +409,7 @@ static int sun8i_ths_remove(struct platform_device *pdev)
> > > > >  {
> > > > >         struct ths_device *tmdev = platform_get_drvdata(pdev);
> > > > >
> > > > > +       clk_disable_unprepare(tmdev->mod_clk);
> > > > >         clk_disable_unprepare(tmdev->bus_clk);
> > > > >         reset_control_assert(tmdev->reset);
> > > > >
> > > > > --
> > > > > 2.17.1
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > linux-arm-kernel mailing list
> > > > > linux-arm-kernel@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Yangtao Li Aug. 25, 2019, 4:14 p.m. UTC | #6
HI Vasily,

On Wed, Aug 14, 2019 at 11:01 AM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> On Tue, Aug 13, 2019 at 1:06 PM Ondřej Jirman <megous@megous.com> wrote:
> >
> > On Mon, Aug 12, 2019 at 04:54:15PM -0700, Vasily Khoruzhick wrote:
> > > On Mon, Aug 12, 2019 at 4:46 PM Frank Lee <tiny.windzz@gmail.com> wrote:
> > > >
> > > > HI Vasily,
> > > >
> > > > On Sat, Aug 10, 2019 at 2:17 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> > > > >
> > > > > On Fri, Aug 9, 2019 at 10:31 PM Yangtao Li <tiny.windzz@gmail.com> wrote:
> > > > > >
> > > > > > H3 has extra clock, so introduce something in ths_thermal_chip/ths_device
> > > > > > and adds the process of the clock.
> > > > > >
> > > > > > This is pre-work for supprt it.
> > > > > >
> > > > > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> > > > > > ---
> > > > > >  drivers/thermal/sun8i_thermal.c | 17 ++++++++++++++++-
> > > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> > > > > > index b934bc81eba7..6f4294c2aba7 100644
> > > > > > --- a/drivers/thermal/sun8i_thermal.c
> > > > > > +++ b/drivers/thermal/sun8i_thermal.c
> > > > > > @@ -54,6 +54,7 @@ struct tsensor {
> > > > > >  };
> > > > > >
> > > > > >  struct ths_thermal_chip {
> > > > > > +       bool            has_mod_clk;
> > > > > >         int             sensor_num;
> > > > > >         int             offset;
> > > > > >         int             scale;
> > > > > > @@ -69,6 +70,7 @@ struct ths_device {
> > > > > >         struct regmap                           *regmap;
> > > > > >         struct reset_control                    *reset;
> > > > > >         struct clk                              *bus_clk;
> > > > > > +       struct clk                              *mod_clk;
> > > > > >         struct tsensor                          sensor[MAX_SENSOR_NUM];
> > > > > >  };
> > > > > >
> > > > > > @@ -274,6 +276,12 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> > > > > >         if (IS_ERR(tmdev->bus_clk))
> > > > > >                 return PTR_ERR(tmdev->bus_clk);
> > > > > >
> > > > > > +       if (tmdev->chip->has_mod_clk) {
> > > > > > +               tmdev->mod_clk = devm_clk_get(&pdev->dev, "mod");
> > > > > > +               if (IS_ERR(tmdev->mod_clk))
> > > > > > +                       return PTR_ERR(tmdev->mod_clk);
> > > > > > +       }
> > > > > > +
> > > > > >         ret = reset_control_deassert(tmdev->reset);
> > > > > >         if (ret)
> > > > > >                 return ret;
> > > > > > @@ -282,12 +290,18 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> > > > > >         if (ret)
> > > > > >                 goto assert_reset;
> > > > > >
> > > > > > -       ret = sun50i_ths_calibrate(tmdev);
> > > > > > +       ret = clk_prepare_enable(tmdev->mod_clk);
> > > > >
> > > > > You have to set rate of modclk before enabling it since you can't rely
> > > > > on whatever bootloader left for you.
> > > > >
> > > > > Also I found that parameters you're using for PC_TEMP_PERIOD, ACQ0 and
> > > > > ACQ1 are too aggressive and may result in high interrupt rate to the
> > > > > point when it may stall RCU. I changed driver a bit to use params from
> > > > > Philipp Rossak's work (modclk set to 4MHz, PC_TEMP_PERIOD is 7, ACQ0
> > > > > is 255, ACQ1 is 63) and it fixed RCU stalls for me, see [1] for
> > > > > details.
> > > >
> > > > Why is the RCU stall happening, is it caused by a deadlock?
> > > > Can you provide log information and your configuration?
> > > > I am a bit curious.
> > >
> > > It's not deadlock, I believe it just can't handle that many interrupts
> > > when running at lowest CPU frequency. Even with Philipp's settings
> > > there's ~20 interrupts a second from ths. I don't remember how many
> > > interrupts were there with your settings.
> > >
> > > Unfortunately there's nothing interesting in backtraces, I'm using
> > > Pine64-LTS board.
> >
> > Recently there was a similar issue, with buggy CCU driver that caused
> > CIR interrupts being fired constantly, and it also resulted in RCU
> > stalls. Looks like a comon cause of RCU stalls.
> >
> > THS timing settings probably need to be made specific to the SoC, because
> > I noticed that the same settings lead to wildly different timings on
> > different SoCs.
> >
> > It would be good to measure how often ths interrupt fires with this driver
> > on various SoCs.
> >
> > 20 times a second and more sounds like overkill. I'd expect a useful
> > range to be at most 5-10 times a second. That should be enough to stop
> > overheating the SoC due to suddenly increased load, even without a
> > heatsink.
>
> Note that A64 has 3 sensors and each sensor has individual interrupt,
> so technically it's 6-7 interrupts per sensor per second

You only need to increase the value of the period to reduce the number
of interrupts.
Can you test the relationship between the period and the number of interrupts
when the mod clock does not change and stays 24M?

Thx.
Yangtao

>
> > regards,
> >         o.
> >
> > > > Thx,
> > > > Yangtao
> > > >
> > > > >
> > > > > [1] https://github.com/anarsoul/linux-2.6/commit/46b8bb0fe2ccd1cd88fa9181a2ecbf79e8d513b2
> > > > >
> > > > >
> > > > > >         if (ret)
> > > > > >                 goto bus_disable;
> > > > > >
> > > > > > +       ret = sun50i_ths_calibrate(tmdev);
> > > > > > +       if (ret)
> > > > > > +               goto mod_disable;
> > > > > > +
> > > > > >         return 0;
> > > > > >
> > > > > > +mod_disable:
> > > > > > +       clk_disable_unprepare(tmdev->mod_clk);
> > > > > >  bus_disable:
> > > > > >         clk_disable_unprepare(tmdev->bus_clk);
> > > > > >  assert_reset:
> > > > > > @@ -395,6 +409,7 @@ static int sun8i_ths_remove(struct platform_device *pdev)
> > > > > >  {
> > > > > >         struct ths_device *tmdev = platform_get_drvdata(pdev);
> > > > > >
> > > > > > +       clk_disable_unprepare(tmdev->mod_clk);
> > > > > >         clk_disable_unprepare(tmdev->bus_clk);
> > > > > >         reset_control_assert(tmdev->reset);
> > > > > >
> > > > > > --
> > > > > > 2.17.1
> > > > > >
> > > > > >
> > > > > > _______________________________________________
> > > > > > linux-arm-kernel mailing list
> > > > > > linux-arm-kernel@lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Vasily Khoruzhick Oct. 21, 2019, 3:41 a.m. UTC | #7
On Sun, Aug 25, 2019 at 9:14 AM Frank Lee <tiny.windzz@gmail.com> wrote:
>
> HI Vasily,

Hi Yangtao,

Sorry for the late reply,

> On Wed, Aug 14, 2019 at 11:01 AM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> >
> > On Tue, Aug 13, 2019 at 1:06 PM Ondřej Jirman <megous@megous.com> wrote:
> > >
> > > On Mon, Aug 12, 2019 at 04:54:15PM -0700, Vasily Khoruzhick wrote:
> > > > On Mon, Aug 12, 2019 at 4:46 PM Frank Lee <tiny.windzz@gmail.com> wrote:
> > > > >
> > > > > HI Vasily,
> > > > >
> > > > > On Sat, Aug 10, 2019 at 2:17 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> > > > > >
> > > > > > On Fri, Aug 9, 2019 at 10:31 PM Yangtao Li <tiny.windzz@gmail.com> wrote:
> > > > > > >
> > > > > > > H3 has extra clock, so introduce something in ths_thermal_chip/ths_device
> > > > > > > and adds the process of the clock.
> > > > > > >
> > > > > > > This is pre-work for supprt it.
> > > > > > >
> > > > > > > Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> > > > > > > ---
> > > > > > >  drivers/thermal/sun8i_thermal.c | 17 ++++++++++++++++-
> > > > > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> > > > > > > index b934bc81eba7..6f4294c2aba7 100644
> > > > > > > --- a/drivers/thermal/sun8i_thermal.c
> > > > > > > +++ b/drivers/thermal/sun8i_thermal.c
> > > > > > > @@ -54,6 +54,7 @@ struct tsensor {
> > > > > > >  };
> > > > > > >
> > > > > > >  struct ths_thermal_chip {
> > > > > > > +       bool            has_mod_clk;
> > > > > > >         int             sensor_num;
> > > > > > >         int             offset;
> > > > > > >         int             scale;
> > > > > > > @@ -69,6 +70,7 @@ struct ths_device {
> > > > > > >         struct regmap                           *regmap;
> > > > > > >         struct reset_control                    *reset;
> > > > > > >         struct clk                              *bus_clk;
> > > > > > > +       struct clk                              *mod_clk;
> > > > > > >         struct tsensor                          sensor[MAX_SENSOR_NUM];
> > > > > > >  };
> > > > > > >
> > > > > > > @@ -274,6 +276,12 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> > > > > > >         if (IS_ERR(tmdev->bus_clk))
> > > > > > >                 return PTR_ERR(tmdev->bus_clk);
> > > > > > >
> > > > > > > +       if (tmdev->chip->has_mod_clk) {
> > > > > > > +               tmdev->mod_clk = devm_clk_get(&pdev->dev, "mod");
> > > > > > > +               if (IS_ERR(tmdev->mod_clk))
> > > > > > > +                       return PTR_ERR(tmdev->mod_clk);
> > > > > > > +       }
> > > > > > > +
> > > > > > >         ret = reset_control_deassert(tmdev->reset);
> > > > > > >         if (ret)
> > > > > > >                 return ret;
> > > > > > > @@ -282,12 +290,18 @@ static int sun8i_ths_resource_init(struct ths_device *tmdev)
> > > > > > >         if (ret)
> > > > > > >                 goto assert_reset;
> > > > > > >
> > > > > > > -       ret = sun50i_ths_calibrate(tmdev);
> > > > > > > +       ret = clk_prepare_enable(tmdev->mod_clk);
> > > > > >
> > > > > > You have to set rate of modclk before enabling it since you can't rely
> > > > > > on whatever bootloader left for you.
> > > > > >
> > > > > > Also I found that parameters you're using for PC_TEMP_PERIOD, ACQ0 and
> > > > > > ACQ1 are too aggressive and may result in high interrupt rate to the
> > > > > > point when it may stall RCU. I changed driver a bit to use params from
> > > > > > Philipp Rossak's work (modclk set to 4MHz, PC_TEMP_PERIOD is 7, ACQ0
> > > > > > is 255, ACQ1 is 63) and it fixed RCU stalls for me, see [1] for
> > > > > > details.
> > > > >
> > > > > Why is the RCU stall happening, is it caused by a deadlock?
> > > > > Can you provide log information and your configuration?
> > > > > I am a bit curious.
> > > >
> > > > It's not deadlock, I believe it just can't handle that many interrupts
> > > > when running at lowest CPU frequency. Even with Philipp's settings
> > > > there's ~20 interrupts a second from ths. I don't remember how many
> > > > interrupts were there with your settings.
> > > >
> > > > Unfortunately there's nothing interesting in backtraces, I'm using
> > > > Pine64-LTS board.
> > >
> > > Recently there was a similar issue, with buggy CCU driver that caused
> > > CIR interrupts being fired constantly, and it also resulted in RCU
> > > stalls. Looks like a comon cause of RCU stalls.
> > >
> > > THS timing settings probably need to be made specific to the SoC, because
> > > I noticed that the same settings lead to wildly different timings on
> > > different SoCs.
> > >
> > > It would be good to measure how often ths interrupt fires with this driver
> > > on various SoCs.
> > >
> > > 20 times a second and more sounds like overkill. I'd expect a useful
> > > range to be at most 5-10 times a second. That should be enough to stop
> > > overheating the SoC due to suddenly increased load, even without a
> > > heatsink.
> >
> > Note that A64 has 3 sensors and each sensor has individual interrupt,
> > so technically it's 6-7 interrupts per sensor per second
>
> You only need to increase the value of the period to reduce the number
> of interrupts.
> Can you test the relationship between the period and the number of interrupts
> when the mod clock does not change and stays 24M?

I played a bit with your settings and 24M,

with PERIOD = 57 I get 26 interrupts / second
with 87 - 18 interrupts / second
with 116 - 12-15 interrupts / second.

I think we should use 116 for A64 since with it we get reasonable
number of ths interrupts in a second.

Regards,
Vasily

> Thx.
> Yangtao
>
> >
> > > regards,
> > >         o.
> > >
> > > > > Thx,
> > > > > Yangtao
> > > > >
> > > > > >
> > > > > > [1] https://github.com/anarsoul/linux-2.6/commit/46b8bb0fe2ccd1cd88fa9181a2ecbf79e8d513b2
> > > > > >
> > > > > >
> > > > > > >         if (ret)
> > > > > > >                 goto bus_disable;
> > > > > > >
> > > > > > > +       ret = sun50i_ths_calibrate(tmdev);
> > > > > > > +       if (ret)
> > > > > > > +               goto mod_disable;
> > > > > > > +
> > > > > > >         return 0;
> > > > > > >
> > > > > > > +mod_disable:
> > > > > > > +       clk_disable_unprepare(tmdev->mod_clk);
> > > > > > >  bus_disable:
> > > > > > >         clk_disable_unprepare(tmdev->bus_clk);
> > > > > > >  assert_reset:
> > > > > > > @@ -395,6 +409,7 @@ static int sun8i_ths_remove(struct platform_device *pdev)
> > > > > > >  {
> > > > > > >         struct ths_device *tmdev = platform_get_drvdata(pdev);
> > > > > > >
> > > > > > > +       clk_disable_unprepare(tmdev->mod_clk);
> > > > > > >         clk_disable_unprepare(tmdev->bus_clk);
> > > > > > >         reset_control_assert(tmdev->reset);
> > > > > > >
> > > > > > > --
> > > > > > > 2.17.1
> > > > > > >
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > linux-arm-kernel mailing list
> > > > > > > linux-arm-kernel@lists.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > >
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox series

Patch

diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
index b934bc81eba7..6f4294c2aba7 100644
--- a/drivers/thermal/sun8i_thermal.c
+++ b/drivers/thermal/sun8i_thermal.c
@@ -54,6 +54,7 @@  struct tsensor {
 };
 
 struct ths_thermal_chip {
+	bool            has_mod_clk;
 	int		sensor_num;
 	int		offset;
 	int		scale;
@@ -69,6 +70,7 @@  struct ths_device {
 	struct regmap				*regmap;
 	struct reset_control			*reset;
 	struct clk				*bus_clk;
+	struct clk                              *mod_clk;
 	struct tsensor				sensor[MAX_SENSOR_NUM];
 };
 
@@ -274,6 +276,12 @@  static int sun8i_ths_resource_init(struct ths_device *tmdev)
 	if (IS_ERR(tmdev->bus_clk))
 		return PTR_ERR(tmdev->bus_clk);
 
+	if (tmdev->chip->has_mod_clk) {
+		tmdev->mod_clk = devm_clk_get(&pdev->dev, "mod");
+		if (IS_ERR(tmdev->mod_clk))
+			return PTR_ERR(tmdev->mod_clk);
+	}
+
 	ret = reset_control_deassert(tmdev->reset);
 	if (ret)
 		return ret;
@@ -282,12 +290,18 @@  static int sun8i_ths_resource_init(struct ths_device *tmdev)
 	if (ret)
 		goto assert_reset;
 
-	ret = sun50i_ths_calibrate(tmdev);
+	ret = clk_prepare_enable(tmdev->mod_clk);
 	if (ret)
 		goto bus_disable;
 
+	ret = sun50i_ths_calibrate(tmdev);
+	if (ret)
+		goto mod_disable;
+
 	return 0;
 
+mod_disable:
+	clk_disable_unprepare(tmdev->mod_clk);
 bus_disable:
 	clk_disable_unprepare(tmdev->bus_clk);
 assert_reset:
@@ -395,6 +409,7 @@  static int sun8i_ths_remove(struct platform_device *pdev)
 {
 	struct ths_device *tmdev = platform_get_drvdata(pdev);
 
+	clk_disable_unprepare(tmdev->mod_clk);
 	clk_disable_unprepare(tmdev->bus_clk);
 	reset_control_assert(tmdev->reset);