RE: Re: Re: Re: Re: [PATCH 1/2] dt-bindings: gpu: mali-valhall-csf: Document i.MX952 support

[Guangliu Ding]

Hi Liviu

Thanks a lot for your sharing.

> On Wed, Apr 01, 2026 at 03:59:23PM +0000, Guangliu Ding wrote:
> > Hi Liviu
> >
> > > On Wed, Apr 01, 2026 at 10:31:01AM +0000, Guangliu Ding wrote:
> > > > Hi Liviu
> > > >
> > > > > On Wed, Apr 01, 2026 at 09:43:12AM +0000, Guangliu Ding wrote:
> > > > > > Hi Daniel
> > > > > >
> > > > > > > On 4/1/26 11:48, Guangliu Ding wrote:
> > > > > > > > [You don't often get email from guangliu.ding@xxxxxxx.
> > > > > > > > Learn why this is important at
> > > > > > > > https://aka.ms/LearnAboutSenderIdentification
> > > > > > > > ]
> > > > > > > >
> > > > > > > > Hi Liviu
> > > > > > > >
> > > > > > > > Thanks for your review. Please refer to my comments below:
> > > > > > > >
> > > > > > > >> On Tue, Mar 31, 2026 at 06:12:38PM +0800, Guangliu Ding
> wrote:
> > > > > > > >>> Add compatible string of Mali G310 GPU on i.MX952 board.
> > > > > > > >>>
> > > > > > > >>> Signed-off-by: Guangliu Ding <guangliu.ding@xxxxxxx>
> > > > > > > >>> Reviewed-by: Jiyu Yang <jiyu.yang@xxxxxxx>
> > > > > > > >>> ---
> > > > > > > >>>
> > > > > > > >>> Documentation/devicetree/bindings/gpu/arm,mali-valhall-c
> > > > > > > >>> sf.y
> > > > > > > >>> aml
> > > > > > > >>> | 1
> > > > > > > >>> +
> > > > > > > >>>  1 file changed, 1 insertion(+)
> > > > > > > >>>
> > > > > > > >>> diff --git
> > > > > > > >>> a/Documentation/devicetree/bindings/gpu/arm,mali-valhall
> > > > > > > >>> -csf
> > > > > > > >>> .yam
> > > > > > > >>> l
> > > > > > > >> b/Documentation/devicetree/bindings/gpu/arm,mali-valhall-csf.
> > > > > > > >> yaml
> > > > > > > >>> index 8eccd4338a2b..6a10843a26e2 100644
> > > > > > > >>> ---
> > > > > > > >>> a/Documentation/devicetree/bindings/gpu/arm,mali-valhall
> > > > > > > >>> -csf
> > > > > > > >>> .yam
> > > > > > > >>> l
> > > > > > > >>> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-val
> > > > > > > >>> +++ hall
> > > > > > > >>> +++ -csf
> > > > > > > >>> +++ .yam
> > > > > > > >>> +++ l
> > > > > > > >>> @@ -20,6 +20,7 @@ properties:
> > > > > > > >>>            - enum:
> > > > > > > >>>                - mediatek,mt8196-mali
> > > > > > > >>>                - nxp,imx95-mali            # G310
> > > > > > > >>> +              - nxp,imx952-mali           # G310
> > > > > > > >> Can you explain why this is needed? Can it not be covered
> > > > > > > >> by the existing compatible?
> > > > > > > > There are functional differences in GPU module (GPUMIX)
> > > > > > > > between
> > > > > > > > i.MX95 and i.MX952. So they cannot be fully covered by a
> > > > > > > > single existing
> > > > > compatible.
> > > > > > > > On i.MX952, The GPU clock is controlled by hardware GPU
> > > > > > > > auto clock-gating mechanism, while the GPU clock is
> > > > > > > > managed explicitly by the
> > > > > > > driver on i.MX95.
> > > > > > > > Because of these behavioral differences, separate
> > > > > > > > compatible strings "nxp,imx95-mali" and "nxp,imx952-mali"
> > > > > > > > are needed to allow the driver to handle the two variants
> > > > > > > > independently and to keep room for future
> > > > > > > divergence.
> > > > > > >
> > > > > > >
> > > > > > > This information should be added in the commit message
> > > > > > > explaining why
> > > > > > >
> > > > > > > the change is needed.
> > > > > > >
> > > > > > >
> > > > > > > But then where is the driver code taking care of these diferences?
> > > > > > >
> > > > > >
> > > > > > Yes. Currently the driver does not require "nxp,imx952-mali" string.
> > > > > > However, when GPU ipa_counters are enabled to calculate the
> > > > > > GPU busy_time/idle_time for GPU DVFS feature, they will
> > > > > > conflict with the hardware GPU auto clock‑gating mechanism,
> > > > > > causing GPU clock to remain
> > > > > always on.
> > > > > > In such cases, ipa_counters need to be disabled so that the
> > > > > > GPU auto clock‑gating mechanism can operate normally, using
> > > "nxp,imx952-mali"
> > > > > string.
> > > > >
> > > > > OK, I understand that you're following guidance from some other
> > > > > senior people on how to upstream patches so you've tried to
> > > > > create the smallest patchset to ensure that it gets reviewed and
> > > > > accepted, but in this case we need to see the other patches as
> > > > > well to decide if your approach is the right one and we do need
> > > > > a separate compatible
> > > string.
> > > > >
> > > > > If enabling GPU ipa_counters causes the clocks to get stuck
> > > > > active, that feels like a hardware bug, so figuring out how to
> > > > > handle that is more important than adding a compatible string.
> > > > >
> > > > > Either add the patch(es) that use the compatible to this series
> > > > > in v2, or put a comment in the commit message on where we can
> > > > > see the
> > > driver changes.
> > > > >
> > > >
> > > > According to discussions with the GPU vendor, this is a hardware
> > > > limitation of Mali-G310 rather than a hardware bug, and it has
> > > > been addressed in newer Mali GPU families.
> > >
> > > I represent the said GPU vendor and I think I know what you're
> > > talking about, but you're taking the wrong approach. All G310s have
> > > a problem where in order to enable access to the ipa_counters the
> > > automatic clock gating gets disabled. So the solution that needs to
> > > be implemented when we add support for IPA_COUNTERs will apply to all
> GPUs, not just MX952.
> >
> > Yes. We have bring-up G310 (V2) GPU on both i.MX95 and i.MX952. And
> > auto clock gating mechanism is firstly introduced in i.MX952 (not supported
> on i.MX95).
> > According to your update, solution needs to be implemented to all GPUs
> > which support auto clock gating mechanism after IPA_COUNTERs are
> supported in the driver, right?
> 
> A solution is needed, yes.
> 
> > What's your suggestions for 952 gpu dtb node?
> 
> There is no IPA_COUNTER use in Panthor at the moment. Unless your DVFS
> controller uses that, I would suggest that we don't introduce a compatible for
> 952 until the time we add support for reading the counters.
> 
> It helps if you think in terms of what is already in upstream, rather than mixing
> with the tests that uses kbase code. Does your hardware need extra code in
> upstream in order to function? If so, where is that code? If not, then let's not
> introduce the compatible until we are absolutely sure we need it because we
> have code specific to that SoC. For everything else we will implement an
> architecture fix if needed.
> 

Got it. The following compatible string is the correct choice since the GPU on
i.MX952 is fully compatible with the GPU on i.MX95 now.
compatible = "nxp,imx95-mali", "arm,mali-valhall-csf";

I will not mix tests with kbase code in the following upstream patches for panthor driver.

> Best regards,
> Liviu
> 
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