[PATCH] clk: imx8qxp: Defer instead of failing probe

[PATCH] clk: imx8qxp: Defer instead of failing probe

[Diogo Manuel Pais Silva]

When of_clk_parent_fill is run before all the parent clocks have been probed then the probe function will return -EINVAL, making it so that the probe isn't attempted again. As fw_devlink is on by default this does not usually happen, but if fw_devlink is disabled then it is very possible that the parent clock will be probed after the lpcg first attempt, and the lpcg clock will not work.

Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
---
 drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
index d0ccaa040225..520a05ea0bef 100644
--- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
+++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
@@ -225,8 +225,8 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,

        ret = of_clk_parent_fill(np, parent_names, count);
        if (ret != count) {
-               dev_err(&pdev->dev, "failed to get clock parent names\n");
-               return count;
+               dev_warn(&pdev->dev, "failed to get all clock parent names\n");
+               return -EPROBE_DEFER;
        }

        ret = of_property_read_string_array(np, "clock-output-names",
--
2.34.1


TTControl - Internal

RE: [PATCH] clk: imx8qxp: Defer instead of failing probe

[Peng Fan]

> Subject: [PATCH] clk: imx8qxp: Defer instead of failing probe
> 
> When of_clk_parent_fill is run before all the parent clocks have been
> probed then the probe function will return -EINVAL, making it so that
> the probe isn't attempted again. As fw_devlink is on by default this
> does not usually happen, but if fw_devlink is disabled then it is very
> possible that the parent clock will be probed after the lpcg first attempt,
> and the lpcg clock will not work.
> 
> Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
> ---
>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-
> imx8qxp-lpcg.c
> index d0ccaa040225..520a05ea0bef 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -225,8 +225,8 @@ static int imx_lpcg_parse_clks_from_dt(struct
> platform_device *pdev,
> 
>         ret = of_clk_parent_fill(np, parent_names, count);
>         if (ret != count) {
> -               dev_err(&pdev->dev, "failed to get clock parent names\n");
> -               return count;
> +               dev_warn(&pdev->dev, "failed to get all clock parent
> names\n");
> +               return -EPROBE_DEFER;

Use dev_err_probe?

Regards,
Peng.

>         }
> 
>         ret = of_property_read_string_array(np, "clock-output-names",
> --
> 2.34.1
> 
> CONFIDENTIALITY: The contents of this e-mail are confidential and
> intended only for the above addressee(s). If you are not the intended
> recipient, or the person responsible for delivering it to the intended
> recipient, copying or delivering it to anyone else or using it in any
> unauthorized manner is prohibited and may be unlawful. If you receive
> this e-mail by mistake, please notify the sender and the systems
> administrator at straymail@tttech.com immediately.
> 
> TTControl - Internal

[PATCH v2] clk: imx8qxp: Defer instead of failing probe

[Diogo Manuel Pais Silva]

When of_clk_parent_fill is ran without all the parent clocks having been probed then the probe function will return -EINVAL, making it so that the probe isn't attempted again. As fw_devlink is on by default this does not usually happen, but if fw_devlink is disabled then it is very possible that the parent clock will be probed after the lpcg first attempt.

Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
---
v2: change from dev_warn to dev_err_probe
---
 drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
index d0ccaa040225..7bd9b745edbe 100644
--- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
+++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
@@ -225,8 +225,8 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
 
 	ret = of_clk_parent_fill(np, parent_names, count);
 	if (ret != count) {
-		dev_err(&pdev->dev, "failed to get clock parent names\n");
-		return count;
+		return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
+				"failed to get all clock parent names\n");
 	}
 
 	ret = of_property_read_string_array(np, "clock-output-names",
-- 
2.34.1

RE: [PATCH v2] clk: imx8qxp: Defer instead of failing probe

[Peng Fan]

> Subject: [PATCH v2] clk: imx8qxp: Defer instead of failing probe
> 
> When of_clk_parent_fill is ran without all the parent clocks having
> been probed then the probe function will return -EINVAL, making it so
> that the probe isn't attempted again. As fw_devlink is on by default
> this does not usually happen, but if fw_devlink is disabled then it is
> very possible that the parent clock will be probed after the lpcg first
> attempt.
> 
> Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

Re: [PATCH v2] clk: imx8qxp: Defer instead of failing probe

[Abel Vesa]

On 24-07-02 08:10:44, Diogo Manuel Pais Silva wrote:
> When of_clk_parent_fill is ran without all the parent clocks having been probed then the probe function will return -EINVAL, making it so that the probe isn't attempted again. As fw_devlink is on by default this does not usually happen, but if fw_devlink is disabled then it is very possible that the parent clock will be probed after the lpcg first attempt.
> 
> Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>

This patch doesn't apply cleanly.

Please respin.

Thanks!

> ---
> v2: change from dev_warn to dev_err_probe
> ---
>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index d0ccaa040225..7bd9b745edbe 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -225,8 +225,8 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
>  
>  	ret = of_clk_parent_fill(np, parent_names, count);
>  	if (ret != count) {
> -		dev_err(&pdev->dev, "failed to get clock parent names\n");
> -		return count;
> +		return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
> +				"failed to get all clock parent names\n");
>  	}
>  
>  	ret = of_property_read_string_array(np, "clock-output-names",
> -- 
> 2.34.1

[PATCH v3] clk: imx8qxp: Defer instead of failing probe

[Diogo Manuel Pais Silva]

When of_clk_parent_fill is ran without all the parent clocks having been
probed then the probe function will return -EINVAL, making it so that
the probe isn't attempted again. As fw_devlink is on by default this
does not usually happen, but if fw_devlink is disabled then it is very
possible that the parent clock will be probed after the lpcg first
attempt.

Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
---
v2: change from dev_warn to dev_err_probe
v3: refresh patch
---
 drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
index d0ccaa040225..cae8f6060fe8 100644
--- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
+++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
@@ -225,8 +225,8 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
 
 	ret = of_clk_parent_fill(np, parent_names, count);
 	if (ret != count) {
-		dev_err(&pdev->dev, "failed to get clock parent names\n");
-		return count;
+		return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
+				     "failed to get all clock parent names\n");
 	}
 
 	ret = of_property_read_string_array(np, "clock-output-names",
-- 
2.34.1
________________________________________
From: Abel Vesa <abel.vesa@linaro.org>
Sent: Wednesday, August 28, 2024 10:31 AM
To: Diogo Manuel Pais Silva
Cc: Peng Fan; abelvesa@kernel.org; linux-clk@vger.kernel.org; shawnguo@kernel.org; kernel@pengutronix.de; s.hauer@pengutronix.de; linux-arm-kernel@lists.infradead.org; sboyd@kernel.org; mturquette@baylibre.com; festevam@gmail.com; imx@lists.linux.dev; EMC: linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] clk: imx8qxp: Defer instead of failing probe

On 24-07-02 08:10:44, Diogo Manuel Pais Silva wrote:
> When of_clk_parent_fill is ran without all the parent clocks having been probed then the probe function will return -EINVAL, making it so that the probe isn't attempted again. As fw_devlink is on by default this does not usually happen, but if fw_devlink is disabled then it is very possible that the parent clock will be probed after the lpcg first attempt.
>
> Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>

This patch doesn't apply cleanly.

Please respin.

Thanks!

> ---
> v2: change from dev_warn to dev_err_probe
> ---
>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index d0ccaa040225..7bd9b745edbe 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -225,8 +225,8 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
>
>       ret = of_clk_parent_fill(np, parent_names, count);
>       if (ret != count) {
> -             dev_err(&pdev->dev, "failed to get clock parent names\n");
> -             return count;
> +             return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
> +                             "failed to get all clock parent names\n");
>       }
>
>       ret = of_property_read_string_array(np, "clock-output-names",
> --
> 2.34.1

RE: [PATCH v3] clk: imx8qxp: Defer instead of failing probe

[Peng Fan]

Hi Diogo,

> Subject: [PATCH v3] clk: imx8qxp: Defer instead of failing probe
> 
> When of_clk_parent_fill is ran without all the parent clocks having
> been probed then the probe function will return -EINVAL, making it so
> that the probe isn't attempted again. As fw_devlink is on by default
> this does not usually happen, but if fw_devlink is disabled then it is
> very possible that the parent clock will be probed after the lpcg first
> attempt.

Did you meet issue with fw_devlink disabled?

> 
> Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
> ---
> v2: change from dev_warn to dev_err_probe
> v3: refresh patch
> ---
>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-
> imx8qxp-lpcg.c
> index d0ccaa040225..cae8f6060fe8 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -225,8 +225,8 @@ static int imx_lpcg_parse_clks_from_dt(struct
> platform_device *pdev,
> 
>         ret = of_clk_parent_fill(np, parent_names, count);
>         if (ret != count) {
> -               dev_err(&pdev->dev, "failed to get clock parent names\n");
> -               return count;
> +               return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
> +                                    "failed to get all clock parent
> + names\n");
>         }

The change is not enough, you also need to handle
        ret = imx_lpcg_parse_clks_from_dt(pdev, np);                                                
        if (!ret)                                                                                   
                return 0;
->
        ret = imx_lpcg_parse_clks_from_dt(pdev, np);                                                
        if (!ret)                                                                                   
                return 0;
        if (ret == -EPROBE_DEFER)
                return ret;

Regards,
Peng.

> 
>         ret = of_property_read_string_array(np, "clock-output-names",
> --
> 2.34.1
> ________________________________________
> From: Abel Vesa <abel.vesa@linaro.org>
> Sent: Wednesday, August 28, 2024 10:31 AM
> To: Diogo Manuel Pais Silva
> Cc: Peng Fan; abelvesa@kernel.org; linux-clk@vger.kernel.org;
> shawnguo@kernel.org; kernel@pengutronix.de;
> s.hauer@pengutronix.de; linux-arm-kernel@lists.infradead.org;
> sboyd@kernel.org; mturquette@baylibre.com; festevam@gmail.com;
> imx@lists.linux.dev; EMC: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] clk: imx8qxp: Defer instead of failing probe
> 
> On 24-07-02 08:10:44, Diogo Manuel Pais Silva wrote:
> > When of_clk_parent_fill is ran without all the parent clocks having
> been probed then the probe function will return -EINVAL, making it so
> that the probe isn't attempted again. As fw_devlink is on by default
> this does not usually happen, but if fw_devlink is disabled then it is
> very possible that the parent clock will be probed after the lpcg first
> attempt.
> >
> > Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
> 
> This patch doesn't apply cleanly.
> 
> Please respin.
> 
> Thanks!
> 
> > ---
> > v2: change from dev_warn to dev_err_probe
> > ---
> >  drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > index d0ccaa040225..7bd9b745edbe 100644
> > --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > @@ -225,8 +225,8 @@ static int
> imx_lpcg_parse_clks_from_dt(struct
> > platform_device *pdev,
> >
> >       ret = of_clk_parent_fill(np, parent_names, count);
> >       if (ret != count) {
> > -             dev_err(&pdev->dev, "failed to get clock parent names\n");
> > -             return count;
> > +             return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
> > +                             "failed to get all clock parent
> > + names\n");
> >       }
> >
> >       ret = of_property_read_string_array(np, "clock-output-names",
> > --
> > 2.34.1
> CONFIDENTIALITY: The contents of this e-mail are confidential and
> intended only for the above addressee(s). If you are not the intended
> recipient, or the person responsible for delivering it to the intended
> recipient, copying or delivering it to anyone else or using it in any
> unauthorized manner is prohibited and may be unlawful. If you receive
> this e-mail by mistake, please notify the sender and the systems
> administrator at straymail@tttech.com immediately.

Re: [PATCH v3] clk: imx8qxp: Defer instead of failing probe

[Diogo Manuel Pais Silva]

Hi Peng,

Thanks for pointing that out, I missed it. I will submit a v4.
Yes, the issue only happens when fw_devlink is disabled. I believe this is because devlink generates the device dependency graphs and only probes this device when all the dependencies are met. When the devlink is disabled this dependency is not respect as far as probing order goes, so it can happen that the device is probed when the clock parent is yet to be probed.

Best regards,
Diogo Silva
________________________________________
From: Peng Fan <peng.fan@nxp.com>
Sent: Wednesday, September 4, 2024 4:03 AM
To: Diogo Manuel Pais Silva; Abel Vesa
Cc: abelvesa@kernel.org; linux-clk@vger.kernel.org; shawnguo@kernel.org; kernel@pengutronix.de; s.hauer@pengutronix.de; linux-arm-kernel@lists.infradead.org; sboyd@kernel.org; mturquette@baylibre.com; festevam@gmail.com; imx@lists.linux.dev; EMC: linux-kernel@vger.kernel.org
Subject: RE: [PATCH v3] clk: imx8qxp: Defer instead of failing probe

Hi Diogo,

> Subject: [PATCH v3] clk: imx8qxp: Defer instead of failing probe
>
> When of_clk_parent_fill is ran without all the parent clocks having
> been probed then the probe function will return -EINVAL, making it so
> that the probe isn't attempted again. As fw_devlink is on by default
> this does not usually happen, but if fw_devlink is disabled then it is
> very possible that the parent clock will be probed after the lpcg first
> attempt.

Did you meet issue with fw_devlink disabled?

>
> Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
> ---
> v2: change from dev_warn to dev_err_probe
> v3: refresh patch
> ---
>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-
> imx8qxp-lpcg.c
> index d0ccaa040225..cae8f6060fe8 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -225,8 +225,8 @@ static int imx_lpcg_parse_clks_from_dt(struct
> platform_device *pdev,
>
>         ret = of_clk_parent_fill(np, parent_names, count);
>         if (ret != count) {
> -               dev_err(&pdev->dev, "failed to get clock parent names\n");
> -               return count;
> +               return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
> +                                    "failed to get all clock parent
> + names\n");
>         }

The change is not enough, you also need to handle
        ret = imx_lpcg_parse_clks_from_dt(pdev, np);
        if (!ret)
                return 0;
->
        ret = imx_lpcg_parse_clks_from_dt(pdev, np);
        if (!ret)
                return 0;
        if (ret == -EPROBE_DEFER)
                return ret;

Regards,
Peng.

>
>         ret = of_property_read_string_array(np, "clock-output-names",
> --
> 2.34.1
> ________________________________________
> From: Abel Vesa <abel.vesa@linaro.org>
> Sent: Wednesday, August 28, 2024 10:31 AM
> To: Diogo Manuel Pais Silva
> Cc: Peng Fan; abelvesa@kernel.org; linux-clk@vger.kernel.org;
> shawnguo@kernel.org; kernel@pengutronix.de;
> s.hauer@pengutronix.de; linux-arm-kernel@lists.infradead.org;
> sboyd@kernel.org; mturquette@baylibre.com; festevam@gmail.com;
> imx@lists.linux.dev; EMC: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] clk: imx8qxp: Defer instead of failing probe
>
> On 24-07-02 08:10:44, Diogo Manuel Pais Silva wrote:
> > When of_clk_parent_fill is ran without all the parent clocks having
> been probed then the probe function will return -EINVAL, making it so
> that the probe isn't attempted again. As fw_devlink is on by default
> this does not usually happen, but if fw_devlink is disabled then it is
> very possible that the parent clock will be probed after the lpcg first
> attempt.
> >
> > Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
>
> This patch doesn't apply cleanly.
>
> Please respin.
>
> Thanks!
>
> > ---
> > v2: change from dev_warn to dev_err_probe
> > ---
> >  drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > index d0ccaa040225..7bd9b745edbe 100644
> > --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > @@ -225,8 +225,8 @@ static int
> imx_lpcg_parse_clks_from_dt(struct
> > platform_device *pdev,
> >
> >       ret = of_clk_parent_fill(np, parent_names, count);
> >       if (ret != count) {
> > -             dev_err(&pdev->dev, "failed to get clock parent names\n");
> > -             return count;
> > +             return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
> > +                             "failed to get all clock parent
> > + names\n");
> >       }
> >
> >       ret = of_property_read_string_array(np, "clock-output-names",
> > --
> > 2.34.1
> CONFIDENTIALITY: The contents of this e-mail are confidential and
> intended only for the above addressee(s). If you are not the intended
> recipient, or the person responsible for delivering it to the intended
> recipient, copying or delivering it to anyone else or using it in any
> unauthorized manner is prohibited and may be unlawful. If you receive
> this e-mail by mistake, please notify the sender and the systems
> administrator at straymail@tttech.com immediately.

[PATCH v4] clk: imx8qxp: Defer instead of failing probe

[Diogo Manuel Pais Silva]

When of_clk_parent_fill is ran without all the parent clocks having been
probed then the probe function will return -EINVAL, making it so that
the probe isn't attempted again. As fw_devlink is on by default this
does not usually happen, but if fw_devlink is disabled then it is very
possible that the parent clock will be probed after the lpcg first
attempt.

Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
---
  v2: change from dev_warn to dev_err_probe
  v3: refresh patch
  v4: correctly propagate probe defer error
---
 drivers/clk/imx/clk-imx8qxp-lpcg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
index d0ccaa040225..1c3e1a7df8ca 100644
--- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
+++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
@@ -225,8 +225,8 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
 
 	ret = of_clk_parent_fill(np, parent_names, count);
 	if (ret != count) {
-		dev_err(&pdev->dev, "failed to get clock parent names\n");
-		return count;
+		return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
+				     "failed to get all clock parent names\n");
 	}
 
 	ret = of_property_read_string_array(np, "clock-output-names",
@@ -301,6 +301,8 @@ static int imx8qxp_lpcg_clk_probe(struct platform_device *pdev)
 	ret = imx_lpcg_parse_clks_from_dt(pdev, np);
 	if (!ret)
 		return 0;
+	if (ret == -EPROBE_DEFER)
+		return ret;
 
 	ss_lpcg = of_device_get_match_data(dev);
 	if (!ss_lpcg)
-- 
2.34.1

________________________________________
From: Peng Fan <peng.fan@nxp.com>
Sent: Wednesday, September 4, 2024 4:03 AM
To: Diogo Manuel Pais Silva; Abel Vesa
Cc: abelvesa@kernel.org; linux-clk@vger.kernel.org; shawnguo@kernel.org; kernel@pengutronix.de; s.hauer@pengutronix.de; linux-arm-kernel@lists.infradead.org; sboyd@kernel.org; mturquette@baylibre.com; festevam@gmail.com; imx@lists.linux.dev; EMC: linux-kernel@vger.kernel.org
Subject: RE: [PATCH v3] clk: imx8qxp: Defer instead of failing probe

Hi Diogo,

> Subject: [PATCH v3] clk: imx8qxp: Defer instead of failing probe
>
> When of_clk_parent_fill is ran without all the parent clocks having
> been probed then the probe function will return -EINVAL, making it so
> that the probe isn't attempted again. As fw_devlink is on by default
> this does not usually happen, but if fw_devlink is disabled then it is
> very possible that the parent clock will be probed after the lpcg first
> attempt.

Did you meet issue with fw_devlink disabled?

>
> Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
> ---
> v2: change from dev_warn to dev_err_probe
> v3: refresh patch
> ---
>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-
> imx8qxp-lpcg.c
> index d0ccaa040225..cae8f6060fe8 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -225,8 +225,8 @@ static int imx_lpcg_parse_clks_from_dt(struct
> platform_device *pdev,
>
>         ret = of_clk_parent_fill(np, parent_names, count);
>         if (ret != count) {
> -               dev_err(&pdev->dev, "failed to get clock parent names\n");
> -               return count;
> +               return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
> +                                    "failed to get all clock parent
> + names\n");
>         }

The change is not enough, you also need to handle
        ret = imx_lpcg_parse_clks_from_dt(pdev, np);
        if (!ret)
                return 0;
->
        ret = imx_lpcg_parse_clks_from_dt(pdev, np);
        if (!ret)
                return 0;
        if (ret == -EPROBE_DEFER)
                return ret;

Regards,
Peng.

>
>         ret = of_property_read_string_array(np, "clock-output-names",
> --
> 2.34.1
> ________________________________________
> From: Abel Vesa <abel.vesa@linaro.org>
> Sent: Wednesday, August 28, 2024 10:31 AM
> To: Diogo Manuel Pais Silva
> Cc: Peng Fan; abelvesa@kernel.org; linux-clk@vger.kernel.org;
> shawnguo@kernel.org; kernel@pengutronix.de;
> s.hauer@pengutronix.de; linux-arm-kernel@lists.infradead.org;
> sboyd@kernel.org; mturquette@baylibre.com; festevam@gmail.com;
> imx@lists.linux.dev; EMC: linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] clk: imx8qxp: Defer instead of failing probe
>
> On 24-07-02 08:10:44, Diogo Manuel Pais Silva wrote:
> > When of_clk_parent_fill is ran without all the parent clocks having
> been probed then the probe function will return -EINVAL, making it so
> that the probe isn't attempted again. As fw_devlink is on by default
> this does not usually happen, but if fw_devlink is disabled then it is
> very possible that the parent clock will be probed after the lpcg first
> attempt.
> >
> > Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
>
> This patch doesn't apply cleanly.
>
> Please respin.
>
> Thanks!
>
> > ---
> > v2: change from dev_warn to dev_err_probe
> > ---
> >  drivers/clk/imx/clk-imx8qxp-lpcg.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > index d0ccaa040225..7bd9b745edbe 100644
> > --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> > @@ -225,8 +225,8 @@ static int
> imx_lpcg_parse_clks_from_dt(struct
> > platform_device *pdev,
> >
> >       ret = of_clk_parent_fill(np, parent_names, count);
> >       if (ret != count) {
> > -             dev_err(&pdev->dev, "failed to get clock parent names\n");
> > -             return count;
> > +             return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
> > +                             "failed to get all clock parent
> > + names\n");
> >       }
> >
> >       ret = of_property_read_string_array(np, "clock-output-names",
> > --
> > 2.34.1

Re: [PATCH v4] clk: imx8qxp: Defer instead of failing probe

[Stephen Boyd]

Quoting Diogo Manuel Pais Silva (2024-09-03 23:50:02)
> When of_clk_parent_fill is ran without all the parent clocks having been
> probed then the probe function will return -EINVAL, making it so that
> the probe isn't attempted again. As fw_devlink is on by default this
> does not usually happen, but if fw_devlink is disabled then it is very
> possible that the parent clock will be probed after the lpcg first
> attempt.
> 
> Signed-off-by: Diogo Silva <diogo.pais@ttcontrol.com>
> ---

Please stop sending as replies to previous patches.

>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index d0ccaa040225..1c3e1a7df8ca 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -225,8 +225,8 @@ static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
> 
>         ret = of_clk_parent_fill(np, parent_names, count);

It would be better to move to struct clk_parent_data here and use the
.index member if possible. That would allow the clks to be registered
without their parent(s) being registered. It lets the core take care of
finding the parents.