[PATCH] clk: Register clkdev after setup of fixed-rate and fixed-factor clocks

[PATCH] clk: Register clkdev after setup of fixed-rate and fixed-factor clocks

[Orson Zhai]

From: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>

When common kernel setups fixed clock, of_clk_provider will be registerred.
But there is no clkdev being registerred at the same time. This will make
it difficult to get the clock by using clk_get(NULL, con_id).

Add clkdev register for fixed-rate and fixed-factor clock and ignore
the error if any.

Signed-off-by: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
---
 drivers/clk/clk-fixed-factor.c | 5 +++++
 drivers/clk/clk-fixed-rate.c   | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/clk/clk-fixed-factor.c b/drivers/clk/clk-fixed-factor.c
index a5d402d..b81656a 100644
--- a/drivers/clk/clk-fixed-factor.c
+++ b/drivers/clk/clk-fixed-factor.c
@@ -9,6 +9,7 @@
  */
 #include <linux/module.h>
 #include <linux/clk-provider.h>
+#include <linux/clkdev.h>
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/of.h>
@@ -186,6 +187,10 @@ static struct clk *_of_fixed_factor_clk_setup(struct device_node *node)
 		return ERR_PTR(ret);
 	}
 
+#ifdef CONFIG_CLKDEV_LOOKUP
+	clk_register_clkdev(clk, clk_name, 0);
+#endif
+
 	return clk;
 }
 
diff --git a/drivers/clk/clk-fixed-rate.c b/drivers/clk/clk-fixed-rate.c
index b5c46b3..d42c8e7 100644
--- a/drivers/clk/clk-fixed-rate.c
+++ b/drivers/clk/clk-fixed-rate.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/clkdev.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/io.h>
@@ -184,6 +185,10 @@ static struct clk *_of_fixed_clk_setup(struct device_node *node)
 		return ERR_PTR(ret);
 	}
 
+#ifdef CONFIG_CLKDEV_LOOKUP
+	clk_register_clkdev(clk, clk_name, 0);
+#endif
+
 	return clk;
 }
 
-- 
1.9.1

Re: [PATCH] clk: Register clkdev after setup of fixed-rate and fixed-factor clocks

[Stephen Boyd]

On 10/11, Orson Zhai wrote:
> From: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> 
> When common kernel setups fixed clock, of_clk_provider will be registerred.
> But there is no clkdev being registerred at the same time. This will make
> it difficult to get the clock by using clk_get(NULL, con_id).
> 
> Add clkdev register for fixed-rate and fixed-factor clock and ignore
> the error if any.
> 
> Signed-off-by: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> ---

Why are we using clkdev lookups for clks populated from DT?
Shouldn't we be able to point to them from the consumers that
would also be in DT? I'm a little lost.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: Register clkdev after setup of fixed-rate and fixed-factor clocks

[Xiaolong Zhang]

On 四, 10月 20, 2016 at 04:01:03下午 -0700, Stephen Boyd wrote:
> On 10/11, Orson Zhai wrote:
> > From: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> > 
> > When common kernel setups fixed clock, of_clk_provider will be registerred.
> > But there is no clkdev being registerred at the same time. This will make
> > it difficult to get the clock by using clk_get(NULL, con_id).
> > 
> > Add clkdev register for fixed-rate and fixed-factor clock and ignore
> > the error if any.
> > 
> > Signed-off-by: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> > Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> > ---
> 
> Why are we using clkdev lookups for clks populated from DT?
> Shouldn't we be able to point to them from the consumers that
> would also be in DT? I'm a little lost.
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
The clk_get interface allows the first argument as NULL. We just assure
consumers can get the clock from DT or by clock name.

Re: [PATCH] clk: Register clkdev after setup of fixed-rate and fixed-factor clocks

[Stephen Boyd]

On 10/22, Xiaolong Zhang wrote:
> On 四, 10月 20, 2016 at 04:01:03下午 -0700, Stephen Boyd wrote:
> > On 10/11, Orson Zhai wrote:
> > > From: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> > > 
> > > When common kernel setups fixed clock, of_clk_provider will be registerred.
> > > But there is no clkdev being registerred at the same time. This will make
> > > it difficult to get the clock by using clk_get(NULL, con_id).
> > > 
> > > Add clkdev register for fixed-rate and fixed-factor clock and ignore
> > > the error if any.
> > > 
> > > Signed-off-by: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> > > Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> > > ---
> > 
> > Why are we using clkdev lookups for clks populated from DT?
> > Shouldn't we be able to point to them from the consumers that
> > would also be in DT? I'm a little lost.
> > 
> The clk_get interface allows the first argument as NULL. We just assure
> consumers can get the clock from DT or by clock name.

Ok. The first argument to clk_get() really shouldn't be NULL. It
should be the device pointer for the device that is associated
with the clk you want to get. The clock name (second argument to
clk_get()) should be device specific and not some globally unique
identifier. It seems that you're using the clk_get() API
incorrectly.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: Register clkdev after setup of fixed-rate and fixed-factor clocks

[Xiaolong Zhang]

