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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
 <CH2PR12MB487527EDE3C4EC2F5A675C34E5142@CH2PR12MB4875.namprd12.prod.outlook.com>
Date: Thu, 2 Jan 2025 10:05:29 +0000
From: "Visavalia, Rohit" <rohit.visavalia@....com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
CC: Stephen Boyd <sboyd@...nel.org>, "Simek, Michal" <michal.simek@....com>,
	"mturquette@...libre.com" <mturquette@...libre.com>, "Sagar, Vishal"
	<vishal.sagar@....com>, "javier.carrasco.cruz@...il.com"
	<javier.carrasco.cruz@...il.com>, "geert+renesas@...der.be"
	<geert+renesas@...der.be>, "u.kleine-koenig@...libre.com"
	<u.kleine-koenig@...libre.com>, "linux-clk@...r.kernel.org"
	<linux-clk@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/3] clk: xilinx: vcu: Update vcu init/reset sequence

Hi Geert,

>-----Original Message-----
>From: Geert Uytterhoeven <geert@...ux-m68k.org>
>Sent: Thursday, January 2, 2025 3:29 PM
>To: Visavalia, Rohit <rohit.visavalia@....com>
>Cc: Stephen Boyd <sboyd@...nel.org>; Simek, Michal <michal.simek@....com>;
>mturquette@...libre.com; Sagar, Vishal <vishal.sagar@....com>;
>javier.carrasco.cruz@...il.com; geert+renesas@...der.be; u.kleine-
>koenig@...libre.com; linux-clk@...r.kernel.org; linux-arm-
>kernel@...ts.infradead.org; linux-kernel@...r.kernel.org
>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@....com> wrote:
>> >From: Geert Uytterhoeven <geert@...ux-m68k.org> On Tue, Dec 31, 2024
>> >at 3:16 PM Visavalia, Rohit <rohit.visavalia@....com>
>> >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@...ux-m68k.org
>
>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