[PATCH 0/3] clk: xilinx: vcu: Sequence update and couple of fixes

[PATCH 0/3] clk: xilinx: vcu: Sequence update and couple of fixes

[Rohit Visavalia]

This patch series updates VCU init/reset sequence and does 2 driver fixes.
Patch1 adds optional reset-gpio support.
Patch2 fixes VCU parent clock issue by preventing pll_ref to be changed.
patch3 fixes call trace related to pll_post clk.

Rohit Visavalia (3):
  clk: xilinx: vcu: Update vcu init/reset sequence
  clk: xilinx: vcu: don't set pll_ref as parent of VCU(enc/dec) clocks
  clk: xilinx: vcu: unregister pll_post only if registered correctly

 drivers/clk/xilinx/xlnx_vcu.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

-- 
2.25.1

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

[Rohit Visavalia]

Updated vcu init/reset sequence as per design changes.
If VCU reset GPIO is available then do assert and de-assert it before
enabling/disabling gasket isolation.
This GPIO is added because gasket isolation will be removed during startup
that requires access to SLCR register space. Post startup, the ownership of
the register interface lies with logiCORE IP

Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com>
---
 drivers/clk/xilinx/xlnx_vcu.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

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
@@ -11,6 +11,7 @@
 #include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/errno.h>
+#include <linux/gpio/consumer.h>
 #include <linux/io.h>
 #include <linux/mfd/syscon.h>
 #include <linux/mfd/syscon/xlnx-vcu.h>
@@ -51,6 +52,7 @@
  * @dev: Platform device
  * @pll_ref: pll ref clock source
  * @aclk: axi clock source
+ * @reset_gpio: vcu reset gpio
  * @logicore_reg_ba: logicore reg base address
  * @vcu_slcr_ba: vcu_slcr Register base address
  * @pll: handle for the VCU PLL
@@ -61,6 +63,7 @@ struct xvcu_device {
 	struct device *dev;
 	struct clk *pll_ref;
 	struct clk *aclk;
+	struct gpio_desc *reset_gpio;
 	struct regmap *logicore_reg_ba;
 	void __iomem *vcu_slcr_ba;
 	struct clk_hw *pll;
@@ -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");
+		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");
+	}
+
 	regmap_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, VCU_GASKET_VALUE);
 
 	ret = xvcu_register_clock_provider(xvcu);
@@ -690,6 +711,7 @@ static int xvcu_probe(struct platform_device *pdev)
 
 error_clk_provider:
 	xvcu_unregister_clock_provider(xvcu);
+error_get_gpio:
 	clk_disable_unprepare(xvcu->aclk);
 	return ret;
 }
@@ -711,6 +733,13 @@ static void xvcu_remove(struct platform_device *pdev)
 	xvcu_unregister_clock_provider(xvcu);
 
 	/* Add the Gasket isolation and put the VCU in reset. */
+	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);
+	}
 	regmap_write(xvcu->logicore_reg_ba, VCU_GASKET_INIT, 0);
 
 	clk_disable_unprepare(xvcu->aclk);
-- 
2.25.1

[PATCH 2/3] clk: xilinx: vcu: don't set pll_ref as parent of VCU(enc/dec) clocks

[Rohit Visavalia]

CCF will try to adjust parent clock to set desire clock frequency of
child clock. So if pll_ref is not a fixed-clock then while setting rate
of enc/dec clocks pll_ref may get change, which may make VCU malfunction.

Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com>
---
 drivers/clk/xilinx/xlnx_vcu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
index f294a2398cb4..c3a4df7e325a 100644
--- a/drivers/clk/xilinx/xlnx_vcu.c
+++ b/drivers/clk/xilinx/xlnx_vcu.c
@@ -550,7 +550,7 @@ static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
 		return PTR_ERR(hw);
 	xvcu->pll_post = hw;
 
-	parent_data[0].fw_name = "pll_ref";
+	parent_data[0].fw_name = "dummy_name";
 	parent_data[1].hw = xvcu->pll_post;
 
 	hws[CLK_XVCU_ENC_CORE] =
-- 
2.25.1

[PATCH 3/3] clk: xilinx: vcu: unregister pll_post only if registered correctly

[Rohit Visavalia]

If registration of pll_post is failed, it will be set to NULL or ERR,
unregistering same will fail with following call trace:

Unable to handle kernel NULL pointer dereference at virtual address 008
pc : clk_hw_unregister+0xc/0x20
lr : clk_hw_unregister_fixed_factor+0x18/0x30
sp : ffff800011923850
...
Call trace:
 clk_hw_unregister+0xc/0x20
 clk_hw_unregister_fixed_factor+0x18/0x30
 xvcu_unregister_clock_provider+0xcc/0xf4 [xlnx_vcu]
 xvcu_probe+0x2bc/0x53c [xlnx_vcu]

Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com>
---
 drivers/clk/xilinx/xlnx_vcu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
