* [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
@ 2009-12-01 17:18 m-karicheri2
2009-12-01 17:19 ` [PATCH - v0 2/2] DaVinci - vpfe capture - Make " m-karicheri2
2009-12-09 16:00 ` [PATCH - v1 1/2] V4L - vpfe capture - make " Kevin Hilman
0 siblings, 2 replies; 19+ messages in thread
From: m-karicheri2 @ 2009-12-01 17:18 UTC (permalink / raw)
To: linux-media, hverkuil, khilman
Cc: davinci-linux-open-source, hvaibhav, Muralidharan Karicheri
From: Muralidharan Karicheri <m-karicheri2@ti.com>
v1 - updated based on comments from Vaibhav Hiremath.
On DM365 we use only vpss master clock, where as on DM355 and
DM6446, we use vpss master and slave clocks for vpfe capture and AM3517
we use internal clock and pixel clock. So this patch makes it configurable
on a per platform basis. This is needed for supporting DM365 for which patches
will be available soon.
Tested-by: Vaibhav Hiremath <hvaibhav@ti.com>, Muralidharan Karicheri <m-karicheri2@ti.com>
Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Muralidharan Karicheri <m-karicheri2@ti.com>
---
drivers/media/video/davinci/vpfe_capture.c | 98 +++++++++++++++++----------
include/media/davinci/vpfe_capture.h | 11 ++-
2 files changed, 70 insertions(+), 39 deletions(-)
diff --git a/drivers/media/video/davinci/vpfe_capture.c b/drivers/media/video/davinci/vpfe_capture.c
index 12a1b3d..c3468ee 100644
--- a/drivers/media/video/davinci/vpfe_capture.c
+++ b/drivers/media/video/davinci/vpfe_capture.c
@@ -1787,61 +1787,87 @@ static struct vpfe_device *vpfe_initialize(void)
return vpfe_dev;
}
+/**
+ * vpfe_disable_clock() - Disable clocks for vpfe capture driver
+ * @vpfe_dev - ptr to vpfe capture device
+ *
+ * Disables clocks defined in vpfe configuration.
+ */
static void vpfe_disable_clock(struct vpfe_device *vpfe_dev)
{
struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
+ int i;
- clk_disable(vpfe_cfg->vpssclk);
- clk_put(vpfe_cfg->vpssclk);
- clk_disable(vpfe_cfg->slaveclk);
- clk_put(vpfe_cfg->slaveclk);
- v4l2_info(vpfe_dev->pdev->driver,
- "vpfe vpss master & slave clocks disabled\n");
+ for (i = 0; i < vpfe_cfg->num_clocks; i++) {
+ clk_disable(vpfe_dev->clks[i]);
+ clk_put(vpfe_dev->clks[i]);
+ }
+ kfree(vpfe_dev->clks);
+ v4l2_info(vpfe_dev->pdev->driver, "vpfe capture clocks disabled\n");
}
+/**
+ * vpfe_enable_clock() - Enable clocks for vpfe capture driver
+ * @vpfe_dev - ptr to vpfe capture device
+ *
+ * Enables clocks defined in vpfe configuration. The function
+ * assumes that at least one clock is to be defined which is
+ * true as of now. re-visit this if this assumption is not true
+ */
static int vpfe_enable_clock(struct vpfe_device *vpfe_dev)
{
struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
- int ret = -ENOENT;
+ int i;
- vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "vpss_master");
- if (NULL == vpfe_cfg->vpssclk) {
- v4l2_err(vpfe_dev->pdev->driver, "No clock defined for"
- "vpss_master\n");
- return ret;
- }
+ if (!vpfe_cfg->num_clocks)
+ return 0;
- if (clk_enable(vpfe_cfg->vpssclk)) {
- v4l2_err(vpfe_dev->pdev->driver,
- "vpfe vpss master clock not enabled\n");
- goto out;
- }
- v4l2_info(vpfe_dev->pdev->driver,
- "vpfe vpss master clock enabled\n");
+ vpfe_dev->clks = kzalloc(vpfe_cfg->num_clocks *
+ sizeof(struct clock *), GFP_KERNEL);
- vpfe_cfg->slaveclk = clk_get(vpfe_dev->pdev, "vpss_slave");
- if (NULL == vpfe_cfg->slaveclk) {
- v4l2_err(vpfe_dev->pdev->driver,
- "No clock defined for vpss slave\n");
- goto out;
+ if (NULL == vpfe_dev->clks) {
+ v4l2_err(vpfe_dev->pdev->driver, "Memory allocation failed\n");
+ return -ENOMEM;
}
- if (clk_enable(vpfe_cfg->slaveclk)) {
- v4l2_err(vpfe_dev->pdev->driver,
- "vpfe vpss slave clock not enabled\n");
- goto out;
+ for (i = 0; i < vpfe_cfg->num_clocks; i++) {
+ if (NULL == vpfe_cfg->clocks[i]) {
+ v4l2_err(vpfe_dev->pdev->driver,
+ "clock %s is not defined in vpfe config\n",
+ vpfe_cfg->clocks[i]);
+ goto out;
+ }
+
+ vpfe_dev->clks[i] = clk_get(vpfe_dev->pdev,
+ vpfe_cfg->clocks[i]);
+ if (NULL == vpfe_dev->clks[i]) {
+ v4l2_err(vpfe_dev->pdev->driver,
+ "Failed to get clock %s\n",
+ vpfe_cfg->clocks[i]);
+ goto out;
+ }
+
+ if (clk_enable(vpfe_dev->clks[i])) {
+ v4l2_err(vpfe_dev->pdev->driver,
+ "vpfe clock %s not enabled\n",
+ vpfe_cfg->clocks[i]);
+ goto out;
+ }
+
+ v4l2_info(vpfe_dev->pdev->driver, "vpss clock %s enabled",
+ vpfe_cfg->clocks[i]);
}
- v4l2_info(vpfe_dev->pdev->driver, "vpfe vpss slave clock enabled\n");
return 0;
out:
- if (vpfe_cfg->vpssclk)
- clk_put(vpfe_cfg->vpssclk);
- if (vpfe_cfg->slaveclk)
- clk_put(vpfe_cfg->slaveclk);
-
- return -1;
+ for (i = 0; i < vpfe_cfg->num_clocks; i++) {
+ if (vpfe_dev->clks[i])
+ clk_put(vpfe_dev->clks[i]);
+ }
+ kfree(vpfe_dev->clks);
+ return -EFAULT;
}
+
/*
* vpfe_probe : This function creates device entries by register
* itself to the V4L2 driver and initializes fields of each
diff --git a/include/media/davinci/vpfe_capture.h b/include/media/davinci/vpfe_capture.h
index d863e5e..7b62a5c 100644
--- a/include/media/davinci/vpfe_capture.h
+++ b/include/media/davinci/vpfe_capture.h
@@ -31,8 +31,6 @@
#include <media/videobuf-dma-contig.h>
#include <media/davinci/vpfe_types.h>
-#define VPFE_CAPTURE_NUM_DECODERS 5
-
/* Macros */
#define VPFE_MAJOR_RELEASE 0
#define VPFE_MINOR_RELEASE 0
@@ -91,9 +89,14 @@ struct vpfe_config {
char *card_name;
/* ccdc name */
char *ccdc;
- /* vpfe clock */
+ /* vpfe clock. Obsolete! Will be removed in next patch */
struct clk *vpssclk;
+ /* Obsolete! Will be removed in next patch */
struct clk *slaveclk;
+ /* number of clocks */
+ int num_clocks;
+ /* clocks used for vpfe capture */
+ char *clocks[];
};
struct vpfe_device {
@@ -104,6 +107,8 @@ struct vpfe_device {
struct v4l2_subdev **sd;
/* vpfe cfg */
struct vpfe_config *cfg;
+ /* clock ptrs for vpfe capture */
+ struct clk **clks;
/* V4l2 device */
struct v4l2_device v4l2_dev;
/* parent device */
--
1.6.0.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH - v0 2/2] DaVinci - vpfe capture - Make clocks configurable
2009-12-01 17:18 [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable m-karicheri2
@ 2009-12-01 17:19 ` m-karicheri2
2009-12-03 6:40 ` Hiremath, Vaibhav
2009-12-09 16:00 ` [PATCH - v1 1/2] V4L - vpfe capture - make " Kevin Hilman
1 sibling, 1 reply; 19+ messages in thread
From: m-karicheri2 @ 2009-12-01 17:19 UTC (permalink / raw)
To: linux-media, hverkuil, khilman
Cc: davinci-linux-open-source, hvaibhav, Muralidharan Karicheri
From: Muralidharan Karicheri <m-karicheri2@ti.com>
Adding the clocks in vpfe capture configuration
Signed-off-by: Muralidharan Karicheri <m-karicheri2@ti.com>
---
arch/arm/mach-davinci/board-dm355-evm.c | 2 ++
arch/arm/mach-davinci/board-dm644x-evm.c | 2 ++
2 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-davinci/board-dm355-evm.c b/arch/arm/mach-davinci/board-dm355-evm.c
index a9b650d..a28985c 100644
--- a/arch/arm/mach-davinci/board-dm355-evm.c
+++ b/arch/arm/mach-davinci/board-dm355-evm.c
@@ -239,6 +239,8 @@ static struct vpfe_config vpfe_cfg = {
.sub_devs = vpfe_sub_devs,
.card_name = "DM355 EVM",
.ccdc = "DM355 CCDC",
+ .num_clocks = 2,
+ .clocks = {"vpss_master", "vpss_slave"},
};
static struct platform_device *davinci_evm_devices[] __initdata = {
diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c
index fd0398b..45beb99 100644
--- a/arch/arm/mach-davinci/board-dm644x-evm.c
+++ b/arch/arm/mach-davinci/board-dm644x-evm.c
@@ -250,6 +250,8 @@ static struct vpfe_config vpfe_cfg = {
.sub_devs = vpfe_sub_devs,
.card_name = "DM6446 EVM",
.ccdc = "DM6446 CCDC",
+ .num_clocks = 2,
+ .clocks = {"vpss_master", "vpss_slave"},
};
static struct platform_device rtc_dev = {
--
1.6.0.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [PATCH - v0 2/2] DaVinci - vpfe capture - Make clocks configurable
2009-12-01 17:19 ` [PATCH - v0 2/2] DaVinci - vpfe capture - Make " m-karicheri2
@ 2009-12-03 6:40 ` Hiremath, Vaibhav
2009-12-03 15:55 ` Karicheri, Muralidharan
2009-12-04 23:12 ` Karicheri, Muralidharan
0 siblings, 2 replies; 19+ messages in thread
From: Hiremath, Vaibhav @ 2009-12-03 6:40 UTC (permalink / raw)
To: Karicheri, Muralidharan, linux-media, hverkuil, khilman
Cc: davinci-linux-open-source
> -----Original Message-----
> From: Karicheri, Muralidharan
> Sent: Tuesday, December 01, 2009 10:49 PM
> To: linux-media@vger.kernel.org; hverkuil@xs4all.nl;
> khilman@deeprootsystems.com
> Cc: davinci-linux-open-source@linux.davincidsp.com; Hiremath,
> Vaibhav; Karicheri, Muralidharan
> Subject: [PATCH - v0 2/2] DaVinci - vpfe capture - Make clocks
> configurable
>
> From: Muralidharan Karicheri <m-karicheri2@ti.com>
>
> Adding the clocks in vpfe capture configuration
>
> Signed-off-by: Muralidharan Karicheri <m-karicheri2@ti.com>
> ---
> arch/arm/mach-davinci/board-dm355-evm.c | 2 ++
> arch/arm/mach-davinci/board-dm644x-evm.c | 2 ++
> 2 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/board-dm355-evm.c
> b/arch/arm/mach-davinci/board-dm355-evm.c
> index a9b650d..a28985c 100644
> --- a/arch/arm/mach-davinci/board-dm355-evm.c
> +++ b/arch/arm/mach-davinci/board-dm355-evm.c
> @@ -239,6 +239,8 @@ static struct vpfe_config vpfe_cfg = {
> .sub_devs = vpfe_sub_devs,
> .card_name = "DM355 EVM",
> .ccdc = "DM355 CCDC",
> + .num_clocks = 2,
> + .clocks = {"vpss_master", "vpss_slave"},
[Hiremath, Vaibhav] Hi Murali,
I was talking to Sekhar about this and actually he made some good points about this implementation.
If we consider specific IP, then the required clocks would remain always be the same. There might be some devices which may not be using some clocks (so as that specific feature).
Actually we are trying to create one more wrapper for clock configuration. Just to illustrate I am putting some other generic drivers examples -
Omap-hsmmc.c -
This driver requires 2 clocks, interface and functional. The devices which would be using this driver have to define clock with names "ick" and "fck".
VPFE-Capture (Considering only current implementation) -
Currently we have vpfe_capture.c file (master/bridge driver) which is handling clk_get/put, and platform data is providing the details about it.
Ideally we should handle it in respective ccdc driver file, since he has all the knowledge about required number of clocks and its name. This way we don't have to maintain/pass clock information in platform data.
I would appreciate any comments/thoughts/pointers here.
Thanks,
Vaibhav
> };
>
> static struct platform_device *davinci_evm_devices[] __initdata = {
> diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c
> b/arch/arm/mach-davinci/board-dm644x-evm.c
> index fd0398b..45beb99 100644
> --- a/arch/arm/mach-davinci/board-dm644x-evm.c
> +++ b/arch/arm/mach-davinci/board-dm644x-evm.c
> @@ -250,6 +250,8 @@ static struct vpfe_config vpfe_cfg = {
> .sub_devs = vpfe_sub_devs,
> .card_name = "DM6446 EVM",
> .ccdc = "DM6446 CCDC",
> + .num_clocks = 2,
> + .clocks = {"vpss_master", "vpss_slave"},
> };
>
> static struct platform_device rtc_dev = {
> --
> 1.6.0.4
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH - v0 2/2] DaVinci - vpfe capture - Make clocks configurable
2009-12-03 6:40 ` Hiremath, Vaibhav
@ 2009-12-03 15:55 ` Karicheri, Muralidharan
2009-12-08 17:27 ` Hiremath, Vaibhav
2009-12-04 23:12 ` Karicheri, Muralidharan
1 sibling, 1 reply; 19+ messages in thread
From: Karicheri, Muralidharan @ 2009-12-03 15:55 UTC (permalink / raw)
To: Hiremath, Vaibhav, linux-media, hverkuil, khilman
Cc: davinci-linux-open-source
>I was talking to Sekhar about this and actually he made some good points
>about this implementation.
>
>If we consider specific IP, then the required clocks would remain always be
>the same. There might be some devices which may not be using some clocks
>(so as that specific feature).
>
>Actually we are trying to create one more wrapper for clock configuration.
>Just to illustrate I am putting some other generic drivers examples -
>
>Omap-hsmmc.c -
>
>This driver requires 2 clocks, interface and functional. The devices which
>would be using this driver have to define clock with names "ick" and "fck".
>
>VPFE-Capture (Considering only current implementation) -
>
>Currently we have vpfe_capture.c file (master/bridge driver) which is
>handling clk_get/put, and platform data is providing the details about it.
>Ideally we should handle it in respective ccdc driver file, since he has
>all the knowledge about required number of clocks and its name. This way we
>don't have to maintain/pass clock information in platform data.
>
>I would appreciate any comments/thoughts/pointers here.
>
Though I agree that this clock could be set by the ccdc driver, I am not sure if the same clock is used by an IP on different SOCs. For example take
the case of ccdc on DM6446 which is also used on OMAP 35xx SOC. Do they use
vpss master and slave clocks as is done on DM6446? If this is true, then we
could set the clock inside ccdc driver.
Let me know so that I can re-work the patch and send it to the list.
Murali
>Thanks,
>Vaibhav
>
>> };
>>
>> static struct platform_device *davinci_evm_devices[] __initdata = {
>> diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c
>> b/arch/arm/mach-davinci/board-dm644x-evm.c
>> index fd0398b..45beb99 100644
>> --- a/arch/arm/mach-davinci/board-dm644x-evm.c
>> +++ b/arch/arm/mach-davinci/board-dm644x-evm.c
>> @@ -250,6 +250,8 @@ static struct vpfe_config vpfe_cfg = {
>> .sub_devs = vpfe_sub_devs,
>> .card_name = "DM6446 EVM",
>> .ccdc = "DM6446 CCDC",
>> + .num_clocks = 2,
>> + .clocks = {"vpss_master", "vpss_slave"},
>> };
>>
>> static struct platform_device rtc_dev = {
>> --
>> 1.6.0.4
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH - v0 2/2] DaVinci - vpfe capture - Make clocks configurable
2009-12-03 6:40 ` Hiremath, Vaibhav
2009-12-03 15:55 ` Karicheri, Muralidharan
@ 2009-12-04 23:12 ` Karicheri, Muralidharan
1 sibling, 0 replies; 19+ messages in thread
From: Karicheri, Muralidharan @ 2009-12-04 23:12 UTC (permalink / raw)
To: Karicheri, Muralidharan, Hiremath, Vaibhav, linux-media,
hverkuil, khilman
Cc: davinci-linux-open-source
Vaibhav,
Could you confirm my question below? I need to submit a patch on Monday.
>>Currently we have vpfe_capture.c file (master/bridge driver) which is
>>handling clk_get/put, and platform data is providing the details about it.
>>Ideally we should handle it in respective ccdc driver file, since he has
>>all the knowledge about required number of clocks and its name. This way
>we
>>don't have to maintain/pass clock information in platform data.
>>
>>I would appreciate any comments/thoughts/pointers here.
>>
>Though I agree that this clock could be set by the ccdc driver, I am not
>sure if the same clock is used by an IP on different SOCs. For example take
>the case of ccdc on DM6446 which is also used on OMAP 35xx SOC. Do they use
>vpss master and slave clocks as is done on DM6446? If this is true, then we
>could set the clock inside ccdc driver.
>
>Let me know so that I can re-work the patch and send it to the list.
>
>Murali
>>Thanks,
>>Vaibhav
>>
>>> };
>>>
>>> static struct platform_device *davinci_evm_devices[] __initdata = {
>>> diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c
>>> b/arch/arm/mach-davinci/board-dm644x-evm.c
>>> index fd0398b..45beb99 100644
>>> --- a/arch/arm/mach-davinci/board-dm644x-evm.c
>>> +++ b/arch/arm/mach-davinci/board-dm644x-evm.c
>>> @@ -250,6 +250,8 @@ static struct vpfe_config vpfe_cfg = {
>>> .sub_devs = vpfe_sub_devs,
>>> .card_name = "DM6446 EVM",
>>> .ccdc = "DM6446 CCDC",
>>> + .num_clocks = 2,
>>> + .clocks = {"vpss_master", "vpss_slave"},
>>> };
>>>
>>> static struct platform_device rtc_dev = {
>>> --
>>> 1.6.0.4
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH - v0 2/2] DaVinci - vpfe capture - Make clocks configurable
2009-12-03 15:55 ` Karicheri, Muralidharan
@ 2009-12-08 17:27 ` Hiremath, Vaibhav
2009-12-08 20:09 ` Karicheri, Muralidharan
0 siblings, 1 reply; 19+ messages in thread
From: Hiremath, Vaibhav @ 2009-12-08 17:27 UTC (permalink / raw)
To: Karicheri, Muralidharan, linux-media, hverkuil, khilman
Cc: davinci-linux-open-source
> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Karicheri, Muralidharan
> Sent: Thursday, December 03, 2009 9:25 PM
> To: Hiremath, Vaibhav; linux-media@vger.kernel.org;
> hverkuil@xs4all.nl; khilman@deeprootsystems.com
> Cc: davinci-linux-open-source@linux.davincidsp.com
> Subject: RE: [PATCH - v0 2/2] DaVinci - vpfe capture - Make clocks
> configurable
>
> >I was talking to Sekhar about this and actually he made some good
> points
> >about this implementation.
> >
> >If we consider specific IP, then the required clocks would remain
> always be
> >the same. There might be some devices which may not be using some
> clocks
> >(so as that specific feature).
> >
> >Actually we are trying to create one more wrapper for clock
> configuration.
> >Just to illustrate I am putting some other generic drivers examples
> -
> >
> >Omap-hsmmc.c -
> >
> >This driver requires 2 clocks, interface and functional. The
> devices which
> >would be using this driver have to define clock with names "ick"
> and "fck".
> >
> >VPFE-Capture (Considering only current implementation) -
> >
> >Currently we have vpfe_capture.c file (master/bridge driver) which
> is
> >handling clk_get/put, and platform data is providing the details
> about it.
> >Ideally we should handle it in respective ccdc driver file, since
> he has
> >all the knowledge about required number of clocks and its name.
> This way we
> >don't have to maintain/pass clock information in platform data.
> >
> >I would appreciate any comments/thoughts/pointers here.
> >
> Though I agree that this clock could be set by the ccdc driver, I am
> not sure if the same clock is used by an IP on different SOCs. For
> example take
> the case of ccdc on DM6446 which is also used on OMAP 35xx SOC. Do
> they use
> vpss master and slave clocks as is done on DM6446? If this is true,
> then we
> could set the clock inside ccdc driver.
>
> Let me know so that I can re-work the patch and send it to the list.
>
[Hiremath, Vaibhav] Murali,
They might be using different naming conventions but the number of clocks will always remain the same. If not, then they are not same IP and most probably will not use same driver.
Just to clarify more on your Q, AM3517 also uses 2 clocks
- vpfe_ck (functional clock)
- vpfe_pck (external clock, pixel clock)
Whereas you are referring them as master and slave clock.
Thanks,
Vaibhav
> Murali
> >Thanks,
> >Vaibhav
> >
> >> };
> >>
> >> static struct platform_device *davinci_evm_devices[] __initdata
> = {
> >> diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c
> >> b/arch/arm/mach-davinci/board-dm644x-evm.c
> >> index fd0398b..45beb99 100644
> >> --- a/arch/arm/mach-davinci/board-dm644x-evm.c
> >> +++ b/arch/arm/mach-davinci/board-dm644x-evm.c
> >> @@ -250,6 +250,8 @@ static struct vpfe_config vpfe_cfg = {
> >> .sub_devs = vpfe_sub_devs,
> >> .card_name = "DM6446 EVM",
> >> .ccdc = "DM6446 CCDC",
> >> + .num_clocks = 2,
> >> + .clocks = {"vpss_master", "vpss_slave"},
> >> };
> >>
> >> static struct platform_device rtc_dev = {
> >> --
> >> 1.6.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH - v0 2/2] DaVinci - vpfe capture - Make clocks configurable
2009-12-08 17:27 ` Hiremath, Vaibhav
@ 2009-12-08 20:09 ` Karicheri, Muralidharan
0 siblings, 0 replies; 19+ messages in thread
From: Karicheri, Muralidharan @ 2009-12-08 20:09 UTC (permalink / raw)
To: Hiremath, Vaibhav, linux-media, hverkuil, khilman
Cc: davinci-linux-open-source
Vaibhav,
I have posted a re-worked patch with clocks configuration moved
to ccdc. From your response below, I understand the clocks
are being called differently on different SoCs even though they
use the same IP. So IMO, it is better to customize it on a SoC
by defining the clock names in the respective platform file as
done by my original patch. This will avoid confusion since we need to
keep the name in sync with hardware signal names.
-Murali
>[Hiremath, Vaibhav] Murali,
>
>They might be using different naming conventions but the number of clocks
>will always remain the same. If not, then they are not same IP and most
>probably will not use same driver.
>
>Just to clarify more on your Q, AM3517 also uses 2 clocks
>
>- vpfe_ck (functional clock)
>- vpfe_pck (external clock, pixel clock)
>
>Whereas you are referring them as master and slave clock.
>
>Thanks,
>Vaibhav
>
>
>> Murali
>> >Thanks,
>> >Vaibhav
>> >
>> >> };
>> >>
>> >> static struct platform_device *davinci_evm_devices[] __initdata
>> = {
>> >> diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c
>> >> b/arch/arm/mach-davinci/board-dm644x-evm.c
>> >> index fd0398b..45beb99 100644
>> >> --- a/arch/arm/mach-davinci/board-dm644x-evm.c
>> >> +++ b/arch/arm/mach-davinci/board-dm644x-evm.c
>> >> @@ -250,6 +250,8 @@ static struct vpfe_config vpfe_cfg = {
>> >> .sub_devs = vpfe_sub_devs,
>> >> .card_name = "DM6446 EVM",
>> >> .ccdc = "DM6446 CCDC",
>> >> + .num_clocks = 2,
>> >> + .clocks = {"vpss_master", "vpss_slave"},
>> >> };
>> >>
>> >> static struct platform_device rtc_dev = {
>> >> --
>> >> 1.6.0.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-
>> media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
2009-12-01 17:18 [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable m-karicheri2
2009-12-01 17:19 ` [PATCH - v0 2/2] DaVinci - vpfe capture - Make " m-karicheri2
@ 2009-12-09 16:00 ` Kevin Hilman
2009-12-09 17:16 ` Karicheri, Muralidharan
2009-12-09 17:45 ` Karicheri, Muralidharan
1 sibling, 2 replies; 19+ messages in thread
From: Kevin Hilman @ 2009-12-09 16:00 UTC (permalink / raw)
To: m-karicheri2; +Cc: linux-media, hverkuil, davinci-linux-open-source, hvaibhav
m-karicheri2@ti.com writes:
> From: Muralidharan Karicheri <m-karicheri2@ti.com>
>
> v1 - updated based on comments from Vaibhav Hiremath.
>
> On DM365 we use only vpss master clock, where as on DM355 and
> DM6446, we use vpss master and slave clocks for vpfe capture and AM3517
> we use internal clock and pixel clock. So this patch makes it configurable
> on a per platform basis. This is needed for supporting DM365 for which patches
> will be available soon.
>
> Tested-by: Vaibhav Hiremath <hvaibhav@ti.com>, Muralidharan Karicheri <m-karicheri2@ti.com>
> Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
> Signed-off-by: Muralidharan Karicheri <m-karicheri2@ti.com>
Sorry for jumping late into this thread, but I have a couple comments.
First, we should *never* pass clock names from platform code to driver
code. We have the clkdevs for this. Using clkdev, the right clock
is determined from the driver being used and any additional info.
Rather than passing the strings along, you should add the driver name
to the 'dev_id' field of the clkdev node, and then use the 'con_id' field
to distinguish between the two clocks:
- CLK(NULL, "vpss_master", &vpss_master_clk),
- CLK(NULL, "vpss_slave", &vpss_slave_clk),
+ CLK("vpfe-capture", "master", &vpss_master_clk),
+ CLK("vpfe-capture", "slave", &vpss_slave_clk),
NOTE: this assumes clks are used from VPFE. When you move to CCDC, this
should change accordingly.
More on the usage below...
> ---
> drivers/media/video/davinci/vpfe_capture.c | 98 +++++++++++++++++----------
> include/media/davinci/vpfe_capture.h | 11 ++-
> 2 files changed, 70 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/media/video/davinci/vpfe_capture.c b/drivers/media/video/davinci/vpfe_capture.c
> index 12a1b3d..c3468ee 100644
> --- a/drivers/media/video/davinci/vpfe_capture.c
> +++ b/drivers/media/video/davinci/vpfe_capture.c
> @@ -1787,61 +1787,87 @@ static struct vpfe_device *vpfe_initialize(void)
> return vpfe_dev;
> }
>
> +/**
> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver
> + * @vpfe_dev - ptr to vpfe capture device
> + *
> + * Disables clocks defined in vpfe configuration.
> + */
> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev)
> {
> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
> + int i;
>
> - clk_disable(vpfe_cfg->vpssclk);
> - clk_put(vpfe_cfg->vpssclk);
> - clk_disable(vpfe_cfg->slaveclk);
> - clk_put(vpfe_cfg->slaveclk);
> - v4l2_info(vpfe_dev->pdev->driver,
> - "vpfe vpss master & slave clocks disabled\n");
> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
> + clk_disable(vpfe_dev->clks[i]);
> + clk_put(vpfe_dev->clks[i]);
While cleaning this up, you should move the clk_put() to module
disable/unload time. You dont' need to put he clock on every disable.
The same for clk_get(). You don't need to get the clock for every
enable. Just do a clk_get() at init time.
> + }
> + kfree(vpfe_dev->clks);
> + v4l2_info(vpfe_dev->pdev->driver, "vpfe capture clocks disabled\n");
> }
>
> +/**
> + * vpfe_enable_clock() - Enable clocks for vpfe capture driver
> + * @vpfe_dev - ptr to vpfe capture device
> + *
> + * Enables clocks defined in vpfe configuration. The function
> + * assumes that at least one clock is to be defined which is
> + * true as of now. re-visit this if this assumption is not true
> + */
> static int vpfe_enable_clock(struct vpfe_device *vpfe_dev)
> {
> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
> - int ret = -ENOENT;
> + int i;
>
> - vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "vpss_master");
Using my clkdevs from above, you just need to change this to:
vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "master");
Now that the device name is in the clkdev node, clk_get() will
find the right clock based on the device name.
> - if (NULL == vpfe_cfg->vpssclk) {
> - v4l2_err(vpfe_dev->pdev->driver, "No clock defined for"
> - "vpss_master\n");
> - return ret;
> - }
> + if (!vpfe_cfg->num_clocks)
> + return 0;
>
> - if (clk_enable(vpfe_cfg->vpssclk)) {
> - v4l2_err(vpfe_dev->pdev->driver,
> - "vpfe vpss master clock not enabled\n");
> - goto out;
> - }
> - v4l2_info(vpfe_dev->pdev->driver,
> - "vpfe vpss master clock enabled\n");
> + vpfe_dev->clks = kzalloc(vpfe_cfg->num_clocks *
> + sizeof(struct clock *), GFP_KERNEL);
>
> - vpfe_cfg->slaveclk = clk_get(vpfe_dev->pdev, "vpss_slave");
And here:
vpfe_cfg->slaveclk = clk_get(vpfe_dev->pdev, "slave");
> - if (NULL == vpfe_cfg->slaveclk) {
> - v4l2_err(vpfe_dev->pdev->driver,
> - "No clock defined for vpss slave\n");
> - goto out;
> + if (NULL == vpfe_dev->clks) {
> + v4l2_err(vpfe_dev->pdev->driver, "Memory allocation failed\n");
> + return -ENOMEM;
> }
>
> - if (clk_enable(vpfe_cfg->slaveclk)) {
> - v4l2_err(vpfe_dev->pdev->driver,
> - "vpfe vpss slave clock not enabled\n");
> - goto out;
> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
> + if (NULL == vpfe_cfg->clocks[i]) {
> + v4l2_err(vpfe_dev->pdev->driver,
> + "clock %s is not defined in vpfe config\n",
> + vpfe_cfg->clocks[i]);
> + goto out;
> + }
> +
> + vpfe_dev->clks[i] = clk_get(vpfe_dev->pdev,
> + vpfe_cfg->clocks[i]);
> + if (NULL == vpfe_dev->clks[i]) {
> + v4l2_err(vpfe_dev->pdev->driver,
> + "Failed to get clock %s\n",
> + vpfe_cfg->clocks[i]);
> + goto out;
> + }
> +
> + if (clk_enable(vpfe_dev->clks[i])) {
> + v4l2_err(vpfe_dev->pdev->driver,
> + "vpfe clock %s not enabled\n",
> + vpfe_cfg->clocks[i]);
> + goto out;
> + }
> +
> + v4l2_info(vpfe_dev->pdev->driver, "vpss clock %s enabled",
> + vpfe_cfg->clocks[i]);
> }
> - v4l2_info(vpfe_dev->pdev->driver, "vpfe vpss slave clock enabled\n");
> return 0;
> out:
> - if (vpfe_cfg->vpssclk)
> - clk_put(vpfe_cfg->vpssclk);
> - if (vpfe_cfg->slaveclk)
> - clk_put(vpfe_cfg->slaveclk);
> -
> - return -1;
> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
> + if (vpfe_dev->clks[i])
> + clk_put(vpfe_dev->clks[i]);
> + }
> + kfree(vpfe_dev->clks);
> + return -EFAULT;
> }
>
> +
> /*
> * vpfe_probe : This function creates device entries by register
> * itself to the V4L2 driver and initializes fields of each
> diff --git a/include/media/davinci/vpfe_capture.h b/include/media/davinci/vpfe_capture.h
> index d863e5e..7b62a5c 100644
> --- a/include/media/davinci/vpfe_capture.h
> +++ b/include/media/davinci/vpfe_capture.h
> @@ -31,8 +31,6 @@
> #include <media/videobuf-dma-contig.h>
> #include <media/davinci/vpfe_types.h>
>
> -#define VPFE_CAPTURE_NUM_DECODERS 5
> -
> /* Macros */
> #define VPFE_MAJOR_RELEASE 0
> #define VPFE_MINOR_RELEASE 0
> @@ -91,9 +89,14 @@ struct vpfe_config {
> char *card_name;
> /* ccdc name */
> char *ccdc;
> - /* vpfe clock */
> + /* vpfe clock. Obsolete! Will be removed in next patch */
I'd drop these comment additions and just comment in the follow up patch
why they were removed.
> struct clk *vpssclk;
> + /* Obsolete! Will be removed in next patch */
> struct clk *slaveclk;
> + /* number of clocks */
> + int num_clocks;
> + /* clocks used for vpfe capture */
> + char *clocks[];
> };
>
> struct vpfe_device {
> @@ -104,6 +107,8 @@ struct vpfe_device {
> struct v4l2_subdev **sd;
> /* vpfe cfg */
> struct vpfe_config *cfg;
> + /* clock ptrs for vpfe capture */
> + struct clk **clks;
> /* V4l2 device */
> struct v4l2_device v4l2_dev;
> /* parent device */
> --
> 1.6.0.4
Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
2009-12-09 16:00 ` [PATCH - v1 1/2] V4L - vpfe capture - make " Kevin Hilman
@ 2009-12-09 17:16 ` Karicheri, Muralidharan
2009-12-09 17:45 ` Karicheri, Muralidharan
1 sibling, 0 replies; 19+ messages in thread
From: Karicheri, Muralidharan @ 2009-12-09 17:16 UTC (permalink / raw)
To: Kevin Hilman
Cc: linux-media, hverkuil, davinci-linux-open-source, Hiremath, Vaibhav
Kevin,
After sending out my last patch (moving clocks to ccdc driver), I thought of reworking it in similar lines. I will re-send the patch after incorporating
this.
Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
phone: 301-407-9583
email: m-karicheri2@ti.com
>-----Original Message-----
>From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>Sent: Wednesday, December 09, 2009 11:00 AM
>To: Karicheri, Muralidharan
>Cc: linux-media@vger.kernel.org; hverkuil@xs4all.nl; davinci-linux-open-
>source@linux.davincidsp.com; Hiremath, Vaibhav
>Subject: Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
>
>m-karicheri2@ti.com writes:
>
>> From: Muralidharan Karicheri <m-karicheri2@ti.com>
>>
>> v1 - updated based on comments from Vaibhav Hiremath.
>>
>> On DM365 we use only vpss master clock, where as on DM355 and
>> DM6446, we use vpss master and slave clocks for vpfe capture and AM3517
>> we use internal clock and pixel clock. So this patch makes it
>configurable
>> on a per platform basis. This is needed for supporting DM365 for which
>patches
>> will be available soon.
>>
>> Tested-by: Vaibhav Hiremath <hvaibhav@ti.com>, Muralidharan Karicheri <m-
>karicheri2@ti.com>
>> Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
>> Signed-off-by: Muralidharan Karicheri <m-karicheri2@ti.com>
>
>Sorry for jumping late into this thread, but I have a couple comments.
>
>First, we should *never* pass clock names from platform code to driver
>code. We have the clkdevs for this. Using clkdev, the right clock
>is determined from the driver being used and any additional info.
>
>Rather than passing the strings along, you should add the driver name
>to the 'dev_id' field of the clkdev node, and then use the 'con_id' field
>to distinguish between the two clocks:
>
>- CLK(NULL, "vpss_master", &vpss_master_clk),
>- CLK(NULL, "vpss_slave", &vpss_slave_clk),
>+ CLK("vpfe-capture", "master", &vpss_master_clk),
>+ CLK("vpfe-capture", "slave", &vpss_slave_clk),
>
>NOTE: this assumes clks are used from VPFE. When you move to CCDC, this
>should change accordingly.
>
>More on the usage below...
>
>
>
>> ---
>> drivers/media/video/davinci/vpfe_capture.c | 98 +++++++++++++++++-----
>-----
>> include/media/davinci/vpfe_capture.h | 11 ++-
>> 2 files changed, 70 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/media/video/davinci/vpfe_capture.c
>b/drivers/media/video/davinci/vpfe_capture.c
>> index 12a1b3d..c3468ee 100644
>> --- a/drivers/media/video/davinci/vpfe_capture.c
>> +++ b/drivers/media/video/davinci/vpfe_capture.c
>> @@ -1787,61 +1787,87 @@ static struct vpfe_device *vpfe_initialize(void)
>> return vpfe_dev;
>> }
>>
>> +/**
>> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver
>> + * @vpfe_dev - ptr to vpfe capture device
>> + *
>> + * Disables clocks defined in vpfe configuration.
>> + */
>> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev)
>> {
>> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
>> + int i;
>>
>> - clk_disable(vpfe_cfg->vpssclk);
>> - clk_put(vpfe_cfg->vpssclk);
>> - clk_disable(vpfe_cfg->slaveclk);
>> - clk_put(vpfe_cfg->slaveclk);
>> - v4l2_info(vpfe_dev->pdev->driver,
>> - "vpfe vpss master & slave clocks disabled\n");
>> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
>> + clk_disable(vpfe_dev->clks[i]);
>> + clk_put(vpfe_dev->clks[i]);
>
>While cleaning this up, you should move the clk_put() to module
>disable/unload time. You dont' need to put he clock on every disable.
>The same for clk_get(). You don't need to get the clock for every
>enable. Just do a clk_get() at init time.
>
>> + }
>> + kfree(vpfe_dev->clks);
>> + v4l2_info(vpfe_dev->pdev->driver, "vpfe capture clocks disabled\n");
>> }
>>
>> +/**
>> + * vpfe_enable_clock() - Enable clocks for vpfe capture driver
>> + * @vpfe_dev - ptr to vpfe capture device
>> + *
>> + * Enables clocks defined in vpfe configuration. The function
>> + * assumes that at least one clock is to be defined which is
>> + * true as of now. re-visit this if this assumption is not true
>> + */
>> static int vpfe_enable_clock(struct vpfe_device *vpfe_dev)
>> {
>> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
>> - int ret = -ENOENT;
>> + int i;
>>
>> - vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "vpss_master");
>
>Using my clkdevs from above, you just need to change this to:
>
> vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "master");
>
>Now that the device name is in the clkdev node, clk_get() will
>find the right clock based on the device name.
>
>> - if (NULL == vpfe_cfg->vpssclk) {
>> - v4l2_err(vpfe_dev->pdev->driver, "No clock defined for"
>> - "vpss_master\n");
>> - return ret;
>> - }
>
>> + if (!vpfe_cfg->num_clocks)
>> + return 0;
>>
>> - if (clk_enable(vpfe_cfg->vpssclk)) {
>> - v4l2_err(vpfe_dev->pdev->driver,
>> - "vpfe vpss master clock not enabled\n");
>> - goto out;
>> - }
>> - v4l2_info(vpfe_dev->pdev->driver,
>> - "vpfe vpss master clock enabled\n");
>> + vpfe_dev->clks = kzalloc(vpfe_cfg->num_clocks *
>> + sizeof(struct clock *), GFP_KERNEL);
>>
>> - vpfe_cfg->slaveclk = clk_get(vpfe_dev->pdev, "vpss_slave");
>
>And here:
>
> vpfe_cfg->slaveclk = clk_get(vpfe_dev->pdev, "slave");
>
>> - if (NULL == vpfe_cfg->slaveclk) {
>> - v4l2_err(vpfe_dev->pdev->driver,
>> - "No clock defined for vpss slave\n");
>> - goto out;
>> + if (NULL == vpfe_dev->clks) {
>> + v4l2_err(vpfe_dev->pdev->driver, "Memory allocation failed\n");
>> + return -ENOMEM;
>> }
>>
>> - if (clk_enable(vpfe_cfg->slaveclk)) {
>> - v4l2_err(vpfe_dev->pdev->driver,
>> - "vpfe vpss slave clock not enabled\n");
>> - goto out;
>> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
>> + if (NULL == vpfe_cfg->clocks[i]) {
>> + v4l2_err(vpfe_dev->pdev->driver,
>> + "clock %s is not defined in vpfe config\n",
>> + vpfe_cfg->clocks[i]);
>> + goto out;
>> + }
>> +
>> + vpfe_dev->clks[i] = clk_get(vpfe_dev->pdev,
>> + vpfe_cfg->clocks[i]);
>> + if (NULL == vpfe_dev->clks[i]) {
>> + v4l2_err(vpfe_dev->pdev->driver,
>> + "Failed to get clock %s\n",
>> + vpfe_cfg->clocks[i]);
>> + goto out;
>> + }
>> +
>> + if (clk_enable(vpfe_dev->clks[i])) {
>> + v4l2_err(vpfe_dev->pdev->driver,
>> + "vpfe clock %s not enabled\n",
>> + vpfe_cfg->clocks[i]);
>> + goto out;
>> + }
>> +
>> + v4l2_info(vpfe_dev->pdev->driver, "vpss clock %s enabled",
>> + vpfe_cfg->clocks[i]);
>> }
>> - v4l2_info(vpfe_dev->pdev->driver, "vpfe vpss slave clock enabled\n");
>> return 0;
>> out:
>> - if (vpfe_cfg->vpssclk)
>> - clk_put(vpfe_cfg->vpssclk);
>> - if (vpfe_cfg->slaveclk)
>> - clk_put(vpfe_cfg->slaveclk);
>> -
>> - return -1;
>> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
>> + if (vpfe_dev->clks[i])
>> + clk_put(vpfe_dev->clks[i]);
>> + }
>> + kfree(vpfe_dev->clks);
>> + return -EFAULT;
>> }
>>
>> +
>> /*
>> * vpfe_probe : This function creates device entries by register
>> * itself to the V4L2 driver and initializes fields of each
>> diff --git a/include/media/davinci/vpfe_capture.h
>b/include/media/davinci/vpfe_capture.h
>> index d863e5e..7b62a5c 100644
>> --- a/include/media/davinci/vpfe_capture.h
>> +++ b/include/media/davinci/vpfe_capture.h
>> @@ -31,8 +31,6 @@
>> #include <media/videobuf-dma-contig.h>
>> #include <media/davinci/vpfe_types.h>
>>
>> -#define VPFE_CAPTURE_NUM_DECODERS 5
>> -
>> /* Macros */
>> #define VPFE_MAJOR_RELEASE 0
>> #define VPFE_MINOR_RELEASE 0
>> @@ -91,9 +89,14 @@ struct vpfe_config {
>> char *card_name;
>> /* ccdc name */
>> char *ccdc;
>> - /* vpfe clock */
>> + /* vpfe clock. Obsolete! Will be removed in next patch */
>
>I'd drop these comment additions and just comment in the follow up patch
>why they were removed.
>
>> struct clk *vpssclk;
>> + /* Obsolete! Will be removed in next patch */
>> struct clk *slaveclk;
>> + /* number of clocks */
>> + int num_clocks;
>> + /* clocks used for vpfe capture */
>> + char *clocks[];
>> };
>>
>> struct vpfe_device {
>> @@ -104,6 +107,8 @@ struct vpfe_device {
>> struct v4l2_subdev **sd;
>> /* vpfe cfg */
>> struct vpfe_config *cfg;
>> + /* clock ptrs for vpfe capture */
>> + struct clk **clks;
>> /* V4l2 device */
>> struct v4l2_device v4l2_dev;
>> /* parent device */
>> --
>> 1.6.0.4
>
>Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
2009-12-09 16:00 ` [PATCH - v1 1/2] V4L - vpfe capture - make " Kevin Hilman
2009-12-09 17:16 ` Karicheri, Muralidharan
@ 2009-12-09 17:45 ` Karicheri, Muralidharan
2009-12-09 18:21 ` Karicheri, Muralidharan
2009-12-10 19:02 ` Kevin Hilman
1 sibling, 2 replies; 19+ messages in thread
From: Karicheri, Muralidharan @ 2009-12-09 17:45 UTC (permalink / raw)
To: Kevin Hilman
Cc: linux-media, hverkuil, davinci-linux-open-source, Hiremath, Vaibhav
Kevin,
>> +/**
>> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver
>> + * @vpfe_dev - ptr to vpfe capture device
>> + *
>> + * Disables clocks defined in vpfe configuration.
>> + */
>> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev)
>> {
>> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
>> + int i;
>>
>> - clk_disable(vpfe_cfg->vpssclk);
>> - clk_put(vpfe_cfg->vpssclk);
>> - clk_disable(vpfe_cfg->slaveclk);
>> - clk_put(vpfe_cfg->slaveclk);
>> - v4l2_info(vpfe_dev->pdev->driver,
>> - "vpfe vpss master & slave clocks disabled\n");
>> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
>> + clk_disable(vpfe_dev->clks[i]);
>> + clk_put(vpfe_dev->clks[i]);
>
>While cleaning this up, you should move the clk_put() to module
>disable/unload time.
[MK] vpfe_disable_clock() is called from remove(). In the new
patch, from ccdc driver remove() function, clk_put() will be called.
Why do you think it should be moved to exit() function of the module?
>You dont' need to put he clock on every disable.
>The same for clk_get(). You don't need to get the clock for every
>enable. Just do a clk_get() at init time.
Are you suggesting to call clk_get() during init() and call clk_put()
from exit()? What is wrong with calling clk_get() from probe()?
I thought following is correct:-
Probe()
clk_get() followed by clk_enable()
Remove()
clk_disable() followed by clk_put()
Suspend()
clk_disable()
Resume()
clk_enable()
Please confirm.
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
2009-12-09 17:45 ` Karicheri, Muralidharan
@ 2009-12-09 18:21 ` Karicheri, Muralidharan
2009-12-09 20:33 ` Karicheri, Muralidharan
2009-12-10 19:02 ` Kevin Hilman
1 sibling, 1 reply; 19+ messages in thread
From: Karicheri, Muralidharan @ 2009-12-09 18:21 UTC (permalink / raw)
To: Karicheri, Muralidharan, Kevin Hilman
Cc: davinci-linux-open-source, linux-media
Kevin,
I tried the following and I get error in clk_enable(). Do you know what might be wrong?
in DM365.c
CLK("isif", "master", &vpss_master_clk)
The driver name is isif. I call clk_get(&pdev->dev, "master") from isif driver. The platform device name is "isif". This call succeeds, but clk_enable() fails...
clk_ptr = clk_get(&pdev->dev, "master");
clk_enable(clk_ptr);
root@dm355-evm:~# cat /proc/davinci_clocks
ref_clk users= 7 24000000 Hz
pll1 users= 6 pll 486000000 Hz
pll1_aux_clk users= 3 pll 24000000 Hz
uart0 users= 1 psc 24000000 Hz
i2c users= 1 psc 24000000 Hz
spi4 users= 0 psc 24000000 Hz
pwm0 users= 0 psc 24000000 Hz
pwm1 users= 0 psc 24000000 Hz
pwm2 users= 0 psc 24000000 Hz
timer0 users= 1 psc 24000000 Hz
timer1 users= 0 psc 24000000 Hz
timer2 users= 1 psc 24000000 Hz
timer3 users= 0 psc 24000000 Hz
usb users= 0 psc 24000000 Hz
pll1_sysclkbp users= 0 pll 24000000 Hz
clkout0 users= 0 pll 24000000 Hz
pll1_sysclk1 users= 0 pll 486000000 Hz
pll1_sysclk2 users= 0 pll 243000000 Hz
pll1_sysclk3 users= 0 pll 243000000 Hz
vpss_dac users= 0 psc 243000000 Hz
mjcp users= 0 psc 243000000 Hz
pll1_sysclk4 users= 3 pll 121500000 Hz
uart1 users= 0 psc 121500000 Hz
mmcsd1 users= 0 psc 121500000 Hz
spi0 users= 0 psc 121500000 Hz
spi1 users= 0 psc 121500000 Hz
spi2 users= 0 psc 121500000 Hz
spi3 users= 0 psc 121500000 Hz
gpio users= 1 psc 121500000 Hz
aemif users= 1 psc 121500000 Hz
emac users= 1 psc 121500000 Hz
asp0 users= 0 psc 121500000 Hz
rto users= 0 psc 121500000 Hz
pll1_sysclk5 users= 0 pll 243000000 Hz
vpss_master users= 0 psc 243000000 Hz
pll1_sysclk6 users= 0 pll 27000000 Hz
pll1_sysclk7 users= 0 pll 486000000 Hz
pll1_sysclk8 users= 0 pll 121500000 Hz
mmcsd0 users= 0 psc 121500000 Hz
pll1_sysclk9 users= 0 pll 243000000 Hz
pll2 users= 1 pll 594000000 Hz
pll2_aux_clk users= 0 pll 24000000 Hz
clkout1 users= 0 pll 24000000 Hz
pll2_sysclk1 users= 0 pll 594000000 Hz
pll2_sysclk2 users= 1 pll 297000000 Hz
arm_clk users= 1 psc 297000000 Hz
pll2_sysclk3 users= 0 pll 594000000 Hz
pll2_sysclk4 users= 0 pll 20482758 Hz
voice_codec users= 0 psc 20482758 Hz
pll2_sysclk5 users= 0 pll 74250000 Hz
pll2_sysclk6 users= 0 pll 594000000 Hz
pll2_sysclk7 users= 0 pll 594000000 Hz
pll2_sysclk8 users= 0 pll 594000000 Hz
pll2_sysclk9 users= 0 pll 594000000 Hz
pwm3 users= 0 psc 24000000 Hz
root@dm355-evm:~#
Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
phone: 301-407-9583
email: m-karicheri2@ti.com
>-----Original Message-----
>From: davinci-linux-open-source-bounces@linux.davincidsp.com
>[mailto:davinci-linux-open-source-bounces@linux.davincidsp.com] On Behalf
>Of Karicheri, Muralidharan
>Sent: Wednesday, December 09, 2009 12:45 PM
>To: Kevin Hilman
>Cc: davinci-linux-open-source@linux.davincidsp.com; linux-
>media@vger.kernel.org
>Subject: RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
>
>Kevin,
>
>>> +/**
>>> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver
>>> + * @vpfe_dev - ptr to vpfe capture device
>>> + *
>>> + * Disables clocks defined in vpfe configuration.
>>> + */
>>> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev)
>>> {
>>> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
>>> + int i;
>>>
>>> - clk_disable(vpfe_cfg->vpssclk);
>>> - clk_put(vpfe_cfg->vpssclk);
>>> - clk_disable(vpfe_cfg->slaveclk);
>>> - clk_put(vpfe_cfg->slaveclk);
>>> - v4l2_info(vpfe_dev->pdev->driver,
>>> - "vpfe vpss master & slave clocks disabled\n");
>>> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
>>> + clk_disable(vpfe_dev->clks[i]);
>>> + clk_put(vpfe_dev->clks[i]);
>>
>>While cleaning this up, you should move the clk_put() to module
>>disable/unload time.
>
>[MK] vpfe_disable_clock() is called from remove(). In the new
>patch, from ccdc driver remove() function, clk_put() will be called.
>Why do you think it should be moved to exit() function of the module?
>
>>You dont' need to put he clock on every disable.
>>The same for clk_get(). You don't need to get the clock for every
>>enable. Just do a clk_get() at init time.
>
>Are you suggesting to call clk_get() during init() and call clk_put()
>from exit()? What is wrong with calling clk_get() from probe()?
>I thought following is correct:-
>Probe()
>clk_get() followed by clk_enable()
>Remove()
>clk_disable() followed by clk_put()
>Suspend()
>clk_disable()
>Resume()
>clk_enable()
>Please confirm.
>_______________________________________________
>Davinci-linux-open-source mailing list
>Davinci-linux-open-source@linux.davincidsp.com
>http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
2009-12-09 18:21 ` Karicheri, Muralidharan
@ 2009-12-09 20:33 ` Karicheri, Muralidharan
2009-12-10 19:06 ` Kevin Hilman
0 siblings, 1 reply; 19+ messages in thread
From: Karicheri, Muralidharan @ 2009-12-09 20:33 UTC (permalink / raw)
To: Karicheri, Muralidharan, Kevin Hilman
Cc: davinci-linux-open-source, linux-media
Kevin,
I think I have figured it out...
First issue was that I was adding my entry at the end of dm644x_clks[]
array. I need to add it before the CLK(NULL, NULL, NULL)
secondly, your suggestion didn't work as is. This is what I had to
do to get it working...
static struct clk ccdc_master_clk = {
.name = "dm644x_ccdc",
.parent = &vpss_master_clk,
};
static struct clk ccdc_slave_clk = {
.name = "dm644x_ccdc",
.parent = &vpss_slave_clk,
};
static struct davinci_clk dm365_clks = {
....
....
CLK("dm644x_ccdc", "master", &ccdc_master_clk),
CLK("dm644x_ccdc", "slave", &ccdc_slave_clk),
CLK(NULL, NULL, NULL);
Let me know if you think there is anything wrong with the above scheme.
Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
phone: 301-407-9583
email: m-karicheri2@ti.com
>-----Original Message-----
>From: Karicheri, Muralidharan
>Sent: Wednesday, December 09, 2009 1:22 PM
>To: Karicheri, Muralidharan; Kevin Hilman
>Cc: davinci-linux-open-source@linux.davincidsp.com; linux-
>media@vger.kernel.org
>Subject: RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
>
>Kevin,
>
>I tried the following and I get error in clk_enable(). Do you know what
>might be wrong?
>
>in DM365.c
>
>CLK("isif", "master", &vpss_master_clk)
>
>The driver name is isif. I call clk_get(&pdev->dev, "master") from isif
>driver. The platform device name is "isif". This call succeeds, but
>clk_enable() fails...
>
>clk_ptr = clk_get(&pdev->dev, "master");
>clk_enable(clk_ptr);
>
>root@dm355-evm:~# cat /proc/davinci_clocks
>ref_clk users= 7 24000000 Hz
> pll1 users= 6 pll 486000000 Hz
> pll1_aux_clk users= 3 pll 24000000 Hz
> uart0 users= 1 psc 24000000 Hz
> i2c users= 1 psc 24000000 Hz
> spi4 users= 0 psc 24000000 Hz
> pwm0 users= 0 psc 24000000 Hz
> pwm1 users= 0 psc 24000000 Hz
> pwm2 users= 0 psc 24000000 Hz
> timer0 users= 1 psc 24000000 Hz
> timer1 users= 0 psc 24000000 Hz
> timer2 users= 1 psc 24000000 Hz
> timer3 users= 0 psc 24000000 Hz
> usb users= 0 psc 24000000 Hz
> pll1_sysclkbp users= 0 pll 24000000 Hz
> clkout0 users= 0 pll 24000000 Hz
> pll1_sysclk1 users= 0 pll 486000000 Hz
> pll1_sysclk2 users= 0 pll 243000000 Hz
> pll1_sysclk3 users= 0 pll 243000000 Hz
> vpss_dac users= 0 psc 243000000 Hz
> mjcp users= 0 psc 243000000 Hz
> pll1_sysclk4 users= 3 pll 121500000 Hz
> uart1 users= 0 psc 121500000 Hz
> mmcsd1 users= 0 psc 121500000 Hz
> spi0 users= 0 psc 121500000 Hz
> spi1 users= 0 psc 121500000 Hz
> spi2 users= 0 psc 121500000 Hz
> spi3 users= 0 psc 121500000 Hz
> gpio users= 1 psc 121500000 Hz
> aemif users= 1 psc 121500000 Hz
> emac users= 1 psc 121500000 Hz
> asp0 users= 0 psc 121500000 Hz
> rto users= 0 psc 121500000 Hz
> pll1_sysclk5 users= 0 pll 243000000 Hz
> vpss_master users= 0 psc 243000000 Hz
> pll1_sysclk6 users= 0 pll 27000000 Hz
> pll1_sysclk7 users= 0 pll 486000000 Hz
> pll1_sysclk8 users= 0 pll 121500000 Hz
> mmcsd0 users= 0 psc 121500000 Hz
> pll1_sysclk9 users= 0 pll 243000000 Hz
> pll2 users= 1 pll 594000000 Hz
> pll2_aux_clk users= 0 pll 24000000 Hz
> clkout1 users= 0 pll 24000000 Hz
> pll2_sysclk1 users= 0 pll 594000000 Hz
> pll2_sysclk2 users= 1 pll 297000000 Hz
> arm_clk users= 1 psc 297000000 Hz
> pll2_sysclk3 users= 0 pll 594000000 Hz
> pll2_sysclk4 users= 0 pll 20482758 Hz
> voice_codec users= 0 psc 20482758 Hz
> pll2_sysclk5 users= 0 pll 74250000 Hz
> pll2_sysclk6 users= 0 pll 594000000 Hz
> pll2_sysclk7 users= 0 pll 594000000 Hz
> pll2_sysclk8 users= 0 pll 594000000 Hz
> pll2_sysclk9 users= 0 pll 594000000 Hz
> pwm3 users= 0 psc 24000000 Hz
>root@dm355-evm:~#
>
>
>
>Murali Karicheri
>Software Design Engineer
>Texas Instruments Inc.
>Germantown, MD 20874
>phone: 301-407-9583
>email: m-karicheri2@ti.com
>
>>-----Original Message-----
>>From: davinci-linux-open-source-bounces@linux.davincidsp.com
>>[mailto:davinci-linux-open-source-bounces@linux.davincidsp.com] On Behalf
>>Of Karicheri, Muralidharan
>>Sent: Wednesday, December 09, 2009 12:45 PM
>>To: Kevin Hilman
>>Cc: davinci-linux-open-source@linux.davincidsp.com; linux-
>>media@vger.kernel.org
>>Subject: RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks
>configurable
>>
>>Kevin,
>>
>>>> +/**
>>>> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver
>>>> + * @vpfe_dev - ptr to vpfe capture device
>>>> + *
>>>> + * Disables clocks defined in vpfe configuration.
>>>> + */
>>>> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev)
>>>> {
>>>> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
>>>> + int i;
>>>>
>>>> - clk_disable(vpfe_cfg->vpssclk);
>>>> - clk_put(vpfe_cfg->vpssclk);
>>>> - clk_disable(vpfe_cfg->slaveclk);
>>>> - clk_put(vpfe_cfg->slaveclk);
>>>> - v4l2_info(vpfe_dev->pdev->driver,
>>>> - "vpfe vpss master & slave clocks disabled\n");
>>>> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
>>>> + clk_disable(vpfe_dev->clks[i]);
>>>> + clk_put(vpfe_dev->clks[i]);
>>>
>>>While cleaning this up, you should move the clk_put() to module
>>>disable/unload time.
>>
>>[MK] vpfe_disable_clock() is called from remove(). In the new
>>patch, from ccdc driver remove() function, clk_put() will be called.
>>Why do you think it should be moved to exit() function of the module?
>>
>>>You dont' need to put he clock on every disable.
>>>The same for clk_get(). You don't need to get the clock for every
>>>enable. Just do a clk_get() at init time.
>>
>>Are you suggesting to call clk_get() during init() and call clk_put()
>>from exit()? What is wrong with calling clk_get() from probe()?
>>I thought following is correct:-
>>Probe()
>>clk_get() followed by clk_enable()
>>Remove()
>>clk_disable() followed by clk_put()
>>Suspend()
>>clk_disable()
>>Resume()
>>clk_enable()
>>Please confirm.
>>_______________________________________________
>>Davinci-linux-open-source mailing list
>>Davinci-linux-open-source@linux.davincidsp.com
>>http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
2009-12-09 17:45 ` Karicheri, Muralidharan
2009-12-09 18:21 ` Karicheri, Muralidharan
@ 2009-12-10 19:02 ` Kevin Hilman
2009-12-10 19:58 ` Karicheri, Muralidharan
1 sibling, 1 reply; 19+ messages in thread
From: Kevin Hilman @ 2009-12-10 19:02 UTC (permalink / raw)
To: Karicheri, Muralidharan
Cc: linux-media, hverkuil, davinci-linux-open-source, Hiremath, Vaibhav
"Karicheri, Muralidharan" <m-karicheri2@ti.com> writes:
> Kevin,
>
>>> +/**
>>> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver
>>> + * @vpfe_dev - ptr to vpfe capture device
>>> + *
>>> + * Disables clocks defined in vpfe configuration.
>>> + */
>>> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev)
>>> {
>>> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
>>> + int i;
>>>
>>> - clk_disable(vpfe_cfg->vpssclk);
>>> - clk_put(vpfe_cfg->vpssclk);
>>> - clk_disable(vpfe_cfg->slaveclk);
>>> - clk_put(vpfe_cfg->slaveclk);
>>> - v4l2_info(vpfe_dev->pdev->driver,
>>> - "vpfe vpss master & slave clocks disabled\n");
>>> + for (i = 0; i < vpfe_cfg->num_clocks; i++) {
>>> + clk_disable(vpfe_dev->clks[i]);
>>> + clk_put(vpfe_dev->clks[i]);
>>
>>While cleaning this up, you should move the clk_put() to module
>>disable/unload time.
>
> [MK] vpfe_disable_clock() is called from remove(). In the new
> patch, from ccdc driver remove() function, clk_put() will be called.
> Why do you think it should be moved to exit() function of the module?
>
>>You dont' need to put he clock on every disable.
>>The same for clk_get(). You don't need to get the clock for every
>>enable. Just do a clk_get() at init time.
>
> Are you suggesting to call clk_get() during init() and call clk_put()
> from exit()? What is wrong with calling clk_get() from probe()?
> I thought following is correct:-
> Probe()
> clk_get() followed by clk_enable()
> Remove()
> clk_disable() followed by clk_put()
> Suspend()
> clk_disable()
> Resume()
> clk_enable()
Yes, that is correct.
I didn't look at the whole driver. My concern was that if the driver
was enhanced to more aggressive clock management, you shouldn't do a
clk_get() every time you do a clk_enable(), same for put.
Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
2009-12-09 20:33 ` Karicheri, Muralidharan
@ 2009-12-10 19:06 ` Kevin Hilman
2009-12-10 20:02 ` Karicheri, Muralidharan
0 siblings, 1 reply; 19+ messages in thread
From: Kevin Hilman @ 2009-12-10 19:06 UTC (permalink / raw)
To: Karicheri, Muralidharan; +Cc: davinci-linux-open-source, linux-media
"Karicheri, Muralidharan" <m-karicheri2@ti.com> writes:
> Kevin,
>
> I think I have figured it out...
>
> First issue was that I was adding my entry at the end of dm644x_clks[]
> array. I need to add it before the CLK(NULL, NULL, NULL)
>
> secondly, your suggestion didn't work as is. This is what I had to
> do to get it working...
>
> static struct clk ccdc_master_clk = {
> .name = "dm644x_ccdc",
> .parent = &vpss_master_clk,
> };
>
> static struct clk ccdc_slave_clk = {
> .name = "dm644x_ccdc",
> .parent = &vpss_slave_clk,
> };
You should not need to add new clocks with new names. I don't thinke
the name field of the struct clk is used anywhere in the matching.
I think it's only used in /proc/davinci_clocks
> static struct davinci_clk dm365_clks = {
> ....
> ....
> CLK("dm644x_ccdc", "master", &ccdc_master_clk),
> CLK("dm644x_ccdc", "slave", &ccdc_slave_clk),
Looks like the drivers name is 'dm644x_ccdc', not 'isif'. I'm
guessing just this should work without having to add new clock names.
CLK("dm644x_ccdc", "master", &vpss_master_clk),
CLK("dm644x_ccdc", "slave", &vpss_slave_clk),
> CLK(NULL, NULL, NULL);
>
> Let me know if you think there is anything wrong with the above scheme.
Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
2009-12-10 19:02 ` Kevin Hilman
@ 2009-12-10 19:58 ` Karicheri, Muralidharan
0 siblings, 0 replies; 19+ messages in thread
From: Karicheri, Muralidharan @ 2009-12-10 19:58 UTC (permalink / raw)
To: Kevin Hilman
Cc: linux-media, hverkuil, davinci-linux-open-source, Hiremath, Vaibhav
>> I thought following is correct:-
>> Probe()
>> clk_get() followed by clk_enable()
>> Remove()
>> clk_disable() followed by clk_put()
>> Suspend()
>> clk_disable()
>> Resume()
>> clk_enable()
>
>Yes, that is correct.
>
>I didn't look at the whole driver. My concern was that if the driver
>was enhanced to more aggressive clock management, you shouldn't do a
>clk_get() every time you do a clk_enable(), same for put.
Got you. I am in sync with your thinking.
-Murali
>
>Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
2009-12-10 19:06 ` Kevin Hilman
@ 2009-12-10 20:02 ` Karicheri, Muralidharan
2009-12-11 18:34 ` Kevin Hilman
0 siblings, 1 reply; 19+ messages in thread
From: Karicheri, Muralidharan @ 2009-12-10 20:02 UTC (permalink / raw)
To: Kevin Hilman; +Cc: davinci-linux-open-source, linux-media
>> Kevin,
>>
>> I think I have figured it out...
>>
>> First issue was that I was adding my entry at the end of dm644x_clks[]
>> array. I need to add it before the CLK(NULL, NULL, NULL)
>>
>> secondly, your suggestion didn't work as is. This is what I had to
>> do to get it working...
>>
>> static struct clk ccdc_master_clk = {
>> .name = "dm644x_ccdc",
>> .parent = &vpss_master_clk,
>> };
>>
>> static struct clk ccdc_slave_clk = {
>> .name = "dm644x_ccdc",
>> .parent = &vpss_slave_clk,
>> };
It doesn't work with out doing this. The cat /proc/davinci_clocks hangs with
your suggestion implemented...
>
>You should not need to add new clocks with new names. I don't thinke
>the name field of the struct clk is used anywhere in the matching.
>I think it's only used in /proc/davinci_clocks
>
>> static struct davinci_clk dm365_clks = {
>> ....
>> ....
>> CLK("dm644x_ccdc", "master", &ccdc_master_clk),
>> CLK("dm644x_ccdc", "slave", &ccdc_slave_clk),
>
>Looks like the drivers name is 'dm644x_ccdc', not 'isif'. I'm
>guessing just this should work without having to add new clock names.
>
No. I have mixed up the names. ISIF is for the new ISIF driver on DM365.
Below are for DM644x ccdc driver. With just these entries added, two
things observed....
1) Only one clock is shown disabled (usually many are shown disabled) during bootup
2) cat /proc/davinci_clocks hangs.
So this is the only way I got it working.
>CLK("dm644x_ccdc", "master", &vpss_master_clk),
>CLK("dm644x_ccdc", "slave", &vpss_slave_clk),
>
>> CLK(NULL, NULL, NULL);
>>
>> Let me know if you think there is anything wrong with the above scheme.
>
>Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
2009-12-10 20:02 ` Karicheri, Muralidharan
@ 2009-12-11 18:34 ` Kevin Hilman
2009-12-11 20:48 ` Karicheri, Muralidharan
0 siblings, 1 reply; 19+ messages in thread
From: Kevin Hilman @ 2009-12-11 18:34 UTC (permalink / raw)
To: Karicheri, Muralidharan; +Cc: davinci-linux-open-source, linux-media
"Karicheri, Muralidharan" <m-karicheri2@ti.com> writes:
>>> Kevin,
>>>
>>> I think I have figured it out...
>>>
>>> First issue was that I was adding my entry at the end of dm644x_clks[]
>>> array. I need to add it before the CLK(NULL, NULL, NULL)
>>>
>>> secondly, your suggestion didn't work as is. This is what I had to
>>> do to get it working...
>>>
>>> static struct clk ccdc_master_clk = {
>>> .name = "dm644x_ccdc",
>>> .parent = &vpss_master_clk,
>>> };
>>>
>>> static struct clk ccdc_slave_clk = {
>>> .name = "dm644x_ccdc",
>>> .parent = &vpss_slave_clk,
>>> };
>
> It doesn't work with out doing this. The cat /proc/davinci_clocks hangs with
> your suggestion implemented...
Can you track down the hang. It sounds like a bug in the walking of
the clock tree for davinci_clocks.
>>
>>You should not need to add new clocks with new names. I don't thinke
>>the name field of the struct clk is used anywhere in the matching.
>>I think it's only used in /proc/davinci_clocks
>>
>>> static struct davinci_clk dm365_clks = {
>>> ....
>>> ....
>>> CLK("dm644x_ccdc", "master", &ccdc_master_clk),
>>> CLK("dm644x_ccdc", "slave", &ccdc_slave_clk),
>>
>>Looks like the drivers name is 'dm644x_ccdc', not 'isif'. I'm
>>guessing just this should work without having to add new clock names.
>>
> No. I have mixed up the names. ISIF is for the new ISIF driver on DM365.
> Below are for DM644x ccdc driver. With just these entries added, two
> things observed....
>
> 1) Only one clock is shown disabled (usually many are shown disabled) during bootup
> 2) cat /proc/davinci_clocks hangs.
>
> So this is the only way I got it working.
Hmm, it worked just fine for me without any of these side effects. I
applied the simple patch below on top of current master branch. It booted
fine showing all the unused clocks being disabled, and I was able to
see davinci_clocks just fine:
diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-davinci/dm644x.c
index e65e29e..e6f3570 100644
--- a/arch/arm/mach-davinci/dm644x.c
+++ b/arch/arm/mach-davinci/dm644x.c
@@ -293,8 +293,8 @@ struct davinci_clk dm644x_clks[] = {
CLK(NULL, "dsp", &dsp_clk),
CLK(NULL, "arm", &arm_clk),
CLK(NULL, "vicp", &vicp_clk),
- CLK(NULL, "vpss_master", &vpss_master_clk),
- CLK(NULL, "vpss_slave", &vpss_slave_clk),
+ CLK("dm644x_ccdc", "master", &vpss_master_clk),
+ CLK("dm644x_ccdc", "slave", &vpss_slave_clk),
CLK(NULL, "arm", &arm_clk),
CLK(NULL, "uart0", &uart0_clk),
CLK(NULL, "uart1", &uart1_clk),
[...]
Clocks: disable unused uart1
Clocks: disable unused uart2
Clocks: disable unused emac
Clocks: disable unused ide
Clocks: disable unused asp0
Clocks: disable unused mmcsd
Clocks: disable unused spi
Clocks: disable unused usb
Clocks: disable unused vlynq
Clocks: disable unused pwm0
Clocks: disable unused pwm1
Clocks: disable unused pwm2
Clocks: disable unused timer1
[...]
root@DM644x:~# uname -r
2.6.32-arm-davinci-default-06873-g1a7277b-dirty
root@DM644x:~# cat /debug/davinci_clocks
ref_clk users= 8 27000000 Hz
pll1 users= 8 pll 594000000 Hz
pll1_sysclk1 users= 0 pll 594000000 Hz
dsp users= 1 psc 594000000 Hz
pll1_sysclk2 users= 2 pll 297000000 Hz
arm users= 2 psc 297000000 Hz
pll1_sysclk3 users= 0 pll 198000000 Hz
vpss_master users= 0 psc 198000000 Hz
vpss_slave users= 0 psc 198000000 Hz
pll1_sysclk5 users= 3 pll 99000000 Hz
emac users= 1 psc 99000000 Hz
ide users= 0 psc 99000000 Hz
asp0 users= 0 psc 99000000 Hz
mmcsd users= 0 psc 99000000 Hz
spi users= 0 psc 99000000 Hz
gpio users= 1 psc 99000000 Hz
usb users= 0 psc 99000000 Hz
vlynq users= 0 psc 99000000 Hz
aemif users= 1 psc 99000000 Hz
pll1_aux_clk users= 3 pll 27000000 Hz
uart0 users= 1 psc 27000000 Hz
uart1 users= 0 psc 27000000 Hz
uart2 users= 0 psc 27000000 Hz
i2c users= 1 psc 27000000 Hz
pwm0 users= 0 psc 27000000 Hz
pwm1 users= 0 psc 27000000 Hz
pwm2 users= 0 psc 27000000 Hz
timer0 users= 1 psc 27000000 Hz
timer1 users= 0 psc 27000000 Hz
timer2 users= 1 psc 27000000 Hz
pll1_sysclkbp users= 0 pll 27000000 Hz
pll2 users= 0 pll 648000000 Hz
pll2_sysclk1 users= 0 pll 54000000 Hz
pll2_sysclk2 users= 0 pll 324000000 Hz
pll2_sysclkbp users= 0 pll 13500000 Hz
root@DM644x:~#
^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
2009-12-11 18:34 ` Kevin Hilman
@ 2009-12-11 20:48 ` Karicheri, Muralidharan
0 siblings, 0 replies; 19+ messages in thread
From: Karicheri, Muralidharan @ 2009-12-11 20:48 UTC (permalink / raw)
To: Kevin Hilman; +Cc: davinci-linux-open-source, linux-media
Kevin,
That is what I had seen. I will check it again on Monday
and let you know.
Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
phone: 301-407-9583
email: m-karicheri2@ti.com
>-----Original Message-----
>From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>Sent: Friday, December 11, 2009 1:35 PM
>To: Karicheri, Muralidharan
>Cc: davinci-linux-open-source@linux.davincidsp.com; linux-
>media@vger.kernel.org
>Subject: Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
>
>"Karicheri, Muralidharan" <m-karicheri2@ti.com> writes:
>
>>>> Kevin,
>>>>
>>>> I think I have figured it out...
>>>>
>>>> First issue was that I was adding my entry at the end of dm644x_clks[]
>>>> array. I need to add it before the CLK(NULL, NULL, NULL)
>>>>
>>>> secondly, your suggestion didn't work as is. This is what I had to
>>>> do to get it working...
>>>>
>>>> static struct clk ccdc_master_clk = {
>>>> .name = "dm644x_ccdc",
>>>> .parent = &vpss_master_clk,
>>>> };
>>>>
>>>> static struct clk ccdc_slave_clk = {
>>>> .name = "dm644x_ccdc",
>>>> .parent = &vpss_slave_clk,
>>>> };
>>
>> It doesn't work with out doing this. The cat /proc/davinci_clocks hangs
>with
>> your suggestion implemented...
>
>Can you track down the hang. It sounds like a bug in the walking of
>the clock tree for davinci_clocks.
>
>>>
>>>You should not need to add new clocks with new names. I don't thinke
>>>the name field of the struct clk is used anywhere in the matching.
>>>I think it's only used in /proc/davinci_clocks
>>>
>>>> static struct davinci_clk dm365_clks = {
>>>> ....
>>>> ....
>>>> CLK("dm644x_ccdc", "master", &ccdc_master_clk),
>>>> CLK("dm644x_ccdc", "slave", &ccdc_slave_clk),
>>>
>>>Looks like the drivers name is 'dm644x_ccdc', not 'isif'. I'm
>>>guessing just this should work without having to add new clock names.
>>>
>> No. I have mixed up the names. ISIF is for the new ISIF driver on DM365.
>> Below are for DM644x ccdc driver. With just these entries added, two
>> things observed....
>>
>> 1) Only one clock is shown disabled (usually many are shown disabled)
>during bootup
>> 2) cat /proc/davinci_clocks hangs.
>>
>> So this is the only way I got it working.
>
>Hmm, it worked just fine for me without any of these side effects. I
>applied the simple patch below on top of current master branch. It booted
>fine showing all the unused clocks being disabled, and I was able to
>see davinci_clocks just fine:
>
>
>diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-
>davinci/dm644x.c
>index e65e29e..e6f3570 100644
>--- a/arch/arm/mach-davinci/dm644x.c
>+++ b/arch/arm/mach-davinci/dm644x.c
>@@ -293,8 +293,8 @@ struct davinci_clk dm644x_clks[] = {
> CLK(NULL, "dsp", &dsp_clk),
> CLK(NULL, "arm", &arm_clk),
> CLK(NULL, "vicp", &vicp_clk),
>- CLK(NULL, "vpss_master", &vpss_master_clk),
>- CLK(NULL, "vpss_slave", &vpss_slave_clk),
>+ CLK("dm644x_ccdc", "master", &vpss_master_clk),
>+ CLK("dm644x_ccdc", "slave", &vpss_slave_clk),
> CLK(NULL, "arm", &arm_clk),
> CLK(NULL, "uart0", &uart0_clk),
> CLK(NULL, "uart1", &uart1_clk),
>
>
>[...]
>Clocks: disable unused uart1
>Clocks: disable unused uart2
>Clocks: disable unused emac
>Clocks: disable unused ide
>Clocks: disable unused asp0
>Clocks: disable unused mmcsd
>Clocks: disable unused spi
>Clocks: disable unused usb
>Clocks: disable unused vlynq
>Clocks: disable unused pwm0
>Clocks: disable unused pwm1
>Clocks: disable unused pwm2
>Clocks: disable unused timer1
>[...]
>
>root@DM644x:~# uname -r
>2.6.32-arm-davinci-default-06873-g1a7277b-dirty
>root@DM644x:~# cat /debug/davinci_clocks
>ref_clk users= 8 27000000 Hz
> pll1 users= 8 pll 594000000 Hz
> pll1_sysclk1 users= 0 pll 594000000 Hz
> dsp users= 1 psc 594000000 Hz
> pll1_sysclk2 users= 2 pll 297000000 Hz
> arm users= 2 psc 297000000 Hz
> pll1_sysclk3 users= 0 pll 198000000 Hz
> vpss_master users= 0 psc 198000000 Hz
> vpss_slave users= 0 psc 198000000 Hz
> pll1_sysclk5 users= 3 pll 99000000 Hz
> emac users= 1 psc 99000000 Hz
> ide users= 0 psc 99000000 Hz
> asp0 users= 0 psc 99000000 Hz
> mmcsd users= 0 psc 99000000 Hz
> spi users= 0 psc 99000000 Hz
> gpio users= 1 psc 99000000 Hz
> usb users= 0 psc 99000000 Hz
> vlynq users= 0 psc 99000000 Hz
> aemif users= 1 psc 99000000 Hz
> pll1_aux_clk users= 3 pll 27000000 Hz
> uart0 users= 1 psc 27000000 Hz
> uart1 users= 0 psc 27000000 Hz
> uart2 users= 0 psc 27000000 Hz
> i2c users= 1 psc 27000000 Hz
> pwm0 users= 0 psc 27000000 Hz
> pwm1 users= 0 psc 27000000 Hz
> pwm2 users= 0 psc 27000000 Hz
> timer0 users= 1 psc 27000000 Hz
> timer1 users= 0 psc 27000000 Hz
> timer2 users= 1 psc 27000000 Hz
> pll1_sysclkbp users= 0 pll 27000000 Hz
> pll2 users= 0 pll 648000000 Hz
> pll2_sysclk1 users= 0 pll 54000000 Hz
> pll2_sysclk2 users= 0 pll 324000000 Hz
> pll2_sysclkbp users= 0 pll 13500000 Hz
>root@DM644x:~#
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
@ 2009-12-03 20:22 Karicheri, Muralidharan
0 siblings, 0 replies; 19+ messages in thread
From: Karicheri, Muralidharan @ 2009-12-03 20:22 UTC (permalink / raw)
To: Karicheri, Muralidharan, Hans Verkuil; +Cc: Hiremath, Vaibhav, linux-media
Hans,
Please hold on to this. I will send you an updated patch.
Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
phone: 301-407-9583
email: m-karicheri2@ti.com
>-----Original Message-----
>From: Karicheri, Muralidharan
>Sent: Tuesday, December 01, 2009 12:22 PM
>To: 'Hans Verkuil'
>Cc: Hiremath, Vaibhav; linux-media@vger.kernel.org
>Subject: FW: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
>
>Hans,
>
>Could you please add this to your hg tree and send a pull
>request to Mauro? This was reviewed by Vaibhav and tested on
>DM355, DM6446, AM3517 & DM365. I will request Kevin to pull
>the Architecture part of this patch.
>
>Thanks.
>
>Murali Karicheri
>Software Design Engineer
>Texas Instruments Inc.
>Germantown, MD 20874
>phone: 301-407-9583
>email: m-karicheri2@ti.com
>
>>-----Original Message-----
>>From: Karicheri, Muralidharan
>>Sent: Tuesday, December 01, 2009 12:19 PM
>>To: linux-media@vger.kernel.org; hverkuil@xs4all.nl;
>>khilman@deeprootsystems.com
>>Cc: davinci-linux-open-source@linux.davincidsp.com; Hiremath, Vaibhav;
>>Karicheri, Muralidharan
>>Subject: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
>>
>>From: Muralidharan Karicheri <m-karicheri2@ti.com>
>>
>>v1 - updated based on comments from Vaibhav Hiremath.
>>
>>On DM365 we use only vpss master clock, where as on DM355 and
>>DM6446, we use vpss master and slave clocks for vpfe capture and AM3517
>>we use internal clock and pixel clock. So this patch makes it configurable
>>on a per platform basis. This is needed for supporting DM365 for which
>>patches
>>will be available soon.
>>
>>Tested-by: Vaibhav Hiremath <hvaibhav@ti.com>, Muralidharan Karicheri <m-
>>karicheri2@ti.com>
>>Acked-by: Vaibhav Hiremath <hvaibhav@ti.com>
>>Signed-off-by: Muralidharan Karicheri <m-karicheri2@ti.com>
>>---
>> drivers/media/video/davinci/vpfe_capture.c | 98 +++++++++++++++++------
>-
>>---
>> include/media/davinci/vpfe_capture.h | 11 ++-
>> 2 files changed, 70 insertions(+), 39 deletions(-)
>>
>>diff --git a/drivers/media/video/davinci/vpfe_capture.c
>>b/drivers/media/video/davinci/vpfe_capture.c
>>index 12a1b3d..c3468ee 100644
>>--- a/drivers/media/video/davinci/vpfe_capture.c
>>+++ b/drivers/media/video/davinci/vpfe_capture.c
>>@@ -1787,61 +1787,87 @@ static struct vpfe_device *vpfe_initialize(void)
>> return vpfe_dev;
>> }
>>
>>+/**
>>+ * vpfe_disable_clock() - Disable clocks for vpfe capture driver
>>+ * @vpfe_dev - ptr to vpfe capture device
>>+ *
>>+ * Disables clocks defined in vpfe configuration.
>>+ */
>> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev)
>> {
>> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
>>+ int i;
>>
>>- clk_disable(vpfe_cfg->vpssclk);
>>- clk_put(vpfe_cfg->vpssclk);
>>- clk_disable(vpfe_cfg->slaveclk);
>>- clk_put(vpfe_cfg->slaveclk);
>>- v4l2_info(vpfe_dev->pdev->driver,
>>- "vpfe vpss master & slave clocks disabled\n");
>>+ for (i = 0; i < vpfe_cfg->num_clocks; i++) {
>>+ clk_disable(vpfe_dev->clks[i]);
>>+ clk_put(vpfe_dev->clks[i]);
>>+ }
>>+ kfree(vpfe_dev->clks);
>>+ v4l2_info(vpfe_dev->pdev->driver, "vpfe capture clocks disabled\n");
>> }
>>
>>+/**
>>+ * vpfe_enable_clock() - Enable clocks for vpfe capture driver
>>+ * @vpfe_dev - ptr to vpfe capture device
>>+ *
>>+ * Enables clocks defined in vpfe configuration. The function
>>+ * assumes that at least one clock is to be defined which is
>>+ * true as of now. re-visit this if this assumption is not true
>>+ */
>> static int vpfe_enable_clock(struct vpfe_device *vpfe_dev)
>> {
>> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg;
>>- int ret = -ENOENT;
>>+ int i;
>>
>>- vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "vpss_master");
>>- if (NULL == vpfe_cfg->vpssclk) {
>>- v4l2_err(vpfe_dev->pdev->driver, "No clock defined for"
>>- "vpss_master\n");
>>- return ret;
>>- }
>>+ if (!vpfe_cfg->num_clocks)
>>+ return 0;
>>
>>- if (clk_enable(vpfe_cfg->vpssclk)) {
>>- v4l2_err(vpfe_dev->pdev->driver,
>>- "vpfe vpss master clock not enabled\n");
>>- goto out;
>>- }
>>- v4l2_info(vpfe_dev->pdev->driver,
>>- "vpfe vpss master clock enabled\n");
>>+ vpfe_dev->clks = kzalloc(vpfe_cfg->num_clocks *
>>+ sizeof(struct clock *), GFP_KERNEL);
>>
>>- vpfe_cfg->slaveclk = clk_get(vpfe_dev->pdev, "vpss_slave");
>>- if (NULL == vpfe_cfg->slaveclk) {
>>- v4l2_err(vpfe_dev->pdev->driver,
>>- "No clock defined for vpss slave\n");
>>- goto out;
>>+ if (NULL == vpfe_dev->clks) {
>>+ v4l2_err(vpfe_dev->pdev->driver, "Memory allocation failed\n");
>>+ return -ENOMEM;
>> }
>>
>>- if (clk_enable(vpfe_cfg->slaveclk)) {
>>- v4l2_err(vpfe_dev->pdev->driver,
>>- "vpfe vpss slave clock not enabled\n");
>>- goto out;
>>+ for (i = 0; i < vpfe_cfg->num_clocks; i++) {
>>+ if (NULL == vpfe_cfg->clocks[i]) {
>>+ v4l2_err(vpfe_dev->pdev->driver,
>>+ "clock %s is not defined in vpfe config\n",
>>+ vpfe_cfg->clocks[i]);
>>+ goto out;
>>+ }
>>+
>>+ vpfe_dev->clks[i] = clk_get(vpfe_dev->pdev,
>>+ vpfe_cfg->clocks[i]);
>>+ if (NULL == vpfe_dev->clks[i]) {
>>+ v4l2_err(vpfe_dev->pdev->driver,
>>+ "Failed to get clock %s\n",
>>+ vpfe_cfg->clocks[i]);
>>+ goto out;
>>+ }
>>+
>>+ if (clk_enable(vpfe_dev->clks[i])) {
>>+ v4l2_err(vpfe_dev->pdev->driver,
>>+ "vpfe clock %s not enabled\n",
>>+ vpfe_cfg->clocks[i]);
>>+ goto out;
>>+ }
>>+
>>+ v4l2_info(vpfe_dev->pdev->driver, "vpss clock %s enabled",
>>+ vpfe_cfg->clocks[i]);
>> }
>>- v4l2_info(vpfe_dev->pdev->driver, "vpfe vpss slave clock enabled\n");
>> return 0;
>> out:
>>- if (vpfe_cfg->vpssclk)
>>- clk_put(vpfe_cfg->vpssclk);
>>- if (vpfe_cfg->slaveclk)
>>- clk_put(vpfe_cfg->slaveclk);
>>-
>>- return -1;
>>+ for (i = 0; i < vpfe_cfg->num_clocks; i++) {
>>+ if (vpfe_dev->clks[i])
>>+ clk_put(vpfe_dev->clks[i]);
>>+ }
>>+ kfree(vpfe_dev->clks);
>>+ return -EFAULT;
>> }
>>
>>+
>> /*
>> * vpfe_probe : This function creates device entries by register
>> * itself to the V4L2 driver and initializes fields of each
>>diff --git a/include/media/davinci/vpfe_capture.h
>>b/include/media/davinci/vpfe_capture.h
>>index d863e5e..7b62a5c 100644
>>--- a/include/media/davinci/vpfe_capture.h
>>+++ b/include/media/davinci/vpfe_capture.h
>>@@ -31,8 +31,6 @@
>> #include <media/videobuf-dma-contig.h>
>> #include <media/davinci/vpfe_types.h>
>>
>>-#define VPFE_CAPTURE_NUM_DECODERS 5
>>-
>> /* Macros */
>> #define VPFE_MAJOR_RELEASE 0
>> #define VPFE_MINOR_RELEASE 0
>>@@ -91,9 +89,14 @@ struct vpfe_config {
>> char *card_name;
>> /* ccdc name */
>> char *ccdc;
>>- /* vpfe clock */
>>+ /* vpfe clock. Obsolete! Will be removed in next patch */
>> struct clk *vpssclk;
>>+ /* Obsolete! Will be removed in next patch */
>> struct clk *slaveclk;
>>+ /* number of clocks */
>>+ int num_clocks;
>>+ /* clocks used for vpfe capture */
>>+ char *clocks[];
>> };
>>
>> struct vpfe_device {
>>@@ -104,6 +107,8 @@ struct vpfe_device {
>> struct v4l2_subdev **sd;
>> /* vpfe cfg */
>> struct vpfe_config *cfg;
>>+ /* clock ptrs for vpfe capture */
>>+ struct clk **clks;
>> /* V4l2 device */
>> struct v4l2_device v4l2_dev;
>> /* parent device */
>>--
>>1.6.0.4
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2009-12-11 20:48 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-12-01 17:18 [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable m-karicheri2
2009-12-01 17:19 ` [PATCH - v0 2/2] DaVinci - vpfe capture - Make " m-karicheri2
2009-12-03 6:40 ` Hiremath, Vaibhav
2009-12-03 15:55 ` Karicheri, Muralidharan
2009-12-08 17:27 ` Hiremath, Vaibhav
2009-12-08 20:09 ` Karicheri, Muralidharan
2009-12-04 23:12 ` Karicheri, Muralidharan
2009-12-09 16:00 ` [PATCH - v1 1/2] V4L - vpfe capture - make " Kevin Hilman
2009-12-09 17:16 ` Karicheri, Muralidharan
2009-12-09 17:45 ` Karicheri, Muralidharan
2009-12-09 18:21 ` Karicheri, Muralidharan
2009-12-09 20:33 ` Karicheri, Muralidharan
2009-12-10 19:06 ` Kevin Hilman
2009-12-10 20:02 ` Karicheri, Muralidharan
2009-12-11 18:34 ` Kevin Hilman
2009-12-11 20:48 ` Karicheri, Muralidharan
2009-12-10 19:02 ` Kevin Hilman
2009-12-10 19:58 ` Karicheri, Muralidharan
2009-12-03 20:22 Karicheri, Muralidharan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.