diff mbox

[RFC,6/6] ARM: dts: am57xx-beagle-x15-common: enable etnaviv

Message ID 20161118024436.13447-6-robertcnelson@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Robert Nelson Nov. 18, 2016, 2:44 a.m. UTC
Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
CC: Julien <jboulnois@gmail.com>
CC: Nishanth Menon <nm@ti.com>
CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
CC: Tony Lindgren <tony@atomide.com>
---
 arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Nishanth Menon Nov. 18, 2016, 2:56 a.m. UTC | #1
On 11/17/2016 08:44 PM, Robert Nelson wrote:
again.. a short commit message at least please? :)

> Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
> CC: Julien <jboulnois@gmail.com>
> CC: Nishanth Menon <nm@ti.com>
> CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
> CC: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> index 6df7829..3bc47be 100644
> --- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> +++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> @@ -97,6 +97,12 @@
>  		#cooling-cells = <2>;
>  	};
>
> +	gpu-subsystem {
A) do we want to make things clear that this is gpu subsystem for gc320?
B) How about other platforms that could equally reuse?

> +		compatible = "ti,gc320-gpu-subsystem";
> +		cores = <&bb2d>;
> +		status = "okay";
> +	};
> +
>  	hdmi0: connector {
>  		compatible = "hdmi-connector";
>  		label = "hdmi";
> @@ -190,6 +196,11 @@
>  		>;
>  	};
>  };
> +
> +&bb2d {
> +	status = "okay";
> +};
> +
>  &i2c1 {
>  	status = "okay";
>  	clock-frequency = <400000>;
>
Robert Nelson Nov. 18, 2016, 3:44 a.m. UTC | #2
On Thu, Nov 17, 2016 at 8:56 PM, Nishanth Menon <nm@ti.com> wrote:
> On 11/17/2016 08:44 PM, Robert Nelson wrote:
> again.. a short commit message at least please? :)

yeah, i'll fix all those. ;)

>
>> Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
>> CC: Julien <jboulnois@gmail.com>
>> CC: Nishanth Menon <nm@ti.com>
>> CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> CC: Tony Lindgren <tony@atomide.com>
>> ---
>>  arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>> b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>> index 6df7829..3bc47be 100644
>> --- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>> +++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>> @@ -97,6 +97,12 @@
>>                 #cooling-cells = <2>;
>>         };
>>
>> +       gpu-subsystem {
>
> A) do we want to make things clear that this is gpu subsystem for gc320?
> B) How about other platforms that could equally reuse?

so the 'gpu-subsystem' comes from etnaviv:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt?id=refs/tags/v4.9-rc5

For a generic name, it's currently only tied to the etnaviv driver:

gpu-subsystem {
 compatible = "fsl,imx-gpu-subsystem";
 cores = <&gpu_2d>, <&gpu_3d>;
};

it would make sense to make that more generic, so you could tie a 2d
vivante and a imgtec/sgx 3d core..  <sad laugh> but that would require
adding a imgtec/sgx driver/bindings to the kernel mainline... </sad
laugh>

>
>> +               compatible = "ti,gc320-gpu-subsystem";
>> +               cores = <&bb2d>;
>> +               status = "okay";
>> +       };
>> +
>>         hdmi0: connector {
>>                 compatible = "hdmi-connector";
>>                 label = "hdmi";
>> @@ -190,6 +196,11 @@
>>                 >;
>>         };
>>  };
>> +
>> +&bb2d {
>> +       status = "okay";
>> +};
>> +
>>  &i2c1 {
>>         status = "okay";
>>         clock-frequency = <400000>;
>>

Regards,
Nishanth Menon Nov. 18, 2016, 4:15 a.m. UTC | #3
On 11/17/2016 09:44 PM, Robert Nelson wrote:
> On Thu, Nov 17, 2016 at 8:56 PM, Nishanth Menon <nm@ti.com> wrote:
>> On 11/17/2016 08:44 PM, Robert Nelson wrote:
>> again.. a short commit message at least please? :)
>
> yeah, i'll fix all those. ;)
>
>>
>>> Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
>>> CC: Julien <jboulnois@gmail.com>
>>> CC: Nishanth Menon <nm@ti.com>
>>> CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> CC: Tony Lindgren <tony@atomide.com>
>>> ---
>>>  arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>>> b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>>> index 6df7829..3bc47be 100644
>>> --- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>>> +++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>>> @@ -97,6 +97,12 @@
>>>                 #cooling-cells = <2>;
>>>         };
>>>
>>> +       gpu-subsystem {
>>
>> A) do we want to make things clear that this is gpu subsystem for gc320?
>> B) How about other platforms that could equally reuse?
>
> so the 'gpu-subsystem' comes from etnaviv:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt?id=refs/tags/v4.9-rc5
>
> For a generic name, it's currently only tied to the etnaviv driver:
>

I was only complaining about "gpu-subsystem {", not the compatible. it 
is not the only gpu subsystem on the SoC. either "gpu-subsystem0 {" or 
something like gpu-subsystem-gc320 might be helpful to clarify.

> gpu-subsystem {
>  compatible = "fsl,imx-gpu-subsystem";
>  cores = <&gpu_2d>, <&gpu_3d>;
> };
>
> it would make sense to make that more generic, so you could tie a 2d
> vivante and a imgtec/sgx 3d core..  <sad laugh> but that would require
> adding a imgtec/sgx driver/bindings to the kernel mainline... </sad
> laugh>
>

