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

[Liviu Dudau <liviu.dudau@arm.com>]

On Wed, Apr 01, 2026 at 06:03:28PM +0000, Guangliu Ding wrote:
> 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@nxp.com.
> > > > > > > > > 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@nxp.com>
> > > > > > > > >>> Reviewed-by: Jiyu Yang <jiyu.yang@nxp.com>
> > > > > > > > >>> ---
> > > > > > > > >>>
> > > > > > > > >>> 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.

OK. I think if you drop patch 2/3 from your series you can send a v3.

Best regards,
Liviu

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