[PATCH v2 1/3] media: dt-bindings: samsung,exynos4212-fimc-is: replace duplicate pmu node with phandle

[PATCH v2 1/3] media: dt-bindings: samsung,exynos4212-fimc-is: replace duplicate pmu node with phandle

[Krzysztof Kozlowski]

The FIMC IS camera must access the PMU (Power Management Unit) IO memory
to control camera power.  This was achieved by duplicating the PMU node
as its child like:

  soc@0 {
    system-controller@10020000 { ... }; // Real PMU

    camera@11800000 {
      fimc-is@12000000 {
        // FIMC IS camera node
        pmu@10020000 {
          reg = <0x10020000 0x3000>; // Fake PMU node
        };
      };
    };
  };

This is not a correct representation of the hardware.  Mapping the PMU
(Power Management Unit) IO memory should be via syscon-like phandle
(samsung,pmu-syscon, already used for other drivers), not by duplicating
"pmu" Devicetree node inside the FIMC IS.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Changes in v2:
1. Add Rb tag.
---
 .../media/samsung,exynos4212-fimc-is.yaml         | 15 ++++++++++-----
 .../devicetree/bindings/media/samsung,fimc.yaml   |  5 +----
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/samsung,exynos4212-fimc-is.yaml b/Documentation/devicetree/bindings/media/samsung,exynos4212-fimc-is.yaml
index 3691cd4962b2..3a5ff3f47060 100644
--- a/Documentation/devicetree/bindings/media/samsung,exynos4212-fimc-is.yaml
+++ b/Documentation/devicetree/bindings/media/samsung,exynos4212-fimc-is.yaml
@@ -75,13 +75,20 @@ properties:
   power-domains:
     maxItems: 1
 
+  samsung,pmu-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Power Management Unit (PMU) system controller interface, used to
+      power/start the ISP.
+
 patternProperties:
   "^pmu@[0-9a-f]+$":
     type: object
     additionalProperties: false
+    deprecated: true
     description:
       Node representing the SoC's Power Management Unit (duplicated with the
-      correct PMU node in the SoC).
+      correct PMU node in the SoC). Deprecated, use samsung,pmu-syscon.
 
     properties:
       reg:
@@ -131,6 +138,7 @@ required:
   - clock-names
   - interrupts
   - ranges
+  - samsung,pmu-syscon
   - '#size-cells'
 
 additionalProperties: false
@@ -179,15 +187,12 @@ examples:
                  <&sysmmu_fimc_fd>, <&sysmmu_fimc_mcuctl>;
         iommu-names = "isp", "drc", "fd", "mcuctl";
         power-domains = <&pd_isp>;
+        samsung,pmu-syscon = <&pmu_system_controller>;
 
         #address-cells = <1>;
         #size-cells = <1>;
         ranges;
 
