[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.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 .../devicetree/bindings/gpu/img,pvrsgx.txt    | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/img,pvrsgx.txt

On Fri, Oct 18, 2019 at 1:46 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
> 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.

>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  .../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.

> diff --git a/Documentation/devicetree/bindings/gpu/img,pvrsgx.txt b/Documentation/devicetree/bindings/gpu/img,pvrsgx.txt
> new file mode 100644
> index 000000000000..4ad87c075791
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.txt
> @@ -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";

The order here is wrong. Should be most specific first.

Drop 'omap-' from the compatible.

> +
> +               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.

> +
> +               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.

> +
> +
> +- reg:         Physical base addresses and lengths of the register areas.

How many?

> +- reg-names:   Names for the register areas.

If only 1 as the example suggests, then you don't need this.

> +- 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?

> +- img,cores:   number of cores. Defaults to <1>.

Not discoverable?

> +
> +/ {
> +       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

> +                       compatible = "ti,sysc-omap4", "ti,sysc";
> +                       reg = <0x5600fe00 0x4>,
> +                             <0x5600fe10 0x4>;

How does it work that these registers overlap the GPU registers?

> +                       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 {

gpu@...



> +                               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
>

Hi Rob,

> Am 21.10.2019 um 17:07 schrieb Rob Herring <robh+dt@kernel.org>:
> 
> On Fri, Oct 18, 2019 at 1:46 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> 
>> 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.

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.

> 
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> .../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.

Is there a tutorial or a tool to convert?
I have only seen that a new format exists but zero experience.

> 
>> diff --git a/Documentation/devicetree/bindings/gpu/img,pvrsgx.txt b/Documentation/devicetree/bindings/gpu/img,pvrsgx.txt
>> new file mode 100644
>> index 000000000000..4ad87c075791
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.txt
>> @@ -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";
> 
> The order here is wrong. Should be most specific first.
> 
> Drop 'omap-' from the compatible.

Ok, yes. Seems to be redundant since omap is from ti only...

> 
>> +
>> +               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.

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).

> 
>> +
>> +               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.

> 
>> +
>> +
>> +- 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.

> 
>> +- reg-names:   Names for the register areas.
> 
> If only 1 as the example suggests, then you don't need this.

ok.

> 
>> +- 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.

> 
> 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.

> 
>> +- 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.

> 
>> +
>> +/ {
>> +       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...

> 
>> +                       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.

> 
>> +                       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 {
> 
> gpu@...

Yes, is better to name it according to the function.

> 
> 
> 
>> +                               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

BR and thanks,
Nikolaus

* H. Nikolaus Schaller <hns@goldelico.com> [191021 15:46]:
> > Am 21.10.2019 um 17:07 schrieb Rob Herring <robh+dt@kernel.org>:
> > On Fri, Oct 18, 2019 at 1:46 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> >> +- 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.
> 
> > 
> >> +- 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.

> >> +                       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

* H. Nikolaus Schaller <hns@goldelico.com> [191021 15:46]:
> > Am 21.10.2019 um 17:07 schrieb Rob Herring <robh+dt@kernel.org>:
> > On Fri, Oct 18, 2019 at 1:46 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> >> +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.
> 
> > 
> > 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.

> >> +- 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

> Am 21.10.2019 um 19:25 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [191021 15:46]:
>>> Am 21.10.2019 um 17:07 schrieb Rob Herring <robh+dt@kernel.org>:
>>> On Fri, Oct 18, 2019 at 1:46 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>> +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.
>> 
>>> 
>>> 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.

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.

> 
>>>> +- 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

* H. Nikolaus Schaller <hns@goldelico.com> [191021 18:08]:
> 
> > Am 21.10.2019 um 19:25 schrieb Tony Lindgren <tony@atomide.com>:
> > 
> > * H. Nikolaus Schaller <hns@goldelico.com> [191021 15:46]:
> >>> Am 21.10.2019 um 17:07 schrieb Rob Herring <robh+dt@kernel.org>:
> >>> On Fri, Oct 18, 2019 at 1:46 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> >>>> +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.
> >> 
> >>> 
> >>> 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.
> 
> 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.

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.

> >>>> +- 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.

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

Hi Tony,

> Am 22.10.2019 um 17:02 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [191021 18:08]:
>> 
>>> Am 21.10.2019 um 19:25 schrieb Tony Lindgren <tony@atomide.com>:
>>> 
>>> * H. Nikolaus Schaller <hns@goldelico.com> [191021 15:46]:
>>>>> Am 21.10.2019 um 17:07 schrieb Rob Herring <robh+dt@kernel.org>:
>>>>> On Fri, Oct 18, 2019 at 1:46 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
>>>>>> +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.
>>>> 
>>>>> 
>>>>> 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.
>> 
>> 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.
> 
> 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.

> 
>>>>>> +- 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.
> 
> 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.

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

* H. Nikolaus Schaller <hns@goldelico.com> [191022 15:12]:
> 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..

> 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.

> 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

> Am 22.10.2019 um 17:36 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [191022 15:12]:
>> 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.

> 
> 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.

> 
>> 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.
> 
>> 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

* H. Nikolaus Schaller <hns@goldelico.com> [191018 18:47]:
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.txt
> @@ -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";

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

Hi,

> Am 30.10.2019 um 17:16 schrieb Tony Lindgren <tony@atomide.com>:
> 
> * H. Nikolaus Schaller <hns@goldelico.com> [191018 18:47]:
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpu/img,pvrsgx.txt
>> @@ -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";
> 
> 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.

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