Re: [PATCH v6 00/12] ARM/MIPS: DTS: add child nodes describing the PVRSGX GPU present in some OMAP SoC and JZ4780 (and many more)

[H. Nikolaus Schaller]

Hi Tony and Philip,

> Am 22.04.2020 um 21:33 schrieb Tony Lindgren <tony@xxxxxxxxxxx>:
> 
> * Philipp Rossak <embed3d@xxxxxxxxx> [200422 19:05]:
>> A few years back, I did a big research on the PowerVR GPUs. Back then I
>> found an interesting TI datasheet. I forgot about this till I have seen the
>> right buzz words. Sorry that I remembered it that late.
>> 
>> Back then I came to the conclusion that all PowerVR GPU's have in general 3
>> Clocks.
>> 
>> A system clock, a memory clock and a core clock. [1].

Great!  This is an excerpt of the am335x TRM.
I may have seen this information in the past but also forgot about it.

Indeed, it seems to change a lot of our thinking.

> 
> Hmm I'm not sure if those names are sgx or SoC specific.

It depends. Here is some quick research:

the am335x lists:
  THALIAIRQ, SYSCLK & MEMCLK (connected in parallel), CORECLK

The omap3530 TRM has different information. It names them
  SGX_FCLK, SGX_ICLK, SGX_RST and SGX_IRQ
  but this is likely a TI nomenclature defined by the PRCM wrapper.

DM3730 and OMAP4 and TRM tells the same.

The OMAP5 TRM is interestingly different. It has:
  GPU_ICLK, GPU_FCLK1, GPU_FCLK2, GPU_RST and GPU_IRQ.
  Really surprising is that the PRCM outputs are called
    GPU_L3_GICLK, GPU_CORE_GCLK and GPU_HYS_GCLK.

  I.e. the same "HYD" as we have seen in the A31. It seems to
  be a feature of the sgx544 to have two functional clocks and
  one being called "HYD".

Now I know why it didn't play a role so far. Because the omap5
wrapper hides this detail from the sgx implementation.

Next I checked the AM572x TRM:
 it has also a hyd_clk, a core_clk, sys_clk, some reset and a gpu_irq

The DRA7xx TRM does the same as AM57xx.

So the "hyd" clock seems to be a second functional clock
with unknown function in some SGX variants. It seems to be
something different from the "memclock" of the am335x but may
be the same.

> 
> Anyways, the sgx clocks for omap variants are already handled
> by the ti-sysc module as "fck" and "ick" so nothing to do there.

Which brings back the question if this complexity and not well
defined clocks of the SGX core should really be part of the bindings
any why we have to care about...

What is the benefit of modeling at this level of pretend accuracy?

> 
>> The hyd_clk at sunxi devices seems to be the system clock.
>> 
>> With those additional information it should be very easy to get a proper
>> binding.
> 
> It would be best to find the clock(s) name used in the sgx docs
> to avoid using SoC specific naming :)

If there were specific SGX docs describing the VHDL signal names :)

> 
> But yeah "sysclk" "memclk" and "coreclk" seem just fine for
> me for the optional clocks if that works for other SoCs.

Well, if the other SoC would follow the PRCM/sysc approach
the omap uses, all these clocks would be part of the wrapper
and can be named and numbered as it best fits to the SoC
data sheet and clock control registers.

> 
> Regards,
> 
> Tony
> 
>> [1]: https://github.com/embed-3d/PVRSGX_hwdoc/blob/master/sources/pdfs/Spruh73c_chapter_SGX_Graphics_Accelerator.pdf

So a compromise could be to

* define

  clock-names:
    items:
      - const: core
      - const: mem
      - const: sys
      - const: hyd

* make clocks optional (for omap or others wanting to use a wrapper driver)
* DTs can request the same clock providers for core and hyd or mem if that fits best
* the driver must enable all 4 clocks if they exists

BR and thanks,
Nikolaus