On Fri, Feb 28, 2025 at 06:43:53PM +0100, Marek Vasut wrote:
> On 2/28/25 11:36 AM, Alexander Stein wrote:
> > Hi Marek,
> 
> Hi,
> 
> > > diff --git a/arch/arm64/boot/dts/freescale/imx95.dtsi 
> > > b/arch/arm64/boot/dts/freescale/imx95.dtsi
> > > index 3af13173de4bd..36bad211e5558 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx95.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx95.dtsi
> > > @@ -249,6 +249,37 @@ dummy: clock-dummy {
> > >                   clock-output-names = "dummy";
> > >           };
> > > + gpu_fixed_reg: fixed-gpu-reg {
> > > +         compatible = "regulator-fixed";
> > > +         regulator-min-microvolt = <920000>;
> > > +         regulator-max-microvolt = <920000>;
> > > +         regulator-name = "vdd_gpu";
> > > +         regulator-always-on;
> > > +         regulator-boot-on;
> > > + };
> > 
> > Is this an internal voltage?
> 
> I think so.
> 
> > > +
> > > + gpu_opp_table: opp_table {
> > 
> > Node-Names use dash instead of underscore.
> 
> Fixed, thanks.
> 
> [...]
> 
> > > @@ -1846,6 +1877,37 @@ netc_emdio: mdio@0,0 {
> > >                           };
> > >                   };
> > > +         gpu_blk_ctrl: reset-controller@4d810000 {
> > > +                 compatible = "fsl,imx95-gpu-blk-ctrl";
> > > +                 reg = <0x0 0x4d810000 0x0 0xc>;
> > 
> > Mh, GPU_BLK_CTRL is /just a bit) more than the GPU reset. Does it make sense
> > to make this an gpu-reset-only node, located at 0x4d810008?
> 
> The block controller itself is larger, it spans 3 or 4 registers, so this
> should describe the entire block controller here.
> 
> > > +                 #reset-cells = <1>;
> > > +                 clocks = <&scmi_clk IMX95_CLK_GPUAPB>;
> > > +                 assigned-clocks = <&scmi_clk IMX95_CLK_GPUAPB>;
> > > +                 assigned-clock-parents = <&scmi_clk 
> > > IMX95_CLK_SYSPLL1_PFD1_DIV2>;
> > > +                 assigned-clock-rates = <133333333>;
> > > +                 power-domains = <&scmi_devpd IMX95_PD_GPU>;
> > > +                 status = "disabled";
> > > +         };
> > > +
> > > +         gpu: gpu@4d900000 {
> > > +                 compatible = "fsl,imx95-mali", "arm,mali-valhall-csf";
> > > +                 reg = <0 0x4d900000 0 0x480000>;
> > > +                 clocks = <&scmi_clk IMX95_CLK_GPU>;
> > 
> > There is also IMX95_CLK_GPUAPB. Is this only required for the rese control 
> > above?
> 
> I think I have to describe those clock here too, possibly as 'coregroup'
> clock ?

The 'coregroup' clock does indeed control the MMU and L2$ blocks as well as the 
AXI interface,
so if that is indeed a separate external clock source it should be defined. 
Could it be why
you're seeing issues with L2$ resume on the fast reset path?

Best regards,
Liviu

> 
> > > +                 clock-names = "core";
> > > +                 interrupts = <GIC_SPI 288 IRQ_TYPE_LEVEL_HIGH>,
> > > +                              <GIC_SPI 289 IRQ_TYPE_LEVEL_HIGH>,
> > > +                              <GIC_SPI 290 IRQ_TYPE_LEVEL_HIGH>;
> > > +                 interrupt-names = "gpu", "job", "mmu";
> > 
> > DT bindings say this order: job, mmu, gpu
> Yes, currently it is sorted by IRQ number, fixed.

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

Reply via email to