mbox series

[v3,0/3] Support NVIDIA Tegra-based Ouya game console

Message ID 20201004133114.845230-1-pgwipeout@gmail.com
Headers show
Series Support NVIDIA Tegra-based Ouya game console | expand

Message

Peter Geis Oct. 4, 2020, 1:31 p.m. UTC
Good Day,

This series introduces upstream kernel support for the Ouya game console device. Please review and apply. Thank you in advance.

Changelog:
v3: - Reorder aliases per Dmitry Osipenko's review.
    - Add sdio clocks per Dmitry Osipenko's review.
    - Add missing ti sleep bits per Dmitry Osipenko's review.
    - Enable lp1 sleep mode.
    - Fix bluetooth comment and add missing power supplies.

v2: - Update pmic and clock handles per Rob Herring's review.
    - Add acks from Rob Herring to patch 2 and 3.

Peter Geis (3):
  ARM: tegra: Add device-tree for Ouya
  dt-bindings: Add vendor prefix for Ouya Inc.
  dt-bindings: ARM: tegra: Add Ouya game console

 .../devicetree/bindings/arm/tegra.yaml        |    3 +
 .../devicetree/bindings/vendor-prefixes.yaml  |    2 +
 arch/arm/boot/dts/Makefile                    |    3 +-
 arch/arm/boot/dts/tegra30-ouya.dts            | 4511 +++++++++++++++++
 4 files changed, 4518 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/boot/dts/tegra30-ouya.dts

Comments

Dmitry Osipenko Oct. 5, 2020, 2:02 p.m. UTC | #1
04.10.2020 16:31, Peter Geis пишет:
> +	thermal-zones {
> +		cpu_thermal: cpu-thermal {
> +			polling-delay = <5000>;
> +			polling-delay-passive = <5000>;
> +
> +			thermal-sensors = <&cpu_temp 1>;
> +
> +			trips {
> +				cpu_alert0: cpu-alert0 {
> +					temperature = <50000>;
> +					hysteresis = <10000>;

Hello, Peter!

A day ago I was tuning thermal zones for Nexus 7 because found that the
current variant is a bit too unpractical. In particular temperature
hysteresis should be small, otherwise cpufreq could get throttled
enormously to the point that device becomes unusable. This is an
active-cooling zone, but it looks to me that hysteresis is a bit too
high and should make Ouya much noisier than it could be.

I suggest to try to set hysteresis to 0.2C here, i.e. hysteresis = <200>.

I also suggest to bump the temperature threshold to 55C in order to
ignore temporal temperature spikes because CPU temp should be about 40C
during idle and then it may raise quickly for a brief moments during
average usage.

> +					type = "active";
> +				};
> +				cpu_alert1: cpu-alert1 {
> +					temperature = <70000>;
> +					hysteresis = <5000>;
> +					type = "passive";
> +				};

And here to 0.2C as well.

> +				cpu_crit: cpu-crit {
> +					temperature = <90000>;
> +					hysteresis = <2000>;
> +					type = "critical";
> +				};

The critical zone perhaps should be fine as-is.
Peter Geis Oct. 5, 2020, 2:22 p.m. UTC | #2
On Mon, Oct 5, 2020 at 10:02 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 04.10.2020 16:31, Peter Geis пишет:
> > +     thermal-zones {
> > +             cpu_thermal: cpu-thermal {
> > +                     polling-delay = <5000>;
> > +                     polling-delay-passive = <5000>;
> > +
> > +                     thermal-sensors = <&cpu_temp 1>;
> > +
> > +                     trips {
> > +                             cpu_alert0: cpu-alert0 {
> > +                                     temperature = <50000>;
> > +                                     hysteresis = <10000>;
>
> Hello, Peter!
>
> A day ago I was tuning thermal zones for Nexus 7 because found that the
> current variant is a bit too unpractical. In particular temperature
> hysteresis should be small, otherwise cpufreq could get throttled
> enormously to the point that device becomes unusable. This is an
> active-cooling zone, but it looks to me that hysteresis is a bit too
> high and should make Ouya much noisier than it could be.
>
> I suggest to try to set hysteresis to 0.2C here, i.e. hysteresis = <200>.
>
> I also suggest to bump the temperature threshold to 55C in order to
> ignore temporal temperature spikes because CPU temp should be about 40C
> during idle and then it may raise quickly for a brief moments during
> average usage.

Good Morning,

The Ouya has a rather large heatsink with poor conductivity and a
single stage fan.
The fan is not terribly loud, as it runs at a rather low rpm.
The temperature at the cpu tends to be much higher than observed at
the heatsink.
I've found that a low hysteresis value leads to a situation where the
fan is constantly toggling on and off.
I actually burned out the power transistor driving the fan on my first
Ouya this way.
The high hysteresis value provides a good balance of fan on/off time
under most loads.
The temperature threshold was chosen for this as well.

>
> > +                                     type = "active";
> > +                             };
> > +                             cpu_alert1: cpu-alert1 {
> > +                                     temperature = <70000>;
> > +                                     hysteresis = <5000>;
> > +                                     type = "passive";
> > +                             };
>
> And here to 0.2C as well.

This value was chosen to protect the CPU in case of fan failure.
Again, with the large heatsink and poor conductivity we need to
maintain throttling for longer.
Otherwise the cpu temp will overshoot and risk thermal shutdown before
throttling can prevent it.

>
> > +                             cpu_crit: cpu-crit {
> > +                                     temperature = <90000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "critical";
> > +                             };
>
> The critical zone perhaps should be fine as-is.
Dmitry Osipenko Oct. 5, 2020, 2:29 p.m. UTC | #3
05.10.2020 17:22, Peter Geis пишет:
> On Mon, Oct 5, 2020 at 10:02 AM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 04.10.2020 16:31, Peter Geis пишет:
>>> +     thermal-zones {
>>> +             cpu_thermal: cpu-thermal {
>>> +                     polling-delay = <5000>;
>>> +                     polling-delay-passive = <5000>;
>>> +
>>> +                     thermal-sensors = <&cpu_temp 1>;
>>> +
>>> +                     trips {
>>> +                             cpu_alert0: cpu-alert0 {
>>> +                                     temperature = <50000>;
>>> +                                     hysteresis = <10000>;
>>
>> Hello, Peter!
>>
>> A day ago I was tuning thermal zones for Nexus 7 because found that the
>> current variant is a bit too unpractical. In particular temperature
>> hysteresis should be small, otherwise cpufreq could get throttled
>> enormously to the point that device becomes unusable. This is an
>> active-cooling zone, but it looks to me that hysteresis is a bit too
>> high and should make Ouya much noisier than it could be.
>>
>> I suggest to try to set hysteresis to 0.2C here, i.e. hysteresis = <200>.
>>
>> I also suggest to bump the temperature threshold to 55C in order to
>> ignore temporal temperature spikes because CPU temp should be about 40C
>> during idle and then it may raise quickly for a brief moments during
>> average usage.
> 
> Good Morning,
> 
> The Ouya has a rather large heatsink with poor conductivity and a
> single stage fan.
> The fan is not terribly loud, as it runs at a rather low rpm.
> The temperature at the cpu tends to be much higher than observed at
> the heatsink.
> I've found that a low hysteresis value leads to a situation where the
> fan is constantly toggling on and off.
> I actually burned out the power transistor driving the fan on my first
> Ouya this way.
> The high hysteresis value provides a good balance of fan on/off time
> under most loads.
> The temperature threshold was chosen for this as well.
> 
>>
>>> +                                     type = "active";
>>> +                             };
>>> +                             cpu_alert1: cpu-alert1 {
>>> +                                     temperature = <70000>;
>>> +                                     hysteresis = <5000>;
>>> +                                     type = "passive";
>>> +                             };
>>
>> And here to 0.2C as well.
> 
> This value was chosen to protect the CPU in case of fan failure.
> Again, with the large heatsink and poor conductivity we need to
> maintain throttling for longer.
> Otherwise the cpu temp will overshoot and risk thermal shutdown before
> throttling can prevent it.

Alright! Then I don't have any more comments for now, thanks :)
Dmitry Osipenko Oct. 5, 2020, 2:29 p.m. UTC | #4
04.10.2020 16:31, Peter Geis пишет:
> The Ouya was the sole device produced by Ouya Inc in 2013.
> It was a game console originally running Android 5 on top of Linux 3.1.10.
> 
> This patch adds the device tree supporting the Ouya.
> It has been tested on the original variant with Samsung ram.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Thierry Reding Oct. 5, 2020, 3:47 p.m. UTC | #5
On Sun, Oct 04, 2020 at 01:31:11PM +0000, Peter Geis wrote:
> Good Day,
> 
> This series introduces upstream kernel support for the Ouya game console device. Please review and apply. Thank you in advance.
> 
> Changelog:
> v3: - Reorder aliases per Dmitry Osipenko's review.
>     - Add sdio clocks per Dmitry Osipenko's review.
>     - Add missing ti sleep bits per Dmitry Osipenko's review.
>     - Enable lp1 sleep mode.
>     - Fix bluetooth comment and add missing power supplies.
> 
> v2: - Update pmic and clock handles per Rob Herring's review.
>     - Add acks from Rob Herring to patch 2 and 3.
> 
> Peter Geis (3):
>   ARM: tegra: Add device-tree for Ouya
>   dt-bindings: Add vendor prefix for Ouya Inc.
>   dt-bindings: ARM: tegra: Add Ouya game console
> 
>  .../devicetree/bindings/arm/tegra.yaml        |    3 +
>  .../devicetree/bindings/vendor-prefixes.yaml  |    2 +
>  arch/arm/boot/dts/Makefile                    |    3 +-
>  arch/arm/boot/dts/tegra30-ouya.dts            | 4511 +++++++++++++++++
>  4 files changed, 4518 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/boot/dts/tegra30-ouya.dts

This looks good to me and I see that Dmitry is happy with it. I'll make
a mental note to apply it after the merge window closes.

Thanks,
Thierry
Stefan Agner Oct. 7, 2020, 7:22 a.m. UTC | #6
Hi Peter,

On 2020-10-04 15:31, Peter Geis wrote:
> Good Day,
> 
> This series introduces upstream kernel support for the Ouya game
> console device. Please review and apply. Thank you in advance.

Interesting patchset, maybe I can give my Ouya a second live now :-) Do
you happen to have (a link) to instructions how to flash the device?

Btw, there was also a driver for the Bluetooth controller on the ML
once, maybe a good time to revive that:
https://spinics.net/lists/linux-input/msg56288.html

--
Stefan

> 
> Changelog:
> v3: - Reorder aliases per Dmitry Osipenko's review.
>     - Add sdio clocks per Dmitry Osipenko's review.
>     - Add missing ti sleep bits per Dmitry Osipenko's review.
>     - Enable lp1 sleep mode.
>     - Fix bluetooth comment and add missing power supplies.
> 
> v2: - Update pmic and clock handles per Rob Herring's review.
>     - Add acks from Rob Herring to patch 2 and 3.
> 
> Peter Geis (3):
>   ARM: tegra: Add device-tree for Ouya
>   dt-bindings: Add vendor prefix for Ouya Inc.
>   dt-bindings: ARM: tegra: Add Ouya game console
> 
>  .../devicetree/bindings/arm/tegra.yaml        |    3 +
>  .../devicetree/bindings/vendor-prefixes.yaml  |    2 +
>  arch/arm/boot/dts/Makefile                    |    3 +-
>  arch/arm/boot/dts/tegra30-ouya.dts            | 4511 +++++++++++++++++
>  4 files changed, 4518 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/boot/dts/tegra30-ouya.dts
Bob Ham Oct. 7, 2020, 1:36 p.m. UTC | #7
Hi all,

The Bluetooth controller driver sent to linux-input by Lukas Rusak
(CC'd) is a bit of a concern.  Here is the original driver:

https://github.com/ouya/ouya_1_1-kernel/blob/master/drivers/hid/hid-ouya.c

and you can see that there is no SPDX header, no license information and
no MODULE_LICENSE declaration.  I'd previously noticed this as a
possible cause for concern in upstreaming the driver.

Meanwhile, Lukas's driver is clearly derived from the Ouya Inc. driver
and even retains the Ouya Inc. copyright notice line.  However, Lukas's
driver now has a MODULE_LICENSE("GPL") declaration added.

Lukas, did you get clear permission to license the driver as GPL?

Alternatively, kernel developers with greater legal or Ouya knowledge,
is this actually a concern or is it nothing to worry about?

Thanks,

Bob


On 07/10/2020 08:22, Stefan Agner wrote:
> Hi Peter,
> 
> On 2020-10-04 15:31, Peter Geis wrote:
>> Good Day,
>>
>> This series introduces upstream kernel support for the Ouya game
>> console device. Please review and apply. Thank you in advance.
> 
> Interesting patchset, maybe I can give my Ouya a second live now :-) Do
> you happen to have (a link) to instructions how to flash the device?
> 
> Btw, there was also a driver for the Bluetooth controller on the ML
> once, maybe a good time to revive that:
> https://spinics.net/lists/linux-input/msg56288.html
> 
> --
> Stefan
> 
>>
>> Changelog:
>> v3: - Reorder aliases per Dmitry Osipenko's review.
>>     - Add sdio clocks per Dmitry Osipenko's review.
>>     - Add missing ti sleep bits per Dmitry Osipenko's review.
>>     - Enable lp1 sleep mode.
>>     - Fix bluetooth comment and add missing power supplies.
>>
>> v2: - Update pmic and clock handles per Rob Herring's review.
>>     - Add acks from Rob Herring to patch 2 and 3.
>>
>> Peter Geis (3):
>>   ARM: tegra: Add device-tree for Ouya
>>   dt-bindings: Add vendor prefix for Ouya Inc.
>>   dt-bindings: ARM: tegra: Add Ouya game console
>>
>>  .../devicetree/bindings/arm/tegra.yaml        |    3 +
>>  .../devicetree/bindings/vendor-prefixes.yaml  |    2 +
>>  arch/arm/boot/dts/Makefile                    |    3 +-
>>  arch/arm/boot/dts/tegra30-ouya.dts            | 4511 +++++++++++++++++
>>  4 files changed, 4518 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/arm/boot/dts/tegra30-ouya.dts
>
Dmitry Osipenko Oct. 7, 2020, 1:53 p.m. UTC | #8
07.10.2020 16:36, Bob Ham пишет:
> Hi all,
> 
> The Bluetooth controller driver sent to linux-input by Lukas Rusak
> (CC'd) is a bit of a concern.  Here is the original driver:
> 
> https://github.com/ouya/ouya_1_1-kernel/blob/master/drivers/hid/hid-ouya.c
> 
> and you can see that there is no SPDX header, no license information and
> no MODULE_LICENSE declaration.  I'd previously noticed this as a
> possible cause for concern in upstreaming the driver.
> 
> Meanwhile, Lukas's driver is clearly derived from the Ouya Inc. driver
> and even retains the Ouya Inc. copyright notice line.  However, Lukas's
> driver now has a MODULE_LICENSE("GPL") declaration added.
> 
> Lukas, did you get clear permission to license the driver as GPL?
> 
> Alternatively, kernel developers with greater legal or Ouya knowledge,
> is this actually a concern or is it nothing to worry about?

Hello Bob,

That can't be a problem because:

1. Ouya Inc. doesn't exists anymore.

2. If code was officially published, then this implies that it can be
derived.

3. Ouya's driver uses kernel symbols that are explicitly marked as
GPL-only, see hid_open_report for example. Hence Ouya's driver inherents
the GPL license.

4. Lukas's patch doesn't remind the original code at all.

5. In practice nobody cares about legal much if money aren't involved.
Even if it happened that driver was 100% violating some copyrights, then
it either won't be accepted to upstream or will be removed by request
from the copyrights holder.
Dmitry Osipenko Oct. 7, 2020, 2:01 p.m. UTC | #9
...
> 3. Ouya's driver uses kernel symbols that are explicitly marked as
> GPL-only, see hid_open_report for example. Hence Ouya's driver inherents
> the GPL license.

Actually, the __hid_register_driver is a better example because
hid_open_report didn't exist when Ouya's driver was created.
Stephen Warren Oct. 7, 2020, 4:08 p.m. UTC | #10
On 10/7/20 7:53 AM, Dmitry Osipenko wrote:
> 07.10.2020 16:36, Bob Ham пишет:
>> Hi all,
>>
>> The Bluetooth controller driver sent to linux-input by Lukas Rusak
>> (CC'd) is a bit of a concern.  Here is the original driver:
>>
>> https://github.com/ouya/ouya_1_1-kernel/blob/master/drivers/hid/hid-ouya.c
>>
>> and you can see that there is no SPDX header, no license information and
>> no MODULE_LICENSE declaration.  I'd previously noticed this as a
>> possible cause for concern in upstreaming the driver.
>>
>> Meanwhile, Lukas's driver is clearly derived from the Ouya Inc. driver
>> and even retains the Ouya Inc. copyright notice line.  However, Lukas's
>> driver now has a MODULE_LICENSE("GPL") declaration added.
>>
>> Lukas, did you get clear permission to license the driver as GPL?
>>
>> Alternatively, kernel developers with greater legal or Ouya knowledge,
>> is this actually a concern or is it nothing to worry about?
> 
> Hello Bob,
> 
> That can't be a problem because:
> 
> 1. Ouya Inc. doesn't exists anymore.
> 
> 2. If code was officially published, then this implies that it can be
> derived.
> 
> 3. Ouya's driver uses kernel symbols that are explicitly marked as
> GPL-only, see hid_open_report for example. Hence Ouya's driver inherents
> the GPL license.
> 
> 4. Lukas's patch doesn't remind the original code at all.
> 
> 5. In practice nobody cares about legal much if money aren't involved.
> Even if it happened that driver was 100% violating some copyrights, then
> it either won't be accepted to upstream or will be removed by request
> from the copyrights holder.

This definitely isn't the correct attitude to copyright.

The facts[1] that Ouya published the code and that it used GPL-only
symbols certainly does imply that they *should* have published under GPL
or a compatible license, but doesn't mean that they definitely did. The
only way to know that for sure is for there to be evidence in the file
content or git history, such as license headers or Signed-off-by lines.

When someone writes Signed-off-by in their code submission, which is
required to contribute to upstream Linux, they are stating/warranting
certain things about the code they're submitting. One aspect is that
they definitely have the rights to submit the code under the given
license. Without evidence to this effect, or having written the code
themselves, nobody can state/warrant this. Take a look at the following
link to see what you're stating/warranting when writing Signed-off-by in
a code submission:

https://developercertificate.org/

[1] I haven't checked the facts.
Peter Geis Oct. 7, 2020, 5:18 p.m. UTC | #11
On Wed, Oct 7, 2020 at 12:08 PM Stephen Warren <swarren@wwwdotorg.org> wrote:
>
>
> This definitely isn't the correct attitude to copyright.
>
> The facts[1] that Ouya published the code and that it used GPL-only
> symbols certainly does imply that they *should* have published under GPL
> or a compatible license, but doesn't mean that they definitely did. The
> only way to know that for sure is for there to be evidence in the file
> content or git history, such as license headers or Signed-off-by lines.
>
> When someone writes Signed-off-by in their code submission, which is
> required to contribute to upstream Linux, they are stating/warranting
> certain things about the code they're submitting. One aspect is that
> they definitely have the rights to submit the code under the given
> license. Without evidence to this effect, or having written the code
> themselves, nobody can state/warrant this. Take a look at the following
> link to see what you're stating/warranting when writing Signed-off-by in
> a code submission:
>
> https://developercertificate.org/
>
> [1] I haven't checked the facts.

Unfortunately the only source release I've been able to find is the
last release.
Ouya purposely purged the git history for the current source release,
it's also completely lacking in signoffs and commit history.
[1] is the commit that added this driver to their source.

Their original source release had the git history intact, but I never
got a copy of it before Ouya purged it from their repos.

[1] https://github.com/ouya/ouya_1_1-kernel/commit/407e72990e39d5c29d015e21110b9497aea2dfe3
Dmitry Osipenko Oct. 8, 2020, 5:05 a.m. UTC | #12
07.10.2020 19:08, Stephen Warren пишет:
...
> The facts[1] that Ouya published the code and that it used GPL-only
> symbols certainly does imply that they *should* have published under GPL
> or a compatible license, but doesn't mean that they definitely did. The
> only way to know that for sure is for there to be evidence in the file
> content or git history, such as license headers or Signed-off-by lines.

The code wasn't only published, but also was distributed in a binary
form to end users. This means that even if Ouya Inc. still existed and
they made a mistake by using GPL-only symbols plus GPL-incompatible
license for the driver, then they had to resilience the code. Hence
either way it's okay to use downstream code as a reference for the
upstream driver.

This is my understanding.
Thierry Reding Nov. 10, 2020, 5:42 p.m. UTC | #13
On Sun, Oct 04, 2020 at 01:31:11PM +0000, Peter Geis wrote:
> Good Day,
> 
> This series introduces upstream kernel support for the Ouya game console device. Please review and apply. Thank you in advance.
> 
> Changelog:
> v3: - Reorder aliases per Dmitry Osipenko's review.
>     - Add sdio clocks per Dmitry Osipenko's review.
>     - Add missing ti sleep bits per Dmitry Osipenko's review.
>     - Enable lp1 sleep mode.
>     - Fix bluetooth comment and add missing power supplies.
> 
> v2: - Update pmic and clock handles per Rob Herring's review.
>     - Add acks from Rob Herring to patch 2 and 3.
> 
> Peter Geis (3):
>   ARM: tegra: Add device-tree for Ouya
>   dt-bindings: Add vendor prefix for Ouya Inc.
>   dt-bindings: ARM: tegra: Add Ouya game console
> 
>  .../devicetree/bindings/arm/tegra.yaml        |    3 +
>  .../devicetree/bindings/vendor-prefixes.yaml  |    2 +
>  arch/arm/boot/dts/Makefile                    |    3 +-
>  arch/arm/boot/dts/tegra30-ouya.dts            | 4511 +++++++++++++++++
>  4 files changed, 4518 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/boot/dts/tegra30-ouya.dts

Applied, thanks.

Thierry