| Message ID | 06fb6569259bb9183d0a0d0fe70ec4f3033b8aab.1586939718.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more) | expand |
Hi, On Wed, Apr 15, 2020 at 10:35:08AM +0200, H. Nikolaus Schaller wrote: > The Imagination PVR/SGX GPU is part of several SoC from > multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo, > Allwinner A83 and others. > > With this binding, we describe how the SGX processor is > interfaced to the SoC (registers, interrupt etc.). > > In most cases, Clock, Reset and power management is handled > by a parent node or elsewhere (e.g. code in the driver). Wouldn't the "code in the driver" still require the clock / reset / power domain to be set in the DT? Maxime
Hi Maxime, > Am 15.04.2020 um 16:21 schrieb Maxime Ripard <maxime@cerno.tech>: > >> >> Well we could add clocks and resets as optional but that would >> allow to wrongly define omap. >> >> Or delegate them to a parent "simple-pm-bus" node. >> >> I have to study that material more to understand what you seem >> to expect. > > The thing is, once that binding is in, it has to be backward > compatible. So every thing that you leave out is something that you'll > need to support in the driver eventually. > > If you don't want it to be a complete nightmare, you'll want to figure > out as much as possible on how the GPU is integrated and make a > binding out of that. Hm. Yes. We know that there likely are clocks and maybe reset but for some SoC this seems to be undocumented and the reset line the VHDL of the sgx gpu provides may be permanently tied to "inactive". So if clocks are optional and not provided, a driver simply can assume they are enabled somewhere else and does not have to care about. If they are specified, the driver can enable/disable them. > If OMAP is too much of a pain, you can also make > a separate binding for it, and a generic one for the rest of us. No, omap isn't any pain at all. The pain is that some other SoC are most easily defined by clocks in the gpu node which the omap doesn't need to explicitly specify. I would expect a much bigger nightmare if we split this into two bindings variants. > I'd say that it's pretty unlikely that the clocks, interrupts (and > even regulators) are optional. It might be fixed on some SoCs, but > that's up to the DT to express that using fixed clocks / regulators, > not the GPU binding itself. omap already has these defined them not to be part of the GPU binding. The reason seems to be that this needs special clock gating control especially for idle states which is beyond simple clock-enable. This sysc target-module@56000000 node is already merged and therefore we are only adding the gpu child node. Without defining clocks. For example: 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>; gpu: gpu@0 { compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544"; reg = <0x0 0x10000>; interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; }; }; The jz4780 example will like this: gpu: gpu@13040000 { compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540"; reg = <0x13040000 0x4000>; clocks = <&cgu JZ4780_CLK_GPU>; clock-names = "gpu"; interrupt-parent = <&intc>; interrupts = <63>; }; So the question is which one is "generic for the rest of us"? And how can we make a single binding for the sgx. Not one for each special SoC variant that may exist. IMHO the best answer is to make clocks an optional property. Or if we do not want to define them explicitly, we use additionalProperties: true. An alternative could be to use a simple-pm-bus like: sgx_module: sgx_module@13040000 { compatible = "simple-pm-bus"; clocks = <&cgu JZ4780_CLK_GPU>; clock-names = "gpu"; #address-cells = <1>; #size-cells = <1>; ranges = <0 0x13040000 0x10000>; gpu: gpu@0 { compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540"; reg = <0x0 0x4000>; interrupt-parent = <&intc>; interrupts = <63>; }; }; This gets rid of any clock, reset and pm definitions for the sgx bindings. But how this is done is outside this sgx bindings. With such a scheme, the binding I propose here would be complete and fully generic. We can even add additionalProperties: false. BR, Nikolaus
On Wed, 15 Apr 2020 10:35:08 +0200, "H. Nikolaus Schaller" wrote: > The Imagination PVR/SGX GPU is part of several SoC from > multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo, > Allwinner A83 and others. > > With this binding, we describe how the SGX processor is > interfaced to the SoC (registers, interrupt etc.). > > In most cases, Clock, Reset and power management is handled > by a parent node or elsewhere (e.g. code in the driver). > > Tested by make dt_binding_check dtbs_check > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > .../devicetree/bindings/gpu/img,pvrsgx.yaml | 122 ++++++++++++++++++ > 1 file changed, 122 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml > My bot found errors running 'make dt_binding_check' on your patch: Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml: while parsing a block mapping in "<unicode string>", line 74, column 13 did not find expected key in "<unicode string>", line 117, column 21 Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/gpu/img,pvrsgx.example.dts' failed make[1]: *** [Documentation/devicetree/bindings/gpu/img,pvrsgx.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... Makefile:1264: recipe for target 'dt_binding_check' failed make: *** [dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1270997 If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date: pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade Please check and re-submit.
Hi, On Wed, Apr 15, 2020 at 06:42:18PM +0200, H. Nikolaus Schaller wrote: > > Am 15.04.2020 um 18:21 schrieb Maxime Ripard <maxime@cerno.tech>: > > > > On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote: > >> Hi Maxime, > >> > >> Hm. Yes. We know that there likely are clocks and maybe reset > >> but for some SoC this seems to be undocumented and the reset > >> line the VHDL of the sgx gpu provides may be permanently tied > >> to "inactive". > >> > >> So if clocks are optional and not provided, a driver simply can assume > >> they are enabled somewhere else and does not have to care about. If > >> they are specified, the driver can enable/disable them. > > > > Except that at the hardware level, the clock is always going to be > > there. You can't control it, but it's there. > > Sure, we can deduce that from general hardware design knowledge. > But not every detail must be described in DT. Only the important > ones. > > >>> If OMAP is too much of a pain, you can also make > >>> a separate binding for it, and a generic one for the rest of us. > >> > >> No, omap isn't any pain at all. > >> > >> The pain is that some other SoC are most easily defined by clocks in > >> the gpu node which the omap doesn't need to explicitly specify. > >> > >> I would expect a much bigger nightmare if we split this into two > >> bindings variants. > >> > >>> I'd say that it's pretty unlikely that the clocks, interrupts (and > >>> even regulators) are optional. It might be fixed on some SoCs, but > >>> that's up to the DT to express that using fixed clocks / regulators, > >>> not the GPU binding itself. > >> > >> omap already has these defined them not to be part of the GPU binding. > >> The reason seems to be that this needs special clock gating control > >> especially for idle states which is beyond simple clock-enable. > >> > >> This sysc target-module@56000000 node is already merged and therefore > >> we are only adding the gpu child node. Without defining clocks. > >> > >> For example: > >> > >> 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>; > >> > >> gpu: gpu@0 { > >> compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544"; > >> reg = <0x0 0x10000>; > >> interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; > >> }; > >> }; > >> > >> The jz4780 example will like this: > >> > >> gpu: gpu@13040000 { > >> compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540"; > >> reg = <0x13040000 0x4000>; > >> > >> clocks = <&cgu JZ4780_CLK_GPU>; > >> clock-names = "gpu"; > >> > >> interrupt-parent = <&intc>; > >> interrupts = <63>; > >> }; > >> > >> So the question is which one is "generic for the rest of us"? > > > > I'd say the latter. > > Why? > > TI SoC seem to be the broadest number of available users > of sgx5xx in the past and nowadays. Others are more the exception. And maybe TI has some complicated stuff around the GPU that others don't have? If I look quickly at the Allwinner stuff, I see nothing looking alike in the SoC, so making the binding like that for everyone just because TI did something doesn't really make much sense. > > If your clock is optional, then you define it but don't mandate > > it. Not documenting it will only result in a mess where everyone will > > put some clock into it, possibly with different semantics each and > > every time. > > So you mean that we should require a dummy clock for the omap gpu node > or did I misunderstand that? > > Well, yes there is of course a clock connection between the > omap target-module and the sgx but it is IMHO pointless to > describe it because it can't and does not need to be controlled > separately. > > As said the target-module is already accepted and upstream and my > proposal is to get the gpu node described there. There is simply > no need for a clocks node for the omap. There is no need for a clocks property *currently* *on the OMAP*. > What I also assume is that developers of DTS know what they do. > So the risk that there is different semantics is IMHO very low. Well, they know what they do if you document the binding. Let's say I have two clocks now on my SoC, and you just document that you want a clocks property, with a generic name in clock-names like "gpu". > If you agree I can add the clocks/clock-names property as an > optional property. This should solve omap and all others. With the above example, what clock should I put in there? In which order? This isn't some random example pulled out of nowhere. The Allwinner A31 has (at least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume that the GPU actually needs at least that amount to be properly integrated into an SoC. This has nothing to do with being dumb or smart. > > This has nothing to do with the binding being complete. And if you use > > a binding like this one, you'll be severely limited when you'll want > > to implement things like DVFS. > > Now you have unhooked me... Nobody seems to know if and how DVFS can be > applied to SGX. IMHO we should bake small bread first and get initial > support into mainline. On the software side, yes, of course. But the discussion here doesn't have much to do with software support, this is about the hardware. No matter if you enable DVFS or not, you'll have those resources connected to the GPU. And if you want to enable the strict minimum in DT for now and expand it later as the software gains support for more stuff, then you'll have to deal with the minimal stuff in software later-on to keep the backward compatibility. But given that the current state on the Allwinner SoCs (at least) is that you can't even read a register, it might be a good idea to delay the introduction of that binding until you have something that works to avoid drowning under the number of special cases to deal with backward compatibility. Maxime
Hi Maxime, > Am 17.04.2020 um 12:25 schrieb Maxime Ripard <maxime@cerno.tech>: > > Hi, > > On Wed, Apr 15, 2020 at 06:42:18PM +0200, H. Nikolaus Schaller wrote: >>> Am 15.04.2020 um 18:21 schrieb Maxime Ripard <maxime@cerno.tech>: >>> >>> On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote: >>>> Hi Maxime, >>>> >>>> Hm. Yes. We know that there likely are clocks and maybe reset >>>> but for some SoC this seems to be undocumented and the reset >>>> line the VHDL of the sgx gpu provides may be permanently tied >>>> to "inactive". >>>> >>>> So if clocks are optional and not provided, a driver simply can assume >>>> they are enabled somewhere else and does not have to care about. If >>>> they are specified, the driver can enable/disable them. >>> >>> Except that at the hardware level, the clock is always going to be >>> there. You can't control it, but it's there. >> >> Sure, we can deduce that from general hardware design knowledge. >> But not every detail must be described in DT. Only the important >> ones. >> >>>>> If OMAP is too much of a pain, you can also make >>>>> a separate binding for it, and a generic one for the rest of us. >>>> >>>> No, omap isn't any pain at all. >>>> >>>> The pain is that some other SoC are most easily defined by clocks in >>>> the gpu node which the omap doesn't need to explicitly specify. >>>> >>>> I would expect a much bigger nightmare if we split this into two >>>> bindings variants. >>>> >>>>> I'd say that it's pretty unlikely that the clocks, interrupts (and >>>>> even regulators) are optional. It might be fixed on some SoCs, but >>>>> that's up to the DT to express that using fixed clocks / regulators, >>>>> not the GPU binding itself. >>>> >>>> omap already has these defined them not to be part of the GPU binding. >>>> The reason seems to be that this needs special clock gating control >>>> especially for idle states which is beyond simple clock-enable. >>>> >>>> This sysc target-module@56000000 node is already merged and therefore >>>> we are only adding the gpu child node. Without defining clocks. >>>> >>>> For example: >>>> >>>> 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>; >>>> >>>> gpu: gpu@0 { >>>> compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544"; >>>> reg = <0x0 0x10000>; >>>> interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; >>>> }; >>>> }; >>>> >>>> The jz4780 example will like this: >>>> >>>> gpu: gpu@13040000 { >>>> compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540"; >>>> reg = <0x13040000 0x4000>; >>>> >>>> clocks = <&cgu JZ4780_CLK_GPU>; >>>> clock-names = "gpu"; >>>> >>>> interrupt-parent = <&intc>; >>>> interrupts = <63>; >>>> }; >>>> >>>> So the question is which one is "generic for the rest of us"? >>> >>> I'd say the latter. >> >> Why? >> >> TI SoC seem to be the broadest number of available users >> of sgx5xx in the past and nowadays. Others are more the exception. > > And maybe TI has some complicated stuff around the GPU that others don't have? Looks so. > If I look quickly at the Allwinner stuff, I see nothing looking alike in the > SoC, so making the binding like that for everyone just because TI did something > doesn't really make much sense. That is why I propose to make the clocks optional. This solves both cases in a simple and neat way. > >>> If your clock is optional, then you define it but don't mandate >>> it. Not documenting it will only result in a mess where everyone will >>> put some clock into it, possibly with different semantics each and >>> every time. >> >> So you mean that we should require a dummy clock for the omap gpu node >> or did I misunderstand that? >> >> Well, yes there is of course a clock connection between the >> omap target-module and the sgx but it is IMHO pointless to >> describe it because it can't and does not need to be controlled >> separately. >> >> As said the target-module is already accepted and upstream and my >> proposal is to get the gpu node described there. There is simply >> no need for a clocks node for the omap. > > There is no need for a clocks property *currently* *on the OMAP*. Yes. But why "currently"? Do you think the OMAPs we already have defined and tested will change? > >> What I also assume is that developers of DTS know what they do. >> So the risk that there is different semantics is IMHO very low. > > Well, they know what they do if you document the binding. Let's say I have two > clocks now on my SoC, and you just document that you want a clocks property, > with a generic name in clock-names like "gpu". Yes, that is what I want to propose for v7: clocks: maxItems: 1 clock-names: maxItems: 1 items: - const: gpu > >> If you agree I can add the clocks/clock-names property as an >> optional property. This should solve omap and all others. > > With the above example, what clock should I put in there? In which order? This > isn't some random example pulled out of nowhere. The Allwinner A31 has (at > least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume > that the GPU actually needs at least that amount to be properly integrated into > an SoC. Ah, now I understand your motivation: you have access and experience with the A31 and you know that our proposal doesn't fit to it. From what I know from your description is that the A31 is quite special with 4 GPU clocks... Are they all really for the GPU or 3 of them for the interface logic (like on OMAP which separates between "functional clocks" and "interface clocks")? Or are there 4 groups of GPU cores with a separate clock for each one? So what would be your proposal for the A31 DT? Then I get a chance to compare DT snippets and try to make a mixture for the bindings. > >>> This has nothing to do with the binding being complete. And if you use >>> a binding like this one, you'll be severely limited when you'll want >>> to implement things like DVFS. >> >> Now you have unhooked me... Nobody seems to know if and how DVFS can be >> applied to SGX. IMHO we should bake small bread first and get initial >> support into mainline. > > On the software side, yes, of course. But the discussion here doesn't have much > to do with software support, this is about the hardware. No matter if you enable > DVFS or not, you'll have those resources connected to the GPU. > > And if you want to enable the strict minimum in DT for now and expand it later > as the software gains support for more stuff, then you'll have to deal with the > minimal stuff in software later-on to keep the backward compatibility. That is IMHO common practise everywhere. Sometimes you even have to adapt years old DT to new limitations of the drivers (this happened recently for combination of SPI and GPIO). And you can still write two different drivers for a single bindings document or use the .data field of the compatibility table. And I think clocks and regulators can also be referenced by name if they are not defined in DT. This is not a "single variety" style, but a potential solution. What I want to say: there are many roads to Rome. > But given that the current state on the Allwinner SoCs (at least) is that you > can't even read a register, it might be a good idea to delay the introduction of > that binding until you have something that works to avoid drowning under the > number of special cases to deal with backward compatibility. Philipp has something minimal working on the A83 which works with the proposed binding and enabled him to read out the sgx revision register. So if you are a specialist for the A31 SGX, please make a proposal for a binding that covers all the A31 needs and all other SoC we know. Or define a separate bindings for the A31. Thank you very much, Nikolaus Schaller
Hi Rob, > Am 16.04.2020 um 22:41 schrieb Rob Herring <robh@kernel.org>: > > On Wed, 15 Apr 2020 10:35:08 +0200, "H. Nikolaus Schaller" wrote: >> The Imagination PVR/SGX GPU is part of several SoC from >> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo, >> Allwinner A83 and others. >> >> With this binding, we describe how the SGX processor is >> interfaced to the SoC (registers, interrupt etc.). >> >> In most cases, Clock, Reset and power management is handled >> by a parent node or elsewhere (e.g. code in the driver). >> >> Tested by make dt_binding_check dtbs_check >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- >> .../devicetree/bindings/gpu/img,pvrsgx.yaml | 122 ++++++++++++++++++ >> 1 file changed, 122 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml >> > > My bot found errors running 'make dt_binding_check' on your patch: > > Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml: while parsing a block mapping > in "<unicode string>", line 74, column 13 It turned out that there was a stray " in line 74 from the last editing phase. Will be fixed in v7. Would it be possible to make dt_binding_check not only report line/column but the contents of the line instead of "<unicode string>"? > did not find expected key > in "<unicode string>", line 117, column 21 > Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/gpu/img,pvrsgx.example.dts' failed > make[1]: *** [Documentation/devicetree/bindings/gpu/img,pvrsgx.example.dts] Error 1 > make[1]: *** Waiting for unfinished jobs.... > Makefile:1264: recipe for target 'dt_binding_check' failed > make: *** [dt_binding_check] Error 2 > > See https://patchwork.ozlabs.org/patch/1270997 > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure dt-schema is up to date: > > pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade > > Please check and re-submit. BR and thanks, Nikolaus Schaller
Hi Nikolaus, Hi Maxime, >>> TI SoC seem to be the broadest number of available users >>> of sgx5xx in the past and nowadays. Others are more the exception. >> >> And maybe TI has some complicated stuff around the GPU that others don't have? > > Looks so. I can only agree on this. > >> >>> What I also assume is that developers of DTS know what they do. >>> So the risk that there is different semantics is IMHO very low. >> >> Well, they know what they do if you document the binding. Let's say I have two >> clocks now on my SoC, and you just document that you want a clocks property, >> with a generic name in clock-names like "gpu". > > Yes, that is what I want to propose for v7: > > clocks: > maxItems: 1 > > clock-names: > maxItems: 1 > items: > - const: gpu > >> >>> If you agree I can add the clocks/clock-names property as an >>> optional property. This should solve omap and all others. >> >> With the above example, what clock should I put in there? In which order? This >> isn't some random example pulled out of nowhere. The Allwinner A31 has (at >> least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume >> that the GPU actually needs at least that amount to be properly integrated into >> an SoC. > > Ah, now I understand your motivation: you have access and experience with > the A31 and you know that our proposal doesn't fit to it. > > From what I know from your description is that the A31 is quite special with > 4 GPU clocks... Are they all really for the GPU or 3 of them for the interface > logic (like on OMAP which separates between "functional clocks" and "interface > clocks")? Or are there 4 groups of GPU cores with a separate clock for each one? > > So what would be your proposal for the A31 DT? > > Then I get a chance to compare DT snippets and try to make a mixture for > the bindings. > This is my current binding for the A83T, the A31 looks similar but there is still something missing, since some parts are not mainlined yet: sun8i-a83t.dtsi: gpu: gpu@1c400000 { compatible = "allwinner,sun8i-a83t-sgx544-115", "img,sgx544-115", "img,sgx544"; reg = <0x01c40000 0x10000>; interrupts = <GIC_SPI 129 IRQ_TYPE_LEVEL_HIGH>; clocks = <&ccu CLK_BUS_GPU>, <&ccu CLK_GPU_CORE>, <&ccu CLK_GPU_MEMORY>, <&ccu CLK_GPU_HYD>; clock-names = "bus", "core", "memory", "hyd"; resets = <&ccu RST_BUS_GPU>; }; sun8i-a83t-bananapi-m3.dts: &gpu { gpu-supply = <®_dcdc4>; }; > >> But given that the current state on the Allwinner SoCs (at least) is that you >> can't even read a register, it might be a good idea to delay the introduction of >> that binding until you have something that works to avoid drowning under the >> number of special cases to deal with backward compatibility. > Maxime is right. Even if you enable the regulator, write 0x0 to the GPU Power Off Gating Register, deassert the reset and enable the clocks you are not able to read the register. You must first run: pvrsrvctl --no-module --start (user space binaries) to access registers otherwise the system will stuck with the following message when accessing them: ./devmem2 0x01C40024 /dev/mem opened. > Philipp has something minimal working on the A83 which works with the proposed > binding and enabled him to read out the sgx revision register. > This is not correct. In the other mail I talked about my reference system. This is an old 3.4.39 kernel, modified by allwinner to run on their SOC's which don't use the common kernel techniques. So it's very hacky, but they got the gpu running. I'm using this system for comparison, to read out registers and for reverse engineering. My current kernel module behaves similar like the reference design, but right now I'm not able to run "pvrsrvctl --no-module --start" without errors. So the initialization never get's finalized and I see the issue described above. > So if you are a specialist for the A31 SGX, please make a proposal for a binding > that covers all the A31 needs and all other SoC we know. Or define a separate > bindings for the A31. The A31 and the A31s have some additional clocks mentioned in their datasheet (@ System Control Register/SRAM Controler). Those are not available in the A83T datasheet, but might be there since the memory map looks similar and allwinner might use the same userspace binaries for their devices. From the knowledge I gained the last 3 days, we should delay the patches for the A83T, A31 and A31s. The idea of the placeholder patches was to show that from this binding also other devices could benefit. Cheers, Philipp
On Fri, Apr 17, 2020 at 02:15:44PM +0200, H. Nikolaus Schaller wrote: > > Am 17.04.2020 um 12:25 schrieb Maxime Ripard <maxime@cerno.tech>: > > On Wed, Apr 15, 2020 at 06:42:18PM +0200, H. Nikolaus Schaller wrote: > >>> Am 15.04.2020 um 18:21 schrieb Maxime Ripard <maxime@cerno.tech>: > >>> > >>> On Wed, Apr 15, 2020 at 05:09:45PM +0200, H. Nikolaus Schaller wrote: > >>>> Hi Maxime, > >>>> > >>>> Hm. Yes. We know that there likely are clocks and maybe reset > >>>> but for some SoC this seems to be undocumented and the reset > >>>> line the VHDL of the sgx gpu provides may be permanently tied > >>>> to "inactive". > >>>> > >>>> So if clocks are optional and not provided, a driver simply can assume > >>>> they are enabled somewhere else and does not have to care about. If > >>>> they are specified, the driver can enable/disable them. > >>> > >>> Except that at the hardware level, the clock is always going to be > >>> there. You can't control it, but it's there. > >> > >> Sure, we can deduce that from general hardware design knowledge. > >> But not every detail must be described in DT. Only the important > >> ones. > >> > >>>>> If OMAP is too much of a pain, you can also make > >>>>> a separate binding for it, and a generic one for the rest of us. > >>>> > >>>> No, omap isn't any pain at all. > >>>> > >>>> The pain is that some other SoC are most easily defined by clocks in > >>>> the gpu node which the omap doesn't need to explicitly specify. > >>>> > >>>> I would expect a much bigger nightmare if we split this into two > >>>> bindings variants. > >>>> > >>>>> I'd say that it's pretty unlikely that the clocks, interrupts (and > >>>>> even regulators) are optional. It might be fixed on some SoCs, but > >>>>> that's up to the DT to express that using fixed clocks / regulators, > >>>>> not the GPU binding itself. > >>>> > >>>> omap already has these defined them not to be part of the GPU binding. > >>>> The reason seems to be that this needs special clock gating control > >>>> especially for idle states which is beyond simple clock-enable. > >>>> > >>>> This sysc target-module@56000000 node is already merged and therefore > >>>> we are only adding the gpu child node. Without defining clocks. > >>>> > >>>> For example: > >>>> > >>>> 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>; > >>>> > >>>> gpu: gpu@0 { > >>>> compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544"; > >>>> reg = <0x0 0x10000>; > >>>> interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; > >>>> }; > >>>> }; > >>>> > >>>> The jz4780 example will like this: > >>>> > >>>> gpu: gpu@13040000 { > >>>> compatible = "ingenic,jz4780-sgx540-130", "img,sgx540-130", "img,sgx540"; > >>>> reg = <0x13040000 0x4000>; > >>>> > >>>> clocks = <&cgu JZ4780_CLK_GPU>; > >>>> clock-names = "gpu"; > >>>> > >>>> interrupt-parent = <&intc>; > >>>> interrupts = <63>; > >>>> }; > >>>> > >>>> So the question is which one is "generic for the rest of us"? > >>> > >>> I'd say the latter. > >> > >> Why? > >> > >> TI SoC seem to be the broadest number of available users > >> of sgx5xx in the past and nowadays. Others are more the exception. > > > > And maybe TI has some complicated stuff around the GPU that others don't have? > > Looks so. > > > If I look quickly at the Allwinner stuff, I see nothing looking alike in the > > SoC, so making the binding like that for everyone just because TI did something > > doesn't really make much sense. > > That is why I propose to make the clocks optional. This solves both > cases in a simple and neat way. > > > > >>> If your clock is optional, then you define it but don't mandate > >>> it. Not documenting it will only result in a mess where everyone will > >>> put some clock into it, possibly with different semantics each and > >>> every time. > >> > >> So you mean that we should require a dummy clock for the omap gpu node > >> or did I misunderstand that? > >> > >> Well, yes there is of course a clock connection between the > >> omap target-module and the sgx but it is IMHO pointless to > >> describe it because it can't and does not need to be controlled > >> separately. > >> > >> As said the target-module is already accepted and upstream and my > >> proposal is to get the gpu node described there. There is simply > >> no need for a clocks node for the omap. > > > > There is no need for a clocks property *currently* *on the OMAP*. > > Yes. But why "currently"? Do you think the OMAPs we already have > defined and tested will change? Like I said, DVFS is likely to be one in the future. > >> What I also assume is that developers of DTS know what they do. > >> So the risk that there is different semantics is IMHO very low. > > > > Well, they know what they do if you document the binding. Let's say I have two > > clocks now on my SoC, and you just document that you want a clocks property, > > with a generic name in clock-names like "gpu". > > Yes, that is what I want to propose for v7: > > clocks: > maxItems: 1 > > clock-names: > maxItems: 1 > items: > - const: gpu If you document what the "gpu" clock is supposed to be. Is it the clock for the bus (clocking the register part of the GPU), the clock for the GPU cores? Something else? > >> If you agree I can add the clocks/clock-names property as an > >> optional property. This should solve omap and all others. > > > > With the above example, what clock should I put in there? In which order? This > > isn't some random example pulled out of nowhere. The Allwinner A31 has (at > > least) 4 clocks for the GPU, 1 reset line and 1 regulator, so I can only assume > > that the GPU actually needs at least that amount to be properly integrated into > > an SoC. > > Ah, now I understand your motivation: you have access and experience with > the A31 and you know that our proposal doesn't fit to it. Not only the A31. If you don't document what your expectations are for a generic component like that, every SoC will assume that your GPU clock is something different and you won't be able to make any sense of it in your driver. > From what I know from your description is that the A31 is quite special with > 4 GPU clocks... Are they all really for the GPU or 3 of them for the interface > logic (like on OMAP which separates between "functional clocks" and "interface > clocks")? Or are there 4 groups of GPU cores with a separate clock for each one? 1 is the equivalent of the interface clock, the others seem to be for the functional clocks. > So what would be your proposal for the A31 DT? > > Then I get a chance to compare DT snippets and try to make a mixture for > the bindings. You'd have to know the GPU to do that, and I don't. > >>> This has nothing to do with the binding being complete. And if you use > >>> a binding like this one, you'll be severely limited when you'll want > >>> to implement things like DVFS. > >> > >> Now you have unhooked me... Nobody seems to know if and how DVFS can be > >> applied to SGX. IMHO we should bake small bread first and get initial > >> support into mainline. > > > > On the software side, yes, of course. But the discussion here doesn't have much > > to do with software support, this is about the hardware. No matter if you enable > > DVFS or not, you'll have those resources connected to the GPU. > > > > And if you want to enable the strict minimum in DT for now and expand it later > > as the software gains support for more stuff, then you'll have to deal with the > > minimal stuff in software later-on to keep the backward compatibility. > > That is IMHO common practise everywhere. Sometimes you even have to adapt > years old DT to new limitations of the drivers (this happened recently for > combination of SPI and GPIO). To some extent, yes. But those old bindings that turn out to be wrong at least contain most infos about the hardware, even though it's incomplete or flawed. Your proposal doesn't. > And you can still write two different drivers for a single bindings document > or use the .data field of the compatibility table. And I think clocks and regulators > can also be referenced by name if they are not defined in DT. This is not a > "single variety" style, but a potential solution. > > What I want to say: there are many roads to Rome. What I want to say is: all the roads you listed above are going to be painful. Take your time, have a generic driver running from your generic binding you want to introduce on all the SoCs you want to support, and *then* start this discussion again. Maxime
On Fri, Apr 17, 2020 at 7:16 AM H. Nikolaus Schaller <hns@goldelico.com> wrote: > > Hi Rob, > > > Am 16.04.2020 um 22:41 schrieb Rob Herring <robh@kernel.org>: > > > > On Wed, 15 Apr 2020 10:35:08 +0200, "H. Nikolaus Schaller" wrote: > >> The Imagination PVR/SGX GPU is part of several SoC from > >> multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo, > >> Allwinner A83 and others. > >> > >> With this binding, we describe how the SGX processor is > >> interfaced to the SoC (registers, interrupt etc.). > >> > >> In most cases, Clock, Reset and power management is handled > >> by a parent node or elsewhere (e.g. code in the driver). > >> > >> Tested by make dt_binding_check dtbs_check > >> > >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > >> --- > >> .../devicetree/bindings/gpu/img,pvrsgx.yaml | 122 ++++++++++++++++++ > >> 1 file changed, 122 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml > >> > > > > My bot found errors running 'make dt_binding_check' on your patch: > > > > Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml: while parsing a block mapping > > in "<unicode string>", line 74, column 13 > > It turned out that there was a stray " in line 74 from the last editing phase. > Will be fixed in v7. > > Would it be possible to make dt_binding_check not only report line/column but the > contents of the line instead of "<unicode string>"? This comes from ruamel.yaml module. I believe "<unicode string>" is supposed to be the type of the data, not what it is. However, it is possible to get something a bit more helpful, but it depends on which parser is used. By default we use the C based parser (aka 'safe'). If we use the round trip parser, we get this: ruamel.yaml.scanner.ScannerError: while scanning a simple key in "<unicode string>", line 84, column 5: maxItems ^ (line: 84) This can be enabled by passing '-n' (line numbers) to dt-doc-validate. Currently, you have have to edit the Makefile to do this. The C parser is a big difference in speed, so I don't want to change the default. I can probably work around this and dump the erroring line, but I'm not sure that's always useful. Many errors are indentation related and those need some context. Plus just dumping the line can be done easily with sed: sed -n ${LINE}p <file> Rob
diff --git a/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml new file mode 100644 index 000000000000..e3a4208dfab1 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml @@ -0,0 +1,122 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/gpu/img,pvrsgx.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Imagination PVR/SGX GPU + +maintainers: + - H. Nikolaus Schaller <hns@goldelico.com> + +description: |+ + This binding describes the Imagination SGX5 series of 3D accelerators which + are found in several different SoC like TI OMAP, Sitara, Ingenic JZ4780, + Allwinner A83, and Intel Poulsbo and CedarView and more. + + For an extensive list see: https://en.wikipedia.org/wiki/PowerVR#Implementations + + The SGX node is usually a child node of some DT node belonging to the SoC + which handles clocks, reset and general address space mapping of the SGX + register area. + +properties: + compatible: + oneOf: + - description: SGX530-121 based SoC + items: + - enum: + - ti,omap3-sgx530-121 # BeagleBoard A/B/C, OpenPandora 600MHz and similar + - const: img,sgx530-121 + - const: img,sgx530 + + - description: SGX530-125 based SoC + items: + - enum: + - ti,am3352-sgx530-125 # BeagleBone Black + - ti,am3517-sgx530-125 + - ti,am4-sgx530-125 + - ti,omap3-sgx530-125 # BeagleBoard XM, GTA04, OpenPandora 1GHz and similar + - ti,ti81xx-sgx530-125 + - const: ti,omap3-sgx530-125 + - const: img,sgx530-125 + - const: img,sgx530 + + - description: SGX535-116 based SoC + items: + - const: intel,poulsbo-gma500-sgx535 # Atom Z5xx + - const: img,sgx535-116 + - const: img,sgx535 + + - description: SGX540-116 based SoC + items: + - const: intel,medfield-gma-sgx540 # Atom Z24xx + - const: img,sgx540-116 + - const: img,sgx540 + + - description: SGX540-120 based SoC + items: + - enum: + - samsung,s5pv210-sgx540-120 + - ti,omap4-sgx540-120 # Pandaboard, Pandaboard ES and similar + - const: img,sgx540-120 + - const: img,sgx540 + + - description: SGX540-130 based SoC + items: + - enum: + - ingenic,jz4780-sgx540-130 # CI20 + - const: img,sgx540-130 + - const: img,sgx540 + + - description: SGX544-112 based SoC + items: + - const: "ti,omap4470-sgx544-112 + - const: img,sgx544-112 + - const: img,sgx544 + + - description: SGX544-115 based SoC + items: + - enum: + - allwinner,sun8i-a31-sgx544-115 + - allwinner,sun8i-a31s-sgx544-115 + - allwinner,sun8i-a83t-sgx544-115 # Banana-Pi-M3 (Allwinner A83T) and similar + - const: img,sgx544-115 + - const: img,sgx544 + + - description: SGX544-116 based SoC + items: + - enum: + - ti,dra7-sgx544-116 # DRA7 + - ti,omap5-sgx544-116 # OMAP5 UEVM, Pyra Handheld and similar + - const: img,sgx544-116 + - const: img,sgx544 + + - description: SGX545 based SoC + items: + - const: intel,cedarview-gma3600-sgx545 # Atom N2600, D2500 + - const: img,sgx545-116 + - const: img,sgx545 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + +required: + - compatible + - reg + - interrupts + +examples: + - |+ + #include <dt-bindings/interrupt-controller/arm-gic.h> + + gpu: gpu@fe00 { + compatible = "ti,omap5-sgx544-116", "img,sgx544-116", "img,sgx544"; + reg = <0xfe00 0x200>; + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; + }; + +...
The Imagination PVR/SGX GPU is part of several SoC from multiple vendors, e.g. TI OMAP, Ingenic JZ4780, Intel Poulsbo, Allwinner A83 and others. With this binding, we describe how the SGX processor is interfaced to the SoC (registers, interrupt etc.). In most cases, Clock, Reset and power management is handled by a parent node or elsewhere (e.g. code in the driver). Tested by make dt_binding_check dtbs_check Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> --- .../devicetree/bindings/gpu/img,pvrsgx.yaml | 122 ++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.yaml