ARM: DTS: OMAP: add child nodes describing the PVRSGX present in some OMAP SoC
Subsystems (3)
cover ARM: DTS: OMAP: add child nodes describing the PVRSGX present in some OMAP SoC
This patch set defines child nodes for the SGX5xx interface inside the OMAP SoC so that a driver can be found and probed by the compatible strings and can retrieve information about the SGX revision that is included in a specific SoC. It also defines the interrupt number and the timer to be used by the SGX driver.
There is currently no mainline driver for these GPUs, but a project [1] is ongoing with the goal to get the open-source part as provided by TI/IMG into drivers/staging/pvr.
The kernel modules built from this project have successfully demonstrated to work with the DTS definitions from this patch set on AM335x BeagleBone Black and OMAP5 Pyra. They partially works on DM3730 and PandaBoard ES but that is likely a problem in the kernel driver or the (non-free) user-space blobs.
There is potential to extend this work to JZ4780 (CI20 board) and BananaPi-M3 (A83) and even some Intel Poulsbo and CedarView devices.
[1]: https://github.com/openpvrsgx-devgroup
H. Nikolaus Schaller (7):
dt-bindings: gpu: pvrsgx: add initial bindings
ARM: DTS: am33xx: add sgx gpu child node
ARM: DTS: am3517: add sgx gpu child node
ARM: DTS: omap3: add sgx gpu child node
ARM: DTS: omap36xx: add sgx gpu child node
ARM: DTS: omap4: add sgx gpu child node
ARM: DTS: omap5: add sgx gpu child node
.../devicetree/bindings/gpu/img,pvrsgx.txt | 76 +++++++++++++++++++ arch/arm/boot/dts/am33xx.dtsi | 11 ++- arch/arm/boot/dts/am3517.dtsi | 13 ++-- arch/arm/boot/dts/omap34xx.dtsi | 13 ++-- arch/arm/boot/dts/omap36xx.dtsi | 13 ++-- arch/arm/boot/dts/omap4.dtsi | 11 ++- arch/arm/boot/dts/omap4470.dts | 16 ++++ arch/arm/boot/dts/omap5.dtsi | 12 ++- 8 files changed, 138 insertions(+), 27 deletions(-) create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.txt create mode 100644 arch/arm/boot/dts/omap4470.dts
Signature · 1 line
2.19.1
PATCH 1/7 dt-bindings: gpu: pvrsgx: add initial bindings
The Imagination PVR/SGX GPU is part of several SoC from multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo and others.
Here we describe how the SGX processor is interfaced to the SoC (registers, interrupt etc.).
Clock, Reset and power management should be handled by the parent node.
--- .../devicetree/bindings/gpu/img,pvrsgx.txt | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.txt
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +76 -0
@@ -0,0 +1,76 @@ +Imagination PVR/SGX GPU + +Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by this binding. + +Required properties: +- compatible: Should be one of + "img,sgx530-121", "img,sgx530", "ti,omap-omap3-sgx530-121"; + - BeagleBoard ABC, OpenPandora 600MHz + "img,sgx530-125", "img,sgx530", "ti,omap-omap3-sgx530-125"; + - BeagleBoard XM, GTA04, OpenPandora 1GHz + "img,sgx530-125", "img,sgx530", "ti,omap-am3517-sgx530-125"; + "img,sgx530-125", "img,sgx530", "ti,omap-am335x-sgx530-125"; + - BeagleBone Black + "img,sgx540-120", "img,sgx540", "ti,omap-omap4-sgx540-120"; + - Pandaboard (ES) + "img,sgx544-112", "img,sgx544", "ti,omap-omap4-sgx544-112"; + "img,sgx544-116", "img,sgx544", "ti,omap-omap5-sgx544-116"; + - OMAP5 UEVM, Pyra Handheld + "img,sgx544-116", "img,sgx544", "ti,omap-dra7-sgx544-116"; + + For further study: + "ti,omap-am3517-sgx530-?" + "ti,omap-am43xx-sgx530-?" + "ti,ti43xx-sgx" + "ti,ti81xx-sgx" + "img,jz4780-sgx5??-?" + "intel,poulsbo-sgx?" + "intel,cedarview-sgx?" + "sunxi,sgx-544-?" - Banana-Pi-M3 (Allwinner A83T) + + The "ti,omap..." entries are needed temporarily to handle SoC + specific builds of the kernel module. + + In the long run, only the "img,sgx..." entry should suffice + to match a generic driver for all architectures and driver + code can dynamically find out on which SoC it is running. + + +- reg: Physical base addresses and lengths of the register areas. +- reg-names: Names for the register areas. +- interrupts: The interrupt numbers. + +Optional properties: +- timer: the timer to be used by the driver. +- img,cores: number of cores. Defaults to <1>. + +/ { + ocp { + sgx_module: target-module@56000000 { + compatible = "ti,sysc-omap4", "ti,sysc"; + reg = <0x5600fe00 0x4>, + <0x5600fe10 0x4>; + reg-names = "rev", "sysc"; + ti,sysc-midle = <SYSC_IDLE_FORCE>, + <SYSC_IDLE_NO>, + <SYSC_IDLE_SMART>; + ti,sysc-sidle = <SYSC_IDLE_FORCE>, + <SYSC_IDLE_NO>, + <SYSC_IDLE_SMART>; + clocks = <&gpu_clkctrl OMAP5_GPU_CLKCTRL 0>; + clock-names = "fck"; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x56000000 0x2000000>; + + sgx@fe00 { + compatible = "img,sgx544-116", "img,sgx544", "ti,omap-omap5-sgx544-116"; + reg = <0xfe00 0x200>; + reg-names = "sgx"; + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; + timer = <&timer11>; + img,cores = <2>; + }; + }; + }; +};
Signature · 1 line
2.19.1
Re: [PATCH 1/7] dt-bindings: gpu: pvrsgx: add initial bindings
Quoted 10 lines · H. Nikolaus Schaller
The Imagination PVR/SGX GPU is part of several SoC from multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo and others.
Here we describe how the SGX processor is interfaced to the SoC (registers, interrupt etc.).
Clock, Reset and power management should be handled by the parent node.
That's TI specific.
Quoted 6 lines
Signed-off-by H. Nikolaus Schaller <[email protected]>--- .../devicetree/bindings/gpu/img,pvrsgx.txt | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.txt
Please make this DT schema format.
Quoted 15 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +9
-0@@ -0,0 +1,76 @@ +Imagination PVR/SGX GPU + +Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by this binding. + +Required properties: +- compatible: Should be one of + "img,sgx530-121", "img,sgx530", "ti,omap-omap3-sgx530-121"; + - BeagleBoard ABC, OpenPandora 600MHz + "img,sgx530-125", "img,sgx530", "ti,omap-omap3-sgx530-125";
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 19
Quoted 10 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +10
-0+ - BeagleBoard XM, GTA04, OpenPandora 1GHz + "img,sgx530-125", "img,sgx530", "ti,omap-am3517-sgx530-125"; + "img,sgx530-125", "img,sgx530", "ti,omap-am335x-sgx530-125"; + - BeagleBone Black + "img,sgx540-120", "img,sgx540", "ti,omap-omap4-sgx540-120"; + - Pandaboard (ES) + "img,sgx544-112", "img,sgx544", "ti,omap-omap4-sgx544-112"; + "img,sgx544-116", "img,sgx544", "ti,omap-omap5-sgx544-116"; + - OMAP5 UEVM, Pyra Handheld + "img,sgx544-116", "img,sgx544", "ti,omap-dra7-sgx544-116";
The order here is wrong. Should be most specific first.
Drop 'omap-' from the compatible.
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 29
Quoted 10 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +10
-0+ + For further study: + "ti,omap-am3517-sgx530-?" + "ti,omap-am43xx-sgx530-?" + "ti,ti43xx-sgx" + "ti,ti81xx-sgx" + "img,jz4780-sgx5??-?" + "intel,poulsbo-sgx?" + "intel,cedarview-sgx?" + "sunxi,sgx-544-?" - Banana-Pi-M3 (Allwinner A83T)
Just drop these.
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 36
Quoted 7 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +7
-0+ + The "ti,omap..." entries are needed temporarily to handle SoC + specific builds of the kernel module. + + In the long run, only the "img,sgx..." entry should suffice + to match a generic driver for all architectures and driver + code can dynamically find out on which SoC it is running.
Drop this. Which compatible an OS matches on is not relevant to the binding. And 'temporarily' is wrong as the SoC specific compatible strings are what are used for handling errata or other integration specific things.
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 39
Quoted 3 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +3
-0+ + +- reg: Physical base addresses and lengths of the register areas.
How many?
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 40
Quoted 1 line
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +1
-0+- reg-names: Names for the register areas.
If only 1 as the example suggests, then you don't need this.
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 44
Quoted 4 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +4
-0+- interrupts: The interrupt numbers. + +Optional properties: +- timer: the timer to be used by the driver.
Needs a better description and vendor prefix at least.
Why is this needed rather than using the OS's timers?
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 45
Quoted 1 line
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +1
-0+- img,cores: number of cores. Defaults to <1>.
Not discoverable?
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 49
Quoted 4 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +4
-0+ +/ { + ocp { + sgx_module: target-module@56000000 {
This is TI specific and this binding covers other chips in theory at least. This part is outside the scope
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 52
Quoted 3 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +3
-0+ compatible = "ti,sysc-omap4", "ti,sysc"; + reg = <0x5600fe00 0x4>, + <0x5600fe10 0x4>;
How does it work that these registers overlap the GPU registers?
Quoted 4 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +4
-0+ reg-names = "rev", "sysc"; + ti,sysc-midle = <SYSC_IDLE_FORCE>, + <SYSC_IDLE_NO>, + <SYSC_IDLE_SMART>;
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 66
Quoted 10 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +10
-0+ ti,sysc-sidle = <SYSC_IDLE_FORCE>, + <SYSC_IDLE_NO>, + <SYSC_IDLE_SMART>; + clocks = <&gpu_clkctrl OMAP5_GPU_CLKCTRL 0>; + clock-names = "fck"; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x56000000 0x2000000>; + + sgx@fe00 {
gpu@...
Quoted 13 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +10
-1+ compatible = "img,sgx544-116", "img,sgx544", "ti,omap-omap5-sgx544-116"; + reg = <0xfe00 0x200>; + reg-names = "sgx"; + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; + timer = <&timer11>; + img,cores = <2>; + }; + }; + }; +}; --2.19.1
Re: [PATCH 1/7] dt-bindings: gpu: pvrsgx: add initial bindings
Hi Rob,
Quoted 8 lines
Am 21.10.2019 um 17:07 schrieb Rob Herring <[email protected]>:
Quoted 5 lines · H. Nikolaus Schaller
The Imagination PVR/SGX GPU is part of several SoC from multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo and others.
Quoted 7 lines
Quoted 5 lines
Here we describe how the SGX processor is interfaced to the SoC (registers, interrupt etc.).
Clock, Reset and power management should be handled by the parent node.
That's TI specific.
Ok. Would this be better:
Clock, Reset and power management is not handled by this binding and can e.g. be described by the parent node.
Quoted 9 lines
Quoted 7 lines
Signed-off-by H. Nikolaus Schaller <[email protected]>---
.../devicetree/bindings/gpu/img,pvrsgx.txt| 76 +++++++++++++++++++1 file changed, 76 insertions(+)create mode 100644
Documentation/devicetree/bindings/gpu/img,pvrsgx.txtPlease make this DT schema format.
Is there a tutorial or a tool to convert?
I have only seen that a new format exists but zero experience.
Quoted 29 lines
Quoted 16 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +9
-0@@ -0,0 +1,76 @@ +Imagination PVR/SGX GPU + +Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by this binding. + +Required properties: +- compatible: Should be one of + "img,sgx530-121", "img,sgx530", "ti,omap-omap3-sgx530-121"; + - BeagleBoard ABC, OpenPandora 600MHz + "img,sgx530-125", "img,sgx530", "ti,omap-omap3-sgx530-125";Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 19
Quoted 10 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +10
-0+ - BeagleBoard XM, GTA04, OpenPandora 1GHz + "img,sgx530-125", "img,sgx530", "ti,omap-am3517-sgx530-125"; + "img,sgx530-125", "img,sgx530", "ti,omap-am335x-sgx530-125"; + - BeagleBone Black + "img,sgx540-120", "img,sgx540", "ti,omap-omap4-sgx540-120"; + - Pandaboard (ES) + "img,sgx544-112", "img,sgx544", "ti,omap-omap4-sgx544-112"; + "img,sgx544-116", "img,sgx544", "ti,omap-omap5-sgx544-116"; + - OMAP5 UEVM, Pyra Handheld + "img,sgx544-116", "img,sgx544", "ti,omap-dra7-sgx544-116";The order here is wrong. Should be most specific first.
Quoted 1 line
Drop 'omap-' from the compatible.
Ok, yes. Seems to be redundant since omap is from ti only...
Quoted 12 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 29
Quoted 11 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +10
-0+ + For further study: + "ti,omap-am3517-sgx530-?" + "ti,omap-am43xx-sgx530-?" + "ti,ti43xx-sgx" + "ti,ti81xx-sgx" + "img,jz4780-sgx5??-?" + "intel,poulsbo-sgx?" + "intel,cedarview-sgx?" + "sunxi,sgx-544-?" - Banana-Pi-M3 (Allwinner A83T)
Quoted 1 line
Just drop these.
Well, the driver code package we have seems to support them and the idea (dream?) is to make it a generic driver compatible to all of them.
So we could leave it out and add later (in the hope that it does not get forgotten).
Quoted 13 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 36
Quoted 8 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +7
-0+ + The "ti,omap..." entries are needed temporarily to handle SoC + specific builds of the kernel module. + + In the long run, only the "img,sgx..." entry should suffice + to match a generic driver for all architectures and driver + code can dynamically find out on which SoC it is running.Drop this. Which compatible an OS matches on is not relevant to the binding. And 'temporarily' is wrong as the SoC specific compatible strings are what are used for handling errata or other integration specific things.
The idea behind this is that a driver can finally find out by different means which SoC it is connected to.
At the moment we have to build different pvrsrvkm.ko for each one since there is no "generic" driver yet.
So in the long run only img,sgx... should be there. And even this might be boiled down to img,sgx5 (assuming that even 530/540/544) is detectable.
But at the moment we are not able to create working code without the mix of soc and sgx versioning.
Basically it boils down if we want a basis that works today and is prepared for tomorrow, or if we have to decide for either today or future and can't bridge between.
Quoted 6 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 39
Quoted 4 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +3
-0+ + +- reg: Physical base addresses and lengths of the register areas.How many?
I assume there is only one. At least it suffices to make the existing driver work with it.
Quoted 4 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 40
Quoted 2 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +1
-0+- reg-names: Names for the register areas.If only 1 as the example suggests, then you don't need this.
ok.
Quoted 7 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 44
Quoted 5 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +4
-0+- interrupts: The interrupt numbers. + +Optional properties: +- timer: the timer to be used by the driver.Needs a better description and vendor prefix at least.
I am not yet sure if it is vendor specific or if all SGX implementations need some timer.
Quoted 2 lines
Why is this needed rather than using the OS's timers?
Because nobody understands the current (out of tree and planned for staging) driver well enough what the timer is doing. It is currently hard coded that some omap refer to timer7 and others use timer11.
Quoted 4 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 45
Quoted 2 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +1
-0+- img,cores: number of cores. Defaults to <1>.Not discoverable?
Not sure if it is. This is probably available in undocumented registers of the sgx.
Quoted 8 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 49
Quoted 5 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +4
-0+ +/ { + ocp { + sgx_module: target-module@56000000 {This is TI specific and this binding covers other chips in theory at least. This part is outside the scope
Ok, it is the only example where we know that it works. So we do not yet know how the GPU integration would have to look like for e.g. CI20 or BananaPi M3 (which are not that well converted to device tree like OMAP). This project is quite at the beginning...
Quoted 6 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 52
Quoted 4 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +3
-0+ compatible = "ti,sysc-omap4", "ti,sysc"; + reg = <0x5600fe00 0x4>, + <0x5600fe10 0x4>;How does it work that these registers overlap the GPU registers?
Both drivers have access to these registers. Likely, the gpu driver ignores them and does access other ranges.
Quoted 16 lines
Quoted 5 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +4
-0+ reg-names = "rev", "sysc"; + ti,sysc-midle = <SYSC_IDLE_FORCE>, + <SYSC_IDLE_NO>, + <SYSC_IDLE_SMART>;Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 66
Quoted 10 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +10
-0+ ti,sysc-sidle = <SYSC_IDLE_FORCE>, + <SYSC_IDLE_NO>, + <SYSC_IDLE_SMART>; + clocks = <&gpu_clkctrl OMAP5_GPU_CLKCTRL 0>; + clock-names = "fck"; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0x56000000 0x2000000>; + + sgx@fe00 {
Quoted 1 line
gpu@...
Yes, is better to name it according to the function.
Quoted 15 lines
Quoted 14 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +10
-1+ compatible = "img,sgx544-116", "img,sgx544", "ti,omap-omap5-sgx544-116"; + reg = <0xfe00 0x200>; + reg-names = "sgx"; + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; + timer = <&timer11>; + img,cores = <2>; + }; + }; + }; +}; --Quoted 1 line
2.19.1
BR and thanks,
Nikolaus
Re: [PATCH 1/7] dt-bindings: gpu: pvrsgx: add initial bindings
Quoted 10 lines · H. Nikolaus Schaller
Quoted 5 lines
Am 21.10.2019 um 17:07 schrieb Rob Herring <[email protected]>:
Quoted 1 line · H. Nikolaus Schaller
+- reg: Physical base addresses and lengths of the register areas.
How many?
I assume there is only one. At least it suffices to make the existing driver work with it.
Quoted 1 line
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
+- reg-names: Names for the register areas.
If only 1 as the example suggests, then you don't need this.
ok.
My guess is that sgx is just a private interconnect instance with few control modules like mmu and clocks, and the driver(s) should consist of independent modules like iommu and clock driver.
So yeah I agree, it's best to leave reg names out of the dts at least for now.
Quoted 8 lines
Quoted 5 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 52
Quoted 3 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +3
-0+ compatible = "ti,sysc-omap4", "ti,sysc"; + reg = <0x5600fe00 0x4>, + <0x5600fe10 0x4>;How does it work that these registers overlap the GPU registers?
Both drivers have access to these registers. Likely, the gpu driver ignores them and does access other ranges.
Unfortunately TI is stuffing the interconnect target module control registers at random places within the unused register space of the child module(s). So the module control registers are all over the map at various offsets.
Adding holes for each module control register to the child nodes seems like an overkill to work around this IMO. Especially considering many drivers only understand one IO range currently.
Regards,
Tony
Re: [PATCH 1/7] dt-bindings: gpu: pvrsgx: add initial bindings
In reply to H. Nikolaus SchallerQuoted 11 lines · H. Nikolaus Schaller
Quoted 6 lines
Am 21.10.2019 um 17:07 schrieb Rob Herring <[email protected]>:
Quoted 2 lines · H. Nikolaus Schaller
+Optional properties:
+- timer: the timer to be used by the driver.Needs a better description and vendor prefix at least.
I am not yet sure if it is vendor specific or if all SGX implementations need some timer.
Quoted 1 line
Quoted 6 lines
Quoted 1 line
Why is this needed rather than using the OS's timers?
Because nobody understands the current (out of tree and planned for staging) driver well enough what the timer is doing. It is currently hard coded that some omap refer to timer7 and others use timer11.
Just configure it in the driver based on the compatible value to keep it out of the dts. It's best to stick to standard bindings.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
+- img,cores: number of cores. Defaults to <1>.
Not discoverable?
Not sure if it is. This is probably available in undocumented registers of the sgx.
This too, and whatever non-standrd other properities you might have.
Regards,
Tony
Re: [PATCH 1/7] dt-bindings: gpu: pvrsgx: add initial bindings
Quoted 17 lines
Am 21.10.2019 um 19:25 schrieb Tony Lindgren <[email protected]>:
Quoted 13 lines · H. Nikolaus Schaller
Quoted 6 lines
Am 21.10.2019 um 17:07 schrieb Rob Herring <[email protected]>:
Quoted 2 lines · H. Nikolaus Schaller
+Optional properties:
+- timer: the timer to be used by the driver.Needs a better description and vendor prefix at least.
I am not yet sure if it is vendor specific or if all SGX implementations need some timer.
Quoted 2 lines
Why is this needed rather than using the OS's timers?
Quoted 8 lines
Quoted 4 lines
Because nobody understands the current (out of tree and planned for staging) driver well enough what the timer is doing. It is currently hard coded that some omap refer to timer7 and others use timer11.
Just configure it in the driver based on the compatible value to keep it out of the dts. It's best to stick to standard bindings.
IMHO leads to ugly code... Since the timer is not part of the SGX IPR module but one of the OMAP timers it is sort of hardware connection that can be chosen a little arbitrarily.
This is the main reason why I think adding it to a device tree source so that a board that really requires to use a timer for a different purpose, can reassign it. This is not possible if we hard-code that into the driver by scanning for compatible. In that case the driver must check board compatible names...
But if we gain a better understanding of its role in the driver (does it really need a dedicated timer and for what and which properties the timer must have) we can probably replace it.
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
+- img,cores: number of cores. Defaults to <1>.
Not discoverable?
Not sure if it is. This is probably available in undocumented registers of the sgx.
This too, and whatever non-standrd other properities you might have.
Here it is a feature of the SGX IPR of the SoC, i.e. describes that the hardware has one or two cores.
BR,
NIkolaus
Re: [PATCH 1/7] dt-bindings: gpu: pvrsgx: add initial bindings
Quoted 37 lines · H. Nikolaus Schaller
Quoted 17 lines
Am 21.10.2019 um 19:25 schrieb Tony Lindgren <[email protected]>:
Quoted 13 lines · H. Nikolaus Schaller
Quoted 6 lines
Am 21.10.2019 um 17:07 schrieb Rob Herring <[email protected]>:
Quoted 2 lines · H. Nikolaus Schaller
+Optional properties:
+- timer: the timer to be used by the driver.Needs a better description and vendor prefix at least.
I am not yet sure if it is vendor specific or if all SGX implementations need some timer.
Quoted 2 lines
Why is this needed rather than using the OS's timers?
Quoted 8 lines
Quoted 4 lines
Because nobody understands the current (out of tree and planned for staging) driver well enough what the timer is doing. It is currently hard coded that some omap refer to timer7 and others use timer11.
Just configure it in the driver based on the compatible value to keep it out of the dts. It's best to stick to standard bindings.
IMHO leads to ugly code... Since the timer is not part of the SGX IPR module but one of the OMAP timers it is sort of hardware connection that can be chosen a little arbitrarily.
This is the main reason why I think adding it to a device tree source so that a board that really requires to use a timer for a different purpose, can reassign it. This is not possible if we hard-code that into the driver by scanning for compatible. In that case the driver must check board compatible names...
Quoted 3 lines
But if we gain a better understanding of its role in the driver (does it really need a dedicated timer and for what and which properties the timer must have) we can probably replace it.
Well how about just leave out the timer from the binding for now, and just carry a patch for it until it is known if/why it's really needed?
If it's needed, yeah I agree a timer property should be used.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
+- img,cores: number of cores. Defaults to <1>.
Not discoverable?
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Not sure if it is. This is probably available in undocumented registers of the sgx.
This too, and whatever non-standrd other properities you might have.
Here it is a feature of the SGX IPR of the SoC, i.e. describes that the hardware has one or two cores.
Here you can have a standard dts binding by putting this into driver struct of_device_id match .data. Then when somebody figures out how to read that from the hardware, it can be just dropped.
Regards,
Tony
Re: [PATCH 1/7] dt-bindings: gpu: pvrsgx: add initial bindings
Hi Tony,
Quoted 44 lines
Am 22.10.2019 um 17:02 schrieb Tony Lindgren <[email protected]>:
Quoted 37 lines · H. Nikolaus Schaller
Quoted 17 lines
Am 21.10.2019 um 19:25 schrieb Tony Lindgren <[email protected]>:
Quoted 13 lines · H. Nikolaus Schaller
Quoted 6 lines
Am 21.10.2019 um 17:07 schrieb Rob Herring <[email protected]>:
Quoted 2 lines · H. Nikolaus Schaller
+Optional properties:
+- timer: the timer to be used by the driver.Needs a better description and vendor prefix at least.
I am not yet sure if it is vendor specific or if all SGX implementations need some timer.
Quoted 2 lines
Why is this needed rather than using the OS's timers?
Quoted 8 lines
Quoted 4 lines
Because nobody understands the current (out of tree and planned for staging) driver well enough what the timer is doing. It is currently hard coded that some omap refer to timer7 and others use timer11.
Just configure it in the driver based on the compatible value to keep it out of the dts. It's best to stick to standard bindings.
IMHO leads to ugly code... Since the timer is not part of the SGX IPR module but one of the OMAP timers it is sort of hardware connection that can be chosen a little arbitrarily.
This is the main reason why I think adding it to a device tree source so that a board that really requires to use a timer for a different purpose, can reassign it. This is not possible if we hard-code that into the driver by scanning for compatible. In that case the driver must check board compatible names...
Quoted 3 lines
But if we gain a better understanding of its role in the driver (does it really need a dedicated timer and for what and which properties the timer must have) we can probably replace it.
Quoted 6 lines
Well how about just leave out the timer from the binding for now, and just carry a patch for it until it is known if/why it's really needed?
If it's needed, yeah I agree a timer property should be used.
Ok, fine. I'll split the bindings into a patch without and keep a private patch to add this for our development tree. Then we either need it or drop it.
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
+- img,cores: number of cores. Defaults to <1>.
Not discoverable?
Not sure if it is. This is probably available in undocumented registers of the sgx.
This too, and whatever non-standrd other properities you might have.
Quoted 7 lines
Quoted 2 lines
Here it is a feature of the SGX IPR of the SoC, i.e. describes that the hardware has one or two cores.
Here you can have a standard dts binding by putting this into driver struct
of_device_idmatch .data. Then when somebody figures out how to read that from the hardware, it can be just dropped.
Hm. How should that work? Some SoC have the sgx544 as single core and others as dual core. This imho does not fit into the "img,sgx544-$revision" scheme which could be matched to.
But maybe we do it the same as with the timer for the moment, i.e. keep it in some unpublished patch set.
At the moment I have more problems understanding how the yaml thing works. I understand and fully support the overall goal, but it is quite difficult to get a start here. And there do not seem to be tools or scripts to help converting from old style text format (even if not perfect, this would be helpful) and I have no yaml editor that helps keeping the indentation correct. So this slows down a v2 a little bit.
BR and thanks,
Nikolaus
Re: [PATCH 1/7] dt-bindings: gpu: pvrsgx: add initial bindings
Quoted 3 lines · H. Nikolaus Schaller
Hm. How should that work? Some SoC have the sgx544 as single core and others as dual core. This imho does not fit into the "img,sgx544-$revision" scheme which could be matched to.
Well don't you have then just two separate child nodes, one for each core with their own register range?
That is assuming they're really separate instances..
Quoted 2 lines
But maybe we do it the same as with the timer for the moment, i.e. keep it in some unpublished patch set.
Yeah makes sense to me until things get sorted out.
Quoted 7 lines
At the moment I have more problems understanding how the yaml thing works. I understand and fully support the overall goal, but it is quite difficult to get a start here. And there do not seem to be tools or scripts to help converting from old style text format (even if not perfect, this would be helpful) and I have no yaml editor that helps keeping the indentation correct. So this slows down a v2 a little bit.
Sounds handy to me :)
Regards,
Tony
Re: [PATCH 1/7] dt-bindings: gpu: pvrsgx: add initial bindings
Quoted 10 lines
Am 22.10.2019 um 17:36 schrieb Tony Lindgren <[email protected]>:
Quoted 3 lines · H. Nikolaus Schaller
Hm. How should that work? Some SoC have the sgx544 as single core and others as dual core. This imho does not fit into the "img,sgx544-$revision" scheme which could be matched to.
Well don't you have then just two separate child nodes, one for each core with their own register range?
Doesn't look so. AFAIK the architecture of SGX is that there is a single scheduler which is accessed by the register range and it internally has control over multiple cores.
Quoted 2 lines
That is assuming they're really separate instances..
No. There is some internal magic in the driver. It just needs to know that there is one or two nodes. Currently this is handled by some #define option when calling the inner Makefile.
My idea was to replace the #ifdef by checking for the img,cores property.
Quoted 19 lines
Quoted 3 lines
But maybe we do it the same as with the timer for the moment, i.e. keep it in some unpublished patch set.
Yeah makes sense to me until things get sorted out.
Quoted 7 lines
At the moment I have more problems understanding how the yaml thing works. I understand and fully support the overall goal, but it is quite difficult to get a start here. And there do not seem to be tools or scripts to help converting from old style text format (even if not perfect, this would be helpful) and I have no yaml editor that helps keeping the indentation correct. So this slows down a v2 a little bit.
Sounds handy to me :)
Regards,
Tony
Re: [PATCH 1/7] dt-bindings: gpu: pvrsgx: add initial bindings
In reply to H. Nikolaus SchallerQuoted 12 lines · H. Nikolaus Schaller
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +9
-0@@ -0,0 +1,76 @@ +Imagination PVR/SGX GPU + +Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by this binding. + +Required properties: +- compatible: Should be one of + "img,sgx530-121", "img,sgx530", "ti,omap-omap3-sgx530-121"; + - BeagleBoard ABC, OpenPandora 600MHz + "img,sgx530-125", "img,sgx530", "ti,omap-omap3-sgx530-125";
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 19
Quoted 10 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +10
-0+ - BeagleBoard XM, GTA04, OpenPandora 1GHz + "img,sgx530-125", "img,sgx530", "ti,omap-am3517-sgx530-125"; + "img,sgx530-125", "img,sgx530", "ti,omap-am335x-sgx530-125"; + - BeagleBone Black + "img,sgx540-120", "img,sgx540", "ti,omap-omap4-sgx540-120"; + - Pandaboard (ES) + "img,sgx544-112", "img,sgx544", "ti,omap-omap4-sgx544-112"; + "img,sgx544-116", "img,sgx544", "ti,omap-omap5-sgx544-116"; + - OMAP5 UEVM, Pyra Handheld + "img,sgx544-116", "img,sgx544", "ti,omap-dra7-sgx544-116";
FYI, the compatible names above have unnecessary omap in them:
"ti,omap-omap3-sgx530-121" should be "ti,omap3-sgx530-121" "ti,omap-am335x-sgx530-125" should be "ti,am335x-sgx530-125"; "ti,omap-dra7-sgx544-116" should be "ti,dra7-sgx544-116"
And so on.
Regards,
Tony
Re: [PATCH 1/7] dt-bindings: gpu: pvrsgx: add initial bindings
Hi,
Quoted 28 lines
Am 30.10.2019 um 17:16 schrieb Tony Lindgren <[email protected]>:
Quoted 12 lines · H. Nikolaus Schaller
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +9
-0@@ -0,0 +1,76 @@ +Imagination PVR/SGX GPU + +Only the Imagination SGX530, SGX540 and SGX544 GPUs are currently covered by this binding. + +Required properties: +- compatible: Should be one of + "img,sgx530-121", "img,sgx530", "ti,omap-omap3-sgx530-121"; + - BeagleBoard ABC, OpenPandora 600MHz + "img,sgx530-125", "img,sgx530", "ti,omap-omap3-sgx530-125";Documentation/devicetree/bindings/gpu/img,pvrsgx.txt line 19
Quoted 10 lines
Documentation/devicetree/bindings/gpu/img,pvrsgx.txt new file +10
-0+ - BeagleBoard XM, GTA04, OpenPandora 1GHz + "img,sgx530-125", "img,sgx530", "ti,omap-am3517-sgx530-125"; + "img,sgx530-125", "img,sgx530", "ti,omap-am335x-sgx530-125"; + - BeagleBone Black + "img,sgx540-120", "img,sgx540", "ti,omap-omap4-sgx540-120"; + - Pandaboard (ES) + "img,sgx544-112", "img,sgx544", "ti,omap-omap4-sgx544-112"; + "img,sgx544-116", "img,sgx544", "ti,omap-omap5-sgx544-116"; + - OMAP5 UEVM, Pyra Handheld + "img,sgx544-116", "img,sgx544", "ti,omap-dra7-sgx544-116";FYI, the compatible names above have unnecessary omap in them:
Quoted 5 lines
"ti,omap-omap3-sgx530-121" should be "ti,omap3-sgx530-121" "ti,omap-am335x-sgx530-125" should be "ti,am335x-sgx530-125"; "ti,omap-dra7-sgx544-116" should be "ti,dra7-sgx544-116"
And so on.
Yes,
Rob already noted a while ago and our latest private code has it fixed.
There is no progress towards a v2 since I am still fighting with the new yaml format he also requested...
BR and thanks,
Nikolaus
PATCH 2/7 ARM: DTS: am33xx: add sgx gpu child node
In reply to H. Nikolaus Schallerand add timer and interrupt.
Tested on BeagleBone Black.
--- arch/arm/boot/dts/am33xx.dtsi | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
arch/arm/boot/dts/am33xx.dtsi +7 -4
@@ -480,10 +480,13 @@ #size-cells = <1>; ranges = <0 0x56000000 0x1000000>; - /* - * Closed source PowerVR driver, no child device - * binding or driver in mainline - */ + sgx: sgx@0 { + compatible = "img,sgx530-125", "img,sgx530", "ti,omap-am335x-sgx530-125"; + reg = <0x00 0x1000000>; /* 16 MB */ + reg-names = "sgx"; + interrupts = <37>; + timer = <&timer7>; + }; }; }; };
Signature · 1 line
2.19.1
PATCH 3/7 ARM: DTS: am3517: add sgx gpu child node
In reply to H. Nikolaus Schallerand add timer and interrupt.
--- arch/arm/boot/dts/am3517.dtsi | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
arch/arm/boot/dts/am3517.dtsi +8 -5
@@ -97,7 +97,7 @@ * revision register instead of the unreadable OCP revision * register. */ - sgx_module: target-module@50000000 { + target-module@50000000 { compatible = "ti,sysc-omap2", "ti,sysc"; reg = <0x50000014 0x4>; reg-names = "rev";
@@ -107,10 +107,13 @@ #size-cells = <1>; ranges = <0 0x50000000 0x4000>; - /* - * Closed source PowerVR driver, no child device - * binding or driver in mainline - */ + sgx: sgx@0 { + compatible = "img,sgx530-125", "img,sgx530", "ti,omap-am3517-sgx530-125"; + reg = <0x0 0x4000>; + reg-names = "sgx"; + interrupts = <21>; + timer = <&timer11>; + }; }; }; };
Signature · 1 line
2.19.1
PATCH 4/7 ARM: DTS: omap3: add sgx gpu child node
In reply to H. Nikolaus Schallerand add timer and interrupt
Tested on OpenPandora 600 MHz.
--- arch/arm/boot/dts/omap34xx.dtsi | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
arch/arm/boot/dts/omap34xx.dtsi +8 -5
@@ -111,7 +111,7 @@ * are also different clocks, but we do not have any dts users * for it. */ - sgx_module: target-module@50000000 { + target-module@50000000 { compatible = "ti,sysc-omap2", "ti,sysc"; reg = <0x50000014 0x4>; reg-names = "rev";
@@ -121,10 +121,13 @@ #size-cells = <1>; ranges = <0 0x50000000 0x4000>; - /* - * Closed source PowerVR driver, no child device - * binding or driver in mainline - */ + sgx: sgx@0 { + compatible = "img,sgx530-121", "img,sgx530", "ti,omap-omap3-sgx530-121"; + reg = <0x0 0x4000>; /* 64kB */ + reg-names = "sgx"; + interrupts = <21>; + timer = <&timer11>; + }; }; };
Signature · 1 line
2.19.1
PATCH 5/7 ARM: DTS: omap36xx: add sgx gpu child node
In reply to H. Nikolaus Schallerand add timer and interrupt.
Tested on GTA04 and BeagleBoard XM.
--- arch/arm/boot/dts/omap36xx.dtsi | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
arch/arm/boot/dts/omap36xx.dtsi +8 -5
@@ -145,7 +145,7 @@ * "ti,sysc-omap4" type register with just sidle and midle bits * available while omap34xx has "ti,sysc-omap2" type sysconfig. */ - sgx_module: target-module@50000000 { + target-module@50000000 { compatible = "ti,sysc-omap4", "ti,sysc"; reg = <0x5000fe00 0x4>, <0x5000fe10 0x4>;
@@ -162,10 +162,13 @@ #size-cells = <1>; ranges = <0 0x50000000 0x2000000>; - /* - * Closed source PowerVR driver, no child device - * binding or driver in mainline - */ + sgx: sgx@0 { + compatible = "img,sgx530-125", "img,sgx530", "ti,omap-omap3-sgx530-125"; + reg = <0x0 0x10000>; /* 64kB */ + reg-names = "sgx"; + interrupts = <21>; + timer = <&timer11>; + }; }; };
Signature · 1 line
2.19.1
PATCH 6/7 ARM: DTS: omap4: add sgx gpu child node
In reply to H. Nikolaus Schallerand add timer and interrupt.
Since omap4420/30/60 and omap4470 come with different SGX variants we need to introduce a new omap4470.dtsi. If an omap4470 board does not want to use SGX it is no problem to still include omap4460.dtsi.
Tested on PandaBoard ES.
--- arch/arm/boot/dts/omap4.dtsi | 11 +++++++---- arch/arm/boot/dts/omap4470.dts | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) create mode 100644 arch/arm/boot/dts/omap4470.dts
arch/arm/boot/dts/omap4.dtsi +7 -4
@@ -347,10 +347,13 @@ #size-cells = <1>; ranges = <0 0x56000000 0x2000000>; - /* - * Closed source PowerVR driver, no child device - * binding or driver in mainline - */ + sgx: sgx@0 { + compatible = "img,sgx540-120", "img,sgx540", "ti,omap-omap4-sgx540-120"; + reg = <0x0 0x2000000>; /* 32MB */ + reg-names = "sgx"; + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; + timer = <&timer11>; + }; }; dss: dss@58000000 {
arch/arm/boot/dts/omap4470.dts new file +16 -0
@@ -0,0 +1,16 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Device Tree Source for OMAP4470 SoC + * + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ + * + * This file is licensed under the terms of the GNU General Public License + * version 2. This program is licensed "as is" without any warranty of any + * kind, whether express or implied. + */ +#include "omap4460.dtsi" + +&sgx { + compatible = "img,sgx544-112", "img,sgx544", "ti,omap-omap4-sgx544-112"; + img,cores = <1>; +};
Signature · 1 line
2.19.1
PATCH 7/7 ARM: DTS: omap5: add sgx gpu child node
In reply to H. Nikolaus Schallerand add timer and interrupt.
Teste on Pyra-Handheld.
--- arch/arm/boot/dts/omap5.dtsi | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
arch/arm/boot/dts/omap5.dtsi +8 -4
@@ -274,10 +274,14 @@ #size-cells = <1>; ranges = <0 0x56000000 0x2000000>; - /* - * Closed source PowerVR driver, no child device - * binding or driver in mainline - */ + sgx: sgx@0 { + compatible = "img,sgx544-116", "img,sgx544", "ti,omap-omap5-sgx544-116"; + reg = <0x0 0x10000>; + reg-names = "sgx"; + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; + timer = <&timer11>; + img,cores = <2>; + }; }; dss: dss@58000000 {
Signature · 1 line
2.19.1