[PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Rajan Vaja]

Fixed factor clock has two initialization at of_clk_init()
time and also during platform driver probe. So declare the
fixed factor clock with CLK_OF_DECLARE_DRIVER instead of
CLK_OF_DECLARE.

See below commit for reference:
"clk: sunxi: apb0: Use new macro CLK_OF_DECLARE_DRIVER"
(sha1: 915128b621a05c63fa58ca9e4cbdf394bbe592f3)

Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
Suggested-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/clk/clk-fixed-factor.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index a5d402d..d72ef2d 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -196,8 +196,9 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
 {
        _of_fixed_factor_clk_setup(node);
 }
-CLK_OF_DECLARE(fixed_factor_clk, "fixed-factor-clock",
-               of_fixed_factor_clk_setup);
+
+CLK_OF_DECLARE_DRIVER(fixed_factor_clk, "fixed-factor-clock",
+                     of_fixed_factor_clk_setup);

 static int of_fixed_factor_clk_remove(struct platform_device *pdev)
 {
--
2.7.4

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Stephen Boyd]

Quoting Rajan Vaja (2018-03-08 06:15:00)
> Fixed factor clock has two initialization at of_clk_init()
> time and also during platform driver probe. So declare the
> fixed factor clock with CLK_OF_DECLARE_DRIVER instead of
> CLK_OF_DECLARE.
> 
> See below commit for reference:
> "clk: sunxi: apb0: Use new macro CLK_OF_DECLARE_DRIVER"
> (sha1: 915128b621a05c63fa58ca9e4cbdf394bbe592f3)
> 
> Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
> Suggested-by: Michal Simek <michal.simek@xilinx.com>
> ---
>  drivers/clk/clk-fixed-factor.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
> index a5d402d..d72ef2d 100644
> --- a/drivers/clk/clk-fixed-factor.c
> +++ b/drivers/clk/clk-fixed-factor.c
> @@ -196,8 +196,9 @@ void __init of_fixed_factor_clk_setup(struct device_node *node)
>  {
>         _of_fixed_factor_clk_setup(node);
>  }
> -CLK_OF_DECLARE(fixed_factor_clk, "fixed-factor-clock",
> -               of_fixed_factor_clk_setup);
> +
> +CLK_OF_DECLARE_DRIVER(fixed_factor_clk, "fixed-factor-clock",
> +                     of_fixed_factor_clk_setup);
> 

Is the intent to register the clk twice? I believe things are working as
intended without this patch, so maybe you can explain a little more what
you're trying to fix.

RE: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Rajan Vaja]

Hi Stephen,