-        pmu@10020000 {
-            reg = <0x10020000 0x3000>;
-        };
-
         i2c-isp@12140000 {
             compatible = "samsung,exynos4212-i2c-isp";
             reg = <0x12140000 0x100>;
diff --git a/Documentation/devicetree/bindings/media/samsung,fimc.yaml b/Documentation/devicetree/bindings/media/samsung,fimc.yaml
index 79ff6d83a9fd..530a08f5d3fe 100644
--- a/Documentation/devicetree/bindings/media/samsung,fimc.yaml
+++ b/Documentation/devicetree/bindings/media/samsung,fimc.yaml
@@ -236,15 +236,12 @@ examples:
                      <&sysmmu_fimc_fd>, <&sysmmu_fimc_mcuctl>;
             iommu-names = "isp", "drc", "fd", "mcuctl";
             power-domains = <&pd_isp>;
+            samsung,pmu-syscon = <&pmu_system_controller>;
 
             #address-cells = <1>;
             #size-cells = <1>;
             ranges;
 
-            pmu@10020000 {
-                reg = <0x10020000 0x3000>;
-            };
-
             i2c-isp@12140000 {
                 compatible = "samsung,exynos4212-i2c-isp";
                 reg = <0x12140000 0x100>;
-- 
2.34.1

[PATCH v2 2/3] media: dt-bindings: samsung,fimc: correct unit addresses in DTS example

[Krzysztof Kozlowski]

The camera node's ranges property and unit addresses of its children
were not correct.  If camera is @11800000, then its fimc child is @0.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Changes in v2:
1. Add Ab tag.
---
 .../bindings/media/samsung,fimc.yaml          | 22 +++++++++----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/samsung,fimc.yaml b/Documentation/devicetree/bindings/media/samsung,fimc.yaml
index 530a08f5d3fe..88b176d594e2 100644
--- a/Documentation/devicetree/bindings/media/samsung,fimc.yaml
+++ b/Documentation/devicetree/bindings/media/samsung,fimc.yaml
@@ -117,7 +117,7 @@ examples:
         #clock-cells = <1>;
         #address-cells = <1>;
         #size-cells = <1>;
-        ranges = <0x0 0x0 0x18000000>;
+        ranges = <0x0 0x0 0xba1000>;
 
         clocks = <&clock CLK_SCLK_CAM0>, <&clock CLK_SCLK_CAM1>,
                  <&clock CLK_PIXELASYNCM0>, <&clock CLK_PIXELASYNCM1>;
@@ -132,9 +132,9 @@ examples:
         pinctrl-0 = <&cam_port_a_clk_active &cam_port_b_clk_active>;
         pinctrl-names = "default";
 
-        fimc@11800000 {
+        fimc@0 {
             compatible = "samsung,exynos4212-fimc";
-            reg = <0x11800000 0x1000>;
+            reg = <0x00000000 0x1000>;
             interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
             clocks = <&clock CLK_FIMC0>,
                      <&clock CLK_SCLK_FIMC0>;
@@ -151,9 +151,9 @@ examples:
 
         /* ... FIMC 1-3 */
 
-        csis@11880000 {
+        csis@80000 {
             compatible = "samsung,exynos4210-csis";
-            reg = <0x11880000 0x4000>;
+            reg = <0x00080000 0x4000>;
             interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
             clocks = <&clock CLK_CSIS0>,
                      <&clock CLK_SCLK_CSIS0>;
@@ -186,9 +186,9 @@ examples:
 
         /* ... CSIS 1 */
 
-        fimc-lite@12390000 {
+        fimc-lite@b90000 {
               compatible = "samsung,exynos4212-fimc-lite";
-              reg = <0x12390000 0x1000>;
+              reg = <0xb90000 0x1000>;
               interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
               power-domains = <&pd_isp>;
               clocks = <&isp_clock CLK_ISP_FIMC_LITE0>;
@@ -198,9 +198,9 @@ examples:
 
         /* ... FIMC-LITE 1 */
 
-        fimc-is@12000000 {
+        fimc-is@800000 {
             compatible = "samsung,exynos4212-fimc-is";
-            reg = <0x12000000 0x260000>;
+            reg = <0x00800000 0x260000>;
             interrupts = <GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>,
                          <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>;
             clocks = <&isp_clock CLK_ISP_FIMC_LITE0>,
@@ -242,9 +242,9 @@ examples:
             #size-cells = <1>;
             ranges;
 
-            i2c-isp@12140000 {
+            i2c-isp@940000 {
                 compatible = "samsung,exynos4212-i2c-isp";
-                reg = <0x12140000 0x100>;
+                reg = <0x00940000 0x100>;
                 clocks = <&isp_clock CLK_ISP_I2C1_ISP>;
                 clock-names = "i2c_isp";
                 pinctrl-0 = <&fimc_is_i2c1>;
-- 
2.34.1

[PATCH v2 3/3] media: exynos4-is: fimc-is: replace duplicate pmu node with phandle

[Krzysztof Kozlowski]

Devicetree for the FIMC IS camera included duplicated PMU node as its
child like:

  soc@0 {
    system-controller@10020000 { ... }; // Real PMU

    camera@11800000 {
      fimc-is@12000000 {
        // FIMC IS camera node
        pmu@10020000 {
          reg = <0x10020000 0x3000>; // Fake PMU node
        };
      };
    };
  };

This is not a correct representation of the hardware.  Mapping the PMU
(Power Management Unit) IO memory should be via syscon-like phandle
(samsung,pmu-syscon, already used for other drivers), not by duplicating
"pmu" Devicetree node inside the FIMC IS.  Backward compatibility is
preserved.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Changes in v2:
1. Use IOMEM_ERR_PTR (Hans)
---
 .../platform/samsung/exynos4-is/fimc-is.c     | 33 ++++++++++++++-----
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/samsung/exynos4-is/fimc-is.c b/drivers/media/platform/samsung/exynos4-is/fimc-is.c
index 530a148fe4d3..c995b1226ca3 100644
--- a/drivers/media/platform/samsung/exynos4-is/fimc-is.c
+++ b/drivers/media/platform/samsung/exynos4-is/fimc-is.c
@@ -767,12 +767,32 @@ static void fimc_is_debugfs_create(struct fimc_is *is)
 static int fimc_is_runtime_resume(struct device *dev);
 static int fimc_is_runtime_suspend(struct device *dev);
 
+static void __iomem *fimc_is_get_pmu_regs(struct device *dev)
+{
+	struct device_node *node;
+	void __iomem *regs;
+
+	node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0);
+	if (!node) {
+		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
+		node = of_get_child_by_name(dev->of_node, "pmu");
+		if (!node)
+			return IOMEM_ERR_PTR(-ENODEV);
+	}
+
+	regs = of_iomap(node, 0);
+	of_node_put(node);
+	if (!regs)
+		return IOMEM_ERR_PTR(-ENOMEM);
+
+	return regs;
+}
+
 static int fimc_is_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct fimc_is *is;
 	struct resource res;
-	struct device_node *node;
 	int ret;
 
 	is = devm_kzalloc(&pdev->dev, sizeof(*is), GFP_KERNEL);
@@ -794,14 +814,9 @@ static int fimc_is_probe(struct platform_device *pdev)
 	if (IS_ERR(is->regs))
 		return PTR_ERR(is->regs);
 
-	node = of_get_child_by_name(dev->of_node, "pmu");
-	if (!node)
-		return -ENODEV;
-
-	is->pmu_regs = of_iomap(node, 0);
-	of_node_put(node);
-	if (!is->pmu_regs)
-		return -ENOMEM;
+	is->pmu_regs = fimc_is_get_pmu_regs(dev);
+	if (IS_ERR(is->pmu_regs))
+		return PTR_ERR(is->pmu_regs);
 
 	is->irq = irq_of_parse_and_map(dev->of_node, 0);
 	if (!is->irq) {
-- 
2.34.1

Re: [PATCH v2 3/3] media: exynos4-is: fimc-is: replace duplicate pmu node with phandle

[Andi Shyti]

Hi Krzysztof,

[...]

> +static void __iomem *fimc_is_get_pmu_regs(struct device *dev)
> +{
> +	struct device_node *node;
> +	void __iomem *regs;
> +
> +	node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0);
> +	if (!node) {
> +		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
> +		node = of_get_child_by_name(dev->of_node, "pmu");
> +		if (!node)
> +			return IOMEM_ERR_PTR(-ENODEV);

in my opinion this should be:

		...
		if (!node)
			return IOMEM_ERR_PTR(-ENODEV);

		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");

Because if you don't have both "samsung,pmu-syscon and "pmu" then
the warning should not be printed and you need to return -ENODEV.

... and... "*please* update your DTB", the user might get upset
and out of sheer spite, decides not to do it – just because! :)

Andi

> +	}

Re: [PATCH v2 3/3] media: exynos4-is: fimc-is: replace duplicate pmu node with phandle

[Krzysztof Kozlowski]

On 08/08/2023 01:13, Andi Shyti wrote:
> Hi Krzysztof,
> 
> [...]
> 
>> +static void __iomem *fimc_is_get_pmu_regs(struct device *dev)
>> +{
>> +	struct device_node *node;
>> +	void __iomem *regs;
>> +
>> +	node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0);
>> +	if (!node) {
>> +		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
>> +		node = of_get_child_by_name(dev->of_node, "pmu");
>> +		if (!node)
>> +			return IOMEM_ERR_PTR(-ENODEV);
> 
> in my opinion this should be:
> 
> 		...
> 		if (!node)
> 			return IOMEM_ERR_PTR(-ENODEV);
> 
> 		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
> 
> Because if you don't have both "samsung,pmu-syscon and "pmu" then
> the warning should not be printed and you need to return -ENODEV.

Why not? Warning is correct - the driver is trying to find, thus
continuous tense "Finding", PMU node via old method.

> 
> ... and... "*please* update your DTB", the user might get upset
> and out of sheer spite, decides not to do it – just because! :)

The message is already long enough, why making it longer? Anyone who
ships DTS outside of Linux kernel is doomed anyway...

Best regards,
Krzysztof

Re: [PATCH v2 3/3] media: exynos4-is: fimc-is: replace duplicate pmu node with phandle

[Andi Shyti]

> >> +static void __iomem *fimc_is_get_pmu_regs(struct device *dev)
> >> +{
> >> +	struct device_node *node;
> >> +	void __iomem *regs;
> >> +
> >> +	node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0);
> >> +	if (!node) {
> >> +		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
> >> +		node = of_get_child_by_name(dev->of_node, "pmu");
> >> +		if (!node)
> >> +			return IOMEM_ERR_PTR(-ENODEV);
> > 
> > in my opinion this should be:
> > 
> > 		...
> > 		if (!node)
> > 			return IOMEM_ERR_PTR(-ENODEV);
> > 
> > 		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
> > 
> > Because if you don't have both "samsung,pmu-syscon and "pmu" then
> > the warning should not be printed and you need to return -ENODEV.
> 
> Why not? Warning is correct - the driver is trying to find, thus
> continuous tense "Finding", PMU node via old method.

Alright, I'll go along with what you're suggesting, but I have to
say, I find it misleading.

From what I understand, you're requesting an update to the dtb
because it's using deprecated methods. However, the reality might 
be that the node is not present in any method at all.

Your statement would be accurate if you failed to find the
previous method but then did end up finding it.

Relying on the present continuous tense for clarity is a bold
move, don't you think? :)

Andi

> > ... and... "*please* update your DTB", the user might get upset
> > and out of sheer spite, decides not to do it – just because! :)
> 
> The message is already long enough, why making it longer? Anyone who
> ships DTS outside of Linux kernel is doomed anyway...
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 3/3] media: exynos4-is: fimc-is: replace duplicate pmu node with phandle

[Krzysztof Kozlowski]

On 08/08/2023 13:42, Andi Shyti wrote:
>>>> +static void __iomem *fimc_is_get_pmu_regs(struct device *dev)
>>>> +{
>>>> +	struct device_node *node;
>>>> +	void __iomem *regs;
>>>> +
>>>> +	node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0);
>>>> +	if (!node) {
>>>> +		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
>>>> +		node = of_get_child_by_name(dev->of_node, "pmu");
>>>> +		if (!node)
>>>> +			return IOMEM_ERR_PTR(-ENODEV);
>>>
>>> in my opinion this should be:
>>>
>>> 		...
>>> 		if (!node)
>>> 			return IOMEM_ERR_PTR(-ENODEV);
>>>
>>> 		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
>>>
>>> Because if you don't have both "samsung,pmu-syscon and "pmu" then
>>> the warning should not be printed and you need to return -ENODEV.
>>
>> Why not? Warning is correct - the driver is trying to find, thus
>> continuous tense "Finding", PMU node via old method.
> 
> Alright, I'll go along with what you're suggesting, but I have to
> say, I find it misleading.
> 
> From what I understand, you're requesting an update to the dtb
> because it's using deprecated methods. However, the reality might 
> be that the node is not present in any method at all.
> 
> Your statement would be accurate if you failed to find the
> previous method but then did end up finding it.
> 
> Relying on the present continuous tense for clarity is a bold
> move, don't you think? :)

I just don't think it matters and is not worth resending.

Best regards,
Krzysztof

Re: [PATCH v2 3/3] media: exynos4-is: fimc-is: replace duplicate pmu node with phandle

[Hans Verkuil]

Hi Krzysztof,

On 08/08/2023 01:13, Andi Shyti wrote:
> Hi Krzysztof,
> 
> [...]
> 
>> +static void __iomem *fimc_is_get_pmu_regs(struct device *dev)
>> +{
>> +	struct device_node *node;
>> +	void __iomem *regs;
>> +
>> +	node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0);
>> +	if (!node) {
>> +		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
>> +		node = of_get_child_by_name(dev->of_node, "pmu");
>> +		if (!node)
>> +			return IOMEM_ERR_PTR(-ENODEV);
> 
> in my opinion this should be:
> 
> 		...
> 		if (!node)
> 			return IOMEM_ERR_PTR(-ENODEV);
> 
> 		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
> 
> Because if you don't have both "samsung,pmu-syscon and "pmu" then
> the warning should not be printed and you need to return -ENODEV.

I agree with Andi for this part.

The only time you want to see this message is if samsung,pmu-syscon is
missing AND pmu is present. If both are missing, then just return ENODEV as
it was before.

> 
> ... and... "*please* update your DTB", the user might get upset
> and out of sheer spite, decides not to do it – just because! :)

I don't care about this bit. I guess it doesn't hurt to add 'please', but
I accept it either way.

Regards,

	Hans

> 
> Andi
> 
>> +	}

Re: [PATCH v2 3/3] media: exynos4-is: fimc-is: replace duplicate pmu node with phandle

[Krzysztof Kozlowski]

On 11/08/2023 11:49, Hans Verkuil wrote:
> Hi Krzysztof,
> 
> On 08/08/2023 01:13, Andi Shyti wrote:
>> Hi Krzysztof,
>>
>> [...]
>>
>>> +static void __iomem *fimc_is_get_pmu_regs(struct device *dev)
>>> +{
>>> +	struct device_node *node;
>>> +	void __iomem *regs;
>>> +
>>> +	node = of_parse_phandle(dev->of_node, "samsung,pmu-syscon", 0);
>>> +	if (!node) {
>>> +		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
>>> +		node = of_get_child_by_name(dev->of_node, "pmu");
>>> +		if (!node)
>>> +			return IOMEM_ERR_PTR(-ENODEV);
>>
>> in my opinion this should be:
>>
>> 		...
>> 		if (!node)
>> 			return IOMEM_ERR_PTR(-ENODEV);
>>
>> 		dev_warn(dev, "Finding PMU node via deprecated method, update your DTB\n");
>>
>> Because if you don't have both "samsung,pmu-syscon and "pmu" then
>> the warning should not be printed and you need to return -ENODEV.
> 
> I agree with Andi for this part.
> 
> The only time you want to see this message is if samsung,pmu-syscon is
> missing AND pmu is present. If both are missing, then just return ENODEV as
> it was before.

OK, understood. I will send a v3.

Best regards,
Krzysztof