This patchset includes device tree support for rotator of exynos4210/4x12/5250. Unfortunately, each of them has slightly different limitations of image size. The rotator can support several image formats(RGB888/555, YCbCr422/420_2/3p). For convinience, however, exynos drm rotator driver only support RGB888 and YCbCr420 2-Plane formats. For example, the exynos4210 has 16k x 16k maximum size . But rest of them has 8k x 8k. In addition, RGB888 X/Y pixel size of exynos5250 should be multiple of 2. But others are multiple of 4. Thus, we should define different compatibles and limit tables for each chipsets.
Changes from v1: - Added exynos5250 binding. - Move limit table into driver code from DT-nodes
Chanho Park (5): drm/exynos: add device tree support for rotator ARM: dts: Add rotator node for exynos4210 ARM: dts: Add rotator node for exynos4x12 ARM: dts: Add rotator node for exynos5250 ARM: dts: Add dt binding documentation for exynos rotator
.../devicetree/bindings/gpu/samsung-rotator.txt | 26 +++++ arch/arm/boot/dts/exynos4.dtsi | 9 ++ arch/arm/boot/dts/exynos4x12.dtsi | 4 + arch/arm/boot/dts/exynos5250.dtsi | 9 ++ drivers/gpu/drm/exynos/exynos_drm_rotator.c | 109 +++++++++++++++----- 5 files changed, 129 insertions(+), 28 deletions(-) create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt
The exynos4 platform is only dt-based since 3.10, we should convert driver data and ids to dt-based parsing methods. The rotator driver has a limit table to get size limit of input picture. Each SoCs has slightly different limit value compared with any others. For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others have 8k x 8k. Another example the exynos5250 should have multiple of 2 pixel size for its X/Y axis. Thus, we should keep different tables for each of them.
Signed-off-by: Chanho Park chanho61.park@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/gpu/drm/exynos/exynos_drm_rotator.c | 109 ++++++++++++++++++++------- 1 file changed, 81 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c index 427640a..39b09e0 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c +++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c @@ -632,6 +632,73 @@ static int rotator_ippdrv_start(struct device *dev, enum drm_exynos_ipp_cmd cmd) return 0; }
+static struct rot_limit_table rot_limit_tbl_4210 = { + .ycbcr420_2p = { + .min_w = 32, + .min_h = 32, + .max_w = SZ_64K, + .max_h = SZ_64K, + .align = 3, + }, + .rgb888 = { + .min_w = 8, + .min_h = 8, + .max_w = SZ_16K, + .max_h = SZ_16K, + .align = 2, + }, +}; + +static struct rot_limit_table rot_limit_tbl_4x12 = { + .ycbcr420_2p = { + .min_w = 32, + .min_h = 32, + .max_w = SZ_32K, + .max_h = SZ_32K, + .align = 3, + }, + .rgb888 = { + .min_w = 8, + .min_h = 8, + .max_w = SZ_8K, + .max_h = SZ_8K, + .align = 2, + }, +}; + +static struct rot_limit_table rot_limit_tbl_5250 = { + .ycbcr420_2p = { + .min_w = 32, + .min_h = 32, + .max_w = SZ_32K, + .max_h = SZ_32K, + .align = 3, + }, + .rgb888 = { + .min_w = 8, + .min_h = 8, + .max_w = SZ_8K, + .max_h = SZ_8K, + .align = 1, + }, +}; + +static const struct of_device_id exynos_rotator_match[] = { + { + .compatible = "samsung,exynos4210-rotator", + .data = &rot_limit_tbl_4210, + }, + { + .compatible = "samsung,exynos4212-rotator", + .data = &rot_limit_tbl_4x12, + }, + { + .compatible = "samsung,exynos5250-rotator", + .data = &rot_limit_tbl_5250, + }, + {}, +}; + static int rotator_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device *pdev) return -ENOMEM; }
- rot->limit_tbl = (struct rot_limit_table *) - platform_get_device_id(pdev)->driver_data; + if (dev->of_node) { + const struct of_device_id *match; + match = of_match_node(of_match_ptr(exynos_rotator_match), + dev->of_node); + if (match == NULL) { + dev_err(dev, "failed to match node\n"); + return -ENODEV; + } + rot->limit_tbl = (struct rot_limit_table *)match->data; + } else { + dev_err(dev, "cannot find binding\n"); + return -ENODEV; + }
rot->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); rot->regs = devm_ioremap_resource(dev, rot->regs_res); @@ -718,31 +796,6 @@ static int rotator_remove(struct platform_device *pdev) return 0; }
-static struct rot_limit_table rot_limit_tbl = { - .ycbcr420_2p = { - .min_w = 32, - .min_h = 32, - .max_w = SZ_32K, - .max_h = SZ_32K, - .align = 3, - }, - .rgb888 = { - .min_w = 8, - .min_h = 8, - .max_w = SZ_8K, - .max_h = SZ_8K, - .align = 2, - }, -}; - -static struct platform_device_id rotator_driver_ids[] = { - { - .name = "exynos-rot", - .driver_data = (unsigned long)&rot_limit_tbl, - }, - {}, -}; - static int rotator_clk_crtl(struct rot_context *rot, bool enable) { if (enable) { @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = { struct platform_driver rotator_driver = { .probe = rotator_probe, .remove = rotator_remove, - .id_table = rotator_driver_ids, .driver = { .name = "exynos-rot", .owner = THIS_MODULE, .pm = &rotator_pm_ops, + .of_match_table = of_match_ptr(exynos_rotator_match), }, };
Hi Chanho,
On 9 August 2013 13:10, Chanho Park chanho61.park@samsung.com wrote:
The exynos4 platform is only dt-based since 3.10, we should convert driver data and ids to dt-based parsing methods. The rotator driver has a limit table to get size limit of input picture. Each SoCs has slightly different limit value compared with any others. For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others have 8k x 8k. Another example the exynos5250 should have multiple of 2 pixel size for its X/Y axis. Thus, we should keep different tables for each of them.
Signed-off-by: Chanho Park chanho61.park@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/exynos/exynos_drm_rotator.c | 109 ++++++++++++++++++++------- 1 file changed, 81 insertions(+), 28 deletions(-)
[snip]
+static const struct of_device_id exynos_rotator_match[] = {
{.compatible = "samsung,exynos4210-rotator",.data = &rot_limit_tbl_4210,},{.compatible = "samsung,exynos4212-rotator",.data = &rot_limit_tbl_4x12,},{.compatible = "samsung,exynos5250-rotator",.data = &rot_limit_tbl_5250,},{},+};
static int rotator_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device *pdev) return -ENOMEM; }
rot->limit_tbl = (struct rot_limit_table *)platform_get_device_id(pdev)->driver_data;
if (dev->of_node) {const struct of_device_id *match;match = of_match_node(of_match_ptr(exynos_rotator_match),
of_match_ptr is not needed since exynos_rotator_match is always compiled in.
[snip]
-static struct platform_device_id rotator_driver_ids[] = {
{.name = "exynos-rot",.driver_data = (unsigned long)&rot_limit_tbl,},{},-};
static int rotator_clk_crtl(struct rot_context *rot, bool enable) { if (enable) { @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = { struct platform_driver rotator_driver = { .probe = rotator_probe, .remove = rotator_remove,
.id_table = rotator_driver_ids, .driver = { .name = "exynos-rot", .owner = THIS_MODULE, .pm = &rotator_pm_ops,
.of_match_table = of_match_ptr(exynos_rotator_match),
ditto
Hi Sachin,
-----Original Message----- From: Sachin Kamat [mailto:sachin.kamat@linaro.org] Sent: Friday, August 09, 2013 6:49 PM To: Chanho Park Cc: inki.dae@samsung.com; kgene.kim@samsung.com; dri- devel@lists.freedesktop.org; kyungmin.park@samsung.com; mark.rutland@arm.com; l.stach@pengutronix.de; s.nawrocki@samsung.com; tomasz.figa@gmail.com; linux-samsung-soc@vger.kernel.org; linux-arm- kernel@lists.infradead.org; devicetree@vger.kernel.org Subject: Re: [PATCHv2 1/5] drm/exynos: add device tree support for rotator
Hi Chanho,
On 9 August 2013 13:10, Chanho Park chanho61.park@samsung.com wrote:
The exynos4 platform is only dt-based since 3.10, we should convert driver data and ids to dt-based parsing methods. The rotator driver has a limit table to get size limit of input picture. Each SoCs has slightly different limit value compared with any others. For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others have 8k x 8k. Another example the exynos5250 should have multiple of 2 pixel size for its X/Y axis. Thus, we should keep different tables for
each of them.
Signed-off-by: Chanho Park chanho61.park@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/exynos/exynos_drm_rotator.c | 109 ++++++++++++++++++++------- 1 file changed, 81 insertions(+), 28 deletions(-)
[snip]
+static const struct of_device_id exynos_rotator_match[] = {
{.compatible = "samsung,exynos4210-rotator",.data = &rot_limit_tbl_4210,},{.compatible = "samsung,exynos4212-rotator",.data = &rot_limit_tbl_4x12,},{.compatible = "samsung,exynos5250-rotator",.data = &rot_limit_tbl_5250,},{},+};
static int rotator_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device *pdev) return -ENOMEM; }
rot->limit_tbl = (struct rot_limit_table *)
platform_get_device_id(pdev)->driver_data;
if (dev->of_node) {const struct of_device_id *match;match =- of_match_node(of_match_ptr(exynos_rotator_match),
of_match_ptr is not needed since exynos_rotator_match is always compiled in.
OK. I'll remove it. Thanks.
[snip]
-static struct platform_device_id rotator_driver_ids[] = {
{.name = "exynos-rot",.driver_data = (unsigned long)&rot_limit_tbl,},{},-};
static int rotator_clk_crtl(struct rot_context *rot, bool enable) { if (enable) { @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = { struct platform_driver rotator_driver = { .probe = rotator_probe, .remove = rotator_remove,
.id_table = rotator_driver_ids, .driver = { .name = "exynos-rot", .owner = THIS_MODULE, .pm = &rotator_pm_ops,
.of_match_table = of_match_ptr(exynos_rotator_match),ditto
-- With warm regards, Sachin
Hi Chanho,
On Friday 09 of August 2013 16:40:49 Chanho Park wrote:
The exynos4 platform is only dt-based since 3.10, we should convert driver data and ids to dt-based parsing methods. The rotator driver has a limit table to get size limit of input picture. Each SoCs has slightly different limit value compared with any others. For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others have 8k x 8k. Another example the exynos5250 should have multiple of 2 pixel size for its X/Y axis. Thus, we should keep different tables for each of them.
Signed-off-by: Chanho Park chanho61.park@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/exynos/exynos_drm_rotator.c | 109 ++++++++++++++++++++------- 1 file changed, 81 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c index 427640a..39b09e0 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c +++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c @@ -632,6 +632,73 @@ static int rotator_ippdrv_start(struct device *dev, enum drm_exynos_ipp_cmd cmd) return 0; }
+static struct rot_limit_table rot_limit_tbl_4210 = {
- .ycbcr420_2p = {
.min_w = 32,.min_h = 32,.max_w = SZ_64K,.max_h = SZ_64K,.align = 3,- },
- .rgb888 = {
.min_w = 8,.min_h = 8,.max_w = SZ_16K,.max_h = SZ_16K,.align = 2,- },
+};
+static struct rot_limit_table rot_limit_tbl_4x12 = {
- .ycbcr420_2p = {
.min_w = 32,.min_h = 32,.max_w = SZ_32K,.max_h = SZ_32K,.align = 3,- },
- .rgb888 = {
.min_w = 8,.min_h = 8,.max_w = SZ_8K,.max_h = SZ_8K,.align = 2,- },
+};
+static struct rot_limit_table rot_limit_tbl_5250 = {
- .ycbcr420_2p = {
.min_w = 32,.min_h = 32,.max_w = SZ_32K,.max_h = SZ_32K,.align = 3,- },
- .rgb888 = {
.min_w = 8,.min_h = 8,.max_w = SZ_8K,.max_h = SZ_8K,.align = 1,- },
+};
+static const struct of_device_id exynos_rotator_match[] = {
- {
.compatible = "samsung,exynos4210-rotator",.data = &rot_limit_tbl_4210,- },
- {
.compatible = "samsung,exynos4212-rotator",.data = &rot_limit_tbl_4x12,- },
- {
.compatible = "samsung,exynos5250-rotator",.data = &rot_limit_tbl_5250,- },
- {},
+};
static int rotator_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device *pdev) return -ENOMEM; }
- rot->limit_tbl = (struct rot_limit_table *)
platform_get_device_id(pdev)->driver_data;
- if (dev->of_node) {
const struct of_device_id *match;match = of_match_node(of_match_ptr(exynos_rotator_match),dev->of_node);if (match == NULL) {dev_err(dev, "failed to match node\n");return -ENODEV;}rot->limit_tbl = (struct rot_limit_table *)match->data;- } else {
dev_err(dev, "cannot find binding\n");
What about having a check for !dev->of_node at the beginning of probe, to not complicate further code?
Also the error message is confusing. It should be something closer to "device does not have of_node".
return -ENODEV;}
rot->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); rot->regs = devm_ioremap_resource(dev, rot->regs_res);
@@ -718,31 +796,6 @@ static int rotator_remove(struct platform_device *pdev) return 0; }
-static struct rot_limit_table rot_limit_tbl = {
- .ycbcr420_2p = {
.min_w = 32,.min_h = 32,.max_w = SZ_32K,.max_h = SZ_32K,.align = 3,- },
- .rgb888 = {
.min_w = 8,.min_h = 8,.max_w = SZ_8K,.max_h = SZ_8K,.align = 2,- },
-};
-static struct platform_device_id rotator_driver_ids[] = {
- {
.name = "exynos-rot",.driver_data = (unsigned long)&rot_limit_tbl,- },
- {},
-};
static int rotator_clk_crtl(struct rot_context *rot, bool enable) { if (enable) { @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = { struct platform_driver rotator_driver = { .probe = rotator_probe, .remove = rotator_remove,
- .id_table = rotator_driver_ids, .driver = { .name = "exynos-rot", .owner = THIS_MODULE, .pm = &rotator_pm_ops,
.of_match_table = of_match_ptr(exynos_rotator_match),};
Otherwise looks fine.
One more thing is that IMHO patch 5/5 could be squashed with this one, so documentation for the binding would be available at the same it is introduced.
Best regards, Tomasz
Hi Tomasz,
-----Original Message----- From: Tomasz Figa [mailto:t.figa@samsung.com] Sent: Friday, August 09, 2013 9:51 PM To: Chanho Park Cc: inki.dae@samsung.com; kgene.kim@samsung.com; dri- devel@lists.freedesktop.org; kyungmin.park@samsung.com; mark.rutland@arm.com; l.stach@pengutronix.de; s.nawrocki@samsung.com; tomasz.figa@gmail.com; linux-samsung-soc@vger.kernel.org; linux-arm- kernel@lists.infradead.org; devicetree@vger.kernel.org Subject: Re: [PATCHv2 1/5] drm/exynos: add device tree support for rotator
Hi Chanho,
On Friday 09 of August 2013 16:40:49 Chanho Park wrote:
The exynos4 platform is only dt-based since 3.10, we should convert driver data and ids to dt-based parsing methods. The rotator driver has a limit table to get size limit of input picture. Each SoCs has slightly different limit value compared with any others. For example, exynos4210's max_size of RGB888 is 16k x 16k. But, others have 8k x 8k. Another example the exynos5250 should have multiple of 2 pixel size for its X/Y axis. Thus, we should keep different tables for each of them.
Signed-off-by: Chanho Park chanho61.park@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
drivers/gpu/drm/exynos/exynos_drm_rotator.c | 109 ++++++++++++++++++++------- 1 file changed, 81 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_rotator.c b/drivers/gpu/drm/exynos/exynos_drm_rotator.c index 427640a..39b09e0 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_rotator.c +++ b/drivers/gpu/drm/exynos/exynos_drm_rotator.c @@ -632,6 +632,73 @@ static int rotator_ippdrv_start(struct device *dev, enum drm_exynos_ipp_cmd cmd) return 0; }
+static struct rot_limit_table rot_limit_tbl_4210 = {
- .ycbcr420_2p = {
.min_w = 32,.min_h = 32,.max_w = SZ_64K,.max_h = SZ_64K,.align = 3,- },
- .rgb888 = {
.min_w = 8,.min_h = 8,.max_w = SZ_16K,.max_h = SZ_16K,.align = 2,- },
+};
+static struct rot_limit_table rot_limit_tbl_4x12 = {
- .ycbcr420_2p = {
.min_w = 32,.min_h = 32,.max_w = SZ_32K,.max_h = SZ_32K,.align = 3,- },
- .rgb888 = {
.min_w = 8,.min_h = 8,.max_w = SZ_8K,.max_h = SZ_8K,.align = 2,- },
+};
+static struct rot_limit_table rot_limit_tbl_5250 = {
- .ycbcr420_2p = {
.min_w = 32,.min_h = 32,.max_w = SZ_32K,.max_h = SZ_32K,.align = 3,- },
- .rgb888 = {
.min_w = 8,.min_h = 8,.max_w = SZ_8K,.max_h = SZ_8K,.align = 1,- },
+};
+static const struct of_device_id exynos_rotator_match[] = {
- {
.compatible = "samsung,exynos4210-rotator",.data = &rot_limit_tbl_4210,- },
- {
.compatible = "samsung,exynos4212-rotator",.data = &rot_limit_tbl_4x12,- },
- {
.compatible = "samsung,exynos5250-rotator",.data = &rot_limit_tbl_5250,- },
- {},
+};
static int rotator_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -645,8 +712,19 @@ static int rotator_probe(struct platform_device *pdev) return -ENOMEM; }
- rot->limit_tbl = (struct rot_limit_table *)
platform_get_device_id(pdev)->driver_data;
- if (dev->of_node) {
const struct of_device_id *match;match = of_match_node(of_match_ptr(exynos_rotator_match),dev->of_node);if (match == NULL) {dev_err(dev, "failed to match node\n");return -ENODEV;}rot->limit_tbl = (struct rot_limit_table *)match->data;- } else {
dev_err(dev, "cannot find binding\n");What about having a check for !dev->of_node at the beginning of probe, to not complicate further code?
I agree with your comment. I'll move it at the beginning.
Also the error message is confusing. It should be something closer to "device does not have of_node".
I'll change it to avoid confusing. Thanks.
return -ENODEV;}
rot->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); rot->regs = devm_ioremap_resource(dev, rot->regs_res); @@ -718,31
+796,6 @@ static int rotator_remove(struct platform_device *pdev) return 0; }
-static struct rot_limit_table rot_limit_tbl = {
- .ycbcr420_2p = {
.min_w = 32,.min_h = 32,.max_w = SZ_32K,.max_h = SZ_32K,.align = 3,- },
- .rgb888 = {
.min_w = 8,.min_h = 8,.max_w = SZ_8K,.max_h = SZ_8K,.align = 2,- },
-};
-static struct platform_device_id rotator_driver_ids[] = {
- {
.name = "exynos-rot",.driver_data = (unsigned long)&rot_limit_tbl,- },
- {},
-};
static int rotator_clk_crtl(struct rot_context *rot, bool enable) { if (enable) { @@ -804,10 +857,10 @@ static const struct dev_pm_ops rotator_pm_ops = { struct platform_driver rotator_driver = { .probe = rotator_probe, .remove = rotator_remove,
- .id_table = rotator_driver_ids, .driver = { .name = "exynos-rot", .owner = THIS_MODULE, .pm = &rotator_pm_ops,
.of_match_table = of_match_ptr(exynos_rotator_match),};
Otherwise looks fine.
One more thing is that IMHO patch 5/5 could be squashed with this one, so documentation for the binding would be available at the same it is introduced.
Yes. it seems to be better consolidating patch1 and 5. I'll apply it to next patch. Thanks.
Best regards, Tomasz
This patch adds a rotator node for exynos4210. The exynos4210 has different limitation of image size compared with later chips.
Signed-off-by: Chanho Park chanho61.park@samsung.com Cc: Thomas Abraham thomas.abraham@linaro.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- arch/arm/boot/dts/exynos4.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi index 597cfcf..002b2b8 100644 --- a/arch/arm/boot/dts/exynos4.dtsi +++ b/arch/arm/boot/dts/exynos4.dtsi @@ -243,6 +243,15 @@ status = "disabled"; };
+ rotator: rotator@12810000 { + compatible = "samsung,exynos4210-rotator"; + reg = <0x12810000 0x1000>; + interrupts = <0 83 0>; + clocks = <&clock 278>; + clock-names = "rotator"; + status = "disabled"; + }; + mfc: codec@13400000 { compatible = "samsung,mfc-v5"; reg = <0x13400000 0x10000>;
On Friday 09 of August 2013 16:40:50 Chanho Park wrote:
This patch adds a rotator node for exynos4210. The exynos4210 has different limitation of image size compared with later chips.
Signed-off-by: Chanho Park chanho61.park@samsung.com Cc: Thomas Abraham thomas.abraham@linaro.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
arch/arm/boot/dts/exynos4.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi index 597cfcf..002b2b8 100644 --- a/arch/arm/boot/dts/exynos4.dtsi +++ b/arch/arm/boot/dts/exynos4.dtsi @@ -243,6 +243,15 @@ status = "disabled"; };
- rotator: rotator@12810000 {
compatible = "samsung,exynos4210-rotator";reg = <0x12810000 0x1000>;interrupts = <0 83 0>;clocks = <&clock 278>;clock-names = "rotator";status = "disabled";
Does the rotator need any board-specific information to work? If no, it should not be disabled by default. Same comment goes to patch 4/5.
Best regards, Tomasz
Hi Tomasz,
-----Original Message----- From: Tomasz Figa [mailto:t.figa@samsung.com] Sent: Friday, August 09, 2013 9:57 PM To: Chanho Park Cc: inki.dae@samsung.com; kgene.kim@samsung.com; dri- devel@lists.freedesktop.org; kyungmin.park@samsung.com; mark.rutland@arm.com; l.stach@pengutronix.de; s.nawrocki@samsung.com; tomasz.figa@gmail.com; linux-samsung-soc@vger.kernel.org; linux-arm- kernel@lists.infradead.org; devicetree@vger.kernel.org; Thomas Abraham Subject: Re: [PATCHv2 2/5] ARM: dts: Add rotator node for exynos4210
On Friday 09 of August 2013 16:40:50 Chanho Park wrote:
This patch adds a rotator node for exynos4210. The exynos4210 has different limitation of image size compared with later chips.
Signed-off-by: Chanho Park chanho61.park@samsung.com Cc: Thomas Abraham thomas.abraham@linaro.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
arch/arm/boot/dts/exynos4.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4.dtsi b/arch/arm/boot/dts/exynos4.dtsi index 597cfcf..002b2b8 100644 --- a/arch/arm/boot/dts/exynos4.dtsi +++ b/arch/arm/boot/dts/exynos4.dtsi @@ -243,6 +243,15 @@ status = "disabled"; };
- rotator: rotator@12810000 {
compatible = "samsung,exynos4210-rotator";reg = <0x12810000 0x1000>;interrupts = <0 83 0>;clocks = <&clock 278>;clock-names = "rotator";status = "disabled";Does the rotator need any board-specific information to work? If no, it should not be disabled by default. Same comment goes to patch 4/5.
It does not have any board-specific information. I'll remove the status node. Thanks.
Best Regards, Chanho Park
This patch adds a rotator node for exynos4212 and 4412. These have different limitation of image size compared with the exynos4210. So, we should define new compatible to distinguish it from the exynos4210.
Signed-off-by: Chanho Park chanho61.park@samsung.com Cc: Thomas Abraham thomas.abraham@linaro.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- arch/arm/boot/dts/exynos4x12.dtsi | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi b/arch/arm/boot/dts/exynos4x12.dtsi index 954628c..7bae33f 100644 --- a/arch/arm/boot/dts/exynos4x12.dtsi +++ b/arch/arm/boot/dts/exynos4x12.dtsi @@ -176,4 +176,8 @@ }; }; }; + + rotator: rotator@12810000 { + compatible = "samsung,exynos4212-rotator"; + }; };
This patch adds a rotator node for exynos5250. It has different align value of image size compared with any other chips. So, we should define new compatible for the exynos5250.
Signed-off-by: Chanho Park chanho61.park@samsung.com Cc: Thomas Abraham thomas.abraham@linaro.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- arch/arm/boot/dts/exynos5250.dtsi | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi index 6f356ce..7ee2da8 100644 --- a/arch/arm/boot/dts/exynos5250.dtsi +++ b/arch/arm/boot/dts/exynos5250.dtsi @@ -570,6 +570,15 @@ }; };
+ rotator: rotator@11C00000 { + compatible = "samsung,exynos5250-rotator"; + reg = <0x11C00000 0x1000>; + interrupts = <0 84 0>; + clocks = <&clock 269>; + clock-names = "rotator"; + status = "disabled"; + }; + gsc_0: gsc@0x13e00000 { compatible = "samsung,exynos5-gsc"; reg = <0x13e00000 0x1000>;
This patch describes each nodes of rotator and specifies a example how to bind it.
Signed-off-by: Chanho Park chanho61.park@samsung.com Cc: Thomas Abraham thomas.abraham@linaro.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- .../devicetree/bindings/gpu/samsung-rotator.txt | 26 ++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt
diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt new file mode 100644 index 0000000..31ee7b5 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt @@ -0,0 +1,26 @@ +* Samsung Image Rotator + +Required properties: + - compatible : value should be following: + (a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210 + (b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412 + (c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250 + + - reg : Physical base address of the IP registers and length of memory + mapped region. + + - interrupts : Interrupt number of Rotator + + - clocks : Clock number described in Documentations/devicetree/binding/clock. + + - clock-names : Clock name. + +Example: + rotator: rotator@12800000 { + compatible = "samsung,exynos4210-rotator"; + reg = <0x12810000 0x1000>; + interrupts = <0 83 0>; + clocks = <&clock 278>; + clock-names = "rotator"; + status = "disabled"; + };
Hi Chanho,
On 9 August 2013 13:10, Chanho Park chanho61.park@samsung.com wrote:
This patch describes each nodes of rotator and specifies a example how to bind it.
Signed-off-by: Chanho Park chanho61.park@samsung.com Cc: Thomas Abraham thomas.abraham@linaro.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
.../devicetree/bindings/gpu/samsung-rotator.txt | 26 ++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt
diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt new file mode 100644 index 0000000..31ee7b5 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt @@ -0,0 +1,26 @@ +* Samsung Image Rotator
+Required properties:
- compatible : value should be following:
This should be "should be one of the following:"
(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210(b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
- reg : Physical base address of the IP registers and length of memory
mapped region.
- interrupts : Interrupt number of Rotator
- clocks : Clock number described in Documentations/devicetree/binding/clock.
The path has several typos. Instead you can simply mention clocks : from common clock binding: handle to rotator clock.
Hi Sachin,
-----Original Message----- From: Sachin Kamat [mailto:sachin.kamat@linaro.org] Sent: Friday, August 09, 2013 6:40 PM To: Chanho Park Cc: inki.dae@samsung.com; kgene.kim@samsung.com; dri- devel@lists.freedesktop.org; kyungmin.park@samsung.com; mark.rutland@arm.com; l.stach@pengutronix.de; s.nawrocki@samsung.com; tomasz.figa@gmail.com; linux-samsung-soc@vger.kernel.org; linux-arm- kernel@lists.infradead.org; devicetree@vger.kernel.org; Thomas Abraham Subject: Re: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator
Hi Chanho,
On 9 August 2013 13:10, Chanho Park chanho61.park@samsung.com wrote:
This patch describes each nodes of rotator and specifies a example how to bind it.
Signed-off-by: Chanho Park chanho61.park@samsung.com Cc: Thomas Abraham thomas.abraham@linaro.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
.../devicetree/bindings/gpu/samsung-rotator.txt | 26
++++++++++++++++++++
1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt
diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt new file mode 100644 index 0000000..31ee7b5 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt @@ -0,0 +1,26 @@ +* Samsung Image Rotator
+Required properties:
- compatible : value should be following:
This should be "should be one of the following:"
I left out while I changed it to select multiple choices. Thanks.
(a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210(b) "samsung,exynos4212-rotator" for Rotator IP inExynos4212/4412
(c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
- reg : Physical base address of the IP registers and length of
memory
mapped region.
- interrupts : Interrupt number of Rotator
- clocks : Clock number described in
Documentations/devicetree/binding/clock.
The path has several typos. Instead you can simply mention clocks : from common clock binding: handle to rotator clock.
Oh, it has many typos. I'll change it. Thanks for your review.
Best Regards, Chanho Park
Hi Chanho,
On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
This patch describes each nodes of rotator and specifies a example how to bind it.
Signed-off-by: Chanho Park chanho61.park@samsung.com Cc: Thomas Abraham thomas.abraham@linaro.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
.../devicetree/bindings/gpu/samsung-rotator.txt | 26 ++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt
diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt new file mode 100644 index 0000000..31ee7b5 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt @@ -0,0 +1,26 @@ +* Samsung Image Rotator
+Required properties:
- compatible : value should be following:
- (a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
- (b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
- (c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
- reg : Physical base address of the IP registers and length of memory
mapped region.
- interrupts : Interrupt number of Rotator
What about: interrupt specifier for rotator interrupt, according to format specific to interrupt parent.
- clocks : Clock number described in
Documentations/devicetree/binding/clock. +
Perhaps: clock specifier for rotator clock, according to generic clock bindings.
- clock-names : Clock name.
Names of clocks specified in "clocks" property. For exynos rotator this property should contain only one name: "rotator".
+Example:
- rotator: rotator@12800000 {
I wonder if we shouldn't use a more generic name here, according to ePAPR node naming recommendation.
compatible = "samsung,exynos4210-rotator";reg = <0x12810000 0x1000>;
Typo. Node has 12800000 in unit-address suffix, but this property has 12810000 instead.
interrupts = <0 83 0>;clocks = <&clock 278>;clock-names = "rotator";status = "disabled";
Status property should be omitted from this example, as it's not a part of this binding.
Best regards, Tomasz
On 08/09/2013 07:15 AM, Tomasz Figa wrote:
Hi Chanho,
On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
This patch describes each nodes of rotator and specifies a example how to bind it.
diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
+* Samsung Image Rotator
+Required properties:
- compatible : value should be following:
- (a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
- (b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
- (c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
Is "rotator" the name that the HW documentation uses to refer to this block? If so, those compatible values seem fine. If not, they seem slightly too generic for me; perhaps better to use the raw HW block name?
On Friday 09 of August 2013 10:38:30 Stephen Warren wrote:
On 08/09/2013 07:15 AM, Tomasz Figa wrote:
Hi Chanho,
On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
This patch describes each nodes of rotator and specifies a example how to bind it.
diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt
+* Samsung Image Rotator
+Required properties:
- compatible : value should be following:
- (a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
- (b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
- (c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
Is "rotator" the name that the HW documentation uses to refer to this block?
Yes, it is. More specifically it is referred to as "Image Rotator".
Best regards, Tomasz
If so, those compatible values seem fine. If not, they seem slightly too generic for me; perhaps better to use the raw HW block name?
Hi Tomasz,
-----Original Message----- From: Tomasz Figa [mailto:t.figa@samsung.com] Sent: Friday, August 09, 2013 10:16 PM To: Chanho Park Cc: inki.dae@samsung.com; kgene.kim@samsung.com; dri- devel@lists.freedesktop.org; kyungmin.park@samsung.com; mark.rutland@arm.com; l.stach@pengutronix.de; s.nawrocki@samsung.com; tomasz.figa@gmail.com; linux-samsung-soc@vger.kernel.org; linux-arm- kernel@lists.infradead.org; devicetree@vger.kernel.org; Thomas Abraham; swarren@wwwdotorg.org; ian.campbell@citrix.com; rob.herring@calxeda.com; pawel.moll@arm.com Subject: Re: [PATCHv2 5/5] ARM: dts: Add dt binding documentation for exynos rotator
Hi Chanho,
On Friday 09 of August 2013 16:40:53 Chanho Park wrote:
This patch describes each nodes of rotator and specifies a example how to bind it.
Signed-off-by: Chanho Park chanho61.park@samsung.com Cc: Thomas Abraham thomas.abraham@linaro.org Cc: Kukjin Kim kgene.kim@samsung.com Cc: Inki Dae inki.dae@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com
.../devicetree/bindings/gpu/samsung-rotator.txt | 26 ++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpu/samsung-rotator.txt
diff --git a/Documentation/devicetree/bindings/gpu/samsung-rotator.txt b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt new file mode 100644 index 0000000..31ee7b5 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/samsung-rotator.txt @@ -0,0 +1,26 @@ +* Samsung Image Rotator
+Required properties:
- compatible : value should be following:
- (a) "samsung,exynos4210-rotator" for Rotator IP in Exynos4210
- (b) "samsung,exynos4212-rotator" for Rotator IP in Exynos4212/4412
- (c) "samsung,exynos5250-rotator" for Rotator IP in Exynos5250
- reg : Physical base address of the IP registers and length of
memory
mapped region.
- interrupts : Interrupt number of Rotator
What about: interrupt specifier for rotator interrupt, according to format specific to interrupt parent.
- clocks : Clock number described in
Documentations/devicetree/binding/clock. +
Perhaps: clock specifier for rotator clock, according to generic clock bindings.
Become more clearly. Thanks.
- clock-names : Clock name.
Names of clocks specified in "clocks" property. For exynos rotator this property should contain only one name: "rotator".
+Example:
- rotator: rotator@12800000 {
I wonder if we shouldn't use a more generic name here, according to ePAPR node naming recommendation.
Hmm. I'm not sure what name is better than rotator. How about
image-rotator@12810000
compatible = "samsung,exynos4210-rotator";reg = <0x12810000 0x1000>;Typo. Node has 12800000 in unit-address suffix, but this property has 12810000 instead.
Yes. It's typo. I'll correct it now.
interrupts = <0 83 0>;clocks = <&clock 278>;clock-names = "rotator";status = "disabled";Status property should be omitted from this example, as it's not a part of this binding.
I'll remove the property. Thanks.
Best Regards, Chanho Park
dri-devel@lists.freedesktop.org
-
Chanho Park -
Chanho Park -
Sachin Kamat -
Stephen Warren -
Tomasz Figa