Thanks for the review.

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Sent: Friday, March 09, 2018 10:25 AM
> To: Rajan Vaja <RAJANV@xilinx.com>; mturquette@baylibre.com
> Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>; Rajan Vaja
> <RAJANV@xilinx.com>
> Subject: Re: [PATCH] clk: clk-fixed-factor: Use new macro
> CLK_OF_DECLARE_DRIVER
> 
> Quoting Rajan Vaja (2018-03-08 06:15:00)
> > Fixed factor clock has two initialization at of_clk_init() time and
> > also during platform driver probe. So declare the fixed factor clock
> > with CLK_OF_DECLARE_DRIVER instead of CLK_OF_DECLARE.
> >
> > See below commit for reference:
> > "clk: sunxi: apb0: Use new macro CLK_OF_DECLARE_DRIVER"
> > (sha1: 915128b621a05c63fa58ca9e4cbdf394bbe592f3)
> >
> > Signed-off-by: Rajan Vaja <rajanv@xilinx.com>
> > Suggested-by: Michal Simek <michal.simek@xilinx.com>
> > ---
> >  drivers/clk/clk-fixed-factor.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/clk/clk-fixed-factor.c
> > b/drivers/clk/clk-fixed-factor.c index a5d402d..d72ef2d 100644
> > --- a/drivers/clk/clk-fixed-factor.c
> > +++ b/drivers/clk/clk-fixed-factor.c
> > @@ -196,8 +196,9 @@ void __init of_fixed_factor_clk_setup(struct
> > device_node *node)  {
> >         _of_fixed_factor_clk_setup(node);  }
> > -CLK_OF_DECLARE(fixed_factor_clk, "fixed-factor-clock",
> > -               of_fixed_factor_clk_setup);
> > +
> > +CLK_OF_DECLARE_DRIVER(fixed_factor_clk, "fixed-factor-clock",
> > +                     of_fixed_factor_clk_setup);
> >
> 
> Is the intent to register the clk twice? I believe things are working as
> intended without this patch, so maybe you can explain a little more what
> you're trying to fix.
[Rajan] Yes. During of_clk_init() if some DT fixed factor clock has parent which is neither mentioned in output-clock-names of clock controller nor registered as clock provider, of_clk_init() will try to forcefully register in second loop. 

                        if (force || parent_ready(clk_provider->np)) {

                                /* Don't populate platform devices */
                                of_node_set_flag(clk_provider->np,
                                                 OF_POPULATED);

So registration of this DT fixed-factor clock would fail as parent would be NULL as below (called from _of_fixed_factor_clk_setup()):
parent_name = of_clk_get_parent_name(node, 0);

On the other hand, even if registration failed, that node will be marked as OF_POPULATED, so probe of clk-fixed-factor.c will also not be called and that DT fixed-factor clock would never be registered. 

Same thing is discussed at  https://lkml.org/lkml/2017/6/5/681 .

RE: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Stephen Boyd]

Quoting Rajan Vaja (2018-03-09 11:27:40)
> > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > 
> > Is the intent to register the clk twice? I believe things are working as
> > intended without this patch, so maybe you can explain a little more what
> > you're trying to fix.
> [Rajan] Yes. During of_clk_init() if some DT fixed factor clock has
> parent which is neither mentioned in output-clock-names of clock
> controller nor registered as clock provider, of_clk_init() will try to
> forcefully register in second loop. 
> 
>                         if (force || parent_ready(clk_provider->np)) {
> 
>                                 /* Don't populate platform devices */
>                                 of_node_set_flag(clk_provider->np,
>                                                  OF_POPULATED);
> 
> So registration of this DT fixed-factor clock would fail as parent
> would be NULL as below (called from _of_fixed_factor_clk_setup()):
> parent_name = of_clk_get_parent_name(node, 0);
> 
> On the other hand, even if registration failed, that node will be
> marked as OF_POPULATED, so probe of clk-fixed-factor.c will also not
> be called and that DT fixed-factor clock would never be registered. 
> 
> Same thing is discussed at  https://lkml.org/lkml/2017/6/5/681 .

Ok. I believe the answer is to fix the DT to describe the parent chain
properly with clock-output-names. Otherwise, we have no good way of
figuring out what the name should be.

The alternative is to start working on a solution for having the clk
framework stop using strings to describe clk topology. My plan there is
to allow clk registration to indicate that another parent names array
should be used with clk_get() of the device or node pointer that's
associated with the clk during registration to find the parent . If we
had that, then we could hook up clks into the tree by calling clk_get()
in the framework and map through the clock-names property. This also
gets us to a point where clk names don't have to be globally unique,
which would be nice.

RE: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Rajan Vaja]

Hi Stephen,

