[PATCH 1/1] clk: imx: scu: remove the calling of device_is_bound

[PATCH 1/1] clk: imx: scu: remove the calling of device_is_bound

[Dong Aisheng]

The device_is_bound() is unvisable to drivers when built as modules.
It's also not aimed to be used by drivers according to Greg K.H.
Let's remove it from clk-scu driver and find another way to do proper
driver loading sequence.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Stephen Boyd <sboyd@kernel.org>
Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding support")
Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/clk/imx/clk-scu.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
index d10f60e48ece..1f5518b7ab39 100644
--- a/drivers/clk/imx/clk-scu.c
+++ b/drivers/clk/imx/clk-scu.c
@@ -153,7 +153,6 @@ static inline struct clk_scu *to_clk_scu(struct clk_hw *hw)
 
 int imx_clk_scu_init(struct device_node *np)
 {
-	struct platform_device *pd_dev;
 	u32 clk_cells;
 	int ret, i;
 
@@ -166,17 +165,11 @@ int imx_clk_scu_init(struct device_node *np)
 	if (clk_cells == 2) {
 		for (i = 0; i < IMX_SC_R_LAST; i++)
 			INIT_LIST_HEAD(&imx_scu_clks[i]);
-		/*
-		 * Note: SCU clock driver depends on SCU power domain to be ready
-		 * first. As there're no power domains under scu clock node in dts,
-		 * we can't use PROBE_DEFER automatically.
-		 */
+
+		/* pd_np will be used to attach power domains later */
 		pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd");
-		pd_dev = of_find_device_by_node(pd_np);
-		if (!pd_dev || !device_is_bound(&pd_dev->dev)) {
-			of_node_put(pd_np);
-			return -EPROBE_DEFER;
-		}
+		if (!pd_np)
+			return -EINVAL;
 	}
 
 	return platform_driver_register(&imx_clk_scu_driver);
-- 
2.23.0

Re: [PATCH 1/1] clk: imx: scu: remove the calling of device_is_bound

[Sudip Mukherjee]

Hi Dong,

On Thu, Nov 19, 2020 at 07:43:02PM +0800, Dong Aisheng wrote:
> The device_is_bound() is unvisable to drivers when built as modules.
> It's also not aimed to be used by drivers according to Greg K.H.
> Let's remove it from clk-scu driver and find another way to do proper
> driver loading sequence.

Greg was asking to use device_link for this issue. Have you tried something
like the following: (untested as I dont have the hardware).

diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c
index 5b3d4ede7c7c..9ae6c768ea05 100644
--- a/drivers/clk/imx/clk-imx8qxp.c
+++ b/drivers/clk/imx/clk-imx8qxp.c
@@ -25,7 +25,7 @@ static int imx8qxp_clk_probe(struct platform_device *pdev)
 	u32 clk_cells;
 	int ret, i;
 
-	ret = imx_clk_scu_init(ccm_node);
+	ret = imx_clk_scu_init(ccm_node, pdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
index d10f60e48ece..e834bfadc2a6 100644
--- a/drivers/clk/imx/clk-scu.c
+++ b/drivers/clk/imx/clk-scu.c
@@ -151,8 +151,9 @@ static inline struct clk_scu *to_clk_scu(struct clk_hw *hw)
 	return container_of(hw, struct clk_scu, hw);
 }
 
-int imx_clk_scu_init(struct device_node *np)
+int imx_clk_scu_init(struct device_node *np, struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct platform_device *pd_dev;
 	u32 clk_cells;
 	int ret, i;
@@ -173,12 +174,12 @@ int imx_clk_scu_init(struct device_node *np)
 		 */
 		pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd");
 		pd_dev = of_find_device_by_node(pd_np);
-		if (!pd_dev || !device_is_bound(&pd_dev->dev)) {
+		if (!pd_dev || !device_link_add(dev, &pd_dev->dev,
+						DL_FLAG_AUTOREMOVE_CONSUMER)) {
 			of_node_put(pd_np);
 			return -EPROBE_DEFER;
 		}
 	}
-
 	return platform_driver_register(&imx_clk_scu_driver);
 }
 
diff --git a/drivers/clk/imx/clk-scu.h b/drivers/clk/imx/clk-scu.h
index e8352164923e..14e2baf14757 100644
--- a/drivers/clk/imx/clk-scu.h
+++ b/drivers/clk/imx/clk-scu.h
@@ -13,7 +13,7 @@
 extern struct list_head imx_scu_clks[];
 extern const struct dev_pm_ops imx_clk_lpcg_scu_pm_ops;
 
-int imx_clk_scu_init(struct device_node *np);
+int imx_clk_scu_init(struct device_node *np, struct platform_device *pdev);
 struct clk_hw *imx_scu_of_clk_src_get(struct of_phandle_args *clkspec,
 				      void *data);
 struct clk_hw *imx_clk_scu_alloc_dev(const char *name,

--
Regards
Sudip

RE: [PATCH 1/1] clk: imx: scu: remove the calling of device_is_bound

[Aisheng Dong]

> From: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> Sent: Thursday, November 19, 2020 9:08 PM
> 
> Hi Dong,
> 
> On Thu, Nov 19, 2020 at 07:43:02PM +0800, Dong Aisheng wrote:
> > The device_is_bound() is unvisable to drivers when built as modules.
> > It's also not aimed to be used by drivers according to Greg K.H.
> > Let's remove it from clk-scu driver and find another way to do proper
> > driver loading sequence.
> 
> Greg was asking to use device_link for this issue. Have you tried something like
> the following: (untested as I dont have the hardware).

It can't work as expected because it requires supplier devices (scu pd) to be probed first.
and if scu pd was probed first, then there're already no issues.

Regards
Aisheng

> 
> diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c index
> 5b3d4ede7c7c..9ae6c768ea05 100644
> --- a/drivers/clk/imx/clk-imx8qxp.c
> +++ b/drivers/clk/imx/clk-imx8qxp.c
> @@ -25,7 +25,7 @@ static int imx8qxp_clk_probe(struct platform_device
> *pdev)
>  	u32 clk_cells;
>  	int ret, i;
> 
> -	ret = imx_clk_scu_init(ccm_node);
> +	ret = imx_clk_scu_init(ccm_node, pdev);
>  	if (ret)
>  		return ret;
> 
> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c index
> d10f60e48ece..e834bfadc2a6 100644
> --- a/drivers/clk/imx/clk-scu.c
> +++ b/drivers/clk/imx/clk-scu.c
> @@ -151,8 +151,9 @@ static inline struct clk_scu *to_clk_scu(struct clk_hw
> *hw)
>  	return container_of(hw, struct clk_scu, hw);  }
> 
> -int imx_clk_scu_init(struct device_node *np)
> +int imx_clk_scu_init(struct device_node *np, struct platform_device
> +*pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct platform_device *pd_dev;
>  	u32 clk_cells;
>  	int ret, i;
> @@ -173,12 +174,12 @@ int imx_clk_scu_init(struct device_node *np)
>  		 */
>  		pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd");
>  		pd_dev = of_find_device_by_node(pd_np);
> -		if (!pd_dev || !device_is_bound(&pd_dev->dev)) {
> +		if (!pd_dev || !device_link_add(dev, &pd_dev->dev,
> +						DL_FLAG_AUTOREMOVE_CONSUMER)) {
>  			of_node_put(pd_np);
>  			return -EPROBE_DEFER;
>  		}
>  	}
> -
>  	return platform_driver_register(&imx_clk_scu_driver);
>  }
> 
> diff --git a/drivers/clk/imx/clk-scu.h b/drivers/clk/imx/clk-scu.h index
> e8352164923e..14e2baf14757 100644
> --- a/drivers/clk/imx/clk-scu.h
> +++ b/drivers/clk/imx/clk-scu.h
> @@ -13,7 +13,7 @@
>  extern struct list_head imx_scu_clks[];  extern const struct dev_pm_ops
> imx_clk_lpcg_scu_pm_ops;
> 
> -int imx_clk_scu_init(struct device_node *np);
> +int imx_clk_scu_init(struct device_node *np, struct platform_device
> +*pdev);
>  struct clk_hw *imx_scu_of_clk_src_get(struct of_phandle_args *clkspec,
>  				      void *data);
>  struct clk_hw *imx_clk_scu_alloc_dev(const char *name,
> 
> --
> Regards
> Sudip

Re: [PATCH 1/1] clk: imx: scu: remove the calling of device_is_bound

[Sudip Mukherjee]

On Thu, Nov 19, 2020 at 3:30 PM Aisheng Dong <aisheng.dong@nxp.com> wrote:
>
> > From: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> > Sent: Thursday, November 19, 2020 9:08 PM
> >
> > Hi Dong,
> >
> > On Thu, Nov 19, 2020 at 07:43:02PM +0800, Dong Aisheng wrote:
> > > The device_is_bound() is unvisable to drivers when built as modules.
> > > It's also not aimed to be used by drivers according to Greg K.H.
> > > Let's remove it from clk-scu driver and find another way to do proper
> > > driver loading sequence.
> >
> > Greg was asking to use device_link for this issue. Have you tried something like
> > the following: (untested as I dont have the hardware).
>
> It can't work as expected because it requires supplier devices (scu pd) to be probed first.
> and if scu pd was probed first, then there're already no issues.

hmm.. thats odd. I was expecting that if "scu-pd" has not registered
then device_link_add() will return NULL and then imx_clk_scu_init()
will return -EPROBE_DEFER.


-- 
Regards
Sudip

RE: [PATCH 1/1] clk: imx: scu: remove the calling of device_is_bound

[Aisheng Dong]

> From: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> Sent: Friday, November 20, 2020 1:45 AM
> On Thu, Nov 19, 2020 at 3:30 PM Aisheng Dong <aisheng.dong@nxp.com>
> wrote:
> >
> > > From: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> > > Sent: Thursday, November 19, 2020 9:08 PM
> > >
> > > Hi Dong,
> > >
> > > On Thu, Nov 19, 2020 at 07:43:02PM +0800, Dong Aisheng wrote:
> > > > The device_is_bound() is unvisable to drivers when built as modules.
> > > > It's also not aimed to be used by drivers according to Greg K.H.
> > > > Let's remove it from clk-scu driver and find another way to do
> > > > proper driver loading sequence.
> > >
> > > Greg was asking to use device_link for this issue. Have you tried
> > > something like the following: (untested as I dont have the hardware).
> >
> > It can't work as expected because it requires supplier devices (scu pd) to be
> probed first.
> > and if scu pd was probed first, then there're already no issues.
> 
> hmm.. thats odd. I was expecting that if "scu-pd" has not registered then
> device_link_add() will return NULL and then imx_clk_scu_init() will return
> -EPROBE_DEFER.

The problem is what if scu-pd has registered but not probed. The device _link_add
won't block the scu clk to continue to run while scu_pd driver is still not ready.
This is not as expected.

Regards
Aisheng


> 
> 
> --
> Regards
> Sudip

RE: [PATCH 1/1] clk: imx: scu: remove the calling of device_is_bound

[Aisheng Dong]

Hi Shawn,

> From: Aisheng Dong <aisheng.dong@nxp.com>
> Sent: Thursday, November 19, 2020 7:43 PM
> 
> The device_is_bound() is unvisable to drivers when built as modules.
> It's also not aimed to be used by drivers according to Greg K.H.
> Let's remove it from clk-scu driver and find another way to do proper driver
> loading sequence.
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding support")
> Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>

Could you apply this fix first as clk-scu has COMPILE_TEST enabled and may affect other
platforms build test?

config MXC_CLK_SCU
        tristate "IMX SCU clock"
        depends on ARCH_MXC || COMPILE_TEST
        depends on IMX_SCU && HAVE_ARM_SMCCC

BTW, I've sent another patch series [1] to support defer probe without calling device_is_bound
per the last discussion with Greg K.H. [2].

1. https://patchwork.kernel.org/project/linux-clk/cover/20201124100802.22775-1-aisheng.dong@nxp.com/
2. https://lore.kernel.org/patchwork/patch/1334670/

Regards
Aisheng

> ---
>  drivers/clk/imx/clk-scu.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c index
> d10f60e48ece..1f5518b7ab39 100644
> --- a/drivers/clk/imx/clk-scu.c
> +++ b/drivers/clk/imx/clk-scu.c
> @@ -153,7 +153,6 @@ static inline struct clk_scu *to_clk_scu(struct clk_hw
> *hw)
> 
>  int imx_clk_scu_init(struct device_node *np)  {
> -	struct platform_device *pd_dev;
>  	u32 clk_cells;
>  	int ret, i;
> 
> @@ -166,17 +165,11 @@ int imx_clk_scu_init(struct device_node *np)
>  	if (clk_cells == 2) {
>  		for (i = 0; i < IMX_SC_R_LAST; i++)
>  			INIT_LIST_HEAD(&imx_scu_clks[i]);
> -		/*
> -		 * Note: SCU clock driver depends on SCU power domain to be ready
> -		 * first. As there're no power domains under scu clock node in dts,
> -		 * we can't use PROBE_DEFER automatically.
> -		 */
> +
> +		/* pd_np will be used to attach power domains later */
>  		pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd");
> -		pd_dev = of_find_device_by_node(pd_np);
> -		if (!pd_dev || !device_is_bound(&pd_dev->dev)) {
> -			of_node_put(pd_np);
> -			return -EPROBE_DEFER;
> -		}
> +		if (!pd_np)
> +			return -EINVAL;
>  	}
> 
>  	return platform_driver_register(&imx_clk_scu_driver);
> --
> 2.23.0

Re: [PATCH 1/1] clk: imx: scu: remove the calling of device_is_bound

[Shawn Guo]

On Thu, Nov 19, 2020 at 07:43:02PM +0800, Dong Aisheng wrote:
> The device_is_bound() is unvisable to drivers when built as modules.

s/unvisable/invisible?

I fixed it up and applied the patch.

Shawn

> It's also not aimed to be used by drivers according to Greg K.H.
> Let's remove it from clk-scu driver and find another way to do proper
> driver loading sequence.
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Fixes: 77d8f3068c63 ("clk: imx: scu: add two cells binding support")
> Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  drivers/clk/imx/clk-scu.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> index d10f60e48ece..1f5518b7ab39 100644
> --- a/drivers/clk/imx/clk-scu.c
> +++ b/drivers/clk/imx/clk-scu.c
> @@ -153,7 +153,6 @@ static inline struct clk_scu *to_clk_scu(struct clk_hw *hw)
>  
>  int imx_clk_scu_init(struct device_node *np)
>  {
> -	struct platform_device *pd_dev;
>  	u32 clk_cells;
>  	int ret, i;
>  
> @@ -166,17 +165,11 @@ int imx_clk_scu_init(struct device_node *np)
>  	if (clk_cells == 2) {
>  		for (i = 0; i < IMX_SC_R_LAST; i++)
>  			INIT_LIST_HEAD(&imx_scu_clks[i]);
> -		/*
> -		 * Note: SCU clock driver depends on SCU power domain to be ready
> -		 * first. As there're no power domains under scu clock node in dts,
> -		 * we can't use PROBE_DEFER automatically.
> -		 */
> +
> +		/* pd_np will be used to attach power domains later */
>  		pd_np = of_find_compatible_node(NULL, NULL, "fsl,scu-pd");
> -		pd_dev = of_find_device_by_node(pd_np);
> -		if (!pd_dev || !device_is_bound(&pd_dev->dev)) {
> -			of_node_put(pd_np);
> -			return -EPROBE_DEFER;
> -		}
> +		if (!pd_np)
> +			return -EINVAL;
>  	}
>  
>  	return platform_driver_register(&imx_clk_scu_driver);
> -- 
> 2.23.0
>