| Message ID | cover.1587760454.git.hns@goldelico.com |
|---|---|
| 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 Nikolaus, Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > Add SGX GPU node with interrupt. Tested on PandaBoard ES. > > 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. > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > arch/arm/boot/dts/omap4.dtsi | 11 ++++++----- > arch/arm/boot/dts/omap4470.dts | 15 +++++++++++++++ > 2 files changed, 21 insertions(+), 5 deletions(-) > create mode 100644 arch/arm/boot/dts/omap4470.dts > > diff --git a/arch/arm/boot/dts/omap4.dtsi > b/arch/arm/boot/dts/omap4.dtsi > index 763bdea8c829..15ff3d7146af 100644 > --- a/arch/arm/boot/dts/omap4.dtsi > +++ b/arch/arm/boot/dts/omap4.dtsi > @@ -389,7 +389,7 @@ abb_iva: regulator-abb-iva { > status = "disabled"; > }; > > - target-module@56000000 { > + sgx_module: target-module@56000000 { > compatible = "ti,sysc-omap4", "ti,sysc"; > reg = <0x5600fe00 0x4>, > <0x5600fe10 0x4>; > @@ -408,10 +408,11 @@ target-module@56000000 { > #size-cells = <1>; > ranges = <0 0x56000000 0x2000000>; > > - /* > - * Closed source PowerVR driver, no child device > - * binding or driver in mainline > - */ > + gpu: gpu@0 { > + compatible = "ti,omap4-sgx540-120", "img,sgx540-120", > "img,sgx540"; > + reg = <0x0 0x2000000>; /* 32MB */ > + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; > + }; > }; > > /* > diff --git a/arch/arm/boot/dts/omap4470.dts > b/arch/arm/boot/dts/omap4470.dts > new file mode 100644 > index 000000000000..f29c581300bf > --- /dev/null > +++ b/arch/arm/boot/dts/omap4470.dts > @@ -0,0 +1,15 @@ > +// 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 { Does this even compile? The node's handle is named sgx_module, not sgx. -Paul > + compatible = "ti,omap4470-sgx544-112", "img,sgx544-112", > "img,sgx544"; > +}; > -- > 2.25.1 >
Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > From: Philipp Rossak <embed3d@gmail.com> > > We are adding the devicetree binding for the PVR-SGX-544-115 gpu. > > This driver is currently under development in the openpvrsgx-devgroup. > Right now the full binding is not figured out, so we provide here a > placeholder. It will be completed as soon as there is a demo > available. > > The currently used binding that is used during development is more > complete and was already verifyed by loading the kernelmodule > successful. > > Signed-off-by: Philipp Rossak <embed3d@gmail.com> > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > arch/arm/boot/dts/sun6i-a31.dtsi | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi > b/arch/arm/boot/dts/sun6i-a31.dtsi > index f3425a66fc0a..933a825bf460 100644 > --- a/arch/arm/boot/dts/sun6i-a31.dtsi > +++ b/arch/arm/boot/dts/sun6i-a31.dtsi > @@ -1417,5 +1417,16 @@ p2wi: i2c@1f03400 { > #address-cells = <1>; > #size-cells = <0>; > }; > + > + gpu: gpu@1c400000 { > + compatible = "allwinner,sun8i-a31-sgx544-115", > + "img,sgx544-115", "img,sgx544"; > + reg = <0x01c40000 0x10000>; > + /* > + * This node is currently a placeholder for the gpu. > + * This will be completed when a full demonstration > + * of the openpvrsgx driver is available for this board. > + */ This node doesn't have clocks, so I don't see how it'd work. Better delay the introduction of the GPU node for this board until you know it works. -Paul > + }; > }; > }; > -- > 2.25.1 >
Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > From: Jonathan Bakker <xc-racer2@live.ca> > > All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached. > > There is no external regulator for it so it can be enabled by default. > > Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/arch/arm/boot/dts/s5pv210.dtsi > b/arch/arm/boot/dts/s5pv210.dtsi > index 2ad642f51fd9..abbdda205c1b 100644 > --- a/arch/arm/boot/dts/s5pv210.dtsi > +++ b/arch/arm/boot/dts/s5pv210.dtsi > @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 { > #interrupt-cells = <1>; > }; > > + gpu: gpu@f3000000 { > + compatible = "samsung,s5pv210-sgx540-120"; > + reg = <0xf3000000 0x10000>; > + interrupt-parent = <&vic2>; > + interrupts = <10>; > + clock-names = "core"; > + clocks = <&clocks CLK_G3D>; > + > + assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>; > + assigned-clock-rates = <0>, <66700000>; > + assigned-clock-parents = <&clocks MOUT_MPLL>; What are these clocks for, and why are they reparented / reclocked? Shouldn't they be passed to 'clocks' as well? -Paul > + }; > + > fimd: fimd@f8000000 { > compatible = "samsung,s5pv210-fimd"; > interrupt-parent = <&vic2>; > -- > 2.25.1 >
Hi Paul, On 2020-04-26 5:56 a.m., Paul Cercueil wrote: > > > Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> From: Jonathan Bakker <xc-racer2@live.ca> >> >> All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached. >> >> There is no external regulator for it so it can be enabled by default. >> >> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- >> arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi >> index 2ad642f51fd9..abbdda205c1b 100644 >> --- a/arch/arm/boot/dts/s5pv210.dtsi >> +++ b/arch/arm/boot/dts/s5pv210.dtsi >> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 { >> #interrupt-cells = <1>; >> }; >> >> + gpu: gpu@f3000000 { >> + compatible = "samsung,s5pv210-sgx540-120"; >> + reg = <0xf3000000 0x10000>; >> + interrupt-parent = <&vic2>; >> + interrupts = <10>; >> + clock-names = "core"; >> + clocks = <&clocks CLK_G3D>; >> + >> + assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>; >> + assigned-clock-rates = <0>, <66700000>; >> + assigned-clock-parents = <&clocks MOUT_MPLL>; > > What are these clocks for, and why are they reparented / reclocked? > > Shouldn't they be passed to 'clocks' as well? > > -Paul > The G3D clock system can have multiple parents, and for stable operation it's recommended to use the MPLL clock as the parent (which in turn is actually a mux as well). MOUT_G3D is simply the mux for CLK_G3D (SGX core clock), DOUT_G3D is the divider. DOUT_G3D could equally be CLK_G3D (and probably should be, for readability) as CLK_G3D is simply the gate and DOUT_G3D is the divider for it. The SGX clock layout on S5PV210 looks something like this: MOUT_MPLL -----------> MOUT_G3D ---> DOUT_G3D ---> CLK_G3D (selectable parent clock) (mux) ---> (divider) ---> (gate) This is fairly common for older Samsung SoCs, eg having a look at arch/arm/boot/dts/exynos4210-universal_c210.dts you can see that the FIMC clocks are parented to MPLL and have a rate set. >> + }; >> + >> fimd: fimd@f8000000 { >> compatible = "samsung,s5pv210-fimd"; >> interrupt-parent = <&vic2>; >> -- >> 2.25.1 >> > > Thanks, Jonathan
Hi Paul, On 26.04.20 14:53, Paul Cercueil wrote: > > > Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> > a écrit : >> From: Philipp Rossak <embed3d@gmail.com> >> >> We are adding the devicetree binding for the PVR-SGX-544-115 gpu. >> >> This driver is currently under development in the openpvrsgx-devgroup. >> Right now the full binding is not figured out, so we provide here a >> placeholder. It will be completed as soon as there is a demo available. >> >> The currently used binding that is used during development is more >> complete and was already verifyed by loading the kernelmodule successful. >> >> Signed-off-by: Philipp Rossak <embed3d@gmail.com> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- >> arch/arm/boot/dts/sun6i-a31.dtsi | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi >> b/arch/arm/boot/dts/sun6i-a31.dtsi >> index f3425a66fc0a..933a825bf460 100644 >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi >> @@ -1417,5 +1417,16 @@ p2wi: i2c@1f03400 { >> #address-cells = <1>; >> #size-cells = <0>; >> }; >> + >> + gpu: gpu@1c400000 { >> + compatible = "allwinner,sun8i-a31-sgx544-115", looks like a copy paste error from my side this should be allwinner,sun6i-a31-sgx544-115 >> + "img,sgx544-115", "img,sgx544"; >> + reg = <0x01c40000 0x10000>; >> + /* >> + * This node is currently a placeholder for the gpu. >> + * This will be completed when a full demonstration >> + * of the openpvrsgx driver is available for this board. >> + */ > > This node doesn't have clocks, so I don't see how it'd work. > > Better delay the introduction of the GPU node for this board until you > know it works. > > -Paul This was already discuss in an earlier version that series that this should be delayed. I will send a follow up patch series, as soon as I mainlined an other driver that I'm working on, which is required to properly describe the gpu. Cheers Philipp
On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote: > Hi Paul, > > On 2020-04-26 5:56 a.m., Paul Cercueil wrote: > > > > > > Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > >> From: Jonathan Bakker <xc-racer2@live.ca> > >> > >> All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached. > >> > >> There is no external regulator for it so it can be enabled by default. > >> > >> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> > >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > >> --- > >> arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++ > >> 1 file changed, 13 insertions(+) > >> > >> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi > >> index 2ad642f51fd9..abbdda205c1b 100644 > >> --- a/arch/arm/boot/dts/s5pv210.dtsi > >> +++ b/arch/arm/boot/dts/s5pv210.dtsi > >> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 { > >> #interrupt-cells = <1>; > >> }; > >> > >> + gpu: gpu@f3000000 { > >> + compatible = "samsung,s5pv210-sgx540-120"; This should not pass the bindings check because you missed last compatibles. > >> + reg = <0xf3000000 0x10000>; > >> + interrupt-parent = <&vic2>; > >> + interrupts = <10>; > >> + clock-names = "core"; > >> + clocks = <&clocks CLK_G3D>; > >> + > >> + assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>; > >> + assigned-clock-rates = <0>, <66700000>; > >> + assigned-clock-parents = <&clocks MOUT_MPLL>; > > > > What are these clocks for, and why are they reparented / reclocked? > > > > Shouldn't they be passed to 'clocks' as well? > > > > -Paul > > > > The G3D clock system can have multiple parents, and for stable operation > it's recommended to use the MPLL clock as the parent (which in turn > is actually a mux as well). MOUT_G3D is simply the mux for CLK_G3D > (SGX core clock), DOUT_G3D is the divider. DOUT_G3D could equally be CLK_G3D > (and probably should be, for readability) as CLK_G3D is simply the gate and > DOUT_G3D is the divider for it. Good point, it should be CLK_G3D instead of DOUT. Can you fix this as well? Best regards, Krzysztof
Hi Paul, > Am 26.04.2020 um 14:50 schrieb Paul Cercueil <paul@crapouillou.net>: > > Hi Nikolaus, > > Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >> Add SGX GPU node with interrupt. Tested on PandaBoard ES. >> 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. >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- >> arch/arm/boot/dts/omap4.dtsi | 11 ++++++----- >> arch/arm/boot/dts/omap4470.dts | 15 +++++++++++++++ >> 2 files changed, 21 insertions(+), 5 deletions(-) >> create mode 100644 arch/arm/boot/dts/omap4470.dts >> diff --git a/arch/arm/boot/dts/omap4.dtsi b/arch/arm/boot/dts/omap4.dtsi >> index 763bdea8c829..15ff3d7146af 100644 >> --- a/arch/arm/boot/dts/omap4.dtsi >> +++ b/arch/arm/boot/dts/omap4.dtsi >> @@ -389,7 +389,7 @@ abb_iva: regulator-abb-iva { >> status = "disabled"; >> }; >> - target-module@56000000 { >> + sgx_module: target-module@56000000 { >> compatible = "ti,sysc-omap4", "ti,sysc"; >> reg = <0x5600fe00 0x4>, >> <0x5600fe10 0x4>; >> @@ -408,10 +408,11 @@ target-module@56000000 { >> #size-cells = <1>; >> ranges = <0 0x56000000 0x2000000>; >> - /* >> - * Closed source PowerVR driver, no child device >> - * binding or driver in mainline >> - */ >> + gpu: gpu@0 { >> + compatible = "ti,omap4-sgx540-120", "img,sgx540-120", "img,sgx540"; >> + reg = <0x0 0x2000000>; /* 32MB */ >> + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; >> + }; >> }; >> /* >> diff --git a/arch/arm/boot/dts/omap4470.dts b/arch/arm/boot/dts/omap4470.dts >> new file mode 100644 >> index 000000000000..f29c581300bf >> --- /dev/null >> +++ b/arch/arm/boot/dts/omap4470.dts ^^^ there is also a missing "i" in the file name >> @@ -0,0 +1,15 @@ >> +// 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 { > > Does this even compile? Good question. So far there is no well known eval board in mainline that #includes this .dtsi (because it is new) and therefore it passes any compile tests. DTC arch/arm/boot/dts/omap4470-test.dtb - due to target missing Error: arch/arm/boot/dts/omap4470.dtsi:13.1-5 Label or path sgx not found I have now added a dummy board (not to be mainlined) for my own compile test... > > The node's handle is named sgx_module, not sgx. Indeed. A fix is queued for v8. BR and thanks, Nikolaus
Hi Krzysztof, On 2020-04-27 8:46 a.m., Krzysztof Kozlowski wrote: > On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote: >> Hi Paul, >> >> On 2020-04-26 5:56 a.m., Paul Cercueil wrote: >>> >>> >>> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >>>> From: Jonathan Bakker <xc-racer2@live.ca> >>>> >>>> All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached. >>>> >>>> There is no external regulator for it so it can be enabled by default. >>>> >>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> >>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>> --- >>>> arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++ >>>> 1 file changed, 13 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi >>>> index 2ad642f51fd9..abbdda205c1b 100644 >>>> --- a/arch/arm/boot/dts/s5pv210.dtsi >>>> +++ b/arch/arm/boot/dts/s5pv210.dtsi >>>> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 { >>>> #interrupt-cells = <1>; >>>> }; >>>> >>>> + gpu: gpu@f3000000 { >>>> + compatible = "samsung,s5pv210-sgx540-120"; > > This should not pass the bindings check because you missed last > compatibles. > Thanks for pointing that out, I'll add it and make sure it passes the bindings check. >>>> + reg = <0xf3000000 0x10000>; >>>> + interrupt-parent = <&vic2>; >>>> + interrupts = <10>; >>>> + clock-names = "core"; >>>> + clocks = <&clocks CLK_G3D>; >>>> + >>>> + assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>; >>>> + assigned-clock-rates = <0>, <66700000>; >>>> + assigned-clock-parents = <&clocks MOUT_MPLL>; >>> >>> What are these clocks for, and why are they reparented / reclocked? >>> >>> Shouldn't they be passed to 'clocks' as well? >>> >>> -Paul >>> >> >> The G3D clock system can have multiple parents, and for stable operation >> it's recommended to use the MPLL clock as the parent (which in turn >> is actually a mux as well). MOUT_G3D is simply the mux for CLK_G3D >> (SGX core clock), DOUT_G3D is the divider. DOUT_G3D could equally be CLK_G3D >> (and probably should be, for readability) as CLK_G3D is simply the gate and >> DOUT_G3D is the divider for it. > > Good point, it should be CLK_G3D instead of DOUT. Can you fix this as > well? Yep, will do. Nikolaus, I'll send you an updated patch to include. > > Best regards, > Krzysztof > Thanks, Jonathan
Hi all, On 2020-04-28 2:39 p.m., Jonathan Bakker wrote: > Hi Krzysztof, > > On 2020-04-27 8:46 a.m., Krzysztof Kozlowski wrote: >> On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote: >>> Hi Paul, >>> >>> On 2020-04-26 5:56 a.m., Paul Cercueil wrote: >>>> >>>> >>>> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit : >>>>> From: Jonathan Bakker <xc-racer2@live.ca> >>>>> >>>>> All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached. >>>>> >>>>> There is no external regulator for it so it can be enabled by default. >>>>> >>>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> >>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>>> --- >>>>> arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++ >>>>> 1 file changed, 13 insertions(+) >>>>> >>>>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi >>>>> index 2ad642f51fd9..abbdda205c1b 100644 >>>>> --- a/arch/arm/boot/dts/s5pv210.dtsi >>>>> +++ b/arch/arm/boot/dts/s5pv210.dtsi >>>>> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 { >>>>> #interrupt-cells = <1>; >>>>> }; >>>>> >>>>> + gpu: gpu@f3000000 { >>>>> + compatible = "samsung,s5pv210-sgx540-120"; >> >> This should not pass the bindings check because you missed last >> compatibles. >> > > Thanks for pointing that out, I'll add it and make sure it passes the bindings check. > >>>>> + reg = <0xf3000000 0x10000>; >>>>> + interrupt-parent = <&vic2>; >>>>> + interrupts = <10>; >>>>> + clock-names = "core"; >>>>> + clocks = <&clocks CLK_G3D>; >>>>> + >>>>> + assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>; >>>>> + assigned-clock-rates = <0>, <66700000>; >>>>> + assigned-clock-parents = <&clocks MOUT_MPLL>; >>>> >>>> What are these clocks for, and why are they reparented / reclocked? >>>> >>>> Shouldn't they be passed to 'clocks' as well? >>>> >>>> -Paul >>>> >>> >>> The G3D clock system can have multiple parents, and for stable operation >>> it's recommended to use the MPLL clock as the parent (which in turn >>> is actually a mux as well). MOUT_G3D is simply the mux for CLK_G3D >>> (SGX core clock), DOUT_G3D is the divider. DOUT_G3D could equally be CLK_G3D >>> (and probably should be, for readability) as CLK_G3D is simply the gate and >>> DOUT_G3D is the divider for it. >> >> Good point, it should be CLK_G3D instead of DOUT. Can you fix this as >> well? > > Yep, will do. Nikolaus, I'll send you an updated patch to include. > How are assigned-clocks handled in the yaml DT schema? When running make dtbs_check, I end up with messages such as arch/arm/boot/dts/s5pv210-aquila.dt.yaml: gpu@f3000000: 'assigned-clock-parents', 'assigned-clock-rates', 'assigned-clocks' do not match any of the regexes: 'pinctrl-[0-9]+' Do they need to explicitly be listed as valid entries? Thanks, Jonathan
Hi, On Tue, Apr 28, 2020 at 03:58:03PM -0700, Jonathan Bakker wrote: > On 2020-04-28 2:39 p.m., Jonathan Bakker wrote: > > On 2020-04-27 8:46 a.m., Krzysztof Kozlowski wrote: > >> On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote: > >>> Hi Paul, > >>> > >>> On 2020-04-26 5:56 a.m., Paul Cercueil wrote: > >>>> > >>>> > >>>> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller <hns@goldelico.com> a écrit : > >>>>> From: Jonathan Bakker <xc-racer2@live.ca> > >>>>> > >>>>> All s5pv210 devices have a PowerVR SGX 540 (revision 120) attached. > >>>>> > >>>>> There is no external regulator for it so it can be enabled by default. > >>>>> > >>>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> > >>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > >>>>> --- > >>>>> arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++ > >>>>> 1 file changed, 13 insertions(+) > >>>>> > >>>>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi b/arch/arm/boot/dts/s5pv210.dtsi > >>>>> index 2ad642f51fd9..abbdda205c1b 100644 > >>>>> --- a/arch/arm/boot/dts/s5pv210.dtsi > >>>>> +++ b/arch/arm/boot/dts/s5pv210.dtsi > >>>>> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 { > >>>>> #interrupt-cells = <1>; > >>>>> }; > >>>>> > >>>>> + gpu: gpu@f3000000 { > >>>>> + compatible = "samsung,s5pv210-sgx540-120"; > >> > >> This should not pass the bindings check because you missed last > >> compatibles. > >> > > > > Thanks for pointing that out, I'll add it and make sure it passes the bindings check. > > > >>>>> + reg = <0xf3000000 0x10000>; > >>>>> + interrupt-parent = <&vic2>; > >>>>> + interrupts = <10>; > >>>>> + clock-names = "core"; > >>>>> + clocks = <&clocks CLK_G3D>; > >>>>> + > >>>>> + assigned-clocks = <&clocks MOUT_G3D>, <&clocks DOUT_G3D>; > >>>>> + assigned-clock-rates = <0>, <66700000>; > >>>>> + assigned-clock-parents = <&clocks MOUT_MPLL>; > >>>> > >>>> What are these clocks for, and why are they reparented / reclocked? > >>>> > >>>> Shouldn't they be passed to 'clocks' as well? > >>>> > >>>> -Paul > >>>> > >>> > >>> The G3D clock system can have multiple parents, and for stable operation > >>> it's recommended to use the MPLL clock as the parent (which in turn > >>> is actually a mux as well). MOUT_G3D is simply the mux for CLK_G3D > >>> (SGX core clock), DOUT_G3D is the divider. DOUT_G3D could equally be CLK_G3D > >>> (and probably should be, for readability) as CLK_G3D is simply the gate and > >>> DOUT_G3D is the divider for it. > >> > >> Good point, it should be CLK_G3D instead of DOUT. Can you fix this as > >> well? > > > > Yep, will do. Nikolaus, I'll send you an updated patch to include. > > > > How are assigned-clocks handled in the yaml DT schema? When running make dtbs_check, > I end up with messages such as > > arch/arm/boot/dts/s5pv210-aquila.dt.yaml: gpu@f3000000: 'assigned-clock-parents', 'assigned-clock-rates', 'assigned-clocks' do not match any of the regexes: 'pinctrl-[0-9]+' > > Do they need to explicitly be listed as valid entries? If you have additionalProperties set to false, yes. But you should really consider not using them in the first place, since they provide no guarantee on whether the setup you did in the DT will remain during the life of the system. I'm not sure why it's needed on that SoC in particular, but this should be fixed in the driver itself (either the clock or the GPU driver). Maxime
Hi Jonathan, Le mar. 28 avril 2020 à 15:58, Jonathan Bakker <xc-racer2@live.ca> a écrit : > Hi all, > > On 2020-04-28 2:39 p.m., Jonathan Bakker wrote: >> Hi Krzysztof, >> >> On 2020-04-27 8:46 a.m., Krzysztof Kozlowski wrote: >>> On Sun, Apr 26, 2020 at 07:57:12AM -0700, Jonathan Bakker wrote: >>>> Hi Paul, >>>> >>>> On 2020-04-26 5:56 a.m., Paul Cercueil wrote: >>>>> >>>>> >>>>> Le ven. 24 avril 2020 à 22:34, H. Nikolaus Schaller >>>>> <hns@goldelico.com> a écrit : >>>>>> From: Jonathan Bakker <xc-racer2@live.ca> >>>>>> >>>>>> All s5pv210 devices have a PowerVR SGX 540 (revision 120) >>>>>> attached. >>>>>> >>>>>> There is no external regulator for it so it can be enabled by >>>>>> default. >>>>>> >>>>>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca> >>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >>>>>> --- >>>>>> arch/arm/boot/dts/s5pv210.dtsi | 13 +++++++++++++ >>>>>> 1 file changed, 13 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm/boot/dts/s5pv210.dtsi >>>>>> b/arch/arm/boot/dts/s5pv210.dtsi >>>>>> index 2ad642f51fd9..abbdda205c1b 100644 >>>>>> --- a/arch/arm/boot/dts/s5pv210.dtsi >>>>>> +++ b/arch/arm/boot/dts/s5pv210.dtsi >>>>>> @@ -512,6 +512,19 @@ vic3: interrupt-controller@f2300000 { >>>>>> #interrupt-cells = <1>; >>>>>> }; >>>>>> >>>>>> + gpu: gpu@f3000000 { >>>>>> + compatible = "samsung,s5pv210-sgx540-120"; >>> >>> This should not pass the bindings check because you missed last >>> compatibles. >>> >> >> Thanks for pointing that out, I'll add it and make sure it passes >> the bindings check. >> >>>>>> + reg = <0xf3000000 0x10000>; >>>>>> + interrupt-parent = <&vic2>; >>>>>> + interrupts = <10>; >>>>>> + clock-names = "core"; >>>>>> + clocks = <&clocks CLK_G3D>; >>>>>> + >>>>>> + assigned-clocks = <&clocks MOUT_G3D>, <&clocks >>>>>> DOUT_G3D>; >>>>>> + assigned-clock-rates = <0>, <66700000>; >>>>>> + assigned-clock-parents = <&clocks MOUT_MPLL>; >>>>> >>>>> What are these clocks for, and why are they reparented / >>>>> reclocked? >>>>> >>>>> Shouldn't they be passed to 'clocks' as well? >>>>> >>>>> -Paul >>>>> >>>> >>>> The G3D clock system can have multiple parents, and for stable >>>> operation >>>> it's recommended to use the MPLL clock as the parent (which in >>>> turn >>>> is actually a mux as well). MOUT_G3D is simply the mux for >>>> CLK_G3D >>>> (SGX core clock), DOUT_G3D is the divider. DOUT_G3D could >>>> equally be CLK_G3D >>>> (and probably should be, for readability) as CLK_G3D is simply >>>> the gate and >>>> DOUT_G3D is the divider for it. >>> >>> Good point, it should be CLK_G3D instead of DOUT. Can you fix >>> this as >>> well? >> >> Yep, will do. Nikolaus, I'll send you an updated patch to include. >> > > How are assigned-clocks handled in the yaml DT schema? When running > make dtbs_check, > I end up with messages such as > > arch/arm/boot/dts/s5pv210-aquila.dt.yaml: gpu@f3000000: > 'assigned-clock-parents', 'assigned-clock-rates', 'assigned-clocks' > do not match any of the regexes: 'pinctrl-[0-9]+' > > Do they need to explicitly be listed as valid entries? The assigned-* can also be moved inside the node of the clocks provider. I would say it makes more sense to have them there. -Paul