[PATCH/RFT,v2,0/3] thermal: add support for r8a77995

This series adds thermal support for r8a77995.
R-Car D3 (r8a77995) have a thermal sensor module which is similar to Gen2.
Therefore this series adds r8a77995 support to rcar_thermal driver not
rcar_gen3_thermal driver.

This series is based on the next branch of Zhang Rui's linux tree.

v2 [Yoshihiro Kaneko]
* As suggested by Geert Uytterhoeven
rcar_thermal.c:
- remove rcar_of_data macro
- store a pointer to rcar_thermal_chip in rcar_thermal_priv
- remove unnecessary cast in rcar_thermal_dt_ids

rcar-thermal.txt:
- drop the fallback for D3
- update the paragraph about interrupts

r8a77995.dtsi:
- fix the base address and the register addresses
- drop the fallback


Yoshihiro Kaneko (3):
  thermal: rcar_thermal: add r8a77995 support
  dt-bindings: thermal: rcar-thermal: add R8A77995 support
  arm64: dts: renesas: r8a77995: add thermal device support

 .../devicetree/bindings/thermal/rcar-thermal.txt   |   7 +-
 arch/arm64/boot/dts/renesas/r8a77995.dtsi          |  30 +++++
 drivers/thermal/rcar_thermal.c                     | 148 ++++++++++++++++-----
 3 files changed, 151 insertions(+), 34 deletions(-)

On Fri, Mar 30, 2018 at 5:13 AM, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

Hi Kaneko-san,

On Fri, Mar 30, 2018 at 5:13 AM, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> Add support for R-Car D3 (r8a77995) thermal sensor.
>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>

Thanks for your patch!

> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -58,10 +58,35 @@ struct rcar_thermal_common {
>         spinlock_t lock;
>  };
>
> +enum rcar_thermal_type {
> +       RCAR_THERMAL,
> +       RCAR_GEN2_THERMAL,
> +       RCAR_GEN3_THERMAL,
> +};
> +
> +struct rcar_thermal_chip {
> +       int use_of_thermal;

This can be a single bit:

    unsigned int use_of_thermal : 1;

> +       enum rcar_thermal_type type;

If you would add feature bits, you can get rid of rcar_thermal_type:

    unsigned int has_filonoff : 1;
    unsigned int has_enr : 1;
    unsigned int needs_suspend_resume : 1;

The number of interrupts can be stored here, too.

> +};
> +
> +static const struct rcar_thermal_chip rcar_thermal = {
> +       .use_of_thermal = 0,
> +       .type = RCAR_THERMAL,

.has_filonoff = 1,
.has_enr = 0,
...
.nirqs = 1,

> @@ -190,7 +222,8 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
>          * enable IRQ
>          */
>         if (rcar_has_irq_support(priv)) {
> -               rcar_thermal_write(priv, FILONOFF, 0);
> +               if (priv->chip->type != RCAR_GEN3_THERMAL)

if (priv->chip->has_filonoff)

> @@ -438,6 +471,9 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>         struct rcar_thermal_priv *priv;
>         struct device *dev = &pdev->dev;
>         struct resource *res, *irq;
> +       struct rcar_thermal_chip *chip = ((struct rcar_thermal_chip *)

I don't think the cast is needed.

> @@ -457,19 +493,35 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>         pm_runtime_enable(dev);
>         pm_runtime_get_sync(dev);
>
> -       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> -       if (irq) {
> -               /*
> -                * platform has IRQ support.
> -                * Then, driver uses common registers
> -                * rcar_has_irq_support() will be enabled
> -                */
> -               res = platform_get_resource(pdev, IORESOURCE_MEM, mres++);
> -               common->base = devm_ioremap_resource(dev, res);
> -               if (IS_ERR(common->base))
> -                       return PTR_ERR(common->base);
> +       for (i = 0; i < nirq; i++) {

for (i = 0; i < priv->nirqs; i++) {

Gr{oetje,eeting}s,

                        Geert

On Fri, Mar 30, 2018 at 12:13:00PM +0900, Yoshihiro Kaneko wrote:
> This series adds thermal support for r8a77995.
> R-Car D3 (r8a77995) have a thermal sensor module which is similar to Gen2.
> Therefore this series adds r8a77995 support to rcar_thermal driver not
> rcar_gen3_thermal driver.
> 
> This series is based on the next branch of Zhang Rui's linux tree.

I have very lightly tested this as follows after enabling
CONFIG_RCAR_THERMAL in the kernel .config.

# cat /sys/devices/virtual/thermal/thermal_zone0/temp
40000
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Simon-san,

2018-03-30 22:49 GMT+09:00 Simon Horman <horms@verge.net.au>:
> On Fri, Mar 30, 2018 at 12:13:00PM +0900, Yoshihiro Kaneko wrote:
>> This series adds thermal support for r8a77995.
>> R-Car D3 (r8a77995) have a thermal sensor module which is similar to Gen2.
>> Therefore this series adds r8a77995 support to rcar_thermal driver not
>> rcar_gen3_thermal driver.
>>
>> This series is based on the next branch of Zhang Rui's linux tree.
>
> I have very lightly tested this as follows after enabling
> CONFIG_RCAR_THERMAL in the kernel .config.
>
> # cat /sys/devices/virtual/thermal/thermal_zone0/temp
> 40000

Thanks for the testing.
Probably 40C is reasonable, is not?


Best regards,
Kaneko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Geert-san,

2018-03-30 18:25 GMT+09:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> Hi Kaneko-san,
>
> On Fri, Mar 30, 2018 at 5:13 AM, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
>> Add support for R-Car D3 (r8a77995) thermal sensor.
>>
>> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
>
> Thanks for your patch!
>
>> --- a/drivers/thermal/rcar_thermal.c
>> +++ b/drivers/thermal/rcar_thermal.c
>> @@ -58,10 +58,35 @@ struct rcar_thermal_common {
>>         spinlock_t lock;
>>  };
>>
>> +enum rcar_thermal_type {
>> +       RCAR_THERMAL,
>> +       RCAR_GEN2_THERMAL,
>> +       RCAR_GEN3_THERMAL,
>> +};
>> +
>> +struct rcar_thermal_chip {
>> +       int use_of_thermal;
>
> This can be a single bit:
>
>     unsigned int use_of_thermal : 1;
>
>> +       enum rcar_thermal_type type;
>
> If you would add feature bits, you can get rid of rcar_thermal_type:
>
>     unsigned int has_filonoff : 1;
>     unsigned int has_enr : 1;
>     unsigned int needs_suspend_resume : 1;
>
> The number of interrupts can be stored here, too.

It's nice!

>
>> +};
>> +
>> +static const struct rcar_thermal_chip rcar_thermal = {
>> +       .use_of_thermal = 0,
>> +       .type = RCAR_THERMAL,
>
> .has_filonoff = 1,
> .has_enr = 0,
> ...
> .nirqs = 1,
>
>> @@ -190,7 +222,8 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
>>          * enable IRQ
>>          */
>>         if (rcar_has_irq_support(priv)) {
>> -               rcar_thermal_write(priv, FILONOFF, 0);
>> +               if (priv->chip->type != RCAR_GEN3_THERMAL)
>
> if (priv->chip->has_filonoff)
>
>> @@ -438,6 +471,9 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>>         struct rcar_thermal_priv *priv;
>>         struct device *dev = &pdev->dev;
>>         struct resource *res, *irq;
>> +       struct rcar_thermal_chip *chip = ((struct rcar_thermal_chip *)
>
> I don't think the cast is needed.

I will make 'chip' a const variable.

>
>> @@ -457,19 +493,35 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>>         pm_runtime_enable(dev);
>>         pm_runtime_get_sync(dev);
>>
>> -       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> -       if (irq) {
>> -               /*
>> -                * platform has IRQ support.
>> -                * Then, driver uses common registers
>> -                * rcar_has_irq_support() will be enabled
>> -                */
>> -               res = platform_get_resource(pdev, IORESOURCE_MEM, mres++);
>> -               common->base = devm_ioremap_resource(dev, res);
>> -               if (IS_ERR(common->base))
>> -                       return PTR_ERR(common->base);
>> +       for (i = 0; i < nirq; i++) {
>
> for (i = 0; i < priv->nirqs; i++) {


Best regards,
Kaneko

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Apr 03, 2018 at 08:00:50PM +0900, Yoshihiro Kaneko wrote:
> Hi Simon-san,
> 
> 2018-03-30 22:49 GMT+09:00 Simon Horman <horms@verge.net.au>:
> > On Fri, Mar 30, 2018 at 12:13:00PM +0900, Yoshihiro Kaneko wrote:
> >> This series adds thermal support for r8a77995.
> >> R-Car D3 (r8a77995) have a thermal sensor module which is similar to Gen2.
> >> Therefore this series adds r8a77995 support to rcar_thermal driver not
> >> rcar_gen3_thermal driver.
> >>
> >> This series is based on the next branch of Zhang Rui's linux tree.
> >
> > I have very lightly tested this as follows after enabling
> > CONFIG_RCAR_THERMAL in the kernel .config.
> >
> > # cat /sys/devices/virtual/thermal/thermal_zone0/temp
> > 40000
> 
> Thanks for the testing.
> Probably 40C is reasonable, is not?

Yes, probably.

Niklas do you have any guidance here?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Simon,

On 2018-04-09 13:59:27 +0200, Simon Horman wrote:
> On Tue, Apr 03, 2018 at 08:00:50PM +0900, Yoshihiro Kaneko wrote:
> > Hi Simon-san,
> > 
> > 2018-03-30 22:49 GMT+09:00 Simon Horman <horms@verge.net.au>:
> > > On Fri, Mar 30, 2018 at 12:13:00PM +0900, Yoshihiro Kaneko wrote:
> > >> This series adds thermal support for r8a77995.
> > >> R-Car D3 (r8a77995) have a thermal sensor module which is similar to Gen2.
> > >> Therefore this series adds r8a77995 support to rcar_thermal driver not
> > >> rcar_gen3_thermal driver.
> > >>
> > >> This series is based on the next branch of Zhang Rui's linux tree.
> > >
> > > I have very lightly tested this as follows after enabling
> > > CONFIG_RCAR_THERMAL in the kernel .config.
> > >
> > > # cat /sys/devices/virtual/thermal/thermal_zone0/temp
> > > 40000
> > 
> > Thanks for the testing.
> > Probably 40C is reasonable, is not?
> 
> Yes, probably.
> 
> Niklas do you have any guidance here?

Yes judging from other Gen3 boards which al be it is using the gen3 
thermal driver the temperature seems to be in range of what is observed 
there in idle where the 3 different zones on those H3 for example is 
between 38C - 42C.

But I have no method of observing the real temperature other then with 
the driver that is being under test. And then tests i do is to 
increasing the CPU load in order to generate heat and observe the 
temperature increasing. But 40C seems like a reasonable value during 
idle compared to other Gen3 SoCs I looked at.

Hi Kaneko-san,

could you re-spin this series with Geerts concerns (below) addressed.

When you repost I think you can add the tested tags and drop the RFT from
the prefix. I think its likely it can then be merged.

On Tue, Apr 03, 2018 at 08:17:01PM +0900, Yoshihiro Kaneko wrote:
> Hi Geert-san,
> 
> 2018-03-30 18:25 GMT+09:00 Geert Uytterhoeven <geert@linux-m68k.org>:
> > Hi Kaneko-san,
> >
> > On Fri, Mar 30, 2018 at 5:13 AM, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> >> Add support for R-Car D3 (r8a77995) thermal sensor.
> >>
> >> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> >
> > Thanks for your patch!
> >
> >> --- a/drivers/thermal/rcar_thermal.c
> >> +++ b/drivers/thermal/rcar_thermal.c
> >> @@ -58,10 +58,35 @@ struct rcar_thermal_common {
> >>         spinlock_t lock;
> >>  };
> >>
> >> +enum rcar_thermal_type {
> >> +       RCAR_THERMAL,
> >> +       RCAR_GEN2_THERMAL,
> >> +       RCAR_GEN3_THERMAL,
> >> +};
> >> +
> >> +struct rcar_thermal_chip {
> >> +       int use_of_thermal;
> >
> > This can be a single bit:
> >
> >     unsigned int use_of_thermal : 1;
> >
> >> +       enum rcar_thermal_type type;
> >
> > If you would add feature bits, you can get rid of rcar_thermal_type:
> >
> >     unsigned int has_filonoff : 1;
> >     unsigned int has_enr : 1;
> >     unsigned int needs_suspend_resume : 1;
> >
> > The number of interrupts can be stored here, too.
> 
> It's nice!
> 
> >
> >> +};
> >> +
> >> +static const struct rcar_thermal_chip rcar_thermal = {
> >> +       .use_of_thermal = 0,
> >> +       .type = RCAR_THERMAL,
> >
> > .has_filonoff = 1,
> > .has_enr = 0,
> > ...
> > .nirqs = 1,
> >
> >> @@ -190,7 +222,8 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
> >>          * enable IRQ
> >>          */
> >>         if (rcar_has_irq_support(priv)) {
> >> -               rcar_thermal_write(priv, FILONOFF, 0);
> >> +               if (priv->chip->type != RCAR_GEN3_THERMAL)
> >
> > if (priv->chip->has_filonoff)
> >
> >> @@ -438,6 +471,9 @@ static int rcar_thermal_probe(struct platform_device *pdev)
> >>         struct rcar_thermal_priv *priv;
> >>         struct device *dev = &pdev->dev;
> >>         struct resource *res, *irq;
> >> +       struct rcar_thermal_chip *chip = ((struct rcar_thermal_chip *)
> >
> > I don't think the cast is needed.
> 
> I will make 'chip' a const variable.
> 
> >
> >> @@ -457,19 +493,35 @@ static int rcar_thermal_probe(struct platform_device *pdev)
> >>         pm_runtime_enable(dev);
> >>         pm_runtime_get_sync(dev);
> >>
> >> -       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >> -       if (irq) {
> >> -               /*
> >> -                * platform has IRQ support.
> >> -                * Then, driver uses common registers
> >> -                * rcar_has_irq_support() will be enabled
> >> -                */
> >> -               res = platform_get_resource(pdev, IORESOURCE_MEM, mres++);
> >> -               common->base = devm_ioremap_resource(dev, res);
> >> -               if (IS_ERR(common->base))
> >> -                       return PTR_ERR(common->base);
> >> +       for (i = 0; i < nirq; i++) {
> >
> > for (i = 0; i < priv->nirqs; i++) {
> 
> 
> Best regards,
> Kaneko
> 
> >
> > Gr{oetje,eeting}s,
> >
> >                         Geert
> >
> > --
> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> >
> > In personal conversations with technical people, I call myself a hacker. But
> > when I'm talking to journalists I just say "programmer" or something like that.
> >                                 -- Linus Torvalds
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Simon-san,

2018-05-07 21:43 GMT+09:00 Simon Horman <horms@verge.net.au>:
> Hi Kaneko-san,
>
> could you re-spin this series with Geerts concerns (below) addressed.
>
> When you repost I think you can add the tested tags and drop the RFT from
> the prefix. I think its likely it can then be merged.

I had posted V3 that was updated with Geert-san's suggestions.
Should I repost V4 with the tested tags and without the RFT prefix?

>
> On Tue, Apr 03, 2018 at 08:17:01PM +0900, Yoshihiro Kaneko wrote:
>> Hi Geert-san,
>>
>> 2018-03-30 18:25 GMT+09:00 Geert Uytterhoeven <geert@linux-m68k.org>:
>> > Hi Kaneko-san,
>> >
>> > On Fri, Mar 30, 2018 at 5:13 AM, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
>> >> Add support for R-Car D3 (r8a77995) thermal sensor.
>> >>
>> >> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
>> >
>> > Thanks for your patch!
>> >
>> >> --- a/drivers/thermal/rcar_thermal.c
>> >> +++ b/drivers/thermal/rcar_thermal.c
>> >> @@ -58,10 +58,35 @@ struct rcar_thermal_common {
>> >>         spinlock_t lock;
>> >>  };
>> >>
>> >> +enum rcar_thermal_type {
>> >> +       RCAR_THERMAL,
>> >> +       RCAR_GEN2_THERMAL,
>> >> +       RCAR_GEN3_THERMAL,
>> >> +};
>> >> +
>> >> +struct rcar_thermal_chip {
>> >> +       int use_of_thermal;
>> >
>> > This can be a single bit:
>> >
>> >     unsigned int use_of_thermal : 1;
>> >
>> >> +       enum rcar_thermal_type type;
>> >
>> > If you would add feature bits, you can get rid of rcar_thermal_type:
>> >
>> >     unsigned int has_filonoff : 1;
>> >     unsigned int has_enr : 1;
>> >     unsigned int needs_suspend_resume : 1;
>> >
>> > The number of interrupts can be stored here, too.
>>
>> It's nice!
>>
>> >
>> >> +};
>> >> +
>> >> +static const struct rcar_thermal_chip rcar_thermal = {
>> >> +       .use_of_thermal = 0,
>> >> +       .type = RCAR_THERMAL,
>> >
>> > .has_filonoff = 1,
>> > .has_enr = 0,
>> > ...
>> > .nirqs = 1,
>> >
>> >> @@ -190,7 +222,8 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
>> >>          * enable IRQ
>> >>          */
>> >>         if (rcar_has_irq_support(priv)) {
>> >> -               rcar_thermal_write(priv, FILONOFF, 0);
>> >> +               if (priv->chip->type != RCAR_GEN3_THERMAL)
>> >
>> > if (priv->chip->has_filonoff)
>> >
>> >> @@ -438,6 +471,9 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>> >>         struct rcar_thermal_priv *priv;
>> >>         struct device *dev = &pdev->dev;
>> >>         struct resource *res, *irq;
>> >> +       struct rcar_thermal_chip *chip = ((struct rcar_thermal_chip *)
>> >
>> > I don't think the cast is needed.
>>
>> I will make 'chip' a const variable.
>>
>> >
>> >> @@ -457,19 +493,35 @@ static int rcar_thermal_probe(struct platform_device *pdev)
>> >>         pm_runtime_enable(dev);
>> >>         pm_runtime_get_sync(dev);
>> >>
>> >> -       irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> >> -       if (irq) {
>> >> -               /*
>> >> -                * platform has IRQ support.
>> >> -                * Then, driver uses common registers
>> >> -                * rcar_has_irq_support() will be enabled
>> >> -                */
>> >> -               res = platform_get_resource(pdev, IORESOURCE_MEM, mres++);
>> >> -               common->base = devm_ioremap_resource(dev, res);
>> >> -               if (IS_ERR(common->base))
>> >> -                       return PTR_ERR(common->base);
>> >> +       for (i = 0; i < nirq; i++) {
>> >
>> > for (i = 0; i < priv->nirqs; i++) {
>>
>>
>> Best regards,
>> Kaneko
>>
>> >
>> > Gr{oetje,eeting}s,
>> >
>> >                         Geert
>> >
>> > --
>> > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>> >
>> > In personal conversations with technical people, I call myself a hacker. But
>> > when I'm talking to journalists I just say "programmer" or something like that.
>> >                                 -- Linus Torvalds
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, May 09, 2018 at 10:35:16PM +0900, Yoshihiro Kaneko wrote:
> Hi Simon-san,
> 
> 2018-05-07 21:43 GMT+09:00 Simon Horman <horms@verge.net.au>:
> > Hi Kaneko-san,
> >
> > could you re-spin this series with Geerts concerns (below) addressed.
> >
> > When you repost I think you can add the tested tags and drop the RFT from
> > the prefix. I think its likely it can then be merged.
> 
> I had posted V3 that was updated with Geert-san's suggestions.
> Should I repost V4 with the tested tags and without the RFT prefix?

Sorry, I missed that when I wrote the above.
I will respond to v3.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html