Thanks for the comment.

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Sent: 16 March 2018 12:17 AM
> To: Rajan Vaja <RAJANV@xilinx.com>
> Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> mturquette@baylibre.com
> Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> CLK_OF_DECLARE_DRIVER
> 
> Quoting Rajan Vaja (2018-03-09 11:27:40)
> > > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > >
> > > Is the intent to register the clk twice? I believe things are working as
> > > intended without this patch, so maybe you can explain a little more what
> > > you're trying to fix.
> > [Rajan] Yes. During of_clk_init() if some DT fixed factor clock has
> > parent which is neither mentioned in output-clock-names of clock
> > controller nor registered as clock provider, of_clk_init() will try to
> > forcefully register in second loop.
> >
> >                         if (force || parent_ready(clk_provider->np)) {
> >
> >                                 /* Don't populate platform devices */
> >                                 of_node_set_flag(clk_provider->np,
> >                                                  OF_POPULATED);
> >
> > So registration of this DT fixed-factor clock would fail as parent
> > would be NULL as below (called from _of_fixed_factor_clk_setup()):
> > parent_name = of_clk_get_parent_name(node, 0);
> >
> > On the other hand, even if registration failed, that node will be
> > marked as OF_POPULATED, so probe of clk-fixed-factor.c will also not
> > be called and that DT fixed-factor clock would never be registered.
> >
> > Same thing is discussed at  https://lkml.org/lkml/2017/6/5/681 .
> 
> Ok. I believe the answer is to fix the DT to describe the parent chain
> properly with clock-output-names. Otherwise, we have no good way of
> figuring out what the name should be.
[Rajan] clock DT binding doc says that clock-output-names property
is optional and sometimes not recommended. 
I think this patch fixes the issue we have which mandates to add clock-output-names
property (for this particular case). Also, IIUC platform driver probe in clk-fixed-factor.c
will never be called unless we use CLK_OF_DECLARE_DRIVER.
I completely agree that proper solution would be to stop using strings to
describe clock topology.

> 
> The alternative is to start working on a solution for having the clk
> framework stop using strings to describe clk topology. My plan there is
> to allow clk registration to indicate that another parent names array
> should be used with clk_get() of the device or node pointer that's
> associated with the clk during registration to find the parent . If we
> had that, then we could hook up clks into the tree by calling clk_get()
> in the framework and map through the clock-names property. This also
> gets us to a point where clk names don't have to be globally unique,
> which would be nice.

RE: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Rajan Vaja]

Hi Stephen,

> -----Original Message-----
> From: Rajan Vaja
> Sent: 16 March 2018 05:20 PM
> To: 'Stephen Boyd' <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> mturquette@baylibre.com
> Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> CLK_OF_DECLARE_DRIVER
> 
> Hi Stephen,
> 
> Thanks for the comment.
> 
> > -----Original Message-----
> > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > Sent: 16 March 2018 12:17 AM
> > To: Rajan Vaja <RAJANV@xilinx.com>
> > Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> > <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> > mturquette@baylibre.com
> > Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> > CLK_OF_DECLARE_DRIVER
> >
> > Quoting Rajan Vaja (2018-03-09 11:27:40)
> > > > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > > >
> > > > Is the intent to register the clk twice? I believe things are working as
> > > > intended without this patch, so maybe you can explain a little more what
> > > > you're trying to fix.
> > > [Rajan] Yes. During of_clk_init() if some DT fixed factor clock has
> > > parent which is neither mentioned in output-clock-names of clock
> > > controller nor registered as clock provider, of_clk_init() will try to
> > > forcefully register in second loop.
> > >
> > >                         if (force || parent_ready(clk_provider->np)) {
> > >
> > >                                 /* Don't populate platform devices */
> > >                                 of_node_set_flag(clk_provider->np,
> > >                                                  OF_POPULATED);
> > >
> > > So registration of this DT fixed-factor clock would fail as parent
> > > would be NULL as below (called from _of_fixed_factor_clk_setup()):
> > > parent_name = of_clk_get_parent_name(node, 0);
> > >
> > > On the other hand, even if registration failed, that node will be
> > > marked as OF_POPULATED, so probe of clk-fixed-factor.c will also not
> > > be called and that DT fixed-factor clock would never be registered.
> > >
> > > Same thing is discussed at  https://lkml.org/lkml/2017/6/5/681 .
> >
> > Ok. I believe the answer is to fix the DT to describe the parent chain
> > properly with clock-output-names. Otherwise, we have no good way of
> > figuring out what the name should be.
> [Rajan] clock DT binding doc says that clock-output-names property
> is optional and sometimes not recommended.
> I think this patch fixes the issue we have which mandates to add clock-output-
> names
> property (for this particular case). Also, IIUC platform driver probe in clk-fixed-
> factor.c
> will never be called unless we use CLK_OF_DECLARE_DRIVER.
> I completely agree that proper solution would be to stop using strings to
> describe clock topology.
[Rajan] Any comments on this?
Unless we have proper solution ready, we need to have some mechanism to handle this scenario.
clock-output-names is optional and without this, it mandates to use clock-output-names.

> 
> >
> > The alternative is to start working on a solution for having the clk
> > framework stop using strings to describe clk topology. My plan there is
> > to allow clk registration to indicate that another parent names array
> > should be used with clk_get() of the device or node pointer that's
> > associated with the clk during registration to find the parent . If we
> > had that, then we could hook up clks into the tree by calling clk_get()
> > in the framework and map through the clock-names property. This also
> > gets us to a point where clk names don't have to be globally unique,
> > which would be nice.

RE: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Stephen Boyd]

