* [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
@ 2011-09-06 5:56 ` Josh Wu
0 siblings, 0 replies; 16+ messages in thread
From: Josh Wu @ 2011-09-06 5:56 UTC (permalink / raw)
To: g.liakhovetski, linux-media, plagnioj
Cc: linux-kernel, linux-arm-kernel, Josh Wu
This patch enable the configuration for ISI_MCK, which is provided by programmable clock.
Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
drivers/media/video/atmel-isi.c | 60 ++++++++++++++++++++++++++++++++++++++-
include/media/atmel-isi.h | 4 ++
2 files changed, 63 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
index 7b89f00..768bf59 100644
--- a/drivers/media/video/atmel-isi.c
+++ b/drivers/media/video/atmel-isi.c
@@ -90,7 +90,10 @@ struct atmel_isi {
struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
struct completion complete;
+ /* ISI peripherial clock */
struct clk *pclk;
+ /* ISI_MCK, provided by PCK */
+ struct clk *mck;
unsigned int irq;
struct isi_platform_data *pdata;
@@ -763,6 +766,10 @@ static int isi_camera_add_device(struct soc_camera_device *icd)
if (ret)
return ret;
+ ret = clk_enable(isi->mck);
+ if (ret)
+ return ret;
+
isi->icd = icd;
dev_dbg(icd->parent, "Atmel ISI Camera driver attached to camera %d\n",
icd->devnum);
@@ -776,6 +783,7 @@ static void isi_camera_remove_device(struct soc_camera_device *icd)
BUG_ON(icd != isi->icd);
+ clk_disable(isi->mck);
clk_disable(isi->pclk);
isi->icd = NULL;
@@ -882,6 +890,49 @@ static struct soc_camera_host_ops isi_soc_camera_host_ops = {
};
/* -----------------------------------------------------------------------*/
+/* Initialize ISI_MCK clock, called by atmel_isi_probe() function */
+static int initialize_mck(struct platform_device *pdev,
+ struct atmel_isi *isi)
+{
+ struct device *dev = &pdev->dev;
+ struct isi_platform_data *pdata = dev->platform_data;
+ struct clk *pck_parent;
+ int ret;
+
+ if (!strlen(pdata->pck_name) || !strlen(pdata->pck_parent_name))
+ return -EINVAL;
+
+ /* ISI_MCK is provided by PCK clock */
+ isi->mck = clk_get(dev, pdata->pck_name);
+ if (IS_ERR(isi->mck)) {
+ dev_err(dev, "Failed to get PCK: %s\n", pdata->pck_name);
+ return PTR_ERR(isi->mck);
+ }
+
+ pck_parent = clk_get(dev, pdata->pck_parent_name);
+ if (IS_ERR(pck_parent)) {
+ ret = PTR_ERR(pck_parent);
+ dev_err(dev, "Failed to get PCK parent: %s\n",
+ pdata->pck_parent_name);
+ goto err_init_mck;
+ }
+
+ ret = clk_set_parent(isi->mck, pck_parent);
+ clk_put(pck_parent);
+ if (ret)
+ goto err_init_mck;
+
+ ret = clk_set_rate(isi->mck, pdata->isi_mck_hz);
+ if (ret < 0)
+ goto err_init_mck;
+
+ return 0;
+
+err_init_mck:
+ clk_put(isi->mck);
+ return ret;
+}
+
static int __devexit atmel_isi_remove(struct platform_device *pdev)
{
struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
@@ -897,6 +948,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)
isi->fb_descriptors_phys);
iounmap(isi->regs);
+ clk_put(isi->mck);
clk_put(isi->pclk);
kfree(isi);
@@ -915,7 +967,8 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
struct isi_platform_data *pdata;
pdata = dev->platform_data;
- if (!pdata || !pdata->data_width_flags) {
+ if (!pdata || !pdata->data_width_flags || !pdata->isi_mck_hz
+ || !pdata->pck_name || !pdata->pck_parent_name) {
dev_err(&pdev->dev,
"No config available for Atmel ISI\n");
return -EINVAL;
@@ -944,6 +997,11 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
INIT_LIST_HEAD(&isi->video_buffer_list);
INIT_LIST_HEAD(&isi->dma_desc_head);
+ /* Initialize ISI_MCK clock */
+ ret = initialize_mck(pdev, isi);
+ if (ret)
+ goto err_alloc_descriptors;
+
isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
sizeof(struct fbd) * MAX_BUFFER_NUM,
&isi->fb_descriptors_phys,
diff --git a/include/media/atmel-isi.h b/include/media/atmel-isi.h
index 26cece5..dcbb822 100644
--- a/include/media/atmel-isi.h
+++ b/include/media/atmel-isi.h
@@ -114,6 +114,10 @@ struct isi_platform_data {
u32 data_width_flags;
/* Using for ISI_CFG1 */
u32 frate;
+ /* Using for ISI_MCK, provided by PCK */
+ u32 isi_mck_hz;
+ const char *pck_name;
+ const char *pck_parent_name;
};
#endif /* __ATMEL_ISI_H__ */
--
1.6.3.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
@ 2011-09-06 5:56 ` Josh Wu
0 siblings, 0 replies; 16+ messages in thread
From: Josh Wu @ 2011-09-06 5:56 UTC (permalink / raw)
To: linux-arm-kernel
This patch enable the configuration for ISI_MCK, which is provided by programmable clock.
Signed-off-by: Josh Wu <josh.wu@atmel.com>
---
drivers/media/video/atmel-isi.c | 60 ++++++++++++++++++++++++++++++++++++++-
include/media/atmel-isi.h | 4 ++
2 files changed, 63 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
index 7b89f00..768bf59 100644
--- a/drivers/media/video/atmel-isi.c
+++ b/drivers/media/video/atmel-isi.c
@@ -90,7 +90,10 @@ struct atmel_isi {
struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
struct completion complete;
+ /* ISI peripherial clock */
struct clk *pclk;
+ /* ISI_MCK, provided by PCK */
+ struct clk *mck;
unsigned int irq;
struct isi_platform_data *pdata;
@@ -763,6 +766,10 @@ static int isi_camera_add_device(struct soc_camera_device *icd)
if (ret)
return ret;
+ ret = clk_enable(isi->mck);
+ if (ret)
+ return ret;
+
isi->icd = icd;
dev_dbg(icd->parent, "Atmel ISI Camera driver attached to camera %d\n",
icd->devnum);
@@ -776,6 +783,7 @@ static void isi_camera_remove_device(struct soc_camera_device *icd)
BUG_ON(icd != isi->icd);
+ clk_disable(isi->mck);
clk_disable(isi->pclk);
isi->icd = NULL;
@@ -882,6 +890,49 @@ static struct soc_camera_host_ops isi_soc_camera_host_ops = {
};
/* -----------------------------------------------------------------------*/
+/* Initialize ISI_MCK clock, called by atmel_isi_probe() function */
+static int initialize_mck(struct platform_device *pdev,
+ struct atmel_isi *isi)
+{
+ struct device *dev = &pdev->dev;
+ struct isi_platform_data *pdata = dev->platform_data;
+ struct clk *pck_parent;
+ int ret;
+
+ if (!strlen(pdata->pck_name) || !strlen(pdata->pck_parent_name))
+ return -EINVAL;
+
+ /* ISI_MCK is provided by PCK clock */
+ isi->mck = clk_get(dev, pdata->pck_name);
+ if (IS_ERR(isi->mck)) {
+ dev_err(dev, "Failed to get PCK: %s\n", pdata->pck_name);
+ return PTR_ERR(isi->mck);
+ }
+
+ pck_parent = clk_get(dev, pdata->pck_parent_name);
+ if (IS_ERR(pck_parent)) {
+ ret = PTR_ERR(pck_parent);
+ dev_err(dev, "Failed to get PCK parent: %s\n",
+ pdata->pck_parent_name);
+ goto err_init_mck;
+ }
+
+ ret = clk_set_parent(isi->mck, pck_parent);
+ clk_put(pck_parent);
+ if (ret)
+ goto err_init_mck;
+
+ ret = clk_set_rate(isi->mck, pdata->isi_mck_hz);
+ if (ret < 0)
+ goto err_init_mck;
+
+ return 0;
+
+err_init_mck:
+ clk_put(isi->mck);
+ return ret;
+}
+
static int __devexit atmel_isi_remove(struct platform_device *pdev)
{
struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
@@ -897,6 +948,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)
isi->fb_descriptors_phys);
iounmap(isi->regs);
+ clk_put(isi->mck);
clk_put(isi->pclk);
kfree(isi);
@@ -915,7 +967,8 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
struct isi_platform_data *pdata;
pdata = dev->platform_data;
- if (!pdata || !pdata->data_width_flags) {
+ if (!pdata || !pdata->data_width_flags || !pdata->isi_mck_hz
+ || !pdata->pck_name || !pdata->pck_parent_name) {
dev_err(&pdev->dev,
"No config available for Atmel ISI\n");
return -EINVAL;
@@ -944,6 +997,11 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
INIT_LIST_HEAD(&isi->video_buffer_list);
INIT_LIST_HEAD(&isi->dma_desc_head);
+ /* Initialize ISI_MCK clock */
+ ret = initialize_mck(pdev, isi);
+ if (ret)
+ goto err_alloc_descriptors;
+
isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
sizeof(struct fbd) * MAX_BUFFER_NUM,
&isi->fb_descriptors_phys,
diff --git a/include/media/atmel-isi.h b/include/media/atmel-isi.h
index 26cece5..dcbb822 100644
--- a/include/media/atmel-isi.h
+++ b/include/media/atmel-isi.h
@@ -114,6 +114,10 @@ struct isi_platform_data {
u32 data_width_flags;
/* Using for ISI_CFG1 */
u32 frate;
+ /* Using for ISI_MCK, provided by PCK */
+ u32 isi_mck_hz;
+ const char *pck_name;
+ const char *pck_parent_name;
};
#endif /* __ATMEL_ISI_H__ */
--
1.6.3.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
2011-09-06 5:56 ` Josh Wu
@ 2011-09-06 6:54 ` Guennadi Liakhovetski
-1 siblings, 0 replies; 16+ messages in thread
From: Guennadi Liakhovetski @ 2011-09-06 6:54 UTC (permalink / raw)
To: Josh Wu; +Cc: linux-media, plagnioj, linux-kernel, linux-arm-kernel
On Tue, 6 Sep 2011, Josh Wu wrote:
> This patch enable the configuration for ISI_MCK, which is provided by programmable clock.
>
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> drivers/media/video/atmel-isi.c | 60 ++++++++++++++++++++++++++++++++++++++-
> include/media/atmel-isi.h | 4 ++
> 2 files changed, 63 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
> index 7b89f00..768bf59 100644
> --- a/drivers/media/video/atmel-isi.c
> +++ b/drivers/media/video/atmel-isi.c
> @@ -90,7 +90,10 @@ struct atmel_isi {
> struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
>
> struct completion complete;
> + /* ISI peripherial clock */
> struct clk *pclk;
> + /* ISI_MCK, provided by PCK */
> + struct clk *mck;
> unsigned int irq;
>
> struct isi_platform_data *pdata;
> @@ -763,6 +766,10 @@ static int isi_camera_add_device(struct soc_camera_device *icd)
> if (ret)
> return ret;
>
> + ret = clk_enable(isi->mck);
> + if (ret)
> + return ret;
> +
Don't you have to disable the pixel clock (isi->pclk), that you just have
enabled above this hunk, on the above error path?
> isi->icd = icd;
> dev_dbg(icd->parent, "Atmel ISI Camera driver attached to camera %d\n",
> icd->devnum);
> @@ -776,6 +783,7 @@ static void isi_camera_remove_device(struct soc_camera_device *icd)
>
> BUG_ON(icd != isi->icd);
>
> + clk_disable(isi->mck);
> clk_disable(isi->pclk);
> isi->icd = NULL;
>
> @@ -882,6 +890,49 @@ static struct soc_camera_host_ops isi_soc_camera_host_ops = {
> };
>
> /* -----------------------------------------------------------------------*/
> +/* Initialize ISI_MCK clock, called by atmel_isi_probe() function */
> +static int initialize_mck(struct platform_device *pdev,
> + struct atmel_isi *isi)
> +{
> + struct device *dev = &pdev->dev;
> + struct isi_platform_data *pdata = dev->platform_data;
> + struct clk *pck_parent;
> + int ret;
> +
> + if (!strlen(pdata->pck_name) || !strlen(pdata->pck_parent_name))
> + return -EINVAL;
> +
> + /* ISI_MCK is provided by PCK clock */
> + isi->mck = clk_get(dev, pdata->pck_name);
I think, it's still not what Russell meant. Look at
drivers/mmc/host/atmel-mci.c:
host->mck = clk_get(&pdev->dev, "mci_clk");
and in arch/arm/mach-at91/at91sam9g45.c they've got
CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.0", &mmc0_clk),
CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.1", &mmc1_clk),
where
#define CLKDEV_CON_DEV_ID(_con_id, _dev_id, _clk) \
{ \
.con_id = _con_id, \
.dev_id = _dev_id, \
.clk = _clk, \
}
I.e., in the device driver (mmc in this case) you only use the (platform)
device instance, whose dev_name(dev) is then matched against one of clock
lookups above, and a connection ID, which is used in case your device is
using more than one clock. In the ISI case, your pck1 clock, that you seem
to need here, doesn't have a clock lookup object, so, you might have to
add one, and then use its connection ID.
> + if (IS_ERR(isi->mck)) {
> + dev_err(dev, "Failed to get PCK: %s\n", pdata->pck_name);
> + return PTR_ERR(isi->mck);
> + }
> +
> + pck_parent = clk_get(dev, pdata->pck_parent_name);
> + if (IS_ERR(pck_parent)) {
> + ret = PTR_ERR(pck_parent);
> + dev_err(dev, "Failed to get PCK parent: %s\n",
> + pdata->pck_parent_name);
> + goto err_init_mck;
> + }
> +
> + ret = clk_set_parent(isi->mck, pck_parent);
I'm not entirely sure on this one, but as we had a similar situation with
clocks, we decided to extablish the clock hierarchy in the board code, and
only deal with the actual device clocks in the driver itself. I.e., we
moved all clk_set_parent() and setting up the parent clock into the board.
And I do think, this makes more sense, than doing this in the driver, not
all users of this driver will need to manage the parent clock, right?
> + clk_put(pck_parent);
> + if (ret)
> + goto err_init_mck;
> +
> + ret = clk_set_rate(isi->mck, pdata->isi_mck_hz);
> + if (ret < 0)
> + goto err_init_mck;
> +
> + return 0;
> +
> +err_init_mck:
> + clk_put(isi->mck);
> + return ret;
> +}
> +
> static int __devexit atmel_isi_remove(struct platform_device *pdev)
> {
> struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
> @@ -897,6 +948,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)
> isi->fb_descriptors_phys);
>
> iounmap(isi->regs);
> + clk_put(isi->mck);
> clk_put(isi->pclk);
> kfree(isi);
>
> @@ -915,7 +967,8 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
> struct isi_platform_data *pdata;
>
> pdata = dev->platform_data;
> - if (!pdata || !pdata->data_width_flags) {
> + if (!pdata || !pdata->data_width_flags || !pdata->isi_mck_hz
> + || !pdata->pck_name || !pdata->pck_parent_name) {
These names you won't need either.
> dev_err(&pdev->dev,
> "No config available for Atmel ISI\n");
> return -EINVAL;
> @@ -944,6 +997,11 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
> INIT_LIST_HEAD(&isi->video_buffer_list);
> INIT_LIST_HEAD(&isi->dma_desc_head);
>
> + /* Initialize ISI_MCK clock */
> + ret = initialize_mck(pdev, isi);
> + if (ret)
> + goto err_alloc_descriptors;
> +
> isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
> sizeof(struct fbd) * MAX_BUFFER_NUM,
> &isi->fb_descriptors_phys,
> diff --git a/include/media/atmel-isi.h b/include/media/atmel-isi.h
> index 26cece5..dcbb822 100644
> --- a/include/media/atmel-isi.h
> +++ b/include/media/atmel-isi.h
> @@ -114,6 +114,10 @@ struct isi_platform_data {
> u32 data_width_flags;
> /* Using for ISI_CFG1 */
> u32 frate;
> + /* Using for ISI_MCK, provided by PCK */
> + u32 isi_mck_hz;
> + const char *pck_name;
> + const char *pck_parent_name;
> };
>
> #endif /* __ATMEL_ISI_H__ */
> --
> 1.6.3.3
>
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
@ 2011-09-06 6:54 ` Guennadi Liakhovetski
0 siblings, 0 replies; 16+ messages in thread
From: Guennadi Liakhovetski @ 2011-09-06 6:54 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 6 Sep 2011, Josh Wu wrote:
> This patch enable the configuration for ISI_MCK, which is provided by programmable clock.
>
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> drivers/media/video/atmel-isi.c | 60 ++++++++++++++++++++++++++++++++++++++-
> include/media/atmel-isi.h | 4 ++
> 2 files changed, 63 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
> index 7b89f00..768bf59 100644
> --- a/drivers/media/video/atmel-isi.c
> +++ b/drivers/media/video/atmel-isi.c
> @@ -90,7 +90,10 @@ struct atmel_isi {
> struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
>
> struct completion complete;
> + /* ISI peripherial clock */
> struct clk *pclk;
> + /* ISI_MCK, provided by PCK */
> + struct clk *mck;
> unsigned int irq;
>
> struct isi_platform_data *pdata;
> @@ -763,6 +766,10 @@ static int isi_camera_add_device(struct soc_camera_device *icd)
> if (ret)
> return ret;
>
> + ret = clk_enable(isi->mck);
> + if (ret)
> + return ret;
> +
Don't you have to disable the pixel clock (isi->pclk), that you just have
enabled above this hunk, on the above error path?
> isi->icd = icd;
> dev_dbg(icd->parent, "Atmel ISI Camera driver attached to camera %d\n",
> icd->devnum);
> @@ -776,6 +783,7 @@ static void isi_camera_remove_device(struct soc_camera_device *icd)
>
> BUG_ON(icd != isi->icd);
>
> + clk_disable(isi->mck);
> clk_disable(isi->pclk);
> isi->icd = NULL;
>
> @@ -882,6 +890,49 @@ static struct soc_camera_host_ops isi_soc_camera_host_ops = {
> };
>
> /* -----------------------------------------------------------------------*/
> +/* Initialize ISI_MCK clock, called by atmel_isi_probe() function */
> +static int initialize_mck(struct platform_device *pdev,
> + struct atmel_isi *isi)
> +{
> + struct device *dev = &pdev->dev;
> + struct isi_platform_data *pdata = dev->platform_data;
> + struct clk *pck_parent;
> + int ret;
> +
> + if (!strlen(pdata->pck_name) || !strlen(pdata->pck_parent_name))
> + return -EINVAL;
> +
> + /* ISI_MCK is provided by PCK clock */
> + isi->mck = clk_get(dev, pdata->pck_name);
I think, it's still not what Russell meant. Look at
drivers/mmc/host/atmel-mci.c:
host->mck = clk_get(&pdev->dev, "mci_clk");
and in arch/arm/mach-at91/at91sam9g45.c they've got
CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.0", &mmc0_clk),
CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.1", &mmc1_clk),
where
#define CLKDEV_CON_DEV_ID(_con_id, _dev_id, _clk) \
{ \
.con_id = _con_id, \
.dev_id = _dev_id, \
.clk = _clk, \
}
I.e., in the device driver (mmc in this case) you only use the (platform)
device instance, whose dev_name(dev) is then matched against one of clock
lookups above, and a connection ID, which is used in case your device is
using more than one clock. In the ISI case, your pck1 clock, that you seem
to need here, doesn't have a clock lookup object, so, you might have to
add one, and then use its connection ID.
> + if (IS_ERR(isi->mck)) {
> + dev_err(dev, "Failed to get PCK: %s\n", pdata->pck_name);
> + return PTR_ERR(isi->mck);
> + }
> +
> + pck_parent = clk_get(dev, pdata->pck_parent_name);
> + if (IS_ERR(pck_parent)) {
> + ret = PTR_ERR(pck_parent);
> + dev_err(dev, "Failed to get PCK parent: %s\n",
> + pdata->pck_parent_name);
> + goto err_init_mck;
> + }
> +
> + ret = clk_set_parent(isi->mck, pck_parent);
I'm not entirely sure on this one, but as we had a similar situation with
clocks, we decided to extablish the clock hierarchy in the board code, and
only deal with the actual device clocks in the driver itself. I.e., we
moved all clk_set_parent() and setting up the parent clock into the board.
And I do think, this makes more sense, than doing this in the driver, not
all users of this driver will need to manage the parent clock, right?
> + clk_put(pck_parent);
> + if (ret)
> + goto err_init_mck;
> +
> + ret = clk_set_rate(isi->mck, pdata->isi_mck_hz);
> + if (ret < 0)
> + goto err_init_mck;
> +
> + return 0;
> +
> +err_init_mck:
> + clk_put(isi->mck);
> + return ret;
> +}
> +
> static int __devexit atmel_isi_remove(struct platform_device *pdev)
> {
> struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
> @@ -897,6 +948,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)
> isi->fb_descriptors_phys);
>
> iounmap(isi->regs);
> + clk_put(isi->mck);
> clk_put(isi->pclk);
> kfree(isi);
>
> @@ -915,7 +967,8 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
> struct isi_platform_data *pdata;
>
> pdata = dev->platform_data;
> - if (!pdata || !pdata->data_width_flags) {
> + if (!pdata || !pdata->data_width_flags || !pdata->isi_mck_hz
> + || !pdata->pck_name || !pdata->pck_parent_name) {
These names you won't need either.
> dev_err(&pdev->dev,
> "No config available for Atmel ISI\n");
> return -EINVAL;
> @@ -944,6 +997,11 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev)
> INIT_LIST_HEAD(&isi->video_buffer_list);
> INIT_LIST_HEAD(&isi->dma_desc_head);
>
> + /* Initialize ISI_MCK clock */
> + ret = initialize_mck(pdev, isi);
> + if (ret)
> + goto err_alloc_descriptors;
> +
> isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
> sizeof(struct fbd) * MAX_BUFFER_NUM,
> &isi->fb_descriptors_phys,
> diff --git a/include/media/atmel-isi.h b/include/media/atmel-isi.h
> index 26cece5..dcbb822 100644
> --- a/include/media/atmel-isi.h
> +++ b/include/media/atmel-isi.h
> @@ -114,6 +114,10 @@ struct isi_platform_data {
> u32 data_width_flags;
> /* Using for ISI_CFG1 */
> u32 frate;
> + /* Using for ISI_MCK, provided by PCK */
> + u32 isi_mck_hz;
> + const char *pck_name;
> + const char *pck_parent_name;
> };
>
> #endif /* __ATMEL_ISI_H__ */
> --
> 1.6.3.3
>
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
2011-09-06 6:54 ` Guennadi Liakhovetski
@ 2011-09-06 20:05 ` Jean-Christophe PLAGNIOL-VILLARD
-1 siblings, 0 replies; 16+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-09-06 20:05 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Josh Wu, linux-media, linux-kernel, linux-arm-kernel
On 08:54 Tue 06 Sep , Guennadi Liakhovetski wrote:
> On Tue, 6 Sep 2011, Josh Wu wrote:
>
> > This patch enable the configuration for ISI_MCK, which is provided by programmable clock.
> >
> > Signed-off-by: Josh Wu <josh.wu@atmel.com>
> > ---
> > drivers/media/video/atmel-isi.c | 60 ++++++++++++++++++++++++++++++++++++++-
> > include/media/atmel-isi.h | 4 ++
> > 2 files changed, 63 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
> > index 7b89f00..768bf59 100644
> > --- a/drivers/media/video/atmel-isi.c
> > +++ b/drivers/media/video/atmel-isi.c
> > @@ -90,7 +90,10 @@ struct atmel_isi {
> > struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
> >
> > struct completion complete;
> > + /* ISI peripherial clock */
> > struct clk *pclk;
> > + /* ISI_MCK, provided by PCK */
> > + struct clk *mck;
> > unsigned int irq;
> >
> > struct isi_platform_data *pdata;
> > @@ -763,6 +766,10 @@ static int isi_camera_add_device(struct soc_camera_device *icd)
> > if (ret)
> > return ret;
> >
> > + ret = clk_enable(isi->mck);
> > + if (ret)
> > + return ret;
> > +
>
> Don't you have to disable the pixel clock (isi->pclk), that you just have
> enabled above this hunk, on the above error path?
>
> > isi->icd = icd;
> > dev_dbg(icd->parent, "Atmel ISI Camera driver attached to camera %d\n",
> > icd->devnum);
> > @@ -776,6 +783,7 @@ static void isi_camera_remove_device(struct soc_camera_device *icd)
> >
> > BUG_ON(icd != isi->icd);
> >
> > + clk_disable(isi->mck);
> > clk_disable(isi->pclk);
> > isi->icd = NULL;
> >
> > @@ -882,6 +890,49 @@ static struct soc_camera_host_ops isi_soc_camera_host_ops = {
> > };
> >
> > /* -----------------------------------------------------------------------*/
> > +/* Initialize ISI_MCK clock, called by atmel_isi_probe() function */
> > +static int initialize_mck(struct platform_device *pdev,
> > + struct atmel_isi *isi)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct isi_platform_data *pdata = dev->platform_data;
> > + struct clk *pck_parent;
> > + int ret;
> > +
> > + if (!strlen(pdata->pck_name) || !strlen(pdata->pck_parent_name))
> > + return -EINVAL;
> > +
> > + /* ISI_MCK is provided by PCK clock */
> > + isi->mck = clk_get(dev, pdata->pck_name);
>
> I think, it's still not what Russell meant. Look at
or what I ask you too
> drivers/mmc/host/atmel-mci.c:
>
> host->mck = clk_get(&pdev->dev, "mci_clk");
>
> and in arch/arm/mach-at91/at91sam9g45.c they've got
>
> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.0", &mmc0_clk),
> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.1", &mmc1_clk),
>
> where
>
> #define CLKDEV_CON_DEV_ID(_con_id, _dev_id, _clk) \
> { \
> .con_id = _con_id, \
> .dev_id = _dev_id, \
> .clk = _clk, \
> }
>
> I.e., in the device driver (mmc in this case) you only use the (platform)
> device instance, whose dev_name(dev) is then matched against one of clock
> lookups above, and a connection ID, which is used in case your device is
> using more than one clock. In the ISI case, your pck1 clock, that you seem
> to need here, doesn't have a clock lookup object, so, you might have to
> add one, and then use its connection ID.
>
> > + if (IS_ERR(isi->mck)) {
> > + dev_err(dev, "Failed to get PCK: %s\n", pdata->pck_name);
> > + return PTR_ERR(isi->mck);
> > + }
> > +
> > + pck_parent = clk_get(dev, pdata->pck_parent_name);
> > + if (IS_ERR(pck_parent)) {
> > + ret = PTR_ERR(pck_parent);
> > + dev_err(dev, "Failed to get PCK parent: %s\n",
> > + pdata->pck_parent_name);
> > + goto err_init_mck;
> > + }
> > +
> > + ret = clk_set_parent(isi->mck, pck_parent);
>
> I'm not entirely sure on this one, but as we had a similar situation with
> clocks, we decided to extablish the clock hierarchy in the board code, and
> only deal with the actual device clocks in the driver itself. I.e., we
> moved all clk_set_parent() and setting up the parent clock into the board.
> And I do think, this makes more sense, than doing this in the driver, not
> all users of this driver will need to manage the parent clock, right?
I don't like to manage the clock in the board except if it's manadatory otherwise
we manage this at soc level
the driver does not have to manage the clock hierachy or detail implementation
but manage the clock enable/disable and speed depending on it's need
Best Regards,
J.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
@ 2011-09-06 20:05 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 16+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-09-06 20:05 UTC (permalink / raw)
To: linux-arm-kernel
On 08:54 Tue 06 Sep , Guennadi Liakhovetski wrote:
> On Tue, 6 Sep 2011, Josh Wu wrote:
>
> > This patch enable the configuration for ISI_MCK, which is provided by programmable clock.
> >
> > Signed-off-by: Josh Wu <josh.wu@atmel.com>
> > ---
> > drivers/media/video/atmel-isi.c | 60 ++++++++++++++++++++++++++++++++++++++-
> > include/media/atmel-isi.h | 4 ++
> > 2 files changed, 63 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
> > index 7b89f00..768bf59 100644
> > --- a/drivers/media/video/atmel-isi.c
> > +++ b/drivers/media/video/atmel-isi.c
> > @@ -90,7 +90,10 @@ struct atmel_isi {
> > struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
> >
> > struct completion complete;
> > + /* ISI peripherial clock */
> > struct clk *pclk;
> > + /* ISI_MCK, provided by PCK */
> > + struct clk *mck;
> > unsigned int irq;
> >
> > struct isi_platform_data *pdata;
> > @@ -763,6 +766,10 @@ static int isi_camera_add_device(struct soc_camera_device *icd)
> > if (ret)
> > return ret;
> >
> > + ret = clk_enable(isi->mck);
> > + if (ret)
> > + return ret;
> > +
>
> Don't you have to disable the pixel clock (isi->pclk), that you just have
> enabled above this hunk, on the above error path?
>
> > isi->icd = icd;
> > dev_dbg(icd->parent, "Atmel ISI Camera driver attached to camera %d\n",
> > icd->devnum);
> > @@ -776,6 +783,7 @@ static void isi_camera_remove_device(struct soc_camera_device *icd)
> >
> > BUG_ON(icd != isi->icd);
> >
> > + clk_disable(isi->mck);
> > clk_disable(isi->pclk);
> > isi->icd = NULL;
> >
> > @@ -882,6 +890,49 @@ static struct soc_camera_host_ops isi_soc_camera_host_ops = {
> > };
> >
> > /* -----------------------------------------------------------------------*/
> > +/* Initialize ISI_MCK clock, called by atmel_isi_probe() function */
> > +static int initialize_mck(struct platform_device *pdev,
> > + struct atmel_isi *isi)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct isi_platform_data *pdata = dev->platform_data;
> > + struct clk *pck_parent;
> > + int ret;
> > +
> > + if (!strlen(pdata->pck_name) || !strlen(pdata->pck_parent_name))
> > + return -EINVAL;
> > +
> > + /* ISI_MCK is provided by PCK clock */
> > + isi->mck = clk_get(dev, pdata->pck_name);
>
> I think, it's still not what Russell meant. Look at
or what I ask you too
> drivers/mmc/host/atmel-mci.c:
>
> host->mck = clk_get(&pdev->dev, "mci_clk");
>
> and in arch/arm/mach-at91/at91sam9g45.c they've got
>
> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.0", &mmc0_clk),
> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.1", &mmc1_clk),
>
> where
>
> #define CLKDEV_CON_DEV_ID(_con_id, _dev_id, _clk) \
> { \
> .con_id = _con_id, \
> .dev_id = _dev_id, \
> .clk = _clk, \
> }
>
> I.e., in the device driver (mmc in this case) you only use the (platform)
> device instance, whose dev_name(dev) is then matched against one of clock
> lookups above, and a connection ID, which is used in case your device is
> using more than one clock. In the ISI case, your pck1 clock, that you seem
> to need here, doesn't have a clock lookup object, so, you might have to
> add one, and then use its connection ID.
>
> > + if (IS_ERR(isi->mck)) {
> > + dev_err(dev, "Failed to get PCK: %s\n", pdata->pck_name);
> > + return PTR_ERR(isi->mck);
> > + }
> > +
> > + pck_parent = clk_get(dev, pdata->pck_parent_name);
> > + if (IS_ERR(pck_parent)) {
> > + ret = PTR_ERR(pck_parent);
> > + dev_err(dev, "Failed to get PCK parent: %s\n",
> > + pdata->pck_parent_name);
> > + goto err_init_mck;
> > + }
> > +
> > + ret = clk_set_parent(isi->mck, pck_parent);
>
> I'm not entirely sure on this one, but as we had a similar situation with
> clocks, we decided to extablish the clock hierarchy in the board code, and
> only deal with the actual device clocks in the driver itself. I.e., we
> moved all clk_set_parent() and setting up the parent clock into the board.
> And I do think, this makes more sense, than doing this in the driver, not
> all users of this driver will need to manage the parent clock, right?
I don't like to manage the clock in the board except if it's manadatory otherwise
we manage this at soc level
the driver does not have to manage the clock hierachy or detail implementation
but manage the clock enable/disable and speed depending on it's need
Best Regards,
J.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
2011-09-06 20:05 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2011-09-06 21:08 ` Sylwester Nawrocki
-1 siblings, 0 replies; 16+ messages in thread
From: Sylwester Nawrocki @ 2011-09-06 21:08 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: Guennadi Liakhovetski, Josh Wu, linux-kernel, linux-arm-kernel,
linux-media
On 09/06/2011 10:05 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> I'm not entirely sure on this one, but as we had a similar situation with
>> clocks, we decided to extablish the clock hierarchy in the board code, and
>> only deal with the actual device clocks in the driver itself. I.e., we
>> moved all clk_set_parent() and setting up the parent clock into the board.
>> And I do think, this makes more sense, than doing this in the driver, not
>> all users of this driver will need to manage the parent clock, right?
>
> I don't like to manage the clock in the board except if it's manadatory otherwise
> we manage this at soc level
>
> the driver does not have to manage the clock hierachy or detail implementation
> but manage the clock enable/disable and speed depending on it's need
We had a similar problem in the past and we ended up having the boot loader
setting up the parent clock for the device clock. The driver only controls clock
gating and sets its clock frequency based on an internal IP version information,
derived from the SoC revision.
AFAIK there is also a generic API at the runtime PM core so the driver can
register the clock(s) with it and only use pm_runtime_clk_* calls afterwards.
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
@ 2011-09-06 21:08 ` Sylwester Nawrocki
0 siblings, 0 replies; 16+ messages in thread
From: Sylwester Nawrocki @ 2011-09-06 21:08 UTC (permalink / raw)
To: linux-arm-kernel
On 09/06/2011 10:05 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> I'm not entirely sure on this one, but as we had a similar situation with
>> clocks, we decided to extablish the clock hierarchy in the board code, and
>> only deal with the actual device clocks in the driver itself. I.e., we
>> moved all clk_set_parent() and setting up the parent clock into the board.
>> And I do think, this makes more sense, than doing this in the driver, not
>> all users of this driver will need to manage the parent clock, right?
>
> I don't like to manage the clock in the board except if it's manadatory otherwise
> we manage this at soc level
>
> the driver does not have to manage the clock hierachy or detail implementation
> but manage the clock enable/disable and speed depending on it's need
We had a similar problem in the past and we ended up having the boot loader
setting up the parent clock for the device clock. The driver only controls clock
gating and sets its clock frequency based on an internal IP version information,
derived from the SoC revision.
AFAIK there is also a generic API at the runtime PM core so the driver can
register the clock(s) with it and only use pm_runtime_clk_* calls afterwards.
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
2011-09-06 6:54 ` Guennadi Liakhovetski
@ 2011-09-09 10:04 ` Nicolas Ferre
-1 siblings, 0 replies; 16+ messages in thread
From: Nicolas Ferre @ 2011-09-09 10:04 UTC (permalink / raw)
To: Josh Wu, linux-arm-kernel, linux-media
Cc: Guennadi Liakhovetski, plagnioj, linux-kernel
Le 06/09/2011 08:54, Guennadi Liakhovetski :
> On Tue, 6 Sep 2011, Josh Wu wrote:
>
>> This patch enable the configuration for ISI_MCK, which is provided by programmable clock.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>> drivers/media/video/atmel-isi.c | 60 ++++++++++++++++++++++++++++++++++++++-
>> include/media/atmel-isi.h | 4 ++
>> 2 files changed, 63 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
[..]
>> /* -----------------------------------------------------------------------*/
>> +/* Initialize ISI_MCK clock, called by atmel_isi_probe() function */
>> +static int initialize_mck(struct platform_device *pdev,
>> + struct atmel_isi *isi)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct isi_platform_data *pdata = dev->platform_data;
>> + struct clk *pck_parent;
>> + int ret;
>> +
>> + if (!strlen(pdata->pck_name) || !strlen(pdata->pck_parent_name))
>> + return -EINVAL;
>> +
>> + /* ISI_MCK is provided by PCK clock */
>> + isi->mck = clk_get(dev, pdata->pck_name);
>
> I think, it's still not what Russell meant. Look at
> drivers/mmc/host/atmel-mci.c:
>
> host->mck = clk_get(&pdev->dev, "mci_clk");
>
> and in arch/arm/mach-at91/at91sam9g45.c they've got
>
> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.0", &mmc0_clk),
> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.1", &mmc1_clk),
>
> where
>
> #define CLKDEV_CON_DEV_ID(_con_id, _dev_id, _clk) \
> { \
> .con_id = _con_id, \
> .dev_id = _dev_id, \
> .clk = _clk, \
> }
>
> I.e., in the device driver (mmc in this case) you only use the (platform)
> device instance, whose dev_name(dev) is then matched against one of clock
> lookups above, and a connection ID, which is used in case your device is
> using more than one clock. In the ISI case, your pck1 clock, that you seem
> to need here, doesn't have a clock lookup object, so, you might have to
> add one, and then use its connection ID.
>
>> + if (IS_ERR(isi->mck)) {
>> + dev_err(dev, "Failed to get PCK: %s\n", pdata->pck_name);
>> + return PTR_ERR(isi->mck);
>> + }
>> +
>> + pck_parent = clk_get(dev, pdata->pck_parent_name);
>> + if (IS_ERR(pck_parent)) {
>> + ret = PTR_ERR(pck_parent);
>> + dev_err(dev, "Failed to get PCK parent: %s\n",
>> + pdata->pck_parent_name);
>> + goto err_init_mck;
>> + }
>> +
>> + ret = clk_set_parent(isi->mck, pck_parent);
>
> I'm not entirely sure on this one, but as we had a similar situation with
> clocks, we decided to extablish the clock hierarchy in the board code, and
> only deal with the actual device clocks in the driver itself. I.e., we
> moved all clk_set_parent() and setting up the parent clock into the board.
> And I do think, this makes more sense, than doing this in the driver, not
> all users of this driver will need to manage the parent clock, right?
Exactly.
Josh, for the two comments by Guennadi above, you can take
sound/soc/atmel/sam9g20_wm8731.c
as an example of using PCK and parent clocks. You will also find how to
use named clocks and how to set the programmable clocks rate...
>> + clk_put(pck_parent);
>> + if (ret)
>> + goto err_init_mck;
>> +
>> + ret = clk_set_rate(isi->mck, pdata->isi_mck_hz);
>> + if (ret < 0)
>> + goto err_init_mck;
>> +
>> + return 0;
>> +
>> +err_init_mck:
>> + clk_put(isi->mck);
>> + return ret;
>> +}
>> +
>> static int __devexit atmel_isi_remove(struct platform_device *pdev)
>> {
>> struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
>> @@ -897,6 +948,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)
>> isi->fb_descriptors_phys);
>>
>> iounmap(isi->regs);
>> + clk_put(isi->mck);
>> clk_put(isi->pclk);
>> kfree(isi);
[..]
Best regards,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
@ 2011-09-09 10:04 ` Nicolas Ferre
0 siblings, 0 replies; 16+ messages in thread
From: Nicolas Ferre @ 2011-09-09 10:04 UTC (permalink / raw)
To: linux-arm-kernel
Le 06/09/2011 08:54, Guennadi Liakhovetski :
> On Tue, 6 Sep 2011, Josh Wu wrote:
>
>> This patch enable the configuration for ISI_MCK, which is provided by programmable clock.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>> drivers/media/video/atmel-isi.c | 60 ++++++++++++++++++++++++++++++++++++++-
>> include/media/atmel-isi.h | 4 ++
>> 2 files changed, 63 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
[..]
>> /* -----------------------------------------------------------------------*/
>> +/* Initialize ISI_MCK clock, called by atmel_isi_probe() function */
>> +static int initialize_mck(struct platform_device *pdev,
>> + struct atmel_isi *isi)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct isi_platform_data *pdata = dev->platform_data;
>> + struct clk *pck_parent;
>> + int ret;
>> +
>> + if (!strlen(pdata->pck_name) || !strlen(pdata->pck_parent_name))
>> + return -EINVAL;
>> +
>> + /* ISI_MCK is provided by PCK clock */
>> + isi->mck = clk_get(dev, pdata->pck_name);
>
> I think, it's still not what Russell meant. Look at
> drivers/mmc/host/atmel-mci.c:
>
> host->mck = clk_get(&pdev->dev, "mci_clk");
>
> and in arch/arm/mach-at91/at91sam9g45.c they've got
>
> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.0", &mmc0_clk),
> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.1", &mmc1_clk),
>
> where
>
> #define CLKDEV_CON_DEV_ID(_con_id, _dev_id, _clk) \
> { \
> .con_id = _con_id, \
> .dev_id = _dev_id, \
> .clk = _clk, \
> }
>
> I.e., in the device driver (mmc in this case) you only use the (platform)
> device instance, whose dev_name(dev) is then matched against one of clock
> lookups above, and a connection ID, which is used in case your device is
> using more than one clock. In the ISI case, your pck1 clock, that you seem
> to need here, doesn't have a clock lookup object, so, you might have to
> add one, and then use its connection ID.
>
>> + if (IS_ERR(isi->mck)) {
>> + dev_err(dev, "Failed to get PCK: %s\n", pdata->pck_name);
>> + return PTR_ERR(isi->mck);
>> + }
>> +
>> + pck_parent = clk_get(dev, pdata->pck_parent_name);
>> + if (IS_ERR(pck_parent)) {
>> + ret = PTR_ERR(pck_parent);
>> + dev_err(dev, "Failed to get PCK parent: %s\n",
>> + pdata->pck_parent_name);
>> + goto err_init_mck;
>> + }
>> +
>> + ret = clk_set_parent(isi->mck, pck_parent);
>
> I'm not entirely sure on this one, but as we had a similar situation with
> clocks, we decided to extablish the clock hierarchy in the board code, and
> only deal with the actual device clocks in the driver itself. I.e., we
> moved all clk_set_parent() and setting up the parent clock into the board.
> And I do think, this makes more sense, than doing this in the driver, not
> all users of this driver will need to manage the parent clock, right?
Exactly.
Josh, for the two comments by Guennadi above, you can take
sound/soc/atmel/sam9g20_wm8731.c
as an example of using PCK and parent clocks. You will also find how to
use named clocks and how to set the programmable clocks rate...
>> + clk_put(pck_parent);
>> + if (ret)
>> + goto err_init_mck;
>> +
>> + ret = clk_set_rate(isi->mck, pdata->isi_mck_hz);
>> + if (ret < 0)
>> + goto err_init_mck;
>> +
>> + return 0;
>> +
>> +err_init_mck:
>> + clk_put(isi->mck);
>> + return ret;
>> +}
>> +
>> static int __devexit atmel_isi_remove(struct platform_device *pdev)
>> {
>> struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
>> @@ -897,6 +948,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)
>> isi->fb_descriptors_phys);
>>
>> iounmap(isi->regs);
>> + clk_put(isi->mck);
>> clk_put(isi->pclk);
>> kfree(isi);
[..]
Best regards,
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
2011-09-06 21:08 ` Sylwester Nawrocki
@ 2011-09-15 13:23 ` Jean-Christophe PLAGNIOL-VILLARD
-1 siblings, 0 replies; 16+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-09-15 13:23 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Guennadi Liakhovetski, Josh Wu, linux-kernel, linux-arm-kernel,
linux-media
On 23:08 Tue 06 Sep , Sylwester Nawrocki wrote:
> On 09/06/2011 10:05 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >> I'm not entirely sure on this one, but as we had a similar situation with
> >> clocks, we decided to extablish the clock hierarchy in the board code, and
> >> only deal with the actual device clocks in the driver itself. I.e., we
> >> moved all clk_set_parent() and setting up the parent clock into the board.
> >> And I do think, this makes more sense, than doing this in the driver, not
> >> all users of this driver will need to manage the parent clock, right?
> >
> > I don't like to manage the clock in the board except if it's manadatory otherwise
> > we manage this at soc level
> >
> > the driver does not have to manage the clock hierachy or detail implementation
> > but manage the clock enable/disable and speed depending on it's need
>
> We had a similar problem in the past and we ended up having the boot loader
> setting up the parent clock for the device clock. The driver only controls clock
> gating and sets its clock frequency based on an internal IP version information,
> derived from the SoC revision.
sorry NACK
I do not want to rely on bootloader
when we will have the DT we will pass it via it right now we need find an
other generic way
Best Regards,
J.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
@ 2011-09-15 13:23 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 16+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-09-15 13:23 UTC (permalink / raw)
To: linux-arm-kernel
On 23:08 Tue 06 Sep , Sylwester Nawrocki wrote:
> On 09/06/2011 10:05 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >> I'm not entirely sure on this one, but as we had a similar situation with
> >> clocks, we decided to extablish the clock hierarchy in the board code, and
> >> only deal with the actual device clocks in the driver itself. I.e., we
> >> moved all clk_set_parent() and setting up the parent clock into the board.
> >> And I do think, this makes more sense, than doing this in the driver, not
> >> all users of this driver will need to manage the parent clock, right?
> >
> > I don't like to manage the clock in the board except if it's manadatory otherwise
> > we manage this at soc level
> >
> > the driver does not have to manage the clock hierachy or detail implementation
> > but manage the clock enable/disable and speed depending on it's need
>
> We had a similar problem in the past and we ended up having the boot loader
> setting up the parent clock for the device clock. The driver only controls clock
> gating and sets its clock frequency based on an internal IP version information,
> derived from the SoC revision.
sorry NACK
I do not want to rely on bootloader
when we will have the DT we will pass it via it right now we need find an
other generic way
Best Regards,
J.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
2011-09-15 13:23 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2011-09-15 15:30 ` Sylwester Nawrocki
-1 siblings, 0 replies; 16+ messages in thread
From: Sylwester Nawrocki @ 2011-09-15 15:30 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: Sylwester Nawrocki, Josh Wu, linux-media, Guennadi Liakhovetski,
linux-arm-kernel, linux-kernel
On 09/15/2011 03:23 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 23:08 Tue 06 Sep , Sylwester Nawrocki wrote:
>> On 09/06/2011 10:05 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>> I'm not entirely sure on this one, but as we had a similar situation with
>>>> clocks, we decided to extablish the clock hierarchy in the board code, and
>>>> only deal with the actual device clocks in the driver itself. I.e., we
>>>> moved all clk_set_parent() and setting up the parent clock into the board.
>>>> And I do think, this makes more sense, than doing this in the driver, not
>>>> all users of this driver will need to manage the parent clock, right?
>>>
>>> I don't like to manage the clock in the board except if it's manadatory otherwise
>>> we manage this at soc level
You often just can't decide about clocks hierarchy at SoC level as it
can depend on the board layout and which devices are used on it.
>>>
>>> the driver does not have to manage the clock hierachy or detail implementation
>>> but manage the clock enable/disable and speed depending on it's need
I think everyone agrees on that.
>>
>> We had a similar problem in the past and we ended up having the boot loader
>> setting up the parent clock for the device clock. The driver only controls clock
>> gating and sets its clock frequency based on an internal IP version information,
>> derived from the SoC revision.
> sorry NACK
:) When we tried to upstream the code setting up parent clocks in board files
some people disagreed on that too..
>
> I do not want to rely on bootloader
Yeah, I don't believe this is best possible solution either..
> when we will have the DT we will pass it via it right now we need find an
I hope this can be be properly resolved with the DT. I thought there is not
much work
going on yet wrt supporting clocks hierarchy in DT though.
> other generic way
>
> Best Regards,
> J.
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
@ 2011-09-15 15:30 ` Sylwester Nawrocki
0 siblings, 0 replies; 16+ messages in thread
From: Sylwester Nawrocki @ 2011-09-15 15:30 UTC (permalink / raw)
To: linux-arm-kernel
On 09/15/2011 03:23 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 23:08 Tue 06 Sep , Sylwester Nawrocki wrote:
>> On 09/06/2011 10:05 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>>> I'm not entirely sure on this one, but as we had a similar situation with
>>>> clocks, we decided to extablish the clock hierarchy in the board code, and
>>>> only deal with the actual device clocks in the driver itself. I.e., we
>>>> moved all clk_set_parent() and setting up the parent clock into the board.
>>>> And I do think, this makes more sense, than doing this in the driver, not
>>>> all users of this driver will need to manage the parent clock, right?
>>>
>>> I don't like to manage the clock in the board except if it's manadatory otherwise
>>> we manage this at soc level
You often just can't decide about clocks hierarchy at SoC level as it
can depend on the board layout and which devices are used on it.
>>>
>>> the driver does not have to manage the clock hierachy or detail implementation
>>> but manage the clock enable/disable and speed depending on it's need
I think everyone agrees on that.
>>
>> We had a similar problem in the past and we ended up having the boot loader
>> setting up the parent clock for the device clock. The driver only controls clock
>> gating and sets its clock frequency based on an internal IP version information,
>> derived from the SoC revision.
> sorry NACK
:) When we tried to upstream the code setting up parent clocks in board files
some people disagreed on that too..
>
> I do not want to rely on bootloader
Yeah, I don't believe this is best possible solution either..
> when we will have the DT we will pass it via it right now we need find an
I hope this can be be properly resolved with the DT. I thought there is not
much work
going on yet wrt supporting clocks hierarchy in DT though.
> other generic way
>
> Best Regards,
> J.
--
Regards,
Sylwester
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
2011-09-09 10:04 ` Nicolas Ferre
@ 2011-09-22 3:23 ` Wu, Josh
-1 siblings, 0 replies; 16+ messages in thread
From: Wu, Josh @ 2011-09-22 3:23 UTC (permalink / raw)
To: Ferre, Nicolas, linux-arm-kernel, linux-media
Cc: Guennadi Liakhovetski, plagnioj, linux-kernel
Hi, Nicolas
On Friday, September 09, 2011 6:05 PM, Nicolas Ferre wrote:
> Le 06/09/2011 08:54, Guennadi Liakhovetski :
>> On Tue, 6 Sep 2011, Josh Wu wrote:
>>
>>> This patch enable the configuration for ISI_MCK, which is provided by programmable clock.
>>>
>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>> ---
>>> drivers/media/video/atmel-isi.c | 60 ++++++++++++++++++++++++++++++++++++++-
>>> include/media/atmel-isi.h | 4 ++
>>> 2 files changed, 63 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/media/video/atmel-isi.c
>>> b/drivers/media/video/atmel-isi.c
> [..]
>>> /*
>>> ---------------------------------------------------------------------
>>> --*/
>>> +/* Initialize ISI_MCK clock, called by atmel_isi_probe() function */
>>> +static int initialize_mck(struct platform_device *pdev,
>>> + struct atmel_isi *isi)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct isi_platform_data *pdata = dev->platform_data;
>>> + struct clk *pck_parent;
>>> + int ret;
>>> +
>>> + if (!strlen(pdata->pck_name) || !strlen(pdata->pck_parent_name))
>>> + return -EINVAL;
>>> +
>>> + /* ISI_MCK is provided by PCK clock */
>>> + isi->mck = clk_get(dev, pdata->pck_name);
>>
>> I think, it's still not what Russell meant. Look at
>> drivers/mmc/host/atmel-mci.c:
>>
>> host->mck = clk_get(&pdev->dev, "mci_clk");
>>
>> and in arch/arm/mach-at91/at91sam9g45.c they've got
>>
>> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.0", &mmc0_clk),
>> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.1", &mmc1_clk),
>>
>> where
>>
>> #define CLKDEV_CON_DEV_ID(_con_id, _dev_id, _clk) \
>> { \
>> .con_id = _con_id, \
>> .dev_id = _dev_id, \
>> .clk = _clk, \
>> }
>>
>> I.e., in the device driver (mmc in this case) you only use the
>> (platform) device instance, whose dev_name(dev) is then matched
>> against one of clock lookups above, and a connection ID, which is used
>> in case your device is using more than one clock. In the ISI case,
>> your pck1 clock, that you seem to need here, doesn't have a clock
>> lookup object, so, you might have to add one, and then use its connection ID.
>>
>>> + if (IS_ERR(isi->mck)) {
>>> + dev_err(dev, "Failed to get PCK: %s\n", pdata->pck_name);
>>> + return PTR_ERR(isi->mck);
>>> + }
>>> +
>>> + pck_parent = clk_get(dev, pdata->pck_parent_name);
>>> + if (IS_ERR(pck_parent)) {
>>> + ret = PTR_ERR(pck_parent);
>>> + dev_err(dev, "Failed to get PCK parent: %s\n",
>>> + pdata->pck_parent_name);
>>> + goto err_init_mck;
>>> + }
>>> +
>>> + ret = clk_set_parent(isi->mck, pck_parent);
>>
>> I'm not entirely sure on this one, but as we had a similar situation
>> with clocks, we decided to extablish the clock hierarchy in the board
>> code, and only deal with the actual device clocks in the driver
>> itself. I.e., we moved all clk_set_parent() and setting up the parent clock into the board.
>> And I do think, this makes more sense, than doing this in the driver,
>> not all users of this driver will need to manage the parent clock, right?
> Exactly.
>
> Josh, for the two comments by Guennadi above, you can take sound/soc/atmel/sam9g20_wm8731.c as an example of using PCK and parent clocks. You will also find how to use named clocks and how to set the programmable clocks rate...
According to sam9g20_wm8731.c file, and combined Guennadi and J.C's advices, I'd like to only add clk_set_rate() and enable/disble isi_mck code in ISI driver.
Also I will define isi_mck clock in soc chip file(at91sam9g45.c).
Then I will move all setting up the parent clock code into the at91sam9g45_devices.c, where there are code to add Atmel ISI device.
I will generate the new version patch for it soon.
>>> + clk_put(pck_parent);
>>> + if (ret)
>>> + goto err_init_mck;
>>> +
>>> + ret = clk_set_rate(isi->mck, pdata->isi_mck_hz);
>>> + if (ret < 0)
>>> + goto err_init_mck;
>>> +
>>> + return 0;
>>> +
>>> +err_init_mck:
>>> + clk_put(isi->mck);
>>> + return ret;
>>> +}
>>> +
>>> static int __devexit atmel_isi_remove(struct platform_device *pdev)
>>> {
>>> struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
>>> @@ -897,6 +948,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)
>>> isi->fb_descriptors_phys);
>>>
>>> iounmap(isi->regs);
>>> + clk_put(isi->mck);
>>> clk_put(isi->pclk);
>>> kfree(isi);
> [..]
> Best regards,
> --
> Nicolas Ferre
Best Regards,
Josh Wu
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
@ 2011-09-22 3:23 ` Wu, Josh
0 siblings, 0 replies; 16+ messages in thread
From: Wu, Josh @ 2011-09-22 3:23 UTC (permalink / raw)
To: linux-arm-kernel
Hi, Nicolas
On Friday, September 09, 2011 6:05 PM, Nicolas Ferre wrote:
> Le 06/09/2011 08:54, Guennadi Liakhovetski :
>> On Tue, 6 Sep 2011, Josh Wu wrote:
>>
>>> This patch enable the configuration for ISI_MCK, which is provided by programmable clock.
>>>
>>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>>> ---
>>> drivers/media/video/atmel-isi.c | 60 ++++++++++++++++++++++++++++++++++++++-
>>> include/media/atmel-isi.h | 4 ++
>>> 2 files changed, 63 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/media/video/atmel-isi.c
>>> b/drivers/media/video/atmel-isi.c
> [..]
>>> /*
>>> ---------------------------------------------------------------------
>>> --*/
>>> +/* Initialize ISI_MCK clock, called by atmel_isi_probe() function */
>>> +static int initialize_mck(struct platform_device *pdev,
>>> + struct atmel_isi *isi)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct isi_platform_data *pdata = dev->platform_data;
>>> + struct clk *pck_parent;
>>> + int ret;
>>> +
>>> + if (!strlen(pdata->pck_name) || !strlen(pdata->pck_parent_name))
>>> + return -EINVAL;
>>> +
>>> + /* ISI_MCK is provided by PCK clock */
>>> + isi->mck = clk_get(dev, pdata->pck_name);
>>
>> I think, it's still not what Russell meant. Look at
>> drivers/mmc/host/atmel-mci.c:
>>
>> host->mck = clk_get(&pdev->dev, "mci_clk");
>>
>> and in arch/arm/mach-at91/at91sam9g45.c they've got
>>
>> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.0", &mmc0_clk),
>> CLKDEV_CON_DEV_ID("mci_clk", "atmel_mci.1", &mmc1_clk),
>>
>> where
>>
>> #define CLKDEV_CON_DEV_ID(_con_id, _dev_id, _clk) \
>> { \
>> .con_id = _con_id, \
>> .dev_id = _dev_id, \
>> .clk = _clk, \
>> }
>>
>> I.e., in the device driver (mmc in this case) you only use the
>> (platform) device instance, whose dev_name(dev) is then matched
>> against one of clock lookups above, and a connection ID, which is used
>> in case your device is using more than one clock. In the ISI case,
>> your pck1 clock, that you seem to need here, doesn't have a clock
>> lookup object, so, you might have to add one, and then use its connection ID.
>>
>>> + if (IS_ERR(isi->mck)) {
>>> + dev_err(dev, "Failed to get PCK: %s\n", pdata->pck_name);
>>> + return PTR_ERR(isi->mck);
>>> + }
>>> +
>>> + pck_parent = clk_get(dev, pdata->pck_parent_name);
>>> + if (IS_ERR(pck_parent)) {
>>> + ret = PTR_ERR(pck_parent);
>>> + dev_err(dev, "Failed to get PCK parent: %s\n",
>>> + pdata->pck_parent_name);
>>> + goto err_init_mck;
>>> + }
>>> +
>>> + ret = clk_set_parent(isi->mck, pck_parent);
>>
>> I'm not entirely sure on this one, but as we had a similar situation
>> with clocks, we decided to extablish the clock hierarchy in the board
>> code, and only deal with the actual device clocks in the driver
>> itself. I.e., we moved all clk_set_parent() and setting up the parent clock into the board.
>> And I do think, this makes more sense, than doing this in the driver,
>> not all users of this driver will need to manage the parent clock, right?
> Exactly.
>
> Josh, for the two comments by Guennadi above, you can take sound/soc/atmel/sam9g20_wm8731.c as an example of using PCK and parent clocks. You will also find how to use named clocks and how to set the programmable clocks rate...
According to sam9g20_wm8731.c file, and combined Guennadi and J.C's advices, I'd like to only add clk_set_rate() and enable/disble isi_mck code in ISI driver.
Also I will define isi_mck clock in soc chip file(at91sam9g45.c).
Then I will move all setting up the parent clock code into the at91sam9g45_devices.c, where there are code to add Atmel ISI device.
I will generate the new version patch for it soon.
>>> + clk_put(pck_parent);
>>> + if (ret)
>>> + goto err_init_mck;
>>> +
>>> + ret = clk_set_rate(isi->mck, pdata->isi_mck_hz);
>>> + if (ret < 0)
>>> + goto err_init_mck;
>>> +
>>> + return 0;
>>> +
>>> +err_init_mck:
>>> + clk_put(isi->mck);
>>> + return ret;
>>> +}
>>> +
>>> static int __devexit atmel_isi_remove(struct platform_device *pdev)
>>> {
>>> struct soc_camera_host *soc_host = to_soc_camera_host(&pdev->dev);
>>> @@ -897,6 +948,7 @@ static int __devexit atmel_isi_remove(struct platform_device *pdev)
>>> isi->fb_descriptors_phys);
>>>
>>> iounmap(isi->regs);
>>> + clk_put(isi->mck);
>>> clk_put(isi->pclk);
>>> kfree(isi);
> [..]
> Best regards,
> --
> Nicolas Ferre
Best Regards,
Josh Wu
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2011-09-22 3:24 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-06 5:56 [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver Josh Wu
2011-09-06 5:56 ` Josh Wu
2011-09-06 6:54 ` Guennadi Liakhovetski
2011-09-06 6:54 ` Guennadi Liakhovetski
2011-09-06 20:05 ` Jean-Christophe PLAGNIOL-VILLARD
2011-09-06 20:05 ` Jean-Christophe PLAGNIOL-VILLARD
2011-09-06 21:08 ` Sylwester Nawrocki
2011-09-06 21:08 ` Sylwester Nawrocki
2011-09-15 13:23 ` Jean-Christophe PLAGNIOL-VILLARD
2011-09-15 13:23 ` Jean-Christophe PLAGNIOL-VILLARD
2011-09-15 15:30 ` Sylwester Nawrocki
2011-09-15 15:30 ` Sylwester Nawrocki
2011-09-09 10:04 ` Nicolas Ferre
2011-09-09 10:04 ` Nicolas Ferre
2011-09-22 3:23 ` Wu, Josh
2011-09-22 3:23 ` Wu, Josh
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.