I should have clarified... I meant other dra7 devices to reuse the 
same definitions. this definition is not by any means constrained to 
EVM - it is a SoC definition, it should be moved to appropriate place 
(convention for dra7 is to mark them as disabled by default in 
SoC.dtsi to prevent proliferation of paper spin dtsi and just do 
"status = okay" in board file to indicate presence in the variation 
for the board).

Yes - I guess some day there might be a bunch of folks like etnaviv 
who might make an community driver possible..
Robert Nelson Nov. 18, 2016, 4:26 a.m. UTC | #4
On Thu, Nov 17, 2016 at 10:15 PM, Nishanth Menon <nm@ti.com> wrote:
> On 11/17/2016 09:44 PM, Robert Nelson wrote:
>>
>> On Thu, Nov 17, 2016 at 8:56 PM, Nishanth Menon <nm@ti.com> wrote:
>>>
>>> On 11/17/2016 08:44 PM, Robert Nelson wrote:
>>> again.. a short commit message at least please? :)
>>
>>
>> yeah, i'll fix all those. ;)
>>
>>>
>>>> Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
>>>> CC: Julien <jboulnois@gmail.com>
>>>> CC: Nishanth Menon <nm@ti.com>
>>>> CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>> CC: Tony Lindgren <tony@atomide.com>
>>>> ---
>>>>  arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>>>> b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>>>> index 6df7829..3bc47be 100644
>>>> --- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>>>> +++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
>>>> @@ -97,6 +97,12 @@
>>>>                 #cooling-cells = <2>;
>>>>         };
>>>>
>>>> +       gpu-subsystem {
>>>
>>>
>>> A) do we want to make things clear that this is gpu subsystem for gc320?
>>> B) How about other platforms that could equally reuse?
>>
>>
>> so the 'gpu-subsystem' comes from etnaviv:
>>
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt?id=refs/tags/v4.9-rc5
>>
>> For a generic name, it's currently only tied to the etnaviv driver:
>>
>
> I was only complaining about "gpu-subsystem {", not the compatible. it is
> not the only gpu subsystem on the SoC. either "gpu-subsystem0 {" or
> something like gpu-subsystem-gc320 might be helpful to clarify.
>
>> gpu-subsystem {
>>  compatible = "fsl,imx-gpu-subsystem";
>>  cores = <&gpu_2d>, <&gpu_3d>;
>> };
>>
>> it would make sense to make that more generic, so you could tie a 2d
>> vivante and a imgtec/sgx 3d core..  <sad laugh> but that would require
>> adding a imgtec/sgx driver/bindings to the kernel mainline... </sad
>> laugh>
>>
>
> I should have clarified... I meant other dra7 devices to reuse the same
> definitions. this definition is not by any means constrained to EVM - it is
> a SoC definition, it should be moved to appropriate place (convention for
> dra7 is to mark them as disabled by default in SoC.dtsi to prevent
> proliferation of paper spin dtsi and just do "status = okay" in board file
> to indicate presence in the variation for the board).

Oh yeah, defintely, we can move gpu-subsystem to the base dra7.dtsi,
as the whole dra.dtsi family has a gc320 and then the board device
tree can enable it via:

&bb2d {
       status = "okay";
};

Regards,
Nishanth Menon Nov. 18, 2016, 4:34 a.m. UTC | #5
On 11/17/2016 10:26 PM, Robert Nelson wrote:
[...]
> Oh yeah, defintely, we can move gpu-subsystem to the base dra7.dtsi,
> as the whole dra.dtsi family has a gc320 and then the board device
> tree can enable it via:
>
> &bb2d {
>        status = "okay";
> };

Yep, thanks.
Rob Herring Nov. 18, 2016, 4:42 p.m. UTC | #6
On Thu, Nov 17, 2016 at 08:56:18PM -0600, Nishanth Menon wrote:
> On 11/17/2016 08:44 PM, Robert Nelson wrote:
> again.. a short commit message at least please? :)
> 
> > Signed-off-by: Robert Nelson <robertcnelson@gmail.com>
> > CC: Julien <jboulnois@gmail.com>
> > CC: Nishanth Menon <nm@ti.com>
> > CC: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > CC: Tony Lindgren <tony@atomide.com>
> > ---
> >  arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> > index 6df7829..3bc47be 100644
> > --- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> > +++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
> > @@ -97,6 +97,12 @@
> >  		#cooling-cells = <2>;
> >  	};
> > 
> > +	gpu-subsystem {
> A) do we want to make things clear that this is gpu subsystem for gc320?

No.

> B) How about other platforms that could equally reuse?

Better yet, get rid of this nonsense. You are defining a wrapper to 
group 1 nodes. Yes, I know this is what the driver wants, but you should 
be able to make it deal with this situation and only have a gc320 node.

> > +		compatible = "ti,gc320-gpu-subsystem";
> > +		cores = <&bb2d>;
> > +		status = "okay";
> > +	};
> > +
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
index 6df7829..3bc47be 100644
--- a/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
+++ b/arch/arm/boot/dts/am57xx-beagle-x15-common.dtsi
@@ -97,6 +97,12 @@ 
 		#cooling-cells = <2>;
 	};
 
+	gpu-subsystem {
+		compatible = "ti,gc320-gpu-subsystem";
+		cores = <&bb2d>;
+		status = "okay";
+	};
+
 	hdmi0: connector {
 		compatible = "hdmi-connector";
 		label = "hdmi";
@@ -190,6 +196,11 @@ 
 		>;
 	};
 };
+
+&bb2d {
+	status = "okay";
+};
+
 &i2c1 {
 	status = "okay";
 	clock-frequency = <400000>;