Quoting Rajan Vaja (2018-05-03 02:18:30)
> Hi Stephen,
> 
> > -----Original Message-----
> > From: Rajan Vaja
> > Sent: 16 March 2018 05:20 PM
> > To: 'Stephen Boyd' <sboyd@kernel.org>
> > Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> > <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> > mturquette@baylibre.com
> > Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> > CLK_OF_DECLARE_DRIVER
> > 
> > Hi Stephen,
> > 
> > Thanks for the comment.
> > 
> > > -----Original Message-----
> > > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > > Sent: 16 March 2018 12:17 AM
> > > To: Rajan Vaja <RAJANV@xilinx.com>
> > > Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> > > <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> > > mturquette@baylibre.com
> > > Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> > > CLK_OF_DECLARE_DRIVER
> > >
> > > Quoting Rajan Vaja (2018-03-09 11:27:40)
> > > > > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > > > >
> > > > > Is the intent to register the clk twice? I believe things are working as
> > > > > intended without this patch, so maybe you can explain a little more what
> > > > > you're trying to fix.
> > > > [Rajan] Yes. During of_clk_init() if some DT fixed factor clock has
> > > > parent which is neither mentioned in output-clock-names of clock
> > > > controller nor registered as clock provider, of_clk_init() will try to
> > > > forcefully register in second loop.
> > > >
> > > >                         if (force || parent_ready(clk_provider->np)) {
> > > >
> > > >                                 /* Don't populate platform devices */
> > > >                                 of_node_set_flag(clk_provider->np,
> > > >                                                  OF_POPULATED);
> > > >
> > > > So registration of this DT fixed-factor clock would fail as parent
> > > > would be NULL as below (called from _of_fixed_factor_clk_setup()):
> > > > parent_name = of_clk_get_parent_name(node, 0);
> > > >
> > > > On the other hand, even if registration failed, that node will be
> > > > marked as OF_POPULATED, so probe of clk-fixed-factor.c will also not
> > > > be called and that DT fixed-factor clock would never be registered.
> > > >
> > > > Same thing is discussed at  https://lkml.org/lkml/2017/6/5/681 .
> > >
> > > Ok. I believe the answer is to fix the DT to describe the parent chain
> > > properly with clock-output-names. Otherwise, we have no good way of
> > > figuring out what the name should be.
> > [Rajan] clock DT binding doc says that clock-output-names property
> > is optional and sometimes not recommended.
> > I think this patch fixes the issue we have which mandates to add clock-output-
> > names
> > property (for this particular case). Also, IIUC platform driver probe in clk-fixed-
> > factor.c
> > will never be called unless we use CLK_OF_DECLARE_DRIVER.
> > I completely agree that proper solution would be to stop using strings to
> > describe clock topology.
> [Rajan] Any comments on this?
> Unless we have proper solution ready, we need to have some mechanism to handle this scenario.
> clock-output-names is optional and without this, it mandates to use clock-output-names.
> 

I couldn't read your outlook sent email and I was too busy to go
translate it. Some bug in my MUA it seems.

Can you add the output-names property? In this case it's practically
mandatory, so if you can't do it I'd like to hear why not.

RE: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Rajan Vaja]

