Re: [PATCH v4 4/5] arm64: dts: renesas: r9a09g057: Add IVC and ISP nodes

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Linus

On Thu, Oct 15, 2026 at 02:36:00PM +0200, Linus Walleij wrote:
> Hi Jacopo,
>
> On Mon, Oct 12, 2026 at 5:39 PM Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
>
> > How was this patch tested ?
>
> I was using the TPG only so far, a camera module is in the mail.
>
> I do have the RPi camera module 3 but didn't get it to work, this
> may explain why...
>
> > > +             ivc: isp-input@16040000 {
> > > +                     compatible = "renesas,r9a09g057-ivc";
> > > +                     reg = <0 0x16040000 0 0x10000>;
> > > +                     clocks = <&cpg CPG_MOD 0xe3>, <&cpg CPG_MOD 0xe4>,
> > > +                              <&cpg CPG_MOD 0xe5>;
> >
> > Compared to Dan's original patch I was carrying my tree the first and
> > third ids of clock and reset properties in the ivc and isp nodes respectivelty
> > are swapped
>
> OK
>
> > -                       resets = <&cpg 0xd3>, <&cpg 0xd1>, <&cpg 0xd2>;
> > +                       resets = <&cpg 0xd2>, <&cpg 0xd1>, <&cpg 0xd3>;
> >                         reset-names = "vresetn", "aresetn", "hresetn";
> >                         status = "disabled";
> >
> > Fixes the issue for me
>
> OK
>
> > Why is this an issue it's not 100% clear to me.
>
> Can you try this?
>
> diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> --- a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> @@ -703,6 +703,12 @@
>                 return ret;
>         }
>
> +       /*
> +        * Allow the ISP's 2500-vclk reset recovery time.
> +        * 200 us cover the RZ/V2H's documented minimum of 20 MHz.
> +        */
> +       usleep_range(200, 300);
> +
>         /* Use "software only" context management. */
>         mali_c55_update_bits(mali_c55, MALI_C55_REG_MCU_CONFIG,
>                              MALI_C55_REG_MCU_CONFIG_OVERRIDE_MASK, 0x01);
>
> When we now retrieve the reset and assert it, it matters that
> we give the hardware time to recover. (If this works I will make
> a separate patch.)

Unfortunately this doesn't help

>
> > I can't access the "additional documentation" from the Renesas website
> > where the clock/reset ids are reported. Could you please check ?
>

I mapped the dts entries to the clock/reset signals.

This is what is implemented by your patch:

* means the clock/reset is only handled by the IVC/ISP drivers
** means the clock/reset is handled by both the IVC/ISP drivers

(why the IVC has three entries as well, it's not 100% clear to me, but
this is unrelated)

ivc:
   clocks:
   - reg: e3 (227)  -> CGC_ISP_0_pclk         *
   - axi: e4 (228)  -> CGC_ISP_0_vin_aclk     **
   - isp: e5 (229)  -> CGC_ISP_0_isp_sclk     **

   resets:
   - reg: d4 (212)  -> ISP_0_PRESETN          *
   - axi: d1 (209)  -> ISP_0_REG_ARESETN      **
   - isp: d3 (211)  -> ISP_0_ISP_SRESETN      **

isp:
   clocks:
   - vclk e5  (229)  -> CGC_ISP_0_isp_sclk    **
   - aclk e4  (228)  -> CGC_ISP_0_vin_aclk    **
   - hclk e2  (226)  -> CGC_ISP_0_reg_aclk    *

   resets:
   - vresetn d3 (211) -> ISP_0_ISP_SRESETN    **
   - aresetn d1 (209) -> ISP_0_VIN_ARESETN    **
   - hresetn d2 (210) -> ISP_0_REG_ARESETN    *

As libcamera calls start_streaming on the IVC first the sequence in
which resets/clocks are bulk_enabled is:

* = number of time the clock/reset signal has been enabled:

        [IVC        ][ISP          ]
        [e3* e4* e5*][e5** e4** e2*]

        [IVC        ][ISP          ]
        [d4* d1* d3*][d3** d1** d2*]

This results in the following overall ordering

        e3: CGC_ISP_0_pclk
        e4: CGC_ISP_0_vin_aclk
        e5: CGC_ISP_0_isp_sclk
        e2: CGC_ISP_0_reg_aclk

        d4: ISP_0_PRESETN
        d1: ISP_0_REG_ARESETN
        d3: ISP_0_ISP_SRESETN
        d2: ISP_0_REG_ARESETN

Now, my proposed patch switches e5 with e2 and d3 with d2

        [IVC        ][ISP          ]
        [e3* e4* e5*][e2* e4** e5**]

        [IVC        ][ISP          ]
        [d4* d1* d3*][d2* d1** d3**]

What I don't get is why this should make any difference as the overall
ordering wouldn't change. When the IVC resumes the 0xe5 clock will
anyway be enabled before the 0xe2 one, regardless of the order in
which they are specified in the isp node.

        [IVC    ][ISP]
        e3 e4 e5  e2
        d4 d1 d3  d2

Unless I don't understand something about the enablement of
reset/clocks with multiple users...

Also the only indication I find on the V2H manual on the expected
enablment ordering is:

    All clocks should be turned on and off simultaneously.

Something the doesn't happen today. Maybe we're *very* lucky that
enabling e2 first in the ISP driver matches some window that is missed
by enabling it last, but this seems very very far fetched, isn't it ?

> These changes contradicts the manual so we can't change things
> like this unless we know more, it'd be too much trial-and-horror.
>
> The manual says:
>
> Video/ISP core
> 0xe5, isp_sclk
> 0xd3, isp_sresetn
>
> Register-interface bridge
> 0xe2, reg_aclk
> 0xd2, reg_aresetn

Do you maybe have any information on how the above signal maps to the
C55 clocks/reset and if there's any indication that might help better
understand the issue ?

>
> Yours,
> Linus Walleij