index c3a4df7e325a..365c64384ebe 100644
--- a/drivers/clk/xilinx/xlnx_vcu.c
+++ b/drivers/clk/xilinx/xlnx_vcu.c
@@ -590,8 +590,8 @@ static void xvcu_unregister_clock_provider(struct xvcu_device *xvcu)
 		xvcu_clk_hw_unregister_leaf(hws[CLK_XVCU_ENC_MCU]);
 	if (!IS_ERR_OR_NULL(hws[CLK_XVCU_ENC_CORE]))
 		xvcu_clk_hw_unregister_leaf(hws[CLK_XVCU_ENC_CORE]);
-
-	clk_hw_unregister_fixed_factor(xvcu->pll_post);
+	if (!IS_ERR_OR_NULL(xvcu->pll_post))
+		clk_hw_unregister_fixed_factor(xvcu->pll_post);
 }
 
 /**
-- 
2.25.1

Re: [PATCH 2/3] clk: xilinx: vcu: don't set pll_ref as parent of VCU(enc/dec) clocks

[Stephen Boyd]

Quoting Rohit Visavalia (2024-12-26 04:20:22)
> CCF will try to adjust parent clock to set desire clock frequency of
> child clock. So if pll_ref is not a fixed-clock then while setting rate
> of enc/dec clocks pll_ref may get change, which may make VCU malfunction.
> 
> Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com>
> ---
>  drivers/clk/xilinx/xlnx_vcu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/xilinx/xlnx_vcu.c b/drivers/clk/xilinx/xlnx_vcu.c
> index f294a2398cb4..c3a4df7e325a 100644
> --- a/drivers/clk/xilinx/xlnx_vcu.c
> +++ b/drivers/clk/xilinx/xlnx_vcu.c
> @@ -550,7 +550,7 @@ static int xvcu_register_clock_provider(struct xvcu_device *xvcu)
>                 return PTR_ERR(hw);
>         xvcu->pll_post = hw;
>  
> -       parent_data[0].fw_name = "pll_ref";
> +       parent_data[0].fw_name = "dummy_name";

"dummy_name" isn't part of the binding. It sounds like you want to not
set CLK_SET_RATE_PARENT flag sometimes?

>         parent_data[1].hw = xvcu->pll_post;
>  
>         hws[CLK_XVCU_ENC_CORE] =

Re: [PATCH 3/3] clk: xilinx: vcu: unregister pll_post only if registered correctly

[Stephen Boyd]

Quoting Rohit Visavalia (2024-12-26 04:20:23)
> If registration of pll_post is failed, it will be set to NULL or ERR,
> unregistering same will fail with following call trace:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 008
> pc : clk_hw_unregister+0xc/0x20
> lr : clk_hw_unregister_fixed_factor+0x18/0x30
> sp : ffff800011923850
> ...
> Call trace:
>  clk_hw_unregister+0xc/0x20
>  clk_hw_unregister_fixed_factor+0x18/0x30
>  xvcu_unregister_clock_provider+0xcc/0xf4 [xlnx_vcu]
>  xvcu_probe+0x2bc/0x53c [xlnx_vcu]
> 
> Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com>

Please add a Fixes tag, and put fixes earlier in the series so they can
be applied while other patches go through review.

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

[Stephen Boyd]

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.

> +               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?

RE: [PATCH 2/3] clk: xilinx: vcu: don't set pll_ref as parent of VCU(enc/dec) clocks

[Visavalia, Rohit]

Hi Stephen,

Thanks for the review.

>-----Original Message-----
>From: Stephen Boyd <sboyd@kernel.org>
>Sent: Tuesday, December 31, 2024 6:06 AM
>To: Visavalia, Rohit <rohit.visavalia@amd.com>; Simek, Michal
><michal.simek@amd.com>; mturquette@baylibre.com; Sagar, Vishal
><vishal.sagar@amd.com>
>Cc: javier.carrasco.cruz@gmail.com; geert+renesas@glider.be; u.kleine-
>koenig@baylibre.com; linux-clk@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Visavalia, Rohit
><rohit.visavalia@amd.com>
>Subject: Re: [PATCH 2/3] clk: xilinx: vcu: don't set pll_ref as parent of
>VCU(enc/dec) clocks
>
>Quoting Rohit Visavalia (2024-12-26 04:20:22)
>> CCF will try to adjust parent clock to set desire clock frequency of
>> child clock. So if pll_ref is not a fixed-clock then while setting
>> rate of enc/dec clocks pll_ref may get change, which may make VCU
>malfunction.
>>
>> Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com>
>> ---
>>  drivers/clk/xilinx/xlnx_vcu.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/xilinx/xlnx_vcu.c
>> b/drivers/clk/xilinx/xlnx_vcu.c index f294a2398cb4..c3a4df7e325a
>> 100644
>> --- a/drivers/clk/xilinx/xlnx_vcu.c
>> +++ b/drivers/clk/xilinx/xlnx_vcu.c
>> @@ -550,7 +550,7 @@ static int xvcu_register_clock_provider(struct
>xvcu_device *xvcu)
>>                 return PTR_ERR(hw);
>>         xvcu->pll_post = hw;
>>
>> -       parent_data[0].fw_name = "pll_ref";
>> +       parent_data[0].fw_name = "dummy_name";
>
>"dummy_name" isn't part of the binding. It sounds like you want to not set
>CLK_SET_RATE_PARENT flag sometimes?
Yes, if parent rate(pll_ref) has been changed then VCU is not working correctly. So, making name as dummy.

>
>>         parent_data[1].hw = xvcu->pll_post;
>>
>>         hws[CLK_XVCU_ENC_CORE] =

Thanks
Rohit

RE: [PATCH 3/3] clk: xilinx: vcu: unregister pll_post only if registered correctly

[Visavalia, Rohit]

Hi Stephen,

Thanks for the review.

>-----Original Message-----
>From: Stephen Boyd <sboyd@kernel.org>
>Sent: Tuesday, December 31, 2024 6:07 AM
>To: Visavalia, Rohit <rohit.visavalia@amd.com>; Simek, Michal
><michal.simek@amd.com>; mturquette@baylibre.com; Sagar, Vishal
><vishal.sagar@amd.com>
>Cc: javier.carrasco.cruz@gmail.com; geert+renesas@glider.be; u.kleine-
>koenig@baylibre.com; linux-clk@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Visavalia, Rohit
><rohit.visavalia@amd.com>
>Subject: Re: [PATCH 3/3] clk: xilinx: vcu: unregister pll_post only if registered
>correctly
>
>Quoting Rohit Visavalia (2024-12-26 04:20:23)
>> If registration of pll_post is failed, it will be set to NULL or ERR,
>> unregistering same will fail with following call trace:
>>
>> Unable to handle kernel NULL pointer dereference at virtual address
>> 008 pc : clk_hw_unregister+0xc/0x20 lr :
>> clk_hw_unregister_fixed_factor+0x18/0x30
>> sp : ffff800011923850
>> ...
>> Call trace:
>>  clk_hw_unregister+0xc/0x20
>>  clk_hw_unregister_fixed_factor+0x18/0x30
>>  xvcu_unregister_clock_provider+0xcc/0xf4 [xlnx_vcu]
>> xvcu_probe+0x2bc/0x53c [xlnx_vcu]
>>
>> Signed-off-by: Rohit Visavalia <rohit.visavalia@amd.com>
>
>Please add a Fixes tag, and put fixes earlier in the series so they can be applied
>while other patches go through review.
I will take care in next patch series.

Thanks
Rohit

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

[Visavalia, Rohit]

Hi Stephen,

Thanks for the review. 

>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.

Thanks
Rohit

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

[Geert Uytterhoeven]

Hi Rohit,

On Tue, Dec 31, 2024 at 3:16 PM Visavalia, Rohit
<rohit.visavalia@amd.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?

Regardless, the reset GPIO should be documented in the DT bindings.
And perhaps marked required, so "make dtbs_check" will flag it when
it's missing?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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

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

[Visavalia, Rohit]

Hi Geert,

Thanks for the review.

>-----Original Message-----
>From: Geert Uytterhoeven <geert@linux-m68k.org>
>Sent: Tuesday, December 31, 2024 7:56 PM
>To: Visavalia, Rohit <rohit.visavalia@amd.com>
>Cc: Stephen Boyd <sboyd@kernel.org>; Simek, Michal <michal.simek@amd.com>;
>mturquette@baylibre.com; Sagar, Vishal <vishal.sagar@amd.com>;
>javier.carrasco.cruz@gmail.com; geert+renesas@glider.be; u.kleine-
>koenig@baylibre.com; linux-clk@vger.kernel.org; linux-arm-
>kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH 1/3] clk: xilinx: vcu: Update vcu init/reset sequence
>
>Hi Rohit,
>
>On Tue, Dec 31, 2024 at 3:16 PM Visavalia, Rohit <rohit.visavalia@amd.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.
>
>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.

Thanks
Rohit

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

[Geert Uytterhoeven]

Hi Rohit,

On Thu, Jan 2, 2025 at 8:01 AM Visavalia, Rohit <rohit.visavalia@amd.com> wrote:
> >From: Geert Uytterhoeven <geert@linux-m68k.org>
> >On Tue, Dec 31, 2024 at 3:16 PM Visavalia, Rohit <rohit.visavalia@amd.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.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-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

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

[Visavalia, Rohit]

Hi Geert,

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