Hi Stephen,

Thanks for the reply. 

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Sent: 02 June 2018 12:11 PM
> To: Rajan Vaja <RAJANV@xilinx.com>
> Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> mturquette@baylibre.com
> Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> CLK_OF_DECLARE_DRIVER
> 
> Quoting Rajan Vaja (2018-05-03 02:18:30)
> > Hi Stephen,
> >
> > > -----Original Message-----
> > > From: Rajan Vaja
> > > Sent: 16 March 2018 05:20 PM
> > > To: 'Stephen Boyd' <sboyd@kernel.org>
> > > Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> > > <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> > > mturquette@baylibre.com
> > > Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> > > CLK_OF_DECLARE_DRIVER
> > >
> > > Hi Stephen,
> > >
> > > Thanks for the comment.
> > >
> > > > -----Original Message-----
> > > > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > > > Sent: 16 March 2018 12:17 AM
> > > > To: Rajan Vaja <RAJANV@xilinx.com>
> > > > Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> > > > <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> > > > mturquette@baylibre.com
> > > > Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> > > > CLK_OF_DECLARE_DRIVER
> > > >
> > > > Quoting Rajan Vaja (2018-03-09 11:27:40)
> > > > > > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > > > > >
> > > > > > Is the intent to register the clk twice? I believe things are working as
> > > > > > intended without this patch, so maybe you can explain a little more
> what
> > > > > > you're trying to fix.
> > > > > [Rajan] Yes. During of_clk_init() if some DT fixed factor clock has
> > > > > parent which is neither mentioned in output-clock-names of clock
> > > > > controller nor registered as clock provider, of_clk_init() will try to
> > > > > forcefully register in second loop.
> > > > >
> > > > >                         if (force || parent_ready(clk_provider->np)) {
> > > > >
> > > > >                                 /* Don't populate platform devices */
> > > > >                                 of_node_set_flag(clk_provider->np,
> > > > >                                                  OF_POPULATED);
> > > > >
> > > > > So registration of this DT fixed-factor clock would fail as parent
> > > > > would be NULL as below (called from _of_fixed_factor_clk_setup()):
> > > > > parent_name = of_clk_get_parent_name(node, 0);
> > > > >
> > > > > On the other hand, even if registration failed, that node will be
> > > > > marked as OF_POPULATED, so probe of clk-fixed-factor.c will also not
> > > > > be called and that DT fixed-factor clock would never be registered.
> > > > >
> > > > > Same thing is discussed at  https://lkml.org/lkml/2017/6/5/681 .
> > > >
> > > > Ok. I believe the answer is to fix the DT to describe the parent chain
> > > > properly with clock-output-names. Otherwise, we have no good way of
> > > > figuring out what the name should be.
> > > [Rajan] clock DT binding doc says that clock-output-names property
> > > is optional and sometimes not recommended.
> > > I think this patch fixes the issue we have which mandates to add clock-
> output-
> > > names
> > > property (for this particular case). Also, IIUC platform driver probe in clk-
> fixed-
> > > factor.c
> > > will never be called unless we use CLK_OF_DECLARE_DRIVER.
> > > I completely agree that proper solution would be to stop using strings to
> > > describe clock topology.
> > [Rajan] Any comments on this?
> > Unless we have proper solution ready, we need to have some mechanism to
> handle this scenario.
> > clock-output-names is optional and without this, it mandates to use clock-
> output-names.
> >
> 
> I couldn't read your outlook sent email and I was too busy to go
> translate it. Some bug in my MUA it seems.
> 
> Can you add the output-names property? In this case it's practically
> mandatory, so if you can't do it I'd like to hear why not.
[Rajan] In our case, we are firmware maintains clock database and driver query clocks
from firmware and registers clock based on information received from firmware. So
output clock names are not fixed. 
https://patchwork.kernel.org/patch/10439893/ - dt-bindings: clock: Add bindings for ZynqMP clock driver
https://patchwork.kernel.org/patch/10439891/ - drivers: clk: Add ZynqMP clock driver

RE: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Rajan Vaja]