On 二, 10月 25, 2016 at 08:40:08下午 +0000, Stephen Boyd wrote:
> On 10/22, Xiaolong Zhang wrote:
> > On 四, 10月 20, 2016 at 04:01:03下午 -0700, Stephen Boyd wrote:
> > > On 10/11, Orson Zhai wrote:
> > > > From: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> > > > 
> > > > When common kernel setups fixed clock, of_clk_provider will be registerred.
> > > > But there is no clkdev being registerred at the same time. This will make
> > > > it difficult to get the clock by using clk_get(NULL, con_id).
> > > > 
> > > > Add clkdev register for fixed-rate and fixed-factor clock and ignore
> > > > the error if any.
> > > > 
> > > > Signed-off-by: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> > > > Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> > > > ---
> > > 
> > > Why are we using clkdev lookups for clks populated from DT?
> > > Shouldn't we be able to point to them from the consumers that
> > > would also be in DT? I'm a little lost.
> > > 
> > The clk_get interface allows the first argument as NULL. We just assure
> > consumers can get the clock from DT or by clock name.
> 
> Ok. The first argument to clk_get() really shouldn't be NULL. It
> should be the device pointer for the device that is associated
> with the clk you want to get. The clock name (second argument to
> clk_get()) should be device specific and not some globally unique
> identifier. It seems that you're using the clk_get() API
> incorrectly.
>
Sorry for late reply.
There are two paths in clk_get:
a) dev is not NULL, the calling procedure is as following
clk_get(dev, con_id)
--->of_clk_get_by_name(dev->of_node, con_id)

b) dev is NULL, the calling procedure is as following
clk_dev(NULL, con_id)
--->clk_get_sys(NULL, con_id)
    --->clk_find(NULL, con_id)

I just cann't understand why you say the first argument shouldn't
be NULL. Is there other consideration in CCF?

Thanks

Xiaolong Zhang
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH] clk: Register clkdev after setup of fixed-rate and fixed-factor clocks

[Stephen Boyd]

On 11/12, Xiaolong Zhang wrote:
> On 二, 10月 25, 2016 at 08:40:08下午 +0000, Stephen Boyd wrote:
> > On 10/22, Xiaolong Zhang wrote:
> > > On 四, 10月 20, 2016 at 04:01:03下午 -0700, Stephen Boyd wrote:
> > > > On 10/11, Orson Zhai wrote:
> > > > > From: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> > > > > 
> > > > > When common kernel setups fixed clock, of_clk_provider will be registerred.
> > > > > But there is no clkdev being registerred at the same time. This will make
> > > > > it difficult to get the clock by using clk_get(NULL, con_id).
> > > > > 
> > > > > Add clkdev register for fixed-rate and fixed-factor clock and ignore
> > > > > the error if any.
> > > > > 
> > > > > Signed-off-by: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> > > > > Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> > > > > ---
> > > > 
> > > > Why are we using clkdev lookups for clks populated from DT?
> > > > Shouldn't we be able to point to them from the consumers that
> > > > would also be in DT? I'm a little lost.
> > > > 
> > > The clk_get interface allows the first argument as NULL. We just assure
> > > consumers can get the clock from DT or by clock name.
> > 
> > Ok. The first argument to clk_get() really shouldn't be NULL. It
> > should be the device pointer for the device that is associated
> > with the clk you want to get. The clock name (second argument to
> > clk_get()) should be device specific and not some globally unique
> > identifier. It seems that you're using the clk_get() API
> > incorrectly.
> >
> Sorry for late reply.
> There are two paths in clk_get:
> a) dev is not NULL, the calling procedure is as following
> clk_get(dev, con_id)
> --->of_clk_get_by_name(dev->of_node, con_id)
> 
> b) dev is NULL, the calling procedure is as following
> clk_dev(NULL, con_id)
> --->clk_get_sys(NULL, con_id)
>     --->clk_find(NULL, con_id)
> 
> I just cann't understand why you say the first argument shouldn't
> be NULL. Is there other consideration in CCF?
> 

Passing NULL as the first argument is allowed, but that's mostly
an artifact of the clk_get() API. If you don't have a device, you
should be calling clk_get_sys() and then a clkdev lookup should
have been added with some appropriate dev_id string that is used
in the clk_get_sys() call. At least this is my understanding of
the clkdev APIs, but Russell is the authority here.

We're really off track now though. Can you please point to some
code that needs this change? If we're using DT then we should be
able to use the of_clk_*() path to find the clk.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: Register clkdev after setup of fixed-rate and fixed-factor clocks

[Xiaolong Zhang]

