Re: [PATCH 1/2] media: davinci: vpss: enable vpss clocks

From: Prabhakar Lad
Date: Mon Mar 25 2013 - 06:34:07 EST

Sekhar,

On Mon, Mar 25, 2013 at 3:54 PM, Sekhar Nori <nsekhar@xxxxxx> wrote:
> On 3/25/2013 3:44 PM, Prabhakar Lad wrote:
>> Hi Sekhar,
>>
>> Thanks for the review!
>>
>> On Mon, Mar 25, 2013 at 11:02 AM, Sekhar Nori <nsekhar@xxxxxx> wrote:
>>> On 3/22/2013 1:23 PM, Prabhakar lad wrote:
>>>> From: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
>>>>
>>>> By default the VPSS clocks are only enabled in capture driver
>>>> for davinci family which creates duplicates. This
>>>> patch adds support to enable the VPSS clocks in VPSS driver.
>>>> This avoids duplication of code and also adding clock aliases.
>>>> This patch cleanups the VPSS clock enabling in the capture driver,
>>>> and also removes the clock alias in machine file. Along side adds
>>>> a vpss slave clock for DM365 as mentioned by Sekhar
>>>> (https://patchwork.kernel.org/patch/1221261/).
>>>>
>>>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@xxxxxxxxx>
>>>> ---
>>>> arch/arm/mach-davinci/dm355.c | 3 -
>>>> arch/arm/mach-davinci/dm365.c | 9 +++-
>>>> arch/arm/mach-davinci/dm644x.c | 5 --
>>>> drivers/media/platform/davinci/dm355_ccdc.c | 39 +----------------
>>>> drivers/media/platform/davinci/dm644x_ccdc.c | 44 -------------------
>>>> drivers/media/platform/davinci/isif.c | 28 ++----------
>>>> drivers/media/platform/davinci/vpss.c | 60 ++++++++++++++++++++++++++
>>>> 7 files changed, 72 insertions(+), 116 deletions(-)
>>>>
>>>> static struct clk arm_clk = {
>>>> .name = "arm_clk",
>>>> .parent = &pll2_sysclk2,
>>>> @@ -450,6 +456,7 @@ static struct clk_lookup dm365_clks[] = {
>>>> CLK(NULL, "pll2_sysclk9", &pll2_sysclk9),
>>>> CLK(NULL, "vpss_dac", &vpss_dac_clk),
>>>> CLK(NULL, "vpss_master", &vpss_master_clk),
>>>> + CLK(NULL, "vpss_slave", &vpss_slave_clk),
>>>
>>> These should use device name for look-up instead of relying just on
>>> con_id. So the entry should look like:
>>>
>>> CLK("vpss", "slave", &vpss_slave_clk),
>>>
>> OK
>>
>>>> CLK(NULL, "arm", &arm_clk),
>>>> CLK(NULL, "uart0", &uart0_clk),
>>>> CLK(NULL, "uart1", &uart1_clk),
>>>> @@ -1239,8 +1246,6 @@ static int __init dm365_init_devices(void)
>>>> clk_add_alias(NULL, dev_name(&dm365_mdio_device.dev),
>>>> NULL, &dm365_emac_device.dev);
>>>>
>>>> - /* Add isif clock alias */
>>>> - clk_add_alias("master", dm365_isif_dev.name, "vpss_master", NULL);
>>>> platform_device_register(&dm365_vpss_device);
>>>> platform_device_register(&dm365_isif_dev);
>>>> platform_device_register(&vpfe_capture_dev);
>>>> diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-davinci/dm644x.c
>>>> index ee0e994..026e7e3 100644
>>>> --- a/arch/arm/mach-davinci/dm644x.c
>>>> +++ b/arch/arm/mach-davinci/dm644x.c
>>>> @@ -901,11 +901,6 @@ int __init dm644x_init_video(struct vpfe_config *vpfe_cfg,
>>>> dm644x_vpfe_dev.dev.platform_data = vpfe_cfg;
>>>> platform_device_register(&dm644x_ccdc_dev);
>>>> platform_device_register(&dm644x_vpfe_dev);
>>>> - /* Add ccdc clock aliases */
>>>> - clk_add_alias("master", dm644x_ccdc_dev.name,
>>>> - "vpss_master", NULL);
>>>> - clk_add_alias("slave", dm644x_ccdc_dev.name,
>>>> - "vpss_slave", NULL);
>>>> }
>>>>
>>>> if (vpbe_cfg) {
>>>> diff --git a/drivers/media/platform/davinci/dm355_ccdc.c b/drivers/media/platform/davinci/dm355_ccdc.c
>>>> index 2364dba..05f8fb7 100644
>>>> --- a/drivers/media/platform/davinci/dm355_ccdc.c
>>>> +++ b/drivers/media/platform/davinci/dm355_ccdc.c
>>>> @@ -37,7 +37,6 @@
>>>> #include <linux/platform_device.h>
>>>> #include <linux/uaccess.h>
>>>> #include <linux/videodev2.h>
>>>> -#include <linux/clk.h>
>>>> #include <linux/err.h>
>>>> #include <linux/module.h>
>>>>
>>>> @@ -59,10 +58,6 @@ static struct ccdc_oper_config {
>>>> struct ccdc_params_raw bayer;
>>>> /* YCbCr configuration */
>>>> struct ccdc_params_ycbcr ycbcr;
>>>> - /* Master clock */
>>>> - struct clk *mclk;
>>>> - /* slave clock */
>>>> - struct clk *sclk;
>>>> /* ccdc base address */
>>>> void __iomem *base_addr;
>>>> } ccdc_cfg = {
>>>> @@ -997,32 +992,10 @@ static int dm355_ccdc_probe(struct platform_device *pdev)
>>>> goto fail_nomem;
>>>> }
>>>>
>>>> - /* Get and enable Master clock */
>>>> - ccdc_cfg.mclk = clk_get(&pdev->dev, "master");
>>>> - if (IS_ERR(ccdc_cfg.mclk)) {
>>>> - status = PTR_ERR(ccdc_cfg.mclk);
>>>> - goto fail_nomap;
>>>> - }
>>>> - if (clk_prepare_enable(ccdc_cfg.mclk)) {
>>>> - status = -ENODEV;
>>>> - goto fail_mclk;
>>>> - }
>>>> -
>>>> - /* Get and enable Slave clock */
>>>> - ccdc_cfg.sclk = clk_get(&pdev->dev, "slave");
>>>> - if (IS_ERR(ccdc_cfg.sclk)) {
>>>> - status = PTR_ERR(ccdc_cfg.sclk);
>>>> - goto fail_mclk;
>>>> - }
>>>> - if (clk_prepare_enable(ccdc_cfg.sclk)) {
>>>> - status = -ENODEV;
>>>> - goto fail_sclk;
>>>> - }
>>>> -
>>>> /* Platform data holds setup_pinmux function ptr */
>>>> if (NULL == pdev->dev.platform_data) {
>>>> status = -ENODEV;
>>>> - goto fail_sclk;
>>>> + goto fail_nomap;
>>>> }
>>>> setup_pinmux = pdev->dev.platform_data;
>>>> /*
>>>> @@ -1033,12 +1006,6 @@ static int dm355_ccdc_probe(struct platform_device *pdev)
>>>> ccdc_cfg.dev = &pdev->dev;
>>>> printk(KERN_NOTICE "%s is registered with vpfe.\n", ccdc_hw_dev.name);
>>>> return 0;
>>>> -fail_sclk:
>>>> - clk_disable_unprepare(ccdc_cfg.sclk);
>>>> - clk_put(ccdc_cfg.sclk);
>>>> -fail_mclk:
>>>> - clk_disable_unprepare(ccdc_cfg.mclk);
>>>> - clk_put(ccdc_cfg.mclk);
>>>> fail_nomap:
>>>> iounmap(ccdc_cfg.base_addr);
>>>> fail_nomem:
>>>> @@ -1052,10 +1019,6 @@ static int dm355_ccdc_remove(struct platform_device *pdev)
>>>> {
>>>> struct resource *res;
>>>>
>>>> - clk_disable_unprepare(ccdc_cfg.sclk);
>>>> - clk_disable_unprepare(ccdc_cfg.mclk);
>>>> - clk_put(ccdc_cfg.mclk);
>>>> - clk_put(ccdc_cfg.sclk);
>>>> iounmap(ccdc_cfg.base_addr);
>>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> if (res)
>>>> diff --git a/drivers/media/platform/davinci/dm644x_ccdc.c b/drivers/media/platform/davinci/dm644x_ccdc.c
>>>> index 971d639..30fa084 100644
>>>> --- a/drivers/media/platform/davinci/dm644x_ccdc.c
>>>> +++ b/drivers/media/platform/davinci/dm644x_ccdc.c
>>>> @@ -38,7 +38,6 @@
>>>> #include <linux/uaccess.h>
>>>> #include <linux/videodev2.h>
>>>> #include <linux/gfp.h>
>>>> -#include <linux/clk.h>
>>>> #include <linux/err.h>
>>>> #include <linux/module.h>
>>>>
>>>> @@ -60,10 +59,6 @@ static struct ccdc_oper_config {
>>>> struct ccdc_params_raw bayer;
>>>> /* YCbCr configuration */
>>>> struct ccdc_params_ycbcr ycbcr;
>>>> - /* Master clock */
>>>> - struct clk *mclk;
>>>> - /* slave clock */
>>>> - struct clk *sclk;
>>>> /* ccdc base address */
>>>> void __iomem *base_addr;
>>>> } ccdc_cfg = {
>>>> @@ -991,38 +986,9 @@ static int dm644x_ccdc_probe(struct platform_device *pdev)
>>>> goto fail_nomem;
>>>> }
>>>>
>>>> - /* Get and enable Master clock */
>>>> - ccdc_cfg.mclk = clk_get(&pdev->dev, "master");
>>>> - if (IS_ERR(ccdc_cfg.mclk)) {
>>>> - status = PTR_ERR(ccdc_cfg.mclk);
>>>> - goto fail_nomap;
>>>> - }
>>>> - if (clk_prepare_enable(ccdc_cfg.mclk)) {
>>>> - status = -ENODEV;
>>>> - goto fail_mclk;
>>>> - }
>>>> -
>>>> - /* Get and enable Slave clock */
>>>> - ccdc_cfg.sclk = clk_get(&pdev->dev, "slave");
>>>> - if (IS_ERR(ccdc_cfg.sclk)) {
>>>> - status = PTR_ERR(ccdc_cfg.sclk);
>>>> - goto fail_mclk;
>>>> - }
>>>> - if (clk_prepare_enable(ccdc_cfg.sclk)) {
>>>> - status = -ENODEV;
>>>> - goto fail_sclk;
>>>> - }
>>>> ccdc_cfg.dev = &pdev->dev;
>>>> printk(KERN_NOTICE "%s is registered with vpfe.\n", ccdc_hw_dev.name);
>>>> return 0;
>>>> -fail_sclk:
>>>> - clk_disable_unprepare(ccdc_cfg.sclk);
>>>> - clk_put(ccdc_cfg.sclk);
>>>> -fail_mclk:
>>>> - clk_disable_unprepare(ccdc_cfg.mclk);
>>>> - clk_put(ccdc_cfg.mclk);
>>>> -fail_nomap:
>>>> - iounmap(ccdc_cfg.base_addr);
>>>> fail_nomem:
>>>> release_mem_region(res->start, resource_size(res));
>>>> fail_nores:
>>>> @@ -1034,10 +1000,6 @@ static int dm644x_ccdc_remove(struct platform_device *pdev)
>>>> {
>>>> struct resource *res;
>>>>
>>>> - clk_disable_unprepare(ccdc_cfg.mclk);
>>>> - clk_disable_unprepare(ccdc_cfg.sclk);
>>>> - clk_put(ccdc_cfg.mclk);
>>>> - clk_put(ccdc_cfg.sclk);
>>>> iounmap(ccdc_cfg.base_addr);
>>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> if (res)
>>>> @@ -1052,18 +1014,12 @@ static int dm644x_ccdc_suspend(struct device *dev)
>>>> ccdc_save_context();
>>>> /* Disable CCDC */
>>>> ccdc_enable(0);
>>>> - /* Disable both master and slave clock */
>>>> - clk_disable_unprepare(ccdc_cfg.mclk);
>>>> - clk_disable_unprepare(ccdc_cfg.sclk);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> static int dm644x_ccdc_resume(struct device *dev)
>>>> {
>>>> - /* Enable both master and slave clock */
>>>> - clk_prepare_enable(ccdc_cfg.mclk);
>>>> - clk_prepare_enable(ccdc_cfg.sclk);
>>>> /* Restore CCDC context */
>>>> ccdc_restore_context();
>>>>
>>>> diff --git a/drivers/media/platform/davinci/isif.c b/drivers/media/platform/davinci/isif.c
>>>> index abc3ae3..3332cca 100644
>>>> --- a/drivers/media/platform/davinci/isif.c
>>>> +++ b/drivers/media/platform/davinci/isif.c
>>>> @@ -32,7 +32,6 @@
>>>> #include <linux/uaccess.h>
>>>> #include <linux/io.h>
>>>> #include <linux/videodev2.h>
>>>> -#include <linux/clk.h>
>>>> #include <linux/err.h>
>>>> #include <linux/module.h>
>>>>
>>>> @@ -88,8 +87,6 @@ static struct isif_oper_config {
>>>> struct isif_ycbcr_config ycbcr;
>>>> struct isif_params_raw bayer;
>>>> enum isif_data_pack data_pack;
>>>> - /* Master clock */
>>>> - struct clk *mclk;
>>>> /* ISIF base address */
>>>> void __iomem *base_addr;
>>>> /* ISIF Linear Table 0 */
>>>> @@ -1039,6 +1036,10 @@ static int isif_probe(struct platform_device *pdev)
>>>> void *__iomem addr;
>>>> int status = 0, i;
>>>>
>>>> + /* Platform data holds setup_pinmux function ptr */
>>>> + if (!pdev->dev.platform_data)
>>>> + return -ENODEV;
>>>> +
>>>
>>> This change seems unrelated. I suggest moving it to a different patch or
>>> atleast note it in the description.
>>>
>> Its just a movement, while fixing the cleanups. I'll add some
>> description about it.
>
> This is fine as is. I failed to notice the movement.
>
>>
>>>> /*
>>>> * first try to register with vpfe. If not correct platform, then we
>>>> * don't have to iomap
>>>> @@ -1047,22 +1048,6 @@ static int isif_probe(struct platform_device *pdev)
>>>> if (status < 0)
>>>> return status;
>>>>
>>>> - /* Get and enable Master clock */
>>>> - isif_cfg.mclk = clk_get(&pdev->dev, "master");
>>>> - if (IS_ERR(isif_cfg.mclk)) {
>>>> - status = PTR_ERR(isif_cfg.mclk);
>>>> - goto fail_mclk;
>>>> - }
>>>> - if (clk_prepare_enable(isif_cfg.mclk)) {
>>>> - status = -ENODEV;
>>>> - goto fail_mclk;
>>>> - }
>>>> -
>>>> - /* Platform data holds setup_pinmux function ptr */
>>>> - if (NULL == pdev->dev.platform_data) {
>>>> - status = -ENODEV;
>>>> - goto fail_mclk;
>>>> - }
>>>> setup_pinmux = pdev->dev.platform_data;
>>>> /*
>>>> * setup Mux configuration for ccdc which may be different for
>>>> @@ -1124,9 +1109,6 @@ fail_nobase_res:
>>>> release_mem_region(res->start, resource_size(res));
>>>> i--;
>>>> }
>>>> -fail_mclk:
>>>> - clk_disable_unprepare(isif_cfg.mclk);
>>>> - clk_put(isif_cfg.mclk);
>>>> vpfe_unregister_ccdc_device(&isif_hw_dev);
>>>> return status;
>>>> }
>>>> @@ -1146,8 +1128,6 @@ static int isif_remove(struct platform_device *pdev)
>>>> i++;
>>>> }
>>>> vpfe_unregister_ccdc_device(&isif_hw_dev);
>>>> - clk_disable_unprepare(isif_cfg.mclk);
>>>> - clk_put(isif_cfg.mclk);
>>>> return 0;
>>>> }
>>>>
>>>> diff --git a/drivers/media/platform/davinci/vpss.c b/drivers/media/platform/davinci/vpss.c
>>>> index a19c552..db69317 100644
>>>> --- a/drivers/media/platform/davinci/vpss.c
>>>> +++ b/drivers/media/platform/davinci/vpss.c
>>>> @@ -17,6 +17,7 @@
>>>> *
>>>> * common vpss system module platform driver for all video drivers.
>>>> */
>>>> +#include <linux/clk.h>
>>>> #include <linux/kernel.h>
>>>> #include <linux/sched.h>
>>>> #include <linux/init.h>
>>>> @@ -126,6 +127,10 @@ struct vpss_oper_config {
>>>> enum vpss_platform_type platform;
>>>> spinlock_t vpss_lock;
>>>> struct vpss_hw_ops hw_ops;
>>>> + /* Master clock */
>>>> + struct clk *mclk;
>>>> + /* slave clock */
>>>> + struct clk *sclk;
>>>> };
>>>>
>>>> static struct vpss_oper_config oper_cfg;
>>>> @@ -429,6 +434,26 @@ static int vpss_probe(struct platform_device *pdev)
>>>> return -ENODEV;
>>>> }
>>>>
>>>> + /* Get and enable Master clock */
>>>> + oper_cfg.mclk = clk_get(&pdev->dev, "vpss_master");
>>>
>>> use devm_clk_get() here to simplify the error handling.
>>>
>> OK
>>
>>>> + if (IS_ERR(oper_cfg.mclk)) {
>>>> + status = PTR_ERR(oper_cfg.mclk);
>>>> + goto fail_getclk;
>>>> + }
>>>> + status = clk_prepare_enable(oper_cfg.mclk);
>>>> + if (status)
>>>> + goto fail_mclk;
>>>> +
>>>> + /* Get and enable Slave clock */
>>>> + oper_cfg.sclk = clk_get(&pdev->dev, "vpss_slave");
>>>> + if (IS_ERR(oper_cfg.sclk)) {
>>>> + status = PTR_ERR(oper_cfg.sclk);
>>>> + goto fail_mclk;
>>>> + }
>>>> + status = clk_prepare_enable(oper_cfg.sclk);
>>>> + if (status)
>>>> + goto fail_sclk;
>>>> +
>>>> dev_info(&pdev->dev, "%s vpss probed\n", platform_name);
>>>> r1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> if (!r1)
>>>> @@ -500,6 +525,13 @@ fail2:
>>>> iounmap(oper_cfg.vpss_regs_base0);
>>>> fail1:
>>>> release_mem_region(r1->start, resource_size(r1));
>>>> +fail_sclk:
>>>> + clk_disable_unprepare(oper_cfg.sclk);
>>>> + clk_put(oper_cfg.sclk);
>>>> +fail_mclk:
>>>> + clk_disable_unprepare(oper_cfg.mclk);
>>>> + clk_put(oper_cfg.mclk);
>>>> +fail_getclk:
>>>> return status;
>>>> }
>>>>
>>>> @@ -510,6 +542,10 @@ static int vpss_remove(struct platform_device *pdev)
>>>> iounmap(oper_cfg.vpss_regs_base0);
>>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> release_mem_region(res->start, resource_size(res));
>>>> + clk_disable_unprepare(oper_cfg.mclk);
>>>> + clk_disable_unprepare(oper_cfg.sclk);
>>>> + clk_put(oper_cfg.mclk);
>>>> + clk_put(oper_cfg.sclk);
>>>> if (oper_cfg.platform == DM355 || oper_cfg.platform == DM365) {
>>>> iounmap(oper_cfg.vpss_regs_base1);
>>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>> @@ -518,10 +554,34 @@ static int vpss_remove(struct platform_device *pdev)
>>>> return 0;
>>>> }
>>>>
>>>
>>>> +static int vpss_suspend(struct device *dev)
>>>> +{
>>>> + /* Disable both master and slave clock */
>>>> + clk_disable_unprepare(oper_cfg.mclk);
>>>> + clk_disable_unprepare(oper_cfg.sclk);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int vpss_resume(struct device *dev)
>>>> +{
>>>> + /* Enable both master and slave clock */
>>>> + clk_prepare_enable(oper_cfg.mclk);
>>>> + clk_prepare_enable(oper_cfg.sclk);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct dev_pm_ops vpss_pm_ops = {
>>>> + .suspend = vpss_suspend,
>>>> + .resume = vpss_resume,
>>>> +};
>>>
>>> Addition of suspend support seems unrelated to this patch. May be make a
>>> seperate patch for it and while at it, please use PM runtime instead of
>>> direct clock enable/disable. Have a look at the davinci_emac driver
>>> which was converted to use PM runtime recently.
>>>
>> I felt having in same patch would be a good idea, since the clock
>> enabling/disabling
>> where removed from dm644x_ccdc.c for suspend/resume and add it in
>> vpss. If you still
>> feel it needs to be separate patch let me know.
>
> Okay, please add some description for this. Its not terribly obvious.
>
>>
>>> Let me know how you want to handle this patch. I suppose you intend this
>>> should go through my tree because of other dependent platform changes?
>>>
>> I want this series to go through the media tree because of few dependencies
>> (dependency on ths7353 video amplifier driver which recently got
>> merged into media tree)
>
> Hmm, this patch does not touch ths7353 so are you sure there is a merge
> dependency? There are a lot of platform changes you are doing in some of
> the recent patches you submitted based on this patch and ideally those
> go through my tree to ease merging for upstream. That said, if there is
> a good reason for this and the other platform changes to go through
> media tree, I am okay with that too.
>
Yes this patch doesnt touch ths7353, but the machine patches for DM365 which
I posted earlier have a dependency on it. And also I have planned to
take the machine
patches for DM365 and DM355 through media tree itself. [1] this is one
is also of dependent
patch which is already merged into media tree. So to avoid
build/dependency I fell this series and
the machine patches for DM365/DM355 should go through media tree.

[1] http://git.linuxtv.org/media_tree.git/commitdiff/ef2d41b19b8100ce63eabba9ee87953aa685921a

Regards,
--Prabhakar

> Thanks,
> Sekhar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/