Hi Stephen,

> -----Original Message-----
> From: Rajan Vaja
> Sent: 04 June 2018 09:11 AM
> To: 'Stephen Boyd' <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> mturquette@baylibre.com
> Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> CLK_OF_DECLARE_DRIVER
> 
> Hi Stephen,
> 
> Thanks for the reply.
> 
> > -----Original Message-----
> > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > Sent: 02 June 2018 12:11 PM
> > To: Rajan Vaja <RAJANV@xilinx.com>
> > Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> > <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> > mturquette@baylibre.com
> > Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> > CLK_OF_DECLARE_DRIVER
> >
> > Quoting Rajan Vaja (2018-05-03 02:18:30)
> > > Hi Stephen,
> > >
> > > > -----Original Message-----
> > > > From: Rajan Vaja
> > > > Sent: 16 March 2018 05:20 PM
> > > > To: 'Stephen Boyd' <sboyd@kernel.org>
> > > > Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> > > > <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> > > > mturquette@baylibre.com
> > > > Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> > > > CLK_OF_DECLARE_DRIVER
> > > >
> > > > Hi Stephen,
> > > >
> > > > Thanks for the comment.
> > > >
> > > > > -----Original Message-----
> > > > > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > > > > Sent: 16 March 2018 12:17 AM
> > > > > To: Rajan Vaja <RAJANV@xilinx.com>
> > > > > Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> > > > > <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> > > > > mturquette@baylibre.com
> > > > > Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> > > > > CLK_OF_DECLARE_DRIVER
> > > > >
> > > > > Quoting Rajan Vaja (2018-03-09 11:27:40)
> > > > > > > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > > > > > >
> > > > > > > Is the intent to register the clk twice? I believe things are working as
> > > > > > > intended without this patch, so maybe you can explain a little more
> > what
> > > > > > > you're trying to fix.
> > > > > > [Rajan] Yes. During of_clk_init() if some DT fixed factor clock has
> > > > > > parent which is neither mentioned in output-clock-names of clock
> > > > > > controller nor registered as clock provider, of_clk_init() will try to
> > > > > > forcefully register in second loop.
> > > > > >
> > > > > >                         if (force || parent_ready(clk_provider->np)) {
> > > > > >
> > > > > >                                 /* Don't populate platform devices */
> > > > > >                                 of_node_set_flag(clk_provider->np,
> > > > > >                                                  OF_POPULATED);
> > > > > >
> > > > > > So registration of this DT fixed-factor clock would fail as parent
> > > > > > would be NULL as below (called from _of_fixed_factor_clk_setup()):
> > > > > > parent_name = of_clk_get_parent_name(node, 0);
> > > > > >
> > > > > > On the other hand, even if registration failed, that node will be
> > > > > > marked as OF_POPULATED, so probe of clk-fixed-factor.c will also not
> > > > > > be called and that DT fixed-factor clock would never be registered.
> > > > > >
> > > > > > Same thing is discussed at  https://lkml.org/lkml/2017/6/5/681 .
> > > > >
> > > > > Ok. I believe the answer is to fix the DT to describe the parent chain
> > > > > properly with clock-output-names. Otherwise, we have no good way of
> > > > > figuring out what the name should be.
> > > > [Rajan] clock DT binding doc says that clock-output-names property
> > > > is optional and sometimes not recommended.
> > > > I think this patch fixes the issue we have which mandates to add clock-
> > output-
> > > > names
> > > > property (for this particular case). Also, IIUC platform driver probe in clk-
> > fixed-
> > > > factor.c
> > > > will never be called unless we use CLK_OF_DECLARE_DRIVER.
> > > > I completely agree that proper solution would be to stop using strings to
> > > > describe clock topology.
> > > [Rajan] Any comments on this?
> > > Unless we have proper solution ready, we need to have some mechanism to
> > handle this scenario.
> > > clock-output-names is optional and without this, it mandates to use clock-
> > output-names.
> > >
> >
> > I couldn't read your outlook sent email and I was too busy to go
> > translate it. Some bug in my MUA it seems.
> >
> > Can you add the output-names property? In this case it's practically
> > mandatory, so if you can't do it I'd like to hear why not.
> [Rajan] In our case, we are firmware maintains clock database and driver query
> clocks
> from firmware and registers clock based on information received from
> firmware. So
> output clock names are not fixed.
> https://patchwork.kernel.org/patch/10439893/ - dt-bindings: clock: Add
> bindings for ZynqMP clock driver
> https://patchwork.kernel.org/patch/10439891/ - drivers: clk: Add ZynqMP
> clock driver
[Rajan] Any comments?

RE: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Stephen Boyd]