On 三, 11月 23, 2016 at 04:38:33下午 -0800, Stephen Boyd wrote:
> On 11/12, Xiaolong Zhang wrote:
> > On 二, 10月 25, 2016 at 08:40:08下午 +0000, Stephen Boyd wrote:
> > > On 10/22, Xiaolong Zhang wrote:
> > > > On 四, 10月 20, 2016 at 04:01:03下午 -0700, Stephen Boyd wrote:
> > > > > On 10/11, Orson Zhai wrote:
> > > > > > From: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> > > > > > 
> > > > > > When common kernel setups fixed clock, of_clk_provider will be registerred.
> > > > > > But there is no clkdev being registerred at the same time. This will make
> > > > > > it difficult to get the clock by using clk_get(NULL, con_id).
> > > > > > 
> > > > > > Add clkdev register for fixed-rate and fixed-factor clock and ignore
> > > > > > the error if any.
> > > > > > 
> > > > > > Signed-off-by: Xiaolong Zhang <xiaolong.zhang@spreadtrum.com>
> > > > > > Signed-off-by: Orson Zhai <orson.zhai@spreadtrum.com>
> > > > > > ---
> > > > > 
> > > > > Why are we using clkdev lookups for clks populated from DT?
> > > > > Shouldn't we be able to point to them from the consumers that
> > > > > would also be in DT? I'm a little lost.
> > > > > 
> > > > The clk_get interface allows the first argument as NULL. We just assure
> > > > consumers can get the clock from DT or by clock name.
> > > 
> > > Ok. The first argument to clk_get() really shouldn't be NULL. It
> > > should be the device pointer for the device that is associated
> > > with the clk you want to get. The clock name (second argument to
> > > clk_get()) should be device specific and not some globally unique
> > > identifier. It seems that you're using the clk_get() API
> > > incorrectly.
> > >
> > Sorry for late reply.
> > There are two paths in clk_get:
> > a) dev is not NULL, the calling procedure is as following
> > clk_get(dev, con_id)
> > --->of_clk_get_by_name(dev->of_node, con_id)
> > 
> > b) dev is NULL, the calling procedure is as following
> > clk_dev(NULL, con_id)
> > --->clk_get_sys(NULL, con_id)
> >     --->clk_find(NULL, con_id)
> > 
> > I just cann't understand why you say the first argument shouldn't
> > be NULL. Is there other consideration in CCF?
> > 
> 
> Passing NULL as the first argument is allowed, but that's mostly
> an artifact of the clk_get() API. If you don't have a device, you
> should be calling clk_get_sys() and then a clkdev lookup should
> have been added with some appropriate dev_id string that is used
> in the clk_get_sys() call. At least this is my understanding of
> the clkdev APIs, but Russell is the authority here.
>

I understand what you mean. If a driver uses the device tree, the
consumers and providers have the fixed relationship. But I think
clk_get() has a purpose is compatible with the non-DT driver.

> We're really off track now though. Can you please point to some
> code that needs this change? If we're using DT then we should be
> able to use the of_clk_*() path to find the clk.
>

Actually, the requirement is raised by our GPU driver. In the
early stage of the GPU DT driver, the GPU driver use the
clk_get(NULL, con_id) to get the clock instance for compatible
with non-DT GPU driver. The new driver have used the of_clk_get()
instead of the clk_get. And we reserved the modification in clock.

Xiaolong Zhang
Thanks
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH] clk: Register clkdev after setup of fixed-rate and fixed-factor clocks

[Stephen Boyd]

On 11/24, Xiaolong Zhang wrote:
> On 三, 11月 23, 2016 at 04:38:33下午 -0800, Stephen Boyd wrote:
> 
> > We're really off track now though. Can you please point to some
> > code that needs this change? If we're using DT then we should be
> > able to use the of_clk_*() path to find the clk.
> >
> 
> Actually, the requirement is raised by our GPU driver. In the
> early stage of the GPU DT driver, the GPU driver use the
> clk_get(NULL, con_id) to get the clock instance for compatible
> with non-DT GPU driver. The new driver have used the of_clk_get()
> instead of the clk_get. And we reserved the modification in clock.
> 

Ok the non-DT version of the GPU driver should be modified to
call clk_get() and pass in the device. The con_id argument there
should be something specific to the GPU device, and not a global
name of a clock on the system. When the clkdev lookup is
populated on the non-DT board make sure to set the dev_id string
to match the device name of the GPU device.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] clk: Register clkdev after setup of fixed-rate and fixed-factor clocks

[Xiaolong Zhang]

On 二, 11月 29, 2016 at 01:10:54下午 -0800, Stephen Boyd wrote:
> On 11/24, Xiaolong Zhang wrote:
> > On 三, 11月 23, 2016 at 04:38:33下午 -0800, Stephen Boyd wrote:
> > 
> > > We're really off track now though. Can you please point to some
> > > code that needs this change? If we're using DT then we should be
> > > able to use the of_clk_*() path to find the clk.
> > >
> > 
> > Actually, the requirement is raised by our GPU driver. In the
> > early stage of the GPU DT driver, the GPU driver use the
> > clk_get(NULL, con_id) to get the clock instance for compatible
> > with non-DT GPU driver. The new driver have used the of_clk_get()
> > instead of the clk_get. And we reserved the modification in clock.
> > 
> 
> Ok the non-DT version of the GPU driver should be modified to
> call clk_get() and pass in the device. The con_id argument there
> should be something specific to the GPU device, and not a global
> name of a clock on the system. When the clkdev lookup is
> populated on the non-DT board make sure to set the dev_id string
> to match the device name of the GPU device.
>
Ok, Thanks sBoyd!
>
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project