[-,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(-)
  

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
--
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
  

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
--
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
  

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.
--
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
  

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
--
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
  

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
--
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
  

"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
--
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
  

"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
--
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
  

>> 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
--
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
  

>> 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
--
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