Quoting Rajan Vaja (2018-06-03 20:41:27)
> > > > > > On the other hand, even if registration failed, that node will be
> > > > > > marked as OF_POPULATED, so probe of clk-fixed-factor.c will also not
> > > > > > be called and that DT fixed-factor clock would never be registered.
> > > > > >
> > > > > > Same thing is discussed at  https://lkml.org/lkml/2017/6/5/681 .
> > > > >
> > > > > Ok. I believe the answer is to fix the DT to describe the parent chain
> > > > > properly with clock-output-names. Otherwise, we have no good way of
> > > > > figuring out what the name should be.
> > > > [Rajan] clock DT binding doc says that clock-output-names property
> > > > is optional and sometimes not recommended.
> > > > I think this patch fixes the issue we have which mandates to add clock-
> > output-
> > > > names
> > > > property (for this particular case). Also, IIUC platform driver probe in clk-
> > fixed-
> > > > factor.c
> > > > will never be called unless we use CLK_OF_DECLARE_DRIVER.
> > > > I completely agree that proper solution would be to stop using strings to
> > > > describe clock topology.
> > > [Rajan] Any comments on this?
> > > Unless we have proper solution ready, we need to have some mechanism to
> > handle this scenario.
> > > clock-output-names is optional and without this, it mandates to use clock-
> > output-names.
> > >
> > 
> > I couldn't read your outlook sent email and I was too busy to go
> > translate it. Some bug in my MUA it seems.
> > 
> > Can you add the output-names property? In this case it's practically
> > mandatory, so if you can't do it I'd like to hear why not.
> [Rajan] In our case, we are firmware maintains clock database and driver query clocks
> from firmware and registers clock based on information received from firmware. So
> output clock names are not fixed. 
> https://patchwork.kernel.org/patch/10439893/ - dt-bindings: clock: Add bindings for ZynqMP clock driver
> https://patchwork.kernel.org/patch/10439891/ - drivers: clk: Add ZynqMP clock driver

output-names are fixed by the time firmware is made. Is the DT also
fixed by the time firmware is made? Why can't the DT be made to match
what the firmware is describing by having the firmware update the DT to
describe it's output names?

And what is this fixed factor clk in DT for?

I'm beginning to think that maybe we should make the fixed factor setup
a little worse by having it unmark itself as OF_POPULATED if it finds
that the DT node has a clocks property that it can't find a name for.
Hopefully we can get by with it registering later on because it isn't
critical for the system to bootstrap interrupts and timers.

I take it you understand that changing the code to be
CLK_OF_DECLARE_DRIVER will be actively bad and cause the clk to be
registered potentially twice so we really need to fix the real issue
which is that the parent name can't be figured out until later on, so
the CLK_OF_DECLARE path needs to fail when it can't find the parent it
needs and then manually mark itself as not populated in that case so
that it probes later on with the platform device driver. Unmarking the
node can actually be used to flag a failure in the init function so that
we don't keep trying to do things with the node until driver time too.

RE: [PATCH] clk: clk-fixed-factor: Use new macro CLK_OF_DECLARE_DRIVER

[Rajan Vaja]

Hi Stephen,

> -----Original Message-----
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Sent: 09 July 2018 12:56 PM
> To: Rajan Vaja <RAJANV@xilinx.com>
> Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Jolly Shah
> <JOLLYS@xilinx.com>; Michal Simek <michals@xilinx.com>;
> mturquette@baylibre.com
> Subject: RE: [PATCH] clk: clk-fixed-factor: Use new macro
> CLK_OF_DECLARE_DRIVER
> 
> EXTERNAL EMAIL
> 
> Quoting Rajan Vaja (2018-06-03 20:41:27)
> > > > > > > On the other hand, even if registration failed, that node will be
> > > > > > > marked as OF_POPULATED, so probe of clk-fixed-factor.c will also
> not
> > > > > > > be called and that DT fixed-factor clock would never be registered.
> > > > > > >
> > > > > > > Same thing is discussed at  https://lkml.org/lkml/2017/6/5/681 .
> > > > > >
> > > > > > Ok. I believe the answer is to fix the DT to describe the parent chain
> > > > > > properly with clock-output-names. Otherwise, we have no good way
> of
> > > > > > figuring out what the name should be.
> > > > > [Rajan] clock DT binding doc says that clock-output-names property
> > > > > is optional and sometimes not recommended.
> > > > > I think this patch fixes the issue we have which mandates to add clock-
> > > output-
> > > > > names
> > > > > property (for this particular case). Also, IIUC platform driver probe in clk-
> > > fixed-
> > > > > factor.c
> > > > > will never be called unless we use CLK_OF_DECLARE_DRIVER.
> > > > > I completely agree that proper solution would be to stop using strings to
> > > > > describe clock topology.
> > > > [Rajan] Any comments on this?
> > > > Unless we have proper solution ready, we need to have some mechanism
> to
> > > handle this scenario.
> > > > clock-output-names is optional and without this, it mandates to use clock-
> > > output-names.
> > > >
> > >
> > > I couldn't read your outlook sent email and I was too busy to go
> > > translate it. Some bug in my MUA it seems.
> > >
> > > Can you add the output-names property? In this case it's practically
> > > mandatory, so if you can't do it I'd like to hear why not.
> > [Rajan] In our case, we are firmware maintains clock database and driver
> query clocks
> > from firmware and registers clock based on information received from
> firmware. So
> > output clock names are not fixed.
> > https://patchwork.kernel.org/patch/10439893/ - dt-bindings: clock: Add
> bindings for ZynqMP clock driver
> > https://patchwork.kernel.org/patch/10439891/ - drivers: clk: Add ZynqMP
> clock driver
> 
> output-names are fixed by the time firmware is made. Is the DT also
> fixed by the time firmware is made? Why can't the DT be made to match
> what the firmware is describing by having the firmware update the DT to
> describe it's output names?
[Rajan] The idea is to have a generic driver which queries clock for the variant from
Firmware. So if some clock changes, driver remains same.
> 
> And what is this fixed factor clk in DT for?
> 
> I'm beginning to think that maybe we should make the fixed factor setup
> a little worse by having it unmark itself as OF_POPULATED if it finds
> that the DT node has a clocks property that it can't find a name for.
> Hopefully we can get by with it registering later on because it isn't
> critical for the system to bootstrap interrupts and timers.
[Rajan] I have submitted separate change for this https://patchwork.kernel.org/patch/10529387/. 

> 
> I take it you understand that changing the code to be
> CLK_OF_DECLARE_DRIVER will be actively bad and cause the clk to be
> registered potentially twice so we really need to fix the real issue
> which is that the parent name can't be figured out until later on, so
> the CLK_OF_DECLARE path needs to fail when it can't find the parent it
> needs and then manually mark itself as not populated in that case so
> that it probes later on with the platform device driver. Unmarking the
> node can actually be used to flag a failure in the init function so that
> we don't keep trying to do things with the node until driver time too.
[Rajan] I understood that making it CLK_OF_DECLARE_DRIVER will try to register clock twice.