diff mbox

[14/17] ARM: dts: Add missing gpu node and binding for omap4

Message ID 20170828211918.11573-15-tony@atomide.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tony Lindgren Aug. 28, 2017, 9:19 p.m. UTC
On omap4 we're missing the PowerVR SGX GPU node with it's related
"ti,hwmods" property that the SoC interconnect code needs.

Note that this will only show up as a bug with "doesn't have
mpu register target base" boot errors when the legacy platform
data is removed.

Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 .../devicetree/bindings/gpu/ti-powervr-sgx.txt     | 34 ++++++++++++++++++++++
 arch/arm/boot/dts/omap4.dtsi                       |  7 +++++
 2 files changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/ti-powervr-sgx.txt

Comments

Sebastian Reichel Aug. 29, 2017, 9 a.m. UTC | #1
Hi,

On Mon, Aug 28, 2017 at 02:19:15PM -0700, Tony Lindgren wrote:
> On omap4 we're missing the PowerVR SGX GPU node with it's related
> "ti,hwmods" property that the SoC interconnect code needs.
> 
> Note that this will only show up as a bug with "doesn't have
> mpu register target base" boot errors when the legacy platform
> data is removed.
> 
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---

I think OMAP3 & OMAP5 should also be documented and getting a
node in this series?

-- Sebastian

>  .../devicetree/bindings/gpu/ti-powervr-sgx.txt     | 34 ++++++++++++++++++++++
>  arch/arm/boot/dts/omap4.dtsi                       |  7 +++++
>  2 files changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpu/ti-powervr-sgx.txt
> 
> diff --git a/Documentation/devicetree/bindings/gpu/ti-powervr-sgx.txt b/Documentation/devicetree/bindings/gpu/ti-powervr-sgx.txt
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/ti-powervr-sgx.txt
> @@ -0,0 +1,34 @@
> +Texas Instruments PowevVR SGX binding
> +
> +SGX can be used for graphics acceleration on Texas Instruments SoCs.
> +
> +Note that the SGX binding is currently only used by the SoC interconnect
> +code to idle the module on init and no open source driver is available
> +for SGX. Please update this documentation if that changes.
> +
> +Required properties:
> +
> +compatible: Shall be one of the following:
> +	    "ti,omap4-gpu"
> +
> +reg: Shall contain the device instance IO range
> +
> +interrupts: Shall contain the device instance interrupt
> +
> +
> +Optional properties:
> +
> +reg-names: Shall contain the IO range names if multiple IO
> +	   ranges are used by the SoC
> +
> +ti,hwmods: Shall contain the TI interconnect module name if needed
> +	   by the SoC
> +
> +
> +Example:
> +	gpu: gpu@56000000 {
> +		compatible = "ti,omap4-gpu";
> +		reg = <0x56000000 0x10000>;
> +		interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> +		ti,hwmods = "gpu";
> +	};
> diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
> --- a/arch/arm/boot/dts/omap4.dtsi
> +++ b/arch/arm/boot/dts/omap4.dtsi
> @@ -1086,6 +1086,13 @@
>  			status = "disabled";
>  		};
>  
> +		gpu: gpu@56000000 {
> +			compatible = "ti,omap4-gpu";
> +			reg = <0x56000000 0x10000>;
> +			interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> +			ti,hwmods = "gpu";
> +		};
> +
>  		dss: dss@58000000 {
>  			compatible = "ti,omap4-dss";
>  			reg = <0x58000000 0x80>;
> -- 
> 2.14.1
> --
> 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
Tomi Valkeinen Aug. 29, 2017, 11:35 a.m. UTC | #2
This message contains a digitally signed email which can be read by opening the attachment.
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 29/08/17 12:00, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, Aug 28, 2017 at 02:19:15PM -0700, Tony Lindgren wrote:
>> On omap4 we're missing the PowerVR SGX GPU node with it's related
>> "ti,hwmods" property that the SoC interconnect code needs.
>>
>> Note that this will only show up as a bug with "doesn't have
>> mpu register target base" boot errors when the legacy platform
>> data is removed.
>>
>> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>> ---
> 
> I think OMAP3 & OMAP5 should also be documented and getting a
> node in this series?

Do we even want to add SGX to the .dts? We don't have proper drivers for
SGX. If we ever do, who knows what kind of DT data they need. I know the
DT data for SGX in TI's kernel tree has changed at least once.

 Tomi
Sebastian Reichel Aug. 29, 2017, 12:10 p.m. UTC | #3
Hi,

On Tue, Aug 29, 2017 at 02:35:09PM +0300, Tomi Valkeinen wrote:
> This message contains a digitally signed email which can be read by opening the attachment.
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

> Date: Tue, 29 Aug 2017 14:35:09 +0300
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> To: Sebastian Reichel <sebastian.reichel@collabora.co.uk>, Tony Lindgren
>  <tony@atomide.com>
> CC: linux-omap@vger.kernel.org, Benoît Cousson <bcousson@baylibre.com>,
>  devicetree@vger.kernel.org
> Subject: Re: [PATCH 14/17] ARM: dts: Add missing gpu node and binding for
>  omap4
> 
> On 29/08/17 12:00, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Mon, Aug 28, 2017 at 02:19:15PM -0700, Tony Lindgren wrote:
> >> On omap4 we're missing the PowerVR SGX GPU node with it's related
> >> "ti,hwmods" property that the SoC interconnect code needs.
> >>
> >> Note that this will only show up as a bug with "doesn't have
> >> mpu register target base" boot errors when the legacy platform
> >> data is removed.
> >>
> >> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >> Signed-off-by: Tony Lindgren <tony@atomide.com>
> >> ---
> > 
> > I think OMAP3 & OMAP5 should also be documented and getting a
> > node in this series?
> 
> Do we even want to add SGX to the .dts? We don't have proper drivers for
> SGX. If we ever do, who knows what kind of DT data they need. I know the
> DT data for SGX in TI's kernel tree has changed at least once.

I don't think reg or interrupts will be removed, so the properties
added by Tony look pretty safe?. I guess if we ever have a driver
it would need some more properties and would bail out. Having no
DT data is does not load at all, the result is the same. OTOH having
the node means the kernel can properly send the module to idle.

I think this patchset should Cc Rob and Mark. 

-- Sebastian
Tomi Valkeinen Aug. 29, 2017, 12:24 p.m. UTC | #4

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 29/08/17 15:10, Sebastian Reichel wrote:

>> Do we even want to add SGX to the .dts? We don't have proper drivers for
>> SGX. If we ever do, who knows what kind of DT data they need. I know the
>> DT data for SGX in TI's kernel tree has changed at least once.
> 
> I don't think reg or interrupts will be removed, so the properties
> added by Tony look pretty safe?. I guess if we ever have a driver

Maybe. At the moment we have this in TI's tree for DRA7:

gpu: gpu@56000000 {
	compatible = "ti,dra7-sgx544", "img,sgx544";
	reg = <0x56000000 0x10000>;
	reg-names = "gpu_ocp_base";
	interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
	ti,hwmods = "gpu";
	clocks = <&l3_iclk_div>, <&gpu_core_gclk_mux>,
		<&gpu_hyd_gclk_mux>;
	clock-names = "iclk", "fclk1", "fclk2";
};

> it would need some more properties and would bail out. Having no
> DT data is does not load at all, the result is the same. OTOH having
> the node means the kernel can properly send the module to idle.

I just get uneasy when adding DT data that we're not really sure if it's
ok or not. I've been fighting with such data for ages. But, as you say,
this probably won't matter if the driver will just reject DT data that
doesn't have all the details.

If we need the DT node to idle SGX, and we don't even mean to actually
use an SGX driver with this data, it sounds fine to me.

 Tomi

--
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
Tomi Valkeinen Aug. 29, 2017, 12:27 p.m. UTC | #5

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 29/08/17 15:10, Sebastian Reichel wrote:

>> Do we even want to add SGX to the .dts? We don't have proper drivers for
>> SGX. If we ever do, who knows what kind of DT data they need. I know the
>> DT data for SGX in TI's kernel tree has changed at least once.
> 
> I don't think reg or interrupts will be removed, so the properties
> added by Tony look pretty safe?. I guess if we ever have a driver

Oh, and one more thing about the regs. I believe SGX consists of
multiple register blocks. But as it's not documented in any public docs,
there's just that single block in the DT data. I'm not sure if single
block or multiple blocks would be the right approach, but then, I guess
we can always live with just a single block and if needed split the
blocks inside the driver.

 Tomi

--
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
Tony Lindgren Aug. 29, 2017, 2:34 p.m. UTC | #6
* Tomi Valkeinen <tomi.valkeinen@ti.com> [170829 05:25]:
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> On 29/08/17 15:10, Sebastian Reichel wrote:
> 
> >> Do we even want to add SGX to the .dts? We don't have proper drivers for
> >> SGX. If we ever do, who knows what kind of DT data they need. I know the
> >> DT data for SGX in TI's kernel tree has changed at least once.
> > 
> > I don't think reg or interrupts will be removed, so the properties
> > added by Tony look pretty safe?. I guess if we ever have a driver
> 
> Maybe. At the moment we have this in TI's tree for DRA7:
> 
> gpu: gpu@56000000 {
> 	compatible = "ti,dra7-sgx544", "img,sgx544";
> 	reg = <0x56000000 0x10000>;
> 	reg-names = "gpu_ocp_base";
> 	interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> 	ti,hwmods = "gpu";
> 	clocks = <&l3_iclk_div>, <&gpu_core_gclk_mux>,
> 		<&gpu_hyd_gclk_mux>;
> 	clock-names = "iclk", "fclk1", "fclk2";
> };
> 
> > it would need some more properties and would bail out. Having no
> > DT data is does not load at all, the result is the same. OTOH having
> > the node means the kernel can properly send the module to idle.
> 
> I just get uneasy when adding DT data that we're not really sure if it's
> ok or not. I've been fighting with such data for ages. But, as you say,
> this probably won't matter if the driver will just reject DT data that
> doesn't have all the details.

The above example looks OK to me, the interrupt is different for omap4.

> If we need the DT node to idle SGX, and we don't even mean to actually
> use an SGX driver with this data, it sounds fine to me.

Well ideally the SGX driver will make use of it too. Let me know
if you want to leave out some parts of the above example from
TI tree.

Regards,

Tony
--
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
Tony Lindgren Aug. 29, 2017, 2:37 p.m. UTC | #7
* Tomi Valkeinen <tomi.valkeinen@ti.com> [170829 05:28]:
> 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> On 29/08/17 15:10, Sebastian Reichel wrote:
> 
> >> Do we even want to add SGX to the .dts? We don't have proper drivers for
> >> SGX. If we ever do, who knows what kind of DT data they need. I know the
> >> DT data for SGX in TI's kernel tree has changed at least once.
> > 
> > I don't think reg or interrupts will be removed, so the properties
> > added by Tony look pretty safe?. I guess if we ever have a driver
> 
> Oh, and one more thing about the regs. I believe SGX consists of
> multiple register blocks. But as it's not documented in any public docs,
> there's just that single block in the DT data. I'm not sure if single
> block or multiple blocks would be the right approach, but then, I guess
> we can always live with just a single block and if needed split the
> blocks inside the driver.

OK, those register blocks can easily be child nodes of the module
if needed. This is for the parent interconnect target module
revc/sysc/syss registers that we already have mapped in legacy
platform data for the interconnect code.

Regards,

Tony
--
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
Tony Lindgren Aug. 29, 2017, 2:42 p.m. UTC | #8
* Sebastian Reichel <sebastian.reichel@collabora.co.uk> [170829 02:01]:
> Hi,
> 
> On Mon, Aug 28, 2017 at 02:19:15PM -0700, Tony Lindgren wrote:
> > On omap4 we're missing the PowerVR SGX GPU node with it's related
> > "ti,hwmods" property that the SoC interconnect code needs.
> > 
> > Note that this will only show up as a bug with "doesn't have
> > mpu register target base" boot errors when the legacy platform
> > data is removed.
> 
> I think OMAP3 & OMAP5 should also be documented and getting a
> node in this series?

Looks like we don't currently have any interconnect data defined
for those, but yeah sure I can add those.

Regards,

Tony
--
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
Adam Ford Aug. 29, 2017, 3:31 p.m. UTC | #9
On Tue, Aug 29, 2017 at 9:42 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Sebastian Reichel <sebastian.reichel@collabora.co.uk> [170829 02:01]:
>> Hi,
>>
>> On Mon, Aug 28, 2017 at 02:19:15PM -0700, Tony Lindgren wrote:
>> > On omap4 we're missing the PowerVR SGX GPU node with it's related
>> > "ti,hwmods" property that the SoC interconnect code needs.
>> >
>> > Note that this will only show up as a bug with "doesn't have
>> > mpu register target base" boot errors when the legacy platform
>> > data is removed.
>>
>> I think OMAP3 & OMAP5 should also be documented and getting a
>> node in this series?
>
> Looks like we don't currently have any interconnect data defined
> for those, but yeah sure I can add those.
>

If there is anything I can test, I had modified the SGX driver for the
OMAP36, but abandoned my attempts since much of the device tree and
reset stuff was missing.  I'd love to see SGX work again.  :-)

adam

> Regards,
>
> Tony
> --
> 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
--
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
Tony Lindgren Aug. 29, 2017, 3:35 p.m. UTC | #10
* Adam Ford <aford173@gmail.com> [170829 08:31]:
> On Tue, Aug 29, 2017 at 9:42 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Sebastian Reichel <sebastian.reichel@collabora.co.uk> [170829 02:01]:
> >> Hi,
> >>
> >> On Mon, Aug 28, 2017 at 02:19:15PM -0700, Tony Lindgren wrote:
> >> > On omap4 we're missing the PowerVR SGX GPU node with it's related
> >> > "ti,hwmods" property that the SoC interconnect code needs.
> >> >
> >> > Note that this will only show up as a bug with "doesn't have
> >> > mpu register target base" boot errors when the legacy platform
> >> > data is removed.
> >>
> >> I think OMAP3 & OMAP5 should also be documented and getting a
> >> node in this series?
> >
> > Looks like we don't currently have any interconnect data defined
> > for those, but yeah sure I can add those.
> >
> 
> If there is anything I can test, I had modified the SGX driver for the
> OMAP36, but abandoned my attempts since much of the device tree and
> reset stuff was missing.  I'd love to see SGX work again.  :-)

Well the resets can be handled using platform data callbacks
for now with pdata-quirks.c. Then when the reset driver is available,
we can just remove the platform data.

And maybe push your changes to some tree so others can participate?
I'm sure the n900 users would like to see it working too.

Regards,

Tony
--
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
Tony Lindgren Aug. 29, 2017, 3:57 p.m. UTC | #11
* Tony Lindgren <tony@atomide.com> [170829 07:35]:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [170829 05:25]:
> > Maybe. At the moment we have this in TI's tree for DRA7:
> > 
> > gpu: gpu@56000000 {
> > 	compatible = "ti,dra7-sgx544", "img,sgx544";

I'll leave out "img,sgx544" as that's the generic component
that's really a child of this interconnect target module.

Regards,

Tony
--
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
Tony Lindgren Aug. 29, 2017, 4:26 p.m. UTC | #12
* Tony Lindgren <tony@atomide.com> [170829 08:58]:
> * Tony Lindgren <tony@atomide.com> [170829 07:35]:
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [170829 05:25]:
> > > Maybe. At the moment we have this in TI's tree for DRA7:
> > > 
> > > gpu: gpu@56000000 {
> > > 	compatible = "ti,dra7-sgx544", "img,sgx544";
> 
> I'll leave out "img,sgx544" as that's the generic component
> that's really a child of this interconnect target module.

And for the gpu clock, on omap4, it seems that the clocks
are just the module clktrl with a mux option. So the top
level module driver implementing runtime PM should be
enough there. And on dra7, there are two functional clocks
maybe as the SGX instance is dual core. And the mainline
kernel is using "fck" naming vs "fclk" naming. So more
consideration is for the clocks and I'll leave them out
for now.

Regards,

Tony
--
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/Documentation/devicetree/bindings/gpu/ti-powervr-sgx.txt b/Documentation/devicetree/bindings/gpu/ti-powervr-sgx.txt
new file mode 100644
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/ti-powervr-sgx.txt
@@ -0,0 +1,34 @@ 
+Texas Instruments PowevVR SGX binding
+
+SGX can be used for graphics acceleration on Texas Instruments SoCs.
+
+Note that the SGX binding is currently only used by the SoC interconnect
+code to idle the module on init and no open source driver is available
+for SGX. Please update this documentation if that changes.
+
+Required properties:
+
+compatible: Shall be one of the following:
+	    "ti,omap4-gpu"
+
+reg: Shall contain the device instance IO range
+
+interrupts: Shall contain the device instance interrupt
+
+
+Optional properties:
+
+reg-names: Shall contain the IO range names if multiple IO
+	   ranges are used by the SoC
+
+ti,hwmods: Shall contain the TI interconnect module name if needed
+	   by the SoC
+
+
+Example:
+	gpu: gpu@56000000 {
+		compatible = "ti,omap4-gpu";
+		reg = <0x56000000 0x10000>;
+		interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+		ti,hwmods = "gpu";
+	};
diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi
--- a/arch/arm/boot/dts/omap4.dtsi
+++ b/arch/arm/boot/dts/omap4.dtsi
@@ -1086,6 +1086,13 @@ 
 			status = "disabled";
 		};
 
+		gpu: gpu@56000000 {
+			compatible = "ti,omap4-gpu";
+			reg = <0x56000000 0x10000>;
+			interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
+			ti,hwmods = "gpu";
+		};
+
 		dss: dss@58000000 {
 			compatible = "ti,omap4-dss";
 			reg = <0x58000000 0x80>;