RE: [PATCH 1/3] clk: xilinx: vcu: Update vcu init/reset sequence

[Visavalia, Rohit]

Hi Geert,

>-----Original Message-----
>From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
>Sent: Thursday, January 2, 2025 3:29 PM
>To: Visavalia, Rohit <rohit.visavalia@xxxxxxx>
>Cc: Stephen Boyd <sboyd@xxxxxxxxxx>; Simek, Michal <michal.simek@xxxxxxx>;
>mturquette@xxxxxxxxxxxx; Sagar, Vishal <vishal.sagar@xxxxxxx>;
>javier.carrasco.cruz@xxxxxxxxx; geert+renesas@xxxxxxxxx; u.kleine-
>koenig@xxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; linux-arm-
>kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH 1/3] clk: xilinx: vcu: Update vcu init/reset sequence
>
>Hi Rohit,
>
>On Thu, Jan 2, 2025 at 8:01 AM Visavalia, Rohit <rohit.visavalia@xxxxxxx> wrote:
>> >From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> On Tue, Dec 31, 2024
>> >at 3:16 PM Visavalia, Rohit <rohit.visavalia@xxxxxxx>
>> >wrote:
>> >> >Subject: Re: [PATCH 1/3] clk: xilinx: vcu: Update vcu init/reset
>> >> >sequence Quoting Rohit Visavalia (2024-12-26 04:20:21)
>> >> >> diff --git a/drivers/clk/xilinx/xlnx_vcu.c
>> >> >> b/drivers/clk/xilinx/xlnx_vcu.c index 81501b48412e..f294a2398cb4
>> >> >> 100644
>> >> >> --- a/drivers/clk/xilinx/xlnx_vcu.c
>> >> >> +++ b/drivers/clk/xilinx/xlnx_vcu.c
>> >> >> @@ -676,6 +679,24 @@ static int xvcu_probe(struct platform_device
>*pdev)
>> >> >>          * Bit 0 : Gasket isolation
>> >> >>          * Bit 1 : put VCU out of reset
>> >> >>          */
>> >> >> +       xvcu->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
>> >> >> +                                                  GPIOD_OUT_LOW);
>> >> >> +       if (IS_ERR(xvcu->reset_gpio)) {
>> >> >> +               ret = PTR_ERR(xvcu->reset_gpio);
>> >> >> +               dev_err(&pdev->dev, "failed to get reset gpio
>> >> >> + for vcu.\n");
>> >> >
>> >> >Use dev_err_probe() and friends.
>> >> I will take care in v2 patch series.
>> >>
>> >> >
>> >> >> +               goto error_get_gpio;
>> >> >> +       }
>> >> >> +
>> >> >> +       if (xvcu->reset_gpio) {
>> >> >> +               gpiod_set_value(xvcu->reset_gpio, 0);
>> >> >> +               /* min 2 clock cycle of vcu pll_ref, slowest freq is 33.33KHz */
>> >> >> +               usleep_range(60, 120);
>> >> >> +               gpiod_set_value(xvcu->reset_gpio, 1);
>> >> >> +               usleep_range(60, 120);
>> >> >> +       } else {
>> >> >> +               dev_warn(&pdev->dev, "No reset gpio info from
>> >> >> + dts for vcu. This may lead to incorrect functionality if VCU
>> >> >> + isolation is removed post initialization.\n");
>> >> >
>> >> >Is it 'vcu' or 'VCU'? Pick one please. Also, this is going to be
>> >> >an unfixable warning message if the reset isn't there. Why have this warning
>at all?
>> >>
>> >> I will use 'VCU' in next(v2) patch series.
>> >>
>> >> Added warning just to inform user that if design has the reset gpio
>> >> and it is
>> >
>> >missing in dt node then it could lead to issue.
>> >
>> >If it could lead to issues, shouldn't the reset GPIO be required instead of
>optional?
>>
>> It is marked as optional as few of the Zynqmp designs are having vcu_reset(reset
>pin of VCU IP) is driven by proc_sys_reset. proc_sys_reset is another PL IP driven
>by the PS pl_reset.  So here the VCU reset is not driven by axi_gpio or PS gpio so
>there will be no gpio entry.
>
>OK, then optional is fine.
>
>> >Regardless, the reset GPIO should be documented in the DT bindings.
>>
>> Yes, I will be sending patch for the same.
>>
>> >And perhaps marked required, so "make dtbs_check" will flag it when it's
>missing?
>> I believe rephrasing the warning to "No reset gpio info found in dts for VCU. This
>may result in incorrect functionality if VCU isolation is removed after initialization
>in designs where the VCU reset is driven by gpio." would make it clearer. Let me
>know your thoughts.
>
>If the reset is optional on some systems and required on other systems, there
>should not be a warning, IMHO, unless the driver can detect by some other means
>when the reset is actually required.
There is no way that driver can detect the system(design) connection for reset pin. I will move warning to debug i.e. dev_dbg().

Thanks
Rohit
>
>Gr{oetje,eeting}s,
>
>                        Geert
>
>--
>Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
>In personal conversations with technical people, I call myself a hacker. But when
>I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds